Allow group members to remove other members from groups #9076

seanh · 2024-11-14T13:01:33Z

Extend the remove-member-from-group API (DELETE /groups/{id}/members/{user}) to allow group members to remove other members (not just themselves) from groups, assuming the authenticated member has the necessary role in the group.

Currently the only valid call to this API is DELETE /groups/{id}/members/me (i.e. {user} is the literal string me) to remove yourself from a group.

While keeping the me alias this PR also enables {user} to be either your own or someone else's userid, and will allow or deny the request according to these rules:

Any group member can remove themselves from a group with either me or their own userid
Only owners can remove other owners and admins
Only owners and admins can remove moderators
Owners, admins and moderators can remove plain members
Plain members, people who aren't members of the group at all, and unauthenticated requests can't remove anyone
Also, it'll 404 if either the group or the target user doesn't exist, or if the target user isn't a member of the group
There's also a separate code path for when the userid is invalid (can't be parsed in acct:{username}@{authority} format), 404s

Testing

Test removing yourself

Log in (http://localhost:5000/login)
Generate an API token (http://localhost:5000/account/developer)
Create a group (http://localhost:5000/groups/new)

Remove yourself from the group:

httpx http://localhost:5000/api/groups/{pubid}/members/acct:{username}@localhost --method DELETE --headers Authorization 'Bearer {apitoken}'

Or with the "me" alias:

httpx http://localhost:5000/api/groups/{pubid}/members/me --method DELETE --headers Authorization 'Bearer {apitoken}'

If you repeat the same request again you should get a 404 because the membership doesn't exist.

Test an owner removing a plain member

Log in
Generate an API token
Create a group
Log in as a different user
Join the group
Make an API request as the first user to remove the second user from the group

Test that plain members can't remove owners

Log in
Generate an API token
Create a group
Log in as a different user
Join the group
Make an API request as the second user to remove the first user from the group. You should get a 404

Test that non-members can't remove members from groups

Log in
Generate an API token
Create a group
Make an API request as a different user to remove the user from the group. You should get a 404

Test that unauthenticated requests can't remove members from groups

Log in
Generate an API token
Create a group
Make an API request without an Authorization headerto remove the user from the group. You should get a 404

seanh · 2024-11-21T18:02:45Z

h/traversal/__init__.py

@@ -77,4 +81,6 @@
    "UserByIDRoot",
    "UserRoot",
    "GroupContext",
+    "GroupMembershipContext",
+    "group_membership_api_factory",


Should really tidy up thie __init__.py file, there's a bunch of stuff in it that doesn't need to be, and the __all__ is unnecessary.

Newest ruff release check (and fixes) the order of these:

https://docs.astral.sh/ruff/rules/unsorted-dunder-all/

Although if I'm not mistaken this is only relevant for * imports. Maybe not that useful for application code like ours (vs libraries)

We could probably remove most of these.

seanh · 2024-11-21T19:04:02Z

h/routes.py

@@ -159,8 +159,7 @@ def includeme(config):  # pylint: disable=too-many-statements
    config.add_route(
        "api.group_member",
        "/api/groups/{pubid}/members/{userid}",
-        factory="h.traversal.GroupRequiredRoot",
-        traverse="/{pubid}",
+        factory="h.traversal.group_membership_api_factory",


The group membership APIs were sharing the same factory (GroupRequiredRoot) with other group-related views like: web pages for viewing and editing groups, admin pages for editing and deleting groups, group GET and PATCH APIs, and the API for getting a list of a group's members. But a group is not the correct context object for group membership API requests: a group membership is. So changing the group membership API route to use its own separate factory that will return group membership context objects.

We don't need the traverse argument: there's no need for h to use traversal and the Pyramid docs recommend against it except for specific use-cases that benefit from traversal, it's simpler without it (see below).

(Some other routes in h do currently use traversal but it's unnecessary--we should simplify it at some point.)

Note that this route is used by two separate API views currently, plus two more views that may be added in future:

add_member() (POST /api/groups/{pubid}/members/{userid}): this is an existing API that's not the remove-member API that this PR is about. The behavior of the add-member API should be unaffected by this PR apart from one small change noted below.

remove_member() (DELETE /api/groups/{pubid}/members/{userid}): another existing API, this is the API that this PR will extend to allow users to remove members other than themselves.

edit_member() (PATCH /api/groups/{pubid}/members/{userid} with new roles in JSON body): this view doesn't exist yet but will be added in a future PR.

get_member() (GET /api/groups/{pubid}/members/{userid}): doesn't exist yet and may not be added because it isn't needed for the current project.

seanh · 2024-11-21T19:30:13Z

h/traversal/group_membership.py

+@dataclass
+class GroupMembershipContext:
+    group: Group
+    user: User
+    membership: GroupMembership | None


This is the new context object that will be passed to security permissions predicate functions and to views for the GET, POST, PATCH and DELETE /api/groups/{pubid}/members/{userid} APIs. This is a more appropriate context object for group membership views: as well as providing convenient access to context.group and context.user it also provides the membership itself including context.membership.roles.

These views previously used GroupContext and this new GroupMembershipContext is backwards-compatible with GroupContext (it has the same group attribute and adds user and membership) so changing to this context class shouldn't break anything.

seanh · 2024-11-21T19:36:27Z

h/traversal/group_membership.py

+def group_membership_api_factory(request) -> GroupMembershipContext:
+    user_service = request.find_service(name="user")
+    group_service = request.find_service(name="group")
+    group_members_service = request.find_service(name="group_members")
+
+    userid = request.matchdict["userid"]
+    pubid = request.matchdict["pubid"]
+
+    def get_user() -> User | None:
+        if userid == "me":
+            if request.authenticated_userid:
+                return user_service.fetch(request.authenticated_userid)
+
+            return None
+
+        try:
+            return user_service.fetch(userid)
+        except InvalidUserId:
+            return None
+
+    user = get_user()
+
+    if not user:
+        raise HTTPNotFound(f"User not found: {userid}")
+
+    group = group_service.fetch(pubid)
+
+    if not group:
+        raise HTTPNotFound(f"Group not found: {pubid}")
+
+    membership = group_members_service.get(group, user)
+
+    if not membership and request.method != "POST":
+        raise HTTPNotFound(f"Membership not found: ({pubid}, {userid})")
+
+    return GroupMembershipContext(group=group, user=user, membership=membership)


The factory="h.traversal.group_membership_api_factory" in the route config means that Pyramid will call group_membership_api_factory() to get the context object for requests that match the "api.group_member" route.

This function either returns a GroupMembershipContext object or raises HTTPNotFound which causes Pyramid to 404 the request without ever calling the security policy or view.

This has some big advantages:

There's a nice separation of concerns where this factory is responsible for finding the group, user and membership (and handling what to do when they don't exist) and then the security policy and view can assume that if they're called then the the group, user and membership do exist.

This factory can do the DB queries to look up these objects once and other code can access them via the context argument, avoiding duplicate DB queries.

h can return 404s for resources that don't exist, and 401s for resources that the request doesn't have permission to act on.

If you don't use a context factory you get a problem. If someone makes an API request with an unknown group ID or unknown user ID in the URL, or with a group ID and user ID for which there is no membership, then Pyramid will end up calling the security policy which must return either Allowed or Denied, neither of which is a good choice:

It doesn't seem quite right to me for the security policy to return Denied in this situation. It's not that the request doesn't have permission to edit the membership, the problem is that the membership doesn't exist. Returning Denied would cause Pyramid to send a 401 response, when a 404 would be more appropriate.

The security policy could return Allowed and then the view could raise HTTPNotFound. This would return the correct response (404) but it seems wrong to me for the security policy to say that the request is permitted to edit a membership that doesn't exist and I imagine there could even be race conditions resulting in security issues (if a membership with the matching IDs gets created by a concurrent request and it happens to be a membership that the API request should not have been permitted to edit).

Using a context factory means that if the resources don't exist the security policy doesn't even get called, avoiding the issue.

seanh · 2024-11-21T19:42:05Z

h/traversal/group_membership.py

+    if not membership and request.method != "POST":
+        raise HTTPNotFound(f"Membership not found: ({pubid}, {userid})")


Special case for POST /api/groups/{pubid}/members/{userid}: this request is creating a new membership, so it doesn't expect the membership to already exist and shouldn't 404 if it doesn't. An alternative implementation would be for POST requests to be routed to a different context factory (possibly even returning a different context object).

Note that this doesn't guarantee that the membership doesn't already exist for POST requests: the view code itself has to handle the case of trying to create a membership that doesn't exist. For example the view could raise an error. Or it could make this case a no-op.

The current behavior on main is that adding a membership that already exists is a no-op. This is possible because the add-member API currently doesn't let you set the role when adding a member (it just always defaults to "member"). In the future if we let people specify the role when adding a member then we may need to change the current behavior: if you try to add a membership with a given role and it already exists with a different role, we'd need to either change the role or return an error. But we're not going to need to support roles in add-member for the current project so we can defer that.

seanh · 2024-11-21T19:44:08Z

h/traversal/group_membership.py

+        if userid == "me":
+            if request.authenticated_userid:
+                return user_service.fetch(request.authenticated_userid)
+
+            return None


The API supports me as an alias for your own user ID. On main me is handled in the view directly but we're going to want this me support in the upcoming PATCH API as well (and also in a GET API if we ever add one), moving it into the context factory here means that all the views support it for free.

This means that the existing POST API for adding a member to a group now supports me to add yourself to a group, which is not needed for any particular reason but it's fine and nice to have me supported consistently.

seanh · 2024-11-21T19:45:40Z

h/views/api/groups.py

@@ -183,7 +174,7 @@ def remove_member(context: GroupContext, request):
    permission=Permission.Group.MEMBER_ADD,
    description="Add the user in the request params to a group.",
 )
-def add_member(context: GroupContext, request):
+def add_member(context: GroupMembershipContext, request):


GroupMembershipContext is backwards-compatible with GroupContext so no changes should be needed to add_member() or to any permissions predicate functions mapped to the MEMBER_ADD permission that add_member() uses.

seanh · 2024-11-21T19:46:15Z

h/views/api/groups.py

-    # Currently, we only support removing the requesting user
-    if request.matchdict.get("userid") == "me":
-        userid = request.authenticated_userid
-    else:
-        raise HTTPBadRequest('Only the "me" user value is currently supported')


me is handled by the context factory now and has already been resolved before the view is called (see above).

seanh · 2024-11-22T16:42:44Z

h/views/api/groups.py

-    try:
-        user = user_svc.fetch(request.matchdict["userid"])
-    except InvalidUserId as err:
-        raise HTTPNotFound() from err
-
-    if user is None:
-        raise HTTPNotFound()


The add_member() view no longer needs to handle 404ing if the user doesn't exist: the context factory will have already done that before the view is called.

marcospri

Looks good and works as expected.

I added a bunch of comments here but they are mostly me adding notes while following the code structure.

marcospri · 2024-11-25T10:16:31Z

h/security/predicates.py

@@ -182,6 +183,42 @@ def group_matches_authenticated_client_authority(identity, context):
    return context.group.authority == identity.auth_client.authority


+@requires(authenticated_user, group_found)
+def group_member_remove(identity, context: GroupMembershipContext):


Not sure If mypy could use the type info here but I find it very useful, it can be tricky to know what exactly context is without checking the route and view definitions.

Yep 👍 I think type annotations are very helpful for these context objects

marcospri · 2024-11-25T10:19:21Z

h/security/predicates.py

+@requires(authenticated_user, group_found)
+def group_member_remove(identity, context: GroupMembershipContext):
+    def get_authenticated_users_membership():
+        """Return the authenticated users membership in the target group."""


I struggling to parse this sentence (but I could be a me problem)

Is users here a plural? Is this the same as authenticated_user_membership?

It's mean to be possessive and is missing an apostrophe, does this make it clearer?

Suggested change

"""Return the authenticated users membership in the target group."""

"""Return the authenticated user's membership of the target group."""

marcospri · 2024-11-25T10:25:00Z

h/services/group_members.py

@@ -24,6 +24,14 @@ def __init__(self, db, user_fetcher, publish):
        self.user_fetcher = user_fetcher
        self.publish = publish

+    def get(self, group, user) -> GroupMembership | None:


I had suggestions in the past to name this type of method more verbosity, eg: get_membership.

I initially thought that was duplicating info as we are already scoped to the members service but I've come around to use that naming scheme, the vast improvement on "greppeability" offsets the small extra typing IMO.

Does it really improve greppability? group_members_service.get() is already greppable if we use a consistent variable naming style, but people do tend to be inconsistent here and sometimes use group_members_svc etc so maybe get_membership() is better for enforcing greppability.

marcospri · 2024-11-25T10:29:54Z

h/traversal/__init__.py

@@ -77,4 +81,6 @@
    "UserByIDRoot",
    "UserRoot",
    "GroupContext",
+    "GroupMembershipContext",
+    "group_membership_api_factory",


Newest ruff release check (and fixes) the order of these:

https://docs.astral.sh/ruff/rules/unsorted-dunder-all/

Although if I'm not mistaken this is only relevant for * imports. Maybe not that useful for application code like ours (vs libraries)

We could probably remove most of these.

marcospri · 2024-11-25T10:37:30Z

h/security/predicates.py

@@ -182,6 +183,42 @@ def group_matches_authenticated_client_authority(identity, context):
    return context.group.authority == identity.auth_client.authority


+@requires(authenticated_user, group_found)
+def group_member_remove(identity, context: GroupMembershipContext):


Is the word "predicate" in this module meant to be "pyramid view predicate"? Or just what we called these?

It's just what Jon called these although he's using the English-language word "predicate" in the same sense as Pyramid uses it in "view predicate": as in all the predicates need to be true in order for the permission to be granted or the view to be called. But these "security predicate functions" are entirely Jon's invention, not a Pyramid thing

marcospri · 2024-11-25T10:42:29Z

h/security/predicates.py

+def group_member_remove(identity, context: GroupMembershipContext):
+    def get_authenticated_users_membership():
+        """Return the authenticated users membership in the target group."""
+        for membership in identity.user.memberships:


Coming back here after reading the context

This iterates over the current user memberships and find the one relevant for *target" group.

marcospri · 2024-11-25T10:43:50Z

h/security/predicates.py

+
+        return None
+
+    membership = get_authenticated_users_membership()


current_uer_membership? There's two memberships here and it's tricky to keep track of

membership. Current user membership

anything on context. Target of the API request.

Renamed this to authenticated_users_membership (to match with the get_authenticated_users_membership() function above, and also with request.authenticated_user etc). A bit long but at least it's unambiguous.

I think generally speaking in security predicates the request is about the authenticated user (request.authenticated_user_id, request.authenticated_user, request.identity, etc, security predicates don't usually access other things like the request path, query params, body, etc) whereas anything on the context is about the resource(s) that the request targets (context.user is the target user, context.group is the target group, etc).

marcospri · 2024-11-25T11:00:19Z

h/views/api/groups.py

        raise HTTPNotFound()

-    group_members_svc.member_join(context.group, user.userid)
+    group_members_svc.member_join(context.group, context.user.userid)


context contains the "target" of the request, add the target user to the target group.

marcospri · 2024-11-25T11:05:47Z

h/traversal/group_membership.py

+@dataclass
+class GroupMembershipContext:
+    group: Group


Not a suggestion, just checking my understanding of the context here:

Could we have

class GroupMembershipContext: target_group: Group target_user: User target_membership: GroupMembership | None autheticated_user: User autheticaed_user_membership: GroupMembership | None

or those last two are just duplicating the info over identity and making the context less reusable?

Yeah, I think it's wrong to include information about the authenticated user in the context: request.identity is about the authenticated user, whereas context is about the target resource(s). For example h has this identity_permits(identity, context, permission) function and you can do things like this:

context = FooContext() for user in users: identity = Identity.from_models(user) if identity_permits(identity, context, "foo_permission"): pass

...that is, you can construct a single context object and use it repeatedly to ask whether different identities have a given permission in that context.

I haven't tried it but I think you could do a similar thing in pure Pyramid (without h's custom identity_permits() function) by constructing requests:

context = FooContext() for user in users: request = Request(...) if request.has_permission(context, "foo_permission"): pass

...but it's more difficult because you need to somehow create a request object that's authenticated as the user in question. May need to use paster_bootstrap()

Extend the remove-member-from-group API (`DELETE /groups/{id}/members/{user}`) to allow group members to remove _other_ members (not just themselves) from groups, assuming the authenticated member has the necessary role in the group. Currently the only valid call to this API is `DELETE /groups/{id}/members/me` (i.e. `{user}` is the literal string `me`) to remove yourself from a group. While keeping the `me` alias this commit also enables `{user}` to be either your own or someone else's userid, and will allow or deny the request according to these rules: * Any group member can remove themselves from a group with either `me` or their own userid * Only owners can remove other owners and admins * Only owners and admins can remove moderators * Owners, admins and moderators can remove plain members * Plain members, people who aren't members of the group at all, and unauthenticated requests can't remove anyone * Also, it'll 404 if either the group or the target user doesn't exist, or if the target user isn't a member of the group * There's also a separate code path for when the userid is invalid (can't be parsed in `acct:{username}@{authority}` format), 404s

seanh requested a review from marcospri November 14, 2024 13:01

seanh changed the title ~~extend remove member api~~ All group members to remove other members from groups Nov 14, 2024

seanh force-pushed the extend-remove-member-api branch 2 times, most recently from bc75726 to 83fb543 Compare November 14, 2024 17:05

seanh changed the title ~~All group members to remove other members from groups~~ Allow group members to remove other members from groups Nov 14, 2024

seanh force-pushed the extend-remove-member-api branch from 83fb543 to 72e7efa Compare November 14, 2024 18:01

seanh force-pushed the iterate-over-user-memberships branch from ec09eea to 0fa6302 Compare November 21, 2024 12:52

Base automatically changed from iterate-over-user-memberships to main November 21, 2024 12:56

seanh force-pushed the extend-remove-member-api branch from 72e7efa to a259082 Compare November 21, 2024 13:07

seanh commented Nov 21, 2024

View reviewed changes

seanh force-pushed the extend-remove-member-api branch from 2000cd7 to 6b18f3b Compare November 22, 2024 16:09

seanh changed the base branch from main to improve-add-member-api-functests November 22, 2024 16:10

seanh commented Nov 22, 2024

View reviewed changes

seanh force-pushed the extend-remove-member-api branch from 6b18f3b to 654b7c6 Compare November 22, 2024 18:12

seanh marked this pull request as ready for review November 22, 2024 18:12

marcospri approved these changes Nov 25, 2024

View reviewed changes

Base automatically changed from improve-add-member-api-functests to main November 26, 2024 11:46

seanh force-pushed the extend-remove-member-api branch from 654b7c6 to 41c247f Compare November 26, 2024 12:32

seanh added 4 commits November 26, 2024 12:37

Tweak a docstring

995bda2

Make a variable name more explicit

50ac6f2

Fix an incorrect comment

e992996

Rename a method to be more greppable

b5893b8

seanh force-pushed the extend-remove-member-api branch from 0387223 to b5893b8 Compare November 26, 2024 13:11

seanh merged commit 354286a into main Nov 26, 2024
9 checks passed

seanh deleted the extend-remove-member-api branch November 26, 2024 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow group members to remove other members from groups #9076

Allow group members to remove other members from groups #9076

seanh commented Nov 14, 2024 •

edited

Loading

seanh Nov 21, 2024

marcospri Nov 25, 2024

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 21, 2024

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 21, 2024

seanh Nov 21, 2024

seanh Nov 22, 2024

marcospri left a comment

marcospri Nov 25, 2024

seanh Nov 26, 2024

marcospri Nov 25, 2024

seanh Nov 26, 2024 •

edited

Loading

marcospri Nov 25, 2024

seanh Nov 26, 2024 •

edited

Loading

marcospri Nov 25, 2024

marcospri Nov 25, 2024

seanh Nov 26, 2024

marcospri Nov 25, 2024

marcospri Nov 25, 2024

seanh Nov 26, 2024

marcospri Nov 25, 2024

marcospri Nov 25, 2024

seanh Nov 26, 2024 •

edited

Loading

		if not membership and request.method != "POST":
		raise HTTPNotFound(f"Membership not found: ({pubid}, {userid})")

	"""Return the authenticated users membership in the target group."""
	"""Return the authenticated user's membership of the target group."""


		return None

		membership = get_authenticated_users_membership()

Allow group members to remove other members from groups #9076

Allow group members to remove other members from groups #9076

Conversation

seanh commented Nov 14, 2024 • edited Loading

Testing

Test removing yourself

Test an owner removing a plain member

Test that plain members can't remove owners

Test that non-members can't remove members from groups

Test that unauthenticated requests can't remove members from groups

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

seanh Nov 21, 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

marcospri left a 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 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Nov 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

seanh commented Nov 14, 2024 •

edited

Loading

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 21, 2024 •

edited

Loading

seanh Nov 26, 2024 •

edited

Loading

seanh Nov 26, 2024 •

edited

Loading

seanh Nov 26, 2024 •

edited

Loading