From e76211fdad3ac23a6fb32919b64d47382e684fd4 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 17 Sep 2021 19:15:46 +0200 Subject: [PATCH 1/2] ensure that user and group IDs in LDAP's tables are also max 64chars - limitation by core tables (e.g. sharing), IDs are always 64chars - when longer group IDs were requested they are hashed (does not affect displaynames) Signed-off-by: Arthur Schiwon --- apps/user_ldap/appinfo/info.xml | 2 +- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + apps/user_ldap/lib/Access.php | 25 +++- .../Version1010Date20200630192842.php | 4 +- .../Version1120Date20210917155206.php | 133 ++++++++++++++++++ apps/user_ldap/tests/AccessTest.php | 31 +++- 7 files changed, 192 insertions(+), 5 deletions(-) create mode 100644 apps/user_ldap/lib/Migration/Version1120Date20210917155206.php diff --git a/apps/user_ldap/appinfo/info.xml b/apps/user_ldap/appinfo/info.xml index 145fb871a8c23..ac62d39c2ad1f 100644 --- a/apps/user_ldap/appinfo/info.xml +++ b/apps/user_ldap/appinfo/info.xml @@ -9,7 +9,7 @@ A user logs into Nextcloud with their LDAP or AD credentials, and is granted access based on an authentication request handled by the LDAP or AD server. Nextcloud does not store LDAP or AD passwords, rather these credentials are used to authenticate a user and then Nextcloud uses a session for the user ID. More information is available in the LDAP User and Group Backend documentation. - 1.10.2 + 1.10.3 agpl Dominik Schmidt Arthur Schiwon diff --git a/apps/user_ldap/composer/composer/autoload_classmap.php b/apps/user_ldap/composer/composer/autoload_classmap.php index c8969223267d7..bd13a97877f38 100644 --- a/apps/user_ldap/composer/composer/autoload_classmap.php +++ b/apps/user_ldap/composer/composer/autoload_classmap.php @@ -58,6 +58,7 @@ 'OCA\\User_LDAP\\Migration\\UUIDFixInsert' => $baseDir . '/../lib/Migration/UUIDFixInsert.php', 'OCA\\User_LDAP\\Migration\\UUIDFixUser' => $baseDir . '/../lib/Migration/UUIDFixUser.php', 'OCA\\User_LDAP\\Migration\\Version1010Date20200630192842' => $baseDir . '/../lib/Migration/Version1010Date20200630192842.php', + 'OCA\\User_LDAP\\Migration\\Version1120Date20210917155206' => $baseDir . '/../lib/Migration/Version1120Date20210917155206.php', 'OCA\\User_LDAP\\Notification\\Notifier' => $baseDir . '/../lib/Notification/Notifier.php', 'OCA\\User_LDAP\\PagedResults\\IAdapter' => $baseDir . '/../lib/PagedResults/IAdapter.php', 'OCA\\User_LDAP\\PagedResults\\Php54' => $baseDir . '/../lib/PagedResults/Php54.php', diff --git a/apps/user_ldap/composer/composer/autoload_static.php b/apps/user_ldap/composer/composer/autoload_static.php index e9ba3c6534130..ef40b451fe848 100644 --- a/apps/user_ldap/composer/composer/autoload_static.php +++ b/apps/user_ldap/composer/composer/autoload_static.php @@ -73,6 +73,7 @@ class ComposerStaticInitUser_LDAP 'OCA\\User_LDAP\\Migration\\UUIDFixInsert' => __DIR__ . '/..' . '/../lib/Migration/UUIDFixInsert.php', 'OCA\\User_LDAP\\Migration\\UUIDFixUser' => __DIR__ . '/..' . '/../lib/Migration/UUIDFixUser.php', 'OCA\\User_LDAP\\Migration\\Version1010Date20200630192842' => __DIR__ . '/..' . '/../lib/Migration/Version1010Date20200630192842.php', + 'OCA\\User_LDAP\\Migration\\Version1120Date20210917155206' => __DIR__ . '/..' . '/../lib/Migration/Version1120Date20210917155206.php', 'OCA\\User_LDAP\\Notification\\Notifier' => __DIR__ . '/..' . '/../lib/Notification/Notifier.php', 'OCA\\User_LDAP\\PagedResults\\IAdapter' => __DIR__ . '/..' . '/../lib/PagedResults/IAdapter.php', 'OCA\\User_LDAP\\PagedResults\\Php54' => __DIR__ . '/..' . '/../lib/PagedResults/Php54.php', diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php index 10e4df30626b4..222443cfa8e5b 100644 --- a/apps/user_ldap/lib/Access.php +++ b/apps/user_ldap/lib/Access.php @@ -58,6 +58,8 @@ use OCP\IConfig; use OCP\ILogger; use OCP\IUserManager; +use function strlen; +use function substr; /** * Class Access @@ -582,7 +584,7 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped return false; } } else { - $intName = $ldapName; + $intName = $this->sanitizeGroupIDCandidate($ldapName); } //a new user/group! Add it only if it doesn't conflict with other backend's users or existing groups @@ -841,6 +843,11 @@ private function _createAltInternalOwnCloudNameForGroups($name) { * @return string|false with with the name to use in Nextcloud or false if unsuccessful */ private function createAltInternalOwnCloudName($name, $isUser) { + // ensure there is space for the "_1234" suffix + if (strlen($name) > 59) { + $name = substr($name, 0, 59); + } + $originalTTL = $this->connection->ldapCacheTTL; $this->connection->setConfiguration(['ldapCacheTTL' => 0]); if ($isUser) { @@ -1435,6 +1442,10 @@ public function sanitizeUsername($name) { // Every remaining disallowed characters will be removed $name = preg_replace('/[^a-zA-Z0-9_.@-]/u', '', $name); + if (strlen($name) > 64) { + $name = (string)hash('sha256', $name, false); + } + if ($name === '') { throw new \InvalidArgumentException('provided name template for username does not contain any allowed characters'); } @@ -1442,6 +1453,18 @@ public function sanitizeUsername($name) { return $name; } + public function sanitizeGroupIDCandidate(string $candidate): string { + $candidate = trim($candidate); + if (strlen($candidate) > 64) { + $candidate = (string)hash('sha256', $candidate, false); + } + if ($candidate === '') { + throw new \InvalidArgumentException('provided name template for username does not contain any allowed characters'); + } + + return $candidate; + } + /** * escapes (user provided) parts for LDAP filter * diff --git a/apps/user_ldap/lib/Migration/Version1010Date20200630192842.php b/apps/user_ldap/lib/Migration/Version1010Date20200630192842.php index f99372d51b911..484607052e115 100644 --- a/apps/user_ldap/lib/Migration/Version1010Date20200630192842.php +++ b/apps/user_ldap/lib/Migration/Version1010Date20200630192842.php @@ -52,7 +52,7 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt ]); $table->addColumn('owncloud_name', Types::STRING, [ 'notnull' => true, - 'length' => 255, + 'length' => 64, 'default' => '', ]); $table->addColumn('directory_uuid', Types::STRING, [ @@ -73,7 +73,7 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt ]); $table->addColumn('owncloud_name', Types::STRING, [ 'notnull' => true, - 'length' => 255, + 'length' => 64, 'default' => '', ]); $table->addColumn('directory_uuid', Types::STRING, [ diff --git a/apps/user_ldap/lib/Migration/Version1120Date20210917155206.php b/apps/user_ldap/lib/Migration/Version1120Date20210917155206.php new file mode 100644 index 0000000000000..d46107733dc5c --- /dev/null +++ b/apps/user_ldap/lib/Migration/Version1120Date20210917155206.php @@ -0,0 +1,133 @@ +dbc = $dbc; + $this->userManager = $userManager; + $this->logger = $logger; + } + + public function getName() { + return 'Adjust LDAP user and group id column lengths to match server lengths'; + } + + /** + * @param IOutput $output + * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param array $options + */ + public function preSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + // ensure that there is no user or group id longer than 64char in LDAP table + $this->handleIDs('ldap_group_mapping', false); + $this->handleIDs('ldap_user_mapping', true); + } + + /** + * @param IOutput $output + * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param array $options + * @return null|ISchemaWrapper + */ + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + $changeSchema = false; + foreach (['ldap_user_mapping', 'ldap_group_mapping'] as $tableName) { + $table = $schema->getTable($tableName); + $column = $table->getColumn('owncloud_name'); + if ($column->getLength() > 64) { + $column->setLength(64); + $changeSchema = true; + } + } + + return $changeSchema ? $schema : null; + } + + protected function handleIDs(string $table, bool $emitHooks) { + $q = $this->getSelectQuery($table); + $u = $this->getUpdateQuery($table); + + $r = $q->executeQuery(); + while ($row = $r->fetch()) { + $newId = hash('sha256', $row['owncloud_name'], false); + if ($emitHooks) { + $this->emitUnassign($row['owncloud_name'], true); + } + $u->setParameter('uuid', $row['directory_uuid']); + $u->setParameter('newId', $newId); + try { + $u->executeStatement(); + if ($emitHooks) { + $this->emitUnassign($row['owncloud_name'], false); + $this->emitAssign($newId); + } + } catch (Exception $e) { + $this->logger->error('Failed to shorten owncloud_name "{oldId}" to "{newId}" (UUID: "{uuid}" of {table})', + [ + 'app' => 'user_ldap', + 'oldId' => $row['owncloud_name'], + 'newId' => $newId, + 'uuid' => $row['directory_uuid'], + 'table' => $table, + 'exception' => $e, + ] + ); + } + } + $r->closeCursor(); + } + + protected function getSelectQuery(string $table): IQueryBuilder { + $q = $this->dbc->getQueryBuilder(); + $q->select('owncloud_name', 'directory_uuid') + ->from($table) + ->where($q->expr()->like('owncloud_name', $q->createNamedParameter(str_repeat('_', 65) . '%'), Types::STRING)); + return $q; + } + + protected function getUpdateQuery(string $table): IQueryBuilder { + $q = $this->dbc->getQueryBuilder(); + $q->update($table) + ->set('owncloud_name', $q->createParameter('newId')) + ->where($q->expr()->eq('directory_uuid', $q->createParameter('uuid'))); + return $q; + } + + protected function emitUnassign(string $oldId, bool $pre): void { + if ($this->userManager instanceof PublicEmitter) { + $this->userManager->emit('\OC\User', $pre ? 'pre' : 'post' . 'UnassignedUserId', [$oldId]); + } + } + + protected function emitAssign(string $newId): void { + if ($this->userManager instanceof PublicEmitter) { + $this->userManager->emit('\OC\User', 'assignedUserId', [$newId]); + } + } +} diff --git a/apps/user_ldap/tests/AccessTest.php b/apps/user_ldap/tests/AccessTest.php index f5f6e162f3c67..6fe0fb1f24f0d 100644 --- a/apps/user_ldap/tests/AccessTest.php +++ b/apps/user_ldap/tests/AccessTest.php @@ -694,7 +694,28 @@ public function intUsernameProvider() { ['epost@poste.test', 'epost@poste.test'], ['frรคnk', $translitExpected], [' gerda ', 'gerda'], - ['๐Ÿ•ฑ๐Ÿต๐Ÿ˜๐Ÿ‘', null] + ['๐Ÿ•ฑ๐Ÿต๐Ÿ˜๐Ÿ‘', null], + [ + 'OneNameToRuleThemAllOneNameToFindThemOneNameToBringThemAllAndInTheDarknessBindThem', + '81ff71b5dd0f0092e2dc977b194089120093746e273f2ef88c11003762783127' + ] + ]; + } + + public function groupIDCandidateProvider() { + return [ + ['alice', 'alice'], + ['b/ob', 'b/ob'], + ['charly๐Ÿฌ', 'charly๐Ÿฌ'], + ['debo rah', 'debo rah'], + ['epost@poste.test', 'epost@poste.test'], + ['frรคnk', 'frรคnk'], + [' gerda ', 'gerda'], + ['๐Ÿ•ฑ๐Ÿต๐Ÿ˜๐Ÿ‘', '๐Ÿ•ฑ๐Ÿต๐Ÿ˜๐Ÿ‘'], + [ + 'OneNameToRuleThemAllOneNameToFindThemOneNameToBringThemAllAndInTheDarknessBindThem', + '81ff71b5dd0f0092e2dc977b194089120093746e273f2ef88c11003762783127' + ] ]; } @@ -712,6 +733,14 @@ public function testSanitizeUsername($name, $expected) { $this->assertSame($expected, $sanitizedName); } + /** + * @dataProvider groupIDCandidateProvider + */ + public function testSanitizeGroupIDCandidate(string $name, string $expected) { + $sanitizedName = $this->access->sanitizeGroupIDCandidate($name); + $this->assertSame($expected, $sanitizedName); + } + public function testUserStateUpdate() { $this->connection->expects($this->any()) ->method('__get') From 031c89ae5297dc3b21f8ce4d0c06c6fa60a8f571 Mon Sep 17 00:00:00 2001 From: blizzz Date: Tue, 5 Oct 2021 11:48:56 +0200 Subject: [PATCH 2/2] adjust to old db interfaces Co-authored-by: Daniel Signed-off-by: Arthur Schiwon --- .../lib/Migration/Version1120Date20210917155206.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/apps/user_ldap/lib/Migration/Version1120Date20210917155206.php b/apps/user_ldap/lib/Migration/Version1120Date20210917155206.php index d46107733dc5c..e8d9e0f7a7bd3 100644 --- a/apps/user_ldap/lib/Migration/Version1120Date20210917155206.php +++ b/apps/user_ldap/lib/Migration/Version1120Date20210917155206.php @@ -5,11 +5,11 @@ namespace OCA\User_LDAP\Migration; use Closure; +use Doctrine\DBAL\DBALException; +use Doctrine\DBAL\Types\Types; use OC\Hooks\PublicEmitter; -use OCP\DB\Exception; use OCP\DB\ISchemaWrapper; use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\DB\Types; use OCP\IDBConnection; use OCP\IUserManager; use OCP\Migration\IOutput; @@ -73,7 +73,7 @@ protected function handleIDs(string $table, bool $emitHooks) { $q = $this->getSelectQuery($table); $u = $this->getUpdateQuery($table); - $r = $q->executeQuery(); + $r = $q->execute(); while ($row = $r->fetch()) { $newId = hash('sha256', $row['owncloud_name'], false); if ($emitHooks) { @@ -82,12 +82,12 @@ protected function handleIDs(string $table, bool $emitHooks) { $u->setParameter('uuid', $row['directory_uuid']); $u->setParameter('newId', $newId); try { - $u->executeStatement(); + $u->execute(); if ($emitHooks) { $this->emitUnassign($row['owncloud_name'], false); $this->emitAssign($newId); } - } catch (Exception $e) { + } catch (DBALException $e) { $this->logger->error('Failed to shorten owncloud_name "{oldId}" to "{newId}" (UUID: "{uuid}" of {table})', [ 'app' => 'user_ldap',