Fix: Editing "Page" is broken for low capability users. #68110
Conversation
|
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. |
|
Size Change: +17 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 27f6951. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12519021520
|
Mamaduka
added
[Type] Bug
|
Hey @jorgefilipecosta, 
But in latest versions, the value is now undefined. Have you encountered any PR or change implemented that might have affected template access for non-admin roles? I have not seen it in changelog. |
|
@jorgefilipecosta, users who can edit posts/pages can also read the templates. I think there might be another issue hiding here. When testing with the Editor role, I can also see a failed request for the |
Good point, I will check the settings part, but this code is still needed access to templates may still be removed for some roles, while they could still access pages. |
| selection: getEditorSelection(), | ||
| postTypeEntities: | ||
| post.type === 'wp_template' | ||
| ? getEntitiesConfig( 'postType' ) | ||
| : null, | ||
| }; | ||
| }, | ||
| [ post.type ] | ||
| [ template, post.type ] |
There was a problem hiding this comment.
Can we pass hasTemplate or a similar boolean value? There is no need to re-run mapSelect after every template change.
It's also odd that this line resolves true when template isn't available. Let's check if the template is passed as an empty object when missing.
There was a problem hiding this comment.
Feedback applied 👍
Yes, editors can normally read templates. The issue is that they cannot find the template associated with a page, such as a homepage and special pages, because these are located in the root site entity (they can not know if a page is homepage so they can not know the template used in it). I added a check for canUser read root site. The other 403 errors are unrelated, and I'm fixing them #68146. |
|
Thanks, @jorgefilipecosta! I'll try to do another review tomorrow. I think we need to improve The gutenberg/packages/core-data/src/private-selectors.ts Lines 178 to 182 in c01cb99 |
| @@ -194,8 +195,7 @@ export const ExperimentalEditorProvider = withRegistryProvider( | |||
| editorSettings: getEditorSettings(), | |||
| isReady: __unstableIsEditorReady(), | |||
| mode: getRenderingMode(), | |||
| defaultMode: | |||
| postTypeObject?.default_rendering_mode ?? 'post-only', | |||
| defaultMode: postTypeObject?.default_rendering_mode, | |||
There was a problem hiding this comment.
We need a fallback here in case the server provides a false value. Consumers can unset default_rendering_mode using PHP filters.
There was a problem hiding this comment.
Yes it was part of a debug change I included the missing commit
| console.log( { | ||
| notRender: ! isReady || ! mode || ! hasLoadedPostObject, | ||
| settings: ! settings.isPreviewMode, | ||
| } ); |
I believe that currently, only administrators can determine whether a page is a homepage or a blog page. As a result, it is expected that the selector should only return a value for administrators. For other users, returning a value could lead to a wrong value; for example, if the page were the homepage, the value would differ from what the selector would return. I wonder why "The /lookup endpoint cannot currently handle a lookup when a page is set as the front page". It means the endpoint returns a wrong value for the homepage. Ideally, we fix the endpoint and simplify the selector (it could simply return the endpoint value). But I guess if the endpoint is not already doing this and the logic is on the frontend it may mean there is a reason and it may have wider implications. I will try to look into this. I guess this may be a short-term fix if changing the endpoint becomes a bigger effort or infeasible. Regarding the issue faced of the patterns modal appearing closing and appearing, I think the ideal non-hacky fix, is to preload the template it like we do for the post. It would also noticeably improve the experience and loading performance of the page editor. I will also look into this. |
| const canDiscoverTemplate = canUser( 'read', { | ||
| kind: 'root', | ||
| name: 'site', | ||
| } ); |
There was a problem hiding this comment.
Maybe we should move this check into the getHomePage selector so we don't have to remember this implementation detail.
There was a problem hiding this comment.
Good point I added the check inside the selector.
|
I don't know; it seems too defensive. Non-Administrators can edit pages and view their templates. I would expect It would be nice to resolve https://core.trac.wordpress.org/ticket/48885 and allow more granular controls over settings read/update permissions. |
jorgefilipecosta
force-pushed
the
fix/Editing-Page-is-broken-for-low-capability-users
branch
from
f341116 to
3879409
Compare
|
Hi @Mamaduka, I agree that it would be ideal for editors to have access the pages templates. This could be achieved either by making the settings on https://core.trac.wordpress.org/ticket/48885 available as read-only or by modifying the lookup endpoint to consider special pages. However, for now, I believe our current solution is adequate. I think we still need all of this code. In cases where there is no template—potentially due to custom permissions on the site from a plugin filter—we should default to post-only. And we also should check for user permissions before making the request. Thus, even with the upcoming long-term API fixes, this code remains valuable and addresses the issue in the short term. |
jorgefilipecosta
force-pushed
the
fix/Editing-Page-is-broken-for-low-capability-users
branch
2 times, most recently
from
bec0e41 to
7c7a1c8
Compare
|
I don't think we can use the "lookup" here because there might be "local edits" in the frontend that alter which template to return. This is the reason the algorithm of the "lookup" is recreated in the client kind of. Now, yeah I agree with @jorgefilipecosta that if we want to allow these users to see the templates, we need to ensure they can retrieve which page is the home page and posts page... So alter the settings endpoint to access readonly values. That seems like a big change, so I think being defensive is probably the quickest path for now (fallback to post-only) |
| @@ -134,6 +139,13 @@ interface SiteData { | |||
| export const getHomePage = createRegistrySelector( ( select ) => | |||
| createSelector( | |||
| () => { | |||
| const canReadSiteData = select( STORE_NAME ).canUser( 'read', { | |||
There was a problem hiding this comment.
Should we do something similar for getPostsPageId and getTemplateId
There was a problem hiding this comment.
Yes for getPostsPageId we should and I included the check. For getTemplateId I think it is not needed as that selector never reads settings directly.
|
It seems we have different ideas on how However, I understand the current REST API limitations, and there's no need to delay @jorgefilipecosta's work further. Jorge, if you don't mind, I'll do another round of tests tomorrow morning and approve the fix. |
Curious about your thoughts here? |
|
Ideally, I would expect it to continue with the regular template lookup, considering that custom page templates are more widely used than "home page" and "page for posts". |
jorgefilipecosta
force-pushed
the
fix/Editing-Page-is-broken-for-low-capability-users
branch
from
7c7a1c8 to
f7d93f5
Compare
I'm not sure, even though the homepage and blog pages are only two pages they are impactful showing the wrong template on them may not be ideal. |
|
@Mamaduka thank you a lot for all the reviews and insights 😊 |
jorgefilipecosta
deleted the
fix/Editing-Page-is-broken-for-low-capability-users
branch
Fixes: #68053
The issue arises because, despite checking that the user does not have permission to read templates and passing an undefined template, we were still passing the template-lock mode. Without a valid template, this mode does not function correctly. So to fix the problem when there is no template we use the post only mode.
This change addresses the issue and is necessary; however, the pull request is still in progress. There is a minor bug where the page start pattern modal briefly closes and reopens during loading. I am currently looking for a suitable solution that is not a hacky fix, for the close and reopen.
Testing Instructions
With the 2024 theme and an editor user verify you can create and edit pages and they load in the normal post view.
Verify that the template view is loaded for admins.