DAV permissions for read only single file shares indicate updatable

[juliusknorr]

This seems like a bug that i noticed when trying to check for the permissions using @nextcloud/files where it turned out my file action did not hide on read only shares.

Steps to reproduce

• as userA: Share a single file as view only (permission: 1) to userB
• as userB: Check the webdav response and see <oc:permissions>SGDNV</oc:permissions> indicating that the file can be updated

Additional context

• This does not happen with folder shares, there both the root mount and files within have correct webdav permissions exposed
• I already spend some time stepping through code
  • Permissions in the FileInfo in

    server/lib/public/Files/DavUtil.php

    Line 36 in dae7c15

    $permissions = $info->getPermissions();

    are already wrong with 11 (unsure where that is originating from, but my guess is somewhere in

    server/lib/private/Files/View.php

    Lines 1482 to 1650 in 01db539

    	public function getDirectoryContent($directory, $mimetype_filter = '', ?\OCP\Files\FileInfo $directoryInfo = null) {
    	$this->assertPathLength($directory);
    	if (!Filesystem::isValidPath($directory)) {
    	return [];
    	}
    	$path = $this->getAbsolutePath($directory);
    	$path = Filesystem::normalizePath($path);
    	$mount = $this->getMount($directory);
    	$storage = $mount->getStorage();
    	$internalPath = $mount->getInternalPath($path);
    	if (!$storage) {
    	return [];
    	}
    	$cache = $storage->getCache($internalPath);
    	$user = \OC_User::getUser();
    	if (!$directoryInfo) {
    	$data = $this->getCacheEntry($storage, $internalPath, $directory);
    	if (!$data instanceof ICacheEntry || !isset($data['fileid'])) {
    	return [];
    	}
    	} else {
    	$data = $directoryInfo;
    	}
    	if (!($data->getPermissions() & Constants::PERMISSION_READ)) {
    	return [];
    	}
    	$folderId = $data->getId();
    	$contents = $cache->getFolderContentsById($folderId); //TODO: mimetype_filter
    	$sharingDisabled = \OCP\Util::isSharingDisabledForUser();
    	$fileNames = array_map(function (ICacheEntry $content) {
    	return $content->getName();
    	}, $contents);
    	/**
    	* @var \OC\Files\FileInfo[] $fileInfos
    	*/
    	$fileInfos = array_map(function (ICacheEntry $content) use ($path, $storage, $mount, $sharingDisabled) {
    	if ($sharingDisabled) {
    	$content['permissions'] = $content['permissions'] & ~\OCP\Constants::PERMISSION_SHARE;
    	}
    	$ownerId = $storage->getOwner($content['path']);
    	if ($ownerId !== false) {
    	$owner = $this->getUserObjectForOwner($ownerId);
    	} else {
    	$owner = null;
    	}
    	return new FileInfo($path . '/' . $content['name'], $storage, $content['path'], $content, $mount, $owner);
    	}, $contents);
    	$files = array_combine($fileNames, $fileInfos);
    	//add a folder for any mountpoint in this directory and add the sizes of other mountpoints to the folders
    	$mounts = Filesystem::getMountManager()->findIn($path);
    	// make sure nested mounts are sorted after their parent mounts
    	// otherwise doesn't propagate the etag across storage boundaries correctly
    	usort($mounts, function (IMountPoint $a, IMountPoint $b) {
    	return $a->getMountPoint() <=> $b->getMountPoint();
    	});
    	$dirLength = strlen($path);
    	foreach ($mounts as $mount) {
    	$mountPoint = $mount->getMountPoint();
    	$subStorage = $mount->getStorage();
    	if ($subStorage) {
    	$subCache = $subStorage->getCache('');
    	$rootEntry = $subCache->get('');
    	if (!$rootEntry) {
    	$subScanner = $subStorage->getScanner();
    	try {
    	$subScanner->scanFile('');
    	} catch (\OCP\Files\StorageNotAvailableException $e) {
    	continue;
    	} catch (\OCP\Files\StorageInvalidException $e) {
    	continue;
    	} catch (\Exception $e) {
    	// sometimes when the storage is not available it can be any exception
    	$this->logger->error('Exception while scanning storage "' . $subStorage->getId() . '"', [
    	'exception' => $e,
    	'app' => 'core',
    	]);
    	continue;
    	}
    	$rootEntry = $subCache->get('');
    	}
    	if ($rootEntry && ($rootEntry->getPermissions() & Constants::PERMISSION_READ)) {
    	$relativePath = trim(substr($mountPoint, $dirLength), '/');
    	if ($pos = strpos($relativePath, '/')) {
    	//mountpoint inside subfolder add size to the correct folder
    	$entryName = substr($relativePath, 0, $pos);
    	// Create parent folders if the mountpoint is inside a subfolder that doesn't exist yet
    	if (!isset($files[$entryName])) {
    	try {
    	[$storage, ] = $this->resolvePath($path . '/' . $entryName);
    	// make sure we can create the mountpoint folder, even if the user has a quota of 0
    	if ($storage->instanceOfStorage(Quota::class)) {
    	$storage->enableQuota(false);
    	}
    	if ($this->mkdir($path . '/' . $entryName) !== false) {
    	$info = $this->getFileInfo($path . '/' . $entryName);
    	if ($info !== false) {
    	$files[$entryName] = $info;
    	}
    	}
    	if ($storage->instanceOfStorage(Quota::class)) {
    	$storage->enableQuota(true);
    	}
    	} catch (\Exception $e) {
    	// Creating the parent folder might not be possible, for example due to a lack of permissions.
    	$this->logger->debug('Failed to create non-existent parent', ['exception' => $e, 'path' => $path . '/' . $entryName]);
    	}
    	}
    	if (isset($files[$entryName])) {
    	$files[$entryName]->addSubEntry($rootEntry, $mountPoint);
    	}
    	} else { //mountpoint in this folder, add an entry for it
    	$rootEntry['name'] = $relativePath;
    	$rootEntry['type'] = $rootEntry['mimetype'] === 'httpd/unix-directory' ? 'dir' : 'file';
    	$permissions = $rootEntry['permissions'];
    	// do not allow renaming/deleting the mount point if they are not shared files/folders
    	// for shared files/folders we use the permissions given by the owner
    	if ($mount instanceof MoveableMount) {
    	$rootEntry['permissions'] = $permissions | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE;
    	} else {
    	$rootEntry['permissions'] = $permissions & (\OCP\Constants::PERMISSION_ALL - (\OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE));
    	}
    	$rootEntry['path'] = substr(Filesystem::normalizePath($path . '/' . $rootEntry['name']), strlen($user) + 2); // full path without /$user/
    	// if sharing was disabled for the user we remove the share permissions
    	if ($sharingDisabled) {
    	$rootEntry['permissions'] = $rootEntry['permissions'] & ~\OCP\Constants::PERMISSION_SHARE;
    	}
    	$ownerId = $subStorage->getOwner('');
    	if ($ownerId !== false) {
    	$owner = $this->getUserObjectForOwner($ownerId);
    	} else {
    	$owner = null;
    	}
    	$files[$rootEntry->getName()] = new FileInfo($path . '/' . $rootEntry['name'], $subStorage, '', $rootEntry, $mount, $owner);
    	}
    	}
    	}
    	}
    	if ($mimetype_filter) {
    	$files = array_filter($files, function (FileInfo $file) use ($mimetype_filter) {
    	if (strpos($mimetype_filter, '/')) {
    	return $file->getMimetype() === $mimetype_filter;
    	} else {
    	return $file->getMimePart() === $mimetype_filter;
    	}
    	});
    	}
    	return array_values($files);
    	}

    )

[juliusknorr]

One step further it actually seems like

server/lib/private/Files/View.php

Lines 1614 to 1616 in 01db539

	if ($mount instanceof MoveableMount) {
	$rootEntry['permissions'] = $permissions | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE;
	} else {

is setting the update permission for movable mounts to support renames but somewhat conflicts the the file still not being updatable. However on the storage/file info permissions we do not have separate ones.

For WebDAV there are and we have a similar logic in

server/lib/public/Files/DavUtil.php

Lines 59 to 61 in dae7c15

	$storage = $info->getStorage();
	if ($info->getInternalPath() === '' && $info->getMountPoint() instanceof IMovableMount) {
	$rootEntry = $storage->getCache()->get('');

[juliusknorr]

Ok so it seems we just loose the difference between move, rename and update permissions that the webdav response has in https://github.com/nextcloud-libraries/nextcloud-files/blob/main/lib/dav/dav.ts#L173C14-L202

@skjnldsv @susnux I think that is something we should handle in @nextcloud/files. Would it make sense to still expose the permission string in the node object? Any other preferences how we could expose this?

Edit: use case and catched when trying to hide a file action for files that user can not edit: https://github.com/nextcloud/files_lock/pull/683/files#diff-a09b7f67f5f992f7ef9d9299f6fac774fb218572c9c2960eb2c30293067a8843R25

[susnux]

So needs to be updatable only:

• is file && 'W'
• is folder && 'NV'

if a file has 'NV' then we can only rename or move it, maybe a new permission here.

[juliusknorr]

I'm not sure what other checks are using the updatable permission as well.

Also for an incoming single file share it could be read only (1) as storage permission but the share itself would still be NV, so not sure how to differentiate that between that and a file with 'NVW'

Currently I see no way around separate options to check if the file is renamable/movable

[tmorgumich]

I reproduced this on a fresh Nextcloud dev stack with three users: admin, alice, bob.
Alice created demo.txt and shared it to Bob as view-only (no edit/upload/delete).
Response (trimmed):
<[oc:permissions]>SGD</oc:permissions>
<[oc:id]>00000277ocr3zcd751v6</oc:id>
However, Bob can still move the file:
curl -i -sS -u 'bob:bob' -X MOVE
-H 'Destination: http://nextcloud.local/remote.php/dav/files/bob/demo-renamed.txt'
'http://nextcloud.local/remote.php/dav/files/bob/demo.txt'

Result: HTTP/1.1 201 Created

This confirms that for single-file incoming shares, the oc:permissions property doesn't reliably indicate actual MOVE (rename) rights— The permissions code, SGD, is correct from Bob’s view: he is only allowed to see/share/delete, and not to update the file.
However, the fact that a MOVE operation succeeds.

To address this, I propose to write some tests to confirm the issue and update View.php and DavUtil.php, thinking that the permission checks are too permissive or not synchronized with the permissions string.

Does this direction sound good to the maintainers? If so, I'm happy to open a PR for this.

Thanks Julius and all!

[tmorgumich]

further assertion of the issue from added test to ViewTest.php:

root@c52595f409c6:/var/www/html# phpunit --bootstrap tests/bootstrap.php --filter testSingleFileReadOnlySharePermissions tests/lib/Files/ViewTest.php
PHPUnit 9.6.24 by Sebastian Bergmann and contributors.
...
There was 1 failure:

1. Test\Files\ViewTest::testSingleFileReadOnlySharePermissions
   Recipient SHOULDNT be able to move a read-only shared file
   Failed asserting that true is false.

/var/www/html/tests/lib/Files/ViewTest.php:2937

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.