From 7ede3044bf6dd8a1979af0a4dc4911594e0fb64a 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 baca642cee970..7378477d48c7f 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -761,7 +761,6 @@ - From 79e48941e0a26dc6b8eae20ba4e22ec002a8e720 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 6d1344779ae0d342c8bf1da1d95e248999d98185 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 fe09c3f423fbb..c2ff6256c70e0 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 421ee1bdc1276..bd980f0c9ef1e 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; @@ -63,12 +64,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 5994423e2a27c94e5e6f6fb0f9d52927f18c03a7 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 {