[Passwords] Trigger reauth before moving password

Whenever a user submits a password that they can move to their account,
they will face the move dialog. If they haven't opted-in, they need to
confirm that they want to opt in by reauthenticating.
Before this CL, this reauthentication was missing and the move would
fail silently.

Bug: 1097699
Change-Id: I9ea54f2bdb21af027d602735a8a81a99d0f9f47c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2257231Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Friedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781756}

chrome/browser/ui/passwords/bubble_controllers/move_to_account_store_bubble_controller.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/ui/passwords/passwords_model_delegate.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/password_manager/core/browser/password_feature_manager.h"
 #include "components/password_manager/core/common/password_manager_ui.h"
 #include "components/signin/public/identity_manager/consent_level.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
@@ -38,7 +39,12 @@ base::string16 MoveToAccountStoreBubbleController::GetTitle() const {
 }
 
 void MoveToAccountStoreBubbleController::AcceptMove() {
-  return delegate_->MovePasswordToAccountStore();
+  if (delegate_->GetPasswordFeatureManager()->IsOptedInForAccountStorage()) {
+    // User has already opted in to the account store. Move without reauth.
+    return delegate_->MovePasswordToAccountStore();
+  }
+  // Otherwise, we should invoke the reauth flow before saving.
+  return delegate_->AuthenticateUserForAccountStoreOptInAndMovePassword();
 }
 
 gfx::Image MoveToAccountStoreBubbleController::GetProfileIcon() {

chrome/browser/ui/passwords/bubble_controllers/move_to_account_store_bubble_controller_unittest.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/ui/passwords/passwords_model_delegate_mock.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/password_manager/core/browser/mock_password_feature_manager.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/signin/public/identity_manager/identity_test_utils.h"
 #include "content/public/browser/web_contents.h"
@@ -36,6 +37,8 @@ class MoveToAccountStoreBubbleControllerTest : public ::testing::Test {
     EXPECT_CALL(*delegate(), OnBubbleShown());
     ON_CALL(*delegate(), GetWebContents())
         .WillByDefault(Return(web_contents_.get()));
+    ON_CALL(*delegate(), GetPasswordFeatureManager())
+        .WillByDefault(Return(&password_feature_manager_));
     controller_ = std::make_unique<MoveToAccountStoreBubbleController>(
         mock_delegate_->AsWeakPtr());
     EXPECT_TRUE(testing::Mock::VerifyAndClearExpectations(delegate()));
@@ -45,11 +48,16 @@ class MoveToAccountStoreBubbleControllerTest : public ::testing::Test {
   PasswordsModelDelegateMock* delegate() { return mock_delegate_.get(); }
   TestingProfile* profile() { return &profile_; }
   MoveToAccountStoreBubbleController* controller() { return controller_.get(); }
+  password_manager::MockPasswordFeatureManager* password_feature_manager() {
+    return &password_feature_manager_;
+  }
 
  private:
   content::BrowserTaskEnvironment task_environment_;
   content::RenderViewHostTestEnabler test_render_host_factories_;
   TestingProfile profile_;
+  testing::NiceMock<password_manager::MockPasswordFeatureManager>
+      password_feature_manager_;
   std::unique_ptr<content::WebContents> web_contents_;
   std::unique_ptr<PasswordsModelDelegateMock> mock_delegate_;
   std::unique_ptr<MoveToAccountStoreBubbleController> controller_;
@@ -60,11 +68,20 @@ TEST_F(MoveToAccountStoreBubbleControllerTest, CloseExplicitly) {
   controller()->OnBubbleClosing();
 }
 
-TEST_F(MoveToAccountStoreBubbleControllerTest, AcceptMove) {
+TEST_F(MoveToAccountStoreBubbleControllerTest, AcceptMoveIfOptedIn) {
+  ON_CALL(*password_feature_manager(), IsOptedInForAccountStorage)
+      .WillByDefault(Return(true));
   EXPECT_CALL(*delegate(), MovePasswordToAccountStore);
   controller()->AcceptMove();
 }
 
+TEST_F(MoveToAccountStoreBubbleControllerTest, AuthenticateMoveIfOptedOut) {
+  ON_CALL(*password_feature_manager(), IsOptedInForAccountStorage)
+      .WillByDefault(Return(false));
+  EXPECT_CALL(*delegate(), AuthenticateUserForAccountStoreOptInAndMovePassword);
+  controller()->AcceptMove();
+}
+
 TEST_F(MoveToAccountStoreBubbleControllerTest, ProvidesTitle) {
   PasswordBubbleControllerBase* controller_ptr = controller();
   EXPECT_EQ(controller_ptr->GetTitle(),

chrome/browser/ui/passwords/manage_passwords_ui_controller.cc

@@ -634,11 +634,24 @@ void ManagePasswordsUIController::
   client->TriggerReauthForPrimaryAccount(
       signin_metrics::ReauthAccessPoint::kPasswordSaveBubble,
       base::BindOnce(&ManagePasswordsUIController::
-                         AuthenticateUserForAccountStoreOptInCallback,
+                         FinishSavingPasswordAfterAccountStoreOptInAuth,
                      weak_ptr_factory_.GetWeakPtr(), passwords_data_.origin(),
                      passwords_data_.form_manager(), username, password));
 }
 
+void ManagePasswordsUIController::
+    AuthenticateUserForAccountStoreOptInAndMovePassword() {
+  DCHECK_EQ(GetState(),
+            password_manager::ui::CAN_MOVE_PASSWORD_TO_ACCOUNT_STATE)
+      << GetState();
+  passwords_data_.client()->TriggerReauthForPrimaryAccount(
+      signin_metrics::ReauthAccessPoint::kPasswordMoveBubble,
+      base::BindOnce(&ManagePasswordsUIController::
+                         FinishMovingPasswordAfterAccountStoreOptInAuth,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     passwords_data_.form_manager()));
+}
+
 bool ManagePasswordsUIController::ArePasswordsRevealedWhenBubbleIsOpened()
     const {
   return are_passwords_revealed_when_next_bubble_is_opened_;
@@ -786,12 +799,14 @@ bool ManagePasswordsUIController::ShowAuthenticationDialog() {
 #endif
 }
 
-void ManagePasswordsUIController::AuthenticateUserForAccountStoreOptInCallback(
-    const url::Origin& origin,
-    password_manager::PasswordFormManagerForUI* form_manager,
-    const base::string16& username,
-    const base::string16& password,
-    password_manager::PasswordManagerClient::ReauthSucceeded reauth_succeeded) {
+void ManagePasswordsUIController::
+    FinishSavingPasswordAfterAccountStoreOptInAuth(
+        const url::Origin& origin,
+        password_manager::PasswordFormManagerForUI* form_manager,
+        const base::string16& username,
+        const base::string16& password,
+        password_manager::PasswordManagerClient::ReauthSucceeded
+            reauth_succeeded) {
   if (reauth_succeeded) {
     passwords_data_.set_auth_for_account_storage_opt_in_failed(false);
     // Save the password only if it is the same origin and same form manager.
@@ -840,4 +855,18 @@ void ManagePasswordsUIController::OnTriggerPostSaveCompromisedBubble(
   UpdateBubbleAndIconVisibility();
 }
 
+void ManagePasswordsUIController::
+    FinishMovingPasswordAfterAccountStoreOptInAuth(
+        password_manager::PasswordFormManagerForUI* form_manager,
+        password_manager::PasswordManagerClient::ReauthSucceeded
+            reauth_succeeded) {
+  if (!reauth_succeeded || passwords_data_.form_manager() != form_manager) {
+    return;
+  }
+  MovePasswordToAccountStore();
+  ClearPopUpFlagForBubble();
+  passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
+  UpdateBubbleAndIconVisibility();
+}
+
 WEB_CONTENTS_USER_DATA_KEY_IMPL(ManagePasswordsUIController)

chrome/browser/ui/passwords/manage_passwords_ui_controller.h

@@ -160,6 +160,7 @@ class ManagePasswordsUIController
   void AuthenticateUserForAccountStoreOptInAndSavePassword(
       const base::string16& username,
       const base::string16& password) override;
+  void AuthenticateUserForAccountStoreOptInAndMovePassword() override;
   bool ArePasswordsRevealedWhenBubbleIsOpened() const override;
 
 #if defined(UNIT_TEST)
@@ -266,7 +267,7 @@ class ManagePasswordsUIController
   // 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(
+  void FinishSavingPasswordAfterAccountStoreOptInAuth(
       const url::Origin& origin,
       password_manager::PasswordFormManagerForUI* form_manager,
       const base::string16& username,
@@ -278,6 +279,13 @@ class ManagePasswordsUIController
       password_manager::PostSaveCompromisedHelper::BubbleType type,
       size_t count_compromised_passwords_);
 
+  // Triggered from a reauthentication flow. If |form_manager| is still valid
+  // and the reauth was successful, the password is moved to the account store.
+  void FinishMovingPasswordAfterAccountStoreOptInAuth(
+      password_manager::PasswordFormManagerForUI* form_manager,
+      password_manager::PasswordManagerClient::ReauthSucceeded
+          reauth_succeeded);
+
   // Timeout in seconds for the manual fallback for saving.
   static int save_fallback_timeout_in_seconds_;
 

chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc

@@ -55,6 +55,7 @@ using password_manager::MockPasswordStore;
 using ReauthSucceeded =
     password_manager::PasswordManagerClient::ReauthSucceeded;
 using ::testing::_;
+using ::testing::AtLeast;
 using ::testing::AtMost;
 using ::testing::Contains;
 using ::testing::DoAll;
@@ -1590,3 +1591,56 @@ TEST_F(ManagePasswordsUIControllerTest, OpenUnsafeStateBubble) {
   ExpectIconAndControllerStateIs(
       password_manager::ui::PASSWORD_UPDATED_UNSAFE_STATE);
 }
+
+TEST_F(ManagePasswordsUIControllerTest, ReauthenticateBeforeMove) {
+  std::vector<const PasswordForm*> matches = {&test_local_form()};
+  auto test_form_manager = CreateFormManagerWithBestMatches(&matches);
+  MockPasswordFormManagerForUI* form_manager = test_form_manager.get();
+
+  // A submitted form triggers the move dialog.
+  EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility())
+      .Times(AtLeast(1));
+  controller()->OnShowMoveToAccountBubble(std::move(test_form_manager));
+  EXPECT_TRUE(controller()->opened_automatic_bubble());
+  ExpectIconAndControllerStateIs(
+      password_manager::ui::CAN_MOVE_PASSWORD_TO_ACCOUNT_STATE);
+
+  // A user confirms the move which closes the dialog but opens a reauth.
+  base::OnceCallback<void(ReauthSucceeded)> reauth_callback;
+  EXPECT_CALL(client(),
+              TriggerReauthForPrimaryAccount(
+                  signin_metrics::ReauthAccessPoint::kPasswordMoveBubble, _))
+      .WillOnce(MoveArg<1>(&reauth_callback));
+  controller()->AuthenticateUserForAccountStoreOptInAndMovePassword();
+
+  // Once the reauth finishes with a success, the passwords is moved.
+  EXPECT_CALL(*form_manager, MoveCredentialsToAccountStore);
+  std::move(reauth_callback).Run(ReauthSucceeded(true));
+  EXPECT_FALSE(controller()->opened_automatic_bubble());
+  ExpectIconAndControllerStateIs(password_manager::ui::MANAGE_STATE);
+}
+
+TEST_F(ManagePasswordsUIControllerTest, ReauthenticateFailsBeforeMove) {
+  std::vector<const PasswordForm*> matches = {&test_local_form()};
+  auto test_form_manager = CreateFormManagerWithBestMatches(&matches);
+
+  // A submitted form triggers the move dialog.
+  EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
+  controller()->OnShowMoveToAccountBubble(std::move(test_form_manager));
+  EXPECT_TRUE(controller()->opened_automatic_bubble());
+  ExpectIconAndControllerStateIs(
+      password_manager::ui::CAN_MOVE_PASSWORD_TO_ACCOUNT_STATE);
+
+  // A user confirms the move which closes the dialog but opens a reauth.
+  base::OnceCallback<void(ReauthSucceeded)> reauth_callback;
+  EXPECT_CALL(client(),
+              TriggerReauthForPrimaryAccount(
+                  signin_metrics::ReauthAccessPoint::kPasswordMoveBubble, _))
+      .WillOnce(MoveArg<1>(&reauth_callback));
+  controller()->AuthenticateUserForAccountStoreOptInAndMovePassword();
+
+  // Once the reauth finishes with a failure, don't move the password.
+  std::move(reauth_callback).Run(ReauthSucceeded(false));
+  ExpectIconAndControllerStateIs(
+      password_manager::ui::CAN_MOVE_PASSWORD_TO_ACCOUNT_STATE);
+}

chrome/browser/ui/passwords/passwords_model_delegate.h

@@ -157,6 +157,11 @@ class PasswordsModelDelegate {
       const base::string16& username,
       const base::string16& password) = 0;
 
+  // Called from the Move bubble controller when gaia re-auth is needed
+  // to move passwords. This method triggers the reauth flow. Upon successful
+  // reauth, it moves the password.
+  virtual void AuthenticateUserForAccountStoreOptInAndMovePassword() = 0;
+
   // Returns true if the password values should be revealed when the bubble is
   // opened.
   virtual bool ArePasswordsRevealedWhenBubbleIsOpened() const = 0;

chrome/browser/ui/passwords/passwords_model_delegate_mock.h

@@ -64,6 +64,7 @@ class PasswordsModelDelegateMock
   MOCK_METHOD0(AuthenticateUser, bool());
   MOCK_METHOD2(AuthenticateUserForAccountStoreOptInAndSavePassword,
                void(const base::string16&, const base::string16&));
+  MOCK_METHOD0(AuthenticateUserForAccountStoreOptInAndMovePassword, void());
   MOCK_CONST_METHOD0(ArePasswordsRevealedWhenBubbleIsOpened, bool());
 
  private:

chrome/browser/ui/webui/signin/signin_reauth_ui.cc

@@ -58,6 +58,7 @@ int GetReauthTitleStringId(signin_metrics::ReauthAccessPoint access_point) {
     case signin_metrics::ReauthAccessPoint::kGeneratePasswordDropdown:
     case signin_metrics::ReauthAccessPoint::kGeneratePasswordContextMenu:
     case signin_metrics::ReauthAccessPoint::kPasswordSaveBubble:
+    case signin_metrics::ReauthAccessPoint::kPasswordMoveBubble:
       return IDS_ACCOUNT_PASSWORDS_REAUTH_SAVE_TITLE;
   }
 }
@@ -72,6 +73,7 @@ int GetReauthConfirmButtonLabelStringId(
     case signin_metrics::ReauthAccessPoint::kGeneratePasswordDropdown:
     case signin_metrics::ReauthAccessPoint::kGeneratePasswordContextMenu:
     case signin_metrics::ReauthAccessPoint::kPasswordSaveBubble:
+    case signin_metrics::ReauthAccessPoint::kPasswordMoveBubble:
       return IDS_ACCOUNT_PASSWORDS_REAUTH_SAVE_BUTTON_LABEL;
   }
 }

components/password_manager/core/browser/password_form_manager.cc

@@ -438,6 +438,7 @@ void PasswordFormManager::OnPasswordsRevealed() {
 }
 
 void PasswordFormManager::MoveCredentialsToAccountStore() {
+  DCHECK(client_->GetPasswordFeatureManager()->IsOptedInForAccountStorage());
   password_save_manager_->MoveCredentialsToAccountStore(
       metrics_util::MoveToAccountStoreTrigger::
           kSuccessfulLoginWithProfileStorePassword);

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -2550,6 +2550,8 @@ TEST_F(PasswordFormManagerTestWithMockedSaver, PermanentlyBlacklist) {
 }
 
 TEST_F(PasswordFormManagerTestWithMockedSaver, MoveCredentialsToAccountStore) {
+  ON_CALL(*client_.GetPasswordFeatureManager(), IsOptedInForAccountStorage)
+      .WillByDefault(Return(true));
   EXPECT_CALL(*mock_password_save_manager(),
               MoveCredentialsToAccountStore(
                   metrics_util::MoveToAccountStoreTrigger::

components/signin/public/base/signin_metrics.h

@@ -180,8 +180,9 @@ enum class ReauthAccessPoint {
   kPasswordSettings = 3,
   kGeneratePasswordDropdown = 4,
   kGeneratePasswordContextMenu = 5,
+  kPasswordMoveBubble = 6,
 
-  kMaxValue = kGeneratePasswordContextMenu
+  kMaxValue = kPasswordMoveBubble
 };
 
 // Enum values which enumerates all user actions on the sign-in promo.