Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6770012:
Handle the PasswordManagerAllowShowPasswords preference in the options webui. (Closed)
|
Created:
9 years, 9 months ago by Joao da Silva Modified:
9 years, 7 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandle the PasswordManagerAllowShowPasswords preference in the options webui.
BUG=59773
TEST=Set the policy, and the password manager won't show passwords.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81229
Patch Set 1 #
Total comments: 16
Patch Set 2 : Keep password column, show "IT-admin" banner #
Total comments: 4
Patch Set 3 : Presentation details moved to JS, cleanups, rebased #
Total comments: 2
Patch Set 4 : Implemented last comments #
Total comments: 2
Patch Set 5 : Moved obfuscated password to js #
Total comments: 3
Patch Set 6 : Nit. #Patch Set 7 : Rebase. #
Messages
Total messages: 24 (0 generated)
This preference used to be in place before, but the new webui PasswordManager wasn't checking it. When showing passwords is disabled, the password data is not sent at all to the webui page, and the "show" button is not enabled either. mnissler: you made the previous implementation of this policy, how does this look? stuartmorgan: you're one of the owners of ui/webui/options, does this look ok to you? Thanks for reviewing!
jhawkins, FYI. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... File chrome/browser/resources/options/password_manager_list.js (right): http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:51: passwordInputDiv.className = 'password'; Having a huge blank space on the right hand side of the table seems unlikely to be the desired UI here. Seems like we should either change the column widths, or the password should be displayed as ****** but not displayable or editable. http://codereview.chromium.org/6770012/diff/1/chrome/browser/ui/webui/options... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/1/chrome/browser/ui/webui/options... chrome/browser/ui/webui/options/password_manager_handler.cc:104: const NotificationSource& source, Alignment is wrong on the continuation lines. http://codereview.chromium.org/6770012/diff/1/chrome/browser/ui/webui/options... chrome/browser/ui/webui/options/password_manager_handler.cc:111: return; Why the early return?
http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... File chrome/browser/resources/options/password_manager_list.js (right): http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:48: if (this.showPassword) { if (!this.showPassword) return; ^ Huge indented blocks are bad for readability. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:51: passwordInputDiv.className = 'password'; On 2011/03/29 15:24:58, stuartmorgan wrote: > Having a huge blank space on the right hand side of the table seems unlikely to > be the desired UI here. Seems like we should either change the column widths, or > the password should be displayed as ****** but not displayable or editable. Agreed. Are we showing some sort of UI to the user to let them know why they aren't seeing the password? If a user is familiar with the UI from another machine where they can see passwords, then they arrive at this situation, it would seem jarring and broken w/out notification. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:76: return; nit: Blank line after if block. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:214: showPasswords: true, Document. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:214: showPasswords: true, s/showPasswords/showPasswords_/ since it's presumably private.
Thanks for your comments, I've contacted UI to get input on how to do this. See below for some comments. On 2011/03/29 17:37:23, James Hawkins wrote: > http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... > File chrome/browser/resources/options/password_manager_list.js (right): > > http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... > chrome/browser/resources/options/password_manager_list.js:48: if > (this.showPassword) { > if (!this.showPassword) > return; > > ^ Huge indented blocks are bad for readability. > > http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... > chrome/browser/resources/options/password_manager_list.js:51: > passwordInputDiv.className = 'password'; > On 2011/03/29 15:24:58, stuartmorgan wrote: > > Having a huge blank space on the right hand side of the table seems unlikely > to > > be the desired UI here. Seems like we should either change the column widths, > or > > the password should be displayed as ****** but not displayable or editable. > > Agreed. Are we showing some sort of UI to the user to let them know why they > aren't seeing the password? If a user is familiar with the UI from another > machine where they can see passwords, then they arrive at this situation, it > would seem jarring and broken w/out notification. There is an infobar at the top of the preferences page when some settings are configured by the domain administrator, informing the user that some of his settings were set by the administrator. The interface could also be left as it is, and when the user clicks "show" a popup is displayed informing him that the administrator has disabled showing passwords. Let's see what the UI people think about this. > > http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... > chrome/browser/resources/options/password_manager_list.js:76: return; > nit: Blank line after if block. > > http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... > chrome/browser/resources/options/password_manager_list.js:214: showPasswords: > true, > Document. > > http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... > chrome/browser/resources/options/password_manager_list.js:214: showPasswords: > true, > s/showPasswords/showPasswords_/ since it's presumably private.
On 2011/03/29 22:10:21, joaodasilva wrote: > The interface could also be left as it is, and when the user clicks "show" a > popup is displayed informing him that the administrator has disabled showing > passwords. Let's see what the UI people think about this. We should definitely not do that. Providing a control that doesn't work just to tell the user it doesn't work would maximize frustration. If UI tells you to do that, I'll go argue with them :) We handle other managed prefs by disabling the control and showing the info bar, not by putting an onclick handler that shows a dialog. We should do something similar here (which is why I suggested ***** without the show button)
I've asked Alex (@ainslie) from UI about this change and sent some mocks, and he supported it. The password input field is now kept, to keep the width consistent and prevent the huge blank space. The "show" button is not shown when the policy is enforced. The password length is fixed at 8 chars when hidden. The rationale is that the IT-admin uses this policy to prevent a user from snooping another user's passwords, and the real password length should also be concealed. A banner is usually shown to the user when the IT admin has modified some preferences. However this only shows up when an <input> element is managed, and there was none tracking this particular policy. I've added a hidden <input> to track it, so that users see the banner and get some clue on why the "show" button isn't there anymore. See the bug for screenshots of how it looks like. Please review! :-) http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... File chrome/browser/resources/options/password_manager_list.js (right): http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:48: if (this.showPassword) { On 2011/03/29 17:37:23, James Hawkins wrote: > if (!this.showPassword) > return; > > ^ Huge indented blocks are bad for readability. This has moved a couple of lines down. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:51: passwordInputDiv.className = 'password'; On 2011/03/29 15:24:58, stuartmorgan wrote: > Having a huge blank space on the right hand side of the table seems unlikely to > be the desired UI here. Seems like we should either change the column widths, or > the password should be displayed as ****** but not displayable or editable. Fixed. The password input div is kept, but the "show" button is not included when passwords are hidden. The password length is also fixed at 8 chars when hidden. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:51: passwordInputDiv.className = 'password'; On 2011/03/29 17:37:23, James Hawkins wrote: > On 2011/03/29 15:24:58, stuartmorgan wrote: > > Having a huge blank space on the right hand side of the table seems unlikely > to > > be the desired UI here. Seems like we should either change the column widths, > or > > the password should be displayed as ****** but not displayable or editable. > > Agreed. Are we showing some sort of UI to the user to let them know why they > aren't seeing the password? If a user is familiar with the UI from another > machine where they can see passwords, then they arrive at this situation, it > would seem jarring and broken w/out notification. When a preference is managed by policy there is a banner at the top of the page telling the user that the IT administrator has changed some preferences. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:76: return; On 2011/03/29 17:37:23, James Hawkins wrote: > nit: Blank line after if block. Done. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:214: showPasswords: true, On 2011/03/29 17:37:23, James Hawkins wrote: > Document. Done. http://codereview.chromium.org/6770012/diff/1/chrome/browser/resources/option... chrome/browser/resources/options/password_manager_list.js:214: showPasswords: true, On 2011/03/29 17:37:23, James Hawkins wrote: > s/showPasswords/showPasswords_/ since it's presumably private. It's not private. Should a getter and setter be used instead? http://codereview.chromium.org/6770012/diff/1/chrome/browser/ui/webui/options... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/1/chrome/browser/ui/webui/options... chrome/browser/ui/webui/options/password_manager_handler.cc:104: const NotificationSource& source, On 2011/03/29 15:24:58, stuartmorgan wrote: > Alignment is wrong on the continuation lines. Done. http://codereview.chromium.org/6770012/diff/1/chrome/browser/ui/webui/options... chrome/browser/ui/webui/options/password_manager_handler.cc:111: return; On 2011/03/29 15:24:58, stuartmorgan wrote: > Why the early return? No reason for it, removed.
http://codereview.chromium.org/6770012/diff/5001/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/5001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/password_manager_handler.cc:109: // Call UpdatePasswordLists() to update the password data model too. Not sure why this comment is here; the call seems self explanatory. http://codereview.chromium.org/6770012/diff/5001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/password_manager_handler.cc:163: string16 hidden_password(ASCIIToUTF16("********")); Why send this to JS at all, vs an empty string? JS is getting the show_value flag, so it can add the dummy password there. Since it's just for presentation, that seems like a better place for it.
stuartmorgan: thanks for reviewing, does it look better now? Mattias, James: what do you think of this patch? http://codereview.chromium.org/6770012/diff/5001/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/5001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/password_manager_handler.cc:109: // Call UpdatePasswordLists() to update the password data model too. On 2011/04/01 22:22:16, stuartmorgan wrote: > Not sure why this comment is here; the call seems self explanatory. Done. http://codereview.chromium.org/6770012/diff/5001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/password_manager_handler.cc:163: string16 hidden_password(ASCIIToUTF16("********")); On 2011/04/01 22:22:16, stuartmorgan wrote: > Why send this to JS at all, vs an empty string? JS is getting the show_value > flag, so it can add the dummy password there. Since it's just for presentation, > that seems like a better place for it. Done.
http://codereview.chromium.org/6770012/diff/10001/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/10001/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/password_manager_handler.cc:176: entries, *show_value); The password list data is unrelated to the preference value that determines whether we can show passwords or not. Handling this value should happen solely on the JS side.
Thanks for reviewing! Please consider the comment inline and let me know what you think. http://codereview.chromium.org/6770012/diff/10001/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/10001/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/password_manager_handler.cc:176: entries, *show_value); On 2011/04/02 23:42:10, James Hawkins wrote: > The password list data is unrelated to the preference value that determines > whether we can show passwords or not. Handling this value should happen solely > on the JS side. I'm concerned that a determined attacker could use the DevTools or similar to inspect the password. Try this on your browser: open the password manager, and paste into the omnibar: javascript:alert(window.PasswordManager.getInstance().savedPasswordsList_.dataModel.item(0)[2]) So I strongly believe the password should not be sent at all to JS in this case. wdyt?
LGTM
Stuart and I talked off-list and decided that my recommendation above should be followed: * The view should no as little as possible about the inability to show passwords: remove the parameter. * Send in '***' from the backend. * Disable the 'Show' button in the view based on the value of the preference.
Please have another look. The parameter has been removed, and the JS code is checking the preference to determine whether to include the "show" button.
On 2011/04/03 19:27:17, joaodasilva wrote: > I'm concerned that a determined attacker could use the DevTools or similar to > inspect the password. A determined hacker can load each page, let the password autofill, and read the password using omnibox JS or the dev tools. Preventing people from showing passwords in the options doesn't provide a security benefit, which is why we've never provided an end-user option to password protect the display in options.
On Tue, Apr 5, 2011 at 5:28 PM, <stuartmorgan@chromium.org> wrote: > A determined hacker can load each page, let the password autofill, and read > the > password using omnibox JS or the dev tools. Yep, just verified it and now I'm worried, this reminds me of the CSS :visited leak. There is a policy to disable autofill too, if admins want to prevent this behavior. > Preventing people from showing passwords in the options doesn't provide a > security benefit, which is why we've never provided an end-user option to > password protect the display in options. Admins have asked for this policy, and the bug has some stars. Unfortunately there are other attack vectors, but I think it's sensible to prevent this particular one. Thanks for the input, does the CL still LGTY?
On 2011/04/05 15:44:53, joaodasilva wrote: > Yep, just verified it and now I'm worried, this reminds me of the CSS > :visited leak. I don't see why; that was a leak of info to sites. A site has no reason to steal its *own* password. This has been discussed extensively (e.g., in the Mozilla bug tracker, years ago).
What I had in mind is the user's address, credit cards, telephone, etc. I don't know if this can be inspected from javascript, has it been discussed already? On Tue, Apr 5, 2011 at 5:48 PM, <stuartmorgan@chromium.org> wrote: > On 2011/04/05 15:44:53, joaodasilva wrote: >> >> Yep, just verified it and now I'm worried, this reminds me of the CSS >> :visited leak. > > I don't see why; that was a leak of info to sites. A site has no reason to > steal > its *own* password. This has been discussed extensively (e.g., in the > Mozilla > bug tracker, years ago). > > http://codereview.chromium.org/6770012/ >
Having seen the code, since the list item needs to know whether or not to show the button anyway, I don't actually see the value of sending "******" vs "" from C++. Now it just feels like the details of the display are split between the model and the view. I'd rather you go back to more like the previous iteration, but keep the removal of the C++->JS param for whether or not to show passwords that came from jhawkin's suggestion. However, I don't think it matters all that much what color the bikeshed is at this point, so LGTM whichever way you and jhawkins agree on. http://codereview.chromium.org/6770012/diff/13002/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/13002/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/password_manager_handler.cc:162: string16 stars(ASCIIToUTF16("***")); Why just three stars?
Agree, and changed it back. http://codereview.chromium.org/6770012/diff/13002/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/13002/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/password_manager_handler.cc:162: string16 stars(ASCIIToUTF16("***")); On 2011/04/05 15:58:29, stuartmorgan wrote: > Why just three stars? No particular reason. This has been moved to js and is now 8 stars.
jhawkins: Ping :-) On 2011/04/05 16:15:59, joaodasilva wrote: > Agree, and changed it back. > > http://codereview.chromium.org/6770012/diff/13002/chrome/browser/ui/webui/opt... > File chrome/browser/ui/webui/options/password_manager_handler.cc (right): > > http://codereview.chromium.org/6770012/diff/13002/chrome/browser/ui/webui/opt... > chrome/browser/ui/webui/options/password_manager_handler.cc:162: string16 > stars(ASCIIToUTF16("***")); > On 2011/04/05 15:58:29, stuartmorgan wrote: > > Why just three stars? > No particular reason. This has been moved to js and is now 8 stars.
http://codereview.chromium.org/6770012/diff/11007/chrome/browser/resources/op... File chrome/browser/resources/options/password_manager_list.js (right): http://codereview.chromium.org/6770012/diff/11007/chrome/browser/resources/op... chrome/browser/resources/options/password_manager_list.js:78: // button doesn't exist when passwords can't be shown. nit: |button| http://codereview.chromium.org/6770012/diff/11007/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/11007/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/password_manager_handler.cc:162: string16 empty; Why don't you just send the asterisks here? That would simplify logic in the view which it should not need to know about in the first place.
http://codereview.chromium.org/6770012/diff/11007/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/password_manager_handler.cc (right): http://codereview.chromium.org/6770012/diff/11007/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/password_manager_handler.cc:162: string16 empty; On 2011/04/11 17:13:26, James Hawkins wrote: > Why don't you just send the asterisks here? That would simplify logic in the > view which it should not need to know about in the first place. See my recent comment. The view already has to know about this to control the button, at which point it makes more sense to have the view control the details of how the view displays this than it does to have the controller pass view data.
On 2011/04/11 17:16:06, stuartmorgan wrote: > http://codereview.chromium.org/6770012/diff/11007/chrome/browser/ui/webui/opt... > File chrome/browser/ui/webui/options/password_manager_handler.cc (right): > > http://codereview.chromium.org/6770012/diff/11007/chrome/browser/ui/webui/opt... > chrome/browser/ui/webui/options/password_manager_handler.cc:162: string16 empty; > On 2011/04/11 17:13:26, James Hawkins wrote: > > Why don't you just send the asterisks here? That would simplify logic in the > > view which it should not need to know about in the first place. > > See my recent comment. The view already has to know about this to control the > button, at which point it makes more sense to have the view control the details > of how the view displays this than it does to have the controller pass view > data. Ok. LGTM with the nit from above.
On 2011/04/11 17:46:17, James Hawkins wrote: > On 2011/04/11 17:16:06, stuartmorgan wrote: > > > http://codereview.chromium.org/6770012/diff/11007/chrome/browser/ui/webui/opt... > > File chrome/browser/ui/webui/options/password_manager_handler.cc (right): > > > > > http://codereview.chromium.org/6770012/diff/11007/chrome/browser/ui/webui/opt... > > chrome/browser/ui/webui/options/password_manager_handler.cc:162: string16 > empty; > > On 2011/04/11 17:13:26, James Hawkins wrote: > > > Why don't you just send the asterisks here? That would simplify logic in the > > > view which it should not need to know about in the first place. > > > > See my recent comment. The view already has to know about this to control the > > button, at which point it makes more sense to have the view control the > details > > of how the view displays this than it does to have the controller pass view > > data. > > Ok. LGTM with the nit from above. Thanks for reviewing! Will commit later today. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
