From 991190b9946442c685d5d58530fe602e008f9a4b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 7 Dec 2017 17:49:33 +0100 Subject: [PATCH 1/2] ensure that users are cached when they are retrieved Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Access.php | 17 ++-- apps/user_ldap/lib/Helper.php | 2 +- apps/user_ldap/tests/AccessTest.php | 116 ++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 7 deletions(-) diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php index 07583798e4639..d1c738719b120 100644 --- a/apps/user_ldap/lib/Access.php +++ b/apps/user_ldap/lib/Access.php @@ -830,6 +830,7 @@ public function fetchListOfUsers($filter, $attr, $limit = null, $offset = null, $recordsToUpdate = array_filter($ldapRecords, function($record) use ($isBackgroundJobModeAjax) { $newlyMapped = false; $uid = $this->dn2ocname($record['dn'][0], null, true, $newlyMapped, $record); + $this->cacheUserExists($uid); return ($uid !== false) && ($newlyMapped || $isBackgroundJobModeAjax); }); } @@ -854,7 +855,6 @@ public function batchApplyUserAttributes(array $ldapRecords){ if($ocName === false) { continue; } - $this->cacheUserExists($ocName); $user = $this->userManager->get($ocName); if($user instanceof OfflineUser) { $user->unmark(); @@ -1019,11 +1019,15 @@ private function invokeLDAPMethod() { /** * retrieved. Results will according to the order in the array. + * + * @param $filter + * @param $base + * @param null $attr * @param int $limit optional, maximum results to be counted * @param int $offset optional, a starting point * @return array|false array with the search result as first value and pagedSearchOK as * second | false if not successful - * @throws \OC\ServerNotAvailableException + * @throws ServerNotAvailableException */ private function executeSearch($filter, $base, &$attr = null, $limit = null, $offset = null) { if(!is_null($attr) && !is_array($attr)) { @@ -1169,6 +1173,7 @@ private function countEntriesInSearchResults($searchResults) { /** * Executes an LDAP search + * * @param string $filter the LDAP filter for the search * @param array $base an array containing the LDAP subtree(s) that shall be searched * @param string|string[] $attr optional, array, one or more attributes that shall be @@ -1176,6 +1181,7 @@ private function countEntriesInSearchResults($searchResults) { * @param int $offset * @param bool $skipHandling * @return array with the search result + * @throws ServerNotAvailableException */ public function search($filter, $base, $attr = null, $limit = null, $offset = null, $skipHandling = false) { $limitPerPage = intval($this->connection->ldapPagingSize); @@ -1189,12 +1195,12 @@ public function search($filter, $base, $attr = null, $limit = null, $offset = nu * loops through until we get $continue equals true and * $findings['count'] < $limit */ - $findings = array(); + $findings = []; $savedoffset = $offset; do { $search = $this->executeSearch($filter, $base, $attr, $limitPerPage, $offset); if($search === false) { - return array(); + return []; } list($sr, $pagedSearchOK) = $search; $cr = $this->connection->getConnectionResource(); @@ -1231,7 +1237,7 @@ public function search($filter, $base, $attr = null, $limit = null, $offset = nu } if(!is_null($attr)) { - $selection = array(); + $selection = []; $i = 0; foreach($findings as $item) { if(!is_array($item)) { @@ -1239,7 +1245,6 @@ public function search($filter, $base, $attr = null, $limit = null, $offset = nu } $item = \OCP\Util::mb_array_change_key_case($item, MB_CASE_LOWER, 'UTF-8'); foreach($attr as $key) { - $key = mb_strtolower($key, 'UTF-8'); if(isset($item[$key])) { if(is_array($item[$key]) && isset($item[$key]['count'])) { unset($item[$key]['count']); diff --git a/apps/user_ldap/lib/Helper.php b/apps/user_ldap/lib/Helper.php index a433ea8e4a78a..3157a7ab09d88 100644 --- a/apps/user_ldap/lib/Helper.php +++ b/apps/user_ldap/lib/Helper.php @@ -229,7 +229,7 @@ public function setLDAPProvider() { /** * sanitizes a DN received from the LDAP server * @param array $dn the DN in question - * @return array the sanitized DN + * @return array|string the sanitized DN */ public function sanitizeDN($dn) { //treating multiple base DNs diff --git a/apps/user_ldap/tests/AccessTest.php b/apps/user_ldap/tests/AccessTest.php index 6c250ff320f3f..d0693fe50e15a 100644 --- a/apps/user_ldap/tests/AccessTest.php +++ b/apps/user_ldap/tests/AccessTest.php @@ -60,6 +60,8 @@ * @package OCA\User_LDAP\Tests */ class AccessTest extends TestCase { + /** @var UserMapping|\PHPUnit_Framework_MockObject_MockObject */ + protected $userMapper; /** @var Connection|\PHPUnit_Framework_MockObject_MockObject */ private $connection; /** @var LDAP|\PHPUnit_Framework_MockObject_MockObject */ @@ -79,6 +81,7 @@ public function setUp() { $this->userManager = $this->createMock(Manager::class); $this->helper = $this->createMock(Helper::class); $this->config = $this->createMock(IConfig::class); + $this->userMapper = $this->createMock(UserMapping::class); $this->access = new Access( $this->connection, @@ -87,6 +90,7 @@ public function setUp() { $this->helper, $this->config ); + $this->access->setUserMapper($this->userMapper); } private function getConnectorAndLdapMock() { @@ -217,6 +221,7 @@ public function dnInputDataProvider() { /** * @dataProvider dnInputDataProvider + * @param array $case */ public function testStringResemblesDN($case) { list($lw, $con, $um, $helper) = $this->getConnectorAndLdapMock(); @@ -440,6 +445,7 @@ public function testSetPasswordWithDisabledChanges() { ->method('__get') ->willReturn(false); + /** @noinspection PhpUnhandledExceptionInspection */ $this->access->setPassword('CN=foo', 'MyPassword'); } @@ -458,6 +464,7 @@ public function testSetPasswordWithLdapNotAvailable() { ->with($connection) ->willReturn(false); + /** @noinspection PhpUnhandledExceptionInspection */ $this->assertFalse($this->access->setPassword('CN=foo', 'MyPassword')); } @@ -485,6 +492,7 @@ public function testSetPasswordWithRejectedChange() { ->with($connection, 'CN=foo', 'MyPassword') ->willThrowException(new ConstraintViolationException()); + /** @noinspection PhpUnhandledExceptionInspection */ $this->access->setPassword('CN=foo', 'MyPassword'); } @@ -508,6 +516,114 @@ public function testSetPassword() { ->with($connection, 'CN=foo', 'MyPassword') ->willReturn(true); + /** @noinspection PhpUnhandledExceptionInspection */ $this->assertTrue($this->access->setPassword('CN=foo', 'MyPassword')); } + + protected function prepareMocksForSearchTests( + $base, + $fakeConnection, + $fakeSearchResultResource, + $fakeLdapEntries + ) { + $this->connection + ->expects($this->any()) + ->method('getConnectionResource') + ->willReturn($fakeConnection); + $this->connection->expects($this->any()) + ->method('__get') + ->willReturnCallback(function($key) use ($base) { + if(stripos($key, 'base') !== false) { + return $base; + } + return null; + }); + + $this->ldap + ->expects($this->any()) + ->method('isResource') + ->willReturnCallback(function ($resource) use ($fakeConnection) { + return $resource === $fakeConnection; + }); + $this->ldap + ->expects($this->any()) + ->method('errno') + ->willReturn(0); + $this->ldap + ->expects($this->once()) + ->method('search') + ->willReturn([$fakeSearchResultResource]); + $this->ldap + ->expects($this->exactly(count($base))) + ->method('getEntries') + ->willReturn($fakeLdapEntries); + + $this->helper->expects($this->any()) + ->method('sanitizeDN') + ->willReturnArgument(0); + } + + public function testSearchNoPagedSearch() { + // scenario: no pages search, 1 search base + $filter = 'objectClass=nextcloudUser'; + $base = ['ou=zombies,dc=foobar,dc=nextcloud,dc=com']; + + $fakeConnection = new \stdClass(); + $fakeSearchResultResource = new \stdClass(); + $fakeLdapEntries = [ + 'count' => 2, + [ + 'dn' => 'uid=sgarth,' . $base[0], + ], + [ + 'dn' => 'uid=wwilson,' . $base[0], + ] + ]; + + $expected = $fakeLdapEntries; + unset($expected['count']); + + $this->prepareMocksForSearchTests($base, $fakeConnection, $fakeSearchResultResource, $fakeLdapEntries); + + /** @noinspection PhpUnhandledExceptionInspection */ + $result = $this->access->search($filter, $base); + $this->assertSame($expected, $result); + } + + public function testFetchListOfUsers() { + $filter = 'objectClass=nextcloudUser'; + $base = ['ou=zombies,dc=foobar,dc=nextcloud,dc=com']; + $attrs = ['dn', 'uid']; + + $fakeConnection = new \stdClass(); + $fakeSearchResultResource = new \stdClass(); + $fakeLdapEntries = [ + 'count' => 2, + [ + 'dn' => 'uid=sgarth,' . $base[0], + 'uid' => [ 'sgarth' ], + ], + [ + 'dn' => 'uid=wwilson,' . $base[0], + 'uid' => [ 'wwilson' ], + ] + ]; + $expected = $fakeLdapEntries; + unset($expected['count']); + array_walk($expected, function(&$v) { + $v['dn'] = [$v['dn']]; // dn is translated into an array internally for consistency + }); + + $this->prepareMocksForSearchTests($base, $fakeConnection, $fakeSearchResultResource, $fakeLdapEntries); + + $this->connection->expects($this->exactly($fakeLdapEntries['count'])) + ->method('writeToCache') + ->with($this->stringStartsWith('userExists'), true); + + /** @noinspection PhpUnhandledExceptionInspection */ + $list = $this->access->fetchListOfUsers($filter, $attrs); + $this->assertSame($expected, $list); + } + + } From 27f14eee26af750e9e246668e761a9bf98b17fee Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 7 Dec 2017 22:02:54 +0100 Subject: [PATCH 2/2] don't cache user, if no internal user id was retrieved/assigned Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Access.php | 17 ++++++++++------- apps/user_ldap/tests/AccessTest.php | 7 +++++++ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php index d1c738719b120..95710cd37f2fb 100644 --- a/apps/user_ldap/lib/Access.php +++ b/apps/user_ldap/lib/Access.php @@ -527,6 +527,7 @@ public function dn2username($fdn, $ldapName = null) { * @param bool|null $newlyMapped * @param array|null $record * @return false|string with with the name to use in Nextcloud + * @throws \Exception */ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped = null, array $record = null) { $newlyMapped = false; @@ -586,7 +587,7 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped // outside of core user management will still cache the user as non-existing. $originalTTL = $this->connection->ldapCacheTTL; $this->connection->setConfiguration(array('ldapCacheTTL' => 0)); - if(($isUser && !\OCP\User::userExists($intName)) + if(($isUser && $intName !== '' && !\OCP\User::userExists($intName)) || (!$isUser && !\OC::$server->getGroupManager()->groupExists($intName))) { if($mapper->map($fdn, $intName, $uuid)) { $this->connection->setConfiguration(array('ldapCacheTTL' => $originalTTL)); @@ -830,7 +831,9 @@ public function fetchListOfUsers($filter, $attr, $limit = null, $offset = null, $recordsToUpdate = array_filter($ldapRecords, function($record) use ($isBackgroundJobModeAjax) { $newlyMapped = false; $uid = $this->dn2ocname($record['dn'][0], null, true, $newlyMapped, $record); - $this->cacheUserExists($uid); + if(is_string($uid)) { + $this->cacheUserExists($uid); + } return ($uid !== false) && ($newlyMapped || $isBackgroundJobModeAjax); }); } @@ -1022,7 +1025,7 @@ private function invokeLDAPMethod() { * * @param $filter * @param $base - * @param null $attr + * @param string[]|string|null $attr * @param int $limit optional, maximum results to be counted * @param int $offset optional, a starting point * @return array|false array with the search result as first value and pagedSearchOK as @@ -1106,6 +1109,7 @@ private function processPagedSearchStatus($sr, $filter, $base, $iFoundItems, $li /** * executes an LDAP search, but counts the results only + * * @param string $filter the LDAP filter for the search * @param array $base an array containing the LDAP subtree(s) that shall be searched * @param string|string[] $attr optional, array, one or more attributes that shall be @@ -1115,7 +1119,7 @@ private function processPagedSearchStatus($sr, $filter, $base, $iFoundItems, $li * @param bool $skipHandling indicates whether the pages search operation is * completed * @return int|false Integer or false if the search could not be initialized - * + * @throws ServerNotAvailableException */ private function count($filter, $base, $attr = null, $limit = null, $offset = null, $skipHandling = false) { \OCP\Util::writeLog('user_ldap', 'Count filter: '.print_r($filter, true), \OCP\Util::DEBUG); @@ -1130,8 +1134,7 @@ private function count($filter, $base, $attr = null, $limit = null, $offset = nu $this->connection->getConnectionResource(); do { - $search = $this->executeSearch($filter, $base, $attr, - $limitPerPage, $offset); + $search = $this->executeSearch($filter, $base, $attr, $limitPerPage, $offset); if($search === false) { return $counter > 0 ? $counter : false; } @@ -1286,7 +1289,7 @@ public function search($filter, $base, $attr = null, $limit = null, $offset = nu */ public function sanitizeUsername($name) { if($this->connection->ldapIgnoreNamingRules) { - return $name; + return trim($name); } // Transliteration diff --git a/apps/user_ldap/tests/AccessTest.php b/apps/user_ldap/tests/AccessTest.php index d0693fe50e15a..cbb695d779a0c 100644 --- a/apps/user_ldap/tests/AccessTest.php +++ b/apps/user_ldap/tests/AccessTest.php @@ -620,6 +620,13 @@ public function testFetchListOfUsers() { ->method('writeToCache') ->with($this->stringStartsWith('userExists'), true); + $this->userMapper->expects($this->exactly($fakeLdapEntries['count'])) + ->method('getNameByDN') + ->willReturnCallback(function($fdn) { + $parts = ldap_explode_dn($fdn, false); + return $parts[0]; + }); + /** @noinspection PhpUnhandledExceptionInspection */ $list = $this->access->fetchListOfUsers($filter, $attrs); $this->assertSame($expected, $list);