Make CapabilityFilter be part of Identity

Make CapabilityFilter be part of Identity

R=yzshen@chromium.org
http://crbug.com/533085

Committed: https://crrev.com/5403e28c9d9bfcf00b12721b6dc96c00f6d1ed24
Cr-Commit-Position: refs/heads/master@{#349815}

Committed: https://crrev.com/3a72d2e4a0f3a6ffa8bc5bee5cd5af0f87b51090
Cr-Commit-Position: refs/heads/master@{#350241}

[yzshen1, 2015-09-18 06:07:54]

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
mojo/shell/application_instance.cc:126: // Unfortunately, it is possible that
|request->target_request()| is null
nit: |request->target_url_request()|

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
File mojo/shell/application_manager.cc (right):

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
mojo/shell/application_manager.cc:153:
params->SetTargetURLRequest(new_request.Pass());
This replaces the target filter with a permissive one.

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
File mojo/shell/connect_to_application_params.cc (right):

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
mojo/shell/connect_to_application_params.cc:31: void
ConnectToApplicationParams::SetTargetURL(const GURL& target_url) {
Related to my comment in identity.h: does it make sense to use reject-all as the
default filter?

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
mojo/shell/connect_to_application_params.cc:37: void
ConnectToApplicationParams::SetTargetURLRequest(URLRequestPtr request) {
It seems we have a few copies of target's contents: (part of) target_ -> target
-> target_. It would be nice to avoid some of those.

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
File mojo/shell/connect_to_application_params.h (right):

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
mojo/shell/connect_to_application_params.h:36: // Sets both |target_| and
|target_url_request_|.
nit: please consider removing this comment and changing line 33 to "The
following four methods set both ...".

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
mojo/shell/connect_to_application_params.h:82: // than |app_url_| (e.g.,
headers, request body). When it is taken, |app_url_|
Please update |app_ur_| related comments.

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/identity.h
File mojo/shell/identity.h (right):

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/identity.h#...
mojo/shell/identity.h:21: // If a CapabilityFilter is not specified via the
constructor, an instance of
I feel that it is less likely for the user to screw up if the default is a
reject-all filter. Since this is security-critical, I feel that it is nice to be
explicit if the user does want a permissive filter.

WDYT? Thanks!

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/identity.h#...
mojo/shell/identity.h:36: GURL url;
I think it is preferred to use either public members or getters/setters, but not
both together.

WDYT? Thanks!

[Ben Goodger (Google), 2015-09-18 14:03:10]

I'll look at the changes needed for a permissive filter when I get in.

The reason I flipped the default was because:
a) this API is internal (i.e. mostly used by mojo/runner & shell internals
and tests)
b) those sites almost exclusively pass a permissive filter.

-Ben

On Thu, Sep 17, 2015 at 11:07 PM, <yzshen@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
> File mojo/shell/application_instance.cc (right):
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
> mojo/shell/application_instance.cc:126: // Unfortunately, it is possible
> that |request->target_request()| is null
> nit: |request->target_url_request()|
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
> File mojo/shell/application_manager.cc (right):
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
> mojo/shell/application_manager.cc:153:
> params->SetTargetURLRequest(new_request.Pass());
> This replaces the target filter with a permissive one.
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
> File mojo/shell/connect_to_application_params.cc (right):
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
> mojo/shell/connect_to_application_params.cc:31: void
> ConnectToApplicationParams::SetTargetURL(const GURL& target_url) {
> Related to my comment in identity.h: does it make sense to use
> reject-all as the default filter?
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
> mojo/shell/connect_to_application_params.cc:37: void
> ConnectToApplicationParams::SetTargetURLRequest(URLRequestPtr request) {
> It seems we have a few copies of target's contents: (part of) target_ ->
> target -> target_. It would be nice to avoid some of those.
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
> File mojo/shell/connect_to_application_params.h (right):
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
> mojo/shell/connect_to_application_params.h:36: // Sets both |target_|
> and |target_url_request_|.
> nit: please consider removing this comment and changing line 33 to "The
> following four methods set both ...".
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
> mojo/shell/connect_to_application_params.h:82: // than |app_url_| (e.g.,
> headers, request body). When it is taken, |app_url_|
> Please update |app_ur_| related comments.
>
>
> https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/identity.h
> File mojo/shell/identity.h (right):
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/identity.h#...
> mojo/shell/identity.h:21: // If a CapabilityFilter is not specified via
> the constructor, an instance of
> I feel that it is less likely for the user to screw up if the default is
> a reject-all filter. Since this is security-critical, I feel that it is
> nice to be explicit if the user does want a permissive filter.
>
> WDYT? Thanks!
>
>
>
https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/identity.h#...
> mojo/shell/identity.h:36: GURL url;
> I think it is preferred to use either public members or getters/setters,
> but not both together.
>
> WDYT? Thanks!
>
> https://codereview.chromium.org/1354003002/
>

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

[yzshen1, 2015-09-18 15:44:09]

I found that I misread the SetTargetURL*() code. So please ignore the following
two comments.

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
File mojo/shell/application_manager.cc (right):

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/application...
mojo/shell/application_manager.cc:153:
params->SetTargetURLRequest(new_request.Pass());
On 2015/09/18 06:07:54, yzshen1 wrote:
> This replaces the target filter with a permissive one.

Please ignore this comment. I think I somehow misread the code. :)

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
File mojo/shell/connect_to_application_params.cc (right):

https://codereview.chromium.org/1354003002/diff/160001/mojo/shell/connect_to_...
mojo/shell/connect_to_application_params.cc:31: void
ConnectToApplicationParams::SetTargetURL(const GURL& target_url) {
On 2015/09/18 06:07:54, yzshen1 wrote:
> Related to my comment in identity.h: does it make sense to use reject-all as
the
> default filter?

Please ignore this comment. I somehow misread the code. Here target_.filter() is
used so it shouldn't be an issue.

[Ben Goodger (Google), 2015-09-18 21:31:53]

OK I updated this to address all comments.
I flipped the default to restrictive filter, it turned out the effect on the use
of the API was minimal as tests run through just a couple of chokepoints that
require a minimal filter so I was able to catch this there.

[yzshen1, 2015-09-18 21:44:04]

https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.cc
File mojo/shell/identity.cc (right):

https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.cc...
mojo/shell/identity.cc:34: qualifier_(GetBaseURLAndQuery(url, nullptr).spec())
{}
nit: it seems safe to use |url_| instead of |GetBaseURLAndQuery(url, nullptr)|
when initializing |qualifier_|. (and also line 38 and line 45)

https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.h
File mojo/shell/identity.h (right):

https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.h#...
mojo/shell/identity.h:22: // Identity is constructed with a permissive filter
(allowing connection to
Please update this comment.

[yzshen1, 2015-09-18 21:44:21]

On 2015/09/18 21:44:04, yzshen1 wrote:
> https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.cc
> File mojo/shell/identity.cc (right):
> 
>
https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.cc...
> mojo/shell/identity.cc:34: qualifier_(GetBaseURLAndQuery(url, nullptr).spec())
> {}
> nit: it seems safe to use |url_| instead of |GetBaseURLAndQuery(url, nullptr)|
> when initializing |qualifier_|. (and also line 38 and line 45)
> 
> https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.h
> File mojo/shell/identity.h (right):
> 
>
https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.h#...
> mojo/shell/identity.h:22: // Identity is constructed with a permissive filter
> (allowing connection to
> Please update this comment.

LGTM with the two nits above.

[Ben Goodger (Google), 2015-09-18 21:47:55]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-09-18 21:47:56]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1354003002/#ps240001
(title: ".")

[commit-bot: I haz the power, 2015-09-18 21:48:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354003002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354003002/240001

[Ben Goodger (Google), 2015-09-18 21:48:56]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-09-18 21:48:56]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1354003002/#ps260001
(title: ".")

[commit-bot: I haz the power, 2015-09-18 21:49:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354003002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354003002/260001

[yzshen1, 2015-09-18 21:51:08]

https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.h
File mojo/shell/identity.h (right):

https://codereview.chromium.org/1354003002/diff/220001/mojo/shell/identity.h#...
mojo/shell/identity.h:22: // Identity is constructed with a permissive filter
(allowing connection to
On 2015/09/18 21:44:04, yzshen1 wrote:
> Please update this comment.

(You probably didn't notice this comment. :))

[commit-bot: I haz the power, 2015-09-18 22:18:31]

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

[commit-bot: I haz the power, 2015-09-18 22:18:32]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2015-09-18 23:18:53]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-09-18 23:18:53]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1354003002/#ps280001
(title: ".")

[commit-bot: I haz the power, 2015-09-18 23:19:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354003002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354003002/280001

[commit-bot: I haz the power, 2015-09-19 00:13:25]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-09-19 00:14:00]

Patchset 15 (id:??) landed as
https://crrev.com/5403e28c9d9bfcf00b12721b6dc96c00f6d1ed24
Cr-Commit-Position: refs/heads/master@{#349815}

[Lei Zhang, 2015-09-21 05:33:40]

Did this break Linux GN?
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32189

[vabr (Chromium), 2015-09-21 06:58:27]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1351443004/ by vabr@chromium.org.

The reason for reverting is: Reverting for test breakage, more info on the bug.
BUG=534227.

[Ben Goodger (Google), 2015-09-22 19:03:28]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-09-22 19:03:29]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1354003002/#ps320001
(title: ".")

[commit-bot: I haz the power, 2015-09-22 19:05:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1354003002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1354003002/320001

[commit-bot: I haz the power, 2015-09-22 21:47:50]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-09-22 22:09:23]

Patchset 17 (id:??) landed as
https://crrev.com/3a72d2e4a0f3a6ffa8bc5bee5cd5af0f87b51090
Cr-Commit-Position: refs/heads/master@{#350241}