Closed Bug 1318194 Opened 3 years ago Closed 3 years ago

Can't autocomplete usernames on insecure pages.

Categories

(Toolkit :: Password Manager, defect, P1)

52 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox51 --- wontfix
firefox52 --- verified
firefox53 --- verified

People

(Reporter: tanvi, Assigned: timdream)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
(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?
I've found the root cause and the patch incoming.
Assignee: nobody → timdream
Blocks: 1289913
Status: NEW → ASSIGNED
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
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.
Attachment #8811562 - Flags: feedback?(MattN+bmo)
Attachment #8811562 - Flags: feedback?(tanvi)
(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.
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 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+
(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)
(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.
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
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
https://hg.mozilla.org/mozilla-central/rev/b2e6a3c250e1
https://hg.mozilla.org/mozilla-central/rev/e4cbefe6aefe
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Tim, can you please request uplift to 52?
Flags: needinfo?(timdream)
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?
Attachment #8812156 - Flags: approval-mozilla-aurora?
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 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+
Verified as fixed on 53.0a1/20170116030326 and 52.0a2/20170112004017.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.