Fetch access tokens and add Authorization header to API requests

Remove the global HTTP interceptor provided by angular-jwt which added
the Authorization header to API requests and replace it with explicit
logic in `createAPICall` to do the same thing.

This will enable replacing the JWT tokens with opaque access tokens when
using a publisher-provided grant token for authentication.

It also provides a more explicit way to only include the access token
with requests to the API, rather than filtering based on the URL prefix
of the request in the `tokenGetter` implementation.

 * Remove angular-jwt's HTTP interceptor and replace it with logic in
   store.js to explicitly fetch an access token using the `auth` module
   and add an Authorization header to API requests.

 * Convert standalone functions and global variables in auth.js to
   methods on the auth service. This will enable swapping out the
   current auth service implementation which uses cookies + CSRF
   for authentication with one that uses the OAuth grant token.

 * Fix several cases in store-test.js where functions that made
   assertions inside Promise callbacks did not explicitly wait for the
   Promise to resolve before finishing the test.

src/sidebar/app.js

@@ -79,12 +79,9 @@ function configureRoutes($routeProvider) {
 }
 
 // @ngInject
-function configureHttp($httpProvider, jwtInterceptorProvider) {
+function configureHttp($httpProvider) {
   // Use the Pyramid XSRF header name
   $httpProvider.defaults.xsrfHeaderName = 'X-CSRF-Token';
-  // Setup JWT tokens for API requests
-  $httpProvider.interceptors.push('jwtInterceptor');
-  jwtInterceptorProvider.tokenGetter = require('./auth').tokenGetter;
 }
 
 // @ngInject
@@ -161,7 +158,7 @@ module.exports = angular.module('h', [
 
   .service('annotationMapper', require('./annotation-mapper'))
   .service('annotationUI', require('./annotation-ui'))
-  .service('auth', require('./auth').service)
+  .service('auth', require('./auth'))
   .service('bridge', require('../shared/bridge'))
   .service('drafts', require('./drafts'))
   .service('features', require('./features'))

src/sidebar/auth.js

 'use strict';
 
-/**
- * Provides functions for retrieving and caching API tokens required by
- * API requests and logging out of the API.
- */
-
 var INITIAL_TOKEN = {
   // The user ID which the current cached token is valid for
   userid: undefined,
@@ -14,14 +9,11 @@ var INITIAL_TOKEN = {
   token: undefined,
 };
 
-var cachedToken = INITIAL_TOKEN;
-
 /**
  * Fetches a new API token for the current logged-in user.
  *
  * @return {Promise} - A promise for a new JWT token.
  */
-// @ngInject
 function fetchToken($http, session, settings) {
   var tokenUrl = new URL('token', settings.apiUrl).href;
 
@@ -43,69 +35,59 @@ function fetchToken($http, session, settings) {
 }
 
 /**
- * Fetches or returns a cached JWT API token for the current user.
- *
- * @return {Promise} - A promise for a JWT API token for the current
- *                     user.
+ * Service for fetching and caching access tokens for the Hypothesis API.
  */
 // @ngInject
-function fetchOrReuseToken($http, jwtHelper, session, settings) {
-  function refreshToken() {
-    return fetchToken($http, session, settings).then(function (token) {
-      return token;
-    });
-  }
+function auth($http, flash, jwtHelper, session, settings) {
 
-  var userid;
+  var cachedToken = INITIAL_TOKEN;
 
-  return session.load()
-    .then(function (data) {
-      userid = data.userid;
-      if (userid === cachedToken.userid && cachedToken.token) {
-        return cachedToken.token;
-      } else {
-        cachedToken = {
-          userid: userid,
-          token: refreshToken(),
-        };
-        return cachedToken.token;
-      }
-    })
-    .then(function (token) {
-      if (jwtHelper.isTokenExpired(token)) {
-        cachedToken = {
-          userid: userid,
-          token: refreshToken(),
-        };
-        return cachedToken.token;
-      } else {
+  /**
+   * Fetches or returns a cached JWT API token for the current user.
+   *
+   * @return {Promise} - A promise for a JWT API token for the current
+   *                     user.
+   */
+  // @ngInject
+  function fetchOrReuseToken($http, jwtHelper, session, settings) {
+    function refreshToken() {
+      return fetchToken($http, session, settings).then(function (token) {
         return token;
-      }
-    });
-}
+      });
+    }
 
-/**
- * JWT token fetcher function for use with 'angular-jwt'
- *
- * angular-jwt should be configured to use this function as its
- * tokenGetter implementation.
- */
-// @ngInject
-function tokenGetter($http, config, jwtHelper, session, settings) {
-  // Only send the token on requests to the annotation storage service
-  if (config.url.slice(0, settings.apiUrl.length) === settings.apiUrl) {
-    return fetchOrReuseToken($http, jwtHelper, session, settings);
-  } else {
-    return null;
+    var userid;
+
+    return session.load()
+      .then(function (data) {
+        userid = data.userid;
+        if (userid === cachedToken.userid && cachedToken.token) {
+          return cachedToken.token;
+        } else {
+          cachedToken = {
+            userid: userid,
+            token: refreshToken(),
+          };
+          return cachedToken.token;
+        }
+      })
+      .then(function (token) {
+        if (jwtHelper.isTokenExpired(token)) {
+          cachedToken = {
+            userid: userid,
+            token: refreshToken(),
+          };
+          return cachedToken.token;
+        } else {
+          return token;
+        }
+      });
   }
-}
 
-function clearCache() {
-  cachedToken = INITIAL_TOKEN;
-}
+  function clearCache() {
+    cachedToken = INITIAL_TOKEN;
+  }
 
-// @ngInject
-function authService(flash, session) {
   /**
    * Log out from the API and clear any cached tokens.
    *
@@ -122,13 +104,20 @@ function authService(flash, session) {
       });
   }
 
+  /**
+   * Return an access token for authenticating API requests.
+   *
+   * @return {Promise<string>}
+   */
+  function tokenGetter() {
+    return fetchOrReuseToken($http, jwtHelper, session, settings);
+  }
+
   return {
+    clearCache: clearCache,
+    tokenGetter: tokenGetter,
     logout: logout,
   };
 }
 
-module.exports = {
-  tokenGetter: tokenGetter,
-  clearCache: clearCache,
-  service: authService,
-};
+module.exports = auth;

src/sidebar/store.js

@@ -81,14 +81,26 @@ function serializeParams(params) {
  * @param links - Object or promise for an object mapping named API routes to
  *                URL templates and methods
  * @param route - The dotted path of the named API route (eg. `annotation.create`)
+ * @param {Function} tokenGetter - Function which returns a Promise for an
+ *                   access token for the API.
  */
-function createAPICall($http, links, route) {
+function createAPICall($http, links, route, tokenGetter) {
   return function (params, data) {
-    return links.then(function (links) {
+    return Promise.all([links, tokenGetter()]).then(function (linksAndToken) {
+      var links = linksAndToken[0];
+      var token = linksAndToken[1];
+
       var descriptor = get(links, route);
       var url = urlUtil.replaceURLParams(descriptor.url, params);
+      var headers = {};
+
+      if (token) {
+        headers.Authorization = 'Bearer ' + token;
+      }
+
       var req = {
         data: data ? stripInternalProperties(data) : null,
+        headers: headers,
         method: descriptor.method,
         params: url.params,
         paramSerializer: serializeParams,
@@ -108,20 +120,22 @@ function createAPICall($http, links, route) {
  * the Hypothesis API (see http://h.readthedocs.io/en/latest/api/).
  */
 // @ngInject
-function store($http, settings) {
+function store($http, auth, settings) {
   var links = retryUtil.retryPromiseOperation(function () {
     return $http.get(settings.apiUrl);
   }).then(function (response) {
     return response.data.links;
   });
 
+  var tokenGetter = auth.tokenGetter;
+
   return {
-    search: createAPICall($http, links, 'search'),
+    search: createAPICall($http, links, 'search', tokenGetter),
     annotation: {
-      create: createAPICall($http, links, 'annotation.create'),
-      delete: createAPICall($http, links, 'annotation.delete'),
-      get: createAPICall($http, links, 'annotation.read'),
-      update: createAPICall($http, links, 'annotation.update'),
+      create: createAPICall($http, links, 'annotation.create', tokenGetter),
+      delete: createAPICall($http, links, 'annotation.delete', tokenGetter),
+      get: createAPICall($http, links, 'annotation.read', tokenGetter),
+      update: createAPICall($http, links, 'annotation.update', tokenGetter),
     },
   };
 }

src/sidebar/test/auth-test.js

@@ -43,27 +43,25 @@ describe('auth', function () {
     };
   });
 
-  afterEach(function () {
-    auth.clearCache();
-  });
+  function authFactory() {
+    var fakeFlash = { error: sinon.stub() };
+    return auth(fakeHttp, fakeFlash, fakeJwtHelper, fakeSession, fakeSettings);
+  }
 
-  describe('tokenGetter', function () {
-    function tokenGetter() {
-      var config = {url:'https://test.hypothes.is/api/search'};
-      return auth.tokenGetter(fakeHttp, config, fakeJwtHelper,
-        fakeSession, fakeSettings);
-    }
+  describe('#tokenGetter', function () {
 
     it('should fetch and return a new token', function () {
-      return tokenGetter().then(function (token) {
+      var auth = authFactory();
+      return auth.tokenGetter().then(function (token) {
         assert.called(fakeHttp.get);
         assert.equal(token, fakeTokens[0]);
       });
     });
 
     it('should cache tokens for future use', function () {
-      return tokenGetter().then(function () {
-        return tokenGetter();
+      var auth = authFactory();
+      return auth.tokenGetter().then(function () {
+        return auth.tokenGetter();
       }).then(function (token) {
         assert.calledOnce(fakeHttp.get);
         assert.equal(token, fakeTokens[0]);
@@ -71,11 +69,12 @@ describe('auth', function () {
     });
 
     it('should refresh expired tokens', function () {
-      return tokenGetter().then(function () {
+      var auth = authFactory();
+      return auth.tokenGetter().then(function () {
         fakeJwtHelper.isTokenExpired = function () {
           return true;
         };
-        return tokenGetter();
+        return auth.tokenGetter();
       }).then(function (token) {
         assert.calledTwice(fakeHttp.get);
         assert.equal(token, fakeTokens[1]);
@@ -83,9 +82,10 @@ describe('auth', function () {
     });
 
     it('should fetch a new token if the userid changes', function () {
-      return tokenGetter().then(function () {
+      var auth = authFactory();
+      return auth.tokenGetter().then(function () {
         fakeSession.state.userid = 'new-user-id';
-        return tokenGetter();
+        return auth.tokenGetter();
       }).then(function (token) {
         assert.calledTwice(fakeHttp.get);
         assert.equal(token, fakeTokens[1]);
@@ -93,10 +93,10 @@ describe('auth', function () {
     });
   });
 
-  describe('.logout', function () {
+  describe('#logout', function () {
     it('should call session.logout', function () {
-      var fakeFlash = {error: sinon.stub()};
-      return auth.service(fakeFlash, fakeSession).logout().then(function () {
+      var auth = authFactory();
+      return auth.logout().then(function () {
         assert.called(fakeSession.logout);
       });
     });

src/sidebar/test/store-test.js

@@ -22,12 +22,44 @@ describe('store', function () {
       })));
   });
 
-  beforeEach(angular.mock.module('h'));
-
-  beforeEach(angular.mock.module(function ($provide) {
+  beforeEach(function () {
     sandbox = sinon.sandbox.create();
-    $provide.value('settings', {apiUrl: 'http://example.com/api'});
-  }));
+
+    var fakeTokenGetter = function () {
+      return Promise.resolve('faketoken');
+    };
+
+    angular.mock.module('h', {
+      auth: {
+        tokenGetter: fakeTokenGetter,
+      },
+      settings: {apiUrl: 'http://example.com/api'},
+    });
+  });
+
+  /**
+   * Wrapper around $httpBackend.flush() which allows for methods under test
+   * needing to fetch data using Promise-returning functions before they
+   * actually make an HTTP request.
+   *
+   * @param {number} [maxTicks] - Maximum number of event loop turns to wait.
+   */
+  function flushHttpRequests(maxTicks) {
+    try {
+      $httpBackend.flush();
+      return null;
+    } catch (err) {
+      if (maxTicks === 0) {
+        throw new Error('No HTTP request was made');
+      }
+      // No HTTP request was made yet. Try again on the next tick of the event
+      // loop.
+      maxTicks = maxTicks || 5;
+      return Promise.resolve().then(function () {
+        flushHttpRequests(maxTicks - 1);
+      });
+    }
+  }
 
   afterEach(function () {
     $httpBackend.verifyNoOutstandingExpectation();
@@ -66,32 +98,41 @@ describe('store', function () {
   }));
 
   it('saves a new annotation', function () {
-    store.annotation.create({}, {}).then(function (saved) {
+    var done = store.annotation.create({}, {}).then(function (saved) {
       assert.isNotNull(saved.id);
     });
+
     $httpBackend.expectPOST('http://example.com/api/annotations')
       .respond(function () {
         return [201, {id: 'new-id'}, {}];
       });
-    $httpBackend.flush();
+    flushHttpRequests();
+
+    return done;
   });
 
   it('updates an annotation', function () {
-    store.annotation.update({id: 'an-id'}, {text: 'updated'});
+    var done = store.annotation.update({id: 'an-id'}, {text: 'updated'});
+
     $httpBackend.expectPUT('http://example.com/api/annotations/an-id')
       .respond(function () {
         return [200, {}, {}];
       });
-    $httpBackend.flush();
+    flushHttpRequests();
+
+    return done;
   });
 
   it('deletes an annotation', function () {
-    store.annotation.delete({id: 'an-id'}, {});
+    var done = store.annotation.delete({id: 'an-id'}, {});
+
     $httpBackend.expectDELETE('http://example.com/api/annotations/an-id')
       .respond(function () {
         return [200, {}, {}];
       });
-    $httpBackend.flush();
+    flushHttpRequests();
+
+    return done;
   });
 
   it('removes internal properties before sending data to the server', function () {
@@ -100,20 +141,26 @@ describe('store', function () {
       $notme: 'nooooo!',
       allowed: 123,
     };
-    store.annotation.create({}, annotation);
+    var done = store.annotation.create({}, annotation);
+
     $httpBackend.expectPOST('http://example.com/api/annotations', {
       allowed: 123,
     })
-      .respond(function () { return {id: 'test'}; });
-    $httpBackend.flush();
+      .respond(function () { return [200, {id: 'test'}, {}]; });
+    flushHttpRequests();
+
+    return done;
   });
 
   // Our backend service interprets semicolons as query param delimiters, so we
   // must ensure to encode them in the query string.
   it('encodes semicolons in query parameters', function () {
-    store.search({'uri': 'http://example.com/?foo=bar;baz=qux'});
+    var done = store.search({'uri': 'http://example.com/?foo=bar;baz=qux'});
+
     $httpBackend.expectGET('http://example.com/api/search?uri=http%3A%2F%2Fexample.com%2F%3Ffoo%3Dbar%3Bbaz%3Dqux')
       .respond(function () { return [200, {}, {}]; });
-    $httpBackend.flush();
+    flushHttpRequests();
+
+    return done;
   });
 });