Adds an AccessPolicy that is queried to determine what a connection can do

Adds an AccessPolicy that is queried to determine what a connection can do

As a result of this I uncovered some things we shouldn't have been
doing. I had to update a number of tests accordingly.

BUG=396804
TEST=covered by test
R=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285760

[sky, 2014-07-25 20:18:58]

I've largely left the capabilities we grant to the window manager intact. I
suspect they are too permissive, but that can done later.

There is still more cleanup to be done, but this is a good point.

[Ben Goodger (Google), 2014-07-25 20:51:46]

There should also be a TODO that explains why each aspect of the window
manager's policy differs from the default.

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/access_policy_delegate.h (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/access_policy_delegate.h:29: virtual bool
IsNodeEmbeddedInAnotherConnectionForAccessPolicy(
Hrm... I think you could come up with a better name for this. This name implies
someone else contains this node. Really you're saying someone else is contained
by this node.

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/default_access_policy.cc (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/default_access_policy.cc:51: return
view->id().connection_id == connection_id_;
I would distill this pattern to an "IdOwnedByThisConnection" call & use for both
views & nodes.

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/default_access_policy.h (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/default_access_policy.h:48: bool
IsNodeFromThisConnection(const Node* node) const;
"From" feels weak here. "Owned" or "CreatedBy" would be stronger.

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/default_access_policy.h:49: 
Aside, is it the case that the only nodes visible to a connection now are:

- nodes it created itself
- its roots

?

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:34: // TODO: change
this!
You can now go ahead and cite http://crbug.com/397660

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.h (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.h:204: // InterfaceImp:
InterfaceImpl

[sky, 2014-07-25 21:10:46]

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/access_policy_delegate.h (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/access_policy_delegate.h:29: virtual bool
IsNodeEmbeddedInAnotherConnectionForAccessPolicy(
On 2014/07/25 20:51:46, Ben Goodger (Google) wrote:
> Hrm... I think you could come up with a better name for this. This name
implies
> someone else contains this node. Really you're saying someone else is
contained
> by this node.

Actually, I'm saying something slightly different. Which manes the names is
doubly bad. I went with IsNodeRootOfAnotherConnectionForAccessPolicy.

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/default_access_policy.cc (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/default_access_policy.cc:51: return
view->id().connection_id == connection_id_;
On 2014/07/25 20:51:46, Ben Goodger (Google) wrote:
> I would distill this pattern to an "IdOwnedByThisConnection" call & use for
both
> views & nodes.

I went with a template (ick!) so that I can keep the type safety (yay!).

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/default_access_policy.h (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/default_access_policy.h:48: bool
IsNodeFromThisConnection(const Node* node) const;
On 2014/07/25 20:51:46, Ben Goodger (Google) wrote:
> "From" feels weak here. "Owned" or "CreatedBy" would be stronger.

Done.

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/default_access_policy.h:49: 
On 2014/07/25 20:51:46, Ben Goodger (Google) wrote:
> Aside, is it the case that the only nodes visible to a connection now are:
> 
> - nodes it created itself
> - its roots
> 
> ?

For everyone but the window manager, yes.

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:34: // TODO: change
this!
On 2014/07/25 20:51:46, Ben Goodger (Google) wrote:
> You can now go ahead and cite http://crbug.com/397660

Done.

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.h (right):

https://codereview.chromium.org/421693002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.h:204: // InterfaceImp:
On 2014/07/25 20:51:46, Ben Goodger (Google) wrote:
> InterfaceImpl

Done.

[Ben Goodger (Google), 2014-07-25 21:56:32]

lgtm

https://codereview.chromium.org/421693002/diff/40001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/421693002/diff/40001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:655:
ViewManagerServiceImpl::IsNodeKnownForAccessPolicy(const Node* node) const {
nit: linebreak after ( instead of the return type.

[sky, 2014-07-25 22:00:13]

https://codereview.chromium.org/421693002/diff/40001/mojo/services/view_manag...
File mojo/services/view_manager/view_manager_service_impl.cc (right):

https://codereview.chromium.org/421693002/diff/40001/mojo/services/view_manag...
mojo/services/view_manager/view_manager_service_impl.cc:655:
ViewManagerServiceImpl::IsNodeKnownForAccessPolicy(const Node* node) const {
On 2014/07/25 21:56:31, Ben Goodger (Google) wrote:
> nit: linebreak after ( instead of the return type.

Done.

[sky, 2014-07-25 22:00:44]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-07-25 22:04:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sky@chromium.org/421693002/60001

[sky, 2014-07-26 14:52:42]

Committed patchset #4 manually as r285760 (presubmit successful).

[Ben Goodger (Google), 2014-07-26 16:23:29]

So did you get all the tests? Can I revive my CL now?

-Ben


On Sat, Jul 26, 2014 at 7:52 AM, <sky@chromium.org> wrote:

> Committed patchset #4 manually as r285760 (presubmit successful).
>
> https://codereview.chromium.org/421693002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-07-26 19:44:07]

You'll need my other to patch land first. I'll update it now.

On Sat, Jul 26, 2014 at 9:23 AM, Ben Goodger (Google) <ben@chromium.org> wrote:
> So did you get all the tests? Can I revive my CL now?
>
> -Ben
>
>
> On Sat, Jul 26, 2014 at 7:52 AM, <sky@chromium.org> wrote:
>>
>> Committed patchset #4 manually as r285760 (presubmit successful).
>>
>> https://codereview.chromium.org/421693002/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.