fix share roots always being marked as writable

Signed-off-by: Robin Appelman <robin@icewind.nl>
nextcloud · Jun 8, 2023 · 533456d · 533456d
1 parent faf0e63
commit 533456d
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 23 deletions.
diff --git a/apps/dav/tests/unit/Connector/Sabre/NodeTest.php b/apps/dav/tests/unit/Connector/Sabre/NodeTest.php
@@ -25,14 +25,18 @@
  * 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\View;
 use OC\Share20\ShareAttributes;
 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 +50,58 @@
 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'])
 			->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);
+		$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 +278,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],
 		];
 	}
 

diff --git a/lib/public/Files/DavUtil.php b/lib/public/Files/DavUtil.php
@@ -32,6 +32,9 @@
 
 namespace OCP\Files;
 
+use OCA\Files_Sharing\SharedStorage;
+use OCP\Constants;
+
 /**
  * This class provides different helper functions related to WebDAV protocol
  *
@@ -75,8 +78,20 @@ public static function getDavPermissions(FileInfo $info): string {
 		if ($info->isUpdateable()) {
 			$p .= 'NV'; // Renameable, Moveable
 		}
+
+		// since we always add update permissions for the root of movable mounts (and thus shares)
+		// we need to check the shared cache item directly to determine if it's writable
+		$storage = $info->getStorage();
+		/** @psalm-suppress UndefinedClass files_sharing may be disabled but this will still work as expected */
+		if ($info->getInternalPath() === '' && $storage->instanceOfStorage(SharedStorage::class)) {
+			$shareEntry = $storage->getCache()->get('');
+			$isWritable = ($shareEntry->getPermissions() & Constants::PERMISSION_UPDATE);
+		} else {
+			$isWritable = $info->isUpdateable();
+		}
+
 		if ($info->getType() === FileInfo::TYPE_FILE) {
-			if ($info->isUpdateable()) {
+			if ($isWritable) {
 				$p .= 'W';
 			}
 		} else {