fix: MFA-enforced accounts leak enumeration via passwordless login guard order by smarcet · Pull Request #154 · OpenStackweb/openstackid

smarcet · 2026-08-12T15:16:30Z

Root cause

UserController::postLogin()'s shouldRequire2FA() guard ran before loginWithOTP() validated the submitted OTP. A caller submitting any 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 usable with zero credentials.

Fix

Introduces AuthService::loginWithOTPEnforcing2FA(). The shouldRequire2FA() check now runs after the OTP is proven valid and before finalizeRedemption()/Auth::login():

An invalid/guessed code is rejected with the generic OTP error before ever reaching the account-status branch (no enumeration).
A valid code against an MFA-enforced account is rejected before any login side effect fires (redemption, Auth::login(), the Login event, the queued PostLoginUser job) — avoids the account-state mutation (last_login_date, resetLoginFailedAttempts()) that a naive reordering would have introduced.

loginWithOTP() (used by InteractiveGrantType and TokenService's OAuth2 grants) is unchanged — only UserController::postLogin()'s interactive web login enforces 2FA at this layer, matching the reported scope.

Avoids a boolean flag parameter (flags-over-objects antipattern — see DevIQ and Ardalis) by exposing two explicitly-named public methods delegating to a shared, flag-free resolveOTPUser() helper.

Testing

New regression test testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus, verified RED (fails against the pre-fix code with the exact leaking message) → GREEN.
Existing passwordless/MFA suite (TwoFactorLoginFlowTest.php, isolated run) green: OK (4 tests, 12 assertions).

Scope note

Not included: removeTrustedDevices() never being called (finding #2) and the OTP brute-force counter depending solely on the cache-backed rate limiter (finding #3) — separate findings from the same review, not addressed here.

Summary by CodeRabbit

Bug Fixes
- Improved passwordless login handling for accounts protected by two-factor authentication.
- Invalid OTP attempts now return a generic rejection without revealing whether additional verification is required.
- Prevented authentication when passwordless login requirements are not satisfied.
- Ensured OTP redemption and authentication occur only after all login requirements are validated.
Tests
- Added coverage to verify secure rejection behavior for invalid OTP submissions and prevent authentication side effects.

…ard order Root cause: UserController::postLogin()'s shouldRequire2FA() guard ran before loginWithOTP() validated the submitted OTP, so a caller without the real code could distinguish MFA-enforced accounts from the rejection message alone - an account-enumeration oracle requiring no credentials. Fix: introduce AuthService::loginWithOTPEnforcing2FA(), which checks shouldRequire2FA() after the OTP is proven valid and before finalizeRedemption()/Auth::login() - so a guessed/invalid code is rejected generically before reaching the account-status branch, and a valid code against an enforced account is rejected before any login side effect (redemption, Auth::login, the Login event / queued PostLoginUser job) fires. loginWithOTP() (used by InteractiveGrantType and TokenService's OAuth2 grants) is unchanged - only UserController::postLogin()'s interactive web login now enforces 2FA at this layer. Avoids a boolean flag parameter (flags-over-objects antipattern) by exposing two explicitly-named public methods delegating to a shared, flag-free resolveOTPUser() helper. Adds a regression test proving an invalid OTP against an MFA-enforced account gets the same generic rejection as any other invalid code.

coderabbitai · 2026-08-12T15:16:41Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 057c3019-8f44-4c38-8dc6-7f471771f4ad

📥 Commits

Reviewing files that changed from the base of the PR and between 67043e3 and e6100e1.

📒 Files selected for processing (1)

tests/TwoFactorLoginFlowTest.php

🚧 Files skipped from review as they are similar to previous changes (1)

tests/TwoFactorLoginFlowTest.php

📝 Walkthrough

Walkthrough

Passwordless login now uses loginWithOTPEnforcing2FA. OTP user resolution is shared between login flows. MFA-enforced users are rejected before OTP redemption or authentication.

Changes

Passwordless OTP MFA enforcement

Layer / File(s)	Summary
OTP authentication flow `app/libs/Auth/AuthService.php`, `app/libs/Utils/Services/IAuthService.php`	The service contract and implementation add `loginWithOTPEnforcing2FA`. Shared user resolution handles registration, email verification, and login eligibility. MFA-required users are rejected before redemption or authentication.
Controller wiring and regression coverage `app/Http/Controllers/UserController.php`, `tests/TwoFactorLoginFlowTest.php`	Passwordless login resolves the OAuth client and calls the enforcing flow. Tests verify generic rejection, no authentication, and no MFA-status disclosure.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant UserController
  participant AuthService
  participant OAuth2OTP
  participant User
  UserController->>AuthService: invoke loginWithOTPEnforcing2FA
  AuthService->>OAuth2OTP: validate OTP claim
  AuthService->>User: resolve and validate OTP user
  AuthService-->>UserController: reject MFA-required user or authenticate user

Possibly related PRs

OpenStackweb/openstackid#130: Related OTP resolution and authentication service changes.

Suggested labels: refactoring

Suggested reviewers: romanetar

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the MFA account-enumeration issue caused by passwordless login guard ordering.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/passwordless-mfa-enumeration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-08-12T15:18:02Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-154/

This page is automatically updated on each push to this PR.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…t unworded testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus only asserted that the flash message lacked the phrase "two-factor authentication". Enumeration is about distinguishability, so that assertion still passed if a future enforced-only branch leaked a differently worded message. It now drives the same invalid code through the same endpoint for the MFA-enforced admin and for a non-enforced control user, and asserts the two rejections are byte-identical. The stale flash is cleared between requests so the comparison cannot read a value against itself. Verified by mutation: restoring the pre-fix guard order with reworded text fails the new assertSame and would have passed the old substring assertion. Also records why testEnforcedUserCannotBypassMFAViaPasswordlessLogin adds no separate "no login side effect" assertions. Both candidates were tried and removed as change detectors that cannot fail: the DB-visible effects (OTP redemption, sibling revocation) are rolled back by the AuthService transaction regardless of where the guard sits, and the session-visible one (Auth::login() and the Login event queuing PostLoginUser) is already caught first by the existing Auth::check() assertion. Both confirmed by mutation. Full file green: OK (58 tests, 380 assertions).

smarcet · 2026-08-12T19:59:22Z

@coderabbitai review

coderabbitai · 2026-08-12T19:59:27Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

github-actions · 2026-08-12T20:00:57Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-154/

This page is automatically updated on each push to this PR.

smarcet requested a lite review from Copilot August 12, 2026 15:16

Copilot AI reviewed Aug 12, 2026

smarcet requested a lite review from Copilot August 12, 2026 15:24

Copilot AI reviewed Aug 12, 2026

smarcet marked this pull request as ready for review August 12, 2026 16:07

smarcet requested a review from romanetar August 12, 2026 16:07

smarcet marked this pull request as draft August 12, 2026 16:09

smarcet marked this pull request as ready for review August 12, 2026 19:48

smarcet self-assigned this Aug 12, 2026

smarcet added the security label Aug 12, 2026

Navigation Menu

Uh oh!

fix: MFA-enforced accounts leak enumeration via passwordless login guard order - #154

fix: MFA-enforced accounts leak enumeration via passwordless login guard order#154
smarcet wants to merge 2 commits into
feat/mfa-phase1---migrations--and--interfacesfrom
fix/passwordless-mfa-enumeration

smarcet commented Aug 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

smarcet commented Aug 12, 2026

Uh oh!

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

smarcet commented Aug 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Fix

Testing

Scope note

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

smarcet commented Aug 12, 2026

Uh oh!

coderabbitai Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

smarcet commented Aug 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading