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

Add response body to add-member API #9188

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Dec 5, 2024

Change the add-group-membership API's response from a 204 No Content to a 200 OK with a JSON representation of the newly-added membership as the body.

Fixes #9141.

Testing:

  1. Log in as devdata_admin

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

  3. Go to http://localhost:5000/admin/oauthclients/new and create an authclient with these properties:

    Authority: localhost
    Grant type: client_credentials
    Trusted: yes

  4. Use the authclient to authenticate an API request to add devdata_user to the group:

    $ httpx http://localhost:5000/api/groups/{pubid}/members/acct:devdata_user@localhost --method POST --auth {client_id} {client_secret}
    HTTP/1.1 200 OK
    
    {
        "authority": "localhost",
        "userid": "acct:devdata_user@localhost",
        "username": "devdata_user",
        "display_name": null,
        "roles": [
            "member"
        ],
        "actions": [],
        "created": "2024-12-06T12:54:27.790391+00:00",
        "updated": "2024-12-06T12:54:27.790394+00:00"
    }
    

@seanh seanh requested a review from marcospri December 5, 2024 23:45
@seanh seanh marked this pull request as draft December 5, 2024 23:47
@seanh seanh force-pushed the add-membership-response-body branch 2 times, most recently from d064db2 to 2ebfb06 Compare December 6, 2024 13:34
@seanh seanh marked this pull request as ready for review December 6, 2024 13:34
@seanh
Copy link
Contributor Author

seanh commented Dec 6, 2024

I think the "actions": [] in the response is correct. The API request is made by an authclient, not by a user. And authclients can't take any further actions against the memberships they created: for example they can't delete or edit the memberships. For example this:

$ httpx http://localhost:5000/api/groups/{groupid}/members/acct:devdata_user@localhost --method DELETE --auth {client_id} {client_secret}

...will get a 404 Not Found response. As will this:

$ httpx http://localhost:5000/api/groups/{groupid}/members/acct:devdata_user@localhost --method PATCH --json '{"roles": ["member"]}' --auth {client_id} {client_secret}

...but both requests would succeed if they were authenticated with devdata_admin's API key rather than with an authclient.

The reason is that the MEMBER_ADD permission just requires an authclient whose authority matches the group's (implementation of predicate function). Whereas MEMBER_REMOVE and MEMBER_EDIT require an authenticated user who has permission (group_member_remove() predicate, group_member_edit() predicate).

This is in the API docs too. Add member is documented as requiring authclient authentication. Whereas delete member is documented as requiring an API key. (Edit-member isn't documented yet.)

The API for getting a list of the group's members will do the same thing if you call it with an authclient:

$ httpx http://localhost:5000/api/groups/{groupid}/members --method GET --auth {client_id} {client_secret}

...this will return "actions": [] for every membership in the group because the authclient isn't permitted to take any of the actions against any of the memberships.

Maybe we'll want to do something about this one day--the fact that the add-member API only works for authclients not for users, get-group-members works with either, and edit-member and remove-member only work for users.

@@ -895,8 +897,12 @@ paths:
- $ref: '#/components/parameters/GroupID'
- $ref: '#/components/parameters/UserID'
responses:
'204':
$ref: '#/components/responses/NoContent'
'200':
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is a significant change to the API but in practice I don't think clients will check for 204 in particular (vs any other 2XX code).

Change the add-group-membership API's response from a 204 No Content to
a 200 OK with a JSON representation of the added membership as the body.

Fixes #9141
@seanh seanh force-pushed the add-membership-response-body branch from 2ebfb06 to c649dc9 Compare December 12, 2024 17:57
@seanh
Copy link
Contributor Author

seanh commented Dec 12, 2024

Removed the docs changes from this PR, gonna do them in #9210 instead.

@seanh seanh requested a review from Copilot December 12, 2024 18:04

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 5 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (2)

tests/functional/api/groups/members_test.py:360

  • [nitpick] The parameter names pubid and userid are ambiguous. Consider renaming them to group_pubid and user_userid for clarity.
def do_request(pubid=group.pubid, userid=user.userid, status=200):

tests/unit/h/views/api/group_members_test.py:144

  • Ensure the test covers the new behavior of returning a JSON response with status code 200.
def test_it(self, pyramid_request, group_members_service, context):
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.

Add a response body to the add-group-member API
2 participants