Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Posted by xjm
Problem/Motivation
(From #787152: Dynamic permissions cannot be automatically assigned to or removed from roles).
The admin role that comes configured in the standard profile has the following unexpected behaviors:
- Permissions of certain (core) modules are not granted to the admin role when these modules are installed.
- Permissions for new configuration items are not granted to the admin role.
- Permissions introduced in a module update are not granted to the admin role.
- Permissions of existing configuration items are not removed (from any role) when the configuration items are removed.
This issue is about point #1 only. Steps to reproduce:
- Install Drupal using the Standard profile.
- Visit
admin/people/permissions
and observe that the administrator role has all permissions are checked for all modules. - Enable Book.
- Visit
admin/people/permissions
again and observe that its permissions are not checked.
Proposed resolution
See attached patch. (@todo--document it here)
Remaining tasks
- This issue is currently blocked by #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations.
- The patch needs test coverage.
- The patch needs code review.
- The hooks added by the patch need documentation.
Related issues
- #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations (blocking this isssue)
- #787152: Dynamic permissions cannot be automatically assigned to or removed from roles (covering points #2 and #3 above)
- #1153072: New content types (and other dynamic) permissions are not auto-selected for admin role—always give it all perms (alternate feature proposed for D8)
- #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced
User interface changes
- New permissions for a module will now be checked at
admin/people/permissions
when the module is first installed.
API changes
The following hooks will be added:
hook_modules_to_be_installed()
hook_modules_to_be_enabled()
hook_user_permissions_insert()
hook_user_permissions_delete()
Comment | File | Size | Author |
---|---|---|---|
#27 | permissions-1454686-27.patch | 7.51 KB | tim.plunkett |
#24 | drupal-1454686-24.patch | 6.96 KB | tim.plunkett |
#24 | interdiff.txt | 2.61 KB | tim.plunkett |
#23 | interdiff.txt | 8.15 KB | irunflower |
#22 | 1454686-write_test_admin_user_get_permissions-22.patch | 6.97 KB | irunflower |
Comments
Comment #1
no_commit_credit CreditAttribution: no_commit_credit commentedPatch by David_Rothstein from #787152: Dynamic permissions cannot be automatically assigned to or removed from roles. The two test failures in this patch are caused by #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations.
Comment #1.0
no_commit_credit CreditAttribution: no_commit_credit commentedupdated summary
Comment #2
xjmTagging.
Comment #3
xjmAnd, I'll write a functional test for this.
Comment #4
sunUgh. The new hook names sound pretty wonky to me.
And, the use-case implementation in User module is even more wonky:
This won't work within module updates, since it's entirely based on hooks.
In fact, this will also lead to unexpected results when modules are disabled (after being installed), and one of the newly installed modules defines the same permission as one of the disabled modules.
Comment #5
catch#607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced too.
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedAgreed. We have so many hooks that run when modules are enabled that all the good names were taken :)
Solving issues with module updates isn't the goal of that code. Or really the goal of this issue anymore, given the way the issues were split...
Although I think the code in the patch actually does help solve it, since that's what functions like user_permissions_insert() and user_permissions_delete() could be used for. As I said in the other issue, I'm not sure we can automate that case further because it doesn't seem like there's any general way to distinguish between a module which added a new permission (which should always be given to the admin role) vs. a module which renamed an existing permission (which should only be given to the admin role, and other roles as well, if they had the old permission).
I'm not sure I follow that. Is there a specific example of what would happen? As far as I can see, this won't cause any problems that don't already exist as a result of #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedMore accurate title (actually borrowing something closer to the one #787152: Dynamic permissions cannot be automatically assigned to or removed from roles used to have originally).... The module's permissions themselves are always granted to the admin role already. The issue here only occurs for dynamic permissions defined by another module (where the list of permissions changes as a result of the new module is being installed).
If I remember right, the main example that actually has a noticeable effect is that turning on the PHP module does not grant permission to use the new "PHP code" text format to the administrator (since that permission is provided by Filter module). So if you turn on the PHP module as an admin and expect to be able to immediately create a node and use the PHP text format just like user 1 can, you can't because you were never granted permission to use it.
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedLooks like this is in the wrong component too... finger slip somewhere? :)
Comment #8.0
David_Rothstein CreditAttribution: David_Rothstein commentedUpdated issue summary.
Comment #9
xjmMore that I often forget to fill out the component because there's no
- None -
option, and I think either my browser or dreditor just uses the component from the last issue I filed, or something. :) Working on the test now.Comment #10
xjmSo... suddenly, somehow, I can't reproduce this locally in either 8.x or 7.x. Bit baffled. It was reproducible on a different machine yesterday. (In fact, there's also already test coverage for this exact behavior in UserPermissionsTestCase::testAdministratorRole().
Going to try to confirm on the other machine on Monday (and check if that test fails there). I haven't been able to figure out what the missing link is.
Edit: I also noticed another, unrelated "unexpected behavior" for the administrative role functionality in the process of testing--when you make a new role the administrative role, that role also doesn't get any of the existing permissions.
Comment #11
lokapujyaI am new to contributing to issue queue. I was attempting to help out with this issue: #1153072: Admin role should always provide all permissions (create a superuser role) (alternate feature proposed for D8) However, I am not sure what the intended behavior should be.
I just noticed this and it might help to be aware of this setting when thinking about this problem:
In Home » Administration » Configuration » People >> Account Settings
exists a section called ADMINISTRATOR ROLE where you can select: disabled, Administrator, or one of the custom roles.
The description says, "This role will be automatically assigned new permissions whenever a module is enabled. Changing this setting will not affect existing permissions."
Comment #12
bartlantz CreditAttribution: bartlantz commentedHere's a test for this condition pre-patch and also a patch with the test and the above fix. joestewart, barnettech and tim.plunkett also worked on this at the drupalcon core code sprint.
Comment #13
xjmAwesome!! Thank you sprinters! The test logic looks good to me. It looks like the bot was stuck, so I requeued both patches on QA.
Meanwhile, a few outstanding things with the patch:
We still need to add the hook documentation and address these @todo.
This is a little confusing; perhaps:
Removes user permissions that are no longer available.
What are the full implications of this?
These should be capitalized and begin with a period. Reference: http://drupal.org/node/1354#inline
We can leave the t() off the assertion message here. Reference: http://drupal.org/simpletest-tutorial-drupal7#t
Comment #14
irunflower CreditAttribution: irunflower commentedI'll be working on cleaning this up!
Comment #15
irunflower CreditAttribution: irunflower commentedI cleaned this help, but it still needs work in two places.
In comment #13,
#1
Didn't quite know what to do - someone make a decision! :)
#3
I'm not sure what the full implications are - can someone answer?
Everything else *should* be fixed (or I attempted to). Please look and edit as needed.
Comment #16
irunflower CreditAttribution: irunflower commentedBlah, forgot to set status.
Comment #18
irunflower CreditAttribution: irunflower commentedRerolled, but:
patch: **** malformed patch at line 34: /**
Can someone take a look? I can't seem to figure what the issue is...
Comment #20
irunflower CreditAttribution: irunflower commentedReroll from 12 - let's try this again..
Comment #22
irunflower CreditAttribution: irunflower commentedAgain...
Questions in #15 still apply. These particular changes were not made.
Comment #23
irunflower CreditAttribution: irunflower commentedInterdiff (maybe)?
Comment #24
tim.plunkettSo the patch in #22 added + signs before a bunch of lines.
So I rerolled it, and here's an interdiff against #12.
Comment #25
andyceo CreditAttribution: andyceo commented#24: drupal-1454686-24.patch queued for re-testing.
Comment #27
tim.plunkettI'm just rerolling this to get it to apply, but it doesn't seem to make sense anymore, since between the patch in #1 and the next patch in #12, the new hook_modules_to_be_installed() invocation was removed...
Is this still a major bug?
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think so, but happy to be overruled... Lowering priority for now. In practice, I've rarely ever seen a situation where this mattered myself; usually it's just a missing checkbox on the permissions page but one that doesn't matter anyway because it's covered by some other permission that you already have.
The problem I've actually seen more often myself is #3 from the issue summary ("Permissions introduced in a module update are not granted to the admin role") but that's covered by a different issue, not this one.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commentedOh, I lied, #3 actually is covered (at least partially) by the current patch, since it provides a user_permissions_insert() function that can be used for this purpose.
Comment #30.0
David_Rothstein CreditAttribution: David_Rothstein commentedUpdated issue summary.
Comment #30.1
xjmRemoving myself from the author field so that I can unfollow the issue. --xjm
Comment #31
claudiu.cristeaThis seems fixed by #2435075: Implement admin role as a flag on the role storage, simplify permissions page, remove user_modules_installed. Please reopen it if you think I'm wrong.