From 046dc717234010388cc08df7560ef6a461b7301a Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Tue, 22 Sep 2026 23:35:05 -0400 Subject: [PATCH 1/3] Add mapbox auth profiles: list every stored credential profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --profile selects a named credential file, but nothing lists which profiles actually exist. Answering that meant `ls ~/.mapbox/credentials-*.json` by hand, and knowing the naming convention to tell the default profile apart from a named one. `mapbox auth profiles` reads the config directory rather than resolving --token/the environment/a single stored profile the way `whoami` does — it answers "what is stored" rather than "what will the next command use", which is a different question with a different audience: someone who has forgotten which named profiles they have logged into. Read-only, like whoami: it loads credentials without refreshing them, so listing profiles cannot itself spend a single-use refresh token. Reports each profile's account (the stored username, falling back to the token's own `u` claim) and expiry, decoded locally the same way whoami's fields are. profile_name_from_filename is credentials_filename's reverse, tested against credentials_filename's own output rather than only hand-written examples so the two cannot quietly drift apart, and checked against every other file a config directory can hold (config.json, update-check.json, a lock file) so none of them are mistaken for a profile. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 6 ++ docs/commands.md | 64 ++++++++++++++- src/auth.rs | 166 +++++++++++++++++++++++++++++++++++++++ src/main.rs | 14 +++- tests/auth_profiles.rs | 161 +++++++++++++++++++++++++++++++++++++ tests/schema_contract.rs | 3 +- 6 files changed, 411 insertions(+), 3 deletions(-) create mode 100644 tests/auth_profiles.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 496775d..a0c2b6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,12 @@ that may never merge. They are not releases and are not listed here. ### Added +- `mapbox auth profiles` — lists every credential profile stored on disk, + not just the one `--profile` would select. Read-only, like `whoami`, and + answers a different question than it does: `whoami` reports which token + the *next* command will use, this reports what's stored at all, for + someone who has forgotten which named profiles they've logged into. + - The README now documents installing without the install script: the archives are plain HTTP downloads, `manifest.json` lists every target with its checksum, and the commands to verify and extract one are written out. diff --git a/docs/commands.md b/docs/commands.md index 03758e3..f00033c 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -45,7 +45,7 @@ nests, and is typed `mapbox styles draft get`. **[Auth](#auth)** — [auth.login](#mapbox-auth-login) · [auth.logout](#mapbox-auth-logout) · [auth.refresh](#mapbox-auth-refresh) · -[auth.whoami](#mapbox-auth-whoami) +[auth.whoami](#mapbox-auth-whoami) · [auth.profiles](#mapbox-auth-profiles) **[Agent skills](#agent-skills)** — [agent-skills.list](#mapbox-agent-skills-list) · @@ -342,6 +342,68 @@ Docs: https://docs.mapbox.com/api/accounts/tokens/ +### `mapbox auth profiles` + +Lists every profile with credentials stored on disk — not just the one +`--profile` would select. `whoami` answers which token the *next* command +will use; this answers what is stored at all, for someone who has forgotten +which named profiles they have logged into. + +Read-only, like `whoami`: it reads the stored credentials without +refreshing, so listing profiles cannot spend a single-use refresh token. +Unlike `whoami`, it never resolves `--token` or the environment — a typed +flag or `MAPBOX_ACCESS_TOKEN` would outrank every stored profile for the +*next* command, but neither has anything to do with what is on disk. + +#### Parameters + +None. There is no `--dry-run`: the command only reads the store. + +#### Examples + +```sh +mapbox auth profiles +``` + +#### Outputs + + + + +
Terminal — -o textAgent — -o json
+ +``` +default user expires in 59 minutes +work work-user +``` + + + +```json +[{"account":"user","expires_at":1788276540,"profile":"default"},{"account":"work-user","expires_at":null,"profile":"work"}] +``` + +
+ +With nothing stored: + + + + +
Terminal — -o textAgent — -o json
+ +``` +No stored profiles. Run `mapbox auth login` to create one. +``` + + + +```json +[] +``` + +
+ --- ## API command groups diff --git a/src/auth.rs b/src/auth.rs index bd81c80..7b363e1 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -457,6 +457,61 @@ fn credentials_path(profile: Option<&str>) -> Result { Ok(config_dir()?.join(credentials_filename(profile)?)) } +/// The reverse of [`credentials_filename`]: the profile name a credentials +/// filename would have been written under, or `None` for anything else in +/// the config directory — `credentials-.json.lock`, `config.json`, +/// `update-check.json`. Pure, and tested against `credentials_filename`'s own +/// output rather than only against hand-written examples, so the two cannot +/// quietly drift apart. +fn profile_name_from_filename(filename: &str) -> Option { + if filename == "credentials.json" { + return Some("default".to_string()); + } + let name = filename + .strip_prefix("credentials-")? + .strip_suffix(".json")?; + (!name.is_empty()).then(|| name.to_string()) +} + +/// Every profile with a credentials file on disk, sorted with `default` +/// first and everything else alphabetically after it. +/// +/// Reads rather than creates: [`config_dir_path`], not [`config_dir`], so +/// asking what is stored is not itself the reason a directory starts to +/// exist. An absent directory reads the same as an empty one. +fn list_profile_names() -> Result> { + let Some(dir) = config_dir_path() else { + return Ok(vec![]); + }; + let entries = match std::fs::read_dir(&dir) { + Ok(entries) => entries, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(vec![]), + Err(e) => { + return Err(e).with_context(|| format!("Failed to read {}", dir.display())); + } + }; + + let mut names = vec![]; + for entry in entries { + let entry = + entry.with_context(|| format!("Failed to read an entry in {}", dir.display()))?; + if let Some(name) = entry + .file_name() + .to_str() + .and_then(profile_name_from_filename) + { + names.push(name); + } + } + names.sort_unstable_by(|a, b| match (a.as_str(), b.as_str()) { + ("default", "default") => std::cmp::Ordering::Equal, + ("default", _) => std::cmp::Ordering::Less, + (_, "default") => std::cmp::Ordering::Greater, + _ => a.cmp(b), + }); + Ok(names) +} + /// One lock file per profile, so refreshing profile A never blocks profile B. fn lock_filename(profile: Option<&str>) -> Result { Ok(format!("{}.lock", credentials_filename(profile)?)) @@ -1301,6 +1356,81 @@ fn profile_name(profile: Option<&str>) -> &str { profile.unwrap_or("default") } +/// One stored profile's summary, for [`profiles`]. +struct ProfileEntry { + name: String, + /// The credentials' own `username` field when `login` recorded one, + /// falling back to the token's `u` claim — the same fallback `whoami` + /// draws on, for a token this crate wrote before that field existed. + account: Option, + expires_at: Option, +} + +impl ProfileEntry { + fn text(&self, now: u64) -> String { + let account = self.account.as_deref().unwrap_or("unknown"); + match self.expires_at { + Some(exp) => format!("{}\t{account}\texpires {}", self.name, time_until(exp, now)), + None => format!("{}\t{account}", self.name), + } + } + + fn json(&self) -> Value { + json!({ + "profile": self.name, + "account": self.account, + "expires_at": self.expires_at, + }) + } +} + +/// `mapbox auth profiles` — every credentials file on disk, not just the one +/// `--profile` would select. +/// +/// Read-only, like `whoami`: it never refreshes, so listing profiles cannot +/// itself spend a single-use refresh token. Unlike `whoami`, it does not +/// resolve `--token` or the environment at all — those answer "what will the +/// next command use", and this answers "what is stored", which is a +/// different question with a different audience: someone who has forgotten +/// which named profiles they have logged into. +pub fn profiles(mode: Mode) -> Result<()> { + let names = list_profile_names()?; + + let entries: Vec = names + .into_iter() + .map(|name| { + let selector = (name != "default").then(|| name.clone()); + let stored = load_credentials(selector.as_deref()); + let account = stored + .as_ref() + .and_then(|c| c.username.clone()) + .or_else(|| stored.as_ref().and_then(|c| token_account(&c.access_token))); + let expires_at = stored + .as_ref() + .and_then(|c| token_expires_at(&c.access_token)); + ProfileEntry { + name, + account, + expires_at, + } + }) + .collect(); + + let now = now(); + let text = if entries.is_empty() { + "No stored profiles. Run `mapbox auth login` to create one.".to_string() + } else { + entries + .iter() + .map(|entry| entry.text(now)) + .collect::>() + .join("\n") + }; + let json = Value::Array(entries.iter().map(ProfileEntry::json).collect()); + + output::emit(mode, &text, json) +} + /// What an `auth` command would do, for `--dry-run`. /// /// One function rather than three, so the three answers keep one shape: the @@ -2930,6 +3060,42 @@ mod tests { ); } + /// The reverse of the two tests above: every filename `credentials_filename` + /// can produce reads back to the profile that produced it, rather than + /// only checked against hand-written examples that could quietly drift + /// from what the forward direction actually writes. + #[test] + fn profile_name_from_filename_reverses_credentials_filename() { + for profile in [None, Some("default"), Some("android_app"), Some("a")] { + let filename = credentials_filename(profile).unwrap(); + assert_eq!( + profile_name_from_filename(&filename).as_deref(), + Some(profile_name(profile)), + "{filename:?} did not reverse to {:?}", + profile_name(profile) + ); + } + } + + #[test] + fn profile_name_from_filename_ignores_everything_else_in_the_directory() { + for other in [ + "credentials-android_app.json.lock", + "config.json", + "update-check.json", + "credentials.json.lock", + "credentials-.json", + "credentials-", + "not-credentials-at-all.json", + ] { + assert_eq!( + profile_name_from_filename(other), + None, + "{other:?} should not read as a profile" + ); + } + } + /// Proves the create-time mode, not a chmod applied afterwards: the mode is /// read straight after `open(2)` returns, before `write_private` would run /// any `set_permissions`. diff --git a/src/main.rs b/src/main.rs index cf56b23..414857a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -567,6 +567,17 @@ fn build_app(specs: &[ServiceSpec]) -> Command { cannot: a revoked token still looks perfectly valid", ), ), + ) + .subcommand( + // No `--dry-run`: read-only, like `whoami`. + Command::new("profiles") + .about("List every stored credential profile") + .long_about( + "List every profile with credentials stored on disk — not just the \ + one `--profile` would select. `whoami` answers which token the next \ + command will use; this answers what is stored at all, for someone \ + who has forgotten which named profiles they have logged into.", + ), ), ); @@ -1088,6 +1099,7 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - profile, mode, )?, + Some(("profiles", _)) => auth::profiles(mode)?, _ => unreachable!("`auth` sets subcommand_required(true)"), }, Some((svc_name, svc_matches)) => { @@ -1789,7 +1801,7 @@ mod tests { #[test] fn the_auth_subcommands_that_write_offer_dry_run() { const WRITES: [&str; 3] = ["login", "logout", "refresh"]; - const READS: [&str; 1] = ["whoami"]; + const READS: [&str; 2] = ["whoami", "profiles"]; let specs = bundled_specs(); let app = build_app(&specs); diff --git a/tests/auth_profiles.rs b/tests/auth_profiles.rs new file mode 100644 index 0000000..c372643 --- /dev/null +++ b/tests/auth_profiles.rs @@ -0,0 +1,161 @@ +//! End-to-end tests for `mapbox auth profiles`. +//! +//! The reverse-filename-parsing logic is unit-tested in `src/auth.rs`. What +//! that cannot show is the real binary reading a real config directory: the +//! default profile alongside named ones, a profile with no `exp` claim next +//! to one that has one, and files that must not be mistaken for a profile at +//! all — `config.json`, a lock file, `update-check.json`. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine as _}; + +fn scratch(name: &str) -> PathBuf { + let home = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(format!("auth-profiles-{name}")); + let _ = std::fs::remove_dir_all(&home); + std::fs::create_dir_all(home.join(".mapbox")).expect("create the scratch config dir"); + home +} + +fn config_dir(home: &Path) -> PathBuf { + home.join(".mapbox") +} + +/// A token shaped the way this crate's own decoders expect: +/// `..` — see `token_account` and +/// `token_expires_at` in `src/auth.rs`, which read position 1 as the +/// payload. `exp` is omitted entirely when `None`, the same as a `pk`/`sk` +/// token that carries no expiry. +fn token(account: &str, exp: Option) -> String { + let payload = match exp { + Some(exp) => format!(r#"{{"u":"{account}","exp":{exp}}}"#), + None => format!(r#"{{"u":"{account}"}}"#), + }; + format!("sk.{}.sig", URL_SAFE_NO_PAD.encode(payload)) +} + +fn write_credentials(dir: &Path, filename: &str, account: &str, exp: Option) { + let json = format!( + r#"{{"access_token":"{}","username":"{account}"}}"#, + token(account, exp) + ); + std::fs::write(dir.join(filename), json).expect("write a fake credentials file"); +} + +fn command(home: &Path) -> Command { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_mapbox")); + cmd.env_remove("MAPBOX_ACCESS_TOKEN") + .env_remove("MapboxAccessToken") + .env_remove("MAPBOX_USERNAME") + .env_remove("MAPBOX_OUTPUT") + .env("HOME", home) + .env("XDG_CONFIG_HOME", home.join(".config")) + .env("MAPBOX_CONFIG_DIR", config_dir(home)); + cmd +} + +fn stdout(output: &Output) -> String { + String::from_utf8_lossy(&output.stdout) + .trim_end() + .to_string() +} + +#[test] +fn no_stored_profiles_says_so_rather_than_an_empty_table() { + let home = scratch("none"); + + let text = command(&home) + .args(["-o", "text", "auth", "profiles"]) + .output() + .expect("run mapbox auth profiles"); + assert!(text.status.success()); + assert!( + stdout(&text).contains("No stored profiles"), + "{}", + stdout(&text) + ); + + let json = command(&home) + .args(["-o", "json", "auth", "profiles"]) + .output() + .expect("run mapbox auth profiles -o json"); + assert!(json.status.success()); + assert_eq!(stdout(&json), "[]"); +} + +#[test] +fn every_stored_profile_is_listed_default_first() { + let home = scratch("populated"); + let dir = config_dir(&home); + write_credentials(&dir, "credentials.json", "alice", Some(4_102_444_800)); + write_credentials(&dir, "credentials-work.json", "bob-work", None); + // A profile named so it sorts before "default" alphabetically, to prove + // the ordering is deliberate rather than incidentally alphabetical. + write_credentials(&dir, "credentials-acme.json", "acme-bot", None); + + let json = command(&home) + .args(["-o", "json", "auth", "profiles"]) + .output() + .expect("run mapbox auth profiles -o json"); + assert!( + json.status.success(), + "{}", + String::from_utf8_lossy(&json.stderr) + ); + + let parsed: serde_json::Value = serde_json::from_str(&stdout(&json)).expect("valid JSON"); + let entries = parsed.as_array().expect("a JSON array"); + assert_eq!(entries.len(), 3); + assert_eq!(entries[0]["profile"], "default"); + assert_eq!(entries[0]["account"], "alice"); + assert!(entries[0]["expires_at"].as_u64().is_some()); + assert_eq!(entries[1]["profile"], "acme"); + assert_eq!(entries[2]["profile"], "work"); + assert_eq!(entries[2]["account"], "bob-work"); + assert!(entries[2]["expires_at"].is_null()); +} + +#[test] +fn only_credentials_files_count_as_a_profile() { + let home = scratch("noise"); + let dir = config_dir(&home); + write_credentials(&dir, "credentials.json", "alice", None); + // Everything a real config directory can hold that is not a profile. + std::fs::write(dir.join("credentials.json.lock"), "").unwrap(); + std::fs::write(dir.join("credentials-work.json.lock"), "").unwrap(); + std::fs::write(dir.join("config.json"), r#"{"update_check":false}"#).unwrap(); + std::fs::write(dir.join("update-check.json"), "{}").unwrap(); + + let json = command(&home) + .args(["-o", "json", "auth", "profiles"]) + .output() + .expect("run mapbox auth profiles -o json"); + assert!(json.status.success()); + let parsed: serde_json::Value = serde_json::from_str(&stdout(&json)).expect("valid JSON"); + let entries = parsed.as_array().expect("a JSON array"); + assert_eq!(entries.len(), 1, "{entries:?}"); + assert_eq!(entries[0]["profile"], "default"); +} + +#[test] +fn an_absent_config_directory_lists_nothing_and_creates_none() { + let home = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join("auth-profiles-absent"); + let _ = std::fs::remove_dir_all(&home); + std::fs::create_dir_all(&home).expect("create the scratch home, with no .mapbox inside it"); + + let json = command(&home) + .args(["-o", "json", "auth", "profiles"]) + .output() + .expect("run mapbox auth profiles -o json"); + assert!( + json.status.success(), + "{}", + String::from_utf8_lossy(&json.stderr) + ); + assert_eq!(stdout(&json), "[]"); + assert!( + !config_dir(&home).exists(), + "listing profiles must not create the config directory" + ); +} diff --git a/tests/schema_contract.rs b/tests/schema_contract.rs index ab710fc..03984ec 100644 --- a/tests/schema_contract.rs +++ b/tests/schema_contract.rs @@ -344,7 +344,8 @@ fn the_hand_written_commands_describe_themselves() { "mapbox auth login", "mapbox auth logout", "mapbox auth refresh", - "mapbox auth whoami" + "mapbox auth whoami", + "mapbox auth profiles" ] ); assert!(commands(&value) From 40cf33bcf65e58dc2789022fa98633e513a8f1d9 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 23 Sep 2026 09:12:13 -0400 Subject: [PATCH 2/3] Address review on mapbox auth profiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugs: - A stray credentials-default.json (never a spelling credentials_filename writes) no longer produces a second `default` row with data read from the wrong file. profile_name_from_filename rejects "default" from the credentials-.json pattern, since the real default is always the bare credentials.json. - Listing profiles no longer creates or hardens the config directory as a side effect. load_credentials_readonly/credentials_path_readonly read through config_dir_path rather than config_dir, so asking what is stored is never why a directory starts to exist or its permissions change. - An expired (not clock-skewed) profile now reads as "expired" rather than "check this machine's clock" — that phrasing fit whoami's context (a token about to be used right now, where a small negative result is as likely to be skew as a real expiry) and was wrong far more often than right for a profile that may have sat untouched for weeks. Text output is now a padded table instead of a raw tab-separated line, which rendered misaligned across rows with differently sized account names. --profile is declared globally so it still parses on this command, but it selects a single profile to act on and this command's whole point is every one of them — so typing it now warns on stderr rather than silently doing nothing, the same shape completion::warn_output_ignored already uses for --output. Docs: the auth profiles section now calls out the --profile exception to the page-wide table, the docs_contract.rs comment counting auth subcommands is five now, and README lists the command. Filed mapbox-cli-private#179 for the reviewer's short-profile-ID suggestion, as requested, rather than folding it into this PR. Co-Authored-By: Claude Sonnet 5 --- README.md | 2 + docs/commands.md | 32 +++++++--- src/auth.rs | 123 +++++++++++++++++++++++++++++++++------ src/main.rs | 2 +- tests/auth_profiles.rs | 129 +++++++++++++++++++++++++++++++++++++++++ tests/docs_contract.rs | 6 +- 6 files changed, 264 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 2e3e67c..41a52f1 100644 --- a/README.md +++ b/README.md @@ -144,6 +144,7 @@ mapbox auth login # opens a browser (OAuth/PKCE) mapbox auth logout # removes stored credentials mapbox auth refresh # force-refreshes the access token mapbox auth whoami # reports which token the next command will use +mapbox auth profiles # lists every stored profile, not just one ``` Credentials live in `~/.mapbox` as plain JSON with locked-down file @@ -159,6 +160,7 @@ wants. ```sh mapbox auth login --profile android_app mapbox --profile android_app styles list +mapbox auth profiles # which profiles are actually stored ``` ### API Related diff --git a/docs/commands.md b/docs/commands.md index f00033c..9ebebb1 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -350,10 +350,25 @@ will use; this answers what is stored at all, for someone who has forgotten which named profiles they have logged into. Read-only, like `whoami`: it reads the stored credentials without -refreshing, so listing profiles cannot spend a single-use refresh token. -Unlike `whoami`, it never resolves `--token` or the environment — a typed -flag or `MAPBOX_ACCESS_TOKEN` would outrank every stored profile for the -*next* command, but neither has anything to do with what is on disk. +refreshing, so listing profiles cannot spend a single-use refresh token — +and it reads each one through a path that never creates or hardens the +config directory, so listing what exists is never the reason a directory +starts to exist or its permissions change. Unlike `whoami`, it never +resolves `--token` or the environment — a typed flag or +`MAPBOX_ACCESS_TOKEN` would outrank every stored profile for the *next* +command, but neither has anything to do with what is on disk. + +**`--profile` from [the table above](#what-every-api-command-takes) is the +one exception on this page: this command does not honor it.** That table's +`--profile ` selects which single stored profile a command reads — +the opposite of this command's whole point, which is every one of them at +once. Typing it anyway parses (it is declared globally) but changes +nothing, so it is warned about on stderr rather than silently ignored. + +An expired token reads as `expired`, not with the "check this machine's +clock" phrasing `whoami` uses for a token about to be used right now — a +stored profile may have been sitting untouched for weeks, where that +clock-skew guess would be wrong far more often than right. #### Parameters @@ -363,6 +378,9 @@ None. There is no `--dry-run`: the command only reads the store. ```sh mapbox auth profiles + +# --profile is warned about, not honored — see above +mapbox auth profiles --profile work ``` #### Outputs @@ -372,14 +390,14 @@ mapbox auth profiles ``` -default user expires in 59 minutes -work work-user +default user expires in 58 minutes +work work-user ``` ```json -[{"account":"user","expires_at":1788276540,"profile":"default"},{"account":"work-user","expires_at":null,"profile":"work"}] +[{"account":"user","expires_at":1790172530,"profile":"default"},{"account":"work-user","expires_at":null,"profile":"work"}] ``` diff --git a/src/auth.rs b/src/auth.rs index 7b363e1..c3e42a8 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -457,6 +457,26 @@ fn credentials_path(profile: Option<&str>) -> Result { Ok(config_dir()?.join(credentials_filename(profile)?)) } +/// The same path, through [`config_dir_path`] rather than [`config_dir`] — +/// for a caller that must not create or harden the directory merely by +/// asking what is in it. `None` where [`credentials_filename`] would have +/// erred (an invalid profile name) or the config directory cannot be +/// resolved at all, folding both into "nothing to read" rather than a +/// failure a read-only command has no business raising. +fn credentials_path_readonly(profile: Option<&str>) -> Option { + Some(config_dir_path()?.join(credentials_filename(profile).ok()?)) +} + +/// [`load_credentials`], without creating or hardening the config +/// directory as a side effect of reading it — see +/// [`credentials_path_readonly`]. What [`profiles`] reads each stored +/// profile through, since listing what exists must not be the reason a +/// directory starts to exist or its permissions change. +fn load_credentials_readonly(profile: Option<&str>) -> Option { + let data = std::fs::read_to_string(credentials_path_readonly(profile)?).ok()?; + serde_json::from_str(&data).ok() +} + /// The reverse of [`credentials_filename`]: the profile name a credentials /// filename would have been written under, or `None` for anything else in /// the config directory — `credentials-.json.lock`, `config.json`, @@ -470,7 +490,15 @@ fn profile_name_from_filename(filename: &str) -> Option { let name = filename .strip_prefix("credentials-")? .strip_suffix(".json")?; - (!name.is_empty()).then(|| name.to_string()) + // `credentials_filename` never writes this file — `Some("default")` + // maps to the bare `credentials.json` above, so a + // `credentials-default.json` sitting in the directory is stray, not a + // second profile that happens to share the reserved name. Reading it + // as one would print a `default` row twice: once for the real file, + // once for this one — and the second row's data would come from + // `load_credentials(Some("default"))`, which resolves back to + // `credentials.json` and never touches the file that produced the row. + (!name.is_empty() && name != "default").then(|| name.to_string()) } /// Every profile with a credentials file on disk, sorted with `default` @@ -1367,12 +1395,24 @@ struct ProfileEntry { } impl ProfileEntry { - fn text(&self, now: u64) -> String { - let account = self.account.as_deref().unwrap_or("unknown"); - match self.expires_at { - Some(exp) => format!("{}\t{account}\texpires {}", self.name, time_until(exp, now)), - None => format!("{}\t{account}", self.name), - } + fn account_display(&self) -> &str { + self.account.as_deref().unwrap_or("unknown") + } + + /// Distinct from `time_until`, which `whoami` reads a token through at + /// the moment it is about to be used — where a small negative result is + /// as likely to be clock skew as a real expiry, worth naming as such. A + /// profile in this list may not have been touched in weeks, so guessing + /// "check this machine's clock" here would be wrong far more often than + /// it would be right; a token past its `exp` claim just reads "expired". + fn expiry_prose(&self, now: u64) -> Option { + self.expires_at.map(|expires_at| { + if expires_at > now { + format!("expires {}", time_until(expires_at, now)) + } else { + "expired".to_string() + } + }) } fn json(&self) -> Value { @@ -1384,23 +1424,69 @@ impl ProfileEntry { } } +/// Columns padded to the widest entry, rather than the tab-separated line +/// an earlier version of this printed — tabs render at whatever width a +/// terminal's tab stops happen to be, which is not the same width twice +/// when account names differ in length, and reads as misaligned rather than +/// as a table at all. +fn render_profiles_table(entries: &[ProfileEntry], now: u64) -> String { + let name_width = entries.iter().map(|e| e.name.len()).max().unwrap_or(0); + let account_width = entries + .iter() + .map(|e| e.account_display().len()) + .max() + .unwrap_or(0); + + entries + .iter() + .map(|entry| { + let expiry = entry.expiry_prose(now).unwrap_or_default(); + format!( + "{:name_width$} {:account_width$} {expiry}", + entry.name, + entry.account_display(), + ) + .trim_end() + .to_string() + }) + .collect::>() + .join("\n") +} + /// `mapbox auth profiles` — every credentials file on disk, not just the one /// `--profile` would select. /// /// Read-only, like `whoami`: it never refreshes, so listing profiles cannot -/// itself spend a single-use refresh token. Unlike `whoami`, it does not -/// resolve `--token` or the environment at all — those answer "what will the -/// next command use", and this answers "what is stored", which is a -/// different question with a different audience: someone who has forgotten -/// which named profiles they have logged into. -pub fn profiles(mode: Mode) -> Result<()> { +/// itself spend a single-use refresh token, and it reads each profile +/// through [`load_credentials_readonly`] rather than [`load_credentials`] so +/// listing what exists is never the reason the config directory starts to +/// exist or its permissions change. Unlike `whoami`, it does not resolve +/// `--token` or the environment at all — those answer "what will the next +/// command use", and this answers "what is stored", which is a different +/// question with a different audience: someone who has forgotten which +/// named profiles they have logged into. +/// +/// `--profile` selects *which* stored profile the rest of this CLI reads; +/// it has nothing to select here, since the whole point is every profile at +/// once. Typing it anyway is warned about rather than silently ignored — +/// the same shape `completion::warn_output_ignored` warns about `--output` +/// in, for the same reason: a flag that visibly parses but visibly does +/// nothing is worse than one clap rejects outright. +pub fn profiles(matches: &clap::ArgMatches, mode: Mode) -> Result<()> { + if matches.value_source("profile") == Some(clap::parser::ValueSource::CommandLine) { + eprintln!( + "Warning: `--profile` is not honored by `auth profiles` — it selects which \ + stored profile a command reads, and this lists every one of them at once." + ); + } + let names = list_profile_names()?; let entries: Vec = names .into_iter() .map(|name| { let selector = (name != "default").then(|| name.clone()); - let stored = load_credentials(selector.as_deref()); + let stored = load_credentials_readonly(selector.as_deref()); let account = stored .as_ref() .and_then(|c| c.username.clone()) @@ -1420,11 +1506,7 @@ pub fn profiles(mode: Mode) -> Result<()> { let text = if entries.is_empty() { "No stored profiles. Run `mapbox auth login` to create one.".to_string() } else { - entries - .iter() - .map(|entry| entry.text(now)) - .collect::>() - .join("\n") + render_profiles_table(&entries, now) }; let json = Value::Array(entries.iter().map(ProfileEntry::json).collect()); @@ -3087,6 +3169,9 @@ mod tests { "credentials-.json", "credentials-", "not-credentials-at-all.json", + // Stray — `credentials_filename` never writes this spelling of + // the default profile, so it must not read as a second `default`. + "credentials-default.json", ] { assert_eq!( profile_name_from_filename(other), diff --git a/src/main.rs b/src/main.rs index 414857a..8af7ed6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1099,7 +1099,7 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - profile, mode, )?, - Some(("profiles", _)) => auth::profiles(mode)?, + Some(("profiles", _)) => auth::profiles(matches, mode)?, _ => unreachable!("`auth` sets subcommand_required(true)"), }, Some((svc_name, svc_matches)) => { diff --git a/tests/auth_profiles.rs b/tests/auth_profiles.rs index c372643..2b970dc 100644 --- a/tests/auth_profiles.rs +++ b/tests/auth_profiles.rs @@ -138,6 +138,135 @@ fn only_credentials_files_count_as_a_profile() { assert_eq!(entries[0]["profile"], "default"); } +#[test] +fn a_stray_credentials_default_json_is_not_a_second_default_profile() { + let home = scratch("stray-default"); + let dir = config_dir(&home); + write_credentials(&dir, "credentials.json", "alice", None); + // Not a spelling `credentials_filename` ever writes for the default + // profile — a leftover from somewhere else, or a hand-edited file. + write_credentials(&dir, "credentials-default.json", "someone-else", None); + + let json = command(&home) + .args(["-o", "json", "auth", "profiles"]) + .output() + .expect("run mapbox auth profiles -o json"); + assert!(json.status.success()); + let parsed: serde_json::Value = serde_json::from_str(&stdout(&json)).expect("valid JSON"); + let entries = parsed.as_array().expect("a JSON array"); + assert_eq!( + entries.len(), + 1, + "the stray file must not produce a second `default` row: {entries:?}" + ); + assert_eq!(entries[0]["profile"], "default"); + assert_eq!(entries[0]["account"], "alice"); +} + +#[test] +fn an_explicit_profile_flag_is_warned_about_not_honored() { + let home = scratch("profile-flag"); + write_credentials(&config_dir(&home), "credentials.json", "alice", None); + + let out = command(&home) + .args(["--profile", "work", "auth", "profiles"]) + .output() + .expect("run mapbox --profile work auth profiles"); + assert!(out.status.success()); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("--profile") && stderr.contains("not honored"), + "{stderr}" + ); + // The flag changes nothing about what is listed — every profile still + // appears, not just the one named. + let parsed: serde_json::Value = + serde_json::from_str(&String::from_utf8_lossy(&out.stdout)).expect("valid JSON"); + assert_eq!(parsed.as_array().expect("a JSON array").len(), 1); +} + +#[test] +fn no_warning_without_an_explicit_profile_flag() { + let home = scratch("no-profile-flag"); + write_credentials(&config_dir(&home), "credentials.json", "alice", None); + + let out = command(&home) + .args(["auth", "profiles"]) + .output() + .expect("run mapbox auth profiles"); + assert!(out.status.success()); + assert_eq!(String::from_utf8_lossy(&out.stderr), ""); +} + +#[test] +fn an_expired_profile_reads_as_expired_not_clock_skew() { + let home = scratch("expired"); + // Well in the past — not a value clock skew could plausibly explain. + write_credentials(&config_dir(&home), "credentials.json", "alice", Some(1)); + + let text = command(&home) + .args(["-o", "text", "auth", "profiles"]) + .output() + .expect("run mapbox auth profiles"); + assert!(text.status.success()); + let rendered = stdout(&text); + assert!(rendered.contains("expired"), "{rendered}"); + assert!( + !rendered.contains("check this machine's clock"), + "a profile expired long ago should not blame the clock: {rendered}" + ); +} + +#[test] +fn the_text_table_columns_line_up() { + let home = scratch("table"); + let dir = config_dir(&home); + write_credentials(&dir, "credentials.json", "a", None); + write_credentials(&dir, "credentials-longer-name.json", "bb", None); + + let text = command(&home) + .args(["-o", "text", "auth", "profiles"]) + .output() + .expect("run mapbox auth profiles"); + assert!(text.status.success()); + let rendered = stdout(&text); + let lines: Vec<&str> = rendered.lines().collect(); + assert_eq!(lines.len(), 2, "{lines:?}"); + + // The name column is padded to the widest name — "longer-name" (11 + // characters) — so both rows' account column starts at the same + // offset, two spaces after it. + assert_eq!(&lines[0][0..13], "default ", "{lines:?}"); + assert_eq!(&lines[1][0..13], "longer-name ", "{lines:?}"); + assert!(lines[0][13..].starts_with('a'), "{lines:?}"); + assert!(lines[1][13..].starts_with("bb"), "{lines:?}"); +} + +#[cfg(unix)] +#[test] +fn listing_existing_profiles_does_not_touch_directory_permissions() { + use std::os::unix::fs::PermissionsExt; + + let home = scratch("no-chmod"); + let dir = config_dir(&home); + write_credentials(&dir, "credentials.json", "alice", None); + // Deliberately not 0700, so a `harden_dir` call as a side effect of + // listing would be visible. + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let out = command(&home) + .args(["auth", "profiles"]) + .output() + .expect("run mapbox auth profiles"); + assert!(out.status.success()); + + let mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o755, + "listing profiles must not chmod the config directory" + ); +} + #[test] fn an_absent_config_directory_lists_nothing_and_creates_none() { let home = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join("auth-profiles-absent"); diff --git a/tests/docs_contract.rs b/tests/docs_contract.rs index 87b9f66..467ac51 100644 --- a/tests/docs_contract.rs +++ b/tests/docs_contract.rs @@ -29,9 +29,9 @@ //! * It doesn't check `--dry-run` section by section, which would raise //! that number. Rejected on purpose: the page states a page-wide fact //! once, and 24 near-identical paragraphs is exactly the duplication -//! that design avoids. Where a split needs spelling out — the four -//! `auth` commands, three of which take the flag and one of which -//! doesn't — the page says so in prose instead. +//! that design avoids. Where a split needs spelling out — the five +//! `auth` commands, three of which take the flag and two of which +//! don't — the page says so in prose instead. //! * It doesn't check the captured **Outputs** blocks. Those are the //! half no test can reach: bytes a real account returned once, a //! dated snapshot re-taken by hand. From 52d588c3753a9b8e6772c306bf43a7b8f1b4f1ec Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Thu, 24 Sep 2026 11:10:38 -0400 Subject: [PATCH 3/3] Address review: skip a bad read_dir entry, fix two stale command counts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit list_profile_names propagated a hard error via ? if a single read_dir entry failed, contradicting this function's own stated design ("nothing to read" rather than a failure) and inconsistent with load_credentials_readonly's .ok() a few lines below — a transient filesystem error on one entry would have made auth profiles fail for every profile instead of just skipping the bad one. Fixed with the same let-else skip config_dir_path already uses above it. docs/commands.md had two references to "four" auth commands left over from before this PR's fifth: the page's own opening count, and the Auth section's "All five commands take" table, which now also says profiles parses --profile without honoring it, matching the note its own section already gives. Deferred per reviewer's own priority call: the config.rs lock-file inconsistency, a --schema contract test for config, deduplicating warn_output_ignored, and the regular-file check on a matching directory entry. 482 tests, fmt and clippy clean. Co-Authored-By: Claude Sonnet 5 --- docs/commands.md | 8 +++++--- src/auth.rs | 10 ++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 6f0b3cd..5cdb5db 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -1,6 +1,6 @@ # Implemented commands -Every command the CLI ships: four auth commands, 33 API operations across 10 +Every command the CLI ships: five auth commands, 33 API operations across 10 command groups, the tilesets-cli proxy, `completion` and `generate-skills`. Each is shown in both of its renderings. Which one you get is decided by `--output`, whose default (`auto`) reads stdout: a terminal gets the left column, a pipe or redirect @@ -117,7 +117,7 @@ Then [Errors](#errors) — the shape a failure takes in each mode. Credentials live in `~/.mapbox`, one file per profile — or in whatever directory `MAPBOX_CONFIG_DIR` names, when it is set. -All four commands take: +All five commands take: | Parameter | Effect | | --- | --- | @@ -125,7 +125,9 @@ All four commands take: | `--output`, `-o` | `auto` \| `text` \| `json`. | `login`, `logout` and `refresh` take `--dry-run` as well. `whoami` does not, -for the reason its own section gives. +for the reason its own section gives. `profiles` takes `--profile` too, in +the sense that it parses — but doesn't honor it, since that command's whole +point is every stored profile at once; see its own section for why. ### `mapbox auth login` diff --git a/src/auth.rs b/src/auth.rs index c3e42a8..7039f35 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -521,8 +521,14 @@ fn list_profile_names() -> Result> { let mut names = vec![]; for entry in entries { - let entry = - entry.with_context(|| format!("Failed to read an entry in {}", dir.display()))?; + // Skipped, not propagated: this function's own promise is "nothing + // to read" rather than a failure, the same reason + // `load_credentials_readonly` below reaches for `.ok()` instead of + // `?`. A transient error reading one entry — permissions changing + // underneath this call, a file removed between `read_dir` and here + // — has nothing to do with whether the *other* entries are valid + // profiles, so it should cost that one entry, not the whole list. + let Ok(entry) = entry else { continue }; if let Some(name) = entry .file_name() .to_str()