Make tab capture media stream requests verify that the request came from extension API

Make tab capture media stream requests verify that the request came from extension API.

Checking that the security_origin of a getUserMedia call for tab media is an extension is not enough. We want to also verify that the extension has the correct permissions to make the call. This is to prevent extensions from calling getUserMedia({tab-media-params}) directly without going through the extension API.

This CL makes it so that tab media requests will go through the webcam/mic media stream infobar permission check, but we will accept right away after verifying that the request is in the TabCaptureRegistry (this is the change in media_stream_devices_controller.cc).

If the request comes from an extension background page or packaged app, we check that the extension/app has the tab capture permission, and we will approve it right away (these are the changes in extension_host.cc and shell_window.cc).

[justinlin, 2012-10-18 02:47:47]

Hi,

xians@, wjia@: please take a look at the media stream changes.
aa@: please review the extension_host.cc and tests changes.

[wjia(left Chromium), 2012-10-18 04:54:27]

https://codereview.chromium.org/11198044/diff/4001/chrome/browser/media/media...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/11198044/diff/4001/chrome/browser/media/media...
chrome/browser/media/media_stream_devices_controller.cc:72:
request_.devices.find(content::MEDIA_TAB_VIDEO_CAPTURE);
MEDIA_TAB_AUDIO_CAPTURE?

https://codereview.chromium.org/11198044/diff/4001/chrome/browser/media/media...
chrome/browser/media/media_stream_devices_controller.cc:92: false);
nit: this fits in one line.

https://codereview.chromium.org/11198044/diff/4001/chrome/browser/media/media...
chrome/browser/media/media_stream_devices_controller.cc:94: return true;
It seems always returning true. Do you really need this return type?

[justinlin, 2012-10-18 07:05:47]

http://codereview.chromium.org/11198044/diff/4001/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/11198044/diff/4001/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:72:
request_.devices.find(content::MEDIA_TAB_VIDEO_CAPTURE);
On 2012/10/18 04:54:27, wjia wrote:
> MEDIA_TAB_AUDIO_CAPTURE?

Done. Thanks.

http://codereview.chromium.org/11198044/diff/4001/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:92: false);
On 2012/10/18 04:54:27, wjia wrote:
> nit: this fits in one line.

Done.

http://codereview.chromium.org/11198044/diff/4001/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:94: return true;
On 2012/10/18 04:54:27, wjia wrote:
> It seems always returning true. Do you really need this return type?

There seems to be 1 case below where it returns false.

[no longer working on chromium, 2012-10-18 08:36:16]

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:653: if (!accepted_an_audio_device
&& content::IsAudioMediaType(it->first) &&
fix the alignment to help readability.
If (*** &&
    *** &&
    ***)

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:656: if (it->first !=
content::MEDIA_TAB_AUDIO_CAPTURE ||
I don't understand here, shouldn't it be
it->first == content::MEDIA_TAB_AUDIO_CAPTURE &&
extension()->HasAPIPermission(APIPermission::kTabCapture)

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.h:176: const
content::MediaStreamRequest* request,
use a reference instead of a pointer.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:78:
DCHECK(tab_audio->second.size() || tab_video->second.size());
use !tab_audio->second.empty(), the same for other places.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:88: video_device_id :
audio_device_id))
Does it mean that we will have either video_device_id or audio_device_id, and
never both?

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:88: video_device_id :
audio_device_id))
add a { because "if" takes two line.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:208: if ((it->first !=
content::MEDIA_TAB_AUDIO_CAPTURE &&
it should be it->first != content::MEDIA_DEVICE_AUDIO_CAPTURE

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:210:
extension()->HasAPIPermission(APIPermission::kTabCapture)) {
indentation.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:210:
extension()->HasAPIPermission(APIPermission::kTabCapture)) {
Dont you need to 
it->first == content::MEDIA_TAB_AUDIO_CAPTURE &&
extension()->HasAPIPermission(APIPermission::kTabCapture

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:216: if ((it->first !=
content::MEDIA_TAB_VIDEO_CAPTURE &&
ditto

[justinlin, 2012-10-18 09:33:29]

Thanks! Made some of the statements hopefully less confusing.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:653: if (!accepted_an_audio_device
&& content::IsAudioMediaType(it->first) &&
On 2012/10/18 08:36:17, xians1 wrote:
> fix the alignment to help readability.
> If (*** &&
>     *** &&
>     ***)

Done.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:656: if (it->first !=
content::MEDIA_TAB_AUDIO_CAPTURE ||
On 2012/10/18 08:36:17, xians1 wrote:
> I don't understand here, shouldn't it be
> it->first == content::MEDIA_TAB_AUDIO_CAPTURE &&
> extension()->HasAPIPermission(APIPermission::kTabCapture)

Sorry if this is confusing. We add the device if it's (not a tab_capture device
so we don't care about permission) or (if it is tab capture device and we have
the permission). The "it->first == content::MEDIA_TAB_AUDIO_CAPTURE" is implied
with the permission check. Made the comment longer.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.h:176: const
content::MediaStreamRequest* request,
On 2012/10/18 08:36:17, xians1 wrote:
> use a reference instead of a pointer.

This is an override. Probably better to fix this in a cleanup CL if needed.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:78:
DCHECK(tab_audio->second.size() || tab_video->second.size());
On 2012/10/18 08:36:17, xians1 wrote:
> use !tab_audio->second.empty(), the same for other places.

Done.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:88: video_device_id :
audio_device_id))
On 2012/10/18 08:36:17, xians1 wrote:
> add a { because "if" takes two line.

Done.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:88: video_device_id :
audio_device_id))
On 2012/10/18 08:36:17, xians1 wrote:
> Does it mean that we will have either video_device_id or audio_device_id, and
> never both?

No, this is a little bit related to http://crbug.com/155444. The issue is that
DeviceRequest only has one requested_device_id, so we key the registry (where
extensions keep track of the media stream requests for tabs) by a single
device_id. If video is request, then we use that one, if not we use the one from
audio, but they will both be the same anyway. I think there's checks before this
that prevent us from getting here without a tab video device right now, but if
later we allowed only capturing tab audio, then we might not have a tab video
device here.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:208: if ((it->first !=
content::MEDIA_TAB_AUDIO_CAPTURE &&
On 2012/10/18 08:36:17, xians1 wrote:
> it should be it->first != content::MEDIA_DEVICE_AUDIO_CAPTURE

Sorry, tried to be clever and probably made it too confusing. Here it's saying
for anything that's not tab, we need kAudioCapture permission, otherwise we need
kTabCapture permission. Made it more clear.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:210:
extension()->HasAPIPermission(APIPermission::kTabCapture)) {
On 2012/10/18 08:36:17, xians1 wrote:
> indentation.

Done.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:210:
extension()->HasAPIPermission(APIPermission::kTabCapture)) {
On 2012/10/18 08:36:17, xians1 wrote:
> Dont you need to 
> it->first == content::MEDIA_TAB_AUDIO_CAPTURE &&
> extension()->HasAPIPermission(APIPermission::kTabCapture

Done.

http://codereview.chromium.org/11198044/diff/9001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:216: if ((it->first !=
content::MEDIA_TAB_VIDEO_CAPTURE &&
On 2012/10/18 08:36:17, xians1 wrote:
> ditto
Same

[Aaron Boodman, 2012-10-18 22:14:09]

http://codereview.chromium.org/11198044/diff/8002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/11198044/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:643: void
ExtensionHost::RequestMediaAccessPermission(
This code looks like a dupe of the one in shell_host.cc. Can it be pushed into
some common location?

Also, the same will need to be implemented in other places that host
WebContents, such as TabContents (or more likely extensions::TabHelper).

[justinlin, 2012-10-19 02:22:54]

http://codereview.chromium.org/11198044/diff/8002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/11198044/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_host.cc:643: void
ExtensionHost::RequestMediaAccessPermission(
On 2012/10/18 22:14:09, Aaron Boodman wrote:
> This code looks like a dupe of the one in shell_host.cc. Can it be pushed into
> some common location?
> 
> Also, the same will need to be implemented in other places that host
> WebContents, such as TabContents (or more likely extensions::TabHelper).

Done the refactor.

Are those implementations needed? It seems that for everything else except
extensions and package apps, it ends up in the implementation in browser.cc.

[no longer working on chromium, 2012-10-19 14:40:53]

http://codereview.chromium.org/11198044/diff/8002/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/11198044/diff/8002/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:78:
DCHECK(!tab_audio->second.empty() && !tab_video->second.empty());
It looks like you also changed from || to &&, is it intentional?

http://codereview.chromium.org/11198044/diff/11002/chrome/common/extensions/r...
File chrome/common/extensions/request_media_access_permission_helper.cc (right):

http://codereview.chromium.org/11198044/diff/11002/chrome/common/extensions/r...
chrome/common/extensions/request_media_access_permission_helper.cc:11: const
content::MediaStreamRequest* request,
Are you sure the request won't be deleted before it quits the function?

http://codereview.chromium.org/11198044/diff/11002/chrome/common/extensions/r...
chrome/common/extensions/request_media_access_permission_helper.cc:15: bool
allow_tab_media) {
why do you need an extra allow_tab_media? reuse the allow_audio should be enough
for you.

[justinlin, 2012-10-19 18:02:01]

http://codereview.chromium.org/11198044/diff/8002/chrome/browser/media/media_...
File chrome/browser/media/media_stream_devices_controller.cc (right):

http://codereview.chromium.org/11198044/diff/8002/chrome/browser/media/media_...
chrome/browser/media/media_stream_devices_controller.cc:78:
DCHECK(!tab_audio->second.empty() && !tab_video->second.empty());
On 2012/10/19 14:40:53, xians1 wrote:
> It looks like you also changed from || to &&, is it intentional?

No. Done. Sorry.

http://codereview.chromium.org/11198044/diff/11002/chrome/common/extensions/r...
File chrome/common/extensions/request_media_access_permission_helper.cc (right):

http://codereview.chromium.org/11198044/diff/11002/chrome/common/extensions/r...
chrome/common/extensions/request_media_access_permission_helper.cc:11: const
content::MediaStreamRequest* request,
On 2012/10/19 14:40:54, xians1 wrote:
> Are you sure the request won't be deleted before it quits the function?

Do you mean I should make a copy? I don't think I see any problem with it
getting deleted since none of the calling functions or original code expect it
to be deleted during authorization.

http://codereview.chromium.org/11198044/diff/11002/chrome/common/extensions/r...
chrome/common/extensions/request_media_access_permission_helper.cc:15: bool
allow_tab_media) {
On 2012/10/19 14:40:54, xians1 wrote:
> why do you need an extra allow_tab_media? reuse the allow_audio should be
enough
> for you.

Trying to preserve the existing functionality. In shell_window.cc (platform
apps), they require kAudioCapture and kVideoCapture permissions to access webcam
and mic (in extensions we didn't require that). For tab audio/video, we require
the kTabCapture permission for both platform_apps/extensions. I don't think we
need separate tab_audio/tab_video permissions because it's really always the
same "device".

Also, on second thought, since I didn't work before to get webcam/mic from an
extension, we should probably just keep it disabled. Made that change.

[wjia(left Chromium), 2012-10-21 05:40:19]

last comment from me.

https://codereview.chromium.org/11198044/diff/19003/chrome/browser/media/medi...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/11198044/diff/19003/chrome/browser/media/medi...
chrome/browser/media/media_stream_devices_controller.cc:88: video_device_id :
audio_device_id)) {
Do you need to verify case when both video_device_id and audio_device_id are
non-empty?

[justinlin, 2012-10-21 05:49:27]

Thanks!

https://codereview.chromium.org/11198044/diff/19003/chrome/browser/media/medi...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/11198044/diff/19003/chrome/browser/media/medi...
chrome/browser/media/media_stream_devices_controller.cc:88: video_device_id :
audio_device_id)) {
On 2012/10/21 05:40:19, wjia wrote:
> Do you need to verify case when both video_device_id and audio_device_id are
> non-empty?

They shouldn't both be empty at this point for tab media. We make sure of that
along the way. It's probably a development bug if they are, which is why I put a
DCHECK above, in case. The TabCapture API in (chrome/extensions) is the first
thing to check this. If it's somehow empty, then we would end up denying the
request, so that's OK.

[justinlin, 2012-10-22 03:27:48]

Sorry, I misread your last comment. They are always the same with the current
tab capture API (the API only lets you specify one tab for audio/video). We do
not allow specifying different devices for tab audio/video in the same request.
In the future, we may allow specifying different tabs for audio/video if we
decide to change the API.

On 2012/10/21 05:49:27, justinlin wrote:
> Thanks!
> 
>
https://codereview.chromium.org/11198044/diff/19003/chrome/browser/media/medi...
> File chrome/browser/media/media_stream_devices_controller.cc (right):
> 
>
https://codereview.chromium.org/11198044/diff/19003/chrome/browser/media/medi...
> chrome/browser/media/media_stream_devices_controller.cc:88: video_device_id :
> audio_device_id)) {
> On 2012/10/21 05:40:19, wjia wrote:
> > Do you need to verify case when both video_device_id and audio_device_id are
> > non-empty?
> 
> They shouldn't both be empty at this point for tab media. We make sure of that
> along the way. It's probably a development bug if they are, which is why I put
a
> DCHECK above, in case. The TabCapture API in (chrome/extensions) is the first
> thing to check this. If it's somehow empty, then we would end up denying the
> request, so that's OK.

[wjia(left Chromium), 2012-10-22 18:40:16]

lgtm on media stream stuff.

[Aaron Boodman, 2012-10-24 01:29:38]

http://codereview.chromium.org/11198044/diff/23001/chrome/common/extensions/r...
File chrome/common/extensions/request_media_access_permission_helper.cc (right):

http://codereview.chromium.org/11198044/diff/23001/chrome/common/extensions/r...
chrome/common/extensions/request_media_access_permission_helper.cc:13: bool
allow_audio,
I think it would be better to pass in Extension* instead of these three
booleans. Important to get the security checks right, would be easy to have a
paste-o and pass the wrong values here.

http://codereview.chromium.org/11198044/diff/19003/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/11198044/diff/19003/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:14: #include
"chrome/common/extensions/request_media_access_permission_helper.h"
Include doesn't seem necessary in this file.

[justinlin, 2012-10-24 02:04:54]

Thanks!, addressed comments. Please take another look.

http://codereview.chromium.org/11198044/diff/23001/chrome/common/extensions/r...
File chrome/common/extensions/request_media_access_permission_helper.cc (right):

http://codereview.chromium.org/11198044/diff/23001/chrome/common/extensions/r...
chrome/common/extensions/request_media_access_permission_helper.cc:13: bool
allow_audio,
On 2012/10/24 01:29:38, Aaron Boodman wrote:
> I think it would be better to pass in Extension* instead of these three
> booleans. Important to get the security checks right, would be easy to have a
> paste-o and pass the wrong values here.

Done.

http://codereview.chromium.org/11198044/diff/19003/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/11198044/diff/19003/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:14: #include
"chrome/common/extensions/request_media_access_permission_helper.h"
On 2012/10/24 01:29:38, Aaron Boodman wrote:
> Include doesn't seem necessary in this file.

Done. Moved to cc.

[Aaron Boodman, 2012-10-24 02:07:59]

lgtm

[justinlin, 2012-10-24 02:12:27]

On 2012/10/24 02:07:59, Aaron Boodman wrote:
> lgtm

jam: ptal the gypi changes

[no longer working on chromium, 2012-10-24 08:01:56]

lgtm too

http://codereview.chromium.org/11198044/diff/35001/chrome/common/extensions/r...
File chrome/common/extensions/request_media_access_permission_helper.cc (right):

http://codereview.chromium.org/11198044/diff/35001/chrome/common/extensions/r...
chrome/common/extensions/request_media_access_permission_helper.cc:15: const
extensions::Extension* extension,
much nicer now. +1

[jam, 2012-10-24 16:40:39]

On 2012/10/24 02:12:27, justinlin wrote:
> On 2012/10/24 02:07:59, Aaron Boodman wrote:
> > lgtm
> 
> jam: ptal the gypi changes

please see chrome/OWNERS

[justinlin, 2012-10-24 16:53:04]

sky@: please take a look at the gypi changes, thanks!

On 2012/10/24 16:40:39, John Abd-El-Malek wrote:
> On 2012/10/24 02:12:27, justinlin wrote:
> > On 2012/10/24 02:07:59, Aaron Boodman wrote:
> > > lgtm
> > 
> > jam: ptal the gypi changes
> 
> please see chrome/OWNERS

[sky, 2012-10-24 20:50:02]

LGTM