Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558843002:
Public Sessions - prompt the user for tabCapture 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 tabCapture requests
In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the TabCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API.
This CL will also whitelist desktopCapture manifest permission feature as it's already safe in its current form (user is prompted) together with tabCapture.
BUG=672093
Committed: https://crrev.com/1f9314626715713912e784d10ada39770fa80c1f
Cr-Commit-Position: refs/heads/master@{#438594}
Patch Set 1 #Patch Set 2 : Updated comments #
Total comments: 8
Patch Set 3 : std::move, git cl format #
Total comments: 7
Patch Set 4 : Drew's nits #Patch Set 5 : Rebase #
Messages
Total messages: 40 (24 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: 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...
isandrk@chromium.org changed reviewers: + atwilson@chromium.org, sergeyu@chromium.org
Hey Sergey, please take a look at c/b/m/webrtc/*. This is the 2nd CL, there will also be a 3rd one for pageCapture. Ivan
Description was changed from ========== Public Sessions - prompt the user for desktopCapture/tabCapture/pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses one of the capture APIs for the first time, we show the user a dialog where they can choose whether to allow the extension access to that API. BUG=672093 ========== to ========== Public Sessions - prompt the user for tabCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the TabCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=672093 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sergeyu@chromium.org changed reviewers: + miu@chromium.org
+miu, in case he has any feedback lgtm https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:457: "desktopCapture", Please mention this change in the CL description. https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/media/we... File chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc (right): https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc:23: return chromeos::LoginState::IsInitialized() && Is there a case when LoginState may not be initialized when we get access request for the API? If there is a valid reason for it not being initialized, maybe we should just reject all there requests because there is no proper way to handle them? https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc:88: extension_install_prompt_map_[extension->id()].reset(prompt.release()); extension_install_prompt_map_[extension->id()] = std::move(prompt); https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc:109: callback, extension); 'git cl format' please to fix intentation
Description was changed from ========== Public Sessions - prompt the user for tabCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the TabCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=672093 ========== to ========== Public Sessions - prompt the user for tabCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the TabCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. This CL will also whitelist desktopCapture manifest permission feature as it's already safe in its current form (user is prompted) together with tabCapture. BUG=672093 ==========
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/2558843002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:457: "desktopCapture", On 2016/12/08 05:10:35, Sergey Ulanov wrote: > Please mention this change in the CL description. Done. https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/media/we... File chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc (right): https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc:23: return chromeos::LoginState::IsInitialized() && On 2016/12/08 05:10:35, Sergey Ulanov wrote: > Is there a case when LoginState may not be initialized when we get access > request for the API? > If there is a valid reason for it not being initialized, maybe we should just > reject all there requests because there is no proper way to handle them? LoginState is always initialized after login on CrOS. The exception are tests where sometimes it is not initialized. https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc:88: extension_install_prompt_map_[extension->id()].reset(prompt.release()); On 2016/12/08 05:10:35, Sergey Ulanov wrote: > extension_install_prompt_map_[extension->id()] = std::move(prompt); Done. https://codereview.chromium.org/2558843002/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc:109: callback, extension); On 2016/12/08 05:10:35, Sergey Ulanov wrote: > 'git cl format' please to fix intentation Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
I don't understand why all this code in a "webrtc" directory. It's not webrtc-specific at all. Please move the files in a follow-up change. Comments on PS3: https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:632: "tabCapture", This should be "tabCapture.capture" since the captureOffscreenTab() API is meant to be used without any user-granted permissions. There's already been a lengthy security discussion around this, but it'd make more sense if you take a look at some of the records: 1. See extension API launch spreadsheet for proposal/design doc links and privacy/security review discussion. 2. The API comments in the IDL explains some things too: https://cs.chromium.org/chromium/src/chrome/common/extensions/api/tab_capture... Note: captureOffscreenTab() is something we're planning on launching soon (prob Q1).
On 2016/12/08 21:18:18, miu wrote: > I don't understand why all this code in a "webrtc" directory. It's not > webrtc-specific at all. Please move the files in a follow-up change. Currently all access control logic for getUserMedia() API is in that directory, including tabCapture, see chrome/browser/media/webrtc/tab_capture_access_handler.h . I don't think it would be fair to say that getUserMedia() is not related to webrtc.
On 2016/12/08 22:07:20, Sergey Ulanov wrote: > On 2016/12/08 21:18:18, miu wrote: > > I don't understand why all this code in a "webrtc" directory. It's not > > webrtc-specific at all. Please move the files in a follow-up change. > > Currently all access control logic for getUserMedia() API is in that directory, > including tabCapture, see > chrome/browser/media/webrtc/tab_capture_access_handler.h . I don't think it > would be fair to say that getUserMedia() is not related to webrtc. Sure, but it's also related to a lot of things that are not WebRTC. ;-) Let's make sure our placement of code modules lines-up with functionality, encapsulation, and abstraction; and is not just a reflection of the Google org chart. Here, we have code that handles access permissions so that the web may capture media. Capture is a concept that is separate from (and upstream of) the implementation that consumes the captured media. WebRTC is only one such consumer. PPAPI, Cast Streaming, the Recording API, WebAudio, HTMLMediaElement, etc.; are all non-WebRTC consumers of MediaStreams (see https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-z...). While it's true that MediaStreams originated for use with WebRTC, there have been several non-WebRTC consumers for over four years now.
Thanks for you input Yuri. I will do some cleanup afterwards. https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:632: "tabCapture", On 2016/12/08 21:18:18, miu wrote: > This should be "tabCapture.capture" since the captureOffscreenTab() API is meant > to be used without any user-granted permissions. There's already been a lengthy > security discussion around this, but it'd make more sense if you take a look at > some of the records: > > 1. See extension API launch spreadsheet for proposal/design doc links and > privacy/security review discussion. > > 2. The API comments in the IDL explains some things too: > https://cs.chromium.org/chromium/src/chrome/common/extensions/api/tab_capture... > > Note: captureOffscreenTab() is something we're planning on launching soon (prob > Q1). This is actually fine, this whitelists all tabCapture.* things. In Public Sessions APIs need to be explicitly whitelisted, that's what security wanted. Just one question for you - does the new captureOffscreenTab use the same codepath as capture? Ie. TabCaptureAccessHandler::HandleRequest. If it does - how can I differentiate between them? If not, everything is fine :-)
lgtm https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:632: "tabCapture", On 2016/12/09 15:30:17, Ivan Šandrk wrote: > Just one question for you - does the new captureOffscreenTab use the same > codepath as capture? Ie. TabCaptureAccessHandler::HandleRequest. If it does - > how can I differentiate between them? If not, everything is fine :-) Same code paths, IIRC. The way you'd tell the difference: An offscreen tab will not show up in the browser's TabStripModel, nor SessionTabHelper, nor is it visible to the chrome.tabs APIs.
Hey Drew, please take a look! Ivan
LGTM with two small nits https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc/public_session_tab_capture_access_handler.h (right): https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.h:17: // access control to TabCapture manifest permission feature inside of Public nit: to the TabCapture manifest permission https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.h:26: class PublicSessionTabCaptureAccessHandler : public CaptureAccessHandlerBase { You have a bug for refactoring this code to share code with PublicSessionMediaAccessHandler - can you put a TODO in this header with that bug number referenced?
https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc/public_session_tab_capture_access_handler.h (right): https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.h:17: // access control to TabCapture manifest permission feature inside of Public On 2016/12/14 16:07:46, Andrew T Wilson (Slow) wrote: > nit: to the TabCapture manifest permission Done. https://codereview.chromium.org/2558843002/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.h:26: class PublicSessionTabCaptureAccessHandler : public CaptureAccessHandlerBase { On 2016/12/14 16:07:45, Andrew T Wilson (Slow) wrote: > You have a bug for refactoring this code to share code with > PublicSessionMediaAccessHandler - can you put a TODO in this header with that > bug number referenced? Done.
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, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2558843002/#ps60001 (title: "Drew's nits")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2558843002/#ps80001 (title: "Rebase")
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": 80001, "attempt_start_ts": 1481740343266700,
"parent_rev": "149624c9a4bc56f7f915b52c0568939a3e6407b1", "commit_rev":
"48fdcaac9504caa7cf3a1b7cdcc16ba7547e0645"}
Message was sent while issue was closed.
Description was changed from ========== Public Sessions - prompt the user for tabCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the TabCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. This CL will also whitelist desktopCapture manifest permission feature as it's already safe in its current form (user is prompted) together with tabCapture. BUG=672093 ========== to ========== Public Sessions - prompt the user for tabCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the TabCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. This CL will also whitelist desktopCapture manifest permission feature as it's already safe in its current form (user is prompted) together with tabCapture. BUG=672093 Review-Url: https://codereview.chromium.org/2558843002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Public Sessions - prompt the user for tabCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the TabCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. This CL will also whitelist desktopCapture manifest permission feature as it's already safe in its current form (user is prompted) together with tabCapture. BUG=672093 Review-Url: https://codereview.chromium.org/2558843002 ========== to ========== Public Sessions - prompt the user for tabCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the TabCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. This CL will also whitelist desktopCapture manifest permission feature as it's already safe in its current form (user is prompted) together with tabCapture. BUG=672093 Committed: https://crrev.com/1f9314626715713912e784d10ada39770fa80c1f Cr-Commit-Position: refs/heads/master@{#438594} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1f9314626715713912e784d10ada39770fa80c1f Cr-Commit-Position: refs/heads/master@{#438594} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
