Add an API for editing a group membership's role #9114

seanh · 2024-11-23T04:52:25Z

Add an API for editing a group member's role:

PATCH /api/groups/{pubid}/members/{userid}
{"roles": ["<new_role>"]}

Testing:

Log in as devdata_admin (http://localhost:5000/login)
Create an API token (http://localhost:5000/account/developer)
Create a group (http://localhost:5000/groups/new)
Log in as a devdata_user and join the group
Use devdata_admin's API token to promote devdata_user to admin: httpx http://localhost:5000/api/groups/{pubid}/members/acct:devdata_user@localhost --method PATCH --headers Authorization 'Bearer {apitoken}' --json '{"roles": ["admin"]}'
Use the "me" alias to demote yourself to a plain member: httpx http://localhost:5000/api/groups/{pubid}/members/me --method PATCH --headers Authorization 'Bearer {apitoken}' --json '{"roles": ["admin"]}'
Now that you're just a plain member if you try to promote yourself again or try to change devdata_user's role you'll get 404s.
If you try to use an invalid role you'll get a validation error, for example: httpx http://localhost:5000/api/groups/{pubid}/members/me --method PATCH --headers Authorization 'Bearer {apitoken}' --json '{"roles": ["INVALID"]}'

seanh · 2024-11-23T04:55:09Z

h/presenters/group_membership_json.py

+    def asdict(self):
+        return {
+            "authority": self.membership.group.authority,
+            "userid": self.membership.user.userid,
+            "username": self.membership.user.username,
+            "display_name": self.membership.user.display_name,
+            "roles": self.membership.roles,
+        }


This is compatible with the user objects currently returned by the get-group-members API just with roles added. (It was previously thought of as a list of users, but now we're thinking of it as a list of memberships.)

seanh · 2024-11-23T04:57:23Z

h/security/predicates.py

+@requires(authenticated_user, group_found)
+def group_member_edit(
+    identity, context: EditGroupMembershipContext
+):  # pylint:disable=too-many-return-statements,too-complex
+    old_roles = context.membership.roles
+    new_roles = context.new_roles
+
+    def get_authenticated_users_roles():
+        """Return the authenticated users roles in the target group."""
+        for membership in identity.user.memberships:
+            if membership.group.id == context.group.id:
+                return membership.roles
+
+        return None
+
+    authenticated_users_roles = get_authenticated_users_roles()
+
+    if not authenticated_users_roles:
+        return False
+
+    if identity.user.userid == context.user.userid:
+        if GroupMembershipRoles.OWNER in authenticated_users_roles:
+            # Owners can change their own role to anything.
+            return True
+
+        if GroupMembershipRoles.ADMIN in authenticated_users_roles:
+            # Admins can change their own role to anything but admin.
+            return GroupMembershipRoles.OWNER not in new_roles
+
+        if GroupMembershipRoles.MODERATOR in authenticated_users_roles:
+            # Moderators can change their own role to anything but owner or admin.
+            return (
+                GroupMembershipRoles.OWNER not in new_roles
+                and GroupMembershipRoles.ADMIN not in new_roles
+            )
+
+        return False
+
+    if GroupMembershipRoles.OWNER in authenticated_users_roles:
+        # Owners can change any other member's role to any role.
+        return True
+
+    if GroupMembershipRoles.ADMIN in authenticated_users_roles:
+        # Admins can change the role of anyone but owners or admins to anything
+        # but owner or admin.
+        if (
+            GroupMembershipRoles.OWNER in old_roles + new_roles
+            or GroupMembershipRoles.ADMIN in old_roles + new_roles
+        ):
+            return False
+
+        return True
+
+    return False


I think there should be a better way to implement this based on rank-ordering of the roles, rather than handling each case individually. (And the same for the group_member_remove() predicate in a previous PR as well.)

seanh · 2024-11-23T04:59:57Z

h/views/api/group_members.py

+    return [
+        GroupMembershipJSONPresenter(membership).asdict()
+        for membership in context.group.memberships
+    ]


The get-group-members API now uses the new GroupMembershipJSONPresenter instead of UserJSONPresenter. It returns the same dicts as UserJSONPresenter does but with roles added. This means that this PR also adds roles to the get-group-members API responses.

seanh · 2024-11-23T05:00:17Z

h/views/api/group_members.py

@@ -28,7 +36,6 @@ def list_members(context: GroupContext, _request):
    permission=Permission.Group.MEMBER_REMOVE,
 )
 def remove_member(context: GroupMembershipContext, request):
-    """Remove a member from a group."""


Removing these pointless docstrings.

seanh · 2024-11-23T05:04:55Z

h/views/api/group_members.py

+    if not request.has_permission(
+        Permission.Group.MEMBER_EDIT,
+        EditGroupMembershipContext(
+            context.group, context.user, context.membership, new_roles
+        ),
+    ):
+        raise HTTPNotFound()


There's no permission in the @api_config() on this view. The reason is that whether or not you have permission to change a role depends on the new role that you want to change it to (only owners can change someone's role to owner or admin, admins can change someone's role to moderator or member, etc).

The new role comes from the request JSON body and isn't available until that JSON body has been parsed and validated, and that happens in the view. So we can't use Pyramid's permission thing where it calls the security policy before calling the view.

Instead we let Pyramid call the view regardless, and then the view itself constructs a context object and calls request.has_permission().

We do need the permissions logic to be implemented in the centralized permissions system because in future another view is going to have to check MEMBER_EDIT permissions as well, so we couldn't just code the logic directly in this view. Also permissions logic just belongs in the permissions system.

seanh · 2024-11-23T05:05:17Z

h/views/api/group_members.py

+    ):
+        raise HTTPNotFound()
+
+    context.membership.roles = new_roles


There's no need for a service to carry out the business logic, it's just an assignment.

seanh · 2024-11-23T05:06:55Z

h/views/api/group_members.py

+        old_roles,
+    )
+
+    return GroupMembershipJSONPresenter(context.membership).asdict()


The edit_member() and list_members() views both use GroupMembershipJSONPresenter so they both return groups in the same format.

If we ever add a get_member() view it can use the same presenter as well.

add_member() could also use it as well but that might be a breaking change (currently it returns an HTTP 204 with no body).

seanh · 2024-11-23T05:09:16Z

tests/functional/api/groups/members_test.py

+        assert res.json == [
+            {
+                "authority": membership.group.authority,
+                "userid": membership.user.userid,
+                "username": membership.user.username,
+                "display_name": membership.user.display_name,
+                "roles": membership.roles,
+            }
+            for membership in group.memberships
+        ]


Made this test assertion more specific so I can see exactly what changes I'm making to the get-group-members API (adding the roles)

seanh · 2024-11-23T05:10:02Z

tests/functional/api/groups/members_test.py

+        assert res.json == [
+            {
+                "authority": membership.group.authority,
+                "userid": membership.user.userid,
+                "username": membership.user.username,
+                "display_name": membership.user.display_name,
+                "roles": membership.roles,
+            }
+            for membership in group.memberships
+        ]


seanh requested a review from marcospri November 23, 2024 04:52

seanh commented Nov 23, 2024

View reviewed changes

seanh force-pushed the add-edit-membership-api- branch from 0c02bd7 to d0791b7 Compare November 23, 2024 05:07

seanh commented Nov 23, 2024

View reviewed changes

seanh force-pushed the add-edit-membership-api- branch from d0791b7 to 79efb82 Compare November 23, 2024 05:10

Add an API for editing a group membership's role

702c89e

seanh force-pushed the add-edit-membership-api- branch from 79efb82 to 702c89e Compare November 23, 2024 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an API for editing a group membership's role #9114

Add an API for editing a group membership's role #9114

seanh commented Nov 23, 2024 •

edited

Loading

seanh Nov 23, 2024

seanh Nov 23, 2024

seanh Nov 23, 2024

seanh Nov 23, 2024

seanh Nov 23, 2024

seanh Nov 23, 2024

seanh Nov 23, 2024 •

edited

Loading

seanh Nov 23, 2024

seanh Nov 23, 2024

Add an API for editing a group membership's role #9114

Are you sure you want to change the base?

Add an API for editing a group membership's role #9114

Conversation

seanh commented Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh commented Nov 23, 2024 •

edited

Loading

seanh Nov 23, 2024 •

edited

Loading