[b4p] Set opt-in preference inside AccountStorageAuthHelper

Before this CL, every caller of the PasswordManagerClient's reauth API
used to set the opt-in pref upon success. Now this is taken care by the
AccountStorageAuthHelper.

Bug: 1070944
Change-Id: I354e10516ac759aa52ee07a8d8a130c7087bbc58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2149655Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#759597}

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc

@@ -448,20 +448,8 @@ void PasswordsPrivateDelegateImpl::SetAccountStorageOptIn(
         ProfileSyncServiceFactory::GetForProfile(profile_), false);
     return;
   }
-  account_storage_opt_in_reauthenticator_.Run(
-      web_contents,
-      base::BindOnce(
-          &PasswordsPrivateDelegateImpl::SetAccountStorageOptInCallback,
-          weak_ptr_factory_.GetWeakPtr()));
-}
-
-void PasswordsPrivateDelegateImpl::SetAccountStorageOptInCallback(
-    password_manager::PasswordManagerClient::ReauthSucceeded reauth_succeeded) {
-  if (reauth_succeeded) {
-    password_manager_util::SetAccountStorageOptIn(
-        profile_->GetPrefs(),
-        ProfileSyncServiceFactory::GetForProfile(profile_), true);
-  }
+  // The opt in pref is automatically set upon successful reauth.
+  account_storage_opt_in_reauthenticator_.Run(web_contents, base::DoNothing());
 }
 
 std::vector<api::passwords_private::CompromisedCredential>

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.h

@@ -147,12 +147,6 @@ class PasswordsPrivateDelegateImpl : public PasswordsPrivateDelegate,
 
   void OnAccountStorageOptInStateChanged();
 
-  // Callback for the reauth flow that is triggered upon the user opting in to
-  // account password storage. Will opt in the user if the reauth succeeded.
-  void SetAccountStorageOptInCallback(
-      password_manager::PasswordManagerClient::ReauthSucceeded
-          reauth_succeeded);
-
   // Triggers an OS-dependent UI to present OS account login challenge and
   // returns true if the user passed that challenge.
   bool OsReauthCall(password_manager::ReauthPurpose purpose);

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl_unittest.cc

@@ -152,9 +152,6 @@ class PasswordsPrivateDelegateImplTest : public testing::Test {
   // PasswordsPrivateEventRouter.
   void SetUpRouters();
 
-  void SetGoogleReauthResponse(PasswordsPrivateDelegateImpl* delegate,
-                               bool should_succeed);
-
   base::HistogramTester& histogram_tester() { return histogram_tester_; }
 
  protected:
@@ -199,22 +196,6 @@ void PasswordsPrivateDelegateImplTest::SetUpRouters() {
       &profile_, base::BindRepeating(&BuildPasswordsPrivateEventRouter));
 }
 
-void PasswordsPrivateDelegateImplTest::SetGoogleReauthResponse(
-    PasswordsPrivateDelegateImpl* delegate,
-    bool should_succeed) {
-  ON_CALL(mock_google_authenticator_, Run)
-      .WillByDefault(
-          [should_succeed](
-              content::WebContents*,
-              PasswordsPrivateDelegateImpl::GoogleReauthCallback callback) {
-            std::move(callback).Run(
-                password_manager::PasswordManagerClient::ReauthSucceeded(
-                    should_succeed));
-          });
-  delegate->set_account_storage_opt_in_reauthenticator(
-      mock_google_authenticator_.Get());
-}
-
 TEST_F(PasswordsPrivateDelegateImplTest, GetSavedPasswordsList) {
   PasswordsPrivateDelegateImpl delegate(&profile_);
 
@@ -322,28 +303,28 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestCopyPasswordCallbackResult) {
       1);
 }
 
-TEST_F(PasswordsPrivateDelegateImplTest, TestShouldOptInIfReauthSucceeds) {
+TEST_F(PasswordsPrivateDelegateImplTest, TestShouldReauthForOptIn) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(
       password_manager::features::kEnablePasswordsAccountStorage);
 
   PasswordsPrivateDelegateImpl delegate(&profile_);
 
-  SetGoogleReauthResponse(&delegate, true);
-
   auto* test_sync_service = static_cast<syncer::SyncService*>(
       ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
           &profile_, base::BindRepeating(&BuildTestSyncService)));
 
   password_manager_util::SetAccountStorageOptIn(profile_.GetPrefs(),
                                                 test_sync_service, false);
-  delegate.SetAccountStorageOptIn(true, nullptr);
 
-  EXPECT_TRUE(password_manager_util::IsOptedInForAccountStorage(
-      profile_.GetPrefs(), test_sync_service));
+  EXPECT_CALL(mock_google_authenticator_, Run);
+  delegate.set_account_storage_opt_in_reauthenticator(
+      mock_google_authenticator_.Get());
+
+  delegate.SetAccountStorageOptIn(true, nullptr);
 }
 
-TEST_F(PasswordsPrivateDelegateImplTest, TestShouldOptOut) {
+TEST_F(PasswordsPrivateDelegateImplTest, TestShouldNotReauthForOptOut) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(
       password_manager::features::kEnablePasswordsAccountStorage);
@@ -356,11 +337,12 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestShouldOptOut) {
 
   password_manager_util::SetAccountStorageOptIn(profile_.GetPrefs(),
                                                 test_sync_service, true);
-  delegate.SetAccountStorageOptIn(false, nullptr);
 
   EXPECT_CALL(mock_google_authenticator_, Run).Times(0);
-  EXPECT_FALSE(password_manager_util::IsOptedInForAccountStorage(
-      profile_.GetPrefs(), test_sync_service));
+  delegate.set_account_storage_opt_in_reauthenticator(
+      mock_google_authenticator_.Get());
+
+  delegate.SetAccountStorageOptIn(false, nullptr);
 }
 
 TEST_F(PasswordsPrivateDelegateImplTest, TestCopyPasswordCallbackResultFail) {

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -221,7 +221,7 @@ ChromePasswordManagerClient::ChromePasswordManagerClient(
       observer_(nullptr),
       credentials_filter_(this, base::BindRepeating(&GetSyncService, profile_)),
 #if !defined(OS_ANDROID)
-      account_storage_auth_helper_(profile_),
+      account_storage_auth_helper_(profile_, &password_feature_manager_),
 #endif
       helper_(this) {
   ContentPasswordManagerDriverFactory::CreateForWebContents(web_contents, this,

chrome/browser/ui/passwords/account_storage_auth_helper.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/signin_view_controller.h"
+#include "components/password_manager/core/browser/password_feature_manager.h"
 #include "components/signin/public/base/signin_metrics.h"
 #include "google_apis/gaia/core_account_id.h"
 
@@ -21,8 +22,12 @@ using ReauthSucceeded =
     password_manager::PasswordManagerClient::ReauthSucceeded;
 }
 
-AccountStorageAuthHelper::AccountStorageAuthHelper(Profile* profile)
-    : profile_(profile) {}
+AccountStorageAuthHelper::AccountStorageAuthHelper(
+    Profile* profile,
+    password_manager::PasswordFeatureManager* password_feature_manager)
+    : profile_(profile), password_feature_manager_(password_feature_manager) {}
+
+AccountStorageAuthHelper::~AccountStorageAuthHelper() = default;
 
 void AccountStorageAuthHelper::TriggerOptInReauth(
     const CoreAccountId& account_id,
@@ -40,13 +45,9 @@ void AccountStorageAuthHelper::TriggerOptInReauth(
   }
   signin_view_controller->ShowReauthPrompt(
       account_id,
-      base::BindOnce(
-          [](base::OnceCallback<void(ReauthSucceeded)> reauth_callback,
-             signin::ReauthResult result) {
-            std::move(reauth_callback)
-                .Run(ReauthSucceeded(result == signin::ReauthResult::kSuccess));
-          },
-          std::move(reauth_callback)));
+      base::BindOnce(&AccountStorageAuthHelper::OnOptInReauthCompleted,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     std::move(reauth_callback)));
 }
 
 void AccountStorageAuthHelper::TriggerSignIn() {
@@ -62,3 +63,12 @@ void AccountStorageAuthHelper::TriggerSignIn() {
   }
 #endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
 }
+
+void AccountStorageAuthHelper::OnOptInReauthCompleted(
+    base::OnceCallback<void(ReauthSucceeded)> reauth_callback,
+    signin::ReauthResult result) {
+  bool succeeded = result == signin::ReauthResult::kSuccess;
+  if (succeeded)
+    password_feature_manager_->SetAccountStorageOptIn(true);
+  std::move(reauth_callback).Run(ReauthSucceeded(succeeded));
+}

chrome/browser/ui/passwords/account_storage_auth_helper.h

@@ -6,8 +6,17 @@
 #define CHROME_BROWSER_UI_PASSWORDS_ACCOUNT_STORAGE_AUTH_HELPER_H_
 
 #include "base/callback.h"
+#include "base/memory/weak_ptr.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 
+namespace password_manager {
+class PasswordFeatureManager;
+}
+
+namespace signin {
+enum class ReauthResult;
+}
+
 struct CoreAccountId;
 class Profile;
 
@@ -15,15 +24,17 @@ class Profile;
 // account storage. Used only by desktop.
 class AccountStorageAuthHelper {
  public:
-  explicit AccountStorageAuthHelper(Profile* profile);
-  ~AccountStorageAuthHelper() = default;
+  AccountStorageAuthHelper(
+      Profile* profile,
+      password_manager::PasswordFeatureManager* password_feature_manager);
+  ~AccountStorageAuthHelper();
 
   AccountStorageAuthHelper(const AccountStorageAuthHelper&) = delete;
   AccountStorageAuthHelper& operator=(const AccountStorageAuthHelper&) = delete;
 
-  // Requests a reauth for the given |account_id|. |reauth_callback| is called
+  // Requests a reauth for the given |account_id|. In case of success, sets the
+  // opt in preference for account storage. |reauth_callback| is then called
   // passing whether the reauth succeeded or not.
-  // TODO(crbug.com/1070944): Set the opt-in pref internally and update comment.
   // TODO(crbug.com/1070944): Retrieve the primary account id internally and
   // update method comment.
   void TriggerOptInReauth(
@@ -36,7 +47,15 @@ class AccountStorageAuthHelper {
   void TriggerSignIn();
 
  private:
+  void OnOptInReauthCompleted(
+      base::OnceCallback<
+          void(password_manager::PasswordManagerClient::ReauthSucceeded)>
+          reauth_callback,
+      signin::ReauthResult result);
+
   Profile* const profile_;
+  password_manager::PasswordFeatureManager* const password_feature_manager_;
+  base::WeakPtrFactory<AccountStorageAuthHelper> weak_ptr_factory_{this};
 };
 
 #endif  // CHROME_BROWSER_UI_PASSWORDS_ACCOUNT_STORAGE_AUTH_HELPER_H_

chrome/browser/ui/passwords/manage_passwords_ui_controller.cc

@@ -729,7 +729,6 @@ void ManagePasswordsUIController::AuthenticateUserForAccountStoreOptInCallback(
     const base::string16& password,
     password_manager::PasswordManagerClient::ReauthSucceeded reauth_succeeded) {
   if (reauth_succeeded) {
-    GetPasswordFeatureManager()->SetAccountStorageOptIn(true);
     // Save the password only if it is the same origin and same form manager.
     // Otherwise it can be dangerous (e.g. saving the credentials against
     // another origin).

chrome/browser/ui/passwords/manage_passwords_ui_controller.h

@@ -255,9 +255,9 @@ class ManagePasswordsUIController
 
   // Gets invoked gaia reauth flow is finished. If the reauth was successful,
   // and the |form_manager| is still the same, |username| and |password| are
-  // saved against the current origin, and sets the user to be opted in for
-  // account store. If the reauth was unsuccessful, it changes the default
-  // destination to profle store and reopens the save bubble.
+  // saved against the current origin. If the reauth was unsuccessful, it
+  // changes the default destination to profle store and reopens the save
+  // bubble.
   void AuthenticateUserForAccountStoreOptInCallback(
       const GURL& origin,
       password_manager::PasswordFormManagerForUI* form_manager,

chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc

@@ -537,10 +537,7 @@ TEST_F(ManagePasswordsUIControllerTest,
   // The bubble gets hidden after the user clicks on save.
   controller()->OnBubbleHidden();
 
-  // Simulate a successful reauth which will cause the opt-in status to be
-  // recorded and the password to be saved.
-  EXPECT_CALL(*client().GetPasswordFeatureManager(),
-              SetAccountStorageOptIn(true));
+  // Simulate a successful reauth which will cause the password to be saved.
   EXPECT_CALL(*test_form_manager_ptr, Save());
   std::move(reauth_callback).Run(ReauthSucceeded(true));
 

components/password_manager/core/browser/password_autofill_manager.cc

@@ -747,7 +747,6 @@ void PasswordAutofillManager::OnUnlockReauthCompleted(
     autofill::PopupItemId unlock_item,
     PasswordManagerClient::ReauthSucceeded reauth_succeeded) {
   if (reauth_succeeded) {
-    password_client_->GetPasswordFeatureManager()->SetAccountStorageOptIn(true);
     if (unlock_item ==
         autofill::POPUP_ITEM_ID_PASSWORD_ACCOUNT_STORAGE_OPT_IN_AND_GENERATE) {
       password_client_->GeneratePassword();

components/password_manager/core/browser/password_autofill_manager.h

@@ -174,8 +174,8 @@ class PasswordAutofillManager : public autofill::AutofillPopupDelegate {
   void OnUnlockItemAccepted(autofill::PopupItemId unlock_item);
 
   // If reauth failed, resets the suggestions to show the |unlock_item| again.
-  // Otherwise, unlocks the account store and either triggers generation or
-  // filling based on the |unlock_item| that was clicked.
+  // Otherwise, triggers either generation or filling based on the |unlock_item|
+  // that was clicked.
   void OnUnlockReauthCompleted(
       autofill::PopupItemId unlock_item,
       PasswordManagerClient::ReauthSucceeded reauth_succeeded);

components/password_manager/core/browser/password_autofill_manager_unittest.cc

@@ -629,7 +629,8 @@ TEST_F(PasswordAutofillManagerTest, ClickOnReSiginTriggersSigninAndHides) {
       autofill::POPUP_ITEM_ID_PASSWORD_ACCOUNT_STORAGE_RE_SIGNIN, 1);
 }
 
-// Test that the popup is updated once "opt in and fill" is clicked.
+// Test that the popup is updated once "opt in and fill" is clicked and the
+// reauth fails.
 TEST_F(PasswordAutofillManagerTest, FailedOptInAndFillUpdatesPopup) {
   TestPasswordManagerClient client;
   NiceMock<MockAutofillClient> autofill_client;
@@ -640,8 +641,6 @@ TEST_F(PasswordAutofillManagerTest, FailedOptInAndFillUpdatesPopup) {
                                      .SetUnconsentedPrimaryAccount(kAliceEmail)
                                      .account_id;
   testing::Mock::VerifyAndClearExpectations(&autofill_client);
-  EXPECT_CALL(*client.GetPasswordFeatureManager(), SetAccountStorageOptIn)
-      .Times(0);
 
   // Accepting a suggestion should trigger a call to update the popup.
   // First the popup enters the waiting state.
@@ -680,7 +679,8 @@ TEST_F(PasswordAutofillManagerTest, FailedOptInAndFillUpdatesPopup) {
   EXPECT_FALSE(suggestions.back().is_loading);
 }
 
-// Test that the popup is updated once "opt in and generate" is clicked.
+// Test that the popup is updated once "opt in and generate" is clicked and the
+// reauth fails.
 TEST_F(PasswordAutofillManagerTest, FailedOptInAndGenerateUpdatesPopup) {
   TestPasswordManagerClient client;
   NiceMock<MockAutofillClient> autofill_client;
@@ -732,8 +732,9 @@ TEST_F(PasswordAutofillManagerTest, FailedOptInAndGenerateUpdatesPopup) {
   EXPECT_FALSE(suggestions.back().is_loading);
 }
 
-// Test that the popup is updated once "opt in and fill" is clicked.
-TEST_F(PasswordAutofillManagerTest, SuccessfullOptInAndFillTriggersOptIn) {
+// Test that the popup is updated once "opt in and fill" is clicked and the
+// reauth is successful.
+TEST_F(PasswordAutofillManagerTest, SuccessfullOptInAndFillHidesPopup) {
   TestPasswordManagerClient client;
   NiceMock<MockAutofillClient> autofill_client;
   InitializePasswordAutofillManager(&client, &autofill_client);
@@ -755,17 +756,16 @@ TEST_F(PasswordAutofillManagerTest, SuccessfullOptInAndFillTriggersOptIn) {
       .WillOnce([](const auto& id, auto reauth_callback) {
         std::move(reauth_callback).Run(ReauthSucceeded(true));
       });
-  EXPECT_CALL(*client.GetPasswordFeatureManager(),
-              SetAccountStorageOptIn(true));
 
   password_autofill_manager_->DidAcceptSuggestion(
       test_username_, autofill::POPUP_ITEM_ID_PASSWORD_ACCOUNT_STORAGE_OPT_IN,
       1);
 }
 
-// Test that the popup is updated once "opt in and generate" is clicked.
+// Test that the popup is hidden and password generation is triggered once
+// "opt in and generate" is clicked and the reauth is successful.
 TEST_F(PasswordAutofillManagerTest,
-       SuccessfullOptInAndGenerateTriggersOptInAndGeneration) {
+       SuccessfullOptInAndGenerateHidesPopupAndTriggersGeneration) {
   TestPasswordManagerClient client;
   NiceMock<MockAutofillClient> autofill_client;
   InitializePasswordAutofillManager(&client, &autofill_client);
@@ -787,8 +787,6 @@ TEST_F(PasswordAutofillManagerTest,
       .WillOnce([](const auto& id, auto reauth_callback) {
         std::move(reauth_callback).Run(ReauthSucceeded(true));
       });
-  EXPECT_CALL(*client.GetPasswordFeatureManager(),
-              SetAccountStorageOptIn(true));
   EXPECT_CALL(
       autofill_client,
       HideAutofillPopup(autofill::PopupHidingReason::kAcceptSuggestion));