From 6e5e5c08bfb5c2ed820f006a929c6bd22ccfe64a Mon Sep 17 00:00:00 2001 From: curo1305 Date: Sat, 18 Apr 2026 21:39:01 +0200 Subject: [PATCH] feat: document delete permissions + three-dots menu portal fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add can_delete column to document_shares (migration 0005) - Inject x-user-is-admin header from backend proxy to doc-service - Add get_user_is_admin() dep in doc-service - Delete endpoint now allows: owner, admin, or group member with can_delete=true - Watch documents (user_id='watch') deletable by admins only - DocumentOut gains viewer_can_delete (computed per-request) - Share UI: 'Allow group members to delete' checkbox + trash badge on shares - RowActionsMenu dropdown portaled to document.body — fixes overflow-hidden clipping - Delete mutation onError handler — no more silent failures Co-Authored-By: Claude Sonnet 4.6 --- backend/CLAUDE.md | 2 +- backend/app/routers/documents_proxy.py | 7 +- .../2026-04-18_document-delete-permissions.md | 25 ++++++ features/doc-service/CLAUDE.md | 2 + .../versions/0005_add_share_can_delete.py | 32 ++++++++ features/doc-service/app/deps.py | 8 ++ .../doc-service/app/models/document_share.py | 3 +- features/doc-service/app/routers/documents.py | 67 +++++++++++++-- features/doc-service/app/schemas/document.py | 1 + features/doc-service/app/schemas/share.py | 3 + frontend/src/api/client.ts | 6 +- frontend/src/components/DocumentSlideOver.tsx | 81 ++++++++++++------- frontend/src/pages/DocumentsPage.tsx | 57 +++++++++---- 13 files changed, 239 insertions(+), 55 deletions(-) create mode 100644 changelog/2026-04-18_document-delete-permissions.md create mode 100644 features/doc-service/alembic/versions/0005_add_share_can_delete.py diff --git a/backend/CLAUDE.md b/backend/CLAUDE.md index 4c85907..82e81df 100644 --- a/backend/CLAUDE.md +++ b/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. --- diff --git a/backend/app/routers/documents_proxy.py b/backend/app/routers/documents_proxy.py index 55bcc61..8f2752f 100644 --- a/backend/app/routers/documents_proxy.py +++ b/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: diff --git a/changelog/2026-04-18_document-delete-permissions.md b/changelog/2026-04-18_document-delete-permissions.md new file mode 100644 index 0000000..8edec8d --- /dev/null +++ b/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 diff --git a/features/doc-service/CLAUDE.md b/features/doc-service/CLAUDE.md index 3068298..25c09f1 100644 --- a/features/doc-service/CLAUDE.md +++ b/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` | --- diff --git a/features/doc-service/alembic/versions/0005_add_share_can_delete.py b/features/doc-service/alembic/versions/0005_add_share_can_delete.py new file mode 100644 index 0000000..1b40cff --- /dev/null +++ b/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") diff --git a/features/doc-service/app/deps.py b/features/doc-service/app/deps.py index 21adc31..d0dd701 100644 --- a/features/doc-service/app/deps.py +++ b/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" diff --git a/features/doc-service/app/models/document_share.py b/features/doc-service/app/models/document_share.py index 45822b3..2f5ee9d 100644 --- a/features/doc-service/app/models/document_share.py +++ b/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 ) diff --git a/features/doc-service/app/routers/documents.py b/features/doc-service/app/routers/documents.py index b68a736..ada4408 100644 --- a/features/doc-service/app/routers/documents.py +++ b/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() diff --git a/features/doc-service/app/schemas/document.py b/features/doc-service/app/schemas/document.py index 4cba912..17776e0 100644 --- a/features/doc-service/app/schemas/document.py +++ b/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} diff --git a/features/doc-service/app/schemas/share.py b/features/doc-service/app/schemas/share.py index 5baff2f..c783e1b 100644 --- a/features/doc-service/app/schemas/share.py +++ b/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 diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 1cf1144..93df1d4 100644 --- a/frontend/src/api/client.ts +++ b/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(`/documents/${docId}/shares`); -export const addDocumentShare = (docId: string, groupId: string) => - api.post(`/documents/${docId}/shares`, { group_id: groupId }); +export const addDocumentShare = (docId: string, groupId: string, canDelete = false) => + api.post(`/documents/${docId}/shares`, { group_id: groupId, can_delete: canDelete }); export const removeDocumentShare = (docId: string, groupId: string) => api.delete(`/documents/${docId}/shares/${groupId}`); diff --git a/frontend/src/components/DocumentSlideOver.tsx b/frontend/src/components/DocumentSlideOver.tsx index 387461f..365bf92 100644 --- a/frontend/src/components/DocumentSlideOver.tsx +++ b/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(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 }:
{group?.name ?? share.group_id} + {share.can_delete && ( + + + + )}
+ addShareMut.mutate(id)} + onShare={(id) => addShareMut.mutate({ groupId: id, canDelete: canDeleteNew })} /> )} - {/* Owner actions */} - {isOwner && ( + {/* Owner/permitted actions */} + {(isOwner || doc.viewer_can_delete) && (
- - + {isOwner && ( + + )} + {doc.viewer_can_delete && ( + + )}
)} diff --git a/frontend/src/pages/DocumentsPage.tsx b/frontend/src/pages/DocumentsPage.tsx index 28d4181..c1795d3 100644 --- a/frontend/src/pages/DocumentsPage.tsx +++ b/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(null); + const [menuPos, setMenuPos] = useState({ top: 0, right: 0 }); + const triggerRef = useRef(null); + const menuRef = useRef(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 ( -
e.stopPropagation()}> +
e.stopPropagation()}> - {open && ( -
+ {open && createPortal( +
- {isOwner && ( + {doc.viewer_can_delete && ( )} -
+
, + document.body )}
); @@ -470,7 +499,7 @@ function DocumentRow({ - + ); -- 2.52.0