Add optional scopes parameter to Identity.getToken().

remoting/webapp/crd/js/identity.js

Index: remoting/webapp/crd/js/identity.js
diff --git a/remoting/webapp/crd/js/identity.js b/remoting/webapp/crd/js/identity.js
index e940dac3d3802329020436ac6bce4ff0c58f285d..742a552d6bb29825fe19d37d0fcf8e71e9515ea3 100644
--- a/remoting/webapp/crd/js/identity.js
+++ b/remoting/webapp/crd/js/identity.js
@@ -31,8 +31,8 @@ remoting.Identity = function(opt_consentDialog) {
   this.email_ = '';
   /** @private {string} */
   this.fullName_ = '';
-  /** @type {base.Deferred<string>} */
-  this.authTokenDeferred_ = null;
+  /** @private {Object<base.Deferred<string>>} */
+  this.authTokensDeferred_ = {};
   /** @private {boolean} */
   this.interactive_ = false;
 };
@@ -55,20 +55,29 @@ remoting.Identity.ConsentDialog.prototype.show = function() {};
 /**
  * Gets an access token.
  *
+ * @param {Array<string>=} opt_scopes Optional OAuth2 scopes to request. If not
+ *     specified, the scopes specified in the manifest will be used. No consent
+ *     prompt will be needed as long as the requested scopes are a subset of
+ *     those already granted (in most cases, the remoting.Application framework
+ *     ensures that the scopes specified in the manifest are already authorized
+ *     before any application code is executed). Callers can request scopes not
+ *     specified in the manifest, but a consent prompt will be shown.
+ *
  * @return {!Promise<string>} A promise resolved with an access token
  *     or rejected with a remoting.Error.
  */
-remoting.Identity.prototype.getToken = function() {
-  /** @const */
-  var that = this;
-
-  if (this.authTokenDeferred_ == null) {
-    this.authTokenDeferred_ = new base.Deferred();
-    chrome.identity.getAuthToken(
-        { 'interactive': this.interactive_ },
-        this.onAuthComplete_.bind(this));
+remoting.Identity.prototype.getToken = function(opt_scopes) {
+  var key = getScopesKey(opt_scopes);
+  if (!this.authTokensDeferred_[key]) {
+    this.authTokensDeferred_[key] = new base.Deferred();
+    var options = {
+      'interactive': this.interactive_,

[kelvinp, 2015/03/18 18:51:26]

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.

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

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.

[kelvinp, 2015/03/18 19:19:33]

Acknowledged.

+      'scopes': opt_scopes
+    };
+    chrome.identity.getAuthToken(options,
+                                 this.onAuthComplete_.bind(this, opt_scopes));

[kelvinp, 2015/03/18 18:51:26]

Why not bind the key instead, as the first thing that onAuthComplete do is to
convert the scope into the key.

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

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.

[kelvinp, 2015/03/18 19:19:33]

Sorry.  I missed that part.  Acknowledged.

   }
-  return this.authTokenDeferred_.promise();
+  return this.authTokensDeferred_[key].promise();
 };
 
 /**
@@ -167,16 +176,20 @@ remoting.Identity.prototype.getEmail = function() {
 /**
  * Callback for the getAuthToken API.
  *
+ * @param {Array<string>|undefined} scopes The explicit scopes passed to
+ *     getToken, or undefined if no scopes were specified.
  * @param {?string} token The auth token, or null if the request failed.
  * @private
  */
-remoting.Identity.prototype.onAuthComplete_ = function(token) {
-  var authTokenDeferred = this.authTokenDeferred_;
+remoting.Identity.prototype.onAuthComplete_ = function(scopes, token) {
+  var key = getScopesKey(scopes);
+  var authTokenDeferred = this.authTokensDeferred_[key];
 
   // Pass the token to the callback(s) if it was retrieved successfully.
   if (token) {
-    authTokenDeferred.resolve(token);
-    this.authTokenDeferred_ = null;
+    var promise = this.authTokensDeferred_[key];
+    delete this.authTokensDeferred_[key];
+    promise.resolve(token);
     return;
   }
 
@@ -190,8 +203,8 @@ remoting.Identity.prototype.onAuthComplete_ = function(token) {
     var error = (error_message == USER_CANCELLED) ?
         new remoting.Error(remoting.Error.Tag.CANCELLED) :
         new remoting.Error(remoting.Error.Tag.NOT_AUTHENTICATED);
-    authTokenDeferred.reject(error);
-    this.authTokenDeferred_ = null;
+    this.authTokensDeferred_[key].reject(error);
+    delete this.authTokensDeferred_[key];
     return;
   }
 
@@ -202,8 +215,12 @@ remoting.Identity.prototype.onAuthComplete_ = function(token) {
       (this.consentDialog_) ? this.consentDialog_.show() : Promise.resolve();
   showConsentDialog.then(function() {
     that.interactive_ = true;
-    chrome.identity.getAuthToken({'interactive': that.interactive_},
-                                 that.onAuthComplete_.bind(that));
+    var options = {
+      'interactive': that.interactive_,
+      'scopes': scopes
+    };
+    chrome.identity.getAuthToken(options,
+                                 that.onAuthComplete_.bind(that, scopes));
   });
 };
 
@@ -216,4 +233,13 @@ remoting.Identity.prototype.isAuthenticated = function() {
   return remoting.identity.email_ !== '';
 };
 
+
+/**
+ * @param {Array<string>=} opt_scopes
+ * @return {string}
+ */
+function getScopesKey(opt_scopes) {
+  return opt_scopes ? JSON.stringify(opt_scopes) : '';

[kelvinp, 2015/03/18 18:51:26]

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]

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:33]

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

[Jamie, 2015/03/18 20:03:59]

Let's keep it simple for now.

+}
+
 })();