Feature | Add MultiFactor Authentication. Initial migrations and entities#125
Feature | Add MultiFactor Authentication. Initial migrations and entities#125matiasperrone-exo wants to merge 2 commits intomainfrom
Conversation
feat(2fa): add migration for 2FA schema foundation (Phase I) feat(2fa): add UserTrustedDevice entity feat(2fa): add TwoFactorAuditLog entity feat(2fa): add UserRecoveryCode entity feat(2fa): add repository interfaces for 2FA entities feat(2fa): add Doctrine implementations for 2FA repositories feat(2fa): register 2FA repositories in service container test(2fa): add repository round-trip tests for 2FA entities
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request introduces a foundational two-factor authentication (2FA) infrastructure by adding three new Doctrine ORM entities for managing trusted devices, audit logging, and recovery codes, along with their corresponding repository interfaces and implementations. A database migration creates the supporting tables, and integration tests validate the persistence layer. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-125/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
app/libs/Auth/Repositories/IUserRecoveryCodeRepository.php (1)
25-28: Consider documenting transaction/atomicity expectations fordeleteAllForUser.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 DQLDELETEbypasses the UnitOfWork and runs directly against the DB. If a caller has pending in-memory changes onUserRecoveryCodeentities for this user (e.g. newly persisted but not flushed, or amarkUsed()pending flush), those will not be considered by the bulk delete and may re-surface on a laterflush(), 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 —JoinColumnis placed beforeManyToOne.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 forgetActiveByUser.No
orderByis specified, so results come back in storage order. For a user-facing "trusted devices" listing, ordering bylast_seen_at DESC(ortrusted_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 onlytwo_factor_enabled. If a prior run failed after addingtwo_factor_enabledbut 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, anddevice_identifieris 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
📒 Files selected for processing (12)
app/Repositories/DoctrineTwoFactorAuditLogRepository.phpapp/Repositories/DoctrineUserRecoveryCodeRepository.phpapp/Repositories/DoctrineUserTrustedDeviceRepository.phpapp/Repositories/RepositoriesProvider.phpapp/libs/Auth/Models/TwoFactorAuditLog.phpapp/libs/Auth/Models/UserRecoveryCode.phpapp/libs/Auth/Models/UserTrustedDevice.phpapp/libs/Auth/Repositories/ITwoFactorAuditLogRepository.phpapp/libs/Auth/Repositories/IUserRecoveryCodeRepository.phpapp/libs/Auth/Repositories/IUserTrustedDeviceRepository.phpdatabase/migrations/Version20260416194357.phptests/TwoFactorRepositoriesTest.php
martinquiroga-exo
left a comment
martinquiroga-exo
left a comment
There was a problem hiding this comment.
@matiasperrone-exo please see comments. Thanks
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-125/ This page is automatically updated on each push to this PR. |
matiasperrone-exo
force-pushed
the
feat/mfa-phase1---migrations--and--interfaces
branch
from
bd6e93f to
f75c63d
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-125/ This page is automatically updated on each push to this PR. |
matiasperrone-exo
force-pushed
the
feat/mfa-phase1---migrations--and--interfaces
branch
from
f75c63d to
4624ff5
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-125/ This page is automatically updated on each push to this PR. |
2 participants
Task:
Ref: https://app.clickup.com/t/86b9e8wm7
Changes
UserTrustedDeviceentityTwoFactorAuditLogentityUserRecoveryCodeentityRequested 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
(datetime, nullable)
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)
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)
ACCEPTANCE CRITERIA
php artisan doctrine:migrations:migrateruns without errors on both fresh and populated databasesAll 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:
app/libs/Auth/Models/UserTrustedDevice.phpapp/libs/Auth/Models/TwoFactorAuditLog.phpapp/libs/Auth/Models/UserRecoveryCode.phpdatabase/migrations/Version*.php(single migration file)Gotchas
Userentity uses Doctrine ORM, not Eloquent. All mappings follow the existing annotation/attribute pattern in the codebase.varchar(255)is sufficient for hex-encoded SHA-256 (64 chars).oauth2_otp) already hasphone_numberandconnection='sms'columns from existing infrastructure. Do NOT modify this table.two_factor_methodshould 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
Tests
Chores