From be0ec70a25bc3ae45dd7333dee5fc19400514470 Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Mon, 24 Aug 2026 18:39:29 +0200 Subject: [PATCH 1/2] feat(settings): add a button to revoke all other sessions Signed-off-by: Peter Ringelmann --- apps/settings/appinfo/routes.php | 1 + apps/settings/lib/Activity/Provider.php | 9 + .../lib/Controller/AuthSettingsController.php | 52 ++++- .../src/components/AuthTokenRevokeAll.spec.ts | 211 ++++++++++++++++++ .../components/AuthTokenRevokeAllDialog.vue | 74 ++++++ .../src/components/AuthTokenSection.vue | 40 ++++ apps/settings/src/store/authtoken.ts | 49 +++- apps/settings/tests/Activity/ProviderTest.php | 111 +++++++++ .../Controller/AuthSettingsControllerTest.php | 142 ++++++++++++ .../e2e/settings/devices-sessions.spec.ts | 70 ++++++ .../fixtures/personal-settings-page.ts | 6 + .../sections/DevicesSessionsSettingsPage.ts | 73 ++++++ 12 files changed, 833 insertions(+), 5 deletions(-) create mode 100644 apps/settings/src/components/AuthTokenRevokeAll.spec.ts create mode 100644 apps/settings/src/components/AuthTokenRevokeAllDialog.vue create mode 100644 apps/settings/tests/Activity/ProviderTest.php create mode 100644 tests/playwright/e2e/settings/devices-sessions.spec.ts create mode 100644 tests/playwright/support/sections/DevicesSessionsSettingsPage.ts diff --git a/apps/settings/appinfo/routes.php b/apps/settings/appinfo/routes.php index 606e501503881..33a4169ceac16 100644 --- a/apps/settings/appinfo/routes.php +++ b/apps/settings/appinfo/routes.php @@ -13,6 +13,7 @@ ['name' => 'AuthSettings#create', 'url' => '/settings/personal/authtokens', 'verb' => 'POST' , 'root' => ''], ['name' => 'AuthSettings#update', 'url' => '/settings/personal/authtokens/{id}', 'verb' => 'PUT' , 'root' => ''], + ['name' => 'AuthSettings#destroyAll', 'url' => '/settings/personal/authtokens', 'verb' => 'DELETE' , 'root' => ''], ['name' => 'AuthSettings#destroy', 'url' => '/settings/personal/authtokens/{id}', 'verb' => 'DELETE' , 'root' => ''], ['name' => 'AuthSettings#wipe', 'url' => '/settings/personal/authtokens/wipe/{id}', 'verb' => 'POST' , 'root' => ''], diff --git a/apps/settings/lib/Activity/Provider.php b/apps/settings/lib/Activity/Provider.php index 4b19366163a54..3142c52a448fc 100644 --- a/apps/settings/lib/Activity/Provider.php +++ b/apps/settings/lib/Activity/Provider.php @@ -29,6 +29,7 @@ class Provider implements IProvider { 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_DELETED_ALL = 'app_token_deleted_all'; 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'; @@ -90,6 +91,13 @@ public function parse($language, IEvent $event, ?IEvent $previousEvent = null): $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_DELETED_ALL) { + $count = (int)($event->getSubjectParameters()['count'] ?? 0); + $subject = $this->l->n( + 'You revoked %n other session', + 'You revoked %n other sessions', + $count, + ); } 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) { @@ -121,6 +129,7 @@ protected function getParameters(IEvent $event): array { case self::PASSWORD_RESET_SELF: case self::EMAIL_CHANGED_SELF: case self::EMAIL_CHANGED: + case self::APP_TOKEN_DELETED_ALL: return []; case self::PASSWORD_CHANGED_BY: case self::EMAIL_CHANGED_BY: diff --git a/apps/settings/lib/Controller/AuthSettingsController.php b/apps/settings/lib/Controller/AuthSettingsController.php index a0e82ed98d1e8..60604daa8b453 100644 --- a/apps/settings/lib/Controller/AuthSettingsController.php +++ b/apps/settings/lib/Controller/AuthSettingsController.php @@ -188,6 +188,46 @@ public function destroy(int $id): JSONResponse { return new JSONResponse([]); } + /** + * Wipe-pending tokens are kept: revoking one cancels its pending wipe, so that + * stays a per-token decision. + */ + #[NoSubAdminRequired] + #[NoAdminRequired] + #[PasswordConfirmationRequired(strict: true)] + public function destroyAll(): JSONResponse { + if ($this->checkAppToken()) { + return new JSONResponse([], Http::STATUS_BAD_REQUEST); + } + + if ($this->userSession->getImpersonatingUserID() !== null) { + return $this->getServiceNotAvailableResponse(); + } + + try { + $currentTokenId = $this->tokenProvider->getToken($this->session->getId())->getId(); + } catch (SessionNotAvailableException|InvalidTokenException) { + return $this->getServiceNotAvailableResponse(); + } + + $revoked = []; + foreach ($this->tokenProvider->getTokenByUser($this->userId) as $token) { + if ($token->getId() === $currentTokenId || $token->getType() === IToken::WIPE_TOKEN) { + continue; + } + + $this->tokenProvider->invalidateTokenById($this->userId, $token->getId()); + $revoked[] = $token->getId(); + } + + if ($revoked !== []) { + // One aggregate entry rather than one per token, so a bulk revoke does not bury the feed. + $this->publishActivity(Provider::APP_TOKEN_DELETED_ALL, null, ['count' => count($revoked)]); + } + + return new JSONResponse(['revoked' => $revoked]); + } + #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired(strict: true)] @@ -222,14 +262,20 @@ public function update(int $id, array $scope, string $name): JSONResponse { return new JSONResponse([]); } - private function publishActivity(string $subject, int $id, array $parameters = []): void { + /** + * @param int|null $id Token the event is about, or null for events that span several tokens + */ + private function publishActivity(string $subject, ?int $id, array $parameters = []): void { $event = $this->activityManager->generateEvent(); $event->setApp('settings') ->setType('security') ->setAffectedUser($this->userId) ->setAuthor($this->userId) - ->setSubject($subject, $parameters) - ->setObject('app_token', $id, 'App Password'); + ->setSubject($subject, $parameters); + + if ($id !== null) { + $event->setObject('app_token', $id, 'App Password'); + } try { $this->activityManager->publish($event); diff --git a/apps/settings/src/components/AuthTokenRevokeAll.spec.ts b/apps/settings/src/components/AuthTokenRevokeAll.spec.ts new file mode 100644 index 0000000000000..29ca848275757 --- /dev/null +++ b/apps/settings/src/components/AuthTokenRevokeAll.spec.ts @@ -0,0 +1,211 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { IToken } from '../store/authtoken.ts' + +import { createTestingPinia } from '@pinia/testing' +import { mount } from '@vue/test-utils' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +// AuthToken.vue, pulled in transitively, reads window.OC.theme.productName at module +// evaluation time. vi.hoisted runs before imports, so it is set before the SFC is parsed. +vi.hoisted(() => { + (window as unknown as { OC: { theme: { productName: string } } }).OC.theme = { productName: 'Nextcloud' } +}) + +vi.mock('@nextcloud/initial-state', () => ({ + loadState: vi.fn((_app: string, key: string) => (key === 'app_tokens' ? [] : true)), +})) + +import NcNoteCard from '@nextcloud/vue/components/NcNoteCard' +import AuthTokenRevokeAllDialog from './AuthTokenRevokeAllDialog.vue' +import AuthTokenSection from './AuthTokenSection.vue' +import { TokenType, useAuthTokenStore } from '../store/authtoken.ts' + +function makeToken(overrides: Partial = {}): IToken { + return { + id: 1, + name: 'Test device', + type: TokenType.PERMANENT_TOKEN, + lastActivity: 1700000000, + canDelete: true, + canRename: true, + scope: { filesystem: true }, + ...overrides, + } +} + +// Renders the `buttons` prop as real buttons, so the callbacks are exercised by clicking +// rather than by reaching into the component instance. +const NcDialogStub = { + props: ['buttons'], + template: '
', +} + +const NcButtonStub = { + template: '', +} + +function mountSection(tokens: IToken[]) { + return mount(AuthTokenSection, { + mocks: { + t: (_: string, text: string) => text, + }, + stubs: { + AuthTokenList: true, + AuthTokenSetup: true, + NcSettingsSection: { template: '
' }, + NcButton: NcButtonStub, + NcDialog: NcDialogStub, + }, + pinia: createTestingPinia({ + createSpy: vi.fn, + initialState: { 'auth-token': { tokens } }, + }), + }) +} + +function mountDialog(props: { count: number, wipePendingCount: number, open?: boolean }) { + return mount(AuthTokenRevokeAllDialog, { + propsData: { open: true, ...props }, + mocks: { + t: (_: string, text: string) => text, + }, + stubs: { + NcDialog: NcDialogStub, + }, + }) +} + +describe('AuthTokenSection revoke-all button', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('hides the button when only the current session exists', () => { + const wrapper = mountSection([makeToken({ id: 1, current: true })]) + + expect(wrapper.find('button').exists()).toBe(false) + expect(wrapper.findComponent(AuthTokenRevokeAllDialog).exists()).toBe(false) + }) + + it('hides the button when the only other token is wipe-pending', () => { + const wrapper = mountSection([ + makeToken({ id: 1, current: true }), + makeToken({ id: 2, type: TokenType.WIPING_TOKEN }), + ]) + + expect(wrapper.find('button').exists()).toBe(false) + }) + + it('shows the button and opens the dialog without revoking anything yet', async () => { + const wrapper = mountSection([ + makeToken({ id: 1, current: true }), + makeToken({ id: 2 }), + ]) + const store = useAuthTokenStore() + + const button = wrapper.find('button') + expect(button.exists()).toBe(true) + + await button.trigger('click') + + const dialog = wrapper.findComponent(AuthTokenRevokeAllDialog) + expect(dialog.exists()).toBe(true) + expect(dialog.props('open')).toBe(true) + expect(store.deleteAllOtherTokens).not.toHaveBeenCalled() + }) + + // One-time tokens are hidden from the table but still revoked, so they count. + it('counts every other token except the wipe-pending ones', async () => { + const wrapper = mountSection([ + makeToken({ id: 1, current: true }), + makeToken({ id: 2 }), + makeToken({ id: 3 }), + makeToken({ id: 4, type: TokenType.WIPING_TOKEN }), + makeToken({ id: 5, type: TokenType.ONETIME_TOKEN }), + ]) + + await wrapper.find('button').trigger('click') + + const dialog = wrapper.findComponent(AuthTokenRevokeAllDialog) + expect(dialog.props('count')).toBe(3) + expect(dialog.props('wipePendingCount')).toBe(1) + }) + + it('revokes only after the dialog emits confirm', async () => { + const wrapper = mountSection([ + makeToken({ id: 1, current: true }), + makeToken({ id: 2 }), + ]) + const store = useAuthTokenStore() + + await wrapper.find('button').trigger('click') + + const dialog = wrapper.findComponent(AuthTokenRevokeAllDialog) + dialog.vm.$emit('confirm') + dialog.vm.$emit('update:open', false) + await wrapper.vm.$nextTick() + + expect(store.deleteAllOtherTokens).toHaveBeenCalledTimes(1) + }) + + it('does not revoke when the dialog is dismissed', async () => { + const wrapper = mountSection([ + makeToken({ id: 1, current: true }), + makeToken({ id: 2 }), + ]) + const store = useAuthTokenStore() + + await wrapper.find('button').trigger('click') + + const dialog = wrapper.findComponent(AuthTokenRevokeAllDialog) + dialog.vm.$emit('update:open', false) + await wrapper.vm.$nextTick() + + expect(wrapper.findComponent(AuthTokenRevokeAllDialog).exists()).toBe(false) + expect(store.deleteAllOtherTokens).not.toHaveBeenCalled() + }) +}) + +describe('AuthTokenRevokeAllDialog', () => { + it('omits the wipe note when nothing is pending a wipe', () => { + const wrapper = mountDialog({ count: 3, wipePendingCount: 0 }) + expect(wrapper.findComponent(NcNoteCard).exists()).toBe(false) + }) + + it('warns that wipe-pending devices keep access', () => { + const wrapper = mountDialog({ count: 3, wipePendingCount: 2 }) + + const noteCard = wrapper.findComponent(NcNoteCard) + expect(noteCard.exists()).toBe(true) + expect(noteCard.props('type')).toBe('warning') + expect(noteCard.text()).toMatch(/wipe/i) + }) + + it('tells the user the current session is kept', () => { + const wrapper = mountDialog({ count: 3, wipePendingCount: 0 }) + expect(wrapper.text()).toMatch(/stay signed in here/i) + }) + + it('emits confirm and closes when the destructive button is used', async () => { + const wrapper = mountDialog({ count: 3, wipePendingCount: 0 }) + + const buttons = wrapper.findAll('button') + await buttons.at(buttons.length - 1).trigger('click') + + expect(wrapper.emitted('confirm')).toHaveLength(1) + expect(wrapper.emitted('update:open')).toEqual([[false]]) + }) + + it('closes without confirming when cancelled', async () => { + const wrapper = mountDialog({ count: 3, wipePendingCount: 0 }) + + await wrapper.findAll('button').at(0).trigger('click') + + expect(wrapper.emitted('confirm')).toBeFalsy() + expect(wrapper.emitted('update:open')).toEqual([[false]]) + }) +}) diff --git a/apps/settings/src/components/AuthTokenRevokeAllDialog.vue b/apps/settings/src/components/AuthTokenRevokeAllDialog.vue new file mode 100644 index 0000000000000..1783facf3b363 --- /dev/null +++ b/apps/settings/src/components/AuthTokenRevokeAllDialog.vue @@ -0,0 +1,74 @@ + + + + + + + diff --git a/apps/settings/src/components/AuthTokenSection.vue b/apps/settings/src/components/AuthTokenSection.vue index 87bd66fb3ddd3..5731d1cc66f8b 100644 --- a/apps/settings/src/components/AuthTokenSection.vue +++ b/apps/settings/src/components/AuthTokenSection.vue @@ -9,6 +9,20 @@ :description="t('settings', 'Web, desktop and mobile clients currently logged in to your account.')"> +
+ + {{ t('settings', 'Revoke all other sessions') }} + +

+ {{ t('settings', 'Signs out every device and app except this one.') }} +

+
+ @@ -16,26 +30,52 @@ import { loadState } from '@nextcloud/initial-state' import { translate as t } from '@nextcloud/l10n' import { defineComponent } from 'vue' +import NcButton from '@nextcloud/vue/components/NcButton' import NcSettingsSection from '@nextcloud/vue/components/NcSettingsSection' import AuthTokenList from './AuthTokenList.vue' +import AuthTokenRevokeAllDialog from './AuthTokenRevokeAllDialog.vue' import AuthTokenSetup from './AuthTokenSetup.vue' +import { useAuthTokenStore } from '../store/authtoken.ts' export default defineComponent({ name: 'AuthTokenSection', components: { AuthTokenList, + AuthTokenRevokeAllDialog, AuthTokenSetup, + NcButton, NcSettingsSection, }, + setup() { + const authTokenStore = useAuthTokenStore() + return { authTokenStore } + }, + data() { return { canCreateToken: loadState('settings', 'can_create_app_token'), + revokeAllDialogOpen: false, } }, methods: { t, + + revokeAllOthers() { + this.authTokenStore.deleteAllOtherTokens() + }, }, }) + + diff --git a/apps/settings/src/store/authtoken.ts b/apps/settings/src/store/authtoken.ts index bd300d999ddde..7c0053b8566f1 100644 --- a/apps/settings/src/store/authtoken.ts +++ b/apps/settings/src/store/authtoken.ts @@ -3,9 +3,9 @@ import axios from '@nextcloud/axios' * SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors * SPDX-License-Identifier: AGPL-3.0-or-later */ -import { showError } from '@nextcloud/dialogs' +import { showError, showSuccess } from '@nextcloud/dialogs' import { loadState } from '@nextcloud/initial-state' -import { translate as t } from '@nextcloud/l10n' +import { translatePlural as n, translate as t } from '@nextcloud/l10n' import { addPasswordConfirmationInterceptors, confirmPassword, PwdConfirmationMode } from '@nextcloud/password-confirmation' import { generateUrl } from '@nextcloud/router' import { defineStore } from 'pinia' @@ -35,6 +35,10 @@ export interface IToken { scope: Record } +export interface IRevokeAllResponse { + revoked: number[] +} + export interface ITokenResponse { /** * The device token created @@ -56,6 +60,26 @@ export const useAuthTokenStore = defineStore('auth-token', { tokens: loadState('settings', 'app_tokens', []), } }, + getters: { + /** + * Must stay in step with `destroyAll()` server side, or the confirmation + * count disagrees with what actually gets revoked. + * + * @param state Current store state + */ + revocableCount(state): number { + return state.tokens.filter((token) => !token.current && token.type !== TokenType.WIPING_TOKEN).length + }, + + /** + * Left alone by a bulk revoke, because cancelling a pending wipe must stay deliberate. + * + * @param state Current store state + */ + wipePendingCount(state): number { + return state.tokens.filter((token) => !token.current && token.type === TokenType.WIPING_TOKEN).length + }, + }, actions: { /** * Update a token on server @@ -109,6 +133,27 @@ export const useAuthTokenStore = defineStore('auth-token', { return false }, + /** + * Reconciles from the returned ids rather than clearing optimistically: the + * server keeps wipe-pending tokens, so it revokes fewer than we asked. + */ + async deleteAllOtherTokens() { + logger.debug('Revoking all other app tokens') + + try { + const { data } = await axios.delete(BASE_URL, { confirmPassword: PwdConfirmationMode.Strict }) + const revoked = new Set(data.revoked) + this.tokens = this.tokens.filter(({ id }) => !revoked.has(id)) + logger.debug('Other app tokens revoked', { count: data.revoked.length }) + showSuccess(n('settings', 'Revoked %n other session', 'Revoked %n other sessions', data.revoked.length)) + return data + } catch (error) { + logger.error('Could not revoke the other app tokens', { error }) + showError(t('settings', 'Could not revoke the other sessions')) + } + return null + }, + /** * Wipe a token and the connected device * diff --git a/apps/settings/tests/Activity/ProviderTest.php b/apps/settings/tests/Activity/ProviderTest.php new file mode 100644 index 0000000000000..bf13ec5f97572 --- /dev/null +++ b/apps/settings/tests/Activity/ProviderTest.php @@ -0,0 +1,111 @@ +l10nFactory = $this->createMock(IFactory::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->activityManager = $this->createMock(IManager::class); + $this->l = $this->createMock(IL10N::class); + + $this->l10nFactory->method('get') + ->with('settings', 'en') + ->willReturn($this->l); + + $this->provider = new Provider( + $this->l10nFactory, + $this->urlGenerator, + $this->userManager, + $this->activityManager, + ); + } + + public function testParseUnrelatedApp(): void { + $event = $this->createMock(IEvent::class); + $event->method('getApp')->willReturn('comments'); + + $this->expectException(UnknownActivityException::class); + $this->provider->parse('en', $event); + } + + public function testParseUnknownSubject(): void { + $event = $this->createMock(IEvent::class); + $event->method('getApp')->willReturn('settings'); + $event->method('getSubject')->willReturn('something_else'); + + $this->expectException(UnknownActivityException::class); + $this->provider->parse('en', $event); + } + + public static function dataRevokedAllCount(): array { + return [ + 'single token' => [1], + 'several tokens' => [7], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataRevokedAllCount')] + public function testParseRevokedAllUsesPluralForm(int $count): void { + $event = $this->createMock(IEvent::class); + $event->method('getApp')->willReturn('settings'); + $event->method('getSubject')->willReturn(Provider::APP_TOKEN_DELETED_ALL); + $event->method('getSubjectParameters')->willReturn(['count' => $count]); + + $this->l->expects($this->once()) + ->method('n') + ->with( + 'You revoked %n other session', + 'You revoked %n other sessions', + $count, + ) + ->willReturn('parsed subject'); + + // Aggregate event, so there are no rich parameters to substitute. + $event->expects($this->once()) + ->method('setRichSubject') + ->with('parsed subject', []); + + $this->provider->parse('en', $event); + } + + public function testParseRevokedAllWithoutCountParameter(): void { + $event = $this->createMock(IEvent::class); + $event->method('getApp')->willReturn('settings'); + $event->method('getSubject')->willReturn(Provider::APP_TOKEN_DELETED_ALL); + $event->method('getSubjectParameters')->willReturn([]); + + $this->l->expects($this->once()) + ->method('n') + ->with($this->anything(), $this->anything(), 0) + ->willReturn('parsed subject'); + + $this->provider->parse('en', $event); + } +} diff --git a/apps/settings/tests/Controller/AuthSettingsControllerTest.php b/apps/settings/tests/Controller/AuthSettingsControllerTest.php index c8886f1c35f8f..de9c933cfb3dc 100644 --- a/apps/settings/tests/Controller/AuthSettingsControllerTest.php +++ b/apps/settings/tests/Controller/AuthSettingsControllerTest.php @@ -16,6 +16,7 @@ use OC\Authentication\Token\IWipeableToken; use OC\Authentication\Token\PublicKeyToken; use OC\Authentication\Token\RemoteWipe; +use OCA\Settings\Activity\Provider; use OCA\Settings\Controller\AuthSettingsController; use OCP\Activity\IEvent; use OCP\Activity\IManager; @@ -271,6 +272,140 @@ public function testDestroyWipePendingEmitsCancelledSubject(): void { $this->assertEquals([], $this->controller->destroy($tokenId)->getData()); } + public function testDestroyAllRevokesEveryTokenButTheCurrent(): void { + $currentToken = $this->mockAuthToken(10); + $otherToken = $this->mockAuthToken(11); + $appPassword = $this->mockAuthToken(12); + + $this->session->method('getId')->willReturn('sessionid'); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with('sessionid') + ->willReturn($currentToken); + $this->tokenProvider->expects($this->once()) + ->method('getTokenByUser') + ->with($this->uid) + ->willReturn([$currentToken, $otherToken, $appPassword]); + + $revokedIds = []; + $this->tokenProvider->expects($this->exactly(2)) + ->method('invalidateTokenById') + ->willReturnCallback(function (string $uid, int $id) use (&$revokedIds): void { + $this->assertSame($this->uid, $uid); + $revokedIds[] = $id; + }); + + $this->mockActivityManager(); + + $response = $this->controller->destroyAll(); + + $this->assertSame([11, 12], $revokedIds, 'the current session token must not be revoked'); + $this->assertSame(['revoked' => [11, 12]], $response->getData()); + } + + public function testDestroyAllKeepsWipePendingTokens(): void { + $currentToken = $this->mockAuthToken(10); + $otherToken = $this->mockAuthToken(11); + $wipingToken = $this->mockAuthToken(12, IToken::WIPE_TOKEN); + + $this->session->method('getId')->willReturn('sessionid'); + $this->tokenProvider->method('getToken')->willReturn($currentToken); + $this->tokenProvider->method('getTokenByUser')->willReturn([$currentToken, $otherToken, $wipingToken]); + + $this->tokenProvider->expects($this->once()) + ->method('invalidateTokenById') + ->with($this->uid, 11); + + $this->mockActivityManager(); + + $this->assertSame(['revoked' => [11]], $this->controller->destroyAll()->getData()); + } + + public function testDestroyAllPublishesOneAggregateActivity(): void { + $currentToken = $this->mockAuthToken(10); + + $this->session->method('getId')->willReturn('sessionid'); + $this->tokenProvider->method('getToken')->willReturn($currentToken); + $this->tokenProvider->method('getTokenByUser') + ->willReturn([$currentToken, $this->mockAuthToken(11), $this->mockAuthToken(12)]); + + $event = $this->createMock(IEvent::class); + $event->method('setApp')->willReturnSelf(); + $event->method('setType')->willReturnSelf(); + $event->method('setAffectedUser')->willReturnSelf(); + $event->method('setAuthor')->willReturnSelf(); + $event->expects($this->once()) + ->method('setSubject') + ->with(Provider::APP_TOKEN_DELETED_ALL, ['count' => 2]) + ->willReturnSelf(); + // Aggregate event, so there is no single app_token to attach. + $event->expects($this->never()) + ->method('setObject'); + + $this->activityManager->expects($this->once()) + ->method('generateEvent') + ->willReturn($event); + $this->activityManager->expects($this->once()) + ->method('publish'); + + $this->controller->destroyAll(); + } + + public function testDestroyAllWithNothingToRevokePublishesNoActivity(): void { + $currentToken = $this->mockAuthToken(10); + + $this->session->method('getId')->willReturn('sessionid'); + $this->tokenProvider->method('getToken')->willReturn($currentToken); + $this->tokenProvider->method('getTokenByUser')->willReturn([$currentToken]); + + $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); + $this->activityManager->expects($this->never())->method('publish'); + + $this->assertSame(['revoked' => []], $this->controller->destroyAll()->getData()); + } + + public function testDestroyAllWithAppPassword(): void { + $this->session->expects($this->once()) + ->method('exists') + ->with('app_password') + ->willReturn(true); + + $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); + + $response = $this->controller->destroyAll(); + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + } + + public function testDestroyAllWhileImpersonating(): void { + $this->userSession->method('getImpersonatingUserID')->willReturn('admin'); + + $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); + + $response = $this->controller->destroyAll(); + $this->assertSame(Http::STATUS_SERVICE_UNAVAILABLE, $response->getStatus()); + } + + public function testDestroyAllSessionNotAvailable(): void { + $this->session->method('getId') + ->willThrowException(new SessionNotAvailableException()); + + $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); + + $response = $this->controller->destroyAll(); + $this->assertSame(Http::STATUS_SERVICE_UNAVAILABLE, $response->getStatus()); + } + + public function testDestroyAllInvalidSessionToken(): void { + $this->session->method('getId')->willReturn('sessionid'); + $this->tokenProvider->method('getToken') + ->willThrowException(new InvalidTokenException('Token does not exist')); + + $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); + + $response = $this->controller->destroyAll(); + $this->assertSame(Http::STATUS_SERVICE_UNAVAILABLE, $response->getStatus()); + } + public function testDestroyWrongUser(): void { $tokenId = 124; $token = $this->createMock(PublicKeyToken::class); @@ -447,6 +582,13 @@ public function testUpdateTokenNonExisting(): void { $this->assertSame(\OCP\AppFramework\Http::STATUS_NOT_FOUND, $response->getStatus()); } + private function mockAuthToken(int $id, int $type = IToken::PERMANENT_TOKEN): PublicKeyToken&MockObject { + $token = $this->createMock(PublicKeyToken::class); + $token->method('getId')->willReturn($id); + $token->method('getType')->willReturn($type); + return $token; + } + private function mockActivityManager(): void { $this->activityManager->expects($this->once()) ->method('generateEvent') diff --git a/tests/playwright/e2e/settings/devices-sessions.spec.ts b/tests/playwright/e2e/settings/devices-sessions.spec.ts new file mode 100644 index 0000000000000..4f3ec2806a91e --- /dev/null +++ b/tests/playwright/e2e/settings/devices-sessions.spec.ts @@ -0,0 +1,70 @@ +/* + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { runOcc } from '@nextcloud/e2e-test-server/docker' +import { expect } from '@playwright/test' +import { test } from '../../support/fixtures/personal-settings-page.ts' + +// Without --password-from-env the token carries no login password. Fine here: the +// test only revokes it, it never authenticates with it. +async function addAppPassword(userId: string, name: string): Promise { + await runOcc(['user:auth-tokens:add', userId, '--name', name]) +} + +test.describe('Settings: Devices & sessions', () => { + test('revokes every other session but keeps the current one', async ({ page, devicesSessionsPage, user }) => { + await addAppPassword(user.userId, 'Playwright device') + + await devicesSessionsPage.open() + await expect(devicesSessionsPage.tokenRows()).toHaveCount(2) + await expect(devicesSessionsPage.tokenRow('Playwright device')).toBeVisible() + + await devicesSessionsPage.revokeAllOtherSessions() + + await expect(devicesSessionsPage.tokenRow('Playwright device')).toHaveCount(0) + await expect(devicesSessionsPage.tokenRow('This session')).toBeVisible() + await expect(devicesSessionsPage.tokenRows()).toHaveCount(1) + + await expect(devicesSessionsPage.revokeAllButton()).toHaveCount(0) + + // A revoked session token would redirect this reload to the login page. + await page.reload() + await expect(devicesSessionsPage.heading()).toBeVisible() + await expect(devicesSessionsPage.tokenRow('This session')).toBeVisible() + }) + + test('revokes several devices at once', async ({ devicesSessionsPage, user }) => { + await addAppPassword(user.userId, 'First device') + await addAppPassword(user.userId, 'Second device') + + await devicesSessionsPage.open() + await expect(devicesSessionsPage.tokenRows()).toHaveCount(3) + + await devicesSessionsPage.revokeAllOtherSessions() + + await expect(devicesSessionsPage.tokenRows()).toHaveCount(1) + await expect(devicesSessionsPage.tokenRow('This session')).toBeVisible() + }) + + test('offers nothing to revoke when only the current session exists', async ({ devicesSessionsPage }) => { + await devicesSessionsPage.open() + + await expect(devicesSessionsPage.tokenRows()).toHaveCount(1) + await expect(devicesSessionsPage.tokenRow('This session')).toBeVisible() + await expect(devicesSessionsPage.revokeAllButton()).toHaveCount(0) + }) + + test('keeps every session when the confirmation is dismissed', async ({ devicesSessionsPage, user }) => { + await addAppPassword(user.userId, 'Playwright device') + + await devicesSessionsPage.open() + const dialog = await devicesSessionsPage.openRevokeAllDialog() + await dialog.getByRole('button', { name: 'Cancel' }).click() + + await expect(dialog).toHaveCount(0) + await expect(devicesSessionsPage.tokenRows()).toHaveCount(2) + await expect(devicesSessionsPage.tokenRow('Playwright device')).toBeVisible() + }) +}) diff --git a/tests/playwright/support/fixtures/personal-settings-page.ts b/tests/playwright/support/fixtures/personal-settings-page.ts index 999049ca7aa86..3f4a7a33cbe65 100644 --- a/tests/playwright/support/fixtures/personal-settings-page.ts +++ b/tests/playwright/support/fixtures/personal-settings-page.ts @@ -4,6 +4,7 @@ */ import { runOcc } from '@nextcloud/e2e-test-server/docker' +import { DevicesSessionsSettingsPage } from '../sections/DevicesSessionsSettingsPage.ts' import { LanguageLocaleSettingsPage } from '../sections/LanguageLocaleSettingsPage.ts' import { ProfileContactSettingsPage } from '../sections/ProfileContactSettingsPage.ts' import { test as userSessionTest } from './random-user-session.ts' @@ -16,6 +17,7 @@ import { test as userSessionTest } from './random-user-session.ts' export const test = userSessionTest.extend<{ profileContactPage: ProfileContactSettingsPage languageLocalePage: LanguageLocaleSettingsPage + devicesSessionsPage: DevicesSessionsSettingsPage }>({ user: async ({ user: baseUser }, use) => { await runOcc(['user:setting', baseUser.userId, 'core', 'lang', 'en']) @@ -30,4 +32,8 @@ export const test = userSessionTest.extend<{ languageLocalePage: async ({ page, user }, use) => { await use(new LanguageLocaleSettingsPage(page, user)) }, + + devicesSessionsPage: async ({ page, user }, use) => { + await use(new DevicesSessionsSettingsPage(page, user)) + }, }) diff --git a/tests/playwright/support/sections/DevicesSessionsSettingsPage.ts b/tests/playwright/support/sections/DevicesSessionsSettingsPage.ts new file mode 100644 index 0000000000000..6f5c1384ca955 --- /dev/null +++ b/tests/playwright/support/sections/DevicesSessionsSettingsPage.ts @@ -0,0 +1,73 @@ +/* + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { User } from '@nextcloud/e2e-test-server' +import type { Locator, Page } from '@playwright/test' + +import { expect } from '@playwright/test' +import { handlePasswordConfirmation } from '../utils/password-confirmation.ts' + +export class DevicesSessionsSettingsPage { + constructor( + private readonly page: Page, + private readonly user: User, + ) {} + + heading(): Locator { + return this.page.getByRole('heading', { name: 'Devices & sessions', level: 2 }) + } + + async open(): Promise { + await this.page.goto('settings/user/security') + await expect(this.heading()).toBeVisible() + } + + /** Product-owned id: the security page renders several unrelated tables. */ + tokenList(): Locator { + return this.page.locator('#app-tokens-table') + } + + tokenRows(): Locator { + return this.tokenList().locator('tbody').getByRole('row') + } + + /** + * @param name - Visible device name, "This session" for the current one + */ + tokenRow(name: string): Locator { + return this.tokenRows().filter({ hasText: name }) + } + + revokeAllButton(): Locator { + return this.page.getByRole('button', { name: 'Revoke all other sessions' }) + } + + revokeAllDialog(): Locator { + return this.page.getByRole('dialog', { name: 'Revoke all other sessions?' }) + } + + async openRevokeAllDialog(): Promise { + await this.revokeAllButton().click() + const dialog = this.revokeAllDialog() + await expect(dialog).toBeVisible() + return dialog + } + + /** + * The DELETE only leaves the browser once the password confirmation is cleared, so + * the response listener has to be registered before the confirm button is used. + */ + async revokeAllOtherSessions(): Promise { + const dialog = await this.openRevokeAllDialog() + + const revoked = this.page.waitForResponse((r) => r.request().method() === 'DELETE' + && r.url().includes('/settings/personal/authtokens') + && r.ok()) + + await dialog.getByRole('button', { name: 'Revoke all others' }).click() + await handlePasswordConfirmation(this.page, this.user.password) + await revoked + } +} From 0088f7d8690d2867c231ee3e84d059270869ab37 Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Tue, 25 Aug 2026 09:25:32 +0200 Subject: [PATCH 2/2] fix: PR feedback Signed-off-by: Peter Ringelmann --- apps/settings/appinfo/routes.php | 2 +- .../lib/Controller/AuthSettingsController.php | 8 +++-- apps/settings/src/store/authtoken.ts | 2 +- .../Controller/AuthSettingsControllerTest.php | 32 +++++++++---------- 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/apps/settings/appinfo/routes.php b/apps/settings/appinfo/routes.php index 33a4169ceac16..bc67981be808b 100644 --- a/apps/settings/appinfo/routes.php +++ b/apps/settings/appinfo/routes.php @@ -13,7 +13,7 @@ ['name' => 'AuthSettings#create', 'url' => '/settings/personal/authtokens', 'verb' => 'POST' , 'root' => ''], ['name' => 'AuthSettings#update', 'url' => '/settings/personal/authtokens/{id}', 'verb' => 'PUT' , 'root' => ''], - ['name' => 'AuthSettings#destroyAll', 'url' => '/settings/personal/authtokens', 'verb' => 'DELETE' , 'root' => ''], + ['name' => 'AuthSettings#destroyOthers', 'url' => '/settings/personal/authtokens', 'verb' => 'DELETE' , 'root' => ''], ['name' => 'AuthSettings#destroy', 'url' => '/settings/personal/authtokens/{id}', 'verb' => 'DELETE' , 'root' => ''], ['name' => 'AuthSettings#wipe', 'url' => '/settings/personal/authtokens/wipe/{id}', 'verb' => 'POST' , 'root' => ''], diff --git a/apps/settings/lib/Controller/AuthSettingsController.php b/apps/settings/lib/Controller/AuthSettingsController.php index 60604daa8b453..508ef9b7f5e49 100644 --- a/apps/settings/lib/Controller/AuthSettingsController.php +++ b/apps/settings/lib/Controller/AuthSettingsController.php @@ -189,13 +189,15 @@ public function destroy(int $id): JSONResponse { } /** - * Wipe-pending tokens are kept: revoking one cancels its pending wipe, so that - * stays a per-token decision. + * Revoke the tokens of the current user other than the session's own. + * + * Wipe-pending tokens are kept too: revoking one cancels its pending wipe, so + * that stays a per-token decision. */ #[NoSubAdminRequired] #[NoAdminRequired] #[PasswordConfirmationRequired(strict: true)] - public function destroyAll(): JSONResponse { + public function destroyOthers(): JSONResponse { if ($this->checkAppToken()) { return new JSONResponse([], Http::STATUS_BAD_REQUEST); } diff --git a/apps/settings/src/store/authtoken.ts b/apps/settings/src/store/authtoken.ts index 7c0053b8566f1..9c3040cff81ba 100644 --- a/apps/settings/src/store/authtoken.ts +++ b/apps/settings/src/store/authtoken.ts @@ -62,7 +62,7 @@ export const useAuthTokenStore = defineStore('auth-token', { }, getters: { /** - * Must stay in step with `destroyAll()` server side, or the confirmation + * Must stay in step with `destroyOthers()` server side, or the confirmation * count disagrees with what actually gets revoked. * * @param state Current store state diff --git a/apps/settings/tests/Controller/AuthSettingsControllerTest.php b/apps/settings/tests/Controller/AuthSettingsControllerTest.php index de9c933cfb3dc..1a6a50a08cc4f 100644 --- a/apps/settings/tests/Controller/AuthSettingsControllerTest.php +++ b/apps/settings/tests/Controller/AuthSettingsControllerTest.php @@ -272,7 +272,7 @@ public function testDestroyWipePendingEmitsCancelledSubject(): void { $this->assertEquals([], $this->controller->destroy($tokenId)->getData()); } - public function testDestroyAllRevokesEveryTokenButTheCurrent(): void { + public function testDestroyOthersRevokesEveryTokenButTheCurrent(): void { $currentToken = $this->mockAuthToken(10); $otherToken = $this->mockAuthToken(11); $appPassword = $this->mockAuthToken(12); @@ -297,13 +297,13 @@ public function testDestroyAllRevokesEveryTokenButTheCurrent(): void { $this->mockActivityManager(); - $response = $this->controller->destroyAll(); + $response = $this->controller->destroyOthers(); $this->assertSame([11, 12], $revokedIds, 'the current session token must not be revoked'); $this->assertSame(['revoked' => [11, 12]], $response->getData()); } - public function testDestroyAllKeepsWipePendingTokens(): void { + public function testDestroyOthersKeepsWipePendingTokens(): void { $currentToken = $this->mockAuthToken(10); $otherToken = $this->mockAuthToken(11); $wipingToken = $this->mockAuthToken(12, IToken::WIPE_TOKEN); @@ -318,10 +318,10 @@ public function testDestroyAllKeepsWipePendingTokens(): void { $this->mockActivityManager(); - $this->assertSame(['revoked' => [11]], $this->controller->destroyAll()->getData()); + $this->assertSame(['revoked' => [11]], $this->controller->destroyOthers()->getData()); } - public function testDestroyAllPublishesOneAggregateActivity(): void { + public function testDestroyOthersPublishesOneAggregateActivity(): void { $currentToken = $this->mockAuthToken(10); $this->session->method('getId')->willReturn('sessionid'); @@ -348,10 +348,10 @@ public function testDestroyAllPublishesOneAggregateActivity(): void { $this->activityManager->expects($this->once()) ->method('publish'); - $this->controller->destroyAll(); + $this->controller->destroyOthers(); } - public function testDestroyAllWithNothingToRevokePublishesNoActivity(): void { + public function testDestroyOthersWithNothingToRevokePublishesNoActivity(): void { $currentToken = $this->mockAuthToken(10); $this->session->method('getId')->willReturn('sessionid'); @@ -361,10 +361,10 @@ public function testDestroyAllWithNothingToRevokePublishesNoActivity(): void { $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); $this->activityManager->expects($this->never())->method('publish'); - $this->assertSame(['revoked' => []], $this->controller->destroyAll()->getData()); + $this->assertSame(['revoked' => []], $this->controller->destroyOthers()->getData()); } - public function testDestroyAllWithAppPassword(): void { + public function testDestroyOthersWithAppPassword(): void { $this->session->expects($this->once()) ->method('exists') ->with('app_password') @@ -372,37 +372,37 @@ public function testDestroyAllWithAppPassword(): void { $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); - $response = $this->controller->destroyAll(); + $response = $this->controller->destroyOthers(); $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); } - public function testDestroyAllWhileImpersonating(): void { + public function testDestroyOthersWhileImpersonating(): void { $this->userSession->method('getImpersonatingUserID')->willReturn('admin'); $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); - $response = $this->controller->destroyAll(); + $response = $this->controller->destroyOthers(); $this->assertSame(Http::STATUS_SERVICE_UNAVAILABLE, $response->getStatus()); } - public function testDestroyAllSessionNotAvailable(): void { + public function testDestroyOthersSessionNotAvailable(): void { $this->session->method('getId') ->willThrowException(new SessionNotAvailableException()); $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); - $response = $this->controller->destroyAll(); + $response = $this->controller->destroyOthers(); $this->assertSame(Http::STATUS_SERVICE_UNAVAILABLE, $response->getStatus()); } - public function testDestroyAllInvalidSessionToken(): void { + public function testDestroyOthersInvalidSessionToken(): void { $this->session->method('getId')->willReturn('sessionid'); $this->tokenProvider->method('getToken') ->willThrowException(new InvalidTokenException('Token does not exist')); $this->tokenProvider->expects($this->never())->method('invalidateTokenById'); - $response = $this->controller->destroyAll(); + $response = $this->controller->destroyOthers(); $this->assertSame(Http::STATUS_SERVICE_UNAVAILABLE, $response->getStatus()); }