Core Data: Introduce entity-aware permission selector #63292
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Type] Enhancement
|
Size Change: +349 B (+0.02%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
Mamaduka
requested review from
ajitbohra,
nerrad,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Mamaduka
requested review from
youknowriad,
jsnajdr,
ellatrix,
tyxla and
ntsekouras
and removed request for
juanmaguitar,
ajitbohra and
ndiego
| } ); | ||
| } catch ( error ) { | ||
| // Do nothing if our OPTIONS request comes back with an API error (4xx or | ||
| // 5xx). The previously determined isAllowed value will remain in the store. |
There was a problem hiding this comment.
I don't think this is right. It should indicate that the user has no permissions for the record I believe.
There was a problem hiding this comment.
I found this comment in the original PR: https://github.com/WordPress/gutenberg/pull/12378/files#r243442316. This is similar to what we do in other resolvers - not touching the store when the request fails.
|
Could this functionality be integrated into There's also a request that |
|
Avoiding yet another public selector deprecation is probably preferable. We can change // new
canUser( 'update', { 'postType', 'wp_block', 5 } );
// legacy
canUser( 'update, 'blocks', 5 );
I'll try to leave a comment in the discussion, but I don't see it as a blocker for this PR, more like something we can support in the future. |
|
I would be cautious about objects in selector arguments, there might be impacts on memoization of selectors and things like that. Personally I don't like that pattern much other than for well defined objects like "query". |
Then maybe we can invent some syntax like @Mamaduka says about the user capabilities:
My idea is to keep some space for the possibility that we use canUser( 'activate_plugins', 'on:this:site' )How would we specify the "object", instead of the |
While I generally agree, how likely is it that we would have to memoize a selector that returns |
Good point, maybe not :). I was just raising it for consideration.
So yeah, if the object argument addresses all the needs without downsides, that's fine by me too. |
tyxla
reviewed
Jul 9, 2024
There was a problem hiding this comment.
Thanks for working on this one!
Haven't had a chance to review the code, but using "permission" seems off to me. In the WordPress core, users can have a role and each role determines their capabilities. With that in mind, I prefer capability to permission. To me, permission indicates file/folder permissions on the server, rather than having access to read data or perform certain types of updates. An with that in mind, I'd expect this new API to be consistent with the terms and tech slang that WordPress has already established.
2 tasks
|
The alternative PR is ready for review - #63322. |
|
It was superseded by #63322. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Resolves #43751.
PR introduces a new entity-aware user permission selector for the
core-datastore. I've also replaced the usage ofcanUserEditEntityRecordin the code base.Why?
canUser- only supports resources in thewp/v2namespace. Additionally, you must know the final REST API path. Typically, however, only an entity's kind and name are known.canUserEntityRecord- is limited to only the edit action check. It was a wrapper forcanUserand only supported thewp/v2namespace, which hasn't been required since WP 5.9.The new selector also opens up the possibility to leverage "target hints" and bulk fetch permissions data for DataViews.
Notes
canUser.canUser-cacheKey = isAllowed.Testing Instructions
Basic Testing
Testing canUserEntityRecord changes
Testing Instructions for Keyboard
Same.