From 7586b19e524761c1e8aab5170375a0d6c9e8f7a2 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 10 Sep 2018 17:02:37 +0200 Subject: [PATCH] Only allow 2FA state changs if providers support the operation Ref https://github.com/nextcloud/server/issues/11019. Add `twofactorauth:cleanup` command Signed-off-by: Christoph Wurst --- core/Command/TwoFactorAuth/Cleanup.php | 61 +++++++ core/Command/TwoFactorAuth/Disable.php | 18 +- core/Command/TwoFactorAuth/Enable.php | 19 ++- core/Command/TwoFactorAuth/State.php | 8 +- core/register_command.php | 9 +- lib/composer/composer/autoload_classmap.php | 3 + lib/composer/composer/autoload_static.php | 3 + .../Exceptions/InvalidProviderException.php | 38 +++++ .../Db/ProviderUserAssignmentDao.php | 9 + .../Authentication/TwoFactorAuth/Manager.php | 26 +-- .../TwoFactorAuth/ProviderManager.php | 98 +++++++++++ .../Authentication/TwoFactorAuth/Registry.php | 3 + .../TwoFactorAuth/IRegistry.php | 15 ++ .../Command/TwoFactorAuth/CleanupTest.php | 66 ++++++++ .../Command/TwoFactorAuth/DisableTest.php | 117 +++++++------ .../Core/Command/TwoFactorAuth/EnableTest.php | 120 ++++++++------ .../Core/Command/TwoFactorAuth/StateTest.php | 113 +++++++++++++ .../Db/ProviderUserAssignmentDaoTest.php | 14 ++ .../TwoFactorAuth/ManagerTest.php | 28 ---- .../TwoFactorAuth/ProviderManagerTest.php | 154 ++++++++++++++++++ .../TwoFactorAuth/RegistryTest.php | 8 + 21 files changed, 752 insertions(+), 178 deletions(-) create mode 100644 core/Command/TwoFactorAuth/Cleanup.php create mode 100644 lib/private/Authentication/Exceptions/InvalidProviderException.php create mode 100644 lib/private/Authentication/TwoFactorAuth/ProviderManager.php create mode 100644 tests/Core/Command/TwoFactorAuth/CleanupTest.php create mode 100644 tests/Core/Command/TwoFactorAuth/StateTest.php create mode 100644 tests/lib/Authentication/TwoFactorAuth/ProviderManagerTest.php diff --git a/core/Command/TwoFactorAuth/Cleanup.php b/core/Command/TwoFactorAuth/Cleanup.php new file mode 100644 index 0000000000..b9acc64978 --- /dev/null +++ b/core/Command/TwoFactorAuth/Cleanup.php @@ -0,0 +1,61 @@ + + * + * @author 2018 Christoph Wurst + * + * @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\Core\Command\TwoFactorAuth; + +use OCP\Authentication\TwoFactorAuth\IRegistry; +use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\OutputInterface; + +class Cleanup extends Base { + + /** @var IRegistry */ + private $registry; + + public function __construct(IRegistry $registry) { + parent::__construct(); + + $this->registry = $registry; + } + + protected function configure() { + parent::configure(); + + $this->setName('twofactorauth:cleanup'); + $this->setDescription('Clean up the two-factor user-provider association of an uninstalled/removed provider'); + $this->addArgument('provider-id', InputArgument::REQUIRED); + } + + protected function execute(InputInterface $input, OutputInterface $output) { + $providerId = $input->getArgument('provider-id'); + + $this->registry->cleanUp($providerId); + + $output->writeln("All user-provider associations for provider $providerId have been removed."); + } + +} diff --git a/core/Command/TwoFactorAuth/Disable.php b/core/Command/TwoFactorAuth/Disable.php index 0564c89017..fc180e790b 100644 --- a/core/Command/TwoFactorAuth/Disable.php +++ b/core/Command/TwoFactorAuth/Disable.php @@ -24,6 +24,7 @@ namespace OC\Core\Command\TwoFactorAuth; use OC\Authentication\TwoFactorAuth\Manager; +use OC\Authentication\TwoFactorAuth\ProviderManager; use OCP\IUserManager; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -31,13 +32,13 @@ use Symfony\Component\Console\Output\OutputInterface; class Disable extends Base { - /** @var Manager */ + /** @var ProviderManager */ private $manager; /** @var IUserManager */ protected $userManager; - public function __construct(Manager $manager, IUserManager $userManager) { + public function __construct(ProviderManager $manager, IUserManager $userManager) { parent::__construct('twofactorauth:disable'); $this->manager = $manager; $this->userManager = $userManager; @@ -49,17 +50,24 @@ class Disable extends Base { $this->setName('twofactorauth:disable'); $this->setDescription('Disable two-factor authentication for a user'); $this->addArgument('uid', InputArgument::REQUIRED); + $this->addArgument('provider_id', InputArgument::REQUIRED); } protected function execute(InputInterface $input, OutputInterface $output) { $uid = $input->getArgument('uid'); + $providerId = $input->getArgument('provider_id'); $user = $this->userManager->get($uid); if (is_null($user)) { $output->writeln("Invalid UID"); - return; + return 1; + } + if ($this->manager->tryDisableProviderFor($providerId, $user)) { + $output->writeln("Two-factor provider $providerId disabled for user $uid."); + return 0; + } else { + $output->writeln("The provider does not support this operation."); + return 2; } - $this->manager->disableTwoFactorAuthentication($user); - $output->writeln("Two-factor authentication disabled for user $uid"); } } diff --git a/core/Command/TwoFactorAuth/Enable.php b/core/Command/TwoFactorAuth/Enable.php index 98e8b178cd..4a9c12e686 100644 --- a/core/Command/TwoFactorAuth/Enable.php +++ b/core/Command/TwoFactorAuth/Enable.php @@ -23,7 +23,7 @@ namespace OC\Core\Command\TwoFactorAuth; -use OC\Authentication\TwoFactorAuth\Manager; +use OC\Authentication\TwoFactorAuth\ProviderManager; use OCP\IUserManager; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -31,13 +31,13 @@ use Symfony\Component\Console\Output\OutputInterface; class Enable extends Base { - /** @var Manager */ + /** @var ProviderManager */ private $manager; /** @var IUserManager */ protected $userManager; - public function __construct(Manager $manager, IUserManager $userManager) { + public function __construct(ProviderManager $manager, IUserManager $userManager) { parent::__construct('twofactorauth:enable'); $this->manager = $manager; $this->userManager = $userManager; @@ -49,17 +49,24 @@ class Enable extends Base { $this->setName('twofactorauth:enable'); $this->setDescription('Enable two-factor authentication for a user'); $this->addArgument('uid', InputArgument::REQUIRED); + $this->addArgument('provider_id', InputArgument::REQUIRED); } protected function execute(InputInterface $input, OutputInterface $output) { $uid = $input->getArgument('uid'); + $providerId = $input->getArgument('provider_id'); $user = $this->userManager->get($uid); if (is_null($user)) { $output->writeln("Invalid UID"); - return; + return 1; + } + if ($this->manager->tryEnableProviderFor($providerId, $user)) { + $output->writeln("Two-factor provider $providerId enabled for user $uid."); + return 0; + } else { + $output->writeln("The provider does not support this operation."); + return 2; } - $this->manager->enableTwoFactorAuthentication($user); - $output->writeln("Two-factor authentication enabled for user $uid"); } } diff --git a/core/Command/TwoFactorAuth/State.php b/core/Command/TwoFactorAuth/State.php index 73e17b4ceb..66d2b4f3ee 100644 --- a/core/Command/TwoFactorAuth/State.php +++ b/core/Command/TwoFactorAuth/State.php @@ -1,6 +1,6 @@ @@ -57,7 +57,7 @@ class State extends Base { $user = $this->userManager->get($uid); if (is_null($user)) { $output->writeln("Invalid UID"); - return; + return 1; } $providerStates = $this->registry->getProviderStates($user); @@ -73,6 +73,8 @@ class State extends Base { $output->writeln(""); $this->printProviders("Enabled providers", $enabled, $output); $this->printProviders("Disabled providers", $disabled, $output); + + return 0; } private function filterEnabledDisabledUnknownProviders(array $providerStates): array { @@ -91,7 +93,7 @@ class State extends Base { } private function printProviders(string $title, array $providers, - OutputInterface $output) { + OutputInterface $output) { if (empty($providers)) { // Ignore and don't print anything return; diff --git a/core/register_command.php b/core/register_command.php index 744ee23e16..ed0220e705 100644 --- a/core/register_command.php +++ b/core/register_command.php @@ -66,12 +66,9 @@ if (\OC::$server->getConfig()->getSystemValue('installed', false)) { $application->add(new OC\Core\Command\App\GetPath()); $application->add(new OC\Core\Command\App\ListApps(\OC::$server->getAppManager())); - $application->add(new OC\Core\Command\TwoFactorAuth\Enable( - \OC::$server->getTwoFactorAuthManager(), \OC::$server->getUserManager() - )); - $application->add(new OC\Core\Command\TwoFactorAuth\Disable( - \OC::$server->getTwoFactorAuthManager(), \OC::$server->getUserManager() - )); + $application->add(\OC::$server->query(\OC\Core\Command\TwoFactorAuth\Cleanup::class)); + $application->add(\OC::$server->query(\OC\Core\Command\TwoFactorAuth\Enable::class)); + $application->add(\OC::$server->query(\OC\Core\Command\TwoFactorAuth\Disable::class)); $application->add(\OC::$server->query(\OC\Core\Command\TwoFactorAuth\State::class)); $application->add(new OC\Core\Command\Background\Cron(\OC::$server->getConfig())); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 849dca209d..0379b76775 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -434,6 +434,7 @@ return array( 'OC\\Archive\\Archive' => $baseDir . '/lib/private/Archive/Archive.php', 'OC\\Archive\\TAR' => $baseDir . '/lib/private/Archive/TAR.php', 'OC\\Archive\\ZIP' => $baseDir . '/lib/private/Archive/ZIP.php', + 'OC\\Authentication\\Exceptions\\InvalidProviderException' => $baseDir . '/lib/private/Authentication/Exceptions/InvalidProviderException.php', 'OC\\Authentication\\Exceptions\\InvalidTokenException' => $baseDir . '/lib/private/Authentication/Exceptions/InvalidTokenException.php', 'OC\\Authentication\\Exceptions\\LoginRequiredException' => $baseDir . '/lib/private/Authentication/Exceptions/LoginRequiredException.php', 'OC\\Authentication\\Exceptions\\PasswordLoginForbiddenException' => $baseDir . '/lib/private/Authentication/Exceptions/PasswordLoginForbiddenException.php', @@ -456,6 +457,7 @@ return array( 'OC\\Authentication\\TwoFactorAuth\\Db\\ProviderUserAssignmentDao' => $baseDir . '/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php', 'OC\\Authentication\\TwoFactorAuth\\Manager' => $baseDir . '/lib/private/Authentication/TwoFactorAuth/Manager.php', 'OC\\Authentication\\TwoFactorAuth\\ProviderLoader' => $baseDir . '/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php', + 'OC\\Authentication\\TwoFactorAuth\\ProviderManager' => $baseDir . '/lib/private/Authentication/TwoFactorAuth/ProviderManager.php', 'OC\\Authentication\\TwoFactorAuth\\ProviderSet' => $baseDir . '/lib/private/Authentication/TwoFactorAuth/ProviderSet.php', 'OC\\Authentication\\TwoFactorAuth\\Registry' => $baseDir . '/lib/private/Authentication/TwoFactorAuth/Registry.php', 'OC\\Avatar' => $baseDir . '/lib/private/Avatar.php', @@ -571,6 +573,7 @@ return array( 'OC\\Core\\Command\\Security\\RemoveCertificate' => $baseDir . '/core/Command/Security/RemoveCertificate.php', 'OC\\Core\\Command\\Status' => $baseDir . '/core/Command/Status.php', 'OC\\Core\\Command\\TwoFactorAuth\\Base' => $baseDir . '/core/Command/TwoFactorAuth/Base.php', + 'OC\\Core\\Command\\TwoFactorAuth\\Cleanup' => $baseDir . '/core/Command/TwoFactorAuth/Cleanup.php', 'OC\\Core\\Command\\TwoFactorAuth\\Disable' => $baseDir . '/core/Command/TwoFactorAuth/Disable.php', 'OC\\Core\\Command\\TwoFactorAuth\\Enable' => $baseDir . '/core/Command/TwoFactorAuth/Enable.php', 'OC\\Core\\Command\\TwoFactorAuth\\State' => $baseDir . '/core/Command/TwoFactorAuth/State.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index c802bf4c57..0456e78442 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -464,6 +464,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Archive\\Archive' => __DIR__ . '/../../..' . '/lib/private/Archive/Archive.php', 'OC\\Archive\\TAR' => __DIR__ . '/../../..' . '/lib/private/Archive/TAR.php', 'OC\\Archive\\ZIP' => __DIR__ . '/../../..' . '/lib/private/Archive/ZIP.php', + 'OC\\Authentication\\Exceptions\\InvalidProviderException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/InvalidProviderException.php', 'OC\\Authentication\\Exceptions\\InvalidTokenException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/InvalidTokenException.php', 'OC\\Authentication\\Exceptions\\LoginRequiredException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/LoginRequiredException.php', 'OC\\Authentication\\Exceptions\\PasswordLoginForbiddenException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/PasswordLoginForbiddenException.php', @@ -486,6 +487,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Authentication\\TwoFactorAuth\\Db\\ProviderUserAssignmentDao' => __DIR__ . '/../../..' . '/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php', 'OC\\Authentication\\TwoFactorAuth\\Manager' => __DIR__ . '/../../..' . '/lib/private/Authentication/TwoFactorAuth/Manager.php', 'OC\\Authentication\\TwoFactorAuth\\ProviderLoader' => __DIR__ . '/../../..' . '/lib/private/Authentication/TwoFactorAuth/ProviderLoader.php', + 'OC\\Authentication\\TwoFactorAuth\\ProviderManager' => __DIR__ . '/../../..' . '/lib/private/Authentication/TwoFactorAuth/ProviderManager.php', 'OC\\Authentication\\TwoFactorAuth\\ProviderSet' => __DIR__ . '/../../..' . '/lib/private/Authentication/TwoFactorAuth/ProviderSet.php', 'OC\\Authentication\\TwoFactorAuth\\Registry' => __DIR__ . '/../../..' . '/lib/private/Authentication/TwoFactorAuth/Registry.php', 'OC\\Avatar' => __DIR__ . '/../../..' . '/lib/private/Avatar.php', @@ -601,6 +603,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Core\\Command\\Security\\RemoveCertificate' => __DIR__ . '/../../..' . '/core/Command/Security/RemoveCertificate.php', 'OC\\Core\\Command\\Status' => __DIR__ . '/../../..' . '/core/Command/Status.php', 'OC\\Core\\Command\\TwoFactorAuth\\Base' => __DIR__ . '/../../..' . '/core/Command/TwoFactorAuth/Base.php', + 'OC\\Core\\Command\\TwoFactorAuth\\Cleanup' => __DIR__ . '/../../..' . '/core/Command/TwoFactorAuth/Cleanup.php', 'OC\\Core\\Command\\TwoFactorAuth\\Disable' => __DIR__ . '/../../..' . '/core/Command/TwoFactorAuth/Disable.php', 'OC\\Core\\Command\\TwoFactorAuth\\Enable' => __DIR__ . '/../../..' . '/core/Command/TwoFactorAuth/Enable.php', 'OC\\Core\\Command\\TwoFactorAuth\\State' => __DIR__ . '/../../..' . '/core/Command/TwoFactorAuth/State.php', diff --git a/lib/private/Authentication/Exceptions/InvalidProviderException.php b/lib/private/Authentication/Exceptions/InvalidProviderException.php new file mode 100644 index 0000000000..6cdd094750 --- /dev/null +++ b/lib/private/Authentication/Exceptions/InvalidProviderException.php @@ -0,0 +1,38 @@ + + * + * @author 2018 Christoph Wurst + * + * @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\Authentication\Exceptions; + +use Exception; +use Throwable; + +class InvalidProviderException extends Exception { + + public function __construct(string $providerId, Throwable $previous = null) { + parent::__construct("The provider '$providerId' does not exist'", 0, $previous); + } + +} diff --git a/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php b/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php index ce03eb0966..d8299a335a 100644 --- a/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php +++ b/lib/private/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDao.php @@ -93,4 +93,13 @@ class ProviderUserAssignmentDao { } + public function deleteAll(string $providerId) { + $qb = $this->conn->getQueryBuilder(); + + $deleteQuery = $qb->delete(self::TABLE_NAME) + ->where($qb->expr()->eq('provider_id', $qb->createNamedParameter($providerId))); + + $deleteQuery->execute(); + } + } diff --git a/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php index 6fa41897e1..71cc5874e5 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -100,12 +100,6 @@ class Manager { * @return boolean */ public function isTwoFactorAuthenticated(IUser $user): bool { - $twoFactorEnabled = ((int) $this->config->getUserValue($user->getUID(), 'core', 'two_factor_auth_disabled', 0)) === 0; - - if (!$twoFactorEnabled) { - return false; - } - $providerStates = $this->providerRegistry->getProviderStates($user); $providers = $this->providerLoader->getProviders($user); $fixedStates = $this->fixMissingProviderStates($providerStates, $providers, $user); @@ -113,25 +107,7 @@ class Manager { $providerIds = array_keys($enabled); $providerIdsWithoutBackupCodes = array_diff($providerIds, [self::BACKUP_CODES_PROVIDER_ID]); - return $twoFactorEnabled && !empty($providerIdsWithoutBackupCodes); - } - - /** - * Disable 2FA checks for the given user - * - * @param IUser $user - */ - public function disableTwoFactorAuthentication(IUser $user) { - $this->config->setUserValue($user->getUID(), 'core', 'two_factor_auth_disabled', 1); - } - - /** - * Enable all 2FA checks for the given user - * - * @param IUser $user - */ - public function enableTwoFactorAuthentication(IUser $user) { - $this->config->deleteUserValue($user->getUID(), 'core', 'two_factor_auth_disabled'); + return !empty($providerIdsWithoutBackupCodes); } /** diff --git a/lib/private/Authentication/TwoFactorAuth/ProviderManager.php b/lib/private/Authentication/TwoFactorAuth/ProviderManager.php new file mode 100644 index 0000000000..234085b062 --- /dev/null +++ b/lib/private/Authentication/TwoFactorAuth/ProviderManager.php @@ -0,0 +1,98 @@ + + * + * @author 2018 Christoph Wurst + * + * @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\Authentication\TwoFactorAuth; + +use OC\Authentication\Exceptions\InvalidProviderException; +use OCP\Authentication\TwoFactorAuth\IActivatableByAdmin; +use OCP\Authentication\TwoFactorAuth\IDeactivableByAdmin; +use OCP\Authentication\TwoFactorAuth\IDeactivatableByAdmin; +use OCP\Authentication\TwoFactorAuth\IProvider; +use OCP\Authentication\TwoFactorAuth\IRegistry; +use OCP\IUser; + +class ProviderManager { + + /** @var ProviderLoader */ + private $providerLoader; + + /** @var IRegistry */ + private $providerRegistry; + + public function __construct(ProviderLoader $providerLoader, IRegistry $providerRegistry) { + $this->providerLoader = $providerLoader; + $this->providerRegistry = $providerRegistry; + } + + private function getProvider(string $providerId, IUser $user): IProvider { + $providers = $this->providerLoader->getProviders($user); + + if (!isset($providers[$providerId])) { + throw new InvalidProviderException($providerId); + } + + return $providers[$providerId]; + } + + /** + * Try to enable the provider with the given id for the given user + * + * @param IUser $user + * + * @return bool whether the provider supports this operation + */ + public function tryEnableProviderFor(string $providerId, IUser $user): bool { + $provider = $this->getProvider($providerId, $user); + + if ($provider instanceof IActivatableByAdmin) { + $provider->enableFor($user); + $this->providerRegistry->enableProviderFor($provider, $user); + return true; + } else { + return false; + } + } + + /** + * Try to disable the provider with the given id for the given user + * + * @param IUser $user + * + * @return bool whether the provider supports this operation + */ + public function tryDisableProviderFor(string $providerId, IUser $user): bool { + $provider = $this->getProvider($providerId, $user); + + if ($provider instanceof IDeactivatableByAdmin) { + $provider->disableFor($user); + $this->providerRegistry->disableProviderFor($provider, $user); + return true; + } else { + return false; + } + } + +} diff --git a/lib/private/Authentication/TwoFactorAuth/Registry.php b/lib/private/Authentication/TwoFactorAuth/Registry.php index 0cfb052440..2fc90e5d6d 100644 --- a/lib/private/Authentication/TwoFactorAuth/Registry.php +++ b/lib/private/Authentication/TwoFactorAuth/Registry.php @@ -52,4 +52,7 @@ class Registry implements IRegistry { $this->assignmentDao->persist($provider->getId(), $user->getUID(), 0); } + public function cleanUp(string $providerId) { + $this->assignmentDao->deleteAll($providerId); + } } diff --git a/lib/public/Authentication/TwoFactorAuth/IRegistry.php b/lib/public/Authentication/TwoFactorAuth/IRegistry.php index 5013892d40..5d97c57bcf 100644 --- a/lib/public/Authentication/TwoFactorAuth/IRegistry.php +++ b/lib/public/Authentication/TwoFactorAuth/IRegistry.php @@ -62,4 +62,19 @@ interface IRegistry { * @since 14.0.0 */ public function disableProviderFor(IProvider $provider, IUser $user); + + /** + * Cleans up all entries of the provider with the given id. This is only + * necessary in edge-cases where an admin disabled and/or uninstalled a + * provider app. Invoking this method will make sure outdated provider + * associations are removed so that users can log in. + * + * @since 15.0.0 + * + * @param string $providerId + * + * @return void + */ + public function cleanUp(string $providerId); + } diff --git a/tests/Core/Command/TwoFactorAuth/CleanupTest.php b/tests/Core/Command/TwoFactorAuth/CleanupTest.php new file mode 100644 index 0000000000..227283decf --- /dev/null +++ b/tests/Core/Command/TwoFactorAuth/CleanupTest.php @@ -0,0 +1,66 @@ + + * + * @author 2018 Christoph Wurst + * + * @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 Core\Command\TwoFactorAuth; + +use OC\Core\Command\TwoFactorAuth\Cleanup; +use OCP\Authentication\TwoFactorAuth\IRegistry; +use PHPUnit\Framework\MockObject\MockObject; +use Symfony\Component\Console\Tester\CommandTester; +use Test\TestCase; + +class CleanupTest extends TestCase { + + /** @var IRegistry|MockObject */ + private $registry; + + /** @var CommandTester */ + private $cmd; + + protected function setUp() { + parent::setUp(); + + $this->registry = $this->createMock(IRegistry::class); + + $cmd = new Cleanup($this->registry); + $this->cmd = new CommandTester($cmd); + } + + public function testCleanup() { + $this->registry->expects($this->once()) + ->method('cleanUp') + ->with('u2f'); + + $rc = $this->cmd->execute([ + 'provider-id' => 'u2f', + ]); + + $this->assertEquals(0, $rc); + $output = $this->cmd->getDisplay(); + $this->assertContains("All user-provider associations for provider u2f have been removed", $output); + } + +} diff --git a/tests/Core/Command/TwoFactorAuth/DisableTest.php b/tests/Core/Command/TwoFactorAuth/DisableTest.php index 1a0bbc6c3d..30ebc007dc 100644 --- a/tests/Core/Command/TwoFactorAuth/DisableTest.php +++ b/tests/Core/Command/TwoFactorAuth/DisableTest.php @@ -1,8 +1,11 @@ + * @copyright 2018 Christoph Wurst * - * @author Roeland Jago Douma + * @author 2018 Christoph Wurst * * @license GNU AGPL version 3 or any later version * @@ -20,80 +23,90 @@ * along with this program. If not, see . * */ + namespace Test\Core\Command\TwoFactorAuth; -use OC\Authentication\TwoFactorAuth\Manager; +use OC\Authentication\TwoFactorAuth\ProviderManager; use OC\Core\Command\TwoFactorAuth\Disable; use OCP\IUser; use OCP\IUserManager; -use Symfony\Component\Console\Input\InputInterface; -use Symfony\Component\Console\Output\OutputInterface; +use PHPUnit\Framework\MockObject\MockObject; +use Symfony\Component\Console\Tester\CommandTester; use Test\TestCase; class DisableTest extends TestCase { - /** @var Manager|\PHPUnit_Framework_MockObject_MockObject */ - private $manager; + /** @var ProviderManager|MockObject */ + private $providerManager; - /** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */ + /** @var IUserManager|MockObject */ private $userManager; - /** @var Disable */ + /** @var CommandTester */ private $command; public function setUp() { parent::setUp(); - $this->manager = $this->createMock(Manager::class); + $this->providerManager = $this->createMock(ProviderManager::class); $this->userManager = $this->createMock(IUserManager::class); - $this->command = new Disable($this->manager, $this->userManager); + $cmd = new Disable($this->providerManager, $this->userManager); + $this->command = new CommandTester($cmd); } - public function testDisableSuccess() { - $user = $this->createMock(IUser::class); - - $input = $this->createMock(InputInterface::class); - $output = $this->createMock(OutputInterface::class); - - $input->method('getArgument') - ->with($this->equalTo('uid')) - ->willReturn('user'); - - $this->userManager->method('get') - ->with('user') - ->willReturn($user); - - $this->manager->expects($this->once()) - ->method('disableTwoFactorAuthentication') - ->with($this->equalTo($user)); - - $output->expects($this->once()) - ->method('writeln') - ->with('Two-factor authentication disabled for user user'); - - $this->invokePrivate($this->command, 'execute', [$input, $output]); - } - - public function testEnableFail() { - $input = $this->createMock(InputInterface::class); - $output = $this->createMock(OutputInterface::class); - - $input->method('getArgument') - ->with($this->equalTo('uid')) - ->willReturn('user'); - - $this->userManager->method('get') - ->with('user') + public function testInvalidUID() { + $this->userManager->expects($this->once()) + ->method('get') + ->with('nope') ->willReturn(null); - $this->manager->expects($this->never()) - ->method($this->anything()); + $rc = $this->command->execute([ + 'uid' => 'nope', + 'provider_id' => 'nope', + ]); - $output->expects($this->once()) - ->method('writeln') - ->with('Invalid UID'); + $this->assertEquals(1, $rc); + $this->assertContains("Invalid UID", $this->command->getDisplay()); + } - $this->invokePrivate($this->command, 'execute', [$input, $output]); + public function testEnableNotSupported() { + $user = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('get') + ->with('ricky') + ->willReturn($user); + $this->providerManager->expects($this->once()) + ->method('tryDisableProviderFor') + ->with('totp', $user) + ->willReturn(false); + + $rc = $this->command->execute([ + 'uid' => 'ricky', + 'provider_id' => 'totp', + ]); + + $this->assertEquals(2, $rc); + $this->assertContains("The provider does not support this operation", $this->command->getDisplay()); + } + + public function testEnabled() { + $user = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('get') + ->with('ricky') + ->willReturn($user); + $this->providerManager->expects($this->once()) + ->method('tryDisableProviderFor') + ->with('totp', $user) + ->willReturn(true); + + $rc = $this->command->execute([ + 'uid' => 'ricky', + 'provider_id' => 'totp', + ]); + + $this->assertEquals(0, $rc); + $this->assertContains("Two-factor provider totp disabled for user ricky", $this->command->getDisplay()); } } diff --git a/tests/Core/Command/TwoFactorAuth/EnableTest.php b/tests/Core/Command/TwoFactorAuth/EnableTest.php index ebca40df9a..f31f92da4a 100644 --- a/tests/Core/Command/TwoFactorAuth/EnableTest.php +++ b/tests/Core/Command/TwoFactorAuth/EnableTest.php @@ -1,8 +1,11 @@ + * @copyright 2018 Christoph Wurst * - * @author Roeland Jago Douma + * @author 2018 Christoph Wurst * * @license GNU AGPL version 3 or any later version * @@ -20,80 +23,91 @@ * along with this program. If not, see . * */ + namespace Test\Core\Command\TwoFactorAuth; -use OC\Authentication\TwoFactorAuth\Manager; +use OC\Authentication\TwoFactorAuth\ProviderManager; use OC\Core\Command\TwoFactorAuth\Enable; use OCP\IUser; use OCP\IUserManager; -use Symfony\Component\Console\Input\InputInterface; -use Symfony\Component\Console\Output\OutputInterface; +use PHPUnit\Framework\MockObject\MockObject; +use Symfony\Component\Console\Tester\CommandTester; use Test\TestCase; class EnableTest extends TestCase { - /** @var Manager|\PHPUnit_Framework_MockObject_MockObject */ - private $manager; + /** @var ProviderManager|MockObject */ + private $providerManager; - /** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */ + /** @var IUserManager|MockObject */ private $userManager; - /** @var Enable */ + /** @var CommandTester */ private $command; public function setUp() { parent::setUp(); - $this->manager = $this->createMock(Manager::class); + $this->providerManager = $this->createMock(ProviderManager::class); $this->userManager = $this->createMock(IUserManager::class); - $this->command = new Enable($this->manager, $this->userManager); + $cmd = new Enable($this->providerManager, $this->userManager); + $this->command = new CommandTester($cmd); } - public function testEnableSuccess() { - $user = $this->createMock(IUser::class); - - $input = $this->createMock(InputInterface::class); - $output = $this->createMock(OutputInterface::class); - - $input->method('getArgument') - ->with($this->equalTo('uid')) - ->willReturn('user'); - - $this->userManager->method('get') - ->with('user') - ->willReturn($user); - - $this->manager->expects($this->once()) - ->method('enableTwoFactorAuthentication') - ->with($this->equalTo($user)); - - $output->expects($this->once()) - ->method('writeln') - ->with('Two-factor authentication enabled for user user'); - - $this->invokePrivate($this->command, 'execute', [$input, $output]); - } - - public function testEnableFail() { - $input = $this->createMock(InputInterface::class); - $output = $this->createMock(OutputInterface::class); - - $input->method('getArgument') - ->with($this->equalTo('uid')) - ->willReturn('user'); - - $this->userManager->method('get') - ->with('user') + public function testInvalidUID() { + $this->userManager->expects($this->once()) + ->method('get') + ->with('nope') ->willReturn(null); - $this->manager->expects($this->never()) - ->method($this->anything()); + $rc = $this->command->execute([ + 'uid' => 'nope', + 'provider_id' => 'nope', + ]); - $output->expects($this->once()) - ->method('writeln') - ->with('Invalid UID'); - - $this->invokePrivate($this->command, 'execute', [$input, $output]); + $this->assertEquals(1, $rc); + $this->assertContains("Invalid UID", $this->command->getDisplay()); } + + public function testEnableNotSupported() { + $user = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('get') + ->with('belle') + ->willReturn($user); + $this->providerManager->expects($this->once()) + ->method('tryEnableProviderFor') + ->with('totp', $user) + ->willReturn(false); + + $rc = $this->command->execute([ + 'uid' => 'belle', + 'provider_id' => 'totp', + ]); + + $this->assertEquals(2, $rc); + $this->assertContains("The provider does not support this operation", $this->command->getDisplay()); + } + + public function testEnabled() { + $user = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('get') + ->with('belle') + ->willReturn($user); + $this->providerManager->expects($this->once()) + ->method('tryEnableProviderFor') + ->with('totp', $user) + ->willReturn(true); + + $rc = $this->command->execute([ + 'uid' => 'belle', + 'provider_id' => 'totp', + ]); + + $this->assertEquals(0, $rc); + $this->assertContains("Two-factor provider totp enabled for user belle", $this->command->getDisplay()); + } + } diff --git a/tests/Core/Command/TwoFactorAuth/StateTest.php b/tests/Core/Command/TwoFactorAuth/StateTest.php new file mode 100644 index 0000000000..580e137fe3 --- /dev/null +++ b/tests/Core/Command/TwoFactorAuth/StateTest.php @@ -0,0 +1,113 @@ + + * + * @author 2018 Christoph Wurst + * + * @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 Core\Command\TwoFactorAuth; + +use OC\Core\Command\TwoFactorAuth\State; +use OCP\Authentication\TwoFactorAuth\IRegistry; +use OCP\IUser; +use OCP\IUserManager; +use PHPUnit\Framework\MockObject\MockObject; +use Symfony\Component\Console\Tester\CommandTester; +use Test\TestCase; + +class StateTest extends TestCase { + + /** @var IRegistry|MockObject */ + private $registry; + + /** @var IUserManager|MockObject */ + private $userManager; + + /** @var CommandTester|MockObject */ + private $cmd; + + protected function setUp() { + parent::setUp(); + + $this->registry = $this->createMock(IRegistry::class); + $this->userManager = $this->createMock(IUserManager::class); + + $cmd = new State($this->registry, $this->userManager); + $this->cmd = new CommandTester($cmd); + } + + public function testWrongUID() { + $this->cmd->execute([ + 'uid' => 'nope', + ]); + + $output = $this->cmd->getDisplay(); + $this->assertContains("Invalid UID", $output); + } + + public function testStateNoProvidersActive() { + $user = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('get') + ->with('eldora') + ->willReturn($user); + $states = [ + 'u2f' => false, + 'totp' => false, + ]; + $this->registry->expects($this->once()) + ->method('getProviderStates') + ->with($user) + ->willReturn($states); + + $this->cmd->execute([ + 'uid' => 'eldora', + ]); + + $output = $this->cmd->getDisplay(); + $this->assertContains("Two-factor authentication is not enabled for user eldora", $output); + } + + public function testStateOneProviderActive() { + $user = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('get') + ->with('mohamed') + ->willReturn($user); + $states = [ + 'u2f' => true, + 'totp' => false, + ]; + $this->registry->expects($this->once()) + ->method('getProviderStates') + ->with($user) + ->willReturn($states); + + $this->cmd->execute([ + 'uid' => 'mohamed', + ]); + + $output = $this->cmd->getDisplay(); + $this->assertContains("Two-factor authentication is enabled for user mohamed", $output); + } + +} diff --git a/tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php b/tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php index b46bce719f..2402fcf9f7 100644 --- a/tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/Db/ProviderUserAssignmentDaoTest.php @@ -131,4 +131,18 @@ class ProviderUserAssignmentDaoTest extends TestCase { $this->assertCount(1, $data); } + public function testDeleteAll() { + $this->dao->persist('twofactor_fail', 'user1', 1); + $this->dao->persist('twofactor_u2f', 'user1', 1); + $this->dao->persist('twofactor_fail', 'user2', 0); + $this->dao->persist('twofactor_u2f', 'user1', 0); + + $this->dao->deleteAll('twofactor_fail'); + + $statesUser1 = $this->dao->getState('user1'); + $statesUser2 = $this->dao->getState('user2'); + $this->assertCount(1, $statesUser1); + $this->assertCount(0, $statesUser2); + } + } diff --git a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php index 1d7c147d9c..301b4cc09d 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -143,13 +143,6 @@ class ManagerTest extends TestCase { } public function testIsTwoFactorAuthenticatedNoProviders() { - $this->user->expects($this->once()) - ->method('getUID') - ->will($this->returnValue('user123')); - $this->config->expects($this->once()) - ->method('getUserValue') - ->with('user123', 'core', 'two_factor_auth_disabled', 0) - ->willReturn(0); $this->providerRegistry->expects($this->once()) ->method('getProviderStates') ->willReturn([]); // No providers registered @@ -161,13 +154,6 @@ class ManagerTest extends TestCase { } public function testIsTwoFactorAuthenticatedOnlyBackupCodes() { - $this->user->expects($this->once()) - ->method('getUID') - ->will($this->returnValue('user123')); - $this->config->expects($this->once()) - ->method('getUserValue') - ->with('user123', 'core', 'two_factor_auth_disabled', 0) - ->willReturn(0); $this->providerRegistry->expects($this->once()) ->method('getProviderStates') ->willReturn([ @@ -187,13 +173,6 @@ class ManagerTest extends TestCase { } public function testIsTwoFactorAuthenticatedFailingProviders() { - $this->user->expects($this->once()) - ->method('getUID') - ->will($this->returnValue('user123')); - $this->config->expects($this->once()) - ->method('getUserValue') - ->with('user123', 'core', 'two_factor_auth_disabled', 0) - ->willReturn(0); $this->providerRegistry->expects($this->once()) ->method('getProviderStates') ->willReturn([ @@ -225,13 +204,6 @@ class ManagerTest extends TestCase { * @dataProvider providerStatesFixData */ public function testIsTwoFactorAuthenticatedFixesProviderStates(bool $providerEnabled, bool $expected) { - $this->user->expects($this->once()) - ->method('getUID') - ->will($this->returnValue('user123')); - $this->config->expects($this->once()) - ->method('getUserValue') - ->with('user123', 'core', 'two_factor_auth_disabled', 0) - ->willReturn(0); $this->providerRegistry->expects($this->once()) ->method('getProviderStates') ->willReturn([]); // Nothing registered yet diff --git a/tests/lib/Authentication/TwoFactorAuth/ProviderManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ProviderManagerTest.php new file mode 100644 index 0000000000..736dfdb913 --- /dev/null +++ b/tests/lib/Authentication/TwoFactorAuth/ProviderManagerTest.php @@ -0,0 +1,154 @@ + + * + * @author 2018 Christoph Wurst + * + * @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 lib\Authentication\TwoFactorAuth; + +use OC\Authentication\TwoFactorAuth\ProviderLoader; +use OC\Authentication\TwoFactorAuth\ProviderManager; +use OCP\Authentication\TwoFactorAuth\IActivatableByAdmin; +use OCP\Authentication\TwoFactorAuth\IDeactivatableByAdmin; +use OCP\Authentication\TwoFactorAuth\IProvider; +use OCP\Authentication\TwoFactorAuth\IRegistry; +use OCP\IUser; +use PHPUnit\Framework\MockObject\MockObject; +use Test\TestCase; + +class ProviderManagerTest extends TestCase { + + /** @var ProviderLoader|MockObject */ + private $providerLoader; + + /** @var IRegistry|MockObject */ + private $registry; + + /** @var ProviderManager */ + private $providerManager; + + protected function setUp() { + parent::setUp(); + + $this->providerLoader = $this->createMock(ProviderLoader::class); + $this->registry = $this->createMock(IRegistry::class); + + $this->providerManager = new ProviderManager( + $this->providerLoader, + $this->registry + ); + } + + /** + * @expectedException \OC\Authentication\Exceptions\InvalidProviderException + */ + public function testTryEnableInvalidProvider() { + $user = $this->createMock(IUser::class); + $this->providerManager->tryEnableProviderFor('none', $user); + } + + public function testTryEnableUnsupportedProvider() { + $user = $this->createMock(IUser::class); + $provider = $this->createMock(IProvider::class); + $this->providerLoader->expects($this->once()) + ->method('getProviders') + ->with($user) + ->willReturn([ + 'u2f' => $provider, + ]); + $this->registry->expects($this->never()) + ->method('enableProviderFor'); + + $res = $this->providerManager->tryEnableProviderFor('u2f', $user); + + $this->assertFalse($res); + } + + public function testTryEnableProvider() { + $user = $this->createMock(IUser::class); + $provider = $this->createMock(IActivatableByAdmin::class); + $this->providerLoader->expects($this->once()) + ->method('getProviders') + ->with($user) + ->willReturn([ + 'u2f' => $provider, + ]); + $provider->expects($this->once()) + ->method('enableFor') + ->with($user); + $this->registry->expects($this->once()) + ->method('enableProviderFor') + ->with($provider, $user); + + $res = $this->providerManager->tryEnableProviderFor('u2f', $user); + + $this->assertTrue($res); + } + + /** + * @expectedException \OC\Authentication\Exceptions\InvalidProviderException + */ + public function testTryDisableInvalidProvider() { + $user = $this->createMock(IUser::class); + $this->providerManager->tryDisableProviderFor('none', $user); + } + + public function testTryDisableUnsupportedProvider() { + $user = $this->createMock(IUser::class); + $provider = $this->createMock(IProvider::class); + $this->providerLoader->expects($this->once()) + ->method('getProviders') + ->with($user) + ->willReturn([ + 'u2f' => $provider, + ]); + $this->registry->expects($this->never()) + ->method('disableProviderFor'); + + $res = $this->providerManager->tryDisableProviderFor('u2f', $user); + + $this->assertFalse($res); + } + + public function testTryDisableProvider() { + $user = $this->createMock(IUser::class); + $provider = $this->createMock(IDeactivatableByAdmin::class); + $this->providerLoader->expects($this->once()) + ->method('getProviders') + ->with($user) + ->willReturn([ + 'u2f' => $provider, + ]); + $provider->expects($this->once()) + ->method('disableFor') + ->with($user); + $this->registry->expects($this->once()) + ->method('disableProviderFor') + ->with($provider, $user); + + $res = $this->providerManager->tryDisableProviderFor('u2f', $user); + + $this->assertTrue($res); + } + +} diff --git a/tests/lib/Authentication/TwoFactorAuth/RegistryTest.php b/tests/lib/Authentication/TwoFactorAuth/RegistryTest.php index 71f104ca42..3d2941e009 100644 --- a/tests/lib/Authentication/TwoFactorAuth/RegistryTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/RegistryTest.php @@ -82,4 +82,12 @@ class RegistryTest extends TestCase { $this->registry->disableProviderFor($provider, $user); } + public function testCleanUp() { + $this->dao->expects($this->once()) + ->method('deleteAll') + ->with('twofactor_u2f'); + + $this->registry->cleanUp('twofactor_u2f'); + } + }