Skip to content

Commit

Permalink
ensure that user and group IDs in LDAP's tables are also max 64chars
Browse files Browse the repository at this point in the history
- 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 <blizzz@arthur-schiwon.de>
  • Loading branch information
blizzz authored and backportbot[bot] committed Sep 27, 2021
1 parent 62a5d27 commit add48a0
Show file tree
Hide file tree
Showing 6 changed files with 191 additions and 4 deletions.
1 change: 1 addition & 0 deletions apps/user_ldap/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
1 change: 1 addition & 0 deletions apps/user_ldap/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
25 changes: 24 additions & 1 deletion apps/user_ldap/lib/Access.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@
use OCP\IConfig;
use OCP\ILogger;
use OCP\IUserManager;
use function strlen;
use function substr;

/**
* Class Access
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -1432,13 +1439,29 @@ 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');
}

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
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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, [
Expand All @@ -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, [
Expand Down
133 changes: 133 additions & 0 deletions apps/user_ldap/lib/Migration/Version1120Date20210917155206.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
<?php

declare(strict_types=1);

namespace OCA\User_LDAP\Migration;

use Closure;
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;
use OCP\Migration\SimpleMigrationStep;
use Psr\Log\LoggerInterface;

class Version1120Date20210917155206 extends SimpleMigrationStep {

/** @var IDBConnection */
private $dbc;
/** @var IUserManager */
private $userManager;
/** @var LoggerInterface */
private $logger;

public function __construct(IDBConnection $dbc, IUserManager $userManager, LoggerInterface $logger) {
$this->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]);
}
}
}
31 changes: 30 additions & 1 deletion apps/user_ldap/tests/AccessTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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'
]
];
}

Expand All @@ -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')
Expand Down

0 comments on commit add48a0

Please sign in to comment.