Remove auth => session dependency

Simplify the "auth" service and remove the dependency on the
"session" service. This will make it possible to introduce a "store" =>
"session" dependency in future in order to support fetching the user's
profile from the access token-authenticated /api/profile endpoint
instead of the cookie-authenticated /app endpoint.

The 'auth' service depended on 'session' for three things:

 - Being able to call `session.load()` in order to retrieve a CSRF
   token. This token is not needed for the `GET /api/token` endpoint
   following https://github.com/hypothesis/h/pull/4322

 - Calling `session.logout()`. This is fixed by removing the
   `auth.logout()` endpoint and changing the caller to call
   `session.logout()` directly instead. `session.logout()` in turn
   calls `auth.clearCache()` to clear cached API tokens.

 - Determining the current user ID in order to invalidate
   the cached token when that changes. The logic to clear the
   cache has instead been moved to the session service.

This commit also adds additional tests for session logout.

src/sidebar/app-controller.js

@@ -132,7 +132,7 @@ module.exports = function AppController(
     });
     drafts.discard();
     $scope.accountDialog.visible = false;
-    auth.logout();
+    session.logout();
   };
 
   $scope.clearSelection = function () {

src/sidebar/auth.js

 'use strict';
 
-var INITIAL_TOKEN = {
-  // The user ID which the current cached token is valid for
-  userid: undefined,
-  // Promise for the API token for 'userid'.
-  // This is initialized when fetchOrReuseToken() is called and
-  // reset when logging out via logout()
-  token: undefined,
-};
-
-/**
- * Fetches a new API token for the current logged-in user.
- *
- * @return {Promise} - A promise for a new JWT token.
- */
-function fetchToken($http, session, settings) {
-  var tokenUrl = new URL('token', settings.apiUrl).href;
-
-  // Explicitly include the CSRF token in the headers. This won't be done
-  // automatically in the extension as this will be a cross-domain request, and
-  // Angular's CSRF support doesn't operate automatically cross-domain.
-  var headers = {};
-  headers[$http.defaults.xsrfHeaderName] = session.state.csrf;
-
-  var config = {
-    headers: headers,
-    // Skip JWT authorization for the token request itself.
-    skipAuthorization: true,
-  };
-
-  return $http.get(tokenUrl, config).then(function (response) {
-    return response.data;
-  });
-}
+var NULL_TOKEN = Promise.resolve(null);
 
 /**
  * Service for fetching and caching access tokens for the Hypothesis API.
  */
 // @ngInject
-function auth($http, flash, jwtHelper, session, settings) {
+function auth($http, jwtHelper, settings) {
 
-  var cachedToken = INITIAL_TOKEN;
+  var cachedToken = NULL_TOKEN;
 
   /**
-   * Fetches or returns a cached JWT API token for the current user.
+   * Fetch a new API token for the current logged-in 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;
-      });
-    }
-
-    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;
-  }
-
-  /**
-   * Log out from the API and clear any cached tokens.
+   * The user is authenticated using their session cookie.
    *
-   * @return {Promise<void>} - A promise for when logout has completed.
+   * @return {Promise<string>} - A promise for a new JWT token.
    */
-  function logout() {
-    return session.logout({}).$promise
-      .then(function() {
-        clearCache();
-      })
-      .catch(function(err) {
-        flash.error('Log out failed!');
-        throw err;
-      });
+  function fetchToken() {
+    var tokenUrl = new URL('token', settings.apiUrl).href;
+    return $http.get(tokenUrl, {}).then(function (response) {
+      return response.data;
+    });
   }
 
   /**
-   * Return an access token for authenticating API requests.
+   * Fetch or return a cached JWT API token for the current user.
    *
-   * @return {Promise<string>}
+   * @return {Promise<string>} - A promise for a JWT API token for the current
+   *                             user.
    */
   function tokenGetter() {
-    return fetchOrReuseToken($http, jwtHelper, session, settings);
+    return cachedToken.then(function (token) {
+      if (!token || jwtHelper.isTokenExpired(token)) {
+        cachedToken = fetchToken();
+        return cachedToken;
+      } else {
+        return token;
+      }
+    });
+  }
+
+  function clearCache() {
+    cachedToken = NULL_TOKEN;
   }
 
   return {
     clearCache: clearCache,
     tokenGetter: tokenGetter,
-    logout: logout,
   };
 }
 

src/sidebar/session.js

@@ -48,7 +48,8 @@ function sessionActions(options) {
  *
  * @ngInject
  */
-function session($http, $resource, $rootScope, annotationUI, flash, raven, settings) {
+function session($http, $resource, $rootScope, annotationUI, auth,
+                 flash, raven, settings) {
   // Headers sent by every request made by the session service.
   var headers = {};
   var actions = sessionActions({
@@ -104,7 +105,7 @@ function session($http, $resource, $rootScope, annotationUI, flash, raven, setti
    *              the response, update() can be used to update the client
    *              when new state has been pushed to it by the server.
    */
-  resource.update = function (model) {
+  function update(model) {
     var prevSession = annotationUI.getState().session;
 
     var isInitialLoad = !prevSession.csrf;
@@ -128,6 +129,8 @@ function session($http, $resource, $rootScope, annotationUI, flash, raven, setti
     });
 
     if (userChanged) {
+      auth.clearCache();
+
       $rootScope.$broadcast(events.USER_CHANGED, {
         initialLoad: isInitialLoad,
         userid: model.userid,
@@ -151,7 +154,7 @@ function session($http, $resource, $rootScope, annotationUI, flash, raven, setti
 
     // Return the model
     return model;
-  };
+  }
 
   function process(data, headersGetter, status) {
     if (status < 200 || status >= 500) {
@@ -179,14 +182,23 @@ function session($http, $resource, $rootScope, annotationUI, flash, raven, setti
       }
     }
 
-    return resource.update(model);
+    return update(model);
+  }
+
+  function logout() {
+    return resource.logout().$promise.then(function () {
+      auth.clearCache();
+    }).catch(function (err) {
+      flash.error('Log out failed');
+      throw err;
+    });
   }
 
   return {
     dismissSidebarTutorial: resource.dismiss_sidebar_tutorial,
     load: resource.load,
     login: resource.login,
-    logout: resource.logout,
+    logout: logout,
 
     // For the moment, we continue to expose the session state as a property on
     // this service. In future, other services which access the session state
@@ -195,7 +207,7 @@ function session($http, $resource, $rootScope, annotationUI, flash, raven, setti
       return annotationUI.getState().session;
     },
 
-    update: resource.update,
+    update: update,
   };
 }
 

src/sidebar/test/app-controller-test.js

@@ -58,9 +58,7 @@ describe('AppController', function () {
       clearSelectedAnnotations: sandbox.spy(),
     };
 
-    fakeAuth = {
-      logout: sandbox.stub().returns(Promise.resolve()),
-    };
+    fakeAuth = {};
 
     fakeDrafts = {
       contains: sandbox.stub(),
@@ -88,6 +86,7 @@ describe('AppController', function () {
 
     fakeSession = {
       load: sandbox.stub().returns(Promise.resolve({userid: null})),
+      logout: sandbox.stub(),
     };
 
     fakeGroups = {focus: sandbox.spy()};
@@ -254,7 +253,13 @@ describe('AppController', function () {
     });
   });
 
-  describe('logout()', function () {
+  describe('#logout()', function () {
+    it('calls session.logout()', function () {
+      createController();
+      $scope.logout();
+      assert.called(fakeSession.logout);
+    });
+
     it('prompts the user if there are drafts', function () {
       fakeDrafts.count.returns(1);
       createController();

src/sidebar/test/auth-test.js

@@ -6,20 +6,15 @@ describe('auth', function () {
   var fakeHttp;
   var fakeJwtHelper;
   var fakeSettings;
-  var fakeSession;
   var fakeTokens = ['token-one', 'token-two'];
   var fakeTokenIndex;
 
   beforeEach(function () {
     fakeTokenIndex = 0;
     fakeHttp = {
-      defaults: {xsrfHeaderName: 'X-CSRF-Token'},
       get: sinon.spy(function (url, config) {
-        assert.equal(config.skipAuthorization, true);
         assert.equal(url, 'https://test.hypothes.is/api/token');
-        assert.deepEqual(config.headers, {
-          'X-CSRF-Token': fakeSession.state.csrf,
-        });
+        assert.deepEqual(config, {});
 
         var result = {status: 200, data: fakeTokens[fakeTokenIndex]};
         ++fakeTokenIndex;
@@ -27,29 +22,16 @@ describe('auth', function () {
       }),
     };
     fakeJwtHelper = {isTokenExpired: sinon.stub()};
-    fakeSession = {
-      load: sinon.spy(function () {
-        return Promise.resolve(fakeSession.state);
-      }),
-      logout: sinon.spy(function () {
-        return {$promise: Promise.resolve()};
-      }),
-      state: {
-        csrf: 'fake-csrf-token',
-      },
-    };
     fakeSettings = {
       apiUrl: 'https://test.hypothes.is/api/',
     };
   });
 
   function authFactory() {
-    var fakeFlash = { error: sinon.stub() };
-    return auth(fakeHttp, fakeFlash, fakeJwtHelper, fakeSession, fakeSettings);
+    return auth(fakeHttp, fakeJwtHelper, fakeSettings);
   }
 
   describe('#tokenGetter', function () {
-
     it('should fetch and return a new token', function () {
       var auth = authFactory();
       return auth.tokenGetter().then(function (token) {
@@ -80,11 +62,13 @@ describe('auth', function () {
         assert.equal(token, fakeTokens[1]);
       });
     });
+  });
 
-    it('should fetch a new token if the userid changes', function () {
+  describe('#clearCache', function () {
+    it('should remove existing cached tokens', function () {
       var auth = authFactory();
       return auth.tokenGetter().then(function () {
-        fakeSession.state.userid = 'new-user-id';
+        auth.clearCache();
         return auth.tokenGetter();
       }).then(function (token) {
         assert.calledTwice(fakeHttp.get);
@@ -92,13 +76,4 @@ describe('auth', function () {
       });
     });
   });
-
-  describe('#logout', function () {
-    it('should call session.logout', function () {
-      var auth = authFactory();
-      return auth.logout().then(function () {
-        assert.called(fakeSession.logout);
-      });
-    });
-  });
 });

src/sidebar/test/session-test.js

@@ -6,10 +6,11 @@ var events = require('../events');
 
 var mock = angular.mock;
 
-describe('h:session', function () {
+describe('session', function () {
   var $httpBackend;
   var $rootScope;
 
+  var fakeAuth;
   var fakeFlash;
   var fakeRaven;
   var sandbox;
@@ -32,6 +33,9 @@ describe('h:session', function () {
         state = session;
       },
     };
+    fakeAuth = {
+      clearCache: sandbox.spy(),
+    };
     fakeFlash = {error: sandbox.spy()};
     fakeRaven = {
       setUserInfo: sandbox.spy(),
@@ -39,6 +43,7 @@ describe('h:session', function () {
 
     mock.module('h', {
       annotationUI: fakeAnnotationUI,
+      auth: fakeAuth,
       flash: fakeFlash,
       raven: fakeRaven,
       settings: {
@@ -237,6 +242,11 @@ describe('h:session', function () {
       assert.calledOnce(userChangeCallback);
     });
 
+    it('clears the API token cache when the user changes', function () {
+      session.update({userid: 'different-user', csrf: 'dummytoken'});
+      assert.called(fakeAuth.clearCache);
+    });
+
     it('updates the user ID for Sentry error reports', function () {
       session.update({
         userid: 'anne',
@@ -256,4 +266,29 @@ describe('h:session', function () {
       $httpBackend.flush();
     });
   });
+
+  describe('#logout()', function () {
+    beforeEach(function () {
+      var url = 'https://test.hypothes.is/root/app?__formid__=logout';
+      $httpBackend.expectPOST(url).respond(200, {
+        model: {
+          userid: 'logged-out-id',
+        },
+      });
+    });
+
+    it('logs the user out on the service and updates the session', function () {
+      session.logout().then(function () {
+        assert.equal(session.state.userid, 'logged-out-id');
+      });
+      $httpBackend.flush();
+    });
+
+    it('clears the API access token cache', function () {
+      session.logout().then(function () {
+        assert.called(fakeAuth.clearCache);
+      });
+      $httpBackend.flush();
+    });
+  });
 });