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: 8866e19e-e731-4510-ab5f-988e55330e58

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.

[martinquiroga-exo]

This should check for members listed in two_factor.enforced_groups not only isAdmin()

    public function isAdmin(): bool
    {
        return $this->belongToGroup(IGroupSlugs::SuperAdminGroup) || $this->belongToGroup(IGroupSlugs::AdminGroup);
    }

It only checks for SuperAdminGroup and AdminGroup which is not enough:

    'enforced_groups' => [
        IGroupSlugs::SuperAdminGroup,
        IGroupSlugs::AdminGroup,
        IGroupSlugs::OAuth2ServerAdminGroup,
        IGroupSlugs::OpenIdServerAdminsGroup,
    ],

[martinquiroga-exo]

All tests pass only if shouldRequire2FA() ignores two_factor.enforced_groups

[martinquiroga-exo]

This proves OAuth2 admin is not covered

[martinquiroga-exo]

This test only validates the fallback path (two_factor_enabled flag) and does not actually prove that config('two_factor.enforced_groups') is used.
Even after fixing shouldRequire2FA() to read config, this test would still pass if config logic is broken again.
Add a complementary test where a user belongs to a group in enforced_groups and two_factor_enabled = false, asserting that shouldRequire2FA() returns true. This ensures config-driven enforcement is actually covered.