Cleaned up OAuth refresh error handling.

remoting/webapp/oauth2.js

Index: remoting/webapp/oauth2.js
diff --git a/remoting/webapp/oauth2.js b/remoting/webapp/oauth2.js
index ece2619959398c7e9ce323a97687d89377fc8c4b..467c9a099121c5dd52e4db32185d730df51be20e 100644
--- a/remoting/webapp/oauth2.js
+++ b/remoting/webapp/oauth2.js
@@ -62,7 +62,7 @@ remoting.OAuth2.prototype.isAuthenticated = function() {
  */
 remoting.OAuth2.prototype.clear = function() {
   window.localStorage.removeItem(this.KEY_EMAIL_);
-  this.clearAccessToken();
+  this.clearAccessToken_();
   this.clearRefreshToken_();
 };
 
@@ -78,7 +78,7 @@ remoting.OAuth2.prototype.clear = function() {
 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_();
 };
 
 /**
@@ -163,8 +163,9 @@ remoting.OAuth2.prototype.getAccessTokenInternal_ = function() {
  * 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.';
   }
@@ -179,22 +180,10 @@ remoting.OAuth2.prototype.needsNewAccessToken = function() {
 };
 
 /**
- * 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() {
-  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_);
 };
 
@@ -202,12 +191,13 @@ remoting.OAuth2.prototype.clearAccessToken = function() {
  * 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

[simonmorris, 2012/07/17 01:25:33]

Doesn't "string" need to be something like "string?" if the parameter can be
null? And shouldn't the prototypes in the processTokenResponse_ call sites also
be changed?

[Jamie, 2012/07/17 01:51:58]

I've changed the default to '', since that minimizes the amount of casting
required.

Regarding the other call sites, I think they're fine as they are: if they don't
use the access token parameter, it's fine for them not to declare it.

  *     completion.
  * @param {XMLHttpRequest} xhr The XHR object for this request.
  * @return {void} Nothing.
  */
 remoting.OAuth2.prototype.processTokenResponse_ = function(onDone, xhr) {
+  var accessToken = null;
   if (xhr.status == 200) {
     try {
       // Don't use jsonParseSafe here unless you move the definition out of
@@ -225,7 +215,8 @@ remoting.OAuth2.prototype.processTokenResponse_ = function(onDone, xhr) {
       // The choice of 2 minutes is arbitrary, but that length of time
       // is part of the contract satisfied by callWithToken().
       // Offset by a further 30 seconds to account for RTT issues.
-      this.setAccessToken(tokens['access_token'],
+      accessToken = tokens['access_token'];
+      this.setAccessToken(accessToken,
           (tokens['expires_in'] - (120 + 30)) * 1000 + Date.now());
     } catch (err) {
       console.error('Invalid "token" response from server:',
@@ -235,7 +226,7 @@ remoting.OAuth2.prototype.processTokenResponse_ = function(onDone, xhr) {
     console.error('Failed to get tokens. Status: ' + xhr.status +
                   ' response: ' + xhr.responseText);
   }
-  onDone(xhr);
+  onDone(xhr, accessToken);
 };
 
 /**
@@ -330,11 +321,7 @@ remoting.OAuth2.prototype.revokeToken_ = function(token) {
 };
 
 /**
- * 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
@@ -344,13 +331,13 @@ remoting.OAuth2.prototype.revokeToken_ = function(token) {
  * @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);
   }
 };
@@ -363,12 +350,14 @@ remoting.OAuth2.prototype.callWithToken = function(onOk, onError) {
  * @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 =