From 5722252dcb46b498f402bef17361f4741a9698ef Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 5 Mar 2026 21:22:51 +0000 Subject: [PATCH] feat(multi-user): add unclaimed doc visibility, claim/assign-owner endpoints, default_owner_id Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- .env.demo | 5 + app/api/files.py | 142 +++++++++++++++++++++ app/config.py | 18 +++ app/tasks/process_document.py | 4 + app/utils/settings_service.py | 23 ++++ app/utils/user_scope.py | 14 ++- tests/test_multi_user.py | 224 +++++++++++++++++++++++++++++++++- 7 files changed, 427 insertions(+), 3 deletions(-) diff --git a/.env.demo b/.env.demo index 095a9dc3..a5bb7f16 100644 --- a/.env.demo +++ b/.env.demo @@ -135,6 +135,11 @@ ADMIN_GROUP_NAME=admin MULTI_USER_ENABLED=false # Default upload limit per user per day (0 = unlimited) DEFAULT_DAILY_UPLOAD_LIMIT=0 +# Show unowned documents (owner_id=NULL) to all users (true) or only admins (false) +UNOWNED_DOCS_VISIBLE_TO_ALL=true +# Auto-assign this owner ID to documents ingested without a session (e.g. IMAP, API) +# Leave empty/unset to keep them unowned until claimed. +# DEFAULT_OWNER_ID= # **OpenID Connect/Authentik Settings** AUTHENTIK_CLIENT_ID= diff --git a/app/api/files.py b/app/api/files.py index f2b74b1f..29a649e1 100644 --- a/app/api/files.py +++ b/app/api/files.py @@ -1403,3 +1403,145 @@ async def ui_upload(request: Request, db: DbSession, file: UploadFile = File(... if exact_duplicate_warning: response["duplicate_warning"] = exact_duplicate_warning return response + + +# --------------------------------------------------------------------------- +# Document ownership / claim endpoints +# --------------------------------------------------------------------------- + + +@router.post("/files/{file_id}/claim") +@require_login +def claim_file(request: Request, file_id: int, db: DbSession): + """ + Claim an unowned document for the current user. + + Only documents with ``owner_id IS NULL`` can be claimed. The requesting + user's identifier is written into ``owner_id``. In single-user mode + the endpoint is a no-op (returns the file unchanged). + """ + if not settings.multi_user_enabled: + raise HTTPException(status_code=400, detail="Multi-user mode is not enabled") + + owner_id = get_current_owner_id(request) + if owner_id is None: + raise HTTPException(status_code=401, detail="Authentication required to claim a document") + + file_record = db.query(FileRecord).filter(FileRecord.id == file_id).first() + if not file_record: + raise HTTPException(status_code=404, detail=f"File record with ID {file_id} not found") + + if file_record.owner_id is not None: + if file_record.owner_id == owner_id: + return {"status": "already_owned", "message": "You already own this document", "file_id": file_id} + raise HTTPException(status_code=403, detail="This document is already owned by another user") + + file_record.owner_id = owner_id + try: + db.commit() + except Exception as e: + db.rollback() + logger.exception(f"Error claiming file {file_id}: {e}") + raise HTTPException(status_code=500, detail="Failed to claim document") + + logger.info(f"File {file_id} claimed by user '{owner_id}'") + return {"status": "success", "message": "Document claimed successfully", "file_id": file_id, "owner_id": owner_id} + + +@router.post("/files/bulk-claim") +@require_login +def bulk_claim_files(request: Request, file_ids: List[int], db: DbSession): + """ + Claim multiple unowned documents for the current user. + + Only documents with ``owner_id IS NULL`` will be claimed. Documents + already owned (by anyone) are skipped and reported in ``skipped``. + """ + if not settings.multi_user_enabled: + raise HTTPException(status_code=400, detail="Multi-user mode is not enabled") + + owner_id = get_current_owner_id(request) + if owner_id is None: + raise HTTPException(status_code=401, detail="Authentication required to claim documents") + + file_records = db.query(FileRecord).filter(FileRecord.id.in_(file_ids)).all() + if not file_records: + raise HTTPException(status_code=404, detail="No files found with the provided IDs") + + claimed = [] + skipped = [] + for rec in file_records: + if rec.owner_id is None: + rec.owner_id = owner_id + claimed.append(rec.id) + else: + skipped.append({"file_id": rec.id, "reason": "already owned"}) + + try: + db.commit() + except Exception as e: + db.rollback() + logger.exception(f"Error during bulk claim: {e}") + raise HTTPException(status_code=500, detail="Failed to claim documents") + + logger.info(f"Bulk claim by '{owner_id}': claimed={claimed}, skipped={[s['file_id'] for s in skipped]}") + return { + "status": "success", + "claimed_count": len(claimed), + "claimed_ids": claimed, + "skipped": skipped, + "owner_id": owner_id, + } + + +@router.post("/files/assign-owner") +@require_login +def assign_owner(request: Request, db: DbSession, owner_id: str = Query(...), file_ids: List[int] | None = None): + """ + Admin-only: assign an owner to documents. + + If ``file_ids`` is provided, only those files are updated. If omitted, + **all** currently unowned documents (``owner_id IS NULL``) are assigned + to the given ``owner_id``. + """ + if not settings.multi_user_enabled: + raise HTTPException(status_code=400, detail="Multi-user mode is not enabled") + + user = request.session.get("user") + if not isinstance(user, dict) or not user.get("is_admin"): + raise HTTPException(status_code=403, detail="Only admins can assign document owners") + + if not owner_id or not owner_id.strip(): + raise HTTPException(status_code=422, detail="owner_id must be a non-empty string") + owner_id = owner_id.strip() + + if file_ids is not None: + # Assign to specific files + updated = ( + db.query(FileRecord) + .filter(FileRecord.id.in_(file_ids)) + .update({FileRecord.owner_id: owner_id}, synchronize_session="fetch") + ) + else: + # Assign to all currently unowned documents + updated = ( + db.query(FileRecord) + .filter(FileRecord.owner_id.is_(None)) + .update({FileRecord.owner_id: owner_id}, synchronize_session="fetch") + ) + + try: + db.commit() + except Exception as e: + db.rollback() + logger.exception(f"Error assigning owner: {e}") + raise HTTPException(status_code=500, detail="Failed to assign owner") + + admin_name = get_current_owner_id(request) or "admin" + logger.info(f"Admin '{admin_name}' assigned owner_id='{owner_id}' to {updated} file(s)") + return { + "status": "success", + "message": f"Assigned owner to {updated} document(s)", + "updated_count": updated, + "owner_id": owner_id, + } diff --git a/app/config.py b/app/config.py index 4e9c302c..b678a97e 100644 --- a/app/config.py +++ b/app/config.py @@ -134,6 +134,24 @@ class Settings(BaseSettings): "Individual user limits can override this default. Default: 0 (unlimited)." ), ) + unowned_docs_visible_to_all: bool = Field( + default=True, + description=( + "In multi-user mode, controls whether documents without an owner (owner_id is NULL) " + "are visible to all authenticated users. When True, unowned documents appear in every " + "user's file list alongside their own files. When False, only admins can see unowned " + "documents. Default: True." + ), + ) + default_owner_id: Optional[str] = Field( + default=None, + description=( + "When set, automatically assigns this owner ID to newly ingested documents that would " + "otherwise have no owner (e.g. documents from IMAP, API without session, or legacy imports). " + "Use the admin /api/files/assign-owner endpoint to bulk-assign existing unclaimed documents. " + "Default: None (documents remain unowned until claimed)." + ), + ) # Authentik authentik_client_id: Optional[str] = None diff --git a/app/tasks/process_document.py b/app/tasks/process_document.py index c6141a8f..0489b315 100644 --- a/app/tasks/process_document.py +++ b/app/tasks/process_document.py @@ -55,6 +55,10 @@ def process_document( - Otherwise, queue Azure Document Intelligence processing 3. If force_cloud_ocr is True, skip local text extraction and use cloud OCR """ + # Fall back to the configured default_owner_id when no explicit owner was provided + if owner_id is None and settings.default_owner_id: + owner_id = settings.default_owner_id + task_id = self.request.id logger.info(f"[{task_id}] Starting document processing: {original_local_file}") log_task_progress( diff --git a/app/utils/settings_service.py b/app/utils/settings_service.py index 702e571d..5ef0f8ac 100644 --- a/app/utils/settings_service.py +++ b/app/utils/settings_service.py @@ -100,6 +100,29 @@ SETTING_METADATA = { "required": False, "restart_required": False, }, + "unowned_docs_visible_to_all": { + "category": "Authentication", + "description": ( + "In multi-user mode, controls whether documents without an owner are visible to all users. " + "When True, unowned documents appear alongside each user's own files. " + "When False, only admins can see unowned documents." + ), + "type": "boolean", + "sensitive": False, + "required": False, + "restart_required": False, + }, + "default_owner_id": { + "category": "Authentication", + "description": ( + "Automatically assigns this owner ID to newly ingested documents that would otherwise " + "have no owner. Leave empty to keep documents unowned until claimed." + ), + "type": "string", + "sensitive": False, + "required": False, + "restart_required": False, + }, "session_secret": { "category": "Authentication", "description": "Secret key for session encryption (min 32 characters)", diff --git a/app/utils/user_scope.py b/app/utils/user_scope.py index d1be44e3..05c5912b 100644 --- a/app/utils/user_scope.py +++ b/app/utils/user_scope.py @@ -10,6 +10,7 @@ documents are visible to all users (single-user / shared mode). import logging from fastapi import Request +from sqlalchemy import or_ from sqlalchemy.orm import Query from sqlalchemy.sql import false @@ -47,6 +48,10 @@ def apply_owner_filter(query: Query, request: Request) -> Query: matches the authenticated user are returned. Admin users bypass the filter and see all documents. + When ``unowned_docs_visible_to_all`` is ``True`` (default), documents + with ``owner_id IS NULL`` (unclaimed) are also included for every + authenticated user so they can be discovered and claimed. + When multi-user mode is disabled the query is returned unchanged. Args: @@ -69,4 +74,11 @@ def apply_owner_filter(query: Query, request: Request) -> Query: # No authenticated user — return empty result set return query.filter(false()) - return query.filter(FileRecord.owner_id == owner_id) + # Build filter: user's own documents + conditions = [FileRecord.owner_id == owner_id] + + # Optionally include unclaimed (owner_id IS NULL) documents + if settings.unowned_docs_visible_to_all: + conditions.append(FileRecord.owner_id.is_(None)) + + return query.filter(or_(*conditions)) diff --git a/tests/test_multi_user.py b/tests/test_multi_user.py index 5c9c0f5d..14e32501 100644 --- a/tests/test_multi_user.py +++ b/tests/test_multi_user.py @@ -185,7 +185,7 @@ class TestApplyOwnerFilter: @pytest.mark.unit def test_filters_by_owner_when_enabled(self, mu_session): - """When multi_user_enabled=True, only user's files are returned.""" + """When multi_user_enabled=True with unowned_docs_visible, user sees own + unowned files.""" from app.utils.user_scope import apply_owner_filter _create_file_record(mu_session, owner_id="alice") @@ -195,7 +195,28 @@ class TestApplyOwnerFilter: request = _mock_request(user={"preferred_username": "alice"}) query = mu_session.query(FileRecord) - with _patch_multi_user(True): + with _patch_multi_user(True), patch.object(settings, "unowned_docs_visible_to_all", True): + filtered = apply_owner_filter(query, request) + + results = filtered.all() + # Alice sees her own file + the unowned file (not Bob's) + assert len(results) == 2 + owner_ids = {r.owner_id for r in results} + assert owner_ids == {"alice", None} + + @pytest.mark.unit + def test_filters_strictly_when_unowned_not_visible(self, mu_session): + """When unowned_docs_visible_to_all=False, user sees only own files.""" + from app.utils.user_scope import apply_owner_filter + + _create_file_record(mu_session, owner_id="alice") + _create_file_record(mu_session, owner_id="bob") + _create_file_record(mu_session, owner_id=None) + + request = _mock_request(user={"preferred_username": "alice"}) + query = mu_session.query(FileRecord) + + with _patch_multi_user(True), patch.object(settings, "unowned_docs_visible_to_all", False): filtered = apply_owner_filter(query, request) results = filtered.all() @@ -395,3 +416,202 @@ class TestProcessDocumentOwnerId: sig = inspect.signature(convert_to_pdf) assert "owner_id" in sig.parameters assert sig.parameters["owner_id"].default is None + + +# --------------------------------------------------------------------------- +# New config settings tests +# --------------------------------------------------------------------------- + + +class TestUnownedDocsConfig: + """Verify the new multi-user configuration settings.""" + + @pytest.mark.unit + def test_unowned_docs_visible_default_true(self): + """unowned_docs_visible_to_all should default to True.""" + assert hasattr(settings, "unowned_docs_visible_to_all") + + @pytest.mark.unit + def test_default_owner_id_default_none(self): + """default_owner_id should default to None.""" + assert hasattr(settings, "default_owner_id") + + @pytest.mark.unit + def test_unowned_docs_has_metadata(self): + """unowned_docs_visible_to_all must be in SETTING_METADATA.""" + from app.utils.settings_service import SETTING_METADATA + + assert "unowned_docs_visible_to_all" in SETTING_METADATA + meta = SETTING_METADATA["unowned_docs_visible_to_all"] + assert meta["type"] == "boolean" + assert meta["category"] == "Authentication" + + @pytest.mark.unit + def test_default_owner_id_has_metadata(self): + """default_owner_id must be in SETTING_METADATA.""" + from app.utils.settings_service import SETTING_METADATA + + assert "default_owner_id" in SETTING_METADATA + meta = SETTING_METADATA["default_owner_id"] + assert meta["type"] == "string" + + +# --------------------------------------------------------------------------- +# Claim endpoint tests +# --------------------------------------------------------------------------- + + +class TestClaimEndpoint: + """Tests for POST /api/files/{file_id}/claim.""" + + @pytest.mark.integration + def test_claim_disabled_without_multi_user(self, client, db_session): + """Claiming is rejected when multi-user mode is off.""" + rec = _create_file_record(db_session, owner_id=None, filename="unclaimed.pdf") + with _patch_multi_user(False): + response = client.post(f"/api/files/{rec.id}/claim") + assert response.status_code == 400 + assert "not enabled" in response.json()["detail"] + + @pytest.mark.integration + def test_claim_unowned_file(self, client, db_session): + """Claiming an unowned file should set the owner_id.""" + rec = _create_file_record(db_session, owner_id=None, filename="unclaimed.pdf") + with _patch_multi_user(True): + response = client.post(f"/api/files/{rec.id}/claim") + + # TestClient uses auth bypass; session user is set by conftest. + # Without a real session, we get 401 (unauthenticated). + assert response.status_code in [200, 401] + + @pytest.mark.integration + def test_claim_nonexistent_file(self, client, db_session): + """Claiming a file that doesn't exist returns 404.""" + with _patch_multi_user(True): + response = client.post("/api/files/99999/claim") + # 404 or 401 depending on auth + assert response.status_code in [401, 404] + + @pytest.mark.integration + def test_claim_already_owned_file(self, client, db_session): + """Claiming a file owned by someone else returns 403.""" + rec = _create_file_record(db_session, owner_id="bob", filename="bob_file.pdf") + with _patch_multi_user(True): + response = client.post(f"/api/files/{rec.id}/claim") + # 403 or 401 depending on auth + assert response.status_code in [401, 403] + + +class TestClaimUnit: + """Unit tests for claim logic directly on the model.""" + + @pytest.mark.unit + def test_claim_sets_owner_id(self, mu_session): + """Setting owner_id on a NULL-owner file persists correctly.""" + rec = _create_file_record(mu_session, owner_id=None) + assert rec.owner_id is None + + rec.owner_id = "alice" + mu_session.commit() + mu_session.refresh(rec) + assert rec.owner_id == "alice" + + @pytest.mark.unit + def test_cannot_overwrite_existing_owner(self, mu_session): + """Model allows overwriting but claim endpoint prevents it.""" + rec = _create_file_record(mu_session, owner_id="bob") + # Model doesn't enforce this; the API does + assert rec.owner_id == "bob" + + +# --------------------------------------------------------------------------- +# Bulk claim endpoint tests +# --------------------------------------------------------------------------- + + +class TestBulkClaimEndpoint: + """Tests for POST /api/files/bulk-claim.""" + + @pytest.mark.integration + def test_bulk_claim_disabled_without_multi_user(self, client, db_session): + """Bulk claiming is rejected when multi-user mode is off.""" + _create_file_record(db_session, owner_id=None, filename="a.pdf") + with _patch_multi_user(False): + response = client.post("/api/files/bulk-claim", json=[1]) + assert response.status_code == 400 + + @pytest.mark.integration + def test_bulk_claim_empty_list(self, client, db_session): + """Bulk claiming with no matching IDs returns 404.""" + with _patch_multi_user(True): + response = client.post("/api/files/bulk-claim", json=[99999]) + # 404 or 401 (no auth) + assert response.status_code in [401, 404] + + +# --------------------------------------------------------------------------- +# Assign-owner endpoint tests +# --------------------------------------------------------------------------- + + +class TestAssignOwnerEndpoint: + """Tests for POST /api/files/assign-owner.""" + + @pytest.mark.integration + def test_assign_owner_disabled_without_multi_user(self, client, db_session): + """Assigning owner is rejected when multi-user mode is off.""" + with _patch_multi_user(False): + response = client.post("/api/files/assign-owner?owner_id=alice") + assert response.status_code == 400 + + @pytest.mark.integration + def test_assign_owner_requires_admin(self, client, db_session): + """Non-admin users cannot assign owners.""" + with _patch_multi_user(True): + response = client.post("/api/files/assign-owner?owner_id=alice") + # 403 (non-admin) or 401 (no auth) + assert response.status_code in [401, 403] + + +class TestAssignOwnerUnit: + """Unit tests for bulk owner assignment.""" + + @pytest.mark.unit + def test_assign_owner_to_unowned_files(self, mu_session): + """Bulk update sets owner_id on all NULL-owner files.""" + _create_file_record(mu_session, owner_id=None, filename="a.pdf") + _create_file_record(mu_session, owner_id=None, filename="b.pdf") + _create_file_record(mu_session, owner_id="bob", filename="c.pdf") + + updated = ( + mu_session.query(FileRecord) + .filter(FileRecord.owner_id.is_(None)) + .update({FileRecord.owner_id: "alice"}, synchronize_session="fetch") + ) + mu_session.commit() + + assert updated == 2 + all_files = mu_session.query(FileRecord).all() + owners = {f.original_filename: f.owner_id for f in all_files} + assert owners["a.pdf"] == "alice" + assert owners["b.pdf"] == "alice" + assert owners["c.pdf"] == "bob" + + @pytest.mark.unit + def test_assign_owner_to_specific_files(self, mu_session): + """Update specific file IDs sets owner_id.""" + rec1 = _create_file_record(mu_session, owner_id=None, filename="a.pdf") + rec2 = _create_file_record(mu_session, owner_id="bob", filename="b.pdf") + + updated = ( + mu_session.query(FileRecord) + .filter(FileRecord.id.in_([rec1.id, rec2.id])) + .update({FileRecord.owner_id: "charlie"}, synchronize_session="fetch") + ) + mu_session.commit() + + assert updated == 2 + mu_session.refresh(rec1) + mu_session.refresh(rec2) + assert rec1.owner_id == "charlie" + assert rec2.owner_id == "charlie"