When a new module is installed, the admin role doesn't get all new permissions

Patch by David_Rothstein from #787152: Dynamic permissions cannot be automatically assigned to or removed from roles. The two test failures in this patch are caused by #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations.

updated summary

Tagging.

And, I'll write a functional test for this.

Ugh. The new hook names sound pretty wonky to me.

And, the use-case implementation in User module is even more wonky:

+++ b/core/modules/user/user.module
@@ -3828,21 +3873,31 @@ function user_register_submit($form, &$form_state) {
+function user_modules_to_be_installed($modules) {
+  // Record all currently defined permissions, for later use in
+  // user_modules_installed().
+  $all_permissions = &drupal_static(__FUNCTION__);
+  $all_permissions = array_keys(module_invoke_all('permission'));
+}

This won't work within module updates, since it's entirely based on hooks.

In fact, this will also lead to unexpected results when modules are disabled (after being installed), and one of the newly installed modules defines the same permission as one of the disabled modules.

#607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced too.

Ugh. The new hook names sound pretty wonky to me.

Agreed. We have so many hooks that run when modules are enabled that all the good names were taken :)

This won't work within module updates, since it's entirely based on hooks.

Solving issues with module updates isn't the goal of that code. Or really the goal of this issue anymore, given the way the issues were split...

Although I think the code in the patch actually does help solve it, since that's what functions like user_permissions_insert() and user_permissions_delete() could be used for. As I said in the other issue, I'm not sure we can automate that case further because it doesn't seem like there's any general way to distinguish between a module which added a new permission (which should always be given to the admin role) vs. a module which renamed an existing permission (which should only be given to the admin role, and other roles as well, if they had the old permission).

In fact, this will also lead to unexpected results when modules are disabled (after being installed), and one of the newly installed modules defines the same permission as one of the disabled modules.

I'm not sure I follow that. Is there a specific example of what would happen? As far as I can see, this won't cause any problems that don't already exist as a result of #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced.

More accurate title (actually borrowing something closer to the one #787152: Dynamic permissions cannot be automatically assigned to or removed from roles used to have originally).... The module's permissions themselves are always granted to the admin role already. The issue here only occurs for dynamic permissions defined by another module (where the list of permissions changes as a result of the new module is being installed).

If I remember right, the main example that actually has a noticeable effect is that turning on the PHP module does not grant permission to use the new "PHP code" text format to the administrator (since that permission is provided by Filter module). So if you turn on the PHP module as an admin and expect to be able to immediately create a node and use the PHP text format just like user 1 can, you can't because you were never granted permission to use it.

Looks like this is in the wrong component too... finger slip somewhere? :)

Updated issue summary.

More that I often forget to fill out the component because there's no - None - option, and I think either my browser or dreditor just uses the component from the last issue I filed, or something. :) Working on the test now.

So... suddenly, somehow, I can't reproduce this locally in either 8.x or 7.x. Bit baffled. It was reproducible on a different machine yesterday. (In fact, there's also already test coverage for this exact behavior in UserPermissionsTestCase::testAdministratorRole().

Going to try to confirm on the other machine on Monday (and check if that test fails there). I haven't been able to figure out what the missing link is.

Edit: I also noticed another, unrelated "unexpected behavior" for the administrative role functionality in the process of testing--when you make a new role the administrative role, that role also doesn't get any of the existing permissions.

I am new to contributing to issue queue. I was attempting to help out with this issue: #1153072: Admin role should always provide all permissions (create a superuser role) (alternate feature proposed for D8) However, I am not sure what the intended behavior should be.

I just noticed this and it might help to be aware of this setting when thinking about this problem:
In Home » Administration » Configuration » People >> Account Settings
exists a section called ADMINISTRATOR ROLE where you can select: disabled, Administrator, or one of the custom roles.

The description says, "This role will be automatically assigned new permissions whenever a module is enabled. Changing this setting will not affect existing permissions."

Here's a test for this condition pre-patch and also a patch with the test and the above fix. joestewart, barnettech and tim.plunkett also worked on this at the drupalcon core code sprint.

Awesome!! Thank you sprinters! The test logic looks good to me. It looks like the bot was stuck, so I requeued both patches on QA.

Meanwhile, a few outstanding things with the patch:

1. +++ b/core/modules/user/user.api.phpundefined
   @@ -463,5 +463,17 @@ function hook_user_role_delete($role) {
    /**
   + * @todo Document this.
   + */
   +function hook_user_permissions_insert($permissions) {
   +}
   +
   +/**
   + * @todo Document this.
   + */
   +function hook_user_permissions_delete($permissions) {
   +}
   
   +++ b/core/modules/user/user.moduleundefined
   @@ -3067,6 +3067,51 @@ function user_role_revoke_permissions($rid, array $permissions = array()) {
   + * @todo: Change documentation since modules don't always have to call it.
   ...
   + * @todo: Change documentation, or make all core modules actually call it.
   

   We still need to add the hook documentation and address these @todo.

2. +++ b/core/modules/user/user.moduleundefined
   @@ -3067,6 +3067,51 @@ function user_role_revoke_permissions($rid, array $permissions = array()) {
   + * Removes no longer available user permissions from the site.
   

   This is a little confusing; perhaps:
   Removes user permissions that are no longer available.

3. +++ b/core/modules/user/user.moduleundefined
   @@ -3823,9 +3878,15 @@ function user_modules_installed($modules) {
   +  // @todo: This will skip any permissions that are not in the database (i.e.,
   +  //   that are not assigned to any roles). This doesn't matter for our
   +  //   purposes, but in theory it's a problem since user_permissions_delete()
   +  //   invokes a hook which should get the full list of "deleted" permissions.
   

   What are the full implications of this?

4. +++ b/core/modules/user/user.testundefined
   @@ -1152,8 +1152,12 @@ class UserPermissionsTestCase extends DrupalWebTestCase {
   +    // enable book
   ...
   +    // test general node permission for enabled book module
   

   These should be capitalized and begin with a period. Reference: http://drupal.org/node/1354#inline

5. +++ b/core/modules/user/user.testundefined
   @@ -1152,8 +1152,12 @@ class UserPermissionsTestCase extends DrupalWebTestCase {
   +    $this->assertTrue(user_access('edit own book content', $this->admin_user), t('The permission for node access was assigned to the administrator role.'));
   

   We can leave the t() off the assertion message here. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

I'll be working on cleaning this up!

I cleaned this help, but it still needs work in two places.

In comment #13,
#1

+  // Grant any new permissions to the administration role.
+  $rid = variable_get('user_admin_role', 0);
+  if ($rid && !empty($permissions)) {
+    user_role_grant_permissions($rid, $permissions);
+  }
+}
+
+/**
+ * Removes user permissions that are no longer available.
+ *
+ * Modules should call this function when the list of permissions that the
+ * module provides has changed (for example, a dynamic permission has
+ * disappeared).
+ *
+ * @todo: Change documentation, or make all core modules actually call it.

Didn't quite know what to do - someone make a decision! :)

#3

+++ b/core/modules/user/user.moduleundefined
@@ -3823,9 +3878,15 @@ function user_modules_installed($modules) {
+  // @todo: This will skip any permissions that are not in the database (i.e.,
+  //   that are not assigned to any roles). This doesn't matter for our
+  //   purposes, but in theory it's a problem since user_permissions_delete()
+  //   invokes a hook which should get the full list of "deleted" permissions.

I'm not sure what the full implications are - can someone answer?

Everything else *should* be fixed (or I attempted to). Please look and edit as needed.

Blah, forgot to set status.

Rerolled, but:

patch: **** malformed patch at line 34: /**

Can someone take a look? I can't seem to figure what the issue is...

Reroll from 12 - let's try this again..

Again...

Questions in #15 still apply. These particular changes were not made.

Interdiff (maybe)?

So the patch in #22 added + signs before a bunch of lines.

So I rerolled it, and here's an interdiff against #12.

#24: drupal-1454686-24.patch queued for re-testing.

I'm just rerolling this to get it to apply, but it doesn't seem to make sense anymore, since between the patch in #1 and the next patch in #12, the new hook_modules_to_be_installed() invocation was removed...

Is this still a major bug?

Is this still a major bug?

I don't think so, but happy to be overruled... Lowering priority for now. In practice, I've rarely ever seen a situation where this mattered myself; usually it's just a missing checkbox on the permissions page but one that doesn't matter anyway because it's covered by some other permission that you already have.

The problem I've actually seen more often myself is #3 from the issue summary ("Permissions introduced in a module update are not granted to the admin role") but that's covered by a different issue, not this one.

Oh, I lied, #3 actually is covered (at least partially) by the current patch, since it provides a user_permissions_insert() function that can be used for this purpose.

Updated issue summary.

Removing myself from the author field so that I can unfollow the issue. --xjm

This seems fixed by #2435075: Implement admin role as a flag on the role storage, simplify permissions page, remove user_modules_installed. Please reopen it if you think I'm wrong.