Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2532323003:
Public Sessions - prompt the user for audioCapture/videoCapture requests (Closed)
|
Created:
4 years ago by Ivan Šandrk Modified:
4 years ago CC:
chromium-reviews, extensions-reviews_chromium.org, oshima+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, chromium-apps-reviews_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPublic Sessions - prompt the user for audioCapture/videoCapture requests
In Public Sessions extensions are force installed by admin policy and some of the permissions are granted at install time so the end user has no say on it. audioCapture and videoCapture APIs are dangerous from a security/privacy standpoint therefore we want to give the user a runtime prompt whether to allow such API.
Only platform apps (and some built-in whitelisted extensions) support audioCapture[1]/videoCapture[2] manifest permission features therefore this filtering will be limited to platform apps.
[1]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_features.json?rcl=0&l=102
[2]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_features.json?rcl=0&l=491
BUG=669521
Committed: https://crrev.com/b040fda5a81f54c4dbab7664f3aaf2099c110b61
Cr-Commit-Position: refs/heads/master@{#437243}
Patch Set 1 #Patch Set 2 : Fixed unit test #
Total comments: 16
Patch Set 3 : nits, save prompt in callback #Patch Set 4 : Using IsAllowed instead of Disallowed #Patch Set 5 : Added a new class for handling media access in Public Sessions #
Total comments: 20
Patch Set 6 : Addressed comments #Patch Set 7 : Fixed presubmit errors (line length, import order) #Patch Set 8 : Rebase #
Total comments: 45
Patch Set 9 : Addressed comments #1 #
Total comments: 1
Patch Set 10 : Drew's comments #
Total comments: 11
Patch Set 11 : Update comment, erase map entry #
Total comments: 4
Patch Set 12 : std::move, formatting #
Messages
Total messages: 92 (64 generated)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
isandrk@chromium.org changed reviewers: + sergeyu@chromium.org
Hi Sergey, please take a look at chrome/browser/media/* Thanks, Ivan
Description was changed from ========== Public Sessions - prompt the user for audioCapture/videoCapture requests In Public Sessions extensions are force installed by admin policy and some of the permissions are granted at install time so the end user has no say on it. audioCapture and videoCapture APIs are dangerous from a security/privacy standpoint therefore we want to give the user a runtime prompt whether to allow such API. BUG=669521 ========== to ========== Public Sessions - prompt the user for audioCapture/videoCapture requests In Public Sessions extensions are force installed by admin policy and some of the permissions are granted at install time so the end user has no say on it. audioCapture and videoCapture APIs are dangerous from a security/privacy standpoint therefore we want to give the user a runtime prompt whether to allow such API. BUG=669521 ==========
isandrk@chromium.org changed reviewers: + atwilson@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... File chrome/browser/media/extension_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:66: IsMediaRequestWhitelistedForExtension(extension)) && This class is applicable for platform apps and whitelisted built-in extensions, while CL description says it applies to all extensions. Regular extensions are handled by PermissionBubbleMediaAccessHandler and I believe it shows the prompt by default. Also even it makes sense to show the prompt for platform apps, I'm not sure about built-in whitelisted extensions. https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:82: bool ExtensionMediaAccessHandler::UserChoice::Disallowed( IsAllowed()? That would make code that uses this function cleaner. https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:85: CHECK(type == content::MEDIA_DEVICE_AUDIO_CAPTURE || Why is this CHECK instead of DCHECK? https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Also it would be cleaner to put a check for type == content::MEDIA_DEVICE_VIDEO_CAPTURE in the else case below. https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:144: // TODO(vrk): This code is largely duplicated in Move this TODO to HandleRequestContinuation() https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:153: if (IsPublicSession()) { Did you consider adding a separate MediaAccessHandler for public sessions? It would allow to avoid mixing all the new logic that's applicable to public session with everything else. Potentially it could be implemented as a wrapper for ExtensionMediaAccessHandler or as a separate MediaAccessHandler that's used before ExtensionMediaAccessHandler (access handlers priority is determined by their order here: https://codesearch.chromium.org/chromium/src/chrome/browser/media/webrtc/medi... ) https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:173: prompt_.reset(new ExtensionInstallPrompt(web_contents)); Does ExtensionInstallPrompt() work when the web_contents is a background page or a platform app window? https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:173: prompt_.reset(new ExtensionInstallPrompt(web_contents)); There may be multiple requests that will need to be handled in parallel, so I think you need to queue requests here instead of replacing the old |prompt_|.
Description was changed from ========== Public Sessions - prompt the user for audioCapture/videoCapture requests In Public Sessions extensions are force installed by admin policy and some of the permissions are granted at install time so the end user has no say on it. audioCapture and videoCapture APIs are dangerous from a security/privacy standpoint therefore we want to give the user a runtime prompt whether to allow such API. BUG=669521 ========== to ========== Public Sessions - prompt the user for audioCapture/videoCapture requests In Public Sessions extensions are force installed by admin policy and some of the permissions are granted at install time so the end user has no say on it. audioCapture and videoCapture APIs are dangerous from a security/privacy standpoint therefore we want to give the user a runtime prompt whether to allow such API. Only platform apps (and some built-in whitelisted extensions) support audioCapture[1]/videoCapture[2] manifest permission features therefore this filtering will be limited to platform apps. [1]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... [2]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... BUG=669521 ==========
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... File chrome/browser/media/extension_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:66: IsMediaRequestWhitelistedForExtension(extension)) && You have a good point, the goal of this CL is to show the prompt only for platform apps. I've updated the CL description to reflect this (added another paragraph at the end). I've also added code that limits the dialog only to platform apps. https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:82: bool ExtensionMediaAccessHandler::UserChoice::Disallowed( On 2016/11/29 19:12:06, Sergey Ulanov wrote: > IsAllowed()? > That would make code that uses this function cleaner. I believe there's a subtle semantic difference between IsAllowed and !Disallowed, but I may be wrong. It seems to me that the first implies that we need explicit permission to do it, while the second implies that it just mustn't be explicitly forbidden (the third option here is status quo outside of Public Sessions where it's neither explicitly allowed or forbidden in which case we allow it). What do you think? https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:85: CHECK(type == content::MEDIA_DEVICE_AUDIO_CAPTURE || On 2016/11/29 19:12:06, Sergey Ulanov wrote: > Why is this CHECK instead of DCHECK? > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > Also it would be cleaner to put a check for type == > content::MEDIA_DEVICE_VIDEO_CAPTURE in the else case below. Good point, done. And changed the other CHECK as well. https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:144: // TODO(vrk): This code is largely duplicated in On 2016/11/29 19:12:06, Sergey Ulanov wrote: > Move this TODO to HandleRequestContinuation() Done. https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:153: if (IsPublicSession()) { On 2016/11/29 19:12:05, Sergey Ulanov wrote: > Did you consider adding a separate MediaAccessHandler for public sessions? It > would allow to avoid mixing all the new logic that's applicable to public > session with everything else. > Potentially it could be implemented as a wrapper for ExtensionMediaAccessHandler > or as a separate MediaAccessHandler that's used before > ExtensionMediaAccessHandler (access handlers priority is determined by their > order here: > https://codesearch.chromium.org/chromium/src/chrome/browser/media/webrtc/medi... > ) This is a good idea :) I will investigate it more tomorrow. https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:173: prompt_.reset(new ExtensionInstallPrompt(web_contents)); On 2016/11/29 19:12:06, Sergey Ulanov wrote: > Does ExtensionInstallPrompt() work when the web_contents is a background page or > a platform app window? I believe it should always work when there's web_contents available. I've tested this functionality with a couple of apps and it works in each case. https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:173: prompt_.reset(new ExtensionInstallPrompt(web_contents)); On 2016/11/29 19:12:06, Sergey Ulanov wrote: > There may be multiple requests that will need to be handled in parallel, so I > think you need to queue requests here instead of replacing the old |prompt_|. Now I've prompted to save the ExtensionInstallPrompt inside the callback itself, I think it should be fine. Do you think there's something inherently wrong with this approach?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... File chrome/browser/media/extension_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:82: bool ExtensionMediaAccessHandler::UserChoice::Disallowed( On 2016/11/29 23:33:46, Ivan Šandrk wrote: > On 2016/11/29 19:12:06, Sergey Ulanov wrote: > > IsAllowed()? > > That would make code that uses this function cleaner. > > I believe there's a subtle semantic difference between IsAllowed and > !Disallowed, but I may be wrong. It seems to me that the first implies that we > need explicit permission to do it, while the second implies that it just mustn't > be explicitly forbidden (the third option here is status quo outside of Public > Sessions where it's neither explicitly allowed or forbidden in which case we > allow it). > > What do you think? Decided to go with the IsAllowed in the end. https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex... chrome/browser/media/extension_media_access_handler.cc:153: if (IsPublicSession()) { On 2016/11/29 23:33:46, Ivan Šandrk wrote: > On 2016/11/29 19:12:05, Sergey Ulanov wrote: > > Did you consider adding a separate MediaAccessHandler for public sessions? It > > would allow to avoid mixing all the new logic that's applicable to public > > session with everything else. > > Potentially it could be implemented as a wrapper for > ExtensionMediaAccessHandler > > or as a separate MediaAccessHandler that's used before > > ExtensionMediaAccessHandler (access handlers priority is determined by their > > order here: > > > https://codesearch.chromium.org/chromium/src/chrome/browser/media/webrtc/medi... > > ) > > This is a good idea :) I will investigate it more tomorrow. Implemented a separate MediaAccessHandler :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
isandrk@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hi Devlin, could you take a look at c/b/media/public_session_media_access_handler.cc, specifically at my usage of ExtensionInstallPrompt, and tell me your opinion. I saw you modified some code inside this class recently so I'm assuming you're the right person to ask :) I'm basically making the install_callback (DoneCallback) the owner of the ExtensionInstallPrompt so the idea is that when the callback returns the Prompt is disposed of as well. I'm assuming when the Dialog Prompt closes, the Prompt is not needed anymore. Is this potentially dangerous or is it safe? Thanks!
On 2016/11/30 18:21:18, Ivan Šandrk wrote: > Hi Devlin, could you take a look at > c/b/media/public_session_media_access_handler.cc, specifically at my usage of > ExtensionInstallPrompt, and tell me your opinion. I saw you modified some code > inside this class recently so I'm assuming you're the right person to ask :) > > I'm basically making the install_callback (DoneCallback) the owner of the > ExtensionInstallPrompt so the idea is that when the callback returns the Prompt > is disposed of as well. I'm assuming when the Dialog Prompt closes, the Prompt > is not needed anymore. Is this potentially dangerous or is it safe? > > > Thanks! Forgot to also add... [1] says "The |install_callback| *MUST* eventually be called", so I guess it could work as I expect it to? [1]: https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_inst...
https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:1575: "media/public_session_media_access_handler.cc", Isn't public session only on CrOS? Can we avoid compiling this on all the other platforms? https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.cc:86: base::Unretained(this), web_contents, request, callback, Why is this unretained safe? https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.cc:87: extension, base::Passed(&prompt)), this strikes me as a bit dangerous - we're moving the object that we're actively calling into... I wouldn't be entirely surprised if some compilers made this dangerous. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.cc:139: if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) { optional: These methods would be more succinct if we did something like: struct PermissionState { bool prompted = false; bool allowed = false; } std::map<Type, PermissionState> states and then here, just if (IsPublicSession()) { const PermissionState state = states[type]; return !state.prompted || state.allowed; } NeedsPrompting() { return states[type].prompted; } Set() { PermissionState& state = states[type]; DCHECK(!state.prompted); state.prompted = true; state.allowed = allowed; } But, up to you. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.h:22: // where he can choose whether to allow the extension access to camera and/or drive-by nit: prefer "they" (I wonder why this wasn't caught by https://cs.chromium.org/chromium/tools/depot_tools/presubmit_canned_checks.py... ?) https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.h:25: class PublicSessionMediaAccessHandler : public MediaAccessHandler { What's the lifetime of this object? https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.h:28: ExtensionMediaAccessHandler& extension_media_access_handler); we disallow non-const references passed into functions or as members by style. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/we... File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/we... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:90: new PublicSessionMediaAccessHandler(*extension_media_access_handler)); Hmm... this is a little weird. I wonder if it would make more sense to have PublicSessionMediaAccessHandler subclass ExtensionMediaAccessHandler, and then media_access_handlers_.push_back( IsPublicSession() ? new PublicSessionExtensionMediaAccessHandler() : new ExtensionMediaAccessHandler()); That way, we always use the right handler (rather than relying on the first in having first say).
https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.cc:36: } 'git cl format' please. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.cc:86: base::Unretained(this), web_contents, request, callback, On 2016/11/30 19:02:48, Devlin wrote: > Why is this unretained safe? +1 https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/we... File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/we... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:90: new PublicSessionMediaAccessHandler(*extension_media_access_handler)); On 2016/11/30 19:02:48, Devlin wrote: > Hmm... this is a little weird. I wonder if it would make more sense to have > PublicSessionMediaAccessHandler subclass ExtensionMediaAccessHandler, and then > media_access_handlers_.push_back( > IsPublicSession() ? > new PublicSessionExtensionMediaAccessHandler() : > new ExtensionMediaAccessHandler()); > > That way, we always use the right handler (rather than relying on the first in > having first say). I think this is a good suggestions. Also add ifdef for OS_CHROMEOS here and compile the new access handler only on ChromeOS
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ptal guys :) https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:1575: "media/public_session_media_access_handler.cc", On 2016/11/30 19:02:48, Devlin wrote: > Isn't public session only on CrOS? Can we avoid compiling this on all the other > platforms? Done. I'm not an expert on BUILD files, you can double check the new version. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.cc:36: } On 2016/12/01 01:21:28, Sergey Ulanov wrote: > 'git cl format' please. Done. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.cc:86: base::Unretained(this), web_contents, request, callback, On 2016/12/01 01:21:28, Sergey Ulanov wrote: > On 2016/11/30 19:02:48, Devlin wrote: > > Why is this unretained safe? > > +1 The lifetime of PublicSessionMediaAccessHandler is the same as lifetime of MediaCaptureDevicesDispatcher (since it gets created in its constructor). MCDD is managed by a base::Singleton and is created during first access and is destroyed at normal process exit[1]. It's created sometime before login, and I'm guessing it gets destroyed after logout during shutdown sequence. [1]: https://cs.chromium.org/chromium/src/base/memory/singleton.h?cl=GROK&gsn=get&... Here I'm making the assumption that the newly created dialog cannot possibly outlive the logged-in session (ie. if it's displayed during logout it gets destroyed). I've added some logging code and this is the output when the dialog is open and then we logout: [extension_install_prompt.cc(658)] ShowDialog [public_session_media_access_handler.cc(134)] ResolvePermissionPrompt [extension_install_prompt.cc(629)] Destructor [media_capture_devices_dispatcher.cc(103)] Destructor [public_session_media_access_handler.cc(36)] Destructor ReSolvePermissionPrompt is called before the destructor of PSMAH so it's safe, and the prompt is then also immediately destroyed. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.cc:87: extension, base::Passed(&prompt)), On 2016/11/30 19:02:48, Devlin wrote: > this strikes me as a bit dangerous - we're moving the object that we're actively > calling into... I wouldn't be entirely surprised if some compilers made this > dangerous. Changed it a bit, good now? https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.cc:139: if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) { On 2016/11/30 19:02:48, Devlin wrote: > optional: These methods would be more succinct if we did something like: > struct PermissionState { > bool prompted = false; > bool allowed = false; > } > std::map<Type, PermissionState> states > > and then here, just > if (IsPublicSession()) { > const PermissionState state = states[type]; > return !state.prompted || state.allowed; > } > > NeedsPrompting() { > return states[type].prompted; > } > > Set() { > PermissionState& state = states[type]; > DCHECK(!state.prompted); > state.prompted = true; > state.allowed = allowed; > } > > But, up to you. Agreed, but it would also complicate the logic in some places afaict. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.h:22: // where he can choose whether to allow the extension access to camera and/or On 2016/11/30 19:02:48, Devlin wrote: > drive-by nit: prefer "they" (I wonder why this wasn't caught by > https://cs.chromium.org/chromium/tools/depot_tools/presubmit_canned_checks.py... > ?) Done. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.h:25: class PublicSessionMediaAccessHandler : public MediaAccessHandler { On 2016/11/30 19:02:48, Devlin wrote: > What's the lifetime of this object? Let's merge this discussion with the one regarding safety of using base::Unretained. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... chrome/browser/media/public_session_media_access_handler.h:28: ExtensionMediaAccessHandler& extension_media_access_handler); On 2016/11/30 19:02:48, Devlin wrote: > we disallow non-const references passed into functions or as members by style. Acknowledged. https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/we... File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/we... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:90: new PublicSessionMediaAccessHandler(*extension_media_access_handler)); On 2016/12/01 01:21:28, Sergey Ulanov wrote: > On 2016/11/30 19:02:48, Devlin wrote: > > Hmm... this is a little weird. I wonder if it would make more sense to have > > PublicSessionMediaAccessHandler subclass ExtensionMediaAccessHandler, and then > > media_access_handlers_.push_back( > > IsPublicSession() ? > > new PublicSessionExtensionMediaAccessHandler() : > > new ExtensionMediaAccessHandler()); > > > > That way, we always use the right handler (rather than relying on the first in > > having first say). > > I think this is a good suggestions. Also add ifdef for OS_CHROMEOS here and > compile the new access handler only on ChromeOS Now I understand what Sergey originally meant with his comment to add a separate class that acts as a wrapper around the original; I'm new to some concepts :) Done to all, except I didn't use IsPublicSession() here because at this point the user hasn't logged in yet.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/01 17:32:05, Ivan Šandrk wrote: > Ptal guys :) > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/BUILD.gn > File chrome/browser/BUILD.gn (right): > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/BUILD.gn... > chrome/browser/BUILD.gn:1575: "media/public_session_media_access_handler.cc", > On 2016/11/30 19:02:48, Devlin wrote: > > Isn't public session only on CrOS? Can we avoid compiling this on all the > other > > platforms? > > Done. I'm not an expert on BUILD files, you can double check the new version. > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... > File chrome/browser/media/public_session_media_access_handler.cc (right): > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... > chrome/browser/media/public_session_media_access_handler.cc:36: } > On 2016/12/01 01:21:28, Sergey Ulanov wrote: > > 'git cl format' please. > > Done. > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... > chrome/browser/media/public_session_media_access_handler.cc:86: > base::Unretained(this), web_contents, request, callback, > On 2016/12/01 01:21:28, Sergey Ulanov wrote: > > On 2016/11/30 19:02:48, Devlin wrote: > > > Why is this unretained safe? > > > > +1 > > The lifetime of PublicSessionMediaAccessHandler is the same as lifetime of > MediaCaptureDevicesDispatcher (since it gets created in its constructor). MCDD > is managed by a base::Singleton and is created during first access and is > destroyed at normal process exit[1]. It's created sometime before login, and I'm > guessing it gets destroyed after logout during shutdown sequence. > > [1]: > https://cs.chromium.org/chromium/src/base/memory/singleton.h?cl=GROK&gsn=get&... > > Here I'm making the assumption that the newly created dialog cannot possibly > outlive the logged-in session (ie. if it's displayed during logout it gets > destroyed). I've added some logging code and this is the output when the dialog > is open and then we logout: > > [extension_install_prompt.cc(658)] ShowDialog > [public_session_media_access_handler.cc(134)] ResolvePermissionPrompt > [extension_install_prompt.cc(629)] Destructor > [media_capture_devices_dispatcher.cc(103)] Destructor > [public_session_media_access_handler.cc(36)] Destructor > > ReSolvePermissionPrompt is called before the destructor of PSMAH so it's safe, > and the prompt is then also immediately destroyed. > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... > chrome/browser/media/public_session_media_access_handler.cc:87: extension, > base::Passed(&prompt)), > On 2016/11/30 19:02:48, Devlin wrote: > > this strikes me as a bit dangerous - we're moving the object that we're > actively > > calling into... I wouldn't be entirely surprised if some compilers made this > > dangerous. > > Changed it a bit, good now? > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... > chrome/browser/media/public_session_media_access_handler.cc:139: if (type == > content::MEDIA_DEVICE_AUDIO_CAPTURE) { > On 2016/11/30 19:02:48, Devlin wrote: > > optional: These methods would be more succinct if we did something like: > > struct PermissionState { > > bool prompted = false; > > bool allowed = false; > > } > > std::map<Type, PermissionState> states > > > > and then here, just > > if (IsPublicSession()) { > > const PermissionState state = states[type]; > > return !state.prompted || state.allowed; > > } > > > > NeedsPrompting() { > > return states[type].prompted; > > } > > > > Set() { > > PermissionState& state = states[type]; > > DCHECK(!state.prompted); > > state.prompted = true; > > state.allowed = allowed; > > } > > > > But, up to you. > > Agreed, but it would also complicate the logic in some places afaict. > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... > File chrome/browser/media/public_session_media_access_handler.h (right): > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... > chrome/browser/media/public_session_media_access_handler.h:22: // where he can > choose whether to allow the extension access to camera and/or > On 2016/11/30 19:02:48, Devlin wrote: > > drive-by nit: prefer "they" (I wonder why this wasn't caught by > > > https://cs.chromium.org/chromium/tools/depot_tools/presubmit_canned_checks.py... > > ?) > > Done. > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... > chrome/browser/media/public_session_media_access_handler.h:25: class > PublicSessionMediaAccessHandler : public MediaAccessHandler { > On 2016/11/30 19:02:48, Devlin wrote: > > What's the lifetime of this object? > > Let's merge this discussion with the one regarding safety of using > base::Unretained. > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/pu... > chrome/browser/media/public_session_media_access_handler.h:28: > ExtensionMediaAccessHandler& extension_media_access_handler); > On 2016/11/30 19:02:48, Devlin wrote: > > we disallow non-const references passed into functions or as members by style. > > Acknowledged. > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/we... > File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): > > https://codereview.chromium.org/2532323003/diff/80001/chrome/browser/media/we... > chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:90: new > PublicSessionMediaAccessHandler(*extension_media_access_handler)); > On 2016/12/01 01:21:28, Sergey Ulanov wrote: > > On 2016/11/30 19:02:48, Devlin wrote: > > > Hmm... this is a little weird. I wonder if it would make more sense to have > > > PublicSessionMediaAccessHandler subclass ExtensionMediaAccessHandler, and > then > > > media_access_handlers_.push_back( > > > IsPublicSession() ? > > > new PublicSessionExtensionMediaAccessHandler() : > > > new ExtensionMediaAccessHandler()); > > > > > > That way, we always use the right handler (rather than relying on the first > in > > > having first say). > > > > I think this is a good suggestions. Also add ifdef for OS_CHROMEOS here and > > compile the new access handler only on ChromeOS > > Now I understand what Sergey originally meant with his comment to add a separate > class that acts as a wrapper around the original; I'm new to some concepts :) > Done to all, except I didn't use IsPublicSession() here because at this point > the user hasn't logged in yet. Hey Devlin can you please follow up on the comments you made
Looks mostly good to me. I just have some style suggestions. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:59: if (!(IsPublicSession() && extension && extension->is_platform_app())) don't need to check extension here. This function should never be called with extesnion==nullptr, because SupportsStreamType() would return false otherwise. Maybe add DCHECK(extension). Also it would be more readable as (!IsPublicSession() || !extension->is_platform_app()). https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:60: return ExtensionMediaAccessHandler::HandleRequest(web_contents, request, need {} for multi-line if blocks: https://google.github.io/styleguide/cppguide.html#Conditionals https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:76: if (needs_prompt) { The code would be more readable if you handle !needs_prompt first: if (!needs_prompt) { ChainHandleRequest(web_contents, request, callback, extension); return; } .. show prompt. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:145: if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) { nit: Maybe replace this with a switch statement Same for the functions below https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:161: return false; add NOTREACHED() here? https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:24: class PublicSessionMediaAccessHandler : public ExtensionMediaAccessHandler { When I suggested to make PublicSessionMediaAccessHandler a wrapper for ExtensionMediaAccessHandler I meant that it should encapsulate it, not inherit from it. Style guide says: "Composition is often more appropriate than inheritance." and I think it's applicable here. It would make this code less error prone, e.g. when MediaAccessHandler interface is updated. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:75: std::map<extensions::ExtensionId, UserChoice> user_choice_cache_; I'm not very familiar with public sessions. Is it possible for one user to log off and another one to log in there? Do we need to reset user_choice_cache_ in that case? https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:92: media_access_handlers_.push_back(new PublicSessionMediaAccessHandler()); Is it possible to check chromeos::LoginState::Get()->IsPublicSessionUser() here instead of in PublicSessionMediaAccessHandler? I.e. can be still create ExtensionMediaAccessHandler() for normal sessions?
Should there be a test for this? Also, it would be great if you can add a screenshot to the bug with how this looks with the install prompt usage, since it's a little strange (but I think it should work). https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:23: #if defined(OS_CHROMEOS) This should only be compiled on CrOS; remove ifdef https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:24: if (chromeos::LoginState::IsInitialized()) { simplify to return chromeos::LoginState::IsInitialized() && chromeos::LoginState::Get()->IsPublicSessionUser(); https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:40: return ExtensionMediaAccessHandler::SupportsStreamType(type, extension); No need to override this if we just return the parent class's impl. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:48: return ExtensionMediaAccessHandler::CheckMediaAccessPermission( ditto https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:76: if (needs_prompt) { On 2016/12/02 20:29:47, Sergey Ulanov wrote: > The code would be more readable if you handle !needs_prompt first: > > if (!needs_prompt) { > ChainHandleRequest(web_contents, request, callback, extension); > return; > } > > .. show prompt. (+1) https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:132: bool allowed = (prompt_result == ExtensionInstallPrompt::Result::ACCEPTED); nit: parens unnecessary https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:147: } else { I like Sergey's idea about the switch, so probably moot, but for future reference no "else" after a return. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:171: DCHECK(type == content::MEDIA_DEVICE_VIDEO_CAPTURE); nit: DCHECK_EQ https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:26: explicit PublicSessionMediaAccessHandler(); no need for explicit except in single-parameter ctors. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:75: std::map<extensions::ExtensionId, UserChoice> user_choice_cache_; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:44: #include "chrome/browser/media/public_session_media_access_handler.h" I *think* CrOS always implies extensions, so this can just go in the block above (line 38).
https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:412: // User is prompted (allow/deny) when an extension requests audioCapture. Clarify this - "the user is prompted when the extension calls <some API>" - explicitly state which API, both here and for videoCapture. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:88: // together with the callback. The callback is guaranteed to be destroyed how is the destruction of the callback guaranteed? This is OK, but these types of mechanisms tend to be fragile - why not store a unique_ptr to the dialog along with the UserChoice for this extension (we can only have one prompt per extension, right?) - that way you know it gets freed when this object is destroyed, and you can free the dialog when ResolvePermissionPrompt is invoked and the user choice is updated. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:115: if (!user_choice.IsAllowed(content::MEDIA_DEVICE_AUDIO_CAPTURE)) Can you comment about what this code is doing? https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:75: std::map<extensions::ExtensionId, UserChoice> user_choice_cache_; On 2016/12/02 20:29:47, Sergey Ulanov wrote: > I'm not very familiar with public sessions. Is it possible for one user to log > off and another one to log in there? Do we need to reset user_choice_cache_ in > that case? The profile is destroyed across sessions (the browser process itself also, I believe?) so I think we're good. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:92: media_access_handlers_.push_back(new PublicSessionMediaAccessHandler()); On 2016/12/02 20:29:47, Sergey Ulanov wrote: > Is it possible to check chromeos::LoginState::Get()->IsPublicSessionUser() here > instead of in PublicSessionMediaAccessHandler? I.e. can be still create > ExtensionMediaAccessHandler() for normal sessions? +1 to this - that would be my preference also.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've addressed all of Sergey's and Devlin's comments so far. Added a screenshot to the bug. About the test I'm unsure, ExtensionMediaAccessHandler doesn't have a test. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:23: #if defined(OS_CHROMEOS) On 2016/12/02 20:40:28, Devlin wrote: > This should only be compiled on CrOS; remove ifdef Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:24: if (chromeos::LoginState::IsInitialized()) { On 2016/12/02 20:40:28, Devlin wrote: > simplify to > return chromeos::LoginState::IsInitialized() && > chromeos::LoginState::Get()->IsPublicSessionUser(); Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:40: return ExtensionMediaAccessHandler::SupportsStreamType(type, extension); On 2016/12/02 20:40:28, Devlin wrote: > No need to override this if we just return the parent class's impl. Acknowledged. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:48: return ExtensionMediaAccessHandler::CheckMediaAccessPermission( On 2016/12/02 20:40:28, Devlin wrote: > ditto Acknowledged. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:59: if (!(IsPublicSession() && extension && extension->is_platform_app())) On 2016/12/02 20:29:47, Sergey Ulanov wrote: > don't need to check extension here. This function should never be called with > extesnion==nullptr, because SupportsStreamType() would return false otherwise. > Maybe add DCHECK(extension). > Also it would be more readable as (!IsPublicSession() || > !extension->is_platform_app()). Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:60: return ExtensionMediaAccessHandler::HandleRequest(web_contents, request, On 2016/12/02 20:29:47, Sergey Ulanov wrote: > need {} for multi-line if blocks: > https://google.github.io/styleguide/cppguide.html#Conditionals Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:76: if (needs_prompt) { On 2016/12/02 20:40:28, Devlin wrote: > On 2016/12/02 20:29:47, Sergey Ulanov wrote: > > The code would be more readable if you handle !needs_prompt first: > > > > if (!needs_prompt) { > > ChainHandleRequest(web_contents, request, callback, extension); > > return; > > } > > > > .. show prompt. > > (+1) Good point! https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:132: bool allowed = (prompt_result == ExtensionInstallPrompt::Result::ACCEPTED); On 2016/12/02 20:40:28, Devlin wrote: > nit: parens unnecessary Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:145: if (type == content::MEDIA_DEVICE_AUDIO_CAPTURE) { On 2016/12/02 20:29:47, Sergey Ulanov wrote: > nit: Maybe replace this with a switch statement > Same for the functions below Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:147: } else { On 2016/12/02 20:40:28, Devlin wrote: > I like Sergey's idea about the switch, so probably moot, but for future > reference no "else" after a return. Good point. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:161: return false; On 2016/12/02 20:29:47, Sergey Ulanov wrote: > add NOTREACHED() here? This part is hit sometimes (when type is content::MEDIA_NO_SERVICE, ie. for example video wasn't requested, only audio). I can also replace this function with: return type == content::MEDIA_DEVICE_AUDIO_CAPTURE && !audio_prompted_ || type == content::MEDIA_DEVICE_VIDEO_CAPTURE && !video_prompted_; Does that seem more readable? https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:171: DCHECK(type == content::MEDIA_DEVICE_VIDEO_CAPTURE); On 2016/12/02 20:40:28, Devlin wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:24: class PublicSessionMediaAccessHandler : public ExtensionMediaAccessHandler { On 2016/12/02 20:29:47, Sergey Ulanov wrote: > When I suggested to make PublicSessionMediaAccessHandler a wrapper for > ExtensionMediaAccessHandler I meant that it should encapsulate it, not inherit > from it. Style guide says: "Composition is often more appropriate than > inheritance." and I think it's applicable here. It would make this code less > error prone, e.g. when MediaAccessHandler interface is updated. Done. I hope that's what you had in mind. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:26: explicit PublicSessionMediaAccessHandler(); On 2016/12/02 20:40:28, Devlin wrote: > no need for explicit except in single-parameter ctors. Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:75: std::map<extensions::ExtensionId, UserChoice> user_choice_cache_; On 2016/12/02 20:29:47, Sergey Ulanov wrote: > I'm not very familiar with public sessions. Is it possible for one user to log > off and another one to log in there? Do we need to reset user_choice_cache_ in > that case? Yes it's possible, and yes it needs resetting. I was told by one person that this place *should* be reset automatically after user logs out, that no data is kept between sessions in memory, but that may not be correct. I was testing this so far only using a local chrome_os=1 build which dies after a logout for some reason, I need to do a test on an actual device. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:75: std::map<extensions::ExtensionId, UserChoice> user_choice_cache_; On 2016/12/02 20:40:28, Devlin wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/w... File chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:44: #include "chrome/browser/media/public_session_media_access_handler.h" On 2016/12/02 20:40:28, Devlin wrote: > I *think* CrOS always implies extensions, so this can just go in the block above > (line 38). Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/w... chrome/browser/media/webrtc/media_capture_devices_dispatcher.cc:92: media_access_handlers_.push_back(new PublicSessionMediaAccessHandler()); On 2016/12/02 20:29:47, Sergey Ulanov wrote: > Is it possible to check chromeos::LoginState::Get()->IsPublicSessionUser() here > instead of in PublicSessionMediaAccessHandler? I.e. can be still create > ExtensionMediaAccessHandler() for normal sessions? This piece of code is executed before first login so we won't know whether the user will actually use the Public Session. I can make the PublicSessionMediaAccessHandler::SupportsStreamType function return true only in Public Sessions, that way we can use both. https://codereview.chromium.org/2532323003/diff/160001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/160001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:74: return ChainHandleRequest(web_contents, request, callback, extension); Returning a void - a bad idea or clever removal of one additional line?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:412: // User is prompted (allow/deny) when an extension requests audioCapture. On 2016/12/04 19:57:01, Andrew T Wilson (Slow) wrote: > Clarify this - "the user is prompted when the extension calls <some API>" - > explicitly state which API, both here and for videoCapture. Done. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:88: // together with the callback. The callback is guaranteed to be destroyed On 2016/12/04 19:57:01, Andrew T Wilson (Slow) wrote: > how is the destruction of the callback guaranteed? This is OK, but these types > of mechanisms tend to be fragile - why not store a unique_ptr to the dialog > along with the UserChoice for this extension (we can only have one prompt per > extension, right?) - that way you know it gets freed when this object is > destroyed, and you can free the dialog when ResolvePermissionPrompt is invoked > and the user choice is updated. Changed the approach. https://codereview.chromium.org/2532323003/diff/140001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:115: if (!user_choice.IsAllowed(content::MEDIA_DEVICE_AUDIO_CAPTURE)) On 2016/12/04 19:57:01, Andrew T Wilson (Slow) wrote: > Can you comment about what this code is doing? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This all looks reasonable to me. Since I'm not an owner here, I'll leave it up to others if they think a test is needed. https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:126: extension_install_prompt_map_[extension->id()].reset(nullptr); This will keep the entry in the map. Instead, we should use erase().
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Patchset #11 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly LG https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:55: if (!IsPublicSession() || !extension->is_platform_app()) { Why are we skipping the public-sessions logic for non-platform apps? I thought we *always* wanted to display our prompts if we are in a public session? https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:126: extension_install_prompt_map_[extension->id()].reset(nullptr); This is fine. I know that .reset() without a param also works, but not sure which is the preferred idiom. https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:20: // In Public Sessions extensions are installed by admin policy hence they can This is a bit confusing, I'd probably phrase it more like this: In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security/privacy standpoint, so when an app uses the capture APIs for the first time, we show the user a dialog where they can choose... https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:78: std::map<extensions::ExtensionId, std::unique_ptr<ExtensionInstallPrompt>> This is fine, but curious why we don't put this in UserChoice?
Mostly LG
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've addressed the comments. I'm still unable to test this on an actual device, keep running into unrelated issues. https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:55: if (!IsPublicSession() || !extension->is_platform_app()) { On 2016/12/06 02:49:45, Andrew T Wilson (Slow) wrote: > Why are we skipping the public-sessions logic for non-platform apps? I thought > we *always* wanted to display our prompts if we are in a public session? C/P from the description: Only platform apps (and some built-in whitelisted extensions) support audioCapture[1]/videoCapture[2] manifest permission features therefore this filtering will be limited to platform apps. https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:126: extension_install_prompt_map_[extension->id()].reset(nullptr); On 2016/12/05 19:25:12, Devlin wrote: > This will keep the entry in the map. Instead, we should use erase(). Done. https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:20: // In Public Sessions extensions are installed by admin policy hence they can On 2016/12/06 02:49:45, Andrew T Wilson (Slow) wrote: > This is a bit confusing, I'd probably phrase it more like this: > > In Public Sessions, apps and extensions are force-installed by admin policy so > the user does not get a chance to review the permissions for these apps. This is > not acceptable from a security/privacy standpoint, so when an app uses the > capture APIs for the first time, we show the user a dialog where they can > choose... Done. https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:78: std::map<extensions::ExtensionId, std::unique_ptr<ExtensionInstallPrompt>> On 2016/12/06 02:49:45, Andrew T Wilson (Slow) wrote: > This is fine, but curious why we don't put this in UserChoice? Seemed better to me to logically separate them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/06 14:02:39, Ivan Šandrk wrote: > I've addressed the comments. I'm still unable to test this on an actual device, > keep running into unrelated issues. > > https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... > File chrome/browser/media/public_session_media_access_handler.cc (right): > > https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... > chrome/browser/media/public_session_media_access_handler.cc:55: if > (!IsPublicSession() || !extension->is_platform_app()) { > On 2016/12/06 02:49:45, Andrew T Wilson (Slow) wrote: > > Why are we skipping the public-sessions logic for non-platform apps? I thought > > we *always* wanted to display our prompts if we are in a public session? > > C/P from the description: > > Only platform apps (and some built-in whitelisted extensions) support > audioCapture[1]/videoCapture[2] manifest permission features therefore this > filtering will be limited to platform apps. > > https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... > chrome/browser/media/public_session_media_access_handler.cc:126: > extension_install_prompt_map_[extension->id()].reset(nullptr); > On 2016/12/05 19:25:12, Devlin wrote: > > This will keep the entry in the map. Instead, we should use erase(). > > Done. > > https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... > File chrome/browser/media/public_session_media_access_handler.h (right): > > https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... > chrome/browser/media/public_session_media_access_handler.h:20: // In Public > Sessions extensions are installed by admin policy hence they can > On 2016/12/06 02:49:45, Andrew T Wilson (Slow) wrote: > > This is a bit confusing, I'd probably phrase it more like this: > > > > In Public Sessions, apps and extensions are force-installed by admin policy so > > the user does not get a chance to review the permissions for these apps. This > is > > not acceptable from a security/privacy standpoint, so when an app uses the > > capture APIs for the first time, we show the user a dialog where they can > > choose... > > Done. > > https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... > chrome/browser/media/public_session_media_access_handler.h:78: > std::map<extensions::ExtensionId, std::unique_ptr<ExtensionInstallPrompt>> > On 2016/12/06 02:49:45, Andrew T Wilson (Slow) wrote: > > This is fine, but curious why we don't put this in UserChoice? > > Seemed better to me to logically separate them. Tested on an actual device, works as expected. So the user_choice_cache_ is reset between user sessions.
LGTM, but note my comment below. Want to make sure that if somehow extensions *can* capture audio/video, that they still get a prompt. https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:55: if (!IsPublicSession() || !extension->is_platform_app()) { On 2016/12/06 14:02:39, Ivan Šandrk wrote: > On 2016/12/06 02:49:45, Andrew T Wilson (Slow) wrote: > > Why are we skipping the public-sessions logic for non-platform apps? I thought > > we *always* wanted to display our prompts if we are in a public session? > > C/P from the description: > > Only platform apps (and some built-in whitelisted extensions) support > audioCapture[1]/videoCapture[2] manifest permission features therefore this > filtering will be limited to platform apps. Just not clear that we need to explicitly check for platform apps here then, if extensions can't actually request those permissions? In fact, it feels safer to *always* prompt, so I'd rather remove the is_platform_app() check here.
Sergey can you ptal again please? https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/180001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:55: if (!IsPublicSession() || !extension->is_platform_app()) { On 2016/12/06 16:57:01, Andrew T Wilson (Slow) wrote: > On 2016/12/06 14:02:39, Ivan Šandrk wrote: > > On 2016/12/06 02:49:45, Andrew T Wilson (Slow) wrote: > > > Why are we skipping the public-sessions logic for non-platform apps? I > thought > > > we *always* wanted to display our prompts if we are in a public session? > > > > C/P from the description: > > > > Only platform apps (and some built-in whitelisted extensions) support > > audioCapture[1]/videoCapture[2] manifest permission features therefore this > > filtering will be limited to platform apps. > > Just not clear that we need to explicitly check for platform apps here then, if > extensions can't actually request those permissions? In fact, it feels safer to > *always* prompt, so I'd rather remove the is_platform_app() check here. Sergey made a comment about it here https://codereview.chromium.org/2532323003/diff/20001/chrome/browser/media/ex...
lgtm
couple more nits https://codereview.chromium.org/2532323003/diff/220001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/220001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:95: extension_install_prompt_map_[extension->id()].reset(prompt.release()); extension_install_prompt_map_[extension->id()] = std::move(prompt); https://codereview.chromium.org/2532323003/diff/220001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:165: content::MediaStreamType type, bool allowed) { one argument per line please, git cl format
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2532323003/diff/220001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2532323003/diff/220001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:95: extension_install_prompt_map_[extension->id()].reset(prompt.release()); On 2016/12/08 05:10:46, Sergey Ulanov wrote: > extension_install_prompt_map_[extension->id()] = std::move(prompt); Good point! https://codereview.chromium.org/2532323003/diff/220001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:165: content::MediaStreamType type, bool allowed) { On 2016/12/08 05:10:46, Sergey Ulanov wrote: > one argument per line please, git cl format Done.
The CQ bit was unchecked by isandrk@chromium.org
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/2532323003/#ps240001 (title: "std::move, formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481205154556970,
"parent_rev": "0a0a9e28e7dcd44de11cef62025e20697f218a9b", "commit_rev":
"baf878f5546f3c7b9581527226e8a8fb9aab1e3c"}
Message was sent while issue was closed.
Description was changed from ========== Public Sessions - prompt the user for audioCapture/videoCapture requests In Public Sessions extensions are force installed by admin policy and some of the permissions are granted at install time so the end user has no say on it. audioCapture and videoCapture APIs are dangerous from a security/privacy standpoint therefore we want to give the user a runtime prompt whether to allow such API. Only platform apps (and some built-in whitelisted extensions) support audioCapture[1]/videoCapture[2] manifest permission features therefore this filtering will be limited to platform apps. [1]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... [2]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... BUG=669521 ========== to ========== Public Sessions - prompt the user for audioCapture/videoCapture requests In Public Sessions extensions are force installed by admin policy and some of the permissions are granted at install time so the end user has no say on it. audioCapture and videoCapture APIs are dangerous from a security/privacy standpoint therefore we want to give the user a runtime prompt whether to allow such API. Only platform apps (and some built-in whitelisted extensions) support audioCapture[1]/videoCapture[2] manifest permission features therefore this filtering will be limited to platform apps. [1]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... [2]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... BUG=669521 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Public Sessions - prompt the user for audioCapture/videoCapture requests In Public Sessions extensions are force installed by admin policy and some of the permissions are granted at install time so the end user has no say on it. audioCapture and videoCapture APIs are dangerous from a security/privacy standpoint therefore we want to give the user a runtime prompt whether to allow such API. Only platform apps (and some built-in whitelisted extensions) support audioCapture[1]/videoCapture[2] manifest permission features therefore this filtering will be limited to platform apps. [1]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... [2]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... BUG=669521 ========== to ========== Public Sessions - prompt the user for audioCapture/videoCapture requests In Public Sessions extensions are force installed by admin policy and some of the permissions are granted at install time so the end user has no say on it. audioCapture and videoCapture APIs are dangerous from a security/privacy standpoint therefore we want to give the user a runtime prompt whether to allow such API. Only platform apps (and some built-in whitelisted extensions) support audioCapture[1]/videoCapture[2] manifest permission features therefore this filtering will be limited to platform apps. [1]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... [2]: https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur... BUG=669521 Committed: https://crrev.com/b040fda5a81f54c4dbab7664f3aaf2099c110b61 Cr-Commit-Position: refs/heads/master@{#437243} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/b040fda5a81f54c4dbab7664f3aaf2099c110b61 Cr-Commit-Position: refs/heads/master@{#437243} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
