Decouple Sync-the-transport from Sync-the-feature, part 2: non-primary accounts

This adds support for starting Sync-the-transport even when there is no
primary (aka signed-into-Chrome aka blessed-for-Sync-the-feature)
account.
This is implemented by SyncAuthManager listening for OnAccountsInCookieUpdated,
and using the first cookie account if there is no primary account and we have
a refresh token for the first cookie account. This also makes it necessary to
cache the current Sync account, so that we can detect changes to it.

IsSyncFeatureEnabled now also checks that the current account is the primary
one, so that we don't accidentally enable full Sync for a secondary account.

Bug: 871221
Change-Id: I07c7db04cc92e9a40126c89c0fcfea49e03117d1
Reviewed-on: https://chromium-review.googlesource.com/1162174
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587082}

components/browser_sync/profile_sync_service.cc

@@ -453,7 +453,14 @@ void ProfileSyncService::AccountStateChanged() {
     StopImpl(CLEAR_DATA);
     DCHECK(!engine_);
   } else {
+#if !defined(OS_CHROMEOS)
+    // TODO(crbug.com/814787): SyncAuthManager shouldn't call us again if we
+    // already have the signed-in account, and hence we shouldn't have an engine
+    // here, but some tests on ChromeOS set the account without notifying, which
+    // get us into an inconsistent state. Since calling TryStart() again in that
+    // case isn't harmful, skip the DCHECK on ChromeOS for now.
     DCHECK(!engine_);
+#endif
     startup_controller_->TryStart(/*force_immediate=*/IsSetupInProgress());
   }
 }
@@ -1614,7 +1621,7 @@ void ProfileSyncService::ConfigureDataTypeManager(
   syncer::ModelTypeSet types = GetPreferredDataTypes();
   // If Sync-the-feature isn't fully enabled, then only a subset of data types
   // is supported.
-  if (!IsSyncFeatureEnabled()) {
+  if (!IsSyncFeatureEnabled() && !IsLocalSyncEnabled()) {
     DCHECK(IsStandaloneTransportEnabled());
     syncer::ModelTypeSet allowed_types = {syncer::USER_CONSENTS};
 
@@ -2011,7 +2018,12 @@ void ProfileSyncService::GetAllNodes(
 
 AccountInfo ProfileSyncService::GetAuthenticatedAccountInfo() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return auth_manager_->GetAuthenticatedAccountInfo();
+  return auth_manager_->GetActiveAccountInfo().account_info;
+}
+
+bool ProfileSyncService::IsAuthenticatedAccountPrimary() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return auth_manager_->GetActiveAccountInfo().is_primary;
 }
 
 syncer::GlobalIdMapper* ProfileSyncService::GetGlobalIdMapper() const {

components/browser_sync/profile_sync_service.h

@@ -293,6 +293,7 @@ class ProfileSyncService : public syncer::SyncService,
   void GetAllNodes(const base::Callback<void(std::unique_ptr<base::ListValue>)>&
                        callback) override;
   AccountInfo GetAuthenticatedAccountInfo() const override;
+  bool IsAuthenticatedAccountPrimary() const override;
   syncer::GlobalIdMapper* GetGlobalIdMapper() const override;
 
   // Add a sync type preference provider. Each provider may only be added once.

components/browser_sync/profile_sync_service_mock.cc

@@ -24,6 +24,10 @@ ProfileSyncServiceMock::GetOpenTabsUIDelegate() {
                        : ProfileSyncService::GetOpenTabsUIDelegate();
 }
 
+bool ProfileSyncServiceMock::IsAuthenticatedAccountPrimary() const {
+  return true;
+}
+
 std::unique_ptr<syncer::SyncSetupInProgressHandle>
 ProfileSyncServiceMock::GetSetupInProgressHandleConcrete() {
   return browser_sync::ProfileSyncService::GetSetupInProgressHandle();

components/browser_sync/profile_sync_service_mock.h

@@ -102,6 +102,11 @@ class ProfileSyncServiceMock : public ProfileSyncService {
 
   MOCK_METHOD0(OnSetupInProgressHandleDestroyed, void());
 
+  // TODO(crbug.com/871221): Remove this override. This is overridden here to
+  // return true by default, as a workaround for tests not setting up an
+  // authenticated account and IsSyncFeatureEnabled() therefore returning false.
+  bool IsAuthenticatedAccountPrimary() const override;
+
   // Gives access to the real implementation of ProfileSyncService methods:
   std::unique_ptr<syncer::SyncSetupInProgressHandle>
   GetSetupInProgressHandleConcrete();

components/browser_sync/profile_sync_service_startup_unittest.cc

@@ -634,16 +634,14 @@ TEST_F(ProfileSyncServiceStartupTest, ManagedStartup) {
   CreateSyncService(ProfileSyncService::MANUAL_START);
 
   // Disable sync through policy.
-  ASSERT_EQ(syncer::SyncService::DISABLE_REASON_NONE,
-            sync_service()->GetDisableReasons());
   pref_service()->SetBoolean(syncer::prefs::kSyncManaged, true);
-  ASSERT_EQ(syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY,
-            sync_service()->GetDisableReasons());
 
   EXPECT_CALL(*component_factory(), CreateSyncEngine(_, _, _, _)).Times(0);
   EXPECT_CALL(*component_factory(), CreateDataTypeManager(_, _, _, _, _, _))
       .Times(0);
   sync_service()->Initialize();
+  EXPECT_EQ(syncer::SyncService::DISABLE_REASON_ENTERPRISE_POLICY,
+            sync_service()->GetDisableReasons());
 }
 
 TEST_F(ProfileSyncServiceWithoutStandaloneTransportStartupTest, SwitchManaged) {

components/browser_sync/sync_auth_manager.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
@@ -20,7 +21,7 @@
 #include "services/identity/public/cpp/identity_manager.h"
 
 namespace identity {
-class PrimaryAccountAccessTokenFetcher;
+class AccessTokenFetcher;
 }
 
 namespace syncer {
@@ -30,12 +31,24 @@ class SyncPrefs;
 
 namespace browser_sync {
 
-// SyncAuthManager tracks the primary (i.e. blessed-for-sync) account and its
-// authentication state.
+// SyncAuthManager tracks the account to be used for Sync and its authentication
+// state. Note that this account may or may not be the primary account (as per
+// IdentityManager::GetPrimaryAccountInfo() etc).
 class SyncAuthManager : public identity::IdentityManager::Observer {
  public:
-  // Called when the existence of an authenticated account changes. Call
-  // GetAuthenticatedAccountInfo to get the new state.
+  struct SyncAccountInfo {
+    SyncAccountInfo();
+    SyncAccountInfo(const AccountInfo& account_info, bool is_primary);
+
+    AccountInfo account_info;
+    bool is_primary = false;
+  };
+
+  // Called when the existence of an authenticated account changes. It's
+  // guaranteed that this is only called for going from "no account" to "have
+  // account" or vice versa, i.e. SyncAuthManager will never directly switch
+  // from one account to a different one. Call GetActiveAccountInfo to get the
+  // new state.
   using AccountStateChangedCallback = base::RepeatingClosure;
   // Called when the credential state changes, i.e. an access token was
   // added/changed/removed. Call GetCredentials to get the new state.
@@ -52,12 +65,14 @@ class SyncAuthManager : public identity::IdentityManager::Observer {
 
   // Tells the tracker to start listening for changes to the account/sign-in
   // status. This gets called during SyncService initialization, except in the
-  // case of local Sync.
+  // case of local Sync. Before this is called, GetActiveAccountInfo will always
+  // return an empty AccountInfo. Note that this will *not* trigger any
+  // callbacks, even if there is an active account afterwards.
   void RegisterForAuthNotifications();
 
-  // Returns the AccountInfo for the primary (i.e. blessed-for-sync) account, or
-  // an empty AccountInfo if there isn't one.
-  AccountInfo GetAuthenticatedAccountInfo() const;
+  // Returns the account which should be used when communicating with the Sync
+  // server. Note that this account may not be blessed for Sync-the-feature.
+  SyncAccountInfo GetActiveAccountInfo() const;
 
   const GoogleServiceAuthError& GetLastAuthError() const {
     return last_auth_error_;
@@ -88,12 +103,24 @@ class SyncAuthManager : public identity::IdentityManager::Observer {
                                        bool is_valid) override;
   void OnRefreshTokenRemovedForAccount(
       const AccountInfo& account_info) override;
+  void OnAccountsInCookieUpdated(
+      const std::vector<AccountInfo>& accounts) override;
 
   // Test-only methods for inspecting/modifying internal state.
   bool IsRetryingAccessTokenFetchForTest() const;
   void ResetRequestAccessTokenBackoffForTest();
 
  private:
+  // Determines which account should be used for Sync and returns the
+  // corresponding AccountInfo.
+  SyncAccountInfo DetermineAccountToUse() const;
+
+  // Updates |sync_account_| to the appropriate account (i.e.
+  // DetermineAccountToUse) if necessary, and notifies observers of any changes
+  // (sign-in/sign-out/"primary" bit change). Note that changing from one
+  // account to another is exposed to observers as a sign-out + sign-in.
+  bool UpdateSyncAccountIfNecessary();
+
   void ClearAccessTokenAndRequest();
 
   void RequestAccessToken();
@@ -107,7 +134,18 @@ class SyncAuthManager : public identity::IdentityManager::Observer {
   const AccountStateChangedCallback account_state_changed_callback_;
   const CredentialsChangedCallback credentials_changed_callback_;
 
-  bool registered_for_auth_notifications_;
+  bool registered_for_auth_notifications_ = false;
+
+  // The account which we are using to sync. If this is non-empty, that does
+  // *not* necessarily imply that Sync is actually running, e.g. because of
+  // delayed startup.
+  SyncAccountInfo sync_account_;
+
+  // Whether we're currently switching the active account, or its |is_primary|
+  // bit. During this time, |sync_account_| won't necessarily match
+  // |DetermineAccountToUse()|. This is set while we're updating the observers
+  // of an account change during UpdateSyncAccountIfNecessary.
+  bool currently_switching_account_ = false;
 
   // This is a cache of the last authentication response we received either
   // from the sync server or from Chrome's identity/token management system.
@@ -125,8 +163,7 @@ class SyncAuthManager : public identity::IdentityManager::Observer {
 
   // Pending request for an access token. Non-null iff there is a request
   // ongoing.
-  std::unique_ptr<identity::PrimaryAccountAccessTokenFetcher>
-      ongoing_access_token_fetch_;
+  std::unique_ptr<identity::AccessTokenFetcher> ongoing_access_token_fetch_;
 
   // If RequestAccessToken fails with transient error then retry requesting
   // access token with exponential backoff.

components/browser_sync/test_profile_sync_service.cc

@@ -31,6 +31,10 @@ void TestProfileSyncService::OnConfigureDone(
   base::RunLoop::QuitCurrentWhenIdleDeprecated();
 }
 
+bool TestProfileSyncService::IsAuthenticatedAccountPrimary() const {
+  return true;
+}
+
 syncer::UserShare* TestProfileSyncService::GetUserShare() const {
   return engine_->GetUserShare();
 }

components/browser_sync/test_profile_sync_service.h

@@ -27,6 +27,11 @@ class TestProfileSyncService : public ProfileSyncService {
   void OnConfigureDone(
       const syncer::DataTypeManager::ConfigureResult& result) override;
 
+  // TODO(crbug.com/871221): This is overridden here to return true by default,
+  // as a workaround for tests not setting up an authenticated account, and
+  // IsSyncFeatureEnabled() therefore returning false.
+  bool IsAuthenticatedAccountPrimary() const override;
+
   // We implement our own version to avoid some DCHECKs.
   syncer::UserShare* GetUserShare() const override;
 

components/sync/driver/fake_sync_service.cc

@@ -58,6 +58,10 @@ syncer::SyncService::TransportState FakeSyncService::GetTransportState() const {
   return TransportState::ACTIVE;
 }
 
+bool FakeSyncService::IsAuthenticatedAccountPrimary() const {
+  return true;
+}
+
 bool FakeSyncService::IsFirstSetupComplete() const {
   return false;
 }

components/sync/driver/fake_sync_service.h

@@ -41,6 +41,7 @@ class FakeSyncService : public SyncService {
   // SyncService implementation.
   int GetDisableReasons() const override;
   TransportState GetTransportState() const override;
+  bool IsAuthenticatedAccountPrimary() const override;
   bool IsFirstSetupComplete() const override;
   bool IsLocalSyncEnabled() const override;
   void TriggerRefresh(const ModelTypeSet& types) override;

components/sync/driver/sync_driver_switches.cc

@@ -45,6 +45,12 @@ const base::Feature kSyncClearDataOnPassphraseEncryption{
 const base::Feature kSyncStandaloneTransport{"SyncStandaloneTransport",
                                              base::FEATURE_DISABLED_BY_DEFAULT};
 
+// If enabled, allows the Sync machinery to start with a signed-in account that
+// has *not* been chosen as Chrome's primary account (see IdentityManager). Only
+// has an effect if SyncStandaloneTransport is also enabled.
+const base::Feature kSyncSupportSecondaryAccount{
+    "SyncSupportSecondaryAccount", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Gates registration and construction of user events machinery. Enabled by
 // default as each use case should have their own gating feature as well.
 const base::Feature kSyncUserEvents{"SyncUserEvents",

components/sync/driver/sync_driver_switches.h

@@ -22,6 +22,7 @@ extern const char kSyncShortNudgeDelayForTest[];
 
 extern const base::Feature kSyncClearDataOnPassphraseEncryption;
 extern const base::Feature kSyncStandaloneTransport;
+extern const base::Feature kSyncSupportSecondaryAccount;
 extern const base::Feature kSyncUserEvents;
 extern const base::Feature kSyncUserFieldTrialEvents;
 extern const base::Feature kSyncUserConsentEvents;

components/sync/driver/sync_service.cc

@@ -14,7 +14,11 @@ SyncSetupInProgressHandle::~SyncSetupInProgressHandle() {
 }
 
 bool SyncService::IsSyncFeatureEnabled() const {
-  return GetDisableReasons() == DISABLE_REASON_NONE && IsFirstSetupComplete();
+  // Note: IsFirstSetupComplete() shouldn't usually be true if we don't have a
+  // primary account, but it could happen if the account changes from primary to
+  // secondary.
+  return GetDisableReasons() == DISABLE_REASON_NONE && IsFirstSetupComplete() &&
+         IsAuthenticatedAccountPrimary();
 }
 
 bool SyncService::CanSyncStart() const {

components/sync/driver/sync_service.h

@@ -169,6 +169,10 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
 
   // Information about the currently signed in user.
   virtual AccountInfo GetAuthenticatedAccountInfo() const = 0;
+  // Whether the currently signed in user is the "primary" browser account (see
+  // IdentityManager). If this is false, then IsSyncFeatureEnabled will also be
+  // false, but Sync-the-transport might still run.
+  virtual bool IsAuthenticatedAccountPrimary() const = 0;
 
   // The last authentication error that was encountered by the SyncService. This
   // error can be either from Chrome's identity system (e.g. while trying to get