From 762363e5a9c8632798fa6301d4378935b5676f77 Mon Sep 17 00:00:00 2001 From: "blu@blu.cx" <38222767+thebrandonlucas@users.noreply.github.com> Date: Tue, 20 Oct 2026 13:47:59 -0500 Subject: [PATCH] feat(cli): replace a saved login when the same user signs in again `voltage login` under a saved name now replaces that login when the new session is for the same user at the same auth URL, under the credential lock, and revokes the replaced refresh token. A different user, or a saved login without a user ID, is still refused, and the new session is revoked. With `--account NAME`, a name saved for an API key, a login at another auth URL, or a login whose secret cannot be read is refused before the browser step. An unreadable secret is refused as a name in use with a `voltage logout --local --account NAME` hint, and `logout --local` removes an account whose secret is already gone, or warns when the secret cannot be removed. The expired-login hint is now only `voltage login --account NAME`. Without `--credential-store`, a replaced login keeps its store and a new login uses the keychain. Any failed secret or configuration write puts the replaced login back, including a write that already renamed its file, and the error says when the replaced login could not be restored. A Ctrl-C while the replaced session is revoked says the new login is saved. `shell_word` moves to `config`, which no longer imports `auth`. Closes #48 --- CHANGELOG.md | 1 + docs/guides/login-and-profiles.md | 2 + docs/guides/troubleshooting.md | 5 +- docs/reference/exit-codes.md | 2 +- src/api.rs | 22 +- src/auth.rs | 99 +++-- src/cli.rs | 8 +- src/config.rs | 585 ++++++++++++++++++++++++++++-- tests/cli_integration.rs | 260 ++++++++++++- 9 files changed, 919 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd89140..248383a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ User-visible changes to `voltage`, newest first. Each pull request that changes ### 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. - 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`. diff --git a/docs/guides/login-and-profiles.md b/docs/guides/login-and-profiles.md index 74a198a..32afd53 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`: diff --git a/docs/guides/troubleshooting.md b/docs/guides/troubleshooting.md index f5b4d2b..b5b4e92 100644 --- a/docs/guides/troubleshooting.md +++ b/docs/guides/troubleshooting.md @@ -60,14 +60,13 @@ An imported API key belongs to one organization and environment, and the command ### The saved login has expired or was revoked -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: +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. ### needs a browser login diff --git a/docs/reference/exit-codes.md b/docs/reference/exit-codes.md index 704770b..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 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. +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 a7b00bd..0ab280c 100644 --- a/src/api.rs +++ b/src/api.rs @@ -12,7 +12,7 @@ use crate::{ cli::{ApiInvocation, GlobalFlags, Origin, Requested}, config::{ API_URL, EXECUTE_VARIABLE, Scope, Settings, Source, new_private, private_dir, read_private, - read_secret, + read_secret, shell_word, }, dry_run, error::{CredentialSources, ErrorDetail, ErrorKind}, @@ -107,7 +107,7 @@ impl Authorization { "Check the key with `voltage auth status`; the API does not accept it." } (Self::Account(ApiCredential::OrganizationToken(_), _), 401) => { - "Check the credential with `voltage auth status`. To sign in again, run `voltage logout --local --account NAME`, then `voltage login --account NAME`." + "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()); @@ -951,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() } @@ -1116,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 { @@ -1167,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"), }; }; @@ -1619,8 +1625,7 @@ mod tests { ); 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")); @@ -1941,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 c575f40..57cfe46 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -9,8 +9,9 @@ use crate::{ api::{SessionChange, SubmissionState}, cli::{GlobalFlags, ImportKeyFlags, LoginFlags}, config::{ - AUTH_URL, Account, AccountKind, Credential, CredentialProblem, InputSource, LOCK_TIMEOUT, - Login, Scope, ScopeSources, Settings, Source, 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::CredentialSources, secret::Secret, @@ -505,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)] @@ -662,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()?; @@ -679,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() { @@ -700,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); @@ -711,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), @@ -748,25 +804,12 @@ 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. + // 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. @@ -935,7 +978,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(); @@ -1089,7 +1138,7 @@ mod tests { let default = base_url(AUTH_URL).unwrap(); 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..69ec296 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)] @@ -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!( 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/tests/cli_integration.rs b/tests/cli_integration.rs index 2219227..a9e941c 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() { @@ -3610,7 +3868,7 @@ 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}"