-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an API for editing a group membership's role #9114
base: add-edit-membership-api
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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, | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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"], | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
Comment on lines
+222
to
+275
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there should be a better way to implement this based on rank-ordering of the roles, rather than handling each case individually. (And the same for the |
||
|
||
|
||
def resolve_predicates(mapping): | ||
""" | ||
Expand predicates with requirements into concrete lists of predicates. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
] | ||
Comment on lines
+24
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The get-group-members API now uses the new |
||
|
||
|
||
@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.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing these pointless docstrings. |
||
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,34 @@ 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)) | ||
new_roles = appstruct["roles"] | ||
|
||
if not request.has_permission( | ||
Permission.Group.MEMBER_EDIT, | ||
EditGroupMembershipContext( | ||
context.group, context.user, context.membership, new_roles | ||
), | ||
): | ||
raise HTTPNotFound() | ||
Comment on lines
+76
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no The new role comes from the request JSON body and isn't available until that JSON body has been parsed and validated, and that happens in the view. So we can't use Pyramid's Instead we let Pyramid call the view regardless, and then the view itself constructs a context object and calls We do need the permissions logic to be implemented in the centralized permissions system because in future another view is going to have to check |
||
|
||
if context.membership.roles != new_roles: | ||
old_roles = context.membership.roles | ||
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The If we ever add a
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is compatible with the user objects currently returned by the get-group-members API just with
roles
added. (It was previously thought of as a list of users, but now we're thinking of it as a list of memberships.)