From ea52693b455e1a6fb4938d81924f61a68d59ca30 Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Thu, 22 Oct 2026 15:37:51 +0200 Subject: [PATCH 1/2] feat(auth): let app passwords be limited to whole apps Signed-off-by: Peter Ringelmann --- .../lib/Controller/AuthSettingsController.php | 6 ++- .../Controller/AuthSettingsControllerTest.php | 6 +++ lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../Security/TokenScopeMiddleware.php | 4 ++ .../Authentication/Token/GrantableApps.php | 40 +++++++++++++++++++ .../Authentication/Token/TokenScopes.php | 14 +++++-- .../Security/TokenScopeMiddlewareTest.php | 4 ++ .../Token/GrantableAppsTest.php | 31 ++++++++++++++ .../Authentication/Token/TokenScopesTest.php | 32 ++++++++++++++- 10 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 lib/private/Authentication/Token/GrantableApps.php create mode 100644 tests/lib/Authentication/Token/GrantableAppsTest.php diff --git a/apps/settings/lib/Controller/AuthSettingsController.php b/apps/settings/lib/Controller/AuthSettingsController.php index 6224fcccb5034..efde78fe0d212 100644 --- a/apps/settings/lib/Controller/AuthSettingsController.php +++ b/apps/settings/lib/Controller/AuthSettingsController.php @@ -11,6 +11,7 @@ use BadMethodCallException; use OC\Authentication\Exceptions\InvalidTokenException as OcInvalidTokenException; use OC\Authentication\Exceptions\PasswordlessTokenException; +use OC\Authentication\Token\GrantableApps; use OC\Authentication\Token\INamedToken; use OC\Authentication\Token\IProvider; use OC\Authentication\Token\RemoteWipe; @@ -54,6 +55,7 @@ public function __construct( private IConfig $serverConfig, private IL10N $l, private TokenScopes $tokenScopes, + private GrantableApps $grantableApps, ) { parent::__construct($appName, $request); } @@ -113,7 +115,7 @@ public function create(string $name = '', bool $qrcodeLogin = false, ?array $sco } else { $type = IToken::PERMANENT_TOKEN; try { - $scope = $scopes === null ? null : $this->tokenScopes->updateScope([], [TokenScopes::KEY => $scopes]); + $scope = $scopes === null ? null : $this->tokenScopes->updateScope([], [TokenScopes::KEY => $scopes], $this->grantableApps->get()); } catch (\InvalidArgumentException) { return new JSONResponse([], Http::STATUS_BAD_REQUEST); } @@ -255,7 +257,7 @@ public function update(int $id, array $scope, string $name): JSONResponse { $currentScope = $token->getScopeAsArray(); try { - $newScope = $this->tokenScopes->updateScope($currentScope, $scope); + $newScope = $this->tokenScopes->updateScope($currentScope, $scope, $this->grantableApps->get()); } catch (\InvalidArgumentException) { return new JSONResponse([], Http::STATUS_BAD_REQUEST); } diff --git a/apps/settings/tests/Controller/AuthSettingsControllerTest.php b/apps/settings/tests/Controller/AuthSettingsControllerTest.php index 279e35cae1159..c1dcaa51478f7 100644 --- a/apps/settings/tests/Controller/AuthSettingsControllerTest.php +++ b/apps/settings/tests/Controller/AuthSettingsControllerTest.php @@ -11,6 +11,7 @@ use OC\AppFramework\Http; use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Token\GrantableApps; use OC\Authentication\Token\IProvider; use OC\Authentication\Token\IToken; use OC\Authentication\Token\IWipeableToken; @@ -49,6 +50,7 @@ class AuthSettingsControllerTest extends TestCase { private RemoteWipe&MockObject $remoteWipe; private IConfig&MockObject $serverConfig; private IL10N&MockObject $l; + private GrantableApps&MockObject $grantableApps; private string $uid = 'jane'; private AuthSettingsController $controller; @@ -68,6 +70,8 @@ protected function setUp(): void { $logger = $this->createMock(LoggerInterface::class); $this->l = $this->createMock(IL10N::class); + $this->grantableApps = $this->createMock(GrantableApps::class); + $this->grantableApps->method('get')->willReturn(['deck']); $this->controller = new AuthSettingsController( 'core', $this->request, @@ -83,6 +87,7 @@ protected function setUp(): void { $this->serverConfig, $this->l, new TokenScopes(), + $this->grantableApps, ); } @@ -144,6 +149,7 @@ public static function dataCreateWithScopes(): array { return [ 'files' => [$files, [TokenScopes::KEY => $sorted, IToken::SCOPE_FILESYSTEM => true]], 'calendar' => [['dav:calendar:read'], [TokenScopes::KEY => ['dav:calendar:read'], IToken::SCOPE_FILESYSTEM => false]], + 'whole app' => [['deck'], [TokenScopes::KEY => ['deck'], IToken::SCOPE_FILESYSTEM => false]], ]; } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index b0cd6b6a66aa8..11752d1b177a4 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1349,6 +1349,7 @@ 'OC\\Authentication\\Login\\UserDisabledCheckCommand' => $baseDir . '/lib/private/Authentication/Login/UserDisabledCheckCommand.php', 'OC\\Authentication\\Login\\WebAuthnChain' => $baseDir . '/lib/private/Authentication/Login/WebAuthnChain.php', 'OC\\Authentication\\Notifications\\Notifier' => $baseDir . '/lib/private/Authentication/Notifications/Notifier.php', + 'OC\\Authentication\\Token\\GrantableApps' => $baseDir . '/lib/private/Authentication/Token/GrantableApps.php', 'OC\\Authentication\\Token\\INamedToken' => $baseDir . '/lib/private/Authentication/Token/INamedToken.php', 'OC\\Authentication\\Token\\IProvider' => $baseDir . '/lib/private/Authentication/Token/IProvider.php', 'OC\\Authentication\\Token\\IToken' => $baseDir . '/lib/private/Authentication/Token/IToken.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 25506f06bc315..2adc848e5c61e 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1390,6 +1390,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Authentication\\Login\\UserDisabledCheckCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/UserDisabledCheckCommand.php', 'OC\\Authentication\\Login\\WebAuthnChain' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/WebAuthnChain.php', 'OC\\Authentication\\Notifications\\Notifier' => __DIR__ . '/../../..' . '/lib/private/Authentication/Notifications/Notifier.php', + 'OC\\Authentication\\Token\\GrantableApps' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/GrantableApps.php', 'OC\\Authentication\\Token\\INamedToken' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/INamedToken.php', 'OC\\Authentication\\Token\\IProvider' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/IProvider.php', 'OC\\Authentication\\Token\\IToken' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/IToken.php', diff --git a/lib/private/AppFramework/Middleware/Security/TokenScopeMiddleware.php b/lib/private/AppFramework/Middleware/Security/TokenScopeMiddleware.php index d6a8dcbba01bd..071c8d1634cf9 100644 --- a/lib/private/AppFramework/Middleware/Security/TokenScopeMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/TokenScopeMiddleware.php @@ -46,6 +46,10 @@ public function beforeController(Controller $controller, string $methodName): vo throw new TokenScopeException(); } + if ($this->lockdownManager->hasScope($this->appName)) { + return; + } + $attribute = $this->tokenScope($controller); if ($attribute instanceof NoTokenScopeRequired && $this->isShipped()) { return; diff --git a/lib/private/Authentication/Token/GrantableApps.php b/lib/private/Authentication/Token/GrantableApps.php new file mode 100644 index 0000000000000..449af69b04ead --- /dev/null +++ b/lib/private/Authentication/Token/GrantableApps.php @@ -0,0 +1,40 @@ + + */ + public function get(): array { + $apps = array_map( + static fn (array $entry): string => $entry['app'] ?? $entry['id'], + $this->navigationManager->getAll(INavigationManager::TYPE_APPS), + ); + return array_values(array_unique(array_diff($apps, self::EXCLUDED))); + } +} diff --git a/lib/private/Authentication/Token/TokenScopes.php b/lib/private/Authentication/Token/TokenScopes.php index 460ece3f60f99..8c9f8b4079794 100644 --- a/lib/private/Authentication/Token/TokenScopes.php +++ b/lib/private/Authentication/Token/TokenScopes.php @@ -38,9 +38,10 @@ public function fromBlob(array $blob): ?array { * Only `filesystem` and `scopes` are taken from `$requested`. Omitted `scopes` stay as stored, * `null` means full access, and scoped tokens get `filesystem` derived from their scopes. * + * @param list $grantableApps * @throws \InvalidArgumentException */ - public function updateScope(array $stored, array $requested): array { + public function updateScope(array $stored, array $requested, array $grantableApps): array { $blob = $stored; if (array_key_exists(self::KEY, $requested)) { @@ -54,7 +55,9 @@ public function updateScope(array $stored, array $requested): array { if (!is_array($requestedScopes)) { throw new \InvalidArgumentException('scopes must be an array'); } - $blob[self::KEY] = $this->validate($requestedScopes); + // Grants already stored stay, so an app that left the navigation doesn't block saving + $appGrants = [...$grantableApps, ...($this->fromBlob($stored) ?? [])]; + $blob[self::KEY] = $this->validate($requestedScopes, $appGrants); } } @@ -74,10 +77,11 @@ public function updateScope(array $stored, array $requested): array { } /** + * @param list $appGrants app ids allowed as whole-app grants * @return list * @throws \InvalidArgumentException */ - private function validate(array $scopes): array { + private function validate(array $scopes, array $appGrants): array { if (count($scopes) > self::MAX_SCOPES) { throw new \InvalidArgumentException('Too many scopes'); } @@ -85,6 +89,10 @@ private function validate(array $scopes): array { if (!is_string($scope) || $scope === '' || strlen($scope) > self::MAX_SCOPE_LENGTH) { throw new \InvalidArgumentException('Invalid scope'); } + // A scope without a colon grants a whole app + if (!str_contains($scope, ':') && !in_array($scope, $appGrants, true)) { + throw new \InvalidArgumentException('Invalid scope'); + } } // Matched by app id: files_sharing can be disabled, and its classes don't load then $files = array_filter($scopes, static fn (string $scope) => in_array(strstr($scope, ':', true) ?: $scope, ['files', 'files_sharing', 'files_trashbin', 'files_versions'], true)); diff --git a/tests/lib/AppFramework/Middleware/Security/TokenScopeMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/TokenScopeMiddlewareTest.php index 8a7ede4682567..57f8229c86618 100644 --- a/tests/lib/AppFramework/Middleware/Security/TokenScopeMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/TokenScopeMiddlewareTest.php @@ -122,6 +122,10 @@ public static function requests(): array { 'app requires its own scope' => [['deck:boards:read'], TokenScopeTestController::class, 'deck', true, 'deck'], 'app requires another app\'s scope' => [[...self::FILES, SharingTokenScopes::SHARES_WRITE], TokenScopeTestController::class, 'share', false, 'deck'], 'app opens itself to every token' => [[], TokenScopeTestController::class, 'anyToken', false, 'deck'], + 'whole app, unannotated' => [['deck'], TokenScopeTestController::class, 'unannotated', true, 'deck'], + 'whole app, annotated' => [['deck'], TokenScopeTestController::class, 'deck', true, 'deck'], + 'whole app of another app' => [['notes'], TokenScopeTestController::class, 'unannotated', false, 'deck'], + 'whole app, admin only' => [['deck'], TokenScopeTestController::class, 'adminOnly', false, 'deck'], ]; } diff --git a/tests/lib/Authentication/Token/GrantableAppsTest.php b/tests/lib/Authentication/Token/GrantableAppsTest.php new file mode 100644 index 0000000000000..6ea5a0767fa50 --- /dev/null +++ b/tests/lib/Authentication/Token/GrantableAppsTest.php @@ -0,0 +1,31 @@ +createMock(INavigationManager::class); + $navigationManager->method('getAll')->with(INavigationManager::TYPE_APPS)->willReturn([ + 'files' => ['id' => 'files', 'app' => 'files'], + 'deck' => ['id' => 'deck', 'app' => 'deck'], + 'dashboard' => ['id' => 'dashboard', 'app' => 'dashboard'], + 'calendar' => ['id' => 'calendar', 'app' => 'calendar'], + 'tables_application_3' => ['id' => 'tables_application_3', 'app' => 'tables'], + 'tables_application_4' => ['id' => 'tables_application_4', 'app' => 'tables'], + 'spreed' => ['id' => 'spreed'], + ]); + + $this->assertSame(['deck', 'tables', 'spreed'], (new GrantableApps($navigationManager))->get()); + } +} diff --git a/tests/lib/Authentication/Token/TokenScopesTest.php b/tests/lib/Authentication/Token/TokenScopesTest.php index d14f059e47239..371d9f9728590 100644 --- a/tests/lib/Authentication/Token/TokenScopesTest.php +++ b/tests/lib/Authentication/Token/TokenScopesTest.php @@ -58,7 +58,7 @@ public static function merges(): array { #[DataProvider('merges')] public function testUpdateScope(array $stored, array $requested, array $expected): void { - $this->assertSame($expected, $this->tokenScopes->updateScope($stored, $requested)); + $this->assertSame($expected, $this->tokenScopes->updateScope($stored, $requested, [])); } public static function invalidUpdates(): array { @@ -82,7 +82,35 @@ public static function invalidUpdates(): array { #[DataProvider('invalidUpdates')] public function testInvalidUpdateIsRejected(array $requested): void { $this->expectException(\InvalidArgumentException::class); - $this->tokenScopes->updateScope([IToken::SCOPE_FILESYSTEM => true], $requested); + $this->tokenScopes->updateScope([IToken::SCOPE_FILESYSTEM => true], $requested, []); + } + + public function testAppGrant(): void { + $this->assertSame( + [IToken::SCOPE_FILESYSTEM => false, TokenScopes::KEY => ['deck']], + $this->tokenScopes->updateScope([IToken::SCOPE_FILESYSTEM => true], [TokenScopes::KEY => ['deck']], ['deck']), + ); + } + + public function testStoredAppGrantSurvivesTheAppLeaving(): void { + $stored = [IToken::SCOPE_FILESYSTEM => false, TokenScopes::KEY => ['deck']]; + $this->assertSame( + [IToken::SCOPE_FILESYSTEM => false, TokenScopes::KEY => ['dav:calendar:read', 'deck']], + $this->tokenScopes->updateScope($stored, [TokenScopes::KEY => ['deck', 'dav:calendar:read']], []), + ); + } + + public static function invalidAppGrants(): array { + return [ + 'app not grantable' => ['notes'], + 'capability' => ['files'], + ]; + } + + #[DataProvider('invalidAppGrants')] + public function testInvalidAppGrantIsRejected(string $scope): void { + $this->expectException(\InvalidArgumentException::class); + $this->tokenScopes->updateScope([IToken::SCOPE_FILESYSTEM => true], [TokenScopes::KEY => [$scope]], ['deck']); } public static function blobs(): array { From 3c8d3c5a8b0f7c74fb0b2e3378f7dd7fde923096 Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Fri, 23 Oct 2026 14:38:03 +0200 Subject: [PATCH 2/2] feat(settings): let users limit app passwords to whole apps Signed-off-by: Peter Ringelmann --- .../lib/Controller/AuthSettingsController.php | 8 +-- .../Settings/Personal/Security/Authtokens.php | 4 ++ .../components/AuthTokenAccessPicker.vue | 65 +++++++++++++++++-- .../src/authtokens/utils/accessLevel.spec.ts | 40 ++++++++---- .../src/authtokens/utils/accessLevel.ts | 33 ++++++++-- .../Controller/AuthSettingsControllerTest.php | 1 + .../Personal/Security/AuthtokensTest.php | 8 ++- build/integration/features/auth.feature | 10 +++ .../Authentication/Token/GrantableApps.php | 36 +++++++--- .../Authentication/Token/TokenScopes.php | 2 +- .../Token/GrantableAppsTest.php | 28 +++++--- .../e2e/settings/devices-sessions.spec.ts | 12 ++++ .../sections/DevicesSessionsSettingsPage.ts | 19 ++++-- 13 files changed, 216 insertions(+), 50 deletions(-) diff --git a/apps/settings/lib/Controller/AuthSettingsController.php b/apps/settings/lib/Controller/AuthSettingsController.php index efde78fe0d212..071babc93975d 100644 --- a/apps/settings/lib/Controller/AuthSettingsController.php +++ b/apps/settings/lib/Controller/AuthSettingsController.php @@ -116,8 +116,8 @@ public function create(string $name = '', bool $qrcodeLogin = false, ?array $sco $type = IToken::PERMANENT_TOKEN; try { $scope = $scopes === null ? null : $this->tokenScopes->updateScope([], [TokenScopes::KEY => $scopes], $this->grantableApps->get()); - } catch (\InvalidArgumentException) { - return new JSONResponse([], Http::STATUS_BAD_REQUEST); + } catch (\InvalidArgumentException $e) { + return new JSONResponse(['message' => $e->getMessage()], Http::STATUS_BAD_REQUEST); } } @@ -258,8 +258,8 @@ public function update(int $id, array $scope, string $name): JSONResponse { try { $newScope = $this->tokenScopes->updateScope($currentScope, $scope, $this->grantableApps->get()); - } catch (\InvalidArgumentException) { - return new JSONResponse([], Http::STATUS_BAD_REQUEST); + } catch (\InvalidArgumentException $e) { + return new JSONResponse(['message' => $e->getMessage()], Http::STATUS_BAD_REQUEST); } if ($newScope !== $currentScope) { diff --git a/apps/settings/lib/Settings/Personal/Security/Authtokens.php b/apps/settings/lib/Settings/Personal/Security/Authtokens.php index 9da095a6cc765..230a13e9b533e 100644 --- a/apps/settings/lib/Settings/Personal/Security/Authtokens.php +++ b/apps/settings/lib/Settings/Personal/Security/Authtokens.php @@ -9,6 +9,7 @@ namespace OCA\Settings\Settings\Personal\Security; +use OC\Authentication\Token\GrantableApps; use OC\Authentication\Token\INamedToken; use OC\Authentication\Token\IProvider as IAuthTokenProvider; use OC\Authentication\Token\IToken; @@ -33,6 +34,7 @@ public function __construct( private IInitialState $initialState, private IConfig $serverConfig, private ?string $userId, + private GrantableApps $grantableApps, ) { } @@ -49,6 +51,8 @@ public function getForm(): TemplateResponse { && $this->serverConfig->getSystemValueBool('auth_can_create_app_token', true) ); + $this->initialState->provideInitialState('grantable_apps', $this->grantableApps->entries()); + Util::addStyle(Application::APP_ID, 'personal-security-authtokens'); Util::addScript(Application::APP_ID, 'personal-security-authtokens'); diff --git a/apps/settings/src/authtokens/components/AuthTokenAccessPicker.vue b/apps/settings/src/authtokens/components/AuthTokenAccessPicker.vue index 75281611d8d7c..d515732982bf7 100644 --- a/apps/settings/src/authtokens/components/AuthTokenAccessPicker.vue +++ b/apps/settings/src/authtokens/components/AuthTokenAccessPicker.vue @@ -11,7 +11,7 @@ import { computed, useTemplateRef, watch } from 'vue' import NcCheckboxRadioSwitch from '@nextcloud/vue/components/NcCheckboxRadioSwitch' import NcNoteCard from '@nextcloud/vue/components/NcNoteCard' import NcRadioGroup from '@nextcloud/vue/components/NcRadioGroup' -import { AREA_OPTIONS, AREAS, areaState, GRANT_LABELS, PRESET_GRANTS, SERVICES, setGrant } from '../utils/accessLevel.ts' +import { appLabel, AREA_OPTIONS, AREAS, areaState, GRANT_LABELS, GRANTABLE_APPS, PRESET_GRANTS, SERVICES, setGrant, toScopes } from '../utils/accessLevel.ts' /** What the app password can reach */ const selection = defineModel({ required: true }) @@ -23,19 +23,23 @@ const props = defineProps<{ const areas = useTemplateRef('areas') +const acrossApps = GRANTABLE_APPS.filter((app) => app.acrossApps) +const plainApps = GRANTABLE_APPS.filter((app) => !app.acrossApps) +const filesDescription = t('settings', 'Most apps need this for attachments and other file features.') + const preset = computed({ get: () => selection.value.preset, set(value: string) { const name = value as Preset - const grants = PRESET_GRANTS[name as keyof typeof PRESET_GRANTS] ?? selection.value.grants - selection.value = { preset: name, grants } + const grants = PRESET_GRANTS[name as keyof typeof PRESET_GRANTS] + selection.value = grants ? { preset: name, grants, apps: [] } : { ...selection.value, preset: name } }, }) // Custom needs something to save, a ticked box or the kept scopes watch([() => selection.value, areas], () => { - const empty = selection.value.grants.length === 0 && props.kept.length === 0 - areas.value?.querySelector('input')?.setCustomValidity(empty ? t('settings', 'Choose at least one area') : '') + const empty = toScopes(selection.value, props.kept)?.length === 0 + areas.value?.querySelector('input')?.setCustomValidity(empty ? t('settings', 'Choose at least one option') : '') }, { flush: 'post', immediate: true }) /** @@ -43,7 +47,16 @@ watch([() => selection.value, areas], () => { * @param on Whether to tick it */ function toggle(grant: Grant, on: boolean) { - selection.value = { preset: 'custom', grants: setGrant(selection.value.grants, grant, on) } + selection.value = { ...selection.value, preset: 'custom', grants: setGrant(selection.value.grants, grant, on) } +} + +/** + * @param app App id to change + * @param on Whether to tick it + */ +function toggleApp(app: string, on: boolean) { + const apps = selection.value.apps.filter((id) => id !== app) + selection.value = { ...selection.value, preset: 'custom', apps: on ? [...apps, app] : apps } } @@ -75,6 +88,7 @@ function toggle(grant: Grant, on: boolean) { {{ GRANT_LABELS[area] }} @@ -95,6 +109,24 @@ function toggle(grant: Grant, on: boolean) { @update:modelValue="toggle(service, $event)"> {{ GRANT_LABELS[service] }} + + {{ appLabel(app) }} + +
+ + + + {{ app.name }} + + @@ -117,5 +149,26 @@ function toggle(grant: Grant, on: boolean) { &__options { padding-inline-start: calc(8 * var(--default-grid-baseline)); } + + &__divider { + // Same inset as the checkboxes, which NcCheckboxRadioSwitch centers in a clickable area + margin-block: calc(2 * var(--default-grid-baseline)); + margin-inline: calc((var(--default-clickable-area) - 20px) / 2); + border: none; + border-top: 1px solid var(--color-border); + } + + &__app { + display: flex; + align-items: center; + gap: calc(2 * var(--default-grid-baseline)); + } + + &__app-icon { + width: calc(4 * var(--default-grid-baseline)); + height: calc(4 * var(--default-grid-baseline)); + // Navigation icons are white, for the header + filter: var(--background-invert-if-bright); + } } diff --git a/apps/settings/src/authtokens/utils/accessLevel.spec.ts b/apps/settings/src/authtokens/utils/accessLevel.spec.ts index ca580922f7c8a..85ca4dacbe555 100644 --- a/apps/settings/src/authtokens/utils/accessLevel.spec.ts +++ b/apps/settings/src/authtokens/utils/accessLevel.spec.ts @@ -5,7 +5,14 @@ import type { Selection } from './accessLevel.ts' -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' +vi.mock('@nextcloud/initial-state', () => ({ + loadState: () => [ + { id: 'spreed', name: 'Talk', icon: '', acrossApps: false }, + { id: 'activity', name: 'Activity', icon: '', acrossApps: true }, + ], +})) + import { accessLabel, areaState, fromScopes, setGrant, toScopes } from './accessLevel.ts' const files = ['files:files:read', 'files:files:write', 'files:dav:read', 'files:dav:write', 'files_sharing:shares:write'] @@ -13,21 +20,32 @@ const calendarContacts = ['dav:calendar:read', 'dav:calendar:write', 'dav:contac describe('accessLevel', () => { it.each<[string, Selection]>([ - ['files', { preset: 'files', grants: ['files', 'filesShare'] }], - ['files with trash and versions', { preset: 'custom', grants: ['files', 'filesShare', 'filesTrash', 'filesVersions'] }], - ['calendar and contacts', { preset: 'calendarContacts', grants: ['calendar', 'calendarWrite', 'contacts', 'contactsWrite'] }], - ['files without sharing', { preset: 'custom', grants: ['files'] }], - ['read-only calendar', { preset: 'custom', grants: ['calendar'] }], - ['files with search', { preset: 'custom', grants: ['files', 'filesShare', 'search'] }], - ['people lookup alone', { preset: 'custom', grants: ['people'] }], + ['files', { preset: 'files', grants: ['files', 'filesShare'], apps: [] }], + ['files with trash and versions', { preset: 'custom', grants: ['files', 'filesShare', 'filesTrash', 'filesVersions'], apps: [] }], + ['calendar and contacts', { preset: 'calendarContacts', grants: ['calendar', 'calendarWrite', 'contacts', 'contactsWrite'], apps: [] }], + ['files without sharing', { preset: 'custom', grants: ['files'], apps: [] }], + ['read-only calendar', { preset: 'custom', grants: ['calendar'], apps: [] }], + ['files with search', { preset: 'custom', grants: ['files', 'filesShare', 'search'], apps: [] }], + ['people lookup alone', { preset: 'custom', grants: ['people'], apps: [] }], ])('round-trips %s', (_, selection) => { const scopes = toScopes(selection, [])! expect(fromScopes(scopes)).toEqual({ selection, kept: [] }) }) + it('reads app ids as ticked apps and keeps unknown ones', () => { + const { selection, kept } = fromScopes(['spreed', 'deck']) + expect(selection).toEqual({ preset: 'custom', grants: [], apps: ['spreed'] }) + expect(kept).toEqual(['deck']) + expect(toScopes(selection, kept)).toEqual(['spreed', 'deck']) + }) + + it('labels apps, with the ones that show other apps\' data marked', () => { + expect(accessLabel({ filesystem: true, scopes: [...files, 'spreed', 'activity'] })).toBe('Files, Talk, and Activity across all apps') + }) + it('reads a token without scopes as all access', () => { expect(fromScopes(undefined).selection.preset).toBe('all') - expect(toScopes({ preset: 'all', grants: ['files'] }, [])).toBeNull() + expect(toScopes({ preset: 'all', grants: ['files'], apps: [] }, [])).toBeNull() }) it('matches presets regardless of order', () => { @@ -38,9 +56,9 @@ describe('accessLevel', () => { const { selection, kept } = fromScopes([...files, 'app:deck', 'dav:calendar:write']) // Write without read is possible through the API, the picker never creates it - expect(selection).toEqual({ preset: 'custom', grants: ['files', 'filesShare'] }) + expect(selection).toEqual({ preset: 'custom', grants: ['files', 'filesShare'], apps: [] }) expect(kept).toEqual(['app:deck', 'dav:calendar:write']) - expect(toScopes({ preset: 'custom', grants: ['contacts'] }, kept)) + expect(toScopes({ preset: 'custom', grants: ['contacts'], apps: [] }, kept)) .toEqual(['dav:contacts:read', 'app:deck', 'dav:calendar:write']) }) diff --git a/apps/settings/src/authtokens/utils/accessLevel.ts b/apps/settings/src/authtokens/utils/accessLevel.ts index aa5abebe0e6f7..a3b22ef481447 100644 --- a/apps/settings/src/authtokens/utils/accessLevel.ts +++ b/apps/settings/src/authtokens/utils/accessLevel.ts @@ -5,6 +5,7 @@ import type { ITokenScope } from '../store/authtoken.ts' +import { loadState } from '@nextcloud/initial-state' import { getLanguage, t } from '@nextcloud/l10n' export type Area = 'files' | 'calendar' | 'contacts' @@ -15,8 +16,20 @@ export type Grant = Area | Service | 'filesShare' | 'filesTrash' | 'filesVersion export type Preset = 'all' | 'files' | 'calendarContacts' | 'custom' type AreaState = 'all' | 'partial' | 'none' +export interface IGrantableApp { + id: string + name: string + icon: string + /** Whether the app shows other apps' data */ + acrossApps: boolean +} + +export const GRANTABLE_APPS = loadState('settings', 'grantable_apps', []) + export interface Selection { preset: Preset + /** Ticked apps, stored as their app id */ + apps: string[] /** Ticked grants, only used when the preset is not `all` */ grants: Grant[] } @@ -71,17 +84,18 @@ const GRANTS = Object.keys(GRANT_SCOPES) as Grant[] */ export function fromScopes(scopes?: string[]): { selection: Selection, kept: string[] } { if (scopes === undefined) { - return { selection: { preset: 'all', grants: [] }, kept: [] } + return { selection: { preset: 'all', grants: [], apps: [] }, kept: [] } } const held = GRANTS.filter((grant) => GRANT_SCOPES[grant].every((scope) => scopes.includes(scope))) const grants = held.filter((grant) => held.includes(parentOf(grant))) - const shown = grants.flatMap((grant) => GRANT_SCOPES[grant]) + const apps = GRANTABLE_APPS.map((app) => app.id).filter((id) => scopes.includes(id)) + const shown = [...grants.flatMap((grant) => GRANT_SCOPES[grant]), ...apps] const kept = scopes.filter((scope) => !shown.includes(scope)) const presets = Object.keys(PRESET_GRANTS) as (keyof typeof PRESET_GRANTS)[] - const preset = kept.length === 0 ? presets.find((name) => sameItems(PRESET_GRANTS[name], grants)) : undefined - return { selection: { preset: preset ?? 'custom', grants }, kept } + const preset = kept.length === 0 && apps.length === 0 ? presets.find((name) => sameItems(PRESET_GRANTS[name], grants)) : undefined + return { selection: { preset: preset ?? 'custom', grants, apps }, kept } } /** @@ -94,7 +108,7 @@ export function toScopes(selection: Selection, kept: string[]): string[] | null if (selection.preset === 'all') { return null } - return [...new Set([...selection.grants.flatMap((grant) => GRANT_SCOPES[grant]), ...kept])] + return [...new Set([...selection.grants.flatMap((grant) => GRANT_SCOPES[grant]), ...selection.apps, ...kept])] } /** @@ -149,6 +163,7 @@ export function accessLabel(scope: ITokenScope): string { return [(state === 'partial' && readOnly[area]) || GRANT_LABELS[area]] }) parts.push(...SERVICES.filter((service) => selection.grants.includes(service)).map((service) => GRANT_LABELS[service])) + parts.push(...GRANTABLE_APPS.filter((app) => selection.apps.includes(app.id)).map(appLabel)) if (kept.length > 0) { // TRANSLATORS Last item of a list such as "Files and other access" parts.push(parts.length === 0 ? t('settings', 'Other access') : t('settings', 'other access')) @@ -159,6 +174,14 @@ export function accessLabel(scope: ITokenScope): string { return new Intl.ListFormat(getLanguage(), { type: 'conjunction' }).format(parts) } +/** + * @param app An app the app password can be limited to + */ +export function appLabel(app: IGrantableApp): string { + // TRANSLATORS {app} is an app name, e.g. "Activity". The app password then reads that app's data from all other apps + return app.acrossApps ? t('settings', '{app} across all apps', { app: app.name }) : app.name +} + /** * @param grant An area, sub-option or service */ diff --git a/apps/settings/tests/Controller/AuthSettingsControllerTest.php b/apps/settings/tests/Controller/AuthSettingsControllerTest.php index c1dcaa51478f7..39d42fb84d748 100644 --- a/apps/settings/tests/Controller/AuthSettingsControllerTest.php +++ b/apps/settings/tests/Controller/AuthSettingsControllerTest.php @@ -174,6 +174,7 @@ public function testCreateInvalidScopes(): void { $response = $this->controller->create('Nexus 4', false, [FilesTokenScopes::FILES_WRITE]); $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertSame(['message' => 'Files scopes require ' . FilesTokenScopes::FILES_READ], $response->getData()); } public function testCreateDisabledBySystemConfig(): void { diff --git a/apps/settings/tests/Settings/Personal/Security/AuthtokensTest.php b/apps/settings/tests/Settings/Personal/Security/AuthtokensTest.php index 9ba738c60e24a..b870f96660506 100644 --- a/apps/settings/tests/Settings/Personal/Security/AuthtokensTest.php +++ b/apps/settings/tests/Settings/Personal/Security/AuthtokensTest.php @@ -9,6 +9,7 @@ namespace OCA\Settings\Tests\Settings\Personal\Security; +use OC\Authentication\Token\GrantableApps; use OC\Authentication\Token\IProvider as IAuthTokenProvider; use OC\Authentication\Token\PublicKeyToken; use OCA\Settings\Settings\Personal\Security\Authtokens; @@ -27,6 +28,7 @@ class AuthtokensTest extends TestCase { private IUserSession&MockObject $userSession; private IInitialState&MockObject $initialState; private IConfig&MockObject $serverConfig; + private GrantableApps&MockObject $grantableApps; private string $uid; private Authtokens $section; @@ -39,6 +41,8 @@ protected function setUp(): void { $this->initialState = $this->createMock(IInitialState::class); $this->serverConfig = $this->createMock(IConfig::class); $this->uid = 'test123'; + $this->grantableApps = $this->createMock(GrantableApps::class); + $this->grantableApps->method('entries')->willReturn([['id' => 'spreed', 'name' => 'Talk', 'icon' => '', 'acrossApps' => false]]); $this->section = new Authtokens( $this->authTokenProvider, @@ -47,6 +51,7 @@ protected function setUp(): void { $this->initialState, $this->serverConfig, $this->uid, + $this->grantableApps, ); } @@ -102,8 +107,9 @@ public function testGetForm(): void { ] ], ['can_create_app_token', true], + ['grantable_apps', [['id' => 'spreed', 'name' => 'Talk', 'icon' => '', 'acrossApps' => false]]], ]; - $this->initialState->expects($this->exactly(2)) + $this->initialState->expects($this->exactly(3)) ->method('provideInitialState') ->willReturnCallback(function () use (&$calls): void { $expected = array_shift($calls); diff --git a/build/integration/features/auth.feature b/build/integration/features/auth.feature index 0bdf65cd839cc..54940e0e5589a 100644 --- a/build/integration/features/auth.feature +++ b/build/integration/features/auth.feature @@ -145,6 +145,16 @@ Feature: auth When requesting "/ocs/v2.php/hovercard/v1/user0" with "GET" using scoped basic token auth Then the HTTP status code should be "200" + Scenario: a dashboard token reaches the dashboard API + Given a new client token limited to "dashboard" is added + When requesting "/ocs/v2.php/apps/dashboard/api/v1/widgets" with "GET" using scoped basic token auth + Then the HTTP status code should be "200" + + Scenario: a dashboard token reads no files + Given a new client token limited to "dashboard" is added + When requesting "/ocs/v2.php/apps/files_sharing/api/v1/shares" with "GET" using scoped basic token auth + Then the HTTP status code should be "403" + Scenario: a files token gets no page render Given a new client token limited to "files:files:read files:files:write" is added When requesting "/index.php/apps/files" with "GET" using scoped basic token auth diff --git a/lib/private/Authentication/Token/GrantableApps.php b/lib/private/Authentication/Token/GrantableApps.php index 449af69b04ead..1ac29eca7a8d4 100644 --- a/lib/private/Authentication/Token/GrantableApps.php +++ b/lib/private/Authentication/Token/GrantableApps.php @@ -16,9 +16,14 @@ */ class GrantableApps { /** - * Apps that show other apps' data, and apps whose access has scopes of its own + * AI apps, and apps whose access has scopes of its own */ - private const EXCLUDED = ['activity', 'assistant', 'calendar', 'contacts', 'dashboard', 'files', 'tasks']; + private const EXCLUDED = ['assistant', 'calendar', 'contacts', 'files', 'tasks']; + + /** + * Apps that show other apps' data + */ + private const ACROSS_APPS = ['activity', 'dashboard']; public function __construct( private INavigationManager $navigationManager, @@ -26,15 +31,28 @@ public function __construct( } /** - * Apps in the current user's navigation, so restricted apps stay out - * * @return list */ public function get(): array { - $apps = array_map( - static fn (array $entry): string => $entry['app'] ?? $entry['id'], - $this->navigationManager->getAll(INavigationManager::TYPE_APPS), - ); - return array_values(array_unique(array_diff($apps, self::EXCLUDED))); + return array_column($this->entries(), 'id'); + } + + /** + * Apps in the current user's navigation, so restricted apps stay out. Apps that show other apps' data come first, then by name + * + * @return list + */ + public function entries(): array { + $entries = []; + foreach ($this->navigationManager->getAll(INavigationManager::TYPE_APPS) as $entry) { + $app = $entry['app'] ?? $entry['id']; + // An app's main entry wins over its extra ones, like the Tables contexts + if (in_array($app, self::EXCLUDED, true) || (isset($entries[$app]) && $entry['id'] !== $app)) { + continue; + } + $entries[$app] = ['id' => $app, 'name' => $entry['name'], 'icon' => $entry['icon'], 'acrossApps' => in_array($app, self::ACROSS_APPS, true)]; + } + usort($entries, static fn (array $a, array $b): int => [$b['acrossApps'], strcasecmp($a['name'], $b['name'])] <=> [$a['acrossApps'], 0]); + return $entries; } } diff --git a/lib/private/Authentication/Token/TokenScopes.php b/lib/private/Authentication/Token/TokenScopes.php index 8c9f8b4079794..fbf7a75b35a1a 100644 --- a/lib/private/Authentication/Token/TokenScopes.php +++ b/lib/private/Authentication/Token/TokenScopes.php @@ -91,7 +91,7 @@ private function validate(array $scopes, array $appGrants): array { } // A scope without a colon grants a whole app if (!str_contains($scope, ':') && !in_array($scope, $appGrants, true)) { - throw new \InvalidArgumentException('Invalid scope'); + throw new \InvalidArgumentException('Only apps in your navigation can be granted as a whole. Files, calendars and contacts have scopes of their own'); } } // Matched by app id: files_sharing can be disabled, and its classes don't load then diff --git a/tests/lib/Authentication/Token/GrantableAppsTest.php b/tests/lib/Authentication/Token/GrantableAppsTest.php index 6ea5a0767fa50..1c346b89a5e99 100644 --- a/tests/lib/Authentication/Token/GrantableAppsTest.php +++ b/tests/lib/Authentication/Token/GrantableAppsTest.php @@ -14,18 +14,28 @@ use Test\TestCase; class GrantableAppsTest extends TestCase { - public function testNavigationAppsWithoutAggregatorsAndCapabilities(): void { + public function testNavigationAppsWithoutCapabilitiesAndAi(): void { $navigationManager = $this->createMock(INavigationManager::class); $navigationManager->method('getAll')->with(INavigationManager::TYPE_APPS)->willReturn([ - 'files' => ['id' => 'files', 'app' => 'files'], - 'deck' => ['id' => 'deck', 'app' => 'deck'], - 'dashboard' => ['id' => 'dashboard', 'app' => 'dashboard'], - 'calendar' => ['id' => 'calendar', 'app' => 'calendar'], - 'tables_application_3' => ['id' => 'tables_application_3', 'app' => 'tables'], - 'tables_application_4' => ['id' => 'tables_application_4', 'app' => 'tables'], - 'spreed' => ['id' => 'spreed'], + 'files' => ['id' => 'files', 'app' => 'files', 'name' => 'Files', 'icon' => 'files.svg'], + 'deck' => ['id' => 'deck', 'app' => 'deck', 'name' => 'Deck', 'icon' => 'deck.svg'], + 'dashboard' => ['id' => 'dashboard', 'app' => 'dashboard', 'name' => 'Dashboard', 'icon' => 'dashboard.svg'], + 'calendar' => ['id' => 'calendar', 'app' => 'calendar', 'name' => 'Calendar', 'icon' => 'calendar.svg'], + 'assistant' => ['id' => 'assistant', 'app' => 'assistant', 'name' => 'Assistant', 'icon' => 'assistant.svg'], + 'tables_application_3' => ['id' => 'tables_application_3', 'app' => 'tables', 'name' => 'Inventory', 'icon' => 'inventory.svg'], + 'tables' => ['id' => 'tables', 'app' => 'tables', 'name' => 'Tables', 'icon' => 'tables.svg'], + 'spreed' => ['id' => 'spreed', 'name' => 'Talk', 'icon' => 'talk.svg'], + 'activity' => ['id' => 'activity', 'app' => 'activity', 'name' => 'Activity', 'icon' => 'activity.svg'], ]); + $grantableApps = new GrantableApps($navigationManager); - $this->assertSame(['deck', 'tables', 'spreed'], (new GrantableApps($navigationManager))->get()); + $this->assertSame([ + ['id' => 'activity', 'name' => 'Activity', 'icon' => 'activity.svg', 'acrossApps' => true], + ['id' => 'dashboard', 'name' => 'Dashboard', 'icon' => 'dashboard.svg', 'acrossApps' => true], + ['id' => 'deck', 'name' => 'Deck', 'icon' => 'deck.svg', 'acrossApps' => false], + ['id' => 'tables', 'name' => 'Tables', 'icon' => 'tables.svg', 'acrossApps' => false], + ['id' => 'spreed', 'name' => 'Talk', 'icon' => 'talk.svg', 'acrossApps' => false], + ], $grantableApps->entries()); + $this->assertSame(['activity', 'dashboard', 'deck', 'tables', 'spreed'], $grantableApps->get()); } } diff --git a/tests/playwright/e2e/settings/devices-sessions.spec.ts b/tests/playwright/e2e/settings/devices-sessions.spec.ts index ef7f8e6a23197..55bee7ce427ba 100644 --- a/tests/playwright/e2e/settings/devices-sessions.spec.ts +++ b/tests/playwright/e2e/settings/devices-sessions.spec.ts @@ -77,4 +77,16 @@ test.describe('Settings: Devices & sessions', () => { await devicesSessionsPage.editAccess('Thunderbird', 'Calendar and contacts') await expect(devicesSessionsPage.tokenRow('Thunderbird')).toContainText('Calendar and Contacts') }) + + test('limits an app password to picked areas and apps', async ({ devicesSessionsPage }) => { + await devicesSessionsPage.open() + + await devicesSessionsPage.createAppPassword('Script', 'Custom', ['Calendar', 'Dashboard across all apps']) + await expect(devicesSessionsPage.tokenRow('Script')).toContainText('Calendar and Dashboard across all apps') + + const dialog = await devicesSessionsPage.openEditAccess('Script') + await expect(dialog.getByRole('radio', { name: 'Custom' })).toBeChecked() + await expect(dialog.getByRole('checkbox', { name: 'Dashboard across all apps', exact: true })).toBeChecked() + await expect(dialog.getByRole('checkbox', { name: 'Files', exact: true })).not.toBeChecked() + }) }) diff --git a/tests/playwright/support/sections/DevicesSessionsSettingsPage.ts b/tests/playwright/support/sections/DevicesSessionsSettingsPage.ts index 43f599776b972..e9f0f987d0ace 100644 --- a/tests/playwright/support/sections/DevicesSessionsSettingsPage.ts +++ b/tests/playwright/support/sections/DevicesSessionsSettingsPage.ts @@ -73,12 +73,16 @@ export class DevicesSessionsSettingsPage { /** * @param name - Name of the new app password * @param access - Label of the access preset to pick + * @param options - Labels of the boxes to tick, with the Custom preset */ - async createAppPassword(name: string, access: string): Promise { + async createAppPassword(name: string, access: string, options: string[] = []): Promise { await this.page.getByRole('button', { name: 'Create new app password' }).click() const dialog = this.accessDialog('Create new app password') await dialog.getByRole('textbox', { name: 'App name' }).fill(name) await dialog.getByRole('radio', { name: access }).check({ force: true }) + for (const option of options) { + await dialog.getByRole('checkbox', { name: option, exact: true }).check({ force: true }) + } const created = this.page.waitForResponse((r) => r.request().method() === 'POST' && r.url().includes('/settings/personal/authtokens') @@ -97,9 +101,7 @@ export class DevicesSessionsSettingsPage { * @param access - Label of the access preset to pick */ async editAccess(row: string, access: string): Promise { - await this.tokenRow(row).getByRole('button', { name: 'Device settings' }).click() - await this.page.getByRole('menuitem', { name: 'Edit access' }).click() - const dialog = this.accessDialog('Edit access') + const dialog = await this.openEditAccess(row) await dialog.getByRole('radio', { name: access }).check({ force: true }) const saved = this.page.waitForResponse((r) => r.request().method() === 'PUT' @@ -109,4 +111,13 @@ export class DevicesSessionsSettingsPage { await awaitPasswordGuardedRequest(this.page, saved) await expect(dialog).toBeHidden() } + + /** + * @param row - Visible name of the app password + */ + async openEditAccess(row: string): Promise { + await this.tokenRow(row).getByRole('button', { name: 'Device settings' }).click() + await this.page.getByRole('menuitem', { name: 'Edit access' }).click() + return this.accessDialog('Edit access') + } }