Commit 0bae0028 authored by Maxim Kolosovskiy's avatar Maxim Kolosovskiy Committed by Commit Bot

[Passwords] Introduce CompromisedCredential.has_script

has_script is going to be divided into |has_script| and
|has_auto_change_button|. It will be done in several steps for easier
reviewing:
1. The existing |has_script| will be renamed to |has_auto_change_button|
2. A "new" |has_script| will be introduced, where |has_script|==
|has_auto_change_button| (this CL).
3. Implement special logic for the new |has_script|.

Bug: 1132230
Change-Id: I71dd73842b406fd9c82f4f11e57b7631febfcb86
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2431925Reviewed-by: default avatarIoana Pandele <ioanap@chromium.org>
Commit-Queue: Maxim Kolosovskiy  <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811219}
parent 3130b7e3
...@@ -64,14 +64,15 @@ class PasswordCheckBridge { ...@@ -64,14 +64,15 @@ class PasswordCheckBridge {
// TODO(crbug.com/1102025): Add call from native. // TODO(crbug.com/1102025): Add call from native.
void onCompromisedCredentialFound(String signonRealm, GURL origin, String username, void onCompromisedCredentialFound(String signonRealm, GURL origin, String username,
String displayOrigin, String displayUsername, String password, String passwordChangeUrl, String displayOrigin, String displayUsername, String password, String passwordChangeUrl,
String associatedApp, long creationTime, boolean hasAutoChangeButton) { String associatedApp, long creationTime, boolean hasScript,
boolean hasAutoChangeButton) {
assert signonRealm != null; assert signonRealm != null;
assert displayOrigin != null; assert displayOrigin != null;
assert username != null; assert username != null;
assert password != null; assert password != null;
mPasswordCheckObserver.onCompromisedCredentialFound(new CompromisedCredential(signonRealm, mPasswordCheckObserver.onCompromisedCredentialFound(new CompromisedCredential(signonRealm,
origin, username, displayOrigin, displayUsername, password, passwordChangeUrl, origin, username, displayOrigin, displayUsername, password, passwordChangeUrl,
associatedApp, creationTime, true, false, hasAutoChangeButton)); associatedApp, creationTime, true, false, hasScript, hasAutoChangeButton));
} }
@CalledByNative @CalledByNative
...@@ -98,10 +99,11 @@ class PasswordCheckBridge { ...@@ -98,10 +99,11 @@ class PasswordCheckBridge {
private static void insertCredential(CompromisedCredential[] credentials, int index, private static void insertCredential(CompromisedCredential[] credentials, int index,
String signonRealm, GURL origin, String username, String displayOrigin, String signonRealm, GURL origin, String username, String displayOrigin,
String displayUsername, String password, String passwordChangeUrl, String associatedApp, String displayUsername, String password, String passwordChangeUrl, String associatedApp,
long creationTime, boolean leaked, boolean phished, boolean hasAutoChangeButton) { long creationTime, boolean leaked, boolean phished, boolean hasScript,
boolean hasAutoChangeButton) {
credentials[index] = new CompromisedCredential(signonRealm, origin, username, displayOrigin, credentials[index] = new CompromisedCredential(signonRealm, origin, username, displayOrigin,
displayUsername, password, passwordChangeUrl, associatedApp, creationTime, leaked, displayUsername, password, passwordChangeUrl, associatedApp, creationTime, leaked,
phished, hasAutoChangeButton); phished, hasScript, hasAutoChangeButton);
} }
/** /**
......
...@@ -31,15 +31,16 @@ public class CompromisedCredential implements Parcelable { ...@@ -31,15 +31,16 @@ public class CompromisedCredential implements Parcelable {
final String passwordChangeUrl = in.readString(); final String passwordChangeUrl = in.readString();
final String associatedApp = in.readString(); final String associatedApp = in.readString();
final long creationTime = in.readLong(); final long creationTime = in.readLong();
boolean[] boolArguments = new boolean[3]; boolean[] boolArguments = new boolean[4];
in.readBooleanArray(boolArguments); in.readBooleanArray(boolArguments);
final boolean leaked = boolArguments[0]; final boolean leaked = boolArguments[0];
final boolean phished = boolArguments[1]; final boolean phished = boolArguments[1];
final boolean hasAutoChangeButton = boolArguments[2]; final boolean hasScript = boolArguments[2];
final boolean hasAutoChangeButton = boolArguments[3];
return new CompromisedCredential(signonRealm, origin, username, displayOrigin, return new CompromisedCredential(signonRealm, origin, username, displayOrigin,
displayUsername, password, passwordChangeUrl, associatedApp, displayUsername, password, passwordChangeUrl, associatedApp,
creationTime, leaked, phished, hasAutoChangeButton); creationTime, leaked, phished, hasScript, hasAutoChangeButton);
} }
@Override @Override
...@@ -59,6 +60,7 @@ public class CompromisedCredential implements Parcelable { ...@@ -59,6 +60,7 @@ public class CompromisedCredential implements Parcelable {
private final long mCreationTime; private final long mCreationTime;
private final boolean mLeaked; private final boolean mLeaked;
private final boolean mPhished; private final boolean mPhished;
private final boolean mHasScript;
private final boolean mHasAutoChangeButton; private final boolean mHasAutoChangeButton;
/** /**
...@@ -75,12 +77,14 @@ public class CompromisedCredential implements Parcelable { ...@@ -75,12 +77,14 @@ public class CompromisedCredential implements Parcelable {
* time at which the compromised credential was first found to be compromised during * time at which the compromised credential was first found to be compromised during
* a check. * a check.
* @param phished True iff the credential was entered on an unsafe site. * @param phished True iff the credential was entered on an unsafe site.
* @param hasAutoChangeButton True iff the credential can be automatically fixed. * @param hasScript True iff there is a script to automatically fix the credential.
* @param hasAutoChangeButton True iff the button to automatically change the credential should
* be shown.
*/ */
public CompromisedCredential(String signonRealm, GURL origin, String username, public CompromisedCredential(String signonRealm, GURL origin, String username,
String displayOrigin, String displayUsername, String password, String passwordChangeUrl, String displayOrigin, String displayUsername, String password, String passwordChangeUrl,
String associatedApp, long creationTime, boolean leaked, boolean phished, String associatedApp, long creationTime, boolean leaked, boolean phished,
boolean hasAutoChangeButton) { boolean hasScript, boolean hasAutoChangeButton) {
assert origin != null : "Credential origin is null! Pass an empty one instead."; assert origin != null : "Credential origin is null! Pass an empty one instead.";
assert signonRealm != null; assert signonRealm != null;
assert passwordChangeUrl != null : "Change URL may be empty but not null!"; assert passwordChangeUrl != null : "Change URL may be empty but not null!";
...@@ -89,6 +93,8 @@ public class CompromisedCredential implements Parcelable { ...@@ -89,6 +93,8 @@ public class CompromisedCredential implements Parcelable {
|| !associatedApp.isEmpty() || !associatedApp.isEmpty()
: "Change URL and app name may not be empty at the same time!"; : "Change URL and app name may not be empty at the same time!";
assert leaked || phished : "A compromised credential must be leaked or phished!"; assert leaked || phished : "A compromised credential must be leaked or phished!";
assert hasScript
|| !hasAutoChangeButton : "Auto change button cannot be shown without a script!";
mSignonRealm = signonRealm; mSignonRealm = signonRealm;
mOrigin = origin; mOrigin = origin;
mUsername = username; mUsername = username;
...@@ -100,6 +106,7 @@ public class CompromisedCredential implements Parcelable { ...@@ -100,6 +106,7 @@ public class CompromisedCredential implements Parcelable {
mCreationTime = creationTime; mCreationTime = creationTime;
mLeaked = leaked; mLeaked = leaked;
mPhished = phished; mPhished = phished;
mHasScript = hasScript;
mHasAutoChangeButton = hasAutoChangeButton; mHasAutoChangeButton = hasAutoChangeButton;
} }
...@@ -140,6 +147,9 @@ public class CompromisedCredential implements Parcelable { ...@@ -140,6 +147,9 @@ public class CompromisedCredential implements Parcelable {
public boolean isPhished() { public boolean isPhished() {
return mPhished; return mPhished;
} }
public boolean hasScript() {
return mHasScript;
}
public boolean hasAutoChangeButton() { public boolean hasAutoChangeButton() {
return mHasAutoChangeButton; return mHasAutoChangeButton;
} }
...@@ -156,6 +166,7 @@ public class CompromisedCredential implements Parcelable { ...@@ -156,6 +166,7 @@ public class CompromisedCredential implements Parcelable {
&& mPasswordChangeUrl.equals(that.mPasswordChangeUrl) && mPasswordChangeUrl.equals(that.mPasswordChangeUrl)
&& mAssociatedApp.equals(that.mAssociatedApp) && mCreationTime == that.mCreationTime && mAssociatedApp.equals(that.mAssociatedApp) && mCreationTime == that.mCreationTime
&& mLeaked == that.mLeaked && mPhished == that.mPhished && mLeaked == that.mLeaked && mPhished == that.mPhished
&& mHasScript == that.mHasScript
&& mHasAutoChangeButton == that.mHasAutoChangeButton; && mHasAutoChangeButton == that.mHasAutoChangeButton;
} }
...@@ -167,14 +178,15 @@ public class CompromisedCredential implements Parcelable { ...@@ -167,14 +178,15 @@ public class CompromisedCredential implements Parcelable {
+ ", displayUsername='" + mDisplayUsername + '\'' + ", password='" + mPassword + ", displayUsername='" + mDisplayUsername + '\'' + ", password='" + mPassword
+ '\'' + ", passwordChangeUrl='" + mPasswordChangeUrl + '\'' + ", associatedApp='" + '\'' + ", passwordChangeUrl='" + mPasswordChangeUrl + '\'' + ", associatedApp='"
+ mAssociatedApp + '\'' + ", creationTime=" + mCreationTime + ", leaked=" + mLeaked + mAssociatedApp + '\'' + ", creationTime=" + mCreationTime + ", leaked=" + mLeaked
+ ", phished=" + mPhished + ", hasAutoChangeButton=" + mHasAutoChangeButton + '}'; + ", phished=" + mPhished + ", hasScript=" + mHasScript
+ ", hasAutoChangeButton=" + mHasAutoChangeButton + '}';
} }
@Override @Override
public int hashCode() { public int hashCode() {
return Objects.hash(mSignonRealm, mOrigin.getPossiblyInvalidSpec(), mUsername, return Objects.hash(mSignonRealm, mOrigin.getPossiblyInvalidSpec(), mUsername,
mDisplayOrigin, mDisplayUsername, mPassword, mPasswordChangeUrl, mAssociatedApp, mDisplayOrigin, mDisplayUsername, mPassword, mPasswordChangeUrl, mAssociatedApp,
mCreationTime, mLeaked, mPhished, mHasAutoChangeButton); mCreationTime, mLeaked, mPhished, mHasScript, mHasAutoChangeButton);
} }
@Override @Override
...@@ -188,7 +200,8 @@ public class CompromisedCredential implements Parcelable { ...@@ -188,7 +200,8 @@ public class CompromisedCredential implements Parcelable {
parcel.writeString(mPasswordChangeUrl); parcel.writeString(mPasswordChangeUrl);
parcel.writeString(mAssociatedApp); parcel.writeString(mAssociatedApp);
parcel.writeLong(mCreationTime); parcel.writeLong(mCreationTime);
parcel.writeBooleanArray(new boolean[] {mLeaked, mPhished, mHasAutoChangeButton}); parcel.writeBooleanArray(
new boolean[] {mLeaked, mPhished, mHasScript, mHasAutoChangeButton});
} }
@Override @Override
......
...@@ -65,10 +65,10 @@ import java.util.concurrent.ExecutionException; ...@@ -65,10 +65,10 @@ import java.util.concurrent.ExecutionException;
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE}) @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
@EnableFeatures({ChromeFeatureList.PASSWORD_CHECK}) @EnableFeatures({ChromeFeatureList.PASSWORD_CHECK})
public class PasswordCheckEditViewTest { public class PasswordCheckEditViewTest {
private static final CompromisedCredential ANA = private static final CompromisedCredential ANA = new CompromisedCredential(
new CompromisedCredential("https://some-url.com/signin", "https://some-url.com/signin", new GURL("https://some-url.com/"), "Ana", "some-url.com",
new GURL("https://some-url.com/"), "Ana", "some-url.com", "Ana", "password", "Ana", "password", "https://some-url.com/.well-known/change-password", "", 1, true,
"https://some-url.com/.well-known/change-password", "", 1, true, false, false); false, false, false);
private PasswordCheckEditFragmentView mPasswordCheckEditView; private PasswordCheckEditFragmentView mPasswordCheckEditView;
......
...@@ -102,26 +102,26 @@ import java.util.concurrent.atomic.AtomicInteger; ...@@ -102,26 +102,26 @@ import java.util.concurrent.atomic.AtomicInteger;
@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("https://some-url.com/signin", "https://some-url.com/signin", new GURL("https://some-url.com/"), "Ana", "some-url.com",
new GURL("https://some-url.com/"), "Ana", "some-url.com", "Ana", "password", "Ana", "password", "https://some-url.com/.well-known/change-password", "", 1, true,
"https://some-url.com/.well-known/change-password", "", 1, true, false, false); false, false, false);
private static final CompromisedCredential PHISHED = private static final CompromisedCredential PHISHED = new CompromisedCredential(
new CompromisedCredential("http://example.com/signin", new GURL("http://example.com/"), "http://example.com/signin", new GURL("http://example.com/"), "", "http://example.com",
"", "http://example.com", "(No username)", "DoSomething", "(No username)", "DoSomething", "http://example.com/.well-known/change-password", "", 1,
"http://example.com/.well-known/change-password", "", 1, false, true, false); false, true, false, false);
private static final CompromisedCredential LEAKED = private static final CompromisedCredential LEAKED =
new CompromisedCredential("https://some-other-url.com/signin", new CompromisedCredential("https://some-other-url.com/signin",
new GURL("https://some-other-url.com/"), "AZiegler", "some-other-url.com", new GURL("https://some-other-url.com/"), "AZiegler", "some-other-url.com",
"AZiegler", "N0M3rcy", "", "com.other.package", 1, true, false, false); "AZiegler", "N0M3rcy", "", "com.other.package", 1, true, false, false, false);
private static final CompromisedCredential LEAKED_AND_PHISHED = private static final CompromisedCredential LEAKED_AND_PHISHED =
new CompromisedCredential("https://super-important.com/signin", new CompromisedCredential("https://super-important.com/signin",
new GURL("https://super-important.com/"), "HSong", "super-important.com", new GURL("https://super-important.com/"), "HSong", "super-important.com",
"HSong", "N3rfTh1s", "", "com.important.super", 1, true, true, false); "HSong", "N3rfTh1s", "", "com.important.super", 1, true, true, false, false);
private static final CompromisedCredential SCRIPTED = private static final CompromisedCredential SCRIPTED = new CompromisedCredential(
new CompromisedCredential("https://script.com/signin", new GURL("https://script.com/"), "https://script.com/signin", new GURL("https://script.com/"), "Charlie", "script.com",
"Charlie", "script.com", "Charlie", "secret", "Charlie", "secret", "https://script.com/.well-known/change-password", "", 1, true,
"https://script.com/.well-known/change-password", "", 1, true, false, true); false, true, true);
private static final int LEAKS_COUNT = 2; private static final int LEAKS_COUNT = 2;
......
...@@ -84,10 +84,11 @@ import org.chromium.url.GURL; ...@@ -84,10 +84,11 @@ import org.chromium.url.GURL;
public class PasswordCheckControllerTest { public class PasswordCheckControllerTest {
private static final CompromisedCredential ANA = private static final CompromisedCredential ANA =
new CompromisedCredential("https://m.a.xyz/signin", mock(GURL.class), "Ana", "m.a.xyz", new CompromisedCredential("https://m.a.xyz/signin", mock(GURL.class), "Ana", "m.a.xyz",
"Ana", "password", "", "xyz.a.some.package", 2, true, false, false); "Ana", "password", "", "xyz.a.some.package", 2, true, false, false, false);
private static final CompromisedCredential BOB = new CompromisedCredential( private static final CompromisedCredential BOB =
"http://www.b.ch/signin", mock(GURL.class), "", "http://www.b.ch", "(No username)", new CompromisedCredential("http://www.b.ch/signin", mock(GURL.class), "",
"DoneSth", "http://www.b.ch/.well-known/change-password", "", 1, true, false, true); "http://www.b.ch", "(No username)", "DoneSth",
"http://www.b.ch/.well-known/change-password", "", 1, true, false, true, true);
private static final Pair<Integer, Integer> PROGRESS_UPDATE = new Pair<>(2, 19); private static final Pair<Integer, Integer> PROGRESS_UPDATE = new Pair<>(2, 19);
private static final String PASSWORD_CHECK_REFERRER_HISTOGRAM = private static final String PASSWORD_CHECK_REFERRER_HISTOGRAM =
"PasswordManager.BulkCheck.PasswordCheckReferrerAndroid"; "PasswordManager.BulkCheck.PasswordCheckReferrerAndroid";
...@@ -616,7 +617,7 @@ public class PasswordCheckControllerTest { ...@@ -616,7 +617,7 @@ public class PasswordCheckControllerTest {
private CompromisedCredential makeCredential( private CompromisedCredential makeCredential(
String origin, String username, long creationTime, boolean leaked, boolean phished) { String origin, String username, long creationTime, boolean leaked, boolean phished) {
return new CompromisedCredential(origin, mock(GURL.class), username, origin, username, return new CompromisedCredential(origin, mock(GURL.class), username, origin, username,
"password", origin, new String(), creationTime, leaked, phished, false); "password", origin, new String(), creationTime, leaked, phished, false, false);
} }
private PropertyModel getHeaderModel() { private PropertyModel getHeaderModel() {
......
...@@ -90,7 +90,7 @@ void PasswordCheckBridge::GetCompromisedCredentials( ...@@ -90,7 +90,7 @@ void PasswordCheckBridge::GetCompromisedCredentials(
password_manager::InsecureCredentialTypeFlags::kCredentialLeaked), password_manager::InsecureCredentialTypeFlags::kCredentialLeaked),
(credential.insecure_type == (credential.insecure_type ==
password_manager::InsecureCredentialTypeFlags::kCredentialPhished), password_manager::InsecureCredentialTypeFlags::kCredentialPhished),
credential.has_auto_change_button); credential.has_script, credential.has_auto_change_button);
} }
} }
......
...@@ -224,10 +224,13 @@ CompromisedCredentialForUI PasswordCheckManager::MakeUICredential( ...@@ -224,10 +224,13 @@ CompromisedCredentialForUI PasswordCheckManager::MakeUICredential(
credential.signon_realm); credential.signon_realm);
ui_credential.display_username = GetDisplayUsername(credential.username); ui_credential.display_username = GetDisplayUsername(credential.username);
ui_credential.has_auto_change_button = ui_credential.has_script =
!credential.username.empty() && ShouldOfferAutomaticPasswordChange() && !credential.username.empty() && ShouldOfferAutomaticPasswordChange() &&
password_script_fetcher_->IsScriptAvailable( password_script_fetcher_->IsScriptAvailable(
url::Origin::Create(credential.url.GetOrigin())); url::Origin::Create(credential.url.GetOrigin()));
// TODO(crbug.com/1132230): Implement separate logic for scripts fetching and
// auto buttons. For now, has_auto_change_button <=> has_script.
ui_credential.has_auto_change_button = ui_credential.has_script;
if (facet.IsValidAndroidFacetURI()) { if (facet.IsValidAndroidFacetURI()) {
const PasswordForm& android_form = const PasswordForm& android_form =
......
...@@ -51,6 +51,7 @@ class PasswordCheckManager ...@@ -51,6 +51,7 @@ class PasswordCheckManager
base::string16 display_origin; base::string16 display_origin;
std::string package_name; std::string package_name;
std::string change_password_url; std::string change_password_url;
bool has_script = false;
bool has_auto_change_button = false; bool has_auto_change_button = false;
}; };
......
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