Merge pull request #38179 from nextcloud/dav-permissions-share-root-w…

…rite

fix share roots always being marked as writable

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

@@ -25,14 +25,20 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>
  *
  */
+
 namespace OCA\DAV\Tests\unit\Connector\Sabre;
 
 use OC\Files\FileInfo;
+use OC\Files\Mount\MountPoint;
 use OC\Files\View;
 use OC\Share20\ShareAttributes;
+use OCA\Files_Sharing\SharedMount;
 use OCA\Files_Sharing\SharedStorage;
+use OCP\Constants;
+use OCP\Files\Cache\ICacheEntry;
 use OCP\Files\Mount\IMountPoint;
 use OCP\Files\Storage;
+use OCP\ICache;
 use OCP\Share\IAttributes;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
@@ -46,40 +52,66 @@
 class NodeTest extends \Test\TestCase {
 	public function davPermissionsProvider() {
 		return [
-			[\OCP\Constants::PERMISSION_ALL, 'file', false, false, 'RGDNVW'],
-			[\OCP\Constants::PERMISSION_ALL, 'dir', false, false, 'RGDNVCK'],
-			[\OCP\Constants::PERMISSION_ALL, 'file', true, false, 'SRGDNVW'],
-			[\OCP\Constants::PERMISSION_ALL, 'file', true, true, 'SRMGDNVW'],
-			[\OCP\Constants::PERMISSION_ALL - \OCP\Constants::PERMISSION_SHARE, 'file', true, false, 'SGDNVW'],
-			[\OCP\Constants::PERMISSION_ALL - \OCP\Constants::PERMISSION_UPDATE, 'file', false, false, 'RGD'],
-			[\OCP\Constants::PERMISSION_ALL - \OCP\Constants::PERMISSION_DELETE, 'file', false, false, 'RGNVW'],
-			[\OCP\Constants::PERMISSION_ALL - \OCP\Constants::PERMISSION_CREATE, 'file', false, false, 'RGDNVW'],
-			[\OCP\Constants::PERMISSION_ALL - \OCP\Constants::PERMISSION_READ, 'file', false, false, 'RDNVW'],
-			[\OCP\Constants::PERMISSION_ALL - \OCP\Constants::PERMISSION_CREATE, 'dir', false, false, 'RGDNV'],
-			[\OCP\Constants::PERMISSION_ALL - \OCP\Constants::PERMISSION_READ, 'dir', false, false, 'RDNVCK'],
+			[Constants::PERMISSION_ALL, 'file', false, Constants::PERMISSION_ALL, false, 'test', 'RGDNVW'],
+			[Constants::PERMISSION_ALL, 'dir', false, Constants::PERMISSION_ALL, false, 'test', 'RGDNVCK'],
+			[Constants::PERMISSION_ALL, 'file', true, Constants::PERMISSION_ALL, false, 'test', 'SRGDNVW'],
+			[Constants::PERMISSION_ALL, 'file', true, Constants::PERMISSION_ALL, true, 'test', 'SRMGDNVW'],
+			[Constants::PERMISSION_ALL, 'file', true, Constants::PERMISSION_ALL, true, '' , 'SRMGDNVW'],
+			[Constants::PERMISSION_ALL, 'file', true, Constants::PERMISSION_ALL - Constants::PERMISSION_UPDATE, true, '' , 'SRMGDNV'],
+			[Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE, 'file', true, Constants::PERMISSION_ALL, false, 'test', 'SGDNVW'],
+			[Constants::PERMISSION_ALL - Constants::PERMISSION_UPDATE, 'file', false, Constants::PERMISSION_ALL, false, 'test', 'RGD'],
+			[Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, 'file', false, Constants::PERMISSION_ALL, false, 'test', 'RGNVW'],
+			[Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE, 'file', false, Constants::PERMISSION_ALL, false, 'test', 'RGDNVW'],
+			[Constants::PERMISSION_ALL - Constants::PERMISSION_READ, 'file', false, Constants::PERMISSION_ALL, false, 'test', 'RDNVW'],
+			[Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE, 'dir', false, Constants::PERMISSION_ALL, false, 'test', 'RGDNV'],
+			[Constants::PERMISSION_ALL - Constants::PERMISSION_READ, 'dir', false, Constants::PERMISSION_ALL, false, 'test', 'RDNVCK'],
 		];
 	}
 
 	/**
 	 * @dataProvider davPermissionsProvider
 	 */
-	public function testDavPermissions($permissions, $type, $shared, $mounted, $expected): void {
+	public function testDavPermissions($permissions, $type, $shared, $shareRootPermissions, $mounted, $internalPath, $expected): void {
 		$info = $this->getMockBuilder(FileInfo::class)
 			->disableOriginalConstructor()
-			->setMethods(['getPermissions', 'isShared', 'isMounted', 'getType'])
+			->onlyMethods(['getPermissions', 'isShared', 'isMounted', 'getType', 'getInternalPath', 'getStorage', 'getMountPoint'])
 			->getMock();
-		$info->expects($this->any())
-			->method('getPermissions')
+		$info->method('getPermissions')
 			->willReturn($permissions);
-		$info->expects($this->any())
-			->method('isShared')
+		$info->method('isShared')
 			->willReturn($shared);
-		$info->expects($this->any())
-			->method('isMounted')
+		$info->method('isMounted')
 			->willReturn($mounted);
-		$info->expects($this->any())
-			->method('getType')
+		$info->method('getType')
 			->willReturn($type);
+		$info->method('getInternalPath')
+			->willReturn($internalPath);
+		$info->method('getMountPoint')
+			->willReturnCallback(function() use ($shared) {
+				if ($shared) {
+					return $this->createMock(SharedMount::class);
+				} else {
+					return $this->createMock(MountPoint::class);
+				}
+			});
+		$storage = $this->createMock(Storage\IStorage::class);
+		if ($shared) {
+			$storage->method('instanceOfStorage')
+				->willReturn(true);
+			$cache = $this->createMock(ICache::class);
+			$storage->method('getCache')
+				->willReturn($cache);
+			$shareRootEntry = $this->createMock(ICacheEntry::class);
+			$cache->method('get')
+				->willReturn($shareRootEntry);
+			$shareRootEntry->method('getPermissions')
+				->willReturn($shareRootPermissions);
+		} else {
+			$storage->method('instanceOfStorage')
+				->willReturn(false);
+		}
+		$info->method('getStorage')
+			->willReturn($storage);
 		$view = $this->getMockBuilder(View::class)
 			->disableOriginalConstructor()
 			->getMock();
@@ -256,7 +288,7 @@ public function testSanitizeMtime($mtime, $expected): void {
 
 	public function invalidSanitizeMtimeProvider() {
 		return [
-			[-1337], [0], ['abcdef'], ['-1337'], ['0'], [12321], [24 * 60 * 60 - 1]
+			[-1337], [0], ['abcdef'], ['-1337'], ['0'], [12321], [24 * 60 * 60 - 1],
 		];
 	}
 

lib/composer/composer/autoload_classmap.php

@@ -350,6 +350,7 @@
     'OCP\\Files\\Lock\\OwnerLockedException' => $baseDir . '/lib/public/Files/Lock/OwnerLockedException.php',
     'OCP\\Files\\Mount\\IMountManager' => $baseDir . '/lib/public/Files/Mount/IMountManager.php',
     'OCP\\Files\\Mount\\IMountPoint' => $baseDir . '/lib/public/Files/Mount/IMountPoint.php',
+    'OCP\\Files\\Mount\\IMovableMount' => $baseDir . '/lib/public/Files/Mount/IMovableMount.php',
     'OCP\\Files\\Mount\\ISystemMountPoint' => $baseDir . '/lib/public/Files/Mount/ISystemMountPoint.php',
     'OCP\\Files\\Node' => $baseDir . '/lib/public/Files/Node.php',
     'OCP\\Files\\NotEnoughSpaceException' => $baseDir . '/lib/public/Files/NotEnoughSpaceException.php',

lib/composer/composer/autoload_static.php

@@ -383,6 +383,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
         'OCP\\Files\\Lock\\OwnerLockedException' => __DIR__ . '/../../..' . '/lib/public/Files/Lock/OwnerLockedException.php',
         'OCP\\Files\\Mount\\IMountManager' => __DIR__ . '/../../..' . '/lib/public/Files/Mount/IMountManager.php',
         'OCP\\Files\\Mount\\IMountPoint' => __DIR__ . '/../../..' . '/lib/public/Files/Mount/IMountPoint.php',
+        'OCP\\Files\\Mount\\IMovableMount' => __DIR__ . '/../../..' . '/lib/public/Files/Mount/IMovableMount.php',
         'OCP\\Files\\Mount\\ISystemMountPoint' => __DIR__ . '/../../..' . '/lib/public/Files/Mount/ISystemMountPoint.php',
         'OCP\\Files\\Node' => __DIR__ . '/../../..' . '/lib/public/Files/Node.php',
         'OCP\\Files\\NotEnoughSpaceException' => __DIR__ . '/../../..' . '/lib/public/Files/NotEnoughSpaceException.php',

lib/private/Files/Mount/MoveableMount.php

@@ -22,10 +22,12 @@
  */
 namespace OC\Files\Mount;
 
+use OCP\Files\Mount\IMovableMount;
+
 /**
  * Defines the mount point to be (re)moved by the user
  */
-interface MoveableMount {
+interface MoveableMount extends IMovableMount {
 	/**
 	 * Move the mount point to $target
 	 *

lib/public/Files/DavUtil.php

@@ -32,6 +32,9 @@
 
 namespace OCP\Files;
 
+use OCP\Constants;
+use OCP\Files\Mount\IMovableMount;
+
 /**
  * This class provides different helper functions related to WebDAV protocol
  *
@@ -73,10 +76,21 @@ public static function getDavPermissions(FileInfo $info): string {
 			$p .= 'D';
 		}
 		if ($info->isUpdateable()) {
-			$p .= 'NV'; // Renameable, Moveable
+			$p .= 'NV'; // Renameable, Movable
 		}
+
+		// since we always add update permissions for the root of movable mounts
+		// we need to check the shared cache item directly to determine if it's writable
+		$storage = $info->getStorage();
+		if ($info->getInternalPath() === '' && $info->getMountPoint() instanceof IMovableMount) {
+			$rootEntry = $storage->getCache()->get('');
+			$isWritable = $rootEntry->getPermissions() & Constants::PERMISSION_UPDATE;
+		} else {
+			$isWritable = $info->isUpdateable();
+		}
+
 		if ($info->getType() === FileInfo::TYPE_FILE) {
-			if ($info->isUpdateable()) {
+			if ($isWritable) {
 				$p .= 'W';
 			}
 		} else {

lib/public/Files/Mount/IMovableMount.php

@@ -0,0 +1,48 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2023 Robin Appelman <[email protected]>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCP\Files\Mount;
+
+/**
+ * Denotes that the mount point can be (re)moved by the user
+ *
+ * @since 28.0.0
+ */
+interface IMovableMount {
+	/**
+	 * Move the mount point to $target
+	 *
+	 * @param string $target the target mount point
+	 * @return bool
+	 * @since 28.0.0
+	 */
+	public function moveMount($target);
+
+	/**
+	 * Remove the mount points
+	 *
+	 * @return bool
+	 * @since 28.0.0
+	 */
+	public function removeMount();
+}