From ad6ccbac0519a186e7ada250cd933b8a266a9fa5 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 19 Oct 2026 23:36:01 +0200 Subject: [PATCH] refactor(core): the core's owner filters go through userscope, each held by a second account Tokens, notifications, quota, debug sessions and the reset-link burn filtered their owner inline. Ten of the fifteen filters had no test that noticed one going missing; each now has a test where another account's rows must not count, show or change. --- backend/app/apitokens.py | 5 +- backend/app/auth.py | 3 +- backend/app/debugsession.py | 7 +- backend/app/notifications.py | 3 +- backend/app/quota.py | 3 +- backend/app/routes/auth.py | 3 +- backend/app/routes/debug_sessions.py | 3 +- backend/app/routes/notifications.py | 9 +- backend/app/routes/quota.py | 3 +- backend/app/routes/tokens.py | 5 +- backend/tests/test_owner_scoping.py | 135 +++++++++++++++++++++++++++ 11 files changed, 164 insertions(+), 15 deletions(-) create mode 100644 backend/tests/test_owner_scoping.py diff --git a/backend/app/apitokens.py b/backend/app/apitokens.py index 8ee6f4e2..d0a831b2 100644 --- a/backend/app/apitokens.py +++ b/backend/app/apitokens.py @@ -26,6 +26,7 @@ from fastapi import Depends, Request from sqlalchemy import delete, or_, select, update from sqlalchemy.orm import Session +from app.userscope import owned_by from app import audit, debugsession, logs from app.audit import AuditAction from app.auth import current_user @@ -223,7 +224,9 @@ def revoke_all(db: Session, user_id: int) -> int: change, "log out everywhere", a revoked untrusted password — ends its tokens too: one minted from a stolen session must not outlive the owner's answer to that theft. Returns how many.""" gone = ( - db.execute(delete(ApiToken).where(ApiToken.user_id == user_id).returning(ApiToken.id)) + db.execute( + delete(ApiToken).where(owned_by(ApiToken.user_id, user_id)).returning(ApiToken.id) + ) .scalars() .all() ) diff --git a/backend/app/auth.py b/backend/app/auth.py index 97485bfd..c908f924 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -4,6 +4,7 @@ from sqlalchemy import delete, func, select from sqlalchemy.orm import Session from starlette.requests import HTTPConnection +from app.userscope import owned_by from app import debugsession from app import email as email_mod from app import logs @@ -230,7 +231,7 @@ def purge_user(db: Session, user: User, background: BackgroundTasks) -> None: # request's access line, a queued row, a later background task would otherwise put rows back # under the id after this DELETE (logs.forget_user). logs.forget_user(user_id) - db.execute(delete(LogEvent).where(LogEvent.user_id == user_id)) + db.execute(delete(LogEvent).where(owned_by(LogEvent.user_id, user_id))) # Every canonically-equal invite goes, not just the literal spelling: is_allowed matches on the # canonical form, so leaving a sibling row behind would keep granting access to a mailbox whose # account was just erased — and prod really does carry approved pairs for one mailbox. diff --git a/backend/app/debugsession.py b/backend/app/debugsession.py index deaa27bc..14f94918 100644 --- a/backend/app/debugsession.py +++ b/backend/app/debugsession.py @@ -26,6 +26,7 @@ from uuid import uuid4 from sqlalchemy import and_, delete, func, not_, select, update from sqlalchemy.orm import Session, defer +from app.userscope import owned_by from app import diagnostics from app.models import DebugScreenshot, DebugSession, LogEvent, User @@ -76,7 +77,9 @@ def open_session(db: Session, user: User) -> DebugSession | None: def _unclosed(db: Session, user: User) -> DebugSession | None: return db.scalar( - select(DebugSession).where(DebugSession.user_id == user.id, DebugSession.ended_at.is_(None)) + select(DebugSession).where( + owned_by(DebugSession.user_id, user), DebugSession.ended_at.is_(None) + ) ) @@ -354,7 +357,7 @@ def stored_screenshot_bytes(db: Session, user_id: int) -> int: db.scalar( select(func.coalesce(func.sum(DebugScreenshot.size), 0)) .join(DebugSession, DebugSession.id == DebugScreenshot.session_id) - .where(DebugSession.user_id == user_id) + .where(owned_by(DebugSession.user_id, user_id)) ) or 0 ) diff --git a/backend/app/notifications.py b/backend/app/notifications.py index ddde22d6..eacb5c61 100644 --- a/backend/app/notifications.py +++ b/backend/app/notifications.py @@ -9,6 +9,7 @@ table can't grow without bound for a heavy user. from sqlalchemy import delete, select from sqlalchemy.orm import Session +from app.userscope import owned_by from app.models import Notification # Keep at most this many READ notifications per user; older read ones are pruned. Unread @@ -52,7 +53,7 @@ def trim_read(db: Session, user_id: int, cap: int = READ_SOFT_CAP) -> int: ids = ( db.execute( select(Notification.id) - .where(Notification.user_id == user_id, Notification.read.is_(True)) + .where(owned_by(Notification.user_id, user_id), Notification.read.is_(True)) .order_by(Notification.created_at.desc()) .offset(cap) ) diff --git a/backend/app/quota.py b/backend/app/quota.py index 6f2e5c81..552babb8 100644 --- a/backend/app/quota.py +++ b/backend/app/quota.py @@ -16,6 +16,7 @@ from sqlalchemy.dialects.postgresql import insert as pg_insert from sqlalchemy.engine import CursorResult from sqlalchemy.orm import Session +from app.userscope import owned_by from app import logs, sysconfig from app.db import SessionLocal from app.models import ApiQuotaUsage, QuotaEvent @@ -173,7 +174,7 @@ def actions_today(db: Session, user_id: int, action: str) -> int: select(func.count()) .select_from(QuotaEvent) .where( - QuotaEvent.user_id == user_id, + owned_by(QuotaEvent.user_id, user_id), QuotaEvent.action == action, QuotaEvent.created_at >= pacific_day_start_utc(), ) diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index f9b40dfe..336d665e 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -16,6 +16,7 @@ from sqlalchemy import select, update from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session +from app.userscope import owned_by from app import apitokens from app import email as email_mod from app import logs @@ -1124,7 +1125,7 @@ def password_reset_confirm(body: TokenPasswordIn, db: Session = Depends(get_db)) # Burn any other outstanding reset tokens for this user. for row in db.execute( select(AuthToken).where( - AuthToken.user_id == user.id, + owned_by(AuthToken.user_id, user), AuthToken.kind.in_(_PASSWORD_TOKEN_KINDS), AuthToken.used_at.is_(None), ) diff --git a/backend/app/routes/debug_sessions.py b/backend/app/routes/debug_sessions.py index 3154d149..a4288c51 100644 --- a/backend/app/routes/debug_sessions.py +++ b/backend/app/routes/debug_sessions.py @@ -18,6 +18,7 @@ from sqlalchemy import select from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session +from app.userscope import owned_by from app import audit, debugsession, logs, logsink from app.audit import AuditAction from app.auth import current_user @@ -95,7 +96,7 @@ def stop_session( def list_sessions(user: User = Depends(current_user), db: Session = Depends(get_db)) -> dict: sessions = db.scalars( select(DebugSession) - .where(DebugSession.user_id == user.id) + .where(owned_by(DebugSession.user_id, user)) .order_by(DebugSession.started_at.desc()) .limit(LIST_MAX) ).all() diff --git a/backend/app/routes/notifications.py b/backend/app/routes/notifications.py index 52d3a24e..018b1343 100644 --- a/backend/app/routes/notifications.py +++ b/backend/app/routes/notifications.py @@ -5,6 +5,7 @@ from fastapi import APIRouter, Depends, Query from sqlalchemy import func, select, update from sqlalchemy.orm import Session +from app.userscope import owned_by from app.auth import current_user from app.db import get_db from app.models import Notification, User @@ -36,7 +37,7 @@ def list_notifications( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - where = [Notification.user_id == user.id] + where = [owned_by(Notification.user_id, user)] if not include_dismissed: where.append(Notification.dismissed.is_(False)) total = db.scalar(select(func.count()).select_from(Notification).where(*where)) or 0 @@ -61,7 +62,7 @@ def unread_count(user: User = Depends(current_user), db: Session = Depends(get_d select(func.count()) .select_from(Notification) .where( - Notification.user_id == user.id, + owned_by(Notification.user_id, user), Notification.read.is_(False), Notification.dismissed.is_(False), ) @@ -86,7 +87,7 @@ def mark_read( def mark_all_read(user: User = Depends(current_user), db: Session = Depends(get_db)) -> dict: db.execute( update(Notification) - .where(Notification.user_id == user.id, Notification.read.is_(False)) + .where(owned_by(Notification.user_id, user), Notification.read.is_(False)) .values(read=True) ) db.commit() @@ -113,7 +114,7 @@ def clear_all(user: User = Depends(current_user), db: Session = Depends(get_db)) """Dismiss every (still-visible) notification for this user.""" db.execute( update(Notification) - .where(Notification.user_id == user.id, Notification.dismissed.is_(False)) + .where(owned_by(Notification.user_id, user), Notification.dismissed.is_(False)) .values(dismissed=True, read=True) ) db.commit() diff --git a/backend/app/routes/quota.py b/backend/app/routes/quota.py index 98df6d07..d4b11b94 100644 --- a/backend/app/routes/quota.py +++ b/backend/app/routes/quota.py @@ -10,6 +10,7 @@ from fastapi import APIRouter, Depends, Query from sqlalchemy import func, select from sqlalchemy.orm import Session +from app.userscope import owned_by from app import quota from app.auth import admin_user, current_user from app.db import get_db @@ -27,7 +28,7 @@ def _sum(db: Session, *conds) -> int: @router.get("/my-usage") def my_usage(user: User = Depends(current_user), db: Session = Depends(get_db)) -> dict: now = datetime.now(timezone.utc) - mine = QuotaEvent.user_id == user.id + mine = owned_by(QuotaEvent.user_id, user) by_action = { action: int(units) for action, units in db.execute( diff --git a/backend/app/routes/tokens.py b/backend/app/routes/tokens.py index 6bcae236..0abda41f 100644 --- a/backend/app/routes/tokens.py +++ b/backend/app/routes/tokens.py @@ -16,6 +16,7 @@ from pydantic import BaseModel, Field from sqlalchemy import func, select from sqlalchemy.orm import Session +from app.userscope import owned_by from app import apitokens, audit from app.apitokens import GRANT_COLUMNS, Right from app.audit import AuditAction @@ -86,7 +87,7 @@ def _serialize(db: Session, token: ApiToken) -> dict: @router.get("") def list_tokens(user: User = Depends(current_user), db: Session = Depends(get_db)) -> list[dict]: tokens = ( - db.execute(select(ApiToken).where(ApiToken.user_id == user.id).order_by(ApiToken.id)) + db.execute(select(ApiToken).where(owned_by(ApiToken.user_id, user)).order_by(ApiToken.id)) .scalars() .all() ) @@ -134,7 +135,7 @@ def create_token( if (epoch or 0) != (user.session_epoch or 0): raise refused(401, "not_authenticated") count = db.execute( - select(func.count(ApiToken.id)).where(ApiToken.user_id == user.id) + select(func.count(ApiToken.id)).where(owned_by(ApiToken.user_id, user)) ).scalar_one() if count >= MAX_TOKENS: raise refused(409, "api_token_limit", limit=MAX_TOKENS) diff --git a/backend/tests/test_owner_scoping.py b/backend/tests/test_owner_scoping.py new file mode 100644 index 00000000..54de7b9e --- /dev/null +++ b/backend/tests/test_owner_scoping.py @@ -0,0 +1,135 @@ +"""The core's owner filters, each held by a second account whose rows must not count, show or change. + +Every one of these reads or writes a user-owned table through `userscope.owned_by`; the fence holds +the form (no inline `Model.user_id == …`), these hold that the filter is there at all. Each test gives +a second account the same kind of rows, so a filter that went missing answers with theirs. DB lane. +""" + +from datetime import datetime, timedelta, timezone + +from sqlalchemy import func, select + +from app import debugsession, notifications, quota +from app.models import DebugScreenshot, DebugSession, Notification, QuotaEvent +from app.routes import tokens as tokens_routes +from app.routes.auth import RESET_TTL, _issue_token + + +def _notify(db, user, n=1, *, read=False): + for i in range(n): + db.add(Notification(user_id=user.id, type="t", title=f"n{i}", read=read)) + db.commit() + + +def _visible(db, user) -> int: + return db.scalar( + select(func.count()) + .select_from(Notification) + .where(Notification.user_id == user.id, Notification.dismissed.is_(False)) + ) + + +def test_the_notification_list_shows_only_mine(db, make_user, as_user): + me, other = make_user(), make_user() + _notify(db, me) + _notify(db, other, 2) + body = as_user(me).get("/api/me/notifications").json() + assert (body["total"], len(body["items"])) == (1, 1) + + +def test_the_unread_count_counts_only_mine(db, make_user, as_user): + me, other = make_user(), make_user() + _notify(db, me) + _notify(db, other, 2) + assert as_user(me).get("/api/me/notifications/unread_count").json() == {"count": 1} + + +def test_reading_all_reads_only_mine(db, make_user, as_user): + me, other = make_user(), make_user() + _notify(db, me) + _notify(db, other, 2) + assert as_user(me).post("/api/me/notifications/read_all").status_code == 200 + assert as_user(other).get("/api/me/notifications/unread_count").json() == {"count": 2} + + +def test_clearing_all_dismisses_only_mine(db, make_user, as_user): + me, other = make_user(), make_user() + _notify(db, me) + _notify(db, other, 2) + assert as_user(me).post("/api/me/notifications/clear").status_code == 200 + db.expire_all() + assert (_visible(db, me), _visible(db, other)) == (0, 2) + + +def test_trimming_read_notifications_trims_only_mine(db, make_user): + me, other = make_user(), make_user() + _notify(db, me, 3, read=True) + _notify(db, other, 3, read=True) + assert notifications.trim_read(db, me.id, cap=1) == 2 + db.commit() + assert _visible(db, other) == 3 + + +def test_todays_actions_count_only_mine(db, make_user): + me, other = make_user(), make_user() + quota.log_action(me.id, quota.QuotaAction.VIDEOS_SEARCH) + quota.log_action(other.id, quota.QuotaAction.VIDEOS_SEARCH) + quota.log_action(other.id, quota.QuotaAction.VIDEOS_SEARCH) + assert quota.actions_today(db, me.id, quota.QuotaAction.VIDEOS_SEARCH) == 1 + + +def test_my_usage_sums_only_mine(db, make_user, as_user): + me, other = make_user(), make_user() + db.add_all( + [ + QuotaEvent(user_id=me.id, action="videos.list", units=5), + QuotaEvent(user_id=other.id, action="videos.list", units=7), + ] + ) + db.commit() + body = as_user(me).get("/api/quota/my-usage").json() + assert (body["today"], body["all_time"], body["by_action"]) == (5, 5, {"videos.list": 5}) + + +def test_the_token_ceiling_counts_only_mine(db, make_user, as_user, monkeypatch): + monkeypatch.setattr(tokens_routes, "MAX_TOKENS", 1) + me, other = make_user(), make_user() + first = as_user(other).post("/api/tokens", json={"name": "theirs", "preset": "hub"}) + assert first.status_code == 200, first.text + r = as_user(me).post("/api/tokens", json={"name": "mine", "preset": "hub"}) + assert r.status_code == 200, r.text + + +def test_a_reset_burns_only_my_other_links(client, db, make_user): + me, other = make_user(), make_user() + mine = _issue_token(db, me, "reset", RESET_TTL) + theirs = _issue_token(db, other, "reset", RESET_TTL) + db.commit() + r = client.post( + "/auth/password-reset/confirm", json={"token": mine, "password": "a brand new passphrase"} + ) + assert r.json() == {"ok": True} + assert client.post("/auth/password-reset/check", json={"token": theirs}).json() == {"ok": True} + + +def test_screenshot_storage_counts_only_mine(db, make_user): + me, other = make_user(), make_user() + now = datetime.now(timezone.utc) + for i, (user, size) in enumerate([(me, 10), (other, 1000)]): + db.add( + DebugSession( + id=f"s{i}", + user_id=user.id, + level="DEBUG", + started_at=now, + until=now + timedelta(hours=1), + ) + ) + db.flush() + db.add( + DebugScreenshot( + session_id=f"s{i}", created_at=now, mime="image/png", size=size, data=b"x" * size + ) + ) + db.commit() + assert debugsession.stored_screenshot_bytes(db, me.id) == 10