From 6af2efb67931863b27d96c74cdff1d2ca2615e52 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Wed, 24 Aug 2016 10:42:07 +0200 Subject: [PATCH] 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. --- apps/dav/lib/Connector/Sabre/Auth.php | 2 +- .../tests/unit/Connector/Sabre/AuthTest.php | 3 +- core/Middleware/TwoFactorMiddleware.php | 10 +++-- .../Authentication/TwoFactorAuth/Manager.php | 18 +++++++- lib/private/legacy/api.php | 2 +- lib/private/legacy/json.php | 2 +- lib/private/legacy/util.php | 2 +- .../Middleware/TwoFactorMiddlewareTest.php | 2 + .../TwoFactorAuth/ManagerTest.php | 45 +++++++++++++++++-- 9 files changed, 71 insertions(+), 15 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Auth.php b/apps/dav/lib/Connector/Sabre/Auth.php index bd80b17b64..a35eed8807 100644 --- 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 6262407eb9..8d77fc03a8 100644 --- 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 9b930edd57..c4c3b724eb 100644 --- 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 66bcafbce7..143fe7dc92 100644 --- 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 3008329486..17ee9c5d46 100644 --- 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 2882ac94ea..f386d03ab1 100644 --- 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 a975da3927..7341331518 100644 --- 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 6b8f492892..8247efa1b8 100644 --- 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 586fd3aaa2..f9489150e2 100644 --- 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() {