ACL inheritance not working as documented if user is member of multiple groups and one of them is read disabled

[thorsten-schwartz]

Hi,
we have nextcloud 20.0.4 with group folders 8.2.0 running for the local fire department. There is a problem if a user belongs to multiple groups and one of this group is read disabled for a folder.

It seems, if a user is read denied from being a member of group via direct setting in ACL for one folder, the user is still disabled even though being a member of an another inherited allowed group - this is wrong!

Believing the manual unset rights are always inherited from the parent folder and also allow overrules deny. But this is not working in our case!

We did a tryout scenario with reduced users and groups - you will find attached all informations as screenshots.

1. create test users and groups (OK -> see screenshot below)
2. create groupfolder (OK -> see screenshot below)
3. create ACL with occ (OK -> see screenshot below)
4. check in ACL in UI as admin (OK -> see screenshot below)
5. login as test users and check visibility of folders and subfolders (ERROR -> see screenshot below)

Here you can see the ACL for the folders when logged in as admin - this is similar to the OCC settings:

Here you can see the UI when logged in as test_manager with the missing folder:

I hope to get feedback soon!

Greets Thorsten

[pierreozoux]

You have 2 ways to deal with acl, either the most restricting wins or the other way around.

Here it was decided that the most restricting wins (disable read override all). It is by design. It is not wrong per say.

Ill close as a wont fix.

But if you have a ux proposal to fix, we might reconsider.

[thorsten-schwartz]

Hi Pierre,
thanks for your feedback, but I still believe you are not right.
I made a more precise pdf showing the rights and the situation of inheritance.

Maybe you are also right and simply the documentation is false.
However, but please take a minute or two and double check.

Thorsten
20210315_NC_groupfolder_ACL_Fehler.pdf

[pierreozoux]

Ok, obviously you did put a lot more effort in this than I did :)

Ill reopen.

Then, if I understand well, there is discrepancy between the doc and the actual behavior, is it correct?

[thorsten-schwartz]

Pierre, you're right. There is a discrepancy between the documentation and the actual behaviour. The point is, if a user belongs to several groups and for one of these groups the access is read disabled by inheritance the user wont get access even though he/she is member of an allowed group also!

So there is a mistake in even in inheritance or in priority allow over deny.
Hopefully someone will confirm and better fix this according the documentation.

Thorsten

[thorsten-schwartz]

Nobody is taking this topic !?!
No fix and no documentation update - sad, sorry?
Meanwhile we are on NC22.0.0 with group folders 9.0.2

Keep in mind, we are non profit fire department - so getting no feedback is poor.

Greets Thorsten

[putt1ck]

Is it possible to reopen the discussion about how ACLs should work? For sure when rules are ordered, a deny rule means users in that group should not get access but an allow rule that happens before means they do i.e. in firewalls the standard behaviour is "deny all"; it's the unwritten last rule in the rules list, which is never reached if an allow rule is matched first.

Or to look at it another way, the idea that a person cannot be in "all staff" and "managers" breaks business logic.

[thorsten-schwartz]

Hi Chris,
of course we can reopen this topic. But to be honest, I don't think the developpers will care about - I never got any reply, except from Pierre - he decided to change the topic's headline...
Anyway, a user can be member of several groups with several ACL rights in the directory tree.
So if a user is disabled in one folder because of being member in one (subsidiary) group, but also beeing member of another (superior) group by inheritage in this folder he/she stays unallowed. So, deny rule wins - in discrepancy to the manual. This is in my opionion the wrong point of view. If someone is member of several groups the allow permission needs to be the winner.

I am totally disappointed about the developpers not responding, keep in mind we are a non profit fire department.
Changing the headline won't help for sure.

Greets Thorsten

[fschrempf]

@thorsten-schwartz

@pierreozoux changed the headline, because he tried to help with triaging and updating the issues for this app, which is a valuable job. Of course this doesn't mean that problems get solved, but it is a very basic preparation before any actual work can happen. Pierre stepped up as a volunteer to do this so you should appreciate his work. I haven't done any NC development myself so far, but I have tried to help here and there, as Pierre did.

On the other hand I can understand your frustration that there is no progress. This is a general problem with understaffed open source projects, in particular with NC and the groupfolders app. Please also see #1215 for a general discussion about this topic. I'm also maintaining a NC instance for a non-profit organization and I know the pain, but I'm afraid I don't have any satisfying solution for now.

About this issue: If there is a discrepancy between documentation and the actual behavior, we could adjust the docs accordingly. If the behavior needs to be changed than this will be a big task and it shouldn't be done on a small scale. Rather someone needs to go over the current state of the ACL features, check all the known issues and decide how to move on. My feeling is that there is a lot of rework and simplification needed to make it user friendly, maintainable and stable.

[putt1ck]

I agree some more work could be done to improve ACL handling/UI (although that's generally true for all software ever, ignoring Apple's claims for their latest release of X), but right now the business logic is broken if people cannot be in overlapping groups - and if that's how it's going to stay we need to know so we can consider options.

[thorsten-schwartz]

Hi, you are totally right the ACL should be reviewed.
But anyway, my problem would be solved if the behaviour was like in the description mentioned: Access should win against deny - even within heritage.
I think, I explained the situation in a very clear and structured way at the beginning of this thread.
Developers, pls. feel free to improve your code.
Thanks in advance.

[YoitoFes]

Following modification worked as you intended.

But I'm not sure that the issue should be fixed, since the statement Any permission **not explicitly set** will inherit the permissions from the parent folder in the document is unclear for me.

If not explicitly set means that no user/group set the permission, current implementation seems to be correct.
Or, if not explicitly set means that individual user/group does not set the permissions themselves,
the issue should be fixed.

diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php
index f40fe48..7483251 100644
--- a/lib/ACL/ACLManager.php
+++ b/lib/ACL/ACLManager.php
@@ -122,10 +122,18 @@ class ACLManager {
 		$path = ltrim($path, '/');
 		$rules = $this->getRules($this->getParents($path));
 
-		return array_reduce($rules, function (int $permissions, array $rules) {
-			$mergedRule = Rule::mergeRules($rules);
-			return $mergedRule->applyPermissions($permissions);
-		}, Constants::PERMISSION_ALL);
+		$inheritedPermissionsByMapping = [];
+		array_walk_recursive($rules, function($rule) use(&$inheritedPermissionsByMapping) {
+			$mappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId();
+			if (!isset($inheritedPermissionsByMapping[$mappingKey])) {
+				$inheritedPermissionsByMapping[$mappingKey] = Constants::PERMISSION_ALL;
+			}
+			$inheritedPermissionsByMapping[$mappingKey] = $rule->applyPermissions($inheritedPermissionsByMapping[$mappingKey]);
+		});
+		if (empty($inheritedPermissionsByMapping)) {
+			return Constants::PERMISSION_ALL;
+		}
+
+		return array_reduce($inheritedPermissionsByMapping, function (int $mergedParmission, int $permissions) {
+			return $mergedParmission | $permissions;
+		}, 0);
 	}
 
 	/**
@@ -138,15 +146,22 @@ class ACLManager {
 		$path = ltrim($path, '/');
 		$rules = $this->ruleManager->getRulesForPrefix($this->user, $this->getRootStorageId(), $path);
 
-		return array_reduce($rules, function (int $permissions, array $rules) {
-			$mergedRule = Rule::mergeRules($rules);
-
-			$invertedMask = ~$mergedRule->getMask();
+		$inheritedPermissionsByMapping = [];
+		array_walk_recursive($rules, function($rule) use(&$inheritedPermissionsByMapping) {
+			$mappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId();
+			if (!isset($inheritedPermissionsByMapping[$mappingKey])) {
+				$inheritedPermissionsByMapping[$mappingKey] = Constants::PERMISSION_ALL;
+			}
+			$invertedMask = ~$rule->getMask();
 			// create a bitmask that has all inherit and allow bits set to 1 and all deny bits to 0
-			$denyMask = $invertedMask | $mergedRule->getPermissions();
+			$denyMask = $invertedMask | $rule->getPermissions();
 
 			// since we only care about the lower permissions, we ignore the allow values
-			return $permissions & $denyMask;
-		}, Constants::PERMISSION_ALL);
+			$inheritedPermissionsByMapping[$mappingKey] = $inheritedPermissionsByMapping[$mappingKey] & $denyMask;
+		});
+		if (empty($inheritedPermissionsByMapping)) {
+			return Constants::PERMISSION_ALL;
+		}
+
+		return array_reduce($inheritedPermissionsByMapping, function (int $mergedParmission, int $permissions) {
+			return $mergedParmission | $permissions;
+		}, 0);
 	}
 }

Edit (9/2):

My modification did not care the condition $inheritedPermissionsByMapping is empty. Fix it.

[thorsten-schwartz]

@YoitoFes
I really appreciate your coding and ideas. Thanks for taking this topic!

In my opinion the meaning ...
not explicitly set means that individual user/group does not set the permissions themselves ...
will be the most fitting interpretation in this context.

First of all the heritage of the whole folder/file-tree should be considered anyway and finally the individual ACL - if set or not.
And basically allow should win against deny rules.

Greets 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.

[provokateurin]

Duplicate of #598