Issue #27: Use Drupal permission system

[donquixote]

Fixes #27

Changes:

• Replace the custom role settings with regular Drupal permissions.
• Have separate permissions per media type.
• Use the entity access API to manage access checks for media preview and edit with Collabora Online. This allows other modules to alter the access, e.g. to grant access based on a group membership.
• Move access checking logic out of the controller, and add them as requirements on the route. As a consequence, the fallback behavior will no longer work.
• Make sure that edit and preview links in different places only show up to eligible users.

[donquixote]

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]

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

[hfiguiere]

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

[donquixote]

I re-added a section, I hope it is more clear this way.

The "which can be managed at /admin/people/permissions." should be obvious to people who are familiar with Drupal, but I mention it anyway.

[donquixote]

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]

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

[AaronGilMartinez]

Changes about permissions are ready @hfiguiere

[hfiguiere]

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.

[AaronGilMartinez]

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.

[donquixote]

In the new version the access is checked at the route level.
This means that, if you attempt to access '/cool/edit/{media}' path when you only have "preview" permission, you get an access denied page, instead of the "readonly" fallback.

Also, now it is technically possible to have an "edit" permission without having a "preview" permission.
Such a user would be able to access the '/cool/edit/{media}' path, but not the '/cool/view/{media}' path.
It would be up to the admin / webmaster / site builder to only assign "edit" to roles that also have "preview".

How important is the scenario with the fallback?
One use case I could imagine is that user A with edit access passes a link to user B who has only read/preview access. In that case we would want user B to see the preview page.

If this is really important, we would need to move part of the the access logic back into the controller.
On the other hand, having the permission on the route level allows Drupal to hide the "edit" link for people with no access, whenever it would be displayed in a menu or in "tabs". (Currently no such places exist for these specific links, where the visibility would depend on the route access check.)

[donquixote]

Permissions are set at the entity level

In the new version the access is checked at the route level.

This sounds contradictory, but in fact both of these statements are true :)
The access check happens at the route level, so before the controller method is called.
However, the route access check then invokes the entity access system, with _entity_access: 'media.edit in collabora'.

[donquixote]

Having the access check on route level, and not in the controller, is a standard practice in Drupal for most routes, providing a clean separation between controller and access check. With all the consequences mentioned above, e.g. when you visit /node/123/edit, if you don't have edit access, you get an access denied, not a fallback to the node view page.

Exceptions are technically possible, e.g. for API urls that should be accessed by a script or service, and where a default "access denied" response would not be suitable.

In this case we would prefer to stick to the default Drupal behavior, at least for now.
This should be ok because we also hide the link for user without edit access.

[AaronGilMartinez]

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

[hfiguiere]

this comment #29 (comment) would benefit being added to the README

[donquixote]

this comment #29 (comment) would benefit being added to the README

So this would explain how to run tests and also how to manually test the module with a Collabora Online docker container.

I can agree this would be useful, but I would prefer to keep it out of this pull request.
Currently the way to run the automated tests is the same as for any Drupal module that does not have its own test setup already configured. In these cases, you always need a separate Drupal installation where you pull in the module you want to test.

In a future version we should add a development setup with docker-compose or ddev, which should also include a Collabora container, and then we can add a nice instruction in the README. For now I don't think it is worthwhile.

[donquixote]

@hfiguiere From our side this PR is good to go.
Let me know if you have any further questions.

[hfiguiere]

Let's merge this.

[donquixote]

Thanks @hfiguiere !
Looks like a rebase + fast-forward merge.
Personally I like to see the merge commits, but it is up to you.

[hfiguiere]

merge commits are terrible. A linear history is much more manageable.

Usually I rework commit to minimise the number of them.

[donquixote]

(off-topic comment I do im my free time)

merge commits are terrible. A linear history is much more manageable.

The "terrible" merge commits are usually those that you might find inside a feature branch as a result of git pull, often obfuscating changes or bad conflict resolution. Or you get distracting merges between master, dev, staging or whichever other branch, resulting from unnecessary workflow conventions.

The "merge feature branch x into main" is usually fine, if the feature branch was sufficiently rebased before. The merge commit then tells a story that is meaningful.

If you like something pure you could look for "semi-linear history", where you get something like a segmented snake. But I found it is generally good enough if a feature branch has no conflict when it is merged, and the feature branch itself is linear (this is also what we do on other projects).