Feature | Implement Two-Factor Authentication (2FA) support for users

[matiasperrone-exo]

Task:

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

Changes

• Created PHPUnit test suit "Two Factor Authentication Test Suite" Modified app/libs/Auth/Models/User.php:
• public const ValidMFAMethods = ['email_otp'];
• 3 new Doctrine-mapped private fields (two_factor_enabled, two_factor_method, two_factor_enforced_at) matching the Phase 1 migration columns
• Constructor initializers for the three fields
• Getters/setters: isTwoFactorEnabled/setTwoFactorEnabled, getTwoFactorMethod/setTwoFactorMethod, getTwoFactorEnforcedAt/setTwoFactorEnforcedAt
• shouldRequire2FA() — config-driven via two_factor.enforced_groups, falls through to the stored flag
• enable2FA(string $method) — whitelists via ValidMFAMethods, throws ValidationException otherwise
• getAvailableTwoFactorMethods() / isTwoFactorMethodEnable()
• Phase II/III stubs returning false: isPhoneNumberVerified, isTOTPConfirmed, isPassKeyEnabled
• Created config/two_factor.php with enforced_groups referencing IGroupSlugs constants (super-admins, administrators, oauth2-server-admins, openid-server-admins).
• Created tests/unit/UserTwoFactorTest.php — 11 test methods (25 assertions), all green.

Verification:

• doctrine:schema:validate: no new diffs on users relating to the 2FA columns — only the pre-existing documented noise (signed/unsigned, index renames).
• UserTwoFactorTest: 11/11 passing.
• TwoFactorRepositoriesTest (Phase 1): 3/3 still passing.

---

Card Description

GOAL

Current state

The User entity (Auth\User) has no 2FA-related properties or methods. Role checking exists via isSuperAdmin() and isAdmin() but there is no mechanism to determine whether a user requires two-factor authentication.

Target state

The User entity exposes the three new database fields as Doctrine-mapped properties and provides the business logic methods needed by the controller and services:
shouldRequire2FA(), enable2FA(), getAvailableTwoFactorMethods(), isTwoFactorMethodEnable(), getTwoFactorMethod(), and the ValidMFAMethods constant.

TASKS

• Add Doctrine property mappings for two_factor_enabled, two_factor_method, two_factor_enforced_at to the User entity with getters and setters
• Add the ValidMFAMethods constant array with value ['email_otp'] (sms_otp, totp, passkey are stubs for Phase II/III)
• Implement shouldRequire2FA(): returns true if user isAdmin(), otherwise returns two_factor_enabled value
• Implement enable2FA(string $method): validates method against ValidMFAMethods, sets two_factor_enabled=true, two_factor_method, and two_factor_enforced_at=now()
• Implement getAvailableTwoFactorMethods(): returns ['email_otp'] if email is verified (Phase I always returns email_otp for enrolled users). Stubs for isPhoneNumberVerified(),
  isTOTPConfirmed(), isPassKeyEnabled() return false.
• Implement isTwoFactorMethodEnable(string $method): returns whether the method is in getAvailableTwoFactorMethods()
• Implement isEmailVerified() if not already present (check existing User entity for email_verified or active status)
• Write unit tests for shouldRequire2FA() with: super-admin user, admin user, regular user with 2FA enabled, regular user with 2FA disabled
• Write unit tests for enable2FA() with valid and invalid method values
• Write unit tests for getAvailableTwoFactorMethods() and isTwoFactorMethodEnable()

ACCEPTANCE CRITERIA

• shouldRequire2FA() returns true for any user in super-admins or administrators groups regardless of two_factor_enabled value
• shouldRequire2FA() returns the value of two_factor_enabled for non-admin users
• enable2FA('email_otp') sets all three fields correctly; enable2FA('invalid') throws ValidationException
• getAvailableTwoFactorMethods() returns ['email_otp'] for active users in Phase I
• isTwoFactorMethodEnable('email_otp') returns true; isTwoFactorMethodEnable('sms_otp') returns false in Phase I
• All unit tests pass

DEVELOPMENT NOTES

Key files:

• Modified: app/libs/Auth/User.php (or Auth\User depending on namespace)
• New: tests/Unit/UserTwoFactorTest.php

Gotchas:

• User::isAdmin() checks SuperAdminGroup OR AdminGroup. shouldRequire2FA() must use isAdmin() not just check one group.
• The SDS specifies that Phase I enforced_groups config also includes oauth2-server-admins and openid-server-admins. shouldRequire2FA() should check membership in ALL groups listed in
  config('two_factor.enforced_groups'), not hardcode group slugs.
• isEmailVerified() semantics: the existing User model may use 'active' status or a specific email_verified flag. Check the existing codebase.
• Phase II/III stub methods (isPhoneNumberVerified, isTOTPConfirmed, isPassKeyEnabled) must return false so getAvailableTwoFactorMethods() only returns email_otp in Phase I.

Out of scope: Database migration (Ticket 1), controller integration, service layer.

[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: cca97619-74df-4dba-99b7-c2fcd4162b96

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-phase1-user-model

---

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-126/

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[martinquiroga-exo]

@matiasperrone-exo please see comments. Thanks.

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[matiasperrone-exo]

@martinquiroga-exo please review again

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[martinquiroga-exo]

LGTM

[caseylocker]

@matiasperrone-exo — small rename request to align this PR with the SDS.

sds/idp-mfa.md uses isTwoFactorMethodEnable() (no trailing d) in 5 places: lines 438, 1988, 2064, 2151, 2186 — including the controller-flow pseudo-code at 438 and the AuthService snippets at 2151/2186 that the downstream MFA Challenge Strategy ticket (86b9j5jup) will reference.

No in-flight PR currently calls either spelling — PR #125 (base) and PR #127 (AuthService validateCredentials) don't reference it — so the rename is safe to make on this branch only:

• app/libs/Auth/Models/User.php:2547 — rename isTwoFactorMethodEnabled → isTwoFactorMethodEnable
• tests/unit/UserTwoFactorTest.php:190-194 — update the 5 assertion call sites (test method name testIsTwoFactorMethodEnable already matches the SDS spelling)

The current spelling is more grammatical, but updating the SDS in lockstep is more disruptive than matching it here.

[caseylocker]

Three more findings on top of the rename request, ordered by severity. Each is also added as a line-level comment below.

1. setTwoFactorEnabled() couples write-state to policy — app/libs/Auth/Models/User.php:2436-2438. setTwoFactorEnabled(false) silently persists true for users in enforced_groups because shouldRequire2FA() short-circuits before reading the flag. Both the SDS § 6 and the ticket DOD prescribe direct assignment in enable2FA() with no policy-overlaying setter.

2. Dead validation tier in setTwoFactorMethod() — app/libs/Auth/Models/User.php:2449-2473. The inner ValidMFAMethods check is unreachable from public callers; only the reflection test exercises it. Both the SDS and the ticket prescribe single-tier validation in enable2FA() with no setTwoFactorMethod() setter at all.

3. Unrelated change in SocialLoginController.php:160 — error message wording shortened. Out of scope by the ticket's own statement ("Out of scope: ... controller integration, service layer."). Recommend reverting and splitting to its own PR.

For context: I cross-checked all three against the SDS (sds/idp-mfa.md § 3.1, § 4.3, § 6) and the ClickUp ticket DOD (86b9h8xt1). All three are supported by both — they are additions/scope drift the spec does not ask for.

A second-pass review (Codex) independently confirmed all three. Two additional concerns I am not posting here:

• A minor design concern about two_factor_method's non-null email_otp default — withdrawn after rechecking, since SDS § 3.1 explicitly prescribes that default.
• A defensive-guard suggestion in shouldRequire2FA() against misconfigured enforced_groups (typo'd slugs, non-array) — nice-to-have, not required.

[caseylocker]

@matiasperrone-exo please see comments. Thanks.

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[matiasperrone-exo]

@matiasperrone-exo — small rename request to align this PR with the SDS.

sds/idp-mfa.md uses isTwoFactorMethodEnable() (no trailing d) in 5 places: lines 438, 1988, 2064, 2151, 2186 — including the controller-flow pseudo-code at 438 and the AuthService snippets at 2151/2186 that the downstream MFA Challenge Strategy ticket (86b9j5jup) will reference.

No in-flight PR currently calls either spelling — PR #125 (base) and PR #127 (AuthService validateCredentials) don't reference it — so the rename is safe to make on this branch only:

• app/libs/Auth/Models/User.php:2547 — rename isTwoFactorMethodEnabled → isTwoFactorMethodEnable
• tests/unit/UserTwoFactorTest.php:190-194 — update the 5 assertion call sites (test method name testIsTwoFactorMethodEnable already matches the SDS spelling)

The current spelling is more grammatical, but updating the SDS in lockstep is more disruptive than matching it here.

We are going to correct this typo in the SDS

[matiasperrone-exo]

@caseylocker please review it again, I addressed all the named issues by moving the validation away from the setters, and leave the setters clean also reverted the unexpected changes in SocialLoginController.

Three more findings on top of the rename request, ordered by severity. Each is also added as a line-level comment below.

1. setTwoFactorEnabled() couples write-state to policy — app/libs/Auth/Models/User.php:2436-2438. setTwoFactorEnabled(false) silently persists true for users in enforced_groups because shouldRequire2FA() short-circuits before reading the flag. Both the SDS § 6 and the ticket DOD prescribe direct assignment in enable2FA() with no policy-overlaying setter.
2. Dead validation tier in setTwoFactorMethod() — app/libs/Auth/Models/User.php:2449-2473. The inner ValidMFAMethods check is unreachable from public callers; only the reflection test exercises it. Both the SDS and the ticket prescribe single-tier validation in enable2FA() with no setTwoFactorMethod() setter at all.
3. Unrelated change in SocialLoginController.php:160 — error message wording shortened. Out of scope by the ticket's own statement ("Out of scope: ... controller integration, service layer."). Recommend reverting and splitting to its own PR.

For context: I cross-checked all three against the SDS (sds/idp-mfa.md § 3.1, § 4.3, § 6) and the ClickUp ticket DOD (86b9h8xt1). All three are supported by both — they are additions/scope drift the spec does not ask for.

A second-pass review (Codex) independently confirmed all three. Two additional concerns I am not posting here:

• A minor design concern about two_factor_method's non-null email_otp default — withdrawn after rechecking, since SDS § 3.1 explicitly prescribes that default.
• A defensive-guard suggestion in shouldRequire2FA() against misconfigured enforced_groups (typo'd slugs, non-array) — nice-to-have, not required.

[caseylocker]

Based on the requirements/ticket this LGTM.