Skip to content

Commit

Permalink
[UPMLocalPwd] Show password migration warning after TTF
Browse files Browse the repository at this point in the history
The CL triggers the password migration warning after the password
filling in the TouchToFillControllerAutofillDelegate.
The autosubmission is disabled if the warning is displayed, because it
interrupts the normal workflow.

Bug: 1439761
Change-Id: I0d5a6a747a005f62946c151dbb5edbca5dfe12a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4594562
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Commit-Queue: Anna Tsvirchkova <atsvirchkova@google.com>
Cr-Commit-Position: refs/heads/main@{#1158734}
  • Loading branch information
Anna Tsvirchkova authored and Chromium LUCI CQ committed Jun 16, 2023
1 parent 68a280a commit d0a4ff7
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "chrome/browser/password_manager/android/local_passwords_migration_warning_util.h"
#include "chrome/browser/password_manager/android/pwd_migration/jni_headers/PasswordMigrationWarningBridge_jni.h"
#include "chrome/browser/profiles/profile_android.h"
#include "components/password_manager/core/common/password_manager_features.h"
#include "ui/android/window_android.h"

using base::android::AttachCurrentThread;
Expand All @@ -15,4 +16,15 @@ void ShowWarning(const gfx::NativeWindow window, Profile* profile) {
AttachCurrentThread(), window->GetJavaObject(),
ProfileAndroid::FromProfile(profile)->GetJavaObject());
}

bool ShouldShowWarning() {
if (!base::FeatureList::IsEnabled(
password_manager::features::
kUnifiedPasswordManagerLocalPasswordsMigrationWarning)) {
return false;
}
// TODO(crbug.com/1451827): Implement the actual logic whether to show the
// warning here.
return true;
}
} // namespace password_manager
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

namespace password_manager {
void ShowWarning(const gfx::NativeWindow window, Profile* profile);
bool ShouldShowWarning();
}

#endif // CHROME_BROWSER_PASSWORD_MANAGER_ANDROID_LOCAL_PASSWORDS_MIGRATION_WARNING_UTIL_H_
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@

#include "base/base64.h"
#include "base/check.h"
#include "base/functional/bind.h"
#include "base/metrics/histogram_functions.h"
#include "base/ranges/algorithm.h"
#include "base/types/pass_key.h"
#include "chrome/browser/password_manager/android/local_passwords_migration_warning_util.h"
#include "chrome/browser/password_manager/chrome_password_manager_client.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/touch_to_fill/touch_to_fill_controller.h"
#include "components/device_reauth/device_authenticator.h"
#include "components/password_manager/core/browser/affiliation/affiliation_utils.h"
Expand All @@ -18,9 +21,12 @@
#include "components/password_manager/core/browser/password_credential_filler.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h"
#include "components/password_manager/core/browser/password_manager_util.h"
#include "content/public/browser/web_contents.h"
#include "services/metrics/public/cpp/ukm_builders.h"
#include "services/metrics/public/cpp/ukm_recorder.h"
#include "services/metrics/public/cpp/ukm_source_id.h"
#include "ui/android/window_android.h"
#include "ui/gfx/native_widget_types.h"
#include "url/gurl.h"

namespace {
Expand All @@ -39,18 +45,24 @@ bool ContainsNonEmptyUsername(

} // namespace

// No-op constructor for tests.
TouchToFillControllerAutofillDelegate::TouchToFillControllerAutofillDelegate(
base::PassKey<TouchToFillControllerAutofillTest>,
password_manager::PasswordManagerClient* password_client,
content::WebContents* web_contents,
scoped_refptr<device_reauth::DeviceAuthenticator> authenticator,
base::WeakPtr<password_manager::WebAuthnCredentialsDelegate>
webauthn_delegate,
std::unique_ptr<password_manager::PasswordCredentialFiller> filler,
ShowHybridOption should_show_hybrid_option)
ShowHybridOption should_show_hybrid_option,
ShowPasswordMigrationWarningCallback show_password_migration_warning)
: password_client_(password_client),
web_contents_(web_contents),
authenticator_(std::move(authenticator)),
webauthn_delegate_(webauthn_delegate),
filler_(std::move(filler)),
show_password_migration_warning_(
std::move(show_password_migration_warning)),
should_show_hybrid_option_(should_show_hybrid_option) {}

TouchToFillControllerAutofillDelegate::TouchToFillControllerAutofillDelegate(
Expand All @@ -61,9 +73,16 @@ TouchToFillControllerAutofillDelegate::TouchToFillControllerAutofillDelegate(
std::unique_ptr<password_manager::PasswordCredentialFiller> filler,
ShowHybridOption should_show_hybrid_option)
: password_client_(password_client),
// |TouchToFillControllerTest| doesn't provide an instance of
// |ChromePasswordManagerClient|, so the test-only constructor should
// be used there.
web_contents_(static_cast<ChromePasswordManagerClient*>(password_client_)
->web_contents()),
authenticator_(std::move(authenticator)),
webauthn_delegate_(webauthn_delegate),
filler_(std::move(filler)),
show_password_migration_warning_(
base::BindRepeating(&password_manager::ShowWarning)),
should_show_hybrid_option_(should_show_hybrid_option),
source_id_(password_client->web_contents()
->GetPrimaryMainFrame()
Expand Down Expand Up @@ -207,11 +226,7 @@ bool TouchToFillControllerAutofillDelegate::ShouldShowHybridOption() {
}

gfx::NativeView TouchToFillControllerAutofillDelegate::GetNativeView() {
// It is not a |ChromePasswordManagerClient| only in
// TouchToFillControllerTest.
return static_cast<ChromePasswordManagerClient*>(password_client_)
->web_contents()
->GetNativeView();
return web_contents_->GetNativeView();
}

void TouchToFillControllerAutofillDelegate::OnReauthCompleted(
Expand All @@ -237,8 +252,13 @@ void TouchToFillControllerAutofillDelegate::FillCredential(
CHECK(action_complete_);
CHECK(filler_->IsReadyToFill());

// Do not trigger autosubmission if the password migration warning is being
// shown because it interrupts the nomal workflow.
filler_->UpdateTriggerSubmission(ShouldTriggerSubmission() &&
!password_manager::ShouldShowWarning());
filler_->FillUsernameAndPassword(credential.username(),
credential.password());
ShowPasswordMigrationWarningIfNeeded();

if (ShouldTriggerSubmission()) {
password_client_->StartSubmissionTrackingAfterTouchToFill(
Expand All @@ -256,3 +276,13 @@ void TouchToFillControllerAutofillDelegate::CleanUpFillerAndReportOutcome(
filler_->CleanUp(ToShowVirtualKeyboard(show_virtual_keyboard));
base::UmaHistogramEnumeration("PasswordManager.TouchToFill.Outcome", outcome);
}

void TouchToFillControllerAutofillDelegate::
ShowPasswordMigrationWarningIfNeeded() {
if (!password_manager::ShouldShowWarning()) {
return;
}
show_password_migration_warning_.Run(
web_contents_->GetTopLevelNativeWindow(),
Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,22 @@ class UiCredential;
class WebAuthnCredentialsDelegate;
} // namespace password_manager

namespace content {
class WebContents;
}

class ChromePasswordManagerClient;
class TouchToFillController;
class Profile;

// Delegate interface for TouchToFillController being used in an autofill
// context.
class TouchToFillControllerAutofillDelegate
: public TouchToFillControllerDelegate {
public:
using ShowHybridOption = base::StrongAlias<struct ShowHybridOptionTag, bool>;
using ShowPasswordMigrationWarningCallback =
base::RepeatingCallback<void(gfx::NativeWindow, Profile*)>;

// The action a user took when interacting with the Touch To Fill sheet.
//
Expand Down Expand Up @@ -71,11 +78,13 @@ class TouchToFillControllerAutofillDelegate
TouchToFillControllerAutofillDelegate(
base::PassKey<class TouchToFillControllerAutofillTest>,
password_manager::PasswordManagerClient* password_client,
content::WebContents* web_contents,
scoped_refptr<device_reauth::DeviceAuthenticator> authenticator,
base::WeakPtr<password_manager::WebAuthnCredentialsDelegate>
webauthn_delegate,
std::unique_ptr<password_manager::PasswordCredentialFiller> filler,
ShowHybridOption should_show_hybrid_option);
ShowHybridOption should_show_hybrid_option,
ShowPasswordMigrationWarningCallback show_password_migration_warning);

TouchToFillControllerAutofillDelegate(
ChromePasswordManagerClient* password_client,
Expand Down Expand Up @@ -121,6 +130,8 @@ class TouchToFillControllerAutofillDelegate
void CleanUpFillerAndReportOutcome(TouchToFillOutcome outcome,
bool show_virtual_keyboard);

void ShowPasswordMigrationWarningIfNeeded();

// Callback to the controller to be invoked when a finalizing action has
// completed. This will result in the destruction of the delegate so
// no internal state should be touched after its invocation.
Expand All @@ -129,6 +140,8 @@ class TouchToFillControllerAutofillDelegate
// Weak pointer to the PasswordManagerClient this class is tied to.
raw_ptr<password_manager::PasswordManagerClient> password_client_ = nullptr;

raw_ptr<content::WebContents> web_contents_;

// Authenticator used to trigger a biometric auth before filling.
scoped_refptr<device_reauth::DeviceAuthenticator> authenticator_;

Expand All @@ -142,6 +155,10 @@ class TouchToFillControllerAutofillDelegate
// the form if required.
std::unique_ptr<password_manager::PasswordCredentialFiller> filler_;

// Shows the password migration warning (expected to be shown after filing
// user's credentials).
ShowPasswordMigrationWarningCallback show_password_migration_warning_;

// Whether the controller should show an option for passkey hybrid sign-in.
ShowHybridOption should_show_hybrid_option_ = ShowHybridOption(false);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// found in the LICENSE file.

#include "chrome/browser/touch_to_fill/touch_to_fill_controller_autofill_delegate.h"
#include "base/functional/callback_forward.h"
#include "base/test/mock_callback.h"

#include <memory>
#include <tuple>
Expand All @@ -20,7 +22,9 @@
#include "base/types/pass_key.h"
#include "chrome/browser/password_manager/android/password_manager_launcher_android.h"
#include "chrome/browser/password_manager/chrome_password_manager_client.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/touch_to_fill/touch_to_fill_controller.h"
#include "chrome/test/base/chrome_render_view_host_test_harness.h"
#include "components/autofill/core/common/mojom/autofill_types.mojom.h"
#include "components/device_reauth/device_authenticator.h"
#include "components/device_reauth/mock_device_authenticator.h"
Expand All @@ -35,6 +39,7 @@
#include "services/metrics/public/cpp/ukm_builders.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/gfx/native_widget_types.h"
#include "url/gurl.h"
#include "url/origin.h"

Expand All @@ -49,6 +54,7 @@ using device_reauth::MockDeviceAuthenticator;
using password_manager::PasskeyCredential;
using password_manager::UiCredential;
using ::testing::_;
using ::testing::AtLeast;
using ::testing::ElementsAreArray;
using ::testing::Eq;
using ::testing::Return;
Expand Down Expand Up @@ -127,11 +133,14 @@ UiCredential MakeUiCredential(MakeUiCredentialParams params) {

} // namespace

class TouchToFillControllerAutofillTest : public testing::Test {
class TouchToFillControllerAutofillTest
: public ChromeRenderViewHostTestHarness {
protected:
using UkmBuilder = ukm::builders::TouchToFill_Shown;

TouchToFillControllerAutofillTest() {
TouchToFillControllerAutofillTest()
: ChromeRenderViewHostTestHarness(
base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
password_manager_launcher::
OverrideManagePasswordWhenPasskeysPresentForTesting(false);

Expand Down Expand Up @@ -174,6 +183,12 @@ class TouchToFillControllerAutofillTest : public testing::Test {
return touch_to_fill_controller_;
}

base::MockCallback<
base::RepeatingCallback<void(gfx::NativeWindow, Profile*)>>&
show_password_migration_warning() {
return show_password_migration_warning_;
}

std::unique_ptr<TouchToFillControllerAutofillDelegate>
MakeTouchToFillControllerDelegate(
autofill::mojom::SubmissionReadinessState submission_readiness,
Expand All @@ -184,8 +199,9 @@ class TouchToFillControllerAutofillTest : public testing::Test {
.WillByDefault(Return(submission_readiness));
return std::make_unique<TouchToFillControllerAutofillDelegate>(
base::PassKey<TouchToFillControllerAutofillTest>(), &client_,
authenticator_, webauthn_credentials_delegate_.AsWeakPtr(),
std::move(filler), should_show_hybrid_option);
web_contents(), authenticator_,
webauthn_credentials_delegate_.AsWeakPtr(), std::move(filler),
should_show_hybrid_option, show_password_migration_warning().Get());
}

password_manager::MockWebAuthnCredentialsDelegate&
Expand All @@ -198,14 +214,13 @@ class TouchToFillControllerAutofillTest : public testing::Test {
}

void SetUp() override {
ChromeRenderViewHostTestHarness::SetUp();
auto mock_view = std::make_unique<MockTouchToFillView>();
mock_view_ = mock_view.get();
touch_to_fill_controller().set_view(std::move(mock_view));
}

private:
base::test::TaskEnvironment task_environment_{
base::test::TaskEnvironment::TimeSource::MOCK_TIME};
raw_ptr<MockTouchToFillView> mock_view_ = nullptr;
scoped_refptr<MockDeviceAuthenticator> authenticator_ =
base::MakeRefCounted<MockDeviceAuthenticator>();
Expand All @@ -216,6 +231,8 @@ class TouchToFillControllerAutofillTest : public testing::Test {
ukm::TestAutoSetUkmRecorder test_recorder_;
TouchToFillController touch_to_fill_controller_;
base::test::ScopedFeatureList scoped_feature_list_;
base::MockCallback<base::RepeatingCallback<void(gfx::NativeWindow, Profile*)>>
show_password_migration_warning_;
raw_ptr<MockPasswordCredentialFiller> weak_filler_;
};

Expand Down Expand Up @@ -309,6 +326,38 @@ TEST_F(TouchToFillControllerAutofillTest, Show_Fill_And_Dont_Submit) {
touch_to_fill_controller().OnCredentialSelected(credentials[0]);
}

TEST_F(TouchToFillControllerAutofillTest,
ShowFillAndShowPasswordMigrationWarning) {
scoped_feature_list().Reset();
scoped_feature_list().InitWithFeatures(
{password_manager::features::
kUnifiedPasswordManagerLocalPasswordsMigrationWarning},
{});
UiCredential credentials[] = {
MakeUiCredential({.username = "alice", .password = "p4ssw0rd"})};
auto filler_to_pass = CreateMockFiller();

EXPECT_CALL(view(), Show(Eq(GURL(kExampleCom)), IsOriginSecure(true),
ElementsAreArray(credentials),
ElementsAreArray(std::vector<PasskeyCredential>()),
TouchToFillView::kNone));
touch_to_fill_controller().Show(
credentials, {},
MakeTouchToFillControllerDelegate(
autofill::mojom::SubmissionReadinessState::kTwoFields,
std::move(filler_to_pass),
TouchToFillControllerAutofillDelegate::ShowHybridOption(false)));

EXPECT_CALL(*last_mock_filler(),
FillUsernameAndPassword(std::u16string(u"alice"),
std::u16string(u"p4ssw0rd")));
EXPECT_CALL(*last_mock_filler(), UpdateTriggerSubmission(false));
EXPECT_CALL(client(), StartSubmissionTrackingAfterTouchToFill(_)).Times(0);
EXPECT_CALL(show_password_migration_warning(), Run);

touch_to_fill_controller().OnCredentialSelected(credentials[0]);
}

TEST_F(TouchToFillControllerAutofillTest, Dont_Submit_With_Empty_Username) {
UiCredential credentials[] = {
MakeUiCredential({.username = "", .password = "p4ssw0rd"}),
Expand All @@ -335,6 +384,7 @@ TEST_F(TouchToFillControllerAutofillTest, Dont_Submit_With_Empty_Username) {
.WillByDefault(Return(false));
// The user picks the credential with an empty username, submission should not
// be triggered.
EXPECT_CALL(*last_mock_filler(), UpdateTriggerSubmission(false));
EXPECT_CALL(*last_mock_filler(),
FillUsernameAndPassword(std::u16string(u""),
std::u16string(u"p4ssw0rd")));
Expand Down Expand Up @@ -367,6 +417,7 @@ TEST_F(TouchToFillControllerAutofillTest,
EXPECT_CALL(*last_mock_filler(),
FillUsernameAndPassword(std::u16string(u""),
std::u16string(u"p4ssw0rd")));
EXPECT_CALL(*last_mock_filler(), UpdateTriggerSubmission(false));
EXPECT_CALL(client(), StartSubmissionTrackingAfterTouchToFill(_)).Times(0);

touch_to_fill_controller().OnCredentialSelected(credentials[0]);
Expand Down

0 comments on commit d0a4ff7

Please sign in to comment.