Merge pull request #191 from hypothesis/remove-jwt-interceptor

Explicitly add Authorization header to API requests

src/sidebar/app.js

@@ -83,12 +83,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
@@ -166,7 +163,7 @@ module.exports = angular.module('h', [
   .service('analytics', require('./analytics'))
   .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

@@ -78,17 +78,33 @@ function serializeParams(params) {
  * Creates a function that will make an API call to a named route.
  *
  * @param $http - The Angular HTTP service
+ * @param $q - The Angular Promises ($q) service.
  * @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, $q, links, route, tokenGetter) {
   return function (params, data) {
-    return links.then(function (links) {
+    // `$q.all` is used here rather than `Promise.all` because testing code that
+    // mixes native Promises with the `$q` promises returned by `$http`
+    // functions gets awkward in tests.
+    return $q.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 +124,24 @@ function createAPICall($http, links, route) {
  * the Hypothesis API (see http://h.readthedocs.io/en/latest/api/).
  */
 // @ngInject
-function store($http, settings) {
+function store($http, $q, auth, settings) {
   var links = retryUtil.retryPromiseOperation(function () {
     return $http.get(settings.apiUrl);
   }).then(function (response) {
     return response.data.links;
   });
 
+  function apiCall(route) {
+    return createAPICall($http, $q, links, route, auth.tokenGetter);
+  }
+
   return {
-    search: createAPICall($http, links, 'search'),
+    search: apiCall('search'),
     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: apiCall('annotation.create'),
+      delete: apiCall('annotation.delete'),
+      get: apiCall('annotation.read'),
+      update: apiCall('annotation.update'),
     },
   };
 }

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,23 @@ 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 fakeAuth = {};
+
+    angular.mock.module('h', {
+      auth: fakeAuth,
+      settings: {apiUrl: 'http://example.com/api'},
+    });
+
+    angular.mock.inject(function (_$q_) {
+      var $q = _$q_;
+      fakeAuth.tokenGetter = function () {
+        return $q.resolve('faketoken');
+      };
+    });
+  });
 
   afterEach(function () {
     $httpBackend.verifyNoOutstandingExpectation();
@@ -65,10 +76,12 @@ describe('store', function () {
     $httpBackend.flush();
   }));
 
-  it('saves a new annotation', function () {
+  it('saves a new annotation', function (done) {
     store.annotation.create({}, {}).then(function (saved) {
       assert.isNotNull(saved.id);
+      done();
     });
+
     $httpBackend.expectPOST('http://example.com/api/annotations')
       .respond(function () {
         return [201, {id: 'new-id'}, {}];
@@ -76,8 +89,11 @@ describe('store', function () {
     $httpBackend.flush();
   });
 
-  it('updates an annotation', function () {
-    store.annotation.update({id: 'an-id'}, {text: 'updated'});
+  it('updates an annotation', function (done) {
+    store.annotation.update({id: 'an-id'}, {text: 'updated'}).then(function () {
+      done();
+    });
+
     $httpBackend.expectPUT('http://example.com/api/annotations/an-id')
       .respond(function () {
         return [200, {}, {}];
@@ -85,8 +101,11 @@ describe('store', function () {
     $httpBackend.flush();
   });
 
-  it('deletes an annotation', function () {
-    store.annotation.delete({id: 'an-id'}, {});
+  it('deletes an annotation', function (done) {
+    store.annotation.delete({id: 'an-id'}, {}).then(function () {
+      done();
+    });
+
     $httpBackend.expectDELETE('http://example.com/api/annotations/an-id')
       .respond(function () {
         return [200, {}, {}];
@@ -94,24 +113,30 @@ describe('store', function () {
     $httpBackend.flush();
   });
 
-  it('removes internal properties before sending data to the server', function () {
+  it('removes internal properties before sending data to the server', function (done) {
     var annotation = {
       $highlight: true,
       $notme: 'nooooo!',
       allowed: 123,
     };
-    store.annotation.create({}, annotation);
+    store.annotation.create({}, annotation).then(function () {
+      done();
+    });
+
     $httpBackend.expectPOST('http://example.com/api/annotations', {
       allowed: 123,
     })
-      .respond(function () { return {id: 'test'}; });
+      .respond(function () { return [200, {id: 'test'}, {}]; });
     $httpBackend.flush();
   });
 
   // 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'});
+  it('encodes semicolons in query parameters', function (done) {
+    store.search({'uri': 'http://example.com/?foo=bar;baz=qux'}).then(function () {
+      done();
+    });
+
     $httpBackend.expectGET('http://example.com/api/search?uri=http%3A%2F%2Fexample.com%2F%3Ffoo%3Dbar%3Bbaz%3Dqux')
       .respond(function () { return [200, {}, {}]; });
     $httpBackend.flush();