Merge pull request #2687 from...

Merge pull request #2687 from hypothesis/github-2686-fix-invalid-permissions-when-creating-annotations-when-signed-out

Set permissions of new annotations on login

h/static/scripts/directive/annotation.coffee

 ### global -validate ###
 
-STORAGE_KEY = 'hypothesis.privacy'
+events = require('../events')
 
 # Validate an annotation.
 # Annotations must be attributed to a user or marked as deleted.
@@ -46,10 +46,10 @@ errorMessage = (reason) ->
 AnnotationController = [
   '$scope', '$timeout', '$q', '$rootScope', '$document',
   'drafts', 'flash', 'permissions', 'tags', 'time',
-  'annotationUI', 'annotationMapper', 'session', 'groups', 'localStorage'
+  'annotationUI', 'annotationMapper', 'session', 'groups',
   ($scope,   $timeout,   $q,   $rootScope,   $document,
    drafts,   flash,   permissions,   tags,   time,
-   annotationUI,   annotationMapper,   session,   groups,   localStorage) ->
+   annotationUI,   annotationMapper,   session,   groups) ->
 
     @annotation = {}
     @action = 'view'
@@ -61,19 +61,14 @@ AnnotationController = [
 
     model = $scope.annotationGet()
 
+    model.user ?= session.state.userid
+
     # Set the group of new annotations.
     if not model.group
       model.group = groups.focused().id
 
     # Set the permissions of new annotations.
-    if not model.permissions
-      defaultLevel = localStorage.getItem(STORAGE_KEY);
-      if (defaultLevel != 'private') and (defaultLevel != 'shared')
-        defaultLevel = 'shared';
-      if defaultLevel == 'private'
-        model.permissions = permissions.private()
-      else
-        model.permissions = permissions.shared(model.group)
+    model.permissions = model.permissions or permissions.default(model.group)
 
     highlight = model.$highlight
     original = null
@@ -140,7 +135,7 @@ AnnotationController = [
       # next time they create an annotation.
       # But _don't_ cache it when they change the privacy level of a reply.
       if not model.references  # If the annotation is not a reply.
-        localStorage.setItem(STORAGE_KEY, privacy);
+        permissions.setDefault(privacy)
 
       if privacy == 'private'
         @annotation.permissions = permissions.private()
@@ -383,11 +378,15 @@ AnnotationController = [
       this.render()
     , true
 
-    # Watch the current user
-    # TODO: fire events instead since watchers are not free and auth is rare
-    $scope.$watch (-> session.state.userid), (userid) ->
-      model.permissions ?= {}
-      model.user ?= userid
+    $scope.$on(events.USER_CHANGED, ->
+      model.user ?= session.state.userid
+
+      # Set model.permissions on sign in, if it isn't already set.
+      # This is because you can create annotations when signed out and they
+      # will have model.permissions = null, then when you sign in we set the
+      # permissions correctly here.
+      model.permissions = model.permissions or permissions.default(model.group)
+    )
 
     # Start editing brand new annotations immediately
     unless model.id? or (this.isHighlight() and highlight) then this.edit()

h/static/scripts/directive/test/annotation-test.coffee

 {module, inject} = angular.mock
 
+events = require('../../events')
+
 describe 'annotation', ->
   $compile = null
   $document = null
@@ -27,7 +29,6 @@ describe 'annotation', ->
   fakeTags = null
   fakeTime = null
   fakeUrlEncodeFilter = null
-  fakeLocalStorage = null
   sandbox = null
 
   createDirective = ->
@@ -71,6 +72,8 @@ describe 'annotation', ->
       permits: sandbox.stub().returns(true)
       shared: sandbox.stub().returns({read: ['everybody']})
       private: sandbox.stub().returns({read: ['justme']})
+      default: sandbox.stub().returns({read: ['default']})
+      setDefault: sandbox.stub()
     }
     fakePersonaFilter = sandbox.stub().returnsArg(0)
     fakeDocumentTitleFilter = (arg) -> ''
@@ -89,10 +92,6 @@ describe 'annotation', ->
       nextFuzzyUpdate: sandbox.stub().returns(30)
     }
     fakeUrlEncodeFilter = (v) -> encodeURIComponent(v)
-    fakeLocalStorage = {
-      setItem: sandbox.stub()
-      getItem: sandbox.stub()
-    }
 
     fakeGroups = {
       focused: -> {}
@@ -114,7 +113,6 @@ describe 'annotation', ->
     $provide.value 'tags', fakeTags
     $provide.value 'time', fakeTime
     $provide.value 'urlencodeFilter', fakeUrlEncodeFilter
-    $provide.value 'localStorage', fakeLocalStorage
     $provide.value 'groups', fakeGroups
     return
 
@@ -152,6 +150,7 @@ describe 'annotation', ->
       $scope.$digest()
       assert.notCalled annotation.$create
       fakeSession.state.userid = 'acct:ted@wyldstallyns.com'
+      $scope.$broadcast(events.USER_CHANGED, {})
       $scope.$digest()
       assert.calledOnce annotation.$create
 
@@ -240,8 +239,7 @@ describe 'annotation', ->
       controller.edit();
       controller.setPrivacy('shared')
       controller.save().then(->
-        assert(fakeLocalStorage.setItem.calledWithExactly(
-          'hypothesis.privacy', 'shared'))
+        assert(fakePermissions.setDefault.calledWithExactly('shared'))
       )
 
     it 'saves the "private" visibility level to localStorage', ->
@@ -249,8 +247,7 @@ describe 'annotation', ->
       controller.edit();
       controller.setPrivacy('private')
       controller.save().then(->
-        assert(fakeLocalStorage.setItem.calledWithExactly(
-          'hypothesis.privacy', 'private'))
+        assert(fakePermissions.setDefault.calledWithExactly('private'))
       )
 
     it "doesn't save the visibility if the annotation is a reply", ->
@@ -259,7 +256,7 @@ describe 'annotation', ->
       controller.edit();
       controller.setPrivacy('private')
       controller.save().then(->
-        assert(not fakeLocalStorage.setItem.called)
+        assert(not fakePermissions.setDefault.called)
       )
 
   describe '#hasContent', ->
@@ -587,8 +584,10 @@ describe("AnnotationController", ->
         isPrivate: (permissions, user) ->
           return user in permissions.read
         permits: -> true
-        shared: (group) -> {"read": [group]}
-        private: -> {"read": [session.state.userid]}
+        shared: -> {}
+        private: -> {}
+        default: -> {}
+        setDefault: ->
       }
       session: session
       tags: tags or {store: ->}
@@ -647,44 +646,57 @@ describe("AnnotationController", ->
     it("creates the directive without crashing", ->
       createAnnotationDirective({})
     )
+  )
 
-    it("sets the permissions of new annotations from local storage", ->
-      {controller} = createAnnotationDirective({
-        localStorage: {
-          setItem: ->
-          getItem: (key) ->
-            if key == 'hypothesis.privacy'
-              return 'shared'
-            else
-              assert(false, "Wrong key requested from localStorage")
-        }
-      })
-      assert(controller.isShared())
+  it("sets the user of new annotations", ->
+    annotation = {}
+
+    {session} = createAnnotationDirective({annotation: annotation})
+
+    assert.equal(annotation.user, session.state.userid)
   )
 
-    it("sets the permissions of new annotations from local storage to private", ->
-      {controller} = createAnnotationDirective({
-        localStorage: {
-          setItem: ->
-          getItem: (key) ->
-            if key == 'hypothesis.privacy'
-              return 'private'
-            else
-              assert(false, "Wrong key requested from localStorage")
+  it("sets the permissions of new annotations", ->
+    # This is a new annotation, doesn't have any permissions yet.
+    annotation = {group: "test-group"}
+    permissions = {
+      default: sinon.stub().returns("default permissions")
+      isShared: ->
+      isPrivate: ->
     }
-      })
-      assert(controller.isPrivate())
+
+    createAnnotationDirective({
+      annotation: annotation, permissions: permissions})
+
+    assert(permissions.default.calledWithExactly("test-group"))
+    assert.equal(annotation.permissions, "default permissions",
+      "It should set a new annotation's permissions to what
+      permissions.default() returns")
   )
 
-    it("defaults to shared if no locally cached visibility", ->
-      {controller} = createAnnotationDirective({
-        localStorage: {
-          setItem: ->
-          getItem: ->
+  it("doesn't overwrite permissions if the annotation already has them", ->
+    # The annotation will already have some permissions, before it's
+    # passed to AnnotationController.
+    annotation = {
+      permissions: {
+        read: ["foo"]
+        update: ["bar"]
+        delete: ["gar"]
+        admin: ["har"]
       }
-      })
-      assert(controller.isShared())
-    )
+    }
+    # Save the original permissions for asserting against later.
+    original_permissions = JSON.parse(JSON.stringify(annotation.permissions))
+    permissions = {
+      default: sinon.stub().returns("new permissions")
+      isShared: ->
+      isPrivate: ->
+    }
+
+    createAnnotationDirective(
+      {annotation: annotation, permissions: permissions})
+
+    assert.deepEqual(annotation.permissions, original_permissions)
   )
 
   describe("save", ->
@@ -718,6 +730,66 @@ describe("AnnotationController", ->
     )
   )
 
+  describe("when the user signs in", ->
+    it("sets the user of unsaved annotations", ->
+      # This annotation has no user yet, because that's what happens
+      # when you create a new annotation while not signed in.
+      annotation = {}
+      session = {state: {userid: null}}  # Not signed in.
+
+      {$rootScope} = createAnnotationDirective(
+        {annotation: annotation, session:session})
+
+      # At this point we would not expect the user to have been set,
+      # even though the annotation has been created, because the user isn't
+      # signed in.
+      assert(!annotation.user)
+
+      # Sign the user in.
+      session.state.userid = "acct:fred@hypothes.is"
+
+      # The session service would broadcast USER_CHANGED after sign in.
+      $rootScope.$broadcast(events.USER_CHANGED, {})
+
+      assert.equal(annotation.user, session.state.userid)
+    )
+
+    it("sets the permissions of unsaved annotations", ->
+      # This annotation has no permissions yet, because that's what happens
+      # when you create a new annotation while not signed in.
+      annotation = {group: "__world__"}
+      session = {state: {userid: null}}  # Not signed in.
+      permissions = {
+        # permissions.default() would return null, because the user isn't
+        # signed in.
+        default: -> null
+        # We just need to mock these two functions so that it doesn't crash.
+        isShared: ->
+        isPrivate: ->
+      }
+
+      {$rootScope} = createAnnotationDirective(
+        {annotation: annotation, session:session, permissions: permissions})
+
+      # At this point we would not expect the permissions to have been set,
+      # even though the annotation has been created, because the user isn't
+      # signed in.
+      assert(!annotation.permissions)
+
+      # Sign the user in.
+      session.state.userid = "acct:fred@hypothes.is"
+
+      # permissions.default() would now return permissions, because the user
+      # is signed in.
+      permissions.default = -> "__default_permissions__"
+
+      # The session service would broadcast USER_CHANGED after sign in.
+      $rootScope.$broadcast(events.USER_CHANGED, {})
+
+      assert.equal(annotation.permissions, "__default_permissions__")
+    )
+  )
+
   ###
   Simulate what happens when the user edits an annotation, clicks Save,
   gets an error because the server fails to save the annotation, then clicks

h/static/scripts/permissions.coffee

+STORAGE_KEY = 'hypothesis.privacy'
+
 ###*
 # @ngdoc service
 # @name  Permissions
@@ -6,7 +8,7 @@
 # This service can set default permissions to annotations properly and
 # offers some utility functions regarding those.
 ###
-module.exports = ['session', (session) ->
+module.exports = ['session', 'localStorage', (session, localStorage) ->
   ALL_PERMISSIONS = {}
   GROUP_WORLD = 'group:__world__'
   ADMIN_PARTY = [{
@@ -37,10 +39,15 @@ module.exports = ['session', (session) ->
   # Typical use: annotation.permissions = permissions.private()
   ###
   private: ->
+    if not session.state.userid
+      return null
+    else
+      return {
         read: [session.state.userid]
         update: [session.state.userid]
         delete: [session.state.userid]
         admin: [session.state.userid]
+      }
 
   ###*
   # @ngdoc method
@@ -52,6 +59,8 @@ module.exports = ['session', (session) ->
   # Typical use: annotation.permissions = permissions.shared(group)
   ###
   shared: (group) ->
+    if not session.state.userid
+      return null
     if group?
       group = 'group:' + group
     else
@@ -63,6 +72,25 @@ module.exports = ['session', (session) ->
       admin: [session.state.userid]
     }
 
+  # Return the initial permissions for a new annotation in the given group.
+  default: (group) ->
+    defaultLevel = localStorage.getItem(STORAGE_KEY);
+    if (defaultLevel != 'private') and (defaultLevel != 'shared')
+      defaultLevel = 'shared';
+    if defaultLevel == 'private'
+      return this.private()
+    else
+      return this.shared(group)
+
+  # Set the default initial permissions for new annotations (that will be
+  # returned by default() above) to "private" or "shared" permissions.
+  #
+  # @param {string} private_or_shared "private" to make default() return the
+  #   necessary permissions for private annotations, "shared" to make it
+  #   return those for shared annotations.
+  setDefault: (private_or_shared) ->
+    localStorage.setItem(STORAGE_KEY, private_or_shared);
+
   ###*
   # @ngdoc method
   # @name permissions#isShared

h/static/scripts/test/permissions-test.coffee

@@ -3,6 +3,7 @@
 describe 'h:permissions', ->
   sandbox = null
   fakeSession = null
+  fakeLocalStorage = null
 
   before ->
     angular.module('h', [])
@@ -17,8 +18,12 @@ describe 'h:permissions', ->
         userid: 'acct:flash@gordon'
       }
     }
+    fakeLocalStorage = {
+      getItem: -> undefined
+    }
 
     $provide.value 'session', fakeSession
+    $provide.value 'localStorage', fakeLocalStorage
     return
 
   afterEach ->
@@ -31,27 +36,71 @@ describe 'h:permissions', ->
     beforeEach inject (_permissions_) ->
       permissions = _permissions_
 
-    it 'private call fills all permissions with auth.user', ->
+    describe 'private()', ->
+
+      it 'fills all permissions with auth.user', ->
         perms = permissions.private()
         assert.equal(perms.read[0], 'acct:flash@gordon')
         assert.equal(perms.update[0], 'acct:flash@gordon')
         assert.equal(perms.delete[0], 'acct:flash@gordon')
         assert.equal(perms.admin[0], 'acct:flash@gordon')
 
-    it 'public call fills the read property with group:__world__', ->
+      it 'returns null if session.state.userid is falsey', ->
+        delete fakeSession.state.userid
+        assert.equal(permissions.private(), null)
+
+    describe 'shared()', ->
+
+      it 'fills the read property with group:__world__', ->
         perms = permissions.shared()
         assert.equal(perms.read[0], 'group:__world__')
         assert.equal(perms.update[0], 'acct:flash@gordon')
         assert.equal(perms.delete[0], 'acct:flash@gordon')
         assert.equal(perms.admin[0], 'acct:flash@gordon')
 
-    it 'public call fills the read property with group:foo if passed "foo"', ->
+      it 'fills the read property with group:foo if passed "foo"', ->
         perms = permissions.shared("foo")
         assert.equal(perms.read[0], 'group:foo')
         assert.equal(perms.update[0], 'acct:flash@gordon')
         assert.equal(perms.delete[0], 'acct:flash@gordon')
         assert.equal(perms.admin[0], 'acct:flash@gordon')
 
+      it 'returns null if session.state.userid is falsey', ->
+        delete fakeSession.state.userid
+        assert.equal(permissions.shared("foo"), null)
+
+    describe 'default()', ->
+
+      it 'returns shared permissions if localStorage contains "shared"', ->
+        fakeLocalStorage.getItem = -> 'shared'
+        assert(permissions.isShared(permissions.default()))
+
+      it 'returns private permissions if localStorage contains "private"', ->
+        fakeLocalStorage.getItem = -> 'private'
+        assert(permissions.isPrivate(
+          permissions.default(), fakeSession.state.userid))
+
+      it 'returns shared permissions if localStorage is empty', ->
+        fakeLocalStorage.getItem = -> undefined
+        assert(permissions.isShared(permissions.default()))
+
+    describe 'setDefault()', ->
+
+      it 'sets the default permissions that default() will return', ->
+        stored = {}
+        fakeLocalStorage.setItem = (key, value) ->
+          stored[key] = value
+        fakeLocalStorage.getItem = (key) ->
+          return stored[key]
+
+        permissions.setDefault('private')
+        assert(permissions.isPrivate(
+          permissions.default(), fakeSession.state.userid))
+
+        permissions.setDefault('shared')
+        assert(permissions.isShared(
+          permissions.default('foo'), 'foo'))
+
     describe 'isPublic', ->
       it 'isPublic() true if the read permission has group:__world__ in it', ->
         permission = {