Always unlock all libsecret items in Password Manager and OSCrypt

components/os_crypt/key_storage_libsecret_unittest.cc

Index: components/os_crypt/key_storage_libsecret_unittest.cc
diff --git a/components/os_crypt/key_storage_libsecret_unittest.cc b/components/os_crypt/key_storage_libsecret_unittest.cc
index 35fa3f7166944e140d6127105113d1a582bbaa2c..94373c9ef9b6fe80e7329304ee7b36445506cc95 100644
--- a/components/os_crypt/key_storage_libsecret_unittest.cc
+++ b/components/os_crypt/key_storage_libsecret_unittest.cc
@@ -16,6 +16,8 @@ namespace {
 // cast to the correct signature. We can reduce SecretValue to an std::string,
 // because we don't use anything else from it.
 using MockSecretValue = std::string;
+// Likewise, we only need a SecretValue from SecretItem.
+using MockSecretItem = MockSecretValue;
 
 const SecretSchema kKeystoreSchemaV1 = {
     "chrome_libsecret_os_crypt_password",
@@ -61,13 +63,6 @@ class MockLibsecretLoader : public LibsecretLoader {
                                                   GError** error,
                                                   ...);
 
-  static MockSecretValue* mock_secret_service_lookup_sync(
-      SecretService* service,
-      const SecretSchema* schema,
-      GHashTable* attributes,
-      GCancellable* cancellable,
-      GError** error);
-
   static void mock_secret_value_unref(gpointer value);
 
   static GList* mock_secret_service_search_sync(SecretService* service,
@@ -82,6 +77,8 @@ class MockLibsecretLoader : public LibsecretLoader {
                                                   GError** error,
                                                   ...);
 
+  static MockSecretValue* mock_secret_item_get_secret(MockSecretItem* item);
+
   // MockLibsecretLoader owns these objects.
   static MockSecretValue* stored_password_mock_ptr_;
   static MockSecretValue* deprecated_password_mock_ptr_;
@@ -111,38 +108,32 @@ gboolean MockLibsecretLoader::mock_secret_password_store_sync(
 }
 
 // static
-MockSecretValue* MockLibsecretLoader::mock_secret_service_lookup_sync(
+void MockLibsecretLoader::mock_secret_value_unref(gpointer value) {}
+
+// static
+GList* MockLibsecretLoader::mock_secret_service_search_sync(
     SecretService* service,
     const SecretSchema* schema,
     GHashTable* attributes,
+    SecretSearchFlags flags,
     GCancellable* cancellable,
     GError** error) {
   bool is_known_schema = strcmp(schema->name, kKeystoreSchemaV2.name) == 0 ||
                          strcmp(schema->name, kKeystoreSchemaV1.name) == 0;
   EXPECT_TRUE(is_known_schema);
 
+  EXPECT_TRUE(flags & SECRET_SEARCH_UNLOCK);
+  EXPECT_TRUE(flags & SECRET_SEARCH_LOAD_SECRETS);
+
+  MockSecretItem* item = nullptr;
   if (strcmp(schema->name, kKeystoreSchemaV2.name) == 0)
-    return stored_password_mock_ptr_;
+    item = stored_password_mock_ptr_;
   else if (strcmp(schema->name, kKeystoreSchemaV1.name) == 0)
-    return deprecated_password_mock_ptr_;
-
-  NOTREACHED();
-  return nullptr;
-}
+    item = deprecated_password_mock_ptr_;
 
-// static
-void MockLibsecretLoader::mock_secret_value_unref(gpointer value) {}
-
-// static
-GList* MockLibsecretLoader::mock_secret_service_search_sync(
-    SecretService* service,
-    const SecretSchema* schema,
-    GHashTable* attributes,
-    SecretSearchFlags flags,
-    GCancellable* cancellable,
-    GError** error) {
-  *error = nullptr;
-  return nullptr;
+  GList* result = nullptr;
+  result = g_list_append(result, item);
+  return result;

[vasilii, 2016/10/21 14:10:07]

Setting |error|?

[cfroussios, 2016/10/21 17:56:36]

Done.

 }
 
 // static
@@ -151,6 +142,7 @@ gboolean MockLibsecretLoader::mock_secret_password_clear_sync(
     GCancellable* cancellable,
     GError** error,
     ...) {
+  // We would only delete entries in the deprecated schema.
   EXPECT_STREQ(kKeystoreSchemaV1.name, schema->name);
   delete deprecated_password_mock_ptr_;
   deprecated_password_mock_ptr_ = nullptr;
@@ -158,22 +150,26 @@ gboolean MockLibsecretLoader::mock_secret_password_clear_sync(
 }
 
 // static
+MockSecretValue* MockLibsecretLoader::mock_secret_item_get_secret(
+    MockSecretItem* item) {
+  return item;
+}
+
+// static
 bool MockLibsecretLoader::ResetForOSCrypt() {
-  // 4 methods used by KeyStorageLibsecret
+  // Methods used by KeyStorageLibsecret
   secret_password_store_sync =
       &MockLibsecretLoader::mock_secret_password_store_sync;
   secret_value_get_text = (decltype(&::secret_value_get_text)) &
                           MockLibsecretLoader::mock_secret_value_get_text;
   secret_value_unref = &MockLibsecretLoader::mock_secret_value_unref;
-  secret_service_lookup_sync =
-      (decltype(&::secret_service_lookup_sync)) &
-      MockLibsecretLoader::mock_secret_service_lookup_sync;
+  secret_service_search_sync =
+      &MockLibsecretLoader::mock_secret_service_search_sync;
+  secret_item_get_secret =
+      (decltype(&::secret_item_get_secret))mock_secret_item_get_secret;

[vasilii, 2016/10/21 14:10:07]

What is the reason to change the type of the function? It doesn't look safe.

[cfroussios, 2016/10/21 17:56:36]

The mock method gets a MockSecretItem and returns a MockSecretValue. These are
not compatible with the real types. Using the real classes would be more code.

The entire scheme for testing keyring and libsecret is a bit hacky. It should
probably be reworked sometime after the native backends are deprecated.

[vasilii, 2016/10/24 10:41:14]

I see only one place where secret_item_get_secret is called
(NativeBackendLibsecret::ConvertFormList). If it's called after this assignment
then the binary may crash. What happens if you just assign nullptr in line 168?

[cfroussios, 2016/10/24 11:25:55]

NativeBackendLibsecret::ConvertFormList is never supposed to be called from
OSCrypt tests.

ToSingleSecret() is where KeyStorageLibsecret calls secret_item_get_secret. The
tests will crash if the function pointer is null.

[vasilii, 2016/10/24 12:39:35]

Here is incomplete list of why this is wrong:
- (c++ 5.2.10) The effect of calling a function through a pointer to a function
type (8.3.5) that is not the same as the type used in the definition of the
function is undefined.
- Converting T1* to T2* and back is unspecified if alignment of T1 and T2 are
unknown.
- C-style cast is prohibited. reinterpret_cast is allowed when you know what you
are doing.

Why didn't you just derive from SecretValue and SecretItem?

[cfroussios, 2016/10/24 13:12:30]

I copied the scheme from native_backend_libsecret_unittest.cc.

If I understand correctly, your concerns apply to the entire scheme, and not
just this particular method. Addressing the concerns would require reworking the
entire file, if not also the respective keyring tests and password manager
tests. Could I start a bug, instead of fixing them in this CL? I will try to
merge this into previous versions and I would like this to be focused on the bug
I'm fixing.

   // Used by Migrate()
   secret_password_clear_sync =
       &MockLibsecretLoader::mock_secret_password_clear_sync;
-  // 1 method used by LibsecretLoader::EnsureLibsecretLoaded()
-  secret_service_search_sync =
-      &MockLibsecretLoader::mock_secret_service_search_sync;
 
   delete stored_password_mock_ptr_;
   stored_password_mock_ptr_ = nullptr;