From a032b9dfd3cbccc39ea3d335cad9ae66e83de57a Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Thu, 15 Oct 2026 11:39:28 +0200 Subject: [PATCH 1/3] feat(dav): limit scoped app passwords to the collections they open Signed-off-by: Peter Ringelmann --- apps/dav/appinfo/v1/caldav.php | 2 + apps/dav/appinfo/v1/carddav.php | 2 + apps/dav/appinfo/v1/webdav.php | 4 + .../composer/composer/autoload_classmap.php | 1 + .../dav/composer/composer/autoload_static.php | 1 + .../lib/Connector/Sabre/TokenScopePlugin.php | 104 ++++++++++++ apps/dav/lib/Server.php | 2 + .../Connector/Sabre/TokenScopePluginTest.php | 157 ++++++++++++++++++ build/integration/features/auth.feature | 40 +++++ 9 files changed, 313 insertions(+) create mode 100644 apps/dav/lib/Connector/Sabre/TokenScopePlugin.php create mode 100644 apps/dav/tests/unit/Connector/Sabre/TokenScopePluginTest.php diff --git a/apps/dav/appinfo/v1/caldav.php b/apps/dav/appinfo/v1/caldav.php index 9547ff88c8dc9..cf6f935da4939 100644 --- a/apps/dav/appinfo/v1/caldav.php +++ b/apps/dav/appinfo/v1/caldav.php @@ -22,6 +22,7 @@ use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin; use OCA\DAV\Connector\Sabre\MaintenancePlugin; use OCA\DAV\Connector\Sabre\Principal; +use OCA\DAV\Connector\Sabre\TokenScopePlugin; use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; use OCP\EventDispatcher\IEventDispatcher; @@ -110,6 +111,7 @@ // Add plugins $server->addPlugin(new MaintenancePlugin(Server::get(IConfig::class), $davL10n)); $server->addPlugin(new \Sabre\DAV\Auth\Plugin($authBackend)); +$server->addPlugin(Server::get(TokenScopePlugin::class)); $server->addPlugin(new \Sabre\CalDAV\Plugin()); $server->addPlugin(new LegacyDAVACL()); diff --git a/apps/dav/appinfo/v1/carddav.php b/apps/dav/appinfo/v1/carddav.php index 4f5218855235e..36d2e03234de3 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -21,6 +21,7 @@ use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin; use OCA\DAV\Connector\Sabre\MaintenancePlugin; use OCA\DAV\Connector\Sabre\Principal; +use OCA\DAV\Connector\Sabre\TokenScopePlugin; use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; use OCP\EventDispatcher\IEventDispatcher; @@ -92,6 +93,7 @@ // Add plugins $server->addPlugin(new MaintenancePlugin(Server::get(IConfig::class), Server::get(IL10nFactory::class)->get('dav'))); $server->addPlugin(new \Sabre\DAV\Auth\Plugin($authBackend)); +$server->addPlugin(Server::get(TokenScopePlugin::class)); $server->addPlugin(new Plugin()); $server->addPlugin(new LegacyDAVACL()); diff --git a/apps/dav/appinfo/v1/webdav.php b/apps/dav/appinfo/v1/webdav.php index 5453c0a8bfb05..de90baf3a1db0 100644 --- a/apps/dav/appinfo/v1/webdav.php +++ b/apps/dav/appinfo/v1/webdav.php @@ -6,9 +6,11 @@ * SPDX-License-Identifier: AGPL-3.0-only */ use OC\Files\Filesystem; +use OC\Lockdown\LockdownManager; use OCA\DAV\Connector\Sabre\Auth; use OCA\DAV\Connector\Sabre\BearerAuth; use OCA\DAV\Connector\Sabre\ServerFactory; +use OCA\DAV\Connector\Sabre\TokenScopePlugin; use OCA\DAV\Events\SabrePluginAddEvent; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Mount\IMountManager; @@ -76,6 +78,8 @@ // use the view for the logged in user return Filesystem::getView(); }); +// The legacy tree is the user's files root +$server->addPlugin(new TokenScopePlugin(Server::get(LockdownManager::class), 'files/')); // allow setup of additional plugins $event = new SabrePluginEvent($server); diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 36779c3833f85..0281e5d4a3024 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -273,6 +273,7 @@ 'OCA\\DAV\\Connector\\Sabre\\StreamedPropFindNotFoundPlugin' => $baseDir . '/../lib/Connector/Sabre/StreamedPropFindNotFoundPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\TagList' => $baseDir . '/../lib/Connector/Sabre/TagList.php', 'OCA\\DAV\\Connector\\Sabre\\TagsPlugin' => $baseDir . '/../lib/Connector/Sabre/TagsPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\TokenScopePlugin' => $baseDir . '/../lib/Connector/Sabre/TokenScopePlugin.php', 'OCA\\DAV\\Connector\\Sabre\\UserIdHeaderPlugin' => $baseDir . '/../lib/Connector/Sabre/UserIdHeaderPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\ZipFolderPlugin' => $baseDir . '/../lib/Connector/Sabre/ZipFolderPlugin.php', 'OCA\\DAV\\Controller\\BirthdayCalendarController' => $baseDir . '/../lib/Controller/BirthdayCalendarController.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 13397cb409415..f95a016d95c7f 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -288,6 +288,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Connector\\Sabre\\StreamedPropFindNotFoundPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/StreamedPropFindNotFoundPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\TagList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/TagList.php', 'OCA\\DAV\\Connector\\Sabre\\TagsPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/TagsPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\TokenScopePlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/TokenScopePlugin.php', 'OCA\\DAV\\Connector\\Sabre\\UserIdHeaderPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/UserIdHeaderPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\ZipFolderPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ZipFolderPlugin.php', 'OCA\\DAV\\Controller\\BirthdayCalendarController' => __DIR__ . '/..' . '/../lib/Controller/BirthdayCalendarController.php', diff --git a/apps/dav/lib/Connector/Sabre/TokenScopePlugin.php b/apps/dav/lib/Connector/Sabre/TokenScopePlugin.php new file mode 100644 index 0000000000000..22e70ba87365a --- /dev/null +++ b/apps/dav/lib/Connector/Sabre/TokenScopePlugin.php @@ -0,0 +1,104 @@ +server = $server; + // After authentication (10), before the ACL plugin (20) + $server->on('beforeMethod:*', [$this, 'beforeMethod'], 15); + } + + public function beforeMethod(RequestInterface $request): void { + if (!$this->lockdownManager->isScoped()) { + return; + } + + $method = $request->getMethod(); + $destination = $method === 'COPY' || $method === 'MOVE' ? $request->getHeader('Destination') : null; + // COPY only reads its source + if (!$this->allows($request->getPath(), $method === 'COPY' ? 'GET' : $method) + || ($destination !== null && !$this->allows($this->server->calculateUri($destination), $method))) { + throw new Forbidden('Not allowed by the app password scopes'); + } + + // Registered here so unscoped requests skip the per-node check. Priority 0 runs before any handler that loads data + $this->server->on('propFind', [$this, 'propFind'], 0); + } + + /** + * Returning false hides the node from listings, multiget and expand-property + */ + public function propFind(PropFind $propFind, INode $node): ?bool { + return $this->allows($propFind->getPath(), 'PROPFIND') ? null : false; + } + + public function allows(string $path, string $method): bool { + $segments = preg_split('#/+#', $this->prefix . $path, -1, PREG_SPLIT_NO_EMPTY); + $write = !in_array($method, self::READ_METHODS, true); + + return match ($segments[0] ?? '') { + '' => !$write, + CalDAVPlugin::CALENDAR_ROOT, CalDAVPlugin::SYSTEM_CALENDAR_ROOT => $this->allowsGroupware($segments, $method, $write, TokenScopes::CALENDAR_READ, TokenScopes::CALENDAR_WRITE), + CardDAVPlugin::ADDRESSBOOK_ROOT => $this->allowsGroupware($segments, $method, $write, TokenScopes::CONTACTS_READ, TokenScopes::CONTACTS_WRITE), + 'principals' => $write + // Proxy membership grants access to all calendars, which counts as sharing + ? !in_array(end($segments), ['calendar-proxy-read', 'calendar-proxy-write'], true) && $this->lockdownManager->hasScope(TokenScopes::CALENDAR_WRITE) + : $this->lockdownManager->hasScope(TokenScopes::CALENDAR_READ) || $this->lockdownManager->hasScope(TokenScopes::CONTACTS_READ), + 'avatars' => $method === 'GET' || $method === 'HEAD', + default => in_array($segments[0], self::FILES_COLLECTIONS, true) && $this->lockdownManager->hasScope(FilesTokenScopes::FILES_READ), + }; + } + + /** + * @param list $segments + */ + private function allowsGroupware(array $segments, string $method, bool $write, string $readScope, string $writeScope): bool { + // POST shares and publishes calendars and address books, except on the scheduling outbox + if ($method === 'POST') { + return $segments[0] === CalDAVPlugin::CALENDAR_ROOT && count($segments) === 3 && $segments[2] === 'outbox' + && $this->lockdownManager->hasScope($writeScope); + } + + return $this->lockdownManager->hasScope($write ? $writeScope : $readScope); + } +} diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index e80229b2e3398..66f7a3e284a76 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -53,6 +53,7 @@ use OCA\DAV\Connector\Sabre\RequestIdHeaderPlugin; use OCA\DAV\Connector\Sabre\SharesPlugin; use OCA\DAV\Connector\Sabre\TagsPlugin; +use OCA\DAV\Connector\Sabre\TokenScopePlugin; use OCA\DAV\Connector\Sabre\UserIdHeaderPlugin; use OCA\DAV\Connector\Sabre\ZipFolderPlugin; use OCA\DAV\DAV\CustomPropertiesBackend; @@ -179,6 +180,7 @@ public function __construct( $authPlugin->addBackend($bearerAuthBackend); // because we are throwing exceptions this plugin has to be the last one $authPlugin->addBackend($authBackend); + $this->server->addPlugin(\OCP\Server::get(TokenScopePlugin::class)); // debugging if ($debugEnabled) { diff --git a/apps/dav/tests/unit/Connector/Sabre/TokenScopePluginTest.php b/apps/dav/tests/unit/Connector/Sabre/TokenScopePluginTest.php new file mode 100644 index 0000000000000..389264a056eaa --- /dev/null +++ b/apps/dav/tests/unit/Connector/Sabre/TokenScopePluginTest.php @@ -0,0 +1,157 @@ +|null $scopes null for a token without scopes + */ + private function plugin(?array $scopes, string $prefix = ''): TokenScopePlugin { + $lockdownManager = new LockdownManager(fn () => $this->createMock(ISession::class), new CoreTokenScopes()); + $token = new PublicKeyToken(); + $token->setScope($scopes === null ? [] : [CoreTokenScopes::KEY => $scopes]); + $lockdownManager->setToken($token); + return new TokenScopePlugin($lockdownManager, $prefix); + } + + public static function dataAllows(): array { + return [ + 'root, any key' => ['', 'PROPFIND', [], true], + 'root write' => ['', 'PROPPATCH', self::FILES, false], + 'files' => ['files/user0/a.txt', 'GET', self::FILES, true], + 'files write, mask guards it' => ['files/user0/a.txt', 'PUT', self::FILES, true], + 'files without files' => ['files/user0', 'PROPFIND', self::CALENDAR, false], + 'uploads' => ['uploads/user0/1', 'MKCOL', self::FILES, true], + 'bulk upload' => ['bulk', 'POST', self::FILES, true], + 'bulk upload without files' => ['bulk', 'POST', self::CALENDAR, false], + 'groupfolder trash without files' => ['trashbin/user0/trash', 'PROPFIND', self::CALENDAR, false], + 'groupfolders' => ['groupfolders/user0', 'PROPFIND', self::FILES, true], + 'versions' => ['versions/user0/versions/1', 'PROPFIND', self::FILES, true], + 'tag definitions read' => ['systemtags/1', 'PROPFIND', self::FILES, true], + 'tag definitions write' => ['systemtags/1', 'PROPPATCH', self::FILES, true], + 'tag definitions without files' => ['systemtags/1', 'PROPFIND', self::CALENDAR, false], + 'tag assignment' => ['systemtags-relations/files/1/2', 'PUT', self::FILES, true], + 'assigned tags' => ['systemtags-assigned/files', 'PROPFIND', self::FILES, true], + 'comments' => ['comments/files/1', 'PROPFIND', self::FILES, false], + 'calendar read' => ['calendars/user0/personal', 'REPORT', self::CALENDAR_READ, true], + 'calendar write without write' => ['calendars/user0/personal/a.ics', 'PUT', self::CALENDAR_READ, false], + 'calendar write' => ['calendars/user0/personal/a.ics', 'PUT', self::CALENDAR, true], + 'calendar without calendar' => ['calendars/user0/personal', 'PROPFIND', self::FILES, false], + 'calendar sharing' => ['calendars/user0/personal', 'POST', self::CALENDAR, false], + 'scheduling outbox' => ['calendars/user0/outbox', 'POST', self::CALENDAR, true], + 'scheduling outbox, doubled slashes' => ['calendars//user0//outbox', 'POST', self::CALENDAR, true], + 'scheduling outbox without write' => ['calendars/user0/outbox', 'POST', self::CALENDAR_READ, false], + 'room calendars' => ['system-calendars/calendar-rooms/room1', 'REPORT', self::CALENDAR, true], + 'address book read' => ['addressbooks/users/user0/contacts', 'PROPFIND', self::CONTACTS_READ, true], + 'address book write without write' => ['addressbooks/users/user0/contacts/a.vcf', 'PUT', self::CONTACTS_READ, false], + 'address book write' => ['addressbooks/users/user0/contacts/a.vcf', 'PUT', self::CONTACTS, true], + 'address book sharing' => ['addressbooks/users/user0/contacts', 'POST', self::CONTACTS, false], + 'principals without groupware' => ['principals/users/user0', 'PROPFIND', self::FILES, false], + 'principals with contacts' => ['principals/users/user0', 'PROPFIND', self::CONTACTS_READ, true], + 'principals with calendar' => ['principals/users/user0', 'PROPFIND', self::CALENDAR_READ, true], + 'default calendar with contacts' => ['principals/users/user0', 'PROPPATCH', self::CONTACTS, false], + 'default calendar with calendar' => ['principals/users/user0', 'PROPPATCH', self::CALENDAR, true], + 'proxy write' => ['principals/users/user0/calendar-proxy-write', 'PROPPATCH', self::CALENDAR, false], + 'proxy read' => ['principals/users/user0/calendar-proxy-read/', 'PROPPATCH', self::CALENDAR, false], + 'proxy listing' => ['principals/users/user0/calendar-proxy-write', 'PROPFIND', self::CALENDAR_READ, true], + 'avatar' => ['avatars/user0/96.png', 'GET', [], true], + 'avatar listing' => ['avatars/user0', 'PROPFIND', [], false], + 'provisioning' => ['provisioning/apple-provisioning.mobileconfig', 'GET', self::CALENDAR, false], + 'public calendars' => ['public-calendars/abc', 'GET', self::CALENDAR, false], + 'remote calendars' => ['remote-calendars/abc', 'PROPFIND', self::CALENDAR, false], + 'app collection' => ['photos/user0/albums', 'PROPFIND', self::FILES, false], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataAllows')] + public function testAllows(string $path, string $method, array $scopes, bool $expected): void { + $this->assertSame($expected, $this->plugin($scopes)->allows($path, $method)); + } + + public function testLegacyWebdavIsFiles(): void { + $this->assertTrue($this->plugin(self::FILES, 'files/')->allows('Documents', 'PROPFIND')); + $this->assertFalse($this->plugin(self::CALENDAR, 'files/')->allows('Documents', 'PROPFIND')); + } + + private function initialize(TokenScopePlugin $plugin): Server { + $server = new Server(new SimpleCollection('root', [ + new SimpleCollection('calendars', [new SimpleCollection('user0', [new SimpleCollection('personal')])]), + new SimpleCollection('comments', [new SimpleCollection('files')]), + ])); + $server->setBaseUri('/'); + $plugin->initialize($server); + return $server; + } + + public function testRefusesRequest(): void { + $server = $this->initialize($this->plugin(self::FILES)); + + $this->expectException(Forbidden::class); + $server->emit('beforeMethod:PROPFIND', [new Request('PROPFIND', '/calendars/user0/personal'), $server->httpResponse]); + } + + public function testRefusesEncodedSegment(): void { + $server = $this->initialize($this->plugin(self::FILES)); + + $this->expectException(Forbidden::class); + $server->emit('beforeMethod:GET', [new Request('GET', '/%63alendars/user0/personal'), $server->httpResponse]); + } + + public function testRefusesCopyIntoCalendar(): void { + $server = $this->initialize($this->plugin(self::FILES)); + $request = new Request('COPY', '/files/user0/a.ics', ['Destination' => '/calendars/user0/personal/a.ics']); + + $this->expectException(Forbidden::class); + $server->emit('beforeMethod:COPY', [$request, $server->httpResponse]); + } + + public function testCopyOnlyReadsItsSource(): void { + $server = $this->initialize($this->plugin([...self::CALENDAR_READ, ...self::FILES])); + $request = new Request('COPY', '/calendars/user0/personal/a.ics', ['Destination' => '/files/user0/a.ics']); + + $this->assertTrue($server->emit('beforeMethod:COPY', [$request, $server->httpResponse])); + } + + public function testHidesNodesInMultistatus(): void { + $server = $this->initialize($this->plugin(self::CALENDAR)); + $server->emit('beforeMethod:REPORT', [new Request('REPORT', '/calendars/user0/personal'), $server->httpResponse]); + + $result = $server->getPropertiesForMultiplePaths(['calendars/user0/personal', 'comments/files'], ['{DAV:}resourcetype']); + + $this->assertSame(['calendars/user0/personal'], array_keys($result)); + } + + public function testUnscopedTokenPasses(): void { + $server = $this->initialize($this->plugin(null)); + + $this->assertTrue($server->emit('beforeMethod:PROPFIND', [new Request('PROPFIND', '/comments/files'), $server->httpResponse])); + $result = $server->getPropertiesForMultiplePaths(['calendars/user0/personal', 'comments/files'], ['{DAV:}resourcetype']); + $this->assertCount(2, $result); + } +} diff --git a/build/integration/features/auth.feature b/build/integration/features/auth.feature index cdcf0a720779b..ba507a7452de1 100644 --- a/build/integration/features/auth.feature +++ b/build/integration/features/auth.feature @@ -139,3 +139,43 @@ Feature: auth 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 Then the HTTP status code should be "403" + + Scenario: a files token reaches files over DAV + Given a new client token limited to "files:read files:write" is added + When requesting "/remote.php/dav/files/user0/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "207" + + Scenario: a files token reads no calendars over DAV + Given a new client token limited to "files:read files:write" is added + When requesting "/remote.php/dav/calendars/user0/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "403" + + Scenario: a files token reaches files over legacy WebDAV + Given a new client token limited to "files:read files:write" is added + When requesting "/remote.php/webdav/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "207" + + Scenario: a files token reads no calendars over legacy CalDAV + Given a new client token limited to "files:read files:write" is added + When requesting "/remote.php/caldav/calendars/user0/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "403" + + Scenario: a calendar token reaches calendars over DAV + Given a new client token limited to "calendar:read calendar:write" is added + When requesting "/remote.php/dav/calendars/user0/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "207" + + Scenario: a calendar token reads no files over DAV + Given a new client token limited to "calendar:read calendar:write" is added + When requesting "/remote.php/dav/files/user0/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "403" + + Scenario: a calendar token reads no files over legacy WebDAV + Given a new client token limited to "calendar:read calendar:write" is added + When requesting "/remote.php/webdav/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "403" + + Scenario: a calendar token cannot list the trash + Given a new client token limited to "calendar:read calendar:write" is added + When requesting "/remote.php/dav/trashbin/user0/trash/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "403" From c27652d5fed416404b8d25913c1548fafd08acb9 Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Fri, 16 Oct 2026 12:55:51 +0200 Subject: [PATCH 2/3] test(dav): use the app-owned scope names in the integration tests Signed-off-by: Peter Ringelmann --- build/integration/features/auth.feature | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/build/integration/features/auth.feature b/build/integration/features/auth.feature index ba507a7452de1..55c0606bb1baf 100644 --- a/build/integration/features/auth.feature +++ b/build/integration/features/auth.feature @@ -141,41 +141,41 @@ Feature: auth Then the HTTP status code should be "403" Scenario: a files token reaches files over DAV - Given a new client token limited to "files:read files:write" is added + Given a new client token limited to "files:files:read files:files:write" is added When requesting "/remote.php/dav/files/user0/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "207" Scenario: a files token reads no calendars over DAV - Given a new client token limited to "files:read files:write" is added + Given a new client token limited to "files:files:read files:files:write" is added When requesting "/remote.php/dav/calendars/user0/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "403" Scenario: a files token reaches files over legacy WebDAV - Given a new client token limited to "files:read files:write" is added + Given a new client token limited to "files:files:read files:files:write" is added When requesting "/remote.php/webdav/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "207" Scenario: a files token reads no calendars over legacy CalDAV - Given a new client token limited to "files:read files:write" is added + Given a new client token limited to "files:files:read files:files:write" is added When requesting "/remote.php/caldav/calendars/user0/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "403" Scenario: a calendar token reaches calendars over DAV - Given a new client token limited to "calendar:read calendar:write" is added + Given a new client token limited to "dav:calendar:read dav:calendar:write" is added When requesting "/remote.php/dav/calendars/user0/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "207" Scenario: a calendar token reads no files over DAV - Given a new client token limited to "calendar:read calendar:write" is added + Given a new client token limited to "dav:calendar:read dav:calendar:write" is added When requesting "/remote.php/dav/files/user0/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "403" Scenario: a calendar token reads no files over legacy WebDAV - Given a new client token limited to "calendar:read calendar:write" is added + Given a new client token limited to "dav:calendar:read dav:calendar:write" is added When requesting "/remote.php/webdav/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "403" Scenario: a calendar token cannot list the trash - Given a new client token limited to "calendar:read calendar:write" is added + Given a new client token limited to "dav:calendar:read dav:calendar:write" is added When requesting "/remote.php/dav/trashbin/user0/trash/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "403" From 98aca833e5fb454ee4b9d3340a9f9988860447d3 Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Fri, 23 Oct 2026 10:19:25 +0200 Subject: [PATCH 3/3] refactor(dav): let apps own the scopes of their DAV collections Signed-off-by: Peter Ringelmann --- apps/dav/appinfo/v1/caldav.php | 3 +- apps/dav/appinfo/v1/carddav.php | 3 +- apps/dav/appinfo/v1/webdav.php | 2 +- apps/dav/lib/AppInfo/PluginManager.php | 9 +++++ .../lib/Connector/Sabre/TokenScopePlugin.php | 18 +++++++--- apps/dav/lib/Server.php | 10 +++--- .../tests/unit/AppInfo/PluginManagerTest.php | 5 +++ .../Connector/Sabre/TokenScopePluginTest.php | 36 +++++++++++++------ apps/files/lib/TokenScopes.php | 2 ++ build/integration/features/auth.feature | 24 +++++++++++-- .../Authentication/Token/TokenScopes.php | 4 +-- .../Authentication/Token/TokenScopesTest.php | 2 ++ 12 files changed, 90 insertions(+), 28 deletions(-) diff --git a/apps/dav/appinfo/v1/caldav.php b/apps/dav/appinfo/v1/caldav.php index cf6f935da4939..565acfd62bf44 100644 --- a/apps/dav/appinfo/v1/caldav.php +++ b/apps/dav/appinfo/v1/caldav.php @@ -7,6 +7,7 @@ */ // Backends use OC\KnownUser\KnownUserService; +use OC\Lockdown\LockdownManager; use OCA\DAV\CalDAV\CalDavBackend; use OCA\DAV\CalDAV\CalendarRoot; use OCA\DAV\CalDAV\DefaultCalendarValidator; @@ -111,7 +112,7 @@ // Add plugins $server->addPlugin(new MaintenancePlugin(Server::get(IConfig::class), $davL10n)); $server->addPlugin(new \Sabre\DAV\Auth\Plugin($authBackend)); -$server->addPlugin(Server::get(TokenScopePlugin::class)); +$server->addPlugin(new TokenScopePlugin(Server::get(LockdownManager::class))); $server->addPlugin(new \Sabre\CalDAV\Plugin()); $server->addPlugin(new LegacyDAVACL()); diff --git a/apps/dav/appinfo/v1/carddav.php b/apps/dav/appinfo/v1/carddav.php index 36d2e03234de3..9aa6c9549293a 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -7,6 +7,7 @@ */ // Backends use OC\KnownUser\KnownUserService; +use OC\Lockdown\LockdownManager; use OCA\DAV\AppInfo\PluginManager; use OCA\DAV\CalDAV\Principal\Collection as PrincipalCollection; use OCA\DAV\CalDAV\Proxy\ProxyMapper; @@ -93,7 +94,7 @@ // Add plugins $server->addPlugin(new MaintenancePlugin(Server::get(IConfig::class), Server::get(IL10nFactory::class)->get('dav'))); $server->addPlugin(new \Sabre\DAV\Auth\Plugin($authBackend)); -$server->addPlugin(Server::get(TokenScopePlugin::class)); +$server->addPlugin(new TokenScopePlugin(Server::get(LockdownManager::class))); $server->addPlugin(new Plugin()); $server->addPlugin(new LegacyDAVACL()); diff --git a/apps/dav/appinfo/v1/webdav.php b/apps/dav/appinfo/v1/webdav.php index de90baf3a1db0..9ff7727f37af1 100644 --- a/apps/dav/appinfo/v1/webdav.php +++ b/apps/dav/appinfo/v1/webdav.php @@ -79,7 +79,7 @@ return Filesystem::getView(); }); // The legacy tree is the user's files root -$server->addPlugin(new TokenScopePlugin(Server::get(LockdownManager::class), 'files/')); +$server->addPlugin(new TokenScopePlugin(Server::get(LockdownManager::class), prefix: 'files/')); // allow setup of additional plugins $event = new SabrePluginEvent($server); diff --git a/apps/dav/lib/AppInfo/PluginManager.php b/apps/dav/lib/AppInfo/PluginManager.php index 9814e2138cad3..37d0356755873 100644 --- a/apps/dav/lib/AppInfo/PluginManager.php +++ b/apps/dav/lib/AppInfo/PluginManager.php @@ -56,6 +56,9 @@ class PluginManager { */ private $calendarPlugins = []; + /** @var array collection name => app id */ + private array $collectionApps = []; + /** @var bool */ private $populated = false; @@ -91,6 +94,11 @@ public function getAppCollections() { return $this->collections; } + public function getCollectionApp(string $name): ?string { + $this->populate(); + return $this->collectionApps[$name] ?? null; + } + /** * @return IAddressBookProvider[] */ @@ -134,6 +142,7 @@ private function populate(): void { $collections = $this->loadSabreCollectionsFromInfoXml($this->extractCollectionList($info)); foreach ($collections as $collection) { $this->collections[] = $collection; + $this->collectionApps[$collection->getName()] = $app; } $addresbookPlugins = $this->loadSabreAddressBookPluginsFromInfoXml($this->extractAddressBookPluginList($info)); diff --git a/apps/dav/lib/Connector/Sabre/TokenScopePlugin.php b/apps/dav/lib/Connector/Sabre/TokenScopePlugin.php index 22e70ba87365a..0d9a758c63987 100644 --- a/apps/dav/lib/Connector/Sabre/TokenScopePlugin.php +++ b/apps/dav/lib/Connector/Sabre/TokenScopePlugin.php @@ -10,9 +10,9 @@ namespace OCA\DAV\Connector\Sabre; use OC\Lockdown\LockdownManager; +use OCA\DAV\AppInfo\PluginManager; use OCA\DAV\CalDAV\Plugin as CalDAVPlugin; use OCA\DAV\TokenScopes; -use OCA\Files\TokenScopes as FilesTokenScopes; use Sabre\CardDAV\Plugin as CardDAVPlugin; use Sabre\DAV\Exception\Forbidden; use Sabre\DAV\INode; @@ -27,16 +27,17 @@ class TokenScopePlugin extends ServerPlugin { private const READ_METHODS = ['GET', 'HEAD', 'OPTIONS', 'PROPFIND', 'REPORT', 'SEARCH']; - /** Collections for files and their tags, plus bulk upload. Writes are guarded by the mount permissions */ - private const FILES_COLLECTIONS = ['files', 'uploads', 'bulk', 'trashbin', 'versions', 'groupfolders', 'systemtags', 'systemtags-relations', 'systemtags-assigned']; + private const FILES_COLLECTIONS = ['files', 'uploads', 'bulk']; private Server $server; /** + * @param PluginManager|null $pluginManager null for servers without app collections * @param string $prefix prepended to every path, for servers whose tree is a single collection */ public function __construct( private LockdownManager $lockdownManager, + private ?PluginManager $pluginManager = null, private string $prefix = '', ) { } @@ -85,10 +86,19 @@ public function allows(string $path, string $method): bool { ? !in_array(end($segments), ['calendar-proxy-read', 'calendar-proxy-write'], true) && $this->lockdownManager->hasScope(TokenScopes::CALENDAR_WRITE) : $this->lockdownManager->hasScope(TokenScopes::CALENDAR_READ) || $this->lockdownManager->hasScope(TokenScopes::CONTACTS_READ), 'avatars' => $method === 'GET' || $method === 'HEAD', - default => in_array($segments[0], self::FILES_COLLECTIONS, true) && $this->lockdownManager->hasScope(FilesTokenScopes::FILES_READ), + default => $this->allowsAppCollection($segments[0], $write), }; } + /** + * Collections need their app's own DAV scopes. Apps register theirs in info.xml + */ + private function allowsAppCollection(string $name, bool $write): bool { + $app = in_array($name, self::FILES_COLLECTIONS, true) ? 'files' : $this->pluginManager?->getCollectionApp($name); + return $app !== null + && ($this->lockdownManager->hasScope($app) || $this->lockdownManager->hasScope($app . ':dav:' . ($write ? 'write' : 'read'))); + } + /** * @param list $segments */ diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 66f7a3e284a76..8e964c97fdeee 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -9,6 +9,7 @@ namespace OCA\DAV; use OC\Files\Filesystem; +use OC\Lockdown\LockdownManager; use OCA\DAV\AppInfo\PluginManager; use OCA\DAV\BulkUpload\BulkUploadPlugin; use OCA\DAV\CalDAV\BirthdayCalendar\EnablePlugin; @@ -180,7 +181,8 @@ public function __construct( $authPlugin->addBackend($bearerAuthBackend); // because we are throwing exceptions this plugin has to be the last one $authPlugin->addBackend($authBackend); - $this->server->addPlugin(\OCP\Server::get(TokenScopePlugin::class)); + $pluginManager = new PluginManager(\OC::$server, \OCP\Server::get(IAppManager::class)); + $this->server->addPlugin(new TokenScopePlugin(\OCP\Server::get(LockdownManager::class), $pluginManager)); // debugging if ($debugEnabled) { @@ -288,7 +290,7 @@ public function __construct( $this->server->addPlugin(new SearchPlugin($lazySearchBackend)); // wait with registering these until auth is handled and the filesystem is setup - $this->server->on('beforeMethod:*', function () use ($root, $lazySearchBackend, $logger): void { + $this->server->on('beforeMethod:*', function () use ($root, $lazySearchBackend, $logger, $pluginManager): void { // custom properties plugin must be the last one $userSession = \OCP\Server::get(IUserSession::class); $user = $userSession->getUser(); @@ -413,10 +415,6 @@ function () { } // register plugins from apps - $pluginManager = new PluginManager( - \OC::$server, - \OCP\Server::get(IAppManager::class) - ); foreach ($pluginManager->getAppPlugins() as $appPlugin) { $this->server->addPlugin($appPlugin); } diff --git a/apps/dav/tests/unit/AppInfo/PluginManagerTest.php b/apps/dav/tests/unit/AppInfo/PluginManagerTest.php index 23cc408edb26e..f8a88fddc4f24 100644 --- a/apps/dav/tests/unit/AppInfo/PluginManagerTest.php +++ b/apps/dav/tests/unit/AppInfo/PluginManagerTest.php @@ -88,6 +88,8 @@ public function test(): void { $dummyCollection1 = $this->createMock(Collection::class); $dummyCollection2 = $this->createMock(Collection::class); $dummy2Collection1 = $this->createMock(Collection::class); + $dummyCollection1->method('getName')->willReturn('one'); + $dummy2Collection1->method('getName')->willReturn('two'); $server->method('get') ->willReturnMap([ @@ -123,5 +125,8 @@ public function test(): void { $this->assertEquals($expectedPlugins, $pluginManager->getAppPlugins()); $this->assertEquals($expectedCalendarPlugins, $pluginManager->getCalendarPlugins()); $this->assertEquals($expectedCollections, $pluginManager->getAppCollections()); + $this->assertSame('adavapp', $pluginManager->getCollectionApp('one')); + $this->assertSame('adavapp2', $pluginManager->getCollectionApp('two')); + $this->assertNull($pluginManager->getCollectionApp('files')); } } diff --git a/apps/dav/tests/unit/Connector/Sabre/TokenScopePluginTest.php b/apps/dav/tests/unit/Connector/Sabre/TokenScopePluginTest.php index 389264a056eaa..9f8856ba1a3f0 100644 --- a/apps/dav/tests/unit/Connector/Sabre/TokenScopePluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/TokenScopePluginTest.php @@ -12,6 +12,7 @@ use OC\Authentication\Token\PublicKeyToken; use OC\Authentication\Token\TokenScopes as CoreTokenScopes; use OC\Lockdown\LockdownManager; +use OCA\DAV\AppInfo\PluginManager; use OCA\DAV\Connector\Sabre\TokenScopePlugin; use OCA\DAV\TokenScopes; use OCA\Files\TokenScopes as FilesTokenScopes; @@ -23,7 +24,7 @@ use Test\TestCase; class TokenScopePluginTest extends TestCase { - private const FILES = [FilesTokenScopes::FILES_READ, FilesTokenScopes::FILES_WRITE]; + private const FILES = [FilesTokenScopes::FILES_READ, FilesTokenScopes::FILES_WRITE, FilesTokenScopes::DAV_READ, FilesTokenScopes::DAV_WRITE]; private const CALENDAR = [TokenScopes::CALENDAR_READ, TokenScopes::CALENDAR_WRITE]; private const CALENDAR_READ = [TokenScopes::CALENDAR_READ]; private const CONTACTS = [TokenScopes::CONTACTS_READ, TokenScopes::CONTACTS_WRITE]; @@ -37,7 +38,14 @@ private function plugin(?array $scopes, string $prefix = ''): TokenScopePlugin { $token = new PublicKeyToken(); $token->setScope($scopes === null ? [] : [CoreTokenScopes::KEY => $scopes]); $lockdownManager->setToken($token); - return new TokenScopePlugin($lockdownManager, $prefix); + $pluginManager = $this->createMock(PluginManager::class); + $pluginManager->method('getCollectionApp')->willReturnMap([ + ['trashbin', 'files_trashbin'], + ['versions', 'files_versions'], + ['groupfolders', 'groupfolders'], + ['photos', 'photos'], + ]); + return new TokenScopePlugin($lockdownManager, $pluginManager, $prefix); } public static function dataAllows(): array { @@ -45,19 +53,23 @@ public static function dataAllows(): array { 'root, any key' => ['', 'PROPFIND', [], true], 'root write' => ['', 'PROPPATCH', self::FILES, false], 'files' => ['files/user0/a.txt', 'GET', self::FILES, true], - 'files write, mask guards it' => ['files/user0/a.txt', 'PUT', self::FILES, true], + 'files write' => ['files/user0/a.txt', 'PUT', self::FILES, true], + 'files write without dav write' => ['files/user0/a.txt', 'PUT', [FilesTokenScopes::DAV_READ], false], + 'files without dav' => ['files/user0/a.txt', 'GET', [FilesTokenScopes::FILES_READ, FilesTokenScopes::FILES_WRITE], false], 'files without files' => ['files/user0', 'PROPFIND', self::CALENDAR, false], 'uploads' => ['uploads/user0/1', 'MKCOL', self::FILES, true], 'bulk upload' => ['bulk', 'POST', self::FILES, true], 'bulk upload without files' => ['bulk', 'POST', self::CALENDAR, false], - 'groupfolder trash without files' => ['trashbin/user0/trash', 'PROPFIND', self::CALENDAR, false], - 'groupfolders' => ['groupfolders/user0', 'PROPFIND', self::FILES, true], - 'versions' => ['versions/user0/versions/1', 'PROPFIND', self::FILES, true], - 'tag definitions read' => ['systemtags/1', 'PROPFIND', self::FILES, true], - 'tag definitions write' => ['systemtags/1', 'PROPPATCH', self::FILES, true], - 'tag definitions without files' => ['systemtags/1', 'PROPFIND', self::CALENDAR, false], - 'tag assignment' => ['systemtags-relations/files/1/2', 'PUT', self::FILES, true], - 'assigned tags' => ['systemtags-assigned/files', 'PROPFIND', self::FILES, true], + 'trash with files only' => ['trashbin/user0/trash', 'PROPFIND', self::FILES, false], + 'trash' => ['trashbin/user0/trash', 'PROPFIND', [...self::FILES, 'files_trashbin:dav:read'], true], + 'trash restore without write' => ['trashbin/user0/trash/a.txt', 'MOVE', [...self::FILES, 'files_trashbin:dav:read'], false], + 'versions' => ['versions/user0/versions/1', 'PROPFIND', [...self::FILES, 'files_versions:dav:read'], true], + 'groupfolders with files only' => ['groupfolders/user0', 'PROPFIND', self::FILES, false], + 'groupfolders, whole app' => ['groupfolders/user0', 'PROPFIND', ['groupfolders'], true], + 'another app\'s scope' => ['groupfolders/user0', 'PROPFIND', ['files_trashbin:dav:read'], false], + 'tag definitions' => ['systemtags/1', 'PROPFIND', self::FILES, false], + 'tag assignment' => ['systemtags-relations/files/1/2', 'PUT', self::FILES, false], + 'assigned tags' => ['systemtags-assigned/files', 'PROPFIND', self::FILES, false], 'comments' => ['comments/files/1', 'PROPFIND', self::FILES, false], 'calendar read' => ['calendars/user0/personal', 'REPORT', self::CALENDAR_READ, true], 'calendar write without write' => ['calendars/user0/personal/a.ics', 'PUT', self::CALENDAR_READ, false], @@ -86,6 +98,8 @@ public static function dataAllows(): array { 'public calendars' => ['public-calendars/abc', 'GET', self::CALENDAR, false], 'remote calendars' => ['remote-calendars/abc', 'PROPFIND', self::CALENDAR, false], 'app collection' => ['photos/user0/albums', 'PROPFIND', self::FILES, false], + 'app collection with its scope' => ['photos/user0/albums', 'PROPFIND', ['photos:dav:read'], true], + 'unknown collection' => ['nope/user0', 'PROPFIND', ['nope'], false], ]; } diff --git a/apps/files/lib/TokenScopes.php b/apps/files/lib/TokenScopes.php index 47b1174d37ba5..c7f40d2308778 100644 --- a/apps/files/lib/TokenScopes.php +++ b/apps/files/lib/TokenScopes.php @@ -15,4 +15,6 @@ class TokenScopes { public const FILES_READ = 'files:files:read'; public const FILES_WRITE = 'files:files:write'; + public const DAV_READ = 'files:dav:read'; + public const DAV_WRITE = 'files:dav:write'; } diff --git a/build/integration/features/auth.feature b/build/integration/features/auth.feature index 55c0606bb1baf..c366385086a15 100644 --- a/build/integration/features/auth.feature +++ b/build/integration/features/auth.feature @@ -141,7 +141,7 @@ Feature: auth Then the HTTP status code should be "403" Scenario: a files token reaches files over DAV - Given a new client token limited to "files:files:read files:files:write" is added + Given a new client token limited to "files:files:read files:files:write files:dav:read files:dav:write" is added When requesting "/remote.php/dav/files/user0/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "207" @@ -151,7 +151,7 @@ Feature: auth Then the HTTP status code should be "403" Scenario: a files token reaches files over legacy WebDAV - Given a new client token limited to "files:files:read files:files:write" is added + Given a new client token limited to "files:files:read files:files:write files:dav:read files:dav:write" is added When requesting "/remote.php/webdav/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "207" @@ -160,6 +160,11 @@ Feature: auth When requesting "/remote.php/caldav/calendars/user0/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "403" + Scenario: a calendar token reaches calendars over legacy CalDAV + Given a new client token limited to "dav:calendar:read dav:calendar:write" is added + When requesting "/remote.php/caldav/calendars/user0/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "207" + Scenario: a calendar token reaches calendars over DAV Given a new client token limited to "dav:calendar:read dav:calendar:write" is added When requesting "/remote.php/dav/calendars/user0/" with "PROPFIND" using scoped basic token auth @@ -179,3 +184,18 @@ Feature: auth Given a new client token limited to "dav:calendar:read dav:calendar:write" is added When requesting "/remote.php/dav/trashbin/user0/trash/" with "PROPFIND" using scoped basic token auth Then the HTTP status code should be "403" + + Scenario: a files token without DAV reads no files over DAV + Given a new client token limited to "files:files:read files:files:write" is added + When requesting "/remote.php/dav/files/user0/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "403" + + Scenario: a files token reads no trash without the trash scope + Given a new client token limited to "files:files:read files:files:write files:dav:read files:dav:write" is added + When requesting "/remote.php/dav/trashbin/user0/trash/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "403" + + Scenario: a files token with the trash scope reads the trash + Given a new client token limited to "files:files:read files:files:write files:dav:read files:dav:write files_trashbin:dav:read" is added + When requesting "/remote.php/dav/trashbin/user0/trash/" with "PROPFIND" using scoped basic token auth + Then the HTTP status code should be "207" diff --git a/lib/private/Authentication/Token/TokenScopes.php b/lib/private/Authentication/Token/TokenScopes.php index dc363be50e208..460ece3f60f99 100644 --- a/lib/private/Authentication/Token/TokenScopes.php +++ b/lib/private/Authentication/Token/TokenScopes.php @@ -86,8 +86,8 @@ private function validate(array $scopes): array { throw new \InvalidArgumentException('Invalid scope'); } } - // Matched by prefix: files_sharing can be disabled, and its classes don't load then - $files = array_filter($scopes, static fn (string $scope) => str_starts_with($scope, 'files:') || str_starts_with($scope, 'files_sharing:')); + // 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)); // Files endpoints look nodes up by ID or path, so every files scope reads file data if ($files !== [] && !in_array(FilesTokenScopes::FILES_READ, $files, true)) { throw new \InvalidArgumentException('Files scopes require ' . FilesTokenScopes::FILES_READ); diff --git a/tests/lib/Authentication/Token/TokenScopesTest.php b/tests/lib/Authentication/Token/TokenScopesTest.php index f1ed8e3f167a2..d14f059e47239 100644 --- a/tests/lib/Authentication/Token/TokenScopesTest.php +++ b/tests/lib/Authentication/Token/TokenScopesTest.php @@ -67,6 +67,8 @@ public static function invalidUpdates(): array { 'files share without read' => [[TokenScopes::KEY => [SharingTokenScopes::SHARES_WRITE]]], 'files write and share without read' => [[TokenScopes::KEY => [FilesTokenScopes::FILES_WRITE, SharingTokenScopes::SHARES_WRITE]]], 'read-only files' => [[TokenScopes::KEY => [FilesTokenScopes::FILES_READ]]], + 'trash without files' => [[TokenScopes::KEY => ['files_trashbin:dav:read']]], + 'versions without files' => [[TokenScopes::KEY => ['files_versions:dav:read']]], 'read and share without write' => [[TokenScopes::KEY => [FilesTokenScopes::FILES_READ, SharingTokenScopes::SHARES_WRITE]]], 'not an array' => [[TokenScopes::KEY => FilesTokenScopes::FILES_READ]], 'nested array entry' => [[TokenScopes::KEY => ['dav:calendar:read', ['x']]]],