From eddd135f14bc0d5d843b3c0ce7b011b603862ea0 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 17 Jan 2018 14:50:27 +0100 Subject: [PATCH 1/5] Dispatch event on twofactor failure and success Signed-off-by: Roeland Jago Douma --- .../Authentication/TwoFactorAuth/Manager.php | 43 +++++++++++++------ lib/private/Server.php | 3 +- .../TwoFactorAuth/IProvider.php | 6 +++ 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php index d527359b2f..933b86ffda 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -1,4 +1,5 @@ appManager = $appManager; $this->session = $session; $this->config = $config; @@ -93,6 +101,7 @@ class Manager { $this->logger = $logger; $this->tokenProvider = $tokenProvider; $this->timeFactory = $timeFactory; + $this->dispatcher = $eventDispatcher; } /** @@ -101,9 +110,9 @@ class Manager { * @param IUser $user * @return boolean */ - public function isTwoFactorAuthenticated(IUser $user) { + public function isTwoFactorAuthenticated(IUser $user): bool { $twoFactorEnabled = ((int) $this->config->getUserValue($user->getUID(), 'core', 'two_factor_auth_disabled', 0)) === 0; - return $twoFactorEnabled && count($this->getProviders($user)) > 0; + return $twoFactorEnabled && \count($this->getProviders($user)) > 0; } /** @@ -131,9 +140,9 @@ class Manager { * @param string $challengeProviderId * @return IProvider|null */ - public function getProvider(IUser $user, $challengeProviderId) { + public function getProvider(IUser $user, string $challengeProviderId) { $providers = $this->getProviders($user, true); - return isset($providers[$challengeProviderId]) ? $providers[$challengeProviderId] : null; + return $providers[$challengeProviderId] ?? null; } /** @@ -156,7 +165,7 @@ class Manager { * @return IProvider[] * @throws Exception */ - public function getProviders(IUser $user, $includeBackupApp = false) { + public function getProviders(IUser $user, bool $includeBackupApp = false): array { $allApps = $this->appManager->getEnabledAppsForUser($user); $providers = []; @@ -167,6 +176,7 @@ class Manager { $info = $this->appManager->getAppInfo($appId); if (isset($info['two-factor-providers'])) { + /** @var string[] $providerClasses */ $providerClasses = $info['two-factor-providers']; foreach ($providerClasses as $class) { try { @@ -192,7 +202,7 @@ class Manager { * * @param string $appId */ - protected function loadTwoFactorApp($appId) { + protected function loadTwoFactorApp(string $appId) { if (!OC_App::isAppLoaded($appId)) { OC_App::loadApp($appId); } @@ -206,9 +216,9 @@ class Manager { * @param string $challenge * @return boolean */ - public function verifyChallenge($providerId, IUser $user, $challenge) { + public function verifyChallenge(string $providerId, IUser $user, string $challenge): bool { $provider = $this->getProvider($user, $providerId); - if (is_null($provider)) { + if ($provider === null) { return false; } @@ -228,10 +238,16 @@ class Manager { $tokenId = $token->getId(); $this->config->deleteUserValue($user->getUID(), 'login_token_2fa', $tokenId); + $dispatchEvent = new GenericEvent($user, ['provider' => $provider->getDisplayName()]); + $this->dispatcher->dispatch(IProvider::EVENT_SUCCESS, $dispatchEvent); + $this->publishEvent($user, 'twofactor_success', [ 'provider' => $provider->getDisplayName(), ]); } else { + $dispatchEvent = new GenericEvent($user, ['provider' => $provider->getDisplayName()]); + $this->dispatcher->dispatch(IProvider::EVENT_FAILED, $dispatchEvent); + $this->publishEvent($user, 'twofactor_failed', [ 'provider' => $provider->getDisplayName(), ]); @@ -244,8 +260,9 @@ class Manager { * * @param IUser $user * @param string $event + * @param array $params */ - private function publishEvent(IUser $user, $event, array $params) { + private function publishEvent(IUser $user, string $event, array $params) { $activity = $this->activityManager->generateEvent(); $activity->setApp('core') ->setType('security') @@ -266,7 +283,7 @@ class Manager { * @param IUser $user the currently logged in user * @return boolean */ - public function needsSecondFactor(IUser $user = null) { + public function needsSecondFactor(IUser $user = null): bool { if ($user === null) { return false; } @@ -295,7 +312,7 @@ class Manager { $tokenId = $token->getId(); $tokensNeeding2FA = $this->config->getUserKeys($user->getUID(), 'login_token_2fa'); - if (!in_array($tokenId, $tokensNeeding2FA, true)) { + if (!\in_array($tokenId, $tokensNeeding2FA, true)) { $this->session->set(self::SESSION_UID_DONE, $user->getUID()); return false; } @@ -326,7 +343,7 @@ class Manager { * @param IUser $user * @param boolean $rememberMe */ - public function prepareTwoFactorLogin(IUser $user, $rememberMe) { + public function prepareTwoFactorLogin(IUser $user, bool $rememberMe) { $this->session->set(self::SESSION_UID_KEY, $user->getUID()); $this->session->set(self::REMEMBER_LOGIN, $rememberMe); diff --git a/lib/private/Server.php b/lib/private/Server.php index f8257cd980..c0fb96dce8 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -420,7 +420,8 @@ class Server extends ServerContainer implements IServerContainer { $c->getActivityManager(), $c->getLogger(), $c->query(\OC\Authentication\Token\IProvider::class), - $c->query(ITimeFactory::class) + $c->query(ITimeFactory::class), + $c->query(EventDispatcherInterface::class) ); }); diff --git a/lib/public/Authentication/TwoFactorAuth/IProvider.php b/lib/public/Authentication/TwoFactorAuth/IProvider.php index 51b126426c..c4c481a2f3 100644 --- a/lib/public/Authentication/TwoFactorAuth/IProvider.php +++ b/lib/public/Authentication/TwoFactorAuth/IProvider.php @@ -30,6 +30,12 @@ use OCP\Template; */ interface IProvider { + /** + * @since 14.0.0 + */ + const EVENT_SUCCESS = self::class . '::success'; + const EVENT_FAILED = self::class . '::failed'; + /** * Get unique identifier of this 2FA provider * From b2ca1d65532a49d13d1727ea837ac13e4f8bfcd6 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 17 Jan 2018 14:51:03 +0100 Subject: [PATCH 2/5] Make admin_audit listen to 2fa events Signed-off-by: Roeland Jago Douma --- apps/admin_audit/lib/Actions/Security.php | 75 ++++++++++++++++++++ apps/admin_audit/lib/AppInfo/Application.php | 16 +++++ 2 files changed, 91 insertions(+) create mode 100644 apps/admin_audit/lib/Actions/Security.php diff --git a/apps/admin_audit/lib/Actions/Security.php b/apps/admin_audit/lib/Actions/Security.php new file mode 100644 index 0000000000..4e631aeddd --- /dev/null +++ b/apps/admin_audit/lib/Actions/Security.php @@ -0,0 +1,75 @@ + + * + * @author Roeland Jago Douma + * + * @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 OCA\AdminAudit\Actions; +use OCP\IUser; + +/** + * Class Sharing logs the sharing actions + * + * @package OCA\AdminAudit\Actions + */ +class Security extends Action { + /** + * Log twofactor auth enabled + * + * @param IUser $user + * @param array $params + */ + public function twofactorFailed(IUser $user, array $params) { + $params['uid'] = $user->getUID(); + $params['displayName'] = $user->getDisplayName(); + + $this->log( + 'Failed two factor attempt by user %s (%s) with provider %s', + $params, + [ + 'displayname', + 'uid', + 'provider', + ] + ); + } + + /** + * Logs unsharing of data + * + * @param IUser $user + * @param array $params + */ + public function twofactorSuccess(IUser $user, array $params) { + $params['uid'] = $user->getUID(); + $params['displayName'] = $user->getDisplayName(); + + $this->log( + 'Successful two factor attempt by user %s (%s) with provider %s', + $params, + [ + 'displayname', + 'uid', + 'provider', + ] + ); + } +} diff --git a/apps/admin_audit/lib/AppInfo/Application.php b/apps/admin_audit/lib/AppInfo/Application.php index d3ae4ad26c..470352f895 100644 --- a/apps/admin_audit/lib/AppInfo/Application.php +++ b/apps/admin_audit/lib/AppInfo/Application.php @@ -33,12 +33,14 @@ use OCA\AdminAudit\Actions\Auth; use OCA\AdminAudit\Actions\Console; use OCA\AdminAudit\Actions\Files; use OCA\AdminAudit\Actions\GroupManagement; +use OCA\AdminAudit\Actions\Security; use OCA\AdminAudit\Actions\Sharing; use OCA\AdminAudit\Actions\Trashbin; use OCA\AdminAudit\Actions\UserManagement; use OCA\AdminAudit\Actions\Versions; use OCP\App\ManagerEvent; use OCP\AppFramework\App; +use OCP\Authentication\TwoFactorAuth\IProvider; use OCP\Console\ConsoleEvent; use OCP\IGroupManager; use OCP\ILogger; @@ -75,6 +77,8 @@ class Application extends App { $this->fileHooks($logger); $this->trashbinHooks($logger); $this->versionsHooks($logger); + + $this->securityHooks($logger); } protected function userManagementHooks(ILogger $logger) { @@ -218,4 +222,16 @@ class Application extends App { Util::connectHook('\OCP\Trashbin', 'preDelete', $trashActions, 'delete'); Util::connectHook('\OCA\Files_Trashbin\Trashbin', 'post_restore', $trashActions, 'restore'); } + + protected function securityHooks(ILogger $logger) { + $eventDispatcher = $this->getContainer()->getServer()->getEventDispatcher(); + $eventDispatcher->addListener(IProvider::EVENT_SUCCESS, function(GenericEvent $event) use ($logger) { + $security = new Security($logger); + $security->twofactorSuccess($event->getSubject(), $event->getArguments()); + }); + $eventDispatcher->addListener(IProvider::EVENT_FAILED, function(GenericEvent $event) use ($logger) { + $security = new Security($logger); + $security->twofactorFailed($event->getSubject(), $event->getArguments()); + }); + } } From c92eff919ea1502d4387ec6deb33956ce6390537 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 24 Jan 2018 08:48:28 +0100 Subject: [PATCH 3/5] Fix tests Signed-off-by: Roeland Jago Douma --- .../Authentication/TwoFactorAuth/ManagerTest.php | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php index 9db27edd70..8afd165f32 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -35,6 +35,7 @@ use OCP\IConfig; use OCP\ILogger; use OCP\ISession; use OCP\IUser; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Test\TestCase; class ManagerTest extends TestCase { @@ -72,6 +73,9 @@ class ManagerTest extends TestCase { /** @var ITimeFactory|\PHPUnit_Framework_MockObject_MockObject */ private $timeFactory; + /** @var EventDispatcherInterface|\PHPUnit_Framework_MockObject_MockObject */ + private $eventDispatcher; + protected function setUp() { parent::setUp(); @@ -83,6 +87,7 @@ class ManagerTest extends TestCase { $this->logger = $this->createMock(ILogger::class); $this->tokenProvider = $this->createMock(TokenProvider::class); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->eventDispatcher = $this->createMock(EventDispatcherInterface::class); $this->manager = $this->getMockBuilder(Manager::class) ->setConstructorArgs([ @@ -92,7 +97,8 @@ class ManagerTest extends TestCase { $this->activityManager, $this->logger, $this->tokenProvider, - $this->timeFactory + $this->timeFactory, + $this->eventDispatcher ]) ->setMethods(['loadTwoFactorApp']) // Do not actually load the apps ->getMock(); @@ -301,7 +307,7 @@ class ManagerTest extends TestCase { ->method('setAffectedUser') ->with($this->equalTo('jos')) ->willReturnSelf(); - $this->fakeProvider->expects($this->once()) + $this->fakeProvider ->method('getDisplayName') ->willReturn('Fake 2FA'); $event->expects($this->once()) @@ -371,7 +377,7 @@ class ManagerTest extends TestCase { ->method('setAffectedUser') ->with($this->equalTo('jos')) ->willReturnSelf(); - $this->fakeProvider->expects($this->once()) + $this->fakeProvider ->method('getDisplayName') ->willReturn('Fake 2FA'); $event->expects($this->once()) @@ -424,7 +430,8 @@ class ManagerTest extends TestCase { $this->activityManager, $this->logger, $this->tokenProvider, - $this->timeFactory + $this->timeFactory, + $this->eventDispatcher ]) ->setMethods(['loadTwoFactorApp','isTwoFactorAuthenticated']) // Do not actually load the apps ->getMock(); From a5fe6a6118cac53b66dd6b210227622d10c639f6 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 24 Jan 2018 09:10:16 +0100 Subject: [PATCH 4/5] Update autoloader Signed-off-by: Roeland Jago Douma --- apps/admin_audit/composer/composer/autoload_classmap.php | 1 + apps/admin_audit/composer/composer/autoload_static.php | 1 + 2 files changed, 2 insertions(+) diff --git a/apps/admin_audit/composer/composer/autoload_classmap.php b/apps/admin_audit/composer/composer/autoload_classmap.php index 487e05172d..c08200c7c2 100644 --- a/apps/admin_audit/composer/composer/autoload_classmap.php +++ b/apps/admin_audit/composer/composer/autoload_classmap.php @@ -12,6 +12,7 @@ return array( 'OCA\\AdminAudit\\Actions\\Console' => $baseDir . '/../lib/Actions/Console.php', 'OCA\\AdminAudit\\Actions\\Files' => $baseDir . '/../lib/Actions/Files.php', 'OCA\\AdminAudit\\Actions\\GroupManagement' => $baseDir . '/../lib/Actions/GroupManagement.php', + 'OCA\\AdminAudit\\Actions\\Security' => $baseDir . '/../lib/Actions/Security.php', 'OCA\\AdminAudit\\Actions\\Sharing' => $baseDir . '/../lib/Actions/Sharing.php', 'OCA\\AdminAudit\\Actions\\Trashbin' => $baseDir . '/../lib/Actions/Trashbin.php', 'OCA\\AdminAudit\\Actions\\UserManagement' => $baseDir . '/../lib/Actions/UserManagement.php', diff --git a/apps/admin_audit/composer/composer/autoload_static.php b/apps/admin_audit/composer/composer/autoload_static.php index b5f055de44..ef088bd22d 100644 --- a/apps/admin_audit/composer/composer/autoload_static.php +++ b/apps/admin_audit/composer/composer/autoload_static.php @@ -27,6 +27,7 @@ class ComposerStaticInitAdminAudit 'OCA\\AdminAudit\\Actions\\Console' => __DIR__ . '/..' . '/../lib/Actions/Console.php', 'OCA\\AdminAudit\\Actions\\Files' => __DIR__ . '/..' . '/../lib/Actions/Files.php', 'OCA\\AdminAudit\\Actions\\GroupManagement' => __DIR__ . '/..' . '/../lib/Actions/GroupManagement.php', + 'OCA\\AdminAudit\\Actions\\Security' => __DIR__ . '/..' . '/../lib/Actions/Security.php', 'OCA\\AdminAudit\\Actions\\Sharing' => __DIR__ . '/..' . '/../lib/Actions/Sharing.php', 'OCA\\AdminAudit\\Actions\\Trashbin' => __DIR__ . '/..' . '/../lib/Actions/Trashbin.php', 'OCA\\AdminAudit\\Actions\\UserManagement' => __DIR__ . '/..' . '/../lib/Actions/UserManagement.php', From 9e76577ead00471d556c4fcf3534fd17c5b21fec Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 25 Jan 2018 13:44:47 +0100 Subject: [PATCH 5/5] Add tests Signed-off-by: Roeland Jago Douma --- apps/admin_audit/lib/Actions/Security.php | 4 +- .../tests/Actions/SecurityTest.php | 75 +++++++++++++++++++ tests/enable_all.php | 1 + tests/phpunit-autotest.xml | 1 + 4 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 apps/admin_audit/tests/Actions/SecurityTest.php diff --git a/apps/admin_audit/lib/Actions/Security.php b/apps/admin_audit/lib/Actions/Security.php index 4e631aeddd..b7ef1332f3 100644 --- a/apps/admin_audit/lib/Actions/Security.php +++ b/apps/admin_audit/lib/Actions/Security.php @@ -45,7 +45,7 @@ class Security extends Action { 'Failed two factor attempt by user %s (%s) with provider %s', $params, [ - 'displayname', + 'displayName', 'uid', 'provider', ] @@ -66,7 +66,7 @@ class Security extends Action { 'Successful two factor attempt by user %s (%s) with provider %s', $params, [ - 'displayname', + 'displayName', 'uid', 'provider', ] diff --git a/apps/admin_audit/tests/Actions/SecurityTest.php b/apps/admin_audit/tests/Actions/SecurityTest.php new file mode 100644 index 0000000000..3a3f25933f --- /dev/null +++ b/apps/admin_audit/tests/Actions/SecurityTest.php @@ -0,0 +1,75 @@ + + * + * @author Roeland Jago Douma + * + * @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 OCA\AdminAudit\Tests\Actions; + +use OCA\AdminAudit\Actions\Security; +use OCP\ILogger; +use OCP\IUser; +use Test\TestCase; + +class SecurityTest extends TestCase { + /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */ + private $logger; + + /** @var Security */ + private $security; + + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject */ + private $user; + + public function setUp() { + parent::setUp(); + + $this->logger = $this->createMock(ILogger::class); + $this->security = new Security($this->logger); + + $this->user = $this->createMock(IUser::class); + $this->user->method('getUID')->willReturn('myuid'); + $this->user->method('getDisplayName')->willReturn('mydisplayname'); + } + + public function testTwofactorFailed() { + $this->logger->expects($this->once()) + ->method('info') + ->with( + $this->equalTo('Failed two factor attempt by user mydisplayname (myuid) with provider myprovider'), + ['app' => 'admin_audit'] + ); + + $this->security->twofactorFailed($this->user, ['provider' => 'myprovider']); + } + + public function testTwofactorSuccess() { + $this->logger->expects($this->once()) + ->method('info') + ->with( + $this->equalTo('Successful two factor attempt by user mydisplayname (myuid) with provider myprovider'), + ['app' => 'admin_audit'] + ); + + $this->security->twofactorSuccess($this->user, ['provider' => 'myprovider']); + } + +} diff --git a/tests/enable_all.php b/tests/enable_all.php index 655597be7c..c494f3e0d5 100644 --- a/tests/enable_all.php +++ b/tests/enable_all.php @@ -24,3 +24,4 @@ enableApp('files_versions'); enableApp('provisioning_api'); enableApp('federation'); enableApp('federatedfilesharing'); +enableApp('admin_audit'); diff --git a/tests/phpunit-autotest.xml b/tests/phpunit-autotest.xml index 34166a09e2..5712838f6b 100644 --- a/tests/phpunit-autotest.xml +++ b/tests/phpunit-autotest.xml @@ -21,6 +21,7 @@ .. ../3rdparty + ../apps/admin_audit/tests ../apps/dav/tests ../apps/encryption/tests ../apps/federatedfilesharing/tests