diff --git a/backend/secuscan/migrations/009_add_saved_views_owner_id.sql b/backend/secuscan/migrations/009_add_saved_views_owner_id.sql new file mode 100644 index 000000000..bebd253b8 --- /dev/null +++ b/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)); \ No newline at end of file diff --git a/backend/secuscan/saved_views.py b/backend/secuscan/saved_views.py index 889ecc406..c67f1e5ed 100644 --- a/backend/secuscan/saved_views.py +++ b/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} \ No newline at end of file diff --git a/testing/backend/unit/test_saved_views.py b/testing/backend/unit/test_saved_views.py index e52b55f3c..9738e727f 100644 --- a/testing/backend/unit/test_saved_views.py +++ b/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