Implement feature policy checks in the browser process

Implement permissions feature policy checks in the browser process

This adds feature policy checks for Geolocation, Midi/sysex, Protected media,
and Mic/Camera to the browser process. These checks are also implemented in
the content layer in the Permission Service
(see https://codereview.chromium.org/2874053003/). The reason to have these
checks implemented in the content/ layer is because they are standardized
checks and we want to ensure that other embedders of content implement the same
behavior.

However there are 2 pitfalls to this:
1) The results returned from the permission service are used to make decisions
in the renderer process and so are not trustworthy. Because of this, most
features that have permission will add additional checks in the browser
process.
2) The permission checks that happen in the browser process most often happen
in the chrome/ layer (through the PermissionManager) so this is also where
feature policy checks need to happen.

In the long term, both chrome/ and content/ should make permission checks
through the same API/mojo service where common permission checks like this one
can live.

BUG=689802
TBR=tommycli@chromium.org,peter@chromium.org

Review-Url: https://codereview.chromium.org/2898663002
Cr-Commit-Position: refs/heads/master@{#475826}
Committed: https://chromium.googlesource.com/chromium/src/+/21b9affc7f019d878b3118c2c0833822d78dfec4

[raymes, 2017-05-23 01:34:49]

Description was changed from

==========
Implement feature policy checks in the browser process

BUG=
==========

to

==========
Implement permissions feature policy checks in the browser process

This adds feature policy checks for Geolocation, Midi/sysex, Protected media,
and Mic/Camera to the browser process. These checks are also implemented in
the content layer in the Permission Service
(see https://codereview.chromium.org/2874053003/). The reason to have these
checks implemented in the content/ layer is because they are standardized
checks and we want to ensure that other embedders of content implement the same
behavior.

However there are 2 pitfalls to this:
1) The results returned from the permission service are used to make decisions
in the renderer process and so are not trustworthy. Because of this, most
features that have permission will add additional checks in the browser
process.
2) The permission checks that happen in the browser process most often happen
in the chrome/ layer (through the PermissionManager) so this is also where
feature policy checks need to happen.

In the long term, both chrome/ and content/ should make permission checks
through the same API/mojo service where common permission checks like this one
can live.

BUG=https://codereview.chromium.org/2874053003/
==========

[raymes, 2017-05-23 01:36:46]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 01:37:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 02:12:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 02:12:58]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[raymes, 2017-05-24 03:32:51]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[raymes, 2017-05-24 03:33:24]

raymes@chromium.org changed reviewers:
+ timloh@chromium.org

[commit-bot: I haz the power, 2017-05-24 03:33:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 03:51:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 03:51:53]

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_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[raymes, 2017-05-24 03:55:11]

Description was changed from

==========
Implement permissions feature policy checks in the browser process

This adds feature policy checks for Geolocation, Midi/sysex, Protected media,
and Mic/Camera to the browser process. These checks are also implemented in
the content layer in the Permission Service
(see https://codereview.chromium.org/2874053003/). The reason to have these
checks implemented in the content/ layer is because they are standardized
checks and we want to ensure that other embedders of content implement the same
behavior.

However there are 2 pitfalls to this:
1) The results returned from the permission service are used to make decisions
in the renderer process and so are not trustworthy. Because of this, most
features that have permission will add additional checks in the browser
process.
2) The permission checks that happen in the browser process most often happen
in the chrome/ layer (through the PermissionManager) so this is also where
feature policy checks need to happen.

In the long term, both chrome/ and content/ should make permission checks
through the same API/mojo service where common permission checks like this one
can live.

BUG=https://codereview.chromium.org/2874053003/
==========

to

==========
Implement permissions feature policy checks in the browser process

This adds feature policy checks for Geolocation, Midi/sysex, Protected media,
and Mic/Camera to the browser process. These checks are also implemented in
the content layer in the Permission Service
(see https://codereview.chromium.org/2874053003/). The reason to have these
checks implemented in the content/ layer is because they are standardized
checks and we want to ensure that other embedders of content implement the same
behavior.

However there are 2 pitfalls to this:
1) The results returned from the permission service are used to make decisions
in the renderer process and so are not trustworthy. Because of this, most
features that have permission will add additional checks in the browser
process.
2) The permission checks that happen in the browser process most often happen
in the chrome/ layer (through the PermissionManager) so this is also where
feature policy checks need to happen.

In the long term, both chrome/ and content/ should make permission checks
through the same API/mojo service where common permission checks like this one
can live.

BUG=689802
==========

[raymes, 2017-05-24 03:55:24]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[raymes, 2017-05-24 03:56:19]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 03:56:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 05:57:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 05:57:42]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-05-26 06:30:33]

On 2017/05/24 03:33:25, raymes wrote:

lgtm

[raymes, 2017-05-29 00:31:53]

raymes@chromium.org changed reviewers:
+ alexmos@chromium.org

[raymes, 2017-05-29 00:31:53]

+alexmos: could you ptal at the changes in content/ to make the tests possible?
Thanks!

[raymes, 2017-05-29 00:41:09]

raymes@chromium.org changed reviewers:
+ iclelland@chromium.org, michaeln@chromium.org, peter@chromium.org,
sergeyu@chromium.org, tommycli@chromium.org

[raymes, 2017-05-29 00:41:10]

TBR OWNERS of client code

michaeln:
  chrome/browser/storage/durable_storage_permission_context.cc
tommycli:
  chrome/browser/plugins/flash_permission_context.cc
sergeyu:
  chrome/browser/media/midi_permission_context.cc
  chrome/browser/media/midi_sysex_permission_context.cc
  chrome/browser/media/protected_media_identifier_permission_context.cc
peter:
  chrome/browser/notifications/notification_permission_context.cc
alexmos:
  content/public/common/content_features.cc
  content/public/common/content_features.h
  content/public/test/test_renderer_host.h
  content/test/test_render_frame_host.cc
  content/test/test_render_frame_host.h
iclelland:
  chrome/browser/background_sync/background_sync_permission_context.cc

[raymes, 2017-05-29 00:43:12]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 00:43:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 01:54:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-29 01:54:37]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-05-30 15:09:02]

On 2017/05/29 00:41:10, raymes wrote:
> TBR OWNERS of client code

> iclelland:
>   chrome/browser/background_sync/background_sync_permission_context.cc

BackgroundSync lgtm

[Sergey Ulanov, 2017-05-30 18:21:07]

chrome/browser/media lgtm

https://codereview.chromium.org/2898663002/diff/140001/chrome/browser/media/w...
File chrome/browser/media/webrtc/media_stream_device_permission_context.cc
(right):

https://codereview.chromium.org/2898663002/diff/140001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_stream_device_permission_context.cc:23: } else
{
No else after return, please
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

Or alternatively use switch statement here

[alexmos, 2017-05-30 20:37:57]

content/ LGTM

https://codereview.chromium.org/2898663002/diff/140001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2898663002/diff/140001/content/public/test/te...
content/public/test/test_renderer_host.h:147: virtual void
SetFeaturePolicyHeader(
nit: SimulateFeaturePolicyHeader might be more consistent with other function
names here.

[michaeln, 2017-05-30 22:49:00]

r/s lgtm

[raymes, 2017-05-31 04:40:15]

Description was changed from

==========
Implement permissions feature policy checks in the browser process

This adds feature policy checks for Geolocation, Midi/sysex, Protected media,
and Mic/Camera to the browser process. These checks are also implemented in
the content layer in the Permission Service
(see https://codereview.chromium.org/2874053003/). The reason to have these
checks implemented in the content/ layer is because they are standardized
checks and we want to ensure that other embedders of content implement the same
behavior.

However there are 2 pitfalls to this:
1) The results returned from the permission service are used to make decisions
in the renderer process and so are not trustworthy. Because of this, most
features that have permission will add additional checks in the browser
process.
2) The permission checks that happen in the browser process most often happen
in the chrome/ layer (through the PermissionManager) so this is also where
feature policy checks need to happen.

In the long term, both chrome/ and content/ should make permission checks
through the same API/mojo service where common permission checks like this one
can live.

BUG=689802
==========

to

==========
Implement permissions feature policy checks in the browser process

This adds feature policy checks for Geolocation, Midi/sysex, Protected media,
and Mic/Camera to the browser process. These checks are also implemented in
the content layer in the Permission Service
(see https://codereview.chromium.org/2874053003/). The reason to have these
checks implemented in the content/ layer is because they are standardized
checks and we want to ensure that other embedders of content implement the same
behavior.

However there are 2 pitfalls to this:
1) The results returned from the permission service are used to make decisions
in the renderer process and so are not trustworthy. Because of this, most
features that have permission will add additional checks in the browser
process.
2) The permission checks that happen in the browser process most often happen
in the chrome/ layer (through the PermissionManager) so this is also where
feature policy checks need to happen.

In the long term, both chrome/ and content/ should make permission checks
through the same API/mojo service where common permission checks like this one
can live.

BUG=689802
TBR=tommycli@chromium.org,peter@chromium.org
==========

[raymes, 2017-05-31 04:46:21]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-05-31 04:46:22]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
michaeln@chromium.org, sergeyu@chromium.org, alexmos@chromium.org,
iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/2898663002/#ps160001
(title: "Implement feature policy checks in the browser process")

[commit-bot: I haz the power, 2017-05-31 04:46:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 06:15:28]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496205981667350,
"parent_rev": "5d983728fda05c6669287d858c82562e1090e9b2", "commit_rev":
"21b9affc7f019d878b3118c2c0833822d78dfec4"}

[commit-bot: I haz the power, 2017-05-31 06:15:39]

Description was changed from

==========
Implement permissions feature policy checks in the browser process

This adds feature policy checks for Geolocation, Midi/sysex, Protected media,
and Mic/Camera to the browser process. These checks are also implemented in
the content layer in the Permission Service
(see https://codereview.chromium.org/2874053003/). The reason to have these
checks implemented in the content/ layer is because they are standardized
checks and we want to ensure that other embedders of content implement the same
behavior.

However there are 2 pitfalls to this:
1) The results returned from the permission service are used to make decisions
in the renderer process and so are not trustworthy. Because of this, most
features that have permission will add additional checks in the browser
process.
2) The permission checks that happen in the browser process most often happen
in the chrome/ layer (through the PermissionManager) so this is also where
feature policy checks need to happen.

In the long term, both chrome/ and content/ should make permission checks
through the same API/mojo service where common permission checks like this one
can live.

BUG=689802
TBR=tommycli@chromium.org,peter@chromium.org
==========

to

==========
Implement permissions feature policy checks in the browser process

This adds feature policy checks for Geolocation, Midi/sysex, Protected media,
and Mic/Camera to the browser process. These checks are also implemented in
the content layer in the Permission Service
(see https://codereview.chromium.org/2874053003/). The reason to have these
checks implemented in the content/ layer is because they are standardized
checks and we want to ensure that other embedders of content implement the same
behavior.

However there are 2 pitfalls to this:
1) The results returned from the permission service are used to make decisions
in the renderer process and so are not trustworthy. Because of this, most
features that have permission will add additional checks in the browser
process.
2) The permission checks that happen in the browser process most often happen
in the chrome/ layer (through the PermissionManager) so this is also where
feature policy checks need to happen.

In the long term, both chrome/ and content/ should make permission checks
through the same API/mojo service where common permission checks like this one
can live.

BUG=689802
TBR=tommycli@chromium.org,peter@chromium.org

Review-Url: https://codereview.chromium.org/2898663002
Cr-Commit-Position: refs/heads/master@{#475826}
Committed:
https://chromium.googlesource.com/chromium/src/+/21b9affc7f019d878b3118c2c083...
==========

[commit-bot: I haz the power, 2017-05-31 06:15:41]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/21b9affc7f019d878b3118c2c083...

[raymes, 2017-05-31 07:07:54]

https://codereview.chromium.org/2898663002/diff/140001/chrome/browser/media/w...
File chrome/browser/media/webrtc/media_stream_device_permission_context.cc
(right):

https://codereview.chromium.org/2898663002/diff/140001/chrome/browser/media/w...
chrome/browser/media/webrtc/media_stream_device_permission_context.cc:23: } else
{
On 2017/05/30 18:21:06, Sergey Ulanov wrote:
> No else after return, please
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> 
> Or alternatively use switch statement here

Done.

https://codereview.chromium.org/2898663002/diff/140001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2898663002/diff/140001/content/public/test/te...
content/public/test/test_renderer_host.h:147: virtual void
SetFeaturePolicyHeader(
On 2017/05/30 20:37:57, alexmos wrote:
> nit: SimulateFeaturePolicyHeader might be more consistent with other function
> names here.

Done.