Merge pull request #1800 from hypothesis/fix-reply-permissions-simplified

Simplified auth plugin

h/static/scripts/account/account-controller.coffee

 class AccountController
-  @inject = ['$scope', '$filter', 'flash', 'session', 'identity', 'formHelpers']
-  constructor: ($scope, $filter, flash, session, identity, formHelpers) ->
+  @inject = [  '$scope', '$filter',
+               'auth', 'flash', 'formHelpers', 'identity', 'session']
+  constructor: ($scope,   $filter,
+                auth,   flash,   formHelpers,   identity,   session) ->
     persona_filter = $filter('persona')
     $scope.subscriptionDescription =
       reply: 'Receive notification emails when: - Someone replies to one of my annotations'
@@ -31,7 +33,7 @@ class AccountController
       $scope.$broadcast 'formState', form.$name, ''  # Update status btn
 
     $scope.tab = 'Account'
-    session.profile({user_id: $scope.persona}).$promise
+    session.profile({user_id: auth.user}).$promise
       .then (result) =>
         $scope.subscriptions = result.subscriptions
 
@@ -45,7 +47,7 @@ class AccountController
       # The extension is then removed from the page.
       # Confirmation of success is given.
       return unless form.$valid
-      username = persona_filter $scope.persona
+      username = persona_filter auth.user
       packet =
         username: username
         pwd: form.pwd.$modelValue
@@ -60,7 +62,7 @@ class AccountController
       formHelpers.applyValidationErrors(form)
       return unless form.$valid
 
-      username = persona_filter $scope.persona
+      username = persona_filter auth.user
       packet =
         username: username
         pwd: form.pwd.$modelValue
@@ -75,7 +77,7 @@ class AccountController
 
     $scope.updated = (index, form) ->
       packet =
-        username: $scope.persona
+        username: auth.user
         subscriptions: JSON.stringify $scope.subscriptions[index]
 
       successHandler = angular.bind(null, onSuccess, form)

h/static/scripts/auth-service.coffee

+###*
+# @ngdoc service
+# @name  Auth
+#
+# @description
+# The 'Auth' service exposes the currently logged in user for other components,
+# configures the Annotator.Auth plugin according to the login/logout events
+# and provides a method for permitting a certain operation for a user with a
+# given annotation
+###
+class Auth
+
+  this.$inject = ['$location', '$rootScope',
+                  'annotator', 'documentHelpers', 'identity']
+  constructor:   ( $location,   $rootScope,
+                   annotator,   documentHelpers,   identity) ->
+    {plugins} = annotator
+    _checkingToken = false
+    @user = undefined
+
+    # Fired when the identity-service successfully requests authentication.
+    # Sets up the Annotator.Auth plugin instance and after the plugin
+    # initialization it sets up an Annotator.Permissions plugin instance
+    # and finally it sets the auth.user property. It sets a flag between
+    # that time period to indicate that the token is being checked.
+    onlogin = (assertion) =>
+      _checkingToken = true
+
+      # Configure the Auth plugin with the issued assertion as refresh token.
+      annotator.addPlugin 'Auth',
+        tokenUrl: documentHelpers.absoluteURI(
+          "/api/token?assertion=#{assertion}")
+
+      # Set the user from the token.
+      plugins.Auth.withToken (token) =>
+        _checkingToken = false
+        annotator.addPlugin 'Permissions',
+          user: token.userId
+          userAuthorize: @permits
+        @user = token.userId
+        $rootScope.$apply()
+
+    # Fired when the identity-service forgets authentication.
+    # Destroys the Annotator.Auth and Permissions plugin instances and sets
+    # the user to null.
+    onlogout = =>
+      plugins.Auth?.element.removeData('annotator:headers')
+      plugins.Auth?.destroy()
+      delete plugins.Auth
+
+      plugins.Permissions?.setUser(null)
+      plugins.Permissions?.destroy()
+      delete plugins.Permissions
+
+      @user = null
+      _checkingToken = false
+
+    # Fired after the identity-service requested authentication (both after
+    # a failed or succeeded request). It detects if the first login request
+    # has failed and if yes, it sets the user value to null. (Otherwise the
+    # onlogin method would set it to userId)
+    onready = =>
+      if @user is undefined and not _checkingToken
+        @user = null
+
+    identity.watch {onlogin, onlogout, onready}
+
+
+  ###*
+  # @ngdoc method
+  # @name auth#permits
+  #
+  # @param {String} action action to authorize (read|update|delete|admin)
+  # @param {Object} annotation to permit action on it or not
+  # @param {String} user the userId
+  #
+  # User authorization function used by (not solely) the Permissions plugin
+  ###
+  permits: (action, annotation, user) ->
+    if annotation.permissions
+      tokens = annotation.permissions[action] || []
+
+      if tokens.length == 0
+        # Empty or missing tokens array: only admin can perform action.
+        return false
+
+      for token in tokens
+        if user == token
+          return true
+        if token == 'group:__world__'
+          return true
+
+      # No tokens matched: action should not be performed.
+      return false
+
+    # Coarse-grained authorization
+    else if annotation.user
+      return user and user == annotation.user
+
+    # No authorization info on annotation: free-for-all!
+    true
+
+angular.module('h')
+.service('auth', Auth)

h/static/scripts/controllers.coffee

-# User authorization function for the Permissions plugin.
-authorizeAction = (action, annotation, user) ->
-  if annotation.permissions
-    tokens = annotation.permissions[action] || []
-
-    if tokens.length == 0
-      # Empty or missing tokens array: only admin can perform action.
-      return false
-
-    for token in tokens
-      if user == token
-        return true
-      if token == 'group:__world__'
-        return true
-
-    # No tokens matched: action should not be performed.
-    return false
-
-  # Coarse-grained authorization
-  else if annotation.user
-    return user and user == annotation.user
-
-  # No authorization info on annotation: free-for-all!
-  true
-
-
 class AppController
   this.$inject = [
     '$location', '$route', '$scope', '$timeout',
-    'annotator', 'flash', 'identity', 'streamer', 'streamfilter',
-    'documentHelpers', 'drafts'
+    'annotator', 'auth', 'documentHelpers', 'drafts', 'flash', 'identity',
+    'streamer', 'streamfilter'
+
   ]
   constructor: (
      $location,   $route,   $scope,   $timeout,
-     annotator,   flash,   identity,   streamer,   streamfilter,
-     documentHelpers,   drafts
+     annotator,   auth,   documentHelpers,   drafts,   flash,   identity,
+     streamer,   streamfilter,
+
   ) ->
     {plugins, host, providers} = annotator
 
-    checkingToken = false
+    $scope.auth = auth
     isFirstRun = $location.search().hasOwnProperty('firstrun')
 
     applyUpdates = (action, data) ->
@@ -65,7 +41,7 @@ class AppController
 
       unless payload instanceof Array then payload = [payload]
 
-      p = $scope.persona
+      p = auth.user
       user = if p? then "acct:" + p.username + "@" + p.provider else ''
       unless payload instanceof Array then payload = [payload]
 
@@ -80,7 +56,7 @@ class AppController
       Store = plugins.Store
       delete plugins.Store
 
-      if $scope.persona or annotator.socialView.name is 'none'
+      if auth.user or annotator.socialView.name is 'none'
         annotator.addPlugin 'Store', annotator.options.Store
 
         $scope.store = plugins.Store
@@ -105,12 +81,12 @@ class AppController
       Store.updateAnnotation = angular.noop
 
       # Sort out which annotations should remain in place.
-      user = $scope.persona
+      user = auth.user
       view = annotator.socialView.name
       cull = (acc, annotation) ->
         if view is 'single-player' and annotation.user != user
           acc.drop.push annotation
-        else if authorizeAction 'read', annotation, user
+        else if auth.permits 'read', annotation, user
           acc.keep.push annotation
         else
           acc.drop.push annotation
@@ -131,47 +107,6 @@ class AppController
         annotator.deleteAnnotation first
         $timeout -> cleanup rest
 
-    onlogin = (assertion) ->
-      checkingToken = true
-
-      # Configure the Auth plugin with the issued assertion as refresh token.
-      annotator.addPlugin 'Auth',
-        tokenUrl: documentHelpers.absoluteURI(
-          "/api/token?assertion=#{assertion}")
-
-      # Set the user from the token.
-      plugins.Auth.withToken (token) ->
-        checkingToken = false
-        annotator.addPlugin 'Permissions',
-          user: token.userId
-          userAuthorize: authorizeAction
-        $scope.$apply ->
-          $scope.persona = token.userId
-          reset()
-
-    onlogout = ->
-      plugins.Auth?.element.removeData('annotator:headers')
-      plugins.Auth?.destroy()
-      delete plugins.Auth
-
-      plugins.Permissions?.setUser(null)
-      plugins.Permissions?.destroy()
-      delete plugins.Permissions
-
-      $scope.persona = null
-      checkingToken = false
-      reset()
-
-    onready = ->
-      if not checkingToken and typeof $scope.persona == 'undefined'
-        # If we're not checking the token and persona is undefined, onlogin
-        # hasn't run, which means we aren't authenticated.
-        $scope.persona = null
-        reset()
-
-        if isFirstRun
-          $scope.login()
-
     oncancel = ->
       $scope.dialog.visible = false
 
@@ -187,12 +122,10 @@ class AppController
       streamer.close()
       streamer.open()
 
-    identity.watch {onlogin, onlogout, onready}
-
     $scope.$watch 'socialView.name', (newValue, oldValue) ->
       return if newValue is oldValue
       initStore()
-      if newValue is 'single-player' and not $scope.persona
+      if newValue is 'single-player' and not auth.user
         annotator.show()
         flash 'info',
           'You will need to sign in for your highlights to be saved.'
@@ -215,6 +148,11 @@ class AppController
 
         streamer.send({filter: streamfilter.getFilter()})
 
+    $scope.$watch 'auth.user', (newVal, oldVal) ->
+      return if newVal is undefined
+      reset()
+      $scope.login() if isFirstRun and not (newVal or oldVal)
+
     $scope.login = ->
       $scope.dialog.visible = true
       identity.request {oncancel}

h/static/scripts/directives/annotation.coffee

@@ -38,9 +38,9 @@ validate = (value) ->
 ###
 AnnotationController = [
   '$scope', '$timeout',
-  'annotator', 'drafts', 'flash', 'documentHelpers', 'timeHelpers',
+  'annotator', 'auth', 'drafts', 'flash', 'documentHelpers', 'timeHelpers'
   ($scope,   $timeout,
-   annotator,   drafts,   flash,   documentHelpers,   timeHelpers
+   annotator,   auth,   drafts,   flash,   documentHelpers,   timeHelpers
   ) ->
     @annotation = {}
     @action = 'view'
@@ -182,11 +182,16 @@ AnnotationController = [
       reply = {references, uri}
       annotator.publish 'beforeAnnotationCreated', reply
 
+      if auth.user?
+        reply.permissions.update = [auth.user]
+        reply.permissions.delete = [auth.user]
+        reply.permissions.admin = [auth.user]
+
         # If replying to a public annotation make the response public.
         if 'group:__world__' in (model.permissions.read or [])
           reply.permissions.read = ['group:__world__']
         else
-        reply.permissions.read = [model.user]
+          reply.permissions.read = [auth.user]
 
     ###*
     # @ngdoc method

tests/js/account/account-controller-test.coffee

@@ -8,6 +8,7 @@ describe 'h.account.AccountController', ->
   fakeSession = null
   fakeIdentity = null
   fakeFormHelpers = null
+  fakeAuth = null
   editProfilePromise = null
   disableUserPromise = null
   profilePromise = null
@@ -22,6 +23,8 @@ describe 'h.account.AccountController', ->
       logout: sandbox.spy()
     fakeFormHelpers =
       applyValidationErrors: sandbox.spy()
+    fakeAuth =
+      user: 'egon@columbia.edu'
 
     $filterProvider.register 'persona', ->
       sandbox.stub().returns('STUBBED_PERSONA_FILTER')
@@ -30,11 +33,11 @@ describe 'h.account.AccountController', ->
     $provide.value 'flash', fakeFlash
     $provide.value 'identity', fakeIdentity
     $provide.value 'formHelpers', fakeFormHelpers
+    $provide.value 'auth', fakeAuth
     return
 
   beforeEach inject ($rootScope, $q, $controller) ->
     $scope = $rootScope.$new()
-    $scope.persona = 'egon@columbia.edu'
 
     disableUserPromise = {then: sandbox.stub()}
     editProfilePromise = {then: sandbox.stub()}

tests/js/auth-service-test.coffee

+assert = chai.assert
+sinon.assert.expose assert, prefix: null
+
+describe 'h', ->
+  fakeAnnotator = null
+  fakeIdentity = null
+  sandbox = null
+
+  beforeEach module('h')
+
+  beforeEach module ($provide) ->
+    sandbox = sinon.sandbox.create()
+
+    fakeAnnotator = {
+      plugins: {
+        Auth:{
+          withToken: sandbox.spy()
+          destroy: sandbox.spy()
+          element: {removeData: sandbox.spy()}
+        }
+        Permissions: {
+          destroy: sandbox.spy()
+          setUser: sandbox.spy()
+        }
+      }
+      options: {}
+      socialView: {name: 'none'}
+      addPlugin: sandbox.spy()
+    }
+
+    fakeIdentity ={
+      watch: sandbox.spy()
+      request: sandbox.spy()
+
+    }
+
+    $provide.value 'annotator', fakeAnnotator
+    $provide.value 'identity', fakeIdentity
+    return
+
+  afterEach ->
+    sandbox.restore()
+
+
+  describe 'auth service', ->
+    auth = null
+
+    beforeEach inject (_auth_) ->
+      auth = _auth_
+
+    it 'watches the identity service for identity change events', ->
+      assert.calledOnce(fakeIdentity.watch)
+
+    it 'sets the user to null when the identity has been checked', ->
+      {onready} = fakeIdentity.watch.args[0][0]
+      onready()
+      assert.isNull(auth.user)
+
+    it 'sets the Permissions plugin and sets auth.user at login', ->
+      {onlogin} = fakeIdentity.watch.args[0][0]
+      onlogin('test-assertion')
+      fakeToken = { userId: 'acct:hey@joe'}
+      userSetter = fakeAnnotator.plugins.Auth.withToken.args[0][0]
+      userSetter(fakeToken)
+      assert.equal(auth.user, 'acct:hey@joe')
+      secondPlugin = fakeAnnotator.addPlugin.args[1]
+      assert.equal(secondPlugin[0], 'Permissions')
+
+    it 'destroys the plugins at logout and sets auth.user to null', ->
+      {onlogout} = fakeIdentity.watch.args[0][0]
+      auth.user = 'acct:hey@joe'
+      authPlugin = fakeAnnotator.plugins.Auth
+      permissionsPlugin = fakeAnnotator.plugins.Permissions
+      onlogout()
+
+      assert.called(authPlugin.destroy)
+      assert.called(permissionsPlugin.destroy)
+      assert.equal(auth.user, null)
+

tests/js/controllers-test.coffee

@@ -3,6 +3,7 @@ sinon.assert.expose assert, prefix: null
 
 describe 'h', ->
   $scope = null
+  fakeAuth = null
   fakeIdentity = null
   fakeLocation = null
   fakeParams = null
@@ -22,10 +23,16 @@ describe 'h', ->
       socialView: {name: 'none'}
       addPlugin: sandbox.spy()
     }
+
+    fakeAuth = {
+      user: null
+    }
+
     fakeIdentity = {
       watch: sandbox.spy()
       request: sandbox.spy()
     }
+
     fakeLocation = {
       search: sandbox.stub().returns({})
     }
@@ -55,41 +62,9 @@ describe 'h', ->
       createController = ->
         $controller('AppController', {$scope: $scope})
 
-    it 'watches the identity service for identity change events', ->
-      app = createController()
-      assert.calledOnce(fakeIdentity.watch)
-
-    it 'sets the persona to null when the identity has been checked', ->
-      app = createController()
-      {onlogin, onlogout, onready} = fakeIdentity.watch.args[0][0]
-      onready()
-      assert.isNull($scope.persona)
-
-    it 'does not set the persona to null while token is still being checked', ->
-      app = createController()
-      {onlogin, onlogout, onready} = fakeIdentity.watch.args[0][0]
-      onlogin()
-      onready()
-      assert.isNotNull($scope.persona)
-
-    it 'shows login form for logged out users on first run', ->
-      fakeLocation.search.returns({'firstrun': ''})
-      app = createController()
-      {onlogin, onlogout, onready} = fakeIdentity.watch.args[0][0]
-      onready()
-      assert.isTrue($scope.dialog.visible)
-
-    it 'does not show login form for logged out users if not first run', ->
-      app = createController()
-      {onlogin, onlogout, onready} = fakeIdentity.watch.args[0][0]
-      onready()
-      assert.isFalse($scope.dialog.visible)
-
     it 'does not show login form for logged in users', ->
-      app = createController()
-      {onlogin, onlogout, onready} = fakeIdentity.watch.args[0][0]
-      onlogin('abcdef123')
-      onready()
+      createController()
+      $scope.$digest()
       assert.isFalse($scope.dialog.visible)
 
   describe 'AnnotationViewerController', ->

tests/js/directives/annotation-test.coffee

@@ -10,10 +10,18 @@ describe 'h.directives.annotation', ->
   annotation = null
   createController = null
   flash = null
+  fakeUser = null
 
   beforeEach module('h')
   beforeEach module('h.templates')
 
+  beforeEach module ($provide) ->
+    fakeAuth =
+      user: 'acct:bill@localhost'
+
+    $provide.value 'auth', fakeAuth
+    return
+
   beforeEach inject (_$compile_, $controller, _$document_, $rootScope, _$timeout_) ->
     $compile = _$compile_
     $document = _$document_
@@ -70,11 +78,18 @@ describe 'h.directives.annotation', ->
       newAnnotation = annotator.publish.lastCall.args[1]
       assert.include(newAnnotation.permissions.read, 'group:__world__')
 
-    it 'does not add the world readable principal if the parent is privace', ->
+    it 'does not add the world readable principal if the parent is private', ->
       controller.reply()
       newAnnotation = annotator.publish.lastCall.args[1]
       assert.notInclude(newAnnotation.permissions.read, 'group:__world__')
 
+    it 'fills the other permissions too', ->
+      controller.reply()
+      newAnnotation = annotator.publish.lastCall.args[1]
+      assert.equal(newAnnotation.permissions.update[0], 'acct:bill@localhost')
+      assert.equal(newAnnotation.permissions.delete[0], 'acct:bill@localhost')
+      assert.equal(newAnnotation.permissions.admin[0], 'acct:bill@localhost')
+
   describe '#render', ->
     controller = null