From 24f2974267684b364d0396db2acaa6e8dea1c1ad Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 17 Sep 2021 19:15:46 +0200 Subject: [PATCH] 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 + .../user_ldap/composer/composer/installed.php | 4 +- apps/user_ldap/lib/Access.php | 25 +++- .../Version1010Date20200630192842.php | 4 +- .../Version1120Date20210917155206.php | 133 ++++++++++++++++++ apps/user_ldap/tests/AccessTest.php | 31 +++- 8 files changed, 194 insertions(+), 7 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 46bff54cb1e91..b5ea52f909ffe 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.11.0 + 1.11.1 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 6d9d221c10fab..4b4ba60da2984 100644 --- a/apps/user_ldap/composer/composer/autoload_classmap.php +++ b/apps/user_ldap/composer/composer/autoload_classmap.php @@ -60,6 +60,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 afcda7d78101e..8d28f09ba7563 100644 --- a/apps/user_ldap/composer/composer/autoload_static.php +++ b/apps/user_ldap/composer/composer/autoload_static.php @@ -75,6 +75,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/composer/composer/installed.php b/apps/user_ldap/composer/composer/installed.php index 412956a2006c8..8acc70b43ac9e 100644 --- a/apps/user_ldap/composer/composer/installed.php +++ b/apps/user_ldap/composer/composer/installed.php @@ -5,7 +5,7 @@ 'type' => 'library', 'install_path' => __DIR__ . '/../', 'aliases' => array(), - 'reference' => '7b056b2ed72b2a7a5937e0c251858e1906277b4b', + 'reference' => 'add48a086275dcfb604556dbda95503345f57292', 'name' => '__root__', 'dev' => false, ), @@ -16,7 +16,7 @@ 'type' => 'library', 'install_path' => __DIR__ . '/../', 'aliases' => array(), - 'reference' => '7b056b2ed72b2a7a5937e0c251858e1906277b4b', + 'reference' => 'add48a086275dcfb604556dbda95503345f57292', 'dev_requirement' => false, ), ), diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php index 0b243b8ad597a..862089e9d30f4 100644 --- a/apps/user_ldap/lib/Access.php +++ b/apps/user_ldap/lib/Access.php @@ -60,6 +60,8 @@ use OCP\IConfig; use OCP\ILogger; use OCP\IUserManager; +use function strlen; +use function substr; /** * Class Access @@ -579,7 +581,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 @@ -838,6 +840,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) { @@ -1432,6 +1439,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'); } @@ -1439,6 +1450,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 754200405c8c4..3d9cc40b5b281 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 a532bd6fd7a2c..8c401722592a2 100644 --- a/apps/user_ldap/tests/AccessTest.php +++ b/apps/user_ldap/tests/AccessTest.php @@ -697,7 +697,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' + ] ]; } @@ -718,6 +739,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')