Closed Bug 1243722 Opened 4 years ago Closed 4 years ago

Wrong doorhanger when updating password on Facebook

Categories

(Toolkit :: Password Manager, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox46 blocking verified
firefox47 --- verified
firefox48 --- verified
firefox-esr45 --- affected

People

(Reporter: simona.marcu, Assigned: timdream)

References

Details

(Keywords: regression)

Attachments

(2 files)

Mozilla/5.0 (X11; Linux i686; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID: 20160127030236
Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0 Build ID: 20160105164030

When updating the password from the website's setting page, a new username is added instead of just updating the password for the existing one.

Steps to reproduce:
1. Launch the latest Nightly with a new profile.
2. Navigate to https://www.facebook.com/
3. Enter your e-mail address and password 
4. Click on the Log-in button
5. In the password doorhanger choose to Remember your log-in 
6. On your Facebook page -> go to Settings -> Click Edit in the Password section
7. Add the new password, re-type it and click on Save Changes.

Expected results:
The password doorhanger with the same username should prompt with the option to Update the password. In the Saved logins list I should have only one username with the updated password.

Actual results:
The password doorhanger with a changed username (instead of the e-mail address used in step 3 I got "2") prompts me to Remember (instead of Update) the log-in as if this is a new log-in when in reality is the same user but with an updated password. A new login is entered in the Saved Logins list.

If I log out first and then I enter my e-mail address with a different password I get the right information in the password doorhanger: the user field is completed with my e-mail address and I get the option to Update the password.

This is also reproducible with other websites: Yahoo for instance - If I update the password while being logged in, I get an empty username and the Remember option in the password doorhanger. 

Reproducible on all platforms (Windows, Mac OS X and Ubuntu) on the latest Nightly 47 and Firefox 43.0.4.
Perhaps another regression from bug 1016051?
Has Regression Range: --- → no
Priority: -- → P1
Regression Range:

Pushlog
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8122ffa9e1aa&tochange=b0e7f63c2138

I couldn't bisect further because these builds are probably too old. I guess changeset https://hg.mozilla.org/mozilla-central/rev/27061dc242e4 might have caused the issue. 

Probably a regression from https://bugzilla.mozilla.org/show_bug.cgi?id=956906
The login form comes with <input name="email"> and <input name="pass">, and the change password form comes with <input name="password_old">, <input name="password_new">, and <input name="password_confirm">. I wonder if our password manager heustic support this?
After attempting to change my own password, I found that we incorrectly capture a

<input class="inputtext" name="password_strength" id="password_strength" type="text">

input box as the username field in that form. The input is inside an `display: hidden` <tr>.

(Hint for testing: you can prevent actually change your password by typing your old password into the new password field)
Summary: Wrong doorhanger when updating password → Wrong doorhanger when updating password on Facebook
The username field was located by this:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#659-671

I wonder if the easiest solution here is just create a recipe for facebook.com? Would that hard to maintain in the long run? I am afraid that adding more checks (e.g. see if the <input> is visible) will break more sites.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/1-2/
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

This is not the best approach. I am touching the logic specified in test_recipe_login_fields.html, so if we want to alter the behavior, I would need to fix the test too.

Alternative would be create another property in the recipe like "skipUsernameFieldIfNotFound" and have the if block in question skip locating the field completely if there isn't one found from the "usernameSelector".

Of course, the new flag need it's own set of test(s).

:MattN, what do you think?
Flags: needinfo?(MattN+bmo)
Attachment #8735698 - Flags: review?(MattN+bmo)
I didn't look closely at this yet but one idea we have been planning to implement is a recipe property to indicate fields that aren't username or password fields e.g.:

> notUsername: "#password_strength"
https://reviewboard.mozilla.org/r/42917/#review40199

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:659
(Diff revision 2)
> -    if (!usernameField) {
> +    // Only try to locate username field when it didn't specify a usernameSelector,
> +    // or when there isn't a recipe at all.

I think this approach will make recipes too fragile and I prefer the `notUsernameSelector` approach.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/2-3/
Attachment #8735698 - Attachment description: MozReview Request: Bug 1243722 - Don't locate username field on our own if there is a recipe, r=MattN → MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN
Attachment #8735698 - Flags: review?(MattN+bmo)
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

The test is not valid -- we will only reach the loop if there isn't a usernameSelector.
Attachment #8735698 - Flags: review?(MattN+bmo)
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/3-4/
Attachment #8735698 - Flags: review?(MattN+bmo)
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a62b77c48145

The test was rearranged with SpawnTask.js because the number of async tasks needed -- I hope this does not make diff too hard to read!
Attachment #8735698 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

https://reviewboard.mozilla.org/r/42917/#review40931

Great, thanks!

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:666
(Diff revision 4)
>          if (LoginHelper.isUsernameFieldType(element)) {
> +          if (fieldOverrideRecipe && fieldOverrideRecipe.notUsernameSelector) {
> +            if (!element.matches(fieldOverrideRecipe.notUsernameSelector)) {
> -          usernameField = element;
> +              usernameField = element;
> -          break;
> +              break;
> -        }
> +            }
> +          } else {
> +            usernameField = element;
> +            break;
> +          }
> +        }
>        }

We normally prefer early-return style (though the old code wasn't doing so) and I think this would be clearer like so:
```
if (!LoginHelper.isUsernameFieldType(element)) {
  continue;
}

if (fieldOverrideRecipe && fieldOverrideRecipe.notUsernameSelector &&
    !element.matches(fieldOverrideRecipe.notUsernameSelector)) {
  continue;
}

usernameField = element;
break;
```

::: toolkit/components/passwordmgr/LoginRecipes.jsm:11
(Diff revision 4)
>  
>  this.EXPORTED_SYMBOLS = ["LoginRecipesContent", "LoginRecipesParent"];
>  
>  const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
>  const REQUIRED_KEYS = ["hosts"];
> -const OPTIONAL_KEYS = ["description", "passwordSelector", "pathRegex", "usernameSelector"];
> +const OPTIONAL_KEYS = ["description", "passwordSelector", "pathRegex", "usernameSelector", "notUsernameSelector"];

Nit: keep these sorted alphabetically

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:13
(Diff revision 4)
>  </head>
>  <body>
> -<script type="application/javascript;version=1.8">
> +<script>
> -SimpleTest.waitForExplicitFinish();
>  
> +var inputEventDeferreds = [];

Nit: use `let`

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:15
(Diff revision 4)
> -<script type="application/javascript;version=1.8">
> +<script>
> -SimpleTest.waitForExplicitFinish();
>  
> +var inputEventDeferreds = [];
> +
> +document.addEventListener("input", function handleInputEvent(evt) {

Could you attach this to #content instead to avoid catching other fills from pwmgr_common in the future? I guess it would need to move in setup() then

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:26
(Diff revision 4)
> +  function Deferred() {
> +    this.promise = new Promise((resolve) => {
> +      this.resolve = resolve;
> +    });
> +  }

Interesting approach… I thought Deferred was frowned upon, at least it is in browser code and Promise.defer was removed. I don't really care though it would be nice to avoid the boilerplate in this test.

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:42
(Diff revision 4)
> +function loadRecipes(recipes) {
> +  pwmgrCommonScript.sendAsyncMessage("loadRecipes", recipes);
> +  return new Promise((resolve) => {
> +    pwmgrCommonScript.addMessageListener("loadedRecipes", resolve);
> +  });
> +}

Can you put this in pwmgr_common.js too (if there's not something like it there already)?

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:102
(Diff revision 4)
> -  }
> -}
> +  elements.forEach((element) => {
> +    is(element.dataset.expected, "true", `${element.name} was filled`);
> +  });

Nit for the future: for…of is generally preferred over forEach as it's a more natural control structure

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:107
(Diff revision 4)
> +add_task(function* resetRecipes() {
> +  pwmgrCommonScript.sendAsyncMessage("resetRecipes", {});
> +
> +  yield new Promise((resolve) => {
> +    pwmgrCommonScript.addMessageListener("doneResetRecipes", resolve);
> +  });
> +});

Could you put this function in pwmgr_common.js then call it with add_task here? I was planning on putting on there for other tests soon anyways.

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:115
(Diff revision 4)
> +  yield new Promise((resolve) => {
> +    pwmgrCommonScript.addMessageListener("doneResetRecipes", resolve);
> +  });
> +});
> +
> +add_task(function* loadSecondRecipes() {

Nit: loadNotUsernameSelectorRecipes

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:126
(Diff revision 4)
> +  });
> +});
> +
> +add_task(function* test_not_username_field() {
> +  document.getElementById("content").innerHTML = `
> +    <!-- The field matches notUsernameSelector should be skipped -->

Nit: s/matches/matching/
https://reviewboard.mozilla.org/r/42917/#review40931

> Can you put this in pwmgr_common.js too (if there's not something like it there already)?

I thought it was already there but I guess it was the patch in bug 1258860 that's not landed yet. If you want to take it from that patch and put it in the same place in the file then it will be easy for me to rebase over your patch.

> Could you put this function in pwmgr_common.js then call it with add_task here? I was planning on putting on there for other tests soon anyways.

Same here. I put a helper in bug 1258860.
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/4-5/
https://reviewboard.mozilla.org/r/42917/#review40931

> Interesting approach… I thought Deferred was frowned upon, at least it is in browser code and Promise.defer was removed. I don't really care though it would be nice to avoid the boilerplate in this test.

The `fillPromiseResolvers` array now saves only the `resolve` function.

> Nit: loadNotUsernameSelectorRecipes

I've also converted all function names to camalCase.
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8472666&repo=fx-team
Flags: needinfo?(timdream)
Sigh. The document loads slower than add_task() on CI. I added a readyState check to guard it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8acd98600cd6
Flags: needinfo?(timdream)
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42917/diff/5-6/
https://hg.mozilla.org/mozilla-central/rev/ea109c3f6b3c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Approval Request Comment
[Feature/regressing bug #]: This seems like mostly a Facebook change or perhaps longstanding behaviour. They have an <input> before the password field that we detect as the username.
[User impact if declined]: Firefox will ask to save the new password as a new login with a garbage username (some number related to password strength) instead of asking to update the existing password. This also adds support for recipes to fix other sites where we detect the wrong username field.
[Describe test coverage new/current, TreeHerder]: New mochitests
[Risks and why]: Low risk change to allow recipes to affect the chosen password field.
[String/UUID change made/needed]: None
Attachment #8735698 - Flags: approval-mozilla-aurora?
Simona, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(simona.marcu)
Comment on attachment 8735698 [details]
MozReview Request: Bug 1243722 - Introduce notUsernameSelector in passwordmgr recipes, r=MattN

Fixes a regression when changing passwords on FB, Aurora47+
Attachment #8735698 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
MattN, Liz, should we consider uplifting this to Beta46? I know the bar is super high as we are in RC week but the user impact here seems severe enough to consider taking a fix this late.
Flags: needinfo?(lhenry)
Flags: needinfo?(MattN+bmo)
Yes. I just talked with Tim and he will check in a little while to make sure the patches apply correctly to beta in this and bug 1243729. We should uplift these to 46 for the RC2 build.
Flags: needinfo?(lhenry)
Attached patch Patch for m-bSplinter Review
Test changes don't apply to m-b.
Attachment #8743077 - Flags: review+
Attachment #8743077 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/releases/mozilla-beta/rev/f51382b3b967

This still needs to land on m-r but I'll make sure m-b doesn't burn first.
Flags: in-testsuite+
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #35)
> https://hg.mozilla.org/releases/mozilla-beta/rev/f51382b3b967
> 
> This still needs to land on m-r but I'll make sure m-b doesn't burn first.

Tomcat, could you land this on m-r please? It looks like it's not going to bounce.
Flags: needinfo?(cbook)
Simona - Would you mind testing this with a build from mozilla-release first? (Though it still would be nice to verify in nightly)
Flags: needinfo?(andrei.vaida)
Comment on attachment 8743077 [details] [diff] [review]
Patch for m-b

Belated approval. Thanks!
Attachment #8743077 - Flags: approval-mozilla-release+
Attachment #8743077 - Flags: approval-mozilla-beta?
Attachment #8743077 - Flags: approval-mozilla-beta+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #39)
> Simona - Would you mind testing this with a build from mozilla-release
> first? (Though it still would be nice to verify in nightly)

We'll have this tested as well during the sign off of 46.0-build4. Leaving the ni? in place until we follow up with test results.
Confirming that the correct doorhanger is now displayed for facebook and other websites when updating the password for an account.
Tested during the Fx 46.0-build4 (build ID 20160420024437) sign-off across platform.

Additional details are available in the actual sign off email sent for Fx46.0 RC2 (build 4), on r-d.
Flags: needinfo?(andrei.vaida)
Verified that the issue is no longer reproducible when following the steps from the Description - when the Facebook password is updated, the Update option and the same username are displayed in the password door hanger.

The verification was done on the latest Nightly 48.a1 (Build ID:20160420030213) and on the latest Aurora 47.0a2 (Build ID:20160421004015) on Windows 10(64bit), Mac OS X 10.9 and Ubuntu 15.04. 

I can still reproduce the Yahoo issue that was also mentioned in the Description - I logged Bug 1266381 to cover that problem.
Flags: needinfo?(simona.marcu)
You need to log in before you can comment on or make changes to this bug.