From 9b2da9e39a486f4623ee166465e05a74c00f0561 Mon Sep 17 00:00:00 2001 From: Kristiaan Van den Eynde Date: Thu, 25 May 2017 11:00:36 +0200 Subject: [PATCH] Issue #540008 by kristiaanvandeneynde, ianthomas_uk, greggles, michaelfavia: Remove the special behavior of uid #1 --- .../Core/Cache/Context/UserRolesCacheContext.php | 7 ------ core/lib/Drupal/Core/Session/AccountInterface.php | 5 +++++ core/lib/Drupal/Core/Session/UserSession.php | 7 +----- .../Drupal/Core/Test/FunctionalTestSetupTrait.php | 2 ++ .../tests/src/Kernel/FileManagedUnitTestBase.php | 2 +- .../tests/src/Kernel/FilterDefaultConfigTest.php | 3 +++ .../tests/src/Functional/Module/UninstallTest.php | 4 +++- .../user/config/install/user.role.superuser.yml | 8 +++++++ core/modules/user/src/AccountForm.php | 6 +++++ core/modules/user/src/AccountSettingsForm.php | 5 +++-- .../user/src/Authentication/Provider/Cookie.php | 13 +++++++++-- core/modules/user/src/Entity/User.php | 19 ++++++++-------- .../EntityReferenceSelection/UserSelection.php | 2 +- .../modules/user/src/Plugin/views/filter/Roles.php | 5 +++-- core/modules/user/src/RoleAccessControlHandler.php | 3 ++- core/modules/user/src/RoleInterface.php | 5 +++++ .../src/Kernel/Migrate/d6/MigrateUserTest.php | 3 +++ .../user/tests/src/Kernel/UserEntityTest.php | 14 ++++++++---- core/modules/user/user.module | 12 ++++++---- .../KernelTests/Core/Render/RenderCacheTest.php | 26 +++++----------------- 20 files changed, 90 insertions(+), 61 deletions(-) create mode 100644 core/modules/user/config/install/user.role.superuser.yml diff --git a/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php b/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php index 0cd0fbe..0a7f909 100644 --- a/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php +++ b/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php @@ -27,13 +27,6 @@ public static function getLabel() { * {@inheritdoc} */ public function getContext($role = NULL) { - // User 1 does not actually have any special behavior for roles; this is - // added as additional security and backwards compatibility protection for - // SA-CORE-2015-002. - // @todo Remove in Drupal 9.0.0. - if ($this->user->id() == 1) { - return 'is-super-user'; - } if ($role === NULL) { return implode(',', $this->user->getRoles()); } diff --git a/core/lib/Drupal/Core/Session/AccountInterface.php b/core/lib/Drupal/Core/Session/AccountInterface.php index 1cf82dc..18bf9bb 100644 --- a/core/lib/Drupal/Core/Session/AccountInterface.php +++ b/core/lib/Drupal/Core/Session/AccountInterface.php @@ -23,6 +23,11 @@ const AUTHENTICATED_ROLE = 'authenticated'; /** + * Role ID for the superuser account. + */ + const SUPERUSER_ROLE = 'superuser'; + + /** * Returns the user ID or 0 for anonymous. * * @return int diff --git a/core/lib/Drupal/Core/Session/UserSession.php b/core/lib/Drupal/Core/Session/UserSession.php index f426803..4c86320 100644 --- a/core/lib/Drupal/Core/Session/UserSession.php +++ b/core/lib/Drupal/Core/Session/UserSession.php @@ -93,7 +93,7 @@ public function getRoles($exclude_locked_roles = FALSE) { $roles = $this->roles; if ($exclude_locked_roles) { - $roles = array_values(array_diff($roles, [AccountInterface::ANONYMOUS_ROLE, AccountInterface::AUTHENTICATED_ROLE])); + $roles = array_values(array_diff($roles, [AccountInterface::ANONYMOUS_ROLE, AccountInterface::AUTHENTICATED_ROLE, AccountInterface::SUPERUSER_ROLE])); } return $roles; @@ -103,11 +103,6 @@ public function getRoles($exclude_locked_roles = FALSE) { * {@inheritdoc} */ public function hasPermission($permission) { - // User #1 has all privileges. - if ((int) $this->id() === 1) { - return TRUE; - } - return $this->getRoleStorage()->isPermissionInRoles($permission, $this->getRoles()); } diff --git a/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php index e9d8d66..a531c34 100644 --- a/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php @@ -9,6 +9,7 @@ use Drupal\Core\DrupalKernel; use Drupal\Core\Extension\MissingDependencyException; use Drupal\Core\Serialization\Yaml; +use Drupal\Core\Session\AccountInterface; use Drupal\Core\Session\UserSession; use Drupal\Core\Site\Settings; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -368,6 +369,7 @@ protected function initUserSession() { 'pass_raw' => $password, 'passRaw' => $password, 'timezone' => date_default_timezone_get(), + 'roles' => [AccountInterface::AUTHENTICATED_ROLE, AccountInterface::SUPERUSER_ROLE] ]); // The child site derives its session name from the database prefix when diff --git a/core/modules/file/tests/src/Kernel/FileManagedUnitTestBase.php b/core/modules/file/tests/src/Kernel/FileManagedUnitTestBase.php index 7c43e9b..f2c8871 100644 --- a/core/modules/file/tests/src/Kernel/FileManagedUnitTestBase.php +++ b/core/modules/file/tests/src/Kernel/FileManagedUnitTestBase.php @@ -32,7 +32,7 @@ protected function setUp() { // Make sure that a user with uid 1 exists, self::createFile() relies on // it. - $user = User::create(['uid' => 1, 'name' => $this->randomMachineName()]); + $user = User::create(['uid' => 1, 'status' => 1, 'name' => $this->randomMachineName()]); $user->enforceIsNew(); $user->save(); \Drupal::currentUser()->setAccount($user); diff --git a/core/modules/filter/tests/src/Kernel/FilterDefaultConfigTest.php b/core/modules/filter/tests/src/Kernel/FilterDefaultConfigTest.php index e16bdda..7b4ef56 100644 --- a/core/modules/filter/tests/src/Kernel/FilterDefaultConfigTest.php +++ b/core/modules/filter/tests/src/Kernel/FilterDefaultConfigTest.php @@ -47,6 +47,7 @@ public function testInstallation() { $this->assertEqual(array_keys(filter_get_roles_by_format($format)), [ RoleInterface::ANONYMOUS_ID, RoleInterface::AUTHENTICATED_ID, + RoleInterface::SUPERUSER_ID, ]); // Verify enabled filters. @@ -76,6 +77,7 @@ public function testUpdateRoles() { $this->assertEqual(array_keys(filter_get_roles_by_format($format)), [ RoleInterface::ANONYMOUS_ID, RoleInterface::AUTHENTICATED_ID, + RoleInterface::SUPERUSER_ID, ]); // Attempt to change roles. @@ -89,6 +91,7 @@ public function testUpdateRoles() { $this->assertEqual(array_keys(filter_get_roles_by_format($format)), [ RoleInterface::ANONYMOUS_ID, RoleInterface::AUTHENTICATED_ID, + RoleInterface::SUPERUSER_ID, ]); } diff --git a/core/modules/system/tests/src/Functional/Module/UninstallTest.php b/core/modules/system/tests/src/Functional/Module/UninstallTest.php index 2b17b8f..c267c69 100644 --- a/core/modules/system/tests/src/Functional/Module/UninstallTest.php +++ b/core/modules/system/tests/src/Functional/Module/UninstallTest.php @@ -5,6 +5,7 @@ use Drupal\Core\Cache\Cache; use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Entity\EntityMalformedException; +use Drupal\Core\Session\AccountInterface; use Drupal\node\Entity\Node; use Drupal\node\Entity\NodeType; use Drupal\Tests\BrowserTestBase; @@ -32,7 +33,8 @@ public function testUserPermsUninstalled() { $this->container->get('module_installer')->uninstall(['module_test']); // Are the perms defined by module_test removed? - $this->assertFalse(user_roles(FALSE, 'module_test perm'), 'Permissions were all removed.'); + $roles = array_diff_key(user_roles(FALSE, 'module_test perm'), [AccountInterface::SUPERUSER_ROLE => 1]); + $this->assertFalse($roles, 'Permissions were all removed.'); } /** diff --git a/core/modules/user/config/install/user.role.superuser.yml b/core/modules/user/config/install/user.role.superuser.yml new file mode 100644 index 0000000..998f596 --- /dev/null +++ b/core/modules/user/config/install/user.role.superuser.yml @@ -0,0 +1,8 @@ +langcode: en +status: true +dependencies: { } +id: superuser +label: 'Superuser' +weight: -1 +is_admin: true +permissions: { } diff --git a/core/modules/user/src/AccountForm.php b/core/modules/user/src/AccountForm.php index f5e2e49..017ba62 100644 --- a/core/modules/user/src/AccountForm.php +++ b/core/modules/user/src/AccountForm.php @@ -200,6 +200,12 @@ public function form(array $form, FormStateInterface $form_state) { '#disabled' => TRUE, ]; + // Special handling for the inevitable "Superuser" role. + $form['account']['roles'][RoleInterface::SUPERUSER_ID] = [ + '#default_value' => $account->id() == 1, + '#disabled' => TRUE, + ]; + $form['account']['notify'] = [ '#type' => 'checkbox', '#title' => $this->t('Notify user of new account'), diff --git a/core/modules/user/src/AccountSettingsForm.php b/core/modules/user/src/AccountSettingsForm.php index eacc378..f935a69 100644 --- a/core/modules/user/src/AccountSettingsForm.php +++ b/core/modules/user/src/AccountSettingsForm.php @@ -104,10 +104,11 @@ public function buildForm(array $form, FormStateInterface $form_state) { '#title' => $this->t('Administrator role'), '#open' => TRUE, ]; - // Do not allow users to set the anonymous or authenticated user roles as the - // administrator role. + // Do not allow users to set the anonymous, authenticated user or superuser + // roles as the administrator role. $roles = user_role_names(TRUE); unset($roles[RoleInterface::AUTHENTICATED_ID]); + unset($roles[RoleInterface::SUPERUSER_ID]); $admin_roles = $this->roleStorage->getQuery() ->condition('is_admin', TRUE) diff --git a/core/modules/user/src/Authentication/Provider/Cookie.php b/core/modules/user/src/Authentication/Provider/Cookie.php index ebcdae7..bd0de3e 100644 --- a/core/modules/user/src/Authentication/Provider/Cookie.php +++ b/core/modules/user/src/Authentication/Provider/Cookie.php @@ -76,11 +76,20 @@ protected function getUserFromSession(SessionInterface $session) { // Check if the user data was found and the user is active. if (!empty($values) && $values['status'] == 1) { - // Add the user's roles. + // Fetch the user's roles. $rids = $this->connection ->query('SELECT roles_target_id FROM {user__roles} WHERE entity_id = :uid', [':uid' => $values['uid']]) ->fetchCol(); - $values['roles'] = array_merge([AccountInterface::AUTHENTICATED_ROLE], $rids); + + // Add in the authenticated user role. If we're dealing with the + // superuser account, add the superuser role as well. + $added_roles = [AccountInterface::AUTHENTICATED_ROLE]; + if ($uid == 1) { + $added_roles[] = AccountInterface::SUPERUSER_ROLE; + } + + // Add the user's roles to the session values. + $values['roles'] = array_merge($added_roles, $rids); return new UserSession($values); } diff --git a/core/modules/user/src/Entity/User.php b/core/modules/user/src/Entity/User.php index 7529532..f376040 100644 --- a/core/modules/user/src/Entity/User.php +++ b/core/modules/user/src/Entity/User.php @@ -80,9 +80,10 @@ public function isNew() { public function preSave(EntityStorageInterface $storage) { parent::preSave($storage); - // Make sure that the authenticated/anonymous roles are not persisted. + // Make sure that the authenticated, anonymous and superuser roles are not + // actually saved to the database. foreach ($this->get('roles') as $index => $item) { - if (in_array($item->target_id, [RoleInterface::ANONYMOUS_ID, RoleInterface::AUTHENTICATED_ID])) { + if (in_array($item->target_id, [RoleInterface::ANONYMOUS_ID, RoleInterface::AUTHENTICATED_ID, RoleInterface::SUPERUSER_ID])) { $this->get('roles')->offsetUnset($index); } } @@ -146,6 +147,11 @@ public function getRoles($exclude_locked_roles = FALSE) { if (!$exclude_locked_roles) { if ($this->isAuthenticated()) { $roles[] = RoleInterface::AUTHENTICATED_ID; + + // The superuser account gets the superuser role. + if ($this->id() == 1) { + $roles[] = RoleInterface::SUPERUSER_ID; + } } else { $roles[] = RoleInterface::ANONYMOUS_ID; @@ -173,8 +179,8 @@ public function hasRole($rid) { */ public function addRole($rid) { - if (in_array($rid, [RoleInterface::AUTHENTICATED_ID, RoleInterface::ANONYMOUS_ID])) { - throw new \InvalidArgumentException('Anonymous or authenticated role ID must not be assigned manually.'); + if (in_array($rid, [RoleInterface::AUTHENTICATED_ID, RoleInterface::ANONYMOUS_ID, RoleInterface::SUPERUSER_ID])) { + throw new \InvalidArgumentException('Anonymous, authenticated or superuser role ID must not be assigned manually.'); } $roles = $this->getRoles(TRUE); @@ -193,11 +199,6 @@ public function removeRole($rid) { * {@inheritdoc} */ public function hasPermission($permission) { - // User #1 has all privileges. - if ((int) $this->id() === 1) { - return TRUE; - } - return $this->getRoleStorage()->isPermissionInRoles($permission, $this->getRoles()); } diff --git a/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php index d06a81c..be5c02f 100644 --- a/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php +++ b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php @@ -129,7 +129,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta '#type' => 'checkboxes', '#title' => $this->t('Restrict to the selected roles'), '#required' => TRUE, - '#options' => array_diff_key(user_role_names(TRUE), [RoleInterface::AUTHENTICATED_ID => RoleInterface::AUTHENTICATED_ID]), + '#options' => array_diff_key(user_role_names(TRUE), [RoleInterface::AUTHENTICATED_ID => RoleInterface::AUTHENTICATED_ID, RoleInterface::SUPERUSER_ID => RoleInterface::SUPERUSER_ID]), '#default_value' => $configuration['filter']['role'], ]; } diff --git a/core/modules/user/src/Plugin/views/filter/Roles.php b/core/modules/user/src/Plugin/views/filter/Roles.php index 47adafb..150b367 100644 --- a/core/modules/user/src/Plugin/views/filter/Roles.php +++ b/core/modules/user/src/Plugin/views/filter/Roles.php @@ -55,6 +55,7 @@ public static function create(ContainerInterface $container, array $configuratio public function getValueOptions() { $this->valueOptions = user_role_names(TRUE); unset($this->valueOptions[RoleInterface::AUTHENTICATED_ID]); + unset($this->valueOptions[RoleInterface::SUPERUSER_ID]); return $this->valueOptions; } @@ -64,8 +65,8 @@ public function getValueOptions() { */ public function operators() { $operators = parent::operators(); - $operators['empty']['title'] = $this->t("Only has the 'authenticated user' role"); - $operators['not empty']['title'] = $this->t("Has roles in addition to 'authenticated user'"); + $operators['empty']['title'] = $this->t("Only has the 'authenticated user' or 'superuser' role"); + $operators['not empty']['title'] = $this->t("Has roles in addition to 'authenticated user' or 'superuser'"); return $operators; } diff --git a/core/modules/user/src/RoleAccessControlHandler.php b/core/modules/user/src/RoleAccessControlHandler.php index e979f2d..4b64130 100644 --- a/core/modules/user/src/RoleAccessControlHandler.php +++ b/core/modules/user/src/RoleAccessControlHandler.php @@ -20,7 +20,8 @@ class RoleAccessControlHandler extends EntityAccessControlHandler { protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) { switch ($operation) { case 'delete': - if ($entity->id() == RoleInterface::ANONYMOUS_ID || $entity->id() == RoleInterface::AUTHENTICATED_ID) { + $internal = [RoleInterface::ANONYMOUS_ID, RoleInterface::AUTHENTICATED_ID, RoleInterface::SUPERUSER_ID]; + if (in_array($entity->id(), $internal)) { return AccessResult::forbidden(); } diff --git a/core/modules/user/src/RoleInterface.php b/core/modules/user/src/RoleInterface.php index 4633637..3c09569 100644 --- a/core/modules/user/src/RoleInterface.php +++ b/core/modules/user/src/RoleInterface.php @@ -23,6 +23,11 @@ const AUTHENTICATED_ID = AccountInterface::AUTHENTICATED_ROLE; /** + * Role ID for the superuser account; should match what's in the "role" table. + */ + const SUPERUSER_ID = AccountInterface::SUPERUSER_ROLE; + + /** * Returns a list of permissions assigned to the role. * * @return array diff --git a/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php index ed28777..79bf355 100644 --- a/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php @@ -89,6 +89,9 @@ public function testUser() { ->execute() ->fetchCol(); $roles = [RoleInterface::AUTHENTICATED_ID]; + if ($source->uid == 1) { + $roles[] = RoleInterface::SUPERUSER_ID; + } $id_map = $this->getMigration('d6_user_role')->getIdMap(); foreach ($rids as $rid) { $role = $id_map->lookupDestinationId([$rid]); diff --git a/core/modules/user/tests/src/Kernel/UserEntityTest.php b/core/modules/user/tests/src/Kernel/UserEntityTest.php index 1c501f0..225173f 100644 --- a/core/modules/user/tests/src/Kernel/UserEntityTest.php +++ b/core/modules/user/tests/src/Kernel/UserEntityTest.php @@ -34,10 +34,16 @@ public function testUserMethods() { $role_storage->create(['id' => 'test_role_two'])->save(); $role_storage->create(['id' => 'test_role_three'])->save(); - $values = [ - 'uid' => 1, - 'roles' => ['test_role_one'], - ]; + // Test that user 1 gets the authenticated and superuser role. + $user = User::create(['uid' => 1]); + $this->assertEqual([RoleInterface::AUTHENTICATED_ID, RoleInterface::SUPERUSER_ID], $user->getRoles()); + + // Test that a regular authenticated user gets the authenticated role. + $user = User::create(['uid' => 2]); + $this->assertEqual([RoleInterface::AUTHENTICATED_ID], $user->getRoles()); + + // Test role manipulation for an authenticated user. + $values = ['uid' => 3, 'roles' => ['test_role_one']]; $user = User::create($values); $this->assertTrue($user->hasRole('test_role_one')); diff --git a/core/modules/user/user.module b/core/modules/user/user.module index 61faa3b..be8fc3d 100644 --- a/core/modules/user/user.module +++ b/core/modules/user/user.module @@ -993,8 +993,10 @@ function user_role_names($membersonly = FALSE, $permission = NULL) { * Implements hook_ENTITY_TYPE_insert() for user_role entities. */ function user_user_role_insert(RoleInterface $role) { - // Ignore the authenticated and anonymous roles or the role is being synced. - if (in_array($role->id(), [RoleInterface::AUTHENTICATED_ID, RoleInterface::ANONYMOUS_ID]) || $role->isSyncing()) { + // Ignore the authenticated, anonymous and superuser roles or when the role is + // being synced. + $internal = [RoleInterface::AUTHENTICATED_ID, RoleInterface::ANONYMOUS_ID, RoleInterface::SUPERUSER_ID]; + if (in_array($role->id(), $internal) || $role->isSyncing()) { return; } @@ -1034,8 +1036,10 @@ function user_user_role_delete(RoleInterface $role) { $user_storage = \Drupal::entityManager()->getStorage('user'); $user_storage->deleteRoleReferences([$role->id()]); - // Ignore the authenticated and anonymous roles or the role is being synced. - if (in_array($role->id(), [RoleInterface::AUTHENTICATED_ID, RoleInterface::ANONYMOUS_ID]) || $role->isSyncing()) { + // Ignore the authenticated, anonymous and superuser roles or when the role is + // being synced. + $internal = [RoleInterface::AUTHENTICATED_ID, RoleInterface::ANONYMOUS_ID, RoleInterface::SUPERUSER_ID]; + if (in_array($role->id(), $internal) || $role->isSyncing()) { return; } diff --git a/core/tests/Drupal/KernelTests/Core/Render/RenderCacheTest.php b/core/tests/Drupal/KernelTests/Core/Render/RenderCacheTest.php index 91061ca..5dbc100 100644 --- a/core/tests/Drupal/KernelTests/Core/Render/RenderCacheTest.php +++ b/core/tests/Drupal/KernelTests/Core/Render/RenderCacheTest.php @@ -32,35 +32,19 @@ protected function setUp() { } /** - * Tests that user 1 has a different permission context with the same roles. + * Check if user 1 has a unique render cache for the user.permissions context. */ public function testUser1PermissionContext() { - $this->doTestUser1WithContexts(['user.permissions']); - } - - /** - * Tests that user 1 has a different roles context with the same roles. - */ - public function testUser1RolesContext() { - $this->doTestUser1WithContexts(['user.roles']); - } + $contexts = ['user.permissions']; - /** - * Ensures that user 1 has a unique render cache for the given context. - * - * @param string[] $contexts - * List of cache contexts to use. - */ - protected function doTestUser1WithContexts($contexts) { - // Test that user 1 does not share the cache with other users who have the - // same roles, even when using a role-based cache context. + // Set up the superuser account (UID 1), two authenticated users and an + // admin user so we can test the output of the render cache for them. $user1 = $this->createUser(); $this->assertEqual($user1->id(), 1); $first_authenticated_user = $this->createUser(); $second_authenticated_user = $this->createUser(); $admin_user = $this->createUser([], NULL, TRUE); - $this->assertEqual($user1->getRoles(), $first_authenticated_user->getRoles(), 'User 1 has the same roles as an authenticated user.'); // Impersonate user 1 and render content that only user 1 should have // permission to see. \Drupal::service('account_switcher')->switchTo($user1); @@ -84,7 +68,7 @@ protected function doTestUser1WithContexts($contexts) { \Drupal::service('account_switcher')->switchBack(); // Verify that the first authenticated user does not see the same content - // as user 1. + // as user 1, who has the superuser role. \Drupal::service('account_switcher')->switchTo($first_authenticated_user); $element = $test_element; $element['#markup'] = 'content for authenticated users'; -- 2.8.1