diff --git a/docker/compose.yaml b/docker/compose.yaml index a769a26c..37e9489a 100644 --- a/docker/compose.yaml +++ b/docker/compose.yaml @@ -12,7 +12,15 @@ services: - ..:/workspace - api-venv:/workspace/.venv # keep the container venv off the host checkout environment: - VISIONSET_DEV_TOKEN: dev-token + # Which workspace this server serves. Inside the bind mount, and + # **/workspace-data/ is already git-ignored, so nothing it writes can be + # committed by accident. + # + # Compose does not create it — `WorkspaceService.init` does. Until it + # exists, /health answers normally and every protected route answers + # 500 NOT_A_WORKSPACE, which is the intended legible failure rather than + # a server that silently serves nothing. + VISIONSET_WORKSPACE: /workspace/workspace-data/dev command: uv run uvicorn visionset.server.main:app --reload --host 0.0.0.0 --port 8000 ports: - "8000:8000" diff --git a/docs/README.md b/docs/README.md index 0f5a6bd2..63117207 100644 --- a/docs/README.md +++ b/docs/README.md @@ -21,3 +21,4 @@ contracts (kernel purity, headless annotator) are described there and enforced i | [persistence.md](persistence.md) | The metadata store: repositories, unit of work, table layout, migrations and `format_version` | | [examples.md](examples.md) | The two runnable examples: the whole cycle in one pass, ingest on its own, and what each is built to demonstrate | | [api.md](api.md) | The REST surface: the one error body, why clients branch on `code` and not on the status, what decides 404 / 409 / 422, what a 5xx does and does not tell you, and which codes are worth retrying | +| [auth.md](auth.md) | Who may call it: per-workspace API tokens, why only a digest is stored, why every refusal is one identical 401, immediate revocation, and how a protected route is built | diff --git a/docs/api.md b/docs/api.md index 65741b09..cab819ef 100644 --- a/docs/api.md +++ b/docs/api.md @@ -7,6 +7,21 @@ endpoint — what a failure looks like, and how to read one. The routes themselves are described by [`openapi.json`](../openapi.json) at the repo root, which is generated (`uv run python scripts/export_openapi.py`) and diffed in CI. Never hand-edit it. +## Authentication + +Every endpoint except `/health` requires a workspace API token: + +``` +Authorization: Bearer vst_hK3n... +``` + +Missing, malformed, unknown and revoked are one identical **401** with a `WWW-Authenticate: +Bearer` challenge — deliberately indistinguishable, so a client cannot use the response to probe +which credentials exist. Tokens come from `visionset token create`; the server serves the single +workspace named by `VISIONSET_WORKSPACE`, and one pointed at something else answers 500 +`NOT_A_WORKSPACE`. See [auth.md](auth.md) for the whole picture, including how to build a +protected route. + ## The error body Every failure — a domain refusal, a missing route, a malformed payload, an unhandled bug — @@ -170,3 +185,8 @@ inside its own `unit_of_work()`. **422 is declared at app level, and that is load-bearing.** It displaces FastAPI's generated `HTTPValidationError`, keeping that model — and the second error shape it implies — out of `openapi.json` entirely. A test asserts it never comes back. + +**401 is *not* declared at app level, and that is load-bearing too.** `/health` is public and +cannot 401, so the guard and its documented response travel together on the router — +`protected_router()` in `server/dependencies.py`. Build every non-public router with it rather +than repeating `Depends(require_token)` per route; see [auth.md](auth.md). diff --git a/docs/auth.md b/docs/auth.md new file mode 100644 index 00000000..59601fbf --- /dev/null +++ b/docs/auth.md @@ -0,0 +1,137 @@ +# Authentication + +A VisionSet workspace is operated with an **API token**. There is one kind of credential, it is +scoped to one workspace, and holding a valid one means holding the whole workspace: granular +permissions are deliberately not here. + +``` +visionset token create --name ci # prints the secret once — issue #26 +``` + +``` +Authorization: Bearer vst_hK3n... +``` + +Every REST endpoint except `/health` requires it. The CLI and the MCP server do not: they call +the SDK in the same process, on a machine whose filesystem the caller already has. + +## The token + +| Field | | +| --- | --- | +| `name` | What an operator calls it. Unique per workspace, case-insensitively. | +| `created_at` | When it was issued. | +| `revoked_at` | When it was burned, or absent while it still works. | + +The secret itself is **not** stored — only its SHA-256 digest. `TokenService.create` returns the +plaintext exactly once, in an `IssuedToken`, and nothing can recover it afterwards. The remedy +for a lost token is a new token. + +> ### Why a digest and not a password KDF +> +> A KDF (argon2, bcrypt) exists to make *low-entropy, human-chosen* input expensive to guess. A +> VisionSet secret is 256 bits from `secrets.token_urlsafe`: there is no dictionary to run and no +> guessing budget that terminates. Two more reasons make it the right call rather than merely a +> defensible one. Verification runs on **every request** and compares the presentation against +> every token the workspace holds, so a 100 ms KDF would cost N × 100 ms *per request* — the +> opposite cost model to a login form, where the check runs once and rate limiting bounds it. And +> `hashlib` is stdlib, where a KDF is a dependency taken on for no gain. +> +> The accepted consequence, stated rather than hidden: the digest is unsalted and deterministic, +> so two identical secrets hash identically. That requires drawing the same 256-bit value twice — +> and it is exactly the property that lets verification be a digest comparison rather than N key +> derivations. + +Names are unique per workspace so that `visionset token revoke ci` resolves to one credential. +Uniqueness is enforced twice, the way project names are: `uq_token_workspace_name` (`COLLATE +NOCASE`) is the guarantee, and `TokenService`'s pre-check is the error message. + +## Issuing and revoking + +`TokenService` is the one door. `AuthProvider` — the port all three surfaces authenticate through +— stays a single method, `verify(token) -> bool`; minting and revoking are use cases, and widening +the port would oblige every future provider to implement issuance it has no business doing. + +**Revocation is immediate and one-way.** `revoke` takes `confirm=True`, because it breaks every +client holding that secret at the next request and there is no `unrevoke`: reinstating a secret +somebody decided to burn is worse than issuing a fresh one, since the reason for burning it does +not expire. Revoking twice is a no-op that keeps the first timestamp, so a retried command is +safe. The row stays — it is the record that the credential existed and when it died — which is +also why revocation does not free the name. + +Nothing caches a verdict. "Revoked, therefore refused" has to mean *now*, so the provider reads +the workspace on every call. That read is cheap: WAL readers never block a writer, and a +read-only unit of work takes no lock at all. + +## What a refusal looks like + +A missing header, a non-bearer scheme, an empty credential, an unknown token and a revoked token +are **one answer**, byte for byte: + +``` +HTTP/1.1 401 Unauthorized +WWW-Authenticate: Bearer + +{"code": "UNAUTHORIZED", "message": "Invalid or missing bearer token", "detail": null} +``` + +That uniformity is the point. A 401 that distinguished "no such token" from "revoked" would let +anyone enumerate a workspace's credentials one request at a time. + +A failure to *decide* is not a refusal. If the store is unreachable or damaged, `verify` raises +rather than answering `False`, and the client sees a 503 (`WORKSPACE_BUSY`) or a 500 +(`WORKSPACE_CORRUPT`). Reporting an outage as a bad credential sends an operator hunting for the +wrong thing. + +## Which workspace the server serves + +One, named by **`VISIONSET_WORKSPACE`**, defaulting to the process's working directory. It is +opened by the first request that needs it and kept for the life of the process — never at import +time, because `scripts/export_openapi.py` imports the application in a checkout that has no +workspace. + +A server pointed at something that is not a workspace answers **500 `NOT_A_WORKSPACE`**, opaque +body plus an `incident_id`, with the path in the log only. That is a deployment fault, not a +client error. It arrives *instead of* a 401 even when no token was sent, because the workspace is +resolved before the credential is looked at — the ordering is what keeps authentication +overridable in tests. + +> The full resolution rule — a `--workspace` flag, the environment variable, cwd detection, and a +> documented precedence shared with the CLI — belongs to issue #26. +> `server/dependencies.py::resolve_workspace_root` is the provisional half, reading the variable +> #26 will keep. + +## For contributors + +Build every non-public router with **`protected_router()`**: + +```python +from visionset.server.dependencies import WorkspaceDep, protected_router +from visionset.server.errors import ERROR_RESPONSES + +router = protected_router(prefix="/projects", tags=["projects"]) + + +@router.get("/{project_id}", responses={404: ERROR_RESPONSES[404]}) +def get_project(project_id: UUID, workspace: WorkspaceDep) -> ProjectOut: ... +``` + +It carries the dependency *and* the documented 401 together, because a route that declares one +without the other is a lie in `openapi.json` either way. Do not repeat `Depends(require_token)` +per route — "everything except `/health`" should be a property of how routers are constructed, +not something each reviewer has to notice — and do not add 401 to `UNIVERSAL_ERROR_RESPONSES`, +because `/health` is public and cannot 401. +`tests/server/test_openapi_contract.py` walks the spec and fails on either mistake. + +`/docs`, `/redoc` and `/openapi.json` stay public. They are `include_in_schema=False`, and the +spec is already a committed artifact in a public repository: a contract you must authenticate to +read is a contract nobody generates a client from. + +**There is no MCP tool for token administration, and that is deliberate.** Every other tool +operates on *datasets*; one that minted a credential would operate on *access to the workspace* — +a privilege-escalation primitive pointed at the agent's own sandbox, producing a durable secret +that outlives the session. The secret is shown exactly once, and an agent's "once" is a +transcript: `confirm: true` guards accidental mutation, not exfiltration. Whoever launched +`visionset mcp` already had workspace access, so a second credential adds capability and subtracts +accountability. `list_tokens` is the only defensible candidate and is still operator surface +rather than dataset surface. diff --git a/docs/persistence.md b/docs/persistence.md index 22dc946e..3a7274a1 100644 --- a/docs/persistence.md +++ b/docs/persistence.md @@ -20,7 +20,7 @@ mapping layer is the thing to extend. Every persisted entity has a UUID primary key and **at most one parent** — a Project belongs to a Workspace, an Annotation to an Asset. That regularity is why a single -generic repository serves all fourteen entity types: +generic repository serves all fifteen entity types: ```python with store.unit_of_work() as uow: @@ -48,7 +48,8 @@ never raises `ProjectNameTaken`. But a rule with no backstop is a wish, so the store carries the constraint too: `uq_project_workspace_name` on `project (workspace_id, name COLLATE NOCASE)`, alongside `uq_schema_project_version`, `uq_member_dataset_asset`, `uq_release_dataset_tag`, -`uq_asset_project_content_hash` and `uq_source_project_kind_path_fps`. The invariant then survives +`uq_asset_project_content_hash`, `uq_source_project_kind_path_fps` and +`uq_token_workspace_name`. The invariant then survives a service bug, a forgotten code path, and a second process. The last of those is the only index here whose terms are not all columns: its fourth is @@ -159,8 +160,9 @@ MIGRATIONS: list[Migration] = [ Migration(version=8, name="ingest_pipeline", upgrade=...), Migration(version=9, name="ingest_job_progress", upgrade=...), Migration(version=10, name="asset_thumbnail", upgrade=...), + Migration(version=11, name="api_tokens", upgrade=...), ] -FORMAT_VERSION: int = MIGRATIONS[-1].version # 10 +FORMAT_VERSION: int = MIGRATIONS[-1].version # 11 ``` `initialize()` reads the version stamped in `_visionset_meta` and runs whatever is @@ -270,6 +272,14 @@ preview in the blob store. No foreign key, so 008's "a column carrying a key can NULL is not a legacy value something has to tolerate but the ordinary state of an asset nobody has rendered a preview for yet. `IngestService.backfill_thumbnails` reads exactly that state. +Migration 011 is the first since 001 to create a **table** rather than alter one, and that +changes which tool does the idempotency: `checkfirst=True` on the `Table` asks `has_table`, a +plain catalogue lookup, and brings both of `token`'s indexes with it — so neither is issued +separately and 008's expression-index trap cannot be met here at all. Nothing to refuse and +nothing to count, which is a claim rather than an oversight: the table existed on no earlier +generation, so there is no legacy row to be honest about, and nothing references it, so +`PRAGMA foreign_keys = ON` has nothing to cascade. + The fresh-versus-migrated test is only as strong as how far back `_downgrade_to_version_one` walks, so every migration added there needs its undo added too. Migrations 006 and 007 are the two places that undo cannot borrow its DDL from `_tables`, @@ -281,6 +291,12 @@ altered, for the reasons 008 gives, so nothing later rebuilds it. The compensati real `ALTER` runs on the way back up from generation 1, which is why it needs no generation twin of `test_migration_nine_alters_a_table_migration_eight_rebuilt`. +Migration 011's undo is a single `DROP TABLE`, and it carries a sharper obligation than 010's: +**without it the fresh-versus-migrated test would still pass.** The table would simply survive the +downgrade and 011 would `checkfirst`-skip, so the `CREATE` nobody ran would be reported as +agreeing with itself. The undo is not what keeps an existing test honest — it is the only thing +that exercises the migration at all. + `format_version` here is the *database* generation. Validating the on-disk workspace layout around it — directories, the blob-store root, what makes a directory a workspace at all — belongs to `WorkspaceService`; see [workspaces.md](workspaces.md). diff --git a/docs/workspaces.md b/docs/workspaces.md index bad97915..c5400998 100644 --- a/docs/workspaces.md +++ b/docs/workspaces.md @@ -21,19 +21,27 @@ or one whose process was killed, is all four entries. Everything written since the last checkpoint lives in `visionset.db-wal` until then. Close the workspace first, or copy all three files together. -Three of the five ports have no line in that layout, and that is the point: the [event -bus](events.md) is in-process and the two [media processors](media.md) are decoders, so none of -them leaves anything behind. They are composed here anyway, because a workspace is what services +Four of the six ports have no line in that layout, and that is the point: the [event +bus](events.md) is in-process, the two [media processors](media.md) are decoders, and the +[auth provider](auth.md) reads a table inside the database above, so none of them leaves anything +behind. They are composed here anyway, because a workspace is what services are handed and every port has to arrive with it. One of each per open workspace, built by -`event_bus_factory`, `image_processor_factory` and `video_processor_factory` — never a -module-level singleton, which two workspaces open at once must not share. +`event_bus_factory`, `image_processor_factory`, `video_processor_factory` and +`auth_provider_factory` — never a module-level singleton, which two workspaces open at once must +not share. + +`auth_provider_factory` is the one that takes arguments: `(metadata_store, workspace_id)`, because +it is the first port derived from another rather than from the path. No kernel service uses it — +it exists for the surfaces above — and it is composed here anyway, because the alternative is a +delivery module naming a kernel adapter. A new port is appended **last** to `WorkspaceService.__init__`, never inserted: `init` and `open` bind those arguments positionally, so a parameter added in the middle silently re-binds every one after it. `WorkspaceService` is the only place in the kernel that names `SqliteMetadataStore`, -`FilesystemBlobStore`, `InProcessEventBus`, `PillowImageProcessor` or `FfmpegVideoProcessor`. +`FilesystemBlobStore`, `InProcessEventBus`, `PillowImageProcessor`, `FfmpegVideoProcessor` or +`StoredTokenAuthProvider`. Everything above it — later surface, the CLI, MCP — gets an open service and reaches the ports through it, so swapping an adapter is a change to two functions and to nowhere else. @@ -254,10 +262,11 @@ Four habits that keep the boundary honest: workspace-level rules with it. - One `unit_of_work()` per operation, and do the whole operation inside it. - Reach the ports through the handle — `workspace.metadata_store`, `workspace.blob_store`, - `workspace.event_bus`, `workspace.image_processor`, `workspace.video_processor`. No service - other than `workspace_service` should name `SqliteMetadataStore`, `FilesystemBlobStore`, - `InProcessEventBus`, `PillowImageProcessor` or `FfmpegVideoProcessor` — if a second one does, - the composition point has stopped being single. + `workspace.event_bus`, `workspace.image_processor`, `workspace.video_processor`, + `workspace.auth_provider`. No service other than `workspace_service` should name + `SqliteMetadataStore`, `FilesystemBlobStore`, `InProcessEventBus`, `PillowImageProcessor`, + `FfmpegVideoProcessor` or `StoredTokenAuthProvider` — if a second one does, the composition + point has stopped being single. - Publish [events](events.md) *after* the `unit_of_work()` block, never inside it. An announcement is about work that committed, and a subscriber that raises must have nothing left to roll back. diff --git a/src/visionset/cli/main.py b/src/visionset/cli/main.py index e0fabaff..94b0d43c 100644 --- a/src/visionset/cli/main.py +++ b/src/visionset/cli/main.py @@ -2,7 +2,6 @@ from __future__ import annotations -import secrets from typing import Annotated import typer @@ -55,7 +54,17 @@ def mcp() -> None: def token_create( name: Annotated[str, typer.Option("--name", help="Human-readable token name.")], ) -> None: - """Generate an API token (no persistence yet).""" - token = f"vst_{secrets.token_urlsafe(32)}" - typer.echo(f"Created token '{name}' (not persisted yet):") - typer.echo(token) + """Issue an API token (stub — see issue #26). + + Persistence landed with the kernel's ``TokenService``; wiring this command to + it needs workspace resolution, which issue #26 owns along with ``token list`` + and ``token revoke``. + + Until then this refuses rather than printing something. It used to echo a + plausible ``vst_...`` string that was never stored — harmless while nothing + could authenticate, and actively misleading now that real tokens exist and + that one would not be among them. + """ + typer.echo(f"Cannot issue token {name!r} yet: the CLI has no workspace to write it to.") + typer.echo("Token issuance lands in issue #26 (visionset token create/list/revoke).") + raise typer.Exit(code=1) diff --git a/src/visionset/kernel/__init__.py b/src/visionset/kernel/__init__.py index e23e6e17..10991171 100644 --- a/src/visionset/kernel/__init__.py +++ b/src/visionset/kernel/__init__.py @@ -47,6 +47,8 @@ SchemaNotFound, SchemaVersionConflict, SourceNotFound, + TokenNameTaken, + TokenNotFound, UnknownAttribute, UnserializableManifest, UnsupportedGeometry, @@ -100,6 +102,8 @@ "SchemaNotFound", "SchemaVersionConflict", "SourceNotFound", + "TokenNameTaken", + "TokenNotFound", "UnknownAttribute", "UnserializableManifest", "UnsupportedGeometry", diff --git a/src/visionset/kernel/adapters/__init__.py b/src/visionset/kernel/adapters/__init__.py index ac55300f..24e8088a 100644 --- a/src/visionset/kernel/adapters/__init__.py +++ b/src/visionset/kernel/adapters/__init__.py @@ -5,6 +5,7 @@ from visionset.kernel.adapters.in_process_event_bus import InProcessEventBus from visionset.kernel.adapters.pillow_image_processor import PillowImageProcessor from visionset.kernel.adapters.sqlite_metadata_store import SqliteMetadataStore +from visionset.kernel.adapters.stored_token_auth_provider import StoredTokenAuthProvider __all__ = [ "FfmpegVideoProcessor", @@ -12,4 +13,5 @@ "InProcessEventBus", "PillowImageProcessor", "SqliteMetadataStore", + "StoredTokenAuthProvider", ] diff --git a/src/visionset/kernel/adapters/_mappers.py b/src/visionset/kernel/adapters/_mappers.py index 20b70bce..b7836701 100644 --- a/src/visionset/kernel/adapters/_mappers.py +++ b/src/visionset/kernel/adapters/_mappers.py @@ -4,18 +4,19 @@ gets one ``EntityMapping`` describing its table, its parent column, and the two directions of the conversion; the repository in ``sqlite_metadata_store`` is written once against that description rather than -fourteen times against fourteen tables. +fifteen times against fifteen tables. Most entities are flat — every field is a column — and share -``_flat_mapping``. The eight that are not say so explicitly: +``_flat_mapping``. The nine that are not say so explicitly: - ``AnnotationSchema``, ``Annotation`` and ``IngestJob`` hold immutable nested values, encoded as JSON. - ``Batch`` and ``AnnotationJob`` own child tables, so their mappings carry a ``sync_children`` hook and rebuild their collections on read. -- ``DatasetChange``, ``Release`` and ``Source`` encode a timezone-aware - timestamp, which a ``String`` column must be handed as text rather than as a - ``datetime``. ``Source`` also carries a nested ``VideoProvenance`` as JSON. +- ``DatasetChange``, ``Release``, ``Source`` and ``Token`` encode a + timezone-aware timestamp, which a ``String`` column must be handed as text + rather than as a ``datetime``. ``Source`` also carries a nested + ``VideoProvenance`` as JSON. """ from __future__ import annotations @@ -52,6 +53,7 @@ SourceKind, SplitRecipe, TaskGroup, + Token, VideoProvenance, Workspace, ) @@ -236,6 +238,30 @@ def _source_to_domain(_: Session, row: Any) -> Source: ) +def _token_to_row(entity: Token) -> t.Base: + return t.TokenRow( + id=entity.id, + workspace_id=entity.workspace_id, + name=entity.name, + secret_hash=entity.secret_hash, + # Spelled out for ``_source_to_row``'s reason: ``_flat_mapping`` dumps in + # python mode and would hand a ``datetime`` to a ``String`` column. + created_at=entity.created_at.isoformat(), + revoked_at=None if entity.revoked_at is None else entity.revoked_at.isoformat(), + ) + + +def _token_to_domain(_: Session, row: Any) -> Token: + return Token( + id=row.id, + workspace_id=row.workspace_id, + name=row.name, + secret_hash=row.secret_hash, + created_at=datetime.fromisoformat(row.created_at), + revoked_at=None if row.revoked_at is None else datetime.fromisoformat(row.revoked_at), + ) + + def _release_to_row(entity: Release) -> t.Base: return t.ReleaseRow( id=entity.id, @@ -371,6 +397,12 @@ def _job_sync_children(session: Session, entity: AnnotationJob) -> None: to_row=_source_to_row, to_domain=_source_to_domain, ) +TOKENS: EntityMapping[Token] = EntityMapping( + row=t.TokenRow, + parent_column="workspace_id", + to_row=_token_to_row, + to_domain=_token_to_domain, +) RELEASES: EntityMapping[Release] = EntityMapping( row=t.ReleaseRow, parent_column="dataset_id", diff --git a/src/visionset/kernel/adapters/_tables.py b/src/visionset/kernel/adapters/_tables.py index f379e503..3b5edc0a 100644 --- a/src/visionset/kernel/adapters/_tables.py +++ b/src/visionset/kernel/adapters/_tables.py @@ -465,3 +465,55 @@ class ReleaseRow(Base): split: Mapped[dict[str, Any] | None] = mapped_column(JSON, nullable=True) created_at: Mapped[str] = mapped_column(String, nullable=False) visionset_version: Mapped[str] = mapped_column(String, nullable=False) + + +class TokenRow(Base): + """API credentials, hashed. Created whole by migration 11, so order is free. + + Every other table with a migrated column declares it last, because SQLite's + ``ALTER TABLE ... ADD COLUMN`` appends and the two creation paths would + otherwise emit different ``CREATE TABLE`` text. This table is exempt for the + reason ``SourceRow`` is: migration 11 builds it with ``Table.create``, from + this same class, so both paths compile identical DDL whatever the order here. + + **That exemption expires for any column added after migration 11.** A + database already stamped at 11 has the table, so a later column reaches it by + ``ALTER`` and must be declared last — the rule ``AssetRow`` and + ``IngestJobRow`` already live under. + + ``secret_hash`` holds a SHA-256 digest and never a plaintext; ``domain.token`` + argues why a digest rather than a KDF is the right call here. It carries no + index: verification hashes the presentation and scans the workspace's tokens, + which is a handful of rows, and an index on a credential-derived value buys a + lookup nothing measures. + """ + + __tablename__ = "token" + + id: Mapped[UUID] = mapped_column(SaUuid, primary_key=True) + workspace_id: Mapped[UUID] = mapped_column( + SaUuid, ForeignKey("workspace.id", ondelete="CASCADE"), index=True, nullable=False + ) + name: Mapped[str] = mapped_column(String, nullable=False) + secret_hash: Mapped[str] = mapped_column(String, nullable=False) + #: ISO-8601 with offset, never SQLite ``DATETIME``. See the module docstring. + created_at: Mapped[str] = mapped_column(String, nullable=False) + #: When the credential was burned, or NULL while it still works. + revoked_at: Mapped[str | None] = mapped_column(String, nullable=True) + + +#: Token names are unique per workspace, case-insensitively. +#: +#: The ``PROJECT_NAME_UNIQUE`` reasoning one entity over, and the same division +#: of labour: ``COLLATE NOCASE`` folds ASCII at the storage layer while +#: ``TokenService`` handles Unicode folding and whitespace where the normalized +#: string is in hand. The service reports the error; this index is the guarantee. +#: +#: It is load-bearing rather than tidy: ``visionset token revoke `` can only +#: mean something if a name resolves to exactly one credential. +TOKEN_NAME_UNIQUE = Index( + "uq_token_workspace_name", + TokenRow.workspace_id, + TokenRow.name.collate("NOCASE"), + unique=True, +) diff --git a/src/visionset/kernel/adapters/migrations.py b/src/visionset/kernel/adapters/migrations.py index 262e8cd8..7fb5d025 100644 --- a/src/visionset/kernel/adapters/migrations.py +++ b/src/visionset/kernel/adapters/migrations.py @@ -58,6 +58,7 @@ IngestJobRow, ReleaseRow, SourceRow, + TokenRow, ) from visionset.kernel.errors import WorkspaceCorrupt @@ -411,6 +412,37 @@ def _add_asset_thumbnail(connection: Connection) -> None: _add_column(connection, cast(Column[object], AssetRow.__table__.c.thumbnail_hash)) +def _add_api_tokens(connection: Connection) -> None: + """Give a workspace somewhere to keep the credentials that reach it. + + The first migration since 1 that creates a **table** rather than altering + one, and that changes which tool does the idempotency. ``checkfirst`` on a + ``Table`` asks ``has_table`` — a plain catalogue lookup — where migration 8 + had to reach for ``CREATE INDEX IF NOT EXISTS`` because SQLAlchemy cannot + *reflect* an expression-based index and re-issued a ``CREATE`` that failed on + every fresh database. Nothing here can meet that trap: the two indexes on + this table come along inside ``Table.create`` and are never issued + separately. + + Nothing to refuse and nothing to count, and that is a claim rather than an + oversight. Migrations 6, 7 and 8 each had to prove a table was empty before + dropping or rebuilding it; this table existed on no earlier generation, so + there is no legacy row to be honest or dishonest about, and no child table + references it, so ``PRAGMA foreign_keys = ON`` has nothing to cascade. The + one thing ``checkfirst`` does *not* check is the shape of a table that is + already there — and on the only path that can reach this migration with one, + it was written by migration 1 from this same class. + + Unlike migration 9 and like migration 10, this one **needs its own undo** in + the tests' ``_downgrade_to_version_one``, and the reason is sharper here: + without a ``drop table token`` line the fresh-versus-migrated test would + still *pass*, because the table would survive the downgrade and this + migration would ``checkfirst``-skip. The undo is what gives it a real + exercise, not what keeps a test honest that was already watching. + """ + cast(Table, TokenRow.__table__).create(connection, checkfirst=True) + + MIGRATIONS: list[Migration] = [ Migration(version=1, name="initial_schema", upgrade=_create_initial_schema), Migration( @@ -458,6 +490,11 @@ def _add_asset_thumbnail(connection: Connection) -> None: name="asset_thumbnail", upgrade=_add_asset_thumbnail, ), + Migration( + version=11, + name="api_tokens", + upgrade=_add_api_tokens, + ), ] FORMAT_VERSION: int = MIGRATIONS[-1].version diff --git a/src/visionset/kernel/adapters/sqlite_metadata_store.py b/src/visionset/kernel/adapters/sqlite_metadata_store.py index a8cb793f..e7fe5af3 100644 --- a/src/visionset/kernel/adapters/sqlite_metadata_store.py +++ b/src/visionset/kernel/adapters/sqlite_metadata_store.py @@ -240,6 +240,7 @@ def __init__(self, session: Session) -> None: self.dataset_members = SqlRepository(session, m.DATASET_MEMBERS) self.dataset_changes = SqlRepository(session, m.DATASET_CHANGES) self.releases = SqlRepository(session, m.RELEASES) + self.tokens = SqlRepository(session, m.TOKENS) class SqliteMetadataStore: diff --git a/src/visionset/kernel/adapters/stored_token_auth_provider.py b/src/visionset/kernel/adapters/stored_token_auth_provider.py new file mode 100644 index 00000000..59cbdef8 --- /dev/null +++ b/src/visionset/kernel/adapters/stored_token_auth_provider.py @@ -0,0 +1,69 @@ +# usage: reached as ``workspace.auth_provider``; named only by WorkspaceService +"""The default ``AuthProvider``: tokens persisted in the workspace itself. + +Named for *where the credentials live*, the way ``EnvTokenAuthProvider`` was +named for the environment variable it read. Not ``SqliteAuthProvider``, which +would be a lie: this holds the ``MetadataStore`` **port** and could not reach +SQL if it wanted to. Which store is behind it is ``WorkspaceService``'s business. + +It takes the store and a workspace id rather than a ``WorkspaceService`` or a +``TokenService``: ``WorkspaceService`` imports this module, so importing back +would be a runtime cycle. +""" + +from __future__ import annotations + +import secrets +from uuid import UUID + +from visionset.kernel.domain import hash_secret +from visionset.kernel.ports import MetadataStore + + +class StoredTokenAuthProvider: + """Verify a bearer token against the workspace's own ``token`` table. + + **Nothing is cached, and that is the design.** ``TokenService.revoke`` + promises a credential stops working immediately; a TTL would downgrade that + to eventually, which is the one thing a revocation may not do. The cost is a + read transaction per call, and it is small: WAL readers never block a writer, + and pysqlite defers ``BEGIN`` to the first *write*, so a read-only unit of + work takes no lock at all. + + **The lookup is a scan, on purpose.** ``Repository`` has no query-by-column + and none is added for this — a workspace holds a handful of tokens, and each + costs one digest comparison. If it ever bites, the sanctioned fix is a method + on the port, never SQL here, which this class cannot write anyway. + """ + + def __init__(self, metadata_store: MetadataStore, workspace_id: UUID) -> None: + self._metadata_store = metadata_store + self._workspace_id = workspace_id + + def verify(self, token: str) -> bool: + """Whether this string is a live credential of this workspace. + + Unknown, malformed and revoked are one answer, so that the result cannot + be used to probe which secrets exist. A store that cannot be read raises + ``WorkspaceBusy`` or ``WorkspaceCorrupt`` rather than answering ``False``: + an outage is not a bad password. + """ + if not token: + return False + presented = hash_secret(token) + with self._metadata_store.unit_of_work() as uow: + # ``list(self._workspace_id)``, never ``list()``. A ``parent_id`` of + # ``None`` is not an error on a scoped entity — it means every row in + # the table — so the bare call would be accidentally correct today + # and a cross-workspace credential leak the day one store holds two. + stored = uow.tokens.list(self._workspace_id) + # ``compare_digest`` over ``==``, and the honest claim is narrow: both + # operands are SHA-256 digests rather than the secret, and the short + # circuit already leaks position, so this closes no channel of value. It + # stays because it costs nothing and because comparing a + # credential-derived value with ``==`` is not the habit to learn here. + return any( + secrets.compare_digest(candidate.secret_hash, presented) + for candidate in stored + if not candidate.revoked + ) diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index a4addbd3..faf41a1a 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -99,6 +99,14 @@ TaskGroup, progress_after_annotating, ) +from visionset.kernel.domain.token import ( + SECRET_BYTES, + SECRET_PREFIX, + IssuedToken, + Token, + generate_secret, + hash_secret, +) from visionset.kernel.domain.transitions import require_move from visionset.kernel.domain.workspace import Workspace @@ -110,6 +118,8 @@ "JOB_TRANSITIONS", "MANIFEST_VERSION", "PROMOTABLE_PROGRESS", + "SECRET_BYTES", + "SECRET_PREFIX", "SETTLED_PROGRESS", "Annotation", "AnnotationJob", @@ -145,6 +155,7 @@ "IngestJob", "IngestResult", "IngestState", + "IssuedToken", "LabelClass", "Manifest", "ManifestAnnotation", @@ -165,6 +176,7 @@ "SplitRecipe", "TaskGroup", "ThumbnailBackfill", + "Token", "VideoFrame", "VideoMetadata", "VideoProvenance", @@ -173,6 +185,8 @@ "canonical_bytes", "canonical_path", "diff_classes", + "generate_secret", + "hash_secret", "normalize_name", "partition_assets", "progress_after_annotating", diff --git a/src/visionset/kernel/domain/token.py b/src/visionset/kernel/domain/token.py new file mode 100644 index 00000000..8d0fce98 --- /dev/null +++ b/src/visionset/kernel/domain/token.py @@ -0,0 +1,157 @@ +# usage: from visionset.kernel.domain import Token, generate_secret, hash_secret +"""API tokens: the credential a surface presents, and the record we keep of it. + +A :class:`Token` is what the workspace stores; the secret it stands for is shown +to an operator exactly once, at creation, and is never recoverable afterwards. +The two travel together only in :class:`IssuedToken`, which is a return value and +is never persisted. + +**Only the hash is stored, and the hash is SHA-256 rather than a password KDF.** +That looks like the wrong call until the input is named. A KDF's entire job is to +make *low-entropy, human-chosen* input expensive to guess; the input here is 256 +bits from :func:`secrets.token_urlsafe`, where there is no dictionary and no +guessing budget that terminates. Two further reasons make it the right call +rather than merely a defensible one: verification runs on **every request** and +compares the presentation against every token the workspace holds, so an argon2 +at 100 ms would cost N x 100 ms *per request* — the opposite cost model to a login +form, where the check runs once and rate limiting bounds it. And ``hashlib`` is +stdlib, where argon2-cffi or bcrypt is a dependency taken on for no gain. + +The accepted consequence, stated rather than hidden: the digest is unsalted and +deterministic, so two identical secrets hash identically. That requires drawing +the same 256-bit value twice, and it is precisely the property that lets +verification be a cheap digest comparison instead of N key derivations. + +**The mint and the check must agree on one spelling of the hash.** +:func:`hash_secret` lives here, in the domain, for the same reason +``canonical_path`` does: ``TokenService`` mints with it and the stored-token +``AuthProvider`` verifies with it, and two spellings would produce a credential +that can never authenticate — a bug that looks like a bad password. +""" + +from __future__ import annotations + +import hashlib +import re +import secrets +from datetime import UTC, datetime +from typing import Final +from uuid import UUID, uuid4 + +from pydantic import BaseModel, ConfigDict, Field, field_validator + +SECRET_PREFIX: Final = "vst_" +"""What every issued secret starts with. + +A visible marker, not a security measure: it lets an operator recognise a +VisionSet token in a config file, and lets a secret scanner match one shape +instead of guessing at every high-entropy string. +""" + +SECRET_BYTES: Final = 32 +"""How much randomness a secret carries — 256 bits, the reason sha256 suffices.""" + +_SHA256_HEX = re.compile(r"^[0-9a-f]{64}$") + + +def generate_secret() -> str: + """A fresh token secret, prefixed and unguessable. + + ``secrets.token_urlsafe`` rather than ``uuid4``: a UUID is 122 bits with a + documented layout, and it is a name for a thing rather than a thing kept + hidden. Nothing in the stored form depends on this spelling — a token + issued by an older build keeps working, because only the digest is kept. + """ + return f"{SECRET_PREFIX}{secrets.token_urlsafe(SECRET_BYTES)}" + + +def hash_secret(secret: str) -> str: + """The stored form of a secret: lowercase hex SHA-256 of its UTF-8 bytes. + + Deterministic and unsalted, for the reason the module docstring gives. The + whole presented string is hashed, prefix included — the prefix is part of + what the operator was handed, so trimming it here would mean accepting a + secret the operator never saw. + """ + return hashlib.sha256(secret.encode("utf-8")).hexdigest() + + +class Token(BaseModel): + """A named API credential belonging to one workspace. + + Not frozen, unlike ``Release``: revocation edits this row, and the store + writes an edit as a whole-row replace fed by ``model_copy(update=...)``. + Immutability is a claim ``Release`` earns by being an artifact; a credential + has a lifecycle. + + **Revocation is a timestamp, and ``revoked`` is derived from it.** The same + doctrine that keeps a schema's "active" version a computed maximum rather + than a stored column: a bool answers "is it dead?" and nothing else, so + "when did we burn it?" would need another migration to ask. ``NULL`` is not a + legacy value something has to tolerate here — it is the ordinary state of a + token nobody has revoked, exactly as ``Asset.thumbnail_hash``'s NULL is the + ordinary state of an asset nobody has rendered. + + ``revoked_at`` is written once and never rewritten, the rule + ``Source.registered_at`` follows, which is what makes a repeated revoke a + no-op rather than a rewrite of when the credential actually died. + + The plaintext is **not** here and cannot be recovered from what is. That is + the point of the entity: a workspace that leaks its metadata store leaks the + names and lifetimes of its credentials, not the credentials. + """ + + id: UUID = Field(default_factory=uuid4) + workspace_id: UUID + name: str + #: SHA-256 of the issued secret. See :func:`hash_secret`. + secret_hash: str + created_at: datetime = Field(default_factory=lambda: datetime.now(UTC)) + #: When this credential was burned, or ``None`` while it still works. + revoked_at: datetime | None = None + + @property + def revoked(self) -> bool: + """Whether this token has been revoked. Derived, never stored.""" + return self.revoked_at is not None + + @field_validator("secret_hash") + @classmethod + def _is_sha256_hex(cls, value: str) -> str: + """Refuse anything that is not a digest, so a plaintext cannot land here. + + The failure this catches is not a typo: it is a caller that assigned the + secret to the wrong field, which would store the credential in clear and + still verify correctly against nothing. + """ + if not _SHA256_HEX.fullmatch(value): + raise ValueError("secret_hash must be 64 lowercase hex chars (SHA-256)") + return value + + @field_validator("created_at", "revoked_at") + @classmethod + def _is_timezone_aware(cls, value: datetime | None) -> datetime | None: + if value is not None and value.tzinfo is None: + raise ValueError("token timestamps must be timezone-aware (UTC)") + return None if value is None else value.astimezone(UTC) + + +class IssuedToken(BaseModel): + """What ``TokenService.create`` hands back: the record, and the one showing. + + Frozen and never persisted — there is no mapper for it and no table behind + it. It exists so that "the secret is shown exactly once" is a shape in the + type system rather than a sentence in a docstring: the only object that ever + holds a plaintext is the return value of the one method that mints one. + + ``secret`` carries ``repr=False``, which is load-bearing rather than tidy. A + traceback rendering local variables, a stray ``print``, a log line + interpolating the model — each of those is a credential in a file somebody + forgot about. ``issued.secret`` still reads it; nothing prints it by + accident. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + token: Token + secret: str = Field(repr=False) diff --git a/src/visionset/kernel/errors.py b/src/visionset/kernel/errors.py index 297eeb15..164e1c7f 100644 --- a/src/visionset/kernel/errors.py +++ b/src/visionset/kernel/errors.py @@ -6,8 +6,8 @@ single ``except`` clause. The kernel NEVER raises a framework exception — ``HTTPException`` and friends belong to the boundary, not here. -Persistence, workspace, project, schema, batch, job, annotation, dataset, release -and media errors live here; later services add their own as they land. +Persistence, workspace, project, schema, batch, job, annotation, dataset, release, +media and token errors live here; later services add their own as they land. """ from __future__ import annotations @@ -583,3 +583,33 @@ class MediaToolUnavailable(VisionSetError): remedy here is a package manager, so an error that merely says "unavailable" has told the operator nothing they did not already suspect. """ + + +class TokenNotFound(VisionSetError): + """No API token with that id or name lives in this workspace. + + The ``ProjectNotFound`` rule at workspace scope: a token belonging to a + different workspace reads as *missing*, never as forbidden. + + Note what this is **not**. Presenting a token that does not verify — unknown, + malformed or revoked — raises nothing at all: ``AuthProvider.verify`` answers + ``False`` and the surface decides what that means. This error is for + *administering* a token an operator named, not for failing to authenticate + with one. Conflating the two would let a 404 on this error become an oracle + for which secrets exist. + """ + + +class TokenNameTaken(VisionSetError): + """Another token in this workspace already uses that name. + + The ``ProjectNameTaken`` rule, one entity over, and enforced twice for the + same reason: the service checks before writing so the caller gets a sentence, + and a unique index refuses the write so a race cannot slip past the check. + + Case-insensitive, like a project name and unlike a release tag. A token name + is a label an operator reads back in a list and types into ``token revoke``, + so ``ci`` and ``CI`` naming two credentials is a trap rather than a feature — + and the name has to resolve to exactly one token for revocation by name to + mean anything. + """ diff --git a/src/visionset/kernel/ports/auth_provider.py b/src/visionset/kernel/ports/auth_provider.py index a6cec5ca..c380d8f7 100644 --- a/src/visionset/kernel/ports/auth_provider.py +++ b/src/visionset/kernel/ports/auth_provider.py @@ -1,8 +1,31 @@ +# usage: from visionset.kernel.ports import AuthProvider +"""Token verification: the seam every delivery surface authenticates through.""" + from typing import Protocol, runtime_checkable @runtime_checkable class AuthProvider(Protocol): - """Token verification. Delivery layers (server/CLI/MCP) depend on this port.""" + """Token verification. Delivery layers (server/CLI/MCP) depend on this port. + + One method, deliberately. Minting and revoking credentials are *use cases* + and live in ``TokenService``; a port that grew a ``create`` would oblige + every future provider — one backed by OIDC, say — to implement issuance it + has no business doing. What a surface needs is the yes-or-no, and that is + all this promises. + + Three obligations an implementation owes, none of which the signature can + express: + + - **``False`` means "this credential does not authenticate", and nothing + more.** Unknown, malformed, expired and revoked are one answer, so that a + caller cannot turn the distinction into an oracle for which secrets exist. + - **A failure to *decide* raises; it never answers ``False``.** An + unreachable or damaged store is an outage, and reporting an outage as a bad + credential sends the operator hunting for the wrong thing. + - **A revocation takes effect immediately.** Caching a positive verdict turns + "revoked, therefore refused" into "refused eventually", which is the one + promise a revocation has to keep. + """ def verify(self, token: str) -> bool: ... diff --git a/src/visionset/kernel/ports/metadata_store.py b/src/visionset/kernel/ports/metadata_store.py index 437cecb9..e08199b2 100644 --- a/src/visionset/kernel/ports/metadata_store.py +++ b/src/visionset/kernel/ports/metadata_store.py @@ -25,6 +25,7 @@ Release, Source, TaskGroup, + Token, Workspace, ) @@ -132,6 +133,17 @@ def dataset_changes(self) -> Repository[DatasetChange]: ... @property def releases(self) -> Repository[Release]: ... + @property + def tokens(self) -> Repository[Token]: + """API credentials, parented on the workspace rather than on a project. + + The only repository here whose parent is the workspace itself, which is + why ``list(workspace_id)`` and not ``list()`` is the correct read: a + ``parent_id`` of ``None`` is not an error on a scoped entity, it means + *every row in the table*. + """ + ... + @runtime_checkable class MetadataStore(Protocol): diff --git a/src/visionset/kernel/services/__init__.py b/src/visionset/kernel/services/__init__.py index 7eaa6c20..48f37d38 100644 --- a/src/visionset/kernel/services/__init__.py +++ b/src/visionset/kernel/services/__init__.py @@ -16,6 +16,7 @@ from visionset.kernel.services.release_service import ReleaseService from visionset.kernel.services.schema_service import SchemaService from visionset.kernel.services.source_service import SourceService +from visionset.kernel.services.token_service import TokenService from visionset.kernel.services.workspace_service import ( BLOBS_DIRNAME, DB_FILENAME, @@ -34,5 +35,6 @@ "ReleaseService", "SchemaService", "SourceService", + "TokenService", "WorkspaceService", ] diff --git a/src/visionset/kernel/services/token_service.py b/src/visionset/kernel/services/token_service.py new file mode 100644 index 00000000..5245f9dc --- /dev/null +++ b/src/visionset/kernel/services/token_service.py @@ -0,0 +1,223 @@ +# usage: from visionset.kernel.services import TokenService +"""API tokens: the one door to a credential that reaches this workspace. + +Issuance and revocation are **use cases**, so they live in a service rather than +on the ``AuthProvider`` port. The port stays one method — ``verify(token)`` — +because that is the seam all three surfaces depend on, and widening it would +oblige every future provider (an OIDC one, say) to implement minting it has no +business doing. Verification reads what this service writes; nothing else does. + +**A secret is shown exactly once.** :meth:`TokenService.create` returns an +:class:`IssuedToken` carrying the plaintext, and the workspace keeps only its +digest. There is no method that reads a secret back, and adding one would be a +different product: the remedy for a lost token is a new token. + +**Revocation is one-way, and it is guarded.** There is deliberately no +``unrevoke``: reinstating a secret an operator decided to burn is worse than +issuing a fresh one, because the reason for burning it — that somebody else has +a copy — does not expire. So :meth:`revoke` takes ``confirm=`` on the standing +rule for destructive operations, and it is destructive in the sense that +matters: every client holding that secret stops working at the next request. + +**There is no ``delete``, and no ``rename``.** A token row is the record that a +credential once existed and when it died; deleting it would erase that, and +revocation is already the terminal state. Renaming rewrites an audit label for no +invariant. The row *is* the log, which is also why this service publishes no +domain event: an auth trail that a subscriber can silently drop — the bus is +in-process, at-most-once and non-persistent — is worse than none, and +``created_at``/``revoked_at`` are durable where a published event is not. + +Composition follows the rule in ``docs/workspaces.md``: this service takes an +open :class:`WorkspaceService` and nothing else, and reaches the ports through +it. It never names an adapter. +""" + +from __future__ import annotations + +from datetime import UTC, datetime +from uuid import UUID + +from visionset.kernel.domain import ( + IssuedToken, + Token, + generate_secret, + hash_secret, + normalize_name, +) +from visionset.kernel.errors import ( + ConfirmationRequired, + ConstraintViolated, + TokenNameTaken, + TokenNotFound, +) +from visionset.kernel.ports import UnitOfWork +from visionset.kernel.services.workspace_service import WorkspaceService + +#: SQLite's own wording when ``uq_token_workspace_name`` refuses a write. The +#: adapter hands the message through verbatim, and it is the only way to tell a +#: name collision apart from any other constraint — see ``_as_name_collision``. +_NAME_INDEX_MESSAGE = "token.workspace_id, token.name" + + +class TokenService: + """Mint, read and revoke the API tokens of one workspace.""" + + def __init__(self, workspace: WorkspaceService) -> None: + self._workspace = workspace + + # --- reading ----------------------------------------------------------- + + def get(self, token_id: UUID) -> Token: + """The token with that id. Never its secret. + + Raises: + TokenNotFound: no such token in this workspace. + """ + with self._workspace.unit_of_work() as uow: + return self.require_token(uow, token_id) + + def get_by_name(self, name: str) -> Token: + """The token an operator would name, resolved case-insensitively. + + Raises: + InvalidName: the name is blank once stripped. + TokenNotFound: no token in this workspace holds that name. + """ + with self._workspace.unit_of_work() as uow: + return self.require_token_named(uow, name) + + # --- writing ----------------------------------------------------------- + + def create(self, name: str) -> IssuedToken: + """Mint a credential, and hand back its plaintext for the only time. + + The returned :class:`IssuedToken` is the one object in the system that + ever holds the secret; what is stored is a digest of it. A caller that + loses the plaintext has lost the credential, and the remedy is to create + another and revoke this one. + + Raises: + InvalidName: the name is blank once stripped. + TokenNameTaken: another token in this workspace holds that name. + """ + secret = generate_secret() + try: + with self._workspace.unit_of_work() as uow: + resolved = self._require_name_free(uow, name) + token = uow.tokens.add( + Token( + workspace_id=self._workspace.workspace_id, + name=resolved, + secret_hash=hash_secret(secret), + ) + ) + except ConstraintViolated as exc: + raise self._as_name_collision(exc, name) from exc + return IssuedToken(token=token, secret=secret) + + def revoke(self, token_id: UUID, *, confirm: bool = False) -> Token: + """Burn a credential. Every client holding its secret stops working. + + Revoking a token that is already revoked is a **no-op** that returns it + unchanged, ``revoked_at`` untouched — the idempotency ``JobService.mark`` + follows, and what makes a retried command safe. Rewriting the timestamp + would move the moment the credential actually died. + + Existence is checked before ``confirm`` is considered, so an unknown id + is a ``TokenNotFound`` with or without the flag. + + Raises: + TokenNotFound: no such token in this workspace. + ConfirmationRequired: ``confirm`` was not ``True``. + """ + with self._workspace.unit_of_work() as uow: + token = self.require_token(uow, token_id) + if token.revoked: + return token + if not confirm: + raise ConfirmationRequired( + f"revoking token {token.name!r} immediately breaks every client holding " + f"its secret, and cannot be undone; pass confirm=True to proceed" + ) + return uow.tokens.update(token.model_copy(update={"revoked_at": datetime.now(UTC)})) + + # --- lookups shared by the operations above ---------------------------- + + def require_token(self, uow: UnitOfWork, token_id: UUID) -> Token: + """The token, or refuse because this workspace does not have it. + + Public, and taking a ``uow``, for the reason ``JobService.require_job`` + is: a caller resolving a token inside its own transaction must not have + to spell the scope rule a second time. + """ + token = uow.tokens.get(token_id) + if token is None or token.workspace_id != self._workspace.workspace_id: + raise TokenNotFound( + f"no token {token_id} in workspace {self._workspace.workspace.name!r}" + ) + return token + + def require_token_named(self, uow: UnitOfWork, name: str) -> Token: + """The token holding that name, compared the way the index compares. + + Unicode case folding here, ASCII ``COLLATE NOCASE`` in the index: the + service is where the full normalized string is in hand, so it is the + stricter of the two. Uniqueness makes "the" token well defined. + """ + wanted = normalize_name(name, what="token").casefold() + for token in uow.tokens.list(self._workspace.workspace_id): + if token.name.casefold() == wanted: + return token + raise TokenNotFound( + f"no token named {name!r} in workspace {self._workspace.workspace.name!r}" + ) + + def _require_name_free(self, uow: UnitOfWork, name: str) -> str: + """The normalized name, or refuse it because this workspace already has it. + + Two layers, and neither is redundant: ``uq_token_workspace_name`` is the + guarantee and this is the error message. Private because ``create`` is + its only caller — there is no ``rename``, so it needs no ``exclude=``. + + Raises: + InvalidName: the name is blank once stripped. + TokenNameTaken: another token in this workspace holds it. + """ + normalized = normalize_name(name, what="token") + wanted = normalized.casefold() + for token in uow.tokens.list(self._workspace.workspace_id): + if token.name.casefold() == wanted: + raise TokenNameTaken( + f"a token named {token.name!r} already exists in workspace " + f"{self._workspace.workspace.name!r}" + ) + return normalized + + def _as_name_collision( + self, exc: ConstraintViolated, name: str + ) -> TokenNameTaken | ConstraintViolated: + """Re-raise the name index's complaint in the vocabulary callers expect. + + Two processes can both pass ``_require_name_free`` and then race to + insert; the loser is refused by the unique index, one layer below where + the pre-check runs. The violation ends its transaction, so this can only + happen outside the ``with`` block — see ``ConstraintViolated``. Any other + constraint is not this service's to reinterpret and travels on unchanged. + """ + if _NAME_INDEX_MESSAGE in str(exc): + return TokenNameTaken( + f"a token named {name!r} already exists in workspace " + f"{self._workspace.workspace.name!r}" + ) + return exc + + # ``list`` shadows the builtin for every annotation below it, so it is last. + def list(self) -> list[Token]: + """Every token in this workspace, revoked ones included, in mint order. + + Revoked tokens stay in the listing because the row is the audit record: + an operator asking "what did we ever issue, and what happened to it?" + gets a worse answer from a list that quietly forgets. + """ + with self._workspace.unit_of_work() as uow: + return uow.tokens.list(self._workspace.workspace_id) diff --git a/src/visionset/kernel/services/workspace_service.py b/src/visionset/kernel/services/workspace_service.py index 601a8f13..6e91732b 100644 --- a/src/visionset/kernel/services/workspace_service.py +++ b/src/visionset/kernel/services/workspace_service.py @@ -4,8 +4,8 @@ Every kernel operation happens in the context of exactly one workspace, so this module is also the **single composition point** for the default adapters. It is the only place in the kernel that names ``SqliteMetadataStore``, -``FilesystemBlobStore``, ``InProcessEventBus``, ``PillowImageProcessor`` or -``FfmpegVideoProcessor``; everything above it — later +``FilesystemBlobStore``, ``InProcessEventBus``, ``PillowImageProcessor``, +``FfmpegVideoProcessor`` or ``StoredTokenAuthProvider``; everything above it — later services, the REST surface, the CLI, MCP — receives an open ``WorkspaceService`` and reaches the ports through it. Swapping an adapter is therefore a change to two functions here and to nowhere else. @@ -22,10 +22,11 @@ the workspace while they are there: a copy taken mid-run that includes only ``visionset.db`` is missing whatever has not been checkpointed yet. -Three of the five ports have no line in that layout, and that is the point: the -event bus is in-process and the two media processors are decoders, so none of -them leaves anything behind. They are composed here anyway, because a workspace -is what services are handed and every port has to arrive with it. +Four of the six ports have no line in that layout, and that is the point: the +event bus is in-process, the two media processors are decoders, and the auth +provider reads a table inside the database above, so none of them leaves +anything behind. They are composed here anyway, because a workspace is what +services are handed and every port has to arrive with it. There is no sidecar file carrying the format version. It lives inside the database it describes, for the same reason there is no alembic ledger: a second @@ -62,6 +63,7 @@ InProcessEventBus, PillowImageProcessor, SqliteMetadataStore, + StoredTokenAuthProvider, ) from visionset.kernel.domain import Workspace, normalize_name from visionset.kernel.errors import ( @@ -73,6 +75,7 @@ ) from visionset.kernel.ports import ( UNINITIALIZED, + AuthProvider, BlobStore, EventBus, ImageProcessor, @@ -111,6 +114,11 @@ #: workspace's: a missing ffmpeg is discovered by the call that needs it, so a #: machine without one still opens workspaces and still ingests images. type VideoProcessorFactory = Callable[[], VideoProcessor] +#: Two arguments, unlike every factory above, and the first port that is derived +#: from another one: verifying a token means reading the workspace's own ``token`` +#: table, scoped to the workspace that owns it. ``StoredTokenAuthProvider`` binds +#: both positionally, so the bare class reference still satisfies this type. +type AuthProviderFactory = Callable[[MetadataStore, UUID], AuthProvider] def _resolved(path: Path | str) -> Path: @@ -124,7 +132,7 @@ def _resolved(path: Path | str) -> Path: class WorkspaceService: - """One open workspace: its identity, its directory, and its five ports. + """One open workspace: its identity, its directory, and its six ports. Instances come from :meth:`init` and :meth:`open`. Constructing one directly is the injection seam — hand it ports and nothing here touches a disk. @@ -154,6 +162,7 @@ def __init__( event_bus: EventBus, image_processor: ImageProcessor, video_processor: VideoProcessor, + auth_provider: AuthProvider, ) -> None: self._root = root self._workspace = workspace @@ -162,6 +171,7 @@ def __init__( self._event_bus = event_bus self._image_processor = image_processor self._video_processor = video_processor + self._auth_provider = auth_provider # --- composition: the only two ways to get one ------------------------ @@ -176,6 +186,7 @@ def init( event_bus_factory: EventBusFactory = InProcessEventBus, image_processor_factory: ImageProcessorFactory = PillowImageProcessor, video_processor_factory: VideoProcessorFactory = FfmpegVideoProcessor, + auth_provider_factory: AuthProviderFactory = StoredTokenAuthProvider, ) -> WorkspaceService: """Create a workspace at ``path`` and return it open. @@ -223,9 +234,11 @@ def init( metadata_store.close() _undo_init(root, created_root=created_root) raise - # The three remaining factories are zero-argument and cannot touch the - # disk, so they run outside the block that would undo a half-made - # workspace. A future port whose construction can fail moves inside it. + # The four remaining factories cannot touch the disk — three take no + # arguments at all, and the auth provider only stores the two references + # it is handed — so they run outside the block that would undo a + # half-made workspace. A future port whose construction can fail moves + # inside it. return cls( root, workspace, @@ -234,6 +247,7 @@ def init( event_bus_factory(), image_processor_factory(), video_processor_factory(), + auth_provider_factory(metadata_store, workspace.id), ) @classmethod @@ -246,6 +260,7 @@ def open( event_bus_factory: EventBusFactory = InProcessEventBus, image_processor_factory: ImageProcessorFactory = PillowImageProcessor, video_processor_factory: VideoProcessorFactory = FfmpegVideoProcessor, + auth_provider_factory: AuthProviderFactory = StoredTokenAuthProvider, ) -> WorkspaceService: """Open the workspace at ``path``, migrating it forward if it is behind. @@ -301,6 +316,7 @@ def open( event_bus_factory(), image_processor_factory(), video_processor_factory(), + auth_provider_factory(metadata_store, rows[0].id), ) # --- what the surfaces and the later services read -------------------- @@ -373,6 +389,27 @@ def video_processor(self) -> VideoProcessor: """ return self._video_processor + @property + def auth_provider(self) -> AuthProvider: + """Whether a presented token may operate this workspace. + + The narrow seam all three delivery surfaces authenticate through, and the + one port here that no kernel service uses: it exists for the layers above. + It is composed here anyway, and has to be, because the alternative is a + surface constructing ``StoredTokenAuthProvider`` itself — a delivery module + naming a kernel adapter, which is exactly what this module exists to + prevent. + + The first port derived from another rather than from the path: the default + adapter reads the ``token`` table through the metadata store above, scoped + to this workspace's id. It holds those two references and nothing else, so + :meth:`close` has nothing to do here — closing the store closes what this + reads through. + + Credentials are minted and revoked by ``TokenService``, never here. + """ + return self._auth_provider + @property def format_version(self) -> int: return self._metadata_store.format_version @@ -434,10 +471,11 @@ def require_project_name( def close(self) -> None: """Release the metadata store's connections. Safe to call twice. - The other four ports are not closed and have nothing to close: the blob + The other five ports are not closed and have nothing to close: the blob store addresses files by hash and opens them per call, the event bus holds - a list of callables, and the two media processors hold nothing whatsoever. - Only the database keeps a connection. A frame iterator does own a running + a list of callables, the two media processors hold nothing whatsoever, and + the auth provider holds a reference to the store this line closes. Only + the database keeps a connection. A frame iterator does own a running decoder, but it belongs to whoever asked for it, not to the workspace. """ self._metadata_store.close() diff --git a/src/visionset/server/dependencies.py b/src/visionset/server/dependencies.py new file mode 100644 index 00000000..12bf7e00 --- /dev/null +++ b/src/visionset/server/dependencies.py @@ -0,0 +1,217 @@ +# usage: from visionset.server.dependencies import WorkspaceDep, protected_router +"""What every route is handed: the workspace it serves, and who is allowed in. + +A module of its own rather than more of ``main.py``, and the reason is a cycle +rather than tidiness. Routes will live in their own modules and must import +``require_token``; ``main`` must import those modules to ``include_router`` them. +Put the dependencies in ``main`` and the arrow points both ways. + +**The server serves exactly one workspace.** It is opened lazily, on the first +request that needs it, and kept for the life of the application — never at import +time, because ``scripts/export_openapi.py`` imports the module-level ``app`` in a +checkout that has no workspace, and CI runs it on every push. + +**Every dependency here is ``def``, not ``async def``.** Opening a workspace and +verifying a token are blocking SQLite calls; an ``async def`` dependency runs on +the event loop and would stall it. A sync one gets the threadpool hop FastAPI +already gives sync routes, which is what the synchronous kernel wants anyway. +""" + +from __future__ import annotations + +import os +import threading +from collections.abc import Callable, Sequence +from pathlib import Path +from typing import Annotated, Final + +from fastapi import APIRouter, Depends, HTTPException, status +from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer +from starlette.requests import Request + +from visionset.kernel.ports import AuthProvider +from visionset.kernel.services import WorkspaceService +from visionset.server.errors import ERROR_RESPONSES + +WORKSPACE_ENV_VAR: Final = "VISIONSET_WORKSPACE" +"""Which workspace this server serves. + +Not a server-specific name on purpose: the CLI writes the tokens the server +reads, so the two have to agree on one spelling of "the workspace". +""" + +bearer_scheme: Final = HTTPBearer( + auto_error=False, + description=( + "A workspace API token, created with `visionset token create`. " + "Sent as `Authorization: Bearer `." + ), +) +"""The one security scheme in the contract. + +``auto_error=False`` so that a missing header, an empty credential and a +non-bearer scheme all arrive here as ``None`` and leave as the *same* 401 that an +unknown token gets. With ``auto_error=True`` FastAPI would raise its own +403-or-401 before ``require_token`` ran, and the three cases would be +distinguishable from outside — an oracle for nothing anybody needs to know. + +It only reaches ``components.securitySchemes`` when a route depends on it, which +is why declaring it costs the committed spec nothing until the first protected +route lands. +""" + + +def resolve_workspace_root() -> Path: + """The workspace root this server was pointed at. + + **Provisional. Issue #26 owns the real rule** — the ``--workspace`` flag, the + environment variable and cwd detection, with a documented precedence that + ``visionset ui`` and the flow commands all reuse. This is the smallest thing + #26 can promote: it already reads the variable #26 will keep, so #26 replaces + the *body* of this function and nothing importing it changes. + + Deliberately missing until then: the flag, because a server started by import + string has no argv of its own; and the upward walk for a ``visionset.db`` in a + parent directory. + + A constraint #26 should not discover late: import-linter forbids + ``visionset.server`` importing ``visionset.cli``, so the promoted resolver + cannot live in the CLI. It belongs beside ``DB_FILENAME`` in + ``kernel/services/workspace_service.py``, which already owns the rule that the + database file is what marks a directory as a workspace. + + Read once per open rather than per request: the workspace is opened once. + """ + return Path(os.environ.get(WORKSPACE_ENV_VAR) or Path.cwd()) + + +def _open_configured_workspace() -> WorkspaceService: + return WorkspaceService.open(resolve_workspace_root()) + + +class WorkspaceHandle: + """One lazily opened workspace, shared by every request of one application. + + Held on ``app.state`` rather than in a module-level cache, and that placement + is the whole design. A ``functools.lru_cache`` here would be three lines + shorter and would keep one SQLite connection alive for an entire pytest + process, shared between every app any test built — so each ``create_app()`` + gets its own handle instead, and two applications never share a workspace. + + Constructing one touches no disk. Only :meth:`get` does, and only from inside + a request. + """ + + def __init__(self, open_workspace: Callable[[], WorkspaceService] | None = None) -> None: + self._open = open_workspace or _open_configured_workspace + self._workspace: WorkspaceService | None = None + # Sync dependencies run in a threadpool, so two concurrent first requests + # would otherwise race two ``WorkspaceService.open`` calls — and the loser + # would leak an engine nothing ever closes. + self._lock = threading.Lock() + + @property + def is_open(self) -> bool: + """Whether the workspace has actually been opened yet.""" + return self._workspace is not None + + def get(self) -> WorkspaceService: + """The open workspace, opening it on the first call. + + Raises: + NotAWorkspace: ``VISIONSET_WORKSPACE`` does not name a workspace. + WorkspaceCorrupt: it names one that cannot be read. + WorkspaceFormatTooNew: it was written by a later VisionSet. + """ + if self._workspace is None: + with self._lock: + if self._workspace is None: + self._workspace = self._open() + return self._workspace + + def close(self) -> None: + """Release the workspace if one was ever opened. Safe to call twice.""" + workspace, self._workspace = self._workspace, None + if workspace is not None: + workspace.close() + + +def get_workspace(request: Request) -> WorkspaceService: + """The workspace this application serves. + + Yields the *service*, never a unit of work: a transaction committed in a + dependency's teardown fails after the response has started, which turns a + ``WorkspaceBusy`` into ``RuntimeError: response already started`` instead of + an ``ErrorBody``. See ``docs/api.md``. + """ + handle: WorkspaceHandle = request.app.state.workspace_handle + return handle.get() + + +def get_auth_provider( + workspace: Annotated[WorkspaceService, Depends(get_workspace)], +) -> AuthProvider: + """Whoever decides whether a token may operate this workspace. + + Its own dependency rather than an attribute read inside ``require_token``, + because this is the seam a test overrides: ``app.dependency_overrides`` reach + every sub-dependency recursively, so replacing this replaces authentication + everywhere without a probe application. + """ + return workspace.auth_provider + + +def require_token( + credentials: Annotated[HTTPAuthorizationCredentials | None, Depends(bearer_scheme)], + auth_provider: Annotated[AuthProvider, Depends(get_auth_provider)], +) -> str: + """Refuse the request unless it carries a live token of this workspace. + + Missing, malformed, unknown and revoked are **one** answer, deliberately: a + 401 that distinguished them would let anyone enumerate which credentials + exist. The ``HTTPException`` is rendered as an ``ErrorBody`` by the handlers + ``create_app`` installs, ``headers`` included — which is what keeps the + ``WWW-Authenticate`` challenge on the response. + """ + if credentials is None or not auth_provider.verify(credentials.credentials): + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid or missing bearer token", + headers={"WWW-Authenticate": "Bearer"}, + ) + return credentials.credentials + + +WorkspaceDep = Annotated[WorkspaceService, Depends(get_workspace)] +"""The workspace, for a route that needs to build a service over it.""" + +TokenDep = Annotated[str, Depends(require_token)] +"""The presented token, for the rare route that needs the credential itself. + +A route on a :func:`protected_router` is already guarded; this is only for one +that wants to *read* what it was given. +""" + + +def protected_router(*, prefix: str = "", tags: Sequence[str] | None = None) -> APIRouter: + """A router whose every route requires a valid bearer token. + + The guard and the documented 401 travel together because a route that + declares one without the other is a lie in ``openapi.json`` either way. Build + every non-public router with this rather than repeating + ``Depends(require_token)`` per route: "everything except ``/health``" should + be a property of how routers are constructed, not a thing each reviewer has to + notice. + + 401 is deliberately **not** in ``UNIVERSAL_ERROR_RESPONSES`` and must not be + added to it — ``/health`` is public and cannot 401 — and the guard is on the + router rather than on the application for the same reason. + """ + return APIRouter( + prefix=prefix, + # ``list[str | Enum]`` is what APIRouter declares and ``list`` is + # invariant, so a ``list[str]`` cannot be passed straight through. + tags=list(tags or []), + dependencies=[Depends(require_token)], + responses={401: ERROR_RESPONSES[401]}, + ) diff --git a/src/visionset/server/errors.py b/src/visionset/server/errors.py index 0029a0dd..7ef56b04 100644 --- a/src/visionset/server/errors.py +++ b/src/visionset/server/errors.py @@ -84,6 +84,8 @@ SchemaNotFound, SchemaVersionConflict, SourceNotFound, + TokenNameTaken, + TokenNotFound, UnknownAttribute, UnserializableManifest, UnsupportedGeometry, @@ -172,6 +174,10 @@ class ErrorRule: DatasetNotFound: ErrorRule(404, "DATASET_NOT_FOUND"), AnnotationNotFound: ErrorRule(404, "ANNOTATION_NOT_FOUND"), ReleaseNotFound: ErrorRule(404, "RELEASE_NOT_FOUND"), + # Administering a token an operator named, never failing to authenticate + # with one: a token that does not verify raises nothing at all, so this 404 + # can never become an oracle for which secrets exist. + TokenNotFound: ErrorRule(404, "TOKEN_NOT_FOUND"), # A job's assets are fixed at approval, so an asset outside the segment is # a sub-resource that does not exist — the "reads as missing, not as # forbidden" rule one scope down. A route that takes the asset id in a @@ -185,6 +191,7 @@ class ErrorRule: # --- 409: well-formed request, the resource's state refuses it --------- ProjectNameTaken: ErrorRule(409, "PROJECT_NAME_TAKEN"), ReleaseTagTaken: ErrorRule(409, "RELEASE_TAG_TAKEN"), + TokenNameTaken: ErrorRule(409, "TOKEN_NAME_TAKEN"), WorkspaceAlreadyExists: ErrorRule(409, "WORKSPACE_ALREADY_EXISTS"), WorkspaceNotEmpty: ErrorRule(409, "WORKSPACE_NOT_EMPTY"), # Retryable, but immediately rather than after a wait — a re-read lands on diff --git a/src/visionset/server/main.py b/src/visionset/server/main.py index 2fadb868..4d4fa2b1 100644 --- a/src/visionset/server/main.py +++ b/src/visionset/server/main.py @@ -2,50 +2,15 @@ from __future__ import annotations -import os -from typing import Annotated +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager -from fastapi import APIRouter, Depends, FastAPI, HTTPException, status -from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer +from fastapi import APIRouter, FastAPI from visionset import __version__ -from visionset.kernel.ports import AuthProvider +from visionset.server.dependencies import WorkspaceHandle from visionset.server.errors import UNIVERSAL_ERROR_RESPONSES, install_error_handlers - -class EnvTokenAuthProvider: - """Dev-only AuthProvider: accepts the single token in $VISIONSET_DEV_TOKEN. - - Real token issuance/persistence lands in a later session behind the same port. - """ - - def verify(self, token: str) -> bool: - expected = os.environ.get("VISIONSET_DEV_TOKEN") - return expected is not None and token == expected - - -_auth_provider: AuthProvider = EnvTokenAuthProvider() -_bearer = HTTPBearer(auto_error=False) - - -async def require_token( - credentials: Annotated[HTTPAuthorizationCredentials | None, Depends(_bearer)], -) -> str: - """Bearer-token dependency for every future non-public endpoint. - - The ``HTTPException`` is rendered as an ``ErrorBody`` by the handler - :func:`create_app` installs, headers included — which is what keeps the - ``WWW-Authenticate`` challenge on the response. - """ - if credentials is None or not _auth_provider.verify(credentials.credentials): - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Invalid or missing bearer token", - headers={"WWW-Authenticate": "Bearer"}, - ) - return credentials.credentials - - DESCRIPTION = "REST surface of the VisionSet SDK. The committed openapi.json is the contract." router = APIRouter() @@ -57,6 +22,20 @@ async def health() -> dict[str, str]: return {"status": "ok", "version": __version__} +@asynccontextmanager +async def _lifespan(app: FastAPI) -> AsyncIterator[None]: + """Close the workspace on shutdown, if a request ever opened one. + + Only the closing half. The handle itself is built in :func:`create_app`, + because ``TestClient(app)`` used without its context manager never runs + startup — and a handle that only existed after startup would simply be + missing there. + """ + yield + handle: WorkspaceHandle = app.state.workspace_handle + handle.close() + + def create_app() -> FastAPI: """Build the application. @@ -65,17 +44,32 @@ def create_app() -> FastAPI: below stays regardless: ``scripts/export_openapi.py`` imports it by name, and so does ``uvicorn visionset.server.main:app``. + It takes **no parameters**, and that is a decision rather than an omission. + ``visionset ui`` starts this server by *import string* — import-linter forbids + ``visionset.cli`` importing ``visionset.server``, and ``uvicorn --reload`` + requires the import-string form anyway — so an argument here would be + unreachable from the only production caller. Production configures through + the environment; tests configure through ``app.dependency_overrides``. One + mechanism per audience, and no third. + + Building the workspace handle touches no disk: it is opened by the first + request that needs it, so importing this module in a checkout with no + workspace stays free. + ``responses=`` is applied here rather than route by route on purpose. It puts ``ErrorBody`` in ``components.schemas`` and displaces FastAPI's generated ``HTTPValidationError``, so no route can quietly document a second - error shape. + error shape. 401 is deliberately not among them — ``/health`` is public and + cannot 401 — so protected routers document it themselves. """ app = FastAPI( title="Robomous VisionSet API", version=__version__, description=DESCRIPTION, responses=UNIVERSAL_ERROR_RESPONSES, + lifespan=_lifespan, ) + app.state.workspace_handle = WorkspaceHandle() install_error_handlers(app) app.include_router(router) return app diff --git a/tests/kernel/test_auth_provider.py b/tests/kernel/test_auth_provider.py new file mode 100644 index 00000000..97ab5f2b --- /dev/null +++ b/tests/kernel/test_auth_provider.py @@ -0,0 +1,217 @@ +"""The `AuthProvider` port: what verification promises, and how it is composed. + +Both halves live here, on `test_events.py`'s precedent that a port's own test +file owns the composition-point wiring for it — the alternative scatters "is this +injectable?" across whichever service happened to need it first. +""" + +from collections.abc import Iterator +from pathlib import Path +from uuid import UUID + +import pytest + +from visionset.kernel import ConfirmationRequired, WorkspaceCorrupt +from visionset.kernel.adapters import SqliteMetadataStore, StoredTokenAuthProvider +from visionset.kernel.domain import hash_secret +from visionset.kernel.ports import AuthProvider, MetadataStore +from visionset.kernel.services import TokenService, WorkspaceService + + +class Fixture: + """A workspace, its token service, and the provider that guards it.""" + + def __init__(self, tmp_path: Path, name: str = "ws") -> None: + self.workspace = WorkspaceService.init(tmp_path / name) + self.tokens = TokenService(self.workspace) + + @property + def auth(self) -> AuthProvider: + return self.workspace.auth_provider + + def close(self) -> None: + self.workspace.close() + + +@pytest.fixture() +def fixture(tmp_path: Path) -> Iterator[Fixture]: + made = Fixture(tmp_path) + yield made + made.close() + + +# --- what verification answers --------------------------------------------- + + +def test_a_freshly_issued_secret_verifies(fixture: Fixture) -> None: + issued = fixture.tokens.create("ci") + assert fixture.auth.verify(issued.secret) is True + + +def test_an_unknown_string_does_not_verify(fixture: Fixture) -> None: + fixture.tokens.create("ci") + assert fixture.auth.verify("vst_not-a-real-token") is False + + +def test_an_empty_string_does_not_verify(fixture: Fixture) -> None: + assert fixture.auth.verify("") is False + + +def test_a_workspace_with_no_tokens_verifies_nothing(fixture: Fixture) -> None: + assert fixture.auth.verify("vst_anything") is False + + +def test_the_stored_hash_is_not_itself_a_credential(fixture: Fixture) -> None: + """The obvious escalation from a leaked metadata store, closed by hashing. + + Presenting the digest gets it hashed again, which matches nothing. + """ + issued = fixture.tokens.create("ci") + assert fixture.auth.verify(issued.token.secret_hash) is False + + +def test_a_secret_is_matched_exactly(fixture: Fixture) -> None: + """No trimming and no case folding: a credential is bytes, not a name.""" + issued = fixture.tokens.create("ci") + assert fixture.auth.verify(f" {issued.secret} ") is False + assert fixture.auth.verify(issued.secret.upper()) is False + + +def test_one_workspaces_token_does_not_open_another(tmp_path: Path) -> None: + """Scoping is by ``workspace_id``, not by "the store holds one workspace".""" + first = Fixture(tmp_path, name="one") + second = Fixture(tmp_path, name="two") + issued = first.tokens.create("ci") + + assert first.auth.verify(issued.secret) is True + assert second.auth.verify(issued.secret) is False + + first.close() + second.close() + + +def test_several_tokens_coexist_and_each_verifies(fixture: Fixture) -> None: + first = fixture.tokens.create("one") + second = fixture.tokens.create("two") + assert fixture.auth.verify(first.secret) is True + assert fixture.auth.verify(second.secret) is True + + +# --- revocation is immediate ----------------------------------------------- + + +def test_a_revoked_token_stops_verifying_in_the_same_process(fixture: Fixture) -> None: + """The acceptance criterion, and the reason nothing may be cached. + + No reopen, no restart: the same provider instance answers ``True`` and then + ``False`` across one ``revoke`` call. + """ + issued = fixture.tokens.create("ci") + assert fixture.auth.verify(issued.secret) is True + + fixture.tokens.revoke(issued.token.id, confirm=True) + + assert fixture.auth.verify(issued.secret) is False + + +def test_revoking_one_token_leaves_the_others_working(fixture: Fixture) -> None: + doomed = fixture.tokens.create("old") + kept = fixture.tokens.create("new") + fixture.tokens.revoke(doomed.token.id, confirm=True) + assert fixture.auth.verify(doomed.secret) is False + assert fixture.auth.verify(kept.secret) is True + + +def test_an_unconfirmed_revoke_leaves_the_token_working(fixture: Fixture) -> None: + issued = fixture.tokens.create("ci") + with pytest.raises(ConfirmationRequired): + fixture.tokens.revoke(issued.token.id) + assert fixture.auth.verify(issued.secret) is True + + +# --- an outage is not a bad credential -------------------------------------- + + +def test_an_unreadable_store_raises_rather_than_refusing(tmp_path: Path) -> None: + """Reporting an outage as a bad token sends an operator hunting the wrong bug. + + ``db_path`` is a *directory*, which SQLite answers with ``SQLITE_CANTOPEN`` — + #80's portable way to force a non-lock ``OperationalError``, which the + adapter translates to ``WorkspaceCorrupt``. + """ + broken = tmp_path / "not-a-file.db" + broken.mkdir() + provider = StoredTokenAuthProvider(SqliteMetadataStore(broken), UUID(int=0)) + with pytest.raises(WorkspaceCorrupt): + provider.verify("vst_anything") + + +# --- composition ------------------------------------------------------------ + + +def test_a_workspace_composes_the_stored_token_provider_by_default(fixture: Fixture) -> None: + assert isinstance(fixture.auth, StoredTokenAuthProvider) + assert isinstance(fixture.auth, AuthProvider) + + +class _StubProvider: + """Records what the factory was handed, and accepts exactly one token.""" + + def __init__(self, metadata_store: MetadataStore, workspace_id: UUID) -> None: + self.metadata_store = metadata_store + self.workspace_id = workspace_id + + def verify(self, token: str) -> bool: + return token == "let-me-in" + + +def test_init_honours_an_injected_auth_provider(tmp_path: Path) -> None: + workspace = WorkspaceService.init(tmp_path / "ws", auth_provider_factory=_StubProvider) + assert workspace.auth_provider.verify("let-me-in") is True + assert workspace.auth_provider.verify("nope") is False + workspace.close() + + +def test_open_honours_an_injected_auth_provider(tmp_path: Path) -> None: + WorkspaceService.init(tmp_path / "ws").close() + workspace = WorkspaceService.open(tmp_path / "ws", auth_provider_factory=_StubProvider) + assert workspace.auth_provider.verify("let-me-in") is True + workspace.close() + + +@pytest.mark.parametrize("through", ["init", "open"]) +def test_the_factory_receives_the_store_and_this_workspaces_id( + tmp_path: Path, through: str +) -> None: + """The two arguments are what make the provider scoped rather than global.""" + if through == "init": + workspace = WorkspaceService.init(tmp_path / "ws", auth_provider_factory=_StubProvider) + else: + WorkspaceService.init(tmp_path / "ws").close() + workspace = WorkspaceService.open(tmp_path / "ws", auth_provider_factory=_StubProvider) + + provider = workspace.auth_provider + assert isinstance(provider, _StubProvider) + assert provider.metadata_store is workspace.metadata_store + assert provider.workspace_id == workspace.workspace_id + workspace.close() + + +def test_two_workspaces_get_their_own_providers(tmp_path: Path) -> None: + first = WorkspaceService.init(tmp_path / "one") + second = WorkspaceService.init(tmp_path / "two") + assert first.auth_provider is not second.auth_provider + first.close() + second.close() + + +def test_a_token_survives_a_reopen(tmp_path: Path) -> None: + """The whole point of persisting: the credential outlives the process.""" + workspace = WorkspaceService.init(tmp_path / "ws") + issued = TokenService(workspace).create("ci") + workspace.close() + + reopened = WorkspaceService.open(tmp_path / "ws") + assert reopened.auth_provider.verify(issued.secret) is True + assert TokenService(reopened).get(issued.token.id).secret_hash == hash_secret(issued.secret) + reopened.close() diff --git a/tests/kernel/test_metadata_store.py b/tests/kernel/test_metadata_store.py index c4d9ad31..1db3e2e5 100644 --- a/tests/kernel/test_metadata_store.py +++ b/tests/kernel/test_metadata_store.py @@ -50,9 +50,11 @@ SourceKind, SplitRecipe, TaskGroup, + Token, VideoMetadata, VideoProvenance, Workspace, + hash_secret, ) from visionset.kernel.ports import UNINITIALIZED, MetadataStore, UnitOfWork @@ -171,6 +173,20 @@ def _seed(uow: UnitOfWork) -> list[tuple[str, UUID]]: visionset_version="0.0.1.dev0", ) ) + # Revoked, and with an explicit ``created_at``, for the reason the source + # above is a video one: leaving ``revoked_at`` at its default would leave + # that column NULL in every store test and the nullable timestamp untested. + token = uow.tokens.add( + Token( + workspace_id=workspace.id, + name="ci", + secret_hash=hash_secret("vst_seed"), + created_at=datetime(2026, 7, 27, 8, 30, tzinfo=UTC), + revoked_at=datetime(2026, 7, 27, 9, 30, tzinfo=UTC), + ) + ) + # Appended LAST, and it has to be: the tests below index into this list by + # position, so inserting anywhere else renames every entity after it. return [ ("workspaces", workspace.id), ("projects", project.id), @@ -187,6 +203,7 @@ def _seed(uow: UnitOfWork) -> list[tuple[str, UUID]]: ("dataset_members", member.id), ("dataset_changes", change.id), ("releases", release.id), + ("tokens", token.id), ] diff --git a/tests/kernel/test_migrations.py b/tests/kernel/test_migrations.py index d792abb3..6dbca79a 100644 --- a/tests/kernel/test_migrations.py +++ b/tests/kernel/test_migrations.py @@ -2,6 +2,7 @@ import pytest from sqlalchemy import inspect, text +from sqlalchemy.exc import IntegrityError from visionset.kernel import WorkspaceCorrupt from visionset.kernel.adapters import SqliteMetadataStore @@ -117,6 +118,14 @@ def _downgrade_to_version_one(store: SqliteMetadataStore) -> None: ) ) connection.execute(text("CREATE INDEX ix_ingest_job_source_id ON ingest_job (source_id)")) + # Migration 11's undo. One line, because SQLite drops a table's indexes + # with it, and order-free, because nothing references ``token``. + # + # This line is also the only thing that exercises migration 11. Without + # it the fresh-versus-migrated test below would still pass: the table + # would survive the downgrade and migration 11 would ``checkfirst``-skip, + # so the ``CREATE`` nobody ran would be reported as agreeing with itself. + connection.execute(text("drop table token")) connection.execute(text("update _visionset_meta set format_version = 1")) @@ -589,6 +598,83 @@ def test_migration_ten_leaves_an_asset_it_did_not_render_alone(tmp_path: Path) - store.close() +def test_migration_eleven_gives_a_workspace_somewhere_to_keep_its_tokens(tmp_path: Path) -> None: + store = SqliteMetadataStore(tmp_path / "visionset.db") + store.initialize() + with store.engine.connect() as connection: + columns = {c["name"] for c in inspect(connection).get_columns("token")} + assert columns == {"id", "workspace_id", "name", "secret_hash", "created_at", "revoked_at"} + store.close() + + +def test_migration_eleven_brings_its_indexes_with_it(tmp_path: Path) -> None: + """``Table.create`` emits the indexes too, which is why none is issued alone. + + Migration 8 had to learn the hard way that an index can go missing from a + creation path — it re-issued a ``CREATE`` for one SQLAlchemy could not + reflect. Here the claim is the opposite one and it is worth pinning: if + ``checkfirst`` on a table ever stopped carrying its indexes, uniqueness would + quietly stop being enforced and every name collision would land in the store. + """ + store = SqliteMetadataStore(tmp_path / "visionset.db") + store.initialize() + with store.engine.connect() as connection: + indexes = {i["name"] for i in inspect(connection).get_indexes("token")} + assert {"ix_token_workspace_id", "uq_token_workspace_name"} <= indexes + store.close() + + +def test_migration_eleven_creates_the_table_on_a_database_that_predates_it( + tmp_path: Path, +) -> None: + """The real exercise: from generation 1 the table is gone, so this one builds it. + + ``_downgrade_to_version_one`` drops ``token``, and that line is the only + thing that makes this migration run at all — see the comment there. Without + it the fresh-versus-migrated test would compare a table nobody re-created + against itself. + """ + store = SqliteMetadataStore(tmp_path / "visionset.db") + store.initialize() + _downgrade_to_version_one(store) + with store.engine.connect() as connection: + assert not inspect(connection).has_table("token") + + store.initialize() + + with store.engine.connect() as connection: + assert inspect(connection).has_table("token") + indexes = {i["name"] for i in inspect(connection).get_indexes("token")} + assert {"ix_token_workspace_id", "uq_token_workspace_name"} <= indexes + assert store.format_version == FORMAT_VERSION + store.close() + + +def test_migration_eleven_makes_a_token_name_unique_within_its_workspace(tmp_path: Path) -> None: + """The index is the guarantee; ``TokenService`` supplies the sentence. + + Case-insensitively, so that ``token revoke ci`` cannot find two credentials. + """ + store = SqliteMetadataStore(tmp_path / "visionset.db") + store.initialize() + with store.engine.begin() as connection: + connection.execute(text("insert into workspace (id, name) values ('w', 'ws')")) + connection.execute( + text( + "insert into token (id, workspace_id, name, secret_hash, created_at) " + f"values ('t1', 'w', 'ci', '{'a' * 64}', '2026-07-27T08:00:00+00:00')" + ) + ) + with pytest.raises(IntegrityError), store.engine.begin() as connection: + connection.execute( + text( + "insert into token (id, workspace_id, name, secret_hash, created_at) " + f"values ('t2', 'w', 'CI', '{'b' * 64}', '2026-07-27T08:00:00+00:00')" + ) + ) + store.close() + + def test_a_fresh_database_and_a_migrated_one_have_the_same_schema(tmp_path: Path) -> None: """Migration 1 is ``create_all`` of *current* metadata, so the two paths differ. diff --git a/tests/kernel/test_token.py b/tests/kernel/test_token.py new file mode 100644 index 00000000..b08b06ae --- /dev/null +++ b/tests/kernel/test_token.py @@ -0,0 +1,161 @@ +"""The token domain: a secret that is unguessable, a hash that is one-way. + +Nothing here touches a store or a service. What is under test is the pair of +functions the mint and the check both call, and the invariants the model refuses +to be built without — chiefly that a plaintext cannot end up in ``secret_hash``. +""" + +from datetime import UTC, datetime, timedelta, timezone +from uuid import uuid4 + +import pytest +from pydantic import ValidationError + +from visionset.kernel.domain import ( + SECRET_PREFIX, + IssuedToken, + Token, + generate_secret, + hash_secret, +) + + +def _token(**overrides: object) -> Token: + fields: dict[str, object] = { + "workspace_id": uuid4(), + "name": "ci", + "secret_hash": hash_secret("vst_whatever"), + } + return Token.model_validate(fields | overrides) + + +# --- secrets --------------------------------------------------------------- + + +def test_a_generated_secret_carries_the_prefix() -> None: + assert generate_secret().startswith(SECRET_PREFIX) + + +def test_generated_secrets_do_not_repeat() -> None: + """Not a randomness test — a wiring test. + + A generator that returned a constant, or seeded itself per call, would pass + every other test in this file and hand every workspace the same credential. + """ + assert len({generate_secret() for _ in range(1000)}) == 1000 + + +def test_a_generated_secret_is_long_enough_to_be_unguessable() -> None: + """256 bits is the premise the whole "sha256 rather than a KDF" argument rests on.""" + body = generate_secret().removeprefix(SECRET_PREFIX) + assert len(body) >= 40 # 32 urlsafe-base64 bytes render as 43 characters + + +# --- hashing --------------------------------------------------------------- + + +def test_the_hash_is_lowercase_hex_of_the_right_length() -> None: + digest = hash_secret("vst_abc") + assert len(digest) == 64 + assert digest == digest.lower() + assert set(digest) <= set("0123456789abcdef") + + +def test_the_hash_is_deterministic() -> None: + """Unsalted on purpose: the check has only the presentation to work from.""" + assert hash_secret("vst_abc") == hash_secret("vst_abc") + + +def test_the_hash_is_not_the_secret() -> None: + secret = generate_secret() + assert hash_secret(secret) != secret + + +def test_different_secrets_hash_differently() -> None: + assert hash_secret("vst_a") != hash_secret("vst_b") + + +def test_the_whole_presented_string_is_hashed_prefix_included() -> None: + """Trimming the prefix here would accept a secret the operator never saw.""" + secret = generate_secret() + assert hash_secret(secret) != hash_secret(secret.removeprefix(SECRET_PREFIX)) + + +# --- the model ------------------------------------------------------------- + + +def test_a_token_hashes_its_secret_and_stores_nothing_else() -> None: + secret = generate_secret() + token = _token(secret_hash=hash_secret(secret)) + assert secret not in token.model_dump_json() + + +def test_a_plaintext_secret_is_refused_as_a_hash() -> None: + """The failure this catches is a caller assigning the wrong field. + + Storing the credential in clear would still verify correctly against + itself — the bug would look exactly like the feature working. + """ + with pytest.raises(ValidationError, match="64 lowercase hex"): + _token(secret_hash=generate_secret()) + + +def test_an_uppercase_hash_is_refused() -> None: + with pytest.raises(ValidationError, match="64 lowercase hex"): + _token(secret_hash=hash_secret("vst_abc").upper()) + + +def test_a_fresh_token_is_not_revoked() -> None: + token = _token() + assert token.revoked_at is None + assert token.revoked is False + + +def test_revoked_is_derived_from_the_timestamp() -> None: + """No stored bool to disagree with the timestamp.""" + revoked = _token(revoked_at=datetime.now(UTC)) + assert revoked.revoked is True + assert "revoked" not in revoked.model_dump() + + +def test_timestamps_must_be_timezone_aware() -> None: + naive = datetime(2026, 7, 27, 8, 0) + with pytest.raises(ValidationError, match="timezone-aware"): + _token(created_at=naive) + with pytest.raises(ValidationError, match="timezone-aware"): + _token(revoked_at=naive) + + +def test_timestamps_are_normalized_to_utc() -> None: + elsewhere = datetime(2026, 7, 27, 8, 0, tzinfo=timezone(timedelta(hours=5))) + token = _token(created_at=elsewhere) + assert token.created_at.tzinfo is UTC + assert token.created_at == elsewhere + + +# --- the one showing ------------------------------------------------------- + + +def test_an_issued_token_carries_the_plaintext_and_the_record() -> None: + secret = generate_secret() + issued = IssuedToken(token=_token(secret_hash=hash_secret(secret)), secret=secret) + assert issued.secret == secret + assert hash_secret(issued.secret) == issued.token.secret_hash + + +def test_the_plaintext_is_not_in_the_repr() -> None: + """``repr=False`` is load-bearing: a traceback renders locals. + + Anything that prints the object by accident — a log line, a pytest failure + report, a debugger frame — must not put the credential in a file. + """ + secret = generate_secret() + issued = IssuedToken(token=_token(secret_hash=hash_secret(secret)), secret=secret) + assert secret not in repr(issued) + + +def test_an_issued_token_is_frozen() -> None: + secret = generate_secret() + issued = IssuedToken(token=_token(secret_hash=hash_secret(secret)), secret=secret) + with pytest.raises(ValidationError): + issued.secret = "vst_other" # type: ignore[misc] diff --git a/tests/kernel/test_token_service.py b/tests/kernel/test_token_service.py new file mode 100644 index 00000000..9a9e7c93 --- /dev/null +++ b/tests/kernel/test_token_service.py @@ -0,0 +1,220 @@ +"""`TokenService`: mint once, store a digest, revoke for good. + +The assertions that matter most are negative ones — that no read path can +reproduce a secret, and that a plaintext never reaches the store — because a +failure there looks exactly like the feature working. +""" + +from collections.abc import Iterator +from pathlib import Path +from uuid import uuid4 + +import pytest + +from visionset.kernel import ( + ConfirmationRequired, + ConstraintViolated, + InvalidName, + TokenNameTaken, + TokenNotFound, +) +from visionset.kernel.domain import hash_secret +from visionset.kernel.services import TokenService, WorkspaceService + + +class Fixture: + """One workspace and its token service.""" + + def __init__(self, tmp_path: Path, name: str = "ws") -> None: + self.workspace = WorkspaceService.init(tmp_path / name) + self.tokens = TokenService(self.workspace) + + def close(self) -> None: + self.workspace.close() + + +@pytest.fixture() +def fixture(tmp_path: Path) -> Iterator[Fixture]: + made = Fixture(tmp_path) + yield made + made.close() + + +# --- creation -------------------------------------------------------------- + + +def test_create_returns_the_plaintext_and_stores_only_its_digest(fixture: Fixture) -> None: + issued = fixture.tokens.create("ci") + assert issued.secret + assert issued.token.secret_hash == hash_secret(issued.secret) + assert issued.token.secret_hash != issued.secret + + +def test_no_read_path_can_reproduce_the_secret(fixture: Fixture) -> None: + """The acceptance criterion, asserted against every way back in.""" + issued = fixture.tokens.create("ci") + + by_id = fixture.tokens.get(issued.token.id) + by_name = fixture.tokens.get_by_name("ci") + listed = fixture.tokens.list() + + for reachable in (by_id, by_name, *listed): + assert issued.secret not in reachable.model_dump_json() + + +def test_a_created_token_belongs_to_this_workspace_and_is_live(fixture: Fixture) -> None: + issued = fixture.tokens.create("ci") + assert issued.token.workspace_id == fixture.workspace.workspace_id + assert issued.token.revoked is False + + +def test_two_tokens_never_share_a_secret(fixture: Fixture) -> None: + first = fixture.tokens.create("one") + second = fixture.tokens.create("two") + assert first.secret != second.secret + assert first.token.secret_hash != second.token.secret_hash + + +def test_a_name_is_normalized_before_it_is_stored(fixture: Fixture) -> None: + issued = fixture.tokens.create(" ci ") + assert issued.token.name == "ci" + + +def test_a_blank_name_is_refused(fixture: Fixture) -> None: + with pytest.raises(InvalidName, match="token name"): + fixture.tokens.create(" ") + + +def test_a_duplicate_name_is_refused_case_insensitively(fixture: Fixture) -> None: + fixture.tokens.create("ci") + with pytest.raises(TokenNameTaken, match="ci"): + fixture.tokens.create("CI") + + +def test_a_name_taken_refusal_writes_nothing(fixture: Fixture) -> None: + fixture.tokens.create("ci") + with pytest.raises(TokenNameTaken): + fixture.tokens.create("ci") + assert len(fixture.tokens.list()) == 1 + + +def test_a_lost_name_race_is_reported_as_a_collision(fixture: Fixture) -> None: + """The index's complaint, translated into the vocabulary callers expect. + + Exercised directly rather than by racing two writers: the pre-check folds + Unicode where the index folds ASCII, so it is strictly the stricter of the + two and nothing single-threaded can slip past it. What is under test is the + translation itself, which is the part that runs when a *second process* + passes its own pre-check and loses at commit. + """ + refusal = ConstraintViolated("UNIQUE constraint failed: token.workspace_id, token.name") + translated = fixture.tokens._as_name_collision(refusal, "ci") + assert isinstance(translated, TokenNameTaken) + assert "ci" in str(translated) + + +def test_another_constraint_is_not_reinterpreted(fixture: Fixture) -> None: + """Only the name index's complaint becomes a ``TokenNameTaken``. + + Anything else is not this service's to rename and travels on unchanged. + """ + refusal = ConstraintViolated("FOREIGN KEY constraint failed") + assert fixture.tokens._as_name_collision(refusal, "ci") is refusal + + +# --- reading --------------------------------------------------------------- + + +def test_get_by_name_folds_case(fixture: Fixture) -> None: + issued = fixture.tokens.create("ci") + assert fixture.tokens.get_by_name("CI").id == issued.token.id + + +def test_an_unknown_id_is_not_found(fixture: Fixture) -> None: + with pytest.raises(TokenNotFound, match="no token "): + fixture.tokens.get(uuid4()) + + +def test_an_unknown_name_is_not_found(fixture: Fixture) -> None: + with pytest.raises(TokenNotFound, match="no token named"): + fixture.tokens.get_by_name("nothing") + + +def test_a_token_from_another_workspace_reads_as_missing(tmp_path: Path) -> None: + """Cross-scope references are *missing*, never forbidden.""" + first = Fixture(tmp_path, name="one") + second = Fixture(tmp_path, name="two") + issued = first.tokens.create("ci") + + with pytest.raises(TokenNotFound): + second.tokens.get(issued.token.id) + with pytest.raises(TokenNotFound): + second.tokens.get_by_name("ci") + assert second.tokens.list() == [] + + first.close() + second.close() + + +def test_list_is_mint_order_and_keeps_revoked_tokens(fixture: Fixture) -> None: + first = fixture.tokens.create("one") + second = fixture.tokens.create("two") + fixture.tokens.revoke(first.token.id, confirm=True) + + listed = fixture.tokens.list() + assert [token.id for token in listed] == [first.token.id, second.token.id] + assert [token.revoked for token in listed] == [True, False] + + +# --- revocation ------------------------------------------------------------ + + +def test_revoke_requires_confirmation(fixture: Fixture) -> None: + issued = fixture.tokens.create("ci") + with pytest.raises(ConfirmationRequired, match="cannot be undone"): + fixture.tokens.revoke(issued.token.id) + assert fixture.tokens.get(issued.token.id).revoked is False + + +def test_revoke_marks_the_token_and_records_when(fixture: Fixture) -> None: + issued = fixture.tokens.create("ci") + revoked = fixture.tokens.revoke(issued.token.id, confirm=True) + assert revoked.revoked is True + assert revoked.revoked_at is not None + assert fixture.tokens.get(issued.token.id).revoked_at == revoked.revoked_at + + +def test_an_unknown_id_is_not_found_with_or_without_confirm(fixture: Fixture) -> None: + """Existence is checked first, so the flag never changes which error fires.""" + missing = uuid4() + with pytest.raises(TokenNotFound): + fixture.tokens.revoke(missing) + with pytest.raises(TokenNotFound): + fixture.tokens.revoke(missing, confirm=True) + + +def test_revoking_twice_is_a_no_op_that_keeps_the_first_timestamp(fixture: Fixture) -> None: + """A retried command must not move the moment the credential died.""" + issued = fixture.tokens.create("ci") + first = fixture.tokens.revoke(issued.token.id, confirm=True) + again = fixture.tokens.revoke(issued.token.id, confirm=True) + assert again.revoked_at == first.revoked_at + + +def test_revoking_an_already_revoked_token_needs_no_confirmation(fixture: Fixture) -> None: + """Nothing is destroyed the second time, so nothing is guarded.""" + issued = fixture.tokens.create("ci") + fixture.tokens.revoke(issued.token.id, confirm=True) + assert fixture.tokens.revoke(issued.token.id).revoked is True + + +def test_revocation_does_not_free_the_name(fixture: Fixture) -> None: + """The row is the audit record, so the name it holds stays held. + + Deliberate: reusing the name of a burned credential would make a log entry + ambiguous about which ``ci`` it refers to. + """ + issued = fixture.tokens.create("ci") + fixture.tokens.revoke(issued.token.id, confirm=True) + with pytest.raises(TokenNameTaken): + fixture.tokens.create("ci") diff --git a/tests/server/_openapi.py b/tests/server/_openapi.py new file mode 100644 index 00000000..86733536 --- /dev/null +++ b/tests/server/_openapi.py @@ -0,0 +1,52 @@ +"""One definition of "an operation", shared by everything that walks the spec. + +Two test modules now iterate ``openapi()["paths"]``, and they must agree on what +a path item contains. OpenAPI allows keys there that are *not* operations — +``parameters``, ``summary``, ``$ref``, ``servers`` — so a walk that treats every +key as an operation raises ``KeyError`` on ``operation["responses"]`` rather than +failing with a sentence about the contract. FastAPI emits none of those today; +this module is what keeps that from being load-bearing. +""" + +from collections.abc import Iterator +from typing import Any, Final + +OPERATION_KEYS: Final = frozenset( + {"get", "put", "post", "delete", "options", "head", "patch", "trace"} +) +"""The HTTP methods an OpenAPI path item may key an operation by.""" + +PUBLIC_OPERATIONS: Final = frozenset({("/health", "get")}) +"""Every operation that is allowed to answer without a token. + +One entry, and adding a second is a security decision that belongs in a review — +which is the point of writing the exception list here rather than skipping +unauthenticated routes in the walk. +""" + +BEARER_SECURITY: Final = [{"HTTPBearer": []}] +"""What FastAPI emits on an operation guarded by ``protected_router()``.""" + + +def operations(spec: dict[str, Any]) -> Iterator[tuple[str, str, dict[str, Any]]]: + """``(path, method, operation)`` for every documented operation in ``spec``.""" + for path, item in spec["paths"].items(): + for method, operation in item.items(): + if method in OPERATION_KEYS: + yield path, method, operation + + +def assert_every_operation_is_protected(spec: dict[str, Any]) -> None: + """Every documented operation but the public ones requires a bearer token. + + Raises ``AssertionError`` naming the offending operation, so a failure reads + as "POST /projects declares no bearer security" rather than as a diff of two + large dictionaries. + """ + for path, method, operation in operations(spec): + where = f"{method.upper()} {path}" + if (path, method) in PUBLIC_OPERATIONS: + assert "security" not in operation, f"{where} is public but declares security" + continue + assert operation.get("security") == BEARER_SECURITY, f"{where} declares no bearer security" + assert "401" in operation["responses"], f"{where} does not document its 401" diff --git a/tests/server/_probe.py b/tests/server/_probe.py new file mode 100644 index 00000000..c4a5b3a6 --- /dev/null +++ b/tests/server/_probe.py @@ -0,0 +1,99 @@ +"""Building blocks for the server tests: probe apps, and a stand-in provider. + +Plain functions rather than pytest fixtures, and there is deliberately still no +`conftest.py` anywhere in this repository — `tests/fixtures/media.py` is the +precedent, and each test module wraps what it needs in its own two-line fixture. + +Every probe app is built by the **real** `create_app()`, never by a bare +`FastAPI()`. That is what makes a probe exercise the real handler set, the real +lifespan and the real router configuration; the only thing that must not happen +is mounting a route on the exported `app`, because it would land in +`openapi.json` and trip the CI drift gate. +""" + +from collections.abc import Callable +from pathlib import Path +from typing import Final + +from fastapi import FastAPI + +from visionset.kernel.services import WorkspaceService +from visionset.server.dependencies import ( + TokenDep, + WorkspaceHandle, + get_auth_provider, + protected_router, +) +from visionset.server.main import create_app + +PROBE_PATH: Final = "/probe/whoami" +"""The one protected route the probe apps carry.""" + +KNOWN_TOKEN: Final = "vst_known-good-token" +"""What :class:`StubAuthProvider` accepts, for tests that do not want a workspace.""" + + +class StubAuthProvider: + """Accepts exactly one token, and counts what it was asked about.""" + + def __init__(self, accepted: str = KNOWN_TOKEN) -> None: + self.accepted = accepted + self.presented: list[str] = [] + + def verify(self, token: str) -> bool: + self.presented.append(token) + return token == self.accepted + + +def probe_app() -> FastAPI: + """A real application carrying one protected route, mounted off the exported one.""" + app = create_app() + router = protected_router(prefix="/probe", tags=["probe"]) + + @router.get("/whoami") + def whoami(token: TokenDep) -> dict[str, str]: + return {"token": token} + + app.include_router(router) + return app + + +def stubbed_app(provider: StubAuthProvider) -> FastAPI: + """A probe app whose authentication is ``provider``, no workspace involved. + + Overriding ``get_auth_provider`` rather than ``require_token`` keeps the real + credential handling under test — the header parsing, the challenge, the body + — and replaces only the thing that would otherwise need a workspace on disk. + """ + app = probe_app() + app.dependency_overrides[get_auth_provider] = lambda: provider + return app + + +def workspace_app(root: Path) -> FastAPI: + """A probe app serving a real workspace at ``root``, opened lazily as usual. + + The handle is replaced rather than the environment patched, so the test says + which workspace it means instead of relying on process-wide state. + """ + app = probe_app() + app.state.workspace_handle = handle_for(root) + return app + + +def handle_for(root: Path) -> WorkspaceHandle: + """A handle that opens the workspace at ``root`` when first asked.""" + return WorkspaceHandle(lambda: WorkspaceService.open(root)) + + +def counting_handle( + open_workspace: Callable[[], WorkspaceService], +) -> tuple[WorkspaceHandle, list[int]]: + """A handle plus a one-element list counting how often it opened anything.""" + calls = [0] + + def opener() -> WorkspaceService: + calls[0] += 1 + return open_workspace() + + return WorkspaceHandle(opener), calls diff --git a/tests/server/test_auth.py b/tests/server/test_auth.py new file mode 100644 index 00000000..d91af710 --- /dev/null +++ b/tests/server/test_auth.py @@ -0,0 +1,198 @@ +"""Bearer authentication: one answer for every way of not being allowed in. + +Two layers are exercised here. Most tests stub the provider through +`app.dependency_overrides` — the mechanism this issue exists to make possible, +and which had zero uses in this repository before it. The last two open a real +workspace and mint a real token, because "a persisted token authenticates" and +"a revoked one stops immediately" are claims about the whole path or about +nothing. +""" + +from collections.abc import Iterator +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient +from tests.server._probe import ( + KNOWN_TOKEN, + PROBE_PATH, + StubAuthProvider, + stubbed_app, + workspace_app, +) + +from visionset.kernel.services import TokenService, WorkspaceService +from visionset.server.main import app + + +@pytest.fixture() +def provider() -> StubAuthProvider: + return StubAuthProvider() + + +@pytest.fixture() +def client(provider: StubAuthProvider) -> Iterator[TestClient]: + with TestClient(stubbed_app(provider)) as made: + yield made + + +# --- refusals: all of them identical --------------------------------------- + + +@pytest.mark.parametrize( + ("label", "headers"), + [ + ("no header at all", {}), + ("an unknown token", {"Authorization": "Bearer vst_nope"}), + ("a bearer with no credential", {"Authorization": "Bearer"}), + ("a non-bearer scheme", {"Authorization": "Basic dXNlcjpwYXNz"}), + ("a bare token with no scheme", {"Authorization": KNOWN_TOKEN}), + ], +) +def test_a_request_without_a_valid_token_is_refused( + client: TestClient, label: str, headers: dict[str, str] +) -> None: + assert client.get(PROBE_PATH, headers=headers).status_code == 401, label + + +def test_every_refusal_gives_byte_identical_answers(client: TestClient) -> None: + """The no-oracle rule: a 401 must not say *why* it refused. + + A body that distinguished "no such token" from "revoked" from "malformed" + would let anyone probe which credentials a workspace holds, one request at a + time. One answer, so there is nothing to learn from asking. + """ + missing = client.get(PROBE_PATH) + unknown = client.get(PROBE_PATH, headers={"Authorization": "Bearer vst_nope"}) + malformed = client.get(PROBE_PATH, headers={"Authorization": "Basic nope"}) + + assert len({missing.text, unknown.text, malformed.text}) == 1 + assert {missing.status_code, unknown.status_code, malformed.status_code} == {401} + + +def test_the_401_is_an_error_body_with_the_challenge(client: TestClient) -> None: + response = client.get(PROBE_PATH) + assert response.json() == { + "code": "UNAUTHORIZED", + "message": "Invalid or missing bearer token", + "detail": None, + } + assert response.headers["WWW-Authenticate"] == "Bearer" + + +# --- acceptance ------------------------------------------------------------- + + +def test_a_valid_token_reaches_the_route(client: TestClient) -> None: + response = client.get(PROBE_PATH, headers={"Authorization": f"Bearer {KNOWN_TOKEN}"}) + assert response.status_code == 200 + assert response.json() == {"token": KNOWN_TOKEN} + + +def test_the_credential_is_passed_through_untouched( + client: TestClient, provider: StubAuthProvider +) -> None: + """No case folding and no prefix handling in the server. + + A token is bytes an operator was handed. Anything the server 'helpfully' + normalizes is a character that stops mattering, which is entropy given away. + """ + client.get(PROBE_PATH, headers={"Authorization": "Bearer vst_MiXeD-CaSe"}) + assert provider.presented == ["vst_MiXeD-CaSe"] + + +def test_health_needs_no_token(client: TestClient) -> None: + assert client.get("/health").status_code == 200 + + +def test_the_exported_app_is_not_the_one_under_test() -> None: + """Probe routes live on a factory-built app, never on the exported one. + + Mounting them on ``app`` would put them in ``openapi.json`` and trip the CI + drift gate — the reason this file builds its own applications at all. + """ + assert PROBE_PATH not in app.openapi()["paths"] + + +# --- the override mechanism itself ------------------------------------------ + + +def test_dependency_overrides_replace_authentication_without_a_probe_app() -> None: + """What #25 bought: the provider is a dependency, not a module global. + + Before this, swapping authentication meant a bare ``FastAPI()`` carrying a + hand-written route — which exercised neither the real handlers nor the real + router configuration. Overriding ``get_auth_provider`` reaches + ``require_token`` through the graph, so the real thing is under test. + """ + accepting = StubAuthProvider(accepted="only-this") + with TestClient(stubbed_app(accepting)) as client: + allowed = client.get(PROBE_PATH, headers={"Authorization": "Bearer only-this"}) + refused = client.get(PROBE_PATH, headers={"Authorization": "Bearer other"}) + assert (allowed.status_code, refused.status_code) == (200, 401) + + +def test_an_override_does_not_leak_between_applications(provider: StubAuthProvider) -> None: + """Overrides live on the app, so two apps cannot contaminate each other.""" + permissive = stubbed_app(StubAuthProvider(accepted="anything")) + strict = stubbed_app(provider) + assert permissive.dependency_overrides != {} + with TestClient(strict) as client: + response = client.get(PROBE_PATH, headers={"Authorization": "Bearer anything"}) + assert response.status_code == 401 + + +# --- against a real workspace ------------------------------------------------ + + +@pytest.fixture() +def workspace(tmp_path: Path) -> Iterator[WorkspaceService]: + made = WorkspaceService.init(tmp_path / "ws") + yield made + made.close() + + +def test_a_persisted_token_authenticates(tmp_path: Path, workspace: WorkspaceService) -> None: + """End to end: minted by the SDK, presented over HTTP, verified from the store.""" + issued = TokenService(workspace).create("ci") + workspace.close() + + with TestClient(workspace_app(tmp_path / "ws")) as client: + response = client.get(PROBE_PATH, headers={"Authorization": f"Bearer {issued.secret}"}) + assert response.status_code == 200 + assert response.json() == {"token": issued.secret} + + +def test_a_revoked_token_is_refused_immediately( + tmp_path: Path, workspace: WorkspaceService +) -> None: + """The #25 acceptance criterion. No restart, and no cache to invalidate. + + The same running application answers 200 and then 401 across one ``revoke`` + call made through a *different* handle on the same workspace — which is what + an operator running ``visionset token revoke`` beside a live server does. + """ + issued = TokenService(workspace).create("ci") + workspace.close() + + with TestClient(workspace_app(tmp_path / "ws")) as client: + header = {"Authorization": f"Bearer {issued.secret}"} + assert client.get(PROBE_PATH, headers=header).status_code == 200 + + beside = WorkspaceService.open(tmp_path / "ws") + TokenService(beside).revoke(issued.token.id, confirm=True) + beside.close() + + assert client.get(PROBE_PATH, headers=header).status_code == 401 + + +def test_a_token_from_another_workspace_does_not_open_this_one(tmp_path: Path) -> None: + served = WorkspaceService.init(tmp_path / "served") + served.close() + other = WorkspaceService.init(tmp_path / "other") + issued = TokenService(other).create("ci") + other.close() + + with TestClient(workspace_app(tmp_path / "served")) as client: + response = client.get(PROBE_PATH, headers={"Authorization": f"Bearer {issued.secret}"}) + assert response.status_code == 401 diff --git a/tests/server/test_errors.py b/tests/server/test_errors.py index 9122c202..90dd78da 100644 --- a/tests/server/test_errors.py +++ b/tests/server/test_errors.py @@ -10,12 +10,13 @@ import inspect import re from collections.abc import Iterator -from typing import Annotated import pytest -from fastapi import Depends, FastAPI +from fastapi import FastAPI from fastapi.testclient import TestClient from pydantic import BaseModel +from tests.server._openapi import operations +from tests.server._probe import PROBE_PATH, StubAuthProvider, stubbed_app from visionset.kernel import ( AssetNotInJob, @@ -38,7 +39,7 @@ install_error_handlers, rule_for, ) -from visionset.server.main import app, require_token +from visionset.server.main import app # --- the table ------------------------------------------------------------ @@ -56,11 +57,13 @@ "DatasetNotFound": (404, "DATASET_NOT_FOUND"), "AnnotationNotFound": (404, "ANNOTATION_NOT_FOUND"), "ReleaseNotFound": (404, "RELEASE_NOT_FOUND"), + "TokenNotFound": (404, "TOKEN_NOT_FOUND"), "AssetNotInJob": (404, "ASSET_NOT_IN_JOB"), "NoSplitRecipe": (404, "NO_SPLIT_RECIPE"), # 409 — well-formed request, the resource's state refuses it "ProjectNameTaken": (409, "PROJECT_NAME_TAKEN"), "ReleaseTagTaken": (409, "RELEASE_TAG_TAKEN"), + "TokenNameTaken": (409, "TOKEN_NAME_TAKEN"), "WorkspaceAlreadyExists": (409, "WORKSPACE_ALREADY_EXISTS"), "WorkspaceNotEmpty": (409, "WORKSPACE_NOT_EMPTY"), "SchemaVersionConflict": (409, "SCHEMA_VERSION_CONFLICT"), @@ -341,14 +344,19 @@ def test_a_wrong_method_speaks_the_same_schema(probe: TestClient) -> None: def test_a_401_carries_the_error_body_and_keeps_its_challenge() -> None: - probe_app = FastAPI() - install_error_handlers(probe_app) + """Unchanged from when the provider was a module global, and asserted so. - @probe_app.get("/protected") - async def protected(token: Annotated[str, Depends(require_token)]) -> dict[str, bool]: - return {"ok": True} + Built on a real ``create_app()`` probe now that ``require_token`` resolves + its provider through the dependency graph; every assertion below is verbatim + what this test made before that move, which is what proves the reshuffle + changed no behaviour. - response = TestClient(probe_app).get("/protected") + The provider is stubbed rather than real because ``get_auth_provider`` + depends on ``get_workspace``: with no workspace configured the sub-dependency + fails first and the answer is a 500, not a 401. ``test_auth.py`` covers that + ordering deliberately; here it would only be in the way. + """ + response = TestClient(stubbed_app(StubAuthProvider())).get(PROBE_PATH) assert response.status_code == 401 assert response.json() == { "code": "UNAUTHORIZED", @@ -373,7 +381,14 @@ def test_the_error_body_is_the_only_error_schema_in_the_contract() -> None: def test_every_route_documents_the_universal_error_responses() -> None: - for path, operations in app.openapi()["paths"].items(): - for method, operation in operations.items(): - declared = set(operation["responses"]) - assert {"422", "500", "503"} <= declared, f"{method.upper()} {path}" + """Walked through the shared helper, which knows what an operation is. + + The hand-rolled loop this replaced iterated *every* key under a path item. + OpenAPI allows non-operation keys there — ``parameters``, ``summary`` — and + FastAPI emits none of them today, so it passed; the day one appeared it would + have raised ``KeyError`` instead of failing with a sentence. ``_openapi.py`` + now owns that definition for both walks over this table. + """ + for path, method, operation in operations(app.openapi()): + declared = set(operation["responses"]) + assert {"422", "500", "503"} <= declared, f"{method.upper()} {path}" diff --git a/tests/server/test_health.py b/tests/server/test_health.py index c7e690bd..276f3c0a 100644 --- a/tests/server/test_health.py +++ b/tests/server/test_health.py @@ -1,11 +1,14 @@ -from typing import Annotated +"""`/health`: the one public operation, and the one that must never need a token. + +The auth tests this file used to carry moved to `test_auth.py` when the provider +stopped being a module global. What is left is the liveness probe itself, and the +claim that it stays reachable without a credential. +""" -import pytest -from fastapi import Depends, FastAPI from fastapi.testclient import TestClient from visionset import __version__ -from visionset.server.main import app, require_token +from visionset.server.main import app client = TestClient(app) @@ -20,28 +23,11 @@ def test_health_is_in_openapi_contract() -> None: assert "/health" in app.openapi()["paths"] -@pytest.fixture() -def protected_client(monkeypatch: pytest.MonkeyPatch) -> TestClient: - monkeypatch.setenv("VISIONSET_DEV_TOKEN", "dev-secret") - probe = FastAPI() - - @probe.get("/protected") - async def protected(token: Annotated[str, Depends(require_token)]) -> dict[str, bool]: - return {"ok": True} - - return TestClient(probe) - - -def test_auth_dependency_rejects_missing_token(protected_client: TestClient) -> None: - assert protected_client.get("/protected").status_code == 401 +def test_health_needs_no_workspace() -> None: + """Answered without opening anything, which is what makes it a liveness probe. - -def test_auth_dependency_rejects_wrong_token(protected_client: TestClient) -> None: - response = protected_client.get("/protected", headers={"Authorization": "Bearer nope"}) - assert response.status_code == 401 - - -def test_auth_dependency_accepts_dev_token(protected_client: TestClient) -> None: - response = protected_client.get("/protected", headers={"Authorization": "Bearer dev-secret"}) - assert response.status_code == 200 - assert response.json() == {"ok": True} + A container is healthy before ``visionset init`` has ever run inside it, and + a probe that opened the workspace would report a deployment fault as death. + """ + client.get("/health") + assert app.state.workspace_handle.is_open is False diff --git a/tests/server/test_openapi_contract.py b/tests/server/test_openapi_contract.py new file mode 100644 index 00000000..a1a5dbd1 --- /dev/null +++ b/tests/server/test_openapi_contract.py @@ -0,0 +1,115 @@ +"""The acceptance walk: every documented operation but `/health` needs a token. + +**Vacuous today, and deliberately committed anyway.** `/health` is the only +operation in the contract, so the walk takes its public branch once and asserts +nothing about authentication. It is a tripwire, not a check: it fires the moment +a route lands that did not come from `protected_router()`. The tests that prove +the walk can *fail* are what make a vacuous assertion worth having — without +them, "it passes" would be indistinguishable from "it looks at nothing". + +The walk only sees *documented* operations. `/docs`, `/redoc` and +`/openapi.json` are `include_in_schema=False` and stay public by decision: the +spec is already a committed artifact in a public repository, and a contract you +must authenticate to read is a contract nobody generates a client from. +""" + +from typing import Any + +import pytest +from fastapi import APIRouter, Depends +from tests.server._openapi import assert_every_operation_is_protected, operations +from tests.server._probe import probe_app + +from visionset.server.dependencies import require_token +from visionset.server.main import app, create_app + + +def test_every_documented_operation_except_health_requires_a_token() -> None: + assert_every_operation_is_protected(app.openapi()) + + +def test_the_committed_contract_has_no_security_scheme_yet() -> None: + """Nothing is protected yet, so the scheme is not in the spec — by design. + + FastAPI collects security definitions per *route*, from its dependency tree. + Declaring ``bearer_scheme`` at module level emits nothing; the scheme enters + ``components`` with the first route that depends on it, which is why this PR + moves ``openapi.json`` not at all and the first endpoint task moves it twice. + """ + assert "securitySchemes" not in app.openapi().get("components", {}) + + +def test_a_protected_route_declares_the_bearer_scheme_and_its_401() -> None: + spec = probe_app().openapi() + operation = spec["paths"]["/probe/whoami"]["get"] + assert operation["security"] == [{"HTTPBearer": []}] + assert "401" in operation["responses"] + assert_every_operation_is_protected(spec) + + +def test_the_bearer_scheme_enters_the_spec_with_this_exact_shape() -> None: + """Pinned here so the diff #27 commits is a decision already reviewed.""" + schemes = probe_app().openapi()["components"]["securitySchemes"] + assert set(schemes) == {"HTTPBearer"} + assert schemes["HTTPBearer"]["type"] == "http" + assert schemes["HTTPBearer"]["scheme"] == "bearer" + assert "visionset token create" in schemes["HTTPBearer"]["description"] + + +def test_the_walk_catches_a_route_that_forgot_the_token() -> None: + """The failure this tripwire exists for: a route mounted the plain way.""" + leaky = create_app() + + @leaky.get("/leak") + def leak() -> dict[str, bool]: + return {"ok": True} + + with pytest.raises(AssertionError, match="declares no bearer security"): + assert_every_operation_is_protected(leaky.openapi()) + + +def test_the_walk_catches_a_protected_route_that_does_not_document_its_401() -> None: + """Guarded but undocumented is still a lie in the contract. + + Which is why ``protected_router()`` carries the dependency and the 401 + together rather than leaving the second to each route. + """ + undocumented = create_app() + router = APIRouter(dependencies=[Depends(require_token)]) + + @router.get("/quiet") + def quiet() -> dict[str, bool]: + return {"ok": True} + + undocumented.include_router(router) + + with pytest.raises(AssertionError, match="does not document its 401"): + assert_every_operation_is_protected(undocumented.openapi()) + + +def test_the_walk_catches_a_public_route_that_should_not_be_public() -> None: + """``/health`` is exempt because it is listed, not because it is unguarded.""" + spec: dict[str, Any] = { + "paths": {"/health": {"get": {"security": [{"HTTPBearer": []}], "responses": {}}}} + } + with pytest.raises(AssertionError, match="is public but declares security"): + assert_every_operation_is_protected(spec) + + +def test_the_walk_skips_non_operation_keys_in_a_path_item() -> None: + """OpenAPI allows them; a walk that assumed otherwise would raise, not fail. + + FastAPI emits none today, which is exactly why this is asserted against a + hand-built spec rather than against the application. + """ + spec: dict[str, Any] = { + "paths": { + "/health": { + "summary": "not an operation", + "parameters": [{"name": "trace", "in": "query"}], + "get": {"responses": {}}, + } + } + } + assert [method for _, method, _ in operations(spec)] == ["get"] + assert_every_operation_is_protected(spec) diff --git a/tests/server/test_workspace_dependency.py b/tests/server/test_workspace_dependency.py new file mode 100644 index 00000000..33ee0838 --- /dev/null +++ b/tests/server/test_workspace_dependency.py @@ -0,0 +1,191 @@ +"""The workspace the server serves: opened once, lazily, per application. + +The load-bearing property is negative — **importing this package must not open a +workspace** — because `scripts/export_openapi.py` imports the module-level `app` +in a checkout that has none, and the CI drift gate runs it on every push. +""" + +import subprocess +import sys +import threading +from collections.abc import Iterator +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient +from tests.server._probe import PROBE_PATH, counting_handle, handle_for, probe_app, workspace_app + +from visionset.kernel.services import TokenService, WorkspaceService +from visionset.server.dependencies import WORKSPACE_ENV_VAR, WorkspaceHandle, resolve_workspace_root +from visionset.server.main import create_app + + +@pytest.fixture() +def workspace_root(tmp_path: Path) -> Iterator[Path]: + root = tmp_path / "ws" + made = WorkspaceService.init(root) + made.close() + yield root + + +# --- nothing opens at import or at build time ------------------------------- + + +def test_creating_the_app_opens_no_workspace() -> None: + assert create_app().state.workspace_handle.is_open is False + + +def test_importing_the_module_level_app_opens_no_workspace(tmp_path: Path) -> None: + """Run in a fresh process from a directory that is not a workspace. + + The `test_kernel_purity.py` pattern, and for the same reason: what is under + test is what an *import* does, which cannot be observed from inside a process + that has already imported. This is literally what the CI OpenAPI job does. + """ + probe = ( + "from visionset.server.main import app\n" + "assert app.state.workspace_handle.is_open is False\n" + "assert '/health' in app.openapi()['paths']\n" + ) + result = subprocess.run( + [sys.executable, "-c", probe], + capture_output=True, + text=True, + cwd=tmp_path, + ) + assert result.returncode == 0, result.stderr + + +# --- opened once, and only once --------------------------------------------- + + +def test_the_workspace_is_opened_once_across_requests(workspace_root: Path) -> None: + handle, calls = counting_handle(lambda: WorkspaceService.open(workspace_root)) + app = probe_app() + app.state.workspace_handle = handle + + with TestClient(app) as client: + for _ in range(3): + client.get(PROBE_PATH) + + assert calls == [1] + + +def test_the_handle_opens_once_under_concurrent_first_calls(workspace_root: Path) -> None: + """Sync dependencies run in a threadpool, so the first call really can race. + + Without the lock the loser's ``WorkspaceService`` is overwritten and its + SQLite engine is never closed — a leak that no single-threaded test can see. + """ + handle, calls = counting_handle(lambda: WorkspaceService.open(workspace_root)) + start = threading.Event() + threads = [threading.Thread(target=lambda: (start.wait(), handle.get())) for _ in range(8)] + for thread in threads: + thread.start() + start.set() + for thread in threads: + thread.join(timeout=10) + assert not thread.is_alive() + + assert calls == [1] + handle.close() + + +def test_two_applications_do_not_share_a_workspace(workspace_root: Path) -> None: + """The property that rules out a module-level cache.""" + first = workspace_app(workspace_root) + second = workspace_app(workspace_root) + assert first.state.workspace_handle is not second.state.workspace_handle + + +# --- shutdown ---------------------------------------------------------------- + + +def test_shutdown_closes_the_workspace(workspace_root: Path) -> None: + app = workspace_app(workspace_root) + with TestClient(app) as client: + client.get(PROBE_PATH) + assert app.state.workspace_handle.is_open is True + assert app.state.workspace_handle.is_open is False + + +def test_closing_a_handle_that_never_opened_is_safe() -> None: + WorkspaceHandle().close() + + +def test_closing_twice_is_safe(workspace_root: Path) -> None: + handle = handle_for(workspace_root) + handle.get() + handle.close() + handle.close() + + +# --- resolution (provisional; #26 owns the real rule) ------------------------ + + +def test_the_environment_variable_names_the_workspace_root( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.setenv(WORKSPACE_ENV_VAR, str(tmp_path / "elsewhere")) + assert resolve_workspace_root() == tmp_path / "elsewhere" + + +def test_the_workspace_defaults_to_the_working_directory( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.delenv(WORKSPACE_ENV_VAR, raising=False) + monkeypatch.chdir(tmp_path) + assert resolve_workspace_root() == tmp_path + + +def test_an_empty_environment_variable_falls_back_to_the_working_directory( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """An unset variable and one set to "" mean the same thing to a shell.""" + monkeypatch.setenv(WORKSPACE_ENV_VAR, "") + monkeypatch.chdir(tmp_path) + assert resolve_workspace_root() == tmp_path + + +def test_the_configured_workspace_is_the_one_that_is_served( + monkeypatch: pytest.MonkeyPatch, workspace_root: Path +) -> None: + """End to end through the environment, the way a deployment configures it.""" + served = WorkspaceService.open(workspace_root) + issued = TokenService(served).create("ci") + served.close() + + monkeypatch.setenv(WORKSPACE_ENV_VAR, str(workspace_root)) + with TestClient(probe_app()) as client: + response = client.get(PROBE_PATH, headers={"Authorization": f"Bearer {issued.secret}"}) + assert response.status_code == 200 + + +# --- a server pointed at nothing --------------------------------------------- + + +def test_a_server_pointed_at_a_non_workspace_answers_500_not_a_workspace( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """A deployment fault, reported as one — and never as a 401. + + ``get_auth_provider`` depends on ``get_workspace``, so a misconfigured server + fails before the credential is even looked at. That ordering is deliberate: + resolving the provider outside the dependency graph would be the only way to + check the token first, and it would take ``dependency_overrides`` with it. + + The path is in the log and not in the body, on ``NOT_A_WORKSPACE``'s rule that + a 5xx message naming the server's own filesystem is the server's business. + """ + monkeypatch.setenv(WORKSPACE_ENV_VAR, str(tmp_path / "nothing-here")) + with TestClient(probe_app(), raise_server_exceptions=False) as client: + response = client.get(PROBE_PATH, headers={"Authorization": "Bearer vst_whatever"}) + + assert response.status_code == 500 + body = response.json() + assert body["code"] == "NOT_A_WORKSPACE" + assert body["detail"]["incident_id"] + assert "nothing-here" not in response.text + assert "nothing-here" in caplog.text