Skip to content

Commit 21aca17

Browse files
joshtrichardsjoshtrichards
authored
fix(core): refactor TwoFactorMiddleware for clarity and maintainability
Signed-off-by: Josh <[email protected]>
1 parent 32327c6 commit 21aca17

File tree

1 file changed

+76
-48
lines changed

1 file changed

+76
-48
lines changed

‎core/Middleware/TwoFactorMiddleware.php‎

Lines changed: 76 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
declare(strict_types=1);
44

55
/**
6-
* SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-FileCopyrightText: 2016-2025 Nextcloud GmbH and Nextcloud contributors
77
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
88
* SPDX-License-Identifier: AGPL-3.0-only
99
*/
@@ -27,6 +27,9 @@
2727
use OCP\IURLGenerator;
2828
use OCP\IUser;
2929

30+
/**
31+
* Two-factor authentication enforcement middleware
32+
*/
3033
class TwoFactorMiddleware extends Middleware {
3134
public function __construct(
3235
private Manager $twoFactorManager,
@@ -39,81 +42,106 @@ public function __construct(
3942
}
4043

4144
/**
42-
* @param Controller $controller
43-
* @param string $methodName
45+
* Enforces two-factor authentication during controller dispatch as required.
46+
*
47+
* Allows requests to proceed only if two-factor authentication is not required, is already completed,
48+
* or the route is explicitly exempt. Blocks access to protected controllers and routes until the user
49+
* completes two-factor authentication.
50+
*
51+
* @param Controller $controller The active controller instance.
52+
* @param string $methodName The name of the method being dispatched.
53+
* @throws TwoFactorAuthRequiredException if 2FA must be completed before proceeding.
54+
* @throws UserAlreadyLoggedInException if attempting to access a 2FA challenge after completing 2FA.
4455
*/
4556
public function beforeController($controller, $methodName) {
57+
$isChallengeController = $controller instanceof TwoFactorChallengeController;
58+
$isSetupController = $controller instanceof ALoginSetupController;
59+
60+
// Allow bypass for routes that explicitly do not require 2FA.
4661
if ($this->reflector->hasAnnotation('NoTwoFactorRequired')) {
47-
// Route handler explicitly marked to work without finished 2FA are
48-
// not blocked
4962
return;
5063
}
5164

65+
// Allow bypass when polling for 2FA notification state ((could probably use NoTwoFactorRequired instead, but explicit policy doesn't hurt).
5266
if ($controller instanceof APIController && $methodName === 'poll') {
53-
// Allow polling the twofactor nextcloud notifications state
5467
return;
5568
}
5669

57-
if ($controller instanceof TwoFactorChallengeController
58-
&& $this->userSession->getUser() !== null
59-
&& !$this->reflector->hasAnnotation('TwoFactorSetUpDoneRequired')) {
60-
$providers = $this->twoFactorManager->getProviderSet($this->userSession->getUser());
70+
// Allow bypass for logging out (could probably use NoTwoFactorRequired instead, but explicit policy doesn't hurt).
71+
if ($controller instanceof LoginController && $methodName === 'logout') {
72+
return;
73+
}
6174

62-
if (!($providers->getPrimaryProviders() === [] && !$providers->isProviderMissing())) {
63-
throw new TwoFactorAuthRequiredException();
64-
}
75+
// Allow bypass if there is no user session to enforce 2FA for.
76+
if (!$this->userSession->isLoggedIn()) {
77+
return;
6578
}
6679

67-
if ($controller instanceof ALoginSetupController
68-
&& $this->userSession->getUser() !== null
69-
&& $this->twoFactorManager->needsSecondFactor($this->userSession->getUser())) {
70-
$providers = $this->twoFactorManager->getProviderSet($this->userSession->getUser());
80+
$user = $this->userSession->getUser();
7181

72-
if ($providers->getPrimaryProviders() === [] && !$providers->isProviderMissing()) {
73-
return;
74-
}
82+
// Allow bypass if session is already 2FA-complete or 2FA exempt.
83+
if ($this->twoFactorManager->isTwoFactorAuthenticated($user)) {
84+
return;
7585
}
7686

77-
if ($controller instanceof LoginController && $methodName === 'logout') {
78-
// Don't block the logout page, to allow canceling the 2FA
87+
// Allow bypass if session is using app/api tokens.
88+
if ($this->session->exists('app_password') || $this->session->exists('app_api')) {
89+
// TODO: Check duplicate code in OC\Authentication\TwoFactorAuth::needsSecondFactor() (and see #1031)
7990
return;
8091
}
8192

82-
if ($this->userSession->isLoggedIn()) {
83-
$user = $this->userSession->getUser();
84-
85-
if ($this->session->exists('app_password') // authenticated using an app password
86-
|| $this->session->exists('app_api') // authenticated using an AppAPI Auth
87-
|| $this->twoFactorManager->isTwoFactorAuthenticated($user)) {
93+
$needsSecondFactor = $this->twoFactorManager->needsSecondFactor($user);
8894

89-
$this->checkTwoFactor($controller, $methodName, $user);
90-
} elseif ($controller instanceof TwoFactorChallengeController) {
91-
// Allow access to the two-factor controllers only if two-factor authentication
92-
// is in progress.
93-
throw new UserAlreadyLoggedInException();
95+
// Access control logic for all 2FA setup routes and most 2FA challenge routes
96+
if (
97+
// a challenge route that doesn't require a completed 2FA setup
98+
($isChallengeController && !$this->reflector->hasAnnotation('TwoFactorSetUpDoneRequired'))
99+
// a setup route when the user needs to go through 2FA
100+
|| ($isSetupController && $needsSecondFactor)
101+
) {
102+
$providers = $this->twoFactorManager->getProviderSet($user);
103+
$primaryProviders = $providers->getPrimaryProviders();
104+
$providerMissing = $providers->isProviderMissing();
105+
106+
// Allow bypass if user has no configured providers and none are required by policy.
107+
if (count($primaryProviders) === 0 && !$providerMissing) {
108+
return;
94109
}
95-
}
96-
// TODO: dont check/enforce 2FA if a auth token is used
97-
}
98-
99-
private function checkTwoFactor(Controller $controller, $methodName, IUser $user) {
100-
// If two-factor auth is in progress disallow access to any controllers
101-
// defined within "LoginController".
102-
$needsSecondFactor = $this->twoFactorManager->needsSecondFactor($user);
103-
$twoFactor = $controller instanceof TwoFactorChallengeController;
104110

105-
// Disallow access to any controller if 2FA needs to be checked
106-
if ($needsSecondFactor && !$twoFactor) {
111+
// Enforce 2FA:
112+
// - If a provider exists, user will be redirected to the appropriate 2FA challenge.
113+
// - If a required provider is missing, this locks the user out until admin intervention.
114+
// TODO: Consider calling out a missing provider (i.e. logging for admin, using a different exception/handling differently)
107115
throw new TwoFactorAuthRequiredException();
108116
}
109-
110-
// Allow access to the two-factor controllers only if two-factor authentication
111-
// is in progress.
112-
if (!$needsSecondFactor && $twoFactor) {
117+
118+
// Block access if user requests a challenge route, but doesn't need 2FA.
119+
if ($isChallengeController && !$needsSecondFactor) {
113120
throw new UserAlreadyLoggedInException();
114121
}
122+
123+
// Enforce 2FA for all other controllers/routes if 2FA is still required.
124+
if ($needsSecondFactor && !$isChallengeController) {
125+
// Ensures users cannot interact with normal login routes while 2FA is still required.
126+
throw new TwoFactorAuthRequiredException();
127+
}
115128
}
116129

130+
/**
131+
* Handles exceptions related to two-factor authentication during controller execution.
132+
*
133+
* - Redirects to the 2FA challenge selection page if a TwoFactorAuthRequiredException is thrown,
134+
* passing along the current or requested URL for redirect after challenge completion.
135+
* - Redirects to the file index view if a UserAlreadyLoggedInException is thrown,
136+
* indicating the user tried to access a 2FA route after already completing authentication.
137+
* - Rethrows all other exceptions for standard handling.
138+
*
139+
* @param Controller $controller The active controller instance.
140+
* @param string $methodName The invoked method name.
141+
* @param Exception $exception The exception that was thrown.
142+
* @return RedirectResponse|null
143+
* @throws Exception For anything not related to 2FA flow.
144+
*/
117145
public function afterException($controller, $methodName, Exception $exception) {
118146
if ($exception instanceof TwoFactorAuthRequiredException) {
119147
$params = [

0 commit comments

Comments
 (0)