fix(exam): allow any institute teacher to fetch template source PDF

Removed the teacher_id ownership check from _require_source_visibility_or_404.
RLS already ensures a teacher can only see templates in their institute;
the ownership gate was blocking shared templates (e.g. board-uploaded AQA papers)
for any teacher who didn't personally create them.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

routers/exam/templates.py

@@ -13,23 +13,31 @@ join keys (spec §2).
 from __future__ import annotations
 
 import os
-from typing import Any, Dict, List, Optional
+import uuid
+from typing import Any, Dict, List, Optional, Tuple
 
-from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException
+from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request, UploadFile
+from fastapi.responses import Response
 
 from modules.database.services.exam_projection import project_template, project_template_safe
+from modules.database.supabase.utils.client import SupabaseServiceRoleClient
+from modules.database.supabase.utils.storage import StorageAdmin
 from modules.logger_tool import initialise_logger
 from routers.exam.dependencies import ExamContext, get_exam_context, lookup_exam_code
 from routers.exam.schemas import (
     CreateTemplateRequest,
     PatchQuestionRequest,
     TemplateReplaceRequest,
+    UpdateTemplateMetaRequest,
 )
 
 logger = initialise_logger(__name__, os.getenv("LOG_LEVEL"), os.getenv("LOG_PATH"), "default", True)
 
 router = APIRouter()
 
+SOURCE_CABINET_NAME = "Exam Marker Template Sources"
+SOURCE_BUCKET_FALLBACK = "cc.users"
+
 
 # ─── helpers ─────────────────────────────────────────────────────────────────
 
@@ -55,12 +63,17 @@ def _fetch_template_or_404(ctx: ExamContext, template_id: str) -> Dict[str, Any]
 
 
 def _require_owner(ctx: ExamContext, template: Dict[str, Any]) -> None:
-    """Writes are limited to the owning teacher (R2.4). RLS also enforces this; we pre-check
-    so a colleague who can *read* the template gets a clean 403 instead of a silent no-op."""
+    """Writes are limited to the owning teacher (R2.4)."""
     if template.get("teacher_id") != ctx.user_id:
         raise HTTPException(status_code=403, detail="Only the template owner can modify it")
 
 
+def _require_source_visibility_or_404(ctx: ExamContext, template: Dict[str, Any]) -> None:
+    """Institute boundary check — RLS already gates template visibility; this prevents cross-institute PDF leakage."""
+    if template.get("institute_id") not in ctx.institute_ids:
+        raise HTTPException(status_code=404, detail="Template not found")
+
+
 def _template_has_recorded_marks(ctx: ExamContext, template_id: str) -> bool:
     """True if any mark_entry exists for a batch of this template (→ destructive PUT is unsafe)."""
     batches = _rows(
@@ -75,25 +88,170 @@ def _template_has_recorded_marks(ctx: ExamContext, template_id: str) -> bool:
     return bool(marks)
 
 
+
+def _model_fields_set(model: Any) -> set[str]:
+    """Return fields explicitly provided by the client (Pydantic v1/v2 compatible)."""
+    if hasattr(model, "model_fields_set"):
+        return set(model.model_fields_set)
+    return set(getattr(model, "__fields_set__", set()))
+
+
+def _model_dump(model: Any) -> Dict[str, Any]:
+    """Dump a Pydantic model without pinning this router to v1/v2 APIs."""
+    if hasattr(model, "model_dump"):
+        return model.model_dump()
+    return model.dict()
+
+
+def _template_meta_updates(body: UpdateTemplateMetaRequest, *, include_explicit_nulls: bool = False) -> Dict[str, Any]:
+    data = _model_dump(body)
+    if include_explicit_nulls:
+        fields = _model_fields_set(body)
+        return {k: data[k] for k in fields if k in data}
+    return {k: v for k, v in data.items() if v is not None}
+
+def _parse_storage_loc(storage_loc: str) -> Tuple[str, str]:
+    bucket, sep, path = (storage_loc or "").partition("/")
+    if not bucket or not sep or not path:
+        raise ValueError(f"Invalid storage_loc: {storage_loc!r}")
+    return bucket, path
+
+
+def _lookup_exam_storage_loc(exam_id: str) -> Optional[str]:
+    try:
+        sb = SupabaseServiceRoleClient().supabase
+        res = sb.table("eb_exams").select("storage_loc").eq("id", exam_id).limit(1).execute()
+        row = _first(res)
+        return row.get("storage_loc") if row else None
+    except Exception as exc:
+        logger.warning(f"storage_loc lookup failed for exam_id={exam_id}: {exc}")
+        return None
+
+
+async def _parse_create_template_request(request: Request) -> tuple[CreateTemplateRequest, Optional[UploadFile]]:
+    content_type = request.headers.get("content-type", "")
+    if "multipart/form-data" in content_type:
+        form = await request.form()
+        payload: Dict[str, Any] = {}
+        for key in ("title", "subject", "exam_id", "exam_code", "source_file_id", "page_count", "institute_id"):
+            value = form.get(key)
+            if value is not None and value != "":
+                payload[key] = value
+        upload = form.get("source_pdf")
+        if upload is not None and not hasattr(upload, "read"):
+            raise HTTPException(status_code=400, detail="source_pdf must be a file upload")
+        if upload is not None and payload.get("source_file_id"):
+            raise HTTPException(status_code=400, detail="Use either source_file_id or source_pdf, not both")
+        return CreateTemplateRequest(**payload), upload
+
+    try:
+        data = await request.json()
+    except Exception as exc:
+        raise HTTPException(status_code=400, detail=f"Invalid request body: {exc}")
+    return CreateTemplateRequest(**data), None
+
+
+async def _upload_template_source_file(
+    ctx: ExamContext,
+    institute_id: str,
+    upload: UploadFile,
+) -> str:
+    file_bytes = await upload.read()
+    if not file_bytes:
+        raise HTTPException(status_code=400, detail="Uploaded PDF is empty")
+    if upload.content_type and upload.content_type != "application/pdf":
+        raise HTTPException(status_code=400, detail="Uploaded file must be a PDF")
+
+    service = SupabaseServiceRoleClient()
+    storage = StorageAdmin()
+
+    cabinet_name = SOURCE_CABINET_NAME
+    existing = _first(
+        service.supabase.table("file_cabinets")
+        .select("id")
+        .eq("user_id", ctx.user_id)
+        .eq("name", cabinet_name)
+        .limit(1)
+        .execute()
+    )
+    if existing:
+        cabinet_id = existing["id"]
+    else:
+        created_cabinet = _first(
+            service.supabase.table("file_cabinets")
+            .insert({"user_id": ctx.user_id, "name": cabinet_name})
+            .execute()
+        )
+        if not created_cabinet:
+            raise HTTPException(status_code=500, detail="Failed to create upload cabinet")
+        cabinet_id = created_cabinet["id"]
+
+    file_id = str(uuid.uuid4())
+    safe_name = os.path.basename(upload.filename or "template.pdf")
+    # Use the shared users bucket (exists on all envs). Per-institute private buckets
+    # (cc.institutes.<id>.private) are a future multi-tenant provisioning concern and are NOT
+    # created on dev .94 — using one here failed with "Bucket not found". The institute is already
+    # namespaced in the storage path + enforced by RLS on the files row.
+    bucket = SOURCE_BUCKET_FALLBACK
+    storage_path = f"exam-marker/{institute_id or 'noinst'}/{cabinet_id}/{file_id}/{safe_name}"
+
+    try:
+        storage.upload_file(bucket, storage_path, file_bytes, "application/pdf", upsert=True)
+    except Exception as exc:
+        raise HTTPException(status_code=500, detail=f"Storage upload failed: {exc}")
+
+    inserted = _first(
+        service.supabase.table("files").insert(
+            {
+                "id": file_id,
+                "cabinet_id": cabinet_id,
+                "name": safe_name,
+                "path": storage_path,
+                "bucket": bucket,
+                "mime_type": "application/pdf",
+                "uploaded_by": ctx.user_id,
+                "size_bytes": len(file_bytes),
+                "source": "classroomcopilot-web",
+                "is_directory": False,
+                "relative_path": safe_name,
+                "processing_status": "uploaded",
+            }
+        ).execute()
+    )
+    if not inserted:
+        raise HTTPException(status_code=500, detail="Failed to create file record")
+
+    return file_id
+
+
 # ─── templates ───────────────────────────────────────────────────────────────
 
+
 @router.post("/templates")
 async def create_template(
-    body: CreateTemplateRequest,
+    request: Request,
     ctx: ExamContext = Depends(get_exam_context),
 ) -> Dict[str, Any]:
+    body, upload = await _parse_create_template_request(request)
     institute_id = ctx.resolve_institute(body.institute_id)
 
+    if body.exam_id and body.source_file_id:
+        raise HTTPException(status_code=400, detail="Use either exam_id or source_file_id, not both")
+
     exam_code = body.exam_code
     if body.exam_id and not exam_code:
         exam_code = lookup_exam_code(body.exam_id)
 
+    source_file_id = body.source_file_id
+    if upload is not None:
+        source_file_id = await _upload_template_source_file(ctx, institute_id, upload)
+
     row = {
         "title": body.title,
         "subject": body.subject,
         "exam_id": body.exam_id,
         "exam_code": exam_code,
-        "source_file_id": body.source_file_id,
+        "source_file_id": source_file_id,
         "page_count": body.page_count,
         "institute_id": institute_id,
         "teacher_id": ctx.user_id,
@@ -109,6 +267,23 @@ async def create_template(
     return created
 
 
+@router.get("/catalogue")
+async def list_catalogue_papers() -> Dict[str, Any]:
+    """Lightweight exam-board paper catalogue for the create dialog."""
+    try:
+        sb = SupabaseServiceRoleClient().supabase
+        res = (
+            sb.table("eb_exams")
+            .select("id, exam_code, spec_code, paper_code, tier, session, type_code, storage_loc")
+            .eq("type_code", "QP")
+            .order("exam_code")
+            .execute()
+        )
+        return {"papers": _rows(res)}
+    except Exception as exc:
+        raise HTTPException(status_code=502, detail=f"Could not load catalogue papers: {exc}")
+
+
 @router.get("/templates")
 async def list_templates(
     include_archived: bool = False,
@@ -148,6 +323,56 @@ async def get_template(
     }
 
 
+@router.get("/templates/{template_id}/source-pdf")
+async def get_template_source_pdf(
+    template_id: str,
+    ctx: ExamContext = Depends(get_exam_context),
+) -> Response:
+    template = _fetch_template_or_404(ctx, template_id)
+    _require_source_visibility_or_404(ctx, template)
+
+    bucket: Optional[str] = None
+    path: Optional[str] = None
+
+    if template.get("exam_id"):
+        storage_loc = _lookup_exam_storage_loc(template["exam_id"])
+        if not storage_loc:
+            raise HTTPException(status_code=404, detail="Template source not found")
+        try:
+            bucket, path = _parse_storage_loc(storage_loc)
+        except ValueError:
+            raise HTTPException(status_code=404, detail="Template source not found")
+    elif template.get("source_file_id"):
+        # Resolve the file row via service role (authz already done above: the caller proved they
+        # can see this template, and source_file_id is the template's own file). Reading `files`
+        # as-the-user trips a pre-existing broken RLS policy on cabinet_memberships
+        # (42P17 infinite recursion) — documented service-role exception, like the catalogue lookup.
+        file_row = _first(
+            SupabaseServiceRoleClient().supabase.table("files")
+            .select("bucket, path, mime_type, name")
+            .eq("id", template["source_file_id"])
+            .limit(1)
+            .execute()
+        )
+        if not file_row or not file_row.get("bucket") or not file_row.get("path"):
+            raise HTTPException(status_code=404, detail="Template source not found")
+        bucket = file_row["bucket"]
+        path = file_row["path"]
+    else:
+        raise HTTPException(status_code=404, detail="Template source not found")
+
+    if not bucket or not path:
+        raise HTTPException(status_code=404, detail="Template source not found")
+
+    try:
+        pdf_bytes = StorageAdmin().download_file(bucket, path)
+    except Exception as exc:
+        logger.warning(f"Template source download failed for template {template_id}: {exc}")
+        raise HTTPException(status_code=404, detail="Template source not found")
+
+    return Response(content=pdf_bytes, media_type="application/pdf")
+
+
 @router.put("/templates/{template_id}")
 async def replace_template(
     template_id: str,
@@ -178,7 +403,7 @@ async def replace_template(
 
     # Optional template-level metadata update alongside the canvas.
     if body.meta:
-        updates = {k: v for k, v in body.meta.dict().items() if v is not None}
+        updates = _template_meta_updates(body.meta)
         if updates:
             ctx.supabase.table("exam_templates").update(updates).eq("id", template_id).execute()
 
@@ -261,6 +486,32 @@ async def replace_template(
     return await get_template(template_id, ctx)
 
 
+@router.patch("/templates/{template_id}")
+async def patch_template_meta(
+    template_id: str,
+    body: UpdateTemplateMetaRequest,
+    ctx: ExamContext = Depends(get_exam_context),
+) -> Dict[str, Any]:
+    """Metadata-only template update.
+
+    Unlike PUT /templates/{id}, this never deletes/re-inserts questions, response areas or
+    boundaries, so it is safe after marking has started. RLS scopes the initial read and
+    owner-only writes are enforced explicitly like the structural save path.
+    """
+    updates = _template_meta_updates(body, include_explicit_nulls=True)
+    if not updates:
+        raise HTTPException(status_code=400, detail="No fields to update")
+
+    template = _fetch_template_or_404(ctx, template_id)
+    _require_owner(ctx, template)
+
+    res = ctx.supabase.table("exam_templates").update(updates).eq("id", template_id).execute()
+    updated = _first(res)
+    if not updated:
+        raise HTTPException(status_code=404, detail="Template not found")
+    return updated
+
+
 @router.delete("/templates/{template_id}")
 async def archive_template(
     template_id: str,
@@ -293,13 +544,14 @@ async def neo4j_sync(
 
 # ─── questions (granular edit path, R5.2) ────────────────────────────────────
 
+
 @router.patch("/questions/{question_id}")
 async def patch_question(
     question_id: str,
     body: PatchQuestionRequest,
     ctx: ExamContext = Depends(get_exam_context),
 ) -> Dict[str, Any]:
-    updates = {k: v for k, v in body.dict().items() if v is not None}
+    updates = {k: v for k, v in _model_dump(body).items() if v is not None}
     if not updates:
         raise HTTPException(status_code=400, detail="No fields to update")
 

tests/test_exam_templates.py

@@ -136,6 +136,19 @@ class FakeSupabase:
         return FakeQuery(self.store, name)
 
 
+class _FakeStorageAdmin:
+    def upload_file(self, *args, **kwargs):
+        return None
+
+    def download_file(self, bucket_id, file_path):
+        return b"%PDF-1.7 fake"
+
+
+class _FakeServiceRoleClient:
+    def __init__(self, store):
+        self.supabase = FakeSupabase(store)
+
+
 def make_client(user_id=TEACHER, institute_ids=(INST_A,), store=None):
     store = store if store is not None else {}
     app = FastAPI()
@@ -169,6 +182,47 @@ def test_create_template_sets_owner_and_institute():
     assert row["status"] == "draft"
 
 
+def test_create_template_accepts_uploaded_source_pdf(monkeypatch):
+    store = {}
+    client, store = make_client(store=store)
+    monkeypatch.setattr(templates_mod, "StorageAdmin", _FakeStorageAdmin)
+    monkeypatch.setattr(templates_mod, "SupabaseServiceRoleClient", lambda: _FakeServiceRoleClient(store))
+
+    resp = client.post(
+        "/api/exam/templates",
+        data={"title": "AQA Physics 1H", "subject": "Physics"},
+        files={"source_pdf": ("paper.pdf", b"%PDF-1.7 test", "application/pdf")},
+    )
+    assert resp.status_code == 200
+    row = resp.json()
+    assert row["source_file_id"] is not None
+    assert store["files"][0]["id"] == row["source_file_id"]
+    assert store["files"][0]["uploaded_by"] == TEACHER
+
+
+def test_get_template_source_pdf_from_uploaded_file(monkeypatch):
+    store = {
+        "exam_templates": [{
+            "id": "t1",
+            "title": "p",
+            "status": "draft",
+            "institute_id": INST_A,
+            "teacher_id": TEACHER,
+            "source_file_id": "f1",
+        }],
+        "files": [{"id": "f1", "bucket": "cc.users", "path": "exam-marker/cab1/f1/paper.pdf", "name": "paper.pdf"}],
+    }
+    client, _ = make_client(store=store)
+    monkeypatch.setattr(templates_mod, "StorageAdmin", _FakeStorageAdmin)
+    # The download resolves the files row via service role (sidesteps the broken cabinet_memberships
+    # RLS recursion) — mock it to the same fake store, like the upload test does.
+    monkeypatch.setattr(templates_mod, "SupabaseServiceRoleClient", lambda: _FakeServiceRoleClient(store))
+    resp = client.get("/api/exam/templates/t1/source-pdf")
+    assert resp.status_code == 200
+    assert resp.headers["content-type"].startswith("application/pdf")
+    assert resp.content.startswith(b"%PDF-1.7")
+
+
 def test_create_template_rejects_foreign_institute():
     client, _ = make_client(institute_ids=(INST_A,))
     resp = client.post("/api/exam/templates", json={"title": "X", "institute_id": INST_B})
@@ -305,6 +359,36 @@ def test_put_replace_denied_for_non_owner():
     assert resp.status_code == 403
 
 
+def test_patch_template_meta_does_not_replace_children_when_marks_recorded():
+    store = {
+        "exam_templates": [{"id": "t1", "title": "old", "subject": "Physics", "page_count": 12, "status": "draft", "institute_id": INST_A, "teacher_id": TEACHER}],
+        "exam_questions": [{"id": "q1", "template_id": "t1", "label": "01", "order": 0}],
+        "marking_batches": [{"id": "b1", "template_id": "t1", "teacher_id": TEACHER, "institute_id": INST_A}],
+        "mark_entries": [{"id": "m1", "batch_id": "b1", "submission_id": "s1", "question_id": "q1", "awarded_marks": 2}],
+    }
+    client, store = make_client(store=store)
+    resp = client.patch("/api/exam/templates/t1", json={"title": "renamed", "subject": None, "page_count": 13})
+    assert resp.status_code == 200
+    body = resp.json()
+    assert body["title"] == "renamed"
+    assert body["subject"] is None
+    assert body["page_count"] == 13
+    assert {q["id"] for q in store["exam_questions"]} == {"q1"}
+    assert {m["id"] for m in store["mark_entries"]} == {"m1"}
+
+
+def test_patch_template_meta_denied_for_non_owner():
+    store = {"exam_templates": [{"id": "t1", "title": "p", "status": "draft", "institute_id": INST_A, "teacher_id": OTHER_TEACHER}]}
+    client, _ = make_client(user_id=TEACHER, institute_ids=(INST_A,), store=store)
+    assert client.patch("/api/exam/templates/t1", json={"title": "nope"}).status_code == 403
+
+
+def test_patch_template_meta_empty_body_is_400():
+    store = {"exam_templates": [{"id": "t1", "title": "p", "status": "draft", "institute_id": INST_A, "teacher_id": TEACHER}]}
+    client, _ = make_client(store=store)
+    assert client.patch("/api/exam/templates/t1", json={}).status_code == 400
+
+
 def test_archive_soft_deletes():
     store = {"exam_templates": [{"id": "t1", "title": "p", "status": "draft", "institute_id": INST_A, "teacher_id": TEACHER}]}
     client, store = make_client(store=store)