From 0c02bd754cef42f8d4204489fbe71278cf9ebe3f Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Sat, 23 Nov 2024 01:40:57 +0000 Subject: [PATCH] Add an API for editing a group membership's role --- h/presenters/__init__.py | 2 + h/presenters/group_membership_json.py | 12 + h/schemas/api/group_membership.py | 18 + h/security/permission_map.py | 1 + h/security/permissions.py | 3 + h/security/predicates.py | 58 +- h/traversal/__init__.py | 2 + h/traversal/group_membership.py | 8 + h/views/api/annotations.py | 18 +- h/views/api/group_members.py | 48 +- h/views/api/groups.py | 19 +- h/views/api/helpers/json_payload.py | 13 + h/views/api/users.py | 13 +- tests/functional/api/groups/members_test.py | 129 ++++- .../presenters/group_membership_json_test.py | 19 + .../h/schemas/api/group_membership_test.py | 36 ++ tests/unit/h/security/predicates_test.py | 531 +++++++++++++++++- tests/unit/h/views/api/group_members_test.py | 121 +++- 18 files changed, 988 insertions(+), 63 deletions(-) create mode 100644 h/presenters/group_membership_json.py create mode 100644 h/schemas/api/group_membership.py create mode 100644 h/views/api/helpers/json_payload.py create mode 100644 tests/unit/h/presenters/group_membership_json_test.py create mode 100644 tests/unit/h/schemas/api/group_membership_test.py diff --git a/h/presenters/__init__.py b/h/presenters/__init__.py index 899735fb4b0..37a9cdb2c5d 100644 --- a/h/presenters/__init__.py +++ b/h/presenters/__init__.py @@ -5,6 +5,7 @@ from h.presenters.document_json import DocumentJSONPresenter from h.presenters.document_searchindex import DocumentSearchIndexPresenter from h.presenters.group_json import GroupJSONPresenter, GroupsJSONPresenter +from h.presenters.group_membership_json import GroupMembershipJSONPresenter from h.presenters.user_json import TrustedUserJSONPresenter, UserJSONPresenter __all__ = ( @@ -17,5 +18,6 @@ "GroupJSONPresenter", "GroupsJSONPresenter", "UserJSONPresenter", + "GroupMembershipJSONPresenter", "TrustedUserJSONPresenter", ) diff --git a/h/presenters/group_membership_json.py b/h/presenters/group_membership_json.py new file mode 100644 index 00000000000..623ac887672 --- /dev/null +++ b/h/presenters/group_membership_json.py @@ -0,0 +1,12 @@ +class GroupMembershipJSONPresenter: + def __init__(self, membership): + self.membership = membership + + 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, + } diff --git a/h/schemas/api/group_membership.py b/h/schemas/api/group_membership.py new file mode 100644 index 00000000000..167acaed0c6 --- /dev/null +++ b/h/schemas/api/group_membership.py @@ -0,0 +1,18 @@ +from h.schemas.base import JSONSchema + + +class EditGroupMembershipAPISchema(JSONSchema): + schema = { + "type": "object", + "properties": { + "roles": { + "type": "array", + "minItems": 1, + "maxItems": 1, + "items": { + "enum": ["member", "moderator", "admin", "owner"], + }, + } + }, + "required": ["roles"], + } diff --git a/h/security/permission_map.py b/h/security/permission_map.py index 5b3e8bfe0ba..dfe2fa8b0b3 100644 --- a/h/security/permission_map.py +++ b/h/security/permission_map.py @@ -56,6 +56,7 @@ ], Permission.Group.MEMBER_ADD: [[p.group_matches_authenticated_client_authority]], Permission.Group.MEMBER_REMOVE: [[p.group_member_remove]], + Permission.Group.MEMBER_EDIT: [[p.group_member_edit]], Permission.Group.MODERATE: [[p.group_created_by_user]], # --------------------------------------------------------------------- # # Annotations diff --git a/h/security/permissions.py b/h/security/permissions.py index 89465040581..be34a46c142 100644 --- a/h/security/permissions.py +++ b/h/security/permissions.py @@ -43,6 +43,9 @@ class Group(Enum): MEMBER_REMOVE = "group:member:remove" """Remove a member from the group.""" + MEMBER_EDIT = "group:member:edit" + """Change a member's role in a group.""" + class Annotation(Enum): """Permissions relating to annotations.""" diff --git a/h/security/predicates.py b/h/security/predicates.py index de5fd439a6d..8e7cb05446f 100644 --- a/h/security/predicates.py +++ b/h/security/predicates.py @@ -13,7 +13,7 @@ from itertools import chain from h.models.group import GroupMembershipRoles, JoinableBy, ReadableBy, WriteableBy -from h.traversal import GroupMembershipContext +from h.traversal import EditGroupMembershipContext, GroupMembershipContext def requires(*parent_predicates): @@ -219,6 +219,62 @@ def get_authenticated_users_membership(): ) +@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 + + def resolve_predicates(mapping): """ Expand predicates with requirements into concrete lists of predicates. diff --git a/h/traversal/__init__.py b/h/traversal/__init__.py index dd8a3af39ff..e1d328462f1 100644 --- a/h/traversal/__init__.py +++ b/h/traversal/__init__.py @@ -63,6 +63,7 @@ from h.traversal.annotation import AnnotationContext, AnnotationRoot from h.traversal.group import GroupContext, GroupRequiredRoot, GroupRoot from h.traversal.group_membership import ( + EditGroupMembershipContext, GroupMembershipContext, group_membership_api_factory, ) @@ -81,6 +82,7 @@ "UserByIDRoot", "UserRoot", "GroupContext", + "EditGroupMembershipContext", "GroupMembershipContext", "group_membership_api_factory", ) diff --git a/h/traversal/group_membership.py b/h/traversal/group_membership.py index a1494617282..432d0c1d189 100644 --- a/h/traversal/group_membership.py +++ b/h/traversal/group_membership.py @@ -13,6 +13,14 @@ class GroupMembershipContext: membership: GroupMembership | None +@dataclass +class EditGroupMembershipContext: + group: Group + user: User + membership: GroupMembership + new_roles: list[str] + + def group_membership_api_factory(request) -> GroupMembershipContext: user_service = request.find_service(name="user") group_service = request.find_service(name="group") diff --git a/h/views/api/annotations.py b/h/views/api/annotations.py index d15dac63994..424f0cd9aa2 100644 --- a/h/views/api/annotations.py +++ b/h/views/api/annotations.py @@ -29,7 +29,7 @@ from h.security import Permission from h.services import AnnotationWriteService from h.views.api.config import api_config -from h.views.api.exceptions import PayloadError +from h.views.api.helpers.json_payload import json_payload _ = i18n.TranslationStringFactory(__package__) @@ -77,7 +77,7 @@ def search(request): def create(request): """Create an annotation from the POST payload.""" schema = CreateAnnotationSchema(request) - appstruct = schema.validate(_json_payload(request)) + appstruct = schema.validate(json_payload(request)) annotation = request.find_service(AnnotationWriteService).create_annotation( data=appstruct @@ -136,7 +136,7 @@ def update(context, request): schema = UpdateAnnotationSchema( request, context.annotation.target_uri, context.annotation.groupid ) - appstruct = schema.validate(_json_payload(request)) + appstruct = schema.validate(json_payload(request)) annotation = request.find_service(AnnotationWriteService).update_annotation( context.annotation, data=appstruct @@ -166,18 +166,6 @@ def delete(context, request): return {"id": context.annotation.id, "deleted": True} -def _json_payload(request): - """ - Return a parsed JSON payload for the request. - - :raises PayloadError: if the body has no valid JSON body - """ - try: - return request.json_body - except ValueError as err: - raise PayloadError() from err - - def _publish_annotation_event(request, annotation, action): """Publish an event to the annotations queue for this annotation action.""" event = AnnotationEvent(request, annotation.id, action) diff --git a/h/views/api/group_members.py b/h/views/api/group_members.py index 6048039e5e2..7f5876ac5b3 100644 --- a/h/views/api/group_members.py +++ b/h/views/api/group_members.py @@ -1,9 +1,15 @@ +import logging + from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound -from h.presenters import UserJSONPresenter +from h.presenters import GroupMembershipJSONPresenter +from h.schemas.api.group_membership import EditGroupMembershipAPISchema from h.security import Permission -from h.traversal import GroupContext, GroupMembershipContext +from h.traversal import EditGroupMembershipContext, GroupContext, GroupMembershipContext from h.views.api.config import api_config +from h.views.api.helpers.json_payload import json_payload + +log = logging.getLogger(__name__) @api_config( @@ -15,8 +21,10 @@ permission=Permission.Group.READ, ) def list_members(context: GroupContext, _request): - """Return a list of a group's members.""" - return [UserJSONPresenter(user).asdict() for user in context.group.members] + return [ + GroupMembershipJSONPresenter(membership).asdict() + for membership in context.group.memberships + ] @api_config( @@ -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.""" group_members_service = request.find_service(name="group_members") group_members_service.member_leave(context.group, context.user.userid) @@ -45,7 +52,6 @@ def remove_member(context: GroupMembershipContext, request): permission=Permission.Group.MEMBER_ADD, ) def add_member(context: GroupMembershipContext, request): - """Add a member to a group.""" group_members_service = request.find_service(name="group_members") if context.user.authority != context.group.authority: @@ -54,3 +60,33 @@ def add_member(context: GroupMembershipContext, request): group_members_service.member_join(context.group, context.user.userid) return HTTPNoContent() + + +@api_config( + versions=["v1", "v2"], + route_name="api.group_member", + request_method="PATCH", + link_name="group.member.edit", + description="Change a user's role in a group", +) +def edit_member(context: GroupMembershipContext, request): + appstruct = EditGroupMembershipAPISchema().validate(json_payload(request)) + old_roles = context.membership.roles + new_roles = appstruct["roles"] + + if not request.has_permission( + Permission.Group.MEMBER_EDIT, + EditGroupMembershipContext( + context.group, context.user, context.membership, new_roles + ), + ): + raise HTTPNotFound() + + context.membership.roles = new_roles + log.info( + "Changed group membership roles: %r (previous roles were: %r)", + context.membership, + old_roles, + ) + + return GroupMembershipJSONPresenter(context.membership).asdict() diff --git a/h/views/api/groups.py b/h/views/api/groups.py index 4ae5cdcfd5d..8334b38e3c4 100644 --- a/h/views/api/groups.py +++ b/h/views/api/groups.py @@ -6,7 +6,7 @@ from h.security import Permission from h.traversal import GroupContext from h.views.api.config import api_config -from h.views.api.exceptions import PayloadError +from h.views.api.helpers.json_payload import json_payload DEFAULT_GROUP_TYPE = "private" @@ -47,7 +47,7 @@ def create(request): appstruct = CreateGroupAPISchema( default_authority=request.default_authority, group_authority=request.effective_authority, - ).validate(_json_payload(request)) + ).validate(json_payload(request)) group_service = request.find_service(name="group") group_create_service = request.find_service(name="group_create") @@ -116,7 +116,7 @@ def update(context: GroupContext, request): appstruct = UpdateGroupAPISchema( default_authority=request.default_authority, group_authority=request.effective_authority, - ).validate(_json_payload(request)) + ).validate(json_payload(request)) group_update_service = request.find_service(name="group_update") group_service = request.find_service(name="group") @@ -133,16 +133,3 @@ def update(context: GroupContext, request): group = group_update_service.update(context.group, **appstruct) return GroupJSONPresenter(group, request).asdict(expand=["organization", "scopes"]) - - -# @TODO This is a duplication of code in h.views.api — move to a util module -def _json_payload(request): - """ - Return a parsed JSON payload for the request. - - :raises PayloadError: if the body has no valid JSON body - """ - try: - return request.json_body - except ValueError as err: - raise PayloadError() from err diff --git a/h/views/api/helpers/json_payload.py b/h/views/api/helpers/json_payload.py new file mode 100644 index 00000000000..a43073ff92f --- /dev/null +++ b/h/views/api/helpers/json_payload.py @@ -0,0 +1,13 @@ +from h.views.api.exceptions import PayloadError + + +def json_payload(request): + """ + Return the parsed JSON payload from `request`. + + :raise PayloadError: if the request has no valid JSON body + """ + try: + return request.json_body + except ValueError as err: + raise PayloadError() from err diff --git a/h/views/api/users.py b/h/views/api/users.py index a930d0d11ae..e9888929733 100644 --- a/h/views/api/users.py +++ b/h/views/api/users.py @@ -6,7 +6,7 @@ from h.security import Permission from h.services.user_unique import DuplicateUserError from h.views.api.config import api_config -from h.views.api.exceptions import PayloadError +from h.views.api.helpers.json_payload import json_payload @api_config( @@ -53,7 +53,7 @@ def create(request): authority :raises HTTPConflict: if user already exists """ - appstruct = CreateUserAPISchema().validate(_json_payload(request)) + appstruct = CreateUserAPISchema().validate(json_payload(request)) # Enforce authority match client_authority = request.identity.auth_client.authority @@ -90,15 +90,8 @@ def update(context, request): This API endpoint allows authorised clients (those able to provide a valid Client ID and Client Secret) to update users in their authority. """ - appstruct = UpdateUserAPISchema().validate(_json_payload(request)) + appstruct = UpdateUserAPISchema().validate(json_payload(request)) user = request.find_service(name="user_update").update(context.user, **appstruct) return TrustedUserJSONPresenter(user).asdict() - - -def _json_payload(request): - try: - return request.json_body - except ValueError as err: - raise PayloadError() from err diff --git a/tests/functional/api/groups/members_test.py b/tests/functional/api/groups/members_test.py index 61d4c6548f2..c488c043efb 100644 --- a/tests/functional/api/groups/members_test.py +++ b/tests/functional/api/groups/members_test.py @@ -1,6 +1,7 @@ import base64 import pytest +from sqlalchemy import select from h.models import GroupMembership, GroupMembershipRoles, Token from h.models.auth_client import AuthClient, GrantType @@ -21,7 +22,16 @@ def test_it_returns_list_of_members_for_restricted_group_without_authn( res = app.get("/api/groups/{pubid}/members".format(pubid=group.pubid)) assert res.status_code == 200 - assert len(res.json) == 3 + 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 + ] def test_it_returns_list_of_members_if_user_has_access_to_private_group( self, app, factories, db_session @@ -39,9 +49,17 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group( headers=token_authorization_header(token), ) - returned_usernames = [member["username"] for member in res.json] - assert returned_usernames == [member.username for member in group.members] assert res.status_code == 200 + 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 + ] def test_it_returns_404_if_user_does_not_have_read_access_to_group( self, app, db_session, factories @@ -417,6 +435,111 @@ def test_when_no_membership(self, app, db_session, factories): ) +class TestEditMembership: + def test_it(self, do_request, group, target_user, db_session): + response = do_request() + + assert response.json["userid"] == target_user.userid + assert response.json["roles"] == ["member"] + membership = db_session.scalars( + select(GroupMembership) + .where(GroupMembership.group == group) + .where(GroupMembership.user == target_user) + ).one() + assert membership.roles == ["member"] + + def test_me_alias_when_not_authenticated(self, do_request, headers): + del headers["Authorization"] + + do_request(status=404) + + def test_when_not_authorized(self, do_request): + do_request(json={"roles": [GroupMembershipRoles.OWNER]}, status=404) + + def test_with_unknown_pubid(self, do_request): + do_request(pubid="UNKNOWN", status=404) + + def test_with_unknown_userid(self, do_request, group): + do_request(userid=f"acct:UNKNOWN@{group.authority}", status=404) + + def test_with_invalid_userid(self, do_request): + do_request(userid=f"INVALID_USERID", status=404) + + def test_when_membership_doesnt_exist(self, do_request, factories): + do_request(userid=factories.User().userid, status=404) + + def test_me_alias(self, do_request, db_session, group, authenticated_user): + response = do_request(userid="me") + + assert response.json["userid"] == authenticated_user.userid + assert response.json["roles"] == ["member"] + membership = db_session.scalars( + select(GroupMembership) + .where(GroupMembership.group == group) + .where(GroupMembership.user == authenticated_user) + ).one() + assert membership.roles == ["member"] + + def test_me_alias_when_not_authorized(self, do_request): + do_request( + userid="me", json={"roles": [GroupMembershipRoles.OWNER]}, status=404 + ) + + def test_with_unknown_role(self, do_request): + response = do_request(json={"roles": ["UNKNOWN"]}, status=400) + + assert response.json["reason"].startswith("roles.0: 'UNKNOWN' is not one of [") + + @pytest.fixture + def group(self, factories): + return factories.Group() + + @pytest.fixture + def target_user(self, db_session, factories, group): + target_user = factories.User() + db_session.add( + GroupMembership( + group=group, user=target_user, roles=[GroupMembershipRoles.MODERATOR] + ) + ) + return target_user + + @pytest.fixture + def authenticated_user(self, db_session, factories, group): + authenticated_user = factories.User() + db_session.add( + GroupMembership( + group=group, user=authenticated_user, roles=[GroupMembershipRoles.ADMIN] + ) + ) + return authenticated_user + + @pytest.fixture + def headers(self, factories, authenticated_user): + return token_authorization_header( + factories.DeveloperToken(user=authenticated_user) + ) + + @pytest.fixture + def do_request(self, app, db_session, group, target_user, headers): + def do_request( + pubid=group.pubid, + userid=target_user.userid, + json={"roles": ["member"]}, + headers=headers, + status=200, + ): + db_session.commit() + return app.patch_json( + f"/api/groups/{pubid}/members/{userid}", + json, + headers=headers, + status=status, + ) + + return do_request + + def token_authorization_header(token) -> dict: """Return an Authorization header for the given developer token.""" return {"Authorization": "Bearer {}".format(token.value)} diff --git a/tests/unit/h/presenters/group_membership_json_test.py b/tests/unit/h/presenters/group_membership_json_test.py new file mode 100644 index 00000000000..b9c56a911ac --- /dev/null +++ b/tests/unit/h/presenters/group_membership_json_test.py @@ -0,0 +1,19 @@ +from h.models import GroupMembership +from h.presenters.group_membership_json import GroupMembershipJSONPresenter + + +class TestGroupMembershipJSONPresenter: + def test_it(self, factories): + user = factories.User.build() + group = factories.Group.build() + membership = GroupMembership(user=user, group=group) + + json = GroupMembershipJSONPresenter(membership).asdict() + + assert json == { + "authority": group.authority, + "userid": user.userid, + "username": user.username, + "display_name": user.display_name, + "roles": membership.roles, + } diff --git a/tests/unit/h/schemas/api/group_membership_test.py b/tests/unit/h/schemas/api/group_membership_test.py new file mode 100644 index 00000000000..170dfbb06d4 --- /dev/null +++ b/tests/unit/h/schemas/api/group_membership_test.py @@ -0,0 +1,36 @@ +import pytest + +from h.schemas import ValidationError +from h.schemas.api.group_membership import EditGroupMembershipAPISchema + + +class TestEditGroupMembershipAPISchema: + @pytest.mark.parametrize( + "data,expected_appstruct", + [ + ({"roles": ["member"]}, {"roles": ["member"]}), + ({"roles": ["moderator"]}, {"roles": ["moderator"]}), + ({"roles": ["admin"]}, {"roles": ["admin"]}), + ({"roles": ["owner"]}, {"roles": ["owner"]}), + ], + ) + def test_valid(self, schema, data, expected_appstruct): + assert schema.validate(data) == expected_appstruct + + @pytest.mark.parametrize( + "data,error_message", + [ + ({"roles": ["unknown"]}, r"^roles\.0: 'unknown' is not one of \[[^]]*\]$"), + ({"roles": []}, r"^roles: \[\] should be non-empty$"), + ({"roles": ["member", "moderator"]}, r"^roles: \[[^]]*\] is too long$"), + ({"roles": 42}, r"^roles: 42 is not of type 'array'$"), + ({}, r"^'roles' is a required property$"), + ], + ) + def test_invalid(self, schema, data, error_message): + with pytest.raises(ValidationError, match=error_message): + schema.validate(data) + + @pytest.fixture + def schema(self): + return EditGroupMembershipAPISchema() diff --git a/tests/unit/h/security/predicates_test.py b/tests/unit/h/security/predicates_test.py index 6bbc29ddbc1..86fda4b4d55 100644 --- a/tests/unit/h/security/predicates_test.py +++ b/tests/unit/h/security/predicates_test.py @@ -1,3 +1,4 @@ +# pylint:disable=too-many-lines from unittest.mock import sentinel import pytest @@ -11,7 +12,12 @@ ) from h.security import Identity, predicates from h.security.identity import LongLivedGroup, LongLivedMembership -from h.traversal import AnnotationContext, GroupMembershipContext, UserContext +from h.traversal import ( + AnnotationContext, + EditGroupMembershipContext, + GroupMembershipContext, + UserContext, +) from h.traversal.group import GroupContext @@ -539,6 +545,529 @@ def context(self, group, user): ) +class TestGroupMemberEdit: + @pytest.mark.parametrize( + "authenticated_users_roles,old_roles,new_roles,expected_result", + [ + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.OWNER], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.ADMIN], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MODERATOR], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MEMBER], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.OWNER], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.ADMIN], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MODERATOR], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MEMBER], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.OWNER], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.ADMIN], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MODERATOR], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MEMBER], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.OWNER], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.ADMIN], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MODERATOR], + True, + ), + ( + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MEMBER], + True, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MODERATOR], + True, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MEMBER], + True, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MODERATOR], + True, + ), + ( + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MEMBER], + True, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.OWNER], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.ADMIN], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.OWNER], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + None, + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + None, + [GroupMembershipRoles.MODERATOR], + [GroupMembershipRoles.MEMBER], + False, + ), + ( + None, + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.OWNER], + False, + ), + ( + None, + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.ADMIN], + False, + ), + ( + None, + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MODERATOR], + False, + ), + ( + None, + [GroupMembershipRoles.MEMBER], + [GroupMembershipRoles.MEMBER], + False, + ), + ], + ) + def test_changing_someone_elses_role( + self, + factories, + identity, + group, + authenticated_users_roles, + old_roles, + new_roles, + expected_result, + ): + user = factories.User() + membership = GroupMembership(group=group, user=user, roles=old_roles) + context = EditGroupMembershipContext( + group=group, user=user, membership=membership, new_roles=new_roles + ) + if authenticated_users_roles: + identity.user.memberships.append( + LongLivedMembership( + group=LongLivedGroup.from_model(group), + user=identity.user, + roles=authenticated_users_roles, + ) + ) + + assert predicates.group_member_edit(identity, context) == expected_result + + @pytest.mark.parametrize( + "old_roles,new_roles,expected_result", + [ + ([GroupMembershipRoles.OWNER], [GroupMembershipRoles.OWNER], True), + ([GroupMembershipRoles.OWNER], [GroupMembershipRoles.ADMIN], True), + ([GroupMembershipRoles.OWNER], [GroupMembershipRoles.MODERATOR], True), + ([GroupMembershipRoles.OWNER], [GroupMembershipRoles.MEMBER], True), + ([GroupMembershipRoles.ADMIN], [GroupMembershipRoles.OWNER], False), + ([GroupMembershipRoles.ADMIN], [GroupMembershipRoles.ADMIN], True), + ([GroupMembershipRoles.ADMIN], [GroupMembershipRoles.MODERATOR], True), + ([GroupMembershipRoles.ADMIN], [GroupMembershipRoles.MEMBER], True), + ([GroupMembershipRoles.MODERATOR], [GroupMembershipRoles.OWNER], False), + ([GroupMembershipRoles.MODERATOR], [GroupMembershipRoles.ADMIN], False), + ([GroupMembershipRoles.MODERATOR], [GroupMembershipRoles.MODERATOR], True), + ([GroupMembershipRoles.MODERATOR], [GroupMembershipRoles.MEMBER], True), + ([GroupMembershipRoles.MEMBER], [GroupMembershipRoles.OWNER], False), + ([GroupMembershipRoles.MEMBER], [GroupMembershipRoles.ADMIN], False), + ([GroupMembershipRoles.MEMBER], [GroupMembershipRoles.MODERATOR], False), + ([GroupMembershipRoles.MEMBER], [GroupMembershipRoles.MEMBER], False), + ], + ) + def test_changing_own_role( + self, factories, identity, group, old_roles, new_roles, expected_result + ): + user = factories.User() + membership = GroupMembership(group=group, user=user, roles=old_roles) + context = EditGroupMembershipContext( + group=group, user=user, membership=membership, new_roles=new_roles + ) + identity.user.memberships.append( + LongLivedMembership( + group=LongLivedGroup.from_model(group), + user=identity.user, + roles=old_roles, + ) + ) + identity.user.userid = context.user.userid + + assert predicates.group_member_edit(identity, context) == expected_result + + @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 + + class TestResolvePredicates: @pytest.mark.parametrize( "clause,expansion", diff --git a/tests/unit/h/views/api/group_members_test.py b/tests/unit/h/views/api/group_members_test.py index a7434250f62..7249ce96ca0 100644 --- a/tests/unit/h/views/api/group_members_test.py +++ b/tests/unit/h/views/api/group_members_test.py @@ -1,4 +1,5 @@ -from unittest.mock import call, create_autospec, sentinel +import logging +from unittest.mock import PropertyMock, call, create_autospec, sentinel import pytest from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound @@ -6,22 +7,28 @@ import h.views.api.group_members as views from h import presenters from h.models import GroupMembership +from h.schemas.base import ValidationError from h.traversal import GroupContext, GroupMembershipContext +from h.views.api.exceptions import PayloadError class TestListMembers: - def test_it(self, context, pyramid_request, UserJSONPresenter): - context.group.members = [sentinel.member_1, sentinel.member_2] - presenter_instances = UserJSONPresenter.side_effect = [ - create_autospec(presenters.UserJSONPresenter, instance=True, spec_set=True), - create_autospec(presenters.UserJSONPresenter, instance=True, spec_set=True), + def test_it(self, context, pyramid_request, GroupMembershipJSONPresenter): + context.group.memberships = [sentinel.membership_1, sentinel.membership_2] + presenter_instances = GroupMembershipJSONPresenter.side_effect = [ + create_autospec( + presenters.GroupMembershipJSONPresenter, instance=True, spec_set=True + ), + create_autospec( + presenters.GroupMembershipJSONPresenter, instance=True, spec_set=True + ), ] response = views.list_members(context, pyramid_request) - assert UserJSONPresenter.call_args_list == [ - call(sentinel.member_1), - call(sentinel.member_2), + assert GroupMembershipJSONPresenter.call_args_list == [ + call(sentinel.membership_1), + call(sentinel.membership_2), ] presenter_instances[0].asdict.assert_called_once_with() presenter_instances[1].asdict.assert_called_once_with() @@ -77,8 +84,100 @@ def context(self, factories): return GroupMembershipContext(group=group, user=user, membership=None) +class TestEditMember: + def test_it( + self, + context, + pyramid_request, + EditGroupMembershipAPISchema, + GroupMembershipJSONPresenter, + caplog, + ): + caplog.set_level(logging.INFO) + + response = views.edit_member(context, pyramid_request) + + EditGroupMembershipAPISchema.return_value.validate.assert_called_once_with( + sentinel.json_body + ) + assert context.membership.roles == sentinel.new_roles + GroupMembershipJSONPresenter.assert_called_once_with(context.membership) + assert response == GroupMembershipJSONPresenter.return_value.asdict.return_value + assert caplog.messages == [ + f"Changed group membership roles: {context.membership!r} (previous roles were: {sentinel.old_roles!r})", + ] + + def test_it_errors_if_the_user_doesnt_have_permission( + self, context, pyramid_request, pyramid_config + ): + pyramid_config.testing_securitypolicy(permissive=False) + + with pytest.raises(HTTPNotFound): + views.edit_member(context, pyramid_request) + + def test_it_errors_if_the_request_isnt_valid_JSON( + self, context, pyramid_request, mocker + ): + value_error = ValueError() + mocker.patch.object( + type(pyramid_request), + "json_body", + PropertyMock(side_effect=value_error), + create=True, + ) + + with pytest.raises(PayloadError) as exc_info: + views.edit_member(context, pyramid_request) + + assert exc_info.value.__cause__ == value_error + + def test_it_errors_if_the_request_is_invalid( + self, context, pyramid_request, EditGroupMembershipAPISchema + ): + EditGroupMembershipAPISchema.return_value.validate.side_effect = ValidationError + + with pytest.raises(ValidationError): + views.edit_member(context, pyramid_request) + + @pytest.fixture + def context(self, factories): + group = factories.Group.build() + user = factories.User.build(authority=group.authority) + membership = GroupMembership(group=group, user=user, roles=sentinel.old_roles) + + return GroupMembershipContext(group=group, user=user, membership=membership) + + @pytest.fixture + def pyramid_request(self, pyramid_request): + pyramid_request.json_body = sentinel.json_body + return pyramid_request + + @pytest.fixture + def EditGroupMembershipAPISchema(self, EditGroupMembershipAPISchema): + EditGroupMembershipAPISchema.return_value.validate.return_value = { + "roles": sentinel.new_roles + } + return EditGroupMembershipAPISchema + + @pytest.fixture(autouse=True) + def pyramid_config(self, pyramid_config): + pyramid_config.testing_securitypolicy(permissive=True) + return pyramid_config + + +@pytest.fixture(autouse=True) +def EditGroupMembershipAPISchema(mocker): + return mocker.patch( + "h.views.api.group_members.EditGroupMembershipAPISchema", + autospec=True, + spec_set=True, + ) + + @pytest.fixture(autouse=True) -def UserJSONPresenter(mocker): +def GroupMembershipJSONPresenter(mocker): return mocker.patch( - "h.views.api.group_members.UserJSONPresenter", autospec=True, spec_set=True + "h.views.api.group_members.GroupMembershipJSONPresenter", + autospec=True, + spec_set=True, )