Skip to content

Apply ldapUserFilter on members of group - #8221

Merged
MorrisJobke merged 2 commits into
nextcloud:masterfrom
Cybso:8220_applyLdapUserFilter_on_members
Mar 8, 2018
Merged

Apply ldapUserFilter on members of group#8221
MorrisJobke merged 2 commits into
nextcloud:masterfrom
Cybso:8220_applyLdapUserFilter_on_members

Conversation

@Cybso

@CybsoCybso commented Feb 7, 2018

Copy link
Copy Markdown

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

codecovBot commented Feb 7, 2018

Copy link
Copy Markdown

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 FilesCoverage ΔComplexity Δ
apps/user_ldap/lib/Access.php35.76% <50%> (-0.01%)322 <0> (+1)
apps/files_trashbin/lib/Trashbin.php72.7% <0%> (+0.24%)136% <0%> (ø)⬇️
apps/files_external/lib/Command/ListCommand.php40.66% <0%> (+2.04%)39% <0%> (ø)⬇️

Comment threadapps/user_ldap/lib/Access.php Outdated
if ($isUser) {
$ldapName = $this->readAttribute($fdn, $nameAttribute, $this->connection->ldapUserFilter);
} else {
$ldapName = $this->readAttribute($fdn, $nameAttribute);

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

Comment threadapps/user_ldap/lib/Access.php Outdated
$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
Copy Markdown
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.

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

Cybso commented Mar 7, 2018

Copy link
Copy Markdown
Author

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

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

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🐘

@MorrisJobke

Copy link
Copy Markdown
Member

CI failure is unrelated.

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

Copy link
Copy Markdown

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
Copy Markdown
Member

@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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsbugfeature: ldap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Cybso@MorrisJobke@zeridon@blizzz@rullzer