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.

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 bd80b17b640..a35eed88073 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 6262407eb95..8d77fc03a8d 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 9b930edd57d..c4c3b724eb5 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 66bcafbce71..143fe7dc927 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 30083294861..17ee9c5d468 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 2882ac94ea9..f386d03ab1b 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 a975da39271..7341331518d 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 6b8f4928928..8247efa1b82 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 586fd3aaa2e..f9489150e21 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() {