Commit a7194720 authored by Friedrich Horschig's avatar Friedrich Horschig Committed by Commit Bot

[PwdCheckAndroid] Implement native side of deletion

In order to natively delete a credential, the compromised credential on
the java-side needs to contain unformatted information about username,
origin, sign-on real, and password. Therefore, this CL:
 * adds the required key attributes
 * makes the key attributes accessible by making JNI headers available
 * makes interfaces that pass 7 attributes to construct a credential
   pass the credential directly instead (minimizes confusion)

The actual delete call is only one line in the password_check_manager
that triggers the deletion and indirectly updates to the UI once the
deletion happened.

The deletion promoted two smaller errors which were one-line-fixed:
 * the mediator never clear old credentials — it just appended new ones
 * the confirmation dialog triggered twice (once with a null handler)

Screenshot of the entire flow in the linked bug.

Bug: 1108358, 1092444
Change-Id: I1f1a3f6e73c40c316cb2f711c44236388ff05b4f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2346345
Commit-Queue: Friedrich [CET] <fhorschig@chromium.org>
Reviewed-by: default avatarIoana Pandele <ioanap@chromium.org>
Reviewed-by: default avatarAndrey Zaytsev <andzaytsev@google.com>
Cr-Commit-Position: refs/heads/master@{#796821}
parent 695bc40c
...@@ -2927,7 +2927,7 @@ static_library("browser") { ...@@ -2927,7 +2927,7 @@ static_library("browser") {
"//chrome/browser/notifications/scheduler/public", "//chrome/browser/notifications/scheduler/public",
"//chrome/browser/offline_pages/prefetch/notifications", "//chrome/browser/offline_pages/prefetch/notifications",
"//chrome/browser/optimization_guide/android:jni_headers", "//chrome/browser/optimization_guide/android:jni_headers",
"//chrome/browser/password_check/android/internal:jni_headers", "//chrome/browser/password_check/android:jni_headers",
"//chrome/browser/payments/android:jni_headers", "//chrome/browser/payments/android:jni_headers",
"//chrome/browser/policy/android:jni_headers", "//chrome/browser/policy/android:jni_headers",
"//chrome/browser/privacy:jni_headers", "//chrome/browser/privacy:jni_headers",
......
...@@ -55,7 +55,10 @@ android_library("public_java") { ...@@ -55,7 +55,10 @@ android_library("public_java") {
} }
android_library("public_ui_java") { android_library("public_ui_java") {
deps = [] deps = [
"//base:base_java",
"//url:gurl_java",
]
sources = [ sources = [
"java/src/org/chromium/chrome/browser/password_check/CompromisedCredential.java", "java/src/org/chromium/chrome/browser/password_check/CompromisedCredential.java",
"java/src/org/chromium/chrome/browser/password_check/PasswordCheckComponentUi.java", "java/src/org/chromium/chrome/browser/password_check/PasswordCheckComponentUi.java",
...@@ -82,6 +85,7 @@ junit_binary("password_check_junit_tests") { ...@@ -82,6 +85,7 @@ junit_binary("password_check_junit_tests") {
"//third_party/hamcrest:hamcrest_java", "//third_party/hamcrest:hamcrest_java",
"//third_party/junit", "//third_party/junit",
"//ui/android:ui_full_java", "//ui/android:ui_full_java",
"//url:gurl_java",
] ]
} }
...@@ -123,6 +127,15 @@ android_library("test_java") { ...@@ -123,6 +127,15 @@ android_library("test_java") {
"//third_party/mockito:mockito_java", "//third_party/mockito:mockito_java",
"//ui/android:ui_full_java", "//ui/android:ui_full_java",
"//ui/android:ui_utils_java", "//ui/android:ui_utils_java",
"//url:gurl_java",
]
}
generate_jni("jni_headers") {
visibility = [ "//chrome/browser" ]
sources = [
"internal/java/src/org/chromium/chrome/browser/password_check/PasswordCheckBridge.java",
"java/src/org/chromium/chrome/browser/password_check/CompromisedCredential.java",
] ]
} }
......
...@@ -90,11 +90,6 @@ android_library("internal_java") { ...@@ -90,11 +90,6 @@ android_library("internal_java") {
resources_package = "org.chromium.chrome.browser.password_check.internal" resources_package = "org.chromium.chrome.browser.password_check.internal"
} }
generate_jni("jni_headers") {
visibility = [ "//chrome/browser" ]
sources = [ "java/src/org/chromium/chrome/browser/password_check/PasswordCheckBridge.java" ]
}
android_resources("java_resources") { android_resources("java_resources") {
deps = [ deps = [
":java_strings_grd", ":java_strings_grd",
......
...@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.password_check; ...@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.password_check;
import org.chromium.base.annotations.CalledByNative; import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.NativeMethods; import org.chromium.base.annotations.NativeMethods;
import org.chromium.url.GURL;
/** /**
* Class handling the communication with the C++ part of the password check feature. It forwards * Class handling the communication with the C++ part of the password check feature. It forwards
...@@ -21,13 +22,9 @@ class PasswordCheckBridge { ...@@ -21,13 +22,9 @@ class PasswordCheckBridge {
interface PasswordCheckObserver { interface PasswordCheckObserver {
/** /**
* Called when a new compromised credential is found by the password check * Called when a new compromised credential is found by the password check
* @param originUrl Origin of the compromised credential. * @param credential The newly found compromised credential.
* @param username Username for the compromised credential.
* @param password Password of the compromised credential.
* @param hasScript True iff a script can be applied to the compromised credential.
*/ */
void onCompromisedCredentialFound( void onCompromisedCredentialFound(CompromisedCredential credential);
String originUrl, String username, String password, boolean hasScript);
/** /**
* Called when the compromised credentials found in a previous check are read from disk. * Called when the compromised credentials found in a previous check are read from disk.
...@@ -56,10 +53,14 @@ class PasswordCheckBridge { ...@@ -56,10 +53,14 @@ class PasswordCheckBridge {
} }
// TODO(crbug.com/1102025): Add call from native. // TODO(crbug.com/1102025): Add call from native.
void onCompromisedCredentialFound( void onCompromisedCredentialFound(String signonRealm, GURL origin, String username,
String originUrl, String username, String password, boolean hasScript) { String displayOrigin, String displayUsername, String password, boolean hasScript) {
mPasswordCheckObserver.onCompromisedCredentialFound( assert signonRealm != null;
originUrl, username, password, hasScript); assert displayOrigin != null;
assert username != null;
assert password != null;
mPasswordCheckObserver.onCompromisedCredentialFound(new CompromisedCredential(signonRealm,
origin, username, displayOrigin, displayUsername, password, false, hasScript));
} }
@CalledByNative @CalledByNative
...@@ -79,10 +80,10 @@ class PasswordCheckBridge { ...@@ -79,10 +80,10 @@ class PasswordCheckBridge {
@CalledByNative @CalledByNative
private static void insertCredential(CompromisedCredential[] credentials, int index, private static void insertCredential(CompromisedCredential[] credentials, int index,
String displayOrigin, String displayUsername, String password, boolean phished, String signonRealm, GURL origin, String username, String displayOrigin,
boolean hasScript) { String displayUsername, String password, boolean phished, boolean hasScript) {
credentials[index] = new CompromisedCredential( credentials[index] = new CompromisedCredential(signonRealm, origin, username, displayOrigin,
displayOrigin, displayUsername, password, phished, hasScript); displayUsername, password, phished, hasScript);
} }
/** /**
......
...@@ -142,7 +142,7 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs ...@@ -142,7 +142,7 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs
* @param credential A {@link CompromisedCredential} to be changed with a script. * @param credential A {@link CompromisedCredential} to be changed with a script.
*/ */
private void launchCctWithScript(CompromisedCredential credential) { private void launchCctWithScript(CompromisedCredential credential) {
Intent intent = buildIntent(credential.getOriginUrl()); Intent intent = buildIntent(credential.getOrigin().getSpec());
populateAutofillAssistantExtras(intent, credential.getUsername()); populateAutofillAssistantExtras(intent, credential.getUsername());
IntentUtils.safeStartActivity(mFragmentView.getActivity(), intent); IntentUtils.safeStartActivity(mFragmentView.getActivity(), intent);
} }
......
...@@ -45,11 +45,8 @@ class PasswordCheckImpl implements PasswordCheck, PasswordCheckObserver { ...@@ -45,11 +45,8 @@ class PasswordCheckImpl implements PasswordCheck, PasswordCheckObserver {
} }
@Override @Override
public void onCompromisedCredentialFound( public void onCompromisedCredentialFound(CompromisedCredential credential) {
String originUrl, String username, String password, boolean hasScript) { for (Observer obs : mObserverList) obs.onCompromisedCredentialFound(credential);
for (Observer obs : mObserverList) {
obs.onCompromisedCredentialFound(originUrl, username, password, hasScript);
}
} }
@Override @Override
......
...@@ -76,6 +76,7 @@ class PasswordCheckMediator ...@@ -76,6 +76,7 @@ class PasswordCheckMediator
.with(RESTART_BUTTON_ACTION, this::runCheck) .with(RESTART_BUTTON_ACTION, this::runCheck)
.build())); .build()));
} }
if (items.size() > 1) items.removeRange(1, items.size() - 1);
for (CompromisedCredential credential : credentials) { for (CompromisedCredential credential : credentials) {
items.add(new ListItem(credential.hasScript() items.add(new ListItem(credential.hasScript()
...@@ -126,22 +127,16 @@ class PasswordCheckMediator ...@@ -126,22 +127,16 @@ class PasswordCheckMediator
} }
@Override @Override
public void onCompromisedCredentialFound( public void onCompromisedCredentialFound(CompromisedCredential leakedCredential) {
String originUrl, String username, String password, boolean hasScript) { assert leakedCredential != null;
assert originUrl != null;
assert username != null;
assert password != null;
ListModel<ListItem> items = mModel.get(ITEMS); ListModel<ListItem> items = mModel.get(ITEMS);
assert items.size() >= 1 : "Needs to initialize list with header before adding items!"; assert items.size() >= 1 : "Needs to initialize list with header before adding items!";
items.add(new ListItem(leakedCredential.hasScript()
CompromisedCredential credential =
new CompromisedCredential(originUrl, username, password, false, hasScript);
items.add(new ListItem(credential.hasScript()
? PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL_WITH_SCRIPT ? PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL_WITH_SCRIPT
: PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL, : PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL,
new PropertyModel new PropertyModel
.Builder(PasswordCheckProperties.CompromisedCredentialProperties.ALL_KEYS) .Builder(PasswordCheckProperties.CompromisedCredentialProperties.ALL_KEYS)
.with(COMPROMISED_CREDENTIAL, credential) .with(COMPROMISED_CREDENTIAL, leakedCredential)
.with(CREDENTIAL_HANDLER, this) .with(CREDENTIAL_HANDLER, this)
.build())); .build()));
} }
...@@ -153,7 +148,7 @@ class PasswordCheckMediator ...@@ -153,7 +148,7 @@ class PasswordCheckMediator
@Override @Override
public void onRemove(CompromisedCredential credential) { public void onRemove(CompromisedCredential credential) {
mModel.set(DELETION_ORIGIN, credential.getOriginUrl()); mModel.set(DELETION_ORIGIN, credential.getDisplayOrigin());
mModel.set( mModel.set(
DELETION_CONFIRMATION_HANDLER, new PasswordCheckDeletionDialogFragment.Handler() { DELETION_CONFIRMATION_HANDLER, new PasswordCheckDeletionDialogFragment.Handler() {
@Override @Override
...@@ -173,7 +168,7 @@ class PasswordCheckMediator ...@@ -173,7 +168,7 @@ class PasswordCheckMediator
@Override @Override
public void onChangePasswordButtonClick(CompromisedCredential credential) { public void onChangePasswordButtonClick(CompromisedCredential credential) {
mLaunchCctWithChangePasswordUrl.accept(credential.getOriginUrl()); mLaunchCctWithChangePasswordUrl.accept(credential.getSignonRealm());
} }
@Override @Override
......
...@@ -15,7 +15,6 @@ import static org.chromium.chrome.browser.password_check.PasswordCheckProperties ...@@ -15,7 +15,6 @@ import static org.chromium.chrome.browser.password_check.PasswordCheckProperties
import static org.chromium.chrome.browser.password_check.PasswordCheckProperties.HeaderProperties.RESTART_BUTTON_ACTION; import static org.chromium.chrome.browser.password_check.PasswordCheckProperties.HeaderProperties.RESTART_BUTTON_ACTION;
import static org.chromium.chrome.browser.password_check.PasswordCheckProperties.HeaderProperties.UNKNOWN_PROGRESS; import static org.chromium.chrome.browser.password_check.PasswordCheckProperties.HeaderProperties.UNKNOWN_PROGRESS;
import static org.chromium.chrome.browser.password_check.PasswordCheckProperties.ITEMS; import static org.chromium.chrome.browser.password_check.PasswordCheckProperties.ITEMS;
import static org.chromium.components.embedder_support.util.UrlUtilities.stripScheme;
import android.content.Context; import android.content.Context;
import android.content.res.Resources; import android.content.res.Resources;
...@@ -64,6 +63,7 @@ class PasswordCheckViewBinder { ...@@ -64,6 +63,7 @@ class PasswordCheckViewBinder {
PasswordCheckViewBinder::connectPropertyModel), PasswordCheckViewBinder::connectPropertyModel),
PasswordCheckViewBinder::createViewHolder)); PasswordCheckViewBinder::createViewHolder));
} else if (propertyKey == DELETION_CONFIRMATION_HANDLER) { } else if (propertyKey == DELETION_CONFIRMATION_HANDLER) {
if (model.get(DELETION_CONFIRMATION_HANDLER) == null) return; // Initial or onDismiss.
view.showDialogFragment(new PasswordCheckDeletionDialogFragment( view.showDialogFragment(new PasswordCheckDeletionDialogFragment(
model.get(DELETION_CONFIRMATION_HANDLER), model.get(DELETION_ORIGIN))); model.get(DELETION_CONFIRMATION_HANDLER), model.get(DELETION_ORIGIN)));
} else if (propertyKey == DELETION_ORIGIN) { } else if (propertyKey == DELETION_ORIGIN) {
...@@ -122,14 +122,11 @@ class PasswordCheckViewBinder { ...@@ -122,14 +122,11 @@ class PasswordCheckViewBinder {
PropertyModel model, View view, PropertyKey propertyKey) { PropertyModel model, View view, PropertyKey propertyKey) {
CompromisedCredential credential = model.get(COMPROMISED_CREDENTIAL); CompromisedCredential credential = model.get(COMPROMISED_CREDENTIAL);
if (propertyKey == COMPROMISED_CREDENTIAL) { if (propertyKey == COMPROMISED_CREDENTIAL) {
TextView pslOriginText = view.findViewById(R.id.credential_origin); TextView originText = view.findViewById(R.id.credential_origin);
String formattedOrigin = stripScheme(credential.getOriginUrl()); originText.setText(credential.getDisplayOrigin());
formattedOrigin =
formattedOrigin.replaceFirst("/$", ""); // Strip possibly trailing slash.
pslOriginText.setText(formattedOrigin);
TextView username = view.findViewById(R.id.compromised_username); TextView username = view.findViewById(R.id.compromised_username);
username.setText(credential.getUsername()); username.setText(credential.getDisplayUsername());
TextView reason = view.findViewById(R.id.compromised_reason); TextView reason = view.findViewById(R.id.compromised_reason);
reason.setText(credential.isPhished() reason.setText(credential.isPhished()
......
...@@ -4,6 +4,9 @@ ...@@ -4,6 +4,9 @@
package org.chromium.chrome.browser.password_check; package org.chromium.chrome.browser.password_check;
import org.chromium.base.annotations.CalledByNative;
import org.chromium.url.GURL;
import java.util.Objects; import java.util.Objects;
/** /**
...@@ -11,35 +14,63 @@ import java.util.Objects; ...@@ -11,35 +14,63 @@ import java.util.Objects;
* settings screen. * settings screen.
*/ */
public class CompromisedCredential { public class CompromisedCredential {
private final String mSignonRealm;
private final GURL mOrigin;
private final String mUsername; private final String mUsername;
private final String mDisplayOrigin;
private final String mDisplayUsername;
private final String mPassword; private final String mPassword;
private final String mOriginUrl;
private final boolean mPhished; private final boolean mPhished;
private final boolean mHasScript; private final boolean mHasScript;
/** /**
* @param username Username shown to the user. * @param signonRealm The URL leading to the sign-on page.
* @param originUrl Origin URL shown to the user in case this credential is a PSL match. * @param origin The origin used to identify this credential (may be empty).
* @param username The name used to identify this credential (may be empty).
* @param displayOrigin The origin displayed to the user. Not necessarily a valid URL (e.g.
* missing scheme).
* @param displayUsername The username displayed to the user (substituted if empty).
* @param password The compromised password.
* @param phished True iff the credential was entered on an unsafe site.
* @param hasScript True iff the credential can be automatically fixed.
*/ */
public CompromisedCredential(String originUrl, String username, String password, public CompromisedCredential(String signonRealm, GURL origin, String username,
boolean phished, boolean hasScript) { String displayOrigin, String displayUsername, String password, boolean phished,
assert originUrl != null : "Credential origin is null! Pass an empty one instead."; boolean hasScript) {
mOriginUrl = originUrl; assert origin != null : "Credential origin is null! Pass an empty one instead.";
assert signonRealm != null;
mSignonRealm = signonRealm;
mOrigin = origin;
mUsername = username; mUsername = username;
mDisplayOrigin = displayOrigin;
mDisplayUsername = displayUsername;
mPassword = password; mPassword = password;
mPhished = phished; mPhished = phished;
mHasScript = hasScript; mHasScript = hasScript;
} }
public String getOriginUrl() { @CalledByNative
return mOriginUrl; public String getSignonRealm() {
return mSignonRealm;
} }
@CalledByNative
public String getUsername() { public String getUsername() {
return mUsername; return mUsername;
} }
@CalledByNative
public GURL getOrigin() {
return mOrigin;
}
@CalledByNative
public String getPassword() { public String getPassword() {
return mPassword; return mPassword;
} }
public String getDisplayUsername() {
return mDisplayUsername;
}
public String getDisplayOrigin() {
return mDisplayOrigin;
}
public boolean isPhished() { public boolean isPhished() {
return mPhished; return mPhished;
} }
...@@ -52,21 +83,25 @@ public class CompromisedCredential { ...@@ -52,21 +83,25 @@ public class CompromisedCredential {
if (this == o) return true; if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false; if (o == null || getClass() != o.getClass()) return false;
CompromisedCredential that = (CompromisedCredential) o; CompromisedCredential that = (CompromisedCredential) o;
return mPhished == that.mPhished && mHasScript == that.mHasScript return mSignonRealm.equals(that.mSignonRealm) && mOrigin.equals(that.mOrigin)
&& mUsername.equals(that.mUsername) && mPassword.equals(that.mPassword) && mUsername.equals(that.mUsername) && mDisplayOrigin.equals(that.mDisplayOrigin)
&& mOriginUrl.equals(that.mOriginUrl); && mDisplayUsername.equals(that.mDisplayUsername)
&& mPassword.equals(that.mPassword) && mPhished == that.mPhished
&& mHasScript == that.mHasScript;
} }
@Override @Override
public String toString() { public String toString() {
return "CompromisedCredential{" return "CompromisedCredential{"
+ "username='" + mUsername + '\'' + ", password='" + mPassword + '\'' + "signonRealm='" + mSignonRealm + ", origin='" + mOrigin + '\'' + '\''
+ ", originUrl='" + mOriginUrl + '\'' + ", phished=" + mPhished + ", username='" + mUsername + '\'' + ", displayOrigin='" + mDisplayOrigin + '\''
+ ", hasScript=" + mHasScript + '}'; + ", displayUsername='" + mDisplayUsername + '\'' + ", password='" + mPassword
+ '\'' + ", phished=" + mPhished + ", hasScript=" + mHasScript + '}';
} }
@Override @Override
public int hashCode() { public int hashCode() {
return Objects.hash(mUsername, mPassword, mOriginUrl, mPhished, mHasScript); return Objects.hash(mSignonRealm, mOrigin, mUsername, mDisplayOrigin, mDisplayUsername,
mPassword, mPhished, mHasScript);
} }
} }
...@@ -33,14 +33,10 @@ public interface PasswordCheck extends PasswordCheckComponentUi.Delegate { ...@@ -33,14 +33,10 @@ public interface PasswordCheck extends PasswordCheckComponentUi.Delegate {
void onPasswordCheckStatusChanged(@PasswordCheckUIStatus int status); void onPasswordCheckStatusChanged(@PasswordCheckUIStatus int status);
/** /**
* Invoked whenever a running check finds another compromised credential. * Invoked whenever a running check finds another leaked credential.
* @param originUrl The origin of the newly found compromised credential. * @param leakedCredential The newly found leaked credential.
* @param username The username of the newly found compromised credential.
* @param password The password of the newly found compromised credential.
* @param hasScript True iff a script can be applied to the newly found credential.
*/ */
void onCompromisedCredentialFound( void onCompromisedCredentialFound(CompromisedCredential leakedCredential);
String originUrl, String username, String password, boolean hasScript);
} }
/** /**
......
...@@ -83,6 +83,7 @@ import org.chromium.content_public.browser.test.util.TouchCommon; ...@@ -83,6 +83,7 @@ import org.chromium.content_public.browser.test.util.TouchCommon;
import org.chromium.ui.modelutil.MVCListAdapter; import org.chromium.ui.modelutil.MVCListAdapter;
import org.chromium.ui.modelutil.PropertyModel; import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.ui.widget.ButtonCompat; import org.chromium.ui.widget.ButtonCompat;
import org.chromium.url.GURL;
/** /**
* View tests for the Password Check component ensure model changes are reflected in the check UI. * View tests for the Password Check component ensure model changes are reflected in the check UI.
...@@ -90,14 +91,18 @@ import org.chromium.ui.widget.ButtonCompat; ...@@ -90,14 +91,18 @@ import org.chromium.ui.widget.ButtonCompat;
@RunWith(ChromeJUnit4ClassRunner.class) @RunWith(ChromeJUnit4ClassRunner.class)
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE}) @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
public class PasswordCheckViewTest { public class PasswordCheckViewTest {
private static final CompromisedCredential ANA = private static final CompromisedCredential ANA = new CompromisedCredential(
new CompromisedCredential("some-url.com", "Ana", "password", false, false); "https://some-url.com/signin", new GURL("https://some-url.com/"), "Ana", "some-url.com",
"Ana", "password", false, false);
private static final CompromisedCredential PHISHED = private static final CompromisedCredential PHISHED =
new CompromisedCredential("example.com", "Baub", "DoSomething", true, false); new CompromisedCredential("http://example.com/signin", new GURL("http://example.com/"),
private static final CompromisedCredential LEAKED = "", "http://example.com", "(No username)", "DoSomething", true, false);
new CompromisedCredential("some-other-url.com", "AZiegler", "N0M3rcy", false, false); private static final CompromisedCredential LEAKED = new CompromisedCredential(
"https://some-other-url.com/signin", new GURL("https://some-other-url.com/"),
"AZiegler", "some-other-url.com", "AZiegler", "N0M3rcy", false, false);
private static final CompromisedCredential SCRIPTED = private static final CompromisedCredential SCRIPTED =
new CompromisedCredential("script.com", "Charlie", "secret", false, true); new CompromisedCredential("https://script.com/signin", new GURL("https://script.com/"),
"Charlie", "script.com", "Charlie", "secret", false, true);
private static final int LEAKS_COUNT = 2; private static final int LEAKS_COUNT = 2;
...@@ -393,14 +398,14 @@ public class PasswordCheckViewTest { ...@@ -393,14 +398,14 @@ public class PasswordCheckViewTest {
waitForListViewToHaveLength(2); waitForListViewToHaveLength(2);
// The phished credential is rendered first: // The phished credential is rendered first:
assertThat(getCredentialOriginAt(0).getText(), is(PHISHED.getOriginUrl())); assertThat(getCredentialOriginAt(0).getText(), is(PHISHED.getDisplayOrigin()));
assertThat(getCredentialUserAt(0).getText(), is(PHISHED.getUsername())); assertThat(getCredentialUserAt(0).getText(), is(PHISHED.getDisplayUsername()));
assertThat(getCredentialReasonAt(0).getText(), assertThat(getCredentialReasonAt(0).getText(),
is(getString(R.string.password_check_credential_row_reason_phished))); is(getString(R.string.password_check_credential_row_reason_phished)));
// The leaked credential is rendered second: // The leaked credential is rendered second:
assertThat(getCredentialOriginAt(1).getText(), is(LEAKED.getOriginUrl())); assertThat(getCredentialOriginAt(1).getText(), is(LEAKED.getDisplayOrigin()));
assertThat(getCredentialUserAt(1).getText(), is(LEAKED.getUsername())); assertThat(getCredentialUserAt(1).getText(), is(LEAKED.getDisplayUsername()));
assertThat(getCredentialReasonAt(1).getText(), assertThat(getCredentialReasonAt(1).getText(),
is(getString(R.string.password_check_credential_row_reason_leaked))); is(getString(R.string.password_check_credential_row_reason_leaked)));
} }
...@@ -412,8 +417,8 @@ public class PasswordCheckViewTest { ...@@ -412,8 +417,8 @@ public class PasswordCheckViewTest {
pollUiThread(() -> Criteria.checkThat(getPasswordCheckViewList().getChildCount(), is(1))); pollUiThread(() -> Criteria.checkThat(getPasswordCheckViewList().getChildCount(), is(1)));
// Origin and username. // Origin and username.
assertThat(getCredentialOriginAt(0).getText(), is(SCRIPTED.getOriginUrl())); assertThat(getCredentialOriginAt(0).getText(), is(SCRIPTED.getDisplayOrigin()));
assertThat(getCredentialUserAt(0).getText(), is(SCRIPTED.getUsername())); assertThat(getCredentialUserAt(0).getText(), is(SCRIPTED.getDisplayUsername()));
// Reason to show credential. // Reason to show credential.
assertThat(getCredentialReasonAt(0).getText(), assertThat(getCredentialReasonAt(0).getText(),
...@@ -509,7 +514,7 @@ public class PasswordCheckViewTest { ...@@ -509,7 +514,7 @@ public class PasswordCheckViewTest {
public void testConfirmingDeletionDialogTriggersHandler() { public void testConfirmingDeletionDialogTriggersHandler() {
PasswordCheckDeletionDialogFragment.Handler mockHandler = PasswordCheckDeletionDialogFragment.Handler mockHandler =
mock(PasswordCheckDeletionDialogFragment.Handler.class); mock(PasswordCheckDeletionDialogFragment.Handler.class);
mModel.set(DELETION_ORIGIN, ANA.getOriginUrl()); mModel.set(DELETION_ORIGIN, ANA.getDisplayOrigin());
runOnUiThreadBlocking(() -> mModel.set(DELETION_CONFIRMATION_HANDLER, mockHandler)); runOnUiThreadBlocking(() -> mModel.set(DELETION_CONFIRMATION_HANDLER, mockHandler));
onView(withText(R.string.password_check_delete_credential_dialog_confirm)) onView(withText(R.string.password_check_delete_credential_dialog_confirm))
......
...@@ -50,6 +50,7 @@ import org.chromium.chrome.test.util.browser.Features.EnableFeatures; ...@@ -50,6 +50,7 @@ import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
import org.chromium.ui.modelutil.ListModel; import org.chromium.ui.modelutil.ListModel;
import org.chromium.ui.modelutil.MVCListAdapter; import org.chromium.ui.modelutil.MVCListAdapter;
import org.chromium.ui.modelutil.PropertyModel; import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.url.GURL;
/** /**
* Controller tests verify that the PasswordCheck controller modifies the model if the API is used * Controller tests verify that the PasswordCheck controller modifies the model if the API is used
...@@ -59,9 +60,11 @@ import org.chromium.ui.modelutil.PropertyModel; ...@@ -59,9 +60,11 @@ import org.chromium.ui.modelutil.PropertyModel;
@EnableFeatures(ChromeFeatureList.PASSWORD_CHECK) @EnableFeatures(ChromeFeatureList.PASSWORD_CHECK)
public class PasswordCheckControllerTest { public class PasswordCheckControllerTest {
private static final CompromisedCredential ANA = private static final CompromisedCredential ANA =
new CompromisedCredential("https://m.a.xyz/", "Ana", "password", false, false); new CompromisedCredential("https://m.a.xyz/signin", mock(GURL.class), "Ana", "m.a.xyz",
"Ana", "password", false, false);
private static final CompromisedCredential BOB = private static final CompromisedCredential BOB =
new CompromisedCredential("https://www.b.ch/", "Baub", "DoneSth", false, true); new CompromisedCredential("http://www.b.ch/signin", mock(GURL.class), "",
"http://www.b.ch", "(No username)", "DoneSth", false, true);
@Rule @Rule
public TestRule mFeaturesProcessorRule = new Features.JUnitProcessor(); public TestRule mFeaturesProcessorRule = new Features.JUnitProcessor();
...@@ -167,14 +170,33 @@ public class PasswordCheckControllerTest { ...@@ -167,14 +170,33 @@ public class PasswordCheckControllerTest {
mMediator.onCompromisedCredentialsFetchCompleted(); mMediator.onCompromisedCredentialsFetchCompleted();
assertThat(mModel.get(ITEMS).size(), is(2)); // Header + existing credentials. assertThat(mModel.get(ITEMS).size(), is(2)); // Header + existing credentials.
mMediator.onCompromisedCredentialFound( mMediator.onCompromisedCredentialFound(BOB);
BOB.getOriginUrl(), BOB.getUsername(), BOB.getPassword(), BOB.hasScript());
assertThat(mModel.get(ITEMS).get(2).type, is(ItemType.COMPROMISED_CREDENTIAL_WITH_SCRIPT)); assertThat(mModel.get(ITEMS).get(2).type, is(ItemType.COMPROMISED_CREDENTIAL_WITH_SCRIPT));
assertThat(mModel.get(ITEMS).get(2).model.get(COMPROMISED_CREDENTIAL), equalTo(BOB)); assertThat(mModel.get(ITEMS).get(2).model.get(COMPROMISED_CREDENTIAL), equalTo(BOB));
assertThat(mModel.get(ITEMS).get(2).model.get(CREDENTIAL_HANDLER), is(mMediator)); assertThat(mModel.get(ITEMS).get(2).model.get(CREDENTIAL_HANDLER), is(mMediator));
} }
@Test
public void testReplacesEntriesForUpdateOfEntireList() {
mMediator.onPasswordCheckStatusChanged(IDLE);
// First call adds only ANA.
when(mPasswordCheck.getCompromisedCredentials())
.thenReturn(new CompromisedCredential[] {ANA});
mMediator.onCompromisedCredentialsFetchCompleted();
assertThat(mModel.get(ITEMS).size(), is(2)); // Header + existing credentials.
// Second call adds BOB and removes ANA.
when(mPasswordCheck.getCompromisedCredentials())
.thenReturn(new CompromisedCredential[] {BOB});
mMediator.onCompromisedCredentialsFetchCompleted();
assertThat(mModel.get(ITEMS).size(), is(2));
assertThat(mModel.get(ITEMS).get(1).type, is(ItemType.COMPROMISED_CREDENTIAL_WITH_SCRIPT));
assertThat(mModel.get(ITEMS).get(1).model.get(COMPROMISED_CREDENTIAL), equalTo(BOB));
assertThat(mModel.get(ITEMS).get(1).model.get(CREDENTIAL_HANDLER), is(mMediator));
}
@Test @Test
public void testRemovingElementTriggersDelegate() { public void testRemovingElementTriggersDelegate() {
// Removing sets a valid handler: // Removing sets a valid handler:
...@@ -191,7 +213,7 @@ public class PasswordCheckControllerTest { ...@@ -191,7 +213,7 @@ public class PasswordCheckControllerTest {
@Test @Test
public void testOnChangePasswordButtonClick() { public void testOnChangePasswordButtonClick() {
mMediator.onChangePasswordButtonClick(ANA); mMediator.onChangePasswordButtonClick(ANA);
verify(mLaunchCctWithChangePasswordUrlConsumer).accept(eq(ANA.getOriginUrl())); verify(mLaunchCctWithChangePasswordUrlConsumer).accept(eq(ANA.getSignonRealm()));
} }
@Test @Test
......
...@@ -7,10 +7,30 @@ ...@@ -7,10 +7,30 @@
#include <jni.h> #include <jni.h>
#include "base/android/jni_string.h" #include "base/android/jni_string.h"
#include "chrome/browser/password_check/android/internal/jni_headers/PasswordCheckBridge_jni.h" #include "chrome/browser/password_check/android/jni_headers/CompromisedCredential_jni.h"
#include "chrome/browser/password_check/android/jni_headers/PasswordCheckBridge_jni.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "components/password_manager/core/browser/ui/compromised_credentials_manager.h" #include "components/password_manager/core/browser/ui/compromised_credentials_manager.h"
#include "url/android/gurl_android.h" #include "url/android/gurl_android.h"
namespace {
password_manager::CredentialView ConvertJavaObjectToCredentialView(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& credential) {
return password_manager::CredentialView(
ConvertJavaStringToUTF8(
env, Java_CompromisedCredential_getSignonRealm(env, credential)),
*url::GURLAndroid::ToNativeGURL(
env, Java_CompromisedCredential_getOrigin(env, credential)),
ConvertJavaStringToUTF16(
env, Java_CompromisedCredential_getUsername(env, credential)),
ConvertJavaStringToUTF16(
env, Java_CompromisedCredential_getPassword(env, credential)));
}
} // namespace
static jlong JNI_PasswordCheckBridge_Create( static jlong JNI_PasswordCheckBridge_Create(
JNIEnv* env, JNIEnv* env,
const base::android::JavaParamRef<jobject>& java_bridge) { const base::android::JavaParamRef<jobject>& java_bridge) {
...@@ -49,6 +69,9 @@ void PasswordCheckBridge::GetCompromisedCredentials( ...@@ -49,6 +69,9 @@ void PasswordCheckBridge::GetCompromisedCredentials(
const auto& credential = credentials[i]; const auto& credential = credentials[i];
Java_PasswordCheckBridge_insertCredential( Java_PasswordCheckBridge_insertCredential(
env, java_credentials, i, env, java_credentials, i,
base::android::ConvertUTF8ToJavaString(env, credential.signon_realm),
url::GURLAndroid::FromNativeGURL(env, credential.url),
base::android::ConvertUTF16ToJavaString(env, credential.username),
base::android::ConvertUTF16ToJavaString(env, credential.display_origin), base::android::ConvertUTF16ToJavaString(env, credential.display_origin),
base::android::ConvertUTF16ToJavaString(env, base::android::ConvertUTF16ToJavaString(env,
credential.display_username), credential.display_username),
...@@ -62,7 +85,8 @@ void PasswordCheckBridge::GetCompromisedCredentials( ...@@ -62,7 +85,8 @@ void PasswordCheckBridge::GetCompromisedCredentials(
void PasswordCheckBridge::RemoveCredential( void PasswordCheckBridge::RemoveCredential(
JNIEnv* env, JNIEnv* env,
const base::android::JavaParamRef<jobject>& credential) { const base::android::JavaParamRef<jobject>& credential) {
// TODO(crbug.com/1108358): implement this. check_manager_.RemoveCredential(
ConvertJavaObjectToCredentialView(env, credential));
} }
void PasswordCheckBridge::Destroy(JNIEnv* env) { void PasswordCheckBridge::Destroy(JNIEnv* env) {
......
...@@ -106,6 +106,11 @@ PasswordCheckManager::GetCompromisedCredentials() const { ...@@ -106,6 +106,11 @@ PasswordCheckManager::GetCompromisedCredentials() const {
return ui_credentials; return ui_credentials;
} }
void PasswordCheckManager::RemoveCredential(
const password_manager::CredentialView& credential) {
compromised_credentials_manager_.RemoveCompromisedCredential(credential);
}
void PasswordCheckManager::OnSavedPasswordsChanged( void PasswordCheckManager::OnSavedPasswordsChanged(
password_manager::SavedPasswordsPresenter::SavedPasswordsView passwords) { password_manager::SavedPasswordsPresenter::SavedPasswordsView passwords) {
if (!is_initialized_) { if (!is_initialized_) {
......
...@@ -70,6 +70,10 @@ class PasswordCheckManager ...@@ -70,6 +70,10 @@ class PasswordCheckManager
// Called by java to retrieve the compromised credentials. // Called by java to retrieve the compromised credentials.
std::vector<CompromisedCredentialForUI> GetCompromisedCredentials() const; std::vector<CompromisedCredentialForUI> GetCompromisedCredentials() const;
// Called by java to remove the given compromised |credential| and trigger a
// UI update on completion.
void RemoveCredential(const password_manager::CredentialView& credential);
// Not copyable or movable // Not copyable or movable
PasswordCheckManager(const PasswordCheckManager&) = delete; PasswordCheckManager(const PasswordCheckManager&) = delete;
PasswordCheckManager& operator=(const PasswordCheckManager&) = delete; PasswordCheckManager& operator=(const PasswordCheckManager&) = delete;
......
...@@ -19,6 +19,7 @@ import org.chromium.base.BuildConfig; ...@@ -19,6 +19,7 @@ import org.chromium.base.BuildConfig;
import org.chromium.base.Callback; import org.chromium.base.Callback;
import org.chromium.base.ContextUtils; import org.chromium.base.ContextUtils;
import org.chromium.chrome.browser.flags.ChromeFeatureList; import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.password_check.CompromisedCredential;
import org.chromium.chrome.browser.password_check.PasswordCheck; import org.chromium.chrome.browser.password_check.PasswordCheck;
import org.chromium.chrome.browser.password_check.PasswordCheckFactory; import org.chromium.chrome.browser.password_check.PasswordCheckFactory;
import org.chromium.chrome.browser.password_check.PasswordCheckUIStatus; import org.chromium.chrome.browser.password_check.PasswordCheckUIStatus;
...@@ -310,8 +311,7 @@ class SafetyCheckMediator implements PasswordCheck.Observer, SafetyCheckCommonOb ...@@ -310,8 +311,7 @@ class SafetyCheckMediator implements PasswordCheck.Observer, SafetyCheckCommonOb
} }
@Override @Override
public void onCompromisedCredentialFound( public void onCompromisedCredentialFound(CompromisedCredential leakedCredential) {}
String originUrl, String username, String password, boolean hasScript) {}
/** Cancels any pending callbacks and registered observers. */ /** Cancels any pending callbacks and registered observers. */
public void destroy() { public void destroy() {
......
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