diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bdf8b6..110d252 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,12 +16,15 @@ User-visible changes to `voltage`, newest first. Each pull request that changes - Guides for wallets, webhooks, troubleshooting, and getting started, and references for global flags, environment variables, and configuration. - A command that takes a resource ID as an argument also accepts `--id ID`, so the flag that `quotes create` and `payments send` take works there too. It takes one of the two, and its usage line shows both, as in `voltage quotes get >`. - In human output, an accepted `payments receive` or `quotes create` names the command that shows the invoice, address, or quote, with the organization, environment, and account that the command used. +- A 403 to `VOLTAGE_API_KEY` says that the key came from that variable and where the organization came from, and suggests `--account NAME` or unsetting the variable. `--json` reports both as `error.detail.credential_sources`. - The wallet picker shows a wallet's currency when its balances report one, and typing a currency filters on it. A wallet with no balance yet shows no currency. ### Changed +- `voltage login` under a saved name replaces that login when it is the same user at the same auth URL, and revokes the replaced session. A different user is still refused. With `--account NAME`, a name saved for an API key or for another auth URL is refused before the browser step. An expired or revoked login's hint is now only `voltage login --account NAME`. A saved login whose secret cannot be read is refused, also before the browser step, with a hint that names `voltage logout --local --account NAME`, and `logout --local` removes an account whose secret is already gone or cannot be removed, with a warning for the latter. Without `--credential-store`, a replaced login keeps its credential store, and a new login uses the keychain. `voltage login --json` reports `replaced: true` when the login replaced a saved one. When a replacement cannot be saved, the replaced login stays saved, and the error says so if it could not be restored. - Human tables fit the terminal width, and all API text is scrubbed of terminal control characters. - Each item of a string list gets its own line in human output. +- A saved login the auth service no longer accepts reports `credential_problem: login_expired` in `--json` `error.detail`. `voltage context` cannot detect it, because it makes no network request. - Errors name the next command: missing scope names the command that lists IDs, credential and profile errors list the saved names, HTTP 401 and 403 point to `voltage auth status` and `voltage context`, and `quote_required` points to `voltage quotes create`. - An API rejection's error leads with the API's reason, and human output shows the rest of the API error as field/value lines instead of JSON. - `-h` shows usage and options; `--help` adds notes, examples, and the guide link. @@ -30,6 +33,7 @@ User-visible changes to `voltage`, newest first. Each pull request that changes ### Fixed +- TLS certificate checks reject the distrusted emSign intermediates: `rustls-webpki` 0.103.16 and `webpki-root-certs` 1.0.10 (RUSTSEC-2026-0382, RUSTSEC-2026-0383). - Help and README examples for changes include `--execute`, so copying one sends it. - `voltage auth import-key` removes the saved key again when it cannot save the configuration, as `voltage login` does, and reports a name in use with the same message. diff --git a/Cargo.lock b/Cargo.lock index 7535b36..4d35fba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1912,9 +1912,9 @@ checksum = "f87165f0995f63a9fbeea62b64d10b4d9d8e78ec6d7d51fb2125fda7bb36788f" [[package]] name = "rustls-webpki" -version = "0.103.15" +version = "0.103.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f3c3cf1d8b1e7d4927e2d154c3fcb02979afb9939629c62cd9048d4f07b60ac2" +checksum = "4443b1b6d39d7c663139942cc4558262562d2c84123bb6282ef177009ca8b281" dependencies = [ "ring", "rustls-pki-types", @@ -2701,9 +2701,9 @@ dependencies = [ [[package]] name = "webpki-root-certs" -version = "1.0.9" +version = "1.0.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b96554aa2acc8ccdb7e1c9a58a7a68dd5d13bccc69cd124cb09406db612a1c9b" +checksum = "58ec80e73fd8baf57bf0d9615554c75348bda990d6d5bbeab964dd068319fecb" dependencies = [ "rustls-pki-types", ] diff --git a/docs/guides/login-and-profiles.md b/docs/guides/login-and-profiles.md index 74a198a..cf09c1c 100644 --- a/docs/guides/login-and-profiles.md +++ b/docs/guides/login-and-profiles.md @@ -15,6 +15,8 @@ voltage logout --local # removes local credentials only A login grants the permissions your account already has. Profiles do not restrict them. +Signing in again under a saved name replaces that login when it is the same user at the same auth URL, and revokes the replaced session. A different user, or a name used by an imported API key, is refused. With `--account NAME`, an API key or a login at another auth URL is refused before the browser step. Without `--credential-store`, the new login stays in the replaced login's credential store; a new name uses the keychain. With `--json`, the output has `replaced: true` when the login replaced a saved one. + ## API keys For automation, use an environment API key. Import one so it never lands in shell history, or pass it through `VOLTAGE_API_KEY`: @@ -69,7 +71,7 @@ voltage profiles delete staging ## See the effective scope -`voltage context` shows the effective profile, account, organization, environments, wallet, API and auth URLs, and configuration directory, and where each came from (`flag`, `profile`, `environment`, `default`, or `unset`). It reports why commands would reject the credential selection as `credential_problem` (`empty_api_key`, `no_credential`, `multiple_credentials`, `unknown_credential`, `bound_elsewhere` for an API key used outside its organization or environment, or `auth_url_mismatch` for a browser login used with a different `--auth-url`), using the same selection commands use. It also lists `VOLTAGE_*` variables that are set but ignored because a flag or the selected profile takes precedence. It reads no saved secret, makes no network request, and creates no files: +`voltage context` shows the effective profile, account, organization, environments, wallet, API and auth URLs, and configuration directory, and where each came from (`flag`, `profile`, `environment`, `default`, or `unset`). It reports why commands would reject the credential selection as `credential_problem` (`empty_api_key`, `no_credential`, `multiple_credentials`, `unknown_credential`, `bound_elsewhere` for an API key used outside its organization or environment, or `auth_url_mismatch` for a browser login used with a different `--auth-url`), using the same selection commands use. It cannot report `login_expired`, a login the auth service no longer accepts: that needs a network request, so the next command that uses the login reports it in `error.detail.credential_problem`. It also lists `VOLTAGE_*` variables that are set but ignored because a flag or the selected profile takes precedence. It reads no saved secret, makes no network request, and creates no files: ```sh voltage context --profile staging diff --git a/docs/guides/troubleshooting.md b/docs/guides/troubleshooting.md index 1e013b9..038342b 100644 --- a/docs/guides/troubleshooting.md +++ b/docs/guides/troubleshooting.md @@ -58,16 +58,17 @@ An imported API key belongs to one organization and environment, and the command `error: Saved credentials cannot be refreshed at a different auth URL`. A browser login refreshes only at the auth URL it was saved with. Omit `--auth-url` to use that URL, which `voltage context` shows. To move the login to the new URL, run `voltage logout --account NAME`, then `voltage login --account NAME --auth-url URL`. -### The saved login has expired or was revoked +### `login_expired` -Replace the saved login with the two commands in the hint. `voltage login` refuses to overwrite a saved credential, so remove the stale one first: +`error: The saved login for NAME has expired or was revoked`. The auth service rejected the login's refresh or token exchange with `invalid_grant`. `--json` reports `"credential_problem": "login_expired"` in `error.detail`. Sign in again with the command in the hint: ```sh -voltage logout --local --account NAME voltage login --account NAME ``` -`--local` skips server revocation, which a revoked or expired login cannot complete. Keep the same `NAME` so profiles that select it keep working. +When you sign in as the same user, the new login replaces the saved one, and the CLI revokes the replaced session. Keep the same `NAME` so profiles that select it keep working. If you sign in as a different user, `voltage login` refuses the name; run `voltage logout --local --account NAME` first, or choose another name. It refuses the name the same way when the saved login's secret cannot be read, for example from a locked OS credential store or a damaged credential file. `voltage logout --local --account NAME` removes that account even when its secret cannot be removed, and warns you to remove the secret yourself. + +`voltage context` cannot report this problem. It reads no saved secret and makes no network request, so it cannot see that the auth service rejects the login. An expired access token alone is not this problem, because the next command refreshes it. `voltage auth status` shows whether the access token has expired, and the next command that uses the login reports `login_expired`. ### needs a browser login @@ -75,7 +76,7 @@ voltage login --account NAME ### HTTP 401 -The API did not accept the credential. `voltage auth status` shows the selected one and whether it has expired. For a saved login, replace it as described in [The saved login has expired or was revoked](#the-saved-login-has-expired-or-was-revoked). +The API did not accept the credential. `voltage auth status` shows the selected one and whether it has expired. For a saved login, replace it as described in [`login_expired`](#login_expired). A checkout command presents its session or stream token, not the account credential. Check the token in `--token-file`, `VOLTAGE_CHECKOUT_TOKEN`, or `VOLTAGE_STREAM_TOKEN`. @@ -83,7 +84,13 @@ A checkout command presents its session or stream token, not the account credent The credential is valid but cannot reach the selected organization or environment. `voltage context` shows which scope and credential the command used. -If `voltage context` shows the credential's location as `environment`, the command used `VOLTAGE_API_KEY` instead of a saved login. Check that the key belongs to the selected organization, or unset `VOLTAGE_API_KEY` (or pass `--account NAME`) to use a saved login. +When the command used `VOLTAGE_API_KEY` instead of a saved login, the hint says so, and names where the organization came from: + +```text +hint: The API key came from VOLTAGE_API_KEY and the organization from VOLTAGE_ORGANIZATION_ID. Check that the key can reach the organization, environment, and wallet in use, which `voltage context` shows, or pass --account NAME or unset VOLTAGE_API_KEY to use a saved login. +``` + +With `--json`, `error.detail.credential_sources` holds the same sources, as `{"credential": "environment", "organization": "environment"}`. The credential is always `environment`. The organization is `environment` for `VOLTAGE_ORGANIZATION_ID`, `flag` for `--org`, and `unset` when neither supplies it. ### Cannot read OS credential store diff --git a/docs/reference/exit-codes.md b/docs/reference/exit-codes.md index 1631d45..8a307e9 100644 --- a/docs/reference/exit-codes.md +++ b/docs/reference/exit-codes.md @@ -13,4 +13,4 @@ After exit code 4 for a payment, check its ID before you retry; see [payment IDs](../concepts/safety-model.md#payment-ids-and-the-request-journal). After exit code 5, keep watching with `voltage payments get PAYMENT_ID --profile prod --wait completed`. -Many errors for exit codes 2 and 3 carry a `hint:` (or `error.hint` with `--json`) that names the next command: the discovery command for a missing `--org`, `--env`, or `--wallet`, the saved credential and profile names for an unknown or ambiguous selection, `voltage logout --local --account NAME` and then `voltage login --account NAME` for an expired or revoked login, `voltage logout --account NAME` and then `voltage login --account NAME --auth-url URL` to move a browser login to a different auth URL, `voltage auth status` for a rejected account credential (HTTP 401), and `voltage context` for an account credential that cannot reach the selected scope (HTTP 403). A rejected checkout token points to `--token-file` or the variable that supplies it, never to the account credential. A payment rejected with `quote_required`, or a USD payment without `--quote`, points to `voltage quotes create`, and a payment rejected with `invalid_currency` for a quoted amount says how to quote the payment's exact amount. +Many errors for exit codes 2 and 3 carry a `hint:` (or `error.hint` with `--json`) that names the next command: the discovery command for a missing `--org`, `--env`, or `--wallet`, the saved credential and profile names for an unknown or ambiguous selection, `voltage login --account NAME` for an expired or revoked login, `voltage logout --account NAME` and then `voltage login --account NAME --auth-url URL` to move a browser login to a different auth URL, `voltage auth status` for a rejected account credential (HTTP 401), and `voltage context` for an account credential that cannot reach the selected scope (HTTP 403). A 403 to `VOLTAGE_API_KEY` instead names that variable and where the organization came from, and suggests `--account NAME` or unsetting the variable; `--json` reports both sources as `error.detail.credential_sources`. A rejected checkout token points to `--token-file` or the variable that supplies it, never to the account credential. A payment rejected with `quote_required`, or a USD payment without `--quote`, points to `voltage quotes create`, and a payment rejected with `invalid_currency` for a quoted amount says how to quote the payment's exact amount. diff --git a/src/api.rs b/src/api.rs index 76f1fa7..0ab280c 100644 --- a/src/api.rs +++ b/src/api.rs @@ -11,11 +11,11 @@ use crate::{ backoff::{Backoff, POLL_CEILING}, cli::{ApiInvocation, GlobalFlags, Origin, Requested}, config::{ - API_URL, EXECUTE_VARIABLE, Scope, Settings, new_private, private_dir, read_private, - read_secret, + API_URL, EXECUTE_VARIABLE, Scope, Settings, Source, new_private, private_dir, read_private, + read_secret, shell_word, }, dry_run, - error::{ErrorDetail, ErrorKind}, + error::{CredentialSources, ErrorDetail, ErrorKind}, input::{PaginationMode, Request}, output::{Envelope, Outcome, Output}, payment::{ @@ -29,6 +29,7 @@ use futures_util::StreamExt; use serde::{Deserialize, Deserializer, Serialize}; use serde_json::Value; use std::{ + borrow::Cow, collections::BTreeSet, io::Write, sync::{Mutex, OnceLock, PoisonError}, @@ -56,7 +57,8 @@ const JOURNAL_DIR: &str = "requests"; /// Credential the client presents, decided once from the operation's auth scheme. enum Authorization { - Account(ApiCredential), + /// An account credential, with its sources when it is `VOLTAGE_API_KEY`. + Account(ApiCredential, Option), CheckoutSession(Secret), CheckoutStream(Secret), None, @@ -72,7 +74,10 @@ impl Authorization { ) -> Result { let scheme = invocation.operation.auth; Ok(match scheme { - AuthScheme::Account => Self::Account(resolver.resolve_organization(scope).await?), + AuthScheme::Account => Self::Account( + resolver.resolve_organization(scope).await?, + resolver.credential_sources, + ), AuthScheme::CheckoutSession => { Self::CheckoutSession(checkout_token(invocation, "VOLTAGE_CHECKOUT_TOKEN").await?) } @@ -86,8 +91,8 @@ impl Authorization { /// Text that must never appear in output. fn secrets(&self) -> Vec<&str> { match self { - Self::Account(ApiCredential::ApiKey(secret)) - | Self::Account(ApiCredential::OrganizationToken(secret)) + Self::Account(ApiCredential::ApiKey(secret), _) + | Self::Account(ApiCredential::OrganizationToken(secret), _) | Self::CheckoutSession(secret) | Self::CheckoutStream(secret) => vec![secret.expose()], Self::None => Vec::new(), @@ -96,28 +101,54 @@ impl Authorization { /// The next step when the API rejects this credential (401) or what it can reach (403). /// Checkout tokens are never the account credential, so they name their own source. - fn rejection_hint(&self, status: u16) -> Option<&'static str> { - match (self, status) { - (Self::Account(ApiCredential::ApiKey(_)), 401) => { - Some("Check the key with `voltage auth status`; the API does not accept it.") + fn rejection_hint(&self, status: u16) -> Option> { + let hint = match (self, status) { + (Self::Account(ApiCredential::ApiKey(_), _), 401) => { + "Check the key with `voltage auth status`; the API does not accept it." } - (Self::Account(ApiCredential::OrganizationToken(_)), 401) => Some( - "Check the credential with `voltage auth status`. To sign in again, run `voltage logout --local --account NAME`, then `voltage login --account NAME`.", - ), - (Self::Account(_), 403) => Some( - "Check the organization, environment, and credential in use with `voltage context`.", - ), - (Self::CheckoutSession(_), 401 | 403) => Some( - "Check that --token-file or VOLTAGE_CHECKOUT_TOKEN holds this checkout session's token; checkout never uses the account credential.", - ), - (Self::CheckoutStream(_), 401 | 403) => Some( - "Check that --token-file or VOLTAGE_STREAM_TOKEN holds this checkout stream's token; checkout never uses the account credential.", - ), + (Self::Account(ApiCredential::OrganizationToken(_), _), 401) => { + "Check the credential with `voltage auth status`. To sign in again, run `voltage login --account NAME`." + } + (Self::Account(_, Some(sources)), 403) => { + return Some(environment_key_hint(sources.organization).into()); + } + (Self::Account(_, None), 403) => { + "Check the organization, environment, and credential in use with `voltage context`." + } + (Self::CheckoutSession(_), 401 | 403) => { + "Check that --token-file or VOLTAGE_CHECKOUT_TOKEN holds this checkout session's token; checkout never uses the account credential." + } + (Self::CheckoutStream(_), 401 | 403) => { + "Check that --token-file or VOLTAGE_STREAM_TOKEN holds this checkout stream's token; checkout never uses the account credential." + } + _ => return None, + }; + Some(hint.into()) + } + + /// Where an environment key and its organization came from, reported with a 403. + fn credential_sources(&self, status: u16) -> Option { + match (self, status) { + (Self::Account(_, sources), 403) => *sources, _ => None, } } } +/// A 403 to `VOLTAGE_API_KEY` names it and where the organization came from, because the key +/// takes precedence over a saved login. A key is used only without a profile, so the +/// organization comes from `--org` or `VOLTAGE_ORGANIZATION_ID`. +fn environment_key_hint(organization: Source) -> String { + let organization = match organization { + Source::Flag => " and the organization from --org", + Source::Environment => " and the organization from VOLTAGE_ORGANIZATION_ID", + Source::Profile | Source::Default | Source::Unset => "", + }; + format!( + "The API key came from VOLTAGE_API_KEY{organization}. Check that the key can reach the organization, environment, and wallet in use, which `voltage context` shows, or pass --account NAME or unset VOLTAGE_API_KEY to use a saved login." + ) +} + /// A checkout credential from `--token-file`, else from `variable`. async fn checkout_token(invocation: &ApiInvocation, variable: &str) -> Result { if let Some(source) = &invocation.token_file { @@ -176,10 +207,10 @@ impl Api { .request(method, format!("{}{path}", self.base)) .query(query); request = match &self.authorization { - Authorization::Account(ApiCredential::ApiKey(key)) => { + Authorization::Account(ApiCredential::ApiKey(key), _) => { request.header("x-api-key", key.expose()) } - Authorization::Account(ApiCredential::OrganizationToken(token)) + Authorization::Account(ApiCredential::OrganizationToken(token), _) | Authorization::CheckoutSession(token) => request.bearer_auth(token.expose()), Authorization::CheckoutStream(token) => { request.query(&[("stream_token", token.expose())]) @@ -243,10 +274,19 @@ impl Api { } else { ErrorKind::for_http_status(status) }; - let error = Error::new(kind, format!("HTTP {status}: non-JSON response omitted")); - match self.authorization.rejection_hint(status) { - Some(hint) => error.with_hint(hint), - None => error, + Error { + hint: self.authorization.rejection_hint(status), + // Only the sources are attached: an HTTP detail would make an HTML 404 + // read as a missing resource. + detail: self + .authorization + .credential_sources(status) + .map(|sources| ErrorDetail::Http { + http_status: status, + data: Value::Null, + credential_sources: Some(sources), + }), + ..Error::new(kind, format!("HTTP {status}: non-JSON response omitted")) } })? }; @@ -257,19 +297,21 @@ impl Api { rejection.message(status), ); // A credential the API does not recognize is a rejected credential, whatever - // the status, so it gets the same next step as a 401. - let hint = if rejection.kind == ApiErrorType::MissingCredentials { - self.authorization.rejection_hint(401) + // the status, so it gets the same next step as a 401 and no sources. + let rejected_as = if rejection.kind == ApiErrorType::MissingCredentials { + 401 } else { - self.authorization.rejection_hint(status) + status }; - if let Some(hint) = hint.or_else(|| rejection.hint()) { + let hint = self.authorization.rejection_hint(rejected_as); + if let Some(hint) = hint.or_else(|| rejection.hint().map(Cow::Borrowed)) { error = error.with_hint(hint); } return Err(error .with_detail(ErrorDetail::Http { http_status: status, data: body, + credential_sources: self.authorization.credential_sources(rejected_as), }) .redacted(&self.secrets())); } @@ -586,11 +628,12 @@ pub async fn execute( pub async fn wallets( global: &GlobalFlags, credential: ApiCredential, + sources: Option, scope: &Scope, ) -> Result { let operation = operation(OperationId::GetAllOrganizationsWalletsAsUser); let request = Request::scoped(operation, scope)?; - let api = Api::new(global, None, Authorization::Account(credential))?; + let api = Api::new(global, None, Authorization::Account(credential, sources))?; Ok(api .send(operation.method, &request.path, &request.query, None) .await? @@ -603,10 +646,11 @@ pub async fn wallets( pub async fn wallet_currency( global: &GlobalFlags, credential: ApiCredential, + sources: Option, org: Uuid, wallet: Uuid, ) -> Result { - let api = Api::new(global, None, Authorization::Account(credential))?; + let api = Api::new(global, None, Authorization::Account(credential, sources))?; let path = format!("/organizations/{org}/wallets/{wallet}"); let read = api.send(Method::Get, &path, &[], None).await?.body; let unknown = || { @@ -907,7 +951,7 @@ fn scope_flags(scope: &Scope) -> String { let account = scope .account .as_deref() - .map(|account| format!(" --account {}", auth::shell_word(account))); + .map(|account| format!(" --account {}", shell_word(account))); org.into_iter().chain(envs).chain(account).collect() } @@ -1072,6 +1116,8 @@ pub(crate) enum SessionChange { Login, /// The server may have revoked the session, but the saved login is not yet removed. Logout { account: String }, + /// A new login is saved, and the session of the login it replaced may not be revoked yet. + RevokeReplaced { account: String }, } struct SubmittedWrite { @@ -1123,6 +1169,10 @@ impl SubmissionState { "Interrupted while logging out {account}; its session may already be revoked" )) .with_hint("Rerun the logout with --local to remove the saved login."), + Some(SessionChange::RevokeReplaced { account }) => Error::interrupted(format!( + "Interrupted while revoking the replaced session for {account}; the new login is saved" + )) + .with_hint("Account global signout ends the replaced session if it is still active."), None => Error::interrupted("Interrupted before any resource change was submitted"), }; }; @@ -1516,14 +1566,16 @@ mod tests { .build() .unwrap() }; - let key = build(Authorization::Account(ApiCredential::ApiKey(Secret::new( - "key".into(), - )))); + let key = build(Authorization::Account( + ApiCredential::ApiKey(Secret::new("key".into())), + None, + )); assert_eq!(key.headers()["x-api-key"], "key"); assert!(key.headers().get("authorization").is_none()); - let org = build(Authorization::Account(ApiCredential::OrganizationToken( - Secret::new("org".into()), - ))); + let org = build(Authorization::Account( + ApiCredential::OrganizationToken(Secret::new("org".into())), + None, + )); assert_eq!(org.headers()["authorization"], "Bearer org"); let session = build(Authorization::CheckoutSession(Secret::new( "session".into(), @@ -1561,20 +1613,23 @@ mod tests { #[test] fn credential_rejections_name_the_next_step_for_their_own_credential() { - let account = Authorization::Account(ApiCredential::ApiKey(Secret::new("key".into()))); + let account = + Authorization::Account(ApiCredential::ApiKey(Secret::new("key".into())), None); let hint = |authorization: &Authorization, status| authorization.rejection_hint(status); assert!(hint(&account, 401).unwrap().contains("voltage auth status")); // Signing in again cannot fix a rejected key, so only a login names `voltage login`. assert!(!hint(&account, 401).unwrap().contains("voltage login")); - let login = - Authorization::Account(ApiCredential::OrganizationToken(Secret::new("t".into()))); + let login = Authorization::Account( + ApiCredential::OrganizationToken(Secret::new("t".into())), + None, + ); let renew = hint(&login, 401).unwrap(); assert!( - renew.contains("voltage logout --local --account NAME") - && renew.contains("voltage login --account NAME"), + renew.contains("voltage login --account NAME") && !renew.contains("voltage logout"), "{renew}" ); assert!(hint(&account, 403).unwrap().contains("voltage context")); + assert_eq!(account.credential_sources(403), None); assert_eq!(hint(&account, 404), None); assert_eq!(hint(&account, 500), None); let session = Authorization::CheckoutSession(Secret::new("session".into())); @@ -1589,6 +1644,56 @@ mod tests { assert_eq!(hint(&Authorization::None, 401), None); } + #[test] + fn a_forbidden_environment_key_names_its_sources() { + let environment_key = |organization| { + Authorization::Account( + ApiCredential::ApiKey(Secret::new("key".into())), + Some(CredentialSources { + credential: Source::Environment, + organization, + }), + ) + }; + let flagged = environment_key(Source::Flag); + let next = flagged.rejection_hint(403).unwrap(); + assert!( + next.starts_with( + "The API key came from VOLTAGE_API_KEY and the organization from --org." + ) && next.contains("pass --account NAME or unset VOLTAGE_API_KEY"), + "{next}" + ); + assert_eq!( + flagged + .credential_sources(403) + .map(|sources| sources.organization), + Some(Source::Flag) + ); + // A rejected key is not a scope problem, so a 401 reports no sources. + assert!( + flagged + .rejection_hint(401) + .unwrap() + .contains("voltage auth status") + ); + assert_eq!(flagged.credential_sources(401), None); + assert!( + environment_key(Source::Environment) + .rejection_hint(403) + .unwrap() + .contains("the organization from VOLTAGE_ORGANIZATION_ID.") + ); + // Without a known organization source, the hint names only the key. + assert!( + environment_key(Source::Unset) + .rejection_hint(403) + .unwrap() + .starts_with( + "The API key came from VOLTAGE_API_KEY. Check that the key can reach the organization, environment, and wallet in use" + ) + ); + } + /// The parsed `error` object of a rejection body. fn rejection(body: Value) -> ApiErrorBody { ApiErrorView::deserialize(body).unwrap_or_default().error @@ -1841,6 +1946,13 @@ mod tests { "Interrupted while logging out person; its session may already be revoked", "--local", ), + ( + SessionChange::RevokeReplaced { + account: "person".into(), + }, + "Interrupted while revoking the replaced session for person; the new login is saved", + "global signout", + ), ] { submission.session_changing(change); let during = submission.interrupted(); diff --git a/src/auth.rs b/src/auth.rs index 3209a7f..becbd0b 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -9,9 +9,11 @@ use crate::{ api::{SessionChange, SubmissionState}, cli::{GlobalFlags, ImportKeyFlags, LoginFlags}, config::{ - AUTH_URL, Account, AccountKind, Credential, CredentialProblem, InputSource, LOCK_TIMEOUT, - Login, Scope, Settings, name_in_use, read_secret, + AUTH_URL, Account, AccountKind, Credential, CredentialProblem, CredentialStore, + InputSource, LOCK_TIMEOUT, Login, Scope, ScopeSources, Settings, Source, name_in_use, + read_secret, shell_word, unreadable_name, }, + error::{CredentialRejection, CredentialSources, ErrorDetail}, secret::Secret, terminal::{self, Terminal}, }; @@ -504,6 +506,8 @@ pub struct LoginOutcome { pub account: String, pub email: Option, pub authenticated: bool, + /// The login replaced a saved login of the same user under this name. + pub replaced: bool, } #[derive(Debug, Serialize)] @@ -546,6 +550,18 @@ fn ambient_api_key(flags: &GlobalFlags) -> Option { std::env::var("VOLTAGE_API_KEY").ok().map(Secret::new) } +/// Where `VOLTAGE_API_KEY` and the organization came from, when this command sends that key. +/// A saved credential returns `None`. +pub(crate) fn environment_key_sources( + flags: &GlobalFlags, + sources: &ScopeSources, +) -> Option { + ambient_api_key(flags).map(|_| CredentialSources { + credential: Source::Environment, + organization: sources.org, + }) +} + /// `VOLTAGE_API_KEY` is set, but a profile or account selection takes precedence over it. pub(crate) fn ignores_ambient_api_key(flags: &GlobalFlags) -> bool { std::env::var_os("VOLTAGE_API_KEY").is_some() && ambient_api_key(flags).is_none() @@ -649,10 +665,18 @@ pub async fn login( ) -> Result { let terminal = global.terminal(); let auth = AuthService::new(global.auth_url.as_deref().unwrap_or(AUTH_URL), terminal)?; + // Only a login of the same user replaces a saved one. That user is known only after + // the browser step, so a name that cannot be a login at this auth URL is refused first. + // A saved login whose secret cannot be read cannot be shown to be the same user either. if let Some(name) = &global.account - && settings.config.accounts.contains_key(name) + && let Some(saved) = settings.config.accounts.get(name) { - return Err(name_in_use(name)); + if !is_login_at(saved, &auth.url) { + return Err(name_in_use(name)); + } + settings + .read_credential(name) + .map_err(|_| unreadable_name(name, saved.store))?; } let device = auth.device_authorization().await?; let verification = device.verification_url()?; @@ -666,7 +690,7 @@ pub async fn login( let token = auth.poll_device_grant(&device, submission).await?; let login = token.into_login(None, None)?; let session = Secret::new(login.refresh_token.expose().to_owned()); - let saved = save_login(settings, &auth, login, flags, global).await; + let saved = save_login(settings, &auth, login, flags, global, submission).await; if saved.is_err() { // A local storage/discovery failure must not silently orphan a remote session. if auth.revoke(&session).await.is_err() { @@ -687,6 +711,7 @@ async fn save_login( mut login: Login, flags: &LoginFlags, global: &GlobalFlags, + submission: &SubmissionState, ) -> Result { let user = auth.current_user(&login.access_token).await?; login.user_id = Some(user.id); @@ -698,24 +723,68 @@ async fn save_login( .or_else(|| login.user_id.clone()) .ok_or_else(|| Error::auth("User response has no account identity"))?; let email = login.email.clone(); + let user_id = login.user_id.clone(); + // Without `--credential-store`, a replaced login keeps its store and a new one uses the + // keychain. `put_account` reads the account again under the lock; if another process + // changed it in between, the replacement is a store change, which it handles. + let store = flags.credential_store.unwrap_or_else(|| { + settings + .config + .accounts + .get(&name) + .filter(|saved| is_login_at(saved, &auth.url)) + .map_or(CredentialStore::Keychain, |saved| saved.store) + }); let account = Account { - store: flags.credential_store, + store, kind: AccountKind::User, email: email.clone(), organization_id: None, environment_id: None, auth_url: auth.url.clone(), }; - settings - .add_account(&name, account, &Credential::Login(login), auth.terminal) + let replaced = settings + .put_account( + &name, + account, + &Credential::Login(login), + auth.terminal, + |saved| is_login_at(saved, &auth.url), + // A saved login without a user ID cannot be shown to be the same user. + |credential| { + matches!(credential, Credential::Login(old) if old.user_id.is_some() && old.user_id == user_id) + }, + ) .await?; + // The new login is saved, so only the replaced session's revoke is still in flight. + match &replaced { + Some(_) => submission.session_changing(SessionChange::RevokeReplaced { + account: name.clone(), + }), + None => submission.session_settled(), + } + if let Some(Credential::Login(old)) = &replaced { + auth.terminal + .notice(format!("Replaced the saved login for {name}.")); + if auth.revoke(&old.refresh_token).await.is_err() { + auth.terminal.important( + "Could not revoke the replaced CLI session. Use account global signout to invalidate its refresh session.", + ); + } + } Ok(LoginOutcome { account: name, email, authenticated: true, + replaced: replaced.is_some(), }) } +/// A saved browser login against the auth service at `url`, which a new login can replace. +fn is_login_at(account: &Account, url: &str) -> bool { + account.kind == AccountKind::User && account.auth_url == url +} + fn saved_login(settings: &Settings, name: &str) -> Result { match settings.read_credential(name)? { Credential::Login(login) => Ok(login), @@ -735,25 +804,15 @@ fn expired_login(account: &str, url: &str) -> Error { Error::auth(format!( "The saved login for {account} has expired or was revoked" )) - // `login` refuses to replace a saved credential, so the stale one goes first. + .with_detail(ErrorDetail::Credential { + credential_problem: CredentialRejection::LoginExpired, + }) + // A login of the same user replaces the stale one, so nothing has to be removed first. .with_hint(format!( - "Remove it with `voltage logout --local --account {account}`, then sign in again with `voltage login --account {account}{auth_url}`." + "Sign in again with `voltage login --account {account}{auth_url}`." )) } -/// `value` as one word a POSIX shell reads back unchanged, for a hint's copyable command. -pub(crate) fn shell_word(value: &str) -> String { - let plain = !value.is_empty() - && value - .chars() - .all(|c| c.is_ascii_alphanumeric() || "@%+,-./:=_".contains(c)); - if plain { - value.to_owned() - } else { - format!("'{}'", value.replace('\'', r"'\''")) - } -} - /// Resolves, refreshes, and exchanges one command's credential, and lists what it can see. /// It holds what stays the same for the whole command. The scope is an argument to each call, /// because a picker can fill in the organization between two calls. @@ -763,6 +822,9 @@ pub struct Resolver<'a> { pub flags: &'a GlobalFlags, /// Records a refresh in flight, so an interruption can report it. pub submission: &'a SubmissionState, + /// Where `VOLTAGE_API_KEY` and the organization came from, when this command sends that + /// key, from [`environment_key_sources`]. + pub credential_sources: Option, } impl Resolver<'_> { @@ -772,6 +834,7 @@ impl Resolver<'_> { settings, flags, submission, + .. } = self; let (name, account) = match select(settings, scope, flags) .map_err(|rejected| rejected.problem.error(scope, settings))? @@ -918,7 +981,13 @@ pub async fn logout( }); auth.revoke(&login.refresh_token).await?; } - settings.delete_credential(&name)?; + // `--local` is how a person removes a login whose secret cannot be removed, so a failed + // removal there is only a warning. + if local { + settings.discard_credential(&name, terminal)?; + } else { + settings.delete_credential(&name)?; + } settings.config.accounts.remove(&name); settings.save()?; submission.session_settled(); @@ -1070,9 +1139,16 @@ mod tests { fn expired_logins_name_commands_that_can_be_pasted() { let hint = |account, url| expired_login(account, url).hint.unwrap(); let default = base_url(AUTH_URL).unwrap(); + // Agents detect a dead login by its problem, not by parsing the message. + assert!(matches!( + expired_login("me", &default).detail, + Some(ErrorDetail::Credential { + credential_problem: CredentialRejection::LoginExpired + }) + )); assert_eq!( hint("me@example.com", &default), - "Remove it with `voltage logout --local --account me@example.com`, then sign in again with `voltage login --account me@example.com`." + "Sign in again with `voltage login --account me@example.com`." ); // `--auth-url` is never saved, so another auth service is named again. assert!(hint("me", "https://auth.staging.example.test").ends_with( diff --git a/src/cli.rs b/src/cli.rs index ee652fa..5c15dff 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -204,9 +204,9 @@ pub struct LoginFlags { /// Print the approval URL without opening it #[arg(long)] pub no_browser: bool, - /// Where to save credentials - #[arg(long, value_enum, default_value_t = CredentialStore::Keychain)] - pub credential_store: CredentialStore, + /// Where to save credentials; by default the keychain, or the store of the login it replaces + #[arg(long, value_enum)] + pub credential_store: Option, } #[derive(Debug, Args)] @@ -1248,7 +1248,7 @@ fn leaf(matches: &ArgMatches) -> (Vec, &ArgMatches) { mod tests { use super::*; use crate::config::CredentialProblem as Problem; - use crate::error::ErrorKind as Kind; + use crate::error::{CredentialRejection, ErrorKind as Kind}; use std::path::Path; const ENV: &str = "22222222-2222-4222-8222-222222222222"; @@ -1542,7 +1542,7 @@ mod tests { panic!("expected login"); }; assert!(flags.no_browser); - assert_eq!(flags.credential_store, CredentialStore::File); + assert_eq!(flags.credential_store, Some(CredentialStore::File)); let profile = Invocation::try_parse_from(["voltage", "profiles", "delete", "stage"]).unwrap(); assert!(matches!( @@ -1883,6 +1883,15 @@ mod tests { assert_eq!(serde_json::to_value(problem).unwrap(), name); assert!(text.contains(&format!("\n### `{name}`\n")), "{name}"); } + let rejected = |rejection: CredentialRejection| match rejection { + CredentialRejection::LoginExpired => "login_expired", + }; + let name = rejected(CredentialRejection::LoginExpired); + assert_eq!( + serde_json::to_value(CredentialRejection::LoginExpired).unwrap(), + name + ); + assert!(text.contains(&format!("\n### `{name}`\n")), "{name}"); } /// Every command below `cmd`, by path, with its rendered long help. diff --git a/src/config.rs b/src/config.rs index 293dbf2..d35e2b8 100644 --- a/src/config.rs +++ b/src/config.rs @@ -68,6 +68,16 @@ pub enum CredentialStore { File, } +impl CredentialStore { + /// The store's name in a message. + fn label(self) -> &'static str { + match self { + Self::Keychain => "OS credential store", + Self::File => "credentials directory", + } + } +} + /// Non-secret facts about one saved credential. #[derive(Clone, Serialize, Deserialize)] pub struct Account { @@ -514,19 +524,24 @@ impl Settings { } pub fn read_credential(&self, name: &str) -> Result { - let raw: Zeroizing = match self.account(name)?.store { - CredentialStore::File => read_private(&self.credential_path(name))?, + let raw = self.read_secret(name, self.account(name)?.store)?; + let stored: StoredCredential = + serde_json::from_str(&raw).map_err(|_| Error::auth("Invalid saved credential"))?; + Credential::try_from(stored) + } + + /// The secret saved for `name` in `store`, as it was written. + fn read_secret(&self, name: &str, store: CredentialStore) -> Result> { + match store { + CredentialStore::File => read_private(&self.credential_path(name)), CredentialStore::Keychain => { - Zeroizing::new(self.keyring_entry(name)?.get_password().map_err(|_| { + Ok(Zeroizing::new(self.keyring_entry(name)?.get_password().map_err(|_| { Error::auth( "Cannot read OS credential store; unlock it or log in with --credential-store file", ) - })?) + })?)) } - }; - let stored: StoredCredential = - serde_json::from_str(&raw).map_err(|_| Error::auth("Invalid saved credential"))?; - Credential::try_from(stored) + } } pub fn write_credential( @@ -535,9 +550,7 @@ impl Settings { store: CredentialStore, credential: &Credential, ) -> Result<()> { - let raw = Zeroizing::new(serde_json::to_string(&StoredCredentialRef::from( - credential, - ))?); + let raw = serialized(credential)?; match store { CredentialStore::File => { private_dir(&self.dir)?; @@ -556,16 +569,48 @@ impl Settings { } pub fn delete_credential(&self, name: &str) -> Result<()> { - match self.account(name)?.store { - CredentialStore::File => fs::remove_file(self.credential_path(name))?, - CredentialStore::Keychain => self - .keyring_entry(name)? - .delete_credential() - .map_err(|_| Error::auth("Cannot remove saved credential"))?, + self.delete_secret(name, self.account(name)?.store) + } + + /// Remove the secret saved for `name` in `store`, whatever store its account names. A + /// secret that is already gone is not an error, so `logout --local` can remove its account. + fn delete_secret(&self, name: &str, store: CredentialStore) -> Result<()> { + match store { + CredentialStore::File => { + fs::remove_file(self.credential_path(name)).or_else(|error| match error.kind() { + std::io::ErrorKind::NotFound => Ok(()), + _ => Err(error.into()), + }) + } + CredentialStore::Keychain => { + self.keyring_entry(name)? + .delete_credential() + .or_else(|error| match error { + keyring::Error::NoEntry => Ok(()), + _ => Err(Error::auth("Cannot remove saved credential")), + }) + } } + } + + /// Remove the secret saved for `name`, as `delete_credential` does, but warn instead of + /// failing when it cannot be removed. + pub(crate) fn discard_credential(&self, name: &str, terminal: Terminal) -> Result<()> { + self.discard_secret(name, self.account(name)?.store, terminal); Ok(()) } + /// Remove a secret that no account reaches any more. A failure leaves the secret behind + /// but does not fail the command, so it is shown as a warning. + fn discard_secret(&self, name: &str, store: CredentialStore, terminal: Terminal) { + if self.delete_secret(name, store).is_err() { + terminal.important(format!( + "Could not remove the saved secret for {name} from the {}; remove it there yourself.", + store.label() + )); + } + } + /// Save a new account and its secret under the credential lock. A name already in use is /// refused. When the configuration cannot be saved, the secret is removed again, so no /// stored secret is left that no account entry reaches. @@ -576,19 +621,126 @@ impl Settings { credential: &Credential, terminal: Terminal, ) -> Result<()> { + self.put_account(name, account, credential, terminal, |_| false, |_| false) + .await + .map(drop) + } + + /// Save an account and its secret under the credential lock, as `add_account` does. A + /// saved account of that name is replaced only when `replaceable` accepts it and then + /// `same` accepts its credential; the replaced credential is returned so its session can + /// be revoked. The saved secret is read only for a replaceable account, and a secret that + /// cannot be read refuses the name with a hint to remove it. When the secret or the + /// configuration cannot be saved, the saved account and secret are restored. + pub async fn put_account( + &mut self, + name: &str, + account: Account, + credential: &Credential, + terminal: Terminal, + replaceable: impl FnOnce(&Account) -> bool, + same: impl FnOnce(&Credential) -> bool, + ) -> Result> { let _lock = self.lock(terminal).await?; self.reload()?; - if self.config.accounts.contains_key(name) { - return Err(name_in_use(name)); + let saved = self + .config + .accounts + .get(name) + .cloned() + .map(|saved| { + if !replaceable(&saved) { + return Err(name_in_use(name)); + } + let old = self + .read_credential(name) + .map_err(|_| unreadable_name(name, saved.store))?; + same(&old) + .then_some((saved, old)) + .ok_or_else(|| name_in_use(name)) + }) + .transpose()?; + let store = account.store; + // A failed write may already have replaced the saved secret, as when the directory + // sync after the rename fails, so every failure restores it. + let written = self + .write_credential(name, store, credential) + .and_then(|()| { + self.config.accounts.insert(name.to_owned(), account); + self.save() + }); + if let Err(error) = written { + return Err(self.restore(name, store, saved, terminal, error)); } - self.write_credential(name, account.store, credential)?; - self.config.accounts.insert(name.to_owned(), account); - if let Err(error) = self.save() { - let _ = self.delete_credential(name); - self.config.accounts.remove(name); - return Err(error); + // A store change is not exercised by tests: they cannot use an OS keychain. + if let Some((saved, _)) = &saved + && saved.store != store + { + self.discard_secret(name, saved.store, terminal); } - Ok(()) + Ok(saved.map(|(_, old)| old)) + } + + /// Undo a `put_account` that failed with `error`: put back the replaced account and + /// secret, or remove the new secret when nothing was replaced, and save the configuration + /// again, since a save that failed after its rename has already written it. The returned + /// error says so when the saved login could not be put back. + fn restore( + &mut self, + name: &str, + store: CredentialStore, + saved: Option<(Account, Credential)>, + terminal: Terminal, + error: Error, + ) -> Error { + // The new secret goes unless the old one is written back over it in the same store. + // As in `put_account`, a store change is not exercised by tests. + if saved.as_ref().is_none_or(|(saved, _)| saved.store != store) { + self.discard_secret(name, store, terminal); + } + let secret_restored = match saved { + Some((saved, old)) => { + let restored = self.write_credential(name, saved.store, &old).is_ok() + || self.holds_secret(name, saved.store, &old); + self.config.accounts.insert(name.to_owned(), saved); + restored + } + None => { + self.config.accounts.remove(name); + true + } + }; + let name_word = shell_word(name); + if !(self.save().is_ok() || self.saved_store_matches(name)) { + return error.with_hint(format!( + "The saved configuration for {name} could not be restored. Remove it with `voltage logout --local --account {name_word}`, then sign in again." + )); + } + if secret_restored { + return error; + } + error.with_hint(format!( + "The replaced login for {name} could not be restored. Sign in again with `voltage login --account {name_word}`." + )) + } + + /// Whether `store` holds `credential` for `name`, as when a failed write never replaced it. + fn holds_secret(&self, name: &str, store: CredentialStore, credential: &Credential) -> bool { + let held = self.read_secret(name, store).and_then(|raw| { + let stored: StoredCredential = serde_json::from_str(&raw)?; + serialized(&Credential::try_from(stored)?) + }); + match (held, serialized(credential)) { + (Ok(held), Ok(expected)) => held == expected, + _ => false, + } + } + + /// Whether the configuration on disk names the same store for `name` as the one in + /// memory, or lacks `name` just as it does, as when a failed save never replaced it. + fn saved_store_matches(&self, name: &str) -> bool { + let store = |config: &Config| config.accounts.get(name).map(|account| account.store); + read_config(&self.dir).is_ok_and(|disk| store(&disk) == store(&self.config)) } /// Exclusive process lock for credential and configuration writes. @@ -658,6 +810,13 @@ fn sourced(value: Option, source: Source) -> (Option, Source) { (value, source) } +/// `credential` as `write_credential` saves it. +fn serialized(credential: &Credential) -> Result> { + Ok(Zeroizing::new(serde_json::to_string( + &StoredCredentialRef::from(credential), + )?)) +} + /// A new account named like a saved one; `login` and `auth import-key` refuse it alike. pub(crate) fn name_in_use(name: &str) -> Error { Error::usage(format!( @@ -665,6 +824,32 @@ pub(crate) fn name_in_use(name: &str) -> Error { )) } +/// A saved name whose secret cannot be read, so it cannot be shown to be replaceable. Only +/// removing the account frees the name, because a new login cannot overwrite it. +pub(crate) fn unreadable_name(name: &str, store: CredentialStore) -> Error { + let unlock = match store { + CredentialStore::Keychain => "Unlock the OS credential store, or remove", + CredentialStore::File => "Remove", + }; + name_in_use(name).with_hint(format!( + "Its saved secret cannot be read. {unlock} it with `voltage logout --local --account {}`.", + shell_word(name) + )) +} + +/// `value` as one word a POSIX shell reads back unchanged, for a hint's copyable command. +pub(crate) fn shell_word(value: &str) -> String { + let plain = !value.is_empty() + && value + .chars() + .all(|c| c.is_ascii_alphanumeric() || "@%+,-./:=_".contains(c)); + if plain { + value.to_owned() + } else { + format!("'{}'", value.replace('\'', r"'\''")) + } +} + /// Saved names, comma-separated, or `None` when there are none. fn joined<'a>(names: impl Iterator) -> Option { let names: Vec<&str> = names.map(String::as_str).collect(); @@ -819,7 +1004,13 @@ pub fn atomic_write(path: &Path, data: &[u8]) -> Result<()> { let mut file = new_private(&temp)?; file.write_all(data)?; file.sync_all()?; + #[cfg(test)] + let fault = write_faults::next(); + #[cfg(test)] + write_faults::inject(fault, write_faults::Fault::BeforeRename)?; fs::rename(&temp, path)?; + #[cfg(test)] + write_faults::inject(fault, write_faults::Fault::AfterRename)?; #[cfg(unix)] File::open(path.parent().unwrap_or(Path::new("/")))?.sync_all()?; Ok(()) @@ -830,6 +1021,39 @@ pub fn atomic_write(path: &Path, data: &[u8]) -> Result<()> { result } +/// Failures that a test makes `atomic_write` report, one per call in order, before or after +/// the rename that replaces the file. +#[cfg(test)] +pub(crate) mod write_faults { + use std::{cell::RefCell, collections::VecDeque}; + + #[derive(Clone, Copy, Debug, Eq, PartialEq)] + pub(crate) enum Fault { + BeforeRename, + AfterRename, + } + + thread_local! { + static FAULTS: RefCell>> = const { RefCell::new(VecDeque::new()) }; + } + + /// Fail the next `atomic_write` calls in this thread; `None` lets a call succeed. + pub(crate) fn set(faults: impl IntoIterator>) { + FAULTS.with(|queue| *queue.borrow_mut() = faults.into_iter().collect()); + } + + pub(super) fn next() -> Option { + FAULTS.with(|queue| queue.borrow_mut().pop_front().flatten()) + } + + pub(super) fn inject(fault: Option, stage: Fault) -> std::io::Result<()> { + if fault == Some(stage) { + return Err(std::io::Error::other("injected write failure")); + } + Ok(()) + } +} + /// Where a command reads a payload or credential from. #[derive(Clone, Debug, Eq, PartialEq)] pub enum InputSource { @@ -1070,4 +1294,313 @@ mod tests { assert!(!settings.config.accounts.contains_key("key")); assert!(!settings.credential_path("key").exists()); } + + fn login(refresh: &str) -> Credential { + Credential::Login(Login { + access_token: Secret::new("access".into()), + refresh_token: Secret::new(refresh.into()), + expires_at: 0, + user_id: Some("user".into()), + email: None, + }) + } + + fn refresh_token(settings: &Settings, name: &str) -> String { + match settings.read_credential(name).unwrap() { + Credential::Login(login) => login.refresh_token.expose().to_owned(), + Credential::ApiKey(_) => panic!("expected a login"), + } + } + + fn user_account() -> Account { + Account { + store: CredentialStore::File, + kind: AccountKind::User, + email: None, + organization_id: None, + environment_id: None, + auth_url: AUTH_URL.into(), + } + } + + /// A configuration directory with the private mode that `Settings` requires. + fn private_tempdir() -> tempfile::TempDir { + let dir = tempfile::tempdir().unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o700)).unwrap(); + } + dir + } + + #[tokio::test] + async fn a_saved_account_is_replaced_only_when_accepted() { + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + settings + .add_account("me", user_account(), &login("old"), terminal) + .await + .unwrap(); + let refused = settings + .put_account( + "me", + user_account(), + &login("new"), + terminal, + |_| true, + |_| false, + ) + .await; + assert!(refused.is_err()); + assert_eq!(refresh_token(&settings, "me"), "old"); + let replaced = settings + .put_account( + "me", + user_account(), + &login("new"), + terminal, + |_| true, + |_| true, + ) + .await + .unwrap(); + assert!( + matches!(replaced, Some(Credential::Login(old)) if old.refresh_token.expose() == "old") + ); + assert_eq!(refresh_token(&settings, "me"), "new"); + } + + /// A saved name whose secret is gone is refused as in use, never as a read error. An + /// account that cannot be replaced is refused without reading its secret, and a + /// replaceable one names the command that removes it. + #[tokio::test] + async fn a_saved_name_whose_secret_cannot_be_read_is_in_use() { + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + settings + .add_account("me", user_account(), &login("old"), terminal) + .await + .unwrap(); + fs::remove_file(settings.credential_path("me")).unwrap(); + let in_use = name_in_use("me").to_string(); + let Err(added) = settings + .add_account("me", user_account(), &login("new"), terminal) + .await + else { + panic!("a saved name was added again"); + }; + assert_eq!(added.to_string(), in_use); + assert!(added.hint.is_none()); + let Err(replaced) = settings + .put_account( + "me", + user_account(), + &login("new"), + terminal, + |_| true, + |_| true, + ) + .await + else { + panic!("an unreadable secret was replaced"); + }; + assert_eq!(replaced.to_string(), in_use); + assert_eq!( + replaced.hint.as_deref(), + Some( + "Its saved secret cannot be read. Remove it with `voltage logout --local --account me`." + ) + ); + assert!(!settings.credential_path("me").exists()); + settings.delete_credential("me").unwrap(); + assert_eq!( + unreadable_name("me", CredentialStore::Keychain) + .hint + .as_deref(), + Some( + "Its saved secret cannot be read. Unlock the OS credential store, or remove it with `voltage logout --local --account me`." + ) + ); + } + + /// A replacement that fails returns an error, so `login` revokes the new session. Each + /// failure, even one after a rename already replaced a file, leaves the replaced login + /// saved, so the session that stays live is the one that is stored. + #[tokio::test] + async fn a_replacement_that_fails_after_a_rename_restores_the_saved_login() { + use write_faults::Fault::{AfterRename, BeforeRename}; + // The secret, then the configuration: each fails before or after its rename, and + // the rollback write of the old secret fails too when nothing had replaced it. + for faults in [ + vec![Some(AfterRename)], + vec![Some(BeforeRename), Some(BeforeRename)], + vec![None, Some(AfterRename)], + ] { + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + settings + .add_account("me", user_account(), &login("old"), terminal) + .await + .unwrap(); + let mut account = user_account(); + account.email = Some("me@example.test".into()); + write_faults::set(faults.clone()); + let Err(error) = settings + .put_account("me", account, &login("new"), terminal, |_| true, |_| true) + .await + else { + panic!("{faults:?}: a failed replacement succeeded"); + }; + write_faults::set([]); + assert_eq!(error.to_string(), "injected write failure", "{faults:?}"); + assert!(error.hint.is_none(), "{faults:?}"); + settings.reload().unwrap(); + assert_eq!(settings.config.accounts["me"].email, None, "{faults:?}"); + assert_eq!(refresh_token(&settings, "me"), "old", "{faults:?}"); + } + } + + /// When the old secret cannot be written back over the new one, the error says the + /// replaced login is lost and how to sign in again. + #[tokio::test] + async fn a_failed_rollback_names_the_lost_login() { + use write_faults::Fault::BeforeRename; + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + settings + .add_account("me", user_account(), &login("old"), terminal) + .await + .unwrap(); + write_faults::set([None, Some(BeforeRename), Some(BeforeRename)]); + let replaced = settings + .put_account( + "me", + user_account(), + &login("new"), + terminal, + |_| true, + |_| true, + ) + .await; + write_faults::set([]); + let Err(error) = replaced else { + panic!("a failed replacement succeeded"); + }; + assert_eq!( + error.hint.as_deref(), + Some( + "The replaced login for me could not be restored. Sign in again with `voltage login --account me`." + ) + ); + assert_eq!(refresh_token(&settings, "me"), "new"); + } + + /// A new account whose secret or configuration write fails after its rename leaves no + /// secret and no account. When the configuration cannot be saved again, the error names + /// the command that removes the account it lists. + #[tokio::test] + async fn a_new_account_that_fails_after_a_rename_leaves_nothing_saved() { + use write_faults::Fault::{AfterRename, BeforeRename}; + for faults in [vec![Some(AfterRename)], vec![None, Some(AfterRename)]] { + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + write_faults::set(faults.clone()); + let added = settings + .add_account("me", user_account(), &login("new"), terminal) + .await; + write_faults::set([]); + let Err(error) = added else { + panic!("{faults:?}: a failed account was added"); + }; + assert!(error.hint.is_none(), "{faults:?}"); + settings.reload().unwrap(); + assert!(!settings.config.accounts.contains_key("me"), "{faults:?}"); + assert!(!settings.credential_path("me").exists(), "{faults:?}"); + } + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + write_faults::set([None, Some(AfterRename), Some(BeforeRename)]); + let added = settings + .add_account("me", user_account(), &login("new"), terminal) + .await; + write_faults::set([]); + let Err(error) = added else { + panic!("a failed account was added"); + }; + assert_eq!( + error.hint.as_deref(), + Some( + "The saved configuration for me could not be restored. Remove it with `voltage logout --local --account me`, then sign in again." + ) + ); + } + + /// A secret that cannot be removed is only a warning for `discard_credential`, which + /// `logout --local` uses. + #[cfg(unix)] + #[tokio::test] + async fn a_secret_that_cannot_be_removed_is_discarded_with_a_warning() { + use std::os::unix::fs::PermissionsExt; + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + settings + .add_account("me", user_account(), &login("old"), terminal) + .await + .unwrap(); + let credentials = dir.path().join(CREDENTIALS_DIR); + let mode = |mode| { + fs::set_permissions(&credentials, fs::Permissions::from_mode(mode)).unwrap(); + }; + mode(0o500); + let deleted = settings.delete_credential("me"); + let discarded = settings.discard_credential("me", terminal); + let kept = settings.credential_path("me").exists(); + mode(0o700); + // The owner's permissions do not bind root, so there is nothing to test. + if deleted.is_ok() { + return; + } + assert!(discarded.is_ok()); + assert!(kept); + } + + /// When the configuration cannot be saved, the replaced account and secret are put back. + #[cfg(unix)] + #[tokio::test] + async fn a_replacement_that_cannot_be_saved_restores_the_saved_login() { + use std::os::unix::fs::PermissionsExt; + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + settings + .add_account("me", user_account(), &login("old"), terminal) + .await + .unwrap(); + let mode = |mode| { + fs::set_permissions(dir.path(), fs::Permissions::from_mode(mode)).unwrap(); + }; + mode(0o500); + // The owner's permissions do not bind root, so there is nothing to test. + if new_private(&dir.path().join("probe")).is_ok() { + mode(0o700); + return; + } + let mut account = user_account(); + account.email = Some("me@example.test".into()); + let replaced = settings + .put_account("me", account, &login("new"), terminal, |_| true, |_| true) + .await; + mode(0o700); + assert!(replaced.is_err()); + assert_eq!(settings.config.accounts["me"].email, None); + assert_eq!(refresh_token(&settings, "me"), "old"); + } } diff --git a/src/error.rs b/src/error.rs index 041e183..c86c2cf 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,6 +1,9 @@ //! Failure categories, their documented exit codes, and the typed detail reported with them. -use crate::output::{redact, redact_text}; +use crate::{ + config::Source, + output::{redact, redact_text}, +}; use serde::Serialize; use serde_json::Value; use std::borrow::Cow; @@ -58,7 +61,13 @@ pub enum SubmissionOutcome { #[serde(untagged)] pub enum ErrorDetail { /// The API answered with a non-success status. - Http { http_status: u16, data: Value }, + Http { + http_status: u16, + data: Value, + /// Present when a 403 was answered to `VOLTAGE_API_KEY`. + #[serde(skip_serializing_if = "Option::is_none")] + credential_sources: Option, + }, /// A mutation was sent, but the transport failed before a result arrived. UncertainSubmission { resource_id: Option, @@ -74,6 +83,28 @@ pub enum ErrorDetail { }, /// A payment's terminal state, as the API reported it. Payment(Value), + /// The auth service rejected a saved credential. + Credential { + credential_problem: CredentialRejection, + }, +} + +/// Why the auth service rejected a saved credential. `voltage context` reads no secret and +/// makes no request, so it cannot report these; they appear only on a command's error. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum CredentialRejection { + /// The auth service answered a saved login's refresh or token exchange with + /// `invalid_grant`: the login expired, was revoked, or its grant was already spent. + LoginExpired, +} + +/// Where the credential and the organization of a request came from. A 403 reports it for +/// `VOLTAGE_API_KEY`, because that key takes precedence over a saved login without saying so. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize)] +pub struct CredentialSources { + pub(crate) credential: Source, + pub(crate) organization: Source, } impl ErrorDetail { @@ -102,7 +133,7 @@ impl ErrorDetail { pub fn redact(&mut self, secrets: &[&str]) { match self { Self::Http { data, .. } | Self::Payment(data) => redact(data, secrets), - Self::UncertainSubmission { .. } | Self::Pending { .. } => {} + Self::UncertainSubmission { .. } | Self::Pending { .. } | Self::Credential { .. } => {} } } } @@ -275,9 +306,17 @@ mod tests { "action": "Query the original resource ID before deciding whether to resubmit" }) ); + assert_eq!( + serde_json::to_value(ErrorDetail::Credential { + credential_problem: CredentialRejection::LoginExpired + }) + .unwrap(), + json!({"credential_problem": "login_expired"}) + ); let http = Error::api("failed").with_detail(ErrorDetail::Http { http_status: 404, data: json!({"api_key": "secret"}), + credential_sources: None, }); assert_eq!(http.http_status(), Some(404)); let redacted = serde_json::to_value(http.redacted(&[]).detail).unwrap(); @@ -285,6 +324,22 @@ mod tests { redacted, json!({"http_status": 404, "data": {"api_key": "[REDACTED]"}}) ); + let forbidden = ErrorDetail::Http { + http_status: 403, + data: Value::Null, + credential_sources: Some(CredentialSources { + credential: Source::Environment, + organization: Source::Flag, + }), + }; + assert_eq!( + serde_json::to_value(forbidden).unwrap(), + json!({ + "http_status": 403, + "data": null, + "credential_sources": {"credential": "environment", "organization": "flag"} + }) + ); let echoed = Error::api("HTTP 400: Bad key key-1").redacted(&["key-1", ""]); assert_eq!(echoed.message, "HTTP 400: Bad key [REDACTED]"); } diff --git a/src/pick.rs b/src/pick.rs index 20003ec..bf3a9a7 100644 --- a/src/pick.rs +++ b/src/pick.rs @@ -154,7 +154,7 @@ async fn ask( picked.push(format!("--env {env}")); } if missing.wallet { - let listed = api::wallets(global, credential, scope).await?; + let listed = api::wallets(global, credential, resolver.credential_sources, scope).await?; let Some(wallet) = choose(terminal, "Wallet:", "wallets", &listed).await? else { return Ok(picked); }; diff --git a/src/startup.rs b/src/startup.rs index 0146357..eccfd74 100644 --- a/src/startup.rs +++ b/src/startup.rs @@ -10,6 +10,7 @@ use crate::{ }, config::{self, Profile, Scope, Settings}, context, + error::CredentialSources, output::{Envelope, Output, OutputFormat, report_error, write_stdout}, pick, price::{PRICE_URL, PriceService}, @@ -138,6 +139,7 @@ struct Session<'a> { settings: Settings, scope: Scope, submission: &'a SubmissionState, + credential_sources: Option, } impl<'a> Session<'a> { @@ -149,11 +151,13 @@ impl<'a> Session<'a> { submission: &'a SubmissionState, ) -> Result { let settings = Settings::open(config::directory(global.config_dir.clone())?)?; - let mut scope = settings.scope(global.scope_selection())?; + let (mut scope, sources) = settings.sourced_scope(global.scope_selection())?; + let credential_sources = auth::environment_key_sources(global, &sources); let resolver = Resolver { settings: &settings, flags: global, submission, + credential_sources, }; pick::fill(command, global, resolver, &mut scope).await?; Ok(Self { @@ -161,6 +165,7 @@ impl<'a> Session<'a> { settings, scope, submission, + credential_sources, }) } @@ -169,6 +174,7 @@ impl<'a> Session<'a> { settings: &self.settings, flags: self.global, submission: self.submission, + credential_sources: self.credential_sources, } } diff --git a/src/wallet_currency.rs b/src/wallet_currency.rs index 7c24041..f6fc591 100644 --- a/src/wallet_currency.rs +++ b/src/wallet_currency.rs @@ -39,7 +39,8 @@ pub async fn fill( .resolve_organization(scope) .await .map_err(|error| error.and_hint("Or pass --currency to skip reading the wallet."))?; - let currency = api::wallet_currency(global, credential, org, wallet).await?; + let currency = + api::wallet_currency(global, credential, resolver.credential_sources, org, wallet).await?; check_unit(flags, currency)?; global.terminal().notice(format!( "Using the wallet's currency, {currency}; pass --currency to choose another." diff --git a/tests/cli_integration.rs b/tests/cli_integration.rs index 1db41a8..35f4dbf 100644 --- a/tests/cli_integration.rs +++ b/tests/cli_integration.rs @@ -1588,6 +1588,223 @@ async fn logout_retains_credentials_on_revocation_failure_and_local_is_explicit( assert!(settings.read_credential(LOGIN_NAME).is_err()); } +/// A device login that is approved at once and signs in as `user_id`. +async fn approved_device_login(server: &MockServer, user_id: &str) { + respond_once( + server, + route("POST", "/oauth/device_authorization"), + ok(json!({ + "device_code":"device-secret","user_code":"ABCD-EFGH", + "verification_uri":"https://app.voltage.cloud/cli/authorize","expires_in":60,"interval":5 + })), + ) + .await; + respond_once( + server, + route("POST", "/oauth/token"), + ok(json!({ + "access_token":"new-access","refresh_token":"new-refresh", + "token_type":"Bearer","expires_in":600 + })), + ) + .await; + respond_once( + server, + route("GET", "/users/current").and(header("authorization", "Bearer new-access")), + ok(json!({"id":user_id,"email":LOGIN_EMAIL})), + ) + .await; +} + +fn login_as(dir: &Path, server: &MockServer, auth_url: &str) -> Command { + let mut cmd = replacing_login(dir, server, auth_url); + cmd.args(["--credential-store", "file"]); + cmd +} + +/// A login under the saved name without `--credential-store`, so it keeps the saved store. +fn replacing_login(dir: &Path, server: &MockServer, auth_url: &str) -> Command { + let mut cmd = cli(dir, server); + cmd.args([ + "login", + "--auth-url", + auth_url, + "--no-browser", + "--account", + LOGIN_NAME, + ]); + cmd +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_login_of_the_same_user_replaces_the_saved_one_and_revokes_it() { + for revoked in [true, false] { + let (server, dir) = fixture().await; + let mut settings = seeded_login(dir.path(), &server, true); + approved_device_login(&server, RESOURCE).await; + respond_once( + &server, + route("POST", "/oauth/revoke") + .and(body_string_contains(format!("token={LOGIN_REFRESH}"))), + if revoked { + ResponseTemplate::new(200) + } else { + ResponseTemplate::new(503).set_body_json(json!({"error":"unavailable"})) + }, + ) + .await; + // Without `--credential-store`, the file-stored login stays in its file. + let login = replacing_login(dir.path(), &server, &server.uri()) + .assert() + .success(); + assert_eq!( + json_stdout(&login)["data"], + json!({"account":LOGIN_NAME,"email":LOGIN_EMAIL,"authenticated":true,"replaced":true}) + ); + settings.reload().unwrap(); + assert_eq!( + settings.account(LOGIN_NAME).unwrap().store, + CredentialStore::File + ); + assert_eq!( + login_tokens(&settings, LOGIN_NAME), + ("new-access".to_owned(), "new-refresh".to_owned()) + ); + // A failed revocation keeps the new login and says how to end the old session. + assert_eq!( + stderr(&login).contains("Could not revoke the replaced CLI session"), + !revoked, + "{}", + stderr(&login) + ); + server.verify().await; + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_login_of_another_user_keeps_the_saved_one_and_revokes_the_new_session() { + let (server, dir) = fixture().await; + let settings = seeded_login(dir.path(), &server, true); + approved_device_login(&server, ORG).await; + respond_once( + &server, + route("POST", "/oauth/revoke").and(body_string_contains("token=new-refresh")), + ResponseTemplate::new(200), + ) + .await; + let login = login_as(dir.path(), &server, &server.uri()) + .assert() + .code(2); + assert_eq!( + json_error(&login)["error"]["message"], + format!( + "Credential {LOGIN_NAME} already exists; log it out first or use a different --account name" + ) + ); + assert_eq!( + login_tokens(&settings, LOGIN_NAME), + (LOGIN_ACCESS.to_owned(), LOGIN_REFRESH.to_owned()) + ); + server.verify().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_login_at_another_auth_url_is_refused_before_the_browser_step() { + let (server, dir) = fixture().await; + let settings = seeded_login(dir.path(), &server, true); + let other = MockServer::start().await; + let login = login_as(dir.path(), &server, &other.uri()).assert().code(2); + assert_eq!( + json_error(&login)["error"]["message"], + format!( + "Credential {LOGIN_NAME} already exists; log it out first or use a different --account name" + ) + ); + assert!(other.received_requests().await.unwrap().is_empty()); + assert!(server.received_requests().await.unwrap().is_empty()); + assert_eq!(login_tokens(&settings, LOGIN_NAME).1, LOGIN_REFRESH); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_saved_login_without_a_user_id_is_not_replaced() { + let (server, dir) = fixture().await; + let settings = seeded_login(dir.path(), &server, true); + settings + .write_credential( + LOGIN_NAME, + CredentialStore::File, + &Credential::Login(Login { + access_token: Secret::new(LOGIN_ACCESS.into()), + refresh_token: Secret::new(LOGIN_REFRESH.into()), + expires_at: 0, + user_id: None, + email: Some(LOGIN_EMAIL.into()), + }), + ) + .unwrap(); + approved_device_login(&server, RESOURCE).await; + respond_once( + &server, + route("POST", "/oauth/revoke").and(body_string_contains("token=new-refresh")), + ResponseTemplate::new(200), + ) + .await; + let login = login_as(dir.path(), &server, &server.uri()) + .assert() + .code(2); + assert_eq!( + json_error(&login)["error"]["message"], + format!( + "Credential {LOGIN_NAME} already exists; log it out first or use a different --account name" + ) + ); + assert_eq!(login_tokens(&settings, LOGIN_NAME).1, LOGIN_REFRESH); + server.verify().await; +} + +/// A saved login whose secret is corrupt or gone is refused before the browser step, with +/// the command that removes it, and that command works. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn a_saved_login_that_cannot_be_read_is_refused_before_the_browser_step() { + for corrupt in [true, false] { + let (server, dir) = fixture().await; + seeded_login(dir.path(), &server, true); + let mut files = std::fs::read_dir(dir.path().join("credentials")).unwrap(); + let file = files.next().unwrap().unwrap().path(); + assert!(files.next().is_none()); + if corrupt { + std::fs::write(&file, "not json").unwrap(); + } else { + std::fs::remove_file(&file).unwrap(); + } + let login = login_as(dir.path(), &server, &server.uri()) + .assert() + .code(2); + let report = json_error(&login); + assert_eq!( + report["error"]["message"], + format!( + "Credential {LOGIN_NAME} already exists; log it out first or use a different --account name" + ) + ); + assert_eq!( + report["error"]["hint"], + format!( + "Its saved secret cannot be read. Remove it with `voltage logout --local --account {LOGIN_NAME}`." + ) + ); + assert!(server.received_requests().await.unwrap().is_empty()); + cli(dir.path(), &server) + .args(["logout", "--account", LOGIN_NAME, "--local"]) + .assert() + .success(); + assert!(!file.exists()); + let mut settings = settings_at(dir.path()); + settings.reload().unwrap(); + assert!(settings.config.accounts.is_empty()); + } +} + // --------------------------------------------------------------------------------------- // Payments: submission, confirmation, waits, and reconciliation // --------------------------------------------------------------------------------------- @@ -2077,6 +2294,47 @@ async fn interrupt_before_a_new_login_is_saved_says_a_session_may_exist() { server.verify().await; } +/// Once the new login is saved, an interruption while the replaced session is revoked says +/// the new login is saved, not that a session may have gone unsaved. +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn interrupt_while_revoking_a_replaced_login_says_the_new_one_is_saved() { + let (server, dir) = fixture().await; + let mut settings = seeded_login(dir.path(), &server, true); + approved_device_login(&server, RESOURCE).await; + let (responder, arrived) = announced(ok(json!({})).set_delay(Duration::from_secs(10))); + respond_once( + &server, + route("POST", "/oauth/revoke").and(body_string_contains(format!("token={LOGIN_REFRESH}"))), + responder, + ) + .await; + let output = interrupt_in_flight( + dir.path(), + &server, + &arrived, + &[ + "login", + "--auth-url", + &server.uri(), + "--no-browser", + "--account", + LOGIN_NAME, + ], + ) + .await; + let report = error_report(&output.stderr); + assert_eq!( + report["error"]["message"], + format!( + "Interrupted while revoking the replaced session for {LOGIN_NAME}; the new login is saved" + ) + ); + settings.reload().unwrap(); + assert_eq!(login_tokens(&settings, LOGIN_NAME).1, "new-refresh"); + server.verify().await; +} + #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn interrupt_during_a_read_does_not_claim_a_change_was_submitted() { @@ -3457,12 +3715,13 @@ fn json_error(assertion: &Assert) -> Value { async fn rejected_credentials_name_the_command_that_checks_them() { for (status, next) in [(401, "voltage auth status"), (403, "voltage context")] { let (server, dir) = fixture().await; + seeded_api_key(dir.path(), "saved", ACCOUNT_KEY); route("GET", wallets_path(ORG)) .respond_with(ResponseTemplate::new(status)) .mount(&server) .await; let result = cli(dir.path(), &server) - .args(["wallets", "list", "--org", ORG]) + .args(["wallets", "list", "--account", "saved", "--org", ORG]) .assert() .code(3); let error = json_error(&result); @@ -3470,9 +3729,119 @@ async fn rejected_credentials_name_the_command_that_checks_them() { error["error"]["hint"].as_str().unwrap().contains(next), "{error}" ); + // A saved credential is the one the user chose, so no sources are reported. + assert!(error["error"]["detail"].get("credential_sources").is_none()); + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn a_forbidden_environment_key_names_where_it_and_the_organization_came_from() { + for (from_flag, organization, named) in [ + (true, "flag", " and the organization from --org"), + ( + false, + "environment", + " and the organization from VOLTAGE_ORGANIZATION_ID", + ), + ] { + let (server, dir) = fixture().await; + route("GET", wallets_path(ORG)) + .respond_with(ResponseTemplate::new(403).set_body_json(json!({"error": { + "type": "access_rights", "detail": "No Access" + }}))) + .mount(&server) + .await; + let mut command = cli(dir.path(), &server); + command.args(["wallets", "list"]); + if from_flag { + command.args(["--org", ORG]); + } else { + command.env("VOLTAGE_ORGANIZATION_ID", ORG); + } + let error = json_error(&command.assert().code(3)); + assert_eq!( + error["error"]["hint"], + format!( + "The API key came from VOLTAGE_API_KEY{named}. Check that the key can reach the organization, environment, and wallet in use, which `voltage context` shows, or pass --account NAME or unset VOLTAGE_API_KEY to use a saved login." + ) + ); + assert_eq!( + error["error"]["detail"]["credential_sources"], + json!({"credential": "environment", "organization": organization}) + ); } } +#[tokio::test(flavor = "multi_thread")] +async fn an_unauthorized_environment_key_reports_no_sources() { + let (server, dir) = fixture().await; + route("GET", wallets_path(ORG)) + .respond_with(ResponseTemplate::new(401)) + .mount(&server) + .await; + let result = cli(dir.path(), &server) + .args(["wallets", "list", "--org", ORG]) + .assert() + .code(3); + let error = json_error(&result); + assert_eq!( + error["error"]["hint"], + "Check the key with `voltage auth status`; the API does not accept it." + ); + assert!(error["error"]["detail"].get("credential_sources").is_none()); +} + +#[tokio::test(flavor = "multi_thread")] +async fn a_forbidden_environment_key_without_a_json_body_still_names_its_sources() { + let (server, dir) = fixture().await; + route("GET", wallets_path(ORG)) + .respond_with(ResponseTemplate::new(403).set_body_raw("denied", "text/html")) + .mount(&server) + .await; + let result = cli(dir.path(), &server) + .args(["wallets", "list", "--org", ORG]) + .assert() + .code(3); + let error = json_error(&result); + assert_eq!( + error["error"]["message"], + "HTTP 403: non-JSON response omitted" + ); + assert!( + error["error"]["hint"] + .as_str() + .unwrap() + .starts_with("The API key came from VOLTAGE_API_KEY and the organization from --org."), + "{error}" + ); + assert_eq!( + error["error"]["detail"]["credential_sources"], + json!({"credential": "environment", "organization": "flag"}) + ); +} + +#[tokio::test(flavor = "multi_thread")] +async fn an_unrecognized_environment_key_on_a_403_reports_no_sources() { + let (server, dir) = fixture().await; + route("GET", wallets_path(ORG)) + .respond_with(ResponseTemplate::new(403).set_body_json(json!({"error": { + "type": "missing_credentials", "detail": "Missing credential" + }}))) + .mount(&server) + .await; + let result = cli(dir.path(), &server) + .args(["wallets", "list", "--org", ORG]) + .assert() + .code(3); + let error = json_error(&result); + // The rejection is about the key itself, so the hint and the sources agree with a 401. + assert_eq!( + error["error"]["hint"], + "Check the key with `voltage auth status`; the API does not accept it." + ); + assert!(error["error"]["detail"].get("credential_sources").is_none()); +} + #[tokio::test(flavor = "multi_thread")] async fn a_revoked_login_names_the_command_that_renews_it() { // An expiring login fails at refresh; a session revoked before its access token @@ -3499,11 +3868,16 @@ async fn a_revoked_login_names_the_command_that_renews_it() { assert_eq!( error["error"]["hint"], format!( - "Remove it with `voltage logout --local --account {LOGIN_NAME}`, then sign in again with `voltage login --account {LOGIN_NAME} --auth-url {}`.", + "Sign in again with `voltage login --account {LOGIN_NAME} --auth-url {}`.", server.uri() ), "expiring={expiring}" ); + assert_eq!( + error["error"]["detail"], + json!({"credential_problem": "login_expired"}), + "expiring={expiring}" + ); } }