prevent infinite redirect loops if the there is no 2fa provider to pass

This fixes infinite loops that are caused whenever a user is about to solve a 2FA
challenge, but the provider app is disabled at the same time. Since the session
value usually indicates that the challenge needs to be solved before we grant access
we have to remove that value instead in this special case.

diff --git a/apps/dav/lib/Connector/Sabre/Auth.php b/apps/dav/lib/Connector/Sabre/Auth.php
index bd80b17b640d12f30ecffb91b40432a95b715b48..a35eed8807395c71b6b4066d366fdfaf59aa7d4f 100644 (file)
--- a/apps/dav/lib/Connector/Sabre/Auth.php
+++ b/apps/dav/lib/Connector/Sabre/Auth.php
@@ -224,7 +224,7 @@ class Auth extends AbstractBasic {
                if($forcedLogout) {
                        $this->userSession->logout();
                } else {
-                       if ($this->twoFactorManager->needsSecondFactor()) {
+                       if($this->twoFactorManager->needsSecondFactor($this->userSession->getUser())) {
                                throw new \Sabre\DAV\Exception\NotAuthenticated('2FA challenge not passed.');
                        }
                        if (\OC_User::handleApacheAuth() ||

diff --git a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php
index 6262407eb950eb138f626df9625815b5d0d757b1..8d77fc03a8de05e9c7497eb4b4100f4ac0ae21ec 100644 (file)
--- a/apps/dav/tests/unit/Connector/Sabre/AuthTest.php
+++ b/apps/dav/tests/unit/Connector/Sabre/AuthTest.php
@@ -374,6 +374,7 @@ class AuthTest extends TestCase {
                        ->willReturn(true);
                $this->twoFactorManager->expects($this->once())
                        ->method('needsSecondFactor')
+                       ->with($user)
                        ->will($this->returnValue(true));
                $this->auth->check($request, $response);
        }
@@ -658,7 +659,7 @@ class AuthTest extends TestCase {
                        ->method('getUID')
                        ->will($this->returnValue('MyTestUser'));
                $this->userSession
-                       ->expects($this->exactly(3))
+                       ->expects($this->exactly(4))
                        ->method('getUser')
                        ->will($this->returnValue($user));
                $response = $this->auth->check($server->httpRequest, $server->httpResponse);

diff --git a/core/Middleware/TwoFactorMiddleware.php b/core/Middleware/TwoFactorMiddleware.php
index 9b930edd57df4029c3a078bce4a371da884f182c..c4c3b724eb52186e15a8603bee243aa1fed8b3e6 100644 (file)
--- a/core/Middleware/TwoFactorMiddleware.php
+++ b/core/Middleware/TwoFactorMiddleware.php
@@ -27,6 +27,7 @@ use Exception;
 use OC\Authentication\Exceptions\TwoFactorAuthRequiredException;
 use OC\Authentication\Exceptions\UserAlreadyLoggedInException;
 use OC\Authentication\TwoFactorAuth\Manager;
+use OC\Core\Controller\LoginController;
 use OC\Core\Controller\TwoFactorChallengeController;
 use OC\User\Session;
 use OCP\AppFramework\Controller;
@@ -36,6 +37,7 @@ use OCP\AppFramework\Utility\IControllerMethodReflector;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IURLGenerator;
+use OCP\IUser;
 
 class TwoFactorMiddleware extends Middleware {
 
@@ -83,7 +85,7 @@ class TwoFactorMiddleware extends Middleware {
                        return;
                }
 
-               if ($controller instanceof \OC\Core\Controller\LoginController && $methodName === 'logout') {
+               if ($controller instanceof LoginController && $methodName === 'logout') {
                        // Don't block the logout page, to allow canceling the 2FA
                        return;
                }
@@ -92,7 +94,7 @@ class TwoFactorMiddleware extends Middleware {
                        $user = $this->userSession->getUser();
 
                        if ($this->twoFactorManager->isTwoFactorAuthenticated($user)) {
-                               $this->checkTwoFactor($controller, $methodName);
+                               $this->checkTwoFactor($controller, $methodName, $user);
                        } else if ($controller instanceof TwoFactorChallengeController) {
                                // Allow access to the two-factor controllers only if two-factor authentication
                                // is in progress.
@@ -102,10 +104,10 @@ class TwoFactorMiddleware extends Middleware {
                // TODO: dont check/enforce 2FA if a auth token is used
        }
 
-       private function checkTwoFactor($controller, $methodName) {
+       private function checkTwoFactor($controller, $methodName, IUser $user) {
                // If two-factor auth is in progress disallow access to any controllers
                // defined within "LoginController".
-               $needsSecondFactor = $this->twoFactorManager->needsSecondFactor();
+               $needsSecondFactor = $this->twoFactorManager->needsSecondFactor($user);
                $twoFactor = $controller instanceof TwoFactorChallengeController;
 
                // Disallow access to any controller if 2FA needs to be checked

diff --git a/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php
index 66bcafbce71305b403f69e33caaeadc5c77ee10a..143fe7dc927ef0308bafc9da2428f282771aebb9 100644 (file)
--- a/lib/private/Authentication/TwoFactorAuth/Manager.php
+++ b/lib/private/Authentication/TwoFactorAuth/Manager.php
@@ -165,10 +165,24 @@ class Manager {
        /**
         * Check if the currently logged in user needs to pass 2FA
         *
+        * @param IUser $user the currently logged in user
         * @return boolean
         */
-       public function needsSecondFactor() {
-               return $this->session->exists(self::SESSION_UID_KEY);
+       public function needsSecondFactor(IUser $user = null) {
+               if (is_null($user) || !$this->session->exists(self::SESSION_UID_KEY)) {
+                       return false;
+               }
+
+               if (!$this->isTwoFactorAuthenticated($user)) {
+                       // There is no second factor any more -> let the user pass
+                       //   This prevents infinite redirect loops when a user is about
+                       //   to solve the 2FA challenge, and the provider app is
+                       //   disabled the same time
+                       $this->session->remove(self::SESSION_UID_KEY);
+                       return false;
+               }
+
+               return true;
        }
 
        /**

diff --git a/lib/private/legacy/api.php b/lib/private/legacy/api.php
index 30083294861c7224b17106696a8fdac26fb3c8c0..17ee9c5d468a7b446f27f823f6ea5a72d8462ef7 100644 (file)
--- a/lib/private/legacy/api.php
+++ b/lib/private/legacy/api.php
@@ -311,7 +311,7 @@ class OC_API {
                // reuse existing login
                $loggedIn = \OC::$server->getUserSession()->isLoggedIn();
                if ($loggedIn === true) {
-                       if (\OC::$server->getTwoFactorAuthManager()->needsSecondFactor()) {
+                       if (\OC::$server->getTwoFactorAuthManager()->needsSecondFactor(\OC::$server->getUserSession()->getUser())) {
                                // Do not allow access to OCS until the 2FA challenge was solved successfully
                                return false;
                        }

diff --git a/lib/private/legacy/json.php b/lib/private/legacy/json.php
index 2882ac94ea9204a8c2f71f3f8187f4c56dcb2271..f386d03ab1bd315b29a6fc07e56d61ae9ffb67ef 100644 (file)
--- a/lib/private/legacy/json.php
+++ b/lib/private/legacy/json.php
@@ -68,7 +68,7 @@ class OC_JSON{
        public static function checkLoggedIn() {
                $twoFactorAuthManger = \OC::$server->getTwoFactorAuthManager();
                if( !OC_User::isLoggedIn()
-                       || $twoFactorAuthManger->needsSecondFactor()) {
+                       || $twoFactorAuthManger->needsSecondFactor(\OC::$server->getUserSession()->getUser())) {
                        $l = \OC::$server->getL10N('lib');
                        http_response_code(\OCP\AppFramework\Http::STATUS_UNAUTHORIZED);
                        self::error(array( 'data' => array( 'message' => $l->t('Authentication error'), 'error' => 'authentication_error' )));

diff --git a/lib/private/legacy/util.php b/lib/private/legacy/util.php
index a975da39271425790fe42f0dd71e32139df6c25b..7341331518d1b397cd97021430ce0781f15f3a97 100644 (file)
--- a/lib/private/legacy/util.php
+++ b/lib/private/legacy/util.php
@@ -975,7 +975,7 @@ class OC_Util {
                        exit();
                }
                // Redirect to index page if 2FA challenge was not solved yet
-               if (\OC::$server->getTwoFactorAuthManager()->needsSecondFactor()) {
+               if (\OC::$server->getTwoFactorAuthManager()->needsSecondFactor(\OC::$server->getUserSession()->getUser())) {
                        header('Location: ' . \OCP\Util::linkToAbsolute('', 'index.php'));
                        exit();
                }

diff --git a/tests/Core/Middleware/TwoFactorMiddlewareTest.php b/tests/Core/Middleware/TwoFactorMiddlewareTest.php
index 6b8f492892896c33f7ad93e1f3bd8f47ba09b76d..8247efa1b82bc0e7d818e3496dce2ca9f7470c62 100644 (file)
--- a/tests/Core/Middleware/TwoFactorMiddlewareTest.php
+++ b/tests/Core/Middleware/TwoFactorMiddlewareTest.php
@@ -132,6 +132,7 @@ class TwoFactorMiddlewareTest extends TestCase {
                        ->will($this->returnValue(true));
                $this->twoFactorManager->expects($this->once())
                        ->method('needsSecondFactor')
+                       ->with($user)
                        ->will($this->returnValue(true));
 
                $this->middleware->beforeController(null, 'index');
@@ -159,6 +160,7 @@ class TwoFactorMiddlewareTest extends TestCase {
                        ->will($this->returnValue(true));
                $this->twoFactorManager->expects($this->once())
                        ->method('needsSecondFactor')
+                       ->with($user)
                        ->will($this->returnValue(false));
 
                $twoFactorChallengeController = $this->getMockBuilder('\OC\Core\Controller\TwoFactorChallengeController')

diff --git a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
index 586fd3aaa2e8a5f30175e11785c35a973ae2f70d..f9489150e2182fe863a8733c5e414d4faae250c4 100644 (file)
--- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
+++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php
@@ -72,6 +72,19 @@ class ManagerTest extends TestCase {
                });
        }
 
+       private function prepareNoProviders() {
+               $this->appManager->expects($this->any())
+                       ->method('getEnabledAppsForUser')
+                       ->with($this->user)
+                       ->will($this->returnValue([]));
+
+               $this->appManager->expects($this->never())
+                       ->method('getAppInfo');
+
+               $this->manager->expects($this->never())
+                       ->method('loadTwoFactorApp');
+       }
+
        private function prepareProviders() {
                $this->appManager->expects($this->any())
                        ->method('getEnabledAppsForUser')
@@ -164,7 +177,7 @@ class ManagerTest extends TestCase {
                        ->method('remove')
                        ->with('two_factor_auth_uid');
 
-               $this->assertEquals(true, $this->manager->verifyChallenge('email', $this->user, $challenge));
+               $this->assertTrue($this->manager->verifyChallenge('email', $this->user, $challenge));
        }
 
        public function testVerifyChallengeInvalidProviderId() {
@@ -177,7 +190,7 @@ class ManagerTest extends TestCase {
                $this->session->expects($this->never())
                        ->method('remove');
 
-               $this->assertEquals(false, $this->manager->verifyChallenge('dontexist', $this->user, $challenge));
+               $this->assertFalse($this->manager->verifyChallenge('dontexist', $this->user, $challenge));
        }
 
        public function testVerifyInvalidChallenge() {
@@ -191,16 +204,40 @@ class ManagerTest extends TestCase {
                $this->session->expects($this->never())
                        ->method('remove');
 
-               $this->assertEquals(false, $this->manager->verifyChallenge('email', $this->user, $challenge));
+               $this->assertFalse($this->manager->verifyChallenge('email', $this->user, $challenge));
        }
 
        public function testNeedsSecondFactor() {
+               $user = $this->getMock('\OCP\IUser');
                $this->session->expects($this->once())
                        ->method('exists')
                        ->with('two_factor_auth_uid')
                        ->will($this->returnValue(false));
 
-               $this->assertEquals(false, $this->manager->needsSecondFactor());
+               $this->assertFalse($this->manager->needsSecondFactor($user));
+       }
+
+       public function testNeedsSecondFactorUserIsNull() {
+               $user = null;
+               $this->session->expects($this->never())
+                       ->method('exists');
+
+               $this->assertFalse($this->manager->needsSecondFactor($user));
+       }
+
+       public function testNeedsSecondFactorWithNoProviderAvailableAnymore() {
+               $this->prepareNoProviders();
+
+               $user = null;
+               $this->session->expects($this->never())
+                       ->method('exists')
+                       ->with('two_factor_auth_uid')
+                       ->will($this->returnValue(true));
+               $this->session->expects($this->never())
+                       ->method('remove')
+                       ->with('two_factor_auth_uid');
+
+               $this->assertFalse($this->manager->needsSecondFactor($user));
        }
 
        public function testPrepareTwoFactorLogin() {