relax strict getHome behaviour for LDAP users in a shadow state

* simplifies deletion process
* less strange behaviour when looking up home storage (as long as it is local)
* thus could enable transfer ownerships after user went invisible on ldap

backport of #17717

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

decouple userExists from userExistsOnLDAP check

allows to mark users as offline right away, avoids a gap of being not a
user and causing weird side effects

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

adjust tests

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

remove superfluous tests

- user_ldap is not exposed to public api, it is always behind ldap_proxy
- this is too much for a unit test
- integration tests cover userExists implicitly

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

ensure that only valid group members are returned

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

apps/user_ldap/lib/Group_LDAP.php

@@ -810,6 +810,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) {
@@ -861,7 +862,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) {
@@ -870,17 +874,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 !== '') {
+				$uid = $this->access->dn2username($member);
-					if(!$this->access->readAttribute($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
+				$groupUsers[] = $uid;
-				if($ocname = $this->access->dn2username($member)) {
-					$groupUsers[] = $ocname;
-				}
 			}
 		}
 

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

apps/user_ldap/lib/User_LDAP.php

@@ -46,7 +46,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;
@@ -58,9 +57,6 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 	/** @var INotificationManager */
 	protected $notificationManager;
 
-	/** @var string */
-	protected $currentUserInDeletionProcess;
-
 	/** @var UserPluginManager */
 	protected $userPluginManager;
 
@@ -75,20 +71,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;
 	}
 
 	/**
@@ -314,6 +296,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
@@ -321,18 +309,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;
 			}
 		}
@@ -341,6 +333,7 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 			$user->unmark();
 		}
 
+		$this->access->connection->writeToCache($cacheKey, true);
 		return true;
 	}
 
@@ -363,15 +356,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, true);
-		$this->access->connection->writeToCache('userExists'.$uid, $result);
+		return true;
-		return $result;
 	}
 
 	/**
@@ -429,21 +417,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($user instanceof User || $user instanceof OfflineUser) {
-			if($this->currentUserInDeletionProcess !== null
+			$path = $user->getHomePath() ?: false;
-				&& $this->currentUserInDeletionProcess === $user->getOCName()
+		} else {
-			) {
-				return $user->getHomePath();
-			} else {
-				throw new NoUserException($uid . ' is not a valid user anymore');
-			}
-		} else if ($user === null) {
 			throw new NoUserException($uid . ' is not a valid user anymore');
 		}
 
-		$path = $user->getHomePath();
 		$this->access->cacheUserHome($uid, $path);
-
 		return $path;
 	}
 

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,
+	public function __construct(
-		INotificationManager $notificationManager, IUserSession $userSession,
+		array $serverConfigPrefixes,
-								UserPluginManager $userPluginManager) {
+		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'),
+						[$this->backends[$prefix], 'userExistsOnLDAP'],
-						array($uid)
+						[$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;
 	}
 
 	/**

apps/user_ldap/tests/User_LDAPTest.php

@@ -313,22 +313,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 */
@@ -508,18 +498,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);
@@ -543,32 +522,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
+		//test for deleted user – always returns true as long as we have the user in DB
-		$this->assertFalse($backend->userExists('formerUser'));
+		$this->assertTrue($backend->userExists('formerUser'));
 	}
 
 	public function testUserExistsForNeverExisting() {
@@ -620,64 +585,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);
 
@@ -835,9 +742,6 @@ class User_LDAPTest extends TestCase {
 		$this->assertFalse($result);
 	}
 
-	/**
-	 * @expectedException \OC\User\NoUserException
-	 */
 	public function testGetHomeDeletedUser() {
 		$uid = 'newyorker';
 
@@ -868,14 +772,16 @@ class User_LDAPTest extends TestCase {
 			->will($this->returnValue(true));
 
 		$offlineUser = $this->createMock(OfflineUser::class);
-		$offlineUser->expects($this->never())
+		$offlineUser->expects($this->atLeastOnce())
-			->method('getHomePath');
+			->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() {
@@ -1111,7 +1017,7 @@ class User_LDAPTest extends TestCase {
 			->willReturn(42);
 
 		$this->assertEquals($this->backend->countUsers(),42);
-	}	
+	}
 
 	public function testLoginName2UserNameSuccess() {
 		$loginName = 'Alice';
@@ -1293,7 +1199,7 @@ class User_LDAPTest extends TestCase {
 
 		$this->assertTrue(\OC_User::setPassword('roland', 'dt'));
 	}
-	
+
 	public function testSetPasswordValid() {
 		$this->prepareAccessForSetPassword($this->access);