Sync test changes in preparation for transport-only sync

These should allow tests in upcoming patches to mimic the experimentally
supported scenario where sync the feature is disabled, but the sync
machinery (the engine acting as a transport mechanism) is active as soon
as the user is signed in, in a special mode (subset of datatypes, with
ephemeral storage).

As first user, we introduce a test for USER_CONSENTS.

Bug: 856179,866814,814307
Change-Id: Ie3b6c2da470dde691862a379977b4e01c4f2ead2
Reviewed-on: https://chromium-review.googlesource.com/1160852
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581137}

chrome/browser/metrics/ukm_browsertest.cc

@@ -204,23 +204,20 @@ class UkmBrowserTestBase : public SyncTest {
             GetFakeServer()->AsWeakPtr()));
 
     std::string username;
-    std::string gaia_id;
 #if defined(OS_CHROMEOS)
     // In browser tests, the profile may already by authenticated with stub
     // account |user_manager::kStubUserEmail|.
     AccountInfo info = SigninManagerFactory::GetForProfile(profile)
                            ->GetAuthenticatedAccountInfo();
     username = info.email;
-    gaia_id = info.gaia;
 #endif
     if (username.empty()) {
       username = "user@gmail.com";
-      gaia_id = "123456789";
     }
 
     std::unique_ptr<ProfileSyncServiceHarness> harness =
         ProfileSyncServiceHarness::Create(
-            profile, username, gaia_id, "unused" /* password */,
+            profile, username, "unused" /* password */,
             ProfileSyncServiceHarness::SigninType::FAKE_SIGNIN);
     EXPECT_TRUE(harness->SetupSync());
     return harness;

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

@@ -16,6 +16,7 @@
 #include "base/test/bind_test_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
+#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/signin/unified_consent_helper.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
@@ -38,6 +39,7 @@
 #include "components/unified_consent/unified_consent_service.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "services/identity/public/cpp/identity_manager.h"
+#include "services/identity/public/cpp/identity_test_utils.h"
 
 using browser_sync::ProfileSyncService;
 using syncer::SyncCycleSnapshot;
@@ -106,23 +108,20 @@ class SyncSetupChecker : public SingleClientStatusChangeChecker {
 std::unique_ptr<ProfileSyncServiceHarness> ProfileSyncServiceHarness::Create(
     Profile* profile,
     const std::string& username,
-    const std::string& gaia_id,
     const std::string& password,
     SigninType signin_type) {
-  return base::WrapUnique(new ProfileSyncServiceHarness(
-      profile, username, gaia_id, password, signin_type));
+  return base::WrapUnique(
+      new ProfileSyncServiceHarness(profile, username, password, signin_type));
 }
 
 ProfileSyncServiceHarness::ProfileSyncServiceHarness(
     Profile* profile,
     const std::string& username,
-    const std::string& gaia_id,
     const std::string& password,
     SigninType signin_type)
     : profile_(profile),
       service_(ProfileSyncServiceFactory::GetForProfile(profile)),
       username_(username),
-      gaia_id_(gaia_id),
       password_(password),
       signin_type_(signin_type),
       oauth2_refesh_token_number_(0),
@@ -153,6 +152,57 @@ bool ProfileSyncServiceHarness::SetupSyncForClearingServerData() {
   return result;
 }
 
+bool ProfileSyncServiceHarness::SignIn() {
+  // TODO(crbug.com/871221): This function should distinguish primary account
+  // (aka sync account) from secondary accounts (content area signin). Let's
+  // migrate tests that exercise transport-only sync to secondary accounts.
+  DCHECK(!username_.empty());
+
+  switch (signin_type_) {
+    case SigninType::UI_SIGNIN: {
+      Browser* browser = chrome::FindBrowserWithProfile(profile_);
+      DCHECK(browser);
+      if (!login_ui_test_utils::SignInWithUI(browser, username_, password_)) {
+        LOG(ERROR) << "Could not sign in to GAIA servers.";
+        return false;
+      }
+      return true;
+    }
+
+    case SigninType::FAKE_SIGNIN: {
+      identity::IdentityManager* identity_manager =
+          IdentityManagerFactory::GetForProfile(profile_);
+      ProfileOAuth2TokenService* token_service =
+          ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
+
+      // Verify HasPrimaryAccount() separately because
+      // MakePrimaryAccountAvailable() below DCHECK fails if there is already
+      // an authenticated account.
+      if (identity_manager->HasPrimaryAccount()) {
+        DCHECK_EQ(identity_manager->GetPrimaryAccountInfo().email, username_);
+        // Don't update the refresh token if we already have one. The reason is
+        // that doing so causes Sync (ServerConnectionManager in particular) to
+        // mark the current access token as invalid. Since tests typically
+        // always hand out the same access token string, any new access token
+        // acquired later would also be considered invalid.
+        if (!identity_manager->HasPrimaryAccountWithRefreshToken()) {
+          identity::SetRefreshTokenForPrimaryAccount(token_service,
+                                                     identity_manager);
+        }
+      } else {
+        // Authenticate sync client using GAIA credentials.
+        identity::MakePrimaryAccountAvailable(
+            SigninManagerFactory::GetForProfile(profile_), token_service,
+            identity_manager, username_);
+      }
+      return true;
+    }
+  }
+
+  NOTREACHED();
+  return false;
+}
+
 bool ProfileSyncServiceHarness::SetupSync(syncer::ModelTypeSet synced_datatypes,
                                           bool skip_passphrase_verification) {
   DCHECK(!profile_->IsLegacySupervised())
@@ -168,35 +218,8 @@ bool ProfileSyncServiceHarness::SetupSync(syncer::ModelTypeSet synced_datatypes,
   // until we've finished configuration.
   sync_blocker_ = service()->GetSetupInProgressHandle();
 
-  DCHECK(!username_.empty());
-  if (signin_type_ == SigninType::UI_SIGNIN) {
-    Browser* browser = chrome::FindBrowserWithProfile(profile_);
-    DCHECK(browser);
-    if (!login_ui_test_utils::SignInWithUI(browser, username_, password_)) {
-      LOG(ERROR) << "Could not sign in to GAIA servers.";
-      return false;
-    }
-  } else if (signin_type_ == SigninType::FAKE_SIGNIN) {
-    identity::IdentityManager* identity_manager =
-        IdentityManagerFactory::GetForProfile(profile_);
-    if (identity_manager->HasPrimaryAccountWithRefreshToken()) {
-      // Don't sign in again if we're already signed in. The reason is that
-      // changing the refresh token causes Sync (ServerConnectionManager in
-      // particular) to mark the current access token as invalid. Since tests
-      // typically always hand out the same access token string, any new access
-      // token acquired later would also be considered invalid.
-      DCHECK_EQ(identity_manager->GetPrimaryAccountInfo().gaia, gaia_id_);
-      DCHECK_EQ(identity_manager->GetPrimaryAccountInfo().email, username_);
-    } else {
-      // Authenticate sync client using GAIA credentials.
-      // TODO(https://crbug.com/814307): This ideally should go through
-      // identity_test_utils.h (and in the long run IdentityTestEnvironment),
-      // but making that change is complex for reasons described in the bug.
-      identity_manager->SetPrimaryAccountSynchronouslyForTests(
-          gaia_id_, username_, GenerateFakeOAuth2RefreshTokenString());
-    }
-  } else {
-    LOG(ERROR) << "Unsupported profile signin type.";
+  if (!SignIn()) {
+    return false;
   }
 
   // Now that auth is completed, request that sync actually start.
@@ -280,7 +303,7 @@ bool ProfileSyncServiceHarness::StartSyncService() {
   DVLOG(1) << "Requesting start for service";
   service()->RequestStart();
 
-  if (!AwaitEngineInitialization(/*skip_passphrase_verification=*/false)) {
+  if (!AwaitEngineInitialization()) {
     LOG(ERROR) << "AwaitEngineInitialization failed.";
     return false;
   }

chrome/browser/sync/test/integration/profile_sync_service_harness.h

@@ -39,11 +39,14 @@ class ProfileSyncServiceHarness {
   static std::unique_ptr<ProfileSyncServiceHarness> Create(
       Profile* profile,
       const std::string& username,
-      const std::string& gaia_id,
       const std::string& password,
       SigninType signin_type);
   virtual ~ProfileSyncServiceHarness();
 
+  // Creates a ProfileSyncService for the profile passed at construction and
+  // signs in without actually enabling sync the feature.
+  bool SignIn();
+
   // Creates a ProfileSyncService for the profile passed at construction and
   // enables sync for all available datatypes. Returns true only after sync has
   // been fully initialized and authenticated, and we are ready to process
@@ -114,7 +117,7 @@ class ProfileSyncServiceHarness {
   // (e.g., auth error) is reached. Returns true if and only if the engine
   // initialized successfully. See ProfileSyncService's IsEngineInitialized()
   // method for the definition of engine initialization.
-  bool AwaitEngineInitialization(bool skip_passphrase_verification);
+  bool AwaitEngineInitialization(bool skip_passphrase_verification = false);
 
   // Blocks the caller until sync setup is complete. Returns true if and only
   // if sync setup completed successfully. See syncer::SyncService's
@@ -160,7 +163,6 @@ class ProfileSyncServiceHarness {
  private:
   ProfileSyncServiceHarness(Profile* profile,
                             const std::string& username,
-                            const std::string& gaia_id,
                             const std::string& password,
                             SigninType signin_type);
 
@@ -182,7 +184,6 @@ class ProfileSyncServiceHarness {
 
   // Credentials used for GAIA authentication.
   std::string username_;
-  std::string gaia_id_;
   std::string password_;
 
   // Used to decide what method of profile signin to use.

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

@@ -16,11 +16,6 @@
 
 namespace {
 
-// Returns true if this service is disabled.
-bool IsSyncDisabled(browser_sync::ProfileSyncService* service) {
-  return !service->IsSetupInProgress() && !service->IsFirstSetupComplete();
-}
-
 // Returns true if these services have matching progress markers.
 bool ProgressMarkersMatch(const browser_sync::ProfileSyncService* service1,
                           const browser_sync::ProfileSyncService* service2) {
@@ -103,10 +98,6 @@ bool QuiesceStatusChangeChecker::IsExitConditionSatisfied() {
   // Check that all progress markers are up to date.
   std::vector<browser_sync::ProfileSyncService*> enabled_services;
   for (const auto& checker : checkers_) {
-    if (IsSyncDisabled(checker->service())) {
-      continue;  // Skip disabled services.
-    }
-
     enabled_services.push_back(checker->service());
 
     if (!checker->IsExitConditionSatisfied()) {

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

@@ -31,7 +31,7 @@ std::string GetAccountId() {
   // impossible to discover otherwise.
   return "user@gmail.com";
 #else
-  return "gaia-id-user@gmail.com";
+  return "gaia_id_for_user@gmail.com";
 #endif
 }
 
@@ -135,7 +135,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserConsentsSyncTest,
 
 IN_PROC_BROWSER_TEST_F(
     SingleClientUserConsentsSyncTest,
-    ShouldPreserveConsentsOnDisableSyncAndResubmitWhenReneabled) {
+    ShouldPreserveConsentsOnDisableSyncAndResubmitWhenReenabled) {
   SetSyncUserConsentSeparateTypeFeature(true);
 
   UserConsentSpecifics specifics;
@@ -159,4 +159,46 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_TRUE(ExpectUserConsents({specifics}));
 }
 
+// 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)
+IN_PROC_BROWSER_TEST_F(SingleClientUserConsentsSyncTest,
+                       ShouldSubmitIfSignedInAlthoughFullSyncNotEnabled) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures(
+      // Enabled.
+      {switches::kSyncStandaloneTransport,
+       switches::kSyncUserConsentSeparateType},
+      // Disabled.
+      {});
+
+  // We avoid calling SetupSync(), because we don't want to turn on full sync,
+  // only sign in such that the standalone transport starts.
+  ASSERT_TRUE(SetupClients());
+  ASSERT_TRUE(GetClient(0)->SignIn());
+  ASSERT_TRUE(GetClient(0)->AwaitEngineInitialization());
+  ASSERT_TRUE(AwaitQuiescence());
+  ASSERT_FALSE(GetSyncService(0)->IsSyncActive())
+      << "Full sync should be disabled";
+  ASSERT_EQ(syncer::SyncService::TransportState::ACTIVE,
+            GetSyncService(0)->GetTransportState());
+  ASSERT_TRUE(
+      GetSyncService(0)->GetActiveDataTypes().Has(syncer::USER_CONSENTS));
+
+  SyncConsent sync_consent;
+  sync_consent.set_confirmation_grd_id(1);
+  sync_consent.set_status(UserConsentTypes::GIVEN);
+
+  ConsentAuditorFactory::GetForProfile(GetProfile(0))
+      ->RecordSyncConsent(GetAccountId(), sync_consent);
+
+  UserConsentSpecifics specifics;
+  specifics.set_confirmation_grd_id(1);
+  // Account id may be compared to the synced account, thus, we need them to
+  // match.
+  specifics.set_account_id(GetAccountId());
+  EXPECT_TRUE(ExpectUserConsents({specifics}));
+}
+#endif  // !defined(OS_CHROMEOS)
+
 }  // namespace

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

@@ -258,8 +258,7 @@ IN_PROC_BROWSER_TEST_F(SyncErrorTest, ClientDataObsoleteTest) {
   // Make server return SUCCESS so that sync can initialize.
   EXPECT_TRUE(GetFakeServer()->TriggerError(sync_pb::SyncEnums::SUCCESS));
 
-  ASSERT_TRUE(GetClient(0)->AwaitEngineInitialization(
-      /*skip_passphrase_verification=*/false));
+  ASSERT_TRUE(GetClient(0)->AwaitEngineInitialization());
 
   // Ensure cache_guid changed.
   GetSyncService(0)->QueryDetailedSyncStatus(&status);

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

@@ -546,8 +546,12 @@ bool SyncTest::SetupClients() {
       LOG(FATAL) << "Could not create Gaia account.";
   }
 
+  auto* cl = base::CommandLine::ForCurrentProcess();
+  if (!cl->HasSwitch(switches::kSyncDeferredStartupTimeoutSeconds)) {
+    cl->AppendSwitchASCII(switches::kSyncDeferredStartupTimeoutSeconds, "1");
+  }
+
 #if defined(OS_CHROMEOS)
-  const auto* cl = base::CommandLine::ForCurrentProcess();
   // ARC_PACKAGE do not support supervised users, switches::kSupervisedUserId
   // need to be set in SetUpCommandLine() when a test will use supervise users.
   if (!cl->HasSwitch(switches::kSupervisedUserId)) {
@@ -622,8 +626,7 @@ void SyncTest::InitializeProfile(int index, Profile* profile) {
 
   DCHECK(!clients_[index]);
   clients_[index] = ProfileSyncServiceHarness::Create(
-      GetProfile(index), username_, "gaia-id-" + username_, password_,
-      singin_type);
+      GetProfile(index), username_, password_, singin_type);
   EXPECT_NE(nullptr, GetClient(index)) << "Could not create Client " << index;
   InitializeInvalidations(index);
 }

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

@@ -32,12 +32,11 @@ bool UpdatedProgressMarkerChecker::IsExitConditionSatisfied() {
   //    GetLastCycleSnapshot() prior to the first sync cycle).
   // 2. Our last sync cycle committed no changes (because commits are followed
   //    by the test-only 'self-notify' cycle).
-  // 3. Sync is still active (e.g. no failures).
-  // 4. No pending local changes (which will ultimately generate new progress
+  // 3. No pending local changes (which will ultimately generate new progress
   //    markers once submitted to the server).
   return !snap.download_progress_markers().empty() &&
          snap.model_neutral_state().num_successful_commits == 0 &&
-         service()->IsSyncActive() && !has_unsynced_items_.value();
+         !has_unsynced_items_.value();
 }
 
 void UpdatedProgressMarkerChecker::GotHasUnsyncedItems(

components/sync/driver/async_directory_type_controller.cc

@@ -47,8 +47,8 @@ void AsyncDirectoryTypeController::LoadModels(
     const ConfigureContext& configure_context,
     const ModelLoadCallback& model_load_callback) {
   DCHECK(CalledOnValidThread());
-  DCHECK_EQ(configure_context.storage_option,
-            ConfigureContext::STORAGE_ON_DISK);
+  DCHECK_EQ(configure_context.storage_option, ConfigureContext::STORAGE_ON_DISK)
+      << " for type " << ModelTypeToString(type());
 
   model_load_callback_ = model_load_callback;
 

services/identity/public/cpp/identity_manager.h

@@ -37,7 +37,6 @@ class MultiProfileFilesAppBrowserTest;
 // Necessary to declare these classes as friends.
 class ArcSupportHostTest;
 class MultiProfileDownloadNotificationTest;
-class ProfileSyncServiceHarness;
 
 namespace identity {
 
@@ -214,7 +213,6 @@ class IdentityManager : public SigninManagerBase::Observer,
                                        IdentityManager* identity_manager,
                                        const std::string& email);
   friend MultiProfileDownloadNotificationTest;
-  friend ProfileSyncServiceHarness;
   friend file_manager::MultiProfileFilesAppBrowserTest;
 
   // These clients needs to call SetPrimaryAccountSynchronously().