[WIP] Issue #27: Fix permissions #29

donquixote · 2024-09-13T16:45:42Z

No description provided.

donquixote · 2024-09-13T16:49:33Z

src/Cool/CoolRequest.php

+
+    if (!$res) {
+        \Drupal::messenger()->addError(t('Cannot fetch from @url.', ['@url' => $discovery_url]));
+    }


This actually happened to me all the time and slowed things down.
Not sure if we can safely include it in this PR.

The question is which version of Collabora Online are you using. I'm currently tracking down an issue with a recent change that the Drupal module seem to trigger. And maybe it is related.

Whichever version we get from using the 'collabora/code' docker image.

indeed it's recent enough to have the issue.

If I apply this patch alone with the two extra lines for the SSL verification, it actually solve the problem in CollaboraOnline/online#10042

I will isolate the patch from this PR, and make a separate PR.
The tests no (longer) depend on the connection to collabora, so it does not impact them.

Removed from the PR for now.

donquixote · 2024-09-13T16:55:29Z

collabora_online.module

+    AccountInterface $account,
+): AccessResultInterface {
+    // @todo Should it make a difference whether a media is published or not?
+    // @todo Should a user be allowed to preview/edit a media in Collabora even


These points are TBD.

For regular media access in core:

With 'administer media' you can do anything.

(Generic "update any media" and "update media" and others are being faded out, I don't mention them below, see https://www.drupal.org/project/drupal/issues/2925459.)

(Create and delete are not relevant for us, I don't mention them below.)

Otherwise:

To view published media, it needs "view media" permission.

To view own unpublished media, it needs "view own unpublished media" permission to view.

To view other people's unpublished media, you need "administer media".

To update any media (published or not), you need to:

"edit any media", OR.

"edit own media", and be the owner.

This actually reveals a lack in Drupal core.
See https://www.drupal.org/project/drupal/issues/2936652
Currently there are permissions to "edit any media" which also applies to unpublished, and does not require ownership.
But to view the same media, you need to be the owner or have "administer media".

I am removing these @todo after talking with my colleagues.
In the current iteration, we make no distinction of published vs not published.
Also we treat these operations as isolated from other media operations.
Instead, we could add description to the permission itself.

donquixote · 2024-09-16T09:26:59Z

Drupal conventions set the indentation level to 2 spaces.
For now I am keeping the 4 spaces even for new code, to keep it consistent within this module.
In the future this can be fixed for the entire module with a separate PR.

donquixote · 2024-09-16T09:37:24Z

src/CollaboraMediaPermissions.php

+      "edit own $type_id in collabora" => [
+        'title' => $this->t('%type_name: Edit own media file in Collabora', $type_params),
+      ],
+      "edit any $type_id in collabora" => [


We had internal spec which said "edit all", but "edit any" is more aligned with core permissions.

Also, "edit any xyz" works better with singular "xyz". With "edit all xyz", xyz would have to be plura.

donquixote · 2024-09-16T09:49:57Z

src/Cool/CoolRequest.php

+    $res = curl_exec($curl);
+
+    if (!$res) {
+        \Drupal::messenger()->addError(t('Cannot fetch from @url.', ['@url' => $discovery_url]));


Here I just log the error but then proceed to return false.
The calling code already does have a condition for return value false.
We could improve the error handling in the future.

donquixote · 2024-09-16T10:08:05Z

README.md

-
-The user role `anonymous` by default disallows editing and is the
-minimal permission for viewing documents in Collabora Online.
-


We can rethink this section. For now I just removed it to avoid confusion.

Right now it is unclear how these new permissions work. Is it possible to have a short write up here?

donquixote · 2024-09-16T10:36:54Z

How to run the tests.

For now I am not adding a phpunit.xml(.dist) file.
This would only make sense if we also have a recommended/prepared way to set up a local development instance.

I personally ran the tests with ddev-drupal-contrib.
Unfortunately, I was not able to make the Collabora instance work, this needs more tweaking. But the tests don't need it.

I assume that Francesco will probably run the tests from a custom project where this module is pulled in as a dependency.
I actually replicated this setup, but can't get the tests to run..

So it is up to the developer/reviewer how they want to run the tests.

donquixote · 2024-09-16T10:39:54Z

src/Controller/ViewerController.php

-    }
+    public function __construct(
+        private readonly RendererInterface $renderer,
+    ) {}


This change simplifies the class by using constructor property promotion.
It is now a bit unnecessary in the final version of the PR, but it made more sense with prior commits in this PR where the constructor parameters were changed. I suggest to keep it.

you did not do it for CollaboraMediaPermissions

so true, bad copy paste

hfiguiere · 2024-09-16T13:34:24Z

src/Cool/CoolRequest.php

+        // stream_context_create(), the 'verify_peer' and 'verify_peer_name'
+        // options were set.
+        // @todo Check if an equivalent to 'verify_peer_name' exists for curl.
+        CURLOPT_SSL_VERIFYPEER => !$disable_checks,


adding

CURLOPT_SSL_VERIFYSTATUS => !$disable_checks, CURLOPT_SSL_VERIFYHOST => !$disable_checks,

seems to work.

Can you tell me what is different with or without these additions?
I am happy to add it but I like to understand why.

Or how we could test it.

my setup use self signed certificates (it's local to my workstation) and without these two option it go through.

I think maybe of the two just CURLOPT_SSL_VERIFYHOST is needed (it doesn't work without,a nd removing CURLOPT_SSL_VERIFYSTATUS seems to be fine). It seems to be the equivalent of verify_peer_name.

Actually it should be

CURLOPT_SSL_VERIFYHOST => $disable_checks ? 0 : 2,

as per https://www.php.net/manual/en/curl.constants.php#constant.curlopt-ssl-verifyhost

drishu · 2024-09-17T12:19:52Z

src/CollaboraMediaPermissions.php

+/**
+ * Provides permissions for Collabora per media type.
+ *
+ * This class is registered in collabora_online.permissions.yml.


for me the '@see' is enough

drishu · 2024-09-17T12:20:05Z

src/CollaboraMediaPermissions.php

+  protected $entityTypeManager;
+
+  /**
+   * MediaPermissions constructor.


Suggested change

* MediaPermissions constructor.

* CollaboraMediaPermissions constructor.

oops lazy copy paste.
Personally I like just "Constructor." but ok for me.
See https://www.drupal.org/project/coding_standards/issues/3457897 which has different opinions.

I checked core and oe_* modules. We have a mix of "Constructs a *** instance" and "*** constructor". I also see the occasional "Constructor." which could have been me and was accepted in a review at the time it happened.

drishu · 2024-09-17T12:23:58Z

collabora_online.routing.yml

@@ -11,13 +12,16 @@ collabora-online.view:
      edit:
        type: boolean
  requirements:
-    _permission: 'access content'
+    # Media id must be an integer.


It's evident from the code, I would not add this comment

drishu · 2024-09-17T13:07:50Z

src/Controller/ViewerController.php

-    }
+    public function __construct(
+        private readonly RendererInterface $renderer,
+    ) {}


you did not do it for CollaboraMediaPermissions

drishu · 2024-09-17T13:14:34Z

src/Controller/WopiController.php

            \Drupal::logger('cool')->error('Token and user permissions do not match.');
            return static::permissionDenied();
        }

-        $user_picture = $user->user_picture->entity;
+        $user_picture = $user->user_picture?->entity;


I might actually split it out of the PR.

drishu · 2024-09-17T13:17:48Z

src/Cool/CoolRequest.php

-            'verify_peer_name'  => !$disable_checks,
-        ]]);
-    $res = file_get_contents($discovery_url, false, $stream_context);
+    // Previously, file_get_contents() was used to fetch the discovery xml data.


let's have a clean PR only with stuff related to our issue

yeah, see comments below.
It first seemed necessary to make things work, but now we can skip it.

Also, the problem was addressed in COOL so it shouldn't be necessary per see.

drishu · 2024-09-17T13:19:38Z

src/Plugin/Field/FieldFormatter/CoolPreview.php

+        $media = $items->getEntity();
+        if (!$media instanceof MediaInterface) {
+            // The formatter is being used on the wrong entity type.
+            // @todo Log this.


todos become wont dos, think its better to have these todos grouped in issues

There will need to be an issue about error handling, then we can clean this up.
To me, the @todo indicates that the code as it is now is not intentionally this way but because we did not have the capacity or scope to do it properly.
But I can drop it.

New issue:

Restrict the field formatter to media entity type, or make it work for other types #32

drishu · 2024-09-17T14:38:00Z

tests/src/Functional/AccessTest.php

+     *   New media entity.
+     */
+    protected function createMediaEntity(string $type, array $values = []): MediaInterface {
+        file_put_contents('public://test.txt', 'Hello test');


use \Drupal::service('file_system')->copy(

We are not copying, we just put file content.. to copy we have to put an example file. or maybe use one that already exists.

yes, I would go with the whole sh'bang, put a fixtures folder under test etc etc

I actually find a bunch of file_put_contents('public://' . in core tests.
I would prefer to keep this simple snippet unless you can point out what could possibly go wrong.

drishu · 2024-09-17T14:41:33Z

tests/src/Functional/AccessTest.php

+     * @return array
+     *   Report to compare against in assertions.
+     */
+    protected function buildAccessReport(array $paths, array $users): array {


this is too cryptic for me (and prob most people), in the test I need to see clearly that role x and y have or not permission for routes. I can guess what this '/cool/view/<document>' => ['reader', 'editor'], means but dont really know until I compare what buildAccessReport is actually composing

The benefit of the "compare big array" is that you see in one big overview what you need to fix.
if we do separate assertions, we only see the first failure which would say "user x has no access to path y".
knowing which users have access and which don't is quite useful for troubleshooting.

Could we resolve this confusion with comments or improved assertion message?
Or is the problem in the "magicalness" of the buildAccessReport()?

magicalness yes, need to be more readable without debuging

drishu · 2024-09-17T14:43:08Z

tests/src/Functional/AccessTest.php

+    /**
+     * Tests a scenario where specific permissions are given to users.
+     */
+    public function testCollaboraMediaPermissions(): void {


you could rather build a simpler test that receives data from a yeilding function (doesn't need to be marked as a provider, we can just loop)

donquixote · 2024-09-18T10:58:10Z

tests/src/Functional/AccessTest.php

+            ],
+            $accounts,
+            $paths,
+        );


@drishu I think this is simpler, is it not?

donquixote · 2024-09-18T11:30:13Z

src/Controller/WopiController.php

+        $media = \Drupal::entityTypeManager()->getStorage('media')->load($id);
+        if (!$media) {
+            // @todo Use default mechanism for access denied response.
+            return static::permissionDenied();


Comment should be changed.

donquixote · 2024-09-18T13:01:40Z

@hfiguiere or whoever wants to merge this:
Please do not squash, keep the history. The commits are very intentionally crafted.

AaronGilMartinez · 2024-09-19T11:09:37Z

tests/src/Functional/PermissionTest.php

+use Drupal\Tests\media\Traits\MediaTypeCreationTrait;
+use Drupal\Tests\TestFileCreationTrait;
+use Drupal\user\Entity\Role;
+use Drupal\user\PermissionHandlerInterface;


Most of the classes aren't used, please removed unused.

AaronGilMartinez · 2024-09-19T11:10:09Z

tests/src/Functional/PermissionTest.php

+     * {@inheritdoc}
+     */
+    protected static $modules = [
+        'media',


Not needed since its a module dependency.
Will be added on install.

AaronGilMartinez · 2024-09-19T15:16:00Z

Changes about permissions are ready @hfiguiere

src/CollaboraMediaPermissions.php

hfiguiere · 2024-09-23T18:06:23Z

src/Controller/ViewerController.php

-
-        /* Make sure that the user is a collaborator if edit is true */
-        $edit = $edit && $permissions['is_collaborator'];
-


How do we check about the write permission? The code above make sure that if you get an "editor" but don't have write permission as per drupal, then you get bounced back to a read-only. It also reject if you don't have viewing permission. This is better than WOPI errors in Collabora online.

Permissions are set at the entity level and can be found in the collabora_online.module. They handle actions like viewing and editing documents. These permissions are not tied to specific roles, allowing sites to configure which roles can access or modify documents.

tests/src/Functional/PermissionTest.php

tests/src/Functional/AccessTest.php

This removes noise from following commits.

…upal permissions. Also remove roles configuration which is no longer needed now.

…README.md, for now. A new version of this section may be re-added later.

…eration.

…n the field formatter.

…ormatter.

…on links, instead of regular media access.

…definition for view and edit routes.

…perms and 'edit own'.

…sionDenied(), drop the `@todo` in the controller method.

AaronGilMartinez · 2024-10-14T13:44:14Z

Hi @hfiguiere, added license for the files, and rebased branch.
Can you check, please?
Hope is clearer about permissions, thanks!

donquixote force-pushed the issue-27-fix-permissions branch from 24d8c60 to dc42d13 Compare September 13, 2024 16:46

donquixote commented Sep 13, 2024

View reviewed changes

donquixote force-pushed the issue-27-fix-permissions branch 2 times, most recently from b969749 to 3707404 Compare September 13, 2024 16:54

donquixote commented Sep 13, 2024

View reviewed changes

donquixote force-pushed the issue-27-fix-permissions branch from 3707404 to 229c013 Compare September 16, 2024 09:25

donquixote commented Sep 16, 2024

View reviewed changes

donquixote force-pushed the issue-27-fix-permissions branch from 229c013 to bcb49de Compare September 16, 2024 10:06

donquixote commented Sep 16, 2024

View reviewed changes

donquixote force-pushed the issue-27-fix-permissions branch 2 times, most recently from e154762 to 0972d4a Compare September 16, 2024 10:11

donquixote commented Sep 16, 2024

View reviewed changes

hfiguiere reviewed Sep 16, 2024

View reviewed changes

drishu suggested changes Sep 17, 2024

View reviewed changes

donquixote force-pushed the issue-27-fix-permissions branch from 0972d4a to e0a2c08 Compare September 18, 2024 10:53

donquixote commented Sep 18, 2024

View reviewed changes

tests/src/Functional/AccessTest.php

],

$accounts,

$paths,

);

Copy link

Author

donquixote Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drishu I think this is simpler, is it not?

donquixote commented Sep 18, 2024

View reviewed changes

donquixote force-pushed the issue-27-fix-permissions branch 3 times, most recently from 5ceeeac to d34d7d0 Compare September 18, 2024 12:58

drishu approved these changes Sep 18, 2024

View reviewed changes

AaronGilMartinez suggested changes Sep 19, 2024

View reviewed changes

AaronGilMartinez approved these changes Sep 19, 2024

View reviewed changes

hfiguiere reviewed Sep 23, 2024

View reviewed changes

src/CollaboraMediaPermissions.php Show resolved Hide resolved

hfiguiere reviewed Sep 23, 2024

View reviewed changes

tests/src/Functional/PermissionTest.php Show resolved Hide resolved

hfiguiere reviewed Sep 23, 2024

View reviewed changes

tests/src/Functional/AccessTest.php Show resolved Hide resolved

donquixote added 2 commits October 14, 2024 14:03

Issue CollaboraOnline#27: Clean up and reorder imports.

b28b42d

This removes noise from following commits.

Issue CollaboraOnline#27: Replace the 'is_anonymous' pseudo-permission.

dcaf2e8

AaronGilMartinez force-pushed the issue-27-fix-permissions branch from 723aebc to 847f29d Compare October 14, 2024 12:26

donquixote and others added 17 commits October 14, 2024 14:39

Issue CollaboraOnline#27: Replace usage of pseudo-permissions with Dr…

6d0f91c

…upal permissions. Also remove roles configuration which is no longer needed now.

Issue CollaboraOnline#27: Remove the 'User permissions' section from …

67f330a

…README.md, for now. A new version of this section may be re-added later.

Issue CollaboraOnline#27: Simplify a condition in wopiCheckFileInfo().

48e64d5

Issue CollaboraOnline#27: Require PHP >= 8.1 to allow for 'readonly'.

864381b

Issue CollaboraOnline#27: Add a config schema.

958e303

Issue CollaboraOnline#27: Replace access checks with entity access op…

97dae81

…eration.

Issue CollaboraOnline#27: Use the media entity from the field items i…

b388a45

…n the field formatter.

Issue CollaboraOnline#27: Rename $element -> $elements in the field f…

54f92b9

…ormatter.

Issue CollaboraOnline#27: Check Collabora access in the field formatter.

7f3eb61

Issue CollaboraOnline#27: Check the new operations for entity operati…

8f144b6

…on links, instead of regular media access.

Issue CollaboraOnline#27: Move controller access checks to the route …

f1ca49f

…definition for view and edit routes.

Issue CollaboraOnline#27: Replace global permissions with per-bundle …

71c2c47

…perms and 'edit own'.

Issue CollaboraOnline#27: Test access to routes.

8e39e10

Issue CollaboraOnline#27: Add some @todo for possibly empty variables.

d424b78

Issue CollaboraOnline#27: Put a doc comment on WopiController::permis…

46b5b5f

…sionDenied(), drop the `@todo` in the controller method.

Issue CollaboraOnline#27: Remove uneeded classes and module in tests.

ca80fee

Issue CollaboraOnline#27: Add license.

805efa2

AaronGilMartinez force-pushed the issue-27-fix-permissions branch from 847f29d to 805efa2 Compare October 14, 2024 12:42


		The user role `anonymous` by default disallows editing and is the
		minimal permission for viewing documents in Collabora Online.

	* MediaPermissions constructor.
	* CollaboraMediaPermissions constructor.


		/* Make sure that the user is a collaborator if edit is true */
		$edit = $edit && $permissions['is_collaborator'];

[WIP] Issue #27: Fix permissions #29

Are you sure you want to change the base?

[WIP] Issue #27: Fix permissions #29

Conversation

donquixote commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donquixote Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donquixote commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donquixote commented Sep 16, 2024 • edited Loading

How to run the tests.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donquixote commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronGilMartinez commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronGilMartinez commented Oct 14, 2024

donquixote Sep 16, 2024 •

edited

Loading

donquixote commented Sep 16, 2024 •

edited

Loading