Feature | MFA Challenge Strategy Pattern (Interface, Abstract, Factory, EmailOTP)

[matiasperrone-exo]

Task:

Ref: https://app.clickup.com/t/86b9j5jup

Depends on

• https://app.clickup.com/t/86b9j51aa - Feature | Add AuthService validateCredentials method #127

Summary

Implements the MFA Challenge Strategy Pattern to provide a flexible, extensible framework for handling different multi-factor authentication methods. This PR introduces a
strategy-based architecture with an interface, abstract base class, factory pattern, and a complete Email OTP implementation.

Changes

New Components

1. Interface: IMFAChallengeStrategy

 - Defines the contract for MFA challenge strategies
 - Methods:
   - `issueChallenge()` — initiates a challenge (sends OTP, etc.)
   - `verifyChallenge()` — validates the user's response
   - `resendChallenge()` — resends the challenge
   - `getPendingState()` — retrieves temporary challenge state
   - `clearPendingState()` — cleans up session data
   - `verifyRecoveryCode()` — validates recovery codes as fallback

2. Abstract Base: AbstractMFAChallengeStrategy

 - Provides common MFA logic shared across all strategies
 - Features:
   - **Session State Management**: Stores pending user ID and challenge timestamp with 5-minute TTL
   - **Recovery Code Verification**: Hash-based recovery code validation against stored codes
   - **Time-based Expiration**: Automatically clears expired challenge state
 - Dependencies: `IUserRecoveryCodeRepository`

3. Concrete Implementation: EmailOTPMFAChallengeStrategy

 - Implements email-based one-time password (OTP) challenges
 - Features:
   - **Challenge Issuance**: Creates OTP via `ITokenService` with email connection type
   - **Challenge Verification**: Validates OTP from repository with:
     - Existence check
     - Lifetime validation
     - Validity verification
   - **Automatic Cleanup**: Redeems other unused OTPs for the user after successful verification
   - **Resend Support**: Allows users to request new OTPs
 - Dependencies: `ITokenService`, `IOAuth2OTPRepository`

4. Factory: MFAChallengeStrategyFactory

 - Uses match expression for clean, type-safe strategy instantiation
 - Extensible design for adding new MFA methods (TOTP, SMS, etc.)
 - Throws `InvalidArgumentException` for unknown methods

Test Coverage

• AbstractMFAChallengeStrategyTest (143 lines)

  • Session state lifecycle (store, retrieve, expire, clear)
  • Time-based expiration logic
  • Recovery code validation and error handling

• EmailOTPMFAChallengeStrategyTest (167 lines)

  • Challenge issuance and OTP creation
  • Challenge verification (valid, expired, invalid codes)
  • Automatic OTP cleanup after verification
  • Resend functionality
  • Error cases and exception handling

• MFAChallengeStrategyFactoryTest (23 lines)

  • Factory creation for email_otp method
  • Invalid method handling

Configuration

• Updated phpunit.xml to register the new test suite for MFA functionality

Design Rationale

• Strategy Pattern: Allows easy addition of new MFA methods (TOTP, SMS, WebAuthn) without modifying existing code
• Separation of Concerns: Abstract class handles state management; concrete classes handle method-specific logic
• Testability: Full unit test coverage for all scenarios
• Session Management: 5-minute TTL prevents long-lived sessions
• Recovery Fallback: Supports recovery codes as secondary verification method

Related Issues

• Depends on: PR Feature | Add AuthService validateCredentials method #127 (AuthService enhancements)
• Ref: ClickUp task

Migration Notes

• This PR introduces new classes only; no breaking changes to existing code
• Ready for integration with OAuth2 authentication flows

---

Requested GOAL

Current state: No MFA challenge infrastructure exists. The login flow has no concept of multi-factor verification strategies.

Target state

A complete strategy pattern is in place: IMFAChallengeStrategy interface defines the contract, AbstractMFAChallengeStrategy provides shared session management and recovery code verification, MFAChallengeStrategyFactory resolves method names to strategy instances, and EmailOTPMFAChallengeStrategy wraps the existing OTP infrastructure for email-based 2FA challenges. This pattern is the extension point for Phase II (SMS) and Phase III (TOTP, passkey) without modifying the controller.

TASKS

• Create IMFAChallengeStrategy interface with methods: issueChallenge(User, ?Client, bool): array, verifyChallenge(User, string): void, resendChallenge(User, ?Client, bool): array, getPendingState(): ?array, clearPendingState(): void, verifyRecoveryCode(User, string): void
• Create AbstractMFAChallengeStrategy implementing getPendingState() (reads 2fa_pending_user_id, 2fa_pending_at, 2fa_remember from session with TTL check), clearPendingState() (forgets all 2FA session keys), and verifyRecoveryCode() (iterates unused recovery codes, Hash::check against input, marks used_at on match, throws AuthenticationException on no match)
• Create MFAChallengeStrategyFactory with a static create(string $method) method that resolves 'email_otp' to EmailOTPMFAChallengeStrategy via the service container. Throws InvalidArgumentException for unknown methods.
  The factory would just be:

  public static function create(string $method): IMFAChallengeStrategy                                                                                                                            
  {                                                                    
      return match($method) {                                   
          'email_otp' => app()->make(EmailOTPMFAChallengeStrategy::class),
          default => throw new \InvalidArgumentException("Unknown MFA method: {$method}"),
      };
  }

• Create EmailOTPMFAChallengeStrategy extending AbstractMFAChallengeStrategy: issueChallenge() stores pending state in session, creates OTP via ITokenService::createOTPFromPayload with connection='email', returns otp_length and otp_lifetime. verifyChallenge() looks up OTP by value/connection/username, validates alive+valid+attempts, redeems, revokes other pending OTPs. resendChallenge() delegates to issueChallenge().
• Write unit tests for AbstractMFAChallengeStrategy::getPendingState() with valid session, expired session, missing session
• Write unit tests for AbstractMFAChallengeStrategy::verifyRecoveryCode() with matching code, non-matching code, all codes used
• Write unit tests for EmailOTPMFAChallengeStrategy::issueChallenge() (mock ITokenService)
• Write unit tests for EmailOTPMFAChallengeStrategy::verifyChallenge() with valid OTP, expired OTP, max attempts exceeded, wrong value
• Write unit tests for MFAChallengeStrategyFactory::create() with valid and invalid method names

ACCEPTANCE CRITERIA

• MFAChallengeStrategyFactory::create('email_otp') returns an EmailOTPMFAChallengeStrategy instance
• MFAChallengeStrategyFactory::create('unknown') throws InvalidArgumentException
• issueChallenge() stores user ID and timestamp in session, creates an OTP via the existing token service, returns array with otp_length and otp_lifetime
• verifyChallenge() with valid unexpired OTP redeems it successfully and revokes other pending OTPs for the user
• verifyChallenge() with expired OTP throws AuthenticationException("Verification code is expired.")
• verifyChallenge() with max attempts exceeded throws AuthenticationException("Verification code is not valid.")
• getPendingState() returns null after session TTL expires (default 300 seconds)
• verifyRecoveryCode() marks the matching code as used (sets used_at) and returns void
• verifyRecoveryCode() with no matching code throws AuthenticationException("Invalid recovery code.")
• All unit tests pass

DEVELOPMENT NOTES

Key files:

• New: app/Strategies/MFA/IMFAChallengeStrategy.php
• New: app/Strategies/MFA/AbstractMFAChallengeStrategy.php
• New: app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php
• New: app/Strategies/MFA/MFAChallengeStrategyFactory.php
• New: tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php
• New: tests/Unit/MFA/AbstractMFAChallengeStrategyTest.php
• New: tests/Unit/MFA/MFAChallengeStrategyFactoryTest.php

Gotchas:

• EmailOTPMFAChallengeStrategy reuses the EXISTING OTP infrastructure: ITokenService::createOTPFromPayload() and IOAuth2OTPRepository. It does NOT create a parallel OTP system.
• Session keys for pending state: '2fa_pending_user_id', '2fa_pending_at', '2fa_remember', '2fa_recovery_attempts'. These are shared across all strategy implementations via the abstract class.
• verifyChallenge() must call logRedeemAttempt() BEFORE comparing values, per SDS section 4.5. This ensures attempt exhaustion even on mismatch.
• OTP revocation after successful verification: query getByUserNameNotRedeemed() and redeem all OTPs except the one just verified.
• The SDS shows OAuth2OTP::fromParams() usage for looking up OTPs. Follow the existing pattern in the codebase for OTP validation.

Out of scope:

Controller wiring

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0849ccdb-9e19-41b2-b1dd-53c684968789

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/mfa-challenge-strategy-pattern

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[caseylocker]

Reviewed against ClickUp 86b9j5jup (this task) plus the linked parent 86b8yfdxd phase decomposition (86b9e8wm7 + 86b9j51aa).

The implementation matches the spec — all 6 task items, all 10 ACs, tests green (15/15, 31 assertions). Approving in the context of the phased rollout: this PR is deliberately scoped to the strategy layer with controller wiring deferred to a separate ticket.

A second-pass review surfaced a handful of items that fall between this PR's scope and the (not-yet-created) controller-wiring task. Flagging them here so they don't slip:

1. Cross-client audience check on `verifyChallenge` — the strategy's interface takes `(User, string)` per spec. `AuthService::loginWithOTP:230,273` passes `$client` and rejects on audience mismatch. The controller will need to either pre-check audience or we extend the strategy interface in Phase II.

2. Transaction boundaries — `AuthService::loginWithOTP` uses two separate `tx_service->transaction()` blocks so `logRedeemAttempt` commits before validation can throw. Strategy has none. The controller will need to replicate that two-tx shape.

3. `2fa_recovery_attempts` session key is declared in the abstract (`AbstractMFAChallengeStrategy.php:15`) but never written. Either the controller wires it as throttling, or we drop the constant.

4. Audit logging via `two_factor_audit_log` (introduced in 86b9e8wm7) needs IP / user-agent which the strategy can't see. Confirms this is structurally a controller-layer concern.

5. Minor: `issueChallenge` stores pending session state before OTP creation. If `createOTPFromPayload` throws, session is left dirty. Worth either reordering or having the controller wrap.

@smarcet — happy to file the controller-wiring ticket with these as explicit ACs if that helps. Approving this PR on the assumption that's where they'll be addressed.

[smarcet]

@caseylocker
Thanks for the pass.
Most of those concerns are already covered by the existing integration/audit/rate-limit tickets and are intentionally outside the scope of Ticket 4, which is limited to the MFA strategy layer itself.
how ever i will review those 2 first bc it seems that SDS left some gaps many thanks

[smarcet]

@matiasperrone-exo please review

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[matiasperrone-exo]

Hello @smarcet thank for the comments.
The changes requested are now in place.

Please review it at your earliest convenience.

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[matiasperrone-exo]

Hello @smarcet thank for the comments.

I added an extra change, the abstract class AbstractMFAChallengeStrategy in verifyRecoveryCode now persists the markedAsUsed() recovery code, to avoid losing the change after the call.

Here is the change:

Please review it at your earliest convenience.

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[smarcet]

Reviewed the new MFA strategy classes against the SDS (idp-mfa.md v2.2) and the existing auth-layer conventions. Two issues, both rooted in transaction ownership — flagged inline.

1. AbstractMFAChallengeStrategy::verifyRecoveryCode self-flushes (add(..., true)) from a layer that has no transaction boundary. Per the SDS the intended caller is the controller (UserController::verify2FARecovery, §4.11 + route /auth/login/2fa/recovery §4.13), and controllers here do not own DB transactions — services do (ITransactionService::transaction(), see AuthService::loginWithOTP / finalizeRedemption, with staged TX-A/B/C and pessimistic row-locking).

2. EmailOTPMFAChallengeStrategy::verifyChallenge builds a transient OTP via OAuth2OTP::fromParams() and never loads or compares the stored OTP, and never flushes its redeem() — so it neither verifies the code nor persists the redemption.

Controller wiring is out of scope for this PR, so (1) is currently latent, but (2) is a real defect that must be fixed before wiring. Recommendation: the recovery-code and OTP-redemption writes belong in a transaction-managed service (the sibling of the existing AuthService::verifyOTPChallenge()), with the controller calling the service; strategies should not self-flush. Note the SDS recovery snippet (§4.5) also uses raw EntityManager::flush() and should be aligned with the tx_service pattern.

[smarcet]

add($recoveryCode, true) forces an immediate persist() + flush() (see DoctrineRepository::add). This strategy has no transaction boundary — its constructor injects only IUserRecoveryCodeRepository, no ITransactionService. Everywhere else in the auth layer, mutations run inside tx_service->transaction() and are flushed once at commit; no repository self-flushes with sync = true (cf. AuthService::finalizeRedemption in app/libs/Auth/AuthService.php).

Per the SDS this method is invoked from the controller (UserController::verify2FARecovery, §4.11 + route §4.13), and controllers here do not own transactions — services do. So a controller-invoked strategy self-flushing a write bypasses the service/transaction layer every other auth write goes through. It is also unsafe to compose inside an outer transaction (premature mid-tx flush) and is excluded from the row-locking / atomicity the OTP redemption path relies on.

Fix: move the mark-used write into a transaction-managed service method and use add(..., false). The SDS snippet here uses raw EntityManager::flush() and should be corrected too.

[smarcet]

This method does not actually verify the OTP. OAuth2OTP::fromParams() is a pure in-memory factory (new self(...), app/Models/OAuth2/OAuth2OTP.php:448) — it never reads the DB. The code never calls otp_repository->getByValueConnectionAndUserName(...) and never compares the submitted value against the stored OTP. Consequences:

• is_null($otp) (L42) is dead code — fromParams always returns an object.
• isAlive() / isValid() (L48–53) run against a freshly-built object with default state, so they effectively always pass.
• $otp->redeem() (L56) and the sibling revoke mutate transient entities that are never persisted (no tx_service, no add, no flush).

Net effect if wired as-is: the submitted code is not validated against what was issued, and nothing is persisted. SDS §4.5 fetches the persisted OTP via the repository and does if ($otp->getValue() != $value) throw — both were dropped here. Recommend delegating to the existing AuthService::verifyOTPChallenge() (transaction-managed, value-checked, row-locked, session-user bound) instead of re-implementing redemption.