-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Apply ldapUserFilter on members of group #8221
Apply ldapUserFilter on members of group #8221
Conversation
Refers to issue nextcloud#8220 user_ldap configured with custom filters for active directory access (group-member-association is "member"). Then it can happen that the members of a group contain members that don't belong to the users available in Nextcloud (the most trivial reason is that the user filter contains "(!(UserAccountControl:1.2.840.113556.1.4.803:=2))" to exclude disabled users from being imported). This can be fixed by applying the ldapUserFilter when resolving the UID for a DN fetched from the group's member list. Signed-off-by: Roland Tapken <roland@bitarbeiter.net>
Codecov Report
@@ Coverage Diff @@
## master #8221 +/- ##
============================================
+ Coverage 51.73% 51.74% +<.01%
- Complexity 25366 25367 +1
============================================
Files 1599 1599
Lines 95064 95072 +8
Branches 1376 1376
============================================
+ Hits 49185 49192 +7
- Misses 45879 45880 +1
|
apps/user_ldap/lib/Access.php
Outdated
if ($isUser) { | ||
$ldapName = $this->readAttribute($fdn, $nameAttribute, $this->connection->ldapUserFilter); | ||
} else { | ||
$ldapName = $this->readAttribute($fdn, $nameAttribute); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for consistency, applying group filter would also make sense in this case. A $filter var can be created in the beginning of the function, so an additional if won't be necessary
apps/user_ldap/lib/Access.php
Outdated
$ldapName = $this->readAttribute($fdn, $nameAttribute, $this->connection->ldapUserFilter); | ||
} else { | ||
$ldapName = $this->readAttribute($fdn, $nameAttribute); | ||
} | ||
if(!isset($ldapName[0]) && empty($ldapName[0])) { | ||
\OCP\Util::writeLog('user_ldap', 'No or empty name for '.$fdn.'.', \OCP\Util::INFO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message needs to be adjusted and take this case into account, too.
Signed-off-by: Roland Tapken <roland@bitarbeiter.net>
@blizzz I've applied the requested changes, please review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐘
CI failure is unrelated. |
Unfortunately this does not fix all possible cases. Currently (nc 13.0.2 with this patch) when trying to view the group that has a disabled user in it the spinner spins and spins and never finishes. The error reported in the logs is:
|
@zeridon right, this was not backported 13. Mostly because we tagged it as enhancement, in fact it fixes a bug. Thanks for pointing it out! Next window is 13.0.5. |
Refers to issue #8220
user_ldap configured with custom filters for active directory access
(group-member-association is "member"). Then it can happen that the
a group contain members that don't belong to the users
available in Nextcloud (the most trivial reason is that the user filter
contains "(!(UserAccountControl:1.2.840.113556.1.4.803:=2))" to exclude
disabled users from being imported).
This can be fixed by applying the ldapUserFilter when resolving the UID
for a DN fetched from the group's member list.
Signed-off-by: Roland Tapken roland@bitarbeiter.net