Make advanced permissions to inherit per user/group

[YoitoFes]

I think that, in following case the member of manager and test should have write permission to foo/bar/.

Folder path	Advanced permissions
foo/	Allow all for manager group
foo/bar/	Deny write for test group

But writing to foo/bar/ by the member is denied, because currently advanced permissions is calculated by the following steps:

1. Merge all user/group's permissions together per depth of path with the rules that "allow" overwrites "deny".
2. Merge permissions of all depth of path together with the rules that child's permissions overwrite parent folder's one.

Fix it by calculating permissions by following steps:

1. Merge permissions of all depth of path together per user/group with the rules that child's permissions overwrite parent folder's one.
2. Merge all user/group's permissions together with the rules that "allow" overwrites "deny"

Close #1212

[thorsten-schwartz]

@YoitoFes Hi, seems to get not enough testing support. But we are still on the right track to hit the goal within one year after launching the original thread. Anyway, thanks for helping - you do all your best - free of charge. Thanks

[fschrempf]

I can't provide a detailed review for this, but overall the approach looks good to me and I definitely support the intention of this change. This would probably also have a positive impact on the problems described in #655 and #598.

[CarlSchwan]

This might end up to be a problematic change. Groupfolders are often used is a way where all users have all access to the top folder and some subfolders are restricted for some groups.

With this change, the restricted folders would not be restricted anymore. This would be a big behavior change.

[fschrempf]

Groupfolders are often used is a way where all users have all access to the top folder and some subfolders are restricted for some groups.

Yes and that's exactly the use case for which this PR would be a great improvement.

With this change, the restricted folders would not be restricted anymore. This would be a big behavior change.

Yes, the introduced behavior is more permissive than the current one. So there could be setups where this will lead to unwanted access for some users. We would need to be very clear about this breaking change if we release it.

Still, I think it is worth the effort as the current behavior is just not intuitive and has caused a lot of trouble in the past (see linked issues). Without this change, one needs to add rules for the "manager" group in each sub directory to explicitly grant them the permissions that in fact should be inherited from the parent. Applying this scheme on a directory tree with several (nested) sub directories and multiple "manager" and "user" groups leads to a ton of unneeded and hard-to-maintain ACL rules being applied. This in turn causes lots of confusion for anyone who maintains or needs to change ACL rules and increases the probability of errors in the ACL rules.

On the other hand, with this applied we would have an easy and coherent behavior. I would even treat this as a bugfix for the currently "broken" ACL rule handling.

[fschrempf]

Here is a extended view of the example from above to visualize the change in the most easy setup possible:

User	Groups
userA	manager
userB	test
userC	manager, test

Current behavior

Folder path	Advanced permissions	Resulting Permissions for userA	Resulting Permissions for userB	Resulting Permissions for userC
foo/	Allow all for manager group	all	inherited from default permissions	all
foo/bar/	Deny write for test group	all	-write	-write

New behavior

Folder path	Advanced permissions	Resulting Permissions for userA	Resulting Permissions for userB	Resulting Permissions for userC
foo/	Allow all for manager group	all	inherited from default permissions	all
foo/bar/	Deny write for test group	all	-write	all

[thorsten-schwartz]

@fschrempf @CarlSchwan

Thanks for taking this issue. The basic topic is open for almost one year.
If you need further info don't hesitate asking me.

Greets Thorsten

[icewind1991]

The problem I see here is that you can't deny access to a folder from a subset of the user that has allow permissions on the parent

User	Groups
userA	manager
userB	manager

Current

Folder path	Advanced permissions	Resulting Permissions for userA	Resulting Permissions for userB
foo/	Allow all for manager group	all	all
foo/bar/	Deny write for userB	all	-write

New

Folder path	Advanced permissions	Resulting Permissions for userA	Resulting Permissions for userB
foo/	Allow all for manager group	all	all
foo/bar/	Deny write for userB	all	all

[fschrempf]

The problem I see here is that you can't deny access to a folder from a subset of the user that has allow permissions on the parent

That's indeed a problem. Maybe we can keep the behavior for ACL rules with a single user as subject and only adjust the behavior for group rules? Meaning user rules would always take precedence over group rules, regardless of inheritance as they are more specific.

[icewind1991]

That only partly resolves the issue. It all comes down to the intent of the admin when setting "conflicting" permissions with overlapping groups. In the original example, the admin could have intended to deny UserC from writing just as well as it could have been an accident.

I think "sub folder always overwrites parent permissions" is the best option as it still allows admins create both situations (in the original example, create a +write rule for manager for foo/bar), while the proposed change only allows for one of the options (the one with more permissions).

[thorsten-schwartz]

Hi developers,

are there any further actions in this topic? As I read in this thread you are discussing about several ideas to get rid of this problem. Please take a look on my basic problem once again whether your suggestions would solve my problem.
As I wrote almost a year ago (one user is member of several groups which one is denied and one is allowed to the same folder).

I'm looking forward for your feedback - thanks!
Thorsten

[rubentolosa]

I'm in Nextcloud 26 and Group folders 14.0.3 and I think I'm facing the same bug.

If I give advanced permissions (write) in a folder to a user that already is in a group without write permissions, he can write to that folder and "see" that has permissions inherited on subfolders. But it is not true, subfolders are not writtable for him.

[vbier]

I think "sub folder always overwrites parent permissions" is the best option as it still allows admins create both situations (in the original example, create a +write rule for manager for foo/bar), while the proposed change only allows for one of the options (the one with more permissions).

The question is do you want to have a solution that can handle 100% of the cases, but in reality is unusable? As soon as you have a tree stucture that is a little bit more complex, you will have to add +write rules to hundreds of folders. I would even go so far to say, that this makes permission inheritance abundant. As you do not know which groups might overlap (and that can change any time by someone adding new users to a group), you will then have to effectively add a complete set of permissions for every group to every folder.

We planned to use nextcloud to host our 850000 files in 260000 folders, with around 300 groups. We have 4600 permission assignments if the proposed change would be implemented. If it stays as it is now, I would have to assign the permissions for every folder and group, which would mean 78 million permission definitions. Aside from the fact that this will probably affect performance, this would be completely unmanageable for admins.

If it would be changed, it would be easy to use and understand. The fact that there are bug reports again and again indicates that the current imlementation seems to be unexpected to a lot of users. It sure does not seem logical to me that a user that is added to another group has less permissions than before.

Another problem with the current implementation is that it is not visible in the web interface. This shows the inherited permissions for the group right next to the permissions defined on the file/folder itself. There is no indication from which level the permissions are inherited. So in case I have definitions for two groups on two different parent folders, I have no way to see the effective permissions the user has on a file if I am an admin (it makes no visual difference in the UI if I allow to group a on level 3 and revoke for groub b on level 4 or vice versa). But the behaviour is different, which I personally find very dangerous.

[loulous24]

I was completely new to the system of inheritance of ACLs and had some trouble with. And I think that there are some bugs with the current system (I will try to make issues report) and that the UX should be clearer (because it is hard to see where the permission come from).

After using it for a time and having different use cases, I strongly agree with the position of icewind. The permission system should stay "sub folder always overwrites parent permissions".

An idea for improvements is perhaps to be able to set a permission for a folder for every user to break the inheritance rules from the parent and set a new set of rule for the folder and the subfolders (that can be overwritten with rules per group/per user).

And if this PR is accepted, please put a switch to disable the new version and keep the current mode for the inheriance of the permissions rights in order to avoid to break the current system