Alice0775 White Reporter
	Description • 7 years ago

Steps To Reproduce:
1. Set Master Password properly
2. Open https://accounts.firefox.com/signin?context=web
3. Click [cancel] when Master Password dialog pops up
   --- no bug, it works as expected
4. Click password field
   --- Master Password dialog pops up, it maybe expected behavior.
5. Click  [cancel] to close the dialog
   --- Master Password dialog pops up again. --- Bug

Actual Results:
Master Password dialog pops up again.
I can not do anything operate Firefox.

Expected Results:
Master Password dialog should disappear.

	Alice0775 White Reporter
	Comment 1 • 7 years ago

This is since Firefox52. 
Firefox51 is not affected.

	Alice0775 White Reporter
	Comment 2 • 7 years ago

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ed8b016100297fdd10f45f02ebf0202b91559a23&tochange=2f89807ca30c474bed545f10c9e52a434f297a71

Regressed by:
2f89807ca30c	Matthew Noorenberghe — Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked. r=mconley

	Johann Hofmann [:johannh] Assignee
	Comment 5 • 7 years ago

Matt, I'm still sorting out how to best write a test for this (suggestions welcome). Feel free to review the first part. I initially wanted to ask for feedback instead of review, but this seems to work quite well. So please let me know if you think there's a flaw in the general approach.

	Matthew N. [:MattN]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8849530 [details]
Bug 1348791 - Add a timeout to master password prompt when searching logins.

https://reviewboard.mozilla.org/r/122324/#review126550

::: modules/libpref/init/all.js:4426
(Diff revision 1)
>  pref("signon.formlessCapture.enabled",      true);
>  pref("signon.storeWhenAutocompleteOff",     true);
>  pref("signon.debug",                        false);
>  pref("signon.recipes.path",                 "chrome://passwordmgr/content/recipes.json");
>  pref("signon.schemeUpgrades",               false);
> +pref("signon.masterPasswordReprompt.timeout_ms", 1800000);

Isn't searchLogins used for many different UIs? And is the timeout applying to all of them? If so, this timeout is way too high as I don't think there's a way for a user to change their mind to decide to login to MP. Ideally it would be just big enough so we don't cause what appears to be an infinite loop.

For the purposes of 52 we really just need to fix autocomplete I think, does that make a fix easier/smaller?

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:61
(Diff revision 1)
> +      if (e.result == Cr.NS_ERROR_ABORT) {
> +        log("User cancelled master password prompt.");

Did you consider using the observer notification instead? It may give you more flexibility. I was thinking that the autocompleteSearch code would check `isLoggedIn` and not touch pwmgr storage if the prompt was recently cancelled (checking a timestamp like you have). What do you think about that?

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 8 • 7 years ago

We could still take a fix for beta 53 and esr52 but too late for 52 release.

	Johann Hofmann [:johannh] Assignee
	Comment 10 • 7 years ago

(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #6)
> Isn't searchLogins used for many different UIs? And is the timeout applying
> to all of them? If so, this timeout is way too high as I don't think there's
> a way for a user to change their mind to decide to login to MP. Ideally it
> would be just big enough so we don't cause what appears to be an infinite
> loop.
>
> For the purposes of 52 we really just need to fix autocomplete I think, does
> that make a fix easier/smaller?

You're right, I think we only need to fix autocomplete. I updated that. I changed the timeout to be 15 minutes, but I don't think we should make it much shorter. I wasn't able to consistently break the infinite loop, it's occurring on all sorts of things like focusing or clicking the password field or focusing the window. Ideally someone would look into the different ways satchel is calling the autocomplete because something must be broken over there.

> 
> ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:61
> (Diff revision 1)
> > +      if (e.result == Cr.NS_ERROR_ABORT) {
> > +        log("User cancelled master password prompt.");
> 
> Did you consider using the observer notification instead? It may give you
> more flexibility. I was thinking that the autocompleteSearch code would
> check `isLoggedIn` and not touch pwmgr storage if the prompt was recently
> cancelled (checking a timestamp like you have). What do you think about that?

That didn't fire when I tried it, so I kept it like this :/

	Matthew N. [:MattN]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8849530 [details]
Bug 1348791 - Add a timeout to master password prompt when searching logins.

https://reviewboard.mozilla.org/r/122324/#review131782

Sorry Johann but I think this has some problems.

Also there really should be a test for this if we want to uplift.

::: modules/libpref/init/all.js:4409
(Diff revision 2)
>  pref("signon.formlessCapture.enabled",      true);
>  pref("signon.storeWhenAutocompleteOff",     true);
>  pref("signon.debug",                        false);
>  pref("signon.recipes.path",                 "chrome://passwordmgr/content/recipes.json");
>  pref("signon.schemeUpgrades",               false);
> +pref("signon.masterPasswordReprompt.timeout_ms", 900000); // 15 Minutes

Add to the comment stating it's for MP caused by autocomplete only for now. If it was general purpose I think it should be much lower but for autocomplete only the user can simply reload the page if they want to force the MP dialog to come back.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:40
(Diff revision 2)
>     * @deprecated
>     */
>    _recipeManager: null,
>  
> +  // Tracks the last unsuccessful master password attempt.
> +  _lastMPLoginAttempt: Math.NEGATIVE_INFINITY,

`_lastMPLoginCancelled`?

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:42
(Diff revision 2)
>    _recipeManager: null,
>  
> +  // Tracks the last unsuccessful master password attempt.
> +  _lastMPLoginAttempt: Math.NEGATIVE_INFINITY,
> +
> +  _searchLogins(formSubmitURL, hostname) {

This method needs a better name e.g. `_searchAndDedupeLogins`

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:61
(Diff revision 2)
> +    // Dedupe so the length checks below still make sense with scheme upgrades.
> +    // Below here we have one login per hostPort + action + username with the
> +    // matching scheme being preferred.

This comment belongs with the original code from line 320

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:258
(Diff revision 2)
> +    let timeDiff = Date.now() - this._lastMPLoginAttempt;
> +    if (!Services.logins.isLoggedIn && timeDiff < this._repromptTimeout) {

Please avoid accessing `Date.now` (which can be quite slow) in this somewhat hot code path (running on many keystrokes in a username+password field) if `isLoggedIn` is true. I think you can inline it and wrap the conditions in two lines.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:260
(Diff revision 2)
> +      log("Not searching logins for autocomplete since the master password " +
> +          `prompt was last cancelled ${timeDiff / 1000} seconds ago.`);

Did you mean to also `Math.round`? That would reduce the spamminess of the logging since the console is supposed to dedupe identical consecutive log messages with a counter.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:260
(Diff revision 2)
> +      log("Not searching logins for autocomplete since the master password " +
> +          `prompt was last cancelled ${timeDiff / 1000} seconds ago.`);
> +      return;
> +    }

Are you sure it's fine to return here? I think we will leak the request in LoginManagerContent if we don't respond. See https://dxr.mozilla.org/mozilla-central/rev/f914d40a48009c5acd1093e9939cc0ec035696dd/toolkit/components/passwordmgr/LoginManagerContent.jsm#208-209,214,216,218,233,252

I think we need to respond with a message for the requestID with an empty array and double check this won't break the insecure password warning (I think it already handles adding the warning row on an empty array but not 100% sure).

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:490
(Diff revision 2)
>                                   .CustomEvent("InsecureLoginFormsStateChange"));
>    },
>  };
> +
> +XPCOMUtils.defineLazyPreferenceGetter(LoginManagerParent, "_repromptTimeout",
> +                                      "signon.masterPasswordReprompt.timeout_ms", 1800 * 1000);

Make this 900000 to match the default.

	Johann Hofmann [:johannh] Assignee
	Comment 15 • 7 years ago
mozreview-review-reply

Comment on attachment 8849530 [details]
Bug 1348791 - Add a timeout to master password prompt when searching logins.

https://reviewboard.mozilla.org/r/122324/#review131782

> Please avoid accessing `Date.now` (which can be quite slow) in this somewhat hot code path (running on many keystrokes in a username+password field) if `isLoggedIn` is true. I think you can inline it and wrap the conditions in two lines.

Good point, though I moved isLoggedIn to a separate if-condition to be able to reuse timeDiff

> Are you sure it's fine to return here? I think we will leak the request in LoginManagerContent if we don't respond. See https://dxr.mozilla.org/mozilla-central/rev/f914d40a48009c5acd1093e9939cc0ec035696dd/toolkit/components/passwordmgr/LoginManagerContent.jsm#208-209,214,216,218,233,252
> 
> I think we need to respond with a message for the requestID with an empty array and double check this won't break the insecure password warning (I think it already handles adding the warning row on an empty array but not 100% sure).

Oh, good catch. I didn't even know this request caching existed.

	Matthew N. [:MattN]
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8849530 [details]
Bug 1348791 - Add a timeout to master password prompt when searching logins.

https://reviewboard.mozilla.org/r/122324/#review139876

Thanks for pushing this over the finish line!

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:39
(Diff revision 3)
> +  // Tracks the last time the user cancelled the master password prompt,
> +  // to avoid spamming master password prompts on autocomplete searches.
> +  _lastMPLoginCancelled: Math.NEGATIVE_INFINITY,

So one thing this doesn't take into account is Sync requesting MP login… I guess that can be done in a follow-up, if necessary since this will still improve the situation.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:43
(Diff revision 3)
>  
> +  // Tracks the last time the user cancelled the master password prompt,
> +  // to avoid spamming master password prompts on autocomplete searches.
> +  _lastMPLoginCancelled: Math.NEGATIVE_INFINITY,
> +
> +  _searchAndDedupeLogins(formSubmitURL, hostname) {

Nit: I'm trying to phase out the `formSubmitURL` and `hostname` names since they are inaccurate so please use `actionOrigin` and `formOrigin` instead.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:43
(Diff revision 3)
> +  _searchAndDedupeLogins(formSubmitURL, hostname) {
> +    let logins;

Please re-order the two arguments since `hostname` is the important one and `formSubmitURL` may be removed one day. (It's been talked about many times)

	Matthew N. [:MattN]
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8865274 [details]
Bug 1348791 - Add a test for autocomplete master password timeout.

https://reviewboard.mozilla.org/r/136916/#review139882

::: toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js:15
(Diff revision 1)
> +    is(dialog.args.title, "Password Required");
> +    dialog.ui.button1.click();
> +  });
> +}
> +
> +add_task(function*() {

Nit: I think it makes sense to always name test tasks since the name shows in output and makes it easier to see what's going on. It would be good to have to test name and/or a comment explaining what this test is trying to verify: Prevent a MP prompt for autocomplete after being cancelled.

::: toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js:24
(Diff revision 1)
> +  // Add a master password.
> +  let pk11db = Cc["@mozilla.org/security/pk11tokendb;1"].getService(Ci.nsIPK11TokenDB);
> +  let token = pk11db.getInternalKeyToken();
> +  token.changePassword("", "masterpass");

This can be `LoginTestUtils.masterPassword.enable()`

::: toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js:27
(Diff revision 1)
> +  token.changePassword("", "masterpass");
> +

You should probably have a registerCleanupFunction with `LoginTestUtils.masterPassword.disable();` to avoid a MP interfering with future tests?

	Johann Hofmann [:johannh] Assignee
	Comment 20 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f84c80479f045036637980fd91ceff88c80b0c0

	Johann Hofmann [:johannh] Assignee
	Comment 22 • 7 years ago
mozreview-review-reply

Comment on attachment 8865274 [details]
Bug 1348791 - Add a test for autocomplete master password timeout.

https://reviewboard.mozilla.org/r/136916/#review139882

> Nit: I think it makes sense to always name test tasks since the name shows in output and makes it easier to see what's going on. It would be good to have to test name and/or a comment explaining what this test is trying to verify: Prevent a MP prompt for autocomplete after being cancelled.

I agree! Just forgot about it :)

> You should probably have a registerCleanupFunction with `LoginTestUtils.masterPassword.disable();` to avoid a MP interfering with future tests?

Good catch!

	Johann Hofmann [:johannh] Assignee
	Comment 25 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c7aa29301e06816a7d9fcf91a7511d46418bb1f

	Pulsebot
	Comment 26 • 7 years ago

Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9576d7dc7efc
Add a timeout to master password prompt when searching logins. r=MattN
https://hg.mozilla.org/integration/autoland/rev/dd246ec28146
Add a test for autocomplete master password timeout. r=MattN

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 27 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9576d7dc7efc
https://hg.mozilla.org/mozilla-central/rev/dd246ec28146

	Nathan Froyd [:froydnj]
	Comment 29 • 7 years ago

Please request uplift on this when you get a chance.

	Nathan Froyd [:froydnj]
	Comment 30 • 7 years ago

This makes using the master password kind of annoying.

	Johann Hofmann [:johannh] Assignee
	Comment 31 • 7 years ago

Yup, we should have enough Nightly bake time now. I'll make the request.

	Johann Hofmann [:johannh] Assignee
	Comment 32 • 7 years ago

I don't think tracking 55 makes sense though, given that it's fixed already.

	Johann Hofmann [:johannh] Assignee
	Comment 33 • 7 years ago

Comment on attachment 8849530 [details]
Bug 1348791 - Add a timeout to master password prompt when searching logins.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1334026
[User impact if declined]: Users basically can not cancel the master password prompt on certain pages, they have to enter the right one, probably causing some of them disable master password completely.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:
- Use a master password
- Go to e.g. http://http-login.badssl.com/
- Enter some credentials and submit the form
- Save the credentials in the password manager
- Restart the browser, don't enter the master password on startup
- Go to e.g. http://http-login.badssl.com/
- You should only be prompted for your master password once until reloading the page, even if you enter things in the input fields
[List of other uplifts needed for the feature/fix]: There is an accompanying test for this in attachment 8865274 [details]. Might want to land that, too.
[Is the change risky?]: Medium
[Why is the change risky/not risky?]: We touch password manager frontend code, if something goes wrong we could potentially break login autofill but it's all covered with automated tests so we should be fine.
[String changes made/needed]: None

	Gerry Chang [:gchang]
	Comment 34 • 7 years ago

Track 54+ as regression.

Hi Alice,
Can you help check if this issue is fixed in the latest Nightly?

	Alice0775 White Reporter
	Comment 35 • 7 years ago

I verified that the problem was fixed in [1]current Nightly55.

[1]
https://hg.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 ID:20170523030206

	Gerry Chang [:gchang]
	Comment 36 • 7 years ago

Comment on attachment 8849530 [details]
Bug 1348791 - Add a timeout to master password prompt when searching logins.

Fix a regression related to Master Password dialog and was verified. Beta54+. Should be in 54 beta 11.

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/7d5ea2d3503b
https://hg.mozilla.org/releases/mozilla-beta/rev/fd12d3d5b412

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 38 • 7 years ago

I'd like to wait a few more days to catch unexpected regressions before uplifting this to ESR52

	Bogdan Maris, Desktop QA
	Comment 39 • 7 years ago

We reproduced the initial issue using affected builds (Fx 53.0.2 and Fx 54beta8), verified that the issue is fixed using Firefox 54 beta 11 build 2 across platforms (Windows 7 64bit, Windows 10 32bit, macOS 10.12.5 and Ubuntu 16.04 64bit).

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 41 • 7 years ago

Comment on attachment 8849530 [details]
Bug 1348791 - Add a timeout to master password prompt when searching logins.

This is a core scenario, fix has been verified on Nightly55 and Beta54, ESR52+

	Ryan VanderMeulen [:RyanVM]
	Comment 42 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/4a52b2cfc49c
https://hg.mozilla.org/releases/mozilla-esr52/rev/27083ff87fb4