From e0f7191bb56f704aa23d67952fe096e631d82292 Mon Sep 17 00:00:00 2001 From: TheMeinerLP Date: Sun, 23 Aug 2026 15:52:56 +0200 Subject: [PATCH] feat(api): let a guild bring its own sign-in `GET /api/auth/login` takes no parameters and reads no cookie, so it cannot choose a guild's OAuth client from an identity it does not have yet. `?guild={slug}` puts the guild in the URL, the state row carries it across the round trip, and the callback reads it back -- so the state, and nothing a caller sends, selects the client for the code exchange. `ConsoleAuth` stops holding one `OAuthClient` and holds a `SignInClients` instead, asked once at each end of the round trip. A sign-in with no guild is unchanged and resolves the environment-configured client, which a test pins. Administrators of a guild register, re-register and remove its client through five routes, and supply its secret through a request of its own. The secret is wrapped to the guild and to its purpose, and there is nowhere in the read model to put it: `GET` answers `has_secret` and never a value. Every refusal on the sign-in path is one 404, so an unknown slug and a half-configured one stay indistinguishable. The Discord account-link flow stays on the environment-configured client: `api` holds the master key and `link` does not, and the chart refuses to render one onto it. --- docs/operations.md | 149 +++++++ src/sturnus/console/adapters.py | 264 +++++++++++- src/sturnus/console/app.py | 48 ++- src/sturnus/console/auth.py | 110 ++++- src/sturnus/console/ports.py | 144 ++++++- src/sturnus/console/routes_oauth.py | 401 +++++++++++++++++ src/sturnus/domain/oauth_clients.py | 144 +++++++ src/sturnus/entrypoints/api.py | 25 +- src/sturnus/observability/events.py | 18 + tests/console/conftest.py | 179 +++++++- tests/console/test_adapters.py | 376 +++++++++++++++- tests/console/test_auth_routes.py | 233 ++++++++++ tests/console/test_oauth_routes.py | 527 +++++++++++++++++++++++ tests/domain/test_oauth_clients.py | 193 +++++++++ tests/infrastructure/test_guild_oauth.py | 59 +++ 15 files changed, 2821 insertions(+), 49 deletions(-) create mode 100644 src/sturnus/console/routes_oauth.py create mode 100644 tests/console/test_oauth_routes.py create mode 100644 tests/domain/test_oauth_clients.py diff --git a/docs/operations.md b/docs/operations.md index 3c5a41f..0dfaf0f 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -2084,6 +2084,155 @@ nobody sizes anything on the migration's paragraph. the partial index that would fix it, and why the obvious cheaper fix (ordering by `status` first) must not be used. +### 6.2.12 A guild's own sign-in link + +A guild may sign its people in against **its own Outline**, rather than +against the one this deployment is configured with. That is the whole of +§2.2 of the phase-two specification, and it exists because a deployment +serving several organisations cannot ask them all to keep accounts in one +Outline. + +**A deployment that configures none of this behaves exactly as it did +before.** `/api/auth/login` with no `guild` parameter uses +`STURNUS_OUTLINE_CLIENT_ID` / `STURNUS_OUTLINE_CLIENT_SECRET` and the +`console_state` row it writes names no guild. Nothing below is required +of anybody. + +#### The problem it solves, and why the link carries the guild + +`GET /api/auth/login` takes no parameters and reads no cookie — there is +no session yet, that is what login is for. So to choose a guild's OAuth +client it would need the guild; to learn the guild it would need an +identity; and to get an identity it would already have had to choose a +client. **The guild goes in the URL**, which is the only place it can be +before the round trip starts: + +``` +https://sturnus.onelitefeather.dev/api/auth/login?guild=acme +``` + +The alternative — a page listing every guild Sturnus serves, so somebody +could pick theirs — was rejected: it discloses which organisations use +this service to anyone, signed in or not. **An administrator distributes +their guild's link themselves.** + +#### What an administrator does, in order + +1. **Register an OAuth application in the guild's own Outline.** Its + redirect URI is this deployment's console callback — the same + `STURNUS_CONSOLE_REDIRECT_URI` value (§1.5), unless the guild has a + reason to use a different one. +2. **Register the client**, without its secret: + + ``` + PUT /api/guilds/{guild_id}/oauth-client + {"slug": "acme", + "provider": "outline", + "base_url": "https://outline.acme.example", + "client_id": "...", + "redirect_uri": null} + ``` + + `redirect_uri: null` means "this deployment's own callback", which is + what nearly every guild wants. +3. **Supply the secret**, which is a separate request and the only one + that ever carries a credential: + + ``` + PUT /api/guilds/{guild_id}/oauth-client/secret + {"client_secret": "..."} + ``` +4. **Hand out the link.** Until step 3 the guild's link answers exactly + as an unknown one does (see below), so the order matters. + +Only an administrator **of that guild** may do any of this. Every other +request — a guild somebody does not administer, a guild that does not +exist, a guild with no client configured — answers +`404 {"error": "no sign-in configuration"}`, and they are deliberately +one answer. + +#### The slug + +It is a public path segment that selects a credential, so its shape is +fixed: **3–32 characters, lowercase ASCII letters, digits and hyphens, +beginning with a letter, no leading, trailing or doubled hyphen**, and not +one of the names this deployment reserves for itself (`api`, `auth`, +`login`, `sign-in`, `static`, …). Beginning with a letter is what keeps a +slug and a Discord snowflake from being confusable in a link somebody +reads. + +Nothing is normalised: `Acme` is refused rather than lowercased, because +a slug quietly rewritten on the way into the table is a slug the +administrator does not recognise in the link they handed out. + +A slug already held by another guild, or one this deployment reserves, +answers `409 {"error": "that sign-in name is not available"}` — the same +reply either way, so which of the two it was cannot be read off the +response. + +#### The secret never comes back + +`GET /api/guilds/{guild_id}/oauth-client` answers the slug, the provider, +the base URL, the client id, the redirect URI and `has_secret`. **Never +the secret**, not masked and not truncated. That is why this is not a +`guild_config` key: the settings API renders every value it holds back to +whichever administrator asks (§4.1), which is right for a setting and a +disclosure for a credential. + +The stored secret is wrapped by the master key **and bound to the row it +sits in** — to the guild and to the purpose, so a wrapped secret moved +into another guild's row, or into that guild's export-target row, fails +to authenticate rather than decrypting into somebody else's credential. + +`DELETE /api/guilds/{guild_id}/oauth-client/secret` clears the secret and +leaves the registration in place, which is what to do when a secret +leaks: the guild's link stops working immediately and nobody else can +claim its slug in the meantime. + +#### Every refusal on the sign-in path is the same 404 + +`/api/auth/login?guild=…` answers `404 {"error": "no such sign-in link"}` +for **all** of: + +- no guild holds that slug, +- the slug is not spelled like a slug at all, +- a guild holds it but has no secret stored yet, +- it is registered against a provider this deployment cannot exchange + with, +- its secret is wrapped by a master key this process does not hold. + +That is the property the whole design exists to keep: an attacker walking +a list of names must not be able to tell "no such organisation here" from +"one, half-configured". The **operator's** way to tell them apart is the +log, not the response — a rotation that was not carried through emits +`key.id_mismatch` with the `guild_id` (§7). + +#### After a master-key rotation + +A guild's client secret is wrapped exactly like an audio data key, and +its `encryption_key_id` names the master key that wrapped it. A rotation +that leaves the old key behind makes that guild's link stop working — +silently from the outside, loudly in the log. Re-supplying the secret +through step 3 above re-wraps it under the current key. + +#### It is the console sign-in only, and that is not an oversight + +**The Discord account-link flow (`/link`) stays on the +environment-configured client, permanently.** `api` holds the master key +and `link` does not — `charts/sturnus/templates/_helpers.tpl` refuses to +render it onto that component at all (§1.4) — so `link` cannot unwrap a +guild's secret and must not be given the ability to. That asymmetry is +the architecture rather than a gap in it: it is what keeps the +internet-facing link process unable to decrypt anything. + +#### Every change is audited + +Registering, changing, or removing a guild's OAuth client, and setting or +clearing its secret, each emit `console.oauth_client_changed` at +**WARNING** with `guild_id`, `requested_by` and an `outcome` of +`registered`, `secret_set`, `secret_cleared` or `removed`. Neither half +of the credential is on the line — not the secret, and not the client id. + ### 6.3 Listening to a recording by hand Every automated check this system has can describe a track — its level, diff --git a/src/sturnus/console/adapters.py b/src/sturnus/console/adapters.py index 23dad90..d78fa8e 100644 --- a/src/sturnus/console/adapters.py +++ b/src/sturnus/console/adapters.py @@ -19,6 +19,7 @@ from __future__ import annotations +import logging from collections.abc import Callable, Iterable, Sequence from dataclasses import dataclass from datetime import UTC, datetime, timedelta, tzinfo @@ -26,6 +27,7 @@ from sqlalchemy import Row, case, delete, distinct, func, or_, select, update from sqlalchemy.dialects.postgresql import insert +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from sqlalchemy.sql import Subquery @@ -47,10 +49,13 @@ CollectionListing, ConsentHolder, ConsentPage, + ConsumedSignIn, DownloadableTrack, GuildDirectory, GuildQueue, GuildRecording, + GuildSignIn, + OAuthClient, OwnConsent, PersonRevocation, QueuedSession, @@ -74,6 +79,12 @@ scope_of, ) from sturnus.domain.exports import SessionDocument +from sturnus.domain.oauth_clients import ( + GuildOAuthClient, + SlugUnavailable, + is_valid_slug, +) +from sturnus.infrastructure.db.guild_oauth import GuildOAuthClientStore from sturnus.infrastructure.db.models import ( AccountLink, AdminMember, @@ -107,6 +118,10 @@ load_session, load_status, ) +from sturnus.infrastructure.documents.outline_oauth import OutlineOAuth +from sturnus.observability.events import Event, log_exception + +log = logging.getLogger(__name__) #: How long a sign-in may take. Ten minutes is a browser round trip #: through a login page with room for somebody to be interrupted, and it @@ -131,29 +146,256 @@ async def new(self, now: datetime) -> str: await self.issue(state, now) return state - async def issue(self, state: str, now: datetime) -> None: + async def issue(self, state: str, now: datetime, guild_id: int | None = None) -> None: + """Writes the state, and which guild's client the sign-in began against. + + `None` is the ordinary case and means the environment-configured + client. The guild is stored rather than held in this process's + memory for the same reason the state is: the callback may be + served by a different pod than issued the redirect. + """ async with self._session_factory() as session: - session.add(ConsoleState(state=state, created_at=now, expires_at=now + self._ttl)) + session.add( + ConsoleState( + state=state, + created_at=now, + expires_at=now + self._ttl, + guild_id=guild_id, + ) + ) await session.commit() - async def consume(self, state: str, now: datetime) -> bool: - """Consumes the state, reporting whether it was valid. + async def consume(self, state: str, now: datetime) -> ConsumedSignIn | None: + """Consumes the state and answers what it says, or `None`. `DELETE ... RETURNING` in one statement, the same shape `LinkStateStore.consume` uses and for the same reason: two callbacks replaying the same state concurrently can never both succeed, because only the delete that actually removes the row gets a result back. + + **The guild comes back out of the row rather than out of the + request**, which is what makes the state the thing that selects + the client for the code exchange: a callback cannot name a guild, + so it cannot ask this process to spend one guild's client secret + against another guild's provider. """ async with self._session_factory() as session: - row = await session.execute( - delete(ConsoleState) - .where(ConsoleState.state == state, ConsoleState.expires_at > now) - .returning(ConsoleState.state) - ) - consumed = row.scalar_one_or_none() is not None + row = ( + await session.execute( + delete(ConsoleState) + .where(ConsoleState.state == state, ConsoleState.expires_at > now) + .returning(ConsoleState.state, ConsoleState.guild_id) + ) + ).first() await session.commit() - return consumed + return None if row is None else ConsumedSignIn(guild_id=row.guild_id) + + +class GuildSignInClients: + """Resolves which OAuth client one sign-in runs against. + + The concrete side of `SignInClients`, and the whole of §2.2's answer + to the chicken-and-egg problem: before the round trip the slug in the + URL names the guild, after it the state does, and a client is built + per sign-in from whichever of those was available. + + **A client is built, not cached.** An `OutlineOAuth` holds a client + secret, and the alternative to constructing one per sign-in is a map + of guild id to live credential that has to be invalidated when an + administrator clears a secret -- and that goes on completing sign-ins + against a revoked credential for as long as it does not notice. The + cost is one indexed row read and one unwrap per redirect, which is + the cheapest half of a login. + + **Everything unusable answers `None`, and they are one answer.** An + unknown slug, a provider this deployment cannot exchange with, a + registration whose secret was never set, and a secret wrapped by a + master key this process does not hold all resolve to nothing. + Distinguishing them would let anybody with a browser learn which + organisations use this service by walking a list of names -- which is + the disclosure the guild-specific-link design exists to avoid, and + the reason it was chosen over a public list of guilds. + + **The Discord account-link flow is not served from here.** `api` + holds the master key and `link` does not; the chart's `_helpers.tpl` + refuses to render it onto that component. So `/link` stays on the + environment-configured client, and that asymmetry is the + architecture rather than an oversight in it: it is what keeps the + internet-facing link process unable to decrypt anything at all. + """ + + def __init__( + self, + environment: OAuthClient, + clients: GuildOAuthClientStore, + *, + redirect_uri: str, + ) -> None: + self._environment = environment + self._clients = clients + #: What a guild's registration means by leaving `redirect_uri` + #: null: this deployment's own console callback. Held here rather + #: than defaulted in the table so that changing where the console + #: lives is a configuration change and not a migration over every + #: guild that never varied it. + self._redirect_uri = redirect_uri + + async def for_slug(self, slug: str) -> GuildSignIn | None: + """The client behind a sign-in link, or `None` if there is none to use. + + The shape of the slug is checked before the table is, because + `is_valid_slug` is what the registration path enforced: a slug it + would have refused cannot be in the table, so asking is a query + that can only ever miss. + """ + if not is_valid_slug(slug): + return None + registration = await self._clients.by_slug(slug) + if registration is None: + return None + client = await self._build(registration) + return None if client is None else GuildSignIn(registration.guild_id, client) + + async def for_guild(self, guild_id: int | None) -> OAuthClient | None: + """The client a state names: this guild's, or the deployment's own. + + `None` for the guild id is not a missing value -- it is the + ordinary sign-in, and it answers the environment-configured + client so that a deployment which has configured nothing per + guild behaves exactly as it did in v0.15.0. + """ + if guild_id is None: + return self._environment + registration = await self._clients.for_guild(guild_id) + return None if registration is None else await self._build(registration) + + async def _build(self, registration: GuildOAuthClient) -> OAuthClient | None: + """A usable client for this registration, or `None`. + + Half a registration is a real state: registering the client and + supplying its secret are two steps, and an administrator is + expected to be between them for as long as it takes to copy a + value out of another screen. A sign-in during that window has + nothing to exchange with, so it resolves to nothing -- the same + answer an unknown slug gets. + """ + if registration.provider != PROVIDER or not registration.has_secret: + return None + try: + secret = await self._clients.client_secret_for(registration.guild_id) + except ValueError as exc: + # A rotation that was not carried through: the row names a + # master key this process does not hold. Worth a line because + # a guild whose every sign-in fails here is an operator's + # problem and is invisible from the reply, which is + # indistinguishable from an unknown slug on purpose. Through + # `log_exception` so the store's message -- which names the + # two key ids -- travels as a type and fields rather than as + # text nobody scrubbed. + log_exception( + log, + logging.WARNING, + Event.KEY_ID_MISMATCH, + "A guild sign-in client is wrapped by a master key this process does not hold", + exc, + guild_id=registration.guild_id, + ) + return None + if secret is None: + return None + return OutlineOAuth( + base_url=registration.base_url, + client_id=registration.client_id, + client_secret=secret, + redirect_uri=registration.redirect_uri or self._redirect_uri, + ) + + +class ConsoleGuildOAuthClients: + """A guild's sign-in registration, for the administrators of that guild. + + Authorisation lives here rather than in the handler, the arrangement + `ConsoleGuildNames` and `ConsoleQueueOverview` use: every method + names who is asking, and there is none that does not. A handler + cannot forget a check it never had the option of making. + + **Not administering the guild and there being no registration are the + same answer**, so the routes can give both the same 404. That is + stricter than the settings endpoints, deliberately: whether a guild + has its own sign-in is the fact §2.2 keeps undiscoverable. + + **This class cannot produce a secret.** It writes one and it reports + whether one is set; there is no method here that returns one and no + field on `GuildOAuthClient` to carry it. Reading a secret is + `GuildSignInClients`'s alone, which is the object the sign-in path + holds and no route does. + """ + + def __init__(self, clients: GuildOAuthClientStore, admins: AdminDirectory) -> None: + self._clients = clients + self._admins = admins + + async def for_guild(self, guild_id: int, *, requested_by: int) -> GuildOAuthClient | None: + if not await self._admins.is_admin(guild_id, requested_by): + return None + return await self._clients.for_guild(guild_id) + + async def save( + self, + guild_id: int, + *, + requested_by: int, + slug: str, + provider: str, + base_url: str, + client_id: str, + redirect_uri: str | None, + now: datetime, + ) -> GuildOAuthClient | None: + """Registers or replaces, and answers with what is now stored. + + The slug collision is caught rather than checked for. Checking + first is a race -- two administrators claiming one name in the + same second -- and the unique constraint is not; translating it + into `SlugUnavailable` is what keeps `sqlalchemy` out of the + route module that has to answer it. + """ + if not await self._admins.is_admin(guild_id, requested_by): + return None + try: + await self._clients.save( + guild_id, + slug=slug, + provider=provider, + base_url=base_url, + client_id=client_id, + redirect_uri=redirect_uri, + now=now, + ) + except IntegrityError as exc: + raise SlugUnavailable("this sign-in name belongs to another guild") from exc + return await self._clients.for_guild(guild_id) + + async def set_secret( + self, guild_id: int, secret: str | None, *, requested_by: int, now: datetime + ) -> GuildOAuthClient | None: + """Stores or clears the secret, and answers with what can be read back. + + Which is never the secret. The re-read is what makes that true by + construction rather than by care: there is no path here from the + value that came in to the value that goes out. + """ + if not await self._admins.is_admin(guild_id, requested_by): + return None + if not await self._clients.set_client_secret(guild_id, secret, now): + return None + return await self._clients.for_guild(guild_id) + + async def delete(self, guild_id: int, *, requested_by: int) -> bool: + if not await self._admins.is_admin(guild_id, requested_by): + return False + return await self._clients.delete(guild_id) class ConsoleLinkDirectory: diff --git a/src/sturnus/console/app.py b/src/sturnus/console/app.py index 38ce604..c671d20 100644 --- a/src/sturnus/console/app.py +++ b/src/sturnus/console/app.py @@ -26,12 +26,19 @@ from aiohttp import web -from sturnus.console import routes_exports, routes_recording, routes_settings, routes_tags +from sturnus.console import ( + routes_exports, + routes_oauth, + routes_recording, + routes_settings, + routes_tags, +) from sturnus.console.audio import AudioDelivery from sturnus.console.auth import ( ConsoleAuth, ExchangeRefused, NotLinked, + UnknownSignIn, UnknownState, ) from sturnus.console.ports import ( @@ -41,9 +48,9 @@ DocumentArtefacts, ExportTargets, GuildNames, + GuildOAuthClients, GuildReports, LinkDirectory, - OAuthClient, PersonalConsents, PreferenceDirectory, ProfileDirectory, @@ -53,6 +60,7 @@ SessionNaming, SessionReads, SettingsStore, + SignInClients, StateStore, TagWriter, TranscriptReader, @@ -182,7 +190,26 @@ async def readyz(request: web.Request) -> web.Response: async def login(request: web.Request) -> web.StreamResponse: - url = await request.app[_AUTH].begin(request.app[_NOW]()) + """Starts a sign-in, against this deployment's client or a guild's own. + + `?guild={slug}` is how a guild's own identity provider is reached, and + it is a query parameter rather than a session or a header because + **there is no session here** -- that is what login is for. The slug is + the only thing that can name a guild before the round trip begins. + + Omitting it is the ordinary case and is unchanged: the deployment's + environment-configured client, exactly as in v0.15.0. + + A slug that names nothing and a slug that names a guild whose client + cannot complete a sign-in get **the same 404 and the same body**. That + is the point of the design rather than a convenience: a refusal that + distinguished them would let anybody with a browser walk a list of + names and learn which organisations use this service. + """ + try: + url = await request.app[_AUTH].begin(request.app[_NOW](), guild=request.query.get("guild")) + except UnknownSignIn: + return web.json_response({"error": "no such sign-in link"}, status=404) raise web.HTTPFound(url) @@ -202,7 +229,13 @@ async def callback(request: web.Request) -> web.StreamResponse: try: user = await request.app[_AUTH].authenticate(code, state, request.app[_NOW]()) - except UnknownState: + except (UnknownState, UnknownSignIn): + # `UnknownSignIn` here means the guild's registration went away + # while this person was at the consent screen. It answers as an + # unknown state rather than getting a status of its own: from the + # browser's side both are "this sign-in can no longer be + # completed, start again", and a distinct reply would say that a + # guild had a client a moment ago. return web.json_response({"error": "unknown or expired sign-in attempt"}, status=400) except ExchangeRefused: return web.json_response({"error": "the identity provider refused"}, status=403) @@ -265,7 +298,7 @@ async def logout(_request: web.Request) -> web.Response: def build_api( *, - oauth: OAuthClient, + clients: SignInClients, states: StateStore, links: LinkDirectory, admins: AdminDirectory, @@ -291,6 +324,7 @@ def build_api( exports: ExportTargets, documents: SessionDocumentDirectory, artefacts: DocumentArtefacts, + oauth_clients: GuildOAuthClients, ) -> web.Application: """Builds the application, with every collaborator injected. @@ -306,7 +340,7 @@ def build_api( from sturnus.console import routes_read app = web.Application() - app[_AUTH] = ConsoleAuth(oauth, states, links) + app[_AUTH] = ConsoleAuth(clients, states, links) app[_SESSIONS] = sessions app[_ADMINS] = admins app[routes_read.READS] = reads @@ -330,6 +364,7 @@ def build_api( app[routes_exports.EXPORT_TARGETS] = exports app[SESSION_DOCUMENTS] = documents app[DOCUMENT_ARTEFACTS] = artefacts + app[routes_oauth.GUILD_OAUTH_CLIENTS] = oauth_clients app.add_routes( [ web.get("/healthz", healthz), @@ -352,4 +387,5 @@ def build_api( routes_recording.register(app) routes_exports.register(app) register_documents(app) + routes_oauth.register(app) return app diff --git a/src/sturnus/console/auth.py b/src/sturnus/console/auth.py index 8c349fb..68500a5 100644 --- a/src/sturnus/console/auth.py +++ b/src/sturnus/console/auth.py @@ -11,6 +11,29 @@ what `session_participant` names, so a login that skipped the lookup would mint a session that can be scoped to nobody -- and the handler that later forgot to check would scope it to everybody. + +**Which client is a per-sign-in question, not a per-process one** (§2.2). +This class held one `OAuthClient` for the life of the process, which is +what made "sign in against *this guild's* identity provider" +unrepresentable. It now holds a `SignInClients` and asks it twice, once at +each end of the round trip, from the only thing available there: + + begin(guild="acme") --> for_slug("acme") --> (guild id, client) + | | + | the guild id goes into the state + v | + authenticate(code, state) <-- for_guild(state.guild_id) <-- client + +The asymmetry is forced and worth naming: `GET /api/auth/login` takes no +parameters and reads no cookie -- there is no session yet, that is what +login is for -- so before the redirect the slug in the URL is the only +thing that can name a guild, and after it the state is. **The state is +what selects the client for the code exchange**, so a callback cannot be +steered onto a different guild's credential by anything the caller sends. + +Sign-in without a guild is unchanged and stays the ordinary case: a +deployment that configures no per-guild client behaves exactly as it did +in v0.15.0, against the environment-configured client. """ from __future__ import annotations @@ -20,9 +43,9 @@ from dataclasses import dataclass from datetime import datetime -from sturnus.console.ports import LinkDirectory, OAuthClient, StateStore +from sturnus.console.ports import LinkDirectory, SignInClients, StateStore from sturnus.infrastructure.documents.outline_oauth import LinkExchangeError -from sturnus.observability.events import Event, log_exception +from sturnus.observability.events import Event, log_event, log_exception log = logging.getLogger(__name__) @@ -55,6 +78,25 @@ class UnknownState(Exception): """No login this server started corresponds to this callback.""" +class UnknownSignIn(Exception): + """No sign-in can be run against what was asked for. + + One exception for four situations that must stay indistinguishable + from outside: the slug names no guild, it names a guild whose client + was never given a secret, it names one registered against a provider + this deployment cannot exchange with, and it names one whose secret + this process cannot unwrap because the master key was rotated without + it. An attacker walking a list of names must not be able to tell + "there is no such organisation here" from "there is one, and its + sign-in is half-configured" -- that disclosure is the thing §2.2 + chose guild-specific links to avoid. + + It is also what `authenticate` raises when the registration a state + named has gone away mid-login, which is the same statement about the + same guild made half a round trip later. + """ + + @dataclass(frozen=True) class AuthenticatedUser: """Who the callback established, in the terms the rest of the console uses.""" @@ -66,16 +108,39 @@ class AuthenticatedUser: class ConsoleAuth: """Begins and completes the console's sign-in flow.""" - def __init__(self, oauth: OAuthClient, states: StateStore, links: LinkDirectory) -> None: - self._oauth = oauth + def __init__(self, clients: SignInClients, states: StateStore, links: LinkDirectory) -> None: + self._clients = clients self._states = states self._links = links - async def begin(self, now: datetime) -> str: - """Issues a fresh state and returns the URL to send the browser to.""" + async def begin(self, now: datetime, *, guild: str | None = None) -> str: + """Issues a fresh state and returns the URL to send the browser to. + + `guild` is the slug out of the sign-in link, or `None` for the + deployment's own sign-in. The guild is resolved *before* the state + is issued, so a slug that names nothing leaves no row behind -- + and so an unusable slug cannot be told from an unknown one by + watching what the server did. + + The resolved guild id goes into the state, which is the whole + mechanism: it is the only thing that survives to the callback, + where there is still no session to read a guild from. + """ + chosen = None if guild is None else await self._clients.for_slug(guild) + if guild is not None and chosen is None: + raise UnknownSignIn("this sign-in link resolves to nothing") + + if chosen is None: + client = await self._clients.for_guild(None) + guild_id = None + if client is None: # pragma: no cover - the environment client always resolves + raise UnknownSignIn("this deployment has no sign-in client") + else: + client, guild_id = chosen.client, chosen.guild_id + state = secrets.token_urlsafe(_STATE_BYTES) - await self._states.issue(state, now) - return self._oauth.authorize_url(state) + await self._states.issue(state, now, guild_id) + return client.authorize_url(state) async def authenticate(self, code: str, state: str, now: datetime) -> AuthenticatedUser: """Completes the flow, or raises the specific reason it could not. @@ -85,12 +150,37 @@ async def authenticate(self, code: str, state: str, now: datetime) -> Authentica worth an outbound request to the provider, and consuming first is also what makes the state single-use against a caller that replays the same URL twice in parallel. + + **The consumed state is what selects the client**, and the query + string contributes nothing but the code. A callback that could + name its own guild would be a callback that could ask this + process to spend one guild's client secret on a code issued by + another guild's provider. """ - if not await self._states.consume(state, now): + consumed = await self._states.consume(state, now) + if consumed is None: raise UnknownState("no login corresponds to this callback") + oauth = await self._clients.for_guild(consumed.guild_id) + if oauth is None: + # Reachable without anybody misbehaving: an administrator + # deleting their guild's registration, or clearing its + # secret, while somebody is at the provider's consent screen. + # Logged because a guild whose every sign-in ends here is an + # operator's problem and is otherwise invisible; the guild id + # is registered and the client id is deliberately not. + log_event( + log, + logging.WARNING, + Event.CONSOLE_SIGN_IN_REJECTED, + "A sign-in came back for a guild that no longer has a usable client", + guild_id=consumed.guild_id, + reason="client_unresolvable", + ) + raise UnknownSignIn("the client this sign-in began against is gone") + try: - identity = await self._oauth.identity_from_code(code) + identity = await oauth.identity_from_code(code) except LinkExchangeError as exc: # Through `log_exception` rather than `%s`: the exception's own # message travels only if `SAFE_MESSAGE_TYPES` vouches for its diff --git a/src/sturnus/console/ports.py b/src/sturnus/console/ports.py index bbfceaf..5cdcfa8 100644 --- a/src/sturnus/console/ports.py +++ b/src/sturnus/console/ports.py @@ -37,27 +37,106 @@ TagUse, ) from sturnus.domain.exports import ExportTarget, SessionDocument +from sturnus.domain.oauth_clients import GuildOAuthClient from sturnus.infrastructure.documents.outline_oauth import ExternalIdentity class OAuthClient(Protocol): - """The identity provider the console authenticates against.""" + """The identity provider *one* sign-in authenticates against. + + One client, not the deployment's client. Which one this is comes from + `SignInClients`, and the whole point of that indirection is that a + handler holding this protocol cannot tell -- and must not have to + know -- whether it is the environment-configured client or a guild's + own. + """ def authorize_url(self, state: str) -> str: ... async def identity_from_code(self, code: str) -> ExternalIdentity: ... +@dataclass(frozen=True) +class GuildSignIn: + """A guild's own client, and the guild it belongs to, resolved from a slug. + + The two travel together because the caller needs both and must not + derive one from the other: the client completes the round trip, and + the guild id is what goes into the state so the *callback* can select + the same client again. A resolver that answered only the client would + leave `ConsoleAuth` looking the guild up a second time from the slug, + which is a second lookup that can disagree with the first. + """ + + guild_id: int + client: OAuthClient + + +class SignInClients(Protocol): + """Which OAuth client a sign-in runs against, asked twice per login. + + This is the seam §2.2 asks for. `ConsoleAuth` used to hold one + `OAuthClient` for the life of the process, which made "this guild's + client" unrepresentable; it now holds this, and resolves a client per + sign-in from the only thing available at each end of the round trip. + + **Before the redirect** the guild is in the URL, so `for_slug` is + asked. **After it** there is no URL and no session -- that is what + login is for -- so the guild comes back out of the state that was + issued, and `for_guild` is asked. + + `for_slug` answers `None` for a slug that names no guild **and** for a + slug that names one whose client cannot complete a sign-in: an + unregistered provider, a secret that was never set, a secret this + process cannot unwrap. Those are one answer on purpose. Telling them + apart would let anybody with a browser walk a list of names and learn + which organisations use this service, which is the disclosure the + whole guild-specific-link design exists to avoid. + + `for_guild(None)` is the environment-configured client and never + answers `None`: a deployment that has configured no per-guild client + at all behaves exactly as it did in v0.15.0. `for_guild(some_id)` + answers `None` under the same conditions `for_slug` does, which is + what happens when a guild's registration is deleted or unwrapped + while somebody is mid-login. + """ + + async def for_slug(self, slug: str) -> GuildSignIn | None: ... + + async def for_guild(self, guild_id: int | None) -> OAuthClient | None: ... + + +@dataclass(frozen=True) +class ConsumedSignIn: + """What a valid state says about the login it was issued for. + + Carries the guild rather than being a `bool`, because **the state is + what selects the client for the code exchange** (§2.2). `None` means + the sign-in was begun without a guild and belongs to the + environment-configured client -- a real value, and the ordinary one, + which is why the absence of a state is `None` *of this type* rather + than a `guild_id` of `None`. + """ + + guild_id: int | None + + class StateStore(Protocol): - """Single-use OAuth states, tying a callback to a login this server began.""" + """Single-use OAuth states, tying a callback to a login this server began. + + `issue` takes the guild the sign-in was begun for, or `None`. It is + stored rather than kept in this process's memory for the reason the + state itself is: the callback may be served by a different pod than + the redirect was. + """ - async def issue(self, state: str, now: datetime) -> None: ... + async def issue(self, state: str, now: datetime, guild_id: int | None = None) -> None: ... - #: `False` for a state that was never issued, has already been used, or + #: `None` for a state that was never issued, has already been used, or #: has expired -- the caller treats all three identically, because from #: the outside they are the same event: this is not a callback for a #: login we started. - async def consume(self, state: str, now: datetime) -> bool: ... + async def consume(self, state: str, now: datetime) -> ConsumedSignIn | None: ... class LinkDirectory(Protocol): @@ -1145,3 +1224,58 @@ class GuildReports(Protocol): """ async def recording_of(self, guild_id: int, *, requested_by: int) -> GuildRecording | None: ... + + +class GuildOAuthClients(Protocol): + """One guild's own sign-in client, if the person asking administers it. + + The write side of what `SignInClients` reads. Two protocols rather + than one, because they are held by different code for different + reasons and one of them must never grow the other's methods: the + sign-in path resolves a *usable client* and never a registration, and + the settings path edits a registration and never obtains a client + that could complete an exchange. + + `requested_by` is not optional and there is no method here without + it, for the reason `GuildNames`, `QueueOverview` and `GuildReports` + have none: the authorisation rule lives inside the call rather than + in a handler that could forget to apply it. + + **`None` and `False` cover "no such guild", "you do not administer + it" and "there is no client configured" alike**, and the routes + answer all three with the same 404. This is stricter than the + settings endpoints next door, which answer 403 for a guild somebody + does not administer, and deliberately so: whether a guild has its own + sign-in configured is the fact §2.2 does not want discoverable, and a + refusal that distinguishes "not yours" from "not configured" is a + two-request oracle for it. + + **No method here returns a secret**, and there is nowhere in + `GuildOAuthClient` to put one. `set_secret` is write-only and answers + with the registration as it now reads -- `has_secret`, never the + value. + """ + + async def for_guild(self, guild_id: int, *, requested_by: int) -> GuildOAuthClient | None: ... + + #: Registers or replaces the registration, secret untouched. Raises + #: `SlugUnavailable` if the slug belongs to another guild. + async def save( + self, + guild_id: int, + *, + requested_by: int, + slug: str, + provider: str, + base_url: str, + client_id: str, + redirect_uri: str | None, + now: datetime, + ) -> GuildOAuthClient | None: ... + + #: Stores the secret, or clears it when `secret` is `None`. + async def set_secret( + self, guild_id: int, secret: str | None, *, requested_by: int, now: datetime + ) -> GuildOAuthClient | None: ... + + async def delete(self, guild_id: int, *, requested_by: int) -> bool: ... diff --git a/src/sturnus/console/routes_oauth.py b/src/sturnus/console/routes_oauth.py new file mode 100644 index 0000000..67edbe7 --- /dev/null +++ b/src/sturnus/console/routes_oauth.py @@ -0,0 +1,401 @@ +"""A guild's own sign-in client: five routes, and one thing that never comes back. + +**The secret is write-only, and there is nowhere in the read model to put +it.** `GET` on an OAuth configuration answers the slug, the provider, the +base URL, the client id, the redirect URI and `has_secret` -- never the +value, not masked, not truncated, not "the last four characters". That is +why this configuration is not a `guild_config` key: the settings API +renders every value it holds straight back to whoever asks for it, which +is the correct behaviour for a setting and a disclosure for a credential +(§2.2). + +**Every refusal here is the same 404.** Not administering the guild, no +such guild, and no client configured for it all answer +`{"error": "no sign-in configuration"}`. That is deliberately stricter +than `routes_settings` next door, which answers 403 for a guild somebody +does not administer, and the difference is the whole point of §2.2's +design: whether a given guild has its own sign-in is exactly the fact the +guild-specific-link arrangement exists to keep undiscoverable. A refusal +that said "you are not an administrator of this guild" would be a +one-request oracle for it. + +**No user input is reflected into a response**, the same rule the rest of +`sturnus.console.app` follows. The reasons below are fixed strings, which +matters more here than elsewhere: one of the values these handlers parse +is a client secret, and an endpoint that echoed what it refused would +have echoed one the first time somebody sent a malformed body. + +**What is validated here and what is not.** The slug and the two URLs are +decided in `sturnus.domain.oauth_clients`, in pure functions a test +reaches without a server, because the sign-in path has to agree with the +write path about what a slug is -- and two copies of that rule is how the +two come to disagree. This module calls them; it does not restate them. +""" + +from __future__ import annotations + +import json +import logging +from dataclasses import dataclass +from datetime import datetime +from typing import Any + +from aiohttp import web + +from sturnus.console.auth import PROVIDER +from sturnus.console.ports import GuildOAuthClients +from sturnus.domain.oauth_clients import ( + GuildOAuthClient, + SlugUnavailable, + has_slug_shape, + is_provider_url, + is_valid_slug, +) +from sturnus.observability.events import Event, log_event + +log = logging.getLogger(__name__) + +#: The store of per-guild sign-in clients, under its own key for the +#: reason `SETTINGS_STORE` has one: `build_api` stays a one-line edit +#: while several branches are adding sections to it. +GUILD_OAUTH_CLIENTS: web.AppKey[GuildOAuthClients] = web.AppKey("guild_oauth_clients") + +#: The one refusal. Every way of not being allowed to see or change a +#: guild's sign-in configuration produces this and nothing else. +_NO_CONFIGURATION = "no sign-in configuration" +_MALFORMED_BODY = "malformed request body" +_MALFORMED_SLUG = "the sign-in name must be lowercase letters, digits and hyphens" +_SLUG_UNAVAILABLE = "that sign-in name is not available" +_UNSUPPORTED_PROVIDER = "unsupported identity provider" +_MALFORMED_URL = "the base URL and redirect URI must be https addresses" +_MALFORMED_CLIENT_ID = "the client id must be a non-empty string" +_MALFORMED_SECRET = "the client secret must be a non-empty string" + +#: What a write did, for the audit line: bounded literals from this +#: file's own source, never the value that was written. +_REGISTERED = "registered" +_SECRET_SET = "secret_set" +_SECRET_CLEARED = "secret_cleared" +_REMOVED = "removed" + +#: Bounds on the two free-text fields. Not a claim about what any +#: provider issues -- they are what keeps a `Text` column from being a +#: place to store a megabyte through an authenticated endpoint. +_MAX_CLIENT_ID = 512 +_MAX_SECRET = 1024 +_MAX_URL = 2048 + + +def register(app: web.Application) -> None: + """Adds the sign-in configuration routes to an application with a session. + + `require_session` on all five, applied here rather than as a + decorator, for the reason `routes_settings.register` gives: the + authentication decision stays visible at the routes it protects, so a + route added without it is visibly public rather than silently public. + + The secret has routes of its own rather than being a field on the + registration, mirroring the two-method split `GuildOAuthClientStore` + already has. It is what makes "save the registration" a request that + demonstrably cannot carry a credential, and it is what lets an + administrator re-register a base URL without re-typing a secret they + may no longer have. + """ + from sturnus.console.app import require_session + + app.add_routes( + [ + web.get("/api/guilds/{guild_id}/oauth-client", require_session(read_client)), + web.put("/api/guilds/{guild_id}/oauth-client", require_session(write_client)), + web.delete("/api/guilds/{guild_id}/oauth-client", require_session(remove_client)), + web.put("/api/guilds/{guild_id}/oauth-client/secret", require_session(write_secret)), + web.delete("/api/guilds/{guild_id}/oauth-client/secret", require_session(clear_secret)), + ] + ) + + +# --------------------------------------------------------------------------- +# Handlers +# --------------------------------------------------------------------------- + + +async def read_client(request: web.Request) -> web.Response: + """This guild's sign-in configuration, without its secret.""" + guild_id = _guild_id(request) + client = await _clients(request).for_guild(guild_id, requested_by=_caller(request)) + if client is None: + raise _refusal(web.HTTPNotFound, _NO_CONFIGURATION) + return _answer(client) + + +async def write_client(request: web.Request) -> web.Response: + """Registers or replaces the registration, leaving any secret alone. + + A replacement rather than a patch, because a guild has one client and + the fields describe one OAuth application: an administrator moving to + a different Outline instance changes the base URL and the client id + together, and a partial write is how a client id ends up pointing at + the wrong host. + """ + guild_id = _guild_id(request) + registration = _registration(await _body(request)) + + try: + client = await _clients(request).save( + guild_id, + requested_by=_caller(request), + now=_now(request), + slug=registration.slug, + provider=registration.provider, + base_url=registration.base_url, + client_id=registration.client_id, + redirect_uri=registration.redirect_uri, + ) + except SlugUnavailable: + raise _refusal(web.HTTPConflict, _SLUG_UNAVAILABLE) from None + if client is None: + raise _refusal(web.HTTPNotFound, _NO_CONFIGURATION) + _audit(request, guild_id, outcome=_REGISTERED) + return _answer(client) + + +async def remove_client(request: web.Request) -> web.Response: + """Removes the registration and frees its slug. + + 204 rather than a body: there is nothing left to read back. A guild + that had none answers 404, so "already gone" and "removed" are not + the same reply to an administrator who clicked twice. + """ + guild_id = _guild_id(request) + if not await _clients(request).delete(guild_id, requested_by=_caller(request)): + raise _refusal(web.HTTPNotFound, _NO_CONFIGURATION) + _audit(request, guild_id, outcome=_REMOVED) + return web.Response(status=204) + + +async def write_secret(request: web.Request) -> web.Response: + """Stores the client secret, and answers with what can be read back. + + Which is the registration with `has_secret` true, and nothing else. + The response to storing a secret is the one response most likely to + be built by echoing what came in, so it is built by re-reading the + row instead. + """ + guild_id = _guild_id(request) + body = await _body(request) + secret = body.get("client_secret") + if not isinstance(secret, str) or not secret or len(secret) > _MAX_SECRET: + raise _refusal(web.HTTPBadRequest, _MALFORMED_SECRET) + + client = await _clients(request).set_secret( + guild_id, secret, requested_by=_caller(request), now=_now(request) + ) + if client is None: + raise _refusal(web.HTTPNotFound, _NO_CONFIGURATION) + _audit(request, guild_id, outcome=_SECRET_SET) + return _answer(client) + + +async def clear_secret(request: web.Request) -> web.Response: + """Forgets the client secret, leaving the registration in place. + + Half a registration is a real state and the interface has to be able + to reach it: an administrator whose secret has leaked wants it gone + now, and re-registering the whole client to achieve that would free + the slug in between. Sign-in through this guild's link stops working + immediately, answering exactly as an unknown slug does. + """ + guild_id = _guild_id(request) + client = await _clients(request).set_secret( + guild_id, None, requested_by=_caller(request), now=_now(request) + ) + if client is None: + raise _refusal(web.HTTPNotFound, _NO_CONFIGURATION) + _audit(request, guild_id, outcome=_SECRET_CLEARED) + return _answer(client) + + +# --------------------------------------------------------------------------- +# Reading the request +# --------------------------------------------------------------------------- + + +def _guild_id(request: web.Request) -> int: + """The guild from the path, or the same 404 everything else answers. + + A guild id that is not a number names no guild, so it gets the + refusal every other unanswerable request here gets rather than a 400 + that would distinguish it. + """ + try: + return int(request.match_info["guild_id"]) + except ValueError: + raise _refusal(web.HTTPNotFound, _NO_CONFIGURATION) from None + + +async def _body(request: web.Request) -> dict[str, Any]: + try: + body = await request.json() + except ValueError: + raise _refusal(web.HTTPBadRequest, _MALFORMED_BODY) from None + if not isinstance(body, dict): + raise _refusal(web.HTTPBadRequest, _MALFORMED_BODY) + return body + + +@dataclass(frozen=True) +class _Registration: + """The four fields a registration is, once they have been believed. + + A value rather than a `dict`, so that the handler that hands them to + the store cannot pass one of them under the wrong keyword and so that + the type checker sees the same four names the port declares. + """ + + slug: str + provider: str + base_url: str + client_id: str + redirect_uri: str | None + + +def _registration(body: dict[str, Any]) -> _Registration: + """The four fields a registration is, each refused on its own terms. + + The slug's two refusals are separate because they are different + answers to the administrator: a slug that is not spelled like a slug + is a mistake in what they typed (400), and one that is spelled + correctly but is not theirs to have is a name that is taken (409). + A reserved name and a name another guild holds are one refusal, so + that which of the two it was cannot be read off the reply. + """ + slug = body.get("slug") + if not isinstance(slug, str) or not has_slug_shape(slug): + raise _refusal(web.HTTPBadRequest, _MALFORMED_SLUG) + if not is_valid_slug(slug): + raise _refusal(web.HTTPConflict, _SLUG_UNAVAILABLE) + + provider = body.get("provider", PROVIDER) + if provider != PROVIDER: + # Not a 400 about a value this deployment might one day accept: + # a registration against a provider nothing here can exchange + # with is a client that resolves to nothing at sign-in time, and + # storing it would produce a guild whose link is permanently and + # silently broken. + raise _refusal(web.HTTPBadRequest, _UNSUPPORTED_PROVIDER) + + base_url = body.get("base_url") + if not isinstance(base_url, str) or len(base_url) > _MAX_URL or not is_provider_url(base_url): + raise _refusal(web.HTTPBadRequest, _MALFORMED_URL) + + redirect_uri = body.get("redirect_uri") + if redirect_uri is not None and ( + not isinstance(redirect_uri, str) + or len(redirect_uri) > _MAX_URL + or not is_provider_url(redirect_uri) + ): + raise _refusal(web.HTTPBadRequest, _MALFORMED_URL) + + client_id = body.get("client_id") + if not isinstance(client_id, str) or not client_id or len(client_id) > _MAX_CLIENT_ID: + raise _refusal(web.HTTPBadRequest, _MALFORMED_CLIENT_ID) + + return _Registration( + slug=slug, + provider=provider, + base_url=base_url, + client_id=client_id, + redirect_uri=redirect_uri, + ) + + +# --------------------------------------------------------------------------- +# Answering, and recording that it happened +# --------------------------------------------------------------------------- + + +def _answer(client: GuildOAuthClient) -> web.Response: + """The read model as JSON. There is no branch here that could add a secret.""" + return web.json_response( + { + # A snowflake as a string: a JSON number silently loses its + # last digits in JavaScript, producing an id that looks right + # and names nobody. + "guild_id": str(client.guild_id), + "oauth_client": { + "slug": client.slug, + "provider": client.provider, + "base_url": client.base_url, + "client_id": client.client_id, + # Present and null rather than absent for a guild using + # the deployment's own callback: a console that could not + # tell "the default" from "an API that does not send this + # field" would have to guess, and both guesses are wrong + # somewhere. + "redirect_uri": client.redirect_uri, + #: Whether a secret is stored. All that is left of it. + "has_secret": client.has_secret, + "created_at": client.created_at.isoformat(), + "updated_at": client.updated_at.isoformat(), + }, + } + ) + + +def _audit(request: web.Request, guild_id: int, *, outcome: str) -> None: + """The only record that a guild's sign-in credential changed. + + WARNING, a level above the settings writes next door, because this is + the credential that decides who gets a session at all: whoever + controls the identity provider behind a slug controls who this + console believes is signing in. + + Who, which guild, and which of the four acts it was. **Neither half + of the credential.** The secret is obvious; the client id is left off + because it is one half of a pair, and a retained, Grafana-readable + log is not the place to narrow the other half's blast radius by one + guess. + """ + log_event( + log, + logging.WARNING, + Event.CONSOLE_OAUTH_CLIENT_CHANGED, + "An administrator changed a guild's sign-in client", + guild_id=guild_id, + requested_by=_caller(request), + outcome=outcome, + ) + + +def _refusal(exception: type[web.HTTPException], reason: str) -> web.HTTPException: + """A refusal with a JSON body, raised rather than returned. + + aiohttp deprecated returning an `HTTPException` from a handler, and + raising lets the guards above read as a straight line instead of + threading an optional response back through every caller. + """ + return exception(text=json.dumps({"error": reason}), content_type="application/json") + + +def _clients(request: web.Request) -> GuildOAuthClients: + return request.app[GUILD_OAUTH_CLIENTS] + + +def _now(request: web.Request) -> datetime: + """The one clock this application was built with.""" + from sturnus.console.app import _NOW + + return request.app[_NOW]() + + +def _caller(request: web.Request) -> int: + """The Discord id of the person making this request. + + Only ever reached from behind `require_session`; `current_user` + raises rather than returning `None` if that is ever untrue, so a + route registered without the wrapper fails loudly instead of quietly + writing somebody else's guild. + """ + from sturnus.console.app import current_user + + return current_user(request).discord_user_id diff --git a/src/sturnus/domain/oauth_clients.py b/src/sturnus/domain/oauth_clients.py index 463bc50..861f7ae 100644 --- a/src/sturnus/domain/oauth_clients.py +++ b/src/sturnus/domain/oauth_clients.py @@ -22,8 +22,112 @@ from __future__ import annotations +import re from dataclasses import dataclass from datetime import datetime +from typing import Final +from urllib.parse import urlparse + +#: Short enough to be typed and read back over a chat message, long enough +#: to name an organisation. The lower bound is also what keeps a slug from +#: landing on the one- and two-letter path segments a deployment is most +#: likely to want for itself. +MIN_SLUG_LENGTH: Final = 3 +MAX_SLUG_LENGTH: Final = 32 + +#: Lowercase, hyphen-separated words, **beginning with a letter**. +#: +#: The leading letter is the rule that costs the most and buys the most: a +#: Discord snowflake is digits, and `/g/1289374650912837465/sign-in` and a +#: guild id in a path are the same string to whoever is reading the link. +#: Requiring a letter first makes a slug and an id unconfusable rather +#: than merely unlikely to be confused. +#: +#: One case, so `/g/Acme/sign-in` and `/g/acme/sign-in` cannot be two +#: links that look identical in a sans-serif font and select different +#: credentials. Refused rather than lowercased: a slug quietly rewritten +#: on the way into the table is a slug the administrator does not +#: recognise in the link they were told to hand out. +_SHAPE: Final = re.compile(r"[a-z][a-z0-9]*(-[a-z0-9]+)*", re.ASCII) + +#: Names this deployment serves, or may serve, itself. +#: +#: The console publishes a guild's link at `/g/{slug}/sign-in` and the API +#: reads the same word out of `?guild=`, so a slug is a path segment in +#: everything but name. Reserving these costs a guild one candidate and +#: buys the certainty that a route added later cannot be shadowed by a +#: name a guild already claimed -- a collision nobody can resolve +#: afterwards, because the loser is somebody's published sign-in link. +#: +#: Every entry is a name the shape rules would otherwise allow; +#: `tests/domain/test_oauth_slugs.py` pins that, so an entry that stops +#: doing any work is visible rather than decorative. +RESERVED_SLUGS: Final[frozenset[str]] = frozenset( + { + "api", + "assets", + "auth", + "callback", + "console", + "guild", + "guilds", + "healthz", + "login", + "logout", + "readyz", + "session", + "sessions", + "sign-in", + "sign-out", + "static", + "sturnus", + "well-known", + } +) + + +def has_slug_shape(slug: str) -> bool: + """Whether this is spelled like a slug, reservations aside. + + Separate from `is_valid_slug` because the two refusals are different + answers to an administrator registering one: a slug that is not + spelled like a slug is a mistake in what they typed, and a slug that + is spelled correctly but is not theirs to have is a name that is + taken. The read path never needs the distinction -- an unusable slug + resolves to nothing however it came to be unusable -- so it calls + `is_valid_slug` and asks no further. + """ + return MIN_SLUG_LENGTH <= len(slug) <= MAX_SLUG_LENGTH and _SHAPE.fullmatch(slug) is not None + + +def is_valid_slug(slug: str) -> bool: + """Whether this may be the word in a guild's sign-in link. + + The one decision about what a slug is, in a pure function with no + database behind it, because it is reached from two directions that + must agree: the administrator registering a slug, and the sign-in + endpoint resolving one. A login that looked up a slug the write path + would have refused is a query that can only ever miss -- and a login + that resolved one the write path allowed is the bug this function + exists to make impossible. + + Nothing here normalises. Trimming whitespace or lowercasing would + make `Acme `, `acme` and ` ACME` the same registration, and the link + an administrator distributes carries whichever of them they typed. + """ + return has_slug_shape(slug) and slug not in RESERVED_SLUGS + + +class SlugUnavailable(Exception): + """The slug asked for is not this guild's to have. + + One exception for "another guild already holds it" and for "this + deployment reserves it", because they are one answer to the person + asking: pick a different name. Splitting them would also hand a + caller a way to tell a claimed slug from a free one by which refusal + came back, and the claimed ones are precisely what §2.2 does not want + enumerable. + """ @dataclass(frozen=True, slots=True) @@ -58,3 +162,43 @@ class GuildOAuthClient: has_secret: bool created_at: datetime updated_at: datetime + + +def is_provider_url(value: str) -> bool: + """Whether this may be a guild's identity-provider base URL or callback. + + Held to more than "parses": the base URL is where a browser is sent + to authorise, and the redirect URI is where it comes back, so both + are addresses an administrator of one guild chooses and other people + follow. + + - **`https` only.** The authorization code and, for the base URL, the + whole consent step travel over it. A guild that could register + `http://` would be a guild whose members' codes cross the network + in the clear. + - **No userinfo.** `https://console.example@evil.example/` is a valid + URL naming `evil.example`, and reads to a human as the first host. + This is the one form where refusing to parse is the difference + between what an administrator reviewing the value sees and what a + browser does. + - **No query and no fragment.** `authorize_url` builds its own query + string; a base URL carrying one would produce two, and a fragment + never reaches a server at all. + + A path is allowed: an Outline behind `https://wiki.example/outline` + is an ordinary deployment, and `OutlineOAuth` appends to whatever it + is given. + """ + if value != value.strip() or any(character.isspace() for character in value): + return False + try: + parsed = urlparse(value) + except ValueError: + return False + return ( + parsed.scheme == "https" + and bool(parsed.hostname) + and "@" not in parsed.netloc + and not parsed.query + and not parsed.fragment + ) diff --git a/src/sturnus/entrypoints/api.py b/src/sturnus/entrypoints/api.py index 290e232..cbe4d29 100644 --- a/src/sturnus/entrypoints/api.py +++ b/src/sturnus/entrypoints/api.py @@ -42,6 +42,7 @@ ConsoleCollectionNames, ConsoleConsentDirectory, ConsoleGuildNames, + ConsoleGuildOAuthClients, ConsoleGuildReports, ConsoleLinkDirectory, ConsolePersonalConsents, @@ -54,6 +55,7 @@ ConsoleTagWriter, ConsoleTrackDirectory, ConsoleTranscripts, + GuildSignInClients, ) from sturnus.console.app import build_api from sturnus.console.audio import AudioDelivery @@ -63,6 +65,7 @@ from sturnus.infrastructure.db.admin_members import AdminMemberStore from sturnus.infrastructure.db.config_store import ConfigStore from sturnus.infrastructure.db.export_targets import ExportTargetStore +from sturnus.infrastructure.db.guild_oauth import GuildOAuthClientStore from sturnus.infrastructure.db.models import ( AccountLink, AdminMember, @@ -76,6 +79,7 @@ SessionDocument, UserPreference, ) +from sturnus.infrastructure.db.models import GuildOAuthClient as GuildOAuthClientRow from sturnus.infrastructure.db.preferences import PreferenceStore from sturnus.infrastructure.documents.outline_oauth import OutlineOAuth from sturnus.infrastructure.objectstore import S3AudioStore, S3DocumentStore @@ -104,6 +108,12 @@ AccountLink.__tablename__, AdminMember.__tablename__, ConsoleState.__tablename__, + # The per-guild sign-in clients. On the list because the *login* + # route reads it on every request that carries `?guild=`, so a + # `/readyz` that passed without it would be a console signing + # people in through the environment client while a guild's own + # link 500s. + GuildOAuthClientRow.__tablename__, # The settings section reads and writes this one. Without it here, # `/readyz` would pass while the first settings page 500s. GuildConfig.__tablename__, @@ -210,10 +220,17 @@ async def _run() -> None: admins = AdminMemberStore(session_factory) config = ConfigStore(session_factory) + # The master key is this process's and only this process's. `link` + # does not hold one -- the chart's `_helpers.tpl` refuses to render + # it there -- which is exactly why per-guild OAuth is available to + # the console sign-in and not to the Discord account-link flow, and + # why an export target's credential is unwrappable here and nowhere + # else. keys = KeyWrapper( base64.b64decode(settings.master_key.get_secret_value()), settings.master_key_id, ) + oauth_clients = GuildOAuthClientStore(session_factory, keys) audio = AudioDelivery( # The configuration store, because the download route's rule @@ -241,7 +258,12 @@ def now() -> datetime: schema_ready = False app = build_api( - oauth=oauth, + # Not the client itself any more: which client a sign-in runs + # against is a per-sign-in question now, and `oauth` above is + # what a sign-in with no guild resolves to. + clients=GuildSignInClients( + oauth, oauth_clients, redirect_uri=settings.console_redirect_uri + ), states=ConsoleStateStore(session_factory), links=ConsoleLinkDirectory(session_factory), admins=admins, @@ -282,6 +304,7 @@ def now() -> datetime: settings.s3_access_key.get_secret_value(), settings.s3_secret_key.get_secret_value(), ), + oauth_clients=ConsoleGuildOAuthClients(oauth_clients, admins), ) runner = web.AppRunner(app) diff --git a/src/sturnus/observability/events.py b/src/sturnus/observability/events.py index 3af421a..72408d4 100644 --- a/src/sturnus/observability/events.py +++ b/src/sturnus/observability/events.py @@ -291,6 +291,24 @@ class Event(StrEnum): #: saying its policy document does not name video. CONSOLE_CONSENT_SCOPE_REFUSED = "console.consent_scope_refused" + #: An administrator registered, changed or removed the OAuth client a + #: guild's own sign-in link runs against. **WARNING**, which is a + #: level above `console.setting_written` next door, because this is + #: the credential that decides *who gets a session at all*: whoever + #: controls the identity provider a slug points at controls who the + #: console believes is signing in. `outcome` says which act it was -- + #: registering, setting or clearing the secret, or removing the + #: registration entirely. + #: + #: It names `guild_id` and `requested_by` and **neither half of the + #: credential**. The secret is obvious; the client id is left off for + #: a less obvious reason, which is that it is one half of a + #: credential pair and a retained, Grafana-readable log is not where + #: the other half's blast radius should be narrowed by one guess. It + #: is in the table, readable by an administrator of that guild + #: through the API, which is where it belongs. + CONSOLE_OAUTH_CLIENT_CHANGED = "console.oauth_client_changed" + # -- cross-cutting ------------------------------------------------------ PROCESS_STARTING = "process.starting" SHUTDOWN_BEGIN = "shutdown.begin" diff --git a/tests/console/conftest.py b/tests/console/conftest.py index adc3518..f398ee9 100644 --- a/tests/console/conftest.py +++ b/tests/console/conftest.py @@ -39,14 +39,17 @@ ConsentDirectory, ConsentHolder, ConsentPage, + ConsumedSignIn, DocumentArtefacts, DownloadableTrack, ExportTargets, GuildDirectory, GuildNames, + GuildOAuthClients, GuildQueue, GuildRecording, GuildReports, + GuildSignIn, LinkDirectory, OAuthClient, OwnConsent, @@ -65,6 +68,7 @@ SessionNaming, SessionReads, SettingsStore, + SignInClients, StateStore, TagWriter, Track, @@ -80,6 +84,7 @@ ) from sturnus.domain import preferences from sturnus.domain.exports import ExportTarget, SessionDocument +from sturnus.domain.oauth_clients import GuildOAuthClient, SlugUnavailable, is_valid_slug from sturnus.infrastructure.crypto import CHUNK_SIZE, encrypt_file from sturnus.infrastructure.documents.outline_oauth import ExternalIdentity, LinkExchangeError @@ -114,42 +119,185 @@ async def make(app: web.Application) -> TestClient[web.Request, web.Application] class FakeOAuth: - """Stands in for `OutlineOAuth` without a live Outline.""" + """Stands in for `OutlineOAuth` without a live Outline. - def __init__(self, identity: ExternalIdentity | None = None, fail: bool = False) -> None: + `base_url` is what makes two of these tellable apart in a `Location` + header, which is how a test asserts that a sign-in went to *this* + guild's provider rather than the deployment's own. `exchanges` is the + other half of the same question, asked at the other end of the round + trip: which client actually spent a code. + """ + + def __init__( + self, + identity: ExternalIdentity | None = None, + fail: bool = False, + base_url: str = "https://outline.example", + ) -> None: self.identity = identity or ExternalIdentity(ANNA_OUTLINE, "Anna Example") self.fail = fail + self.base_url = base_url self.authorize_calls: list[str] = [] + #: Every code this client was asked to exchange, in order. + self.exchanges: list[str] = [] def authorize_url(self, state: str) -> str: self.authorize_calls.append(state) - return f"https://outline.example/oauth/authorize?state={state}" + return f"{self.base_url}/oauth/authorize?state={state}" async def identity_from_code(self, code: str) -> ExternalIdentity: + self.exchanges.append(code) if self.fail: raise LinkExchangeError("refused", status_code=400) - del code return self.identity class FakeStates: - """The single-use OAuth state store, in memory.""" + """The single-use OAuth state store, in memory. + + Remembers which guild each state was issued for, because that is the + thing the callback selects a client with -- a double that dropped it + would let a test pass while the state carried nothing. + """ def __init__(self) -> None: self.issued: list[str] = [] - self._valid: set[str] = set() + #: Which guild each issued state names, `None` for a sign-in + #: begun without one. + self.issued_for: dict[str, int | None] = {} + self._valid: dict[str, int | None] = {} - async def issue(self, state: str, now: datetime) -> None: + async def issue(self, state: str, now: datetime, guild_id: int | None = None) -> None: del now self.issued.append(state) - self._valid.add(state) + self.issued_for[state] = guild_id + self._valid[state] = guild_id - async def consume(self, state: str, now: datetime) -> bool: + async def consume(self, state: str, now: datetime) -> ConsumedSignIn | None: del now if state not in self._valid: + return None + return ConsumedSignIn(guild_id=self._valid.pop(state)) + + +class FakeSignInClients: + """Which client a sign-in runs against, without a database. + + `environment` is what a sign-in with no guild resolves to -- the + behaviour a deployment that has configured nothing per guild keeps. + `guilds` maps a slug to the guild that holds it and the client behind + it; `unusable` names slugs whose guild exists but whose client cannot + complete a sign-in, which is the case §2.2 requires to answer exactly + as an unknown slug does. + """ + + def __init__( + self, + environment: OAuthClient | None = None, + guilds: Mapping[str, tuple[int, OAuthClient]] | None = None, + ) -> None: + self.environment = environment or FakeOAuth() + self.guilds: dict[str, tuple[int, OAuthClient]] = dict(guilds or {}) + #: Every slug that was asked for, in order. + self.asked: list[str] = [] + + async def for_slug(self, slug: str) -> GuildSignIn | None: + self.asked.append(slug) + if not is_valid_slug(slug): + return None + found = self.guilds.get(slug) + return None if found is None else GuildSignIn(found[0], found[1]) + + async def for_guild(self, guild_id: int | None) -> OAuthClient | None: + if guild_id is None: + return self.environment + for held, client in self.guilds.values(): + if held == guild_id: + return client + return None + + +class FakeGuildOAuthClients: + """A guild's sign-in registration, in memory, with the same rule attached. + + Authorisation lives in the double exactly where it lives in the real + adapter: every method names who is asking, and "not an administrator" + and "no registration" are the same answer -- which is what lets the + routes give both the same 404. + """ + + def __init__( + self, + admins: AdminDirectory | None = None, + clients: dict[int, GuildOAuthClient] | None = None, + ) -> None: + self.admins = admins or FakeAdmins({ANNA}) + self.clients = clients or {} + #: The secrets, so a test can assert one was stored without any + #: route being able to read one back. + self.secrets: dict[int, str] = {} + + async def _may(self, guild_id: int, requested_by: int) -> bool: + return await self.admins.is_admin(guild_id, requested_by) + + async def for_guild(self, guild_id: int, *, requested_by: int) -> GuildOAuthClient | None: + if not await self._may(guild_id, requested_by): + return None + return self.clients.get(guild_id) + + async def save( + self, + guild_id: int, + *, + requested_by: int, + slug: str, + provider: str, + base_url: str, + client_id: str, + redirect_uri: str | None, + now: datetime, + ) -> GuildOAuthClient | None: + if not await self._may(guild_id, requested_by): + return None + for other, held in self.clients.items(): + if held.slug == slug and other != guild_id: + raise SlugUnavailable("taken") + existing = self.clients.get(guild_id) + self.clients[guild_id] = GuildOAuthClient( + guild_id=guild_id, + slug=slug, + provider=provider, + base_url=base_url, + client_id=client_id, + redirect_uri=redirect_uri, + has_secret=guild_id in self.secrets, + created_at=existing.created_at if existing else now, + updated_at=now, + ) + return self.clients[guild_id] + + async def set_secret( + self, guild_id: int, secret: str | None, *, requested_by: int, now: datetime + ) -> GuildOAuthClient | None: + if not await self._may(guild_id, requested_by): + return None + client = self.clients.get(guild_id) + if client is None: + return None + if secret is None: + self.secrets.pop(guild_id, None) + else: + self.secrets[guild_id] = secret + self.clients[guild_id] = replace( + client, has_secret=guild_id in self.secrets, updated_at=now + ) + return self.clients[guild_id] + + async def delete(self, guild_id: int, *, requested_by: int) -> bool: + if not await self._may(guild_id, requested_by): return False - self._valid.discard(state) - return True + self.secrets.pop(guild_id, None) + return self.clients.pop(guild_id, None) is not None class FakeLinks: @@ -1050,6 +1198,7 @@ async def get(self, key: str) -> bytes: def build_test_api( *, oauth: OAuthClient | None = None, + clients: SignInClients | None = None, states: StateStore | None = None, links: LinkDirectory | None = None, admins: AdminDirectory | None = None, @@ -1071,6 +1220,7 @@ def build_test_api( exports: ExportTargets | None = None, documents: SessionDocumentDirectory | None = None, artefacts: DocumentArtefacts | None = None, + oauth_clients: GuildOAuthClients | None = None, sessions: SessionCookie | None = None, now: Callable[[], datetime] | None = None, schema_ready: bool = True, @@ -1097,7 +1247,11 @@ def build_test_api( # while the two disagreed about who administers what. administrators = admins or FakeAdmins() return build_api( - oauth=oauth or FakeOAuth(), + # `oauth` names the client a sign-in with no guild resolves to, + # which is what nearly every test in this suite means by it. + # `clients` is the override for a test that is actually about + # which client gets chosen. + clients=clients or FakeSignInClients(environment=oauth or FakeOAuth()), states=states or FakeStates(), links=links or FakeLinks(), admins=administrators, @@ -1124,6 +1278,7 @@ def build_test_api( exports=exports or FakeExportTargets(), documents=documents or FakeSessionDocuments(), artefacts=artefacts or FakeArtefacts(), + oauth_clients=oauth_clients or FakeGuildOAuthClients(admins=administrators), sessions=sessions or SessionCookie(SECRET, timedelta(hours=12)), now=now or now_at(), schema_ready=lambda: schema_ready, diff --git a/tests/console/test_adapters.py b/tests/console/test_adapters.py index f070bcf..427ec13 100644 --- a/tests/console/test_adapters.py +++ b/tests/console/test_adapters.py @@ -8,12 +8,14 @@ from __future__ import annotations from datetime import UTC, datetime, timedelta +from urllib.parse import unquote import pytest from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine from sturnus.console.adapters import ( + ConsoleGuildOAuthClients, ConsoleLinkDirectory, ConsoleProfileDirectory, ConsoleSessionDocuments, @@ -22,14 +24,19 @@ ConsoleTagWriter, ConsoleTrackDirectory, ConsoleTranscripts, + GuildSignInClients, ) +from sturnus.console.ports import ConsumedSignIn, OAuthClient from sturnus.console.statistics import SessionName from sturnus.domain import settings +from sturnus.domain.oauth_clients import SlugUnavailable from sturnus.infrastructure.crypto import KeyWrapper from sturnus.infrastructure.db.admin_members import AdminMemberStore from sturnus.infrastructure.db.config_store import ConfigStore from sturnus.infrastructure.db.export_targets import ExportTargetStore +from sturnus.infrastructure.db.guild_oauth import GuildOAuthClientStore from sturnus.infrastructure.db.models import ( + AdminMember, Base, Session, SessionParticipant, @@ -38,6 +45,7 @@ ) from sturnus.infrastructure.db.repositories import AccountLinkRepository from sturnus.infrastructure.db.session_documents import SessionDocumentStore +from sturnus.infrastructure.documents.outline_oauth import ExternalIdentity T0 = datetime(2026, 8, 21, 12, 0, 0, tzinfo=UTC) ANNA, BEN = 100, 200 @@ -65,14 +73,48 @@ async def test_an_issued_state_can_be_consumed_once( ) -> None: states = ConsoleStateStore(factory) await states.issue("abc", T0) - assert await states.consume("abc", T0) is True - assert await states.consume("abc", T0) is False + assert await states.consume("abc", T0) == ConsumedSignIn(guild_id=None) + assert await states.consume("abc", T0) is None async def test_a_state_that_was_never_issued_is_refused( factory: async_sessionmaker[AsyncSession], ) -> None: - assert await ConsoleStateStore(factory).consume("never-issued", T0) is False + assert await ConsoleStateStore(factory).consume("never-issued", T0) is None + + +async def test_a_state_carries_the_guild_its_sign_in_began_against( + factory: async_sessionmaker[AsyncSession], +) -> None: + """The state is what selects the client for the code exchange (2.2). + + There is no session and no URL at the callback, so this row is the + only thing that survives the round trip -- and because it is a row + rather than process memory, the pod that serves the callback need not + be the one that issued the redirect. + """ + states = ConsoleStateStore(factory) + await states.issue("abc", T0, GUILD) + consumed = await states.consume("abc", T0) + assert consumed is not None + assert consumed.guild_id == GUILD + + +async def test_a_sign_in_with_no_guild_says_so_rather_than_saying_nothing( + factory: async_sessionmaker[AsyncSession], +) -> None: + """`None` is a value here, not an absence. + + It means the environment-configured client, which is the ordinary + sign-in and the one a deployment that configures no per-guild client + ever uses. The absence of a state is `None` of the whole result, so + the two are never the same answer. + """ + states = ConsoleStateStore(factory) + await states.issue("abc", T0) + consumed = await states.consume("abc", T0) + assert consumed is not None + assert consumed.guild_id is None async def test_an_expired_state_is_refused( @@ -85,7 +127,7 @@ async def test_an_expired_state_is_refused( """ states = ConsoleStateStore(factory, ttl=timedelta(minutes=10)) await states.issue("abc", T0) - assert await states.consume("abc", T0 + timedelta(minutes=11)) is False + assert await states.consume("abc", T0 + timedelta(minutes=11)) is None async def test_a_console_state_belongs_to_no_discord_user_yet( @@ -970,3 +1012,329 @@ async def test_another_sessions_document_is_not_reachable_through_this_one( await _publish(factory, theirs, target_id) assert await ConsoleSessionDocuments(factory).document_of(mine, target_id) is None + + +# --------------------------------------------------------------------------- +# Which client a sign-in runs against +# --------------------------------------------------------------------------- + + +MASTER = b"m" * 32 +OTHER_GUILD = 9911 +FALLBACK_REDIRECT = "https://console.example/api/auth/callback" + + +def signin_clients( + factory: async_sessionmaker[AsyncSession], + environment: OAuthClient | None = None, + key_id: str = "master-1", +) -> GuildSignInClients: + return GuildSignInClients( + environment or StubOAuth(), + GuildOAuthClientStore(factory, KeyWrapper(MASTER, key_id)), + redirect_uri=FALLBACK_REDIRECT, + ) + + +class StubOAuth: + """The environment-configured client, recognisable by what it builds.""" + + def authorize_url(self, state: str) -> str: + return f"https://environment.example/oauth/authorize?state={state}" + + async def identity_from_code(self, code: str) -> ExternalIdentity: # pragma: no cover + del code + raise AssertionError("the environment client is not exchanged with in these tests") + + +async def register( + factory: async_sessionmaker[AsyncSession], + guild_id: int, + slug: str, + *, + secret: str | None = "hunter2", + provider: str = "outline", + redirect_uri: str | None = None, + key_id: str = "master-1", +) -> GuildOAuthClientStore: + store = GuildOAuthClientStore(factory, KeyWrapper(MASTER, key_id)) + await store.save( + guild_id, + slug=slug, + provider=provider, + base_url="https://outline.acme.example", + client_id="acme-client", + redirect_uri=redirect_uri, + now=T0, + ) + if secret is not None: + await store.set_client_secret(guild_id, secret, T0) + return store + + +async def test_a_deployment_that_configured_nothing_signs_in_exactly_as_before( + factory: async_sessionmaker[AsyncSession], +) -> None: + """The property 2.2 requires and a test pins: no guild, no change. + + A sign-in with no guild resolves the environment-configured client, + and it does so without the per-guild table having anything in it. + """ + clients = signin_clients(factory) + resolved = await clients.for_guild(None) + assert resolved is not None + assert resolved.authorize_url("s").startswith("https://environment.example/") + + +async def test_a_registered_slug_resolves_to_that_guilds_client( + factory: async_sessionmaker[AsyncSession], +) -> None: + await register(factory, GUILD, "acme") + found = await signin_clients(factory).for_slug("acme") + assert found is not None + assert found.guild_id == GUILD + assert found.client.authorize_url("s").startswith("https://outline.acme.example/") + + +async def test_the_guild_a_state_names_selects_the_same_client_again( + factory: async_sessionmaker[AsyncSession], +) -> None: + """The callback has no URL to read a slug from; it has the state.""" + await register(factory, GUILD, "acme") + resolved = await signin_clients(factory).for_guild(GUILD) + assert resolved is not None + assert resolved.authorize_url("s").startswith("https://outline.acme.example/") + + +async def test_an_unknown_slug_and_a_half_configured_one_answer_alike( + factory: async_sessionmaker[AsyncSession], +) -> None: + """The disclosure 2.2 chose this whole design to avoid. + + A guild registered but never given a secret must be indistinguishable + from a guild that does not exist -- otherwise anybody with a browser + can walk a list of names and learn which organisations use this + service, signed in or not. + """ + await register(factory, GUILD, "acme", secret=None) + clients = signin_clients(factory) + assert await clients.for_slug("acme") is None + assert await clients.for_slug("no-such-guild") is None + + +async def test_a_provider_this_deployment_cannot_exchange_with_resolves_to_nothing( + factory: async_sessionmaker[AsyncSession], +) -> None: + """A registration nothing here can complete is not half a sign-in.""" + await register(factory, GUILD, "acme", provider="confluence") + assert await signin_clients(factory).for_slug("acme") is None + + +async def test_a_slug_the_write_path_would_have_refused_is_never_looked_up( + factory: async_sessionmaker[AsyncSession], +) -> None: + """One rule, in one pure function, read from both ends. + + A slug `is_valid_slug` refuses cannot be in the table, so the lookup + is a query that can only miss -- and skipping it keeps the shape rule + and the storage from ever disagreeing about what a slug is. + """ + clients = signin_clients(factory) + for refused in ("API", "acme/../x", "1289374650912837465", "api"): + assert await clients.for_slug(refused) is None + + +async def test_a_secret_this_process_cannot_unwrap_resolves_to_nothing( + factory: async_sessionmaker[AsyncSession], +) -> None: + """A rotation that was not carried through is a configuration error. + + It must not become a sign-in that half-works, and it must not be + distinguishable from an unknown slug from outside -- the operator + learns about it from the log line, which is where it belongs. + """ + await register(factory, GUILD, "acme", key_id="master-1") + assert await signin_clients(factory, key_id="master-2").for_slug("acme") is None + + +async def test_a_guild_that_named_no_callback_gets_this_deployments_own( + factory: async_sessionmaker[AsyncSession], +) -> None: + """Null `redirect_uri` means "the one this deployment is configured + with", which is what nearly every guild will want.""" + await register(factory, GUILD, "acme") + found = await signin_clients(factory).for_slug("acme") + assert found is not None + assert FALLBACK_REDIRECT in unquote(found.client.authorize_url("s")) + + +async def test_a_guild_that_named_its_own_callback_keeps_it( + factory: async_sessionmaker[AsyncSession], +) -> None: + await register(factory, GUILD, "acme", redirect_uri="https://acme.example/back") + found = await signin_clients(factory).for_slug("acme") + assert found is not None + assert "https://acme.example/back" in unquote(found.client.authorize_url("s")) + + +# --------------------------------------------------------------------------- +# Editing a guild's registration, which only its administrators may do +# --------------------------------------------------------------------------- + + +async def admin_of(factory: async_sessionmaker[AsyncSession], guild_id: int, member: int) -> None: + async with factory() as session: + session.add(AdminMember(guild_id=guild_id, discord_user_id=member, synced_at=T0)) + await session.commit() + + +def crud(factory: async_sessionmaker[AsyncSession]) -> ConsoleGuildOAuthClients: + return ConsoleGuildOAuthClients( + GuildOAuthClientStore(factory, KeyWrapper(MASTER, "master-1")), + AdminMemberStore(factory), + ) + + +async def test_an_administrator_registers_their_guilds_client( + factory: async_sessionmaker[AsyncSession], +) -> None: + await admin_of(factory, GUILD, CARA) + saved = await crud(factory).save( + GUILD, + requested_by=CARA, + slug="acme", + provider="outline", + base_url="https://outline.acme.example", + client_id="acme-client", + redirect_uri=None, + now=T0, + ) + assert saved is not None + assert saved.slug == "acme" + assert saved.has_secret is False + + +async def test_an_administrator_of_another_guild_may_not_touch_this_registration( + factory: async_sessionmaker[AsyncSession], +) -> None: + """Same rule the settings endpoints have: per guild, not in general.""" + await admin_of(factory, OTHER_GUILD, CARA) + clients = crud(factory) + assert ( + await clients.save( + GUILD, + requested_by=CARA, + slug="acme", + provider="outline", + base_url="https://outline.acme.example", + client_id="acme-client", + redirect_uri=None, + now=T0, + ) + is None + ) + assert await clients.for_guild(GUILD, requested_by=CARA) is None + assert await clients.delete(GUILD, requested_by=CARA) is False + + +async def test_a_slug_another_guild_holds_is_refused_rather_than_stolen( + factory: async_sessionmaker[AsyncSession], +) -> None: + """The unique constraint answers, not a check-then-write. + + Checking first is a race between two administrators claiming one name + in the same second; the constraint is not. + """ + await admin_of(factory, GUILD, CARA) + await admin_of(factory, OTHER_GUILD, CARA) + clients = crud(factory) + await clients.save( + GUILD, + requested_by=CARA, + slug="acme", + provider="outline", + base_url="https://outline.acme.example", + client_id="acme-client", + redirect_uri=None, + now=T0, + ) + with pytest.raises(SlugUnavailable): + await clients.save( + OTHER_GUILD, + requested_by=CARA, + slug="acme", + provider="outline", + base_url="https://outline.other.example", + client_id="other-client", + redirect_uri=None, + now=T0, + ) + + +async def test_the_secret_goes_in_and_only_has_secret_comes_out( + factory: async_sessionmaker[AsyncSession], +) -> None: + """There is no method on this adapter that returns a secret. + + Reading one is `GuildSignInClients`'s alone -- the object the sign-in + path holds and no route does. + """ + await admin_of(factory, GUILD, CARA) + clients = crud(factory) + await clients.save( + GUILD, + requested_by=CARA, + slug="acme", + provider="outline", + base_url="https://outline.acme.example", + client_id="acme-client", + redirect_uri=None, + now=T0, + ) + + stored = await clients.set_secret(GUILD, "hunter2", requested_by=CARA, now=T0) + + assert stored is not None + assert stored.has_secret is True + assert "hunter2" not in repr(stored) + assert not any("hunter2" in str(getattr(stored, field)) for field in stored.__slots__) + + +async def test_clearing_the_secret_stops_that_guilds_link_working( + factory: async_sessionmaker[AsyncSession], +) -> None: + """An administrator whose secret leaked wants it gone now. + + Re-registering the whole client to achieve that would free the slug + in between, so the secret has its own write -- and a sign-in through + the link answers exactly as an unknown slug does the moment it is + cleared. + """ + await admin_of(factory, GUILD, CARA) + await register(factory, GUILD, "acme") + assert await signin_clients(factory).for_slug("acme") is not None + + cleared = await crud(factory).set_secret(GUILD, None, requested_by=CARA, now=T0) + + assert cleared is not None + assert cleared.has_secret is False + assert await signin_clients(factory).for_slug("acme") is None + + +async def test_setting_a_secret_on_a_guild_with_no_registration_says_so( + factory: async_sessionmaker[AsyncSession], +) -> None: + await admin_of(factory, GUILD, CARA) + assert await crud(factory).set_secret(GUILD, "hunter2", requested_by=CARA, now=T0) is None + + +async def test_deleting_a_registration_frees_its_slug( + factory: async_sessionmaker[AsyncSession], +) -> None: + await admin_of(factory, GUILD, CARA) + await register(factory, GUILD, "acme") + clients = crud(factory) + + assert await clients.delete(GUILD, requested_by=CARA) is True + assert await clients.delete(GUILD, requested_by=CARA) is False + assert await signin_clients(factory).for_slug("acme") is None diff --git a/tests/console/test_auth_routes.py b/tests/console/test_auth_routes.py index 537c9f5..557a29a 100644 --- a/tests/console/test_auth_routes.py +++ b/tests/console/test_auth_routes.py @@ -30,6 +30,7 @@ FakeLinks, FakeOAuth, FakeReads, + FakeSignInClients, FakeStates, FakeTracks, build_test_api, @@ -330,3 +331,235 @@ async def test_an_expired_session_is_unauthorised_rather_than_a_server_error( expired = SessionCookie(SECRET, timedelta(seconds=-1)).issue(SignedSession(ANNA), now=T0) client.session.cookie_jar.update_cookies({SESSION_COOKIE: expired}) assert (await client.get(path)).status == 401 + + +# --------------------------------------------------------------------------- +# Signing in through a guild's own link +# --------------------------------------------------------------------------- + + +ACME, OTHER = 4711, 9911 + + +def guild_app( + guilds: dict[str, tuple[int, FakeOAuth]] | None = None, + environment: FakeOAuth | None = None, + states: FakeStates | None = None, + links: FakeLinks | None = None, +) -> web.Application: + """An application whose sign-in can resolve a guild's own client.""" + return build_test_api( + clients=FakeSignInClients(environment=environment or FakeOAuth(), guilds=guilds or {}), + states=states or FakeStates(), + links=links or FakeLinks(), + sessions=SessionCookie(SECRET, timedelta(hours=12)), + now=now_at(), + ) + + +def acme_oauth() -> FakeOAuth: + """A client recognisable in a `Location` header as not the default one.""" + return FakeOAuth(base_url="https://outline.acme.example") + + +async def test_a_guilds_link_redirects_to_that_guilds_provider( + aiohttp_client: AiohttpClientFactory, +) -> None: + """The whole of 2.2 in one request. + + `/api/auth/login` takes no parameters and reads no cookie, so the + slug in the URL is the only thing that can name a guild before the + round trip begins -- and here it does. + """ + client = await aiohttp_client(guild_app(guilds={"acme": (ACME, acme_oauth())})) + + response = await client.get("/api/auth/login?guild=acme", allow_redirects=False) + + assert response.status == 302 + assert response.headers["Location"].startswith("https://outline.acme.example/") + + +async def test_a_guilds_sign_in_issues_a_state_that_names_the_guild( + aiohttp_client: AiohttpClientFactory, +) -> None: + """The state is what selects the client for the code exchange. + + Nothing else survives the round trip: the callback has no session to + read a guild from, which is the problem the guild-specific link + exists to solve. + """ + states = FakeStates() + client = await aiohttp_client(guild_app(guilds={"acme": (ACME, acme_oauth())}, states=states)) + + await client.get("/api/auth/login?guild=acme", allow_redirects=False) + + assert states.issued_for[states.issued[0]] == ACME + + +async def test_a_sign_in_with_no_guild_behaves_exactly_as_it_did( + aiohttp_client: AiohttpClientFactory, +) -> None: + """The compatibility promise 2.2 makes, pinned. + + A deployment that never configures a per-guild client must behave + identically to v0.15.0: the environment-configured client, and a + state that names no guild. + """ + states, environment = FakeStates(), FakeOAuth() + client = await aiohttp_client( + guild_app(guilds={"acme": (ACME, acme_oauth())}, environment=environment, states=states) + ) + + response = await client.get("/api/auth/login", allow_redirects=False) + + assert response.status == 302 + assert response.headers["Location"].startswith("https://outline.example/oauth/authorize") + assert environment.authorize_calls == states.issued + assert states.issued_for[states.issued[0]] is None + + +async def test_an_unknown_slug_answers_exactly_as_a_misconfigured_one( + aiohttp_client: AiohttpClientFactory, +) -> None: + """The property the owner chose this design to keep. + + A slug that names nothing and a slug that names a guild whose client + cannot complete a sign-in must be one answer, byte for byte. + Otherwise anybody with a browser can walk a list of names and learn + which organisations use this service -- which is precisely what the + rejected alternative, a public list of guilds, would have done. + """ + # `half-configured` is registered to no usable client, exactly as a + # guild whose secret has not been supplied yet resolves. + client = await aiohttp_client(guild_app(guilds={"acme": (ACME, acme_oauth())})) + + unknown = await client.get("/api/auth/login?guild=no-such-guild", allow_redirects=False) + misconfigured = await client.get("/api/auth/login?guild=half-configured", allow_redirects=False) + + assert unknown.status == misconfigured.status == 404 + assert await unknown.json() == await misconfigured.json() + assert "location" not in {header.lower() for header in unknown.headers} + + +@pytest.mark.parametrize( + "slug", ["API", "acme%2f..%2fx", "1289374650912837465", "api", "a", "acme--x"] +) +async def test_a_slug_that_is_not_a_slug_answers_the_same_404( + aiohttp_client: AiohttpClientFactory, slug: str +) -> None: + """A refused shape must not be a different reply from a refused name. + + A 400 for "that is not a valid slug" and a 404 for "no such slug" + would together tell an attacker which of the names they tried were + well-formed but unregistered, which is half the enumeration back. + """ + client = await aiohttp_client(guild_app(guilds={"acme": (ACME, acme_oauth())})) + response = await client.get(f"/api/auth/login?guild={slug}", allow_redirects=False) + assert response.status == 404 + assert await response.json() == {"error": "no such sign-in link"} + + +async def test_the_refusal_reflects_nothing_of_what_was_asked_for( + aiohttp_client: AiohttpClientFactory, +) -> None: + """The rule the whole of `sturnus.console.app` follows. + + This endpoint is unauthenticated and takes a string straight out of a + URL; an error body that echoed it would be an XSS sink reachable by + anybody. + """ + client = await aiohttp_client(guild_app()) + response = await client.get( + "/api/auth/login?guild=%3Cscript%3Ealert(1)%3C/script%3E", allow_redirects=False + ) + assert response.status == 404 + assert "script" not in await response.text() + + +async def test_the_state_and_not_the_callback_selects_the_client( + aiohttp_client: AiohttpClientFactory, +) -> None: + """A code is exchanged against the client the sign-in began with. + + Two guilds are configured; the sign-in begins on one of them, and the + exchange must happen there. Nothing in the callback URL names a + guild, which is what stops a caller asking this process to spend one + guild's client secret on another guild's code. + """ + acme, other = acme_oauth(), acme_oauth() + acme.identity = ExternalIdentity(ANNA_OUTLINE, "Anna") + states = FakeStates() + client = await aiohttp_client( + guild_app(guilds={"acme": (ACME, acme), "other": (OTHER, other)}, states=states) + ) + await client.get("/api/auth/login?guild=acme", allow_redirects=False) + + response = await client.get( + f"/api/auth/callback?code=abc&state={states.issued[0]}", allow_redirects=False + ) + + assert response.status == 302 + assert acme.exchanges == ["abc"] + assert other.exchanges == [] + + +async def test_a_sign_in_with_no_guild_still_exchanges_against_the_environment( + aiohttp_client: AiohttpClientFactory, +) -> None: + environment, acme = FakeOAuth(), acme_oauth() + states = FakeStates() + client = await aiohttp_client( + guild_app(guilds={"acme": (ACME, acme)}, environment=environment, states=states) + ) + await client.get("/api/auth/login", allow_redirects=False) + + await client.get(f"/api/auth/callback?code=abc&state={states.issued[0]}", allow_redirects=False) + + assert environment.exchanges == ["abc"] + assert acme.exchanges == [] + + +async def test_a_callback_for_a_registration_that_went_away_mints_no_session( + aiohttp_client: AiohttpClientFactory, +) -> None: + """An administrator can delete a registration mid-login. + + The state is valid and there is nothing to exchange with, so it + answers as an unknown sign-in attempt does -- and above all it does + not fall back to the environment-configured client, which would sign + somebody in through a client their guild deliberately replaced. + """ + states = FakeStates() + clients = FakeSignInClients(guilds={"acme": (ACME, acme_oauth())}) + app_with_removal = build_test_api( + clients=clients, + states=states, + sessions=SessionCookie(SECRET, timedelta(hours=12)), + now=now_at(), + ) + client = await aiohttp_client(app_with_removal) + await client.get("/api/auth/login?guild=acme", allow_redirects=False) + clients.guilds.clear() + + response = await client.get( + f"/api/auth/callback?code=abc&state={states.issued[0]}", allow_redirects=False + ) + + assert response.status == 400 + assert SESSION_COOKIE not in response.cookies + + +async def test_an_empty_guild_parameter_is_not_the_same_as_no_guild( + aiohttp_client: AiohttpClientFactory, +) -> None: + """`?guild=` asked for a guild and named none of them. + + Treating it as "no guild" would sign somebody in against the + deployment's own client on a link that was meant to select a guild's + -- which is the one substitution this whole design exists to prevent. + It is refused like any other slug that resolves to nothing. + """ + client = await aiohttp_client(guild_app(guilds={"acme": (ACME, acme_oauth())})) + response = await client.get("/api/auth/login?guild=", allow_redirects=False) + assert response.status == 404 + assert await response.json() == {"error": "no such sign-in link"} diff --git a/tests/console/test_oauth_routes.py b/tests/console/test_oauth_routes.py new file mode 100644 index 0000000..4fefe70 --- /dev/null +++ b/tests/console/test_oauth_routes.py @@ -0,0 +1,527 @@ +"""Configuring a guild's own sign-in client, through the real routes. + +Three properties carry this file, and each of them is a decision from +§2.2 rather than an implementation detail: + +- **The secret never comes back.** Not from the endpoint that stored it, + not masked, not truncated. That is why this configuration is not a + `guild_config` key: the settings API renders every value it holds + straight back to whoever asks. +- **Every refusal is the same 404.** Not administering the guild, no such + guild and no client configured answer identically, because whether a + given guild has its own sign-in is the fact the guild-specific-link + design exists to keep undiscoverable. +- **Changing a guild's sign-in credential is audited.** It is the + credential that decides who gets a session at all, and the log line is + the only record that anybody changed it. +""" + +from __future__ import annotations + +import logging +from datetime import timedelta + +import pytest +from aiohttp import web +from aiohttp.test_utils import TestClient + +from sturnus.console.session import SessionCookie, SignedSession +from sturnus.observability.events import Event +from tests.console.conftest import ( + ANNA, + BEN, + GUILD, + SECRET, + T0, + AiohttpClientFactory, + FakeAdmins, + FakeGuildOAuthClients, + build_test_api, + now_at, +) + +SESSION_COOKIE = "sturnus_session" + +#: A second guild, administered by nobody in these tests unless a test +#: says so. Real-shaped, like `GUILD`. +OTHER_GUILD = 9911 + +REGISTRATION = { + "slug": "acme", + "provider": "outline", + "base_url": "https://outline.acme.example", + "client_id": "acme-client", + "redirect_uri": None, +} + + +def token(discord_user_id: int = ANNA) -> str: + return SessionCookie(SECRET, timedelta(hours=12)).issue(SignedSession(discord_user_id), now=T0) + + +def build(clients: FakeGuildOAuthClients | None = None) -> web.Application: + admins = FakeAdmins({ANNA}) + return build_test_api( + admins=admins, + oauth_clients=clients or FakeGuildOAuthClients(admins=admins), + sessions=SessionCookie(SECRET, timedelta(hours=12)), + now=now_at(), + ) + + +async def signed_in( + aiohttp_client: AiohttpClientFactory, + app: web.Application, + as_user: int = ANNA, +) -> TestClient[web.Request, web.Application]: + client = await aiohttp_client(app) + client.session.cookie_jar.update_cookies({SESSION_COOKIE: token(as_user)}) + return client + + +def url(guild_id: int | str = GUILD) -> str: + return f"/api/guilds/{guild_id}/oauth-client" + + +def secret_url(guild_id: int | str = GUILD) -> str: + return f"/api/guilds/{guild_id}/oauth-client/secret" + + +def _events(caplog: pytest.LogCaptureFixture, event: Event) -> list[logging.LogRecord]: + return [r for r in caplog.records if getattr(r, "sturnus_event", None) == str(event)] + + +def _fields(record: logging.LogRecord) -> dict[str, object]: + fields = getattr(record, "sturnus_fields", None) + assert isinstance(fields, dict) + return fields + + +# --------------------------------------------------------------------------- +# Nothing here is reachable without a session +# --------------------------------------------------------------------------- + + +async def test_every_route_refuses_a_request_with_no_session( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await aiohttp_client(build()) + assert (await client.get(url())).status == 401 + assert (await client.put(url(), json=REGISTRATION)).status == 401 + assert (await client.delete(url())).status == 401 + assert (await client.put(secret_url(), json={"client_secret": "x"})).status == 401 + assert (await client.delete(secret_url())).status == 401 + + +# --------------------------------------------------------------------------- +# Registering, and reading back what was registered +# --------------------------------------------------------------------------- + + +async def test_an_administrator_registers_their_guilds_sign_in( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build()) + + response = await client.put(url(), json=REGISTRATION) + + assert response.status == 200 + body = await response.json() + assert body["guild_id"] == str(GUILD) + assert body["oauth_client"] == { + "slug": "acme", + "provider": "outline", + "base_url": "https://outline.acme.example", + "client_id": "acme-client", + "redirect_uri": None, + "has_secret": False, + "created_at": T0.isoformat(), + "updated_at": T0.isoformat(), + } + + +async def test_the_guild_id_is_a_string_because_a_snowflake_is( + aiohttp_client: AiohttpClientFactory, +) -> None: + """A JSON number silently loses its last digits in JavaScript.""" + client = await signed_in(aiohttp_client, build()) + body = await (await client.put(url(), json=REGISTRATION)).json() + assert isinstance(body["guild_id"], str) + + +async def test_a_registration_reads_back_as_it_was_written( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build()) + await client.put(url(), json=REGISTRATION) + + response = await client.get(url()) + + assert response.status == 200 + assert (await response.json())["oauth_client"]["slug"] == "acme" + + +async def test_a_guild_may_name_its_own_callback( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build()) + body = await ( + await client.put(url(), json=REGISTRATION | {"redirect_uri": "https://acme.example/back"}) + ).json() + assert body["oauth_client"]["redirect_uri"] == "https://acme.example/back" + + +async def test_registering_again_does_not_disturb_the_secret( + aiohttp_client: AiohttpClientFactory, +) -> None: + """Registering the client and supplying its secret are two steps. + + An administrator correcting a base URL should not have to re-type a + secret they may no longer have a copy of. + """ + client = await signed_in(aiohttp_client, build()) + await client.put(url(), json=REGISTRATION) + await client.put(secret_url(), json={"client_secret": "hunter2"}) + + body = await ( + await client.put(url(), json=REGISTRATION | {"base_url": "https://moved.example"}) + ).json() + + assert body["oauth_client"]["has_secret"] is True + + +# --------------------------------------------------------------------------- +# The secret, which goes in and does not come out +# --------------------------------------------------------------------------- + + +async def test_the_stored_secret_is_never_in_any_response( + aiohttp_client: AiohttpClientFactory, +) -> None: + """Never send a secret back -- not even masked-but-recoverable (2.2). + + Checked against the raw text of every response the four endpoints + that could possibly hold one produce, rather than against a field + name, because a field nobody thought of is exactly how one would + escape. + """ + clients = FakeGuildOAuthClients(admins=FakeAdmins({ANNA})) + client = await signed_in(aiohttp_client, build(clients)) + await client.put(url(), json=REGISTRATION) + + stored = await client.put(secret_url(), json={"client_secret": "hunter2"}) + read = await client.get(url()) + written = await client.put(url(), json=REGISTRATION) + + assert clients.secrets[GUILD] == "hunter2" + for response in (stored, read, written): + assert "hunter2" not in await response.text() + + +async def test_storing_a_secret_says_only_that_there_is_one( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build()) + await client.put(url(), json=REGISTRATION) + + response = await client.put(secret_url(), json={"client_secret": "hunter2"}) + + assert response.status == 200 + assert (await response.json())["oauth_client"]["has_secret"] is True + + +async def test_the_secret_can_be_cleared_without_freeing_the_slug( + aiohttp_client: AiohttpClientFactory, +) -> None: + """An administrator whose secret leaked wants it gone now. + + Deleting the whole registration to achieve that would release the + slug, and somebody else could claim it in between. + """ + client = await signed_in(aiohttp_client, build()) + await client.put(url(), json=REGISTRATION) + await client.put(secret_url(), json={"client_secret": "hunter2"}) + + response = await client.delete(secret_url()) + + assert response.status == 200 + body = await response.json() + assert body["oauth_client"]["has_secret"] is False + assert body["oauth_client"]["slug"] == "acme" + + +@pytest.mark.parametrize("body", [{}, {"client_secret": ""}, {"client_secret": 7}, {"secret": "x"}]) +async def test_a_secret_that_is_not_a_secret_is_refused( + aiohttp_client: AiohttpClientFactory, body: dict[str, object] +) -> None: + client = await signed_in(aiohttp_client, build()) + await client.put(url(), json=REGISTRATION) + assert (await client.put(secret_url(), json=body)).status == 400 + + +async def test_a_secret_for_a_guild_with_no_registration_is_the_same_404( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build()) + response = await client.put(secret_url(), json={"client_secret": "hunter2"}) + assert response.status == 404 + assert await response.json() == {"error": "no sign-in configuration"} + + +# --------------------------------------------------------------------------- +# Who may see and change one, which is one answer for three questions +# --------------------------------------------------------------------------- + + +async def test_a_guild_you_do_not_administer_answers_as_one_that_does_not_exist( + aiohttp_client: AiohttpClientFactory, +) -> None: + """The enumeration property, from the authenticated side. + + An administrator of one guild is nobody in another, and the refusal + must not say which of "not yours" and "not configured" it was -- + those two answers together are a map of which guilds run their own + sign-in. + """ + clients = FakeGuildOAuthClients(admins=FakeAdmins({ANNA})) + client = await signed_in(aiohttp_client, build(clients)) + await client.put(url(), json=REGISTRATION) + + configured_but_not_theirs = await signed_in(aiohttp_client, build(clients), as_user=BEN) + theirs_but_unconfigured = await client.get(url(OTHER_GUILD)) + refused = await configured_but_not_theirs.get(url()) + + assert refused.status == theirs_but_unconfigured.status == 404 + assert await refused.json() == await theirs_but_unconfigured.json() + + +async def test_somebody_else_cannot_write_a_guilds_sign_in( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build(), as_user=BEN) + assert (await client.put(url(), json=REGISTRATION)).status == 404 + assert (await client.delete(url())).status == 404 + assert (await client.put(secret_url(), json={"client_secret": "x"})).status == 404 + + +async def test_a_guild_id_that_is_not_a_number_is_the_same_404( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build()) + response = await client.get(url("not-a-guild")) + assert response.status == 404 + assert await response.json() == {"error": "no sign-in configuration"} + + +# --------------------------------------------------------------------------- +# What may be written, decided in the domain and refused here +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "slug", + ["Acme", "acme industries", "acme/x", "-acme", "acme--x", "1289374650912837465", "ab"], +) +async def test_a_slug_that_is_not_a_slug_is_refused( + aiohttp_client: AiohttpClientFactory, slug: str +) -> None: + """The shape is the domain's decision; this endpoint reports it. + + A slug selects a credential from a public URL, so its shape is the + one rule the sign-in path and the write path must agree about. + """ + client = await signed_in(aiohttp_client, build()) + assert (await client.put(url(), json=REGISTRATION | {"slug": slug})).status == 400 + + +async def test_a_name_this_deployment_serves_itself_is_not_available( + aiohttp_client: AiohttpClientFactory, +) -> None: + """And it answers exactly as a name another guild already holds. + + Registration needs a uniqueness answer or an administrator cannot be + told why their choice failed -- so the two refusals are made one, and + the reply says nothing about which it was. + """ + clients = FakeGuildOAuthClients(admins=FakeAdmins({ANNA}, {OTHER_GUILD: {ANNA}})) + client = await signed_in(aiohttp_client, build(clients)) + await client.put(url(OTHER_GUILD), json=REGISTRATION | {"slug": "taken"}) + + reserved = await client.put(url(), json=REGISTRATION | {"slug": "api"}) + claimed = await client.put(url(), json=REGISTRATION | {"slug": "taken"}) + + assert reserved.status == claimed.status == 409 + assert await reserved.json() == await claimed.json() + + +@pytest.mark.parametrize( + "base_url", + [ + "http://outline.acme.example", + "https://outline.acme.example@evil.example/", + "javascript:alert(1)", + "outline.acme.example", + "", + 7, + ], +) +async def test_a_base_url_other_browsers_follow_must_be_an_https_address( + aiohttp_client: AiohttpClientFactory, base_url: object +) -> None: + client = await signed_in(aiohttp_client, build()) + assert (await client.put(url(), json=REGISTRATION | {"base_url": base_url})).status == 400 + + +async def test_a_redirect_uri_is_held_to_the_same_rule( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build()) + refused = await client.put(url(), json=REGISTRATION | {"redirect_uri": "http://acme.example"}) + assert refused.status == 400 + + +async def test_a_provider_this_deployment_cannot_exchange_with_is_refused( + aiohttp_client: AiohttpClientFactory, +) -> None: + """Storing one would produce a guild whose link is silently broken. + + A registration nothing here can complete resolves to nothing at + sign-in time, which is indistinguishable from an unknown slug -- so + the administrator would never learn what went wrong. + """ + client = await signed_in(aiohttp_client, build()) + assert (await client.put(url(), json=REGISTRATION | {"provider": "confluence"})).status == 400 + + +@pytest.mark.parametrize("body", [{"client_id": ""}, {"client_id": 7}]) +async def test_a_client_id_that_is_not_one_is_refused( + aiohttp_client: AiohttpClientFactory, body: dict[str, object] +) -> None: + client = await signed_in(aiohttp_client, build()) + assert (await client.put(url(), json=REGISTRATION | body)).status == 400 + + +async def test_a_body_that_is_not_an_object_is_refused( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build()) + assert (await client.put(url(), data="not json")).status == 400 + assert (await client.put(url(), json=["acme"])).status == 400 + + +async def test_no_refusal_reflects_what_it_refused( + aiohttp_client: AiohttpClientFactory, +) -> None: + """One of the values these handlers parse is a client secret. + + An endpoint that echoed what it refused would have echoed one the + first time somebody sent a malformed body. + """ + client = await signed_in(aiohttp_client, build()) + response = await client.put(url(), json=REGISTRATION | {"slug": ""}) + assert response.status == 400 + assert "script" not in await response.text() + + +# --------------------------------------------------------------------------- +# Removing one +# --------------------------------------------------------------------------- + + +async def test_removing_a_registration_frees_it( + aiohttp_client: AiohttpClientFactory, +) -> None: + client = await signed_in(aiohttp_client, build()) + await client.put(url(), json=REGISTRATION) + + assert (await client.delete(url())).status == 204 + assert (await client.get(url())).status == 404 + + +async def test_removing_one_twice_is_not_the_same_reply( + aiohttp_client: AiohttpClientFactory, +) -> None: + """An administrator who clicked twice is told which click did it. + + "Already gone" and "removed" are different answers to the same + gesture, and collapsing them would hide a registration that somebody + else removed in between. + """ + client = await signed_in(aiohttp_client, build()) + await client.put(url(), json=REGISTRATION) + await client.delete(url()) + assert (await client.delete(url())).status == 404 + + +# --------------------------------------------------------------------------- +# The audit line +# --------------------------------------------------------------------------- + + +async def test_every_change_to_a_guilds_sign_in_is_audited( + aiohttp_client: AiohttpClientFactory, caplog: pytest.LogCaptureFixture +) -> None: + """The credential that decides who gets a session at all. + + WARNING, a level above the settings writes next door: whoever + controls the identity provider a slug points at controls who this + console believes is signing in, and this line is the only record that + anybody changed it. + """ + client = await signed_in(aiohttp_client, build()) + + with caplog.at_level(logging.INFO): + await client.put(url(), json=REGISTRATION) + await client.put(secret_url(), json={"client_secret": "hunter2"}) + await client.delete(secret_url()) + await client.delete(url()) + + lines = _events(caplog, Event.CONSOLE_OAUTH_CLIENT_CHANGED) + assert [_fields(line)["outcome"] for line in lines] == [ + "registered", + "secret_set", + "secret_cleared", + "removed", + ] + assert {line.levelno for line in lines} == {logging.WARNING} + for line in lines: + assert _fields(line)["guild_id"] == GUILD + assert _fields(line)["requested_by"] == ANNA + + +async def test_the_audit_line_carries_neither_half_of_the_credential( + aiohttp_client: AiohttpClientFactory, caplog: pytest.LogCaptureFixture +) -> None: + """The secret is obvious. The client id is left off deliberately. + + It is one half of a pair, and a retained, Grafana-readable log is not + the place to narrow the other half's blast radius by one guess. + """ + client = await signed_in(aiohttp_client, build()) + + with caplog.at_level(logging.INFO): + await client.put(url(), json=REGISTRATION) + await client.put(secret_url(), json={"client_secret": "hunter2"}) + + for line in _events(caplog, Event.CONSOLE_OAUTH_CLIENT_CHANGED): + rendered = str(_fields(line)) + line.getMessage() + assert "hunter2" not in rendered + assert "acme-client" not in rendered + + +async def test_a_refused_change_is_not_an_audit_line( + aiohttp_client: AiohttpClientFactory, caplog: pytest.LogCaptureFixture +) -> None: + """Nothing changed, so there is nothing to record. + + An audit line for every refused attempt would also be a way for + anybody with a session to write into an operator's log by guessing + guild ids. + """ + client = await signed_in(aiohttp_client, build(), as_user=BEN) + + with caplog.at_level(logging.INFO): + await client.put(url(), json=REGISTRATION) + await client.delete(url()) + + assert _events(caplog, Event.CONSOLE_OAUTH_CLIENT_CHANGED) == [] diff --git a/tests/domain/test_oauth_clients.py b/tests/domain/test_oauth_clients.py new file mode 100644 index 0000000..8436ee1 --- /dev/null +++ b/tests/domain/test_oauth_clients.py @@ -0,0 +1,193 @@ +"""What may be written in the URL that chooses which OAuth client signs you in. + +A slug is not a label. It is a public path segment that **selects a +credential**: `/api/auth/login?guild=acme` decides which client id and +which client secret complete the round trip. So the shape is decided +here, in a pure function a test can reach without a database, and +everything outside it is refused rather than normalised -- a slug that +was quietly rewritten before being stored is a slug the administrator +does not recognise in the link they were told to distribute. + +Two properties the shape exists for: + +- **It cannot be confused with a route.** The console serves the link at + `/g/{slug}/sign-in`, and a deployment that later serves anything else + under a name a guild has already claimed has a collision it cannot + resolve. +- **It cannot be confused with a guild id.** A snowflake is digits, so a + slug must not be. +""" + +from __future__ import annotations + +import pytest + +from sturnus.domain.oauth_clients import ( + MAX_SLUG_LENGTH, + MIN_SLUG_LENGTH, + RESERVED_SLUGS, + is_provider_url, + is_valid_slug, +) + + +@pytest.mark.parametrize( + "slug", + [ + "acme", + "acme-industries", + "onelitefeather", + "team-42", + "a" * MAX_SLUG_LENGTH, + "a" * MIN_SLUG_LENGTH, + ], +) +def test_an_ordinary_name_is_a_slug(slug: str) -> None: + assert is_valid_slug(slug) is True + + +@pytest.mark.parametrize( + "slug", + [ + "", + "a" * (MIN_SLUG_LENGTH - 1), + "a" * (MAX_SLUG_LENGTH + 1), + ], +) +def test_a_slug_is_long_enough_to_name_something_and_short_enough_for_a_url(slug: str) -> None: + assert is_valid_slug(slug) is False + + +@pytest.mark.parametrize( + "slug", + [ + "Acme", + "ACME", + "acme industries", + "acme_industries", + "acme.industries", + "acme/sign-in", + "acme%2f", + "acme?guild=other", + "acme#fragment", + "acme\n", + "acmé", + "../acme", + ], +) +def test_anything_that_would_have_to_be_escaped_is_not_a_slug(slug: str) -> None: + """A slug goes into a URL unencoded, so it may only be URL-safe text. + + Refused rather than percent-encoded: a slug the administrator has to + escape before pasting it into a chat message is a link nobody can + read back to the person who typed it. + """ + assert is_valid_slug(slug) is False + + +@pytest.mark.parametrize("slug", ["-acme", "acme-", "acme--industries", "--"]) +def test_a_hyphen_separates_words_and_does_not_start_or_end_one(slug: str) -> None: + assert is_valid_slug(slug) is False + + +@pytest.mark.parametrize("slug", ["1289374650912837465", "42", "007"]) +def test_a_slug_cannot_be_mistaken_for_a_guild_id(slug: str) -> None: + """A snowflake is digits, and a link is read by people. + + `/g/1289374650912837465/sign-in` and a guild id in a URL are the same + string to a reader, and the two select different things. Requiring a + letter first makes the two unconfusable by construction. + """ + assert is_valid_slug(slug) is False + + +@pytest.mark.parametrize("slug", sorted(RESERVED_SLUGS)) +def test_a_slug_may_not_be_a_name_this_deployment_already_serves(slug: str) -> None: + """`/g/api/sign-in` is a link, and `/api/...` is the whole API. + + Reserving them costs a guild one candidate name and buys the + certainty that no route ever added to this deployment can be shadowed + by a name a guild already claimed. + """ + assert is_valid_slug(slug) is False + + +def test_every_reserved_name_would_otherwise_have_been_a_slug() -> None: + """The reservation list is only doing work while its entries are legal. + + A reserved name that the shape rules already refuse is a line nobody + would notice going stale -- so the list is held to naming things that + are refused *because they are reserved*, not by accident. + """ + for reserved in RESERVED_SLUGS: + assert _matches_the_shape(reserved), reserved + + +def _matches_the_shape(slug: str) -> bool: + """The shape rules alone, with the reservation lifted.""" + from sturnus.domain.oauth_clients import has_slug_shape + + return has_slug_shape(slug) + + +# --------------------------------------------------------------------------- +# The two URLs a guild registers, which other people's browsers follow +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "url", + [ + "https://outline.example", + "https://outline.example/", + "https://wiki.example/outline", + "https://outline.example:8443/outline", + ], +) +def test_an_ordinary_provider_address_is_accepted(url: str) -> None: + assert is_provider_url(url) is True + + +@pytest.mark.parametrize( + "url", + [ + "http://outline.example", + "ftp://outline.example", + "javascript:alert(1)", + "data:text/html,hi", + "//outline.example", + "outline.example", + "", + ], +) +def test_only_https_carries_an_authorization_code(url: str) -> None: + """The code and the consent step both travel over this address. + + A guild that could register `http://` would be a guild whose members + hand their authorization codes to whoever is on the path. + """ + assert is_provider_url(url) is False + + +def test_a_url_that_reads_as_one_host_and_resolves_to_another_is_refused() -> None: + """The one form where parsing correctly is a security property. + + `https://outline.example@evil.example/` names `evil.example`, and an + administrator reviewing the value in a form reads the first host. + """ + assert is_provider_url("https://outline.example@evil.example/") is False + + +@pytest.mark.parametrize( + "url", + [ + "https://outline.example/?client_id=stolen", + "https://outline.example/#fragment", + "https://outline.example/ ", + " https://outline.example/", + "https://outline\n.example/", + ], +) +def test_a_base_url_carries_no_query_no_fragment_and_no_whitespace(url: str) -> None: + """`authorize_url` builds the query; a second one would collide with it.""" + assert is_provider_url(url) is False diff --git a/tests/infrastructure/test_guild_oauth.py b/tests/infrastructure/test_guild_oauth.py index 6f0a387..9a3eb91 100644 --- a/tests/infrastructure/test_guild_oauth.py +++ b/tests/infrastructure/test_guild_oauth.py @@ -227,3 +227,62 @@ async def test_deleting_a_client_frees_its_slug(store: GuildOAuthClientStore) -> assert await store.delete(GUILD) is False await save(store, OTHER_GUILD, "acme") assert await store.by_slug("acme") is not None + + +async def test_a_secret_moved_the_other_way_between_guilds_does_not_decrypt_either( + store: GuildOAuthClientStore, sessions: async_sessionmaker[AsyncSession] +) -> None: + """The binding is symmetric, and a test that checked one direction + would pass against an implementation that bound only the row it + happened to read first. + """ + await save(store, GUILD, "acme") + await save(store, OTHER_GUILD, "other") + await store.set_client_secret(OTHER_GUILD, "hunter2", T0) + + async with sessions() as session: + stolen = await session.scalar( + select(GuildOAuthClient.wrapped_client_secret).where( + GuildOAuthClient.guild_id == OTHER_GUILD + ) + ) + await session.execute( + update(GuildOAuthClient) + .where(GuildOAuthClient.guild_id == GUILD) + .values(wrapped_client_secret=stolen, encryption_key_id="master-1") + ) + await session.commit() + + with pytest.raises(InvalidTag): + await store.client_secret_for(GUILD) + + +async def test_an_oauth_secret_does_not_decrypt_as_an_export_secret( + store: GuildOAuthClientStore, sessions: async_sessionmaker[AsyncSession] +) -> None: + """The purpose binding, in the direction the other test does not take. + + The same guild owns both rows, so binding to the guild alone would + leave a client secret usable as a Confluence token and the other way + round. It is the client secret's turn to be the one that was moved. + """ + targets = ExportTargetStore(sessions, KeyWrapper(MASTER, "master-1")) + target_id = await targets.save( + GUILD, format="confluence", name="Wiki", target="ENG", config={}, now=T0 + ) + await save(store, GUILD, "acme") + await store.set_client_secret(GUILD, "hunter2", T0) + + async with sessions() as session: + stolen = await session.scalar( + select(GuildOAuthClient.wrapped_client_secret).where(GuildOAuthClient.guild_id == GUILD) + ) + await session.execute( + update(GuildExportTarget) + .where(GuildExportTarget.id == target_id) + .values(wrapped_secret=stolen, encryption_key_id="master-1") + ) + await session.commit() + + with pytest.raises(InvalidTag): + await targets.secret_for(GUILD, target_id)