Skip to content

Commit

Permalink
[TTFPassGen] Reject generated password
Browse files Browse the repository at this point in the history
When user clicks the "Create my own" button in the password generation
bottom sheet, the following should happen:
- The bottom sheet is closed.
- The keyboard and KA with the "Suggest strong password" button is shown
for the password field.

This CL handles dismissing the bottom sheet and re-triggering the button
in the KA. But re-triggering the keyboard is non-trivial and will be
handled in a separate CL.

Bug: 1421753
Change-Id: I65517065a194693fd23ca2256632ebae34e75ab2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4797769
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Anna Tsvirchkova <atsvirchkova@google.com>
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188428}
  • Loading branch information
Anna Tsvirchkova authored and Chromium LUCI CQ committed Aug 25, 2023
1 parent 854d039 commit c1e8db3
Show file tree
Hide file tree
Showing 23 changed files with 161 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ class PasswordGenerationController {

virtual std::unique_ptr<TouchToFillPasswordGenerationController>
CreateTouchToFillGenerationControllerForTesting(
std::unique_ptr<TouchToFillPasswordGenerationBridge> bridge) = 0;
std::unique_ptr<TouchToFillPasswordGenerationBridge> bridge,
base::WeakPtr<ManualFillingController> manual_filling_controller) = 0;

// -----------------
// Member accessors:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,19 +224,22 @@ PasswordGenerationControllerImpl::CreateTouchToFillGenerationController() {
std::make_unique<TouchToFillPasswordGenerationBridgeImpl>(),
base::BindOnce(&PasswordGenerationControllerImpl::
OnTouchToFillForGenerationDismissed,
base::Unretained(this)));
base::Unretained(this)),
ManualFillingController::GetOrCreate(&GetWebContents()));
}

std::unique_ptr<TouchToFillPasswordGenerationController>
PasswordGenerationControllerImpl::
CreateTouchToFillGenerationControllerForTesting(
std::unique_ptr<TouchToFillPasswordGenerationBridge> bridge) {
std::unique_ptr<TouchToFillPasswordGenerationBridge> bridge,
base::WeakPtr<ManualFillingController> manual_filling_controller) {
return std::make_unique<TouchToFillPasswordGenerationController>(
active_frame_driver_, &GetWebContents(), *generation_element_data_,
std::move(bridge),
base::BindOnce(&PasswordGenerationControllerImpl::
OnTouchToFillForGenerationDismissed,
base::Unretained(this)));
base::Unretained(this)),
manual_filling_controller);
}

void PasswordGenerationControllerImpl::ShowDialog(PasswordGenerationType type) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ class PasswordGenerationControllerImpl
// for testing.
std::unique_ptr<TouchToFillPasswordGenerationController>
CreateTouchToFillGenerationControllerForTesting(
std::unique_ptr<TouchToFillPasswordGenerationBridge> bridge) override;
std::unique_ptr<TouchToFillPasswordGenerationBridge> bridge,
base::WeakPtr<ManualFillingController> manual_filling_controller)
override;
gfx::NativeWindow top_level_native_window() override;
content::WebContents* web_contents() override;
autofill::FieldSignature get_field_signature_for_testing() override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,8 @@ class PasswordGenerationControllerTest

ON_CALL(create_ttf_generation_controller_, Run).WillByDefault([this]() {
return controller()->CreateTouchToFillGenerationControllerForTesting(
std::make_unique<MockTouchToFillPasswordGenerationBridge>());
std::make_unique<MockTouchToFillPasswordGenerationBridge>(),
mock_manual_filling_controller_.AsWeakPtr());
});

PasswordGenerationControllerImpl::CreateForWebContentsForTesting(
Expand Down Expand Up @@ -491,7 +492,8 @@ TEST_F(PasswordGenerationControllerTest,
EXPECT_CALL(create_ttf_generation_controller_, Run)
.WillOnce([this, &ttf_password_generation_bridge]() {
return controller()->CreateTouchToFillGenerationControllerForTesting(
std::move(ttf_password_generation_bridge));
std::move(ttf_password_generation_bridge),
mock_manual_filling_controller_.AsWeakPtr());
});

// Keyboard accessory shouldn't show up.
Expand Down Expand Up @@ -543,7 +545,8 @@ TEST_F(PasswordGenerationControllerTest,
EXPECT_CALL(create_ttf_generation_controller_, Run)
.WillOnce([this, &ttf_password_generation_bridge]() {
return controller()->CreateTouchToFillGenerationControllerForTesting(
std::move(ttf_password_generation_bridge));
std::move(ttf_password_generation_bridge),
mock_manual_filling_controller_.AsWeakPtr());
});

// Keyboard accessory should show up.
Expand All @@ -567,7 +570,8 @@ TEST_F(PasswordGenerationControllerTest,
EXPECT_CALL(create_ttf_generation_controller_, Run)
.WillOnce([this, &ttf_password_generation_bridge]() {
return controller()->CreateTouchToFillGenerationControllerForTesting(
std::move(ttf_password_generation_bridge));
std::move(ttf_password_generation_bridge),
mock_manual_filling_controller_.AsWeakPtr());
});

// Keyboard accessory shouldn't be called.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ source_set("public") {
deps = [
":android",
"//base",
"//chrome/browser/autofill:autofill",
"//chrome/browser/password_manager/android:password_generation_utils",
"//components/password_manager/content/browser:browser",
"//content/public/browser",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ FakeTouchToFillPasswordGenerationBridge::

bool FakeTouchToFillPasswordGenerationBridge::Show(
content::WebContents* web_contents,
base::WeakPtr<TouchToFillPasswordGenerationDelegate> delegate,
TouchToFillPasswordGenerationDelegate* delegate,
std::u16string password,
std::string account) {
delegate_ = delegate;
Expand All @@ -31,3 +31,6 @@ void FakeTouchToFillPasswordGenerationBridge::OnDismissed(JNIEnv* env) {
void FakeTouchToFillPasswordGenerationBridge::OnGeneratedPasswordAccepted(
JNIEnv* env,
const base::android::JavaParamRef<jstring>& password) {}

void FakeTouchToFillPasswordGenerationBridge::OnGeneratedPasswordRejected(
JNIEnv* env) {}
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,18 @@ class FakeTouchToFillPasswordGenerationBridge
~FakeTouchToFillPasswordGenerationBridge() override;

bool Show(content::WebContents* web_contents,
base::WeakPtr<TouchToFillPasswordGenerationDelegate> delegate,
TouchToFillPasswordGenerationDelegate* delegate,
std::u16string password,
std::string account) override;
void Hide() override;
void OnDismissed(JNIEnv* env) override;
void OnGeneratedPasswordAccepted(
JNIEnv* env,
const base::android::JavaParamRef<jstring>& password) override;
void OnGeneratedPasswordRejected(JNIEnv* env) override;

private:
base::WeakPtr<TouchToFillPasswordGenerationDelegate> delegate_;
raw_ptr<TouchToFillPasswordGenerationDelegate> delegate_;
};

#endif // CHROME_BROWSER_TOUCH_TO_FILL_PASSWORD_GENERATION_ANDROID_FAKE_TOUCH_TO_FILL_PASSWORD_GENERATION_BRIDGE_H_
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
android:gravity="center"
style="style/FilledButton.Flat"/>
<org.chromium.ui.widget.ButtonCompat
android:id="@+id/create_password_button"
android:id="@+id/reject_password_button"
android:text="@string/password_generation_bottom_sheet_dismiss_button"
android:layout_width="match_parent"
android:layout_height="wrap_content"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,19 @@ public void onGeneratedPasswordAccepted(String password) {
mNativeTouchToFillPasswordGenerationBridge, password);
}

@Override
public void onGeneratedPasswordRejected() {
if (mNativeTouchToFillPasswordGenerationBridge == 0) return;

TouchToFillPasswordGenerationBridgeJni.get().onGeneratedPasswordRejected(
mNativeTouchToFillPasswordGenerationBridge);
}

@NativeMethods
interface Natives {
void onDismissed(long nativeTouchToFillPasswordGenerationBridge);
void onGeneratedPasswordAccepted(
long nativeTouchToFillPasswordGenerationBridge, String password);
void onGeneratedPasswordRejected(long nativeTouchToFillPasswordGenerationBridge);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,10 @@ public void testOnGeneratedPasswordAccepted() {
mBridge.onGeneratedPasswordAccepted(generatedPassword);
verify(mBridgeJniMock).onGeneratedPasswordAccepted(sTestNativePointer, generatedPassword);
}

@Test
public void testOnGeneratedPasswordRejected() {
mBridge.onGeneratedPasswordRejected();
verify(mBridgeJniMock).onGeneratedPasswordRejected(sTestNativePointer);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import static org.chromium.chrome.browser.touch_to_fill.password_generation.TouchToFillPasswordGenerationProperties.ACCOUNT_EMAIL;
import static org.chromium.chrome.browser.touch_to_fill.password_generation.TouchToFillPasswordGenerationProperties.GENERATED_PASSWORD;
import static org.chromium.chrome.browser.touch_to_fill.password_generation.TouchToFillPasswordGenerationProperties.PASSWORD_ACCEPTED_CALLBACK;
import static org.chromium.chrome.browser.touch_to_fill.password_generation.TouchToFillPasswordGenerationProperties.PASSWORD_REJECTED_CALLBACK;

import android.content.Context;
import android.view.LayoutInflater;
Expand Down Expand Up @@ -35,6 +36,9 @@ interface Delegate {
* @param password the password, which was used.
*/
void onGeneratedPasswordAccepted(String password);

/** Called when the user rejects the generated password. */
void onGeneratedPasswordRejected();
}

private final TouchToFillPasswordGenerationView mTouchToFillPasswordGenerationView;
Expand Down Expand Up @@ -76,6 +80,7 @@ public boolean show(String generatedPassword, String account) {
.with(ACCOUNT_EMAIL, account)
.with(GENERATED_PASSWORD, generatedPassword)
.with(PASSWORD_ACCEPTED_CALLBACK, this::onGeneratedPasswordAccepted)
.with(PASSWORD_REJECTED_CALLBACK, this::onGeneratedPasswordRejected)
.build();
setUpModelChangeProcessors(model, mTouchToFillPasswordGenerationView);

Expand All @@ -99,7 +104,12 @@ private void onDismissed(@StateChangeReason int reason) {

private void onGeneratedPasswordAccepted(String password) {
mTouchToFillPasswordGenerationDelegate.onGeneratedPasswordAccepted(password);
onDismissed(StateChangeReason.NONE);
onDismissed(StateChangeReason.INTERACTION_COMPLETE);
}

private void onGeneratedPasswordRejected() {
mTouchToFillPasswordGenerationDelegate.onGeneratedPasswordRejected();
onDismissed(StateChangeReason.INTERACTION_COMPLETE);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,23 @@ public void testBottomSheetIsHiddenAfterAcceptingPassword() {
verify(mBottomSheetController).hideContent(any(), anyBoolean());
verify(mDelegate).onDismissed();
}

@Test
public void testGeneratedPasswordRejectedCalled() {
mCoordinator.show(sGeneratedPassword, sTestEmailAddress);

Button rejectPasswordButton = mContent.findViewById(R.id.reject_password_button);
rejectPasswordButton.performClick();
verify(mDelegate).onGeneratedPasswordRejected();
}

@Test
public void testBottomSheetIsHiddenAfterRejectingPassword() {
mCoordinator.show(sGeneratedPassword, sTestEmailAddress);

Button rejectPasswordButton = mContent.findViewById(R.id.reject_password_button);
rejectPasswordButton.performClick();
verify(mBottomSheetController).hideContent(any(), anyBoolean());
verify(mDelegate).onDismissed();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ class TouchToFillPasswordGenerationProperties {
public static final ReadableObjectPropertyKey<Callback<String>> PASSWORD_ACCEPTED_CALLBACK =
new ReadableObjectPropertyKey<>();

public static final PropertyKey[] ALL_KEYS =
new PropertyKey[] {ACCOUNT_EMAIL, GENERATED_PASSWORD, PASSWORD_ACCEPTED_CALLBACK};
public static final ReadableObjectPropertyKey<Runnable> PASSWORD_REJECTED_CALLBACK =
new ReadableObjectPropertyKey<>();

public static final PropertyKey[] ALL_KEYS = new PropertyKey[] {ACCOUNT_EMAIL,
GENERATED_PASSWORD, PASSWORD_ACCEPTED_CALLBACK, PASSWORD_REJECTED_CALLBACK};
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ void setPasswordAcceptedCallback(Callback<String> callback) {
(v) -> callback.onResult(mPasswordView.getText().toString()));
}

void setPasswordRejectedCallback(Runnable callback) {
Button passwordRejectedButton = mContent.findViewById(R.id.reject_password_button);
passwordRejectedButton.setOnClickListener((v) -> callback.run());
}

@Override
public View getContentView() {
return mContent;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import static org.chromium.chrome.browser.touch_to_fill.password_generation.TouchToFillPasswordGenerationProperties.ACCOUNT_EMAIL;
import static org.chromium.chrome.browser.touch_to_fill.password_generation.TouchToFillPasswordGenerationProperties.GENERATED_PASSWORD;
import static org.chromium.chrome.browser.touch_to_fill.password_generation.TouchToFillPasswordGenerationProperties.PASSWORD_ACCEPTED_CALLBACK;
import static org.chromium.chrome.browser.touch_to_fill.password_generation.TouchToFillPasswordGenerationProperties.PASSWORD_REJECTED_CALLBACK;

import org.chromium.ui.modelutil.PropertyKey;
import org.chromium.ui.modelutil.PropertyModel;
Expand All @@ -30,6 +31,8 @@ static void bindView(
view.setGeneratedPassword(model.get(GENERATED_PASSWORD));
} else if (propertyKey == PASSWORD_ACCEPTED_CALLBACK) {
view.setPasswordAcceptedCallback(model.get(PASSWORD_ACCEPTED_CALLBACK));
} else if (propertyKey == PASSWORD_REJECTED_CALLBACK) {
view.setPasswordRejectedCallback(model.get(PASSWORD_REJECTED_CALLBACK));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class MockTouchToFillPasswordGenerationBridge
MOCK_METHOD(bool,
Show,
(content::WebContents*,
base::WeakPtr<TouchToFillPasswordGenerationDelegate>,
TouchToFillPasswordGenerationDelegate*,
std::u16string,
std::string),
(override));
Expand All @@ -29,6 +29,7 @@ class MockTouchToFillPasswordGenerationBridge
OnGeneratedPasswordAccepted,
(JNIEnv*, const base::android::JavaParamRef<jstring>&),
(override));
MOCK_METHOD(void, OnGeneratedPasswordRejected, (JNIEnv*), (override));
};

#endif // CHROME_BROWSER_TOUCH_TO_FILL_PASSWORD_GENERATION_ANDROID_MOCK_TOUCH_TO_FILL_PASSWORD_GENERATION_BRIDGE_H_
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,16 @@ class TouchToFillPasswordGenerationBridge {
public:
virtual ~TouchToFillPasswordGenerationBridge() = default;

virtual bool Show(
content::WebContents* web_contents,
base::WeakPtr<TouchToFillPasswordGenerationDelegate> delegate_,
std::u16string password,
std::string account) = 0;
virtual bool Show(content::WebContents* web_contents,
TouchToFillPasswordGenerationDelegate* delegate,
std::u16string password,
std::string account) = 0;
virtual void Hide() = 0;
virtual void OnDismissed(JNIEnv* env) = 0;
virtual void OnGeneratedPasswordAccepted(
JNIEnv* env,
const base::android::JavaParamRef<jstring>& password) = 0;
virtual void OnGeneratedPasswordRejected(JNIEnv* env) = 0;
};

#endif // CHROME_BROWSER_TOUCH_TO_FILL_PASSWORD_GENERATION_ANDROID_TOUCH_TO_FILL_PASSWORD_GENERATION_BRIDGE_H_
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ TouchToFillPasswordGenerationBridgeImpl::

bool TouchToFillPasswordGenerationBridgeImpl::Show(
content::WebContents* web_contents,
base::WeakPtr<TouchToFillPasswordGenerationDelegate> delegate,
TouchToFillPasswordGenerationDelegate* delegate,
std::u16string password,
std::string account) {
if (!web_contents->GetNativeView() ||
Expand Down Expand Up @@ -70,3 +70,9 @@ void TouchToFillPasswordGenerationBridgeImpl::OnGeneratedPasswordAccepted(
delegate_->OnGeneratedPasswordAccepted(
base::android::ConvertJavaStringToUTF16(env, password));
}

void TouchToFillPasswordGenerationBridgeImpl::OnGeneratedPasswordRejected(
JNIEnv* env) {
CHECK(delegate_);
delegate_->OnGeneratedPasswordRejected();
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
#ifndef CHROME_BROWSER_TOUCH_TO_FILL_PASSWORD_GENERATION_ANDROID_TOUCH_TO_FILL_PASSWORD_GENERATION_BRIDGE_IMPL_H_
#define CHROME_BROWSER_TOUCH_TO_FILL_PASSWORD_GENERATION_ANDROID_TOUCH_TO_FILL_PASSWORD_GENERATION_BRIDGE_IMPL_H_

#include "base/android/scoped_java_ref.h"
#include "base/memory/weak_ptr.h"
#include "base/memory/raw_ptr.h"
#include "chrome/browser/touch_to_fill/password_generation/android/touch_to_fill_password_generation_bridge.h"
#include "chrome/browser/touch_to_fill/password_generation/android/touch_to_fill_password_generation_delegate.h"
#include "content/public/browser/web_contents.h"
Expand All @@ -22,7 +21,7 @@ class TouchToFillPasswordGenerationBridgeImpl
~TouchToFillPasswordGenerationBridgeImpl() override;

bool Show(content::WebContents* web_contents,
base::WeakPtr<TouchToFillPasswordGenerationDelegate> delegate,
TouchToFillPasswordGenerationDelegate* delegate,
std::u16string password,
std::string account) override;

Expand All @@ -34,10 +33,14 @@ class TouchToFillPasswordGenerationBridgeImpl
JNIEnv* env,
const base::android::JavaParamRef<jstring>& password) override;

void OnGeneratedPasswordRejected(JNIEnv* env) override;

private:
// The corresponding Java TouchToFillCreditCardViewBridge.
base::android::ScopedJavaGlobalRef<jobject> java_object_;
base::WeakPtr<TouchToFillPasswordGenerationDelegate> delegate_;
// The `delegate_` is the owner of this bridge, so its lifetime is for sure
// longer than this bridge's lifetime.
raw_ptr<TouchToFillPasswordGenerationDelegate> delegate_ = nullptr;
};

#endif // CHROME_BROWSER_TOUCH_TO_FILL_PASSWORD_GENERATION_ANDROID_TOUCH_TO_FILL_PASSWORD_GENERATION_BRIDGE_IMPL_H_

0 comments on commit c1e8db3

Please sign in to comment.