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

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

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);

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

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;
 	}
 
 	/**

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;
 			}

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' )));

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();
 		}

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')

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