Add a "Manage permissions" tab for each bundle that has associated permissions

Adding credit for @larowlan since this was his proposal in #2862422: Add per-media type creation permissions for media and #1975064: Add more granular block content permissions

And @yoroy and @SKAUGHT for these proposed UIs. (There are probably others who should be added as well.)

I like this idea a lot, this would be very helpful especially when creating new bundles.

One question, though, the proposed UI has a checkbox that implies those permissions are optional. But with the way our current permissions work, that's not really possible because we can not take away permissions. This UI is similar to field permissions module but the difference is that by default, fields are editable. So it is by default editable unless you start to set permissions.

This is not the case for bundle/entity-permissions, by default, access is not allowed. The only way this could work is with a rather awkward fallback permission: " create/update/delete any non-customized node type".

It probably makes more sense to simply put them in a collapsible/collapsed details element and if available in a vertical tab?

Also wondering about the best way to implement this. It could be a custom form element that would accept a list of permissions #permissions and possibly some other limiting options?

At today's UX meeting, we got onto the subject of the permissions Ui. I like both of these proposals, especially proposa 1.

#2809177: Introduce entity permission providers could be helpful for Proposal 1 ("on the bundle forms").

Wow, proposal 1 looks AMAZING! :D

On looking at this again, I think Proposal 1 would be the most ideal.

Something very similar to Proposal 2 is implemented in #3216341: Provide a module-specific permissions form.

• All we need is a link to the module-specific permissions form from the confirmation message when adding the bundle (e.g., a new custom block type).
• You cannot use /admin/people/permissions/{module} for the path, since that conflicts with /admin/people/permissions/{role}.

Of course, it would be great to implement both proposals.

That's an interesting idea @benjifisher, could be a quick win since we would then already have the foundations in place once #3216341. From a purely technical stand point that's probably the simplest option to implement but agree that it would be nice if we could get Proposal 1 in.

I guess a more fundamental problem, is as far as I can remember we don't have a Content Entity Form class specifically for Config Entities which are used as bundles, so at the very least we'd need a new Form Class or a Trait that both Core and Contrib Config Entities could use. We'd also probably need a way for permissions to be defined as relating to a Bundle, similar to what #2571235: [regression] Roles should depend on objects that are building the granted permissions did recently.

Maybe that's a further use-case for actually progressing #2809177: Introduce entity permission providers, which would abstract some of that away and of course remove a little bit more boilerplate work when creating Entity Types.

As I said in #19, #3216341: Provide a module-specific permissions form implements something similar to Proposal 2. The page /admin/people/permissions/module/comment shows just the permissions provided by the Comment module. (We cannot use the path /admin/people/permissions/comment since that is already taken: it shows one column of permissions, for the comment user role.)

I think the implementation of #3216341: Provide a module-specific permissions form goes about half way to implementing Proposal 1. We did a little refactoring of the class Drupal\user\Form\UserPermissionsForm, adding the method permissionsByProvider():

  /**
   * Group permissions by the modules that provide them.
   *
   * @return string[][]
   *   A nested array. The outer keys are modules that provide permissions. The
   *   inner arrays are permission names keyed by their machine names.
   */
  protected function permissionsByProvider(): array {

In #3216341, we extended UserPermissionsForm and overrode that method, starting with the parent method and selecting one or more of the outer array keys. For this issue, we could do that, then select inner keys for the newly added bundle. That gives you the form you want for Proposal 1. Now, you just have to insert it on the page and figure out how to deal with the super-wide table on sites that have dozens of user roles.

For custom blocks (similar to other entity types) the bundle-edit page already has several tabs:

• Edit
• Manage fields
• Manage form display
• Manage display

If you are willing to add another tab, "Manage permissions", instead of inserting the form on an existing page, then the suggestion in #21 gets you about 80% of the way there.

In #2571235: [regression] Roles should depend on objects that are building the granted permissions, we added Drupal\Core\Entity\BundlePermissionHandlerTrait with a single method, generatePermissions(array $bundles, callable $permission_builder). The callback provides the permissions associated to a bundle, and then this new method adds dependencies to the permissions array.

It would be a lot cleaner if the classes that provide permissions callbacks implemented an interface. Then the interface would determine the permission-builder, and we would not have to pass a callable.

There are 9 core modules that have permission_callbacks keys in their .permissions.yml files. In this table, the namespace is always Drupal\module_name.

Unfortunately, the different methods names buildPermissions() all have different argument types, which is why BundlePermissionHandlerTrait cannot declare the method. (We tried that, which is why the patch in #2571235-215: [regression] Roles should depend on objects that are building the granted permissions failed so spectacularly. The patch would have worked if the testbot had been using PHP 7 instead of PHP 8.)

For this issue, I think we want a public version of buildPermissions() declared in an interface that the various Permissions classes can implement. If we can get that done in time for 9.3.0, then we can update BundlePermissionHandlerTrait without having to worry about BC. Then we can deprecate buildPermissions(), which is declared protected in all three classes, make it a wrapper for the new function, and remove it in Drupal 10.0.

The argument for the three buildPermissions() methods only needs to have id() and label() methods.

If you are willing to add another tab, "Manage permissions", instead of inserting the form on an existing page, then the suggestion in #21 gets you about 80% of the way there.

I think that would also be much simpler to implement, and follows good practice of not having one form perform too many different tasks, so +1 to that idea.

I am adding #3216341: Provide a module-specific permissions form as a related issue.

I am marking this issue NR, but the attached patch is just a proof of concept.

This patch works just for the node module. It adds a "Manage permissions" tab to the content-type edit form:

I think this is a good idea, but I would like some other opinions before I put any more work into it.

• Usability: is the new tab worth the extra clutter, and will site administrators easily discover it?
• Complexity: is it worth adding this feature to Drupal core?

The current patch could easily be adapted to a contrib module. If we want to do this automatically for any entity type that implements a new interface that we plan to add, then I think we have to do it in the core entity system.

This makes a lot of sense! When creating a new node entity type, more often than not, the next step is to navigate to /admin/people/permissions, so why not just add a tab to manage the permissions while you’re managing the fields and displays, makes perfect sense to me. IMO, managing the permissions are just as important as what fields are included and how those fields are displayed. I believe a contrib module would be the next step to ensure the concept is battle tested against core and other contrib modules.

@benjifisher I think that's a great proof of concept, I'm fully in favour of us exploring this further!

Regarding your two questions:

1. Usability: I think it is definitely worth the extra tab, agree with everything @Chris Matthews said in comment #27. This really improves the usability of managing permissions for bundles and makes the relevant permissions much more discoverable. A new users to Drupal might wonder why non admin users can't see their new bundle after they created it, not realising that they need to navigate to a completely different and very complex permissions screen. This makes it much clearer that there are permissions to be configured and improves the administration experience for all users.
2. Complexity: I think this is absolutely worth adding to the Core Entity API, there is no doubt in my mind that this is a valuable addition, not only for the reasons mentioned above, but also because it reduces the complexity around finding and managing permissions. From an information architecture perspective, it keeps everything to do with a particular bundle in one place, which is much more sensible than the current approach of needing to navigate to a completely different part of the site. For new users particularly, there is a real risk of getting completely lost in what should be a rather simple and basic task of configuring permissions,

If we had a list of "top tasks" when it came to content types (or entity bundles in general) configuring permissions would be right up there, so I am in full support of this addition!

One additional thing to consider (and this might be more of a follow-up candidate), how do bring the Field UI permissions into this interface; To me it would seems sensible that the permissions that the Field UI generates for a given bundle are also brought in, but that might be more complex. I think that might be made easier if it were possible for permissions to declare that they belong to a specific entity type and bundle, which I don't believe is something we have right now. The same question could be asked for Layout Builder, or any other module which dynamically declares permissions on a per entity type, per bundle basis.

It looks as though we will go ahead with my suggestion. I do not have time right now to update the "Proposed resolution" in the issue summary, so I will add the tag for an issue summary update.

@AaronMcHale:

Regarding your last point, I am not sure what permissions are related to the Field UI. There is a contrib module that adds permissions per field, but I cannot think of anything in core.

... if it were possible for permissions to declare that they belong to a specific entity type and bundle ...

Permissions declare their dependencies after #2571235: [regression] Roles should depend on objects that are building the granted permissions. See the change record Permissions can define dependencies.

@benjifisher

Re permissions, Field UI does generate three different permissions, one for each of the "Manage" tabs, but after looking at a Drupal site I realised it only appears to be per Entity Type, not per Bundle:

So I think Field UI is no longer relevant here (unless there is a way for it an Entity Type to specify the granularity as being per bundle).

Layout Builder does however have permissions per bundle, but only for the bundles that Layout Builder is enabled for:

Regarding #2571235: [regression] Roles should depend on objects that are building the granted permissions, yes I do remember that now that you mention it, it must have slipped my mind. Therefor we can probably use that we find filter the permissions list based on which ones depend on the given bundle.

Updated issue summary.

@benjifisher This patch added Manage Permission tab inside in content type

Thanks for the patch for ref sharing screenshot.....

Step to Reproduce

1. Install Drupal 9.3.x-dev
2. Go to Directory admin/structure/types/manage/page/fields/
3. U will see there is no any tab related to Manage Permissions
4. Now apply patch and Rebuild cache
5. Now u will see after patch there is tab for ' Manage Permissions ' tab where u can set permission

@vikashoni:

Thanks for checking, but I think you skipped one of the important steps listed in the contributor guide Review a patch or merge request:

Read the issue summary.

The issue summary says, in part,

A proof of concept patch can be found in comment #26.

and then, under "Remaining tasks",

Work on the proof of concept patch to work it into a generic implementation that could works for any bundle.

To avoid further confusion, I am updating the issue status to NW.

Rewrote the Remaining Tasks section, I don't know where my head was when I wrote that section, clearly too tired to be write issue summaries heh.

Following up on the discussion from the bottom of #28 to #29 and #30:

If we only care about bundles that are defined by configuration, then we should use config dependencies to generate the list. I have attached a patch that does this. It is still a proof of concept, since it still handles only the node module.

Here is a screenshot, using the Recipe content type from the Umami installation profile. Notice that the content_translation module is enabled, as well as layout_builder, and both define permissions based on the content type.

This method has some advantages over the first patch:

• It does not require an API addition to describe the permissions associated to an entity bundle.
• It automatically handles permissions from other modules that depend on the bundle.

The only disadvantage I can think of is that this method only works for bundles defined by config entities. This includes all the common use cases (content types, vocabularies. media types, custom block types) but bundles can be defined in other ways.

In fact, I have been worrying about another problem with adding an API for per-bundle permissions. We would need something, like a new annotation on the bundle class (e.g., Drupal\node\Entity\NodeType), to tell us what class has the method for generating permissions (or permission names) for a bundle. Normally, that would be the same class that defines the permission callback. But we already have a way to specify that class: the permission_callbacks key in *.permissions.yml. It seems like a bad idea to have two different places to specify the same class.

I think the next steps are

• Make the code more generic, not just for the node module.
• Move the code out of the node module. Perhaps the user module, since that is where the main Permissions page is defined.
• Define the routes and local tasks in PHP code, not YAML files.
• What if there are no permissions depending on a bundle? An empty tab is the simplest solution. Can we figure it out when defining the route without duplicating a lot of code?

Edit: the user module, not the system module defines the main Permissions page.

Great progress here, thanks Benji!

For bundles which aren't based on configuration entities, part of the solution there could be relying on the new BundlePermissionHandlerTrait to get the list of permissions that need to be displayed.

BundlePermissionHandlerTrait only supplies a helper function for adding dependencies. You have to supply that helper function with a list of bundles and a callback that generates the permissions for each bundle.

I talked about this in #23. If we want to support all bundle types, not just the ones defined by configuration options, then we can add an interface for specifying bundle permissions (the API of this issue's title). We could use that interface for this issue and to simplify BundlePermissionHandlerTrait, getting rid of the callback. But then we need some other way to add permissions from other modules.

I mentioned #2571235: [regression] Roles should depend on objects that are building the granted permissions in #23 and #29. That is the issue that added BundlePermissionHandlerTrait and adds dependency information to permissions. I am adding it now as a related issue.

I think I will also update the screenshot in the issue summary, using the one from #35.

I made good progress today: MR 1296 creates a "Manage permissions" tab for every content type whose bundles are defined by (config?) entities. For example, Media:

I borrowed heavily from the field_ui module, which is responsible for most of the sibling tabs.

One technical problem is that different entity types have different base routes. For example, node types have /admin/structure/types/manage/{node_type} and Media types have /admin/structure/media/manage/{media_type}. A form builder for one or the other would have a parameter $node_type or $media_type with the appropriate type declaration, and Drupal would upcast the parameter. The name of the parameter matters, as does the type (NodeType or MediaType), so the generic form builder cannot do that.

Following the example of the field_ui module, I solved this problem using a route enhancer. Is there a simpler way, since I need this for just one form builder?

I thought of another todo: add a "Manage permissions" option to the drop button on the page that lists bundles, such as /admin/structure/types. I will add that to the issue summary, along with the last point from #35 and automated tests.

Some of the drop button options are defined in field_ui_entity_operation().

For the module-specific permissions form, we added a custom access check to avoid creating a page with no permissions. I could do the same thing here. Would that automatically hide the local tasks?

Fixed tests and added a Administer permissions operation to entity bundles.

Would that automatically hide the local tasks?

Yes. The IMCE module implements a custom access check for a route which has a local task.
The local task respects the custom access check.

I meant to upload this image in #39, but the bot commented on the MR and the form was outdated ...

I am updating the issue summary following #42.

It is nice to know that a custom access function would hide the local tasks (#43). But I have been thinking about it, and I think it is better not to create the local tasks, nor the routes, in the first place. Since we create them in PHP, not in static YAML files, we can do this. For comparison, the module-specific permissions page has a route defined in YAML, so we implemented a custom access function for that.

There is already duplicated code (at least the exit-early tests) in the form class, the route subscriber, and the route enhancer in the current MR. I think we can move that code, along with the code that generates the list of permissions, to a trait. Then use that trait in those three places.

I am adding another todo under "Remaining tasks".

Quick note: I did a little testing with the Umami demo profile.

I noticed that the drop button works as expected, but instead of "Manage permissions" it has "Administer permissions". For consistency with the tabs and with the other options in the drop button, it should be "Manage permissisons".

I also noticed that the taxonomy pages had List, Edit, Manage fields, Manage form display, and Manage display as tabs (local tasks) on one page, but Manage permissions and Translate taxonomy vocabulary display as tabs on a separate page. The problem is that there are two different base paths: /admin/structure/taxonomy/manage/{vocabulary}/overview and /admin/structure/taxonomy/manage/{vocabulary}. We will have to get this sorted out. I think we should also fix the content_translation module. (Both make it a tab on the right page and shorten the title to "Translate vocabulary" if we can.)

Edit: the content_translation module, not the taxonomy module, is responsible for the "Translate taxonomy vocabulary" tab.

Never mind what I said in #45. When deciding whether there are any permissions to manage, we have to look at the route parameter, so we cannot decide when creating the route whether to skip it. For example, a site administrator might enable Layout Builder for some bundle of some entity type, and that would add permissions for that bundle. If there are no other permissions for bundles of this entity type, then we want to create a permissions page for one bundle, not the others.

So yes, we need a custom access function.

One difficulty with the access function is that it does not get the parameters provided by the route enhancer. After all, we sometimes need to check access when on a different route. For example, a sibling local task needs to check access before adding a tab for the permissions form.

Once again, the field_ui module had an example I could follow, in one of its custom access handlers.

Since the custom access checker needs to figure out the bundle object, I decided to do the same for the form builder. That means we do not need a new route enhancer: we can use the one from the field_ui module. In case that module is not installed, I moved the route enhancer to the user module. It is odd that the user module now defines the service field_ui.route_enhancer. Perhaps we should define two services for this class, and deprecate the existing one.

I think the other changes I made are clear from their commit messages.

Still to do: fix the local tasks for editing a vocabulary. It took me a while to figure out why the field_ui module creates local tasks and then alters them. The point is that you need to create all the local tasks before you can get the other tasks you need from the plugin manager. I still have to do the same for the permissions form.

I added a new section to the User module help page. Here is one existing item, for context, and the new one:

Setting permissions

After creating roles, you can set permissions for each role on the Permissions page. Granting a permission allows users who have been assigned a particular role to perform an action on the site, such as viewing content, editing or creating a particular type of content, administering settings for a particular module, or using a particular function of the site (such as search).

Other permissions pages

The main Permissions page can be overwhelming, so each module that defines permissions has its own page for setting them. There are links to these pages on the Extend page. When editing a content type, vocabulary, etc., there is also a Manage permissions tab for permissions related to that configuration.

I looked through the help_topics pages, but did not see a good place to add something.

I also moved the route subscriber to the \Drupal\user\Routing namespace. I think it was in the EventSubscriber namespace because I created the first version using drush gen.

I also added a draft change record.

I think that leaves just some automated tests.

I am working on the tests, so I am assigning this issue to myself to avoid duplicated effort.

If I do not comment again within 3 days, you can assume that I lost track and re-assign this issue.

I am uploading the current version of the MR as a patch, for use with https://simplytest.me/.

I am sure that a good review will suggest ways to improve this code, but I have added some tests and I think this issue is ready for that review.

I am adding the "9.3.0 release highlights" as a suggestion to the release managers. If this issue does make it in to 9.3.0, then I think it is worth mentioning along with two other issues (already fixed) and a few smaller related issues that might also make it:

• (Fixed) #2571235: [regression] Roles should depend on objects that are building the granted permissions
• (Fixed) #3216341: Provide a module-specific permissions form
• (RTBC) #3236443: Link to module-specific permissions forms in Help pages
• (NR) #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions: this issue
• (NW) #2521758: Module anchors on Permissions pages do not get you where you would expect: front end, needs tests (Nightwatch?)
• (Active) #3236447: Update the title of the module-specific permissions pages: needs some design work

I am also updating the issue summary, since I decided to move the route enhancer to the system module, not the user module. And I am removing an extra space from the issue title.

I went through all the tests and I really like what I see. I had a few comment/questions but they are all resolved. So as far tests go, I think this issue is in great shape!

I'm going to set to "Needs work" while these comments get addressed:

https://git.drupalcode.org/project/drupal/-/merge_requests/1296#note_49474
https://git.drupalcode.org/project/drupal/-/merge_requests/1296#note_49493

I addressed the two comments on the MR. Summary:

1. @danflanagan8 is right: we should use field_ui_base_route if it is defined, whether or not the Field UI module is enabled. This makes the code a little simpler.
2. The performance of the access checker does not seem to be a real problem. I did add a shortcut for the Media, Node, and Taxonomy modules, since these always define per-bundle permissions. And I added an @todo comment for when #2809291: Add "edit block $type" permissions is fixed.

If we decide that performance is a problem, I suggest the following. First, pull these lines out of permissionsByProvidere() into a separate method:

    // Get the names of all config entities that depend on $this->bundle.
    $config_name = $this->bundle->getConfigDependencyName();
    $config_entities = $this->configManager
      ->getConfigEntitiesToChangeOnDependencyRemoval('config', [$config_name]);
    $config_names = array_map(
      function ($dependent_config) {
        return $dependent_config->getConfigDependencyName();
      }, $config_entities['delete'] ?? []
    );
    $config_names[] = $config_name;

Then, in the access checker, do something like this and cache the result:

    $permissions = $this->permissionHandler->getPermissions();
    $configs = [];
    foreach ($permissions as $permission) {
      foreach ($permission['dependencies']['config'] ?? [] as $config_name) {
        $configs[$config_name] = TRUE;
      }
    }

Finally, compare the values of $config_names to the keys of $configs.

It might be worth caching $config_names, whether or not we make the other changes to the access checker. If we save the result, then we avoid calling getConfigEntitiesToChangeOnDependencyRemoval() from the access checker and then again from the form builder.

I am crediting @danflanagan8 for his reviews on the MR.

@benjifisher, my comments on the MR are indeed resolved. Very nice!

I'm hoping to take another detailed look at the tests since a few things in there have changed (and I generally understand the issue more deeply).

I took a look at the changes to the tests and they all look really good.

This is very close, but I found a tiny bug that should be addressed: https://git.drupalcode.org/project/drupal/-/merge_requests/1296#note_49639

I updated the MR. Back to NR.

@benjifisher The access check is looking very robust now! This is looking great. I'm setting as Needs work though to get your thoughts on the following questions.

I wanted to check how this feature might work with non-core entities so I installed paragraphs and paragraphs_type_permissions. I created a Paragraph type called test for testing. Two things of note.

1. Neither the local tasks nor the operations for "Manage permissions" show up.

This is a bug in paragraphs, though. The issue is that the per-paragraph-type permissions defined in paragraphs_type_permissions do not declare any dependencies. The permissionsByProvider relies on dependencies to figure out which permissions relate the bundle at hand.

2. When I navigate directly to where I want the permissions form to be (/admin/structure/paragraphs_type/test/permissions), I get a 403 instead of a 404. On a "typo route" like /admin/structure/paragraphs_type/test_typo/permissions I correctly get a 404.

Any ideas what might be going on there?

Once this issue lands we should probably be good citizens and make an issue for Paragraphs to make it compatible.

Still need to do a more detailed review, but one thing that I picked up on, regarding #57:

@danflanagan8 is right: we should use field_ui_base_route if it is defined, whether or not the Field UI module is enabled. This makes the code a little simpler.

My only concern here is that it's theoretically possible for an entity type to have bundles but not have the Field UI annotation key; Even if using this key doesn't create a hard dependency on Field UI, I don't feel comfortable with this feature using a key where the intention is clearly meant to be used for only Field UI. What I would suggest is that we add a new key, that looks and behaves in the same way, but then open a follow-up issue to discuss if it's worth rationalising these into one single key that does not have an informational dependency on Field UI, something like bundle_settings_base_route, that way it could also be used for other future purposes.

Similar to that, I wonder if we want to make this work for entity types which don't define bundles, say config entities or content entities without bundles? Maybe that's something for a follow-up?

My only concern here is that it's theoretically possible for an entity type to have bundles but not have the Field UI annotation key

Not even theoretical. Taxonomy vocabulary does not declare a field_ui_base_route. Everything in this patch still works great for taxonomy vocabs.

Re #62:

Once this issue lands we should probably be good citizens and make an issue for Paragraphs to make it compatible.

We do not have to wait for this issue to land. Whichever module defines the permissions (probably paragraphs_type_permissions, judging by its name) should declare dependencies after #2571235: [regression] Roles should depend on objects that are building the granted permissions. See the change record Permissions can define dependencies.

I will look into the 403/404 issue tomorrow.

Re #63:

Personally, I think it is appropriate to use field_ui_base_route when it is defined.

If we add a new bundle_settings_base_route, then it will not be useful until other modules start declaring it. We could handle core modules as part of this issue, but what about contrib? Most or all of the time, the two base routes will be the same, so we are making modules provide the same information twice.

Whichever decision we make at this point, we might change our minds later and need to fix it. I think it is easier to start by using field_ui_base_route and add the new parameter later than the other way around.

I wonder if we want to make this work for entity types which don't define bundles, say config entities or content entities without bundles?

I am not sure what you mean. If there are no bundles, then there are no per-bundle permissions. For example, the user entity has no bundles.

There are permissions for each text format: see /admin/people/permissions/module/filter. There could be other examples, or a conrib module could add additional permissions for each text format. Even if we want to implement a permissions form for that use case, I think it is out of scope for this issue, since the implementation here relies heavily on the information we get from the Entity API.

This suggestion looks to me like a solution in search of a problem.

I guess the one thing we should consider at this point is whether the form class added in this issue (form builder and access checker) is generic enough that it can be used for other config items. As a test: can we add a static route in the filter module using this form class and create a form at /admin/config/content/formats/manage/basic_html/permissions to manage the single permission use text format basic_html?

I just updated the merge request, adding the same shortcut to the form builder that the access checker already has: pass the bundle object instead of the bundle name in the bundle parameter. We can now create a form at /admin/config/content/formats/manage/basic_html/permissions using the following addition to filter.routing.yml:

entity.filter_format.permission_form:
  path: '/admin/config/content/formats/manage/{bundle}/permissions'
  defaults:
    _title: 'Manage permissions'
    _form: 'Drupal\user\Form\UserPermissionsBundleForm'
    bundle_entity_type: filter_format
  requirements:
    _permission: 'administer permissions'
    _custom_access: '\Drupal\user\Form\UserPermissionsBundleForm::access'
  options:
    parameters:
      bundle:
        type: 'entity:filter_format'
        with_config_overrides: true

Since this route does not have the option _field_ui: true, the route enhancer is not called. (When the route enhancer is called, it forces $bundle to be a string.)

A typo generates a 404 response.

The one thing that seems odd is that the parameter name is bundle rather than filter_format. We are stuck with the parameter name bundle unless we want to change the route enhancer.

We would need some additional work if we wanted to make the new page a local task.

As I said in the previous comment, this example is just a test that the new form class supports other use cases. A form to manage a single permission is not very useful.

The custom access checker is not needed, but it still works.

If we add a new bundle_settings_base_route, then it will not be useful until other modules start declaring it. We could handle core modules as part of this issue, but what about contrib? Most or all of the time, the two base routes will be the same, so we are making modules provide the same information twice.

Whichever decision we make at this point, we might change our minds later and need to fix it. I think it is easier to start by using field_ui_base_route and add the new parameter later than the other way around.

If we assume that we might change field_ui_base_route into a more generic key later on, I assume would likely want to do that for D10 because it would be a BC-break.

I'll open a follow-up issue so we have a space to discuss the change, and on reflection, if we consider using field_ui_base_route more of a temporary and easy solution, then I'm more okay with us using it here. Using it here probably gives weight to introducing a more generic one later on.

I am not sure what you mean. If there are no bundles, then there are no per-bundle permissions. For example, the user entity has no bundles.

Yeah in that case it wouldn't be per-bundle permissions, it would just be permissions related to the entity type.

Edit: Actually, on second thought, this wouldn't work for content entities that don't define bundles, because in those cases the permissions would just depend on the module, there would be no config entity for them to depend on, so we wouldn't have any permissions to display. Did we ever get a solution to that problem?

Re @danflanagan8 in #62

I wanted to check how this feature might work with non-core entities so I installed paragraphs and paragraphs_type_permissions. I created a Paragraph type called test for testing. Two things of note.

...

I suspect Paragraphs will need to address that for 9.3 anyway and start declaring dependencies in their generated permissions on account of the changes to permissions in 9.3:

Permissions can define dependencies

In any case sounds like a issue should be filed against the relevant part of Paragraphs to implement the changes in the CR linked above.

I suspect Paragraphs will need to address that

Yeah, I'm no longer thinking that my questions in #62 are important to think about within the scope of this issue. It was important and interesting testing, but it looks like something that needs to be resolved from within the paragraphs module.

So consider my concerns from #62 resolved.

I went ahead and put in an issue for Paragraphs: #3244433: Paragraph Bundle Permissions should declare dependencies

Right, the 403 response is expected if someone navigates to /admin/structure/paragraphs_type/test/permissions when

• The test paragraph type has been created
• The Paragraphs module does not declare dependencies for the permissions it creates.

A 404 response would be better, but 403 is the best I know how to do. At least, the 403 response is enough to prevent the local task from being shown, so we are not generating any links to that page.

I am copying most of #54 into the issue summary.

I updated the MR in response to @Aaron McHale's comment. Back to NR.

Setting to RTBC.

The only unresolved thread contains a request for feedback from a core committer: https://git.drupalcode.org/project/drupal/-/merge_requests/1296#note_50099

Hiding patches because there's an MR

I'm part way through a review of this, will continue tomorrow, moving back to needs review in the meantime, as I've some questions in the partial review that I think need resolving before this is committed

Great work here, left a few comments/questions on the MR

Thanks for the review! I am assigning this issue to myself while I work through the suggestions.

Should I leave the status at RTBC, or did you mean to set it to NW?

Yeah, that was a crosspost, thanks

@larowlan:

I responded to the review on the MR. I made several of the requested changes, and those threads can be resolved if you approve. There are still a few open questions.

I fixed the tests that failed the last time I pushed my local branch. Back to NR.

I really like the use of permission_granularity from the entity annotation. I'm kicking myself for not discovering that earlier. That will put to rest any of my outstanding performance concerns for contrib entities.

I also like the new defensive programming for maintaining BC. And it even has tests!

I've gone through the new threads in the MR and I like what I see. I have the latest version of the branch locally and all my spot checking looks good.

I'm going to set this back to RTBC so that @larowlan knows that someone else has looked at the latest changes. I know there's a test that's still running, but @benjifisher assures us the broken test has been fixed and passes locally.

Didn't review the hole MR yet, one thing to be aware of in regards to permission_granularity. Despite the generic name, that property is currently actually solely used for *translation* permissions. It doesn't necessarily mean an entity type has regular per-bundle permissions nor that it does _not_ have them. It is possible that you would have per-bundle permissions but don't need per-bundle translation permissions and possible although very unlikely to have per-bundle translation permissions but not regular ones. So we might get some false positives/negatives with that. Paragraphs would be one example, because we have no interest in exposing any translation permissions, as that is commonly inherited from the parent.

( sorry for double post, d.o was timing out..)

That's interesting, @Berdir. The MR uses the permission_granularity property as a shortcut for access checking, but it doesn't require that the property be set in order for the Manage Permissions tab to work.

For example, see this related issue for Paragraphs: #3244433: Paragraph Bundle Permissions should declare dependencies.

I guess based on your comment, I will not update my work on that issue to alter the Paragraphs annotation!

Yeah, I saw that, it's just an optimization then for types are _known_ to have permissions, correct? The only problem would be a module that really doesn't have bundle permissions but has that flag set. We can probably live with some edge cases that would show it and then not actually have any permissions there. Worse things have happened :)

@Berdir, thanks for taking a look at this issue!

I had some reservations about using permission_granularity. Search this page for "legalistic", and you should find the comment copied from the MR.

As @danflanagan8 said, the code does not require that property. It is just used to bypass a more expensive check in the access checker:

1. If the granularity is set to "bundle", then grant access.
2. If not, then find all permissions that depend on the bundle. If there are none, then 403.

There is a separate permission requirement, of course.

The only bad result from relying on permission granularity is that we might create a local task with a permissions form that has no permissions.

I see I am too slow: x-post.

Ah thanks, I didn't realise that permission_granularity was solely for content translation.

Does that mean we should add our own annotation here solely for this functionality?

It is just an optimization in the end. If we're worried about performance that, we could just add a cache for it? Question is how often we'd have a cache hit I suppose with such a rarely used backed operation? Was the performance overhead actually profiled? There is an old issue bout caching permissions, so having that would be one more reason to do it I suppose..

I'm about to go through all of threads/discussion that I've missed from the past few days, but one point on permission_granularity:

This is similar to my reasoning around renaming/replacing field_ui_base_route, I think it makes sense to try and share these annotation keys where we can, because all of these individual keys and features, they don't act alone, they are all interconnected pieces that come together in particular ways to form Entity Types; So with that in mind, when we have opportunities to share annotation keys, where it make sense to do so, I think it's highly worth exploring those options.

Okay, back to read...

Overall this is looking really good, opened a couple of minor nit threads, and commented on a few more, looks like most other threads can be resolved.

@Berdir, regarding #87, I voiced my concerns about performance in a thread in Gitlab: https://git.drupalcode.org/project/drupal/-/merge_requests/1296#note_49493

My concern was about the content type page where there may be many node types that are each doing an access check to see whether the Manage Permissions operation should be available. I have some numbers in that thread for Docker on Mac.

That page was annoyingly slow when I had xdebug running, but it turned out it wasn't bad without xdebug on. That's when @benjifisher had the idea to add a quick check of the entity type (since changed to checking the permission_granularity). Once that was done, the content types page was lightning fast with or without xdebug on.

But a different entity type that doesn't declare permission_granularity could still have some slowness on the bundle listing page under certain circumstances.

Re #87, #90:

Another thing @danflanagan8 mentioned on the MR is that managing the list of permissions is less of a problem than computing the configuration dependencies.

I am nervous about a persistent cache, but it would be easy to statically cache the (boolean) result per bundle. That would help if one page had several links to the same Permissions form and needed to check access for them.

Re #81-#87:

As explained in #81, Paragraphs does not declare "permission_granularity = bundle" because it does not want permissions from the content_translation module.

Webform submissions do have "permission_granularity = bundle", and the bundle type is webform. We might cause a performance problem if we checked access for every webform on the admin page listing them all. In fact, we do check access, but it is quick because of the granularity setting. The result is what we said in #84, #85: we get an empty permission form.

Bug or feature? The empty Permissions form for a webform is not a local task on the page with the edit form. There is a link to the empty form in the drop button for the form on /admin/structure/webform.

This example supports the suggestion that we only create the route when the entity type defines field_ui_base_route.

Re #63, #65, #67, #88:

I think we agree that "Manage permissions" should be a tab on the same as "Manage fields". Since we do not want entity definitions to say the same thing twice, we should use field_ui_base_route when it is defined. We have #3245020: Rename the field_ui_base_route entity property to discuss changing the name of that parameter.

Sorry, I did not mean to set the issue to RTBC.

@AaronMcHale:

Thanks for the review.

I prefer not to make the changes you suggested. See the explanation on the MR or magically copied above.

I did make some other changes:

• Use the bundle name, not the entity name, in the route and local task. For example, the route entity.node.permission_form is now entity.node_type.permission_form.
• Only define the route (and the local task) when field_ui_base_route is defined. Use that as the base route and base path.

Because of the second change, we will no longer create useless tabs (local tasks) on Webform pages. Those pages already have a lot of tabs, so an extra one can be quite annoying.

Back to NR.

@benjifisher

Yep in the interest of consistency, I think I agree with what you've said.

Use the bundle name, not the entity name, in the route and local task. For example, the route entity.node.permission_form is now entity.node_type.permission_form.

I gave this a bit of thought, I looked at the RouteSubscriber in Field UI, and I agree with this change, because:

1. The permissions form is about the permissions that depend on that bundle (config entity);
2. Whereas the Field UI, yes is separated by bundle, but what you're doing in the Field UI is directly modifying the field schema for the content entity;
3. Therefor, from an information architecture perspective, I would logically also put this route under the config entity.

Opened #3245487: Change Vocabulary overview-form and edit-form paths to be more consistent with other core entity types and improve the information architecture as a follow-up to #mr1296-note51248.

Alright, after chatting briefly in Slack, benji and I are now very confident in this issue and it looks like all threads have been addressed, so RTBC we go 🎉

Just two questions on the MR from me, one is a follow-up, but the other I'm not sure about the required change - I can't see any reference to it in the existing resolved threads or here.

1. Follow-up: do we want a separate issue or should we add it to #3245020: Rename the field_ui_base_route entity property? I added an item to the "Remaining tasks" in the issue summary to do one or the other.
2. Question on the tests: I answered on the MR.

I am happy to mark threads on the MR as resolved, but as a matter of process I think the MR author should not do that. What if I think a thread is resolved but the reviewer is not satisfied with my response?

Follow-up: do we want a separate issue or should we add it to #3245020: Rename the field_ui_base_route entity property? I added an item to the "Remaining tasks" in the issue summary to do one or the other.

Separate issue in my book

Issue credits

Thanks all, this is a killer feature for 9.3 in my book

Nice, 9.3 is going to be an awesome release 🎉

This is causing core tests to fail see https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/44883/

We need to revert unfortunately. I tried to get the tests to fail locally but couldn't.

I have no idea why the committed MR passed on 8.0 and not 7.4 or 8.1 - very odd.

I can reproduce the error. It's to do with APCu. If it is enabled for cli it will fail. If I set apc.enable_cli=1 this test will fail regardless of PHP version. So we don't have apcu on on PHP 8.0 - probably because it wasn't ready for PHP 8 when we added the containers. I'll alert @mixologic.

If we want to keep the checking of the role we can. We just need to avoid $this->container. Here's the smallest fix possible. Personally I prefer #106 because we should minimise the amount of direct API access tests have but I can see why people might argue otherwise.

@alexpott:

Thanks for investigating the test failures. I feel bad about breaking HEAD, but on the bright side we (thanks to you) learned that we need to update the testbot.

I do not feel strongly about which fix to use for the tests. The test I added borrows code from testUserPermissionChanges() earlier in the same file. I borrowed that code for all the usual reasons: laziness, ignorance, and because it is good to be consistent, especially within a single file. That last point suggests adopting the minimal fix (#109).

@andypost:

Thanks for the link to #2066993: Use \Drupal consistently in tests.

We've fixed the test fail and my changes are nought to do with the changes made by the patch so I'm going to set to RTBC and commit #106 because I prefer that one.

Committed 91fdb8a and pushed to 9.3.x. Thanks!

I added #3246064: Rename the _field_ui route option, taking care of the last "Remaining task".

I also updated the change record Add "Manage permissions" tab after "Manage display". I mentioned the addition to the Operations drop buttons, and I added a section for module developers.

I added a comment to #2809291: Add "edit block $type" permissions and updated the issue summary.

Confirmed via Slack the status change was unintentional.

I've reverted this from 9.3.x due to #3252750: The new permission page breaks the Group module.

We need to figure out what to do here exactly, so putting back to 'needs work' against 9.3.x so that contributors to this issue can see it.

A conflict like this seems always possible when core adds something.

We could define the route possibly only if no existing route on that path exists, but that's rather inefficient to check, as they are keyed by route name, so we'd have to loop over all and check the path.

Alternatively, we could deprecate using this path now and add the feature in 10.0 Or we make it opt-in and possibly even with a configurable path, like with any other entity related route. That also solves several problems that we've been discussing like efficiently deciding if this route should be added.

To fight the ever-expanding list of default properties, we could have a @DefaultContentEntityType annotation or something like it, that generates all the things you have to define based on only the absolutely required properties. We can then update that in every major update to include new features. Or even include multiple versions of those classes.

I really like the thought of default annotation bits. Especially if you could still override them on the actual entity type annotation. But yeah, as I've commented on the other issue, an opt-in would have been far less disruptive. And I agree that being able to configure the path like with other entity routes seems like the most flexible solution.

@DefaultContentEntityType is out of scope for this issue.

From #119:

Or we make it opt-in and possibly even with a configurable path, like with any other entity related route.

That seems manageable. Until today, I never looked at link templates, but they seem pretty simple after all. (Maybe I am missing something.) I think I can come up with a patch before I get some sleep.

One question is whether the link template should be added to the content entity type (e.g., Node or Group) or the bundle type (e.g., NodeType or GroupType). Since the path is something like /admin/structure/types/manage/{node_type}/permissions, my first thought is to use the bundle type. But the route subscriber also needs field_ui_base_route which is declared on the content entity type. So I think I will add the annotation there.

We need to avoid conflicts in route names as well as paths. The opt-in method should fix that. If a module wants to enable the new Permissions form, it can specify the path with a link template; if there is a conflict with an existing route name, then the module can define its own route as described in the change record.

Looking at the Group module, I see that it defines the route entity.group_type.permissions_form. That is not the same as the route defined in this issue, entity.group_type.permission_form, but it is too close for comfort. That is, the two are close enough that it would not be surprising if some other module defined a conflicting route.

I added the issue tag "9.3.0 release highlights" as a suggestion to the release managers. Since this issue was not mentioned in the release notes for 9.3.0-alpha1, I think that means the release managers already decided not to include it. That tag was replaced with "9.4.0 release highlights" in #118, but I think we should just remove the suggestion at this point.

I am adding the issue tag "Needs change record updates" and mentioning that in the remaining tasks.

I think this MR should do the job. I ran all the tests affected by the MR, and they all pass. Let's give the testbot a chance to run all the other tests as well.

I am also uploading a patch that should apply to the 9.4.x branch, where the feature has not been reverted.

I do not see a test running for the MR, so I am uploading a patch for 9.3.x.

1. +++ b/core/modules/user/src/Routing/RouteSubscriber.php
   @@ -0,0 +1,82 @@
   +  protected function alterRoutes(RouteCollection $collection) {
   +    foreach ($this->entityTypeManager->getDefinitions() as $entity_type_id => $entity_type) {
   +      if (!$link_template = $entity_type->getLinkTemplate('permission-form')) {
   +        continue;
   +      }
   +
   +      if (!$route_name = $entity_type->get('field_ui_base_route')) {
   +        continue;
   +      }
   

   hm. all other link-template based routes are defined in DefaultHtmlRouteProvider and explicitly registered on the entity type.

   this one depends on user.module stuff (technically, many things in entity system too, but we're trying to get away from that I guess, not add more).

   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? We now have an opt-in mechanism for this. If you define this and _don't_ have bundle permission then that's nobodys fault but your own? One example of that would be contact message, you add the link template there, but contact module (in core) does not have any dynamic/bundle permission. I would suggest to remove that definition (and double check the others) and then just assume that there will be permissions. And then you can IMHO simplify both this (a simple permission check maybe?) and also drop the has-bundle-permissions logic from the access check itself.

From Group's GroupType entity annotation:

*   links = {
*     "add-form" = "/admin/group/types/add",
*     "collection" = "/admin/group/types",
*     "content-plugins" = "/admin/group/types/manage/{group_type}/content",
*     "delete-form" = "/admin/group/types/manage/{group_type}/delete",
*     "edit-form" = "/admin/group/types/manage/{group_type}",
*     "permissions-form" = "/admin/group/types/manage/{group_type}/permissions"
*   },

So that would be similarly "too close for comfort" as described in #121. However, if we use a route subscriber as @Berdir pointed out, then it's not as bad as Group could just not implement said subscriber. That still leaves the underlying issue that, no matter what we do, we run the risk of introducing a new key that contrib is already using.

In the early days of Drupal 8 I noticed some link templates being prefixed with drupal- to form link relations. i don't know if that is still the case, but perhaps it's a discussion we can start elsewhere: When core introduces new link templates or paths on entities' UI pages, should we namespace it with "drupal" or "core" to avoid any collisions in contrib?

Another alternative I see is to use link templates, but also an annotation key that tells the route provider which link template to use. Then with berdir's suggestion of defaults, we could have it point to permission-form by default, but allow extending annotation's to have it point to something else instead.

So something like this by default:

*   links = {
*     "permissions-form" = "/admin/path/to/bundle/{bundle_entity}/permissions"
*   },

But like this when the template key collides:

*   permission_form_template = "core-permissions-form",
*   links = {
*     "core-permissions-form" = "/admin/path/to/bundle/{bundle_entity}/permissions"
*     "permissions-form" = "/admin/group/types/manage/{group_type}/permissions"
*   },

If there's a fear of this becoming too cluttered over time (as was the case with labels), we can put them inside an array called "third_party_templates" or something and require anyone who adds to it to prefix their key with the module name:

*   third_party_templates = {
*     "drupal-permissions-form" = "bundle-permissions-form"
*   },
*   links = {
*     "bundle-permissions-form" = "/admin/path/to/bundle/{bundle_entity}/permissions"
*     "permissions-form" = "/admin/group/types/manage/{group_type}/permissions"
*   },

But that's a lot of extra steps on top of perhaps simply prefixing all templates with either drupal- or the providing module's name.

I don't see the benefit of an extra key that defines the link template name.

IMHO, core is allowed to add more link templates, and it's up to contrib to prefix them if defining additional ones, not the other way round.

And no, doing it through a route provider won't change anything in regards to the link template name, that just changes a single place that uses it, we still need to have a fixed link template name in several other places to check if a given entity type uses that feature. (local tasks, operations, ..)

I'm also fine to prefix my template names with group- starting from 2.0.x, but isn't this something we ideally communicate clearly (if we haven't already)? I can't recall ever seeing a big fat warning saying: "Careful how you name your link templates, core might also claim the name at any point".

126.1: I did say that I might be missing something. Where are link templates documented? I looked at https://www.drupal.org/docs/8/api/entity-api/structure-of-an-entity-anno... and some of its sibling pages, but I did not see much. The API docs for EntityTypeInterface::getLinkTemplates() tell me that I can use the entity type and the bundle entity type in the template. I did not read it yesterday, but the main Entity API has some information.

all other link-template based routes are defined in DefaultHtmlRouteProvider and explicitly registered on the entity type.

I am not sure what you mean by "explicitly registered". Is that something other than adding them to the links array?

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.

By "another entity type annotation", do you mean we have to introduce another key? Or do you just mean that we have to add something like this?

     *   handlers = {
     * ...
     *     "route_provider" = {
     *       "html" = "Drupal\user\Entity\UserRouteProvider",
     *     },
     * ...
     *   },

126.2: If this still has a chance to get into 9.3.0, then I prefer to keep the changes to a minimum. Even if some checks (from the patch that was reverted) are redundant, it seems safer to leave them in.

126.3: Same point, but 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.

As for the contact module, the same point about minimizing changes applies. The patch that was reverted in 9.3.x was not reverted (yet) in 9.4.x: it applies to the contact module, so minimizing changes means that we should opt in there. If this issue is changed to target 9.4.x, then we can reconsider.

127: Good point about the link template key being "too close for comfort". I am happy to go with @Berdir’s reply in #128.

I think the testbot is telling me the same thing as 126.1. I may not have time to implement a fix today. I am setting the status back to NW to fix the failing tests (and address 126.1).

Ah, I see, this also covers permissions added by other modules. Makes sense, that does complicate the performance aspects of this.

Yes, by explicitly registered, I mean it's an explicitly set handler that is responsible for doing it and can be subclassed to customize things, then the default implementation doesn't have to handle every possible edge case. It's never as easy of course, there is for example a common subclass of AdminHtmlRouteProvider to create routes with the admin theme flag set, but a separate one would need to do that dynamically.

I'll leave it to @catch and others to decide if that's still feasible for the 9.3, I was already operating under the assumption that it isn't which, on the plus side, would give us time to rethink some of the implications of switching to a link template more thoroughly.

This issue was reverted in the 9.3.x branch but not the 9.4.x branch. After checking with @alexpott on Slack, I am changing this issue to target 9.4.x and marking it Fixed again. I will open a new issue, targeting 9.4.x, to implement the opt-in strategy discussed in #119 and following comments.

There are two change records associated with this issue. Someone already updated one (marking it as a draft and targeting Drupal 9.4.0). It will need further updates if the follow-up issue is fixed. I updated the other change record to target 9.4.0, but I left it published.

I am removing the "Needs CR update" tag and adding it to #3253955: Let modules opt in to the bundle-specific permissions form.

I've updated the release note section to be for 9.4.0.

Based on the change records I think this isn't disruptive; it's a pure feature and API addition with a clean deprecation. (Unless the site owner must update the permissions or access will be changed, but I don't think that is the case?) Or if there was a disruption, #3253955: Let modules opt in to the bundle-specific permissions form fixed it. Definitely worth adding to the highlights though.

Unless the site owner must update the permissions or access will be changed, but I don't think that is the case?

Correct: site owners do not have to update permissions.

This issue just adds a new form with a subset of the permissions from /admin/people/permissions. Site owners can manage those permissions from either page.

How to get this Manage permissions tab automatically for custom entities?

Hi @kopeboy

The change record has information for those developing entity types, please refer to the "Module developers" section at https://www.drupal.org/node/3242827

You can also find the change record linked at the top of the issue in the sidebar.

Thanks