Skip to content
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

Enable users to call the add-member API #9200

Open
wants to merge 1 commit into
base: refactor-contexts
Choose a base branch
from

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Dec 10, 2024

h's add-member-to-group API can only be called by authclients, not users, even though other membership APIs (list members, edit-membership, remove-membership) can all be called by users.

This PR enhances the add-membership API to allow it to be called by users as well as authclients.

This PR doesn't update the API docs, I'll send a follow-up PR to do that.

Testing

  1. Log in as devdata_admin (password: pass).

  2. Go to http://localhost:5000/groups/new and create a new group.

  3. Go to http://localhost:5000/account/developer and create an API key.

  4. Use the API key to authenticate a request to the add-member API:

    $ httpx http://localhost:5000/api/groups/{pubid}/members/acct:devdata_user@localhost --method POST --headers Authorization 'Bearer {apikey}
    

@seanh seanh changed the base branch from main to add-membership-request-body December 10, 2024 16:55
@seanh seanh changed the title enable users to call add member API Enable users to call the add-member API Dec 10, 2024
Comment on lines +69 to +72
Permission.Group.MEMBER_ADD: [
[p.group_member_add],
[p.group_matches_authenticated_client_authority],
],
Copy link
Contributor Author

@seanh seanh Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the MEMBER_ADD permission will be granted if either group_member_add() returns True or group_matches_authenticated_client_authority() returns True.

If you're wondering what this "permission map" is DD: Group Owners, Admins and Moderators contains a section explaining how security permissions work in Pyramid and h.

@seanh seanh force-pushed the enable-users-to-call-add-member-API branch from c522c82 to a5d7e31 Compare December 11, 2024 15:57
@seanh seanh changed the base branch from add-membership-request-body to refactor-contexts December 11, 2024 15:57
@seanh seanh force-pushed the enable-users-to-call-add-member-API branch from a5d7e31 to 5297204 Compare December 11, 2024 16:04
Comment on lines +224 to +251
@requires(authenticated_user, group_found)
def group_member_add(identity, context: AddGroupMembershipContext):
assert (
context.new_roles is not None
), "new_roles must be set before checking permissions"

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 []

authenticated_users_roles = get_authenticated_users_roles()

if GroupMembershipRoles.OWNER in authenticated_users_roles:
# Owners can add members with any roles.
return True

if GroupMembershipRoles.ADMIN in authenticated_users_roles:
# Admins can add members with any role below admin.
return (
GroupMembershipRoles.OWNER not in context.new_roles
and GroupMembershipRoles.ADMIN not in context.new_roles
)

return False
Copy link
Contributor Author

@seanh seanh Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the add-member API can be called by users we need this need security permissions predicate function to check that the authenticated user should be permitted to make the request: to add a user to a group with role "owner" or "admin" the authenticated user must be a member of the group also with role "owner". Or, if the authenticated user has role "admin", they can add a user with role "member" or "moderator". If you're not an owner or admin of a group then you're not allowed to add users to that group with any role.

(This predicate wasn't needed previously because the add-member API could only be called by authclients and authclients are permitted to do anything.)

The get_authenticated_users_roles() helper has been duplicated a few times now, I'll send a follow-up PR to refactor and deduplicate this and other duplication in predicates.py (#9205).

@seanh seanh force-pushed the enable-users-to-call-add-member-API branch from 5297204 to d349530 Compare December 11, 2024 16:47
@seanh seanh force-pushed the refactor-contexts branch from b171774 to fc5dbd6 Compare December 11, 2024 16:47
@seanh seanh force-pushed the enable-users-to-call-add-member-API branch 2 times, most recently from de08675 to e79457b Compare December 11, 2024 17:22
@seanh seanh marked this pull request as ready for review December 11, 2024 17:23
@seanh seanh requested a review from Copilot December 11, 2024 17:23

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 9 changed files in this pull request and generated no suggestions.

Files not reviewed (4)
  • h/security/predicates.py: Evaluated as low risk
  • h/services/group_members.py: Evaluated as low risk
  • h/views/api/group_members.py: Evaluated as low risk
  • tests/functional/api/groups/members_test.py: Evaluated as low risk
@seanh seanh requested a review from mtomilov December 11, 2024 17:41
@mtomilov
Copy link

Tested it locally seems to be working just fine.

Allow users (not just authclients) to call the add-membership API.
@seanh seanh force-pushed the enable-users-to-call-add-member-API branch from e79457b to 8491ea9 Compare December 12, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants