Merge pull request #7916 from nextcloud/2fa_log

Add 2FA to logfile

11 files changed, 219 insertions, 18 deletions

diff --git a/apps/admin_audit/composer/composer/autoload_classmap.php b/apps/admin_audit/composer/composer/autoload_classmap.php
index 487e05172de..c08200c7c20 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 b5f055de44e..ef088bd22d9 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',
diff --git a/apps/admin_audit/lib/Actions/Security.php b/apps/admin_audit/lib/Actions/Security.php
new file mode 100644
index 00000000000..b7ef1332f36
--- /dev/null
+++ b/apps/admin_audit/lib/Actions/Security.php
@@ -0,0 +1,75 @@
+<?php
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2018 Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @author Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+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 d3ae4ad26c1..470352f895e 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());
+		});
+	}
 }
diff --git a/apps/admin_audit/tests/Actions/SecurityTest.php b/apps/admin_audit/tests/Actions/SecurityTest.php
new file mode 100644
index 00000000000..3a3f25933f4
--- /dev/null
+++ b/apps/admin_audit/tests/Actions/SecurityTest.php
@@ -0,0 +1,75 @@
+<?php
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2018 Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @author Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+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/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php
index d527359b2ff..933b86ffda5 100644
--- a/lib/private/Authentication/TwoFactorAuth/Manager.php
+++ b/lib/private/Authentication/TwoFactorAuth/Manager.php
@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -40,6 +41,8 @@ use OCP\IConfig;
 use OCP\ILogger;
 use OCP\ISession;
 use OCP\IUser;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;
+use Symfony\Component\EventDispatcher\GenericEvent;
 
 class Manager {
 
@@ -70,6 +73,9 @@ class Manager {
 	/** @var ITimeFactory */
 	private $timeFactory;
 
+	/** @var EventDispatcherInterface */
+	private $dispatcher;
+
 	/**
 	 * @param AppManager $appManager
 	 * @param ISession $session
@@ -78,6 +84,7 @@ class Manager {
 	 * @param ILogger $logger
 	 * @param TokenProvider $tokenProvider
 	 * @param ITimeFactory $timeFactory
+	 * @param EventDispatcherInterface $eventDispatcher
 	 */
 	public function __construct(AppManager $appManager,
 								ISession $session,
@@ -85,7 +92,8 @@ class Manager {
 								IManager $activityManager,
 								ILogger $logger,
 								TokenProvider $tokenProvider,
-								ITimeFactory $timeFactory) {
+								ITimeFactory $timeFactory,
+								EventDispatcherInterface $eventDispatcher) {
 		$this->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 f8257cd9801..c0fb96dce86 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 51b126426c3..c4c481a2f32 100644
--- a/lib/public/Authentication/TwoFactorAuth/IProvider.php
+++ b/lib/public/Authentication/TwoFactorAuth/IProvider.php
@@ -31,6 +31,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
 	 *
 	 * @since 9.1.0
diff --git a/tests/enable_all.php b/tests/enable_all.php
index 655597be7c8..c494f3e0d55 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/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
index 9db27edd70c..8afd165f320 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();
diff --git a/tests/phpunit-autotest.xml b/tests/phpunit-autotest.xml
index 34166a09e2e..5712838f6bd 100644
--- a/tests/phpunit-autotest.xml
+++ b/tests/phpunit-autotest.xml
@@ -21,6 +21,7 @@
 			<directory suffix=".php">..</directory>
 			<exclude>
 				<directory suffix=".php">../3rdparty</directory>
+				<directory suffix=".php">../apps/admin_audit/tests</directory>
 				<directory suffix=".php">../apps/dav/tests</directory>
 				<directory suffix=".php">../apps/encryption/tests</directory>
 				<directory suffix=".php">../apps/federatedfilesharing/tests</directory>