-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Fix: the trash post action doesn't take into account user capabilities. #62589
Fix: the trash post action doesn't take into account user capabilities. #62589
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: +84 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
ae3839d
to
09c79cc
Compare
successMessage = sprintf( | ||
/* translators: The item's title. */ | ||
__( '"%s" moved to trash.' ), | ||
function useTrashPostAction( postType ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like where this is going. I understand why this was done but I like to think actions should be global objects that don't require a hook to define them. This is going in the opposite direction of all my extensibility PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @youknowriad, this seems important to address on WP 6.6, we will be showing the trash button on the post editor on posts a user has no permission to trash, which is a considerable regression. As of right now there is no reliable way to fix the issue without using a hook, as soon as our APIs allow it and we can fix the issue without using a hook I'm happy to iterate and remove the hook here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed 6.6 is close, so I'm totally fine with this fix for 6.6 but we should definitely look at better APIs for actions after that.
@@ -1020,6 +1062,7 @@ export function usePostActions( { postType, onActionPerformed, context } ) { | |||
); | |||
|
|||
const duplicatePostAction = useDuplicatePostAction( postType ); | |||
const trashPostAction = useTrashPostAction( postType ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we check dynamically add the action here instead of trying to check for the capabilities in isEligible
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ellatrix, because a user may be able to trash a post and not another one. The permissions depend on the specific post. For example, a user maybe have permissions to delete only drafs, or permission to delete drafs authored by that user or permissions to delete private posts but not public ones, etc... We need to check for the specific post in isEligible.
I minimised the diff, so it's easier to review and less likely to have cherry-pick conflicts. |
@@ -1048,7 +1078,7 @@ export function usePostActions( { postType, onActionPerformed, context } ) { | |||
isTemplateOrTemplatePart ? resetTemplateAction : restorePostAction, | |||
isTemplateOrTemplatePart || isPattern | |||
? deletePostAction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need the same for deletePostAction
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need the same for some other actions, my plan was to define the fix here and after we agree on a fix I will replicate it to the other cases where it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested with the testing instructions, and it seems to be the same capability: I can also not delete it permanently in trunk.
label: __( 'Move to Trash' ), | ||
isPrimary: true, | ||
icon: trash, | ||
...trashPostAction, | ||
isEligible( item ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the isEligible
call here won't be re-called when the value of canUser( 'delete', resource, item.id )
changes no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe we need something like useEligible
somewhere. Or maybe this object should define which capabilities to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related discussion #62505 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will because we make the memo depend on canUserResolvers (details in #62589 (comment)) so when the resolver finishes a rerender happens and isEligible will be recalled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤨 seems fine for now but not ideal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very hard to follow. I'm not sure I understand what canUserResolvers
is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
canUserResolvers is the result getCachedResolvers().canUser. So basically when there is a change in the caching of canUser resolvers we will rerender making isEligible recalled. I will add some inline comments, explaining what we discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why keeping the action as object just to be used inside the hook
action? Anyway since it's for 6.6, but it becomes clear that an action needs a way to have access to the store in more places than the callback and we should work towards that direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ntsekouras I did that to minimise the diff, it was huge and unclear what changed + less likely to have cherry pick conflicts, and if it does, easier to resolve manually (I don't know what has changed in these files recently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it's useful for reusability #62754 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not ideal, but we're not exposing any public APIs.
cc4ac6c
to
3dce17e
Compare
3dce17e
to
df4207a
Compare
Handled in #62823. |
Right now we show the trash post action all the time even if the user does not have the correct capabilities.
We essentially regressed on #23144.
This PR adds equivalent checks to the ones added at #23174.
It is a bit hacky because we are calling a selector inside isEligible and to make sure things rerender we make the memorization depend on the result of getCachedResolvers.
It is also not ideal in the sense we make a REST API request per post to make sure the user has the permissions. In a list with 50 posts, it is not a very good solution.
I considered not using canUser, and instead retrieving the capabilities of the post and the user: delete_others_posts, delete_post, delete_posts, delete_private_posts, delete_published_posts,delete_published_posts. Implement the verification on the client e.g.: check if the post is published or private, and verify if the user capabilities are there. These would only depend on postType and user so we would not require a request per post. However, it is incompatible with some ways developers change capabilities (e.g.: the sample below would not work with this approach).
So I opted for the more reliable approach that we were already using.
This should be part of 6.6 as showing the UI to trash posts when the user has no capability for that is a regression.
cc: @youknowriad, @ntsekouras as people inside the actions work.
Reviewers note:
Hide whitespace: https://github.com/WordPress/gutenberg/pull/62589/files?w=1
The only changes are the inclusion of useRegistry and useSelect and their use inside isEligible, excluding these changes the action was kept exactly as before.
Testing Instructions
I pasted the following code snipped on load.php:
I verified the option to trash pages was not available on Dataviews pages list and site editor.
I opened a post on the post editor and verified that the action to trash the post was unavailable.