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 |
| |||
#20 | interdiff.txt | 394 bytes | benjy |
#20 | 2693405-20.patch | 36.93 KB | benjy |
| |||
#18 | interdiff.txt | 814 bytes | benjy |
#18 | 2693405-16.patch | 36.94 KB | benjy |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #3
benjy CreditAttribution: benjy at PreviousNext commentedI 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 commentedTODO
Comment #5
benjy CreditAttribution: benjy at PreviousNext commentedMight add the new tests in #2693399: Add new tests for RoleDelegationSettingsForm
Comment #6
benjy CreditAttribution: benjy at PreviousNext commentedRefactored 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 commentedRemoved 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 commentedOK, 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 commentedRemoved un-needed setUp(), ready for a review here.
Comment #10
benjy CreditAttribution: benjy at PreviousNext commentedJust 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 commentedLooking good. RTBC after comments read.
Incomplete docs.
Left over? Just set the title.
This too?
Could value ever not be an int and cause issues here?
Why not save the account outside the loop?
Could return early by checking this.
Can almost use Role::loadMultiple() here. It's not giving you that much.
Comment #12
benjy CreditAttribution: benjy at PreviousNext commentedNo, 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 commentedFixed the patch file.
Comment #15
benjy CreditAttribution: benjy at PreviousNext commentedSeparated out the permission generation. This is ready for committing now.
Comment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #18
benjy CreditAttribution: benjy at PreviousNext commentedMissing groups on the kernel tests.
Comment #20
benjy CreditAttribution: benjy at PreviousNext commentedFixed tests.
Comment #21
benjy CreditAttribution: benjy at PreviousNext commentedKernel tests in right folder.
Comment #22
benjy CreditAttribution: benjy at PreviousNext commentedBack to rtbc, will commit tonight and update project page.
Comment #23
benjy CreditAttribution: benjy at PreviousNext commentedCommitted.