fix(settings,oauth2): preserve wipe state across admin deletion paths

Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>
This commit is contained in:
Peter Ringelmann 2026-05-21 07:35:57 +02:00 committed by Peter R.
parent 16ca990bdb
commit 4b1c3fbe3b
10 changed files with 292 additions and 54 deletions

View file

@ -8,6 +8,7 @@ declare(strict_types=1);
*/
namespace OCA\OAuth2\Controller;
use OC\Authentication\Token\IProvider as IAuthTokenProvider;
use OCA\OAuth2\Db\AccessTokenMapper;
use OCA\OAuth2\Db\Client;
use OCA\OAuth2\Db\ClientMapper;
@ -15,13 +16,15 @@ use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
use OCP\AppFramework\Http\JSONResponse;
use OCP\Authentication\Token\IProvider as IAuthTokenProvider;
use OCP\Authentication\Exceptions\InvalidTokenException;
use OCP\Authentication\Exceptions\WipeTokenException;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IUser;
use OCP\IUserManager;
use OCP\Security\ICrypto;
use OCP\Security\ISecureRandom;
use Psr\Log\LoggerInterface;
class SettingsController extends Controller {
@ -37,6 +40,7 @@ class SettingsController extends Controller {
private IAuthTokenProvider $tokenProvider,
private IUserManager $userManager,
private ICrypto $crypto,
private LoggerInterface $logger,
) {
parent::__construct($appName, $request);
}
@ -73,7 +77,26 @@ class SettingsController extends Controller {
$client = $this->clientMapper->getByUid($id);
$this->userManager->callForSeenUsers(function (IUser $user) use ($client): void {
$this->tokenProvider->invalidateTokensOfUser($user->getUID(), $client->getName());
// Skip tokens that are marked for remote wipe so revoking the
// OAuth2 client does not silently cancel a pending wipe.
$tokens = $this->tokenProvider->getTokenByUser($user->getUID());
foreach ($tokens as $token) {
if ($token->getName() !== $client->getName()) {
continue;
}
try {
$this->tokenProvider->getTokenById($token->getId());
} catch (WipeTokenException $e) {
$this->logger->info('Preserving token {tokenId} of user {uid}: marked for remote wipe, OAuth2 client revoke would cancel the wipe.', [
'tokenId' => $token->getId(),
'uid' => $user->getUID(),
]);
continue;
} catch (InvalidTokenException $e) {
// Token already invalid; let invalidateTokenById handle it.
}
$this->tokenProvider->invalidateTokenById($user->getUID(), $token->getId());
}
});
$this->accessTokenMapper->deleteByClientId($id);

View file

@ -6,13 +6,15 @@
*/
namespace OCA\OAuth2\Tests\Controller;
use OC\Authentication\Token\IProvider as IAuthTokenProvider;
use OCA\OAuth2\Controller\SettingsController;
use OCA\OAuth2\Db\AccessTokenMapper;
use OCA\OAuth2\Db\Client;
use OCA\OAuth2\Db\ClientMapper;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\JSONResponse;
use OCP\Authentication\Token\IProvider as IAuthTokenProvider;
use OCP\Authentication\Exceptions\WipeTokenException;
use OCP\Authentication\Token\IToken;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IUser;
@ -20,6 +22,8 @@ use OCP\IUserManager;
use OCP\Security\ICrypto;
use OCP\Security\ISecureRandom;
use OCP\Server;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;
use Test\TestCase;
#[\PHPUnit\Framework\Attributes\Group(name: 'DB')]
@ -42,6 +46,7 @@ class SettingsControllerTest extends TestCase {
private $l;
/** @var ICrypto|\PHPUnit\Framework\MockObject\MockObject */
private $crypto;
private LoggerInterface&MockObject $logger;
protected function setUp(): void {
parent::setUp();
@ -53,6 +58,7 @@ class SettingsControllerTest extends TestCase {
$this->authTokenProvider = $this->createMock(IAuthTokenProvider::class);
$this->userManager = $this->createMock(IUserManager::class);
$this->crypto = $this->createMock(ICrypto::class);
$this->logger = $this->createMock(LoggerInterface::class);
$this->l = $this->createMock(IL10N::class);
$this->l->method('t')
->willReturnArgument(0);
@ -65,7 +71,8 @@ class SettingsControllerTest extends TestCase {
$this->l,
$this->authTokenProvider,
$this->userManager,
$this->crypto
$this->crypto,
$this->logger,
);
}
@ -132,11 +139,15 @@ class SettingsControllerTest extends TestCase {
$user1->updateLastLoginTimestamp();
$tokenProviderMock = $this->getMockBuilder(IAuthTokenProvider::class)->getMock();
// expect one call per user and ensure the correct client name
// One getTokenByUser call per user; we return no matching tokens here
// so invalidateTokenById is never invoked.
$tokenProviderMock
->expects($this->exactly($count + 1))
->method('invalidateTokensOfUser')
->with($this->isType('string'), 'My Client Name');
->method('getTokenByUser')
->willReturn([]);
$tokenProviderMock
->expects($this->never())
->method('invalidateTokenById');
$client = new Client();
$client->setId(123);
@ -167,7 +178,8 @@ class SettingsControllerTest extends TestCase {
$this->l,
$tokenProviderMock,
$userManager,
$this->crypto
$this->crypto,
$this->logger,
);
$result = $settingsController->deleteClient(123);
@ -177,6 +189,96 @@ class SettingsControllerTest extends TestCase {
$user1->delete();
}
public function testDeleteClientPreservesWipePendingToken(): void {
$userManager = Server::get(IUserManager::class);
$user = $userManager->createUser('test_wipe_preserve', 'test_wipe_preserve');
$user->updateLastLoginTimestamp();
$client = new Client();
$client->setId(456);
$client->setName('My Client Name');
$client->setRedirectUri('https://example.com/');
$client->setSecret(bin2hex('MyHashedSecret'));
$client->setClientIdentifier('MyClientIdentifier');
// Token marked for wipe with a matching client name: must NOT be invalidated.
$wipeToken = $this->createMock(IToken::class);
$wipeToken->method('getId')->willReturn(11);
$wipeToken->method('getName')->willReturn('My Client Name');
// Regular token with matching name: must be invalidated.
$regularToken = $this->createMock(IToken::class);
$regularToken->method('getId')->willReturn(12);
$regularToken->method('getName')->willReturn('My Client Name');
// Non-matching name: must be left alone.
$otherToken = $this->createMock(IToken::class);
$otherToken->method('getId')->willReturn(13);
$otherToken->method('getName')->willReturn('Some Other Client');
$tokenProviderMock = $this->getMockBuilder(IAuthTokenProvider::class)->getMock();
$tokenProviderMock
->method('getTokenByUser')
->willReturnCallback(function (string $uid) use ($wipeToken, $regularToken, $otherToken) {
return $uid === 'test_wipe_preserve'
? [$wipeToken, $regularToken, $otherToken]
: [];
});
// Wipe state is signalled via WipeTokenException from getTokenById.
$tokenProviderMock
->method('getTokenById')
->willReturnCallback(function (int $id) use ($wipeToken, $regularToken) {
if ($id === 11) {
throw new WipeTokenException($wipeToken);
}
return $regularToken;
});
$tokenProviderMock
->expects($this->once())
->method('invalidateTokenById')
->with('test_wipe_preserve', 12);
$this->clientMapper
->method('getByUid')
->with(456)
->willReturn($client);
$this->accessTokenMapper
->expects($this->once())
->method('deleteByClientId')
->with(456);
$this->clientMapper
->expects($this->once())
->method('delete')
->with($client);
$logger = $this->createMock(LoggerInterface::class);
$logger->expects($this->atLeastOnce())
->method('info')
->with($this->stringContains('Preserving token'), $this->callback(function (array $context) {
return ($context['tokenId'] ?? null) === 11
&& ($context['uid'] ?? null) === 'test_wipe_preserve';
}));
$settingsController = new SettingsController(
'oauth2',
$this->request,
$this->clientMapper,
$this->secureRandom,
$this->accessTokenMapper,
$this->l,
$tokenProviderMock,
$userManager,
$this->crypto,
$logger,
);
$result = $settingsController->deleteClient(456);
$this->assertInstanceOf(JSONResponse::class, $result);
$this->assertEquals([], $result->getData());
$user->delete();
}
public function testInvalidRedirectUri(): void {
$result = $this->settingsController->addClient('test', 'invalidurl');

View file

@ -27,6 +27,7 @@ class Provider implements IProvider {
public const EMAIL_CHANGED = 'email_changed';
public const APP_TOKEN_CREATED = 'app_token_created';
public const APP_TOKEN_DELETED = 'app_token_deleted';
public const APP_TOKEN_DELETED_WIPE_CANCELLED = 'app_token_deleted_wipe_cancelled';
public const APP_TOKEN_RENAMED = 'app_token_renamed';
public const APP_TOKEN_FILESYSTEM_GRANTED = 'app_token_filesystem_granted';
public const APP_TOKEN_FILESYSTEM_REVOKED = 'app_token_filesystem_revoked';
@ -86,6 +87,8 @@ class Provider implements IProvider {
}
} elseif ($event->getSubject() === self::APP_TOKEN_DELETED) {
$subject = $this->l->t('You deleted app password "{token}"');
} elseif ($event->getSubject() === self::APP_TOKEN_DELETED_WIPE_CANCELLED) {
$subject = $this->l->t('You deleted app password "{token}" and cancelled its pending remote wipe');
} elseif ($event->getSubject() === self::APP_TOKEN_RENAMED) {
$subject = $this->l->t('You renamed app password "{token}" to "{newToken}"');
} elseif ($event->getSubject() === self::APP_TOKEN_FILESYSTEM_GRANTED) {
@ -125,6 +128,7 @@ class Provider implements IProvider {
];
case self::APP_TOKEN_CREATED:
case self::APP_TOKEN_DELETED:
case self::APP_TOKEN_DELETED_WIPE_CANCELLED:
case self::APP_TOKEN_FILESYSTEM_GRANTED:
case self::APP_TOKEN_FILESYSTEM_REVOKED:
return [

View file

@ -179,17 +179,21 @@ class AuthSettingsController extends Controller {
return new JSONResponse([], Http::STATUS_BAD_REQUEST);
}
$subject = Provider::APP_TOKEN_DELETED;
try {
$token = $this->findTokenByIdAndUser($id);
} catch (WipeTokenException $e) {
//continue as we can destroy tokens in wipe
// Deleting a wipe-pending token cancels the pending wipe; the device
// may already be uninstalled so we allow it, but record it under a
// distinct subject so the audit trail captures the consequence.
$token = $e->getToken();
$subject = Provider::APP_TOKEN_DELETED_WIPE_CANCELLED;
} catch (InvalidTokenException $e) {
return new JSONResponse([], Http::STATUS_NOT_FOUND);
}
$this->tokenProvider->invalidateTokenById($this->userId, $token->getId());
$this->publishActivity(Provider::APP_TOKEN_DELETED, $token->getId(), ['name' => $token->getName()]);
$this->publishActivity($subject, $token->getId(), ['name' => $token->getName()]);
return [];
}

View file

@ -119,7 +119,8 @@ describe('AuthToken revoke flow', () => {
dialog.vm.$emit('update:open', false)
await wrapper.vm.$nextTick()
expect(dialog.props('open')).toBe(false)
// Dialog is v-if'd off the tree once closed
expect(wrapper.findComponent(AuthTokenDeleteDialog).exists()).toBe(false)
expect(store.deleteToken).not.toHaveBeenCalled()
})

View file

@ -83,11 +83,13 @@
</NcActions>
</td>
<AuthTokenDeleteDialog
v-if="deleteDialogOpen"
:token="token"
:open="deleteDialogOpen"
@update:open="deleteDialogOpen = $event"
@confirm="confirmDelete" />
<AuthTokenWipeDialog
v-if="wipeDialogOpen"
:token="token"
:open="wipeDialogOpen"
@update:open="wipeDialogOpen = $event"

View file

@ -6,7 +6,7 @@
<template>
<NcDialog
:open="open"
:name="dialogTitle"
:name="copy.title"
:buttons="buttons"
size="normal"
@update:open="onUpdateOpen">
@ -19,7 +19,7 @@
</p>
</NcNoteCard>
<p class="auth-token-delete-dialog__body">
{{ bodyText }}
{{ copy.body }}
</p>
</NcDialog>
</template>
@ -65,22 +65,19 @@ export default defineComponent({
return this.token.type === TokenType.WIPING_TOKEN
},
dialogTitle(): string {
return this.wiping
? t('settings', 'Revoke and cancel pending wipe?')
: t('settings', 'Revoke app password?')
},
bodyText(): string {
return this.wiping
? t('settings', 'Only continue if you no longer need the device to be wiped.')
: t('settings', 'The app or device will lose access on its next sync. This cannot be undone.')
},
destructiveLabel(): string {
return this.wiping
? t('settings', 'Revoke and cancel wipe')
: t('settings', 'Revoke')
copy(): { title: string, body: string, action: string } {
if (this.wiping) {
return {
title: t('settings', 'Revoke and cancel pending wipe?'),
body: t('settings', 'Only continue if you no longer need the device to be wiped.'),
action: t('settings', 'Revoke and cancel wipe'),
}
}
return {
title: t('settings', 'Revoke app password?'),
body: t('settings', 'The app or device will lose access on its next sync. This cannot be undone.'),
action: t('settings', 'Revoke'),
}
},
buttons(): IDialogButton[] {
@ -93,7 +90,7 @@ export default defineComponent({
},
},
{
label: this.destructiveLabel,
label: this.copy.action,
variant: 'error',
callback: () => {
this.$emit('confirm')

View file

@ -233,6 +233,43 @@ class AuthSettingsControllerTest extends TestCase {
$this->assertSame([], $this->controller->destroy($tokenId));
}
public function testDestroyWipePendingEmitsCancelledSubject(): void {
$tokenId = 125;
$token = $this->createMock(PublicKeyToken::class);
$token->method('getId')->willReturn($tokenId);
$token->method('getName')->willReturn('My phone');
$this->tokenProvider->expects($this->once())
->method('getTokenById')
->with($tokenId)
->willThrowException(new \OCP\Authentication\Exceptions\WipeTokenException($token));
// The token is still invalidated (the user opted into cancelling the wipe).
$this->tokenProvider->expects($this->once())
->method('invalidateTokenById')
->with($this->uid, $tokenId);
// Activity event must use the distinguishing subject.
$event = $this->createMock(IEvent::class);
$event->method('setApp')->willReturnSelf();
$event->method('setType')->willReturnSelf();
$event->method('setAffectedUser')->willReturnSelf();
$event->method('setAuthor')->willReturnSelf();
$event->method('setObject')->willReturnSelf();
$event->expects($this->once())
->method('setSubject')
->with(\OCA\Settings\Activity\Provider::APP_TOKEN_DELETED_WIPE_CANCELLED, ['name' => 'My phone'])
->willReturnSelf();
$this->activityManager->expects($this->once())
->method('generateEvent')
->willReturn($event);
$this->activityManager->expects($this->once())
->method('publish');
$this->assertEquals([], $this->controller->destroy($tokenId));
}
public function testDestroyWrongUser(): void {
$tokenId = 124;
$token = $this->createMock(PublicKeyToken::class);

View file

@ -9,6 +9,8 @@ namespace OC\Core\Command\User\AuthTokens;
use DateTimeImmutable;
use OC\Authentication\Token\IProvider;
use OC\Core\Command\Base;
use OCP\Authentication\Exceptions\InvalidTokenException;
use OCP\Authentication\Exceptions\WipeTokenException;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Exception\RuntimeException;
use Symfony\Component\Console\Input\InputArgument;
@ -43,6 +45,12 @@ class Delete extends Base {
null,
InputOption::VALUE_REQUIRED,
'Delete tokens last used before a given date.'
)
->addOption(
'cancel-wipe',
null,
InputOption::VALUE_NONE,
'Allow deleting a token that is marked for remote wipe. The pending wipe will not run.'
);
}
@ -51,6 +59,7 @@ class Delete extends Base {
$uid = $input->getArgument('uid');
$id = (int)$input->getArgument('id');
$before = $input->getOption('last-used-before');
$cancelWipe = (bool)$input->getOption('cancel-wipe');
if ($before) {
if ($id) {
@ -63,6 +72,19 @@ class Delete extends Base {
if (!$id) {
throw new RuntimeException('Not enough arguments. Specify the token <id> or use the --last-used-before option.');
}
if (!$cancelWipe) {
try {
$this->tokenProvider->getTokenById($id);
} catch (WipeTokenException $e) {
$output->writeln('<error>Token ' . $id . ' is marked for remote wipe. Pass --cancel-wipe to delete it anyway; the pending wipe will not run.</error>');
return Command::FAILURE;
} catch (InvalidTokenException $e) {
// Token doesn't exist, has expired, or is otherwise unusable.
// Defer to invalidateTokenById, which handles the no-op case.
}
}
return $this->deleteById($uid, $id);
}

View file

@ -8,6 +8,8 @@ namespace Tests\Core\Command\User\AuthTokens;
use OC\Authentication\Token\IProvider;
use OC\Core\Command\User\AuthTokens\Delete;
use OCP\Authentication\Exceptions\WipeTokenException;
use OCP\Authentication\Token\IToken;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Exception\RuntimeException;
use Symfony\Component\Console\Input\InputInterface;
@ -39,6 +41,20 @@ class DeleteTest extends TestCase {
$this->command = new Delete($tokenProvider);
}
/**
* Default option mapping: --last-used-before unset, --cancel-wipe unset.
*
* @param string|null $lastUsedBefore
* @param bool $cancelWipe
*/
private function mockOptions(?string $lastUsedBefore = null, bool $cancelWipe = false): void {
$this->consoleInput->method('getOption')
->willReturnMap([
['last-used-before', $lastUsedBefore],
['cancel-wipe', $cancelWipe],
]);
}
public function testDeleteTokenById(): void {
$this->consoleInput->expects($this->exactly(2))
->method('getArgument')
@ -47,10 +63,7 @@ class DeleteTest extends TestCase {
['id', '42']
]);
$this->consoleInput->expects($this->once())
->method('getOption')
->with('last-used-before')
->willReturn(null);
$this->mockOptions();
$this->tokenProvider->expects($this->once())
->method('invalidateTokenById')
@ -68,10 +81,7 @@ class DeleteTest extends TestCase {
['id', null]
]);
$this->consoleInput->expects($this->once())
->method('getOption')
->with('last-used-before')
->willReturn(null);
$this->mockOptions();
$this->expectException(RuntimeException::class);
@ -89,10 +99,7 @@ class DeleteTest extends TestCase {
['id', null]
]);
$this->consoleInput->expects($this->once())
->method('getOption')
->with('last-used-before')
->willReturn('946684800');
$this->mockOptions('946684800');
$this->tokenProvider->expects($this->once())
->method('invalidateLastUsedBefore')
@ -110,10 +117,7 @@ class DeleteTest extends TestCase {
['id', null]
]);
$this->consoleInput->expects($this->once())
->method('getOption')
->with('last-used-before')
->willReturn('2000-01-01T00:00:00Z');
$this->mockOptions('2000-01-01T00:00:00Z');
$this->tokenProvider->expects($this->once())
->method('invalidateLastUsedBefore')
@ -131,10 +135,7 @@ class DeleteTest extends TestCase {
['id', null]
]);
$this->consoleInput->expects($this->once())
->method('getOption')
->with('last-used-before')
->willReturn('2000-01-01');
$this->mockOptions('2000-01-01');
$this->tokenProvider->expects($this->once())
->method('invalidateLastUsedBefore')
@ -152,10 +153,7 @@ class DeleteTest extends TestCase {
['id', '42']
]);
$this->consoleInput->expects($this->once())
->method('getOption')
->with('last-used-before')
->willReturn('946684800');
$this->mockOptions('946684800');
$this->expectException(RuntimeException::class);
@ -164,4 +162,52 @@ class DeleteTest extends TestCase {
$result = self::invokePrivate($this->command, 'execute', [$this->consoleInput, $this->consoleOutput]);
$this->assertSame(Command::SUCCESS, $result);
}
public function testDeleteByIdRefusesWipePendingWithoutFlag(): void {
$this->consoleInput->expects($this->exactly(2))
->method('getArgument')
->willReturnMap([
['uid', 'user'],
['id', '42']
]);
$this->mockOptions();
$wipeToken = $this->createMock(IToken::class);
$this->tokenProvider->expects($this->once())
->method('getTokenById')
->with(42)
->willThrowException(new WipeTokenException($wipeToken));
$this->tokenProvider->expects($this->never())->method('invalidateTokenById');
$this->consoleOutput->expects($this->once())
->method('writeln')
->with($this->stringContains('marked for remote wipe'));
$result = self::invokePrivate($this->command, 'execute', [$this->consoleInput, $this->consoleOutput]);
$this->assertSame(Command::FAILURE, $result);
}
public function testDeleteByIdAllowsWipePendingWithFlag(): void {
$this->consoleInput->expects($this->exactly(2))
->method('getArgument')
->willReturnMap([
['uid', 'user'],
['id', '42']
]);
$this->mockOptions(null, true);
// With --cancel-wipe, the wipe-state pre-check is skipped entirely
// (the operator has already opted in), so getTokenById should not run.
$this->tokenProvider->expects($this->never())->method('getTokenById');
$this->tokenProvider->expects($this->once())
->method('invalidateTokenById')
->with('user', 42);
$result = self::invokePrivate($this->command, 'execute', [$this->consoleInput, $this->consoleOutput]);
$this->assertSame(Command::SUCCESS, $result);
}
}