Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LDAP inGroup() failing #24252

Closed
Hexasoft opened this issue Nov 20, 2020 · 6 comments · Fixed by #24402
Closed

LDAP inGroup() failing #24252

Hexasoft opened this issue Nov 20, 2020 · 6 comments · Fixed by #24402
Assignees
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug feature: ldap

Comments

@Hexasoft
Copy link

Hexasoft commented Nov 20, 2020

We can't move a directory into an other directory when the first one is a shared one, shared by group.

Steps to reproduce

  1. User A creates a directory DA, then share it to group G
  2. User B (which is in group G) sees DA. Trying to drag&drop DA into an other (regular) directory failed, and in logs we get "InvalidArgumentException: Invalid recipient"

Expected behaviour

Directory DA should be movable.
It is the case when directory is shared by user (not by group).
It is also the case when using local groups (not LDAP groups).

Actual behaviour

Directory can't be moved into an other directory.

Server configuration

Debian 10, Apache, MySQL (MariaDB), PHP7. All up-to-date installed from OS.

NextCloud 20.0.1, updated with OCC from 18.x.

LDAP configuration

LDAP config
+-------------------------------+-----------------------------------+
| Configuration                 | s01                               |
+-------------------------------+-----------------------------------+
| hasMemberOfFilterSupport      | 0                                 |
| homeFolderNamingRule          | attr:uid                          |
| lastJpegPhotoLookup           | 0                                 |
| ldapAgentName                 | cn=l***   |
| ldapAgentPassword             | ***                               |
| ldapAttributesForGroupSearch  |                                   |
| ldapAttributesForUserSearch   |                                   |
| ldapBackupHost                | ***.***.***.***                   |
| ldapBackupPort                | 389                               |
| ldapBase                      | dc=***            |
| ldapBaseGroups                | ou=***  |
| ldapBaseUsers                 | ou=*** |
| ldapCacheTTL                  | 600                               |
| ldapConfigurationActive       | 1                                 |
| ldapDefaultPPolicyDN          |                                   |
| ldapDynamicGroupMemberURL     |                                   |
| ldapEmailAttribute            | mail                              |
| ldapExperiencedAdmin          | 0                                 |
| ldapExpertUUIDGroupAttr       |                                   |
| ldapExpertUUIDUserAttr        |                                   |
| ldapExpertUsernameAttr        | uid                               |
| ldapExtStorageHomeAttribute   |                                   |
| ldapGidNumber                 | gidNumber                         |
| ldapGroupDisplayName          | cn                                |
| ldapGroupFilter               | (objectclass=*)                   |
| ldapGroupFilterGroups         |                                   |
| ldapGroupFilterMode           | 1                                 |
| ldapGroupFilterObjectclass    |                                   |
| ldapGroupMemberAssocAttr      | memberUid                         |
| ldapHost                      | ***                   |
| ldapIgnoreNamingRules         |                                   |
| ldapLoginFilter               | uid=%uid                          |
| ldapLoginFilterAttributes     |                                   |
| ldapLoginFilterEmail          | 0                                 |
| ldapLoginFilterMode           | 1                                 |
| ldapLoginFilterUsername       | 1                                 |
| ldapMatchingRuleInChainState  | unknown                           |
| ldapNestedGroups              | 0                                 |
| ldapOverrideMainServer        |                                   |
| ldapPagingSize                | 500                               |
| ldapPort                      | 389                               |
| ldapQuotaAttribute            |                                   |
| ldapQuotaDefault              |                                   |
| ldapTLS                       | 1                                 |
| ldapUserAvatarRule            | default                           |
| ldapUserDisplayName           | displayname                       |
| ldapUserDisplayName2          |                                   |
| ldapUserFilter                | (objectclass=*)                   |
| ldapUserFilterGroups          |                                   |
| ldapUserFilterMode            | 1                                 |
| ldapUserFilterObjectclass     |                                   |
| ldapUuidGroupAttribute        | auto                              |
| ldapUuidUserAttribute         | auto                              |
| turnOffCertCheck              | 1                                 |
| turnOnPasswordChange          | 0                                 |
| useMemberOfToDetectMembership | 1                                 |
+-------------------------------+-----------------------------------+

Debug / workaround

I tracked down the error "InvalidArgumentException: Invalid recipient" and found that the method inGroup() in file apps/user_ldap/lib/Group_LDAP.php replies FALSE when I try to move directory DA into a local directory. I am in group G, and DA is shared by its owner to LDAP group G (note: all users come from LDAP).

After added some debug, I found that $members table holds the LDAP entries for the tested group (here group G).
The inGroup() method uses in_array($userDN, $members) to check if user is in the group.
But : this table is not a flat table. It is an array with one line per member, each line is an array of property => value with properties stuff like "mail", "dn", "displayName"... and value an array containing the value.
So the actual DN for a given user is stored in $members[index]['dn'][0].

I replaced the in_array with a loop: foreach($members as $m) { if (in_array($userDN, $m['dn']) { $isInGroup=TRUE; beak; } } and the problem is solved (after cleaning caches, and I made this change at the two places were in_array is used).

Is it a problem with our LDAP configuration or a bug in the inGroup()? Regards.

@Hexasoft Hexasoft added 0. Needs triage Pending check for reproducibility or if it fits our roadmap bug labels Nov 20, 2020
@steffen-moser
Copy link

This sounds very interesting. Maybe there is a link to the problem which I commented here: #22775. The error I see is triggered by "acceptShare()" in "/lib/private/Share20/DefaultShareProvider.php" after calling "inGroup()". An LDAP user which should be in the group is not in the group. We see this in 19.0.4 and 19.0.5, though. We have not seen this in 18.x.

@blizzz
Copy link
Member

blizzz commented Nov 23, 2020

This sounds very interesting. Maybe there is a link to the problem which I commented here: #22775. The error I see is triggered by "acceptShare()" in "/lib/private/Share20/DefaultShareProvider.php" after calling "inGroup()". An LDAP user which should be in the group is not in the group. We see this in 19.0.4 and 19.0.5, though. We have not seen this in 18.x.

It's not related On a second look it might, provided that the backtrace there is not from the background job run, but originating from a user action.

@blizzz blizzz self-assigned this Nov 23, 2020
@blizzz
Copy link
Member

blizzz commented Nov 23, 2020

@Hexasoft thank you for the report and your initial analysis! I'll try to get to this issue this week.

@steffen-moser
Copy link

This sounds very interesting. Maybe there is a link to the problem which I commented here: #22775. The error I see is triggered by "acceptShare()" in "/lib/private/Share20/DefaultShareProvider.php" after calling "inGroup()". An LDAP user which should be in the group is not in the group. We see this in 19.0.4 and 19.0.5, though. We have not seen this in 18.x.

It's not related On a second look it might, provided that the backtrace there is not from the background job run, but originating from a user action.

In the meantime, we checked it exhaustively: It is definitely related! After applying the patch proposed by Hexasoft, we could solve the problems I mentioned in my comment in #22775. The problem appeared after we had upgraded from 18.0.x to 19.0.4 and it is still existent in 19.0.5. It is triggered when an LDAP group, which a Nextcloud folder is shared with, gets a new member. In this case, the acceptShare() method asks inGroup() whether the user is part of the group.

The code, Hexasoft found faulty could only return FALSE due to the array format. It was completely irrelevant whether the user was actually a member of a group or not.

My backtrace stems from the background job but it is not complete. Maybe this was confusing.

Interestingly, the error stops the whole background job of syncing LDAP groups with Nextcloud groups. This made debugging this a bit tricky. We looked and looked at whether one of our users was affected. The reason is: The error is triggered by the group member join which happens to the oldest LDAP group because the algorithm gets the LDAP groups (at least from OpenLDAP) in the order of their creation. After temporarily removing the user from the group, the second-oldest group which got a new member triggered the error.

The question is: Should I open a new issue for this problem and propose that Hexasoft's patch fixes it?

@Hexasoft
Copy link
Author

Good news if it can solve your problem too!
Note: my patch is not intended to be use as-is (or to be used in prod env). It was a quick-and-dirty change to validate my hypothesis.

@blizzz
Copy link
Member

blizzz commented Nov 26, 2020

The question is: Should I open a new issue for this problem and propose that Hexasoft's patch fixes it?

nope, one ticket is sufficient :)

I opened a pull request with a fix at #24402

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug feature: ldap
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants