Dynamic permissions cannot be automatically assigned to or removed from roles

The tests don't pass but otherwise this patch does the trick. I did a bunch of debugging and can't figure it out.

Arguably, node_type_save() and node_type_delete() should clear the static node_type_build cache before calling their hooks.

Yup, 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.

Needs the debug() line out of the patch. :)

Tested and it worked wonderfully :).

1. Before Patch: I created a node-type "nodeynode" and admin didn't have any permissions to it
2. After Patch: I created a node-type "nodernoder" and admin had all permissions to it as well as all permissions to "nodeynode" types.

+++ modules/user/user.module	2 May 2010 05:55:08 -0000
@@ -3553,3 +3542,45 @@ function user_rdf_mapping() {
+/**
+ * Implements hook_node_type_insert().
+ */
+function user_node_type_insert($type) {
+  user_administrator_reset();
+}
+
+/**
+ * Implements hook_node_type_update().
+ */
+function user_node_type_update() {
+  user_administrator_reset();
+}

Doesn't this belong in node module? It seems unrealistic for user.module to know about all things that could change the permissions.

Agreed.

I 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.

User is core required, Node is slowly moving to core optional. Consequently, it makes more sense to have those in node.

If, 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).

You 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.

Node it is.

oops

tsvenson: 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.

Awesome! :) thanks!

It 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.

The 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 :)

+++ modules/node/node.module
@@ -524,9 +524,13 @@ function node_type_save($info) {
+    // Clear the node type cache.
+    drupal_static_reset('_node_types_build');

@@ -536,14 +540,18 @@ function node_type_save($info) {
+    // Clear the node type cache.
+    drupal_static_reset('_node_types_build');
...
-  // Clear the node type cache.
-  drupal_static_reset('_node_types_build');

@@ -627,10 +635,11 @@ function node_type_delete($type) {
-  module_invoke_all('node_type_delete', $info);
 
   // Clear the node type cache.
   drupal_static_reset('_node_types_build');
+
+  module_invoke_all('node_type_delete', $info);

If 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.

+++ modules/user/user.module
@@ -3573,18 +3573,7 @@ function user_register_submit($form, &$form_state) {
+  user_administrator_grant_permissions($modules);

@@ -3664,3 +3653,31 @@ function user_rdf_mapping() {
+function user_administrator_grant_permissions($modules = NULL) {

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.

This 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. :\

@tsvenson if you change to beta2 tag, it don't show in the Critical count list. Go get again in the dev version.

Looking 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.

Can we do something like,

  // User #1 has all privileges.
  if ($account->uid == 1) {
    return TRUE;
  }
  // So do users with the administrator role.
  if (($admin_role = variable_get('admin_role', 0)) && isset($account->roles[$admin_role])) {
    return TRUE;
  }

in user_access(), or am I missing something big?

I guess that doesn't work because of this (description for the admin role option):

Changing this setting will not affect existing permissions.

/**
 * Implements hook_modules_installed().
 */
function user_modules_installed($modules) {
  // Assign all available permissions to the administrator role.
  $rid = variable_get('user_admin_role', 0);
  if ($rid) {
    $permissions = array();
    foreach ($modules as $module) {
      if ($module_permissions = module_invoke($module, 'permission')) {
        $permissions = array_merge($permissions, array_keys($module_permissions));
      }
    }
    if (!empty($permissions)) {
      user_role_grant_permissions($rid, $permissions);
    }
  }
}

This is a flimsy way to go about things. The description of the admin role reads,

This role will be automatically assigned new permissions whenever a module is enabled.

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():

  // Create a default role for site administrators, with all available permissions assigned.
  $admin_role = new stdClass();
  $admin_role->name = 'administrator';
  $admin_role->weight = 2;
  user_role_save($admin_role);
  user_role_grant_permissions($admin_role->rid, array_keys(module_invoke_all('permission')));
  // Set this as the administrator role.
  variable_set('user_admin_role', $admin_role->rid);

Apparently, the only reason why the admin role works at all, is that the standard profile sets it up by default.

This role will be automatically assigned new permissions whenever a module is enabled.

I 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.

Only modules that implement hook_install() are passed to hook_modules_installed()

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.

@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.

Here'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.

@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.

@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.

new title

First 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.

With 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?

It 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:

$rid_admin = variable_get('user_admin_role');
$existing = array_flip(db_query('SELECT permission FROM {role_permission} WHERE rid = :rid', array(':rid' => $rid_admin))->fetchCol());
$current = module_invoke_all('permission');
$new = array_diff_key($current, $existing);
user_role_gant_permissions($rid_admin, array_keys($new));

--
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.

Resorted to git.

#46: drupal.user-dynamic-perms.46.patch queued for re-testing.

I 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.

If 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.

If 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.

A 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:

1. Permissions of certain (core) modules are not granted to the admin role when being installed.
2. Permissions for new configuration items are not granted to the admin role.
3. Permissions introduced in a module update are not granted to the admin role.
4. Permissions of existing configuration items are not removed (from any role) when the configuration items are removed.

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.

@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.

Also see #1153072: New content types (and other dynamic) permissions are not auto-selected for admin role—always give it all perms and subscribe.

#1005554: Administrator role does not get new content type or vocabulary permissions was also marked as a duplicate of this. Subscribing.

I'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.

If 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.

Most 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().

I 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".

#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.

Auto-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).

There 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.

That 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.

I 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.

re: #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.

The 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.

I 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.

If 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.

So 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.

@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).

One tag too few.

Everyone 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.

If 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.

Everyone 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.

True, 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."

@boombatower, this is what you posted:

So 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.

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.

At 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

@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."

bump, any chance of getting a review for the patch mentioned in #80.

This 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.

#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.

Even 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:

1. Permissions of certain (core) modules are not granted to the admin role when being installed.

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.

2. Permissions for new configuration items are not granted to the admin role.
3. Permissions introduced in a module update are not granted to the admin role.

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:

• For 2, some modules may want to make the permissions an immediate choice in the UI (filter module does this for new text formats), and that must be respected. So only the module can decide when/how/if to grant the permission.
• Similarly for 3, core has no way of knowing whether a "new" permission is actually new (as opposed to renamed), so it cannot grant it automatically to the admin role. Again, only the module can decide that, by writing an update function to make the actual changes they want to make.

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.)

just 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.

maybe 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).

I, 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).

The 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.

sad 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.

This issue could use a good summary.

Added issue summary. Feel free to edit.

Thanks, 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.

Here 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:

-    '#description' => t('This role will be automatically assigned new permissions whenever a module is enabled. Changing this setting will not affect existing permissions.'),
+    '#description' => t('This role will be automatically assigned new permissions whenever a module is installed or when new permissions otherwise appear. Changing this setting will not affect existing permissions.'),

There are a number of problems with this, and the patch is going to fail tests pretty badly.

The second patch just does this:

-    '#description' => t('This role will be automatically assigned new permissions whenever a module is enabled. Changing this setting will not affect existing permissions.'),
+    '#description' => t('This role will be automatically assigned new permissions whenever a module is installed. Changing this setting will not affect existing permissions.'),

(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).

The 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.

apparently 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.

Retagging.

Added improved issue summary

Updated issue summary.

Updated issue summary.

Updated issue summary.

I 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.

Updated issue summary.

Updated issue summary.

In 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.

Add information explaining why #1153072 is a separate Drupal 8 issue, rather than combined with this one

fix typo

Updated issue summary.

It 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.

@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)?