Add GetKeyBundle() method to key registry

Add a convenience method GetKeyBundle() to CryptAuthKeyRegistry that
returns a const pointer to the key bundle in the registry if it exists.
If no key bundle is found, nullptr is returned.

We clean up the unit tests by using this method. Also, we make sure
EXPECT_EQ has the form EXPECT_EQ(<expected>, <actual>).

Bug: 899080
Change-Id: I7d2ca0d8ccfe0fc2a091a90cba794dbd1ad15780
Reviewed-on: https://chromium-review.googlesource.com/c/1474646
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635593}

chromeos/services/device_sync/cryptauth_key_registry.cc

@@ -17,6 +17,15 @@ CryptAuthKeyRegistry::enrolled_key_bundles() const {
   return enrolled_key_bundles_;
 }
 
+const CryptAuthKeyBundle* CryptAuthKeyRegistry::GetKeyBundle(
+    CryptAuthKeyBundle::Name name) const {
+  auto it = enrolled_key_bundles_.find(name);
+  if (it == enrolled_key_bundles_.end())
+    return nullptr;
+
+  return &it->second;
+}
+
 const CryptAuthKey* CryptAuthKeyRegistry::GetActiveKey(
     CryptAuthKeyBundle::Name name) const {
   auto it_bundle = enrolled_key_bundles_.find(name);

chromeos/services/device_sync/cryptauth_key_registry.h

@@ -25,8 +25,13 @@ class CryptAuthKeyRegistry {
   // Returns the underlying map from the key-bundle name to the key bundle.
   virtual const KeyBundleMap& enrolled_key_bundles() const;
 
+  // Returns the key bundle with name |name| if it exists in the key registry,
+  // and returns null if it cannot be found.
+  virtual const CryptAuthKeyBundle* GetKeyBundle(
+      CryptAuthKeyBundle::Name name) const;
+
   // Returns the key with status kActive if one exists in the key bundle with
-  // name |name|.
+  // name |name|, and returns null if one cannot be found.
   virtual const CryptAuthKey* GetActiveKey(CryptAuthKeyBundle::Name name) const;
 
   // Adds |key| to the key bundle with |name|. If the key being added is active,

chromeos/services/device_sync/cryptauth_key_registry_impl.h

@@ -5,9 +5,9 @@
 #ifndef CHROMEOS_SERVICES_DEVICE_SYNC_CRYPTAUTH_KEY_REGISTRY_IMPL_H_
 #define CHROMEOS_SERVICES_DEVICE_SYNC_CRYPTAUTH_KEY_REGISTRY_IMPL_H_
 
-#include "chromeos/services/device_sync/cryptauth_key_registry.h"
-
 #include "base/macros.h"
+#include "base/values.h"
+#include "chromeos/services/device_sync/cryptauth_key_registry.h"
 
 class PrefRegistrySimple;
 class PrefService;

chromeos/services/device_sync/cryptauth_key_registry_impl_unittest.cc

@@ -30,7 +30,7 @@ class CryptAuthKeyRegistryImplTest : public testing::Test {
     const base::DictionaryValue* dict =
         pref_service_.GetDictionary(prefs::kCryptAuthKeyRegistry);
     ASSERT_TRUE(dict);
-    EXPECT_EQ(*dict, expected_dict);
+    EXPECT_EQ(expected_dict, *dict);
   }
 
   PrefService* pref_service() { return &pref_service_; }
@@ -69,7 +69,7 @@ TEST_F(CryptAuthKeyRegistryImplTest, GetActiveKey) {
   const CryptAuthKey* key =
       key_registry()->GetActiveKey(CryptAuthKeyBundle::Name::kUserKeyPair);
   ASSERT_TRUE(key);
-  EXPECT_EQ(*key, asym_key);
+  EXPECT_EQ(asym_key, *key);
 }
 
 TEST_F(CryptAuthKeyRegistryImplTest, AddKey) {
@@ -77,18 +77,18 @@ TEST_F(CryptAuthKeyRegistryImplTest, AddKey) {
                        cryptauthv2::KeyType::RAW256, "sym-handle");
   key_registry()->AddEnrolledKey(CryptAuthKeyBundle::Name::kUserKeyPair,
                                  sym_key);
-  const auto& it = key_registry()->enrolled_key_bundles().find(
-      CryptAuthKeyBundle::Name::kUserKeyPair);
-  ASSERT_TRUE(it != key_registry()->enrolled_key_bundles().end());
+  const CryptAuthKeyBundle* key_bundle =
+      key_registry()->GetKeyBundle(CryptAuthKeyBundle::Name::kUserKeyPair);
+  ASSERT_TRUE(key_bundle);
 
   const CryptAuthKey* active_key =
       key_registry()->GetActiveKey(CryptAuthKeyBundle::Name::kUserKeyPair);
   ASSERT_TRUE(active_key);
-  EXPECT_EQ(*active_key, sym_key);
+  EXPECT_EQ(sym_key, *active_key);
 
   CryptAuthKeyBundle expected_bundle(CryptAuthKeyBundle::Name::kUserKeyPair);
   expected_bundle.AddKey(sym_key);
-  EXPECT_EQ(expected_bundle, it->second);
+  EXPECT_EQ(expected_bundle, *key_bundle);
 
   base::Value expected_dict(base::Value::Type::DICTIONARY);
   expected_dict.SetKey(
@@ -104,12 +104,12 @@ TEST_F(CryptAuthKeyRegistryImplTest, AddKey) {
                                  asym_key);
 
   expected_bundle.AddKey(asym_key);
-  EXPECT_EQ(expected_bundle, it->second);
+  EXPECT_EQ(expected_bundle, *key_bundle);
 
   active_key =
       key_registry()->GetActiveKey(CryptAuthKeyBundle::Name::kUserKeyPair);
   ASSERT_TRUE(active_key);
-  EXPECT_EQ(*active_key, asym_key);
+  EXPECT_EQ(asym_key, *active_key);
 
   expected_dict.SetKey(
       CryptAuthKeyBundle::KeyBundleNameEnumToString(expected_bundle.name()),
@@ -136,7 +136,7 @@ TEST_F(CryptAuthKeyRegistryImplTest, SetActiveKey) {
   EXPECT_TRUE(key);
 
   sym_key.set_status(CryptAuthKey::Status::kActive);
-  EXPECT_EQ(*key, sym_key);
+  EXPECT_EQ(sym_key, *key);
 
   CryptAuthKeyBundle expected_bundle(CryptAuthKeyBundle::Name::kUserKeyPair);
   expected_bundle.AddKey(sym_key);
@@ -190,12 +190,14 @@ TEST_F(CryptAuthKeyRegistryImplTest, DeleteKey) {
   key_registry()->DeleteKey(CryptAuthKeyBundle::Name::kUserKeyPair,
                             "sym-handle");
 
-  const auto& it = key_registry()->enrolled_key_bundles().find(
-      CryptAuthKeyBundle::Name::kUserKeyPair);
-  ASSERT_TRUE(it != key_registry()->enrolled_key_bundles().end());
+  const CryptAuthKeyBundle* key_bundle =
+      key_registry()->GetKeyBundle(CryptAuthKeyBundle::Name::kUserKeyPair);
+  ASSERT_TRUE(key_bundle);
 
-  EXPECT_FALSE(base::ContainsKey(it->second.handle_to_key_map(), "sym-handle"));
-  EXPECT_TRUE(base::ContainsKey(it->second.handle_to_key_map(), "asym-handle"));
+  EXPECT_FALSE(
+      base::ContainsKey(key_bundle->handle_to_key_map(), "sym-handle"));
+  EXPECT_TRUE(
+      base::ContainsKey(key_bundle->handle_to_key_map(), "asym-handle"));
 
   CryptAuthKeyBundle expected_bundle(CryptAuthKeyBundle::Name::kUserKeyPair);
   expected_bundle.AddKey(asym_key);
@@ -217,13 +219,13 @@ TEST_F(CryptAuthKeyRegistryImplTest, SetKeyDirective) {
   key_registry()->SetKeyDirective(CryptAuthKeyBundle::Name::kUserKeyPair,
                                   key_directive);
 
-  const auto& it = key_registry()->enrolled_key_bundles().find(
-      CryptAuthKeyBundle::Name::kUserKeyPair);
-  ASSERT_TRUE(it != key_registry()->enrolled_key_bundles().end());
+  const CryptAuthKeyBundle* key_bundle =
+      key_registry()->GetKeyBundle(CryptAuthKeyBundle::Name::kUserKeyPair);
+  ASSERT_TRUE(key_bundle);
 
-  EXPECT_TRUE(it->second.key_directive());
-  EXPECT_EQ(it->second.key_directive()->SerializeAsString(),
-            key_directive.SerializeAsString());
+  EXPECT_TRUE(key_bundle->key_directive());
+  EXPECT_EQ(key_directive.SerializeAsString(),
+            key_bundle->key_directive()->SerializeAsString());
 
   CryptAuthKeyBundle expected_bundle(CryptAuthKeyBundle::Name::kUserKeyPair);
   expected_bundle.AddKey(sym_key);
@@ -249,16 +251,16 @@ TEST_F(CryptAuthKeyRegistryImplTest, ConstructorPopulatesBundlesUsingPref) {
   std::unique_ptr<CryptAuthKeyRegistry> new_registry =
       CryptAuthKeyRegistryImpl::Factory::Get()->BuildInstance(pref_service());
 
-  EXPECT_TRUE(new_registry->enrolled_key_bundles().size() == 1);
+  EXPECT_EQ(1u, new_registry->enrolled_key_bundles().size());
 
-  const auto& it = new_registry->enrolled_key_bundles().find(
-      CryptAuthKeyBundle::Name::kUserKeyPair);
-  ASSERT_TRUE(it != new_registry->enrolled_key_bundles().end());
+  const CryptAuthKeyBundle* key_bundle =
+      key_registry()->GetKeyBundle(CryptAuthKeyBundle::Name::kUserKeyPair);
+  ASSERT_TRUE(key_bundle);
 
   CryptAuthKeyBundle expected_bundle(CryptAuthKeyBundle::Name::kUserKeyPair);
   expected_bundle.AddKey(sym_key);
   expected_bundle.set_key_directive(key_directive);
-  EXPECT_EQ(it->second, expected_bundle);
+  EXPECT_EQ(expected_bundle, *key_bundle);
 }
 
 }  // namespace device_sync