Abstract mutation permission checks

[jasonbahl]

We should abstract mutation permission checks.

see: #140 (comment)

[CodeProKid]

@jasonbahl was this added in #140 ? Can't tell because the PR is gigantic and my browser won't load it 😆

[jasonbahl]

@CodeProKid this was not included. This issue was created 3 days after that PR, I believe in response to some non-DRY things happening in that PR

[jasonbahl]

I'm thinking that we should probably think about moving the actual logic of mutations into the Model Layer as well.

This will allow us to have more granular mutations that still pass through the same cap checks

For example, we have updatePost but let's pretend we also wented a very granular setPostTitle mutation, if the logic for writing to a post was centralized in a Model Layer, then smaller mutations could exist and make use of the central capability checks, etc. . .much like on the query side.

[renatonascalves]

Hi guys! Something about the members mutations for the BuddyPress API that I want to discuss.

BuddyPress has some members actions, delete and update, that we can use the same mutations from the plugin.

But the current implementation has no way for me to filter the permissions check. For example. In BuddyPress we check if the user has the edit_user cap to update his own account or the delete_user to delete his own account.

Also we have admins or mods that have a specific cap bp_moderate.

The problem is that I can't hook into the mutations permissions check and verify those credentials. Also I don't want do add the delete_users cap to a user, that's too broad a permission to give, only so that he can delete his account.

Either by moving the code to the model or adding a hook here: https://github.com/wp-graphql/wp-graphql/blob/develop/src/Mutation/UserDelete.php#L81

I think this is a nice addition not only to this mutation but to all of the other mutations. This will bring more flexibility to plugin authors.

At the BP REST API, we added in a way one could hook into the permission check and change that: https://github.com/buddypress/BP-REST/blob/64a675a0a23cde1254a073f531bb6091d3e96c6a/includes/bp-activity/classes/class-bp-rest-activity-endpoint.php#L332 which is different from how the core API does it.

[CodeProKid]

I definitely agree that we should centralize the cap checks for mutations, but I don't think the model layer is the correct place to do so. The model layer is to model WordPress data to conform to the shape of the Graph. The checks for mutations are fundamentally different in that they don't affect the shape of objects in any way, but rather enforce rules around what actions a user can take.

@renatonascalves to get you through your current issue, I think there are some core filters you could potentially leverage to achieve your desired result. I'm hesitant to start adding filters on these cap checks individually because once we centralize the checks, we'll want to rip out any individual filters and move to a generic filter like we have in the model layer, and adding one off filters now will make it more difficult to centralize this functionality down the road while maintaining BC.

I think the most straightforward path here would be to add the $user_id as the second arg of the current_user_can() call, and then that would allow people extending the plugin to use the map_meta_cap filter to have finer control over the capability check here.

[renatonascalves]

Hi folks! I'd like us to get moving on this. A core support would be prefeable than a custom "hack". Is there any options that are more interesting for the current codebase?

@CodeProKid Tks! That's a nice idea. Will use that in the meantime. :)

[CodeProKid]

@renatonascalves & @jasonbahl I think I'm starting to change my thinking on the approach for this. I think the Model layer approach actually could be a good implementation to achieve this. I forgot that we also have to translate the input field names to WP arg names, which is basically just the inverse of what we are doing in the Model layer currently. The biggest departure will probably just be having the check be more black and white, where if some certain conditions aren't met, we don't allow the mutation to happen at all.

Let me know if you think think approach sounds good, and I can work up a POC for this.

[renatonascalves]

Sorry, could you share a simple code example of how that would work to help me visualize it better?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jasonbahl]

I'm going to close this for now.

I think there's some hints of good ideas here, but nothing concrete to take action on yet.

[justlevine]

The new WPMutationType class should open a path forward if/when we want to revisit