Delete file shares through attachments API

Previously the file was deleted in the file structure of the user is not expected as the file might not only be related to the card. Signed-off-by: Julius Härtl <jus@bitgrid.net>
nextcloud · Sep 8, 2021 · ebbe4eb · ebbe4eb
1 parent 16cbbd4
commit ebbe4eb
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 52 deletions.
diff --git a/docs/API.md b/docs/API.md
@@ -959,6 +959,7 @@ For now only `deck_file` is supported as an attachment type.
 
 ### DELETE /boards/{boardId}/stacks/{stackId}/cards/{cardId}/attachments/{attachmentId} - Delete an attachment
 
+
 #### Request parameters
 
 | Parameter    | Type    | Description                                   |

diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php
@@ -35,6 +35,7 @@
 use OCA\Deck\NoPermissionException;
 use OCA\Deck\NotFoundException;
 use OCA\Deck\StatusException;
+use OCP\AppFramework\Db\IMapperException;
 use OCP\AppFramework\Http\Response;
 use OCP\ICache;
 use OCP\ICacheFactory;
@@ -320,14 +321,10 @@ public function update($cardId, $attachmentId, $data, $type = 'deck_file') {
 	 * Either mark an attachment as deleted for later removal or just remove it depending
 	 * on the IAttachmentService implementation
 	 *
-	 * @param $attachmentId
-	 * @return \OCP\AppFramework\Db\Entity
-	 * @throws \OCA\Deck\NoPermissionException
-	 * @throws \OCP\AppFramework\Db\DoesNotExistException
-	 * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
-	 * @throws BadRequestException
+	 * @throws NoPermissionException
+	 * @throws NotFoundException
 	 */
-	public function delete($cardId, $attachmentId, $type = 'deck_file') {
+	public function delete(int $cardId, int $attachmentId, string $type = 'deck_file'): Attachment {
 		try {
 			$service = $this->getService($type);
 		} catch (InvalidAttachmentType $e) {
@@ -340,40 +337,32 @@ public function delete($cardId, $attachmentId, $type = 'deck_file') {
 			$attachment->setType($type);
 			$attachment->setCardId($cardId);
 			$service->extendData($attachment);
-			$service->delete($attachment);
-			$this->changeHelper->cardChanged($attachment->getCardId());
-			$this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $attachment, ActivityManager::SUBJECT_ATTACHMENT_DELETE);
-			return $attachment;
-		}
-
-		try {
-			$attachment = $this->attachmentMapper->find($attachmentId);
-		} catch (\Exception $e) {
-			throw new NoPermissionException('Permission denied');
+		} else {
+			try {
+				$attachment = $this->attachmentMapper->find($attachmentId);
+			} catch (IMapperException $e) {
+				throw new NoPermissionException('Permission denied');
+			}
 		}
-
 		$this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT);
-		$this->cache->clear('card-' . $attachment->getCardId());
 
 		if ($service->allowUndo()) {
 			$service->markAsDeleted($attachment);
-			$this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $attachment, ActivityManager::SUBJECT_ATTACHMENT_DELETE);
-			$this->changeHelper->cardChanged($attachment->getCardId());
-			return $this->attachmentMapper->update($attachment);
+			$attachment = $this->attachmentMapper->update($attachment);
+		} else {
+			$service->delete($attachment);
+			if (!$service instanceof ICustomAttachmentService) {
+				$attachment = $this->attachmentMapper->delete($attachment);
+			}
 		}
-		$service->delete($attachment);
 
-		$attachment = $this->attachmentMapper->delete($attachment);
+		$this->cache->clear('card-' . $attachment->getCardId());
 		$this->changeHelper->cardChanged($attachment->getCardId());
 		$this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $attachment, ActivityManager::SUBJECT_ATTACHMENT_DELETE);
 		return $attachment;
 	}
 
-	public function restore($cardId, $attachmentId, $type = 'deck_file') {
-		if (is_numeric($attachmentId) === false) {
-			throw new BadRequestException('attachment id must be a number');
-		}
-
+	public function restore(int $cardId, int $attachmentId, string $type = 'deck_file'): Attachment {
 		try {
 			$attachment = $this->attachmentMapper->find($attachmentId);
 		} catch (\Exception $e) {

diff --git a/lib/Service/FilesAppService.php b/lib/Service/FilesAppService.php
@@ -23,7 +23,10 @@
 
 namespace OCA\Deck\Service;
 
+use OCA\Deck\Db\Acl;
 use OCA\Deck\Db\Attachment;
+use OCA\Deck\Db\CardMapper;
+use OCA\Deck\NoPermissionException;
 use OCA\Deck\Sharing\DeckShareProvider;
 use OCA\Deck\StatusException;
 use OCP\AppFramework\Http\StreamResponse;
@@ -38,6 +41,7 @@
 use OCP\Share\Exceptions\ShareNotFound;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
+use Psr\Log\LoggerInterface;
 
 class FilesAppService implements IAttachmentService, ICustomAttachmentService {
 	private $request;
@@ -48,8 +52,10 @@ class FilesAppService implements IAttachmentService, ICustomAttachmentService {
 	private $configService;
 	private $l10n;
 	private $preview;
-	private $permissionService;
 	private $mimeTypeDetector;
+	private $permissionService;
+	private $cardMapper;
+	private $logger;
 
 	public function __construct(
 		IRequest $request,
@@ -59,8 +65,10 @@ public function __construct(
 		ConfigService $configService,
 		DeckShareProvider $shareProvider,
 		IPreview $preview,
-		PermissionService $permissionService,
 		IMimeTypeDetector $mimeTypeDetector,
+		PermissionService $permissionService,
+		CardMapper  $cardMapper,
+		LoggerInterface $logger,
 		string $userId = null
 	) {
 		$this->request = $request;
@@ -72,15 +80,20 @@ public function __construct(
 		$this->userId = $userId;
 		$this->preview = $preview;
 		$this->mimeTypeDetector = $mimeTypeDetector;
+		$this->permissionService = $permissionService;
+		$this->cardMapper = $cardMapper;
+		$this->logger = $logger;
 	}
 
 	public function listAttachments(int $cardId): array {
 		$shares = $this->shareProvider->getSharedWithByType($cardId, IShare::TYPE_DECK, -1, 0);
-		$shares = array_filter($shares, function ($share) {
-			return $share->getPermissions() > 0;
-		});
-		return array_map(function (IShare $share) use ($cardId) {
-			$file = $share->getNode();
+		return array_filter(array_map(function (IShare $share) use ($cardId) {
+			try {
+				$file = $share->getNode();
+			} catch (NotFoundException $e) {
+				$this->logger->debug('Unable to find node for share with ID ' . $share->getId());
+				return null;
+			}
 			$attachment = new Attachment();
 			$attachment->setType('file');
 			$attachment->setId((int)$share->getId());
@@ -89,9 +102,9 @@ public function listAttachments(int $cardId): array {
 			$attachment->setData($file->getName());
 			$attachment->setLastModified($file->getMTime());
 			$attachment->setCreatedAt($share->getShareTime()->getTimestamp());
-			$attachment->setDeletedAt(0);
+			$attachment->setDeletedAt($share->getPermissions() === 0 ? $share->getShareTime()->getTimestamp() : 0);
 			return $attachment;
-		}, $shares);
+		}, $shares));
 	}
 
 	public function getAttachmentCount(int $cardId): int {
@@ -144,6 +157,7 @@ public function extendData(Attachment $attachment) {
 	}
 
 	public function display(Attachment $attachment) {
+		// Problem: Folders
 		/** @psalm-suppress InvalidCatch */
 		try {
 			$share = $this->shareProvider->getShareById($attachment->getId());
@@ -165,6 +179,9 @@ public function create(Attachment $attachment) {
 		$file = $this->getUploadedFile();
 		$fileName = $file['name'];
 
+		// get shares for current card
+		// check if similar filename already exists
+
 		$userFolder = $this->rootFolder->getUserFolder($this->userId);
 		try {
 			$folder = $userFolder->get($this->configService->getAttachmentFolder());
@@ -245,12 +262,16 @@ public function delete(Attachment $attachment) {
 		$file = $share->getNode();
 		$attachment->setData($file->getName());
 
-		if ($file->getOwner() !== null && $file->getOwner()->getUID() === $this->userId) {
-			$file->delete();
+		// Deleting a Nextcloud file attachment will remove the share to the card, keeping the source file untouched
+		// Opt-out of individual shares per user is no longer performed within deck but can still be done through the files app
+		$canEdit = $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT);
+		$isFileOwner = $file->getOwner() !== null && $file->getOwner()->getUID() === $this->userId;
+		if ($isFileOwner || $canEdit) {
+			$this->shareManager->deleteShare($share);
 			return;
 		}
 
-		$this->shareManager->deleteFromSelf($share, $this->userId);
+		throw new NoPermissionException('No permission to remove the attachment from the card');
 	}
 
 	public function allowUndo() {

diff --git a/src/components/card/AttachmentList.vue b/src/components/card/AttachmentList.vue
@@ -22,7 +22,7 @@
 
 <template>
 	<AttachmentDragAndDrop :card-id="cardId" class="drop-upload--sidebar">
-		<div class="button-group">
+		<div class="button-group" v-if="!isReadOnly">
 			<button class="icon-upload" @click="uploadNewFile()">
 				{{ t('deck', 'Upload new files') }}
 			</button>
@@ -49,31 +49,38 @@
 			</li>
 			<li v-for="attachment in attachments"
 				:key="attachment.id"
-				class="attachment">
+				class="attachment"
+				:class="{ 'attachment--deleted': attachment.deletedAt > 0 }">
 				<a class="fileicon"
+					:href="internalLink(attachment)"
 					:style="mimetypeForAttachment(attachment)"
 					@click.prevent="showViewer(attachment)" />
 				<div class="details">
-					<a @click.prevent="showViewer(attachment)">
+					<a :href="internalLink(attachment)" @click.prevent="showViewer(attachment)">
 						<div class="filename">
 							<span class="basename">{{ attachment.data }}</span>
 						</div>
-						<span class="filesize">{{ formattedFileSize(attachment.extendedData.filesize) }}</span>
-						<span class="filedate">{{ relativeDate(attachment.createdAt*1000) }}</span>
-						<span class="filedate">{{ attachment.createdBy }}</span>
+						<div v-if="attachment.deletedAt === 0">
+							<span class="filesize">{{ formattedFileSize(attachment.extendedData.filesize) }}</span>
+							<span class="filedate">{{ relativeDate(attachment.createdAt*1000) }}</span>
+							<span class="filedate">{{ attachment.createdBy }}</span>
+						</div>
+						<div v-else>
+							<span class="attachment--info">{{ t('deck', 'Pending share') }}</span>
+						</div>
 					</a>
 				</div>
 				<Actions v-if="selectable">
 					<ActionButton icon="icon-confirm" @click="$emit('selectAttachment', attachment)">
 						{{ t('deck', 'Add this attachment') }}
 					</ActionButton>
 				</Actions>
-				<Actions v-if="removable" :force-menu="true">
+				<Actions v-if="removable && !isReadOnly" :force-menu="true">
 					<ActionLink v-if="attachment.extendedData.fileid" icon="icon-folder" :href="internalLink(attachment)">
 						{{ t('deck', 'Show in Files') }}
 					</ActionLink>
-					<ActionButton v-if="attachment.extendedData.fileid" icon="icon-delete" @click="unshareAttachment(attachment)">
-						{{ t('deck', 'Unshare file') }}
+					<ActionButton v-if="attachment.extendedData.fileid && !isReadOnly" icon="icon-delete" @click="unshareAttachment(attachment)">
+						{{ t('deck', 'Remove attachment') }}
 					</ActionButton>
 
 					<ActionButton v-if="!attachment.extendedData.fileid && attachment.deletedAt === 0" icon="icon-delete" @click="$emit('deleteAttachment', attachment)">
@@ -143,6 +150,7 @@ export default {
 	},
 	computed: {
 		attachments() {
+			// FIXME sort propertly by last modified / deleted at
 			return [...this.$store.getters.attachmentsByCard(this.cardId)].filter(attachment => attachment.deletedAt >= 0).sort((a, b) => b.id - a.id)
 		},
 		mimetypeForAttachment() {
@@ -320,9 +328,10 @@ export default {
 					opacity: 0.7;
 				}
 			}
+			.attachment--info,
 			.filesize, .filedate {
 				font-size: 90%;
-				color: darkgray;
+				color: var(--color-text-maxcontrast);
 			}
 			.app-popover-menu-utils {
 				position: relative;

diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<files psalm-version="4.8.1@f73f2299dbc59a3e6c4d66cff4605176e728ee69">
+<files psalm-version="4.7.3@38c452ae584467e939d55377aaf83b5a26f19dd1">
   <file src="lib/Activity/ActivityManager.php">
     <TypeDoesNotContainType occurrences="1">
       <code>$message !== null</code>
@@ -189,6 +189,9 @@
       <code>$cardId</code>
     </InvalidScalarArgument>
   </file>
+  <file src="lib/Service/AttachmentService.php">
+    <InvalidCatch occurrences="1"/>
+  </file>
   <file src="lib/Service/BoardService.php">
     <TooManyArguments occurrences="2">
       <code>findAll</code>
Original file line number	Diff line number	Diff line change
Expand Up		@@ -959,6 +959,7 @@ For now only `deck_file` is supported as an attachment type.

		### DELETE /boards/{boardId}/stacks/{stackId}/cards/{cardId}/attachments/{attachmentId} - Delete an attachment


		#### Request parameters

		\| Parameter \| Type \| Description \|
Expand Down