diff --git a/docs/superpowers/specs/2026-08-06-s3-provider-prefix-design.md b/docs/superpowers/specs/2026-08-06-s3-provider-prefix-design.md new file mode 100644 index 00000000..be6d9ca4 --- /dev/null +++ b/docs/superpowers/specs/2026-08-06-s3-provider-prefix-design.md @@ -0,0 +1,135 @@ +# Configurable S3 provider + bucket folder prefix + +**Date:** 2026-08-06 +**Module:** `modules/file_storage` + +## Problem + +Two gaps in the `file_storage` module: + +1. **Provider portability.** The S3 backend exposes `s3_endpoint_url`, so + MinIO/R2 partly work, but it never constructs a `botocore.config.Config`. + That makes path-style addressing unreachable, which is the single most + common breakage on MinIO, Ceph, and any endpoint that is an IP or + `localhost`. `s3_region` is also required by the settings validator, so + region-less providers (R2 wants `auto`) fail to boot. Presigned URLs are + minted against the internal endpoint, so a browser cannot open a download + when the app reaches storage over a private network. + +2. **No folder prefix.** `service._generate_key()` emits `YYYY/MM/DD/` + at the bucket root. There is no way to confine objects to a folder, and no + way for an operator to configure one. + +## Requirements + +- Any S3-compatible provider must be usable (AWS, MinIO, R2, Wasabi, B2, + DigitalOcean Spaces, self-hosted gateways). +- All objects live under a configurable folder. +- The folder is configured **at runtime through the admin Settings UI**, not + hardcoded and not env-only. + +## Design + +### Key prefix + +New DB-backed setting in a **General** group, so it renders in the existing +per-module Settings UI and picks up hot reload via +`settings.reload.apply_changes_and_reload`: + +```python +key_prefix: str = Field(default="", json_schema_extra={"group": "General"}) +``` + +Normalized and validated by a `field_validator` on `FileStorageSettings`, so a +bad value fails at config time rather than at first upload: + +| Input | Stored | +|---|---| +| `media` | `media/` | +| `/media/` | `media/` | +| `a//b` | `a/b/` | +| `` (blank) | `` (bucket root) | +| `../escape`, `a/../b`, `C:\x` | rejected with `ValueError` | + +**Scope: generic, all backends.** One `key_prefix` applies to every backend +rather than an S3-only field. The filesystem backend nests it under +`fs_root_path` (`./uploads/media/2026/...`); S3 prepends it to the object key. + +**Application point: baked into the stored key.** The prefix is applied once, +at upload, inside `_generate_key`: + +```python +def _generate_key(filename: str, prefix: str = "") -> str: + return f"{prefix}{datetime.now(UTC):%Y/%m/%d}/{uuid.uuid4().hex}{Path(filename).suffix}" +``` + +The full prefixed key is persisted in `StoredFile.key`. Backends receive +already-qualified keys and never prepend anything themselves. + +Consequence, and the reason for this choice: editing the prefix later steers +only new uploads. Every previously stored file keeps resolving, because its row +still carries the key it was written under. The transparent-backend alternative +(store bare keys, prepend on every operation) would orphan every existing +object the moment an operator edits the prefix. + +A blank default means existing installations observe no behavior change. + +### S3 provider compatibility + +New fields in the existing **S3** settings group, all optional and inert when +blank: + +| Field | Purpose | +|---|---| +| `s3_addressing_style` | `auto` \| `path` \| `virtual`. Requires building a real `botocore.config.Config`, which `_build` does not do today. Unblocks MinIO/Ceph. | +| `s3_public_endpoint_url` | Presign against this host instead of the internal one, so browser download redirects work across a private network boundary. | +| `s3_signature_version` | Blank = botocore default. For gateways needing an explicit `s3v4`/`s3`. | +| `s3_verify_ssl` | Allow self-signed internal gateways. | + +`s3_region` drops out of the hard validator and defaults to `us-east-1`, so R2 +and region-less providers boot. `s3_bucket` remains required when the S3 +backend is selected. + +`S3Backend` holds a second set of client kwargs used *only* by +`presigned_get_url`, identical to the main set except for `endpoint_url`. When +no public endpoint is configured the two are the same object and behavior is +unchanged. Signing against the public host is correct precisely because that is +the host the browser will contact. + +`botocore` is imported lazily alongside `aioboto3`, matching the existing +pattern, so the module still loads without the `[s3]` extra installed. + +### Error handling + +Unchanged in shape. Config errors surface as `ConfigurationError` at boot +(fail-fast on a misconfigured prod deploy); per-operation failures stay +`StorageBackendError` / `StorageNotFoundError`. Prefix validation raises +`ValueError` from the pydantic validator, which the settings UI surfaces as a +field error. + +### Testing + +- Prefix normalization table, including every rejection case. +- Prefixed round-trip (put → get → delete) through both backends. +- `Config` carries path-style addressing when configured. +- Presign uses the public endpoint when set, the internal one when not. +- S3 backend builds with no region configured. +- Regression that guards the core guarantee: a pre-existing prefix-free key + still downloads after a prefix is configured. + +## Out of scope + +- **Filesystem shard repair.** `FilesystemBackend._resolve` shards on `key[:2]`, + which is `20` for every date-based key ever written, so all objects already + land in one directory. A prefix makes it `me` rather than `20` — equally + degenerate, not worse. Fixing it properly would relocate existing files on + disk. This spec corrects the misleading docstring and leaves behavior alone. +- Provider presets and a "test connection" button in the settings UI. +- Multipart upload for very large files (already deferred to v2). + +## Docs + +`modules/file_storage/README.md` is stale: it documents `SM_FILE_STORAGE_*` env +vars as the configuration path (settings are DB-backed now) and calls the +filesystem backend `local` rather than `filesystem`. Corrected alongside the +new fields. diff --git a/modules/file_storage/README.md b/modules/file_storage/README.md index 6b70103b..06c4370e 100644 --- a/modules/file_storage/README.md +++ b/modules/file_storage/README.md @@ -17,7 +17,30 @@ pip install "simple_module_file_storage[s3]" - `POST /api/file-storage/upload` multipart upload endpoint. - `GET /api/file-storage/files` (paged list), `GET /api/file-storage/files/{file_id}` (metadata), `GET /api/file-storage/files/{file_id}/download` (signed-URL redirect or stream), `DELETE /api/file-storage/files/{file_id}`. - Pluggable backend selected by the `backend` setting (`filesystem` default | `s3`); third-party backends can register under any id. -- Settings (`backend`, `s3_bucket`, `s3_region`, `s3_endpoint_url` for R2/MinIO/etc., `s3_access_key_id`, `s3_secret_access_key`, `s3_presign_ttl_seconds`) are configured from the DB settings store via the admin UI — they are not read from `SM_FILE_STORAGE_*` environment variables at runtime. +- Works with any S3-compatible provider — AWS, MinIO, Cloudflare R2, Wasabi, Backblaze B2, DigitalOcean Spaces, self-hosted gateways. +- All settings are configured from the DB settings store via the admin UI — they are **not** read from `SM_FILE_STORAGE_*` environment variables at runtime. Changing them rebuilds the backend in place; no restart needed. + +### Settings + +| Setting | Group | Notes | +|---|---|---| +| `backend` | General | `filesystem` (default) or `s3` | +| `key_prefix` | General | Folder every object is stored under, e.g. `media/`. Blank = bucket root. Applies to all backends. | +| `fs_root_path` | Filesystem | Local storage root | +| `s3_bucket` | S3 | Required when `backend=s3` | +| `s3_region` | S3 | Defaults to `us-east-1`; region-less providers (R2) accept `auto` | +| `s3_access_key_id` / `s3_secret_access_key` | S3 | Blank falls back to the ambient AWS credential chain | +| `s3_endpoint_url` | S3 | Custom endpoint for MinIO / R2. Blank uses the AWS default | +| `s3_public_endpoint_url` | S3 | Host used to *sign* download URLs when it differs from the endpoint the app connects to | +| `s3_addressing_style` | S3 | `auto` / `path` / `virtual`. Use `path` for MinIO, Ceph, and IP/localhost endpoints | +| `s3_signature_version` | S3 | e.g. `s3v4`. Blank uses the default | +| `s3_verify_ssl` | S3 | Set false only for internal gateways with self-signed certificates | +| `s3_presign_ttl_seconds` | S3 | Lifetime of download redirect URLs | +| `max_file_size_bytes` / `allowed_content_types` | Limits | Upload validation | + +**Folder prefix.** `key_prefix` is baked into each object's key at upload time and stored on the row, so changing it later affects only new uploads — every existing file keeps resolving. `media` and `/media/` both normalise to `media/`; `..` segments are rejected. + +**Presigning across a network boundary.** If the app reaches storage privately (`http://minio:9000`) but browsers need a public host, set `s3_public_endpoint_url` to `https://files.example.com`. Download URLs are signed for that host, since an S3 signature is bound to the host in the URL. ## Usage @@ -38,7 +61,17 @@ async def attach_receipt( return {"file_id": record.id} ``` -Backend and S3 credentials are configured from the admin UI at **/settings/modules → FileStorage** (DB-backed settings); switch `backend` to `s3` and fill in `s3_bucket`, `s3_region`, `s3_endpoint_url` (for MinIO/R2), `s3_access_key_id`, and `s3_secret_access_key`. +Backend and S3 credentials are configured from the admin UI at **/settings/modules → FileStorage** (DB-backed settings); switch `backend` to `s3` and fill in `s3_bucket`, `s3_access_key_id`, and `s3_secret_access_key`. Set `key_prefix` to keep every object inside one folder. + +Provider examples: + +| Provider | `s3_endpoint_url` | `s3_region` | `s3_addressing_style` | +|---|---|---|---| +| AWS S3 | *(blank)* | your region | `auto` | +| MinIO | `http://minio:9000` | `us-east-1` | `path` | +| Cloudflare R2 | `https://.r2.cloudflarestorage.com` | `auto` | `auto` | +| DigitalOcean Spaces | `https://.digitaloceanspaces.com` | your region | `auto` | +| Backblaze B2 | `https://s3..backblazeb2.com` | your region | `auto` | ## Depends on diff --git a/modules/file_storage/file_storage/backends/filesystem.py b/modules/file_storage/file_storage/backends/filesystem.py index 9b4d863d..c591ba56 100644 --- a/modules/file_storage/file_storage/backends/filesystem.py +++ b/modules/file_storage/file_storage/backends/filesystem.py @@ -20,10 +20,14 @@ class FilesystemBackend: """Stores objects on the local filesystem under ``root``. - Keys are sharded by their first two characters to keep any single - directory from accumulating millions of entries (which slows ``readdir`` - on most filesystems). A key like ``2026/04/19/abc123.png`` is written to - ``/20/2026/04/19/abc123.png``. + Keys are prefixed with their first two characters, so a key like + ``2026/04/19/abc123.png`` is written to ``/20/2026/04/19/abc123.png``. + + Note this is *not* an effective shard: every key begins with either the + year (``20``) or the configured ``key_prefix``, so in practice all objects + land in a single top-level directory and are spread only by the date + segments below it. Genuine sharding would need to key off the uuid, which + would relocate every already-stored file — so the layout stays as-is. """ backend_id = constants.BackendId.FILESYSTEM diff --git a/modules/file_storage/file_storage/backends/s3.py b/modules/file_storage/file_storage/backends/s3.py index e177cc2d..807e385b 100644 --- a/modules/file_storage/file_storage/backends/s3.py +++ b/modules/file_storage/file_storage/backends/s3.py @@ -36,7 +36,14 @@ class S3Backend: backend_id = constants.BackendId.S3 supports_presigned_url = True - def __init__(self, *, bucket: str, region: str, client_kwargs: dict[str, Any]) -> None: + def __init__( + self, + *, + bucket: str, + region: str, + client_kwargs: dict[str, Any], + presign_client_kwargs: dict[str, Any] | None = None, + ) -> None: try: import aioboto3 except ImportError as exc: @@ -49,10 +56,18 @@ def __init__(self, *, bucket: str, region: str, client_kwargs: dict[str, Any]) - self.bucket = bucket self.region = region self.client_kwargs = client_kwargs + # Presigning may need to sign against a different host than the one we + # connect to — the signature is bound to the host in the URL, so a URL + # signed for an internal endpoint is invalid at the public one. Falls + # back to the same kwargs when no public endpoint is configured. + self.presign_client_kwargs = presign_client_kwargs or client_kwargs def _client(self): return self._session.client("s3", **self.client_kwargs) + def _presign_client(self): + return self._session.client("s3", **self.presign_client_kwargs) + async def put( self, key: str, @@ -116,7 +131,7 @@ async def exists(self, key: str) -> bool: async def presigned_get_url(self, key: str, ttl_seconds: int) -> str: try: - async with self._client() as client: + async with self._presign_client() as client: return await client.generate_presigned_url( "get_object", Params={"Bucket": self.bucket, "Key": key}, @@ -139,18 +154,57 @@ def _is_not_found(exc: Exception) -> bool: } and "404" in str(exc) +def _build_botocore_config(settings: FileStorageSettings): + """Assemble a ``botocore.config.Config``, or ``None`` when all defaults. + + Addressing style and signature version are reachable *only* through this + object — they are not client kwargs — which is why non-AWS providers could + not be configured before. Returning ``None`` for an all-default setup keeps + botocore's own negotiation intact rather than freezing it. + """ + try: + from botocore.config import Config + except ImportError as exc: # pragma: no cover - aioboto3 always brings botocore + raise ConfigurationError( + "S3 backend requires the 'botocore' package (installed with aioboto3)." + ) from exc + + config_kwargs: dict[str, Any] = {} + if settings.s3_addressing_style != constants.AddressingStyle.AUTO: + config_kwargs["s3"] = {"addressing_style": settings.s3_addressing_style} + if settings.s3_signature_version: + config_kwargs["signature_version"] = settings.s3_signature_version + return Config(**config_kwargs) if config_kwargs else None + + @register_backend(constants.BackendId.S3) def _build(settings: FileStorageSettings) -> S3Backend: - if not settings.s3_bucket or not settings.s3_region: - raise ConfigurationError("S3 backend requires s3_bucket and s3_region.") - client_kwargs: dict[str, Any] = {"region_name": settings.s3_region} + if not settings.s3_bucket: + raise ConfigurationError("S3 backend requires s3_bucket.") + + region = settings.s3_region or constants.DEFAULT_S3_REGION + client_kwargs: dict[str, Any] = {"region_name": region} if settings.s3_endpoint_url: client_kwargs["endpoint_url"] = settings.s3_endpoint_url if settings.s3_access_key_id and settings.s3_secret_access_key: client_kwargs["aws_access_key_id"] = settings.s3_access_key_id client_kwargs["aws_secret_access_key"] = settings.s3_secret_access_key + if not settings.s3_verify_ssl: + client_kwargs["verify"] = False + config = _build_botocore_config(settings) + if config is not None: + client_kwargs["config"] = config + + presign_client_kwargs = client_kwargs + if settings.s3_public_endpoint_url: + presign_client_kwargs = { + **client_kwargs, + "endpoint_url": settings.s3_public_endpoint_url, + } + return S3Backend( bucket=settings.s3_bucket, - region=settings.s3_region, + region=region, client_kwargs=client_kwargs, + presign_client_kwargs=presign_client_kwargs, ) diff --git a/modules/file_storage/file_storage/constants.py b/modules/file_storage/file_storage/constants.py index e6d44eb0..653c1390 100644 --- a/modules/file_storage/file_storage/constants.py +++ b/modules/file_storage/file_storage/constants.py @@ -58,6 +58,20 @@ class BackendId: S3: Final = "s3" +class AddressingStyle: + """S3 URL addressing modes, mapped straight onto botocore's values. + + ``VIRTUAL`` puts the bucket in the hostname (``bucket.s3.amazonaws.com``); + ``PATH`` keeps it in the path (``host/bucket/key``). Providers reached by + IP or ``localhost`` — MinIO, Ceph — can only do path-style, since a bucket + hostname would not resolve. ``AUTO`` defers to botocore's own heuristic. + """ + + AUTO: Final = "auto" + PATH: Final = "path" + VIRTUAL: Final = "virtual" + + class Permission: UPLOAD: Final = "file_storage.upload" DOWNLOAD: Final = "file_storage.download" @@ -95,6 +109,12 @@ class I18nKey: # ── Defaults ───────────────────────────────────────────────────────── DEFAULT_BACKEND: Final = BackendId.FILESYSTEM DEFAULT_FS_ROOT: Final = "./uploads" +DEFAULT_KEY_PREFIX: Final = "" # blank = bucket root, preserving legacy layout +# Providers that ignore regions (R2, most MinIO deployments) still need *some* +# region string for SigV4 to compute a signature, so we default rather than +# require. ``us-east-1`` is the conventional filler. +DEFAULT_S3_REGION: Final = "us-east-1" +DEFAULT_ADDRESSING_STYLE: Final = AddressingStyle.AUTO DEFAULT_MAX_FILE_SIZE_BYTES: Final = 100 * 1024 * 1024 # 100 MB DEFAULT_PRESIGN_TTL_SECONDS: Final = 300 # 5 minutes DEFAULT_CHUNK_SIZE: Final = 64 * 1024 # 64 KB diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index ecbefb8f..13aa24fc 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -8,6 +8,7 @@ from typing import TYPE_CHECKING from fastapi import APIRouter +from simple_module_core.events import EventBus from simple_module_core.feature_flags import FeatureFlagDefinition, FeatureFlagRegistry from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta @@ -52,6 +53,38 @@ def register_settings(self, app: FastAPI) -> None: lambda s: FileStorageServices(settings=s), ) + def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> None: + """Rebuild the storage backend when file_storage settings reload. + + The backend is a singleton built once in ``on_startup`` from the + settings of that moment. Without this, editing the bucket, endpoint, or + credentials in the settings UI would swap ``services.settings`` while + every upload kept talking to the old provider until the next restart — + the config would appear to save and silently do nothing. + + ``key_prefix`` needs no rebuild (the service reads it per request), but + rebuilding on any change keeps the rule simple and construction is cheap. + """ + if app is None: + return + + import importlib + + settings_reloaded = importlib.import_module("settings.contracts.events").SettingsReloaded + from file_storage.backends import build_backend + + async def _rebuild_backend(event: settings_reloaded) -> None: + if event.package != constants.MODULE_NAME: + return + services = app.state.file_storage + services.backend = build_backend(services.settings) + logger.info( + "file_storage backend rebuilt after settings change: %s", + ", ".join(event.changed), + ) + + bus.subscribe(settings_reloaded, _rebuild_backend) + def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from file_storage.endpoints.api import router as api from file_storage.endpoints.views import router as views @@ -123,8 +156,11 @@ async def on_startup(self, app: FastAPI) -> None: logger.info("file_storage filesystem backend ready at %s", root) elif settings.backend == constants.BackendId.S3: logger.info( - "file_storage S3 backend configured for bucket=%s region=%s endpoint=%s", + "file_storage S3 backend configured for bucket=%s region=%s " + "endpoint=%s addressing=%s prefix=%s", settings.s3_bucket, settings.s3_region, settings.s3_endpoint_url or "(default)", + settings.s3_addressing_style, + settings.key_prefix or "(bucket root)", ) diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index 490a7ec9..d3b2ccd7 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -96,7 +96,7 @@ async def _hashing_stream() -> AsyncIterator[bytes]: sha.update(chunk) yield chunk - key = _generate_key(upload.filename or "file") + key = _generate_key(upload.filename or "file", self.settings.key_prefix) await self.backend.put( key, _hashing_stream(), @@ -200,11 +200,18 @@ async def delete(self, file_id: uuid.UUID) -> StoredFile: return row -def _generate_key(filename: str) -> str: - """Build a date-sharded, collision-proof key from the original filename.""" +def _generate_key(filename: str, prefix: str = "") -> str: + """Build a date-sharded, collision-proof key from the original filename. + + ``prefix`` (already normalised to ``''`` or ``'a/b/'`` by the settings + validator) is baked into the key here, once, and then persisted on the row. + Backends therefore receive fully-qualified keys and never prepend anything + themselves — which is what lets an operator change ``key_prefix`` later + without orphaning a single existing object. + """ today = datetime.now(UTC) suffix = Path(filename).suffix - return f"{today:%Y/%m/%d}/{uuid.uuid4().hex}{suffix}" + return f"{prefix}{today:%Y/%m/%d}/{uuid.uuid4().hex}{suffix}" def _to_out_dict(row: StoredFile) -> dict: diff --git a/modules/file_storage/file_storage/settings.py b/modules/file_storage/file_storage/settings.py index 82e913e1..4eaf6aec 100644 --- a/modules/file_storage/file_storage/settings.py +++ b/modules/file_storage/file_storage/settings.py @@ -13,11 +13,19 @@ from pathlib import Path -from pydantic import Field, model_validator +from pydantic import Field, field_validator, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict from file_storage import constants +_ALLOWED_ADDRESSING_STYLES = frozenset( + { + constants.AddressingStyle.AUTO, + constants.AddressingStyle.PATH, + constants.AddressingStyle.VIRTUAL, + } +) + class FileStorageSettings(BaseSettings): """Configuration for the file_storage module. @@ -31,7 +39,17 @@ class FileStorageSettings(BaseSettings): model_config = SettingsConfigDict(extra="ignore") - backend: str = constants.DEFAULT_BACKEND + backend: str = Field(default=constants.DEFAULT_BACKEND, json_schema_extra={"group": "General"}) + + key_prefix: str = Field( + default=constants.DEFAULT_KEY_PREFIX, + json_schema_extra={"group": "General"}, + description=( + "Folder every object is stored under, e.g. 'media/'. " + "Blank stores at the bucket root. Applies to all backends. " + "Changing it only affects new uploads — existing files keep working." + ), + ) # Filesystem backend fs_root_path: str = Field( @@ -41,7 +59,11 @@ class FileStorageSettings(BaseSettings): # S3-compatible backend (works with AWS S3, MinIO, R2, etc.) s3_bucket: str = Field(default="", json_schema_extra={"group": "S3"}) - s3_region: str = Field(default="", json_schema_extra={"group": "S3"}) + s3_region: str = Field( + default=constants.DEFAULT_S3_REGION, + json_schema_extra={"group": "S3"}, + description="Region name. Providers that ignore regions (R2) accept 'auto'.", + ) s3_access_key_id: str = Field(default="", json_schema_extra={"group": "S3"}) s3_secret_access_key: str = Field(default="", json_schema_extra={"group": "S3"}) s3_endpoint_url: str = Field( @@ -49,18 +71,81 @@ class FileStorageSettings(BaseSettings): json_schema_extra={"group": "S3"}, description="Custom endpoint for MinIO / R2. Blank uses AWS default.", ) + s3_public_endpoint_url: str = Field( + default="", + json_schema_extra={"group": "S3"}, + description=( + "Host used to sign download URLs, when it differs from the endpoint " + "the app itself connects to (e.g. app reaches MinIO at " + "http://minio:9000 but browsers need https://files.example.com). " + "Blank signs against the regular endpoint." + ), + ) + s3_addressing_style: str = Field( + default=constants.DEFAULT_ADDRESSING_STYLE, + json_schema_extra={"group": "S3"}, + description=( + "'auto', 'path', or 'virtual'. Use 'path' for MinIO, Ceph, and any " + "endpoint addressed by IP or localhost." + ), + ) + s3_signature_version: str = Field( + default="", + json_schema_extra={"group": "S3"}, + description="Override the signing algorithm, e.g. 's3v4'. Blank uses the default.", + ) + s3_verify_ssl: bool = Field( + default=True, + json_schema_extra={"group": "S3"}, + description="Set false only for internal gateways with self-signed certificates.", + ) s3_presign_ttl_seconds: int = Field( default=constants.DEFAULT_PRESIGN_TTL_SECONDS, json_schema_extra={"group": "S3"}, ) # Limits - max_file_size_bytes: int = constants.DEFAULT_MAX_FILE_SIZE_BYTES + max_file_size_bytes: int = Field( + default=constants.DEFAULT_MAX_FILE_SIZE_BYTES, + json_schema_extra={"group": "Limits"}, + ) allowed_content_types: list[str] | None = Field( default=None, + json_schema_extra={"group": "Limits"}, description="Whitelist of MIME types. None = any type allowed.", ) + @field_validator("key_prefix", mode="after") + @classmethod + def _normalise_key_prefix(cls, value: str) -> str: + """Canonicalise the prefix to ``''`` or ``'a/b/'``. + + Operators type this by hand in the settings UI, so we accept the + obvious variants (``media``, ``/media``, ``media/``, ``a//b``) rather + than rejecting them. Traversal segments are rejected outright: with the + filesystem backend a ``..`` would escape ``fs_root_path``, and the + backend's own guard raises a generic StorageBackendError only at upload + time — far too late to be actionable. + """ + cleaned = value.strip().replace("\\", "/") + segments = [part for part in cleaned.split("/") if part not in ("", ".")] + if any(part == ".." for part in segments): + raise ValueError(f"key_prefix must not contain '..' segments: {value!r}") + if any(":" in part for part in segments): + raise ValueError(f"key_prefix must be a relative folder path: {value!r}") + return f"{'/'.join(segments)}/" if segments else "" + + @field_validator("s3_addressing_style", mode="after") + @classmethod + def _validate_addressing_style(cls, value: str) -> str: + normalised = value.strip().lower() + if normalised not in _ALLOWED_ADDRESSING_STYLES: + raise ValueError( + f"s3_addressing_style must be one of " + f"{sorted(_ALLOWED_ADDRESSING_STYLES)}, got {value!r}." + ) + return normalised + @model_validator(mode="after") def _validate_backend_config(self) -> FileStorageSettings: """Enforce per-backend required fields. @@ -68,18 +153,13 @@ def _validate_backend_config(self) -> FileStorageSettings: We validate at config time rather than at backend construction so misconfigured prod boots fail fast with a clear message instead of deferring the error until the first upload. + + Only the bucket is genuinely required: a region always has a usable + default, since providers that ignore regions still need a filler string + for SigV4 rather than a correct one. """ - if self.backend == constants.BackendId.S3: - missing = [ - name - for name, value in ( - ("s3_bucket", self.s3_bucket), - ("s3_region", self.s3_region), - ) - if not value - ] - if missing: - raise ValueError(f"S3 backend selected but {', '.join(missing)} is not set.") + if self.backend == constants.BackendId.S3 and not self.s3_bucket: + raise ValueError("S3 backend selected but s3_bucket is not set.") return self def resolved_fs_root(self) -> Path: diff --git a/modules/file_storage/tests/test_file_storage_prefix_config.py b/modules/file_storage/tests/test_file_storage_prefix_config.py new file mode 100644 index 00000000..14572bb4 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_prefix_config.py @@ -0,0 +1,200 @@ +"""Key-prefix normalisation and S3 provider-compatibility client construction. + +These are pure unit tests — no moto server — covering the settings validators +and the kwargs handed to aioboto3. The end-to-end prefixed round-trip lives in +``test_s3_backend.py`` where the moto fixtures are. +""" + +from __future__ import annotations + +import pytest +from file_storage import constants +from file_storage.backends.s3 import _build, _build_botocore_config +from file_storage.service import _generate_key +from file_storage.settings import FileStorageSettings +from pydantic import ValidationError + +_BUCKET = "test-bucket" + + +def _s3_settings(**overrides) -> FileStorageSettings: + base = { + "backend": constants.BackendId.S3, + "s3_bucket": _BUCKET, + "s3_access_key_id": "test", + "s3_secret_access_key": "test", + } + return FileStorageSettings(**{**base, **overrides}) + + +# ── key_prefix normalisation ───────────────────────────────────────── + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ("", ""), + (" ", ""), + ("/", ""), + ("media", "media/"), + ("media/", "media/"), + ("/media", "media/"), + ("/media/", "media/"), + ("a//b", "a/b/"), + ("a/b/c", "a/b/c/"), + ("./media", "media/"), + ("uploads\\media", "uploads/media/"), + ], +) +def test_key_prefix_normalises_to_canonical_form(raw: str, expected: str): + assert FileStorageSettings(key_prefix=raw).key_prefix == expected + + +@pytest.mark.parametrize("raw", ["..", "../escape", "a/../b", "media/..", "C:/media"]) +def test_key_prefix_rejects_traversal_and_absolute_paths(raw: str): + """Rejected at config time — the filesystem backend's own guard would only + surface this as a generic error at first upload.""" + with pytest.raises(ValidationError): + FileStorageSettings(key_prefix=raw) + + +def test_key_prefix_defaults_to_bucket_root(): + assert FileStorageSettings().key_prefix == "" + + +# ── key generation ─────────────────────────────────────────────────── + + +def test_generate_key_without_prefix_keeps_legacy_layout(): + key = _generate_key("photo.png") + assert not key.startswith("/") + assert key.endswith(".png") + assert key.count("/") == 3 # YYYY/MM/DD/.png + + +def test_generate_key_bakes_prefix_into_the_key(): + key = _generate_key("photo.png", "media/") + assert key.startswith("media/") + assert key.endswith(".png") + + +def test_generate_key_supports_nested_prefix(): + assert _generate_key("a.txt", "tenant/uploads/").startswith("tenant/uploads/") + + +# ── S3 provider compatibility ──────────────────────────────────────── + + +def test_addressing_style_defaults_to_auto_and_builds_no_config(): + """All-default config must stay ``None`` so botocore keeps negotiating.""" + assert _build_botocore_config(_s3_settings()) is None + + +def test_path_addressing_reaches_botocore_config(): + config = _build_botocore_config(_s3_settings(s3_addressing_style="path")) + assert config is not None + assert config.s3["addressing_style"] == "path" + + +def test_signature_version_reaches_botocore_config(): + config = _build_botocore_config(_s3_settings(s3_signature_version="s3v4")) + assert config is not None + assert config.signature_version == "s3v4" + + +@pytest.mark.parametrize("raw", ["PATH", " path "]) +def test_addressing_style_accepts_untidy_operator_input(raw: str): + assert _s3_settings(s3_addressing_style=raw).s3_addressing_style == "path" + + +def test_addressing_style_rejects_unknown_value(): + with pytest.raises(ValidationError): + _s3_settings(s3_addressing_style="bucket-hostname") + + +def test_s3_builds_without_an_explicit_region(): + """R2 and other region-less providers must boot; region is filler for SigV4.""" + backend = _build(_s3_settings(s3_region="")) + assert backend.region == constants.DEFAULT_S3_REGION + assert backend.client_kwargs["region_name"] == constants.DEFAULT_S3_REGION + + +def test_s3_backend_requires_only_a_bucket(): + with pytest.raises(ValidationError): + _s3_settings(s3_bucket="") + + +def test_presign_uses_the_same_endpoint_when_no_public_one_is_set(): + backend = _build(_s3_settings(s3_endpoint_url="http://minio:9000")) + assert backend.presign_client_kwargs["endpoint_url"] == "http://minio:9000" + + +def test_presign_uses_the_public_endpoint_when_configured(): + """The app talks to MinIO internally; browsers must get a reachable host.""" + backend = _build( + _s3_settings( + s3_endpoint_url="http://minio:9000", + s3_public_endpoint_url="https://files.example.com", + ) + ) + assert backend.client_kwargs["endpoint_url"] == "http://minio:9000" + assert backend.presign_client_kwargs["endpoint_url"] == "https://files.example.com" + # Credentials and config must carry across to the presigning client, or the + # signature would be computed with different material than the request. + assert backend.presign_client_kwargs["aws_access_key_id"] == "test" + + +def test_verify_ssl_disabled_reaches_client_kwargs(): + assert _build(_s3_settings(s3_verify_ssl=False)).client_kwargs["verify"] is False + + +def test_verify_ssl_enabled_leaves_kwargs_untouched(): + assert "verify" not in _build(_s3_settings()).client_kwargs + + +# ── live reconfiguration ───────────────────────────────────────────── + + +@pytest.mark.anyio +async def test_settings_reload_rebuilds_the_backend(app): + """Editing storage config in the admin UI must take effect without a restart. + + The backend is built once at startup, so without the SettingsReloaded + subscription the settings would save while uploads kept using the old + provider — the change would appear to apply and silently do nothing. + """ + from settings.contracts.events import SettingsReloaded + + state = app.state.file_storage + original = state.backend + assert original.backend_id == constants.BackendId.FILESYSTEM + + state.settings = state.settings.model_copy( + update={ + "backend": constants.BackendId.S3, + "s3_bucket": _BUCKET, + "s3_access_key_id": "test", + "s3_secret_access_key": "test", + } + ) + await app.state.sm.event_bus.publish( + SettingsReloaded(package="file_storage", changed=("backend", "s3_bucket")) + ) + + assert state.backend is not original + assert state.backend.backend_id == constants.BackendId.S3 + assert state.backend.bucket == _BUCKET + + +@pytest.mark.anyio +async def test_settings_reload_for_another_module_leaves_the_backend_alone(app): + from settings.contracts.events import SettingsReloaded + + state = app.state.file_storage + original = state.backend + + await app.state.sm.event_bus.publish( + SettingsReloaded(package="users", changed=("oauth_microsoft_client_id",)) + ) + + assert state.backend is original diff --git a/modules/file_storage/tests/test_s3_backend.py b/modules/file_storage/tests/test_s3_backend.py index ec3d9589..2ab5bee9 100644 --- a/modules/file_storage/tests/test_s3_backend.py +++ b/modules/file_storage/tests/test_s3_backend.py @@ -101,3 +101,53 @@ async def test_s3_presigned_url_format(s3_settings): assert url.startswith("http") assert "any/key.txt" in url assert "X-Amz-Signature" in url or "Signature" in url + + +async def test_s3_roundtrip_under_a_folder_prefix(s3_settings, moto_endpoint): + """Prefixed keys are ordinary keys to the backend — it never prepends.""" + key = "media/2026/04/19/abc.bin" + backend = build_backend(s3_settings) + payload = b"prefixed contents" + await backend.put( + key, + _bytes_stream(payload), + content_type="application/octet-stream", + size=len(payload), + ) + + assert await backend.exists(key) is True + assert await _drain(backend.get(key)) == payload + + # The object really lives under the folder in the bucket. + listing = boto3.client( + "s3", + region_name=_REGION, + endpoint_url=moto_endpoint, + aws_access_key_id="test", + aws_secret_access_key="test", + ).list_objects_v2(Bucket=_BUCKET, Prefix="media/") + assert [obj["Key"] for obj in listing["Contents"]] == [key] + + await backend.delete(key) + assert await backend.exists(key) is False + + +async def test_s3_path_addressing_still_reaches_the_bucket(s3_settings): + """Path-style is what MinIO/Ceph need; prove it round-trips, not just that + the Config object was built.""" + settings = s3_settings.model_copy(update={"s3_addressing_style": "path"}) + backend = build_backend(settings) + payload = b"path style" + await backend.put( + "p/one.bin", _bytes_stream(payload), content_type="application/octet-stream", size=10 + ) + assert await _drain(backend.get("p/one.bin")) == payload + + +async def test_s3_presigned_url_points_at_the_public_endpoint(s3_settings): + settings = s3_settings.model_copy( + update={"s3_public_endpoint_url": "https://files.example.com"} + ) + url = await build_backend(settings).presigned_get_url("media/a.txt", ttl_seconds=300) + assert url.startswith("https://files.example.com") + assert "media/a.txt" in url diff --git a/modules/file_storage/tests/test_service.py b/modules/file_storage/tests/test_service.py index 16b6491e..48104f03 100644 --- a/modules/file_storage/tests/test_service.py +++ b/modules/file_storage/tests/test_service.py @@ -194,6 +194,65 @@ async def presigned_get_url(self, key, ttl_seconds): assert download2.url.startswith("https://signed/") +async def test_upload_stores_file_under_configured_prefix(tmp_path, db_session: AsyncSession): + settings = _settings(tmp_path, key_prefix="media") + svc = FileStorageService(db_session, FilesystemBackend(root=tmp_path), settings) + + out = await svc.upload(_upload("note.txt", b"hello", "text/plain")) + + assert out.key.startswith("media/") + assert await _drain((await svc.download(out.id)).body) == b"hello" + + +async def test_prefix_applies_to_the_filesystem_backend_on_disk(tmp_path, db_session: AsyncSession): + settings = _settings(tmp_path, key_prefix="media") + svc = FileStorageService(db_session, FilesystemBackend(root=tmp_path), settings) + + await svc.upload(_upload("note.txt", b"hello", "text/plain")) + + assert list(tmp_path.rglob("media/**/*.txt")), f"no file under media/ in {tmp_path}" + + +async def test_blank_prefix_preserves_the_legacy_root_layout(tmp_path, db_session: AsyncSession): + svc = FileStorageService(db_session, FilesystemBackend(root=tmp_path), _settings(tmp_path)) + + out = await svc.upload(_upload("note.txt", b"hello", "text/plain")) + + assert out.key[:4].isdigit() # starts with the year, as before + + +async def test_existing_files_keep_resolving_after_prefix_changes( + tmp_path, db_session: AsyncSession +): + """The reason the prefix is baked into the stored key rather than applied + transparently by the backend: editing it must not orphan stored objects.""" + backend = FilesystemBackend(root=tmp_path) + + before = await FileStorageService(db_session, backend, _settings(tmp_path)).upload( + _upload("old.txt", b"old bytes", "text/plain") + ) + + # Operator sets a prefix in the settings UI, and later changes it again. + for prefix in ("media", "files"): + svc = FileStorageService(db_session, backend, _settings(tmp_path, key_prefix=prefix)) + assert await _drain((await svc.download(before.id)).body) == b"old bytes" + + new = await svc.upload(_upload("new.txt", b"new bytes", "text/plain")) + assert new.key.startswith(f"{prefix}/") + assert await _drain((await svc.download(new.id)).body) == b"new bytes" + + +async def test_delete_removes_the_prefixed_object(tmp_path, db_session: AsyncSession): + settings = _settings(tmp_path, key_prefix="media") + backend = FilesystemBackend(root=tmp_path) + svc = FileStorageService(db_session, backend, settings) + + out = await svc.upload(_upload("note.txt", b"hello", "text/plain")) + await svc.delete(out.id) + + assert await backend.exists(out.key) is False + + async def _drain(stream: AsyncIterator[bytes]) -> bytes: out = b"" async for chunk in stream: