Reflect public shares in isPublic flag and fix permission check

apps/dav/appinfo/v1/publicwebdav.php

@@ -68,47 +68,55 @@
 $linkCheckPlugin = new PublicLinkCheckPlugin();
 $filesDropPlugin = new FilesDropPlugin();
 
-$server = $serverFactory->createServer(false, $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);
-	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]);
+$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);
+		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);

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;
 		}

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;
@@ -70,15 +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);
-		// Fire up server
-		if ($isPublicShare) {
-			$rootCollection = new SimpleCollection('root');
-			$tree = new CachingTree($rootCollection);
-		} else {
-			$rootCollection = null;
-			$tree = new ObjectTree();
-		}
+		[$tree, $rootCollection] = $this->getTree($useCollection);
 		$server = new Server($tree);
 		// Set URL explicitly due to reverse-proxy situations
 		$server->httpRequest->setUrl($requestUri);
@@ -127,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, $isPublicShare, $rootCollection, $debugEnabled): void {
+		$server->on('beforeMethod:*', function () use ($server,
+			$tree, $viewCallBack, $isPublicShare, $rootCollection, $debugEnabled): void {
 			// ensure the skeleton is copied
 			$userFolder = \OC::$server->getUserFolder();
 
@@ -147,36 +146,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/<share token>
-				$rootCollection->addChild(new RootCollection(
-					$root,
-					$userPrincipalBackend,
-					'principals/shares',
-				));
-
-				// Mount the upload collection at /public.php/dav/uploads/<share token>
-				$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);
@@ -191,7 +162,7 @@ public function createServer(
 					$this->userSession,
 					\OCP\Server::get(IFilenameValidator::class),
 					\OCP\Server::get(IAccountManager::class),
-					false,
+					$isPublicShare,
 					!$debugEnabled
 				)
 			);
@@ -252,4 +223,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/<share token>
+		$rootCollection->addChild(
+			new RootCollection(
+				$root,
+				$userPrincipalBackend,
+				'principals/shares',
+			)
+		);
+
+		// Mount the upload collection at /public.php/dav/uploads/<share token>
+		$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),
+			)
+		);
+	}
 }

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')

build/psalm-baseline.xml

@@ -736,7 +736,6 @@
   </file>
   <file src="apps/dav/lib/Connector/Sabre/ServerFactory.php">
     <DeprecatedMethod>
-      <code><![CDATA[getL10NFactory]]></code>
       <code><![CDATA[getUserFolder]]></code>
     </DeprecatedMethod>
     <InternalMethod>