Issue #2417895 by David_Rothstein, Berdir, Wim Leers, willzyx, catch: AccountPermissionsCacheContext/PermissionsHashGenerator must special case user 1, since permissions don't apply to it

core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php

@@ -9,6 +9,9 @@ namespace Drupal\Core\Cache\Context;
 
 /**
  * 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 @@ class UserRolesCacheContext extends UserCacheContext implements CalculatedCacheC
    * {@inheritdoc}
    */
   public function getContext($role = NULL) {
+    // User 1 does not actually have any special behavior for roles, this is
+    // added as additional security and BC compatibility for SA-CORE-2015-002.
+    // user.
+    // @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());
     }

core/lib/Drupal/Core/Session/PermissionsHashGenerator.php

@@ -51,6 +51,12 @@ class PermissionsHashGenerator implements PermissionsHashGeneratorInterface {
    * 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 @@ class PermissionsHashGenerator implements PermissionsHashGeneratorInterface {
     $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);
   }
 
 }

core/modules/system/src/Tests/Render/RenderCacheTest.php

@@ -0,0 +1,119 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\system\Tests\Render\RenderCacheTest.
+ */
+
+namespace Drupal\system\Tests\Render;
+
+use Drupal\simpletest\KernelTestBase;
+use Drupal\simpletest\UserCreationTrait;
+
+/**
+ * Tests the caching of render items via functional tests.
+ *
+ * @group Render
+ */
+class RenderCacheTest extends KernelTestBase {
+
+  use UserCreationTrait;
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = ['user', 'system'];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+    $this->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();
+  }
+
+}

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 @@ use Drupal\Tests\UnitTestCase;
  * @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 @@ class PermissionsHashTest extends UnitTestCase {
 
     new Settings(array('hash_salt' => 'test'));
 
-    // Account 1: 'administrator' and 'authenticated' roles.
+    // The mocked super user account, with the same roles as Account 2.
-    $roles_1 = array('administrator', 'authenticated');
     $this->account1 = $this->getMockBuilder('Drupal\user\Entity\User')
       ->disableOriginalConstructor()
-      ->setMethods(array('getRoles'))
+      ->setMethods(array('getRoles', 'id'))
       ->getMock();
     $this->account1->expects($this->any())
-      ->method('getRoles')
+      ->method('id')
-      ->will($this->returnValue($roles_1));
+      ->willReturn(1);
+    $this->account1->expects($this->never())
+      ->method('getRoles');
 
-    // Account 2: 'authenticated' and 'administrator' roles (different order).
+    // Account 2: 'administrator' and 'authenticated' roles.
-    $roles_2 = array('authenticated', 'administrator');
+    $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.
+    // Account 3: 'authenticated' and 'administrator' roles (different order).
-    $roles_1_updated = array('editor', 'administrator', 'authenticated');
+    $roles_3 = array('authenticated', 'administrator');
-    $this->account1Updated = $this->getMockBuilder('Drupal\user\Entity\User')
+    $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 @@ class PermissionsHashTest extends UnitTestCase {
    * 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);
+    $updated_hash_2 = $this->permissionsHash->generate($this->account2Updated);
-    $this->assertNotSame($hash_1, $updated_hash_1, 'Same user with updated roles generates different permissions hash.');
+    $this->assertNotSame($hash_2, $updated_hash_2, 'Same user with updated roles generates different permissions hash.');
   }
 
   /**
@@ -146,7 +178,7 @@ class PermissionsHashTest extends UnitTestCase {
     $this->cache->expects($this->never())
       ->method('set');
 
-    $this->permissionsHash->generate($this->account1);
+    $this->permissionsHash->generate($this->account2);
   }
 
   /**
@@ -164,7 +196,7 @@ class PermissionsHashTest extends UnitTestCase {
       ->method('set')
       ->with($expected_cid, $this->isType('string'));
 
-    $this->permissionsHash->generate($this->account1);
+    $this->permissionsHash->generate($this->account2);
   }
 
 }