Issue CollaboraOnline#49: Require media view access for Collabora ope…

…rations.

README.md

@@ -197,24 +197,32 @@ Collabora is used within Drupal.  Most of the time this permission is
 not needed, if the Collabora instance is configured from outside of
 Drupal.
 
-For each media type, the module introduces two permissions:
-- (media type): Edit any media file in Collabora Users with this
-  permission are allowed to edit documents attached to a media entity
-  of the given type, using the Collabora Online editor.
-- (media type): Preview media file in Collabora Users with this
-  permission are allowed to preview documents attached to a media
-  entity of the given type, using the Collabora Online editor in
-  preview/readonly mode.
-
-In the current version, preview and edit access with the Collabora
-Online editor are checked independently of the publishing status of
-the respective media, and independently of the regular view or edit
-access on that media entity.
+For each media type, the module introduces three permissions:
+- "(media type): Edit any media file in Collabora"  
+  Users with this permission are allowed to edit documents attached
+  to a media entity of the given type, using the Collabora Online
+  editor, if they also have regular view access for that media entity.
+- "(media type): Edit own media file in Collabora"  
+  Users with this permission are allowed to edit documents attached
+  to a media entity of the given type, using the Collabora Online
+  editor, if they also have regular view access for that media entity,
+  and if they are the owner/author of that media entity.
+- "(media type): Preview media file in Collabora"  
+  Users with this permission are allowed to preview documents attached 
+  to a media entity of the given type, using the Collabora Online 
+  editor in preview/readonly mode, if they also have regular view 
+  access for that media entity.
+
+For unpublished media entities, this means that a user needs either the 
+'administer media' permission or the 'view own unpublished media'
+permission, to open the respective document in Collabora Online.
+
+In the current version of this module, the 'administer media' permission
+from Drupal core grants access to all media operations, including the use 
+of the Collabora Online editor for preview and edit.
 
 For a consistent experience, it is recommended that a role with the
-edit permission should also be granted the preview permission, and
-that a user with any of the Collabora media permissions should also be
-granted permissions to view the respective media entity in Drupal.
+edit permission should also be granted the preview permission.
 
 Developers can use entity access hooks to alter which users may edit
 or preview media files in Collabora. This would allow to grant access

collabora_online.module

@@ -12,6 +12,7 @@
 use Drupal\collabora_online\Cool\CoolUtils;
 use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Access\AccessResultInterface;
+use Drupal\Core\Access\AccessResultReasonInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\media\MediaInterface;
@@ -123,26 +124,55 @@ function collabora_online_entity_operation(EntityInterface $entity) {
  * Checks access for the new media operations provided by this module.
  */
 function collabora_online_media_access(MediaInterface $media, string $operation, AccountInterface $account): AccessResultInterface {
+    // Use switch () to align with other entity access hooks.
+    switch ($operation) {
+        case 'preview in collabora':
+        case 'edit in collabora':
+            break;
+
+        default:
+            return AccessResult::neutral();
+    }
+
+    // Both edit and preview require regular media view access.
+    $media_view_access = $media->access('view', $account, TRUE);
+    if (!$media_view_access->isAllowed()) {
+        $reason = "Media 'view' access denied.";
+        if ($media_view_access instanceof AccessResultReasonInterface) {
+            $media_view_reason = $media_view_access->getReason();
+            if ($media_view_reason !== NULL) {
+                $reason = "Media 'view' access denied: $media_view_reason";
+            }
+        }
+        return AccessResult::forbidden($reason)
+            ->addCacheableDependency($media_view_access);
+    }
+
     $type = $media->bundle();
     switch ($operation) {
         case 'preview in collabora':
-            return AccessResult::allowedIfHasPermission($account, "preview $type in collabora");
+            $collabora_access = AccessResult::allowedIfHasPermission($account, "preview $type in collabora");
+            break;
 
-        case 'edit in collabora':
+        default:
             if ($account->hasPermission("edit any $type in collabora")) {
-                return AccessResult::allowed()->cachePerPermissions();
+                $collabora_access = AccessResult::allowed()
+                    ->cachePerPermissions();
             }
-            if ($account->hasPermission("edit own $type in collabora")) {
+            elseif ($account->hasPermission("edit own $type in collabora")) {
                 // Use '==' because Drupal sometimes loads integers as strings.
                 $is_owner = ($account->id() && $account->id() == $media->getOwnerId());
-                return AccessResult::allowedIf($is_owner)
+                $collabora_access = AccessResult::allowedIf($is_owner)
                     ->cachePerPermissions()
                     ->cachePerUser()
                     ->addCacheableDependency($media);
             }
-            return AccessResult::neutral()->cachePerPermissions();
-
-        default:
-            return AccessResult::neutral();
+            else {
+                $collabora_access = AccessResult::neutral()
+                    ->cachePerPermissions();
+            }
+            break;
     }
+
+    return $collabora_access->addCacheableDependency($media_view_access);
 }

tests/src/Functional/AccessTest.php

@@ -27,6 +27,14 @@
 
 /**
  * Tests access to collabora routes.
+ *
+ * There is a kernel test with a similar purpose.
+ * The kernel test is (obviously) a lot faster, while this functional test might
+ * be more realistic.
+ * For now, both of these tests are kept, but most new development will happen
+ * in the kernel test.
+ *
+ * @see \Drupal\Tests\collabora_online\Kernel\CollaboraMediaAccessTest
  */
 class AccessTest extends BrowserTestBase {
 
@@ -134,7 +142,13 @@ public function testCollaboraMediaPermissions(): void {
             'diary keeper' => $this->createUser([
                 // There is no 'preview own *' permission in this module.
                 'preview diary in collabora',
-                'edit own diary in collabora'
+                'edit own diary in collabora',
+                'view own unpublished media',
+            ]),
+            // Create a user without 'view own unpublished media'.
+            'public diary keeper' => $this->createUser([
+                'preview diary in collabora',
+                'edit own diary in collabora',
             ]),
         ];
 
@@ -145,7 +159,17 @@ public function testCollaboraMediaPermissions(): void {
             'own diary' => $this->createMediaEntity('diary', [
                 'uid' => $accounts['diary keeper']->id(),
             ]),
-            'other diary' => $this->createMediaEntity('diary'),
+            'other diary' => $this->createMediaEntity('diary', [
+                'uid' => $accounts['public diary keeper']->id(),
+            ]),
+            'own secret diary' => $this->createMediaEntity('diary', [
+                'status' => 0,
+                'uid' => $accounts['diary keeper']->id(),
+            ]),
+            'other secret diary' => $this->createMediaEntity('diary', [
+                'status' => 0,
+                'uid' => $accounts['public diary keeper']->id(),
+            ]),
         ];
 
         $paths = [];
@@ -162,10 +186,14 @@ public function testCollaboraMediaPermissions(): void {
                 '/cool/edit/<wiki>' => ['anonymous'],
                 '/cool/view/<announcement>' => ['anonymous'],
                 '/cool/edit/<announcement>' => [],
-                '/cool/view/<own diary>' => ['diary keeper'],
+                '/cool/view/<own diary>' => ['diary keeper', 'public diary keeper'],
                 '/cool/edit/<own diary>' => ['diary keeper'],
-                '/cool/view/<other diary>' => ['diary keeper'],
-                '/cool/edit/<other diary>' => [],
+                '/cool/view/<other diary>' => ['diary keeper', 'public diary keeper'],
+                '/cool/edit/<other diary>' => ['public diary keeper'],
+                '/cool/view/<own secret diary>' => ['diary keeper'],
+                '/cool/edit/<own secret diary>' => ['diary keeper'],
+                '/cool/view/<other secret diary>' => [],
+                '/cool/edit/<other secret diary>' => [],
             ],
             $accounts,
             $paths,