diff --git a/.trash/rework.rs b/.trash/rework.rs new file mode 100644 index 00000000..3874fe04 --- /dev/null +++ b/.trash/rework.rs @@ -0,0 +1,285 @@ +//! Completing an `action=rework` round-trip (task 0191). +//! +//! The shape of `super::issue`, with two differences that are the whole of the +//! task: the check is **membership only** — an account old enough once is old +//! enough forever, so age is never re-proved on a rework — and what happens +//! after the check is a **swap** rather than a reconcile: the old key is +//! deleted and a new one created in one operation, capped at once per quota +//! period (`keys::cap`). +//! +//! # Why a round-trip, and not a `POST` with the session cookie +//! +//! The eligibility table (`eligibility`, ADR 0010 §8) says a rework re-proves +//! membership, and a proof is a fresh Discord token, which only a top-level +//! navigation can fetch. So "rework is unreachable with a session cookie +//! alone" is structural here exactly as "issue is unreachable with a session +//! cookie alone" is in `issue`: the only route a session cookie reaches is +//! `POST /key/rework`, the read-only pre-check, which writes nothing. +//! +//! # Every outcome is a redirect, every target a literal +//! +//! | query | meaning | +//! | --- | --- | +//! | `?rework=ok` | the old key is gone, the new one is on the plan — the page reveals it | +//! | `?rework=capped&next_eligible_at=YYYY-MM-DD` | the current key was created inside this period; nothing moved. The date is ours (`period`) | +//! | `?rework=no_key` | there was nothing to replace — the page offers the issue round-trip | +//! | `?rework=not_member` | Discord confirmed no such membership, or screening not cleared | +//! | `?rework=unknown` | membership could **not** be verified — refused without accusation | +//! | `?rework=failed` | our key service could not complete the swap. Never a statement about the visitor | +//! | `?rework=cancelled` / `?rework=denied` | the round-trip ended at Discord before any check ran — `issue`'s pair, for the same reasons | +//! +//! `capped` is a *verdict*, not a failure, and it is the one with a date: the +//! wait is weeks, and "try again later" would be a lie by omission. The date +//! travels as `YYYY-MM-DD` — digits and dashes by construction (`keys::cap`) +//! — so no request-derived byte reaches a `Location` header through it. +//! +//! # Precedence: membership, then the cap +//! +//! A departed member is told to rejoin before being told to wait, because the +//! membership answer is already in hand (the token was just exchanged) and +//! the cap needs a control-plane listing. Both are refused; the order decides +//! which fixable thing the visitor hears about first. + +use std::time::Instant; + +use axum::response::Response; + +use super::super::eligibility::{self, Membership}; +use super::super::keys::{self, ReworkOutcome}; +use super::discord::{self, AccessToken, MemberLookup}; +use super::issue::{ISSUE_BUDGET, RECONCILE_FLOOR}; +use super::secret::OauthSecret; +use super::session::{self, Session}; +use super::state_token; +use super::{AuthState, PORTAL_HOME, cookies, redirect}; + +/// See the module table. Literals, like `?issue=…`; the dynamic one is +/// [`capped_query`], whose only variable part is a `YYYY-MM-DD` built from a +/// `NaiveDate`. +pub(super) const REWORK_OK_QUERY: &str = "?rework=ok"; +pub(super) const REWORK_NO_KEY_QUERY: &str = "?rework=no_key"; +pub(super) const REWORK_NOT_MEMBER_QUERY: &str = "?rework=not_member"; +pub(super) const REWORK_UNKNOWN_QUERY: &str = "?rework=unknown"; +pub(super) const REWORK_FAILED_QUERY: &str = "?rework=failed"; +pub(super) const REWORK_CANCELLED_QUERY: &str = "?rework=cancelled"; +pub(super) const REWORK_DENIED_QUERY: &str = "?rework=denied"; + +/// The one parameterised landing state: when the next rework becomes +/// available. `next_eligible_date` is `keys::cap`'s `YYYY-MM-DD`. +pub(super) fn capped_query(next_eligible_date: &str) -> String { + // Defence in depth on top of the type: the value is built from a + // `NaiveDate` and cannot contain anything else, but this is a `Location` + // header, so the shape is asserted where it is used. + debug_assert!( + next_eligible_date + .bytes() + .all(|b| b.is_ascii_digit() || b == b'-') + ); + format!("?rework=capped&next_eligible_at={next_eligible_date}") +} + +/// Refuse to *start* a rework round-trip on a deployment that cannot finish +/// one — `issue::refuse_issue_start`'s twin, landing on `?rework=failed`. +pub(super) fn refuse_rework_start(oauth: bool, gateway: bool, settings: bool) -> Response { + tracing::error!( + oauth, + gateway, + settings, + "a rework round-trip was started on a deployment that cannot complete one" + ); + redirect(&format!("{PORTAL_HOME}{REWORK_FAILED_QUERY}"), Vec::new()) +} + +/// Land a Discord failure that happened on an `action=rework` round-trip — +/// `issue::refuse_issue_discord`'s twin, with the same fault split: +/// `UnexpectedScope` is our registration drifting (`denied`), anything else +/// is Discord not answering (`unknown`). +pub(super) fn refuse_rework_discord( + stage: &str, + error: discord::DiscordError, + drop_pending: String, +) -> Response { + let query = match error { + discord::DiscordError::UnexpectedScope { .. } => REWORK_DENIED_QUERY, + _ => REWORK_UNKNOWN_QUERY, + }; + tracing::warn!( + stage, + error = %error, + landing = query, + "portal rework round-trip could not complete with Discord" + ); + redirect(&format!("{PORTAL_HOME}{query}"), vec![drop_pending]) +} + +/// Finish an `action=rework` callback: check membership, then swap, then +/// land. +/// +/// Runs after `state` verification, the code exchange and the granted-scope +/// check — `token` is the fresh, scope-verified token. The same ordering as +/// `issue::complete_issue`, for the same reasons: the membership call +/// **borrows** the token, the identity read **consumes** it, and the session +/// is refreshed on every outcome past identity. +/// +/// `started` is the request's arrival, not this function's — the swap spends +/// the same Lambda budget as the exchange before it (`issue::ISSUE_BUDGET`). +pub(super) async fn complete_rework( + state: &AuthState, + oauth: &OauthSecret, + token: AccessToken, + drop_pending: String, + started: Instant, +) -> Response { + // Only the guild id is needed — the age threshold is deliberately not + // read. Resolved per action, so an operator's `put-parameter` is + // honoured without a redeploy; unreadable is `unknown`, never a 5xx. + let guild_id = match state.issue.settings.as_deref() { + None => { + tracing::error!("a rework callback arrived with no eligibility settings wired"); + None + } + Some(settings) => match settings.guild_id().await { + Ok(guild_id) => Some(guild_id), + Err(error) => { + tracing::error!( + error = %error, + "the guild id could not be read; refusing the rework without accusation" + ); + None + } + }, + }; + + let member = match &guild_id { + 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 { + 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, + Err(error) => return refuse_rework_discord("identity read", error, drop_pending), + }; + + 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()], + ) + }; + + // Membership only. `eligibility::membership` is the same `pending` table + // `decide` uses, minus the age check — see that module's per-action table. + let membership = match &member { + Some(member) => eligibility::membership(member), + None => Membership::Unknown, + }; + match membership { + Membership::Member => {} + Membership::NotMember => { + tracing::info!(outcome = "not_member", "portal rework refused"); + return land(REWORK_NOT_MEMBER_QUERY); + } + Membership::Unknown => { + tracing::info!( + outcome = "unknown", + "portal rework refused without accusation" + ); + return land(REWORK_UNKNOWN_QUERY); + } + } + + let Some(gateway) = state.issue.gateway.as_deref() else { + tracing::error!("a member's rework callback arrived with no control plane wired"); + return land(REWORK_FAILED_QUERY); + }; + + // The swap is list + create + attach + one delete per old key + read, + // against what is left of the invocation — same arithmetic as the issue. + let remaining = ISSUE_BUDGET.saturating_sub(started.elapsed()); + if remaining < RECONCILE_FLOOR { + tracing::error!( + remaining_ms = remaining.as_millis() as u64, + "membership passed but the invocation's budget is spent; \ + refusing to start a swap that cannot finish" + ); + return land(REWORK_FAILED_QUERY); + } + let deadline = remaining.min(state.issue.deadline); + + match keys::rework_for(gateway, &user.id, deadline).await { + ReworkOutcome::Replaced => { + // The cached usage (if any) describes a key that no longer + // exists; the new key starts from a clean counter. + if let Some(cache) = &state.issue.usage_cache { + cache.invalidate(&user.id); + } + land(REWORK_OK_QUERY) + } + ReworkOutcome::NoKey => land(REWORK_NO_KEY_QUERY), + ReworkOutcome::Capped { next_eligible_date } => land(&capped_query(&next_eligible_date)), + ReworkOutcome::Failed => land(REWORK_FAILED_QUERY), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Every landing state is a distinct literal under the portal home — the + /// rework half of `the_issue_landing_states_are_distinct_portal_literals`. + #[test] + fn the_rework_landing_states_are_distinct_portal_literals() { + let fixed = [ + REWORK_OK_QUERY, + REWORK_NO_KEY_QUERY, + REWORK_NOT_MEMBER_QUERY, + REWORK_UNKNOWN_QUERY, + REWORK_FAILED_QUERY, + REWORK_CANCELLED_QUERY, + REWORK_DENIED_QUERY, + ]; + for (i, a) in fixed.iter().enumerate() { + assert!(a.starts_with("?rework=")); + 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 date and nothing else. + #[test] + fn the_capped_query_carries_a_bare_date() { + assert_eq!( + capped_query("2026-09-01"), + "?rework=capped&next_eligible_at=2026-09-01" + ); + } + + /// `?rework=…` and `?issue=…` never collide: the page reads them as two + /// separate one-shot params, and a rework landing must never render as + /// an issue verdict. + #[test] + fn rework_and_issue_landings_live_under_different_params() { + assert!(REWORK_OK_QUERY.starts_with("?rework=")); + assert!(super::super::issue::ISSUE_OK_QUERY.starts_with("?issue=")); + } +} diff --git a/docs/epics/self-service-onboarding.md b/docs/epics/self-service-onboarding.md index 6866577c..afd1891f 100644 --- a/docs/epics/self-service-onboarding.md +++ b/docs/epics/self-service-onboarding.md @@ -120,24 +120,35 @@ appears as an acceptance criterion in every iteration of our design response the claim that our date and AWS's coincide is unverified until measured (task 0180). If they turn out to differ, we render our date and the quota counter does its own thing; that is a UX wrinkle, not a correctness bug, because the cap is ours to define. - - **Rework is a swap, not a delete-and-wait (settled 2026-08-07):** the old key is + - **Status 2026-08-21 (task 0191):** the cap ships defined as our rule — the calendar + month, UTC, one definition in `packages/prices-api/src/portal/period.rs` shared by + the usage panel and the rework — and the `DAY`-period proxy measurement of AWS's + reset instant is recorded in task 0191's Step 0 (the 2026-08-13 run had died after + three samples and was re-run). The real `MONTH` rollover cannot be observed before + **1 September 2026**; until then the proxy is evidence, not proof, and nothing here + presents the boundary as AWS-documented. + - ~~**Rework is a swap, not a delete-and-wait (settled 2026-08-07):** the old key is deleted and a new one issued in the same operation, so a user is never left without - a working key. The cap blocks the _next_ rework, not the replacement. Eligibility is - measured from `coalesce(last_rotated_at, created_at)`, so a key issued inside the - current period cannot be reworked either — without that fallback a user could take a - key, exhaust the quota and rework into a clean counter within the same period, which - is the loophole this cap exists to close. - - **Rework flow on the dashboard (settled 2026-08-07):** the action opens a modal that - states plainly that the current key is deleted and **stops working immediately** — - anything using it breaks the moment the user confirms. The confirm button stays disabled - until the user types the phrase **`delete-key`**. A refused rework (one already performed - in the current quota period) renders the next eligible date, not a generic error. - - **Revocation is not covered.** This heading says "Rotation/revocation", but only rotation - is specified above and rotation does not appear in the acceptance criteria below. A user - whose key leaks mid-period cannot invalidate it and must wait for the period boundary. - API Gateway exposes `UpdateApiKey(enabled=false)`, which invalidates a key in one call - without touching the quota counter, so this is cheap to add and does not need to share - the rework cap. Recorded here as a known gap, deliberately deferred. + a working key. The cap blocks the _next_ rework, not the replacement.~~ + **Superseded 2026-08-21 (Adam, task 0191): rework IS delete-and-wait.** "Replace my + key" deactivates the current key immediately (`UpdateApiKey enabled=false`) and + issues nothing; a new key can be issued only from the start of the next quota + period. The owner is keyless until the 1st, and that is the point: a leaked key on + the 3rd dies on the 3rd, and the replacement being a month away is what stops + "replace" from being a free counter reset. The disabled key is the revocation + record (its `lastUpdatedDate` decides the re-issue cap) — no registry needed. + This absorbs task 0192's revoke; revoke and replace are one action. + The original reasoning stays struck through above rather than deleted, so the + 2026-08-07 decision and its reversal are both on the record. + - **Rework flow on the dashboard (settled 2026-08-07, wording updated 2026-08-21):** the + action opens a modal that states plainly that the current key is deactivated and + **stops working immediately** — anything using it breaks the moment the user confirms + — and that **no new key is issued until the next quota period**. The confirm button + stays disabled until the user types the phrase **`delete-key`**. A revoked user who + asks for a key early is shown the next eligible date, not a generic error. + - ~~**Revocation is not covered.**~~ **Covered since 2026-08-21 (task 0191):** replace + _is_ revocation — `UpdateApiKey(enabled=false)`, immediate, session-only, counter + preserved (measured, 0180 item 8) — and the re-issue waits for the next period. ## Rate limiting — override the design doc's default @@ -221,8 +232,8 @@ thirteen vertical increments, each one a user story that can be deployed and sho | 5 | `0188` | See usage against quota and the reset date | | 6 | `0189` | Only Stellar Discord members with a non-new account can get a key | | 7 | `0190` | The key registry table — deferred, and has to justify itself | -| 8 | `0191` | Replace a key, capped at once per quota period | -| 9 | `0192` | Revoke a leaked key — no replacement until the cap allows | +| 8 | `0191` | Replace my key — revoke now, re-issue next quota period (merged with `0192`) | +| 9 | `0192` | ~~Revoke a leaked key~~ — merged into `0191` on 2026-08-21, superseded | | 10 | `0193` | Make it presentable | | 11 | `0194` | Audit the assembled configuration (Tranche 3 AC 6) | | 12 | `0195` | Swagger UI, per-prefix SPA fallback, custom domain | diff --git a/docs/runbooks/manual-api-key-tier.md b/docs/runbooks/manual-api-key-tier.md index f1fcc16f..c7da940f 100644 --- a/docs/runbooks/manual-api-key-tier.md +++ b/docs/runbooks/manual-api-key-tier.md @@ -347,8 +347,12 @@ Then delete the row from the table below. `MONTH` is calendar-aligned or runs from plan creation. Note also that `QuotaSettings.offset` is _"the number of requests subtracted from the given limit in the initial time period"_ — a request count, not a way to shift the - reset day, so it cannot be used to force alignment. Unverified; task 0180 #7. - Do not promise a customer a specific reset date until it is measured. + reset day, so it cannot be used to force alignment. Task 0180 #7, carried to + task 0191, which measured the `DAY`-period proxy (result and date in that + task's Step 0 table) — a `MONTH` rollover itself cannot be observed before + 1 September 2026. The portal states "the 1st, 00:00 UTC" as **our** period + rule for its own quota cap and dashboard; do not promise a customer that AWS's + counter resets at that instant until the `MONTH` observation exists. --- diff --git a/docs/runbooks/portal-oauth-deploy-prep.md b/docs/runbooks/portal-oauth-deploy-prep.md index caa9ab6f..f481a950 100644 --- a/docs/runbooks/portal-oauth-deploy-prep.md +++ b/docs/runbooks/portal-oauth-deploy-prep.md @@ -406,8 +406,9 @@ any behaviour until the flag moves. ### The IAM, and the three limits that come with it -CDK grants the api-handler role six control-plane actions and nothing else: -`GET`/`POST` on `/apikeys`, `GET`/`DELETE` on `/apikeys/*`, `POST` on +CDK grants the api-handler role seven control-plane actions and nothing else: +`GET`/`POST` on `/apikeys`, `GET`/`PATCH`/`DELETE` on `/apikeys/*` (`PATCH` +is task 0191's revoke — see below), `POST` on `/usageplans/{the free plan}/keys`, and — task 0188 — `GET` on `/usageplans/{the free plan}/usage` (`GetUsage`, the dashboard's usage read). The last two are declared in `api-gateway-stack.ts` rather than @@ -425,16 +426,44 @@ three are written out in full in `compute-stack.ts`; the short version: partner keys included. The handler always asks for `includeValues=false`, so this is what code execution in the Lambda would buy an attacker, not what the feature does. -- **`DELETE /apikeys/*` CAN be narrowed** with an `aws:ResourceTag/ManagedBy` - condition — API Gateway supports tag conditions on control-plane actions, and - the keys are already tagged. It is not written yet: task 0194 owns it, because - it should be verified against the deployed stack and it changes behaviour for - a console-created duplicate (untagged → `AccessDenied` → `502` instead of - reconciling). Until then the reconciler's blast radius is bounded by code, not - by IAM. +- **`GET`/`PATCH`/`DELETE` on `/apikeys/*` are tag-scoped** to + `ManagedBy=prices-portal` (task 0191). Without the condition the per-key + `GET` was the real exposure — `GetApiKey(includeValue=true)` reads the value + of any key in the account by id — and `PATCH` could rename a partner key + into a portal name. The behaviour change to know: an exact-name key created + **by hand** in the console is untagged and is no longer adopted; the routes + answer `502` and the key is left alone. The listing (`/apikeys`) cannot + take the condition and stays account-wide (values never requested). Task 0194 audits all three. +### Replacing a key (task 0191) — one new grant, and a wait that is the point + +"Replace my key" on the dashboard is a **revocation**: `UpdateApiKey +(enabled=false)` on the caller's key, session-authorized, no Discord +round-trip (a leaked key has to be killable while Discord is down). Nothing +is issued: the replacement is an ordinary issue round-trip, and the issue path +refuses it until the quota period of the revocation has rolled — the 1st of +the next month, 00:00 UTC, **our** rule. That wait is deliberate: quota is +scoped to `(usagePlanId, apiKeyId)`, so a replacement issued on the spot would +be a clean counter and "replace my key" would be the button people press on +the 20th of a heavy month. + +**The seventh grant** is `apigateway:PATCH` on `/apikeys/*` +(`PortalReadDisableAndDeleteOwnApiKeys` in `compute-stack.ts`). Disable rather +than delete, because the disabled key _is_ the record of the revocation: its +`lastUpdatedDate` is what refuses the re-issue, and there is no registry +(task 0190) to hold that fact otherwise. The handler sends exactly one patch +operation, never re-enables a key, and `PATCH /apikeys/*` cannot reach a plan +or a stage. No `UpdateUsagePlan`, no `UpdateUsage`. + +Operationally: a disabled key answers `403 Forbidden`, byte-identical to no +key at all, after the usual tens of seconds of propagation (0180 item 8); its +counter is preserved, so the dashboard keeps reporting it honestly. Once the +period rolls, the next issue deletes the disabled key and creates the new one. +A revoked user who asks for a key early lands on `?issue=capped` with the +date; nothing to operate. + ### Verifying it, and the warning that comes with that The round-trip is in diff --git a/infra/src/lib/stacks/compute-stack.ts b/infra/src/lib/stacks/compute-stack.ts index 9e570ff0..30fee40e 100644 --- a/infra/src/lib/stacks/compute-stack.ts +++ b/infra/src/lib/stacks/compute-stack.ts @@ -503,7 +503,7 @@ export class ComputeStack extends cdk.Stack { // Self-service API keys (task 0187) — API Gateway CONTROL plane. // --------------------------------------------------------------- // - // Four of the six calls the portal makes. The other two — + // Five of the seven calls the portal makes. The other two — // `POST /usageplans/{id}/keys` (task 0187's attach) and // `GET /usageplans/{id}/usage` (task 0188's `GetUsage`) — are granted in // `ApiGatewayStack` instead, because the plan id lives there and importing @@ -538,43 +538,62 @@ export class ComputeStack extends cdk.Stack { // attacker with code execution in this Lambda would gain, not what the // feature does. Worth stating precisely because it is easy to read the // list of verbs as harmless next to `DELETE`. - // 3. **`DELETE` on `/apikeys/*` CAN be narrowed, and is not yet.** The path - // wildcard is forced — AWS generates the key id, so it is unknowable at - // synth time — but the resource is not the only axis available. API - // Gateway supports `aws:ResourceTag/${TagKey}` conditions on control-plane - // actions + // 3. **`PATCH` on `/apikeys/*` IS narrowed by tag; `GET` and `DELETE` are + // still not.** The path wildcard is forced on all three — AWS generates + // the key id, so it is unknowable at synth time — but API Gateway + // supports `aws:ResourceTag/${TagKey}` conditions on per-key + // control-plane actions // (docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-tagging-iam-policy.html), - // and every key this feature creates already carries - // `ManagedBy=prices-portal`, so the condition that would reduce "delete - // any API key in the account, including a partner's" to "delete a key - // this portal made" is available and unwritten. + // and every key this feature creates carries `ManagedBy=prices-portal` + // from the create call. // - // It is left to **task 0194**, which owns the IAM audit and can verify it - // against the deployed stack rather than a synth — and it carries one - // trade-off that has to be decided rather than assumed: an exact-name - // duplicate created BY HAND in the console has no tag, so a tag-scoped - // `DELETE` would fail it with `AccessDenied` and the reconciler would - // answer `502` instead of converging. That is arguably the better - // outcome — this service deleting a key a human made is the case the - // name guard exists for — but it is a behaviour change, not a tightening. - // Do NOT put the condition on `GET`: adopting a console-created key is a - // documented requirement of this slice. + // **The new verb is born narrow.** `PATCH` is task 0191's and nothing + // depends on it being account-wide, so it gets the condition in the same + // change that grants it — an unconditioned per-key patch could rename any + // key in the account into a portal name the reconciler would then adopt + // and reveal, or re-enable a key its owner revoked. It lives in its own + // statement for exactly this reason: a condition on a shared statement + // would silently reach the two verbs below. // - // Until then, the guard that actually holds is in the handler - // (`portal/keys/naming.rs`), which never ranks or deletes a key whose - // name is not exactly the caller's — a guard in code, on a grant that is - // account-wide in IAM. + // **`GET` and `DELETE` stay as task 0187 left them — deliberately.** The + // condition that would reduce "read or delete any API key in the account, + // including a partner's" to "one this portal made" is still available and + // still unwritten, and it is still **task 0194**'s, which owns the IAM + // audit and can verify it against the deployed stack rather than a synth. + // Writing it here would be a behaviour change to two shipped code paths + // smuggled into a feature slice: an exact-name key created BY HAND in the + // console is untagged, and adoption is decided by NAME + // (`naming::exact_matches` + `current_key`), not by tag — so such a key is + // still listed (the collection grant below carries no condition), still + // ranked winner, still attached, and only then `AccessDenied`s on + // `GetApiKey includeValue=true`. The visitor gets a permanent `502` on a + // key the portal chose for them. Adopting a console-created key is a + // documented requirement of 0187; retiring it is a decision 0194 makes + // with the audit in hand, not a side effect of shipping a revoke. // - // What is deliberately NOT here: `PATCH` (nothing in this slice updates a - // key), `apigateway:*`, and any grant on `/usageplans` beyond the key + // Until then, the guard that actually holds on those two is in the + // handler (`portal/keys/naming.rs`), which never ranks or deletes a key + // whose name is not exactly the caller's — a guard in code, on a grant + // that is account-wide in IAM. + // + // What is deliberately NOT here: `apigateway:*`, `PUT /tags/*` (the portal + // never re-tags a key), and any grant on `/usageplans` beyond the key // attachment and the usage read — both of those need the plan id, so both // live in `ApiGatewayStack`'s standalone policy (`POST …/keys` for 0187's // attach, `GET …/usage` for 0188's `GetUsage`). // // `DELETE` **is** here, and it is this slice's: the reconciler removes // duplicate keys after a double-submit ("keep the earliest createdDate, - // DeleteApiKey the rest"). Task 0192's revocation will find it already - // granted; that is a coincidence of scope, not this task implementing it. + // DeleteApiKey the rest"). + // + // `PATCH` on `/apikeys/*` is task 0191's: `UpdateApiKey(enabled=false)`, + // the revocation behind "Replace my key". Disable rather than delete, + // because the disabled key IS the record of the revocation — its + // `lastUpdatedDate` is what refuses a re-issue inside the same quota + // period, and there is no registry (task 0190) to hold that fact + // otherwise. The handler sends exactly one patch operation, + // `replace /enabled false`; it never re-enables, renames or re-tags a + // key, and `PATCH` on `/apikeys/*` cannot reach a usage plan or a stage. this.apiHandlerRole.addToPrincipalPolicy( new iam.PolicyStatement({ sid: 'PortalCreateAndListApiKeys', @@ -589,6 +608,19 @@ export class ComputeStack extends cdk.Stack { resources: [`arn:aws:apigateway:${awsRegion}::/apikeys/*`], }), ); + this.apiHandlerRole.addToPrincipalPolicy( + new iam.PolicyStatement({ + sid: 'PortalDisableOwnApiKeys', + actions: ['apigateway:PATCH'], + resources: [`arn:aws:apigateway:${awsRegion}::/apikeys/*`], + // See limit 3 above: the revoke touches portal-made keys only, and + // this condition is on the new verb ALONE — do not fold this statement + // back into the one above. + conditions: { + StringEquals: { 'aws:ResourceTag/ManagedBy': 'prices-portal' }, + }, + }), + ); // The usage-plan id, read at cold start. // diff --git a/lore/1-tasks/active/0191_FEATURE_rework-key-once-per-quota-period.md b/lore/1-tasks/active/0191_FEATURE_rework-key-once-per-quota-period.md index c338a3df..1be24d13 100644 --- a/lore/1-tasks/active/0191_FEATURE_rework-key-once-per-quota-period.md +++ b/lore/1-tasks/active/0191_FEATURE_rework-key-once-per-quota-period.md @@ -1,11 +1,11 @@ --- id: "0191" -title: "Replace my key — rework, capped at once per quota period" +title: "Replace my key — revoke now, re-issue next quota period (merged with 0192)" type: FEATURE status: active related_adr: ["0010"] related_tasks: ["0183", "0157", "0160", "0180", "0187", "0189", "0190", "0192", "0193"] -tags: [layer-backend, priority-medium, effort-medium, milestone-M3, epic-self-service-onboarding, api-gateway, usage-plan, slice-8] +tags: [layer-backend, priority-medium, effort-medium, milestone-M3, epic-self-service-onboarding, api-gateway, usage-plan, security, slice-8, slice-9] milestone: 3 links: - "../../../docs/epics/self-service-onboarding.md" @@ -29,19 +29,98 @@ history: rollover measurement runs alongside the implementation; the MONTH confirmation cannot happen before 1 September 2026 and is recorded as an open limit, not assumed. - + - date: "2026-08-21" + status: active + who: akot + note: > + **Model reversed by Adam after seeing it live.** The 2026-08-07 "swap" + (delete old + issue new in one operation, capped once per period) was + built and shipped to PR #238, then replaced the same day: "Replace my + key" is a **revocation** — the key is deactivated immediately, nothing + is issued, and a new key can be issued only from the next quota period. + Disable rather than delete, so the disabled key's `lastUpdatedDate` is + the revocation record the re-issue cap reads (no registry). This + absorbs [[0192]]; the `action=rework` OAuth round-trip is gone (revoke + is session-only — a leak must be killable while Discord is down), and + one IAM grant is added (`apigateway:PATCH` on `/apikeys/*`). + - date: "2026-08-21" + status: active + who: akot + note: > + **Merged with [[0192]]** at Adam's request — the reversal above made + "Replace my key" exactly 0192's rule, so two tasks described one + feature. 0192 is archived as `superseded`; its rule, the three measured + properties it was designed around, and its unmet criterion (the ~25 s + data-plane propagation window must be said out loud) move here. Title + and tags widened (`slice-9`, `security`); file name kept because the + branch and PR #238 carry it. + - date: "2026-08-24" + status: active + who: akot + note: > + **Step 0's `DAY` proxy abandoned, deliberately, and the scratch stack + torn down.** Two runs died silently (13 and 21 August; 183 samples, all + `429`, the second dead ~9 h before the midnight it existed to observe). + Dropped rather than run a third time because the economics inverted: the + proxy existed to avoid waiting for 1 September, which is now 8 days out, + and the real `MONTH` answer comes off production from the `GetUsage` + reset warn in `keys/gateway.rs`. The criterion it served — stop calling + the boundary AWS-documented — was already met by the wording work on + 2026-08-21, and nothing in the build depends on the instant. Scratch + key/plan/API deleted 07:45Z (the script's own `teardown` reported + success while leaving the plan alive; defect recorded in the script). + Dead poll logs moved to `.trash/`. One AC left: the 1 September + confirmation. --- # Rework — a new key, once a period ## Summary -**Story:** *as a developer who has lost or leaked a key, I can generate a new -one — but not often enough to use it as a free quota reset.* +**Story:** *as a developer whose key has leaked, I can kill it immediately — +knowing that I will not have a working key again until the next quota period.* + +> **Reversed 2026-08-21.** The paragraph below is the 2026-08-07 design; it was +> implemented, then reversed by Adam on seeing it live. **Current rule:** +> "Replace my key" **deactivates** the current key now (`UpdateApiKey +> enabled=false`) and issues nothing; "Get my API key" is refused until the +> 1st of the next month (00:00 UTC, our period rule), naming the date. The +> disabled key is the record of the revocation. See Implementation Notes. -An atomic swap: the old key is deleted and a new one issued in the same +~~An atomic swap: the old key is deleted and a new one issued in the same operation, so the user is never without a working key. The cap blocks the *next* -rework, not the replacement. +rework, not the replacement.~~ + +## The revoke rule (from [[0192]]) + +**Revoking does not reset, consume or bypass the cap. It kills a key and +issues nothing.** A user who was issued a key on 3 August and revokes on the +4th is keyless until 1 September. Settled by Adam on 2026-08-13; not a default +to re-derive. + +- The cap exists so a burnt quota cannot be escaped by minting a fresh + `apiKeyId` with a clean counter (quota is per `(usagePlanId, apiKeyId)`). If + revoke handed out a replacement, "revoke" would be the button pressed on the + 20th of a heavy month. +- Being keyless is the correct cost: the same as not using the leaked key, + minus the risk of someone else using it. So the confirmation must say it is + destructive to the user's own access, with the actual date. + +Three properties measured under [[0180]] item 8 (2026-08-12) that the build +rests on: + +| Measured | Consequence | +| --- | --- | +| `UpdateApiKey(enabled=false)` **preserves** the usage counter (drained → disabled → re-enabled → still `429`) | revocation is not itself a quota reset, so it needed no cross-key tracking | +| A disabled key is `403`, **byte-identical to no key** | the portal cannot infer a revocation from the gateway; the disabled key's own record (`lastUpdatedDate`) is what the reveal and the cap read | +| Disable/enable take **~25 s** to reach the data plane | a `200` from the revoke reports the control plane, not reality — the window must be stated, not hidden | + +What 0192 planned and this merge did differently: it specified `DeleteApiKey` +plus a persisted revocation record in ClickHouse (append-only table, a write +grant from BE, a writer mTLS bundle on the Lambda). **Disabling instead of +deleting makes the key its own record**, so none of that storage work exists. +The ClickHouse sketch stays in the archived 0192 file for the day a record is +needed that a key cannot carry. ## Step 0 — measure the rollover (was [[0180]] item 7) @@ -51,29 +130,87 @@ instant nor its timezone; the only statement anywhere is an example caption, is a **request count**, not a time shift. This is ADR 0010's correction #2 and it is still open. -A `DAY`-period scratch plan is a proxy for the instant and the timezone, and the -scratch resources from the abandoned run are **still standing** for it: REST API -`9utcrbmoc6`, usage plan `ox7pv0`, key `2ke0ixjy7h`, plus -`item7-quota-rollover.sh` and its partial log in the archived -`0180_RESEARCH_.../measurement/`. Drain the key, poll across a UTC midnight, -record when `429` becomes `200`. +A `DAY`-period scratch plan was the proxy for the instant and the timezone: +drain the key, poll across a UTC midnight, record when `429` becomes `200`. +**Abandoned 2026-08-24 and the scratch stack torn down** — see the status block +below. `item7-quota-rollover.sh` stays in the archived +`0180_RESEARCH_.../measurement/` with its defects recorded at the top of the +file. -**One honest limit, stated up front: this cannot be fully settled before -1 September 2026**, the next real `MONTH` rollover. The `DAY` proxy is strong -evidence, not proof — and it is enough, because the criterion is to stop +**The honest limit that decided it: this could never be fully settled before +1 September 2026**, the next real `MONTH` rollover — the `DAY` proxy was always +evidence, not proof. It was *enough* only because the criterion is to stop presenting "00:00 UTC on the 1st" as AWS-documented, not to prove AWS's -implementation. Correct [[0157]]'s and this task's wording either way, and run -the `MONTH` confirmation on 1 September if the epic is still open. +implementation; and that criterion is met by the wording alone, which is why +losing the proxy costs nothing. The `MONTH` confirmation on 1 September stands. + +**If anything here is ever re-run:** `UpdateUsagePlan` is throttled to **1 +request per 20 seconds per account, non-adjustable**, and the whole control +plane shares a **10 rps / burst 40** budget with our deploys. A careless loop +slows CI for everyone. -**Careful with the control plane while doing it:** `UpdateUsagePlan` is throttled -to **1 request per 20 seconds per account, non-adjustable**, and the whole -control plane shares a **10 rps / burst 40** budget with our deploys. A careless -loop here slows CI for everyone. +> **Status 2026-08-24 — the `DAY` proxy is abandoned, deliberately.** Two runs +> were attempted and both died silently: 2026-08-13 (three samples, dead two +> minutes in, and the archived note kept saying "running" for a week) and +> 2026-08-21 (183 samples, 11:51Z → 15:03Z, dead ~9 h before the UTC midnight it +> existed to observe). Nothing was ever measured; nothing is invented below. +> +> Dropped rather than attempted a third time because **the economics inverted**. +> The proxy existed to avoid waiting for 1 September — 19 days away when it was +> designed, 8 days away now. A third run needs the script's defects fixed *and* +> credentials that outlive a 26 h window, to produce what this task already +> calls "evidence, not proof". +> +> **What replaces it:** the real `MONTH` rollover on 1 September, read off +> production by the warn `keys/gateway.rs` already emits when `GetUsage` reports +> a reset inside the queried period. Real data, no scratch stack, no credentials +> marathon. That is the last open AC. +> +> Nothing in the build waits on the answer — the cap is **our** rule, one +> definition in `portal/period.rs`, and a different AWS instant changes the +> dashboard label, not the cap. The wording half of the criterion (stop calling +> the boundary AWS-documented) was done 2026-08-21 and stands. +> +> **Teardown, 2026-08-24 07:45Z:** key `2ke0ixjy7h`, usage plan `ox7pv0` and +> REST API `9utcrbmoc6` deleted; the account greps clean of `lore0180*` and +> `pricing-api-free-production` (`71t9im`) is untouched. Noted because the +> script's `teardown` **reported success while leaving the plan alive** — it +> deletes the plan before the REST API that still references its stage, and +> every delete is `|| true`. The plan was removed by hand once the API was gone. + +| Question | Result | Date | +| --- | --- | --- | +| `DAY` reset instant and timezone | **not measured — abandoned 2026-08-24** after two silently-dead runs; superseded by the 1 September `MONTH` observation on production | 2026-08-24 | +| Calendar-aligned or creation-anchored? | **not measured** — dropped with the proxy; the question stays open and costs nothing, since the cap is ours | 2026-08-24 | +| `GetUsage` agreeing with enforcement at the boundary? | **not measured** — dropped with the proxy | 2026-08-24 | +| Inference for `MONTH` | **cannot be settled before 1 September 2026** — recorded as an open limit, not assumed. Whatever `DAY` shows is evidence, not proof | — | +| Text restated as our rule? | **done** — [[0157]] (dated correction), the epic doc, `manual-api-key-tier.md`, `portal/period.rs`, the usage panel's copy, the rework copy | 2026-08-21 | ## Implementation -- `POST /api-tokens/api/key/rework`. Delete the old key, `CreateApiKey` + - `CreateUsagePlanKey` for the new one, return the new value. +> **As built (2026-08-21, after the reversal):** +> +> - `POST /api-tokens/api/key/rework` — session-authorized **revoke**: +> `UpdateApiKey(enabled=false)` on every key under the caller's name; answers +> `{revoked, next_eligible_at, revoked_at}`; idempotent; `404 no_key` with +> nothing to revoke. `POST`-only + `SameSite=Lax` is the CSRF guard. +> - `GET /key` on a revoked key → `404 key_revoked` + `details.next_eligible_at` +> — the value is never revealed again, and the page shows the date instead +> of the issue link. +> - The **issue** path enforces the cap: all keys under the name disabled → +> `cap::decide(latest lastUpdatedDate, Period::now())`; capped → +> `?issue=capped&next_eligible_at=YYYY-MM-DD`, nothing written; allowed → +> the disabled records are deleted and the ordinary create/attach runs. +> - Frontend: the dialog says the key is deactivated immediately AND that no +> new key is issued until the next period; `delete-key` arms; one `POST`, +> disabled on submit; the revoked state renders the date. +> - IAM: `apigateway:PATCH` on `/apikeys/*` (the seventh grant). No +> `UpdateUsagePlan`, no `UpdateUsage`. +> +> The bullets below are the superseded 2026-08-07 spec, kept for the record. + +- ~~`POST /api-tokens/api/key/rework`. Delete the old key, `CreateApiKey` + + `CreateUsagePlanKey` for the new one, return the new value.~~ - **The cap:** allowed only when the current key's creation instant falls **before** the current quota period start (1st of the month, 00:00 UTC). Since a rework deletes and re-creates, the surviving key's `createdDate` is that @@ -104,24 +241,74 @@ loop here slows CI for everyone. ## Acceptance Criteria -- [ ] **Ships closed.** With `PORTAL_ENABLED=false` ([[0183]]) this slice's - routes return an empty `404`; with it on, they behave normally — every - deploy goes straight to production -- [ ] Item 7 measured on the `DAY`-period proxy and written up with the date; - [[0157]] and this task stop presenting the boundary as AWS-documented -- [ ] Rework issues a new key and deletes the old one in one operation; the user - is never keyless -- [ ] The old key returns `403` immediately after; the new one returns `200` -- [ ] A second attempt in the same quota period is refused with `409` and - `next_eligible_at` -- [ ] Reworking on 3 August refuses until 1 September and succeeds on - 1 September — the meeting's worked example, tested -- [ ] Rework is unreachable with a session cookie alone -- [ ] A user who has left the guild is refused on rework, with a message that - names the server rather than a generic error -- [ ] The modal states the old key dies immediately; confirm is gated on typing - `delete-key` and cannot double-fire -- [ ] `MONTH` confirmation scheduled for 1 September 2026 if the epic is open +*(Rewritten 2026-08-21 for the revoke-now / re-issue-next-period model. The +original swap-model criteria were all green at PR #238's first commit; they +are replaced, not re-counted.)* + +- [x] **Ships closed.** With `PORTAL_ENABLED=false` ([[0183]]) the revoke is an + empty `404` with a valid session and zero control-plane calls + (`revoke_is_an_empty_404_while_the_portal_is_closed`) +- [x] **The boundary is no longer presented as AWS-documented** — [[0157]], + the epic, `manual-api-key-tier.md`, `portal/period.rs` and the portal + copy all state it as our rule (2026-08-21). The `DAY`-proxy measurement + that would have backed it with evidence is **deliberately abandoned + 2026-08-24** — two runs died silently and the real `MONTH` answer is 8 + days away on production — with the scratch stack torn down. Reasoning, + teardown and replacement in Step 0 +- [x] "Replace my key" disables the key in one `UpdateApiKey` and issues + nothing — no Discord call, nothing created + (`revoke_disables_the_key_in_one_call_and_issues_nothing`); every key + under the name, not only the current one; idempotent; a failed disable + is a `502`, never a false "revoked" +- [x] The revoked value is never revealed again; the reveal answers + `key_revoked` with the date + (`a_revoked_key_is_never_revealed_again_and_the_reveal_names_the_date`). + The data-plane `403` is by construction (disabled key = `403`, measured + under 0180 item 8) and the live `curl` in `README.md` §3c +- [x] A new key cannot be issued in the period of the revocation: the issue + round-trip passes eligibility and still lands on + `?issue=capped&next_eligible_at=…` with nothing written + (`an_issue_after_a_revoke_in_the_same_period_is_capped_with_the_date`) +- [x] Revoked on the 3rd → refused until the 1st, issued once it has passed, + old record deleted, new key created and attached — tested with literal + dates in `keys/cap.rs` and relative to the real calendar over HTTP + (`revoked_on_the_3rd_refuses_until_the_1st_and_issues_once_it_has_passed`, + `the_full_cycle_issue_revoke_wait_reissue`) +- [x] A session cookie can revoke **its own** key and nothing else — `POST` + only (`GET` is `405`), unauthenticated is `401` before AWS, another + user's key and a console lookalike are untouched + (`a_session_can_only_revoke_its_own_key`) +- [x] Membership is still re-proved on the re-issue (it is an issue), and a + non-member is told to rejoin before being told to wait + (`membership_is_still_checked_before_the_cap`) +- [x] The modal states the key is deactivated immediately and that no new key + is issued until the next period; confirm is gated on typing + `delete-key` and cannot double-fire; the revoked state renders the date + (12 frontend tests) +- [x] **(from 0192)** The revoke response and the dialog do not claim + immediacy the data plane does not have: the dialog says the key stops + working "within about half a minute" and to treat it as live until + then; the revoked state renders the API's `revoked_at` as a UTC + instant ("21 August 2026, 12:00 UTC") and repeats the window anchored + on it; the word "immediately" is asserted absent. The button reads + "Deactivate my key". Tests: the dialog wording spec and + `revokes with one POST … renders the revoked state with the date`; + the backend test is renamed + `revoke_disables_the_key_in_one_call_and_issues_nothing` so its name + does not claim what the mock cannot show +- [x] **(from 0192)** Revocation works while Discord is unreachable — the + route is session-only and the revoke test makes zero Discord calls +- [x] **(from 0192)** Revocation does not reset the quota counter — measured + under 0180 item 8, not assumed; the re-issue is a new key and is what + the cap governs +- [x] **(from 0192)** The choice of `UpdateApiKey(enabled=false)` over + `DeleteApiKey` is recorded with its reasoning (decision #17; the + reverse of 0192's draft, for the reason given there) +- [ ] `MONTH` confirmation scheduled for 1 September 2026 if the epic is + open — dated note, not performed: on/after 2026-09-01 look for + `summarize_days`' `quota reset inside the queried period` warn in the + api-handler log, or re-run the `DAY` proxy script against a `MONTH` + scratch plan drained on 31 August ## Notes @@ -131,5 +318,470 @@ loop here slows CI for everyone. - If the measured AWS rollover instant differs from ours, the dashboard renders our date and the counter does its own thing. A UX wrinkle, not a correctness bug: the cap is ours to define. -- The rework cap is why a leaked key cannot be invalidated until the 1st. That - gap is [[0192]], and it is no longer blocked. +- ~~The rework cap is why a leaked key cannot be invalidated until the 1st. That + gap is [[0192]], and it is no longer blocked.~~ Closed by the reversal: the + action *is* the revocation, and [[0192]] is merged into this task. + +## Implementation Notes + +Built on `develop` with [[0188]] and [[0189]] merged, in the existing axum +router (ADR 0008), mirroring [[0189]]'s shape: the round-trip is the proof, +the callback completes the action, every outcome is a redirect to a literal. + +**New backend:** + +- `portal/period.rs` (~150 lines) — **the** quota period: calendar month, + UTC. Extracted from `usage/mod.rs` (its `current_period` and the four tests + moved here unchanged in meaning) so the dashboard's label and the rework + cap cannot disagree. Adds `start_secs()` (what a `createdDate` is compared + against), `next_start_ymd()` and `resets_at()` — the same instant. +- `portal/keys/cap.rs` (~190 lines) — the pure cap: `decide(created_at, + &Period) -> Allowed | Capped { next_eligible_at (RFC 3339), + next_eligible_date (YYYY-MM-DD) }`. Strictly *before* the period start; + an undated key is capped, not waved through. The 3 August → 1 September + example is a test with those literal dates. +- `portal/auth/rework.rs` (~280 lines) — `complete_rework`: guild id → member + (token borrowed) → identity (token consumed) → session → **membership + only** (`eligibility::membership`) → budget → `keys::rework_for` → one of + eight `?rework=` landings, declared in one place. +- `keys/mod.rs`: `POST /key/rework` (the read-only pre-check: `200 + {eligible:true}` / `409 rework_capped` + `details.next_eligible_at` / `404 + no_key`), `rework_for` + `swap` (list → cap → create → attach → delete every + old key → read back; a failed delete rolls the replacement back), + `ReworkOutcome`. + +**Changed backend:** `state_token.rs` (`Action::Rework`; `revoke` is now the +"arrives early" example), `eligibility.rs` (`membership()` extracted from +`decide`, which now calls it — one `pending` table for both paths, with a +test that they agree on every shape), `auth/mod.rs` (login refusal for both +re-auth actions, `prompt=none` on both, the cancelled/denied/exchange/dispatch +arms), `issue.rs` (`ISSUE_BUDGET`/`RECONCILE_FLOOR` shared; `IssueDeps` docs), +`usage/mod.rs` (`UsageCache::invalidate` — evict *everything* for the caller, +because after a rework the cached numbers describe a key that no longer +exists), `portal/mod.rs` (module). + +**Frontend:** `api/portal.ts` (`reworkUrl`, `checkRework`, `navigateTo` +seam), `app.tsx` (`ReplaceKey` dialog, eight `?rework=` landings, +`describeNextEligible`, the settling rule extended to `rework=ok`, one +`useOneShotParams` over both landing families). + +**No infra change.** The swap is five control-plane calls the role already +holds; no `UpdateUsagePlan`, no `UpdateUsage`, no new parameter. CI check 5 +(the `apigateway:` grant shape) passes unchanged. + +**Tests: 85 covering this task** (workspace 558 → 655 Rust, 0 failed; portal +70 → 93). + +| where | count | covers | +| --- | --- | --- | +| `period.rs` | 5 | the four moved period tests + `start_secs` | +| `cap.rs` | 6 | 3 Aug → 1 Sep both halves, issued-inside-period, the boundary second, December, undated, URL-safe date | +| `state_token.rs` / `eligibility.rs` / `rework.rs` / `auth/mod.rs` | +6 | rework pair round-trip + cross-action mismatch, `parse`, membership/decide agreement, landing literals, `prompt=none` on both re-auth actions | +| `keys/mod.rs` | +1 | the pre-check path's depth | +| `tests/portal_rework.rs` (new) | 31 | ships closed; the swap and its **write ordering**; duplicates; two simultaneous reworks; the cap on pre-check and round-trip; the worked example relative to the calendar; issued-this-period; session-alone unreachability; left-the-guild + key keeps working; 429/5xx/401/403 → unknown; `pending` absent; age not re-checked; fresh token; `prompt=none`; drifted grant; cancelled/denied; exchange/identity failures; unwired deployment; control-plane down / failed delete rollback / vanished replacement / missing plan / deadline — each leaving the old key; usage-cache eviction; re-auth identity; `no-store` | +| `tests/portal_auth.rs` | 1 reshaped | `revoke` is now the unimplemented action | +| `app.spec.tsx` | 23 new | the dialog (opens, not navigates; POST pre-check; wording; arming on the exact phrase; once-and-only-once navigation; capped renders the date and never arms; failed pre-check; cancel) and the eight landings + settling + URL cleanup | + +Verified: `cargo fmt --all --check`, `cargo clippy -p prices-api --all-targets` +(0 warnings), `cargo test --workspace` (655 passed, 0 failed), `cargo check -p +prices-api --features lambda`, `nx run-many -t lint typecheck build test -p +portal`, `nx format:check --all`, `make -C infra synth-production`, `npm run +openapi:lint`, `openapi:verify-routes`, `openapi:verify-servers`. + +## Issues Encountered + +- **The 0180 item-7 measurement had never run.** The archived note said + "running since 2026-08-13 08:10Z"; the log holds three samples and the + poller output ends at the poll start. Found while reading the data to carry + it in, not while reading the note. Corrected in the note with a date; the + result table now lives in this task so it cannot happen twice. +- **The AWS SSO session was expired for the whole build**, so the re-run + could not be started from this session. Nothing about the implementation + depends on it — recorded honestly in Step 0 and the AC rather than waited + on. +- **`window.location` is unforgeable under jsdom 22** — neither `delete` + nor `defineProperty` can replace it — so the "navigates once, never + twice" property of the confirm could not be tested against + `location.assign` directly. A one-line `navigateTo` seam in `api/portal.ts` + is what the spec mocks; everything else in that module stays real. +- **A mock `createdDate` frozen at 1 800 000 000 (2027)** would have made + every cap test pass for the wrong reason (a future date is always "inside + the period"). The mock now stamps the current time like the service, with + an override for tests that need a date. + +**Broken/modified tests** (intentional): `portal_auth.rs`'s +`login_refuses_an_action_it_does_not_implement` now uses `revoke` — `rework` +is implemented; `state_token.rs`'s "an action this build does not know" +examples likewise. `usage/mod.rs`'s four period tests moved to `period.rs`. +No behaviour of an existing route changed. + +## Design Decisions + +### From Plan + +1. **Rework is an OAuth round-trip (`action=rework`), and the callback + completes it** — ADR 0010 §8 and [[0189]]'s per-action table: membership + is re-proved against a fresh token, age is never re-checked. The `409` + + `next_eligible_at` envelope the task asks for is the **pre-check** + (`POST /key/rework`), which the dialog calls on open; the callback + decides the cap again, authoritatively, before any key moves. +2. **Cap = `createdDate < period start`**, strictly, with the period the + calendar month in UTC — our rule, defined once. An undated key is capped. +3. **Create and attach first, delete after.** Never keyless; the ordering is + a tested property of the write sequence. +4. **`409`, not `429`**; `next_eligible_at` in `details`, RFC 3339. +5. **The modal's wording** — the old key dies immediately, `delete-key` arms, + disabled on submit — and the refusal renders a calendar date. + +### Emerged + +6. **`POST /key/rework` is read-only by construction, like `/key`.** It lists + and compares; it cannot create, attach or delete. A `GET` on it is a `405`. + This keeps "rework is unreachable with a session cookie alone" structural: + the only session-reachable route cannot swap anything. +7. **Membership before the cap.** A departed member is told to rejoin before + being told to wait — the membership answer is already in hand and the cap + needs a listing. Both are refusals; the order decides which fixable thing + is heard first. +8. **Every old key under the name is deleted, not only the winner.** A + duplicate left by an earlier double-submit is a working credential the + visitor was just told had died. +9. **A failed delete rolls the replacement back** (best effort) and lands + `failed`: the visitor holds exactly the key they had, not two keys of which + the page reveals the older. +10. **A replacement that vanishes before attach, or after the old key is + gone, is `failed`, loudly** — not healed by creating a third key inside + a budget already mostly spent. The heal is one issue round-trip. Two + simultaneous reworks (two tabs) may leave two keys, both capped, which the + next issue's reconciler converges; accepted rather than serialised on a + control plane with no lock. +11. **`UsageCache::invalidate` evicts everything for the caller**, where the + issue path evicts only `NoKey`: after a rework the cached numbers are not + stale, they are about a key that no longer exists. An in-flight real + answer for the old key can still land for one TTL, dated by `as_of`. +12. **`prompt=none` joins the rework** by one variant in the condition, as + [[0189]] decision #23 said it would; sign-in still never sends it. +13. **The `Period` extraction was forced, not optional.** Two copies of "the + 1st, 00:00 UTC" were a label problem; with the cap they became a + correctness problem (a dashboard saying "resets on the 1st" beside a cap + counting from a different instant). One module, two readers. +14. **The worked example is tested twice**: with the literal 2026-08-03 → + 2026-09-01 dates in the pure cap, and relative to the real calendar over + HTTP (the 3rd of this month vs the 3rd of last) — so the HTTP suite is + valid on every day of the year without a clock seam in production code. +15. **`next_eligible_at` travels in the URL as `YYYY-MM-DD`** (digits and + dashes by construction from a `NaiveDate`) and in JSON as RFC 3339; the + page renders both as "1 September 2026" **in UTC** — rendered in the + viewer's zone, "1 September 00:00 UTC" reads as 31 August west of + Greenwich. +16. **The refusal banners all say the existing key keeps working.** A + refused rework leaves the visitor with exactly what they had; saying so + is the difference between a refusal and a scare. + +## Future Work + +Nothing new spawned — every follow-up already has an owner or a date: + +- **The `DAY` proxy re-run** → this task's Step 0, on the next AWS session + (`item7-quota-rollover.sh drain` + `poll`); **the `MONTH` confirmation** → + on/after 2026-09-01, per the last AC. +- The live `403`/`200` curl pair → the deploy + [[0164]]'s evidence pass. +- Styling of the dialog and the eight landings → [[0193]] (wording not + re-decided). +- ~~Revoke → [[0192]]~~ — merged into this task on 2026-08-21; nothing left + to hand over. +- The now-seven-call surface and the per-load cost → [[0194]]. + +## Implementation Notes — 2026-08-21, after the reversal + +The swap shipped first (the notes above), was seen live by Adam, and was +reversed the same afternoon. What changed, file by file: + +- **Removed:** `auth/rework.rs`, `Action::Rework`, the eight `?rework=` + landings, `reworkUrl`/`checkRework`/`navigateTo` on the frontend, the swap + and its 31 tests. +- **`keys/naming.rs`:** `KeyRecord` gains `enabled` and `last_updated_at`; + `current_key()` — the earliest *enabled* key, else the earliest record — + is what the reveal and the revoke act on. +- **`keys/gateway.rs`:** `disable()` (`UpdateApiKey`, one `replace /enabled + false`); `list_named`/`create` fill the two new fields. Seven calls now. +- **`keys/cap.rs`:** the same rule, re-pointed: `decide(revoked_at, period)` + — strictly before the period start, undated → capped. +- **`keys/mod.rs`:** `POST /key/rework` is the revoke (`disable_all`); the + reveal answers `key_revoked`; `attempt()` grows step 1b — all keys disabled + → cap → `Attempt::Capped`, or delete the records and fall into the create. + `IssueOutcome::Capped` → `issue.rs` lands `?issue=capped&next_eligible_at=`. +- **Frontend:** `revokeKey()`, `PortalKeyRevoked` on `fetchKey`, the + `ReplaceKey` dialog re-worded (deactivated now, no new key until the next + period), the `revoked` view with the date and a deferred issue link, + `?issue=capped` landing. +- **Infra:** `PortalReadDisableAndDeleteOwnApiKeys` — `apigateway:PATCH` + added on `/apikeys/*`, reasoning at the grant. Synth and CI check 5 green. +- **Docs:** epic decision struck through with the reversal dated; runbook §7; + README §3c; [[0192]] absorbed. + +**Tests:** `tests/portal_rework.rs` rewritten — 19 over HTTP (revoke +immediate/total/idempotent/own-key-only/502-never-false; cap on the issue path +with the worked example relative to the calendar; latest revocation governs; +live-beside-revoked; full cycle; cache eviction; `no-store`); `cap.rs` 6; +`naming.rs` +1; `app.spec.tsx` 12 for the dialog and the revoked/capped +states. Workspace 640 Rust, portal 88, 0 failed. + +### Design Decisions — emerged in the reversal + +17. **Disable, not delete.** Both propagate in ~25 s; only disabling leaves + the revocation record (`lastUpdatedDate`) that the re-issue cap needs, + and it costs one grant. The revoked value is never revealed again. +18. **Revoke is session-only.** It issues nothing and is destructive only to + the caller's own access; a leak must be killable while Discord is down. + `POST`-only + `SameSite=Lax` is the CSRF stance, re-derived as + `auth/mod.rs` requires. +19. **The cap moved to the issue path.** A revoked user's "Get my API key" + is an ordinary issue round-trip (membership + age re-proved) that the + reconciler refuses at step 1b; the `409`-with-date of the old model is now + `?issue=capped&next_eligible_at=` on the landing and `404 key_revoked` + with `details.next_eligible_at` on the reveal. +20. **The latest revocation governs**, so a stale duplicate revoked last month + cannot reopen a door closed this month. +21. **The 2026-08-07 decision is struck through, not deleted**, in the epic + and here: both the decision and its reversal are on the record. + +## Review round — 2026-08-21 audit (four lenses + two measurements) + +Two measurements made on the spot, because the findings depended on them: + +| measured | result | +| --- | --- | +| does `UpdateApiKey(enabled=false)` bump `lastUpdatedDate`? | **yes** (14:46:49 → 14:47:56 on a scratch key) — the cap has something to stand on | +| does a no-op patch, or a `description` patch, bump it? | **yes, both** — so the code must never re-patch a disabled key (it does not), and a console edit of a `discord-*` key extends its owner's cap | + +Fixed in this round (A1–A5, B7, B8 of the audit list): + +22. **One selector, one cap instant, four readers.** `naming::current_key` + (earliest live, else earliest record) and the new + `naming::revocation_instant` (the **latest** revocation; undated poisons + it) are what the reveal, the revoke, the issue path **and the usage + route** read. Before: usage ignored `enabled`, the reveal capped on the + earliest record's date while the issue capped on the latest — two + revocation records from different months made the page offer an issue + the round-trip refused. Usage now also answers `no_key` once a + revocation's period has rolled, as the reveal does. + (`the_reveal_and_the_issue_agree_on_the_cap_with_mixed_period_revocations`, + `usage_follows_the_same_key_as_the_reveal`) +23. **The re-listing after a post-roll create excludes the records just + deleted and anything disabled.** `GetApiKeys` is eventually consistent; + a phantom earlier-created record would win, 404 on attach, and spend the + single retry — leaving the new key created and unattached. + (`a_stale_listing_after_the_roll_does_not_rank_the_deleted_record`; the + mock gained a sticky `list_resurrects_deleted`) +24. **A create is started only with ≥ 4 s of the deadline left** + (`CREATE_FLOOR`), else `Attempt::OutOfTime` → `?issue=failed` with + nothing written; and **`CreateApiKey` is sent without SDK retries** — no + idempotency token, so a retried request whose first try landed is a + duplicate. (`a_lost_create_response_is_not_retried_into_duplicates`; the + mock gained `fail_next_create_after_creating`) +25. **`POST /key/rework` requires the portal's own request marker** + (`X-Requested-With: stellar-prices-portal`) and refuses + `Sec-Fetch-Site: cross-site`/`same-site` — `403 cross_site_request`, + before the session is read. `SameSite=Lax` alone is site-scoped, and + after the custom-domain cutover ([[0195]]) a sibling host's form `POST` + would carry the cookie and cost the victim their key for a month. + (`a_revoke_without_the_same_origin_markers_is_refused_before_anything_is_read`) +26. **IAM `/apikeys/*` is tag-scoped** (`aws:ResourceTag/ManagedBy = + prices-portal`) on `GET`/`PATCH`/`DELETE`. The per-key `GET` with + `includeValue=true` was the real account-wide exposure, not the listing; + `PATCH` could rename a partner key into a portal name. Accepted change: + a hand-made exact-name key is no longer adopted (`502`, left alone). + Synth and CI check 5 green; the stale "NOT here: PATCH" paragraph is + gone. +27. **Frontend:** the dashboard learns of an in-page revoke — the usage + section drops "your key is new", re-asks (the backend evicted its + cache), and says "deactivated" rather than "issue one above" when AWS + has no row; an unparseable `next_eligible_at` keeps the issue link + hidden (the safe direction); the revoke `fetch` carries the marker + header. + +Still open from the audit, deliberately: B6 (per-caller cache on the reveal +/ dedup of `GetApiKeys` across `/key` and `/usage` — the control-plane +budget), B9 ([[0205]] deploy), B10/B11 (CI allow-list; `timeoutSeconds` vs +the Rust budgets), C (dialog focus/Escape), D (ADR 0010 correction #3, epic +`:278-410`, [[0164]]'s plan, grant counts, stale comments), E (poller gap + +`429` body logging). + +## Review round — 2026-08-21 code review of the audit round + +Eight findings against the audit round's own diff; all eight closed. + +28. **An undated record no longer poisons the cap instant.** + `naming::revocation_instant` skipped to `None` on a single record without + `lastUpdatedDate`, and `None` is capped — but `next_eligible_at` is then + recomputed from the *current* period on every read, so the date rolled + forward every month and the owner was locked out permanently, with no + support action short of deleting the record. It is now the max over the + dated records; `None` only when nothing under the name is datable. + Skipping can only under-cap, and only in a shape AWS does not produce; + the lockout was permanent. + (`the_revocation_instant_is_the_latest_dated_one`, + `an_undated_duplicate_does_not_lock_the_owner_out_forever` over HTTP; the + mock's `last_updated_at` became `Option` with a `Store::undate`) +29. **The revoke stopped being a fifth reader of the cap.** It answered + `next_eligible_at: period.resets_at()` directly, so an idempotent revoke + of a key revoked two periods ago said "next month" while the reveal said + `no_key` and the round-trip would have issued — a stale tab or a + double-submit cost the visitor a month that was not owed. It now goes + through `cap::decide` like the other four readers, and answers *now* when + the period has rolled. + (`an_idempotent_revoke_after_the_period_rolled_says_a_key_is_due_now`) +30. **`revoked_at` is nullable rather than an invented epoch.** + `unwrap_or(0)` rendered as "deactivated on 1 January 1970"; the field is + `Option` in the JSON, and the page renders the revocation with no + instant instead. `describeUtcInstant` returns `null` for a missing or + unparseable value — its old fallbacks were "just now" (a claim about a + revocation that may be weeks old) and, via `describeNextEligible`, + "deactivated on the start of the next quota period", the *next-eligible* + phrase presented as the revocation instant. + (`renders an undated revocation without inventing an instant`) +31. **The propagation window is present-tense only while it is open.** The + revoked view renders on every page load through the reveal, so "it stops + working within about half a minute — until then treat it as live" was + telling the owner of a key that died last week to keep treating it as + live. Past tense beyond `PROPAGATION_FRESH_MS` (5 min); the measured + window is still named, only the tense changes. + (`states the propagation window in the past tense for an old revocation`) +32. **`RECONCILE_FLOOR` (2s) below `CREATE_FLOOR` (4s) is recorded as + deliberate, not reconciled away.** Between the two only an *adoption* can + end in a key — a create is refused, because one cut off by the deadline + leaves an enabled unattached key. Raising the floor would save one + `GetApiKeys` on a doomed first press and cost every returning press its + recovery. Both constants now document the gap and a `const` assertion + fails if the ordering is ever inverted. +- **Docs:** `README.md` §3c no longer says "deactivates the key + immediately" — the claim this round removed from the dialog and asserts + absent in the spec; it states the ~25 s window instead. `api/portal.ts`'s + `revokeKey` doc no longer credits `POST` + `SameSite` as the CSRF guard, + which the backend explicitly rejects: the marker header is. + +**Tests:** 648 Rust across the workspace (+3 for this round), portal 93 (+3), 0 failed. `cargo fmt --check`, +`clippy --all-targets` (0 warnings), `cargo check --features lambda`, +`nx run-many -t lint typecheck build test -p portal`, `nx format:check --all` +all green. + +--- + +## Review round — 2026-08-24, karczuRF's code review of PR #238 + +Seven findings, all verified against the branch before acting; five were real +as written, two needed correcting first. What that verification changed: + +- **Finding 2's stated invariant was the wrong way round.** `app.tsx`'s dialog + doc guarantees *"revoked" is never shown for a key that still works* — the + failure copy breaks the **opposite** direction (claiming a key is still + active when a disable may have landed). The defect is real; only the reason + given for it was. +- **Finding 6 overstated the user-facing hole.** The usage panel does render + with nothing marking the key dead, but the key section above it renders + `key-revoked` on the same screen. The defect is the comment, which points a + future reader at copy that is not in that branch. +- **Two things the review missed**, both found while checking it: the `PATCH` + paragraph in `compute-stack.ts` still said the tag condition was "available + to task 0194" while limit 3 twenty lines above said 0191 had written it; and + `disable_all` dated the revocation from **this process's clock** while every + later `cap::decide` reads AWS's `lastUpdatedDate` — invisible except across + 00:00 UTC on the 1st, where the two fall in different quota periods and the + page promises a replacement a month early. + +### Decisions + +33. **The tag condition goes on `PATCH` alone, in its own statement — `GET` + and `DELETE` go back to 0187's unconditioned grant.** The condition was + written across all three verbs once `PATCH` joined the statement. That is + a behaviour change to two shipped code paths inside a feature slice, and + 0187's own comment forbade it in as many words ("Do NOT put the condition + on `GET`: adopting a console-created key is a documented requirement of + this slice"). The new verb is still born narrow — nothing depends on + `PATCH` being account-wide — and narrowing the other two stays **task + 0194's**, which owns the IAM audit and can verify against the deployed + stack. The separate `PortalDisableOwnApiKeys` sid is deliberate: a + condition on a shared statement silently reaches every action in it. + + Also corrected in that comment: "so it is no longer adopted" was wrong. + Adoption is by NAME (`exact_matches` + `current_key`), not by tag — an + untagged console key is still listed, ranked and attached, and only then + `AccessDenied`s on the value read. The outcome the comment described was + right; the mechanism was not. + +34. **The post-roll cleanup logs and steps over a failed delete, like the + loser sweep does.** `attempt()`'s `gateway.delete(&dead.id).await?` was the + one place the "housekeeping must not withhold the key the request is for" + rule was not applied — and it is the worse place for it, because that + branch runs on *every* press once the name holds nothing but revoked keys. + One undeletable record meant `?issue=failed` forever with no in-product + recovery. Records that fail now stay in `revoked` for the end-of-function + sweep to retry instead of being cleared. + +35. **A revocation is `Done`, `Partial` or an error — three outcomes, not + two.** Propagating the first failed disable answered `502`, and `502` copy + has to describe a state the page cannot know. Now: at least one disable + landed and none failed → `Done`; some landed and some failed → `Partial`, + which reaches the page as `partial: true` and renders "one of your keys + could not be deactivated, a duplicate may still work" — never a plain + "revoked", which is the claim the dialog's docs forbid making about a key + that still answers. Nothing landed and something failed → the error, and + the `502` stands, because there "we could not deactivate it" is true. + + The `502` copy no longer says the key "is still active" either. A `502` is + either a refusal with nothing written or a lost response on a patch that + landed; the page says the deactivation was not *confirmed* and tells the + visitor to reload. + +36. **Every enabled key racing away is `NoKey`, not a dated `Done`.** All + disables answering `NotFound` left no disabled record in the account, so + "deactivated, next eligible on the 1st" was contradicted by the very next + issue press, which finds the name empty and creates a key outright. + +37. **The revocation instant comes off the `UpdateApiKey` response.** + `Gateway::disable` returns `Disable::Applied(Option)` carrying the + patched key's `lastUpdatedDate` — the same byte `cap::decide` compares + against the period on every later read — instead of `bool` plus a local + `SystemTime::now()`. Two clocks, one of them ours, cannot straddle a period + boundary if only one of them is ever consulted. The `Disable` enum follows + `Attachment`'s precedent in the same module: two outcomes, the second not + an error. + +38. **`revocation_instant`'s rustdoc gets its own body back.** An edit had + concatenated `current_key`'s doc block onto it, leaving `current_key` + undocumented and `revocation_instant` opening with a selection rule that is + not its own. + +39. **`rework_round_trip` is deleted.** Left from the abandoned swap design and + unreachable: `Action::parse("rework")` returns `None` (asserted in + `state_token.rs`), so `/auth/login?action=rework` answers `400` and the + helper's `303` assertion could only panic. `round_trip`'s doc now records + why there is no rework variant. + +**Tests:** +4 Rust (`a_partial_revocation_is_reported_as_partial`, +`a_revocation_that_raced_away_is_no_key_not_a_phantom_record`, +`the_revocation_instant_comes_from_the_control_plane_not_our_clock`, +`an_undeletable_revoked_record_does_not_block_the_re_issue`) and +2 portal +(partial warning renders; clean revocation renders none). Two mock knobs added +for them: `fail_disable_of` (the per-id twin of `fail_disables`) and +`disable_stamps_at`. The existing failed-revoke spec was renamed and now +asserts the copy does NOT claim the key is still active. `cargo test -p +prices-api` 0 failed, `clippy --all-targets -D warnings` clean, portal 95/95, +`tsc --noEmit` on `infra` and `web/portal` clean, prettier clean. + +40. **A partial revocation renders neither cap sentence.** Found on a re-read + of the round above, not in the review: the revoked view still rendered + "You can generate a new key from . Until then you do not have a + working key." for a `Partial`, and both halves are false there — the + duplicate that refused to be disabled IS a working key, and the issue path + adopts it rather than refusing (`a_partial_revocation_is_reported_as_partial` + asserts exactly that: `?issue=ok`, nothing minted). The backend still + computes a cap for the answer, because the shape is shared; the page is + what must not repeat it. The warning carries the only instruction that + applies. + +**Not changed:** the five "checked and cleared" items in the review all hold — +`into_service_error` was re-verified against the resolved +`aws-smithy-runtime-api` 1.12.3 source (the non-`ServiceError` arm builds an +unhandled error, it does not panic). diff --git a/lore/1-tasks/active/0193_FEATURE_portal-presentable-ui-pass.md b/lore/1-tasks/active/0193_FEATURE_portal-presentable-ui-pass.md index bd1653c4..59dc4cb1 100644 --- a/lore/1-tasks/active/0193_FEATURE_portal-presentable-ui-pass.md +++ b/lore/1-tasks/active/0193_FEATURE_portal-presentable-ui-pass.md @@ -45,7 +45,7 @@ this point; this slice is about it being legible. The rule that keeps this task honest: **it re-decides no copy.** The wording of the two eligibility refusals is [[0189]]'s, the `delete-key` modal is [[0191]]'s, -the revoke confirmation and its "no replacement is issued" line are [[0192]]'s, +the revoke confirmation and its "no replacement is issued" line are [[0191]]'s (0192 merged into it), the `GetUsage` lag line is [[0188]]'s. If this slice finds one of them wrong, fix it in the owning task rather than quietly here — otherwise the reason behind the wording is lost and the next person edits it back. diff --git a/lore/1-tasks/archive/0157_FEATURE_default-key-limits-1rps-monthly-quota.md b/lore/1-tasks/archive/0157_FEATURE_default-key-limits-1rps-monthly-quota.md index 8cfa7432..06584b4a 100644 --- a/lore/1-tasks/archive/0157_FEATURE_default-key-limits-1rps-monthly-quota.md +++ b/lore/1-tasks/archive/0157_FEATURE_default-key-limits-1rps-monthly-quota.md @@ -365,6 +365,15 @@ conclusion stands on the remaining arguments. measured, treat the single-date property as a **design intent**, and make sure the wording in [[0160]] and [[0163]] presents the rework boundary as our rule rather than as AWS behaviour. + **Correction 2026-08-21 ([[0191]]):** the sentence above that opens this + bullet — "assumed calendar-aligned, resetting on the 1st at 00:00 UTC" — is + now the portal's **stated product rule**, implemented once in + `portal/period.rs` and used by both the usage panel and the rework cap; it is + not, and this task no longer reads it as, an AWS guarantee. [[0180]] #7's + `DAY`-period proxy measurement (the 2026-08-13 run had died after three + samples) was re-run by [[0191]], whose Step 0 table carries the result and + its date. A real `MONTH` rollover cannot be observed before 1 September 2026; + [[0191]] records that as an open limit rather than assuming it. - **The quota binds far harder than the rate.** At 1 req/s a key could produce ~2.6M requests/month; the quota stops it at 100 000. So the operative limit a user meets is the quota, and the per-second throttle is what stops them diff --git a/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/README.md b/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/README.md index 4d3a035d..83b51613 100644 --- a/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/README.md +++ b/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/README.md @@ -106,7 +106,9 @@ history: nothing shipped in three days of a spike that is honestly minutes of work per item. The `item7` poller was stopped mid-run; its scratch REST API (`9utcrbmoc6`), usage plan (`ox7pv0`) and key (`2ke0ixjy7h`) are left - standing on purpose for [[0191]] to reuse. Archived rather than trashed + standing on purpose for [[0191]] to reuse. **Torn down 2026-08-24** — [[0191]] + abandoned the item 7 measurement after a second run died silently, so + there was nothing left to reuse; the account is clean of `lore0180*`. Archived rather than trashed because the raw evidence tables in `notes/` are what ADR 0010 now cites. --- diff --git a/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/measurement/data/item7-poll.tsv b/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/measurement/data/item7-poll.tsv deleted file mode 100644 index 7b9bd49e..00000000 --- a/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/measurement/data/item7-poll.tsv +++ /dev/null @@ -1,8 +0,0 @@ -# task 0180 item 7 -- quota rollover -# api=9utcrbmoc6 plan=ox7pv0 key=2ke0ixjy7h quota=3/DAY region=eu-central-1 -# setup_utc=2026-08-13T08:10:17Z key_created=2026-08-13T10:10:16+02:00 -# poll_interval=60s -- reset instant is resolved to +/- this -utc http used remaining -2026-08-13T08:12:37Z 429 3 0 -2026-08-13T08:13:37Z 429 - - -2026-08-13T08:14:37Z 429 - - diff --git a/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/measurement/item7-quota-rollover.sh b/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/measurement/item7-quota-rollover.sh index 0aa53d4d..4b227479 100755 --- a/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/measurement/item7-quota-rollover.sh +++ b/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/measurement/item7-quota-rollover.sh @@ -30,6 +30,38 @@ # our deploys. Setup makes a handful of writes, spaced. The poll loop makes # data-plane requests and read-only GetUsage calls. # +# STATUS 2026-08-24 -- ABANDONED. Read this before running anything below. +# +# The measurement this exists for was dropped (task 0191, Step 0): two runs +# died silently (2026-08-13, 2026-08-21) and the real MONTH rollover on +# 1 September 2026 is the cheaper, better answer. The scratch stack this +# script created was torn down on 2026-08-24 -- `state.env` is archived as +# `state.env.torn-down-*`, so `setup` is the entry point, not `drain`. +# +# Kept as a working harness. Three defects, all found the hard way: +# +# 1. `teardown` REPORTS SUCCESS WHILE LEAVING THE PLAN ALIVE. It deletes the +# usage plan before the REST API whose stage the plan still references, +# AWS refuses, and `|| true` swallows the refusal. Observed 2026-08-24: +# key and API gone, plan `ox7pv0` still standing. Delete the API first, or +# the plan last, and drop the `|| true` on the deletes. +# 2. THE POLL LOOP DIES SILENTLY. `set -euo pipefail` plus `read_usage`'s +# `aws | awk` pipeline means one failed GetUsage -- an expired SSO token, +# say -- terminates the run with no output at all. There is no exit trap, +# so both deaths were invisible until someone diffed timestamps days +# later, and an archived note claimed "running" over a corpse for a week. +# Needs a trap that records the exit, and a `read_usage` that degrades to +# "?,?" instead of killing the run. +# 3. A 26 h DEFAULT WINDOW OUTLIVES A TYPICAL SSO SESSION, which makes +# hitting defect 2 near-certain partway through. Use credentials that last +# the window (an IAM user scoped to the scratch, or run it on a role), not +# a laptop SSO token. +# +# Also: the log is appended across runs under a single header, and `analyse` +# scans the whole file -- so re-running `poll` without a fresh `drain` can +# report a "reset instant" spanning two runs, days apart, looking plausible. +# Log per run before trusting `analyse` again. +# # Usage: # ./item7-quota-rollover.sh setup # create scratch API + plan + key # ./item7-quota-rollover.sh drain # exhaust the quota diff --git a/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/notes/R-apigw-namequery-quota-and-disable.md b/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/notes/R-apigw-namequery-quota-and-disable.md index 7b6cd334..259195b7 100644 --- a/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/notes/R-apigw-namequery-quota-and-disable.md +++ b/lore/1-tasks/archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/notes/R-apigw-namequery-quota-and-disable.md @@ -160,6 +160,16 @@ stage `test`), plan `ox7pv0` at **3/DAY**, key `2ke0ixjy7h`. Drained to `429` at - Inference for `MONTH` → - Text restated in 0157 / 0158 / 0160 as our rule? → +> **Correction 2026-08-21 (task 0191).** The run "running since 2026-08-13 +> 08:10Z" **did not run**: `data/item7-poll.tsv` holds three samples +> (08:12:37Z–08:14:37Z, all `429`) and `poller.out` ends at the poll start — +> the poller died two minutes in, and this note kept saying "running" for a +> week. Nothing above was measured. The scratch stack (`9utcrbmoc6` / +> `ox7pv0` / `2ke0ixjy7h`) is still standing and task 0191 re-runs the +> procedure unchanged; **the result table lives in 0191's Step 0 from now +> on**, not here, so that a second dead run cannot hide behind a table in an +> archived note. + The worked example ("reworked 3 August → next 1 September") stands either way. Its *justification* does not. diff --git a/lore/1-tasks/backlog/0192_FEATURE_revoke-a-leaked-key.md b/lore/1-tasks/archive/0192_FEATURE_revoke-a-leaked-key.md similarity index 90% rename from lore/1-tasks/backlog/0192_FEATURE_revoke-a-leaked-key.md rename to lore/1-tasks/archive/0192_FEATURE_revoke-a-leaked-key.md index c2a76b04..5711e9f7 100644 --- a/lore/1-tasks/backlog/0192_FEATURE_revoke-a-leaked-key.md +++ b/lore/1-tasks/archive/0192_FEATURE_revoke-a-leaked-key.md @@ -2,7 +2,7 @@ id: "0192" title: "Revoke a leaked key — kills it now, but the once-per-period cap still governs the replacement" type: FEATURE -status: backlog +status: superseded related_adr: ["0010"] related_tasks: ["0183", "0160", "0180", "0187", "0191", "0193"] tags: [layer-backend, priority-medium, effort-small, milestone-M3, epic-self-service-onboarding, api-gateway, security, slice-9] @@ -11,7 +11,7 @@ links: - "../archive/0160_FEATURE_onboarding-backend-endpoints.md" - "../archive/0180_RESEARCH_settle-undocumented-discord-and-aws-behaviours/notes/R-apigw-namequery-quota-and-disable.md" history: - - date: 2026-08-13 + - date: "2026-08-13" status: backlog who: akot note: > @@ -20,7 +20,7 @@ history: usage counters, so revocation cannot become a free quota reset. That was the whole reason the 2026-08-07 meeting deferred it — the deferral is now a scheduling choice, not a correctness one. - - date: 2026-08-13 + - date: "2026-08-13" status: backlog who: akot note: > @@ -29,10 +29,32 @@ history: only issuable on the 1st. Revoke is a kill switch, not a second door to the same room — which makes the honesty of the confirmation screen the main design work in this slice. + - date: "2026-08-21" + status: superseded + by: ["0191"] + who: akot + note: > + Merged into [[0191]] at Adam's request. The 2026-08-21 reversal of 0191 + made "Replace my key" exactly this task's rule — `UpdateApiKey + (enabled=false)` now, nothing issued, re-issue from the next period — + so one feature had two task files. The rule, the three 0180 item 8 + measurements and the open "say the ~25 s window" criterion now live in + 0191; the ClickHouse revocation-record sketch stays here, unbuilt, + because disabling makes the key its own record. Archived as + superseded, not completed: nothing was built under this id. --- # Revoke a leaked key +> **Superseded — merged into [[0191]] on 2026-08-21.** "Replace my key" became +> exactly this task's rule — `UpdateApiKey(enabled=false)` now, nothing issued, +> re-issue from the next period — so there is no separate revoke to build and +> no separate task to track it. The one difference from the text below: +> **disable, not delete**, because the disabled key's `lastUpdatedDate` is the +> revocation record the cap reads, which is why the *Storage* section below was +> never needed. Everything still open from here is an acceptance criterion in +> 0191. The text below is the 2026-08-13 plan, kept for the record. + ## Summary **Story:** *as a developer whose key leaked, I can kill it immediately — knowing diff --git a/lore/1-tasks/backlog/0163_DOCS_quickstart-and-example-queries.md b/lore/1-tasks/backlog/0163_DOCS_quickstart-and-example-queries.md index 000a172a..38939eee 100644 --- a/lore/1-tasks/backlog/0163_DOCS_quickstart-and-example-queries.md +++ b/lore/1-tasks/backlog/0163_DOCS_quickstart-and-example-queries.md @@ -43,7 +43,7 @@ history: hand-over this document waits on is now [[0187]] (a key on screen), not [[0162]]; its hosting is [[0184]] and its Swagger UI [[0195]]. Two additions to step one: the prerequisites are [[0189]]'s membership and - account-age rules, and — until [[0192]] ships — the honest answer to "my + account-age rules, and — until [[0191]] (which absorbed 0192) ships — the honest answer to "my key leaked" is "stop using it and wait for the period to roll over", which belongs in the document rather than in a support conversation. --- diff --git a/lore/1-tasks/backlog/0194_TEST_portal-security-and-ops-audit.md b/lore/1-tasks/backlog/0194_TEST_portal-security-and-ops-audit.md index 9e83eff2..4c5a2bd6 100644 --- a/lore/1-tasks/backlog/0194_TEST_portal-security-and-ops-audit.md +++ b/lore/1-tasks/backlog/0194_TEST_portal-security-and-ops-audit.md @@ -130,5 +130,5 @@ that works is a smaller risk than leaving a finished one closed. - Deliberately a `TEST`, not a `FEATURE`. If this task ends up writing code, the slice that should have written it is the one to change. -- Runs after [[0192]] and before [[0164]]. [[0164]] verifies the user-visible +- Runs after [[0191]] (0192 merged into it) and before [[0164]]. [[0164]] verifies the user-visible flow; this verifies the configuration underneath it. diff --git a/packages/prices-api/README.md b/packages/prices-api/README.md index 34d975a5..3579522d 100644 --- a/packages/prices-api/README.md +++ b/packages/prices-api/README.md @@ -308,12 +308,58 @@ deletes a key. Since task 0189 the `/key` route is read-only too, which is why the page now calls both on load — creating a key is the issue round-trip's job and nobody else's. +### 3c. Replacing a key (task 0191) + +On the page, **Replace my key…** beside the key opens a confirmation; the +confirm button arms only once you type `delete-key`. Pressing it **deactivates +the key and issues nothing** — `POST /api-tokens/api/key/rework`, +session-authenticated, no Discord round-trip. The control plane answers at +once; the **data plane follows within about half a minute** (~25 s measured, +0180 item 8), so treat the value as live until then — which is what the +dialog says, and why neither it nor this section claims "immediately". + +A new key can be issued only from the start of the next quota period (the 1st +of next month, 00:00 UTC, **our** rule): until then "Get my API key" lands on +`?issue=capped` with the date, and the page says so instead of offering the +link. + +```bash +# Before: one enabled key. +aws apigateway get-api-keys --name-query "discord--key" \ + --query 'items[].{id:id,enabled:enabled,updated:lastUpdatedDate}' + +# Confirm on the page, then: +aws apigateway get-api-keys --name-query "discord--key" \ + --query 'items[].{id:id,enabled:enabled,updated:lastUpdatedDate}' +# → the SAME id, enabled: false, lastUpdatedDate just now — the revocation record. + +# The value is refused within tens of seconds (0180 item 8) — the data-plane +# check CI cannot make. 403 is byte-identical to sending no key at all. +curl -sS -o /dev/null -w '%{http_code}\n' -H "X-API-Key: " \ + https:///production/v1/assets # → 403 +``` + +Press **Get my API key** now: the round-trip passes eligibility and lands on +`?issue=capped&next_eligible_at=<1st of next month>` — nothing created, the +disabled key untouched. On or after the 1st the same press deletes the +disabled key, creates a new one and attaches it; the old value stays dead. + +The principal needs the seventh grant, `apigateway:PATCH` on `/apikeys/*`, in +its own statement (`PortalDisableOwnApiKeys`) and scoped by +`aws:ResourceTag/ManagedBy = prices-portal`, so the revoke can only touch keys +this portal created. The `GET`/`DELETE` statement beside it is 0187's and stays +unconditioned — narrowing those two is task 0194's call, because it would stop +the portal adopting a key created by hand in the console. No `UpdateUsagePlan`, +no `UpdateUsage`. + ### 4. Afterwards ```bash aws apigateway delete-api-key --api-key ``` +(A revoke leaves the key in place, disabled; delete it by id like any other.) + ## Env (Lambda) | Var | Purpose | diff --git a/packages/prices-api/src/portal/auth/issue.rs b/packages/prices-api/src/portal/auth/issue.rs index 9a0a3210..02383e73 100644 --- a/packages/prices-api/src/portal/auth/issue.rs +++ b/packages/prices-api/src/portal/auth/issue.rs @@ -20,6 +20,7 @@ //! | `?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, or Discord failing the exchange or the identity read) — refused without accusation | //! | `?issue=failed` | our key service could not produce a key — the control plane refused, or issuance is not wired on this deployment. Never a statement about the visitor | +//! | `?issue=capped&next_eligible_at=YYYY-MM-DD` | eligible, but the visitor revoked their key inside this quota period (task 0191); a new one is issued from the date. Nothing was written | //! //! `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 @@ -97,12 +98,25 @@ pub(super) const ISSUE_FAILED_QUERY: &str = "?issue=failed"; pub(super) const ISSUE_CANCELLED_QUERY: &str = "?issue=cancelled"; pub(super) const ISSUE_DENIED_QUERY: &str = "?issue=denied"; -/// The one parameterised landing state: how long until the account clears the -/// threshold. Digits only, by type. +/// The first 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}") } +/// The second (task 0191): when a revoked key's replacement can be issued. +/// `next_eligible_date` is `keys::cap`'s `YYYY-MM-DD` — digits and dashes by +/// construction from a `NaiveDate`, asserted here because it reaches a +/// `Location` header. +pub(super) fn capped_query(next_eligible_date: &str) -> String { + debug_assert!( + next_eligible_date + .bytes() + .all(|b| b.is_ascii_digit() || b == b'-') + ); + format!("?issue=capped&next_eligible_at={next_eligible_date}") +} + /// Refuse to *start* an issue round-trip on a deployment that cannot finish /// one — no OAuth credentials, no control plane, or no eligibility parameters. /// @@ -188,14 +202,29 @@ const ISSUE_BUDGET: Duration = Duration::from_secs(12); /// The least time worth starting a reconciliation with. /// -/// A reconciliation needs at least a list, a create and an attach. Beginning -/// one with less than this cannot end in a key, and starting control-plane -/// writes that the invocation will be killed part-way through is how an -/// unattached orphan is made. Below this the path lands on `?issue=failed` — -/// which is the honest answer: eligibility passed, our key service did not -/// have time. +/// A reconciliation needs at least a list, and then either an adoption (one +/// more read) or a create and an attach. Beginning one with less than this +/// cannot end in a key at all, and starting control-plane writes that the +/// invocation will be killed part-way through is how an unattached orphan is +/// made. Below this the path lands on `?issue=failed` — which is the honest +/// answer: eligibility passed, our key service did not have time. +/// +/// **Deliberately below [`keys::CREATE_FLOOR`] (4s), not an oversight.** +/// Between the two only an *adoption* can end in a key: `attempt` refuses to +/// start a create with less than `CREATE_FLOOR` left, because a create cut +/// off by the deadline leaves an enabled, unattached key its owner is shown +/// as one that answers `403`. Raising this floor to `CREATE_FLOOR + a list` +/// would save one `GetApiKeys` on a doomed first press and would cost every +/// returning press its recovery — a key already created by a previous +/// invocation is adopted in two reads, well inside 2s. The static assertion +/// below is what fails if the ordering is ever inverted by accident. const RECONCILE_FLOOR: Duration = Duration::from_secs(2); +const _: () = assert!( + RECONCILE_FLOOR.as_millis() <= keys::CREATE_FLOOR.as_millis(), + "RECONCILE_FLOOR above CREATE_FLOOR would refuse adoptions that fit; see the note on RECONCILE_FLOOR" +); + /// Everything the issue arm needs beyond what sign-in already carries. /// /// All optional, like `AuthState::oauth` and `KeysState::gateway`, and for the @@ -409,6 +438,9 @@ pub(super) async fn complete_issue( } land(ISSUE_OK_QUERY) } + IssueOutcome::Capped { next_eligible_date } => { + land(&capped_query(&next_eligible_date)) + } IssueOutcome::Failed => land(ISSUE_FAILED_QUERY), } } @@ -445,6 +477,15 @@ mod tests { /// The one dynamic landing state renders a `u64` and nothing else — no /// request-derived byte can reach the `Location` header through it. + /// The capped landing renders a bare date and nothing else. + #[test] + fn the_capped_query_carries_a_bare_date() { + assert_eq!( + capped_query("2026-09-01"), + "?issue=capped&next_eligible_at=2026-09-01" + ); + } + #[test] fn the_too_young_query_is_digits_only() { assert_eq!(too_young_query(173), "?issue=too_young&wait_secs=173"); diff --git a/packages/prices-api/src/portal/auth/mod.rs b/packages/prices-api/src/portal/auth/mod.rs index 2aff51e9..fb586d92 100644 --- a/packages/prices-api/src/portal/auth/mod.rs +++ b/packages/prices-api/src/portal/auth/mod.rs @@ -194,8 +194,8 @@ pub fn routes(state: AuthState) -> Router { #[derive(Debug, Deserialize)] struct LoginQuery { /// Which action the round-trip authorizes. Absent means sign-in; [0189] - /// sends `issue` and `rework`. An unknown value is a `400` rather than a - /// default — see [`Action::parse`]. + /// sends `issue`. An unknown value is a `400` rather than a default — see + /// [`Action::parse`]. action: Option, } @@ -320,7 +320,8 @@ fn authorize_url( // anyway it does so as an OAuth error on the callback, which // `refuse_oauth_error` logs and lands on `?issue=denied`, rendered rather // than silent. 0180 item 5's consent capture confirms it at no extra - // cost. + // cost. (Task 0191's revoke is session-only and never crosses this + // endpoint, so the issue is still the one re-authorisation.) if action == Action::Issue { query.append_pair("prompt", "none"); } @@ -1044,6 +1045,7 @@ mod tests { issue::ISSUE_UNKNOWN_QUERY, issue::ISSUE_FAILED_QUERY, &issue::too_young_query(173), + &issue::capped_query("2026-09-01"), ] { assert!(format!("{PORTAL_HOME}{query}").starts_with("/api-tokens/?")); } diff --git a/packages/prices-api/src/portal/auth/state_token.rs b/packages/prices-api/src/portal/auth/state_token.rs index ae46d632..af88dba4 100644 --- a/packages/prices-api/src/portal/auth/state_token.rs +++ b/packages/prices-api/src/portal/auth/state_token.rs @@ -116,8 +116,8 @@ impl Action { /// Parse the `action` query parameter of `/auth/login`. /// /// 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 + /// mean a client asking for an action this build does not know (0192's + /// `revoke`, 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 { @@ -450,12 +450,12 @@ mod tests { assert_eq!(state.action, Action::SignIn); assert_eq!(pending.action, Action::SignIn); - // And a state naming an action this build does not know — 0191's - // `rework`, arriving early — is refused at deserialization. + // And a state naming an action this build does not know — 0192's + // `revoke`, arriving early — is refused at deserialization. let crossed = sign_claims( KEY, CTX_STATE, - &serde_json::json!({ "action": "rework", "nonce": pending.nonce, "exp": state.exp }), + &serde_json::json!({ "action": "revoke", "nonce": pending.nonce, "exp": state.exp }), ); assert_eq!( accept(KEY, &crossed, Some(&started.pending_cookie), NOW).unwrap_err(), @@ -515,7 +515,11 @@ mod tests { fn an_unknown_action_query_value_is_rejected_rather_than_defaulted() { assert_eq!(Action::parse("signin"), Some(Action::SignIn)); assert_eq!(Action::parse("issue"), Some(Action::Issue)); - for unknown in ["rework", "", "SIGNIN", "ISSUE", "issue ", "signin "] { + // `rework` is deliberately NOT an action: task 0191's revoke is + // session-only and never crosses the authorize endpoint. + for unknown in [ + "rework", "revoke", "", "SIGNIN", "ISSUE", "issue ", "signin ", + ] { assert_eq!(Action::parse(unknown), None, "accepted {unknown:?}"); } } diff --git a/packages/prices-api/src/portal/eligibility.rs b/packages/prices-api/src/portal/eligibility.rs index 19a97938..a0966e45 100644 --- a/packages/prices-api/src/portal/eligibility.rs +++ b/packages/prices-api/src/portal/eligibility.rs @@ -12,18 +12,18 @@ //! | 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 | +//! | Replace / revoke ([0191]) | no | session only — a **deliberate exception**: a user must be able to kill a leaked key while Discord is down, and the action issues nothing. The replacement is an ordinary issue in the next quota period | //! //! 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. +//! is old enough forever. //! //! # 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. +//! right to be issued a *replacement* after a revoke ([0191]) — that is an +//! issue, and issue re-proves membership. //! 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. //! @@ -236,24 +236,10 @@ pub fn decide( 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; - } - }, + match membership(member) { + Membership::Member => {} + Membership::NotMember => return Eligibility::NotMember, + Membership::Unknown => return Eligibility::Unknown, } let Some(created_ms) = account_created_ms(snowflake) else { @@ -270,6 +256,47 @@ pub fn decide( } } +/// The membership half of the verdict, on its own. +/// +/// What a **rework** (task 0191) re-proves: the per-action table at the top +/// of this module says membership only, never age, because an account old +/// enough once is old enough forever. [`decide`] is this plus the age check, +/// so the two paths cannot disagree about what a member is — the `pending` +/// rules below are the single statement of it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Membership { + /// `pending == Some(false)` — a full member, admin bypass included. + Member, + /// Confirmed non-membership, or `pending == Some(true)`. + NotMember, + /// Could not verify, including an absent `pending`. Refuse, never accuse. + Unknown, +} + +/// Classify one membership answer. See [`decide`] for the `pending` rules, +/// which live in this function and are documented there. +pub fn membership(member: &MemberLookup) -> Membership { + match member { + MemberLookup::NotMember { .. } => Membership::NotMember, + MemberLookup::Unknown { status, detail } => { + tracing::warn!(?status, detail, "membership could not be verified"); + Membership::Unknown + } + MemberLookup::Member(m) => match m.pending { + Some(false) => Membership::Member, + Some(true) => Membership::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" + ); + Membership::Unknown + } + }, + } +} + /// 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 { @@ -456,6 +483,37 @@ mod tests { assert_eq!(settings.guild_id().await.unwrap(), "897514728459468821"); } + /// The rework path reads [`membership`] alone; it must agree with + /// [`decide`] on every membership shape, or a rework could be allowed to + /// someone an issue would refuse (or the reverse). + #[test] + fn the_membership_half_agrees_with_the_full_verdict() { + let now = DOCUMENTED_CREATED_MS + 6 * 60_000; + let cases = [ + (member(Some(false)), Membership::Member), + (member(Some(true)), Membership::NotMember), + (member(None), Membership::Unknown), + ( + MemberLookup::NotMember { code: 10_007 }, + Membership::NotMember, + ), + ( + MemberLookup::NotMember { code: 10_004 }, + Membership::NotMember, + ), + (unknown(), Membership::Unknown), + ]; + for (lookup, expected) in cases { + assert_eq!(membership(&lookup), expected, "{lookup:?}"); + let full = decide(&lookup, DOCUMENTED_SNOWFLAKE, 5, now); + match expected { + Membership::Member => assert_eq!(full, Eligibility::Eligible), + Membership::NotMember => assert_eq!(full, Eligibility::NotMember), + Membership::Unknown => assert_eq!(full, Eligibility::Unknown), + } + } + } + #[test] fn an_unparseable_snowflake_is_unknown_not_too_young() { assert_eq!( diff --git a/packages/prices-api/src/portal/keys/cap.rs b/packages/prices-api/src/portal/keys/cap.rs new file mode 100644 index 00000000..cae08529 --- /dev/null +++ b/packages/prices-api/src/portal/keys/cap.rs @@ -0,0 +1,184 @@ +//! The re-issue cap: a revoked key is replaced only in the next quota period +//! (task 0191). +//! +//! **The rule.** "Replace my key" deactivates the current key **immediately** +//! and issues nothing. A new key can be issued only once the quota period in +//! which the revocation happened has rolled — the 1st of the next calendar +//! month, 00:00 UTC, under [`Period`]'s rule. A leak on the 3rd is dead on the +//! 3rd; the replacement comes on the 1st. +//! +//! **Why the wait is the point, not a cost to engineer away.** Quota is scoped +//! to `(usagePlanId, apiKeyId)`, so a new key is a clean counter. If a revoke +//! handed out a replacement, "replace my key" would be the button people press +//! on the 20th of a heavy month and the monthly quota would be decorative. The +//! wait is the same cost the owner would pay by simply not using the leaked +//! key, minus the risk of somebody else using it. +//! +//! **What is compared.** The revoked key stays in the account, disabled; its +//! `lastUpdatedDate` is the revocation instant, and a re-issue is allowed only +//! when that instant falls strictly **before** the current period began. There +//! is no stored timestamp because API Gateway already keeps this one — which +//! is why task 0190's registry stayed cancelled. +//! +//! **Worked example:** revoked on 3 August → "Get my API key" is refused until +//! 1 September 00:00 UTC, naming the date, and allowed from that instant. +//! +//! Pure, so the whole boundary table is decidable by a test with dates typed +//! by hand. No clock is read here: the caller supplies the period. + +use super::super::period::Period; + +/// Whether a key may be re-issued now. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Cap { + /// The revocation predates the current period: a new key may be issued. + Allowed, + /// Revoked inside the current period (or undatable — see [`decide`]). + /// Refused until the period rolls. + Capped { + /// The instant a new key becomes available, RFC 3339 — the + /// `next_eligible_at` in the envelopes and the revoke answer. + next_eligible_at: String, + /// The same instant as a bare `YYYY-MM-DD`, for the callback's landing + /// query: digits and dashes only, so no request-derived byte can reach + /// a `Location` header through it. + next_eligible_date: String, + }, +} + +/// Decide the cap for a key revoked at `revoked_at` (Unix seconds — the +/// disabled key's `lastUpdatedDate`) against `period`. +/// +/// Strictly **before** the period start: a revocation at exactly 00:00:00 on +/// the 1st happened inside the period that begins then, and the replacement +/// waits for the next one — the boundary instant belongs to the period it +/// begins. +/// +/// `None` — a key AWS did not date — is **capped**, not allowed. The service +/// always sends `lastUpdatedDate`, so this is a shape that should never occur; +/// when it does, the cap cannot prove the revocation predates the period, and +/// the failure that matters is a quota laundered through a revoke-and-reissue, +/// not a replacement delayed to the 1st. Logged, because a deployment where it +/// fires has a control plane answering in a shape nobody has seen. +pub fn decide(revoked_at: Option, period: &Period) -> Cap { + let capped = || Cap::Capped { + next_eligible_at: period.resets_at(), + next_eligible_date: period.next_start_ymd(), + }; + match revoked_at { + Some(revoked_at) if revoked_at < period.start_secs() => Cap::Allowed, + Some(_) => capped(), + None => { + tracing::warn!( + "the revoked key carries no lastUpdatedDate; refusing the re-issue rather than \ + assuming the revocation predates the period" + ); + capped() + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono::NaiveDate; + + fn period(y: i32, m: u32, d: u32) -> Period { + Period::containing(NaiveDate::from_ymd_opt(y, m, d).unwrap()) + } + + /// Unix seconds for a UTC date-time, so the table below reads as dates. + fn at(y: i32, m: u32, d: u32, h: u32, min: u32, s: u32) -> u64 { + NaiveDate::from_ymd_opt(y, m, d) + .unwrap() + .and_hms_opt(h, min, s) + .unwrap() + .and_utc() + .timestamp() as u64 + } + + /// The worked example, both halves: revoked on 3 August, refused for the + /// rest of August naming 1 September — and allowed from the first second + /// of 1 September. + #[test] + fn revoked_on_3_august_refuses_until_1_september_and_succeeds_on_it() { + let revoked = at(2026, 8, 3, 14, 30, 0); + + for day in [3, 4, 20, 31] { + assert_eq!( + decide(Some(revoked), &period(2026, 8, day)), + Cap::Capped { + next_eligible_at: "2026-09-01T00:00:00Z".into(), + next_eligible_date: "2026-09-01".into(), + }, + "August {day}" + ); + } + + assert_eq!(decide(Some(revoked), &period(2026, 9, 1)), Cap::Allowed); + assert_eq!(decide(Some(revoked), &period(2026, 9, 30)), Cap::Allowed); + } + + /// A key issued AND revoked on the same day is capped like any other: the + /// cap is about the revocation instant, so a fresh key revoked at once + /// cannot be turned into a fresh counter. + #[test] + fn a_key_revoked_minutes_after_issue_is_capped_too() { + assert!(matches!( + decide(Some(at(2026, 8, 21, 12, 0, 0)), &period(2026, 8, 21)), + Cap::Capped { .. } + )); + } + + /// The boundary instant belongs to the period it begins. + #[test] + fn the_boundary_instant_is_inside_the_new_period() { + let midnight = at(2026, 8, 1, 0, 0, 0); + assert!(matches!( + decide(Some(midnight), &period(2026, 8, 15)), + Cap::Capped { .. } + )); + assert_eq!( + decide(Some(midnight - 1), &period(2026, 8, 15)), + Cap::Allowed + ); + } + + /// December names January of the following year. + #[test] + fn a_december_refusal_names_the_new_year() { + assert_eq!( + decide(Some(at(2026, 12, 10, 0, 0, 0)), &period(2026, 12, 24)), + Cap::Capped { + next_eligible_at: "2027-01-01T00:00:00Z".into(), + next_eligible_date: "2027-01-01".into(), + } + ); + } + + /// A revocation AWS did not date cannot be shown to predate the period. + #[test] + fn an_undated_revocation_is_capped_not_waved_through() { + assert!(matches!( + decide(None, &period(2026, 8, 15)), + Cap::Capped { .. } + )); + } + + /// The landing-query form is digits and dashes only, by construction. + #[test] + fn the_date_form_is_url_safe_by_construction() { + let Cap::Capped { + next_eligible_date, .. + } = decide(Some(u64::MAX), &period(2026, 8, 15)) + else { + panic!("a future revocation is capped"); + }; + assert!( + next_eligible_date + .bytes() + .all(|b| b.is_ascii_digit() || b == b'-') + ); + assert_eq!(next_eligible_date.len(), 10); + } +} diff --git a/packages/prices-api/src/portal/keys/gateway.rs b/packages/prices-api/src/portal/keys/gateway.rs index 8cb74653..19feef87 100644 --- a/packages/prices-api/src/portal/keys/gateway.rs +++ b/packages/prices-api/src/portal/keys/gateway.rs @@ -1,8 +1,9 @@ -//! The API Gateway **control plane**, wrapped down to six calls (task 0187, -//! plus task 0188's `GetUsage`). +//! The API Gateway **control plane**, wrapped down to seven calls (task 0187, +//! task 0188's `GetUsage`, task 0191's `UpdateApiKey`). //! //! Not the data plane. These are `GetApiKeys`, `CreateApiKey`, -//! `CreateUsagePlanKey`, `GetApiKey`, `DeleteApiKey` and `GetUsage` — the API +//! `CreateUsagePlanKey`, `GetApiKey`, `DeleteApiKey`, `UpdateApiKey` +//! (disable only) and `GetUsage` — the API //! the console drives — and they are the reason this slice needs no database: //! **API Gateway is the source of truth for whether a key exists** (task 0158's //! own argument, restated in 0187's context), and for how much it has been @@ -131,6 +132,20 @@ pub enum Attachment { KeyGone, } +/// What [`Gateway::disable`] observed (task 0191). +/// +/// Two outcomes rather than `()` for the same reason as [`Attachment`]: the +/// second one is not an error and the caller has copy for it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Disable { + /// The key is off. Carries its `lastUpdatedDate` — the revocation instant + /// the cap will read — or `None` for the shape AWS does not produce, which + /// the cap treats as capped. + Applied(Option), + /// The key no longer exists, so there was nothing to disable. + KeyGone, +} + /// One key's consumption over a queried period, as AWS reports it. /// /// Derived from `GetUsage`'s daily `[used, remaining]` pairs rather than read @@ -230,6 +245,30 @@ impl Gateway { pub async fn from_ambient_config(free_plan_id: String) -> Self { let shared = aws_config::load_defaults(aws_sdk_apigateway::config::BehaviorVersion::latest()).await; + + // Resolve credentials ONCE, here, instead of lazily on the first call. + // + // The default chain initialises its provider on first use, and two + // requests racing that first use — which is exactly what a dashboard + // load does, `/key` and `/usage` in parallel — can leave the loser + // with `profile file credentials provider initialization error + // already taken` (aws-config's SSO/profile provider; observed on a + // local `serve` run, 2026-08-21). In the Lambda the chain resolves + // from the environment and this is a no-op that costs nothing; it is + // the local run, on an SSO profile, that needs the first resolution + // serialised. A failure here is logged and NOT fatal: the per-call + // error is still the one the handlers report. + if let Some(provider) = shared.credentials_provider() { + use aws_sdk_apigateway::config::ProvideCredentials as _; + if let Err(error) = provider.provide_credentials().await { + tracing::warn!( + error = %error, + "could not resolve AWS credentials at cold start; every control-plane \ + call will retry the resolution" + ); + } + } + let config = aws_sdk_apigateway::config::Builder::from(&shared) .timeout_config(timeouts()) .build(); @@ -338,6 +377,11 @@ impl Gateway { .created_date() .map(|d| d.secs()) .and_then(|secs| u64::try_from(secs).ok()), + enabled: item.enabled(), + last_updated_at: item + .last_updated_date() + .map(|d| d.secs()) + .and_then(|secs| u64::try_from(secs).ok()), }); } @@ -360,6 +404,14 @@ impl Gateway { /// a separate `TagResource` could fail and leave an untagged key, which is /// the one thing tagging exists to prevent. pub async fn create(&self, name: &str) -> Result<(KeyRecord, KeyValue), GatewayError> { + // **No SDK retries on a create.** `CreateApiKey` has no idempotency + // token, so a retried attempt whose first try actually landed makes a + // second key — under the user's exact name, enabled, attached to + // nothing. The SDK's standard mode retries transport errors and 5xx + // up to three times inside `OPERATION_TIMEOUT`, and `ATTEMPT_TIMEOUT` + // (2s) is short enough for a cold control plane to trip it. The + // reconciler sweeps duplicates, but only when the next listing is + // consistent; better not to make them. Reads keep their retries. let created = self .client .create_api_key() @@ -368,6 +420,11 @@ impl Gateway { .enabled(true) .tags(TAG_MANAGED_BY.0, TAG_MANAGED_BY.1) .tags(TAG_ISSUED_BY.0, TAG_ISSUED_BY.1) + .customize() + .config_override( + aws_sdk_apigateway::config::Builder::new() + .retry_config(aws_sdk_apigateway::config::retry::RetryConfig::disabled()), + ) .send() .await .map_err(|e| GatewayError::Call { @@ -395,11 +452,73 @@ impl Gateway { .created_date() .map(|d| d.secs()) .and_then(|secs| u64::try_from(secs).ok()), + enabled: created.enabled(), + last_updated_at: created + .last_updated_date() + .map(|d| d.secs()) + .and_then(|secs| u64::try_from(secs).ok()), }, KeyValue(value.to_string()), )) } + /// Disable a key — `UpdateApiKey` with `replace /enabled false` — the + /// revocation (task 0191). + /// + /// **Disable, not delete.** Both take ~25 s to reach the data plane (0180 + /// item 8, measured), so deleting buys no speed; what disabling buys is + /// the *record*: the key stays in the account with its `lastUpdatedDate` + /// as the revocation instant, and that is the only thing — with no + /// registry — that can refuse a re-issue inside the same quota period. + /// The counter is preserved across disable (same measurement), so the + /// dashboard keeps reporting the revoked key's usage honestly. + /// + /// [`Disable::KeyGone`] is the deletion race — the key was listed and is + /// gone — and is a state the caller has an answer for (nothing to revoke), + /// not an error. + /// + /// [`Disable::Applied`] carries the patched key's **`lastUpdatedDate`**, + /// read off this call's own response rather than re-listed or invented. + /// That is the byte the re-issue cap is decided against on every later read + /// (`cap::decide`), so returning it here is what stops the revoke answer + /// and the issue path disagreeing: a local `SystemTime::now()` is a + /// different clock from the control plane's, and two clocks straddling + /// 00:00 UTC on the 1st fall in different quota periods — the page would + /// promise a replacement a month before the round-trip would honour it. + pub async fn disable(&self, key_id: &str) -> Result { + let patch = aws_sdk_apigateway::types::PatchOperation::builder() + .op(aws_sdk_apigateway::types::Op::Replace) + .path("/enabled") + .value("false") + .build(); + match self + .client + .update_api_key() + .api_key(key_id) + .patch_operations(patch) + .send() + .await + { + Ok(updated) => Ok(Disable::Applied( + updated + .last_updated_date() + .map(|d| d.secs()) + .and_then(|secs| u64::try_from(secs).ok()), + )), + Err(e) => { + let message = sdk_message(&e); + if e.into_service_error().is_not_found_exception() { + Ok(Disable::KeyGone) + } else { + Err(GatewayError::Call { + operation: "UpdateApiKey", + message, + }) + } + } + } + } + /// Attach a key to the free usage plan, which is what makes it work against /// `/v1/`. A key that exists but is on no plan authenticates and is then /// refused by the plan check — the confusing half-state this call closes. diff --git a/packages/prices-api/src/portal/keys/mod.rs b/packages/prices-api/src/portal/keys/mod.rs index c727fc00..500d11aa 100644 --- a/packages/prices-api/src/portal/keys/mod.rs +++ b/packages/prices-api/src/portal/keys/mod.rs @@ -7,13 +7,31 @@ //! //! | route | does | //! | --- | --- | -//! | `GET`/`POST /api-tokens/api/key` | reveal — the lookup, plus the value. **Never creates.** | +//! | `GET`/`POST /api-tokens/api/key` | reveal — the lookup, plus the value. **Never creates.** A revoked key answers `404 key_revoked` with the date a new one can be issued | +//! | `POST /api-tokens/api/key/rework` | **revoke** (task 0191): deactivate the caller's key now, issue nothing. Session-authorized, deliberately — see below | //! //! 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. +//! fresh Discord token can provide, and a session cookie is not one. Since +//! task 0191 the issue path also enforces the **re-issue cap**: a key the +//! owner revoked inside the current quota period blocks a new one until the +//! period rolls ([`cap`]), which is what makes "replace my key" a revocation +//! rather than a free counter reset. +//! +//! # Revoke is session-only, on purpose +//! +//! The one write a session cookie can cause is `UpdateApiKey(enabled=false)` +//! on the caller's **own** key — destructive to their own access and to +//! nothing else. It is deliberately not behind a Discord round-trip: a leaked +//! key has to be killable while Discord is down, and re-proving membership +//! buys nothing for an action that issues no credential. The CSRF stance is +//! re-derived as `auth/mod.rs` requires: the route is `POST`-only and the +//! session cookie is `SameSite=Lax`, so a cross-site page cannot make the +//! browser send it with a `POST`; a `GET` on the path is a `405`. The worst +//! outcome of the write is the visitor's own key dying a month early, which +//! is exactly what the confirmation dialog made them type `delete-key` for. //! //! # There is no database, and that is the design //! @@ -58,7 +76,8 @@ //! 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]). +//! what a departed member loses is the replacement after a revoke ([0191]), +//! which is an issue and re-proves membership. //! //! # Never log a key value //! @@ -71,6 +90,7 @@ //! tracing subscriber's `info` level records the route and the outcome, never //! the credential. +pub mod cap; pub mod gateway; pub mod naming; @@ -82,18 +102,28 @@ use axum::Router; use axum::extract::State; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; -use axum::routing::get; +use axum::routing::{get, post}; use serde::Serialize; use crate::common::{cache_control, errors}; use super::auth::secret::OauthSecret; -use gateway::{Attachment, Gateway, GatewayError, KeyValue}; -use naming::{KeyRecord, choose_winner, exact_matches, key_name, losers}; +use super::period::Period; +use cap::Cap; +use gateway::{Attachment, Disable, Gateway, GatewayError, KeyValue}; +use naming::{ + KeyRecord, choose_winner, current_key, exact_matches, key_name, losers, revocation_instant, +}; /// The reveal, on both verbs — see [`key`] for why `POST` answers identically. pub const KEY_PATH: &str = "/api-tokens/api/key"; +/// The revocation (task 0191). `POST` only — the verb is the CSRF guard, see +/// the module docs — and the path keeps the task's name for the action the +/// dashboard calls "Replace my key": the key is replaced, just not until the +/// next period. +pub const REWORK_PATH: &str = "/api-tokens/api/key/rework"; + /// Error code for a caller with no valid session. const NOT_SIGNED_IN: &str = "not_signed_in"; /// Error code for a control plane that would not answer. @@ -105,6 +135,30 @@ const KEYS_UNCONFIGURED: &str = "keys_unconfigured"; /// because the frontend reads both: a `404` is "no key" only when this /// envelope says so. const NO_KEY: &str = "no_key"; +/// Error code for a caller whose key is revoked and whose replacement is not +/// due yet (task 0191). A `404` like `no_key` — there is no usable key — but +/// a distinct code, because the page must not offer the issue round-trip: the +/// envelope's `details` carries `next_eligible_at` (RFC 3339) and +/// `revoked_at`, the slot `ErrorEnvelope` has for exactly this kind of +/// structured context. +const KEY_REVOKED: &str = "key_revoked"; +/// Error code for a revoke that did not come from the portal page (task +/// 0191). See [`is_same_origin_write`]. +const CROSS_SITE_REQUEST: &str = "cross_site_request"; + +/// The header the portal's own `fetch` sets on the revoke, and its value. +/// +/// A custom header makes the request *non-simple*: a cross-origin page can +/// only send it after a CORS preflight, and this API answers no preflight — +/// so a browser never delivers such a request from anywhere but the portal's +/// origin. This is the classic CSRF defence that does not depend on what the +/// cookie's `SameSite` considers "the same site", which matters because +/// `SameSite=Lax` is **site**-scoped: today `*.cloudfront.net` makes every +/// distribution its own site, but after the custom-domain cutover ([0195]) +/// any sibling host under the registrable domain becomes same-site, and a +/// form `POST` from it would carry the cookie. One request from such a host +/// would then cost the victim their key for the rest of the month. +pub const PORTAL_REQUEST_HEADER: (&str, &str) = ("x-requested-with", "stellar-prices-portal"); /// How many times the whole flow is re-run when the key it settled on turns out /// to have been deleted underneath it. @@ -199,6 +253,7 @@ impl KeysState { pub fn routes(state: KeysState) -> Router { Router::new() .route(KEY_PATH, get(key).post(key)) + .route(REWORK_PATH, post(revoke)) .with_state(state) } @@ -289,7 +344,39 @@ async fn reveal(state: &KeysState, headers: &HeaderMap) -> Response { }; match looked_up { - Ok(Some((record, value))) => { + Ok(Lookup::Revoked { revoked_at }) => { + // The owner killed this key. There is nothing to reveal — the + // value is a leaked credential by the owner's own statement — and + // nothing to issue until the period rolls; the page renders the + // date rather than the "get my API key" link. + let Cap::Capped { + next_eligible_at, .. + } = cap::decide(revoked_at, &Period::now()) + else { + // Revoked in an earlier period: a new key is due, and the + // issue round-trip will delete this one and create it. Until + // that press the honest answer is still "no usable key". + return no_store(no_key_response()); + }; + no_store( + ( + StatusCode::NOT_FOUND, + Json(errors::ErrorEnvelope { + code: KEY_REVOKED, + message: format!( + "your API key was revoked; a new one can be issued from \ + {next_eligible_at}" + ), + details: Some(serde_json::json!({ + "next_eligible_at": next_eligible_at, + "revoked_at": revoked_at.map(rfc3339), + })), + }), + ) + .into_response(), + ) + } + Ok(Lookup::Key(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. An in-process eviction, not a @@ -314,20 +401,225 @@ async fn reveal(state: &KeysState, headers: &HeaderMap) -> Response { // 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( + Ok(Lookup::None) => no_store(no_key_response()), + Err(error) => { + // `error` cannot carry a key value — see `gateway::sdk_message`. + tracing::error!(error = %error, "portal key reveal failed"); + no_store( + ( + StatusCode::BAD_GATEWAY, + Json(errors::ErrorEnvelope { + code: KEY_UNAVAILABLE, + message: "could not reach the API key service; try again".into(), + details: None, + }), + ) + .into_response(), + ) + } + } +} + +/// Whether a state-changing request came from the portal's own page. +/// +/// Two independent signals, both required to agree: +/// +/// 1. [`PORTAL_REQUEST_HEADER`] is present with its value — the non-simple +/// request marker a cross-origin page cannot produce without a preflight. +/// 2. `Sec-Fetch-Site`, when the browser sends it (every current browser +/// does), is `same-origin`. `none` (a typed URL, a bookmark) is accepted +/// too — it cannot be a `POST` from another page. A request carrying +/// `cross-site` or `same-site` is refused even with the header: the +/// header guards against a preflight-less browser, the fetch metadata +/// guards against a misconfigured CORS answer ever appearing in front of +/// this API. +/// +/// Absence of `Sec-Fetch-Site` alone does not refuse — `curl` and older +/// browsers omit it — which is why the header is the primary check. +fn is_same_origin_write(headers: &HeaderMap) -> bool { + let marker = headers + .get(PORTAL_REQUEST_HEADER.0) + .and_then(|v| v.to_str().ok()) + .is_some_and(|v| v.eq_ignore_ascii_case(PORTAL_REQUEST_HEADER.1)); + let fetch_site_ok = match headers.get("sec-fetch-site").and_then(|v| v.to_str().ok()) { + None => true, + Some(site) => site.eq_ignore_ascii_case("same-origin") || site.eq_ignore_ascii_case("none"), + }; + marker && fetch_site_ok +} + +/// `404 no_key`, shared by the reveal's two keyless answers. +fn no_key_response() -> Response { + ( + 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() +} + +/// Unix seconds → RFC 3339, for the envelopes. +fn rfc3339(secs: u64) -> String { + chrono::DateTime::::from_timestamp(secs as i64, 0) + .map(|t| t.to_rfc3339_opts(chrono::SecondsFormat::Secs, true)) + .unwrap_or_default() +} + +/// What the revoke answers. +#[derive(Serialize)] +struct RevokeResponse { + /// Always `true` on a `200`; a failure is a status, never a `false`. + revoked: bool, + /// When a new key can be issued under our period rule, RFC 3339 — + /// **decided by [`cap::decide`]**, not by the period alone, so this route + /// is not a reader of the cap that disagrees with the other four + /// (decision #22). For a revocation whose period has already rolled it is + /// now: an idempotent revoke must not tell a stale tab to wait another + /// month for a key the issue round-trip would hand over today. + next_eligible_at: String, + /// When the key was revoked, RFC 3339 — now, or earlier if it already + /// was. `null` when no record under the name carries a date: a made-up + /// epoch instant would render as "deactivated on 1 January 1970". + revoked_at: Option, + /// `true` when at least one key under the name could **not** be disabled + /// while another was (task 0191's partial revocation). The page must not + /// render a plain "revoked" for it: a duplicate that still answers on + /// `/v1/` is exactly the state the dialog's docs forbid calling revoked. + /// Omitted from the JSON in the ordinary case, so the common answer keeps + /// the shape it had. + #[serde(skip_serializing_if = "std::ops::Not::not")] + partial: bool, +} + +/// `POST /key/rework` — revoke the caller's key now, issue nothing +/// (task 0191). +/// +/// **Deactivates, immediately, and that is the whole operation.** Every +/// enabled key under the caller's name is disabled (`UpdateApiKey`); the +/// answer carries the date a replacement can be issued — the 1st of the next +/// month under our period rule — and the page renders it. Nothing is created: +/// the cap on re-issue is what makes this a revocation rather than a free +/// counter reset, and the wait is the visitor's own access, which the +/// confirmation dialog made them type `delete-key` to accept. +/// +/// Idempotent: a key already revoked answers `200` again with the same dates, +/// so a retry after a dropped response is safe. A caller with no key is a +/// `404 no_key`. Session-authorized — see the module docs for why that is +/// deliberate and how the `POST`-only shape stands in for a CSRF token. +/// +/// | answer | meaning | +/// | --- | --- | +/// | `200 {revoked: true, next_eligible_at, revoked_at}` | the key is off (the data plane follows within tens of seconds — 0180 item 8) | +/// | `404 no_key` | nothing to revoke | +/// | `401` / `502` / `503` | as the reveal | +async fn revoke(State(state): State, headers: HeaderMap) -> Response { + let Some(oauth) = state.oauth.as_ref() else { + return unconfigured(); + }; + let Some(gateway) = state.gateway.as_ref() else { + return unconfigured(); + }; + // Before the session is even read: the one write a session can cause + // must come from the portal's own page. See `is_same_origin_write`. + if !is_same_origin_write(&headers) { + tracing::warn!("a revoke arrived without the same-origin markers; refusing"); + return no_store( + ( + StatusCode::FORBIDDEN, + Json(errors::ErrorEnvelope { + code: CROSS_SITE_REQUEST, + message: "this action can only be taken from the portal page".into(), + details: None, + }), + ) + .into_response(), + ); + } + let Some(session) = super::auth::current_session(oauth, &headers) else { + return no_store(errors::unauthorized_with( + NOT_SIGNED_IN, + "sign in with Discord before asking to replace your API key", + )); + }; + // Same guard as the reveal — and here it stands in front of a write. + let Some(name) = key_name(&session.sub) else { + 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 asking to replace your API key", + )); + }; + + let revoked = match tokio::time::timeout(state.deadline, disable_all(gateway, &name)).await { + Ok(revoked) => revoked, + Err(_elapsed) => { + tracing::error!( + deadline_secs = state.deadline.as_secs_f32(), + "portal key revocation ran out of time" + ); + return no_store(errors::service_unavailable( + KEY_UNAVAILABLE, + "the API key service is taking too long to answer; try again", + )); + } + }; + + // Read before the match consumes `revoked`; both arms below answer `200` + // with the same dates and differ only in this flag. + let partial = matches!(&revoked, Ok(Revocation::Partial { .. })); + match revoked { + Ok(Revocation::Done { at }) | Ok(Revocation::Partial { at }) => { + if partial { + tracing::error!( + "portal revoked an API key only in part; a duplicate may still answer" + ); + } else { + tracing::info!("portal revoked an API key"); + } + // The cached usage describes a key that is now off. Its counter is + // preserved (0180 item 8), so the numbers would still be right — + // but the page's state changed underneath them, and a fresh read + // is one call. + if let Some(cache) = &state.usage_cache { + cache.invalidate(&session.sub); + } + // The same decision function the reveal, the usage route and the + // issue path read, from the same instant — never `Period::now()` + // straight, which would answer "next month" for a revocation that + // happened two periods ago and hide the issue link the round-trip + // would have honoured. + let next_eligible_at = match cap::decide(at, &Period::now()) { + Cap::Capped { + next_eligible_at, .. + } => next_eligible_at, + Cap::Allowed => rfc3339(now_secs()), + }; + no_store( + Json(RevokeResponse { + revoked: true, + next_eligible_at, + revoked_at: at.map(rfc3339), + partial, + }) + .into_response(), + ) + } + Ok(Revocation::NoKey) => no_store( ( StatusCode::NOT_FOUND, Json(errors::ErrorEnvelope { code: NO_KEY, - message: "you have no API key yet; issue one from the portal".into(), + message: "you have no API key to replace; issue one first".into(), details: None, }), ) .into_response(), ), Err(error) => { - // `error` cannot carry a key value — see `gateway::sdk_message`. - tracing::error!(error = %error, "portal key reveal failed"); + tracing::error!(error = %error, "portal key revocation failed"); no_store( ( StatusCode::BAD_GATEWAY, @@ -343,34 +635,184 @@ async fn reveal(state: &KeysState, headers: &HeaderMap) -> Response { } } +/// Disable every key under `name`. [`Revocation::NoKey`] means there was +/// none left to disable; [`Revocation::Done`] and [`Revocation::Partial`] +/// carry the revocation instant — the control plane's own `lastUpdatedDate` +/// for a key this call disabled, the recorded one for a key that already was, +/// and `None` for the shape where no record under the name is dated. +/// +/// **Every** enabled key, not only the current one: a duplicate left by an +/// earlier double-submit is a working credential the visitor was just told +/// had died. +/// +/// **A failure mid-loop is logged and stepped over, not propagated** — the +/// reverse of what this function did when it shipped, and the reason is the +/// sentence at the other end of it. Propagating the first error answered +/// `502`, and the dialog's `502` copy said the key "is still active" while the +/// keys disabled before the error were off. There is no rollback for a +/// revocation (re-enabling a leaked key is the wrong direction to fail in), so +/// the state after a partial failure is real and has to be *reported* rather +/// than denied: +/// +/// - at least one disable applied, none failed → [`Revocation::Done`]; +/// - at least one applied **and** at least one failed → [`Revocation::Partial`], +/// which the page renders as "your key is off, a duplicate may still work" — +/// never as a plain "revoked", because that is the claim `app.tsx`'s dialog +/// docs forbid making about a key that still answers; +/// - nothing applied and something failed → the error, and the `502` stands: +/// here "still active" is the truth, and no write of ours landed. +/// +/// A key that answers `NotFound` is not a failure but the deletion race, and +/// counts as neither: if every enabled key raced away there is nothing to +/// revoke and no record to date, which is [`Revocation::NoKey`] — the same +/// answer the very next `?action=issue` press will act on when it finds the +/// name empty and creates a key outright. +async fn disable_all(gateway: &Gateway, name: &str) -> Result { + let keys = exact_matches(gateway.list_named(name).await?, name); + let Some(current) = current_key(&keys) else { + return Ok(Revocation::NoKey); + }; + if !current.enabled { + // Already revoked — answer with the recorded instant (the latest, + // the same one the cap reads), write nothing. Undatable stays + // `None` all the way to the JSON: the page has copy for "we cannot + // date it", and none for the epoch. + return Ok(Revocation::Done { + at: revocation_instant(&keys), + }); + } + // The LATEST instant among the disables that landed, which is what + // `naming::revocation_instant` will read back off the records afterwards + // and what `cap::decide` compares against the period. Never + // `SystemTime::now()`: that is a different clock from the control plane's, + // and on the 1st the two can fall either side of the period boundary — the + // page would then name a `next_eligible_at` a month before the issue + // round-trip would honour it. + let mut applied: Option> = None; + let mut failure: Option = None; + for key in keys.iter().filter(|k| k.enabled) { + // The same last-line guard the reconciler keeps before its deletes: + // on the record, immediately before the write. + if key.name != name { + tracing::error!( + key_id = %key.id, + "refusing to disable a key whose name is not the caller's; \ + the exact-name filter has been bypassed" + ); + continue; + } + match gateway.disable(&key.id).await { + Ok(Disable::Applied(at)) => { + tracing::info!(key_id = %key.id, "portal disabled a revoked API key"); + applied = Some(match applied { + Some(previous) => previous.max(at), + None => at, + }); + } + Ok(Disable::KeyGone) => { + tracing::info!( + key_id = %key.id, + "a key listed for revocation was already gone; nothing to disable" + ); + } + Err(error) => { + tracing::error!( + key_id = %key.id, + error = %error, + "could not disable a key during a revocation; continuing with the rest" + ); + if failure.is_none() { + failure = Some(error); + } + } + } + } + match (applied, failure) { + (Some(at), None) => Ok(Revocation::Done { at }), + (Some(at), Some(_)) => Ok(Revocation::Partial { at }), + // Nothing of ours landed, so the `502` is honest and the dialog's + // "we could not deactivate it" is the right thing to say. + (None, Some(error)) => Err(error), + // Every enabled key raced away between the listing and the patch. + // Not `Done`: there is no disabled record in the account, so telling + // the visitor "deactivated, next eligible on the 1st" would be + // contradicted by the next issue press, which finds the name empty + // and creates a key on the spot. + (None, None) => Ok(Revocation::NoKey), + } +} + +/// What [`disable_all`] found and did. +enum Revocation { + /// Nothing under the name left to revoke. + NoKey, + /// Every key is off. `at` is the revocation instant, `None` only when no + /// record under the name carries a `lastUpdatedDate`. + Done { at: Option }, + /// Some keys are off and at least one could not be disabled — so a + /// credential under this name may still answer on `/v1/`. `at` dates the + /// disables that did land, and the cap is decided from it exactly as for + /// [`Self::Done`]: the owner is capped either way, because a revocation + /// they cannot fully complete must not also cost them the refusal. + Partial { at: Option }, +} + +/// Seconds since the Unix epoch, saturating. +fn now_secs() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0) +} + +/// What the read-only lookup found. +enum Lookup { + /// A live key and its value. + Key(KeyRecord, KeyValue), + /// The owner's key, revoked — no value is read for it. Carries the + /// instant the cap is decided from (`naming::revocation_instant`, the + /// latest revocation), which is what the issue path reads too. + Revoked { revoked_at: Option }, + /// Nothing to reveal. + None, +} + /// The read-only lookup: list, filter, rank, read. Nothing here mutates. /// -/// `Ok(None)` is "no key to reveal" — including the raced case where the +/// `Lookup::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> { +async fn lookup(gateway: &Gateway, name: &str) -> Result { let candidates = exact_matches(gateway.list_named(name).await?, name); - let Some(winner) = choose_winner(&candidates).cloned() else { - return Ok(None); + let Some(current) = current_key(&candidates).cloned() else { + return Ok(Lookup::None); }; - match gateway.value_of(&winner.id).await? { - Some(value) => Ok(Some((winner, value))), - None => Ok(None), + if !current.enabled { + return Ok(Lookup::Revoked { + revoked_at: revocation_instant(&candidates), + }); + } + match gateway.value_of(¤t.id).await? { + Some(value) => Ok(Lookup::Key(current, value)), + None => Ok(Lookup::None), } } /// What the eligibility-checked issue round-trip needs to know. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, 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 owner revoked their key inside the current quota period, so no + /// new one is issued until it rolls (task 0191). Nothing was written. + Capped { + /// `YYYY-MM-DD`, for the landing query. + next_eligible_date: String, + }, /// 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 @@ -392,8 +834,9 @@ pub(crate) async fn issue_for(gateway: &Gateway, sub: &str, deadline: Duration) return IssueOutcome::Failed; }; - match tokio::time::timeout(deadline, reconcile(gateway, &name)).await { - Ok(Ok(Some(outcome))) => { + let deadline_at = std::time::Instant::now() + deadline; + match tokio::time::timeout(deadline, reconcile(gateway, &name, deadline_at)).await { + Ok(Ok(Reconciled::Issued(outcome))) => { tracing::info!( key_id = %outcome.record.id, created = outcome.created, @@ -401,8 +844,22 @@ pub(crate) async fn issue_for(gateway: &Gateway, sub: &str, deadline: Duration) ); IssueOutcome::Issued } + Ok(Ok(Reconciled::Capped { next_eligible_date })) => { + tracing::info!( + outcome = "capped", + "portal issue refused: the key was revoked this quota period" + ); + IssueOutcome::Capped { next_eligible_date } + } + Ok(Ok(Reconciled::OutOfTime)) => { + tracing::error!( + deadline_secs = deadline.as_secs_f32(), + "portal key issuance refused to start a create it could not finish" + ); + IssueOutcome::Failed + } // Every attempt found a key and then lost it before reading its value. - Ok(Ok(None)) => { + Ok(Ok(Reconciled::Lost)) => { tracing::warn!( attempts = MAX_ATTEMPTS, "a key was deleted underneath every issue attempt" @@ -436,36 +893,170 @@ struct Outcome { created: bool, } +/// What the reconciler settled on. +enum Reconciled { + Issued(Outcome), + /// A revoked key inside the current period stands in the way. + Capped { + next_eligible_date: String, + }, + /// Every attempt settled on a key that was deleted before its value could + /// be read. + Lost, + /// Too little time left to create and attach. Nothing was written. + OutOfTime, +} + +/// One attempt's verdict; `Retry` re-enters the flow. +enum Attempt { + Done(Outcome), + Capped { + next_eligible_date: String, + }, + /// Too little of the deadline left to create and attach — nothing was + /// written. Not retried: the next attempt would have even less. + OutOfTime, + Retry, +} + +/// The least time a create is started with. `CreateApiKey` and +/// `CreateUsagePlanKey` each get up to `gateway::OPERATION_TIMEOUT` (5s) in +/// the worst case; in practice each is a few hundred milliseconds. 4s is +/// enough for both at ordinary latency and refuses to start them when the +/// invocation is about to be killed — which is the one way this flow can +/// leave an enabled, unattached key behind. +/// +/// Above `auth::issue::RECONCILE_FLOOR` (2s) on purpose: between the two a +/// reconciliation can still adopt an existing key but will not start a +/// create. The note on that constant is where the trade-off is argued. +pub(crate) const CREATE_FLOOR: Duration = Duration::from_secs(4); + /// List, filter, rank, converge — the reconciler, run up to [`MAX_ATTEMPTS`] /// times. /// -/// `Ok(None)` means every attempt settled on a key that was deleted before its +/// `Lost` means every attempt settled on a key that was deleted before its /// value could be read. That is a real, transient state (a console session, or /// another invocation's reconciler) and it is reported as such rather than as a /// failure of this service. -async fn reconcile(gateway: &Gateway, name: &str) -> Result, GatewayError> { +async fn reconcile( + gateway: &Gateway, + name: &str, + deadline: std::time::Instant, +) -> Result { for _ in 0..MAX_ATTEMPTS { - if let Some(outcome) = attempt(gateway, name).await? { - return Ok(Some(outcome)); + match attempt(gateway, name, deadline).await? { + Attempt::Done(outcome) => return Ok(Reconciled::Issued(outcome)), + Attempt::Capped { next_eligible_date } => { + return Ok(Reconciled::Capped { next_eligible_date }); + } + Attempt::OutOfTime => return Ok(Reconciled::OutOfTime), + Attempt::Retry => {} } } - Ok(None) + Ok(Reconciled::Lost) } -/// One pass of the reconciler. -async fn attempt(gateway: &Gateway, name: &str) -> Result, GatewayError> { +/// One pass of the reconciler. `deadline` is the instant the caller's +/// `timeout` fires; a create is not started without room for itself. +async fn attempt( + gateway: &Gateway, + name: &str, + deadline: std::time::Instant, +) -> Result { // Step 1: everything the control plane has under this name PREFIX, across // all pages, narrowed to exact equality here. Both halves matter and both // are argued for in `naming`. let existing = exact_matches(gateway.list_named(name).await?, name); + // Step 1b (task 0191): a revoked key. If every key under the name is + // disabled, the owner revoked it, and the re-issue cap decides: inside the + // period of the revocation → refused with the date, nothing written; + // after it → the revocation records are deleted and the flow continues + // into a create, exactly as for a first issue. Deleting BEFORE creating + // is safe here, unlike a swap, because a disabled key is not a credential + // anybody can lose; and it keeps the next listing from ranking a dead key + // ahead of the live one. + let (live, mut revoked): (Vec, Vec) = + existing.into_iter().partition(|k| k.enabled); + // Ids this attempt deleted. `GetApiKeys` is eventually consistent, so the + // re-listing after a create can still show them; ranked, a deleted + // earlier-created record wins, its attach 404s, and the single retry is + // spent on a phantom — leaving the key just created unattached. Filtered + // out of the re-listing instead, along with anything disabled. + let mut deleted_here: Vec = Vec::new(); + if live.is_empty() && !revoked.is_empty() { + // The LATEST revocation governs: a duplicate revoked last month must + // not open a door a revocation this month closed. + let revoked_at = revocation_instant(&revoked); + if let Cap::Capped { + next_eligible_date, .. + } = cap::decide(revoked_at, &Period::now()) + { + return Ok(Attempt::Capped { next_eligible_date }); + } + // Logged and stepped over, NOT propagated — the same rule, for the + // same reason, as the loser sweep at the end of this function. A `?` + // here makes a delete that cannot succeed permanent: this branch is + // reached on EVERY press once the name holds nothing but revoked keys, + // so one undeletable record (an untagged exact-name key made by hand in + // the console, against the tag condition task 0194 may add to `DELETE`) + // would answer `?issue=failed` forever, with no in-product recovery. + // Housekeeping must not be able to withhold the key the request is for. + // + // The create below is safe with the record still there: the re-listing + // drops everything disabled, so a survivor is neither ranked nor + // adopted, and the sweep at the end tries it again. + let mut undeleted: Vec = Vec::new(); + for dead in &revoked { + if dead.name != name { + tracing::error!( + key_id = %dead.id, + "refusing to delete a key whose name is not the caller's; \ + the exact-name filter has been bypassed" + ); + continue; + } + tracing::info!(key_id = %dead.id, "deleting a revoked key whose period has rolled"); + match gateway.delete(&dead.id).await { + Ok(()) => deleted_here.push(dead.id.clone()), + Err(error) => { + tracing::error!( + key_id = %dead.id, + error = %error, + "could not delete a revoked key whose period has rolled; continuing \ + into the create and leaving it for the sweep" + ); + undeleted.push(dead.clone()); + } + } + } + // Whatever was deleted here is gone, so the sweep below has nothing of + // theirs left to do; whatever failed stays in the list for it to retry. + revoked = undeleted; + } + let mut created: Option = None; - let mut candidates = existing; + // Live keys only: a revoked key that survived the branch above (one live + // key beside it — a console re-enable, or a duplicate) is neither ranked + // nor adopted; it is swept like any other loser below. + let mut candidates = live; // 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() { + // A create is the one write that cannot be undone by a timeout: if + // the deadline cancels this future after the request left, the key + // exists, attached to nothing, revealed to its owner as a key that + // answers `403`. So the create is started only with room for itself + // and the attach behind it — see `CREATE_FLOOR`. + if deadline.saturating_duration_since(std::time::Instant::now()) < CREATE_FLOOR { + tracing::error!( + "not enough of the deadline left to create AND attach a key; refusing to \ + start a create that could be cut off" + ); + return Ok(Attempt::OutOfTime); + } let (record, _value) = gateway.create(name).await?; // Re-list rather than returning what we just made. Two simultaneous @@ -475,7 +1066,10 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew // the loser is deleted" is actually met. The rank rule is deterministic // (`naming::choose_winner`), so both invocations pick the same survivor // from the same list rather than each deleting the other's. - candidates = exact_matches(gateway.list_named(name).await?, name); + candidates = exact_matches(gateway.list_named(name).await?, name) + .into_iter() + .filter(|k| k.enabled && !deleted_here.contains(&k.id)) + .collect(); if candidates.is_empty() { // The control plane did not list what it just created. Rather than // fail, trust the create — it answered with an id and a value, and @@ -485,9 +1079,9 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew // out a dead id — the one thing the adopt-or-recreate rule exists to // prevent — so this re-enters the flow like any other lost race. if gateway.attach_to_free_plan(&record.id).await? == Attachment::KeyGone { - return Ok(None); + return Ok(Attempt::Retry); } - return Ok(Some(Outcome { + return Ok(Attempt::Done(Outcome { record, created: true, })); @@ -522,7 +1116,7 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew // all (`curl` reports `000`) and an invocation error on the Lambda's // `Errors` metric. Falling through to the retry answers `503` instead. let Some(winner) = choose_winner(&candidates).cloned() else { - return Ok(None); + return Ok(Attempt::Retry); }; // Step 4: the winner is on the plan before anybody is handed it — however @@ -558,11 +1152,15 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew // place that race can surface, so it has to answer it rather than turn a // hand-deleted key back into the dead end this slice exists to remove. if gateway.attach_to_free_plan(&winner.id).await? == Attachment::KeyGone { - return Ok(None); + return Ok(Attempt::Retry); } - // Step 5: everything that is not the winner is deleted. - for loser in losers(&candidates, &winner) { + // Step 5: everything that is not the winner is deleted — duplicates, and + // (task 0191) any revoked key left beside a live one. + for loser in losers(&candidates, &winner) + .into_iter() + .chain(revoked.iter()) + { // Re-asserted immediately before the destructive call, on the record // itself rather than on the query that produced it. `exact_matches` has // already run; this is the guard that survives somebody "simplifying" @@ -604,7 +1202,7 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew if let Some(record) = created && record.id == winner.id { - return Ok(Some(Outcome { + return Ok(Attempt::Done(Outcome { record: winner, created: true, })); @@ -615,11 +1213,11 @@ async fn attempt(gateway: &Gateway, name: &str) -> Result, Gatew // 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(Attempt::Done(Outcome { record: winner, created: false, })), - None => Ok(None), + None => Ok(Attempt::Retry), } } @@ -664,4 +1262,16 @@ mod tests { assert_eq!(rest, "key"); assert!(!rest.contains('/')); } + + /// The revoke route is one segment deeper — `key/rework` — which is + /// still inside the depth the currently-deployed gateway maps (0205's + /// note: depth 1–2 under the prefix work today, depth 3 answers `403` + /// until the greedy proxy deploys). Pinned so it is not moved deeper. + #[test] + fn the_rework_route_is_under_the_key_route_and_no_deeper() { + assert!(REWORK_PATH.starts_with(super::super::PORTAL_API_PREFIX)); + assert_eq!(REWORK_PATH, format!("{KEY_PATH}/rework")); + let rest = REWORK_PATH.trim_start_matches(super::super::PORTAL_API_PREFIX); + assert_eq!(rest.split('/').count(), 2); + } } diff --git a/packages/prices-api/src/portal/keys/naming.rs b/packages/prices-api/src/portal/keys/naming.rs index cc3d7c2e..06b606ff 100644 --- a/packages/prices-api/src/portal/keys/naming.rs +++ b/packages/prices-api/src/portal/keys/naming.rs @@ -79,6 +79,60 @@ pub struct KeyRecord { /// ranks a missing value **last** so that a key we cannot date can never /// beat one we can. pub created_at: Option, + /// Whether the key is enabled (task 0191). A key the owner has revoked is + /// **disabled, not deleted**: it stays in the account as the record of + /// the revocation, because there is no registry (task 0190) and the only + /// fact that can refuse a re-issue inside the same quota period is a + /// disabled key whose [`Self::last_updated_at`] falls inside it. + pub enabled: bool, + /// `lastUpdatedDate`, Unix seconds — for a disabled key, the instant it + /// was revoked, which is what the re-issue cap is decided against. + pub last_updated_at: Option, +} + +/// The instant the re-issue cap is decided from, for a user whose keys are +/// ALL revoked: the **latest** revocation among them (task 0191). +/// +/// One function for the reveal, the revoke and the issue path, so the three +/// cannot disagree — they did: the issue read `max`, the reveal read the +/// earliest record's date, and with two revocation records from different +/// months the page offered "Get my API key" while the round-trip refused it. +/// +/// An **undated** record is skipped, not fatal: `None` comes back only when +/// no record among `revoked` carries a date at all, and the cap treats that +/// as capped. Poisoning the whole answer on one undated record was worse +/// than the shape it guarded against — with `None` the cap recomputes +/// `next_eligible_at` from the *current* period on every read, so the date +/// rolls forward every month and the owner is locked out for good, with no +/// support action short of deleting the record. Skipping can only under-cap, +/// and only in a shape AWS does not produce (`lastUpdatedDate` is always +/// sent); the lockout was permanent. +pub fn revocation_instant(revoked: &[KeyRecord]) -> Option { + revoked + .iter() + .filter_map(|record| record.last_updated_at) + .max() +} + +/// The key the owner currently holds, among `records`: the earliest **enabled** +/// key if there is one, otherwise the earliest key of any state (task 0191). +/// +/// Enabled keys win over disabled ones whatever their dates, because a +/// disabled key is a revocation record and an enabled one is a credential: if +/// both exist (a console re-enable, a duplicate), the credential is what the +/// visitor is holding and what a revoke must act on. Among keys of one state +/// the rule is [`choose_winner`]'s, so both sides of a double-submit agree. +/// +/// The reveal, the revoke and the usage route all select through this, so the +/// key whose value is handed out, the key a revoke disables and the key whose +/// counter is reported are the same key by construction. +pub fn current_key(records: &[KeyRecord]) -> Option<&KeyRecord> { + let enabled: Vec<&KeyRecord> = records.iter().filter(|r| r.enabled).collect(); + if enabled.is_empty() { + choose_winner(records) + } else { + enabled.into_iter().min_by_key(|r| rank(r)) + } } /// Keep only the records whose name is **exactly** `name`. @@ -128,9 +182,71 @@ mod tests { id: id.into(), name: name.into(), created_at, + enabled: true, + last_updated_at: created_at, } } + fn disabled(id: &str, name: &str, created_at: Option) -> KeyRecord { + KeyRecord { + enabled: false, + ..record(id, name, created_at) + } + } + + /// A credential beats a revocation record whatever their dates; among + /// credentials the earliest wins; with no credential the earliest record + /// is what the re-issue cap is read from. + #[test] + fn the_current_key_is_the_earliest_enabled_one_or_else_the_earliest_record() { + let records = vec![ + disabled("revoked-early", "n", Some(10)), + record("live-late", "n", Some(200)), + record("live-early", "n", Some(100)), + ]; + assert_eq!(current_key(&records).unwrap().id, "live-early"); + + let only_disabled = vec![ + disabled("later", "n", Some(20)), + disabled("earlier", "n", Some(10)), + ]; + assert_eq!(current_key(&only_disabled).unwrap().id, "earlier"); + assert!(current_key(&[]).is_none()); + } + + /// The latest revocation governs; an undated record is skipped, and only + /// a set with no dated record at all is undatable (→ capped). + #[test] + fn the_revocation_instant_is_the_latest_dated_one() { + let earlier = KeyRecord { + last_updated_at: Some(10), + ..disabled("a", "n", Some(1)) + }; + let later = KeyRecord { + last_updated_at: Some(20), + ..disabled("b", "n", Some(2)) + }; + assert_eq!( + revocation_instant(&[earlier.clone(), later.clone()]), + Some(20) + ); + assert_eq!(revocation_instant(&[later.clone(), earlier]), Some(20)); + let undated = KeyRecord { + last_updated_at: None, + ..disabled("c", "n", Some(3)) + }; + // One undated record beside a dated one does NOT erase the date: a + // `None` here caps the owner against a next_eligible_at recomputed + // from the current period every read, which never arrives. + assert_eq!( + revocation_instant(&[undated.clone(), later.clone()]), + Some(20) + ); + // Nothing datable at all is the one undatable case. + assert_eq!(revocation_instant(&[undated]), None); + assert_eq!(revocation_instant(&[]), None); + } + #[test] fn a_name_is_the_id_wrapped_in_both_fixed_parts() { assert_eq!( diff --git a/packages/prices-api/src/portal/mod.rs b/packages/prices-api/src/portal/mod.rs index f6ba7413..a1e0d178 100644 --- a/packages/prices-api/src/portal/mod.rs +++ b/packages/prices-api/src/portal/mod.rs @@ -45,6 +45,7 @@ pub mod auth; pub mod eligibility; pub mod keys; +pub mod period; pub mod usage; use axum::extract::{Request, State}; diff --git a/packages/prices-api/src/portal/period.rs b/packages/prices-api/src/portal/period.rs new file mode 100644 index 00000000..2be69c63 --- /dev/null +++ b/packages/prices-api/src/portal/period.rs @@ -0,0 +1,157 @@ +//! The quota period under **our** rule: the calendar month, UTC +//! (task 0188's dashboard, task 0191's rework cap). +//! +//! One definition, two readers. The usage route renders a period around +//! AWS's counters, and the rework cap decides whether a key's `createdDate` +//! falls before the current period began. Both used to be able to carry their +//! own copy of "the 1st of the month, 00:00 UTC"; with the cap this becomes a +//! correctness property rather than a label — a dashboard that says the quota +//! resets on the 1st while the cap counts from a different instant would let +//! a rework through that the page said was refused, or the reverse — so the +//! rule lives here and nowhere else. +//! +//! # This is our rule, not AWS's — and what has been measured +//! +//! AWS documents neither the instant its `MONTH` quota rolls nor the timezone +//! it rolls in (ADR 0010, correction #2). The only statement anywhere is an +//! example caption, *"creates a usage plan that resets at the beginning of +//! the month"*, and `offset` is a request count, not a time shift. Task 0191 +//! measures the instant on a `DAY`-period scratch plan as a proxy (the +//! result, with its date, is in that task's Step 0 table); the first real +//! `MONTH` rollover observable after this code exists is 1 September 2026. +//! Nothing here is contingent on the answer: if AWS's counter turns out to +//! roll at a different instant, the dashboard label is a UX wrinkle to word +//! around, and the cap is ours to define regardless. + +use chrono::{Datelike, NaiveDate, Utc}; + +/// The current quota period — a calendar month in UTC. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Period { + /// The 1st of the month. + start: NaiveDate, + /// The 1st of the following month: the instant the period ends, and the + /// instant a rework capped in this period becomes available again. + next_start: NaiveDate, +} + +impl Period { + /// The period containing `today`. + /// + /// Pure, so the December rollover and the leap year are decidable by a + /// unit test rather than by waiting for one. AWS is deliberately not + /// consulted — see the module docs. + pub fn containing(today: NaiveDate) -> Self { + let start = NaiveDate::from_ymd_opt(today.year(), today.month(), 1) + .expect("the 1st of a real month exists"); + let next_start = if today.month() == 12 { + NaiveDate::from_ymd_opt(today.year() + 1, 1, 1) + } else { + NaiveDate::from_ymd_opt(today.year(), today.month() + 1, 1) + } + .expect("the 1st of the following month exists"); + Self { start, next_start } + } + + /// The period the clock says we are in, UTC. + pub fn now() -> Self { + Self::containing(Utc::now().date_naive()) + } + + /// First day of the period, `YYYY-MM-DD`. + pub fn start_ymd(&self) -> String { + self.start.format("%Y-%m-%d").to_string() + } + + /// Last day of the period, inclusive, `YYYY-MM-DD` — what `GetUsage`'s + /// `endDate` expects. + pub fn end_ymd(&self) -> String { + self.next_start + .pred_opt() + .expect("the day before the 1st exists") + .format("%Y-%m-%d") + .to_string() + } + + /// First day of the **following** period, `YYYY-MM-DD` — the date a + /// rework refused in this period names as "next eligible". + pub fn next_start_ymd(&self) -> String { + self.next_start.format("%Y-%m-%d").to_string() + } + + /// The instant this period ends and the next begins, RFC 3339 — the + /// dashboard's `resets_at` and the rework refusal's `next_eligible_at` + /// are the same instant by construction. + pub fn resets_at(&self) -> String { + format!("{}T00:00:00Z", self.next_start_ymd()) + } + + /// The instant this period began, in Unix seconds — what a key's + /// `createdDate` (also Unix seconds) is compared against. + pub fn start_secs(&self) -> u64 { + let secs = self + .start + .and_hms_opt(0, 0, 0) + .expect("midnight exists") + .and_utc() + .timestamp(); + u64::try_from(secs).unwrap_or(0) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn period(y: i32, m: u32, d: u32) -> Period { + Period::containing(NaiveDate::from_ymd_opt(y, m, d).unwrap()) + } + + /// Mid-month, mid-year — the ordinary case. + #[test] + fn the_period_is_the_calendar_month() { + let p = period(2026, 8, 19); + assert_eq!(p.start_ymd(), "2026-08-01"); + assert_eq!(p.end_ymd(), "2026-08-31"); + assert_eq!(p.next_start_ymd(), "2026-09-01"); + assert_eq!(p.resets_at(), "2026-09-01T00:00:00Z"); + } + + /// The 1st and the last day are both inside their own month. + #[test] + fn the_boundaries_belong_to_their_month() { + let first = period(2026, 9, 1); + assert_eq!(first.start_ymd(), "2026-09-01"); + assert_eq!(first.end_ymd(), "2026-09-30"); + let last = period(2026, 9, 30); + assert_eq!(last.start_ymd(), "2026-09-01"); + assert_eq!(last.resets_at(), "2026-10-01T00:00:00Z"); + } + + /// December resets into the next year. + #[test] + fn december_rolls_into_january() { + let p = period(2026, 12, 31); + assert_eq!(p.start_ymd(), "2026-12-01"); + assert_eq!(p.end_ymd(), "2026-12-31"); + assert_eq!(p.resets_at(), "2027-01-01T00:00:00Z"); + } + + /// February in a leap year has its 29th. + #[test] + fn a_leap_february_ends_on_the_29th() { + let p = period(2028, 2, 15); + assert_eq!(p.end_ymd(), "2028-02-29"); + assert_eq!(p.resets_at(), "2028-03-01T00:00:00Z"); + } + + /// The instant the period began, as the number a `createdDate` is + /// compared against: 2026-08-01T00:00:00Z is 1 785 542 400. + #[test] + fn the_start_instant_is_midnight_utc_on_the_first() { + assert_eq!(period(2026, 8, 21).start_secs(), 1_785_542_400); + assert_eq!(period(2026, 8, 1).start_secs(), 1_785_542_400); + // Epoch-adjacent dates do not underflow. + assert_eq!(period(1970, 1, 15).start_secs(), 0); + } +} diff --git a/packages/prices-api/src/portal/usage/mod.rs b/packages/prices-api/src/portal/usage/mod.rs index 39e6785e..b531ea84 100644 --- a/packages/prices-api/src/portal/usage/mod.rs +++ b/packages/prices-api/src/portal/usage/mod.rs @@ -81,14 +81,16 @@ use axum::extract::State; use axum::http::{HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::routing::get; -use chrono::{Datelike, NaiveDate, SecondsFormat, Utc}; +use chrono::{SecondsFormat, Utc}; use serde::Serialize; use crate::common::{cache_control, errors}; use super::auth::secret::OauthSecret; +use super::keys::cap::{self, Cap}; use super::keys::gateway::{Gateway, GatewayError}; -use super::keys::naming::{choose_winner, exact_matches, key_name}; +use super::keys::naming::{current_key, exact_matches, key_name, revocation_instant}; +use super::period::Period; /// The one route. `GET` only — reading a counter must not share a path shape /// with anything that writes. @@ -195,6 +197,27 @@ struct EpochMark { pub struct UsageCache(Arc>); impl UsageCache { + /// Drop **whatever** is cached for `sub` — a real answer or a "no key" — + /// and bump the caller's epoch (task 0191). + /// + /// The rework path's eviction. A rework deletes the key the cached + /// numbers describe and issues one with a fresh counter, so a cached + /// usage answer is not stale but *about a key that no longer exists*; + /// serving it for the rest of the TTL would show the old key's + /// consumption under the new key's name. The epoch bump keeps an + /// in-flight "no key" from being stored (see [`CacheInner`]); an in-flight + /// **real** answer for the old key can still land for one TTL, which the + /// response's `as_of` dates honestly and which is bounded by + /// [`CACHE_TTL`] — accepted, because the alternative is discarding every + /// real answer whose lookup overlapped any rework. + pub fn invalidate(&self, sub: &str) { + { + let mut cache = self.0.lock().expect("the usage cache lock is not poisoned"); + cache.entries.remove(sub); + } + self.invalidate_no_key(sub); + } + /// Drop a cached "no key" answer for `sub`, if that is what is cached — /// and bump the caller's epoch either way, so an in-flight lookup that /// snapshotted the keyless state cannot write it back afterwards (see @@ -352,12 +375,12 @@ async fn usage(State(state): State, headers: HeaderMap) -> Response // last month's answer wearing this month's label, and the minute after a // month boundary is exactly when a viewer checks whether the reset // happened. - let period_now = current_period(Utc::now().date_naive()); + let period_start = Period::now().start_ymd(); // Fresh cache hit: no control-plane call of any kind. This is the // "repeated dashboard loads do not produce one GetUsage call each" // acceptance criterion, in one branch. - if let Some(entry) = cached(&state, &session.sub, state.ttl, &period_now.start) { + if let Some(entry) = cached(&state, &session.sub, state.ttl, &period_start) { return answer(entry.answer); } @@ -376,7 +399,7 @@ async fn usage(State(state): State, headers: HeaderMap) -> Response // timing — so it gets the same answer as the throttle arm below: // the last good answer (re-stamped, so the next TTL of loads // leaves the struggling control plane alone) beats the error page. - if let Some(entry) = cached(&state, &session.sub, STALE_KEEP, &period_now.start) { + if let Some(entry) = cached(&state, &session.sub, STALE_KEEP, &period_start) { tracing::warn!( deadline_secs = state.deadline.as_secs_f32(), "portal usage lookup ran out of time; serving the cached answer" @@ -407,7 +430,7 @@ async fn usage(State(state): State, headers: HeaderMap) -> Response // happening and invite a retry; the entry the next success writes ends // the condition. Err(GatewayError::Throttled { operation }) => { - if let Some(entry) = cached(&state, &session.sub, STALE_KEEP, &period_now.start) { + if let Some(entry) = cached(&state, &session.sub, STALE_KEEP, &period_start) { tracing::warn!( operation, "control plane is throttling; serving the cached usage answer" @@ -456,13 +479,26 @@ async fn fetch(gateway: &Gateway, name: &str) -> Result Result Result Period { - let start = NaiveDate::from_ymd_opt(today.year(), today.month(), 1) - .expect("the 1st of a real month exists"); - let next_first = if today.month() == 12 { - NaiveDate::from_ymd_opt(today.year() + 1, 1, 1) - } else { - NaiveDate::from_ymd_opt(today.year(), today.month() + 1, 1) - } - .expect("the 1st of the following month exists"); - let end = next_first - .pred_opt() - .expect("the day before the 1st exists"); - - Period { - start: start.format("%Y-%m-%d").to_string(), - end: end.format("%Y-%m-%d").to_string(), - resets_at: format!("{}T00:00:00Z", next_first.format("%Y-%m-%d")), - } -} - /// The cached entry for `sub`, if it is younger than `max_age` **and answers /// for the current period**. fn cached( @@ -693,47 +694,6 @@ mod tests { assert!(!rest.contains('/')); } - fn period(y: i32, m: u32, d: u32) -> Period { - current_period(NaiveDate::from_ymd_opt(y, m, d).unwrap()) - } - - /// Mid-month, mid-year — the ordinary case. - #[test] - fn the_period_is_the_calendar_month() { - let p = period(2026, 8, 19); - assert_eq!(p.start, "2026-08-01"); - assert_eq!(p.end, "2026-08-31"); - assert_eq!(p.resets_at, "2026-09-01T00:00:00Z"); - } - - /// The 1st and the last day are both inside their own month. - #[test] - fn the_boundaries_belong_to_their_month() { - let first = period(2026, 9, 1); - assert_eq!(first.start, "2026-09-01"); - assert_eq!(first.end, "2026-09-30"); - let last = period(2026, 9, 30); - assert_eq!(last.start, "2026-09-01"); - assert_eq!(last.resets_at, "2026-10-01T00:00:00Z"); - } - - /// December resets into the next year. - #[test] - fn december_rolls_into_january() { - let p = period(2026, 12, 31); - assert_eq!(p.start, "2026-12-01"); - assert_eq!(p.end, "2026-12-31"); - assert_eq!(p.resets_at, "2027-01-01T00:00:00Z"); - } - - /// February in a leap year has its 29th. - #[test] - fn a_leap_february_ends_on_the_29th() { - let p = period(2028, 2, 15); - assert_eq!(p.end, "2028-02-29"); - assert_eq!(p.resets_at, "2028-03-01T00:00:00Z"); - } - fn usage_answer(period_start: &str) -> CachedAnswer { CachedAnswer::Usage(UsageResponse { used: Some(1), diff --git a/packages/prices-api/tests/portal_auth.rs b/packages/prices-api/tests/portal_auth.rs index b484e4c1..4cdd98ca 100644 --- a/packages/prices-api/tests/portal_auth.rs +++ b/packages/prices-api/tests/portal_auth.rs @@ -395,7 +395,7 @@ 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 — 0191's `rework`, arriving early — +/// action this build does not implement — 0192's `revoke`, arriving early — /// cannot start a round-trip that the callback would then have to decide what /// to do with. #[tokio::test] @@ -407,7 +407,7 @@ async fn login_refuses_an_action_it_does_not_implement() { .status, StatusCode::SEE_OTHER ); - let refused = fetch(&open, &format!("{LOGIN_PATH}?action=rework"), &[]).await; + let refused = fetch(&open, &format!("{LOGIN_PATH}?action=revoke"), &[]).await; assert_eq!(refused.status, StatusCode::BAD_REQUEST); assert!(refused.set_cookies().is_empty()); } diff --git a/packages/prices-api/tests/portal_keys/harness.rs b/packages/prices-api/tests/portal_keys/harness.rs index bf17912c..c4dda4b6 100644 --- a/packages/prices-api/tests/portal_keys/harness.rs +++ b/packages/prices-api/tests/portal_keys/harness.rs @@ -63,6 +63,11 @@ pub struct StoredKey { pub created_at: u64, pub tags: HashMap, pub enabled: bool, + /// `lastUpdatedDate` — stamped by `UpdateApiKey` (the revoke, task + /// 0191), which is what the re-issue cap reads off a disabled key. + /// `None` is the shape the service never sends and [`Store::undate`] + /// makes, so the cap's handling of it is testable end to end. + pub last_updated_at: Option, } #[derive(Default)] @@ -135,6 +140,44 @@ pub struct Store { /// reaches that code. Worth knowing — it means the branch guards a listing /// that disagrees with the reader, not a busy deleter. pub read_always_404: bool, + /// Stamp every key `CreateApiKey` mints with this `createdDate` instead of + /// the current time (task 0191) — so a test can create a key "on the 3rd + /// of last month" and then ask the cap about it. + pub created_at_override: Option, + /// Answer `DeleteApiKey` for exactly these ids with a 500, and succeed for + /// every other id (task 0191). Unlike the sticky `fail_deletes`, this lets + /// a test fail the delete of the OLD key while the rollback delete of the + /// replacement succeeds — which is the only way to observe the rollback. + pub fail_delete_of: Vec, + /// Every write, in the order it arrived: `create:`, `attach:`, + /// `delete:`, `disable:` (task 0191) — so a test can assert on + /// what was written and in which order, not only on the end state. + pub ops: Vec, + /// Answer every `UpdateApiKey` with a 500 (task 0191). Sticky, like + /// `fail_deletes`, for the same reason. + pub fail_disables: bool, + /// Answer `UpdateApiKey` for exactly these ids with a 500 and succeed for + /// every other id (task 0191) — the per-id twin of `fail_disables`, and the + /// only way to reach a PARTIAL revocation: one key of a double-submit pair + /// goes off while the other refuses. + pub fail_disable_of: Vec, + /// Stamp a disabled key's `lastUpdatedDate` with this instant instead of + /// the mock's clock (task 0191), so a test can prove the revocation date in + /// the answer came from the control plane's response and not from the + /// handler's own `SystemTime::now()`. + pub disable_stamps_at: Option, + /// Answer EVERY `GetApiKeys` with the keys the store holds PLUS every key + /// deleted so far, as if the listing never caught up with the deletes + /// (task 0191). Sticky, so the re-listing AFTER a delete is the one that + /// shows the phantom — the shape that made the post-roll re-issue rank a + /// deleted record and burn its retry. + pub list_resurrects_deleted: bool, + /// Snapshots of every key deleted, for the knob above. + pub deleted_keys: Vec, + /// Answer the next `CreateApiKey` with a 500 AFTER creating the key — the + /// "request landed, response lost" shape that an SDK retry turns into a + /// duplicate (task 0191). One-shot. + pub fail_next_create_after_creating: bool, pub next_id: usize, // ----------------------------------------------------------------------- @@ -183,10 +226,29 @@ impl Store { created_at, tags: HashMap::new(), enabled: true, + last_updated_at: Some(created_at), }); id } + /// A key the owner revoked at `revoked_at` (task 0191): disabled, with + /// `lastUpdatedDate` set to the revocation instant. + pub fn seed_revoked(&mut self, name: &str, created_at: u64, revoked_at: u64) -> String { + let id = self.seed(name, created_at); + let key = self.keys.last_mut().unwrap(); + key.enabled = false; + key.last_updated_at = Some(revoked_at); + id + } + + /// Strip a key's `lastUpdatedDate`, the shape AWS does not send — what + /// the cap must survive without locking its owner out forever. + pub fn undate(&mut self, id: &str) { + if let Some(key) = self.keys.iter_mut().find(|k| k.id == id) { + key.last_updated_at = None; + } + } + pub fn mint_id(&mut self) -> String { self.next_id += 1; format!("key{:03}", self.next_id) @@ -222,7 +284,10 @@ impl MockGateway { let router = Router::new() .route("/apikeys", get(list_keys).post(create_key)) - .route("/apikeys/{id}", get(read_key).delete(delete_key)) + .route( + "/apikeys/{id}", + get(read_key).delete(delete_key).patch(update_key), + ) .route("/usageplans/{plan}/keys", post(attach_key)) .route("/usageplans/{plan}/usage", get(read_usage)) .with_state(store.clone()); @@ -288,12 +353,21 @@ pub async fn list_keys( } else { None }; - let matched: Vec = store + let mut matched: Vec = store .list(&prefix) .into_iter() .filter(|k| Some(&k.id) != newest.as_ref()) .cloned() .collect(); + if store.list_resurrects_deleted { + let ghosts: Vec = store + .deleted_keys + .iter() + .filter(|k| k.name.starts_with(&prefix)) + .cloned() + .collect(); + matched.extend(ghosts); + } let start: usize = query .position .as_deref() @@ -322,9 +396,13 @@ pub async fn create_key( id: id.clone(), name: body["name"].as_str().unwrap_or_default().to_string(), value: format!("VALUE-{id}-CANARY"), - // Whole seconds, like the service — which is why the winner rule needs - // an id tie-break. - created_at: 1_800_000_000, + // Whole seconds and the CURRENT time, like the service — whole seconds + // is why the winner rule needs an id tie-break, and "now" is what lets + // task 0191's cap be tested: a key the mock just created must read as + // created inside the current quota period, exactly as a real one + // would. `created_at_override` pins it for tests that need a date. + created_at: store.created_at_override.unwrap_or_else(now_secs), + last_updated_at: Some(store.created_at_override.unwrap_or_else(now_secs)), tags: body["tags"] .as_object() .map(|o| { @@ -336,6 +414,10 @@ pub async fn create_key( enabled: body["enabled"].as_bool().unwrap_or(false), }; store.keys.push(created.clone()); + store.ops.push(format!("create:{id}")); + if std::mem::take(&mut store.fail_next_create_after_creating) { + return (StatusCode::INTERNAL_SERVER_ERROR, "response lost").into_response(); + } (StatusCode::CREATED, Json(api_key_json(&created))).into_response() } @@ -378,11 +460,15 @@ pub async fn delete_key( Path(id): Path, ) -> Response { let mut store = store.lock().unwrap(); - if store.fail_deletes { + if store.fail_deletes || store.fail_delete_of.contains(&id) { return (StatusCode::INTERNAL_SERVER_ERROR, "control plane is unwell").into_response(); } store.deleted.push(id.clone()); + store.ops.push(format!("delete:{id}")); let existed = store.keys.iter().any(|k| k.id == id); + if let Some(gone) = store.keys.iter().find(|k| k.id == id).cloned() { + store.deleted_keys.push(gone); + } store.keys.retain(|k| k.id != id); if existed { StatusCode::ACCEPTED.into_response() @@ -391,6 +477,41 @@ pub async fn delete_key( } } +/// `PATCH /apikeys/{id}` — `UpdateApiKey`, task 0191's revoke. Applies +/// `replace /enabled`, stamps `lastUpdatedDate` like the service, and answers +/// the key. The body shape is what restJson1 sends: `{"patchOperations": +/// [{"op":"replace","path":"/enabled","value":"false"}]}`. +pub async fn update_key( + State(store): State>>, + Path(id): Path, + Json(body): Json, +) -> Response { + let mut store = store.lock().unwrap(); + if store.fail_disables || store.fail_disable_of.contains(&id) { + return (StatusCode::INTERNAL_SERVER_ERROR, "control plane is unwell").into_response(); + } + let now = store.disable_stamps_at.unwrap_or_else(now_secs); + let Some(key) = store.keys.iter_mut().find(|k| k.id == id) else { + return not_found(); + }; + for op in body["patchOperations"].as_array().into_iter().flatten() { + if op["op"] == "replace" && op["path"] == "/enabled" { + key.enabled = op["value"] == "true"; + key.last_updated_at = Some(now); + } + } + let snapshot = key.clone(); + store.ops.push(format!( + "{}:{id}", + if snapshot.enabled { + "enable" + } else { + "disable" + } + )); + Json(api_key_json(&snapshot)).into_response() +} + pub async fn attach_key( State(store): State>>, Path(plan): Path, @@ -422,6 +543,7 @@ pub async fn attach_key( return conflict(); } store.plan_keys.push((plan, key_id.clone())); + store.ops.push(format!("attach:{key_id}")); ( StatusCode::CREATED, Json(json!({ "id": key_id, "type": "API_KEY" })), @@ -554,6 +676,7 @@ pub fn api_key_json(key: &StoredKey) -> Value { "value": key.value, "enabled": key.enabled, "createdDate": key.created_at, + "lastUpdatedDate": key.last_updated_at, "tags": key.tags, }) } @@ -749,10 +872,25 @@ pub async fn call(router: Router, method: &str, cookie: Option<&str>) -> Reply { } pub async fn call_path(router: Router, method: &str, path: &str, cookie: Option<&str>) -> Reply { + call_path_with(router, method, path, cookie, &[]).await +} + +/// [`call_path`] plus extra request headers — what the revoke (task 0191) +/// needs, since it refuses a request without the portal's own marker. +pub async fn call_path_with( + router: Router, + method: &str, + path: &str, + cookie: Option<&str>, + extra: &[(&str, &str)], +) -> Reply { let mut request = Request::builder().method(method).uri(path); if let Some(cookie) = cookie { request = request.header(header::COOKIE, cookie); } + for (name, value) in extra { + request = request.header(*name, *value); + } let response = router .oneshot(request.body(Body::empty()).unwrap()) .await @@ -787,10 +925,20 @@ pub async fn post_key(mock: &MockGateway, sub: &str) -> Reply { /// callback's reply — a `303` whose `Location` is one of the five /// `?issue=…` landing states. pub async fn issue_round_trip(router: &Router) -> Reply { + round_trip(router, "issue").await +} + +/// The shared body of the round-trip helpers above. +/// +/// `action` must be one [`Action::parse`] accepts; anything else answers `400` +/// at `/auth/login` and the `303` assertion below panics. That is why there is +/// no `rework` variant here: task 0191's revoke is a same-origin `POST`, not an +/// OAuth round-trip, and `state_token.rs` asserts `"rework"` is not an action. +async fn round_trip(router: &Router, action: &str) -> Reply { let login = call_path( router.clone(), "GET", - "/api-tokens/api/auth/login?action=issue", + &format!("/api-tokens/api/auth/login?action={action}"), None, ) .await; diff --git a/packages/prices-api/tests/portal_rework.rs b/packages/prices-api/tests/portal_rework.rs new file mode 100644 index 00000000..4ed0a306 --- /dev/null +++ b/packages/prices-api/tests/portal_rework.rs @@ -0,0 +1,1051 @@ +//! Replacing a key — revoke now, re-issue next period — over HTTP, end to end +//! (task 0191). +//! +//! "Replace my key" is a **revocation**: `POST /key/rework` disables the +//! caller's key immediately and issues nothing. The replacement is an ordinary +//! issue round-trip (`tests/portal_issue.rs`'s flow, driven here through the +//! same mock Discord), and the issue path refuses it until the quota period in +//! which the revocation happened has rolled — the re-issue cap, decided from +//! the disabled key's `lastUpdatedDate`. +//! +//! Three properties are the spine of this file: +//! +//! - **Revoke is immediate and total.** Every key under the name is disabled +//! in one `POST`, with no Discord call; the reveal then says "revoked" and +//! names the date, and never hands the dead value out again. +//! - **The replacement waits for the 1st.** An issue inside the revocation's +//! period is `?issue=capped&next_eligible_at=…` with nothing written; the +//! same issue once the period has rolled deletes the revocation record and +//! creates the new key. +//! - **A session cookie can revoke its own key and nothing else.** The route +//! is `POST`-only, writes only `enabled=false` on the caller's exact name, +//! and every other state the store can be in leaves it untouched. + +#[path = "portal_keys/harness.rs"] +mod harness; +#[path = "common/mock_discord.rs"] +mod mock_discord; + +use axum::Router; +use axum::http::{StatusCode, header}; +use chrono::{Datelike, NaiveDate, Utc}; + +use harness::*; +use mock_discord::{GRANTED_SCOPE, MemberReply, MockDiscord}; +use prices_api::portal::auth::discord::Endpoints; +use prices_api::portal::keys::gateway::Gateway; +use prices_api::portal::keys::{KEY_PATH, PORTAL_REQUEST_HEADER, REWORK_PATH}; +use prices_api::portal::usage::USAGE_PATH; + +fn app_with_discord(discord: &MockDiscord, gateway: &MockGateway) -> 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, "5")), + ) +} + +fn key_name() -> String { + format!("discord-{USER_ID}-key") +} + +/// Unix seconds for a UTC date at noon. +fn noon(date: NaiveDate) -> u64 { + date.and_hms_opt(12, 0, 0).unwrap().and_utc().timestamp() as u64 +} + +/// The 1st of the month `months` after this one, UTC. +fn first_of_month_offset(months: i32) -> NaiveDate { + let today = Utc::now().date_naive(); + let total = today.year() * 12 + today.month0() as i32 + months; + NaiveDate::from_ymd_opt(total.div_euclid(12), (total.rem_euclid(12) + 1) as u32, 1).unwrap() +} + +/// "Revoked on the 3rd" of the month beginning `first`. +fn the_3rd_of(first: NaiveDate) -> u64 { + noon(first.with_day(3).unwrap()) +} + +fn next_eligible_expected() -> (String, String) { + let next = first_of_month_offset(1); + ( + format!("{}T00:00:00Z", next.format("%Y-%m-%d")), + next.format("%Y-%m-%d").to_string(), + ) +} + +/// Seed an attached, live key for the user, created at `created_at`. +fn seed_attached(gateway: &MockGateway, created_at: u64) -> String { + gateway.with(|s| { + let id = s.seed(&key_name(), created_at); + s.plan_keys.push((PLAN_ID.to_string(), id.clone())); + id + }) +} + +/// Seed a key the user revoked at `revoked_at`. +fn seed_revoked(gateway: &MockGateway, revoked_at: u64) -> String { + gateway.with(|s| { + let id = s.seed_revoked(&key_name(), 1_000, revoked_at); + s.plan_keys.push((PLAN_ID.to_string(), id.clone())); + id + }) +} + +/// The portal page's revoke: `POST` with the same-origin marker header the +/// backend requires (`PORTAL_REQUEST_HEADER`). +async fn revoke(router: &Router, cookie: Option<&str>) -> Reply { + call_path_with( + router.clone(), + "POST", + REWORK_PATH, + cookie, + &[PORTAL_REQUEST_HEADER, ("sec-fetch-site", "same-origin")], + ) + .await +} + +async fn reveal_via(router: &Router) -> Reply { + call_path( + router.clone(), + "GET", + KEY_PATH, + Some(&session_cookie(USER_ID)), + ) + .await +} + +// --------------------------------------------------------------------------- +// The gate — ships closed +// --------------------------------------------------------------------------- + +/// The revoke is an empty `404` while the portal is closed, with a valid +/// session presented and zero control-plane calls. +#[tokio::test] +async fn revoke_is_an_empty_404_while_the_portal_is_closed() { + let gateway = MockGateway::start().await; + seed_attached(&gateway, 1_000); + let closed = build_app( + false, + Some(Gateway::against(&gateway.base, PLAN_ID.to_string())), + ); + + let reply = revoke(&closed, Some(&session_cookie(USER_ID))).await; + assert_eq!(reply.status, StatusCode::NOT_FOUND); + assert!(reply.body.is_empty()); + assert_eq!(gateway.with(|s| s.list_calls), 0); + assert_eq!(gateway.with(|s| s.ops.len()), 0); + assert!(gateway.with(|s| s.keys[0].enabled), "nothing was touched"); +} + +// --------------------------------------------------------------------------- +// Revoke — immediate, total, no Discord +// --------------------------------------------------------------------------- + +/// The user's scenario: a key issued TODAY leaks, "Replace my key" is +/// pressed, and the key is disabled on the control plane in one +/// `UpdateApiKey` — no Discord call, nothing created — with the answer naming +/// the 1st of next month and the revocation instant. (The data plane follows +/// in ~25 s, 0180 item 8; that window is the page's to state, and it does.) +#[tokio::test] +async fn revoke_disables_the_key_in_one_call_and_issues_nothing() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let key = seed_attached(&gateway, now_secs()); + let app = app_with_discord(&discord, &gateway); + let (expected_at, _) = next_eligible_expected(); + + let reply = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!( + reply.status, + StatusCode::OK, + "{}", + String::from_utf8_lossy(&reply.body) + ); + let body = reply.json(); + assert_eq!(body["revoked"], true); + assert_eq!(body["next_eligible_at"], expected_at); + assert!(body["revoked_at"].as_str().unwrap().ends_with('Z')); + assert_eq!(reply.cache_control(), "no-store"); + + gateway.with(|s| { + assert!(!s.keys[0].enabled, "the key is off"); + assert_eq!(s.keys.len(), 1, "and nothing was created"); + assert_eq!(s.ops, vec![format!("disable:{key}")]); + }); + assert_eq!(discord.exchanges(), 0, "no Discord round-trip for a revoke"); + assert_eq!(discord.member_calls(), 0); +} + +/// After the revoke the reveal never hands the dead value out again: it +/// answers `404 key_revoked` with the date, and the page renders that instead +/// of the "get my API key" link. +#[tokio::test] +async fn a_revoked_key_is_never_revealed_again_and_the_reveal_names_the_date() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_attached(&gateway, 1_000); + let app = app_with_discord(&discord, &gateway); + let (expected_at, _) = next_eligible_expected(); + + assert_eq!( + revoke(&app, Some(&session_cookie(USER_ID))).await.status, + StatusCode::OK + ); + + let revealed = reveal_via(&app).await; + assert_eq!(revealed.status, StatusCode::NOT_FOUND); + let body = revealed.json(); + assert_eq!(body["code"], "key_revoked"); + assert_eq!(body["details"]["next_eligible_at"], expected_at); + assert!(body["details"]["revoked_at"].is_string()); + assert!( + !String::from_utf8_lossy(&revealed.body).contains("CANARY"), + "the revoked value must not appear anywhere" + ); + assert_eq!(revealed.cache_control(), "no-store"); +} + +/// Idempotent: a second press answers `200` with the same dates and writes +/// nothing more — a retry after a dropped response is safe. +#[tokio::test] +async fn revoking_twice_is_one_write() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let key = seed_attached(&gateway, 1_000); + let app = app_with_discord(&discord, &gateway); + + let first = revoke(&app, Some(&session_cookie(USER_ID))).await; + let second = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!(second.status, StatusCode::OK); + assert_eq!( + second.json()["next_eligible_at"], + first.json()["next_eligible_at"] + ); + assert_eq!( + gateway.with(|s| s.ops.clone()), + vec![format!("disable:{key}")] + ); +} + +/// Every key under the name dies, not only the current one: a duplicate left +/// by an earlier double-submit is a working credential the visitor was just +/// told had stopped. +#[tokio::test] +async fn every_key_under_the_name_is_revoked_not_only_the_current_one() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_attached(&gateway, 1_000); + gateway.with(|s| s.seed(&key_name(), 2_000)); + let app = app_with_discord(&discord, &gateway); + + assert_eq!( + revoke(&app, Some(&session_cookie(USER_ID))).await.status, + StatusCode::OK + ); + gateway.with(|s| { + assert!(s.keys.iter().all(|k| !k.enabled)); + assert_eq!(s.ops.len(), 2); + }); +} + +/// The ordinary answer carries no `partial` at all — the flag is omitted, not +/// sent as `false`, so a client that never learned about it reads the same +/// shape it always did. +#[tokio::test] +async fn a_clean_revocation_omits_the_partial_flag() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_attached(&gateway, 1_000); + let app = app_with_discord(&discord, &gateway); + + let reply = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!(reply.status, StatusCode::OK); + assert!( + reply.json().get("partial").is_none(), + "a clean revocation sends no partial flag: {}", + reply.json() + ); +} + +/// A disable that fails for ONE key of a pair is reported as partial — not +/// dressed up as a clean revocation, and not thrown away as a `502` either. +/// +/// The visitor's own key is off, so `502` would be a lie in the other +/// direction (and would have re-armed a dialog whose work was half done). The +/// answer carries `partial`, which is what stops the page rendering a plain +/// "revoked" while a duplicate still answers on `/v1/`. +#[tokio::test] +async fn a_partial_revocation_is_reported_as_partial() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let first = seed_attached(&gateway, 1_000); + let stubborn = gateway.with(|s| s.seed(&key_name(), 2_000)); + gateway.with(|s| s.fail_disable_of = vec![stubborn.clone()]); + let app = app_with_discord(&discord, &gateway); + + let reply = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!(reply.status, StatusCode::OK); + assert_eq!(reply.json()["revoked"], true); + assert_eq!(reply.json()["partial"], true); + assert!( + !reply.json()["revoked_at"].is_null(), + "the disables that landed are still dated" + ); + gateway.with(|s| { + let by_id = |id: &str| s.keys.iter().find(|k| k.id == id).unwrap().enabled; + assert!(!by_id(&first), "the key that could be disabled is off"); + assert!(by_id(&stubborn), "the one that refused is untouched"); + }); + // And the cap does NOT bite, because a live key survived: the issue path + // adopts it rather than refusing. That is the whole reason `partial` has to + // reach the page — the backend cannot pretend this was a revocation, and + // the visitor's next move is to press Replace again, not to wait for the + // 1st. + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + assert_eq!( + gateway.with(|s| s.create_calls), + 0, + "nothing new was minted" + ); +} + +/// Every enabled key raced away between the listing and the patch: `no_key`, +/// not a phantom "deactivated". +/// +/// Nothing was written, so there is no disabled record for the cap to read — +/// and the very next issue press finds the name empty and creates a key +/// outright. Answering `Done` here would have promised a wait the issue path +/// does not honour. +#[tokio::test] +async fn a_revocation_that_raced_away_is_no_key_not_a_phantom_record() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let gone = seed_attached(&gateway, 1_000); + // Listed, but no longer in the store: the patch 404s, which is the + // deletion race, not a failure. + gateway.with(|s| { + let key = s.keys.iter().find(|k| k.id == gone).unwrap().clone(); + s.deleted_keys.push(key); + s.keys.retain(|k| k.id != gone); + s.list_resurrects_deleted = true; + }); + let app = app_with_discord(&discord, &gateway); + + let reply = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!(reply.status, StatusCode::NOT_FOUND); + assert_eq!(reply.json()["code"], "no_key"); +} + +/// The revocation instant in the answer is the control plane's +/// `lastUpdatedDate`, read off the patch response — not this process's clock. +/// +/// The two are seconds apart in the ordinary case and invisible; across 00:00 +/// UTC on the 1st they fall in different quota periods, and the page would then +/// name a `next_eligible_at` a month before the issue round-trip would honour +/// it. Pinning the mock's stamp to a date of our choosing is the only way to +/// tell which of the two clocks the answer used. +#[tokio::test] +async fn the_revocation_instant_comes_from_the_control_plane_not_our_clock() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_attached(&gateway, 1_000); + // Last month's 3rd — a period that has already rolled, which our own clock + // could never produce for a revocation happening now. + let stamped = the_3rd_of(first_of_month_offset(-1)); + gateway.with(|s| s.disable_stamps_at = Some(stamped)); + let app = app_with_discord(&discord, &gateway); + + let reply = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!(reply.status, StatusCode::OK); + assert_eq!( + reply.json()["revoked_at"].as_str().unwrap(), + chrono::DateTime::from_timestamp(stamped as i64, 0) + .unwrap() + .to_rfc3339_opts(chrono::SecondsFormat::Secs, true) + ); + // And the cap agrees with it rather than with `now`: that period has + // rolled, so a key is due immediately. + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); +} + +/// A revoked record that cannot be deleted does not withhold the new key. +/// +/// The post-roll cleanup runs on EVERY issue press once the name holds nothing +/// but revoked keys, so propagating its first failure made one undeletable +/// record — an untagged exact-name key made by hand in the console, against a +/// tag-scoped `DELETE` — a permanent `?issue=failed` with no in-product +/// recovery. Logged and stepped over instead, exactly as the loser sweep does. +#[tokio::test] +async fn an_undeletable_revoked_record_does_not_block_the_re_issue() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let dead = seed_revoked(&gateway, the_3rd_of(first_of_month_offset(-1))); + gateway.with(|s| s.fail_delete_of = vec![dead.clone()]); + let app = app_with_discord(&discord, &gateway); + + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + gateway.with(|s| { + let live: Vec<_> = s.keys.iter().filter(|k| k.enabled).collect(); + assert_eq!(live.len(), 1, "the visitor got a working key"); + assert_ne!(live[0].id, dead); + assert!( + s.plan_keys + .contains(&(PLAN_ID.to_string(), live[0].id.clone())) + ); + assert!( + s.keys.iter().any(|k| k.id == dead && !k.enabled), + "the undeletable record is left for the next reconciliation" + ); + }); + // The new key is what the reveal hands out, not the stale record. + let revealed = reveal_via(&app).await; + assert_eq!(revealed.status, StatusCode::OK); + assert_ne!(revealed.json()["name"], serde_json::Value::Null); +} + +/// Revoking with no key is `404 no_key` and writes nothing. +#[tokio::test] +async fn revoking_with_no_key_is_no_key_and_writes_nothing() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let app = app_with_discord(&discord, &gateway); + + let reply = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!(reply.status, StatusCode::NOT_FOUND); + assert_eq!(reply.json()["code"], "no_key"); + assert_eq!(gateway.with(|s| s.ops.len()), 0); +} + +/// Unauthenticated is refused before AWS is touched; a `GET` on the path is +/// not a route. The `POST`-only shape plus `SameSite=Lax` is the CSRF guard. +#[tokio::test] +async fn revoke_needs_a_session_and_the_post_verb() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_attached(&gateway, 1_000); + let app = app_with_discord(&discord, &gateway); + + let anonymous = revoke(&app, None).await; + assert_eq!(anonymous.status, StatusCode::UNAUTHORIZED); + assert_eq!(anonymous.json()["code"], "not_signed_in"); + assert_eq!(anonymous.cache_control(), "no-store"); + + let get = call_path(app, "GET", REWORK_PATH, Some(&session_cookie(USER_ID))).await; + assert_eq!(get.status, StatusCode::METHOD_NOT_ALLOWED); + + assert_eq!(gateway.with(|s| s.list_calls), 0); + assert!(gateway.with(|s| s.keys[0].enabled)); +} + +/// A forged session cannot revoke anybody's key, and one user's session +/// cannot reach another user's key — the name is derived from the signed +/// `sub`, and only exact matches are touched. +#[tokio::test] +async fn a_session_can_only_revoke_its_own_key() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let mine = seed_attached(&gateway, 1_000); + let theirs = gateway.with(|s| s.seed("discord-999999999999999999-key", 1_000)); + // A prefix neighbour — `discord--key-old` — which `nameQuery` + // returns and the exact filter must drop. + let lookalike = gateway.with(|s| s.seed(&format!("{}-old", key_name()), 1_000)); + let app = app_with_discord(&discord, &gateway); + + assert_eq!( + revoke(&app, Some(&session_cookie(USER_ID))).await.status, + StatusCode::OK + ); + gateway.with(|s| { + let by_id = |id: &str| s.keys.iter().find(|k| k.id == id).unwrap().enabled; + assert!(!by_id(&mine)); + assert!(by_id(&theirs), "somebody else's key is untouched"); + assert!(by_id(&lookalike), "a console lookalike is untouched"); + }); + + let forged = format!( + "{}=not-a-real-session", + prices_api::portal::auth::cookies::SESSION_COOKIE + ); + let reply = revoke(&app, Some(&forged)).await; + assert_eq!(reply.status, StatusCode::UNAUTHORIZED); +} + +/// A control plane that refuses the disable is a `502`, and "revoked" is +/// never said of a key that still works. +#[tokio::test] +async fn a_failed_disable_is_a_502_not_a_false_revoked() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_attached(&gateway, 1_000); + gateway.with(|s| s.fail_disables = true); + let app = app_with_discord(&discord, &gateway); + + let reply = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!(reply.status, StatusCode::BAD_GATEWAY); + assert_eq!(reply.json()["code"], "key_unavailable"); + assert!(gateway.with(|s| s.keys[0].enabled)); + // And the reveal still hands the (un-revoked) key out. + assert_eq!(reveal_via(&app).await.status, StatusCode::OK); +} + +/// A successful revoke evicts the usage route's cached answer, so the next +/// dashboard load re-reads rather than serving a pre-revoke snapshot. +#[tokio::test] +async fn a_revoke_evicts_the_cached_usage() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let key = seed_attached(&gateway, 1_000); + gateway.with(|s| { + s.usage.insert(key.clone(), vec![vec![42, 99_958]]); + }); + let app = app_with_discord(&discord, &gateway); + let session = session_cookie(USER_ID); + + let before = call_path(app.clone(), "GET", USAGE_PATH, Some(&session)).await; + assert_eq!(before.json()["used"], 42); + assert_eq!(gateway.with(|s| s.usage_calls), 1); + + assert_eq!(revoke(&app, Some(&session)).await.status, StatusCode::OK); + + // Inside the 60s TTL, so only the eviction explains a second read. The + // counter is preserved across a disable (0180 item 8), so the numbers + // are the same — and honest. + let after = call_path(app, "GET", USAGE_PATH, Some(&session)).await; + assert_eq!(after.status, StatusCode::OK); + assert_eq!(after.json()["used"], 42); + assert_eq!(gateway.with(|s| s.usage_calls), 2); +} + +// --------------------------------------------------------------------------- +// The re-issue cap — the replacement waits for the 1st +// --------------------------------------------------------------------------- + +/// Revoke, then "Get my API key" in the same period: eligibility passes and +/// the issue is still refused — `?issue=capped&next_eligible_at=…` — with +/// nothing created and the revoked key left as the record. +#[tokio::test] +async fn an_issue_after_a_revoke_in_the_same_period_is_capped_with_the_date() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let key = seed_attached(&gateway, 1_000); + let app = app_with_discord(&discord, &gateway); + let (_, expected_date) = next_eligible_expected(); + + assert_eq!( + revoke(&app, Some(&session_cookie(USER_ID))).await.status, + StatusCode::OK + ); + + let reply = issue_round_trip(&app).await; + assert_eq!(reply.status, StatusCode::SEE_OTHER); + assert_eq!( + reply.location(), + format!("/api-tokens/?issue=capped&next_eligible_at={expected_date}") + ); + assert_eq!( + discord.member_calls(), + 1, + "eligibility ran and passed first" + ); + gateway.with(|s| { + assert_eq!(s.create_calls, 0); + assert_eq!(s.keys.len(), 1); + assert_eq!(s.keys[0].id, key); + assert!(!s.keys[0].enabled, "the revocation record stays"); + assert_eq!(s.deleted.len(), 0); + }); + // And the reveal keeps saying revoked, not "no key". + assert_eq!(reveal_via(&app).await.json()["code"], "key_revoked"); +} + +/// The worked example against the real calendar: revoked on the 3rd of THIS +/// month → capped until the 1st of next; revoked on the 3rd of LAST month → +/// the period has rolled, the revocation record is deleted, a new key is +/// created and attached, and the reveal hands the NEW one out. +#[tokio::test] +async fn revoked_on_the_3rd_refuses_until_the_1st_and_issues_once_it_has_passed() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let (expected_at, expected_date) = next_eligible_expected(); + + // This month's 3rd: capped. + let gateway = MockGateway::start().await; + seed_revoked(&gateway, the_3rd_of(first_of_month_offset(0))); + let app = app_with_discord(&discord, &gateway); + assert_eq!( + issue_round_trip(&app).await.location(), + format!("/api-tokens/?issue=capped&next_eligible_at={expected_date}") + ); + assert_eq!(gateway.with(|s| s.create_calls), 0); + let revealed = reveal_via(&app).await; + assert_eq!(revealed.json()["code"], "key_revoked"); + assert_eq!(revealed.json()["details"]["next_eligible_at"], expected_at); + + // Last month's 3rd: the 1st has passed. + let gateway = MockGateway::start().await; + let dead = seed_revoked(&gateway, the_3rd_of(first_of_month_offset(-1))); + let app = app_with_discord(&discord, &gateway); + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + gateway.with(|s| { + assert_eq!( + s.deleted, + vec![dead.clone()], + "the revocation record is gone" + ); + assert_eq!(s.keys.len(), 1); + assert_ne!(s.keys[0].id, dead); + assert!(s.keys[0].enabled); + assert!( + s.plan_keys + .contains(&(PLAN_ID.to_string(), s.keys[0].id.clone())) + ); + let new = s.keys[0].id.clone(); + assert_eq!( + s.ops, + vec![ + format!("delete:{dead}"), + format!("create:{new}"), + format!("attach:{new}") + ] + ); + }); + let revealed = reveal_via(&app).await; + assert_eq!(revealed.status, StatusCode::OK); + assert_eq!( + revealed.json()["key_id"], + gateway.with(|s| s.keys[0].id.clone()) + ); +} + +/// A revoked key from last month — whose replacement is therefore due — is +/// "no key" on the reveal until the owner presses the issue link: the dead +/// value is never revealed, and the page offers the round-trip. +#[tokio::test] +async fn a_revocation_whose_period_has_rolled_reveals_as_no_key() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_revoked(&gateway, the_3rd_of(first_of_month_offset(-1))); + let app = app_with_discord(&discord, &gateway); + + let revealed = reveal_via(&app).await; + assert_eq!(revealed.status, StatusCode::NOT_FOUND); + assert_eq!(revealed.json()["code"], "no_key"); + assert_eq!( + gateway.with(|s| s.ops.len()), + 0, + "the reveal still writes nothing" + ); +} + +/// An idempotent revoke of a key revoked in an EARLIER period does not tell +/// the visitor to wait another month: `next_eligible_at` comes from +/// `cap::decide`, like the reveal's and the issue path's, so a stale tab that +/// re-`POST`s cannot hide an issue link the round-trip would have honoured. +#[tokio::test] +async fn an_idempotent_revoke_after_the_period_rolled_says_a_key_is_due_now() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let revoked_at = the_3rd_of(first_of_month_offset(-1)); + seed_revoked(&gateway, revoked_at); + let app = app_with_discord(&discord, &gateway); + + let reply = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!(reply.status, StatusCode::OK); + let next = reply.json()["next_eligible_at"] + .as_str() + .unwrap() + .to_owned(); + let (next_month, _) = next_eligible_expected(); + assert_ne!( + next, next_month, + "the period has rolled; the answer must not name the next one" + ); + // "From now", so the page offers the issue link instead of a date. + let announced = chrono::DateTime::parse_from_rfc3339(&next).unwrap(); + assert!(announced.timestamp() as u64 <= now_secs() + 5); + // The recorded instant, not "now" and not the epoch. + assert_eq!( + reply.json()["revoked_at"], + serde_json::json!( + chrono::DateTime::::from_timestamp(revoked_at as i64, 0) + .unwrap() + .to_rfc3339_opts(chrono::SecondsFormat::Secs, true) + ) + ); + assert_eq!(gateway.with(|s| s.ops.len()), 0, "nothing was written"); +} + +/// An undated revocation record beside a dated one must not erase the date: +/// a `None` instant caps against a `next_eligible_at` recomputed from the +/// current period on every read, which rolls forward forever. +#[tokio::test] +async fn an_undated_duplicate_does_not_lock_the_owner_out_forever() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_revoked(&gateway, the_3rd_of(first_of_month_offset(-1))); + gateway.with(|s| { + let id = s.seed_revoked(&key_name(), 900, 0); + s.undate(&id); + s.plan_keys.push((PLAN_ID.to_string(), id)); + }); + let app = app_with_discord(&discord, &gateway); + + // Last month's revocation is the one that governs, so the replacement is + // due: the reveal says "no key" and the round-trip issues. + let revealed = reveal_via(&app).await; + assert_eq!(revealed.json()["code"], "no_key"); + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + assert_eq!(gateway.with(|s| s.create_calls), 1); +} + +/// The LATEST revocation governs: a duplicate revoked last month beside a key +/// revoked this month must not open the door this month's revocation closed. +#[tokio::test] +async fn the_latest_revocation_governs_the_cap() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_revoked(&gateway, the_3rd_of(first_of_month_offset(-1))); + seed_revoked(&gateway, the_3rd_of(first_of_month_offset(0))); + let app = app_with_discord(&discord, &gateway); + + assert!( + issue_round_trip(&app) + .await + .location() + .starts_with("/api-tokens/?issue=capped") + ); + assert_eq!(gateway.with(|s| s.create_calls), 0); + assert_eq!(gateway.with(|s| s.deleted.len()), 0); +} + +/// The cap refuses only after eligibility: a non-member who revoked is told +/// to rejoin, not to wait — and nothing is written either way. +#[tokio::test] +async fn membership_is_still_checked_before_the_cap() { + let discord = MockDiscord::start_with( + GRANTED_SCOPE, + None, + MemberReply::NotFound { code: 10_007 }, + mock_discord::USER_ID, + ) + .await; + let gateway = MockGateway::start().await; + seed_revoked(&gateway, now_secs()); + let app = app_with_discord(&discord, &gateway); + + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=not_member" + ); + assert_eq!(gateway.with(|s| s.list_calls), 0); +} + +/// A live key beside a revoked one (a console re-enable, a duplicate): the +/// live key is the current key — revealed, adopted — and the revoked record +/// is swept by the next issue like any duplicate. +#[tokio::test] +async fn a_live_key_beside_a_revoked_one_is_the_current_key() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let dead = seed_revoked(&gateway, now_secs()); + let live = seed_attached(&gateway, 2_000); + let app = app_with_discord(&discord, &gateway); + + assert_eq!(reveal_via(&app).await.json()["key_id"], live); + + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + gateway.with(|s| { + assert_eq!(s.deleted, vec![dead.clone()]); + assert_eq!(s.keys.len(), 1); + assert_eq!(s.keys[0].id, live); + assert_eq!(s.create_calls, 0); + }); +} + +/// Regression: a live key from last month is still simply adopted by the +/// issue — the cap exists only for revoked keys. +#[tokio::test] +async fn a_live_key_is_adopted_as_before_the_cap_existed() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let key = seed_attached(&gateway, 1_000); + let app = app_with_discord(&discord, &gateway); + + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + assert_eq!(gateway.with(|s| s.create_calls), 0); + assert_eq!(reveal_via(&app).await.json()["key_id"], key); +} + +/// The full cycle on one router: issue → revoke → issue refused → (period +/// rolls, simulated by back-dating the revocation) → issue creates the new +/// key and the old value is gone for good. +#[tokio::test] +async fn the_full_cycle_issue_revoke_wait_reissue() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let app = app_with_discord(&discord, &gateway); + let session = session_cookie(USER_ID); + + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + let first = gateway.with(|s| s.keys[0].clone()); + + assert_eq!(revoke(&app, Some(&session)).await.status, StatusCode::OK); + assert!( + issue_round_trip(&app) + .await + .location() + .starts_with("/api-tokens/?issue=capped") + ); + + // The 1st arrives. + gateway.with(|s| { + s.keys[0].last_updated_at = Some(the_3rd_of(first_of_month_offset(-1))); + }); + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + + gateway.with(|s| { + assert_eq!(s.keys.len(), 1); + assert_ne!(s.keys[0].id, first.id); + assert_ne!(s.keys[0].value, first.value); + assert!(s.deleted.contains(&first.id)); + }); + let revealed = reveal_via(&app).await; + assert_eq!(revealed.status, StatusCode::OK); + assert_ne!(revealed.json()["value"], first.value); +} + +/// Every revoke answer is uncacheable and sets no cookie. +#[tokio::test] +async fn every_revoke_answer_carries_no_store() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let app = app_with_discord(&discord, &gateway); + + for reply in [ + revoke(&app, None).await, + revoke(&app, Some(&session_cookie(USER_ID))).await, + ] { + assert_eq!(reply.cache_control(), "no-store"); + assert!(reply.headers.get(header::SET_COOKIE).is_none()); + } +} + +// --------------------------------------------------------------------------- +// Review findings (2026-08-21 audit) +// --------------------------------------------------------------------------- + +/// A revoke without the portal's own request marker — a cross-site form +/// `POST` that `SameSite=Lax` would let through once the portal and another +/// page share a registrable domain — is refused before the session is read, +/// and so is one that carries the marker but says `Sec-Fetch-Site: +/// cross-site`. Nothing is written either way. +#[tokio::test] +async fn a_revoke_without_the_same_origin_markers_is_refused_before_anything_is_read() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + seed_attached(&gateway, 1_000); + let app = app_with_discord(&discord, &gateway); + let session = session_cookie(USER_ID); + + for (label, headers) in [ + ("no marker", vec![]), + ("wrong marker", vec![("x-requested-with", "XMLHttpRequest")]), + ( + "marker but cross-site", + vec![PORTAL_REQUEST_HEADER, ("sec-fetch-site", "cross-site")], + ), + ( + "marker but same-site sibling host", + vec![PORTAL_REQUEST_HEADER, ("sec-fetch-site", "same-site")], + ), + ] { + let reply = + call_path_with(app.clone(), "POST", REWORK_PATH, Some(&session), &headers).await; + assert_eq!(reply.status, StatusCode::FORBIDDEN, "{label}"); + assert_eq!(reply.json()["code"], "cross_site_request", "{label}"); + assert_eq!(reply.cache_control(), "no-store", "{label}"); + } + assert_eq!(gateway.with(|s| s.list_calls), 0, "refused before AWS"); + assert!(gateway.with(|s| s.keys[0].enabled)); + + // A typed-URL / bookmark style request (`none`) and a browser that sends + // no fetch metadata at all are both fine WITH the marker. + for headers in [ + vec![PORTAL_REQUEST_HEADER, ("sec-fetch-site", "none")], + vec![PORTAL_REQUEST_HEADER], + ] { + let reply = + call_path_with(app.clone(), "POST", REWORK_PATH, Some(&session), &headers).await; + assert_eq!(reply.status, StatusCode::OK); + } +} + +/// The reveal and the issue path decide the cap from the SAME instant — the +/// latest revocation — so two revocation records from different months can +/// never make the page offer an issue the round-trip refuses. +#[tokio::test] +async fn the_reveal_and_the_issue_agree_on_the_cap_with_mixed_period_revocations() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + // Older record revoked last month, newer one revoked this month. + seed_revoked(&gateway, the_3rd_of(first_of_month_offset(-1))); + seed_revoked(&gateway, the_3rd_of(first_of_month_offset(0))); + let app = app_with_discord(&discord, &gateway); + let (expected_at, expected_date) = next_eligible_expected(); + + let revealed = reveal_via(&app).await; + assert_eq!(revealed.json()["code"], "key_revoked", "not no_key"); + assert_eq!(revealed.json()["details"]["next_eligible_at"], expected_at); + + assert_eq!( + issue_round_trip(&app).await.location(), + format!("/api-tokens/?issue=capped&next_eligible_at={expected_date}") + ); + + // And the revoke's idempotent answer names the same instant. + let again = revoke(&app, Some(&session_cookie(USER_ID))).await; + assert_eq!(again.json()["next_eligible_at"], expected_at); +} + +/// The usage section reads the same key the reveal does: a live key beside a +/// revoked record shows its OWN counter, and a revocation whose period has +/// rolled answers `no_key` like the reveal — never a record the next issue +/// will delete. +#[tokio::test] +async fn usage_follows_the_same_key_as_the_reveal() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let session = session_cookie(USER_ID); + + // Live beside revoked (the revoked one is OLDER, so a blind "earliest + // wins" would pick it). + let gateway = MockGateway::start().await; + let dead = seed_revoked(&gateway, now_secs()); + let live = seed_attached(&gateway, 2_000); + gateway.with(|s| { + s.usage.insert(dead.clone(), vec![vec![999, 99_001]]); + s.usage.insert(live.clone(), vec![vec![7, 99_993]]); + }); + let app = app_with_discord(&discord, &gateway); + let usage = call_path(app.clone(), "GET", USAGE_PATH, Some(&session)).await; + assert_eq!( + usage.json()["used"], + 7, + "{}", + String::from_utf8_lossy(&usage.body) + ); + assert_eq!( + gateway.with(|s| s.usage_queries.last().unwrap().0.clone()), + live + ); + + // Revoked this period: the counter is preserved and still shown. + let gateway = MockGateway::start().await; + let dead = seed_revoked(&gateway, now_secs()); + gateway.with(|s| { + s.usage.insert(dead.clone(), vec![vec![42, 99_958]]); + }); + let app = app_with_discord(&discord, &gateway); + let usage = call_path(app.clone(), "GET", USAGE_PATH, Some(&session)).await; + assert_eq!(usage.status, StatusCode::OK); + assert_eq!(usage.json()["used"], 42); + + // Revoked LAST period: the reveal says no_key; so does usage. + let gateway = MockGateway::start().await; + let dead = seed_revoked(&gateway, the_3rd_of(first_of_month_offset(-1))); + gateway.with(|s| { + s.usage.insert(dead.clone(), vec![vec![42, 99_958]]); + }); + let app = app_with_discord(&discord, &gateway); + assert_eq!(reveal_via(&app).await.json()["code"], "no_key"); + let usage = call_path(app, "GET", USAGE_PATH, Some(&session)).await; + assert_eq!(usage.status, StatusCode::NOT_FOUND); + assert_eq!(usage.json()["code"], "no_key"); +} + +/// Eventual consistency on the post-roll re-issue: the listing after the +/// create still shows the record just deleted. It must not be ranked — it +/// would win (earliest), its attach would 404, and the single retry would be +/// spent on a phantom, leaving the NEW key created and unattached. +#[tokio::test] +async fn a_stale_listing_after_the_roll_does_not_rank_the_deleted_record() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + let dead = seed_revoked(&gateway, the_3rd_of(first_of_month_offset(-1))); + // Every listing from now on includes whatever has been deleted — so the + // re-listing after the record is deleted and the new key created still + // shows the dead record, exactly as a lagging `GetApiKeys` would. + gateway.with(|s| s.list_resurrects_deleted = true); + let app = app_with_discord(&discord, &gateway); + + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + gateway.with(|s| { + assert_eq!(s.keys.len(), 1); + assert_ne!(s.keys[0].id, dead); + assert!(s.keys[0].enabled); + assert!( + s.plan_keys + .contains(&(PLAN_ID.to_string(), s.keys[0].id.clone())), + "the new key is attached — not orphaned by a phantom winner" + ); + assert_eq!(s.create_calls, 1); + }); +} + +/// `CreateApiKey` is never retried by the SDK: a create whose request landed +/// but whose response was lost makes ONE key, not two or three, and the +/// round-trip lands `failed` honestly rather than minting duplicates. +#[tokio::test] +async fn a_lost_create_response_is_not_retried_into_duplicates() { + let discord = MockDiscord::start(GRANTED_SCOPE, None).await; + let gateway = MockGateway::start().await; + gateway.with(|s| s.fail_next_create_after_creating = true); + let app = app_with_discord(&discord, &gateway); + + let reply = issue_round_trip(&app).await; + assert_eq!(reply.location(), "/api-tokens/?issue=failed"); + assert_eq!( + gateway.with(|s| s.create_calls), + 1, + "one create request, however the SDK would like to retry it" + ); + // The next press adopts the key that landed, as before. + assert_eq!( + issue_round_trip(&app).await.location(), + "/api-tokens/?issue=ok" + ); + assert_eq!(gateway.with(|s| s.create_calls), 1); +} diff --git a/web/portal/src/api/portal.ts b/web/portal/src/api/portal.ts index fe39d8f1..4d1dde4e 100644 --- a/web/portal/src/api/portal.ts +++ b/web/portal/src/api/portal.ts @@ -284,6 +284,118 @@ export const signInUrl = (): string => `${PORTAL_API}/auth/login`; */ export const issueUrl = (): string => `${PORTAL_API}/auth/login?action=issue`; +/** + * What `POST /api-tokens/api/key/rework` answers (task 0191): the revocation. + * + * `next_eligible_at` is when a new key can be issued under OUR period rule + * (not an AWS guarantee; see the backend's `portal/period.rs`) — the 1st of + * the next month for a revocation in this period, and *now* for an idempotent + * revoke of a key whose period has already rolled, so a stale tab cannot hide + * an issue link the round-trip would honour. + * + * `revoked_at` is when the key went off. Absent when the backend cannot date + * the revocation (no `lastUpdatedDate` on any record) — the page renders the + * revocation without an instant rather than inventing one. + */ +export interface PortalRevocation { + revoked: true; + next_eligible_at: string; + revoked_at?: string; + /** + * Task 0191: the backend disabled some keys under this name and failed on at + * least one other, so a duplicate may still answer on `/v1/`. Absent from the + * ordinary answer — read it as `false` when missing, and never render a plain + * "revoked" while it is `true`. + */ + partial?: boolean; +} + +/** + * `POST /api-tokens/api/key/rework` — "Replace my key": revoke the key NOW, + * issue nothing (task 0191). + * + * A `fetch`, not a navigation: unlike issuing, revoking needs no fresh Discord + * token — it is destructive to the visitor's own access and to nothing else, + * and a leaked key has to be killable while Discord is down. The session cookie + * rides on this same-origin `POST` — `SameSite=Lax` lets it, and `SameSite` + * alone is NOT the guard (it is site-scoped, so a sibling host's form `POST` + * would carry the cookie): the CSRF guard is the custom request header below, + * which a cross-origin page cannot send without a preflight this API never + * answers. + * + * The replacement is an ordinary `issueUrl()` round-trip — refused by the + * backend until the quota period of the revocation has rolled, which the page + * renders as the date rather than offering a link that would only be refused. + */ +export async function revokeKey(): Promise { + const url = `${PORTAL_API}/key/rework`; + let response: Response; + try { + response = await fetch(url, { + method: 'POST', + // The custom header is the CSRF guard: it makes this a non-simple + // request, which a cross-origin page cannot send without a CORS + // preflight this API never answers. The backend refuses a revoke + // without it (`PORTAL_REQUEST_HEADER` in `portal/keys/mod.rs`). + headers: { + accept: 'application/json', + 'x-requested-with': 'stellar-prices-portal', + }, + signal: AbortSignal.timeout(KEY_TIMEOUT_MS), + }); + } catch (error) { + if (isTimeout(error)) { + throw new PortalApiError( + `${url} did not answer within ${KEY_TIMEOUT_MS / 1000}s`, + ); + } + throw new PortalApiError(`${url} could not be reached`); + } + if (!response.ok) { + throw new PortalApiError( + failureMessage(url, response.status, await readEnvelope(response)), + response.status, + ); + } + let body: { + revoked?: unknown; + next_eligible_at?: unknown; + revoked_at?: unknown; + partial?: unknown; + }; + try { + body = (await response.json()) as typeof body; + } catch { + throw new PortalApiError( + `${url} answered ${response.status}, not JSON`, + response.status, + ); + } + if ( + body.revoked !== true || + typeof body.next_eligible_at !== 'string' || + (body.revoked_at !== undefined && + body.revoked_at !== null && + typeof body.revoked_at !== 'string') + ) { + // A `200` that does not say `revoked: true` is not a shape this client + // knows; rendering "revoked" on it would be the one false statement this + // dialog must never make. + throw new PortalApiError(`${url} answered 200 without revoked: true`, 200); + } + return { + revoked: true, + next_eligible_at: body.next_eligible_at, + revoked_at: + typeof body.revoked_at === 'string' ? body.revoked_at : undefined, + // Absent on the ordinary answer (the backend omits it when false), and + // anything other than a literal `true` is read as "not partial": this flag + // only ever ADDS a warning, so a malformed value must not be able to + // manufacture one. + partial: body.partial === true, + }; +} + /** * `POST /api-tokens/api/auth/logout` — clear the session. * @@ -419,7 +531,25 @@ export async function fetchUsage(): Promise { } /** - * Reveal the key this account already has, or learn there is none. + * What the key route says when the owner revoked their key (task 0191): no + * usable key, and no issue offered until `next_eligible_at`. + */ +export interface PortalKeyRevoked { + revoked: true; + next_eligible_at: string; + revoked_at?: string; + /** + * Carried over from a revoke that only partly succeeded, in this page load + * (see `PortalRevocation`). The reveal never sets it: a later `GET /key` + * cannot tell a duplicate that survived a failed disable from one that was + * never there, and it does not need to — it lists what is enabled. + */ + partial?: boolean; +} + +/** + * Reveal the key this account already has, or learn there is none — or that + * it was revoked and when a new one can be issued. * * `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), @@ -433,7 +563,7 @@ export async function fetchUsage(): Promise { * 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 fetchKey(): Promise { +export async function fetchKey(): Promise { const url = `${PORTAL_API}/key`; let response: Response; try { @@ -451,10 +581,30 @@ export async function fetchKey(): Promise { } if (response.status === 404) { try { - const body = (await response.json()) as { code?: string }; + const body = (await response.json()) as { + code?: string; + details?: { next_eligible_at?: unknown; revoked_at?: unknown }; + }; if (body.code === 'no_key') { return null; } + // The owner revoked it (task 0191). Distinct from `no_key` because the + // page must NOT offer the issue round-trip: the backend would refuse it + // until the date, and a link that only ever refuses is worse than the + // date itself. + if ( + body.code === 'key_revoked' && + typeof body.details?.next_eligible_at === 'string' + ) { + return { + revoked: true, + next_eligible_at: body.details.next_eligible_at, + revoked_at: + typeof body.details.revoked_at === 'string' + ? body.details.revoked_at + : undefined, + }; + } } catch { // Not JSON — the gate's empty 404, or something else entirely. } diff --git a/web/portal/src/app/app.spec.tsx b/web/portal/src/app/app.spec.tsx index fa3eef1f..13f174e7 100644 --- a/web/portal/src/app/app.spec.tsx +++ b/web/portal/src/app/app.spec.tsx @@ -71,6 +71,8 @@ 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'; +/** The revocation — "Replace my key" (task 0191). */ +const REWORK_URL = '/api-tokens/api/key/rework'; /** Where both "get my API key" and every retry link point (task 0189). */ const ISSUE_HREF = '/api-tokens/api/auth/login?action=issue'; @@ -1631,3 +1633,497 @@ describe('usage against quota', () => { expect(document.body.textContent).not.toContain('answered 401'); }); }); + +describe('replace my key', () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + afterEach(() => { + vi.unstubAllGlobals(); + }); + + const KEY = { + key_id: 'abc123', + name: 'discord-308994132968210433-key', + value: 'aBcDeF0123456789aBcDeF0123456789aBcDeF01', + }; + + const REVOKED = { + revoked: true, + next_eligible_at: '2026-09-01T00:00:00Z', + revoked_at: '2026-08-21T12:00:00Z', + }; + + /** The reveal's answer once the key is revoked (task 0191). */ + const keyRevoked = () => ({ + ok: false, + status: 404, + json: async () => ({ + code: 'key_revoked', + message: 'your API key was revoked', + details: { + next_eligible_at: '2026-09-01T00:00:00Z', + revoked_at: '2026-08-21T12:00:00Z', + }, + }), + }); + + const signedIn = ( + revoke: () => Partial & { json?: () => unknown } = () => ({ + json: async () => REVOKED, + }), + key: () => Partial & { json?: () => unknown } = () => ({ + json: async () => KEY, + }), + ) => + stubRoutes({ + [CONFIG_URL]: openConfig, + [ME_URL]: () => ({ + json: async () => ({ + authenticated: true, + user_id: '308994132968210433', + username: 'adam', + }), + }), + [KEY_URL]: key, + [USAGE_URL]: usageNoKey, + [REWORK_URL]: revoke, + }); + + const renderApp = (entry = '/') => + render( + + + + , + ); + + const openDialog = async () => { + fireEvent.click(await screen.findByTestId('replace-key-open')); + return screen.findByTestId('replace-key-dialog'); + }; + + const revokeCalls = (fetchMock: ReturnType) => + fetchMock.mock.calls.filter(([url]) => url === REWORK_URL); + + /** + * The control lives beside the key and nowhere else, and pressing it opens + * a confirmation: nothing is sent to the backend until the armed confirm. + */ + it('offers replacement only beside an existing key, and opening the dialog sends nothing', async () => { + const fetchMock = signedIn(); + renderApp(); + + await openDialog(); + expect(revokeCalls(fetchMock)).toHaveLength(0); + }); + + it('does not offer replacement to a visitor with no key', async () => { + signedIn(undefined, keyNoKey); + renderApp(); + + await screen.findByRole('link', { name: /get my api key/i }); + expect(screen.queryByTestId('replace-key-open')).toBeNull(); + }); + + /** + * The wording is the acceptance criterion: the key is deactivated + * immediately, and no new key is issued until the next period. + */ + it('states that the key is deactivated, names the propagation window, and says no new key is issued until the next period', async () => { + signedIn(); + renderApp(); + + await openDialog(); + const warning = await screen.findByTestId('replace-key-warning'); + expect(warning.textContent).toMatch(/deactivates the current one/i); + // The 0192 criterion: no claim of immediacy the data plane does not + // have. ~25 s measured (0180 item 8); the copy says half a minute and + // tells the visitor to treat the key as live until then. + expect(warning.textContent).toMatch(/within about half a minute/i); + expect(warning.textContent).toMatch(/treat it as live until then/i); + expect(warning.textContent).not.toMatch(/immediately/i); + expect(warning.textContent).toMatch(/will break/i); + expect(warning.textContent).toMatch(/no new key is issued now/i); + expect(warning.textContent).toMatch(/next quota period/i); + }); + + /** Confirm is disabled until `delete-key` is typed — exactly that phrase. */ + it('keeps confirm disabled until the visitor types delete-key', async () => { + const fetchMock = signedIn(); + renderApp(); + + await openDialog(); + const confirm = (await screen.findByTestId( + 'replace-key-confirm', + )) as HTMLButtonElement; + const phrase = screen.getByTestId('replace-key-phrase') as HTMLInputElement; + expect(confirm.disabled).toBe(true); + + for (const typed of ['delete', 'delete key', 'DELETE-KEY', 'delete-key ']) { + fireEvent.change(phrase, { target: { value: typed } }); + expect(confirm.disabled, typed).toBe(true); + fireEvent.click(confirm); + } + expect(revokeCalls(fetchMock)).toHaveLength(0); + + fireEvent.change(phrase, { target: { value: 'delete-key' } }); + expect(confirm.disabled).toBe(false); + }); + + /** + * One armed press is one `POST`; the button is disabled again on submit so + * a double-click cannot fire two. On success the panel shows the revoked + * state with the backend's date, and the old value is gone from the page. + */ + it('revokes with one POST, cannot double-fire, and renders the revoked state with the date', async () => { + const fetchMock = signedIn(); + renderApp(); + + await openDialog(); + const confirm = (await screen.findByTestId( + 'replace-key-confirm', + )) as HTMLButtonElement; + fireEvent.change(screen.getByTestId('replace-key-phrase'), { + target: { value: 'delete-key' }, + }); + + fireEvent.click(confirm); + expect(confirm.disabled).toBe(true); + fireEvent.click(confirm); + fireEvent.click(confirm); + + const revoked = await screen.findByTestId('key-revoked'); + expect(revoked.textContent).toMatch(/deactivated/i); + // The revocation instant from the API, in UTC, is the anchor for the + // propagation window — not "now", not the viewer's zone. + expect(screen.getByTestId('revoked-at').textContent).toBe( + '21 August 2026, 12:00 UTC', + ); + expect(revoked.textContent).toMatch(/within about half a minute/i); + expect(revoked.textContent).toMatch(/1 September 2026/); + expect(revoked.textContent).toMatch(/do not have a working key/i); + expect(revokeCalls(fetchMock)).toHaveLength(1); + expect((revokeCalls(fetchMock)[0][1] as RequestInit).method).toBe('POST'); + // The CSRF marker: a non-simple header the backend requires and a + // cross-origin page cannot send without a preflight. + expect( + ( + (revokeCalls(fetchMock)[0][1] as RequestInit).headers as Record< + string, + string + > + )['x-requested-with'], + ).toBe('stellar-prices-portal'); + expect(revokeCalls(fetchMock)[0][0].startsWith('http')).toBe(false); + expect(screen.queryByTestId('api-key')).toBeNull(); + expect(document.body.textContent).not.toContain(KEY.value); + // And no issue link while the date is ahead. + expect(screen.queryByRole('link', { name: /get my api key/i })).toBeNull(); + }); + + /** + * A partial revocation warns instead of reading as a clean one. + * + * The backend answers `200 partial` when it disabled some keys under the + * name and failed on another — the visitor's own key is off, so a `502` + * would be false, but a duplicate may still answer on `/v1/` and a plain + * "revoked" would be false too. The warning is what tells them the next move + * is to press Replace again, not to wait for the 1st. + */ + it('warns on a partial revocation instead of calling it revoked', async () => { + signedIn(() => ({ + json: async () => ({ ...REVOKED, partial: true }), + })); + renderApp(); + + await openDialog(); + fireEvent.change(screen.getByTestId('replace-key-phrase'), { + target: { value: 'delete-key' }, + }); + fireEvent.click(screen.getByTestId('replace-key-confirm')); + + const warning = await screen.findByTestId('revoke-partial'); + expect(warning.textContent).toMatch(/could not be deactivated/i); + expect(warning.textContent).toMatch(/may still work/i); + // The dates still render — the disables that landed are real. + expect(screen.getByTestId('revoked-at')).toBeTruthy(); + // But NOT the cap sentences: the surviving duplicate is a working key, + // and the issue path adopts it rather than refusing, so both "you do not + // have a working key" and the next-eligible date would be false here. + const revoked = screen.getByTestId('key-revoked'); + expect(revoked.textContent).not.toMatch(/do not have a working key/i); + expect(revoked.textContent).not.toMatch(/1 September 2026/); + expect(screen.queryByRole('link', { name: /get my api key/i })).toBeNull(); + }); + + /** The ordinary answer carries no flag, and renders no warning. */ + it('renders no partial warning on a clean revocation', async () => { + signedIn(); + renderApp(); + + await openDialog(); + fireEvent.change(screen.getByTestId('replace-key-phrase'), { + target: { value: 'delete-key' }, + }); + fireEvent.click(screen.getByTestId('replace-key-confirm')); + + await screen.findByTestId('key-revoked'); + expect(screen.queryByTestId('revoke-partial')).toBeNull(); + }); + + /** + * A failed revoke says so and keeps the key — "revoked" is never false. + * + * And it does not make the opposite claim either: a `502` can be a refusal + * with nothing written or a lost response on a patch that landed, so the copy + * says the deactivation was not CONFIRMED rather than that the key is still + * active. + */ + it('reports a failed revoke without claiming the key is still active', async () => { + signedIn(() => ({ + ok: false, + status: 502, + json: async () => ({ + code: 'key_unavailable', + message: 'could not reach the API key service; try again', + }), + })); + renderApp(); + + await openDialog(); + fireEvent.change(screen.getByTestId('replace-key-phrase'), { + target: { value: 'delete-key' }, + }); + fireEvent.click(screen.getByTestId('replace-key-confirm')); + + const failed = await screen.findByTestId('replace-key-failed'); + expect(failed.textContent).toMatch(/could not confirm the deactivation/i); + expect(failed.textContent).not.toMatch(/it is still active/i); + expect(failed.textContent).toMatch(/could not reach the API key service/i); + expect(screen.queryByTestId('key-revoked')).toBeNull(); + expect(screen.getByTestId('api-key')).toBeTruthy(); + // The dialog stays, re-armed, for a retry. + expect( + (screen.getByTestId('replace-key-confirm') as HTMLButtonElement).disabled, + ).toBe(false); + }); + + it('closes without sending anything', async () => { + const fetchMock = signedIn(); + renderApp(); + + await openDialog(); + fireEvent.click(screen.getByRole('button', { name: /cancel/i })); + expect(screen.queryByTestId('replace-key-dialog')).toBeNull(); + expect(screen.getByTestId('replace-key-open')).toBeTruthy(); + expect(revokeCalls(fetchMock)).toHaveLength(0); + }); + + // ------------------------------------------------------------------------- + // The revoked state on load, and the capped issue landing + // ------------------------------------------------------------------------- + + /** + * A reload after a revoke: the reveal answers `key_revoked`, the page + * renders the date and never the value, and offers no issue link while the + * date is ahead. + */ + it('renders a revoked key as revoked with the date, not as "no key"', async () => { + signedIn(undefined, keyRevoked); + renderApp(); + + const revoked = await screen.findByTestId('key-revoked'); + expect(revoked.textContent).toMatch(/1 September 2026/); + expect(screen.queryByRole('link', { name: /get my api key/i })).toBeNull(); + expect(screen.queryByTestId('replace-key-open')).toBeNull(); + // The key section's keyless copy (and its issue link) must not render; + // the usage section beside it may still say "no key" — that is its own + // endpoint's answer, stubbed here, not the key section's. + expect(screen.queryByText(/one key, on the free plan/i)).toBeNull(); + }); + + /** + * After an in-page revoke the usage section stops describing the key as + * new, re-asks the backend (which evicted its cache), and — when AWS has no + * row — says the key was deactivated rather than "issue one above". + */ + it('tells the usage section about an in-page revoke', async () => { + const fetchMock = signedIn(); + renderApp(); + + // Before: a key on screen, usage says no key → "your key is new". + await screen.findByTestId('api-key'); + expect(await screen.findByText(/your key is new/i)).toBeTruthy(); + const usageCallsBefore = fetchMock.mock.calls.filter( + ([url]) => url === USAGE_URL, + ).length; + + await openDialog(); + fireEvent.change(screen.getByTestId('replace-key-phrase'), { + target: { value: 'delete-key' }, + }); + fireEvent.click(screen.getByTestId('replace-key-confirm')); + await screen.findByTestId('key-revoked'); + + expect(await screen.findByTestId('usage-after-revoke')).toBeTruthy(); + expect(screen.queryByText(/your key is new/i)).toBeNull(); + expect(document.body.textContent).not.toMatch(/issue one above/i); + await waitFor(() => + expect( + fetchMock.mock.calls.filter(([url]) => url === USAGE_URL).length, + ).toBe(usageCallsBefore + 1), + ); + }); + + /** + * A malformed `next_eligible_at` must NOT unlock the issue link — the safe + * direction for garbage is to keep waiting (the server would only refuse). + */ + it('keeps the issue link hidden when the revoked date is unparseable', async () => { + signedIn(undefined, () => ({ + ok: false, + status: 404, + json: async () => ({ + code: 'key_revoked', + details: { next_eligible_at: 'not-a-date' }, + }), + })); + renderApp(); + + await screen.findByTestId('key-revoked'); + expect(screen.queryByRole('link', { name: /get my api key/i })).toBeNull(); + expect(document.body.textContent).toMatch( + /start of the next quota period/i, + ); + }); + + /** Once the date has passed, the issue link comes back. */ + it('offers the issue round-trip once the revocation period has passed', async () => { + signedIn(undefined, () => ({ + ok: false, + status: 404, + json: async () => ({ + code: 'key_revoked', + details: { next_eligible_at: '2020-01-01T00:00:00Z' }, + }), + })); + renderApp(); + + await screen.findByTestId('key-revoked'); + const link = await screen.findByRole('link', { name: /get my api key/i }); + expect(link.getAttribute('href')).toBe(ISSUE_HREF); + }); + + /** + * A revocation the backend cannot date renders WITHOUT an instant — never + * "deactivated on just now", and never the next-eligible phrase presented + * as the revocation instant. + */ + it('renders an undated revocation without inventing an instant', async () => { + signedIn(undefined, () => ({ + ok: false, + status: 404, + json: async () => ({ + code: 'key_revoked', + details: { next_eligible_at: '2026-09-01T00:00:00Z' }, + }), + })); + renderApp(); + + const revoked = await screen.findByTestId('key-revoked'); + expect(revoked.textContent).toMatch(/deactivated/i); + expect(screen.queryByTestId('revoked-at')).toBeNull(); + expect(revoked.textContent).not.toMatch(/just now/i); + expect(revoked.textContent).not.toMatch( + /deactivated on the start of the next quota period/i, + ); + // The date it CAN be re-issued is still named — that one is known. + expect(revoked.textContent).toMatch(/1 September 2026/); + }); + + /** + * The propagation window is a statement about now. Rendered on a page load + * days after the revocation (the reveal path), it must not tell the owner + * of a long-dead key to keep treating it as live. + */ + it('states the propagation window in the past tense for an old revocation', async () => { + signedIn(undefined, () => ({ + ok: false, + status: 404, + json: async () => ({ + code: 'key_revoked', + details: { + next_eligible_at: '2026-09-01T00:00:00Z', + revoked_at: '2026-08-01T09:00:00Z', + }, + }), + })); + renderApp(); + + const revoked = await screen.findByTestId('key-revoked'); + expect(screen.getByTestId('revoked-at').textContent).toBe( + '1 August 2026, 09:00 UTC', + ); + expect(revoked.textContent).toMatch(/stopped working/i); + expect(revoked.textContent).not.toMatch(/treat it as live/i); + // The measured window is still named — only the tense changed. + expect(revoked.textContent).toMatch(/within about half a minute/i); + }); + + /** + * An idempotent revoke whose period has already rolled answers with a + * next-eligible instant of *now*, and the page offers the round-trip + * rather than a date a month out. + */ + it('offers the issue link when the revoke answers a next eligible date already passed', async () => { + signedIn(() => ({ + json: async () => ({ + revoked: true, + next_eligible_at: '2020-01-01T00:00:00Z', + revoked_at: '2019-12-03T12:00:00Z', + }), + })); + renderApp(); + + await openDialog(); + fireEvent.change(screen.getByTestId('replace-key-phrase'), { + target: { value: 'delete-key' }, + }); + fireEvent.click(screen.getByTestId('replace-key-confirm')); + + await screen.findByTestId('key-revoked'); + const link = await screen.findByRole('link', { name: /get my api key/i }); + expect(link.getAttribute('href')).toBe(ISSUE_HREF); + }); + + /** The capped issue landing renders the date, sanitised, and no link. */ + it('renders a capped issue with the next eligible date', async () => { + signedIn(undefined, keyRevoked); + renderApp('/?issue=capped&next_eligible_at=2026-09-01'); + + const capped = await screen.findByTestId('issue-capped'); + expect(capped.textContent).toMatch(/1 September 2026/); + expect(capped.textContent).toMatch(/do not have a working key/i); + expect(capped.querySelector('a')).toBeNull(); + }); + + it('sanitises a nonsense next_eligible_at instead of rendering it', async () => { + signedIn(undefined, keyRevoked); + renderApp('/?issue=capped&next_eligible_at=