Add optional scopes parameter to Identity.getToken().

Add optional scopes parameter to Identity.getToken().

BUG=b/13306957

Committed: https://crrev.com/e13a9d574fb9c9a4d0f36351e0e91e89b4e17928
Cr-Commit-Position: refs/heads/master@{#321211}

[Jamie, 2015-03-18 01:01:09]

jamiewalch@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Jamie, 2015-03-18 01:01:10]

PTAL

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
File remoting/webapp/crd/js/identity_unittest.js (left):

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity_unittest.js:27: this.scopes = undefined;
This was not being used. It was a hangover from the fact that I originally wrote
these unit tests as part of this CL, and wanted to be able to verify that the
"interactive: false" call had the correct scopes. However, since the application
framework enforces that the first call does not specify scopes, I don't think
it's an important test case.

[kelvinp, 2015-03-18 18:51:27]

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:74: 'interactive': this.interactive_,
Do we need to reset interactive?
In the current implementation, once we show the prompt once, we will set
this.interactive_ to true for all subsequent requests.

interactive should probably need to be bound to onAuthComplete or have an
individual value per scope.

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:78: this.onAuthComplete_.bind(this,
opt_scopes));
Why not bind the key instead, as the first thing that onAuthComplete do is to
convert the scope into the key.

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:242: return opt_scopes ?
JSON.stringify(opt_scopes) : '';
I think we should sort the scopes first so that order does not matter.
if (Array.isArray(opt_scopes)) {
  return JSON.stringify(opt_scopes.sort());
}
return '';

[Jamie, 2015-03-18 19:08:49]

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:74: 'interactive': this.interactive_,
On 2015/03/18 18:51:26, kelvinp wrote:
> Do we need to reset interactive?
> In the current implementation, once we show the prompt once, we will set
> this.interactive_ to true for all subsequent requests.
> 
> interactive should probably need to be bound to onAuthComplete or have an
> individual value per scope.

Support for scopes is part of the reason I changed the interactive flag to be a
member variable instead of always trying "interactive: false" first (which I
believed was a requirement of the API, thinking that the interactive version
could only be called from a UI event). The problem with that approach is that
it's not clear how to handle multiple outstanding getToken requests with
different scopes. The naive solution involves multiple calls to the consent
callback, each of which results in some DOM manipulation, and it quickly becomes
very hard to reason about the possible outcomes.

Modifying the Application framework to pre-authorize all scopes declared in the
manifest makes the behaviour a lot more predictable because the consent callback
can now only be invoked at start-up, making the ramifications of modifying the
DOM a lot more predictable. It's still possible for chrome.identity to require
authorization (if the app requests a scope not declared in the manifest or if
the user revokes authorization), and at this point the UX will be jarring, but
those are both corner cases and still less jarring than the in-DOM dialog would
be.

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:78: this.onAuthComplete_.bind(this,
opt_scopes));
On 2015/03/18 18:51:26, kelvinp wrote:
> Why not bind the key instead, as the first thing that onAuthComplete do is to
> convert the scope into the key.

onAuthComplete needs both the scopes and the key, so I can't replace the scopes
with the key. Are you proposing binding both? I don't have a strong opinion as
to which is cleaner. Binding both does mean I can do away with the getScopesKey
function.

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:242: return opt_scopes ?
JSON.stringify(opt_scopes) : '';
On 2015/03/18 18:51:26, kelvinp wrote:
> I think we should sort the scopes first so that order does not matter.
> if (Array.isArray(opt_scopes)) {
>   return JSON.stringify(opt_scopes.sort());
> }
> return '';

I was about to do this when I considered how I could change the unit tests to
verify it, and realized that there would be no change. Sorting the list would
only reduce the number of Deferred instances we need to save between
getAuthToken and its callback. I don't object to doing this, but it makes the
code more complex and I don't think it really buys us anything.

[kelvinp, 2015-03-18 19:19:34]

lgtm

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:74: 'interactive': this.interactive_,
On 2015/03/18 19:08:49, Jamie wrote:
> On 2015/03/18 18:51:26, kelvinp wrote:
> > Do we need to reset interactive?
> > In the current implementation, once we show the prompt once, we will set
> > this.interactive_ to true for all subsequent requests.
> > 
> > interactive should probably need to be bound to onAuthComplete or have an
> > individual value per scope.
> 
> Support for scopes is part of the reason I changed the interactive flag to be
a
> member variable instead of always trying "interactive: false" first (which I
> believed was a requirement of the API, thinking that the interactive version
> could only be called from a UI event). The problem with that approach is that
> it's not clear how to handle multiple outstanding getToken requests with
> different scopes. The naive solution involves multiple calls to the consent
> callback, each of which results in some DOM manipulation, and it quickly
becomes
> very hard to reason about the possible outcomes.
> 
> Modifying the Application framework to pre-authorize all scopes declared in
the
> manifest makes the behaviour a lot more predictable because the consent
callback
> can now only be invoked at start-up, making the ramifications of modifying the
> DOM a lot more predictable. It's still possible for chrome.identity to require
> authorization (if the app requests a scope not declared in the manifest or if
> the user revokes authorization), and at this point the UX will be jarring, but
> those are both corner cases and still less jarring than the in-DOM dialog
would
> be.

Acknowledged.

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:78: this.onAuthComplete_.bind(this,
opt_scopes));
On 2015/03/18 19:08:49, Jamie wrote:
> On 2015/03/18 18:51:26, kelvinp wrote:
> > Why not bind the key instead, as the first thing that onAuthComplete do is
to
> > convert the scope into the key.
> 
> onAuthComplete needs both the scopes and the key, so I can't replace the
scopes
> with the key. Are you proposing binding both? I don't have a strong opinion as
> to which is cleaner. Binding both does mean I can do away with the
getScopesKey
> function.

Sorry.  I missed that part.  Acknowledged.

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:242: return opt_scopes ?
JSON.stringify(opt_scopes) : '';
On 2015/03/18 19:08:49, Jamie wrote:
> On 2015/03/18 18:51:26, kelvinp wrote:
> > I think we should sort the scopes first so that order does not matter.
> > if (Array.isArray(opt_scopes)) {
> >   return JSON.stringify(opt_scopes.sort());
> > }
> > return '';
> 
> I was about to do this when I considered how I could change the unit tests to
> verify it, and realized that there would be no change. Sorting the list would
> only reduce the number of Deferred instances we need to save between
> getAuthToken and its callback. I don't object to doing this, but it makes the
> code more complex and I don't think it really buys us anything.

I think the complexity would only be extra line of code.  I don't feel strongly
either way.

[Jamie, 2015-03-18 20:04:00]

FYI

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
File remoting/webapp/crd/js/identity.js (right):

https://codereview.chromium.org/1015043002/diff/1/remoting/webapp/crd/js/iden...
remoting/webapp/crd/js/identity.js:242: return opt_scopes ?
JSON.stringify(opt_scopes) : '';
On 2015/03/18 19:19:33, kelvinp wrote:
> On 2015/03/18 19:08:49, Jamie wrote:
> > On 2015/03/18 18:51:26, kelvinp wrote:
> > > I think we should sort the scopes first so that order does not matter.
> > > if (Array.isArray(opt_scopes)) {
> > >   return JSON.stringify(opt_scopes.sort());
> > > }
> > > return '';
> > 
> > I was about to do this when I considered how I could change the unit tests
to
> > verify it, and realized that there would be no change. Sorting the list
would
> > only reduce the number of Deferred instances we need to save between
> > getAuthToken and its callback. I don't object to doing this, but it makes
the
> > code more complex and I don't think it really buys us anything.
> 
> I think the complexity would only be extra line of code.  I don't feel
strongly
> either way.

Let's keep it simple for now.

[Jamie, 2015-03-18 20:04:03]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-03-18 20:04:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1015043002/1

[commit-bot: I haz the power, 2015-03-18 21:38:40]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-18 21:39:41]

Patchset 1 (id:??) landed as
https://crrev.com/e13a9d574fb9c9a4d0f36351e0e91e89b4e17928
Cr-Commit-Position: refs/heads/master@{#321211}