Repository navigation
feat(mfa): add IRecoveryCodeService::disableTwoFactor #169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| use App\libs\Auth\Models\TwoFactorAuditLog; | ||
| use App\libs\Auth\Models\UserRecoveryCode; | ||
| use Auth\Repositories\IUserRecoveryCodeRepository; | ||
| use Auth\Repositories\IUserTrustedDeviceRepository; | ||
| use Auth\Repositories\IUserRepository; | ||
| use Auth\User; | ||
| use Illuminate\Support\Facades\Hash; | ||
|
|
@@ -36,6 +37,7 @@ final class RecoveryCodeService implements IRecoveryCodeService | |
| public function __construct( | ||
| private readonly IUserRecoveryCodeRepository $repository, | ||
| private readonly IUserRepository $user_repository, | ||
| private readonly IUserTrustedDeviceRepository $trusted_device_repository, | ||
| private readonly ITransactionService $tx_service, | ||
| private readonly ITwoFactorAuditService $audit_service, | ||
| ) { | ||
|
|
@@ -121,6 +123,42 @@ public function enableTwoFactorAndGenerateCodes(User $user, string $method): arr | |
| return $codes; | ||
| } | ||
|
|
||
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void | ||
| { | ||
| // self-service requires the current password; console and admin | ||
| // operations do not (see IRecoveryCodeService::disableTwoFactor) | ||
| if (!is_null($actor) && $actor->getId() === $user->getId() | ||
| && !$user->checkPassword(trim((string)$currentPassword))) { | ||
| throw new ValidationException('current_password is not correct.'); | ||
| } | ||
|
|
||
| // 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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 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 |
||
| $user->disable2FA(); | ||
| $this->user_repository->add($user, false); | ||
| $this->repository->deleteAllForUser($user); | ||
| $this->trusted_device_repository->revokeAllForUser($user); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @romanetar Heads-up from the #160 review: #160 removes |
||
| }); | ||
|
|
||
| // Best-effort: 2FA is already disabled and the codes and devices are gone, | ||
| // an audit-logging failure must not make the caller believe it did not happen. | ||
| try { | ||
| $this->audit_service->log( | ||
| $user, | ||
| TwoFactorAuditLog::EventDeviceRevoked, | ||
| $user->getTwoFactorMethod(), | ||
| IPHelper::getUserIp() | ||
| ); | ||
| } catch (\Throwable $ex) { | ||
| Log::warning($ex); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Invalidates every existing recovery code for the user and generates a | ||
| * fresh batch, within the caller's already-open transaction. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| <?php namespace Tests; | ||
| /** | ||
| * Copyright 2026 OpenStack Foundation | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| **/ | ||
|
|
||
| use App\libs\Auth\Models\TwoFactorAuditLog; | ||
| use App\Services\Auth\IDeviceTrustService; | ||
| use App\Services\Auth\IRecoveryCodeService; | ||
| use Auth\User; | ||
| use Illuminate\Support\Facades\DB; | ||
| use LaravelDoctrine\ORM\Facades\EntityManager; | ||
|
|
||
| /** | ||
| * Class DisableTwoFactorIntegrationTest | ||
| * IRecoveryCodeService::disableTwoFactor against the real database. | ||
| * @package Tests | ||
| */ | ||
| final class DisableTwoFactorIntegrationTest extends BrowserKitTestCase | ||
| { | ||
| private IRecoveryCodeService $service; | ||
| private User $user; | ||
|
|
||
| protected function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
| $this->service = $this->app->make(IRecoveryCodeService::class); | ||
|
|
||
| $user = new User(); | ||
| $user->setEmail('disable-2fa@nomail.com'); | ||
| $user->setFirstName('Disable'); | ||
| $user->setLastName('TwoFactor'); | ||
| $user->setIdentifier('disable-2fa@nomail.com'); | ||
| $user->setPassword('P@sswordS3cret'); | ||
| $user->verifyEmail(false); | ||
| EntityManager::persist($user); | ||
| EntityManager::flush(); | ||
| $this->user = $user; | ||
|
|
||
| // enrolled, with a code batch and two trusted devices | ||
| $this->service->enableTwoFactorAndGenerateCodes($user, User::MFAMethod_OTP); | ||
| $devices = $this->app->make(IDeviceTrustService::class); | ||
| $devices->trustDevice($user, 'agent-a', '127.0.0.1'); | ||
| $devices->trustDevice($user, 'agent-b', '127.0.0.1'); | ||
| } | ||
|
|
||
| private function userRow(): object | ||
| { | ||
| return DB::table('users')->where('id', $this->user->getId())->first(); | ||
| } | ||
|
|
||
| public function testDisableClearsEverything(): void | ||
| { | ||
| $this->assertSame(1, (int)$this->userRow()->two_factor_enabled); | ||
| $this->assertSame(10, DB::table('user_recovery_codes')->where('user_id', $this->user->getId())->count()); | ||
|
|
||
| $this->service->disableTwoFactor($this->user, null, null); | ||
|
|
||
| $row = $this->userRow(); | ||
| $this->assertSame(0, (int)$row->two_factor_enabled); | ||
| $this->assertNull($row->two_factor_enforced_at); | ||
| $this->assertSame(0, DB::table('user_recovery_codes')->where('user_id', $this->user->getId())->count()); | ||
|
|
||
| $devices = DB::table('user_trusted_devices')->where('user_id', $this->user->getId())->get(); | ||
| $this->assertCount(2, $devices); | ||
| foreach ($devices as $device) { | ||
| $this->assertSame(1, (int)$device->is_revoked); | ||
| } | ||
|
|
||
| $this->assertSame(1, DB::table('two_factor_audit_log') | ||
| ->where('user_id', $this->user->getId()) | ||
| ->where('event_type', TwoFactorAuditLog::EventDeviceRevoked) | ||
| ->count()); | ||
| // settings_changed is the caller's responsibility | ||
| $this->assertSame(0, DB::table('two_factor_audit_log') | ||
| ->where('user_id', $this->user->getId()) | ||
| ->where('event_type', TwoFactorAuditLog::EventSettingsChanged) | ||
| ->count()); | ||
| } | ||
|
|
||
| public function testSelfServiceWithWrongPasswordLeavesStateUntouched(): void | ||
| { | ||
| try { | ||
| $this->service->disableTwoFactor($this->user, 'wrong', $this->user); | ||
| $this->fail('ValidationException expected'); | ||
| } catch (\models\exceptions\ValidationException $ex) { | ||
| } | ||
|
|
||
| $this->assertSame(1, (int)$this->userRow()->two_factor_enabled); | ||
| $this->assertSame(10, DB::table('user_recovery_codes')->where('user_id', $this->user->getId())->count()); | ||
| $this->assertSame(0, DB::table('user_trusted_devices') | ||
| ->where('user_id', $this->user->getId())->where('is_revoked', 1)->count()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| <?php namespace Tests\unit; | ||
| /** | ||
| * Copyright 2026 OpenStack Foundation | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| **/ | ||
|
|
||
| use App\libs\Auth\Models\TwoFactorAuditLog; | ||
| use App\Services\Auth\ITwoFactorAuditService; | ||
| use App\Services\Auth\RecoveryCodeService; | ||
| use Auth\Repositories\IUserRecoveryCodeRepository; | ||
| use Auth\Repositories\IUserRepository; | ||
| use Auth\Repositories\IUserTrustedDeviceRepository; | ||
| use Auth\User; | ||
| use Mockery; | ||
| use models\exceptions\ValidationException; | ||
| use Tests\BrowserKitTestCase; | ||
| use Utils\Db\ITransactionService; | ||
|
|
||
| /** | ||
| * Class RecoveryCodeServiceDisableTwoFactorTest | ||
| * @package Tests\unit | ||
| */ | ||
| final class RecoveryCodeServiceDisableTwoFactorTest extends BrowserKitTestCase | ||
| { | ||
| private RecoveryCodeService $service; | ||
|
|
||
| /** @var \Mockery\MockInterface&IUserRecoveryCodeRepository */ | ||
| private $code_repo; | ||
|
|
||
| /** @var \Mockery\MockInterface&IUserRepository */ | ||
| private $user_repo; | ||
|
|
||
| /** @var \Mockery\MockInterface&IUserTrustedDeviceRepository */ | ||
| private $device_repo; | ||
|
|
||
| /** @var \Mockery\MockInterface&ITwoFactorAuditService */ | ||
| private $audit_service; | ||
|
|
||
| /** @var \Mockery\MockInterface&ITransactionService */ | ||
| private $tx_service; | ||
|
|
||
| protected function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
| $this->code_repo = Mockery::mock(IUserRecoveryCodeRepository::class); | ||
| $this->user_repo = Mockery::mock(IUserRepository::class); | ||
| $this->device_repo = Mockery::mock(IUserTrustedDeviceRepository::class); | ||
| $this->audit_service = Mockery::mock(ITwoFactorAuditService::class); | ||
| $this->tx_service = Mockery::mock(ITransactionService::class); | ||
| $this->tx_service->shouldReceive('transaction')->andReturnUsing(fn($cb) => $cb())->byDefault(); | ||
|
|
||
| $this->service = new RecoveryCodeService( | ||
| $this->code_repo, | ||
| $this->user_repo, | ||
| $this->device_repo, | ||
| $this->tx_service, | ||
| $this->audit_service | ||
| ); | ||
| } | ||
|
|
||
| protected function tearDown(): void | ||
| { | ||
| // Mockery expectations are verified on close(): count them as assertions | ||
| $this->addToAssertionCount(Mockery::getContainer()->mockery_getExpectationCount()); | ||
| Mockery::close(); | ||
| parent::tearDown(); | ||
| } | ||
|
|
||
| private function buildUser(int $id): \Mockery\MockInterface | ||
| { | ||
| $user = Mockery::mock(User::class); | ||
| $user->shouldReceive('getId')->andReturn($id); | ||
| $user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP); | ||
| return $user; | ||
| } | ||
|
|
||
| private function expectFullDisable(\Mockery\MockInterface $user): void | ||
| { | ||
| $user->shouldReceive('disable2FA')->once(); | ||
| $this->user_repo->shouldReceive('add')->once()->with($user, false); | ||
| $this->code_repo->shouldReceive('deleteAllForUser')->once()->with($user)->andReturn(10); | ||
| $this->device_repo->shouldReceive('revokeAllForUser')->once()->with($user); | ||
| $this->audit_service->shouldReceive('log') | ||
| ->once() | ||
| ->with($user, TwoFactorAuditLog::EventDeviceRevoked, User::MFAMethod_OTP, Mockery::type('string')); | ||
| } | ||
|
|
||
| public function testConsoleActorNeedsNoPassword(): void | ||
| { | ||
| $user = $this->buildUser(1); | ||
| $user->shouldNotReceive('checkPassword'); | ||
| $this->expectFullDisable($user); | ||
|
|
||
| $this->service->disableTwoFactor($user, null, null); | ||
| } | ||
|
|
||
| public function testAdminActorNeedsNoPassword(): void | ||
| { | ||
| $user = $this->buildUser(1); | ||
| $user->shouldNotReceive('checkPassword'); | ||
| $admin = $this->buildUser(2); | ||
| $this->expectFullDisable($user); | ||
|
|
||
| $this->service->disableTwoFactor($user, null, $admin); | ||
| } | ||
|
|
||
| public function testSelfServiceWithCorrectPassword(): void | ||
| { | ||
| $user = $this->buildUser(1); | ||
| $user->shouldReceive('checkPassword')->once()->with('secret')->andReturn(true); | ||
| $this->expectFullDisable($user); | ||
|
|
||
| $this->service->disableTwoFactor($user, ' secret ', $user); | ||
| } | ||
|
|
||
| public function testSelfServiceWithWrongPasswordChangesNothing(): void | ||
| { | ||
| $user = $this->buildUser(1); | ||
| $user->shouldReceive('checkPassword')->once()->andReturn(false); | ||
| $user->shouldNotReceive('disable2FA'); | ||
| $this->tx_service->shouldNotReceive('transaction'); | ||
| $this->code_repo->shouldNotReceive('deleteAllForUser'); | ||
| $this->device_repo->shouldNotReceive('revokeAllForUser'); | ||
| $this->audit_service->shouldNotReceive('log'); | ||
|
|
||
| $this->expectException(ValidationException::class); | ||
| $this->service->disableTwoFactor($user, 'wrong', $user); | ||
| } | ||
|
|
||
| public function testSelfServiceWithoutPasswordIsRejected(): void | ||
| { | ||
| $user = $this->buildUser(1); | ||
| $user->shouldReceive('checkPassword')->once()->with('')->andReturn(false); | ||
| $user->shouldNotReceive('disable2FA'); | ||
|
|
||
| $this->expectException(ValidationException::class); | ||
| $this->service->disableTwoFactor($user, null, $user); | ||
| } | ||
|
|
||
| public function testAuditFailureDoesNotFailTheOperation(): void | ||
| { | ||
| $user = $this->buildUser(1); | ||
| $user->shouldReceive('disable2FA')->once(); | ||
| $this->user_repo->shouldReceive('add')->once(); | ||
| $this->code_repo->shouldReceive('deleteAllForUser')->once()->andReturn(0); | ||
| $this->device_repo->shouldReceive('revokeAllForUser')->once(); | ||
| $this->audit_service->shouldReceive('log')->once()->andThrow(new \RuntimeException('audit down')); | ||
|
|
||
| $this->service->disableTwoFactor($user, null, null); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romanetar A self-enrolled user without a password (social-only account) can never disable their own 2FA through this path:
enableTwoFactordoes not ask for a password, but the self-service branch here requirescheckPassword(), andUser::checkPassword()returnsfalsewhen the password is empty.No caller uses the self-service branch yet, so nothing breaks today, but the profile toggle of ticket 14 (https://app.clickup.com/t/86bc64n02) will be that caller.
Suggested fix: document in the
IRecoveryCodeService::disableTwoFactorPHPDoc that, for self-service with!$user->hasPasswordSet(), the caller must require another proof of identity. Under SDS rev 2.5 that is a recent 2FA challenge (MFAConstants::SESSION_KEY_COMPLETED_AT). Add a unit test for the passwordless self-service case so the behaviour is pinned.