PS - Scrub URL down to origin in Public Sessions

PS - Scrub URL down to origin in Public Sessions

In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we scrub the URL returned by chrome.tabs API down to the origin.

TEST=
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate
BUG=709513
Review-Url: https://codereview.chromium.org/2830903003
Cr-Commit-Position: refs/heads/master@{#466679}
Committed: https://chromium.googlesource.com/chromium/src/+/e09511329119adbacc5e291b5d16b31fcb14a4d4

[Ivan Šandrk, 2017-04-20 16:07:13]

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

[commit-bot: I haz the power, 2017-04-20 16:07:46]

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

[Ivan Šandrk, 2017-04-20 16:14:47]

isandrk@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Ivan Šandrk, 2017-04-20 16:14:47]

Hey Devlin, please take a look!

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:848: // In
Public Sessions set the PS delegate on ExtensionTabUtil (used to scrub
Should I make this comment longer perhaps? I was thinking that this is maybe
fine since there's a similar comment right before it.

[Ivan Šandrk, 2017-04-20 16:23:55]

isandrk@chromium.org changed reviewers:
+ alemate@chromium.org

[Ivan Šandrk, 2017-04-20 16:23:55]

Alexander, ptal at chrome_user_manager_impl

[Devlin, 2017-04-20 16:33:18]

Nice! lgtm

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc
(right):

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc:28:
tab->url.reset(new std::string(scrubbed_url));
could even one-line this:
tab->url = base::MakeUnique<std::string>(GURL(*tab->url).GetOrigin().spec());

If you think that's a little too hard to read, we should at least use
`base::MakeUnique<std::string>(std::move(scrubbed_url))` to avoid bare calls to
new and creating an unnecessary copy.

[Ivan Šandrk, 2017-04-20 16:42:47]

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

[Ivan Šandrk, 2017-04-20 16:42:51]

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

[commit-bot: I haz the power, 2017-04-20 16:43:12]

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

[Ivan Šandrk, 2017-04-20 16:44:10]

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc
(right):

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc:28:
tab->url.reset(new std::string(scrubbed_url));
On 2017/04/20 16:33:18, Devlin wrote:
> could even one-line this:
> tab->url = base::MakeUnique<std::string>(GURL(*tab->url).GetOrigin().spec());
> 
> If you think that's a little too hard to read, we should at least use
> `base::MakeUnique<std::string>(std::move(scrubbed_url))` to avoid bare calls
to
> new and creating an unnecessary copy.

Roger. I did it this way because that's how it was done inside
extension_tab_util.cc

[commit-bot: I haz the power, 2017-04-20 17:38:56]

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

[commit-bot: I haz the power, 2017-04-20 17:38:56]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-04-24 11:35:53]

lgtm with nit.

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:848: // In
Public Sessions set the PS delegate on ExtensionTabUtil (used to scrub
On 2017/04/20 16:14:47, Ivan Šandrk wrote:
> Should I make this comment longer perhaps? I was thinking that this is maybe
> fine since there's a similar comment right before it.

I think we should reference PermissionsUpdaterDelegateChromeOS here just in case
something is inserted in between in the future.

Could you add a comment like '(like PermissionsUpdaterDelegateChromeOS above)' ?

[Ivan Šandrk, 2017-04-24 12:14:56]

Thanks for the reviews guys. Devlin, that was blazingly fast!

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/2830903003/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:848: // In
Public Sessions set the PS delegate on ExtensionTabUtil (used to scrub
On 2017/04/24 11:35:53, Alexander Alekseev wrote:
> On 2017/04/20 16:14:47, Ivan Šandrk wrote:
> > Should I make this comment longer perhaps? I was thinking that this is maybe
> > fine since there's a similar comment right before it.
> 
> I think we should reference PermissionsUpdaterDelegateChromeOS here just in
case
> something is inserted in between in the future.
> 
> Could you add a comment like '(like PermissionsUpdaterDelegateChromeOS above)'
?

Done.

[Ivan Šandrk, 2017-04-24 12:15:04]

The CQ bit was checked by isandrk@chromium.org

[Ivan Šandrk, 2017-04-24 12:15:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, alemate@chromium.org
Link to the patchset: https://codereview.chromium.org/2830903003/#ps40001
(title: "Comment update")

[commit-bot: I haz the power, 2017-04-24 12:15:24]

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-04-24 15:12:32]

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

[commit-bot: I haz the power, 2017-04-24 15:12:34]

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

[Ivan Šandrk, 2017-04-24 15:19:06]

The CQ bit was checked by isandrk@chromium.org

[commit-bot: I haz the power, 2017-04-24 15:19:47]

CQ is trying da patch.

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

[Ivan Šandrk, 2017-04-24 16:30:07]

The CQ bit was checked by isandrk@chromium.org

[Ivan Šandrk, 2017-04-24 16:30:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
alemate@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2830903003/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-24 16:30:47]

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-04-24 17:53:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493051407004680,
"parent_rev": "0b2a55e0ffdce5f803763794025178f32fea18ab", "commit_rev":
"e09511329119adbacc5e291b5d16b31fcb14a4d4"}

[commit-bot: I haz the power, 2017-04-24 17:53:45]

Description was changed from

==========
PS - Scrub URL down to origin in Public Sessions

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we scrub the URL returned by
chrome.tabs API down to the origin.

TEST=
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate
BUG=709513
==========

to

==========
PS - Scrub URL down to origin in Public Sessions

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we scrub the URL returned by
chrome.tabs API down to the origin.

TEST=
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate
BUG=709513

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

[commit-bot: I haz the power, 2017-04-24 17:53:46]

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

[Andrew T Wilson (Slow), 2017-04-25 15:11:08]

atwilson@chromium.org changed reviewers:
+ atwilson@chromium.org

[Andrew T Wilson (Slow), 2017-04-25 15:11:09]

https://codereview.chromium.org/2830903003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc
(right):

https://codereview.chromium.org/2830903003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc:28:
tab->url = base::MakeUnique<std::string>(GURL(*tab->url).GetOrigin().spec());
Quick question - what happens if the URL in the tab is malformed? Sounds like
GURL::spec() will assert in this case?

[Devlin, 2017-04-25 15:19:22]

https://codereview.chromium.org/2830903003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc
(right):

https://codereview.chromium.org/2830903003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc:28:
tab->url = base::MakeUnique<std::string>(GURL(*tab->url).GetOrigin().spec());
On 2017/04/25 15:11:09, Andrew T Wilson (Slow) wrote:
> Quick question - what happens if the URL in the tab is malformed? Sounds like
> GURL::spec() will assert in this case?

These values come from us populating a dictionary from a WebContents or session
history entry - I hope that neither of those could have invalid urls?

[Andrew T Wilson (Slow), 2017-04-25 17:05:53]

On 2017/04/25 15:19:22, Devlin wrote:
>
https://codereview.chromium.org/2830903003/diff/60001/chrome/browser/chromeos...
> File
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc
> (right):
> 
>
https://codereview.chromium.org/2830903003/diff/60001/chrome/browser/chromeos...
> chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos.cc:28:
> tab->url = base::MakeUnique<std::string>(GURL(*tab->url).GetOrigin().spec());
> On 2017/04/25 15:11:09, Andrew T Wilson (Slow) wrote:
> > Quick question - what happens if the URL in the tab is malformed? Sounds
like
> > GURL::spec() will assert in this case?
> 
> These values come from us populating a dictionary from a WebContents or
session
> history entry - I hope that neither of those could have invalid urls?

OK, maybe not? I just wasn't sure whether we could get some weird behavior in
there by (say) calling window.open(<random string>). If we're guaranteed that
it's valid, then cool (probably should add a DCHECK() to that effect :)