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.

Sign up for GitHub

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

Jump to bottom

Global Styles: Allow non-admin users access to global styles data in post editor #64797

Closed
wants to merge 6 commits into from
Closed

Global Styles: Allow non-admin users access to global styles data in post editor #64797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3a91175
Allow base global styles data in post editor
aaronrobertshaw Aug 26, 2024
06f1fbd
Try getting user global styles working
aaronrobertshaw Aug 26, 2024
790febd
Add working permissions checks for global styles base config
talldan Aug 27, 2024
24d51e8
Check there is a current theme stylesheet before trying to access bas…
talldan Aug 27, 2024
4baa390
Revert "Add working permissions checks for global styles base config"
talldan Aug 27, 2024
a1b00fc
Remove placeholder code
aaronrobertshaw Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions lib/class-wp-rest-global-styles-controller-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,12 +586,11 @@ public function get_item_schema() {
* @return true|WP_Error True if the request has read access for the item, WP_Error object otherwise.
*/
public function get_theme_item_permissions_check( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable

/*
* Verify if the current user has edit_theme_options capability.
* This capability is required to edit/view/delete templates.
* Verify if the current user has edit_posts capability.
* This capability is required to view global styles.
*/
if ( ! current_user_can( 'edit_theme_options' ) ) {
if ( ! current_user_can( 'edit_posts' ) ) {
Copy link
Member

@ramonjd ramonjd Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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:

https://github.com/ramonjd/wordpress-develop/blob/30523ee4b6a0d33b1c2bd8c9ca4021b6256a96c3/src/wp-includes/post.php#L492-L492

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ramonjd ramonjd Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, exactly.

return new WP_Error(
'rest_cannot_manage_global_styles',
__( 'Sorry, you are not allowed to access the global styles on this site.', 'gutenberg' ),
Expand Down
1 change: 1 addition & 0 deletions lib/compat/wordpress-6.6/rest-api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ function gutenberg_block_editor_preload_paths_6_6( $paths, $context ) {
$paths[] = '/wp/v2/global-styles/themes/' . get_stylesheet();
$paths[] = '/wp/v2/themes?context=edit&status=active';
$paths[] = '/wp/v2/global-styles/' . WP_Theme_JSON_Resolver_Gutenberg::get_user_global_styles_post_id() . '?context=edit';
$paths[] = '/wp/v2/global-styles/' . WP_Theme_JSON_Resolver_Gutenberg::get_user_global_styles_post_id();
}
return $paths;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core-data/src/entities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ export const rootEntitiesConfig = [
name: 'globalStyles',
kind: 'root',
baseURL: '/wp/v2/global-styles',
baseURLParams: { context: 'edit' },
baseURLParams: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to work ok with context: edit, so maybe it's ok to undo these changes.

plural: 'globalStylesVariations', // Should be different from name.
getTitle: ( record ) => record?.title?.rendered || record?.title,
getRevisionsUrl: ( parentId, revisionId ) =>
Expand Down
7 changes: 2 additions & 5 deletions packages/editor/src/components/global-styles-provider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,13 +146,10 @@ function useGlobalStylesUserConfig() {

function useGlobalStylesBaseConfig() {
const baseConfig = useSelect( ( select ) => {
const { __experimentalGetCurrentThemeBaseGlobalStyles, canUser } =
const { __experimentalGetCurrentThemeBaseGlobalStyles } =
select( coreStore );

return (
canUser( 'read', { kind: 'root', name: 'theme' } ) &&
Copy link
Contributor

@talldan talldan Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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). 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor

@talldan talldan Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

const activeThemes = await resolveSelect.getEntityRecords(
'root',
'theme',
{ status: 'active' }
);

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>.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've pushed a commit that adds working permissions checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if the approach in 790febd is going back towards checking capabilities directly as before #63812. Does that mean we can also roll back the changes to the global styles entity definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If pushing changes to this PR, it might also helps those pinged for input if the PR description is updated to match new changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

__experimentalGetCurrentThemeBaseGlobalStyles()
);
return __experimentalGetCurrentThemeBaseGlobalStyles();
}, [] );

return [ !! baseConfig, baseConfig ];
Expand Down
Loading