Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15738004:
Add a policy list for access to capture devices (Closed)
|
Created:
7 years, 7 months ago by tommi (sloooow) - chröme Modified:
7 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, joaodasilva+watch_chromium.org, battre Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a policy list for access to capture devices.
The approach is slightly different from my proposal in the bug so that we have more configuration options as well as backwards compatibility with Chrome M25.
This change introduces two new policy lists that allow admins to whitelist URLs to get no-prompt access to capture devices via URLs or URL patterns.
The pattern matching is applied only to the security origin of the URLs.
The behavior of the existing AudioCaptureAllowed and VideoCaptureAllowed is reverted back to how it was prior to r180416. This means that admins must use the whitelist to allow device access without prompt.
If no match for a URL is found in the whitelists, the default behavior is determined by the [Audio|Video]CaptureAllowed policy value.
BUG=225045
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202572
Patch Set 1 #Patch Set 2 : Change approach to make the configuration more flexible and backwards compatible. #Patch Set 3 : Add tests and improve documentation #
Total comments: 8
Patch Set 4 : Change capture policy values to be per-profile. #
Total comments: 2
Patch Set 5 : Use ContentSettingsPattern instead of MatchPattern. #
Total comments: 9
Patch Set 6 : Address comments #Patch Set 7 : Only allow the whitelist policy when kioskmode is enabled. #
Total comments: 2
Patch Set 8 : Rebase #Patch Set 9 : Ignore wildcard entries #
Total comments: 1
Patch Set 10 : Check for kiosk mode on ChromeOS #Patch Set 11 : initialize more cros singletons #Patch Set 12 : Back out of mocking the world for cros #
Total comments: 17
Patch Set 13 : Address commentsDD #Patch Set 14 : Updated policy_test_cases #
Total comments: 2
Patch Set 15 : Reference bug for code cleanup #Messages
Total messages: 41 (0 generated)
Hey Mattias and Julian. I hope this approach will satisfy the requirements. Feel free to add more people to the review.
I don't see issues with the policy structure but before I stamp it please run the new policy structure description as well as the user-facing policy description texts on the bug for the wider audience there to see and approve to avoid more issues with this set of policies. Another thing I would love to see as part of this CL if possible or a new one is the set of configured URLs for the new policies to be visible in the exceptions list on the content settings dialogue so that it is more transparent for the user in case an evil admin decides to spy on them by configuring this policy. Lastly I will add Markus from the Privacy team to review this CL as well since he is the main driving force behind content settings and can give a privacy critical review of this change. https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3796: 'per_profile': False, This is actually both per profile and global policy, however only on Chrome OS it can be set as a device policy so I would suggest to flag it as per profile one. https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3822: 'per_profile': False, This is actually a per profile policy. Please change that. https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3838: 'per_profile': False, This is actually a per profile policy. Please change that. It was my mistake to make it not per profile in the first place. https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3864: 'per_profile': False, This is actually a per profile policy. Please change that.
Thanks. I like the idea of showing the user what config the admin has applied on the machine, but that should be done in a separate CL (from experience I'm guessing that deciding on a proper UX will take some time). Markus - one idea for how to implement Julian's suggestion is to add all the policy values to the "Media Exceptions" list and render them grayed out? Possibly showing the "admin enforced" icon next to these values. We should then also show an entry of "*" if the admin has set the [Audio|Video]CaptureAllowed policy to "disabled", to indicate that all user entries in the list will be preempted. https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3796: 'per_profile': False, On 2013/05/23 08:33:12, pastarmovj wrote: > This is actually both per profile and global policy, however only on Chrome OS > it can be set as a device policy so I would suggest to flag it as per profile > one. Done. https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3822: 'per_profile': False, On 2013/05/23 08:33:12, pastarmovj wrote: > This is actually a per profile policy. Please change that. Done. https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3838: 'per_profile': False, On 2013/05/23 08:33:12, pastarmovj wrote: > This is actually a per profile policy. Please change that. It was my mistake to > make it not per profile in the first place. Done. https://codereview.chromium.org/15738004/diff/3019/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3864: 'per_profile': False, On 2013/05/23 08:33:12, pastarmovj wrote: > This is actually a per profile policy. Please change that. Done.
I'd like to have input from the privacy folks regarding supporting patterns like "*" or [ "a*", "b*", ... ]. https://codereview.chromium.org/15738004/diff/13001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/13001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:216: MatchPattern(request_.security_origin.spec(), value)) { MatchPattern() accepts "*", do we want to enable that? That's the same as setting AudioCaptureAllowed=true. Maybe this is OK, I just want us to make a conscious decision here. IIRC content settings has its own pattern matching, maybe this policy should use the same format? (and it also allows '*')
https://codereview.chromium.org/15738004/diff/13001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/13001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:216: MatchPattern(request_.security_origin.spec(), value)) { On 2013/05/23 13:28:36, Joao da Silva wrote: > MatchPattern() accepts "*", do we want to enable that? That's the same as > setting AudioCaptureAllowed=true. Maybe this is OK, I just want us to make a > conscious decision here. Yes, that's a conscious decision. I don't think it's realistic for this policy to draw a line that we don't trust admins to cross or to assume that whatever line we draw would somehow make the end user more secure. If there's a site (that the admin wants to allow access to, then a very specific rule will have the same effect as a very generic one. (and I don't think we'll expect the user to go into content settings in order to figure out what rules have been applied) > > IIRC content settings has its own pattern matching, maybe this policy should use > the same format? (and it also allows '*') Yes, makes sense. I followed how a similar policy was implemented in Chrome Frame, but using the same code as content settings does sounds like the right thing to do. I'll ping back when I've got that ready.
oh and thanks for the feedback :)
Switched over to using ContentSettingsPattern. PTAL.
Just a heads-up that I'm going on vacation mid next week. I'd like to get this change in before then if possible, ideally a day or two before in case there are some unknown unknowns. On Thu, May 23, 2013 at 4:24 PM, <tommi@chromium.org> wrote: > Switched over to using ContentSettingsPattern. PTAL. > > https://codereview.chromium.**org/15738004/<https://codereview.chromium.org/1... >
1) Policy Is there a particular reason why you don't integrate the new policy with the HostContentSettingsMap similar to all the other existing similar policies? I think we approach a point were we should clean up a few things and integration all media settings related policies with the HostContentSettingsMap rather than (re-)buildling parts of it. 2) UI The UI should be consistent with the existing UI. I guess this is essentially what Tommi recommended. Your life will be easier here if you integrate with the HostContentSettingsMap. 3) Patterns @joao: I'm not sure if you have any specific question. If you integrate with the HostContentSettingsMap you get all this for free. Please don't start building yet another pattern class (but from the CL I saw that you are already using content settings patterns). The formate of content settings pattern is the same format as for existing policies e.g. blockImagesForUrls. Here are some examples [*.]example.com same as *://[*.]example.com:* https://[*.]example.com same as https://[*.]example.com:* foo.example.com:8081 some as *://foo.example.com:8081 URL Paths are only supported for file URLs. @Julian,Joao,Mattias: Feel free to grab me to discuss this . https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:211: if (prefs->IsManagedPreference(whitelist_policy_name)) { Is there a particular reason why you don't integrate with the HostContentSettingsMap? This is diverging more and more, and I thing we should really start integrating this with the content settings system instead of build yet another thingy.
Policy lgtm. There is UI in the content settings for the media settings, and it won't show what is configured via policy; I think that should be fixed eventually, either in this CL or in a subsequent one. I'll leave that for Markus :-) https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:234: return ALWAYS_DENY; Suggestion: I think the logic here would be clearer as if (prefs->IsManagedPref(policy_name) && !prefs->GetBoolean(policy_name)) return ALWAYS_DENY; return POLICY_NOT_SET;
https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:230: prefs->GetBoolean(policy_name)) { Will this not re-introduce the same issue as before? If an admin configures the policy for the default settings and set's it to true, then the user will never see an info bar right?
Thanks for the quick turnaround. Sounds like we're all in agreement for how the UI should work. I can do the necessary cleanup and integration with HostContentSettingsMap. https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:211: if (prefs->IsManagedPreference(whitelist_policy_name)) { On 2013/05/24 10:55:35, markusheintz_ wrote: > Is there a particular reason why you don't integrate with the > HostContentSettingsMap? Yes - ignorance! 8) > > This is diverging more and more, and I thing we should really start integrating > this with the content settings system instead of build yet another thingy. I looked at it and agree that we (i.e. I) should move this code there. I added a TODO in the header for myself to clean this up. https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:230: prefs->GetBoolean(policy_name)) { On 2013/05/24 13:01:54, markusheintz_ wrote: > Will this not re-introduce the same issue as before? > > If an admin configures the policy for the default settings and set's it to true, > then the user will never see an info bar right? In this case the function returns POLICY_NOT_SET, so the user will see the info bar. Setting the policy to true is now equal to not setting it at all (same behavior as it was prior to the change back in February). Maybe you're misreading the || as && (and missed the !)? https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:234: return ALWAYS_DENY; On 2013/05/24 12:48:01, Joao da Silva wrote: > Suggestion: I think the logic here would be clearer as > > if (prefs->IsManagedPref(policy_name) && !prefs->GetBoolean(policy_name)) > return ALWAYS_DENY; > return POLICY_NOT_SET; Done.
Can we please add a unittest for this? Please also file a bug for displaying the per site exceptions on the chrome settings page. https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:211: if (prefs->IsManagedPreference(whitelist_policy_name)) { On 2013/05/24 13:08:31, tommi wrote: > On 2013/05/24 10:55:35, markusheintz_ wrote: > > Is there a particular reason why you don't integrate with the > > HostContentSettingsMap? > > Yes - ignorance! 8) :) Please file a bug for integrating the policies with the PolicyProvider of the HostContentSettingsMap. THX > > > > > This is diverging more and more, and I thing we should really start > integrating > > this with the content settings system instead of build yet another thingy. > > I looked at it and agree that we (i.e. I) should move this code there. I added > a TODO in the header for myself to clean this up. https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:217: ContentSettingsPattern::FromString(value); If an admin could use the Wildcard pattern "*" in the whitelist policy to allow media access for all websites. Chrome would not displaying the infobar in this case. I guess you could fix this by ignoring Wildcard patterns: e.g. if (pattern == ContentSettingsPattern::Wildcard) continue; https://codereview.chromium.org/15738004/diff/27001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:230: prefs->GetBoolean(policy_name)) { On 2013/05/24 13:08:31, tommi wrote: > On 2013/05/24 13:01:54, markusheintz_ wrote: > > Will this not re-introduce the same issue as before? > > > > If an admin configures the policy for the default settings and set's it to > true, > > then the user will never see an info bar right? > > In this case the function returns POLICY_NOT_SET, so the user will see the info > bar. Setting the policy to true is now equal to not setting it at all (same > behavior as it was prior to the change back in February). > > Maybe you're misreading the || as && (and missed the !)? True. Sorry I missed that.
I quickly discussed the issue that we don't display the exceptions set via whitelist policy in the UI with Dominic (Privacy Team TL cc'ed). Is it possible to limit the whitelist/blacklist policy to kiosk mode only until we display the exceptions on the settings page? I think it is confusing if users in companies that use these policies wonder why Chrome does not ask before granting Microphone access. They don't get any visual feedback In the past we were always careful give the user visual feedback in such cases.
Thanks Markus. Sure, that's not a problem. Done and change uploaded.
On 2013/05/24 16:46:15, tommi wrote: > Thanks Markus. Sure, that's not a problem. Done and change uploaded. Thanks a lot :)
Please Please Please let's have unittests for this. If not file at least a bug and include it in a comment! The MediaStreamDevicesController really need some test. Not having tests already bit me. https://codereview.chromium.org/15738004/diff/58001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/58001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:223: ContentSettingsPattern::FromString(value); Please don't forget to ignore the "*" Wildcard patterns :)
https://codereview.chromium.org/15738004/diff/58001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/58001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:223: ContentSettingsPattern::FromString(value); On 2013/05/24 16:53:57, markusheintz_ wrote: > Please don't forget to ignore the "*" Wildcard patterns :) I guess #13 got buried so I'll repost: > I guess you could fix this by ignoring Wildcard patterns: e.g. > if (pattern == ContentSettingsPattern::Wildcard) > continue;
On 2013/05/24 16:53:57, markusheintz_ wrote: > Please Please Please let's have unittests for this. > > If not file at least a bug and include it in a comment! > > The MediaStreamDevicesController really need some test. Not having tests already > bit me. > Did you look in policy_browsertest.cc or do you mean some other tests? > https://codereview.chromium.org/15738004/diff/58001/chrome/browser/media/medi... > File chrome/browser/media/media_stream_devices_controller.cc (right): > > https://codereview.chromium.org/15738004/diff/58001/chrome/browser/media/medi... > chrome/browser/media/media_stream_devices_controller.cc:223: > ContentSettingsPattern::FromString(value); > Please don't forget to ignore the "*" Wildcard patterns :) Done. Sorry, didn't mean to leave that out. :)
On 2013/05/27 11:04:51, tommi wrote: > On 2013/05/24 16:53:57, markusheintz_ wrote: > > Please Please Please let's have unittests for this. > > > > If not file at least a bug and include it in a comment! > > > > The MediaStreamDevicesController really need some test. Not having tests > already > > bit me. > > > > Did you look in policy_browsertest.cc or do you mean some other tests? > > > > https://codereview.chromium.org/15738004/diff/58001/chrome/browser/media/medi... > > File chrome/browser/media/media_stream_devices_controller.cc (right): > > > > > https://codereview.chromium.org/15738004/diff/58001/chrome/browser/media/medi... > > chrome/browser/media/media_stream_devices_controller.cc:223: > > ContentSettingsPattern::FromString(value); > > Please don't forget to ignore the "*" Wildcard patterns :) > > Done. Sorry, didn't mean to leave that out. :) oops, ran into some problems and the change hasn't been uploaded. I'll ping back when it's ready.
OK, the changes have been uploaded. ptal.
Sorry for the delay it's meeting Monday. We are almost there on last thing. https://codereview.chromium.org/15738004/diff/77001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/77001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:211: CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode); What about CROS ? The Kiosk mode on CROS is a bit different. You need to use the ManagedUser class (see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...)
On 2013/05/27 14:37:24, markusheintz_ wrote: > Sorry for the delay it's meeting Monday. > > We are almost there on last thing. > > https://codereview.chromium.org/15738004/diff/77001/chrome/browser/media/medi... > File chrome/browser/media/media_stream_devices_controller.cc (right): > > https://codereview.chromium.org/15738004/diff/77001/chrome/browser/media/medi... > chrome/browser/media/media_stream_devices_controller.cc:211: > CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode); > What about CROS ? > > The Kiosk mode on CROS is a bit different. > > You need to use the ManagedUser class (see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...) I gave it a try but ran into some issues with the unit test: * The UserManager class requires quite a bit of state (globals) to be initialized before it can be used and the environment checked (kiosk mode). * This includes CrosSettings, DeviceSettingsService and the proper threads (must be initialized on Chrome's UI thread with a UI message loop). I got to the "initialize on the UI thread" stage when I hit a conflict with the browser tests in general, which need to be able to set their own UI thread. Long story short, I think this can be done, but it's not simple and involves instantiating the proper mock objects on the correct thread before the tests can run. This is a rather big change, so I'd like to propose this compromise instead: * Check for cros kiosk mode as well as Chrome's command line flag in MediaStreamDevicesController. * The tests only set the Chrome command line flag to indicate kiosk mode. This way the tests work on all platforms. On CrOS we will check the user manager correctly, but there won't be a test that covers the user manager specifically.
https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:233: if (kiosk_mode && prefs->IsManagedPreference(whitelist_policy_name)) { IIUC the whitelist only works in kiosk mode, and this is a temporary condition until it integrates with the content settings and the UI. If that's the case then please document that this policy only works in kiosk mode, in policy_templates.json.
LGTM. Thanks a lot for going the extra rounds! :) https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:50: return true; Nit. Absolutely optional to fix: #if defined(OS_CHROMEOS) const chromeos::UserManager* user_manager = chromeos::UserManager::Get(); return (user_manager && user_manager->IsLoggedInAsKioskApp()) #else return false; #endif https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:227: bool kiosk_mode = IsInKioskMode(); Nit. Optional to fix: Inline this bool. And move the comment to the if clause in line 233.
Drive-by https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:233: if (kiosk_mode && prefs->IsManagedPreference(whitelist_policy_name)) { nit: You probably want to drop the IsManagedPreference check here. With the current code it doesn't make a difference, but when integrating with regular content settings you don't care where the pref comes from. https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:255: if (prefs->IsManagedPreference(policy_name) && same here. https://codereview.chromium.org/15738004/diff/97001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/15738004/diff/97001/chrome/common/pref_names.... chrome/common/pref_names.cc:2032: // with the kAudioCaptureAllowedUrls policy list. Is that true? Looking at the code kAudioCaptureAllowedUrls takes precedence over kAudioCaptureAllowed. https://codereview.chromium.org/15738004/diff/97001/chrome/test/data/policy/p... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/15738004/diff/97001/chrome/test/data/policy/p... chrome/test/data/policy/policy_test_cases.json:967: "AudioCaptureAllowedUrls": { This policy is mapped to a pref, so it should have a proper block here. The indicator parts don't apply though (if my understanding is correct that the exception lists are not wired up in the UI).
https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:233: if (kiosk_mode && prefs->IsManagedPreference(whitelist_policy_name)) { On 2013/05/28 07:54:23, Joao da Silva wrote: > IIUC the whitelist only works in kiosk mode, and this is a temporary condition > until it integrates with the content settings and the UI. > > If that's the case then please document that this policy only works in kiosk > mode, in policy_templates.json. What's the recommended way of doing this in policy_templates.json? Using the desc? Is there a similar case in policy_templates.json that could be used as example?
https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:233: if (kiosk_mode && prefs->IsManagedPreference(whitelist_policy_name)) { On 2013/05/28 08:11:39, Mattias Nissler wrote: > nit: You probably want to drop the IsManagedPreference check here. With the > current code it doesn't make a difference, but when integrating with regular > content settings you don't care where the pref comes from. @Mattias: why can't this test stay for now? Once we integrate with the host content settings map. We won't use prefs at all here. So I don't see why this should be removed. I would argue that we need this, since this is supposed to be a policy and not a pref. Otherwise you could put this into the pref file. same below.
https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:233: if (kiosk_mode && prefs->IsManagedPreference(whitelist_policy_name)) { On 2013/05/28 08:15:27, markusheintz_ wrote: > On 2013/05/28 08:11:39, Mattias Nissler wrote: > > nit: You probably want to drop the IsManagedPreference check here. With the > > current code it doesn't make a difference, but when integrating with regular > > content settings you don't care where the pref comes from. > > @Mattias: why can't this test stay for now? Once we integrate with the host > content settings map. We won't use prefs at all here. So I don't see why this > should be removed. > I would argue that we need this, since this is supposed to be a policy and not a > pref. Otherwise you could put this into the pref file. > > same below. So how would that hurt? We don't have UI for setting that pref in the prefs file, and users wanting to shoot themselves in the foot have plenty opportunity to do so otherwise already. The PrefService interface is designed to supply values mostly, no matter whether they come from policy or prefs or the command line or elsewhere. The whole point of the PrefService abstraction is that callers don't have to worry about the nitty-gritty details of where the value comes from - meta data like whether a pref comes from policy, is still the default, etc. shouldn't influence behavior in the common case IMHO.
On 2013/05/28 08:21:03, Mattias Nissler wrote: > https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... > File chrome/browser/media/media_stream_devices_controller.cc (right): > > https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... > chrome/browser/media/media_stream_devices_controller.cc:233: if (kiosk_mode && > prefs->IsManagedPreference(whitelist_policy_name)) { > On 2013/05/28 08:15:27, markusheintz_ wrote: > > On 2013/05/28 08:11:39, Mattias Nissler wrote: > > > nit: You probably want to drop the IsManagedPreference check here. With the > > > current code it doesn't make a difference, but when integrating with regular > > > content settings you don't care where the pref comes from. > > > > @Mattias: why can't this test stay for now? Once we integrate with the host > > content settings map. We won't use prefs at all here. So I don't see why this > > should be removed. > > I would argue that we need this, since this is supposed to be a policy and not > a > > pref. Otherwise you could put this into the pref file. > > > > same below. > > So how would that hurt? We don't have UI for setting that pref in the prefs > file, and users wanting to shoot themselves in the foot have plenty opportunity > to do so otherwise already. > > The PrefService interface is designed to supply values mostly, no matter whether > they come from policy or prefs or the command line or elsewhere. The whole point > of the PrefService abstraction is that callers don't have to worry about the > nitty-gritty details of where the value comes from - meta data like whether a > pref comes from policy, is still the default, etc. shouldn't influence behavior > in the common case IMHO. I think in this case it matters and that we only want to use this pref value if it was supplied by policy. If someone or something (e.g. some malicious SW) puts this in a prefs file for a user, then there is no (easy) way for the user to recover. Therefor I would prefer to ignore the pref if it was not set by policy. BTW: I don't think the code cares much about the nitty-gritty details. It just checks that this pref was set by policy and only by policy. Isn't this exactely why we have this kind of meta data?
On 2013/05/28 08:31:28, markusheintz_ wrote: > On 2013/05/28 08:21:03, Mattias Nissler wrote: > > > https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... > > File chrome/browser/media/media_stream_devices_controller.cc (right): > > > > > https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... > > chrome/browser/media/media_stream_devices_controller.cc:233: if (kiosk_mode && > > prefs->IsManagedPreference(whitelist_policy_name)) { > > On 2013/05/28 08:15:27, markusheintz_ wrote: > > > On 2013/05/28 08:11:39, Mattias Nissler wrote: > > > > nit: You probably want to drop the IsManagedPreference check here. With > the > > > > current code it doesn't make a difference, but when integrating with > regular > > > > content settings you don't care where the pref comes from. > > > > > > @Mattias: why can't this test stay for now? Once we integrate with the host > > > content settings map. We won't use prefs at all here. So I don't see why > this > > > should be removed. > > > I would argue that we need this, since this is supposed to be a policy and > not > > a > > > pref. Otherwise you could put this into the pref file. > > > > > > same below. > > > > So how would that hurt? We don't have UI for setting that pref in the prefs > > file, and users wanting to shoot themselves in the foot have plenty > opportunity > > to do so otherwise already. > > > > The PrefService interface is designed to supply values mostly, no matter > whether > > they come from policy or prefs or the command line or elsewhere. The whole > point > > of the PrefService abstraction is that callers don't have to worry about the > > nitty-gritty details of where the value comes from - meta data like whether a > > pref comes from policy, is still the default, etc. shouldn't influence > behavior > > in the common case IMHO. > > I think in this case it matters and that we only want to use this pref value if > it was supplied by policy. > If someone or something (e.g. some malicious SW) puts this in a prefs file for a > user, then there is no (easy) way for the user to recover. You'll have user-supplied pref values anyways once we start doing content settings UI, and few users will manage to find the content settings dialog that lets them recover if the value gets put there by malware. On a related note, this brings up the old discussion around HCSM abusing PrefService by using different pref names for the different pref sources... this discussion wouldn't exist otherwise as you'd have no motivation to do IsManaged() checks in HCSM if it weren't abusing prefs. > Therefor I would prefer to ignore the pref if it was not set by policy. > BTW: I don't think the code cares much about the nitty-gritty details. It just > checks that this pref was set by policy and only by policy. Isn't this exactely > why we have this kind of meta data? I would consider the source of the pref value a mostly nitty-gritty detail. As mentioned already, prefs primarily supply values. So as a feature developer, if the prefs system says enable-goat-teleporter is set to true, you enable the goat teleporter. In most cases, you shouldn't have to worry about who made that decision, it can be the user, or the admin, the pref system sorts out getting the right value to you. That's the theory and the design idea behind PrefService. There are of course some edge cases where it does make sense to change behavior based on pref source, but these should be rare. If you worry about prefs being read from the user prefs file when they shouldn't, a much better and consistent way to accomplish that would be to filter prefs at the JsonPrefStore level. If as a caller you're interested of reading policy and only policy, then nowadays you're better off querying PolicyService directly. So far for the ranting. I don't feel strong if you decide to keep the check given that this doesn't make a behavioral difference. My motivation for putting this comment is to remind people about the original design ideas of PrefService and use it appropriately.
https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... File chrome/browser/media/media_stream_devices_controller.cc (right): https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:50: return true; On 2013/05/28 08:06:31, markusheintz_ wrote: > Nit. Absolutely optional to fix: > > #if defined(OS_CHROMEOS) > const chromeos::UserManager* user_manager = chromeos::UserManager::Get(); > return (user_manager && user_manager->IsLoggedInAsKioskApp()) > #else > return false; > #endif Done. https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:227: bool kiosk_mode = IsInKioskMode(); On 2013/05/28 08:06:31, markusheintz_ wrote: > Nit. Optional to fix: > > Inline this bool. And move the comment to the if clause in line 233. Done. https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:233: if (kiosk_mode && prefs->IsManagedPreference(whitelist_policy_name)) { On 2013/05/28 08:21:03, Mattias Nissler wrote: > On 2013/05/28 08:15:27, markusheintz_ wrote: > > On 2013/05/28 08:11:39, Mattias Nissler wrote: > > > nit: You probably want to drop the IsManagedPreference check here. With the > > > current code it doesn't make a difference, but when integrating with regular > > > content settings you don't care where the pref comes from. > > > > @Mattias: why can't this test stay for now? Once we integrate with the host > > content settings map. We won't use prefs at all here. So I don't see why this > > should be removed. > > I would argue that we need this, since this is supposed to be a policy and not > a > > pref. Otherwise you could put this into the pref file. > > > > same below. > > So how would that hurt? We don't have UI for setting that pref in the prefs > file, and users wanting to shoot themselves in the foot have plenty opportunity > to do so otherwise already. > > The PrefService interface is designed to supply values mostly, no matter whether > they come from policy or prefs or the command line or elsewhere. The whole point > of the PrefService abstraction is that callers don't have to worry about the > nitty-gritty details of where the value comes from - meta data like whether a > pref comes from policy, is still the default, etc. shouldn't influence behavior > in the common case IMHO. > I added this note to the description in policy_templates.json: "NOTE: This policy is currently only supported when running in Kiosk mode." Removed the call to IsManagedPreference() (I don't have a preference - no pun intended - and am just going with the last comment :)). https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... chrome/browser/media/media_stream_devices_controller.cc:255: if (prefs->IsManagedPreference(policy_name) && On 2013/05/28 08:11:39, Mattias Nissler wrote: > same here. Done. https://codereview.chromium.org/15738004/diff/97001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/15738004/diff/97001/chrome/common/pref_names.... chrome/common/pref_names.cc:2032: // with the kAudioCaptureAllowedUrls policy list. On 2013/05/28 08:11:39, Mattias Nissler wrote: > Is that true? Looking at the code kAudioCaptureAllowedUrls takes precedence over > kAudioCaptureAllowed. Thanks for catching. This was true a few patch sets ago but it isn't anymore. Updated comments. https://codereview.chromium.org/15738004/diff/97001/chrome/test/data/policy/p... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/15738004/diff/97001/chrome/test/data/policy/p... chrome/test/data/policy/policy_test_cases.json:967: "AudioCaptureAllowedUrls": { On 2013/05/28 08:11:39, Mattias Nissler wrote: > This policy is mapped to a pref, so it should have a proper block here. The > indicator parts don't apply though (if my understanding is correct that the > exception lists are not wired up in the UI). Can you help me out here? What would be the appropriate test policy?
All comments addressed (I believe) and changes uploaded. PTAL. https://codereview.chromium.org/15738004/diff/97001/chrome/test/data/policy/p... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/15738004/diff/97001/chrome/test/data/policy/p... chrome/test/data/policy/policy_test_cases.json:967: "AudioCaptureAllowedUrls": { On 2013/05/28 10:19:43, tommi wrote: > On 2013/05/28 08:11:39, Mattias Nissler wrote: > > This policy is mapped to a pref, so it should have a proper block here. The > > indicator parts don't apply though (if my understanding is correct that the > > exception lists are not wired up in the UI). > > Can you help me out here? What would be the appropriate test policy? Nevermind. Done.
On 2013/05/28 08:57:47, Mattias Nissler wrote: > On 2013/05/28 08:31:28, markusheintz_ wrote: > > On 2013/05/28 08:21:03, Mattias Nissler wrote: > > > > > > https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... > > > File chrome/browser/media/media_stream_devices_controller.cc (right): > > > > > > > > > https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... > > > chrome/browser/media/media_stream_devices_controller.cc:233: if (kiosk_mode > && > > > prefs->IsManagedPreference(whitelist_policy_name)) { > > > On 2013/05/28 08:15:27, markusheintz_ wrote: > > > > On 2013/05/28 08:11:39, Mattias Nissler wrote: > > > > > nit: You probably want to drop the IsManagedPreference check here. With > > the > > > > > current code it doesn't make a difference, but when integrating with > > regular > > > > > content settings you don't care where the pref comes from. > > > > > > > > @Mattias: why can't this test stay for now? Once we integrate with the > host > > > > content settings map. We won't use prefs at all here. So I don't see why > > this > > > > should be removed. > > > > I would argue that we need this, since this is supposed to be a policy and > > not > > > a > > > > pref. Otherwise you could put this into the pref file. > > > > > > > > same below. > > > > > > So how would that hurt? We don't have UI for setting that pref in the prefs > > > file, and users wanting to shoot themselves in the foot have plenty > > opportunity > > > to do so otherwise already. > > > > > > The PrefService interface is designed to supply values mostly, no matter > > whether > > > they come from policy or prefs or the command line or elsewhere. The whole > > point > > > of the PrefService abstraction is that callers don't have to worry about the > > > nitty-gritty details of where the value comes from - meta data like whether > a > > > pref comes from policy, is still the default, etc. shouldn't influence > > behavior > > > in the common case IMHO. > > > > I think in this case it matters and that we only want to use this pref value > if > > it was supplied by policy. > > If someone or something (e.g. some malicious SW) puts this in a prefs file for > a > > user, then there is no (easy) way for the user to recover. > > You'll have user-supplied pref values anyways once we start doing content > settings UI, and few users will manage to find the content settings dialog that > lets them recover if the value gets put there by malware. How is that related to the policy only pref? You won't have UI for them right? There will be no way to reset them. > > On a related note, this brings up the old discussion around HCSM abusing > PrefService by using different pref names for the different pref sources... this > discussion wouldn't exist otherwise as you'd have no motivation to do > IsManaged() checks in HCSM if it weren't abusing prefs. You know exactly why the HCSM has to "abuse" the PrefService. I'm open for suggestion how to merge dictionaries and how to improve the situation. :) And what about the ADMX templates do they support map types now? > > > Therefor I would prefer to ignore the pref if it was not set by policy. > > BTW: I don't think the code cares much about the nitty-gritty details. It just > > checks that this pref was set by policy and only by policy. Isn't this > exactely > > why we have this kind of meta data? > > I would consider the source of the pref value a mostly nitty-gritty detail. As > mentioned already, prefs primarily supply values. So as a feature developer, if > the prefs system says enable-goat-teleporter is set to true, you enable the goat > teleporter. In most cases, you shouldn't have to worry about who made that > decision, it can be the user, or the admin, the pref system sorts out getting > the right value to you. > > That's the theory and the design idea behind PrefService. There are of course > some edge cases where it does make sense to change behavior based on pref > source, but these should be rare. If you worry about prefs being read from the > user prefs file when they shouldn't, a much better and consistent way to > accomplish that would be to filter prefs at the JsonPrefStore level. That's actually a good idea. Let's sync about this when you are in MUC. > > If as a caller you're interested of reading policy and only policy, then > nowadays you're better off querying PolicyService directly. This is good to know. Thanks :) I guess I need to catch up on this. > > So far for the ranting. I don't feel strong if you decide to keep the check > given that this doesn't make a behavioral difference. My motivation for putting > this comment is to remind people about the original design ideas of PrefService > and use it appropriately. That's great! I totally appreciate this. But all this should live in the HCSM anyway. So this WILL go away :) (hopefully).
On 2013/05/28 11:13:49, markusheintz_ wrote: > On 2013/05/28 08:57:47, Mattias Nissler wrote: > > On 2013/05/28 08:31:28, markusheintz_ wrote: > > > On 2013/05/28 08:21:03, Mattias Nissler wrote: > > > > > > > > > > https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... > > > > File chrome/browser/media/media_stream_devices_controller.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/15738004/diff/97001/chrome/browser/media/medi... > > > > chrome/browser/media/media_stream_devices_controller.cc:233: if > (kiosk_mode > > && > > > > prefs->IsManagedPreference(whitelist_policy_name)) { > > > > On 2013/05/28 08:15:27, markusheintz_ wrote: > > > > > On 2013/05/28 08:11:39, Mattias Nissler wrote: > > > > > > nit: You probably want to drop the IsManagedPreference check here. > With > > > the > > > > > > current code it doesn't make a difference, but when integrating with > > > regular > > > > > > content settings you don't care where the pref comes from. > > > > > > > > > > @Mattias: why can't this test stay for now? Once we integrate with the > > host > > > > > content settings map. We won't use prefs at all here. So I don't see why > > > this > > > > > should be removed. > > > > > I would argue that we need this, since this is supposed to be a policy > and > > > not > > > > a > > > > > pref. Otherwise you could put this into the pref file. > > > > > > > > > > same below. > > > > > > > > So how would that hurt? We don't have UI for setting that pref in the > prefs > > > > file, and users wanting to shoot themselves in the foot have plenty > > > opportunity > > > > to do so otherwise already. > > > > > > > > The PrefService interface is designed to supply values mostly, no matter > > > whether > > > > they come from policy or prefs or the command line or elsewhere. The whole > > > point > > > > of the PrefService abstraction is that callers don't have to worry about > the > > > > nitty-gritty details of where the value comes from - meta data like > whether > > a > > > > pref comes from policy, is still the default, etc. shouldn't influence > > > behavior > > > > in the common case IMHO. > > > > > > I think in this case it matters and that we only want to use this pref value > > if > > > it was supplied by policy. > > > If someone or something (e.g. some malicious SW) puts this in a prefs file > for > > a > > > user, then there is no (easy) way for the user to recover. > > > > You'll have user-supplied pref values anyways once we start doing content > > settings UI, and few users will manage to find the content settings dialog > that > > lets them recover if the value gets put there by malware. > > How is that related to the policy only pref? You won't have UI for them right? > There will be no way to reset them. You could: - Delete and recreate your profile - Wipe the Chrome data directory - Uninstall and reinstall Chrome I just don't buy that the average user will find the content settings UI to protect them. > > > > > On a related note, this brings up the old discussion around HCSM abusing > > PrefService by using different pref names for the different pref sources... > this > > discussion wouldn't exist otherwise as you'd have no motivation to do > > IsManaged() checks in HCSM if it weren't abusing prefs. > > You know exactly why the HCSM has to "abuse" the PrefService. > I'm open for suggestion how to merge dictionaries and how to improve the > situation. :) We've discussed that in the past, and there's no "has to" here. It's just that nobody has signed themselves up to do this properly until now. > And what about the ADMX templates do they support map types now? We have JSON blobs in other places already and will use that for structured policy in general as well. > > > > > > Therefor I would prefer to ignore the pref if it was not set by policy. > > > BTW: I don't think the code cares much about the nitty-gritty details. It > just > > > checks that this pref was set by policy and only by policy. Isn't this > > exactely > > > why we have this kind of meta data? > > > > I would consider the source of the pref value a mostly nitty-gritty detail. As > > mentioned already, prefs primarily supply values. So as a feature developer, > if > > the prefs system says enable-goat-teleporter is set to true, you enable the > goat > > teleporter. In most cases, you shouldn't have to worry about who made that > > decision, it can be the user, or the admin, the pref system sorts out getting > > the right value to you. > > > > That's the theory and the design idea behind PrefService. There are of course > > some edge cases where it does make sense to change behavior based on pref > > source, but these should be rare. If you worry about prefs being read from > the > > user prefs file when they shouldn't, a much better and consistent way to > > accomplish that would be to filter prefs at the JsonPrefStore level. > > That's actually a good idea. Let's sync about this when you are in MUC. > > > > > If as a caller you're interested of reading policy and only policy, then > > nowadays you're better off querying PolicyService directly. > > This is good to know. Thanks :) I guess I need to catch up on this. > > > > > So far for the ranting. I don't feel strong if you decide to keep the check > > given that this doesn't make a behavioral difference. My motivation for > putting > > this comment is to remind people about the original design ideas of > PrefService > > and use it appropriately. > > That's great! I totally appreciate this. But all this should live in the HCSM > anyway. So this WILL go away :) (hopefully). Famous last words ;)
LGTM https://codereview.chromium.org/15738004/diff/111001/chrome/browser/media/med... File chrome/browser/media/media_stream_devices_controller.h (right): https://codereview.chromium.org/15738004/diff/111001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.h:31: // TODO(tommi): Clean up all the policy code and integrate with nit: bug reference?
Thanks. Ping: pastarmovj, battre. At the end of today I'll be going on vacation so if you can take a look now, that would be appreciated. https://codereview.chromium.org/15738004/diff/111001/chrome/browser/media/med... File chrome/browser/media/media_stream_devices_controller.h (right): https://codereview.chromium.org/15738004/diff/111001/chrome/browser/media/med... chrome/browser/media/media_stream_devices_controller.h:31: // TODO(tommi): Clean up all the policy code and integrate with On 2013/05/28 11:55:24, Mattias Nissler wrote: > nit: bug reference? Done.
Moved battre@ to cc after discussing offline - Julian, that leaves you :)
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommi@chromium.org/15738004/125001
Message was sent while issue was closed.
Change committed as 202572 |
