Merge pull request #5437 from hypothesis/allow-auth-group-patch

Allow AuthClients to `PATCH groups/{id}` without Forwarded User

docs/_extra/api-reference/hypothesis.yaml

@@ -379,7 +379,7 @@ paths:
       summary: Update a  group
       operationId: patchGroup
       description: >
-        Update an existing group. The authenticated user must be the group's original creator to have permission to update it.
+        Update an existing group. If `developerAPIKey` authentication is used, the authenticated user must be the group's creator. If `authClientCredentials` or `authClientForwardedUser` are used, the group's authority must match the client's authority.
       parameters:
         - name: id
           in: path
@@ -407,6 +407,7 @@ paths:
             $ref: '#/definitions/ConflictError'
       security:
         - authClientForwardedUser: []
+        - authClientCredentials: []
         - developerAPIKey: []
 
     put:

h/models/group.py

@@ -187,11 +187,13 @@ def is_public(self):
 
     def __acl__(self):
         terms = []
+        # This authority principal may be used to grant auth clients
+        # permissions for groups within their authority
+        authority_principal = "client_authority:{}".format(self.authority)
 
         # auth_clients that have the same authority as the target group
         # may add members to it
-        member_add_principal = "client_authority:{}".format(self.authority)
-        terms.append((security.Allow, member_add_principal, 'member_add'))
+        terms.append((security.Allow, authority_principal, 'member_add'))
 
         join_principal = _join_principal(self)
         if join_principal is not None:
@@ -210,7 +212,11 @@ def __acl__(self):
             terms.append((security.Allow, write_principal, 'write'))
 
         if self.creator:
+            # The creator of the group should be able to update it
             terms.append((security.Allow, self.creator.userid, 'admin'))
+            # auth_clients that have the same authority as this group
+            # should be allowed to update it
+            terms.append((security.Allow, authority_principal, 'admin'))
             terms.append((security.Allow, self.creator.userid, 'moderate'))
             # The creator may update this group in an upsert context
             terms.append((security.Allow, self.creator.userid, 'upsert'))

tests/functional/api/groups/test_update.py

@@ -123,24 +123,39 @@ def test_it_allows_auth_client_with_valid_forwarded_user(self,
         assert res.status_code == 200
         assert res.json_body['name'] == 'My Group'
 
-    def test_it_returns_404_if_forwarded_user_is_not_authorized(self,
-                                                                app,
-                                                                auth_client_header,
-                                                                third_party_user,
-                                                                factories,
-                                                                db_session):
-        # Not created by `third_party_user`
-        group = factories.Group(authority=third_party_user.authority)
+    def test_it_allows_auth_client_with_matching_authority(self,
+                                                           app,
+                                                           auth_client_header,
+                                                           third_party_user,
+                                                           factories,
+                                                           db_session):
+        group = factories.Group(creator=third_party_user, authority=third_party_user.authority)
         db_session.commit()
 
-        headers = auth_client_header
-        headers[native_str('X-Forwarded-User')] = native_str(third_party_user.userid)
         group_payload = {
             'name': 'My Group'
         }
 
         path = '/api/groups/{id}'.format(id=group.pubid)
-        res = app.patch_json(path, group_payload, headers=headers, expect_errors=True)
+        res = app.patch_json(path, group_payload, headers=auth_client_header)
+
+        assert res.status_code == 200
+        assert res.json_body['name'] == 'My Group'
+
+    def test_it_does_not_allow_auth_client_with_mismatched_authority(self,
+                                                                     app,
+                                                                     auth_client_header,
+                                                                     factories,
+                                                                     db_session):
+        group = factories.Group(authority='rando.biz')
+        db_session.commit()
+
+        group_payload = {
+            'name': 'My Group'
+        }
+
+        path = '/api/groups/{id}'.format(id=group.pubid)
+        res = app.patch_json(path, group_payload, headers=auth_client_header, expect_errors=True)
 
         assert res.status_code == 404
 

tests/h/models/group_test.py

@@ -277,9 +277,19 @@ def test_not_writeable(self, group, authz_policy):
     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):
+    def test_creator_has_admin_permission(self, group, authz_policy):
         assert authz_policy.permits(group, 'acct:luke@example.com', 'admin')
 
+    def test_auth_client_with_matching_authority_has_admin_permission(self, group, authz_policy):
+        group.authority = 'weewhack.com'
+
+        assert authz_policy.permits(group, ['flip', 'client_authority:weewhack.com'], 'admin')
+
+    def test_auth_client_without_matching_authority_does_not_have_admin_permission(self, group, authz_policy):
+        group.authority = 'weewhack.com'
+
+        assert not authz_policy.permits(group, ['flip', 'client_authority:2weewhack.com'], 'admin')
+
     def test_creator_has_upsert_permissions(self, group, authz_policy):
         assert authz_policy.permits(group, 'acct:luke@example.com', 'upsert')