[FIX] dms: fix access rules for better security

[kobros-tech]

based on migration to 18.0 PR #385 and focusing on updating security groups and rules for better functionality and privacy.

The value of this commit is to improve vunrable security layers of dms module for all groups:

• managers can access and edit files/dirs/groups even locked files they can edit and delete even hidden Storage they can access.
• dms users can access files/dirs NOT Storage, they can lock files on each other, only locker user can edit/delete except for managers.
• internal users are not dms users by default, but we can grant any internal user this group, they can not access any file/dir/storage, but only if we grant any user to be a follower of a specific file then granted user can access that file and its dir and still see ONLY the specific file within the dir.
• portal users are following a similar rule for base internal users, so our partners/customers can Only access files and dir that they are granted to follow and can never access any other files.
• public users are removed from all groups and rules for files/dirs/storage
• We returned the share button that allows anybody to access the files/dirs with a pass token, dms users and portal users are able to get and share that token. portal users can share their own granted files they access only.

There is now rules for tags/categories for internal users based on being followers to specific files, and also they can access files and their dirs via potal page.

We did not review the ability to upload a file in sale and delete it there whether it gets locked by some other user yet. I hope to find people come here and cooperate for their sake and all community sake and for the public good.

[kobros-tech]

@pedrobaeza @victoralmau @etobella

as long as it it a PR for a fix I assume, what should be the behaviour in portal? Should every portal user see documents related to other portal users? if not then how can this be done in version 17.0?

I may be not aware and hardly could be correct so some values could come by discussion.

[victoralmau]

First of all, thank you for trying to improve this module.

Although I understand that these changes were already made previously in the other PR, it would have been more appropriate to discuss this in an issue to avoid making changes and spending effort on something that may not have the right approach.

First of all and without trying to be rude, it is important to know in detail the functioning of the “permissions” in dms, the flexibility they have and the reason why they are like this, do you know it?

I try to explain a little of how they work (up to v17) and some important clarifications.

• The rules (ir.rule) of this module are totally exceptional and intentional, there is no module in odoo core (that I know of) that uses something similar.
• The way dms works in Odoo Enterprise has a different approach, it assigns groups (res.groups) to directories and thus “models” permissions.
• The dms User and Manager specific permissions allow to manage some specific things (access groups, categories, tags, etc).
• Only users with the DMS: User group will be able to see the Documents menu.
• The most important thing in dms is “Access groups”, a directory can have as many access groups as you want.
• Access groups allow you to organize users as you need: parent groups, explicit users and/or linked groups (e.g. Sales: Administrator). The users it contains will be displayed in the “Users” tab, and allow that if someone is added/removed from a group (Sales: Administrator for example) he/she will already belong to the corresponding access group.
• In each access group you can define the permissions you have (if none are specified, they will be read permissions).
• The permissions of a directory are “cumulative”, i.e. if the “Inherit Groups” checkbox is checked, the permissions of a subfolder will be the access groups of that subdirectory + all the access groups of the parent directory.
• The permissions of a file are those of a directory; that is, if someone can access a directory, they will be able to access the files it contains. If someone can “delete” a directory, they will be able to delete the files it contains.
• In the portal (/my) all directories and files to which the user (even if he/she is a portal user) has access will be displayed.
• Using the share option to use an access_token link to a file will allow anyone to view that file.
• Sharing a directory is more risky, because anyone with that link will be able to see the directory and everything in it.
• Even if internal users do not have the Documents: User permission and cannot see the “Documents” menu, they will be able to see the directories and files they have access to (for example through dms_field, a specific way to view dms files linked to a record).
• Categories and tags do not have access rules because they are non-sensitive data (rhese data are not currently displayed on the portal, although they could be displayed).
• A storage of type Attachment has a totally different access operation, i.e. it does NOT use access groups. Directories are linked to a record (partner, invoice, etc), and whoever has access to the linked record will have access to that directory.
• Other modules (dms_user_role) add more options to the access groups.

The approach you propose (with followers in files) is not flexible or manageable, with an example of 10 files and 5 directories, it would be necessary to manage everything adding/deleting followers in all the files, besides that it would not give the flexibility to indicate the permissions (read/create/write/unlink), because the followers do not allow that.

[kobros-tech]

Description video of the new feature:

https://youtu.be/5z7yPdCnZCo?si=RA_jZ4V5efV-_YQf

[kobros-tech]

The cause of the new features can be shown in this example:

`

file = self.env['dms.file'] file.search_count([]) 36 domain = [('permission_read', '=', self.env.user.id)] file.search_count(domain) 36 domain = [('permission_read', '=', 123456)] file.search_count(domain) 36 domain = [('permission_write', '=', 'foo bar baz')] file.search_count(domain) 36 portal_user = self.env.ref('base.demo_user0') portal_user.name 'Joel Willis' domain = [('permission_unlink', '=', portal_user.id)] file.search_count(domain) 36 domain = [('permission_unlink', '=', 'Can any value give permission to do anything?')] file.search_count(domain) 36 domain = [('permission_write', '=', 'Can any value give permission to do anything?')] file.search_count(domain) 36 domain = [('permission_create', '=', 'Can any value give permission to do anything?')] file.search_count(domain) 36 domain = [('permission_create', '=', False)] file.search_count(domain) 0 domain = [('permission_create', '=', True)] file.search_count(domain) 36 domain = [('permission_unlink', '=', self.env.user.id)] file.search_count(domain) 36 domain = [('permission_create', '=', self.env.user.id)] file.search_count(domain) 36

`

[kobros-tech]

[victoralmau]

Reviewing the query examples, I don't see any problem, that is, it doesn't matter which domain you apply, the rule is always applied first with the domain that corresponds to your user, for that reason 36 files are always returned. The only exception is when you define False in the value, and it is intentional, due to https://github.com/OCA/dms/blob/da92eb72aca776f28ea1bb03c82cad8db66556bb/dms/models/dms_security_mixin.py#L220

[kobros-tech]

First of all, thank you for trying to improve this module.

Although I understand that these changes were already made previously in the other PR, it would have been more appropriate to discuss this in an issue to avoid making changes and spending effort on something that may not have the right approach.

First of all and without trying to be rude, it is important to know in detail the functioning of the “permissions” in dms, the flexibility they have and the reason why they are like this, do you know it?

I try to explain a little of how they work (up to v17) and some important clarifications.

* The rules (`ir.rule`) of this module are totally exceptional and intentional, there is no module in odoo core (that I know of) that uses something similar.

* The way dms works in Odoo Enterprise has a different approach, it assigns groups (`res.groups`) to directories and thus “models” permissions.

* The dms User and Manager specific permissions allow to manage some specific things (access groups, categories, tags, etc).

* Only users with the DMS: User group will be able to see the Documents menu.

* The most important thing in dms is “Access groups”, a directory can have as many access groups as you want.

* Access groups allow you to organize users as you need: parent groups, explicit users and/or linked groups (e.g. Sales: Administrator). The users it contains will be displayed in the “Users” tab, and allow that if someone is added/removed from a group (Sales: Administrator for example) he/she will already belong to the corresponding access group.

* In each access group you can define the permissions you have (if none are specified, they will be read permissions).

* The permissions of a directory are “cumulative”, i.e. if the “Inherit Groups” checkbox is checked, the permissions of a subfolder will be the access groups of that subdirectory + all the access groups of the parent directory.

* The permissions of a file are those of a directory; that is, if someone can access a directory, they will be able to access the files it contains. If someone can “delete” a directory, they will be able to delete the files it contains.

* In the portal (/my) all directories and files to which the user (even if he/she is a portal user) has access will be displayed.

* Using the share option to use an access_token link to a file will allow anyone to view that file.

* Sharing a directory is more risky, because anyone with that link will be able to see the directory and everything in it.

* Even if internal users do not have the Documents: User permission and cannot see the “Documents” menu, they will be able to see the directories and files they have access to (for example through `dms_field`, a specific way to view dms files linked to a record).

* Categories and tags do not have access rules because they are non-sensitive data (rhese data are not currently displayed on the portal, although they could be displayed).

* A storage of type Attachment has a totally different access operation, i.e. it does NOT use access groups. Directories are linked to a record (partner, invoice, etc), and whoever has access to the linked record will have access to that directory.

* Other modules (`dms_user_role`) add more options to the access groups.

The approach you propose (with followers in files) is not flexible or manageable, with an example of 10 files and 5 directories, it would be necessary to manage everything adding/deleting followers in all the files, besides that it would not give the flexibility to indicate the permissions (read/create/write/unlink), because the followers do not allow that.

all right lets work in parallel then we can meet at some point, on my side there is a connection module between dms and avatax exemption, and another between dms and sign_oca, so by applying them I will improve dms as the behaviour needs, and if there is a risk on security will appear.

I am looking forward for seeing the migration you will introduce to 18.0 of dms.

[victoralmau]

But then, these changes are not necessary, right? There is no security problem then, right?

So you want me to make the migration to v18 that you have already started? Remember that we commented not to add these changes in that PR, otherwise, you can continue it if you want.

[kobros-tech]

But then, these changes are not necessary, right? There is no security problem then, right?

So you want me to make the migration to v18 that you have already started? Remember that we commented not to add these changes in that PR, otherwise, you can continue it if you want.

yes, continue the way you like, I myself sometimes when I am not sure of one approach I make two and compare them in the end to finish with a single one.

If you start you will find a security error in portal based on the same security xml from 17.0, but from my migration the error will disappear so that I complained of comparing boolean value to integer of user id, but I encourage you to try it yoursef.

[github-actions[bot]]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.