Let modules opt in to the bundle-specific permissions form

I created a MR for this issue, so I am setting it to NR.

The failing tests in #2934995-124: Add a "Manage permissions" tab for each bundle that has associated permissions are because the new link type was not declared. I fixed that by adding user.link_relation_types.yml. But then the same tests failed a few lines later. I tracked this down to Drupal\rest\Plugin\rest\resource\EntityResource::addLinkHeaders(). It seems that these link templates have to be added to the bundle entity type (such as NodeType, not Node).

I think we should improve the documentation for link templates. If there is any documentation explaining the points I just mentioned, I could not find it.

Since #2934995 adds the bundle-specific Permissions form to any entity that has bundles and sets field_ui_base_route in its annotations, I enabled the form for the following entity types:

• BlockContentType
• CommentType
• ContactForm
• MediaType
• NodeType
• Vocabulary

I am pretty sure we want the Permissions form for the last three. Maybe we also want it for the first three. Even if the module defining an entity type does not define bundle-specific permissions, other modules may. In particular, the core Configuration Translation and Layout Builder modules create permissions for any configured bundle types. The access checker added in #2934995 makes sure not to show the page unless there are some permissions to manage.

In the commits I just added, I dropped the check for field_ui_base_route when defining the route. I kept that check when defining the local task.

If field_ui_base_route is not defined, should we still create a local task? If so, should the base route be the edit form for the bundle?

I am adding related issues. I think the issues for the Group module can be closed (outdated) if this issue is fixed.

Here is a shortened version of @Berdir's review from #2934995-126: Add a "Manage permissions" tab for each bundle that has associated permissions:

1. An additional route provider might be an option. it does require another entity type annotation, but the system is designed to have multiple route providers. That would also avoid the problem that some core entity types do not yet use the default html route provider, they could just additionally add the user one.

2. +++ b/core/modules/user/user.module
   @@ -1323,3 +1326,41 @@ function user_filter_format_disable(FilterFormatInterface $filter_format) {
   +
   +  if (!$bundle_entity_type = $entity->bundle()) {
   +    return [];
   +  }
   +
   

   I don't think you need to worry about this case, bundle is always defined, even for entity types that don't have bundles.

3. +++ b/core/modules/user/user.module
   @@ -1323,3 +1326,41 @@ function user_filter_format_disable(FilterFormatInterface $filter_format) {
   +
   +  $url = Url::fromRoute($route, [$bundle_entity_type => $entity->id()]);
   +  if (!$url->access()) {
   +    return [];
   +  }
   

   Hm. I think I missed how the url/route access is called through this, makes sense that we tried to optimize it then. This is quite slow. not only do we need to go through route upcasting, the access check then loads the bundle entity and does the permission check.

   Do we really still need that? ...

My replies:

1. This is the option I am using on this issue. Thanks for suggesting the approach.
2. While there was still a possibility of getting the feature back into 9.3.x, I wanted to avoid non-essential changes. Now that we are targeting 9.4.x, I fixed this.
3. I already said, in #2934995-130:
   

   ... there are other reasons to keep the check. For example, the taxonomy module does not know whether the config_translation or layout_builder modules are enabled, or if they are configured to apply to any vocabularies.

   According to @danflanagan's testing (see the MR for #2934995) the slowest part of the access check is not the overhead of $url->access() and it is not calculating the permissions. The slowest part is computing the config dependencies.

I just added #3254866: Update the deprecation notices related to "Manage Permissions" pages.

@Berdir, @kristiaanvandeneynde:

Thanks for the reviews. I updated the MR with your recommendations. I am afraid I made the testbot work a bit, but I hope the latest commit will pass tests.

One more fix for core/modules/system/tests/src/Functional/Menu/LocalTasksTest.php.

There is another failing test, but that is because of #3255749: Composer v2.2 prompts to authorize plugins.

Update: the failing test is caused by #3255836: Test fails due to Composer 2.2.

I updated the CR https://www.drupal.org/node/3242827.

No one replied to my comment in #3 about the core entity types, so the current MR does what I said there: it enables the Permissions form for every core entity type that was affected by #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions. That means that this issue does not create any user interface changes.

Or should I consider contrib modules as well? A contrib module might get a Permissions form after #2934995, and it will be removed by this issue unless the module is updated to "opt in".

I added a release-notes snippet.

I am removing the tags for CR/IS updates. Since there are no changes to the MR, I am moving the status back to RTBC.

Thanks to all for the attention to this issue.

I am happy to have the status set back to NW.

While revising the CR, I looked at the code again and realized how little it has to do with bundles. Originally, this feature had a lot to do with bundles. We start with a content entity, like Node, then look for the bundle entity, like NodeType.

In the current version, we start with the bundle type and look for the related (content) entity type. But that is only used in a few places:

1. The (content) entity type is where field_ui_base_route may be set. The deriver for local tasks (tabs) needs this to decide where (if anywhere) to add local tasks.
2. The (content) entity type is where permission_granularity may be set to "bundle". If so, then the access checker skips the dependency calculation and grants access.

Of these, (1) makes a lot of sense, but it is independent of the rest of the feature.

In order to support (2), the route provider checks whether the current entity type is a bundle type. If not, it exits early. If so, then it adds an extra default to the route to save a few steps for the access checker.

Instead of (2), we could provide an "opt in" strategy for the access checker. The Node module creates permissions based on each NodeType entity (content type). It can opt out of the access check. The Block Content module does not create permissions for each BlockContentType entity (custom block type), but other modules (Layout Builder, Configuration Translation) can. So Block Content should opt in to the access check.

The mechanism would be to provide two route providers. One would extend the other and provide the access checker.

What do you think? Should I make the changes requested in #17 and open a follow-up issue for this suggestion? Or if everyone thinks this is a good idea, then I can implement it as part of this issue.

@AaronMcHale:

I disagree on both points: see my replies on the MR (or the MR comments should appear just above this).

As I said in #19, I am thinking of making further changes anyway, but for now I am setting the status back to NR with no code changes.

the "resolve thread" button apparently isn't actually doing anything for me.

Right, only the person who opens the MR, and maybe core committers, can resolve a thread. I think the person who opens a thread should decide when it can be closed, so thanks for being explicit about that.

I marked one thread as resolved, and I replied on the other.

The commits I just added implement part of what I suggested in #19.

When I wrote #19, I forgot one crucial use of the "related (content) entity type". We need it in order to use EntityBundleRouteEnhancer (formerly FieldUiRouteEnhancer). The job of that route enhancer is to provide a (string) $bundle parameter to the form builder, even though the original parameter is something like $node_type.

Getting around that seems out of scope for this issue. I may work on a follow-up issue to do it. Then again, that may be a solution without a problem.

I did make several changes with the idea that we might later remove the reliance on EntityBundleRouteEnhancer:

1. Change the name of the route from entity.$entity_type_id.bundle_permissions_form to entity.$entity_type_id.entity_permissions_form.
2. Change the link relation type from bundle-permissions-form to entity-permissions-form.
3. Change UserPermissionsBundleForm to UserPermissionsEntityForm.

I also gave into temptation and fixed a couple of code comments. (See the commit messages starting with "Fix".)

If any of that seems out of scope for this issue, I can revert those commits.

The substantive changes to implement the opt-in strategy for the access checker are

1. Do not use permission granularity as a shortcut.
2. Remove the access check from the entity_permissions_form route.
3. Add a route provider that includes the access check.
4. Use the custom access checker when needed.

Change Bundle to Entity in method names, variable names, and comments.

I think the test failures are not related to this issue/MR. See #2829040-143: [meta] Known intermittent, random, and environment-specific test failures.

entity-permissions-form/entity_permissions_form, can we simplify that to permissions-form/ermissions_form, or does that come too close to causing the same issues with the group module, as the word "entity" seems redundant there?

I do think the shorter version runs the risk of conflicts. The longer version will be consistent with the proposed changes (see below) for the class names.

For those implementing this in custom/contrib entities, we need to be really clear about when it's appropriate to use UserPermissionsRouteProviderWithCheck vs UserPermissionsRouteProvider, we could add more info to the docblocks in addition to the change record and also on the entity API docs pages.

I will add something to the doc blocks.

Do we need to prefix those classes with User, seems redundant and possibly confusing, implying that they are specifically for user permissions?

I am reluctant to refer to Object-oriented code > Naming conventions. The recommendations there confuse me.

Let's look at what core modules have so far:

$ find core/modules -name '*RouteProv*'
core/modules/media/src/Routing/MediaRouteProvider.php
core/modules/user/src/Entity/UserRouteProvider.php
core/modules/node/src/Entity/NodeRouteProvider.php
core/modules/aggregator/src/FeedHtmlRouteProvider.php
core/modules/system/src/Tests/Routing/MockRouteProvider.php
core/modules/content_moderation/src/Entity/Routing/EntityModerationRouteProvider.php
core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php

I wish the namespaces were more consistent: Routing, Entity, and Entity\Routing are all used.

Both NodeRouteProvider and UserRouteProvider are used only in the modules that define them. I did not check the others.

So I think the question, and the implicit suggestion, are on point. There is no need to start the class names with User.

@AaronMcHale, will you be disappointed if I change the class names to EntityPermissionsRouteProvider and EntityPermissionsRouteProviderWithCheck?

While I am at it, maybe I will change UserPermissionsEntityForm to EntityPermissionsForm.

I rebased on the 9.5.x branch because of #27. I made the changes I promised in #26:

• Change class names.
• Add documentation to the route provider documentation blocks.

@AaronMcHale:

See #28: I already made the changes that I proposed in #26.

I do not really like ReallyLongClassNames, so I think I prefer not to add "Form" as in EntityPermissionsFormRouteProvider and EntityPermissionsFormRouteProviderWithCheck. Since you listed that as a suggestion, are you willing to call this issue RTBC?

One other thought: now that we make it possible to opt in or out of the access check, it is possible to get an empty form. I hacked BlockContentType to generate this screenshot:

I think we should avoid the do-nothing form, but maybe we can leave that for a follow-up issue.

I am converting this issue to a child of #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions.

I added a sibling issue: #3279355: Do not show an empty table when there are no entity-specific permissions.

I updated the change record Add "Manage permissions" tab after "Manage display".

We missed the deadline for 9.4.0-alpha1. I do not know if this feature is eligible for the beta release. For now, I did not change the version mentioned in the deprecation notices and the change record. If this issue is not eligible for a back-port to 9.4.x, then we will have to do that.

@kristiaanvandeneynde:

I think you missed #27 on this issue:

After discussing this with @catch we decided to revert #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions from 9.4.x to give us a chance to land this in 9.5.x.

So the parent issue was reverted on 9.4.x, although there are no new comment on that issue. That revert is now reverted, and this issue is committed.

@alexpott:

+1 to "yay!"

I was prepared to wait another 6 months for 9.5 and 10.0, but now I can look forward to the beta release of 9.4. :)

I removed "[PP-1]" from the title of the follow-up #3279355: Do not show an empty table when there are no entity-specific permissions. (I forgot to set the status to Postponed when I created the issue.)

I see you already published the change record. +1