diff --git a/README.md b/README.md index 40f45d29..83c72505 100644 --- a/README.md +++ b/README.md @@ -197,24 +197,27 @@ 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 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. +For each media type, the module introduces four 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): 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 are the owner/author of that media entity. +- "(media type): Preview published media file in Collabora" + Users with this permission are allowed to preview documents attached + to a published media entity of the given type, using the Collabora + Online editor in preview/readonly mode. +- "(media type): Preview own unpublished media file in Collabora" + Users with this permission are allowed to preview documents attached + to an unpublished media entity of the given type, using the Collabora Online + editor in preview/readonly mode. + +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. Developers can use entity access hooks to alter which users may edit or preview media files in Collabora. This would allow to grant access diff --git a/collabora_online.module b/collabora_online.module index 61662aad..fd2cc452 100644 --- a/collabora_online.module +++ b/collabora_online.module @@ -126,21 +126,51 @@ function collabora_online_media_access(MediaInterface $media, string $operation, $type = $media->bundle(); switch ($operation) { case 'preview in collabora': - return AccessResult::allowedIfHasPermission($account, "preview $type in collabora"); + if ($media->isPublished()) { + return AccessResult::allowedIfHasPermission($account, "preview $type in collabora") + ->addCacheableDependency($media); + } + $preview_own_permission = "preview own unpublished $type in collabora"; + $access_result = AccessResult::allowedIfHasPermission($account, $preview_own_permission) + ->addCacheableDependency($media); + if (!$access_result->isAllowed()) { + return $access_result; + } + // Use '==' because Drupal sometimes loads integers as strings. + $is_owner = ($account->id() && $account->id() == $media->getOwnerId()); + if ($is_owner) { + $access_result = AccessResult::allowed(); + } + else { + $access_result = AccessResult::neutral() + ->setReason("The user has the '$preview_own_permission' permission, but is not the owner of the media item."); + } + return $access_result + ->cachePerUser() + ->addCacheableDependency($media); case 'edit in collabora': if ($account->hasPermission("edit any $type in collabora")) { - return AccessResult::allowed()->cachePerPermissions(); + return AccessResult::allowed() + ->cachePerPermissions(); } - if ($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) - ->cachePerPermissions() - ->cachePerUser() - ->addCacheableDependency($media); + $edit_own_permission = "edit own $type in collabora"; + $access_result = AccessResult::allowedIfHasPermission($account, $edit_own_permission); + if (!$access_result->isAllowed()) { + return $access_result; + } + // Use '==' because Drupal sometimes loads integers as strings. + $is_owner = ($account->id() && $account->id() == $media->getOwnerId()); + if (!$is_owner) { + $access_result = AccessResult::neutral() + ->setReason("The user has the '$edit_own_permission' permission, but is not the owner of the media item."); + } + else { + $access_result = AccessResult::allowed(); } - return AccessResult::neutral()->cachePerPermissions(); + return $access_result + ->cachePerUser() + ->addCacheableDependency($media); default: return AccessResult::neutral(); diff --git a/src/CollaboraMediaPermissions.php b/src/CollaboraMediaPermissions.php index 956948d5..2eecaf31 100644 --- a/src/CollaboraMediaPermissions.php +++ b/src/CollaboraMediaPermissions.php @@ -77,7 +77,10 @@ protected function buildPermissions(MediaTypeInterface $type) { return [ "preview $type_id in collabora" => [ - 'title' => $this->t('%type_name: Preview media file in Collabora', $type_params), + 'title' => $this->t('%type_name: Preview published media file in Collabora', $type_params), + ], + "preview own unpublished $type_id in collabora" => [ + 'title' => $this->t('%type_name: Preview own unpublished media file in Collabora', $type_params), ], "edit own $type_id in collabora" => [ 'title' => $this->t('%type_name: Edit own media file in Collabora', $type_params), diff --git a/tests/src/Functional/AccessTest.php b/tests/src/Functional/AccessTest.php deleted file mode 100644 index 816e8f99..00000000 --- a/tests/src/Functional/AccessTest.php +++ /dev/null @@ -1,238 +0,0 @@ -createMediaType('file', ['id' => 'document']); - $media = $this->createMediaEntity('document'); - $media_id = $media->id(); - - $users = [ - 'anonymous' => new AnonymousUserSession(), - 'authenticated' => $this->createUser(), - 'admin' => $this->createUser(admin: TRUE), - ]; - - // Build a report and assert the full result at once. - // This provides a very complete picture in case the assertion fails. - $this->assertPathsAccessByUsers( - [ - // Test the front page as an example that everybody can access. - '/' => ['anonymous', 'authenticated', 'admin'], - // Test an administration page that only admin can see. - '/admin/config' => ['admin'], - // Test the user route. - '/user/' . $users['authenticated']->id() => ['authenticated', 'admin'], - // Test the core media route for reference. - "/media/$media_id/edit" => ['admin'], - ], - $users, - ); - } - - /** - * Tests a scenario when only the administrator has access. - */ - public function testOnlyAdminHasAccess(): void { - $this->createMediaType('file', ['id' => 'document']); - $media = $this->createMediaEntity('document'); - - $users = [ - 'anonymous' => new AnonymousUserSession(), - 'authenticated' => $this->createUser(), - 'admin' => $this->createUser(admin: TRUE), - ]; - - // Both routes are only accessible for admin. - $this->assertPathsAccessByUsers( - [ - '/cool/view/' . $media->id() => ['admin'], - '/cool/edit/' . $media->id() => ['admin'], - ], - $users, - ); - } - - /** - * Tests a scenario where specific permissions are given to users. - */ - public function testCollaboraMediaPermissions(): void { - $this->createMediaType('file', ['id' => 'document']); - $this->createMediaType('file', ['id' => 'public_wiki']); - $this->createMediaType('file', ['id' => 'public_announcement']); - $this->createMediaType('file', ['id' => 'diary']); - $this->grantPermissions( - Role::load(RoleInterface::ANONYMOUS_ID), - [ - 'preview public_announcement in collabora', - 'preview public_wiki in collabora', - 'edit any public_wiki in collabora', - ], - ); - - $accounts = [ - 'anonymous' => new AnonymousUserSession(), - 'authenticated' => $this->createUser(), - 'reader' => $this->createUser([ - 'preview document in collabora', - ]), - 'editor' => $this->createUser([ - 'preview document in collabora', - 'edit any document in collabora', - ]), - // The 'writer' has write access, but no read access. - 'writer' => $this->createUser([ - 'edit any document in collabora', - ]), - 'diary keeper' => $this->createUser([ - // There is no 'preview own *' permission in this module. - 'preview diary in collabora', - 'edit own diary in collabora', - ]), - ]; - - $media_entities = [ - 'document' => $this->createMediaEntity('document'), - 'wiki' => $this->createMediaEntity('public_wiki'), - 'announcement' => $this->createMediaEntity('public_announcement'), - 'own diary' => $this->createMediaEntity('diary', [ - 'uid' => $accounts['diary keeper']->id(), - ]), - 'other diary' => $this->createMediaEntity('diary'), - ]; - - $paths = []; - foreach ($media_entities as $media_key => $media) { - $paths["/cool/view/<$media_key>"] = '/cool/view/' . $media->id(); - $paths["/cool/edit/<$media_key>"] = '/cool/edit/' . $media->id(); - } - - $this->assertPathsAccessByUsers( - [ - '/cool/view/' => ['reader', 'editor'], - '/cool/edit/' => ['editor', 'writer'], - '/cool/view/' => ['anonymous'], - '/cool/edit/' => ['anonymous'], - '/cool/view/' => ['anonymous'], - '/cool/edit/' => [], - '/cool/view/' => ['diary keeper'], - '/cool/edit/' => ['diary keeper'], - '/cool/view/' => ['diary keeper'], - '/cool/edit/' => [], - ], - $accounts, - $paths, - ); - } - - /** - * Builds a report about which users can access a given content. - * - * @param array> $expected - * Array indicating which url should be accessible by which user. - * The array keys are either paths or string keys from the $paths array. - * The array values are lists of keys from the $accounts array with access - * to that path. - * @param array $accounts - * Accounts to test access with, keyed by a distinguishable name. - * @param array|null $paths - * An array of paths, or NULL to just use the array keys from $expected. - * This parameter is useful if the paths all look very similar. - */ - protected function assertPathsAccessByUsers(array $expected, array $accounts, ?array $paths = NULL): void { - if ($paths === NULL) { - $paths = array_keys($expected); - $paths = array_combine($paths, $paths); - } - // Build a report and assert it all at once, to have a more complete - // overview on failure. - $actual = []; - foreach ($paths as $path_key => $path) { - $url = Url::fromUserInput($path); - // Filter the user list by access to the url. - $accounts_with_access = array_filter($accounts, $url->access(...)); - $actual[$path_key] = array_keys($accounts_with_access); - } - // Use yaml to avoid integer keys in list output. - $this->assertSame( - Yaml::encode($expected), - Yaml::encode($actual), - 'Users with access to given paths' - ); - } - - /** - * Creates a media entity with attached file. - * - * @param string $type - * Media type. - * @param array $values - * Values for the media entity. - * - * @return \Drupal\media\MediaInterface - * New media entity. - */ - protected function createMediaEntity(string $type, array $values = []): MediaInterface { - file_put_contents('public://test.txt', 'Hello test'); - $file = File::create([ - 'uri' => 'public://test.txt', - ]); - $file->save(); - $values += [ - 'bundle' => $type, - 'field_media_file' => $file->id(), - ]; - $media = Media::create($values); - $media->save(); - return $media; - } - -} diff --git a/tests/src/Kernel/CollaboraMediaAccessTest.php b/tests/src/Kernel/CollaboraMediaAccessTest.php new file mode 100644 index 00000000..fd9a9c42 --- /dev/null +++ b/tests/src/Kernel/CollaboraMediaAccessTest.php @@ -0,0 +1,321 @@ +installEntitySchema('user'); + $this->installEntitySchema('file'); + $this->installSchema('file', 'file_usage'); + $this->installEntitySchema('media'); + $this->installConfig([ + 'field', + 'system', + 'user', + 'image', + 'file', + 'media', + ]); + + $this->createMediaType('file', ['id' => 'document']); + $this->createMediaType('file', ['id' => 'book']); + + // Consume the user id 1. + $this->createUser(); + } + + /** + * Tests media access for Collabora routes and operations. + */ + public function testCollaboraMediaAccess(): void { + $this->assertCollaboraMediaAccess( + [], + new AnonymousUserSession(), + 'No access for anonymous without permissions', + ); + + // Check authenticated with irrelevant permissions. + // This also covers the "no permissions" case. + $this->assertCollaboraMediaAccess( + [], + $this->createUser([ + // Add general 'media' permissions. + 'administer media types', + 'view media', + 'update any media', + 'view own unpublished media', + // Add Collabora permissions for a different media type. + 'preview book in collabora', + 'preview own unpublished book in collabora', + 'edit any book in collabora', + 'edit own book in collabora', + ]), + 'No access with irrelevant permissions', + ); + + $this->assertCollaboraMediaAccessForPermission( + [ + 'published document' => ['preview'], + 'own published document' => ['preview'], + ], + 'preview document in collabora', + ); + + $this->assertCollaboraMediaAccessForPermission( + [ + 'own unpublished document' => ['preview'], + ], + 'preview own unpublished document in collabora', + ); + + $this->assertCollaboraMediaAccessForPermission( + [ + 'published document' => ['edit'], + 'unpublished document' => ['edit'], + 'own published document' => ['edit'], + 'own unpublished document' => ['edit'], + ], + 'edit any document in collabora', + ); + + $this->assertCollaboraMediaAccessForPermission( + [ + 'own published document' => ['edit'], + 'own unpublished document' => ['edit'], + ], + 'edit own document in collabora', + ); + + // The 'administer media' permission grants access to everything. + $this->assertCollaboraMediaAccessForPermission( + [ + 'published document' => ['preview', 'edit'], + 'unpublished document' => ['preview', 'edit'], + 'own published document' => ['preview', 'edit'], + 'own unpublished document' => ['preview', 'edit'], + ], + 'administer media', + ); + + $this->assertCollaboraMediaAccess( + [ + 'published document' => ['preview', 'edit'], + 'unpublished document' => ['preview', 'edit'], + 'own published document' => ['preview', 'edit'], + 'own unpublished document' => ['preview', 'edit'], + ], + $this->createUser(admin: TRUE), + "Access for admin user", + ); + } + + /** + * Tests scenarios where the anonymous user has more permissions. + * + * This verifies the special treatment of uid 0 to determine the owner of a + * media entity. + */ + public function testAnonymousOwnAccess(): void { + user_role_grant_permissions(RoleInterface::ANONYMOUS_ID, [ + 'preview own unpublished document in collabora', + 'edit own document in collabora', + ]); + $this->assertCollaboraMediaAccess( + [], + new AnonymousUserSession(), + "Anonymous user with '... own ...' permissions.", + ); + + user_role_grant_permissions(RoleInterface::ANONYMOUS_ID, [ + 'preview document in collabora', + 'edit any document in collabora', + ]); + $this->assertCollaboraMediaAccess( + [ + 'published document' => ['preview', 'edit'], + 'unpublished document' => ['edit'], + 'own published document' => ['preview', 'edit'], + 'own unpublished document' => ['edit'], + ], + new AnonymousUserSession(), + "Anonymous user with all Collabora media permissions.", + ); + } + + /** + * Creates a user with one permission, and asserts access to media entities. + * + * @param array> $expected + * Expected access. + * @param string $permission + * Permission machine name. + */ + protected function assertCollaboraMediaAccessForPermission(array $expected, string $permission): void { + $account = $this->createUser([$permission]); + $message = "User with '$permission' permission."; + $this->assertCollaboraMediaAccess($expected, $account, $message); + } + + /** + * Asserts Collabora media access for a user account. + * + * @param array $expected + * Expected access matrix. + * The keys identify media entities that are created in this test. + * The values identify operations. + * @param \Drupal\Core\Session\AccountInterface $account + * Account to check access for. + * @param string $message + * Message for the assertion. + */ + protected function assertCollaboraMediaAccess(array $expected, AccountInterface $account, string $message): void { + $other_user = $this->createUser(); + $entities = [ + "published document" => $this->createMediaEntity('document', [ + 'uid' => $other_user->id(), + ]), + "unpublished document" => $this->createMediaEntity('document', [ + 'uid' => $other_user->id(), + 'status' => 0, + ]), + "own published document" => $this->createMediaEntity('document', [ + 'uid' => $account->id(), + ]), + "own unpublished document" => $this->createMediaEntity('document', [ + 'uid' => $account->id(), + 'status' => 0, + ]), + ]; + + // Test $entity->access() with different operations on all entities. + $operations = [ + 'preview' => 'preview in collabora', + 'edit' => 'edit in collabora', + ]; + $actual_entity_access = []; + foreach ($entities as $entity_key => $entity) { + foreach ($operations as $operation_key => $operation) { + $has_entity_access = $entity->access($operation, $account); + if ($has_entity_access) { + $actual_entity_access[$entity_key][] = $operation_key; + } + } + } + $this->assertSameYaml( + $expected, + $actual_entity_access, + 'Entity access: ' . $message, + ); + + // Test path access. + // The result is expected to be exactly the same, due to how the route + // access is configured. + // Testing the paths like this introduces some level of redundancy or + // duplication, but it is cheap and easy, so for now this is what we do. + $sprintf_path_patterns = [ + 'preview' => '/cool/view/%s', + 'edit' => '/cool/edit/%s', + ]; + $actual_path_access = []; + foreach ($entities as $entity_key => $entity) { + foreach ($sprintf_path_patterns as $pattern_key => $sprintf_path_pattern) { + $path = sprintf($sprintf_path_pattern, $entity->id()); + $has_path_access = Url::fromUserInput($path)->access($account); + if ($has_path_access) { + $actual_path_access[$entity_key][] = $pattern_key; + } + } + } + $this->assertSameYaml( + $expected, + $actual_path_access, + 'Path access: ' . $message, + ); + } + + /** + * Creates a media entity with attached file. + * + * @param string $type + * Media type. + * @param array $values + * Values for the media entity. + * + * @return \Drupal\media\MediaInterface + * New media entity. + */ + protected function createMediaEntity(string $type, array $values = []): MediaInterface { + file_put_contents('public://test.txt', 'Hello test'); + $file = File::create([ + 'uri' => 'public://test.txt', + ]); + $file->save(); + $values += [ + 'bundle' => $type, + 'field_media_file' => $file->id(), + ]; + $media = Media::create($values); + $media->save(); + return $media; + } + + /** + * Asserts that two values are the same when exported to yaml. + * + * This provides a nicer diff output, without numeric array keys. + * + * @param mixed $expected + * Expected value. + * @param mixed $actual + * Actual value. + * @param string $message + * Message. + */ + protected function assertSameYaml(mixed $expected, mixed $actual, string $message = ''): void { + $this->assertSame( + "\n" . Yaml::encode($expected), + "\n" . Yaml::encode($actual), + $message, + ); + } + +} diff --git a/tests/src/Functional/PermissionTest.php b/tests/src/Kernel/PermissionTest.php similarity index 74% rename from tests/src/Functional/PermissionTest.php rename to tests/src/Kernel/PermissionTest.php index 4023bc1a..20f9739a 100644 --- a/tests/src/Functional/PermissionTest.php +++ b/tests/src/Kernel/PermissionTest.php @@ -12,30 +12,46 @@ declare(strict_types=1); -namespace Drupal\Tests\collabora_online\Functional; +namespace Drupal\Tests\collabora_online\Kernel; -use Drupal\Tests\BrowserTestBase; +use Drupal\KernelTests\KernelTestBase; use Drupal\Tests\media\Traits\MediaTypeCreationTrait; +use Drupal\Tests\user\Traits\UserCreationTrait; use Drupal\user\PermissionHandlerInterface; /** * Tests dynamically created permissions. */ -class PermissionTest extends BrowserTestBase { +class PermissionTest extends KernelTestBase { use MediaTypeCreationTrait; + use UserCreationTrait; /** * {@inheritdoc} */ protected static $modules = [ 'collabora_online', + 'media', + 'user', + 'field', + 'system', + 'file', + 'image', ]; /** * {@inheritdoc} */ - protected $defaultTheme = 'stark'; + protected function setUp(): void { + parent::setUp(); + + $this->installEntitySchema('user'); + $this->installEntitySchema('file'); + $this->installSchema('file', 'file_usage'); + $this->installEntitySchema('media'); + $this->installConfig(['field', 'system', 'user', 'file', 'media']); + } /** * Tests that dynamic permissions are properly created. @@ -80,8 +96,12 @@ static function (array $permission) { 'title' => 'Public wiki: Edit own media file in Collabora', 'dependencies' => ['config' => ['media.type.public_wiki']], ], + 'preview own unpublished public_wiki in collabora' => [ + 'title' => 'Public wiki: Preview own unpublished media file in Collabora', + 'dependencies' => ['config' => ['media.type.public_wiki']], + ], 'preview public_wiki in collabora' => [ - 'title' => 'Public wiki: Preview media file in Collabora', + 'title' => 'Public wiki: Preview published media file in Collabora', 'dependencies' => ['config' => ['media.type.public_wiki']], ], ], $permissions);