Merge pull request #517 from hypothesis/oauth-simplify-token-refresh

Remove logic to refresh access token before it expires.

src/sidebar/oauth-auth.js

@@ -11,6 +11,15 @@ var serviceConfig = require('./service-config');
  *   use in future sessions.
  */
 
+/**
+ * An object holding the details of an access token from the tokenUrl endpoint.
+ * @typedef {Object} TokenInfo
+ * @property {string} accessToken  - The access token itself.
+ * @property {number} expiresAt    - The date when the timestamp will expire.
+ * @property {string} refreshToken - The refresh token that can be used to
+ *                                   get a new access token.
+ */
+
 /**
  * OAuth-based authorization service.
  *
@@ -27,10 +36,17 @@ function auth($http, $window, flash, localStorage, random, settings) {
   var authCode;
 
   /**
-   * Access token retrieved via `POST /token` endpoint.
-   * @type {Promise<string>}
+   * Token info retrieved via `POST /api/token` endpoint.
+   *
+   * Resolves to `null` if the user is not logged in.
+   *
+   * @type {Promise<TokenInfo|null>}
+   */
+  var tokenInfoPromise;
+
+  /**
+   * Absolute URL of the `/api/token` endpoint.
    */
-  var accessTokenPromise;
   var tokenUrl = resolve('token', settings.apiUrl);
 
   /**
@@ -53,15 +69,6 @@ function auth($http, $window, flash, localStorage, random, settings) {
     );
   }
 
-  /**
-   * An object holding the details of an access token from the tokenUrl endpoint.
-   * @typedef {Object} TokenInfo
-   * @property {string} accessToken  - The access token itself.
-   * @property {number} expiresAt    - The date when the timestamp will expire.
-   * @property {string} refreshToken - The refresh token that can be used to
-   *                                   get a new access token.
-   */
-
   /**
    * Return a new TokenInfo object from the given tokenUrl endpoint response.
    * @param {Object} response - The HTTP response from a POST to the tokenUrl
@@ -73,9 +80,10 @@ function auth($http, $window, flash, localStorage, random, settings) {
     return {
       accessToken:  data.access_token,
 
-      // Set the expiry date to some time before the actual expiry date so that
-      // we will refresh it before it actually expires.
-      expiresAt: Date.now() + (data.expires_in * 1000 * 0.91),
+      // Set the expiry date to some time slightly before that implied by
+      // `expires_in` to account for the delay in the client receiving the
+      // response.
+      expiresAt: Date.now() + ((data.expires_in - 10) * 1000),
 
       refreshToken: data.refresh_token,
     };
@@ -181,7 +189,7 @@ function auth($http, $window, flash, localStorage, random, settings) {
    *
    * @param {string} refreshToken
    * @param {RefreshOptions} options
-   * @return {Promise<string|null>} Promise for the new access token
+   * @return {Promise<TokenInfo>} Promise for the new access token
    */
   function refreshAccessToken(refreshToken, options) {
     var data = { grant_type: 'refresh_token', refresh_token: refreshToken };
@@ -192,61 +200,23 @@ function auth($http, $window, flash, localStorage, random, settings) {
         saveToken(tokenInfo);
       }
 
-      refreshAccessTokenBeforeItExpires(tokenInfo, {
-        persist: options.persist,
-      });
-      accessTokenPromise = Promise.resolve(tokenInfo.accessToken);
-
-      return tokenInfo.accessToken;
-    }).catch(function() {
-      showAccessTokenExpiredErrorMessage(
-        'You must reload the page to continue annotating.');
-      return null;
+      return tokenInfo;
     });
   }
 
-  /**
-   * Schedule a refresh of an access token a few minutes before it expires.
-   *
-   * @param {TokenInfo} tokenInfo
-   * @param {RefreshOptions} options
-   */
-  function refreshAccessTokenBeforeItExpires(tokenInfo, options) {
-    // The delay, in milliseconds, before we will poll again to see if it's
-    // time to refresh the access token.
-    var delay = 30000;
-
-    // If the token info's refreshAfter time will have passed before the next
-    // time we poll, then refresh the token this time.
-    var refreshAfter = tokenInfo.expiresAt - delay;
-
-    function refreshAccessTokenIfNearExpiry() {
-      if (Date.now() > refreshAfter) {
-        refreshAccessToken(tokenInfo.refreshToken, {
-          persist: options.persist,
-        });
-      } else {
-        refreshAccessTokenBeforeItExpires(tokenInfo, options);
-      }
-    }
-
-    refreshTimer = $window.setTimeout(refreshAccessTokenIfNearExpiry, delay);
-  }
-
   /**
    * Retrieve an access token for the API.
    *
    * @return {Promise<string>} The API access token.
    */
   function tokenGetter() {
-    if (!accessTokenPromise) {
+    if (!tokenInfoPromise) {
       var grantToken = grantTokenFromHostPage();
 
       if (grantToken) {
         // Exchange host-page provided grant token for a new access token.
-        accessTokenPromise = exchangeJWT(grantToken).then((tokenInfo) => {
-          refreshAccessTokenBeforeItExpires(tokenInfo, { persist: false });
-          return tokenInfo.accessToken;
+        tokenInfoPromise = exchangeJWT(grantToken).then((tokenInfo) => {
+          return tokenInfo;
         }).catch(function(err) {
           showAccessTokenExpiredErrorMessage(
             'You must reload the page to annotate.');
@@ -255,31 +225,44 @@ function auth($http, $window, flash, localStorage, random, settings) {
       } else if (authCode) {
         // Exchange authorization code retrieved from login popup for a new
         // access token.
-        accessTokenPromise = exchangeAuthCode(authCode).then((tokenInfo) => {
+        tokenInfoPromise = exchangeAuthCode(authCode).then((tokenInfo) => {
           saveToken(tokenInfo);
-          refreshAccessTokenBeforeItExpires(tokenInfo, { persist: true });
-          return tokenInfo.accessToken;
+          return tokenInfo;
         });
       } else {
         // Attempt to load the tokens from the previous session.
-        var tokenInfo = loadToken();
-        if (!tokenInfo) {
-          // No token. The user will need to log in.
-          accessTokenPromise = Promise.resolve(null);
-        } else if (Date.now() > tokenInfo.expiresAt) {
-          // Token has expired. Attempt to refresh it.
-          accessTokenPromise = refreshAccessToken(tokenInfo.refreshToken, {
-            persist: true,
-          });
-        } else {
-          // Token still valid, but schedule a refresh.
-          refreshAccessTokenBeforeItExpires(tokenInfo, { persist: true });
-          accessTokenPromise = Promise.resolve(tokenInfo.accessToken);
-        }
+        tokenInfoPromise = Promise.resolve(loadToken());
       }
     }
 
-    return accessTokenPromise;
+    var origToken = tokenInfoPromise;
+
+    return tokenInfoPromise.then(token => {
+      if (!token) {
+        // No token available. User will need to log in.
+        return null;
+      }
+
+      if (origToken !== tokenInfoPromise) {
+        // A token refresh has been initiated via a call to `refreshAccessToken`
+        // below since `tokenGetter()` was called.
+        return tokenGetter();
+      }
+
+      if (Date.now() > token.expiresAt) {
+        // Token expired. Attempt to refresh.
+        tokenInfoPromise = refreshAccessToken(token.refreshToken, {
+          persist: true,
+        }).catch(() => {
+          // If refreshing the token fails, the user is simply logged out.
+          return null;
+        });
+
+        return tokenGetter();
+      } else {
+        return token.accessToken;
+      }
+    });
   }
 
   /**
@@ -288,7 +271,7 @@ function auth($http, $window, flash, localStorage, random, settings) {
   function clearCache() {
     // Once cookie auth has been removed, the `clearCache` method can be removed
     // from the public API of this service in favor of `logout`.
-    accessTokenPromise = Promise.resolve(null);
+    tokenInfoPromise = Promise.resolve(null);
     localStorage.removeItem(storageKey());
     $window.clearTimeout(refreshTimer);
   }
@@ -355,7 +338,7 @@ function auth($http, $window, flash, localStorage, random, settings) {
       // Save the auth code. It will be exchanged for an access token when the
       // next API request is made.
       authCode = resp.code;
-      accessTokenPromise = null;
+      tokenInfoPromise = null;
     });
   }
 
@@ -365,9 +348,9 @@ function auth($http, $window, flash, localStorage, random, settings) {
    * This revokes and then forgets any OAuth credentials that the user has.
    */
   function logout() {
-    return accessTokenPromise.then(accessToken => {
+    return tokenInfoPromise.then(token => {
       return formPost(settings.oauthRevokeUrl, {
-        token: accessToken,
+        token: token.accessToken,
       });
     }).then(() => {
       clearCache();

src/sidebar/test/oauth-auth-test.js

@@ -208,22 +208,27 @@ describe('sidebar.oauth-auth', function () {
     // the pending Promise for the first request again (and not send a second
     // concurrent HTTP request).
     it('should not make two concurrent access token requests', function () {
-      makeServerUnresponsive();
+      var respond;
+      fakeHttp.post.returns(new Promise(resolve => {
+        respond = resolve;
+      }));
 
       // The first time tokenGetter() is called it sends the access token HTTP
       // request and returns a Promise for the access token.
-      var firstAccessTokenPromise = auth.tokenGetter();
-
-      // No matter how many times it's called while there's an HTTP request
-      // in-flight, tokenGetter() never sends a second concurrent HTTP request.
-      auth.tokenGetter();
-      auth.tokenGetter();
+      var tokens = [auth.tokenGetter(), auth.tokenGetter()];
 
-      // It just keeps on returning the same Promise for the access token.
-      var accessTokenPromise = auth.tokenGetter();
-
-      assert.strictEqual(accessTokenPromise, firstAccessTokenPromise);
       assert.equal(fakeHttp.post.callCount, 1);
+
+      // Resolve the initial request for an access token in exchange for a JWT.
+      respond({
+        status: 200,
+        data: {
+          access_token: 'foo',
+          refresh_token: 'bar',
+          expires_in: 3600,
+        },
+      });
+      return Promise.all(tokens);
     });
 
     it('should return null if no grant token was provided', function () {
@@ -234,7 +239,7 @@ describe('sidebar.oauth-auth', function () {
       });
     });
 
-    it('should refresh the access token before it expires', function () {
+    it('should refresh the access token if it expired', function () {
       function callTokenGetter () {
         var tokenPromise = auth.tokenGetter();
 
@@ -264,49 +269,42 @@ describe('sidebar.oauth-auth', function () {
         };
       }
 
-      function assertThatTokenGetterNowReturnsNewAccessToken () {
-        return auth.tokenGetter().then(function (token) {
-          assert.equal(token, 'secondAccessToken');
-        });
-      }
-
       return callTokenGetter()
         .then(resetHttpSpy)
         .then(expireAccessToken)
-        .then(assertRefreshTokenWasUsed('firstRefreshToken'))
-        .then(resetHttpSpy)
-        .then(assertThatTokenGetterNowReturnsNewAccessToken)
-        .then(expireAccessToken)
-        .then(assertRefreshTokenWasUsed('secondRefreshToken'));
-    });
-
-    // While a refresh token HTTP request is in-flight, calls to tokenGetter()
-    // should just return the old access token immediately.
-    it('returns the access token while a refresh is in-flight', function() {
-      return auth.tokenGetter().then(function(firstAccessToken) {
-        makeServerUnresponsive();
-
-        expireAccessToken();
-
-        // The refresh token request will still be in-flight, but tokenGetter()
-        // should still return a Promise for the old access token.
-        return auth.tokenGetter().then(function(secondAccessToken) {
-          assert.equal(firstAccessToken, secondAccessToken);
-        });
-      });
+        .then(() => auth.tokenGetter())
+        .then(token => assert.equal(token, 'secondAccessToken'))
+        .then(assertRefreshTokenWasUsed('firstRefreshToken'));
     });
 
     // It only sends one refresh request, even if tokenGetter() is called
     // multiple times and the refresh response hasn't come back yet.
     it('does not send more than one refresh request', function () {
+      // Perform an initial token fetch which will exchange the JWT grant for an
+      // access token.
       return auth.tokenGetter()
-        .then(resetHttpSpy) // Reset fakeHttp.post.callCount to 0 so that the
-                            // initial access token request isn't counted.
-        .then(auth.tokenGetter)
-        .then(makeServerUnresponsive)
-        .then(auth.tokenGetter)
-        .then(expireAccessToken)
-        .then(function () {
+        .then(() => {
+          // Expire the access token to trigger a refresh request on the next
+          // token fetch.
+          fakeHttp.post.reset();
+          expireAccessToken();
+
+          // Delay the response to the refresh request.
+          var respond;
+          fakeHttp.post.returns(new Promise(resolve => {
+            respond = resolve;
+          }));
+
+          // Request an auth token multiple times.
+          var tokens = Promise.all([auth.tokenGetter(), auth.tokenGetter()]);
+
+          // Finally, respond to the refresh request.
+          respond({ access_token: 'a_new_token', refresh_token: 'a_delayed_token', expires_in: 3600 });
+
+          return tokens;
+        })
+        .then(() => {
+          // Check that only one refresh request was made.
           assert.equal(fakeHttp.post.callCount, 1);
         });
     });
@@ -321,19 +319,12 @@ describe('sidebar.oauth-auth', function () {
         };
       });
 
-      it('shows an error message to the user', function () {
-        function assertThatErrorMessageWasShown() {
-          assert.calledOnce(fakeFlash.error);
-          assert.equal(
-            fakeFlash.error.firstCall.args[0],
-            'You must reload the page to continue annotating.'
-          );
-        }
+      it('logs the user out', function () {
+        expireAccessToken();
 
-        return auth.tokenGetter()
-          .then(expireAccessToken)
-          .then(function () { clock.tick(1); })
-          .then(assertThatErrorMessageWasShown);
+        return auth.tokenGetter(token => {
+          assert.equal(token, null);
+        });
       });
     });
   });
@@ -350,7 +341,7 @@ describe('sidebar.oauth-auth', function () {
         assert.calledWith(fakeLocalStorage.setObject, TOKEN_KEY, {
           accessToken: 'firstAccessToken',
           refreshToken: 'firstRefreshToken',
-          expiresAt: 910000,
+          expiresAt: 990000,
         });
       });
     });
@@ -377,7 +368,7 @@ describe('sidebar.oauth-auth', function () {
         assert.calledWith(fakeLocalStorage.setObject, TOKEN_KEY, {
           accessToken: 'secondToken',
           refreshToken: 'secondRefreshToken',
-          expiresAt: 1910000,
+          expiresAt: 1990000,
         });
       });
     });
@@ -421,7 +412,7 @@ describe('sidebar.oauth-auth', function () {
           {
             accessToken: 'secondToken',
             refreshToken: 'secondRefreshToken',
-            expiresAt: 910200,
+            expiresAt: 990200,
           }
         );
       });
@@ -599,12 +590,6 @@ describe('sidebar.oauth-auth', function () {
     clock.tick(DEFAULT_TOKEN_EXPIRES_IN_SECS * 1000);
   }
 
-  // Make $http.post() return a pending Promise (simulates a still in-flight
-  // HTTP request).
-  function makeServerUnresponsive () {
-    fakeHttp.post.returns(new Promise(function () {}));
-  }
-
   // Reset fakeHttp's spy history (.called, .callCount, etc).
   function resetHttpSpy () {
     fakeHttp.post.resetHistory();