From 654b7c64dced5cafc1ad6f28ef939f98894b87ed Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Thu, 14 Nov 2024 12:09:43 +0000 Subject: [PATCH] Allow members to remove other members from groups 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 --- h/routes.py | 3 +- h/security/permission_map.py | 1 + h/security/permissions.py | 3 + h/security/predicates.py | 37 +++ h/services/group_members.py | 20 +- h/traversal/__init__.py | 6 + h/traversal/group_membership.py | 51 ++++ h/views/api/groups.py | 35 +-- tests/functional/api/groups/members_test.py | 220 +++++++++++++++++- tests/unit/h/routes_test.py | 3 +- tests/unit/h/security/predicates_test.py | 120 +++++++++- tests/unit/h/services/group_members_test.py | 22 ++ .../unit/h/traversal/group_membership_test.py | 76 ++++++ tests/unit/h/views/api/groups_test.py | 69 ++---- 14 files changed, 565 insertions(+), 101 deletions(-) create mode 100644 h/traversal/group_membership.py create mode 100644 tests/unit/h/traversal/group_membership_test.py diff --git a/h/routes.py b/h/routes.py index cf3c8259cfa..0257f4377b7 100644 --- a/h/routes.py +++ b/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", ) config.add_route("api.search", "/api/search") config.add_route("api.users", "/api/users", factory="h.traversal.UserRoot") diff --git a/h/security/permission_map.py b/h/security/permission_map.py index a5e56dcda0a..5b3e8bfe0ba 100644 --- a/h/security/permission_map.py +++ b/h/security/permission_map.py @@ -55,6 +55,7 @@ [p.group_has_user_as_moderator], ], Permission.Group.MEMBER_ADD: [[p.group_matches_authenticated_client_authority]], + Permission.Group.MEMBER_REMOVE: [[p.group_member_remove]], Permission.Group.MODERATE: [[p.group_created_by_user]], # --------------------------------------------------------------------- # # Annotations diff --git a/h/security/permissions.py b/h/security/permissions.py index 14d3baead3b..89465040581 100644 --- a/h/security/permissions.py +++ b/h/security/permissions.py @@ -40,6 +40,9 @@ class Group(Enum): MEMBER_ADD = "group:member:add" """Add a user other than yourself (that's JOIN) to a group.""" + MEMBER_REMOVE = "group:member:remove" + """Remove a member from the group.""" + class Annotation(Enum): """Permissions relating to annotations.""" diff --git a/h/security/predicates.py b/h/security/predicates.py index 15fe3ad59ee..de5fd439a6d 100644 --- a/h/security/predicates.py +++ b/h/security/predicates.py @@ -13,6 +13,7 @@ from itertools import chain from h.models.group import GroupMembershipRoles, JoinableBy, ReadableBy, WriteableBy +from h.traversal import GroupMembershipContext def requires(*parent_predicates): @@ -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): + def get_authenticated_users_membership(): + """Return the authenticated users membership in the target group.""" + for membership in identity.user.memberships: + if membership.group.id == context.group.id: + return membership + + return None + + membership = get_authenticated_users_membership() + + if not membership: + # You can't remove anyone from a group you're not a member of. + return False + + if identity.user.userid == context.user.userid: + # Any member can remove themselves from a group. + return True + + if "owner" in context.membership.roles or "admin" in context.membership.roles: + # Only owners can remove other owners. + return "owner" in membership.roles + + if "moderator" in context.membership.roles: + # Owners and admins can remove moderators. + return "owner" in membership.roles or "admin" in membership.roles + + # Owners, admins and moderators can remove plain members. + return ( + "owner" in membership.roles + or "admin" in membership.roles + or "moderator" in membership.roles + ) + + def resolve_predicates(mapping): """ Expand predicates with requirements into concrete lists of predicates. diff --git a/h/services/group_members.py b/h/services/group_members.py index 37c044f0326..899bf8b0daf 100644 --- a/h/services/group_members.py +++ b/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: + """Return `user`'s existing membership in `group`, if any.""" + return self.db.scalar( + select(GroupMembership) + .where(GroupMembership.group == group) + .where(GroupMembership.user == user) + ) + def add_members(self, group, userids): """ Add the users indicated by userids to this group's members. @@ -63,11 +71,7 @@ def member_join(self, group, userid): """Add `userid` to the member list of `group`.""" user = self.user_fetcher(userid) - existing_membership = self.db.scalar( - select(GroupMembership) - .where(GroupMembership.user == user) - .where(GroupMembership.group == group) - ) + existing_membership = self.get(group, user) if existing_membership: # The user is already a member of the group. @@ -86,11 +90,7 @@ def member_leave(self, group, userid): """Remove `userid` from the member list of `group`.""" user = self.user_fetcher(userid) - membership = self.db.scalars( - select(GroupMembership) - .where(GroupMembership.group == group) - .where(GroupMembership.user == user) - ).one_or_none() + membership = self.get(group, user) if not membership: return diff --git a/h/traversal/__init__.py b/h/traversal/__init__.py index 2ce76ba5c3a..dd8a3af39ff 100644 --- a/h/traversal/__init__.py +++ b/h/traversal/__init__.py @@ -62,6 +62,10 @@ from h.traversal.annotation import AnnotationContext, AnnotationRoot from h.traversal.group import GroupContext, GroupRequiredRoot, GroupRoot +from h.traversal.group_membership import ( + GroupMembershipContext, + group_membership_api_factory, +) from h.traversal.organization import OrganizationContext, OrganizationRoot from h.traversal.user import UserByIDRoot, UserByNameRoot, UserContext, UserRoot @@ -77,4 +81,6 @@ "UserByIDRoot", "UserRoot", "GroupContext", + "GroupMembershipContext", + "group_membership_api_factory", ) diff --git a/h/traversal/group_membership.py b/h/traversal/group_membership.py new file mode 100644 index 00000000000..a1494617282 --- /dev/null +++ b/h/traversal/group_membership.py @@ -0,0 +1,51 @@ +from dataclasses import dataclass + +from pyramid.httpexceptions import HTTPNotFound + +from h.exceptions import InvalidUserId +from h.models import Group, GroupMembership, User + + +@dataclass +class GroupMembershipContext: + group: Group + user: User + membership: GroupMembership | None + + +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) diff --git a/h/views/api/groups.py b/h/views/api/groups.py index 751c28aed12..56ce9c3699f 100644 --- a/h/views/api/groups.py +++ b/h/views/api/groups.py @@ -1,16 +1,10 @@ -from pyramid.httpexceptions import ( - HTTPBadRequest, - HTTPConflict, - HTTPNoContent, - HTTPNotFound, -) +from pyramid.httpexceptions import HTTPConflict, HTTPNoContent, HTTPNotFound -from h.exceptions import InvalidUserId from h.i18n import TranslationString as _ from h.presenters import GroupJSONPresenter, GroupsJSONPresenter, UserJSONPresenter from h.schemas.api.group import CreateGroupAPISchema, UpdateGroupAPISchema from h.security import Permission -from h.traversal import GroupContext +from h.traversal import GroupContext, GroupMembershipContext from h.views.api.config import api_config from h.views.api.exceptions import PayloadError @@ -161,17 +155,13 @@ def read_members(context: GroupContext, _request): link_name="group.member.delete", description="Remove the current user from a group", is_authenticated=True, + permission=Permission.Group.MEMBER_REMOVE, ) -def remove_member(context: GroupContext, request): +def remove_member(context: GroupMembershipContext, request): """Remove a member from the given group.""" - # 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') group_members_service = request.find_service(name="group_members") - group_members_service.member_leave(context.group, userid) + group_members_service.member_leave(context.group, context.user.userid) return HTTPNoContent() @@ -184,28 +174,19 @@ 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): """ Add a member to a given group. :raise HTTPNotFound: if the user is not found or if the use and group authorities don't match. """ - user_svc = request.find_service(name="user") group_members_svc = request.find_service(name="group_members") - try: - user = user_svc.fetch(request.matchdict["userid"]) - except InvalidUserId as err: - raise HTTPNotFound() from err - - if user is None: - raise HTTPNotFound() - - if user.authority != context.group.authority: + if context.user.authority != context.group.authority: raise HTTPNotFound() - group_members_svc.member_join(context.group, user.userid) + group_members_svc.member_join(context.group, context.user.userid) return HTTPNoContent() diff --git a/tests/functional/api/groups/members_test.py b/tests/functional/api/groups/members_test.py index 5c3eee38090..61d4c6548f2 100644 --- a/tests/functional/api/groups/members_test.py +++ b/tests/functional/api/groups/members_test.py @@ -2,7 +2,7 @@ import pytest -from h.models import GroupMembership, Token +from h.models import GroupMembership, GroupMembershipRoles, Token from h.models.auth_client import AuthClient, GrantType @@ -135,7 +135,6 @@ def test_it_ignores_forwarded_users( # It has *not* added the user from the X-Forwarded-User header to the group. assert forwarded_user not in group.members - @pytest.mark.xfail def test_me_alias_with_forwarded_user( self, do_request, factories, authclient, headers, group ): @@ -193,27 +192,230 @@ def user(self, authclient, factories): class TestRemoveMember: - def test_it(self, app, db_session, factories): + @pytest.mark.parametrize( + "authenticated_users_role,target_users_role,expect_success", + [ + # Only owners can remove other owners. + (GroupMembershipRoles.OWNER, GroupMembershipRoles.OWNER, True), + (GroupMembershipRoles.ADMIN, GroupMembershipRoles.OWNER, False), + (GroupMembershipRoles.MODERATOR, GroupMembershipRoles.OWNER, False), + (GroupMembershipRoles.MEMBER, GroupMembershipRoles.OWNER, False), + # Only owners can remove admins. + (GroupMembershipRoles.OWNER, GroupMembershipRoles.ADMIN, True), + (GroupMembershipRoles.ADMIN, GroupMembershipRoles.ADMIN, False), + (GroupMembershipRoles.MODERATOR, GroupMembershipRoles.ADMIN, False), + (GroupMembershipRoles.MEMBER, GroupMembershipRoles.ADMIN, False), + # Owners and admins can remove moderators. + (GroupMembershipRoles.OWNER, GroupMembershipRoles.MODERATOR, True), + (GroupMembershipRoles.ADMIN, GroupMembershipRoles.MODERATOR, True), + (GroupMembershipRoles.MODERATOR, GroupMembershipRoles.MODERATOR, False), + (GroupMembershipRoles.MEMBER, GroupMembershipRoles.MODERATOR, False), + # Owners, admins and moderators can remove members. + (GroupMembershipRoles.OWNER, GroupMembershipRoles.MEMBER, True), + (GroupMembershipRoles.ADMIN, GroupMembershipRoles.MEMBER, True), + (GroupMembershipRoles.MODERATOR, GroupMembershipRoles.MEMBER, True), + (GroupMembershipRoles.MEMBER, GroupMembershipRoles.MEMBER, False), + # Non-members can't remove anyone. + (None, GroupMembershipRoles.OWNER, False), + (None, GroupMembershipRoles.ADMIN, False), + (None, GroupMembershipRoles.MEMBER, False), + (None, GroupMembershipRoles.MODERATOR, False), + ], + ) + def test_it( + self, + app, + db_session, + factories, + authenticated_users_role, + target_users_role, + expect_success, + ): + group, other_group = factories.Group.create_batch(size=2) + # The target user who we will remove from the group. + target_user = factories.User() + db_session.add( + GroupMembership(group=group, user=target_user, roles=[target_users_role]) + ) + # Another user who is a member of the group. + # This user should *not* be removed from the group. + other_user = factories.User() + db_session.add(GroupMembership(user=other_user, group=group)) + # Make the target user a member of another group as well. + # The target user should *not* be removed from this group. + db_session.add(GroupMembership(user=target_user, group=other_group)) + # The authenticated user who will make the request. + authenticated_user = factories.User() + if authenticated_users_role: + db_session.add( + GroupMembership( + group=group, + user=authenticated_user, + roles=[authenticated_users_role], + ) + ) + token = factories.DeveloperToken(user=authenticated_user) + db_session.commit() + + app.delete( + f"/api/groups/{group.pubid}/members/{target_user.userid}", + headers=token_authorization_header(token), + status=204 if expect_success else 404, + ) + + if expect_success: + assert target_user not in group.members + else: + assert target_user in group.members + assert target_user in other_group.members + assert other_user in group.members + + @pytest.mark.parametrize( + "role", + [ + GroupMembershipRoles.OWNER, + GroupMembershipRoles.ADMIN, + GroupMembershipRoles.MODERATOR, + GroupMembershipRoles.MEMBER, + ], + ) + def test_any_member_can_remove_themselves_from_a_group( + self, app, db_session, factories, role + ): + group, other_group = factories.Group.create_batch(size=2) user, other_user = factories.User.create_batch(size=2) + db_session.add_all( + [ + GroupMembership(group=group, user=user, roles=[role]), + GroupMembership(group=other_group, user=user), + GroupMembership(group=group, user=other_user), + ] + ) + token = factories.DeveloperToken(user=user) + db_session.commit() + + app.delete( + f"/api/groups/{group.pubid}/members/{user.userid}", + headers=token_authorization_header(token), + ) + + assert user not in group.members + assert user in other_group.members + assert other_user in group.members + + @pytest.mark.parametrize( + "role", + [ + GroupMembershipRoles.OWNER, + GroupMembershipRoles.ADMIN, + GroupMembershipRoles.MODERATOR, + GroupMembershipRoles.MEMBER, + ], + ) + def test_unauthenticated_requests_cant_remove_anyone_from_groups( + self, app, db_session, factories, role + ): + group = factories.Group() + user = factories.User() + db_session.add(GroupMembership(group=group, user=user, roles=[role])) + db_session.commit() + + app.delete(f"/api/groups/{group.pubid}/members/{user.userid}", status=404) + + assert user in group.members + + def test_me_alias(self, app, db_session, factories): group, other_group = factories.Group.create_batch(size=2) + user, other_user = factories.User.create_batch(size=2) db_session.add_all( [ - GroupMembership(user=user, group=group), - GroupMembership(user=user, group=other_group), - GroupMembership(user=other_user, group=group), + GroupMembership(group=group, user=user), + GroupMembership(group=other_group, user=user), + GroupMembership(group=group, user=other_user), ] ) token = factories.DeveloperToken(user=user) - db_session.add(token) - headers = {"Authorization": "Bearer {}".format(token.value)} db_session.commit() - app.delete("/api/groups/{}/members/me".format(group.pubid), headers=headers) + app.delete( + f"/api/groups/{group.pubid}/members/me", + headers=token_authorization_header(token), + ) assert user not in group.members assert user in other_group.members assert other_user in group.members + def test_me_alias_when_not_authenticated(self, app, factories, db_session): + group = factories.Group() + db_session.commit() + + app.delete(f"/api/groups/{group.pubid}/members/me", status=404) + + def test_when_group_not_found(self, app, db_session, factories): + user = factories.User() + token = factories.DeveloperToken(user=user) + db_session.commit() + + app.delete( + f"/api/groups/DOESNT_EXIST/members/{user.userid}", + headers=token_authorization_header(token), + status=404, + ) + + def test_when_user_not_found(self, app, db_session, factories): + user = factories.User.build() # `user` has a valid userid but isn't in the DB. + group = factories.Group() + authenticated_user = factories.User() + db_session.add( + GroupMembership( + group=group, user=authenticated_user, roles=[GroupMembershipRoles.OWNER] + ) + ) + token = factories.DeveloperToken(user=authenticated_user) + db_session.commit() + + app.delete( + f"/api/groups/{group.pubid}/members/{user.userid}", + headers=token_authorization_header(token), + status=404, + ) + + def test_when_userid_invalid(self, app, db_session, factories): + group = factories.Group() + authenticated_user = factories.User() + db_session.add( + GroupMembership( + group=group, user=authenticated_user, roles=[GroupMembershipRoles.OWNER] + ) + ) + token = factories.DeveloperToken(user=authenticated_user) + db_session.commit() + + app.delete( + f"/api/groups/{group.pubid}/members/INVALID_USERID", + headers=token_authorization_header(token), + status=404, + ) + + def test_when_no_membership(self, app, db_session, factories): + group = factories.Group() + target_user = factories.User() # The target user isn't a member of the group. + authenticated_user = factories.User() + db_session.add( + GroupMembership( + group=group, user=authenticated_user, roles=[GroupMembershipRoles.OWNER] + ) + ) + token = factories.DeveloperToken(user=authenticated_user) + db_session.commit() + + app.delete( + f"/api/groups/{group.pubid}/members/{target_user.userid}", + headers=token_authorization_header(token), + status=404, + ) + def token_authorization_header(token) -> dict: """Return an Authorization header for the given developer token.""" diff --git a/tests/unit/h/routes_test.py b/tests/unit/h/routes_test.py index dcf2d7201fc..488269892f0 100644 --- a/tests/unit/h/routes_test.py +++ b/tests/unit/h/routes_test.py @@ -152,8 +152,7 @@ def test_includeme(): call( "api.group_member", "/api/groups/{pubid}/members/{userid}", - factory="h.traversal.GroupRequiredRoot", - traverse="/{pubid}", + factory="h.traversal.group_membership_api_factory", ), call("api.search", "/api/search"), call("api.users", "/api/users", factory="h.traversal.UserRoot"), diff --git a/tests/unit/h/security/predicates_test.py b/tests/unit/h/security/predicates_test.py index bd2d7791e59..6bbc29ddbc1 100644 --- a/tests/unit/h/security/predicates_test.py +++ b/tests/unit/h/security/predicates_test.py @@ -10,7 +10,8 @@ WriteableBy, ) from h.security import Identity, predicates -from h.traversal import AnnotationContext, UserContext +from h.security.identity import LongLivedGroup, LongLivedMembership +from h.traversal import AnnotationContext, GroupMembershipContext, UserContext from h.traversal.group import GroupContext @@ -421,6 +422,123 @@ def group_context(self, factories): return GroupContext(group=factories.Group.build()) +class TestGroupMemberRemove: + @pytest.mark.parametrize( + "authenticated_users_role,target_users_role,expected_result", + [ + # Only owners can remove other owners. + (GroupMembershipRoles.OWNER, GroupMembershipRoles.OWNER, True), + (GroupMembershipRoles.ADMIN, GroupMembershipRoles.OWNER, False), + (GroupMembershipRoles.MODERATOR, GroupMembershipRoles.OWNER, False), + (GroupMembershipRoles.MEMBER, GroupMembershipRoles.OWNER, False), + # Only owners can remove admins. + (GroupMembershipRoles.OWNER, GroupMembershipRoles.ADMIN, True), + (GroupMembershipRoles.ADMIN, GroupMembershipRoles.ADMIN, False), + (GroupMembershipRoles.MODERATOR, GroupMembershipRoles.ADMIN, False), + (GroupMembershipRoles.MEMBER, GroupMembershipRoles.ADMIN, False), + # Owners and admins can remove moderators. + (GroupMembershipRoles.OWNER, GroupMembershipRoles.MODERATOR, True), + (GroupMembershipRoles.ADMIN, GroupMembershipRoles.MODERATOR, True), + (GroupMembershipRoles.MODERATOR, GroupMembershipRoles.MODERATOR, False), + (GroupMembershipRoles.MEMBER, GroupMembershipRoles.MODERATOR, False), + # Owners, admins and moderators can remove members. + (GroupMembershipRoles.OWNER, GroupMembershipRoles.MEMBER, True), + (GroupMembershipRoles.ADMIN, GroupMembershipRoles.MEMBER, True), + (GroupMembershipRoles.MODERATOR, GroupMembershipRoles.MEMBER, True), + (GroupMembershipRoles.MEMBER, GroupMembershipRoles.MEMBER, False), + # Non-members can't remove anyone. + (None, GroupMembershipRoles.OWNER, False), + (None, GroupMembershipRoles.ADMIN, False), + (None, GroupMembershipRoles.MEMBER, False), + (None, GroupMembershipRoles.MODERATOR, False), + ], + ) + def test_it( + self, + identity, + context, + group, + target_users_role, + authenticated_users_role, + expected_result, + ): + if authenticated_users_role: + identity.user.memberships.append( + LongLivedMembership( + group=LongLivedGroup.from_model(group), + user=identity.user, + roles=[authenticated_users_role], + ) + ) + context.membership.roles = [target_users_role] + + assert predicates.group_member_remove(identity, context) == expected_result + + @pytest.mark.parametrize( + "role", + [ + GroupMembershipRoles.OWNER, + GroupMembershipRoles.ADMIN, + GroupMembershipRoles.MODERATOR, + GroupMembershipRoles.MEMBER, + ], + ) + def test_any_member_can_remove_themselves_from_a_group( + self, identity, context, role, group + ): + identity.user.userid = context.user.userid + identity.user.memberships.append( + LongLivedMembership( + group=LongLivedGroup.from_model(group), + user=identity.user, + roles=[role], + ) + ) + context.membership.roles = [role] + + assert predicates.group_member_remove(identity, context) is True + + @pytest.fixture + def authenticated_user(self, db_session, authenticated_user, factories): + # Make the authenticated user a member of a *different* group, + # to make sure that unrelated memberships don't accidentally allow or + # deny permissions. + db_session.add( + GroupMembership( + user=authenticated_user, + group=factories.Group(), + roles=[GroupMembershipRoles.OWNER], + ) + ) + + return authenticated_user + + @pytest.fixture + def group(self, db_session, factories): + group = factories.Group() + + # Make a *different* user a member of the target group + # to make sure that unrelated memberships don't accidentally allow or + # deny permissions. + db_session.add( + GroupMembership( + group=group, user=factories.User(), roles=[GroupMembershipRoles.OWNER] + ) + ) + + return group + + @pytest.fixture + def user(self, factories): + return factories.User() + + @pytest.fixture + def context(self, group, user): + return GroupMembershipContext( + group=group, user=user, membership=GroupMembership(group=group, user=user) + ) + + class TestResolvePredicates: @pytest.mark.parametrize( "clause,expansion", diff --git a/tests/unit/h/services/group_members_test.py b/tests/unit/h/services/group_members_test.py index 4691645fe5a..2d121a43519 100644 --- a/tests/unit/h/services/group_members_test.py +++ b/tests/unit/h/services/group_members_test.py @@ -8,6 +8,28 @@ from h.services.group_members import GroupMembersService, group_members_factory +class TestGet: + def test_it(self, group_members_service, factories, db_session): + group = factories.Group.build() + user = factories.User.build() + membership = GroupMembership(group=group, user=user) + db_session.add(membership) + + result = group_members_service.get(group, user) + + assert result == membership + + def test_it_when_theres_no_matching_membership( + self, group_members_service, factories + ): + group = factories.Group() + user = factories.User() + + result = group_members_service.get(group, user) + + assert result is None + + class TestMemberJoin: def test_it_adds_user_to_group( self, group_members_service, factories, caplog, db_session diff --git a/tests/unit/h/traversal/group_membership_test.py b/tests/unit/h/traversal/group_membership_test.py new file mode 100644 index 00000000000..db25caaa643 --- /dev/null +++ b/tests/unit/h/traversal/group_membership_test.py @@ -0,0 +1,76 @@ +import re +from unittest.mock import sentinel + +import pytest +from pyramid.httpexceptions import HTTPNotFound + +from h.exceptions import InvalidUserId +from h.traversal.group_membership import ( + GroupMembershipContext, + group_membership_api_factory, +) + + +@pytest.mark.usefixtures("group_service", "user_service", "group_members_service") +class TestGroupMembershipAPIFactory: + def test_it( + self, group_service, user_service, group_members_service, pyramid_request + ): + context = group_membership_api_factory(pyramid_request) + + group_service.fetch.assert_called_once_with(sentinel.pubid) + user_service.fetch.assert_called_once_with(sentinel.userid) + group_members_service.get.assert_called_once_with( + group_service.fetch.return_value, user_service.fetch.return_value + ) + assert isinstance(context, GroupMembershipContext) + assert context.group == group_service.fetch.return_value + assert context.user == user_service.fetch.return_value + assert context.membership == group_members_service.get.return_value + + def test_when_no_matching_group(self, group_service, pyramid_request): + group_service.fetch.return_value = None + + with pytest.raises(HTTPNotFound, match="Group not found: sentinel.pubid"): + group_membership_api_factory(pyramid_request) + + def test_when_no_matching_user(self, user_service, pyramid_request): + user_service.fetch.return_value = None + + with pytest.raises(HTTPNotFound, match="User not found: sentinel.userid"): + group_membership_api_factory(pyramid_request) + + def test_when_invalid_userid(self, user_service, pyramid_request): + user_service.fetch.side_effect = InvalidUserId(sentinel.userid) + + with pytest.raises(HTTPNotFound, match="User not found: sentinel.userid"): + group_membership_api_factory(pyramid_request) + + def test_when_no_matching_membership(self, group_members_service, pyramid_request): + group_members_service.get.return_value = None + + with pytest.raises( + HTTPNotFound, + match=re.escape("Membership not found: (sentinel.pubid, sentinel.userid)"), + ): + group_membership_api_factory(pyramid_request) + + def test_me_alias(self, pyramid_config, pyramid_request, user_service): + pyramid_config.testing_securitypolicy(userid=sentinel.userid) + pyramid_request.matchdict["userid"] = "me" + + group_membership_api_factory(pyramid_request) + + user_service.fetch.assert_called_once_with(sentinel.userid) + + def test_me_alias_when_not_authenticated(self, pyramid_request): + pyramid_request.matchdict["userid"] = "me" + + with pytest.raises(HTTPNotFound, match="User not found: me"): + group_membership_api_factory(pyramid_request) + + @pytest.fixture + def pyramid_request(self, pyramid_request): + pyramid_request.matchdict["pubid"] = sentinel.pubid + pyramid_request.matchdict["userid"] = sentinel.userid + return pyramid_request diff --git a/tests/unit/h/views/api/groups_test.py b/tests/unit/h/views/api/groups_test.py index 23e21687465..ec72bb56d82 100644 --- a/tests/unit/h/views/api/groups_test.py +++ b/tests/unit/h/views/api/groups_test.py @@ -1,19 +1,13 @@ from unittest.mock import PropertyMock, call, create_autospec, sentinel import pytest -from pyramid.httpexceptions import ( - HTTPBadRequest, - HTTPConflict, - HTTPNoContent, - HTTPNotFound, -) +from pyramid.httpexceptions import HTTPConflict, HTTPNoContent, HTTPNotFound import h.views.api.groups as views from h import presenters -from h.exceptions import InvalidUserId -from h.models import User +from h.models import GroupMembership, User from h.schemas.base import ValidationError -from h.traversal.group import GroupContext +from h.traversal import GroupContext, GroupMembershipContext from h.views.api.exceptions import PayloadError @@ -433,59 +427,33 @@ def test_it(self, context, pyramid_request, UserJSONPresenter): class TestRemoveMember: def test_it(self, context, pyramid_request, group_members_service): - pyramid_request.matchdict = {"userid": "me"} - response = views.remove_member(context, pyramid_request) group_members_service.member_leave.assert_called_once_with( - context.group, pyramid_request.authenticated_userid + context.group, context.user.userid ) assert isinstance(response, HTTPNoContent) - def test_it_doesnt_let_you_remove_another_member(self, context, pyramid_request): - pyramid_request.matchdict = {"userid": "other"} - - with pytest.raises( - HTTPBadRequest, match='Only the "me" user value is currently supported' - ): - views.remove_member(context, pyramid_request) + @pytest.fixture + def context(self, factories): + group = factories.Group.build() + user = factories.User.build() + membership = GroupMembership(group=group, user=user) + return GroupMembershipContext(group=group, user=user, membership=membership) -@pytest.mark.usefixtures("user_service", "group_members_service") +@pytest.mark.usefixtures("group_members_service") class TestAddMember: - def test_it( - self, context, pyramid_request, user_service, group_members_service, factories - ): - user = user_service.fetch.return_value = factories.User( - authority=context.group.authority - ) - + def test_it(self, pyramid_request, group_members_service, context): response = views.add_member(context, pyramid_request) - user_service.fetch.assert_called_once_with(sentinel.userid) group_members_service.member_join.assert_called_once_with( - context.group, user.userid + context.group, context.user.userid ) assert isinstance(response, HTTPNoContent) - def test_it_with_malformed_userid(self, context, pyramid_request, user_service): - user_service.fetch.side_effect = InvalidUserId("invalid_userid") - - with pytest.raises(HTTPNotFound) as exc_info: - views.add_member(context, pyramid_request) - - assert exc_info.value.__cause__ == user_service.fetch.side_effect - - def test_it_with_unknown_userid(self, context, pyramid_request, user_service): - user_service.fetch.return_value = None - - with pytest.raises(HTTPNotFound): - views.add_member(context, pyramid_request) - - def test_it_with_authority_mismatch( - self, context, pyramid_request, user_service, factories - ): - user_service.fetch.return_value = factories.User(authority="other") + def test_it_with_authority_mismatch(self, pyramid_request, context): + context.group.authority = "other" with pytest.raises(HTTPNotFound): views.add_member(context, pyramid_request) @@ -496,9 +464,10 @@ def pyramid_request(self, pyramid_request): return pyramid_request @pytest.fixture - def context(self, context, factories): - context.group = factories.Group() - return context + def context(self, factories): + group = factories.Group.build() + user = factories.User.build(authority=group.authority) + return GroupMembershipContext(group=group, user=user, membership=None) @pytest.fixture