Change generated PW leak pref to query for unconsented primary accounts

Previously the generated password leak preference queried for sync
consented primary accounts to confirm whether the PW leak feature was
active.

This is incorrect and the passwords leak detection feature does not
require a sync consent (only a functioning sync service and a primary
account).

CL also simplifies the front end logic for deciding whether the
additional text is added to the PW leak detection toggle sublabel,
removing the requirement for the security page to observe the sync
state directly.

Bug: 1156264
Change-Id: I091459fc47f4d07d3923bbfdf9fb7e4a49246512
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611257
Commit-Queue: Theodore Olsauskas-Warren <sauski@google.com>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841525}

chrome/browser/password_manager/generated_password_leak_detection_pref.cc

@@ -34,7 +34,11 @@ bool IsUserSignedInAndSyncing(Profile* profile) {
       status_labels.message_type == sync_ui_util::SYNC_ERROR ||
       status_labels.message_type == sync_ui_util::PASSWORDS_ONLY_SYNC_ERROR;
 
-  return identity_manager->HasPrimaryAccount() && !sync_error;
+  // Password leak detection only requires a signed in account and a functioning
+  // sync service, it does not require sync consent.
+  return identity_manager->HasPrimaryAccount(
+             signin::ConsentLevel::kNotRequired) &&
+         !sync_error;
 }
 
 // Returns whether the effective value of the Safe Browsing preferences for

chrome/browser/password_manager/generated_password_leak_detection_pref_unittest.cc

@@ -6,6 +6,8 @@
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/extensions/api/settings_private/generated_pref_test_base.h"
 #include "chrome/browser/extensions/api/settings_private/generated_prefs_factory.h"
+#include "chrome/browser/signin/chrome_signin_client_factory.h"
+#include "chrome/browser/signin/chrome_signin_client_test_util.h"
 #include "chrome/browser/signin/identity_test_environment_profile_adaptor.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/test/base/testing_profile.h"
@@ -15,15 +17,30 @@
 #include "components/sync/driver/test_sync_service.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/browser_task_environment.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
+constexpr char kTestProfileName[] = "test@test.com";
+
 std::unique_ptr<KeyedService> BuildTestSyncService(
     content::BrowserContext* context) {
   return std::make_unique<syncer::TestSyncService>();
 }
 
+std::unique_ptr<TestingProfile> BuildTestProfile(
+    network::TestURLLoaderFactory& url_loader_factory) {
+  TestingProfile::Builder profile_builder;
+  profile_builder.SetProfileName(kTestProfileName);
+  profile_builder.AddTestingFactory(
+      ChromeSigninClientFactory::GetInstance(),
+      base::BindRepeating(&BuildChromeSigninClientWithURLLoader,
+                          &url_loader_factory));
+  return IdentityTestEnvironmentProfileAdaptor::
+      CreateProfileForIdentityTestEnvironment(profile_builder);
+}
+
 }  // namespace
 
 namespace settings_api = extensions::api::settings_private;
@@ -31,8 +48,12 @@ namespace settings_private = extensions::settings_private;
 
 class GeneratedPasswordLeakDetectionPrefTest : public testing::Test {
  public:
+  GeneratedPasswordLeakDetectionPrefTest() {
+    identity_test_env()->SetTestURLLoaderFactory(&test_url_loader_factory_);
+  }
+
   signin::IdentityTestEnvironment* identity_test_env() {
-    return identity_test_env_adaptor_->identity_test_env();
+    return identity_test_env_adaptor_.identity_test_env();
   }
 
   sync_preferences::TestingPrefServiceSyncable* prefs() {
@@ -46,18 +67,16 @@ class GeneratedPasswordLeakDetectionPrefTest : public testing::Test {
   content::BrowserTaskEnvironment task_environment_;
 
  private:
+  network::TestURLLoaderFactory test_url_loader_factory_;
   std::unique_ptr<TestingProfile> profile_ =
-      IdentityTestEnvironmentProfileAdaptor::
-          CreateProfileForIdentityTestEnvironment({});
+      BuildTestProfile(test_url_loader_factory_);
   syncer::TestSyncService* sync_service_ =
       static_cast<syncer::TestSyncService*>(
           ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
               profile(),
               base::BindRepeating(&BuildTestSyncService)));
-  std::unique_ptr<IdentityTestEnvironmentProfileAdaptor>
-      identity_test_env_adaptor_ =
-          std::make_unique<IdentityTestEnvironmentProfileAdaptor>(
-              profile_.get());
+  IdentityTestEnvironmentProfileAdaptor identity_test_env_adaptor_{
+      profile_.get()};
 };
 
 TEST_F(GeneratedPasswordLeakDetectionPrefTest, NotifyPrefUpdates) {
@@ -68,7 +87,10 @@ TEST_F(GeneratedPasswordLeakDetectionPrefTest, NotifyPrefUpdates) {
 
   // Check that the observer fires for identity updates.
   identity_test_env()->EnableRemovalOfExtendedAccountInfo();
-  identity_test_env()->MakePrimaryAccountAvailable("test@test.com");
+
+  // Create a sync consented account so revoking the refresh token also triggers
+  // the preference updated observer.
+  identity_test_env()->MakePrimaryAccountAvailable(kTestProfileName);
   EXPECT_EQ(test_observer.GetUpdatedPrefName(),
             kGeneratedPasswordLeakDetectionPref);
 
@@ -120,7 +142,7 @@ TEST_F(GeneratedPasswordLeakDetectionPrefTest, UpdatePreference) {
   prefs()->SetDefaultPrefValue(
       password_manager::prefs::kPasswordLeakDetectionEnabled,
       base::Value(false));
-  identity_test_env()->MakePrimaryAccountAvailable("test@test.com");
+  identity_test_env()->MakeUnconsentedPrimaryAccountAvailable(kTestProfileName);
 
   // Check setting the generated pref updates the underlying preference.
   EXPECT_EQ(pref.SetPref(std::make_unique<base::Value>(true).get()),
@@ -161,7 +183,7 @@ TEST_F(GeneratedPasswordLeakDetectionPrefTest, ProfileState) {
 
   // Check when signed in and Safe Browsing set to standard, both user control
   // and the pref are enabled.
-  identity_test_env()->MakePrimaryAccountAvailable("test@test.com");
+  identity_test_env()->MakeUnconsentedPrimaryAccountAvailable(kTestProfileName);
   prefs()->SetUserPref(prefs::kSafeBrowsingEnabled,
                        std::make_unique<base::Value>(true));
   prefs()->SetUserPref(prefs::kSafeBrowsingEnhanced,
@@ -215,7 +237,7 @@ TEST_F(GeneratedPasswordLeakDetectionPrefTest, ManagementState) {
 
   // Check that the preference cannot be changed when the backing preference is
   // managed, but the preference could otherwise be changed.
-  identity_test_env()->MakePrimaryAccountAvailable("test@test.com");
+  identity_test_env()->MakeUnconsentedPrimaryAccountAvailable(kTestProfileName);
   prefs()->SetUserPref(prefs::kSafeBrowsingEnabled,
                        std::make_unique<base::Value>(true));
   prefs()->SetUserPref(prefs::kSafeBrowsingEnhanced,

chrome/browser/resources/settings/privacy_page/BUILD.gn

@@ -146,7 +146,6 @@ js_library("privacy_page") {
     "..:route",
     "..:router.m",
     "../controls:settings_toggle_button.m",
-    "../people_page:sync_browser_proxy.m",
     "../prefs:prefs_behavior.m",
     "../site_settings:constants",
     "../site_settings:site_data_details_subpage",
@@ -235,7 +234,6 @@ js_library("security_page") {
     "..:metrics_browser_proxy",
     "..:route",
     "..:router.m",
-    "../people_page:sync_browser_proxy.m",
     "../prefs:prefs_behavior.m",
     "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
     "//ui/webui/resources/js:assert.m",

chrome/browser/resources/settings/privacy_page/privacy_page.html

@@ -95,8 +95,7 @@
             associated-control="[[$$('#securityLinkRow')]]"
             learn-more-url="$i18n{safeBrowsingHelpCenterURL}">
           <settings-security-page prefs="{{prefs}}"
-              focus-config="[[focusConfig_]]"
-              sync-status="[[syncStatus]]">
+              focus-config="[[focusConfig_]]">
           </settings-security-page>
         </settings-subpage>
       </template>

chrome/browser/resources/settings/privacy_page/privacy_page.js

@@ -28,7 +28,6 @@ import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bun
 import {HatsBrowserProxyImpl} from '../hats_browser_proxy.js';
 import {loadTimeData} from '../i18n_setup.js';
 import {MetricsBrowserProxy, MetricsBrowserProxyImpl, PrivacyElementInteractions} from '../metrics_browser_proxy.js';
-import {SyncBrowserProxyImpl, SyncStatus} from '../people_page/sync_browser_proxy.m.js';
 import {PrefsBehavior} from '../prefs/prefs_behavior.m.js';
 import {routes} from '../route.js';
 import {RouteObserverBehavior, Router} from '../router.m.js';
@@ -66,12 +65,6 @@ Polymer({
       notify: true,
     },
 
-    /**
-     * The current sync status, supplied by SyncBrowserProxy.
-     * @type {?SyncStatus}
-     */
-    syncStatus: Object,
-
     /** @private */
     isGuest_: {
       type: Boolean,
@@ -236,11 +229,6 @@ Polymer({
         'onBlockAutoplayStatusChanged',
         this.onBlockAutoplayStatusChanged_.bind(this));
 
-    SyncBrowserProxyImpl.getInstance().getSyncStatus().then(
-        this.handleSyncStatus_.bind(this));
-    this.addWebUIListener(
-        'sync-status-changed', this.handleSyncStatus_.bind(this));
-
     SiteSettingsPrefsBrowserProxyImpl.getInstance()
         .getCookieSettingDescription()
         .then(description => this.cookieSettingDescription_ = description);
@@ -249,15 +237,6 @@ Polymer({
         description => this.cookieSettingDescription_ = description);
   },
 
-  /**
-   * Handler for when the sync state is pushed from the browser.
-   * @param {?SyncStatus} syncStatus
-   * @private
-   */
-  handleSyncStatus_(syncStatus) {
-    this.syncStatus = syncStatus;
-  },
-
   /** @protected */
   currentRouteChanged() {
     this.showClearBrowsingDataDialog_ =

chrome/browser/resources/settings/privacy_page/security_page.html

@@ -132,7 +132,7 @@
                 pref="{{prefs.generated.password_leak_detection}}"
                 sub-label="[[getPasswordsLeakToggleSubLabel_(
                               prefs.profile.password_manager_leak_detection.*,
-                              syncStatus.*)]]">
+                              prefs.generated.password_leak_detection.*)]]">
             </settings-toggle-button>
           </div>
         </settings-collapse-radio-button>

chrome/browser/resources/settings/privacy_page/security_page.js

@@ -21,7 +21,6 @@ import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bun
 
 import {loadTimeData} from '../i18n_setup.js';
 import {MetricsBrowserProxy, MetricsBrowserProxyImpl, PrivacyElementInteractions, SafeBrowsingInteractions} from '../metrics_browser_proxy.js';
-import {SyncStatus} from '../people_page/sync_browser_proxy.m.js';
 import {PrefsBehavior} from '../prefs/prefs_behavior.m.js';
 import {routes} from '../route.js';
 import {Route, RouteObserverBehavior, Router} from '../router.m.js';
@@ -52,9 +51,6 @@ Polymer({
   ],
 
   properties: {
-    /** @type {SyncStatus} */
-    syncStatus: Object,
-
     /**
      * Preferences state.
      */
@@ -221,9 +217,11 @@ Polymer({
    */
   getPasswordsLeakToggleSubLabel_() {
     let subLabel = this.i18n('passwordsLeakDetectionGeneralDescription');
+    // If the backing password leak detection preference is enabled, but the
+    // generated preference is disabled, then additional text explaining that
+    // the feature will be enabled if the user signs in is added.
     if (this.getPref('profile.password_manager_leak_detection').value &&
-        (!this.syncStatus.signedIn ||
-         !!this.syncStatus.signedIn && !!this.syncStatus.hasError)) {
+        !this.getPref('generated.password_leak_detection').value) {
       subLabel +=
           ' ' +  // Whitespace is a valid sentence separator w.r.t. i18n.
           this.i18n('passwordsLeakDetectionSignedOutEnabledDescription');

chrome/test/data/webui/settings/privacy_page_test.js

@@ -7,7 +7,7 @@ import {webUIListenerCallback} from 'chrome://resources/js/cr.m.js';
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
 import {flush} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 import {ClearBrowsingDataBrowserProxyImpl, ContentSettingsTypes, CookieControlsMode, SafeBrowsingSetting, SiteSettingsPrefsBrowserProxyImpl} from 'chrome://settings/lazy_load.js';
-import {HatsBrowserProxyImpl, MetricsBrowserProxyImpl, PrivacyElementInteractions, PrivacyPageBrowserProxyImpl, Route, Router, routes, SecureDnsMode, SyncBrowserProxyImpl} from 'chrome://settings/settings.js';
+import {HatsBrowserProxyImpl, MetricsBrowserProxyImpl, PrivacyElementInteractions, PrivacyPageBrowserProxyImpl, Route, Router, routes, SecureDnsMode} from 'chrome://settings/settings.js';
 
 import {assertEquals, assertFalse, assertTrue} from '../chai_assert.js';
 import {flushTasks, isChildVisible, isVisible} from '../test_util.m.js';
@@ -17,7 +17,6 @@ import {TestHatsBrowserProxy} from './test_hats_browser_proxy.js';
 import {TestMetricsBrowserProxy} from './test_metrics_browser_proxy.js';
 import {TestPrivacyPageBrowserProxy} from './test_privacy_page_browser_proxy.js';
 import {TestSiteSettingsPrefsBrowserProxy} from './test_site_settings_prefs_browser_proxy.js';
-import {TestSyncBrowserProxy} from './test_sync_browser_proxy.m.js';
 
 // clang-format on
 
@@ -82,14 +81,12 @@ suite('PrivacyPage', function() {
     });
   });
 
-  setup(async function() {
+  setup(function() {
     testClearBrowsingDataBrowserProxy = new TestClearBrowsingDataBrowserProxy();
     ClearBrowsingDataBrowserProxyImpl.instance_ =
         testClearBrowsingDataBrowserProxy;
     const testBrowserProxy = new TestPrivacyPageBrowserProxy();
     PrivacyPageBrowserProxyImpl.instance_ = testBrowserProxy;
-    const testSyncBrowserProxy = new TestSyncBrowserProxy();
-    SyncBrowserProxyImpl.instance_ = testSyncBrowserProxy;
     siteSettingsBrowserProxy = new TestSiteSettingsPrefsBrowserProxy();
     SiteSettingsPrefsBrowserProxyImpl.instance_ = siteSettingsBrowserProxy;
     siteSettingsBrowserProxy.setCookieSettingDescription(testLabels[0]);
@@ -112,7 +109,7 @@ suite('PrivacyPage', function() {
           {mode: {value: SecureDnsMode.AUTOMATIC}, templates: {value: ''}},
     };
     document.body.appendChild(page);
-    return testSyncBrowserProxy.whenCalled('getSyncStatus');
+    return flushTasks();
   });
 
   teardown(function() {

chrome/test/data/webui/settings/security_page_test.js

@@ -7,13 +7,12 @@ import {isLacros, isMac, isWindows} from 'chrome://resources/js/cr.m.js';
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
 import {flush} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 import {SafeBrowsingSetting} from 'chrome://settings/lazy_load.js';
-import {MetricsBrowserProxyImpl, PrivacyElementInteractions, PrivacyPageBrowserProxyImpl, Router, routes, SafeBrowsingInteractions, SecureDnsMode, SyncBrowserProxyImpl} from 'chrome://settings/settings.js';
+import {MetricsBrowserProxyImpl, PrivacyElementInteractions, PrivacyPageBrowserProxyImpl, Router, routes, SafeBrowsingInteractions, SecureDnsMode} from 'chrome://settings/settings.js';
 import {assertEquals, assertFalse, assertTrue} from '../chai_assert.js';
 import {flushTasks, isChildVisible} from '../test_util.m.js';
 
 import {TestMetricsBrowserProxy} from './test_metrics_browser_proxy.js';
 import {TestPrivacyPageBrowserProxy} from './test_privacy_page_browser_proxy.js';
-import {TestSyncBrowserProxy} from './test_sync_browser_proxy.m.js';
 
 // clang-format on
 
@@ -21,9 +20,6 @@ suite('CrSettingsSecurityPageTest', function() {
   /** @type {!TestMetricsBrowserProxy} */
   let testMetricsBrowserProxy;
 
-  /** @type {!TestSyncBrowserProxy} */
-  let syncBrowserProxy;
-
   /** @type {!TestPrivacyPageBrowserProxy} */
   let testPrivacyBrowserProxy;
 
@@ -42,8 +38,6 @@ suite('CrSettingsSecurityPageTest', function() {
     MetricsBrowserProxyImpl.instance_ = testMetricsBrowserProxy;
     testPrivacyBrowserProxy = new TestPrivacyPageBrowserProxy();
     PrivacyPageBrowserProxyImpl.instance_ = testPrivacyBrowserProxy;
-    syncBrowserProxy = new TestSyncBrowserProxy();
-    SyncBrowserProxyImpl.instance_ = syncBrowserProxy;
     document.body.innerHTML = '';
     page = /** @type {!SettingsSecurityPageElement} */ (
         document.createElement('settings-security-page'));
@@ -57,12 +51,11 @@ suite('CrSettingsSecurityPageTest', function() {
           type: chrome.settingsPrivate.PrefType.NUMBER,
           value: SafeBrowsingSetting.STANDARD,
         },
-        password_leak_detection: {value: true, userControlDisabled: false},
+        password_leak_detection: {value: false},
       },
       dns_over_https:
           {mode: {value: SecureDnsMode.AUTOMATIC}, templates: {value: ''}},
     };
-    page.set('syncStatus', {signedIn: false, hasError: false});
     document.body.appendChild(page);
     page.$$('#safeBrowsingEnhanced').updateCollapsed();
     page.$$('#safeBrowsingStandard').updateCollapsed();
@@ -119,13 +112,13 @@ suite('CrSettingsSecurityPageTest', function() {
     flush();
     assertEquals(activeWhenSignedInSubLabel, toggle.subLabel);
 
-    page.set('syncStatus', {signedIn: true});
+    page.set('prefs.generated.password_leak_detection.value', true);
     flush();
     assertEquals(defaultSubLabel, toggle.subLabel);
 
-    page.set('syncStatus', {signedIn: true, hasError: true});
+    page.set('prefs.profile.password_manager_leak_detection.value', false);
     flush();
-    assertEquals(activeWhenSignedInSubLabel, toggle.subLabel);
+    assertEquals(defaultSubLabel, toggle.subLabel);
   });
 
   test('LogSafeBrowsingExtendedToggle', async function() {
@@ -601,7 +594,6 @@ suite('CrSettingsSecurityPageTest_FlagsDisabled', function() {
       dns_over_https:
           {mode: {value: SecureDnsMode.AUTOMATIC}, templates: {value: ''}},
     };
-    page.set('syncStatus', {signedIn: true, hasError: false});
     document.body.appendChild(page);
     flush();
   });