Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions app/Repositories/DoctrineUserTrustedDeviceRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,6 @@ public function getByUserAndDeviceIdentifier(User $user, string $deviceIdentifie
return $result instanceof UserTrustedDevice ? $result : null;
}

public function revokeAllForUser(User $user): void
{
$this->getEntityManager()
->createQueryBuilder()
->update($this->getBaseEntity(), 'd')
->set('d.is_revoked', ':revoked')
->where('d.user = :user')
->setParameter('revoked', true)
->setParameter('user', $user)
->getQuery()
->execute();
}

public function getActiveByUserAndIdentifier(User $user, string $deviceIdentifier): ?UserTrustedDevice
{
$criteria = Criteria::create()
Expand All @@ -77,4 +64,14 @@ public function getActiveByUser(User $user): array

return $this->matching($criteria)->toArray();
}

public function revokeAllActiveForUser(User $user): array
{
$devices = $this->getActiveByUser($user);
foreach ($devices as $device) {
$device->setIsRevoked(true);
$this->add($device, false);
}
return $devices;
}
}
34 changes: 27 additions & 7 deletions app/Services/Auth/DeviceTrustService.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use DateTime;
use DateInterval;
use DateTimeZone;
use Illuminate\Support\Facades\Log;
use Utils\IPHelper;
use Utils\Db\ITransactionService;

Expand Down Expand Up @@ -98,13 +99,32 @@ public function isDeviceTrusted(User $user, ?string $cookieToken): bool

public function removeTrustedDevices(User $user): void
{
$this->repository->revokeAllForUser($user);
$devices = $this->tx_service->transaction(function () use ($user) {
return $this->repository->revokeAllActiveForUser($user);
});

$this->audit_service->log(
$user,
TwoFactorAuditLog::EventDeviceRevoked,
$user->getTwoFactorMethod(),
IPHelper::getUserIp()
);
foreach ($devices as $device) {
$this->logDeviceRevoked($user, $device);
}
}

/**
* Best-effort, after the revocation is committed: an audit failure must not
* fail an operation whose devices are already revoked, and audit_service->log()
* opens its own transaction, which cannot be nested inside ours.
*/
private function logDeviceRevoked(User $user, UserTrustedDevice $device): void
{
try {
$this->audit_service->log(
$user,
TwoFactorAuditLog::EventDeviceRevoked,
$user->getTwoFactorMethod(),
IPHelper::getUserIp(),
['device_id' => $device->getId()]
);
} catch (\Throwable $ex) {
Log::warning($ex);
}
}
}
32 changes: 32 additions & 0 deletions app/Services/Auth/IRecoveryCodeService.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,36 @@ public function countUnusedRecoveryCodes(User $user): int;
* @return RecoveryCodesStatus remaining/total/low-threshold standing for the user
*/
public function getStatus(User $user): RecoveryCodesStatus;

/**
* Disables 2FA for the user: clears two_factor_enabled and two_factor_enforced_at,
* deletes every recovery code and revokes every trusted device, all in a single
* transaction. A device_revoked audit event is recorded best-effort afterwards.
*
* $actor identifies who performs the operation:
* - null: a system/console operation (e.g. idp:reset-2fa), no password is required;
* - the same user: self-service, $currentPassword must match;
* - another user: an administrator acting on the account, no password is required.
* Authorizing the administrator is the caller's responsibility.
*
* Passwordless users (social-only accounts, !$user->hasPasswordSet()) can never
* pass the self-service password check, as User::checkPassword() returns false
* for an empty password, so self-service always throws for them. Such a caller
* must first require another proof of identity (under SDS rev 2.5, a recent 2FA
* challenge: MFAConstants::SESSION_KEY_COMPLETED_AT) and only then invoke this
* method on the user's behalf with $actor = null.
*
* The caller is responsible for emitting the settings_changed audit event, as only
* it knows the context (reason, actor, operator).
*
* Group-enforced users are still required to complete 2FA at login afterwards,
* as enforcement is derived from group membership (User::shouldRequire2FA()).
*
* @param User $user
* @param string|null $currentPassword
* @param User|null $actor
* @return void
* @throws ValidationException if the actor is the user and $currentPassword does not match
*/
public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void;
}
43 changes: 43 additions & 0 deletions app/Services/Auth/RecoveryCodeService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
) {
Expand Down Expand Up @@ -121,6 +123,47 @@ public function enableTwoFactorAndGenerateCodes(User $user, string $method): arr
return $codes;
}

/**
* @inheritDoc
*/
public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

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.

{
// self-service requires the current password; console and admin
// operations do not (see IRecoveryCodeService::disableTwoFactor)
if (!is_null($actor) && $actor->getId() === $user->getId()
Comment thread
smarcet marked this conversation as resolved.
Resolved
&& !$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
$revoked_devices = $this->tx_service->transaction(function () use ($user) {
$user->disable2FA();
$this->user_repository->add($user, false);
$this->repository->deleteAllForUser($user);
return $this->trusted_device_repository->revokeAllActiveForUser($user);
});

// 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.
// One device_revoked event per device actually revoked, with its device_id
// (same semantics as DeviceTrustService::removeTrustedDevices).
foreach ($revoked_devices as $device) {
try {
$this->audit_service->log(
$user,
TwoFactorAuditLog::EventDeviceRevoked,
$user->getTwoFactorMethod(),
IPHelper::getUserIp(),
['device_id' => $device->getId()]
);
} 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.
Expand Down
9 changes: 7 additions & 2 deletions app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,14 @@ interface IUserTrustedDeviceRepository extends IBaseRepository
public function getByUserAndDeviceIdentifier(User $user, string $deviceIdentifier): ?UserTrustedDevice;

/**
* Revoke all trusted devices for the given user (sets is_revoked = true).
* Marks every active (non-revoked, non-expired) trusted device of the user as revoked
* and returns them. Revokes entity by entity and opens no transaction of its own:
* the caller owns the transaction and logs one audit event per returned device
* after it commits.
*
* @return UserTrustedDevice[]
*/
public function revokeAllForUser(User $user): void;
public function revokeAllActiveForUser(User $user): array;

/**
* Look up an active (non-revoked, non-expired) trusted device for a user by its hashed identifier.
Expand Down
23 changes: 18 additions & 5 deletions tests/DeviceTrustServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -275,20 +275,33 @@ public function testTrustDeviceSetsExpiresAtFromConfig(): void
// removeTrustedDevices
// -------------------------------------------------------------------------

public function testRemoveTrustedDevicesRevokesAll(): void
public function testRemoveTrustedDevicesRevokesAllAndLogsOneEventPerDevice(): void
{
$user = Mockery::mock(User::class);
$user->shouldReceive('getTwoFactorMethod')->andReturn(User::MFAMethod_OTP);

$devices = [new UserTrustedDevice(), new UserTrustedDevice()];

$this->repo
->shouldReceive('revokeAllForUser')
->shouldReceive('revokeAllActiveForUser')
->once()
->with($user);
->with($user)
->andReturn($devices);

$this->audit_service
->shouldReceive('log')
->once()
->with($user, \App\libs\Auth\Models\TwoFactorAuditLog::EventDeviceRevoked, User::MFAMethod_OTP, Mockery::type('string'));
->twice()
->with($user, \App\libs\Auth\Models\TwoFactorAuditLog::EventDeviceRevoked, User::MFAMethod_OTP, Mockery::type('string'), Mockery::type('array'));

$this->service->removeTrustedDevices($user);
}

public function testRemoveTrustedDevicesWithoutActiveDevicesLogsNothing(): void
{
$user = Mockery::mock(User::class);

$this->repo->shouldReceive('revokeAllActiveForUser')->once()->with($user)->andReturn([]);
$this->audit_service->shouldNotReceive('log');

$this->service->removeTrustedDevices($user);
}
Expand Down
103 changes: 103 additions & 0 deletions tests/DisableTwoFactorIntegrationTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<?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);
}

// one device_revoked event per revoked device
$this->assertSame(2, 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());
}
}
Loading
Loading