diff --git a/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php b/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php index 375d80b..ceb3e8c 100644 --- a/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php +++ b/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php @@ -9,6 +9,9 @@ /** * Defines the UserRolesCacheContext service, for "per role" caching. + * + * Only use this cache context when checking explicitly for certain roles. Use + * user.permissions for anything that checks permissions. */ class UserRolesCacheContext extends UserCacheContext implements CalculatedCacheContextInterface{ @@ -23,6 +26,13 @@ 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 'r.' . implode(',', $this->user->getRoles()); } diff --git a/core/lib/Drupal/Core/Session/PermissionsHashGenerator.php b/core/lib/Drupal/Core/Session/PermissionsHashGenerator.php index f176afd..c4fe7b9 100644 --- a/core/lib/Drupal/Core/Session/PermissionsHashGenerator.php +++ b/core/lib/Drupal/Core/Session/PermissionsHashGenerator.php @@ -51,6 +51,12 @@ public function __construct(PrivateKey $private_key, CacheBackendInterface $cach * Cached by role, invalidated whenever permissions change. */ public function generate(AccountInterface $account) { + // User 1 is the super user, and can always access all permissions. Use a + // different, unique identifier for the hash. + if ($account->id() == 1) { + return $this->hash('is-super-user'); + } + $sorted_roles = $account->getRoles(); sort($sorted_roles); $role_list = implode(',', $sorted_roles); @@ -81,9 +87,28 @@ protected function doGenerate(array $roles) { $permissions_by_role = user_role_permissions($roles); foreach ($permissions_by_role as $role => $permissions) { sort($permissions); + // Note that for admin roles (\Drupal\user\RoleInterface::isAdmin()), the + // permissions returned will be empty ($permissions = []). Therefore the + // presence of the role ID as a key in $permissions_by_role is essential + // to ensure that the hash correctly recognizes admin roles. (If the hash + // was based solely on the union of $permissions, the admin roles would + // effectively be no-ops, allowing for hash collisions.) $permissions_by_role[$role] = $permissions; } - return hash('sha256', $this->privateKey->get() . Settings::getHashSalt() . serialize($permissions_by_role)); + return $this->hash(serialize($permissions_by_role)); + } + + /** + * Hashes the given string. + * + * @param string $identifier + * The string to be hashed. + * + * @return string + * The hash. + */ + protected function hash($identifier) { + return hash('sha256', $this->privateKey->get() . Settings::getHashSalt() . $identifier); } } diff --git a/core/modules/system/src/Tests/Render/RenderCacheTest.php b/core/modules/system/src/Tests/Render/RenderCacheTest.php new file mode 100644 index 0000000..b4317d1 --- /dev/null +++ b/core/modules/system/src/Tests/Render/RenderCacheTest.php @@ -0,0 +1,119 @@ +installEntitySchema('user'); + $this->installConfig(['user']); + $this->installSchema('system', ['sequences']); + } + + /** + * Tests that user 1 has a different permission context with the same roles. + */ + 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']); + } + + /** + * 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. + $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); + $test_element = [ + '#cache' => [ + 'keys' => ['test'], + 'contexts' => $contexts, + ], + ]; + $element = $test_element; + $element['#markup'] = 'content for user 1'; + $output = \Drupal::service('renderer')->render($element); + $this->assertEqual($output, 'content for user 1'); + + // Verify the cache is working by rendering the same element but with + // different markup passed in; the result should be the same. + $element = $test_element; + $element['#markup'] = 'should not be used'; + $output = \Drupal::service('renderer')->render($element); + $this->assertEqual($output, 'content for user 1'); + \Drupal::service('account_switcher')->switchBack(); + + // Verify that the first authenticated user does not see the same content + // as user 1. + \Drupal::service('account_switcher')->switchTo($first_authenticated_user); + $element = $test_element; + $element['#markup'] = 'content for authenticated users'; + $output = \Drupal::service('renderer')->render($element); + $this->assertEqual($output, 'content for authenticated users'); + \Drupal::service('account_switcher')->switchBack(); + + // Verify that the second authenticated user shares the cache with the + // first authenticated user. + \Drupal::service('account_switcher')->switchTo($second_authenticated_user); + $element = $test_element; + $element['#markup'] = 'should not be used'; + $output = \Drupal::service('renderer')->render($element); + $this->assertEqual($output, 'content for authenticated users'); + \Drupal::service('account_switcher')->switchBack(); + + // Verify that the admin user (who has an admin role without explicit + // permissions) does not share the same cache. + \Drupal::service('account_switcher')->switchTo($admin_user); + $element = $test_element; + $element['#markup'] = 'content for admin user'; + $output = \Drupal::service('renderer')->render($element); + $this->assertEqual($output, 'content for admin user'); + \Drupal::service('account_switcher')->switchBack(); + } + +} diff --git a/core/tests/Drupal/Tests/Core/Session/PermissionsHashTest.php b/core/tests/Drupal/Tests/Core/Session/PermissionsHashGeneratorTest.php similarity index 65% rename from core/tests/Drupal/Tests/Core/Session/PermissionsHashTest.php rename to core/tests/Drupal/Tests/Core/Session/PermissionsHashGeneratorTest.php index 2bc202c..616788d 100644 --- a/core/tests/Drupal/Tests/Core/Session/PermissionsHashTest.php +++ b/core/tests/Drupal/Tests/Core/Session/PermissionsHashGeneratorTest.php @@ -2,7 +2,7 @@ /** * @file - * Contains \Drupal\Tests\Core\Session\PermissionsHashTest. + * Contains \Drupal\Tests\Core\Session\PermissionsHashGeneratorTest. */ namespace Drupal\Tests\Core\Session { @@ -17,28 +17,35 @@ * @coversDefaultClass \Drupal\Core\Session\PermissionsHashGenerator * @group Session */ -class PermissionsHashTest extends UnitTestCase { +class PermissionsHashGeneratorTest extends UnitTestCase { /** - * A mocked account. + * The mocked super user account. * * @var \Drupal\user\UserInterface|\PHPUnit_Framework_MockObject_MockObject */ protected $account1; /** + * A mocked account. + * + * @var \Drupal\user\UserInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $account2; + + /** * An "updated" mocked account. * * @var \Drupal\user\UserInterface|\PHPUnit_Framework_MockObject_MockObject */ - protected $account1Updated; + protected $account2Updated; /** * A different account. * * @var \Drupal\user\UserInterface|\PHPUnit_Framework_MockObject_MockObject */ - protected $account2; + protected $account3; /** * The mocked private key service. @@ -69,35 +76,55 @@ protected function setUp() { new Settings(array('hash_salt' => 'test')); - // Account 1: 'administrator' and 'authenticated' roles. - $roles_1 = array('administrator', 'authenticated'); + // The mocked super user account, with the same roles as Account 2. $this->account1 = $this->getMockBuilder('Drupal\user\Entity\User') ->disableOriginalConstructor() - ->setMethods(array('getRoles')) + ->setMethods(array('getRoles', 'id')) ->getMock(); $this->account1->expects($this->any()) - ->method('getRoles') - ->will($this->returnValue($roles_1)); + ->method('id') + ->willReturn(1); + $this->account1->expects($this->never()) + ->method('getRoles'); - // Account 2: 'authenticated' and 'administrator' roles (different order). - $roles_2 = array('authenticated', 'administrator'); + // Account 2: 'administrator' and 'authenticated' roles. + $roles_1 = array('administrator', 'authenticated'); $this->account2 = $this->getMockBuilder('Drupal\user\Entity\User') ->disableOriginalConstructor() - ->setMethods(array('getRoles')) + ->setMethods(array('getRoles', 'id')) ->getMock(); $this->account2->expects($this->any()) ->method('getRoles') - ->will($this->returnValue($roles_2)); + ->will($this->returnValue($roles_1)); + $this->account2->expects($this->any()) + ->method('id') + ->willReturn(2); - // Updated account 1: now also 'editor' role. - $roles_1_updated = array('editor', 'administrator', 'authenticated'); - $this->account1Updated = $this->getMockBuilder('Drupal\user\Entity\User') + // Account 3: 'authenticated' and 'administrator' roles (different order). + $roles_3 = array('authenticated', 'administrator'); + $this->account3 = $this->getMockBuilder('Drupal\user\Entity\User') ->disableOriginalConstructor() - ->setMethods(array('getRoles')) + ->setMethods(array('getRoles', 'id')) ->getMock(); - $this->account1Updated->expects($this->any()) + $this->account3->expects($this->any()) ->method('getRoles') - ->will($this->returnValue($roles_1_updated)); + ->will($this->returnValue($roles_3)); + $this->account3->expects($this->any()) + ->method('id') + ->willReturn(3); + + // Updated account 2: now also 'editor' role. + $roles_2_updated = array('editor', 'administrator', 'authenticated'); + $this->account2Updated = $this->getMockBuilder('Drupal\user\Entity\User') + ->disableOriginalConstructor() + ->setMethods(array('getRoles', 'id')) + ->getMock(); + $this->account2Updated->expects($this->any()) + ->method('getRoles') + ->will($this->returnValue($roles_2_updated)); + $this->account2Updated->expects($this->any()) + ->method('id') + ->willReturn(2); // Mocked private key + cache services. $random = Crypt::randomBytesBase64(55); @@ -119,14 +146,19 @@ protected function setUp() { * Tests the generate() method. */ public function testGenerate() { + // Ensure that the super user (user 1) always gets the same hash. + $super_user_hash = $this->permissionsHash->generate($this->account1); + // Ensure that two user accounts with the same roles generate the same hash. - $hash_1 = $this->permissionsHash->generate($this->account1); $hash_2 = $this->permissionsHash->generate($this->account2); - $this->assertSame($hash_1, $hash_2, 'Different users with the same roles generate the same permissions hash.'); + $hash_3 = $this->permissionsHash->generate($this->account3); + $this->assertSame($hash_2, $hash_3, 'Different users with the same roles generate the same permissions hash.'); + + $this->assertNotSame($hash_2, $super_user_hash, 'User 1 has a different hash despite having the same roles'); // Compare with hash for user account 1 with an additional role. - $updated_hash_1 = $this->permissionsHash->generate($this->account1Updated); - $this->assertNotSame($hash_1, $updated_hash_1, 'Same user with updated roles generates different permissions hash.'); + $updated_hash_2 = $this->permissionsHash->generate($this->account2Updated); + $this->assertNotSame($hash_2, $updated_hash_2, 'Same user with updated roles generates different permissions hash.'); } /** @@ -146,7 +178,7 @@ public function testGenerateCache() { $this->cache->expects($this->never()) ->method('set'); - $this->permissionsHash->generate($this->account1); + $this->permissionsHash->generate($this->account2); } /** @@ -164,7 +196,7 @@ public function testGenerateNoCache() { ->method('set') ->with($expected_cid, $this->isType('string')); - $this->permissionsHash->generate($this->account1); + $this->permissionsHash->generate($this->account2); } }