- Patch #73874 by pwolanin: normalize the permissions table and wrote...

- Patch #73874 by pwolanin: normalize the permissions table and wrote simpletests for the (new) permission handling.  At last.

modules/simpletest/drupal_web_test_case.php

@@ -273,9 +273,7 @@ function drupalCreateUser($permissions = NULL) {
   private function _drupalCreateRole($permissions = NULL) {
     // Generate string version of permissions list.
     if ($permissions === NULL) {
-      $permission_string = 'access comments, access content, post comments, post comments without approval';
-    } else {
-      $permission_string = implode(', ', $permissions);
+      $permissions = array('access comments', 'access content', 'post comments', 'post comments without approval');
     }
 
     // Create new role.
@@ -285,8 +283,11 @@ private function _drupalCreateRole($permissions = NULL) {
     $this->assertTrue($role, t('Created role of name: @role_name, id: @rid', array('@role_name' => $role_name, '@rid' => (isset($role->rid) ? $role->rid : t('-n/a-')))), t('Role'));
     if ($role && !empty($role->rid)) {
       // Assign permissions to role and mark it for clean-up.
-      db_query("INSERT INTO {permission} (rid, perm) VALUES (%d, '%s')", $role->rid, $permission_string);
-      $this->assertTrue(db_affected_rows(), t('Created permissions: @perms', array('@perms' => $permission_string)), t('Role'));
+      foreach ($permissions as $permission_string) {
+        db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", $role->rid, $permission_string);
+      }
+      $count = db_result(db_query("SELECT COUNT(*) FROM {role_permission} WHERE rid = %d", $role->rid));
+      $this->assertTrue($count == count($permissions), t('Created permissions: @perms', array('@perms' => implode(', ', $permissions))), t('Role'));
       return $role->rid;
     }
     else {

modules/system/system.install

@@ -366,11 +366,18 @@ function system_install() {
   // This sets uid 1 (superuser). We skip uid 2 but that's not a big problem.
   db_query("UPDATE {users} SET uid = 1 WHERE name = '%s'", 'placeholder-for-uid-1');
 
+  // Built-in roles.
   db_query("INSERT INTO {role} (name) VALUES ('%s')", 'anonymous user');
   db_query("INSERT INTO {role} (name) VALUES ('%s')", 'authenticated user');
 
-  db_query("INSERT INTO {permission} (rid, perm, tid) VALUES (%d, '%s', %d)", 1, 'access content', 0);
-  db_query("INSERT INTO {permission} (rid, perm, tid) VALUES (%d, '%s', %d)", 2, 'access comments, access content, post comments, post comments without approval', 0);
+  // Anonymous role permissions.
+  db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", 1, 'access content');
+
+  // Authenticated role permissions.
+  db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", 2, 'access comments');
+  db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", 2, 'access content');
+  db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", 2, 'post comments');
+  db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", 2, 'post comments without approval');
 
   db_query("INSERT INTO {variable} (name, value) VALUES ('%s', '%s')", 'theme_default', 's:7:"garland";');
   db_query("UPDATE {system} SET status = %d WHERE type = '%s' AND name = '%s'", 1, 'theme', 'garland');
@@ -2944,6 +2951,51 @@ function system_update_7006() {
   return $ret;
 }
 
+/**
+ * Convert to new method of storing permissions.
+ *
+ * This update is in system.install rather than user.install so that
+ * all modules can use the updated permission scheme during their updates.
+ */
+function system_update_7007() {
+  $ret = array();
+
+  $schema['role_permission'] = array(
+    'fields' => array(
+      'rid' => array(
+        'type' => 'int',
+        'unsigned' => TRUE,
+        'not null' => TRUE,
+      ),
+      'permission' => array(
+        'type' => 'varchar',
+        'length' => 64,
+        'not null' => TRUE,
+        'default' => '',
+      ),
+    ),
+    'primary key' => array('rid', 'permission'),
+    'indexes' => array(
+      'permission' => array('permission'),
+    ),
+  );
+
+  db_create_table($ret, 'role_permission', $schema['role_permission']);
+
+  // Copy the permissions from the old {permission} table to the new {role_permission} table.
+  $result = db_query("SELECT rid, perm FROM {permission} ORDER BY rid ASC");
+  while ($role = db_fetch_object($result)) {
+    foreach (explode(', ', $role->perm) as $perm) {
+      db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", $role->rid, $perm);
+    }
+    $ret[] = array('success' => TRUE, 'query' => "Inserted into {role_permission} the permissions for role ID " . $role->rid);
+  }
+  db_drop_table($ret, 'permission');
+
+  return $ret;
+}
+
+
 /**
  * @} End of "defgroup updates-6.x-to-7.x"
  * The next series of updates should start at 8000.

modules/user/user.admin.inc

@@ -493,27 +493,20 @@ function user_admin_settings() {
  * @see theme_user_admin_perm()
  */
 function user_admin_perm($form_state, $rid = NULL) {
-  if (is_numeric($rid)) {
-    $result = db_query('SELECT r.rid, p.perm FROM {role} r LEFT JOIN {permission} p ON r.rid = p.rid WHERE r.rid = %d', $rid);
-  }
-  else {
-    $result = db_query('SELECT r.rid, p.perm FROM {role} r LEFT JOIN {permission} p ON r.rid = p.rid ORDER BY name');
-  }
-
-  // Compile role array:
-  // Add a comma at the end so when searching for a permission, we can
-  // always search for "$perm," to make sure we do not confuse
-  // permissions that are substrings of each other.
-  while ($role = db_fetch_object($result)) {
-    $role_permissions[$role->rid] = $role->perm . ',';
-  }
 
   // Retrieve role names for columns.
   $role_names = user_roles();
   if (is_numeric($rid)) {
     $role_names = array($rid => $role_names[$rid]);
   }
+  // Fetch permissions for all roles or the one selected role.
+  $role_permissions = user_role_permissions($role_names);
 
+  // Store $role_names for use when saving the data.
+  $form['role_names'] = array(
+    '#type' => 'value',
+    '#value' => $role_names,
+  );
   // Render role/permission overview:
   $options = array();
   $hide_descriptions = !system_admin_compact_mode();
@@ -537,7 +530,7 @@ function user_admin_perm($form_state, $rid = NULL) {
         );
         foreach ($role_names as $rid => $name) {
           // Builds arrays for checked boxes for each role
-          if (strpos($role_permissions[$rid], $perm . ',') !== FALSE) {
+          if (isset($role_permissions[$rid][$perm])) {
             $status[$rid][] = $perm;
           }
         }
@@ -555,24 +548,24 @@ function user_admin_perm($form_state, $rid = NULL) {
   return $form;
 }
 
+/**
+ * Save permissions selected on the administer permissions page.
+ *
+ * @see user_admin_perm
+ */
 function user_admin_perm_submit($form, &$form_state) {
-  // Save permissions:
-  $result = db_query('SELECT * FROM {role}');
-  while ($role = db_fetch_object($result)) {
-    if (isset($form_state['values'][$role->rid])) {
-      // Delete, so if we clear every checkbox we reset that role;
-      // otherwise permissions are active and denied everywhere.
-      db_query('DELETE FROM {permission} WHERE rid = %d', $role->rid);
-      $form_state['values'][$role->rid] = array_filter($form_state['values'][$role->rid]);
-      if (count($form_state['values'][$role->rid])) {
-        db_query("INSERT INTO {permission} (rid, perm) VALUES (%d, '%s')", $role->rid, implode(', ', array_keys($form_state['values'][$role->rid])));
-      }
+  foreach ($form_state['values']['role_names'] as $rid => $name) {
+    $checked = array_filter($form_state['values'][$rid]);
+    // Delete existing permissions for the role. This handles "unchecking" checkboxes.
+    db_query("DELETE FROM {role_permission} WHERE rid = %d", $rid);
+    foreach ($checked as $permission) {
+      db_query("INSERT INTO {role_permission} (rid, permission) VALUES (%d, '%s')", $rid, $permission);
     }
   }
 
   drupal_set_message(t('The changes have been saved.'));
 
-  // Clear the cached pages
+  // Clear the cached pages and blocks.
   cache_clear_all();
 }
 
@@ -697,7 +690,7 @@ function user_admin_role_submit($form, &$form_state) {
   }
   else if ($form_state['values']['op'] == t('Delete role')) {
     db_query('DELETE FROM {role} WHERE rid = %d', $form_state['values']['rid']);
-    db_query('DELETE FROM {permission} WHERE rid = %d', $form_state['values']['rid']);
+    db_query('DELETE FROM {role_permission} WHERE rid = %d', $form_state['values']['rid']);
     // Update the users who have this role set:
     db_query('DELETE FROM {users_roles} WHERE rid = %d', $form_state['values']['rid']);
 

modules/user/user.install

@@ -41,38 +41,26 @@ function user_schema() {
     'primary key' => array('aid'),
   );
 
-  $schema['permission'] = array(
-    'description' => t('Stores permissions for users.'),
+  $schema['role_permission'] = array(
+    'description' => t('Stores the permissions assigned to user roles.'),
     'fields' => array(
-      'pid' => array(
-        'type' => 'serial',
-        'not null' => TRUE,
-        'description' => t('Primary Key: Unique permission ID.'),
-      ),
       'rid' => array(
         'type' => 'int',
         'unsigned' => TRUE,
         'not null' => TRUE,
-        'default' => 0,
-        'description' => t('The {role}.rid to which the permissions are assigned.'),
-      ),
-      'perm' => array(
-        'type' => 'text',
-        'not null' => FALSE,
-        'size' => 'big',
-        'description' => t('List of permissions being assigned.'),
+        'description' => t('Foreign Key: {role}.rid.'),
       ),
-      'tid' => array(
-        'type' => 'int',
-        'unsigned' => TRUE,
+      'permission' => array(
+        'type' => 'varchar',
+        'length' => 64,
         'not null' => TRUE,
-        'default' => 0,
-        'description' => t('Originally intended for taxonomy-based permissions, but never used.'),
+        'default' => '',
+        'description' => t('A single permission granted to the role identified by rid.'),
       ),
     ),
-    'primary key' => array('pid'),
+    'primary key' => array('rid', 'permission'),
     'indexes' => array(
-      'rid' => array('rid'),
+      'permission' => array('permission'),
     ),
   );
 
@@ -83,7 +71,7 @@ function user_schema() {
         'type' => 'serial',
         'unsigned' => TRUE,
         'not null' => TRUE,
-        'description' => t('Primary Key: Unique role id.'),
+        'description' => t('Primary Key: Unique role ID.'),
       ),
       'name' => array(
         'type' => 'varchar',

modules/user/user.module

@@ -433,6 +433,59 @@ function user_password($length = 10) {
   return $pass;
 }
 
+/**
+ * Determine the permissions for one or more roles.
+ *
+ * @param $roles
+ *   An array whose keys are the role IDs of interest, such as $user->roles.
+ * @param $reset
+ *   Optional parameter - if TRUE data in the static variable is rebuilt.
+ *
+ * @return
+ *   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($roles = array(), $reset = FALSE) {
+  static $stored_permissions = array();
+
+  if ($reset) {
+    // Clear the data cached in the static variable.
+    $stored_permissions = array();
+  }
+
+  $role_permissions = $fetch = array();
+
+  if ($roles) {
+    foreach ($roles as $rid => $name) {
+      if (isset($stored_permissions[$rid])) {
+        $role_permissions[$rid] = $stored_permissions[$rid];
+      }
+      else {
+        // Add this rid to the list of those needing to be fetched.
+        $fetch[] = $rid;
+        // Prepare in case no permissions are returned.
+        $stored_permissions[$rid] = array();
+      }
+    }
+
+    if ($fetch) {
+      // Get from the database permissions that were not in the static variable.
+      // Only role IDs with at least one permission assigned will return rows.
+      $result = db_query("SELECT r.rid, p.permission FROM {role} r INNER JOIN {role_permission} p ON p.rid = r.rid WHERE r.rid IN (" . db_placeholders($fetch) . ")", $fetch);
+
+      while ($row = db_fetch_array($result)) {
+        $stored_permissions[$row['rid']][$row['permission']] = TRUE;
+      }
+      foreach ($fetch as $rid) {
+        // For every rid, we know we at least assigned an empty array.
+        $role_permissions[$rid] = $stored_permissions[$rid];
+      }
+    }
+  }
+
+  return $role_permissions;
+}
+
 /**
  * Determine whether the user has a given privilege.
  *
@@ -472,11 +525,11 @@ function user_access($string, $account = NULL, $reset = FALSE) {
   // To reduce the number of SQL queries, we cache the user's permissions
   // in a static variable.
   if (!isset($perm[$account->uid])) {
-    $result = db_query("SELECT p.perm FROM {role} r INNER JOIN {permission} p ON p.rid = r.rid WHERE r.rid IN (" . db_placeholders($account->roles) . ")", array_keys($account->roles));
+    $role_permissions = user_role_permissions($account->roles, $reset);
 
     $perms = array();
-    while ($row = db_fetch_object($result)) {
-      $perms += array_flip(explode(', ', $row->perm));
+    foreach ($role_permissions as $one_role) {
+      $perms += $one_role;
     }
     $perm[$account->uid] = $perms;
   }
@@ -1590,7 +1643,7 @@ function user_roles($membersonly = FALSE, $permission = NULL) {
   );
 
   if (!empty($permission)) {
-    $result = db_query("SELECT r.* FROM {role} r INNER JOIN {permission} p ON r.rid = p.rid WHERE p.perm LIKE '%%%s%%' ORDER BY r.name", $permission);
+    $result = db_query("SELECT r.* FROM {role} r INNER JOIN {role_permission} p ON r.rid = p.rid WHERE p.permission = '%s' ORDER BY r.name", $permission);
   }
   else {
     $result = db_query('SELECT * FROM {role} ORDER BY name');
@@ -1854,8 +1907,8 @@ function user_filters() {
   ksort($options);
   $filters['permission'] = array(
     'title' => t('permission'),
-    'join' => 'LEFT JOIN {permission} p ON ur.rid = p.rid',
-    'where' => " ((p.perm IS NOT NULL AND p.perm LIKE '%%%s%%') OR u.uid = 1) ",
+    'join' => 'LEFT JOIN {role_permission} p ON ur.rid = p.rid',
+    'where' => " (p.permission = '%s' OR u.uid = 1) ",
     'options' => $options,
   );
 

modules/user/user.test

@@ -207,3 +207,57 @@ class UserDeleteTestCase extends DrupalWebTestCase {
     $this->assertFalse(user_load($edit), t('User is not found in the database'));
   }
 }
+
+
+class UserPermissionsTestCase extends DrupalWebTestCase {
+  protected $admin_user;
+  protected $rid;
+
+  /**
+   * Implementation of getInfo().
+   */
+  function getInfo() {
+    return array(
+      'name' => t('Role permissions'),
+      'description' => t('Verify that role permissions can be added and removed via the permissions page.'),
+      'group' => t('User')
+    );
+  }
+
+  function setUp() {
+    parent::setUp();
+
+    $this->admin_user = $this->drupalCreateUser(array('administer permissions', 'access user profiles'));
+
+    // Find the new role ID - it must be the maximum.
+    $all_rids = array_keys($this->admin_user->roles);
+    sort($all_rids);
+    $this->rid = array_pop($all_rids);
+  }
+
+  /**
+   * Change user permissions and check user_access().
+   */
+  function testUserPermissionChanges() {
+    $this->drupalLogin($this->admin_user);
+    $rid = $this->rid;
+    $account = $this->admin_user;
+
+    // Add a permission.
+    $this->assertFalse(user_access('administer nodes', $account, TRUE), t('User does not have "administer nodes" permission.'));
+    $edit = array();
+    $edit[$rid . '[administer nodes]'] = TRUE;
+    $this->drupalPost('admin/user/permissions', $edit, t('Save permissions'));
+    $this->assertText(t('The changes have been saved.'), t('Successful save message displayed.'));
+    $this->assertTrue(user_access('administer nodes', $account, TRUE), t('User now has "administer nodes" permission.'));
+
+    // Remove a permission.
+    $this->assertTrue(user_access('access user profiles', $account, TRUE), t('User has "access user profiles" permission.'));
+    $edit = array();
+    $edit[$rid . '[access user profiles]'] = FALSE;
+    $this->drupalPost('admin/user/permissions', $edit, t('Save permissions'));
+    $this->assertText(t('The changes have been saved.'), t('Successful save message displayed.'));
+    $this->assertFalse(user_access('access user profiles', $account, TRUE), t('User no longer has "access user profiles" permission.'));
+  }
+
+}