2840664 - Create extra user management permissions

[ribel]

Original PR: #427
Related issue: https://www.drupal.org/node/2840664

Summary

This PR changes some conditions for accessing 'admin/people' page by adding new permissions: 'view users', 'block users'. A user that has such permissions could view 'admin/people' page, use filters and use action 'Block the selected user(s)'.

Changes:

1. Created custom permissions 'view users', 'block users'
2. Added a new views access plugin to the 'admin/people' page, added conditions for checking by custom permissions.
3. Override a block user action with the custom plugin and added here conditions for checking by custom permissions.
4. Added update hook to set up new permissions for 'contentmanager' role.
5. Altered 'admin/people' form to show actions based on permissions.
6. Added Behat test 'content-manage-view-block-people-page' for testing.

HTT

There are Behat test 'content-manage-view-block-people-page.feature' for testing.

[ribel]

@A-Kom, please also add the same permissions for sitemanager role.

[akhomy]

@ribel, this is fixed now.

[bramtenhove]

@ribel, you added the duplicate label to this pull request. Should we close it? What is the status?

[Kingdutch]

We should just grant the new permissions here to the role that should have them (preferably based on the permission it's splitting off from if possible). Calling this function in an update hook might reset permissions that a site-administrator or developer has changed.

e.g. if a site using Open Social has revoked the "select account cancellation method" (which should probably not be given to all users anyway?) then this update hook will grant that permission again.

[akhomy]

Hi, @Kingdutch, yup we could update method for setting permissions. If you want - you could fix it by yourself, otherwise, I could fix it later.

[bramtenhove]

Just a friendly reminder that this still needs a little bit of work ;)

[Kingdutch]

Removing the Duplicate tag as the duplicate of this (#427) has been closed unmerged.

[jochemvn]

Had a small discussion with @jaapjan about granting these permissions to the CM. We agreed we shouldn't just give these permissions to the CM.

Also the remarks from @Kingdutch are correct. So as far as I can see we need to change the following:

1. Remove the 'view users' and 'block users' permission from the CM and only grant them to the SM
2. On install granting all permissions is fine, but in the update hook only grant the 2 selected permissions to the SM
3. Change the supplied behat test

If there's the need in some project to grant these permissions to the CM, please do that in these projects, but not in the distro.

[bramtenhove]

@ribel @andrii-khomych-lemberg-co-uk could one of you make the changes to the pull request as suggested in @jochemvn's comment?

[jochemvn]

Works as described. Can be merged when the tests are 'green'

[Kingdutch]

I think the revised update hook does what it's supposed to. However, I think in the future we should try less to write 'clean code' vs more stable code. What I mean with this:

Prefer:

function update_hook_N() {
   $new_permissions = ['some_permission'];
   drupal_grant_permissions('some_role', $new_permissions);
}

Over:

function update_hook_N() {
  $new_permissions = some_permission_function('some_role');
  drupal_grant_permissions('some_role', $new_permissions);
}

The motivation for this is that the contents of some_permission_function will change in the project lifetime to reflect the permissions granted to roles at installation time. This means that if these permissions change and you want to know what update_hook_N did in a few months time, it will no longer be accurate (which means that updating from 1.0 to 1.1 can be different than from 1.0 to 1.5). This is dangerous.

We should strive for update hooks to be deterministic.

It leads to slightly more lines of code but the upgrade path will be easier to reason about, now and in the future.