Fix: add auth and owner isolation to saved views API (closes #1743)

backend/secuscan/migrations/009_add_saved_views_owner_id.sql

@@ -0,0 +1,37 @@
+-- Migration: 009_add_saved_views_owner_id
+-- Closes issue #1743: the saved_views table had no owner_id column at all,
+-- and the API layer performed no authentication or ownership checks, so any
+-- caller could list, rename, overwrite, or delete every user's saved views.
+--
+-- This migration:
+--   1. Adds an owner_id column (defaulting existing rows to 'default', the
+--      same convention used for tasks/reports/workflows/etc).
+--   2. Recreates the table to replace the old UNIQUE(name) constraint with
+--      UNIQUE(owner_id, name) — two different owners may now reuse the same
+--      view name, matching how workflows/credential_vault were migrated.
+--   3. Rebuilds the supporting index scoped by owner.
+--
+-- The API layer (saved_views.py) now requires authentication and scopes
+-- every query by the resolved owner_id.
+
+ALTER TABLE saved_views ADD COLUMN owner_id TEXT NOT NULL DEFAULT 'default';
+
+CREATE TABLE saved_views_new (
+    id          TEXT PRIMARY KEY,
+    owner_id    TEXT NOT NULL DEFAULT 'default',
+    name        TEXT NOT NULL,
+    filter_json TEXT NOT NULL,
+    created_at  TIMESTAMP NOT NULL DEFAULT (datetime('now')),
+    updated_at  TIMESTAMP NOT NULL DEFAULT (datetime('now')),
+    UNIQUE(owner_id, name)
+);
+
+INSERT INTO saved_views_new (id, owner_id, name, filter_json, created_at, updated_at)
+SELECT id, COALESCE(owner_id, 'default'), name, filter_json, created_at, updated_at
+FROM saved_views;
+
+DROP TABLE saved_views;
+ALTER TABLE saved_views_new RENAME TO saved_views;
+
+DROP INDEX IF EXISTS idx_saved_views_name;
+CREATE INDEX IF NOT EXISTS idx_saved_views_owner_name ON saved_views(owner_id, LOWER(name));

backend/secuscan/saved_views.py

@@ -4,12 +4,22 @@
 import uuid
 from typing import Any, Dict, List, Optional
 
-from fastapi import APIRouter, HTTPException
+from fastapi import APIRouter, Depends, HTTPException
 from pydantic import BaseModel, Field, field_validator
 
+from .auth import get_current_owner, require_api_key
 from .database import get_db
 
-saved_views_router = APIRouter(prefix="/api/v1/saved-views", tags=["saved-views"])
+# Issue #1743: this router previously had no auth dependency and never
+# scoped queries by owner, so any caller (authenticated or not) could list,
+# rename, overwrite, or delete every user's saved views. It now requires the
+# same API key as the rest of the app and enforces per-owner isolation,
+# mirroring require_owned_task's pattern in routes.py.
+saved_views_router = APIRouter(
+    prefix="/api/v1/saved-views",
+    tags=["saved-views"],
+    dependencies=[Depends(require_api_key)],
+)
 
 _VALID_SORT_MODES = {"severity", "newest", "oldest", "target"}
 _VALID_SEVERITIES = {"all", "critical", "high", "medium", "low", "info"}
@@ -95,28 +105,50 @@ def validate_filter_json(cls, v: Optional[str]) -> Optional[str]:
 
 
 
+async def require_owned_saved_view(db, view_id: str, owner: str) -> Dict[str, Any]:
+    """Fetch a saved view and enforce that it belongs to ``owner`` (issue #1743).
+
+    Raises 404 when the view does not exist and 403 when it exists but is
+    owned by a different user/workspace, matching require_owned_task's
+    behaviour for tasks in routes.py.
+    """
+    row = await db.fetchone(
+        "SELECT id, owner_id FROM saved_views WHERE id = ?", (view_id,)
+    )
+    if row is None:
+        raise HTTPException(status_code=404, detail="Saved view not found")
+    if row["owner_id"] != owner:
+        raise HTTPException(
+            status_code=403, detail="You do not have access to this saved view"
+        )
+    return row
+
+
 @saved_views_router.get("")
-async def list_saved_views() -> Dict[str, Any]:
-    """Return all saved views ordered by creation date."""
+async def list_saved_views(owner: str = Depends(get_current_owner)) -> Dict[str, Any]:
+    """Return all saved views for the current owner, ordered by creation date."""
     db = await get_db()
     rows: List[Dict] = await db.fetchall(
         "SELECT id, name, filter_json, created_at, updated_at "
-        "FROM saved_views ORDER BY created_at ASC"
+        "FROM saved_views WHERE owner_id = ? ORDER BY created_at ASC",
+        (owner,),
     )
     return {"views": rows, "total": len(rows)}
 
 
 @saved_views_router.post("", status_code=201)
-async def create_saved_view(body: SavedViewCreate) -> Dict[str, Any]:
+async def create_saved_view(
+    body: SavedViewCreate, owner: str = Depends(get_current_owner)
+) -> Dict[str, Any]:
     """
-    Create a new saved view.
-    Returns 409 if a view with the same name already exists.
+    Create a new saved view for the current owner.
+    Returns 409 if the owner already has a view with the same name.
     """
     db = await get_db()
 
-
     existing = await db.fetchone(
-        "SELECT id FROM saved_views WHERE LOWER(name) = LOWER(?)", (body.name,)
+        "SELECT id FROM saved_views WHERE LOWER(name) = LOWER(?) AND owner_id = ?",
+        (body.name, owner),
     )
     if existing:
         raise HTTPException(
@@ -128,34 +160,37 @@ async def create_saved_view(body: SavedViewCreate) -> Dict[str, Any]:
     view_id = str(uuid.uuid4())
     await db.execute(
         """
-        INSERT INTO saved_views (id, name, filter_json)
-        VALUES (?, ?, ?)
+        INSERT INTO saved_views (id, name, filter_json, owner_id)
+        VALUES (?, ?, ?, ?)
         """,
-        (view_id, body.name, body.filter_json),
+        (view_id, body.name, body.filter_json, owner),
     )
     return {"id": view_id, "name": body.name, "created": True}
 
 
 @saved_views_router.put("/{view_id}")
-async def update_saved_view(view_id: str, body: SavedViewUpdate) -> Dict[str, Any]:
+async def update_saved_view(
+    view_id: str,
+    body: SavedViewUpdate,
+    owner: str = Depends(get_current_owner),
+) -> Dict[str, Any]:
     """
-    Overwrite name and/or filter_json for an existing view.
+    Overwrite name and/or filter_json for an existing view owned by the caller.
     Also accepts PATCH semantics — only supplied fields are updated.
     """
     db = await get_db()
 
-    row = await db.fetchone("SELECT id FROM saved_views WHERE id = ?", (view_id,))
-    if not row:
-        raise HTTPException(status_code=404, detail="Saved view not found")
+    await require_owned_saved_view(db, view_id, owner)
 
     updates: List[str] = []
     params: List[Any] = []
 
     if body.name is not None:
-        # Check for name collision with a *different* record
+        # Check for name collision with a *different* record owned by this caller
         collision = await db.fetchone(
-            "SELECT id FROM saved_views WHERE LOWER(name) = LOWER(?) AND id != ?",
-            (body.name, view_id),
+            "SELECT id FROM saved_views WHERE LOWER(name) = LOWER(?) "
+            "AND id != ? AND owner_id = ?",
+            (body.name, view_id, owner),
         )
         if collision:
             raise HTTPException(
@@ -174,17 +209,33 @@ async def update_saved_view(view_id: str, body: SavedViewUpdate) -> Dict[str, An
 
     updates.append("updated_at = datetime('now')")
     params.append(view_id)
+    params.append(owner)
 
     await db.execute(
-        f"UPDATE saved_views SET {', '.join(updates)} WHERE id = ?",
+        f"UPDATE saved_views SET {', '.join(updates)} WHERE id = ? AND owner_id = ?",
         tuple(params),
     )
     return {"id": view_id, "updated": True}
 
 
 @saved_views_router.delete("/{view_id}")
-async def delete_saved_view(view_id: str) -> Dict[str, Any]:
-    """Delete a saved view by id. Idempotent — returns 200 even if not found."""
+async def delete_saved_view(
+    view_id: str, owner: str = Depends(get_current_owner)
+) -> Dict[str, Any]:
+    """Delete a saved view owned by the caller. Idempotent — returns 200 even
+    if the view was already gone. Raises 403 if it exists but belongs to a
+    different owner, so callers can't confirm/erase other users' views."""
     db = await get_db()
-    await db.execute("DELETE FROM saved_views WHERE id = ?", (view_id,))
+
+    row = await db.fetchone(
+        "SELECT owner_id FROM saved_views WHERE id = ?", (view_id,)
+    )
+    if row is not None and row["owner_id"] != owner:
+        raise HTTPException(
+            status_code=403, detail="You do not have access to this saved view"
+        )
+
+    await db.execute(
+        "DELETE FROM saved_views WHERE id = ? AND owner_id = ?", (view_id, owner)
+    )
     return {"id": view_id, "deleted": True}

testing/backend/unit/test_saved_views.py

@@ -1,6 +1,8 @@
 from __future__ import annotations
 
 import json
+import tempfile
+
 import pytest
 import pytest_asyncio
 from httpx import AsyncClient, ASGITransport
@@ -9,6 +11,7 @@
 from backend.secuscan.saved_views import saved_views_router
 from backend.secuscan.database import Database, get_db
 import backend.secuscan.database as _db_module
+import backend.secuscan.auth as _auth_module
 from pathlib import Path
 
 
@@ -18,7 +21,9 @@
 async def app_client():
     """
     Spin up an isolated FastAPI app with an in-memory SQLite database
-    and the saved_views_router registered.
+    and the saved_views_router registered. The client authenticates as
+    the "default" owner (no X-User-Id header) using a real API key,
+    matching how routes.py's own tests exercise auth (issue #1743).
     """
     # In-memory DB — isolated per test function
     test_db = Database(":memory:")
@@ -29,12 +34,35 @@ async def app_client():
     _app = FastAPI()
     _app.include_router(saved_views_router)
 
-    transport = ASGITransport(app=_app)
-    async with AsyncClient(transport=transport, base_url="http://test") as client:
-        yield client
+    with tempfile.TemporaryDirectory() as tmp_data_dir:
+        api_key = _auth_module.init_api_key(tmp_data_dir)
+
+        transport = ASGITransport(app=_app)
+        async with AsyncClient(
+            transport=transport,
+            base_url="http://test",
+            headers={"X-Api-Key": api_key},
+        ) as client:
+            client.api_key = api_key
+            client.test_transport = transport
+            yield client
 
     await test_db.disconnect()
     _db_module.db = None
+    _auth_module._api_key = None
+
+
+@pytest_asyncio.fixture
+async def other_owner_client(app_client: AsyncClient):
+    """A second authenticated client acting as a different owner (`bob`),
+    sharing the same app/db as ``app_client`` but scoped to a different
+    X-User-Id, for cross-owner isolation tests."""
+    async with AsyncClient(
+        transport=app_client.test_transport,
+        base_url="http://test",
+        headers={"X-Api-Key": app_client.api_key, "X-User-Id": "bob"},
+    ) as client:
+        yield client
 
 
 # ─── Helpers ──────────────────────────────────────────────────────────────────
@@ -315,6 +343,89 @@ async def test_filter_json_with_null_values_rejected(app_client: AsyncClient):
     res = await app_client.post("/api/v1/saved-views", json=make_body("Null Sev", bad_preset))
     assert res.status_code == 422
 
+# ─── Auth & owner isolation (issue #1743) ────────────────────────────────────
+
+@pytest.mark.asyncio
+async def test_unauthenticated_request_rejected(app_client: AsyncClient):
+    """Requests without a valid API key/session are rejected, not served."""
+    res = await app_client.get(
+        "/api/v1/saved-views", headers={"X-Api-Key": ""}
+    )
+    assert res.status_code == 401
+
+
+@pytest.mark.asyncio
+async def test_wrong_api_key_rejected(app_client: AsyncClient):
+    """A malformed/incorrect API key is rejected."""
+    res = await app_client.get(
+        "/api/v1/saved-views", headers={"X-Api-Key": "not-the-real-key"}
+    )
+    assert res.status_code == 401
+
+
+@pytest.mark.asyncio
+async def test_list_is_scoped_to_owner(app_client: AsyncClient, other_owner_client: AsyncClient):
+    """A view created by one owner does not appear in another owner's list."""
+    await app_client.post("/api/v1/saved-views", json=make_body("Owner A's View"))
+
+    other_res = await other_owner_client.get("/api/v1/saved-views")
+    assert other_res.status_code == 200
+    assert other_res.json()["total"] == 0
+
+    own_res = await app_client.get("/api/v1/saved-views")
+    assert own_res.json()["total"] == 1
+
+
+@pytest.mark.asyncio
+async def test_different_owners_can_reuse_the_same_name(
+    app_client: AsyncClient, other_owner_client: AsyncClient
+):
+    """Per-owner uniqueness: two owners can each have a view named 'Alpha'."""
+    res_a = await app_client.post("/api/v1/saved-views", json=make_body("Alpha"))
+    res_b = await other_owner_client.post("/api/v1/saved-views", json=make_body("Alpha"))
+    assert res_a.status_code == 201
+    assert res_b.status_code == 201
+
+
+@pytest.mark.asyncio
+async def test_cannot_read_other_owners_view_by_guessing_id(
+    app_client: AsyncClient, other_owner_client: AsyncClient
+):
+    """
+    There's no GET-by-id endpoint, but PUT/DELETE both accept a bare id — this
+    verifies neither leaks or mutates another owner's row (BOLA regression).
+    """
+    create_res = await app_client.post("/api/v1/saved-views", json=make_body("Private View"))
+    view_id = create_res.json()["id"]
+
+    put_res = await other_owner_client.put(
+        f"/api/v1/saved-views/{view_id}", json={"name": "Hijacked"}
+    )
+    assert put_res.status_code == 403
+
+    del_res = await other_owner_client.delete(f"/api/v1/saved-views/{view_id}")
+    assert del_res.status_code == 403
+
+    # Confirm the original owner's view is untouched
+    list_res = await app_client.get("/api/v1/saved-views")
+    assert list_res.json()["views"][0]["name"] == "Private View"
+
+
+@pytest.mark.asyncio
+async def test_cannot_delete_other_owners_view(
+    app_client: AsyncClient, other_owner_client: AsyncClient
+):
+    """Deleting another owner's view id returns 403 and leaves it intact."""
+    create_res = await app_client.post("/api/v1/saved-views", json=make_body("Keep Safe"))
+    view_id = create_res.json()["id"]
+
+    res = await other_owner_client.delete(f"/api/v1/saved-views/{view_id}")
+    assert res.status_code == 403
+
+    list_res = await app_client.get("/api/v1/saved-views")
+    assert list_res.json()["total"] == 1
+
+
 # ── File-backed DB migration path ─────────────────────────────────────────────
 
 @pytest.mark.asyncio