fix coding style and increase code coverage

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

diff --git a/core/Controller/TwoFactorChallengeController.php b/core/Controller/TwoFactorChallengeController.php
index 13c87b7b0abfc7ce93be8182e96aacd1b163a544..fd4811d3ff675dfbb65f708e835cc8860cab2dde 100644 (file)
--- a/core/Controller/TwoFactorChallengeController.php
+++ b/core/Controller/TwoFactorChallengeController.php
@@ -26,10 +26,10 @@ namespace OC\Core\Controller;
 use OC\Authentication\TwoFactorAuth\Manager;
 use OC_User;
 use OC_Util;
-use OCP\Authentication\TwoFactorAuth\TwoFactorException;
 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;
@@ -116,20 +116,19 @@ class TwoFactorChallengeController extends Controller {
                        $backupProvider = null;
                }
 
-               $error_message = "";
+               $errorMessage = '';
+               $error = false;
                if ($this->session->exists('two_factor_auth_error')) {
                        $this->session->remove('two_factor_auth_error');
                        $error = true;
-                       $error_message = $this->session->get("two_factor_auth_error_message");
+                       $errorMessage = $this->session->get("two_factor_auth_error_message");
                        $this->session->remove('two_factor_auth_error_message');
-               } else {
-                       $error = false;
                }
                $tmpl = $provider->getTemplate($user);
                $tmpl->assign('redirect_url', $redirect_url);
                $data = [
                        'error' => $error,
-                       'error_message' => $error_message,
+                       'error_message' => $errorMessage,
                        'provider' => $provider,
                        'backupProvider' => $backupProvider,
                        'logout_attribute' => $this->getLogoutAttribute(),
@@ -161,7 +160,7 @@ class TwoFactorChallengeController extends Controller {
                                if (!is_null($redirect_url)) {
                                        return new RedirectResponse($this->urlGenerator->getAbsoluteURL(urldecode($redirect_url)));
                                }
-                               return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index'));
+                               return new RedirectResponse(OC_Util::getDefaultPageUrl());
                        }
                } catch (TwoFactorException $e) {
                        /*
@@ -169,8 +168,7 @@ class TwoFactorChallengeController extends Controller {
                         * 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_message', $e->getMessage());
                }
 
                $this->session->set('two_factor_auth_error', true);

diff --git a/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php b/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php
index 9de5db6aa548685b20de119566573a200e9a13e7..76e728b6ab04c1f815259cb8c97bece258fdc377 100644 (file)
--- a/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php
+++ b/lib/public/Authentication/TwoFactorAuth/TwoFactorException.php
@@ -1,7 +1,9 @@
 <?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
@@ -16,22 +18,21 @@
  * 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/>
  *
- * User: cornelius
- * Date: 14.11.16
- */
-
-/*
- * This is the public API of ownCloud. 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.
  */
 
-// use OCP namespace for all classes that are considered public.
-// This means that they should be used by apps instead of the internal ownCloud classes
 namespace OCP\Authentication\TwoFactorAuth;
 
+use Exception;
+
 /**
  * Two Factor Authentication failed
- * @since 9.2.0
+ *
+ * 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 {}
+class TwoFactorException extends Exception {
+
+}

diff --git a/tests/Core/Controller/TwoFactorChallengeControllerTest.php b/tests/Core/Controller/TwoFactorChallengeControllerTest.php
index 3486ae55e2ee4226c0226c04ee58e21e4b9993d2..bef343f90434b3468fbf93fc8639c42167963605 100644 (file)
--- 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')
@@ -128,9 +142,7 @@ class TwoFactorChallengeControllerTest extends TestCase {
                        ->will($this->returnValue(true));
                $this->session->expects($this->exactly(2))
                        ->method('remove')
-                       ->with($this->logicalOr(
-                               $this->equalTo('two_factor_auth_error'),
-                               $this->equalTo('two_factor_auth_error_message')));
+                       ->with($this->logicalOr($this->equalTo('two_factor_auth_error'), $this->equalTo('two_factor_auth_error_message')));
                $provider->expects($this->once())
                        ->method('getTemplate')
                        ->with($user)
@@ -139,7 +151,7 @@ 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,
@@ -153,7 +165,7 @@ class TwoFactorChallengeControllerTest extends TestCase {
        }
 
        public function testShowInvalidChallenge() {
-               $user = $this->getMockBuilder('\OCP\IUser')->getMock();
+               $user = $this->createMock(IUser::class);
 
                $this->userSession->expects($this->once())
                        ->method('getUser')
@@ -167,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')
@@ -191,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')
@@ -210,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')
@@ -247,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'));
        }