CREDENTIAL: Disable auto sign-in for credentials after clearing site …

…data. If the user clears their state, we should presume that they're signed out of websites as well. This patch sets 'skip_zero_click' accordingly by wiring a new method on 'PasswordStore' to the 'BrowsingDataRemover' on one end, and all the password store backends on the other. BUG=589779 [email protected], [email protected], [email protected] Review URL: https://codereview.chromium.org/1732723005 Cr-Commit-Position: refs/heads/master@{#378211}
huningxin · Feb 29, 2016 · 6fd6144 · 6fd6144
1 parent 4a1dac9
commit 6fd6144
Show file tree

Hide file tree

Showing 30 changed files with 436 additions and 16 deletions.
diff --git a/chrome/browser/browsing_data/browsing_data_remover.cc b/chrome/browser/browsing_data/browsing_data_remover.cc
@@ -621,9 +621,6 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
     }
 
     MediaDeviceIDSalt::Reset(profile_->GetPrefs());
-
-    // TODO(mkwst): If we're not removing passwords, then clear the 'zero-click'
-    // flag for all credentials in the password store.
   }
 
   // Channel IDs are not separated for protected and unprotected web
@@ -727,6 +724,21 @@ void BrowsingDataRemover::RemoveImpl(const TimeRange& time_range,
     }
   }
 
+  if (remove_mask & REMOVE_COOKIES) {
+    password_manager::PasswordStore* password_store =
+        PasswordStoreFactory::GetForProfile(profile_,
+                                            ServiceAccessType::EXPLICIT_ACCESS)
+            .get();
+
+    if (password_store) {
+      waiting_for_clear_auto_sign_in_ = true;
+      base::Closure on_cleared_auto_sign_in =
+          base::Bind(&BrowsingDataRemover::OnClearedAutoSignIn,
+                     weak_ptr_factory_.GetWeakPtr());
+      password_store->DisableAutoSignInForAllLogins(on_cleared_auto_sign_in);
+    }
+  }
+
   if (remove_mask & REMOVE_HISTORY) {
     password_manager::PasswordStore* password_store =
         PasswordStoreFactory::GetForProfile(
@@ -986,22 +998,16 @@ base::Time BrowsingDataRemover::CalculateBeginDeleteTime(
 
 bool BrowsingDataRemover::AllDone() {
   return !waiting_for_clear_autofill_origin_urls_ &&
-         !waiting_for_clear_cache_ &&
-         !waiting_for_clear_content_licenses_ &&
-         !waiting_for_clear_channel_ids_ &&
-         !waiting_for_clear_cookies_count_ &&
+         !waiting_for_clear_cache_ && !waiting_for_clear_content_licenses_ &&
+         !waiting_for_clear_channel_ids_ && !waiting_for_clear_cookies_count_ &&
          !waiting_for_clear_domain_reliability_monitor_ &&
-         !waiting_for_clear_form_ &&
-         !waiting_for_clear_history_ &&
+         !waiting_for_clear_form_ && !waiting_for_clear_history_ &&
          !waiting_for_clear_hostname_resolution_cache_ &&
-         !waiting_for_clear_keyword_data_ &&
-         !waiting_for_clear_nacl_cache_ &&
+         !waiting_for_clear_keyword_data_ && !waiting_for_clear_nacl_cache_ &&
          !waiting_for_clear_network_predictor_ &&
          !waiting_for_clear_networking_history_ &&
-         !waiting_for_clear_passwords_ &&
-         !waiting_for_clear_passwords_stats_ &&
-         !waiting_for_clear_platform_keys_ &&
-         !waiting_for_clear_plugin_data_ &&
+         !waiting_for_clear_passwords_ && !waiting_for_clear_passwords_stats_ &&
+         !waiting_for_clear_platform_keys_ && !waiting_for_clear_plugin_data_ &&
          !waiting_for_clear_pnacl_cache_ &&
 #if BUILDFLAG(ANDROID_JAVA_UI)
          !waiting_for_clear_precache_history_ &&
@@ -1011,7 +1017,8 @@ bool BrowsingDataRemover::AllDone() {
 #if defined(ENABLE_WEBRTC)
          !waiting_for_clear_webrtc_logs_ &&
 #endif
-         !waiting_for_clear_storage_partition_data_;
+         !waiting_for_clear_storage_partition_data_ &&
+         !waiting_for_clear_auto_sign_in_;
 }
 
 void BrowsingDataRemover::OnKeywordsLoaded() {
@@ -1145,6 +1152,12 @@ void BrowsingDataRemover::OnClearedPasswordsStats() {
   NotifyIfDone();
 }
 
+void BrowsingDataRemover::OnClearedAutoSignIn() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  waiting_for_clear_auto_sign_in_ = false;
+  NotifyIfDone();
+}
+
 void BrowsingDataRemover::OnClearedCookies() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 

diff --git a/chrome/browser/browsing_data/browsing_data_remover.h b/chrome/browser/browsing_data/browsing_data_remover.h
@@ -353,6 +353,9 @@ class BrowsingDataRemover : public KeyedService
   // cleared.
   void OnClearedPasswordsStats();
 
+  // Callback for when the autosignin state of passwords has been revoked.
+  void OnClearedAutoSignIn();
+
   // Callback for when cookies have been deleted. Invokes NotifyIfDone.
   void OnClearedCookies();
 
@@ -452,6 +455,7 @@ class BrowsingDataRemover : public KeyedService
 #if defined(ENABLE_WEBRTC)
   bool waiting_for_clear_webrtc_logs_ = false;
 #endif
+  bool waiting_for_clear_auto_sign_in_ = false;
 
   // The removal mask for the current removal operation.
   int remove_mask_ = 0;

diff --git a/chrome/browser/browsing_data/browsing_data_remover_unittest.cc b/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
@@ -2277,3 +2277,27 @@ TEST_F(BrowsingDataRemoverTest, RemovePasswordsByOrigin) {
   BlockUntilOriginDataRemoved(BrowsingDataRemover::EVERYTHING,
                               BrowsingDataRemover::REMOVE_PASSWORDS, kOrigin1);
 }
+
+TEST_F(BrowsingDataRemoverTest, DisableAutoSignIn) {
+  RemovePasswordsTester tester(GetProfile());
+
+  EXPECT_CALL(*tester.store(), DisableAutoSignInForAllLoginsImpl())
+      .WillOnce(Return(password_manager::PasswordStoreChangeList()));
+
+  BlockUntilBrowsingDataRemoved(BrowsingDataRemover::EVERYTHING,
+                                BrowsingDataRemover::REMOVE_COOKIES, false);
+}
+
+TEST_F(BrowsingDataRemoverTest, DisableAutoSignInAfterRemovingPasswords) {
+  RemovePasswordsTester tester(GetProfile());
+
+  EXPECT_CALL(*tester.store(), RemoveLoginsCreatedBetweenImpl(_, _))
+      .WillOnce(Return(password_manager::PasswordStoreChangeList()));
+  EXPECT_CALL(*tester.store(), DisableAutoSignInForAllLoginsImpl())
+      .WillOnce(Return(password_manager::PasswordStoreChangeList()));
+
+  BlockUntilBrowsingDataRemoved(BrowsingDataRemover::EVERYTHING,
+                                BrowsingDataRemover::REMOVE_COOKIES |
+                                    BrowsingDataRemover::REMOVE_PASSWORDS,
+                                false);
+}
diff --git a/chrome/browser/password_manager/native_backend_gnome_x.cc b/chrome/browser/password_manager/native_backend_gnome_x.cc
@@ -681,6 +681,23 @@ bool NativeBackendGnome::RemoveLoginsSyncedBetween(
   return RemoveLoginsBetween(delete_begin, delete_end, SYNC_TIMESTAMP, changes);
 }
 
+bool NativeBackendGnome::DisableAutoSignInForAllLogins(
+    password_manager::PasswordStoreChangeList* changes) {
+  ScopedVector<PasswordForm> forms;
+  if (!GetAllLogins(&forms))
+    return false;
+
+  for (auto& form : forms) {
+    if (!form->skip_zero_click) {
+      form->skip_zero_click = true;
+      if (!UpdateLogin(*form, changes))
+        return false;
+    }
+  }
+
+  return true;
+}
+
 bool NativeBackendGnome::GetLogins(const PasswordForm& form,
                                    ScopedVector<PasswordForm>* forms) {
   DCHECK_CURRENTLY_ON(BrowserThread::DB);

diff --git a/chrome/browser/password_manager/native_backend_gnome_x.h b/chrome/browser/password_manager/native_backend_gnome_x.h
@@ -107,6 +107,8 @@ class NativeBackendGnome : public PasswordStoreX::NativeBackend,
       base::Time delete_begin,
       base::Time delete_end,
       password_manager::PasswordStoreChangeList* changes) override;
+  bool DisableAutoSignInForAllLogins(
+      password_manager::PasswordStoreChangeList* changes) override;
   bool GetLogins(const autofill::PasswordForm& form,
                  ScopedVector<autofill::PasswordForm>* forms) override;
   bool GetAutofillableLogins(

diff --git a/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc b/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc
@@ -1165,6 +1165,49 @@ TEST_F(NativeBackendGnomeTest, RemoveLoginsSyncedBetween) {
   CheckRemoveLoginsBetween(SYNCED);
 }
 
+TEST_F(NativeBackendGnomeTest, DisableAutoSignInForAllLogins) {
+  NativeBackendGnome backend(42);
+  backend.Init();
+  form_google_.skip_zero_click = false;
+  form_facebook_.skip_zero_click = false;
+
+  BrowserThread::PostTask(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(base::IgnoreResult(&NativeBackendGnome::AddLogin),
+                 base::Unretained(&backend), form_google_));
+  BrowserThread::PostTask(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(base::IgnoreResult(&NativeBackendGnome::AddLogin),
+                 base::Unretained(&backend), form_facebook_));
+
+  RunBothThreads();
+
+  EXPECT_EQ(2u, mock_keyring_items.size());
+  for (const auto& item : mock_keyring_items)
+    CheckUint32Attribute(&item, "should_skip_zero_click", 0);
+
+  // Set the canonical forms to the updated value for the following comparison.
+  form_google_.skip_zero_click = true;
+  form_facebook_.skip_zero_click = true;
+  PasswordStoreChangeList expected_changes;
+  expected_changes.push_back(
+      PasswordStoreChange(PasswordStoreChange::UPDATE, form_facebook_));
+  expected_changes.push_back(
+      PasswordStoreChange(PasswordStoreChange::UPDATE, form_google_));
+
+  PasswordStoreChangeList changes;
+  BrowserThread::PostTaskAndReplyWithResult(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(&NativeBackendGnome::DisableAutoSignInForAllLogins,
+                 base::Unretained(&backend), &changes),
+      base::Bind(&CheckPasswordChangesWithResult, &expected_changes, &changes));
+  RunBothThreads();
+
+  EXPECT_EQ(2u, mock_keyring_items.size());
+  for (const auto& item : mock_keyring_items)
+    CheckUint32Attribute(&item, "should_skip_zero_click", 1);
+}
+
 TEST_F(NativeBackendGnomeTest, ReadDuplicateForms) {
   NativeBackendGnome backend(42);
   backend.Init();

diff --git a/chrome/browser/password_manager/native_backend_kwallet_x.cc b/chrome/browser/password_manager/native_backend_kwallet_x.cc
@@ -581,6 +581,22 @@ bool NativeBackendKWallet::RemoveLoginsSyncedBetween(
   return RemoveLoginsBetween(delete_begin, delete_end, SYNC_TIMESTAMP, changes);
 }
 
+bool NativeBackendKWallet::DisableAutoSignInForAllLogins(
+    password_manager::PasswordStoreChangeList* changes) {
+  ScopedVector<autofill::PasswordForm> all_forms;
+  if (!GetAllLogins(&all_forms))
+    return false;
+
+  for (auto& form : all_forms) {
+    if (!form->skip_zero_click) {
+      form->skip_zero_click = true;
+      if (!UpdateLogin(*form, changes))
+        return false;
+    }
+  }
+  return true;
+}
+
 bool NativeBackendKWallet::GetLogins(
     const PasswordForm& form,
     ScopedVector<autofill::PasswordForm>* forms) {

diff --git a/chrome/browser/password_manager/native_backend_kwallet_x.h b/chrome/browser/password_manager/native_backend_kwallet_x.h
@@ -58,6 +58,8 @@ class NativeBackendKWallet : public PasswordStoreX::NativeBackend {
       base::Time delete_begin,
       base::Time delete_end,
       password_manager::PasswordStoreChangeList* changes) override;
+  bool DisableAutoSignInForAllLogins(
+      password_manager::PasswordStoreChangeList* changes) override;
   bool GetLogins(const autofill::PasswordForm& form,
                  ScopedVector<autofill::PasswordForm>* forms) override;
   bool GetAutofillableLogins(

diff --git a/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc b/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
@@ -983,6 +983,40 @@ TEST_P(NativeBackendKWalletTest, RemoveLoginsSyncedBetween) {
   TestRemoveLoginsBetween(SYNCED);
 }
 
+TEST_P(NativeBackendKWalletTest, DisableAutoSignInForAllLogins) {
+  NativeBackendKWalletStub backend(42, desktop_env_);
+  EXPECT_TRUE(backend.InitWithBus(mock_session_bus_));
+
+  form_google_.skip_zero_click = false;
+
+  BrowserThread::PostTask(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(base::IgnoreResult(&NativeBackendKWallet::AddLogin),
+                 base::Unretained(&backend), form_google_));
+
+  RunDBThread();
+
+  // Set the canonical forms to the updated value for the following comparison.
+  form_google_.skip_zero_click = true;
+  PasswordStoreChangeList expected_changes;
+  expected_changes.push_back(
+      PasswordStoreChange(PasswordStoreChange::UPDATE, form_google_));
+
+  PasswordStoreChangeList changes;
+  BrowserThread::PostTaskAndReplyWithResult(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(&NativeBackendKWallet::DisableAutoSignInForAllLogins,
+                 base::Unretained(&backend), &changes),
+      base::Bind(&CheckPasswordChangesWithResult, &expected_changes, &changes));
+  RunDBThread();
+
+  std::vector<const PasswordForm*> forms;
+  forms.push_back(&form_google_);
+  ExpectationArray expected;
+  expected.push_back(make_pair(std::string(form_google_.signon_realm), forms));
+  CheckPasswordForms("Chrome Form Data (42)", expected);
+}
+
 TEST_P(NativeBackendKWalletTest, ReadDuplicateForms) {
   NativeBackendKWalletStub backend(42, desktop_env_);
   EXPECT_TRUE(backend.InitWithBus(mock_session_bus_));

diff --git a/chrome/browser/password_manager/native_backend_libsecret.cc b/chrome/browser/password_manager/native_backend_libsecret.cc
@@ -408,6 +408,23 @@ bool NativeBackendLibsecret::RemoveLoginsSyncedBetween(
   return RemoveLoginsBetween(delete_begin, delete_end, SYNC_TIMESTAMP, changes);
 }
 
+bool NativeBackendLibsecret::DisableAutoSignInForAllLogins(
+    password_manager::PasswordStoreChangeList* changes) {
+  ScopedVector<autofill::PasswordForm> all_forms;
+  if (!GetLoginsList(nullptr, ALL_LOGINS, &all_forms))
+    return false;
+
+  for (auto& form : all_forms) {
+    if (!form->skip_zero_click) {
+      form->skip_zero_click = true;
+      if (!UpdateLogin(*form, changes))
+        return false;
+    }
+  }
+
+  return true;
+}
+
 bool NativeBackendLibsecret::GetLogins(
     const PasswordForm& form,
     ScopedVector<autofill::PasswordForm>* forms) {

diff --git a/chrome/browser/password_manager/native_backend_libsecret.h b/chrome/browser/password_manager/native_backend_libsecret.h
@@ -71,6 +71,8 @@ class NativeBackendLibsecret : public PasswordStoreX::NativeBackend,
       base::Time delete_begin,
       base::Time delete_end,
       password_manager::PasswordStoreChangeList* changes) override;
+  bool DisableAutoSignInForAllLogins(
+      password_manager::PasswordStoreChangeList* changes) override;
   bool GetLogins(const autofill::PasswordForm& form,
                  ScopedVector<autofill::PasswordForm>* forms) override;
   bool GetAutofillableLogins(

diff --git a/chrome/browser/password_manager/native_backend_libsecret_unittest.cc b/chrome/browser/password_manager/native_backend_libsecret_unittest.cc
@@ -875,6 +875,37 @@ TEST_F(NativeBackendLibsecretTest, RemoveLoginsSyncedBetween) {
   CheckRemoveLoginsBetween(SYNCED);
 }
 
+TEST_F(NativeBackendLibsecretTest, DisableAutoSignInForAllLogins) {
+  NativeBackendLibsecret backend(42);
+  backend.Init();
+  form_google_.skip_zero_click = false;
+  form_facebook_.skip_zero_click = false;
+
+  VerifiedAdd(&backend, form_google_);
+  VerifiedAdd(&backend, form_facebook_);
+
+  EXPECT_EQ(2u, global_mock_libsecret_items->size());
+  for (const auto& item : *global_mock_libsecret_items)
+    CheckUint32Attribute(item, "should_skip_zero_click", 0);
+
+  // Set the canonical forms to the updated value for the following comparison.
+  form_google_.skip_zero_click = true;
+  form_facebook_.skip_zero_click = true;
+  PasswordStoreChangeList expected_changes;
+  expected_changes.push_back(
+      PasswordStoreChange(PasswordStoreChange::UPDATE, form_facebook_));
+  expected_changes.push_back(
+      PasswordStoreChange(PasswordStoreChange::UPDATE, form_google_));
+
+  PasswordStoreChangeList changes;
+  EXPECT_TRUE(backend.DisableAutoSignInForAllLogins(&changes));
+  CheckPasswordChanges(expected_changes, changes);
+
+  EXPECT_EQ(2u, global_mock_libsecret_items->size());
+  for (const auto& item : *global_mock_libsecret_items)
+    CheckUint32Attribute(item, "should_skip_zero_click", 1);
+}
+
 TEST_F(NativeBackendLibsecretTest, SomeKeyringAttributesAreMissing) {
   // Absent attributes should be filled with default values.
   NativeBackendLibsecret backend(42);

diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
@@ -1198,6 +1198,18 @@ PasswordStoreChangeList PasswordStoreMac::RemoveLoginsSyncedBetweenImpl(
   return changes;
 }
 
+PasswordStoreChangeList PasswordStoreMac::DisableAutoSignInForAllLoginsImpl() {
+  ScopedVector<PasswordForm> forms;
+  PasswordStoreChangeList list;
+  if (login_metadata_db_ && login_metadata_db_->GetAutoSignInLogins(&forms) &&
+      login_metadata_db_->DisableAutoSignInForAllLogins()) {
+    for (const auto& form : forms)
+      list.push_back(PasswordStoreChange(PasswordStoreChange::UPDATE, *form));
+  }
+
+  return list;
+}
+
 bool PasswordStoreMac::RemoveStatisticsCreatedBetweenImpl(
     base::Time delete_begin,
     base::Time delete_end) {

diff --git a/chrome/browser/password_manager/password_store_mac.h b/chrome/browser/password_manager/password_store_mac.h
@@ -89,6 +89,8 @@ class PasswordStoreMac : public password_manager::PasswordStore {
   password_manager::PasswordStoreChangeList RemoveLoginsSyncedBetweenImpl(
       base::Time delete_begin,
       base::Time delete_end) override;
+  password_manager::PasswordStoreChangeList DisableAutoSignInForAllLoginsImpl()
+      override;
   bool RemoveStatisticsCreatedBetweenImpl(base::Time delete_begin,
                                           base::Time delete_end) override;
   ScopedVector<autofill::PasswordForm> FillMatchingLogins(