Commit 331278ef authored by Maxim Kolosovskiy's avatar Maxim Kolosovskiy Committed by Commit Bot

[Passwords] Rename |has_script| to |has_auto_change_button|

(in CompromisedCredential*)

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|
(this CL)
2. A "new" |has_script| will be introduced, where |has_script|==
|has_auto_change_button|.
3. Implement special logic for the new |has_script|.

Bug: 1132230
Change-Id: I684921dc2e4aedaea20ac467c67512915dfa3590
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429409
Commit-Queue: Maxim Kolosovskiy  <kolos@chromium.org>
Reviewed-by: default avatarIoana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811181}
parent 62ced48e
...@@ -64,14 +64,14 @@ class PasswordCheckBridge { ...@@ -64,14 +64,14 @@ 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 hasScript) { String associatedApp, long creationTime, 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, hasScript)); associatedApp, creationTime, true, false, hasAutoChangeButton));
} }
@CalledByNative @CalledByNative
...@@ -98,10 +98,10 @@ class PasswordCheckBridge { ...@@ -98,10 +98,10 @@ 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 hasScript) { long creationTime, boolean leaked, boolean phished, 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, hasScript); phished, hasAutoChangeButton);
} }
/** /**
......
...@@ -274,7 +274,7 @@ class PasswordCheckMediator ...@@ -274,7 +274,7 @@ class PasswordCheckMediator
@Override @Override
public void onChangePasswordButtonClick(CompromisedCredential credential) { public void onChangePasswordButtonClick(CompromisedCredential credential) {
PasswordCheckMetricsRecorder.recordUiUserAction(credential.hasScript() PasswordCheckMetricsRecorder.recordUiUserAction(credential.hasAutoChangeButton()
? PasswordCheckUserAction.CHANGE_PASSWORD_MANUALLY ? PasswordCheckUserAction.CHANGE_PASSWORD_MANUALLY
: PasswordCheckUserAction.CHANGE_PASSWORD); : PasswordCheckUserAction.CHANGE_PASSWORD);
mChangePasswordDelegate.launchAppOrCctWithChangePasswordUrl(credential); mChangePasswordDelegate.launchAppOrCctWithChangePasswordUrl(credential);
...@@ -282,7 +282,7 @@ class PasswordCheckMediator ...@@ -282,7 +282,7 @@ class PasswordCheckMediator
@Override @Override
public void onChangePasswordWithScriptButtonClick(CompromisedCredential credential) { public void onChangePasswordWithScriptButtonClick(CompromisedCredential credential) {
assert credential.hasScript(); assert credential.hasAutoChangeButton();
PasswordCheckMetricsRecorder.recordUiUserAction( PasswordCheckMetricsRecorder.recordUiUserAction(
PasswordCheckUserAction.CHANGE_PASSWORD_AUTOMATICALLY); PasswordCheckUserAction.CHANGE_PASSWORD_AUTOMATICALLY);
mChangePasswordDelegate.launchCctWithScript(credential); mChangePasswordDelegate.launchCctWithScript(credential);
...@@ -350,7 +350,7 @@ class PasswordCheckMediator ...@@ -350,7 +350,7 @@ class PasswordCheckMediator
mIconHelper.getLargeIcon(credential, (faviconOrFallback) -> { mIconHelper.getLargeIcon(credential, (faviconOrFallback) -> {
credentialModel.set(FAVICON_OR_FALLBACK, faviconOrFallback); credentialModel.set(FAVICON_OR_FALLBACK, faviconOrFallback);
}); });
return new ListItem(credential.hasScript() return new ListItem(credential.hasAutoChangeButton()
? PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL_WITH_SCRIPT ? PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL_WITH_SCRIPT
: PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL, : PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL,
credentialModel); credentialModel);
......
...@@ -165,7 +165,7 @@ class PasswordCheckViewBinder { ...@@ -165,7 +165,7 @@ class PasswordCheckViewBinder {
}); });
setTintListForCompoundDrawables(button.getCompoundDrawablesRelative(), setTintListForCompoundDrawables(button.getCompoundDrawablesRelative(),
view.getContext(), org.chromium.ui.R.color.default_text_color_inverse); view.getContext(), org.chromium.ui.R.color.default_text_color_inverse);
if (credential.hasScript()) { if (credential.hasAutoChangeButton()) {
ButtonCompat button_with_script = ButtonCompat button_with_script =
view.findViewById(R.id.credential_change_button_with_script); view.findViewById(R.id.credential_change_button_with_script);
button_with_script.setOnClickListener(unusedView -> { button_with_script.setOnClickListener(unusedView -> {
...@@ -179,7 +179,8 @@ class PasswordCheckViewBinder { ...@@ -179,7 +179,8 @@ class PasswordCheckViewBinder {
ButtonCompat button = view.findViewById(R.id.credential_change_button); ButtonCompat button = view.findViewById(R.id.credential_change_button);
button.setVisibility(model.get(HAS_MANUAL_CHANGE_BUTTON) ? View.VISIBLE : View.GONE); button.setVisibility(model.get(HAS_MANUAL_CHANGE_BUTTON) ? View.VISIBLE : View.GONE);
TextView changeHint = view.findViewById(R.id.credential_change_hint); TextView changeHint = view.findViewById(R.id.credential_change_hint);
changeHint.setVisibility(model.get(HAS_MANUAL_CHANGE_BUTTON) || credential.hasScript() changeHint.setVisibility(
model.get(HAS_MANUAL_CHANGE_BUTTON) || credential.hasAutoChangeButton()
? View.GONE ? View.GONE
: View.VISIBLE); : View.VISIBLE);
} else if (propertyKey == FAVICON_OR_FALLBACK) { } else if (propertyKey == FAVICON_OR_FALLBACK) {
......
...@@ -35,11 +35,11 @@ public class CompromisedCredential implements Parcelable { ...@@ -35,11 +35,11 @@ public class CompromisedCredential implements Parcelable {
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 hasScript = boolArguments[2]; final boolean hasAutoChangeButton = boolArguments[2];
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, hasScript); creationTime, leaked, phished, hasAutoChangeButton);
} }
@Override @Override
...@@ -59,7 +59,7 @@ public class CompromisedCredential implements Parcelable { ...@@ -59,7 +59,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;
/** /**
* @param signonRealm The URL leading to the sign-on page. * @param signonRealm The URL leading to the sign-on page.
...@@ -75,12 +75,12 @@ public class CompromisedCredential implements Parcelable { ...@@ -75,12 +75,12 @@ 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 hasScript True iff the credential can be automatically fixed. * @param hasAutoChangeButton True iff the credential can be automatically fixed.
*/ */
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 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!";
...@@ -100,7 +100,7 @@ public class CompromisedCredential implements Parcelable { ...@@ -100,7 +100,7 @@ public class CompromisedCredential implements Parcelable {
mCreationTime = creationTime; mCreationTime = creationTime;
mLeaked = leaked; mLeaked = leaked;
mPhished = phished; mPhished = phished;
mHasScript = hasScript; mHasAutoChangeButton = hasAutoChangeButton;
} }
@CalledByNative @CalledByNative
...@@ -140,8 +140,8 @@ public class CompromisedCredential implements Parcelable { ...@@ -140,8 +140,8 @@ public class CompromisedCredential implements Parcelable {
public boolean isPhished() { public boolean isPhished() {
return mPhished; return mPhished;
} }
public boolean hasScript() { public boolean hasAutoChangeButton() {
return mHasScript; return mHasAutoChangeButton;
} }
@Override @Override
...@@ -156,7 +156,7 @@ public class CompromisedCredential implements Parcelable { ...@@ -156,7 +156,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;
} }
@Override @Override
...@@ -167,14 +167,14 @@ public class CompromisedCredential implements Parcelable { ...@@ -167,14 +167,14 @@ 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 + ", hasScript=" + mHasScript + '}'; + ", phished=" + mPhished + ", 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, mHasScript); mCreationTime, mLeaked, mPhished, mHasAutoChangeButton);
} }
@Override @Override
...@@ -188,7 +188,7 @@ public class CompromisedCredential implements Parcelable { ...@@ -188,7 +188,7 @@ 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, mHasScript}); parcel.writeBooleanArray(new boolean[] {mLeaked, mPhished, mHasAutoChangeButton});
} }
@Override @Override
......
...@@ -738,7 +738,7 @@ public class PasswordCheckViewTest { ...@@ -738,7 +738,7 @@ public class PasswordCheckViewTest {
} }
private MVCListAdapter.ListItem buildCredentialItem(CompromisedCredential credential) { private MVCListAdapter.ListItem buildCredentialItem(CompromisedCredential credential) {
return new MVCListAdapter.ListItem(credential.hasScript() return new MVCListAdapter.ListItem(credential.hasAutoChangeButton()
? PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL_WITH_SCRIPT ? PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL_WITH_SCRIPT
: PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL, : PasswordCheckProperties.ItemType.COMPROMISED_CREDENTIAL,
new PropertyModel new PropertyModel
......
...@@ -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_script); credential.has_auto_change_button);
} }
} }
......
...@@ -224,7 +224,7 @@ CompromisedCredentialForUI PasswordCheckManager::MakeUICredential( ...@@ -224,7 +224,7 @@ 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_script = ui_credential.has_auto_change_button =
!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()));
......
...@@ -51,7 +51,7 @@ class PasswordCheckManager ...@@ -51,7 +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;
}; };
// `observer` must outlive `this`. // `observer` must outlive `this`.
......
...@@ -188,7 +188,7 @@ auto ExpectCompromisedCredentialForUI( ...@@ -188,7 +188,7 @@ auto ExpectCompromisedCredentialForUI(
const base::Optional<std::string>& package_name, const base::Optional<std::string>& package_name,
const base::Optional<std::string>& change_password_url, const base::Optional<std::string>& change_password_url,
InsecureCredentialTypeFlags insecure_type, InsecureCredentialTypeFlags insecure_type,
bool has_script) { bool has_auto_change_button) {
auto package_name_field_matcher = auto package_name_field_matcher =
package_name.has_value() package_name.has_value()
? Field(&CompromisedCredentialForUI::package_name, ? Field(&CompromisedCredentialForUI::package_name,
...@@ -204,7 +204,8 @@ auto ExpectCompromisedCredentialForUI( ...@@ -204,7 +204,8 @@ auto ExpectCompromisedCredentialForUI(
Field(&CompromisedCredentialForUI::display_origin, display_origin), Field(&CompromisedCredentialForUI::display_origin, display_origin),
package_name_field_matcher, change_password_url_field_matcher, package_name_field_matcher, change_password_url_field_matcher,
Field(&CompromisedCredentialForUI::insecure_type, insecure_type), Field(&CompromisedCredentialForUI::insecure_type, insecure_type),
Field(&CompromisedCredentialForUI::has_script, has_script)); Field(&CompromisedCredentialForUI::has_auto_change_button,
has_auto_change_button));
} }
} // namespace } // namespace
...@@ -343,7 +344,7 @@ TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForSiteCredential) { ...@@ -343,7 +344,7 @@ TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForSiteCredential) {
base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"), base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
base::nullopt, "https://example.com/", base::nullopt, "https://example.com/",
InsecureCredentialTypeFlags::kCredentialLeaked, InsecureCredentialTypeFlags::kCredentialLeaked,
/*has_script=*/false))); /*has_auto_change_button=*/false)));
} }
TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForAppCredentials) { TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForAppCredentials) {
...@@ -367,12 +368,12 @@ TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForAppCredentials) { ...@@ -367,12 +368,12 @@ TEST_F(PasswordCheckManagerTest, CorrectlyCreatesUIStructForAppCredentials) {
base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16(kUsername1),
base::ASCIIToUTF16("App (com.example.app)"), "com.example.app", base::ASCIIToUTF16("App (com.example.app)"), "com.example.app",
base::nullopt, InsecureCredentialTypeFlags::kCredentialLeaked, base::nullopt, InsecureCredentialTypeFlags::kCredentialLeaked,
/*has_script=*/false), /*has_auto_change_button=*/false),
ExpectCompromisedCredentialForUI( ExpectCompromisedCredentialForUI(
base::ASCIIToUTF16(kUsername2), base::ASCIIToUTF16("Example App"), base::ASCIIToUTF16(kUsername2), base::ASCIIToUTF16("Example App"),
"com.example.app", base::nullopt, "com.example.app", base::nullopt,
InsecureCredentialTypeFlags::kCredentialLeaked, InsecureCredentialTypeFlags::kCredentialLeaked,
/*has_script=*/false))); /*has_auto_change_button=*/false)));
} }
TEST_F(PasswordCheckManagerTest, SetsTimestampOnSuccessfulCheck) { TEST_F(PasswordCheckManagerTest, SetsTimestampOnSuccessfulCheck) {
...@@ -422,7 +423,7 @@ TEST_F(PasswordCheckManagerTest, ...@@ -422,7 +423,7 @@ TEST_F(PasswordCheckManagerTest,
base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"), base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
base::nullopt, "https://example.com/", base::nullopt, "https://example.com/",
InsecureCredentialTypeFlags::kCredentialLeaked, InsecureCredentialTypeFlags::kCredentialLeaked,
/*has_script=*/false))); /*has_auto_change_button=*/false)));
} }
TEST_F(PasswordCheckManagerTest, TEST_F(PasswordCheckManagerTest,
...@@ -449,7 +450,7 @@ TEST_F(PasswordCheckManagerTest, ...@@ -449,7 +450,7 @@ TEST_F(PasswordCheckManagerTest,
base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"), base::ASCIIToUTF16(kUsername1), base::ASCIIToUTF16("example.com"),
base::nullopt, "https://example.com/", base::nullopt, "https://example.com/",
InsecureCredentialTypeFlags::kCredentialLeaked, InsecureCredentialTypeFlags::kCredentialLeaked,
/*has_script=*/true))); /*has_auto_change_button=*/true)));
} }
TEST_F(PasswordCheckManagerTest, TEST_F(PasswordCheckManagerTest,
...@@ -476,7 +477,7 @@ TEST_F(PasswordCheckManagerTest, ...@@ -476,7 +477,7 @@ TEST_F(PasswordCheckManagerTest,
base::ASCIIToUTF16("No username"), base::ASCIIToUTF16("example.com"), base::ASCIIToUTF16("No username"), base::ASCIIToUTF16("example.com"),
base::nullopt, "https://example.com/", base::nullopt, "https://example.com/",
InsecureCredentialTypeFlags::kCredentialLeaked, InsecureCredentialTypeFlags::kCredentialLeaked,
/*has_script=*/false))); /*has_auto_change_button=*/false)));
} }
TEST_F(PasswordCheckManagerTest, UpdatesProgressCorrectly) { TEST_F(PasswordCheckManagerTest, UpdatesProgressCorrectly) {
......
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