From 8c7930015644fdd3121dd8399c975b7d15bf40a7 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 11 Dec 2015 00:12:41 +0100 Subject: [PATCH 1/5] throw NoUserException in getHome when the requested user does not exist anymore --- apps/user_ldap/user_ldap.php | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/apps/user_ldap/user_ldap.php b/apps/user_ldap/user_ldap.php index 0097dda89b..fc62c16857 100644 --- a/apps/user_ldap/user_ldap.php +++ b/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; } @@ -212,6 +216,10 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn return false; } + if($user instanceof OfflineUser) { + $user->unmark(); + } + return true; } @@ -274,10 +282,13 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn } /** - * get the user's home directory - * @param string $uid the username - * @return string|bool - */ + * get the user's home directory + * + * @param string $uid the username + * @return bool|string + * @throws NoUserException + * @throws \Exception + */ public function getHome($uid) { if(isset($this->homesToKill[$uid]) && !empty($this->homesToKill[$uid])) { //a deleted user who needs some clean up @@ -295,6 +306,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->getUID()))) { + 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); From 4020d5b77a249e75ae81f5c3646db5692488a812 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 11 Dec 2015 01:56:53 +0100 Subject: [PATCH 2/5] look for DN changes before marking a user as deleted --- apps/user_ldap/lib/access.php | 52 +++++++++++++++++++ .../user_ldap/lib/mapping/abstractmapping.php | 12 ++++- apps/user_ldap/user_ldap.php | 15 +++++- 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/apps/user_ldap/lib/access.php b/apps/user_ldap/lib/access.php index 667f107623..3be0b6818d 100644 --- a/apps/user_ldap/lib/access.php +++ b/apps/user_ldap/lib/access.php @@ -1276,6 +1276,58 @@ 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') { + $dn = ''; + $result = $this->readAttribute($dn, 'dn'); + if(is_array($result) && isset($result[0])) { + return $result[0]; + } + } else { + $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 diff --git a/apps/user_ldap/lib/mapping/abstractmapping.php b/apps/user_ldap/lib/mapping/abstractmapping.php index f0f0f6df75..c3d38ce8b7 100644 --- a/apps/user_ldap/lib/mapping/abstractmapping.php +++ b/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 diff --git a/apps/user_ldap/user_ldap.php b/apps/user_ldap/user_ldap.php index fc62c16857..a266be7b7f 100644 --- a/apps/user_ldap/user_ldap.php +++ b/apps/user_ldap/user_ldap.php @@ -213,7 +213,18 @@ 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); } - return false; + + 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) { @@ -306,7 +317,7 @@ 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->getUID()))) { + 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) { From aedbc0c6269aa16a733bd025aeb6d1a2360743fe Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 11 Dec 2015 12:52:12 +0100 Subject: [PATCH 3/5] adjust unit test --- apps/user_ldap/tests/user_ldap.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/apps/user_ldap/tests/user_ldap.php b/apps/user_ldap/tests/user_ldap.php index 7593371d85..b08706c71d 100644 --- a/apps/user_ldap/tests/user_ldap.php +++ b/apps/user_ldap/tests/user_ldap.php @@ -686,6 +686,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() { From 35a2639701ffdd257adbd7270ef8648c4ae9f15b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 11 Dec 2015 17:25:57 +0100 Subject: [PATCH 4/5] unit test on getHome in combination with OfflineUser --- apps/user_ldap/tests/user_ldap.php | 54 ++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/apps/user_ldap/tests/user_ldap.php b/apps/user_ldap/tests/user_ldap.php index b08706c71d..3aec2a5ce5 100644 --- a/apps/user_ldap/tests/user_ldap.php +++ b/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') From e39415c946338c3093257a21d09a7360e0c9ffd4 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 14 Dec 2015 22:42:27 +0100 Subject: [PATCH 5/5] fix find DN by UUID for AD --- apps/user_ldap/lib/access.php | 69 ++++++++++++++++++++++++++++------- 1 file changed, 56 insertions(+), 13 deletions(-) diff --git a/apps/user_ldap/lib/access.php b/apps/user_ldap/lib/access.php index 3be0b6818d..693a420a74 100644 --- a/apps/user_ldap/lib/access.php +++ b/apps/user_ldap/lib/access.php @@ -1310,19 +1310,15 @@ class Access extends LDAPUtility implements user\IUserTools { $uuidAttr = $this->connection->ldapUuidUserAttribute; if($uuidAttr === 'guid' || $uuidAttr === 'objectguid') { - $dn = ''; - $result = $this->readAttribute($dn, 'dn'); - if(is_array($result) && isset($result[0])) { - return $result[0]; - } - } else { - $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]; - } + $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'); @@ -1430,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