Feature | Add MultiFactor Authentication. Initial migrations and entities#123
Feature | Add MultiFactor Authentication. Initial migrations and entities#123matiasperrone-exo wants to merge 1 commit 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
📝 WalkthroughWalkthroughThis PR introduces a comprehensive two-factor authentication infrastructure consisting of three new Doctrine ORM entities (TwoFactorAuditLog, UserRecoveryCode, UserTrustedDevice), their corresponding repository interfaces and implementations, a database migration creating the necessary tables, and integration tests validating 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 docstrings
🧪 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-123/ This page is automatically updated on each push to this PR. |
|
Only repository collaborators, contributors, or members can run CodeRabbit commands. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
database/migrations/Version20260416194357.php (2)
37-37:setDefault(null)on a nullable DateTime is redundant and may emitDEFAULT NULLexplicitly.Nullable columns default to NULL without an explicit default clause. Dropping
->setDefault(null)avoids any DBAL quirks with DateTime defaults across platforms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/Version20260416194357.php` at line 37, Remove the explicit ->setDefault(null) on the two_factor_enforced_at DateTime column definition; keep it nullable via ->setNotnull(false) (or equivalent) and let the column default to NULL implicitly to avoid emitting an explicit DEFAULT NULL and DBAL DateTime quirks—look for the chain that references 'two_factor_enforced_at', 'setNotnull' and 'setDefault' and delete the setDefault(null) call.
33-39: Idempotency check only covers one column.The guard checks
!hasColumn("users", "two_factor_enabled")and then adds all three columns inside the closure. If a previous partial run (or manual DBA step) already addedtwo_factor_enabledbut nottwo_factor_method/two_factor_enforced_at— or vice versa — this migration will either skip needed work or fail when re-adding an existing column. Consider guarding each column independently:🛠️ Suggested per-column guard
- 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); + } + }); + }🤖 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 currently checks only !hasColumn("users","two_factor_enabled") before adding three columns, which breaks idempotency if some columns already exist; update the logic to check each column individually (use $builder->hasColumn('users', 'two_factor_enabled'), hasColumn('users','two_factor_method'), hasColumn('users','two_factor_enforced_at')) and only add the specific missing columns to the users table (call $builder->table('users', ...) or separate closures accordingly) so you never try to re-create an existing column.app/libs/Auth/Repositories/ITwoFactorAuditLogRepository.php (1)
18-24: Consider documenting/validating$limit.Callers passing
0or a negative value will propagate straight tofindBy. A docblock noting the expected range (or a guard in the implementation) would be a nice-to-have.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/libs/Auth/Repositories/ITwoFactorAuditLogRepository.php` around lines 18 - 24, The interface method ITwoFactorAuditLogRepository::getRecentByUser currently allows callers to pass 0 or negative $limit which will flow into underlying findBy calls; update the interface PHPDoc to document the expected valid range (e.g. positive integer) for $limit and add a guard in the implementation of getRecentByUser to normalize/validate $limit (throw InvalidArgumentException or coerce to a minimum of 1 or default 50) before delegating to findBy; reference TwoFactorAuditLog return type and IBaseRepository usage so reviewers can locate and update the concrete repository implementation accordingly.app/Repositories/DoctrineUserRecoveryCodeRepository.php (1)
34-42: DQL bulk delete bypasses the UnitOfWork.
DELETEvia DQL does not trigger lifecycle callbacks and does not detach already-managedUserRecoveryCodeentities from theEntityManager. If a caller loaded codes viagetUnusedByUser()and then invokeddeleteAllForUser()within the same request, those entities remain managed (and stale) in the UoW, which can cause surprising behavior on subsequent flushes.Consider
$em->clear(UserRecoveryCode::class)after execute, or document this constraint at the interface level. Low priority since Phase I callers likely run this in isolation.🤖 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, deleteAllForUser performs a DQL bulk DELETE which bypasses the UnitOfWork and leaves any previously loaded UserRecoveryCode entities managed and stale (e.g. those returned by getUnusedByUser). After executing the delete query in deleteAllForUser, call the EntityManager->clear(UserRecoveryCode::class) to detach any managed UserRecoveryCode instances (or alternatively document this behavior on the repository interface); locate deleteAllForUser and the getEntityManager() usage to add the clear(UserRecoveryCode::class) call immediately after $qb->getQuery()->execute() so the UoW no longer holds stale entities.tests/TwoFactorRepositoriesTest.php (1)
130-136: Assert the unused-code query returns and then excludes the target code.
assertNotEmpty($unused)can pass because of unrelated seeded rows. Check that$idis in the unused result, then aftermarkUsed()flushes, assert that the same ID is no longer returned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/TwoFactorRepositoriesTest.php` around lines 130 - 136, The test currently only asserts $unused is non-empty which can be satisfied by other seeded rows; update the test around the getUnusedByUser() call to explicitly assert that the created code id ($id) is present in the $unused results (e.g. check for an element with id $id from getUnusedByUser($this->user)), then after calling markUsed() on the reloaded UserRecoveryCode and flushing, assert that the same id is no longer present in the results of getUnusedByUser($this->user); use the repository method getUnusedByUser, the entity UserRecoveryCode, and the $id variable to locate and perform these checks.
🤖 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 75-79: The setters setEventType(string $value) and
setMethod(string $value) currently accept any string; change them to validate
$value against the class's allowed constants (the EVENT_* and METHOD_* sets
declared in this model) and reject unknown values by throwing an
InvalidArgumentException (or similar) with a clear message. Implement validation
either by checking in_array($value, self::ALLOWED_EVENTS) /
self::ALLOWED_METHODS (or building the allowed list from the defined EVENT_* /
METHOD_* constants) before assigning to $this->event_type / $this->method so
typos or invalid inputs are rejected.
In `@app/libs/Auth/Models/UserRecoveryCode.php`:
- Around line 61-64: The markUsed method in UserRecoveryCode currently
overwrites the used_at timestamp on every call; change markUsed() so it is
idempotent by only setting $this->used_at to a new UTC DateTime when used_at is
null/empty (i.e., if ($this->used_at === null) { ... }), preserving the original
first-use timestamp on subsequent calls; reference the
UserRecoveryCode::markUsed method and the $used_at property when making this
change.
In `@app/libs/Auth/Models/UserTrustedDevice.php`:
- Around line 28-29: The UserTrustedDevice entity currently allows multiple
active rows per (user_id, device_identifier) which breaks the expectation of
getActiveByUserAndIdentifier() returning a single record; update the ORM mapping
on class UserTrustedDevice to add a UNIQUE constraint across (user_id,
device_identifier) (adjust the #[ORM\Table] or #[ORM\UniqueConstraint]
annotations) so the DB enforces uniqueness, and/or modify the creation flow
(where trusted devices are saved) to atomically revoke any existing active
device for that user+identifier before inserting (update repository method
getActiveByUserAndIdentifier() and the save/create logic to perform the
revoke-then-insert or to surface unique constraint violations). Ensure you touch
the UserTrustedDevice entity annotations and the repository/save path that calls
getActiveByUserAndIdentifier() to enforce this invariant.
In `@tests/TwoFactorRepositoriesTest.php`:
- Around line 36-40: The test TwoFactorRepositoriesTest currently reuses an
arbitrary seeded user via IUserRepository->findOneBy([]) and then calls
deleteAllForUser($this->user), risking deletion of shared fixtures; change the
setup to create an isolated test user (e.g., via the IUserRepository create
method or a factory) for each test or run the test inside a DB
transaction/rollback (use a RefreshDatabase/transaction wrapper) so
deleteAllForUser($this->user) only affects the per-test user; apply the same
change to the other occurrence around lines 138-139 where deleteAllForUser is
invoked.
- Around line 69-75: The repository methods getActiveByUserAndIdentifier and
getActiveByUser currently only check revocation; update their query logic to
also exclude devices whose expires_at is in the past (i.e., only return rows
where expires_at is NULL or expires_at > now) so both revocation and expiration
are enforced; then add unit tests that create a revoked device and an expired
device and assert they are not returned by getActiveByUserAndIdentifier and
getActiveByUser while a valid non-revoked/non-expired device is returned to
prevent regressions.
---
Nitpick comments:
In `@app/libs/Auth/Repositories/ITwoFactorAuditLogRepository.php`:
- Around line 18-24: The interface method
ITwoFactorAuditLogRepository::getRecentByUser currently allows callers to pass 0
or negative $limit which will flow into underlying findBy calls; update the
interface PHPDoc to document the expected valid range (e.g. positive integer)
for $limit and add a guard in the implementation of getRecentByUser to
normalize/validate $limit (throw InvalidArgumentException or coerce to a minimum
of 1 or default 50) before delegating to findBy; reference TwoFactorAuditLog
return type and IBaseRepository usage so reviewers can locate and update the
concrete repository implementation accordingly.
In `@app/Repositories/DoctrineUserRecoveryCodeRepository.php`:
- Around line 34-42: deleteAllForUser performs a DQL bulk DELETE which bypasses
the UnitOfWork and leaves any previously loaded UserRecoveryCode entities
managed and stale (e.g. those returned by getUnusedByUser). After executing the
delete query in deleteAllForUser, call the
EntityManager->clear(UserRecoveryCode::class) to detach any managed
UserRecoveryCode instances (or alternatively document this behavior on the
repository interface); locate deleteAllForUser and the getEntityManager() usage
to add the clear(UserRecoveryCode::class) call immediately after
$qb->getQuery()->execute() so the UoW no longer holds stale entities.
In `@database/migrations/Version20260416194357.php`:
- Line 37: Remove the explicit ->setDefault(null) on the two_factor_enforced_at
DateTime column definition; keep it nullable via ->setNotnull(false) (or
equivalent) and let the column default to NULL implicitly to avoid emitting an
explicit DEFAULT NULL and DBAL DateTime quirks—look for the chain that
references 'two_factor_enforced_at', 'setNotnull' and 'setDefault' and delete
the setDefault(null) call.
- Around line 33-39: The migration currently checks only
!hasColumn("users","two_factor_enabled") before adding three columns, which
breaks idempotency if some columns already exist; update the logic to check each
column individually (use $builder->hasColumn('users', 'two_factor_enabled'),
hasColumn('users','two_factor_method'),
hasColumn('users','two_factor_enforced_at')) and only add the specific missing
columns to the users table (call $builder->table('users', ...) or separate
closures accordingly) so you never try to re-create an existing column.
In `@tests/TwoFactorRepositoriesTest.php`:
- Around line 130-136: The test currently only asserts $unused is non-empty
which can be satisfied by other seeded rows; update the test around the
getUnusedByUser() call to explicitly assert that the created code id ($id) is
present in the $unused results (e.g. check for an element with id $id from
getUnusedByUser($this->user)), then after calling markUsed() on the reloaded
UserRecoveryCode and flushing, assert that the same id is no longer present in
the results of getUnusedByUser($this->user); use the repository method
getUnusedByUser, the entity UserRecoveryCode, and the $id variable to locate and
perform these checks.
🪄 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: 3ff776b1-55ed-47da-812e-4b2c36f918d2
📒 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
| public function getEventType(): string { return $this->event_type; } | ||
| public function setEventType(string $value): void { $this->event_type = $value; } | ||
|
|
||
| public function getMethod(): string { return $this->method; } | ||
| public function setMethod(string $value): void { $this->method = $value; } |
There was a problem hiding this comment.
Validate audit event and method values.
The class defines the allowed event/method constants, but the setters accept arbitrary strings. Reject unknown values here to avoid unclassifiable MFA audit rows from typos or caller bugs.
Proposed fix
- public function setEventType(string $value): void { $this->event_type = $value; }
+ public function setEventType(string $value): void
+ {
+ if (!in_array($value, [
+ self::EventChallengeIssued,
+ self::EventChallengeSucceeded,
+ self::EventChallengeFailed,
+ self::EventEnrollmentChanged,
+ self::EventDeviceTrusted,
+ self::EventDeviceRevoked,
+ self::EventRecoveryUsed,
+ self::EventSettingsChanged,
+ ], true)) {
+ throw new \InvalidArgumentException(sprintf('Unsupported two-factor audit event type "%s".', $value));
+ }
+
+ $this->event_type = $value;
+ }
public function getMethod(): string { return $this->method; }
- public function setMethod(string $value): void { $this->method = $value; }
+ public function setMethod(string $value): void
+ {
+ if (!in_array($value, [
+ self::MethodEmailOtp,
+ self::MethodSmsOtp,
+ self::MethodTotp,
+ self::MethodPasskey,
+ self::MethodRecovery,
+ ], true)) {
+ throw new \InvalidArgumentException(sprintf('Unsupported two-factor audit method "%s".', $value));
+ }
+
+ $this->method = $value;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/libs/Auth/Models/TwoFactorAuditLog.php` around lines 75 - 79, The setters
setEventType(string $value) and setMethod(string $value) currently accept any
string; change them to validate $value against the class's allowed constants
(the EVENT_* and METHOD_* sets declared in this model) and reject unknown values
by throwing an InvalidArgumentException (or similar) with a clear message.
Implement validation either by checking in_array($value, self::ALLOWED_EVENTS) /
self::ALLOWED_METHODS (or building the allowed list from the defined EVENT_* /
METHOD_* constants) before assigning to $this->event_type / $this->method so
typos or invalid inputs are rejected.
| public function markUsed(): void | ||
| { | ||
| $this->used_at = new \DateTime('now', new \DateTimeZone('UTC')); | ||
| } |
There was a problem hiding this comment.
Preserve the first-use timestamp.
markUsed() currently overwrites used_at on every call. For one-time recovery codes, keep this state transition idempotent so retries or duplicate handling do not erase the original usage time.
Proposed fix
public function markUsed(): void
{
+ if ($this->isUsed()) {
+ return;
+ }
$this->used_at = new \DateTime('now', new \DateTimeZone('UTC'));
}🤖 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 61 - 64, The markUsed
method in UserRecoveryCode currently overwrites the used_at timestamp on every
call; change markUsed() so it is idempotent by only setting $this->used_at to a
new UTC DateTime when used_at is null/empty (i.e., if ($this->used_at === null)
{ ... }), preserving the original first-use timestamp on subsequent calls;
reference the UserRecoveryCode::markUsed method and the $used_at property when
making this change.
| #[ORM\Column(name: 'device_identifier', type: 'string', length: 255)] | ||
| private $device_identifier; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository lookup implementation:"
rg -n -C4 'function\s+getActiveByUserAndIdentifier|getActiveByUserAndIdentifier\s*\('
echo
echo "Trusted-device schema indexes/constraints:"
rg -n -C4 'user_trusted_devices|utd_user_device_idx|device_identifier'Repository: OpenStackweb/openstackid
Length of output: 9908
🏁 Script executed:
cat -n app/libs/Auth/Models/UserTrustedDevice.php | head -80Repository: OpenStackweb/openstackid
Length of output: 3863
🏁 Script executed:
rg -n 'uniqueConstraints|UNIQUE|unique' app/libs/Auth/Models/UserTrustedDevice.phpRepository: OpenStackweb/openstackid
Length of output: 50
🏁 Script executed:
rg -n -B5 -A10 'new\s+UserTrustedDevice|setUser.*setDeviceIdentifier|addTrusted' --type phpRepository: OpenStackweb/openstackid
Length of output: 17008
🏁 Script executed:
git ls-files database/migrations/ | grep -i trustedRepository: OpenStackweb/openstackid
Length of output: 50
🏁 Script executed:
rg -n 'createTrustedDevice|addTrustedDevice|revokeBefore' --type phpRepository: OpenStackweb/openstackid
Length of output: 50
🏁 Script executed:
rg -n 'UserTrustedDevice' --type php | grep -i 'service\|repository\|factory' | head -20Repository: OpenStackweb/openstackid
Length of output: 2042
🏁 Script executed:
rg -n -B5 -A15 'class.*UserTrustedDevice.*Service|class.*UserTrustedDevice.*Factory' --type phpRepository: OpenStackweb/openstackid
Length of output: 50
🏁 Script executed:
rg -n -B3 -A10 'persist.*device|flush.*device' tests/ --type php | grep -A10 -B3 'UserTrustedDevice'Repository: OpenStackweb/openstackid
Length of output: 50
🏁 Script executed:
# Search for any service/class that might create UserTrustedDevice
fd -type f -name '*Service*.php' -o -name '*Factory*.php' | xargs rg -l 'UserTrustedDevice' 2>/dev/nullRepository: OpenStackweb/openstackid
Length of output: 493
🏁 Script executed:
cat -n tests/TwoFactorRepositoriesTest.phpRepository: OpenStackweb/openstackid
Length of output: 5977
🏁 Script executed:
rg -n 'is_revoked|revoke' app/libs/Auth/Models/UserTrustedDevice.php app/Repositories/DoctrineUserTrustedDeviceRepository.phpRepository: OpenStackweb/openstackid
Length of output: 795
🏁 Script executed:
# Check if there's any revoke-before-insert or conflict handling
rg -n -B5 -A10 'revoke.*device|device.*revoke' --type phpRepository: OpenStackweb/openstackid
Length of output: 2869
Add UNIQUE constraint for active trusted devices per user/identifier.
The schema defines only a non-unique index on (user_id, device_identifier), but getActiveByUserAndIdentifier() expects singular lookup and uses findOneBy() which returns only the first match. Multiple active rows for the same user and identifier will silently select an arbitrary device instead of failing loudly or enforcing single ownership. Add a UNIQUE constraint on (user_id, device_identifier) at the database level, or implement an application-level invariant (revoke previous active device before adding a new one) to prevent this security gap in MFA trusted-device handling.
🤖 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 28 - 29, The
UserTrustedDevice entity currently allows multiple active rows per (user_id,
device_identifier) which breaks the expectation of
getActiveByUserAndIdentifier() returning a single record; update the ORM mapping
on class UserTrustedDevice to add a UNIQUE constraint across (user_id,
device_identifier) (adjust the #[ORM\Table] or #[ORM\UniqueConstraint]
annotations) so the DB enforces uniqueness, and/or modify the creation flow
(where trusted devices are saved) to atomically revoke any existing active
device for that user+identifier before inserting (update repository method
getActiveByUserAndIdentifier() and the save/create logic to perform the
revoke-then-insert or to surface unique constraint violations). Ensure you touch
the UserTrustedDevice entity annotations and the repository/save path that calls
getActiveByUserAndIdentifier() to enforce this invariant.
| // Pull any persisted user; tests don't assert on user fields, only on FK linkage | ||
| $userRepo = App::make(IUserRepository::class); | ||
| $this->user = $userRepo->findOneBy([]); | ||
| if (is_null($this->user)) { | ||
| $this->markTestSkipped('No User exists; database must be seeded.'); |
There was a problem hiding this comment.
Use an isolated test user before calling broad deletes.
The test selects any seeded user, then deleteAllForUser($this->user) removes all recovery codes for that user. This can corrupt shared fixtures and make test order matter; create a dedicated user/fixture per test or wrap the test in a rollback transaction.
Also applies to: 138-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/TwoFactorRepositoriesTest.php` around lines 36 - 40, The test
TwoFactorRepositoriesTest currently reuses an arbitrary seeded user via
IUserRepository->findOneBy([]) and then calls deleteAllForUser($this->user),
risking deletion of shared fixtures; change the setup to create an isolated test
user (e.g., via the IUserRepository create method or a factory) for each test or
run the test inside a DB transaction/rollback (use a RefreshDatabase/transaction
wrapper) so deleteAllForUser($this->user) only affects the per-test user; apply
the same change to the other occurrence around lines 138-139 where
deleteAllForUser is invoked.
| $found = $repo->getActiveByUserAndIdentifier($this->user, $deviceId); | ||
| $this->assertNotNull($found); | ||
| $this->assertEquals($deviceId, $found->getDeviceIdentifier()); | ||
| $this->assertFalse($found->isRevoked()); | ||
|
|
||
| $active = $repo->getActiveByUser($this->user); | ||
| $this->assertNotEmpty($active); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C6 'function\s+getActiveByUser|expires_at|is_revoked' --iglob '*DoctrineUserTrustedDeviceRepository.php'Repository: OpenStackweb/openstackid
Length of output: 2028
🏁 Script executed:
#!/bin/bash
# Find UserTrustedDevice model definition
fd -type f -name '*.php' | xargs rg -l 'class UserTrustedDevice' | head -5Repository: OpenStackweb/openstackid
Length of output: 298
🏁 Script executed:
#!/bin/bash
# Check the UserTrustedDevice model for expires_at field
fd -type f -name '*.php' | xargs rg -A 20 'class UserTrustedDevice'Repository: OpenStackweb/openstackid
Length of output: 1819
🏁 Script executed:
#!/bin/bash
# Search for any usage of expires_at in the codebase related to trusted devices
rg -n 'expires_at' --type php | grep -i 'trusted\|device' | head -20Repository: OpenStackweb/openstackid
Length of output: 497
🏁 Script executed:
#!/bin/bash
# Check TwoFactorRepositoriesTest.php for current test coverage
cat -n tests/TwoFactorRepositoriesTest.php | head -100Repository: OpenStackweb/openstackid
Length of output: 4335
🏁 Script executed:
#!/bin/bash
# Search for any expiration validation logic in the codebase
rg -n 'expires_at\|getExpiresAt\|isExpired' app/ | head -30Repository: OpenStackweb/openstackid
Length of output: 50
Critical issue: Repository methods do not filter expired trusted devices.
The getActiveByUserAndIdentifier() and getActiveByUser() methods filter only by revocation status, completely ignoring the expires_at field. Since trusted devices gate MFA bypass, expired devices that should be inactive remain active. The repository implementation must filter expires_at alongside is_revoked. Additionally, add test cases covering expired and revoked device exclusion to prevent regression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/TwoFactorRepositoriesTest.php` around lines 69 - 75, The repository
methods getActiveByUserAndIdentifier and getActiveByUser currently only check
revocation; update their query logic to also exclude devices whose expires_at is
in the past (i.e., only return rows where expires_at is NULL or expires_at >
now) so both revocation and expiration are enforced; then add unit tests that
create a revoked device and an expired device and assert they are not returned
by getActiveByUserAndIdentifier and getActiveByUser while a valid
non-revoked/non-expired device is returned to prevent regressions.
matiasperrone-exo
changed the title
1 participant
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.