Cleaned up OAuth refresh error handling.

remoting/webapp/oauth2.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview
  * OAuth2 class that handles retrieval/storage of an OAuth2 token.
  *
  * Uses a content script to trampoline the OAuth redirect page back into the
  * extension context.  This works around the lack of native support for
@@ skipping 44 matching lines @@
   return false;
 };
 
 /**
  * Removes all storage, and effectively unauthenticates the user.
  *
  * @return {void} Nothing.
  */
 remoting.OAuth2.prototype.clear = function() {
   window.localStorage.removeItem(this.KEY_EMAIL_);
-  this.clearAccessToken();
+  this.clearAccessToken_();
   this.clearRefreshToken_();
 };
 
 /**
  * Sets the refresh token.
  *
  * This method also marks the token as revokable, so that this object will
  * revoke the token when it no longer needs it.
  *
  * @param {string} token The new refresh token.
  * @return {void} Nothing.
  */
 remoting.OAuth2.prototype.setRefreshToken = function(token) {
   window.localStorage.setItem(this.KEY_REFRESH_TOKEN_, escape(token));
   window.localStorage.setItem(this.KEY_REFRESH_TOKEN_REVOKABLE_, true);
-  this.clearAccessToken();
+  this.clearAccessToken_();
 };
 
 /**
  * Gets the refresh token.
  *
  * This method also marks the refresh token as not revokable, so that this
  * object will not revoke the token when it no longer needs it. After this
  * object has exported the token, it cannot know whether it is still in use
  * when this object no longer needs it.
  *
@@ skipping 64 matching lines @@
   console.log('Invalid access token stored.');
   return {'token': '', 'expiration': 0};
 };
 
 /**
  * Returns true if the access token is expired, or otherwise invalid.
  *
  * Will throw if !isAuthenticated().
  *
  * @return {boolean} True if a new access token is needed.
+ * @private
  */
-remoting.OAuth2.prototype.needsNewAccessToken = function() {
+remoting.OAuth2.prototype.needsNewAccessToken_ = function() {
   if (!this.isAuthenticated()) {
     throw 'Not Authenticated.';
   }
   var access_token = this.getAccessTokenInternal_();
   if (!access_token['token']) {
     return true;
   }
   if (Date.now() > access_token['expiration']) {
     return true;
   }
   return false;
 };
 
 /**
- * Returns the current access token.
- *
- * Will throw if !isAuthenticated() or needsNewAccessToken().
- *
- * @return {string} The access token.
+ * @return {void} Nothing.
  * @private
  */
-remoting.OAuth2.prototype.getAccessToken_ = function() {

[Jamie, 2012/07/17 00:25:38]

Only used in two places, one of which was a potential time-related bug as
mentioned in the CL description. Now inlined in callWithToken.

-  if (this.needsNewAccessToken()) {
-    throw 'Access Token expired.';
-  }
-  return this.getAccessTokenInternal_()['token'];
-};
-
-/** @return {void} Nothing. */
-remoting.OAuth2.prototype.clearAccessToken = function() {
+remoting.OAuth2.prototype.clearAccessToken_ = function() {
   window.localStorage.removeItem(this.KEY_ACCESS_TOKEN_);
 };
 
 /**
  * Update state based on token response from the OAuth2 /token endpoint.
  *
  * @private
- * @param {function(XMLHttpRequest): void} onDone Callback to invoke on
+ * @param {function(XMLHttpRequest, string): void} onDone Callback to invoke on
  *     completion.
  * @param {XMLHttpRequest} xhr The XHR object for this request.
  * @return {void} Nothing.
  */
 remoting.OAuth2.prototype.processTokenResponse_ = function(onDone, xhr) {
   if (xhr.status == 200) {
     try {
       // Don't use jsonParseSafe here unless you move the definition out of
       // remoting.js, otherwise this won't work from the OAuth trampoline.
       // TODO(jamiewalch): Fix this once we're no longer using the trampoline.
@@ skipping 12 matching lines @@
       this.setAccessToken(tokens['access_token'],
           (tokens['expires_in'] - (120 + 30)) * 1000 + Date.now());
     } catch (err) {
       console.error('Invalid "token" response from server:',
                     /** @type {*} */ (err));
     }
   } else {
     console.error('Failed to get tokens. Status: ' + xhr.status +
                   ' response: ' + xhr.responseText);
   }
-  onDone(xhr);
+  onDone(xhr, tokens['access_token']);

[simonmorris, 2012/07/17 00:52:50]

Is 'tokens' in scope here? What happens if xhr.status isn't 200?

[Jamie, 2012/07/17 01:14:11]

You're right, it will fail. Good catch!

 };
 
 /**
  * Asynchronously retrieves a new access token from the server.
  *
  * Will throw if !isAuthenticated().
  *
  * @param {function(XMLHttpRequest): void} onDone Callback to invoke on
  *     completion.
  * @return {void} Nothing.
@@ skipping 74 matching lines @@
       console.log('Failed to revoke token. Status: ' + xhr.status +
                   ' ; response: ' + xhr.responseText + ' ; xhr: ', xhr);
     }
   };
   remoting.xhr.post(this.OAUTH2_REVOKE_TOKEN_ENDPOINT_,
                     processResponse,
                     parameters);
 };
 
 /**
- * Call myfunc with an access token as the only parameter.
- *
- * This will refresh the access token if necessary.  If the access token
- * cannot be refreshed, an error is thrown.
- *
+ * Call a function with an access token, refreshing it first if necessary.
  * The access token will remain valid for at least 2 minutes.
  *
  * @param {function(string):void} onOk Function to invoke with access token if
  *     an access token was successfully retrieved.
  * @param {function(remoting.Error):void} onError Function to invoke with an
  *     error code on failure.
  * @return {void} Nothing.
  */
 remoting.OAuth2.prototype.callWithToken = function(onOk, onError) {
-  try {
-    if (this.needsNewAccessToken()) {
+  if (this.isAuthenticated()) {
+    if (this.needsNewAccessToken_()) {
       this.refreshAccessToken_(this.onRefreshToken_.bind(this, onOk, onError));
     } else {
-      onOk(this.getAccessToken_());
+      onOk(this.getAccessTokenInternal_()['token']);
     }
-  } catch (error) {
+  } else {
     onError(remoting.Error.NOT_AUTHENTICATED);
   }
 };
 
 /**
  * Process token refresh results and notify caller.
  *
  * @param {function(string):void} onOk Function to invoke with access token if
  *     an access token was successfully retrieved.
  * @param {function(remoting.Error):void} onError Function to invoke with an
  *     error code on failure.
  * @param {XMLHttpRequest} xhr The result of the refresh operation.
+ * @param {string} accessToken The fresh access token.
  * @private
  */
-remoting.OAuth2.prototype.onRefreshToken_ = function(onOk, onError, xhr) {
+remoting.OAuth2.prototype.onRefreshToken_ = function(onOk, onError, xhr,
+                                                     accessToken) {
   var error = remoting.Error.UNEXPECTED;
   if (xhr.status == 200) {
-    onOk(this.getAccessToken_());
+    onOk(accessToken);
     return;
   } else if (xhr.status == 400) {
     var result =
         /** @type {{error: string}} */ (jsonParseSafe(xhr.responseText));
     if (result && result.error == 'invalid_grant') {
       error = remoting.Error.AUTHENTICATION_FAILED;
     }
   } else if (xhr.status == 401) {
     // According to the OAuth2 draft RFC, the server shouldn't return 401,
     // but AUTHENTICATION_FAILED is the obvious interpretation if it does.
@@ skipping 51 matching lines @@
  * @return {?string} The email address, if it has been cached by a previous call
  *     to getEmail, otherwise null.
  */
 remoting.OAuth2.prototype.getCachedEmail = function() {
   var value = window.localStorage.getItem(this.KEY_EMAIL_);
   if (typeof value == 'string') {
     return value;
   }
   return null;
 };