Merge pull request 'feat: document delete permissions + three-dots menu portal fix' (#2) from feat/document-delete-permissions into main

Reviewed-on: #2

backend/CLAUDE.md

@@ -215,7 +215,7 @@ Auth: is_superuser OR member of group listed in manifest `required_groups`. Retu
 
 ### Documents and Categories — proxied
 
-`/api/documents/*` and `/api/documents/categories/*` are transparently proxied to `doc-service:8001`. The backend injects `x-user-id` and `x-user-groups` headers. See `features/doc-service/CLAUDE.md` for the internal endpoint list.
+`/api/documents/*` and `/api/documents/categories/*` are transparently proxied to `doc-service:8001`. The backend injects `x-user-id`, `x-user-groups`, and `x-user-is-admin` headers. See `features/doc-service/CLAUDE.md` for the internal endpoint list.
 
 ---
 

backend/app/routers/documents_proxy.py

@@ -50,13 +50,16 @@ _HOP_BY_HOP = frozenset([
 _STRIP_RESPONSE = frozenset([*_HOP_BY_HOP, "content-length", "content-type"])
 
 
-async def _forward_headers(request: Request, user_id: str, db: AsyncSession) -> dict:
+async def _forward_headers(
+    request: Request, user_id: str, is_admin: bool, db: AsyncSession
+) -> dict:
     headers = {
         k: v
         for k, v in request.headers.items()
         if k.lower() not in _HOP_BY_HOP
     }
     headers["x-user-id"] = user_id
+    headers["x-user-is-admin"] = "true" if is_admin else "false"
 
     # Inject the user's group memberships so the doc-service can evaluate
     # group-shared document access without querying the backend DB.
@@ -78,7 +81,7 @@ async def proxy_documents(
     path: str = "",
 ) -> Response:
     url = f"/documents/{path}" if path else "/documents"
-    headers = await _forward_headers(request, str(current_user.id), db)
+    headers = await _forward_headers(request, str(current_user.id), current_user.is_superuser, db)
     body = await request.body()
 
     try:

changelog/2026-04-18_document-delete-permissions.md

@@ -0,0 +1,25 @@
+# 2026-04-18 — Document delete permissions + three-dots menu fix
+
+**Timestamp:** 2026-04-18T00:00:00Z
+
+## Summary
+
+Added a proper permission model for document deletion: owners and admins can always delete; group members can delete only when the share was explicitly granted `can_delete=true`. Fixed silent delete failures (watch docs returning 404 with no user feedback) and fixed the three-dots context menu being clipped by `overflow-hidden` on the table container.
+
+## Files Added / Modified / Deleted
+
+### Added
+- `features/doc-service/alembic/versions/0005_add_share_can_delete.py` — migration: adds `can_delete BOOLEAN NOT NULL DEFAULT false` to `document_shares`
+
+### Modified
+- `features/doc-service/app/models/document_share.py` — added `can_delete: Mapped[bool]` column
+- `features/doc-service/app/schemas/share.py` — added `can_delete` to `DocumentShareOut` and `DocumentShareCreate`; added `viewer_can_delete` to `SharedDocumentOut`
+- `features/doc-service/app/schemas/document.py` — added `viewer_can_delete: bool = False` to `DocumentOut`
+- `features/doc-service/app/deps.py` — added `get_user_is_admin()` dep reading `x-user-is-admin` header
+- `features/doc-service/app/routers/documents.py` — added `_get_deletable_doc_ids()` helper; updated list/get/delete endpoints with permission logic; updated `add_share` to store `can_delete`; updated shared-with-me to include `viewer_can_delete`
+- `backend/app/routers/documents_proxy.py` — `_forward_headers()` now injects `x-user-is-admin` header
+- `frontend/src/api/client.ts` — `DocumentOut`: added `viewer_can_delete`; `DocumentShareOut`: added `can_delete`; `addDocumentShare`: accepts `canDelete` param
+- `frontend/src/pages/DocumentsPage.tsx` — `RowActionsMenu`: replaced absolute dropdown with `createPortal` to fix clipping; delete button now uses `doc.viewer_can_delete`; added `onError` handler for silent failures
+- `frontend/src/components/DocumentSlideOver.tsx` — sharing section: shows trash icon badge on shares with `can_delete=true`; added "Allow group members to delete" checkbox before group picker; delete button uses `doc.viewer_can_delete`
+- `features/doc-service/CLAUDE.md` — updated `document_shares` table docs + migration chain
+- `backend/CLAUDE.md` — noted `x-user-is-admin` header injection

features/doc-service/CLAUDE.md

@@ -100,6 +100,7 @@ features/doc-service/
 | `document_id` | String | indexed, NOT NULL | not FK — trusts proxy |
 | `group_id` | String | indexed, NOT NULL | group from backend |
 | `shared_by_user_id` | String | NOT NULL | owner who shared |
+| `can_delete` | Boolean | NOT NULL, default=false | whether group members may delete the doc |
 | `created_at` | DateTime(tz) | server_default=now() | |
 
 Unique constraint: `(document_id, group_id)`
@@ -112,6 +113,7 @@ Unique constraint: `(document_id, group_id)`
 | `0002` | `add_document_title` |
 | `0003` | `add_watch_columns` |
 | `0004` | `add_document_shares` |
+| `0005` | `add_share_can_delete` |
 
 ---
 

features/doc-service/alembic/versions/0005_add_share_can_delete.py

@@ -0,0 +1,32 @@
+"""add can_delete to document_shares
+
+Revision ID: 0005
+Revises: 0004
+Create Date: 2026-04-18
+
+"""
+from typing import Sequence, Union
+
+import sqlalchemy as sa
+from alembic import op
+
+revision: str = "0005"
+down_revision: Union[str, None] = "0004"
+branch_labels: Union[str, Sequence[str], None] = None
+depends_on: Union[str, Sequence[str], None] = None
+
+
+def upgrade() -> None:
+    op.add_column(
+        "document_shares",
+        sa.Column(
+            "can_delete",
+            sa.Boolean(),
+            nullable=False,
+            server_default=sa.text("false"),
+        ),
+    )
+
+
+def downgrade() -> None:
+    op.drop_column("document_shares", "can_delete")

features/doc-service/app/deps.py

@@ -21,3 +21,11 @@ async def get_user_groups(x_user_groups: str = Header(default="")) -> list[str]:
     if not x_user_groups:
         return []
     return [g.strip() for g in x_user_groups.split(",") if g.strip()]
+
+
+async def get_user_is_admin(x_user_is_admin: str = Header(default="false")) -> bool:
+    """
+    Extract the admin flag injected by the main backend proxy.
+    Returns True only if the header value is exactly "true" (lowercase).
+    """
+    return x_user_is_admin.lower() == "true"

features/doc-service/app/models/document_share.py

@@ -1,7 +1,7 @@
 import uuid
 from datetime import datetime
 
-from sqlalchemy import DateTime, String, UniqueConstraint, func
+from sqlalchemy import Boolean, DateTime, String, UniqueConstraint, func
 from sqlalchemy.orm import Mapped, mapped_column
 
 from app.database import Base
@@ -14,6 +14,7 @@ class DocumentShare(Base):
     document_id: Mapped[str] = mapped_column(String, nullable=False, index=True)
     group_id: Mapped[str] = mapped_column(String, nullable=False, index=True)
     shared_by_user_id: Mapped[str] = mapped_column(String, nullable=False)
+    can_delete: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False, server_default="false")
     created_at: Mapped[datetime] = mapped_column(
         DateTime(timezone=True), server_default=func.now(), nullable=False
     )

features/doc-service/app/routers/documents.py

@@ -13,7 +13,7 @@ from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.orm import selectinload
 
 from app.database import AsyncSessionLocal, get_db
-from app.deps import get_user_groups, get_user_id
+from app.deps import get_user_groups, get_user_id, get_user_is_admin
 from app.models.category import DocumentCategory
 from app.models.category_assignment import CategoryAssignment
 from app.models.document import Document
@@ -73,7 +73,26 @@ async def _get_share_counts(doc_ids: list[str], db: AsyncSession) -> dict[str, i
     return {row[0]: row[1] for row in rows.all()}
 
 
-def _doc_with_categories(doc: Document, share_count: int = 0) -> DocumentOut:
+async def _get_deletable_doc_ids(
+    doc_ids: list[str], user_groups: list[str], db: AsyncSession
+) -> set[str]:
+    """Return doc IDs for which the user has delete permission via a group share."""
+    if not doc_ids or not user_groups:
+        return set()
+    rows = await db.execute(
+        select(DocumentShare.document_id)
+        .where(
+            DocumentShare.document_id.in_(doc_ids),
+            DocumentShare.group_id.in_(user_groups),
+            DocumentShare.can_delete.is_(True),
+        )
+    )
+    return {row[0] for row in rows.all()}
+
+
+def _doc_with_categories(
+    doc: Document, share_count: int = 0, viewer_can_delete: bool = False
+) -> DocumentOut:
     from app.schemas.document import CategoryOut
     cats = [CategoryOut(id=a.category.id, name=a.category.name) for a in doc.category_assignments]
     return DocumentOut(
@@ -95,6 +114,7 @@ def _doc_with_categories(doc: Document, share_count: int = 0) -> DocumentOut:
         suggested_folder=doc.suggested_folder,
         suggested_filename=doc.suggested_filename,
         share_count=share_count,
+        viewer_can_delete=viewer_can_delete,
     )
 
 
@@ -209,6 +229,8 @@ async def list_documents(
     search: str | None = Query(default=None),
     category_id: str | None = Query(default=None),
     user_id: str = Depends(get_user_id),
+    user_groups: list[str] = Depends(get_user_groups),
+    is_admin: bool = Depends(get_user_is_admin),
     db: AsyncSession = Depends(get_db),
 ) -> DocumentPage:
     sort_col = _SORT_COLUMNS.get(sort, Document.created_at)
@@ -254,8 +276,19 @@ async def list_documents(
     )
     docs = items_result.scalars().all()
 
-    share_counts = await _get_share_counts([d.id for d in docs], db)
-    items = [_doc_with_categories(d, share_counts.get(d.id, 0)) for d in docs]
+    doc_ids = [d.id for d in docs]
+    share_counts = await _get_share_counts(doc_ids, db)
+
+    if is_admin:
+        deletable_ids = set(doc_ids)
+    else:
+        deletable_ids = {d.id for d in docs if d.user_id == user_id}
+        deletable_ids |= await _get_deletable_doc_ids(doc_ids, user_groups, db)
+
+    items = [
+        _doc_with_categories(d, share_counts.get(d.id, 0), viewer_can_delete=d.id in deletable_ids)
+        for d in docs
+    ]
 
     return DocumentPage(
         items=items,
@@ -367,6 +400,7 @@ async def list_shared_with_me(
                 source=doc.source,
                 shared_by_user_id=share.shared_by_user_id if share else "",
                 shared_via_group_id=share.group_id if share else "",
+                viewer_can_delete=bool(share and share.can_delete),
             )
         )
 
@@ -382,11 +416,19 @@ async def list_shared_with_me(
 async def get_document(
     doc_id: str,
     user_id: str = Depends(get_user_id),
+    user_groups: list[str] = Depends(get_user_groups),
+    is_admin: bool = Depends(get_user_is_admin),
     db: AsyncSession = Depends(get_db),
 ) -> DocumentOut:
     doc = await _get_user_doc(doc_id, user_id, db)
     counts = await _get_share_counts([doc.id], db)
-    return _doc_with_categories(doc, counts.get(doc.id, 0))
+    if is_admin:
+        viewer_can_delete = True
+    elif doc.user_id == user_id:
+        viewer_can_delete = True
+    else:
+        viewer_can_delete = bool(await _get_deletable_doc_ids([doc.id], user_groups, db))
+    return _doc_with_categories(doc, counts.get(doc.id, 0), viewer_can_delete=viewer_can_delete)
 
 
 @router.get("/{doc_id}/status", response_model=DocumentStatusOut)
@@ -479,14 +521,26 @@ async def reprocess_document(
 async def delete_document(
     doc_id: str,
     user_id: str = Depends(get_user_id),
+    user_groups: list[str] = Depends(get_user_groups),
+    is_admin: bool = Depends(get_user_is_admin),
     db: AsyncSession = Depends(get_db),
 ) -> None:
     result = await db.execute(
-        select(Document).where(Document.id == doc_id, Document.user_id == user_id)
+        select(Document).where(
+            Document.id == doc_id,
+            or_(Document.user_id == user_id, Document.user_id == _WATCH_USER_ID),
+        )
     )
     doc = result.scalar_one_or_none()
     if doc is None:
         raise HTTPException(status_code=404, detail="Document not found")
+
+    is_owner = doc.user_id == user_id
+    if not is_owner and not is_admin:
+        can_delete_via_group = bool(await _get_deletable_doc_ids([doc_id], user_groups, db))
+        if not can_delete_via_group:
+            raise HTTPException(status_code=403, detail="Delete not permitted")
+
     delete_file(doc.file_path)
     await db.delete(doc)
     await db.commit()
@@ -718,6 +772,7 @@ async def add_document_share(
         document_id=doc_id,
         group_id=body.group_id,
         shared_by_user_id=user_id,
+        can_delete=body.can_delete,
     )
     db.add(share)
     await db.commit()

features/doc-service/app/schemas/document.py

@@ -28,6 +28,7 @@ class DocumentOut(BaseModel):
     suggested_folder: str | None = None
     suggested_filename: str | None = None
     share_count: int = 0
+    viewer_can_delete: bool = False
 
     model_config = {"from_attributes": True}
 

features/doc-service/app/schemas/share.py

@@ -8,6 +8,7 @@ class DocumentShareOut(BaseModel):
     document_id: str
     group_id: str
     shared_by_user_id: str
+    can_delete: bool
     created_at: datetime
 
     model_config = {"from_attributes": True}
@@ -15,6 +16,7 @@ class DocumentShareOut(BaseModel):
 
 class DocumentShareCreate(BaseModel):
     group_id: str
+    can_delete: bool = False
 
 
 class SharedDocumentOut(BaseModel):
@@ -34,6 +36,7 @@ class SharedDocumentOut(BaseModel):
     categories: list = []
     source: str = "upload"
     share_count: int = 0
+    viewer_can_delete: bool = False
     # Sharing context
     shared_by_user_id: str
     shared_via_group_id: str

frontend/src/api/client.ts

@@ -220,6 +220,7 @@ export interface DocumentOut {
   suggested_folder: string | null;
   suggested_filename: string | null;
   share_count: number;
+  viewer_can_delete: boolean;
 }
 
 export interface SharedDocumentOut extends DocumentOut {
@@ -232,6 +233,7 @@ export interface DocumentShareOut {
   document_id: string;
   group_id: string;
   shared_by_user_id: string;
+  can_delete: boolean;
   created_at: string;
 }
 
@@ -270,8 +272,8 @@ export const listSharedWithMe = (params: DocumentListParams = {}) =>
 export const getDocumentShares = (docId: string) =>
   api.get<DocumentShareOut[]>(`/documents/${docId}/shares`);
 
-export const addDocumentShare = (docId: string, groupId: string) =>
-  api.post<DocumentShareOut>(`/documents/${docId}/shares`, { group_id: groupId });
+export const addDocumentShare = (docId: string, groupId: string, canDelete = false) =>
+  api.post<DocumentShareOut>(`/documents/${docId}/shares`, { group_id: groupId, can_delete: canDelete });
 
 export const removeDocumentShare = (docId: string, groupId: string) =>
   api.delete(`/documents/${docId}/shares/${groupId}`);

frontend/src/components/DocumentSlideOver.tsx

@@ -186,6 +186,7 @@ export default function DocumentSlideOver({ doc, isOwner, onClose, onDeleted }:
   const [titleValue, setTitleValue] = useState("");
   const [editingType, setEditingType] = useState(false);
   const [tagInput, setTagInput] = useState("");
+  const [canDeleteNew, setCanDeleteNew] = useState(false);
   const titleInputRef = useRef<HTMLInputElement>(null);
 
   useEffect(() => {
@@ -279,8 +280,12 @@ export default function DocumentSlideOver({ doc, isOwner, onClose, onDeleted }:
   });
 
   const addShareMut = useMutation({
-    mutationFn: (groupId: string) => addDocumentShare(doc!.id, groupId),
-    onSuccess: () => queryClient.invalidateQueries({ queryKey: ["document-shares", doc!.id] }),
+    mutationFn: ({ groupId, canDelete }: { groupId: string; canDelete: boolean }) =>
+      addDocumentShare(doc!.id, groupId, canDelete),
+    onSuccess: () => {
+      queryClient.invalidateQueries({ queryKey: ["document-shares", doc!.id] });
+      setCanDeleteNew(false);
+    },
   });
 
   const removeShareMut = useMutation({
@@ -694,6 +699,11 @@ export default function DocumentSlideOver({ doc, isOwner, onClose, onDeleted }:
                     <div key={share.id} className="flex items-center gap-2 text-sm">
                       <Users className="h-3.5 w-3.5 text-muted shrink-0" />
                       <span className="flex-1 text-sm">{group?.name ?? share.group_id}</span>
+                      {share.can_delete && (
+                        <span title="Group members can delete this document">
+                          <Trash2 className="h-3 w-3 text-muted shrink-0" />
+                        </span>
+                      )}
                       <button
                         onClick={() => removeShareMut.mutate(share.group_id)}
                         disabled={removeShareMut.isPending}
@@ -706,41 +716,54 @@ export default function DocumentSlideOver({ doc, isOwner, onClose, onDeleted }:
                   );
                 })}
               </div>
+              <label className="flex items-center gap-1.5 text-xs text-muted cursor-pointer mt-1 mb-0.5 select-none">
+                <input
+                  type="checkbox"
+                  checked={canDeleteNew}
+                  onChange={(e) => setCanDeleteNew(e.target.checked)}
+                  className="h-3 w-3 accent-primary"
+                />
+                Allow group members to delete
+              </label>
               <GroupCombobox
                 groups={myGroups}
                 sharedGroupIds={sharedGroupIds}
-                onShare={(id) => addShareMut.mutate(id)}
+                onShare={(id) => addShareMut.mutate({ groupId: id, canDelete: canDeleteNew })}
               />
             </div>
           )}
 
-          {/* Owner actions */}
-          {isOwner && (
+          {/* Owner/permitted actions */}
+          {(isOwner || doc.viewer_can_delete) && (
             <div className="flex gap-2 pt-1">
-              <Button
-                variant="outline"
-                size="sm"
-                onClick={() => reprocessMut.mutate()}
-                disabled={reprocessMut.isPending || doc.status === "pending" || doc.status === "processing"}
-                className="gap-1.5"
-              >
-                <RefreshCw className={cn("h-3.5 w-3.5", reprocessMut.isPending && "animate-spin")} />
-                Re-analyse
-              </Button>
-              <Button
-                variant="outline"
-                size="sm"
-                onClick={() => {
-                  if (confirm(`Delete "${doc.title ?? doc.filename}"? This cannot be undone.`)) {
-                    deleteMut.mutate();
-                  }
-                }}
-                disabled={deleteMut.isPending}
-                className="gap-1.5 text-red-500 border-red-200 hover:bg-red-50 dark:hover:bg-red-900/20"
-              >
-                <Trash2 className="h-3.5 w-3.5" />
-                Delete
-              </Button>
+              {isOwner && (
+                <Button
+                  variant="outline"
+                  size="sm"
+                  onClick={() => reprocessMut.mutate()}
+                  disabled={reprocessMut.isPending || doc.status === "pending" || doc.status === "processing"}
+                  className="gap-1.5"
+                >
+                  <RefreshCw className={cn("h-3.5 w-3.5", reprocessMut.isPending && "animate-spin")} />
+                  Re-analyse
+                </Button>
+              )}
+              {doc.viewer_can_delete && (
+                <Button
+                  variant="outline"
+                  size="sm"
+                  onClick={() => {
+                    if (confirm(`Delete "${doc.title ?? doc.filename}"? This cannot be undone.`)) {
+                      deleteMut.mutate();
+                    }
+                  }}
+                  disabled={deleteMut.isPending}
+                  className="gap-1.5 text-red-500 border-red-200 hover:bg-red-50 dark:hover:bg-red-900/20"
+                >
+                  <Trash2 className="h-3.5 w-3.5" />
+                  Delete
+                </Button>
+              )}
             </div>
           )}
 

frontend/src/pages/DocumentsPage.tsx

@@ -1,4 +1,5 @@
 import { useState, useRef, useCallback, useEffect } from "react";
+import { createPortal } from "react-dom";
 import { useSearchParams } from "react-router-dom";
 import { useQuery, useMutation, useQueryClient } from "@tanstack/react-query";
 import {
@@ -47,43 +48,70 @@ function formatDate(iso: string) {
 // ── Row actions dropdown ──────────────────────────────────────────────────────
 
 function RowActionsMenu({
-  doc, isOwner, onSelect,
-}: { doc: DocumentOut; isOwner: boolean; onSelect: () => void }) {
+  doc, onSelect,
+}: { doc: DocumentOut; onSelect: () => void }) {
   const [open, setOpen] = useState(false);
-  const ref = useRef<HTMLDivElement>(null);
+  const [menuPos, setMenuPos] = useState({ top: 0, right: 0 });
+  const triggerRef = useRef<HTMLButtonElement>(null);
+  const menuRef = useRef<HTMLDivElement>(null);
   const queryClient = useQueryClient();
 
   useEffect(() => {
-    const handler = (e: MouseEvent) => {
-      if (ref.current && !ref.current.contains(e.target as Node)) setOpen(false);
+    const close = (e: MouseEvent) => {
+      if (
+        triggerRef.current && !triggerRef.current.contains(e.target as Node) &&
+        menuRef.current && !menuRef.current.contains(e.target as Node)
+      ) setOpen(false);
+    };
+    const closeOnScroll = () => setOpen(false);
+    document.addEventListener("mousedown", close);
+    window.addEventListener("scroll", closeOnScroll, true);
+    window.addEventListener("resize", closeOnScroll);
+    return () => {
+      document.removeEventListener("mousedown", close);
+      window.removeEventListener("scroll", closeOnScroll, true);
+      window.removeEventListener("resize", closeOnScroll);
     };
-    document.addEventListener("mousedown", handler);
-    return () => document.removeEventListener("mousedown", handler);
   }, []);
 
   const deleteMut = useMutation({
     mutationFn: () => deleteDocument(doc.id),
     onSuccess: () => queryClient.invalidateQueries({ queryKey: ["documents"] }),
+    onError: () => alert("Failed to delete document. Please try again."),
   });
 
+  const handleToggle = (e: React.MouseEvent) => {
+    e.stopPropagation();
+    if (!open && triggerRef.current) {
+      const rect = triggerRef.current.getBoundingClientRect();
+      setMenuPos({ top: rect.bottom + 4, right: window.innerWidth - rect.right });
+    }
+    setOpen((o) => !o);
+  };
+
   return (
-    <div className="relative" ref={ref} onClick={(e) => e.stopPropagation()}>
+    <div onClick={(e) => e.stopPropagation()}>
       <button
-        onClick={() => setOpen((o) => !o)}
+        ref={triggerRef}
+        onClick={handleToggle}
         className="p-1 rounded text-muted hover:text-foreground hover:bg-muted/20 transition-colors opacity-0 group-hover:opacity-100"
         title="Actions"
       >
         <MoreHorizontal className="h-4 w-4" />
       </button>
-      {open && (
-        <div className="absolute right-0 top-full mt-1 z-20 bg-surface border border-border rounded-lg shadow-lg w-36 py-1">
+      {open && createPortal(
+        <div
+          ref={menuRef}
+          style={{ position: "fixed", top: menuPos.top, right: menuPos.right, zIndex: 9999 }}
+          className="bg-surface border border-border rounded-lg shadow-lg w-36 py-1"
+        >
           <button
             className="w-full text-left px-3 py-1.5 text-sm hover:bg-muted/20 transition-colors"
             onClick={() => { onSelect(); setOpen(false); }}
           >
             Open details
           </button>
-          {isOwner && (
+          {doc.viewer_can_delete && (
             <button
               className="w-full text-left px-3 py-1.5 text-sm hover:bg-muted/20 transition-colors text-red-500"
               onClick={() => {
@@ -94,7 +122,8 @@ function RowActionsMenu({
               Delete
             </button>
           )}
-        </div>
+        </div>,
+        document.body
       )}
     </div>
   );
@@ -470,7 +499,7 @@ function DocumentRow({
       </td>
 
       <td className="px-3 py-2.5 w-8 text-right">
-        <RowActionsMenu doc={doc} isOwner={isOwner} onSelect={onOpen} />
+        <RowActionsMenu doc={doc} onSelect={onOpen} />
       </td>
     </tr>
   );