fix: QA follow-ups for #551 (owner-scoped usage, download CSP, settings defaults, inline errors)

CHANGELOG.md

@@ -336,6 +336,13 @@ All notable changes to this project are documented in this file. The format is b
   `tenants` resolver it selects among the user's own memberships.
 
 ### Fixed
+- `file_storage` browse page: a caller without `file_storage.manage` sees their
+  own usage and no tenant quota, instead of the tenant-wide total; the
+  authenticated download now carries the same sandbox CSP as public serving.
+- Settings editor: saving a field's default (clearing an optional field
+  included) removes its override row instead of storing it, so the field
+  reads "default" again; a field's validation error sits on its own row below
+  the control instead of wrapping beside it.
 - A runtime permission source that raises is no longer cached as empty until
   invalidated: it contributes nothing for 30 seconds (one logged traceback),
   then is retried; `invalidate_source` retries it at once (#517).

modules/file_storage/file_storage/endpoints/api.py

@@ -44,7 +44,7 @@
     StoredFileNotFoundError,
     StreamDownload,
 )
-from file_storage.serving import etag_for, not_found, thumbnail_response
+from file_storage.serving import etag_for, not_found, sandbox_headers, thumbnail_response
 
 router = APIRouter()
 
@@ -221,6 +221,7 @@ async def download_file(
             headers={
                 "Content-Disposition": content_disposition(row.filename, attachment=True),
                 "ETag": etag,
+                **sandbox_headers(),
             },
             etag=etag,
         )

modules/file_storage/file_storage/endpoints/views.py

@@ -91,8 +91,12 @@ async def browse(
             # stays None until an operator says what the ceiling is — the
             # screen reports usage rather than inventing a total.
             "backend": settings.backend,
-            "used_bytes": aggregates.used_bytes,
-            "quota_bytes": settings.quota_bytes,
+            # The quota measures the tenant, so only callers who see the whole
+            # tenant get it; everyone else sees their own usage, no ceiling.
+            "used_bytes": (
+                aggregates.used_bytes if owner is None else await service.used_bytes_by(owner)
+            ),
+            "quota_bytes": settings.quota_bytes if owner is None else None,
             "max_file_size_bytes": settings.max_file_size_bytes,
             # Above this the page uploads in resumable parts; null when the
             # backend cannot (a third-party one without multipart support).

modules/file_storage/file_storage/queries.py

@@ -161,6 +161,14 @@ async def content_type_facets(db: AsyncSession, *, created_by: str | None = None
     return [{"value": str(row[0]), "count": int(row[1])} for row in rows]
 
 
+async def used_bytes_by(db: AsyncSession, owner: str) -> int:
+    """Total size of one uploader's files, summed in the database."""
+    query = select(func.coalesce(func.sum(StoredFile.size_bytes), 0)).where(
+        StoredFile.created_by == owner
+    )
+    return int((await db.execute(query)).scalar_one())
+
+
 def public_url_for(file_id: object, filename: str) -> str:
     """The anonymous URL of a public file, with its name as a trailing segment."""
     name = quote(filename, safe="")

modules/file_storage/file_storage/reads.py

@@ -99,5 +99,9 @@ async def content_type_facets(self, *, created_by: str | None = None) -> list[di
     async def uploader_facets(self) -> list[dict]:
         return [facet.as_dict() for facet in (await self.storage_aggregates()).uploaders]
 
+    async def used_bytes_by(self, owner: str) -> int:
+        """Bytes held by one uploader's files (not cached, not tenant-wide)."""
+        return await queries.used_bytes_by(self.db, owner)
+
     async def used_bytes(self) -> int:
         return (await self.storage_aggregates()).used_bytes

modules/file_storage/file_storage/serving.py

@@ -33,15 +33,18 @@ def not_found(t) -> HTTPException:
     )
 
 
-def _headers(etag: str, cache_control: str) -> dict[str, str]:
+def sandbox_headers() -> dict[str, str]:
+    """Headers that stop served bytes from running as the app's origin."""
     return {
-        "ETag": etag,
-        "Cache-Control": cache_control,
         "X-Content-Type-Options": "nosniff",
         "Content-Security-Policy": constants.PUBLIC_CSP,
     }
 
 
+def _headers(etag: str, cache_control: str) -> dict[str, str]:
+    return {"ETag": etag, "Cache-Control": cache_control, **sandbox_headers()}
+
+
 def _not_modified(request: Request, etag: str) -> bool:
     sent = request.headers.get("if-none-match", "")
     return etag in {part.strip().removeprefix("W/") for part in sent.split(",")}

modules/file_storage/tests/test_file_storage_p3_download_csp.py

@@ -0,0 +1,34 @@
+"""Authenticated downloads carry the sandbox CSP."""
+
+from __future__ import annotations
+
+from file_storage import constants
+from file_storage_uploads_helpers import API, upload_text
+
+
+class TestDownloadSandbox:
+    async def test_streamed_download_is_sandboxed(self, tenant_client):
+        async with tenant_client("member") as a:
+            file_id = await upload_text(a.client, "a.txt")
+            resp = await a.client.get(f"{API}/files/{file_id}/download")
+
+            assert resp.status_code == 200
+            assert resp.headers["content-security-policy"] == constants.PUBLIC_CSP
+            assert resp.headers["x-content-type-options"] == "nosniff"
+            assert "attachment" in resp.headers["content-disposition"]
+
+    async def test_thumbnail_keeps_the_sandbox_csp(self, tenant_client):
+        async with tenant_client("member") as a:
+            import io
+
+            from PIL import Image
+
+            buf = io.BytesIO()
+            Image.new("RGB", (4, 4)).save(buf, "PNG")
+            resp = await a.client.post(
+                f"{API}/upload", files={"file": ("p.png", buf.getvalue(), "image/png")}
+            )
+            file_id = resp.json()["id"]
+            thumb = await a.client.get(f"{API}/files/{file_id}/thumbnail")
+            assert thumb.status_code == 200
+            assert thumb.headers["content-security-policy"] == constants.PUBLIC_CSP

modules/file_storage/tests/test_file_storage_p3_scope_usage.py

@@ -0,0 +1,46 @@
+"""Owner-scoped callers see their own usage and no tenant quota; downloads are sandboxed."""
+
+from __future__ import annotations
+
+from file_storage import constants
+from file_storage_uploads_helpers import API, INERTIA, upload_text
+
+VIEW = f"{constants.ROUTE_PREFIX_VIEW}/"
+
+
+async def _props(client) -> dict:
+    resp = await client.get(VIEW, headers=INERTIA)
+    assert resp.status_code == 200, resp.text
+    return resp.json()["props"]
+
+
+class TestUsageScope:
+    async def test_member_sees_own_usage_and_no_quota(self, tenant_client):
+        async with (
+            tenant_client("member") as a,
+            tenant_client("member", tenant_id=a.tenant_id) as b,
+            tenant_client("member", tenant_id=a.tenant_id) as empty,
+        ):
+            a.client._transport.app.state.file_storage.settings.quota_bytes = 1024**3
+            await upload_text(a.client, "a.txt")  # 2 bytes
+            await b.client.post(
+                f"{API}/upload", files={"file": ("b.txt", b"x" * 100, "text/plain")}
+            )
+
+            assert (await _props(a.client))["used_bytes"] == 2
+            assert (await _props(b.client))["used_bytes"] == 100
+            assert (await _props(empty.client))["used_bytes"] == 0
+            for who in (a, b, empty):
+                assert (await _props(who.client))["quota_bytes"] is None
+
+    async def test_manager_keeps_tenant_wide_usage_and_quota(self, tenant_client):
+        async with (
+            tenant_client("member") as a,
+            tenant_client("admin", tenant_id=a.tenant_id) as boss,
+        ):
+            boss.client._transport.app.state.file_storage.settings.quota_bytes = 1024**3
+            await upload_text(a.client, "a.txt")
+
+            props = await _props(boss.client)
+            assert props["used_bytes"] == 2
+            assert props["quota_bytes"] == 1024**3

modules/settings/settings/pages/components/ModuleFieldRow.tsx

@@ -63,12 +63,23 @@ export function ModuleFieldRow({
         <fieldset disabled={!!field.env_set} className="min-w-0 border-0 p-0">
           <FieldInput id={id} field={field} value={value} onChange={onChange} />
         </fieldset>
-        {error && <p className="mt-1 text-xs text-destructive">{error}</p>}
       </div>
 
       <div className="sm:col-span-2 lg:col-span-1 lg:justify-self-start">
         <FieldSource field={field} onReset={onReset} />
       </div>
+
+      {/* Its own grid row, spanning the control and trailing columns: inside
+          the control's cell it shared that cell with the input and wrapped a
+          word per line whenever the cell was narrow. */}
+      {error && (
+        <p
+          data-testid={`field-error-${field.name}`}
+          className="break-words text-xs text-destructive sm:col-span-1 sm:col-start-2 lg:col-span-2"
+        >
+          {error}
+        </p>
+      )}
     </div>
   );
 }

modules/settings/settings/reload.py

@@ -68,6 +68,17 @@ def _relocate(exc: ValidationError, field_name: str) -> ValidationError:
     return ValidationError.from_exception_data(exc.title, lines)
 
 
+def _is_default(cls: type[BaseSettings], field_name: str, value: Any) -> bool:
+    """Whether the validated ``value`` is what the field defaults to."""
+    info = cls.model_fields[field_name]
+    if info.is_required():
+        return False
+    default = info.get_default(call_default_factory=True)
+    return to_jsonable_python(_unwrap_secrets(default)) == to_jsonable_python(
+        _unwrap_secrets(value)
+    )
+
+
 async def apply_changes_and_reload(
     app: FastAPI,
     bus: EventBus,
@@ -95,6 +106,17 @@ async def apply_changes_and_reload(
     # rewritten to the store on every save.
     merged = current.model_dump()
     diff = {k: v for k, v in changes.items() if merged.get(k) != v}
+
+    # A row that already holds the default (stored before saving a default
+    # cleared the row) is not a change, so the diff never reaches it; without
+    # this the field stays "overridden in DB" however often it is saved.
+    unchanged = set(changes) - set(diff) - env_supplied_fields(cls)
+    if unchanged:
+        stored = await store.get_overrides(package)
+        for name in sorted(unchanged & set(stored)):
+            if _is_default(cls, name, merged.get(name)):
+                await store.clear_override(package, name)
+
     if not diff:
         return current
 
@@ -106,6 +128,7 @@ async def apply_changes_and_reload(
 
     merged.update(diff)
     validated = cls(**merged)
+    dumped_all = validated.model_dump()
 
     # A json field stores what validated, not what was sent: a field validator
     # may have turned the editor's text into the declared type (``json.loads``
@@ -116,6 +139,12 @@ async def apply_changes_and_reload(
     # are unwrapped before the validated value is made JSON-able.
     for field_name, raw_value in diff.items():
         vtype = value_type_for_field(cls, field_name)
+        if _is_default(cls, field_name, dumped_all.get(field_name, raw_value)):
+            # Saving the default is clearing the override: a stored row would
+            # keep the field labelled "overridden in DB" while holding exactly
+            # what the code would have supplied anyway.
+            await store.clear_override(package, field_name)
+            continue
         if vtype == "json":
             dumped = validated.model_dump(include={field_name}).get(field_name, raw_value)
             raw_value = to_jsonable_python(_unwrap_secrets(dumped))

modules/settings/tests-js/ModuleForm.test.tsx

@@ -102,6 +102,11 @@ describe('ModuleForm', () => {
     fireEvent.click(screen.getByRole('button', { name: 'Save' }));
     await waitFor(() => expect(screen.getByText(/item rejected/)).toBeVisible());
     expect(screen.queryByRole('alert')).not.toBeInTheDocument();
+    // The error is a row of its own beneath the control, not a child of the
+    // control's cell, where it wrapped a word per line.
+    const error = screen.getByText(/item rejected/);
+    expect(error.contains(screen.getByLabelText('retention_days'))).toBe(false);
+    expect(error.className).toContain('lg:col-span-2');
   });
 
   test('shows an error whose loc names no field at form level', async () => {

modules/settings/tests/test_settings_default_clears_override.py

@@ -0,0 +1,80 @@
+"""Saving a field's default removes its override row instead of storing it."""
+
+from __future__ import annotations
+
+import pytest
+from background_tasks.settings import DEFAULT_RETENTION_DAYS
+from settings.service import SettingService
+from settings.store import SettingsStore
+
+URL = "/api/settings/modules"
+
+
+async def _overrides(app, package: str) -> dict:
+    async with app.state.sm.db.session_factory() as db:
+        return await SettingsStore(SettingService(db)).get_overrides(package)
+
+
+async def _field(client, package: str, name: str) -> dict:
+    resp = await client.get(URL)
+    module = next(m for m in resp.json()["modules"] if m["package"] == package)
+    return next(f for f in module["fields"] if f["name"] == name)
+
+
+@pytest.mark.asyncio
+async def test_clearing_optional_field_removes_row_and_reports_default(authenticated_client, app):
+    put = authenticated_client.put
+    assert (await put(f"{URL}/file_storage", json={"quota_bytes": 1000})).status_code == 200
+    assert "quota_bytes" in await _overrides(app, "file_storage")
+
+    resp = await put(f"{URL}/file_storage", json={"quota_bytes": None})
+
+    assert resp.status_code == 200, resp.text
+    assert "quota_bytes" not in await _overrides(app, "file_storage")
+    field = await _field(authenticated_client, "file_storage", "quota_bytes")
+    assert field["source"] == "default"
+    assert not field["db_override"]
+    assert app.state.file_storage.settings.quota_bytes is None
+
+
+@pytest.mark.asyncio
+async def test_non_default_value_is_still_stored(authenticated_client, app):
+    resp = await authenticated_client.put(f"{URL}/file_storage", json={"quota_bytes": 1000})
+
+    assert resp.status_code == 200, resp.text
+    assert "quota_bytes" in await _overrides(app, "file_storage")
+    field = await _field(authenticated_client, "file_storage", "quota_bytes")
+    assert field["source"] == "db"
+
+
+@pytest.mark.asyncio
+async def test_value_equal_to_non_none_default_removes_row(authenticated_client, app):
+    put = authenticated_client.put
+    assert (await put(f"{URL}/background_tasks", json={"retention_days": 7})).status_code == 200
+    assert "retention_days" in await _overrides(app, "background_tasks")
+
+    resp = await put(f"{URL}/background_tasks", json={"retention_days": DEFAULT_RETENTION_DAYS})
+
+    assert resp.status_code == 200, resp.text
+    assert "retention_days" not in await _overrides(app, "background_tasks")
+    field = await _field(authenticated_client, "background_tasks", "retention_days")
+    assert field["source"] == "default"
+
+
+@pytest.mark.asyncio
+async def test_noop_save_removes_a_legacy_null_row(authenticated_client, app):
+    async with app.state.sm.db.session_factory() as db:
+        await SettingsStore(SettingService(db)).set_override(
+            "file_storage", "quota_bytes", "null", "json"
+        )
+        await db.commit()
+    field = await _field(authenticated_client, "file_storage", "quota_bytes")
+    assert field["db_override"]
+
+    resp = await authenticated_client.put(f"{URL}/file_storage", json={"quota_bytes": None})
+
+    assert resp.status_code == 200, resp.text
+    assert "quota_bytes" not in await _overrides(app, "file_storage")
+    field = await _field(authenticated_client, "file_storage", "quota_bytes")
+    assert field["source"] == "default"
+    assert not field["db_override"]