From da71f3120738c689cd86e0b1bf4553a727a7b8a2 Mon Sep 17 00:00:00 2001 From: Matthew Noorenberghe Date: Sat, 28 Mar 2020 04:50:13 +0000 Subject: [PATCH] Bug 1551730 - Use hasBeenTypePassword to determine if we're on a password field for autocomplete. r=sfoster Differential Revision: https://phabricator.services.mozilla.com/D68323 --HG-- rename : toolkit/components/passwordmgr/test/mochitest/test_autocomplete_basic_form_subdomain.html => toolkit/components/passwordmgr/test/mochitest/test_autocomplete_hasBeenTypePassword.html extra : moz-landing-system : lando --- .../passwordmgr/LoginAutoComplete.jsm | 30 ++--- .../passwordmgr/LoginManagerParent.jsm | 4 +- .../browser_autocomplete_master_password.js | 2 +- .../passwordmgr/test/mochitest/mochitest.ini | 3 + ...test_autocomplete_hasBeenTypePassword.html | 117 ++++++++++++++++++ ...LoginManagerParent_doAutocompleteSearch.js | 4 +- .../unit/test_login_autocomplete_result.js | 56 ++++----- .../satchel/nsFormFillController.cpp | 17 ++- 8 files changed, 175 insertions(+), 58 deletions(-) create mode 100644 toolkit/components/passwordmgr/test/mochitest/test_autocomplete_hasBeenTypePassword.html diff --git a/toolkit/components/passwordmgr/LoginAutoComplete.jsm b/toolkit/components/passwordmgr/LoginAutoComplete.jsm index 59f2bbfa6c12..cd88909fbe49 100644 --- a/toolkit/components/passwordmgr/LoginAutoComplete.jsm +++ b/toolkit/components/passwordmgr/LoginAutoComplete.jsm @@ -159,7 +159,7 @@ class InsecureLoginFormAutocompleteItem extends AutocompleteItem { class LoginAutocompleteItem extends AutocompleteItem { constructor( login, - isPasswordField, + hasBeenTypePassword, duplicateUsernames, actor, isOriginMatched @@ -184,7 +184,7 @@ class LoginAutocompleteItem extends AutocompleteItem { }); XPCOMUtils.defineLazyGetter(this, "value", () => { - return isPasswordField ? login.password : login.username; + return hasBeenTypePassword ? login.password : login.username; }); XPCOMUtils.defineLazyGetter(this, "comment", () => { @@ -257,7 +257,7 @@ function LoginAutoCompleteResult( willAutoSaveGeneratedPassword, isSecure, actor, - isPasswordField, + hasBeenTypePassword, hostname, telemetryEventData, } @@ -272,7 +272,7 @@ function LoginAutoCompleteResult( // Don't show the footer on non-empty password fields as it's not providing // value and only adding noise since a password was already filled. - if (isPasswordField && aSearchString && !generatedPassword) { + if (hasBeenTypePassword && aSearchString && !generatedPassword) { log.debug("Hiding footer: non-empty password field"); return false; } @@ -280,7 +280,7 @@ function LoginAutoCompleteResult( if ( !matchingLogins.length && !generatedPassword && - isPasswordField && + hasBeenTypePassword && formFillController.passwordPopupAutomaticallyOpened ) { hidingFooterOnPWFieldAutoOpened = true; @@ -311,7 +311,7 @@ function LoginAutoCompleteResult( for (let login of logins) { let item = new LoginAutocompleteItem( login, - isPasswordField, + hasBeenTypePassword, duplicateUsernames, actor, LoginHelper.isOriginMatching(login.origin, formOrigin, { @@ -483,7 +483,7 @@ LoginAutoComplete.prototype = { if (isSecure) { isSecure = InsecurePasswordUtils.isFormSecure(form); } - let isPasswordField = aElement.type == "password"; + let { hasBeenTypePassword } = aElement; let hostname = aElement.ownerDocument.documentURIObject.host; let formOrigin = LoginHelper.getLoginOrigin( aElement.ownerDocument.documentURI @@ -529,7 +529,7 @@ LoginAutoComplete.prototype = { willAutoSaveGeneratedPassword, actor: loginManagerActor, isSecure, - isPasswordField, + hasBeenTypePassword, hostname, telemetryEventData, } @@ -545,7 +545,7 @@ LoginAutoComplete.prototype = { } if ( - isPasswordField && + hasBeenTypePassword && aSearchString && !loginManagerActor.isPasswordGenerationForcedOn(aElement) ) { @@ -578,7 +578,7 @@ LoginAutoComplete.prototype = { inputElement: aElement, form, formOrigin, - isPasswordField, + hasBeenTypePassword, }); completeSearch(acLookupPromise).catch(log.error.bind(log)); }, @@ -593,7 +593,7 @@ LoginAutoComplete.prototype = { inputElement, form, formOrigin, - isPasswordField, + hasBeenTypePassword, }) { let actionOrigin = LoginHelper.getFormActionOrigin(form); let autocompleteInfo = inputElement.getAutocompleteInfo(); @@ -603,7 +603,7 @@ LoginAutoComplete.prototype = { ); let forcePasswordGeneration = false; let isProbablyANewPasswordField = false; - if (isPasswordField) { + if (hasBeenTypePassword) { forcePasswordGeneration = loginManagerActor.isPasswordGenerationForcedOn( inputElement ); @@ -620,8 +620,8 @@ LoginAutoComplete.prototype = { searchString, previousResult, forcePasswordGeneration, + hasBeenTypePassword, isSecure: InsecurePasswordUtils.isFormSecure(form), - isPasswordField, isProbablyANewPasswordField, }; @@ -632,9 +632,9 @@ LoginAutoComplete.prototype = { log.debug("LoginAutoComplete search:", { forcePasswordGeneration, isSecure: messageData.isSecure, - isPasswordField, + hasBeenTypePassword, isProbablyANewPasswordField, - searchString: isPasswordField + searchString: hasBeenTypePassword ? "*".repeat(searchString.length) : searchString, }); diff --git a/toolkit/components/passwordmgr/LoginManagerParent.jsm b/toolkit/components/passwordmgr/LoginManagerParent.jsm index 0a1a2b8d8e53..24ddb2b48f74 100644 --- a/toolkit/components/passwordmgr/LoginManagerParent.jsm +++ b/toolkit/components/passwordmgr/LoginManagerParent.jsm @@ -417,8 +417,8 @@ class LoginManagerParent extends JSWindowActorParent { searchString, previousResult, forcePasswordGeneration, + hasBeenTypePassword, isSecure, - isPasswordField, isProbablyANewPasswordField, }) { // Note: previousResult is a regular object, not an @@ -477,7 +477,7 @@ class LoginManagerParent extends JSWindowActorParent { // Remove results that are too short, or have different prefix. // Also don't offer empty usernames as possible results except // for on password fields. - if (isPasswordField) { + if (hasBeenTypePassword) { return true; } return match && match.toLowerCase().startsWith(searchStringLower); diff --git a/toolkit/components/passwordmgr/test/browser/browser_autocomplete_master_password.js b/toolkit/components/passwordmgr/test/browser/browser_autocomplete_master_password.js index edbec399fcf0..a45d14ff1d56 100644 --- a/toolkit/components/passwordmgr/test/browser/browser_autocomplete_master_password.js +++ b/toolkit/components/passwordmgr/test/browser/browser_autocomplete_master_password.js @@ -100,8 +100,8 @@ add_task(async function test_mpAutocompleteUIBusy() { actionOrigin: "", searchString: "", previousResult: null, + hasBeenTypePassword: true, isSecure: false, - isPasswordField: true, isProbablyANewPasswordField: true, }; diff --git a/toolkit/components/passwordmgr/test/mochitest/mochitest.ini b/toolkit/components/passwordmgr/test/mochitest/mochitest.ini index 6497511a8ea3..aa33f6f01026 100644 --- a/toolkit/components/passwordmgr/test/mochitest/mochitest.ini +++ b/toolkit/components/passwordmgr/test/mochitest/mochitest.ini @@ -37,6 +37,9 @@ scheme = https [test_autocomplete_basic_form_subdomain.html] skip-if = toolkit == 'android' # android:autocomplete. scheme = https +[test_autocomplete_hasBeenTypePassword.html] +scheme = https +skip-if = toolkit == 'android' # autocomplete [test_autocomplete_highlight.html] scheme = https skip-if = toolkit == 'android' # autocomplete diff --git a/toolkit/components/passwordmgr/test/mochitest/test_autocomplete_hasBeenTypePassword.html b/toolkit/components/passwordmgr/test/mochitest/test_autocomplete_hasBeenTypePassword.html new file mode 100644 index 000000000000..5ce5e14a22e7 --- /dev/null +++ b/toolkit/components/passwordmgr/test/mochitest/test_autocomplete_hasBeenTypePassword.html @@ -0,0 +1,117 @@ + + + + + Test that passwords are autocompleted into fields that were previously type=password + + + + + + + + +Login Manager test: Test that passwords are autocompleted into fields that were previously type=password + + +

+ + +
+ + +
+ + + +
+
+
+
+
+ + diff --git a/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_doAutocompleteSearch.js b/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_doAutocompleteSearch.js index 97668f65a046..11327a41dc0b 100644 --- a/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_doAutocompleteSearch.js +++ b/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_doAutocompleteSearch.js @@ -16,8 +16,8 @@ const NEW_PASSWORD_TEMPLATE_ARG = { searchString: "", previousResult: null, requestId: "foo", + hasBeenTypePassword: true, isSecure: true, - isPasswordField: true, isProbablyANewPasswordField: true, }; @@ -72,7 +72,7 @@ add_task(async function test_generated_noLogins() { let result3 = await LMP.doAutocompleteSearch({ ...NEW_PASSWORD_TEMPLATE_ARG, ...{ - isPasswordField: false, + hasBeenTypePassword: false, isProbablyANewPasswordField: false, }, }); diff --git a/toolkit/components/passwordmgr/test/unit/test_login_autocomplete_result.js b/toolkit/components/passwordmgr/test/unit/test_login_autocomplete_result.js index 6bb28cc51ec9..f628a2cac72f 100644 --- a/toolkit/components/passwordmgr/test/unit/test_login_autocomplete_result.js +++ b/toolkit/components/passwordmgr/test/unit/test_login_autocomplete_result.js @@ -104,7 +104,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -154,7 +154,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: false, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins: [], items: [ { @@ -175,7 +175,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: false, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -232,7 +232,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins, items: [ { @@ -282,7 +282,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: false, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins, items: [ { @@ -339,7 +339,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: true, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -389,7 +389,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: false, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -439,7 +439,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins, items: [ { @@ -489,7 +489,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: false, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins, items: [ { @@ -539,7 +539,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -589,7 +589,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: false, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -646,7 +646,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins, items: [ { @@ -696,7 +696,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: false, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins, items: [ { @@ -753,7 +753,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: true, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -803,7 +803,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: false, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins: [], items: [ { @@ -817,7 +817,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: false, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins: [], searchString: "foo", items: [ @@ -832,7 +832,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: false, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -882,7 +882,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins, items: [ { @@ -932,7 +932,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: false, isSecure: false, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins, items: [ { @@ -982,7 +982,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins: [], items: [ { @@ -996,7 +996,7 @@ add_task(async function test_all_patterns() { { insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins: [], searchString: "foo", items: [], @@ -1005,7 +1005,7 @@ add_task(async function test_all_patterns() { generatedPassword: "9ljgfd4shyktb45", insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins: [], items: [ { @@ -1032,7 +1032,7 @@ add_task(async function test_all_patterns() { willAutoSaveGeneratedPassword: true, insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins: [], items: [ { @@ -1058,7 +1058,7 @@ add_task(async function test_all_patterns() { generatedPassword: "9ljgfd4shyktb45", insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins: [], searchString: "9ljgfd4shyktb45", items: [ @@ -1084,7 +1084,7 @@ add_task(async function test_all_patterns() { formOrigin: "https://sub.mochi.test:8888", insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -1136,7 +1136,7 @@ add_task(async function test_all_patterns() { formOrigin: "https://sub.mochi.test:8888", insecureFieldWarningEnabled: true, isSecure: true, - isPasswordField: true, + hasBeenTypePassword: true, matchingLogins, items: [ { @@ -1188,7 +1188,7 @@ add_task(async function test_all_patterns() { formOrigin: "https://mochi.test:8888", schemeUpgrades: false, isSecure: true, - isPasswordField: false, + hasBeenTypePassword: false, matchingLogins, items: [ { @@ -1261,7 +1261,7 @@ add_task(async function test_all_patterns() { generatedPassword: pattern.generatedPassword, willAutoSaveGeneratedPassword: !!pattern.willAutoSaveGeneratedPassword, isSecure: pattern.isSecure, - isPasswordField: pattern.isPasswordField, + hasBeenTypePassword: pattern.hasBeenTypePassword, telemetryEventData: { searchStartTimeMS: testIndex }, } ); diff --git a/toolkit/components/satchel/nsFormFillController.cpp b/toolkit/components/satchel/nsFormFillController.cpp index 18f164eadc29..bb4d40e44d31 100644 --- a/toolkit/components/satchel/nsFormFillController.cpp +++ b/toolkit/components/satchel/nsFormFillController.cpp @@ -647,9 +647,8 @@ nsFormFillController::GetNoRollupOnCaretMove(bool* aNoRollupOnCaretMove) { NS_IMETHODIMP nsFormFillController::GetNoRollupOnEmptySearch(bool* aNoRollupOnEmptySearch) { - if (mFocusedInput && - (mPwmgrInputs.Get(mFocusedInput) || - mFocusedInput->ControlType() == NS_FORM_INPUT_PASSWORD)) { + if (mFocusedInput && (mPwmgrInputs.Get(mFocusedInput) || + mFocusedInput->HasBeenTypePassword())) { // Don't close the login popup when the field is cleared (bug 1534896). *aNoRollupOnEmptySearch = true; } else { @@ -680,9 +679,8 @@ nsFormFillController::StartSearch(const nsAString& aSearchString, // handle the autocomplete. Otherwise, handle with form history. // This method is sometimes called in unit tests and from XUL without a // focused node. - if (mFocusedInput && - (mPwmgrInputs.Get(mFocusedInput) || - mFocusedInput->ControlType() == NS_FORM_INPUT_PASSWORD)) { + if (mFocusedInput && (mPwmgrInputs.Get(mFocusedInput) || + mFocusedInput->HasBeenTypePassword())) { MOZ_LOG(sLogger, LogLevel::Debug, ("StartSearch: login field")); // Handle the case where a password field is focused but @@ -941,8 +939,7 @@ void nsFormFillController::MaybeStartControllingInput( bool hasList = !!aInput->GetList(); bool isPwmgrInput = false; - if (mPwmgrInputs.Get(aInput) || - aInput->ControlType() == NS_FORM_INPUT_PASSWORD) { + if (mPwmgrInputs.Get(aInput) || aInput->HasBeenTypePassword()) { isPwmgrInput = true; } @@ -958,7 +955,7 @@ void nsFormFillController::MaybeStartControllingInput( #ifdef NIGHTLY_BUILD // Trigger an asynchronous login reputation query when user focuses on the // password field. - if (aInput->ControlType() == NS_FORM_INPUT_PASSWORD) { + if (aInput->HasBeenTypePassword()) { StartQueryLoginReputation(aInput); } #endif @@ -981,7 +978,7 @@ nsresult nsFormFillController::HandleFocus(HTMLInputElement* aInput) { // multiple input forms and the fact that a mousedown into an already focused // field does not trigger another focus. - if (mFocusedInput->ControlType() != NS_FORM_INPUT_PASSWORD) { + if (!mFocusedInput->HasBeenTypePassword()) { return NS_OK; }