fix(files_sharing): fix parsing of remote shares

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

@@ -7,6 +7,7 @@
  */
 namespace OCA\DAV\Connector\Sabre;
 
+use OC\Share20\Exception\BackendError;
 use OCA\DAV\Connector\Sabre\Node as DavNode;
 use OCP\Files\Folder;
 use OCP\Files\Node;
@@ -33,24 +34,19 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin {
 	 * @var \Sabre\DAV\Server
 	 */
 	private $server;
-	private IManager $shareManager;
-	private Tree $tree;
 	private string $userId;
-	private Folder $userFolder;
+
 	/** @var IShare[][] */
 	private array $cachedShares = [];
 	/** @var string[] */
 	private array $cachedFolders = [];
 
 	public function __construct(
-		Tree $tree,
-		IUserSession $userSession,
-		Folder $userFolder,
-		IManager $shareManager
+		private Tree $tree,
+		private IUserSession $userSession,
+		private Folder $userFolder,
+		private IManager $shareManager,
 	) {
-		$this->tree = $tree;
-		$this->shareManager = $shareManager;
-		$this->userFolder = $userFolder;
 		$this->userId = $userSession->getUser()->getUID();
 	}
 
@@ -91,18 +87,29 @@ private function getShare(Node $node): array {
 			IShare::TYPE_DECK,
 			IShare::TYPE_SCIENCEMESH,
 		];
+
 		foreach ($requestedShareTypes as $requestedShareType) {
-			$shares = $this->shareManager->getSharesBy(
+			$result = array_merge($result, $this->shareManager->getSharesBy(
 				$this->userId,
 				$requestedShareType,
 				$node,
 				false,
 				-1
-			);
-			foreach ($shares as $share) {
-				$result[] = $share;
+			));
+
+			// Also check for shares where the user is the recipient
+			try {
+				$result = array_merge($result, $this->shareManager->getSharedWith(
+					$this->userId,
+					$requestedShareType,
+					$node,
+					-1
+				));
+			} catch (BackendError $e) {
+				// ignore
 			}
 		}
+
 		return $result;
 	}
 
@@ -124,27 +131,29 @@ private function getSharesFolder(Folder $node): array {
 	 */
 	private function getShares(DavNode $sabreNode): array {
 		if (isset($this->cachedShares[$sabreNode->getId()])) {
-			$shares = $this->cachedShares[$sabreNode->getId()];
-		} else {
-			[$parentPath,] = \Sabre\Uri\split($sabreNode->getPath());
-			if ($parentPath === '') {
-				$parentPath = '/';
-			}
-			// if we already cached the folder this file is in we know there are no shares for this file
-			if (array_search($parentPath, $this->cachedFolders) === false) {
-				try {
-					$node = $sabreNode->getNode();
-				} catch (NotFoundException $e) {
-					return [];
-				}
-				$shares = $this->getShare($node);
-				$this->cachedShares[$sabreNode->getId()] = $shares;
-			} else {
+			return $this->cachedShares[$sabreNode->getId()];
+		}
+
+		[$parentPath,] = \Sabre\Uri\split($sabreNode->getPath());
+		if ($parentPath === '') {
+			$parentPath = '/';
+		}
+
+		// if we already cached the folder containing this file
+		// then we already know there are no shares here.
+		if (array_search($parentPath, $this->cachedFolders) === false) {
+			try {
+				$node = $sabreNode->getNode();
+			} catch (NotFoundException $e) {
 				return [];
 			}
+
+			$shares = $this->getShare($node);
+			$this->cachedShares[$sabreNode->getId()] = $shares;
+			return $shares;
 		}
 
-		return $shares;
+		return [];
 	}
 
 	/**
@@ -161,7 +170,9 @@ public function handleGetProperties(
 			return;
 		}
 
-		// need prefetch ?
+		// If the node is a directory and we are requesting share types or sharees
+		// then we get all the shares in the folder and cache them.
+		// This is more performant than iterating each files afterwards.
 		if ($sabreNode instanceof Directory
 			&& $propFind->getDepth() !== 0
 			&& (

apps/dav/lib/Server.php

@@ -263,13 +263,15 @@ public function __construct(IRequest $request, string $baseUri) {
 						$this->server->tree, \OC::$server->getTagManager()
 					)
 				);
+
 				// TODO: switch to LazyUserFolder
 				$userFolder = \OC::$server->getUserFolder();
+				$shareManager = \OCP\Server::get(\OCP\Share\IManager::class);
 				$this->server->addPlugin(new SharesPlugin(
 					$this->server->tree,
 					$userSession,
 					$userFolder,
-					\OC::$server->getShareManager()
+					$shareManager,
 				));
 				$this->server->addPlugin(new CommentPropertiesPlugin(
 					\OC::$server->getCommentsManager(),
@@ -304,7 +306,7 @@ public function __construct(IRequest $request, string $baseUri) {
 						$this->server->tree,
 						$user,
 						\OC::$server->getRootFolder(),
-						\OC::$server->getShareManager(),
+						$shareManager,
 						$view,
 						\OCP\Server::get(IFilesMetadataManager::class)
 					));

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

@@ -97,7 +97,7 @@ public function testGetProperties($shareTypes): void {
 			->with(
 				$this->equalTo('user1'),
 				$this->anything(),
-				$this->anything(),
+				$this->equalTo($node),
 				$this->equalTo(false),
 				$this->equalTo(-1)
 			)
@@ -111,6 +111,16 @@ public function testGetProperties($shareTypes): void {
 				return [];
 			});
 
+		$this->shareManager->expects($this->any())
+			->method('getSharedWith')
+			->with(
+				$this->equalTo('user1'),
+				$this->anything(),
+				$this->equalTo($node),
+				$this->equalTo(-1)
+			)
+			->willReturn([]);
+
 		$propFind = new \Sabre\DAV\PropFind(
 			'/dummyPath',
 			[self::SHARETYPES_PROPERTYNAME],
@@ -199,6 +209,16 @@ public function testPreloadThenGetProperties($shareTypes): void {
 				return [];
 			});
 
+		$this->shareManager->expects($this->any())
+			->method('getSharedWith')
+			->with(
+				$this->equalTo('user1'),
+				$this->anything(),
+				$this->equalTo($node),
+				$this->equalTo(-1)
+			)
+			->willReturn([]);
+
 		$this->shareManager->expects($this->any())
 			->method('getSharesInFolder')
 			->with(

apps/files/src/actions/moveOrCopyActionUtils.ts

@@ -51,7 +51,8 @@ export const canDownload = (nodes: Node[]) => {
 }
 
 export const canCopy = (nodes: Node[]) => {
-	// For now the only restriction is that a shared file
-	// cannot be copied if the download is disabled
+	// a shared file cannot be copied if the download is disabled
+	// it can be copied if the user has at least read permissions
 	return canDownload(nodes)
+		&& !nodes.some(node => node.permissions === Permission.NONE)
 }

apps/files/src/components/FileEntry.vue

@@ -65,7 +65,7 @@
 			class="files-list__row-mtime"
 			data-cy-files-list-row-mtime
 			@click="openDetailsIfAvailable">
-			<NcDateTime :timestamp="source.mtime" :ignore-seconds="true" />
+			<NcDateTime v-if="source.mtime" :timestamp="source.mtime" :ignore-seconds="true" />
 		</td>
 
 		<!-- View columns -->
@@ -177,17 +177,17 @@ export default defineComponent({
 		},
 
 		size() {
-			const size = parseInt(this.source.size, 10) || 0
-			if (typeof size !== 'number' || size < 0) {
+			const size = parseInt(this.source.size, 10)
+			if (typeof size !== 'number' || isNaN(size) || size < 0) {
 				return this.t('files', 'Pending')
 			}
 			return formatFileSize(size, true)
 		},
 		sizeOpacity() {
 			const maxOpacitySize = 10 * 1024 * 1024
 
-			const size = parseInt(this.source.size, 10) || 0
-			if (!size || size < 0) {
+			const size = parseInt(this.source.size, 10)
+			if (!size || isNaN(size) || size < 0) {
 				return {}
 			}
 

apps/files_sharing/src/actions/sharingStatusAction.ts

@@ -17,14 +17,22 @@ import { getCurrentUser } from '@nextcloud/auth'
 
 import './sharingStatusAction.scss'
 
-const generateAvatarSvg = (userId: string) => {
-	const avatarUrl = generateUrl('/avatar/{userId}/32', { userId })
+const isDarkMode = window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches === true
+	|| document.querySelector('[data-themes*=dark]') !== null
+
+const generateAvatarSvg = (userId: string, isGuest = false) => {
+	const url = isDarkMode ? '/avatar/{userId}/32/dark' : '/avatar/{userId}/32'
+	const avatarUrl = generateUrl(isGuest ? url : url + '?guestFallback=true', { userId })
 	return `<svg width="32" height="32" viewBox="0 0 32 32"
 		xmlns="http://www.w3.org/2000/svg" class="sharing-status__avatar">
 		<image href="${avatarUrl}" height="32" width="32" />
 	</svg>`
 }
 
+const isExternal = (node: Node) => {
+	return node.attributes.remote_id !== undefined
+}
+
 export const action = new FileAction({
 	id: 'sharing-status',
 	displayName(nodes: Node[]) {
@@ -33,7 +41,7 @@ export const action = new FileAction({
 		const ownerId = node?.attributes?.['owner-id']
 
 		if (shareTypes.length > 0
-			|| (ownerId && ownerId !== getCurrentUser()?.uid)) {
+			|| (ownerId !== getCurrentUser()?.uid || isExternal(node))) {
 			return t('files_sharing', 'Shared')
 		}
 
@@ -46,11 +54,11 @@ export const action = new FileAction({
 		const ownerDisplayName = node?.attributes?.['owner-display-name']
 
 		// Mixed share types
-		if (Array.isArray(node.attributes?.['share-types'])) {
+		if (Array.isArray(node.attributes?.['share-types']) && node.attributes?.['share-types'].length > 1) {
 			return t('files_sharing', 'Shared multiple times with different people')
 		}
 
-		if (ownerId && ownerId !== getCurrentUser()?.uid) {
+		if (ownerId && (ownerId !== getCurrentUser()?.uid || isExternal(node))) {
 			return t('files_sharing', 'Shared by {ownerDisplayName}', { ownerDisplayName })
 		}
 
@@ -62,7 +70,7 @@ export const action = new FileAction({
 		const shareTypes = Object.values(node?.attributes?.['share-types'] || {}).flat() as number[]
 
 		// Mixed share types
-		if (Array.isArray(node.attributes?.['share-types'])) {
+		if (Array.isArray(node.attributes?.['share-types']) && node.attributes?.['share-types'].length > 1) {
 			return AccountPlusSvg
 		}
 
@@ -84,8 +92,8 @@ export const action = new FileAction({
 		}
 
 		const ownerId = node?.attributes?.['owner-id']
-		if (ownerId && ownerId !== getCurrentUser()?.uid) {
-			return generateAvatarSvg(ownerId)
+		if (ownerId && (ownerId !== getCurrentUser()?.uid || isExternal(node))) {
+			return generateAvatarSvg(ownerId, isExternal(node))
 		}
 
 		return AccountPlusSvg
@@ -107,7 +115,7 @@ export const action = new FileAction({
 		}
 
 		// If the node is shared by someone else
-		if (ownerId && ownerId !== getCurrentUser()?.uid) {
+		if (ownerId && (ownerId !== getCurrentUser()?.uid || isExternal(node))) {
 			return true
 		}
 

apps/files_sharing/src/services/SharingService.spec.ts

@@ -336,12 +336,27 @@ describe('SharingService share to Node mapping', () => {
 		expect(folder.attributes.favorite).toBe(1)
 	})
 
+	test('Empty', async () => {
+		jest.spyOn(logger, 'error').mockImplementationOnce(() => {})
+		jest.spyOn(axios, 'get').mockReturnValueOnce(Promise.resolve({
+			data: {
+				ocs: {
+					data: [],
+				},
+			},
+		}))
+
+		const shares = await getContents(false, true, false, false)
+		expect(shares.contents).toHaveLength(0)
+		expect(logger.error).toHaveBeenCalledTimes(0)
+	})
+
 	test('Error', async () => {
 		jest.spyOn(logger, 'error').mockImplementationOnce(() => {})
 		jest.spyOn(axios, 'get').mockReturnValueOnce(Promise.resolve({
 			data: {
 				ocs: {
-					data: [{}],
+					data: [null],
 				},
 			},
 		}))