fix(files): a token made as every session ends goes with them, and a file being shared holds its job
This commit is contained in:
@@ -1147,9 +1147,17 @@ def bump_session_epoch(db: Session, user: User) -> None:
|
|||||||
"""Invalidate every EXISTING signed cookie for this account by advancing its epoch (SA4). Cookies
|
"""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
|
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
|
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."""
|
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
|
user.session_epoch = (user.session_epoch or 0) + 1
|
||||||
if user.id is not None:
|
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)
|
apitokens.revoke_all(db, user.id)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -797,7 +797,7 @@ def create_link(
|
|||||||
user: User = Depends(current_user),
|
user: User = Depends(current_user),
|
||||||
db: Session = Depends(get_db),
|
db: Session = Depends(get_db),
|
||||||
) -> dict:
|
) -> 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:
|
if job.status != JobStatus.DONE or job.asset_id is None:
|
||||||
raise refused(409, "share_unfinished")
|
raise refused(409, "share_unfinished")
|
||||||
link, token = _new_link(db, user, body, job_id=job_id)
|
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)
|
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:
|
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
|
"""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)
|
is the user's channel to a machine of theirs: neither is a shelf to hand out. Held (FOR SHARE)
|
||||||
|
|||||||
@@ -120,8 +120,14 @@ def create_token(
|
|||||||
name = body.name.strip()
|
name = body.name.strip()
|
||||||
if not name:
|
if not name:
|
||||||
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.
|
# The user's row, locked: two requests at once cannot both slip under the ceiling, and a
|
||||||
db.execute(select(User.id).where(User.id == user.id).with_for_update())
|
# 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(
|
count = db.execute(
|
||||||
select(func.count(ApiToken.id)).where(ApiToken.user_id == user.id)
|
select(func.count(ApiToken.id)).where(ApiToken.user_id == user.id)
|
||||||
).scalar_one()
|
).scalar_one()
|
||||||
|
|||||||
@@ -10,16 +10,19 @@ holds nothing, and a client trying tokens is slowed down.
|
|||||||
from datetime import datetime, timedelta, timezone
|
from datetime import datetime, timedelta, timezone
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
from fastapi import Depends, Request
|
||||||
from fastapi.testclient import TestClient
|
from fastapi.testclient import TestClient
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
|
|
||||||
from app import apitokens
|
from app import apitokens
|
||||||
from app.apitokens import files_caller
|
from app.apitokens import files_caller
|
||||||
|
from app.auth import current_user
|
||||||
from app.downloads import folders, service
|
from app.downloads import folders, service
|
||||||
from app.downloads import gc as gcmod
|
from app.downloads import gc as gcmod
|
||||||
from app.downloads import upload as uploadmod
|
from app.downloads import upload as uploadmod
|
||||||
from app.downloads.status import JobStatus
|
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.main import app
|
from app.main import app
|
||||||
from app.models import (
|
from app.models import (
|
||||||
ApiToken,
|
ApiToken,
|
||||||
@@ -29,9 +32,11 @@ from app.models import (
|
|||||||
FileFolder,
|
FileFolder,
|
||||||
MediaAsset,
|
MediaAsset,
|
||||||
Notification,
|
Notification,
|
||||||
|
User,
|
||||||
)
|
)
|
||||||
from app.ratelimit import RateLimiter
|
from app.ratelimit import RateLimiter
|
||||||
from app.routes import tokens as tokens_routes
|
from app.routes import tokens as tokens_routes
|
||||||
|
from app.routes.auth import bump_session_epoch
|
||||||
from app.security import hash_token
|
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() == []
|
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):
|
def test_a_user_has_at_most_the_ceiling_of_tokens(db, make_user, as_user, monkeypatch):
|
||||||
monkeypatch.setattr(tokens_routes, "MAX_TOKENS", 2)
|
monkeypatch.setattr(tokens_routes, "MAX_TOKENS", 2)
|
||||||
c = as_user(make_user())
|
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 "")
|
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 ------------------------------------------------------------------
|
# --- a token that does not hold ------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -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()
|
(Path(settings.files_root) / asset.rel_path).unlink()
|
||||||
assert _ids(client, token) == [shelf["inside"][1]]
|
assert _ids(client, token) == [shelf["inside"][1]]
|
||||||
assert client.get(f"/api/public/watch/{token}/files/{gone}/file").status_code == 404
|
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() == []
|
||||||
|
|||||||
Reference in New Issue
Block a user