diff --git a/.env.example b/.env.example index 31145a0..4a8c591 100644 --- a/.env.example +++ b/.env.example @@ -3,6 +3,12 @@ # Copy this file to `.env`, or set the same variables in your process environment. # FORMS_DATABASE_URL is required. Everything below it is optional and shown with # its built-in default; uncomment the lines you want to change. +# +# Management API keys are not configured here and there is no variable for one. +# They live in the database so that creating and revoking a key needs no restart. +# Create the first one with: +# +# python -m hymical_forms.cli create-key --name local-admin # SQLAlchemy database URL. PostgreSQL is the intended production database. # Alembic reads this too, so `alembic upgrade head` needs nothing else set. diff --git a/README.md b/README.md index 98f0e90..bbb88b8 100644 --- a/README.md +++ b/README.md @@ -25,9 +25,10 @@ open-source. sent to them together with the durable obligation to deliver them, and runs a separate worker that performs the signed webhook delivery and retries it. -Endpoint and webhook configuration is completely unauthenticated: anyone who can -reach the API can create an endpoint pointing anywhere. There is no rate limiting -and no spam protection, so do not expose this to the public internet. +Endpoint configuration now requires a management API key. Form ingestion stays +public, because an ingestion URL is meant to sit in the `action` attribute of +somebody's HTML form. There is still no rate limiting and no spam protection, +so a public deployment is exposed to whatever volume the internet sends it. | Capability | Status | | ----------------------------- | ------------------------- | @@ -41,7 +42,7 @@ and no spam protection, so do not expose this to the public internet. | Durable delivery queue | Implemented | | Retries with backoff | Implemented | | Schema migrations | Implemented | -| API keys / authentication | **Not implemented** | +| API keys / authentication | Implemented | | Manual delivery replay | **Not implemented** | | Rate limiting, spam handling | **Not implemented** | | Export, retention, dashboards | **Not implemented** | @@ -81,13 +82,17 @@ See [`.env.example`](.env.example) for every setting and its default. ## Run -Hymical Forms is two processes sharing one database. Migrate it first, then -start them: +Hymical Forms is two processes sharing one database. Migrate it first, create a +management API key, then start them: ```bash alembic upgrade head ``` +```bash +python -m hymical_forms.cli create-key --name local-admin +``` + ```bash uvicorn hymical_forms.main:app --reload ``` @@ -186,6 +191,120 @@ build rather than surfacing in production. Interactive API documentation is served at `http://127.0.0.1:8000/docs`. +## Management API keys + +Administering the service needs a credential. Submitting a form does not. + +| Route | Credential | +| ---------------------- | -------------------------------- | +| `POST /endpoints` | Management API key required | +| `POST /f/{endpoint_id}`| **Public**, no credential | +| `GET /health` | **Public**, no credential | + +A management API key is an opaque bearer token that administers the whole +service. It is not a user account, not a login, and not scoped to a tenant: +there are no users, accounts, organizations, roles or permissions in this build, +and every valid key can do everything a management key can do. It is also +completely separate from a webhook signing secret. A `whsec_...` secret proves +to *your* server that a delivery came from Hymical Forms; a `hym_live_...` key +proves to Hymical Forms that a management request came from you. Neither can be +used in place of the other. + +### Creating the first key + +Keys are created against the database with the operator CLI, never over HTTP. +A route that issued a management credential without needing one would be an +unauthenticated way to gain full management access, which is the thing this +boundary exists to remove. Nothing is generated at startup, and no key ships +with this repository. + +```bash +python -m hymical_forms.cli create-key --name local-admin +``` + +``` +Created management API key mk_634c4efc22fb40fab4b19b82202b23bb (local-admin). + + hym_live_EXAMPLEONLYNOTAREALKEYREPLACETHISWITHYOURS + +Save this key now. It is shown here and nowhere else: the server stores +only a digest of it and cannot show it again. If you lose it, create a +replacement and revoke this one by its key ID. +``` + +> **Save the key now.** That line is the only time it is ever displayed. The +> server stores a SHA-256 digest of it and nothing else, so there is no command, +> route or database query that can show it to you again. Losing a key means +> creating another one and revoking the old one by its key ID, which `list-keys` +> can still tell you. + +The command reads `FORMS_DATABASE_URL`, the same setting everything else reads, +and refuses to run against a database that is not at the migration revision this +build expects. + +**A key is not configuration.** There is no `FORMS_API_KEY` variable, and there +should not be: keys live in the database so they can be created and revoked +without restarting anything, and so revoking one takes effect on the very next +request rather than on the next deploy. + +### Authenticating a management request + +```bash +curl -X POST http://127.0.0.1:8000/endpoints \ + -H 'Authorization: Bearer hym_live_REPLACE_WITH_YOUR_KEY' \ + -H 'Content-Type: application/json' \ + -d '{"id": "contact-form", "name": "Contact form"}' +``` + +The `Bearer` scheme is required. Credentials are never accepted in a query +parameter, where they would end up in access logs, browser history and +`Referer` headers. + +### Listing keys + +```bash +python -m hymical_forms.cli list-keys +``` + +``` +KEY ID NAME PREFIX CREATED LAST USED STATUS +mk_634c4efc22fb40fab4b19b82202b23bb local-admin hym_live_EXAMPLE1 2026-08-24T22:53:28+00:00 never active +``` + +Only what the database holds, which is no credential. `PREFIX` is the first few +characters of the key, kept so that you can tell two credentials apart in this +listing and match one against the key you saved. There is no HTTP route that +lists keys. + +### Revoking a key + +```bash +python -m hymical_forms.cli revoke-key mk_634c4efc22fb40fab4b19b82202b23bb +``` + +The key stops authenticating on the next request. Nothing caches a credential, +so revocation is immediate rather than eventual. The row is not deleted: it +keeps the key's identity, when it was created, when it was last used and when it +was withdrawn, which is what makes a revoked key still explainable later. +Revoking twice is accepted and reports the moment the key actually stopped +working. Rotation beyond create plus revoke is not implemented: to rotate, +create a new key, move your callers onto it, then revoke the old one. + +### Key format + +A key is `hym_live_` followed by 32 random bytes from the operating system, +rendered as unpadded base64url: 43 characters, 256 bits of entropy. The prefix +is there so an operator who finds the string in a config file knows immediately +whose credential it is and what to revoke. + +Only a hex SHA-256 digest of the whole key is stored, and lookup is by that +digest, so authentication is one indexed read. Password-style slow hashing is +deliberately not used: it exists because passwords are low-entropy and guessable, +and it would only add latency to every management request here. There is no +server-side pepper either, for the same reason: a pepper protects a digest that +is feasible to attack offline, which a 256-bit random secret is not, and it would +add a second secret whose loss would silently invalidate every key in the table. + ## API ### `GET /health` @@ -205,11 +324,14 @@ deciding whether to restart the process. Registers a form endpoint. Submissions are only accepted for endpoints that exist here. -**This route is unauthenticated.** Authentication is deliberately out of scope -for now, so keep the service on a private network. +**This route requires a management API key.** See +[Management API keys](#management-api-keys) for how to create one. A request +without a usable one is refused with `401 authentication_required`, and one +carrying a credential that does not authenticate with `401 invalid_api_key`. ```bash curl -X POST http://127.0.0.1:8000/endpoints \ + -H 'Authorization: Bearer hym_live_REPLACE_WITH_YOUR_KEY' \ -H 'Content-Type: application/json' \ -d '{"id": "contact-form", "name": "Contact form", "webhook_url": "https://example.com/hooks/forms"}' @@ -249,10 +371,19 @@ Reusing an ID returns `409 endpoint_already_exists`. There is no route to list, update or delete endpoints yet, so a webhook can only be configured at creation time. +The key that created an endpoint is not recorded on it. A management key +administers the service rather than owning a slice of it, and there is no +tenancy model for an owner column to belong to. + ### `POST /f/{endpoint_id}` Accepts a form submission for a registered endpoint and stores it. +**This route is public and stays public.** It is the URL that goes in the +`action` attribute of an HTML form, so it cannot require a header a browser form +has no way to send. No management credential is read here, and one sent anyway +is ignored rather than forwarded anywhere. + A submission to an ID that does not exist is rejected with `404 endpoint_not_found`, and one to an inactive endpoint with `409 endpoint_inactive`. Neither leaves anything in the database. @@ -331,9 +462,11 @@ as its submission is stored. accepts UUIDs, hex, base64 and base64url. Anything else is rejected with `400 invalid_idempotency_key`, including a header that is present but empty. -The 16-character floor exists because keys are endpoint-scoped and this API is -unauthenticated, so every client of an endpoint shares one key space. A short or -predictable key would collide with a stranger's submission. Use a random value. +The 16-character floor exists because keys are endpoint-scoped and form +ingestion is public, so every client of an endpoint shares one key space. A short +or predictable key would collide with a stranger's submission. Use a random +value. Management API keys do not change this: they authenticate the endpoint's +administrator, not the visitors submitting the form. ### Webhook delivery @@ -495,12 +628,19 @@ Do not enable it in production. ### Try it +```bash +python -m hymical_forms.cli create-key --name local-admin +``` + ```bash curl -X POST http://127.0.0.1:8000/endpoints \ + -H 'Authorization: Bearer hym_live_REPLACE_WITH_YOUR_KEY' \ -H 'Content-Type: application/json' \ -d '{"id": "contact-form", "name": "Contact form"}' ``` +Submitting needs no credential at all: + ```bash curl -i -X POST http://127.0.0.1:8000/f/contact-form -d email=dev@example.com -d message=hello ``` @@ -535,6 +675,8 @@ add. | ------ | -------------------------- | -------------------------------------------------------- | | 400 | `malformed_form_body` | Body does not parse as the declared content type | | 400 | `invalid_idempotency_key` | `Idempotency-Key` header breaks the key format rules | +| 401 | `authentication_required` | A management route was called without a bearer credential | +| 401 | `invalid_api_key` | The management API key is malformed, unknown or revoked | | 404 | `invalid_endpoint_id` | Submission path is not a well-formed endpoint ID | | 404 | `endpoint_not_found` | Endpoint ID is well formed but no such endpoint exists | | 404 | `not_found` | Unknown path | @@ -560,6 +702,13 @@ Ingestion rule codes are `too_many_fields`, `field_name_too_long`, from: `404` when it arrived as a submission path that addresses nothing, `422` when it arrived as a field in a request body. +Both `401`s carry `WWW-Authenticate: Bearer`. They are deliberately `401` and +not `403`: a `403` says the caller is known and not permitted, which needs a +permission model this build does not have. `invalid_api_key` is one answer for +malformed, unknown and revoked credentials alike, so that a guesser cannot sort +their attempts into "nearly right" and "wrong". The credential a request sent is +never echoed back in an error. + ## Configuration All settings are read from `FORMS_`-prefixed environment variables, or from a @@ -582,6 +731,11 @@ All settings are read from `FORMS_`-prefixed environment variables, or from a | `FORMS_WORKER_BATCH_SIZE` | `10` | Deliveries a worker claims at once | | `FORMS_WORKER_LEASE_SECONDS` | `60` | How long a worker's claim holds | +There is deliberately no setting for a management API key. Keys live in the +database so that creating and revoking one needs no restart, and so that a +revoked key stops working on the very next request. See +[Management API keys](#management-api-keys). + ## Development ```bash @@ -609,7 +763,11 @@ pytest tests/integration -m postgres ``` One of those tests asserts that the migrations and the models describe the same -schema, which is what keeps the fast suite's shortcut honest. +schema, which is what keeps the fast suite's shortcut honest. Others upgrade a +PostgreSQL database that already holds endpoints, submissions, deliveries and +attempts, and check that the data is exactly what it was afterwards, that the +downgrade removes only what the newer revision added, and that the data survives +that too. CI runs the lint, format and type checks once, the fast suite across Python 3.11 to 3.13, and the PostgreSQL suite once against a PostgreSQL 17 service. @@ -619,6 +777,8 @@ CI runs the lint, format and type checks once, the fast suite across Python ``` src/hymical_forms/ app.py application assembly and startup + apikeys.py management key rules: format, generation, digesting + cli.py the operator command line for management keys config.py typed settings db.py engine and session lifecycle errors.py the shared JSON error envelope @@ -632,15 +792,19 @@ src/hymical_forms/ schema.py the boundary between the application and Alembic main.py ASGI entrypoint api/ HTTP routes and response models + security.py the management authentication dependency migrations/ Alembic environment and revisions ``` -`ingestion.py` and `webhooks.py` hold the domain rules and know nothing about -HTTP or the database. `models.py` and `storage.py` are the only modules that -write queries, and `delivery.py` is the only one that makes an outbound request. -`api/` translates requests into domain rules and storage calls, and their -outcomes into responses. `worker.py` is a separate process and shares only the -database with the API. +`ingestion.py`, `webhooks.py` and `apikeys.py` hold the domain rules and know +nothing about HTTP or the database. `models.py` and `storage.py` are the only +modules that write queries, and `delivery.py` is the only one that makes an +outbound request. `api/` translates requests into domain rules and storage calls, +and their outcomes into responses. `api/security.py` holds the one authentication +dependency every management route declares, so the rules cannot drift apart route +by route. `worker.py` and `cli.py` are separate processes and share only the +database with the API; the worker does not authenticate through the HTTP API at +all. ### Storage notes @@ -688,9 +852,11 @@ an expired lease becomes reclaimable by exactly one worker. successfully and dies before recording it will have its lease expire, and the next worker will deliver the same event again. Deduplicate on the submission `id` in the signed payload. -- **Only one migration exists so far.** The upgrade path is real and tested, but - it has only ever been exercised from an empty database to the baseline. Nothing - has yet had to migrate data it cared about. +- **There is no user, account or role model.** A management key administers the + whole service. Keys cannot be scoped to an endpoint, a tenant or a permission, + and every valid key can do everything a management key can do. Separate keys + are useful for revoking one caller without disturbing another, and for nothing + else yet. - **A failed delivery is final and cannot be replayed.** Once a delivery reaches `failed`, nothing retries it and there is no manual replay route. - **The lease must outlast a delivery attempt.** A batch is delivered @@ -705,10 +871,19 @@ an expired lease becomes reclaimable by exactly one worker. and DNS rebinding is not addressed at all. Closing this properly means resolving at request time and pinning the connection to the validated address. Treat the current checks as a guardrail against mistakes, not a defence against - an attacker who can configure endpoints. -- **No authentication.** Anyone who can reach the API can create an endpoint, - point its webhook anywhere, and post to any active one. There is no rate - limiting or spam protection. + an attacker who can configure endpoints. Authentication narrows who that is to + whoever holds a management key; it does not make the checks complete. +- **Form ingestion is public and unrated.** Anyone who can reach the API can post + to any active endpoint. There is no rate limiting, no spam protection and no + CAPTCHA, so a public deployment accepts whatever volume it is sent. +- **A lost management key cannot be recovered,** only replaced. The server holds + a digest and nothing else. Create a new key, move your callers onto it, and + revoke the old one by the key ID `list-keys` still shows. +- **`last_used_at` is written on every authenticated management request.** That + is one extra small write per request, which management traffic is rare enough + to absorb and the ingestion path never pays because it is not authenticated. A + failure to write it is logged and ignored rather than allowed to turn a valid + credential into a `401`. - **A webhook can only be set when the endpoint is created.** There is no route to change a destination or rotate a signing secret. - **No API for delivery attempts.** They are recorded, but reading them means @@ -726,13 +901,16 @@ an expired lease becomes reclaimable by exactly one worker. - **`multipart/form-data` bodies are buffered in memory,** bounded by `FORMS_MAX_BODY_BYTES`. - A rejected submission reveals whether an endpoint ID exists, which allows - enumeration. This is unavoidable while the API is unauthenticated. + enumeration. This is unavoidable while form ingestion is public, and it stays + true now that endpoint creation is not. - **Idempotency keys never expire.** A key stays spent for as long as its submission is stored, so the table only grows. Expiry belongs with retention. - **Idempotency keys are shared across all clients of an endpoint,** because there is nothing to scope them to yet. Guessing another client's key returns that submission's ID and timestamp, though never its contents. Random keys of - the required length make this impractical, and API keys will close it properly. + the required length make this impractical. Management API keys do not close + this: they authenticate whoever administers the endpoint, not the visitors + submitting the form, and the submission route stays public by design. - **A replay is only recognised once the first attempt has committed.** A retry sent while the original is still in flight is treated as a concurrent request, which is safe, but a retry sent after the original *failed* is a new diff --git a/src/hymical_forms/api/endpoints.py b/src/hymical_forms/api/endpoints.py index eb78fdd..0b68798 100644 --- a/src/hymical_forms/api/endpoints.py +++ b/src/hymical_forms/api/endpoints.py @@ -4,6 +4,7 @@ from __future__ import annotations +import logging from datetime import datetime from http import HTTPStatus @@ -11,6 +12,7 @@ from pydantic import BaseModel, Field from hymical_forms import storage, webhooks +from hymical_forms.api.security import ManagementKeyDep from hymical_forms.config import Settings from hymical_forms.db import SessionDep from hymical_forms.errors import ApiError, ErrorResponse @@ -18,6 +20,8 @@ from hymical_forms.models import ENDPOINT_NAME_MAX_LENGTH from hymical_forms.webhooks import WEBHOOK_URL_MAX_LENGTH +logger = logging.getLogger(__name__) + router = APIRouter(tags=["endpoints"]) @@ -124,6 +128,7 @@ class EndpointResponse(BaseModel): status_code=HTTPStatus.CREATED, summary="Create a form endpoint", responses={ + 401: {"model": ErrorResponse, "description": "Missing or invalid management API key"}, 409: {"model": ErrorResponse, "description": "Endpoint ID already taken"}, 422: { "model": ErrorResponse, @@ -133,17 +138,26 @@ class EndpointResponse(BaseModel): }, ) def create_endpoint( - payload: CreateEndpointRequest, request: Request, session: SessionDep + payload: CreateEndpointRequest, + request: Request, + session: SessionDep, + principal: ManagementKeyDep, ) -> EndpointResponse: """ create an endpoint that submissions may then be addressed to :param payload: the endpoint identifier, label, active state and optional webhook :param request: the incoming request, read for the active configuration :param session: the session this request does its database work through + :param principal: the management key this request authenticated as :returns: the endpoint as persisted, including its signing secret if one was made """ # A plain ``def`` route, so FastAPI runs it in a worker thread and the # synchronous database calls never block the event loop. + # + # The authenticated key is not recorded on the endpoint. A management key + # administers the service rather than owning a slice of it, and an + # ``api_key_id`` column here would be the beginning of a tenancy model + # nothing in this build has a use for. if not is_valid_endpoint_id(payload.id): raise InvalidEndpointId() @@ -175,6 +189,17 @@ def create_endpoint( session.commit() + # Configuring a webhook destination is the most consequential thing this API + # does, so which credential did it is worth having in the log. Only the key's + # non-secret identity is available to log, which is the point of the + # principal carrying nothing else. + logger.info( + "endpoint %s created by management key %s (%s)", + endpoint.id, + principal.key_id, + principal.name, + ) + # The secret leaves the service exactly once, in this response. There is no # route that reads it back, so a caller that loses it has to make a new # endpoint rather than being handed the old secret again. diff --git a/src/hymical_forms/api/security.py b/src/hymical_forms/api/security.py new file mode 100644 index 0000000..ac814d1 --- /dev/null +++ b/src/hymical_forms/api/security.py @@ -0,0 +1,170 @@ +""" +the management authentication boundary + +One dependency resolves a management API key, and every route that administers +the service declares it. A future management route gets these rules by asking +for the same dependency rather than by reading the ``Authorization`` header +itself, which is what stops the rules from drifting apart route by route. + +Public routes deliberately do not appear here. Form ingestion and the health +check are reachable without a credential, because an ingestion URL is meant to +sit in the ``action`` attribute of somebody's HTML form. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from http import HTTPStatus +from typing import Annotated + +from fastapi import Depends +from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm import Session + +from hymical_forms import apikeys, storage +from hymical_forms.db import SessionDep +from hymical_forms.errors import ApiError +from hymical_forms.models import utcnow + +logger = logging.getLogger(__name__) + +# RFC 9110 says a 401 carries a challenge, and a client library that looks for +# one to decide how to authenticate should find it. +WWW_AUTHENTICATE = {"WWW-Authenticate": "Bearer"} + + +class AuthenticationRequired(ApiError): + """ + raised when a management request arrived without a usable bearer credential + """ + + status_code = HTTPStatus.UNAUTHORIZED + code = "authentication_required" + headers = WWW_AUTHENTICATE + + def __init__(self) -> None: + """ + state how a management request is meant to authenticate + """ + # Separate from ``invalid_api_key`` because the two are genuinely + # different problems for an integrator to fix, and because whether a + # request carried an Authorization header is something its sender + # already knows. Nothing is disclosed by saying so. + super().__init__( + f"This endpoint requires a management API key. {apikeys.MANAGEMENT_KEY_RULE}" + ) + + +class InvalidApiKey(ApiError): + """ + raised when a bearer credential was supplied but does not authenticate + """ + + # 401 rather than 403. A 403 says "I know who you are and you may not do + # this", which needs a permission model, and this build has none: every valid + # management key may do everything a management key can do. + status_code = HTTPStatus.UNAUTHORIZED + code = "invalid_api_key" + headers = WWW_AUTHENTICATE + + def __init__(self) -> None: + """ + refuse a credential without describing what was wrong with it + """ + # Malformed, unknown and revoked all arrive here and all leave with these + # exact words. Telling them apart would let somebody sort guesses into + # "nearly right" and "wrong", which is the signal enumeration runs on. + # The supplied credential is never echoed. + super().__init__("The management API key is not valid.") + + +@dataclass(frozen=True, slots=True) +class ManagementPrincipal: + """ + the identity a management request authenticated as + """ + + # Only non-secret identity crosses this boundary. Routes are handed no + # credential material, so no handler downstream is in a position to log it, + # store it, or forward it to somebody else's server. + key_id: str + name: str + display_prefix: str + + +_bearer = HTTPBearer( + scheme_name="ManagementApiKey", + description="A management API key, created with `python -m hymical_forms.cli create-key`.", + # Errors are raised by this module so that they reach the shared JSON + # envelope. Left on, the scheme would answer with FastAPI's own error body + # and a caller would meet two different error shapes from one API. + auto_error=False, +) + + +def require_management_key( + session: SessionDep, + credentials: Annotated[HTTPAuthorizationCredentials | None, Depends(_bearer)], +) -> ManagementPrincipal: + """ + resolve the management key a request authenticated with + :param session: the session this request does its database work through + :param credentials: the parsed bearer credential, or None if there was no usable one + :returns: the non-secret identity of the key that authenticated + :raises AuthenticationRequired: if no bearer credential was supplied + :raises InvalidApiKey: if the credential is malformed, unknown or revoked + """ + if credentials is None: + # No Authorization header, an unparseable one, a scheme other than + # Bearer, or Bearer with nothing after it. + raise AuthenticationRequired() + + supplied = credentials.credentials + if not apikeys.is_valid_management_key(supplied): + # A syntax gate in front of the database, so a sweep of unrelated tokens + # costs no query. It is not a security decision: a well-formed key that + # is not in the table is refused in exactly the same words. + raise InvalidApiKey() + + digest = apikeys.digest_key(supplied) + record = storage.find_management_key_by_digest(session, digest) + if record is None or not apikeys.digests_match(digest, record.key_digest): + raise InvalidApiKey() + if record.revoked_at is not None: + # Read from the row on every request. Nothing caches a key anywhere, so a + # revocation takes effect on the next request rather than whenever some + # cache would have expired. + raise InvalidApiKey() + + _record_use(session, record.id) + return ManagementPrincipal( + key_id=record.id, + name=record.name, + display_prefix=record.display_prefix, + ) + + +ManagementKeyDep = Annotated[ManagementPrincipal, Depends(require_management_key)] + + +def _record_use(session: Session, key_id: str) -> None: + """ + note that a key authenticated a request, without letting that failure matter + :param session: the session to write through + :param key_id: the key that authenticated + """ + # This is telemetry, not authentication, and it is written after the decision + # has already been made. One small write per authenticated management request + # is affordable because management traffic is rare by nature; it would not be + # if this ran on the ingestion path, which is exactly why it does not. + # + # The failure is swallowed on purpose. A database that cannot record a + # timestamp must not be able to turn a valid credential into a 401, and the + # rollback is what hands the route a usable session afterwards. + try: + storage.record_management_key_use(session, key_id, now=utcnow()) + except SQLAlchemyError: + session.rollback() + logger.warning("could not record last use of management key %s", key_id) diff --git a/src/hymical_forms/apikeys.py b/src/hymical_forms/apikeys.py new file mode 100644 index 0000000..58e354a --- /dev/null +++ b/src/hymical_forms/apikeys.py @@ -0,0 +1,152 @@ +""" +management API key rules: format, generation and digesting + +Nothing in this module performs I/O or knows about HTTP or the database. +Minting a credential is kept apart from storing one so that the format, the +entropy and the digest can be tested without a database, and so that the only +moment a full key exists in this process is the moment it was asked for. +""" + +from __future__ import annotations + +import hashlib +import hmac +import re +import secrets +import uuid +from dataclasses import dataclass + +# A recognisable prefix, so an operator who finds this string in a config file +# knows immediately whose credential it is and what to revoke. ``live`` leaves +# room for a differently scoped credential later without changing what this one +# means. +MANAGEMENT_KEY_PREFIX = "hym_live_" + +# 32 random bytes from the OS, rendered as unpadded base64url: 256 bits of +# entropy in 43 characters. Deliberately not a UUID4, which carries 122 bits and +# spends 6 of them on fixed version and variant nibbles. This credential sits on +# the public internet and can be guessed at indefinitely, so it is sized for +# that rather than for being a database identifier. +MANAGEMENT_KEY_SECRET_BYTES = 32 +MANAGEMENT_KEY_SECRET_LENGTH = 43 + +MANAGEMENT_KEY_LENGTH = len(MANAGEMENT_KEY_PREFIX) + MANAGEMENT_KEY_SECRET_LENGTH + +_SECRET_PATTERN = re.compile(rf"[A-Za-z0-9_-]{{{MANAGEMENT_KEY_SECRET_LENGTH}}}") + +MANAGEMENT_KEY_ID_PREFIX = "mk_" +MANAGEMENT_KEY_ID_MAX_LENGTH = len(MANAGEMENT_KEY_ID_PREFIX) + 32 + +MANAGEMENT_KEY_NAME_MAX_LENGTH = 200 + +# The prefix plus the first few characters of the secret, kept so that a key can +# be recognised in a listing without the listing holding the credential. Eight of +# forty-three base64url characters leaves 210 bits unknown, which is still far +# more than anything else in this service relies on. +DISPLAY_PREFIX_SECRET_CHARS = 8 +DISPLAY_PREFIX_LENGTH = len(MANAGEMENT_KEY_PREFIX) + DISPLAY_PREFIX_SECRET_CHARS + +# A SHA-256 digest rendered as hex. +KEY_DIGEST_LENGTH = 64 + +# Stated once so that every message mentioning the rule words it identically. +MANAGEMENT_KEY_RULE = ( + f"Send a management API key as 'Authorization: Bearer {MANAGEMENT_KEY_PREFIX}...'." +) + + +@dataclass(frozen=True, slots=True) +class GeneratedKey: + """ + a freshly minted management API key, in both of its forms + """ + + # ``key`` is the credential and the only copy of it that will ever exist: it + # is handed to the operator once and is not among the fields the database is + # given. The other three are what may safely be persisted. + id: str + key: str + display_prefix: str + digest: str + + +def new_management_key() -> GeneratedKey: + """ + mint a management API key and the safe representation of it + :returns: the full credential together with the identity and digest to store + """ + secret = secrets.token_urlsafe(MANAGEMENT_KEY_SECRET_BYTES) + key = f"{MANAGEMENT_KEY_PREFIX}{secret}" + return GeneratedKey( + id=new_management_key_id(), + key=key, + display_prefix=display_prefix(key), + digest=digest_key(key), + ) + + +def new_management_key_id() -> str: + """ + generate an opaque, non-secret identifier for a management key + :returns: a fresh key id such as ``mk_1f0c9a...`` + """ + # Separate from the credential on purpose. This is what an operator names in + # a revoke command and what a log line may safely carry, and knowing it tells + # nobody anything about the secret it belongs to. + return f"{MANAGEMENT_KEY_ID_PREFIX}{uuid.uuid4().hex}" + + +def digest_key(key: str) -> str: + """ + reduce a full management API key to the value that may be stored + :param key: the full credential as the client would send it + :returns: a hex SHA-256 digest of the credential + """ + # A single SHA-256, not a password hash. Password hashing is slow on purpose + # because passwords are low-entropy and guessable; this secret is 256 bits + # from the OS random source, so an attacker holding the digest has nothing to + # guess at and the cost would buy only latency on every management request. + # + # No pepper either. A pepper protects a digest that is feasible to attack + # offline, which this one is not, and it would introduce a second secret that + # has to be deployed, backed up and rotated, whose loss would silently + # invalidate every key in the table. + return hashlib.sha256(key.encode("utf-8")).hexdigest() + + +def digests_match(supplied: str, stored: str) -> bool: + """ + compare a computed digest against a stored one without leaking timing + :param supplied: digest of the credential the client sent + :param stored: digest read from the database + :returns: True if the two digests are identical + """ + # Candidates are found by digest, so the database has already compared these + # for us and this is belt and braces. It is here because it is the primitive + # a credential comparison should be written with: if lookup ever changes to + # resolve a candidate by its display prefix, this line is what keeps the + # comparison that decides the answer safe. + return hmac.compare_digest(supplied, stored) + + +def is_valid_management_key(key: str) -> bool: + """ + report whether a supplied credential is even shaped like one of ours + :param key: the bearer credential taken from the Authorization header + :returns: True if the credential has this service's prefix and secret shape + """ + # A cheap syntax gate in front of the database, so an internet-wide sweep of + # unrelated tokens costs no query. It is not a security check: a well-formed + # key that is not in the table is refused in exactly the same words. + if not key.startswith(MANAGEMENT_KEY_PREFIX): + return False + return _SECRET_PATTERN.fullmatch(key[len(MANAGEMENT_KEY_PREFIX) :]) is not None + + +def display_prefix(key: str) -> str: + """ + take the non-secret fragment of a key that identifies it in a listing + :param key: the full credential + :returns: the prefix plus the first few characters of the secret + """ + return key[:DISPLAY_PREFIX_LENGTH] diff --git a/src/hymical_forms/cli.py b/src/hymical_forms/cli.py new file mode 100644 index 0000000..9508fde --- /dev/null +++ b/src/hymical_forms/cli.py @@ -0,0 +1,296 @@ +""" +the operator command line for management API keys + +Run it as its own process, against the database ``FORMS_DATABASE_URL`` names:: + + python -m hymical_forms.cli create-key --name local-admin + python -m hymical_forms.cli list-keys + python -m hymical_forms.cli revoke-key mk_1f0c9a... + +Keys are minted here rather than over HTTP, and that is the whole answer to how +the first one comes into being. A route that issued a management credential +without needing one would be an unauthenticated way to gain full management +access, which is the thing this boundary exists to remove. Nothing is generated +at startup and no key is shipped in this repository, so a deployment has exactly +the credentials an operator deliberately created. +""" + +from __future__ import annotations + +import argparse +import sys +from collections.abc import Sequence +from datetime import datetime +from typing import TextIO + +from pydantic import ValidationError +from sqlalchemy.engine import make_url +from sqlalchemy.exc import ArgumentError, SQLAlchemyError +from sqlalchemy.orm import Session + +from hymical_forms import apikeys, storage +from hymical_forms.config import Settings +from hymical_forms.db import create_engine_from_url, create_session_factory +from hymical_forms.models import ManagementApiKey, utcnow +from hymical_forms.schema import SchemaNotReady, verify_schema + +PROGRAM = "python -m hymical_forms.cli" + +EXIT_OK = 0 +EXIT_FAILED = 1 + + +def main(argv: Sequence[str] | None = None) -> int: + """ + run one operator command + :param argv: command line arguments, or None to read them from the process + :returns: the process exit code + """ + parser = build_parser() + arguments = parser.parse_args(argv) + + try: + settings = Settings() + except ValidationError: + # The message is written here rather than relayed, because pydantic's own + # is a validation report about a settings model the operator has never + # seen and does not need to learn about. + print( + "FORMS_DATABASE_URL is not set. Export it, or put it in a .env file " + "in this directory, and run the command again.", + file=sys.stderr, + ) + return EXIT_FAILED + + try: + return _run(arguments, settings, out=sys.stdout) + except SchemaNotReady as exc: + print(f"The database schema is not ready: {exc}", file=sys.stderr) + return EXIT_FAILED + except SQLAlchemyError as exc: + # Driver text is useful to whoever is debugging a connection, but it can + # repeat back the URL it was handed, so the password is taken out of it + # first. No API key can appear here: none is ever passed to the database. + print( + f"The database could not be used: {_redact(str(exc), settings.database_url)}", + file=sys.stderr, + ) + return EXIT_FAILED + + +def build_parser() -> argparse.ArgumentParser: + """ + build the argument parser for the operator commands + :returns: a parser covering every subcommand + """ + parser = argparse.ArgumentParser( + prog=PROGRAM, + description="Create, list and revoke Hymical Forms management API keys.", + ) + subcommands = parser.add_subparsers(dest="command", required=True) + + create = subcommands.add_parser( + "create-key", + help="Create a management API key and print it once.", + ) + create.add_argument( + "--name", + required=True, + help="Human-readable label, so this key can be recognised later.", + ) + + subcommands.add_parser( + "list-keys", + help="List management keys and their state. Never shows a credential.", + ) + + revoke = subcommands.add_parser( + "revoke-key", + help="Revoke a management API key by its key ID.", + ) + revoke.add_argument("key_id", help="The key ID, as shown by list-keys.") + + return parser + + +def _run(arguments: argparse.Namespace, settings: Settings, *, out: TextIO) -> int: + """ + open the database, check the schema, and dispatch to the chosen command + :param arguments: the parsed command line + :param settings: the configuration naming the database to work against + :param out: the stream ordinary output is written to + :returns: the process exit code + """ + engine = create_engine_from_url(settings.database_url) + try: + # The same check the API and the worker make on startup. Writing a key + # into a database at an older revision would either fail on a missing + # table or, worse, appear to work against a schema this build does not + # understand. + verify_schema(engine) + with create_session_factory(engine)() as session: + return _dispatch(arguments, session, out=out) + finally: + engine.dispose() + + +def _dispatch(arguments: argparse.Namespace, session: Session, *, out: TextIO) -> int: + """ + run the chosen command against an open session + :param arguments: the parsed command line + :param session: the session to do the work through + :param out: the stream ordinary output is written to + :returns: the process exit code + """ + if arguments.command == "create-key": + return _create_key(session, name=arguments.name, out=out) + if arguments.command == "list-keys": + return _list_keys(session, out=out) + return _revoke_key(session, key_id=arguments.key_id, out=out) + + +def _create_key(session: Session, *, name: str, out: TextIO) -> int: + """ + mint a management key, store its safe representation, and show it once + :param session: the session to write through + :param name: human-readable label for the key + :param out: the stream ordinary output is written to + :returns: the process exit code + """ + generated = apikeys.new_management_key() + storage.create_management_key( + session, + key_id=generated.id, + name=name, + display_prefix=generated.display_prefix, + key_digest=generated.digest, + now=utcnow(), + ) + session.commit() + + # Printed after the commit, so a key that could not be stored is never + # presented as one that works. The credential goes to stdout and nowhere + # else: it is not logged, and nothing writes it to disk. + print(f"Created management API key {generated.id} ({name}).", file=out) + print(file=out) + print(f" {generated.key}", file=out) + print(file=out) + print( + "Save this key now. It is shown here and nowhere else: the server stores\n" + "only a digest of it and cannot show it again. If you lose it, create a\n" + "replacement and revoke this one by its key ID.", + file=out, + ) + return EXIT_OK + + +def _list_keys(session: Session, *, out: TextIO) -> int: + """ + show every management key without showing any credential + :param session: the session to query through + :param out: the stream ordinary output is written to + :returns: the process exit code + """ + keys = storage.list_management_keys(session) + if not keys: + print("No management API keys exist. Create one with 'create-key --name ...'.", file=out) + return EXIT_OK + + # Only columns the database actually holds, and the database holds no + # credential, so there is nothing here that could reconstruct one. + print(_row("KEY ID", "NAME", "PREFIX", "CREATED", "LAST USED", "STATUS"), file=out) + for key in keys: + print( + _row( + key.id, + key.name, + key.display_prefix, + _moment(key.created_at), + _moment(key.last_used_at), + "active" if key.is_active else f"revoked {_moment(key.revoked_at)}", + ), + file=out, + ) + return EXIT_OK + + +def _revoke_key(session: Session, *, key_id: str, out: TextIO) -> int: + """ + withdraw a management key so that it stops authenticating + :param session: the session to write through + :param key_id: the identifier of the key to revoke + :param out: the stream ordinary output is written to + :returns: the process exit code + """ + already_revoked = _revocation_moment(session, key_id) + key = storage.revoke_management_key(session, key_id, now=utcnow()) + if key is None: + print(f"No management API key with the ID {key_id!r} exists.", file=sys.stderr) + return EXIT_FAILED + + if already_revoked is not None: + # Idempotent on purpose, and it reports the moment the credential + # actually stopped working rather than the moment of this second attempt. + print( + f"Management API key {key.id} ({key.name}) was already revoked at " + f"{_moment(already_revoked)}.", + file=out, + ) + return EXIT_OK + + print( + f"Revoked management API key {key.id} ({key.name}). It will not authenticate " + "another request.", + file=out, + ) + return EXIT_OK + + +def _revocation_moment(session: Session, key_id: str) -> datetime | None: + """ + read when a key was revoked, before this command possibly revokes it + :param session: the session to query through + :param key_id: the identifier of the key to inspect + :returns: when it was revoked, or None if it exists and is active or does not exist + """ + key: ManagementApiKey | None = storage.get_management_key(session, key_id) + return key.revoked_at if key is not None else None + + +def _row(*columns: str) -> str: + """ + lay one listing row out in fixed-width columns + :param columns: the cell values, in listing order + :returns: the rendered line + """ + widths = (35, 24, 17, 26, 26, 0) + return " ".join(value.ljust(width) for value, width in zip(columns, widths, strict=True)) + + +def _moment(value: datetime | None) -> str: + """ + render a timestamp for a listing + :param value: the instant to render, or None if there is not one + :returns: an ISO 8601 timestamp, or ``never`` when there is none + """ + return value.isoformat(timespec="seconds") if value is not None else "never" + + +def _redact(text: str, database_url: str) -> str: + """ + hide the database password if a driver message repeated it back + :param text: the message about to be shown to the operator + :param database_url: the URL this invocation was configured with + :returns: the message with any occurrence of the password replaced + """ + try: + password = make_url(database_url).password + except ArgumentError: + # The URL did not parse, which is very likely what the message is about. + # Nothing can be located in it to redact, so nothing is shown. + return "the configured FORMS_DATABASE_URL is not a usable SQLAlchemy URL" + return text.replace(password, "***") if password else text + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/src/hymical_forms/errors.py b/src/hymical_forms/errors.py index e077747..2f9f548 100644 --- a/src/hymical_forms/errors.py +++ b/src/hymical_forms/errors.py @@ -54,11 +54,15 @@ class ApiError(Exception): an error that maps directly onto the public error envelope """ - # Subclasses fix ``status_code`` and ``code``; instances supply the message - # and any structured details. + # Subclasses fix ``status_code``, ``code`` and any headers the status + # requires; instances supply the message and any structured details. status_code: ClassVar[int] = 500 code: ClassVar[str] = "internal_error" + # Only for headers a status code is defined in terms of, such as the + # ``WWW-Authenticate`` an RFC 9110 401 is not a complete response without. + headers: ClassVar[dict[str, str] | None] = None + def __init__(self, message: str, *, details: dict[str, Any] | None = None) -> None: """ record the message and context for an error response @@ -79,6 +83,7 @@ def as_response(self) -> JSONResponse: code=self.code, message=self.message, details=self.details, + headers=self.headers, ) @@ -88,6 +93,7 @@ def error_response( code: str, message: str, details: dict[str, Any] | None = None, + headers: dict[str, str] | None = None, ) -> JSONResponse: """ build a JSON response in the standard error envelope @@ -95,10 +101,15 @@ def error_response( :param code: stable, machine-readable error identifier :param message: human-readable explanation of the failure :param details: optional structured context, omitted from the body when absent + :param headers: optional response headers the status code requires :returns: a JSONResponse carrying the envelope """ payload = ErrorResponse(error=ErrorDetail(code=code, message=message, details=details)) - return JSONResponse(status_code=status_code, content=payload.model_dump(exclude_none=True)) + return JSONResponse( + status_code=status_code, + content=payload.model_dump(exclude_none=True), + headers=headers, + ) def register_exception_handlers(app: FastAPI) -> None: diff --git a/src/hymical_forms/migrations/versions/0002_20260824_management_api_keys.py b/src/hymical_forms/migrations/versions/0002_20260824_management_api_keys.py new file mode 100644 index 0000000..6b0a672 --- /dev/null +++ b/src/hymical_forms/migrations/versions/0002_20260824_management_api_keys.py @@ -0,0 +1,60 @@ +""" +management api keys + +Adds the credential table the management authentication boundary resolves keys +against. Nothing the baseline created is touched, so a database already holding +endpoints, submissions, deliveries and attempts gains a table and loses nothing, +and the downgrade removes exactly that table again. + +The credential itself has no column here. Only a digest of it is stored, so a +copy of this table is not a set of working keys. + +Timestamps are written as ``sa.DateTime(timezone=True)`` rather than the +application's ``UtcDateTime`` decorator, for the reason given in ``0001``: a +migration is a frozen record of a change, not a view of the current models. + +revision: 0002 +revises: 0001 +created: 2026-08-24 +""" + +from __future__ import annotations + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "0002" +down_revision: str | None = "0001" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + """ + create the management api key table + """ + op.create_table( + "management_api_keys", + sa.Column("id", sa.String(length=35), nullable=False), + sa.Column("name", sa.String(length=200), nullable=False), + sa.Column("display_prefix", sa.String(length=17), nullable=False), + sa.Column("key_digest", sa.String(length=64), nullable=False), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), + sa.Column("revoked_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("last_used_at", sa.DateTime(timezone=True), nullable=True), + sa.PrimaryKeyConstraint("id", name=op.f("pk_management_api_keys")), + # Authentication resolves a candidate by digest, so this is both the + # uniqueness guarantee and the index every management request rides on. + sa.UniqueConstraint("key_digest", name="uq_management_api_keys_key_digest"), + ) + + +def downgrade() -> None: + """ + remove the schema this revision created + """ + # Nothing else references this table, so dropping it leaves everything the + # baseline created exactly as it was. + op.drop_table("management_api_keys") diff --git a/src/hymical_forms/models.py b/src/hymical_forms/models.py index 06f8a3c..2880904 100644 --- a/src/hymical_forms/models.py +++ b/src/hymical_forms/models.py @@ -1,5 +1,6 @@ """ -the persisted schema: endpoints and the submissions addressed to them +the persisted schema: endpoints, the submissions addressed to them, and the +management credentials that administer the service """ from __future__ import annotations @@ -19,6 +20,12 @@ from sqlalchemy.engine import Dialect from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column +from hymical_forms.apikeys import ( + DISPLAY_PREFIX_LENGTH, + KEY_DIGEST_LENGTH, + MANAGEMENT_KEY_ID_MAX_LENGTH, + MANAGEMENT_KEY_NAME_MAX_LENGTH, +) from hymical_forms.ingestion import ( ENDPOINT_ID_MAX_LENGTH, IDEMPOTENCY_KEY_MAX_LENGTH, @@ -324,3 +331,53 @@ class DeliveryAttempt(Base): # Response bodies are deliberately not stored. They are unbounded, written by # somebody else's server, and nothing in this build reads them back. error: Mapped[str | None] = mapped_column(String(DELIVERY_ERROR_MAX_LENGTH), default=None) + + +class ManagementApiKey(Base): + """ + a credential that authenticates management requests against this service + """ + + # These keys administer the whole service. There is no account, tenant or + # role model to attach them to, and inventing owner columns for one would be + # guessing at a design nothing has asked for yet. Endpoints deliberately do + # not record which key created them for the same reason. + __tablename__ = "management_api_keys" + + __table_args__ = ( + # Authentication resolves a candidate by digest, so this constraint is + # both the uniqueness guarantee and the index that lookup rides on. + UniqueConstraint("key_digest", name="uq_management_api_keys_key_digest"), + ) + + id: Mapped[str] = mapped_column(String(MANAGEMENT_KEY_ID_MAX_LENGTH), primary_key=True) + name: Mapped[str] = mapped_column(String(MANAGEMENT_KEY_NAME_MAX_LENGTH)) + + # The prefix and the first few characters of the secret, so an operator can + # tell two credentials apart in a listing. Everything after it is missing, + # which is what makes the fragment safe to display and to log. + display_prefix: Mapped[str] = mapped_column(String(DISPLAY_PREFIX_LENGTH)) + + # The only representation of the credential this service keeps. The key + # itself is never written anywhere: whoever creates one either saves it at + # that moment or makes another. + key_digest: Mapped[str] = mapped_column(String(KEY_DIGEST_LENGTH)) + + created_at: Mapped[datetime] = mapped_column(UtcDateTime, default=utcnow) + + # Revocation sets a timestamp rather than deleting the row, so the identity + # and the history of a withdrawn credential survive being withdrawn. + revoked_at: Mapped[datetime | None] = mapped_column(UtcDateTime, default=None) + + # Best-effort telemetry, written after a request has already authenticated. + # Nothing decides anything on it, which is what lets a failure to update it + # be ignored rather than turning a valid key into a rejected one. + last_used_at: Mapped[datetime | None] = mapped_column(UtcDateTime, default=None) + + @property + def is_active(self) -> bool: + """ + report whether this key still authenticates + :returns: True if the key has not been revoked + """ + return self.revoked_at is None diff --git a/src/hymical_forms/storage.py b/src/hymical_forms/storage.py index 9b54fe5..0cc57a8 100644 --- a/src/hymical_forms/storage.py +++ b/src/hymical_forms/storage.py @@ -3,12 +3,14 @@ Most functions here leave the commit to the caller, so a request handler decides when its work becomes durable and a failure anywhere before that commit leaves -the database untouched. Three functions own their transaction and say so: +the database untouched. A few own their transaction and say so: :func:`store_submission`, because it writes a submission and the obligation to deliver it as one atomic unit and must roll both back together to settle an idempotency race; :func:`claim_due_deliveries`, because a claim is only worth -anything once it is committed; and :func:`complete_attempt`, because the audit -record and the state it justifies have to land together. +anything once it is committed; :func:`complete_attempt`, because the audit +record and the state it justifies have to land together; and the two management +key writes, :func:`revoke_management_key` and :func:`record_management_key_use`, +because each is the whole of what its caller came to do. """ from __future__ import annotations @@ -379,6 +381,118 @@ def complete_attempt( return attempt +def create_management_key( + session: Session, + *, + key_id: str, + name: str, + display_prefix: str, + key_digest: str, + now: datetime, +) -> models.ManagementApiKey: + """ + add a management API key, storing only its safe representation + :param session: the session to add the key through + :param key_id: the non-secret identifier the key will be administered by + :param name: human-readable label for the key + :param display_prefix: the non-secret fragment shown in a listing + :param key_digest: digest of the credential, which is never stored itself + :param now: the instant the key was created + :returns: the pending key, not yet committed + """ + # The credential is not a parameter here, and that is the point: this + # function could not persist it even if a caller wanted it to. + key = models.ManagementApiKey( + id=key_id, + name=name, + display_prefix=display_prefix, + key_digest=key_digest, + created_at=now, + ) + session.add(key) + session.flush() + return key + + +def find_management_key_by_digest( + session: Session, key_digest: str +) -> models.ManagementApiKey | None: + """ + look up the key a supplied credential digests to + :param session: the session to query through + :param key_digest: digest of the credential the client sent + :returns: the key, or None if no key digests to that value + """ + # Revoked keys are returned too. Whether a key still authenticates is the + # caller's decision to make and to answer for, and filtering here would hide + # the distinction from the one place that has to be explicit about it. + return session.scalars( + select(models.ManagementApiKey).where(models.ManagementApiKey.key_digest == key_digest) + ).one_or_none() + + +def get_management_key(session: Session, key_id: str) -> models.ManagementApiKey | None: + """ + look a management key up by its non-secret identifier + :param session: the session to query through + :param key_id: the identifier to resolve + :returns: the key, or None if no key holds that identifier + """ + return session.get(models.ManagementApiKey, key_id) + + +def list_management_keys(session: Session) -> list[models.ManagementApiKey]: + """ + read every management key, newest first + :param session: the session to query through + :returns: the keys, carrying no credential material + """ + return list( + session.scalars( + select(models.ManagementApiKey).order_by(models.ManagementApiKey.created_at.desc()) + ) + ) + + +def revoke_management_key( + session: Session, key_id: str, *, now: datetime +) -> models.ManagementApiKey | None: + """ + withdraw a management key without discarding what it was + :param session: the session to write through + :param key_id: the identifier of the key to revoke + :param now: the instant the revocation takes effect + :returns: the key, revoked and committed, or None if no such key exists + """ + key = session.get(models.ManagementApiKey, key_id) + if key is None: + return None + # Idempotent, and the first revocation is the one that counts: revoking twice + # must not quietly move the moment the credential stopped being valid. + if key.revoked_at is None: + key.revoked_at = now + session.commit() + return key + + +def record_management_key_use(session: Session, key_id: str, *, now: datetime) -> None: + """ + note that a key authenticated a request + :param session: the session to write through + :param key_id: the key that authenticated + :param now: the instant the request was authenticated + """ + # An UPDATE rather than a load-and-set, so this costs one statement and does + # not put an object in the caller's session that a later rollback would + # expire underneath them. + session.execute( + update(models.ManagementApiKey) + .where(models.ManagementApiKey.id == key_id) + .values(last_used_at=now) + ) + session.commit() + + def _settle(existing: models.Submission, payload_fingerprint: str | None) -> Submission: """ decide whether an earlier submission is a replay of this one or a clash diff --git a/tests/conftest.py b/tests/conftest.py index c4f9d33..7e1fd11 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,6 +5,12 @@ values that are cheap to exercise, and so that a developer's local environment can never change a test's outcome. Each application gets its own in-memory SQLite database, which starts empty and disappears when the test ends. + +Each application also gets a management API key, written straight into its +database the way the operator CLI writes one. Clients send it by default so that +the tests which are about ingestion stay about ingestion; a client built with +``authenticate=False`` genuinely sends no ``Authorization`` header, which is what +the public-route tests need. """ from __future__ import annotations @@ -22,15 +28,20 @@ from pydantic_settings import SettingsConfigDict from sqlalchemy.orm import Session, sessionmaker +from hymical_forms import apikeys, storage from hymical_forms.app import create_app from hymical_forms.config import Settings from hymical_forms.delivery import create_webhook_client +from hymical_forms.models import utcnow from hymical_forms.schema import create_all from hymical_forms.worker import process_batch from webhook_server import WebhookRecorder URLENCODED_HEADERS = {"content-type": "application/x-www-form-urlencoded"} +AUTHORIZATION_HEADER = "Authorization" +DEFAULT_KEY_NAME = "test-operator" + DEFAULT_ENDPOINT_ID = "contact-form" DEFAULT_ENDPOINT_NAME = "Contact form" @@ -76,24 +87,69 @@ def create_endpoint( name: str = DEFAULT_ENDPOINT_NAME, is_active: bool = True, webhook_url: str | None = None, + api_key: str | None = None, ) -> dict[str, Any]: """ - register an endpoint through the public API, failing loudly if it does not take + register an endpoint through the management API, failing loudly if it does not take :param client: the client whose application should hold the endpoint :param endpoint_id: the public identifier to register :param name: human-readable label for the endpoint :param is_active: whether the endpoint should accept submissions :param webhook_url: destination to deliver submissions to, or None for no webhook + :param api_key: management key to authenticate with, or None to use the client's own :returns: the created endpoint as the API returned it """ body: dict[str, Any] = {"id": endpoint_id, "name": name, "is_active": is_active} if webhook_url is not None: body["webhook_url"] = webhook_url - response = client.post("/endpoints", json=body) + headers = bearer(api_key) if api_key is not None else None + response = client.post("/endpoints", json=body, headers=headers) assert response.status_code == 201, response.text return cast(dict[str, Any], response.json()) +def bearer(api_key: str) -> dict[str, str]: + """ + build the authorization header a management request carries + :param api_key: the full management key + :returns: headers authenticating as that key + """ + return {AUTHORIZATION_HEADER: f"Bearer {api_key}"} + + +def issue_management_key(client: TestClient, *, name: str = DEFAULT_KEY_NAME) -> str: + """ + mint a management key into the client's database, the way the operator CLI does + :param client: the client whose application database should hold the key + :param name: human-readable label for the key + :returns: the full credential, which exists only here and in the caller + """ + # Written through the same domain and storage functions the CLI uses, rather + # than through a fixture-only shortcut, so what the tests authenticate with + # is what an operator would actually be holding. + generated = apikeys.new_management_key() + with open_session(client) as session: + storage.create_management_key( + session, + key_id=generated.id, + name=name, + display_prefix=generated.display_prefix, + key_digest=generated.digest, + now=utcnow(), + ) + session.commit() + return generated.key + + +def management_key(client: TestClient) -> str: + """ + read the management key an authenticated client sends + :param client: a client built with authentication enabled + :returns: the full credential the fixture issued for it + """ + return client.headers[AUTHORIZATION_HEADER].removeprefix("Bearer ") + + def app_settings(client: TestClient) -> Settings: """ read the settings the client's application was built with @@ -155,10 +211,13 @@ def make_client() -> Iterator[ClientFactory]: """ with ExitStack() as stack: - def factory(*, seed_endpoint: bool = True, **overrides: Any) -> TestClient: + def factory( + *, seed_endpoint: bool = True, authenticate: bool = True, **overrides: Any + ) -> TestClient: """ build a client for an app configured with the given overrides :param seed_endpoint: whether to register the default endpoint first + :param authenticate: whether the client should send its management key by default :param overrides: setting values to replace the built-in defaults :returns: a test client closed when the fixture tears down """ @@ -170,8 +229,15 @@ def factory(*, seed_endpoint: bool = True, **overrides: Any) -> TestClient: app = create_app(build_settings(**overrides)) create_all(app.state.engine) client = stack.enter_context(TestClient(app)) + + # A key always exists, so seeding an endpoint works either way. Only + # whether the client sends it by default depends on ``authenticate``, + # which is what lets a test prove a route is reachable without one. + key = issue_management_key(client) + if authenticate: + client.headers[AUTHORIZATION_HEADER] = f"Bearer {key}" if seed_endpoint: - create_endpoint(client) + create_endpoint(client, api_key=key) return client yield factory diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index e97682a..5bd2d31 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -22,7 +22,12 @@ from hymical_forms.db import create_engine_from_url from hymical_forms.models import Base from hymical_forms.schema import alembic_config -from integration.support import POSTGRES_URL_VARIABLE, IsolatedSettings, drop_everything +from integration.support import ( + POSTGRES_URL_VARIABLE, + IsolatedSettings, + drop_everything, + seed_management_key, +) def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: @@ -90,15 +95,23 @@ def sessions(migrated_engine: Engine) -> sessionmaker[Session]: @pytest.fixture -def pg_client(postgres_url: str, migrated_engine: Engine) -> Iterator[TestClient]: +def pg_client( + postgres_url: str, migrated_engine: Engine, sessions: sessionmaker[Session] +) -> Iterator[TestClient]: """ provide an API client backed by the migrated PostgreSQL database :param postgres_url: the database URL to run against :param migrated_engine: unused, but forces the schema to exist first + :param sessions: session factory used to mint the client's management key :returns: an iterator yielding a test client """ app = create_app( IsolatedSettings(database_url=postgres_url, allow_private_webhook_targets=True) ) + # Issued after ``clean_database`` has truncated everything, so the key exists + # for exactly the test that is about to run. + with sessions() as session: + key = seed_management_key(session) with TestClient(app) as client: + client.headers["Authorization"] = f"Bearer {key}" yield client diff --git a/tests/integration/support.py b/tests/integration/support.py index 4d6a0aa..98487bc 100644 --- a/tests/integration/support.py +++ b/tests/integration/support.py @@ -19,9 +19,10 @@ from sqlalchemy.engine import make_url from sqlalchemy.orm import Session -from hymical_forms import models +from hymical_forms import apikeys, models, storage from hymical_forms.config import Settings from hymical_forms.db import create_engine_from_url +from hymical_forms.models import utcnow from hymical_forms.webhooks import DeliveryState POSTGRES_URL_VARIABLE = "HYMICAL_TEST_POSTGRES_URL" @@ -73,6 +74,26 @@ def drop_everything(engine: Engine) -> None: connection.execute(text("CREATE SCHEMA public")) +def seed_management_key(session: Session, name: str = "integration-tests") -> str: + """ + mint a management key into the database, the way the operator CLI does + :param session: the session to insert through + :param name: human-readable label for the key + :returns: the full credential, which exists only here and in the caller + """ + generated = apikeys.new_management_key() + storage.create_management_key( + session, + key_id=generated.id, + name=name, + display_prefix=generated.display_prefix, + key_digest=generated.digest, + now=utcnow(), + ) + session.commit() + return generated.key + + def seed_endpoint(session: Session, endpoint_id: str = "contact-form") -> models.Endpoint: """ insert an endpoint with a webhook configured diff --git a/tests/integration/test_migrations_postgres.py b/tests/integration/test_migrations_postgres.py index 7b65e42..d34abb9 100644 --- a/tests/integration/test_migrations_postgres.py +++ b/tests/integration/test_migrations_postgres.py @@ -8,17 +8,31 @@ from __future__ import annotations +from collections.abc import Iterator +from contextlib import contextmanager +from datetime import UTC, datetime + from alembic import command from alembic.autogenerate import compare_metadata +from alembic.config import Config from alembic.migration import MigrationContext -from sqlalchemy import inspect, text +from sqlalchemy import Connection, Engine, inspect, text from hymical_forms.db import create_engine_from_url from hymical_forms.models import Base from hymical_forms.schema import alembic_config, current_revision, head_revision from integration.support import temporary_database -EXPECTED_TABLES = {"endpoints", "submissions", "webhook_deliveries", "delivery_attempts"} +BASELINE_TABLES = {"endpoints", "submissions", "webhook_deliveries", "delivery_attempts"} +EXPECTED_TABLES = BASELINE_TABLES | {"management_api_keys"} + +# Representative interval 6 data: an endpoint with a webhook, a submission sent +# with an idempotency key, the delivery it owes, and one recorded attempt. +SEEDED_AT = datetime(2026, 8, 24, 12, 0, tzinfo=UTC) +SEEDED_ENDPOINT = "contact-form" +SEEDED_SUBMISSION = "sub_11111111111111111111111111111111" +SEEDED_DELIVERY = "whd_22222222222222222222222222222222" +SEEDED_ATTEMPT = "att_33333333333333333333333333333333" def test_an_empty_database_upgrades_to_head(postgres_url: str) -> None: @@ -77,6 +91,7 @@ def test_the_migration_creates_the_constraints_the_application_relies_on( assert { "uq_submissions_endpoint_idempotency_key", "uq_webhook_deliveries_submission", + "uq_management_api_keys_key_digest", "ck_endpoints_webhook_configuration", "ck_submissions_idempotency_identity", "ck_webhook_deliveries_completion", @@ -136,3 +151,202 @@ def test_the_migration_round_trips(postgres_url: str) -> None: assert difference == [] finally: engine.dispose() + + +# --- upgrading a database that already holds data ---------------------------- +# +# Interval 6 built the migration machinery but only ever ran it against an empty +# database. These tests are the first time an upgrade has had to preserve data it +# cares about, which is the property an operator is actually relying on. + + +def test_upgrading_a_populated_baseline_preserves_its_data(postgres_url: str) -> None: + """ + an existing endpoint, submission, delivery and attempt must survive 0001 to 0002 + :param postgres_url: a URL on the PostgreSQL server to work against + """ + with _database_at_baseline(postgres_url) as (config, engine): + with engine.begin() as connection: + _seed_baseline_data(connection) + + command.upgrade(config, "0002") + + assert current_revision(engine) == "0002" + with engine.connect() as connection: + _assert_baseline_data_intact(connection) + + +def test_upgrading_a_populated_baseline_adds_the_key_table(postgres_url: str) -> None: + with _database_at_baseline(postgres_url) as (config, engine): + with engine.begin() as connection: + _seed_baseline_data(connection) + assert "management_api_keys" not in set(inspect(engine).get_table_names()) + + command.upgrade(config, "0002") + + assert "management_api_keys" in set(inspect(engine).get_table_names()) + with engine.connect() as connection: + assert connection.scalar(text("select count(*) from management_api_keys")) == 0 + + +def test_downgrading_from_0002_leaves_the_baseline_data_alone(postgres_url: str) -> None: + """ + the downgrade must remove only what 0002 added + :param postgres_url: a URL on the PostgreSQL server to work against + """ + with _database_at_baseline(postgres_url) as (config, engine): + with engine.begin() as connection: + _seed_baseline_data(connection) + command.upgrade(config, "0002") + with engine.begin() as connection: + connection.execute( + text( + "insert into management_api_keys " + "(id, name, display_prefix, key_digest, created_at) " + "values ('mk_test', 'operator', 'hym_live_abcdefgh', :digest, :now)" + ), + {"digest": "d" * 64, "now": SEEDED_AT}, + ) + + command.downgrade(config, "0001") + + assert current_revision(engine) == "0001" + assert "management_api_keys" not in set(inspect(engine).get_table_names()) + with engine.connect() as connection: + _assert_baseline_data_intact(connection) + + +def test_a_populated_database_upgraded_again_still_matches_the_models(postgres_url: str) -> None: + """ + the whole round trip on real data must end with zero migration and model drift + :param postgres_url: a URL on the PostgreSQL server to work against + """ + with _database_at_baseline(postgres_url) as (config, engine): + with engine.begin() as connection: + _seed_baseline_data(connection) + + command.upgrade(config, "0002") + command.downgrade(config, "0001") + command.upgrade(config, "head") + + assert current_revision(engine) == head_revision() + with engine.connect() as connection: + _assert_baseline_data_intact(connection) + difference = compare_metadata(MigrationContext.configure(connection), Base.metadata) + assert difference == [], f"migrated schema differs from the models: {difference}" + + +@contextmanager +def _database_at_baseline(postgres_url: str) -> Iterator[tuple[Config, Engine]]: + """ + provide a throwaway database migrated to 0001 and nothing further + :param postgres_url: a URL on the PostgreSQL server to work against + :returns: a context manager yielding the alembic config and an engine on it + """ + with temporary_database(postgres_url) as url: + config = alembic_config(url) + command.upgrade(config, "0001") + engine = create_engine_from_url(url) + try: + yield config, engine + finally: + engine.dispose() + + +def _seed_baseline_data(connection: Connection) -> None: + """ + insert one of each interval 6 row, as an operator's database would hold + :param connection: the connection to insert through + """ + # Written as SQL rather than through the ORM on purpose. The models describe + # head, and this data is meant to be what a database at 0001 already contains. + connection.execute( + text( + "insert into endpoints (id, name, is_active, created_at, webhook_url, webhook_secret) " + "values (:id, 'Contact form', true, :now, 'https://example.invalid/hook', :secret)" + ), + {"id": SEEDED_ENDPOINT, "now": SEEDED_AT, "secret": "whsec_" + "a" * 64}, + ) + connection.execute( + text( + "insert into submissions " + "(id, endpoint_id, received_at, fields, idempotency_key, payload_fingerprint) " + "values (:id, :endpoint, :now, :fields, :key, :fingerprint)" + ), + { + "id": SEEDED_SUBMISSION, + "endpoint": SEEDED_ENDPOINT, + "now": SEEDED_AT, + "fields": '{"email": ["dev@example.com"]}', + "key": "b8f1c2d4e5a67890b8f1c2d4e5a67890", + "fingerprint": "f" * 64, + }, + ) + connection.execute( + text( + "insert into webhook_deliveries " + "(id, submission_id, destination_url, signing_secret, state, attempts, " + "next_attempt_at, created_at, completed_at) " + "values (:id, :submission, 'https://example.invalid/hook', :secret, 'delivered', 1, " + ":now, :now, :now)" + ), + { + "id": SEEDED_DELIVERY, + "submission": SEEDED_SUBMISSION, + "now": SEEDED_AT, + "secret": "whsec_" + "a" * 64, + }, + ) + connection.execute( + text( + "insert into delivery_attempts " + "(id, delivery_id, submission_id, attempt_number, destination_url, attempted_at, " + "outcome, response_status) " + "values (:id, :delivery, :submission, 1, 'https://example.invalid/hook', :now, " + "'succeeded', 200)" + ), + { + "id": SEEDED_ATTEMPT, + "delivery": SEEDED_DELIVERY, + "submission": SEEDED_SUBMISSION, + "now": SEEDED_AT, + }, + ) + + +def _assert_baseline_data_intact(connection: Connection) -> None: + """ + check that every seeded row is still there and still says what it said + :param connection: the connection to query through + """ + endpoint = connection.execute( + text("select name, webhook_url, webhook_secret from endpoints where id = :id"), + {"id": SEEDED_ENDPOINT}, + ).one() + assert endpoint.name == "Contact form" + assert endpoint.webhook_url == "https://example.invalid/hook" + assert endpoint.webhook_secret == "whsec_" + "a" * 64 + + submission = connection.execute( + text("select endpoint_id, fields, idempotency_key from submissions where id = :id"), + {"id": SEEDED_SUBMISSION}, + ).one() + assert submission.endpoint_id == SEEDED_ENDPOINT + assert submission.fields == {"email": ["dev@example.com"]} + assert submission.idempotency_key == "b8f1c2d4e5a67890b8f1c2d4e5a67890" + + delivery = connection.execute( + text("select submission_id, state, attempts from webhook_deliveries where id = :id"), + {"id": SEEDED_DELIVERY}, + ).one() + assert delivery.submission_id == SEEDED_SUBMISSION + assert delivery.state == "delivered" + assert delivery.attempts == 1 + + attempt = connection.execute( + text("select delivery_id, outcome, response_status from delivery_attempts where id = :id"), + {"id": SEEDED_ATTEMPT}, + ).one() + assert attempt.delivery_id == SEEDED_DELIVERY + assert attempt.outcome == "succeeded" + assert attempt.response_status == 200 diff --git a/tests/test_api_keys.py b/tests/test_api_keys.py new file mode 100644 index 0000000..f8a8610 --- /dev/null +++ b/tests/test_api_keys.py @@ -0,0 +1,168 @@ +""" +management API key generation, digesting and storage +""" + +from __future__ import annotations + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import select + +from conftest import DEFAULT_KEY_NAME, issue_management_key, management_key, open_session +from hymical_forms import apikeys, models, storage +from hymical_forms.models import utcnow + +# --- format and entropy ------------------------------------------------------ + + +def test_a_generated_key_is_recognisably_ours(empty_client: TestClient) -> None: + key = issue_management_key(empty_client) + + assert key.startswith(apikeys.MANAGEMENT_KEY_PREFIX) + assert len(key) == apikeys.MANAGEMENT_KEY_LENGTH + assert apikeys.is_valid_management_key(key) + + +def test_two_generated_keys_differ() -> None: + """ + the secret has to come from a random source, not from anything derivable + """ + # Twenty is plenty to catch a constant or a counter, and asserts nothing about + # any particular random value. + keys = {apikeys.new_management_key().key for _ in range(20)} + ids = {apikeys.new_management_key().id for _ in range(20)} + + assert len(keys) == 20 + assert len(ids) == 20 + + +def test_the_key_id_is_not_derived_from_the_secret() -> None: + generated = apikeys.new_management_key() + + assert generated.id.startswith(apikeys.MANAGEMENT_KEY_ID_PREFIX) + assert generated.id not in generated.key + assert generated.key.removeprefix(apikeys.MANAGEMENT_KEY_PREFIX) not in generated.id + + +def test_the_display_prefix_reveals_only_its_first_characters() -> None: + generated = apikeys.new_management_key() + + assert generated.key.startswith(generated.display_prefix) + assert len(generated.display_prefix) == apikeys.DISPLAY_PREFIX_LENGTH + assert len(generated.display_prefix) < len(generated.key) + + +def test_the_digest_is_a_hex_sha256_of_the_whole_key() -> None: + generated = apikeys.new_management_key() + + assert len(generated.digest) == apikeys.KEY_DIGEST_LENGTH + assert generated.digest == apikeys.digest_key(generated.key) + assert generated.digest != apikeys.digest_key(generated.key + "x") + + +@pytest.mark.parametrize( + ("description", "candidate"), + [ + ("empty", ""), + ("the prefix alone", apikeys.MANAGEMENT_KEY_PREFIX), + ("no prefix", "a" * apikeys.MANAGEMENT_KEY_SECRET_LENGTH), + ("the wrong prefix", "hym_test_" + "a" * apikeys.MANAGEMENT_KEY_SECRET_LENGTH), + ("too short", apikeys.MANAGEMENT_KEY_PREFIX + "a" * 10), + ("too long", apikeys.MANAGEMENT_KEY_PREFIX + "a" * 100), + ("a character outside base64url", apikeys.MANAGEMENT_KEY_PREFIX + "a!" + "a" * 41), + ("a uuid", "hym_live_f81d4fae-7dec-11d0-a765-00a0c91e6bf6"), + ], +) +def test_a_malformed_key_is_not_even_well_formed(description: str, candidate: str) -> None: + assert not apikeys.is_valid_management_key(candidate), description + + +# --- what the database is allowed to hold ------------------------------------ + + +def test_the_stored_row_holds_no_credential(empty_client: TestClient) -> None: + """ + the whole point of the digest is that the table is not a set of working keys + :param empty_client: test client whose app holds no endpoints + """ + key = management_key(empty_client) + secret = key.removeprefix(apikeys.MANAGEMENT_KEY_PREFIX) + + with open_session(empty_client) as session: + rows = list(session.scalars(select(models.ManagementApiKey))) + + assert len(rows) == 1 + stored = rows[0] + values = [str(getattr(stored, column.name)) for column in models.ManagementApiKey.__table__.c] + assert key not in values + assert not any(secret in value for value in values) + assert stored.key_digest == apikeys.digest_key(key) + + +def test_the_stored_digest_authenticates_the_generated_key(empty_client: TestClient) -> None: + key = issue_management_key(empty_client) + + with open_session(empty_client) as session: + found = storage.find_management_key_by_digest(session, apikeys.digest_key(key)) + + assert found is not None + assert found.is_active + + +def test_another_key_does_not_resolve_to_a_stored_one(empty_client: TestClient) -> None: + issue_management_key(empty_client) + other = apikeys.new_management_key() + + with open_session(empty_client) as session: + assert storage.find_management_key_by_digest(session, other.digest) is None + + +def test_revoking_records_the_moment_without_deleting_the_row(empty_client: TestClient) -> None: + key = issue_management_key(empty_client) + + with open_session(empty_client) as session: + stored = storage.find_management_key_by_digest(session, apikeys.digest_key(key)) + assert stored is not None + revoked = storage.revoke_management_key(session, stored.id, now=utcnow()) + + assert revoked is not None + assert revoked.revoked_at is not None + assert not revoked.is_active + + with open_session(empty_client) as session: + assert storage.get_management_key(session, revoked.id) is not None + + +def test_revoking_twice_keeps_the_first_moment(empty_client: TestClient) -> None: + """ + revocation is idempotent, and must not move when the credential stopped working + :param empty_client: test client whose app holds no endpoints + """ + key = issue_management_key(empty_client) + + with open_session(empty_client) as session: + stored = storage.find_management_key_by_digest(session, apikeys.digest_key(key)) + assert stored is not None + first = storage.revoke_management_key(session, stored.id, now=utcnow()) + assert first is not None + moment = first.revoked_at + + second = storage.revoke_management_key(session, stored.id, now=utcnow()) + + assert second is not None + assert second.revoked_at == moment + + +def test_revoking_an_unknown_key_reports_it(empty_client: TestClient) -> None: + with open_session(empty_client) as session: + assert storage.revoke_management_key(session, "mk_nope", now=utcnow()) is None + + +def test_listing_returns_every_key(empty_client: TestClient) -> None: + issue_management_key(empty_client, name="second-operator") + + with open_session(empty_client) as session: + names = [key.name for key in storage.list_management_keys(session)] + + # The fixture's own key, plus the one this test added. + assert set(names) == {DEFAULT_KEY_NAME, "second-operator"} diff --git a/tests/test_cli.py b/tests/test_cli.py new file mode 100644 index 0000000..fa17ef4 --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,326 @@ +""" +the operator command line: creating, listing and revoking management keys + +Each test runs against a SQLite file rather than the in-memory database the API +tests use, because the CLI is a separate process in real life and opens its own +connection to whatever ``FORMS_DATABASE_URL`` names. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import select + +from conftest import IsolatedSettings, bearer, build_settings +from hymical_forms import apikeys, cli, models +from hymical_forms.app import create_app +from hymical_forms.db import create_engine_from_url, create_session_factory +from hymical_forms.schema import create_all + +CREATE_BODY = {"id": "contact-form", "name": "Contact form"} + + +@pytest.fixture +def database(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[str]: + """ + provide a migrated SQLite file the CLI will find through the environment + :param tmp_path: pytest fixture giving this test a directory of its own + :param monkeypatch: pytest fixture used to point the CLI at that database + :returns: an iterator yielding the database URL + """ + url = f"sqlite:///{tmp_path / 'forms.db'}" + engine = create_engine_from_url(url) + create_all(engine) + engine.dispose() + + monkeypatch.setenv("FORMS_DATABASE_URL", url) + # The CLI builds Settings itself, which would otherwise read a .env file the + # developer running the suite happens to have. This is the same isolation the + # API tests get from building their settings explicitly. + monkeypatch.setattr(cli, "Settings", IsolatedSettings) + yield url + + +@pytest.fixture +def unmigrated_database(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[str]: + """ + provide a database URL naming a file that holds no schema at all + :param tmp_path: pytest fixture giving this test a directory of its own + :param monkeypatch: pytest fixture used to point the CLI at that database + :returns: an iterator yielding the database URL + """ + url = f"sqlite:///{tmp_path / 'empty.db'}" + monkeypatch.setenv("FORMS_DATABASE_URL", url) + monkeypatch.setattr(cli, "Settings", IsolatedSettings) + yield url + + +def stored_keys(url: str) -> list[models.ManagementApiKey]: + """ + read every management key row straight out of the database + :param url: the database to read from + :returns: the stored keys + """ + engine = create_engine_from_url(url) + try: + with create_session_factory(engine)() as session: + return list(session.scalars(select(models.ManagementApiKey))) + finally: + engine.dispose() + + +def created_key(output: str) -> str: + """ + pick the credential out of what create-key printed + :param output: everything the command wrote to stdout + :returns: the full key + """ + keys = [word for word in output.split() if word.startswith(apikeys.MANAGEMENT_KEY_PREFIX)] + assert len(keys) == 1, f"expected exactly one credential in the output, found {len(keys)}" + return keys[0] + + +# --- create-key -------------------------------------------------------------- + + +def test_create_key_prints_the_credential_once( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + assert cli.main(["create-key", "--name", "local-admin"]) == 0 + + output = capsys.readouterr().out + key = created_key(output) + assert apikeys.is_valid_management_key(key) + assert output.count(key) == 1 + + +def test_create_key_tells_the_operator_to_save_it( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + cli.main(["create-key", "--name", "local-admin"]) + + output = capsys.readouterr().out + assert "Save this key now" in output + assert "local-admin" in output + + +def test_create_key_stores_no_plaintext_credential( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + """ + a copy of the table must not be a set of working credentials + :param database: a migrated database the CLI is pointed at + :param capsys: pytest fixture capturing what the command printed + """ + cli.main(["create-key", "--name", "local-admin"]) + key = created_key(capsys.readouterr().out) + secret = key.removeprefix(apikeys.MANAGEMENT_KEY_PREFIX) + + rows = stored_keys(database) + + assert len(rows) == 1 + values = [str(getattr(rows[0], column.name)) for column in models.ManagementApiKey.__table__.c] + assert not any(secret in value for value in values) + assert rows[0].key_digest == apikeys.digest_key(key) + assert rows[0].name == "local-admin" + + +def test_a_key_created_by_the_cli_authenticates( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + """ + the CLI and the HTTP boundary have to agree on what a credential is + :param database: a migrated database the CLI is pointed at + :param capsys: pytest fixture capturing what the command printed + """ + cli.main(["create-key", "--name", "local-admin"]) + key = created_key(capsys.readouterr().out) + + app = create_app(build_settings(database_url=database)) + with TestClient(app) as client: + response = client.post("/endpoints", json=CREATE_BODY, headers=bearer(key)) + + assert response.status_code == 201 + + +def test_two_created_keys_differ(database: str, capsys: pytest.CaptureFixture[str]) -> None: + cli.main(["create-key", "--name", "first"]) + first = created_key(capsys.readouterr().out) + cli.main(["create-key", "--name", "second"]) + second = created_key(capsys.readouterr().out) + + assert first != second + assert len(stored_keys(database)) == 2 + + +# --- list-keys --------------------------------------------------------------- + + +def test_list_keys_says_so_when_there_are_none( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + assert cli.main(["list-keys"]) == 0 + + assert "No management API keys exist" in capsys.readouterr().out + + +def test_list_keys_shows_safe_metadata_only( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + cli.main(["create-key", "--name", "local-admin"]) + key = created_key(capsys.readouterr().out) + + assert cli.main(["list-keys"]) == 0 + + output = capsys.readouterr().out + assert key not in output + assert key.removeprefix(apikeys.MANAGEMENT_KEY_PREFIX) not in output + assert stored_keys(database)[0].id in output + assert "local-admin" in output + assert apikeys.display_prefix(key) in output + assert "active" in output + assert "never" in output + + +def test_list_keys_reports_a_revoked_key_as_revoked( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + cli.main(["create-key", "--name", "local-admin"]) + capsys.readouterr() + key_id = stored_keys(database)[0].id + cli.main(["revoke-key", key_id]) + capsys.readouterr() + + cli.main(["list-keys"]) + + assert "revoked" in capsys.readouterr().out + + +# --- revoke-key -------------------------------------------------------------- + + +def test_revoke_key_withdraws_the_credential( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + cli.main(["create-key", "--name", "local-admin"]) + key = created_key(capsys.readouterr().out) + key_id = stored_keys(database)[0].id + + assert cli.main(["revoke-key", key_id]) == 0 + + output = capsys.readouterr().out + assert "Revoked management API key" in output + assert key not in output + assert stored_keys(database)[0].revoked_at is not None + + +def test_a_revoked_key_no_longer_authenticates( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + cli.main(["create-key", "--name", "local-admin"]) + key = created_key(capsys.readouterr().out) + cli.main(["revoke-key", stored_keys(database)[0].id]) + + app = create_app(build_settings(database_url=database)) + with TestClient(app) as client: + response = client.post("/endpoints", json=CREATE_BODY, headers=bearer(key)) + + assert response.status_code == 401 + assert response.json()["error"]["code"] == "invalid_api_key" + + +def test_revoking_twice_is_accepted_and_says_so( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + cli.main(["create-key", "--name", "local-admin"]) + capsys.readouterr() + key_id = stored_keys(database)[0].id + cli.main(["revoke-key", key_id]) + first = stored_keys(database)[0].revoked_at + + assert cli.main(["revoke-key", key_id]) == 0 + + assert "already revoked" in capsys.readouterr().out + assert stored_keys(database)[0].revoked_at == first + + +def test_revoking_an_unknown_key_fails_cleanly( + database: str, capsys: pytest.CaptureFixture[str] +) -> None: + assert cli.main(["revoke-key", "mk_does_not_exist"]) == 1 + + captured = capsys.readouterr() + assert "No management API key" in captured.err + assert captured.out == "" + + +# --- operator errors --------------------------------------------------------- + + +def test_an_unmigrated_database_produces_a_useful_error( + unmigrated_database: str, capsys: pytest.CaptureFixture[str] +) -> None: + """ + the CLI makes the same schema check the API and the worker make on startup + :param unmigrated_database: a database URL naming a file with no schema + :param capsys: pytest fixture capturing what the command printed + """ + assert cli.main(["create-key", "--name", "local-admin"]) == 1 + + captured = capsys.readouterr() + assert "schema is not ready" in captured.err + assert "alembic upgrade head" in captured.err + assert captured.out == "" + + +def test_a_missing_database_url_produces_a_useful_error( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + monkeypatch.setattr(cli, "Settings", IsolatedSettings) + + assert cli.main(["create-key", "--name", "local-admin"]) == 1 + + assert "FORMS_DATABASE_URL is not set" in capsys.readouterr().err + + +def test_a_database_that_cannot_be_opened_produces_a_useful_error( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + monkeypatch.setenv("FORMS_DATABASE_URL", f"sqlite:///{tmp_path / 'missing' / 'forms.db'}") + monkeypatch.setattr(cli, "Settings", IsolatedSettings) + + assert cli.main(["create-key", "--name", "local-admin"]) == 1 + + captured = capsys.readouterr() + assert "The database could not be used" in captured.err + assert captured.out == "" + + +def test_a_driver_message_never_carries_the_database_password() -> None: + """ + some driver errors repeat the URL back, and the password must not ride along + """ + url = "postgresql+psycopg://forms:sup3rs3cret@localhost:5432/forms" + + redacted = cli._redact(f"could not connect using {url}", url) + + assert "sup3rs3cret" not in redacted + assert "***" in redacted + + +def test_an_unparseable_database_url_is_not_repeated_back() -> None: + redacted = cli._redact("Could not parse SQLAlchemy URL from 'sup3rs3cret'", "sup3rs3cret") + + assert "sup3rs3cret" not in redacted + + +def test_a_failing_command_creates_no_key( + unmigrated_database: str, capsys: pytest.CaptureFixture[str] +) -> None: + cli.main(["create-key", "--name", "local-admin"]) + + assert apikeys.MANAGEMENT_KEY_PREFIX not in capsys.readouterr().out diff --git a/tests/test_management_auth.py b/tests/test_management_auth.py new file mode 100644 index 0000000..89a11a5 --- /dev/null +++ b/tests/test_management_auth.py @@ -0,0 +1,324 @@ +""" +the management authentication boundary on ``POST /endpoints``, and what stays public +""" + +from __future__ import annotations + +import logging + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import select + +from conftest import ( + ClientFactory, + bearer, + create_endpoint, + issue_management_key, + management_key, + open_session, + work_once, +) +from hymical_forms import apikeys, models, storage +from hymical_forms.models import utcnow +from webhook_server import WebhookRecorder + +CREATE_BODY = {"id": "contact-form", "name": "Contact form"} + +# --- what authenticates, and what does not ----------------------------------- + + +def test_a_valid_key_creates_an_endpoint(empty_client: TestClient) -> None: + response = empty_client.post("/endpoints", json=CREATE_BODY) + + assert response.status_code == 201 + assert response.json()["id"] == "contact-form" + + +def test_creating_an_endpoint_without_credentials_is_refused(make_client: ClientFactory) -> None: + client = make_client(seed_endpoint=False, authenticate=False) + + response = client.post("/endpoints", json=CREATE_BODY) + + assert response.status_code == 401 + assert response.json()["error"]["code"] == "authentication_required" + + +def test_a_refused_request_creates_nothing(make_client: ClientFactory) -> None: + client = make_client(seed_endpoint=False, authenticate=False) + + client.post("/endpoints", json=CREATE_BODY) + + with open_session(client) as session: + assert list(session.scalars(select(models.Endpoint))) == [] + + +@pytest.mark.parametrize( + ("description", "value"), + [ + ("empty", ""), + ("the scheme alone", "Bearer"), + ("the scheme with nothing after it", "Bearer "), + ("no scheme at all", "hym_live_abcdefgh"), + ("basic auth", "Basic aHltOmxpdmU="), + ("a different scheme", "Token hym_live_abcdefgh"), + ("nonsense", "!!!"), + ], +) +def test_an_unusable_authorization_header_is_refused( + make_client: ClientFactory, description: str, value: str +) -> None: + client = make_client(seed_endpoint=False, authenticate=False) + + response = client.post("/endpoints", json=CREATE_BODY, headers={"Authorization": value}) + + assert response.status_code == 401, description + # No bearer credential arrived, so the caller is told how to send one rather + # than that a key they never supplied was checked and refused. + assert response.json()["error"]["code"] == "authentication_required", description + + +@pytest.mark.parametrize( + ("description", "key"), + [ + ("malformed", "not-a-hymical-key"), + ("the right prefix but too short", apikeys.MANAGEMENT_KEY_PREFIX + "abc"), + ("well formed but unknown", apikeys.new_management_key().key), + ], +) +def test_a_bearer_credential_that_does_not_authenticate_is_refused( + make_client: ClientFactory, description: str, key: str +) -> None: + client = make_client(seed_endpoint=False, authenticate=False) + + response = client.post("/endpoints", json=CREATE_BODY, headers=bearer(key)) + + assert response.status_code == 401, description + # Malformed, unknown and revoked deliberately answer identically, so a + # guesser learns nothing about which of their guesses was closer. + assert response.json()["error"]["code"] == "invalid_api_key", description + + +def test_a_revoked_key_stops_authenticating_immediately(empty_client: TestClient) -> None: + """ + revocation must take effect on the next request, with nothing caching the key + :param empty_client: test client whose app holds no endpoints + """ + key = management_key(empty_client) + assert empty_client.post("/endpoints", json=CREATE_BODY).status_code == 201 + + with open_session(empty_client) as session: + stored = storage.find_management_key_by_digest(session, apikeys.digest_key(key)) + assert stored is not None + storage.revoke_management_key(session, stored.id, now=utcnow()) + + response = empty_client.post("/endpoints", json={"id": "waitlist", "name": "Waitlist"}) + + assert response.status_code == 401 + assert response.json()["error"]["code"] == "invalid_api_key" + + +def test_one_application_accepts_more_than_one_key(empty_client: TestClient) -> None: + second = issue_management_key(empty_client, name="second-operator") + + response = empty_client.post("/endpoints", json=CREATE_BODY, headers=bearer(second)) + + assert response.status_code == 201 + + +def test_revoking_one_key_leaves_another_working(empty_client: TestClient) -> None: + first = management_key(empty_client) + second = issue_management_key(empty_client, name="second-operator") + + with open_session(empty_client) as session: + stored = storage.find_management_key_by_digest(session, apikeys.digest_key(first)) + assert stored is not None + storage.revoke_management_key(session, stored.id, now=utcnow()) + + response = empty_client.post("/endpoints", json=CREATE_BODY, headers=bearer(second)) + + assert response.status_code == 201 + + +# --- the shape of a refusal -------------------------------------------------- + + +@pytest.mark.parametrize( + ("description", "headers"), + [ + ("no credentials", {}), + ("an invalid key", {"Authorization": "Bearer " + apikeys.new_management_key().key}), + ], +) +def test_a_refusal_carries_the_bearer_challenge( + make_client: ClientFactory, description: str, headers: dict[str, str] +) -> None: + client = make_client(seed_endpoint=False, authenticate=False) + + response = client.post("/endpoints", json=CREATE_BODY, headers=headers) + + assert response.headers["www-authenticate"] == "Bearer", description + + +def test_a_refusal_uses_the_shared_error_envelope(make_client: ClientFactory) -> None: + client = make_client(seed_endpoint=False, authenticate=False) + + response = client.post("/endpoints", json=CREATE_BODY) + + error = response.json()["error"] + assert set(error) <= {"code", "message", "details"} + assert isinstance(error["code"], str) + assert isinstance(error["message"], str) + + +def test_a_refusal_never_echoes_the_supplied_credential(make_client: ClientFactory) -> None: + """ + a credential reflected into an error body would end up in somebody's log + :param make_client: factory for clients bound to a configured app + """ + client = make_client(seed_endpoint=False, authenticate=False) + key = apikeys.new_management_key().key + + response = client.post("/endpoints", json=CREATE_BODY, headers=bearer(key)) + + assert key not in response.text + assert key.removeprefix(apikeys.MANAGEMENT_KEY_PREFIX) not in response.text + + +def test_a_successful_request_does_not_log_the_credential( + empty_client: TestClient, caplog: pytest.LogCaptureFixture +) -> None: + key = management_key(empty_client) + + with caplog.at_level(logging.DEBUG): + empty_client.post("/endpoints", json=CREATE_BODY) + + assert caplog.text != "" + assert key not in caplog.text + assert key.removeprefix(apikeys.MANAGEMENT_KEY_PREFIX) not in caplog.text + + +# --- authentication does not change what endpoint creation does -------------- + + +def test_a_duplicate_endpoint_still_conflicts_when_authenticated( + empty_client: TestClient, +) -> None: + create_endpoint(empty_client, "contact-form") + + response = empty_client.post("/endpoints", json={"id": "contact-form", "name": "Another"}) + + assert response.status_code == 409 + assert response.json()["error"]["code"] == "endpoint_already_exists" + + +def test_the_endpoint_records_nothing_about_the_key_that_made_it( + empty_client: TestClient, +) -> None: + """ + a management key administers the service, it does not own what it configures + :param empty_client: test client whose app holds no endpoints + """ + create_endpoint(empty_client, "contact-form") + + columns = {column.name for column in models.Endpoint.__table__.c} + + assert "api_key_id" not in columns + assert not any("key" in name for name in columns - {"webhook_secret"}) + + +def test_authenticating_records_when_the_key_was_last_used(empty_client: TestClient) -> None: + key = management_key(empty_client) + + with open_session(empty_client) as session: + stored = storage.find_management_key_by_digest(session, apikeys.digest_key(key)) + assert stored is not None + assert stored.last_used_at is None + + empty_client.post("/endpoints", json=CREATE_BODY) + + with open_session(empty_client) as session: + stored = storage.find_management_key_by_digest(session, apikeys.digest_key(key)) + assert stored is not None + assert stored.last_used_at is not None + + +# --- routes that must stay public -------------------------------------------- + + +def test_health_needs_no_credentials(make_client: ClientFactory) -> None: + client = make_client(seed_endpoint=False, authenticate=False) + + response = client.get("/health") + + assert response.status_code == 200 + assert "authorization" not in client.headers + assert response.json()["status"] == "ok" + + +def test_form_ingestion_needs_no_credentials(make_client: ClientFactory) -> None: + """ + the ingestion URL sits in somebody's HTML form, so it cannot require a header + :param make_client: factory for clients bound to a configured app + """ + client = make_client(authenticate=False) + + response = client.post("/f/contact-form", data={"email": "dev@example.com"}) + + assert "authorization" not in client.headers + assert response.status_code == 202 + assert response.json()["endpoint_id"] == "contact-form" + + +def test_an_unknown_endpoint_still_answers_404_without_credentials( + make_client: ClientFactory, +) -> None: + # A 401 here would tell a browser form to ask for credentials it can never + # have, so the public path has to keep answering the way it always did. + client = make_client(seed_endpoint=False, authenticate=False) + + response = client.post("/f/nothing-here", data={"email": "dev@example.com"}) + + assert response.status_code == 404 + assert response.json()["error"]["code"] == "endpoint_not_found" + + +def test_a_public_submission_still_queues_a_delivery( + make_client: ClientFactory, webhook: WebhookRecorder +) -> None: + client = make_client( + seed_endpoint=False, authenticate=False, allow_private_webhook_targets=True + ) + key = issue_management_key(client, name="setup") + create_endpoint(client, "contact-form", webhook_url=webhook.url, api_key=key) + + accepted = client.post("/f/contact-form", data={"email": "dev@example.com"}) + + assert accepted.status_code == 202 + assert accepted.json()["delivery"]["queued"] is True + assert work_once(client) == 1 + assert len(webhook.received) == 1 + + +def test_a_management_authorization_header_never_reaches_a_webhook( + make_client: ClientFactory, webhook: WebhookRecorder +) -> None: + """ + a credential forwarded to a destination would hand it full management access + :param make_client: factory for clients bound to a configured app + :param webhook: a local server recording the deliveries it is sent + """ + client = make_client(seed_endpoint=False, allow_private_webhook_targets=True) + key = management_key(client) + create_endpoint(client, "contact-form", webhook_url=webhook.url) + + # Submitted with the management credential attached, which the public route + # has no use for. It must go no further than this process. + client.post("/f/contact-form", data={"email": "dev@example.com"}, headers=bearer(key)) + work_once(client) + + assert len(webhook.received) == 1 + delivered = webhook.received[0] + assert "authorization" not in delivered.headers + assert key not in str(delivered.headers) + assert key.encode() not in delivered.body