Refactor auth in the client #2415

Merged
merged 1 commit into from Aug 3, 2015

Projects

None yet

3 participants

@tilgovi
Contributor
tilgovi commented Jul 30, 2015

Start to take better control over the authentication and authorization
systems of the front end.

  • Remove dependency on Annotator Auth plugin. With this change,
    Annotator is completely gone from the Angular application.
  • Use angular-jwt and its http interceptor to ensure that auth tokens
    are up-to-date and auth token requests block store requests.
  • With the interceptor in place, it's no longer necessary to resolve
    auth before the ng-view controllers.
  • Use the session directly for getting the current user. This is in
    line with what is likely to happen for groups, too, soon.
@landscape-bot

Code Health
Code quality remained the same when pulling 580e33b on hypothesis:client-auth-refactor into 3920527 on hypothesis:master.

@nickstenning nickstenning commented on an outdated diff Jul 31, 2015
h/static/scripts/app-controller.coffee
@@ -68,6 +80,8 @@ module.exports = class AppController
else
$scope.$emit('annotationDeleted', container.message)
+ identity.watch({onlogin, onlogout, onready})
@nickstenning
nickstenning Jul 31, 2015 Member

Suggest you move this up, right under the definitions of onlog{in, out}..., so it's immediately clear why they're being defined and what they're used for. Or, frankly, given the size of them:

identity.watch({
  onlogin: (identity) -> $scope.auth.user = auth.userid(identity)
  onlogout: -> $scope.auth.user = null
  onready: -> $scope.auth.user ?= null
})
@nickstenning nickstenning commented on the diff Jul 31, 2015
h/static/scripts/auth.js
+/**
+ * @ngdoc service
+ * @name auth
+ *
+ * @description
+ * The 'auth' service exposes authorization helpers for other components.
+ */
+
+Auth.$inject = ['jwtHelper'];
+function Auth ( jwtHelper ) {
+ this.userid = function userid(identity) {
+ try {
+ if (jwtHelper.isTokenExpired(identity)) {
+ return null;
+ } else {
+ var payload = jwtHelper.decodeToken(identity);
@nickstenning
nickstenning Jul 31, 2015 Member

Eww to an API that let's you decode an expired token without massive flashing red lights, but hey -- you work with what you're given.

@nickstenning nickstenning commented on an outdated diff Jul 31, 2015
h/static/scripts/config/identity.js
@@ -0,0 +1,139 @@
+var JWT_BEARER = 'urn:ietf:params:oauth:grant-type:jwt-bearer';
+
+var _authPromise = null;
+var _tokenPromise = null;
+var _tokenResponse = null;
+
+
+fetchToken.$inject = ['$http', 'jwtHelper', 'serviceUrl', 'session']
+function fetchToken ( $http, jwtHelper, serviceUrl, session ) {
+ var tokenUrl = new URL('token', serviceUrl).href;
+ var token = (_tokenResponse === null) ? null : _tokenResponse;
@nickstenning
nickstenning Jul 31, 2015 Member

This is semantically identical to var token = _tokenResponse; :)

@nickstenning nickstenning commented on an outdated diff Jul 31, 2015
h/static/scripts/config/identity.js
@@ -0,0 +1,139 @@
+var JWT_BEARER = 'urn:ietf:params:oauth:grant-type:jwt-bearer';
+
+var _authPromise = null;
@nickstenning
nickstenning Jul 31, 2015 Member

Given this is a browserify module, there's no particular need to give these "private" names.

@nickstenning nickstenning commented on an outdated diff Jul 31, 2015
h/static/scripts/config/identity.js
@@ -0,0 +1,139 @@
+var JWT_BEARER = 'urn:ietf:params:oauth:grant-type:jwt-bearer';
+
+var _authPromise = null;
+var _tokenPromise = null;
+var _tokenResponse = null;
@nickstenning
nickstenning Jul 31, 2015 Member

Given how you use this later on, perhaps it should just be var currentToken?

@nickstenning nickstenning commented on an outdated diff Jul 31, 2015
h/static/scripts/config/identity.js
+}
+
+
+configureIdentity.$inject = ['identityProvider', 'jwtInterceptorProvider'];
+function configureIdentity ( identityProvider, jwtInterceptorProvider ) {
+
+ identityProvider.checkAuthentication = [
+ '$injector', '$q', 'session', function($injector, $q, session) {
+ if (_authPromise === null) {
+ var deferred = $q.defer();
+ _authPromise = deferred.promise;
+
+ session.load().$promise
+ .then(function (data) {
+ if (data.userid) {
+ $q.when($injector.invoke(fetchToken))
@nickstenning
nickstenning Jul 31, 2015 Member

This could be simplified by having fetchToken return a promise even when it has a token already, i.e. return Promise.resolve(token).

@nickstenning
nickstenning Jul 31, 2015 Member

In general, if a thing can return a promise, it should probably always return a promise: https://www.w3.org/2001/tag/doc/promises-guide

@nickstenning nickstenning commented on the diff Jul 31, 2015
h/static/scripts/directive/annotation.coffee
@@ -108,7 +108,12 @@ AnnotationController = [
###
this.authorize = (action) ->
return false unless model?
- permissions.permits action, model, auth.user
+ # TODO: this should use auth instead of permissions but we might need
+ # an auth cache or the JWT -> userid decoding might start to be a
+ # performance bottleneck and we would need to get the id token into the
+ # session, which we should probably do anyway (and move to opaque bearer
+ # tokens for the access token).
+ return permissions.permits action, model, session.state.userid
@nickstenning
nickstenning Jul 31, 2015 Member

Not sure I understand this. auth.user is computed once on auth change, not on every access.

@tilgovi
tilgovi Jul 31, 2015 Contributor

auth.user doesn't exist anymore. It's auth.userid(identity).

@nickstenning
nickstenning Aug 3, 2015 Member

user doesn't exist as a property of the auth session, no. I was referring to $scope.auth.user.

@nickstenning
Member

In general this looks a lot better, but it's still confusing. In particular, there are two different things going on here:

  1. We need an access token which we send to the API with all requests. For that, I like the interceptor pattern.
  2. We need to know some things about the currently authenticated user (such as their userid). Here we're still confused -- some things are using session.state.userid and some things are using auth.user.

As things stand right now, I don't see any reason to try and derive the userid from the JWT. Given that we exchange a cookie for the JWT, and the same cookie also gives us access to the "session" (the /app route), why can't we just remove everything to do with extracting the userid from the token and use session.state.userid exclusively for access to the authenticated userid.

@nickstenning nickstenning reopened this Jul 31, 2015
@nickstenning
Member

Keyboard fail. Sorry!

@tilgovi
Contributor
tilgovi commented Jul 31, 2015

To answer your question "why can't we just remove everything to do with extracting the userid from the token":

I think I'd like to keep this as is right now.

The confusion comes from the fact that the onlogin callback in AppController isn't updating the session because the session has no client-side storage API.

I would much prefer we have a client-side session. Mostly, I don't want to rely on us making credentialed requests to get session state. So, while I'm happy to read from the session whenever we need the userid elsewhere, I'd prefer not to remove the callbacks and the identity->auth flow that AppController does. Next, hopefully, we can populate the session there.

In the meantime, though, we're storing only the userid in the session, not the identity. That's what my comment in AnnotationController is getting at. I'd like to get to a place where we have a session that can store one or more identities. The shape of these is TDB, but I'm thinking it'll take the form of an access token (not necessarily a JWT) and we may wish to make a user info request during authentication in order to populate a cache used by auth.userid(access_token) or some such thing.

@tilgovi
Contributor
tilgovi commented Jul 31, 2015

Thanks for the really helpful comments. Good catch on everything.

Rebased and fixed with all your recommendations.

@tilgovi tilgovi Refactor auth in the client
Start to take better control over the authentication and authorization
systems of the front end.

- Remove dependency on Annotator Auth plugin. With this change,
  Annotator is completely gone from the Angular application.

- Use angular-jwt and its http interceptor to ensure that auth tokens
  are up-to-date and auth token requests block store requests.

- With the interceptor in place, it's no longer necessary to resolve
  auth before the ng-view controllers.

- Use the session directly for getting the current user. This is in
  line with what is likely to happen for groups, too, soon.
ab8d52e
@landscape-bot

Code Health
Code quality remained the same when pulling ab8d52e on hypothesis:client-auth-refactor into 8725c40 on hypothesis:master.

@nickstenning nickstenning commented on the diff Aug 3, 2015
h/static/scripts/app.coffee
@@ -123,7 +117,6 @@ module.exports = angular.module('h', [
.provider('identity', require('./identity'))
-.service('annotator', -> new Annotator(angular.element('<div>')))
@nickstenning
Member

For the sake of moving things along, I'm going to merge this. Could you take a look at writing some tests for h/static/scripts/config/identity.js, though? It's got some pretty nested conditional logic handling token promises and requests -- some tests would help clarify intended behaviour.

@nickstenning nickstenning merged commit db221d3 into master Aug 3, 2015

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 63.181%
Details
@nickstenning nickstenning deleted the client-auth-refactor branch Aug 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment