Merge pull request #1031 from nextcloud/2fa-infinite-redirect-loop

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

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