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 <[email protected]> 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 <[email protected]> adjust tests Signed-off-by: Arthur Schiwon <[email protected]> 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 <[email protected]> ensure that only valid group members are returned Signed-off-by: Arthur Schiwon <[email protected]>
nextcloud · Feb 28, 2020 · 2056758 · 2056758
1 parent 2ecedea
commit 2056758
Show file tree

Hide file tree

Showing 5 changed files with 96 additions and 155 deletions.
diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
@@ -811,6 +811,7 @@ private function getGroupsByMember($dn, &$seen = null) {
 	 * @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) {
@@ -862,7 +863,10 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
 				//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) {
@@ -871,17 +875,32 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
 				$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
@@ -175,6 +175,21 @@ public function update() {
 		}
 	}
 
+	/**
+	 * 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
@@ -46,7 +46,6 @@
 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 @@ public function __construct(Access $access, IConfig $ocConfig, INotificationMana
 		$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,25 +296,35 @@ public function userExistsOnLDAP($user) {
 		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
 		if(!is_array($this->access->readAttribute($dn, '', $this->access->connection->ldapUserFilter))) {
 			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 @@ public function userExistsOnLDAP($user) {
 			$user->unmark();
 		}
 
+		$this->access->connection->writeToCache($cacheKey, true);
 		return true;
 	}
 
@@ -363,15 +356,10 @@ public function userExists($uid) {
 				$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;
 	}
 
 	/**
@@ -429,21 +417,13 @@ public function getHome($uid) {
 
 		// 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
@@ -37,7 +37,8 @@
 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 @@ protected function callOnLastSeenOn($uid, $method, $parameters, $passOnWhen) {
 					&& 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 @@ public function getUsers($search = '', $limit = 10, $offset = 0) {
 	 * @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;
 	}
 
 	/**