From 27427eef32f1db260adefea5b936ded73d990e04 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 15 Oct 2026 06:05:51 +0200 Subject: [PATCH] fix(files): ending every session and making a token lock the user without holding up rows that refer to it --- backend/app/routes/auth.py | 6 +- backend/app/routes/tokens.py | 5 +- backend/tests/test_api_tokens.py | 73 ++++++++++++++++++++++- frontend/src/components/SettingsPanel.tsx | 5 +- 4 files changed, 81 insertions(+), 8 deletions(-) diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 65edaf05..f9b40dfe 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -1150,12 +1150,14 @@ def bump_session_epoch(db: Session, user: User) -> None: 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.""" + the same moment either commits first and is deleted here, or waits and sees the new epoch. FOR NO + KEY UPDATE (`key_share`): the two still exclude each other, but a row that only refers to the user + (a notice, an audit line) is not held up — a token's expiry notice would deadlock with it.""" 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() + select(User.session_epoch).where(User.id == user.id).with_for_update(key_share=True) ).scalar_one() user.session_epoch = (epoch or 0) + 1 apitokens.revoke_all(db, user.id) diff --git a/backend/app/routes/tokens.py b/backend/app/routes/tokens.py index d98f570d..40e3edc2 100644 --- a/backend/app/routes/tokens.py +++ b/backend/app/routes/tokens.py @@ -122,9 +122,10 @@ def create_token( raise refused(400, "api_token_name_required") # 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. + # it — `bump_session_epoch` takes this lock before it revokes them all. No key update: a row that + # only refers to the user (a folder delete's audit line) must not wait on it, or the two deadlock. epoch = db.execute( - select(User.session_epoch).where(User.id == user.id).with_for_update() + select(User.session_epoch).where(User.id == user.id).with_for_update(key_share=True) ).scalar_one() if (epoch or 0) != (user.session_epoch or 0): raise refused(401, "not_authenticated") diff --git a/backend/tests/test_api_tokens.py b/backend/tests/test_api_tokens.py index 64023c70..aa7e64c9 100644 --- a/backend/tests/test_api_tokens.py +++ b/backend/tests/test_api_tokens.py @@ -12,9 +12,9 @@ from datetime import datetime, timedelta, timezone import pytest from fastapi import Depends, Request from fastapi.testclient import TestClient -from sqlalchemy import select +from sqlalchemy import select, update -from app import apitokens +from app import apitokens, audit from app.apitokens import files_caller from app.auth import current_user from app.downloads import folders, service @@ -24,6 +24,7 @@ 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.notifications import create_notification from app.models import ( ApiToken, ApiTokenGrant, @@ -427,6 +428,74 @@ def test_a_token_asked_for_as_every_session_ends_is_refused(db, make_user, clien assert db.execute(select(ApiToken.id)).scalars().all() == [] +def _meanwhile(work): + """Run `work(other_session)` in another thread and session, committed; (thread, outcome).""" + import threading + + 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_ending_every_session_never_deadlocks_with_a_tokens_expiry_notice(db, make_user): + """The notice's sweep holds the token's row and then writes a row naming the user; the sessions' + end holds the user and then deletes the token. A lock on the user that a mere reference to it + waits on would make that a deadlock — one of the two failing.""" + alice = make_user() + raw = apitokens.mint() + soon = datetime.now(timezone.utc) + timedelta(days=1) + tok = ApiToken( + user_id=alice.id, name="t", prefix=raw[:12], token_hash=hash_token(raw), expires_at=soon + ) + db.add(tok) + db.commit() + # `warn_expiring`, its two halves held apart: the token row taken… + db.execute(update(ApiToken).where(ApiToken.id == tok.id).values(expiry_warned=True)) + t, outcome = _meanwhile(lambda other: bump_session_epoch(other, other.get(User, alice.id))) + t.join(timeout=1) + assert t.is_alive() # the sessions' end waits for the token row + # …and then its notice, naming the user. + create_notification(db, alice.id, "api_token_expiring", title="t", commit=False) + db.commit() + t.join(timeout=10) + assert outcome == [None] + db.expire_all() + assert db.execute(select(ApiToken.id)).scalars().all() == [] + + +def test_a_token_granted_a_folder_being_deleted_never_deadlocks(db, make_user): + """The delete holds the folder and then writes its audit line naming the user; the token holds + the user and then waits for the folder.""" + alice = make_user() + fid = _own(db, alice.id, "Mine").id + folders.delete_own(db, db.get(FileFolder, fid)) # the folder held, as the delete route does + body = tokens_routes.TokenIn( + name="x", grants=[tokens_routes.GrantIn(folder_id=fid, rights=["read"])] + ) + t, outcome = _meanwhile( + lambda other: tokens_routes.create_token(body, other.get(User, alice.id), other) + ) + t.join(timeout=1) + assert t.is_alive() # the token waits for the folder + audit.record(db, alice.id, "file_folder_delete", target_type="file_folder", target_id=fid) + db.commit() + t.join(timeout=10) + [refusal] = outcome + assert getattr(refusal, "status_code", None) == 409, refusal # the folder went first + + 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 diff --git a/frontend/src/components/SettingsPanel.tsx b/frontend/src/components/SettingsPanel.tsx index a67c1b0e..e2995d8d 100644 --- a/frontend/src/components/SettingsPanel.tsx +++ b/frontend/src/components/SettingsPanel.tsx @@ -387,7 +387,8 @@ export function SignInMethods({ me }: { me: Me }) { setSessBusy(true); try { await api.logoutOthers(); - // Every API token went with the other sessions: the Tokens tab must not show them from cache. + // Every API token went with the other sessions: the Tokens tab asks again when it next opens, + // rather than trusting its cached list for another half minute. void qc.invalidateQueries({ queryKey: qk.apiTokens() }); notify({ level: "success", message: t("settings.account.sessions.done") }); } catch (e) { @@ -407,7 +408,7 @@ export function SignInMethods({ me }: { me: Me }) { setCurrent(""); setNext(""); // Refresh `me` so has_password flips and the section switches to "Change password". Every API - // token went with the old password, so the Tokens tab must not show them from cache either. + // token went with the old password, so the Tokens tab asks again when it next opens, too. void qc.invalidateQueries({ queryKey: qk.apiTokens() }); await qc.invalidateQueries({ queryKey: qk.me() }); notify({