Merge pull request #5347 from hypothesis/moderate-permission-and-resp…

…onses

Establish annotation `moderate` permission; protect API endpoints

h/models/group.py

@@ -144,6 +144,8 @@ def __acl__(self):
 
         if self.creator:
             terms.append((security.Allow, self.creator.userid, 'admin'))
+            terms.append((security.Allow, self.creator.userid, 'moderate'))
+
         terms.append(security.DENY_ALL)
 
         return terms

h/traversal/contexts.py

@@ -55,17 +55,28 @@ def __acl__(self):
             return [DENY_ALL]
 
         acl = []
+
+        # For shared annotations, some permissions are derived from the
+        # permissions for this annotation's containing group.
+        # Otherwise they are derived from the annotation's creator
         if self.annotation.shared:
             for principal in self._group_principals(self.group, 'read'):
                 acl.append((Allow, principal, 'read'))
 
             for principal in self._group_principals(self.group, 'flag'):
                 acl.append((Allow, principal, 'flag'))
 
+            for principal in self._group_principals(self.group, 'moderate'):
+                acl.append((Allow, principal, 'moderate'))
+
         else:
             acl.append((Allow, self.annotation.userid, 'read'))
+            # Flagging one's own private annotations is nonsensical,
+            # but from an authz perspective, allowed. It is up to services/views
+            # to handle these situations appropriately
             acl.append((Allow, self.annotation.userid, 'flag'))
 
+        # The user who created the annotation always has the following permissions
         for action in ['admin', 'update', 'delete']:
             acl.append((Allow, self.annotation.userid, action))
 

h/views/api/moderation.py

@@ -2,8 +2,7 @@
 
 from __future__ import unicode_literals
 
-from pyramid import security
-from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound
+from pyramid.httpexceptions import HTTPNoContent
 
 from h import events
 from h.views.api.config import api_config
@@ -13,10 +12,8 @@
             request_method='PUT',
             link_name='annotation.hide',
             description='Hide an annotation as a group moderator.',
-            effective_principals=security.Authenticated)
+            permission='moderate')
 def create(context, request):
-    if not request.has_permission('admin', context.group):
-        raise HTTPNotFound()
 
     svc = request.find_service(name='annotation_moderation')
     svc.hide(context.annotation)
@@ -31,10 +28,8 @@ def create(context, request):
             request_method='DELETE',
             link_name='annotation.unhide',
             description='Unhide an annotation as a group moderator.',
-            effective_principals=security.Authenticated)
+            permission='moderate')
 def delete(context, request):
-    if not request.has_permission('admin', context.group):
-        raise HTTPNotFound()
 
     svc = request.find_service(name='annotation_moderation')
     svc.unhide(context.annotation)

tests/functional/api/test_moderation.py

@@ -0,0 +1,168 @@
+# -*- coding: utf-8 -*-
+
+from __future__ import unicode_literals
+
+import pytest
+
+# String type for request/response headers and metadata in WSGI.
+#
+# Per PEP-3333, this is intentionally `str` under both Python 2 and 3, even
+# though it has different meanings.
+#
+# See https://www.python.org/dev/peps/pep-3333/#a-note-on-string-types
+native_str = str
+
+
+@pytest.mark.functional
+class TestPutHide(object):
+
+    def test_it_returns_http_204_for_group_creator(self,
+                                                   app,
+                                                   group_annotation,
+                                                   user_with_token):
+        user, token = user_with_token
+        headers = {'Authorization': str('Bearer {}'.format(token.value))}
+
+        res = app.put('/api/annotations/{id}/hide'.format(id=group_annotation.id),
+                                                          headers=headers)
+
+        # The creator of a group has moderation rights over the annotations in that group
+        assert res.status_code == 204
+
+    def test_it_returns_http_404_if_annotation_is_in_world_group(self,
+                                                                 app,
+                                                                 world_annotation,
+                                                                 user_with_token):
+        user, token = user_with_token
+        headers = {'Authorization': str('Bearer {}'.format(token.value))}
+
+        res = app.put('/api/annotations/{id}/hide'.format(id=world_annotation.id),
+                                                          headers=headers,
+                                                          expect_errors=True)
+        # The current user does not have moderation rights on the world group
+        assert res.status_code == 404
+
+    def test_it_returns_http_404_if_no_authn(self,
+                                             app,
+                                             group_annotation):
+
+        res = app.put('/api/annotations/{id}/hide'.format(id=group_annotation.id),
+                                                          expect_errors=True)
+
+        assert res.status_code == 404
+
+    def test_it_returns_http_404_if_annotation_is_private(self,
+                                                          app,
+                                                          private_group_annotation,
+                                                          user_with_token):
+
+        user, token = user_with_token
+        headers = {'Authorization': str('Bearer {}'.format(token.value))}
+
+        res = app.put('/api/annotations/{id}/hide'.format(id=private_group_annotation.id),
+                                                          headers=headers,
+                                                          expect_errors=True)
+        # private annotations cannot be moderated
+        assert res.status_code == 404
+
+
+@pytest.mark.functional
+class TestDeleteHide(object):
+
+    def test_it_returns_http_204_for_group_creator(self,
+                                                   app,
+                                                   group_annotation,
+                                                   user_with_token):
+        user, token = user_with_token
+        headers = {'Authorization': str('Bearer {}'.format(token.value))}
+
+        res = app.delete('/api/annotations/{id}/hide'.format(id=group_annotation.id),
+                                                             headers=headers)
+
+        # The creator of a group has moderation rights over the annotations in that group
+        assert res.status_code == 204
+
+    def test_it_returns_http_404_if_annotation_is_in_world_group(self,
+                                                                 app,
+                                                                 world_annotation,
+                                                                 user_with_token):
+        user, token = user_with_token
+        headers = {'Authorization': str('Bearer {}'.format(token.value))}
+
+        res = app.delete('/api/annotations/{id}/hide'.format(id=world_annotation.id),
+                                                             headers=headers,
+                                                             expect_errors=True)
+        # The current user does not have moderation rights on the world group
+        assert res.status_code == 404
+
+    def test_it_returns_http_404_if_no_authn(self,
+                                             app,
+                                             group_annotation):
+
+        res = app.delete('/api/annotations/{id}/hide'.format(id=group_annotation.id),
+                                                             expect_errors=True)
+
+        assert res.status_code == 404
+
+    def test_it_returns_http_404_if_annotation_is_private(self,
+                                                          app,
+                                                          private_group_annotation,
+                                                          user_with_token):
+
+        user, token = user_with_token
+        headers = {'Authorization': str('Bearer {}'.format(token.value))}
+
+        res = app.delete('/api/annotations/{id}/hide'.format(id=private_group_annotation.id),
+                                                             headers=headers,
+                                                             expect_errors=True)
+        # private annotations cannot be moderated
+        assert res.status_code == 404
+
+
+@pytest.fixture
+def user(db_session, factories):
+    user = factories.User()
+    db_session.commit()
+    return user
+
+
+@pytest.fixture
+def group(user, db_session, factories):
+    group = factories.Group(creator=user)
+    db_session.commit()
+    return group
+
+
+@pytest.fixture
+def world_annotation(user, db_session, factories):
+    ann = factories.Annotation(userid=user.userid,
+                               groupid='__world__',
+                               shared=True)
+    db_session.commit()
+    return ann
+
+
+@pytest.fixture
+def group_annotation(user, group, db_session, factories):
+    ann = factories.Annotation(userid='acct:someone@example.com',
+                               groupid=group.pubid,
+                               shared=True)
+    db_session.commit()
+    return ann
+
+
+@pytest.fixture
+def private_group_annotation(user, group, db_session, factories):
+    ann = factories.Annotation(userid='acct:someone@example.com',
+                               groupid=group.pubid,
+                               shared=False)
+    db_session.commit()
+    return ann
+
+
+@pytest.fixture
+def user_with_token(user, db_session, factories):
+    token = factories.DeveloperToken(userid=user.userid)
+    db_session.add(token)
+    db_session.commit()
+    return (user, token)

tests/h/models/groups_test.py

@@ -157,11 +157,16 @@ def test_world_readable(self, group, authz_policy):
         group.readable_by = ReadableBy.world
         assert authz_policy.permits(group, [security.Everyone], 'read')
 
-    def test_world_flaggable(self, group, authz_policy):
+    def test_world_readable_grants_flag_permissions(self, group, authz_policy):
         group.readable_by = ReadableBy.world
         assert authz_policy.permits(group, [security.Authenticated], 'flag')
         assert not authz_policy.permits(group, [security.Everyone], 'flag')
 
+    def test_world_readable_does_not_grant_moderate_permissions(self, group, authz_policy):
+        group.readable_by = ReadableBy.world
+        assert not authz_policy.permits(group, [security.Authenticated], 'moderate')
+        assert not authz_policy.permits(group, [security.Everyone], 'moderate')
+
     def test_members_readable(self, group, authz_policy):
         group.readable_by = ReadableBy.members
         assert authz_policy.permits(group, ['group:test-group'], 'read')
@@ -170,6 +175,10 @@ def test_members_flaggable(self, group, authz_policy):
         group.readable_by = ReadableBy.members
         assert authz_policy.permits(group, ['group:test-group'], 'flag')
 
+    def test_non_creator_members_do_not_have_moderate_permission(self, group, authz_policy):
+        group.readable_by = ReadableBy.members
+        assert not authz_policy.permits(group, ['group:test-group'], 'moderate')
+
     def test_not_readable(self, group, authz_policy):
         group.readable_by = None
         assert not authz_policy.permits(group, [security.Everyone, 'group:test-group'], 'read')
@@ -190,6 +199,9 @@ def test_not_writeable(self, group, authz_policy):
         group.writeable_by = None,
         assert not authz_policy.permits(group, ['authority:example.com', 'group:test-group'], 'write')
 
+    def test_creator_has_moderate_permission(self, group, authz_policy):
+        assert authz_policy.permits(group, 'acct:luke@example.com', 'moderate')
+
     def test_creator_has_admin_permissions(self, group, authz_policy):
         assert authz_policy.permits(group, 'acct:luke@example.com', 'admin')
 
@@ -199,6 +211,12 @@ def test_no_admin_permission_when_no_creator(self, group, authz_policy):
         principals = authz_policy.principals_allowed_by_permission(group, 'admin')
         assert len(principals) == 0
 
+    def test_no_moderate_permission_when_no_creator(self, group, authz_policy):
+        group.creator = None
+
+        principals = authz_policy.principals_allowed_by_permission(group, 'moderate')
+        assert len(principals) == 0
+
     def test_fallback_is_deny_all(self, group, authz_policy):
         assert not authz_policy.permits(group, [security.Everyone], 'foobar')
 
@@ -214,6 +232,3 @@ def group(self):
                              creator=creator)
         group.pubid = 'test-group'
         return group
-
-    def permissions(self, acl):
-        return [term[-1] for term in acl]

tests/h/traversal/contexts_test.py

@@ -37,6 +37,7 @@ def test_acl_private(self, factories, group_service, links_service):
         ann = factories.Annotation(shared=False, userid='saoirse')
         res = AnnotationContext(ann, group_service, links_service)
         actual = res.__acl__()
+        # Note NOT the ``moderate`` permission
         expect = [(security.Allow, 'saoirse', 'read'),
                   (security.Allow, 'saoirse', 'flag'),
                   (security.Allow, 'saoirse', 'admin'),
@@ -69,7 +70,7 @@ def test_acl_deleted(self, factories, group_service, links_service):
         ann = factories.Annotation(userid='saoirse', deleted=True)
         res = AnnotationContext(ann, group_service, links_service)
 
-        for perm in ['read', 'admin', 'update', 'delete']:
+        for perm in ['read', 'admin', 'update', 'delete', 'moderate']:
             assert not policy.permits(res, ['saiorse'], perm)
 
     @pytest.mark.parametrize('groupid,userid,permitted', [
@@ -162,6 +163,52 @@ def test_acl_flag_shared(self,
         else:
             assert not pyramid_request.has_permission('flag', res)
 
+    @pytest.mark.parametrize('groupid,userid,permitted', [
+        ('freeforall', 'jim', True),
+        ('freeforall', 'saoirse', True),
+        ('freeforall', None, False),
+        ('only-saoirse', 'jim', False),
+        ('only-saoirse', 'saoirse', True),
+        ('only-saoirse', None, False),
+        ('pals', 'jim', True),
+        ('pals', 'saoirse', True),
+        ('pals', 'francis', False),
+        ('pals', None, False),
+        ('unknown-group', 'jim', False),
+        ('unknown-group', 'saoirse', False),
+        ('unknown-group', 'francis', False),
+        ('unknown-group', None, False),
+    ])
+    def test_acl_moderate_shared(self,
+                                 factories,
+                                 pyramid_config,
+                                 pyramid_request,
+                                 groupid,
+                                 userid,
+                                 permitted,
+                                 group_service,
+                                 links_service):
+        """
+        Moderate permissions should only be applied when an annotation
+        is shared—as the annotation here is shared, anyone set as a principal
+        for the given ``FakeGroup`` will receive the ``moderate`` permission.
+        """
+        # Set up the test with a dummy authn policy and a real ACL authz
+        # policy:
+        policy = ACLAuthorizationPolicy()
+        pyramid_config.testing_securitypolicy(userid)
+        pyramid_config.set_authorization_policy(policy)
+
+        ann = factories.Annotation(shared=True,
+                                   userid='mioara',
+                                   groupid=groupid)
+        res = AnnotationContext(ann, group_service, links_service)
+
+        if permitted:
+            assert pyramid_request.has_permission('moderate', res)
+        else:
+            assert not pyramid_request.has_permission('moderate', res)
+
     @pytest.fixture
     def groups(self):
         return {
@@ -310,8 +357,16 @@ def __init__(self, principals):
             acl.append((security.Allow, p, 'read'))
             if p == security.Everyone:
                 acl.append((security.Allow, security.Authenticated, 'flag'))
+                acl.append((security.Allow, security.Authenticated, 'moderate'))
             else:
                 acl.append((security.Allow, p, 'flag'))
+                # Normally, the ``moderate`` permission would only be applied
+                # to the admin (creator) of a group, but this ``FakeGroup``
+                # is indeed fake. Tests in this module are merely around whether
+                # this permission is translated appropriately from a group
+                # to an annotation context (i.e. it should not be applied
+                # to private annotations)
+                acl.append((security.Allow, p, 'moderate'))
         self.__acl__ = acl