diff --git a/core/Controller/TwoFactorChallengeController.php b/core/Controller/TwoFactorChallengeController.php index 34f0092bea..fd4811d3ff 100644 --- a/core/Controller/TwoFactorChallengeController.php +++ b/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); diff --git a/core/templates/twofactorshowchallenge.php b/core/templates/twofactorshowchallenge.php index 20b92be952..4f3741b5df 100644 --- a/core/templates/twofactorshowchallenge.php +++ b/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'];

getDisplayName()); ?>

-

t('Error while validating your second factor')); ?>

+ +

+ +

t('Error while validating your second factor')); ?>

+
diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 2e5fc685e8..d0ca4646e5 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/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', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 2b8233f656..6fe9a95c24 100644 --- a/lib/composer/composer/autoload_static.php +++ b/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', diff --git a/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php b/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php new file mode 100644 index 0000000000..76e728b6ab --- /dev/null +++ b/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php @@ -0,0 +1,38 @@ + + * @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 + * + */ + +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 { + +} diff --git a/tests/Core/Controller/TwoFactorChallengeControllerTest.php b/tests/Core/Controller/TwoFactorChallengeControllerTest.php index 9421a2537e..bef343f904 100644 --- a/tests/Core/Controller/TwoFactorChallengeControllerTest.php +++ b/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('')); - $expected = new \OCP\AppFramework\Http\TemplateResponse('core', 'twofactorshowchallenge', [ + $expected = new TemplateResponse('core', 'twofactorshowchallenge', [ 'error' => true, 'provider' => $provider, 'backupProvider' => $backupProvider, 'logout_attribute' => 'logoutAttribute', 'template' => '', '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')); }