Change module architecture to grant extra access instead of removing existing access [#2693405]

Problem/Motivation

When a user has 'administer permissions' they have a number of interface they can use to assign roles to users. The module finds these user interfaces and makes them respect the new permissions the module introduces. This means a lot of extra code and the possibility in the future that new user interfaces and mechanisms for assigning roles are missed and thus breaks the expectations the module lays out.

Proposed resolution

Instead of locking down users with 'administer permissions', make this permission override all existing role_delegation permissions. If you have 'administer permissions', role_delegation is irrelevant.

When you are assigned a role_delegation permission and NOT the 'administer permissions' permission, then you get access to an extra user interface for assigning roles. This means there is only a SINGLE place that access checking is required.

Remaining tasks

Patch.

User interface changes

Removal of all code that interacts with core forms.

API changes

Updated expectations of what role_delegation permissions do.

Data model changes

None.

Comment	File	Size	Author
#21	2693405-21.patch	37.05 KB	benjy
#21	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 7 pass
#20	interdiff.txt	394 bytes	benjy
#20	2693405-20.patch	36.93 KB	benjy
#20	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
#18	interdiff.txt	814 bytes	benjy
#18	2693405-16.patch	36.94 KB	benjy
#18	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 1 pass, 3 fail
#15	2693405-15.patch	36.88 KB	benjy
#15	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 1 pass, 3 fail
#13	2693405-13.patch	35.9 KB	benjy
#13	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
#12	interdiff.txt	11.6 KB	benjy
#12	2693405-12.patch	30.05 KB	benjy
#12	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 1 pass, 2 fail
#10	2693405-10.patch	35.8 KB	benjy
#10	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
#9	interdiff.txt	990 bytes	benjy
#9	2693405-9.patch	35.52 KB	benjy
#9	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass PHP 7 & SQLite 3.14, D8.2 3 pass
#8	interdiff.txt	20.87 KB	benjy
#8	2693405-8.patch	36.08 KB	benjy
#8	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
#7	interdiff.txt	14.51 KB	benjy
#7	2693405-7.patch	28.08 KB	benjy
#7	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
#6	interdiff.txt	6.74 KB	benjy
#6	2693405-6.patch	22.65 KB	benjy
#6	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
#3	2693405-3.patch	13.94 KB	benjy
#3	8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

24 March 2016 at 07:40

benjy created an issue. See original summary.

Comment #2

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented 1 April 2016 at 09:28

Title:	Clean up role_delegation_form_alter()	» Change module architecture to grant extra access instead of removing existing access
Issue summary:	View changes

Comment #3

benjy CreditAttribution: benjy at PreviousNext commented 1 April 2016 at 09:36

Status:

Active

» Needs review

File	Size
2693405-3.patch	13.94 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass

I agree, its not for this module to try and lock things down. I already found an issue that would have been a security vulnerability if it wasn't that administer users was a restricted permission.

Heres the user/%/edit roles widget removed and any kind of VBO filtering.

Comment #4

benjy CreditAttribution: benjy at PreviousNext commented 1 April 2016 at 11:05

TODO

Refactor the class that adds the form elements and move all that logic into the form with clean-ups.
Add new tests to ensure the access control around the assignable roles works as expected
Update the project page.
Create an issue against core to fix the issue with access to VBO roles when you have "administer users"

Comment #5

benjy CreditAttribution: benjy at PreviousNext commented 1 April 2016 at 11:09

Might add the new tests in #2693399: Add new tests for RoleDelegationSettingsForm

Comment #6

benjy CreditAttribution: benjy at PreviousNext commented 1 April 2016 at 23:54

File	Size
2693405-6.patch	22.65 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
interdiff.txt	6.74 KB

Refactored the form and turned the helper into a real service. Not sure what to call the service, its simply called RoleDelegation at the moment, maybe RoleManager or RoleDelegationManager?

Comment #7

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 02:06

File	Size
2693405-7.patch	28.08 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
interdiff.txt	14.51 KB

Removed more classes, consolidated the permission callback into the same service and then adding some more kernel tests which now have 100% coverage for for the RoleDelegation service and the AccessCheck.

I just want to do some more clean-up here, rename a couple of things and then its just the web tests that need fleshing out again.

Comment #8

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 04:59

File	Size
2693405-8.patch	36.08 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
interdiff.txt	20.87 KB

OK, added some new web tests as well. I converted the role delete test into a kernel test because it didn't need WTB and I removed the role rename test since it wasn't testing any code of ours, it was only the role label been renamed anyway.

Comment #9

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 05:01

File	Size
2693405-9.patch	35.52 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass PHP 7 & SQLite 3.14, D8.2 3 pass
interdiff.txt	990 bytes

Removed un-needed setUp(), ready for a review here.

Comment #10

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 06:15

File	Size
2693405-10.patch	35.8 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass

Just removed the README which was empty anyway. Also looks like rewriting the tests fixed the tests for PHP7 and SQLite which is a nice bonus.

Comment #11

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented 2 April 2016 at 06:19

Looking good. RTBC after comments read.

+++ b/src/Form/RoleDelegationSettingsForm.php
@@ -17,17 +19,57 @@ use Drupal\user\UserInterface;
+   * @param \Drupal\Core\Session\AccountInterface $current_user
+   *   The current user viewing the form.

Incomplete docs.

+++ b/src/Form/RoleDelegationSettingsForm.php
@@ -17,17 +19,57 @@ use Drupal\user\UserInterface;
+      '#title' => isset($form['account']['roles']['#title']) ? $form['account']['roles']['#title'] : t('Roles'),

Left over? Just set the title.

+++ b/src/Form/RoleDelegationSettingsForm.php
@@ -17,17 +19,57 @@ use Drupal\user\UserInterface;
+      '#description' => isset($form['account']['roles']['#description']) ? $form['account']['roles']['#description'] : t('Change roles assigned to user.'),

This too?

+++ b/src/Form/RoleDelegationSettingsForm.php
@@ -42,22 +84,13 @@ class RoleDelegationSettingsForm extends FormBase {
+      $value === 0 ? $account->removeRole($rid) : $account->addRole($rid);

Could value ever not be an int and cause issues here?

+++ b/src/Form/RoleDelegationSettingsForm.php
@@ -42,22 +84,13 @@ class RoleDelegationSettingsForm extends FormBase {
+      $account->save();

Why not save the account outside the loop?

+++ b/src/RoleDelegation.php
@@ -0,0 +1,53 @@
+      if ($account->hasPermission(sprintf('assign %s role', $role->id())) || $account->hasPermission('assign all roles')) {

Could return early by checking this.

+++ b/src/RoleDelegation.php
@@ -0,0 +1,53 @@
+    $all_roles = user_roles(TRUE);

Can almost use Role::loadMultiple() here. It's not giving you that much.

Comment #12

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 06:57

File	Size
2693405-12.patch	30.05 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 1 pass, 2 fail
interdiff.txt	11.6 KB

Could value ever not be an int and cause issues here?

No, I think Form API handles that.

All the rest of the feedback and the RoleDelegation service renamed to DelegatableRoles as discussed.

Comment #13

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 06:59

File	Size
2693405-13.patch	35.9 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass

Fixed the patch file.

Comment #14

2 April 2016 at 07:07

The last submitted patch, 12: 2693405-12.patch, failed testing.

Comment #15

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 07:16

File	Size
2693405-15.patch	36.88 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 1 pass, 3 fail

Separated out the permission generation. This is ready for committing now.

Comment #16

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented 2 April 2016 at 07:17

Status:

Needs review

» Reviewed & tested by the community

Comment #17

2 April 2016 at 07:18

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 15: 2693405-15.patch, failed testing.

Comment #18

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 07:19

Status:

Needs work

» Reviewed & tested by the community

File	Size
2693405-16.patch	36.94 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 1 pass, 3 fail
interdiff.txt	814 bytes

Missing groups on the kernel tests.

Comment #19

2 April 2016 at 07:21

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 18: 2693405-16.patch, failed testing.

Comment #20

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 07:29

Status:

Needs work

» Needs review

File	Size
2693405-20.patch	36.93 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 3 pass
interdiff.txt	394 bytes

Fixed tests.

Comment #21

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 07:31

File	Size
2693405-21.patch	37.05 KB
8.x-1.x: PHP 5.5 & MySQL 5.5, D8.2 7 pass

Kernel tests in right folder.

Comment #22

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 07:34

Status:

Needs review

» Reviewed & tested by the community

Back to rtbc, will commit tonight and update project page.

Comment #23

benjy CreditAttribution: benjy at PreviousNext commented 2 April 2016 at 11:22

Status:

Reviewed & tested by the community

» Fixed

Committed.

Comment #24

2 April 2016 at 11:22

benjy committed a925b94 on 8.x-1.x

Issue #2693405 by benjy, Sam152: Change module architecture to grant...

Comment #25

16 April 2016 at 11:24

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Change module architecture to grant extra access instead of removing existing access

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

TODO

Related issues

Referenced by