Skip to content

refactor(core): improve TwoFactorMiddleware clarity and maintainability #57297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
joshtrichards wants to merge 2 commits into
base: master
Choose a base branch
from
+78 −50
Draft

refactor(core): improve TwoFactorMiddleware clarity and maintainability #57297

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21aca17
fix(core): refactor TwoFactorMiddleware for clarity and maintainability
joshtrichards Dec 31, 2025
9286538
chore: fix/add type hints where possible
joshtrichards Dec 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 78 additions & 50 deletions core/Middleware/TwoFactorMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-FileCopyrightText: 2016-2025 Nextcloud GmbH and Nextcloud contributors
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
* SPDX-License-Identifier: AGPL-3.0-only
*/
Expand All @@ -27,6 +27,9 @@
use OCP\IURLGenerator;
use OCP\IUser;

/**
* Two-factor authentication enforcement middleware
*/
class TwoFactorMiddleware extends Middleware {
public function __construct(
private Manager $twoFactorManager,
Expand All @@ -39,82 +42,107 @@
}

/**
* @param Controller $controller
* @param string $methodName
* Enforces two-factor authentication during controller dispatch as required.
*
* Allows requests to proceed only if two-factor authentication is not required, is already completed,
* or the route is explicitly exempt. Blocks access to protected controllers and routes until the user
* completes two-factor authentication.
*
* @param Controller $controller The active controller instance.
* @param string $methodName The name of the method being dispatched.
* @throws TwoFactorAuthRequiredException if 2FA must be completed before proceeding.
* @throws UserAlreadyLoggedInException if attempting to access a 2FA challenge after completing 2FA.
*/
public function beforeController($controller, $methodName) {
public function beforeController(Controller $controller, string $methodName) {
$isChallengeController = $controller instanceof TwoFactorChallengeController;
$isSetupController = $controller instanceof ALoginSetupController;

// Allow bypass for routes that explicitly do not require 2FA.
if ($this->reflector->hasAnnotation('NoTwoFactorRequired')) {
// Route handler explicitly marked to work without finished 2FA are
// not blocked
return;
}

// Allow bypass when polling for 2FA notification state ((could probably use NoTwoFactorRequired instead, but explicit policy doesn't hurt).
if ($controller instanceof APIController && $methodName === 'poll') {

Check failure on line 66 in core/Middleware/TwoFactorMiddleware.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

TypeDoesNotContainType 

core/Middleware/TwoFactorMiddleware.php:66:7: TypeDoesNotContainType: Cannot resolve types for $controller - OCP\AppFramework\Controller does not contain OCA\TwoFactorNextcloudNotification\Controller\APIController (see https://psalm.dev/056)
// Allow polling the twofactor nextcloud notifications state
return;
}

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

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

if ($controller instanceof ALoginSetupController
&& $this->userSession->getUser() !== null
&& $this->twoFactorManager->needsSecondFactor($this->userSession->getUser())) {
$providers = $this->twoFactorManager->getProviderSet($this->userSession->getUser());
$user = $this->userSession->getUser();

if ($providers->getPrimaryProviders() === [] && !$providers->isProviderMissing()) {
return;
}
// Allow bypass if session is already 2FA-complete or 2FA exempt.
if ($this->twoFactorManager->isTwoFactorAuthenticated($user)) {
return;
}

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

if ($this->userSession->isLoggedIn()) {
$user = $this->userSession->getUser();

if ($this->session->exists('app_password') // authenticated using an app password
|| $this->session->exists('app_api') // authenticated using an AppAPI Auth
|| $this->twoFactorManager->isTwoFactorAuthenticated($user)) {
$needsSecondFactor = $this->twoFactorManager->needsSecondFactor($user);

$this->checkTwoFactor($controller, $methodName, $user);
} elseif ($controller instanceof TwoFactorChallengeController) {
// Allow access to the two-factor controllers only if two-factor authentication
// is in progress.
throw new UserAlreadyLoggedInException();
// Access control logic for all 2FA setup routes and most 2FA challenge routes
if (
// a challenge route that doesn't require a completed 2FA setup
($isChallengeController && !$this->reflector->hasAnnotation('TwoFactorSetUpDoneRequired'))
// a setup route when the user needs to go through 2FA
|| ($isSetupController && $needsSecondFactor)
) {
$providers = $this->twoFactorManager->getProviderSet($user);
$primaryProviders = $providers->getPrimaryProviders();
$providerMissing = $providers->isProviderMissing();

// Allow bypass if user has no configured providers and none are required by policy.
if (count($primaryProviders) === 0 && !$providerMissing) {
return;
}
}
// TODO: dont check/enforce 2FA if a auth token is used
}

private function checkTwoFactor(Controller $controller, $methodName, IUser $user) {
// If two-factor auth is in progress disallow access to any controllers
// defined within "LoginController".
$needsSecondFactor = $this->twoFactorManager->needsSecondFactor($user);
$twoFactor = $controller instanceof TwoFactorChallengeController;

// Disallow access to any controller if 2FA needs to be checked
if ($needsSecondFactor && !$twoFactor) {
// Enforce 2FA:
// - If a provider exists, user will be redirected to the appropriate 2FA challenge.
// - If a required provider is missing, this locks the user out until admin intervention.
// TODO: Consider calling out a missing provider (i.e. logging for admin, using a different exception/handling differently)
throw new TwoFactorAuthRequiredException();
}

// Allow access to the two-factor controllers only if two-factor authentication
// is in progress.
if (!$needsSecondFactor && $twoFactor) {

// Block access if user requests a challenge route, but doesn't need 2FA.
if ($isChallengeController && !$needsSecondFactor) {
throw new UserAlreadyLoggedInException();
}

// Enforce 2FA for all other controllers/routes if 2FA is still required.
if ($needsSecondFactor && !$isChallengeController) {
// Ensures users cannot interact with normal login routes while 2FA is still required.
throw new TwoFactorAuthRequiredException();
}
}

public function afterException($controller, $methodName, Exception $exception) {
/**
* Handles exceptions related to two-factor authentication during controller execution.
*
* - Redirects to the 2FA challenge selection page if a TwoFactorAuthRequiredException is thrown,
* passing along the current or requested URL for redirect after challenge completion.
* - Redirects to the file index view if a UserAlreadyLoggedInException is thrown,
* indicating the user tried to access a 2FA route after already completing authentication.
* - Rethrows all other exceptions for standard handling.
*
* @param Controller $controller The active controller instance.
* @param string $methodName The invoked method name.
* @param Exception $exception The exception that was thrown.
* @return RedirectResponse
* @throws Exception For anything not related to 2FA flow.
*/
public function afterException(Controller $controller, string $methodName, Exception $exception) {
if ($exception instanceof TwoFactorAuthRequiredException) {
$params = [
'redirect_url' => $this->request->getParam('redirect_url'),
Expand Down
Loading