Add PermissionManager::GetPermissionStatusForFrame function

Add PermissionManager::GetPermissionStatusForFrame function

This adds a function to PermissionManager which allows checking the
permission for a particular RenderFrameHost. Some permission checks
depend on the exact context of the requesting frame. For example:
-For permission delegation we need to know the FeaturePolicy associated
with the frame, which is attached to the RenderFrameHost
-Some ChromeOS media-related permission checks require the
RenderFrameHost. These currently live in media_permissions.cc but it
would be good to move them into the PermissionContext subclass.
-Android permission checks currently require the WebContents in order to
determine their state.

A frame isn't always available when we want to determine the status of a
permission, so when it's not available the old version of the function
which takes in origins can be used. GetPermissionStatusForFrame should
always perform more checks than GetPermissionStatus and tend to be used
wherever possible.

The RenderFrameHost is passed down to the
PermissionContextBase::GetPermissionStatus. At this internal level of
the code, its value may be nullptr if the call does not originate from a
frame context. Subclasses should always check the RenderFrameHost before
using it.

TBR=peter@chromium.org,tommycli@chromium.org,xhwang@chromium.org,michaeln@chromium.org
BUG=596786
Review-Url: https://codereview.chromium.org/2723983004
Cr-Commit-Position: refs/heads/master@{#455621}
Committed: https://chromium.googlesource.com/chromium/src/+/f6104d499c9b96a1309360c378edfb24ad3d0028

[raymes, 2017-03-02 05:54:08]

raymes@chromium.org changed reviewers:
+ benwells@chromium.org, dominickn@chromium.org, timloh@chromium.org

[dominickn, 2017-03-02 06:54:33]

Looks pretty good to me. Thanks raymes!

https://codereview.chromium.org/2723983004/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.h (right):

https://codereview.chromium.org/2723983004/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.h:129: PermissionResult
GetPermissionStatusHelper(
This is a little strangely named (I expect a Helper to be a class). Not really
sure what a better name here is though.

[benwells, 2017-03-02 07:33:05]

lgtm

https://codereview.chromium.org/2723983004/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.h (right):

https://codereview.chromium.org/2723983004/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.h:129: PermissionResult
GetPermissionStatusHelper(
On 2017/03/02 06:54:33, dominickn wrote:
> This is a little strangely named (I expect a Helper to be a class). Not really
> sure what a better name here is though.

Maybe GetPermissonStatusInternal?

[raymes, 2017-03-07 02:31:05]

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

[raymes, 2017-03-07 02:31:15]

https://codereview.chromium.org/2723983004/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_manager.h (right):

https://codereview.chromium.org/2723983004/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_manager.h:129: PermissionResult
GetPermissionStatusHelper(
On 2017/03/02 07:33:05, benwells wrote:
> On 2017/03/02 06:54:33, dominickn wrote:
> > This is a little strangely named (I expect a Helper to be a class). Not
really
> > sure what a better name here is though.
> 
> Maybe GetPermissonStatusInternal?

I'd like to avoid GetPermissonStatusInternal because we use it in a different
way in PermissionContextBase and I think having a different name will lead to
less confusion.

I think a helper function is a reasonably well understood term. For example it
looks like Bjarne Stroustrup uses the term in his books. If you think of a
better name though I'm happy to change it :)

[commit-bot: I haz the power, 2017-03-07 02:31:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[raymes, 2017-03-07 05:27:42]

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

[commit-bot: I haz the power, 2017-03-07 05:28:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[raymes, 2017-03-07 05:29:10]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2017-03-07 05:29:14]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-03-07 05:29:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2723983004/#ps40001
(title: "Add PermissionManager::GetPermissionStatusForFrame function")

[commit-bot: I haz the power, 2017-03-07 05:29:55]

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-03-07 05:39:31]

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

[commit-bot: I haz the power, 2017-03-07 05:39:32]

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...)

[raymes, 2017-03-07 06:07:55]

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

[commit-bot: I haz the power, 2017-03-07 06:08:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[raymes, 2017-03-07 06:11:44]

Description was changed from

==========
Add PermissionManager::GetPermissionStatusForFrame function

This adds a function to PermissionManager which allows checking the
permission for a particular RenderFrameHost. Some permission checks
depend on the exact context of the requesting frame. For example:
-For permission delegation we need to know the FeaturePolicy associated
with the frame, which is attached to the RenderFrameHost
-Some ChromeOS media-related permission checks require the
RenderFrameHost. These currently live in media_permissions.cc but it
would be good to move them into the PermissionContext subclass.
-Android permission checks currently require the WebContents in order to
determine their state.

A frame isn't always available when we want to determine the status of a
permission, so when it's not available the old version of the function
which takes in origins can be used. GetPermissionStatusForFrame should
always perform more checks than GetPermissionStatus and tend to be used
wherever possible.

The RenderFrameHost is passed down to the
PermissionContextBase::GetPermissionStatus. At this internal level of
the code, its value may be nullptr if the call does not originate from a
frame context. Subclasses should always check the RenderFrameHost before
using it.

BUG=596786
==========

to

==========
Add PermissionManager::GetPermissionStatusForFrame function

This adds a function to PermissionManager which allows checking the
permission for a particular RenderFrameHost. Some permission checks
depend on the exact context of the requesting frame. For example:
-For permission delegation we need to know the FeaturePolicy associated
with the frame, which is attached to the RenderFrameHost
-Some ChromeOS media-related permission checks require the
RenderFrameHost. These currently live in media_permissions.cc but it
would be good to move them into the PermissionContext subclass.
-Android permission checks currently require the WebContents in order to
determine their state.

A frame isn't always available when we want to determine the status of a
permission, so when it's not available the old version of the function
which takes in origins can be used. GetPermissionStatusForFrame should
always perform more checks than GetPermissionStatus and tend to be used
wherever possible.

The RenderFrameHost is passed down to the
PermissionContextBase::GetPermissionStatus. At this internal level of
the code, its value may be nullptr if the call does not originate from a
frame context. Subclasses should always check the RenderFrameHost before
using it.

TBR=peter@chromium.org,tommycli@chromium.org,xhwang@chromium.org,michaeln@chr...
BUG=596786
==========

[raymes, 2017-03-07 06:11:44]

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

[raymes, 2017-03-07 06:12:14]

TBR OWNERs of client code:

peter:
  chrome/browser/notifications/notification_permission_context.cc
  chrome/browser/notifications/notification_permission_context.h
  chrome/browser/notifications/notification_permission_context_unittest.cc
tommycli:
  chrome/browser/plugins/flash_permission_context.cc
  chrome/browser/plugins/flash_permission_context.h
xhwang:
  chrome/browser/media/midi_permission_context_unittest.cc
  chrome/browser/media/protected_media_identifier_permission_context.cc
  chrome/browser/media/protected_media_identifier_permission_context.h
michaeln:
  chrome/browser/storage/durable_storage_permission_context.cc
  chrome/browser/storage/durable_storage_permission_context_unittest.cc

[xhwang, 2017-03-07 06:28:16]

xhwang@chromium.org changed reviewers:
+ dcheng@chromium.org

[xhwang, 2017-03-07 06:28:17]

A few comments for discussion:

- MediaPermission exists because media/ doesn't depend on content/. So it's a
media interface for getting permission status, which is implemented in content
layer.

- Shall we pass in the specific FeaturePolicy or whatever we need to determine
the permission status, instead of passing in the RenderFrameHost, which is
pretty much everything? :)

- Not strictly related to this CL. For permission requests coming from a frame,
we should enforce that the |requesting_origin| is consistent with the origin of
the RFH. But I think that should be checked very early on, at *PermissionContext
I feel it's already too late. I talked with +dcheng about this and it seemed to
us this is a security issue since we cannot trust the |requesting_origin| passed
from the render process.

[xhwang, 2017-03-07 06:32:12]

On 2017/03/07 06:28:17, xhwang_slow wrote:
> A few comments for discussion:
> 
> - MediaPermission exists because media/ doesn't depend on content/. So it's a
> media interface for getting permission status, which is implemented in content
> layer.

Oh you might be talking about chrome/browser/media/webrtc/media_permission.h,
and I was thinking about media/base/media_permission.h... I don't really know
how the former works.
 
> - Shall we pass in the specific FeaturePolicy or whatever we need to determine
> the permission status, instead of passing in the RenderFrameHost, which is
> pretty much everything? :)
> 
> - Not strictly related to this CL. For permission requests coming from a
frame,
> we should enforce that the |requesting_origin| is consistent with the origin
of
> the RFH. But I think that should be checked very early on, at
*PermissionContext
> I feel it's already too late. I talked with +dcheng about this and it seemed
to
> us this is a security issue since we cannot trust the |requesting_origin|
passed
> from the render process.

[commit-bot: I haz the power, 2017-03-07 08:24:17]

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

[commit-bot: I haz the power, 2017-03-07 08:24:18]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2017-03-07 08:25:00]

I think this is a step in the right direction. Is there a plan for things that
don't have a frame context? Is it things like workers, etc that don't have the
frame context?

[raymes, 2017-03-08 04:20:39]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-03-08 04:20:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2723983004/#ps60001
(title: "Add PermissionManager::GetPermissionStatusForFrame function")

[raymes, 2017-03-08 04:20:44]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2017-03-08 04:27:33]

On 2017/03/07 06:28:17, xhwang_slow wrote:
> A few comments for discussion:
> 
> - MediaPermission exists because media/ doesn't depend on content/. So it's a
> media interface for getting permission status, which is implemented in content
> layer.
> 

As you noted we're looking at the chrome/ version here so that shouldn't be a
problem :)

> - Shall we pass in the specific FeaturePolicy or whatever we need to determine
> the permission status, instead of passing in the RenderFrameHost, which is
> pretty much everything? :)

There are many things related to the RenderFrameHost/WebContents that we use to
determine the permission. So I think it's safer/more extendable to pass the
RenderFrameHost.

> - Not strictly related to this CL. For permission requests coming from a
frame,
> we should enforce that the |requesting_origin| is consistent with the origin
of
> the RFH. But I think that should be checked very early on, at
*PermissionContext
> I feel it's already too late. I talked with +dcheng about this and it seemed
to
> us this is a security issue since we cannot trust the |requesting_origin|
passed
> from the render process.


I agree - I'm going to be looking at some of these callsites. See
https://bugs.chromium.org/p/chromium/issues/detail?id=698985 to follow along.


On 2017/03/07 08:25:00, dcheng wrote:
> I think this is a step in the right direction. Is there a plan for things that
> don't have a frame context? Is it things like workers, etc that don't have the
> frame context?

We don't have a proper plan for workers yet - they will still pass an origin.
Thankfully they can't really use many things that require permission grants yet,
so it's somewhat mitigated.

[raymes, 2017-03-08 04:27:40]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-03-08 04:27:53]

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-03-08 04:30:38]

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

[commit-bot: I haz the power, 2017-03-08 04:30:39]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[raymes, 2017-03-09 00:16:18]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-03-09 00:16:49]

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-03-09 01:20:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489018578009720,
"parent_rev": "90854585bebf660fbf80db77c8a9712eeba42968", "commit_rev":
"f6104d499c9b96a1309360c378edfb24ad3d0028"}

[commit-bot: I haz the power, 2017-03-09 01:22:14]

Description was changed from

==========
Add PermissionManager::GetPermissionStatusForFrame function

This adds a function to PermissionManager which allows checking the
permission for a particular RenderFrameHost. Some permission checks
depend on the exact context of the requesting frame. For example:
-For permission delegation we need to know the FeaturePolicy associated
with the frame, which is attached to the RenderFrameHost
-Some ChromeOS media-related permission checks require the
RenderFrameHost. These currently live in media_permissions.cc but it
would be good to move them into the PermissionContext subclass.
-Android permission checks currently require the WebContents in order to
determine their state.

A frame isn't always available when we want to determine the status of a
permission, so when it's not available the old version of the function
which takes in origins can be used. GetPermissionStatusForFrame should
always perform more checks than GetPermissionStatus and tend to be used
wherever possible.

The RenderFrameHost is passed down to the
PermissionContextBase::GetPermissionStatus. At this internal level of
the code, its value may be nullptr if the call does not originate from a
frame context. Subclasses should always check the RenderFrameHost before
using it.

TBR=peter@chromium.org,tommycli@chromium.org,xhwang@chromium.org,michaeln@chr...
BUG=596786
==========

to

==========
Add PermissionManager::GetPermissionStatusForFrame function

This adds a function to PermissionManager which allows checking the
permission for a particular RenderFrameHost. Some permission checks
depend on the exact context of the requesting frame. For example:
-For permission delegation we need to know the FeaturePolicy associated
with the frame, which is attached to the RenderFrameHost
-Some ChromeOS media-related permission checks require the
RenderFrameHost. These currently live in media_permissions.cc but it
would be good to move them into the PermissionContext subclass.
-Android permission checks currently require the WebContents in order to
determine their state.

A frame isn't always available when we want to determine the status of a
permission, so when it's not available the old version of the function
which takes in origins can be used. GetPermissionStatusForFrame should
always perform more checks than GetPermissionStatus and tend to be used
wherever possible.

The RenderFrameHost is passed down to the
PermissionContextBase::GetPermissionStatus. At this internal level of
the code, its value may be nullptr if the call does not originate from a
frame context. Subclasses should always check the RenderFrameHost before
using it.

TBR=peter@chromium.org,tommycli@chromium.org,xhwang@chromium.org,michaeln@chr...
BUG=596786

Review-Url: https://codereview.chromium.org/2723983004
Cr-Commit-Position: refs/heads/master@{#455621}
Committed:
https://chromium.googlesource.com/chromium/src/+/f6104d499c9b96a1309360c378ed...
==========

[commit-bot: I haz the power, 2017-03-09 01:22:15]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f6104d499c9b96a1309360c378ed...