diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index fdd3b482..532ad6fb 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -591,20 +591,10 @@ public function postLogin() if ($flow == IAuthService::AuthenticationFlowPasswordless) { - // Passwordless login is single-factor (email access only) and - // must not be usable to satisfy MFA enforcement (SDS idp-mfa.md - // §7.4 / Open Question #3). - $existing_user = $this->auth_service->getUserByUsername($username); - if (!is_null($existing_user) && $existing_user->shouldRequire2FA()) { - throw new AuthenticationException( - "This account requires password and two-factor authentication. Please use the password login option." - ); - } - $client = $this->resolveClientFromMemento(); $otpClaim = OAuth2OTP::fromParams($username, $connection, $password); - $this->auth_service->loginWithOTP($otpClaim, $client); + $this->auth_service->loginWithOTPEnforcing2FA($otpClaim, $client); // A completed login must not leave the OTP screen restorable // on a later refresh - same identity-leakage concern already // fixed for the MFA flow's verify2FA()/verify2FARecovery(). diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index f90af6cd..5473cce2 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -271,38 +271,92 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ ); // TX-C: resolve or create user, finalize, login - return $this->tx_service->transaction(function () use ($otp, $otpClaim, $client, $remember) { + return $this->tx_service->transaction(function () use ($otp, $client, $remember) { + $user = $this->resolveOTPUser($otp); + $this->finalizeRedemption($otp, $user, $client); + Auth::login($user, $remember); + Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId())); + return $otp; + }); + } - $user = $this->getUserByUsername($otp->getUserName()); + /** + * @param OAuth2OTP $otpClaim + * @param Client|null $client + * @param bool $remember + * @return OAuth2OTP|null + * @throws Exception + */ + public function loginWithOTPEnforcing2FA(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP + { + Log::debug(sprintf("AuthService::loginWithOTPEnforcing2FA otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName())); - if (is_null($user)) { - Log::debug(sprintf("AuthService::loginWithOTP user %s does not exist; auto-registering.", $otp->getUserName())); - $user = $this->auth_user_service->registerUser( - [ - 'email' => $otp->getUserName(), - 'email_verified' => true, - 'send_email_verified_notice' => false, - 'active' => true, - ], - $otp - ); - } else if ($user->isActive()) { - $user->verifyEmail(false); - } + $otp = $this->findAndValidateOTP( + $otpClaim->getValue(), + $otpClaim->getUserName(), + $otpClaim->getConnection(), + $otpClaim->getScope(), + $client + ); - if (!$user->canLogin()) { - Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login (not active).", $user->getId())); - throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); + // TX-C: resolve or create user, enforce 2FA, finalize, login + return $this->tx_service->transaction(function () use ($otp, $client, $remember) { + $user = $this->resolveOTPUser($otp); + + // Passwordless login is single-factor (email access only) and must not + // be usable to satisfy MFA enforcement (SDS idp-mfa.md §7.4 / Open + // Question #3). Checked here - after the OTP is proven valid, before + // finalizeRedemption()/Auth::login() - so neither a guessed code nor a + // rejected valid code ever triggers a login side effect (redemption, + // Auth::login, the Login event / queued PostLoginUser job). + if ($user->shouldRequire2FA()) { + throw new AuthenticationException( + "This account requires password and two-factor authentication. Please use the password login option." + ); } $this->finalizeRedemption($otp, $user, $client); - Auth::login($user, $remember); - Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId())); + Log::debug(sprintf("AuthService::loginWithOTPEnforcing2FA user %s logged in.", $user->getId())); return $otp; }); } + /** + * Resolves the user for an already-validated passwordless OTP, auto-registering + * a brand-new email if needed. Does not finalize redemption or log in - callers + * decide that (and whether to enforce 2FA first). + * @param OAuth2OTP $otp + * @return User + * @throws AuthenticationException + */ + private function resolveOTPUser(OAuth2OTP $otp): User + { + $user = $this->getUserByUsername($otp->getUserName()); + + if (is_null($user)) { + Log::debug(sprintf("AuthService::resolveOTPUser user %s does not exist; auto-registering.", $otp->getUserName())); + $user = $this->auth_user_service->registerUser( + [ + 'email' => $otp->getUserName(), + 'email_verified' => true, + 'send_email_verified_notice' => false, + 'active' => true, + ], + $otp + ); + } else if ($user->isActive()) { + $user->verifyEmail(false); + } + + if (!$user->canLogin()) { + Log::warning(sprintf("AuthService::resolveOTPUser user %s cannot login (not active).", $user->getId())); + throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); + } + + return $user; + } + /** * Verifies an OTP against an already-authenticated session user (MFA primitive). * diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index ce68d06c..a2ebd235 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -91,6 +91,18 @@ public function loginUser(User $user, bool $remember): void; */ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP; + /** + * Same as loginWithOTP(), but rejects the login when the resolved user has + * MFA enforced - passwordless (email-only) proof is not sufficient for an + * account that requires two-factor authentication. + * @param OAuth2OTP $otpClaim + * @param Client|null $client + * @param bool $remember + * @return OAuth2OTP|null + * @throws AuthenticationException + */ + public function loginWithOTPEnforcing2FA(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP; + /** * @param string $username diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index aa0abd56..eb6145aa 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -193,6 +193,15 @@ public function testNonAdminWithoutMFALogsInNormally(): void public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void { + // No separate "no login side effect" assertions here on purpose: the + // DB-visible ones (OTP redemption, sibling revocation) are unreachable + // by construction, because the guard throws inside the AuthService + // transaction and DoctrineTransactionService rolls the whole closure + // back - moving the guard below finalizeRedemption() leaves the OTP + // un-redeemed all the same. The session-visible one (Auth::login() and + // the Login event that queues PostLoginUser) is already covered by the + // Auth::check() assertion below, which is what fails first if the guard + // is moved below Auth::login(). Both were verified by mutation. $this->emitOTP(self::ADMIN_EMAIL); $code = $this->latestOtpCode(self::ADMIN_EMAIL); @@ -213,6 +222,56 @@ public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void $this->assertSame('otp', Session::get('flow'), 'a reload must land back on the OTP screen, not silently fall back to password'); } + public function testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus(): void + { + // Regression guard: the enforcement check used to run BEFORE the OTP was + // validated, so an attacker submitting a garbage/guessed code against an + // MFA-enforced account's email got the distinguishing "requires password + // and two-factor authentication" message without ever proving control of + // the inbox - an account-enumeration oracle. The check must now only be + // reachable after loginWithOTPEnforcing2FA() has proven the code valid, + // so an invalid code gets the same generic rejection for any account. + // + // Enumeration is about DISTINGUISHABILITY, not about one phrase: merely + // asserting the absence of "two-factor authentication" would still pass + // if some future enforced-only branch leaked a *differently* worded + // message. So the enforced account's rejection is compared byte-for-byte + // against a non-enforced control driven through the same endpoint with + // the same bad code. + $control_email = $this->createPlainUser(); + + $this->emitOTP(self::ADMIN_EMAIL); + $this->postLoginOTP(self::ADMIN_EMAIL, 'not-the-real-code'); + + $this->assertFalse(Auth::check(), 'an invalid code must never authenticate'); + $this->assertResponseStatus(302); + $enforced_notice = Session::get('flash_notice'); + $this->assertNotNull($enforced_notice, 'the enforced account must get a flashed rejection'); + + // Cleared so the control's assertion cannot silently read the enforced + // account's leftover flash and compare a value against itself. + Session::forget('flash_notice'); + + $this->emitOTP($control_email); + $this->postLoginOTP($control_email, 'not-the-real-code'); + + $this->assertFalse(Auth::check(), 'an invalid code must never authenticate the control account either'); + $this->assertResponseStatus(302); + $control_notice = Session::get('flash_notice'); + $this->assertNotNull($control_notice, 'the control account must get a flashed rejection'); + + $this->assertSame( + $control_notice, + $enforced_notice, + 'an invalid code must produce an identical rejection for an enforced and a non-enforced account - any difference is an enumeration oracle' + ); + $this->assertStringNotContainsString( + 'two-factor authentication', + $enforced_notice, + 'an invalid code must not leak that the account is MFA-enforced' + ); + } + public function testNonEnforcedUserStillLogsInViaPasswordlessLogin(): void { $email = $this->createPlainUser();