Problem/Motivation
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 points #2 and #3 only.
Proposed resolution
TBD
Remaining tasks
TBD
Related issues
- #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations (blocking this isssue)
- #1454686: When a new module is installed, the admin role doesn't get all new permissions (covering point #1 above and blocking this issue)
- #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). By changing the behavior of the admin role so that this role is forced to always have all permissions (and can never have those permissions unassigned), this would automatically solve points #1, #2, and #3. However, this approach is not likely to be backportable to Drupal 7 (since it is a behavior change, and also because it requires large changes to the user interface to prevent people from unassigning permissions from the admin role). Therefore, it is being dealt with in a separate issue where its merits as a Drupal 8 feature request can be discussed.
Original report by tsvenson
When enabling optional core modules, the administrator role is not getting all the new permissions ticked for them. I tried the old 6.x trick by saving the account settings, but it didn't help in this case.
This is what I did:
1 - Installed DC on my local Wamp (Win XP).
2 - Added a user and assigned administrator role to it.
3 - Logged in as the new user.
4 - Enabled every module except Aggregator, Book, Content translation, OpenID, PHP Filter, Syslog and Testing.
When I looked in the permissions list some permissions has been set for the administrator role, but any related to Create, Edit own/any and Delete own/any for Blog, Poll, Forum topic and Edit/Delete terms for Forums was not set.
I could manually set them without complains though.
Comment | File | Size | Author |
---|---|---|---|
#96 | drupal.user-dynamic-perms.96-FULL.patch | 16.72 KB | David_Rothstein |
#96 | drupal.user-dynamic-perms.96-PARTIAL.patch | 11.66 KB | David_Rothstein |
#94 | drupal.user-dynamic-perms.94-FULL.patch | 16.71 KB | David_Rothstein |
#94 | drupal.user-dynamic-perms.94-PARTIAL.patch | 11.65 KB | David_Rothstein |
#46 | drupal.user-dynamic-perms.46.patch | 2.24 KB | sun |
Comments
Comment #1
dmitrig01 CreditAttribution: dmitrig01 commentedComment #2
dmitrig01 CreditAttribution: dmitrig01 commentedThe tests don't pass but otherwise this patch does the trick. I did a bunch of debugging and can't figure it out.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedArguably, node_type_save() and node_type_delete() should clear the static node_type_build cache before calling their hooks.
Comment #4
dmitrig01 CreditAttribution: dmitrig01 commentedYup, that would do the trick. I wouldn't argue about that, it seems logical.
I'd RTBC the patch but since I wrote part of it I'm not qualified. But #3 has my green light.
Comment #5
Dave ReidNeeds the debug() line out of the patch. :)
Comment #6
dmitrig01 CreditAttribution: dmitrig01 commentedComment #7
jzacsh CreditAttribution: jzacsh commentedTested and it worked wonderfully :).
Comment #8
Dries CreditAttribution: Dries commentedDoesn't this belong in node module? It seems unrealistic for user.module to know about all things that could change the permissions.
Comment #9
webchickAgreed.
Comment #10
Dave ReidI thought it makes more sense to have this in user.module. If we're trying to keep node.module decoupled from everything else, it makes sense this way. Otherwise if this moves to node.module we'll have to add some function_exists() or module_exists() checks.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedUser is core required, Node is slowly moving to core optional. Consequently, it makes more sense to have those in node.
Comment #12
dmitrig01 CreditAttribution: dmitrig01 commentedIf, however, user becomes optional at some point, it would make it sense to have it in user. There is no harm of having an unimplemented hook, but otherwise you need to have function_ or module_ exists (like Dave said).
Comment #13
webchickYou can't very well hack user.module to add additional hooks to cover saving Views, or Flags, or...
Those modules are consumers of User module's API. So is Node module. node.module is the right place for these calls to user_administrator_reset().
If user module ever becomes optional, I'm sure a good many other changes will be required, and where this function call lives will be the least of our worries. :P We can cross that bridge if we get to it.
Comment #14
dmitrig01 CreditAttribution: dmitrig01 commentedNode it is.
Comment #16
dmitrig01 CreditAttribution: dmitrig01 commentedoops
Comment #18
tsvenson CreditAttribution: tsvenson commentedJust installed alpha5 and it is still not working correct. Since the last patch failed testing I assume it never made it to the release...
I added the Blog, Contact, Locale, Statistics and Tracker modules. All permissions, except the blog related in Node, was set correctly.
Comment #19
tsvenson CreditAttribution: tsvenson commentedTested on Alpha 6 and it still does not add permissions for the Node Create/Edit/Delete Blog entry. All other permissions are being set, including contributed modules, but not for the blog module.
Comment #20
webchicktsvenson: Do you think you could try your hand at re-rolling and applying this patch to see if it fixes the problem?
http://drupal.org/patch has some good tips. Feel free to drop by #drupal or #drupal-contribute too if you need help.
Comment #21
tsvenson CreditAttribution: tsvenson commentedHi webchick,
Sure, no problems. On my way out right now though, but it will be the first thing I do when I get back.
Comment #22
webchickAwesome! :) thanks!
Comment #23
tsvenson CreditAttribution: tsvenson commentedOki, patch applied and here are unfortunately it doesn't make any difference. Blog is still not getting permissions for the administrator role.
Comment #24
int CreditAttribution: int commentedComment #25
webchickIt would be awesome for someone to get around to debugging and re-rolling this. This isn't bad enough to be a release-blocker, but it certainly is a very obvious, user-facing bug.
Over at #725058: Built-in administrator role doesn't catch dynamically created permissions (now marked duplicate), Freso points out that this bug may manifest itself in other contexts as well.
Comment #26
dhthwy CreditAttribution: dhthwy commentedThe test was failing because node_node_type_insert and node_node_type_update hooks were not being invoked (so the permissions were never updated). For example when you enable the book module it instead tries to invoke book_node_type_insert, book_node_type_update. This means for the current patch to work any module that creates their own content types will have to implement both hooks (which means a much bigger patch). Book module implements node_type_update only, blog module doesn't implement either one, and how many other modules are going to need them?
Rather than doing that, I think it would be better (compared to the alternative) to simply put the call to user_administrator_reset() in node_type_save() in node.module.
In this patch I also changed user_administrator_reset() to user_administrator_grant_permissions() to better reflect what the function is doing.
Of course anyone can feel free to revert that if they don't like the new name :)
Comment #27
sunIf there's any actual reasoning for those cache resets, then we need better comments to clarify the situation for the next developer that touches those lines. Just looking at this code (and not reading the issue), I don't get the difference and why we are moving/adding these lines here.
The function name is slightly backwards... "administrator" refers to a single user, while this function grants permission to a role.
Additionally, the function belongs to the other User Role API functions.
I'd suggest to rename it to user_admin_role_grant_permissions(), or similar, but I think that name cuts it pretty well.
Lastly, the function should be defined below the existing user_role_revoke_permissions(), not arbitrarily appended to the end of user.module, please.
Powered by Dreditor.
Comment #28
tsvenson CreditAttribution: tsvenson commentedThis bug is still existing in Beta 2.
So far I have identified the following core permissions that is not automatically set when optional core modules are enabled:
Filter
- Use the PHP code text format
Node
- Create new Blog entry content
- Edit own Blog entry content
- Edit any Blog entry content
- Delete own Blog entry content
- Delete any Blog entry content
- Create new Poll content
- Edit own Poll content
- Edit any Poll content
- Delete own Poll content
- Delete any Poll content
- Create new Forum topic content
- Edit own Forum topic content
- Edit any Forum topic content
- Delete own Forum topic content
- Delete any Forum topic content
Taxonomy
- Edit terms in Forums
- Delete terms from Forums
Comment #29
webchickThis is actually a pretty bad issue because it's functionality that's broken for all users of the D7 default install profile. It also smells like something we might need to make an API/behaviour change to fix. I'm bumping to critical. Sorry guys. :\
Comment #30
int CreditAttribution: int commented@tsvenson if you change to beta2 tag, it don't show in the Critical count list. Go get again in the dev version.
Comment #31
carlos8f CreditAttribution: carlos8f commentedLooking at this patch (and #28) makes me a little worried about the current implementation of admin role. The concept seems pretty simple -- when the return of hook_permission() changes, update the admin role. Problem is, catching that change at the right time, I think. I'm not really sure what the best way to go about that is, other than the proposed solution (requiring modules to call user_admin_role_grant_permissions() when changing their permission list). That certainly puts the burden on module developers for the admin role to work correctly.
Comment #32
carlos8f CreditAttribution: carlos8f commentedCan we do something like,
in user_access(), or am I missing something big?
Comment #33
carlos8f CreditAttribution: carlos8f commentedI guess that doesn't work because of this (description for the admin role option):
Comment #34
carlos8f CreditAttribution: carlos8f commentedThis is a flimsy way to go about things. The description of the admin role reads,
However, only hook_modules_installed() is implemented. There are apparently no changes caught when modules are enabled, specifically (or on update.php). Only modules that implement hook_install() are passed to hook_modules_installed(), (Edit: I have since been corrected on this) which has nothing to do with whether a module implements hook_permissions() or not.
Also, from standard_install():
Apparently, the only reason why the admin role works at all, is that the standard profile sets it up by default.
Comment #35
David_Rothstein CreditAttribution: David_Rothstein commentedI think that's actually a case of the description in the UI being incorrect.
I remember that @catch's intention with this feature was only to have it occur when installing a new module. See his rationale: http://drupal.org/node/480660#comment-1664986
So to fix that part, we should probably just change or clarify the above sentence.
I don't think so...? I'm pretty sure any uninstalled module gets passed in there when module_enable() is called on it.
***
To properly fix the overall bug, one possibility would be to have the user module store a list of all permissions defined by modules (before the new modules are installed), then compare it to the same list generated immediately after the new modules are installed, and assign any new ones to the admin role (regardless of which module defined those new permissions).
However, I don't think we currently have hooks in core that allow that.
Comment #36
carlos8f CreditAttribution: carlos8f commented@David, I have confirmed that I was wrong about hook_install() and hook_modules_installed(). Thanks for pointing that out.
One problem with the patches in this issue so far: looks like we are breaking the contract of "Changing this setting will not affect existing permissions.". Calling user_administrator_grant_permissions() (or whatever it will be called) outside of hook_install() would overwrite existing permissions.
I echo what @David says, we need to keep a persistent list of permissions and act on the new ones, but the trick is finding a reliable way and time to catch those changes.
Comment #37
carlos8f CreditAttribution: carlos8f commentedHere's a wild idea I just had.
Use a cache_get() in user_role_permissions(), and on a cache miss we invoke hook_permissions(), and update the admin role then, if the list includes new permissions. Then the API function to call when adding/changing permissions would just clear the cache.
Comment #38
tsvenson CreditAttribution: tsvenson commented@int
Sorry about that. Didn't know it worked that way. Next time I'll keep it in dev and mention in the post itself what version I was using.
Comment #39
catch@carlos: if we can do that cleanly, it seems like a decent option here that'd avoid a serious API change.
Is this a new table, or a status column in {role_permission}?
My original intention with the admin role patch was that you could disable a handful of absolutely site pwning permissions manually, I still think that and the fact that these are real permissions rather than user_access() magic was a good idea, but I definitely didn't think about dynamic permissions when writing the patch. The number of modules that define dynamic permissions is relatively small, so if they need to add a cache clear (for example in node_type_save()) that's better than requiring something of every module that defines hook_permission().
Note there's an issue for caching user_role_permissions() here - #684612.
Comment #40
carlos8f CreditAttribution: carlos8f commented@catch: thanks for chiming in there, I am experimenting with a patch right now. I don't know the exact implementation yet but I have something going.
Comment #41
carlos8f CreditAttribution: carlos8f commentednew title
Comment #42
carlos8f CreditAttribution: carlos8f commentedFirst shot at a patch. User and Upgrade tests are passing, which is encouraging. I also was able to install manually without anything wonky happening.
- This patch handles changes in permissions through user_role_permission(), which in conjunction with a new function user_flush_permissions() makes the admin role eat up new permissions on-demand. It also adds cache and locking API support to make the rebuilding process as painless as possible.
- To facilitate determining newly defined permissions, I added a status flag to the {role_permission} table, and I store all the defined permissions for the admin role, with status 1 meaning permission is granted.
- Theoretically you would also be able to switch the admin role without losing the list of defined permissions; I tried to build it to handle that case.
- And finally, it keeps the contract of "Changing this setting will not affect existing permissions.", at least I think it does :)
There is probably some stuff this patch doesn't address yet, but it's a good start, I think.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedWith this patch, if I create a new text format and uncheck the box next to "administrator", administrators still wind up with permission to use the format... Similarly, it looks to me like any time drupal_flush_all_caches() gets called in any context, the administrator role can randomly be assigned new permissions, which doesn't seem good.
So I think this approach might really need a database "cache" that is outside the normal caching system (i.e., one that never gets flushed automatically), so that it is entirely opt-in.
But taking a step back here, are we sure we are fixing a bug, vs adding a new feature at the same time? The OP is definitely a legitimate bug; it points out that when you install a new module, you don't always get all new permissions associated with that module (even though we say you do). But that's not the same as saying the administrator must get any new dynamic permission that appears in any other context. I can see how it would be less confusing (to keep the row of checkboxes nice and neat) but as a practical matter, if you've given your administrators catch-all permissions like "bypass node access" or "administer taxonomy" then the dynamic permissions are superfluous anyway.
Also, they are somewhat independent issues, because consider the case of the PHP module: Even though the filter system does not in general want Drupal to automatically grant admin permissions when a new text format is created, turning on the PHP module probably still should grant the "Use the PHP code text format" permission (since that's what we promise, and it's the entire point of turning on that module).
If all we want to do is solve the original OP here, then the questions (and the patch here) become a lot simpler. The only place we'd need to cache current permission info would be at the beginning of module_enable(), and it could be a static cache (since it is only used by user_modules_installed() later in the same request). We could do this by adding a new hook that fires right before modules are enabled, or honestly if we wanted to avoid a new hook this late in the game we could hardcode it in module_enable() itself (for Drupal 7); that would be ugly, but given that module_disable() already hardcodes node module stuff, it's not any uglier to have module_enable() hardcode user module stuff :)
So.... what is the scope of the actual bug we are trying to solve in this issue?
Comment #45
sunIt looks like the only solid way would be to
1) Add a {role_permission}.status column. (1 = granted, 0 = not granted)
2) Which implies to store all permissions in {role_permissions}, not only granted permissions.
3) Which implies that there is a difference between "never granted" and "not granted" permissions.
4) Whenever a permissions of a module change (e.g., node_type_save()), make it invoke a user role API function; e.g., user_role_change_permissions(), without any arguments.
5) That API function invokes a hook, which calls an implementation of User module similar to user_modules_installed(), but which is then able to query {role_permissions} for the status column to figure out any new permissions. Pseudo code:
--
However, it's way too late for doing that. So we need a simpler and possibly a bit more fragile workaround that depends on modules implementing the right code at the right time.
Modules that implement configuration items using dynamic permissions should know very well when they have new permissions and when previously existing permissions are removed (because the configuration item has been removed).
Therefore, we can get away with two simple API functions: user_role_permissions_insert() and user_role_permissions_delete().
By the way: I guess that the stale {role_permission} records for removed, dynamic permissions is a hidden minor bug that no one discovered so far. We purposively changed the user permissions API and UI to not blatantly change or remove any permissions that were not explicitly passed to be changed, so as to not change existing user permissions of modules that are (temporarily) disabled currently. This, however, can lead to stale records, especially for modules implementing dynamic permissions.
--
I have a basic implementation ready, but the connection to d.o CVS times out.
Comment #46
sunResorted to git.
Comment #48
int CreditAttribution: int commented#46: drupal.user-dynamic-perms.46.patch queued for re-testing.
Comment #50
carlos8f CreditAttribution: carlos8f commentedI think the method in #46 has a lot of opportunity for failure, if the permissions aren't passed exactly to the permission change API as they are generated in hook_permission(). We're even seeing in this small patch that we forget to reset the _node_types_build static, so node_list_permissions() is failing. Ideally, permissions are never generated outside of hook_permission().
I'm resigned to dropping my earlier idea if the consensus is that it's too late. It certainly would be a large under-the-hood change this late in the cycle, and would need to be tested up the wazoo.
Comment #51
catchIf this is genuinely a critical bug, then I don't see a problem with the schema change. We added a much bigger one for filters which introduced security and upgrade path issues, and that was for a feature request.
However I think David's right that this doesn't really qualify much as a bug at all.
The original admin role issue (and the D6 module) was designed to solve the problem that when you enable a new module, you can't use it /at all/ if it defines its own permission and you're not user/1, until you went to the permissions page. The inconsistency here doesn't affect this at all, it just affects node types, vocabularies etc., for which you'll often be configuring permissions for other roles too as part of the process of setting them up.
Comment #52
carlos8f CreditAttribution: carlos8f commentedIf this is not a bug, is it "by design" then? Perhaps we just change the wording to say that some permissions must be manually set.
There's another thing that hasn't really been discussed: since the admin role currently only picks up new permissions when modules are installed, it means it won't pick up new permissions that might be added in new versions of the module. That would extend the problem to modules that provide statically defined permissions.
If we're going to do a "fix" for this though, and have arbitrary new permissions added to the admin role, I like David's idea of carrying a static cache of permissions from module_enable() to hook_modules_enabled() and compare the results of user_permission_get_modules(). That would seem to be a very simple and probably effective approach.
Comment #53
sunA regular cache is too fragile. Core does not provide any non-volatile cache-alike data store facility aside from variables. Storing the complete list of last known permissions in a variable would have a negative performance impact on every single request.
But yes, we identified more than one problem:
The last item does not really belong into this issue, but the underlying, technical challenge is exactly the same, so it would be a good idea to take it into account in here.
Comment #54
carlos8f CreditAttribution: carlos8f commented@sun, the static idea David had would mean we wouldn't have to use cache API or a variable, for catching new permissions when modules are enabled. But if we want to catch new permissions on update.php for example, we would need more than that. Hmm. My patch deals with the update.php case by clearing the permissions cache on hook_flush_caches(), but in hindsight it might be better to code that directly into drupal_flush_all_caches() so it doesn't run on every cron, etc.
Comment #55
boombatower CreditAttribution: boombatower commentedAlso see #1153072: New content types (and other dynamic) permissions are not auto-selected for admin role—always give it all perms and subscribe.
Comment #56
sunComment #57
scotwith1t#1005554: Administrator role does not get new content type or vocabulary permissions was also marked as a duplicate of this. Subscribing.
Comment #58
webchickI'd still really like to see this fixed. It's a very bad, obvious, user-facing bug, as evidenced by numerous duplicate reports. I don't think we can call this "by design"; the whole point of the administration role is "set it and forget it", not "set it and forget it, oh except for when..". However, retaining the ability to de-select certain options from the administrator role and not have them creep back up on you is indeed a necessary constraint. sun's approach in #45 though feels too big for D7. My initial thought was variables table, but it's true that this list could get extremely lengthy and that could add up to potentially KB of RAM in a request.
This was set to major at one point, then bumped to critical. When it was decided it's not critical, it shouldn't have been bumped back to "normal" again, but back to major; restoring old status.
Comment #59
tsvenson CreditAttribution: tsvenson commentedYes. That is very important. Here is a great use case for that:
I have created a setup where I use both the Core Toolbar and the Administration menu. For those users that is given the "administrator" role, I have turned off the Toolbar as the Administration menu provides a much faster way of navigating around in the admin interface and 6 shortcuts is simply to limited in most cases.
For roles that work on content however, the Toolbar provides a much more polished interface and combined with tailored shortcut sets, based on role, it will give a much more intuitive user experience for content administrators for example.
To get this to work, the Toolbar permission for the "administrator" role needs to be turned of, otherwise those roles will have the Administration menu on top of the Toolbar.
Comment #60
catchIf we're going to try to fix this in D7, we should do the status column as sun outlined in #45, I don't see another way to do it cleanly.
Comment #61
sunMost probably, #45 is the only viable solution. However, we should double-check that it actually fixes all issues listed in #53. I'm not sure whether 2-4 would actually be covered -- unless we'd hack the permissions sync into user_flush_caches().
Comment #62
scotwith1tI don't follow the whole conditional administrator role mentality. The "set it and forget it" approach that webchick and others are talking about is, in real terms, allowing for multiple "user 1"s with all permissions. Period. If you want to limit someone's role, you should have to create one for that purpose, even if they will only be denied one permission (administrator - 2nd tier or something). This functionality should be "all the way".
Comment #63
boombatower CreditAttribution: boombatower commented#62 puts it exactly right. The original module that this feature came from had that mentality, that's how I use it, that's what makes sense. You are and Administrator or you aren't. Anything else is another role...a select all checkbox that you can then deselect what you want is more then sufficient. Lets not over-complicate a very simple and extremely useful feature.
Comment #64
catchAuto-assigning permissions vs. hard coded user_access() style magic was discussed in detail and was rejected for the reasons in #58/#59 (although iirc more so due hard coding being confusing - the security team still gets e-mails from people who don't understand why user/1 can do everything even though they don't have roles/permissions assigned).
If you seriously want to change that, please open a new issue for it and read #182023: Add a third default role to core for handling Administrative duties, #480660: Add an 'administrator' role to core, #497500: Create an administrator account in the default install profile, separate from user 1, #570572: Change label for user/1 account from "administrator" to "site maintenance account", #1597: Create Site Owner Role and Replace uid==1 checks with rid==DRUPAL_SITEOWNER_RID and #468768: Remove hardcoded anonymous and authenticated user roles (at least). IMO that is a Drupal 8-only feature request, if there was sufficient support for it and it could be backported to Drupal 7 without a UI change then go ahead, but it would only fix half the bug here anyway.
Re-titling to make it clear what the actual bug is here (as opposed to just the symptom).
Comment #65
boombatower CreditAttribution: boombatower commentedThere are two issues here: 1) fix the current administrator role to be simply that an administrator role equivalent to user 1, and 2) followup by removing hard coded user/1 access rules by assigning user/1 administrator role.
#1 can be done without #2. I think from a abstract perspective #2 is a good idea, but for practical reasons I get the feeling people will hose their Drupal sites and user/1 at least provides a nice insurance policy. Obviously, we could go forward anyway and/or provide some sort of steps to create an admin account if you accidentally them all :)
To be clear #1 is not dependent on #2 and #2 will be a lot more work and discussion, but #1 can be done quickly and easily for both D7 and D8.
http://drupal.org/node/1153072#comment-4500082 solves the problem for both dynamic and administrator role behaving in the expected and predictable manor. If you want some sort of weird semi-dynamic role with specific permissions being excluded then you should write that functionality (probably contrib). Meaning have permissions that are "banned" or "excluded" for a particular role, but the role get's everything else. Currently, that is what the code is attempting to provide, but doesn't do so and isn't clear to expect that as the result.
The current design is unpredictable, doesn't work with dynamic permissions, and contradicts expectations and code comments.
Comment #66
catchThat leaves http://api.drupal.org/api/drupal/modules--user--user.module/function/use...
If a module creates a node type in hook_install() and removes it in hook_uninstall(), there's no way for user module to remove the permissions automatically.
Comment #67
webchickI don't know that automatic removal is that big of a deal. The extra permissions will do nothing, since the functionality they affected is gone, and the next time you save admin/user/permissions it should clean things up.
Comment #68
scotwith1tre: #66 (webchick already basically said it, but i had typed this and forgot to save) shouldn't matter if a) you're an admin and should have permissions to everything (theoretically) and b) the content type doesn't exist anymore (besides, if they're recreated, you should again have access if you're admin)? If people don't get that user 1 (and people assigned an admin role theoretically) have all permissions automatically, perhaps this is a documentation flaw and just needs to be made more clear via a larger font, popup confirmation, whatever, to make it clear to newbies that admin role has all permissions. Could perhaps be made more clear in the initial Drupal install as well if it's cause for a bunch of needless busy work for the security team responding to inquiries about this...
boombatower hit it head on too in #65 that the more dynamic/complicated option can be done independently from this relatively simple fix. i second that the more complex fix should be pushed to D8 only as a new feature or even tabled as altogether unnecessary.
Comment #69
boombatower CreditAttribution: boombatower commentedThe issue referenced in #65 performs the role updates on flush caches so if the content types were removed and caches flushed (either by the process or manually) the stale permissions are then removed since they are no longer part of the full list of permissions.
Comment #70
catchIf we deleted permissions just because they're missing from the form, then you would see data loss just from disabling and enabling modules (those permissions will also not be found in hook_permission()).
Assigned permissions for content types stick around forever if you delete a content type - I just ran through manually and checked the database to be sure.
All permissions for any role are loaded into memory on every request, so there is a runtime cost to having stale permissions.
Comment #71
boombatower CreditAttribution: boombatower commentedSo the clean fix is to get rid of all the silly exceptions and rules and make a true administrator role like in #65 that always has all the permissions and is always rebuilt on cache clear so you have no ill-effects from module enable/disable.
Seems to be more confirmation that this approach is all that can be done without a lot more work and thought for all the odd edge cases. Definitely seems like contrib material the more edge-cases are brought up.
Comment #72
catch@boombatower, this bug affects all permissions that are assigned via the interface and code for any role, it is not at all related to the administrator role handling except that it exposes the bug. Sun pointed this out in November in #53, so it is strange that people are both ignoring it and claiming core behaves in a completely different way to how it actually does over and over again. Adding the needs tests tag, I'll probably write one that demonstrates this bug if only to avoid having to explain it any more times.
I really don't care about the behaviour of the administrator role at this point, if people are happy to make it impossible to remove permissions from it in Drupal 7, then IMO work should continue on the other issue you opened. This means that sites relying on the current behaviour will see permissions added to that role that they had previously removed - I have no way of knowing if any sites are relying on that, but comments in here suggest it is desired for some workflows.
However even if you do that, while it will fix the user facing symptom reported here via a behaviour change, it would do nothing about the underlying issue in the user API (i.e. that dynamic permissions that no longer exist are never, ever cleaned up, and there is no way to programmatically act on newly defined dynamic permissions).
Comment #73
catchOne tag too few.
Comment #74
boombatower CreditAttribution: boombatower commentedEveryone seems to forget that the role already has unknown permissions since it depends when you enable the role during your site setup. If I install all my modules and then enable it I get nothing...if I enable the role and then add all my modules I get everything. Already supper inconsistent. I don't know exactly how this is all irelevant to the bug that was pointed out...no where did I state this solves that bug...it simply removes the issues for the admin role as a side effect of making the admin role work consistently. I seem to have been somehow offensive which was not my intent.
Comment #75
David_Rothstein CreditAttribution: David_Rothstein commentedIf this is really a major bug, could someone justify that with a specific example of pain that this is causing actual sites?
Per earlier comments, the fact that we have "administer taxonomy" and "bypass node access" should render most of these dynamic permissions superfluous to a typical admin user anyway.
The PHP module (and text formats in general) are an exception to that, and of course there may be other exceptions in contrib, so it's a bug either way... but before we spend lots of time on what would likely be a difficult solution (in my opinion all solutions listed here so far have some difficulties, especially when it comes to D7 backport), it would be useful to decide if this really should count towards the 100 major bug limit or not.
Comment #76
David_Rothstein CreditAttribution: David_Rothstein commentedTrue, but that issue sounds like it would be a whole lot easier to fix than the problem with dynamic permissions we're discussing here. We could easily create an API function that gets called when the admin role is enabled and which assigns all existing permissions to the new role at that time.
It's also documented behavior that it doesn't do that currently; the form item description says: "This role will be automatically assigned new permissions whenever a module is enabled. Changing this setting will not affect existing permissions."
Comment #77
catch@boombatower, this is what you posted:
I read this to mean that this would solve this overall issue, and I'm not really sure how else to read it.
To summarize my position on this:
- I don't really care if we allow people to disable permissions for the admin role or not. In terms of fixing the user facing symptom, dropping that feature is definitely the quickest way to get that part fixed. When I originally worked on this, that was not a critical feature, it was just something accounted for and it appears to be behaviour that people wanted/want.
- To change that behaviour t is going to require disabling checkboxes on admin/people/permissions (to make it clear permissions can't be removed), possibly changing help texts etc.- it is really up to webchick whether that kind of change is acceptable for Drupal 7 (along with the change in permissions behaviour itself). Does the other issue deal with those changes yet?
- The fact that there is no way to respond to the creation and removal of dynamic permissions is a pre-existing bug that was simply exposed by the administrator role. Unfortunately it was only noticed a year after the administrator role patch (which I unfortunately worked on, hence taking this slightly personally which I apologise for). These deficiencies in the API are also related to other major bugs like #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced (which fundamentally comes down to us linking permissions to modules rather than functionality).
- if we fix those underlying bugs, then fixing the current implementation would be fairly simple to implement, I don't think there will be that many edge cases/workarounds on top of what's necessary to fix the underlying API.
So what it comes down to is:
#1 Fix this by changing the behaviour - working around the underlying bugs which can then be worked on separately. This requires possibly non-trivial UI/feature changes during a stable release but shouldn't require schema changes or API changes.
vs.
#2 Fix the underlying bugs (which requires non-trivial schema changes and API additions), then try to fix the user-facing issue based on that.
Neither of these are great options, but I'm assuming people want this fixed in D7, so we need to decide what the least worst is and try to implement that within the constraints of a stable release.
Comment #78
tsvenson CreditAttribution: tsvenson commentedIsn't the way the Administrator role works the complete opposite to how every other role is working? I mean, for any other role the default setting is that the permission is not set, but for the Admin it is set.
Then, to be able to manually unset permissions for the administrator role, those are the ones Drupal needs to know. Wouldn't that be easy to find out when I manually trigger save permissions? Drupal can then store the list of the "unset" administrator permissions to a variable.
Then, when new permissions are added, such as when a module is enabled, Drupal only need to read that variable row and set every permission for the administrator role except those it find there. There would be no need to know the names of the newly added permissions.
Just thinking loud...
Comment #79
sunAt this point I think we can only opt for #77 2) Fix the underlying bugs with internal DB changes, since we just simply don't know whether anyone actively relies on the current behavior. Changing it under the hood would be a serious security issue.
Let me also clarify that cleaning up permissions has to be a non-automated operation, because we do not know which permissions exist and existed. This primarily concerns disabling and enabling of modules, see #535680: Prevent removing permissions of disabled modules
Comment #80
boombatower CreditAttribution: boombatower commented@catch: My point was simply that a symptom of changing the way the admin role works is that it doesn't expose the bug you referred to. Not that we should choose the solution because of that.
All along I have been arguing that the current functionality is just plain dumb regardless of any implementation issues. The fact that changing it to work without all the odd exceptions happens to hide the bug is just another pleasant side-effect.
A simple upgrade path for Drupal 7 would be to convert current admin role to another role (unset it as the admin role and optionally make a new role for that). That way if anyone needs it to behave in an inconsistent and odd fashion they get whatever permissions they had. So if changing that is too much for Drupal 7, then great the bugs need to be fixed either way so lets just make this feature functionally useful in Drupal 8.
Start by implementing http://drupal.org/node/1153072#comment-4500082 and then we can add some sort of "banned" or "excluded" permissions for roles which is what this current odd behavior was trying to support. Then people can have either behavior instead of just one. The excluded permissions will also be useful for other roles and even more so once dynamic permission handling is "fixed."
Comment #81
boombatower CreditAttribution: boombatower commentedbump, any chance of getting a review for the patch mentioned in #80.
Comment #82
webchickThis continues to be a totally embarrassing and obvious bug. :P Grumble.
I'm warming up to the idea of just forcing all admin perms on for the administrator role, as if you want a "conditional admin" role you could do it by creating your own and managing by hand. But not sure we can still make that change this late in D7. Hrm.
Comment #83
tsvenson CreditAttribution: tsvenson commented@webchick:
Do you mean no option than all perms always on? If so, I'm not to fond of that. I would like to have the option to be able to turn off some and only have new perms ticked automatically.
What about a button, "Apply for all permissions", in the Administrator Role field group on admin/config/people/accounts/settings.
Comment #84
boombatower CreditAttribution: boombatower commented#80 describes both a D7 upgrade path and as others have mentioned much more is needed to support what is currently _attempted_ to be supported. That being a conditional admin role. So wether or not people want that a) it's much more complicated, b) the original module that is marked as being in core supported a "admin" role (aka user/1 clone) which is confusing since this is not. So lets get on with life and _fix_ this so that that module is accurately merged into core and then fix the _feature_ request to support conditional admin roles.
Comment #85
David_Rothstein CreditAttribution: David_Rothstein commentedEven if we wanted to change the intended admin role behavior, we'd have to change the UI also. (Everywhere this role's permission checkboxes appear in core and contrib, the checkbox would have to be disabled.) I don't think that's going to happen for D7.
Here is a possible plan for fixing the bug for D7 instead. In #53, @sun listed three things that this issue is about:
This is the original report that started this issue, and it's definitely a bug. I believe this can be fixed in a backportable way via the method I described in #35 and #44. I haven't heard anyone describe why that wouldn't work.
Whether or not these are bugs is up for debate, but certainly some modules may want to do this. However, there is no way core can do this automatically for them, because:
The most core can do here is provide an API to help modules do what they want. @sun's patch in #46 looks like a good start at this.
***
By combining those approaches it seems like we could solve everything here for D7. (In fact, this could be split into different issues since the two things aren't really related.)
Comment #86
liquidcms CreditAttribution: liquidcms commentedjust my $0.02 worth.
adminrole module in D6 was used on all our D6 projects.
Cool that people are coming up with all sorts of variations of this and sucks that what is core in D7 missed that boat. But that module's very obvious, very basic functionality is still a major requirement - "expand the ability of uid = 1 to a role". All the variations on this (mentioned above), like apis for modules to do this themselves, should be secondary to the main idea that that module did so nicely.
Comment #87
liquidcms CreditAttribution: liquidcms commentedmaybe the functionality of adminrole module (D6) with a hook for modules to opt out (although can't for the life of me think of why that would ever be needed - if you have some role where you don't want certain modules to be managed by a certain role, hey.. make a new role and set it up as you like).
Comment #88
scotwith1tI, like @boombatower and others, am wondering why this is so complicated. admin module did one thing and did it well; grant uid=1 access to a role (except for update.php). if you want a "semi-admin" role, create one and manually manage the permissions. don't let modules opt out of the admin role functionality, it was baked into core with the understanding that it would be "adminrole-like" (i.e. you can't deny that role permissions to use the module or anything else for that matter).
Comment #89
boombatower CreditAttribution: boombatower commentedThe opting out definitely feels like a less useful (and overly complicated with many different possiblities) case that should be dealt with in contrib or in a separate issue.
Comment #90
liquidcms CreditAttribution: liquidcms commentedsad that this still hasn't gotten sorted out. maybe at the least we should remove the "admin role" setting in user settings as it certainly must be misleading to people setting up a site for the first time as it doesn't do what the obvious intent would be.. doesn't really do anything imho other than confuse people into thinking that they have an actual "Admin" role that can be assigned to their users.
Comment #91
xjmThis issue could use a good summary.
Comment #92
scotwith1tAdded issue summary. Feel free to edit.
Comment #93
David_Rothstein CreditAttribution: David_Rothstein commentedThanks, but that summary seems to be mixing #1153072: New content types (and other dynamic) permissions are not auto-selected for admin role—always give it all perms with this issue a bit. Several people above have explained why that approach won't work for Drupal 7.
I think #85 is still a relatively accurate summary. I'm going to work on this a bit and try to come up with a new patch.
Comment #94
David_Rothstein CreditAttribution: David_Rothstein commentedHere are two patches, building off @sun's work in #46 and adding to it as discussed in #85.
Both patches guarantee that a newly-installed module has all its permissions assigned to the admin role (i.e. they both fix the original issue that was reported here), and both also add the API which allows modules to go further if they need to.
The first patch additionally makes core modules use this API for their dynamic permissions; i.e., it tries to make this true:
There are a number of problems with this, and the patch is going to fail tests pretty badly.
The second patch just does this:
(i.e., corrects the misinformation and then makes it actually do what it says)
Above I mentioned splitting this into two issues, and to me this seems like a really good place to split it. (Especially keeping in mind that for core, the "missing checkboxes" that remain after the second patch have essentially no meaning anyway; it's basically just a cosmetic issue once we've fixed the original bug reported here).
The patch still needs work in a couple areas, and it also needs tests. Another thing to note is that unfortunately the new D8 hooks like hook_modules_preinstall() and hook_modules_preenable() do not work for the purpose of this issue, so I needed to add another set of module hooks that runs even earlier. That's why the patch files wind up large (there are a lot of indentation changes in module.inc that result from adding the new hooks, but still not too many actual code changes).
Comment #96
David_Rothstein CreditAttribution: David_Rothstein commentedThe 1,000 exceptions are due to a small bug in the patch (only manifests itself when user.module is being installed). That's fixed in the attached patches.
The 4 failures that are shared between the patches are due to simpletest weirdness. I've verified that combining this patch with #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations fixes them. So, we'd need to get that patch committed first.
Comment #98
scotwith1tapparently i don't understand the issue because I fail to see the difference between this and #1153072: New content types (and other dynamic) permissions are not auto-selected for admin role—always give it all perms seem like duplicates to me, but i don't claim to know anything about the inner-workings of the code, just from a site-building perspective it seems like the same thing.
Comment #99
xjmRetagging.
Comment #99.0
xjmAdded improved issue summary
Comment #99.1
xjmUpdated issue summary.
Comment #99.2
xjmUpdated issue summary.
Comment #99.3
xjmUpdated issue summary.
Comment #100
xjmI talked to @David_Rothstein about this issue a bit on IRC, and it seems to me there are separate functional bugs, as @sun nicely outlined in #53. The difficult part of this issue (and much of the discussion above) involves how to handle dynamic permissions, as in the title. However, that's not actually the issue with the highest impact.
The major bug here is the issue described in the original post, where a module's permissions are not granted to the administrator when the module is installed. Once that issue is resolved, the issue of dynamic permissions not being assigned becomes less significant, because in every case I could find (going through a few contributed modules), the dynamic permissions were always a partial subset of one of the module's main permissions. So, if we fix that, we fix the functional bug for most users, and are left with a less significant bug--it's inconsistent that the dynamic permissions are still not granted automatically, and even if the user can do what those permissions would allow, it's bad UX for those boxes to be unchecked.
So, based on this (and the two separate patches above), I opened #1454686: When a new module is installed, the admin role doesn't get all new permissions at major. I left this issue as the second bug, because it already has the discussion about the implications of the dynamic permissions. I'm in the process of trying to disentangle the two issues (and patches) a bit more, and adding partial summaries.
Comment #100.0
xjmUpdated issue summary.
Comment #100.1
xjmUpdated issue summary.
Comment #101
David_Rothstein CreditAttribution: David_Rothstein commentedIn response to #98 and others, I went ahead and added some information to the issue summary to provide a little more detail about why #1153072: New content types (and other dynamic) permissions are not auto-selected for admin role—always give it all perms is a separate issue from this one.
Comment #101.0
David_Rothstein CreditAttribution: David_Rothstein commentedAdd information explaining why #1153072 is a separate Drupal 8 issue, rather than combined with this one
Comment #101.1
David_Rothstein CreditAttribution: David_Rothstein commentedfix typo
Comment #101.2
xjmUpdated issue summary.
Comment #102
mitokens CreditAttribution: mitokens as a volunteer commentedComment #103
claudiu.cristeaIt seems that Drupal 8 is solving the problem in a different way. See #2435075: Implement admin role as a flag on the role storage, simplify permissions page, remove user_modules_installed. In this case this remains a Drupal 7 issue.
Comment #104
liquidcms CreditAttribution: liquidcms commented@claudiu.cristea, certainly not a solution to fixing core; but a couple years back i did port over the D6 adminrole module (https://www.drupal.org/project/adminrole) to D7 to provide what most expect of a basic "admin role". perhaps this is solution enough for D7 (although i still think removing suggestions in D7 that core does this, would be good)?