Set permissions of new annotations on login

While signed out create a new annotation. You'll see a "You must sign in
to create an annotation" card. This card is in fact an annotation
in the client-side code, but one that has no user, no permissions, etc
etc. If you sign in, you'll then see a full annotation card with editor
open in place of the previous card, you can type an annotation and save
it.

Except that this will send invalid permissions to the server, which will
happy save them (no validation) and then crash.

The fix:

On sign in, set the model.permissions of such annotations. This means
that the Post dropdown will set itself correctly to shared or private
and on posting the annotation the correct permissions will be sent to
the server.

Also fixed permissions.coffee to return null instead of invalid
permissions when not signed in.

Fixes #2686.

h/static/scripts/directive/annotation.coffee

@@ -61,19 +61,24 @@ AnnotationController = [
 
     model = $scope.annotationGet()
 
+    # Set model.permissions to the default permissions for new annotations,
+    # _if_ model.permissions isn't already set.
+    setDefaultPermissions = ->
+      if not model.permissions?.read
+        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)
+
     # 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)
+    setDefaultPermissions()
 
     highlight = model.$highlight
     original = null
@@ -379,6 +384,8 @@ AnnotationController = [
         else
           drafts.add model, => this.revert()
 
+      setDefaultPermissions()
+
       updateTimestamp(model is old)  # repeat on first run
       this.render()
     , true

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

@@ -587,8 +587,16 @@ describe("AnnotationController", ->
         isPrivate: (permissions, user) ->
           return user in permissions.read
         permits: -> true
-        shared: (group) -> {"read": [group]}
-        private: -> {"read": [session.state.userid]}
+        shared: (group) ->
+          if not session?.state?.userid
+            return null
+          else
+            {"read": [group]}
+        private: ->
+          if not session?.state?.userid
+            return null
+          else
+            {"read": [session.state.userid]}
       }
       session: session
       tags: tags or {store: ->}
@@ -647,44 +655,63 @@ 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 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 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")
-        }
-      })
-      assert(controller.isPrivate())
-    )
+  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")
+      }
+    })
+    assert(controller.isPrivate())
+  )
 
-    it("defaults to shared if no locally cached visibility", ->
-      {controller} = createAnnotationDirective({
-        localStorage: {
-          setItem: ->
-          getItem: ->
-        }
-      })
-      assert(controller.isShared())
-    )
+  it("defaults to shared if no locally cached visibility", ->
+    {controller} = createAnnotationDirective({
+      localStorage: {
+        setItem: ->
+        getItem: ->
+      }
+    })
+    assert(controller.isShared())
+  )
+
+  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"]
+      }
+    }
+    # Save the original permissions for asserting against later.
+    original_permissions = JSON.parse(JSON.stringify(annotation.permissions))
+
+    createAnnotationDirective({annotation: annotation})
+
+    assert.deepEqual(annotation.permissions, original_permissions)
   )
 
   describe("save", ->
@@ -718,6 +745,20 @@ describe("AnnotationController", ->
     )
   )
 
+  describe("when the user signs in", ->
+    it("sets the permissions of unsaved annotations", ->
+      annotation = {group: "__world__"}
+      session = {state: {userid: null}}  # Not signed in.
+
+      {$rootScope} = createAnnotationDirective(
+        {annotation: annotation, session:session})
+
+      session.state.userid = "acct:fred@hypothes.is"  # The user signs in.
+      $rootScope.$digest()  # Trigger the $scope.$watch().
+      assert.deepEqual(annotation.permissions, {read: ["__world__"]})
+    )
+  )
+
   ###
   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

@@ -37,10 +37,15 @@ module.exports = ['session', (session) ->
   # Typical use: annotation.permissions = permissions.private()
   ###
   private: ->
-    read: [session.state.userid]
-    update: [session.state.userid]
-    delete: [session.state.userid]
-    admin: [session.state.userid]
+    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 +57,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

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

@@ -31,26 +31,38 @@ describe 'h:permissions', ->
     beforeEach inject (_permissions_) ->
       permissions = _permissions_
 
-    it 'private call 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__', ->
-      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"', ->
-      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')
+    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 '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 '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 'isPublic', ->
       it 'isPublic() true if the read permission has group:__world__ in it', ->