Turn role permission assignments into configuration.

That doesn't block this. I've seen Drupal install and the Field UI upgrade test pass. If that test passes... note: I have made some cleanups that could've been done otherwise, mostly to use API functions where they weren't. Also, I brutally simplified a Views filter, left a message to Daniel to verify it is indeed unnecessary code.

While it doesn't quite block, it is quite tedious :)

After a discussion with dawehner: a) we need a filter for enabled permissions cos only uninstalled permissions are removed -- but the list of permissions retrieved by module_invoke_all includes only the permissions by enabled modules so we don't need to consult the other config file which is slated for removal anyways b) we will want to move $rid to the config object name from the top key so that we have one object per rid. Next revision will include a) and once that's passed I will get to b).

Started to work on tests for the filter and field, which clearly shows how many issues we have with the filter.

This splits the config objects per role id because -- config('foo')->get() is always an array but config('foo')->get('something') can be NULL and PHP doesnt like a NULL when it expects an array. Huge props to dawehner for making the Views user test coverage more solid.

One more roll, the modules-permissions is moved out of the user.permission. namespace to avoid confusion. The commented out Views tests have a todo , dawehner will file a followup and get 'em fixed. This should be ready.

Two line breaks! Oh dear. Quick, let's fix it before the universe unravels in the face of such a horror.

I am grateful for the review but I couldn't resist.

Keeping up with HEAD. As interdiff is not possible, I attached a direct diff of the two patches.

fubhy points out that while update_rename_permissions is now update_replace_permissions the variable and the doxygen didnt'f wolow. As you can see, replacing letters is easy and cheap :)

Grr!

To clarify the last few rerolls: during the commitfest another installSchema call got added. I needed to remove that. But, that installSchema does not get a list of tables to install but the name of the module and then either a string or an array is a DrupalWTF. And, has absolutely nothing to do with the substrance of this patch and so the RTBC is correct.

#22: 1872876_22.patch queued for re-testing.

It seems like the HandlerFilterPermissionTest is unfinished

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,122 @@
+    // Filter by a non existing permission with operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('Non existent permission');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), count($this->users), 'A non existent permission with the NOt operator should result in every user');
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $expected[] = array('uid' => 3);
+    // $expected[] = array('uid' => 4);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,122 @@
+    // Filter by a permission with operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('administer permissions');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), 2);
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,122 @@
+    // Filter by another permission of a role with multiple permissions and
+    // operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('administer users');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), 3);
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $expected[] = array('uid' => 3);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,122 @@
+    // @todo Test the available UI options.

Opened another issue for the @todo in the test: #1959296: Fix the not operation in the many to one handling.

That's just a rerole.

Added tests for the UI part.

Rerolled.

It's actually good to see that we got more and more tests over time.

I don't really understand the todo here:

+  // @TODO: remove.
+  $modules = user_permission_get_modules();
+  $config = Drupal::config('user.module.permission');
+  foreach ($modules as $permission => $module) {
+    $config->set("$module.$permission", TRUE);
   }
+  $config->save();

I hope that chx can enlighten us.

This is just a list of small cleanups/adaptions to new things in drupal.

That whole permissions-by-module stuff is just for uninstall. Die, die, die! I hope we can remove it at one point. So that's what the todo is: remove if we can.

Another one bites the dust!
@dawehner++ @chx++

We agree that this @todo is not helpful there, so let's drop it.

This requires follow-up to be filed.

+++ b/core/modules/user/lib/Drupal/user/Plugin/views/field/Permissions.phpundefined
@@ -40,32 +40,24 @@ function pre_render(&$values) {
+      $permission_names = \Drupal::moduleHandler()->invokeAll('permission');
+      $query = db_select('users_roles', 'u');
...
+      foreach ($result as $row) {
+        foreach (\Drupal::config("user.permission.$row->rid")->get() as $permission) {
+          $this->items[$row->uid][$permission]['permission'] = $permission_names[$permission]['title'];

Maybe better to implement helper in follow-up. Seems hook execution for for each field is too expensive without caching

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,131 @@
+   * @todo Fix the different commented out tests by fixing the many to one
+   *   handler handling with the NOT operator.
...
+    // Filter by a non existing permission with operator NOT.
+    // $view->initHandlers();
...
+    // Filter by a permission with operator NOT.
+    // $view->initHandlers();
...
+    // Filter by another permission of a role with multiple permissions and
+    // operator NOT.
+    // $view->initHandlers();

Is there a reason to not make a follow up and add link here for @todo

Thanks for your comment.

Maybe better to implement helper in follow-up. Seems hook execution for for each field is too expensive without caching

This just happens once for the full result. I don't think that this is a performance issue we have to tackle.

Is there a reason to not make a follow up and add link here for @todo

There is already a follow up for that: #1959296: Fix the not operation in the many to one handling.

From a conversation on IRC.

alexpott:
just wondering why the info contained in user.permission.anonymous.yml is not just an array on user.role.anonymous.yml. If I want to deploy a role - to me that's always going to include its permissions… and the relationship between these files will always be 1 to 1
dawehner:
that is a good point. as far as I know one goal to the current approach is to avoid using the user roles config entity
alexpott:
gotta ask… why?
dawehner:
i'm not 100% sure, but I think it's considered as a bad practice to load the config files directly for config entities... then loading the entity might be too much overhead
alexpott:
hmmm... it seems a shame

There doesn't seem to be be any explanation on the issue as to why adding a permission property to the role config entity was not considered.

Putting the issue at needs review cause I want to know why we're making it more difficult to deploy a role we have move three files instead of just two... (remember the manifest :) )

Tagging

The interdiff is against #34 because that's the last reviewed patch. The fix wasn't too hard.

Hopefully this is the last. Still interdiff against #34.

Opsie. I suspect this doesn't have test coverage.

Looks awesome! +1 to RTBC

NW for user.schema and let's not commit dead code

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/Role.phpundefined
@@ -65,6 +65,13 @@ class Role extends ConfigEntityBase implements RoleInterface {
+   * The permissions belonging to this role.
...
+  public $permissions = array();

/user/config/schema/user/schema.yml needs update too

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFilterPermissionTest.phpundefined
@@ -0,0 +1,131 @@
+    // Filter by a non existing permission with operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('Non existent permission');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), count($this->users), 'A non existent permission with the NOt operator should result in every user');
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $expected[] = array('uid' => 3);
+    // $expected[] = array('uid' => 4);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();
...
+    // Filter by a permission with operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('administer permissions');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), 2);
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();
...
+    // Filter by another permission of a role with multiple permissions and
+    // operator NOT.
+    // $view->initHandlers();
+    // $view->filter['permission']->value = array('administer users');
+    // $view->filter['permission']->operator = 'not';
+    // $this->executeView($view);
+    // $this->assertEqual(count($view->result), 3);
+    // $expected = array();
+    // $expected[] = array('uid' => 1);
+    // $expected[] = array('uid' => 2);
+    // $expected[] = array('uid' => 3);
+    // $this->assertIdenticalResultset($view, $expected, $column_map);
+    // $view->destroy();

do we really need this code without @todo and link?

Views tests are by dawehner and I have nothing to do with them so I just removed them so this can get in. Added schema.

should be green!

Thanks.

Maybe easier to understand this way.

Sigh.

Back to RTBC

We're nearly there...

+++ b/core/modules/system/system.installundefined
@@ -1521,13 +1521,12 @@ function system_update_8020() {
+    config('user.module.permission')
+      ->set('ban.ban IP addresses', TRUE)
+      ->save();

+++ b/core/modules/user/user.moduleundefined
@@ -1982,23 +1989,20 @@ function user_role_change_permissions($rid, array $permissions = array()) {
+  $modules = user_permission_get_modules();
+  $config = Drupal::config('user.module.permission');
+  foreach ($modules as $permission => $module) {
+    $config->set("$module.$permission", TRUE);
   }
+  $config->save();

@@ -2635,11 +2632,25 @@ function user_modules_installed($modules) {
 function user_modules_uninstalled($modules) {
-   db_delete('role_permission')
-     ->condition('module', $modules, 'IN')
-     ->execute();
+  $config = Drupal::config('user.module.permission');
+  $permissions_to_remove = array();
+  foreach ($modules as $module) {
+    $permissions_to_remove += (array) $config->get($module);
+    $config->clear($module);
+  }
+  $config->save();
+  if ($permissions_to_remove) {
+    $permissions_to_remove = array_keys($permissions_to_remove);
+    foreach (user_roles() as $rid => $role_entity) {
+      $role_entity->permissions = array_diff($role_entity->permissions, $permissions_to_remove);
+      $role_entity->save();
+    }
+  }

I think we can do something different in user_modules_uninstalled(). We should either move the info in user.module.permission to state OR (and I like preferably)... just get all the permissions by invoking hook_perm and then removing all the ones that don't exist from each role. What's really nice is that the snippet from system_update_8020() becomes completely unnecessary... as this is the only place where we've actually taken care of this information during upgrade.

+++ b/core/modules/user/lib/Drupal/user/RoleStorageController.phpundefined
@@ -34,25 +34,30 @@ public function preSave(EntityInterface $entity) {
+    $prefix = 'user.permission.';
+    if ($ids) {
+      $names = array();
+      foreach ($ids as $id) {
+        $names[] = $prefix . $id;
+      }
+    }
+    else {
+      $names = \Drupal::service('config.storage')->listAll($prefix);
+    }
+    foreach ($names as $name) {
+      config($name)->init();
+    }

This code is unnecessary now that permissions are stored in the role configuration entity.

Agreed, right now we do not have a better choice so this revision only changes RoleStorageController

#62: 1872876_62.patch queued for re-testing.

Sigh. bot flukes.

Came up with a solution that does not use an additionally config file (which is not config) and does not use state :)...

Basically in the role entity permissions are stored as an array keyed by the permission name and with a value of the implementing module...

This patch also fixes a bug on admin/people where in #62 and before you'll get an sql error is you select a permission no one has...

Removed @todo I introduced... and removed some british optimisation :)

Also some performance testing...

Without patch

With patch

Fixed comments indexing perms...

Okay so here is what's better about #70...

1. The additional config file user.module.permission should never be deployed
2. user.module.permission's upgrade path was not complete... afaics the only module's perms that would have been in here after a 7.x to 8.x upgrade would have been ban's if you were using the blocked ip feature otherwise this file would not even exist although you would have roles with permissions....
3. Leading on from (2) user.module.permission's maintenance is completely untested
4. In #70 keying by permissions means that we can use isset() rather than in_array() for all of our lookups on the permissions array http://stackoverflow.com/questions/13483219/what-is-faster-in-array-or-i... (apart from when uninstalling modules but who cares about the performance there)...
5. Since http://buytaert.net/reducing-risk-in-the-drupal-8-release-schedule we really should not be committing patches with an any critical followups.

So in summary: #70 is complete, has upgrade path (because there is no additional storage of info), better tested (less new stuff), and more performant.

Noticed a bug... obviously we're missing some views test coverage :)

More british optimisation replaced with true Drupal optimization!

Assigning to catch for commit/review

+++ b/core/includes/update.incundefined
@@ -1597,3 +1597,35 @@ function update_add_cache_columns($table) {
+ * @param array $replace
+ *   An associative array. The keys are the old permissions the values are
+ *   an array keyed by new permission with a value of the module that implements
+ *   that permission. If the list is an empty array, the old permission is

Could we maybe add a @code example here?

+++ b/core/modules/comment/comment.moduleundefined
@@ -1211,18 +1211,21 @@ function comment_node_update_index(EntityInterface $node, $langcode) {
+    // comments is not granted by the 'authenticated user' role. In this case
+    // all users might have both permissions from various roles but it is also
+    // possible to set up a user to have only search content and so a user
+    // edit could change the security situation so it is not safe to index the

The last sentence is very long, "and so.." onwards could be a new sentence maybe.

+++ b/core/modules/node/node.installundefined
@@ -479,6 +479,15 @@ function _update_7000_node_get_types() {
 /**
+ * Implements hook_update_dependency().
+ */
+function node_update_dependency() {
+  $dependencies['node'][8013] = array(
+    'user' => 8002,
+  );
+}
+
+/**
  * Rename node type language variable names.
  *
  * @see http://drupal.org/node/540294
@@ -704,15 +713,11 @@ function node_update_8012() {

@@ -704,15 +713,11 @@ function node_update_8012() {
  * Renames global revision permissions to use the word 'all'.
  */
 function node_update_8013() {
-  $actions = array('view', 'delete', 'revert');
-
-  foreach ($actions as $action) {
-    db_update('role_permission')
-      ->fields(array('permission' => $action . ' all revisions'))
-      ->condition('permission', $action . ' revisions')
-      ->condition('module', 'node')
-      ->execute();
-  }
+  update_replace_permissions(array(
+    'view revisions' => array('view all revisions' => 'node'),
+    'revert revisions' => array('revert all revisions' => 'node'),
+    'delete revisions' => array('delete all revisions' => 'node'),
+  ));
 }

I'm not sure about these retrospective updates, mainly due to the horrors of the Drupal 7 upgrade path, but in this case it's a tiny amount of data, we're still pretty far off supporting head to head updates, and saves doing the work twice, so it's probably fine.

+++ b/core/modules/node/node.views_execution.incundefined
@@ -30,17 +30,9 @@ function node_views_analyze(ViewExecutable $view) {
+          $anonymous_has_access = isset(entity_load('user_role', DRUPAL_ANONYMOUS_RID)->permissions['access content']);
+          $authenticated_has_access = isset(entity_load('user_role', DRUPAL_AUTHENTICATED_RID)->permissions['access content']);

Both the existing code and the new could use user_role_permissions() (or whatever that changes to if this patch removes it).

+++ b/core/modules/user/config/user.role.anonymous.ymlundefined
@@ -1,4 +1,6 @@
+permissions:
+  'access content': node

The module key shouldn't be necessary in the yml. This originally got added because of disabled modules which there's a critical issue to remove. module/permission relationships if they have to be kept for now could go in state or somewhere but there's no way this is config much less for individual assignments.

+++ b/core/modules/user/user.moduleundefined
@@ -400,44 +399,53 @@ function user_password($length = 10) {
+/**
+ * Determine the permissions for one or more roles during update.
+ *
+ * A separate version is needed because during update the entity system can't
+ * be used and in non-update situations the entity system is preferred because
+ * of the hook system.
+ *
+ * @param array $roles
+ *   An array whose keys are the role IDs of interest, such as $user->roles.
+ *
+ * @return array
+ *   An array indexed by role ID. Each value is an array whose keys are the
+ *   permission strings for the given role ID.
+ */
+function _user_role_permissions_update($roles) {
+  $role_permissions = array();
+  foreach ($roles as $rid => $name) {
+    $role_permissions[$rid] = array();
+    $permissions = Drupal::config("user.role.$rid")->get('permissions') ?: array();
+    foreach ($permissions as $permission => $module) {
+      $role_permissions[$rid][$permission] = TRUE;
     }

If this is for updates only shouldn't it be in user.install? We shouldn't be using the config API in update situations either but there's already an issue for that and it's not introduced here.

user_role_grant_permissions() doesn't require a module name so it's a regression to have to specify that.

I've moved the module - permissions info into state :)

Also implemented a has() method on the role to make the in_array nicer.

Note that as of #1199946-148: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed Dries has stated he is OK with removing the disabled module state. This work is progressing at #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair. So it appears we can remove all the disable-specific code from this patch and move on.

Moving to catch so we can get an opinion on how to continue with this.

we discussed this on irc and we will go ahead w/o cleanup for now

Fixed the code/tests

#108: user_permissions_config-1872876-106.patch queued for re-testing.

Walked through patch and found no problems. RTBC?

+++ b/core/modules/user/lib/Drupal/user/RoleInterface.php
@@ -14,4 +14,45 @@
+  public function getPermissions();
...
+  public function hasPermission($permission);
...
+  public function grantPermission($permission);
...
+  public function revokePermission($permission);

Great!

+++ b/core/modules/comment/comment.moduleundefined
@@ -1223,20 +1223,24 @@ function comment_node_update_index(EntityInterface $node, $langcode) {
+    $roles = Drupal::entityManager()->getStorageController('user_role')->load();
+    $authenticated_can_access = $roles[DRUPAL_AUTHENTICATED_RID]->hasPermission('access comments');
+    foreach ($roles as $rid => $role) {
+      if ($role->hasPermission('search content')) {
+        if (!$role->hasPermission('access comments') && ($rid == DRUPAL_AUTHENTICATED_RID || $rid == DRUPAL_ANONYMOUS_RID || !$authenticated_can_access)) {
+          $index_comments = FALSE;
+          break;
+        }
       }
     }

This would be even sweeter if it looked like this...

    $roles = Drupal::entityManager()->getStorageController('user_role')->load();
    $authenticated_cannot_access = $roles[DRUPAL_AUTHENTICATED_RID]->cannot('access comments');
    foreach ($roles as $rid => $role) {
      if ($role->can('search content') && $role->cannot('access comments')) {
        if ($rid == DRUPAL_AUTHENTICATED_RID || $rid == DRUPAL_ANONYMOUS_RID || $authenticated_cannot_access) {
          $index_comments = FALSE;
          break;
        }
      }
    }

Even if you don't change hasPermission() to can() and implement cannot() I think we separate the if's as above as it makes it easier to correlate with the big comment above!

Now it much more readable.

+++ b/core/modules/node/node.views_execution.incundefined
@@ -30,17 +30,11 @@ function node_views_analyze(ViewExecutable $view) {
+          $anonmyous_role = entity_load('user_role', DRUPAL_ANONYMOUS_RID);

anonmyous

Can we open a critical, postponed, follow-up for the uninstall cleanup? Otherwise this looks ready.

Updated issue summary.

Added followup to issue summary add rerolled for

+++ b/core/modules/node/node.views_execution.incundefined
@@ -30,17 +30,11 @@ function node_views_analyze(ViewExecutable $view) {
+          $anonmyous_role = entity_load('user_role', DRUPAL_ANONYMOUS_RID);
+          $anonymous_has_access = $anonymous_role && $anonmyous_role->hasPermission('access content');

Actually a bug...

Committed/pushed to 8.x, thanks!

Can we get a change notice here.

update tags (normalize to "Needs change notification")

Looks like the change notice was posted.

Update