Add support to GCMKeyStore for multiple keys per app_id

components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc

Index: components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc
diff --git a/components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc b/components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc
index 2b2d06932683896692edec17e6ee65ca7ab9b6a0..fd679bd4c73e2a171976c8ea8d0df3793dbfda93 100644
--- a/components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc
+++ b/components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc
@@ -11,6 +11,7 @@
 
 #include "base/base64url.h"
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -29,6 +30,7 @@ namespace gcm {
 namespace {
 
 const char kExampleAppId[] = "my-app-id";
+const char kExampleAuthorizedEntity[] = "my-sender-id";
 const char kExampleMessage[] = "Hello, world, this is the GCM Driver!";
 
 const char kValidEncryptionHeader[] =
@@ -65,15 +67,21 @@ class GCMEncryptionProviderTest : public ::testing::Test {
                             std::string* auth_secret_out,
                             const std::string& p256dh,
                             const std::string& auth_secret) {
-    *p256dh_out = p256dh;
-    *auth_secret_out = auth_secret;
+    if (p256dh_out)
+      *p256dh_out = p256dh;
+    if (auth_secret_out)
+      *auth_secret_out = auth_secret;
   }
 
-  // To be used as a callback for GCMKeyStore::CreateKeys().
-  void DidCreateKeys(KeyPair* pair_out, std::string* auth_secret_out,
-                     const KeyPair& pair, const std::string& auth_secret) {
-    *pair_out = pair;
-    *auth_secret_out = auth_secret;
+  // To be used as a callback for GCMKeyStore::{GetKeys,CreateKeys}.
+  void HandleKeysCallback(KeyPair* pair_out,
+                          std::string* auth_secret_out,
+                          const KeyPair& pair,
+                          const std::string& auth_secret) {
+    if (pair_out)
+      *pair_out = pair;
+    if (auth_secret_out)
+      *auth_secret_out = auth_secret;
   }
 
  protected:
@@ -89,6 +97,32 @@ class GCMEncryptionProviderTest : public ::testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
+  // Checks that the underlying key store has a key for the |kExampleAppId| +
+  // authorized entity pair if and only if |should_have_key| is true. Must wrap
+  // with ASSERT/EXPECT_NO_FATAL_FAILURE.
+  void CheckHasKey(const std::string& instance_id_authorized_entity,
+                   bool should_have_key) {
+    KeyPair pair;
+    std::string auth_secret;
+    encryption_provider()->key_store_->GetKeys(
+        kExampleAppId, instance_id_authorized_entity,
+        false /* fallback_to_empty_authorized_entity */,
+        base::Bind(&GCMEncryptionProviderTest::HandleKeysCallback,
+                   base::Unretained(this), &pair, &auth_secret));
+
+    base::RunLoop().RunUntilIdle();
+
+    if (should_have_key) {
+      ASSERT_GT(pair.public_key().size(), 0u);
+      ASSERT_GT(pair.private_key().size(), 0u);
+      ASSERT_GT(auth_secret.size(), 0u);
+    } else {
+      ASSERT_EQ(0u, pair.public_key().size());
+      ASSERT_EQ(0u, pair.private_key().size());
+      ASSERT_EQ(0u, auth_secret.size());
+    }
+  }
+
   // Returns the result of the previous decryption operation.
   GCMEncryptionProvider::DecryptionResult decryption_result() {
     return decryption_result_;
@@ -101,6 +135,12 @@ class GCMEncryptionProviderTest : public ::testing::Test {
     return encryption_provider_.get();
   }
 
+  // Performs a full round-trip test of the encryption feature. Must wrap this
+  // in ASSERT_NO_FATAL_FAILURE.
+  void TestEncryptionRoundTrip(
+      const std::string& app_id,
+      const std::string& instance_id_authorized_entity);
+
  private:
   void DidDecryptMessage(GCMEncryptionProvider::DecryptionResult result,
                          const IncomingMessage& message) {
@@ -207,7 +247,7 @@ TEST_F(GCMEncryptionProviderTest, VerifiesExistingKeys) {
 
   std::string public_key, auth_secret;
   encryption_provider()->GetEncryptionInfo(
-      kExampleAppId,
+      kExampleAppId, "" /* empty authorized entity for GCM registration */,
       base::Bind(&GCMEncryptionProviderTest::DidGetEncryptionInfo,
                  base::Unretained(this), &public_key, &auth_secret));
 
@@ -222,7 +262,170 @@ TEST_F(GCMEncryptionProviderTest, VerifiesExistingKeys) {
             decryption_result());
 }
 
-TEST_F(GCMEncryptionProviderTest, EncryptionRoundTrip) {
+TEST_F(GCMEncryptionProviderTest, VerifiesKeyRemovalGCMRegistration) {

[Peter Beverloo, 2016/05/09 14:10:10]

While I'm all for tests, this doesn't actually test the logic in the
GCMEncryptionProvider and has a significant amount of overlap with
GCMKeyStoreTest.RemoveKeysWildcardAuthorizedEntity. Could we merge?

[johnme, 2016/05/09 18:15:54]

This (and its twin, VerifiesKeyRemovalInstanceIDToken) is the only test coverage
of GCMEncryptionProvider::RemoveEncryptionInfo, which is the public API to the
crypto component. It's also the only test coverage of calling GetEncryptionInfo
for a key that already exists (as opposed to calling it in order to create a new
key). So it seems worth keeping, though I agree that the duplication is
unfortunate.

+  // Removing encryption info for an InstanceID token shouldn't affect a legacy
+  // GCM registration.
+
+  // Legacy GCM callers pass an empty string for instance_id_authorized_entity.
+  std::string instance_id_authorized_entity_gcm = "";
+  std::string instance_id_authorized_entity_1 =
+      kExampleAuthorizedEntity + std::string("1");
+  std::string instance_id_authorized_entity_2 =
+      kExampleAuthorizedEntity + std::string("2");
+
+  // Should create encryption info.
+  std::string public_key, auth_secret;
+  encryption_provider()->GetEncryptionInfo(
+      kExampleAppId, instance_id_authorized_entity_gcm,
+      base::Bind(&GCMEncryptionProviderTest::DidGetEncryptionInfo,
+                 base::Unretained(this), &public_key, &auth_secret));
+
+  // Should get encryption info created above (although these calls are async

[Peter Beverloo, 2016/05/09 14:10:09]

Instead of the comment in parenthesis, can we just place a RunUntilIdle() in
between for clarity?

[johnme, 2016/05/09 18:15:54]

Sure; instead I expanded GCMKeyStoreTest.CreateGetAndRemoveKeysSynchronously to
cover synchronous gets.

+  // with no RunUntilIdle between, they should get handled in order of posting).
+  std::string read_public_key, read_auth_secret;
+  encryption_provider()->GetEncryptionInfo(
+      kExampleAppId, instance_id_authorized_entity_gcm,
+      base::Bind(&GCMEncryptionProviderTest::DidGetEncryptionInfo,
+                 base::Unretained(this), &read_public_key, &read_auth_secret));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_GT(public_key.size(), 0u);
+  EXPECT_GT(auth_secret.size(), 0u);
+  EXPECT_EQ(public_key, read_public_key);
+  EXPECT_EQ(auth_secret, read_auth_secret);
+
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_gcm, true));

[Peter Beverloo, 2016/05/09 14:10:10]

This should be an ASSERT_NO_FATAL_FAILURE. There is little point in continuing
if any of the asserts in the CheckHasKey() methods failed.

[johnme, 2016/05/09 18:15:54]

Done (ditto below, since it's always the first failure that matters here).

+
+  encryption_provider()->RemoveEncryptionInfo(kExampleAppId,
+                                              instance_id_authorized_entity_1,
+                                              base::Bind(&base::DoNothing));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_gcm, true));
+
+  encryption_provider()->RemoveEncryptionInfo(kExampleAppId, "*",
+                                              base::Bind(&base::DoNothing));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_gcm, true));
+
+  encryption_provider()->RemoveEncryptionInfo(kExampleAppId,
+                                              instance_id_authorized_entity_gcm,
+                                              base::Bind(&base::DoNothing));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_NO_FATAL_FAILURE(
+      CheckHasKey(instance_id_authorized_entity_gcm, false));
+}
+
+TEST_F(GCMEncryptionProviderTest, VerifiesKeyRemovalInstanceIDToken) {
+  // Removing encryption info for a legacy GCM registration shouldn't affect an
+  // InstanceID token.
+
+  // Legacy GCM callers pass an empty string for instance_id_authorized_entity.
+  std::string instance_id_authorized_entity_gcm = "";
+  std::string instance_id_authorized_entity_1 =
+      kExampleAuthorizedEntity + std::string("1");
+  std::string instance_id_authorized_entity_2 =
+      kExampleAuthorizedEntity + std::string("2");
+
+  std::string public_key_1, auth_secret_1;
+  encryption_provider()->GetEncryptionInfo(
+      kExampleAppId, instance_id_authorized_entity_1,
+      base::Bind(&GCMEncryptionProviderTest::DidGetEncryptionInfo,
+                 base::Unretained(this), &public_key_1, &auth_secret_1));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_GT(public_key_1.size(), 0u);
+  EXPECT_GT(auth_secret_1.size(), 0u);
+
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_1, true));
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_2, false));
+
+  std::string public_key_2, auth_secret_2;
+  encryption_provider()->GetEncryptionInfo(
+      kExampleAppId, instance_id_authorized_entity_2,
+      base::Bind(&GCMEncryptionProviderTest::DidGetEncryptionInfo,
+                 base::Unretained(this), &public_key_2, &auth_secret_2));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_GT(public_key_2.size(), 0u);
+  EXPECT_GT(auth_secret_2.size(), 0u);
+  EXPECT_NE(public_key_1, public_key_2);
+  EXPECT_NE(auth_secret_1, auth_secret_2);
+
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_1, true));
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_2, true));
+
+  std::string read_public_key_1, read_auth_secret_1;
+  encryption_provider()->GetEncryptionInfo(
+      kExampleAppId, instance_id_authorized_entity_1,
+      base::Bind(&GCMEncryptionProviderTest::DidGetEncryptionInfo,
+                 base::Unretained(this), &read_public_key_1,
+                 &read_auth_secret_1));
+
+  base::RunLoop().RunUntilIdle();
+
+  // Should have returned existing info for instance_id_authorized_entity_1.
+  EXPECT_EQ(public_key_1, read_public_key_1);
+  EXPECT_EQ(auth_secret_1, read_auth_secret_1);
+
+  encryption_provider()->RemoveEncryptionInfo(kExampleAppId,
+                                              instance_id_authorized_entity_gcm,
+                                              base::Bind(&base::DoNothing));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_1, true));
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_2, true));
+
+  encryption_provider()->RemoveEncryptionInfo(kExampleAppId,
+                                              instance_id_authorized_entity_1,
+                                              base::Bind(&base::DoNothing));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_1, false));
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_2, true));
+
+  std::string public_key_1_refreshed, auth_secret_1_refreshed;
+  encryption_provider()->GetEncryptionInfo(
+      kExampleAppId, instance_id_authorized_entity_1,
+      base::Bind(&GCMEncryptionProviderTest::DidGetEncryptionInfo,
+                 base::Unretained(this), &public_key_1_refreshed,
+                 &auth_secret_1_refreshed));
+
+  base::RunLoop().RunUntilIdle();
+
+  // Since the info was removed, GetEncryptionInfo should have created new info.
+  EXPECT_GT(public_key_1_refreshed.size(), 0u);
+  EXPECT_GT(auth_secret_1_refreshed.size(), 0u);
+  EXPECT_NE(public_key_1, public_key_1_refreshed);
+  EXPECT_NE(auth_secret_1, auth_secret_1_refreshed);
+  EXPECT_NE(public_key_2, public_key_1_refreshed);
+  EXPECT_NE(auth_secret_2, auth_secret_1_refreshed);
+
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_1, true));
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_2, true));
+
+  encryption_provider()->RemoveEncryptionInfo(kExampleAppId, "*",
+                                              base::Bind(&base::DoNothing));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_1, false));
+  EXPECT_NO_FATAL_FAILURE(CheckHasKey(instance_id_authorized_entity_2, false));
+}
+
+void GCMEncryptionProviderTest::TestEncryptionRoundTrip(
+    const std::string& app_id,
+    const std::string& instance_id_authorized_entity) {
   // Performs a full round-trip of the encryption feature, including getting a
   // public/private key-pair and performing the cryptographic operations. This
   // is more of an integration test than a unit test.
@@ -234,13 +437,13 @@ TEST_F(GCMEncryptionProviderTest, EncryptionRoundTrip) {
   // that the GCMEncryptionProvider will only share the public key with users.
   // Also create a second pair, which will act as the server's keys.
   encryption_provider()->key_store_->CreateKeys(
-      kExampleAppId,
-      base::Bind(&GCMEncryptionProviderTest::DidCreateKeys,
+      app_id, instance_id_authorized_entity,
+      base::Bind(&GCMEncryptionProviderTest::HandleKeysCallback,
                  base::Unretained(this), &pair, &auth_secret));
 
   encryption_provider()->key_store_->CreateKeys(
-      std::string(kExampleAppId) + "-server",
-      base::Bind(&GCMEncryptionProviderTest::DidCreateKeys,
+      "server-" + app_id, instance_id_authorized_entity,
+      base::Bind(&GCMEncryptionProviderTest::HandleKeysCallback,
                  base::Unretained(this), &server_pair, &server_authentication));
 
   // Creating the public keys will be done asynchronously.
@@ -266,6 +469,8 @@ TEST_F(GCMEncryptionProviderTest, EncryptionRoundTrip) {
   IncomingMessage message;
   size_t record_size;
 
+  message.sender_id = kExampleAuthorizedEntity;
+
   // Encrypts the |kExampleMessage| using the generated shared key and the
   // random |salt|, storing the result in |record_size| and the message.
   GCMMessageCryptographer cryptographer(
@@ -302,4 +507,18 @@ TEST_F(GCMEncryptionProviderTest, EncryptionRoundTrip) {
   EXPECT_EQ(kExampleMessage, decrypted_message().raw_data);
 }
 
+TEST_F(GCMEncryptionProviderTest, EncryptionRoundTripGCMRegistration) {
+  // GCMEncryptionProvider::DecryptMessage should succeed when the message was
+  // sent to a legacy GCM registration (empty instance_id_authorized_entity).
+  ASSERT_NO_FATAL_FAILURE(TestEncryptionRoundTrip(
+      kExampleAppId, "" /* empty authorized entity for GCM registration */));
+}
+
+TEST_F(GCMEncryptionProviderTest, EncryptionRoundTripInstanceIDToken) {
+  // GCMEncryptionProvider::DecryptMessage should succeed when the message was
+  // sent to an InstanceID token (non-empty instance_id_authorized_entity).
+  ASSERT_NO_FATAL_FAILURE(
+      TestEncryptionRoundTrip(kExampleAppId, kExampleAuthorizedEntity));
+}
+
 }  // namespace gcm