From 57ab1825d7d70c82363d04f436ee6d4bf0425177 Mon Sep 17 00:00:00 2001 From: Adam <65679285+adamkoot@users.noreply.github.com> Date: Thu, 20 Aug 2026 10:51:20 +0200 Subject: [PATCH 01/11] chore(lore-0189): activate task --- ...gibility-gate-discord-membership-and-account-age.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) rename lore/1-tasks/{backlog => active}/0189_FEATURE_eligibility-gate-discord-membership-and-account-age.md (95%) diff --git a/lore/1-tasks/backlog/0189_FEATURE_eligibility-gate-discord-membership-and-account-age.md b/lore/1-tasks/active/0189_FEATURE_eligibility-gate-discord-membership-and-account-age.md similarity index 95% rename from lore/1-tasks/backlog/0189_FEATURE_eligibility-gate-discord-membership-and-account-age.md rename to lore/1-tasks/active/0189_FEATURE_eligibility-gate-discord-membership-and-account-age.md index a61ecae8..1ff041bf 100644 --- a/lore/1-tasks/backlog/0189_FEATURE_eligibility-gate-discord-membership-and-account-age.md +++ b/lore/1-tasks/active/0189_FEATURE_eligibility-gate-discord-membership-and-account-age.md @@ -2,7 +2,7 @@ id: "0189" title: "Eligibility gate — Stellar Discord membership and minimum account age before a key is issued" type: FEATURE -status: backlog +status: active related_adr: ["0010"] related_tasks: ["0183", "0156", "0159", "0179", "0180", "0186", "0187", "0191", "0193"] tags: [layer-backend, priority-high, effort-medium, milestone-M3, epic-self-service-onboarding, discord, auth, abuse-prevention, spike, slice-6] @@ -21,6 +21,14 @@ history: items 1–5. Those five measurements were a task-shaped blocker in front of the entire epic; they are actually the first hour of this one task, and nothing before this slice depends on them. + - date: 2026-08-20 + status: active + who: akot + note: > + Activated. Branch cut from [[0188]]'s (not yet merged; this slice edits + the same portal files). Step 0's five measurements remain operator-owned + prerequisites — the code is written to the documented safe rules and the + result tables stay empty until they are run. --- # Eligibility gate — membership and account age From b027c2a0befe3716e4afcc39cad33badbd507eee Mon Sep 17 00:00:00 2001 From: Adam <65679285+adamkoot@users.noreply.github.com> Date: Thu, 20 Aug 2026 11:28:19 +0200 Subject: [PATCH 02/11] =?UTF-8?q?feat(lore-0189):=20eligibility=20gate=20?= =?UTF-8?q?=E2=80=94=20membership=20+=20account=20age=20before=20a=20key?= =?UTF-8?q?=20is=20issued?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue path moves behind a fresh OAuth round-trip (action=issue in the signed state): the callback checks Stellar Discord membership (GET /users/@me/guilds/{guild}/member with the just-exchanged user token) and the snowflake-derived account age against two operator-seeded SSM parameters, and only then runs 0187's reconciler. Three outcomes, not two: only Discord's own 10007/10004 on a 404 is 'not a member'; 401/403/429/5xx, unrecognised shapes and an absent pending field refuse without accusation. The /key route goes fully read-only — a session cookie alone can cause zero control-plane writes — and answers no_key when there is nothing to reveal. Scope becomes exactly 'identify guilds.members.read', compared as a set. --- packages/prices-api/src/bin/serve.rs | 13 + packages/prices-api/src/config.rs | 110 +++ packages/prices-api/src/main.rs | 10 + .../prices-api/src/portal/auth/discord.rs | 329 ++++++- packages/prices-api/src/portal/auth/issue.rs | 310 +++++++ packages/prices-api/src/portal/auth/mod.rs | 85 +- .../prices-api/src/portal/auth/state_token.rs | 91 +- packages/prices-api/src/portal/eligibility.rs | 456 +++++++++ packages/prices-api/src/portal/keys/mod.rs | 322 ++++--- packages/prices-api/src/portal/mod.rs | 58 +- packages/prices-api/tests/auth.rs | 1 + .../prices-api/tests/common/mock_discord.rs | 248 +++++ packages/prices-api/tests/common/mod.rs | 1 + packages/prices-api/tests/endpoints_it.rs | 1 + packages/prices-api/tests/health.rs | 1 + packages/prices-api/tests/list_it.rs | 1 + packages/prices-api/tests/ohlcv_it.rs | 1 + packages/prices-api/tests/openapi.rs | 1 + packages/prices-api/tests/portal.rs | 1 + packages/prices-api/tests/portal_auth.rs | 234 ++--- packages/prices-api/tests/portal_issue.rs | 871 ++++++++++++++++++ packages/prices-api/tests/portal_keys.rs | 683 ++++---------- .../prices-api/tests/portal_keys/harness.rs | 111 ++- packages/prices-api/tests/portal_keys_logs.rs | 34 +- packages/prices-api/tests/portal_usage.rs | 21 +- packages/prices-api/tests/price.rs | 1 + packages/prices-api/tests/price_it.rs | 1 + 27 files changed, 3134 insertions(+), 862 deletions(-) create mode 100644 packages/prices-api/src/portal/auth/issue.rs create mode 100644 packages/prices-api/src/portal/eligibility.rs create mode 100644 packages/prices-api/tests/common/mock_discord.rs create mode 100644 packages/prices-api/tests/portal_issue.rs diff --git a/packages/prices-api/src/bin/serve.rs b/packages/prices-api/src/bin/serve.rs index 4778988b..4fed6e3c 100644 --- a/packages/prices-api/src/bin/serve.rs +++ b/packages/prices-api/src/bin/serve.rs @@ -67,6 +67,19 @@ async fn main() { .await .expect("failed to configure portal key issuance"); + // The eligibility gate (task 0189). This build has no Parameters and + // Secrets extension client, so both knobs come from the local-only seams, + // compiled out of the Lambda like `PORTAL_FREE_PLAN_ID`: + // + // PORTAL_GUILD_ID= PORTAL_MIN_ACCOUNT_AGE_MINUTES=5 + // + // Point `DISCORD_API_BASE` at a mock (or use the real one) — the member + // check runs against whatever Discord the sign-in does. + config + .load_portal_eligibility() + .await + .expect("failed to configure the portal eligibility gate"); + let port: u16 = std::env::var("PORT") .ok() .and_then(|p| p.parse().ok()) diff --git a/packages/prices-api/src/config.rs b/packages/prices-api/src/config.rs index 7e45952e..70d2ec87 100644 --- a/packages/prices-api/src/config.rs +++ b/packages/prices-api/src/config.rs @@ -68,6 +68,13 @@ pub struct AppConfig { /// [`Self::from_env`], because building it resolves credentials and reading /// the plan id is an HTTP call. pub portal_keys: Option, + /// Where the eligibility gate's two knobs come from (task 0189): the + /// Stellar guild id and the minimum account age. `None` means the gate is + /// not configured, which is the normal state while `portal_enabled` is + /// false; filled by [`Self::load_portal_eligibility`], which also probes + /// both values once so a mis-seeded parameter fails the cold start rather + /// than a visitor's click. + pub portal_eligibility: Option, } impl AppConfig { @@ -93,6 +100,7 @@ impl AppConfig { portal_oauth: None, portal_endpoints: crate::portal::auth::discord::Endpoints::from_env(), portal_keys: None, + portal_eligibility: None, } } @@ -168,6 +176,108 @@ impl AppConfig { Some(crate::portal::keys::gateway::Gateway::from_ambient_config(plan_id).await); Ok(()) } + + /// Fill [`Self::portal_eligibility`] with the sources of the eligibility + /// gate's two knobs (task 0189). + /// + /// Conditional on [`Self::portal_enabled`] for exactly the reasons the two + /// loaders above are. With the portal **open**, a missing source is fatal + /// — a portal whose "get my key" round-trip can only ever answer "could + /// not verify" is worse than a deploy that fails in `Init Errors` — and so + /// is an unreadable or malformed *value*: both parameters are **probed + /// once here**, so `discord-guild-id` seeded with a name instead of a + /// snowflake, or `min-account-age-minutes` holding "five", is a cold-start + /// failure with the parameter named, not a per-visitor refusal. + /// + /// What is stored is the **source**, not the probed value: every issuance + /// resolves it again, which is what makes an operator's `put-parameter` + /// take effect without a redeploy (bounded only by the Parameters and + /// Secrets extension's ~5 min cache). + /// + /// # Where the values come from + /// + /// `PORTAL_GUILD_ID_PARAM` and `PORTAL_MIN_ACCOUNT_AGE_PARAM` carry the + /// **names of SSM parameters** (`/prices/{env}/discord-guild-id`, + /// `/prices/{env}/min-account-age-minutes`), seeded by the operator at + /// deploy prep — never created by CDK, because a CloudFormation-managed + /// parameter is restored to the committed value by the next `cdk deploy`, + /// which would silently un-flip production back to the test guild after + /// [0179]. The direct-value overrides are local-only seams, compiled out + /// of the Lambda like `PORTAL_FREE_PLAN_ID`. + pub async fn load_portal_eligibility(&mut self) -> Result<(), PortalEligibilityError> { + if !self.portal_enabled { + return Ok(()); + } + let settings = eligibility_settings()?; + // Probe both values now. The per-action resolve keeps them tunable; + // this makes a bad seed loud at deploy time. + settings + .guild_id() + .await + .map_err(PortalEligibilityError::Probe)?; + settings + .min_account_age_minutes() + .await + .map_err(PortalEligibilityError::Probe)?; + self.portal_eligibility = Some(settings); + Ok(()) + } +} + +/// Why the eligibility gate could not be configured at cold start. +#[derive(Debug, thiserror::Error)] +pub enum PortalEligibilityError { + #[error( + "the portal is open but the eligibility gate has no sources; set PORTAL_GUILD_ID_PARAM \ + and PORTAL_MIN_ACCOUNT_AGE_PARAM to the SSM parameters the operator seeds at \ + /prices//discord-guild-id and /prices//min-account-age-minutes (see the \ + deploy-prep runbook)" + )] + NoSource, + #[error("probing the eligibility parameters failed: {0}")] + Probe(crate::portal::eligibility::EligibilityError), +} + +/// Build the eligibility sources from the environment. +fn eligibility_settings() +-> Result { + use crate::portal::eligibility::{EligibilitySettings, ParamSource}; + + let source = |direct_var: &str, param_var: &str| -> Option { + // A direct value, for a local run. Checked first, and **compiled out + // of the Lambda**, exactly as `PORTAL_FREE_PLAN_ID` is and for the + // same reason: `lambda:UpdateFunctionConfiguration` is a permission + // distinct from `UpdateFunctionCode`, and these two values decide + // which guild gates issuance and how old an account must be — left + // readable in the Lambda, one configuration change would silently + // point the gate at a guild of somebody else's choosing. + #[cfg(not(feature = "lambda"))] + if let Ok(value) = std::env::var(direct_var) + && !value.trim().is_empty() + { + return Some(ParamSource::Direct(value)); + } + #[cfg(feature = "lambda")] + let _ = direct_var; + + let name = std::env::var(param_var).ok()?; + if name.trim().is_empty() { + return None; + } + Some(ParamSource::Ssm(name.trim().to_string())) + }; + + let guild_id = source("PORTAL_GUILD_ID", "PORTAL_GUILD_ID_PARAM") + .ok_or(PortalEligibilityError::NoSource)?; + let min_account_age = source( + "PORTAL_MIN_ACCOUNT_AGE_MINUTES", + "PORTAL_MIN_ACCOUNT_AGE_PARAM", + ) + .ok_or(PortalEligibilityError::NoSource)?; + Ok(EligibilitySettings { + guild_id, + min_account_age, + }) } /// Why key issuance could not be configured at cold start. diff --git a/packages/prices-api/src/main.rs b/packages/prices-api/src/main.rs index 98260cfb..7f3caa0d 100644 --- a/packages/prices-api/src/main.rs +++ b/packages/prices-api/src/main.rs @@ -54,6 +54,16 @@ async fn main() { .await .expect("failed to configure portal key issuance at cold start"); + // The eligibility gate (task 0189). Stores the SSM parameter names for the + // guild id and the minimum account age — resolved per issuance so operator + // changes need no redeploy — and probes both once, so a mis-seeded + // parameter fails here in `Init Errors` rather than at a visitor's click. + // A no-op while `PORTAL_ENABLED` is false, like the two loads above. + config + .load_portal_eligibility() + .await + .expect("failed to configure the portal eligibility gate at cold start"); + // Build the CH client eagerly at cold start; it is Arc-backed and shared via // AppState across warm invocations. `client_from_lambda_env` reads // MTLS_SECRET_NAME + CH_DOMAIN (set by CDK) and fetches the cert bundle from diff --git a/packages/prices-api/src/portal/auth/discord.rs b/packages/prices-api/src/portal/auth/discord.rs index 1b1ce248..57f182cb 100644 --- a/packages/prices-api/src/portal/auth/discord.rs +++ b/packages/prices-api/src/portal/auth/discord.rs @@ -1,19 +1,27 @@ -//! The two calls this service makes to Discord, and nothing else (task 0186). +//! The three calls this service makes to Discord, and nothing else +//! (tasks 0186 + 0189). //! -//! `POST /oauth2/token` to turn an authorization code into an access token, then -//! `GET /users/@me` to read who it belongs to. The token is dropped at the end of -//! the callback — see [`AccessToken`]. +//! `POST /oauth2/token` to turn an authorization code into an access token, +//! `GET /users/@me/guilds/{guild}/member` on an issue round-trip to ask whether +//! that user is a member of the Stellar guild, then `GET /users/@me` to read +//! who the token belongs to. The token is dropped at the end of the callback — +//! see [`AccessToken`]. //! -//! # Scope is exactly `identify` +//! # Scope is exactly `identify` + `guilds.members.read` //! //! Requested in [`super::authorize_url`] and **verified in the token response** //! by [`TokenResponse::granted_scopes`]. Verifying is not paranoia about //! Discord: the requested scope set is also declared in the Developer Portal, so //! the authorize URL and the registration can disagree, and the response is the -//! only place the actual grant is observable. `guilds.members.read` is [0189]'s -//! to add — in both places — and `guilds` and `email` are refused outright by -//! ADR 0010, the first for returning every server a user belongs to and the -//! second for collecting data we have decided not to hold. +//! only place the actual grant is observable. The comparison is **set +//! equality** over whitespace-separated tokens — RFC 6749 §3.3 makes scope an +//! unordered set, so `guilds.members.read identify` is the same grant — and a +//! set compare still refuses anything wider *or* narrower. `guilds` and +//! `email` are refused outright by ADR 0010, the first for returning every +//! server a user belongs to and the second for collecting data we have decided +//! not to hold; `guilds.members.read` returns one membership in one guild the +//! user consented to reveal, which is the narrowest surface that can answer +//! the question at all. use std::time::Duration; @@ -66,9 +74,13 @@ pub enum DiscordError { UnexpectedScope { granted: String }, } -/// The one scope requested, sent to the authorize endpoint and checked on the -/// way back. ADR 0010: never `guilds`, never `email`. -pub const SCOPE: &str = "identify"; +/// The scopes requested, sent to the authorize endpoint and checked on the way +/// back — as a set, see [`scopes_match`]. ADR 0010: never `guilds`, never +/// `email`. The same pair must be declared in the Developer Portal +/// registration (deploy-prep runbook §1 step 3); a registration that drifts +/// narrower is refused by Discord at the authorize step, one that drifts wider +/// is refused here on the token response. +pub const SCOPE: &str = "identify guilds.members.read"; /// Discord's OAuth2 authorize page — where the visitor is sent, not an API call. pub const DEFAULT_AUTHORIZE_URL: &str = "https://discord.com/oauth2/authorize"; @@ -165,6 +177,37 @@ impl Endpoints { fn current_user_url(&self) -> String { format!("{}/users/@me", self.api_base.trim_end_matches('/')) } + + /// The membership route for one guild (task 0189). + /// + /// The guild id is operator-seeded configuration (SSM), not code — it is + /// validated to be a bare snowflake before it becomes a path segment, so a + /// mis-seeded value cannot smuggle `../` or a query string into the URL. + /// Validation failure is the caller's `Unknown` outcome, not a panic. + fn member_url(&self, guild_id: &str) -> Option { + if guild_id.is_empty() || !guild_id.bytes().all(|b| b.is_ascii_digit()) { + return None; + } + Some(format!( + "{}/users/@me/guilds/{guild_id}/member", + self.api_base.trim_end_matches('/') + )) + } +} + +/// Compare a granted scope string against [`SCOPE`] as a **set**. +/// +/// RFC 6749 §3.3: scope is a space-delimited, unordered set. String equality +/// would refuse `guilds.members.read identify` — the exact grant we asked for, +/// echoed in an order we do not control. A set compare is order-independent +/// and still refuses a grant that is wider (registration drifted to include +/// `guilds` or `email`) or narrower (the member scope missing, which would +/// turn every membership check into a `401`-shaped `Unknown`). +fn scopes_match(granted: &str) -> bool { + use std::collections::BTreeSet; + let granted: BTreeSet<&str> = granted.split_whitespace().collect(); + let requested: BTreeSet<&str> = SCOPE.split_whitespace().collect(); + granted == requested } /// Build the HTTP client used for both calls. @@ -251,11 +294,12 @@ pub async fn exchange_code( message: e.to_string(), })?; - // Compared as a whole string, not as "contains identify". A grant of - // `identify guilds` contains it and is exactly what must be refused: it - // would mean the Developer Portal registration had drifted from this code, - // and ADR 0010 rejects `guilds` on privacy grounds. - if token.granted_scopes().trim() != SCOPE { + // Compared as a set, not as "contains identify". A grant of + // `identify guilds guilds.members.read` contains both requested scopes and + // is exactly what must be refused: it would mean the Developer Portal + // registration had drifted from this code, and ADR 0010 rejects `guilds` + // on privacy grounds. See `scopes_match` for why not string equality. + if !scopes_match(token.granted_scopes()) { return Err(DiscordError::UnexpectedScope { granted: token.granted_scopes().to_string(), }); @@ -264,6 +308,131 @@ pub async fn exchange_code( Ok(AccessToken(token.access_token)) } +/// The fields of `GET /users/@me/guilds/{guild}/member` this service reads. +/// +/// One, out of the dozen the endpoint returns. `flags` and `joined_at` are +/// deliberately not deserialized: nothing here stores membership data (ADR +/// 0010 — "the registry stores no membership data"), and what is not named +/// cannot leak into a log. +#[derive(Debug, Deserialize)] +pub struct GuildMember { + /// Membership Screening: `Some(true)` means the user joined but has not + /// cleared the screening gate. **Optional on purpose** — the docs' + /// presence guarantee is written about gateway events, not this REST + /// route, and 0180 item 2 (which would have settled it) is still + /// unmeasured. Absent is a third state the caller must handle, never + /// "cleared". + pub pending: Option, +} + +/// What the membership route answered. Three outcomes, not two, and +/// deliberately infallible: a Discord that is down is an ordinary answer here, +/// not an error for the callback to 502 on, because "could not verify" must +/// never be rendered as "not a member" (task 0189). +#[derive(Debug)] +pub enum MemberLookup { + /// A member object came back. Whether it *counts* is eligibility's call. + Member(GuildMember), + /// A `404` whose JSON `code` is 10007 (Unknown Member) or 10004 (Unknown + /// Guild) — the only shapes read as "not a member". 10004 usually means + /// the *guild id* is wrong, which is our configuration and not the user; + /// the caller logs it loudly for exactly that reason. + NotMember { code: u64 }, + /// Anything else: `401`/`403`/`429`/`5xx`, a `404` whose body does not + /// carry a recognised code, a transport fault, an unparseable body. Do not + /// issue, and do not accuse. + Unknown { + status: Option, + detail: String, + }, +} + +/// JSON error codes on a `404` that mean "no such membership". +/// +/// 10007 "Unknown Member", 10004 "Unknown Guild" — the only two documented +/// shapes. The exact live behaviour is 0180 item 1, still unmeasured; until it +/// is, an unlisted code on a 404 lands in `Unknown`, which fails safe in both +/// directions (no key issued, no accusation rendered). +const NOT_MEMBER_CODES: [u64; 2] = [10_007, 10_004]; + +/// Ask whether the token's owner is a member of `guild_id` (task 0189). +/// +/// Called with the **user's own** consented token — no bot in the guild, no +/// admin rights — and **by reference**: it runs before [`current_user`] +/// consumes the token, so the consumed-at-end property of the callback is +/// untouched. +pub async fn guild_member( + client: &reqwest::Client, + endpoints: &Endpoints, + token: &AccessToken, + guild_id: &str, +) -> MemberLookup { + let Some(url) = endpoints.member_url(guild_id) else { + return MemberLookup::Unknown { + status: None, + detail: "guild id is not a snowflake — check the SSM parameter".into(), + }; + }; + + let response = match client.get(&url).bearer_auth(&token.0).send().await { + Ok(response) => response, + Err(e) => { + return MemberLookup::Unknown { + status: None, + detail: e.to_string(), + }; + } + }; + + let status = response.status(); + // The body is read for classification only (the JSON `code` on a 404) and + // is never echoed to the visitor or logged verbatim — same caution as the + // token exchange above. + let body = response.bytes().await.unwrap_or_default(); + classify_member_response(status, &body) +} + +/// Map one HTTP answer from the member route to a [`MemberLookup`]. +/// +/// Pure so the whole decision table is unit-testable without a socket. Only a +/// `404` carrying a recognised JSON `code` is `NotMember`; a `2xx` must parse +/// as a member object; everything else is `Unknown`. +fn classify_member_response(status: reqwest::StatusCode, body: &[u8]) -> MemberLookup { + if status.is_success() { + return match serde_json::from_slice::(body) { + Ok(member) => MemberLookup::Member(member), + Err(e) => MemberLookup::Unknown { + status: Some(status), + detail: format!("member object did not parse: {e}"), + }, + }; + } + + if status == reqwest::StatusCode::NOT_FOUND { + #[derive(Deserialize)] + struct ErrorBody { + code: Option, + } + if let Ok(ErrorBody { code: Some(code) }) = serde_json::from_slice::(body) + && NOT_MEMBER_CODES.contains(&code) + { + return MemberLookup::NotMember { code }; + } + // A 404 with no recognised code is NOT proof of non-membership — it + // could be a proxy, an outage page, or a shape 0180 item 1 has not + // measured yet. Fail safe: refuse without accusing. + return MemberLookup::Unknown { + status: Some(status), + detail: "404 without a recognised error code".into(), + }; + } + + MemberLookup::Unknown { + status: Some(status), + detail: "membership not verifiable".into(), + } +} + /// Read the identity the token belongs to. /// /// Takes the token **by value** so it is consumed here: after this call the @@ -301,12 +470,126 @@ mod tests { use super::*; #[test] - fn the_requested_scope_is_exactly_identify() { - assert_eq!(SCOPE, "identify"); + fn the_requested_scopes_are_exactly_identify_and_members_read() { + assert_eq!(SCOPE, "identify guilds.members.read"); // The two ADR 0010 forbids, stated as a test so a future edit that adds - // one has to delete an assertion rather than change a string. - assert!(!SCOPE.contains("guilds")); - assert!(!SCOPE.contains("email")); + // one has to delete an assertion rather than change a string. Compared + // per token — `guilds.members.read` legitimately *contains* "guilds". + for token in SCOPE.split_whitespace() { + assert_ne!(token, "guilds"); + assert_ne!(token, "email"); + } + } + + /// RFC 6749 §3.3: scope is an unordered set. Discord echoing the pair in + /// the other order is the same grant; anything wider or narrower is not. + #[test] + fn the_granted_scope_is_compared_as_a_set_not_a_string() { + assert!(scopes_match("identify guilds.members.read")); + assert!(scopes_match("guilds.members.read identify")); + assert!(scopes_match(" guilds.members.read identify ")); + + // Narrower: the member scope missing means every membership check + // would come back 401-shaped — refuse at the exchange instead. + assert!(!scopes_match("identify")); + assert!(!scopes_match("guilds.members.read")); + assert!(!scopes_match("")); + // Wider: the registration drifted. `guilds` and `email` are the ADR's + // named refusals. + assert!(!scopes_match("identify guilds.members.read guilds")); + assert!(!scopes_match("identify guilds.members.read email")); + assert!(!scopes_match("identify guilds")); + } + + #[test] + fn the_member_url_is_built_only_from_a_bare_snowflake() { + let endpoints = Endpoints::default(); + assert_eq!( + endpoints.member_url("897514728459468821").as_deref(), + Some("https://discord.com/api/users/@me/guilds/897514728459468821/member") + ); + // Operator input never becomes a path segment un-validated. + for bad in ["", "stellar_test", "123/../admin", "123?x=1", "123 456"] { + assert_eq!(endpoints.member_url(bad), None, "accepted {bad:?}"); + } + } + + /// The whole decision table for one membership answer, pinned pure. + /// + /// Only a 404 carrying JSON code 10007/10004 is "not a member"; every + /// other refusal — including a 404 whose body is empty, non-JSON or + /// carries an unlisted code — is `Unknown`, because 0180 item 1 (the live + /// shape) is unmeasured and an accusation must not rest on a guess. + #[test] + fn only_a_recognised_404_code_reads_as_not_a_member() { + use reqwest::StatusCode; + + let is_not_member = |status: StatusCode, body: &str| { + matches!( + classify_member_response(status, body.as_bytes()), + MemberLookup::NotMember { .. } + ) + }; + let is_unknown = |status: StatusCode, body: &str| { + matches!( + classify_member_response(status, body.as_bytes()), + MemberLookup::Unknown { .. } + ) + }; + + assert!(is_not_member( + StatusCode::NOT_FOUND, + r#"{"message": "Unknown Member", "code": 10007}"# + )); + assert!(is_not_member( + StatusCode::NOT_FOUND, + r#"{"message": "Unknown Guild", "code": 10004}"# + )); + + // A 404 that cannot prove what it is. + assert!(is_unknown(StatusCode::NOT_FOUND, "")); + assert!(is_unknown(StatusCode::NOT_FOUND, "gateway")); + assert!(is_unknown(StatusCode::NOT_FOUND, r#"{"code": 0}"#)); + assert!(is_unknown(StatusCode::NOT_FOUND, r#"{"code": 10008}"#)); + assert!(is_unknown(StatusCode::NOT_FOUND, r#"{"message": "hm"}"#)); + + // The statuses the task names, plus the ones it implies. + for status in [ + StatusCode::UNAUTHORIZED, + StatusCode::FORBIDDEN, + StatusCode::TOO_MANY_REQUESTS, + StatusCode::INTERNAL_SERVER_ERROR, + StatusCode::BAD_GATEWAY, + StatusCode::SERVICE_UNAVAILABLE, + ] { + // Even a body that LOOKS like a not-member error does not count + // outside a 404 — a throttled proxy echoing an error template + // must not read as an accusation. + assert!(is_unknown(status, r#"{"code": 10007}"#), "{status}"); + } + } + + /// A member object parses with `pending` present (either value) or absent, + /// and absence survives as `None` — the third state eligibility handles + /// explicitly rather than defaulting. + #[test] + fn a_member_response_keeps_pending_optional() { + let of = + |body: &str| match classify_member_response(reqwest::StatusCode::OK, body.as_bytes()) { + MemberLookup::Member(m) => m.pending, + other => panic!("expected a member, got {other:?}"), + }; + assert_eq!(of(r#"{"pending": false, "flags": 0}"#), Some(false)); + assert_eq!(of(r#"{"pending": true}"#), Some(true)); + assert_eq!(of(r#"{"joined_at": "2020-01-01T00:00:00Z"}"#), None); + + // A 200 whose body is not a member object at all is Unknown, not a + // pass — a mock or proxy answering `{}` IS a member object (all + // fields optional), but non-JSON is not. + assert!(matches!( + classify_member_response(reqwest::StatusCode::OK, b"not json"), + MemberLookup::Unknown { .. } + )); } /// **The `lambda` build has no code path that reads the overrides.** @@ -384,6 +667,6 @@ mod tests { fn a_missing_scope_field_reads_as_no_scope_rather_than_a_decode_error() { let response: TokenResponse = serde_json::from_str(r#"{"access_token":"t"}"#).unwrap(); assert_eq!(response.granted_scopes(), ""); - assert_ne!(response.granted_scopes().trim(), SCOPE); + assert!(!scopes_match(response.granted_scopes())); } } diff --git a/packages/prices-api/src/portal/auth/issue.rs b/packages/prices-api/src/portal/auth/issue.rs new file mode 100644 index 00000000..9f72e5af --- /dev/null +++ b/packages/prices-api/src/portal/auth/issue.rs @@ -0,0 +1,310 @@ +//! Completing an `action=issue` round-trip (task 0189). +//! +//! The callback arrives here holding a **fresh** Discord token — the +//! eligibility proof ADR 0010 §8 requires. This module asks Discord, right +//! now, whether the token's owner is a member of the Stellar guild and whether +//! their account is old enough, and only then hands off to the issue path +//! (`super::super::keys::issue_for`). Nothing is remembered: the next issue or +//! rework proves itself again. +//! +//! # Every outcome is a redirect, and the outcomes are five literals +//! +//! The visitor is mid-navigation (Discord sent them here), so the answer is a +//! `303` back to the portal with one of: +//! +//! | query | meaning | +//! | --- | --- | +//! | `?issue=ok` | the key exists and is on the plan — the page reveals it | +//! | `?issue=not_member` | Discord confirmed no such membership (its own 10007/10004), or the member has not cleared screening | +//! | `?issue=too_young&wait_secs=N` | account below the threshold; `N` is the wait, so the page's copy follows the operator's setting | +//! | `?issue=unknown` | membership could **not** be verified (throttle, outage, absent `pending`, unreadable parameter) — refused without accusation | +//! | `?issue=failed` | eligibility passed but the control plane would not produce a key | +//! +//! `unknown` and `failed` are separate on purpose: one says "Discord could not +//! vouch for you, try shortly", the other says "you are fine, our key service +//! was not". Collapsing them would render an AWS incident as a doubt about the +//! visitor's membership. +//! +//! The **key value never rides in a `Location`** — after `?issue=ok` the page +//! calls the reveal route, which is read-only and session-authorized. +//! +//! # The session is refreshed on every outcome past identity +//! +//! Identity was just proven, so even a refused visitor leaves signed in — a +//! non-member still legitimately holds reveal and usage (the epic's non-goal: +//! leaving the guild never forfeits an existing key). If a session for a +//! *different* account was present, the fresh identity simply replaces it — +//! the same rule the sign-in arm applies, so the key issued and the session +//! shown can never disagree about who the visitor is. + +use std::sync::Arc; +use std::time::Duration; + +use axum::response::Response; + +use super::super::eligibility::{self, Eligibility, EligibilitySettings}; +use super::super::keys::{self, IssueOutcome}; +use super::super::usage::UsageCache; +use super::discord::{self, AccessToken, MemberLookup}; +use super::secret::OauthSecret; +use super::session::{self, Session}; +use super::state_token; +use super::{AuthState, PORTAL_HOME, cookies, redirect, refuse_discord}; +use crate::portal::keys::gateway::Gateway; + +/// See the module table. Literals, like `?signin=…` — the dynamic one is +/// [`too_young_query`], whose only variable part is a `u64` rendered in +/// decimal, so no request-derived byte can reach a `Location` header. +pub(super) const ISSUE_OK_QUERY: &str = "?issue=ok"; +pub(super) const ISSUE_NOT_MEMBER_QUERY: &str = "?issue=not_member"; +pub(super) const ISSUE_UNKNOWN_QUERY: &str = "?issue=unknown"; +pub(super) const ISSUE_FAILED_QUERY: &str = "?issue=failed"; + +/// The one parameterised landing state: how long until the account clears the +/// threshold. Digits only, by type. +pub(super) fn too_young_query(wait_secs: u64) -> String { + format!("?issue=too_young&wait_secs={wait_secs}") +} + +/// Mirrors `keys`' `KEYS_UNCONFIGURED` — the same deployment fault ("portal +/// open, key issuance not wired") reported under the same code whichever door +/// it is noticed at. `/auth/login?action=issue` refuses with this instead of +/// starting a round-trip that cannot end in a key. +pub(super) const KEYS_UNCONFIGURED: &str = "keys_unconfigured"; + +/// Everything the issue arm needs beyond what sign-in already carries. +/// +/// All optional, like `AuthState::oauth` and `KeysState::gateway`, and for the +/// same reason: the api-handler boots with the portal closed and nothing +/// provisioned. `config::load_portal_eligibility` fails the cold start when +/// the portal is *open* with these missing, so a `None` here in production +/// means the portal is closed and the gate answers before any handler does. +#[derive(Clone)] +pub struct IssueDeps { + pub(super) gateway: Option>, + pub(super) usage_cache: Option, + pub(super) settings: Option>, + /// The same wall-clock ceiling 0187's handler put on the reconciliation. + pub(super) deadline: Duration, +} + +impl Default for IssueDeps { + fn default() -> Self { + Self { + gateway: None, + usage_cache: None, + settings: None, + deadline: keys::RECONCILE_DEADLINE, + } + } +} + +impl IssueDeps { + pub fn new( + gateway: Option, + usage_cache: Option, + settings: Option, + ) -> Self { + Self { + gateway: gateway.map(Arc::new), + usage_cache, + settings: settings.map(Arc::new), + deadline: keys::RECONCILE_DEADLINE, + } + } + + /// Shorten the deadline for tests — compiled out of the Lambda for the + /// reason `KeysState::with_deadline` is: a deployed build must contain no + /// way to set this to something that lets a slow control plane outlive + /// the invocation. + #[cfg(not(feature = "lambda"))] + pub fn with_deadline(mut self, deadline: Duration) -> Self { + self.deadline = deadline; + self + } + + /// Whether an issue round-trip could complete on this deployment. + pub(super) fn is_wired(&self) -> bool { + self.gateway.is_some() && self.settings.is_some() + } +} + +/// Finish an `action=issue` callback: check, then issue, then land. +/// +/// Runs after `state` verification, the code exchange and the granted-scope +/// check — `token` is the fresh, scope-verified token those produced. The +/// order below is load-bearing: the membership call borrows the token, the +/// identity read consumes it, and everything after holds no token at all. +pub(super) async fn complete_issue( + state: &AuthState, + oauth: &OauthSecret, + token: AccessToken, + drop_pending: String, +) -> Response { + // Resolve the two operator knobs first — per action, so an SSM change is + // honoured without a redeploy. Failure is `unknown`, not a 5xx: the + // visitor is mid-navigation, the fault is ours, and "could not verify" + // is the honest refusal that does not accuse them of anything. + let verified = match state.issue.settings.as_deref() { + None => { + // `login` refuses `action=issue` on an unwired deployment, so + // arriving here means the deployment changed under an in-flight + // round-trip. Log it as ours; tell the visitor to retry. + tracing::error!("an issue callback arrived with no eligibility settings wired"); + None + } + Some(settings) => match ( + settings.guild_id().await, + settings.min_account_age_minutes().await, + ) { + (Ok(guild_id), Ok(min_age)) => Some((guild_id, min_age)), + (guild, age) => { + for error in [guild.err(), age.err()].into_iter().flatten() { + tracing::error!( + error = %error, + "eligibility parameters could not be read; refusing without accusation" + ); + } + None + } + }, + }; + + // The membership call BORROWS the token; the identity read then consumes + // it. Asked in this order so that one round-trip serves both questions — + // Discord does not re-prompt for consent on repeat authorisation, so this + // whole detour cost the visitor a redirect, not a login. + let member = match &verified { + Some((guild_id, _)) => { + let looked_up = + discord::guild_member(&state.http, &state.endpoints, &token, guild_id).await; + if let MemberLookup::NotMember { code: 10_004 } = looked_up { + // "Unknown Guild" is far more likely to be OUR mis-seeded + // parameter than the visitor's standing. Still rendered as + // the spec says (0180 item 1 will settle the real shape), + // but loud in CloudWatch so a config fault is visible on the + // first refusal rather than after a support thread. + tracing::warn!( + guild_id = %guild_id, + "membership check answered Unknown Guild (10004) — \ + is the discord-guild-id parameter right?" + ); + } + Some(looked_up) + } + None => None, + }; + + let user = match discord::current_user(&state.http, &state.endpoints, token).await { + Ok(user) => user, + // No identity, no session, no verdict — the same 502 the sign-in arm + // answers when Discord will not say who the visitor is. + Err(error) => return refuse_discord("identity read", error, drop_pending), + }; + + // Identity is proven: from here on every outcome carries a fresh session, + // replacing whatever was there. See the module docs. + let session = Session::issue(&user.id, &user.username, state_token::now_secs()); + let session_cookie = cookies::set( + cookies::SESSION_COOKIE, + &session.encode(&oauth.signing_key), + cookies::SESSION_PATH, + session::SESSION_TTL_SECS, + ); + let land = |query: &str| { + redirect( + &format!("{PORTAL_HOME}{query}"), + vec![drop_pending.clone(), session_cookie.clone()], + ) + }; + + let verdict = match (&verified, &member) { + (Some((_, min_age)), Some(member)) => { + eligibility::decide(member, &user.id, *min_age, eligibility::now_ms()) + } + // Parameters unreadable — the membership question was never asked. + _ => Eligibility::Unknown, + }; + + match verdict { + Eligibility::NotMember => { + tracing::info!(outcome = "not_member", "portal issue refused"); + land(ISSUE_NOT_MEMBER_QUERY) + } + Eligibility::TooYoung { wait_secs } => { + tracing::info!(outcome = "too_young", wait_secs, "portal issue refused"); + land(&too_young_query(wait_secs)) + } + Eligibility::Unknown => { + // The load-bearing warns (which check could not answer, and why) + // fired where the answer was known; this line is the one that + // says what the visitor was told. + tracing::info!( + outcome = "unknown", + "portal issue refused without accusation" + ); + land(ISSUE_UNKNOWN_QUERY) + } + Eligibility::Eligible => { + let Some(gateway) = state.issue.gateway.as_deref() else { + tracing::error!("an eligible issue callback arrived with no control plane wired"); + return land(ISSUE_FAILED_QUERY); + }; + match keys::issue_for(gateway, &user.id, state.issue.deadline).await { + IssueOutcome::Issued => { + // A key now exists, so a cached "no key" on the usage + // route is false — same eviction the reveal performs, + // for the page this redirect is about to land on. + if let Some(cache) = &state.issue.usage_cache { + cache.invalidate_no_key(&user.id); + } + land(ISSUE_OK_QUERY) + } + IssueOutcome::Failed => land(ISSUE_FAILED_QUERY), + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The five landing states are distinct literals under the portal home, + /// like `?signin=…` — extending `the_only_redirect_targets_are_the_portal + /// _itself` to the issue flow. + #[test] + fn the_issue_landing_states_are_distinct_portal_literals() { + let fixed = [ + ISSUE_OK_QUERY, + ISSUE_NOT_MEMBER_QUERY, + ISSUE_UNKNOWN_QUERY, + ISSUE_FAILED_QUERY, + ]; + for (i, a) in fixed.iter().enumerate() { + assert!(a.starts_with("?issue=")); + assert!(format!("{PORTAL_HOME}{a}").starts_with("/api-tokens/?")); + for b in &fixed[i + 1..] { + assert_ne!(a, b); + } + } + } + + /// The one dynamic landing state renders a `u64` and nothing else — no + /// request-derived byte can reach the `Location` header through it. + #[test] + fn the_too_young_query_is_digits_only() { + assert_eq!(too_young_query(173), "?issue=too_young&wait_secs=173"); + let rendered = too_young_query(u64::MAX); + let (prefix, value) = rendered.split_once("wait_secs=").unwrap(); + assert_eq!(prefix, "?issue=too_young&"); + assert!(value.bytes().all(|b| b.is_ascii_digit())); + } + + #[test] + fn issue_deps_default_to_unwired_with_the_production_deadline() { + let deps = IssueDeps::default(); + assert!(!deps.is_wired()); + assert_eq!(deps.deadline, keys::RECONCILE_DEADLINE); + } +} diff --git a/packages/prices-api/src/portal/auth/mod.rs b/packages/prices-api/src/portal/auth/mod.rs index 2c2e80ef..89dc345b 100644 --- a/packages/prices-api/src/portal/auth/mod.rs +++ b/packages/prices-api/src/portal/auth/mod.rs @@ -8,7 +8,8 @@ //! | route | does | //! | --- | --- | //! | `GET /auth/login` | mints `state` + PKCE, redirects to Discord | -//! | `GET /auth/callback` | verifies `state`, exchanges the code, issues a session | +//! | `GET /auth/login?action=issue` | the same, for the eligibility-checked issue round-trip ([0189]) | +//! | `GET /auth/callback` | verifies `state`, exchanges the code, completes the action it names | //! | `GET /auth/me` | reports who the caller is, or that they are nobody | //! | `POST /auth/logout` | clears the session | //! @@ -17,13 +18,16 @@ //! these routes are exempt from that gate, because a visitor signing in to get //! a key does not have one yet (`crate::auth::is_exempt`). //! -//! # What this slice deliberately does not do +//! # What a callback completes depends on what its `state` names //! -//! Everything that turns an identity into an entitlement is [0189]: the -//! `guilds.members.read` scope, the guild membership call, `pending`, the -//! snowflake account-age minimum. Issuing a key is [0187]. Nothing here reads or -//! writes any store — there is no registry yet ([0190] decides whether there -//! ever is one) and no Discord token is kept (see [`session`]). +//! A `signin` callback issues a session and nothing else. An `issue` callback +//! ([`issue`], task 0189) additionally checks guild membership and account age +//! against the **fresh** token before handing off to the key path — that is +//! ADR 0010 §8's "the callback completes that action and nothing else", and +//! the action slot in [`state_token`] is what makes the two round-trips +//! non-interchangeable. Nothing here reads or writes any store — there is no +//! registry yet ([0190] decides whether there ever is one) and no Discord +//! token is kept (see [`session`]). //! //! # Why the routes are keyless, and what stands in for a key //! @@ -37,6 +41,7 @@ pub mod cookies; pub mod crypto; pub mod discord; +pub mod issue; pub mod secret; pub mod session; pub mod state_token; @@ -136,6 +141,9 @@ pub struct AuthState { oauth: Option>, endpoints: std::sync::Arc, http: reqwest::Client, + /// What the `action=issue` round-trip needs beyond sign-in (task 0189). + /// Defaults to unwired; [`super::apply`] wires it when the portal opens. + issue: issue::IssueDeps, } impl AuthState { @@ -144,8 +152,17 @@ impl AuthState { oauth: oauth.map(std::sync::Arc::new), endpoints: std::sync::Arc::new(endpoints), http: discord::build_client(), + issue: issue::IssueDeps::default(), } } + + /// Wire the issue round-trip's dependencies in. A builder, like + /// `KeysState::with_usage_cache`, so every existing constructor and test + /// stays valid. + pub fn with_issue(mut self, deps: issue::IssueDeps) -> Self { + self.issue = deps; + self + } } /// The four routes, as a `Router` carrying its own state. @@ -203,6 +220,19 @@ async fn login( }, }; + // An issue round-trip on a deployment with no control plane or eligibility + // parameters wired cannot end in a key — refuse before sending the visitor + // to Discord, under the same code the key routes use for the same fault. + // Only reachable with the portal open and issuance unprovisioned, exactly + // like `unconfigured()` below; `load_portal_eligibility` fails the cold + // start on that combination, so this is the second line, not the first. + if action == Action::Issue && !state.issue.is_wired() { + return no_store(errors::service_unavailable( + issue::KEYS_UNCONFIGURED, + "API key issuance is not configured on this deployment", + )); + } + let started = state_token::start(&oauth.signing_key, action, state_token::now_secs()); let location = authorize_url(&state.endpoints, oauth, &started); @@ -236,8 +266,9 @@ fn authorize_url( .append_pair("response_type", "code") .append_pair("client_id", &oauth.client_id) .append_pair("redirect_uri", &oauth.redirect_uri) - // Exactly `identify`. ADR 0010; also declared in the Developer Portal, - // and verified again on the token response (`discord::exchange_code`). + // Exactly `identify guilds.members.read`. ADR 0010; also declared in + // the Developer Portal, and verified again — as a set — on the token + // response (`discord::exchange_code`). .append_pair("scope", discord::SCOPE) .append_pair("state", &started.state_param) .append_pair("code_challenge", &started.code_challenge) @@ -349,11 +380,10 @@ async fn callback( return refuse_query("callback carried neither `code` nor `error`", drop_pending); }; - // One action exists, and it is still matched rather than assumed. When - // [0189] adds `issue`, the arm it needs is a new `match` line and not a - // restructuring of this handler. + // The action decides what this callback is allowed to complete — matched + // rather than assumed, which is what the slot was carried for. match accepted.action { - Action::SignIn => {} + Action::SignIn | Action::Issue => {} // Compiled only into the test build, and unreachable even there: // `Action::parse` never yields `TestOther`, so `/auth/login` cannot mint // a round-trip for it. Refused rather than `unreachable!()` — a panic in @@ -382,6 +412,14 @@ async fn callback( Err(error) => return refuse_discord("token exchange", error, drop_pending), }; + // An issue round-trip diverges here, with the fresh, scope-verified token: + // membership and account age are checked against it before any key moves, + // and every outcome is a redirect (see `issue`). The sign-in tail below + // never sees an `Issue` action. + if accepted.action == Action::Issue { + return issue::complete_issue(&state, oauth, token, drop_pending).await; + } + // `token` is moved here, so from this line on the handler cannot reach it. let user = match discord::current_user(&state.http, &state.endpoints, token).await { Ok(user) => user, @@ -709,7 +747,7 @@ mod tests { /// only place a mistake shows up on Discord's error page rather than in our /// logs. #[test] - fn the_authorize_url_asks_for_identify_with_s256_pkce() { + fn the_authorize_url_asks_for_the_two_scopes_with_s256_pkce() { let secret = oauth(); let started = state_token::start(&secret.signing_key, Action::SignIn, 1_800_000_000); let url = authorize_url(&discord::Endpoints::default(), &secret, &started); @@ -734,8 +772,8 @@ mod tests { get("redirect_uri"), "https://portal.example/api-tokens/api/auth/callback" ); - // Exactly `identify` — not a superset, not a second scope. - assert_eq!(get("scope"), "identify"); + // Exactly the pair — not a superset, and never `guilds` or `email`. + assert_eq!(get("scope"), "identify guilds.members.read"); assert_eq!(get("code_challenge_method"), "S256"); assert_eq!(get("code_challenge"), started.code_challenge); assert_eq!(get("state"), started.state_param); @@ -883,7 +921,8 @@ mod tests { } } - /// The redirect target is a literal in every branch. An `assert` rather than + /// The redirect target is a literal in every branch — sign-in's two + /// landing states and the issue flow's five alike. An `assert` rather than /// a comment, so a later slice that adds a `redirect_to` parameter has to /// delete this to do it. #[test] @@ -891,7 +930,17 @@ mod tests { assert_eq!(PORTAL_HOME, "/api-tokens/"); assert!(PORTAL_HOME.starts_with('/')); assert!(!PORTAL_HOME.starts_with("//")); - assert!(format!("{PORTAL_HOME}{CANCELLED_QUERY}").starts_with("/api-tokens/?")); + for query in [ + CANCELLED_QUERY, + FAILED_QUERY, + issue::ISSUE_OK_QUERY, + issue::ISSUE_NOT_MEMBER_QUERY, + issue::ISSUE_UNKNOWN_QUERY, + issue::ISSUE_FAILED_QUERY, + &issue::too_young_query(173), + ] { + assert!(format!("{PORTAL_HOME}{query}").starts_with("/api-tokens/?")); + } } /// The registered redirect URI and the route that serves it are one string, diff --git a/packages/prices-api/src/portal/auth/state_token.rs b/packages/prices-api/src/portal/auth/state_token.rs index 67fa207e..ae46d632 100644 --- a/packages/prices-api/src/portal/auth/state_token.rs +++ b/packages/prices-api/src/portal/auth/state_token.rs @@ -46,12 +46,10 @@ //! //! # The action slot //! -//! [`Action`] has one variant today and the task requires it anyway, because -//! [0189] binds "issue a key" and "rework a key" to a round-trip and adding the -//! field then would mean re-deriving the signing format while a deployed portal -//! holds live cookies in the old one. It is verified now, on both halves, so -//! that when a second action exists the check is already there and already -//! tested rather than being added alongside the thing it is meant to constrain. +//! [`Action`] was carried with a single variant from [0186] precisely so that +//! [0189] could add [`Action::Issue`] to a signing format that deployed portals +//! already hold live cookies in, with the mismatch check already present and +//! already tested rather than arriving alongside the thing it constrains. //! //! ADR 0010 §8 is what the slot is for: eligibility travels by //! re-authentication, not in the session, and "the callback completes that @@ -85,19 +83,26 @@ const TOKEN_BYTES: usize = 32; /// cannot renumber an existing one. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub enum Action { - /// Establish who the visitor is. The only action in this slice. + /// Establish who the visitor is. #[serde(rename = "signin")] SignIn, - /// A second action that exists **only under `cfg(test)`**. + /// Issue an API key (task 0189). /// - /// Not [0189]'s `issue` arriving early — it is never parsed from a query - /// string, never minted by [`start`] outside a test, and is compiled out of - /// every shipped binary. It exists because with a single variant the - /// [`StateError::ActionMismatch`] arm in [`accept`] is **unreachable**, and - /// an unreachable branch is an untested one: deleting the comparison - /// entirely left the whole suite green, which is exactly the regression the - /// action slot is supposed to be protected against when [0189] adds a real - /// second action. + /// The round-trip is the eligibility proof: the callback holds a fresh + /// Discord token with which to check guild membership and account age at + /// the moment of issuance, per ADR 0010 §8. A `signin` callback cannot + /// complete an issuance — that is exactly what the mismatch check refuses. + #[serde(rename = "issue")] + Issue, + /// An extra action that exists **only under `cfg(test)`**. + /// + /// It is never parsed from a query string, never minted by [`start`] + /// outside a test, and is compiled out of every shipped binary. It existed + /// before [`Action::Issue`] because with a single variant the + /// [`StateError::ActionMismatch`] arm in [`accept`] was **unreachable**, + /// and an unreachable branch is an untested one: deleting the comparison + /// entirely left the whole suite green. It stays now so the mismatch tests + /// need no knowledge of which real actions a build happens to have. /// /// The alternative — asserting the mismatch through a hand-signed payload /// carrying an unknown action string — does not test this branch at all. It @@ -110,14 +115,15 @@ pub enum Action { impl Action { /// Parse the `action` query parameter of `/auth/login`. /// - /// An unknown value is rejected rather than defaulted. Defaulting would mean - /// that when [0189] adds `issue`, a client asking for an action this build - /// does not know would silently get a sign-in — and the callback would then - /// complete a *different* action than the one confirmed, which is the exact - /// thing the slot exists to prevent. + /// An unknown value is rejected rather than defaulted. Defaulting would + /// mean a client asking for an action this build does not know (0191's + /// `rework`, say) would silently get a sign-in — and the callback would + /// then complete a *different* action than the one confirmed, which is the + /// exact thing the slot exists to prevent. pub fn parse(raw: &str) -> Option { match raw { "signin" => Some(Self::SignIn), + "issue" => Some(Self::Issue), // `TestOther` is deliberately absent: it must not be reachable from // a query string even in a test build. _ => None, @@ -444,13 +450,12 @@ mod tests { assert_eq!(state.action, Action::SignIn); assert_eq!(pending.action, Action::SignIn); - // And a disagreement between them is refused. Constructed by signing a - // mismatched pair directly, which is the only way to reach this arm - // while one action exists — and the point is that it stays reachable. + // And a state naming an action this build does not know — 0191's + // `rework`, arriving early — is refused at deserialization. let crossed = sign_claims( KEY, CTX_STATE, - &serde_json::json!({ "action": "issue", "nonce": pending.nonce, "exp": state.exp }), + &serde_json::json!({ "action": "rework", "nonce": pending.nonce, "exp": state.exp }), ); assert_eq!( accept(KEY, &crossed, Some(&started.pending_cookie), NOW).unwrap_err(), @@ -509,8 +514,42 @@ mod tests { #[test] fn an_unknown_action_query_value_is_rejected_rather_than_defaulted() { assert_eq!(Action::parse("signin"), Some(Action::SignIn)); - for unknown in ["issue", "rework", "", "SIGNIN", "signin "] { + assert_eq!(Action::parse("issue"), Some(Action::Issue)); + for unknown in ["rework", "", "SIGNIN", "ISSUE", "issue ", "signin "] { assert_eq!(Action::parse(unknown), None, "accepted {unknown:?}"); } } + + /// The variant 0186 carried the slot for: an issue round-trip mints, + /// verifies and reports `Action::Issue` end to end. + #[test] + fn an_issue_pair_round_trips_and_reports_its_action() { + let started = start(KEY, Action::Issue, NOW); + let accepted = accept( + KEY, + &started.state_param, + Some(&started.pending_cookie), + NOW + 1, + ) + .expect("a freshly minted issue pair must verify"); + assert_eq!(accepted.action, Action::Issue); + + // And crossing it with a sign-in cookie is the mismatch the slot + // exists to refuse — two REAL actions now, not the test sentinel. + let state: StateClaims = verify_claims(KEY, CTX_STATE, &started.state_param).unwrap(); + let crossed_pending = sign_claims( + KEY, + CTX_PENDING, + &PendingClaims { + action: Action::SignIn, + nonce: state.nonce.clone(), + verifier: "a-verifier".into(), + exp: state.exp, + }, + ); + assert_eq!( + accept(KEY, &started.state_param, Some(&crossed_pending), NOW).unwrap_err(), + StateError::ActionMismatch + ); + } } diff --git a/packages/prices-api/src/portal/eligibility.rs b/packages/prices-api/src/portal/eligibility.rs new file mode 100644 index 00000000..9689688b --- /dev/null +++ b/packages/prices-api/src/portal/eligibility.rs @@ -0,0 +1,456 @@ +//! Who may be issued a key (task 0189). +//! +//! The whole of the epic's abuse story, per ADR 0010 §3: a key is issuable +//! only by a **member of the Stellar Discord** whose account is **older than a +//! configured minimum** — and both facts are proved *per action*, by a fresh +//! OAuth round-trip, never carried in the session (§8). A signed "eligible" +//! claim would date the verdict to sign-in time; this module's verdicts are +//! dated to the moment the callback holds a fresh token. +//! +//! | Path | Re-auth | Checks | +//! | --- | --- | --- | +//! | Sign in | — | identity only | +//! | Issue a key | **yes** | membership (`pending === false`) + account age | +//! | Reveal / usage | no | session only | +//! | Rework ([0191]) | **yes** | membership only — age is never re-checked | +//! | Revoke ([0192]) | no | session only — a **deliberate exception**: a user must be able to kill a leaked key while Discord is down | +//! +//! Account age is checked only at issuance because an account old enough once +//! is old enough forever; re-checking it on a rework would be noise. +//! +//! # The non-goal, stated so nobody "fixes" it +//! +//! **A user who leaves the guild after issuance keeps their key.** Sign-in +//! proves membership at the moment of issuance and nothing afterwards; reveal +//! and usage never consult Discord at all. What a departed member loses is the +//! right to *rework* the key ([0191]), which re-proves membership when asked. +//! The registry stores no membership data — every check reads Discord live at +//! the moment it matters, which is also why there is nothing here to expire. +//! +//! # Three outcomes, not two +//! +//! [`decide`] answers eligible / not-a-member / too-young / **unknown**, and +//! `Unknown` is a first-class verdict rather than an error: a throttled or +//! down Discord must refuse issuance *without accusing the visitor of +//! non-membership*, because "could not verify" is fixable by waiting and "you +//! are not a member" is an accusation they can only disprove by joining again. +//! Only a confirmed `404` carrying Discord's own "no such membership" code +//! ever reads as not-a-member — see `discord::classify_member_response`. +//! +//! # The two knobs are operator-seeded SSM parameters +//! +//! `/prices/{env}/discord-guild-id` and `/prices/{env}/min-account-age-minutes`, +//! read **at runtime, per action** — never `valueForStringParameter`, which +//! freezes the value into the deployed template, and never a CDK-owned +//! `StringParameter`, which the next `cdk deploy` would silently restore +//! (un-flipping production back to the test guild after [0179] step 4). The +//! same ownership split as the OAuth secret: CDK owns the *names*, the +//! operator owns the *values*. Reads go through the Parameters and Secrets +//! extension like the plan id (`config::fetch_plan_id`), whose in-process +//! cache (~5 min) is the only delay between an operator's `put-parameter` and +//! the running Lambda honouring it — no redeploy. + +use serde::Serialize; + +use super::auth::discord::MemberLookup; + +/// Discord's epoch: 2015-01-01T00:00:00Z, in milliseconds since Unix epoch. +/// The high 42 bits of a snowflake are milliseconds since this instant. +const DISCORD_EPOCH_MS: u64 = 1_420_070_400_000; + +/// Where one eligibility parameter's value comes from. +#[derive(Debug, Clone)] +pub enum ParamSource { + /// A literal value, for local runs and tests. Only constructed from the + /// environment in non-`lambda` builds — see `config::load_portal_eligibility`. + Direct(String), + /// The **name** of an SSM parameter, fetched per action so an operator's + /// change takes effect without a redeploy. + Ssm(String), +} + +impl ParamSource { + /// Resolve the current value, trimmed. + /// + /// Trimmed for the same reason the plan id is: an operator's + /// `echo | aws ssm put-parameter` leaves a trailing newline, and the + /// guild id becomes a URL path segment. + pub async fn resolve(&self) -> Result { + match self { + Self::Direct(value) => Ok(value.trim().to_string()), + Self::Ssm(name) => Ok(fetch_parameter(name).await?.trim().to_string()), + } + } +} + +/// The two operator-tunable knobs, resolved per action. +#[derive(Debug, Clone)] +pub struct EligibilitySettings { + pub guild_id: ParamSource, + pub min_account_age: ParamSource, +} + +impl EligibilitySettings { + /// The guild whose membership gates issuance. + pub async fn guild_id(&self) -> Result { + let id = self.guild_id.resolve().await?; + if id.is_empty() { + return Err(EligibilityError::Empty { + what: "discord-guild-id", + }); + } + Ok(id) + } + + /// The minimum account age, in minutes. + pub async fn min_account_age_minutes(&self) -> Result { + let raw = self.min_account_age.resolve().await?; + raw.parse().map_err(|_| EligibilityError::NotMinutes { + what: "min-account-age-minutes", + }) + } +} + +/// Why a parameter could not be resolved. +/// +/// At cold start (the probe in `config::load_portal_eligibility`) any of these +/// is fatal; at action time they all land in [`Eligibility::Unknown`] — the +/// visitor is refused without accusation, and the log names the real fault. +#[derive(Debug, thiserror::Error)] +pub enum EligibilityError { + #[error("reading SSM parameter `{name}` failed: {message}")] + Fetch { name: String, message: String }, + #[error("the `{what}` parameter is empty")] + Empty { what: &'static str }, + #[error("the `{what}` parameter is not a whole number of minutes")] + NotMinutes { what: &'static str }, +} + +/// Read one parameter through the Parameters and Secrets extension — the same +/// localhost listener, token and in-process cache the mTLS bundle, the OAuth +/// secret and the plan id already use. The extension's cache is what bounds +/// how quickly an operator's change is honoured (~5 min), and is also why a +/// per-action read does not call Systems Manager on a warm container. +#[cfg(feature = "aws-mtls")] +async fn fetch_parameter(name: &str) -> Result { + prices_clickhouse::mtls::fetch_parameter_string(name) + .await + .map_err(|e| EligibilityError::Fetch { + name: name.to_string(), + message: e.to_string(), + }) +} + +#[cfg(not(feature = "aws-mtls"))] +async fn fetch_parameter(name: &str) -> Result { + Err(EligibilityError::Fetch { + name: name.to_string(), + message: "this build has no Parameters and Secrets extension client (build with \ + `--features lambda`, or set PORTAL_GUILD_ID and \ + PORTAL_MIN_ACCOUNT_AGE_MINUTES for a local run)" + .into(), + }) +} + +/// The verdict on one issuance attempt. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub enum Eligibility { + /// A member with `pending == Some(false)` and an old-enough account. + Eligible, + /// Confirmed non-membership (Discord's own 10007/10004 on a 404), or a + /// member who has not cleared Membership Screening (`pending: true` — + /// joining and completing the screening is the same user action as + /// joining, so it renders under the same refusal). + NotMember, + /// The account exists but is younger than the threshold. A *wait*, not a + /// rejection: `wait_secs` is what the page renders, so the copy follows + /// the operator's threshold instead of hard-coding one. + TooYoung { wait_secs: u64 }, + /// Could not verify: Discord answered 401/403/429/5xx or an unrecognised + /// shape, `pending` was absent, or the snowflake would not parse. Refuse, + /// but never claim non-membership. + Unknown, +} + +/// When the account behind `snowflake` was created, in ms since Unix epoch. +/// +/// `(id >> 22) + DISCORD_EPOCH_MS`, per Discord's snowflake layout. Done in +/// `u64` — the task's `BigInt` note is about *JavaScript*, where `Number` +/// loses integer precision above 2^53; a snowflake fits comfortably in 64 +/// bits and this backend never puts one in a float. `None` when the string is +/// not a bare integer, which the caller treats as [`Eligibility::Unknown`]. +pub fn account_created_ms(snowflake: &str) -> Option { + let id: u64 = snowflake.parse().ok()?; + Some((id >> 22) + DISCORD_EPOCH_MS) +} + +/// Combine one membership answer and one account age into a verdict. +/// +/// Pure, so the whole decision table is unit-tested. Precedence: membership +/// first, then age — a non-member's account age is not their problem yet, and +/// "join the server" plus "wait four minutes" as two sequential messages beats +/// both at once. +/// +/// The `pending` rules, each deliberate and each reversible once 0180's +/// measurements exist: +/// +/// - `Some(false)` **passes**. This is true even when an admin waved the user +/// through via `BYPASSES_VERIFICATION` — ADR 0010 reads "must be a member" +/// as "the guild considers them a full member", and second-guessing an +/// admin's bypass would require the `flags` field this service deliberately +/// does not read. +/// - `Some(true)` is **not a member yet** — they joined but have not cleared +/// Membership Screening, and completing it is the same "join the server" +/// action the refusal already names. +/// - `None` is **unknown**, never a pass: the docs' presence guarantee for +/// `pending` is written about gateway events, not this REST route, and 0180 +/// item 2 (which would settle what absence means here) is unmeasured. If +/// measurement shows the field is simply absent on REST, this one arm is +/// what changes — logged loudly (`pending_absent`) so the gap is visible in +/// CloudWatch the first time it fires rather than silently refusing every +/// member. +pub fn decide( + member: &MemberLookup, + snowflake: &str, + min_age_minutes: u64, + now_ms: u64, +) -> Eligibility { + match member { + MemberLookup::NotMember { .. } => return Eligibility::NotMember, + MemberLookup::Unknown { status, detail } => { + tracing::warn!(?status, detail, "membership could not be verified"); + return Eligibility::Unknown; + } + MemberLookup::Member(m) => match m.pending { + Some(false) => {} + Some(true) => return Eligibility::NotMember, + None => { + tracing::warn!( + reason = "pending_absent", + "the member response carried no `pending` field; refusing without \ + accusation — see 0180 item 2 before changing this arm" + ); + return Eligibility::Unknown; + } + }, + } + + let Some(created_ms) = account_created_ms(snowflake) else { + tracing::warn!("a Discord user id did not parse as a snowflake; cannot derive account age"); + return Eligibility::Unknown; + }; + + let old_enough_at = created_ms.saturating_add(min_age_minutes.saturating_mul(60_000)); + if now_ms >= old_enough_at { + return Eligibility::Eligible; + } + Eligibility::TooYoung { + wait_secs: (old_enough_at - now_ms).div_ceil(1000), + } +} + +/// Milliseconds since the Unix epoch, saturating like `state_token::now_secs`: +/// a clock before 1970 makes every account look brand new, which fails closed. +pub fn now_ms() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_millis() as u64) + .unwrap_or(0) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::portal::auth::discord::GuildMember; + + /// A snowflake with a known timestamp: Discord's own documentation example + /// `175928847299117063` decodes to 2016-04-30 11:18:25.796 UTC. + const DOCUMENTED_SNOWFLAKE: &str = "175928847299117063"; + const DOCUMENTED_CREATED_MS: u64 = 1_462_015_105_796; + + fn member(pending: Option) -> MemberLookup { + MemberLookup::Member(GuildMember { pending }) + } + + fn unknown() -> MemberLookup { + MemberLookup::Unknown { + status: None, + detail: "test".into(), + } + } + + #[test] + fn the_snowflake_epoch_math_matches_discords_documented_example() { + assert_eq!( + account_created_ms(DOCUMENTED_SNOWFLAKE), + Some(DOCUMENTED_CREATED_MS) + ); + } + + /// The id in the JSON is a string because it exceeds 2^53 — the reason the + /// task says `BigInt` for JavaScript. In u64 the math is exact; this pins + /// it with an id above 2^53 so a future refactor through `f64` fails here. + #[test] + fn an_id_above_2_pow_53_is_exact_not_rounded() { + let id: u64 = (1 << 55) + 4_194_305; // low bits would vanish in an f64 + let created = account_created_ms(&id.to_string()).unwrap(); + assert_eq!(created, (id >> 22) + DISCORD_EPOCH_MS); + // The nearest f64 to `id` is a different integer; if a float snuck in, + // the shifted result would differ. + assert_ne!((id as f64) as u64, id); + } + + #[test] + fn a_non_numeric_id_is_none_not_a_panic() { + for bad in ["", "abc", "12.5", "-3", "1e10", " 175928847299117063"] { + assert_eq!(account_created_ms(bad), None, "parsed {bad:?}"); + } + } + + #[test] + fn a_cleared_member_with_an_old_account_is_eligible() { + let now = DOCUMENTED_CREATED_MS + 6 * 60_000; + assert_eq!( + decide(&member(Some(false)), DOCUMENTED_SNOWFLAKE, 5, now), + Eligibility::Eligible + ); + } + + /// Exactly at the threshold passes — "older than five minutes" is enforced + /// as `>=` because the boundary instant is not worth a refusal, and the + /// test pins which side the boundary is on so it cannot drift silently. + #[test] + fn the_age_boundary_passes_exactly_at_the_threshold() { + let threshold = DOCUMENTED_CREATED_MS + 5 * 60_000; + assert_eq!( + decide(&member(Some(false)), DOCUMENTED_SNOWFLAKE, 5, threshold), + Eligibility::Eligible + ); + assert_eq!( + decide(&member(Some(false)), DOCUMENTED_SNOWFLAKE, 5, threshold - 1), + Eligibility::TooYoung { wait_secs: 1 } + ); + } + + /// `wait_secs` is a ceiling: 90.001 seconds remaining renders as 91, never + /// as 90-then-refused-again. + #[test] + fn the_wait_is_rounded_up_to_whole_seconds() { + let now = DOCUMENTED_CREATED_MS + 5 * 60_000 - 90_001; + assert_eq!( + decide(&member(Some(false)), DOCUMENTED_SNOWFLAKE, 5, now), + Eligibility::TooYoung { wait_secs: 91 } + ); + } + + /// A changed SSM value changes the verdict with no other input changing — + /// the "tunable without a redeploy" property at the decision layer. + #[test] + fn the_threshold_is_an_input_not_a_constant() { + let now = DOCUMENTED_CREATED_MS + 6 * 60_000; + assert_eq!( + decide(&member(Some(false)), DOCUMENTED_SNOWFLAKE, 5, now), + Eligibility::Eligible + ); + assert_eq!( + decide(&member(Some(false)), DOCUMENTED_SNOWFLAKE, 10, now), + Eligibility::TooYoung { wait_secs: 240 } + ); + assert_eq!( + decide(&member(Some(false)), DOCUMENTED_SNOWFLAKE, 0, now), + Eligibility::Eligible + ); + } + + #[test] + fn a_confirmed_non_member_is_refused_before_age_is_even_computed() { + // The nonsense snowflake would be Unknown if age ran first; the + // membership verdict takes precedence. + assert_eq!( + decide( + &MemberLookup::NotMember { code: 10_007 }, + "not-a-flake", + 5, + 0 + ), + Eligibility::NotMember + ); + } + + #[test] + fn a_pending_member_is_not_a_member_yet() { + let now = DOCUMENTED_CREATED_MS + 6 * 60_000; + assert_eq!( + decide(&member(Some(true)), DOCUMENTED_SNOWFLAKE, 5, now), + Eligibility::NotMember + ); + } + + /// The task's own wording: `pending === undefined` "is handled explicitly + /// and does not silently pass". It is Unknown — refused, not accused. + #[test] + fn an_absent_pending_field_is_unknown_never_a_pass() { + let now = DOCUMENTED_CREATED_MS + 6 * 60_000; + assert_eq!( + decide(&member(None), DOCUMENTED_SNOWFLAKE, 5, now), + Eligibility::Unknown + ); + } + + #[test] + fn an_unverifiable_membership_is_unknown_whatever_the_age() { + let now = DOCUMENTED_CREATED_MS + 6 * 60_000; + assert_eq!( + decide(&unknown(), DOCUMENTED_SNOWFLAKE, 5, now), + Eligibility::Unknown + ); + } + + #[test] + fn an_unparseable_snowflake_is_unknown_not_too_young() { + assert_eq!( + decide(&member(Some(false)), "not-a-snowflake", 5, u64::MAX), + Eligibility::Unknown + ); + } + + #[tokio::test] + async fn a_direct_source_resolves_trimmed() { + let settings = EligibilitySettings { + guild_id: ParamSource::Direct(" 897514728459468821\n".into()), + min_account_age: ParamSource::Direct("5\n".into()), + }; + assert_eq!(settings.guild_id().await.unwrap(), "897514728459468821"); + assert_eq!(settings.min_account_age_minutes().await.unwrap(), 5); + } + + #[tokio::test] + async fn an_empty_or_non_numeric_parameter_is_a_named_error() { + let empty = EligibilitySettings { + guild_id: ParamSource::Direct(" ".into()), + min_account_age: ParamSource::Direct("five".into()), + }; + assert!(matches!( + empty.guild_id().await.unwrap_err(), + EligibilityError::Empty { .. } + )); + assert!(matches!( + empty.min_account_age_minutes().await.unwrap_err(), + EligibilityError::NotMinutes { .. } + )); + } + + /// A build without the extension client refuses an SSM source with a + /// message naming the local seams — the same behaviour `fetch_plan_id` + /// has, asserted here so the two cannot drift apart silently. + #[cfg(not(feature = "aws-mtls"))] + #[tokio::test] + async fn an_ssm_source_in_a_non_extension_build_names_the_local_seams() { + let settings = EligibilitySettings { + guild_id: ParamSource::Ssm("/prices/test/discord-guild-id".into()), + min_account_age: ParamSource::Ssm("/prices/test/min-account-age-minutes".into()), + }; + let error = settings.guild_id().await.unwrap_err(); + assert!(error.to_string().contains("PORTAL_GUILD_ID")); + } +} diff --git a/packages/prices-api/src/portal/keys/mod.rs b/packages/prices-api/src/portal/keys/mod.rs index 03f97e7d..c727fc00 100644 --- a/packages/prices-api/src/portal/keys/mod.rs +++ b/packages/prices-api/src/portal/keys/mod.rs @@ -1,13 +1,19 @@ -//! Issue a key, and show it (task 0187). +//! Reveal a key — and issue one, but only for [0189]'s eligibility-checked +//! callback (task 0187, re-shaped by task 0189). //! -//! Two routes under [`PORTAL_API_PREFIX`](super::PORTAL_API_PREFIX), so they -//! inherit [0183]'s gate exactly as sign-in does: with `PORTAL_ENABLED` false -//! both are an empty `404`, byte-identical to a path that was never deployed. +//! One route under [`PORTAL_API_PREFIX`](super::PORTAL_API_PREFIX), so it +//! inherits [0183]'s gate exactly as sign-in does: with `PORTAL_ENABLED` false +//! it is an empty `404`, byte-identical to a path that was never deployed. //! //! | route | does | //! | --- | --- | -//! | `POST /api-tokens/api/key` | issue — or adopt the one that already exists | -//! | `GET /api-tokens/api/key` | reveal — the same lookup, plus the value | +//! | `GET`/`POST /api-tokens/api/key` | reveal — the lookup, plus the value. **Never creates.** | +//! +//! Issuance itself is [`issue_for`], reachable **only** from the OAuth +//! callback completing an `action=issue` round-trip +//! (`super::auth::issue`) — because issuing requires an eligibility proof +//! (Stellar Discord membership + minimum account age, ADR 0010 §8) that only a +//! fresh Discord token can provide, and a session cookie is not one. //! //! # There is no database, and that is the design //! @@ -18,40 +24,41 @@ //! history; neither is required to answer "where is my key", and whether it is //! ever needed is [0190]'s to justify. //! -//! What follows from that is the shape of everything here: every request is a -//! **reconciliation**, not a lookup of something we wrote down. The flow is -//! list → filter → rank → converge, and it is the same flow for both routes, -//! which is why the task groups them ("reveal is the same lookup as issue"). -//! -//! # Why a `GET` may create +//! # The route is read-only, and that is [0189]'s invariant //! -//! Reveal re-enters the issue flow when there is nothing to reveal, per the -//! task: *"a key deleted by hand in the console otherwise leaves the user with a -//! dead id forever"*. Without a registry there is no way to tell "deleted by -//! hand" from "never issued" — that distinction is precisely what the registry -//! would have stored — so honouring that requirement means a `GET` that can -//! create. +//! Task 0187 shipped this route as one create-capable handler for both verbs +//! ("reveal is the same lookup as issue"), with a documented argument for why +//! a `GET` that creates was safe. [0189] re-derives the question and reverses +//! the answer, because the premise changed: issuing now requires an +//! eligibility proof, so a route reachable with a session cookie alone **must +//! not create** — that is the acceptance criterion "issue is unreachable with +//! a session cookie alone", made structural. The reveal is now a pure lookup: +//! list → filter → rank → read. No create, no attach, no delete — a session +//! cookie can cause **zero control-plane writes**, which also retires 0187's +//! `SameSite=Lax` top-level-navigation concern outright rather than bounding +//! it. //! -//! That is worth stating plainly rather than burying, because `auth/mod.rs` -//! warns against inheriting its CSRF reasoning here. The exposure is a -//! third-party page causing a top-level navigation to this URL, which -//! `SameSite=Lax` does send the session cookie on. What it buys an attacker is -//! bounded to nothing: +//! What that costs, and why it is fine: //! -//! - The flow is **idempotent**. A visitor who has a key gets that same key; a -//! visitor who does not gets the one key they were entitled to press a button -//! for. No amount of forged navigation produces a second key, because the -//! reconciler deletes all but one. -//! - The response is **not readable cross-origin**. A navigation renders JSON in -//! the victim's own tab; the attacker's page cannot read it, and no CORS -//! header here says otherwise. -//! - `POST` is not reachable cross-site at all under `SameSite=Lax`, which only -//! releases the cookie for top-level `GET`. +//! - **A key deleted by hand in the console is no longer resurrected by a +//! reveal.** The caller sees "no key" and the page offers the issue +//! round-trip; one press re-proves eligibility and recreates it. That is +//! strictly better than 0187's behaviour — the recreate now happens behind +//! the gate instead of around it. +//! - **A winner that exists but was never attached** (a crash between create +//! and attach) reveals as a key that answers `403` on `/v1/`. The heal is +//! the same press: [`issue_for`] adopts and attaches it. The reveal must +//! not fix it in place, because attach is a write. +//! - **Duplicates are no longer swept by reveal** — [`issue_for`] still +//! converges them. Until the owner next issues, the reveal answers with the +//! same deterministic winner the issue flow would pick +//! (`naming::choose_winner`), so nothing diverges by waiting. //! -//! So the worst outcome is that a visitor ends up holding the key they could -//! have issued themselves — the same reasoning `auth`'s sign-out uses, and it is -//! restated rather than inherited because the conclusion had to be re-derived -//! for a route that creates a production credential. +//! **A user who has left the Stellar Discord keeps their key, and this route +//! keeps working for them.** Reveal consults the session only — never +//! Discord. That is the epic's stated non-goal, not an oversight: membership +//! is proved at the moment of issuance and nothing afterwards (ADR 0010 §7); +//! what a departed member loses is the right to rework ([0191]). //! //! # Never log a key value //! @@ -84,7 +91,7 @@ use super::auth::secret::OauthSecret; use gateway::{Attachment, Gateway, GatewayError, KeyValue}; use naming::{KeyRecord, choose_winner, exact_matches, key_name, losers}; -/// Issue (`POST`) and reveal (`GET`) share one path — they are one resource. +/// The reveal, on both verbs — see [`key`] for why `POST` answers identically. pub const KEY_PATH: &str = "/api-tokens/api/key"; /// Error code for a caller with no valid session. @@ -93,6 +100,11 @@ const NOT_SIGNED_IN: &str = "not_signed_in"; const KEY_UNAVAILABLE: &str = "key_unavailable"; /// Error code for a deployment with the portal open and no usage plan wired. const KEYS_UNCONFIGURED: &str = "keys_unconfigured"; +/// Error code for a caller who has no key. The same code (and the same +/// envelope-vs-empty-body distinction from [0183]'s gate) as the usage route's, +/// because the frontend reads both: a `404` is "no key" only when this +/// envelope says so. +const NO_KEY: &str = "no_key"; /// How many times the whole flow is re-run when the key it settled on turns out /// to have been deleted underneath it. @@ -122,7 +134,7 @@ const MAX_ATTEMPTS: usize = 2; /// 10s leaves the handler ~5s of the function's budget to serialize an answer /// and for the runtime to send it, and sits far inside API Gateway's own 29s /// cap. -const RECONCILE_DEADLINE: Duration = Duration::from_secs(10); +pub(crate) const RECONCILE_DEADLINE: Duration = Duration::from_secs(10); /// What both handlers need, cloned per request. /// @@ -179,11 +191,11 @@ impl KeysState { } } -/// The two routes, as a `Router` carrying its own state. +/// The route, as a `Router` carrying its own state. /// /// One `.route()` with both verbs, so the path is written once: a second /// `.route()` for the same path would panic at construction, and that is the -/// only thing keeping issue and reveal from drifting apart in the router. +/// only thing keeping the two verbs from drifting apart in the router. pub fn routes(state: KeysState) -> Router { Router::new() .route(KEY_PATH, get(key).post(key)) @@ -211,31 +223,23 @@ struct KeyResponse { name: String, /// The key itself — what goes in `X-API-Key`. value: String, - /// Whether this request created the key, as opposed to finding it. - /// - /// Display only, and honest about a race: two simultaneous first presses can - /// both report `true` while converging on one key, because each created one - /// and one of the two was then reconciled away. - created: bool, } -/// Both verbs. `POST` issues, `GET` reveals, and they are **one handler** -/// because they are one operation. +/// Both verbs, one handler, and both are the **reveal**. /// -/// That is not a shortcut: task 0187 says "reveal is the same lookup as issue", -/// and without a registry it has to be. There is no stored key id to reveal, so -/// a reveal lists, filters, ranks and converges exactly as an issue does, and -/// the only thing left that could differ between them is whether they are -/// allowed to create — which they cannot be, because "deleted by hand" and -/// "never issued" are the same observation. Two functions with identical bodies -/// would have claimed a distinction that does not exist and invited someone to -/// invent one. +/// `POST` used to be the issue; issuance now lives behind the eligibility +/// round-trip (`super::auth::issue`), and this route must not create however +/// it is called. Both verbs stay routed and answer identically so that "a +/// `POST` with a session cookie creates nothing" is a *tested property* of a +/// live route rather than a hole left by an unrouted one — and so the +/// gateway's mapped verbs need no change. async fn key(State(state): State, headers: HeaderMap) -> Response { - ensure_key(&state, &headers).await + reveal(&state, &headers).await } -/// The whole of both routes: authenticate, reconcile, answer. -async fn ensure_key(state: &KeysState, headers: &HeaderMap) -> Response { +/// The whole of the route: authenticate, look up, answer. Read-only — see the +/// module docs for why that is [0189]'s invariant, not an optimisation. +async fn reveal(state: &KeysState, headers: &HeaderMap) -> Response { let Some(oauth) = state.oauth.as_ref() else { return unconfigured(); }; @@ -243,15 +247,14 @@ async fn ensure_key(state: &KeysState, headers: &HeaderMap) -> Response { return unconfigured(); }; - // The session is the authorization for this whole route. There is no API - // key to present — the caller is here to get one — so the signed cookie is - // the only thing standing between a stranger and a production credential, - // and `PORTAL_ENABLED` is the only thing standing in front of that until - // [0189]'s eligibility gate lands. + // The session is the authorization for this route — and for this route it + // is enough, because a reveal only shows the caller what already belongs + // to them. Creating is what needs more (an eligibility proof via the + // issue round-trip), which is why nothing below can create. let Some(session) = super::auth::current_session(oauth, headers) else { return no_store(errors::unauthorized_with( NOT_SIGNED_IN, - "sign in with Discord before issuing an API key", + "sign in with Discord before asking for your API key", )); }; @@ -259,27 +262,24 @@ async fn ensure_key(state: &KeysState, headers: &HeaderMap) -> Response { // while the signing key is intact — Discord ids are digits and the cookie is // signed — which is exactly why it is checked: this is the last line if that // stops being true, and the alternative is an attacker-chosen `nameQuery` - // aimed at the reconciler's `DeleteApiKey`. + // aimed at the control plane. let Some(name) = key_name(&session.sub) else { - tracing::warn!("a session carried a user id that is not a snowflake; refusing to issue"); + tracing::warn!("a session carried a user id that is not a snowflake; refusing"); return no_store(errors::unauthorized_with( NOT_SIGNED_IN, - "sign in with Discord before issuing an API key", + "sign in with Discord before asking for your API key", )); }; - // The deadline wraps the whole reconciliation, not each call inside it — - // see `RECONCILE_DEADLINE`. Elapsing is a `503`: the work may well have been - // half-done (a key created, a duplicate deleted), the flow is idempotent, and - // the next request reconciles whatever this one left. Saying so is the whole - // point — the alternative is Lambda killing the invocation with no answer at - // all. - let reconciled = match tokio::time::timeout(state.deadline, reconcile(gateway, &name)).await { - Ok(reconciled) => reconciled, + // The deadline wraps the whole lookup — `list_named` alone can walk pages, + // each with its own per-call budget. Elapsing is a `503` rather than a + // Lambda-killed invocation with no response at all. + let looked_up = match tokio::time::timeout(state.deadline, lookup(gateway, &name)).await { + Ok(looked_up) => looked_up, Err(_elapsed) => { tracing::error!( deadline_secs = state.deadline.as_secs_f32(), - "portal key reconciliation ran out of time" + "portal key lookup ran out of time" ); return no_store(errors::service_unavailable( KEY_UNAVAILABLE, @@ -288,50 +288,46 @@ async fn ensure_key(state: &KeysState, headers: &HeaderMap) -> Response { } }; - match reconciled { - Ok(Some(outcome)) => { - tracing::info!( - key_id = %outcome.record.id, - created = outcome.created, - "portal issued or revealed an API key" - ); + match looked_up { + Ok(Some((record, value))) => { + tracing::info!(key_id = %record.id, "portal revealed an API key"); // This response proves a key exists, so a cached "no key" on the - // usage route is now false — for the page's own refetch after the - // press, and for any reload inside that cache's TTL. Evicted on - // every success rather than only on `created`: an ADOPTED key - // (console-created, or a create another invocation raced in) also - // arrives with "no key" plausibly cached, and the eviction is - // narrow enough that firing it spuriously costs nothing. + // usage route is now false. An in-process eviction, not a + // control-plane write — the read-only invariant is about what a + // session cookie can make AWS do. if let Some(cache) = &state.usage_cache { cache.invalidate_no_key(&session.sub); } no_store( Json(KeyResponse { - key_id: outcome.record.id, - name: outcome.record.name, + key_id: record.id, + name: record.name, // The one call site of `expose`, and the reason the type // exists: everything else in this module can only hold the // value, never read it. - value: outcome.value.expose().to_string(), - created: outcome.created, + value: value.expose().to_string(), }) .into_response(), ) } - // Every attempt found a key and then lost it before reading its value. - Ok(None) => { - tracing::warn!( - attempts = MAX_ATTEMPTS, - "a key was deleted underneath every issue attempt" - ); - no_store(errors::service_unavailable( - KEY_UNAVAILABLE, - "your key is being changed by something else right now; try again", - )) - } + // Nothing under this name — never issued, deleted by hand, or (rarely) + // deleted between the list and the read. All three answer the same + // envelope, because without a registry they are the same observation, + // and all three are fixed the same way: the issue round-trip. + Ok(None) => no_store( + ( + StatusCode::NOT_FOUND, + Json(errors::ErrorEnvelope { + code: NO_KEY, + message: "you have no API key yet; issue one from the portal".into(), + details: None, + }), + ) + .into_response(), + ), Err(error) => { // `error` cannot carry a key value — see `gateway::sdk_message`. - tracing::error!(error = %error, "portal key issuance failed"); + tracing::error!(error = %error, "portal key reveal failed"); no_store( ( StatusCode::BAD_GATEWAY, @@ -347,10 +343,96 @@ async fn ensure_key(state: &KeysState, headers: &HeaderMap) -> Response { } } +/// The read-only lookup: list, filter, rank, read. Nothing here mutates. +/// +/// `Ok(None)` is "no key to reveal" — including the raced case where the +/// winner was listed and deleted before its value could be read. The reveal +/// answers `no_key` for it rather than retrying into a create, because +/// retrying into a create is exactly what this route gave up. +async fn lookup( + gateway: &Gateway, + name: &str, +) -> Result, GatewayError> { + let candidates = exact_matches(gateway.list_named(name).await?, name); + let Some(winner) = choose_winner(&candidates).cloned() else { + return Ok(None); + }; + match gateway.value_of(&winner.id).await? { + Some(value) => Ok(Some((winner, value))), + None => Ok(None), + } +} + +/// What the eligibility-checked issue round-trip needs to know. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum IssueOutcome { + /// A key exists, is on the free plan, and its value is readable — created + /// now or adopted. The value is deliberately not carried: the callback + /// that consumes this answers with a redirect, and a credential must + /// never ride in a `Location`. + Issued, + /// The control plane would not produce a usable key — an error, a + /// deadline, or a key deleted underneath every attempt. Logged inside + /// with the same distinctions 0187's handler drew; the visitor is told to + /// try again either way. + Failed, +} + +/// Issue (or adopt) the key for `sub` — the create-capable flow, reachable +/// only from `super::auth::issue` after an eligibility proof. +/// +/// This is 0187's reconciler unchanged — list → filter → rank → create if +/// missing → attach → sweep duplicates → read — behind the same deadline its +/// handler used, so the callback cannot outlive the Lambda's budget either. +pub(crate) async fn issue_for(gateway: &Gateway, sub: &str, deadline: Duration) -> IssueOutcome { + // Same guard as the reveal, for the same reason — and here it also stands + // in front of `DeleteApiKey`. + let Some(name) = key_name(sub) else { + tracing::warn!("an issue round-trip carried a user id that is not a snowflake; refusing"); + return IssueOutcome::Failed; + }; + + match tokio::time::timeout(deadline, reconcile(gateway, &name)).await { + Ok(Ok(Some(outcome))) => { + tracing::info!( + key_id = %outcome.record.id, + created = outcome.created, + "portal issued an API key" + ); + IssueOutcome::Issued + } + // Every attempt found a key and then lost it before reading its value. + Ok(Ok(None)) => { + tracing::warn!( + attempts = MAX_ATTEMPTS, + "a key was deleted underneath every issue attempt" + ); + IssueOutcome::Failed + } + Ok(Err(error)) => { + // `error` cannot carry a key value — see `gateway::sdk_message`. + tracing::error!(error = %error, "portal key issuance failed"); + IssueOutcome::Failed + } + Err(_elapsed) => { + tracing::error!( + deadline_secs = deadline.as_secs_f32(), + "portal key issuance ran out of time" + ); + IssueOutcome::Failed + } + } +} + /// The result of a successful reconciliation. +/// +/// Deliberately does **not** carry the key value. The issue flow's caller is a +/// redirect (`auth::issue`), and a value it cannot receive is a value that +/// cannot leak into a `Location` or a log by later mistake; the reveal reads +/// the value through its own read-only [`lookup`]. The reconciler still calls +/// `value_of` where readability is what is being verified. struct Outcome { record: KeyRecord, - value: KeyValue, created: bool, } @@ -377,12 +459,14 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew // are argued for in `naming`. let existing = exact_matches(gateway.list_named(name).await?, name); - let mut created: Option<(KeyRecord, KeyValue)> = None; + let mut created: Option = None; let mut candidates = existing; - // Step 2: nothing yet — create one. + // Step 2: nothing yet — create one. The value the create answers with is + // dropped on purpose — see `Outcome`; a successful create is itself the + // proof the key is readable. if candidates.is_empty() { - let (record, value) = gateway.create(name).await?; + let (record, _value) = gateway.create(name).await?; // Re-list rather than returning what we just made. Two simultaneous // first presses both find nothing and both create, so the list is the @@ -405,7 +489,6 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew } return Ok(Some(Outcome { record, - value, created: true, })); } @@ -426,7 +509,7 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew if !candidates.iter().any(|c| c.id == record.id) { candidates.push(record.clone()); } - created = Some((record, value)); + created = Some(record); } // Step 3: one survivor. @@ -515,26 +598,25 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew } } - // Step 6: the value. Free if we just created the winner; otherwise a read. - if let Some((record, value)) = created + // Step 6: readability. Free if we just created the winner; otherwise a + // read — not to hand the value out (see `Outcome`), but to prove the key + // the caller will be sent to reveal actually answers. + if let Some(record) = created && record.id == winner.id { return Ok(Some(Outcome { record: winner, - value, created: true, })); } // `None` here is the raced deletion: the winner existed when it was listed // and does not now. The caller re-runs the whole flow, which will adopt - // whatever survived or create a replacement — this is the "not returned as a - // dead id" property, and the reason a reveal is a reconciliation rather than - // a lookup. + // whatever survived or create a replacement — this is the "not handed out + // as a dead id" property. match gateway.value_of(&winner.id).await? { - Some(value) => Ok(Some(Outcome { + Some(_value) => Ok(Some(Outcome { record: winner, - value, created: false, })), None => Ok(None), diff --git a/packages/prices-api/src/portal/mod.rs b/packages/prices-api/src/portal/mod.rs index 288046da..787fca00 100644 --- a/packages/prices-api/src/portal/mod.rs +++ b/packages/prices-api/src/portal/mod.rs @@ -43,6 +43,7 @@ //! the Lambda already loads) and a different task — do not reach for it here. pub mod auth; +pub mod eligibility; pub mod keys; pub mod usage; @@ -122,38 +123,49 @@ pub fn apply(router: Router, config: &AppConfig) -> Router { .route(CONFIG_PATH, get(config_handler)) .with_state(gate.clone()); - // Sign-in (task 0186), merged the same way and for the same reason. Mounted - // UNCONDITIONALLY, including when no OAuth credentials were loaded: the - // handlers answer `503` in that case rather than the routes silently not - // existing, so a deployment that opens the portal without provisioning the - // secret says so instead of looking like a portal with no sign-in. While the - // portal is closed the gate below makes the distinction moot — every path - // here is the same empty `404` as an unrouted one. - let sign_in = auth::routes(auth::AuthState::new( - config.portal_oauth.clone(), - config.portal_endpoints.clone(), - )); - // Usage against quota (task 0188), merged the same way and mounted under - // the same conditions as sign-in: unconditionally, answering `503` when - // nothing is provisioned rather than not existing. It shares the key - // routes' control-plane client — usage is scoped to + // the same conditions as everything below: unconditionally, answering + // `503` when nothing is provisioned rather than not existing. It shares + // the key routes' control-plane client — usage is scoped to // `(usagePlanId, apiKeyId)` and the key id comes from the same lookup — // but carries a state of its own, because it also owns the in-process // cache that keeps dashboard refreshes off the account-wide control-plane - // budget. Built before the key routes so they can hold the cache handle - // below. + // budget. Built first so sign-in and the key routes can hold the cache + // handle below. let usage_state = usage::UsageState::new(config.portal_oauth.clone(), config.portal_keys.clone()); let usage_cache = usage_state.cache_handle(); let usage = usage::routes(usage_state); - // Self-service API keys (task 0187), merged the same way and mounted under - // the same conditions: unconditionally, answering `503` when nothing is - // provisioned rather than not existing. The state carries the OAuth secret - // because the session cookie is what authorizes a key — there is no API key - // to present on the route whose job is to hand one out. The usage-cache - // handle lets a successful issue evict a cached "no key" (task 0188). + // Sign-in (task 0186) and the eligibility-checked issue round-trip + // (task 0189), merged the same way and for the same reason. Mounted + // UNCONDITIONALLY, including when no OAuth credentials were loaded: the + // handlers answer `503` in that case rather than the routes silently not + // existing, so a deployment that opens the portal without provisioning the + // secret says so instead of looking like a portal with no sign-in. While the + // portal is closed the gate below makes the distinction moot — every path + // here is the same empty `404` as an unrouted one. + // + // The issue deps carry the control-plane client and the usage-cache handle + // because the `action=issue` callback is where a key is actually created + // (`keys::issue_for`) — the key ROUTE below is read-only, which is what + // makes "issue is unreachable with a session cookie alone" structural. + let sign_in = auth::routes( + auth::AuthState::new(config.portal_oauth.clone(), config.portal_endpoints.clone()) + .with_issue(auth::issue::IssueDeps::new( + config.portal_keys.clone(), + Some(usage_cache.clone()), + config.portal_eligibility.clone(), + )), + ); + + // The key reveal (task 0187, read-only since task 0189), merged the same + // way and mounted under the same conditions: unconditionally, answering + // `503` when nothing is provisioned rather than not existing. The state + // carries the OAuth secret because the session cookie is what authorizes a + // reveal — showing the caller what already belongs to them, which is why a + // session suffices here and does not for the issue above. The usage-cache + // handle lets a successful reveal evict a cached "no key" (task 0188). let api_keys = keys::routes( keys::KeysState::new(config.portal_oauth.clone(), config.portal_keys.clone()) .with_usage_cache(usage_cache), diff --git a/packages/prices-api/tests/auth.rs b/packages/prices-api/tests/auth.rs index 5f3469a1..62e1de9b 100644 --- a/packages/prices-api/tests/auth.rs +++ b/packages/prices-api/tests/auth.rs @@ -30,6 +30,7 @@ fn armed_config_with_portal(portal_enabled: bool) -> AppConfig { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, } } diff --git a/packages/prices-api/tests/common/mock_discord.rs b/packages/prices-api/tests/common/mock_discord.rs new file mode 100644 index 00000000..1a8b9590 --- /dev/null +++ b/packages/prices-api/tests/common/mock_discord.rs @@ -0,0 +1,248 @@ +//! A mock Discord on loopback, shared by the sign-in suite +//! (`tests/portal_auth.rs`) and the issue round-trip suite +//! (`tests/portal_issue.rs`) via `#[path]`, the same way +//! `tests/portal_keys/harness.rs` is shared. +//! +//! Serves the three routes the service calls — `POST /oauth2/token`, +//! `GET /users/@me`, and `GET /users/@me/guilds/{guild}/member` (task 0189) — +//! and **records what it received**, so tests assert on the request the code +//! actually made: the client secret in the form body, the PKCE verifier, the +//! bearer token and guild id on the member call. +//! +//! # Why a mock server rather than a trait +//! +//! Injecting a `DiscordClient` trait would let the tests skip the HTTP layer +//! entirely — and the HTTP layer is where the requirements live: that the +//! client secret goes in the form body and not the URL, that the verifier sent +//! is the one the challenge was derived from, that the granted scope is +//! checked, and that a member `404` is only "not a member" when the JSON +//! `code` says so. A fake satisfying a trait proves none of them. +// Each binary uses a subset of these helpers, so "unused" here is per-binary +// noise rather than dead code — the same reason `portal_keys/harness.rs` says so. +#![allow(dead_code)] + +use std::net::SocketAddr; +use std::sync::{Arc, Mutex}; + +use axum::extract::{Path, State}; +use axum::http::{HeaderMap, StatusCode, header}; +use axum::response::IntoResponse; +use axum::routing::{get, post}; +use axum::{Json, Router}; +use serde_json::json; + +/// The scope pair production requests — what the mock grants unless a test +/// overrides it to model registration drift. +pub const GRANTED_SCOPE: &str = "identify guilds.members.read"; + +/// The mock user's snowflake: 2017-04-02, comfortably older than any sane +/// threshold, so age never interferes with a membership-focused test. +pub const USER_ID: &str = "308994132968210433"; +pub const USER_NAME: &str = "adam"; + +/// How `GET /users/@me/guilds/{guild}/member` answers. +#[derive(Clone)] +pub enum MemberReply { + /// `200` with a member object; `pending` present or absent as given. + Member { pending: Option }, + /// `404` with Discord's error envelope carrying this JSON `code` + /// (10007 Unknown Member, 10004 Unknown Guild — or anything else, to + /// model an unrecognised shape). + NotFound { code: u64 }, + /// A bare status — 429, 500, 403… — with a non-envelope body. + Status(StatusCode), + /// `200` whose body is not JSON at all. + Malformed, +} + +/// What the mock saw, so the tests can assert on the request rather than only +/// on the response. +#[derive(Default)] +pub struct Recorded { + /// The form body of the last `POST /oauth2/token`, decoded. + pub token_form: Vec<(String, String)>, + /// The `Authorization` header of the last `GET /users/@me`. + pub bearer: Option, + /// How many code exchanges were attempted. + pub exchanges: usize, + /// How many membership lookups were attempted. + pub member_calls: usize, + /// The `Authorization` header of the last membership lookup. + pub member_bearer: Option, + /// The `{guild}` path segment of the last membership lookup. + pub member_guild: Option, +} + +#[derive(Clone)] +struct MockState { + recorded: Arc>, + granted_scope: String, + token_status: Option, + member: MemberReply, + user_id: String, +} + +pub struct MockDiscord { + pub base: String, + pub recorded: Arc>, +} + +impl MockDiscord { + /// The common case: the production scope pair granted, the member in good + /// standing (`pending: false`), the default user. + pub async fn start(granted_scope: &str, token_status: Option) -> Self { + Self::start_with( + granted_scope, + token_status, + MemberReply::Member { + pending: Some(false), + }, + USER_ID, + ) + .await + } + + /// Full control, for the issue suite: what the member route answers, and + /// which snowflake `/users/@me` reports (a freshly minted one drives the + /// too-young refusal). + pub async fn start_with( + granted_scope: &str, + token_status: Option, + member: MemberReply, + user_id: &str, + ) -> Self { + let recorded = Arc::new(Mutex::new(Recorded::default())); + let state = MockState { + recorded: recorded.clone(), + granted_scope: granted_scope.to_string(), + token_status, + member, + user_id: user_id.to_string(), + }; + + let router = Router::new() + .route("/oauth2/token", post(token)) + .route("/users/@me", get(current_user)) + .route("/users/@me/guilds/{guild}/member", get(guild_member)) + .with_state(state); + + // Port 0: the OS picks a free one, so the suite can run in parallel + // with itself and with anything else on the machine. + let listener = tokio::net::TcpListener::bind::(([127, 0, 0, 1], 0).into()) + .await + .expect("the mock must bind to loopback"); + let base = format!("http://{}", listener.local_addr().unwrap()); + tokio::spawn(async move { + let _ = axum::serve(listener, router).await; + }); + + Self { base, recorded } + } + + pub fn form_field(&self, name: &str) -> Option { + self.recorded + .lock() + .unwrap() + .token_form + .iter() + .find(|(k, _)| k == name) + .map(|(_, v)| v.clone()) + } + + pub fn exchanges(&self) -> usize { + self.recorded.lock().unwrap().exchanges + } + + pub fn member_calls(&self) -> usize { + self.recorded.lock().unwrap().member_calls + } + + pub fn member_bearer(&self) -> Option { + self.recorded.lock().unwrap().member_bearer.clone() + } + + pub fn member_guild(&self) -> Option { + self.recorded.lock().unwrap().member_guild.clone() + } +} + +async fn token(State(state): State, body: String) -> axum::response::Response { + { + let mut recorded = state.recorded.lock().unwrap(); + recorded.token_form = form_urlencoded::parse(body.as_bytes()) + .into_owned() + .collect(); + recorded.exchanges += 1; + } + if let Some(status) = state.token_status { + return (status, "upstream said no").into_response(); + } + Json(json!({ + "access_token": "an-access-token", + "token_type": "Bearer", + "expires_in": 604800, + "refresh_token": "a-refresh-token", + "scope": state.granted_scope, + })) + .into_response() +} + +async fn current_user( + State(state): State, + headers: HeaderMap, +) -> Json { + state.recorded.lock().unwrap().bearer = headers + .get(header::AUTHORIZATION) + .and_then(|v| v.to_str().ok()) + .map(str::to_string); + Json(json!({ + "id": state.user_id, + "username": USER_NAME, + "discriminator": "0", + "global_name": "Adam", + // Fields this service must ignore rather than carry anywhere. + "email": "someone@example.com", + "verified": true, + })) +} + +async fn guild_member( + State(state): State, + Path(guild): Path, + headers: HeaderMap, +) -> axum::response::Response { + { + let mut recorded = state.recorded.lock().unwrap(); + recorded.member_calls += 1; + recorded.member_bearer = headers + .get(header::AUTHORIZATION) + .and_then(|v| v.to_str().ok()) + .map(str::to_string); + recorded.member_guild = Some(guild); + } + match &state.member { + MemberReply::Member { pending } => { + // The full-ish member object, so the code's "read only `pending`" + // rule is exercised against fields it must ignore. + let mut body = json!({ + "roles": ["1"], + "joined_at": "2020-01-01T00:00:00.000000+00:00", + "deaf": false, + "mute": false, + "flags": 0, + "user": { "id": USER_ID, "username": USER_NAME }, + }); + if let Some(pending) = pending { + body["pending"] = json!(pending); + } + Json(body).into_response() + } + MemberReply::NotFound { code } => ( + StatusCode::NOT_FOUND, + Json(json!({ "message": "Unknown", "code": code })), + ) + .into_response(), + MemberReply::Status(status) => (*status, "not an envelope").into_response(), + MemberReply::Malformed => "definitely not json".into_response(), + } +} diff --git a/packages/prices-api/tests/common/mod.rs b/packages/prices-api/tests/common/mod.rs index 36b51bc4..cfb3f35b 100644 --- a/packages/prices-api/tests/common/mod.rs +++ b/packages/prices-api/tests/common/mod.rs @@ -32,6 +32,7 @@ pub fn app_without_ch() -> Router { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, }; app(&config, AppState::without_ch()) } diff --git a/packages/prices-api/tests/endpoints_it.rs b/packages/prices-api/tests/endpoints_it.rs index 62313763..c4f2c6dc 100644 --- a/packages/prices-api/tests/endpoints_it.rs +++ b/packages/prices-api/tests/endpoints_it.rs @@ -124,6 +124,7 @@ fn config() -> AppConfig { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, } } diff --git a/packages/prices-api/tests/health.rs b/packages/prices-api/tests/health.rs index 9f6b4c84..7117f93c 100644 --- a/packages/prices-api/tests/health.rs +++ b/packages/prices-api/tests/health.rs @@ -25,6 +25,7 @@ fn test_config() -> AppConfig { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, } } diff --git a/packages/prices-api/tests/list_it.rs b/packages/prices-api/tests/list_it.rs index e87bcc88..a65fc1e9 100644 --- a/packages/prices-api/tests/list_it.rs +++ b/packages/prices-api/tests/list_it.rs @@ -159,6 +159,7 @@ fn config() -> AppConfig { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, } } diff --git a/packages/prices-api/tests/ohlcv_it.rs b/packages/prices-api/tests/ohlcv_it.rs index 9d831dac..1cb09df0 100644 --- a/packages/prices-api/tests/ohlcv_it.rs +++ b/packages/prices-api/tests/ohlcv_it.rs @@ -103,6 +103,7 @@ fn config() -> AppConfig { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, } } diff --git a/packages/prices-api/tests/openapi.rs b/packages/prices-api/tests/openapi.rs index 164fcc92..eb580927 100644 --- a/packages/prices-api/tests/openapi.rs +++ b/packages/prices-api/tests/openapi.rs @@ -64,6 +64,7 @@ fn config_with(base_url: Option<&str>, api_keys: Vec) -> AppConfig { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, } } diff --git a/packages/prices-api/tests/portal.rs b/packages/prices-api/tests/portal.rs index 28a3dd54..254a58ed 100644 --- a/packages/prices-api/tests/portal.rs +++ b/packages/prices-api/tests/portal.rs @@ -35,6 +35,7 @@ fn config_with_keys(portal_enabled: bool, api_keys: Vec) -> AppConfig { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, } } diff --git a/packages/prices-api/tests/portal_auth.rs b/packages/prices-api/tests/portal_auth.rs index f036b338..a500f5ae 100644 --- a/packages/prices-api/tests/portal_auth.rs +++ b/packages/prices-api/tests/portal_auth.rs @@ -16,17 +16,12 @@ //! them. The mock records what it actually received, and the assertions are //! against that. //! -//! The mock is [`MockDiscord`]: an axum app on an ephemeral port, serving -//! `/oauth2/token` and `/users/@me`, pointed at by `DISCORD_API_BASE`. +//! The mock is [`MockDiscord`] (`tests/common/mock_discord.rs`, shared with +//! the issue round-trip suite): an axum app on an ephemeral port, serving +//! `/oauth2/token`, `/users/@me` and the member route. -use std::net::SocketAddr; -use std::sync::{Arc, Mutex}; - -use axum::extract::State; +use axum::Router; use axum::http::{HeaderMap, StatusCode, header}; -use axum::response::IntoResponse; -use axum::routing::{get, post}; -use axum::{Json, Router}; use prices_api::portal::auth::{ CALLBACK_PATH, LOGIN_PATH, LOGOUT_PATH, ME_PATH, cookies, crypto, discord::Endpoints, secret::OauthSecret, session::Session, state_token, @@ -35,114 +30,9 @@ use prices_api::{AppConfig, AppState, app}; use serde_json::{Value, json}; use tower::ServiceExt; -// --------------------------------------------------------------------------- -// Mock Discord -// --------------------------------------------------------------------------- - -/// What the mock saw, so the tests can assert on the request rather than only on -/// the response. -#[derive(Default)] -struct Recorded { - /// The form body of the last `POST /oauth2/token`, decoded. - token_form: Vec<(String, String)>, - /// The `Authorization` header of the last `GET /users/@me`. - bearer: Option, - /// How many code exchanges were attempted. - exchanges: usize, -} - -#[derive(Clone)] -struct MockState { - recorded: Arc>, - /// Scope the mock claims to have granted. Overridden by one test. - granted_scope: String, - /// When set, `/oauth2/token` answers with this status instead of a token. - token_status: Option, -} - -struct MockDiscord { - base: String, - recorded: Arc>, -} - -impl MockDiscord { - async fn start(granted_scope: &str, token_status: Option) -> Self { - let recorded = Arc::new(Mutex::new(Recorded::default())); - let state = MockState { - recorded: recorded.clone(), - granted_scope: granted_scope.to_string(), - token_status, - }; - - let router = Router::new() - .route("/oauth2/token", post(token)) - .route("/users/@me", get(current_user)) - .with_state(state); - - // Port 0: the OS picks a free one, so the suite can run in parallel with - // itself and with anything else on the machine. - let listener = tokio::net::TcpListener::bind::(([127, 0, 0, 1], 0).into()) - .await - .expect("the mock must bind to loopback"); - let base = format!("http://{}", listener.local_addr().unwrap()); - tokio::spawn(async move { - let _ = axum::serve(listener, router).await; - }); - - Self { base, recorded } - } - - fn form_field(&self, name: &str) -> Option { - self.recorded - .lock() - .unwrap() - .token_form - .iter() - .find(|(k, _)| k == name) - .map(|(_, v)| v.clone()) - } - - fn exchanges(&self) -> usize { - self.recorded.lock().unwrap().exchanges - } -} - -async fn token(State(state): State, body: String) -> axum::response::Response { - { - let mut recorded = state.recorded.lock().unwrap(); - recorded.token_form = form_urlencoded::parse(body.as_bytes()) - .into_owned() - .collect(); - recorded.exchanges += 1; - } - if let Some(status) = state.token_status { - return (status, "upstream said no").into_response(); - } - Json(json!({ - "access_token": "an-access-token", - "token_type": "Bearer", - "expires_in": 604800, - "refresh_token": "a-refresh-token", - "scope": state.granted_scope, - })) - .into_response() -} - -async fn current_user(State(state): State, headers: HeaderMap) -> Json { - state.recorded.lock().unwrap().bearer = headers - .get(header::AUTHORIZATION) - .and_then(|v| v.to_str().ok()) - .map(str::to_string); - Json(json!({ - "id": "308994132968210433", - "username": "adam", - "discriminator": "0", - "global_name": "Adam", - // Fields this service must ignore rather than carry anywhere. - "email": "someone@example.com", - "verified": true, - })) -} +#[path = "common/mock_discord.rs"] +mod mock_discord; +use mock_discord::{GRANTED_SCOPE, MockDiscord}; // --------------------------------------------------------------------------- // Router under test @@ -210,6 +100,7 @@ fn build_app(portal_enabled: bool, endpoints: Endpoints) -> Router { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, }; app(&config, AppState::without_ch()) } @@ -413,7 +304,7 @@ async fn a_closed_portal_answers_a_well_formed_callback_the_same_as_a_bare_one() // --------------------------------------------------------------------------- #[tokio::test] -async fn login_redirects_to_discord_asking_for_identify_and_nothing_else() { +async fn login_redirects_to_discord_asking_for_the_two_scopes_and_nothing_else() { let open = signed_in_app(true); let reply = fetch(&open, LOGIN_PATH, &[]).await; @@ -436,7 +327,9 @@ async fn login_redirects_to_discord_asking_for_identify_and_nothing_else() { .unwrap_or_default() }; - assert_eq!(field("scope"), "identify"); + // The pair, verbatim (task 0189) — and never `guilds` or `email`, which + // ADR 0010 refuses outright. + assert_eq!(field("scope"), "identify guilds.members.read"); assert_eq!(field("response_type"), "code"); assert_eq!(field("code_challenge_method"), "S256"); assert_eq!(field("redirect_uri"), REDIRECT_URI); @@ -491,8 +384,9 @@ async fn two_logins_produce_two_different_states() { } /// The action slot is verified at the door as well as in the signature, so an -/// action this build does not implement cannot start a round-trip that the -/// callback would then have to decide what to do with. +/// action this build does not implement — 0191's `rework`, arriving early — +/// cannot start a round-trip that the callback would then have to decide what +/// to do with. #[tokio::test] async fn login_refuses_an_action_it_does_not_implement() { let open = signed_in_app(true); @@ -502,18 +396,31 @@ async fn login_refuses_an_action_it_does_not_implement() { .status, StatusCode::SEE_OTHER ); - let refused = fetch(&open, &format!("{LOGIN_PATH}?action=issue"), &[]).await; + let refused = fetch(&open, &format!("{LOGIN_PATH}?action=rework"), &[]).await; assert_eq!(refused.status, StatusCode::BAD_REQUEST); assert!(refused.set_cookies().is_empty()); } +/// `action=issue` IS implemented (task 0189) — but on a deployment with no +/// control plane or eligibility parameters wired it is refused before the +/// visitor is sent to Discord, under the same code the key routes use for the +/// same fault. A round-trip that can only ever end in "failed" must not start. +#[tokio::test] +async fn login_refuses_an_issue_round_trip_on_an_unwired_deployment() { + let open = signed_in_app(true); + let refused = fetch(&open, &format!("{LOGIN_PATH}?action=issue"), &[]).await; + assert_eq!(refused.status, StatusCode::SERVICE_UNAVAILABLE); + assert_eq!(refused.json()["code"], "keys_unconfigured"); + assert!(refused.set_cookies().is_empty()); +} + // --------------------------------------------------------------------------- // /auth/callback — the happy path // --------------------------------------------------------------------------- #[tokio::test] async fn a_complete_round_trip_signs_the_visitor_in() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); let started = start_login(&open).await; @@ -556,6 +463,11 @@ async fn a_complete_round_trip_signs_the_visitor_in() { // And the pending cookie is gone — the replay defence, on the wire. assert!(reply.clears(cookies::PENDING_COOKIE)); + // Sign-in checks identity only — the membership route is the ISSUE + // round-trip's, and a sign-in that consulted it would be re-inventing the + // session-carried eligibility ADR 0010 §8 forbids. + assert_eq!(mock.member_calls(), 0); + // The exchange really happened, with the client secret in the BODY and the // PKCE verifier that matches the challenge sent at login. assert_eq!(mock.exchanges(), 1); @@ -586,7 +498,7 @@ async fn a_complete_round_trip_signs_the_visitor_in() { /// backend's side: after the round-trip, `/auth/me` reports both. #[tokio::test] async fn me_reports_the_username_and_id_after_a_round_trip() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); let started = start_login(&open).await; @@ -617,7 +529,7 @@ async fn me_reports_the_username_and_id_after_a_round_trip() { /// outlives the request. #[tokio::test] async fn no_discord_token_survives_the_callback() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); let started = start_login(&open).await; @@ -670,7 +582,7 @@ async fn no_discord_token_survives_the_callback() { /// the caller sees, and that no session comes out of it. #[tokio::test] async fn a_mismatched_state_is_rejected_and_issues_no_session() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); // Two independent logins. Present one browser's cookie with the other's @@ -700,7 +612,7 @@ async fn a_mismatched_state_is_rejected_and_issues_no_session() { #[tokio::test] async fn a_callback_with_no_pending_cookie_is_rejected() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); let state = start_login(&open).await.state; @@ -716,7 +628,7 @@ async fn a_callback_with_no_pending_cookie_is_rejected() { /// who captured the URL — the `code` and `state` alone are not enough. #[tokio::test] async fn replaying_a_callback_url_after_the_cookie_is_cleared_is_rejected() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); let started = start_login(&open).await; @@ -740,7 +652,7 @@ async fn replaying_a_callback_url_after_the_cookie_is_cleared_is_rejected() { #[tokio::test] async fn a_forged_state_signed_with_another_key_is_rejected() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); let pending = start_login(&open).await.pending; @@ -765,7 +677,7 @@ async fn a_forged_state_signed_with_another_key_is_rejected() { /// signed with their key and not ours. #[tokio::test] async fn a_self_signed_pair_is_rejected() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); let forged = state_token::start( @@ -829,7 +741,7 @@ async fn a_cancelled_sign_in_returns_to_the_portal_saying_so() { async fn an_unverifiable_callback_cannot_cancel_someone_elses_sign_in() { // Needs a working Discord: the point is that the victim's own callback // still completes, which means it has to reach the token exchange. - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); for attack in [ @@ -876,7 +788,7 @@ async fn an_unverifiable_callback_cannot_cancel_someone_elses_sign_in() { /// be gone. #[tokio::test] async fn a_verified_callback_always_drops_the_pending_cookie() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); // Success. @@ -903,7 +815,7 @@ async fn a_verified_callback_always_drops_the_pending_cookie() { assert!(cancel.clears(cookies::PENDING_COOKIE)); // Verified, but Discord did not complete. - let broken = MockDiscord::start("identify", Some(StatusCode::UNAUTHORIZED)).await; + let broken = MockDiscord::start(GRANTED_SCOPE, Some(StatusCode::UNAUTHORIZED)).await; let broken_app = app_against(&broken); let failed = start_login(&broken_app).await; let upstream = fetch( @@ -1057,32 +969,58 @@ async fn a_callback_with_no_code_and_no_error_is_a_client_error() { } /// The Developer Portal registration and this code can disagree about scope, -/// and the token response is the only place the real grant is visible. A -/// broader grant than `identify` is refused rather than quietly accepted. +/// and the token response is the only place the real grant is visible. A grant +/// wider OR narrower than the requested pair is refused rather than quietly +/// accepted — while the pair in the other order, which is the same set per +/// RFC 6749 §3.3, is not. #[tokio::test] -async fn a_grant_wider_than_identify_is_refused() { - let mock = MockDiscord::start("identify guilds", None).await; - let open = app_against(&mock); +async fn a_grant_that_is_not_exactly_the_two_scopes_is_refused() { + for drifted in [ + // Wider: the registration grew `guilds` or `email` — ADR 0010's + // named refusals. + "identify guilds.members.read guilds", + "identify guilds.members.read email", + // Narrower: the member scope missing would turn every membership + // check into a 401-shaped "unknown"; refuse at the exchange instead. + "identify", + "guilds.members.read", + ] { + let mock = MockDiscord::start(drifted, None).await; + let open = app_against(&mock); + let started = start_login(&open).await; + let (state, pending) = (&started.state, &started.pending); + let reply = fetch( + &open, + &format!("{CALLBACK_PATH}?code=c&state={state}"), + &[(cookies::PENDING_COOKIE, pending)], + ) + .await; + + assert_eq!(reply.status, StatusCode::BAD_GATEWAY, "scope={drifted}"); + assert_eq!(reply.json()["code"], "discord_unavailable", "{drifted}"); + assert!(reply.cookie(cookies::SESSION_COOKIE).is_none(), "{drifted}"); + } + + // The same set in the other order is the same grant. + let reordered = MockDiscord::start("guilds.members.read identify", None).await; + let open = app_against(&reordered); let started = start_login(&open).await; - let (state, pending) = (&started.state, &started.pending); let reply = fetch( &open, - &format!("{CALLBACK_PATH}?code=c&state={state}"), - &[(cookies::PENDING_COOKIE, pending)], + &format!("{CALLBACK_PATH}?code=c&state={}", started.state), + &[(cookies::PENDING_COOKIE, &started.pending)], ) .await; - - assert_eq!(reply.status, StatusCode::BAD_GATEWAY); - assert_eq!(reply.json()["code"], "discord_unavailable"); - assert!(reply.cookie(cookies::SESSION_COOKIE).is_none()); + assert_eq!(reply.status, StatusCode::SEE_OTHER); + assert!(reply.cookie(cookies::SESSION_COOKIE).is_some()); } /// Discord having an incident must not read as a bug here, and must not leak /// Discord's response body to the visitor. #[tokio::test] async fn a_failed_token_exchange_is_a_502_with_no_upstream_detail() { - let mock = MockDiscord::start("identify", Some(StatusCode::UNAUTHORIZED)).await; + let mock = MockDiscord::start(GRANTED_SCOPE, Some(StatusCode::UNAUTHORIZED)).await; let open = app_against(&mock); let started = start_login(&open).await; @@ -1155,7 +1093,7 @@ async fn me_ignores_a_forged_or_expired_session() { #[tokio::test] async fn logout_clears_the_session_at_the_path_that_set_it() { - let mock = MockDiscord::start("identify", None).await; + let mock = MockDiscord::start(GRANTED_SCOPE, None).await; let open = app_against(&mock); let started = start_login(&open).await; @@ -1223,6 +1161,7 @@ async fn an_open_portal_with_no_credentials_answers_503_on_login() { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, }; let router = app(&config, AppState::without_ch()); @@ -1255,6 +1194,7 @@ async fn sign_in_needs_no_api_key_even_when_the_key_gate_is_armed() { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, }; let router = app(&config, AppState::without_ch()); diff --git a/packages/prices-api/tests/portal_issue.rs b/packages/prices-api/tests/portal_issue.rs new file mode 100644 index 00000000..37ded05c --- /dev/null +++ b/packages/prices-api/tests/portal_issue.rs @@ -0,0 +1,871 @@ +//! The eligibility-checked issue round-trip, over HTTP, end to end +//! (task 0189). +//! +//! Every test drives the real router through the real flow: `GET +//! /auth/login?action=issue` mints the signed state pair, the callback +//! exchanges the code against a **mock Discord**, asks it for the guild +//! membership, derives the account age from the snowflake, and — only when +//! all of that passes — runs 0187's reconciler against a **mock control +//! plane**. Both mocks record what they were asked, so the assertions are +//! about what the flow *did*: which token the membership call carried, which +//! guild it named, and above all how many keys were created (usually: zero). +//! +//! The spec's three-outcome rule is the spine of this file. Only Discord's own +//! 10007/10004 on a `404` is "not a member"; a throttle, an outage, an +//! unrecognised shape or an absent `pending` field refuses **without +//! accusation** (`?issue=unknown`); and a control-plane fault after a passed +//! check is `?issue=failed` — the visitor is fine, our key service was not. +//! +//! The create/attach/delete tests that lived in `tests/portal_keys.rs` under +//! 0187 live here now, driven through the callback, because the callback is +//! the only place a key is created since the route went read-only. + +#[path = "portal_keys/harness.rs"] +mod harness; +#[path = "common/mock_discord.rs"] +mod mock_discord; + +use axum::Router; +use axum::http::StatusCode; + +use harness::*; +use mock_discord::{GRANTED_SCOPE, MemberReply, MockDiscord}; +use prices_api::portal::auth::discord::Endpoints; +use prices_api::portal::auth::{cookies, session::Session, state_token}; +use prices_api::portal::keys::gateway::Gateway; +use prices_api::portal::usage::USAGE_PATH; + +/// Milliseconds since the Discord epoch, shifted into snowflake position — +/// how the too-young tests mint an account "created N seconds ago". +fn snowflake_created_secs_ago(secs: u64) -> String { + const DISCORD_EPOCH_MS: u64 = 1_420_070_400_000; + let now_ms = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis() as u64; + ((now_ms - DISCORD_EPOCH_MS - secs * 1_000) << 22).to_string() +} + +fn issue_app(discord: &MockDiscord, gateway: &MockGateway) -> Router { + issue_app_with(discord, gateway, GUILD_ID, "5") +} + +fn issue_app_with( + discord: &MockDiscord, + gateway: &MockGateway, + guild_id: &str, + min_age_minutes: &str, +) -> Router { + build_app_with( + true, + Some(Gateway::against(&gateway.base, PLAN_ID.to_string())), + Endpoints { + api_base: discord.base.clone(), + ..Endpoints::default() + }, + Some(eligibility(guild_id, min_age_minutes)), + ) +} + +fn key_name() -> String { + format!("discord-{USER_ID}-key") +} + +// --------------------------------------------------------------------------- +// The gate — ships closed +// --------------------------------------------------------------------------- + +/// The whole issue flow is an empty `404` while the portal is closed — the +/// login that would start it and the callback that would finish it — with +/// **zero** calls to Discord and zero to the control plane. This is the slice +/// 0194 is waiting on before `PORTAL_ENABLED` may flip, so "closed" is +/// asserted with everything else fully wired. +#[tokio::test] +async fn everything_including_issue_is_an_empty_404_while_the_portal_is_closed() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let closed = build_app_with( + false, + Some(Gateway::against(&gateway.base, PLAN_ID.to_string())), + Endpoints { + api_base: discord.base.clone(), + ..Endpoints::default() + }, + Some(eligibility(GUILD_ID, "5")), + ); + + for path in [ + "/api-tokens/api/auth/login?action=issue", + "/api-tokens/api/auth/callback?code=c&state=s", + ] { + let reply = call_path(closed.clone(), "GET", path, None).await; + assert_eq!(reply.status, StatusCode::NOT_FOUND, "{path}"); + assert!(reply.body.is_empty(), "{path} carried a body"); + } + assert_eq!(discord.exchanges(), 0); + assert_eq!(discord.member_calls(), 0); + assert_eq!(gateway.with(|s| s.list_calls), 0); + assert_eq!(gateway.with(|s| s.create_calls), 0); +} + +// --------------------------------------------------------------------------- +// The happy path +// --------------------------------------------------------------------------- + +/// A member in good standing, on an old account: the round-trip creates one +/// enabled, tagged key on the free plan, lands on `?issue=ok` with a fresh +/// session — and the key value appears **nowhere** in the redirect. The page +/// then reveals it over the read-only route. +#[tokio::test] +async fn a_member_in_good_standing_gets_a_key_and_lands_on_issue_ok() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let app = issue_app(&discord, &gateway); + + // Note the driver presents NO session cookie: the signed state pair plus + // the fresh code are the authentication (ADR 0010 §8). + let reply = issue_round_trip(&app).await; + + assert_eq!(reply.status, StatusCode::SEE_OTHER); + assert_eq!(reply.location(), "/api-tokens/?issue=ok"); + assert!( + reply.cookie(cookies::SESSION_COOKIE).is_some(), + "the round-trip proves identity, so it signs the visitor in" + ); + + let stored = gateway.with(|s| s.keys.clone()); + assert_eq!(stored.len(), 1, "exactly one key"); + assert_eq!(stored[0].name, key_name()); + assert!( + stored[0].enabled, + "a key that is not enabled cannot be used" + ); + assert_eq!( + stored[0].tags.get("ManagedBy").map(String::as_str), + Some("prices-portal") + ); + assert_eq!( + gateway.with(|s| s.plan_keys.clone()), + vec![(PLAN_ID.to_string(), stored[0].id.clone())], + "the key is on the plan before anyone is told it exists" + ); + + // The credential never rides in the Location — or anywhere on the redirect. + let headers = format!("{:?}", reply.headers); + assert!(!headers.contains(&stored[0].value), "{headers}"); + assert!(!headers.contains("CANARY"), "{headers}"); + + // The page's next step: reveal it, session-only, over the read-only route. + let revealed = reveal(&gateway, USER_ID).await; + assert_eq!(revealed.status, StatusCode::OK); + assert_eq!(revealed.json()["key_id"], stored[0].id); + assert_eq!(revealed.json()["value"], stored[0].value); +} + +/// A second round-trip converges on the same key — issuance is idempotent +/// through the callback exactly as it was through 0187's button. +#[tokio::test] +async fn a_second_round_trip_returns_the_same_key() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let app = issue_app(&discord, &gateway); + + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + + assert_eq!( + gateway.with(|s| s.create_calls), + 1, + "one create, however many trips" + ); + assert_eq!(gateway.with(|s| s.keys.len()), 1); +} + +/// Two people get two keys, and neither is handed the other's. +#[tokio::test] +async fn two_users_get_two_different_keys() { + let gateway = MockGateway::start().await; + let mine = MockDiscord::start(GRANTED_SCOPE, None).await; + let theirs = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::Member { + pending: Some(false), + }, + "111111111111111111", + ) + .await; + + assert_eq!( + issue_round_trip(&issue_app(&mine, &gateway)) + .await + .location(), + "/api-tokens/?issue=ok" + ); + assert_eq!( + issue_round_trip(&issue_app(&theirs, &gateway)) + .await + .location(), + "/api-tokens/?issue=ok" + ); + + let names: Vec = gateway.with(|s| s.keys.iter().map(|k| k.name.clone()).collect()); + assert_eq!(names.len(), 2); + assert!(names.contains(&key_name())); + assert!(names.contains(&"discord-111111111111111111-key".to_string())); +} + +/// However two simultaneous round-trips interleave, the user ends with exactly +/// one key — the reconciler is what gets them there. +#[tokio::test] +async fn two_simultaneous_round_trips_leave_exactly_one_key() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let app = issue_app(&discord, &gateway); + + let (a, b) = tokio::join!(issue_round_trip(&app), issue_round_trip(&app)); + assert_eq!(a.location(), "/api-tokens/?issue=ok"); + assert_eq!(b.location(), "/api-tokens/?issue=ok"); + + assert_eq!( + gateway.with(|s| s.named(&key_name()).len()), + 1, + "the reconciler must converge on one key" + ); + + // And the survivor is what a subsequent reveal hands out, so whichever of + // the two lost, the user is not left holding a deleted value. + let survivor = gateway.with(|s| s.named(&key_name())[0].id.clone()); + assert_eq!(reveal(&gateway, USER_ID).await.json()["key_id"], survivor); +} + +// --------------------------------------------------------------------------- +// The membership verdicts — three outcomes, not two +// --------------------------------------------------------------------------- + +/// A confirmed non-member (Discord's own 10007 on a 404) is refused, no key is +/// created — and identity was still proven, so the refusal carries a session: +/// a non-member legitimately holds reveal and usage for any key they already +/// have. +#[tokio::test] +async fn a_non_member_is_refused_and_no_key_is_created() { + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::NotFound { code: 10_007 }, + mock_discord::USER_ID, + ) + .await; + let gateway = MockGateway::start().await; + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.status, StatusCode::SEE_OTHER); + assert_eq!(reply.location(), "/api-tokens/?issue=not_member"); + assert!(reply.cookie(cookies::SESSION_COOKIE).is_some()); + + assert_eq!( + gateway.with(|s| s.create_calls), + 0, + "no key for a non-member" + ); + assert_eq!( + gateway.with(|s| s.list_calls), + 0, + "the control plane is not even asked" + ); +} + +/// 10004 ("Unknown Guild") also reads as not-a-member per the spec — the +/// warn naming the guild id (a mis-seeded parameter is the likelier cause) is +/// asserted at the unit level; here the wire outcome is what is pinned. +#[tokio::test] +async fn an_unknown_guild_code_is_also_refused_as_not_a_member() { + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::NotFound { code: 10_004 }, + mock_discord::USER_ID, + ) + .await; + let gateway = MockGateway::start().await; + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=not_member"); + assert_eq!(gateway.with(|s| s.create_calls), 0); +} + +/// A 404 whose body does not carry a recognised code proves nothing — an +/// unmeasured shape (0180 item 1 is still open) must not become an accusation. +#[tokio::test] +async fn a_404_with_an_unrecognised_code_is_unknown_not_an_accusation() { + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::NotFound { code: 10_008 }, + mock_discord::USER_ID, + ) + .await; + let gateway = MockGateway::start().await; + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=unknown"); + assert_eq!(gateway.with(|s| s.create_calls), 0); +} + +/// The acceptance criterion verbatim: a `429` or `5xx` from Discord refuses +/// **without** claiming non-membership. `401`/`403` land the same way — an +/// auth fault on our side is not the visitor's standing either. +#[tokio::test] +async fn a_429_or_5xx_from_discord_refuses_without_claiming_non_membership() { + for status in [ + StatusCode::TOO_MANY_REQUESTS, + StatusCode::INTERNAL_SERVER_ERROR, + StatusCode::SERVICE_UNAVAILABLE, + StatusCode::UNAUTHORIZED, + StatusCode::FORBIDDEN, + ] { + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::Status(status), + mock_discord::USER_ID, + ) + .await; + let gateway = MockGateway::start().await; + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!( + reply.location(), + "/api-tokens/?issue=unknown", + "a {status} must be 'could not verify', never 'not a member'" + ); + assert_eq!(gateway.with(|s| s.create_calls), 0, "{status}"); + } +} + +/// `pending: true` — joined, but not through Membership Screening — is not a +/// member yet. Completing the screening is the same "join the server" action +/// the refusal names, so it lands under the same state. +#[tokio::test] +async fn a_pending_member_is_refused() { + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::Member { + pending: Some(true), + }, + mock_discord::USER_ID, + ) + .await; + let gateway = MockGateway::start().await; + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=not_member"); + assert_eq!(gateway.with(|s| s.create_calls), 0); +} + +/// The acceptance criterion: `pending === undefined` is handled explicitly +/// and does not silently pass. It is `unknown` — refused without accusation — +/// because 0180 item 2 (whether the REST route carries the field at all) is +/// unmeasured, and reading absence as "cleared" would void the gate. +#[tokio::test] +async fn an_absent_pending_field_does_not_silently_pass() { + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::Member { pending: None }, + mock_discord::USER_ID, + ) + .await; + let gateway = MockGateway::start().await; + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=unknown"); + assert_eq!(gateway.with(|s| s.create_calls), 0); +} + +/// A 200 whose body is not a member object proves nothing either. +#[tokio::test] +async fn a_malformed_member_body_is_unknown() { + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::Malformed, + mock_discord::USER_ID, + ) + .await; + let gateway = MockGateway::start().await; + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=unknown"); + assert_eq!(gateway.with(|s| s.create_calls), 0); +} + +/// The membership call carries the FRESH token from this round-trip's own +/// exchange — that is what "eligibility travels by re-authentication" means on +/// the wire — and asks about exactly the configured guild. +#[tokio::test] +async fn the_member_call_carries_the_fresh_token_and_the_configured_guild() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + + issue_round_trip(&issue_app(&discord, &gateway)).await; + + assert_eq!(discord.member_calls(), 1); + assert_eq!( + discord.member_bearer().as_deref(), + Some("Bearer an-access-token"), + "the member call must authenticate with the just-exchanged token" + ); + assert_eq!(discord.member_guild().as_deref(), Some(GUILD_ID)); +} + +/// A grant missing the member scope is refused at the exchange, before any +/// membership call — a registration that drifted narrower must not turn every +/// eligibility check into a refusal with a misleading shape. +#[tokio::test] +async fn a_grant_missing_the_member_scope_is_refused_before_any_member_call() { + let discord = MockDiscord::start("identify", None).await; + let gateway = MockGateway::start().await; + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.status, StatusCode::BAD_GATEWAY); + assert_eq!(reply.json()["code"], "discord_unavailable"); + assert_eq!(discord.member_calls(), 0); + assert_eq!(gateway.with(|s| s.create_calls), 0); +} + +// --------------------------------------------------------------------------- +// Account age +// --------------------------------------------------------------------------- + +/// The acceptance criterion: an account below the threshold is refused **with +/// the time remaining** — carried as `wait_secs`, digits the page renders, so +/// the copy follows the operator's threshold instead of hard-coding one. +#[tokio::test] +async fn an_account_below_the_threshold_is_refused_with_the_time_remaining() { + // Created two minutes ago, against a five-minute threshold. + let young = snowflake_created_secs_ago(120); + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::Member { + pending: Some(false), + }, + &young, + ) + .await; + let gateway = MockGateway::start().await; + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + let location = reply.location(); + let (base, wait) = location + .split_once("&wait_secs=") + .unwrap_or_else(|| panic!("no wait_secs in {location}")); + assert_eq!(base, "/api-tokens/?issue=too_young"); + let wait: u64 = wait.parse().expect("wait_secs must be digits"); + // ~180s remain; generous bounds absorb test-runner latency. + assert!((150..=181).contains(&wait), "wait_secs={wait}"); + assert_eq!(gateway.with(|s| s.create_calls), 0); + // Still signed in — a wait is not a rejection. + assert!(reply.cookie(cookies::SESSION_COOKIE).is_some()); +} + +/// The threshold is configuration, not code: the same two-minute-old account +/// is refused under a five-minute rule and issued under a zero-minute rule. +/// (Live, the value is re-read from SSM per action — changing it needs no +/// redeploy; here the two sources stand in for the two values.) +#[tokio::test] +async fn the_threshold_value_decides_the_verdict_at_action_time() { + let young = snowflake_created_secs_ago(120); + + let strict_discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::Member { + pending: Some(false), + }, + &young, + ) + .await; + let gateway = MockGateway::start().await; + let strict = issue_round_trip(&issue_app_with(&strict_discord, &gateway, GUILD_ID, "5")).await; + assert!( + strict + .location() + .starts_with("/api-tokens/?issue=too_young"), + "{}", + strict.location() + ); + + let lax_discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::Member { + pending: Some(false), + }, + &young, + ) + .await; + let lax = issue_round_trip(&issue_app_with(&lax_discord, &gateway, GUILD_ID, "0")).await; + assert_eq!(lax.location(), "/api-tokens/?issue=ok"); +} + +// --------------------------------------------------------------------------- +// Sessions and identities +// --------------------------------------------------------------------------- + +/// An existing session for a DIFFERENT account does not survive the +/// round-trip: the fresh re-auth identity wins, the key is named for it, and +/// the cookie now says so — key and session can never disagree. +#[tokio::test] +async fn a_session_for_someone_else_is_replaced_by_the_re_auth_identity() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let app = issue_app(&discord, &gateway); + + // Start the round-trip, then complete the callback while presenting a + // session for somebody else alongside the pending cookie. + let login = call_path( + app.clone(), + "GET", + "/api-tokens/api/auth/login?action=issue", + None, + ) + .await; + let pending = login.cookie(cookies::PENDING_COOKIE).unwrap(); + let query = login.location().split_once('?').unwrap().1.to_string(); + let state = form_urlencoded::parse(query.as_bytes()) + .find(|(k, _)| k == "state") + .unwrap() + .1 + .to_string(); + + let other = session_cookie("999999999999999999"); + let reply = call_path( + app, + "GET", + &format!("/api-tokens/api/auth/callback?code=c&state={state}"), + Some(&format!("{}={pending}; {other}", cookies::PENDING_COOKIE)), + ) + .await; + + assert_eq!(reply.location(), "/api-tokens/?issue=ok"); + let cookie = reply.cookie(cookies::SESSION_COOKIE).unwrap(); + let session = Session::decode(SIGNING_KEY.as_bytes(), &cookie, state_token::now_secs()) + .expect("the fresh session must verify"); + assert_eq!(session.sub, USER_ID, "the re-auth identity wins"); + + let names: Vec = gateway.with(|s| s.keys.iter().map(|k| k.name.clone()).collect()); + assert_eq!( + names, + vec![key_name()], + "the key belongs to the fresh identity too" + ); +} + +/// The epic's non-goal, as a test: a user who has left the guild keeps their +/// key — reveal and usage still work with the session alone, and **Discord is +/// never consulted** on either route. +#[tokio::test] +async fn reveal_and_usage_still_work_for_a_user_who_has_left_the_guild() { + // Discord now says "not a member" — they left after issuance. + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::NotFound { code: 10_007 }, + mock_discord::USER_ID, + ) + .await; + let gateway = MockGateway::start().await; + gateway.with(|s| { + let id = s.seed(&key_name(), 1_000); + s.plan_keys.push((PLAN_ID.to_string(), id)); + }); + let app = issue_app(&discord, &gateway); + + let revealed = call_path( + app.clone(), + "GET", + prices_api::portal::keys::KEY_PATH, + Some(&session_cookie(USER_ID)), + ) + .await; + assert_eq!(revealed.status, StatusCode::OK); + assert_eq!(revealed.json()["name"], key_name()); + + let usage = call_path(app, "GET", USAGE_PATH, Some(&session_cookie(USER_ID))).await; + assert_eq!( + usage.status, + StatusCode::OK, + "{}", + String::from_utf8_lossy(&usage.body) + ); + + assert_eq!( + discord.member_calls(), + 0, + "membership is proved at issuance and never re-checked by reveal or usage" + ); + assert_eq!(discord.exchanges(), 0); +} + +// --------------------------------------------------------------------------- +// Configuration faults +// --------------------------------------------------------------------------- + +/// An eligibility parameter that cannot be resolved — empty guild id, or a +/// threshold that is not a number — refuses as `unknown`: the fault is ours, +/// the log names it, and the visitor is told to try again rather than +/// anything about their membership. The member call never happens. +#[tokio::test] +async fn an_unreadable_eligibility_parameter_is_unknown() { + for (guild, age) in [("", "5"), (GUILD_ID, "five"), ("not-a-snowflake", "5")] { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let app = issue_app_with(&discord, &gateway, guild, age); + + let reply = issue_round_trip(&app).await; + assert_eq!( + reply.location(), + "/api-tokens/?issue=unknown", + "guild={guild:?} age={age:?}" + ); + assert_eq!(gateway.with(|s| s.create_calls), 0); + // An empty or unparseable parameter never becomes a member URL; the + // non-snowflake one is refused by the URL builder itself. + if guild.is_empty() || age == "five" { + assert_eq!(discord.member_calls(), 0, "guild={guild:?} age={age:?}"); + } + } +} + +// --------------------------------------------------------------------------- +// The control plane, after eligibility passed +// --------------------------------------------------------------------------- + +/// A control-plane failure after a PASSED check is `?issue=failed`, not +/// `unknown` — the visitor's membership was verified, and rendering an AWS +/// incident as a doubt about it would be both false and unfixable by them. +#[tokio::test] +async fn a_control_plane_failure_after_eligibility_lands_on_issue_failed_not_unknown() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + gateway.with(|s| s.fail_list = true); + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=failed"); + assert_eq!(discord.member_calls(), 1, "eligibility ran and passed"); + // Still signed in: identity and membership both proved. + assert!(reply.cookie(cookies::SESSION_COOKIE).is_some()); +} + +/// A usage plan that does not exist is `failed` too — and the flow must not +/// keep minting keys while it is broken. +#[tokio::test] +async fn a_missing_usage_plan_is_issue_failed_without_minting_more_keys() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + gateway.with(|s| s.attach_always_404 = true); + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=failed"); + assert_eq!(gateway.with(|s| s.create_calls), 1); +} + +/// A stale listing whose winner never reads bounds the attempts and lands on +/// `failed` — an answer, not a timeout. +#[tokio::test] +async fn a_stale_listing_bounds_the_attempts_and_lands_on_failed() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + gateway.with(|s| { + s.seed(&key_name(), 1_000); + s.read_always_404 = true; + }); + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=failed"); + assert_eq!( + gateway.with(|s| s.list_calls), + 2, + "one listing per attempt, and exactly two attempts" + ); + assert_eq!(gateway.with(|s| s.create_calls), 0); +} + +// --------------------------------------------------------------------------- +// The reconciler, driven through the callback (moved from portal_keys.rs) +// --------------------------------------------------------------------------- + +/// A key that exists but is on no usage plan is adopted and put on it — the +/// heal for the orphan the read-only reveal deliberately leaves alone. +#[tokio::test] +async fn an_adopted_key_is_put_on_the_free_plan() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let orphan = gateway.with(|s| s.seed(&key_name(), 1_000)); + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=ok"); + assert_eq!( + gateway.with(|s| s.plan_keys.clone()), + vec![(PLAN_ID.to_string(), orphan.clone())], + "an adopted key is only useful once it is on the plan" + ); + assert_eq!( + gateway.with(|s| s.create_calls), + 0, + "adopting must not mint a second key" + ); + assert_eq!(reveal(&gateway, USER_ID).await.json()["key_id"], orphan); +} + +/// Duplicates converge on the **earliest** key and the rest are deleted — +/// through the round-trip, which is now the only path that may delete. +#[tokio::test] +async fn duplicates_converge_on_the_earliest_and_the_losers_are_deleted() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let (early, late, later) = gateway.with(|s| { + ( + s.seed(&key_name(), 1_000), + s.seed(&key_name(), 2_000), + s.seed(&key_name(), 3_000), + ) + }); + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=ok"); + + let mut deleted = gateway.with(|s| s.deleted.clone()); + deleted.sort(); + let mut expected = vec![late, later]; + expected.sort(); + assert_eq!(deleted, expected); + assert_eq!(gateway.with(|s| s.named(&key_name()).len()), 1); + assert_eq!( + reveal(&gateway, USER_ID).await.json()["key_id"], + early, + "the earliest key must survive" + ); +} + +/// The reconciler must see **every** page before it ranks — five duplicates +/// at a page size of two, with the earliest deliberately on the last page. +#[tokio::test] +async fn the_reconciler_pages_get_api_keys_to_exhaustion_before_ranking() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let earliest = gateway.with(|s| { + s.page_size = 2; + s.seed(&key_name(), 5_000); + s.seed(&key_name(), 4_000); + s.seed(&key_name(), 3_000); + s.seed(&key_name(), 2_000); + s.seed(&key_name(), 1_000) // earliest, and last in the listing + }); + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=ok"); + assert_eq!( + gateway.with(|s| s.list_calls), + 3, + "five keys at a page size of two is three pages" + ); + assert_eq!(gateway.with(|s| s.deleted.len()), 4); + assert_eq!( + gateway.with(|s| s.named(&key_name())[0].id.clone()), + earliest, + "the winner came from the last page" + ); +} + +/// A key that vanishes before the attach re-enters the flow rather than +/// becoming a dead end. +#[tokio::test] +async fn a_key_that_vanishes_before_the_attach_is_not_a_dead_end() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let doomed = gateway.with(|s| { + let id = s.seed(&key_name(), 1_000); + s.vanish_on_next_attach = true; + id + }); + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=ok"); + + let survivors: Vec = + gateway.with(|s| s.named(&key_name()).iter().map(|k| k.id.clone()).collect()); + assert_eq!(survivors.len(), 1); + assert_ne!( + survivors[0], doomed, + "the vanished key is not what survived" + ); + assert_eq!( + gateway.with(|s| s.plan_keys.len()), + 1, + "the replacement is on the plan" + ); +} + +/// An eventually-consistent listing that has not caught up must not orphan +/// the key just created. +#[tokio::test] +async fn a_key_the_listing_has_not_caught_up_with_is_not_orphaned() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let older = gateway.with(|s| { + let older = s.seed(&key_name(), 1_000); + s.next_list_is_empty = true; + s.next_list_omits_newest = true; + older + }); + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=ok"); + assert_eq!( + gateway.with(|s| s.keys.len()), + 1, + "the key created during this request was reconciled away, not orphaned" + ); + assert_eq!( + gateway.with(|s| s.plan_keys.clone()), + vec![(PLAN_ID.to_string(), older)], + "and the survivor is on the plan" + ); +} + +/// A duplicate that will not delete does not withhold the outcome. +#[tokio::test] +async fn a_duplicate_that_will_not_delete_does_not_withhold_the_key() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let earliest = gateway.with(|s| { + let earliest = s.seed(&key_name(), 1_000); + s.seed(&key_name(), 2_000); + s.fail_deletes = true; + earliest + }); + + let reply = issue_round_trip(&issue_app(&discord, &gateway)).await; + assert_eq!(reply.location(), "/api-tokens/?issue=ok"); + assert_eq!( + gateway.with(|s| s.keys.len()), + 2, + "the duplicate survives, for the next round-trip to try again" + ); + assert_eq!(gateway.with(|s| s.create_calls), 0); + assert_eq!( + reveal(&gateway, USER_ID).await.json()["key_id"], + earliest, + "the winner is still the earliest, and it is what the reveal hands out" + ); +} diff --git a/packages/prices-api/tests/portal_keys.rs b/packages/prices-api/tests/portal_keys.rs index d5516a94..4262145c 100644 --- a/packages/prices-api/tests/portal_keys.rs +++ b/packages/prices-api/tests/portal_keys.rs @@ -1,15 +1,22 @@ -//! Issuing and revealing a key, over HTTP, against a mock control plane -//! (task 0187). +//! Revealing a key, over HTTP, against a mock control plane (task 0187, +//! re-shaped read-only by task 0189). //! //! The unit tests next to `portal/keys/naming.rs` cover the rules — the name, //! the exact filter, the winner — on lists somebody typed by hand. This file -//! covers what none of them can: the two routes wired into the real +//! covers what none of them can: the route wired into the real //! [`prices_api::app`] router, driving the **actual AWS SDK** against a mock //! API Gateway control plane bound to loopback. The mock, and the argument for //! why it is a service rather than a trait, live in `portal_keys/harness.rs`. //! -//! The one assertion that is **not** here is "no key value reaches the logs": -//! it needs a process to itself and lives in `portal_keys_logs.rs`. +//! **The create path is not here.** Task 0189 moved issuance behind the +//! eligibility-checked OAuth round-trip, so everything that creates, attaches +//! or deletes is exercised in `tests/portal_issue.rs`, driven through the +//! callback. What this file owns is the inverse property: a session cookie +//! alone can cause **zero control-plane writes**, on either verb, in every +//! state the store can be in. +//! +//! The one assertion that is **not** in either file is "no key value reaches +//! the logs": it needs a process to itself and lives in `portal_keys_logs.rs`. #[path = "portal_keys/harness.rs"] mod harness; @@ -22,14 +29,11 @@ use tower::ServiceExt; use harness::*; // --------------------------------------------------------------------------- -// The gate (AC 1) +// The gate // --------------------------------------------------------------------------- -/// **The slice the flag exists for.** Until [0189]'s eligibility gate lands, -/// `PORTAL_ENABLED=false` is the only thing between a stranger who can sign in -/// and a real production API key — so "closed" is asserted on both verbs, with a -/// valid session presented, and byte-for-byte against a path that was never -/// routed. +/// "Closed" is asserted on both verbs, with a valid session presented, and +/// byte-for-byte against a path that was never routed. #[tokio::test] async fn both_key_routes_are_an_empty_404_while_the_portal_is_closed() { for method in ["GET", "POST"] { @@ -68,9 +72,9 @@ async fn both_key_routes_are_an_empty_404_while_the_portal_is_closed() { ); } -/// With the portal open and nothing provisioned the routes still exist and say -/// so — a `503`, not a `404`, so a half-configured deployment is distinguishable -/// from a closed one. +/// With the portal open and nothing provisioned the route still exists and +/// says so — a `503`, not a `404`, so a half-configured deployment is +/// distinguishable from a closed one. #[tokio::test] async fn an_unprovisioned_deployment_answers_503_rather_than_vanishing() { let reply = call( @@ -87,11 +91,11 @@ async fn an_unprovisioned_deployment_answers_503_rather_than_vanishing() { // Authentication // --------------------------------------------------------------------------- -/// No session, no key — and, just as importantly, **no call to AWS**. A route -/// that reached the control plane before checking the cookie would let an -/// anonymous caller drive `GetApiKeys` at the portal's 10 req/s throttle. +/// No session, no answer — and, just as importantly, **no call to AWS**. A +/// route that reached the control plane before checking the cookie would let +/// an anonymous caller drive `GetApiKeys` at the portal's 10 req/s throttle. #[tokio::test] -async fn issuing_without_a_session_is_refused_before_aws_is_touched() { +async fn asking_without_a_session_is_refused_before_aws_is_touched() { let mock = MockGateway::start().await; for method in ["GET", "POST"] { let reply = call(app_against(&mock), method, None).await; @@ -102,11 +106,11 @@ async fn issuing_without_a_session_is_refused_before_aws_is_touched() { assert_eq!(mock.with(|s| s.create_calls), 0); } -/// The forgery that matters: edit `sub` and you are someone else — and from this -/// slice on, someone else's API key. The signature is what stops it, and nothing -/// reaches AWS on the way to finding that out. +/// The forgery that matters: edit `sub` and you are someone else — and from +/// this slice on, someone else's API key. The signature is what stops it, and +/// nothing reaches AWS on the way to finding that out. #[tokio::test] -async fn a_forged_session_cannot_issue_a_key() { +async fn a_forged_session_cannot_reveal_a_key() { use base64::Engine as _; let mock = MockGateway::start().await; @@ -129,201 +133,122 @@ async fn a_forged_session_cannot_issue_a_key() { } // --------------------------------------------------------------------------- -// Issue and reveal (ACs 2, 4, 5) +// The reveal — and the read-only invariant (task 0189) // --------------------------------------------------------------------------- -/// The first press: one key, named for the user, enabled, tagged, on the free -/// plan, and its value on screen. +/// The reveal shows an existing key: id, name, value. #[tokio::test] -async fn the_first_press_creates_one_key_on_the_free_plan_and_shows_it() { +async fn an_existing_key_is_revealed_with_its_value() { let mock = MockGateway::start().await; - let reply = issue(&mock, USER_ID).await; + let name = format!("discord-{USER_ID}-key"); + let id = mock.with(|s| s.seed(&name, 1_000)); + let reply = reveal(&mock, USER_ID).await; assert_eq!(reply.status, StatusCode::OK); let body = reply.json(); - assert_eq!(body["name"], format!("discord-{USER_ID}-key")); - assert_eq!(body["created"], true); - - let stored = mock.with(|s| s.keys.clone()); - assert_eq!(stored.len(), 1, "exactly one key"); - assert_eq!(body["key_id"], stored[0].id); - assert_eq!(body["value"], stored[0].value); - assert!( - stored[0].enabled, - "a key that is not enabled cannot be used" - ); - assert_eq!( - stored[0].tags.get("ManagedBy").map(String::as_str), - Some("prices-portal"), - "the tag is the only attribution `CreateApiKey` can be given" - ); - - // Attached to the plan whose id came from SSM — which is what makes the key - // work against `/v1/` rather than authenticate and then be refused. - assert_eq!( - mock.with(|s| s.plan_keys.clone()), - vec![(PLAN_ID.to_string(), stored[0].id.clone())] - ); -} - -/// A second press returns the same key. Not a new one, and not a second create. -#[tokio::test] -async fn a_second_press_returns_the_same_key() { - let mock = MockGateway::start().await; - let first = issue(&mock, USER_ID).await.json(); - let second = issue(&mock, USER_ID).await.json(); - - assert_eq!(first["key_id"], second["key_id"]); - assert_eq!(first["value"], second["value"]); - assert_eq!(second["created"], false); - assert_eq!(mock.with(|s| s.create_calls), 1); - assert_eq!(mock.with(|s| s.keys.len()), 1); + assert_eq!(body["key_id"], id); + assert_eq!(body["name"], name); + assert_eq!(body["value"], mock.with(|s| s.keys[0].value.clone())); } -/// Signing out and back in is a new cookie for the same Discord id, and the key -/// is a property of the id — so it survives. This is the criterion that would -/// fail if anything about the key were kept in the session. +/// **The acceptance criterion "issue is unreachable with a session cookie +/// alone", verified by calling it directly with nothing else.** Both verbs, +/// empty store — the state 0187's handler would have created in — and the +/// answer is `no_key`, with zero writes of any kind on the control plane. #[tokio::test] -async fn signing_out_and_back_in_still_shows_the_same_key() { +async fn a_session_cookie_alone_cannot_create_a_key_on_either_verb() { let mock = MockGateway::start().await; - let before = issue(&mock, USER_ID).await.json(); - // A fresh cookie, minted as a second sign-in would mint it. - let after = reveal(&mock, USER_ID).await.json(); - - assert_eq!(before["key_id"], after["key_id"]); - assert_eq!(before["value"], after["value"]); - assert_eq!(mock.with(|s| s.create_calls), 1); + for method in ["GET", "POST"] { + let reply = call(app_against(&mock), method, Some(&session_cookie(USER_ID))).await; + assert_eq!(reply.status, StatusCode::NOT_FOUND, "{method}"); + assert_eq!(reply.json()["code"], "no_key", "{method}"); + assert_eq!(reply.cache_control(), "no-store", "{method}"); + } + assert_eq!(mock.with(|s| s.create_calls), 0, "nothing was created"); + assert_eq!(mock.with(|s| s.attach_calls), 0, "nothing was attached"); + assert!(mock.with(|s| s.deleted.is_empty()), "nothing was deleted"); } -/// Two people get two keys, and neither is handed the other's. +/// Around duplicates too: the reveal answers with the same deterministic +/// winner the issue flow would pick, and sweeps **nothing** — converging +/// duplicates stays the issue flow's job, because a delete reachable from a +/// session-only route would be a control-plane write a forged top-level +/// navigation could trigger. #[tokio::test] -async fn two_users_get_two_different_keys() { - let mock = MockGateway::start().await; - let mine = issue(&mock, USER_ID).await.json(); - let theirs = issue(&mock, "111111111111111111").await.json(); - assert_ne!(mine["key_id"], theirs["key_id"]); - assert_ne!(mine["value"], theirs["value"]); - assert_eq!(mock.with(|s| s.keys.len()), 2); -} - -// --------------------------------------------------------------------------- -// The reconciler (ACs 6, 7, 8) -// --------------------------------------------------------------------------- - -/// Duplicates converge on the **earliest** key and the rest are deleted. This is -/// the state a double-submit leaves behind, seeded directly so the assertion is -/// about the rule rather than about a scheduler. -#[tokio::test] -async fn duplicates_converge_on_the_earliest_and_the_losers_are_deleted() { +async fn a_reveal_never_creates_attaches_or_deletes_even_around_duplicates() { let mock = MockGateway::start().await; let name = format!("discord-{USER_ID}-key"); - let (early, late, later) = mock.with(|s| { - ( - s.seed(&name, 1_000), - s.seed(&name, 2_000), - s.seed(&name, 3_000), - ) + let earliest = mock.with(|s| { + let earliest = s.seed(&name, 1_000); + s.seed(&name, 2_000); + s.seed(&name, 3_000); + earliest }); - let body = issue(&mock, USER_ID).await.json(); - - assert_eq!(body["key_id"], early, "the earliest key must survive"); - assert_eq!(body["created"], false); - let mut deleted = mock.with(|s| s.deleted.clone()); - deleted.sort(); - let mut expected = vec![late, later]; - expected.sort(); - assert_eq!(deleted, expected); - assert_eq!(mock.with(|s| s.named(&name).len()), 1); -} - -/// Two simultaneous first presses. However they interleave, the user ends with -/// exactly one key — and the reconciler is what gets them there. -#[tokio::test] -async fn two_simultaneous_first_presses_leave_exactly_one_key() { - let mock = MockGateway::start().await; - let (a, b) = tokio::join!(issue(&mock, USER_ID), issue(&mock, USER_ID)); - - assert_eq!(a.status, StatusCode::OK); - assert_eq!(b.status, StatusCode::OK); - - let name = format!("discord-{USER_ID}-key"); + let reply = reveal(&mock, USER_ID).await; + assert_eq!(reply.status, StatusCode::OK); assert_eq!( - mock.with(|s| s.named(&name).len()), - 1, - "the reconciler must converge on one key" + reply.json()["key_id"], + earliest, + "the reveal ranks like the issue flow" ); - // And the survivor is what a subsequent reveal hands out, so whichever of - // the two lost, the user is not left holding a deleted value. - let survivor = mock.with(|s| s.named(&name)[0].id.clone()); - let after = reveal(&mock, USER_ID).await.json(); - assert_eq!(after["key_id"], survivor); + assert_eq!( + mock.with(|s| s.keys.len()), + 3, + "the duplicates survive a reveal" + ); + assert!(mock.with(|s| s.deleted.is_empty())); + assert_eq!(mock.with(|s| s.create_calls), 0); + assert_eq!(mock.with(|s| s.attach_calls), 0); } -/// The reconciler must see **every** page before it ranks. Ranking off page one -/// picks a winner from a partial list and then deletes a key it never saw. -/// -/// Five duplicates at a page size of two: three pages, with the earliest key -/// deliberately on the last one, so an implementation that stops early both -/// returns the wrong key and deletes the right one. +/// A key that exists but is on no plan is revealed as it is — NOT silently +/// attached, because attach is a write. It answers `403` on `/v1/` until the +/// holder presses "get my key", whose round-trip adopts and attaches it +/// (asserted in `portal_issue.rs`). #[tokio::test] -async fn the_reconciler_pages_get_api_keys_to_exhaustion_before_ranking() { +async fn an_unattached_key_is_revealed_but_not_repaired_in_place() { let mock = MockGateway::start().await; let name = format!("discord-{USER_ID}-key"); - let earliest = mock.with(|s| { - s.page_size = 2; - s.seed(&name, 5_000); - s.seed(&name, 4_000); - s.seed(&name, 3_000); - s.seed(&name, 2_000); - s.seed(&name, 1_000) // earliest, and last in the listing - }); - - let body = issue(&mock, USER_ID).await.json(); + let orphan = mock.with(|s| s.seed(&name, 1_000)); - assert_eq!( - body["key_id"], earliest, - "the winner came from the last page; a partial list was ranked" - ); - assert_eq!( - mock.with(|s| s.list_calls), - 3, - "five keys at a page size of two is three pages" - ); - assert_eq!(mock.with(|s| s.deleted.len()), 4); + let reply = reveal(&mock, USER_ID).await; + assert_eq!(reply.status, StatusCode::OK); + assert_eq!(reply.json()["key_id"], orphan); + assert_eq!(mock.with(|s| s.attach_calls), 0); + assert!(mock.with(|s| s.plan_keys.is_empty())); } -/// A key deleted by hand in the console is re-created, not returned as a dead -/// id. Without a registry, "deleted by hand" and "never issued" are the same -/// observation — which is why a reveal is a reconciliation. +/// A key deleted by hand in the console answers `no_key` — it is **not** +/// resurrected by a reveal, which is the deliberate reversal of 0187's +/// behaviour: recreating a key is issuance, and issuance now lives behind the +/// eligibility round-trip. The heal is one press away, and behind the gate. #[tokio::test] -async fn a_key_deleted_by_hand_is_recreated_on_the_next_reveal() { +async fn a_key_deleted_by_hand_answers_no_key_rather_than_recreating() { let mock = MockGateway::start().await; - let first = issue(&mock, USER_ID).await.json(); + let name = format!("discord-{USER_ID}-key"); + mock.with(|s| s.seed(&name, 1_000)); + assert_eq!(reveal(&mock, USER_ID).await.status, StatusCode::OK); // Somebody opens the console and deletes it. mock.with(|s| s.keys.clear()); - let second = reveal(&mock, USER_ID).await; - assert_eq!(second.status, StatusCode::OK); - let second = second.json(); - assert_ne!( - second["key_id"], first["key_id"], - "a new key, not the old id" - ); - assert_eq!(second["created"], true); + let after = reveal(&mock, USER_ID).await; + assert_eq!(after.status, StatusCode::NOT_FOUND); + assert_eq!(after.json()["code"], "no_key"); assert_eq!( - second["key_id"], - mock.with(|s| s.keys[0].id.clone()), - "the id returned must be one that exists" + mock.with(|s| s.create_calls), + 0, + "the reveal must not recreate" ); } /// The narrower race: the key is listed, and gone by the time its value is -/// read. The flow re-runs rather than reporting a failure or a dead id. +/// read. `no_key`, not an error and not a retry-into-create — the observation +/// is the same as "deleted by hand", and so is the honest answer. #[tokio::test] -async fn a_key_that_vanishes_between_the_list_and_the_read_is_not_a_dead_id() { +async fn a_key_that_vanishes_between_the_list_and_the_read_answers_no_key() { let mock = MockGateway::start().await; let name = format!("discord-{USER_ID}-key"); mock.with(|s| { @@ -332,33 +257,17 @@ async fn a_key_that_vanishes_between_the_list_and_the_read_is_not_a_dead_id() { }); let reply = reveal(&mock, USER_ID).await; - assert_eq!(reply.status, StatusCode::OK); - let body = reply.json(); - - let live: Vec = mock.with(|s| s.keys.iter().map(|k| k.id.clone()).collect()); - assert!( - live.contains(&body["key_id"].as_str().unwrap().to_string()), - "returned {:?}, which is not among the keys that exist: {live:?}", - body["key_id"] - ); - assert_eq!(body["created"], true, "the replacement was created"); + assert_eq!(reply.status, StatusCode::NOT_FOUND); + assert_eq!(reply.json()["code"], "no_key"); + assert_eq!(mock.with(|s| s.create_calls), 0); } -/// A listing that keeps returning a key the read will not produce: every -/// attempt settles on a winner it cannot get a value for. -/// -/// The answer is a `503` — "try again", because the condition is transient — -/// rather than a `502`, which would blame AWS for a control plane that is -/// answering both calls, or an unbounded retry, which would spend the rest of -/// the Lambda's budget and return a timeout instead of an answer. -/// -/// **Reaching this branch takes a stale LISTING, not a busy deleter**, and that -/// is worth recording: an attempt that creates the winner already holds its -/// value and never reads it back, so a mock that merely deletes keys on read -/// makes the next attempt create one and succeed. The retry exists for a reader -/// and a lister that disagree. +/// A listing that keeps returning a key the read will not produce — a stale +/// lister and a reader that disagree. One pass, `no_key`, done: 0187's +/// bounded retry existed to give the CREATE path a second chance, and with no +/// create there is nothing a second identical read would learn. #[tokio::test] -async fn a_winner_whose_value_never_reads_is_a_bounded_503() { +async fn a_stale_listing_whose_keys_never_read_answers_no_key_in_one_pass() { let mock = MockGateway::start().await; let name = format!("discord-{USER_ID}-key"); mock.with(|s| { @@ -367,76 +276,87 @@ async fn a_winner_whose_value_never_reads_is_a_bounded_503() { }); let reply = reveal(&mock, USER_ID).await; - assert_eq!(reply.status, StatusCode::SERVICE_UNAVAILABLE); - assert_eq!(reply.json()["code"], "key_unavailable"); - assert_eq!(reply.cache_control(), "no-store"); - // Bounded at two attempts, so this is an answer rather than a timeout. + assert_eq!(reply.status, StatusCode::NOT_FOUND); + assert_eq!(reply.json()["code"], "no_key"); assert_eq!( mock.with(|s| s.list_calls), - 2, - "one listing per attempt, and exactly two attempts" + 1, + "a lookup is one pass, not a retry loop" ); - assert_eq!(mock.with(|s| s.create_calls), 0, "nothing was created"); + assert_eq!(mock.with(|s| s.create_calls), 0); +} + +/// Signing out and back in is a new cookie for the same Discord id, and the +/// key is a property of the id — so it survives. This is the criterion that +/// would fail if anything about the key were kept in the session. +#[tokio::test] +async fn signing_out_and_back_in_still_shows_the_same_key() { + let mock = MockGateway::start().await; + let name = format!("discord-{USER_ID}-key"); + mock.with(|s| s.seed(&name, 1_000)); + + // Two independently minted cookies, as two sign-ins would mint them. + let before = reveal(&mock, USER_ID).await.json(); + let after = reveal(&mock, USER_ID).await.json(); + assert_eq!(before["key_id"], after["key_id"]); + assert_eq!(before["value"], after["value"]); } // --------------------------------------------------------------------------- -// The prefix hazard (AC 9) +// The prefix hazard // --------------------------------------------------------------------------- -/// A shorter Discord id is a prefix of a longer one, and `nameQuery` is a prefix -/// match. The `-key` suffix plus the exact filter are what stop user `123…` from -/// seeing — or **deleting** — the key of user `123…9`. -/// -/// The mock matches by prefix exactly as the service does, so this test fails if -/// either guard is removed. +/// A shorter Discord id is a prefix of a longer one, and `nameQuery` is a +/// prefix match. The `-key` suffix plus the exact filter are what stop user +/// `123…` from seeing the key of user `123…9` — and a read-only route still +/// must not *reveal* across the boundary, even though it can no longer delete. #[tokio::test] -async fn a_user_whose_id_prefixes_another_can_neither_see_nor_delete_their_key() { +async fn a_user_whose_id_prefixes_another_cannot_see_their_key() { let mock = MockGateway::start().await; let victim_name = "discord-1234567890123456789-key"; let victim = mock.with(|s| s.seed(victim_name, 1)); - let body = issue(&mock, "123456789012345678").await.json(); + let reply = call( + app_against(&mock), + "GET", + Some(&session_cookie("123456789012345678")), + ) + .await; - assert_ne!(body["key_id"], victim); - assert!( - mock.with(|s| s.keys.iter().any(|k| k.id == victim)), - "the other user's key was deleted by the reconciler" + assert_eq!( + reply.status, + StatusCode::NOT_FOUND, + "the neighbour's key is not theirs" ); + assert_eq!(reply.json()["code"], "no_key"); assert!( - mock.with(|s| s.deleted.is_empty()), - "nothing at all should have been deleted" + mock.with(|s| s.keys.iter().any(|k| k.id == victim)), + "the other user's key must survive untouched" ); + assert!(mock.with(|s| s.deleted.is_empty())); } -/// Names a human typed in the console prefix ours too, and the reconciler holds -/// `DeleteApiKey`. They are neither returned nor deleted. +/// Names a human typed in the console prefix ours too. They are neither +/// returned nor (self-evidently now) deleted. #[tokio::test] -async fn console_created_lookalikes_are_neither_returned_nor_deleted() { +async fn console_created_lookalikes_are_not_revealed() { let mock = MockGateway::start().await; let name = format!("discord-{USER_ID}-key"); - let lookalikes = mock.with(|s| { - [ - s.seed(&format!("{name}-old"), 1), - s.seed(&format!("{name}s"), 2), - s.seed(&format!("{name}-BACKUP"), 3), - ] + mock.with(|s| { + s.seed(&format!("{name}-old"), 1); + s.seed(&format!("{name}s"), 2); + s.seed(&format!("{name}-BACKUP"), 3); }); - let body = issue(&mock, USER_ID).await.json(); - - assert_eq!(body["created"], true, "none of those was ours"); - for id in lookalikes { - assert!( - mock.with(|s| s.keys.iter().any(|k| k.id == id)), - "a console-created key named like ours was deleted" - ); - assert_ne!(body["key_id"], id); - } + let reply = reveal(&mock, USER_ID).await; + assert_eq!(reply.status, StatusCode::NOT_FOUND, "none of those is ours"); + assert_eq!(reply.json()["code"], "no_key"); + assert_eq!(mock.with(|s| s.keys.len()), 3); assert!(mock.with(|s| s.deleted.is_empty())); } // --------------------------------------------------------------------------- -// Caching, secrecy and failure (AC 10) +// Caching, secrecy and failure // --------------------------------------------------------------------------- /// The handler's own half of "reveal is not cached". The other two halves are @@ -446,23 +366,34 @@ async fn console_created_lookalikes_are_neither_returned_nor_deleted() { #[tokio::test] async fn every_answer_carries_no_store() { let mock = MockGateway::start().await; - assert_eq!(issue(&mock, USER_ID).await.cache_control(), "no-store"); + let name = format!("discord-{USER_ID}-key"); + mock.with(|s| s.seed(&name, 1_000)); + + assert_eq!(reveal(&mock, USER_ID).await.cache_control(), "no-store"); + assert_eq!(post_key(&mock, USER_ID).await.cache_control(), "no-store"); + + // The `no_key` refusal — a cached one would tell a fresh key-holder they + // have nothing. + mock.with(|s| s.keys.clear()); assert_eq!(reveal(&mock, USER_ID).await.cache_control(), "no-store"); - // Including the refusals — a cached `401` is a signed-in visitor being told - // they are signed out. + + // And the 401 — a cached one is a signed-in visitor being told they are + // signed out. assert_eq!( call(app_against(&mock), "GET", None).await.cache_control(), "no-store" ); } -/// The listing must not ask for values. A reconciler sweep that requested them -/// would pull every matched key's credential into this process for a field -/// nothing reads. +/// The listing must not ask for values. A lookup that requested them would +/// pull every matched key's credential into this process for a field nothing +/// reads until the single `GetApiKey` at the end. #[tokio::test] async fn the_listing_never_requests_key_values() { let mock = MockGateway::start().await; - issue(&mock, USER_ID).await; + let name = format!("discord-{USER_ID}-key"); + mock.with(|s| s.seed(&name, 1_000)); + reveal(&mock, USER_ID).await; let seen = mock.with(|s| s.include_values_seen.clone()); assert!(!seen.is_empty(), "the listing must have run"); for value in seen { @@ -470,30 +401,24 @@ async fn the_listing_never_requests_key_values() { } } -/// A control plane that will not answer is a `502`, not a `500`: the failure is -/// upstream, and saying so is what stops an operator hunting for a bug here -/// during an AWS incident. It is also not a `404`, which would read as "you have -/// no key" and send the visitor to press the button again. +/// A control plane that will not answer is a `502`, not a `500`: the failure +/// is upstream, and saying so is what stops an operator hunting for a bug here +/// during an AWS incident. It is also not a `404`, which would read as "you +/// have no key". #[tokio::test] async fn a_control_plane_failure_is_a_502() { let mock = MockGateway::start().await; mock.with(|s| s.fail_list = true); - let reply = issue(&mock, USER_ID).await; + let reply = reveal(&mock, USER_ID).await; assert_eq!(reply.status, StatusCode::BAD_GATEWAY); assert_eq!(reply.json()["code"], "key_unavailable"); assert_eq!(reply.cache_control(), "no-store"); assert_eq!(mock.with(|s| s.create_calls), 0); } -/// A `GetApiKey` that fails for a reason that is **not** "gone" is a `502`, not -/// a silent re-issue. -/// -/// The distinction lives in one line of `Gateway::value_of`: a `404` becomes -/// `Ok(None)` and re-enters the flow, everything else is an error. Get that -/// wrong in the permissive direction — treat any failure as "gone" — and an -/// AccessDenied or a throttle would make the handler create a **second** key for -/// a user who already has one, on every request, which is the one thing the -/// reconciler exists to prevent. +/// A `GetApiKey` that fails for a reason that is **not** "gone" is a `502`, +/// not `no_key`: an AccessDenied or a throttle must not read as "you have no +/// key" and send the visitor off to re-issue. #[tokio::test] async fn a_read_that_fails_for_any_other_reason_is_a_502_and_creates_nothing() { let mock = MockGateway::start().await; @@ -514,234 +439,10 @@ async fn a_read_that_fails_for_any_other_reason_is_a_502_and_creates_nothing() { assert_eq!(mock.with(|s| s.keys.len()), 1, "and nothing was deleted"); } -/// **A key that exists but is on no usage plan is put on it, not handed out as -/// it is.** -/// -/// Three things produce such a key and only the first is ours to prevent: a -/// `CreateApiKey` that succeeded followed by a `CreateUsagePlanKey` that failed -/// (these are control-plane calls, throttled hard); a `CreateApiKey` that timed -/// out *after* the service made the key, so no id ever reached us; and somebody -/// creating one with this exact name in the console. -/// -/// While the attach lived on the create path, all three were **permanent**: the -/// next request adopted the orphan, answered `200`, and the holder had a key -/// that returns `403` from `/v1/` with no retry that could fix it — every retry -/// took the same branch. This is the test that fails if the attach is moved back -/// there. -#[tokio::test] -async fn an_adopted_key_is_put_on_the_free_plan() { - let mock = MockGateway::start().await; - let name = format!("discord-{USER_ID}-key"); - let orphan = mock.with(|s| s.seed(&name, 1_000)); - - let body = issue(&mock, USER_ID).await.json(); - - assert_eq!(body["key_id"], orphan, "the existing key is adopted"); - assert_eq!(body["created"], false, "and it was not re-created"); - assert_eq!( - mock.with(|s| s.plan_keys.clone()), - vec![(PLAN_ID.to_string(), orphan)], - "an adopted key is only useful once it is on the plan" - ); - assert_eq!( - mock.with(|s| s.create_calls), - 0, - "adopting must not mint a second key" - ); -} - -/// The attach runs on **every** request, so the ordinary case is a key that is -/// already on the plan — which API Gateway answers with `409 ConflictException`. -/// -/// That is the desired state, not a failure, and this is what makes attaching -/// unconditionally affordable: the conflict costs one call and changes nothing. -/// The mock answers `409` like the service, so a handler that treated the -/// conflict as an error would fail here rather than in production on the second -/// press. -#[tokio::test] -async fn a_key_already_on_the_plan_stays_on_it_and_is_not_an_error() { - let mock = MockGateway::start().await; - - let first = issue(&mock, USER_ID).await; - let second = issue(&mock, USER_ID).await; - let third = reveal(&mock, USER_ID).await; - - assert_eq!(first.status, StatusCode::OK); - assert_eq!(second.status, StatusCode::OK); - assert_eq!(third.status, StatusCode::OK); - assert_eq!(first.json()["key_id"], third.json()["key_id"]); - - let key_id = first.json()["key_id"].as_str().unwrap().to_string(); - assert_eq!( - mock.with(|s| s.plan_keys.clone()), - vec![(PLAN_ID.to_string(), key_id)], - "one membership, however many times it was asserted" - ); - assert_eq!( - mock.with(|s| s.attach_calls), - 3, - "one attach per request — the conflict is the answer, not a retry" - ); -} - -/// **A key that vanishes before the ATTACH is not a dead end either.** -/// -/// The sibling of `a_key_that_vanishes_between_the_list_and_the_read_is_not_a_dead_id`, -/// and it exists because attaching the winner moved that race earlier: the -/// attach now runs before the read, so a key deleted in the console after the -/// listing is observed by `CreateUsagePlanKey` first. Reporting that `404` as a -/// control-plane failure would have restored the dead end this slice removed — -/// on a narrower path, which is the kind that stays broken because nothing -/// exercises it. -#[tokio::test] -async fn a_key_that_vanishes_before_the_attach_is_not_a_dead_id() { - let mock = MockGateway::start().await; - let name = format!("discord-{USER_ID}-key"); - let doomed = mock.with(|s| { - let id = s.seed(&name, 1_000); - s.vanish_on_next_attach = true; - id - }); - - let reply = reveal(&mock, USER_ID).await; - - assert_eq!(reply.status, StatusCode::OK); - let body = reply.json(); - assert_ne!(body["key_id"], doomed, "the vanished key is not handed out"); - assert_eq!(body["created"], true, "a replacement was made"); - let survivors = mock.with(|s| { - s.named(&name) - .iter() - .map(|k| k.id.clone()) - .collect::>() - }); - assert_eq!( - survivors, - vec![body["key_id"].as_str().unwrap().to_string()], - "and the id returned is the one that exists" - ); - assert_eq!( - mock.with(|s| s.plan_keys.len()), - 1, - "the replacement is on the plan" - ); -} - -/// **A duplicate that will not delete does not withhold the key.** -/// -/// By the time the deletions run, the winner is created, attached and ready. -/// Propagating a failed `DeleteApiKey` would answer `502` and hand the caller -/// nothing — housekeeping denying the thing the request was for — and it would -/// not even be transient: task 0194 may put an `aws:ResourceTag/ManagedBy` -/// condition on `DELETE`, and an exact-name duplicate created by hand in the -/// console carries no tag, so it would fail on every request forever. -#[tokio::test] -async fn a_duplicate_that_will_not_delete_does_not_withhold_the_key() { - let mock = MockGateway::start().await; - let name = format!("discord-{USER_ID}-key"); - let earliest = mock.with(|s| { - let earliest = s.seed(&name, 1_000); - s.seed(&name, 2_000); - s.fail_deletes = true; - earliest - }); - - let reply = issue(&mock, USER_ID).await; - - assert_eq!(reply.status, StatusCode::OK); - assert_eq!( - reply.json()["key_id"], - earliest, - "the winner is still the earliest, and it is still handed out" - ); - assert_eq!( - mock.with(|s| s.keys.len()), - 2, - "the duplicate survives, for the next reconciliation to try again" - ); - assert_eq!( - mock.with(|s| s.create_calls), - 0, - "and nothing was re-created to work around it" - ); -} - -/// **A usage plan that does not exist is a `502`, not "try again".** -/// -/// API Gateway reports a missing key and a missing usage plan with the same -/// `NotFoundException`. Reading it as "the key is gone" turns a stale or -/// mistyped plan id — a typo in `PORTAL_FREE_PLAN_ID` on a local run, an SSM -/// parameter that drifted, a plan replaced under a warm container — into -/// `Ok(None)`, which the caller retries, exhausts, and reports as *"your key is -/// being changed by something else right now; try again"*. Forever, for every -/// user, while each request leaves an enabled key attached to nothing. -#[tokio::test] -async fn a_usage_plan_that_does_not_exist_is_a_502_not_a_transient_503() { - let mock = MockGateway::start().await; - mock.with(|s| s.attach_always_404 = true); - - let reply = issue(&mock, USER_ID).await; - - assert_eq!( - reply.status, - StatusCode::BAD_GATEWAY, - "a broken deployment must not read as a transient race" - ); - assert_eq!(reply.json()["code"], "key_unavailable"); - assert_eq!( - mock.with(|s| s.create_calls), - 1, - "and it must not keep minting keys while it is broken" - ); -} - -/// **A listing that has not caught up must not orphan the key we just made.** -/// -/// `GetApiKeys` is eventually consistent. When the re-list comes back non-empty -/// but without our key, the record was neither ranked nor deleted — a live, -/// enabled production key with this user's exact name, on no plan, left until -/// the user happens to return. The union puts it back in the running, so it is -/// either handed out or reconciled away like any other duplicate. -#[tokio::test] -async fn a_key_the_listing_has_not_caught_up_with_is_not_orphaned() { - let mock = MockGateway::start().await; - let name = format!("discord-{USER_ID}-key"); - let older = mock.with(|s| { - let older = s.seed(&name, 1_000); - // The first listing does not show the seeded key, so the handler creates - // one; the second shows the seeded key but not the one just created. - s.next_list_is_empty = true; - s.next_list_omits_newest = true; - older - }); - - let reply = issue(&mock, USER_ID).await; - - assert_eq!(reply.status, StatusCode::OK); - assert_eq!( - reply.json()["key_id"], - older, - "the earlier key still wins the rank" - ); - assert_eq!( - mock.with(|s| s.keys.len()), - 1, - "the key created during this request was reconciled away, not orphaned" - ); - assert_eq!( - mock.with(|s| s.plan_keys.clone()), - vec![(PLAN_ID.to_string(), older)], - "and the survivor is on the plan" - ); -} - -/// **A slow control plane gets an answer, not a dead invocation.** -/// -/// The per-call timeouts do not compose into a request-level bound: one attempt -/// makes up to six calls plus a delete per duplicate, a listing can walk 50 -/// pages each with its own budget, and the reconciler runs twice. Against a 15s -/// Lambda that is a killed invocation — no body, no `no-store`, an entry on the -/// `Errors` metric. The wall-clock deadline is what turns that into a `503`. +/// **A slow control plane gets an answer, not a dead invocation.** The +/// per-call timeouts do not compose into a request-level bound — a listing can +/// walk 50 pages each with its own budget. Against a 15s Lambda that is a +/// killed invocation; the wall-clock deadline turns it into a `503`. #[tokio::test] async fn a_control_plane_slower_than_the_deadline_answers_503() { let mock = MockGateway::start().await; diff --git a/packages/prices-api/tests/portal_keys/harness.rs b/packages/prices-api/tests/portal_keys/harness.rs index af4251a7..cf477503 100644 --- a/packages/prices-api/tests/portal_keys/harness.rs +++ b/packages/prices-api/tests/portal_keys/harness.rs @@ -581,18 +581,48 @@ pub fn oauth_secret() -> OauthSecret { } pub fn build_app(portal_enabled: bool, gateway: Option) -> Router { + build_app_with(portal_enabled, gateway, Default::default(), None) +} + +/// [`build_app`], plus where Discord is and where the eligibility knobs come +/// from — what the issue round-trip suite (`tests/portal_issue.rs`) needs: +/// the `action=issue` callback talks to a mock Discord AND the mock control +/// plane in one request. +pub fn build_app_with( + portal_enabled: bool, + gateway: Option, + endpoints: prices_api::portal::auth::discord::Endpoints, + eligibility: Option, +) -> Router { let config = AppConfig { ch_enabled: false, base_url: None, api_keys: vec![], portal_enabled, portal_oauth: portal_enabled.then(oauth_secret), - portal_endpoints: Default::default(), + portal_endpoints: endpoints, portal_keys: gateway, + portal_eligibility: eligibility, }; app(&config, AppState::without_ch()) } +/// Eligibility knobs for tests: direct values, no SSM. The guild id is a +/// snowflake because the code validates that before building the member URL. +pub fn eligibility( + guild_id: &str, + min_age_minutes: &str, +) -> prices_api::portal::eligibility::EligibilitySettings { + use prices_api::portal::eligibility::{EligibilitySettings, ParamSource}; + EligibilitySettings { + guild_id: ParamSource::Direct(guild_id.to_string()), + min_account_age: ParamSource::Direct(min_age_minutes.to_string()), + } +} + +/// The guild the issue suite gates on — any syntactically valid snowflake. +pub const GUILD_ID: &str = "897514728459468821"; + /// A router with the portal open, sign-in configured, and the control plane /// pointed at `mock`. pub fn app_against(mock: &MockGateway) -> Router { @@ -659,6 +689,34 @@ impl Reply { .map(|v| v.to_str().unwrap().to_string()) .unwrap_or_default() } + + pub fn location(&self) -> String { + self.headers + .get(header::LOCATION) + .map(|v| v.to_str().unwrap().to_string()) + .unwrap_or_default() + } + + /// The value a browser would store for cookie `name`, or `None` if this + /// response clears it or never set it. + pub fn cookie(&self, name: &str) -> Option { + self.headers + .get_all(header::SET_COOKIE) + .iter() + .map(|v| v.to_str().unwrap().to_string()) + .filter(|c| c.starts_with(&format!("{name}="))) + .filter(|c| !c.contains("Max-Age=0")) + .map(|c| { + c.split_once('=') + .unwrap() + .1 + .split(';') + .next() + .unwrap() + .to_string() + }) + .next() + } } /// The usage route alone (task 0188), with the cache TTL and the deadline both @@ -711,10 +769,55 @@ pub async fn call_path(router: Router, method: &str, path: &str, cookie: Option< } } -pub async fn issue(mock: &MockGateway, sub: &str) -> Reply { +/// `GET /key` — the reveal. Since task 0189 the `POST` verb answers +/// identically (read-only); [`post_key`] exists so tests can pin exactly that. +pub async fn reveal(mock: &MockGateway, sub: &str) -> Reply { + call(app_against(mock), "GET", Some(&session_cookie(sub))).await +} + +/// `POST /key` — 0187's issue verb, which task 0189 made a second reveal. +pub async fn post_key(mock: &MockGateway, sub: &str) -> Reply { call(app_against(mock), "POST", Some(&session_cookie(sub))).await } -pub async fn reveal(mock: &MockGateway, sub: &str) -> Reply { - call(app_against(mock), "GET", Some(&session_cookie(sub))).await +/// Drive a full `action=issue` OAuth round-trip against `router` (which must +/// be built with `build_app_with`, pointed at a mock Discord): `/auth/login` +/// mints the state pair, the callback completes the action. Returns the +/// callback's reply — a `303` whose `Location` is one of the five +/// `?issue=…` landing states. +pub async fn issue_round_trip(router: &Router) -> Reply { + let login = call_path( + router.clone(), + "GET", + "/api-tokens/api/auth/login?action=issue", + None, + ) + .await; + assert_eq!( + login.status, + StatusCode::SEE_OTHER, + "login must redirect to Discord: {}", + String::from_utf8_lossy(&login.body) + ); + let pending = login + .cookie(cookies::PENDING_COOKIE) + .expect("login must set the pending-login cookie"); + let location = login.location(); + let query = location + .split_once('?') + .expect("the authorize URL carries a query") + .1; + let state = form_urlencoded::parse(query.as_bytes()) + .find(|(k, _)| k == "state") + .expect("the authorize URL must carry `state`") + .1 + .to_string(); + + call_path( + router.clone(), + "GET", + &format!("/api-tokens/api/auth/callback?code=an-auth-code&state={state}"), + Some(&format!("{}={pending}", cookies::PENDING_COOKIE)), + ) + .await } diff --git a/packages/prices-api/tests/portal_keys_logs.rs b/packages/prices-api/tests/portal_keys_logs.rs index a1d48c76..8c5d020b 100644 --- a/packages/prices-api/tests/portal_keys_logs.rs +++ b/packages/prices-api/tests/portal_keys_logs.rs @@ -38,10 +38,13 @@ #[path = "portal_keys/harness.rs"] mod harness; +#[path = "common/mock_discord.rs"] +mod mock_discord; use std::sync::{Arc, Mutex}; use harness::*; +use mock_discord::{GRANTED_SCOPE, MockDiscord}; /// A `MakeWriter` that keeps every byte the subscriber emits. #[derive(Clone, Default)] @@ -96,8 +99,27 @@ async fn no_key_value_ever_reaches_the_logs() { s.seed(&name, 2_000); }); - let body = issue(&mock, USER_ID).await.json(); - reveal(&mock, USER_ID).await; + // The full issue round-trip (task 0189): the create and delete paths run + // through the callback now, and the delete path is the one that + // legitimately names key IDS in its logs — the assertion has to be about + // the credential, not about log lines being sparse. Then the reveal, which + // is the path the VALUE flows through. + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let app = build_app_with( + true, + Some(prices_api::portal::keys::gateway::Gateway::against( + &mock.base, + PLAN_ID.to_string(), + )), + prices_api::portal::auth::discord::Endpoints { + api_base: discord.base.clone(), + ..Default::default() + }, + Some(eligibility(GUILD_ID, "5")), + ); + let issued = issue_round_trip(&app).await; + assert_eq!(issued.location(), "/api-tokens/?issue=ok"); + let body = reveal(&mock, USER_ID).await.json(); let text = String::from_utf8_lossy(&logs.0.lock().unwrap()).to_string(); @@ -108,8 +130,12 @@ async fn no_key_value_ever_reaches_the_logs() { "the subscriber captured no TRACE events, so this test proves nothing: {text:?}" ); assert!( - text.contains("portal issued or revealed an API key"), - "the routes did not run under this subscriber, so this test proves nothing: {text:?}" + text.contains("portal issued an API key"), + "the issue flow did not run under this subscriber, so this test proves nothing: {text:?}" + ); + assert!( + text.contains("portal revealed an API key"), + "the reveal did not run under this subscriber, so this test proves nothing: {text:?}" ); let value = body["value"].as_str().unwrap(); diff --git a/packages/prices-api/tests/portal_usage.rs b/packages/prices-api/tests/portal_usage.rs index d36fe181..af7ffb4f 100644 --- a/packages/prices-api/tests/portal_usage.rs +++ b/packages/prices-api/tests/portal_usage.rs @@ -506,11 +506,14 @@ async fn a_served_stale_answer_backs_off_the_next_load() { ); } -/// Issuing a key evicts a cached "no key": without this, the page's own -/// refetch after the press — and any reload for the next minute — would be -/// served the stale `NoKey` and tell a key-holder they have no key. +/// A reveal that finds a key evicts a cached "no key": without this, a reload +/// inside the TTL after the key appears (issued through 0189's round-trip, or +/// adopted) would be served the stale `NoKey` and tell a key-holder they have +/// no key. The eviction on the ISSUE path itself is asserted where the issue +/// path now lives, in `tests/portal_issue.rs`'s happy-path round trip; this +/// pins the reveal's half, which is what the page's own refetch hits. #[tokio::test] -async fn issuing_a_key_evicts_a_cached_no_key() { +async fn a_reveal_that_finds_a_key_evicts_a_cached_no_key() { let mock = MockGateway::start().await; let router = app_against(&mock); @@ -524,8 +527,14 @@ async fn issuing_a_key_evicts_a_cached_no_key() { assert_eq!(before.status, StatusCode::NOT_FOUND); assert_eq!(before.json()["code"], "no_key"); - let issued = call(router.clone(), "POST", Some(&session_cookie(USER_ID))).await; - assert_eq!(issued.status, StatusCode::OK); + // The key appears — 0189's callback created it in another request. + mock.with(|s| { + let id = s.seed(&format!("discord-{USER_ID}-key"), 1_000); + s.plan_keys.push((PLAN_ID.to_string(), id)); + }); + + let revealed = call(router.clone(), "GET", Some(&session_cookie(USER_ID))).await; + assert_eq!(revealed.status, StatusCode::OK); // Inside the 60s TTL — only the eviction can explain a non-stale answer. let after = call_path(router, "GET", USAGE_PATH, Some(&session_cookie(USER_ID))).await; diff --git a/packages/prices-api/tests/price.rs b/packages/prices-api/tests/price.rs index 5153207e..0d0bf465 100644 --- a/packages/prices-api/tests/price.rs +++ b/packages/prices-api/tests/price.rs @@ -27,6 +27,7 @@ fn test_config() -> AppConfig { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, } } diff --git a/packages/prices-api/tests/price_it.rs b/packages/prices-api/tests/price_it.rs index 47e4d260..9fca31a8 100644 --- a/packages/prices-api/tests/price_it.rs +++ b/packages/prices-api/tests/price_it.rs @@ -97,6 +97,7 @@ fn config() -> AppConfig { // is what every non-portal test wants — with no client in the // config there is no code path here that can reach API Gateway. portal_keys: None, + portal_eligibility: None, } } From 5246ecb5690f4fc668a81f8955b027a7073bb96a Mon Sep 17 00:00:00 2001 From: Adam <65679285+adamkoot@users.noreply.github.com> Date: Thu, 20 Aug 2026 11:36:38 +0200 Subject: [PATCH 03/11] feat(lore-0189): frontend for the eligibility gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'Get my API key' becomes a top-level link into the issue round-trip; the key section fetches the (now read-only) reveal on mount and renders the five ?issue= landing states in the wording this task decides: not-a-member names the server and links discord.gg/stellardev, too-young renders the backend's wait_secs as a wait rather than a rejection, and could-not-verify is explicitly not an accusation. Landing params are one-shot — read once, stripped from the URL — which also closes 0186's O10 stale-banner item. The landing page states both prerequisites before the visitor authenticates. --- web/portal/src/api/portal.ts | 63 +++-- web/portal/src/app/app.spec.tsx | 419 +++++++++++++++++++++++--------- web/portal/src/app/app.tsx | 276 ++++++++++++++++----- 3 files changed, 561 insertions(+), 197 deletions(-) diff --git a/web/portal/src/api/portal.ts b/web/portal/src/api/portal.ts index 3d33ed85..00fe8230 100644 --- a/web/portal/src/api/portal.ts +++ b/web/portal/src/api/portal.ts @@ -77,13 +77,13 @@ export class PortalApiError extends Error { const PROBE_TIMEOUT_MS = 10_000; /** - * How long the page waits on key issuance. + * How long the page waits on the key reveal. * * Longer than {@link PROBE_TIMEOUT_MS}, because this call is not a probe: a cold - * Lambda resolves credentials, reads an SSM parameter and then makes up to five - * control-plane calls. The api-handler's own Lambda timeout is 15s and API - * Gateway cuts everything off at 29s, so 20s is inside the window where an - * answer — including a `502` — is still possible. + * Lambda resolves credentials, reads an SSM parameter and then walks a + * paginated listing plus a read. The api-handler's own Lambda timeout is 15s + * and API Gateway cuts everything off at 29s, so 20s is inside the window + * where an answer — including a `502` — is still possible. */ const KEY_TIMEOUT_MS = 20_000; @@ -197,6 +197,20 @@ export const fetchSession = (): Promise => */ export const signInUrl = (): string => `${PORTAL_API}/auth/login`; +/** + * Where the "Get my API key" control points (task 0189). + * + * The same plain-link reasoning as {@link signInUrl}, because issuing a key IS + * an OAuth round-trip now: eligibility (Stellar Discord membership + minimum + * account age) is proved per action by re-authentication, never carried in the + * session — a fresh Discord token is what the backend checks membership with, + * and only a top-level navigation can fetch one. Discord does not re-prompt + * for consent on repeat authorisation of the same scopes, so for a signed-in + * visitor this is a redirect, not a login. The callback lands back on the + * portal with `?issue=`, which `app.tsx` renders. + */ +export const issueUrl = (): string => `${PORTAL_API}/auth/login?action=issue`; + /** * `POST /api-tokens/api/auth/logout` — clear the session. * @@ -226,7 +240,8 @@ export async function signOut(): Promise { } /** - * What `POST` and `GET /api-tokens/api/key` answer (task 0187). + * What `GET` (and `POST`) `/api-tokens/api/key` answer (task 0187, read-only + * since task 0189 — the route reveals and never creates). * * Mirrors `KeyResponse` in `packages/prices-api/src/portal/keys/mod.rs`, and * hand-written for the same reason the two types above are: the portal's routes @@ -239,8 +254,6 @@ export interface PortalKey { name: string; /** The credential itself — what goes in `X-API-Key`. */ value: string; - /** Whether this request created the key, as opposed to finding it. */ - created: boolean; } /** @@ -334,24 +347,25 @@ export async function fetchUsage(): Promise { } /** - * Issue a key, or return the one this account already has. + * Reveal the key this account already has, or learn there is none. * - * `POST`, and deliberately not a `GET` the page fires on load. The backend - * treats both verbs identically (it must — without a registry, "deleted by - * hand" and "never issued" are the same observation), so keeping the page's - * only call behind a press is what makes the visitor's intent explicit rather - * than implied by having opened a URL. + * `GET`, and — since task 0189 — safe to fire on load: the backend's key route + * is **read-only by construction** (it can never create, attach or delete), + * tested as such, so opening the dashboard cannot mint anything. That reverses + * 0187's fetch-nothing-on-mount rule by re-deriving it rather than ignoring + * it: the rule existed because the route could create, and it no longer can. + * Creating is `issueUrl()`'s round-trip, behind an explicit press. * - * Idempotent: a second press returns the same key, not a second one. That is a - * property of the backend's reconciler, not of this function, and it is why - * there is no client-side guard against double-clicking. + * Resolves to `null` when the caller has no key (the backend's `404 no_key` + * envelope) — a renderable state, not a failure — with the same gate-404 + * caution `fetchUsage` takes: an EMPTY 404 is task 0183's closed portal, and + * reading it as "you have no key" would be a false statement. */ -export async function issueKey(): Promise { +export async function fetchKey(): Promise { const url = `${PORTAL_API}/key`; let response: Response; try { response = await fetch(url, { - method: 'POST', headers: { accept: 'application/json' }, signal: AbortSignal.timeout(KEY_TIMEOUT_MS), }); @@ -363,6 +377,17 @@ export async function issueKey(): Promise { } throw new PortalApiError(`${url} could not be reached`); } + if (response.status === 404) { + try { + const body = (await response.json()) as { code?: string }; + if (body.code === 'no_key') { + return null; + } + } catch { + // Not JSON — the gate's empty 404, or something else entirely. + } + throw new PortalApiError(`${url} answered 404`, 404); + } if (!response.ok) { // `401` is the one status this page can act on: it means the session // expired while the tab was open, and the answer is to sign in again rather diff --git a/web/portal/src/app/app.spec.tsx b/web/portal/src/app/app.spec.tsx index c7be6a83..56ab786b 100644 --- a/web/portal/src/app/app.spec.tsx +++ b/web/portal/src/app/app.spec.tsx @@ -1,10 +1,21 @@ import { fireEvent, render, screen, waitFor } from '@testing-library/react'; -import { MemoryRouter } from 'react-router-dom'; +import { MemoryRouter, useLocation } from 'react-router-dom'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { ROUTER_BASENAME } from '../base-path'; import App from './app'; +/** + * Records the router's current query string, so the one-shot landing-param + * tests (task 0189, closing 0186's O10) can assert the URL was cleaned while + * the banner stayed — `MemoryRouter` has no `window.location` to inspect. + */ +let lastSearch = ''; +function LocationSpy() { + lastSearch = useLocation().search; + return null; +} + /** * The gate is the only behaviour in this slice worth testing, and both of its * branches matter for different reasons: the closed one is what every visitor @@ -59,6 +70,9 @@ const CONFIG_URL = '/api-tokens/api/config'; const ME_URL = '/api-tokens/api/auth/me'; const LOGOUT_URL = '/api-tokens/api/auth/logout'; const USAGE_URL = '/api-tokens/api/usage'; +const KEY_URL = '/api-tokens/api/key'; +/** Where both "get my API key" and every retry link point (task 0189). */ +const ISSUE_HREF = '/api-tokens/api/auth/login?action=issue'; /** * The usage endpoint's "no key yet" answer (task 0188) — the default for every @@ -72,6 +86,17 @@ const usageNoKey = () => ({ json: async () => ({ code: 'no_key', message: 'you have no API key yet' }), }); +/** + * The key route's "no key" answer — the same envelope discipline as + * `usageNoKey`, and the default for signed-in stubs since task 0189 made the + * route read-only and the page fetch it on mount. + */ +const keyNoKey = () => ({ + ok: false, + status: 404, + json: async () => ({ code: 'no_key', message: 'you have no API key yet' }), +}); + /** The portal open, and nobody signed in. */ const openAndSignedOut = () => stubRoutes({ @@ -90,6 +115,7 @@ const openAndSignedIn = () => username: 'adam', }), }), + [KEY_URL]: keyNoKey, [USAGE_URL]: usageNoKey, }); @@ -333,6 +359,54 @@ describe('sign in with Discord', () => { expect(screen.getByRole('heading', { level: 1 })).toBeTruthy(); }); + /** + * The acceptance criterion (task 0189): both prerequisites are stated + * **before** the visitor authenticates — learning about the membership + * requirement after the consent screen means they authorised an app for + * nothing. The invite is the registered vanity code; the account-age line + * names no number, because the threshold is operator configuration the + * backend reports when it matters. + */ + it('states both prerequisites before the visitor authenticates', async () => { + openAndSignedOut(); + renderAt('/'); + + await screen.findByRole('link', { name: /sign in with discord/i }); + expect( + screen.getByRole('link', { name: /stellar developers discord/i }), + ).toBeTruthy(); + expect( + screen + .getByRole('link', { name: /stellar developers discord/i }) + .getAttribute('href'), + ).toBe('https://discord.gg/stellardev'); + expect(screen.getByText(/not brand new/i)).toBeTruthy(); + // Not a hard-coded threshold — that would drift the moment the SSM + // parameter changes. + expect(document.body.textContent).not.toMatch(/5 minutes/i); + }); + + /** + * The landing param is one-shot (task 0189, closing 0186's O10): the banner + * renders for the landing that carried it, and the URL is cleaned in place — + * so a sign-out after a cancelled attempt, or a reload, shows no stale + * "Sign-in cancelled". + */ + it('clears the signin outcome from the URL so it cannot go stale', async () => { + openAndSignedOut(); + render( + + + + , + ); + + expect(await screen.findByText(/sign-in cancelled/i)).toBeTruthy(); + await waitFor(() => expect(lastSearch).not.toContain('signin')); + // The banner survives the cleanup — it belongs to this landing. + expect(screen.getByText(/sign-in cancelled/i)).toBeTruthy(); + }); + /** * The visitor pressed Cancel at Discord's consent screen; the callback * redirected to `/api-tokens/?signin=cancelled`. Plain text, and the button @@ -396,6 +470,7 @@ describe('sign in with Discord', () => { authenticated = false; return { status: 204, json: async () => ({}) }; }, + [KEY_URL]: keyNoKey, [USAGE_URL]: usageNoKey, }); @@ -471,12 +546,14 @@ describe('sign in with Discord', () => { }); /** - * The API key (task 0187). + * The API key (task 0187; issuance re-shaped by task 0189). * * Every test here starts signed in, because that is the only state the control * exists in. `fetch` is stubbed rather than the module mocked, so these also - * cover `issueKey` in `src/api/portal.ts` — including that its URL is relative - * and its verb is `POST`. + * cover `fetchKey` in `src/api/portal.ts` — including that its URL is relative + * and that nothing this page does POSTs to the key route: issuing is a + * top-level navigation through the eligibility round-trip, and the round-trip + * outcomes land back here as `?issue=`. */ describe('the API key', () => { beforeEach(() => { @@ -486,7 +563,6 @@ describe('the API key', () => { vi.unstubAllGlobals(); }); - const KEY_URL = '/api-tokens/api/key'; const KEY_VALUE = 'aBcDeF0123456789aBcDeF0123456789aBcDeF01'; const signedInWithKey = ( @@ -494,7 +570,6 @@ describe('the API key', () => { key_id: 'abc123', name: 'discord-308994132968210433-key', value: KEY_VALUE, - created: true, }, ) => stubRoutes({ @@ -510,40 +585,42 @@ describe('the API key', () => { [USAGE_URL]: usageNoKey, }); - const renderApp = () => + const signedInWithoutKey = ( + keyRoute: () => Partial & { json?: () => unknown } = keyNoKey, + ) => + stubRoutes({ + [CONFIG_URL]: () => ({ json: async () => ({ enabled: true }) }), + [ME_URL]: () => ({ + json: async () => ({ + authenticated: true, + user_id: '308994132968210433', + username: 'adam', + }), + }), + [KEY_URL]: keyRoute, + [USAGE_URL]: usageNoKey, + }); + + const renderApp = (entry = '/') => render( - + + , ); /** - * **Nothing is fetched until the visitor presses.** The backend's `GET` and - * `POST` on `/key` are the same operation — without a registry it cannot tell - * "deleted by hand" from "never issued", so a reveal has to be able to create - * — which means a page that asked on load would issue a real production API - * key to anyone who merely opened it. + * **The reversal of 0187's fetch-nothing rule, re-derived.** The key route + * is read-only since task 0189 — it can never create — so the page shows + * the visitor the key they already have without a press. What it must never + * do is POST its way to one: issuing is the eligibility round-trip. */ - it('issues nothing until the button is pressed', async () => { + it('fetches the existing key on mount and shows it masked', async () => { const fetchMock = signedInWithKey(); renderApp(); - await screen.findByRole('button', { name: /get my api key/i }); - expect(fetchMock.mock.calls.some(([url]) => url === KEY_URL)).toBe(false); - }); - - it('issues the key on a relative POST and shows it masked', async () => { - const fetchMock = signedInWithKey(); - renderApp(); - - fireEvent.click( - await screen.findByRole('button', { name: /get my api key/i }), - ); - const shown = await screen.findByTestId('api-key'); - // Masked on arrival — the visitor asked for a key, not for it to appear on - // screen while they were looking at the button. This renders during - // screen-shares. + // Masked on arrival — this renders during screen-shares. expect(shown.textContent).not.toContain(KEY_VALUE); expect(shown.textContent).toMatch(/^•+$/); @@ -552,7 +629,57 @@ describe('the API key', () => { RequestInit, ]; expect(call[0].startsWith('http')).toBe(false); - expect(call[1].method).toBe('POST'); + // A GET — and nothing on this page ever POSTs the key route. + expect(call[1]?.method).toBeUndefined(); + expect( + fetchMock.mock.calls.some( + ([url, init]) => + url === KEY_URL && (init as RequestInit | undefined)?.method, + ), + ).toBe(false); + }); + + /** + * No key: the control is a **link into the issue round-trip**, not a button + * with a fetch — the eligibility proof needs a fresh Discord token, which + * only a top-level navigation can carry — and the prerequisites are stated + * right where the decision is made. + */ + it('offers get-my-api-key as a link into the issue round-trip, not a fetch', async () => { + const fetchMock = signedInWithoutKey(); + renderApp(); + + const link = await screen.findByRole('link', { name: /get my api key/i }); + expect(link.getAttribute('href')).toBe(ISSUE_HREF); + expect(link.getAttribute('href')?.startsWith('http')).toBe(false); + // The prerequisites, at the point of decision. + expect(screen.getAllByText(/not brand new/i).length).toBeGreaterThan(0); + // And no request was made that could have created anything. + expect( + fetchMock.mock.calls.every( + ([, init]) => !(init as RequestInit | undefined)?.method, + ), + ).toBe(true); + }); + + /** + * Only the backend's own `no_key` envelope means "no key". The gate's empty + * 404 (task 0183, reachable when the portal closes under an open tab) must + * render as a stated failure — offering "get my API key" against a closed + * portal would send the visitor into a round-trip that answers 404. + */ + it("does not read the gate's empty 404 as no key", async () => { + signedInWithoutKey(() => ({ + ok: false, + status: 404, + json: async () => { + throw new SyntaxError('Unexpected end of JSON input'); + }, + })); + renderApp(); + + expect(await screen.findByText(/could not get your api key/i)).toBeTruthy(); + expect(screen.queryByRole('link', { name: /get my api key/i })).toBeNull(); }); /** @@ -563,9 +690,6 @@ describe('the API key', () => { it('leaks no part of the value while masked', async () => { signedInWithKey(); renderApp(); - fireEvent.click( - await screen.findByRole('button', { name: /get my api key/i }), - ); await screen.findByTestId('api-key'); for (const fragment of [ @@ -580,9 +704,6 @@ describe('the API key', () => { it('reveals and re-hides the value on the toggle', async () => { signedInWithKey(); renderApp(); - fireEvent.click( - await screen.findByRole('button', { name: /get my api key/i }), - ); await screen.findByTestId('api-key'); fireEvent.click(screen.getByRole('button', { name: /^reveal$/i })); @@ -597,9 +718,6 @@ describe('the API key', () => { vi.stubGlobal('navigator', { clipboard: { writeText } }); signedInWithKey(); renderApp(); - fireEvent.click( - await screen.findByRole('button', { name: /get my api key/i }), - ); await screen.findByTestId('api-key'); fireEvent.click(screen.getByRole('button', { name: /^copy$/i })); @@ -620,9 +738,6 @@ describe('the API key', () => { vi.stubGlobal('navigator', {}); signedInWithKey(); renderApp(); - fireEvent.click( - await screen.findByRole('button', { name: /get my api key/i }), - ); await screen.findByTestId('api-key'); fireEvent.click(screen.getByRole('button', { name: /^copy$/i })); @@ -632,59 +747,23 @@ describe('the API key', () => { expect(screen.getByTestId('api-key')).toBeTruthy(); }); - it('reports a failure and leaves the button pressable', async () => { - stubRoutes({ - [CONFIG_URL]: () => ({ json: async () => ({ enabled: true }) }), - [ME_URL]: () => ({ - json: async () => ({ - authenticated: true, - user_id: '308994132968210433', - username: 'adam', - }), - }), - [KEY_URL]: () => ({ ok: false, status: 502 }), - [USAGE_URL]: usageNoKey, - }); + it('reports a reveal failure as a stated failure', async () => { + signedInWithoutKey(() => ({ ok: false, status: 502 })); renderApp(); - fireEvent.click( - await screen.findByRole('button', { name: /get my api key/i }), - ); - expect(await screen.findByText(/could not get your api key/i)).toBeTruthy(); - // A dead end is worse than a failure: a `502` here is usually transient, so - // the control the visitor would use to retry has to survive it. - expect( - screen.getByRole('button', { name: /get my api key/i }), - ).toBeTruthy(); }); /** - * `401` is the one failure with an answer other than "press it again": the + * `401` is the one failure with an answer other than "try again": the * session expired while the tab sat open. `api/portal.ts` carries the status * through for exactly this, and without this branch that would be a promise - * the page did not keep — the visitor would be told "answered 401" and left - * to work out that they need to sign in again. + * the page did not keep. */ it('tells the visitor to sign in again when the session has expired', async () => { - stubRoutes({ - [CONFIG_URL]: () => ({ json: async () => ({ enabled: true }) }), - [ME_URL]: () => ({ - json: async () => ({ - authenticated: true, - user_id: '308994132968210433', - username: 'adam', - }), - }), - [KEY_URL]: () => ({ ok: false, status: 401 }), - [USAGE_URL]: usageNoKey, - }); + signedInWithoutKey(() => ({ ok: false, status: 401 })); renderApp(); - fireEvent.click( - await screen.findByRole('button', { name: /get my api key/i }), - ); - expect(await screen.findByText(/session has expired/i)).toBeTruthy(); // And not the raw status, which says nothing a visitor can act on. expect(document.body.textContent).not.toContain('answered 401'); @@ -696,16 +775,13 @@ describe('the API key', () => { renderApp(); await screen.findByRole('link', { name: /sign in with discord/i }); - expect( - screen.queryByRole('button', { name: /get my api key/i }), - ).toBeNull(); + expect(screen.queryByRole('link', { name: /get my api key/i })).toBeNull(); expect(screen.queryByTestId('api-key')).toBeNull(); }); /** - * The closed portal must offer no key control at all: the route answers an - * empty `404` while the flag is off, and until task 0189's eligibility gate - * lands that flag is the only thing between a stranger and a real key. + * The closed portal must offer no key control at all: the whole flow — + * login, callback, reveal — answers an empty `404` while the flag is off. */ it('is not rendered while the portal is closed', async () => { stubFetch({ json: async () => ({ enabled: false }) }); @@ -713,6 +789,118 @@ describe('the API key', () => { await screen.findByText(/not yet available/i); expect(screen.queryAllByRole('button')).toHaveLength(0); + expect(screen.queryAllByRole('link')).toHaveLength(0); + }); + + // ------------------------------------------------------------------------- + // The issue round-trip's landing states (task 0189) — the wording this task + // decides, and 0193 restyles without re-deciding. + // ------------------------------------------------------------------------- + + it('welcomes a completed issue and shows the key', async () => { + signedInWithKey(); + renderApp('/?issue=ok'); + + expect(await screen.findByTestId('issue-ok')).toBeTruthy(); + expect(await screen.findByTestId('api-key')).toBeTruthy(); + }); + + /** + * Not a member: name the server, link the registered vanity invite, and + * offer retry as the same round-trip — eligibility is proved per attempt, + * never remembered, so joining and pressing again is all it takes. + */ + it('names the server and links the invite when the visitor is not a member', async () => { + signedInWithoutKey(); + renderApp('/?issue=not_member'); + + const refusal = await screen.findByTestId('issue-not-member'); + expect(refusal.textContent).toMatch(/stellar developers discord/i); + const invite = screen.getAllByRole('link', { + name: /stellar developers discord/i, + })[0]; + expect(invite.getAttribute('href')).toBe('https://discord.gg/stellardev'); + const retry = screen.getAllByRole('link', { name: /try again/i })[0]; + expect(retry.getAttribute('href')).toBe(ISSUE_HREF); + }); + + /** + * Too young is a WAIT, not a rejection: the remaining time comes from the + * backend's `wait_secs` — never a calendar date (that pattern is 0191's, + * for a weeks-long cap), and never a hard-coded "5 minutes". + */ + it("renders too-young as a wait with the backend's remaining time", async () => { + signedInWithoutKey(); + renderApp('/?issue=too_young&wait_secs=173'); + + const refusal = await screen.findByTestId('issue-too-young'); + expect(refusal.textContent).toMatch(/about 3 minutes/i); + expect(refusal.textContent).toMatch(/not a rejection/i); + // Not a calendar date, and not a number the backend did not send. + expect(refusal.textContent).not.toMatch(/\d{4}-\d{2}-\d{2}/); + expect(refusal.textContent).not.toMatch(/5 minutes/i); + const retry = screen.getAllByRole('link', { name: /get my api key/i })[0]; + expect(retry.getAttribute('href')).toBe(ISSUE_HREF); + }); + + /** `wait_secs` arrives in a URL, so nonsense renders as generic wording. */ + it('sanitises a nonsense wait_secs instead of rendering it', async () => { + signedInWithoutKey(); + renderApp('/?issue=too_young&wait_secs=