Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9432003:
Disable multi-profile UI in managed mode. (Closed)
|
Created:
8 years, 10 months ago by Bernhard Bauer Modified:
8 years, 9 months ago CC:
chromium-reviews, rginda+watch_chromium.org, achuith+watch_chromium.org, Joao da Silva, Chris Evans Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDisable multi-profile UI in managed mode.
If the browser is in managed mode, certain preferences in the profile can be restricted, so we need to make sure that the user can't switch to a different profile or add a new one.
BUG=115103
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125264
Patch Set 1 #Patch Set 2 : add notreached() #
Total comments: 7
Patch Set 3 : review #
Total comments: 4
Patch Set 4 : review #Patch Set 5 : review #
Total comments: 2
Patch Set 6 : fix #Patch Set 7 : revert #Patch Set 8 : git try --root src #Patch Set 9 : . #
Total comments: 3
Patch Set 10 : review #
Messages
Total messages: 31 (0 generated)
Clicked the wrong link in the email and had a look at this one too. Here are a couple of comments :-) http://codereview.chromium.org/9432003/diff/5003/chrome/browser/profiles/avat... File chrome/browser/profiles/avatar_menu_model.cc (right): http://codereview.chromium.org/9432003/diff/5003/chrome/browser/profiles/avat... chrome/browser/profiles/avatar_menu_model.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. Nit: copyright year http://codereview.chromium.org/9432003/diff/5003/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/9432003/diff/5003/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options2/browser_options_handler2.cc:700: if (!g_browser_process->local_state()->GetBoolean(prefs::kInManagedMode)) { Is this really a !InManagedMode? http://codereview.chromium.org/9432003/diff/5003/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options2/manage_profile_handler2.cc (right): http://codereview.chromium.org/9432003/diff/5003/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options2/manage_profile_handler2.cc:206: if (!g_browser_process->local_state()->GetBoolean(prefs::kInManagedMode)) { Same?
http://codereview.chromium.org/9432003/diff/5003/chrome/browser/profiles/avat... File chrome/browser/profiles/avatar_menu_model.cc (right): http://codereview.chromium.org/9432003/diff/5003/chrome/browser/profiles/avat... chrome/browser/profiles/avatar_menu_model.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/02/23 12:05:14, Joao da Silva wrote: > Nit: copyright year That's fixed up by the commit queue automatically, so I didn't bother to do it manually (and if I should land this CL manually and forget to update the header, it will trigger a presubmit check). http://codereview.chromium.org/9432003/diff/5003/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/9432003/diff/5003/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options2/browser_options_handler2.cc:700: if (!g_browser_process->local_state()->GetBoolean(prefs::kInManagedMode)) { On 2012/02/23 12:05:14, Joao da Silva wrote: > Is this really a !InManagedMode? Yeah, that's backwards :-D http://codereview.chromium.org/9432003/diff/5003/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options2/manage_profile_handler2.cc (right): http://codereview.chromium.org/9432003/diff/5003/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options2/manage_profile_handler2.cc:206: if (!g_browser_process->local_state()->GetBoolean(prefs::kInManagedMode)) { On 2012/02/23 12:05:14, Joao da Silva wrote: > Same? Yeah (you can tell I didn't test this patch set yet :-D )
lgtm. I trust you'll test the patch before landing though :-) http://codereview.chromium.org/9432003/diff/5003/chrome/browser/profiles/avat... File chrome/browser/profiles/avatar_menu_model.cc (right): http://codereview.chromium.org/9432003/diff/5003/chrome/browser/profiles/avat... chrome/browser/profiles/avatar_menu_model.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/02/23 12:14:36, Bernhard Bauer wrote: > On 2012/02/23 12:05:14, Joao da Silva wrote: > > Nit: copyright year > > That's fixed up by the commit queue automatically, so I didn't bother to do it > manually (and if I should land this CL manually and forget to update the header, > it will trigger a presubmit check). Didn't know that, cool.
On 2012/02/23 15:28:18, Joao da Silva wrote: > lgtm. I trust you'll test the patch before landing though :-) Isn't that what the commit queue is good for? ;-) Adding James for WebUI options.
On 2012/02/23 16:27:00, Bernhard Bauer wrote: > On 2012/02/23 15:28:18, Joao da Silva wrote: > > lgtm. I trust you'll test the patch before landing though :-) > > Isn't that what the commit queue is good for? ;-) > > Adding James for WebUI options. James, ping? :)
lgtm http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options2/browser_options_handler2.cc:702: return; Don't 'handle' NOTREACHEDs by returning.
Oops, I didn't mean to LG yet :-/ On 2012/03/01 12:42:33, James Hawkins wrote: > lgtm > > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... > File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): > > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... > chrome/browser/ui/webui/options2/browser_options_handler2.cc:702: return; > Don't 'handle' NOTREACHEDs by returning.
http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options2/browser_options_handler2.cc:702: return; On 2012/03/01 12:42:33, James Hawkins wrote: > Don't 'handle' NOTREACHEDs by returning. Hm, what do you propose? Silently returning or CHECK? FTR, I see both `NOTREACHED(); return;` as well as `CHECK` in this file.
http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options2/browser_options_handler2.cc:702: return; On 2012/03/01 16:08:32, Bernhard Bauer wrote: > On 2012/03/01 12:42:33, James Hawkins wrote: > > Don't 'handle' NOTREACHEDs by returning. > > Hm, what do you propose? Silently returning or CHECK? FTR, I see both > `NOTREACHED(); return;` as well as `CHECK` in this file. If it really should not fail, use NOTREACHED and don't return at all. http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options2/browser_options_handler2.cc:702: return; On 2012/03/01 19:51:36, James Hawkins wrote: > On 2012/03/01 16:08:32, Bernhard Bauer wrote: > > On 2012/03/01 12:42:33, James Hawkins wrote: > > > Don't 'handle' NOTREACHEDs by returning. > > > > Hm, what do you propose? Silently returning or CHECK? FTR, I see both > > `NOTREACHED(); return;` as well as `CHECK` in this file. > > If it really should not fail, use NOTREACHED and don't return at all. > > http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... It can happen, in the case where a malicious user gets the chrome://settings page to call this method when it shouldn't be called, so I don't want to just DCHECK here. I guess I can silently return (and add a comment why)?
+cevans On 2012/03/01 23:02:16, Bernhard Bauer wrote: > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... > File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): > > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... > chrome/browser/ui/webui/options2/browser_options_handler2.cc:702: return; > On 2012/03/01 19:51:36, James Hawkins wrote: > > On 2012/03/01 16:08:32, Bernhard Bauer wrote: > > > On 2012/03/01 12:42:33, James Hawkins wrote: > > > > Don't 'handle' NOTREACHEDs by returning. > > > > > > Hm, what do you propose? Silently returning or CHECK? FTR, I see both > > > `NOTREACHED(); return;` as well as `CHECK` in this file. > > > > If it really should not fail, use NOTREACHED and don't return at all. > > > > > http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... > > It can happen, in the case where a malicious user gets the chrome://settings > page to call this method when it shouldn't be called, so I don't want to just > DCHECK here. > > I guess I can silently return (and add a comment why)? Hmm, this is the second time I've heard this argument for this pattern. It sounds like silently failing in this case is sub-optimal...can (should) we crash the render process knowing this should only happen if the page is compromised?
On Fri Mar 02 23:58:03 GMT+100 2012, <jhawkins@chromium.org> wrote: > +cevans > > On 2012/03/01 23:02:16, Bernhard Bauer wrote: > > > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... > > File chrome/browser/ui/webui/options2/browser_options_handler2.cc > (right): > > > > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti... > > chrome/browser/ui/webui/options2/browser_options_handler2.cc:702: return; > > On 2012/03/01 19:51:36, James Hawkins wrote: > > > On 2012/03/01 16:08:32, Bernhard Bauer wrote: > > > > On 2012/03/01 12:42:33, James Hawkins wrote: > > > > > Don't 'handle' NOTREACHEDs by returning. > > > > > > > > Hm, what do you propose? Silently returning or CHECK? FTR, I see both > > > > `NOTREACHED(); return;` as well as `CHECK` in this file. > > > > > > If it really should not fail, use NOTREACHED and don't return at all. > > > > > > > > > http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... > > > It can happen, in the case where a malicious user gets the > > chrome://settings > > page to call this method when it shouldn't be called, so I don't want to > > just > > DCHECK here. > > > I guess I can silently return (and add a comment why)? > > Hmm, this is the second time I've heard this argument for this pattern. It > sounds like silently failing in this case is sub-optimal...can (should) we > crash > the render process knowing this should only happen if the page is > compromised? > Yeah, the other time was me as well ;-) To give you some context here, I'm trying to prevent someone from bypassing the disabled multiprofile controls by e.g. simply executing some Javascript in the context of chrome://settings via the web inspector. I'm specifically not worried about someone using an exploit to hijack the renderer, because that kind of user would not require a managed mode in the first place :-D http://codereview.chromium.org/9432003/<https://www.google.com/url?sa=D&q=htt... > >
On 2012/03/02 23:49:30, please use chromium account wrote: > On Fri Mar 02 23:58:03 GMT+100 2012, <mailto:jhawkins@chromium.org> wrote: > > > +cevans > > > > On 2012/03/01 23:02:16, Bernhard Bauer wrote: > > > > > > > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti...> > > > File chrome/browser/ui/webui/options2/browser_options_handler2.cc > > (right): > > > > > > > > > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti...> > > > chrome/browser/ui/webui/options2/browser_options_handler2.cc:702: return; > > > On 2012/03/01 19:51:36, James Hawkins wrote: > > > > On 2012/03/01 16:08:32, Bernhard Bauer wrote: > > > > > On 2012/03/01 12:42:33, James Hawkins wrote: > > > > > > Don't 'handle' NOTREACHEDs by returning. > > > > > > > > > > Hm, what do you propose? Silently returning or CHECK? FTR, I see both > > > > > `NOTREACHED(); return;` as well as `CHECK` in this file. > > > > > > > > If it really should not fail, use NOTREACHED and don't return at all. > > > > > > > > > > > > > > > http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...> > > > > > It can happen, in the case where a malicious user gets the > > > chrome://settings > > > page to call this method when it shouldn't be called, so I don't want to > > > just > > > DCHECK here. > > > > > I guess I can silently return (and add a comment why)? > > > > Hmm, this is the second time I've heard this argument for this pattern. It > > sounds like silently failing in this case is sub-optimal...can (should) we > > crash > > the render process knowing this should only happen if the page is > > compromised? > > > > Yeah, the other time was me as well ;-) To give you some context here, I'm > trying to prevent someone from bypassing the disabled multiprofile controls > by e.g. simply executing some Javascript in the context of > chrome://settings via the web inspector. I'm specifically not worried about > someone using an exploit to hijack the renderer, because that kind of user > would not require a managed mode in the first place :-D > OK, let's silently drop it then. Add a comment to this effect.
On 2012/03/03 00:25:26, James Hawkins wrote: > On 2012/03/02 23:49:30, please use chromium account wrote: > > On Fri Mar 02 23:58:03 GMT+100 2012, <mailto:jhawkins@chromium.org> wrote: > > > > > +cevans > > > > > > On 2012/03/01 23:02:16, Bernhard Bauer wrote: > > > > > > > > > > > > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti...> > > > > File chrome/browser/ui/webui/options2/browser_options_handler2.cc > > > (right): > > > > > > > > > > > > > > > http://codereview.chromium.org/9432003/diff/9003/chrome/browser/ui/webui/opti...> > > > > chrome/browser/ui/webui/options2/browser_options_handler2.cc:702: return; > > > > On 2012/03/01 19:51:36, James Hawkins wrote: > > > > > On 2012/03/01 16:08:32, Bernhard Bauer wrote: > > > > > > On 2012/03/01 12:42:33, James Hawkins wrote: > > > > > > > Don't 'handle' NOTREACHEDs by returning. > > > > > > > > > > > > Hm, what do you propose? Silently returning or CHECK? FTR, I see both > > > > > > `NOTREACHED(); return;` as well as `CHECK` in this file. > > > > > > > > > > If it really should not fail, use NOTREACHED and don't return at all. > > > > > > > > > > > > > > > > > > > > > > http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...> > > > > > > > It can happen, in the case where a malicious user gets the > > > > chrome://settings > > > > page to call this method when it shouldn't be called, so I don't want to > > > > just > > > > DCHECK here. > > > > > > > I guess I can silently return (and add a comment why)? > > > > > > Hmm, this is the second time I've heard this argument for this pattern. It > > > sounds like silently failing in this case is sub-optimal...can (should) we > > > crash > > > the render process knowing this should only happen if the page is > > > compromised? > > > > > > > Yeah, the other time was me as well ;-) To give you some context here, I'm > > trying to prevent someone from bypassing the disabled multiprofile controls > > by e.g. simply executing some Javascript in the context of > > chrome://settings via the web inspector. I'm specifically not worried about > > someone using an exploit to hijack the renderer, because that kind of user > > would not require a managed mode in the first place :-D > > > > OK, let's silently drop it then. Add a comment to this effect. Done. Sailesh, can I get an OWNERS review for profiles/? Note that the changes to chrome/browser/profiles/profile_manager.cc are the same as in Pam's http://codereview.chromium.org/9566007/.
lgtm
Hi Bernhard, could you add the following information to the CL and the issue: - what is managed mode? when will it be set and when will it not be set? - why disable multi-profiles in managed mode? In your CL you're sprinkling the check for managed mode in multiple places. Can you consolidate it into ProfileManager::IsMultipleProfilesEnabled() instead?
+rsesek since he wrote AvatarMenuModel
lgtm http://codereview.chromium.org/9432003/diff/29001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/9432003/diff/29001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:721: prefs->RegisterBooleanPref(prefs::kInManagedMode, false); Is this pref not already registered?
On 2012/03/05 18:30:52, sail wrote: > Hi Bernhard, could you add the following information to the CL and the issue: > - what is managed mode? when will it be set and when will it not be set? > - why disable multi-profiles in managed mode? OK, I updated the CL description to explain why we need to disable multi-profile UI in managed mode, and added a link to some design documents to the bug to provide background. > In your CL you're sprinkling the check for managed mode in multiple places. Can > you consolidate it into ProfileManager::IsMultipleProfilesEnabled() instead? Sure, I'll do that tomorrow (and sync with Pam). http://codereview.chromium.org/9432003/diff/29001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/9432003/diff/29001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:721: prefs->RegisterBooleanPref(prefs::kInManagedMode, false); On 2012/03/05 18:33:49, rsesek wrote: > Is this pref not already registered? I forgot that in the previous CL :-(
OK, I rebased my CL on top of Pam's and moved the managed mode check into ProfileManager::IsMultipleProfilesEnabled. Sailesh, could I get an LGTM for profiles/? Thanks!
Do you need any addition code to track the status of the pref and update the ui if it changes? if so you might be able to simplify things by posting a profile info cache changed notification. Most of the profile ui already listens for this notification. On Mar 6, 2012 9:51 AM, <bauerb@chromium.org> wrote: > OK, I rebased my CL on top of Pam's and moved the managed mode check into > ProfileManager::**IsMultipleProfilesEnabled. > > Sailesh, could I get an LGTM for profiles/? Thanks! > > http://codereview.chromium.**org/9432003/<http://codereview.chromium.org/9432... >
Yeah, so far I've been using a separate notification, but they really are used mostly in the same places. Thanks! On Tue Mar 06 18:59:10 GMT+100 2012, Sailesh Agrawal <sail@google.com> wrote: > Do you need any addition code to track the status of the pref and update > the ui if it changes? > > if so you might be able to simplify things by posting a profile info cache > changed notification. Most of the profile ui already listens for this > notification. > On Mar 6, 2012 9:51 AM, <bauerb@chromium.org> wrote: > > OK, I rebased my CL on top of Pam's and moved the managed mode check into > ProfileManager::**IsMultipleProfilesEnabled. > > Sailesh, could I get an LGTM for profiles/? Thanks! > > http://codereview.chromium.**org/9432003/<http://codereview.chromium.org/9432... > >
http://codereview.chromium.org/9432003/diff/39029/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/9432003/diff/39029/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/browser_options_handler2.cc:1007: if (g_browser_process->local_state()->GetBoolean(prefs::kInManagedMode)) use ProfileManager::IsMultipleProfilesEnabled() instead? http://codereview.chromium.org/9432003/diff/39029/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options2/manage_profile_handler2.cc (right): http://codereview.chromium.org/9432003/diff/39029/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/manage_profile_handler2.cc:208: if (g_browser_process->local_state()->GetBoolean(prefs::kInManagedMode)) use ProfileManager::IsMultipleProfilesEnabled() instead?
http://codereview.chromium.org/9432003/diff/39029/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right): http://codereview.chromium.org/9432003/diff/39029/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/browser_options_handler2.cc:1007: if (g_browser_process->local_state()->GetBoolean(prefs::kInManagedMode)) On 2012/03/06 19:12:50, sail wrote: > use ProfileManager::IsMultipleProfilesEnabled() instead? Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bauerb@chromium.org/9432003/44004
Try job failure for 9432003-44004 (retry) on linux_rel for steps "safe_browsing_tests, browser_tests, ui_tests". It's a second try, previously, steps "safe_browsing_tests, browser_tests, ui_tests" failed. 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/bauerb@chromium.org/9432003/44004
Try job failure for 9432003-44004 (retry) on linux_clang for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bauerb@chromium.org/9432003/44004
Change committed as 125264 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
