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() {