summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--apps/dav/lib/Connector/Sabre/Auth.php2
-rw-r--r--apps/dav/tests/unit/Connector/Sabre/AuthTest.php3
-rw-r--r--core/Middleware/TwoFactorMiddleware.php10
-rw-r--r--lib/private/Authentication/TwoFactorAuth/Manager.php18
-rw-r--r--lib/private/legacy/api.php2
-rw-r--r--lib/private/legacy/json.php2
-rw-r--r--lib/private/legacy/util.php2
-rw-r--r--tests/Core/Middleware/TwoFactorMiddlewareTest.php2
-rw-r--r--tests/lib/Authentication/TwoFactorAuth/ManagerTest.php45
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() {
generated by cgit v1.2.3 (git 2.39.1) at 2025-01-15 18:35:52 +0000