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

Apply ldapUserFilter on members of group #8221

Merged

Conversation

Cybso
Copy link

@Cybso Cybso commented Feb 7, 2018

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

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
Copy link

codecov bot commented Feb 7, 2018

Codecov Report

Merging #8221 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@             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
Impacted Files Coverage Δ Complexity Δ
apps/user_ldap/lib/Access.php 35.76% <50%> (-0.01%) 322 <0> (+1)
apps/files_trashbin/lib/Trashbin.php 72.7% <0%> (+0.24%) 136% <0%> (ø) ⬇️
apps/files_external/lib/Command/ListCommand.php 40.66% <0%> (+2.04%) 39% <0%> (ø) ⬇️

if ($isUser) {
$ldapName = $this->readAttribute($fdn, $nameAttribute, $this->connection->ldapUserFilter);
} else {
$ldapName = $this->readAttribute($fdn, $nameAttribute);
Copy link
Member

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

$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);
Copy link
Member

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.

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 6, 2018
Signed-off-by: Roland Tapken <roland@bitarbeiter.net>
@Cybso
Copy link
Author

Cybso commented Mar 7, 2018

@blizzz I've applied the requested changes, please review again

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 8, 2018
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@MorrisJobke
Copy link
Member

CI failure is unrelated.

@MorrisJobke MorrisJobke merged commit cccf6f4 into nextcloud:master Mar 8, 2018
@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Mar 8, 2018
@zeridon
Copy link

zeridon commented Jun 12, 2018

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:

OC\User\NoUserException: REDACTED is not a valid user anymore
[internal function] OCA\User_LDAP\User_LDAP->getHome('REDACTED')
/var/www/html/apps/user_ldap/lib/User_Proxy.php - line 108: call_user_func_array(Array, Array)
/var/www/html/apps/user_ldap/lib/Proxy.php - line 150: OCA\User_LDAP\User_Proxy->callOnLastSeenOn('REDACTED', 'getHome', Array, false)
/var/www/html/apps/user_ldap/lib/User_Proxy.php - line 227: OCA\User_LDAP\Proxy->handleRequest('REDACTED', 'getHome', Array)
/var/www/html/lib/private/User/User.php - line 282: OCA\User_LDAP\User_Proxy->getHome('REDACTED')
/var/www/html/settings/Controller/UsersController.php - line 261: OC\User\User->getHome()
/var/www/html/settings/Controller/UsersController.php - line 322: OC\Settings\Controller\UsersController->formatUserForIndex(Object(OC\User\User))
[internal function] OC\Settings\Controller\UsersController->index(0, 50, 'Law', '', '')
/var/www/html/lib/private/AppFramework/Http/Dispatcher.php - line 161: call_user_func_array(Array, Array)
/var/www/html/lib/private/AppFramework/Http/Dispatcher.php - line 91: OC\AppFramework\Http\Dispatcher->executeController(Object(OC\Settings\Controller\UsersController), 'index')
/var/www/html/lib/private/AppFramework/App.php - line 115: OC\AppFramework\Http\Dispatcher->dispatch(Object(OC\Settings\Controller\UsersController), 'index')
/var/www/html/lib/private/AppFramework/Routing/RouteActionHandler.php - line 47: OC\AppFramework\App main('OC\\Settings\\Con...', 'index', Object(OC\AppFramework\DependencyInjection\DIContainer), Array)
[internal function] OC\AppFramework\Routing\RouteActionHandler->__invoke(Array)
/var/www/html/lib/private/Route/Router.php - line 297: call_user_func(Object(OC\AppFramework\Routing\RouteActionHandler), Array)
/var/www/html/lib/base.php - line 999: OC\Route\Router->match('/settings/users...')
/var/www/html/index.php - line 37: OC handleRequest()
{main}

@blizzz
Copy link
Member

blizzz commented Jun 12, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants