fix(files): ending every session and making a token lock the user without holding up rows that refer to it
This commit is contained in:
@@ -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.
|
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 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:
|
if user.id is None:
|
||||||
user.session_epoch = (user.session_epoch or 0) + 1
|
user.session_epoch = (user.session_epoch or 0) + 1
|
||||||
return
|
return
|
||||||
epoch = db.execute(
|
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()
|
).scalar_one()
|
||||||
user.session_epoch = (epoch or 0) + 1
|
user.session_epoch = (epoch or 0) + 1
|
||||||
apitokens.revoke_all(db, user.id)
|
apitokens.revoke_all(db, user.id)
|
||||||
|
|||||||
@@ -122,9 +122,10 @@ def create_token(
|
|||||||
raise refused(400, "api_token_name_required")
|
raise refused(400, "api_token_name_required")
|
||||||
# The user's row, locked: two requests at once cannot both slip under the ceiling, and a
|
# 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
|
# 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(
|
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()
|
).scalar_one()
|
||||||
if (epoch or 0) != (user.session_epoch or 0):
|
if (epoch or 0) != (user.session_epoch or 0):
|
||||||
raise refused(401, "not_authenticated")
|
raise refused(401, "not_authenticated")
|
||||||
|
|||||||
@@ -12,9 +12,9 @@ from datetime import datetime, timedelta, timezone
|
|||||||
import pytest
|
import pytest
|
||||||
from fastapi import Depends, Request
|
from fastapi import Depends, Request
|
||||||
from fastapi.testclient import TestClient
|
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.apitokens import files_caller
|
||||||
from app.auth import current_user
|
from app.auth import current_user
|
||||||
from app.downloads import folders, service
|
from app.downloads import folders, service
|
||||||
@@ -24,6 +24,7 @@ from app.downloads.status import JobStatus
|
|||||||
from app.config import settings
|
from app.config import settings
|
||||||
from app.db import SessionLocal, get_db
|
from app.db import SessionLocal, get_db
|
||||||
from app.main import app
|
from app.main import app
|
||||||
|
from app.notifications import create_notification
|
||||||
from app.models import (
|
from app.models import (
|
||||||
ApiToken,
|
ApiToken,
|
||||||
ApiTokenGrant,
|
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() == []
|
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):
|
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."""
|
"""The other order: the token is on its way in when the sessions end."""
|
||||||
import threading
|
import threading
|
||||||
|
|||||||
@@ -387,7 +387,8 @@ export function SignInMethods({ me }: { me: Me }) {
|
|||||||
setSessBusy(true);
|
setSessBusy(true);
|
||||||
try {
|
try {
|
||||||
await api.logoutOthers();
|
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() });
|
void qc.invalidateQueries({ queryKey: qk.apiTokens() });
|
||||||
notify({ level: "success", message: t("settings.account.sessions.done") });
|
notify({ level: "success", message: t("settings.account.sessions.done") });
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
@@ -407,7 +408,7 @@ export function SignInMethods({ me }: { me: Me }) {
|
|||||||
setCurrent("");
|
setCurrent("");
|
||||||
setNext("");
|
setNext("");
|
||||||
// Refresh `me` so has_password flips and the section switches to "Change password". Every API
|
// 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() });
|
void qc.invalidateQueries({ queryKey: qk.apiTokens() });
|
||||||
await qc.invalidateQueries({ queryKey: qk.me() });
|
await qc.invalidateQueries({ queryKey: qk.me() });
|
||||||
notify({
|
notify({
|
||||||
|
|||||||
Reference in New Issue
Block a user