diff --git a/CHANGELOG.md b/CHANGELOG.md index bc1b07a..aaf61bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,14 @@ - A POST is no longer replayed after a 5xx or a network failure once sent, which could post a message twice. It fails with exit code 8 or 7 and a hint to check before re-sending. A 429 or a connection that was never made is still retried, and read-only POSTs (`search`, `presence get-batch`) retry as before. - A POST answered 2xx whose response body cannot be read or parsed now says Graph performed it and not to re-send, instead of a bare network or parse error. +- `teams auth status` reports `user` and `tenant_id` at the top level, as `auth list` does, not only inside `token_diagnostics`. A token store that cannot be read is now `KEYRING_ERROR` (exit code 10) instead of `authenticated: false`, and a signed-out profile says why in `reason`. +- A token store that cannot be read (a locked keychain, a denied prompt, an SSH session) is reported as `KEYRING_ERROR` (exit code 10) with its cause, instead of "Not authenticated" (exit code 3). Only a profile with no stored token is "Not authenticated". +- A profile index that cannot be read or parsed is an error (exit code 10) instead of an empty list. `auth login` no longer rewrites it to hold only the new profile, orphaning the rest; `auth list` and `auth logout --all` no longer report no profiles as a success. +- `teams auth switch` with `--config` wrote the selected profile to the default config file, overwriting it with the `--config` file's contents and leaving that file unchanged. It now saves to the `--config` file, and a failed save is a config error (exit code 10) instead of a warning beside a success. +- `teams config path` ignored `--config` and always reported the default path. It now reports the file in use, with `source` set to `flag` or `default`. +- Config saves (`config set`, `config init`, `auth switch`) truncated the file and wrote it in place, so a failed write could leave a partial config. They now write a temporary file and rename it over the original, as `auth login` already did. +- A new profile's name is limited to 1-64 of `A-Z a-z 0-9 . _ -`, not `.` or `..`. `TEAMS_CLI_TOKEN_STORE=file` maps `:`, `/` and `\` to `.` in token file names, so `customer/a` and `customer.a` shared one token and could run as the wrong account. `auth login`, `config set profiles....` and `config set default.profile` refuse another name as invalid input (exit code 2). Profiles already in the config or the token index keep working under their names. A new name whose token file name matches an existing profile's, ignoring case, is refused too. + ## v0.9.0 - 2026-10-04 ### Added diff --git a/docs/command-reference.md b/docs/command-reference.md index f86c631..0832240 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -15,7 +15,7 @@ teams [OPTIONS] | `-v, --verbose` | Increase stderr logging. Repeat for debug/trace. | | `--no-color` | Disable ANSI colors. | | `--config ` | Use an explicit config path. | -| `--profile ` | Use a named credential profile. | +| `--profile ` | Use a named credential profile. New names: 1-64 of `A-Z a-z 0-9 . _ -`. | | `--timeout ` | Override request timeout. | | `--retry ` | Override retry attempts. | | `--page-size ` | Page size for paginated list operations. | diff --git a/docs/man/teams.1 b/docs/man/teams.1 index d100036..9799c26 100644 --- a/docs/man/teams.1 +++ b/docs/man/teams.1 @@ -39,6 +39,13 @@ then the configured default profile, then .B default. An explicit NAME always wins, even when it is the literal .B default. +A new profile's name is 1 to 64 of +.B A-Z a-z 0-9 . _ - +(not +.B . +or +.B ..); +existing profiles keep their names. .TP .B --timeout SECONDS Override Graph request timeout. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 846ce2a..bcb51d6 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -140,6 +140,9 @@ Actions: - Expect one prompt per profile after upgrading or rebuilding; approve it and the grant holds until the binary changes again. +- A denied prompt or a locked keychain (common over SSH) fails with + `KEYRING_ERROR` (exit code 10), not "Not authenticated". Unlock the + keychain (`security unlock-keychain`) or use `TEAMS_CLI_TOKEN_STORE=file`. - An unattended process (a daemon, a scheduled job) cannot answer the prompt and waits on it. Set `TEAMS_CLI_TOKEN_STORE=file` for that process and sign in once with it set; tokens then live in `0600` files under the config diff --git a/src/auth/keyring.rs b/src/auth/keyring.rs index de20026..b47b852 100644 --- a/src/auth/keyring.rs +++ b/src/auth/keyring.rs @@ -79,9 +79,8 @@ fn parse_store_kind(value: Option<&str>) -> Result { } } -/// Refuse a mistyped `TEAMS_CLI_TOKEN_STORE` before any command runs. Token -/// reads report every store error as "not signed in", so without this a typo -/// would send the caller to log in again instead of naming the variable. +/// Refuse a mistyped `TEAMS_CLI_TOKEN_STORE` before any command runs, naming +/// the variable rather than failing on the first token read. pub fn check_token_store() -> Result<()> { store_kind().map(|_| ()) } @@ -133,9 +132,21 @@ fn absent_as_none(result: ::keyring::Result, what: &str) -> Result(result: ::keyring::Result) -> Result> { + absent_as_none(result, "retrieve token").map_err(|e| match e { + TeamsError::KeyringError(message) => TeamsError::KeyringError(format!( + "{message}. If the keychain is locked or this is an SSH session, unlock it \ + or set {TOKEN_STORE_ENV}=file" + )), + other => other, + }) +} + impl SecretStore for OsKeyring { fn get_password(&self, key: &str) -> Result> { - absent_as_none(Self::entry(key)?.get_password(), "retrieve token") + read_keyring(Self::entry(key)?.get_password()) } fn set_password(&self, key: &str, value: &str) -> Result<()> { @@ -148,7 +159,7 @@ impl SecretStore for OsKeyring { } fn get_secret(&self, key: &str) -> Result>> { - absent_as_none(Self::entry(key)?.get_secret(), "retrieve token") + read_keyring(Self::entry(key)?.get_secret()) } fn set_secret(&self, key: &str, value: &[u8]) -> Result<()> { @@ -170,13 +181,20 @@ struct FileStore { dir: PathBuf, } +/// The file name the file token store uses for `key`. It maps `:`, `/` and +/// `\\` to `.`, so distinct keys can share a name; new profile names are +/// checked against it (`config::check_new_profile_name`). +pub(crate) fn file_store_name(key: &str) -> String { + key.replace([':', '/', '\\'], ".") +} + impl FileStore { fn new(dir: PathBuf) -> Self { Self { dir } } fn path(&self, key: &str) -> PathBuf { - self.dir.join(key.replace([':', '/', '\\'], ".")) + self.dir.join(file_store_name(key)) } fn ensure_dir(&self) -> Result<()> { @@ -241,8 +259,8 @@ fn sync_dir(dir: &Path) { } /// The store writes every file `0600`, so a token readable by the group or by -/// others was copied or loosened by hand. Say so on stderr rather than refuse: -/// a refused read would surface as "not signed in", which hides the cause. +/// others was copied or loosened by hand. Say so on stderr rather than refuse, +/// which would lock the owner out of their own token. #[cfg(unix)] fn warn_if_exposed(path: &Path) { use std::os::unix::fs::PermissionsExt; @@ -322,9 +340,11 @@ pub fn store_token(profile: &str, token: &TokenInfo) -> Result<()> { store_token_in(&*store()?, profile, token, chunk_bytes_for(store_kind()?)) } -pub fn get_token(profile: &str) -> Result { +/// The stored token for `profile`, or `Ok(None)` when none is stored. A +/// disabled keyring stores nothing, so it reads as absent. +pub fn get_token(profile: &str) -> Result> { if disabled() { - return Err(TeamsError::KeyringError("Keyring is disabled".into())); + return Ok(None); } get_token_from(&*store()?, profile) } @@ -386,17 +406,16 @@ fn stored_chunk_count(store: &dyn SecretStore, profile: &str) -> Result { .map_or(0, |header| header.chunks)) } -fn get_token_from(store: &dyn SecretStore, profile: &str) -> Result { - let primary = store.get_password(&entry_key(profile))?.ok_or_else(|| { - TeamsError::KeyringError(format!( - "Failed to retrieve token: no stored token for profile `{profile}`" - )) - })?; +fn get_token_from(store: &dyn SecretStore, profile: &str) -> Result> { + let Some(primary) = store.get_password(&entry_key(profile))? else { + return Ok(None); + }; let json = match serde_json::from_str::(&primary) { Ok(header) => read_chunks(store, profile, header.chunks)?, Err(_) => primary, }; serde_json::from_str(&json) + .map(Some) .map_err(|e| TeamsError::KeyringError(format!("Failed to parse stored token: {e}"))) } @@ -449,46 +468,64 @@ fn delete_chunks( } } -pub fn list_profiles() -> Vec { - if disabled() { - return vec![]; - } +const PROFILE_INDEX_KEY: &str = "profile-index"; - // Keyring doesn't support enumeration natively. - // We maintain a separate index entry. - match store().and_then(|s| s.get_password("profile-index")) { - Ok(Some(json)) => serde_json::from_str(&json).unwrap_or_default(), - _ => vec![], +/// The indexed profiles; empty when there is no index. An index that cannot be +/// read or parsed is an error, never an empty list, so no caller rewrites it +/// with the other profiles dropped. +pub fn list_profiles() -> Result> { + if disabled() { + return Ok(vec![]); } + list_profiles_in(&*store()?) } pub fn add_profile_to_index(profile: &str) -> Result<()> { if disabled() { return Ok(()); } - - let mut profiles = list_profiles(); - if !profiles.contains(&profile.to_string()) { - profiles.push(profile.to_string()); - } - write_profile_index(&profiles) + add_profile_to_index_in(&*store()?, profile) } pub fn remove_profile_from_index(profile: &str) -> Result<()> { if disabled() { return Ok(()); } + remove_profile_from_index_in(&*store()?, profile) +} + +// Keyring doesn't support enumeration natively. +// We maintain a separate index entry. +fn list_profiles_in(store: &dyn SecretStore) -> Result> { + let json = store + .get_password(PROFILE_INDEX_KEY) + .map_err(|e| TeamsError::KeyringError(format!("Failed to read profile index: {e}")))?; + match json { + None => Ok(vec![]), + Some(json) => serde_json::from_str(&json) + .map_err(|e| TeamsError::KeyringError(format!("Stored profile index is corrupt: {e}"))), + } +} - let mut profiles = list_profiles(); +fn add_profile_to_index_in(store: &dyn SecretStore, profile: &str) -> Result<()> { + let mut profiles = list_profiles_in(store)?; + if !profiles.contains(&profile.to_string()) { + profiles.push(profile.to_string()); + } + write_profile_index(store, &profiles) +} + +fn remove_profile_from_index_in(store: &dyn SecretStore, profile: &str) -> Result<()> { + let mut profiles = list_profiles_in(store)?; profiles.retain(|p| p != profile); - write_profile_index(&profiles) + write_profile_index(store, &profiles) } -fn write_profile_index(profiles: &[String]) -> Result<()> { +fn write_profile_index(store: &dyn SecretStore, profiles: &[String]) -> Result<()> { let json = serde_json::to_string(profiles) .map_err(|e| TeamsError::KeyringError(format!("Failed to serialize index: {e}")))?; - store()? - .set_password("profile-index", &json) + store + .set_password(PROFILE_INDEX_KEY, &json) .map_err(|e| TeamsError::KeyringError(format!("Failed to store index: {e}"))) } @@ -585,12 +622,14 @@ mod tests { single.is_err(), "Credential Manager accepted an oversized blob; the premise of chunking is gone" ); - let loaded = loaded.expect("chunked store and load"); + let loaded = loaded + .expect("chunked store and load") + .expect("token stored"); assert_eq!(loaded.access_token, original.access_token); assert_eq!(loaded.refresh_token, original.refresh_token); assert_eq!(loaded.scope, original.scope); swept.expect("logout sweep"); - assert!(after.is_err(), "logout left the token readable"); + assert!(matches!(after, Ok(None)), "logout left the token readable"); } fn token(access_token: &str) -> TokenInfo { @@ -635,7 +674,7 @@ mod tests { 0o600 ); } - let loaded = get_token_from(&store, "work").unwrap(); + let loaded = get_token_from(&store, "work").unwrap().unwrap(); assert_eq!(loaded.access_token, original.access_token); assert_eq!(loaded.refresh_token, original.refresh_token); @@ -646,7 +685,7 @@ mod tests { ); delete_token_from(&store, "work", false).unwrap(); - assert!(get_token_from(&store, "work").is_err()); + assert!(get_token_from(&store, "work").unwrap().is_none()); assert!( !store.delete("work:token").unwrap(), "a second delete finds nothing" @@ -693,7 +732,7 @@ mod tests { store_token_in(&store, "work", &original, None).unwrap(); assert_eq!(store.keys(), vec!["work:token"]); - let loaded = get_token_from(&store, "work").unwrap(); + let loaded = get_token_from(&store, "work").unwrap().unwrap(); assert_eq!(loaded.access_token, original.access_token); assert_eq!(loaded.refresh_token, original.refresh_token); } @@ -718,7 +757,7 @@ mod tests { format!("{{\"chunks\":{expected_chunks}}}") ); - let loaded = get_token_from(&store, "work").unwrap(); + let loaded = get_token_from(&store, "work").unwrap().unwrap(); assert_eq!(loaded.access_token, original.access_token); assert_eq!(loaded.refresh_token, original.refresh_token); assert_eq!(loaded.scope, original.scope); @@ -737,7 +776,13 @@ mod tests { "{\"chunks\":1}" ); assert_eq!(store.keys(), vec!["work:token", "work:token:0"]); - assert_eq!(get_token_from(&store, "work").unwrap().access_token, "x"); + assert_eq!( + get_token_from(&store, "work") + .unwrap() + .unwrap() + .access_token, + "x" + ); } #[test] @@ -753,7 +798,10 @@ mod tests { let expected_chunks = serialized_len(&token("short")).div_ceil(64); assert_eq!(after.len(), expected_chunks + 1); assert_eq!( - get_token_from(&store, "work").unwrap().access_token, + get_token_from(&store, "work") + .unwrap() + .unwrap() + .access_token, "short" ); } @@ -767,7 +815,10 @@ mod tests { .unwrap(); assert_eq!( - get_token_from(&store, "work").unwrap().access_token, + get_token_from(&store, "work") + .unwrap() + .unwrap() + .access_token, "legacy" ); } @@ -785,9 +836,96 @@ mod tests { } #[test] - fn missing_primary_entry_is_an_error() { + fn missing_primary_entry_reads_as_absent() { + let store = MemoryStore::default(); + assert!(get_token_from(&store, "work").unwrap().is_none()); + } + + /// Fails every read, as a locked keychain or a denied prompt does, and + /// records every write. + #[derive(Default)] + struct UnreadableStore { + writes: Mutex>, + } + + impl SecretStore for UnreadableStore { + fn get_password(&self, _key: &str) -> Result> { + Err(TeamsError::KeyringError( + "Failed to retrieve token: denied".into(), + )) + } + + fn set_password(&self, key: &str, _value: &str) -> Result<()> { + self.writes.lock().unwrap().push(key.to_string()); + Ok(()) + } + + fn get_secret(&self, key: &str) -> Result>> { + self.get_password(key).map(|v| v.map(String::into_bytes)) + } + + fn set_secret(&self, key: &str, value: &[u8]) -> Result<()> { + self.set_password(key, std::str::from_utf8(value).unwrap()) + } + + fn delete(&self, _key: &str) -> Result { + Ok(false) + } + } + + #[test] + fn unreadable_store_is_an_error_not_an_absent_token() { + let err = get_token_from(&UnreadableStore::default(), "work").unwrap_err(); + assert!(err.to_string().contains("denied"), "{err}"); + } + + #[test] + fn absent_index_lists_no_profiles_and_is_created_on_add() { let store = MemoryStore::default(); - assert!(get_token_from(&store, "work").is_err()); + assert!(list_profiles_in(&store).unwrap().is_empty()); + + add_profile_to_index_in(&store, "work").unwrap(); + add_profile_to_index_in(&store, "work").unwrap(); + add_profile_to_index_in(&store, "home").unwrap(); + assert_eq!(list_profiles_in(&store).unwrap(), ["work", "home"]); + + remove_profile_from_index_in(&store, "work").unwrap(); + assert_eq!(list_profiles_in(&store).unwrap(), ["home"]); + } + + /// The failure that orphaned profiles: a corrupt index read as empty, so + /// the next login rewrote it to hold only the profile just signed in. + #[test] + fn corrupt_index_is_an_error_and_never_rewritten() { + let store = MemoryStore::default(); + store.set_password(PROFILE_INDEX_KEY, "[\"work\",").unwrap(); + + let err = list_profiles_in(&store).unwrap_err(); + assert!( + err.to_string().contains("profile index is corrupt"), + "{err}" + ); + assert_eq!(err.exit_code(), 10); + assert!(add_profile_to_index_in(&store, "home").is_err()); + assert!(remove_profile_from_index_in(&store, "work").is_err()); + + assert_eq!( + store.get_password(PROFILE_INDEX_KEY).unwrap().as_deref(), + Some("[\"work\",") + ); + } + + #[test] + fn unreadable_index_is_an_error_and_never_rewritten() { + let store = UnreadableStore::default(); + + let err = list_profiles_in(&store).unwrap_err(); + assert!(err.to_string().contains("profile index"), "{err}"); + assert!(err.to_string().contains("denied"), "{err}"); + assert!(add_profile_to_index_in(&store, "home").is_err()); + assert!(remove_profile_from_index_in(&store, "work").is_err()); + + assert!(store.writes.lock().unwrap().is_empty()); } #[test] @@ -803,7 +941,10 @@ mod tests { .iter() .all(|key| key.starts_with("other:token"))); assert_eq!( - get_token_from(&store, "other").unwrap().access_token, + get_token_from(&store, "other") + .unwrap() + .unwrap() + .access_token, "keep" ); } @@ -878,7 +1019,7 @@ mod tests { for size in 1..=5 { store_token_in(&store, "work", &original, Some(size)).unwrap(); - let loaded = get_token_from(&store, "work").unwrap(); + let loaded = get_token_from(&store, "work").unwrap().unwrap(); assert_eq!(loaded.scope, original.scope, "chunk size {size}"); assert_eq!(loaded.profile, original.profile, "chunk size {size}"); } diff --git a/src/auth/mod.rs b/src/auth/mod.rs index e46e702..a3355f2 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -19,7 +19,8 @@ const EXPIRY_SKEW_SECS: i64 = 60; /// 1. TEAMS_CLI_ACCESS_TOKEN env var (used verbatim, never refreshed) /// 2. Keyring: if the stored token is (near) expired, silently redeem its /// refresh token; otherwise return it as-is -/// 3. Error +/// 3. Error: not authenticated when no token is stored; the store's own +/// error when it cannot be read pub async fn resolve_token(profile: &str) -> Result { // 1. Direct env var override if let Ok(token) = std::env::var("TEAMS_CLI_ACCESS_TOKEN") { @@ -34,8 +35,8 @@ pub async fn resolve_token(profile: &str) -> Result { } // 2. Keyring - match keyring::get_token(profile) { - Ok(info) => { + match keyring::get_token(profile)? { + Some(info) => { if !needs_refresh(&info) { return Ok(info); } @@ -48,7 +49,7 @@ pub async fn resolve_token(profile: &str) -> Result { Err(e) => handle_refresh_failure(&info, e), } } - Err(_) => Err(not_authenticated()), + None => Err(not_authenticated()), } } @@ -122,7 +123,7 @@ pub async fn refresh_token_with_scopes( profile: &str, scope: Option<&str>, ) -> Result<(TokenInfo, String)> { - let info = keyring::get_token(profile).map_err(|_| not_authenticated())?; + let info = keyring::get_token(profile)?.ok_or_else(not_authenticated)?; if info.refresh_token.is_none() { return Err(TeamsError::AuthError( diff --git a/src/cli/auth.rs b/src/cli/auth.rs index 93999a0..f4b8185 100644 --- a/src/cli/auth.rs +++ b/src/cli/auth.rs @@ -336,6 +336,7 @@ async fn login( format: OutputFormat, ) -> Result<()> { let start = Instant::now(); + config::check_new_profile_name(profile, config)?; let (client_id, tenant_id) = login_registration(&args, config, profile)?; let client_secret = if args.client_credentials { Some( @@ -490,6 +491,8 @@ pub async fn run( let msg = serde_json::json!({ "authenticated": true, "profile": profile, + "user": claims.as_ref().and_then(|c| c.user()), + "tenant_id": claims.as_ref().and_then(|c| c.tid.clone()), "expires_at": token.expires_at.map(|e| e.to_rfc3339()), "scope": token.scope, "token_diagnostics": token_diagnostics(claims.as_ref()), @@ -498,10 +501,14 @@ pub async fn run( output::print_success(format, &msg, start); Ok(()) } - Err(_) => { + // A token store that cannot be read is not "signed out"; report it as + // every other command does (exit 10). + Err(e @ TeamsError::KeyringError(_)) => Err(e), + Err(e) => { let msg = serde_json::json!({ "authenticated": false, "profile": profile, + "reason": e.to_string(), }); output::print_success(format, &msg, start); std::process::exit(1); @@ -511,9 +518,12 @@ pub async fn run( AuthCommand::List => { let start = Instant::now(); - let profiles: Vec = auth::keyring::list_profiles() + let profiles: Vec = auth::keyring::list_profiles()? .iter() - .map(|name| summarize_profile(name, auth::keyring::get_token(name).ok().as_ref())) + .map(|name| { + let token = auth::keyring::get_token(name).ok().flatten(); + summarize_profile(name, token.as_ref()) + }) .collect(); let msg = serde_json::json!({ "profiles": profiles, @@ -538,9 +548,7 @@ pub async fn run( // Update config to set default profile let mut updated_config = config.clone(); updated_config.default.profile = Some(name.clone()); - if let Err(e) = config::save_config(&updated_config, None) { - tracing::warn!("Could not save profile switch to config: {e}"); - } + config::save_config(&updated_config, config_path)?; let msg = serde_json::json!({ "message": format!("Switched to profile '{name}'"), @@ -556,7 +564,7 @@ pub async fn run( } => { let start = Instant::now(); if all { - let profiles = auth::keyring::list_profiles(); + let profiles = auth::keyring::list_profiles()?; for p in &profiles { auth::keyring::delete_token(p)?; auth::keyring::remove_profile_from_index(p)?; diff --git a/src/cli/config_cmd.rs b/src/cli/config_cmd.rs index 498230e..5d4ea2b 100644 --- a/src/cli/config_cmd.rs +++ b/src/cli/config_cmd.rs @@ -87,6 +87,11 @@ pub async fn run( .map_err(|e| TeamsError::ConfigError(format!("Serialization error: {e}")))?; let parts: Vec<&str> = key.split('.').collect(); + match parts.as_slice() { + ["profiles", name, ..] => config::check_new_profile_name(name, config)?, + ["default", "profile"] => config::check_new_profile_name(&value, config)?, + _ => {} + } let mut current = &mut config_json; for (i, part) in parts.iter().enumerate() { if i == parts.len() - 1 { @@ -112,9 +117,13 @@ pub async fn run( ConfigCommand::Path => { let start = Instant::now(); - let path = config::default_config_path()?; + let (path, source) = match config_path { + Some(path) => (PathBuf::from(path), "flag"), + None => (config::default_config_path()?, "default"), + }; let msg = serde_json::json!({ "path": path.display().to_string(), + "source": source, "exists": path.exists(), "namespace": config::NAMESPACE, }); diff --git a/src/config.rs b/src/config.rs index 5b4f68b..3aa023a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -216,30 +216,9 @@ pub fn save_config(config: &ConfigFile, path: Option<&str>) -> Result<()> { Some(p) => PathBuf::from(p), None => default_config_path()?, }; - - if let Some(parent) = config_path.parent() { - fs::create_dir_all(parent) - .map_err(|e| TeamsError::ConfigError(format!("Failed to create config dir: {e}")))?; - } - let content = toml::to_string_pretty(config) .map_err(|e| TeamsError::ConfigError(format!("Failed to serialize config: {e}")))?; - - fs::write(&config_path, &content).map_err(|e| { - TeamsError::ConfigError(format!( - "Failed to write config to {}: {e}", - config_path.display() - )) - })?; - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o600); - fs::set_permissions(&config_path, perms).ok(); - } - - Ok(()) + write_config_text(&config_path, &content) } /// Resolve the active profile: an explicit `--profile` flag or @@ -253,6 +232,61 @@ pub fn resolve_profile<'a>(cli_profile: Option<&'a str>, config: &'a ConfigFile) } } +/// Longest name a new profile may have. +pub const MAX_PROFILE_NAME_LEN: usize = 64; + +/// Whether `name` may name a new profile: `A-Z a-z 0-9 . _ -`, at most +/// `MAX_PROFILE_NAME_LEN` long, not `.` or `..`. The file token store maps +/// `:`, `/` and `\` to `.`, so wider names could share a token file. +pub fn is_valid_profile_name(name: &str) -> bool { + !name.is_empty() + && name.len() <= MAX_PROFILE_NAME_LEN + && name != "." + && name != ".." + && name + .bytes() + .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'_' | b'-')) +} + +/// Refuse to create a profile under an invalid name, or under one whose token +/// file name matches an existing profile's, ignoring case as macOS and Windows +/// file systems do by default. A profile that already exists, in +/// the config or the token index, keeps its name. +pub fn check_new_profile_name(name: &str, config: &ConfigFile) -> Result<()> { + check_new_profile_name_with(name, config, crate::auth::keyring::list_profiles) +} + +fn check_new_profile_name_with( + name: &str, + config: &ConfigFile, + index: impl FnOnce() -> Result>, +) -> Result<()> { + if config.profiles.contains_key(name) { + return Ok(()); + } + let indexed = index()?; + if indexed.iter().any(|p| p == name) { + return Ok(()); + } + if !is_valid_profile_name(name) { + return Err(TeamsError::InvalidInput(format!( + "Invalid profile name '{name}': use A-Z a-z 0-9 . _ - (max {MAX_PROFILE_NAME_LEN}, not . or ..)" + ))); + } + let file_name = crate::auth::keyring::file_store_name(name); + if let Some(other) = config + .profiles + .keys() + .chain(&indexed) + .find(|p| crate::auth::keyring::file_store_name(p).eq_ignore_ascii_case(&file_name)) + { + return Err(TeamsError::InvalidInput(format!( + "Profile name '{name}' would share a token file with profile '{other}'; choose another" + ))); + } + Ok(()) +} + pub fn resolve_client_id( cli_flag: Option<&str>, profile: &str, @@ -636,6 +670,77 @@ scopes = "User.Read People.Read offline_access" assert_eq!(resolve_profile(None, &config), "default"); } + #[test] + fn profile_names_are_limited_to_a_file_safe_set() { + for name in ["default", "work", "customer.a", "a_b-C9", &"x".repeat(64)] { + assert!(is_valid_profile_name(name), "{name}"); + } + for name in [ + "", + ".", + "..", + "customer/a", + "a\\b", + "a:b", + "a b", + "ñ", + &"x".repeat(65), + ] { + assert!(!is_valid_profile_name(name), "{name}"); + } + } + + /// The name the CLI falls back to must itself be a valid new name. + #[test] + fn the_default_profile_name_is_valid() { + let config = ConfigFile::default(); + assert!(is_valid_profile_name(resolve_profile(None, &config))); + } + + #[test] + fn check_new_profile_name_keeps_existing_profiles() { + let mut config = ConfigFile::default(); + let err = + check_new_profile_name_with("customer/a", &config, || Ok(Vec::new())).unwrap_err(); + assert_eq!(err.exit_code(), 2); + check_new_profile_name_with("customer/a", &config, || Ok(vec!["customer/a".into()])) + .unwrap(); + config + .profiles + .insert("customer/a".into(), ProfileConfig::default()); + check_new_profile_name_with("customer/a", &config, || Ok(Vec::new())).unwrap(); + } + + #[test] + fn check_new_profile_name_reports_an_unreadable_index() { + let config = ConfigFile::default(); + let unreadable = || Err(TeamsError::KeyringError("denied".into())); + for name in ["other/b", "valid"] { + let err = check_new_profile_name_with(name, &config, unreadable).unwrap_err(); + assert_eq!(err.exit_code(), 10); + } + } + + #[test] + fn check_new_profile_name_refuses_a_shared_token_file() { + let mut config = ConfigFile::default(); + let legacy = || Ok(vec!["customer/a".to_string()]); + let err = check_new_profile_name_with("customer.a", &config, legacy).unwrap_err(); + assert_eq!(err.exit_code(), 2); + assert!(err.to_string().contains("customer/a"), "{err}"); + check_new_profile_name_with("customer.b", &config, legacy).unwrap(); + let existing = || Ok(vec!["Work".to_string()]); + let err = check_new_profile_name_with("work", &config, existing).unwrap_err(); + assert!(err.to_string().contains("'Work'"), "{err}"); + + config + .profiles + .insert("customer:a".into(), ProfileConfig::default()); + let err = + check_new_profile_name_with("customer.a", &config, || Ok(Vec::new())).unwrap_err(); + assert!(err.to_string().contains("customer:a"), "{err}"); + } + #[test] fn resolve_profile_explicit_default_beats_config_default() { // An explicit --profile default must address the profile named @@ -1082,6 +1187,29 @@ client_id = \"other-app\" assert_eq!(fs::read_to_string(&target).unwrap(), "a = 2\n"); } + /// `save_config` replaces the file rather than truncating it, so a write + /// that fails, here for want of a writable directory, keeps the old file. + #[cfg(unix)] + #[test] + fn save_config_failure_keeps_the_old_file() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("config.toml"); + fs::write(&path, "[default]\nprofile = \"old\"\n").unwrap(); + fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o500)).unwrap(); + + let mut config = ConfigFile::default(); + config.default.profile = Some("new".into()); + let saved = save_config(&config, path.to_str()); + fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o700)).unwrap(); + + assert!(matches!(saved, Err(TeamsError::ConfigError(_)))); + assert_eq!( + fs::read_to_string(&path).unwrap(), + "[default]\nprofile = \"old\"\n" + ); + } + #[test] fn id_source_follows_the_resolution_order() { assert_eq!(IdSource::of(true, true, true), IdSource::Flag); diff --git a/tests/cli.rs b/tests/cli.rs index a204250..780c2ee 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1328,9 +1328,7 @@ fn message_update_requires_channel_alongside_team() { } /// A mistyped `TEAMS_CLI_TOKEN_STORE` is refused before any command runs, in -/// the error envelope with exit code 2. Token reads treat every store error as -/// "not signed in", so without the check a typo would read as exit code 3 and -/// send the caller to log in again. +/// the error envelope with exit code 2, naming the variable. #[test] fn unknown_token_store_is_invalid_input_not_an_auth_error() { for args in [["chat", "list"], ["auth", "list"]] { @@ -1399,6 +1397,141 @@ fn file_token_store_lists_and_logs_out_a_profile() { ); } +/// A new profile name is limited to `A-Z a-z 0-9 . _ -`: the file token +/// store maps `/` to `.`, so `customer/a` would share `customer.a`'s token. +/// Each way a name can be created refuses it as invalid input and writes +/// nothing. +#[test] +fn an_invalid_new_profile_name_is_refused() { + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let config_path = dir.path().join("config.toml"); + let config = config_path.to_str().unwrap(); + let refused = |cmd: &mut Command| { + cmd.args(["--config", config, "--output", "json"]) + .assert() + .code(2) + .stdout( + predicate::str::contains("INVALID_INPUT") + .and(predicate::str::contains("Invalid profile name")), + ); + }; + let login = ["auth", "login", "--client-credentials"]; + let long = "x".repeat(65); + for name in ["customer/a", "a:b", "..", "", long.as_str()] { + refused(teams().args(["--profile", name]).args(login)); + } + refused(teams().env("TEAMS_CLI_PROFILE", "customer/a").args(login)); + refused(teams().args(["config", "set", "profiles.customer/a.tenant_id", "t"])); + refused(teams().args(["config", "set", "default.profile", "customer/a"])); + assert!(!config_path.exists()); +} + +/// A valid name gets past the name check; this login then stops for want of +/// a client secret, before any network call. +#[test] +fn a_valid_new_profile_name_is_accepted() { + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let config_path = dir.path().join("config.toml"); + let config = config_path.to_str().unwrap(); + for name in ["customer.a", "work_2-B"] { + teams() + .args(["--config", config, "--output", "json", "--profile", name]) + .args(["auth", "login", "--client-credentials"]) + .args(["--client-id", "app-1", "--tenant-id", "tenant-1"]) + .assert() + .code(2) + .stdout(predicate::str::contains("Client secret is required")); + } + teams() + .args(["--config", config, "--output", "json"]) + .args(["config", "set", "profiles.customer-a.tenant_id", "t"]) + .assert() + .success(); +} + +/// A profile created before names were limited keeps working: it loads from +/// the config, can be edited and signed in to again. +#[test] +fn a_legacy_profile_name_in_the_config_keeps_working() { + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let config_path = dir.path().join("config.toml"); + let config = config_path.to_str().unwrap(); + fs::write( + &config_path, + r#"[default] +profile = "customer/a" + +[profiles."customer/a"] +client_id = "app-1" +tenant_id = "tenant-1" +"#, + ) + .unwrap(); + teams() + .args(["--config", config, "--output", "json", "config", "profiles"]) + .assert() + .success() + .stdout(predicate::str::contains("customer/a")); + teams() + .args(["--config", config, "--output", "json"]) + .args(["config", "set", "profiles.customer/a.tenant_id", "tenant-2"]) + .assert() + .success(); + // Resolved from `default.profile`, and again given explicitly. + for profile in [&[][..], &["--profile", "customer/a"][..]] { + teams() + .args(["--config", config, "--output", "json"]) + .args(profile) + .args(["auth", "login", "--client-credentials"]) + .assert() + .code(2) + .stdout(predicate::str::contains("Client secret is required")); + } +} + +/// A legacy name known only from the file store's profile index also keeps +/// working. Not on Windows, whose config directory ignores `HOME`. +#[cfg(not(windows))] +#[test] +fn a_legacy_profile_name_in_the_token_index_keeps_working() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let file_store = || { + let mut cmd = teams(); + cmd.env_remove("TEAMS_CLI_DISABLE_KEYRING") + .env("TEAMS_CLI_TOKEN_STORE", "file") + .env("HOME", home.path()) + .env("XDG_CONFIG_HOME", home.path()); + cmd + }; + let output = file_store() + .args(["config", "path", "--output", "json"]) + .output() + .unwrap(); + let output: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + let config_path = std::path::PathBuf::from(output["data"]["path"].as_str().unwrap()); + assert!(config_path.starts_with(home.path())); + let tokens = config_path.parent().unwrap().join("tokens"); + fs::create_dir_all(&tokens).unwrap(); + fs::write(tokens.join("profile-index"), r#"["customer/a"]"#).unwrap(); + + for (name, expected) in [ + ("customer/a", "Client secret is required"), + ("customer/b", "Invalid profile name"), + ( + "customer.a", + "would share a token file with profile 'customer/a'", + ), + ] { + file_store() + .args(["--profile", name, "--output", "json"]) + .args(["auth", "login", "--client-credentials"]) + .args(["--client-id", "app-1", "--tenant-id", "tenant-1"]) + .assert() + .code(2) + .stdout(predicate::str::contains(expected)); + } +} + /// `--help` names the environment variable a client secret can come from, but /// never prints the secret itself, which would put it in a terminal /// scrollback, a pasted bug report or an agent transcript. @@ -1493,3 +1626,251 @@ fn a_failed_login_does_not_save_the_given_ids() { .stdout(predicate::str::contains("Client secret is required")); assert!(!config_path.exists()); } + +/// A `teams` command factory using the file token store under `home`, and +/// that store's `tokens/` directory, created. Not on Windows, whose config +/// directory ignores `HOME`. +#[cfg(not(windows))] +fn file_store_in(home: &std::path::Path) -> (impl Fn() -> Command + '_, std::path::PathBuf) { + let output = teams() + .env("HOME", home) + .env("XDG_CONFIG_HOME", home) + .args(["config", "path", "--output", "json"]) + .output() + .unwrap(); + let data: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + let config_path = std::path::PathBuf::from(data["data"]["path"].as_str().unwrap()); + assert!(config_path.starts_with(home)); + let tokens = config_path.parent().unwrap().join("tokens"); + fs::create_dir_all(&tokens).unwrap(); + let cmd = move || { + let mut cmd = teams(); + cmd.env_remove("TEAMS_CLI_DISABLE_KEYRING") + .env("TEAMS_CLI_TOKEN_STORE", "file") + .env("HOME", home) + .env("XDG_CONFIG_HOME", home); + cmd + }; + (cmd, tokens) +} + +/// A corrupt profile index is an error (exit code 10), never an empty list: +/// `auth list` must not report no profiles, `logout --all` must not report +/// "0 profile(s)" as a success, and none of them may rewrite the index. +#[cfg(not(windows))] +#[test] +fn corrupt_profile_index_is_a_storage_error_and_left_untouched() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let (file_store, tokens) = file_store_in(home.path()); + let index = tokens.join("profile-index"); + fs::write(&index, "not json").unwrap(); + + for args in [ + &["auth", "list"][..], + &["auth", "logout", "--all"], + &["auth", "logout", "--profile", "work"], + ] { + file_store() + .args(args) + .args(["--output", "json"]) + .assert() + .code(10) + .stdout( + predicate::str::contains("KEYRING_ERROR") + .and(predicate::str::contains("profile index")), + ); + assert_eq!(fs::read_to_string(&index).unwrap(), "not json", "{args:?}"); + } +} + +/// An index that exists but cannot be read is an error too, not "no profiles". +#[cfg(not(windows))] +#[test] +fn unreadable_profile_index_is_a_storage_error() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let (file_store, tokens) = file_store_in(home.path()); + // a directory where the index file belongs: reading it fails, but not as + // "not found" + fs::create_dir(tokens.join("profile-index")).unwrap(); + + for args in [&["auth", "list"][..], &["auth", "logout", "--all"]] { + file_store() + .args(args) + .args(["--output", "json"]) + .assert() + .code(10) + .stdout(predicate::str::contains("KEYRING_ERROR")); + } +} + +/// Only a token that is not stored at all means "not authenticated" (exit +/// code 3). A token the store holds but cannot read is a storage error (exit +/// code 10) naming the cause, so the caller is not sent to log in again. +#[cfg(not(windows))] +#[test] +fn absent_token_is_auth_error_but_unreadable_token_is_storage_error() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let (file_store, tokens) = file_store_in(home.path()); + let token = tokens.join("work.token"); + + file_store() + .args(["--profile", "work", "user", "me", "--output", "json"]) + .assert() + .code(3) + .stdout(predicate::str::contains("Not authenticated.")); + + fs::create_dir(&token).unwrap(); + file_store() + .args(["--profile", "work", "user", "me", "--output", "json"]) + .assert() + .code(10) + .stdout( + predicate::str::contains("KEYRING_ERROR") + .and(predicate::str::contains("Failed to retrieve token")) + .and(predicate::str::contains("Not authenticated").not()), + ); + + fs::remove_dir(&token).unwrap(); + fs::write(&token, "not json").unwrap(); + file_store() + .args(["--profile", "work", "auth", "refresh", "--output", "json"]) + .assert() + .code(10) + .stdout(predicate::str::contains("Failed to parse stored token")); +} + +/// The default config path under `home`, as `config path` reports it. +#[cfg(not(windows))] +fn default_config_path_in(home: &std::path::Path) -> std::path::PathBuf { + let output = teams() + .env("HOME", home) + .env("XDG_CONFIG_HOME", home) + .args(["config", "path", "--output", "json"]) + .output() + .unwrap(); + let data: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(data["data"]["source"], "default"); + std::path::PathBuf::from(data["data"]["path"].as_str().unwrap()) +} + +/// `config path` reports the `--config` file when one is given. +#[test] +fn config_path_reports_the_config_flag() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("customer.toml"); + let result = teams() + .args(["--config", path.to_str().unwrap()]) + .args(["config", "path", "--output", "json"]) + .assert() + .success(); + let data: serde_json::Value = serde_json::from_slice(&result.get_output().stdout).unwrap(); + assert_eq!(data["data"]["path"], path.to_str().unwrap()); + assert_eq!(data["data"]["source"], "flag"); + assert_eq!(data["data"]["exists"], false); +} + +/// `auth switch --config F` records the profile in F and leaves the default +/// config alone. Not on Windows, whose config directory ignores `HOME`. +#[cfg(not(windows))] +#[test] +fn auth_switch_saves_to_the_config_flag() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let default_path = default_config_path_in(home.path()); + assert!(default_path.starts_with(home.path())); + let dir = tempfile::tempdir().unwrap(); + let path = write_customer_profile_config(&dir); + + teams() + .env("HOME", home.path()) + .env("XDG_CONFIG_HOME", home.path()) + .env("TEAMS_CLI_ACCESS_TOKEN", "not-a-jwt") + .args(["--config", path.to_str().unwrap()]) + .args(["auth", "switch", "customer", "--output", "json"]) + .assert() + .success(); + + let saved = fs::read_to_string(&path).unwrap(); + assert!(saved.contains("profile = \"customer\""), "{saved}"); + assert!(saved.contains("11111111-1111-1111-1111-111111111111")); + assert!(!default_path.exists()); +} + +/// A switch that cannot be saved fails with a config error instead of +/// reporting success. +#[test] +fn auth_switch_fails_when_the_config_cannot_be_saved() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let dir = tempfile::tempdir().unwrap(); + let not_a_dir = dir.path().join("file"); + fs::write(¬_a_dir, "").unwrap(); + let path = not_a_dir.join("config.toml"); + + teams() + .env("HOME", home.path()) + .env("XDG_CONFIG_HOME", home.path()) + .env("TEAMS_CLI_ACCESS_TOKEN", "not-a-jwt") + .args(["--config", path.to_str().unwrap()]) + .args(["auth", "switch", "customer", "--output", "json"]) + .assert() + .code(10) + .stdout(predicate::str::contains("\"success\": false")); +} + +/// `auth status` names the signed-in user and tenant at the top level, as +/// `auth list` does, not only inside `token_diagnostics`. +#[test] +fn auth_status_reports_user_and_tenant() { + use base64::Engine as _; + let b64 = |v: serde_json::Value| { + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(v.to_string()) + }; + let jwt = format!( + "{}.{}.sig", + b64(serde_json::json!({ "alg": "none" })), + b64(serde_json::json!({ + "preferred_username": "a@contoso.com", + "tid": "tenant-1", + "aud": "https://graph.microsoft.com", + "exp": 4_102_444_800u64 + })) + ); + let output = teams() + .env("TEAMS_CLI_ACCESS_TOKEN", jwt) + .args(["auth", "status", "--output", "json"]) + .output() + .unwrap(); + let data: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(data["data"]["user"], "a@contoso.com", "{data}"); + assert_eq!(data["data"]["tenant_id"], "tenant-1"); +} + +/// `auth status` reports an unreadable token store as a storage error (exit +/// 10), and only a profile with no token as signed out (exit 1). +#[cfg(not(windows))] +#[test] +fn auth_status_reports_an_unreadable_store_as_a_storage_error() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let (file_store, tokens) = file_store_in(home.path()); + + let output = file_store() + .args(["auth", "status", "--output", "json"]) + .output() + .unwrap(); + assert_eq!(output.status.code(), Some(1)); + let data: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(data["data"]["authenticated"], false); + assert!( + data["data"]["reason"] + .as_str() + .unwrap() + .contains("Not authenticated"), + "{data}" + ); + + fs::create_dir_all(tokens.join("default.token")).unwrap(); + file_store() + .args(["auth", "status", "--output", "json"]) + .assert() + .code(10) + .stdout(predicate::str::contains("KEYRING_ERROR")); +}