Issue #3411837 by larowlan, longwave, catch, uditrawat, marcoscano,...

Issue #3411837 by larowlan, longwave, catch, uditrawat, marcoscano, smustgrave, cilefen, greggles: Media revision listing is accessible to anonymous users

(cherry picked from commit 8f3f3749)

core/modules/media/src/MediaAccessControlHandler.php

@@ -50,6 +50,7 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    * {@inheritdoc}
    */
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
+    /** @var \Drupal\media\MediaInterface $entity */
     // Allow admin permission to override all operations.
     if ($account->hasPermission($this->entityType->getAdminPermission())) {
       return AccessResult::allowed()->cachePerPermissions();
@@ -121,18 +122,17 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
       case 'view all revisions':
       case 'view revision':
         if ($account->hasPermission('view any ' . $type . ' media revisions') || $account->hasPermission("view all media revisions")) {
-          return AccessResult::allowed()->cachePerPermissions();
-        }
+          // Check the access to this revision and if the media passed in is not
+          // the default revision then access to that too.
+          $entity_access = $entity->access('view', $account, TRUE);
+          if (!$entity->isDefaultRevision()) {
+            $media_storage = $this->entityTypeManager->getStorage($entity->getEntityTypeId());
+            $entity_access->andIf($this->access($media_storage->load($entity->id()), 'view', $account, TRUE));
+          }
 
-        // First check the access to the default revision and finally, if the
-        // media passed in is not the default revision then access to that,
-        // too.
-        $media_storage = $this->entityTypeManager->getStorage($entity->getEntityTypeId());
-        $access = $this->access($media_storage->load($entity->id()), 'view', $account, TRUE);
-        if (!$entity->isDefaultRevision()) {
-          $access = $access->andIf($this->access($entity, 'view', $account, TRUE));
+          return AccessResult::allowed()->cachePerPermissions()->andIf($entity_access);
         }
-        return $access->cachePerPermissions()->addCacheableDependency($entity);
+        return AccessResult::neutral()->cachePerPermissions();
 
       case 'revert':
         return AccessResult::allowedIfHasPermission($account, 'revert any ' . $type . ' media revisions')

core/modules/media/tests/src/Functional/MediaAccessTest.php

@@ -106,7 +106,9 @@ public function testMediaAccess() {
     $this->assertNoCacheContext('user');
     $this->assertCacheContext('user.permissions');
     $assert_session->statusCodeEquals(200);
-    $user_media->setUnpublished()->save();
+    $previous_revision = $user_media->getLoadedRevisionId();
+    $user_media->setUnpublished()->setNewRevision();
+    $user_media->save();
     $this->drupalGet('media/' . $user_media->id());
     $this->assertCacheContext('user.permissions');
     $assert_session->statusCodeEquals(403);
@@ -117,6 +119,43 @@ public function testMediaAccess() {
     $this->assertCacheContext('user');
     $assert_session->statusCodeEquals(200);
 
+    // Test revision access - logged-in user.
+    $this->grantPermissions($role, ['view all media revisions']);
+    $this->drupalGet('media/' . $user_media->id() . '/revisions');
+    $this->assertCacheContext('user');
+    $assert_session->statusCodeEquals(200);
+    $this->drupalGet('media/' . $user_media->id() . '/revisions/' . $user_media->getRevisionId() . '/view');
+    $this->assertCacheContext('user');
+    $assert_session->statusCodeEquals(200);
+    $this->drupalGet('media/' . $user_media->id() . '/revisions/' . $previous_revision . '/view');
+    $this->assertCacheContext('user.permissions');
+    $assert_session->statusCodeEquals(200);
+    $role->revokePermission('view own unpublished media')->save();
+    $this->drupalGet('media/' . $user_media->id() . '/revisions/' . $user_media->getRevisionId() . '/view');
+    $this->assertCacheContext('user.permissions');
+    $assert_session->statusCodeEquals(403);
+
+    $user_media->setPublished()->setNewRevision();
+    $user_media->save();
+
+    // Revision access - logged-out user.
+    $this->drupalLogout();
+    $this->drupalGet('media/' . $user_media->id() . '/revisions');
+    $assert_session->statusCodeEquals(403);
+    $this->drupalGet('media/' . $user_media->id() . '/revisions/' . $user_media->getRevisionId() . '/view');
+    $assert_session->statusCodeEquals(403);
+    $this->drupalGet('media/' . $user_media->id() . '/revisions/' . $previous_revision . '/view');
+    $assert_session->statusCodeEquals(403);
+
+    // Reverse revision access testing changes.
+    $role
+      ->revokePermission('view all media revisions')
+      ->grantPermission('view own unpublished media')
+      ->save();
+    $user_media->setPublished()->setNewRevision();
+    $user_media->save();
+    $this->drupalLogin($this->nonAdminUser);
+
     // Test 'create media' permission.
     $this->drupalGet('media/add/' . $media_type->id());
     $this->assertCacheContext('user.permissions');

core/modules/media/tests/src/Functional/MediaRevisionTest.php

@@ -83,7 +83,6 @@ public function testRevisions() {
     // Test 'view all media revisions' permission ('view media' permission is
     // needed as well).
     user_role_revoke_permissions($role->id(), [
-      'view media',
       'view all media revisions',
     ]);
     $this->drupalGet($media->toUrl('revision'));

core/modules/media/tests/src/Functional/Rest/MediaResourceTestBase.php

@@ -245,7 +245,6 @@ protected function getExpectedNormalizedEntity() {
           'url' => base_path() . 'user/' . $author->id(),
         ],
       ],
-      'revision_log_message' => [],
       'revision_translation_affected' => [
         [
           'value' => TRUE,

core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php

@@ -483,7 +483,7 @@ public function providerAccess() {
       'view all revisions',
       AccessResult::neutral(),
       ['user.permissions'],
-      ['media:1'],
+      [],
       TRUE,
     ];
     $test_data['admins can view all revisions'] = [
@@ -496,12 +496,12 @@ public function providerAccess() {
       TRUE,
     ];
     $test_data['view all revisions with view bundle permission'] = [
-      ['view any test media revisions'],
-      [],
+      ['view any test media revisions', 'view media'],
+      ['status' => TRUE],
       'view all revisions',
       AccessResult::allowed(),
       ['user.permissions'],
-      [],
+      ['media:1'],
       TRUE,
     ];
     // Revert revisions:
@@ -769,10 +769,7 @@ public function testRevisionLogFieldAccess(): void {
     $entity->save();
     $this->assertTrue($entity->get('revision_log_message')->access('view', $admin));
     $this->assertTrue($entity->get('revision_log_message')->access('view', $editor));
-    // revision_log_message field access can be granted with the "view revision"
-    // operation. "view revision" access is granted if the user is allowed to
-    // view the default revision of the media entity.
-    $this->assertTrue($entity->get('revision_log_message')->access('view', $viewer));
+    $this->assertFalse($entity->get('revision_log_message')->access('view', $viewer));
     $entity->setUnpublished()->save();
     \Drupal::entityTypeManager()->getAccessControlHandler('media')->resetCache();
     $this->assertFalse($entity->get('revision_log_message')->access('view', $viewer));