Closed
Bug 1318194
Opened 8 years ago
Closed 8 years ago
Can't autocomplete usernames on insecure pages.
Categories
(Toolkit :: Password Manager, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla53
People
(Reporter: tanvi, Assigned: timdream)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
(deleted),
text/x-review-board-request
|
MattN
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details |
(deleted),
text/x-review-board-request
|
jcristau
:
approval-mozilla-aurora+
|
Details |
Go to about:config and set:
security.insecure_field_warning.contextual.enabled to true
signon.autofillForms.http to true
Go to an http:// page where you have one or more saved logins. (For example, you could go to http://mysqueezebox.com/user/login and enter a fake username/password. Hit submit, and select for the password manager to save the credential).
Go to the login page again. Focus on the username or password field. You see the insecure password warning with a Learn More link. But you don't not see your username underneath the insecure warning. You cannot select your username and fill it.
Expected behavior: You should see a username underneath the insecure warning in both the username and password fields.
This happens when you have one or more saved logins.
I am using Nightly 53.0a1 from 2016-11-16 on OSX.
Assignee | ||
Comment 1•8 years ago
|
||
(In reply to Tanvi Vyas - behind on bugzilla [:tanvi] from comment #0)
> Go to about:config and set:
> security.insecure_field_warning.contextual.enabled to true
> signon.autofillForms.http to true
I can only reproduce this bug when I flip this pref to "false" (it's default to true currently). Is this a typo?
Assignee | ||
Comment 2•8 years ago
|
||
I've found the root cause and the patch incoming.
Comment 3•8 years ago
|
||
Hi Tanvi, I can not reproduce this bug based on the description. Please see the video link [1]. Do you think I miss any steps?
[1] https://youtu.be/dNxGgBESZks
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
Talked to Sean offline. To be clear, my patch was based on comment 1, and the rationale that the user has to have access to saved login from the dropdown, regardless of the pref. Please disregard my patch if either is the case.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8811562 -
Flags: feedback?(MattN+bmo)
Assignee | ||
Updated•8 years ago
|
Attachment #8811562 -
Flags: feedback?(tanvi)
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1)
> I can only reproduce this bug when I flip this pref to "false" (it's default
> to true currently). Is this a typo?
Tanvi said yes over the e-mail.
Comment hidden (mozreview-request) |
Comment 9•8 years ago
|
||
MattN and I are discussed this before at bug 1289913 comment 47.
> ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1256
> (Diff revisions 3 - 7)
> > + if (!prefShowInsecureAutoFillForms) {
> > + matchingLogins = [];
> > + }
>
> Don't you only want to do this if `!prefShowInsecureAutoFillForms &&
> !isSecure`?
>
> With what you have I think you're preventing autocomplete suggestions for
> both secure and insecure.
>
Hi Tanvi,
Hi Tanvi, to me this one is more like a UI decision instead of a bug. In the RST we already set the |signon.autofillForms.http| to |false|, which means we shouldn't display form input history under http connection. So to me it makes sense to not show the username underneath the insecure warning when the |signon.autofillForms.http| is set to |false|
Do you know who is the UX for this feature, maybe we should ni UX to have their comments as well.
Flags: needinfo?(tanvi)
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8811562 [details]
Bug 1318194 - Always lists matching logins regardless of page security,
https://reviewboard.mozilla.org/r/93636/#review93930
I thought that Sean's patch before was using an empty array to replace the array with only the warning BEFORE adding the login. I didn't notice that it wasn't providing matching logins in this case. The pref is only about "autofill", not autocomplete so the plan was always for autocomplete to continue to work and that was part of the original UX discussion.
::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1297
(Diff revision 3)
> seen.add(login.username);
> }
> return duplicates;
> }
>
> - let currentMatchingLogins = (!LoginHelper.insecureAutofill && !this._isSecure) ?
> + let currentMatchingLogins = this._matchingLogins;
Please get rid of this now unnecesarry redirection (inline `this._matchingLogins`)
Attachment #8811562 -
Flags: review?(MattN+bmo) → review+
Reporter | ||
Comment 12•8 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1)
> (In reply to Tanvi Vyas - behind on bugzilla [:tanvi] from comment #0)
> > Go to about:config and set:
> > security.insecure_field_warning.contextual.enabled to true
> > signon.autofillForms.http to true
>
> I can only reproduce this bug when I flip this pref to "false" (it's default
> to true currently). Is this a typo?
signon.autoFillForms.http to false. Sorry for the typo.
Flags: needinfo?(tanvi)
Reporter | ||
Comment 13•8 years ago
|
||
(In reply to Vance Chen [:vchen][skype:vance.lucida][vchen@mozilla.com] from comment #10)
> Hi Tanvi,
>
> Hi Tanvi, to me this one is more like a UI decision instead of a bug. In the
> RST we already set the |signon.autofillForms.http| to |false|, which means
> we shouldn't display form input history under http connection. So to me it
> makes sense to not show the username underneath the insecure warning when
> the |signon.autofillForms.http| is set to |false|
>
> Do you know who is the UX for this feature, maybe we should ni UX to have
> their comments as well.
The UX design by Aislinn and Phillip is here - https://bug1217150.bmoattachments.org/attachment.cgi?id=8791926
We don't want to autofill the password. But we want to give the user the option to autocomplete it (i.e. select it and fill it with user interaction) once they have seen the warning. Hence, the usernames (whether there is just one saved, or more than one) should appear under the warning and a user should be able to select them.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 15•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2e6a3c250e1
Always lists matching logins regardless of page security, r=MattN
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment 17•8 years ago
|
||
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/e4cbefe6aefe
Always lists matching logins regardless of page security. Fix tests. r=bustage-fix
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b2e6a3c250e1
https://hg.mozilla.org/mozilla-central/rev/e4cbefe6aefe
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 19•8 years ago
|
||
Updated•8 years ago
|
status-firefox51:
--- → wontfix
status-firefox52:
--- → affected
Reporter | ||
Comment 20•8 years ago
|
||
Tim, can you please request uplift to 52?
Flags: needinfo?(timdream)
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8811562 [details]
Bug 1318194 - Always lists matching logins regardless of page security,
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1289913
[User impact if declined]: Incorrect behavior of bug 1289913 when enabled on Fx52
[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]: See comment 0
[List of other uplifts needed for the feature/fix]: See dependency in bug 1289913
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a local local change and only affect the new feature in question.
[String changes made/needed]:
Flags: needinfo?(timdream)
Attachment #8811562 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8812156 -
Flags: approval-mozilla-aurora?
Comment 22•8 years ago
|
||
Comment on attachment 8811562 [details]
Bug 1318194 - Always lists matching logins regardless of page security,
fix for login form autocomplete, take in aurora52
Attachment #8811562 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 23•8 years ago
|
||
Comment on attachment 8812156 [details]
Bug 1318194 - Follow up, fix unit test
followup tests fix, for aurora52
Attachment #8812156 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•8 years ago
|
||
bugherder uplift |
Comment 25•8 years ago
|
||
Verified as fixed on 53.0a1/20170116030326 and 52.0a2/20170112004017.
You need to log in
before you can comment on or make changes to this bug.
Description
•