When checking memberof, apply the group filter after getting all groups #683

jvillafanez · 2021-09-22T10:51:35Z

For nested groups, this was a problem because the group filter could
remove the nested group, so we couldn't check the memberof attribute of
the nested group and we couldn't go upwards in the group tree.

GroupA contains GroupB, and GroupB contains User1. If the group filter
contains just GroupA (not GroupB), checking the "memberof" of User1
would return no group. This PR fixes this by returning GroupA, which is
part of the group filter (if the "nested groups" checkbox is active)

Related to https://github.com/owncloud/enterprise/issues/4754

Steps to reproduce

In the AD create GroupA; inside GroupA create GroupB; inside GroupB create some users
Ensure the AD has additional users and groups
Configure the user_ldap app to connect to the AD
1. User filter is something like (&(|(objectclass=user))(|(|(memberof:1.2.840.113556.1.4.1941:=CN=GroupA,CN=Users,DC=forest,DC=dungeon,DC=prv)(primaryGroupID=2048)))) Basically, only members of the GroupA will be allowed. Note the 1.2.840.113556.1.4.1941 for the recursive search in AD.
2. Login filter is the same, with the samaccountname as uid: (&(&(|(objectclass=user))(|(|(memberof:1.2.840.113556.1.4.1941:=CN=GroupA,CN=Users,DC=forest,DC=dungeon,DC=prv)(primaryGroupID=2048))))(samaccoutname=%uid))
3. Group filter just takes into account the GroupA: (&(|(objectclass=group))(|(|(cn=GroupA))))
4. Group-member association in the advanced tab must be "member (AD)"
5. Nested groups checkbox must be enabled
6. The rest of the option could be the default ones.
Go to the users page

Previous behavior

Users inside the GroupB don't appear as member of GroupA. The'll likely appear without any group membership if they don't belong to any other group.
Clicking in the GroupA (which should appear in the left sidebar) doesn't show any user

PR's behavior

Users inside GroupB appear ONLY as member of groupA. Note that GroupB isn't part of the "accepted" groups in the group filter, so this is ok.
Clicking inside the GroupA shows all the users that are inside that group and any other group that is inside GroupA, such as GroupB. The users are shown only as members of the GroupA, not GroupB.

jvillafanez · 2021-09-22T14:57:54Z

Tested with OC 10.6 and 10.8, works in both cases. Note that the patch can't be applied cleanly in any user_ldap's released version.

phil-davis · 2021-09-22T15:05:45Z

@jvillafanez PR #684 has been merged. You can rebase this PR.

ownclouders · 2021-09-23T09:48:09Z

💥 Acceptance tests pipeline webUIProvisioning-master-chrome-mysql8.0-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3486/56/1

mrow4a

ahh makes sense, probably was overlooked / not tested. will approve after integration test passing

ownclouders · 2021-09-24T07:10:23Z

💥 Acceptance tests pipeline webUIProvisioning-master-chrome-mysql8.0-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3489/56/1

ownclouders · 2021-09-27T17:30:04Z

💥 Acceptance tests pipeline webUIProvisioning-master-chrome-mysql8.0-php7.3 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3493/55/1

jvillafanez · 2021-09-28T08:26:40Z

@janackermann @JammingBen it seems we broke something with the group UI and it's breaking these tests. If it's intentional we need to adjust or drop the failing tests

master

10.8.0

AlexAndBear · 2021-09-28T08:32:01Z

@jvillafanez Is everything else working correctly?
At first, it was not intended that those groups won't be shown, but why the heck, we want to show x groups while creating a user, if they are not assignable?

So I think this is an advancement, IF everything else is working

phil-davis · 2021-09-28T09:07:19Z

Note: I am making a core PR to @skipOnOcV10.8.0 etc a lot of core test scenarios that had core master changes recently and are known to not pass any more against "latest" core 10.8.0. That will fix all failures that are in pipelines that run against core "latest". After removing this "noise", I will see what else is failing...

jvillafanez · 2021-09-28T09:20:02Z

At first, it was not intended that those groups won't be shown, but why the heck, we want to show x groups while creating a user, if they are not assignable?

You're right. Makes sense to me.
Let's wait for the changes in the tests of core.

ownclouders · 2021-09-29T09:58:26Z

💥 Acceptance tests pipeline webUIProvisioningS-master-chrome-mysql8.0-php7.3 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3497/69/1

phil-davis · 2021-09-29T11:12:09Z

Some core skips that I missed yesterday are coming in core PR owncloud/core#39305

phil-davis · 2021-09-29T15:00:15Z

PR #686 has been merged - CI is green now. This PR and any others should be rebased.

ownclouders · 2021-09-30T08:07:05Z

💥 Acceptance tests pipeline apiUserLDAP-latest-postgres9.4-php7.3 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3506/38/1

jvillafanez · 2021-10-04T07:28:44Z

Type error: Too few arguments to function LoggingContext::assertLogFileContainsAtLeastOneEntryMatchingTable(), 2 passed in /var/www/owncloud/testrunner/tests/acceptance/features/bootstrap/LoggingContext.php on line 214 and exactly 3 expected (Behat\Testwork\Call\Exception\FatalThrowableError)

@phil-davis this seems to be a problem in the core tests.

ownclouders · 2021-10-05T08:07:31Z

💥 Acceptance tests pipeline core-apiAll-21-20-master-mysql8.0-php7.3 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3513/90/1

AlexAndBear · 2021-10-12T09:09:49Z

This branch is out-of-date with the base branch
@jvillafanez can you take care ?

lib/Group_LDAP.php

For nested groups, this was a problem because the group filter could remove the nested group, so we couldn't check the memberof attribute of the nested group and we couldn't go upwards in the group tree. GroupA contains GroupB, and GroupB contains User1. If the group filter contains just GroupA (not GroupB), checking the "memberof" of User1 would return no group. This PR fixes this by returning GroupA, which is part of the group filter (if the "nested groups" checkbox is active)

sonarcloud · 2021-11-15T09:10:04Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

jnweiger · 2021-12-16T02:26:40Z

Confirmed fixed in 10.9.0 RC2

But it works in more cases than specifed above:

Also works, when checkbox [ ] nested groups is not checked.
Also works with algorithm memberOf, which is explained to not work with nested groups.

Note:
The ldap queries must be written as ...(memberof:1.2.840.113556.1.4.1941:=CN=GroupA,...
The syntax propsed by the query generator ...(memberof=CN=GroupA,... only finds direct members of GroupA

jvillafanez · 2021-12-16T08:22:57Z

To clarify a bit, this PR just fixes the linked issue, whose steps to reproduce are described at the beginning. It doesn't have anything to do with the group membership algorithm because they were introduced later.

There might be some interactions between the algorithm and the checkbox that we should investigate.

mmattel · 2021-12-16T12:29:54Z

@jvillafanez @jnweiger is the way how the query string is used docs relevant?
Imho it looks like, but I am not sure. This may be not relevant for this PR but in general.

jvillafanez · 2021-12-16T12:45:52Z

I don't think it's doc relevant by itself. I mean, it fixes an issue with recursive group membership in AD, but you should know about recursive group membership in the first place.

If we don't have docs about recursive group membership, we could decide whether we should document something or not, but I think it's out of the scope of this ticket. In addition, this is mainly for AD (I'm not sure how many providers support this feature, but I don't think is too common)

mmattel · 2021-12-16T16:25:39Z

we don't have docs about recursive group membership,

As far I know, we dont have that, therefore I think it is a good idea to have a docs ticket to keep track on.

jvillafanez added the 3 - To Review label Sep 22, 2021

jvillafanez self-assigned this Sep 22, 2021

jvillafanez force-pushed the fix_nested_group_with_filter branch from 3134455 to 4a225ef Compare September 22, 2021 15:45

mrow4a reviewed Sep 23, 2021

View reviewed changes

jvillafanez force-pushed the fix_nested_group_with_filter branch from d632f7e to 576b8f4 Compare September 30, 2021 07:47

jvillafanez force-pushed the fix_nested_group_with_filter branch from 576b8f4 to eeb16d1 Compare October 5, 2021 07:10

jvillafanez force-pushed the fix_nested_group_with_filter branch from eeb16d1 to b9025be Compare October 6, 2021 07:21

jvillafanez force-pushed the fix_nested_group_with_filter branch from b9025be to b70e224 Compare October 13, 2021 07:24

AlexAndBear requested changes Oct 19, 2021

View reviewed changes

lib/Group_LDAP.php Outdated Show resolved Hide resolved

AlexAndBear approved these changes Oct 20, 2021

View reviewed changes

jvillafanez force-pushed the fix_nested_group_with_filter branch from f389f71 to 7b29a9a Compare October 26, 2021 12:34

jvillafanez added 3 commits November 15, 2021 09:44

Do not filter while getting the members of the group

3bcfbd1

Adjust unit test

fd560ae

Don't reassign variable

184ca44

jvillafanez force-pushed the fix_nested_group_with_filter branch from 7b29a9a to 184ca44 Compare November 15, 2021 08:44

jvillafanez merged commit 2ccc261 into master Nov 16, 2021

delete-merged-branch bot deleted the fix_nested_group_with_filter branch November 16, 2021 08:44

This was referenced Nov 22, 2021

LDAP 0.16.0 #681

Closed

Testplan for user_ldap 0.16.0 #682

Closed

jnweiger mentioned this pull request Dec 16, 2021

nested groups work when checkbox nested groups is unchecked #699

Open

jvillafanez mentioned this pull request Nov 14, 2022

LDAP recursive group membership and AD owncloud/docs-server#735

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When checking memberof, apply the group filter after getting all groups #683

When checking memberof, apply the group filter after getting all groups #683

jvillafanez commented Sep 22, 2021

jvillafanez commented Sep 22, 2021

phil-davis commented Sep 22, 2021

ownclouders commented Sep 23, 2021

mrow4a left a comment

ownclouders commented Sep 24, 2021

ownclouders commented Sep 27, 2021

jvillafanez commented Sep 28, 2021

AlexAndBear commented Sep 28, 2021 •

edited

phil-davis commented Sep 28, 2021

jvillafanez commented Sep 28, 2021

ownclouders commented Sep 29, 2021

phil-davis commented Sep 29, 2021

phil-davis commented Sep 29, 2021

ownclouders commented Sep 30, 2021

jvillafanez commented Oct 4, 2021

ownclouders commented Oct 5, 2021

AlexAndBear commented Oct 12, 2021

sonarcloud bot commented Nov 15, 2021

jnweiger commented Dec 16, 2021

jvillafanez commented Dec 16, 2021 •

edited

mmattel commented Dec 16, 2021

jvillafanez commented Dec 16, 2021

mmattel commented Dec 16, 2021

When checking memberof, apply the group filter after getting all groups #683

When checking memberof, apply the group filter after getting all groups #683

Conversation

jvillafanez commented Sep 22, 2021

Steps to reproduce

Previous behavior

PR's behavior

jvillafanez commented Sep 22, 2021

phil-davis commented Sep 22, 2021

ownclouders commented Sep 23, 2021

mrow4a left a comment

Choose a reason for hiding this comment

ownclouders commented Sep 24, 2021

ownclouders commented Sep 27, 2021

jvillafanez commented Sep 28, 2021

AlexAndBear commented Sep 28, 2021 • edited

phil-davis commented Sep 28, 2021

jvillafanez commented Sep 28, 2021

ownclouders commented Sep 29, 2021

phil-davis commented Sep 29, 2021

phil-davis commented Sep 29, 2021

ownclouders commented Sep 30, 2021

jvillafanez commented Oct 4, 2021

ownclouders commented Oct 5, 2021

AlexAndBear commented Oct 12, 2021

sonarcloud bot commented Nov 15, 2021

jnweiger commented Dec 16, 2021

jvillafanez commented Dec 16, 2021 • edited

mmattel commented Dec 16, 2021

jvillafanez commented Dec 16, 2021

mmattel commented Dec 16, 2021

AlexAndBear commented Sep 28, 2021 •

edited

jvillafanez commented Dec 16, 2021 •

edited