Create a a way to filter/modify a permissions list prior to display in the UI.

I have a different idea and approach for this. The problem to being solved is that developers want to restrict permissions that are displayed on the permission form. One reason is to prevent their site administrators – who can probably administer users and roles – from granting escalated permissons.

Instead of expanding the API of PermissionHandler, I think we should keep this logic inside of PermissionForm. The PermissionForm can dispatch an event (or alter hook) to allow other modules to implement logic which reduces the permissions displayed. This could be similar to #763074: Add hook_permission_alter() so that permission descriptions and other properties can be altered → and allow modifying the descriptions shown or flagging permissions as elevated security.

My thoughts: The permissionsByProvider method in PermissionForm would be modified to look like the following

  protected function permissionsByProvider(): array {
    $permissions = $this->permissionHandler->getPermissions();
    $event = new PermissionsListFilterEvent($permissions);
    $this->eventDispatcher->dispatch($event);
    $permissions = $event->getPermissions();

    // Move the access content permission to the Node module if it is installed.

The suggestion from @mglaman is good, this needs to be customizable, but expanding the API of PermissionHanlder seemed like a bit much when I implemented it. With the event, the implementation is a more direct application of what is actually needed. There is a new MR with this approach, with test coverage.

I love it 🙌. Left comments on the phpDoc comments.

Can it be highlighted which MR should be reviewed? The link in #5 "new MR" throws a 404.

But looks like mglaman reviewed 4401 which has CC failures.

bnjmnm → closed merge request !4383

Think next step would be for a CR.

May be a non issue but I applied the MR and enabled user_permissions_test module and when I go to the permissions page
I see Test permission repeated 3 times.

@smustgrave that is due to user_permissions_test.permissions.yml

c:
  title: 'Test permission'
a:
  title: 'Test permission'
b:
  title: 'Test permission'

It doesn't have a/b/c in the title.

CR added

Just looked into the MR and I like the approach.

While it doesn't cover all the requirements we (@kingdutch and myself) had discussed in Vienna, I think it is a really helpful building block, on top of which we can then build the other things like modifying the permission form depending upon the role(s) the user has who's editing that form. So, I'd RTBC this but leave it at NR for others to have a deeper look.

Think this is almost there!

Only moving to NW for CC failure.

#13

other things like modifying the permission form depending upon the role(s) the user has who's editing that form

I created a contrib module → that will allows changing permissions depending on roles. I'm sure some fine tuning is needed, but it definitely works. The module depends on the event created in this issue, so once this lands it can get whatever refinements are needed to have it be useful for a site.

I left an improvement inspired by FeyP's review in Slack (https://drupal.slack.com/archives/C1BMUQ9U6/p1690214902005739?thread_ts=...)

Can we make sure they're credited on this issue?

For the one open thread. I always appreciate a good code sample in the comments :)

Thread addressed.

This is looking very good already!

I think the new event is a great idea. I also like how we now limit the API to its intended purpose by explicitly only allowing to filter permissions, which also makes for much nicer DX. Thanks for implementing this change!

I made another quick code review and left some comments on the MR. Nothing major, just one more question where you probably had your reasons and I just don't see why, and a few nits I noticed on the way. For now I'm leaving this at "Needs review" so that we can maybe get some more eyes at this.

Also made some minor changes to the IS.

I'm putting to NW. We need EventDispatcherInterface $event_dispatcher = NULL in the constructor.

Feedback addressed + deprecation tests added.

Thanks. I now tested this manually. The contrib module doesn't seem to work with the current state of the MR (which is of course absolutely fine), so I wrote my own subscriber and in general it works great. Testing some edge cases, when I filtered away all permissions except one permission provided by user module I got this:

TypeError: array_keys(): Argument #1 ($array) must be of type array, null given in array_keys() (line 124 of core/modules/user/src/Form/UserPermissionsForm.php). when I filtered away all permissions provided by the node module.

I'm not sure how common it is that you'd get this in practice, but I think we should fix this. I don't think we need to add tests for that case, but I won't object, if you think it would be a good idea.

I made some slight adjustments to the existing CR to update it to the current state of the MR. Since both forms are marked as internal, I think we don't need to write a separate CR for the constructor changes, so I think we're good there.

W/r/t the test coverage we have that for the user permissions form and we have a deprecation test, which is great. I wonder if we should add the same test coverage we have for the user permissions form also for the other permission forms that extend this form, i.e. UserPermissionsRoleSpecificForm, UserPermissionsModuleSpecificForm and EntityPermissionsForm? What do you think? If you don't think it is required, I might RTBC without this, so feel free to convince me that it isn't needed :).

Setting to needs work to fix that edge case I mentioned above and potentially the additional test coverage. Then I think we should be good to go, so hopefully the final iteration from my end.

Good feedback on #23. I expanded test coverage to the additional forms and fixed the empty node permissions bug. Test coverage was not expanded to that based on the overhead needed - an entire additional module with another subscriber would be needed - unless there's a technique I'm unaware of. Will understand if others feel this is fully necessary, though.

Thanks for working on this, this is really almost there now :)

The new test coverage for the other permission forms looks good.

Unfortunately, I think we didn't get the fix for the "access content" permission logic quite right yet. I've made another inline comment about that. Let me know, if I missed something. So setting back to needs work for that.

And I forgot to answer your question w/r/t to the additional subscriber needed to expand test coverage to cover the edge cases (all node permissions filtered, system access content permission filtered): One thing that's often done in cases like this is to use the state service. You could get the permission names to (not) filter from state in the test module, then set the state with the permissions to (not) filter before getting the form. See it in action in the test coverage here for example, although in that case it's used the other way around. That way you only need one test module. As I said, I will not block on this, because I think hitting this edge case is fairly uncommon in practice, so a potential regression there not being caught by a test wouldn't be too bad, but maybe the info is still useful in case a core committer will.

Ah! Thank you for reminding me about the state service approach. For whatever reason using it in a subscriber didn't cross my mind 🤷‍♂️. That can facilitate all sorts of edge cases that might come up, then.

Good spot on that edge case. MR updated with a solution and test coverage expanded to include it.

Thanks, great work here! This access content logic is haunting us a bit... I've added a few questions and one suggestion inline.

Thanks once more for working on this! All feedback in the MR has been addressed and I can't find anything new :). I did another manual test on 11.x and everything works great.

The IS looks good and reflects the current state of the issue. The CR still reflects the current state of the MR. As I said previously, since both affected forms are marked as internal, I think we don't need a separate CR for the constructor changes, so I think we're good there.

We've got test coverage for all user permission forms in core, we got test coverage for the deprecation message in the constructors, we got test coverage for the system access content logic in UserPermissionsForm including some edge cases that might happen during filtering. Tests pass locally and on CI and removing some code in a few selected places, the tests fail as intended.

I think this is RTBC! 🎉

Didn't look deeper into the code but I was wondering if this is something that couldn't be done at all right now for example with form alters? Or is this just a DX improvement? It sounds like there are couple people who are interested in having this API and it isn't clear why (at least to me), so I think it would be helpful to have the justification documented in the issue summary.

Tagging for framework manager review as its adding a new API so that they can decide if this is something core would like to support.

It is a DX improvement when building applications on Drupal. The form alter is unbearable due to the complex structure of the form.

Tagging with DX in that case 😊

Updated IS to better describe DX benefits.

Thanks @bnjmnm! That definitely makes it much easier to understand why this is being proposed. 💯 I think it's still good idea to get a framework manager review on this since it's their responsibility to weigh the additional maintenance burden to the benefits that we get from this. 😊

I'm triaging RTBC issues → . I read the IS and skimmed through the comments. I didn't spot any unanswered question or anything else to do. Leaving at RTBC.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.