Pass the right context objects to edit_member() and add_member()

[seanh]

Background

Pyramid has the handy concept of a "context" object for a request, named context. The way the context object works is:

• routes.py defines the "factory" for the "api.group_member" route:

  config.add_route(
      "api.group_member",
      "/api/groups/{pubid}/members/{userid}",
      factory="h.traversal.group_membership_api_factory",
  )

• The factory="h.traversal.group_membership_api_factory" means that whenever a request for /api/groups/{pubid}/members/{userid} comes in Pyramid will call group_membership_api_factory() to get the context object.

• group_membership_api_factory() returns a GroupMembershipContext object for the context:

  @dataclass
  class GroupMembershipContext:
      group: Group
      user: User
      membership: GroupMembership | None

• Pyramid then passes that object as the context argument to various layers in its request processing cycle. For example:

  • The "predicate functions" in security/predicates.py (which make security decisions about whether to permit or deny a request) receive context as an argument.
  • Views also receive context as an argument. There are three views for the "api.group_member" route (remove_member(), edit_member() and add_member(), Add GET membership API #9197 will also add get_member()) that all receive GroupMembershipContext objects as the context argument.
  • The context object is also available in lots of other places, either passed into things as the context argument or available via the request object as request.context.

Problem

group_membership_api_factory() is the context factory for all views that use the "api.group_member" route:

• remove_member() (request_method="DELETE")
• add_member() (request_method="POST")
• edit_member() (request_method="PATCH")
• Add in Add GET membership API #9197 also get_member() (request_method="GET")

group_membership_api_factory() returns a GroupMembershipContext object but:

• GroupMembershipContext isn't the right context for edit_member(): it lacks the context.new_roles attribute for the new roles that the request wants to change context.membership.roles to.

• GroupMembershipContext isn't the right context for add_member(): it has an inappropriate context.membership attribute that has to be set to None (when adding a new membership there shouldn't be an existing membership in the context) and it lacks the context.new_roles attribute for the roles that the request wants to create a new membership with.

The context for the edit_member() view should be an EditGroupMembershipContext object, and for add_member() it should be an AddGroupMembershipContext.

As a result the edit_member() view has to create its own context object to pass to request.has_permission():

h/h/views/api/group_members.py

Lines 116 to 126 in cfe86f0

	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()

When a future PR (#9200) enables users (not just auth clients) to call the add-membership API the add_member() view will have to do something
similar: constructing its own AddGroupMembershipContext object and passing it to request.has_permission().

This means there are two different context objects in play for the edit_member() and add_member() views: the context that is passed to the view is a GroupMembershipContext, but the context that is passed to request.has_permission() is an EditGroupMembershipContext or AddGroupMembershipContext constructed by the view itself.

Solution

This commit changes group_membership_api_factory() to return a GroupMembershipContext for GET and DELETE requests but return an EditGroupMembershipContext for PATCH requests and an AddGroupMembershipContext for POSTs.

It's not possible for group_membership_api_factory() to set the context's new_roles attribute: the value for new_roles isn't available until later in the request processing cycle after the view has parsed and validated the request's JSON body. So the factory returns context objects with context.new_roles=None and the edit_member() view has to set new_roles before calling has_permission():

def edit_member(context: EditGroupMembershipContext, request):
    appstruct = EditGroupMembershipAPISchema().validate(json_payload(request))
    context.new_roles = appstruct["roles"]

    if not request.has_permission(Permission.Group.MEMBER_EDIT, context):
        raise HTTPNotFound()

In #9200 the add_member() view will have to do the same. So this is still a little weird, but I think it's better than having two different context objects for a single request.

Testing

For now the add-member API can only be called by authclients, not users, so you'll have to create an authclient to test it:

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

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

   Authority: localhost
   Grant type: client_credentials
   Trusted: yes

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

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

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

The edit-member and remove-member APIs can only be called by users, not authclients, so you'll have to go to http://localhost:5000/account/developer and create an API key to test them.

Now use the API key to authenticate a request to change devdata_user's role:

$ httpx http://localhost:5000/api/groups/{pubid}/members/acct:devdata_user@localhost --method PATCH --headers Authorization 'Bearer {apitoken}' --json '{"roles": ["admin"]}'

Finally, use the API key to authenticate a request to delete devdata_user's group membership:

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

[seanh]

Adding a new AddGroupMembershipContext class for the add_member() view to use, alongside the existing GroupMembershipContext and EditGroupMembershipContext classes already used by other views.

It's unfortunate to have to type-annotate this as | None here, but AddGroupMembershipContext's are initially created with new_roles=None and then new_roles is set later so 🤷‍♂️

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

Files not reviewed (2)

• h/traversal/init.py: Evaluated as low risk
• h/traversal/group_membership.py: Evaluated as low risk

[seanh]

We tend to import names from h/*/*.py files into h/*/__init__.py to make imports easier. It means you can do this:

from h.traversal import Foo, Bar, Gar

Instead of having to do this:

from h.traversal.foo import Foo
from h.traversal.bar import Bar
from h.traversal.gar import Gar

It also means the importer doesn't know about the internal structure of h/traversal/ so we can reorganise the internal files without breaking any importers.

[seanh]

We should get rid of this __all__, it's unnecessary

[seanh]

The view always has to set context.new_roles before calling request.has_permission(some_permission, context) (which ends up calling this group_member_edit() predicate). Adding this assert here to make sure that the security predicate won't run if new_roles isn't set, as that could potentially lead to permissions being granted when they shouldn't be.

[seanh]

DD: Group Owners, Admins and Moderators contains a section explaining how security permissions work in Pyramid and h.

[mtomilov]

Tested it locally seems to be working just fine.