Stop session sync when user signs out (sync paused)

This is the most reliable way to make sure session-related information
is not tracked for syncing purposes while sync is paused (DICe signout).

It also means chrome://history and possibly other features will stop
exposing foreign tabs are soon as the user signs out.

Bug: 914747
Change-Id: I97e0922632e51fd2b25cc9cca686e4db8d7b7794
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1535889
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#644354}

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

@@ -21,9 +21,9 @@ namespace {
 
 using browser_sync::ProfileSyncService;
 
-class SyncActiveChecker : public SingleClientStatusChangeChecker {
+class SyncTransportActiveChecker : public SingleClientStatusChangeChecker {
  public:
-  explicit SyncActiveChecker(ProfileSyncService* service)
+  explicit SyncTransportActiveChecker(ProfileSyncService* service)
       : SingleClientStatusChangeChecker(service) {}
 
   bool IsExitConditionSatisfied() override {
@@ -70,7 +70,7 @@ IN_PROC_BROWSER_TEST_F(LocalSyncTest, ShouldStart) {
           browser()->profile());
 
   // Wait until the first sync cycle is completed.
-  ASSERT_TRUE(SyncActiveChecker(service).Wait());
+  ASSERT_TRUE(SyncTransportActiveChecker(service).Wait());
 
   EXPECT_TRUE(service->IsLocalSyncEnabled());
 }

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

@@ -52,25 +52,34 @@ constexpr char kMalformedOAuth2Token[] = R"({ "foo": )";
 // Waits until local changes are committed or an auth error is encountered.
 class TestForAuthError : public UpdatedProgressMarkerChecker {
  public:
-  explicit TestForAuthError(browser_sync::ProfileSyncService* service);
+  explicit TestForAuthError(browser_sync::ProfileSyncService* service)
+      : UpdatedProgressMarkerChecker(service) {}
 
   // StatusChangeChecker implementation.
-  bool IsExitConditionSatisfied() override;
-  std::string GetDebugMessage() const override;
+  bool IsExitConditionSatisfied() override {
+    return (service()->GetSyncTokenStatus().last_get_token_error.state() !=
+            GoogleServiceAuthError::NONE) ||
+           UpdatedProgressMarkerChecker::IsExitConditionSatisfied();
+  }
+
+  std::string GetDebugMessage() const override {
+    return "Waiting for auth error";
+  }
 };
 
-TestForAuthError::TestForAuthError(browser_sync::ProfileSyncService* service)
-    : UpdatedProgressMarkerChecker(service) {}
+class SyncTransportActiveChecker : public SingleClientStatusChangeChecker {
+ public:
+  explicit SyncTransportActiveChecker(browser_sync::ProfileSyncService* service)
+      : SingleClientStatusChangeChecker(service) {}
 
-bool TestForAuthError::IsExitConditionSatisfied() {
-  return (service()->GetSyncTokenStatus().last_get_token_error.state() !=
-          GoogleServiceAuthError::NONE) ||
-         UpdatedProgressMarkerChecker::IsExitConditionSatisfied();
-}
+  // StatusChangeChecker implementation.
+  bool IsExitConditionSatisfied() override {
+    return service()->GetTransportState() ==
+           syncer::SyncService::TransportState::ACTIVE;
+  }
 
-std::string TestForAuthError::GetDebugMessage() const {
-  return "Waiting for auth error";
-}
+  std::string GetDebugMessage() const override { return "Sync Active"; }
+};
 
 class SyncAuthTest : public SyncTest {
  public:
@@ -331,11 +340,12 @@ IN_PROC_BROWSER_TEST_F(SyncAuthTest, SyncPausedState) {
   ASSERT_TRUE(GetSyncService(0)->GetAuthError().IsPersistentError());
   ASSERT_TRUE(AttemptToTriggerAuthError());
 
+  // Pausing sync may issue a reconfiguration, so wait until it finishes.
+  SyncTransportActiveChecker(GetSyncService(0)).Wait();
+
   // While Sync itself is still considered active, the active data types should
   // now be empty.
   EXPECT_TRUE(GetSyncService(0)->IsSyncFeatureActive());
-  EXPECT_EQ(GetSyncService(0)->GetTransportState(),
-            syncer::SyncService::TransportState::ACTIVE);
 
   // Clear the "Sync paused" state again.
   GetClient(0)->ExitSyncPausedStateForPrimaryAccount();
@@ -344,9 +354,10 @@ IN_PROC_BROWSER_TEST_F(SyncAuthTest, SyncPausedState) {
   NoAuthErrorChecker(GetSyncService(0)).Wait();
   ASSERT_FALSE(GetSyncService(0)->GetAuthError().IsPersistentError());
 
+  // Resuming sync could issue a reconfiguration, so wait until it finishes.
+  SyncTransportActiveChecker(GetSyncService(0)).Wait();
+
   // Now the active data types should be back.
   EXPECT_TRUE(GetSyncService(0)->IsSyncFeatureActive());
-  EXPECT_EQ(GetSyncService(0)->GetTransportState(),
-            syncer::SyncService::TransportState::ACTIVE);
   EXPECT_EQ(GetSyncService(0)->GetActiveDataTypes(), active_types);
 }

components/browser_sync/profile_sync_service.cc

@@ -538,6 +538,9 @@ void ProfileSyncService::Shutdown() {
   NotifyShutdown();
   ShutdownImpl(syncer::BROWSER_SHUTDOWN);
 
+  DCHECK(!data_type_manager_);
+  data_type_controllers_.clear();
+
   // All observers must be gone now: All KeyedServices should have unregistered
   // their observers already before, in their own Shutdown(), and all others
   // should have done it now when they got the shutdown notification.
@@ -550,9 +553,6 @@ void ProfileSyncService::Shutdown() {
 
   if (sync_thread_)
     sync_thread_->Stop();
-
-  DCHECK(!data_type_manager_);
-  data_type_controllers_.clear();
 }
 
 void ProfileSyncService::ShutdownImpl(syncer::ShutdownReason reason) {

components/sync/driver/sync_auth_manager.cc

@@ -50,16 +50,6 @@ constexpr net::BackoffEntry::Policy kRequestAccessTokenBackoffPolicy = {
     false,
 };
 
-bool IsWebSignout(const GoogleServiceAuthError& auth_error) {
-  // The identity code sets an account's refresh token to be invalid (error
-  // CREDENTIALS_REJECTED_BY_CLIENT) if the user signs out of that account on
-  // the web.
-  return auth_error ==
-         GoogleServiceAuthError::FromInvalidGaiaCredentialsReason(
-             GoogleServiceAuthError::InvalidGaiaCredentialsReason::
-                 CREDENTIALS_REJECTED_BY_CLIENT);
-}
-
 }  // namespace
 
 SyncAuthManager::SyncAuthManager(

components/sync/driver/sync_auth_util.cc

@@ -6,6 +6,7 @@
 
 #include <vector>
 
+#include "google_apis/gaia/google_service_auth_error.h"
 #include "services/identity/public/cpp/identity_manager.h"
 
 namespace syncer {
@@ -44,4 +45,14 @@ SyncAccountInfo DetermineAccountToUse(
   return SyncAccountInfo();
 }
 
+bool IsWebSignout(const GoogleServiceAuthError& auth_error) {
+  // The identity code sets an account's refresh token to be invalid (error
+  // CREDENTIALS_REJECTED_BY_CLIENT) if the user signs out of that account on
+  // the web.
+  return auth_error ==
+         GoogleServiceAuthError::FromInvalidGaiaCredentialsReason(
+             GoogleServiceAuthError::InvalidGaiaCredentialsReason::
+                 CREDENTIALS_REJECTED_BY_CLIENT);
+}
+
 }  // namespace syncer

components/sync/driver/sync_auth_util.h

@@ -7,6 +7,8 @@
 
 #include "components/signin/core/browser/account_info.h"
 
+class GoogleServiceAuthError;
+
 namespace identity {
 class IdentityManager;
 }  // namespace identity
@@ -28,6 +30,10 @@ SyncAccountInfo DetermineAccountToUse(
     identity::IdentityManager* identity_manager,
     bool allow_secondary_accounts);
 
+// Returns whether |auth_error| indicates the user has locally signed out of
+// content area, rejecting credentials.
+bool IsWebSignout(const GoogleServiceAuthError& auth_error);
+
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_DRIVER_SYNC_AUTH_UTIL_H_

components/sync_sessions/BUILD.gn

@@ -62,6 +62,7 @@ static_library("sync_sessions") {
     "//components/prefs",
     "//components/variations",
     "//components/version_info",
+    "//google_apis",
     "//ui/base:base",
     "//ui/gfx",
     "//url",

components/sync_sessions/DEPS

@@ -8,6 +8,7 @@ include_rules = [
   "+components/sync",
   "+components/variations",
   "+components/version_info",
+  "+google_apis",
   "+ui/base/page_transition_types.h",
   "+ui/gfx",
 ]

components/sync_sessions/session_model_type_controller.cc

@@ -7,10 +7,19 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "components/prefs/pref_service.h"
+#include "components/sync/driver/sync_auth_util.h"
 #include "components/sync/driver/sync_service.h"
+#include "google_apis/gaia/google_service_auth_error.h"
 
 namespace sync_sessions {
+namespace {
+
+const base::Feature kStopSessionsIfSyncPaused{"StopSessionsIfSyncPaused",
+                                              base::FEATURE_ENABLED_BY_DEFAULT};
+
+}  // namespace
 
 SessionModelTypeController::SessionModelTypeController(
     syncer::SyncService* sync_service,
@@ -21,6 +30,9 @@ SessionModelTypeController::SessionModelTypeController(
       sync_service_(sync_service),
       pref_service_(pref_service),
       history_disabled_pref_name_(history_disabled_pref_name) {
+  // TODO(crbug.com/906995): remove this observing mechanism once all sync
+  // datatypes are stopped by ProfileSyncService, when sync is paused.
+  sync_service_->AddObserver(this);
   pref_registrar_.Init(pref_service);
   pref_registrar_.Add(
       history_disabled_pref_name_,
@@ -29,11 +41,21 @@ SessionModelTypeController::SessionModelTypeController(
           base::AsWeakPtr(this)));
 }
 
-SessionModelTypeController::~SessionModelTypeController() {}
+SessionModelTypeController::~SessionModelTypeController() {
+  sync_service_->RemoveObserver(this);
+}
 
 bool SessionModelTypeController::ReadyForStart() const {
   DCHECK(CalledOnValidThread());
-  return !pref_service_->GetBoolean(history_disabled_pref_name_);
+  return !pref_service_->GetBoolean(history_disabled_pref_name_) &&
+         !(syncer::IsWebSignout(sync_service_->GetAuthError()) &&
+           base::FeatureList::IsEnabled(kStopSessionsIfSyncPaused));
+}
+
+void SessionModelTypeController::OnStateChanged(syncer::SyncService* sync) {
+  DCHECK(CalledOnValidThread());
+  // Most of these calls will be no-ops but SyncService handles that just fine.
+  sync_service_->ReadyForStartChanged(type());
 }
 
 void SessionModelTypeController::OnSavingBrowserHistoryPrefChanged() {

components/sync_sessions/session_model_type_controller.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/sync/driver/model_type_controller.h"
+#include "components/sync/driver/sync_service_observer.h"
 
 class PrefService;
 
@@ -21,7 +22,8 @@ class SyncService;
 namespace sync_sessions {
 
 // Overrides LoadModels to check if history sync is allowed by policy.
-class SessionModelTypeController : public syncer::ModelTypeController {
+class SessionModelTypeController : public syncer::ModelTypeController,
+                                   public syncer::SyncServiceObserver {
  public:
   SessionModelTypeController(
       syncer::SyncService* sync_service,
@@ -33,6 +35,9 @@ class SessionModelTypeController : public syncer::ModelTypeController {
   // DataTypeController overrides.
   bool ReadyForStart() const override;
 
+  // syncer::SyncServiceObserver implementation.
+  void OnStateChanged(syncer::SyncService* sync) override;
+
  private:
   void OnSavingBrowserHistoryPrefChanged();