Merge pull request #2849 from nextcloud/downstream-add-two-factor-exception

Downstream add two factor exception

core/Controller/TwoFactorChallengeController.php

@@ -29,6 +29,7 @@ use OC_Util;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\Authentication\TwoFactorAuth\TwoFactorException;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IURLGenerator;
@@ -115,16 +116,19 @@ class TwoFactorChallengeController extends Controller {
 			$backupProvider = null;
 		}
 
+		$errorMessage = '';
+		$error = false;
 		if ($this->session->exists('two_factor_auth_error')) {
 			$this->session->remove('two_factor_auth_error');
 			$error = true;
-		} else {
-			$error = false;
+			$errorMessage = $this->session->get("two_factor_auth_error_message");
+			$this->session->remove('two_factor_auth_error_message');
 		}
 		$tmpl = $provider->getTemplate($user);
 		$tmpl->assign('redirect_url', $redirect_url);
 		$data = [
 			'error' => $error,
+			'error_message' => $errorMessage,
 			'provider' => $provider,
 			'backupProvider' => $backupProvider,
 			'logout_attribute' => $this->getLogoutAttribute(),
@@ -151,11 +155,20 @@ class TwoFactorChallengeController extends Controller {
 			return new RedirectResponse($this->urlGenerator->linkToRoute('core.TwoFactorChallenge.selectChallenge'));
 		}
 
-		if ($this->twoFactorManager->verifyChallenge($challengeProviderId, $user, $challenge)) {
-			if (!is_null($redirect_url)) {
-				return new RedirectResponse($this->urlGenerator->getAbsoluteURL(urldecode($redirect_url)));
+		try {
+			if ($this->twoFactorManager->verifyChallenge($challengeProviderId, $user, $challenge)) {
+				if (!is_null($redirect_url)) {
+					return new RedirectResponse($this->urlGenerator->getAbsoluteURL(urldecode($redirect_url)));
+				}
+				return new RedirectResponse(OC_Util::getDefaultPageUrl());
 			}
-			return new RedirectResponse(OC_Util::getDefaultPageUrl());
+		} catch (TwoFactorException $e) {
+			/*
+			 * The 2FA App threw an TwoFactorException. Now we display more
+			 * information to the user. The exception text is stored in the
+			 * session to be used in showChallenge()
+			 */
+			$this->session->set('two_factor_auth_error_message', $e->getMessage());
 		}
 
 		$this->session->set('two_factor_auth_error', true);

core/templates/twofactorshowchallenge.php

@@ -3,6 +3,8 @@
 /** @var $_ array */
 /* @var $error boolean */
 $error = $_['error'];
+/* @var $error_message string */
+$error_message = $_['error_message'];
 /* @var $provider OCP\Authentication\TwoFactorAuth\IProvider */
 $provider = $_['provider'];
 /* @var $template string */
@@ -12,7 +14,11 @@ $template = $_['template'];
 <div class="warning">
 		<h2 class="two-factor-header"><?php p($provider->getDisplayName()); ?></h2>
 		<?php if ($error): ?>
-		<p><strong><?php p($l->t('Error while validating your second factor')); ?></strong></p>
+			<?php if($error_message): ?>
+				<p><strong><?php p($error_message); ?></strong></p>
+			<?php else: ?>
+				<p><strong><?php p($l->t('Error while validating your second factor')); ?></strong></p>
+			<?php endif; ?>
 		<?php endif; ?>
 		<?php print_unescaped($template); ?>
 </div>

lib/composer/composer/autoload_classmap.php

@@ -56,6 +56,7 @@ return array(
     'OCP\\App\\ManagerEvent' => $baseDir . '/lib/public/App/ManagerEvent.php',
     'OCP\\Authentication\\IApacheBackend' => $baseDir . '/lib/public/Authentication/IApacheBackend.php',
     'OCP\\Authentication\\TwoFactorAuth\\IProvider' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/IProvider.php',
+    'OCP\\Authentication\\TwoFactorAuth\\TwoFactorException' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php',
     'OCP\\AutoloadNotAllowedException' => $baseDir . '/lib/public/AutoloadNotAllowedException.php',
     'OCP\\BackgroundJob' => $baseDir . '/lib/public/BackgroundJob.php',
     'OCP\\BackgroundJob\\IJob' => $baseDir . '/lib/public/BackgroundJob/IJob.php',

lib/composer/composer/autoload_static.php

@@ -86,6 +86,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OCP\\App\\ManagerEvent' => __DIR__ . '/../../..' . '/lib/public/App/ManagerEvent.php',
         'OCP\\Authentication\\IApacheBackend' => __DIR__ . '/../../..' . '/lib/public/Authentication/IApacheBackend.php',
         'OCP\\Authentication\\TwoFactorAuth\\IProvider' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/IProvider.php',
+        'OCP\\Authentication\\TwoFactorAuth\\TwoFactorException' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php',
         'OCP\\AutoloadNotAllowedException' => __DIR__ . '/../../..' . '/lib/public/AutoloadNotAllowedException.php',
         'OCP\\BackgroundJob' => __DIR__ . '/../../..' . '/lib/public/BackgroundJob.php',
         'OCP\\BackgroundJob\\IJob' => __DIR__ . '/../../..' . '/lib/public/BackgroundJob/IJob.php',

lib/public/Authentication/TwoFactorAuth/TwoFactorException.php

@@ -0,0 +1,38 @@
+<?php
+
+/**
+ * @author Cornelius Kölbel <cornelius.koelbel@netknights.it>
+ * @copyright Copyright (c) 2016, ownCloud GmbH.
+ *
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * 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, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCP\Authentication\TwoFactorAuth;
+
+use Exception;
+
+/**
+ * Two Factor Authentication failed
+ *
+ * It defines an Exception a 2FA app can
+ * throw in case of an error. The 2FA Controller will catch this exception and
+ * display this error.
+ *
+ * @since 12
+ */
+class TwoFactorException extends Exception {
+
+}

tests/Core/Controller/TwoFactorChallengeControllerTest.php

@@ -22,32 +22,52 @@
 
 namespace Test\Core\Controller;
 
+use OC\Authentication\TwoFactorAuth\Manager;
 use OC\Core\Controller\TwoFactorChallengeController;
+use OC_Util;
+use OCP\AppFramework\Http\RedirectResponse;
+use OCP\AppFramework\Http\TemplateResponse;
+use OCP\Authentication\TwoFactorAuth\IProvider;
+use OCP\Authentication\TwoFactorAuth\TwoFactorException;
+use OCP\IRequest;
+use OCP\ISession;
+use OCP\IURLGenerator;
+use OCP\IUser;
+use OCP\IUserSession;
+use OCP\Template;
+use PHPUnit_Framework_MockObject_MockObject;
 use Test\TestCase;
 
 class TwoFactorChallengeControllerTest extends TestCase {
 
+	/** @var IRequest|PHPUnit_Framework_MockObject_MockObject */
 	private $request;
+
+	/** @var Manager|PHPUnit_Framework_MockObject_MockObject */
 	private $twoFactorManager;
+
+	/** @var IUserSession|PHPUnit_Framework_MockObject_MockObject */
 	private $userSession;
+
+	/** @var ISession|PHPUnit_Framework_MockObject_MockObject */
 	private $session;
+
+	/** @var IURLGenerator|PHPUnit_Framework_MockObject_MockObject */
 	private $urlGenerator;
 
-	/** @var TwoFactorChallengeController|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var TwoFactorChallengeController|PHPUnit_Framework_MockObject_MockObject */
 	private $controller;
 
 	protected function setUp() {
 		parent::setUp();
 
-		$this->request = $this->getMockBuilder('\OCP\IRequest')->getMock();
-		$this->twoFactorManager = $this->getMockBuilder('\OC\Authentication\TwoFactorAuth\Manager')
-			->disableOriginalConstructor()
-			->getMock();
-		$this->userSession = $this->getMockBuilder('\OCP\IUserSession')->getMock();
-		$this->session = $this->getMockBuilder('\OCP\ISession')->getMock();
-		$this->urlGenerator = $this->getMockBuilder('\OCP\IURLGenerator')->getMock();
+		$this->request = $this->createMock(IRequest::class);
+		$this->twoFactorManager = $this->createMock(Manager::class);
+		$this->userSession = $this->createMock(IUserSession::class);
+		$this->session = $this->createMock(ISession::class);
+		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 
-		$this->controller = $this->getMockBuilder('OC\Core\Controller\TwoFactorChallengeController')
+		$this->controller = $this->getMockBuilder(TwoFactorChallengeController::class)
 			->setConstructorArgs([
 				'core',
 				$this->request,
@@ -64,7 +84,7 @@ class TwoFactorChallengeControllerTest extends TestCase {
 	}
 
 	public function testSelectChallenge() {
-		$user = $this->getMockBuilder('\OCP\IUser')->getMock();
+		$user = $this->getMockBuilder(IUser::class)->getMock();
 		$providers = [
 			'prov1',
 			'prov2',
@@ -82,27 +102,21 @@ class TwoFactorChallengeControllerTest extends TestCase {
 			->with($user)
 			->will($this->returnValue('backup'));
 
-		$expected = new \OCP\AppFramework\Http\TemplateResponse('core', 'twofactorselectchallenge', [
+		$expected = new TemplateResponse('core', 'twofactorselectchallenge', [
 			'providers' => $providers,
 			'backupProvider' => 'backup',
 			'redirect_url' => '/some/url',
 			'logout_attribute' => 'logoutAttribute',
-		], 'guest');
+			], 'guest');
 
 		$this->assertEquals($expected, $this->controller->selectChallenge('/some/url'));
 	}
 
 	public function testShowChallenge() {
-		$user = $this->getMockBuilder('\OCP\IUser')->getMock();
-		$provider = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')
-			->disableOriginalConstructor()
-			->getMock();
-		$backupProvider = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')
-			->disableOriginalConstructor()
-			->getMock();
-		$tmpl = $this->getMockBuilder('\OCP\Template')
-			->disableOriginalConstructor()
-			->getMock();
+		$user = $this->createMock(IUser::class);
+		$provider = $this->createMock(IProvider::class);
+		$backupProvider = $this->createMock(IProvider::class);
+		$tmpl = $this->createMock(Template::class);
 
 		$this->userSession->expects($this->once())
 			->method('getUser')
@@ -126,9 +140,9 @@ class TwoFactorChallengeControllerTest extends TestCase {
 			->method('exists')
 			->with('two_factor_auth_error')
 			->will($this->returnValue(true));
-		$this->session->expects($this->once())
+		$this->session->expects($this->exactly(2))
 			->method('remove')
-			->with('two_factor_auth_error');
+			->with($this->logicalOr($this->equalTo('two_factor_auth_error'), $this->equalTo('two_factor_auth_error_message')));
 		$provider->expects($this->once())
 			->method('getTemplate')
 			->with($user)
@@ -137,20 +151,21 @@ class TwoFactorChallengeControllerTest extends TestCase {
 			->method('fetchPage')
 			->will($this->returnValue('<html/>'));
 
-		$expected = new \OCP\AppFramework\Http\TemplateResponse('core', 'twofactorshowchallenge', [
+		$expected = new TemplateResponse('core', 'twofactorshowchallenge', [
 			'error' => true,
 			'provider' => $provider,
 			'backupProvider' => $backupProvider,
 			'logout_attribute' => 'logoutAttribute',
 			'template' => '<html/>',
 			'redirect_url' => '/re/dir/ect/url',
+			'error_message' => null,
 			], 'guest');
 
 		$this->assertEquals($expected, $this->controller->showChallenge('myprovider', '/re/dir/ect/url'));
 	}
 
 	public function testShowInvalidChallenge() {
-		$user = $this->getMockBuilder('\OCP\IUser')->getMock();
+		$user = $this->createMock(IUser::class);
 
 		$this->userSession->expects($this->once())
 			->method('getUser')
@@ -164,16 +179,14 @@ class TwoFactorChallengeControllerTest extends TestCase {
 			->with('core.TwoFactorChallenge.selectChallenge')
 			->will($this->returnValue('select/challenge/url'));
 
-		$expected = new \OCP\AppFramework\Http\RedirectResponse('select/challenge/url');
+		$expected = new RedirectResponse('select/challenge/url');
 
 		$this->assertEquals($expected, $this->controller->showChallenge('myprovider', 'redirect/url'));
 	}
 
 	public function testSolveChallenge() {
-		$user = $this->getMockBuilder('\OCP\IUser')->getMock();
-		$provider = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')
-			->disableOriginalConstructor()
-			->getMock();
+		$user = $this->createMock(IUser::class);
+		$provider = $this->createMock(IProvider::class);
 
 		$this->userSession->expects($this->once())
 			->method('getUser')
@@ -188,12 +201,37 @@ class TwoFactorChallengeControllerTest extends TestCase {
 			->with('myprovider', $user, 'token')
 			->will($this->returnValue(true));
 
-		$expected = new \OCP\AppFramework\Http\RedirectResponse(\OC_Util::getDefaultPageUrl());
+		$expected = new RedirectResponse(OC_Util::getDefaultPageUrl());
 		$this->assertEquals($expected, $this->controller->solveChallenge('myprovider', 'token'));
 	}
 
+	public function testSolveValidChallengeAndRedirect() {
+		$user = $this->createMock(IUser::class);
+		$provider = $this->createMock(IProvider::class);
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+		$this->twoFactorManager->expects($this->once())
+			->method('getProvider')
+			->with($user, 'myprovider')
+			->will($this->returnValue($provider));
+
+		$this->twoFactorManager->expects($this->once())
+			->method('verifyChallenge')
+			->with('myprovider', $user, 'token')
+			->willReturn(true);
+		$this->urlGenerator->expects($this->once())
+			->method('getAbsoluteURL')
+			->with('redirect url')
+			->willReturn('redirect/url');
+
+		$expected = new RedirectResponse('redirect/url');
+		$this->assertEquals($expected, $this->controller->solveChallenge('myprovider', 'token', 'redirect%20url'));
+	}
+
 	public function testSolveChallengeInvalidProvider() {
-		$user = $this->getMockBuilder('\OCP\IUser')->getMock();
+		$user = $this->getMockBuilder(IUser::class)->getMock();
 
 		$this->userSession->expects($this->once())
 			->method('getUser')
@@ -207,16 +245,14 @@ class TwoFactorChallengeControllerTest extends TestCase {
 			->with('core.TwoFactorChallenge.selectChallenge')
 			->will($this->returnValue('select/challenge/url'));
 
-		$expected = new \OCP\AppFramework\Http\RedirectResponse('select/challenge/url');
+		$expected = new RedirectResponse('select/challenge/url');
 
 		$this->assertEquals($expected, $this->controller->solveChallenge('myprovider', 'token'));
 	}
 
 	public function testSolveInvalidChallenge() {
-		$user = $this->getMockBuilder('\OCP\IUser')->getMock();
-		$provider = $this->getMockBuilder('\OCP\Authentication\TwoFactorAuth\IProvider')
-			->disableOriginalConstructor()
-			->getMock();
+		$user = $this->createMock(IUser::class);
+		$provider = $this->createMock(IProvider::class);
 
 		$this->userSession->expects($this->once())
 			->method('getUser')
@@ -244,7 +280,45 @@ class TwoFactorChallengeControllerTest extends TestCase {
 			->method('getId')
 			->will($this->returnValue('myprovider'));
 
-		$expected = new \OCP\AppFramework\Http\RedirectResponse('files/index/url');
+		$expected = new RedirectResponse('files/index/url');
+		$this->assertEquals($expected, $this->controller->solveChallenge('myprovider', 'token', '/url'));
+	}
+
+	public function testSolveChallengeTwoFactorException() {
+		$user = $this->createMock(IUser::class);
+		$provider = $this->createMock(IProvider::class);
+		$exception = new TwoFactorException("2FA failed");
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+		$this->twoFactorManager->expects($this->once())
+			->method('getProvider')
+			->with($user, 'myprovider')
+			->will($this->returnValue($provider));
+
+		$this->twoFactorManager->expects($this->once())
+			->method('verifyChallenge')
+			->with('myprovider', $user, 'token')
+			->will($this->throwException($exception));
+		$this->session->expects($this->at(0))
+			->method('set')
+			->with('two_factor_auth_error_message', "2FA failed");
+		$this->session->expects($this->at(1))
+			->method('set')
+			->with('two_factor_auth_error', true);
+		$this->urlGenerator->expects($this->once())
+			->method('linkToRoute')
+			->with('core.TwoFactorChallenge.showChallenge', [
+				'challengeProviderId' => 'myprovider',
+				'redirect_url' => '/url',
+			])
+			->will($this->returnValue('files/index/url'));
+		$provider->expects($this->once())
+			->method('getId')
+			->will($this->returnValue('myprovider'));
+
+		$expected = new RedirectResponse('files/index/url');
 		$this->assertEquals($expected, $this->controller->solveChallenge('myprovider', 'token', '/url'));
 	}