Merge pull request #5330 from hypothesis/refactor-flag-permission

Refactor new `flag` permission

h/models/group.py

@@ -134,6 +134,10 @@ def __acl__(self):
         if read_principal is not None:
             terms.append((security.Allow, read_principal, 'read'))
 
+        flag_principal = _flag_principal(self)
+        if flag_principal is not None:
+            terms.append((security.Allow, flag_principal, 'flag'))
+
         write_principal = _write_principal(self)
         if write_principal is not None:
             terms.append((security.Allow, write_principal, 'write'))
@@ -166,6 +170,16 @@ def _read_principal(group):
     }.get(group.readable_by)
 
 
+def _flag_principal(group):
+    # If a user can read (see) annotations within this group,
+    # they can also flag them—but they need to be logged in
+    # (``pyramid.security.Authenticated``)
+    return {
+        ReadableBy.members: 'group:{}'.format(group.pubid),
+        ReadableBy.world: security.Authenticated,
+    }.get(group.readable_by)
+
+
 def _write_principal(group):
     return {
         WriteableBy.authority: 'authority:{}'.format(group.authority),

h/traversal/contexts.py

@@ -22,7 +22,7 @@
 from __future__ import unicode_literals
 
 from pyramid.security import DENY_ALL
-from pyramid.security import Allow, Everyone, Authenticated
+from pyramid.security import Allow
 from pyramid.security import principals_allowed_by_permission
 
 from h.models.organization import ORGANIZATION_DEFAULT_PUBID
@@ -56,15 +56,12 @@ def __acl__(self):
 
         acl = []
         if self.annotation.shared:
-            for principal in self._group_principals(self.group):
+            for principal in self._group_principals(self.group, 'read'):
                 acl.append((Allow, principal, 'read'))
-                # If this annotation is readable by everyone, it should be
-                # flaggable by any authenticated user, i.e. replace
-                # ``security.Everyone`` with ``security.Authenticated``
-                if principal == Everyone:
-                    acl.append((Allow, Authenticated, 'flag'))
-                else:
-                    acl.append((Allow, principal, 'flag'))
+
+            for principal in self._group_principals(self.group, 'flag'):
+                acl.append((Allow, principal, 'flag'))
+
         else:
             acl.append((Allow, self.annotation.userid, 'read'))
             acl.append((Allow, self.annotation.userid, 'flag'))
@@ -78,10 +75,11 @@ def __acl__(self):
         return acl
 
     @staticmethod
-    def _group_principals(group):
+    def _group_principals(group, principal):
         if group is None:
             return []
-        return principals_allowed_by_permission(group, 'read')
+        principals = principals_allowed_by_permission(group, principal)
+        return principals
 
 
 class OrganizationContext(object):

tests/h/models/groups_test.py

@@ -157,14 +157,27 @@ 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):
+        group.readable_by = ReadableBy.world
+        assert authz_policy.permits(group, [security.Authenticated], 'flag')
+        assert not authz_policy.permits(group, [security.Everyone], 'flag')
+
     def test_members_readable(self, group, authz_policy):
         group.readable_by = ReadableBy.members
         assert authz_policy.permits(group, ['group:test-group'], 'read')
 
+    def test_members_flaggable(self, group, authz_policy):
+        group.readable_by = ReadableBy.members
+        assert authz_policy.permits(group, ['group:test-group'], 'flag')
+
     def test_not_readable(self, group, authz_policy):
         group.readable_by = None
         assert not authz_policy.permits(group, [security.Everyone, 'group:test-group'], 'read')
 
+    def test_not_flaggable(self, group, authz_policy):
+        group.readable_by = None
+        assert not authz_policy.permits(group, [security.Authenticated, 'group:test-group'], 'flag')
+
     def test_authority_writeable(self, group, authz_policy):
         group.writeable_by = WriteableBy.authority
         assert authz_policy.permits(group, ['authority:example.com'], 'write')

tests/h/traversal/contexts_test.py

@@ -296,8 +296,23 @@ def test_default_property_if_default_organization(self, factories, pyramid_reque
 
 
 class FakeGroup(object):
+    # NB: Tests that use this do not validate that the principals are correct
+    # for the indicated group. They validate that those principals are being
+    # transferred over to the annotation as expected
+    # As such, this has sort of a partial and wonky re-implementation of
+    # ``h.models.Group.__acl__``
+    # This is a symptom of the disease that is splitting ACL concerns between
+    # traversal/resources and model classes
+    # TODO: Refactor once we're able to move ACLs off of models
     def __init__(self, principals):
-        self.__acl__ = [(security.Allow, p, 'read') for p in principals]
+        acl = []
+        for p in principals:
+            acl.append((security.Allow, p, 'read'))
+            if p == security.Everyone:
+                acl.append((security.Allow, security.Authenticated, 'flag'))
+            else:
+                acl.append((security.Allow, p, 'flag'))
+        self.__acl__ = acl
 
 
 @pytest.fixture