Keep account scoped data when Profile is wiped

Add a separate data type for account scoped data that is not included
in WIPE_PROFILE or ALL_DATA_TYPES.

Bug: 1126947
Change-Id: I07618954753a631a6f328dba4513151f9bab4987
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2404160
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806958}

chrome/browser/android/browsing_data/browsing_data_bridge.cc

@@ -111,6 +111,8 @@ static void JNI_BrowsingDataBridge_ClearBrowsingData(
         break;
       case browsing_data::BrowsingDataType::PASSWORDS:
         remove_mask |= ChromeBrowsingDataRemoverDelegate::DATA_TYPE_PASSWORDS;
+        remove_mask |=
+            ChromeBrowsingDataRemoverDelegate::DATA_TYPE_ACCOUNT_PASSWORDS;
         break;
       case browsing_data::BrowsingDataType::FORM_DATA:
         remove_mask |= ChromeBrowsingDataRemoverDelegate::DATA_TYPE_FORM_DATA;

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc

@@ -812,21 +812,6 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
               TracingDataType::kCompromisedCredentials));
     }
 
-    auto account_store = AccountPasswordStoreFactory::GetForProfile(
-        profile_, ServiceAccessType::EXPLICIT_ACCESS);
-
-    if (account_store) {
-      account_store->RemoveLoginsByURLAndTime(
-          filter, delete_begin_, delete_end_,
-          CreateTaskCompletionClosure(TracingDataType::kAccountPasswords),
-          CreateTaskCompletionCallback(TracingDataType::kAccountPasswordsSynced,
-                                       DATA_TYPE_PASSWORDS));
-      account_store->RemoveCompromisedCredentialsByUrlAndTime(
-          nullable_filter, delete_begin_, delete_end_,
-          CreateTaskCompletionClosure(
-              TracingDataType::kAccountCompromisedCredentials));
-    }
-
     BrowserContext::GetDefaultStoragePartition(profile_)
         ->GetNetworkContext()
         ->ClearHttpAuthCache(delete_begin_,
@@ -857,6 +842,23 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
 #endif  // defined(OS_MAC)
   }
 
+  if (remove_mask & DATA_TYPE_ACCOUNT_PASSWORDS) {
+    auto account_store = AccountPasswordStoreFactory::GetForProfile(
+        profile_, ServiceAccessType::EXPLICIT_ACCESS);
+
+    if (account_store) {
+      account_store->RemoveLoginsByURLAndTime(
+          filter, delete_begin_, delete_end_,
+          CreateTaskCompletionClosure(TracingDataType::kAccountPasswords),
+          CreateTaskCompletionCallback(TracingDataType::kAccountPasswordsSynced,
+                                       DATA_TYPE_ACCOUNT_PASSWORDS));
+      account_store->RemoveCompromisedCredentialsByUrlAndTime(
+          nullable_filter, delete_begin_, delete_end_,
+          CreateTaskCompletionClosure(
+              TracingDataType::kAccountCompromisedCredentials));
+    }
+  }
+
   if (remove_mask & content::BrowsingDataRemover::DATA_TYPE_COOKIES) {
     password_manager::PasswordStore* password_store =
         PasswordStoreFactory::GetForProfile(profile_,

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h

@@ -76,6 +76,7 @@ class ChromeBrowsingDataRemoverDelegate
     DATA_TYPE_CONTENT_SETTINGS = DATA_TYPE_EMBEDDER_BEGIN << 9,
     DATA_TYPE_BOOKMARKS = DATA_TYPE_EMBEDDER_BEGIN << 10,
     DATA_TYPE_ISOLATED_ORIGINS = DATA_TYPE_EMBEDDER_BEGIN << 11,
+    DATA_TYPE_ACCOUNT_PASSWORDS = DATA_TYPE_EMBEDDER_BEGIN << 12,
 
     // Group datatypes.
 
@@ -106,11 +107,14 @@ class ChromeBrowsingDataRemoverDelegate
 
     // Datatypes with account-scoped data that needs to be removed
     // before Google cookies are deleted.
-    DEFERRED_COOKIE_DELETION_DATA_TYPES = DATA_TYPE_PASSWORDS,
+    DEFERRED_COOKIE_DELETION_DATA_TYPES = DATA_TYPE_ACCOUNT_PASSWORDS,
 
     // Includes all the available remove options. Meant to be used by clients
     // that wish to wipe as much data as possible from a Profile, to make it
-    // look like a new Profile.
+    // look like a new Profile. Does not delete account-scoped data like
+    // passwords but will remove access to account-scoped data by signing the
+    // user out.
+
     ALL_DATA_TYPES = DATA_TYPE_SITE_DATA |  //
                      content::BrowsingDataRemover::DATA_TYPE_CACHE |
                      content::BrowsingDataRemover::DATA_TYPE_DOWNLOADS |
@@ -161,6 +165,10 @@ class ChromeBrowsingDataRemoverDelegate
                 "Deferred deletion is currently not implemented for filterable "
                 "data types");
 
+  static_assert((DEFERRED_COOKIE_DELETION_DATA_TYPES & WIPE_PROFILE) == 0,
+                "Account data should not be included in deletions that remove "
+                "all local data");
+
   explicit ChromeBrowsingDataRemoverDelegate(
       content::BrowserContext* browser_context);
   ~ChromeBrowsingDataRemoverDelegate() override;

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc

@@ -2036,6 +2036,26 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest,
   EXPECT_CALL(*tester.profile_store(), RemoveLoginsByURLAndTimeImpl(_, _, _))
       .WillOnce(Return(password_manager::PasswordStoreChangeList()));
 
+  // Only DATA_TYPE_PASSWORDS is cleared. Accounts passwords are not affected.
+  EXPECT_CALL(*tester.account_store(), RemoveLoginsByURLAndTimeImpl(_, _, _))
+      .Times(0);
+
+  BlockUntilBrowsingDataRemoved(
+      base::Time(), base::Time::Max(),
+      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_PASSWORDS, false);
+}
+
+TEST_F(ChromeBrowsingDataRemoverDelegateTest,
+       RemoveAccountPasswordsByTimeOnly_WithAccountStore) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      password_manager::features::kEnablePasswordsAccountStorage);
+
+  RemovePasswordsTester tester(GetProfile());
+
+  EXPECT_CALL(*tester.profile_store(), RemoveLoginsByURLAndTimeImpl(_, _, _))
+      .Times(0);
+
   EXPECT_CALL(*tester.account_store(), RemoveLoginsByURLAndTimeImpl(_, _, _))
       .WillOnce(Return(password_manager::PasswordStoreChangeList()));
   // For the account store, the remover delegate also waits until all the
@@ -2046,11 +2066,11 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest,
 
   BlockUntilBrowsingDataRemoved(
       base::Time(), base::Time::Max(),
-      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_PASSWORDS, false);
+      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_ACCOUNT_PASSWORDS, false);
 }
 
 TEST_F(ChromeBrowsingDataRemoverDelegateTest,
-       RemovePasswordsByTimeOnly_WithAccountStore_Failure) {
+       RemoveAccountPasswordsByTimeOnly_WithAccountStore_Failure) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(
       password_manager::features::kEnablePasswordsAccountStorage);
@@ -2058,7 +2078,7 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest,
   RemovePasswordsTester tester(GetProfile());
 
   EXPECT_CALL(*tester.profile_store(), RemoveLoginsByURLAndTimeImpl(_, _, _))
-      .WillOnce(Return(password_manager::PasswordStoreChangeList()));
+      .Times(0);
 
   EXPECT_CALL(*tester.account_store(), RemoveLoginsByURLAndTimeImpl(_, _, _))
       .WillOnce(Return(password_manager::PasswordStoreChangeList()));
@@ -2073,9 +2093,9 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest,
 
   uint64_t failed_data_types = BlockUntilBrowsingDataRemoved(
       base::Time(), base::Time::Max(),
-      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_PASSWORDS, false);
+      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_ACCOUNT_PASSWORDS, false);
   EXPECT_EQ(failed_data_types,
-            ChromeBrowsingDataRemoverDelegate::DATA_TYPE_PASSWORDS);
+            ChromeBrowsingDataRemoverDelegate::DATA_TYPE_ACCOUNT_PASSWORDS);
 }
 
 // Disabled, since passwords are not yet marked as a filterable datatype.
@@ -2145,7 +2165,7 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest,
 }
 
 TEST_F(ChromeBrowsingDataRemoverDelegateTest,
-       RemoveCompromisedCredentialsByTimeOnly_WithAccountStore) {
+       RemoveCompromisedAccountCredentialsByTimeOnly_WithAccountStore) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(
       password_manager::features::kEnablePasswordsAccountStorage);
@@ -2153,10 +2173,9 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest,
   RemovePasswordsTester tester(GetProfile());
   base::RepeatingCallback<bool(const GURL&)> empty_filter;
 
-  EXPECT_CALL(
-      *tester.profile_store(),
-      RemoveCompromisedCredentialsByUrlAndTimeImpl(
-          ProbablySameFilter(empty_filter), base::Time(), base::Time::Max()));
+  EXPECT_CALL(*tester.profile_store(),
+              RemoveCompromisedCredentialsByUrlAndTimeImpl(_, _, _))
+      .Times(0);
 
   EXPECT_CALL(
       *tester.account_store(),
@@ -2165,7 +2184,7 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest,
 
   BlockUntilBrowsingDataRemoved(
       base::Time(), base::Time::Max(),
-      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_PASSWORDS, false);
+      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_ACCOUNT_PASSWORDS, false);
 }
 
 // TODO(crbug.com/589586): Disabled, since history is not yet marked as
@@ -3233,18 +3252,18 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest,
       password_manager::features::kEnablePasswordsAccountStorage);
 
   auto* delegate = GetProfile()->GetBrowsingDataRemoverDelegate();
+
   auto domains = delegate->GetDomainsForDeferredCookieDeletion(
-      ChromeBrowsingDataRemoverDelegate::ALL_DATA_TYPES);
+      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_ACCOUNT_PASSWORDS);
   EXPECT_EQ(domains.size(), 1u);
   EXPECT_EQ(domains[0], "google.com");
 
   domains = delegate->GetDomainsForDeferredCookieDeletion(
       ChromeBrowsingDataRemoverDelegate::DATA_TYPE_PASSWORDS);
-  EXPECT_EQ(domains.size(), 1u);
-  EXPECT_EQ(domains[0], "google.com");
+  EXPECT_EQ(domains.size(), 0u);
 
   domains = delegate->GetDomainsForDeferredCookieDeletion(
-      content::BrowsingDataRemover::DATA_TYPE_COOKIES);
+      ChromeBrowsingDataRemoverDelegate::ALL_DATA_TYPES);
   EXPECT_EQ(domains.size(), 0u);
 }
 

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

@@ -836,13 +836,14 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerSyncTest,
   EXPECT_TRUE(password_manager::features_util::IsOptedInForAccountStorage(
       GetProfile(0)->GetPrefs(), GetSyncService(0)));
 
-  // Clear all data including cookies and passwords.
+  // Clear cookies and account passwords.
   content::BrowsingDataRemover* remover =
       content::BrowserContext::GetBrowsingDataRemover(GetProfile(0));
   content::BrowsingDataRemoverCompletionObserver observer(remover);
   remover->RemoveAndReply(
       base::Time(), base::Time::Max(),
-      ChromeBrowsingDataRemoverDelegate::ALL_DATA_TYPES,
+      ChromeBrowsingDataRemoverDelegate::DATA_TYPE_SITE_DATA |
+          ChromeBrowsingDataRemoverDelegate::DATA_TYPE_ACCOUNT_PASSWORDS,
       content::BrowsingDataRemover::ORIGIN_TYPE_UNPROTECTED_WEB, &observer);
   observer.BlockUntilCompletion();
 

chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc

@@ -279,6 +279,8 @@ void ClearBrowsingDataHandler::HandleClearBrowsingData(
         break;
       case BrowsingDataType::PASSWORDS:
         remove_mask |= ChromeBrowsingDataRemoverDelegate::DATA_TYPE_PASSWORDS;
+        remove_mask |=
+            ChromeBrowsingDataRemoverDelegate::DATA_TYPE_ACCOUNT_PASSWORDS;
         break;
       case BrowsingDataType::FORM_DATA:
         remove_mask |= ChromeBrowsingDataRemoverDelegate::DATA_TYPE_FORM_DATA;