Merge pull request #21133 from owncloud/fix-shared-files-of-deleted-users

Fix shared files of deleted users, detect DN change when checking for existence on LDAP

apps/user_ldap/lib/access.php

@@ -1276,6 +1276,54 @@ class Access extends LDAPUtility implements user\IUserTools {
 		return $result;
 	}
 
+	/**
+	 * reverse lookup of a DN given a known UUID
+	 *
+	 * @param string $uuid
+	 * @return string
+	 * @throws \Exception
+	 */
+	public function getUserDnByUuid($uuid) {
+		$uuidOverride = $this->connection->ldapExpertUUIDUserAttr;
+		$filter       = $this->connection->ldapUserFilter;
+		$base         = $this->connection->ldapBaseUsers;
+
+		if($this->connection->ldapUuidUserAttribute === 'auto' && empty($uuidOverride)) {
+			// Sacrebleu! The UUID attribute is unknown :( We need first an
+			// existing DN to be able to reliably detect it.
+			$result = $this->search($filter, $base, ['dn'], 1);
+			if(!isset($result[0]) || !isset($result[0]['dn'])) {
+				throw new \Exception('Cannot determine UUID attribute');
+			}
+			$dn = $result[0]['dn'][0];
+			if(!$this->detectUuidAttribute($dn, true)) {
+				throw new \Exception('Cannot determine UUID attribute');
+			}
+		} else {
+			// The UUID attribute is either known or an override is given.
+			// By calling this method we ensure that $this->connection->$uuidAttr
+			// is definitely set
+			if(!$this->detectUuidAttribute('', true)) {
+				throw new \Exception('Cannot determine UUID attribute');
+			}
+		}
+
+		$uuidAttr = $this->connection->ldapUuidUserAttribute;
+		if($uuidAttr === 'guid' || $uuidAttr === 'objectguid') {
+			$uuid = $this->formatGuid2ForFilterUser($uuid);
+		}
+
+		$filter = $uuidAttr . '=' . $uuid;
+		$result = $this->searchUsers($filter, ['dn'], 2);
+		if(is_array($result) && isset($result[0]) && isset($result[0]['dn']) && count($result) === 1) {
+			// we put the count into account to make sure that this is
+			// really unique
+			return $result[0]['dn'][0];
+		}
+
+		throw new \Exception('Cannot determine UUID attribute');
+	}
+
 	/**
 	 * auto-detects the directory's UUID attribute
 	 * @param string $dn a known DN used to check against
@@ -1378,6 +1426,53 @@ class Access extends LDAPUtility implements user\IUserTools {
 		return strtoupper($hex_guid_to_guid_str);
 	}
 
+	/**
+	 * the first three blocks of the string-converted GUID happen to be in
+	 * reverse order. In order to use it in a filter, this needs to be
+	 * corrected. Furthermore the dashes need to be replaced and \\ preprended
+	 * to every two hax figures.
+	 *
+	 * If an invalid string is passed, it will be returned without change.
+	 *
+	 * @param string $guid
+	 * @return string
+	 */
+	public function formatGuid2ForFilterUser($guid) {
+		if(!is_string($guid)) {
+			throw new \InvalidArgumentException('String expected');
+		}
+		$blocks = explode('-', $guid);
+		if(count($blocks) !== 5) {
+			/*
+			 * Why not throw an Exception instead? This method is a utility
+			 * called only when trying to figure out whether a "missing" known
+			 * LDAP user was or was not renamed on the LDAP server. And this
+			 * even on the use case that a reverse lookup is needed (UUID known,
+			 * not DN), i.e. when finding users (search dialog, users page,
+			 * login, …) this will not be fired. This occurs only if shares from
+			 * a users are supposed to be mounted who cannot be found. Throwing
+			 * an exception here would kill the experience for a valid, acting
+			 * user. Instead we write a log message.
+			 */
+			\OC::$server->getLogger()->info(
+				'Passed string does not resemble a valid GUID. Known UUID ' .
+				'({uuid}) probably does not match UUID configuration.',
+				[ 'app' => 'user_ldap', 'uuid' => $guid ]
+			);
+			return $guid;
+		}
+		for($i=0; $i < 3; $i++) {
+			$pairs = str_split($blocks[$i], 2);
+			$pairs = array_reverse($pairs);
+			$blocks[$i] = implode('', $pairs);
+		}
+		for($i=0; $i < 5; $i++) {
+			$pairs = str_split($blocks[$i], 2);
+			$blocks[$i] = '\\' . implode('\\', $pairs);
+		}
+		return implode('', $blocks);
+	}
+
 	/**
 	 * gets a SID of the domain of the given dn
 	 * @param string $dn

apps/user_ldap/lib/mapping/abstractmapping.php

@@ -158,7 +158,7 @@ abstract class AbstractMapping {
 	}
 
 	/**
-	 * Gets the name based on the provided LDAP DN.
+	 * Gets the name based on the provided LDAP UUID.
 	 * @param string $uuid
 	 * @return string|false
 	 */
@@ -166,6 +166,16 @@ abstract class AbstractMapping {
 		return $this->getXbyY('owncloud_name', 'directory_uuid', $uuid);
 	}
 
+	/**
+	 * Gets the UUID based on the provided LDAP DN
+	 * @param string $dn
+	 * @return false|string
+	 * @throws \Exception
+	 */
+	public function getUUIDByDN($dn) {
+		return $this->getXbyY('directory_uuid', 'ldap_dn', $dn);
+	}
+
 	/**
 	 * gets a piece of the mapping list
 	 * @param int $offset

apps/user_ldap/tests/user_ldap.php

@@ -70,14 +70,26 @@ class Test_User_Ldap_Direct extends \Test\TestCase {
 									array($lw, null, null));
 
 		$this->configMock = $this->getMock('\OCP\IConfig');
-		$um = new \OCA\user_ldap\lib\user\Manager(
+
+		$offlineUser = $this->getMockBuilder('\OCA\user_ldap\lib\user\OfflineUser')
+			->disableOriginalConstructor()
+			->getMock();
+
+		$um = $this->getMockBuilder('\OCA\user_ldap\lib\user\Manager')
+			->setMethods(['getDeletedUser'])
+			->setConstructorArgs([
 				$this->configMock,
 				$this->getMock('\OCA\user_ldap\lib\FilesystemHelper'),
 				$this->getMock('\OCA\user_ldap\lib\LogWrapper'),
 				$this->getMock('\OCP\IAvatarManager'),
 				$this->getMock('\OCP\Image'),
 				$this->getMock('\OCP\IDBConnection')
-			);
+			  ])
+			->getMock();
+
+		$um->expects($this->any())
+			->method('getDeletedUser')
+			->will($this->returnValue($offlineUser));
 
 		$access = $this->getMock('\OCA\user_ldap\lib\Access',
 								 $accMethods,
@@ -661,6 +673,44 @@ class Test_User_Ldap_Direct extends \Test\TestCase {
 		$this->assertFalse($result);
 	}
 
+	/**
+	 * @expectedException \OC\User\NoUserException
+	 */
+	public function testGetHomeDeletedUser() {
+		$access = $this->getAccessMock();
+		$backend = new UserLDAP($access, $this->getMock('\OCP\IConfig'));
+		$this->prepareMockForUserExists($access);
+
+		$access->connection->expects($this->any())
+				->method('__get')
+				->will($this->returnCallback(function($name) {
+					if($name === 'homeFolderNamingRule') {
+						return 'attr:testAttribute';
+					}
+					return null;
+				}));
+
+		$access->expects($this->any())
+				->method('readAttribute')
+				->will($this->returnValue([]));
+
+		$userMapper = $this->getMockBuilder('\OCA\User_LDAP\Mapping\UserMapping')
+				->disableOriginalConstructor()
+				->getMock();
+
+		$access->expects($this->any())
+				->method('getUserMapper')
+				->will($this->returnValue($userMapper));
+
+		$this->configMock->expects($this->any())
+			->method('getUserValue')
+			->will($this->returnValue(true));
+
+		//no path at all – triggers OC default behaviour
+		$result = $backend->getHome('newyorker');
+		$this->assertFalse($result);
+	}
+
 	private function prepareAccessForGetDisplayName(&$access) {
 		$access->connection->expects($this->any())
 			   ->method('__get')
@@ -686,6 +736,14 @@ class Test_User_Ldap_Direct extends \Test\TestCase {
 							return false;
 				   }
 			   }));
+
+		$userMapper = $this->getMockBuilder('\OCA\User_LDAP\Mapping\UserMapping')
+			->disableOriginalConstructor()
+			->getMock();
+
+		$access->expects($this->any())
+			->method('getUserMapper')
+			->will($this->returnValue($userMapper));
 	}
 
 	public function testGetDisplayName() {

apps/user_ldap/user_ldap.php

@@ -30,6 +30,7 @@
 
 namespace OCA\user_ldap;
 
+use OC\User\NoUserException;
 use OCA\user_ldap\lib\BackendUtility;
 use OCA\user_ldap\lib\Access;
 use OCA\user_ldap\lib\user\OfflineUser;
@@ -190,15 +191,18 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 
 	/**
 	 * checks whether a user is still available on LDAP
+	 *
 	 * @param string|\OCA\User_LDAP\lib\user\User $user either the ownCloud user
 	 * name or an instance of that user
 	 * @return bool
+	 * @throws \Exception
+	 * @throws \OC\ServerNotAvailableException
 	 */
 	public function userExistsOnLDAP($user) {
 		if(is_string($user)) {
 			$user = $this->access->userManager->get($user);
 		}
-		if(!$user instanceof User) {
+		if(is_null($user)) {
 			return false;
 		}
 
@@ -209,8 +213,23 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 			if(is_null($lcr)) {
 				throw new \Exception('No LDAP Connection to server ' . $this->access->connection->ldapHost);
 			}
+
+			try {
+				$uuid = $this->access->getUserMapper()->getUUIDByDN($dn);
+				if(!$uuid) {
 					return false;
 				}
+				$newDn = $this->access->getUserDnByUuid($uuid);
+				$this->access->getUserMapper()->setDNbyUUID($newDn, $uuid);
+				return true;
+			} catch (\Exception $e) {
+				return false;
+			}
+		}
+
+		if($user instanceof OfflineUser) {
+			$user->unmark();
+		}
 
 		return true;
 	}
@@ -275,8 +294,11 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 
 	/**
 	 * get the user's home directory
+	 *
 	 * @param string $uid the username
-	* @return string|bool
+	 * @return bool|string
+	 * @throws NoUserException
+	 * @throws \Exception
 	 */
 	public function getHome($uid) {
 		if(isset($this->homesToKill[$uid]) && !empty($this->homesToKill[$uid])) {
@@ -295,6 +317,15 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 		}
 
 		$user = $this->access->userManager->get($uid);
+		if(is_null($user) || ($user instanceof OfflineUser && !$this->userExistsOnLDAP($user->getOCName()))) {
+			throw new NoUserException($uid . ' is not a valid user anymore');
+		}
+		if($user instanceof OfflineUser) {
+			// apparently this user survived the userExistsOnLDAP check,
+			// we request the user instance again in order to retrieve a User
+			// instance instead
+			$user = $this->access->userManager->get($uid);
+		}
 		$path = $user->getHomePath();
 		$this->access->cacheUserHome($uid, $path);