As a contributor / author navigation block only shows a spinner

spacedmonkey commented 2 years ago

Description

As a contrubitor or author user, I create a navigation block and all is shown is a spinner. See video.

Step-by-step reproduction instructions

Login as a contributor user
Create new post
Create new navigation block
Spinner never disappears.

Screenshots, screen recording, code snippet

https://user-images.githubusercontent.com/237508/140661583-e636a7b3-9962-4f90-86d0-3870f8b7f685.mov

Environment info

Chrome lastest
Guternberg 11.8
WordPress 5.8.1

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

TimothyBJacobs commented 2 years ago

We're going to fix part of this in the REST API by allowing anyone with CPT edit access to read this information. But we should investigate why there isn't an error message of some kind when that REST API request fails.

getdave commented 2 years ago

I'll take a look at the UI side of this.

TimothyBJacobs commented 2 years ago

Actually, can someone clarify (@youknowriad ?) whether any user should be able to use this block? This would expose all menu data to contributors and up which may be undesirable.

youknowriad commented 2 years ago

I'm not familiar with this block personally but yes, I believe like all blocks, it's supposed to be usable by everyone, so I guess we'd have to adapt its behavior (hide things) for contributors.

getdave commented 2 years ago

Yep. Several tasks will likely need to spin out of this one in terms of making sure the block adapts to different user types.

getdave commented 2 years ago

So as pointed out above I'm getting a 403 Forbidden for the Menus endpoint. This will get fixed but we should add a fallback in the UI which I will do now.

Ok it looks like the fallback in the UI is already in place it's just a bug with Core Data. I have a fix in https://github.com/WordPress/gutenberg/pull/36353.

TimothyBJacobs commented 2 years ago

Thanks @youknowriad. Do you think it is acceptable that this block won't show the "Convert from an existing menu" UI for non-administrators?

talldan commented 2 years ago

👍 I think it's ok that it won't show that for classic menus. Looks like contributors can't access those menus anyway.

Also need to consider whether the user has access to the new wp_navigation post type. If they don't, they can't really edit a navigation block with the way it works right now.

Also, at the moment, wp admin shows the menu items for template parts and navigation menus for contributors, but both show a 'Sorry, you are not allowed to access this page.' message when clicking them:

talldan commented 2 years ago

Also need to consider whether the user has access to the new wp_navigation post type. If they don't, they can't really edit a navigation block with the way it works right now.

I was just checking how this might work for reusable blocks, and it looks like it doesn't 😬

A contributor can add a resuable block created by an admin, edit it, but they can't save it (tested in the post editor). Saving seems to throw an error:

Uncaught TypeError: this.props.setEntitiesSavedStatesCallback is not a function

So it seems there isn't much prior art for this kind of situation.

getdave commented 2 years ago

I'm taking a look at a solution now as the current experience is not suitable.

getdave commented 2 years ago

I started exploring if we could use the block locking API to stop users editing if they have lower permissions. Also experimenting with showing a warning message above the block when it is selected to warn users that they have insufficient permissions to edit the block.

What we really need is a "no edits" mode for blocks.

getdave commented 2 years ago

Actually, can someone clarify (@youknowriad ?) whether any user should be able to use this block? This would expose all menu data to contributors and up which may be undesirable.

I think they should be able to read but not write. The Site Editor needs to display the Navigation block and its items but not allow them to edit the items.

To do that we need the endpoint to allow returning the data for lower permission users.

getdave commented 2 years ago

Following some discussion

Nav block (inc data) should be viewable by lower permission users but not editable.
Given we are in beta period we need to find a simple means of disabling editing whilst allowing viewing.
~We need the REST API endpoints to allow READ requests (currently these fails) but disallow WRITE requests for lower perm users.~ I was wrong about this. We can read wp_navigation posts just fine.
The Site Title block uses a permissions check and renders a completely different set of components for lower perm users. We could explore this for Nav block.
We could also explore using the <Disabled> component.

Also noting that the Site Editor is no longer accessible by Contributor role users so this might be less of an issue.

spacedmonkey commented 2 years ago

We need the REST API endpoints to allow READ requests (currently these fails) but disallow WRITE requests for lower perm users.

You can read data from the menu item endpoint, if you can edit any post you can access the menu data. So what exactly has to change? I am confused

getdave commented 2 years ago

My mistake. We can read wp_navigation posts ok. What we need is a way to check whether

the current user has permission to edit the current wp_navigation assigned to the Navigation block.
(or if no Navigation assigned) whether the user has permission to _create_ and/or _publish_wp_navigation` posts.

The first one is achievable using canUser the second one is not.

spacedmonkey commented 2 years ago

To be clear, there will also need to check permission checks for menus ( as in classic menus) as well not just wp_navigation posts.

talldan commented 2 years ago

To be clear, there will also need to check permission checks for menus ( as in classic menus) as well not just wp_navigation posts.

@spacedmonkey I think this was handled in https://github.com/WordPress/gutenberg/pull/36353, but it doesn't check the permissions up front, just handles the failure response from the GET request.

Users can't directly edit classic menus from the navigation block, so I don't think it's too bad the way it is. What do you think?

(or if no Navigation assigned) whether the user has permission to publish wp_navigation posts.

@getdave I'm not really sure what the distinction is between editing and publishing. Both require saving wp_navigation posts, so I think they're equivalent. You should still be able to use canUser. I don't think the user would be able to edit a navigation block (including creating a new menu after adding the block for the first time) if they can't publish wp_navigation posts with the way it works right now.

spacedmonkey commented 2 years ago

The navigation post type's capabilities map to post post types capabilities. This means a user like a contributor could create a post but not publish it. If a contributor creates a post in the editor, they can submit it for review but not publish.

Canuser do not currently allow you to check for publish capabilities. Just that a user can post to an endpoint doesn't mean they can publish. To see if you can publish, you have check the links and see if the 'wp:action-publish' exists.

getdave commented 2 years ago

@talldan What @spacedmonkey outlines is what I'm experiencing. You can create a post and "save" it but because you cannot publish the wp_navigation Post is never available for reuse once you reload the page. You will see a request going out but it returns 403 explaining you have insufficient permissions to publish.

getdave commented 2 years ago

Also if we find a way to reliably determine that the user cannot create Navigation we should remove the Navigation block from the block picker (but not unregister it).

That way they never even have to see the "You don't have permission to create" message, but they'd still be able to view (but not edit) existing Navigations.

getdave commented 2 years ago

Current state

The Navigation block allows users to control Navigation on a website. This block is potentially used for "site wide" Navigation such as in the header of the site.

The block saves its inner blocks to an entity of Post type wp_navigation.

Currently this block is available to be interacted with in the (Post) editor by all users. That's a problem because historically only "admin" users have been able to edit Menus.

The Goal

The Navigation block should behave in a similar way to Menus in classic WordPress.

The Menus screen is only available to users with the edit_theme_options capability (by default that's "admins" only).

We should replicate that for the Nav block in that it should not be possible to update a Navigation if you don't have that permission level.

However, due to the concept of WYSIWYG editing, we should (ideally) still allow users with lower capabilities to see (i.e. "view") Navigations. They should just not be able to edit them.

Also worth remembering that the Site Editor is only accessible to Admin users, so this is really only a problem for editors which lower permission users have access to.

Editing an (existing) Navigation as a Contributor user

When attempting to "Save", users with insufficient capabilities cannot do so. However this is not made apparant in the UI via any kind of feedback. The only way to know what is happening is to look at the Network tab and notice the 403 request for http://localhost:8888/index.php?rest_route=/wp/v2/navigation/108&_locale=user.

Here's a video showing me as a "Contributor" Role user, trying to edit an existing Navigation (which was created by an Admin):

https://user-images.githubusercontent.com/444434/145561259-ebe03c92-459c-4323-9b78-0c64621d2656.mov

Why does this happen?

As wp_navigation is (by default) mapped to Post capabilities, the user is determined to have insufficient privileges to edit because they don't have the edit_others_posts permission [on the wp_navigation Post].

Why don't we show an error message?

Ideally we could detect the 403 on the response and show an error message. However, currently Gutenberg's getEntity system does not track response errors...

https://github.com/WordPress/gutenberg/blob/1b15cf17199126b73aeb3644be7527e7806e22fc/packages/core-data/src/resolvers.js#L107-L110

...and therefore it is not possible for the React component to determine if/when a request made via the @wordpress/data system has errored.

Using canUser to determine permissions

Why can't we use canUser to determine in advance whether the user has permission to edit? Well... we can like this

wp.data.select('core').canUser('update','navigation', 108);

This allow us to display a message to the user within the block that they don't have permission to edit. This won't stop them interacting with the block, but at least they'll know they can't save.

Remember this only works once we have a wp_navigation Post to work with. Running

wp.data.select('core').canUser('update','navigation');

...will return misleading results.

Creating a Navigation as a Contributor user

Currently there's nothing to stop a low capability user from inserting a Navigation block and trying to "Create" a new Navigation. However (as shown below) whilst the UI allows them to "Create" a new Navigation, they do not have permission to publish it and thus the UI simply "hangs" in a loading state due to the rest_cannot_publish 403 response from the REST API:

https://user-images.githubusercontent.com/444434/145564330-a85c7fe5-cf29-4eea-bc26-6b90d15e71ba.mov

This seems good - in the same way as above, we can show a feedback message to the user by checking whether they have permission.

However...the problem is we cannot know whether a user has permission until the wp_navigation post is created. This is because it's a Meta capability check against a single Post rather than a general permissions check. You can verify this yourself:

As an admin user.
Create new Post.
Open console and run - wp.data.select('core').canUser('update','navigation');. Do this a few times until the result is not undefined.
Notice it is false - that's misleading. We know we can update Navigations as an admin!
Now switch to a contributor user and repeat the same steps. Again it's false but again that doesn't help us as both permissions checks for admin and contributor users evaluate to false.

That means we have no way to determine whether the user can create Navigations prior to showing them the Create option in the Navigation block placeholder.

Navigations in Private Posts

It's also possible to create Navigations within private Posts. Should a Navigation created in this way be accessible to other users?

Potential Solutions

Map capabilities to `edit_theme_options`

@spacedmonkey has suggested we map the capabilities of the wp_navigation post type to edit_theme_options. This will effectively mean that only "admins" can access Navigation posts.

One concern about this approach is that it would mean that lower permission users would not be able to "view" Navigations at all (see "Goals" above). However, this might not be a problem given that the Site Editor is not accessible to lower permission users.

Show UI warning messages where possible

Either way, we should use the existing canUser system to display UI messages to users when we can accuracyely determine they have insufficient capability to interact with the Navigation block.

I've started exploring this in the following PRs:

Remove the Navigation block from the inserter

We should hide the Nav block from the inserter if the user does not have permission to view. We should not unregister the block however, as that would cause errors if the user opens a Post containing the Navigation block. Which should simply not allow them to edit.

I'm exploring in https://github.com/WordPress/gutenberg/pull/37291

Restrictions/limitations

As we are in the 5.9 Beta period we can only take steps to fix this bug. We cannot invent new features.

Therefore, we need to come up with an expedient solution that is achievable with the remaining Beta period.

Next Steps

I'd appreciate it if folks could:

validate and confirm my assessment above
advise on their preferred solutions

Once done I recommend we spin out several PRs to tackle each part independently. I have already created a "god" PR in https://github.com/WordPress/gutenberg/pull/37029 but I'm going to close it and then pull it apart and harvest it for solutions.

spacedmonkey commented 2 years ago

For some extra context here, we had to tweak the permission for nav_menu_item to look like this.

We can create a custom REST API controller / tweak permissions so that navigations posts public to read by limited in who can create them. This means that navigation blocks are seen as public first, not sensitive. If navigation are seen as public, if you embed one in a private post / draft post, you know that it is public. However, this would only make sense in a world where we had a navigation editor, when you create a public menu and then simply embed into the post / page.

For navigation posts to work as expected, even contributor users would have to have the ability to publish navigation posts. It's worth noting, that just being able to create a post does not mean that you can publish it. Contributors can create a post in the editor but are not allowed to publish them. A post goes into a pending state and another editor/admin user can review and publish it for them. The current checks for wp.data.select('core').canUser('create','navigation'); does not take into account if a user can publish the post or not. Weather a user can publish is post or not is not based on a global capability, but is on a post by post basis. One user may have access to publish there own but not others posts for example. To work out if user can publish a post, you must first have id. Then you can do a request and check if the action-publish key exists in links.

I believe we need to update canUser to support a check for publish capability.

I am personally worried about timelines here, as we are currently beta 2, resolve this may take some time. Stripping the navigation block out anything other than the site editor, maybe the best path forward.

getdave commented 2 years ago

we need to update canUser to support a check for publish capability.

I can do that. I'll spin up a dedicated PR.

spacedmonkey commented 2 years ago

Another idea, would be treat navigation post the same as attachments and use the Inherit post status. This would make the navigation post a child post of the post/page/template_part. When capability checks are done, there are not then done on the navigation post but on the parent. In our case, this solve the private post and contributor issues, because if you are viewing the post, then you should be able to see the navigation.

It would mean a pretty simple change to the JS, to pass the parent id to save / update call. There will likely be some small tweaks to the rest controller, but extending the existing post controller not a custom controller. Thoughts @getdave

It is also worth noting, that this would make it harder to support the navigation block in widget screen and contexts that do not use posts, widgets do not store in post, but options, so not have post id. But as the widget block screen doesn't support the navigation block, we can cross that bridge when we come to it.

getdave commented 2 years ago

This is an interesting idea. I had no idea this concept even existed.

Whether we pursue it is determinate on whether we agree that Navigations should be considered to be connected to an individual Post/Page. There's no existing precedent for this in Menus as far as I know.

Also, what happens when we're in the Site Editor? How is the capability determined then? Based on the Template Part?

My instinct is that Navigations are a global concept and thus we don't need to worry about whether they were created in private Post but I could be very wrong. I guess as a user, I just feel like I'm creating content in a Post - I'm not necessarily aware that my Nav items are saved to a wp_navigation Post. Therefore when those items are visible to other users I might be very surprised.

How much work would it be to make your proposed changes to the Post and the REST API? Could we experiment with it quickly or is it a large investment of work?

draganescu commented 2 years ago

Stripping the navigation block out anything other than the site editor, maybe the best path forward.

I find this appealing, even short term. What is the purpose of using a navigation block as it is today in any other place but template parts?

Longer term, from the description of the problem I understand that permissions do work, but a user without the right perms has a poor UX. Ideally,

as a user that cannot create/save/publish navigation posts, on which the block relies entirely, the content should be readonly, and the UI should tell me that it is readonly because of perms. This could be generalized to all entities that can't be created/saved/published because of perms and the corresponding blocks that use them.

Would this be a better experience than hiding the block from the inserter?

getdave commented 2 years ago

Stripping the navigation block out anything other than the site editor, maybe the best path forward.

Can I check whether this means

Hide the block from the inserter in any but the Site Editor?
Unregister the block in any but the Site Editor?

spacedmonkey commented 2 years ago

@draganescu It isn't just a case of create/save/publish it is also read the a couple of cases where a post could not be accessed.

User does not have rights to read / edit others posts.
Navigation post is embedded in private post, so should not have access.
Navigation post is marked as draft.

The key thing is, we need the navigation block in site edit, but we can remove/unregister it from other editors until we can really solve these problems.

How much work would it be to make your proposed changes to the Post and the REST API? Could we experiment with it quickly or is it a large investment of work?

I do think it is something that should be easier enough to hack together a POC. Might need a little help with the JS, but I could get something working early next week.

getdave commented 2 years ago

Stripping the navigation block out anything other than the site editor, maybe the best path forward.

@spacedmonkey @draganescu Please see https://github.com/WordPress/gutenberg/pull/37291.

Also note

spacedmonkey commented 2 years ago

I have a possibly solution here. https://github.com/WordPress/gutenberg/pull/37302

I would love a review.

TimothyBJacobs commented 2 years ago

I'm not sure we should be introducing more usage of inherit in WordPress Core, it adds quite a bit of complexity because a lot of inherit handling is special-cased to the attachment post type. Beyond that, the use of inherit already poses us problems in the REST API when the "parent" post gets deleted or has it's post status transitioned.

I like the approach of setting the capabilities to edit_theme_options like we have elsewhere. We can use map_meta_cap to allow for users who can edit any post type to have read only access to the navigation post type.

Ccing @peterwilsoncc who has looked a lot at post type capabilities.

spacedmonkey commented 2 years ago

I personally agree with you @TimothyBJacobs but I am not sure I see another way around this. If I build a private, draft or scheduled a embed a navigation block in it, if that data is public, it could be seen as leaking information, as if any users can access any navigation, you could see what was in a private post. Consider the following example.

Boss of company writes a blog post, saying they are going to be job loses, schedules the post go out on the 1st of Jan. He / she / they embed a navigation block that links to information about the job loses. If user b, creates a post and lists the navigation block, they could embed the navigation block used in the job loses post and find out the contents of that blog post before it's public.

When you consider a navigation a part of the parent posts content and not a global and public thing, then inherited is the only mental model for navigation blocks that start life as part of content. Again, if we had a navigation editor, this would be different but if a navigation post / block starts life in the editor, there, it should follow the permission model of the parent post. However, navigation posts, created in the site editor, should always be seen as public and do not have the same rules.

spacedmonkey commented 2 years ago

Another very simple workaround, is to only save navigation posts, if you are in the site editor and save the navigation block data into the contents of the current post you are editing. That would mean, blocks are not available to select unless you created the in site editing.

TimothyBJacobs commented 2 years ago

I don't think we necessarily need to solve for that case in the initial release as long as it is clear to the user that menus are available to be selected to all authors. There is a similar issue with Reusable Blocks. I think a solution for both would be to allow publishing privately with the private status.

Another very simple workaround, is to only save navigation posts, if you are in the site editor and save the navigation block data into the contents of the current post you are editing. That would mean, blocks are not available to select unless you created the in site editing.

I think this is a good solution as well.

noisysocks commented 2 years ago

That means we have no way to determine whether the user can create Navigations prior to showing them the Create option in the Navigation block placeholder.

Shouldn't we be using canUser( 'create', 'navigation' ) instead of canUser( 'update', 'navigation' )?

update requires an ID because it's analogous to PUT.

https://github.com/WordPress/gutenberg/blob/2e04e2631a3933c6642f7e8540dfe05175260d93/packages/core-data/src/resolvers.js#L268-L273

edit: Never mind, Johnny explained in https://github.com/WordPress/gutenberg/issues/36286#issuecomment-990907809 that the issue is we need to check "can publish" and not "can create".

talldan commented 2 years ago

Ideally we could detect the 403 on the response and show an error message. However, currently Gutenberg's getEntity system does not track response errors...

There's a getLastEntitySaveError selector that should help with that..

carolinan commented 2 years ago

Please help me understand better. If users, including admins, are not able to create new templates and customize all parts of that template using template editing, -including the header and footer area with their navigation menus -then what use is the template editing for classic themes? Not being able to create new fully working templates would be a big step backward. If I want a template for a page or post, but I can't have a navigation menu, then how is that template going to be useful? The template editing mode in the block editor was meant to be the bridge between classic themes and full site editing themes.

getdave commented 2 years ago

Hi @carolinan. This particular issue is focusing on the Navigation block and how it behaves/presents itself for users with lower capabilities/permissions (e.g. contributor role users).

...not able to create new templates and customize all parts of that template using template editing, -including the header and footer area with their navigation menus -then what use is the template editing for classic themes?

Specifically in terms of Nav Menus, the concerns I highlighted are that some users shouldn't have permission to edit certain key areas of the site. Navigations are potentially key areas of a site.

I currently believe this paradigm is consistent with classic Themes, in that - for example - only Admins can edit Menus (under Appearance -> Menus) in classic Themes.

Applying the same thinking to FSE means that those same users shouldn't be able to make edits to Navigation Menus (via the Navigation block). This is what #37286 is trying to address.

I hope that helps? Let me know if there's something that's not clear or if you have any questions.

carolinan commented 2 years ago

But the current solution in 37291 is to remove it for all users of template editing, not only non admins. Users of existing themes that are not converted to block themes are now going to get a significantly less useful template editing experience.

getdave commented 2 years ago

Ok understood. Perhaps we can discuss the concerns about disabling in Site Editor over at https://github.com/WordPress/gutenberg/pull/37291.

The reason I didn't merge this is that I want to get more feedback on cases such as those which you mention. We may require more nuance that blanket "disable in everything but Site Editor". Consider the PR a proposal for review and feedback.

Appreciate your time here.

getdave commented 2 years ago

Recommendations for WP 5.9 Beta 3 or 4

I've been working with others on a range of solutions to this problem around lower permission users' interactions with the Nav block.

I now believe that in order for a fix to be in place for WP 5.9, the best and most expedient option is to display a UI warning to the user explaining that their changes cannot be persisted.

I recognise that ideally, if the user has insufficient permissions, the blocks would be entirely read-only. However, we do not have sufficient time to extend/enhance the existing block locking system or scope to create a new API to enable this (although this is something we should pursue in WP 6.0).

We should consider that these permissions issues will likely only effect a small subset of users because:

Most Navigations will be added via the Site Editor and only admins can view the Site Editor.
Page/Post templates are only accessible to admins.

Nonetheless, whilst we cannot make blocks read only, we should endeavour to at least alert users to the fact that their edits will not be persisted.

I have x2 PRs which attempt to do this:

I believe these are good to merge. They may require some follow ups but they move us towards a better experience.

What we now need are:

technical review and ✅ - possibly @spacedmonkey and @adamziel / @talldan ?
design/UX review and ✅ - possibly @jasmussen?

Thank you all in advance for your help 🙇

jasmussen commented 2 years ago

I left a comment on #37286, which — of the two suggested — feels like the best path forward to me: no layout shifts, and clearly announced error.

spacedmonkey commented 2 years ago

I believe we missed the cut off for beta 3 already. I believe it is due out Wednesday. Beta 4 is 100% possible.

carolinan commented 2 years ago

Beta 3 is the string freeze, so 4 would be too late to add a new error message.

WordPress / gutenberg