feat(cli): replace a saved login when the same user signs in again

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`.

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`:

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
 

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.

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();

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<String>,
     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<LoginOutcome> {
     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<LoginOutcome> {
     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<Login> {
     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(

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<CredentialStore>,
 }
 
 #[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!(