Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11783008:
Add a lock to the managed user settings page and require authentication for unlocking. (Closed)
|
Created:
7 years, 11 months ago by Adrian Kuegel Modified:
7 years, 10 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd support for a manager passphrase of a managed account.
The managed user settings page will be shown with every control disabled. A click on the button "Unlock" shows the passphrase dialog. After a successful authentication, the controls of the settings page are enabled. The authentication is reset when the settings page
is closed.
BUG=171370
TEST=browser_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184206
Patch Set 1 #
Total comments: 77
Patch Set 2 : Added a randomly generated salt, small fix in key generation #
Total comments: 10
Patch Set 3 : Style issue fixes #
Total comments: 4
Patch Set 4 : Several fixes #
Total comments: 49
Patch Set 5 : Some more fixes, mostly coding style #Patch Set 6 : Prevent showing the settings and set password page without authentication #Patch Set 7 : Rebased to TOT, added const to function declarations when possible #Patch Set 8 : Add a check if the passphrase is set before closing the settings. #
Total comments: 16
Patch Set 9 : Add unit tests for Managed User Passphrase Handler #Patch Set 10 : Add browsertest for Managed User Passphrase Handler. #Patch Set 11 : Use ManagedUserService instead of ManagedMode. #Patch Set 12 : Add browser test for ManagedUserSetPassphraseOverlay #
Total comments: 14
Patch Set 13 : Fixed sync on cancel bug and other small issues. #Patch Set 14 : Adress comments and rebase to TOT. #Patch Set 15 : Use setCustomValidity in managed_user_set_passphrase.js. #
Total comments: 10
Patch Set 16 : Adressed comments regarding the set passphrase dialog. #
Total comments: 6
Patch Set 17 : A few more fixes. #
Total comments: 4
Patch Set 18 : Replace test class by test object. #
Total comments: 1
Patch Set 19 : Rebase to ToT, add test for ManagedUserSettings page. #
Total comments: 16
Patch Set 20 : Rebase to ToT and address review comments. #
Total comments: 4
Patch Set 21 : Address review comments. #
Total comments: 20
Patch Set 22 : Address review comments. #Patch Set 23 : Rebase to ToT. #Patch Set 24 : Fix problem with some WebUI tests. #Messages
Total messages: 55 (0 generated)
Please take a look and give me some feedback. So far, the salt is still hard-coded, please tell me how I should change it.
On 2013/01/07 09:52:46, Adrian Kuegel wrote: > Please take a look and give me some feedback. So far, the salt is still > hard-coded, please tell me how I should change it. As discussed offline, store the salt in a separate preference next to the hash. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.cc:95: "default", I think an empty string would be better as a default value (a human-readable string would make me think it's the clear text passphrase, but it's the hash, right?). https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_user_passphrase.cc (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.cc:20: if (!base::Base64Encode(passphrase_hash, encoded_passphrase_hash)) { Nit: I usually do `bool success = ...; DCHECK(success);` (NOTREACHED is a DCHECK(false), so this is a slightly more convoluted way to do the same). https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.cc:26: std::string* passphrase_hash) { Please align this parameter with the previous one (or if it doesn't fit anymore, put this first one on a new line, indented four spaces. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_user_passphrase.h (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.h:15: std::string* encoded_passphrase_hash); Please add some newlines here. At least before the private section, and between different types of declarations. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.h:17: static void GetPassphraseHash(const std::string& passphrase, You might want to change the interface to one method that a creates a random salt and a new hash with it and returns both, and one that checks a given passphrase, salt and hash and returns whether they match. Dunno :) https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.h:19: static const std::string kSalt_; As a rule of thumb, static and private should trigger a warning flag. If it's not a member and can't be accessed from the outside, you can usually declare it in an anonymous namespace in the .cc file. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.h:20: ManagedUserPassphrase(); See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order for the right order. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.h:20: ManagedUserPassphrase(); I think at the moment there is no need for a constructor, as the class has no (non-static) members. If you don't plan to add members, you could think about making it a namespace instead. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... File chrome/browser/resources/managed_user_passphrase_dialog.css (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.css:1: #family-control-passphrase-page { WHAT IS THIS FAMILY CONTROL YOU SPEAK OF https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.css:5: #instruct { Don't use abbreviations in identifiers (you probably want to say "instructions"?). https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.css:6: padding: 5px 15px; I think you could group these selectors. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.css:25: #incorrect_passphrase_warning { DOM IDs use hyphens to separate words. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... File chrome/browser/resources/managed_user_passphrase_dialog.html (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.html:13: <img class="chrome_kid_logo_img" src="chrome_kid.png" alt="Chrome kid logo" I don't think we need an alt attribute here. You could have a title (for accessibility), but we probably don't need it either, as the logo is purely decorative. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.html:14: width="65" height="65"> Width and height is probably also unnecessary? Remember, we don't need to fetch this over the network, so having the layout in place before the image is loaded shouldn't be a big problem. Hm, thinking about it, do you think it would be possible to add the image only in CSS? https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.html:46: <script src="chrome://resources/js/i18n_template2.js"></script> Can you move this to the header? https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... File chrome/browser/resources/managed_user_passphrase_dialog.js (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.js:7: if ($('passphrase_entry').value == '') { Braces around one-line statements are unnecessary. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.js:8: return; Hm, does that mean that we don't allow empty passwords? Do we need to check that here? https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.js:12: } Needs a semicolon. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.js:15: } Semicolon. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.js:19: chrome.send('DialogClose', ['correct']); Do you need to pass a string constant? Could you pass a boolean instead? https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.js:23: $('incorrect_passphrase_warning').style.visibility = 'visible'; Modifying style attributes directly kinda violates the MVC separation. Instead, set the |.hidden| attribute to true or false. Also, are you resetting it when the user checks the password again? That might look better (especially if we add something like a backoff later). https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.js:26: // Add handlers to HTML elements. This comment isn't really necessary :-D https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/manage_profile_overlay.js:65: ['ManageProfileOverlay.showManagedUserSettingsDialog']); If our code gets more complicated, we might need to change the API so we can pass in arbitrary closures, but right now that's probably not necessary. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/managed_user_set_passphrase.html (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.html:10: placeholder="Passphrase"> You're showing a non-internationalized string here. You want to set `i18n-values=".placeholder:passphrase"` (with "passphrase" defined in the localized strings). https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.js:12: * Encapsulated handling of XXX. Wait, that does not sound right ;-) https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.js:16: //this.activeNavTab = null; Please remove. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.js:34: // TODO display proper error message here Format TODOs as TODO(ldap), to make it easier to grep for them. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... File chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:32: virtual ~ManagedUserPassphraseDialogMessageHandler() {} Moar newlines plz :) https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:42: base::Unretained(this))); base::Unretained makes me queasy. How do you know that this object will still be around? I think you want to use weak pointers (base::WeakPtr(Factory)) here. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:47: const base::Value* passphrase_arg; Can you initialize this to null? https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:58: NOTREACHED(); DCHECK(pref); https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:60: if (!pref->GetValue()->GetAsString(&stored_passphrase_hash)) { bool success = ...; DCHECK(success); https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:75: void ManagedUserPassphraseDialogWebUI::CreateManagedUserPassphraseDialog( I think you could get rid of the static factory method and make the constructor public instead. For TabModalConfirmationDialog it was different, because the static method was in a different class (to allow platform-dependent implementations). https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:78: // this is deleted automatically when the user closes the dialog Capitalize please (also below). https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:113: if (closing_) { Single-line statements don't need braces. An empty line afterwards instead would be nice though. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:118: callback_.Run(); Hm, never running the callback when the user cancels might make things difficult later. Can you call it with a success value? https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:132: closing_ = false; You can initialize members directly. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... File chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h:31: // creates a passphrase dialog which will be deleted automatically when the Nit: Capitalize the sentence please. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h:53: virtual ~ManagedUserPassphraseDialogWebUI(); Please add a newline. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h:56: base::Callback<void(void)> callback_; This is typedef'd as a base::Closure. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:59: RegisterStrings(localized_strings, resources, arraysize(resources)); Could you switch this line with the empty one above? Then they're grouped together nicely. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:97: // was used for testing Then remove please :) https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.h (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:22: virtual ~ManagedUserPassphraseHandler(); Newline pls. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:23: virtual void InitializeHandler() OVERRIDE; Please add a comment stating which class you override. Hm, I guess it's OptionsPageUIHandler? In that case, move this down so they're together. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:34: void CorrectPassphraseEntered(); Moar newlinez!!!11!oneone https://codereview.chromium.org/11783008/diff/12001/chrome/browser/ui/webui/m... File chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h (right): https://codereview.chromium.org/11783008/diff/12001/chrome/browser/ui/webui/m... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h:5: #ifndef CHROME_BROWSER_UI_WEBUI_MANAGED_USER_PASSPHRASE_DIALOG_WEBUI_H_ Filename nit: the _webui at the end is probably not necessary (this dialog only exists in WebUI, and it's already in the path).
https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.cc:98: "managed_salt", Again, please use an empty string or something. https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_user_passphrase.cc (right): https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_user_passphrase.cc:24: if (salt_.size() == 0) { You can use `salt_.empty()` here. https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_user_passphrase.cc:36: salt_ = ""; salt_.clear(); https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_user_passphrase.cc:37: srand(time(0)); Please don't. I'm sure we have a cryptographic random number function somewhere. https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/ma... File chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc (right): https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/ma... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:51: std::string salt, stored_passphrase_hash, encoded_passphrase_hash; Please declare each variable separately. https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right): https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:71: const base::Value* callbackArg; Style guide says hacker_style for variables. https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:74: LOG(INFO) << callback_function_name_; At least DLOG? https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:75: content::WebContents *web_contents = web_ui()->GetWebContents(); Asterisk comes left. https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:79: base::Unretained(this))); Again, might want to use weak pointers. https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:88: ManagedUserPassphrase passphrase_key_generator(""); Using the empty std::string() constructor instead of the implicit one from const char* is preferred, because it's a little bit faster. https://codereview.chromium.org/11783008/diff/12001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_user_passphrase.cc (right): https://codereview.chromium.org/11783008/diff/12001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase.cc:17: const int kNumberOfIterations = 1; Fun fact: const variables automatically get internal linkage, so you don't need the anonymous namespace around here. https://codereview.chromium.org/11783008/diff/12001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11783008/diff/12001/chrome/common/pref_names.... chrome/common/pref_names.cc:846: const char kChildLockDisabled[] = "child.locking_disabled"; This preference is not used. https://codereview.chromium.org/11783008/diff/12001/chrome/common/pref_names.... chrome/common/pref_names.cc:2075: const char kManagedModeLocalPassphrase[] = "managed_mode.passphrase"; You should probably add a comment explaining these.
https://codereview.chromium.org/11783008/diff/1/chrome/app/generated_resource... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11783008/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:8079: + Enter passphrase to unlock managed user settings For the description: where does this message appear? For the message: Should it end in a period (.)? (If not, then should IDS_SET_PASSPHRASE_INSTRUCTIONS still have one?) https://codereview.chromium.org/11783008/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:8081: + <message name="IDS_UNLOCK_PASSPHRASE_BUTTON" desc="A button for unclocking the family control settings page."> Typo: "unclocking" https://codereview.chromium.org/11783008/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:8131: + <message name="IDS_SET_PASSPHRASE_TITLE" desc="XXX"> Please add descriptions to replace these two XXX. https://codereview.chromium.org/11783008/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:8143: + <message name="IDS_SAVE_PASSPHRASE_BUTTON" desc="A label for saving the passhrase to lock a managed mode user."> I'd suggest "A button label..." https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.cc:95: "default", I think we should make this empty as a default, and if the passphrase is empty, then we don't show a dialog asking for one. That lets custodians choose not to lock the settings if they prefer. The first part of that can be done here; the second part (skipping the dialogs when there's no passphrase) can be done in a later change. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_user_passphrase.cc (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.cc:12: const std::string ManagedUserPassphrase::kSalt_ = "managed_salt"; You'll be fixing this as your very next change, right? :) Too often, this kind of temporary placeholder ends up sticking around for months or years... https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.cc:17: DCHECK(encoded_passphrase_hash); This should just be a CHECK. If somebody passes a NULL in here, we're going to crash anyway. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.cc:36: DCHECK(encryption_key.get()); I don't think this is necessary, since the next line will helpfully crash for us if encryption_key is NULL. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_user_passphrase.h (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.h:12: class ManagedUserPassphrase { Please add a brief class comment describing what this thing is. I know it seems obvious from the name, but five years from now, when nobody's changed managed mode for three years, some new developer may come along and not be familiar with it at all. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.h:14: static void GenerateHashFromPassphrase(const std::string& passphrase, Please add a function comment describing what this does and what its arguments are expected to be. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.h:18: std::string* passphrase_hash); std::string is pretty lightweight, and actually OK to just pass back by value. This way is OK too, if you prefer it or it makes the API easier. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.h:21: ~ManagedUserPassphrase(); As a style point, I normally expect to see the constructor and destructor first in the list, even before static member functions. Also, there should be a blank line before the "public" section. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... File chrome/browser/resources/managed_user_passphrase_dialog.css (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.css:1: #family-control-passphrase-page { You need the copyright/license header here. /* Copyright (c) 2012 The Chromium Authors. All rights reserved. * Use of this source code is governed by a BSD-style license that can be * found in the LICENSE file. */ https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... File chrome/browser/resources/managed_user_passphrase_dialog.html (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.html:1: <!DOCTYPE html> Right after the DOCTYPE line: <!-- Copyright (c) 2012 The Chromium Authors. All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. --> https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.html:1: <!DOCTYPE html> Right after the DOCTYPE line: <!-- Copyright (c) 2012 The Chromium Authors. All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. --> https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.html:12: <div id="family-control-passphrase-page" class="page"> Please change "family" and "kid" to more general terms (managed mode, managed user) to reflect our wider set of use cases. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.html:26: <a href="http://www.google.com" target="_blank"> Let's just comment out the whole "forgot passphrase" div, until we have a place for it to point. (Probably a help page explaining the command-line recovery option, eventually.) https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... File chrome/browser/resources/managed_user_passphrase_dialog.js (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.js:7: if ($('passphrase_entry').value == '') { Should we support an empty password, if I decide not to set one? Or will we require *something* there, even one character? https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... chrome/browser/resources/managed_user_passphrase_dialog.js:11: $('passphrase_entry').value = ''; Is this necessary? I always find it a little odd-looking when what I've typed disappears, even if I got the passphrase wrong and need to retype it anyway. If we could have the field auto-select when clicked, would that be sufficient? https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/manage_profile_overlay.js:331: OptionsPage.navigateToPage('managedUser'); Hm, do we in any way prevent a user from just navigating there directly, by typing in the options URL? https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/managed_user_set_passphrase.html (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.html:1: <div id="managed-user-set-passphrase-overlay" class="page" hidden> Needs a license header https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.js:34: // TODO display proper error message here On 2013/01/07 14:20:22, Bernhard Bauer wrote: > Format TODOs as TODO(ldap), to make it easier to grep for them. With a colon after the userid; i.e., TODO(akuegel): display... Seems like it would be pretty easy to change the message in the "wrong password" error div to report these errors. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.js:42: chrome.send('setPassphrase', [$('passphrase-foo').value]); Is chrome.send sufficiently robust to be sending a plaintext password across it? I realize that JavaScript itself isn't guaranteed 100% attack-proof, but this adds even more attack surface. Perhaps check with the security team? https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... File chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h:31: // creates a passphrase dialog which will be deleted automatically when the On 2013/01/07 14:20:22, Bernhard Bauer wrote: > Nit: Capitalize the sentence please. Also please end it with a period. https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:70: // store the name of the callback function Capital letter and period please (also below). https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/optio... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:74: LOG(INFO) << callback_function_name_; Please remove this, or change it to DLOG, or even better a DVLOG. https://codereview.chromium.org/11783008/diff/1/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11783008/diff/1/chrome/common/pref_names.h#ne... chrome/common/pref_names.h:304: extern const char kChildLockDisabled[]; Please use more general terminology.
On 2013/01/07 14:20:22, Bernhard Bauer wrote: > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > chrome/browser/managed_mode/managed_user_passphrase.h:17: static void > GetPassphraseHash(const std::string& passphrase, > You might want to change the interface to one method that a creates a random > salt and a new hash with it and returns both, and one that checks a given > passphrase, salt and hash and returns whether they match. Dunno :) Changed it to a class which gets a salt as parameter to the constructor. If this provided salt is empty, a new salt is generated randomly which can be accessed with a GetSalt method. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > chrome/browser/managed_mode/managed_user_passphrase.h:19: static const > std::string kSalt_; > As a rule of thumb, static and private should trigger a warning flag. If it's > not a member and can't be accessed from the outside, you can usually declare it > in an anonymous namespace in the .cc file. This variable is now not static anymore. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > chrome/browser/managed_mode/managed_user_passphrase.h:20: > ManagedUserPassphrase(); > I think at the moment there is no need for a constructor, as the class has no > (non-static) members. If you don't plan to add members, you could think about > making it a namespace instead. The class interface has been changed, see above. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > File chrome/browser/resources/managed_user_passphrase_dialog.css (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.css:1: > #family-control-passphrase-page { > WHAT IS THIS FAMILY CONTROL YOU SPEAK OF I guess I meant managed-mode. Changed. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.html:14: width="65" > height="65"> > Width and height is probably also unnecessary? Remember, we don't need to fetch > this over the network, so having the layout in place before the image is loaded > shouldn't be a big problem. > > Hm, thinking about it, do you think it would be possible to add the image only > in CSS? The image is now added as background image of a div in CSS. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.html:46: <script > src="chrome://resources/js/i18n_template2.js"></script> > Can you move this to the header? I tried this, but then it doesn't work. I also saw in other html pages that this script is included in the body, so maybe it has to be that way. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.js:8: return; > Hm, does that mean that we don't allow empty passwords? Do we need to check that > here? We need to discuss this more I guess. In my opinion, either we shouldn't show the dialog because the user doesn't want to use a passphrase, or if we show it, it should be a non-empty passphrase required for unlocking. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.js:19: > chrome.send('DialogClose', ['correct']); > Do you need to pass a string constant? Could you pass a boolean instead? Yes, I think a String is required (or no parameter at all, but I already use that for Cancel), but it could be an empty string as in cloud_print_setup_login.js. Now I used 'true' as a string constant. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.js:23: > $('incorrect_passphrase_warning').style.visibility = 'visible'; > Modifying style attributes directly kinda violates the MVC separation. Instead, > set the |.hidden| attribute to true or false. Also, are you resetting it when > the user checks the password again? That might look better (especially if we add > something like a backoff later). Changed. Also the warning message will now disappear when the user starts typing the passphrase again. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > chrome/browser/resources/options/manage_profile_overlay.js:65: > ['ManageProfileOverlay.showManagedUserSettingsDialog']); > If our code gets more complicated, we might need to change the API so we can > pass in arbitrary closures, but right now that's probably not necessary. Ok, I can change it later if we see it doesn't work that way. > chrome/browser/resources/options/managed_user_set_passphrase.js:34: // TODO > display proper error message here > Format TODOs as TODO(ldap), to make it easier to grep for them. Done. Actually I removed the TODOs because I already did those changes, but I added another TODO in the (hopefully) right format in managed_user_passphrase_dialog.html. > chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:75: void > ManagedUserPassphraseDialogWebUI::CreateManagedUserPassphraseDialog( > I think you could get rid of the static factory method and make the constructor > public instead. For TabModalConfirmationDialog it was different, because the > static method was in a different class (to allow platform-dependent > implementations). I could get rid of it, but since this class will be created from all UI-Handlers which need to provide the locking-mechanism, I thought it would be helpful if we don't have to worry about accidentally calling delete on that class because we forget that it is deleted automatically. Also, we would probably need to add a comment everytime an instance of this class is created that it is deleted automatically. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... > chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h:56: > base::Callback<void(void)> callback_; > This is typedef'd as a base::Closure. As it is now void(bool), I didn't use a typedef. Is there also one for Callback<void(bool)>? > https://codereview.chromium.org/11783008/diff/12001/chrome/browser/ui/webui/m... > chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h:5: #ifndef > CHROME_BROWSER_UI_WEBUI_MANAGED_USER_PASSPHRASE_DIALOG_WEBUI_H_ > Filename nit: the _webui at the end is probably not necessary (this dialog only > exists in WebUI, and it's already in the path). I could change it, but the tab_modal_confirm_dialog_webui had the same naming, so I thought I should do it the same way. So far, I didn't change it.
On 2013/01/07 14:39:22, Bernhard Bauer wrote: > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... > File chrome/browser/managed_mode/managed_mode.cc (right): > > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... > chrome/browser/managed_mode/managed_mode.cc:98: "managed_salt", > Again, please use an empty string or something. Done. > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... > File chrome/browser/managed_mode/managed_user_passphrase.cc (right): > > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... > chrome/browser/managed_mode/managed_user_passphrase.cc:24: if (salt_.size() == > 0) { > You can use `salt_.empty()` here. Changed. > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... > chrome/browser/managed_mode/managed_user_passphrase.cc:36: salt_ = ""; > salt_.clear(); > > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/managed_mod... > chrome/browser/managed_mode/managed_user_passphrase.cc:37: srand(time(0)); > Please don't. I'm sure we have a cryptographic random number function somewhere. Replaced by crypto::RandBytes from crypto/random.h. > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/ma... > File chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc (right): > > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/ma... > chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:51: std::string > salt, stored_passphrase_hash, encoded_passphrase_hash; > Please declare each variable separately. Done. > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... > File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right): > > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... > chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:71: const > base::Value* callbackArg; > Style guide says hacker_style for variables. Changed. > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... > chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:74: LOG(INFO) > << callback_function_name_; > At least DLOG? Removed. This was still in there from debugging, but it is not actually needed anymore. > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... > chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:75: > content::WebContents *web_contents = web_ui()->GetWebContents(); > Asterisk comes left. Changed. > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... > chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:79: > base::Unretained(this))); > Again, might want to use weak pointers. Forgot to change every instance of this, will do this in the next upload. > https://codereview.chromium.org/11783008/diff/3001/chrome/browser/ui/webui/op... > chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:88: > ManagedUserPassphrase passphrase_key_generator(""); > Using the empty std::string() constructor instead of the implicit one from const > char* is preferred, because it's a little bit faster. Done. > https://codereview.chromium.org/11783008/diff/12001/chrome/browser/managed_mo... > File chrome/browser/managed_mode/managed_user_passphrase.cc (right): > > https://codereview.chromium.org/11783008/diff/12001/chrome/browser/managed_mo... > chrome/browser/managed_mode/managed_user_passphrase.cc:17: const int > kNumberOfIterations = 1; > Fun fact: const variables automatically get internal linkage, so you don't need > the anonymous namespace around here. Moved them to outside the namespace. > https://codereview.chromium.org/11783008/diff/12001/chrome/common/pref_names.cc > File chrome/common/pref_names.cc (right): > > https://codereview.chromium.org/11783008/diff/12001/chrome/common/pref_names.... > chrome/common/pref_names.cc:846: const char kChildLockDisabled[] = > "child.locking_disabled"; > This preference is not used. Removed the preference. > https://codereview.chromium.org/11783008/diff/12001/chrome/common/pref_names.... > chrome/common/pref_names.cc:2075: const char kManagedModeLocalPassphrase[] = > "managed_mode.passphrase"; > You should probably add a comment explaining these. Done.
On 2013/01/07 14:51:49, Pam wrote: > https://codereview.chromium.org/11783008/diff/1/chrome/app/generated_resource... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/app/generated_resource... > chrome/app/generated_resources.grd:8079: + Enter passphrase to unlock > managed user settings > For the description: where does this message appear? > For the message: Should it end in a period (.)? (If not, then should > IDS_SET_PASSPHRASE_INSTRUCTIONS still have one?) It should have a period I think, so I added a period. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > File chrome/browser/managed_mode/managed_mode.cc (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > chrome/browser/managed_mode/managed_mode.cc:95: "default", > I think we should make this empty as a default, and if the passphrase is empty, > then we don't show a dialog asking for one. That lets custodians choose not to > lock the settings if they prefer. > > The first part of that can be done here; the second part (skipping the dialogs > when there's no passphrase) can be done in a later change. Yes, I agree with that. If passphrase is empty we can use this to indicate that the user does not want a unlock dialog being shown. The default is now the empty passphrase. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > File chrome/browser/managed_mode/managed_user_passphrase.cc (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > chrome/browser/managed_mode/managed_user_passphrase.cc:12: const std::string > ManagedUserPassphrase::kSalt_ = "managed_salt"; > You'll be fixing this as your very next change, right? :) Too often, this kind > of temporary placeholder ends up sticking around for months or years... This is fixed now. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > chrome/browser/managed_mode/managed_user_passphrase.h:18: std::string* > passphrase_hash); > std::string is pretty lightweight, and actually OK to just pass back by value. > This way is OK too, if you prefer it or it makes the API easier. I didn't change it, because I think it makes it a bit easier, since this method will call another method which requires a string pointer. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > chrome/browser/managed_mode/managed_user_passphrase.h:21: > ~ManagedUserPassphrase(); > As a style point, I normally expect to see the constructor and destructor first > in the list, even before static member functions. Also, there should be a blank > line before the "public" section. The constructors are now first, and there is a blank line between them and the first member function. Is it also necessary to include a blank line before the public: keyword? > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > File chrome/browser/resources/managed_user_passphrase_dialog.css (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.css:1: > #family-control-passphrase-page { > You need the copyright/license header here. > > /* Copyright (c) 2012 The Chromium Authors. All rights reserved. > * Use of this source code is governed by a BSD-style license that can be > * found in the LICENSE file. */ Done. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > File chrome/browser/resources/managed_user_passphrase_dialog.html (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.html:1: <!DOCTYPE html> > Right after the DOCTYPE line: > > <!-- Copyright (c) 2012 The Chromium Authors. All rights reserved. > Use of this source code is governed by a BSD-style license that can be > found in the LICENSE file. --> > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.html:1: <!DOCTYPE html> > Right after the DOCTYPE line: > > <!-- Copyright (c) 2012 The Chromium Authors. All rights reserved. > Use of this source code is governed by a BSD-style license that can be > found in the LICENSE file. --> Done. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.html:12: <div > id="family-control-passphrase-page" class="page"> > Please change "family" and "kid" to more general terms (managed mode, managed > user) to reflect our wider set of use cases. Done. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.html:26: <a > href="http://www.google.com" target="_blank"> > Let's just comment out the whole "forgot passphrase" div, until we have a place > for it to point. (Probably a help page explaining the command-line recovery > option, eventually.) Done. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > File chrome/browser/resources/managed_user_passphrase_dialog.js (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.js:7: if > ($('passphrase_entry').value == '') { > Should we support an empty password, if I decide not to set one? Or will we > require *something* there, even one character? I think if someone clicks on "set passphrase", he should also enter *something* there, otherwise I see no reason why he would want to set a passphrase at all. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > chrome/browser/resources/managed_user_passphrase_dialog.js:11: > $('passphrase_entry').value = ''; > Is this necessary? I always find it a little odd-looking when what I've typed > disappears, even if I got the passphrase wrong and need to retype it anyway. If > we could have the field auto-select when clicked, would that be sufficient? Changed. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > File chrome/browser/resources/options/manage_profile_overlay.js (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > chrome/browser/resources/options/manage_profile_overlay.js:331: > OptionsPage.navigateToPage('managedUser'); > Hm, do we in any way prevent a user from just navigating there directly, by > typing in the options URL? No, this is currently not prevented. I am also not sure if it is that easy, especially since we need to allow the direct access when a new managed user profile is created. I may be wrong, of course. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > File chrome/browser/resources/options/managed_user_set_passphrase.html (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > chrome/browser/resources/options/managed_user_set_passphrase.html:1: <div > id="managed-user-set-passphrase-overlay" class="page" hidden> > Needs a license header I am not sure about that. All options html pages don't have a header. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > File chrome/browser/resources/options/managed_user_set_passphrase.js (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > chrome/browser/resources/options/managed_user_set_passphrase.js:34: // TODO > display proper error message here > On 2013/01/07 14:20:22, Bernhard Bauer wrote: > > Format TODOs as TODO(ldap), to make it easier to grep for them. > > With a colon after the userid; i.e., > > TODO(akuegel): display... > > Seems like it would be pretty easy to change the message in the "wrong password" > error div to report these errors. Done. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > chrome/browser/resources/options/managed_user_set_passphrase.js:42: > chrome.send('setPassphrase', [$('passphrase-foo').value]); > Is chrome.send sufficiently robust to be sending a plaintext password across it? > I realize that JavaScript itself isn't guaranteed 100% attack-proof, but this > adds even more attack surface. Perhaps check with the security team? I will try to check that.
As a general note, you can directly reply to individual comments inline in the diff view. That makes it easier to see the code and have discussions about individual issues. On 2013/01/08 16:19:24, Adrian Kuegel wrote: > On 2013/01/07 14:51:49, Pam wrote: > > > https://codereview.chromium.org/11783008/diff/1/chrome/app/generated_resource... > > File chrome/app/generated_resources.grd (right): > > > > > https://codereview.chromium.org/11783008/diff/1/chrome/app/generated_resource... > > chrome/app/generated_resources.grd:8079: + Enter passphrase to unlock > > managed user settings > > For the description: where does this message appear? > > For the message: Should it end in a period (.)? (If not, then should > > IDS_SET_PASSPHRASE_INSTRUCTIONS still have one?) > > It should have a period I think, so I added a period. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > > File chrome/browser/managed_mode/managed_mode.cc (right): > > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > > chrome/browser/managed_mode/managed_mode.cc:95: "default", > > I think we should make this empty as a default, and if the passphrase is > empty, > > then we don't show a dialog asking for one. That lets custodians choose not to > > lock the settings if they prefer. > > > > The first part of that can be done here; the second part (skipping the dialogs > > when there's no passphrase) can be done in a later change. > > Yes, I agree with that. If passphrase is empty we can use this to indicate that > the user does not want a unlock dialog being shown. The default is now the empty > passphrase. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > > File chrome/browser/managed_mode/managed_user_passphrase.cc (right): > > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > > chrome/browser/managed_mode/managed_user_passphrase.cc:12: const std::string > > ManagedUserPassphrase::kSalt_ = "managed_salt"; > > You'll be fixing this as your very next change, right? :) Too often, this kind > > of temporary placeholder ends up sticking around for months or years... > > This is fixed now. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > > chrome/browser/managed_mode/managed_user_passphrase.h:18: std::string* > > passphrase_hash); > > std::string is pretty lightweight, and actually OK to just pass back by value. > > This way is OK too, if you prefer it or it makes the API easier. > > I didn't change it, because I think it makes it a bit easier, since this method > will call another method which requires a string pointer. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > > chrome/browser/managed_mode/managed_user_passphrase.h:21: > > ~ManagedUserPassphrase(); > > As a style point, I normally expect to see the constructor and destructor > first > > in the list, even before static member functions. Also, there should be a > blank > > line before the "public" section. > > The constructors are now first, and there is a blank line between them and the > first member function. Is it also necessary to include a blank line before the > public: keyword? I'm too lazy to look it up formally in the style guide right now, but that's the way I have seen it everywhere. > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > File chrome/browser/resources/managed_user_passphrase_dialog.css (right): > > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.css:1: > > #family-control-passphrase-page { > > You need the copyright/license header here. > > > > /* Copyright (c) 2012 The Chromium Authors. All rights reserved. > > * Use of this source code is governed by a BSD-style license that can be > > * found in the LICENSE file. */ > > Done. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > File chrome/browser/resources/managed_user_passphrase_dialog.html (right): > > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.html:1: <!DOCTYPE > html> > > Right after the DOCTYPE line: > > > > <!-- Copyright (c) 2012 The Chromium Authors. All rights reserved. > > Use of this source code is governed by a BSD-style license that can be > > found in the LICENSE file. --> > > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.html:1: <!DOCTYPE > html> > > Right after the DOCTYPE line: > > > > <!-- Copyright (c) 2012 The Chromium Authors. All rights reserved. > > Use of this source code is governed by a BSD-style license that can be > > found in the LICENSE file. --> > > Done. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.html:12: <div > > id="family-control-passphrase-page" class="page"> > > Please change "family" and "kid" to more general terms (managed mode, managed > > user) to reflect our wider set of use cases. > > Done. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.html:26: <a > > href="http://www.google.com" target="_blank"> > > Let's just comment out the whole "forgot passphrase" div, until we have a > place > > for it to point. (Probably a help page explaining the command-line recovery > > option, eventually.) > > Done. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > File chrome/browser/resources/managed_user_passphrase_dialog.js (right): > > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.js:7: if > > ($('passphrase_entry').value == '') { > > Should we support an empty password, if I decide not to set one? Or will we > > require *something* there, even one character? > > I think if someone clicks on "set passphrase", he should also enter *something* > there, otherwise I see no reason why he would want to set a passphrase at all. I agree; we have a separate checkbox to select whether we want a passphrase at all. That's definitely clearer. https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_user_passphrase.cc (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_user_passphrase.cc:17: DCHECK(encoded_passphrase_hash); On 2013/01/07 14:51:49, Pam wrote: > This should just be a CHECK. If somebody passes a NULL in here, we're going to > crash anyway. Well, then you can probably leave it out completely, right? https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/manage_profile_overlay.js:331: OptionsPage.navigateToPage('managedUser'); On 2013/01/07 14:51:49, Pam wrote: > Hm, do we in any way prevent a user from just navigating there directly, by > typing in the options URL? Either that, or ignore any commands that get sent from there unless the user has successfully authenticated (so we need to keep some state for that). I think that would be a bit more robust. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/managed_user_set_passphrase.html (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.html:1: <div id="managed-user-set-passphrase-overlay" class="page" hidden> On 2013/01/07 14:51:49, Pam wrote: > Needs a license header Ping. https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.js:42: chrome.send('setPassphrase', [$('passphrase-foo').value]); On 2013/01/07 14:51:49, Pam wrote: > Is chrome.send sufficiently robust to be sending a plaintext password across it? > I realize that JavaScript itself isn't guaranteed 100% attack-proof, but this > adds even more attack surface. Perhaps check with the security team? FWIW, sync also sends the passphrase in plaintext over chrome.send(). https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_user_passphrase.cc (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_user_passphrase.cc:30: std::string bytes(32, '\0'); Can you pull the size out into a constant as well? Also, the null bytes probably aren't necessary, as RandBytes will write over them anyway (and WriteInto will reserve the space). https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_user_passphrase.cc:31: crypto::RandBytes(WriteInto(&bytes, bytes.size()), bytes.size()); I think this will set the length of the string to 31 instead of 32 (it reserves one additional byte in the backing buffer for the null byte, but that is not counted as part of the string, so that strlen and .length() return the same value). https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_user_passphrase.h (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_user_passphrase.h:20: std::string GetSalt(); Newlines before comments would be nice. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_user_passphrase.h:25: std::string* encoded_passphrase_hash); Align the parameter please. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_user_passphrase.h:29: std::string* passphrase_hash); Align please. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m... File chrome/browser/resources/managed_user_passphrase_dialog.css (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m... chrome/browser/resources/managed_user_passphrase_dialog.css:35: padding: 5px 15px; Can you move this into the selector above as well? https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m... File chrome/browser/resources/managed_user_passphrase_dialog.html (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m... chrome/browser/resources/managed_user_passphrase_dialog.html:16: <div id="chrome-kid-logo"></div> Spaces instead of tabs please. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m... chrome/browser/resources/managed_user_passphrase_dialog.html:27: <!-- TODO(akuegel): Create a link to a page which explains how to reset the Less indentation? Also, we can probably leave the link in, even if it's not working. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m... chrome/browser/resources/managed_user_passphrase_dialog.html:37: i18n-content="cancel_passphrase_button"></button> Indent four space w/r/t previous line. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m... chrome/browser/resources/managed_user_passphrase_dialog.html:40: i18n-content="unlock_passphrase_button" disabled></button> Indentation https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... File chrome/browser/resources/options/chrome_kid.png (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... chrome/browser/resources/options/chrome_kid.png:1: ‰PNG Can you rename this file to robot.png or something? https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... File chrome/browser/resources/options/managed_user_set_passphrase.css (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... chrome/browser/resources/options/managed_user_set_passphrase.css:13: .equal-passphrases { I would actually only use one class that is added or remved, and make the attributes of the other class the default. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... chrome/browser/resources/options/managed_user_set_passphrase.js:46: $('passphrase-mismatch').hidden = false; Could you do this with CSS rules (based on the class name of #passphrase-confirm, for example)? https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... chrome/browser/resources/options/managed_user_set_passphrase.js:47: $('passphrase-confirm').setAttribute('class', 'unequal-passphrases'); You can use .classList (https://developer.mozilla.org/en-US/docs/DOM/element.classList) here. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... chrome/browser/resources/options/managed_user_set_passphrase.js:50: else { Move to previous line https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... chrome/browser/resources/options/managed_user_set_passphrase.js:51: $('save-passphrase').disabled = ($('passphrase-confirm').value == '' || I think this one isn't necessary (if one of them is empty but the other one isn't, we wouldn't be in this branch anyway). https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/ma... File chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/ma... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:64: DCHECK(pref); The pref checks are unnecessary again. Oh, and you don't actually need to go via FindPreference(), you can directly call GetString(). https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/ma... chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:76: &encoded_passphrase_hash); Align with previous parameter, please. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:58: if (success) I think I would actually plumb the success through to the Web UI. You might want to do different things in the UI if authentication fails. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:66: args->Get(0, &callback_arg); Here you can use ListValue::GetString(). https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:68: content::WebContents* web_contents = web_ui()->GetWebContents(); You could probably inline this. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:83: passphrase_key_generator.GenerateHashFromPassphrase(passphrase, Align the parameters please (here and below). https://codereview.chromium.org/11783008/diff/3005/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11783008/diff/3005/chrome/common/pref_names.h... chrome/common/pref_names.h:698: // Used for the managed mode to calculate and store a hash of the password Descriptions are in the header file only (and if you two different descriptions, it can get really confusing).
On 2013/01/08 15:18:42, Adrian Kuegel wrote: > On 2013/01/07 14:20:22, Bernhard Bauer wrote: > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > > chrome/browser/managed_mode/managed_user_passphrase.h:17: static void > > GetPassphraseHash(const std::string& passphrase, > > You might want to change the interface to one method that a creates a random > > salt and a new hash with it and returns both, and one that checks a given > > passphrase, salt and hash and returns whether they match. Dunno :) > > Changed it to a class which gets a salt as parameter to the constructor. If this > provided salt is empty, a new salt is generated randomly which can be accessed > with a GetSalt method. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > > chrome/browser/managed_mode/managed_user_passphrase.h:19: static const > > std::string kSalt_; > > As a rule of thumb, static and private should trigger a warning flag. If it's > > not a member and can't be accessed from the outside, you can usually declare > it > > in an anonymous namespace in the .cc file. > > This variable is now not static anymore. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > > chrome/browser/managed_mode/managed_user_passphrase.h:20: > > ManagedUserPassphrase(); > > I think at the moment there is no need for a constructor, as the class has no > > (non-static) members. If you don't plan to add members, you could think about > > making it a namespace instead. > > The class interface has been changed, see above. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > File chrome/browser/resources/managed_user_passphrase_dialog.css (right): > > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.css:1: > > #family-control-passphrase-page { > > WHAT IS THIS FAMILY CONTROL YOU SPEAK OF > > I guess I meant managed-mode. Changed. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.html:14: width="65" > > height="65"> > > Width and height is probably also unnecessary? Remember, we don't need to > fetch > > this over the network, so having the layout in place before the image is > loaded > > shouldn't be a big problem. > > > > Hm, thinking about it, do you think it would be possible to add the image only > > in CSS? > > The image is now added as background image of a div in CSS. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.html:46: <script > > src="chrome://resources/js/i18n_template2.js"></script> > > Can you move this to the header? > > I tried this, but then it doesn't work. I also saw in other html pages that this > script is included in the body, so maybe it has to be that way. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.js:8: return; > > Hm, does that mean that we don't allow empty passwords? Do we need to check > that > > here? > > We need to discuss this more I guess. In my opinion, either we shouldn't show > the dialog because the user doesn't want to use a passphrase, or if we show it, > it should be a non-empty passphrase required for unlocking. As discussed offline, ok. > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.js:19: > > chrome.send('DialogClose', ['correct']); > > Do you need to pass a string constant? Could you pass a boolean instead? > > Yes, I think a String is required (or no parameter at all, but I already use > that for Cancel), but it could be an empty string as in > cloud_print_setup_login.js. Now I used 'true' as a string constant. Are you sure that an actual string is required? Do you have a check that fails when you pass a boolean? > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/mana... > > chrome/browser/resources/managed_user_passphrase_dialog.js:23: > > $('incorrect_passphrase_warning').style.visibility = 'visible'; > > Modifying style attributes directly kinda violates the MVC separation. > Instead, > > set the |.hidden| attribute to true or false. Also, are you resetting it when > > the user checks the password again? That might look better (especially if we > add > > something like a backoff later). > > Changed. Also the warning message will now disappear when the user starts typing > the passphrase again. > > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > > chrome/browser/resources/options/manage_profile_overlay.js:65: > > ['ManageProfileOverlay.showManagedUserSettingsDialog']); > > If our code gets more complicated, we might need to change the API so we can > > pass in arbitrary closures, but right now that's probably not necessary. > > Ok, I can change it later if we see it doesn't work that way. > > > chrome/browser/resources/options/managed_user_set_passphrase.js:34: // TODO > > display proper error message here > > Format TODOs as TODO(ldap), to make it easier to grep for them. > > Done. Actually I removed the TODOs because I already did those changes, but I > added another TODO in the (hopefully) right format in > managed_user_passphrase_dialog.html. > > > chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:75: void > > ManagedUserPassphraseDialogWebUI::CreateManagedUserPassphraseDialog( > > I think you could get rid of the static factory method and make the > constructor > > public instead. For TabModalConfirmationDialog it was different, because the > > static method was in a different class (to allow platform-dependent > > implementations). > > I could get rid of it, but since this class will be created from all UI-Handlers > which need to provide the locking-mechanism, I thought it would be helpful if we > don't have to worry about accidentally calling delete on that class because we > forget that it is deleted automatically. The destructor can still be private, so clients couldn't delete it. > Also, we would probably need to add a > comment everytime an instance of this class is created that it is deleted > automatically. Eh, that comment is purely to convey to casual readers of the code that it doesn't actually leak. When passing ownership we have the scoped_ptr<> idiom now which lets both the compiler and the reader verify that the caller takes ownership, but there's no such thing for self-deleting objects, hence the comment. > > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/ui/webui/manag... > > chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h:56: > > base::Callback<void(void)> callback_; > > This is typedef'd as a base::Closure. > > As it is now void(bool), I didn't use a typedef. Is there also one for > Callback<void(bool)>? No, only completely void (otherwise you would probably very soon run out of short names, which would negate the point of the abbreviated typedefs :) ) > > > https://codereview.chromium.org/11783008/diff/12001/chrome/browser/ui/webui/m... > > chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.h:5: #ifndef > > CHROME_BROWSER_UI_WEBUI_MANAGED_USER_PASSPHRASE_DIALOG_WEBUI_H_ > > Filename nit: the _webui at the end is probably not necessary (this dialog > only > > exists in WebUI, and it's already in the path). > > I could change it, but the tab_modal_confirm_dialog_webui had the same naming, > so I thought I should do it the same way. So far, I didn't change it. Eh... TabModalConfirmDialogWebUI is a platform-specific implementation of a superclass. This class stands on its on.
On 2013/01/08 17:43:14, Bernhard Bauer wrote: > As a general note, you can directly reply to individual comments inline in the > diff view. That makes it easier to see the code and have discussions about > individual issues. Ok, I will try to do that now. > > The constructors are now first, and there is a blank line between them and the > > first member function. Is it also necessary to include a blank line before the > > public: keyword? > > I'm too lazy to look it up formally in the style guide right now, but that's the > way I have seen it everywhere. I have looked it up now: according to the C++ style guide, there should be no blank line before the first such keyword: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... > https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/m... > chrome/browser/managed_mode/managed_user_passphrase.cc:17: > DCHECK(encoded_passphrase_hash); > On 2013/01/07 14:51:49, Pam wrote: > > This should just be a CHECK. If somebody passes a NULL in here, we're going to > > crash anyway. > > Well, then you can probably leave it out completely, right? Ok, I will leave it out. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > File chrome/browser/resources/options/manage_profile_overlay.js (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > chrome/browser/resources/options/manage_profile_overlay.js:331: > OptionsPage.navigateToPage('managedUser'); > On 2013/01/07 14:51:49, Pam wrote: > > Hm, do we in any way prevent a user from just navigating there directly, by > > typing in the options URL? > > Either that, or ignore any commands that get sent from there unless the user has > successfully authenticated (so we need to keep some state for that). I think > that would be a bit more robust. Yes, I guess it would be better to keep some state for authentication. But then we need to consider when the authenticated state should change back to the non-authenticated state, either automatically when closing some window with access restriction, or manually when the user clicks on some kind of lock. Could the InManagedMode preference be used for that, or do we need something separate? > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > File chrome/browser/resources/options/managed_user_set_passphrase.html (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > chrome/browser/resources/options/managed_user_set_passphrase.html:1: <div > id="managed-user-set-passphrase-overlay" class="page" hidden> > On 2013/01/07 14:51:49, Pam wrote: > > Needs a license header > > Ping. Almost no html options file has such a license header (in fact, all those that I did check so far didn't have one), that was the reason I didn't include it so far. I discussed this offline with Pam, and I will add it now, because it doesn't hurt to have it there. > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > File chrome/browser/resources/options/managed_user_set_passphrase.js (right): > > https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... > chrome/browser/resources/options/managed_user_set_passphrase.js:42: > chrome.send('setPassphrase', [$('passphrase-foo').value]); > On 2013/01/07 14:51:49, Pam wrote: > > Is chrome.send sufficiently robust to be sending a plaintext password across > it? > > I realize that JavaScript itself isn't guaranteed 100% attack-proof, but this > > adds even more attack surface. Perhaps check with the security team? > > FWIW, sync also sends the passphrase in plaintext over chrome.send(). I also looked at how it is done in resources/chromeos/login/screen_gaia_sigin.js and it seems to send the plaintext password with chrome.send as well.
Let me know when you've uploaded the latest version. On Wed, Jan 9, 2013 at 10:23 AM, <akuegel@chromium.org> wrote: > On 2013/01/08 17:43:14, Bernhard Bauer wrote: > >> As a general note, you can directly reply to individual comments inline >> in the >> diff view. That makes it easier to see the code and have discussions about >> individual issues. >> > > Ok, I will try to do that now. > > > > The constructors are now first, and there is a blank line between them >> and >> > the > >> > first member function. Is it also necessary to include a blank line >> before >> > the > >> > public: keyword? >> > > I'm too lazy to look it up formally in the style guide right now, but >> that's >> > the > >> way I have seen it everywhere. >> > > I have looked it up now: according to the C++ style guide, there should be > no > blank line before the first such keyword: > http://google-styleguide.**googlecode.com/svn/trunk/** > cppguide.xml?showone=Class_**Format#Class_Format<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_Format#Class_Format> Oh, the first one! Yes, no blank line before that. Sorry, I misread your question. > > https://codereview.chromium.**org/11783008/diff/1/chrome/** > browser/managed_mode/managed_**user_passphrase.cc#newcode17<https://codereview.chromium.org/11783008/diff/1/chrome/browser/managed_mode/managed_user_passphrase.cc#newcode17> > >> chrome/browser/managed_mode/**managed_user_passphrase.cc:17: >> DCHECK(encoded_passphrase_**hash); >> On 2013/01/07 14:51:49, Pam wrote: >> > This should just be a CHECK. If somebody passes a NULL in here, we're >> going >> > to > >> > crash anyway. >> > > Well, then you can probably leave it out completely, right? >> > > Ok, I will leave it out. > > > > https://codereview.chromium.**org/11783008/diff/1/chrome/** > browser/resources/options/**manage_profile_overlay.js<https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/options/manage_profile_overlay.js> > >> File chrome/browser/resources/**options/manage_profile_**overlay.js >> (right): >> > > > https://codereview.chromium.**org/11783008/diff/1/chrome/** > browser/resources/options/**manage_profile_overlay.js#**newcode331<https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/options/manage_profile_overlay.js#newcode331> > >> chrome/browser/resources/**options/manage_profile_**overlay.js:331: >> OptionsPage.navigateToPage('**managedUser'); >> On 2013/01/07 14:51:49, Pam wrote: >> > Hm, do we in any way prevent a user from just navigating there >> directly, by >> > typing in the options URL? >> > > Either that, or ignore any commands that get sent from there unless the >> user >> > has > >> successfully authenticated (so we need to keep some state for that). I >> think >> that would be a bit more robust. >> > > Yes, I guess it would be better to keep some state for authentication. But > then > we need to consider when the authenticated state should change back to the > non-authenticated state, either automatically when closing some window with > access restriction, or manually when the user clicks on some kind of lock. > Could > the InManagedMode preference be used for that, or do we need something > separate? Well, kInManagedMode is at the moment at global preference, so we would need to add something in the user preferences. At that point, we should probably name it "authenticated" or something, to get away from the "mode" term. But yes, we definitely need some a way to change back. Another idea would be to store the authenticated flag per WebUI, so it would be cleared if the user closes the tab. I guess we'll have to play around it with a bit. > > > https://codereview.chromium.**org/11783008/diff/1/chrome/** > browser/resources/options/**managed_user_set_passphrase.**html<https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/options/managed_user_set_passphrase.html> > >> File chrome/browser/resources/**options/managed_user_set_** >> passphrase.html >> > (right): > > > https://codereview.chromium.**org/11783008/diff/1/chrome/** > browser/resources/options/**managed_user_set_passphrase.**html#newcode1<https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/options/managed_user_set_passphrase.html#newcode1> > >> chrome/browser/resources/**options/managed_user_set_**passphrase.html:1: >> <div >> id="managed-user-set-**passphrase-overlay" class="page" hidden> >> On 2013/01/07 14:51:49, Pam wrote: >> > Needs a license header >> > > Ping. >> > > Almost no html options file has such a license header (in fact, all those > that I > did check so far didn't have one), that was the reason I didn't include it > so > far. I discussed this offline with Pam, and I will add it now, because it > doesn't hurt to have it there. > > > > https://codereview.chromium.**org/11783008/diff/1/chrome/** > browser/resources/options/**managed_user_set_passphrase.js<https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/options/managed_user_set_passphrase.js> > >> File chrome/browser/resources/**options/managed_user_set_**passphrase.js >> (right): >> > > > https://codereview.chromium.**org/11783008/diff/1/chrome/** > browser/resources/options/**managed_user_set_passphrase.**js#newcode42<https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/options/managed_user_set_passphrase.js#newcode42> > >> chrome/browser/resources/**options/managed_user_set_**passphrase.js:42: >> chrome.send('setPassphrase', [$('passphrase-foo').value]); >> On 2013/01/07 14:51:49, Pam wrote: >> > Is chrome.send sufficiently robust to be sending a plaintext password >> across >> it? >> > I realize that JavaScript itself isn't guaranteed 100% attack-proof, but >> > this > >> > adds even more attack surface. Perhaps check with the security team? >> > > FWIW, sync also sends the passphrase in plaintext over chrome.send(). >> > > I also looked at how it is done in resources/chromeos/login/** > screen_gaia_sigin.js > and it seems to send the plaintext password with chrome.send as well. > > https://codereview.chromium.**org/11783008/<https://codereview.chromium.org/1... >
> > Yes, I guess it would be better to keep some state for authentication. But > > then > > we need to consider when the authenticated state should change back to the > > non-authenticated state, either automatically when closing some window with > > access restriction, or manually when the user clicks on some kind of lock. > > Could > > the InManagedMode preference be used for that, or do we need something > > separate? > > > Well, kInManagedMode is at the moment at global preference, so we would > need to add something in the user preferences. At that point, we should > probably name it "authenticated" or something, to get away from the "mode" > term. But yes, we definitely need some a way to change back. Another idea > would be to store the authenticated flag per WebUI, so it would be cleared > if the user closes the tab. I guess we'll have to play around it with a bit. We'll need something like that for the history auditing anyway. For the settings page, ideally we'd disable it entirely, not even show the page, if it's not unlocked (authenticated). Or at least disable all the controls. - Pam
On Wed, Jan 9, 2013 at 12:52 PM, <pam@chromium.org> wrote: > > Yes, I guess it would be better to keep some state for authentication. >> But >> > then >> > we need to consider when the authenticated state should change back to >> the >> > non-authenticated state, either automatically when closing some window >> with >> > access restriction, or manually when the user clicks on some kind of >> lock. >> > Could >> > the InManagedMode preference be used for that, or do we need something >> > separate? >> > > > Well, kInManagedMode is at the moment at global preference, so we would >> need to add something in the user preferences. At that point, we should >> probably name it "authenticated" or something, to get away from the "mode" >> term. But yes, we definitely need some a way to change back. Another idea >> would be to store the authenticated flag per WebUI, so it would be cleared >> if the user closes the tab. I guess we'll have to play around it with a >> bit. >> > > We'll need something like that for the history auditing anyway. > > For the settings page, ideally we'd disable it entirely, not even show the > page, > if it's not unlocked (authenticated). Or at least disable all the controls. > Hm. We should definitely have a check when settings are changed, and if that works, everything else in the UI is just cosmetic. So we could disable navigating to the managed user settings page if we're not authenticated (if that doesn't turn out to be too much work), for example, but I wouldn't feel it's necessary as long as there is no chance a user could accidentally end up there. > - Pam > > https://codereview.chromium.**org/11783008/<https://codereview.chromium.org/1... >
On 2013/01/09 11:58:50, Bernhard Bauer wrote: > Hm. We should definitely have a check when settings are changed, and if > that works, everything else in the UI is just cosmetic. So we could disable > navigating to the managed user settings page if we're not authenticated (if > that doesn't turn out to be too much work), for example, but I wouldn't > feel it's necessary as long as there is no chance a user could accidentally > end up there. > Right, all the UI changes are in addition to a robust back-end check, not instead of that.
On 2013/01/08 17:47:46, Bernhard Bauer wrote:
> > > chrome.send('DialogClose', ['correct']);
> > > Do you need to pass a string constant? Could you pass a boolean instead?
> >
> > Yes, I think a String is required (or no parameter at all, but I already use
> > that for Cancel), but it could be an empty string as in
> > cloud_print_setup_login.js. Now I used 'true' as a string constant.
>
> Are you sure that an actual string is required? Do you have a check that fails
> when you pass a boolean?
I both tried it, and looked at the code. In the method OnDialogCloseMessage in
constrained_web_dialog_ui.cc the argument is parsed as a string.
> > > Filename nit: the _webui at the end is probably not necessary (this dialog
> > only
> > > exists in WebUI, and it's already in the path).
> >
> > I could change it, but the tab_modal_confirm_dialog_webui had the same
naming,
> > so I thought I should do it the same way. So far, I didn't change it.
>
> Eh... TabModalConfirmDialogWebUI is a platform-specific implementation of a
> superclass. This class stands on its on.
Now I removed the WebUI (and the _webui from the file names).
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_passphrase.cc (right):
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_passphrase.cc:30: std::string bytes(32,
'\0');
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Can you pull the size out into a constant as well? Also, the null bytes
probably
> aren't necessary, as RandBytes will write over them anyway (and WriteInto will
> reserve the space).
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_passphrase.cc:31:
crypto::RandBytes(WriteInto(&bytes, bytes.size()), bytes.size());
Yes, you are right. The question is: which real salt size do we want?
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod...
File chrome/browser/managed_mode/managed_user_passphrase.h (right):
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_passphrase.h:20: std::string GetSalt();
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Newlines before comments would be nice.
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_passphrase.h:25: std::string*
encoded_passphrase_hash);
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Align the parameter please.
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod...
chrome/browser/managed_mode/managed_user_passphrase.h:29: std::string*
passphrase_hash);
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Align please.
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m...
File chrome/browser/resources/managed_user_passphrase_dialog.css (right):
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m...
chrome/browser/resources/managed_user_passphrase_dialog.css:35: padding: 5px
15px;
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Can you move this into the selector above as well?
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m...
File chrome/browser/resources/managed_user_passphrase_dialog.html (right):
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m...
chrome/browser/resources/managed_user_passphrase_dialog.html:16: <div
id="chrome-kid-logo"></div>
Done. This was caused by my bad configuration of gvim that I used shortly.
Should not happen again.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m...
chrome/browser/resources/managed_user_passphrase_dialog.html:27: <!--
TODO(akuegel): Create a link to a page which explains how to reset the
I guess there were some tabs in there, too, they are now replaced by two spaces
each so this comments is aligned to the previous closing </div>.
Pam suggested to take the link out, so I did.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m...
chrome/browser/resources/managed_user_passphrase_dialog.html:37:
i18n-content="cancel_passphrase_button"></button>
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Indent four space w/r/t previous line.
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/m...
chrome/browser/resources/managed_user_passphrase_dialog.html:40:
i18n-content="unlock_passphrase_button" disabled></button>
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Indentation
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o...
File chrome/browser/resources/options/managed_user_set_passphrase.css (right):
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o...
chrome/browser/resources/options/managed_user_set_passphrase.css:13:
.equal-passphrases {
Ok, I changed that.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o...
File chrome/browser/resources/options/managed_user_set_passphrase.js (right):
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o...
chrome/browser/resources/options/managed_user_set_passphrase.js:46:
$('passphrase-mismatch').hidden = false;
Not sure how I could do that. I haven't done much with css before, and I guess
this would need some advanced stuff.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o...
chrome/browser/resources/options/managed_user_set_passphrase.js:47:
$('passphrase-confirm').setAttribute('class', 'unequal-passphrases');
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> You can use .classList
> (https://developer.mozilla.org/en-US/docs/DOM/element.classList) here.
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o...
chrome/browser/resources/options/managed_user_set_passphrase.js:50: else {
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Move to previous line
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o...
chrome/browser/resources/options/managed_user_set_passphrase.js:51:
$('save-passphrase').disabled = ($('passphrase-confirm').value == '' ||
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> I think this one isn't necessary (if one of them is empty but the other one
> isn't, we wouldn't be in this branch anyway).
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/ma...
File chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc (right):
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/ma...
chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:64:
DCHECK(pref);
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> The pref checks are unnecessary again.
>
> Oh, and you don't actually need to go via FindPreference(), you can directly
> call GetString().
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/ma...
chrome/browser/ui/webui/managed_user_passphrase_dialog_webui.cc:76:
&encoded_passphrase_hash);
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Align with previous parameter, please.
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right):
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:58: if
(success)
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> I think I would actually plumb the success through to the Web UI. You might
want
> to do different things in the UI if authentication fails.
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:66:
args->Get(0, &callback_arg);
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Here you can use ListValue::GetString().
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:68:
content::WebContents* web_contents = web_ui()->GetWebContents();
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> You could probably inline this.
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:83:
passphrase_key_generator.GenerateHashFromPassphrase(passphrase,
On 2013/01/08 17:43:14, Bernhard Bauer wrote:
> Align the parameters please (here and below).
Done.
https://codereview.chromium.org/11783008/diff/3005/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):
https://codereview.chromium.org/11783008/diff/3005/chrome/common/pref_names.h...
chrome/common/pref_names.h:698: // Used for the managed mode to calculate and
store a hash of the password
In this case, they seem to be in both cc and h files, and actually there are
more of them in the pref_names.cc file than in the pref_names.h file. So where
should I remove it?
Needs BUG= and TEST= lines. (And tests.) https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/managed_user_set_passphrase.js:42: chrome.send('setPassphrase', [$('passphrase-foo').value]); On 2013/01/08 17:43:14, Bernhard Bauer wrote: > On 2013/01/07 14:51:49, Pam wrote: > > Is chrome.send sufficiently robust to be sending a plaintext password across > it? > > I realize that JavaScript itself isn't guaranteed 100% attack-proof, but this > > adds even more attack surface. Perhaps check with the security team? > > FWIW, sync also sends the passphrase in plaintext over chrome.send(). Fair enough, then. https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:55: // Sets the boolean value if a custodian is currently authenticated or not nit: Period at the end of the comment please. https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_user_passphrase.cc (right): https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase.cc:14: const int kNumberOfIterations = 1; I know it seems obvious, but please give a brief comment explaining what this is used for. https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase.cc:58: kDerivedKeySize)); These are reversed from the order they were in as literal numbers. Is that a deliberate change? https://codereview.chromium.org/11783008/diff/40001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right): https://codereview.chromium.org/11783008/diff/40001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:96: "ManagedUserSettings.isPassphraseSet", Move onto previous line, then align next parameter? https://codereview.chromium.org/11783008/diff/40001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11783008/diff/40001/chrome/common/pref_names.... chrome/common/pref_names.cc:2077: // This preference is used to store the hash of a password of the custodian of This should go up with the white-/blacklist prefs. https://codereview.chromium.org/11783008/diff/40001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11783008/diff/40001/chrome/common/pref_names.... chrome/common/pref_names.h:700: // Used for the managed mode to calculate and store a hash of the password Up with the kManagedMode*List names
First, sorry that I did not respond earlier. I had the flu, so when I was not taking a Noogler class I was using the time to recover. On 2013/01/14 15:37:50, Pam wrote: > Needs BUG= and TEST= lines. (And tests.) I guess there is no BUG for this so far. So how do I create one? About TEST: I added a unit test for the managed_user_passphrase.cc (unfortunately when writing the line about the new patch, I added the word Handler which should not be there). I am not sure if the other classes like managed_user_passphrase_handler.cc and managed_user_passphrase_dialog.cc can be tested by unit tests. I guess there are browser tests needed for these. If yes, I would like to discuss with you how to best do this, for example what should they cover, etc. https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:55: // Sets the boolean value if a custodian is currently authenticated or not On 2013/01/14 15:37:50, Pam wrote: > nit: Period at the end of the comment please. Done. https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_user_passphrase.cc (right): https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase.cc:14: const int kNumberOfIterations = 1; On 2013/01/14 15:37:50, Pam wrote: > I know it seems obvious, but please give a brief comment explaining what this is > used for. Done. https://codereview.chromium.org/11783008/diff/40001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase.cc:58: kDerivedKeySize)); I had the order wrong first, Bernhard found the bug and told me offline. So the new order is a deliberate change. https://codereview.chromium.org/11783008/diff/40001/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right): https://codereview.chromium.org/11783008/diff/40001/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:96: "ManagedUserSettings.isPassphraseSet", On 2013/01/14 15:37:50, Pam wrote: > Move onto previous line, then align next parameter? Done. https://codereview.chromium.org/11783008/diff/40001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11783008/diff/40001/chrome/common/pref_names.... chrome/common/pref_names.cc:2077: // This preference is used to store the hash of a password of the custodian of On 2013/01/14 15:37:50, Pam wrote: > This should go up with the white-/blacklist prefs. Done. https://codereview.chromium.org/11783008/diff/40001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11783008/diff/40001/chrome/common/pref_names.... chrome/common/pref_names.h:700: // Used for the managed mode to calculate and store a hash of the password On 2013/01/14 15:37:50, Pam wrote: > Up with the kManagedMode*List names Done.
I added a browsertest for the ManagedUserPassphrase handler. Can you please take a look if this goes into the right direction? Do I need tests which cover how the passphrase is entered in the dialog? If yes, how can I do that?
I have now also written a browsertest for the ManagedUserSetPassphraseOverlay which covers the set passphrase flow. Is there any other test needed? Or are the tests I have written now sufficient?
Sorry for the delay! I was also sick. I think these tests are sufficient. https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_user_passphrase_unittest.cc (right): https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase_unittest.cc:11: // This test checks the class ManagedUserPassphraseTest when providing a salt It actually checks the ManagedUserPassphrase class, right? https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase_unittest.cc:13: TEST(ManagedUserPassphraseTest, WithSaltProvided) { Please add blank lines to make all these tests easier to read. It's partly personal style, but I'd put them before each internal comment, since those mark separate logical chunks. https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase_unittest.cc:60: std::string passphrase_hash; Once you've verified the presence of two different salts in the two passphrases, I think it's safe to skip the rest of this test, since it duplicates what's above. https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_user_service.cc (right): https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_service.cc:131: prefs->RegisterBooleanPref(prefs::kManagedModeLocked, What's the difference between locked == false and passphrase == ""? https://codereview.chromium.org/11783008/diff/72001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11783008/diff/72001/chrome/common/pref_names.... chrome/common/pref_names.cc:42: // a managed user. It allows to unlock options which should be not available to This use of "unlock" makes it sound like it's the opposite of the "locked" pref below. https://codereview.chromium.org/11783008/diff/72001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11783008/diff/72001/chrome/common/pref_names.... chrome/common/pref_names.h:708: extra blank line
https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_user_passphrase_unittest.cc (right): https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/28 13:53:27, Pam wrote: > 2013 Done. https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase_unittest.cc:11: // This test checks the class ManagedUserPassphraseTest when providing a salt Right, I fixed that now. https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase_unittest.cc:13: TEST(ManagedUserPassphraseTest, WithSaltProvided) { On 2013/01/28 13:53:27, Pam wrote: > Please add blank lines to make all these tests easier to read. It's partly > personal style, but I'd put them before each internal comment, since those mark > separate logical chunks. Done. https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_passphrase_unittest.cc:60: std::string passphrase_hash; True. It was actually just copy/paste, so it is definitely a duplicate test. I removed these lines now. https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_user_service.cc (right): https://codereview.chromium.org/11783008/diff/72001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_user_service.cc:131: prefs->RegisterBooleanPref(prefs::kManagedModeLocked, Logically there is no difference, it would have just made the sync easier when displaying the settings page. Anyway, after some thoughts I decided to remove this preference again and do manual syncing with a callback function. There was a callback function in place anyway which checks if the passphrase is set, so I only had to modify it slightly. https://codereview.chromium.org/11783008/diff/72001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11783008/diff/72001/chrome/common/pref_names.... chrome/common/pref_names.cc:42: // a managed user. It allows to unlock options which should be not available to As I removed the "locked" pref, I guess it is now ok. https://codereview.chromium.org/11783008/diff/72001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11783008/diff/72001/chrome/common/pref_names.... chrome/common/pref_names.h:708: Removed.
LGTM! - Pam
On 2013/01/09 12:10:04, Adrian Kuegel wrote:
> On 2013/01/08 17:47:46, Bernhard Bauer wrote:
> > > > chrome.send('DialogClose', ['correct']);
> > > > Do you need to pass a string constant? Could you pass a boolean instead?
> > >
> > > Yes, I think a String is required (or no parameter at all, but I already
use
> > > that for Cancel), but it could be an empty string as in
> > > cloud_print_setup_login.js. Now I used 'true' as a string constant.
> >
> > Are you sure that an actual string is required? Do you have a check that
fails
> > when you pass a boolean?
>
> I both tried it, and looked at the code. In the method OnDialogCloseMessage in
> constrained_web_dialog_ui.cc the argument is parsed as a string.
Oh, wait, the argument is actually supposed to be JSON, it's just that so far
it's ignored in every OnDialogClose() implementation. OK, then a string "true"
is correct (as it would get deserialized to a boolean value), even if we don't
bother to do it).
https://chromiumcodereview.appspot.com/11783008/diff/3005/chrome/browser/mana...
File chrome/browser/managed_mode/managed_user_passphrase.cc (right):
https://chromiumcodereview.appspot.com/11783008/diff/3005/chrome/browser/mana...
chrome/browser/managed_mode/managed_user_passphrase.cc:31:
crypto::RandBytes(WriteInto(&bytes, bytes.size()), bytes.size());
On 2013/01/09 12:10:05, Adrian Kuegel wrote:
> Yes, you are right. The question is: which real salt size do we want?
32 is fine. You probably should set kSaltSize to 32 though and then pass
kSaltSize+1 as the parameter to WriteInto.
https://chromiumcodereview.appspot.com/11783008/diff/3005/chrome/browser/reso...
File chrome/browser/resources/options/managed_user_set_passphrase.js (right):
https://chromiumcodereview.appspot.com/11783008/diff/3005/chrome/browser/reso...
chrome/browser/resources/options/managed_user_set_passphrase.js:46:
$('passphrase-mismatch').hidden = false;
On 2013/01/09 12:10:05, Adrian Kuegel wrote:
> Not sure how I could do that. I haven't done much with css before, and I guess
> this would need some advanced stuff.
Well, you wanna learn, right? ;-)
You can actually use more stuff from the HTML5 validation API (required fields,
custom error messages, :valid pseudo-attributes. See
http://jsbin.com/welcome/72023 for an example.
https://chromiumcodereview.appspot.com/11783008/diff/40001/chrome/browser/res...
File chrome/browser/resources/managed_user_passphrase_dialog.html (right):
https://chromiumcodereview.appspot.com/11783008/diff/40001/chrome/browser/res...
chrome/browser/resources/managed_user_passphrase_dialog.html:27: <!--
TODO(akuegel): Create a link to a page which explains how to reset the
I think this is 81 characters now.
https://chromiumcodereview.appspot.com/11783008/diff/40001/chrome/browser/res...
chrome/browser/resources/managed_user_passphrase_dialog.html:28: passphrase
No need to align with the TODO colon, a period at the end would be nice though.
@Bernhard: can you please take another look? https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_user_passphrase.cc (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_user_passphrase.cc:31: crypto::RandBytes(WriteInto(&bytes, bytes.size()), bytes.size()); On 2013/02/04 16:13:35, Bernhard Bauer wrote: > On 2013/01/09 12:10:05, Adrian Kuegel wrote: > > Yes, you are right. The question is: which real salt size do we want? > > 32 is fine. You probably should set kSaltSize to 32 though and then pass > kSaltSize+1 as the parameter to WriteInto. Done. https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/3005/chrome/browser/resources/o... chrome/browser/resources/options/managed_user_set_passphrase.js:46: $('passphrase-mismatch').hidden = false; I think it is not possible to set the hidden attribute in css depending on the valid attribute of the input field. In my specific case, in css I would first need to select this input field according to the valid attribute, then I would need to select the parent, and then the sibling of the parent which is the error div. Although there are selectors for siblings, it seems there is no selector for the parent. https://codereview.chromium.org/11783008/diff/40001/chrome/browser/resources/... File chrome/browser/resources/managed_user_passphrase_dialog.html (right): https://codereview.chromium.org/11783008/diff/40001/chrome/browser/resources/... chrome/browser/resources/managed_user_passphrase_dialog.html:27: <!-- TODO(akuegel): Create a link to a page which explains how to reset the Fixed. https://codereview.chromium.org/11783008/diff/40001/chrome/browser/resources/... chrome/browser/resources/managed_user_passphrase_dialog.html:28: passphrase On 2013/02/04 16:13:35, Bernhard Bauer wrote: > No need to align with the TODO colon, a period at the end would be nice though. Done.
Getting there :) https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... File chrome/browser/resources/options/managed_user_set_passphrase.html (right): https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.html:9: <span i18n-content="setPassphraseInstructions"></span> You could set the i18n-content property directly on the parent div (likewise below for passphraseMismatch). https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.html:12: <input id="managed-user-passphrase" type="password" align="center" What's the align=center for? Could you do that in CSS? https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:42: if ($('passphrase-confirm').value != $('managed-user-passphrase').value) { I think it would be nicer to pull the result of this check into a local variable and then set the other properties based on that variable. https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:48: else { On the same line as the closing brace, please :) https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:49: $('save-passphrase').disabled = $('passphrase-confirm').value == ''; Instead of directly checking the value of #passphrase-confirm, you could check its validity.valid attribute. That way you're not coupled to the fact that we require a value (instead, the logic is: the save button is enabled iff all input fields are valid). Also, I think it would be clearer to check #passphrase instead of passphrase-confirm (the values have to be equal for us to be in that branch, but it seems a bit clearer to me that we require the *passphrase* to be non-empty). https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:54: getPassphraseInput_: function() { Are these actually used?
New patch uploaded. https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... File chrome/browser/resources/options/managed_user_set_passphrase.html (right): https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.html:9: <span i18n-content="setPassphraseInstructions"></span> On 2013/02/05 12:52:23, Bernhard Bauer wrote: > You could set the i18n-content property directly on the parent div (likewise > below for passphraseMismatch). Done. https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.html:12: <input id="managed-user-passphrase" type="password" align="center" I think that was some temporary test, it is not needed anymore, so I just removed it. https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:49: $('save-passphrase').disabled = $('passphrase-confirm').value == ''; I restructured the code accordingly. Now I also don't need the if/else blocks, so the comment above is addressed automatically :) https://codereview.chromium.org/11783008/diff/92001/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:54: getPassphraseInput_: function() { Not really, just by the test functions. But they can return the elements directly, so I simplified it.
https://codereview.chromium.org/11783008/diff/93051/chrome/browser/resources/... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/93051/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:45: $('passphrase-confirm').setCustomValidity(valid ? '' : Move |valid| to a new line please. https://codereview.chromium.org/11783008/diff/93051/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:58: ManagedUserSetPassphraseOverlay.getPassphraseInput = function() { I'm wondering if we could stuff these into a separate object so it's clear they're for testing purposes only. https://codereview.chromium.org/11783008/diff/93051/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/managed_user_set_passphrase_browsertest.js (right): https://codereview.chromium.org/11783008/diff/93051/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/managed_user_set_passphrase_browsertest.js:6: var includeLine = '#include "chrome/browser/ui/webui/options/'; You could concatenate the strings directly instead of using += (or even using a local variable).
And another patch :-) https://codereview.chromium.org/11783008/diff/93051/chrome/browser/resources/... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/93051/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:45: $('passphrase-confirm').setCustomValidity(valid ? '' : On 2013/02/05 13:34:44, Bernhard Bauer wrote: > Move |valid| to a new line please. Done. https://codereview.chromium.org/11783008/diff/93051/chrome/browser/resources/... chrome/browser/resources/options/managed_user_set_passphrase.js:58: ManagedUserSetPassphraseOverlay.getPassphraseInput = function() { On 2013/02/05 13:34:44, Bernhard Bauer wrote: > I'm wondering if we could stuff these into a separate object so it's clear > they're for testing purposes only. Done. https://codereview.chromium.org/11783008/diff/93051/chrome/browser/ui/webui/o... File chrome/browser/ui/webui/options/managed_user_set_passphrase_browsertest.js (right): https://codereview.chromium.org/11783008/diff/93051/chrome/browser/ui/webui/o... chrome/browser/ui/webui/options/managed_user_set_passphrase_browsertest.js:6: var includeLine = '#include "chrome/browser/ui/webui/options/'; On 2013/02/05 13:34:44, Bernhard Bauer wrote: > You could concatenate the strings directly instead of using += (or even using a > local variable). Done.
https://codereview.chromium.org/11783008/diff/103001/chrome/browser/resources... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/103001/chrome/browser/resources... chrome/browser/resources/options/managed_user_set_passphrase.js:58: function ManagedUserSetPassphraseForTesting() { This can really just be an object. https://codereview.chromium.org/11783008/diff/103001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_set_passphrase_browsertest.js (right): https://codereview.chromium.org/11783008/diff/103001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_set_passphrase_browsertest.js:44: var instance = new options.ManagedUserSetPassphraseForTesting(); You don't need to create a new instance; just call the methods on options.ManagedUserSetPassphraseForTesting directly.
And another patch. https://codereview.chromium.org/11783008/diff/103001/chrome/browser/resources... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/103001/chrome/browser/resources... chrome/browser/resources/options/managed_user_set_passphrase.js:58: function ManagedUserSetPassphraseForTesting() { On 2013/02/05 16:21:32, Bernhard Bauer wrote: > This can really just be an object. Done. https://codereview.chromium.org/11783008/diff/103001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_set_passphrase_browsertest.js (right): https://codereview.chromium.org/11783008/diff/103001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_set_passphrase_browsertest.js:44: var instance = new options.ManagedUserSetPassphraseForTesting(); On 2013/02/05 16:21:32, Bernhard Bauer wrote: > You don't need to create a new instance; just call the methods on > options.ManagedUserSetPassphraseForTesting directly. Done.
LGTM!
@jhawkins: can I please get OWNERS approval?
On 2013/02/05 17:05:46, Adrian Kuegel wrote: > @jhawkins: can I please get OWNERS approval? For which files?
All files except the ones in chrome/browser/managed_mode/
Please attach screenshots. Also, would be really awesome if you could find a way to break this CL up. It's really quite large. https://codereview.chromium.org/11783008/diff/106001/chrome/browser/resources... File chrome/browser/resources/managed_user_passphrase_dialog.css (right): https://codereview.chromium.org/11783008/diff/106001/chrome/browser/resources... chrome/browser/resources/managed_user_passphrase_dialog.css:1: /* Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: It's not 2012 anymore.
Unfortunately I see no way how to significantly break this CL up. A possible split I see would be 3 files in one and all others in the rest. Here are a few screenshots: https://docs.google.com/a/google.com/drawings/d/1ZSy4TH1k-nOwgza8G-sHc_z_n6-y... https://docs.google.com/a/google.com/drawings/d/1jBhocl1LZ7xI9oMLeeVO_AOTJbc0... https://docs.google.com/a/google.com/drawings/d/1nOQ58i09wpI8Cky7rpy2oUiWmyg4... https://docs.google.com/a/google.com/drawings/d/1Pw05PSHxZVHK5RyvgxNv_6pE9JI9... https://docs.google.com/a/google.com/drawings/d/1LE6pZklMnBGwtbO6ULgOMK8B--BL...
On 2013/02/06 17:54:22, Adrian Kuegel wrote: > Unfortunately I see no way how to significantly break this CL up. A possible > split I see would be 3 files in one and all others in the rest. > > Here are a few screenshots: > https://docs.google.com/a/google.com/drawings/d/1ZSy4TH1k-nOwgza8G-sHc_z_n6-y... > > https://docs.google.com/a/google.com/drawings/d/1jBhocl1LZ7xI9oMLeeVO_AOTJbc0... > > https://docs.google.com/a/google.com/drawings/d/1nOQ58i09wpI8Cky7rpy2oUiWmyg4... > > https://docs.google.com/a/google.com/drawings/d/1Pw05PSHxZVHK5RyvgxNv_6pE9JI9... > > https://docs.google.com/a/google.com/drawings/d/1LE6pZklMnBGwtbO6ULgOMK8B--BL... Has anyone in UX seen these yet? They definitely need some polish. The changes in managed_mode/ can't be pulled into a different CL?
On 2013/02/06 17:58:33, James Hawkins wrote: > On 2013/02/06 17:54:22, Adrian Kuegel wrote: > > Unfortunately I see no way how to significantly break this CL up. A possible > > split I see would be 3 files in one and all others in the rest. > > > > Here are a few screenshots: > > > https://docs.google.com/a/google.com/drawings/d/1ZSy4TH1k-nOwgza8G-sHc_z_n6-y... > > > > > https://docs.google.com/a/google.com/drawings/d/1jBhocl1LZ7xI9oMLeeVO_AOTJbc0... > > > > > https://docs.google.com/a/google.com/drawings/d/1nOQ58i09wpI8Cky7rpy2oUiWmyg4... > > > > > https://docs.google.com/a/google.com/drawings/d/1Pw05PSHxZVHK5RyvgxNv_6pE9JI9... > > > > > https://docs.google.com/a/google.com/drawings/d/1LE6pZklMnBGwtbO6ULgOMK8B--BL... > > Has anyone in UX seen these yet? They definitely need some polish. Yes, we're working with UX on this. Note also that this feature is behind a flag, so the lack of polish isn't user-visible. That being said, do you have things that bother you? > The changes in managed_mode/ can't be pulled into a different CL? That, and setting the passphrase in managed user settings could be separated from showing the dialog that asks for it.
On 2013/02/06 18:18:26, Bernhard Bauer wrote: > On 2013/02/06 17:58:33, James Hawkins wrote: > > On 2013/02/06 17:54:22, Adrian Kuegel wrote: > > > Unfortunately I see no way how to significantly break this CL up. A possible > > > split I see would be 3 files in one and all others in the rest. > > > > > > Here are a few screenshots: > > > > > > https://docs.google.com/a/google.com/drawings/d/1ZSy4TH1k-nOwgza8G-sHc_z_n6-y... > > > > > > > > > https://docs.google.com/a/google.com/drawings/d/1jBhocl1LZ7xI9oMLeeVO_AOTJbc0... > > > > > > > > > https://docs.google.com/a/google.com/drawings/d/1nOQ58i09wpI8Cky7rpy2oUiWmyg4... > > > > > > > > > https://docs.google.com/a/google.com/drawings/d/1Pw05PSHxZVHK5RyvgxNv_6pE9JI9... > > > > > > > > > https://docs.google.com/a/google.com/drawings/d/1LE6pZklMnBGwtbO6ULgOMK8B--BL... > > > > Has anyone in UX seen these yet? They definitely need some polish. > > Yes, we're working with UX on this. Note also that this feature is behind a > flag, so the lack of polish isn't user-visible. > > That being said, do you have things that bother you? > In the 'enter passphrase' dialog, I'd add some margins between the controls. I suggest just sending the screenshots to UX and asking for quick feedback. It will likely be faster than waiting till it's all done. > > The changes in managed_mode/ can't be pulled into a different CL? > > That, and setting the passphrase in managed user settings could be separated > from showing the dialog that asks for it. Yes, that would be great if you could break it out.
I uploaded a new patch which is the remaining part after breaking everything else into other CLs. This CL depends on https://codereview.chromium.org/12218118/ in order to work, so the other CL has to be committed first.
https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... File chrome/browser/resources/options/managed_user_settings.js (right): https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:102: ManagedUserSettings.getInstance().updateControls(true); This would be clearer if the argument were flipped (enable rather than disable). https://codereview.chromium.org/11783008/diff/110001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.h (right): https://codereview.chromium.org/11783008/diff/110001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:36: // This function displays the passphrase dialog where the manager of the Nit: For brevity, I would remove all the "This function" pieces, and just start with (e.g.) "Displays the passphrase...". Also from SetLocalPassphrase, in that case.
I uploaded a new patch which addresses the review comments. Please take another look if it is ok now. https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... File chrome/browser/resources/options/managed_user_settings.js (right): https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:102: ManagedUserSettings.getInstance().updateControls(true); On 2013/02/15 09:31:18, Pam wrote: > This would be clearer if the argument were flipped (enable rather than disable). Done. I also renamed the function to enableControls, which should make it even more clearer. https://codereview.chromium.org/11783008/diff/110001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.h (right): https://codereview.chromium.org/11783008/diff/110001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:36: // This function displays the passphrase dialog where the manager of the On 2013/02/15 09:31:18, Pam wrote: > Nit: For brevity, I would remove all the "This function" pieces, and just start > with (e.g.) "Displays the passphrase...". Also from SetLocalPassphrase, in that > case. Done.
https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... File chrome/browser/resources/options/managed_user_settings.js (right): https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:34: authenticationChecked: false, I think a better name for this would be "isAuthenticated"? Also, would it make sense to combine these two flags into a tri-state variable (unauthenticated, checking, authenticated)? https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:58: if (!this.isAuthenticationChecking) { You could invert the condition and early-return. https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:76: $('set-passphrase').disabled = disable; Please don't simply set the disabled attribute. There might be other reasons why a control can be disabled, and so updating the attribute directly might interfere. Instead, there is a setDisabled() method on pref UI controls (in pref_ui.js). https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:88: chrome.send('isPassphraseSet', Instead of asking the browser asynchronously, could you just store this value on the Javascript side? https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:109: ManagedUserSettings.isAuthenticated = function(success) { I think it would be nicer if you delegate to an instance method on ManagedUserSettings. https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:118: ManagedUserSettings.isPassphraseSet = function(success) { |success| means whether the passphrase is set? Could use a clearer variable name.
I uploaded a new patch which is rebased to ToT which now includes the Passphrase Dialog. Can you please take another look? https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... File chrome/browser/resources/options/managed_user_settings.js (right): https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:34: authenticationChecked: false, Yes, good idea. I changed it like that. https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:58: if (!this.isAuthenticationChecking) { Done. https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:76: $('set-passphrase').disabled = disable; If I understand that right, I can only use this on pref UI controls. I guess a pref UI control is one which has a pref="..." in the HTML, so I modified it only for those controls. https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:88: chrome.send('isPassphraseSet', On 2013/02/15 10:05:20, Bernhard Bauer wrote: > Instead of asking the browser asynchronously, could you just store this value on > the Javascript side? Done. https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:109: ManagedUserSettings.isAuthenticated = function(success) { On 2013/02/15 10:05:20, Bernhard Bauer wrote: > I think it would be nicer if you delegate to an instance method on > ManagedUserSettings. Done. https://codereview.chromium.org/11783008/diff/110001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:118: ManagedUserSettings.isPassphraseSet = function(success) { On 2013/02/15 10:05:20, Bernhard Bauer wrote: > |success| means whether the passphrase is set? Could use a clearer variable > name. Done.
https://codereview.chromium.org/11783008/diff/123001/chrome/browser/resources... File chrome/browser/resources/options/managed_user_settings.js (right): https://codereview.chromium.org/11783008/diff/123001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:31: var Authentication = { I think we should prefix this with ManagedUser? https://codereview.chromium.org/11783008/diff/123001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_settings_handler.h (right): https://codereview.chromium.org/11783008/diff/123001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_settings_handler.h:38: StringPrefMember passphrase_; You don't actually need a PrefMember here. Just use a PrefChangeRegistrar and fetch the pref value from the PrefService when needed.
https://codereview.chromium.org/11783008/diff/123001/chrome/browser/resources... File chrome/browser/resources/options/managed_user_settings.js (right): https://codereview.chromium.org/11783008/diff/123001/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:31: var Authentication = { On 2013/02/20 13:06:20, Bernhard Bauer wrote: > I think we should prefix this with ManagedUser? Done. https://codereview.chromium.org/11783008/diff/123001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_settings_handler.h (right): https://codereview.chromium.org/11783008/diff/123001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_settings_handler.h:38: StringPrefMember passphrase_; On 2013/02/20 13:06:20, Bernhard Bauer wrote: > You don't actually need a PrefMember here. Just use a PrefChangeRegistrar and > fetch the pref value from the PrefService when needed. Done.
lgtm
https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_set_passphrase.js:51: /** @override */ nit: Blank line above this. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... File chrome/browser/resources/options/managed_user_settings.js (right): https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:31: var ManagedUserAuthentication = { nit: Document enum. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:49: // Stores if the local passphrase of the manager of the managed account is nit: // True if the local passphrase... https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:60: var self = this; nit: Move this var to right above where it's used. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:82: /** @override */ nit: Add one blank line above each method where none currently exists. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:93: enableControls: function(enable) { nit: Document new methods and params. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right): https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:91: // This is deleted automatically when the dialog is closed. What does 'this' refer to? https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.h (right): https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:31: // Used to set the passphrase of the manager of the managed account. The nit: // Sets the passphrase... https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:41: // Will be called after the user either clicked Cancel or successfully entered nit: Document what it does, not when it's called. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:56: // Used to save the name of the Javascript function which should be called nit: // The name of the...
Thanks for the review, James :-) I uploaded a new patch to address your comments. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... File chrome/browser/resources/options/managed_user_set_passphrase.js (right): https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_set_passphrase.js:51: /** @override */ On 2013/02/20 17:24:56, James Hawkins wrote: > nit: Blank line above this. Done. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... File chrome/browser/resources/options/managed_user_settings.js (right): https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:31: var ManagedUserAuthentication = { On 2013/02/20 17:24:56, James Hawkins wrote: > nit: Document enum. Done. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:49: // Stores if the local passphrase of the manager of the managed account is On 2013/02/20 17:24:56, James Hawkins wrote: > nit: // True if the local passphrase... Done. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:60: var self = this; On 2013/02/20 17:24:56, James Hawkins wrote: > nit: Move this var to right above where it's used. Done. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:82: /** @override */ On 2013/02/20 17:24:56, James Hawkins wrote: > nit: Add one blank line above each method where none currently exists. Done. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/resources... chrome/browser/resources/options/managed_user_settings.js:93: enableControls: function(enable) { On 2013/02/20 17:24:56, James Hawkins wrote: > nit: Document new methods and params. Done. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc (right): https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.cc:91: // This is deleted automatically when the dialog is closed. It refers to the ManagedUserPassphraseDialog object. Maybe it is better if I remove the word This? https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/managed_user_passphrase_handler.h (right): https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:31: // Used to set the passphrase of the manager of the managed account. The On 2013/02/20 17:24:56, James Hawkins wrote: > nit: // Sets the passphrase... Done. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:41: // Will be called after the user either clicked Cancel or successfully entered On 2013/02/20 17:24:56, James Hawkins wrote: > nit: Document what it does, not when it's called. Done. https://codereview.chromium.org/11783008/diff/131002/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/managed_user_passphrase_handler.h:56: // Used to save the name of the Javascript function which should be called On 2013/02/20 17:24:56, James Hawkins wrote: > nit: // The name of the... Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/11783008/123006
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/11783008/123006
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/11783008/127005
Message was sent while issue was closed.
Change committed as 184206 |
