Tech-debt batch 2: revoke sessions on disable/role change, enforce quota on plain uploads (#581 #586)

CHANGELOG.md

@@ -16,6 +16,18 @@ All notable changes to this project are documented in this file. The format is b
 ## [Unreleased]
 
 ### Fixed
+- Users (security): disabling or enabling an account, or replacing its roles from the admin
+  screens, now bumps `session_version` in the same transaction and evicts the cached counter on
+  every worker after commit (an atomic SQL increment, so overlapping changes cannot lose one), so
+  existing cookie sessions and bearer access tokens are refused on their next request; disabling
+  also revokes the account's refresh tokens. Before, a disabled or demoted user kept the roles
+  cached in their cookie until the session expired. **Behaviour change:** a role change signs that
+  user out of their existing sessions; a remembered sign-in re-authenticates with the new roles
+  (#581).
+- File storage: **Behaviour change.** `quota_bytes` is now enforced for plain uploads as well as
+  resumable ones (stored bytes plus active reservations, serialised per tenant on Postgres); an
+  upload that would exceed it answers 413 and its object is removed. Previously only resumable
+  sessions were checked, and the setting was described as report-only (#586).
 - File storage: `DELETE /api/file_storage/files/{id}` and the bulk delete now drop the stored
   bytes only after the request transaction commits, so a failed commit no longer leaves a live
   row with no bytes. A new hourly `file_storage.reap_objects` beat task (needs

docs/modules/file_storage.md

@@ -286,6 +286,7 @@ DB-backed via `register_module_settings`; pydantic defaults seed at boot. Live e
 | `s3_signature_version` | `"s3v4"` | presign signature version; pinned so custom endpoints get SigV4 |
 | `s3_presign_ttl_seconds` | `300` | TTL for presigned `GET` URLs |
 | `max_file_size_bytes` | `100 * 1024 * 1024` | upload limit (100 MB) |
+| `quota_bytes` | `None` | Enforced on plain and resumable uploads: stored bytes plus unexpired active session reservations; `None` means unlimited aggregate storage. Usage follows file-listing tenant scope; platform uploads count platform-owned files separately. PostgreSQL serializes quota admissions until the recording transaction commits. Rejected plain uploads delete the uploaded object. |
 | `allowed_content_types` | `None` | optional MIME-type whitelist; `None` ⇒ any |
 
 ## Backends

modules/branding/branding/endpoints/api.py

@@ -10,7 +10,7 @@
 from branding import constants
 from branding.contracts.schemas import BrandingOut, BrandingUpdate
 from branding.deps import BrandingServiceDep
-from branding.images import validate_image
+from branding.images import store_image, validate_image
 from branding.presets import find_preset
 
 router = APIRouter()
@@ -67,7 +67,7 @@ async def upload_logo(
     storage: FileStorageService = Depends(get_file_storage_service),
 ) -> BrandingOut:
     await validate_image(file)
-    stored = await storage.upload(file, platform=True)
+    stored = await store_image(storage, file, platform=True)
     return await service.set_logo(str(stored.id))
 
 
@@ -91,7 +91,7 @@ async def upload_logo_dark(
     storage: FileStorageService = Depends(get_file_storage_service),
 ) -> BrandingOut:
     await validate_image(file)
-    stored = await storage.upload(file, platform=True)
+    stored = await store_image(storage, file, platform=True)
     return await service.set_logo_dark(str(stored.id))
 
 
@@ -107,7 +107,7 @@ async def upload_favicon(
     storage: FileStorageService = Depends(get_file_storage_service),
 ) -> BrandingOut:
     await validate_image(file)
-    stored = await storage.upload(file, platform=True)
+    stored = await store_image(storage, file, platform=True)
     return await service.set_favicon(str(stored.id))
 
 

modules/branding/branding/endpoints/tenant_api.py

@@ -27,7 +27,7 @@
 
 from branding.constants import PACKAGE, PATH_TENANT_ASSET, TENANT_ASSETS, TENANT_IMAGE_FIELDS
 from branding.contracts.schemas import BrandingOut
-from branding.images import validate_image
+from branding.images import store_image, validate_image
 from branding.reaper import schedule_reap
 from branding.service import to_out
 from branding.tenant_branding import merge, overrides_from
@@ -83,7 +83,7 @@ async def upload_tenant_asset(
     field = _field(asset)
     tenant_id = active_tenant(request)
     await validate_image(file)
-    stored = await storage.upload(file)
+    stored = await store_image(storage, file)
     return await _swap(request, settings, storage, tenant_id, field, str(stored.id))
 
 

modules/branding/branding/images.py

@@ -12,10 +12,14 @@
 
 from __future__ import annotations
 
-from typing import Final
+from typing import TYPE_CHECKING, Final
 
 from fastapi import HTTPException, UploadFile
 
+if TYPE_CHECKING:
+    from file_storage.contracts.schemas import StoredFileOut
+    from file_storage.service import FileStorageService
+
 # Raster + icon only. SVG is excluded on purpose: it is an XML document that can
 # carry <script>, so serving one back from our own origin would be stored XSS.
 # The asset route defends in depth (attachment + nosniff), but the narrower
@@ -96,3 +100,22 @@ async def validate_image(file: UploadFile) -> None:
                 f"{content_type!r}. {ALLOWED_IMAGE_HINT}"
             ),
         )
+
+
+async def store_image(
+    storage: FileStorageService, file: UploadFile, *, platform: bool = False
+) -> StoredFileOut:
+    """Hand a validated image to ``file_storage``, answering 413 when the quota is full.
+
+    ``file_storage`` enforces ``quota_bytes`` on every upload; its own endpoint
+    maps the refusal to 413, but branding calls the service directly, so an
+    unmapped ``QuotaExceededError`` would surface as a 500.
+    """
+    from file_storage.upload_support import QuotaExceededError
+
+    try:
+        return await storage.upload(file, platform=platform)
+    except QuotaExceededError:
+        raise HTTPException(
+            status_code=413, detail="The storage quota is full; the image was not saved."
+        ) from None

modules/file_storage/file_storage/endpoints/api.py

@@ -38,6 +38,7 @@
 from file_storage.contracts.service import StorageNotFoundError
 from file_storage.deps import OwnerScope, get_event_bus, get_file_storage_service
 from file_storage.downloads import content_disposition
+from file_storage.endpoints.upload_errors import quota
 from file_storage.format import format_bytes
 from file_storage.ranges import file_body_response
 from file_storage.service import (
@@ -49,6 +50,7 @@
     StreamDownload,
 )
 from file_storage.serving import etag_for, not_found, sandbox_headers, thumbnail_response
+from file_storage.upload_support import QuotaExceededError
 
 router = APIRouter()
 
@@ -68,6 +70,8 @@ async def upload_file(
 ) -> StoredFileOut:
     try:
         out = await service.upload(file, public=public)
+    except QuotaExceededError as exc:
+        raise quota(t) from exc
     except FileTooLargeError as exc:
         # The limit belongs in the sentence: "too large" is not actionable to
         # someone holding a 40 MB file, and every client that shows this

modules/file_storage/file_storage/service.py

@@ -31,6 +31,7 @@
 from file_storage.models import StoredFile
 from file_storage.reads import FileStorageReads
 from file_storage.scope import PLATFORM_TENANT_ID, owning_tenant, platform_scope
+from file_storage.upload_guards import StorageQuota
 from file_storage.visibility import FileStoragePublic
 
 if TYPE_CHECKING:
@@ -54,7 +55,7 @@ class StoredFileNotFoundError(Exception):
     """Raised when a StoredFile lookup misses; endpoints map this to HTTP 404."""
 
 
-class FileStorageService(FileStoragePublic, FileStorageReads, FileStorageDeletes):
+class FileStorageService(FileStoragePublic, FileStorageReads, FileStorageDeletes, StorageQuota):
     """Orchestrates validation, hashing, backend IO, and DB lifecycle."""
 
     def __init__(
@@ -121,6 +122,7 @@ async def _hashing_stream() -> AsyncIterator[bytes]:
         # the cleanup error after logging; the orphaned key can be reaped
         # by a janitor sweep.
         try:
+            await self._check_quota(size, reserve=True, platform=platform)
             row = StoredFile(
                 tenant_id=tenant_id,
                 key=key,

modules/file_storage/file_storage/settings.py

@@ -82,17 +82,17 @@ class FileStorageSettings(DbBackedSettings):
         default=None,
         description="Whitelist of MIME types. None = any type allowed.",
     )
-    # Reported, not enforced: the browse screen states "1.2 GB of 5 GB used"
-    # only for installs that have said what 5 GB means to them. Left unset,
-    # the screen reports usage without inventing a ceiling, and uploads are
-    # bounded by ``max_file_size_bytes`` alone as before.
+    # Enforced for plain uploads and resumable sessions: stored bytes plus
+    # unexpired active reservations. None leaves aggregate storage unlimited.
     # Positive, and no larger than a browser can hold exactly: the screen and
     # the settings editor read it as a JavaScript number (#548).
     quota_bytes: int | None = Field(
         default=None,
         gt=0,
         le=constants.MAX_JS_SAFE_INTEGER,
-        description="Total storage the browse screen measures usage against. None = unmeasured.",
+        description=(
+            "Storage ceiling for stored files and active upload reservations. None = unlimited."
+        ),
     )
 
     public_files_cors: bool = Field(

modules/file_storage/file_storage/upload_guards.py

@@ -1,4 +1,4 @@
-"""Cleanup and quota guards for upload sessions (#454)."""
+"""Cleanup and shared quota guards for uploads."""
 
 from __future__ import annotations
 
@@ -12,7 +12,7 @@
 from file_storage import constants
 from file_storage.contracts.service import StorageError
 from file_storage.models import StoredFile, UploadSession
-from file_storage.scope import owning_tenant
+from file_storage.scope import PLATFORM_TENANT_ID, owning_tenant, platform_scope
 from file_storage.upload_support import DirectUploadUnsupportedError, QuotaExceededError
 
 if TYPE_CHECKING:
@@ -28,13 +28,62 @@
 _QUOTA_LOCK_KEY = 0x4653544F << 32
 
 
-class UploadGuards:
-    """Mixed into :class:`~file_storage.uploads.UploadSessionService`."""
+class StorageQuota:
+    """Quota checks shared by plain uploads and resumable reservations."""
 
-    files: FileStorageService
     db: AsyncSession
     settings: FileStorageSettings
 
+    async def _serialize_reservations(self, *, platform: bool = False) -> None:
+        """Make concurrent quota admissions take turns on Postgres.
+
+        Each upload sums stored + reserved bytes before recording a file or
+        reservation. Under READ COMMITTED, concurrent uploads can read the same
+        sum and all pass. The transaction-scoped lock lasts until the new row
+        becomes visible at commit. SQLite serialises writers already.
+        """
+        bind = self.db.get_bind()
+        if bind.dialect.name != "postgresql":
+            return
+        tenant = owning_tenant(self.db, platform=platform)
+        await self.db.execute(
+            text("SELECT pg_advisory_xact_lock(:key)"),
+            {"key": _QUOTA_LOCK_KEY + zlib.crc32(tenant.encode())},
+        )
+
+    async def _check_quota(self, size: int, *, reserve: bool, platform: bool = False) -> None:
+        """Refuse ``size`` bytes that would push stored (+ reserved) bytes past the quota.
+
+        Read live, not from the browse screen's cached totals: a cache a minute
+        stale would let a burst of sessions each see an empty bucket. With
+        ``reserve``, sessions still open count at their declared size, so opening
+        many small-looking sessions cannot overbook the quota.
+        """
+        quota = self.settings.quota_bytes
+        if quota is None:
+            return
+        if reserve:
+            await self._serialize_reservations(platform=platform)
+        stored_query = select(func.coalesce(func.sum(StoredFile.size_bytes), 0))
+        reserved_query = select(func.coalesce(func.sum(UploadSession.total_size), 0)).where(
+            UploadSession.status == constants.UploadStatus.ACTIVE,  # ty: ignore[invalid-argument-type]
+            UploadSession.expires_at > datetime.now(UTC),  # ty: ignore[invalid-argument-type]
+        )
+        if platform:
+            stored_query = stored_query.where(StoredFile.tenant_id == PLATFORM_TENANT_ID)
+            reserved_query = reserved_query.where(UploadSession.tenant_id == PLATFORM_TENANT_ID)
+        async with platform_scope(self.db, platform):
+            stored = await self.db.scalar(stored_query)
+            reserved = await self.db.scalar(reserved_query) if reserve else 0
+        if (stored or 0) + (reserved or 0) + size > quota:
+            raise QuotaExceededError(str(quota))
+
+
+class UploadGuards(StorageQuota):
+    """Mixed into :class:`~file_storage.uploads.UploadSessionService`."""
+
+    files: FileStorageService
+
     @property
     def backend(self) -> DirectUploadBackend:
         backend = self.files.backend
@@ -73,45 +122,3 @@ async def purge_finished(self, *, owner: str | None) -> int:
             )
         )
         return result.rowcount or 0
-
-    async def _serialize_reservations(self) -> None:
-        """Make concurrent session openers take turns on Postgres.
-
-        Each opener sums stored + reserved bytes and then inserts its own
-        reservation; under READ COMMITTED, N openers all read the same sum and
-        all pass. A transaction-scoped advisory lock (held until this request
-        commits, which is when the new reservation becomes visible) closes the
-        window. SQLite serialises writers already.
-        """
-        bind = self.db.get_bind()
-        if bind.dialect.name != "postgresql":
-            return
-        await self.db.execute(
-            text("SELECT pg_advisory_xact_lock(:key)"),
-            {"key": _QUOTA_LOCK_KEY + zlib.crc32(owning_tenant(self.db).encode())},
-        )
-
-    async def _check_quota(self, size: int, *, reserve: bool) -> None:
-        """Refuse ``size`` bytes that would push stored (+ reserved) bytes past the quota.
-
-        Read live, not from the browse screen's cached totals: a cache a minute
-        stale would let a burst of sessions each see an empty bucket. With
-        ``reserve``, sessions still open count at their declared size, so opening
-        many small-looking sessions cannot overbook the quota.
-        """
-        quota = self.settings.quota_bytes
-        if quota is None:
-            return
-        if reserve:
-            await self._serialize_reservations()
-        stored = await self.db.scalar(select(func.coalesce(func.sum(StoredFile.size_bytes), 0)))
-        reserved = 0
-        if reserve:
-            reserved = await self.db.scalar(
-                select(func.coalesce(func.sum(UploadSession.total_size), 0)).where(
-                    UploadSession.status == constants.UploadStatus.ACTIVE,  # ty: ignore[invalid-argument-type]
-                    UploadSession.expires_at > datetime.now(UTC),  # ty: ignore[invalid-argument-type]
-                )
-            )
-        if (stored or 0) + (reserved or 0) + size > quota:
-            raise QuotaExceededError(str(quota))

modules/file_storage/tests/test_file_storage_plain_quota.py

@@ -0,0 +1,59 @@
+"""Plain uploads share stored-byte limits and resumable reservations (#586)."""
+
+from file_storage import constants
+from file_storage_uploads_helpers import API, open_session
+
+
+async def upload(client, size):
+    return await client.post(
+        f"{API}/upload", files={"file": ("small.bin", b"x" * size, "application/octet-stream")}
+    )
+
+
+async def test_plain_uploads_enforce_aggregate_quota(authenticated_client, storage):
+    storage.settings.quota_bytes = 10
+    assert (await upload(authenticated_client, 6)).status_code == 201
+    assert (await upload(authenticated_client, 4)).status_code == 201
+    rejected = await upload(authenticated_client, 1)
+    assert rejected.status_code == 413
+    assert rejected.json()["detail"]["code"] == constants.ErrorCode.QUOTA_EXCEEDED
+    listing = (await authenticated_client.get(f"{API}/files")).json()
+    assert listing["total"] == 2
+    assert len([p for p in storage.backend.root.rglob("*") if p.is_file()]) == 2
+
+
+async def test_plain_upload_honors_reservations_and_abort_releases_them(
+    authenticated_client, storage, checksum_calls
+):
+    storage.settings.quota_bytes = 10
+    response = await open_session(authenticated_client, total=8)
+    assert response.status_code == 201
+    assert (await upload(authenticated_client, 3)).status_code == 413
+    assert (await upload(authenticated_client, 2)).status_code == 201
+    await authenticated_client.delete(f"{API}/uploads/{response.json()['id']}")
+    assert (await upload(authenticated_client, 8)).status_code == 201
+
+
+async def test_platform_quota_counts_only_platform_files(db_session, tmp_path):
+    from io import BytesIO
+
+    import pytest
+    from fastapi import UploadFile
+    from file_storage.backends.filesystem import FilesystemBackend
+    from file_storage.service import FileStorageService
+    from file_storage.settings import FileStorageSettings
+    from file_storage.upload_support import QuotaExceededError
+    from simple_module_db import tenant_context
+
+    backend = FilesystemBackend(root=tmp_path)
+    service = FileStorageService(db_session, backend, FileStorageSettings(quota_bytes=3))
+
+    def file(size):
+        return UploadFile(BytesIO(b"x" * size), filename="small.bin")
+
+    with tenant_context("tenant-a"):
+        await service.upload(file(3))
+    await service.upload(file(3), platform=True)
+    with pytest.raises(QuotaExceededError):
+        await service.upload(file(1), platform=True)
+    assert len([p for p in tmp_path.rglob("*") if p.is_file()]) == 2