Global Styles: Allow non-admin users access to global styles data in post editor #64797
Conversation
aaronrobertshaw
added
[Type] Bug
aaronrobertshaw
changed the title
|
@jorgefilipecosta or @youknowriad when you get a chance would you be able to point me in the right direction on how best to allow non-admin users access to global styles data in the post editor? @talldan and I looked into this today but we could do with some expert opinions 🙏 |
|
Size Change: +542 B (+0.03%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
| select( coreStore ); | ||
|
|
||
| return ( | ||
| canUser( 'read', { kind: 'root', name: 'theme' } ) && |
There was a problem hiding this comment.
Something I just noticed, this is only checking whether the user can read the theme. While the __experimentalGetCurrentThemeBaseGlobalStyles does get the current theme as part of its logic, there's nothing to check whether the user can read the globalStyles entity (the actual entity type being returned). 🤔
There was a problem hiding this comment.
How our permissions checks are supposed to work with entities is going over my head at the moment 🫠
The REST API endpoint called when retrieving this base global styles data does check if the user has the edit_posts capability in this PR, or edit_theme_options on trunk. So is that not "something"?
There was a problem hiding this comment.
When debugging I find it helpful to remove the preloading so that you can see the requests in your dev tools. The permissions checks from canUser are generally OPTIONS requests, and the Allow header in the response provides a list of the HTTP verbs that the current user is allowed to perform.
The canUser call you deleted here doesn't seem to work from what I can tell. I think that's because it doesn't match the actual call that's made to get the current theme. My understanding is that canUser( 'read', { kind: 'root', name: 'theme' } ) is checking "can the user GET a list of installed themes" (which I'd expect lower tiered users not to be able to do). The actual request to get the current theme is querying for status: active:
gutenberg/packages/core-data/src/resolvers.js
Lines 374 to 378 in 482c188
The status: active part changes the permission check in the REST Controller:
https://github.com/WordPress/wordpress-develop/blob/f4761a3f8c5aa6920d4407e3af271ef70db5f305/src/wp-includes/rest-api/endpoints/class-wp-rest-themes-controller.php#L103-L106
The user only needs edit_posts caps:
https://github.com/WordPress/wordpress-develop/blob/f4761a3f8c5aa6920d4407e3af271ef70db5f305/src/wp-includes/rest-api/endpoints/class-wp-rest-themes-controller.php#L149-L152
I'll have a look into whether there's a way to replace that with a check that actually works.
I think there's a similar issue with trying to do canUser( 'read', { kind: 'root', name: 'globalStyles' } ). That would check whether the user can list all global styles, but we actually need to check a route that's more like globalStyles/theme/<activeStylesheet>.
There was a problem hiding this comment.
I've pushed a commit that adds working permissions checks.
There was a problem hiding this comment.
If pushing changes to this PR, it might also helps those pinged for input if the PR description is updated to match new changes.
There was a problem hiding this comment.
Actually, seems like I was logged in as admin accidentally when I thought it was working. It doesn't work with non-admin so I'll revert those commits.
The explanation is still correct, but unfortunately it doesn't look like there's a way to check permissions using canUser for those requests.
| baseURLParams: { context: 'edit' }, | ||
| baseURLParams: {}, |
There was a problem hiding this comment.
It seems to work ok with context: edit, so maybe it's ok to undo these changes.
| canUser, | ||
| } = select( coreStore ); | ||
|
|
||
| const canReadActiveTheme = canUser( 'read', 'themes?status=active' ); |
There was a problem hiding this comment.
So to prevent a flash of global styles in the post editor, we'll need to preload these paths as well?
|
@talldan retesting this PR after the latest commits shows a regression. The global styles data isn't accessible to the non-admin user anymore and the block style variation styles aren't generated.
In the before picture, the custom global styles for the button outline block style I set in Global Styles are applied. In the latest on this branch, now not even the base outline styles apply. |
Yeah, my bad, I was switching users a lot and accidentally was logged in as admin when testing my changes. I've reverted those changes. I'm not really sure of the best way to check permissions now if that doesn't work, perhaps it's best just to try making the request and if it fails, handle it. |
There was a problem hiding this comment.
Thanks for getting this PR started, and sorry if this is premature. Is the plan to allow read access to merged/global styles CPT data as well?
I just did some manual testing with various user roles to check what's available. In this PR the base theme styles are coming through 👍🏻
// Check in the post editor for styles values:
await wp.data.resolveSelect( 'core/block-editor').getSettings()
// Check read access
await wp.data.resolveSelect( 'core' ).getEntityRecord( 'root', 'globalStyles', await wp.data.resolveSelect( 'core' ).__experimentalGetCurrentGlobalStylesId() )
Yes, the complete merged global styles data is required in the post editor for non-admins to enable block style variation styles and style inheritance.
This is the first half of the equation. The part I'm less clear on how to tackle are the permissions for the global styles CPT entity storing the user set global styles. This PR incorrectly removes the |
This might sound far-fetched - in my mind, a read-only global styles CPT belongs to the "site". It's owner (whichever admin starts to edit global styles) is a side effect of the fact that only admins can access global styles. So is it too simplistic to allow GET requests and do permissions checks on the rest? Probably not yet relevant, but global styles revisions won't still only be readable by admins: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php#L186 |
| */ | ||
| if ( ! current_user_can( 'edit_theme_options' ) ) { | ||
| if ( ! current_user_can( 'edit_posts' ) ) { |
There was a problem hiding this comment.
To allow the canUser( 'read', { kind: 'root', name: 'globalStyles', id: _globalStylesId, } ) check in useGlobalStylesUserConfig() to pass we might need to update the read capabilities of the wp_global_styles custom post type:
E.g., to 'read' => 'edit_posts',
That would mean that editors/authors would be able to read the global styles post, e.g., await wp.data.resolveSelect( 'core' ).getEntityRecord( 'root', 'globalStyles', await wp.data.resolveSelect( 'core' ).__experimentalGetCurrentGlobalStylesId() )
It seems harmless enough, but I'm not sure what's kosher.
To display the values, the current editor/author needs to have read access to both.
I haven't thought this through, but I wonder if a new read-only endpoint to retrieve merged theme data might be in order. The edit-post package could call it directly from the core store and there'd be no need to shuffle around block settings.
There was a problem hiding this comment.
Yeah, I think if we think that we should allow "read" for global styles objects using the edit_posts capability, we should just make the update in the existing endpoint instead of creating a new one.
It seems ok to me to do so. I don't see any harm for "editors" to have access to this information (without the ability to modify it)
There was a problem hiding this comment.
Just commenting on the REST API side of it, that makes sense to me. We do need to adapt the permission check to not just be edit_posts though, it needs to include any show_in_rest cpt. See \WP_REST_Themes_Controller::check_read_active_theme_permission for an example.
There was a problem hiding this comment.
Thanks for the feedback!
Ah I see what you mean about WP_REST_Themes_Controller::check_read_active_theme_permission - so if a user had rights to edit any type of post, they pass the test?
|
I think it'd be good to bring some REST API folk into this conversation (maybe @TimothyBJacobs?). Also some other contributors that know global styles well (perhaps @oandregal and @ajlende?). (The TLDR is that as of 6.6 the post editor uses the global-styles endpoint, but a problem is that only admins (users with The proposal here is to lower the permissions to |
|
I've just been experimenting with the ideas proposed in this PR in: Mainly to judge the approach, and test viability. There might be bugs 😄 but if folks think it's a reasonable direction we can pull it across to this PR. |
youknowriad
deleted the
try/allow-non-admin-users-to-read-global-styles
branch
Related: #64755
What?
This draft PR is only intended to illustrate the sort of changes required to make global styles data available in the post editor to non-admin users (users without
edit_theme_options).Why?
Global styles data is relied upon in the post editor to generate block style variation styles. It will also be needed in the current work around style inheritance and displaying inherited values in the block inspector controls.
How?
edit_postscapabilities to read base theme global styles.Testing Instructions
Known Issue
This approach seems to have reintroduced an issue in the post editor where there is a delay in block style variation styles being applied. This was solved previously by adding preloaded paths for global styles when in the post editor context. I'm not sure what I'm missing this time around.