Skip to content

feat: Implement Two-Factor Authentication (2FA) support for users#124

Closed
matiasperrone-exo wants to merge 1 commit intofeature/mfa-phase1from
feat/mfa-phase2
Closed

feat: Implement Two-Factor Authentication (2FA) support for users#124
matiasperrone-exo wants to merge 1 commit intofeature/mfa-phase1from
feat/mfa-phase2

Conversation

@matiasperrone-exo
Copy link
Copy Markdown
Contributor

@matiasperrone-exo matiasperrone-exo commented Apr 22, 2026

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.

  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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 22, 2026

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: 37cb4d4a-4c6e-4a36-b2f8-36119ec1956e

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-phase2

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 and usage tips.

@matiasperrone-exo matiasperrone-exo self-assigned this Apr 22, 2026
@github-actions
Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

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

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

@matiasperrone-exo matiasperrone-exo marked this pull request as ready for review April 22, 2026 18:19
@matiasperrone-exo matiasperrone-exo deleted the feat/mfa-phase2 branch April 22, 2026 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant