From 39974acc5e8e7bd7afc46dbf0f5c770568035b3f Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Tue, 25 May 2021 20:25:19 +0200 Subject: [PATCH] Dont allow reusing usernames We have cases in which app developers forget to delete user data when a user is being deleted. This could be problematic as future users with the same username could then access this data. This mitigates this risk a bit by not allowing reusing of usernames. That said, we still need to fix occurrences of these issues by deleting missed data. Signed-off-by: Lukas Reschke --- .../Version22000Date20210525173326.php | 34 +++++++++ lib/composer/composer/autoload_classmap.php | 3 + lib/composer/composer/autoload_static.php | 3 + lib/private/Server.php | 2 + lib/private/User/Manager.php | 4 + lib/private/User/UserDeletedListener.php | 47 ++++++++++++ .../UsernameDuplicationPreventionManager.php | 75 +++++++++++++++++++ tests/lib/User/UserDeletedListenerTest.php | 53 +++++++++++++ ...ernameDuplicationPreventionManagerTest.php | 61 +++++++++++++++ 9 files changed, 282 insertions(+) create mode 100644 core/Migrations/Version22000Date20210525173326.php create mode 100644 lib/private/User/UserDeletedListener.php create mode 100644 lib/private/User/UsernameDuplicationPreventionManager.php create mode 100644 tests/lib/User/UserDeletedListenerTest.php create mode 100644 tests/lib/User/UsernameDuplicationPreventionManagerTest.php diff --git a/core/Migrations/Version22000Date20210525173326.php b/core/Migrations/Version22000Date20210525173326.php new file mode 100644 index 0000000000..889cccf8fb --- /dev/null +++ b/core/Migrations/Version22000Date20210525173326.php @@ -0,0 +1,34 @@ +createTable('previously_used_userids'); + + $table->addColumn('user_id_hash', \OCP\DB\Types::STRING, [ + 'notnull' => true, + 'length' => 128, + ]); + + $table->setPrimaryKey(['user_id_hash'], 'uid_hash_idx'); + + return $schema; + } +} diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 010c2a53a3..ba5dca578b 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -960,6 +960,7 @@ return array( 'OC\\Core\\Migrations\\Version21000Date20210309185126' => $baseDir . '/core/Migrations/Version21000Date20210309185126.php', 'OC\\Core\\Migrations\\Version21000Date20210309185127' => $baseDir . '/core/Migrations/Version21000Date20210309185127.php', 'OC\\Core\\Migrations\\Version22000Date20210216080825' => $baseDir . '/core/Migrations/Version22000Date20210216080825.php', + 'OC\\Core\\Migrations\\Version22000Date20210525173326' => $baseDir . '/core/Migrations/Version22000Date20210525173326.php', 'OC\\Core\\Notification\\CoreNotifier' => $baseDir . '/core/Notification/CoreNotifier.php', 'OC\\Core\\Service\\LoginFlowV2Service' => $baseDir . '/core/Service/LoginFlowV2Service.php', 'OC\\DB\\Adapter' => $baseDir . '/lib/private/DB/Adapter.php', @@ -1433,6 +1434,8 @@ return array( 'OC\\User\\NoUserException' => $baseDir . '/lib/private/User/NoUserException.php', 'OC\\User\\Session' => $baseDir . '/lib/private/User/Session.php', 'OC\\User\\User' => $baseDir . '/lib/private/User/User.php', + 'OC\\User\\UserDeletedListener' => $baseDir . '/lib/private/User/UserDeletedListener.php', + 'OC\\User\\UsernameDuplicationPreventionManager' => $baseDir . '/lib/private/User/UsernameDuplicationPreventionManager.php', 'OC_API' => $baseDir . '/lib/private/legacy/OC_API.php', 'OC_App' => $baseDir . '/lib/private/legacy/OC_App.php', 'OC_DB' => $baseDir . '/lib/private/legacy/OC_DB.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 698cbc3cfa..214f4073b4 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -989,6 +989,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Core\\Migrations\\Version21000Date20210309185126' => __DIR__ . '/../../..' . '/core/Migrations/Version21000Date20210309185126.php', 'OC\\Core\\Migrations\\Version21000Date20210309185127' => __DIR__ . '/../../..' . '/core/Migrations/Version21000Date20210309185127.php', 'OC\\Core\\Migrations\\Version22000Date20210216080825' => __DIR__ . '/../../..' . '/core/Migrations/Version22000Date20210216080825.php', + 'OC\\Core\\Migrations\\Version22000Date20210525173326' => __DIR__ . '/../../..' . '/core/Migrations/Version22000Date20210525173326.php', 'OC\\Core\\Notification\\CoreNotifier' => __DIR__ . '/../../..' . '/core/Notification/CoreNotifier.php', 'OC\\Core\\Service\\LoginFlowV2Service' => __DIR__ . '/../../..' . '/core/Service/LoginFlowV2Service.php', 'OC\\DB\\Adapter' => __DIR__ . '/../../..' . '/lib/private/DB/Adapter.php', @@ -1462,6 +1463,8 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\User\\NoUserException' => __DIR__ . '/../../..' . '/lib/private/User/NoUserException.php', 'OC\\User\\Session' => __DIR__ . '/../../..' . '/lib/private/User/Session.php', 'OC\\User\\User' => __DIR__ . '/../../..' . '/lib/private/User/User.php', + 'OC\\User\\UserDeletedListener' => __DIR__ . '/../../..' . '/lib/private/User/UserDeletedListener.php', + 'OC\\User\\UsernameDuplicationPreventionManager' => __DIR__ . '/../../..' . '/lib/private/User/UsernameDuplicationPreventionManager.php', 'OC_API' => __DIR__ . '/../../..' . '/lib/private/legacy/OC_API.php', 'OC_App' => __DIR__ . '/../../..' . '/lib/private/legacy/OC_App.php', 'OC_DB' => __DIR__ . '/../../..' . '/lib/private/legacy/OC_DB.php', diff --git a/lib/private/Server.php b/lib/private/Server.php index 9047a7c454..ffda834668 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -142,6 +142,7 @@ use OC\Share20\ShareHelper; use OC\SystemTag\ManagerFactory as SystemTagManagerFactory; use OC\Tagging\TagMapper; use OC\Template\JSCombiner; +use OC\User\UserDeletedListener; use OCA\Theming\ImageManager; use OCA\Theming\ThemingDefaults; use OCA\Theming\Util; @@ -1445,6 +1446,7 @@ class Server extends ServerContainer implements IServerContainer { $eventDispatched = $this->get(IEventDispatcher::class); $eventDispatched->addServiceListener(LoginFailed::class, LoginFailedListener::class); $eventDispatched->addServiceListener(PostLoginEvent::class, UserLoggedInListener::class); + $eventDispatched->addServiceListener(UserDeletedEvent::class, UserDeletedListener::class); } /** diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 59c007b6b5..bed87fbbf7 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -378,6 +378,10 @@ class Manager extends PublicEmitter implements IUserManager { throw new HintException($l->t('The user limit has been reached and the user was not created.')); } + if (\OC::$server->get(UsernameDuplicationPreventionManager::class)->wasUsed($uid)) { + $l = \OC::$server->getL10N('lib'); + throw new HintException($l->t("The user name has already been used previously.")); + } $localBackends = []; foreach ($this->backends as $backend) { if ($backend instanceof Database) { diff --git a/lib/private/User/UserDeletedListener.php b/lib/private/User/UserDeletedListener.php new file mode 100644 index 0000000000..2f446343c7 --- /dev/null +++ b/lib/private/User/UserDeletedListener.php @@ -0,0 +1,47 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\User; + +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use OCP\User\Events\UserDeletedEvent; + +class UserDeletedListener implements IEventListener { + /** @var UsernameDuplicationPreventionManager */ + private $usernameDuplicationPreventionManager; + + public function __construct(UsernameDuplicationPreventionManager $usernameDuplicationPreventionManager) { + $this->usernameDuplicationPreventionManager = $usernameDuplicationPreventionManager; + } + + public function handle(Event $event): void { + if (!($event instanceof UserDeletedEvent)) { + return; + } + + $user = $event->getUser(); + $this->usernameDuplicationPreventionManager->markUsed($user->getUID()); + } +} diff --git a/lib/private/User/UsernameDuplicationPreventionManager.php b/lib/private/User/UsernameDuplicationPreventionManager.php new file mode 100644 index 0000000000..671e11abb8 --- /dev/null +++ b/lib/private/User/UsernameDuplicationPreventionManager.php @@ -0,0 +1,75 @@ + + * + * @author Lukas Reschke + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\User; + +use OCP\IDBConnection; + +class UsernameDuplicationPreventionManager { + /** @var IDBConnection */ + private $dbConnection; + private const TABLE_NAME = 'previously_used_userids'; + private const HASHED_USER_ID_NAME = 'user_id_hash'; + + public function __construct(IDBConnection $connection) { + $this->dbConnection = $connection; + } + + private function calculateUserNameHash(string $username) : string { + return hash('sha512', $username); + } + + public function markUsed(string $userName) : void { + $queryBuilder = $this->dbConnection->getQueryBuilder(); + $queryBuilder->insert(self::TABLE_NAME) + ->values([ + self::HASHED_USER_ID_NAME => $queryBuilder->createNamedParameter($this->calculateUserNameHash($userName)), + ]) + ->executeStatement(); + } + + public function wasUsed(string $userName) : bool { + $queryBuilder = $this->dbConnection->getQueryBuilder(); + $result = $queryBuilder->select($queryBuilder->func()->count()) + ->from(self::TABLE_NAME) + ->where( + $queryBuilder->expr()->eq( + self::HASHED_USER_ID_NAME, + $queryBuilder->expr()->literal($this->calculateUserNameHash($userName)) + ) + ) + ->executeQuery(); + return (int)$result->fetchOne() !== 0; + } + + public function cleanUp(): void { + $qb = $this->dbConnection->getQueryBuilder(); + + $qb + ->delete(self::TABLE_NAME) + ->executeStatement(); + } +} diff --git a/tests/lib/User/UserDeletedListenerTest.php b/tests/lib/User/UserDeletedListenerTest.php new file mode 100644 index 0000000000..c1834e17e3 --- /dev/null +++ b/tests/lib/User/UserDeletedListenerTest.php @@ -0,0 +1,53 @@ + + * + * @author Lukas Reschke + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace Test\User; + +use OC\User\UserDeletedListener; +use OC\User\UsernameDuplicationPreventionManager; +use OCP\IUser; +use OCP\User\Events\UserDeletedEvent; +use Test\TestCase; + +class UserDeletedListenerTest extends TestCase { + public function testHandle() { + /** @var UsernameDuplicationPreventionManager|PHPUnit_Framework_MockObject_MockObject $usernameDuplicationPreventionManager */ + $usernameDuplicationPreventionManager = $this->createMock(UsernameDuplicationPreventionManager::class); + $usernameDuplicationPreventionManager + ->expects($this->once()) + ->method('markUsed') + ->with($this->equalTo('ThisIsTheUsername')); + /** @var IUser|PHPUnit_Framework_MockObject_MockObject $mockUser */ + $mockUser = $this->createMock(IUser::class); + $mockUser + ->expects($this->once()) + ->method('getUID') + ->willReturn('ThisIsTheUsername'); + + $listener = new UserDeletedListener($usernameDuplicationPreventionManager); + $listener->handle(new UserDeletedEvent($mockUser)); + } +} diff --git a/tests/lib/User/UsernameDuplicationPreventionManagerTest.php b/tests/lib/User/UsernameDuplicationPreventionManagerTest.php new file mode 100644 index 0000000000..8ed27c2314 --- /dev/null +++ b/tests/lib/User/UsernameDuplicationPreventionManagerTest.php @@ -0,0 +1,61 @@ + + * + * @author Lukas Reschke + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace Test\User; + +use OC\User\UsernameDuplicationPreventionManager; +use Test\TestCase; + +/** + * @group DB + */ +class UsernameDuplicationPreventionManagerTest extends TestCase { + /** @var UsernameDuplicationPreventionManager */ + private $usernameDuplicationPreventionManager; + + protected function setUp(): void { + parent::setUp(); + + $this->usernameDuplicationPreventionManager = \OC::$server->get(UsernameDuplicationPreventionManager::class); + } + + protected function tearDown(): void { + parent::tearDown(); + + $this->usernameDuplicationPreventionManager->cleanUp(); + } + + public function testNotMarkedAsDeleted() { + $return = $this->usernameDuplicationPreventionManager->wasUsed('not_deleted_user'); + $this->assertFalse($return); + } + + public function testMarkedAsDeleted() { + $this->usernameDuplicationPreventionManager->markUsed('deleted_user'); + $return = $this->usernameDuplicationPreventionManager->wasUsed('deleted_user'); + $this->assertTrue($return); + } +}