Merge pull request #1001 from Hexastack/fix/disallow-attachment-delete

fix: disalow attachment delete

api/src/attachment/controllers/attachment.controller.spec.ts

@@ -9,7 +9,10 @@
 import fs from 'fs';
 
 import { CACHE_MANAGER } from '@nestjs/cache-manager';
-import { BadRequestException } from '@nestjs/common/exceptions';
+import {
+  BadRequestException,
+  MethodNotAllowedException,
+} from '@nestjs/common/exceptions';
 import { NotFoundException } from '@nestjs/common/exceptions/not-found.exception';
 import { MongooseModule } from '@nestjs/mongoose';
 import { Request } from 'express';
@@ -215,29 +218,10 @@ describe('AttachmentController', () => {
   });
 
   describe('deleteOne', () => {
-    it('should delete an attachment by id', async () => {
-      jest.spyOn(attachmentService, 'deleteOne');
-      const result = await attachmentController.deleteOne(
-        attachmentToDelete.id,
-      );
-
-      expect(attachmentService.deleteOne).toHaveBeenCalledWith(
-        attachmentToDelete.id,
-      );
-      expect(result).toEqual({
-        acknowledged: true,
-        deletedCount: 1,
-      });
-    });
-
-    it('should throw a NotFoundException when attempting to delete an attachment by id', async () => {
+    it('should throw a MethodNotAllowedException when attempting to delete an attachment by id', async () => {
       await expect(
         attachmentController.deleteOne(attachmentToDelete.id),
-      ).rejects.toThrow(
-        new NotFoundException(
-          `Attachment with ID ${attachmentToDelete.id} not found`,
-        ),
-      );
+      ).rejects.toThrow(MethodNotAllowedException);
     });
   });
 });

api/src/attachment/controllers/attachment.controller.ts

@@ -13,6 +13,7 @@ import {
   ForbiddenException,
   Get,
   HttpCode,
+  MethodNotAllowedException,
   NotFoundException,
   Param,
   Post,
@@ -32,7 +33,6 @@ import { config } from '@/config';
 import { CsrfInterceptor } from '@/interceptors/csrf.interceptor';
 import { Roles } from '@/utils/decorators/roles.decorator';
 import { BaseController } from '@/utils/generics/base-controller';
-import { DeleteResult } from '@/utils/generics/base-repository';
 import { PageQueryDto } from '@/utils/pagination/pagination-query.dto';
 import { PageQueryPipe } from '@/utils/pagination/pagination-query.pipe';
 import { SearchFilterPipe } from '@/utils/pipes/search-filter.pipe';
@@ -185,20 +185,21 @@ export class AttachmentController extends BaseController<Attachment> {
   }
 
   /**
-   * Deletes an attachment with the specified ID.
+   * Deletion of attachments is disallowed to prevent database inconsistencies.
+   * Attachments may be referenced by blocks, messages, or content elements,
+   * and deleting them directly could lead to orphaned references or broken UI.
    *
-   * @param id - The ID of the attachment to delete.
-   * @returns A promise that resolves to the result of the deletion operation.
+   * @param id - The ID of the attachment (not used since deletion is not allowed).
+   * @throws MethodNotAllowedException - Always thrown to indicate deletion is not permitted.
    */
   @CsrfCheck(true)
   @Delete(':id')
-  @HttpCode(204)
-  async deleteOne(@Param('id') id: string): Promise<DeleteResult> {
-    const result = await this.attachmentService.deleteOne(id);
-    if (result.deletedCount === 0) {
-      this.logger.warn(`Unable to delete attachment by id ${id}`);
-      throw new NotFoundException(`Attachment with ID ${id} not found`);
-    }
-    return result;
+  @HttpCode(405)
+  async deleteOne(@Param('id') id: string): Promise<void> {
+    // Deletion is explicitly disallowed due to potential reference issues.
+    this.logger.warn(`Attempted deletion of attachment ${id} is not allowed.`);
+    throw new MethodNotAllowedException(
+      'Deletion of attachments is not permitted to avoid data inconsistency.',
+    );
   }
 }

frontend/src/app-components/attachment/AttachmentThumbnail.tsx

@@ -1,13 +1,12 @@
 /*
- * Copyright © 2024 Hexastack. All rights reserved.
+ * Copyright © 2025 Hexastack. All rights reserved.
  *
  * Licensed under the GNU Affero General Public License v3.0 (AGPLv3) with the following additional terms:
  * 1. The name "Hexabot" is a trademark of Hexastack. You may not use this name in derivative works without express written permission.
  * 2. All derivative works must include clear attribution to the original creator and software, Hexastack and Hexabot, in a prominent location (e.g., in the software's "About" section, documentation, and README file).
  */
 
-import CancelOutlinedIcon from "@mui/icons-material/CancelOutlined";
-import DeleteOutlineOutlinedIcon from "@mui/icons-material/DeleteOutlineOutlined";
+import ClearIcon from "@mui/icons-material/Clear";
 import FileOpenIcon from "@mui/icons-material/FileOpen";
 import MusicNoteIcon from "@mui/icons-material/MusicNote";
 import VideoCameraBackOutlinedIcon from "@mui/icons-material/VideoCameraBackOutlined";
@@ -21,18 +20,11 @@ import {
 } from "@mui/material";
 import { FC } from "react";
 
-import { useDelete } from "@/hooks/crud/useDelete";
 import { useGet } from "@/hooks/crud/useGet";
-import { useDialogs } from "@/hooks/useDialogs";
 import useFormattedFileSize from "@/hooks/useFormattedFileSize";
-import { useHasPermission } from "@/hooks/useHasPermission";
-import { useToast } from "@/hooks/useToast";
 import { useTranslate } from "@/hooks/useTranslate";
 import { EntityType } from "@/services/types";
 import { IAttachment } from "@/types/attachment.types";
-import { PermissionAction } from "@/types/permission.types";
-
-import { ConfirmDialogBody } from "../dialogs";
 
 const AttachmentPreview = ({
   attachment,
@@ -79,22 +71,10 @@ const AttachmentThumbnail: FC<AttachmentThumbnailProps> = ({
   onChange,
 }) => {
   const formatSize = useFormattedFileSize();
-  const hasPermission = useHasPermission();
   const { data: attachment } = useGet(id, {
     entity: EntityType.ATTACHMENT,
   });
-  const { toast } = useToast();
   const { t } = useTranslate();
-  const dialogs = useDialogs();
-  const { mutate: deleteAttachment } = useDelete(EntityType.ATTACHMENT, {
-    onError: () => {
-      toast.error(t("message.internal_server_error"));
-    },
-    onSuccess: () => {
-      toast.success(t("message.success_save"));
-      onChange && onChange(null);
-    },
-  });
 
   if (!attachment) {
     return t("message.attachment_not_found") + id;
@@ -117,45 +97,22 @@ const AttachmentThumbnail: FC<AttachmentThumbnailProps> = ({
             </Typography>
           </CardContent>
 
-          {format === "full" &&
-          hasPermission(EntityType.ATTACHMENT, PermissionAction.DELETE) &&
-          onChange ? (
-            <>
-              <CardActions sx={{ justifyContent: "center", flex: "1 1 50%" }}>
-                <Button
-                  color="primary"
-                  variant="contained"
-                  startIcon={<CancelOutlinedIcon />}
-                  onClick={(e) => {
-                    onChange && onChange(null);
-                    e.preventDefault();
-                    e.stopPropagation();
-                  }}
-                  size="small"
-                >
-                  {t("button.unselect")}
-                </Button>
-                <Button
-                  color="secondary"
-                  variant="contained"
-                  startIcon={<DeleteOutlineOutlinedIcon />}
-                  onClick={async (e) => {
-                    const isConfirmed = await dialogs.confirm(
-                      ConfirmDialogBody,
-                    );
-
-                    if (isConfirmed) {
-                      deleteAttachment(attachment.id);
-                    }
-                    e.preventDefault();
-                    e.stopPropagation();
-                  }}
-                  size="small"
-                >
-                  {t("button.remove")}
-                </Button>
-              </CardActions>
-            </>
+          {format === "full" && onChange ? (
+            <CardActions sx={{ justifyContent: "center", flex: "1 1 50%" }}>
+              <Button
+                color="inherit"
+                variant="contained"
+                startIcon={<ClearIcon />}
+                onClick={(e) => {
+                  onChange?.(null);
+                  e.preventDefault();
+                  e.stopPropagation();
+                }}
+                size="small"
+              >
+                {t("button.delete")}
+              </Button>
+            </CardActions>
           ) : null}
         </>
       ) : null}