White-listing Chrome Remote Desktop to use the identity API in Public Session

White-listing Chrome Remote Desktop to use the identity API in Public Session

Chrome Remote Desktop currently uses Identity API to retrieve an OAuth token
for authentication with the backend.  By default, Identity API will return
an empty token in a public session. This CL

1. Modifies the Identity API to return a token from the device robot account
   when the request comes from a white-listed first party app.
2. Adds Chrome Remote Desktop to the white-list in (1)
3. Adds Chrome Remote Desktop to list of allowed apps in the public session.

BUG=508608
Committed: https://crrev.com/9a0f259cb27f78c3012d1b32bdf279e1c1609772
Cr-Commit-Position: refs/heads/master@{#351396}

[kelvinp, 2015-09-17 17:52:59]

Patchset #1 (id:1) has been deleted

[kelvinp, 2015-09-17 18:00:31]

kelvinp@chromium.org changed reviewers:
+ bartfab@chromium.org, courage@chromium.org

[kelvinp, 2015-09-17 18:06:15]

Patchset #1 (id:20001) has been deleted

[kelvinp, 2015-09-17 18:13:03]

Patchset #1 (id:40001) has been deleted

[kelvinp, 2015-09-17 18:13:12]

PTAL

[Andrew T Wilson (Slow), 2015-09-18 07:13:29]

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

[Andrew T Wilson (Slow), 2015-09-18 07:13:30]

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/identity_api.h:270: 
Two things:

1) This API should be private, not public.
2) This API should have a test (which maybe is a reason to make it public)

Having it public but not tested is the worst of both worlds :)

[bartfab (slow), 2015-09-18 12:24:57]

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/identity_api.cc:383:
IsOriginWhitelistedInPublicSession()) &&
You should first check whether this is a public session. Otherwise, you are
short-cutting the flow in lines 390 onward for the whitelisted apps when run in
a regular session.

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/identity_api.h:266: // will be called
only in ChromeOS for:
Nit: s/ChromeOS/Chrome OS/

[kelvinp, 2015-09-22 00:50:37]

PTAL

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/identity_api.cc:383:
IsOriginWhitelistedInPublicSession()) &&
On 2015/09/18 12:24:57, bartfab wrote:
> You should first check whether this is a public session. Otherwise, you are
> short-cutting the flow in lines 390 onward for the whitelisted apps when run
in
> a regular session.

Good point.  IsOriginWhitelistedInPublicSession() returns false when we are not
in a public session.  See line 768.
I did a quick check and the API does return the user's identity in a non-public
session so I think we are covered in this scenario.

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/identity_api.h:266: // will be called
only in ChromeOS for:
On 2015/09/18 12:24:57, bartfab wrote:
> Nit: s/ChromeOS/Chrome OS/

Done.

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/identity_api.h:270: 
On 2015/09/18 07:13:29, Andrew T Wilson wrote:
> Two things:
> 
> 1) This API should be private, not public.
> 2) This API should have a test (which maybe is a reason to make it public)
> 
> Having it public but not tested is the worst of both worlds :)

This function is private.  I have added a test case to ensure that
non-whitelisted apps have no access to the device token.

[Michael Courage, 2015-09-22 03:09:02]

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/identity_api.cc:383:
IsOriginWhitelistedInPublicSession()) &&
On 2015/09/22 00:50:37, kelvinp wrote:
> On 2015/09/18 12:24:57, bartfab wrote:
> > You should first check whether this is a public session. Otherwise, you are
> > short-cutting the flow in lines 390 onward for the whitelisted apps when run
> in
> > a regular session.
> 
> Good point.  IsOriginWhitelistedInPublicSession() returns false when we are
not
> in a public session.  See line 768.
> I did a quick check and the API does return the user's identity in a
non-public
> session so I think we are covered in this scenario.

The Kiosk path is already kind of convoluted. It would be nice if it the code
could be more obviously correct. It's hard to follow because of the large
conditional here, almost but not quite replicated in StartMintToken, plus the
fact that IsOriginWhitelistedInPublicSession checks two things (1. is it a
public session, and 2. is this app allowed to operate in a public session).

A possible improvement:
Are non-whitelisted apps running in a public session ever supposed to get a
useful result out of getAuthToken? If not, I think it would be good to error out
right up top as we do for incognito on 315. With the whitelist taken care of up
front, you don't have to worry about it through the rest of the flow.

[bartfab (slow), 2015-09-23 12:37:42]

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/identity_api.cc:383:
IsOriginWhitelistedInPublicSession()) &&
On 2015/09/22 03:09:02, Michael Courage wrote:
> On 2015/09/22 00:50:37, kelvinp wrote:
> > On 2015/09/18 12:24:57, bartfab wrote:
> > > You should first check whether this is a public session. Otherwise, you
are
> > > short-cutting the flow in lines 390 onward for the whitelisted apps when
run
> > in
> > > a regular session.
> > 
> > Good point.  IsOriginWhitelistedInPublicSession() returns false when we are
> not
> > in a public session.  See line 768.
> > I did a quick check and the API does return the user's identity in a
> non-public
> > session so I think we are covered in this scenario.
> 
> The Kiosk path is already kind of convoluted. It would be nice if it the code
> could be more obviously correct. It's hard to follow because of the large
> conditional here, almost but not quite replicated in StartMintToken, plus the
> fact that IsOriginWhitelistedInPublicSession checks two things (1. is it a
> public session, and 2. is this app allowed to operate in a public session).
> 
> A possible improvement:
> Are non-whitelisted apps running in a public session ever supposed to get a
> useful result out of getAuthToken? If not, I think it would be good to error
out
> right up top as we do for incognito on 315. With the whitelist taken care of
up
> front, you don't have to worry about it through the rest of the flow.

The order needs to be the opposite: Check whether we are in a public session
first and only if so, consult the whitelist. Otherwise, we would start refusing
non-whitelisted apps in non-public-sessions as well.

I guess we can collapse the kiosk and public session cases somewhat if we want
to:

if (enterprise_managed && (is_kiosk || is_public_session)) {
  if (is_public_session && !is_origin_whitelisted())
    return false;
  StartMintTokenFlow();
  return true;
}

[kelvinp, 2015-09-23 21:08:54]

FYI

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/1306053013/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/identity/identity_api.cc:383:
IsOriginWhitelistedInPublicSession()) &&
On 2015/09/23 12:37:42, bartfab wrote:
> On 2015/09/22 03:09:02, Michael Courage wrote:
> > On 2015/09/22 00:50:37, kelvinp wrote:
> > > On 2015/09/18 12:24:57, bartfab wrote:
> > > > You should first check whether this is a public session. Otherwise, you
> are
> > > > short-cutting the flow in lines 390 onward for the whitelisted apps when
> run
> > > in
> > > > a regular session.
> > > 
> > > Good point.  IsOriginWhitelistedInPublicSession() returns false when we
are
> > not
> > > in a public session.  See line 768.
> > > I did a quick check and the API does return the user's identity in a
> > non-public
> > > session so I think we are covered in this scenario.
> > 
> > The Kiosk path is already kind of convoluted. It would be nice if it the
code
> > could be more obviously correct. It's hard to follow because of the large
> > conditional here, almost but not quite replicated in StartMintToken, plus
the
> > fact that IsOriginWhitelistedInPublicSession checks two things (1. is it a
> > public session, and 2. is this app allowed to operate in a public session).
> > 
> > A possible improvement:
> > Are non-whitelisted apps running in a public session ever supposed to get a
> > useful result out of getAuthToken? If not, I think it would be good to error
> out
> > right up top as we do for incognito on 315. With the whitelist taken care of
> up
> > front, you don't have to worry about it through the rest of the flow.
> 
> The order needs to be the opposite: Check whether we are in a public session
> first and only if so, consult the whitelist. Otherwise, we would start
refusing
> non-whitelisted apps in non-public-sessions as well.
> 
> I guess we can collapse the kiosk and public session cases somewhat if we want
> to:
> 
> if (enterprise_managed && (is_kiosk || is_public_session)) {
>   if (is_public_session && !is_origin_whitelisted())
>     return false;
>   StartMintTokenFlow();
>   return true;
> }
>      

I don't think there is a concern.  For regular sessions (non-public session),
IsOrginWhiteListedInPublicSession will return false. And it will go through the
normal flow in line 390.

[kelvinp, 2015-09-24 21:51:42]

ping

[Michael Courage, 2015-09-24 22:08:18]

On 2015/09/24 21:51:42, kelvinp wrote:
> ping

For my part, I'd still like the conditionals to be easier to read. In particular
I think it would be good if readers could more easily see two separate checks
happening:

1. Is the user a special kiosk/public user?
2. If #1 is true, is the app allowed to proceed?

Even if the current code is correct, it's harder than necessary to follow, which
creates work for readers, and could cause bugs in the future if someone makes
changes without correctly understanding the flow.

[kelvinp, 2015-09-25 01:04:12]

Patchset #3 (id:100001) has been deleted

[kelvinp, 2015-09-25 01:05:42]

Thank you for all the feedback.
I have addressed all open comments.
PTAL

[kelvinp, 2015-09-25 23:47:36]

On 2015/09/25 01:05:42, kelvinp wrote:
> Thank you for all the feedback.
> I have addressed all open comments.
> PTAL

ping

[Michael Courage, 2015-09-25 23:49:04]

https://codereview.chromium.org/1306053013/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/1306053013/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/identity/identity_api.cc:518: return;
What's the intended behavior here? Returning out here without setting an error,
or kicking off a token request will probably hang. I don't know if it's actually
possible to get to this line, but it seems kind of scary.

[kelvinp, 2015-09-28 22:02:58]

PTAL

https://codereview.chromium.org/1306053013/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/1306053013/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/identity/identity_api.cc:518: return;
On 2015/09/25 23:49:04, Michael Courage wrote:
> What's the intended behavior here? Returning out here without setting an
error,
> or kicking off a token request will probably hang. I don't know if it's
actually
> possible to get to this line, but it seems kind of scary.

Done.

[Michael Courage, 2015-09-28 22:08:57]

chrome/browser/extensions/api/identity lgtm

[kelvinp, 2015-09-28 22:13:06]

Thank you Michael.
bartfab and atwilson, can you take a second look as we are getting closer to
branch point?

[Andrew T Wilson (Slow), 2015-09-29 13:20:30]

LGTM with a few nits. Would also like clarification as to why we are
whitelisting both Chrome RDP and Chrome Remote Desktop apps.

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc
(right):

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:23:
"cbkkbcmdlboombapidmoeolnmdacpkch",  // Chrome RDP
Is "Chrome RDP" now obsolete then?

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/identity/identity_api.cc:515: // Always force
minting token for ChromeOS kiosk app.
Comment is out of date since we support public session apps too.

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/identity/identity_api.cc:783: for (unsigned int i
= 0; i < arraysize(kPublicSessionAllowedOrigins); i++) {
nit: should i be size_t, not unsigned int? Also, we prefer ints over unsigned
ints wherever possible, so shouldn't use unsigned int here anyway since it's
prohibited by the style guide:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/identity/identity_api.cc:786: DCHECK(extension());
Move this DCHECK outside the loop? And maybe also move construction of GURL
below outside the loop?

[kelvinp, 2015-09-29 18:08:50]

Thank you for the review.
FYI

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc
(right):

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:23:
"cbkkbcmdlboombapidmoeolnmdacpkch",  // Chrome RDP
On 2015/09/29 13:20:30, Andrew T Wilson wrote:
> Is "Chrome RDP" now obsolete then?

No, it is an orthogonal effort.
Chrome RDP is a RDP client that allows you to connect to any RDP service. It is
good for situations when you are not allowed to install the host (as CRD
requires a host binary) on the target machine and when you don't require
firewall traversal.

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/identity/identity_api.cc:515: // Always force
minting token for ChromeOS kiosk app.
On 2015/09/29 13:20:30, Andrew T Wilson wrote:
> Comment is out of date since we support public session apps too.

Done.

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/identity/identity_api.cc:783: for (unsigned int i
= 0; i < arraysize(kPublicSessionAllowedOrigins); i++) {
On 2015/09/29 13:20:30, Andrew T Wilson wrote:
> nit: should i be size_t, not unsigned int? Also, we prefer ints over unsigned
> ints wherever possible, so shouldn't use unsigned int here anyway since it's
> prohibited by the style guide:
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types

Good catch.

https://codereview.chromium.org/1306053013/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/identity/identity_api.cc:786: DCHECK(extension());
On 2015/09/29 13:20:30, Andrew T Wilson wrote:
> Move this DCHECK outside the loop? And maybe also move construction of GURL
> below outside the loop?

Done.

[kelvinp, 2015-09-29 18:09:46]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-09-29 18:09:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
courage@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1306053013/#ps160001
(title: "Reviewer's feedback")

[commit-bot: I haz the power, 2015-09-29 18:10:58]

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

[kelvinp, 2015-09-29 18:13:49]

The CQ bit was unchecked by kelvinp@chromium.org

[commit-bot: I haz the power, 2015-09-29 18:14:24]

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

[commit-bot: I haz the power, 2015-09-29 18:14:25]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kelvinp, 2015-09-29 18:17:45]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-09-29 18:17:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
courage@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1306053013/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-29 18:19:02]

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

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

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

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

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kelvinp, 2015-09-29 18:26:08]

Patchset #6 (id:180001) has been deleted

[kelvinp, 2015-09-29 19:01:17]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-09-29 19:01:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
courage@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1306053013/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-29 19:02:32]

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

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

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

[commit-bot: I haz the power, 2015-09-29 19:21:50]

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

[kelvinp, 2015-09-29 20:17:48]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-09-29 20:17:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
courage@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1306053013/#ps220001
(title: "Fix compiler error on Windows")

[commit-bot: I haz the power, 2015-09-29 20:17:57]

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

[commit-bot: I haz the power, 2015-09-29 21:01:29]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2015-09-29 21:03:03]

Patchset 7 (id:??) landed as
https://crrev.com/9a0f259cb27f78c3012d1b32bdf279e1c1609772
Cr-Commit-Position: refs/heads/master@{#351396}