From 620f84dba4ca532540ceaabe08ad19dbb47b0162 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 15 Oct 2026 05:47:00 +0200 Subject: [PATCH] fix(files): a token made as every session ends goes with them, and a file being shared holds its job --- backend/app/routes/auth.py | 16 ++++-- backend/app/routes/downloads.py | 18 +++++- backend/app/routes/tokens.py | 10 +++- backend/tests/test_api_tokens.py | 92 ++++++++++++++++++++++++++++++ backend/tests/test_folder_links.py | 70 +++++++++++++++++++++++ 5 files changed, 199 insertions(+), 7 deletions(-) diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 7ff41634..65edaf05 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -1147,10 +1147,18 @@ def bump_session_epoch(db: Session, user: User) -> None: """Invalidate every EXISTING signed cookie for this account by advancing its epoch (SA4). Cookies minted before the bump carry a now-stale epoch and are rejected by `current_user`. The account's API tokens go with them (`apitokens.revoke_all`): a token minted from a stolen session must not - outlive the answer to that theft. Not committed — the caller commits it with its change.""" - user.session_epoch = (user.session_epoch or 0) + 1 - if user.id is not None: - apitokens.revoke_all(db, user.id) + outlive the answer to that theft. Not committed — the caller commits it with its change. + + The user's row is locked before the tokens go, the lock `create_token` takes too: a token made at + the same moment either commits first and is deleted here, or waits and sees the new epoch.""" + if user.id is None: + user.session_epoch = (user.session_epoch or 0) + 1 + return + epoch = db.execute( + select(User.session_epoch).where(User.id == user.id).with_for_update() + ).scalar_one() + user.session_epoch = (epoch or 0) + 1 + apitokens.revoke_all(db, user.id) def revoke_untrusted_preset_password(db: Session, user: User) -> None: diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 873b6cf7..2c897fe1 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -797,7 +797,7 @@ def create_link( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - job = owned_or_404(db, DownloadJob, job_id, user, what="download") + job = _shareable_job(db, user, job_id) if job.status != JobStatus.DONE or job.asset_id is None: raise refused(409, "share_unfinished") link, token = _new_link(db, user, body, job_id=job_id) @@ -805,6 +805,22 @@ def create_link( return linksmod.owner_view(link, token=token) +def _shareable_job(db: Session, user: User, job_id: int) -> DownloadJob: + """One of the user's files, held (FOR SHARE) for the rest of the transaction: a delete of it at + the same moment (the user's, or the retention sweep's) waits and takes the new link with it, + rather than failing the link's foreign key — or, done first, leaves a 404.""" + owned_or_404(db, DownloadJob, job_id, user, what="download") + job = db.execute( + select(DownloadJob) + .where(DownloadJob.id == job_id) + .with_for_update(read=True) + .execution_options(populate_existing=True) + ).scalar_one_or_none() + if job is None: + raise not_found("download") + return job + + def _shareable_folder(db: Session, user: User, folder_id: int) -> FileFolder: """One of the user's own folders. A source folder lets its files go on its own clock, and the Hub is the user's channel to a machine of theirs: neither is a shelf to hand out. Held (FOR SHARE) diff --git a/backend/app/routes/tokens.py b/backend/app/routes/tokens.py index adfbcbbd..d98f570d 100644 --- a/backend/app/routes/tokens.py +++ b/backend/app/routes/tokens.py @@ -120,8 +120,14 @@ def create_token( name = body.name.strip() if not name: raise refused(400, "api_token_name_required") - # The user's row, locked: two requests at once cannot both slip under the ceiling. - db.execute(select(User.id).where(User.id == user.id).with_for_update()) + # The user's row, locked: two requests at once cannot both slip under the ceiling, and a + # session ended since `current_user` read the epoch (unlocked) cannot mint a token that outlives + # it — `bump_session_epoch` takes this lock before it revokes them all. + epoch = db.execute( + select(User.session_epoch).where(User.id == user.id).with_for_update() + ).scalar_one() + 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) ).scalar_one() diff --git a/backend/tests/test_api_tokens.py b/backend/tests/test_api_tokens.py index 211e0e98..64023c70 100644 --- a/backend/tests/test_api_tokens.py +++ b/backend/tests/test_api_tokens.py @@ -10,16 +10,19 @@ holds nothing, and a client trying tokens is slowed down. from datetime import datetime, timedelta, timezone import pytest +from fastapi import Depends, Request from fastapi.testclient import TestClient from sqlalchemy import select from app import apitokens from app.apitokens import files_caller +from app.auth import current_user from app.downloads import folders, service from app.downloads import gc as gcmod from app.downloads import upload as uploadmod from app.downloads.status import JobStatus from app.config import settings +from app.db import SessionLocal, get_db from app.main import app from app.models import ( ApiToken, @@ -29,9 +32,11 @@ from app.models import ( FileFolder, MediaAsset, Notification, + User, ) from app.ratelimit import RateLimiter from app.routes import tokens as tokens_routes +from app.routes.auth import bump_session_epoch from app.security import hash_token @@ -173,6 +178,39 @@ def test_a_token_is_granted_only_the_users_own_folders_and_writes_only_where_upl assert db.execute(select(ApiToken)).all() == [] +@pytest.mark.parametrize("right", ["read", "write"]) +def test_a_folder_granted_is_held_until_the_token_is_in(db, make_user, right): + """A delete of the folder at the same moment waits and takes the new grant with it, rather than + failing the grant's foreign key.""" + import threading + + alice = make_user() + fid = _own(db, alice.id, "Mine").id + body = tokens_routes.TokenIn( + name="x", grants=[tokens_routes.GrantIn(folder_id=fid, rights=[right])] + ) + tokens_routes._wanted_grants(db, alice, body) + outcome: list[object] = [] + + def delete_it(): + other = SessionLocal() + try: + outcome.append(folders.delete_own(other, other.get(FileFolder, fid))) + other.commit() + except Exception as e: # noqa: BLE001 — the assertion below names what went wrong + outcome.append(e) + finally: + other.close() + + t = threading.Thread(target=delete_it) + t.start() + t.join(timeout=1) + assert t.is_alive() # the delete waits for the token on its way in + db.commit() + t.join(timeout=10) + assert not isinstance(outcome[0], Exception), outcome + + def test_a_user_has_at_most_the_ceiling_of_tokens(db, make_user, as_user, monkeypatch): monkeypatch.setattr(tokens_routes, "MAX_TOKENS", 2) c = as_user(make_user()) @@ -368,6 +406,60 @@ def test_whatever_ends_every_session_ends_every_token_too(db, make_user, client, assert revoked.actor_id == alice.id and "every session" in (revoked.summary or "") +def test_a_token_asked_for_as_every_session_ends_is_refused(db, make_user, client, override_dep): + """The session was still good when its request came in, and ended before the token was made.""" + alice = make_user(email="alice@example.com") + c = _signed_in(client) + + def ended_meanwhile(request: Request, session=Depends(get_db)): + user = current_user(request, session) + other = SessionLocal() + try: + bump_session_epoch(other, other.get(User, alice.id)) + other.commit() + finally: + other.close() + return user + + override_dep(current_user, ended_meanwhile) + r = c.post("/api/tokens", json={"name": "late", "preset": "hub"}) + assert r.status_code == 401 + assert db.execute(select(ApiToken.id)).scalars().all() == [] + + +def test_a_token_made_as_every_session_ends_goes_with_them(db, make_user): + """The other order: the token is on its way in when the sessions end.""" + import threading + + alice = make_user(email="alice@example.com") + db.execute(select(User.id).where(User.id == alice.id).with_for_update()) # as create_token + raw = apitokens.mint() + db.add(ApiToken(user_id=alice.id, name="t", prefix=raw[:12], token_hash=hash_token(raw))) + db.flush() + outcome: list[object] = [] + + def end_sessions(): + other = SessionLocal() + try: + bump_session_epoch(other, other.get(User, alice.id)) + other.commit() + outcome.append("done") + except Exception as e: # noqa: BLE001 — the assertion below names what went wrong + outcome.append(e) + finally: + other.close() + + t = threading.Thread(target=end_sessions) + t.start() + t.join(timeout=1) + assert t.is_alive() # it waits for the token being made + db.commit() + t.join(timeout=10) + assert outcome == ["done"] + db.expire_all() + assert db.execute(select(ApiToken.id)).scalars().all() == [] + + # --- a token that does not hold ------------------------------------------------------------------ diff --git a/backend/tests/test_folder_links.py b/backend/tests/test_folder_links.py index 87e9f082..2bbcf4a5 100644 --- a/backend/tests/test_folder_links.py +++ b/backend/tests/test_folder_links.py @@ -325,3 +325,73 @@ def test_a_file_whose_bytes_are_gone_is_not_offered(shelf, client, db): (Path(settings.files_root) / asset.rel_path).unlink() assert _ids(client, token) == [shelf["inside"][1]] assert client.get(f"/api/public/watch/{token}/files/{gone}/file").status_code == 404 + + +def _meanwhile(work): + """Run `work(other_session)` in another thread and session, committed; (thread, outcome).""" + import threading + + from app.db import SessionLocal + + outcome: list[object] = [] + + def run(): + other = SessionLocal() + try: + outcome.append(work(other)) + other.commit() + except Exception as e: # noqa: BLE001 — the assertion below names what went wrong + outcome.append(e) + finally: + other.close() + + t = threading.Thread(target=run) + t.start() + return t, outcome + + +def test_two_opens_at_once_count_as_two_views(shelf, db): + from app.routes import public + + made = _link(shelf) + link = db.get(DownloadLink, made["id"]) + public._opened(db, link, made["token"]) # this open's count is not committed yet + t, outcome = _meanwhile( + lambda other: public._opened(other, other.get(DownloadLink, made["id"]), made["token"]) + ) + t.join(timeout=1) + db.commit() + t.join(timeout=10) + assert not isinstance(outcome[0], Exception), outcome + db.expire_all() + assert db.get(DownloadLink, made["id"]).view_count == 2 + + +def test_a_file_shared_as_it_is_deleted_takes_its_new_link_with_it(shelf, db): + from app.routes import downloads as routes + + job_id = shelf["outside"] + routes._shareable_job(db, shelf["user"], job_id) # held from the check until this commits + t, outcome = _meanwhile(lambda other: other.delete(other.get(DownloadJob, job_id))) + t.join(timeout=1) + assert t.is_alive() # the delete waits for the link on its way in + db.add(DownloadLink(job_id=job_id, token_hash="h" * 64, created_by=shelf["user"].id)) + db.commit() + t.join(timeout=10) + assert outcome == [None] + assert db.execute(select(DownloadLink.id)).scalars().all() == [] + + +def test_a_folder_shared_as_it_is_deleted_takes_its_new_link_with_it(shelf, db): + from app.routes import downloads as routes + + fid = shelf["folder"] + routes._shareable_folder(db, shelf["user"], fid) # held from the check until this commits + t, outcome = _meanwhile(lambda other: folders.delete_own(other, other.get(FileFolder, fid))) + t.join(timeout=1) + assert t.is_alive() # the delete waits for the link on its way in + db.add(DownloadLink(folder_id=fid, token_hash="h" * 64, created_by=shelf["user"].id)) + db.commit() + t.join(timeout=10) + assert not isinstance(outcome[0], Exception), outcome + assert db.execute(select(DownloadLink.id)).scalars().all() == []