Commit 1ee151aa authored by Mohamed Amir Yosef's avatar Mohamed Amir Yosef Committed by Commit Bot

[Passwords] Invoke the Reauth flow from the save bubble for butter

For users who haven't opted-in butter, they should go the reauth flow
before being able to save passwords in their account.

If users have opted-in already or decided to store locally,
the credentials are saved directly without invoking the reauth flow.

Bug: 1044038
Change-Id: I519e2739d8f87bfc16b84a484c2aa1d522df0b9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2050280Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747261}
parent ecfd097c
......@@ -8,6 +8,7 @@
#include "base/time/default_clock.h"
#include "chrome/browser/password_manager/password_store_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/identity_manager_factory.h"
#include "chrome/browser/sync/profile_sync_service_factory.h"
#include "chrome/browser/ui/passwords/manage_passwords_view_utils.h"
#include "chrome/browser/ui/passwords/passwords_model_delegate.h"
......@@ -21,8 +22,10 @@
#include "components/password_manager/core/common/password_manager_features.h"
#include "components/password_manager/core/common/password_manager_pref_names.h"
#include "components/prefs/pref_service.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "components/sync/driver/sync_service.h"
#include "content/public/browser/web_contents.h"
#include "google_apis/gaia/core_account_id.h"
#include "ui/base/l10n/l10n_util.h"
namespace {
......@@ -83,6 +86,16 @@ bool IsSyncUser(Profile* profile) {
return password_bubble_experiment::IsSmartLockUser(sync_service);
}
CoreAccountId GetSignedInAccount(Profile* profile) {
signin::IdentityManager* identity_manager =
IdentityManagerFactory::GetForProfile(profile);
if (!identity_manager)
return CoreAccountId();
return identity_manager
->GetPrimaryAccountInfo(signin::ConsentLevel::kNotRequired)
.account_id;
}
} // namespace
SaveUpdateWithAccountStoreBubbleController::
......@@ -158,8 +171,19 @@ void SaveUpdateWithAccountStoreBubbleController::OnSaveClicked() {
dismissal_reason_ = metrics_util::CLICKED_SAVE;
if (delegate_) {
CleanStatisticsForSite(GetProfile(), origin_);
if (!IsUsingAccountStore() ||
delegate_->GetPasswordFeatureManager()->IsOptedInForAccountStorage()) {
// User is saving locally or already has opted in to the account store.
// Save directly without the need for reauth.
delegate_->SavePassword(pending_password_.username_value,
pending_password_.password_value);
} else {
// Otherwise, we should invoke the reauth flow before saving.
CoreAccountId account_id = GetSignedInAccount(GetProfile());
delegate_->AuthenticateUserForAccountStoreOptInAndSavePassword(
account_id, pending_password_.username_value,
pending_password_.password_value);
}
}
}
......
......@@ -18,6 +18,7 @@
#include "chrome/browser/password_manager/password_store_factory.h"
#include "chrome/browser/ui/passwords/passwords_model_delegate_mock.h"
#include "chrome/test/base/testing_profile.h"
#include "components/password_manager/core/browser/mock_password_feature_manager.h"
#include "components/password_manager/core/browser/mock_password_store.h"
#include "components/password_manager/core/browser/password_form_metrics_recorder.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h"
......@@ -68,6 +69,8 @@ class SaveUpdateWithAccountStoreBubbleControllerTest : public ::testing::Test {
content::WebContentsTester::CreateTestWebContents(&profile_, nullptr);
mock_delegate_ =
std::make_unique<testing::NiceMock<PasswordsModelDelegateMock>>();
ON_CALL(*mock_delegate_, GetPasswordFeatureManager())
.WillByDefault(Return(&password_feature_manager_));
ON_CALL(*mock_delegate_, GetPasswordFormMetricsRecorder())
.WillByDefault(Return(nullptr));
PasswordStoreFactory::GetInstance()->SetTestingFactoryAndUse(
......@@ -99,6 +102,10 @@ class SaveUpdateWithAccountStoreBubbleControllerTest : public ::testing::Test {
PasswordsModelDelegateMock* delegate() { return mock_delegate_.get(); }
password_manager::MockPasswordFeatureManager* password_feature_manager() {
return &password_feature_manager_;
}
SaveUpdateWithAccountStoreBubbleController* controller() {
return controller_.get();
}
......@@ -128,6 +135,8 @@ class SaveUpdateWithAccountStoreBubbleControllerTest : public ::testing::Test {
TestingProfile profile_;
std::unique_ptr<content::WebContents> test_web_contents_;
std::unique_ptr<SaveUpdateWithAccountStoreBubbleController> controller_;
testing::NiceMock<password_manager::MockPasswordFeatureManager>
password_feature_manager_;
std::unique_ptr<PasswordsModelDelegateMock> mock_delegate_;
autofill::PasswordForm pending_password_;
};
......@@ -257,7 +266,32 @@ TEST_F(SaveUpdateWithAccountStoreBubbleControllerTest,
password_manager::metrics_util::NO_DIRECT_INTERACTION);
}
TEST_F(SaveUpdateWithAccountStoreBubbleControllerTest, ClickSave) {
TEST_F(SaveUpdateWithAccountStoreBubbleControllerTest, ClickSaveInLocalStore) {
ON_CALL(*password_feature_manager(), GetDefaultPasswordStore)
.WillByDefault(Return(autofill::PasswordForm::Store::kProfileStore));
PretendPasswordWaiting();
EXPECT_TRUE(controller()->enable_editing());
EXPECT_FALSE(controller()->IsCurrentStateUpdate());
EXPECT_CALL(*GetStore(), RemoveSiteStatsImpl(GURL(kSiteOrigin).GetOrigin()));
EXPECT_CALL(*delegate(), OnPasswordsRevealed()).Times(0);
EXPECT_CALL(*delegate(), SavePassword(pending_password().username_value,
pending_password().password_value));
EXPECT_CALL(*delegate(), NeverSavePassword()).Times(0);
EXPECT_CALL(*delegate(), OnNopeUpdateClicked()).Times(0);
EXPECT_CALL(*delegate(), AuthenticateUserForAccountStoreOptInAndSavePassword)
.Times(0);
controller()->OnSaveClicked();
DestroyModelExpectReason(password_manager::metrics_util::CLICKED_SAVE);
}
TEST_F(SaveUpdateWithAccountStoreBubbleControllerTest,
ClickSaveInAccountStoreWhileOptedIn) {
ON_CALL(*password_feature_manager(), GetDefaultPasswordStore)
.WillByDefault(Return(autofill::PasswordForm::Store::kAccountStore));
ON_CALL(*password_feature_manager(), IsOptedInForAccountStorage)
.WillByDefault(Return(true));
PretendPasswordWaiting();
EXPECT_TRUE(controller()->enable_editing());
......@@ -269,6 +303,30 @@ TEST_F(SaveUpdateWithAccountStoreBubbleControllerTest, ClickSave) {
pending_password().password_value));
EXPECT_CALL(*delegate(), NeverSavePassword()).Times(0);
EXPECT_CALL(*delegate(), OnNopeUpdateClicked()).Times(0);
EXPECT_CALL(*delegate(), AuthenticateUserForAccountStoreOptInAndSavePassword)
.Times(0);
controller()->OnSaveClicked();
DestroyModelExpectReason(password_manager::metrics_util::CLICKED_SAVE);
}
TEST_F(SaveUpdateWithAccountStoreBubbleControllerTest,
ClickSaveInAccountStoreWhileNotOptedIn) {
ON_CALL(*password_feature_manager(), GetDefaultPasswordStore)
.WillByDefault(Return(autofill::PasswordForm::Store::kAccountStore));
ON_CALL(*password_feature_manager(), IsOptedInForAccountStorage)
.WillByDefault(Return(false));
PretendPasswordWaiting();
EXPECT_TRUE(controller()->enable_editing());
EXPECT_FALSE(controller()->IsCurrentStateUpdate());
EXPECT_CALL(*GetStore(), RemoveSiteStatsImpl(GURL(kSiteOrigin).GetOrigin()));
EXPECT_CALL(*delegate(), SavePassword).Times(0);
EXPECT_CALL(*delegate(), NeverSavePassword()).Times(0);
EXPECT_CALL(*delegate(), OnNopeUpdateClicked()).Times(0);
EXPECT_CALL(*delegate(), AuthenticateUserForAccountStoreOptInAndSavePassword(
_, pending_password().username_value,
pending_password().password_value));
controller()->OnSaveClicked();
DestroyModelExpectReason(password_manager::metrics_util::CLICKED_SAVE);
}
......
......@@ -20,8 +20,7 @@
namespace password_manager {
class PasswordFormManagerForUI;
class PasswordManagerClient;
}
} // namespace password_manager
// ManagePasswordsState keeps the current state for ManagePasswordsUIController
// as well as up-to-date data for this state.
......@@ -38,6 +37,8 @@ class ManagePasswordsState {
client_ = client;
}
password_manager::PasswordManagerClient* client() { return client_; }
// The methods below discard the current state/data of the object and move it
// to the specified state.
......
......@@ -16,6 +16,7 @@
#include "chrome/browser/password_manager/account_storage/account_password_store_factory.h"
#include "chrome/browser/password_manager/chrome_password_manager_client.h"
#include "chrome/browser/password_manager/password_store_factory.h"
#include "chrome/browser/signin/identity_manager_factory.h"
#include "chrome/browser/signin/signin_ui_util.h"
#include "chrome/browser/ui/autofill/autofill_bubble_handler.h"
#include "chrome/browser/ui/browser_command_controller.h"
......@@ -41,8 +42,10 @@
#include "components/password_manager/core/browser/password_ui_utils.h"
#include "components/password_manager/core/browser/statistics_table.h"
#include "components/password_manager/core/common/credential_manager_types.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/web_contents.h"
#include "google_apis/gaia/core_account_id.h"
#include "ui/base/l10n/l10n_util.h"
#if defined(OS_WIN)
......@@ -332,8 +335,7 @@ ManagePasswordsUIController::GetPasswordFormMetricsRecorder() {
password_manager::PasswordFeatureManager*
ManagePasswordsUIController::GetPasswordFeatureManager() {
password_manager::PasswordManagerClient* client =
ChromePasswordManagerClient::FromWebContents(web_contents());
password_manager::PasswordManagerClient* client = passwords_data_.client();
return client->GetPasswordFeatureManager();
}
......@@ -451,7 +453,6 @@ void ManagePasswordsUIController::SavePassword(const base::string16& username,
password_manager::PasswordFormMetricsRecorder::DetailedUserAction::
kTriggeredManualFallbackForSaving);
}
save_fallback_timer_.Stop();
passwords_data_.form_manager()->Save();
passwords_data_.TransitionToState(password_manager::ui::MANAGE_STATE);
......@@ -535,6 +536,20 @@ bool ManagePasswordsUIController::AuthenticateUser() {
#endif
}
void ManagePasswordsUIController::
AuthenticateUserForAccountStoreOptInAndSavePassword(
CoreAccountId account_id,
const base::string16& username,
const base::string16& password) {
password_manager::PasswordManagerClient* client = passwords_data_.client();
client->TriggerReauthForAccount(
account_id,
base::BindOnce(&ManagePasswordsUIController::
AuthenticateUserForAccountStoreOptInCallback,
weak_ptr_factory_.GetWeakPtr(), passwords_data_.origin(),
passwords_data_.form_manager(), username, password));
}
bool ManagePasswordsUIController::ArePasswordsRevealedWhenBubbleIsOpened()
const {
return are_passwords_revealed_when_next_bubble_is_opened_;
......@@ -682,4 +697,31 @@ bool ManagePasswordsUIController::ShowAuthenticationDialog() {
#endif
}
void ManagePasswordsUIController::AuthenticateUserForAccountStoreOptInCallback(
const GURL& origin,
password_manager::PasswordFormManagerForUI* form_manager,
const base::string16& username,
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).
if (passwords_data_.origin() == origin &&
passwords_data_.form_manager() == form_manager) {
SavePassword(username, password);
}
return;
}
// If reauth wasn't successful, change to local store and reopen the bubble is
// the state didn't change.
GetPasswordFeatureManager()->SetDefaultPasswordStore(
autofill::PasswordForm::Store::kProfileStore);
if (passwords_data_.state() != password_manager::ui::PENDING_PASSWORD_STATE)
return;
bubble_status_ = BubbleStatus::SHOULD_POP_UP;
UpdateBubbleAndIconVisibility();
}
WEB_CONTENTS_USER_DATA_KEY_IMPL(ManagePasswordsUIController)
......@@ -15,6 +15,7 @@
#include "chrome/browser/ui/passwords/passwords_leak_dialog_delegate.h"
#include "chrome/browser/ui/passwords/passwords_model_delegate.h"
#include "chrome/common/buildflags.h"
#include "components/password_manager/core/browser/password_manager_client.h"
#include "components/password_manager/core/browser/password_store.h"
#include "content/public/browser/web_contents_observer.h"
#include "content/public/browser/web_contents_user_data.h"
......@@ -142,6 +143,10 @@ class ManagePasswordsUIController
bool is_default_promo_account) override;
void OnDialogHidden() override;
bool AuthenticateUser() override;
void AuthenticateUserForAccountStoreOptInAndSavePassword(
CoreAccountId account_id,
const base::string16& username,
const base::string16& password) override;
bool ArePasswordsRevealedWhenBubbleIsOpened() const override;
#if defined(UNIT_TEST)
......@@ -243,6 +248,19 @@ class ManagePasswordsUIController
// Shows an authentication dialog and returns true if auth is successful.
virtual bool ShowAuthenticationDialog();
// 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.
void AuthenticateUserForAccountStoreOptInCallback(
const GURL& origin,
password_manager::PasswordFormManagerForUI* form_manager,
const base::string16& username,
const base::string16& password,
password_manager::PasswordManagerClient::ReauthSucceeded
reauth_succeeded);
// Timeout in seconds for the manual fallback for saving.
static int save_fallback_timeout_in_seconds_;
......
......@@ -12,6 +12,7 @@
#include "base/strings/string16.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/gmock_move_support.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/mock_callback.h"
#include "build/build_config.h"
......@@ -26,6 +27,7 @@
#include "components/password_manager/core/browser/mock_password_form_manager_for_ui.h"
#include "components/password_manager/core/browser/password_bubble_experiment.h"
#include "components/password_manager/core/browser/password_form_metrics_recorder.h"
#include "components/password_manager/core/browser/password_manager_client.h"
#include "components/password_manager/core/browser/statistics_table.h"
#include "components/password_manager/core/browser/stub_password_manager_client.h"
#include "components/password_manager/core/common/password_manager_ui.h"
......@@ -35,6 +37,7 @@
#include "content/public/test/mock_navigation_handle.h"
#include "content/public/test/test_utils.h"
#include "content/public/test/web_contents_tester.h"
#include "google_apis/gaia/core_account_id.h"
#include "services/metrics/public/cpp/ukm_builders.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -42,6 +45,8 @@
using autofill::PasswordForm;
using base::ASCIIToUTF16;
using password_manager::MockPasswordFormManagerForUI;
using ReauthSucceeded =
password_manager::PasswordManagerClient::ReauthSucceeded;
using ::testing::_;
using ::testing::AtMost;
using ::testing::Contains;
......@@ -85,7 +90,16 @@ class TestManagePasswordsIconView : public ManagePasswordsIconView {
password_manager::ui::State state_;
};
// This sublass is used to disable some code paths which are not essential for
class MockPasswordManagerClient
: public password_manager::StubPasswordManagerClient {
public:
MOCK_METHOD(void,
TriggerReauthForAccount,
(const CoreAccountId&, base::OnceCallback<void(ReauthSucceeded)>),
(override));
};
// This subclass is used to disable some code paths which are not essential for
// testing.
class TestManagePasswordsUIController : public ManagePasswordsUIController {
public:
......@@ -159,7 +173,7 @@ class ManagePasswordsUIControllerTest : public ChromeRenderViewHostTestHarness {
public:
void SetUp() override;
password_manager::StubPasswordManagerClient& client() { return client_; }
MockPasswordManagerClient& client() { return client_; }
PasswordForm& test_local_form() { return test_local_form_; }
PasswordForm& test_federated_form() { return test_federated_form_; }
PasswordForm& submitted_form() { return submitted_form_; }
......@@ -184,7 +198,7 @@ class ManagePasswordsUIControllerTest : public ChromeRenderViewHostTestHarness {
void TestNotChangingStateOnAutofill(password_manager::ui::State state);
private:
password_manager::StubPasswordManagerClient client_;
MockPasswordManagerClient client_;
PasswordForm test_local_form_;
PasswordForm test_federated_form_;
......@@ -501,6 +515,78 @@ TEST_F(ManagePasswordsUIControllerTest, PasswordSavedUKMRecording) {
}
}
TEST_F(ManagePasswordsUIControllerTest,
PasswordSavedInAccountStoreWhenReauthSucceeds) {
std::vector<const PasswordForm*> best_matches;
auto test_form_manager = CreateFormManagerWithBestMatches(&best_matches);
MockPasswordFormManagerForUI* test_form_manager_ptr = test_form_manager.get();
EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
controller()->OnPasswordSubmitted(std::move(test_form_manager));
// The user hasn't opted in, so a reauth flow will be triggered.
base::OnceCallback<void(ReauthSucceeded)> reauth_callback;
EXPECT_CALL(client(), TriggerReauthForAccount)
.WillOnce(MoveArg<1>(&reauth_callback));
// The user clicks save which will invoke the reauth flow.
controller()->AuthenticateUserForAccountStoreOptInAndSavePassword(
CoreAccountId(), submitted_form().username_value,
submitted_form().password_value);
// 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));
EXPECT_CALL(*test_form_manager_ptr, Save());
std::move(reauth_callback).Run(ReauthSucceeded(true));
// We should be now in the manage state and no other bubble should be opened
// automatically.
ExpectIconAndControllerStateIs(password_manager::ui::MANAGE_STATE);
}
TEST_F(ManagePasswordsUIControllerTest,
PasswordNotSavedInAccountStoreWhenReauthFails) {
std::vector<const PasswordForm*> best_matches;
auto test_form_manager = CreateFormManagerWithBestMatches(&best_matches);
EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
EXPECT_CALL(*test_form_manager, Save()).Times(0);
controller()->OnPasswordSubmitted(std::move(test_form_manager));
// The user hasn't opted in, so a reauth flow will be triggered.
base::OnceCallback<void(ReauthSucceeded)> reauth_callback;
EXPECT_CALL(client(), TriggerReauthForAccount)
.WillOnce(MoveArg<1>(&reauth_callback));
// Unsuccessful reauth should change the default store to profile store.
EXPECT_CALL(
*client().GetPasswordFeatureManager(),
SetDefaultPasswordStore(autofill::PasswordForm::Store::kProfileStore));
// The user clicks save which will invoke the reauth flow.
controller()->AuthenticateUserForAccountStoreOptInAndSavePassword(
CoreAccountId(), submitted_form().username_value,
submitted_form().password_value);
// The bubble gets hidden after the user clicks on save.
controller()->OnBubbleHidden();
// Simulate an unsuccessful reauth which will cause the bubble to be open
// again.
EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
std::move(reauth_callback).Run(ReauthSucceeded(false));
// The bubble should have been opened again.
ExpectIconAndControllerStateIs(password_manager::ui::PENDING_PASSWORD_STATE);
EXPECT_TRUE(controller()->opened_automatic_bubble());
}
TEST_F(ManagePasswordsUIControllerTest, PasswordBlacklisted) {
std::vector<const PasswordForm*> best_matches;
auto test_form_manager = CreateFormManagerWithBestMatches(&best_matches);
......
......@@ -12,6 +12,7 @@
#include "components/password_manager/core/browser/manage_passwords_referrer.h"
#include "components/password_manager/core/common/credential_manager_types.h"
#include "components/password_manager/core/common/password_manager_ui.h"
#include "google_apis/gaia/core_account_id.h"
namespace autofill {
struct PasswordForm;
......@@ -127,6 +128,15 @@ class PasswordsModelDelegate {
// if the authentication was successful.
virtual bool AuthenticateUser() = 0;
// Called from the Save/Update bubble controller when gaia re-auth is needed
// to save passwords. This method triggers the reauth flow. Upon successful
// reauth, it saves the password if it's still relevant. Otherwise, it changes
// the default destination to local and reopens the save bubble.
virtual void AuthenticateUserForAccountStoreOptInAndSavePassword(
CoreAccountId account_id,
const base::string16& username,
const base::string16& password) = 0;
// Returns true if the password values should be revealed when the bubble is
// opened.
virtual bool ArePasswordsRevealedWhenBubbleIsOpened() const = 0;
......
......@@ -56,6 +56,10 @@ class PasswordsModelDelegateMock
void(const AccountInfo& account, bool is_default_promo_account));
MOCK_METHOD0(OnDialogHidden, void());
MOCK_METHOD0(AuthenticateUser, bool());
MOCK_METHOD3(AuthenticateUserForAccountStoreOptInAndSavePassword,
void(CoreAccountId,
const base::string16&,
const base::string16&));
MOCK_CONST_METHOD0(ArePasswordsRevealedWhenBubbleIsOpened, bool());
private:
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment