From 9d03b7c6b7131761f29f79e6c33473b0ca483856 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 21 Aug 2015 00:55:42 +0200 Subject: [PATCH 1/3] read all relevant user attributes on login and user search, in one query. saves us some. --- apps/user_ldap/lib/access.php | 29 ++++- apps/user_ldap/lib/user/manager.php | 37 ++++++ apps/user_ldap/lib/user/user.php | 180 ++++++++++++++++++++++---- apps/user_ldap/tests/access.php | 67 ++++++++-- apps/user_ldap/tests/group_ldap.php | 6 +- apps/user_ldap/tests/user/manager.php | 42 ++++++ apps/user_ldap/tests/user/user.php | 65 +++++++++- apps/user_ldap/tests/user_ldap.php | 18 ++- apps/user_ldap/user_ldap.php | 60 ++------- 9 files changed, 404 insertions(+), 100 deletions(-) diff --git a/apps/user_ldap/lib/access.php b/apps/user_ldap/lib/access.php index a2eb834b49..fe9eefb311 100644 --- a/apps/user_ldap/lib/access.php +++ b/apps/user_ldap/lib/access.php @@ -536,6 +536,16 @@ class Access extends LDAPUtility implements user\IUserTools { return $ownCloudNames; } + /** + * caches the user display name + * @param string $ocName the internal ownCloud username + * @param string|false $home the home directory path + */ + public function cacheUserHome($ocName, $home) { + $cacheKey = 'getHome'.$ocName; + $this->connection->writeToCache($cacheKey, $home); + } + /** * caches a user as existing * @param string $ocName the internal ownCloud username @@ -656,7 +666,24 @@ class Access extends LDAPUtility implements user\IUserTools { * @return array */ public function fetchListOfUsers($filter, $attr, $limit = null, $offset = null) { - return $this->fetchList($this->searchUsers($filter, $attr, $limit, $offset), (count($attr) > 1)); + $ldapRecords = $this->searchUsers($filter, $attr, $limit, $offset); + $this->batchApplyUserAttributes($ldapRecords); + return $this->fetchList($ldapRecords, (count($attr) > 1)); + } + + /** + * provided with an array of LDAP user records the method will fetch the + * user object and requests it to process the freshly fetched attributes and + * and their values + * @param array $ldapRecords + */ + public function batchApplyUserAttributes(array $ldapRecords){ + foreach($ldapRecords as $userRecord) { + $ocName = $this->dn2ocname($userRecord['dn'], $userRecord[$this->connection->ldapUserDisplayName]); + $this->cacheUserExists($ocName); + $user = $this->userManager->get($ocName); + $user->processAttributes($userRecord); + } } /** diff --git a/apps/user_ldap/lib/user/manager.php b/apps/user_ldap/lib/user/manager.php index b70e057741..86f0b8991d 100644 --- a/apps/user_ldap/lib/user/manager.php +++ b/apps/user_ldap/lib/user/manager.php @@ -126,6 +126,43 @@ class Manager { } } + /** + * returns a list of attributes that will be processed further, e.g. quota, + * email, displayname, or others. + * @param bool $minimal - optional, set to true to skip attributes with big + * payload + * @return string[] + */ + public function getAttributes($minimal = false) { + $attributes = array('dn', 'uid', 'samaccountname', 'memberof'); + $possible = array( + $this->access->getConnection()->ldapQuotaAttribute, + $this->access->getConnection()->ldapEmailAttribute, + $this->access->getConnection()->ldapUserDisplayName, + ); + foreach($possible as $attr) { + if(!is_null($attr)) { + $attributes[] = $attr; + } + } + + $homeRule = $this->access->getConnection()->homeFolderNamingRule; + if(strpos($homeRule, 'attr:') === 0) { + $attributes[] = substr($homeRule, strlen('attr:')); + } + + if(!$minimal) { + // attributes that are not really important but may come with big + // payload. + $attributes = array_merge($attributes, array( + 'jpegphoto', + 'thumbnailphoto' + )); + } + + return $attributes; + } + /** * Checks whether the specified user is marked as deleted * @param string $id the ownCloud user name diff --git a/apps/user_ldap/lib/user/user.php b/apps/user_ldap/lib/user/user.php index ac5d8f5a37..3bfe0c2707 100644 --- a/apps/user_ldap/lib/user/user.php +++ b/apps/user_ldap/lib/user/user.php @@ -138,6 +138,69 @@ class User { } } + /** + * processes results from LDAP for attributes as returned by getAttributesToRead() + * @param array $ldapEntry the user entry as retrieved from LDAP + */ + public function processAttributes($ldapEntry) { + $this->markRefreshTime(); + //Quota + $attr = strtolower($this->connection->ldapQuotaAttribute); + if(isset($ldapEntry[$attr])) { + $this->updateQuota($ldapEntry[$attr]); + } + unset($attr); + + //Email + $attr = strtolower($this->connection->ldapEmailAttribute); + if(isset($ldapEntry[$attr])) { + $this->updateEmail($ldapEntry[$attr]); + } + unset($attr); + + //displayName + $attr = strtolower($this->connection->ldapUserDisplayName); + if(isset($ldapEntry[$attr])) { + $displayName = $ldapEntry[$attr]; + if(!empty($displayName)) { + $this->storeDisplayName($displayName); + $this->access->cacheUserDisplayName($this->getUsername(), $displayName); + } + } + unset($attr); + + // LDAP Username, needed for s2s sharing + if(isset($ldapEntry['uid'])) { + $this->storeLDAPUserName($ldapEntry['uid']); + } else if(isset($ldapEntry['samaccountname'])) { + $this->storeLDAPUserName($ldapEntry['samaccountname']); + } + //homePath + if(strpos($this->connection->homeFolderNamingRule, 'attr:') === 0) { + $attr = strtolower(substr($this->connection->homeFolderNamingRule, strlen('attr:'))); + if(isset($ldapEntry[$attr])) { + $this->access->cacheUserHome( + $this->getUsername(), $this->getHomePath($ldapEntry[$attr])); + } + } + //memberOf groups + $cacheKey = 'getMemberOf'.$this->getUsername(); + $groups = false; + if(isset($ldapEntry['memberof'])) { + $groups = $ldapEntry['memberof']; + } + $this->connection->writeToCache($cacheKey, $groups); + //Avatar + $attrs = array('jpegphoto', 'thumbnailphoto'); + foreach ($attrs as $attr) { + if(isset($ldapEntry[$attr])) { + $this->avatarImage = $ldapEntry[$attr]; + $this->updateAvatar(); + break; + } + } + } + /** * @brief returns the LDAP DN of the user * @return string @@ -154,6 +217,65 @@ class User { return $this->uid; } + /** + * returns the home directory of the user if specified by LDAP settings + * @param string $valueFromLDAP + * @return bool|string + * @throws \Exception + */ + public function getHomePath($valueFromLDAP = null) { + $path = $valueFromLDAP; + + if( is_null($path) + && strpos($this->access->connection->homeFolderNamingRule, 'attr:') === 0 + && $this->access->connection->homeFolderNamingRule !== 'attr:') + { + $attr = substr($this->access->connection->homeFolderNamingRule, strlen('attr:')); + $homedir = $this->access->readAttribute( + $this->access->username2dn($this->getUsername()), $attr); + if ($homedir && isset($homedir[0])) { + $path = $homedir[0]; + } + } + + if(!empty($path)) { + //if attribute's value is an absolute path take this, otherwise append it to data dir + //check for / at the beginning or pattern c:\ resp. c:/ + if( '/' !== $path[0] + && !(3 < strlen($path) && ctype_alpha($path[0]) + && $path[1] === ':' && ('\\' === $path[2] || '/' === $path[2])) + ) { + $path = $this->config->getSystemValue('datadirectory', + \OC::$SERVERROOT.'/data' ) . '/' . $path; + } + //we need it to store it in the DB as well in case a user gets + //deleted so we can clean up afterwards + $this->config->setUserValue( + $this->getUsername(), 'user_ldap', 'homePath', $path + ); + return $path; + } + + if($this->config->getAppValue('user_ldap', 'enforce_home_folder_naming_rule', true)) { + // a naming rule attribute is defined, but it doesn't exist for that LDAP user + throw new \Exception('Home dir attribute can\'t be read from LDAP for uid: ' . $this->getUsername()); + } + + //false will apply default behaviour as defined and done by OC_User + $this->config->setUserValue($this->getUsername(), 'user_ldap', 'homePath', ''); + return false; + } + + public function getMemberOfGroups() { + $cacheKey = 'getMemberOf'.$this->getUsername(); + if($this->connection->isCached($cacheKey)) { + return $this->connection->getFromCache($cacheKey); + } + $groupDNs = $this->access->readAttribute($this->getDN(), 'memberOf'); + $this->connection->writeToCache($cacheKey, $groupDNs); + return $groupDNs; + } + /** * @brief reads the image from LDAP that shall be used as Avatar * @return string data (provided by LDAP) | false @@ -189,7 +311,7 @@ class User { * @brief marks the time when user features like email have been updated * @return null */ - private function markRefreshTime() { + public function markRefreshTime() { $this->config->setUserValue( $this->uid, 'user_ldap', self::USER_PREFKEY_LASTREFRESH, time()); } @@ -252,48 +374,52 @@ class User { } /** - * @brief fetches the email from LDAP and stores it as ownCloud user value + * fetches the email from LDAP and stores it as ownCloud user value + * @param string $valueFromLDAP if known, to save an LDAP read request * @return null */ - public function updateEmail() { + public function updateEmail($valueFromLDAP = null) { if($this->wasRefreshed('email')) { return; } - - $email = null; - $emailAttribute = $this->connection->ldapEmailAttribute; - if(!empty($emailAttribute)) { - $aEmail = $this->access->readAttribute($this->dn, $emailAttribute); - if($aEmail && (count($aEmail) > 0)) { - $email = $aEmail[0]; - } - if(!is_null($email)) { - $this->config->setUserValue( - $this->uid, 'settings', 'email', $email); + $email = $valueFromLDAP; + if(is_null($valueFromLDAP)) { + $emailAttribute = $this->connection->ldapEmailAttribute; + if(!empty($emailAttribute)) { + $aEmail = $this->access->readAttribute($this->dn, $emailAttribute); + if(is_array($aEmail) && (count($aEmail) > 0)) { + $email = $aEmail[0]; + } } } + if(!is_null($email)) { + $this->config->setUserValue( + $this->uid, 'settings', 'email', $email); + } } /** - * @brief fetches the quota from LDAP and stores it as ownCloud user value + * fetches the quota from LDAP and stores it as ownCloud user value + * @param string $valueFromLDAP the quota attribute's value can be passed, + * to save the readAttribute request * @return null */ - public function updateQuota() { + public function updateQuota($valueFromLDAP = null) { if($this->wasRefreshed('quota')) { return; } - - $quota = null; + //can be null $quotaDefault = $this->connection->ldapQuotaDefault; - $quotaAttribute = $this->connection->ldapQuotaAttribute; - if(!empty($quotaDefault)) { - $quota = $quotaDefault; - } - if(!empty($quotaAttribute)) { - $aQuota = $this->access->readAttribute($this->dn, $quotaAttribute); - - if($aQuota && (count($aQuota) > 0)) { - $quota = $aQuota[0]; + $quota = !is_null($valueFromLDAP) + ? $valueFromLDAP + : $quotaDefault !== '' ? $quotaDefault : null; + if(is_null($valueFromLDAP)) { + $quotaAttribute = $this->connection->ldapQuotaAttribute; + if(!empty($quotaAttribute)) { + $aQuota = $this->access->readAttribute($this->dn, $quotaAttribute); + if($aQuota && (count($aQuota) > 0)) { + $quota = $aQuota[0]; + } } } if(!is_null($quota)) { diff --git a/apps/user_ldap/tests/access.php b/apps/user_ldap/tests/access.php index c4cd203646..5bf1a65bd5 100644 --- a/apps/user_ldap/tests/access.php +++ b/apps/user_ldap/tests/access.php @@ -29,7 +29,7 @@ use \OCA\user_ldap\lib\Connection; use \OCA\user_ldap\lib\ILDAPWrapper; class Test_Access extends \Test\TestCase { - private function getConnecterAndLdapMock() { + private function getConnectorAndLdapMock() { static $conMethods; static $accMethods; static $umMethods; @@ -56,7 +56,7 @@ class Test_Access extends \Test\TestCase { } public function testEscapeFilterPartValidChars() { - list($lw, $con, $um) = $this->getConnecterAndLdapMock(); + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); $access = new Access($con, $lw, $um); $input = 'okay'; @@ -64,7 +64,7 @@ class Test_Access extends \Test\TestCase { } public function testEscapeFilterPartEscapeWildcard() { - list($lw, $con, $um) = $this->getConnecterAndLdapMock(); + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); $access = new Access($con, $lw, $um); $input = '*'; @@ -73,7 +73,7 @@ class Test_Access extends \Test\TestCase { } public function testEscapeFilterPartEscapeWildcard2() { - list($lw, $con, $um) = $this->getConnecterAndLdapMock(); + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); $access = new Access($con, $lw, $um); $input = 'foo*bar'; @@ -83,7 +83,7 @@ class Test_Access extends \Test\TestCase { /** @dataProvider convertSID2StrSuccessData */ public function testConvertSID2StrSuccess(array $sidArray, $sidExpected) { - list($lw, $con, $um) = $this->getConnecterAndLdapMock(); + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); $access = new Access($con, $lw, $um); $sidBinary = implode('', $sidArray); @@ -118,7 +118,7 @@ class Test_Access extends \Test\TestCase { } public function testConvertSID2StrInputError() { - list($lw, $con, $um) = $this->getConnecterAndLdapMock(); + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); $access = new Access($con, $lw, $um); $sidIllegal = 'foobar'; @@ -128,7 +128,7 @@ class Test_Access extends \Test\TestCase { } public function testGetDomainDNFromDNSuccess() { - list($lw, $con, $um) = $this->getConnecterAndLdapMock(); + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); $access = new Access($con, $lw, $um); $inputDN = 'uid=zaphod,cn=foobar,dc=my,dc=server,dc=com'; @@ -143,7 +143,7 @@ class Test_Access extends \Test\TestCase { } public function testGetDomainDNFromDNError() { - list($lw, $con, $um) = $this->getConnecterAndLdapMock(); + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); $access = new Access($con, $lw, $um); $inputDN = 'foobar'; @@ -178,7 +178,7 @@ class Test_Access extends \Test\TestCase { } public function testStringResemblesDN() { - list($lw, $con, $um) = $this->getConnecterAndLdapMock(); + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); $access = new Access($con, $lw, $um); $cases = $this->getResemblesDNInputData(); @@ -199,7 +199,7 @@ class Test_Access extends \Test\TestCase { } public function testStringResemblesDNLDAPmod() { - list($lw, $con, $um) = $this->getConnecterAndLdapMock(); + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); $lw = new \OCA\user_ldap\lib\LDAP(); $access = new Access($con, $lw, $um); @@ -213,4 +213,51 @@ class Test_Access extends \Test\TestCase { $this->assertSame($case['expectedResult'], $access->stringResemblesDN($case['input'])); } } + + public function testCacheUserHome() { + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); + $access = new Access($con, $lw, $um); + + $con->expects($this->once()) + ->method('writeToCache'); + + $access->cacheUserHome('foobar', '/foobars/path'); + } + + public function testBatchApplyUserAttributes() { + list($lw, $con, $um) = $this->getConnectorAndLdapMock(); + $access = new Access($con, $lw, $um); + $mapperMock = $this->getMockBuilder('\OCA\User_LDAP\Mapping\UserMapping') + ->disableOriginalConstructor() + ->getMock(); + $userMock = $this->getMockBuilder('\OCA\user_ldap\lib\user\User') + ->disableOriginalConstructor() + ->getMock(); + + $access->setUserMapper($mapperMock); + + $data = array( + array( + 'dn' => 'foobar', + $con->ldapUserDisplayName => 'barfoo' + ), + array( + 'dn' => 'foo', + $con->ldapUserDisplayName => 'bar' + ), + array( + 'dn' => 'raboof', + $con->ldapUserDisplayName => 'oofrab' + ) + ); + + $userMock->expects($this->exactly(count($data))) + ->method('processAttributes'); + + $um->expects($this->exactly(count($data))) + ->method('get') + ->will($this->returnValue($userMock)); + + $access->batchApplyUserAttributes($data); + } } diff --git a/apps/user_ldap/tests/group_ldap.php b/apps/user_ldap/tests/group_ldap.php index 805238e7d3..7007afd516 100644 --- a/apps/user_ldap/tests/group_ldap.php +++ b/apps/user_ldap/tests/group_ldap.php @@ -53,6 +53,10 @@ class Test_Group_Ldap extends \Test\TestCase { $accMethods, array($connector, $lw, $um)); + $access->expects($this->any()) + ->method('getConnection') + ->will($this->returnValue($connector)); + return $access; } @@ -391,7 +395,7 @@ class Test_Group_Ldap extends \Test\TestCase { $access->connection->hasPrimaryGroups = false; - $access->expects($this->once()) + $access->expects($this->any()) ->method('username2dn') ->will($this->returnValue($dn)); diff --git a/apps/user_ldap/tests/user/manager.php b/apps/user_ldap/tests/user/manager.php index d659323fc7..98e48638d8 100644 --- a/apps/user_ldap/tests/user/manager.php +++ b/apps/user_ldap/tests/user/manager.php @@ -37,6 +37,16 @@ class Test_User_Manager extends \Test\TestCase { $image = $this->getMock('\OCP\Image'); $dbc = $this->getMock('\OCP\IDBConnection'); + $connection = new \OCA\user_ldap\lib\Connection( + $lw = $this->getMock('\OCA\user_ldap\lib\ILDAPWrapper'), + '', + null + ); + + $access->expects($this->any()) + ->method('getConnection') + ->will($this->returnValue($connection)); + return array($access, $config, $filesys, $image, $log, $avaMgr, $dbc); } @@ -206,4 +216,36 @@ class Test_User_Manager extends \Test\TestCase { $this->assertNull($user); } + public function testGetAttributesAll() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc) = + $this->getTestInstances(); + + $manager = new Manager($config, $filesys, $log, $avaMgr, $image, $dbc); + $manager->setLdapAccess($access); + + $connection = $access->getConnection(); + $connection->setConfiguration(array('ldapEmailAttribute' => 'mail')); + + $attributes = $manager->getAttributes(); + + $this->assertTrue(in_array('dn', $attributes)); + $this->assertTrue(in_array($access->getConnection()->ldapEmailAttribute, $attributes)); + $this->assertTrue(in_array('jpegphoto', $attributes)); + $this->assertTrue(in_array('thumbnailphoto', $attributes)); + } + + public function testGetAttributesMinimal() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc) = + $this->getTestInstances(); + + $manager = new Manager($config, $filesys, $log, $avaMgr, $image, $dbc); + $manager->setLdapAccess($access); + + $attributes = $manager->getAttributes(true); + + $this->assertTrue(in_array('dn', $attributes)); + $this->assertTrue(!in_array('jpegphoto', $attributes)); + $this->assertTrue(!in_array('thumbnailphoto', $attributes)); + } + } diff --git a/apps/user_ldap/tests/user/user.php b/apps/user_ldap/tests/user/user.php index 80baafcb25..c074b2ceb7 100644 --- a/apps/user_ldap/tests/user/user.php +++ b/apps/user_ldap/tests/user/user.php @@ -221,7 +221,7 @@ class Test_User_User extends \Test\TestCase { $connection->expects($this->at(0)) ->method('__get') ->with($this->equalTo('ldapQuotaDefault')) - ->will($this->returnValue('23 GB')); + ->will($this->returnValue('25 GB')); $connection->expects($this->at(1)) ->method('__get') @@ -242,7 +242,7 @@ class Test_User_User extends \Test\TestCase { ->with($this->equalTo('alice'), $this->equalTo('files'), $this->equalTo('quota'), - $this->equalTo('23 GB')) + $this->equalTo('25 GB')) ->will($this->returnValue(true)); $uid = 'alice'; @@ -278,14 +278,14 @@ class Test_User_User extends \Test\TestCase { ->method('readAttribute') ->with($this->equalTo('uid=alice,dc=foo,dc=bar'), $this->equalTo('myquota')) - ->will($this->returnValue(array('23 GB'))); + ->will($this->returnValue(array('27 GB'))); $config->expects($this->once()) ->method('setUserValue') ->with($this->equalTo('alice'), $this->equalTo('files'), $this->equalTo('quota'), - $this->equalTo('23 GB')) + $this->equalTo('27 GB')) ->will($this->returnValue(true)); $uid = 'alice'; @@ -679,4 +679,61 @@ class Test_User_User extends \Test\TestCase { //photo is returned $photo = $user->getAvatarImage(); } + + public function testProcessAttributes() { + list(, $config, $filesys, $image, $log, $avaMgr, $dbc) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $uid = 'alice'; + $dn = 'uid=alice'; + + $requiredMethods = array( + 'markRefreshTime', + 'updateQuota', + 'updateEmail', + 'storeDisplayName', + 'storeLDAPUserName', + 'getHomePath', + 'updateAvatar' + ); + + $userMock = $this->getMockBuilder('OCA\user_ldap\lib\user\User') + ->setConstructorArgs(array($uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr)) + ->setMethods($requiredMethods) + ->getMock(); + + $connection->setConfiguration(array( + 'homeFolderNamingRule' => 'homeDirectory' + )); + + $connection->expects($this->any()) + ->method('__get') + //->will($this->returnArgument(0)); + ->will($this->returnCallback(function($name) { + if($name === 'homeFolderNamingRule') { + return 'attr:homeDirectory'; + } + return $name; + })); + + $record = array( + strtolower($connection->ldapQuotaAttribute) => array('4096'), + strtolower($connection->ldapEmailAttribute) => array('alice@wonderland.org'), + strtolower($connection->ldapUserDisplayName) => array('Aaaaalice'), + 'uid' => array($uid), + 'homedirectory' => array('Alice\'s Folder'), + 'memberof' => array('cn=groupOne', 'cn=groupTwo'), + 'jpegphoto' => array('here be an image') + ); + + foreach($requiredMethods as $method) { + $userMock->expects($this->once()) + ->method($method); + } + + $userMock->processAttributes($record); + } } diff --git a/apps/user_ldap/tests/user_ldap.php b/apps/user_ldap/tests/user_ldap.php index 2b99e1c2dc..69a76c0b7a 100644 --- a/apps/user_ldap/tests/user_ldap.php +++ b/apps/user_ldap/tests/user_ldap.php @@ -34,6 +34,7 @@ use \OCA\user_ldap\lib\ILDAPWrapper; class Test_User_Ldap_Direct extends \Test\TestCase { protected $backend; protected $access; + protected $configMock; protected function setUp() { parent::setUp(); @@ -61,8 +62,9 @@ class Test_User_Ldap_Direct extends \Test\TestCase { $conMethods, array($lw, null, null)); + $this->configMock = $this->getMock('\OCP\IConfig'); $um = new \OCA\user_ldap\lib\user\Manager( - $this->getMock('\OCP\IConfig'), + $this->configMock, $this->getMock('\OCA\user_ldap\lib\FilesystemHelper'), $this->getMock('\OCA\user_ldap\lib\LogWrapper'), $this->getMock('\OCP\IAvatarManager'), @@ -586,6 +588,13 @@ class Test_User_Ldap_Direct extends \Test\TestCase { $backend = new UserLDAP($access, $config); $this->prepareMockForUserExists($access); + $dataDir = \OC::$server->getConfig()->getSystemValue( + 'datadirectory', \OC::$SERVERROOT.'/data'); + + $this->configMock->expects($this->once()) + ->method('getSystemValue') + ->will($this->returnValue($dataDir)); + $access->connection->expects($this->any()) ->method('__get') ->will($this->returnCallback(function($name) { @@ -609,14 +618,9 @@ class Test_User_Ldap_Direct extends \Test\TestCase { return false; } })); - //datadir-relativ path - $datadir = '/my/data/dir'; - $config->expects($this->once()) - ->method('getSystemValue') - ->will($this->returnValue($datadir)); $result = $backend->getHome('ladyofshadows'); - $this->assertEquals($datadir.'/susannah/', $result); + $this->assertEquals($dataDir.'/susannah/', $result); } /** diff --git a/apps/user_ldap/user_ldap.php b/apps/user_ldap/user_ldap.php index 00cba71836..7304088ba9 100644 --- a/apps/user_ldap/user_ldap.php +++ b/apps/user_ldap/user_ldap.php @@ -98,9 +98,8 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn */ public function getLDAPUserByLoginName($loginName) { //find out dn of the user name - $attrs = array($this->access->connection->ldapUserDisplayName, 'dn', - 'uid', 'samaccountname'); - $users = $this->access->fetchUsersByLoginName($loginName, $attrs); + $attrs = $this->access->userManager->getAttributes(); + $users = $this->access->fetchUsersByLoginName($loginName, $attrs, 1); if(count($users) < 1) { throw new \Exception('No user available for the given login name.'); } @@ -137,16 +136,9 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn return false; } + $this->access->cacheUserExists($user->getUsername()); + $user->processAttributes($ldapRecord); $user->markLogin(); - if(isset($ldapRecord[$this->access->connection->ldapUserDisplayName])) { - $dpn = $ldapRecord[$this->access->connection->ldapUserDisplayName]; - $user->storeDisplayName($dpn); - } - if(isset($ldapRecord['uid'])) { - $user->storeLDAPUserName($ldapRecord['uid']); - } else if(isset($ldapRecord['samaccountname'])) { - $user->storeLDAPUserName($ldapRecord['samaccountname']); - } return $user->getUsername(); } @@ -188,7 +180,7 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn //do the search and translate results to owncloud names $ldap_users = $this->access->fetchListOfUsers( $filter, - array($this->access->connection->ldapUserDisplayName, 'dn'), + $this->access->userManager->getAttributes(true), $limit, $offset); $ldap_users = $this->access->ownCloudUserNames($ldap_users); \OCP\Util::writeLog('user_ldap', 'getUsers: '.count($ldap_users). ' Users found', \OCP\Util::DEBUG); @@ -302,44 +294,12 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn if($this->access->connection->isCached($cacheKey)) { return $this->access->connection->getFromCache($cacheKey); } - if(strpos($this->access->connection->homeFolderNamingRule, 'attr:') === 0 && - $this->access->connection->homeFolderNamingRule !== 'attr:') { - $attr = substr($this->access->connection->homeFolderNamingRule, strlen('attr:')); - $homedir = $this->access->readAttribute( - $this->access->username2dn($uid), $attr); - if($homedir && isset($homedir[0])) { - $path = $homedir[0]; - //if attribute's value is an absolute path take this, otherwise append it to data dir - //check for / at the beginning or pattern c:\ resp. c:/ - if( - '/' === $path[0] - || (3 < strlen($path) && ctype_alpha($path[0]) - && $path[1] === ':' && ('\\' === $path[2] || '/' === $path[2])) - ) { - $homedir = $path; - } else { - $homedir = $this->ocConfig->getSystemValue('datadirectory', - \OC::$SERVERROOT.'/data' ) . '/' . $homedir[0]; - } - $this->access->connection->writeToCache($cacheKey, $homedir); - //we need it to store it in the DB as well in case a user gets - //deleted so we can clean up afterwards - $this->ocConfig->setUserValue( - $uid, 'user_ldap', 'homePath', $homedir - ); - //TODO: if home directory changes, the old one needs to be removed. - return $homedir; - } - if($this->ocConfig->getAppValue('user_ldap', 'enforce_home_folder_naming_rule', true)) { - // a naming rule attribute is defined, but it doesn't exist for that LDAP user - throw new \Exception('Home dir attribute can\'t be read from LDAP for uid: ' . $uid); - } - } - //false will apply default behaviour as defined and done by OC_User - $this->access->connection->writeToCache($cacheKey, false); - $this->ocConfig->setUserValue($uid, 'user_ldap', 'homePath', ''); - return false; + $user = $this->access->userManager->get($uid); + $path = $user->getHomePath(); + $this->access->cacheUserHome($uid, $path); + + return $path; } /** From 845485cfe637c3a7fdb72a110cb68c769b2f8d4b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 26 Aug 2015 15:17:46 +0200 Subject: [PATCH 2/3] adjust to nested group fix --- apps/user_ldap/group_ldap.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index a5fc59d3b0..d2e69b4e12 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -31,6 +31,7 @@ namespace OCA\user_ldap; use OCA\user_ldap\lib\Access; use OCA\user_ldap\lib\BackendUtility; +use OCA\user_ldap\lib\user\User; class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { protected $enabled = false; @@ -195,7 +196,11 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { return array(); } $seen[$DN] = 1; - $groups = $this->access->readAttribute($DN, 'memberOf'); + $user = $this->access->userManager->get($DN); + if(!$user instanceof User) { + return array(); + } + $groups = $user->getMemberOfGroups(); if (!is_array($groups)) { return array(); } From 002b6bf059377853dc174c1d49792e62983c4bec Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 23 Sep 2015 16:52:48 +0200 Subject: [PATCH 3/3] do not throw exception when no attribute is specified --- apps/user_ldap/lib/user/user.php | 5 +- apps/user_ldap/tests/user/user.php | 103 +++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/apps/user_ldap/lib/user/user.php b/apps/user_ldap/lib/user/user.php index 3bfe0c2707..6498cdf913 100644 --- a/apps/user_ldap/lib/user/user.php +++ b/apps/user_ldap/lib/user/user.php @@ -225,6 +225,7 @@ class User { */ public function getHomePath($valueFromLDAP = null) { $path = $valueFromLDAP; + $attr = null; if( is_null($path) && strpos($this->access->connection->homeFolderNamingRule, 'attr:') === 0 @@ -256,7 +257,9 @@ class User { return $path; } - if($this->config->getAppValue('user_ldap', 'enforce_home_folder_naming_rule', true)) { + if( !is_null($attr) + && $this->config->getAppValue('user_ldap', 'enforce_home_folder_naming_rule', true) + ) { // a naming rule attribute is defined, but it doesn't exist for that LDAP user throw new \Exception('Home dir attribute can\'t be read from LDAP for uid: ' . $this->getUsername()); } diff --git a/apps/user_ldap/tests/user/user.php b/apps/user_ldap/tests/user/user.php index c074b2ceb7..1c41eb71ec 100644 --- a/apps/user_ldap/tests/user/user.php +++ b/apps/user_ldap/tests/user/user.php @@ -736,4 +736,107 @@ class Test_User_User extends \Test\TestCase { $userMock->processAttributes($record); } + + public function emptyHomeFolderAttributeValueProvider() { + return array( + 'empty' => array(''), + 'prefixOnly' => array('attr:'), + ); + } + + /** + * @dataProvider emptyHomeFolderAttributeValueProvider + */ + public function testGetHomePathNotConfigured($attributeValue) { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $connection->expects($this->any()) + ->method('__get') + ->with($this->equalTo('homeFolderNamingRule')) + ->will($this->returnValue($attributeValue)); + + $access->expects($this->never()) + ->method('readAttribute'); + + $config->expects($this->never()) + ->method('getAppValue'); + + $uid = 'alice'; + $dn = 'uid=alice,dc=foo,dc=bar'; + + $user = new User( + $uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr); + + $path = $user->getHomePath(); + $this->assertSame($path, false); + } + + public function testGetHomePathConfiguredNotAvailableAllowed() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $connection->expects($this->any()) + ->method('__get') + ->with($this->equalTo('homeFolderNamingRule')) + ->will($this->returnValue('attr:foobar')); + + $access->expects($this->once()) + ->method('readAttribute') + ->will($this->returnValue(false)); + + // asks for "enforce_home_folder_naming_rule" + $config->expects($this->once()) + ->method('getAppValue') + ->will($this->returnValue(false)); + + $uid = 'alice'; + $dn = 'uid=alice,dc=foo,dc=bar'; + + $user = new User( + $uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr); + + $path = $user->getHomePath(); + + $this->assertSame($path, false); + } + + /** + * @expectedException \Exception + */ + public function testGetHomePathConfiguredNotAvailableNotAllowed() { + list($access, $config, $filesys, $image, $log, $avaMgr, $dbc) = + $this->getTestInstances(); + + list($access, $connection) = + $this->getAdvancedMocks($config, $filesys, $log, $avaMgr, $dbc); + + $connection->expects($this->any()) + ->method('__get') + ->with($this->equalTo('homeFolderNamingRule')) + ->will($this->returnValue('attr:foobar')); + + $access->expects($this->once()) + ->method('readAttribute') + ->will($this->returnValue(false)); + + // asks for "enforce_home_folder_naming_rule" + $config->expects($this->once()) + ->method('getAppValue') + ->will($this->returnValue(true)); + + $uid = 'alice'; + $dn = 'uid=alice,dc=foo,dc=bar'; + + $user = new User( + $uid, $dn, $access, $config, $filesys, $image, $log, $avaMgr); + + $user->getHomePath(); + } }