Merge pull request #37401 from pedro-vo/log-failed-twofactor-auth

Log failed twofactor authentication
owncloud · Jun 15, 2020 · 6038c8e · 6038c8e
2 parents 2193357 + a7f67e6
commit 6038c8e
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 3 deletions.
diff --git a/changelog/unreleased/37401 b/changelog/unreleased/37401
@@ -0,0 +1,6 @@
+Bugfix: Log failed twofactor authentication
+
+When user entered bad twofactor authentication (i.e. code) there was no message
+in application log. This change will log this failed authentication.
+
+https://github.com/owncloud/core/pull/37401
diff --git a/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php
@@ -29,6 +29,8 @@
 use OCP\AppFramework\QueryException;
 use OCP\Authentication\TwoFactorAuth\IProvider;
 use OCP\IConfig;
+use OCP\IRequest;
+use OCP\ILogger;
 use OCP\ISession;
 use OCP\IUser;
 
@@ -44,15 +46,25 @@ class Manager {
 	/** @var IConfig */
 	private $config;
 
+	/** @var IRequest */
+	private $request;
+
+	/** @var ILogger */
+	private $logger;
+
 	/**
 	 * @param AppManager $appManager
 	 * @param ISession $session
 	 * @param IConfig $config
+	 * @param IRequest $request
+	 * @param ILogger $logger
 	 */
-	public function __construct(AppManager $appManager, ISession $session, IConfig $config) {
+	public function __construct(AppManager $appManager, ISession $session, IConfig $config, IRequest $request, ILogger $logger) {
 		$this->appManager = $appManager;
 		$this->session = $session;
 		$this->config = $config;
+		$this->request = $request;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -157,6 +169,9 @@ public function verifyChallenge($providerId, IUser $user, $challenge) {
 		$result = $provider->verifyChallenge($user, $challenge);
 		if ($result) {
 			$this->session->remove(self::SESSION_UID_KEY);
+		} else {
+			$this->logger->warning("Two factor verify failed: '{$user->getUserName()}' (Remote IP: '{$this->request->getRemoteAddress()}')",
+				['app' => 'core']);
 		}
 		return $result;
 	}

diff --git a/lib/private/Server.php b/lib/private/Server.php
@@ -387,7 +387,7 @@ public function __construct($webRoot, \OC\Config $config) {
 		});
 
 		$this->registerService('\OC\Authentication\TwoFactorAuth\Manager', function (Server $c) {
-			return new \OC\Authentication\TwoFactorAuth\Manager($c->getAppManager(), $c->getSession(), $c->getConfig());
+			return new \OC\Authentication\TwoFactorAuth\Manager($c->getAppManager(), $c->getSession(), $c->getConfig(), $c->getRequest(), $c->getLogger());
 		});
 
 		$this->registerService('NavigationManager', function (Server $c) {

diff --git a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
@@ -42,6 +42,12 @@ class ManagerTest extends TestCase {
 	/** @var \OCP\IConfig|\PHPUnit\Framework\MockObject\MockObject */
 	private $config;
 
+	/** @var \OCP\IRequest | \PHPUnit\Framework\MockObject\MockObject */
+	private $request;
+
+	/** @var \OCP\ILogger | \PHPUnit\Framework\MockObject\MockObject */
+	private $logger;
+
 	/** @var \OCP\Authentication\TwoFactorAuth\IProvider|\PHPUnit\Framework\MockObject\MockObject */
 	private $fakeProvider;
 
@@ -54,9 +60,11 @@ protected function setUp(): void {
 			->getMock();
 		$this->session = $this->createMock('\OCP\ISession');
 		$this->config = $this->createMock('\OCP\IConfig');
+		$this->request = $this->createMock('\OCP\IRequest');
+		$this->logger = $this->createMock('\OCP\ILogger');
 
 		$this->manager = $this->getMockBuilder('\OC\Authentication\TwoFactorAuth\Manager')
-			->setConstructorArgs([$this->appManager, $this->session, $this->config])
+			->setConstructorArgs([$this->appManager, $this->session, $this->config, $this->request, $this->logger])
 			->setMethods(['loadTwoFactorApp']) // Do not actually load the apps
 			->getMock();