Provide a module-specific permissions form

The role-specific form uses the path /admin/people/permissions/{user_role}. The proposed path /admin/people/permissions/module/{modules} does not conflict with this.

It is not hard to create the new form. The MR for this issue already does that.

Implementation note: the $form array from UserPermissionsForm::buildForm() uses both module names and permission names as top-level keys. This means there is no easy way to generate that form and then restrict to the specified list of modules. We have to add a few lines of code to the parent form class.

When specifying more than one module, the form sections will be in the same order in the new form as in the original form. The order that modules are listed in the path makes no difference.

Because of the proposed path, the main Permissions form is available as a breadcrumb. If you have a role named Module (more precisely, if the machine name is "module"), then there will also be a breadcrumb (with link text "Edit role") to the Permissions form for that role.

I added these two lines to the route requirements:

    modules: '[a-z][a-z_,]*'
    _custom_access: '\Drupal\user\Form\UserPermissionsModuleSpecificForm::access'

The first line is a sloppy way to say "one or more valid module names, separated by commas". I could make a more complicated regular expression that comes closer, but I do not think it is worth it. More important: what is a valid module name? The best reference I could find was Naming and placing your Drupal module, which tells me that the name can up to 50 lower-case letters and underscores, starting with a letter. There are some other restrictions.

The second line defines a custom access function, which checks whether there are any permissions defined by any of the modules on the list. I am not sure that access is the right mechanism to use, but the nice thing is that it will avoid producing links to pages that have no permissions on them. Basically, this access function takes care of the checks that are currently made in Drupal\system\Form\ModulesListForm::buildRow().

Do I have to worry about caching the access result, or will that cache be cleared whenever a module is (un)installed?

@Berdir, @AaronMcHale, thanks for the early feedback!

I'd just check if the module is installed, if not, access denied in your custom access check.

If the module is not installed, then $this->permissionHandler->moduleProvidesPermissions($module) returns FALSE, so access denied (in the current MR). What if a module is installed but does not provide any permissions? Currently the access function returns AccessResult::forbidden(). I think this is a good thing. If I change that, then we will generate links to pages like /admin/people/permissions/module/ckeditor, which does not seem useful. Besides, doing it this way saves me the trouble of deciding what to do in the case: "Sorry, the ... module does not provide any permissions."

I had a look at the two modules you suggested. The fpa module is pretty complicated, modifying the existing Permissions form. The pfm module is similar to the approach I am taking. It creates a single new page instead of one per module. I think a feature of my approach is that each page has an access checker, as above. It is nice that the pfm lets you filter on module or role or both. It is not nice that it duplicates most of the logic of the core Permissions form. Since I am doing it as a core patch instead of a contrib module, I can take a simpler approach, adding a few lines to the existing form class.

... does that mean it would be the responsibility of the Permissions links on the module page to appropriately construct the URL with all relevant dependencies?

I will read that question as applying to any situation where we create a link to the module-specific permissions page.

After reading the rest of #7, I think the answer is yes. (My bias is to give the same answer, since that keeps this patch simpler.) Considering the node, user, and field_ui modules shows that automatically including modules required by the requested modules is not the right answer.

My next step will be to update the links on /admin/modules to go to the new pages. Each module will have a link to the page for just that module. I suggest that doing anything more complicated is out of scope for this issue.

I also see problems with including other modules. For example, the CKEditor module does not provide any permissions, so there will be no link (same as current behavior). But it depends on the Text Editor, Filter, User, System, File, and Field modules. I think it is confusing for CKEditor to provide a link to a permissions page that has permissions for all of those modules but does not even mention CKEditor.

In the System module, both ModulesListForm and ModulesListConfirmForm generate a confirmation message when enabling modules. Instead of adding to the duplicated code, I am adding a trait and generating the confirmation message there.

Is it OK to just use $this->currentUser and $this->formatPlural() in the trait? Or should I declare properties $currentUser and $stringTranslation in the trait? Or should I pass those to the helper function?

The main form, but not the confirmation form, already injects the access_manager service. I assume this is more performant than $url = Url::fromRoute(...) and then checking $url->access(...). Is it worth injecting the service into the confirmation form so that I can use it in the new trait?

There are a lot of links in implementations of hook_help() and in the help_topics module that go to the main Permissions page with a specific fragment (anchor). I am willing to update all of those as part of this issue, or do so in a follow-up issue.

I think this issue is ready for review. I have done everything I suggested in the issue description, except that I only covered the most common use case (new content type) of configuration that adds new permissions.

@Berdir, if you feel strongly about either of the suggestions you made in #5, and you are not convinced by #8, then I will take your advice.

@Berdir, thanks for the review! I implemented two of your requested changes and replied to the other two.

I am adding the issue tag for follow-up issues, and I will add details in the issue summary.

I added some test coverage.

There is duplicated code in the Permissions form and the confirmation form. This issue adds a trait to hold some of that code. I am adding a note to the issue summary to move more of the duplicated code, in a follow-up issue.

To be clear, the link after installing a module (or more than one) is an important part of this issue. The standard advice after enabling a module is to configure permissions for it. Currently, we direct site administrators to the overwhelming /admin/people/permissions for this task. I want to make it easy to follow the standard advice. My approach to this issue is influenced by this use case:

• Create a page with permissions for one or more selected modules.
• Make it simple for the calling function. For example, the calling function does not have to check which of the modules provide permissions.

@dww, thanks for the review!

I notice when the issue is updated. I guess I will have to watch the MR, too. If this issue had been marked NW, then it would be back to NR now.

I am adding details to the "Proposed resolution" section of the issue description.

I am adding screenshots to the issue description.

Follow-up to part of #9:

I added use StringTranslationTrait to make sure that $this->formatPlural() is available. It should not be a problem that FormBase already does that.

I added abstract protected function currentUser();. That function is defined in FormBase, but my new trait does not "know" that. Since it uses curretnUser(), it should declare it.

@mannojithape:

Thanks for testing, and for the detailed description of your testing steps. I am glad to see that you tested submitting the form. That is very important! I added some steps to UserPermissionsTest to check the same thing.

Based on #20, I would say that this issue is tested, but not yet reviewed. It needs both before we can call it RTBC.

I think any issue that makes a change to the UI is supposed to have a change record, so I am adding an issue tag for that.

I am uploading the current diff from the MR as a patch. This way, people can apply the patch for testing purposes without being affected by possible further changes to the MR.

@longwave, thanks for the review!

1. I added the test you suggested.
2. I explained why I prefer to break the long line.
3. I explained why it is simpler to filter the list of modules before building the form and suggested an alternative.

I implemented the alternative suggested in (3) locally, but have not pushed it to the remote. It passes the two tests touched by this issue. If you like the idea, then I can push my local changes.

I just updated the MR with the permissionsByProvider() method so you can see more specifically what it does. (I made some changes to the commit since my previous comment, so I am glad I waited.)

I do not think it is a good idea to alter the result of getPermissions() in order to change permissions on this page. As you say, that might have other side effects. I prefer a more specific alter hook. Also, there does not seem to be much momentum behind #763074: Add hook_permission_alter() so that permission descriptions and other properties can be altered.

In the snippet that appears above, foreach ($permissions_by_provider as $provider => $permissions) is now foreach ($this->permissionsByProvider() as $provider => $permissions). Unless we want to rewrite buildForm(), we really need the permissions grouped by module (provider). Maybe I do not understand you suggestion, but so far I do not like the idea of changing the name to groupPermissions().

Can you explain a little more your idea for passing in the permission list as an argument? I do not see how that would help. If the goal is to move that code to the Node module, then we have to provide an alter hook somewhere. Where would you put it? (To be clear: that change would not be part of this issue.)

I do consider that simplification to be out of scope for this issue. But I am willing to make changes to support simplification in a future issue. For example, we should decide now whether to pass in the permissions list as an argument.

We can also make the same simplification inside permissionsByProvider():

  protected function permissionsByProvider() {
    $permissions = $this->permissionHandler->getPermissions();
    $permissions_by_provider = [];

    // Move the access content permission to the Node module if it is installed.
    // @todo Add an alter so that this section can be moved to the Node module.
    if ($this->moduleHandler->moduleExists('node')) {
      $permissions['access content']['provider'] = 'node';
    }

    foreach ($permissions as $permission_name => $permission) {
      $permissions_by_provider[$permission['provider']][$permission_name] = $permission;
    }

    return $permissions_by_provider;
  }

That moves the permission from System to Node, but puts it in alphabetical order instead of before 'view own unpublished content'. I do not think we want to change that, certainly not as part of this issue.

Whether or not we simplify the Node-specific code, I think it should be done inside the permissionsByProvider() method. If we pass the permissions list as an argument, then the alteration has to be done in buildForm(). This issue moves that code out of buildForm(), and a future issue can provide an alter hook. Either way, I would like to keep that complexity out of the form method.

I added the draft change record Permissions can be viewed and edited for one module or a list of modules. I also added a Release notes snippet to the issue summary.

I am happy to have that discussion. I think the last few commits improved the implementation.

I am uploading the changes from the current version of the MR as a patch in order to make it easier to test.

I should use git diff 9.3.x... when generating a patch, not git diff 9.3.x... I should also look at the patch before uploading it.

From #26:

Changing the provider of a permission may have other side effects though!

Maybe I am beating a dead horse here (*) but now that #2571235: [regression] Roles should depend on objects that are building the granted permissions has been fixed, I can think of an important side effect. Saving any role with the "access content" permission will recalculate dependencies, making the role depend on the Node module. If the Node module is then uninstalled ... what happens? I think the permission is removed, and the role is saved. That would be bad: all of a sudden, some roles lose the "access content" permission.

(*) "Beating a dead horse" is an idiom for continuing to fight after you have already won.

One suggestion that was made on Slack is to change the title of the new pages. Currently, the tile is "Module Permissions", the same as the main permissions page (/admin/people/permissions).

The question is what to use for the title. If there is just one module, then something like "Node Permissions" would work. If there are a few modules, maybe it should be "Permissions for Content Moderation, Node, and Workflows Modules". If there are many modules, then this option leads to a long title.

An alternative is to leave the title as is, but provide a description at the top of the form, listing the modules on the page. But does this add information to the page or is it just noise?

If we can agree on a good suggestion, then I am happy to implement it as part of this issue. We could also add a follow-up issue to continue the discussion.

I see that #3192585: Fix up topics to use new help_topic_link function is fixed (and included in Drupal 9.2.3). That will affect one of the proposed follow-up issues, so I am adding a link in the issue summary.

@larowlan:

Thanks for the review!

There's a number of unresolved MR threads too, would be good to mark the resolved ones as such

I prefer to let the one who opened the thread (the reviewer) mark the thread as resolved, but I will resolve them myself if that is OK.

I will try to update the MR after work today. But I am going on vacation tomorrow, so I might not get to it for another week.

I resolved all of the old threads on the MR, since @larowlan already had a chance to review them. I made changes for the new threads and left them open.

Back to NR.

Here is a copy of the current diff as a patch.

@larowlan:

I missed that change, but I like it. From Coding standards > Parameter and return typehinting:

Parameter and return typehints should be included for all new functions and methods, including new child implementations of methods for existing classes and interfaces.

Does that apply to test methods? I added a new test in two different classes, but you did not mention them.

I updated the MR, adding return types to the four functions you mentioned in #44 and also

+++ b/core/modules/system/src/Form/ModulesEnabledTrait.php
@@ -33,7 +34,7 @@ abstract protected function currentUser();
...
-  protected function modulesEnabledConfirmationMessage(array $modules) {
+  protected function modulesEnabledConfirmationMessage(array $modules): PluralTranslatableMarkup {

By the way, the Drupal docs refer to "typehints", but https://www.php.net/manual/en/language.types.declarations.php in the PHP docs calls them "Type declarations". I think there used to be a note on that page about previously calling them "hints", but I searched the page just now, and "hint" appears only in a user comment.

Since you left the issue status at RTBC, I cannot use the dispensation you granted me. Can I save that for next time? ;)

I added follow-up issues:

• #3236442: After adding configuration that has related permissions, provide a link to the module-specific Permissions form
• #3236443: Link to module-specific permissions forms in Help pages
• #3236446: Avoid duplicated code for the Modules form and the Modules Confirmation form
• #3236447: Update the title of the module-specific permissions pages

The first three were mentioned as remaining tasks in the issue summary. (I am removing those tasks.) The last was suggested in #36, #37.

Oops, I made a mistake in the @return comment:

+++ b/core/modules/user/src/Form/UserPermissionsForm.php
@@ -84,7 +84,7 @@ protected function getRoles() {
   /**
    * Group permissions by the modules that provide them.
    *
-   * @return string[][][]
+   * @return string[][]
    *   A nested array. The outer keys are modules that provide permissions. The
    *   inner arrays are permission names keyed by their machine names.