Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Device retrieval and revocation app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php, app/Repositories/DoctrineUserTrustedDeviceRepository.php, app/Services/Auth/IDeviceTrustService.php, app/Services/Auth/DeviceTrustService.php, tests/DeviceTrustServiceTest.php |
The service retrieves active devices and revokes devices scoped to their owner. Successful revocations are audited individually; audit failures are logged. Tests cover bulk and individual revocation, ownership, idempotency, and audit failures. |
Trusted-device API and serialization app/ModelSerializers/Auth/UserTrustedDeviceSerializer.php, app/ModelSerializers/SerializerRegistry.php, app/Http/Controllers/Api/UserApiController.php, app/Http/Controllers/Traits/MFACookieManager.php, routes/web.php, tests/TrustedDevicesApiTest.php, tests/TwoFactorProfileRoutesCsrfTest.php |
Authenticated routes list active devices and revoke one or all devices. Responses identify the current device without exposing its identifier or raw user agent. Revocation expires the trust cookie when the current device is among those revoked. |
Profile controls and client actions resources/js/profile/actions.js, resources/views/profile.blade.php, resources/js/profile/profile.js, resources/js/components/trusted_devices_section.js, tests/js/profile/actions.test.js, tests/js/components/trusted_devices_section.test.js |
The profile displays trusted-device details and revocation controls when two-factor authentication is enabled. Client actions call the list and revocation endpoints. Tests cover loading, display, confirmation, and revocation results. |
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant TrustedDevicesSection
participant UserApiController
participant DeviceTrustService
participant TrustedDeviceRepository
participant AuditLogger
TrustedDevicesSection->>UserApiController: Request device list or revocation
UserApiController->>DeviceTrustService: Retrieve or revoke devices for authenticated user
DeviceTrustService->>TrustedDeviceRepository: Read active devices or owner-scoped device
TrustedDeviceRepository-->>DeviceTrustService: Return matching device records
DeviceTrustService->>TrustedDeviceRepository: Persist device revocations
DeviceTrustService->>AuditLogger: Attempt per-device revocation audit
DeviceTrustService-->>UserApiController: Return devices or revocation result
UserApiController-->>TrustedDevicesSection: Return serialized devices or deleted response
Suggested reviewers: matiasperrone-exo, smarcet
Merge Risk: 
e1547
-
The reviewed trusted-device listing and revocation paths have no established issue requiring a fix before merge. The reported end-to-end test failure was not independently established as a regression from this change.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
-
❌ Failed checks (1 warning)
Check name Status Explanation Resolution Docstring Coverage ⚠️ WarningDocstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 65 functions across 18 files. 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 clearly and concisely describes the main changes: trusted-device management UI and revocation endpoints for 2FA. 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.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Fix failing CI checks
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
ℹ️ Recent review info
-
⚙️ Run configuration
-
Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID: ab057271-6d7b-42ff-b8a4-4e63e06a5ce1
📒 Files selected for processing (18)
-
app/Http/Controllers/Api/UserApiController.phpapp/Http/Controllers/Traits/MFACookieManager.phpapp/ModelSerializers/Auth/UserTrustedDeviceSerializer.phpapp/ModelSerializers/SerializerRegistry.phpapp/Repositories/DoctrineUserTrustedDeviceRepository.phpapp/Services/Auth/DeviceTrustService.phpapp/Services/Auth/IDeviceTrustService.phpapp/libs/Auth/Repositories/IUserTrustedDeviceRepository.phpresources/js/components/trusted_devices_section.jsresources/js/profile/actions.jsresources/js/profile/profile.jsresources/views/profile.blade.phproutes/web.phptests/DeviceTrustServiceTest.phptests/TrustedDevicesApiTest.phptests/TwoFactorProfileRoutesCsrfTest.phptests/js/components/trusted_devices_section.test.jstests/js/profile/actions.test.js
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
ℹ️ Recent review info
-
⚙️ Run configuration
-
Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID:
ab057271-6d7b-42ff-b8a4-4e63e06a5ce1
📒 Files selected for processing (18)
-
app/Http/Controllers/Api/UserApiController.phpapp/Http/Controllers/Traits/MFACookieManager.phpapp/ModelSerializers/Auth/UserTrustedDeviceSerializer.phpapp/ModelSerializers/SerializerRegistry.phpapp/Repositories/DoctrineUserTrustedDeviceRepository.phpapp/Services/Auth/DeviceTrustService.phpapp/Services/Auth/IDeviceTrustService.phpapp/libs/Auth/Repositories/IUserTrustedDeviceRepository.phpresources/js/components/trusted_devices_section.jsresources/js/profile/actions.jsresources/js/profile/profile.jsresources/views/profile.blade.phproutes/web.phptests/DeviceTrustServiceTest.phptests/TrustedDevicesApiTest.phptests/TwoFactorProfileRoutesCsrfTest.phptests/js/components/trusted_devices_section.test.jstests/js/profile/actions.test.js
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
-
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-160/ This page is automatically updated on each push to this PR. |
|
@smarcet the e2e job on this PR fails, but the failure is not caused by this PR. It comes from the base branch: Symptom: every login e2e test ( Root cause (from the Playwright trace artifact): the page is served from
- if (Config::get('server.ssl_enabled', false))
+ if (!App::isLocal())
URL::forceScheme('https');I understand the Some options that would work for both:
Which one do you prefer? I can open a PR against the base branch.
The other checks on this PR are green, including |
2d489c3 to
d6c7ac2
Compare
50cbd8b to
3bdab76
Compare
e154749 to
b7bc1b5
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-160/ This page is automatically updated on each push to this PR. |
| * Returns null when the id does not exist or belongs to another user. | ||
| */ | ||
| public function revokeAllForUser(User $user): void; | ||
| public function getByIdAndUser(int $id, User $user): ?UserTrustedDevice; |
There was a problem hiding this comment.
@romanetar Removing revokeAllForUser() from this interface (and from DoctrineUserTrustedDeviceRepository) breaks RecoveryCodeService::disableTwoFactor() in #169, which calls it inside its transaction (RecoveryCodeService.php:145), and with it idp:reset-2fa in #171. Both branches sit on the MFA branch independently, so git does not flag it: git merge-tree of #171 and this PR resolves with no conflicts, and the merged tree still calls revokeAllForUser while the interface and the repository no longer define it. The first idp:reset-2fa after both merge fails with "Call to undefined method".
The two PRs also disagree on audit semantics: this PR logs one device_revoked per device actually revoked (with device_id), while #169 logs a single row without device_id, even when there were no devices.
Suggested fix: keep one bulk revocation with one semantics. For example, replace revokeAllForUser() with revokeAllActiveForUser(User): UserTrustedDevice[], which revokes entity by entity (without opening its own transaction) and returns the devices. DeviceTrustService::removeTrustedDevices() and disableTwoFactor() both call it inside their transactions and log one device_revoked per returned device after commit. Whichever PR merges second adapts to the other; run phpunit on the combined tree before merging.
There was a problem hiding this comment.
Done, following the suggested fix.
- feat(2fa): trusted devices management UI and revoke endpoints #160 (d32fdf4):
revokeAllForUser()is replaced byrevokeAllActiveForUser(User): UserTrustedDevice[]. It revokes entity by entity, opens no transaction of its own and returns the devices.DeviceTrustService::removeTrustedDevices()calls it inside its transaction and logs onedevice_revoked(withdevice_id) per returned device after commit. Added a repository test. - feat(mfa): add IRecoveryCodeService::disableTwoFactor #169 (be0d1c4): same method, same implementation.
disableTwoFactor()andremoveTrustedDevices()use it inside their transactions and log onedevice_revokedwithdevice_idper revoked device, best-effort after commit. With no active devices, nodevice_revokedrow is written. Tests updated, plus one for the no-devices case.
Each branch passes its own tests. Merging now conflicts textually (repository, interface, DeviceTrustService and its test) instead of failing at runtime. Whichever merges second should keep a single revokeAllActiveForUser, and I'll run phpunit on the combined tree before merging.
| Route::get('actions', "UserActionApiController@getActionsByCurrentUser"); | ||
| Route::post('recovery-codes/regenerate', ['middleware' => ['csrf'], 'uses' => "UserApiController@regenerateRecoveryCodes"]); | ||
| Route::post('2fa/enable', ['middleware' => ['csrf'], 'uses' => "UserApiController@enableTwoFactor"]); | ||
| Route::get('2fa/devices', "UserApiController@getMyTrustedDevices"); |
There was a problem hiding this comment.
@romanetar These routes deviate from SDS sds/idp-mfa.md §5.6, which puts the device endpoints in a new Api\UserTwoFactorApiController under a users/me/2fa group. The PR description documents and justifies the choice (not splitting the 2FA profile resource across two controllers), so no code change is needed here, but the SDS now contradicts the code. The SDS §5.6 should be amended to adopt admin/api/v1/users/me and UserApiController for the profile 2FA endpoints; that amendment is being handled on the SDS side.
There was a problem hiding this comment.
Understood, no code change: the device endpoints stay in UserApiController under admin/api/v1/users/me, as documented in the PR description. I'll open the SDS amendment for §5.6 so it matches the code.
There was a problem hiding this comment.
Update: rather than opening a second SDS PR that would conflict with it, I commented on fntechgit/ftn-docsnsklz#179. That PR (rev 2.6) already rewrites §5.6 to match this code: UserApiController, the admin/api/v1/users/me group and the three device routes. My comment asks to add the two details from #160 and #169 that it does not cover yet: the revokeAllActiveForUser contract, and one device_revoked row per device in disableTwoFactor and idp:reset-2fa. https://github.com/fntechgit/ftn-docsnsklz/pull/179#issuecomment-6775610153
Add GET/DELETE /admin/api/v1/users/me/2fa/devices[/{id}] so a user can
list their active trusted devices and revoke one or all of them
(SDS idp-mfa.md 5.6, CU-86bc647cd).
- Revocation is scoped by owner: another user's device id returns 404.
- Revoking an already revoked or expired device is a 204 no-op and is
not audited; revoke-all logs one device_revoked event per device it
actually revokes and none when there are no active devices.
- Audit runs best-effort after the revocation commits, following
RecoveryCodeService: audit_service->log() opens its own transaction,
which cannot be nested.
- The device-trust cookie is expired when the revoked device is the one
the request came from.
- The list never exposes device_identifier or user_agent and flags the
current device via is_current; it does not touch last_seen_at.
- Replace the bulk revokeAllForUser() UPDATE with a per-entity revoke of
active devices so each revoked device can be audited.
Add a Trusted Devices subsection to the profile page (SDS idp-mfa.md 5.5, CU-86bc647cd), shown only when 2FA applies to the user. It lists the user's active trusted devices (name, IP, trusted, last seen, expiry), labels the current one, and lets the user revoke a single device or all of them (with a confirmation), updating the table in place. Buttons are plain onClick handlers since the profile page is a single <form>. Expose the three device endpoints on window from profile.blade.php and add getTrustedDevices / revokeTrustedDevice / revokeAllTrustedDevices helpers, covered by Jest tests for the component and the unmocked request layer.
b7bc1b5 to
a0cb900
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-160/ This page is automatically updated on each push to this PR. |
…ing the revoked devices The repository revokes entity by entity inside the caller's transaction and returns the devices, so callers can log one device_revoked per device after commit. Replaces the revokeAllForUser contract that other MFA branches rely on. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-160/ This page is automatically updated on each push to this PR. |
295c98b
into
feat/mfa-phase1---migrations--and--interfaces
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent revocations can emit duplicate audit events, while failed device-list loads falsely show no trusted devices and hide revocation controls.
2 open issues
What changed in this PR
Adds self-service trusted-device management to the existing two-factor authentication flow, letting users view and revoke trusted devices from their profile without ending their current session.
Changes:
- Adds authenticated listing and revocation endpoints with ownership checks, audit logging, and current-device cookie expiration.
- Adds a profile section with device details, individual revocation, and confirmed revoke-all.
- Extends backend and frontend coverage for device management and request handling.
| File | Description |
|---|---|
tests/TwoFactorRepositoriesTest.php |
Tests active-device bulk revocation. |
tests/TwoFactorProfileRoutesCsrfTest.php |
Checks CSRF middleware on DELETE routes. |
tests/TrustedDevicesApiTest.php |
Covers listing, ownership, revocation, auditing, and cookies. |
tests/js/profile/actions.test.js |
Tests trusted-device request helpers. |
tests/js/components/trusted_devices_section.test.js |
Covers rendering and revocation interactions. |
tests/DeviceTrustServiceTest.php |
Tests service revocation and audit-failure handling. |
routes/web.php |
Registers trusted-device endpoints. |
resources/views/profile.blade.php |
Exposes endpoint URLs to JavaScript. |
resources/js/profile/profile.js |
Mounts device management when 2FA is enabled. |
resources/js/profile/actions.js |
Adds listing and revocation request helpers. |
resources/js/components/trusted_devices_section.js |
Implements the trusted-device table and controls. |
app/Services/Auth/IDeviceTrustService.php |
Defines listing and revocation contracts. |
app/Services/Auth/DeviceTrustService.php |
Implements revocation with post-commit auditing. |
app/Repositories/DoctrineUserTrustedDeviceRepository.php |
Adds owner-scoped lookup and active-device revocation. |
app/ModelSerializers/SerializerRegistry.php |
Registers the trusted-device serializer. |
app/ModelSerializers/Auth/UserTrustedDeviceSerializer.php |
Serializes device details and current-device status. |
app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php |
Updates lookup and revocation repository contracts. |
app/Http/Controllers/Traits/MFACookieManager.php |
Adds trusted-device cookie expiration. |
app/Http/Controllers/Api/UserApiController.php |
Handles device listing, revocation, and cookie clearing. |
Review effort: Balanced
| if ($device->isRevoked() || $device->isExpired()) { | ||
| return $device; |
| const [devices, setDevices] = useState([]); | ||
| const [loaded, setLoaded] = useState(false); | ||
| const [busy, setBusy] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| getTrustedDevices().then(({response}) => { | ||
| setDevices(response?.data ?? []); | ||
| setLoaded(true); | ||
| }).catch((err) => { | ||
| setLoaded(true); | ||
| handleErrorResponse(err); | ||
| }); | ||
| }, []); |
ref https://app.clickup.com/t/86bc647cd (SDS idp-mfa.md 5.5 / 5.6)
Summary
Users could mark a device as trusted when completing a 2FA challenge, but had no way to see or revoke those devices: a lost or shared device kept bypassing the challenge until the cookie expired (30 days by default). This adds self-service listing and revocation, on the API and on the profile page.
Backend
New routes in the existing
admin/api/v1→users/megroup (session-authenticated,ssl+auth):GET/admin/api/v1/users/me/2fa/devices200{data: [...]}with the caller's active devicesDELETE/admin/api/v1/users/me/2fa/devices/{id}204(csrf)DELETE/admin/api/v1/users/me/2fa/devices204(csrf)UserTrustedDeviceSerializerreturnsid,device_name,ip_address,trusted_at,expires_at,last_seen_atandis_current(the device whose cookie came with the request). It never exposesdevice_identifieroruser_agent, and it reads through the repository, so it does not refreshlast_seen_atthe wayisDeviceTrusted()does.IUserTrustedDeviceRepository::getByIdAndUser()scopes the lookup by owner. Another user's device id is indistinguishable from a missing one (404), and the row is left untouched.device_revokedis logged only on a realfalse → truetransition ofis_revoked, one row per device.204no-op with no audit row.queueDeviceTrustCookie()(MFACookieManager::expireDeviceTrustCookie()), so the next login is challenged. The session itself stays active.removeTrustedDevices()now revokes active devices entity by entity inside a transaction and returns them. The old bulkrevokeAllForUser()DQLUPDATEis removed; its only caller was this method, and it could not tell which devices it revoked.Frontend
TrustedDevicesSection(resources/js/components/trusted_devices_section.js), mounted under the 2FA section of the profile page and shown only whentwoFactorEnabled:onClick(the profile page is a single<form>).windowfromprofile.blade.php; fetch helpers inresources/js/profile/actions.js.Deviations from the SDS / ticket
Api\UserApiControllerunderusers/me/2fainroutes/web.php, next to2fa/enableandrecovery-codes/regenerate, instead of the SDS's newApi\UserTwoFactorApiControllerinroutes/api.php. This avoids splitting the same resource across two controllers.302to/auth/login, not403: the group'sauthmiddleware rejects guests before the controller runs, same as the sibling routes. The ticket's acceptance criterion was updated accordingly.ITwoFactorAuditService::log()opens its own transaction, and nestingDoctrineTransactionService::transaction()calls is unsafe (seeRecoveryCodeService::enableTwoFactorAndGenerateCodes()). This follows the same pattern: an audit failure is logged as a warning and does not fail an already-committed revocation.flow = mfa; the actual session value is2fa, and that is what the test asserts.fn_device_trust; the code keeps readingconfig('two_factor.cookie_name').Testing
tests/TrustedDevicesApiTest.php(new, 15 tests):is_currentandlast_seen_atuntouched;404and unknown id404;302on all three routes.tests/DeviceTrustServiceTest.php: updated for the newremoveTrustedDevices()behaviour; new unit tests forrevokeTrustedDevice()and for surviving an audit failure.tests/TwoFactorProfileRoutesCsrfTest.php: both DELETE routes added.tests/js/components/trusted_devices_section.test.js(render, current-device label, empty state, revoke, failed revoke, revoke-all confirm/cancel, no submit buttons) andtests/js/profile/actions.test.js(the three helpers against the unmocked request layer).Results:
DeviceTrustServiceTest,TrustedDevicesApiTest,TwoFactorProfileRoutesCsrfTest,TwoFactorRepositoriesTest,TwoFactorLoginFlowTest,RecoveryCodeRegenerationTest,OAuth2NativeMFALoginFlowTest,VerifyOTPChallengeTest,AuthServiceLoginUserTestand thetests/unitMFA / 2FA tests. The full PHPUnit suite was not run.yarn test:unitgreen, 13 suites / 76 tests.Out of scope
2FA disable toggle and method selection, phone verification, admin-side device management, and friendly user-agent parsing for
device_name.Summary by CodeRabbit