diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index ec35fc4cae..40b8bcf16c 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -812,6 +812,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD * @param int $limit * @param int $offset * @return array with user ids + * @throws \Exception */ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { if(!$this->enabled) { @@ -863,7 +864,10 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD //we got uids, need to get their DNs to 'translate' them to user names $filter = $this->access->combineFilterWithAnd(array( str_replace('%uid', trim($member), $this->access->connection->ldapLoginFilter), - $this->access->getFilterPartForUserSearch($search) + $this->access->combineFilterWithAnd([ + $this->access->getFilterPartForUserSearch($search), + $this->access->connection->ldapUserFilter + ]) )); $ldap_users = $this->access->fetchListOfUsers($filter, $attrs, 1); if(count($ldap_users) < 1) { @@ -872,17 +876,32 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD $groupUsers[] = $this->access->dn2username($ldap_users[0]['dn'][0]); } else { //we got DNs, check if we need to filter by search or we can give back all of them - if ($search !== '') { - if(!$this->access->readAttribute($member, + $uid = $this->access->dn2username($member); + if(!$uid) { + continue; + } + + $cacheKey = 'userExistsOnLDAP' . $uid; + $userExists = $this->access->connection->getFromCache($cacheKey); + if($userExists === false) { + continue; + } + if($userExists === null || $search !== '') { + if (!$this->access->readAttribute($member, $this->access->connection->ldapUserDisplayName, - $this->access->getFilterPartForUserSearch($search))) { + $this->access->combineFilterWithAnd([ + $this->access->getFilterPartForUserSearch($search), + $this->access->connection->ldapUserFilter + ]))) + { + if($search === '') { + $this->access->connection->writeToCache($cacheKey, false); + } continue; } + $this->access->connection->writeToCache($cacheKey, true); } - // dn2username will also check if the users belong to the allowed base - if($ocname = $this->access->dn2username($member)) { - $groupUsers[] = $ocname; - } + $groupUsers[] = $uid; } } diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index 364f5f8cfa..dea5d91c0c 100644 --- a/apps/user_ldap/lib/User/User.php +++ b/apps/user_ldap/lib/User/User.php @@ -175,6 +175,21 @@ class User { } } + /** + * marks a user as deleted + * + * @throws \OCP\PreConditionNotMetException + */ + public function markUser() { + $curValue = $this->config->getUserValue($this->getUsername(), 'user_ldap', 'isDeleted', '0'); + if($curValue === '1') { + // the user is already marked, do not write to DB again + return; + } + $this->config->setUserValue($this->getUsername(), 'user_ldap', 'isDeleted', '1'); + $this->config->setUserValue($this->getUsername(), 'user_ldap', 'foundDeleted', (string)time()); + } + /** * processes results from LDAP for attributes as returned by getAttributesToRead() * @param array $ldapEntry the user entry as retrieved from LDAP diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php index b62a5f95a4..323e59860f 100644 --- a/apps/user_ldap/lib/User_LDAP.php +++ b/apps/user_ldap/lib/User_LDAP.php @@ -47,7 +47,6 @@ use OCA\User_LDAP\User\OfflineUser; use OCA\User_LDAP\User\User; use OCP\IConfig; use OCP\ILogger; -use OCP\IUser; use OCP\IUserSession; use OCP\Notification\IManager as INotificationManager; use OCP\Util; @@ -59,9 +58,6 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn /** @var INotificationManager */ protected $notificationManager; - /** @var string */ - protected $currentUserInDeletionProcess; - /** @var UserPluginManager */ protected $userPluginManager; @@ -76,20 +72,6 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn $this->ocConfig = $ocConfig; $this->notificationManager = $notificationManager; $this->userPluginManager = $userPluginManager; - $this->registerHooks($userSession); - } - - protected function registerHooks(IUserSession $userSession) { - $userSession->listen('\OC\User', 'preDelete', [$this, 'preDeleteUser']); - $userSession->listen('\OC\User', 'postDelete', [$this, 'postDeleteUser']); - } - - public function preDeleteUser(IUser $user) { - $this->currentUserInDeletionProcess = $user->getUID(); - } - - public function postDeleteUser() { - $this->currentUserInDeletionProcess = null; } /** @@ -315,6 +297,12 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn if(is_null($user)) { return false; } + $uid = $user instanceof User ? $user->getUsername() : $user->getOCName(); + $cacheKey = 'userExistsOnLDAP' . $uid; + $userExists = $this->access->connection->getFromCache($cacheKey); + if(!is_null($userExists)) { + return (bool)$userExists; + } $dn = $user->getDN(); //check if user really still exists by reading its entry @@ -322,18 +310,22 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn try { $uuid = $this->access->getUserMapper()->getUUIDByDN($dn); if (!$uuid) { + $this->access->connection->writeToCache($cacheKey, false); return false; } $newDn = $this->access->getUserDnByUuid($uuid); //check if renamed user is still valid by reapplying the ldap filter if ($newDn === $dn || !is_array($this->access->readAttribute($newDn, '', $this->access->connection->ldapUserFilter))) { + $this->access->connection->writeToCache($cacheKey, false); return false; } $this->access->getUserMapper()->setDNbyUUID($newDn, $uuid); + $this->access->connection->writeToCache($cacheKey, true); return true; } catch (ServerNotAvailableException $e) { throw $e; } catch (\Exception $e) { + $this->access->connection->writeToCache($cacheKey, false); return false; } } @@ -342,6 +334,7 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn $user->unmark(); } + $this->access->connection->writeToCache($cacheKey, true); return true; } @@ -364,15 +357,10 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn $this->access->connection->ldapHost, ILogger::DEBUG); $this->access->connection->writeToCache('userExists'.$uid, false); return false; - } else if($user instanceof OfflineUser) { - //express check for users marked as deleted. Returning true is - //necessary for cleanup - return true; } - $result = $this->userExistsOnLDAP($user); - $this->access->connection->writeToCache('userExists'.$uid, $result); - return $result; + $this->access->connection->writeToCache('userExists'.$uid, true); + return true; } /** @@ -430,21 +418,13 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn // early return path if it is a deleted user $user = $this->access->userManager->get($uid); - if($user instanceof OfflineUser) { - if($this->currentUserInDeletionProcess !== null - && $this->currentUserInDeletionProcess === $user->getOCName() - ) { - return $user->getHomePath(); - } else { - throw new NoUserException($uid . ' is not a valid user anymore'); - } - } else if ($user === null) { + if($user instanceof User || $user instanceof OfflineUser) { + $path = $user->getHomePath() ?: false; + } else { throw new NoUserException($uid . ' is not a valid user anymore'); } - $path = $user->getHomePath(); $this->access->cacheUserHome($uid, $path); - return $path; } diff --git a/apps/user_ldap/lib/User_Proxy.php b/apps/user_ldap/lib/User_Proxy.php index 6fdaa2998e..96be4a7529 100644 --- a/apps/user_ldap/lib/User_Proxy.php +++ b/apps/user_ldap/lib/User_Proxy.php @@ -37,7 +37,8 @@ use OCP\IUserSession; use OCP\Notification\IManager as INotificationManager; class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface, IUserLDAP { - private $backends = array(); + private $backends = []; + /** @var User_LDAP */ private $refBackend = null; /** @@ -49,9 +50,14 @@ class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface, * @param INotificationManager $notificationManager * @param IUserSession $userSession */ - public function __construct(array $serverConfigPrefixes, ILDAPWrapper $ldap, IConfig $ocConfig, - INotificationManager $notificationManager, IUserSession $userSession, - UserPluginManager $userPluginManager) { + public function __construct( + array $serverConfigPrefixes, + ILDAPWrapper $ldap, + IConfig $ocConfig, + INotificationManager $notificationManager, + IUserSession $userSession, + UserPluginManager $userPluginManager + ) { parent::__construct($ldap); foreach($serverConfigPrefixes as $configPrefix) { $this->backends[$configPrefix] = @@ -105,13 +111,13 @@ class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface, && method_exists($this->getAccess($prefix), $method)) { $instance = $this->getAccess($prefix); } - $result = call_user_func_array(array($instance, $method), $parameters); + $result = call_user_func_array([$instance, $method], $parameters); if($result === $passOnWhen) { //not found here, reset cache to null if user vanished //because sometimes methods return false with a reason $userExists = call_user_func_array( - array($this->backends[$prefix], 'userExists'), - array($uid) + [$this->backends[$prefix], 'userExistsOnLDAP'], + [$uid] ); if(!$userExists) { $this->writeToCache($cacheKey, null); @@ -170,7 +176,22 @@ class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface, * @return boolean */ public function userExists($uid) { - return $this->handleRequest($uid, 'userExists', array($uid)); + $existsOnLDAP = false; + $existsLocally = $this->handleRequest($uid, 'userExists', array($uid)); + if($existsLocally) { + $existsOnLDAP = $this->userExistsOnLDAP($uid); + } + if($existsLocally && !$existsOnLDAP) { + try { + $user = $this->getLDAPAccess($uid)->userManager->get($uid); + if($user instanceof User) { + $user->markUser(); + } + } catch (\Exception $e) { + // ignore + } + } + return $existsLocally; } /** diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index f80b043016..7ea4cb463d 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -1054,7 +1054,7 @@ class Group_LDAPTest extends TestCase { $ldap = new GroupLDAP($access, $pluginManager); $resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]); - $this->assertEquals($expectedMembers, $resultingMembers, '', 0.0, 10, true); + $this->assertEqualsCanonicalizing($expectedMembers, $resultingMembers); } public function displayNameProvider() { diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php index 7982ef0f42..e4a105b2bd 100644 --- a/apps/user_ldap/tests/User_LDAPTest.php +++ b/apps/user_ldap/tests/User_LDAPTest.php @@ -314,22 +314,12 @@ class User_LDAPTest extends TestCase { $offlineUser->expects($this->once()) ->method('getHomePath') ->willReturn($home); - $offlineUser->expects($this->once()) - ->method('getOCName') - ->willReturn($uid); $this->userManager->expects($this->atLeastOnce()) ->method('get') ->willReturn($offlineUser); $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); - /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->createMock(IUser::class); - $user->expects($this->once()) - ->method('getUID') - ->willReturn($uid); - - $backend->preDeleteUser($user); $result = $backend->deleteUser($uid); $this->assertTrue($result); /** @noinspection PhpUnhandledExceptionInspection */ @@ -509,18 +499,7 @@ class User_LDAPTest extends TestCase { $this->prepareMockForUserExists(); $user = $this->createMock(User::class); - $user->expects($this->any()) - ->method('getDN') - ->willReturn('dnOfRoland,dc=test'); - $this->access->expects($this->any()) - ->method('readAttribute') - ->will($this->returnCallback(function($dn) { - if($dn === 'dnOfRoland,dc=test') { - return array(); - } - return false; - })); $this->userManager->expects($this->atLeastOnce()) ->method('get') ->willReturn($user); @@ -544,32 +523,18 @@ class User_LDAPTest extends TestCase { ->with('dnOfFormerUser,dc=test') ->willReturn('45673458748'); - $this->access->expects($this->any()) - ->method('readAttribute') - ->will($this->returnCallback(function($dn) { - if($dn === 'dnOfRoland,dc=test') { - return array(); - } - return false; - })); $this->access->expects($this->any()) ->method('getUserMapper') ->willReturn($mapper); - $this->access->expects($this->once()) - ->method('getUserDnByUuid') - ->willThrowException(new \Exception()); $user = $this->createMock(User::class); - $user->expects($this->any()) - ->method('getDN') - ->willReturn('dnOfFormerUser,dc=test'); $this->userManager->expects($this->atLeastOnce()) ->method('get') ->willReturn($user); - //test for deleted user - $this->assertFalse($backend->userExists('formerUser')); + //test for deleted user – always returns true as long as we have the user in DB + $this->assertTrue($backend->userExists('formerUser')); } public function testUserExistsForNeverExisting() { @@ -621,64 +586,6 @@ class User_LDAPTest extends TestCase { $this->assertTrue($result); } - public function testUserExistsPublicAPIForDeleted() { - $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); - $this->prepareMockForUserExists(); - \OC_User::useBackend($backend); - - $mapper = $this->createMock(UserMapping::class); - $mapper->expects($this->any()) - ->method('getUUIDByDN') - ->with('dnOfFormerUser,dc=test') - ->willReturn('45673458748'); - - $this->access->expects($this->any()) - ->method('readAttribute') - ->will($this->returnCallback(function($dn) { - if($dn === 'dnOfRoland,dc=test') { - return array(); - } - return false; - })); - $this->access->expects($this->any()) - ->method('getUserMapper') - ->willReturn($mapper); - $this->access->expects($this->once()) - ->method('getUserDnByUuid') - ->willThrowException(new \Exception()); - - $user = $this->createMock(User::class); - $user->expects($this->any()) - ->method('getDN') - ->willReturn('dnOfFormerUser,dc=test'); - - $this->userManager->expects($this->atLeastOnce()) - ->method('get') - ->willReturn($user); - - //test for deleted user - $this->assertFalse(\OC::$server->getUserManager()->userExists('formerUser')); - } - - public function testUserExistsPublicAPIForNeverExisting() { - $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); - $this->prepareMockForUserExists(); - \OC_User::useBackend($backend); - - $this->access->expects($this->any()) - ->method('readAttribute') - ->will($this->returnCallback(function($dn) { - if($dn === 'dnOfRoland,dc=test') { - return array(); - } - return false; - })); - - //test for never-existing user - $result = \OC::$server->getUserManager()->userExists('mallory'); - $this->assertFalse($result); - } - public function testDeleteUserExisting() { $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); @@ -787,7 +694,7 @@ class User_LDAPTest extends TestCase { $this->assertEquals($dataDir.'/susannah/', $result); } - + public function testGetHomeNoPath() { $this->expectException(\Exception::class); @@ -836,10 +743,7 @@ class User_LDAPTest extends TestCase { $this->assertFalse($result); } - public function testGetHomeDeletedUser() { - $this->expectException(\OC\User\NoUserException::class); - $uid = 'newyorker'; $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager); @@ -869,14 +773,16 @@ class User_LDAPTest extends TestCase { ->will($this->returnValue(true)); $offlineUser = $this->createMock(OfflineUser::class); - $offlineUser->expects($this->never()) - ->method('getHomePath'); + $offlineUser->expects($this->atLeastOnce()) + ->method('getHomePath') + ->willReturn(''); $this->userManager->expects($this->atLeastOnce()) ->method('get') ->willReturn($offlineUser); - $backend->getHome($uid); + $result = $backend->getHome($uid); + $this->assertFalse($result); } public function testGetHomeWithPlugin() { @@ -1112,7 +1018,7 @@ class User_LDAPTest extends TestCase { ->willReturn(42); $this->assertEquals($this->backend->countUsers(),42); - } + } public function testLoginName2UserNameSuccess() { $loginName = 'Alice'; @@ -1280,7 +1186,7 @@ class User_LDAPTest extends TestCase { })); } - + public function testSetPasswordInvalid() { $this->expectException(\OC\HintException::class); $this->expectExceptionMessage('Password fails quality checking policy'); @@ -1294,7 +1200,7 @@ class User_LDAPTest extends TestCase { $this->assertTrue(\OC_User::setPassword('roland', 'dt')); } - + public function testSetPasswordValid() { $this->prepareAccessForSetPassword($this->access); @@ -1324,7 +1230,7 @@ class User_LDAPTest extends TestCase { $this->assertFalse(\OC_User::setPassword('roland', 'dt12234$')); } - + public function testSetPasswordWithInvalidUser() { $this->expectException(\Exception::class); $this->expectExceptionMessage('LDAP setPassword: Could not get user object for uid NotExistingUser. Maybe the LDAP entry has no set display name attribute?'); @@ -1425,7 +1331,7 @@ class User_LDAPTest extends TestCase { $this->assertEquals($newDisplayName, $this->backend->setDisplayName('uid', $newDisplayName)); } - + public function testSetDisplayNameErrorWithPlugin() { $this->expectException(\OC\HintException::class);