Feature | Add MultiFactor Authentication. Initial migrations and entities

[matiasperrone-exo]

Task:

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

Changes

• Add migration for 2FA schema foundation (Phase I)
• Add UserTrustedDevice entity
• Add TwoFactorAuditLog entity
• Add UserRecoveryCode entity
• Add repository interfaces for 2FA entities
• Add Doctrine implementations for 2FA repositories
• Register 2FA repositories in service container
• Add repository round-trip tests for 2FA entities

---

Requested Goal

Current state

The IDP database has no 2FA-related tables or columns. The User entity lacks fields for tracking two-factor enrollment or enforcement status.

Target state

Three new tables (user_trusted_devices, two_factor_audit_log, user_recovery_codes) and three new columns on the users table (two_factor_enabled, two_factor_method, two_factor_enforced_at) exist via a single additive Doctrine migration. Corresponding Doctrine entities and repositories are registered in the ORM mappings.

This migration is the foundation for every other Phase I ticket. It must run with zero downtime since it only adds nullable columns and new tables.

Tasks

• Create Doctrine migration adding three columns to the users table: two_factor_enabled (boolean, default false), two_factor_method (string/enum, default 'email_otp'), two_factor_enforced_at
  (datetime, nullable)
• Create UserTrustedDevice Doctrine entity with fields: id, user_id (FK), device_identifier (varchar 255), device_name (varchar 255), ip_address (varchar 45), user_agent (text), trusted_at,
  expires_at, last_seen_at, is_revoked (boolean, default false), created_at, updated_at. Indexes on (user_id, device_identifier), (user_id, is_revoked), (expires_at)
• Create TwoFactorAuditLog Doctrine entity with fields: id, user_id (FK), event_type (enum: challenge_issued, challenge_succeeded, challenge_failed, enrollment_changed, device_trusted,
  device_revoked, recovery_used, settings_changed), method (enum: email_otp, sms_otp, totp, passkey, recovery), ip_address (varchar 45), user_agent (text), metadata (json, nullable), created_at.
  Indexes on (user_id, event_type, created_at), (created_at)
• Create UserRecoveryCode Doctrine entity with fields: id, user_id (FK), code_hash (varchar 255, bcrypt hash), used_at (datetime, nullable), created_at. Index on (user_id, used_at)
• Create IUserTrustedDeviceRepository, IUserRecoveryCodeRepository, ITwoFactorAuditLogRepository interfaces and their Doctrine implementations
• Register all new entities in the Doctrine ORM entity mappings
• Verify migration runs cleanly on a fresh database and on an existing database with data

ACCEPTANCE CRITERIA

php artisan doctrine:migrations:migrate runs without errors on both fresh and populated databases
All three new tables exist with correct column types, defaults, and indexes
Users table has the three new columns with correct defaults (two_factor_enabled=false, two_factor_method='email_otp', two_factor_enforced_at=NULL)
Each Doctrine entity can be persisted and retrieved via its repository ( provide unit tests )
Migration is fully additive: no columns dropped, no data modified, no destructive operations
Existing application functionality is unaffected after migration (login, OTP flow, admin panel all work)

DEVELOPMENT NOTES

Key files:

• New: app/libs/Auth/Models/UserTrustedDevice.php
• New: app/libs/Auth/Models/TwoFactorAuditLog.php
• New: app/libs/Auth/Models/UserRecoveryCode.php
• New: database/migrations/Version*.php (single migration file)
• Modified: Doctrine entity mapping configuration

Gotchas

• The User entity uses Doctrine ORM, not Eloquent. All mappings follow the existing annotation/attribute pattern in the codebase.
• device_identifier stores a SHA-256 hash, not the raw cookie token. Size varchar(255) is sufficient for hex-encoded SHA-256 (64 chars).
• The OTP table (oauth2_otp) already has phone_number and connection='sms' columns from existing infrastructure. Do NOT modify this table.
• two_factor_method should be stored as a string column, not a database-level ENUM, to allow adding values in Phase II/III without migration.

Out of scope:

User entity PHP methods (separate ticket), service layer code, UI changes.

Summary by CodeRabbit

• New Features

  • 2FA foundation: trusted device management (trust/expire/revoke), single‑use recovery codes, and detailed 2FA audit logging with event/method types and user-scoped queries.
  • Added repository support and interfaces for trusted devices, recovery codes, and 2FA audit logs.

• Tests

  • Comprehensive tests for trusted-device behavior, recovery-code lifecycle/deletion, audit-log ordering, and device-identifier uniqueness.

• Chores

  • Database migrations to add 2FA columns/tables and enforce unique (user, device) identifiers.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds Phase I two-factor: three Doctrine entities (trusted devices, audit logs, recovery codes), repository interfaces and Doctrine implementations, provider bindings, two migrations (schema + unique index), and integration tests exercising repository behavior.

Changes

Two-factor foundation

Layer / File(s)	Summary
Entities & domain models app/libs/Auth/Models/TwoFactorAuditLog.php, app/libs/Auth/Models/UserRecoveryCode.php, app/libs/Auth/Models/UserTrustedDevice.php	New Doctrine entities mapped to two_factor_audit_log, user_recovery_codes, and user_trusted_devices with fields, relations to users, lifecycle initialization, getters/setters, validation (allowlists and bcrypt check), and repositoryClass hints.
Repository contracts app/libs/Auth/Repositories/ITwoFactorAuditLogRepository.php, app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php, app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php	New interfaces declaring repository operations: getRecentByUser(User,int): array, getUnusedByUser(User): array, deleteAllForUser(User): int, getActiveByUserAndIdentifier(User,string): ?UserTrustedDevice, and getActiveByUser(User): array.
Doctrine repository implementations app/Repositories/DoctrineTwoFactorAuditLogRepository.php, app/Repositories/DoctrineUserRecoveryCodeRepository.php, app/Repositories/DoctrineUserTrustedDeviceRepository.php	Concrete repositories wired to their entity classes: recent-audit query ordered by created_at desc; retrieval and DQL deletion of recovery codes; active trusted-device queries enforcing is_revoked = false and expires_at > now (UTC).
Service provider wiring app/Repositories/RepositoriesProvider.php	Container bindings registered for the three repository interfaces to their Doctrine repositories and added to the provider provides() list for deferred resolution.
Migrations database/migrations/Version20260416194357.php, database/migrations/Version20260424120000.php	Migration adding 2FA columns to users and creating user_trusted_devices, two_factor_audit_log, user_recovery_codes; separate migration to convert the user/device index into a unique index after checking for duplicates.
Integration tests tests/TwoFactorRepositoriesTest.php	Repository integration tests: trusted device round-trips, exclusion of expired/revoked devices from active queries, uniqueness index assertion, audit-log ordering and retrieval, recovery-code lifecycle (unused, mark used, deleteAllForUser), and validation on double-use.

sequenceDiagram
    rect rgba(100,150,240,0.5)
    participant Client
    end
    rect rgba(120,200,160,0.5)
    participant Repository
    end
    rect rgba(200,100,140,0.5)
    participant EntityManager
    end
    rect rgba(240,200,100,0.5)
    participant Database
    end

    Client->>Repository: persistTrustedDevice(data)
    Repository->>EntityManager: persist(UserTrustedDevice)
    EntityManager->>Database: INSERT user_trusted_devices
    Database-->>EntityManager: OK
    EntityManager-->>Repository: persisted entity

    Client->>Repository: getActiveByUserAndIdentifier(user, id)
    Repository->>EntityManager: createQuery(filter active conditions)
    EntityManager->>Database: SELECT ... WHERE user_id=? AND device_identifier=? AND is_revoked=0 AND expires_at > now()
    Database-->>EntityManager: rows
    EntityManager-->>Repository: entity/NULL
    Repository-->>Client: entity/NULL

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

• smarcet

Poem

🐰
I hopped through tables, logs, and codes,
Marked trusted pawprints on safe roads,
I logged each challenge with timestamp bright,
Hid recovery seeds out of sight,
Two-factor blooms — a rabbit's delight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.86% 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 accurately and concisely summarizes the main objective: adding Multi-Factor Authentication foundation through migrations and entities.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/mfa-phase1---migrations--and--interfaces

---

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.}

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (6)

app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php (1)

25-28: Consider documenting transaction/atomicity expectations for deleteAllForUser.

Since this is typically invoked during recovery-code regeneration (delete-then-insert), callers likely need the operation wrapped in a transaction with the subsequent re-insert to avoid leaving a user with zero recovery codes on partial failure. Document whether callers must manage the transaction themselves, or have the implementation wrap it. A one-line docblock addition is sufficient.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php` around lines 25 -
28, Add a one-line sentence to the deleteAllForUser(User $user): int docblock
clarifying transaction/atomicity expectations: state whether implementations
will wrap this operation in a database transaction or whether callers must
perform a surrounding transaction when doing delete-then-insert (e.g., "This
method does NOT start a transaction; callers should wrap delete-and-reinsert
operations in a transaction to ensure atomicity." or the converse if
implementations guarantee transactional semantics). Reference the method name
deleteAllForUser in the docblock so callers know the requirement.

app/Repositories/DoctrineUserRecoveryCodeRepository.php (1)

34-42: Consider flushing pending UoW changes or documenting caller responsibility.

Query::execute() on a DQL DELETE bypasses the UnitOfWork and runs directly against the DB. If a caller has pending in-memory changes on UserRecoveryCode entities for this user (e.g. newly persisted but not flushed, or a markUsed() pending flush), those will not be considered by the bulk delete and may re-surface on a later flush(), or conflict. Either document this (bulk operation; caller must flush/clear beforehand) or call $em->flush()/$em->clear(UserRecoveryCode::class) around the delete. Given the "used when regenerating" use case, a docblock note is likely sufficient.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Repositories/DoctrineUserRecoveryCodeRepository.php` around lines 34 -
42, The bulk DQL delete in deleteAllForUser bypasses the UnitOfWork and can
conflict with pending in-memory UserRecoveryCode changes; update the docblock
for DoctrineUserRecoveryCodeRepository::deleteAllForUser to state this is a bulk
operation and callers must flush or clear pending UserRecoveryCode changes
before calling, or alternatively call $em->flush() and/or
$em->clear(UserRecoveryCode::class) around the delete inside deleteAllForUser to
ensure UoW consistency (refer to getEntityManager(), UserRecoveryCode and the
Query::execute() bulk delete in your change).

app/libs/Auth/Models/UserRecoveryCode.php (1)

27-29: Minor: attribute order — JoinColumn is placed before ManyToOne.

Doctrine accepts either order, but the conventional pairing is #[ManyToOne] first, then #[JoinColumn], which matches Doctrine docs/examples and the surrounding codebase's style. Purely stylistic; no behavior change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/libs/Auth/Models/UserRecoveryCode.php` around lines 27 - 29, Swap the
attribute order on the $user property so #[ORM\ManyToOne(...)] appears before
#[ORM\JoinColumn(...)]; specifically update the attributes on the private $user
property (currently using #[ORM\JoinColumn(...)] then #[ORM\ManyToOne(...)] ) to
#[ORM\ManyToOne(targetEntity: \Auth\User::class)] followed by
#[ORM\JoinColumn(name: 'user_id', referencedColumnName: 'id', onDelete:
'CASCADE')], preserving the same arguments.

app/Repositories/DoctrineUserTrustedDeviceRepository.php (1)

35-41: Consider deterministic ordering for getActiveByUser.

No orderBy is specified, so results come back in storage order. For a user-facing "trusted devices" listing, ordering by last_seen_at DESC (or trusted_at DESC) produces a more predictable/useful listing and avoids flaky tests if ever asserting order.

♻️ Suggested change

     public function getActiveByUser(User $user): array
     {
         return $this->findBy([
             'user'       => $user,
             'is_revoked' => false,
-        ]);
+        ], ['last_seen_at' => 'DESC']);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/Repositories/DoctrineUserTrustedDeviceRepository.php` around lines 35 -
41, getActiveByUser currently returns results with no deterministic ordering;
update the repository method (getActiveByUser) to return active devices ordered
by most-recent activity (e.g. order by last_seen_at DESC or trusted_at DESC
depending on your entity field name) so the user-facing list and tests are
stable—use the Doctrine findBy third parameter for orderBy or switch to the
query builder in DoctrineUserTrustedDeviceRepository::getActiveByUser to apply
the DESC ordering on the appropriate timestamp field.

database/migrations/Version20260416194357.php (2)

33-39: Partial-state guard only checks one column.

The up() guard checks only two_factor_enabled. If a prior run failed after adding two_factor_enabled but before adding the other two columns (or vice versa in a manual intervention), subsequent runs will silently skip the missing columns. Consider guarding each column addition independently, e.g.:

-        if ($schema->hasTable("users") && !$builder->hasColumn("users", "two_factor_enabled")) {
-            $builder->table('users', function (Table $table) {
-                $table->boolean('two_factor_enabled')->setNotnull(true)->setDefault(false);
-                $table->string('two_factor_method', 32)->setNotnull(true)->setDefault('email_otp');
-                $table->dateTime('two_factor_enforced_at')->setNotnull(false)->setDefault(null);
-            });
-        }
+        if ($schema->hasTable("users")) {
+            $builder->table('users', function (Table $table) use ($builder) {
+                if (!$builder->hasColumn("users", "two_factor_enabled")) {
+                    $table->boolean('two_factor_enabled')->setNotnull(true)->setDefault(false);
+                }
+                if (!$builder->hasColumn("users", "two_factor_method")) {
+                    $table->string('two_factor_method', 32)->setNotnull(true)->setDefault('email_otp');
+                }
+                if (!$builder->hasColumn("users", "two_factor_enforced_at")) {
+                    $table->dateTime('two_factor_enforced_at')->setNotnull(false)->setDefault(null);
+                }
+            });
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@database/migrations/Version20260416194357.php` around lines 33 - 39, The
migration's up() currently gates adding all three columns behind a single check
for two_factor_enabled, so if a prior run added one column and failed the rest
will be skipped; change the logic to first verify $schema->hasTable("users") and
then individually check $builder->hasColumn("users", "<column_name>") for each
of the three columns (two_factor_enabled, two_factor_method,
two_factor_enforced_at) and add each missing column (using the existing
builder->table('users', function (Table $table) { ... }) block or separate
blocks) so each column is created idempotently even if previous runs partially
applied the migration.

---

55-58: Consider a UNIQUE index on (user_id, device_identifier).

DoctrineUserTrustedDeviceRepository::getActiveByUserAndIdentifier() is expected to return a single row for a given (user, device_identifier) pair, and device_identifier is a SHA-256 hex string (globally unique per device/token). A plain index allows duplicate rows to accumulate (e.g., re-trusting the same device on retries) and makes the "single active device" contract unenforceable at the DB layer. A unique constraint would prevent silent duplication and align the schema with the repository's semantics.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@database/migrations/Version20260416194357.php` around lines 55 - 58, Replace
the non-unique index on (user_id, device_identifier) with a UNIQUE constraint so
the DB enforces one row per (user_id, device_identifier) pair; update the
migration where $table->index(["user_id", "device_identifier"],
"utd_user_device_idx") is declared in Version20260416194357.php to create a
unique index/constraint (e.g. using $table->unique(...) or the DBAL equivalent)
with a clear name like "utd_user_device_unique", and remove or replace the
existing plain index to avoid duplicate-key conflicts; this aligns the schema
with DoctrineUserTrustedDeviceRepository::getActiveByUserAndIdentifier()
expectations.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/libs/Auth/Models/UserRecoveryCode.php`:
- Line 66: The magic __get method on class UserRecoveryCode exposes
private/protected properties (notably code_hash) and bypasses the explicit
getters; replace it with a safe approach by removing the passthrough or
implementing an allow-list: in UserRecoveryCode::__get only return values for an
explicit whitelist of non-sensitive keys (e.g., id, user_id, created_at) or
throw/return null for anything else, so sensitive fields like code_hash are not
accessible via property access while preserving legacy compatibility for
approved fields.

In `@app/libs/Auth/Models/UserTrustedDevice.php`:
- Line 85: Remove the magic __get() passthrough from the UserTrustedDevice
entity: delete the public function __get($name) { return $this->{$name}; } so
callers no longer bypass explicit accessors (e.g. getIsRevoked(), isRevoked(),
getUser()) and Doctrine proxies fire their lazy-load hooks; update any call
sites that relied on property-style access to use the declared getters instead
to preserve type casting and avoid silent nulls for missing properties.

In `@tests/TwoFactorRepositoriesTest.php`:
- Around line 37-41: The test currently grabs an arbitrary real user via
IUserRepository->findOneBy([]) in setUp and testRecoveryCodeRoundTrip calls
RecoveryCodeRepository->deleteAllForUser($this->user), risking deletion of real
users' data; change setUp to create (and persist) a dedicated test user or
select a user explicitly marked for testing, then use that user object for the
test, and in tearDown remove that test user and assert deleteAllForUser removed
exactly the number of recovery codes inserted by the test (or call delete only
for the specific created entities) so that testRecoveryCodeRoundTrip no longer
wipes production/seeding data. Ensure you update any references to findOneBy,
testRecoveryCodeRoundTrip, deleteAllForUser, setUp and tearDown accordingly.

---

Nitpick comments:
In `@app/libs/Auth/Models/UserRecoveryCode.php`:
- Around line 27-29: Swap the attribute order on the $user property so
#[ORM\ManyToOne(...)] appears before #[ORM\JoinColumn(...)]; specifically update
the attributes on the private $user property (currently using
#[ORM\JoinColumn(...)] then #[ORM\ManyToOne(...)] ) to
#[ORM\ManyToOne(targetEntity: \Auth\User::class)] followed by
#[ORM\JoinColumn(name: 'user_id', referencedColumnName: 'id', onDelete:
'CASCADE')], preserving the same arguments.

In `@app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php`:
- Around line 25-28: Add a one-line sentence to the deleteAllForUser(User
$user): int docblock clarifying transaction/atomicity expectations: state
whether implementations will wrap this operation in a database transaction or
whether callers must perform a surrounding transaction when doing
delete-then-insert (e.g., "This method does NOT start a transaction; callers
should wrap delete-and-reinsert operations in a transaction to ensure
atomicity." or the converse if implementations guarantee transactional
semantics). Reference the method name deleteAllForUser in the docblock so
callers know the requirement.

In `@app/Repositories/DoctrineUserRecoveryCodeRepository.php`:
- Around line 34-42: The bulk DQL delete in deleteAllForUser bypasses the
UnitOfWork and can conflict with pending in-memory UserRecoveryCode changes;
update the docblock for DoctrineUserRecoveryCodeRepository::deleteAllForUser to
state this is a bulk operation and callers must flush or clear pending
UserRecoveryCode changes before calling, or alternatively call $em->flush()
and/or $em->clear(UserRecoveryCode::class) around the delete inside
deleteAllForUser to ensure UoW consistency (refer to getEntityManager(),
UserRecoveryCode and the Query::execute() bulk delete in your change).

In `@app/Repositories/DoctrineUserTrustedDeviceRepository.php`:
- Around line 35-41: getActiveByUser currently returns results with no
deterministic ordering; update the repository method (getActiveByUser) to return
active devices ordered by most-recent activity (e.g. order by last_seen_at DESC
or trusted_at DESC depending on your entity field name) so the user-facing list
and tests are stable—use the Doctrine findBy third parameter for orderBy or
switch to the query builder in
DoctrineUserTrustedDeviceRepository::getActiveByUser to apply the DESC ordering
on the appropriate timestamp field.

In `@database/migrations/Version20260416194357.php`:
- Around line 33-39: The migration's up() currently gates adding all three
columns behind a single check for two_factor_enabled, so if a prior run added
one column and failed the rest will be skipped; change the logic to first verify
$schema->hasTable("users") and then individually check
$builder->hasColumn("users", "<column_name>") for each of the three columns
(two_factor_enabled, two_factor_method, two_factor_enforced_at) and add each
missing column (using the existing builder->table('users', function (Table
$table) { ... }) block or separate blocks) so each column is created
idempotently even if previous runs partially applied the migration.
- Around line 55-58: Replace the non-unique index on (user_id,
device_identifier) with a UNIQUE constraint so the DB enforces one row per
(user_id, device_identifier) pair; update the migration where
$table->index(["user_id", "device_identifier"], "utd_user_device_idx") is
declared in Version20260416194357.php to create a unique index/constraint (e.g.
using $table->unique(...) or the DBAL equivalent) with a clear name like
"utd_user_device_unique", and remove or replace the existing plain index to
avoid duplicate-key conflicts; this aligns the schema with
DoctrineUserTrustedDeviceRepository::getActiveByUserAndIdentifier()
expectations.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 672e540a-70f5-47fd-8667-ab3e07d97782

📥 Commits

Reviewing files that changed from the base of the PR and between db9f777 and 6a2bd34.

📒 Files selected for processing (12)

• app/Repositories/DoctrineTwoFactorAuditLogRepository.php
• app/Repositories/DoctrineUserRecoveryCodeRepository.php
• app/Repositories/DoctrineUserTrustedDeviceRepository.php
• app/Repositories/RepositoriesProvider.php
• app/libs/Auth/Models/TwoFactorAuditLog.php
• app/libs/Auth/Models/UserRecoveryCode.php
• app/libs/Auth/Models/UserTrustedDevice.php
• app/libs/Auth/Repositories/ITwoFactorAuditLogRepository.php
• app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php
• app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php
• database/migrations/Version20260416194357.php
• tests/TwoFactorRepositoriesTest.php

[martinquiroga-exo]

@matiasperrone-exo please see comments. Thanks

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[matiasperrone-exo]

@martinquiroga-exo please review again

[martinquiroga-exo]

@matiasperrone-exo please see comments

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

app/libs/Auth/Models/UserTrustedDevice.php (1)

19-20: Add UniqueConstraint annotation to mirror the database migration.

The migration enforces uniqueness on (user_id, device_identifier) via UNIQUE INDEX utd_user_device_uniq, but the entity metadata declares no corresponding UniqueConstraint. This drift causes schema validation mismatches and lets metadata-driven tools miss the constraint. Add a #[ORM\UniqueConstraint(name: 'utd_user_device_uniq', columns: ['user_id', 'device_identifier'])] attribute to the #[ORM\Table] declaration.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/libs/Auth/Models/UserTrustedDevice.php` around lines 19 - 20, The ORM
entity for UserTrustedDevice is missing the UniqueConstraint that mirrors the DB
migration; update the #[ORM\Table(...)] attribute on the UserTrustedDevice class
to include #[ORM\UniqueConstraint(name: 'utd_user_device_uniq', columns:
['user_id', 'device_identifier'])] so the entity metadata matches the migration
(ensure the attribute is added alongside the existing name and repositoryClass
in the class declaration for UserTrustedDevice).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/libs/Auth/Models/TwoFactorAuditLog.php`:
- Around line 22-35: The setters for audit event/method must validate input
against the allowed constants to prevent typos from being persisted; update the
TwoFactorAuditLog class to add validation in the relevant setters (e.g., the
methods that set event and method values) by building arrays of allowed values
from the constants (Event* and Method*) and checking inputs with an explicit
membership test (throw a ValidationException/InvalidArgumentException or return
an error instead of accepting unknown strings), and apply the same guard to the
other setters referenced around lines 75-79 so only the defined constants can be
stored (also consider adding a DB constraint/migration as a follow-up).

In `@database/migrations/Version20260424120000.php`:
- Around line 27-33: In Version20260424120000.php inside the up(Schema $schema)
method, guard the ALTER TABLE that replaces utd_user_device_idx with the UNIQUE
utd_user_device_uniq by first running a preflight duplicate check on
user_trusted_devices for (user_id, device_identifier) and aborting/messaging the
migration if any duplicates exist; if duplicates are found implement a
deterministic dedupe/backfill step (e.g. keep the most recent row per (user_id,
device_identifier) or copy conflicting rows to a retention table and delete
extras) before executing the ALTER TABLE, and then proceed to DROP INDEX
utd_user_device_idx and ADD UNIQUE INDEX utd_user_device_uniq only after
deduplication completes successfully.

---

Nitpick comments:
In `@app/libs/Auth/Models/UserTrustedDevice.php`:
- Around line 19-20: The ORM entity for UserTrustedDevice is missing the
UniqueConstraint that mirrors the DB migration; update the #[ORM\Table(...)]
attribute on the UserTrustedDevice class to include #[ORM\UniqueConstraint(name:
'utd_user_device_uniq', columns: ['user_id', 'device_identifier'])] so the
entity metadata matches the migration (ensure the attribute is added alongside
the existing name and repositoryClass in the class declaration for
UserTrustedDevice).

🪄 Autofix (Beta)

❌ Autofix failed (check again to retry)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8e15f9c4-68e3-4df9-a2d3-f1e2af5e201f

📥 Commits

Reviewing files that changed from the base of the PR and between 6a2bd34 and 16e6ebb.

📒 Files selected for processing (6)

• app/Repositories/DoctrineUserTrustedDeviceRepository.php
• app/libs/Auth/Models/TwoFactorAuditLog.php
• app/libs/Auth/Models/UserRecoveryCode.php
• app/libs/Auth/Models/UserTrustedDevice.php
• database/migrations/Version20260424120000.php
• tests/TwoFactorRepositoriesTest.php

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[matiasperrone-exo]

Add UniqueConstraint annotation to mirror the database migration.

done

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found.

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[matiasperrone-exo]

@smarcet please review now.

• TwoFactorAuditLog now extends from BaseEntity

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[smarcet]

LGTM

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[github-actions]

📘 OpenAPI / Swagger preview

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

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

[Copilot]

Pull request overview

This PR lays the groundwork for Multi-Factor Authentication (MFA/2FA) across the OpenStackID codebase by adding database schema, Doctrine entities/repositories, service-layer components (challenge strategy, device trust, audit logging, rate limiting), and expanding frontend/login/profile UX support with new unit and E2E coverage.

Changes:

• Add additive Doctrine migration for 2FA columns on users and new 2FA-related tables (trusted devices, audit log, recovery codes).
• Introduce backend 2FA services/strategies/middleware (challenge issuance/verification, rate limiting, device trust, recovery-code management, audit logging) and wire them into routes/service providers.
• Add/expand frontend components + Jest/Playwright test infrastructure to validate login/profile 2FA flows.

Reviewed changes

Copilot reviewed 139 out of 142 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
webpack.common.js	Adjust module resolution for JSX.
tests/unit/OAuth2LoginStrategyTest.php	Add coverage for OAuth2 postLogin behavior + facade isolation.
tests/unit/MFAGateServiceTest.php	Unit tests for MFA gate decision logic.
tests/unit/MFA/MFAChallengeStrategyFactoryTest.php	Unit tests for challenge strategy factory.
tests/unit/DisqusSSOProfileMappingTest.php	Make Disqus SSO persistence test idempotent.
tests/TurnstileProtectedControllersTest.php	Ensure Turnstile tests run over HTTPS when SSL middleware is enabled.
tests/OAuth2NativeMFALoginFlowTest.php	Integration test for MFA-required response shaping by display mode.
tests/js/validator/validator.test.js	Jest tests for shared validators.
tests/js/setup.js	Jest environment setup (TextEncoder/TextDecoder).
tests/js/profile/actions.test.js	Jest tests for profile action request layer.
tests/js/login/login.mfa.test.js	Jest unit tests for login MFA error handling.
tests/js/login/components/two-factor-form.test.js	Component tests for MFA OTP form UI states.
tests/js/components/two_factor_section.test.js	Component tests for enabling 2FA in profile section.
tests/js/components/recovery_codes_panel.test.js	Component tests for recovery-code panel behavior.
tests/js/components/recovery_code_modal.test.js	Component tests for recovery-code modal timing/locking.
tests/js/components/recovery_code_display.test.js	Component tests for copy/download recovery-code display.
tests/js/components/DividerWithText.test.js	Component tests for divider helper.
tests/js/components/CustomSnackbar.test.js	Component tests for snackbar behavior.
tests/js/components/Banner.test.js	Component tests for banner rendering (text/HTML).
tests/js/mocks/fileMock.js	Jest asset stub.
tests/e2e/utils/otp.ts	Helper to fetch real OTP for E2E runs.
tests/e2e/tsconfig.json	TS config for Playwright E2E suite.
tests/e2e/tests/auth/login.spec.ts	Playwright login smoke tests.
tests/e2e/pages/RegisterPage.ts	Playwright page object for registration.
tests/e2e/pages/LoginPage.ts	Playwright page object for login + MFA steps.
tests/e2e/fixtures/index.ts	Playwright fixtures + origin rewriting for dockerized runs.
tests/AuthServiceValidateCredentialsIntegrationTest.php	DB integration tests for validateCredentials behavior.
tests/AuthServiceLoginUserTest.php	Unit test asserting Auth::login ordering vs principal register.
storage/framework/cache/data/.gitignore	Remove ignore rules for storage cache path.
start_local_server.sh	Improve local bootstrap ordering; add e2e prerequisites.
routes/web.php	Add 2FA endpoints, apply 2FA rate-limit middleware, change cancel to POST.
resources/views/profile.blade.php	Expose 2FA/recovery-code endpoints + state to profile SPA.
resources/views/auth/login.blade.php	Expose MFA endpoints/state/countdown params to login SPA.
resources/js/utils.js	Add shared text-file download helper.
resources/js/signup/signup.js	Make Turnstile requirement conditional on configured key.
resources/js/shared/recovery_codes.js	Shared sessionStorage keys + defaults for recovery-code UX.
resources/js/shared/HTMLRender.js	Centralized DOMPurify-backed HTML rendering helper.
resources/js/profile/profile.module.scss	Add styles for recovery-code/2FA section wrapper.
resources/js/profile/profile.js	Render TwoFactorSection on profile page.
resources/js/profile/actions.js	Add enable2FA/regenerate recovery-code request helpers.
resources/js/login/login.module.scss	Add styles for MFA UI elements (countdown, disabled links, etc.).
resources/js/login/constants.js	Centralize login/MFA constants.
resources/js/login/components/use_otp_countdown.js	Shared countdown hook for OTP/MFA code expiry.
resources/js/login/components/two_factor_form.js	MFA OTP verify form component + resend cooldown.
resources/js/login/components/third_party_identity_providers.js	Extract third-party IdP buttons component.
resources/js/login/components/recovery_code_form.js	Recovery-code entry form for MFA recovery path.
resources/js/login/components/password_input_form.js	Refactor password step into standalone form component.
resources/js/login/components/otp_input_form.js	Refactor passwordless OTP step into standalone form component.
resources/js/login/components/otp_help_links.js	Extract OTP resend/cooldown UI for passwordless.
resources/js/login/components/otp_code_input.js	Shared OTP code input block for OTP + MFA flows.
resources/js/login/components/help_links.js	Consolidate contextual help links rendering.
resources/js/login/components/existing_account_actions.js	Extract “sign in with OTP” / reset-password actions.
resources/js/login/components/email_input_form.js	Refactor email step into standalone form component.
resources/js/login/components/email_error_actions.js	Extract email-step error CTA buttons.
resources/js/login/actions.js	Add MFA verify/resend/recovery/cancel request helpers.
resources/js/components/two_factor_section.js	Profile UI for enabling 2FA and showing recovery codes.
resources/js/components/recovery_codes.module.scss	Styles for recovery-code panel/modal/display.
resources/js/components/recovery_codes_panel.js	Recovery-code count, regenerate flow, low-code warning.
resources/js/components/recovery_code_modal.js	One-time modal to present newly generated codes.
resources/js/components/recovery_code_display.js	Display + copy/download for recovery codes.
resources/js/base_actions.js	Add postRawRequestFull helper returning {response}.
readme.md	Document backend/Jest/Playwright test workflows.
playwright.config.ts	Playwright config + report output.
phpunit.xml	Add dedicated “Two Factor Authentication” PHPUnit suite.
package.json	Add Jest/Playwright scripts + dev deps.
jest.config.js	Jest configuration for jsdom + module mapping + coverage.
docker-compose/playwright/Dockerfile	Playwright runner image with Docker CLI for OTP retrieval.
docker-compose.yml	Add Playwright service (profile e2e) + cache volume.
doc/adrs/0001-recovery-code-generation-not-triggered-by-group-enforcement.md	ADR documenting recovery-code generation trade-off.
database/migrations/Version20260416194357.php	Additive migration for 2FA columns and 2FA tables.
config/two_factor.php	New 2FA config (kill switch, enforced groups, rate limits, cookie).
config/session.php	Make session cookie secure/samesite env-configurable.
config/auth.php	Add recovery-code policy configuration.
config/app.php	Register TwoFactorServiceProvider.
babel.config.js	Add babel env config for Jest runs.
app/Strategies/OAuth2LoginStrategy.php	Add challengeRequired response path for OAuth2 flow.
app/Strategies/MFA/MFAChallengeStrategyFactory.php	Factory for selecting MFA challenge strategy by method.
app/Strategies/MFA/IMFAChallengeStrategy.php	Interface for MFA challenge strategies.
app/Strategies/MFA/EmailOTPMFAChallengeStrategy.php	Email OTP-based MFA challenge issuance/verification.
app/Strategies/MFA/AbstractMFAChallengeStrategy.php	Shared session pending-state + recovery-code verification.
app/Strategies/ILoginStrategy.php	Add MFA_REQUIRED constant + challengeRequired method.
app/Strategies/IDisplayResponseStrategy.php	Add getChallengeRequiredResponse contract.
app/Strategies/DisplayResponseUserAgentStrategy.php	Implement challengeRequired via redirect+session state.
app/Strategies/DisplayResponseJsonStrategy.php	Implement challengeRequired via JSON+412 response.
app/Strategies/DefaultLoginStrategy.php	Implement challengeRequired redirect behavior.
app/Services/Auth/TwoFactorServiceProvider.php	Register 2FA services and RateLimiter definitions.
app/Services/Auth/TwoFactorRateLimitService.php	Cache-backed 2FA rate-limit service.
app/Services/Auth/TwoFactorAuditService.php	Persist 2FA audit log + emit OTEL job attributes.
app/Services/Auth/RecoveryCodeService.php	Generate/regenerate recovery codes + audit logging.
app/Services/Auth/MFAGateService.php	Decide whether 2FA challenge is required (enforced + trust).
app/Services/Auth/ITwoFactorRateLimitService.php	Interface/constants for 2FA rate limit service.
app/Services/Auth/ITwoFactorGateService.php	Interface for 2FA gate decision logic.
app/Services/Auth/ITwoFactorAuditService.php	Interface for 2FA audit logger.
app/Services/Auth/IRecoveryCodeService.php	Interface for recovery-code management service.
app/Services/Auth/IDeviceTrustService.php	Interface for trusted-device management.
app/Services/Auth/DeviceTrustService.php	Implement trusted-device persistence and checks.
app/Repositories/RepositoriesProvider.php	Bind new 2FA Doctrine repositories.
app/Repositories/DoctrineUserTrustedDeviceRepository.php	Doctrine repo for trusted-device records.
app/Repositories/DoctrineUserRecoveryCodeRepository.php	Doctrine repo for recovery codes + locking + deletion.
app/Repositories/DoctrineTwoFactorAuditLogRepository.php	Doctrine repo for 2FA audit logs.
app/Providers/RouteServiceProvider.php	Remove old otp RateLimiter definition.
app/Providers/AppServiceProvider.php	Force HTTPS only when SSL_ENABLED is true.
app/libs/Utils/Services/IAuthService.php	Add MFA flow constants + MFA issue/verify/resend methods.
app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php	Repo interface for trusted devices.
app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php	Repo interface for recovery codes.
app/libs/Auth/Repositories/ITwoFactorAuditLogRepository.php	Repo interface for 2FA audit logs.
app/libs/Auth/Models/UserTrustedDevice.php	Doctrine entity for trusted devices.
app/libs/Auth/Models/UserRecoveryCode.php	Doctrine entity for recovery codes.
app/libs/Auth/Models/TwoFactorAuditLog.php	Doctrine entity for 2FA audit events.
app/Http/Middleware/TwoFactorRateLimitMiddleware.php	Middleware enforcing 2FA rate-limit semantics by action.
app/Http/Middleware/EncryptCookies.php	Exclude device trust cookie from encryption via config.
app/Http/Kernel.php	Register 2fa.rate route middleware alias.
app/Http/Controllers/Traits/MFACookieManager.php	Trait to read/queue trusted-device cookie token.
app/Http/Controllers/Traits/JsonResponses.php	Use HTTP status constants; add unauthorized helper.
app/Http/Controllers/Auth/RegisterController.php	Make Turnstile rule conditional on configured secret.
app/Http/Controllers/Api/UserApiController.php	Add enableTwoFactor + regenerateRecoveryCodes endpoints.
app/Console/Kernel.php	Register new artisan commands for E2E support.
app/Console/Commands/GetLatestOtp.php	Command to print latest pending OTP for an email.
app/Console/Commands/CreateRawUser.php	Command to create a plain verified user (E2E helper).
app/Console/Commands/CreateOAuth2TestClient.php	Command to seed a non-destructive OAuth2 E2E client.
.gitignore	Ignore Playwright/Jest outputs and misc local artifacts.
.github/workflows/push.yml	Add Turnstile secrets to backend workflow environment.
.github/workflows/push_frontend_tests.yml	New CI workflow for Jest + Playwright on push.
.github/workflows/pull_request_unit_tests.yml	Add Turnstile secrets to PR unit-test workflow environment.
.github/workflows/pull_request_frontend_tests.yml	New CI workflow for Jest + Playwright on PRs.

Suppressed comments (1)

storage/framework/cache/data/.gitignore:1

• Removing this .gitignore can cause generated cache files under storage/framework/cache/data/ to show up as untracked changes and accidentally get committed.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.