Justin Dolske [:Dolske] Reporter
	Description • 10 years ago

It's been suggested that we remove the "Show All" button from the password manager.

It has an unclear use case (specifically around showing ALL passwords, see note below), and is a perceived security risk from users who are surprised that someone can walk up, click it, and see everything. The latter is, IMO, not a highly compelling reason for removal, but that it repeatedly comes up and combined with an unclear usecase makes me support removing it.

Note that we _do_ need some mechanism to be able to see the password for at least individual logins. The intent is not to prevent being able to see actual passwords. Details TBD, I think the suggestion was to initially show masked passwords (i.e. "••••••••••"), make it editable upon focus, and show the actual password then.

The "show all" was originally added in the Wallet version of this UI on 2004-03-09, from bug 78754. Seems it was easier to just implement showing everything-or-nothing.

	Robert Kaiser
	Comment 1 • 10 years ago

For me, the primary use case that use the "show all" thing for is that sometimes I need passwords on a device/application not synched to my Firefox (e.g. Firefox OS or a desktop client for a service that I have a web login for) or autofilling a password doesn't work, but I do not remember the password, so I go into the password manager and want to reveal the password for this site. Right now, "show all" is the only way to do that, but for my usecase it would be enough if *in the password manager* (and that's the important piece) I could reveal a single password.

	Justin Dolske [:Dolske] Reporter
	Comment 2 • 10 years ago

Like I said, there's no intent to remove the ability to view individual passwords.

	Matthew N. [:MattN] Assignee
	Comment 4 • 9 years ago

Ryan has designed UX for this now. WIP patch (90% done but depending on a <tree> improvement) coming up.

	Matthew N. [:MattN] Assignee
	Comment 5 • 9 years ago

Bug 1121291 - Remove "Show Passwords" button from pwmgr and allow inline password editing.

	Matthew N. [:MattN] Assignee
	Comment 6 • 9 years ago

Screenshot in attachment 8645311 [details]

	Matthew N. [:MattN] Assignee
	Comment 7 • 9 years ago

Comment on attachment 8645314 [details]
MozReview Request: Bug 1121291 - Remove "Show Passwords" button from pwmgr and allow inline password editing. r=Dolske,rchtara

Bug 1121291 - Remove "Show Passwords" button from pwmgr and allow inline password editing. r=Dolske,rchtara

	Riadh Chtara[:rchtara]
	Comment 8 • 9 years ago

Comment on attachment 8645314 [details]
MozReview Request: Bug 1121291 - Remove "Show Passwords" button from pwmgr and allow inline password editing. r=Dolske,rchtara

https://reviewboard.mozilla.org/r/15515/#review17057

perfect, great job matt :-)

	Justin Dolske [:Dolske] Reporter
	Comment 9 • 9 years ago

Comment on attachment 8645314 [details]
MozReview Request: Bug 1121291 - Remove "Show Passwords" button from pwmgr and allow inline password editing. r=Dolske,rchtara

https://reviewboard.mozilla.org/r/15515/#review17619

r+ with a couple caveats:

1) Telemetry seems to show high usage of this, but I expect that's likely to be from people just wanting to view 1 password. Still, I think we need to do more work to make that a pleasant, discoverable experience -- I think many users will not realize they need to doubleclick/edit the password cell to see it, and it's a bit of an awkward interaction. Matt says y'all have discussed copying the Safari approach (show password on selected rows), or adding a hover-affordance to reveal it.

2) As a result, please don't land in the next few days. This doesn't feel quite ready to ride to Aurora in the pending uplift. (Uplift later on is fine if we think it's ready.)

::: toolkit/components/passwordmgr/content/passwordManager.js:134
(Diff revision 2)
> +    window.close();

Maybe return here? If we're closing the window, no point in running the rest of this code?

::: toolkit/components/passwordmgr/content/passwordManager.js
(Diff revision 2)
> -  Services.telemetry.getHistogramById("PWMGR_MANAGE_VISIBILITY_TOGGLED").add(showingPasswords);

Looking at this telemetry data, out of curiousity...

There are 155K samples on FF40 release, 87% for "show", 13% for "hide". As a rough comparison, PWMGR_NUM_SAVED_PASSWORDS has 19.4M samples (16% have no passwords, or 62% having 0-5 passwords). 155K is fairly small relative to 19M.

But... PWMGR_MANAGE_OPENED, which measures how many times this UI is shown, has 222K samples. 155K is very large relative to that. In fact, it implies ~60% of the time people open this dialog they clock Show Passwords.

Still, this isn't super surprising. I'd expect a major use case to be "see the password I previously saved", and the "Show Passwords" is the only way to do that. We don't really know from this data how many uses actually want to see _all_ their password.

One option would be to improve the UI/affordances to view a single password, and seem how much Show Password usage drops (although I expect some people will continue to use it from habit). Or we can just kill it and watch feedback.

::: toolkit/components/passwordmgr/content/passwordManager.js:307
(Diff revision 2)
> -  if (showingPasswords && aSignon.password &&
> +  if (Services.logins.isLoggedIn && aSignon.password &&

Kind of related to the other bug about maybe prompting for the windows login, but I wonder if we could simplify all the master-password checking stuff by just refusing to open the dialog / show any logins in the first place, if no MP is entered.

Not really relevant here, but something to consider.

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/e4e3a8b66db4

	Matthew N. [:MattN] Assignee
	Comment 11 • 9 years ago

https://reviewboard.mozilla.org/r/15515/#review17619

> Kind of related to the other bug about maybe prompting for the windows login, but I wonder if we could simplify all the master-password checking stuff by just refusing to open the dialog / show any logins in the first place, if no MP is entered.
> 
> Not really relevant here, but something to consider.

That's what my window.close addition was helping with. Before we would show a blank list and no we will never show it (from in-content prefs) or immediately close it in the windowed version (not ideal but arguably no worse than an empty tree).

	Carsten Book [:Tomcat]
	Comment 12 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/e4e3a8b66db4

	Stefan Plewako [:stef]
	Comment 13 • 9 years ago

This bug seems to add yet another column to password manager but do not adjust dialog size - for English this still works but for a locale with longer labels it doesn't anymore.

	Kamil Jozwiak [:kjozwiak]
	Comment 14 • 9 years ago

Thanks Stefan. I've created Bug # 1210938.

	Paul Silaghi, QA [:pauly]
	Comment 15 • 9 years ago

Verified fixed FF 44.0a1 (2015-10-14) Win 7

	Tony Mechelynck [:tonymec]
	Comment 16 • 9 years ago

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0 SeaMonkey/2.41a1" ID:20151017003002 c-c:fb83f130e2d554c0d0d007c6a5159a7e993e3e07 m-c:01e37977f8da2e1f8b9ce9b777e556ffb1437960 en-US

Verified on Linux x86_64 SeaMonkey by opening chrome://passwordmgr/content/passwordManager.xul in a tab. (The Data Manager, which "Tools → Password Manager → Manage Stored Passwords" opens, is still broken.)

	Majken Connor [:Kensie]
	Comment 17 • 9 years ago

Please let me know a better place to leave this feedback, but I really dislike the edit UX in place of view. If I can edit it I can change it, I don't want that, I just want to see it. It's also not necessarily intuitive that edit will let you see the password, not just over-write it (when you change your password anywhere else it never lets you see the old password, only submit a new one).

Is there a plan to add view only?

	Robert Kaiser
	Comment 18 • 9 years ago

I was confused by not having a "View" as well. If me and Kensie, who are well-versed in software, get confused by this, I imaging that people that are not that enthusiastic about technology will have an even harder time to grasp this.

	Justin Dolske [:Dolske] Reporter
	Comment 19 • 9 years ago

(In reply to Justin Dolske [:Dolske] from comment #9)

> r+ with a couple caveats:
> 
> 1) Telemetry seems to show high usage of this, but I expect that's likely to
> be from people just wanting to view 1 password. Still, I think we need to do
> more work to make that a pleasant, discoverable experience -- I think many
> users will not realize they need to doubleclick/edit the password cell to
> see it, and it's a bit of an awkward interaction. Matt says y'all have
> discussed copying the Safari approach (show password on selected rows), or
> adding a hover-affordance to reveal it.
> 
> 2) As a result, please don't land in the next few days. This doesn't feel
> quite ready to ride to Aurora in the pending uplift. (Uplift later on is
> fine if we think it's ready.)

Was there ever a final outcome on what the UI we want here is?

IIRC (and I may not!), it seemed that there was agreement on needing to improve this before shipping. Should we consider backing this out until we're ready to implement that?

	Matthew N. [:MattN] Assignee
	Comment 20 • 9 years ago

(In reply to Justin Dolske [:Dolske] from comment #19)
> Was there ever a final outcome on what the UI we want here is?

Yes, bug 1208145.

> IIRC (and I may not!), it seemed that there was agreement on needing to
> improve this before shipping. Should we consider backing this out until
> we're ready to implement that?

Yes, I agree and it's one of the bugs I mentioned earlier today that I want to finish soon. A straight backout won't work since the password column will be shown permanently for users who used a build with this fix due to XUL persistence being setup for the hidden attribute on that column.

Kairo and Majken, as comment 9 indicates, the plan was never to let this get to release as-is and I was tracking this but unfortunately got pulled off password manager abruptly so have less time for this :(

	Robert Kaiser
	Comment 21 • 9 years ago

(In reply to Matthew N. [:MattN] from comment #20)
> A straight backout won't work since the password column will
> be shown permanently for users who used a build with this fix due to XUL
> persistence being setup for the hidden attribute on that column.

For now, this looks to be in Developer Edition 44 (and Nightly) only, so I think if we back out right away, the amount of users affected is not too large - but given that this is sensitive, it would be good if we can take care of those users even there. Do you have a good way to do this? Also, can we get a bug filed and tracked for the 44 train to make sure this is done properly and this incomplete experience doesn't ship there?

	Matthew N. [:MattN] Assignee
	Comment 22 • 9 years ago

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #21)
> (In reply to Matthew N. [:MattN] from comment #20)
> > A straight backout won't work since the password column will
> > be shown permanently for users who used a build with this fix due to XUL
> > persistence being setup for the hidden attribute on that column.
> 
> For now, this looks to be in Developer Edition 44 (and Nightly) only, so I
> think if we back out right away, the amount of users affected is not too
> large - but given that this is sensitive, it would be good if we can take
> care of those users even there. Do you have a good way to do this? Also, can
> we get a bug filed and tracked for the 44 train to make sure this is done
> properly and this incomplete experience doesn't ship there?

I already said it's bug 1208145 that we should implement.

	Robert Kaiser
	Comment 23 • 9 years ago

(In reply to Matthew N. [:MattN] from comment #22)
> I already said it's bug 1208145 that we should implement.

That sounds like it needs an additional string, which can't be done in something that already is on Dev Edition or higher channel.

	captain
	Comment 24 • 9 years ago

Praise Jesus!  Thanks to these comments I too finally figured out that I just have to double click the password field to see the password.  Still, I'd like to be able to see ALL of the passwords at the same time TOO.  Having BOTH options would be ideal.  Maybe restore the Show Passwords button, but when hovering over it (or upon clicking it) a popup box could inform users that double-clicking on any individual password field will reveal ONLY that one password.

	Matth78
	Comment 25 • 9 years ago

Same here ! Got a hard time to figure how to see my previous saved passwords... Never would have guessed double clicking will reveal an individual password. But being in edit mode I think it's a bad thing, you could easily overwrote password without intending to.

Not being able to see multiple passwords is really a bad thing as most of the times that is what I want to do => recently profiles and passwords have been stolen on a forum I subscribed. I wanted to check if I didn't used same login/password on other sites but doing this is no longer possible.

	Tony Mechelynck [:tonymec]
	Comment 26 • 9 years ago

(In reply to captain from comment #24)
> Praise Jesus!  Thanks to these comments I too finally figured out that I
> just have to double click the password field to see the password.  Still,
> I'd like to be able to see ALL of the passwords at the same time TOO. 
> Having BOTH options would be ideal.  Maybe restore the Show Passwords
> button, but when hovering over it (or upon clicking it) a popup box could
> inform users that double-clicking on any individual password field will
> reveal ONLY that one password.

(In reply to Eriatile from comment #25)
> Same here ! Got a hard time to figure how to see my previous saved
> passwords... Never would have guessed double clicking will reveal an
> individual password. But being in edit mode I think it's a bad thing, you
> could easily overwrote password without intending to.
> 
> Not being able to see multiple passwords is really a bad thing as most of
> the times that is what I want to do => recently profiles and passwords have
> been stolen on a forum I subscribed. I wanted to check if I didn't used same
> login/password on other sites but doing this is no longer possible.

You can also right-click anywhere on that line, then click "Edit Password" in the context menu. This makes it more obvious that the password is in read-write mode.

In SeaMonkey which I use, the interface this bug is about is available as chrome://passwordmgr/content/passwordManager.xul which I have bookmarked, and the "Passwords" tab of the Data Manager still has the [Show Passwords] button and its are-you-sure dialog. That Data Manager used to be available as an add-on for Firefox bu apparently it has been retired.