Wallet USS: clear data on signout

Make sure the USS implementation also clears data on signout, just like
the directory-based implementation does. This also enables an
integration test to test this in the butter scenario.

Also making a small fix: Updating the storage in PDM in
OnSyncInitialized, because if sync is disabled, we won't get the
OnStateChanged call.

Bug: 880735
Change-Id: I6aa49b8c459a48cb92a1e3d7283f05eed8e6eb5c
Reviewed-on: https://chromium-review.googlesource.com/1206490
Commit-Queue: Florian Uunk <feuunk@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589197}

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

@@ -353,8 +353,15 @@ class SingleClientWalletSyncTest : public UssSwitchToggler, public SyncTest {
  protected:
   void RefreshAndWaitForOnPersonalDataChanged(
       autofill::PersonalDataManager* pdm) {
+    WaitForOnPersonalDataChanged(/*should_trigger_refresh=*/true, pdm);
+  }
+
+  void WaitForOnPersonalDataChanged(bool should_trigger_refresh,
+                                    autofill::PersonalDataManager* pdm) {
     pdm->AddObserver(&personal_data_observer_);
+    if (should_trigger_refresh) {
       pdm->Refresh();
+    }
     base::RunLoop run_loop;
     EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged())
         .WillRepeatedly(QuitMessageLoop(&run_loop));
@@ -412,11 +419,12 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest, DownloadProfileStorage) {
 // ChromeOS does not support late signin after profile creation, so the test
 // below does not apply, at least in the current form.
 #if !defined(OS_CHROMEOS)
-// TODO(crbug.com/853688): Reenable once the USS implementation of
-// AUTOFILL_WALLET_DATA (AutofillWalletSyncBridge) has sufficient functionality.
-IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest,
-                       DISABLED_DownloadAccountStorage_Card) {
-  InitWithFeatures(
+// The account storage requires USS, so we only test the USS implementation
+// here.
+IN_PROC_BROWSER_TEST_F(SingleClientWalletSyncTest,
+                       DownloadAccountStorage_Card) {
+  base::test::ScopedFeatureList features;
+  features.InitWithFeatures(
       /*enabled_features=*/{switches::kSyncStandaloneTransport,
                             switches::kSyncUSSAutofillWalletData,
                             autofill::features::
@@ -424,6 +432,8 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest,
       /*disabled_features=*/{});
 
   ASSERT_TRUE(SetupClients());
+  autofill::PersonalDataManager* pdm = GetPersonalDataManager(0);
+  pdm->OnSyncServiceInitialized(GetSyncService(0));
 
   GetFakeServer()->SetWalletData(
       {CreateDefaultSyncWalletAddress(), CreateDefaultSyncWalletCard()});
@@ -446,19 +456,32 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest,
   EXPECT_EQ(0U, GetServerCards(profile_data).size());
   EXPECT_EQ(0U, GetServerProfiles(profile_data).size());
 
-  // Check that one card and one profile are stored in the account storage.
+  // Check that one card is stored in the account storage.
   EXPECT_EQ(1U, GetServerCards(account_data).size());
-  EXPECT_EQ(1U, GetServerProfiles(account_data).size());
 
-  autofill::PersonalDataManager* pdm = GetPersonalDataManager(0);
   ASSERT_NE(nullptr, pdm);
   std::vector<CreditCard*> cards = pdm->GetCreditCards();
   ASSERT_EQ(1uL, cards.size());
-  std::vector<AutofillProfile*> profiles = pdm->GetServerProfiles();
-  ASSERT_EQ(1uL, profiles.size());
 
   ExpectDefaultCreditCardValues(*cards[0]);
-  ExpectDefaultProfileValues(*profiles[0]);
+
+  // Now sign back out.
+  GetClient(0)->SignOutPrimaryAccount();
+
+  // Verify that sync is stopped.
+  ASSERT_EQ(syncer::SyncService::TransportState::DISABLED,
+            GetSyncService(0)->GetTransportState());
+  ASSERT_FALSE(GetSyncService(0)->GetActiveDataTypes().Has(
+      syncer::AUTOFILL_WALLET_DATA));
+
+  // Wait for PDM to receive the data change.
+  WaitForOnPersonalDataChanged(/*should_trigger_refresh=*/false, pdm);
+
+  // Check that the account storage is now cleared.
+  EXPECT_EQ(0U, GetServerCards(account_data).size());
+
+  cards = pdm->GetCreditCards();
+  EXPECT_EQ(0U, cards.size());
 }
 #endif  // !defined(OS_CHROMEOS)
 
@@ -481,6 +504,27 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest, ClearOnDisableSync) {
   ASSERT_EQ(0uL, cards.size());
 }
 
+// ChromeOS does not sign out, so the test below does not apply.
+#if !defined(OS_CHROMEOS)
+// Wallet data should get cleared from the database when the user signs out.
+IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest, ClearOnSignOut) {
+  InitWithDefaultFeatures();
+  GetFakeServer()->SetWalletData(
+      {CreateDefaultSyncWalletAddress(), CreateDefaultSyncWalletCard()});
+  ASSERT_TRUE(SetupSync());
+
+  // Make sure the card is in the DB.
+  autofill::PersonalDataManager* pdm = GetPersonalDataManager(0);
+  ASSERT_NE(nullptr, pdm);
+  ASSERT_EQ(1uL, pdm->GetCreditCards().size());
+
+  // Turn off sync, the card should be gone.
+  GetClient(0)->SignOutPrimaryAccount();
+
+  ASSERT_EQ(0uL, pdm->GetCreditCards().size());
+}
+#endif  // !defined(OS_CHROMEOS)
+
 // Wallet is not using incremental updates. Make sure existing data gets
 // replaced when synced down.
 IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest,

components/autofill/core/browser/personal_data_manager.cc

@@ -433,9 +433,11 @@ class PersonalDatabaseHelper
       return;
     }
 
-    if (server_database_ != nullptr && server_database_ != profile_database_) {
+    if (server_database_ != nullptr) {
+      if (server_database_ != profile_database_) {
         // Remove the previous observer if we had any.
         server_database_->RemoveObserver(personal_data_manager_);
+      }
       personal_data_manager_->CancelPendingServerQueries();
     }
     server_database_ = new_server_database;
@@ -558,6 +560,13 @@ void PersonalDataManager::OnSyncServiceInitialized(
       ResetFullServerCards(/*dry_run=*/!base::FeatureList::IsEnabled(
           features::kAutofillResetFullServerCardsOnAuthError));
     }
+    if (base::FeatureList::IsEnabled(
+            autofill::features::kAutofillEnableAccountWalletStorage)) {
+      // Use the ephemeral account storage when the user didn't enable the sync
+      // feature explicitly.
+      database_helper_->SetUseAccountStorageForServerCards(
+          !sync_service->IsSyncFeatureEnabled());
+    }
   }
 }
 
@@ -1859,6 +1868,9 @@ void PersonalDataManager::CancelPendingServerQueries() {
   if (pending_server_creditcards_query_) {
     CancelPendingServerQuery(&pending_server_creditcards_query_);
   }
+  if (pending_customer_data_query_) {
+    CancelPendingServerQuery(&pending_customer_data_query_);
+  }
   // TODO(crbug.com/864519): also cancel the server addresses query once they
   // use the account storage.
 }

components/autofill/core/browser/personal_data_manager_unittest.cc

@@ -179,8 +179,8 @@ class PersonalDataManagerTestBase {
             : nullptr,
         prefs_.get(), identity_test_env_.identity_manager(), is_incognito);
     personal_data_->AddObserver(&personal_data_observer_);
-    personal_data_->OnSyncServiceInitialized(&sync_service_);
     sync_service_.SetIsAuthenticatedAccountPrimary(!use_account_server_storage);
+    personal_data_->OnSyncServiceInitialized(&sync_service_);
     personal_data_->OnStateChanged(&sync_service_);
 
     // Verify that the web database has been updated and the notification sent.
@@ -6206,6 +6206,8 @@ TEST_F(PersonalDataManagerTest,
 
   // Call OnSyncServiceInitialized with a sync service in auth error.
   TestSyncService sync_service;
+  sync_service.SetIsAuthenticatedAccountPrimary(
+      /*is_authenticated_account_primary=*/false);
   sync_service.SetInAuthError(true);
   personal_data_->OnSyncServiceInitialized(&sync_service);
   WaitForOnPersonalDataChanged();

components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.cc

@@ -187,6 +187,17 @@ bool AutofillWalletSyncBridge::SupportsIncrementalUpdates() const {
   return false;
 }
 
+AutofillWalletSyncBridge::StopSyncResponse
+AutofillWalletSyncBridge::ApplyStopSyncChanges(
+    std::unique_ptr<syncer::MetadataChangeList> delete_metadata_change_list) {
+  // If a metadata change list gets passed in, that means sync is actually
+  // disabled, so we want to delete the payments data.
+  if (delete_metadata_change_list) {
+    SetSyncData(syncer::EntityChangeList());
+  }
+  return StopSyncResponse::kModelStillReadyToSync;
+}
+
 void AutofillWalletSyncBridge::SetSyncData(
     const syncer::EntityChangeList& entity_data) {
   bool wallet_data_changed = false;

components/autofill/core/browser/webdata/autofill_wallet_sync_bridge.h

@@ -63,6 +63,9 @@ class AutofillWalletSyncBridge : public base::SupportsUserData::Data,
   std::string GetClientTag(const syncer::EntityData& entity_data) override;
   std::string GetStorageKey(const syncer::EntityData& entity_data) override;
   bool SupportsIncrementalUpdates() const override;
+  StopSyncResponse ApplyStopSyncChanges(
+      std::unique_ptr<syncer::MetadataChangeList> delete_metadata_change_list)
+      override;
 
  private:
   struct AutofillWalletDiff {

components/autofill/core/browser/webdata/autofill_wallet_sync_bridge_unittest.cc

@@ -32,6 +32,7 @@
 #include "components/sync/model/mock_model_type_change_processor.h"
 #include "components/sync/model/sync_data.h"
 #include "components/sync/model_impl/client_tag_based_model_type_processor.h"
+#include "components/sync/model_impl/in_memory_metadata_change_list.h"
 #include "components/sync/protocol/autofill_specifics.pb.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync/test/test_matchers.h"
@@ -606,6 +607,36 @@ TEST_F(AutofillWalletSyncBridgeTest, LoadMetadataCalled) {
   ResetBridge();
 }
 
+TEST_F(AutofillWalletSyncBridgeTest, ApplyStopSyncChanges_ClearAllData) {
+  // Create one profile and one card on the client.
+  AutofillProfile local_profile = test::GetServerProfile();
+  table()->SetServerProfiles({local_profile});
+  CreditCard local_card = test::GetMaskedServerCard();
+  table()->SetServerCreditCards({local_card});
+
+  EXPECT_CALL(*backend(), NotifyOfMultipleAutofillChanges());
+  // Passing in a non-null metadata change list indicates to the bridge that
+  // sync is stopping because it was disabled.
+  bridge()->ApplyStopSyncChanges(
+      std::make_unique<syncer::InMemoryMetadataChangeList>());
+
+  EXPECT_TRUE(GetAllLocalData().empty());
+}
+
+TEST_F(AutofillWalletSyncBridgeTest, ApplyStopSyncChanges_KeepData) {
+  // Create one profile and one card on the client.
+  AutofillProfile local_profile = test::GetServerProfile();
+  table()->SetServerProfiles({local_profile});
+  CreditCard local_card = test::GetMaskedServerCard();
+  table()->SetServerCreditCards({local_card});
+
+  // Passing in a non-null metadata change list indicates to the bridge that
+  // sync is stopping but the data type is not disabled.
+  bridge()->ApplyStopSyncChanges(/*delete_metadata_change_list=*/nullptr);
+
+  EXPECT_FALSE(GetAllLocalData().empty());
+}
+
 class AutofillWalletEphemeralSyncBridgeTest
     : public AutofillWalletSyncBridgeTest {
  public: