Merge pull request #46987 from nextcloud/fix/dav-public

fix(dav): Ensure share properties are also set on public remote endpoint

apps/dav/appinfo/v2/publicremote.php

@@ -10,6 +10,7 @@ use OC\Files\Filesystem;
 use OC\Files\Storage\Wrapper\PermissionsMask;
 use OC\Files\View;
 use OCA\DAV\Storage\PublicOwnerWrapper;
+use OCA\DAV\Storage\PublicShareWrapper;
 use OCA\FederatedFileSharing\FederatedShareProvider;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\Mount\IMountManager;
@@ -98,6 +99,12 @@ $server = $serverFactory->createServer($baseuri, $requestUri, $authPlugin, funct
 		return new PublicOwnerWrapper(['storage' => $storage, 'owner' => $share->getShareOwner()]);
 	});
 
+	// Ensure that also private shares have the `getShare` method
+	/** @psalm-suppress MissingClosureParamType */
+	Filesystem::addStorageWrapper('getShare', function ($mountPoint, $storage) use ($share) {
+		return new PublicShareWrapper(['storage' => $storage, 'share' => $share]);
+	}, 0);
+
 	/** @psalm-suppress InternalMethod */
 	Filesystem::logWarningWhenAddingStorageWrapper($previousLog);
 

apps/dav/composer/composer/autoload_classmap.php

@@ -350,6 +350,7 @@ return array(
     'OCA\\DAV\\SetupChecks\\NeedsSystemAddressBookSync' => $baseDir . '/../lib/SetupChecks/NeedsSystemAddressBookSync.php',
     'OCA\\DAV\\SetupChecks\\WebdavEndpoint' => $baseDir . '/../lib/SetupChecks/WebdavEndpoint.php',
     'OCA\\DAV\\Storage\\PublicOwnerWrapper' => $baseDir . '/../lib/Storage/PublicOwnerWrapper.php',
+    'OCA\\DAV\\Storage\\PublicShareWrapper' => $baseDir . '/../lib/Storage/PublicShareWrapper.php',
     'OCA\\DAV\\SystemTag\\SystemTagList' => $baseDir . '/../lib/SystemTag/SystemTagList.php',
     'OCA\\DAV\\SystemTag\\SystemTagMappingNode' => $baseDir . '/../lib/SystemTag/SystemTagMappingNode.php',
     'OCA\\DAV\\SystemTag\\SystemTagNode' => $baseDir . '/../lib/SystemTag/SystemTagNode.php',

apps/dav/composer/composer/autoload_static.php

@@ -365,6 +365,7 @@ class ComposerStaticInitDAV
         'OCA\\DAV\\SetupChecks\\NeedsSystemAddressBookSync' => __DIR__ . '/..' . '/../lib/SetupChecks/NeedsSystemAddressBookSync.php',
         'OCA\\DAV\\SetupChecks\\WebdavEndpoint' => __DIR__ . '/..' . '/../lib/SetupChecks/WebdavEndpoint.php',
         'OCA\\DAV\\Storage\\PublicOwnerWrapper' => __DIR__ . '/..' . '/../lib/Storage/PublicOwnerWrapper.php',
+        'OCA\\DAV\\Storage\\PublicShareWrapper' => __DIR__ . '/..' . '/../lib/Storage/PublicShareWrapper.php',
         'OCA\\DAV\\SystemTag\\SystemTagList' => __DIR__ . '/..' . '/../lib/SystemTag/SystemTagList.php',
         'OCA\\DAV\\SystemTag\\SystemTagMappingNode' => __DIR__ . '/..' . '/../lib/SystemTag/SystemTagMappingNode.php',
         'OCA\\DAV\\SystemTag\\SystemTagNode' => __DIR__ . '/..' . '/../lib/SystemTag/SystemTagNode.php',

apps/dav/lib/Connector/Sabre/FilesPlugin.php

@@ -345,17 +345,14 @@ class FilesPlugin extends ServerPlugin {
 				return $node->getNode()->getInternalPath() === '' ? 'true' : 'false';
 			});
 
-			$propFind->handle(self::SHARE_NOTE, function () use ($node, $httpRequest): ?string {
+			$propFind->handle(self::SHARE_NOTE, function () use ($node): ?string {
 				$user = $this->userSession->getUser();
-				if ($user === null) {
-					return null;
-				}
 				return $node->getNoteFromShare(
-					$user->getUID()
+					$user?->getUID()
 				);
 			});
 
-			$propFind->handle(self::DATA_FINGERPRINT_PROPERTYNAME, function () use ($node) {
+			$propFind->handle(self::DATA_FINGERPRINT_PROPERTYNAME, function () {
 				return $this->config->getSystemValue('data-fingerprint', '');
 			});
 			$propFind->handle(self::CREATIONDATE_PROPERTYNAME, function () use ($node) {

apps/dav/lib/Connector/Sabre/Node.php

@@ -15,6 +15,8 @@ use OCA\DAV\Connector\Sabre\Exception\InvalidPath;
 use OCP\Files\DavUtil;
 use OCP\Files\FileInfo;
 use OCP\Files\IRootFolder;
+use OCP\Files\NotFoundException;
+use OCP\Files\Storage\ISharedStorage;
 use OCP\Files\StorageNotAvailableException;
 use OCP\Share\Exceptions\ShareNotFound;
 use OCP\Share\IManager;
@@ -261,8 +263,8 @@ abstract class Node implements \Sabre\DAV\INode {
 			$storage = null;
 		}
 
-		if ($storage && $storage->instanceOfStorage('\OCA\Files_Sharing\SharedStorage')) {
-			/** @var \OCA\Files_Sharing\SharedStorage $storage */
+		if ($storage && $storage->instanceOfStorage(ISharedStorage::class)) {
+			/** @var ISharedStorage $storage */
 			$permissions = (int)$storage->getShare()->getPermissions();
 		} else {
 			$permissions = $this->info->getPermissions();
@@ -298,16 +300,15 @@ abstract class Node implements \Sabre\DAV\INode {
 	 * @return array
 	 */
 	public function getShareAttributes(): array {
-		$attributes = [];
-
 		try {
-			$storage = $this->info->getStorage();
-		} catch (StorageNotAvailableException $e) {
-			$storage = null;
+			$storage = $this->node->getStorage();
+		} catch (NotFoundException $e) {
+			return [];
 		}
 
-		if ($storage && $storage->instanceOfStorage(\OCA\Files_Sharing\SharedStorage::class)) {
-			/** @var \OCA\Files_Sharing\SharedStorage $storage */
+		$attributes = [];
+		if ($storage->instanceOfStorage(ISharedStorage::class)) {
+			/** @var ISharedStorage $storage */
 			$attributes = $storage->getShare()->getAttributes();
 			if ($attributes === null) {
 				return [];
@@ -319,29 +320,24 @@ abstract class Node implements \Sabre\DAV\INode {
 		return $attributes;
 	}
 
-	/**
-	 * @param string $user
-	 * @return string
-	 */
-	public function getNoteFromShare($user) {
-		if ($user === null) {
-			return '';
+	public function getNoteFromShare(?string $user): ?string {
+		try {
+			$storage = $this->node->getStorage();
+		} catch (NotFoundException) {
+			return null;
 		}
 
-		// Retrieve note from the share object already loaded into
-		// memory, to avoid additional database queries.
-		$storage = $this->getNode()->getStorage();
-		if (!$storage->instanceOfStorage(\OCA\Files_Sharing\SharedStorage::class)) {
-			return '';
+		if ($storage->instanceOfStorage(ISharedStorage::class)) {
+			/** @var ISharedStorage $storage */
+			$share = $storage->getShare();
+			if ($user === $share->getShareOwner()) {
+				// Note is only for recipient not the owner
+				return null;
+			}
+			return $share->getNote();
 		}
-		/** @var \OCA\Files_Sharing\SharedStorage $storage */
 
-		$share = $storage->getShare();
-		$note = $share->getNote();
-		if ($share->getShareOwner() !== $user) {
-			return $note;
-		}
-		return '';
+		return null;
 	}
 
 	/**

apps/dav/lib/DAV/ViewOnlyPlugin.php

@@ -13,6 +13,7 @@ use OCA\DAV\Connector\Sabre\File as DavFile;
 use OCA\Files_Versions\Sabre\VersionFile;
 use OCP\Files\Folder;
 use OCP\Files\NotFoundException;
+use OCP\Files\Storage\ISharedStorage;
 use Sabre\DAV\Exception\NotFound;
 use Sabre\DAV\Server;
 use Sabre\DAV\ServerPlugin;
@@ -81,11 +82,11 @@ class ViewOnlyPlugin extends ServerPlugin {
 
 			$storage = $node->getStorage();
 
-			if (!$storage->instanceOfStorage(\OCA\Files_Sharing\SharedStorage::class)) {
+			if (!$storage->instanceOfStorage(ISharedStorage::class)) {
 				return true;
 			}
 			// Extract extra permissions
-			/** @var \OCA\Files_Sharing\SharedStorage $storage */
+			/** @var ISharedStorage $storage */
 			$share = $storage->getShare();
 
 			$attributes = $share->getAttributes();

apps/dav/lib/Storage/PublicShareWrapper.php

@@ -0,0 +1,39 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCA\DAV\Storage;
+
+use OC\Files\Storage\Wrapper\Wrapper;
+use OCP\Files\Storage\ISharedStorage;
+use OCP\Share\IShare;
+
+class PublicShareWrapper extends Wrapper implements ISharedStorage {
+
+	private IShare $share;
+
+	/**
+	 * @param array $arguments ['storage' => $storage, 'share' => $share]
+	 *
+	 * $storage: The storage the permissions mask should be applied on
+	 * $share: The share to use in case no share is found
+	 */
+	public function __construct($arguments) {
+		parent::__construct($arguments);
+		$this->share = $arguments['share'];
+	}
+
+	public function getShare(): IShare {
+		$storage = parent::getWrapperStorage();
+		if (method_exists($storage, 'getShare')) {
+			/** @var ISharedStorage $storage */
+			return $storage->getShare();
+		}
+
+		return $this->share;
+	}
+}

apps/dav/tests/unit/Connector/Sabre/NodeTest.php

@@ -10,6 +10,7 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre;
 
 use OC\Files\FileInfo;
 use OC\Files\Mount\MountPoint;
+use OC\Files\Node\Folder;
 use OC\Files\View;
 use OC\Share20\ShareAttributes;
 use OCA\Files_Sharing\SharedMount;
@@ -21,6 +22,7 @@ use OCP\Files\Storage;
 use OCP\ICache;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
+use PHPUnit\Framework\MockObject\MockObject;
 
 /**
  * Class NodeTest
@@ -201,14 +203,16 @@ class NodeTest extends \Test\TestCase {
 
 		$share->expects($this->once())->method('getAttributes')->willReturn($attributes);
 
-		$info = $this->getMockBuilder(FileInfo::class)
+		/** @var Folder&MockObject $info */
+		$info = $this->getMockBuilder(Folder::class)
 			->disableOriginalConstructor()
-			->setMethods(['getStorage', 'getType'])
+			->onlyMethods(['getStorage', 'getType'])
 			->getMock();
 
 		$info->method('getStorage')->willReturn($storage);
 		$info->method('getType')->willReturn(FileInfo::TYPE_FOLDER);
 
+		/** @var View&MockObject $view */
 		$view = $this->getMockBuilder(View::class)
 			->disableOriginalConstructor()
 			->getMock();
@@ -225,14 +229,16 @@ class NodeTest extends \Test\TestCase {
 
 		$shareManager = $this->getMockBuilder(IManager::class)->disableOriginalConstructor()->getMock();
 
-		$info = $this->getMockBuilder(FileInfo::class)
+		/** @var Folder&MockObject */
+		$info = $this->getMockBuilder(Folder::class)
 			->disableOriginalConstructor()
-			->setMethods(['getStorage', 'getType'])
+			->onlyMethods(['getStorage', 'getType'])
 			->getMock();
 
 		$info->method('getStorage')->willReturn($storage);
 		$info->method('getType')->willReturn(FileInfo::TYPE_FOLDER);
 
+		/** @var View&MockObject */
 		$view = $this->getMockBuilder(View::class)
 			->disableOriginalConstructor()
 			->getMock();

apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php

@@ -15,6 +15,7 @@ use OCA\Files_Versions\Sabre\VersionFile;
 use OCA\Files_Versions\Versions\IVersion;
 use OCP\Files\File;
 use OCP\Files\Folder;
+use OCP\Files\Storage\ISharedStorage;
 use OCP\Files\Storage\IStorage;
 use OCP\IUser;
 use OCP\Share\IAttributes;
@@ -65,7 +66,7 @@ class ViewOnlyPluginTest extends TestCase {
 
 		$storage = $this->createMock(IStorage::class);
 		$file->method('getStorage')->willReturn($storage);
-		$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(false);
+		$storage->method('instanceOfStorage')->with(ISharedStorage::class)->willReturn(false);
 
 		$this->assertTrue($this->plugin->checkViewOnly($this->request));
 	}
@@ -140,7 +141,7 @@ class ViewOnlyPluginTest extends TestCase {
 		$nodeInfo->expects($this->once())
 			->method('getStorage')
 			->willReturn($storage);
-		$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
+		$storage->method('instanceOfStorage')->with(ISharedStorage::class)->willReturn(true);
 		$storage->method('getShare')->willReturn($share);
 
 		$extAttr = $this->createMock(IAttributes::class);

apps/files_sharing/lib/ISharedStorage.php

@@ -8,5 +8,8 @@ namespace OCA\Files_Sharing;
 
 use OCP\Files\Storage\IStorage;
 
+/**
+ * @deprecated 30.0.0 use `\OCP\Files\Storage\ISharedStorage` instead
+ */
 interface ISharedStorage extends IStorage {
 }

apps/files_sharing/lib/SharedStorage.php

@@ -18,15 +18,16 @@ use OC\Files\Storage\Wrapper\PermissionsMask;
 use OC\Files\Storage\Wrapper\Wrapper;
 use OC\User\NoUserException;
 use OCA\Files_External\Config\ConfigAdapter;
+use OCA\Files_Sharing\ISharedStorage as LegacyISharedStorage;
 use OCP\Constants;
 use OCP\Files\Cache\ICacheEntry;
 use OCP\Files\Config\IUserMountCache;
 use OCP\Files\Folder;
 use OCP\Files\IHomeStorage;
 use OCP\Files\IRootFolder;
-use OCP\Files\Node;
 use OCP\Files\NotFoundException;
 use OCP\Files\Storage\IDisableEncryptionStorage;
+use OCP\Files\Storage\ISharedStorage;
 use OCP\Files\Storage\IStorage;
 use OCP\Lock\ILockingProvider;
 use OCP\Share\IShare;
@@ -35,7 +36,7 @@ use Psr\Log\LoggerInterface;
 /**
  * Convert target path to source path and pass the function call to the correct storage provider
  */
-class SharedStorage extends \OC\Files\Storage\Wrapper\Jail implements ISharedStorage, IDisableEncryptionStorage {
+class SharedStorage extends \OC\Files\Storage\Wrapper\Jail implements LegacyISharedStorage, ISharedStorage, IDisableEncryptionStorage {
 	/** @var \OCP\Share\IShare */
 	private $superShare;
 

apps/files_versions/lib/Versions/LegacyVersionsBackend.php

@@ -12,8 +12,6 @@ namespace OCA\Files_Versions\Versions;
 use Exception;
 use OC\Files\View;
 use OCA\DAV\Connector\Sabre\Exception\Forbidden;
-use OCA\Files_Sharing\ISharedStorage;
-use OCA\Files_Sharing\SharedStorage;
 use OCA\Files_Versions\Db\VersionEntity;
 use OCA\Files_Versions\Db\VersionsMapper;
 use OCA\Files_Versions\Storage;
@@ -24,6 +22,7 @@ use OCP\Files\IMimeTypeLoader;
 use OCP\Files\IRootFolder;
 use OCP\Files\Node;
 use OCP\Files\NotFoundException;
+use OCP\Files\Storage\ISharedStorage;
 use OCP\Files\Storage\IStorage;
 use OCP\IUser;
 use OCP\IUserManager;
@@ -48,7 +47,7 @@ class LegacyVersionsBackend implements IVersionBackend, IDeletableVersionBackend
 	public function getVersionsForFile(IUser $user, FileInfo $file): array {
 		$storage = $file->getStorage();
 
-		if ($storage->instanceOfStorage(SharedStorage::class)) {
+		if ($storage->instanceOfStorage(ISharedStorage::class)) {
 			$owner = $storage->getOwner('');
 			$user = $this->userManager->get($owner);
 
@@ -192,7 +191,7 @@ class LegacyVersionsBackend implements IVersionBackend, IDeletableVersionBackend
 
 		// Shared files have their versions in the owners root folder so we need to obtain them from there
 		if ($storage->instanceOfStorage(ISharedStorage::class) && $owner) {
-			/** @var SharedStorage $storage */
+			/** @var ISharedStorage $storage */
 			$userFolder = $this->rootFolder->getUserFolder($owner->getUID());
 			$user = $owner;
 			$ownerPathInStorage = $sourceFile->getInternalPath();

build/integration/dav_features/dav-v2-public.feature

@@ -0,0 +1,22 @@
+# SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors
+# SPDX-License-Identifier: AGPL-3.0-or-later
+Feature: dav-v2-public
+	Background:
+		Given using api version "1"
+
+	Scenario: See note to recipient in public shares
+		Given using new dav path
+		And As an "admin"
+		And user "user0" exists
+		And user "user1" exists
+		And As an "user1"
+		And user "user1" created a folder "/testshare"
+		And as "user1" creating a share with
+		  | path | testshare |
+		  | shareType | 3 |
+		  | permissions | 1 |
+		  | note | Hello |
+		And As an "user0"
+		Given using new public dav path
+		When Requesting share note on dav endpoint
+		Then the single response should contain a property "{http://nextcloud.org/ns}note" with value "Hello"

build/integration/features/bootstrap/WebDav.php

@@ -52,6 +52,14 @@ trait WebDav {
 		$this->usingOldDavPath = false;
 	}
 
+	/**
+	 * @Given /^using new public dav path$/
+	 */
+	public function usingNewPublicDavPath() {
+		$this->davPath = "public.php/dav";
+		$this->usingOldDavPath = false;
+	}
+
 	public function getDavFilesPath($user) {
 		if ($this->usingOldDavPath === true) {
 			return $this->davPath;
@@ -75,7 +83,7 @@ trait WebDav {
 		];
 		if ($user === 'admin') {
 			$options['auth'] = $this->adminUser;
-		} else {
+		} elseif ($user !== '') {
 			$options['auth'] = [$user, $this->regularUser];
 		}
 		return $client->request($method, $fullUrl, $options);
@@ -941,6 +949,23 @@ trait WebDav {
 		}
 	}
 
+	/**
+	 * @When Requesting share note on dav endpoint
+	 */
+	public function requestingShareNote() {
+		$propfind = '<d:propfind xmlns:d="DAV:" xmlns:nc="http://nextcloud.org/ns"><d:prop><nc:note /></d:prop></d:propfind>';
+		if (count($this->lastShareData->data->element) > 0) {
+			$token = $this->lastShareData->data[0]->token;
+		} else {
+			$token = $this->lastShareData->data->token;
+		}
+		try {
+			$this->response = $this->makeDavRequest('', 'PROPFIND', $token, [], $propfind);
+		} catch (\GuzzleHttp\Exception\ClientException $e) {
+			$this->response = $e->getResponse();
+		}
+	}
+
 	/**
 	 * @Then there are no duplicate headers
 	 */

build/integration/run-docker.sh

@@ -201,7 +201,7 @@ cd "$(dirname $0)"
 # "--image XXX" option can be provided to set the Docker image to use to run
 # the integration tests (one of the "nextcloudci/phpX.Y:phpX.Y-Z" or
 # "ghcr.io/nextcloud/continuous-integration-integration-phpX.Y:latest" images).
-NEXTCLOUD_LOCAL_IMAGE="ghcr.io/nextcloud/continuous-integration-integration-php8.0:latest"
+NEXTCLOUD_LOCAL_IMAGE="ghcr.io/nextcloud/continuous-integration-integration-php8.2:latest"
 if [ "$1" = "--image" ]; then
 	NEXTCLOUD_LOCAL_IMAGE=$2
 
@@ -227,9 +227,9 @@ fi
 # "--database-image XXX" option can be provided to set the Docker image to use
 # for the database container (ignored when using "sqlite").
 if [ "$DATABASE" = "mysql" ]; then
-	DATABASE_IMAGE="mysql:5.7"
+	DATABASE_IMAGE="mysql:8.4"
 elif [ "$DATABASE" = "pgsql" ]; then
-	DATABASE_IMAGE="postgres:10"
+	DATABASE_IMAGE="postgres:15"
 fi
 if [ "$1" = "--database-image" ]; then
 	DATABASE_IMAGE=$2

lib/composer/composer/autoload_classmap.php

@@ -432,6 +432,7 @@ return array(
     'OCP\\Files\\Storage\\ILockingStorage' => $baseDir . '/lib/public/Files/Storage/ILockingStorage.php',
     'OCP\\Files\\Storage\\INotifyStorage' => $baseDir . '/lib/public/Files/Storage/INotifyStorage.php',
     'OCP\\Files\\Storage\\IReliableEtagStorage' => $baseDir . '/lib/public/Files/Storage/IReliableEtagStorage.php',
+    'OCP\\Files\\Storage\\ISharedStorage' => $baseDir . '/lib/public/Files/Storage/ISharedStorage.php',
     'OCP\\Files\\Storage\\IStorage' => $baseDir . '/lib/public/Files/Storage/IStorage.php',
     'OCP\\Files\\Storage\\IStorageFactory' => $baseDir . '/lib/public/Files/Storage/IStorageFactory.php',
     'OCP\\Files\\Storage\\IWriteStreamStorage' => $baseDir . '/lib/public/Files/Storage/IWriteStreamStorage.php',

lib/composer/composer/autoload_static.php

@@ -465,6 +465,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
         'OCP\\Files\\Storage\\ILockingStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/ILockingStorage.php',
         'OCP\\Files\\Storage\\INotifyStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/INotifyStorage.php',
         'OCP\\Files\\Storage\\IReliableEtagStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IReliableEtagStorage.php',
+        'OCP\\Files\\Storage\\ISharedStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/ISharedStorage.php',
         'OCP\\Files\\Storage\\IStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IStorage.php',
         'OCP\\Files\\Storage\\IStorageFactory' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IStorageFactory.php',
         'OCP\\Files\\Storage\\IWriteStreamStorage' => __DIR__ . '/../../..' . '/lib/public/Files/Storage/IWriteStreamStorage.php',

lib/public/Files/Storage/ISharedStorage.php

@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+namespace OCP\Files\Storage;
+
+use OCP\Share\IShare;
+
+/**
+ * Interface for a storage that is based on a file share
+ *
+ * @since 30.0.0
+ */
+interface ISharedStorage extends IStorage {
+	/**
+	 * The the associated share
+	 *
+	 * @return IShare
+	 * @since 30.0.0
+	 */
+	public function getShare(): IShare;
+}