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
 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
bd66c1f to
3df2d8e
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the shared two-factor disabling service.
No open issues 🎉
What changed in this PR
Adds a shared disableTwoFactor service method for the planned MFA reset command and profile/admin controls, building on the existing MFA infrastructure.
Changes:
- Defines the actor-based password requirement and caller-owned
settings_changedaudit contract. - Disables 2FA, deletes recovery codes, and revokes trusted devices in one transaction, followed by best-effort audit logging.
- Adds unit and database integration tests covering actor rules, state cleanup, and audit failure handling.
| File | Description |
|---|---|
tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php |
Tests password rules, disable operations, and audit failure handling. |
tests/DisableTwoFactorIntegrationTest.php |
Verifies database cleanup, audit events, and unchanged state after an incorrect password. |
app/Services/Auth/RecoveryCodeService.php |
Implements transactional disabling and trusted-device revocation. |
app/Services/Auth/IRecoveryCodeService.php |
Declares the method and documents password, audit, and group-enforcement behavior. |
Review effort: Balanced
| // a single transaction() call: nesting another one (e.g. through the audit | ||
| // service) inside it would let an inner failure close the entity manager | ||
| // under this still-running outer transaction | ||
| $this->tx_service->transaction(function () use ($user) { |
There was a problem hiding this comment.
@romanetar Once the users-row lock from SDS rev 2.6 lands (https://github.com/fntechgit/ftn-docsnsklz/pull/179, §3.5; ticket https://app.clickup.com/t/86bcy7rv7, task 3), this transaction should take the same PESSIMISTIC_WRITE lock on the user's row first. It deletes recovery codes too, and without the lock an admin reset racing a user regenerate (READ_COMMITTED, bulk DELETE with no lock) can let the regenerate insert after the delete, leaving a "reset" user with a fresh batch.
No change needed in this PR: the lock does not exist yet. It is tracked in task 9 of the recovery-code enrollment ticket, next to resetting recovery_codes_acknowledged_at here.
There was a problem hiding this comment.
Agreed, no change here. Noted that when the users-row lock lands, generateRecoveryCodes and enableTwoFactorAndGenerateCodes need it too, not just disableTwoFactor; otherwise the race remains. Tracked in task 9.
3df2d8e to
fd29e5c
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
Shared service method that disables 2FA for a user: clears two_factor_enabled and two_factor_enforced_at, deletes every recovery code and revokes every trusted device in a single transaction, then records a device_revoked audit event best-effort. The actor decides the password rule: null (console) and another user (admin) need no password, the user acting on their own account must provide the current one. The settings_changed audit event is left to the caller, which knows the context (reason, actor, operator). Group-enforced users are still challenged at login afterwards, as enforcement is derived from group membership.
fd29e5c to
c3f0def
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
A social-only user cannot pass the self-service password check, as User::checkPassword() returns false for an empty password. Document that the caller must require another proof of identity (a recent 2FA challenge) and pin the behaviour with a unit test.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
…d per device Replaces the revokeAllForUser bulk UPDATE, which the trusted-devices branch removes. disableTwoFactor and removeTrustedDevices revoke inside their transaction and log one device_revoked (with device_id) per revoked device after commit, with the same semantics as the devices-management endpoints. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
…eTwoFactorIntegrationTest
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/ This page is automatically updated on each push to this PR. |
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void |
There was a problem hiding this comment.
@romanetar #186 adds a second RecoveryCodeService::disableTwoFactor() with the same signature, and git merges this file without a conflict: only IRecoveryCodeService.php and the two test files conflict. git merge-tree of this branch and feat/mfa-disable-two-factor-self-service leaves both methods in RecoveryCodeService.php (lines 238 and 300 of the merged file), and php -l fails with Cannot redeclare App\Services\Auth\RecoveryCodeService::disableTwoFactor(). The container resolves this class in the UserController and UserApiController constructors, so a tree merged that way breaks those routes, login included.
The two versions also differ in behaviour:
- feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 rejects non-console calls for group-enforced users; this one has no group guard.
- feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 takes
lockUserRow()and clearsrecovery_codes_acknowledged_at; this one does neither. - feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 still calls
revokeAllForUser(), which this PR and #160 remove. Merging feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 with feat(2fa): trusted devices management UI and revoke endpoints #160 leaves that call in place (RecoveryCodeService.php:275). - feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 writes an
enrollment_changedrow inside the service, while this contract leaves the event to the caller andidp:reset-2fa(#171) writessettings_changed. A reset would end up writing both.
Suggested fix: keep one method before either PR merges. This one is what #171 consumes and what SDS rev 2.6 describes (per-device device_revoked, fntechgit/ftn-docsnsklz#179). Port #186's group guard, row lock, acknowledgment reset and passwordless window into it instead of adding a second definition, and decide whether the enrollment event comes from the service or the caller. Then run php -l and phpunit on the combined tree.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please review
ref: https://app.clickup.com/t/86ba2zp7r
Stacked on #166 (base:
feat/mfa-admin-enforcement-and-reset-commands), which is stacked on #125. Merge bottom-up: #125, #166, then this one.Summary
Provisional implementation of the shared disable/reset service method that ClickUp ticket 12 (
idp:reset-2fa) needs and ticket 14 (profile toggle and admin CRUD) will reuse. The ticket allows implementing it in ticket 12 when ticket 14 has not landed, with this exact contract:IRecoveryCodeService::disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): voiddisable2FA()(clearstwo_factor_enabledandtwo_factor_enforced_at), deletes every recovery code, and revokes every trusted device (is_revoked = 1, rows are kept).device_revokedaudit event, best-effort: a failure there does not fail the operation.$actordecides the password rule:null(console, e.g.idp:reset-2fa): no password.$currentPasswordmust match, otherwiseValidationExceptionand nothing changes.settings_changed: the caller knows the context (reason, actor, operator) and emits it, so there is exactly one row.User::shouldRequire2FA()).Things to know
DeviceTrustService::removeTrustedDevicesas the ticket suggests: that method's audit call opens its own transaction, and nesting transactions can close the entity manager if the inner one fails (the existing code inRecoveryCodeServicewarns about this). It usesIUserTrustedDeviceRepository::revokeAllForUserinside the same transaction instead, which is whatremoveTrustedDevicesdoes for the DB part.RecoveryCodeServicegets a new constructor dependency (IUserTrustedDeviceRepository). Nothing instantiates it withnew; the container resolves it.disableTwoFactor, the two need to be reconciled; ticket 14 can adopt this one directly.Test plan
tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php: 6 tests (console, admin, self-service with correct / wrong / missing password, audit failure).tests/DisableTwoFactorIntegrationTest.php: 2 tests against the DB (everything cleared, 2 devices revoked, 1device_revokedrow and nosettings_changed; wrong password leaves state untouched).tests/RecoveryCodeRegenerationTest.phpalready fails on the base branch (Class "Strategies\MFA\MFAChallengeStrategyFactory" not found), unrelated to this change.