Defer reset of account storage opt-in for passwords

The opt-in for account storage of passwords needs to be reset after
password deletion is finished and committed to the sync server.
Add integration test for password deletion for account storage.

Bug: 1052005
Change-Id: I342f60b076a2ee0a7a8dfae0de4341b7ab684cb3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2250186
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779831}

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc

@@ -366,6 +366,7 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
            ~content::BrowsingDataRemover::DATA_TYPE_AVOID_CLOSING_CONNECTIONS &
            ~FILTERABLE_DATA_TYPES) == 0) ||
          filter_builder->IsEmptyBlacklist());
+  DCHECK(!should_clear_password_account_storage_settings_);
 
   TRACE_EVENT0("browsing_data",
                "ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData");
@@ -724,14 +725,9 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
 
     if (nullable_filter.is_null() ||
         nullable_filter.Run(GaiaUrls::GetInstance()->google_url())) {
-      // Explicitly clear any opt-ins to the account-scoped password storage
-      // when cookies are being cleared.
-      // TODO(crbug.com/1052005, crbug.com/1078762): These *should* get cleared
-      // automatically when the Google cookies are deleted, but currently this
-      // doesn't always work reliably. When these bugs get resolved, the
-      // following line can be removed.
-      password_manager::features_util::ClearAccountStorageSettingsForAllUsers(
-          prefs);
+      // Set a flag to clear account storage settings later instead of clearing
+      // it now as we can not reset this setting before passwords are deleted.
+      should_clear_password_account_storage_settings_ = true;
     }
   }
 
@@ -1281,6 +1277,19 @@ void ChromeBrowsingDataRemoverDelegate::OnTaskComplete(
   if (!pending_sub_tasks_.empty())
     return;
 
+  // Explicitly clear any opt-ins to the account-scoped password storage
+  // when cookies are being cleared. This needs to happen after passwords
+  // have been deleted, so it is performed when all other tasks are completed.
+  // TODO(crbug.com/1052005, crbug.com/1078762): These *should* get cleared
+  // automatically when the Google cookies are deleted, but currently this
+  // doesn't always work reliably. When these bugs get resolved, the
+  // following line and associated code can be removed.
+  if (should_clear_password_account_storage_settings_) {
+    should_clear_password_account_storage_settings_ = false;
+    password_manager::features_util::ClearAccountStorageSettingsForAllUsers(
+        profile_->GetPrefs());
+  }
+
   slow_pending_tasks_closure_.Cancel();
 
   DCHECK(!callback_.is_null());

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h

@@ -335,6 +335,8 @@ class ChromeBrowsingDataRemoverDelegate
   std::unique_ptr<WebappRegistry> webapp_registry_;
 #endif
 
+  bool should_clear_password_account_storage_settings_ = false;
+
   base::WeakPtrFactory<ChromeBrowsingDataRemoverDelegate> weak_ptr_factory_{
       this};
 

chrome/browser/sync/test/integration/password_manager_sync_test.cc

@@ -8,6 +8,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
+#include "chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h"
 #include "chrome/browser/password_manager/account_storage/account_password_store_factory.h"
 #include "chrome/browser/password_manager/password_manager_test_base.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
@@ -27,7 +28,10 @@
 #include "components/sync/driver/profile_sync_service.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/test/fake_server/fake_server_nigori_helper.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/browsing_data_remover.h"
 #include "content/public/test/browser_test.h"
+#include "content/public/test/browsing_data_remover_test_util.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/http/http_status_code.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -623,6 +627,43 @@ IN_PROC_BROWSER_TEST_F(
               ElementsAre(MatchesLogin("accountuser", "accountpass")));
 }
 
+IN_PROC_BROWSER_TEST_F(PasswordManagerSyncTest,
+                       PasswordDeletionsPropagateToServer) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  // Add credential to server.
+  AddCredentialToFakeServer(CreateTestPasswordForm("user", "pass"));
+
+  SetupSyncTransportWithPasswordAccountStorage();
+  auto* account_store = passwords_helper::GetAccountPasswordStore(0);
+
+  // Make sure the password show up in the account store and on the server.
+  ASSERT_EQ(passwords_helper::GetAllLogins(account_store).size(), 1u);
+  ASSERT_EQ(fake_server_->GetSyncEntitiesByModelType(syncer::PASSWORDS).size(),
+            1u);
+  EXPECT_TRUE(password_manager::features_util::IsOptedInForAccountStorage(
+      GetProfile(0)->GetPrefs(), GetSyncService(0)));
+
+  // Clear all data including cookies and passwords.
+  content::BrowsingDataRemover* remover =
+      content::BrowserContext::GetBrowsingDataRemover(GetProfile(0));
+  content::BrowsingDataRemoverCompletionObserver observer(remover);
+  remover->RemoveAndReply(
+      base::Time(), base::Time::Max(),
+      ChromeBrowsingDataRemoverDelegate::ALL_DATA_TYPES,
+      content::BrowsingDataRemover::ORIGIN_TYPE_UNPROTECTED_WEB, &observer);
+  observer.BlockUntilCompletion();
+
+  // Now passwords should be removed from the client and server.
+  EXPECT_EQ(passwords_helper::GetAllLogins(account_store).size(), 0u);
+  EXPECT_EQ(fake_server_->GetSyncEntitiesByModelType(syncer::PASSWORDS).size(),
+            0u);
+
+  // The opt-in should be gone as well.
+  EXPECT_FALSE(password_manager::features_util::IsOptedInForAccountStorage(
+      GetProfile(0)->GetPrefs(), GetSyncService(0)));
+}
+
 #endif  // !defined(OS_CHROMEOS)
 
 }  // namespace