From c5ad20d92546c4e69f07e9c2e85eae2d156fe023 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Tue, 16 Sep 2025 16:58:09 +0200 Subject: [PATCH 1/4] refactor: extract tree initialization logic Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- .../dav/lib/Connector/Sabre/ServerFactory.php | 101 +++++++++++------- build/psalm-baseline.xml | 1 - 2 files changed, 62 insertions(+), 40 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index d01ea6b223e94..c86d22956be4e 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -37,6 +37,7 @@ use OCP\ITagManager; use OCP\IUserManager; use OCP\IUserSession; +use OCP\L10N\IFactory; use OCP\SabrePluginEvent; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; @@ -71,14 +72,7 @@ public function createServer( callable $viewCallBack, ): Server { $debugEnabled = $this->config->getSystemValue('debug', false); - // Fire up server - if ($isPublicShare) { - $rootCollection = new SimpleCollection('root'); - $tree = new CachingTree($rootCollection); - } else { - $rootCollection = null; - $tree = new ObjectTree(); - } + [$tree, $rootCollection] = $this->getTree($isPublicShare); $server = new Server($tree); // Set URL explicitly due to reverse-proxy situations $server->httpRequest->setUrl($requestUri); @@ -128,7 +122,7 @@ public function createServer( // wait with registering these until auth is handled and the filesystem is setup $server->on('beforeMethod:*', function () use ($server, $tree, - $viewCallBack, $isPublicShare, $rootCollection, $debugEnabled): void { + $viewCallBack, $rootCollection, $debugEnabled): void { // ensure the skeleton is copied $userFolder = \OC::$server->getUserFolder(); @@ -147,36 +141,8 @@ public function createServer( $root = new File($view, $rootInfo); } - if ($isPublicShare) { - $userPrincipalBackend = new Principal( - \OCP\Server::get(IUserManager::class), - \OCP\Server::get(IGroupManager::class), - \OCP\Server::get(IAccountManager::class), - \OCP\Server::get(\OCP\Share\IManager::class), - \OCP\Server::get(IUserSession::class), - \OCP\Server::get(IAppManager::class), - \OCP\Server::get(ProxyMapper::class), - \OCP\Server::get(KnownUserService::class), - \OCP\Server::get(IConfig::class), - \OC::$server->getL10NFactory(), - ); - - // Mount the share collection at /public.php/dav/shares/ - $rootCollection->addChild(new RootCollection( - $root, - $userPrincipalBackend, - 'principals/shares', - )); - - // Mount the upload collection at /public.php/dav/uploads/ - $rootCollection->addChild(new \OCA\DAV\Upload\RootCollection( - $userPrincipalBackend, - 'principals/shares', - \OCP\Server::get(CleanupService::class), - \OCP\Server::get(IRootFolder::class), - \OCP\Server::get(IUserSession::class), - \OCP\Server::get(\OCP\Share\IManager::class), - )); + if ($rootCollection !== null) { + $this->initRootCollection($rootCollection, $root); } else { /** @var ObjectTree $tree */ $tree->init($root, $view, $this->mountManager); @@ -252,4 +218,61 @@ public function createServer( }, 30); // priority 30: after auth (10) and acl(20), before lock(50) and handling the request return $server; } + + /** + * Returns a Tree object and, if $useCollection is true, the collection used + * as root. + * + * @param bool $useCollection Whether to use a collection or the legacy + * ObjectTree, which doesn't use collections. + * @return array{0: CachingTree|ObjectTree, 1: SimpleCollection|null} + */ + public function getTree(bool $useCollection): array { + if ($useCollection) { + $rootCollection = new SimpleCollection('root'); + $tree = new CachingTree($rootCollection); + return [$tree, $rootCollection]; + } + + return [new ObjectTree(), null]; + } + + /** + * Adds the user's principal backend to $rootCollection. + */ + private function initRootCollection(SimpleCollection $rootCollection, Directory|File $root): void { + $userPrincipalBackend = new Principal( + \OCP\Server::get(IUserManager::class), + \OCP\Server::get(IGroupManager::class), + \OCP\Server::get(IAccountManager::class), + \OCP\Server::get(\OCP\Share\IManager::class), + \OCP\Server::get(IUserSession::class), + \OCP\Server::get(IAppManager::class), + \OCP\Server::get(ProxyMapper::class), + \OCP\Server::get(KnownUserService::class), + \OCP\Server::get(IConfig::class), + \OCP\Server::get(IFactory::class), + ); + + // Mount the share collection at /public.php/dav/files/ + $rootCollection->addChild( + new RootCollection( + $root, + $userPrincipalBackend, + 'principals/shares', + ) + ); + + // Mount the upload collection at /public.php/dav/uploads/ + $rootCollection->addChild( + new \OCA\DAV\Upload\RootCollection( + $userPrincipalBackend, + 'principals/shares', + \OCP\Server::get(CleanupService::class), + \OCP\Server::get(IRootFolder::class), + \OCP\Server::get(IUserSession::class), + \OCP\Server::get(\OCP\Share\IManager::class), + ) + ); + } } diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index d8e42dff4da25..8fbc32c4fecde 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -736,7 +736,6 @@ - From 38f8423cd6f9f4e45f9c094268809e16d277de88 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Tue, 16 Sep 2025 17:08:23 +0200 Subject: [PATCH 2/4] fix: isPublicShare =true when share is public The isPublicShare was set to false in one instance where it should have been true. Flipping the value to true, would break the functionality for PROPFIND /public.php/webdav/ which returns properties of files in a share identified by the username being the share token. Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- apps/dav/appinfo/v1/publicwebdav.php | 11 ++++++++++- apps/dav/lib/Connector/Sabre/ServerFactory.php | 13 +++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/apps/dav/appinfo/v1/publicwebdav.php b/apps/dav/appinfo/v1/publicwebdav.php index af49ca5462c38..8bc170f2630e4 100644 --- a/apps/dav/appinfo/v1/publicwebdav.php +++ b/apps/dav/appinfo/v1/publicwebdav.php @@ -68,7 +68,16 @@ $linkCheckPlugin = new PublicLinkCheckPlugin(); $filesDropPlugin = new FilesDropPlugin(); -$server = $serverFactory->createServer(false, $baseuri, $requestUri, $authPlugin, function (\Sabre\DAV\Server $server) use ($authBackend, $linkCheckPlugin, $filesDropPlugin) { +$server = $serverFactory->createServer( + true, + $baseuri, + $requestUri, + $authPlugin, + function (\Sabre\DAV\Server $server) use ( + $authBackend, + $linkCheckPlugin, + $filesDropPlugin + ) { $isAjax = in_array('XMLHttpRequest', explode(',', $_SERVER['HTTP_X_REQUESTED_WITH'] ?? '')); /** @var FederatedShareProvider $shareProvider */ $federatedShareProvider = Server::get(FederatedShareProvider::class); diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index c86d22956be4e..7a8f6fd032a60 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -71,8 +71,13 @@ public function createServer( Plugin $authPlugin, callable $viewCallBack, ): Server { + // /public.php/webdav/ shows the files in the share in the root itself + // and not under /public.php/webdav/files/{token} so we should keep + // compatibility for that. + $needsSharesInRoot = $baseUri === '/public.php/webdav/'; + $useCollection = $isPublicShare && !$needsSharesInRoot; $debugEnabled = $this->config->getSystemValue('debug', false); - [$tree, $rootCollection] = $this->getTree($isPublicShare); + [$tree, $rootCollection] = $this->getTree($useCollection); $server = new Server($tree); // Set URL explicitly due to reverse-proxy situations $server->httpRequest->setUrl($requestUri); @@ -121,8 +126,8 @@ public function createServer( } // wait with registering these until auth is handled and the filesystem is setup - $server->on('beforeMethod:*', function () use ($server, $tree, - $viewCallBack, $rootCollection, $debugEnabled): void { + $server->on('beforeMethod:*', function () use ($server, + $tree, $viewCallBack, $isPublicShare, $rootCollection, $debugEnabled): void { // ensure the skeleton is copied $userFolder = \OC::$server->getUserFolder(); @@ -157,7 +162,7 @@ public function createServer( $this->userSession, \OCP\Server::get(IFilenameValidator::class), \OCP\Server::get(IAccountManager::class), - false, + $isPublicShare, !$debugEnabled ) ); From 4ac0fcf02eb520cfb86ff4c3e418f82f00f73774 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Tue, 16 Sep 2025 17:10:54 +0200 Subject: [PATCH 3/4] fix: check instance of storage using helper function instanceof cannot be used to check the instance of a storage, doing so breaks the check in certain cases. In this case, enabling the `files_accesscontrol` app breaks the check. Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- apps/dav/lib/Connector/Sabre/Directory.php | 2 +- apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index a5333fbc9d908..87685c5bb2aeb 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -181,7 +181,7 @@ public function getChild($name, $info = null, ?IRequest $request = null, ?IL10N // If we are, then only PUT and MKCOL are allowed (see plugin) // so we are safe to return the directory without a risk of // leaking files and folders structure. - if ($storage instanceof PublicShareWrapper) { + if ($storage->instanceOfStorage(PublicShareWrapper::class)) { $share = $storage->getShare(); $allowDirectory = ($share->getPermissions() & Constants::PERMISSION_READ) !== Constants::PERMISSION_READ; } diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index 9ebe921d5b2dc..1da7126987b94 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -21,6 +21,7 @@ use OCP\Files\ForbiddenException; use OCP\Files\InvalidPathException; use OCP\Files\Mount\IMountPoint; +use OCP\Files\Storage\IStorage; use OCP\Files\StorageNotAvailableException; use PHPUnit\Framework\MockObject\MockObject; use Test\Traits\UserTrait; @@ -61,12 +62,16 @@ class DirectoryTest extends \Test\TestCase { private View&MockObject $view; private FileInfo&MockObject $info; + private IStorage&MockObject $storage; protected function setUp(): void { parent::setUp(); $this->view = $this->createMock(View::class); $this->info = $this->createMock(FileInfo::class); + $this->storage = $this->createMock(IStorage::class); + $this->info->method('getStorage') + ->willReturn($this->storage); $this->info->method('isReadable') ->willReturn(true); $this->info->method('getType') From 631318f86fd512a0229de1f24eabd1c1504cc557 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Wed, 15 Oct 2025 14:58:28 +0200 Subject: [PATCH 4/4] style: apply cs-fixer to publicwebdav.php Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- apps/dav/appinfo/v1/publicwebdav.php | 77 +++++++++---------- .../dav/lib/Connector/Sabre/ServerFactory.php | 2 +- 2 files changed, 39 insertions(+), 40 deletions(-) diff --git a/apps/dav/appinfo/v1/publicwebdav.php b/apps/dav/appinfo/v1/publicwebdav.php index 8bc170f2630e4..bc917e5b046d8 100644 --- a/apps/dav/appinfo/v1/publicwebdav.php +++ b/apps/dav/appinfo/v1/publicwebdav.php @@ -78,46 +78,45 @@ function (\Sabre\DAV\Server $server) use ( $linkCheckPlugin, $filesDropPlugin ) { - $isAjax = in_array('XMLHttpRequest', explode(',', $_SERVER['HTTP_X_REQUESTED_WITH'] ?? '')); - /** @var FederatedShareProvider $shareProvider */ - $federatedShareProvider = Server::get(FederatedShareProvider::class); - if ($federatedShareProvider->isOutgoingServer2serverShareEnabled() === false && !$isAjax) { - // this is what is thrown when trying to access a non-existing share - throw new \Sabre\DAV\Exception\NotAuthenticated(); - } - - $share = $authBackend->getShare(); - $owner = $share->getShareOwner(); - $isReadable = $share->getPermissions() & Constants::PERMISSION_READ; - $fileId = $share->getNodeId(); - - // FIXME: should not add storage wrappers outside of preSetup, need to find a better way - $previousLog = Filesystem::logWarningWhenAddingStorageWrapper(false); - Filesystem::addStorageWrapper('sharePermissions', function ($mountPoint, $storage) use ($share) { - return new PermissionsMask(['storage' => $storage, 'mask' => $share->getPermissions() | Constants::PERMISSION_SHARE]); + $isAjax = in_array('XMLHttpRequest', explode(',', $_SERVER['HTTP_X_REQUESTED_WITH'] ?? '')); + /** @var FederatedShareProvider $shareProvider */ + $federatedShareProvider = Server::get(FederatedShareProvider::class); + if ($federatedShareProvider->isOutgoingServer2serverShareEnabled() === false && !$isAjax) { + // this is what is thrown when trying to access a non-existing share + throw new \Sabre\DAV\Exception\NotAuthenticated(); + } + + $share = $authBackend->getShare(); + $owner = $share->getShareOwner(); + $isReadable = $share->getPermissions() & Constants::PERMISSION_READ; + $fileId = $share->getNodeId(); + + // FIXME: should not add storage wrappers outside of preSetup, need to find a better way + $previousLog = Filesystem::logWarningWhenAddingStorageWrapper(false); + Filesystem::addStorageWrapper('sharePermissions', function ($mountPoint, $storage) use ($share) { + return new PermissionsMask(['storage' => $storage, 'mask' => $share->getPermissions() | Constants::PERMISSION_SHARE]); + }); + Filesystem::addStorageWrapper('shareOwner', function ($mountPoint, $storage) use ($share) { + return new PublicOwnerWrapper(['storage' => $storage, 'owner' => $share->getShareOwner()]); + }); + Filesystem::logWarningWhenAddingStorageWrapper($previousLog); + + $rootFolder = Server::get(IRootFolder::class); + $userFolder = $rootFolder->getUserFolder($owner); + $node = $userFolder->getFirstNodeById($fileId); + if (!$node) { + throw new \Sabre\DAV\Exception\NotFound(); + } + $linkCheckPlugin->setFileInfo($node); + + // If not readable (files_drop) enable the filesdrop plugin + if (!$isReadable) { + $filesDropPlugin->enable(); + } + $filesDropPlugin->setShare($share); + + return new View($node->getPath()); }); - Filesystem::addStorageWrapper('shareOwner', function ($mountPoint, $storage) use ($share) { - return new PublicOwnerWrapper(['storage' => $storage, 'owner' => $share->getShareOwner()]); - }); - Filesystem::logWarningWhenAddingStorageWrapper($previousLog); - - $rootFolder = Server::get(IRootFolder::class); - $userFolder = $rootFolder->getUserFolder($owner); - $node = $userFolder->getFirstNodeById($fileId); - if (!$node) { - throw new \Sabre\DAV\Exception\NotFound(); - } - $linkCheckPlugin->setFileInfo($node); - - // If not readable (files_drop) enable the filesdrop plugin - if (!$isReadable) { - $filesDropPlugin->enable(); - } - $filesDropPlugin->setShare($share); - - $view = new View($node->getPath()); - return $view; -}); $server->addPlugin($linkCheckPlugin); $server->addPlugin($filesDropPlugin); diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index 7a8f6fd032a60..19dd5584c5167 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -229,7 +229,7 @@ public function createServer( * as root. * * @param bool $useCollection Whether to use a collection or the legacy - * ObjectTree, which doesn't use collections. + * ObjectTree, which doesn't use collections. * @return array{0: CachingTree|ObjectTree, 1: SimpleCollection|null} */ public function getTree(bool $useCollection): array {