Check for Chrome OS sync feature when showing passphrase notification

Chrome OS sync and browser sync can both be encrypted with a shared
passphrase. However, a user can choose to leave OS sync enabled and
browser sync disabled, or vice versa. Show the sync passphrase error
notification when either feature is enabled.

Bug: 1033222, 1013466
Test: updated unit_tests
Change-Id: If4b8eddb76f7c9be1de3a7f094c692b2d03b7c6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1972899
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#725974}

chrome/browser/sync/sync_error_notifier_ash.cc

@@ -91,9 +91,7 @@ BubbleViewParameters GetBubbleViewParameters(
 
 SyncErrorNotifier::SyncErrorNotifier(syncer::SyncService* sync_service,
                                      Profile* profile)
-    : sync_service_(sync_service),
-      profile_(profile),
-      notification_displayed_(false) {
+    : sync_service_(sync_service), profile_(profile) {
   // Create a unique notification ID for this profile.
   notification_id_ =
       kProfileSyncNotificationId + profile_->GetProfileUserName();

chrome/browser/sync/sync_error_notifier_ash.h

@@ -31,12 +31,12 @@ class SyncErrorNotifier : public syncer::SyncServiceObserver,
   syncer::SyncService* sync_service_;
 
   // The Profile this service belongs to.
-  Profile* profile_;
+  Profile* const profile_;
 
   // Notification was added to NotificationUIManager. This flag is used to
   // prevent displaying passphrase notification to user if they already saw (and
   // potentially dismissed) previous one.
-  bool notification_displayed_;
+  bool notification_displayed_ = false;
 
   // Used to keep track of the message center notification.
   std::string notification_id_;

chrome/browser/sync/sync_error_notifier_ash_unittest.cc

@@ -7,12 +7,13 @@
 #include <memory>
 
 #include "base/bind.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/chromeos/login/users/mock_user_manager.h"
 #include "chrome/browser/notifications/notification_display_service_tester.h"
-#include "chrome/browser/sync/sync_ui_util.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
+#include "chromeos/constants/chromeos_features.h"
 #include "components/sync/driver/test_sync_service.h"
 #include "components/user_manager/scoped_user_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -45,8 +46,8 @@ std::unique_ptr<KeyedService> BuildFakeLoginUIService(
 
 class SyncErrorNotifierTest : public BrowserWithTestWindowTest {
  public:
-  SyncErrorNotifierTest() {}
-  ~SyncErrorNotifierTest() override {}
+  SyncErrorNotifierTest() = default;
+  ~SyncErrorNotifierTest() override = default;
 
   void SetUp() override {
     BrowserWithTestWindowTest::SetUp();
@@ -69,18 +70,7 @@ class SyncErrorNotifierTest : public BrowserWithTestWindowTest {
   }
 
  protected:
-  // Utility function to test that SyncErrorNotifier behaves correctly for the
-  // given error condition.
-  void VerifySyncErrorNotifierResult(GoogleServiceAuthError::State error_state,
-                                     bool is_signed_in,
-                                     bool is_error,
-                                     bool expected_notification) {
-    service_.SetFirstSetupComplete(is_signed_in);
-    service_.SetAuthError(GoogleServiceAuthError(error_state));
-    ASSERT_EQ(is_error, sync_ui_util::ShouldShowPassphraseError(&service_));
-
-    error_notifier_->OnStateChanged(&service_);
-
+  void ExpectNotificationShown(bool expected_notification) {
     base::Optional<message_center::Notification> notification =
         display_service_->GetNotification(kNotificationId);
     if (expected_notification) {
@@ -96,57 +86,56 @@ class SyncErrorNotifierTest : public BrowserWithTestWindowTest {
   syncer::TestSyncService service_;
   FakeLoginUI login_ui_;
   std::unique_ptr<NotificationDisplayServiceTester> display_service_;
+  user_manager::ScopedUserManager scoped_user_manager_{
+      std::make_unique<chromeos::MockUserManager>()};
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SyncErrorNotifierTest);
 };
 
-// Test that SyncErrorNotifier shows an notification if a passphrase is
-// required.
-TEST_F(SyncErrorNotifierTest, PassphraseNotification) {
-  user_manager::ScopedUserManager scoped_enabler(
-      std::make_unique<chromeos::MockUserManager>());
-  ASSERT_FALSE(display_service_->GetNotification(kNotificationId));
+TEST_F(SyncErrorNotifierTest, NoNotificationWhenNoPassphrase) {
+  service_.SetPassphraseRequiredForPreferredDataTypes(false);
+  service_.SetFirstSetupComplete(true);
+  error_notifier_->OnStateChanged(&service_);
+  ExpectNotificationShown(false);
+}
 
-  service_.SetPassphraseRequired(true);
+TEST_F(SyncErrorNotifierTest, NoNotificationWhenSyncDisabled) {
   service_.SetPassphraseRequiredForPreferredDataTypes(true);
-  {
-    SCOPED_TRACE("Expected a notification for passphrase error");
-    VerifySyncErrorNotifierResult(GoogleServiceAuthError::NONE,
-                                  true /* signed in */, true /* error */,
-                                  true /* expecting notification */);
-  }
+  service_.SetFirstSetupComplete(false);
+  error_notifier_->OnStateChanged(&service_);
+  ExpectNotificationShown(false);
+}
 
-  // Simulate discarded notification and check that notification is not shown.
-  display_service_->RemoveNotification(NotificationHandler::Type::TRANSIENT,
-                                       kNotificationId, true /* by_user */);
-  {
-    SCOPED_TRACE("Not expecting notification, one was already discarded");
-    VerifySyncErrorNotifierResult(GoogleServiceAuthError::NONE,
-                                  true /* signed in */, true /* error */,
-                                  false /* not expecting notification */);
-  }
+TEST_F(SyncErrorNotifierTest, NotificationShownWhenBrowserSyncEnabled) {
+  service_.SetPassphraseRequiredForPreferredDataTypes(true);
+  service_.SetFirstSetupComplete(true);
+  error_notifier_->OnStateChanged(&service_);
+  ExpectNotificationShown(true);
+}
 
-  // Check that no notification is shown if there is no error.
-  service_.SetPassphraseRequired(false);
-  service_.SetPassphraseRequiredForPreferredDataTypes(false);
-  {
-    SCOPED_TRACE("Not expecting notification since no error exists");
-    VerifySyncErrorNotifierResult(GoogleServiceAuthError::NONE,
-                                  true /* signed in */, false /* no error */,
-                                  false /* not expecting notification */);
-  }
+TEST_F(SyncErrorNotifierTest, NotificationShownWhenOsSyncEnabled) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  service_.SetPassphraseRequiredForPreferredDataTypes(true);
+  service_.GetUserSettings()->SetOsSyncFeatureEnabled(true);
+  service_.SetFirstSetupComplete(false);
+  error_notifier_->OnStateChanged(&service_);
+  ExpectNotificationShown(true);
+}
 
-  // Check that no notification is shown if sync setup is not completed.
-  service_.SetPassphraseRequired(true);
+TEST_F(SyncErrorNotifierTest, NotificationShownOnce) {
   service_.SetPassphraseRequiredForPreferredDataTypes(true);
-  {
-    SCOPED_TRACE("Not expecting notification since sync setup is incomplete");
-    VerifySyncErrorNotifierResult(
-        GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS,
-        false /* not signed in */, false /* no error */,
-        false /* not expecting notification */);
-  }
+  service_.GetUserSettings()->SetOsSyncFeatureEnabled(true);
+  service_.SetFirstSetupComplete(true);
+  error_notifier_->OnStateChanged(&service_);
+  ExpectNotificationShown(true);
+
+  // Close the notification and verify it isn't shown again.
+  display_service_->RemoveNotification(NotificationHandler::Type::TRANSIENT,
+                                       kNotificationId, true /* by_user */);
+  error_notifier_->OnStateChanged(&service_);
+  ExpectNotificationShown(false);
 }
 
 }  // namespace

chrome/browser/sync/sync_ui_util.cc

@@ -21,6 +21,10 @@
 #include "google_apis/gaia/gaia_urls.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/constants/chromeos_features.h"
+#endif
+
 namespace sync_ui_util {
 
 namespace {
@@ -177,6 +181,20 @@ void OpenTabForSyncKeyRetrievalWithURL(Browser* browser, const GURL& url) {
   ShowSingletonTabOverwritingNTP(browser, std::move(params));
 }
 
+// Returns true if the user has consented to browser sync-the-feature or
+// Chrome OS sync.
+bool HasUserOptedInToSync(const syncer::SyncUserSettings* settings) {
+  if (settings->IsFirstSetupComplete())
+    return true;
+#if defined(OS_CHROMEOS)
+  if (chromeos::features::IsSplitSettingsSyncEnabled() &&
+      settings->GetOsSyncFeatureEnabled()) {
+    return true;
+  }
+#endif  // defined(OS_CHROMEOS)
+  return false;
+}
+
 }  // namespace
 
 StatusLabels GetStatusLabels(syncer::SyncService* sync_service,
@@ -306,15 +324,15 @@ bool ShouldRequestSyncConfirmation(const syncer::SyncService* service) {
 }
 
 bool ShouldShowPassphraseError(const syncer::SyncService* service) {
-  return service->GetUserSettings()->IsFirstSetupComplete() &&
-         service->GetUserSettings()
-             ->IsPassphraseRequiredForPreferredDataTypes();
+  const syncer::SyncUserSettings* settings = service->GetUserSettings();
+  return HasUserOptedInToSync(settings) &&
+         settings->IsPassphraseRequiredForPreferredDataTypes();
 }
 
 bool ShouldShowSyncKeysMissingError(const syncer::SyncService* service) {
-  return service->GetUserSettings()->IsFirstSetupComplete() &&
-         service->GetUserSettings()
-             ->IsTrustedVaultKeyRequiredForPreferredDataTypes();
+  const syncer::SyncUserSettings* settings = service->GetUserSettings();
+  return HasUserOptedInToSync(settings) &&
+         settings->IsTrustedVaultKeyRequiredForPreferredDataTypes();
 }
 
 void OpenTabForSyncKeyRetrieval(Browser* browser) {

chrome/browser/sync/sync_ui_util_unittest.cc

@@ -17,6 +17,11 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_CHROMEOS)
+#include "base/test/scoped_feature_list.h"
+#include "chromeos/constants/chromeos_features.h"
+#endif
+
 namespace sync_ui_util {
 
 void PrintTo(const StatusLabels& labels, std::ostream* out) {
@@ -334,6 +339,39 @@ TEST(SyncUIUtilTest, IgnoreSyncErrorForNonSyncAccount) {
                                 IDS_SETTINGS_EMPTY_STRING, NO_ACTION));
 }
 
+TEST(SyncUIUtilTest, ShouldShowPassphraseError) {
+  syncer::TestSyncService service;
+  service.SetFirstSetupComplete(true);
+  service.SetPassphraseRequiredForPreferredDataTypes(true);
+  EXPECT_TRUE(ShouldShowPassphraseError(&service));
+}
+
+TEST(SyncUIUtilTest, ShouldShowPassphraseError_SyncDisabled) {
+  syncer::TestSyncService service;
+  service.SetFirstSetupComplete(false);
+  service.SetPassphraseRequiredForPreferredDataTypes(true);
+  EXPECT_FALSE(ShouldShowPassphraseError(&service));
+}
+
+TEST(SyncUIUtilTest, ShouldShowPassphraseError_NotUsingPassphrase) {
+  syncer::TestSyncService service;
+  service.SetFirstSetupComplete(true);
+  service.SetPassphraseRequiredForPreferredDataTypes(false);
+  EXPECT_FALSE(ShouldShowPassphraseError(&service));
+}
+
+#if defined(OS_CHROMEOS)
+TEST(SyncUIUtilTest, ShouldShowPassphraseError_OsSyncEnabled) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  syncer::TestSyncService service;
+  service.SetPassphraseRequiredForPreferredDataTypes(true);
+  service.SetFirstSetupComplete(false);
+  service.GetUserSettings()->SetOsSyncFeatureEnabled(true);
+  EXPECT_TRUE(ShouldShowPassphraseError(&service));
+}
+#endif  // defined(OS_CHROMEOS)
+
 }  // namespace
 
 }  // namespace sync_ui_util