From 38471080bfbda504c37807806706da7fea47e9ed Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:46:19 +0100 Subject: [PATCH 01/11] Report token store read failures instead of hiding them list_profiles() turned any index read error or corrupt JSON into an empty list. After a denied macOS keychain prompt, the next login rewrote the index to hold only the new profile, orphaning the rest, and `auth logout --all` reported "Logged out from 0 profile(s)" as a success. resolve_token likewise mapped every get_token error to "Not authenticated" (exit 3), sending a user with a locked keychain to log in again. get_token now returns Ok(None) for an absent token (and when the keyring is disabled); only that is "Not authenticated". Any other store error propagates as KEYRING_ERROR (exit 10) with its cause, and keychain read errors add a hint to unlock it or set TEAMS_CLI_TOKEN_STORE=file. A Windows token with a missing chunk is now exit 10 too. list_profiles returns Result: an absent index is empty, an unreadable or corrupt one is an error, and add/remove never write after a failed read. `auth list` and `logout --all` surface it. No on-disk or keychain format change. Tested with unit tests on in-memory and always-failing stores (corrupt or unreadable index is never rewritten) and file-store CLI tests: corrupt or unreadable index makes `auth list` / `logout` exit 10 with the index unchanged; absent token exits 3; unreadable or unparseable token exits 10 with the cause. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 5 + docs/troubleshooting.md | 3 + src/auth/keyring.rs | 230 +++++++++++++++++++++++++++++++--------- src/auth/mod.rs | 11 +- src/cli/auth.rs | 9 +- tests/cli.rs | 116 +++++++++++++++++++- 6 files changed, 315 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c2b46..c66617d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Fixed + +- 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. + ## v0.9.0 - 2026-10-04 ### Added 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..c9ef739 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<()> { @@ -241,8 +252,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 +333,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 +399,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 +461,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}"))), + } +} + +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) +} - let mut profiles = list_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 +615,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 +667,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 +678,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 +725,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 +750,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 +769,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 +791,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 +808,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 +829,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!(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(); - assert!(get_token_from(&store, "work").is_err()); + 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 +934,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 +1012,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..358f57d 100644 --- a/src/cli/auth.rs +++ b/src/cli/auth.rs @@ -511,9 +511,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, @@ -556,7 +559,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/tests/cli.rs b/tests/cli.rs index a204250..a463e79 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"]] { @@ -1493,3 +1491,115 @@ 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")); +} From ba2454eb2dd8a8a1de1c1b39e073ecfcbc4444a9 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:46:47 +0100 Subject: [PATCH 02/11] Honour --config in auth switch and config path; save atomically `auth switch` saved with `save_config(.., None)`, so with `--config customer.toml` it wrote customer.toml's contents plus the new default profile over the default config file and left customer.toml unchanged. A failed save was only a tracing warning beside a success. `config path` always reported the default path, ignoring `--config`. `save_config` truncated the file and wrote it in place, so a failed write (full disk, crash) could leave a partial config. `auth switch` now saves to the `--config` path, and a failed save is a config error (exit 10). `config path` reports the `--config` file when given and adds `source`: `flag` or `default`. `save_config` goes through `write_config_text`, the temp-file-and-rename writer login already uses (0600, writes through symlinks), so `config set`, `config init` and `auth switch` keep the old file on failure. No cross-process locking. Tests: switch with `--config` updates that file and creates no default config under a temp HOME; a switch whose save fails exits 10; `config path` with and without `--config`; `save_config` into a read-only directory fails and leaves the original file intact. All four fail without the fix. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 3 ++ src/cli/auth.rs | 4 +-- src/cli/config_cmd.rs | 6 +++- src/config.rs | 46 +++++++++++++------------- tests/cli.rs | 77 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c66617d..7ce9850 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ - 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. ## v0.9.0 - 2026-10-04 diff --git a/src/cli/auth.rs b/src/cli/auth.rs index 358f57d..11d97a8 100644 --- a/src/cli/auth.rs +++ b/src/cli/auth.rs @@ -541,9 +541,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}'"), diff --git a/src/cli/config_cmd.rs b/src/cli/config_cmd.rs index 498230e..c6f434e 100644 --- a/src/cli/config_cmd.rs +++ b/src/cli/config_cmd.rs @@ -112,9 +112,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..89cebf0 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 @@ -1082,6 +1061,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 a463e79..8e97587 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1603,3 +1603,80 @@ fn absent_token_is_auth_error_but_unreadable_token_is_storage_error() { .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")); +} From 6626b11481b75888bf32838fc5745a41e2f40d65 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:45:31 +0100 Subject: [PATCH 03/11] Limit new profile names to a file-safe character set With TEAMS_CLI_TOKEN_STORE=file, token file names map ':', '/' and '\' to '.', so profiles `customer/a` and `customer.a` shared one token file and a command could run as the wrong account. A new profile's name must now be 1-64 of A-Z a-z 0-9 . _ - and not `.` or `..`. `auth login` (via --profile, TEAMS_CLI_PROFILE or default.profile), `config set profiles....` and `config set default.profile` refuse other names with exit code 2. A name already in the config or the token index is accepted, so existing profiles keep working. The only name the CLI derives itself, `default`, conforms. The file store's name encoding is unchanged. Tests: unit tests for the name check, the `default` fallback and the existing-profile exemption; CLI tests for each refusal path, valid names, and legacy names in the config and in the file store index. An invalid name is checked against the token index; if that index cannot be read, the storage error (exit 10) is reported rather than read as no profiles. Valid names never read the index. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 1 + docs/command-reference.md | 2 +- docs/man/teams.1 | 7 ++ src/cli/auth.rs | 1 + src/cli/config_cmd.rs | 5 ++ src/config.rs | 88 +++++++++++++++++++++++++ tests/cli.rs | 131 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 234 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ce9850..a234c73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - `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. ## v0.9.0 - 2026-10-04 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/src/cli/auth.rs b/src/cli/auth.rs index 11d97a8..f6716b0 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( diff --git a/src/cli/config_cmd.rs b/src/cli/config_cmd.rs index c6f434e..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 { diff --git a/src/config.rs b/src/config.rs index 89cebf0..c2b85b2 100644 --- a/src/config.rs +++ b/src/config.rs @@ -232,6 +232,44 @@ 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. 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 is_valid_profile_name(name) + || config.profiles.contains_key(name) + || index()?.iter().any(|p| p == name) + { + return Ok(()); + } + Err(TeamsError::InvalidInput(format!( + "Invalid profile name '{name}': use A-Z a-z 0-9 . _ - (max {MAX_PROFILE_NAME_LEN}, not . or ..)" + ))) +} + pub fn resolve_client_id( cli_flag: Option<&str>, profile: &str, @@ -615,6 +653,56 @@ 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())); + let err = check_new_profile_name_with("other/b", &config, unreadable).unwrap_err(); + assert_eq!(err.exit_code(), 10); + check_new_profile_name_with("valid", &config, || panic!("index read")).unwrap(); + } + #[test] fn resolve_profile_explicit_default_beats_config_default() { // An explicit --profile default must address the profile named diff --git a/tests/cli.rs b/tests/cli.rs index 8e97587..f19e780 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1397,6 +1397,137 @@ 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"), + ] { + 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. From ca0b0eb3b7e54a6148a3b7228549405eeb049d5e Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 17:18:13 +0100 Subject: [PATCH 04/11] Refuse a new profile name that shares an existing token file The file token store maps ':', '/' and '\' to '.', and existing profiles keep names outside the new set. So a legacy 'customer/a' and a new, valid 'customer.a' still mapped to one token file: logging in to the new one overwrote the old one's credentials. A new name is now also refused (exit 2) when its token file name matches any existing profile's, in the config or the token index. The mapping is shared with the file store, so the two cannot drift. New valid names now read the index; an unreadable index is still reported (exit 10), not taken as no profiles. Tests: unit cases for an indexed and a configured legacy name, and the CLI login case with 'customer/a' in the file store index; the CLI case fails against the previous check. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 2 +- src/auth/keyring.rs | 9 ++++++- src/config.rs | 58 +++++++++++++++++++++++++++++++++++---------- tests/cli.rs | 4 ++++ 4 files changed, 59 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a234c73..38b4127 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ - `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 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 is refused too. ## v0.9.0 - 2026-10-04 diff --git a/src/auth/keyring.rs b/src/auth/keyring.rs index c9ef739..b47b852 100644 --- a/src/auth/keyring.rs +++ b/src/auth/keyring.rs @@ -181,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<()> { diff --git a/src/config.rs b/src/config.rs index c2b85b2..494effd 100644 --- a/src/config.rs +++ b/src/config.rs @@ -248,8 +248,9 @@ pub fn is_valid_profile_name(name: &str) -> bool { .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'_' | b'-')) } -/// Refuse to create a profile under an invalid name. A profile that already -/// exists, in the config or the token index, keeps its name. +/// Refuse to create a profile under an invalid name, or under one whose token +/// file name matches an existing profile's. 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) } @@ -259,15 +260,30 @@ fn check_new_profile_name_with( config: &ConfigFile, index: impl FnOnce() -> Result>, ) -> Result<()> { - if is_valid_profile_name(name) - || config.profiles.contains_key(name) - || index()?.iter().any(|p| p == name) - { + if config.profiles.contains_key(name) { + return Ok(()); + } + let indexed = index()?; + if indexed.iter().any(|p| p == name) { return Ok(()); } - Err(TeamsError::InvalidInput(format!( - "Invalid profile name '{name}': use A-Z a-z 0-9 . _ - (max {MAX_PROFILE_NAME_LEN}, not . or ..)" - ))) + 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) == 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( @@ -698,9 +714,27 @@ scopes = "User.Read People.Read offline_access" fn check_new_profile_name_reports_an_unreadable_index() { let config = ConfigFile::default(); let unreadable = || Err(TeamsError::KeyringError("denied".into())); - let err = check_new_profile_name_with("other/b", &config, unreadable).unwrap_err(); - assert_eq!(err.exit_code(), 10); - check_new_profile_name_with("valid", &config, || panic!("index read")).unwrap(); + 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(); + + 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] diff --git a/tests/cli.rs b/tests/cli.rs index f19e780..eb2b600 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1517,6 +1517,10 @@ fn a_legacy_profile_name_in_the_token_index_keeps_working() { 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"]) From 0c15e50979a93be507e07db2c698f812ceb21ea6 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 17:21:05 +0100 Subject: [PATCH 05/11] Compare profile token file names without regard to case macOS and Windows file systems ignore case by default, so 'Work' and 'work' address one token file. The collision check compared names exactly, letting a new 'work' replace an existing 'Work' profile's credentials. Compare them ignoring ASCII case. Test: an existing 'Work' refuses a new 'work'; fails with an exact comparison. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 2 +- src/config.rs | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38b4127..2b91740 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ - `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 is refused too. +- 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 diff --git a/src/config.rs b/src/config.rs index 494effd..3aa023a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -249,7 +249,8 @@ pub fn is_valid_profile_name(name: &str) -> bool { } /// Refuse to create a profile under an invalid name, or under one whose token -/// file name matches an existing profile's. A profile that already exists, in +/// 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) @@ -277,7 +278,7 @@ fn check_new_profile_name_with( .profiles .keys() .chain(&indexed) - .find(|p| crate::auth::keyring::file_store_name(p) == file_name) + .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" @@ -728,6 +729,9 @@ scopes = "User.Read People.Read offline_access" 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 From f0a83750e46cbcdbb95cebc2eababad75f63c980 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:16:30 +0100 Subject: [PATCH 06/11] Report user, tenant and store errors in auth status auth status put the signed-in user only inside token_diagnostics, so .data.user read null although auth list shows it; and any failure to load a token, a locked or denied keychain included, came out as authenticated: false with exit 1, the "signed out" answer. Report user and tenant_id at the top level as well. A token store that cannot be read is now KEYRING_ERROR (exit 10), as for every other command; any other failure stays authenticated: false (exit 1) and says why in reason. Tests: top-level user and tenant from a token's claims; a missing token is exit 1 with a reason, an unreadable store is exit 10. Both fail against the previous handler. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 1 + src/cli/auth.rs | 8 ++++++- tests/cli.rs | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b91740..60ce7ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- `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. diff --git a/src/cli/auth.rs b/src/cli/auth.rs index f6716b0..f4b8185 100644 --- a/src/cli/auth.rs +++ b/src/cli/auth.rs @@ -491,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()), @@ -499,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); diff --git a/tests/cli.rs b/tests/cli.rs index eb2b600..780c2ee 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1815,3 +1815,62 @@ fn auth_switch_fails_when_the_config_cannot_be_saved() { .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")); +} From 0c0ed40f3ccfdcda6a44273c43598a8c6f6c6ec2 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:50:01 +0100 Subject: [PATCH 07/11] Report pagination continuation in list metadata List commands without --all-pages fetched one page and printed it with success true, discarding Graph's @odata.nextLink. A caller could not tell a complete collection from a truncated one, nor resume: `teams chat list --page-size 1` looked the same whether one chat existed or fifty. get_paged and get_paged_without_top now return a Page { items, next_link }; list API wrappers pass it through and CLI list commands print it with output::print_success_page. JSON metadata gains has_more and, when true, next_link (the existing, previously unused field). --all-pages gives has_more false. data is unchanged. Human and plain output print "More results available; use --all-pages" on stderr, so stdout stays parseable. Envelope docs (README, docs/README.md, teams-agent-contract.7) describe the fields. Tests: wiremock tests of get_paged for a truncated page (next_link kept), the last page (none) and --all-pages (links followed, none); output tests for has_more/next_link in the envelope and for data matching the plain list envelope. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- AGENTS.md | 2 +- CHANGELOG.md | 4 ++ CLAUDE.md | 1 + README.md | 2 + docs/README.md | 2 + docs/man/teams-agent-contract.7 | 14 ++++ src/api/apps.rs | 5 +- src/api/channels.rs | 10 +-- src/api/chats.rs | 18 ++++-- src/api/client.rs | 110 ++++++++++++++++++++++++++++---- src/api/files.rs | 4 +- src/api/meetings.rs | 7 +- src/api/messages.rs | 12 ++-- src/api/subscriptions.rs | 3 +- src/api/tags.rs | 5 +- src/api/teams.rs | 17 ++--- src/api/users.rs | 3 +- src/cli/app.rs | 4 +- src/cli/channel.rs | 8 ++- src/cli/chat.rs | 13 +++- src/cli/file.rs | 4 +- src/cli/meeting.rs | 12 +++- src/cli/message.rs | 5 +- src/cli/message_media.rs | 2 +- src/cli/subscribe.rs | 4 +- src/cli/tab.rs | 4 +- src/cli/tag.rs | 4 +- src/cli/team.rs | 8 ++- src/cli/user.rs | 4 +- src/models/common.rs | 49 ++++++++++++++ src/output/mod.rs | 78 +++++++++++++++++++++- 31 files changed, 346 insertions(+), 72 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d5af05e..c8d9016 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -105,7 +105,7 @@ Most command modules follow this shape: 6. Call `api::::...`. 7. Print through `output::print_success`, `print_success_list`, or a table helper. -For paginated list commands, pass `&PaginationOpts` from `src/cli/mod.rs` and use `client.get_paged` in the API layer. +For paginated list commands, pass `&PaginationOpts` from `src/cli/mod.rs` and use `client.get_paged` in the API layer. It returns a `Page`; print it with `output::print_success_page` (and `output::print_more_hint` after a table) so `metadata.has_more`/`next_link` reach the caller. For new command domains, update: diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c2b46..f897046 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Added + +- List commands without `--all-pages` report whether the result is complete: JSON `metadata.has_more`, plus `metadata.next_link` (Graph's `@odata.nextLink`) when more pages exist. Previously a truncated page looked identical to a complete collection. With `--all-pages`, `has_more` is false. `data` is unchanged. Human and plain output print "More results available; use --all-pages" on stderr. + ## v0.9.0 - 2026-10-04 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 966f413..6c1d55e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,6 +61,7 @@ Single-crate Rust binary. Key modules: ### Output Contract All commands emit a JSON envelope: `{ "success": bool, "data": ..., "metadata": { "request_id", "timestamp", "duration_ms" } }`. +Paged list results add `metadata.has_more` and, when true, `metadata.next_link`; human/plain output prints a stderr hint instead. When stdout is a TTY, defaults to human-readable table format. When piped, defaults to JSON. Plain list columns include keys from every row, with blank cells for absent values. Human message lists include a Subject column; absent message subjects remain omitted in JSON. diff --git a/README.md b/README.md index 356bccf..5557b7e 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,8 @@ Every command, when piped or called programmatically, returns a **JSON envelope* } ``` +Paged list results add `metadata.has_more` (more pages exist) and, when true, `metadata.next_link` (Graph's `@odata.nextLink`). With `--all-pages`, `has_more` is false. Human and plain output note truncation on stderr. + On failure: ```json diff --git a/docs/README.md b/docs/README.md index 95bdc5d..8a15161 100644 --- a/docs/README.md +++ b/docs/README.md @@ -53,6 +53,8 @@ When stdout is piped or called by an agent, the CLI defaults to JSON: } ``` +Paged list results add `metadata.has_more` and, when true, `metadata.next_link` (Graph's `@odata.nextLink`); `--all-pages` gives `has_more: false`. + Errors use the same envelope shape with `success: false` and a stable error code. See [command reference](command-reference.md#exit-codes) and `teams-agent-contract(7)`. ## Official Microsoft references diff --git a/docs/man/teams-agent-contract.7 b/docs/man/teams-agent-contract.7 index 8360522..67d6297 100644 --- a/docs/man/teams-agent-contract.7 +++ b/docs/man/teams-agent-contract.7 @@ -95,6 +95,20 @@ HTTP 5xx and network send failures use exponential backoff. List commands use .B --page-size and can follow all pages with .B --all-pages. +.PP +Paged list results add +.B metadata.has_more +(true when Graph has further pages) and, only when true, +.B metadata.next_link +(Graph's +.B @odata.nextLink\c +). With +.B --all-pages +.B has_more +is false. +.B data +is unchanged. Human and plain output print +"More results available; use --all-pages" on stderr instead. .SH AUTHENTICATION FOR AGENTS For automation that posts normal Teams chat or channel messages, use a delegated profile or a future Teams bot mode. Microsoft Graph app-only tokens diff --git a/src/api/apps.rs b/src/api/apps.rs index d78a8d0..cfa6e4b 100644 --- a/src/api/apps.rs +++ b/src/api/apps.rs @@ -2,6 +2,7 @@ use crate::error::Result; use crate::models::app::{ CreateTabRequest, InstallAppRequest, TeamsAppInstallation, TeamsTab, UpdateTabRequest, }; +use crate::models::common::Page; use super::client::{GraphClient, PaginationOpts}; use super::endpoints; @@ -10,7 +11,7 @@ pub async fn list_team_apps( client: &GraphClient, team_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged( &endpoints::team_installed_apps(team_id), @@ -45,7 +46,7 @@ pub async fn list_tabs( team_id: &str, channel_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged( &endpoints::channel_tabs(team_id, channel_id), diff --git a/src/api/channels.rs b/src/api/channels.rs index 97b1608..bb1c759 100644 --- a/src/api/channels.rs +++ b/src/api/channels.rs @@ -1,5 +1,6 @@ use crate::error::Result; use crate::models::channel::{Channel, ChannelCreateRequest, ChannelUpdateRequest}; +use crate::models::common::Page; use crate::models::member::{AddMemberRequest, ConversationMember}; use super::client::{GraphClient, PaginationOpts}; @@ -9,7 +10,7 @@ pub async fn list_channels( client: &GraphClient, team_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { list_channels_at(client, &endpoints::channels(team_id), pagination).await } @@ -17,7 +18,7 @@ async fn list_channels_at( client: &GraphClient, url: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client.get_paged_without_top(url, &[], pagination).await } @@ -57,7 +58,7 @@ pub async fn list_members( team_id: &str, channel_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged( &endpoints::channel_members(team_id, channel_id), @@ -145,7 +146,8 @@ mod tests { }, ) .await - .unwrap(); + .unwrap() + .items; assert_eq!(channels.len(), 1); assert_eq!(channels[0].display_name.as_deref(), Some("General")); diff --git a/src/api/chats.rs b/src/api/chats.rs index 2db8957..7118823 100644 --- a/src/api/chats.rs +++ b/src/api/chats.rs @@ -1,5 +1,6 @@ use crate::error::Result; use crate::models::chat::{Chat, ChatCreateRequest, ChatUpdateRequest}; +use crate::models::common::Page; use crate::models::member::{AddMemberRequest, ConversationMember}; use super::client::{GraphClient, PaginationOpts}; @@ -31,7 +32,7 @@ pub async fn list_chats( client: &GraphClient, order: ChatOrder, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { list_chats_at(client, &endpoints::my_chats(), order, pagination).await } @@ -40,7 +41,7 @@ async fn list_chats_at( url: &str, order: ChatOrder, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client.get_paged(url, order.query(), pagination).await } @@ -66,7 +67,7 @@ pub async fn list_members( client: &GraphClient, chat_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { list_members_at(client, &endpoints::chat_members(chat_id), pagination).await } @@ -76,7 +77,7 @@ pub(crate) async fn list_members_at( client: &GraphClient, url: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client.get_paged_without_top(url, &[], pagination).await } @@ -173,7 +174,8 @@ mod tests { }, ) .await - .unwrap(); + .unwrap() + .items; assert_eq!(chats.len(), 1); } @@ -201,7 +203,8 @@ mod tests { }, ) .await - .unwrap(); + .unwrap() + .items; assert_eq!(chats.len(), 1); } @@ -233,7 +236,8 @@ mod tests { }, ) .await - .unwrap(); + .unwrap() + .items; assert_eq!(members.len(), 1); assert_eq!(members[0].display_name.as_deref(), Some("Alice")); diff --git a/src/api/client.rs b/src/api/client.rs index 94e6a61..c4f35ff 100644 --- a/src/api/client.rs +++ b/src/api/client.rs @@ -6,7 +6,7 @@ use serde::de::DeserializeOwned; use crate::auth::token::TokenInfo; use crate::config::NetworkConfig; use crate::error::{Result, TeamsError}; -use crate::models::common::PageResponse; +use crate::models::common::{Page, PageResponse}; /// Options for paginated API requests. #[derive(Debug, Clone)] @@ -148,22 +148,19 @@ impl GraphClient { } /// GET with pagination support. Appends `$top` and optionally follows all pages. + /// A single page keeps Graph's `@odata.nextLink`. pub async fn get_paged( &self, url: &str, query: &[(&str, &str)], pagination: &PaginationOpts, - ) -> Result> { + ) -> Result> { let top_str = pagination.page_size.to_string(); let mut full_query: Vec<(&str, &str)> = query.to_vec(); full_query.push(("$top", &top_str)); - if pagination.all_pages { - self.get_all_pages(url, &full_query).await - } else { - let resp: PageResponse = self.get(url, &full_query).await?; - Ok(resp.value) - } + self.get_paged_without_top(url, &full_query, pagination) + .await } /// GET with pagination support for endpoints that reject `$top`. @@ -172,12 +169,12 @@ impl GraphClient { url: &str, query: &[(&str, &str)], pagination: &PaginationOpts, - ) -> Result> { + ) -> Result> { if pagination.all_pages { - self.get_all_pages(url, query).await + Ok(Page::complete(self.get_all_pages(url, query).await?)) } else { let resp: PageResponse = self.get(url, query).await?; - Ok(resp.value) + Ok(resp.into()) } } @@ -991,7 +988,8 @@ mod tests { }, ) .await - .unwrap(); + .unwrap() + .items; assert_eq!(channels.len(), 1); assert_eq!(channels[0]["displayName"], "General"); @@ -1032,10 +1030,96 @@ mod tests { }, ) .await - .unwrap(); + .unwrap() + .items; assert_eq!(channels.len(), 2); assert_eq!(channels[0]["id"], "channel-1"); assert_eq!(channels[1]["id"], "channel-2"); } + + /// Mounts a two-page `/me/chats` collection; returns the second page's URL. + async fn mount_two_pages(server: &MockServer) -> String { + let next = format!("{}/me/chats?$skiptoken=abc", server.uri()); + Mock::given(method("GET")) + .and(path("/me/chats")) + .and(query_param_is_missing("$skiptoken")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [{ "id": "chat-1" }], + "@odata.nextLink": next + }))) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path("/me/chats")) + .and(query_param("$skiptoken", "abc")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [{ "id": "chat-2" }] + }))) + .mount(server) + .await; + next + } + + fn one_page() -> PaginationOpts { + PaginationOpts { + page_size: 1, + all_pages: false, + } + } + + #[tokio::test] + async fn get_paged_keeps_next_link_of_a_truncated_page() { + let server = MockServer::start().await; + let next = mount_two_pages(&server).await; + + let page = test_client() + .get_paged::(&format!("{}/me/chats", server.uri()), &[], &one_page()) + .await + .unwrap(); + + assert_eq!(page.items, vec![serde_json::json!({ "id": "chat-1" })]); + assert_eq!(page.next_link.as_deref(), Some(next.as_str())); + } + + #[tokio::test] + async fn get_paged_reports_no_next_link_on_the_last_page() { + let server = MockServer::start().await; + let next = mount_two_pages(&server).await; + + let page = test_client() + .get_paged::(&next, &[], &one_page()) + .await + .unwrap(); + + assert_eq!(page.items, vec![serde_json::json!({ "id": "chat-2" })]); + assert!(page.next_link.is_none()); + } + + #[tokio::test] + async fn get_paged_all_pages_follows_links_and_reports_no_next_link() { + let server = MockServer::start().await; + mount_two_pages(&server).await; + + let page = test_client() + .get_paged::( + &format!("{}/me/chats", server.uri()), + &[], + &PaginationOpts { + page_size: 1, + all_pages: true, + }, + ) + .await + .unwrap(); + + assert_eq!( + page.items, + vec![ + serde_json::json!({ "id": "chat-1" }), + serde_json::json!({ "id": "chat-2" }) + ] + ); + assert!(page.next_link.is_none()); + } } diff --git a/src/api/files.rs b/src/api/files.rs index dbecb5d..2c6ec84 100644 --- a/src/api/files.rs +++ b/src/api/files.rs @@ -1,5 +1,5 @@ use crate::error::{Result, TeamsError}; -use crate::models::common::PageResponse; +use crate::models::common::{Page, PageResponse}; use crate::models::file::{ DriveInviteRequest, DriveItem, DrivePermission, DriveRecipient, FilesFolder, ShareLinkRequest, ShareLinkResponse, @@ -25,7 +25,7 @@ pub async fn list_files( team_id: &str, channel_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { let folder = get_files_folder(client, team_id, channel_id).await?; let drive_id = folder .parent_reference diff --git a/src/api/meetings.rs b/src/api/meetings.rs index 0b43b61..a183972 100644 --- a/src/api/meetings.rs +++ b/src/api/meetings.rs @@ -1,4 +1,5 @@ use crate::error::Result; +use crate::models::common::Page; use crate::models::meeting::{ AttendanceRecord, CreateMeetingRequest, MeetingAttendanceReport, OnlineMeeting, UpdateMeetingRequest, @@ -10,7 +11,7 @@ use super::endpoints; pub async fn list_meetings( client: &GraphClient, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged(&endpoints::my_online_meetings(), &[], pagination) .await @@ -47,7 +48,7 @@ pub async fn list_attendance_reports( client: &GraphClient, meeting_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged( &endpoints::meeting_attendance_reports(meeting_id), @@ -76,7 +77,7 @@ pub async fn list_attendance_records( meeting_id: &str, report_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged( &endpoints::attendance_records(meeting_id, report_id), diff --git a/src/api/messages.rs b/src/api/messages.rs index d60c67d..99d60e4 100644 --- a/src/api/messages.rs +++ b/src/api/messages.rs @@ -1,4 +1,5 @@ use crate::error::{Result, TeamsError}; +use crate::models::common::Page; use crate::models::message::{ ChatMessage, ChatMessageHostedContent, PinMessageRequest, PinnedMessage, ReactionRequest, ReplyWithQuoteRequest, SendMessageRequest, @@ -164,7 +165,7 @@ pub async fn list_channel_messages( team_id: &str, channel_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged( &endpoints::channel_messages(team_id, channel_id), @@ -273,7 +274,7 @@ pub async fn list_channel_message_replies( channel_id: &str, message_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { list_channel_message_replies_at( client, &endpoints::channel_message_replies(team_id, channel_id, message_id), @@ -286,7 +287,7 @@ async fn list_channel_message_replies_at( client: &GraphClient, url: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client.get_paged(url, &[], pagination).await } @@ -294,7 +295,7 @@ pub async fn list_chat_messages( client: &GraphClient, chat_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged(&endpoints::chat_messages(chat_id), &[], pagination) .await @@ -895,7 +896,8 @@ mod tests { }, ) .await - .unwrap(); + .unwrap() + .items; assert_eq!(replies.len(), 2); assert_eq!(replies[0].id.as_deref(), Some("1700000000002")); diff --git a/src/api/subscriptions.rs b/src/api/subscriptions.rs index 21de584..893856c 100644 --- a/src/api/subscriptions.rs +++ b/src/api/subscriptions.rs @@ -1,4 +1,5 @@ use crate::error::Result; +use crate::models::common::Page; use crate::models::subscription::{ CreateSubscriptionRequest, RenewSubscriptionRequest, Subscription, }; @@ -16,7 +17,7 @@ pub async fn create_subscription( pub async fn list_subscriptions( client: &GraphClient, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged(&endpoints::subscriptions(), &[], pagination) .await diff --git a/src/api/tags.rs b/src/api/tags.rs index 17c65be..7f5073b 100644 --- a/src/api/tags.rs +++ b/src/api/tags.rs @@ -1,4 +1,5 @@ use crate::error::Result; +use crate::models::common::Page; use crate::models::tag::{ AddTagMemberRequest, CreateTagRequest, TeamworkTag, TeamworkTagMember, UpdateTagRequest, }; @@ -10,7 +11,7 @@ pub async fn list_tags( client: &GraphClient, team_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged(&endpoints::team_tags(team_id), &[], pagination) .await @@ -49,7 +50,7 @@ pub async fn list_tag_members( team_id: &str, tag_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged(&endpoints::tag_members(team_id, tag_id), &[], pagination) .await diff --git a/src/api/teams.rs b/src/api/teams.rs index 91fccd4..3c337f5 100644 --- a/src/api/teams.rs +++ b/src/api/teams.rs @@ -1,5 +1,5 @@ use crate::error::Result; -use crate::models::common::PageResponse; +use crate::models::common::Page; use crate::models::member::{AddMemberRequest, ConversationMember}; use crate::models::team::{Team, TeamCloneRequest, TeamCreateRequest, TeamUpdateRequest}; @@ -9,15 +9,10 @@ use super::endpoints; pub async fn list_joined_teams( client: &GraphClient, pagination: &PaginationOpts, -) -> Result> { - if pagination.all_pages { - client - .get_all_pages(&endpoints::my_joined_teams(), &[]) - .await - } else { - let page: PageResponse = client.get(&endpoints::my_joined_teams(), &[]).await?; - Ok(page.value) - } +) -> Result> { + client + .get_paged_without_top(&endpoints::my_joined_teams(), &[], pagination) + .await } pub async fn get_team(client: &GraphClient, id: &str) -> Result { @@ -62,7 +57,7 @@ pub async fn list_members( client: &GraphClient, team_id: &str, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { client .get_paged(&endpoints::team_members(team_id), &[], pagination) .await diff --git a/src/api/users.rs b/src/api/users.rs index 941a0e4..20df547 100644 --- a/src/api/users.rs +++ b/src/api/users.rs @@ -1,4 +1,5 @@ use crate::error::Result; +use crate::models::common::Page; use crate::models::user::User; use super::client::{GraphClient, PaginationOpts}; @@ -20,7 +21,7 @@ pub async fn list_users( client: &GraphClient, filter: Option<&str>, pagination: &PaginationOpts, -) -> Result> { +) -> Result> { let mut query: Vec<(&str, &str)> = vec![]; if let Some(f) = filter { query.push(("$filter", f)); diff --git a/src/cli/app.rs b/src/cli/app.rs index 7916822..1d339ec 100644 --- a/src/cli/app.rs +++ b/src/cli/app.rs @@ -50,6 +50,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["Installation ID", "App Name", "Version", "Distribution"]; let rows: Vec> = apps + .items .iter() .map(|a| { let app = a.teams_app.as_ref(); @@ -64,8 +65,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&apps); } else { - output::print_success_list(format, &apps, start); + output::print_success_page(format, &apps, start); } Ok(()) } diff --git a/src/cli/channel.rs b/src/cli/channel.rs index 74a9d23..9fdb4de 100644 --- a/src/cli/channel.rs +++ b/src/cli/channel.rs @@ -114,6 +114,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["ID", "Display Name", "Type", "Email"]; let rows: Vec> = channels + .items .iter() .map(|c| { vec![ @@ -125,8 +126,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&channels); } else { - output::print_success_list(format, &channels, start); + output::print_success_page(format, &channels, start); } Ok(()) } @@ -209,6 +211,7 @@ async fn run_members( if format == OutputFormat::Human { let headers = vec!["ID", "Display Name", "Roles", "Email"]; let rows: Vec> = members + .items .iter() .map(|m| { vec![ @@ -220,8 +223,9 @@ async fn run_members( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&members); } else { - output::print_success_list(format, &members, start); + output::print_success_page(format, &members, start); } Ok(()) } diff --git a/src/cli/chat.rs b/src/cli/chat.rs index e8866e7..b791fb8 100644 --- a/src/cli/chat.rs +++ b/src/cli/chat.rs @@ -115,10 +115,15 @@ pub async fn run( ChatOrder::Default => vec!["ID", "Topic", "Type", "Last Updated"], ChatOrder::Activity => vec!["ID", "Topic", "Type", "Last Message", "From"], }; - let rows = chats.iter().map(|c| chat_list_row(c, order)).collect(); + let rows = chats + .items + .iter() + .map(|c| chat_list_row(c, order)) + .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&chats); } else { - output::print_success_list(format, &chats, start); + output::print_success_page(format, &chats, start); } Ok(()) } @@ -255,6 +260,7 @@ async fn run_members( if format == OutputFormat::Human { let headers = vec!["ID", "Display Name", "Roles", "Email"]; let rows: Vec> = members + .items .iter() .map(|m| { vec![ @@ -266,8 +272,9 @@ async fn run_members( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&members); } else { - output::print_success_list(format, &members, start); + output::print_success_page(format, &members, start); } Ok(()) } diff --git a/src/cli/file.rs b/src/cli/file.rs index 938d5f3..47661a7 100644 --- a/src/cli/file.rs +++ b/src/cli/file.rs @@ -113,6 +113,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["ID", "Name", "Size", "Type", "Modified"]; let rows: Vec> = files + .items .iter() .map(|f| { let file_type = if f.folder.is_some() { "folder" } else { "file" }; @@ -126,8 +127,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&files); } else { - output::print_success_list(format, &files, start); + output::print_success_page(format, &files, start); } Ok(()) } diff --git a/src/cli/meeting.rs b/src/cli/meeting.rs index 4aed776..afbeb09 100644 --- a/src/cli/meeting.rs +++ b/src/cli/meeting.rs @@ -86,6 +86,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["ID", "Subject", "Start", "End", "Join URL"]; let rows: Vec> = meetings + .items .iter() .map(|m| { vec![ @@ -98,8 +99,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&meetings); } else { - output::print_success_list(format, &meetings, start); + output::print_success_page(format, &meetings, start); } Ok(()) } @@ -183,6 +185,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["ID", "Email", "Role", "Attendance (s)"]; let rows: Vec> = records + .items .iter() .map(|r| { vec![ @@ -196,8 +199,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&records); } else { - output::print_success_list(format, &records, start); + output::print_success_page(format, &records, start); } } else { let reports = @@ -206,6 +210,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["Report ID", "Participants", "Start", "End"]; let rows: Vec> = reports + .items .iter() .map(|r| { vec![ @@ -219,8 +224,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&reports); } else { - output::print_success_list(format, &reports, start); + output::print_success_page(format, &reports, start); } } Ok(()) diff --git a/src/cli/message.rs b/src/cli/message.rs index 421723b..383cd07 100644 --- a/src/cli/message.rs +++ b/src/cli/message.rs @@ -476,10 +476,11 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["ID", "From", "Subject", "Body Preview", "Date"]; - let rows: Vec> = messages.iter().map(message_list_row).collect(); + let rows: Vec> = messages.items.iter().map(message_list_row).collect(); output::table::print_table(headers, rows); + output::print_more_hint(&messages); } else { - output::print_success_list(format, &messages, start); + output::print_success_page(format, &messages, start); } Ok(()) } diff --git a/src/cli/message_media.rs b/src/cli/message_media.rs index 10b28b6..5193cb5 100644 --- a/src/cli/message_media.rs +++ b/src/cli/message_media.rs @@ -134,7 +134,7 @@ async fn chat_recipients_at( }, ) .await?; - Ok(invite_recipients(&members, me.id.as_deref())) + Ok(invite_recipients(&members.items, me.id.as_deref())) } .await; match lookup { diff --git a/src/cli/subscribe.rs b/src/cli/subscribe.rs index fad8740..d70bf94 100644 --- a/src/cli/subscribe.rs +++ b/src/cli/subscribe.rs @@ -92,6 +92,7 @@ pub async fn run( "Notification URL", ]; let rows: Vec> = subs + .items .iter() .map(|s| { vec![ @@ -104,8 +105,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&subs); } else { - output::print_success_list(format, &subs, start); + output::print_success_page(format, &subs, start); } Ok(()) } diff --git a/src/cli/tab.rs b/src/cli/tab.rs index 858cdb9..04875cc 100644 --- a/src/cli/tab.rs +++ b/src/cli/tab.rs @@ -65,6 +65,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["Tab ID", "Display Name", "App Name", "Web URL"]; let rows: Vec> = tabs + .items .iter() .map(|t| { vec![ @@ -79,8 +80,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&tabs); } else { - output::print_success_list(format, &tabs, start); + output::print_success_page(format, &tabs, start); } Ok(()) } diff --git a/src/cli/tag.rs b/src/cli/tag.rs index 27fd816..6a4b366 100644 --- a/src/cli/tag.rs +++ b/src/cli/tag.rs @@ -91,6 +91,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["ID", "Display Name", "Description", "Member Count"]; let rows: Vec> = tags + .items .iter() .map(|t| { vec![ @@ -102,8 +103,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&tags); } else { - output::print_success_list(format, &tags, start); + output::print_success_page(format, &tags, start); } Ok(()) } diff --git a/src/cli/team.rs b/src/cli/team.rs index 5b7f542..fec7821 100644 --- a/src/cli/team.rs +++ b/src/cli/team.rs @@ -121,6 +121,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["ID", "Display Name", "Description", "Archived"]; let rows: Vec> = teams + .items .iter() .map(|t| { vec![ @@ -132,8 +133,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&teams); } else { - output::print_success_list(format, &teams, start); + output::print_success_page(format, &teams, start); } Ok(()) } @@ -242,6 +244,7 @@ async fn run_members( if format == OutputFormat::Human { let headers = vec!["ID", "Display Name", "Roles", "Email"]; let rows: Vec> = members + .items .iter() .map(|m| { vec![ @@ -253,8 +256,9 @@ async fn run_members( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&members); } else { - output::print_success_list(format, &members, start); + output::print_success_page(format, &members, start); } Ok(()) } diff --git a/src/cli/user.rs b/src/cli/user.rs index c0a5bf2..f21e416 100644 --- a/src/cli/user.rs +++ b/src/cli/user.rs @@ -66,6 +66,7 @@ pub async fn run( if format == OutputFormat::Human { let headers = vec!["ID", "Display Name", "Email", "Job Title"]; let rows: Vec> = users + .items .iter() .map(|u| { vec![ @@ -80,8 +81,9 @@ pub async fn run( }) .collect(); output::table::print_table(headers, rows); + output::print_more_hint(&users); } else { - output::print_success_list(format, &users, start); + output::print_success_page(format, &users, start); } Ok(()) } diff --git a/src/models/common.rs b/src/models/common.rs index 84d89d4..8fa0c9f 100644 --- a/src/models/common.rs +++ b/src/models/common.rs @@ -8,6 +8,9 @@ pub struct Metadata { pub api_version: Option, #[serde(skip_serializing_if = "Option::is_none")] pub duration_ms: Option, + /// Set on paged list results: true when Graph has further pages. + #[serde(skip_serializing_if = "Option::is_none")] + pub has_more: Option, #[serde(skip_serializing_if = "Option::is_none")] pub next_link: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -21,6 +24,7 @@ impl Metadata { timestamp: chrono::Utc::now().to_rfc3339(), api_version: Some("v1.0".to_string()), duration_ms: None, + has_more: None, next_link: None, total_count: None, } @@ -30,6 +34,12 @@ impl Metadata { self.duration_ms = Some(ms); self } + + pub fn with_page(mut self, next_link: Option<&str>) -> Self { + self.has_more = Some(next_link.is_some()); + self.next_link = next_link.map(str::to_string); + self + } } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -109,6 +119,31 @@ pub struct PageResponse { pub next_link: Option, } +/// A list result: one page, or every page with `next_link` None. +#[derive(Debug, Clone)] +pub struct Page { + pub items: Vec, + pub next_link: Option, +} + +impl Page { + pub fn complete(items: Vec) -> Self { + Self { + items, + next_link: None, + } + } +} + +impl From> for Page { + fn from(resp: PageResponse) -> Self { + Self { + items: resp.value, + next_link: resp.next_link, + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -138,6 +173,20 @@ mod tests { assert_eq!(json["error"]["message"], "bad credentials"); } + #[test] + fn page_metadata_reports_continuation() { + let more = serde_json::to_value(Metadata::new().with_page(Some("https://next"))).unwrap(); + assert_eq!(more["has_more"], true); + assert_eq!(more["next_link"], "https://next"); + + let last = serde_json::to_value(Metadata::new().with_page(None)).unwrap(); + assert_eq!(last["has_more"], false); + assert!(last.get("next_link").is_none()); + + let plain = serde_json::to_value(Metadata::new()).unwrap(); + assert!(plain.get("has_more").is_none()); + } + #[test] fn metadata_has_uuid_and_timestamp() { let meta = Metadata::new(); diff --git a/src/output/mod.rs b/src/output/mod.rs index 5dfacf7..e9bbd8e 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -4,7 +4,7 @@ pub mod progress; pub mod table; use crate::error::TeamsError; -use crate::models::common::{Envelope, Metadata}; +use crate::models::common::{Envelope, Metadata, Page}; use serde::Serialize; use std::io::{self, IsTerminal, Write}; use std::time::Instant; @@ -99,6 +99,36 @@ pub fn print_success_list(format: OutputFormat, data: &[T], start: } } +/// Print a paged list result. JSON metadata carries `has_more` and +/// `next_link`; other formats note truncation on stderr. +pub fn print_success_page(format: OutputFormat, page: &Page, start: Instant) { + match format { + OutputFormat::Json => { + json::print(&page_envelope(page, start.elapsed().as_millis() as u64)); + } + OutputFormat::Human | OutputFormat::Plain => { + print_success_list(format, &page.items, start); + print_more_hint(page); + } + } +} + +fn page_envelope(page: &Page, duration_ms: u64) -> Envelope<&[T]> { + let metadata = Metadata::new() + .with_duration(duration_ms) + .with_page(page.next_link.as_deref()); + Envelope::success(page.items.as_slice(), metadata) +} + +/// Stderr footer for a truncated list; for table output. +pub fn print_more_hint(page: &Page) { + if page.next_link.is_some() { + eprintln!("{MORE_RESULTS_HINT}"); + } +} + +const MORE_RESULTS_HINT: &str = "More results available; use --all-pages"; + pub fn print_error(format: OutputFormat, err: &TeamsError) { let metadata = Metadata::new(); match format { @@ -116,3 +146,49 @@ pub fn print_error(format: OutputFormat, err: &TeamsError) { } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn items() -> Vec { + vec![ + serde_json::json!({ "id": "a" }), + serde_json::json!({ "id": "b" }), + ] + } + + fn render(next_link: Option<&str>) -> serde_json::Value { + let page = Page { + items: items(), + next_link: next_link.map(str::to_string), + }; + serde_json::to_value(page_envelope(&page, 1)).unwrap() + } + + #[test] + fn truncated_page_reports_has_more_and_next_link() { + let json = render(Some( + "https://graph.microsoft.com/v1.0/me/chats?$skiptoken=x", + )); + assert_eq!(json["metadata"]["has_more"], true); + assert_eq!( + json["metadata"]["next_link"], + "https://graph.microsoft.com/v1.0/me/chats?$skiptoken=x" + ); + } + + #[test] + fn complete_page_reports_no_more_and_omits_next_link() { + let json = render(None); + assert_eq!(json["metadata"]["has_more"], false); + assert!(json["metadata"].get("next_link").is_none()); + } + + #[test] + fn page_data_matches_plain_list_envelope() { + let list = serde_json::to_value(Envelope::success(&items(), Metadata::new())).unwrap(); + assert_eq!(render(Some("https://next"))["data"], list["data"]); + assert_eq!(render(None)["data"], list["data"]); + } +} From d321216b741fb2ac4dc77a995cb995939d82126b Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:19:47 +0100 Subject: [PATCH 08/11] Write downloads through an exclusive temp file Attachment downloads wrote to a fixed `.part`, opened with truncate and no exclusive create, then renamed it into place. A symlink planted at that path redirected the write to its target, a stray `.part` was clobbered, and two downloads to one target shared one temp file. `file download --path` wrote straight to the target, leaving a truncated file if the write failed. Both now create a uniquely named temp file in the target directory with O_EXCL (tempfile::Builder::tempfile_in), write, and persist it over the target. The temp file is removed on failure. An existing target is still overwritten and keeps its mode; a new one gets 0666 less umask as before. `attachments download --path -` is unchanged. tempfile moves from dev-dependencies to dependencies for its exclusive, self-cleaning temp file and atomic persist. Tests: symlink at the old `.part` path left alone with its target unmodified; stray `.part` untouched; a failed persist leaves no temp file; two concurrent writers leave one whole file; mode kept on replace; long filenames still fit. On Windows a rename onto a file another rename is replacing can be refused ("Access is denied"), so the concurrent-writers test lets one writer lose there; the target must still be one whole payload with no temp file left. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 4 + Cargo.toml | 2 +- docs/attachments-spec.md | 3 +- src/cli/file.rs | 7 +- src/cli/message_attachments.rs | 185 +++++++++++++++++++++++++++++---- 5 files changed, 171 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c2b46..76812e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Fixed + +- `message attachments download` and `file download --path` write through a uniquely named, exclusively created temp file in the target directory, then rename it into place. Attachments used a fixed `.part`, so a symlink there redirected the write, a stray `.part` was clobbered, and concurrent downloads to one target shared a temp file; `file download` wrote in place, through any symlink at the target, and left a truncated file on failure; a symlink at the target is now replaced. A failed write now leaves no temp file. An existing target is still overwritten and keeps its mode, and `message attachments download --path -` still streams to stdout. + ## v0.9.0 - 2026-10-04 ### Added diff --git a/Cargo.toml b/Cargo.toml index e36c7ac..c6767dc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -46,10 +46,10 @@ anyhow = "1" urlencoding = "2" bytes = "1" mime_guess = "2" +tempfile = "3" [dev-dependencies] wiremock = "0.6" -tempfile = "3" assert_cmd = "2" predicates = "3" diff --git a/docs/attachments-spec.md b/docs/attachments-spec.md index f8c9401..db682c5 100644 --- a/docs/attachments-spec.md +++ b/docs/attachments-spec.md @@ -269,7 +269,8 @@ Ordered, each step compiles and passes tests independently. - 404 on hostedContent (expired/deleted) → exit 5, name the index and message ID. - Partial failures download what they can, then exit non-zero with an error that names the failed item and lists the paths already saved. -- Never write partial files: download to `{path}.part`, rename on success. +- Never write partial files: download to a uniquely named, exclusively created temp file + beside the target, rename on success. ### Security notes diff --git a/src/cli/file.rs b/src/cli/file.rs index 938d5f3..fabc38e 100644 --- a/src/cli/file.rs +++ b/src/cli/file.rs @@ -205,12 +205,7 @@ pub async fn run( let bytes = api::files::download_file(&client, &team, &channel, &file_id).await?; if let Some(path) = path { - std::fs::write(&path, &bytes).map_err(|e| { - crate::error::TeamsError::InvalidInput(format!( - "Failed to write file '{}': {e}", - path - )) - })?; + super::message_attachments::write_atomically(std::path::Path::new(&path), &bytes)?; if format != OutputFormat::Plain { let start = Instant::now(); let result = serde_json::json!({ diff --git a/src/cli/message_attachments.rs b/src/cli/message_attachments.rs index 75218fe..624dfde 100644 --- a/src/cli/message_attachments.rs +++ b/src/cli/message_attachments.rs @@ -294,6 +294,16 @@ async fn download_item( None => unique_path(dir.join(default_filename(message_id, item, content_type.as_deref()))), }; + if let Some(parent) = target.parent() { + if !parent.as_os_str().is_empty() && !parent.exists() { + std::fs::create_dir_all(parent).map_err(|e| { + TeamsError::InvalidInput(format!( + "Failed to create directory '{}': {e}", + parent.display() + )) + })?; + } + } write_atomically(&target, &bytes)?; Ok(serde_json::json!({ @@ -376,29 +386,44 @@ fn unique_path(candidate: PathBuf) -> PathBuf { .expect("unbounded suffix search always terminates") } -/// Write to `.part` then rename, so a failed download never leaves a -/// truncated file at the final path. -fn write_atomically(target: &Path, bytes: &[u8]) -> Result<()> { - if let Some(parent) = target.parent() { - if !parent.as_os_str().is_empty() && !parent.exists() { - std::fs::create_dir_all(parent).map_err(|e| { - TeamsError::InvalidInput(format!( - "Failed to create directory '{}': {e}", - parent.display() - )) - })?; - } +/// Write to a uniquely named, exclusively created temp file beside `target`, +/// then rename over it. A failed download leaves nothing behind, and an +/// existing file or symlink at a temp path is never followed or clobbered. +pub(crate) fn write_atomically(target: &Path, bytes: &[u8]) -> Result<()> { + use std::io::Write; + + let fail = |e: std::io::Error| { + TeamsError::InvalidInput(format!("Failed to write '{}': {e}", target.display())) + }; + if target.file_name().is_none() { + return Err(TeamsError::InvalidInput( + "Target path has no filename".into(), + )); + } + let dir = match target.parent() { + Some(p) if !p.as_os_str().is_empty() => p, + _ => Path::new("."), + }; + let mut builder = tempfile::Builder::new(); + builder.prefix(".teams-download-").suffix(".part"); + // Keep an existing target's mode exactly; otherwise 0666 less umask, as a + // plain create would, not tempfile's 0600. + #[cfg(unix)] + let existing = std::fs::metadata(target).ok().map(|m| m.permissions()); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = existing.as_ref().map_or(0o666, |p| p.mode() & 0o777); + builder.permissions(std::fs::Permissions::from_mode(mode)); + } + let mut tmp = builder.tempfile_in(dir).map_err(fail)?; + #[cfg(unix)] + if let Some(perms) = existing { + tmp.as_file().set_permissions(perms).map_err(fail)?; } - let filename = target - .file_name() - .ok_or_else(|| TeamsError::InvalidInput("Target path has no filename".into()))?; - let part = target.with_file_name(format!("{}.part", filename.to_string_lossy())); - std::fs::write(&part, bytes).map_err(|e| { - TeamsError::InvalidInput(format!("Failed to write '{}': {e}", part.display())) - })?; - std::fs::rename(&part, target).map_err(|e| { - TeamsError::InvalidInput(format!("Failed to rename to '{}': {e}", target.display())) - }) + tmp.write_all(bytes).map_err(fail)?; + tmp.persist(target).map_err(|e| fail(e.error))?; + Ok(()) } fn augment_error(err: TeamsError, note: &str) -> TeamsError { @@ -461,6 +486,122 @@ mod tests { write_atomically(&no_extension, b"more").unwrap(); assert_eq!(std::fs::read(&no_extension).unwrap(), b"more"); assert!(!dir.path().join("out.part").exists()); + assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 2); + } + + #[cfg(unix)] + #[test] + fn write_atomically_ignores_symlink_at_old_part_path() { + let dir = tempfile::tempdir().unwrap(); + let victim = dir.path().join("victim"); + std::fs::write(&victim, b"keep").unwrap(); + let part = dir.path().join("out.png.part"); + std::os::unix::fs::symlink(&victim, &part).unwrap(); + let target = dir.path().join("out.png"); + + write_atomically(&target, b"payload").unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"payload"); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep"); + assert!(std::fs::symlink_metadata(&part) + .unwrap() + .file_type() + .is_symlink()); + } + + #[cfg(unix)] + #[test] + fn write_atomically_keeps_default_file_mode() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let plain = dir.path().join("plain"); + std::fs::write(&plain, b"x").unwrap(); + let target = dir.path().join("out.png"); + + write_atomically(&target, b"payload").unwrap(); + + let mode = |p: &Path| std::fs::metadata(p).unwrap().permissions().mode() & 0o777; + assert_eq!(mode(&target), mode(&plain)); + + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o600)).unwrap(); + write_atomically(&target, b"again").unwrap(); + assert_eq!(mode(&target), 0o600); + + // Bits the umask would strip on create survive a replace. + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o664)).unwrap(); + write_atomically(&target, b"third").unwrap(); + assert_eq!(mode(&target), 0o664); + } + + #[cfg(unix)] + #[test] + fn write_atomically_accepts_long_filename() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join(format!("{}.bin", "a".repeat(246))); + write_atomically(&target, b"payload").unwrap(); + assert_eq!(std::fs::read(&target).unwrap(), b"payload"); + } + + #[test] + fn write_atomically_leaves_stray_part_file_alone() { + let dir = tempfile::tempdir().unwrap(); + let part = dir.path().join("out.png.part"); + std::fs::write(&part, b"stray").unwrap(); + let target = dir.path().join("out.png"); + + write_atomically(&target, b"payload").unwrap(); + + assert_eq!(std::fs::read(&target).unwrap(), b"payload"); + assert_eq!(std::fs::read(&part).unwrap(), b"stray"); + } + + #[test] + fn write_atomically_failure_leaves_no_temp_file() { + let dir = tempfile::tempdir().unwrap(); + // A directory at the target makes the final rename fail after the write. + let target = dir.path().join("out.png"); + std::fs::create_dir(&target).unwrap(); + + assert!(write_atomically(&target, b"payload").is_err()); + + assert!(target.is_dir()); + let entries: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .map(|e| e.unwrap().file_name()) + .collect(); + assert_eq!(entries, vec![std::ffi::OsString::from("out.png")]); + } + + #[test] + fn write_atomically_concurrent_writers_leave_one_whole_file() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("out.bin"); + let payloads = [vec![b'a'; 4 << 20], vec![b'b'; 4 << 20]]; + let barrier = std::sync::Barrier::new(payloads.len()); + + std::thread::scope(|s| { + let handles: Vec<_> = payloads + .iter() + .map(|p| { + let (target, barrier) = (&target, &barrier); + s.spawn(move || { + barrier.wait(); + write_atomically(target, p) + }) + }) + .collect(); + let results: Vec<_> = handles.into_iter().map(|h| h.join().unwrap()).collect(); + // Windows can refuse a rename onto a file another rename is replacing + // ("Access is denied"); one writer may lose there. Elsewhere both win. + let failed = results.iter().filter(|r| r.is_err()).count(); + assert!(failed == 0 || (cfg!(windows) && failed == 1), "{results:?}"); + }); + + // Either way the target is one whole payload, never a mix, and the + // losing writer's temp file is gone. + let written = std::fs::read(&target).unwrap(); + assert!(payloads.contains(&written)); + assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1); } #[test] From 9c3952051569a7f0736c629f0b57d350796d3fc8 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:24:50 +0100 Subject: [PATCH 09/11] Add message attachments delete for OneDrive uploads `message send --chat C --attach FILE` uploads FILE to the sender's OneDrive ("Microsoft Teams Chat Files"), but nothing in the CLI could remove it: `file delete` only handles channel files, and deleting the message leaves the upload behind. `teams message attachments delete` takes the same targets as `attachments list` plus `--index N` and `--yes`. Each selected `reference` attachment is resolved via GET /shares/{u!token}/driveItem and deleted with DELETE /drives/{id}/items/{id} only when it is a file whose parentReference.driveId matches GET /me/drive. Inline images, cards, folders, other people's files and channel files are skipped with a reason; so is a share Graph reports as 403/404, since that cannot be told apart from a file the caller cannot see. A DELETE 404 counts as deleted. Other Graph failures are errors that name the items already deleted. Missing `--yes` is refused before any token is resolved (exit 2), as in `message delete`. The message is not changed. Tested with wiremock at the handler layer (share token path, own-drive check, delete, 404 as deleted, foreign drive and folder never deleted, inline image skipped, --index selection, partial-failure notes) and CLI tests for --yes and target conflicts. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 4 + README.md | 7 + docs/command-reference.md | 3 + docs/man/teams.1 | 19 ++ src/api/endpoints.rs | 31 ++- src/api/files.rs | 57 +++- src/cli/message.rs | 10 +- src/cli/message_attachments.rs | 484 ++++++++++++++++++++++++++++++++- src/models/file.rs | 6 + tests/cli.rs | 94 +++++++ 10 files changed, 710 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76812e6..9537b13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - `message attachments download` and `file download --path` write through a uniquely named, exclusively created temp file in the target directory, then rename it into place. Attachments used a fixed `.part`, so a symlink there redirected the write, a stray `.part` was clobbered, and concurrent downloads to one target shared a temp file; `file download` wrote in place, through any symlink at the target, and left a truncated file on failure; a symlink at the target is now replaced. A failed write now leaves no temp file. An existing target is still overwritten and keeps its mode, and `message attachments download --path -` still streams to stdout. +### Added + +- `teams message attachments delete (--chat C | --team T --channel CH [--reply R]) --message M [--index N] --yes` deletes files attached with `message send --attach` from the sender's OneDrive. Each `reference` attachment is resolved through Graph's shares API and deleted only when it is in the caller's own drive; inline images, other people's files and channel files are skipped with a reason. A file already gone counts as deleted. The message and its file card are left as they are; `message delete` is separate. + ## v0.9.0 - 2026-10-04 ### Added diff --git a/README.md b/README.md index 356bccf..f1d97b3 100644 --- a/README.md +++ b/README.md @@ -393,6 +393,7 @@ teams message list --team --channel teams message list --team --channel --message-id --all-pages teams message list --chat teams message get --team --channel --message +teams message attachments delete --chat --message [--index N] --yes teams message reply --team --channel --message --mention --body "Thanks!" teams message update --team --channel --message --body "Corrected" teams message update --chat --message --body "Corrected" @@ -417,6 +418,12 @@ confirmed to match the sender's; other recipients use email. Lookup or sharing failures warn on stderr and allow upload/send to continue. Members without a usable address are reported for manual sharing from OneDrive. +`message attachments delete ... --yes` removes those uploads from your OneDrive +(`--index N` for one item, numbered as in `attachments list`). Only files in +your own OneDrive are deleted; inline images, files shared by others and +channel files are skipped with a reason. The message and its file card stay; +use `message delete` for the message. + The `--mention USER` flag on `message send` and `message reply` tags a person as a real Teams @mention — the kind that pings them, not literal `@Name` text. The flag is repeatable and `USER` may be an Entra object ID or UPN; the display name is resolved through Microsoft Graph. It works for chat sends, diff --git a/docs/command-reference.md b/docs/command-reference.md index f86c631..47990f4 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -111,6 +111,7 @@ teams message list (--team TEAM_ID --channel CHANNEL_ID [--message-id ROOT_MESSA teams message get --team TEAM_ID --channel CHANNEL_ID (MESSAGE_ID | --message MESSAGE_ID) [--with-attachments] teams message attachments list (--team TEAM_ID --channel CHANNEL_ID [--reply REPLY_ID] | --chat CHAT_ID) (MESSAGE_ID | --message MESSAGE_ID) teams message attachments download (--team TEAM_ID --channel CHANNEL_ID [--reply REPLY_ID] | --chat CHAT_ID) (MESSAGE_ID | --message MESSAGE_ID) [--index N] [--dir DIR | --path FILE] +teams message attachments delete (--team TEAM_ID --channel CHANNEL_ID [--reply REPLY_ID] | --chat CHAT_ID) (MESSAGE_ID | --message MESSAGE_ID) [--index N] --yes teams message reply --team TEAM_ID --channel CHANNEL_ID --message-id MESSAGE_ID [--body TEXT | --stdin] [--content-type text|html] [--image PATH]... [--attach PATH]... [--mention USER]... teams message update (--team TEAM_ID --channel CHANNEL_ID | --chat CHAT_ID) (MESSAGE_ID | --message MESSAGE_ID) --body TEXT [--content-type text|html] teams message delete (--team TEAM_ID --channel CHANNEL_ID [--reply REPLY_ID] | --chat CHAT_ID) (MESSAGE_ID | --message MESSAGE_ID) --yes @@ -172,6 +173,8 @@ teams message send --chat CHAT_ID --quote MESSAGE_ID --body "Yes, that works." `message attachments` unifies the two ways Teams stores message media: inline images pasted into the compose box (Graph "hosted contents") and files attached via SharePoint/OneDrive (`reference` attachments). `list` returns an indexed inventory; `download` fetches everything downloadable by default, or one item with `--index` (add `--path FILE` for an exact destination, or `--path -` to stream to stdout). Inline images and code snippets need no scopes beyond message reads; file attachments additionally require the `Files.Read.All` delegated scope. `message get --with-attachments` embeds the same inventory under `attachment_items` in the message output. +`message attachments delete` removes files you attached (`message send --attach`) from your OneDrive. Each selected `reference` item is resolved through Graph's `/shares/{token}/driveItem` and deleted only when it is a file in your own drive (`/me/drive`); inline images, code snippets, cards, files shared by others and channel files are skipped with a `reason`, and so is a file Graph cannot find or let you see. Without `--index` every item is considered; `--index N` uses the numbering of `attachments list`. Output lists `{index, name, deleted, reason?}` per item; a file already gone counts as deleted (`reason: "already deleted"`). A Graph failure on a delete is an error, naming items deleted before it. Needs `Files.ReadWrite`, and `--yes` (without it the command exits with code 2 and sends nothing). The message itself is not changed: its file card remains, now pointing at a missing file. Use `message delete` to remove the message. + ## Chats ```bash diff --git a/docs/man/teams.1 b/docs/man/teams.1 index d100036..9ab06f0 100644 --- a/docs/man/teams.1 +++ b/docs/man/teams.1 @@ -161,6 +161,7 @@ teams message list (--team TEAM_ID --channel CHANNEL_ID [--message-id ROOT_MESSA teams message get --team TEAM_ID --channel CHANNEL_ID (MESSAGE_ID | --message MESSAGE_ID) [--with-attachments] teams message attachments list (--team TEAM_ID --channel CHANNEL_ID [--reply REPLY_ID] | --chat CHAT_ID) (MESSAGE_ID | --message MESSAGE_ID) teams message attachments download (--team TEAM_ID --channel CHANNEL_ID [--reply REPLY_ID] | --chat CHAT_ID) (MESSAGE_ID | --message MESSAGE_ID) [--index N] [--dir DIR | --path FILE] +teams message attachments delete (--team TEAM_ID --channel CHANNEL_ID [--reply REPLY_ID] | --chat CHAT_ID) (MESSAGE_ID | --message MESSAGE_ID) [--index N] --yes teams message reply --team TEAM_ID --channel CHANNEL_ID --message-id MESSAGE_ID [--body TEXT | --stdin] [--content-type text|html] [--image PATH]... [--attach PATH]... [--mention USER]... teams message reply --team TEAM_ID --channel CHANNEL_ID --message MESSAGE_ID --body TEXT teams message update (--team TEAM_ID --channel CHANNEL_ID | --chat CHAT_ID) (MESSAGE_ID | --message MESSAGE_ID) --body TEXT [--content-type text|html] @@ -245,6 +246,24 @@ the sender and recipient share a tenant; other recipients use email. Lookup failures, members without a usable address, and grant failures warn on stderr with manual OneDrive sharing guidance. Lookup or sharing failures do not stop upload/send. +.PP +.B message attachments delete +removes files you attached from your OneDrive. Each selected reference +attachment is resolved through /shares/{token}/driveItem and deleted only +when it lives in your own drive (/me/drive). Inline images, code snippets, +cards, files shared by others, channel files and files Graph cannot find +are skipped with a reason. Without +.B --index +every item is considered; +.B --index N +uses the numbering of +.B attachments list. +A file already gone counts as deleted. Needs Files.ReadWrite and +.B --yes +(without it the command exits with code 2). The message and its file card +remain; use +.B message delete +for the message. .SH CHAT COMMANDS .nf teams chat list [--order-by activity] diff --git a/src/api/endpoints.rs b/src/api/endpoints.rs index 67bf487..18500f4 100644 --- a/src/api/endpoints.rs +++ b/src/api/endpoints.rs @@ -324,7 +324,11 @@ pub fn channel_files_folder(team_id: &str, channel_id: &str) -> String { } pub fn drive_item(drive_id: &str, item_id: &str) -> String { - format!("{GRAPH_V1}/drives/{drive_id}/items/{item_id}") + drive_item_at(GRAPH_V1, drive_id, item_id) +} + +pub fn drive_item_at(base: &str, drive_id: &str, item_id: &str) -> String { + format!("{base}/drives/{drive_id}/items/{item_id}") } pub fn drive_item_children(drive_id: &str, item_id: &str) -> String { @@ -350,6 +354,10 @@ pub fn drive_upload_to_folder_rename(drive_id: &str, parent_id: &str, filename: ) } +pub fn me_drive_at(base: &str) -> String { + format!("{base}/me/drive") +} + pub fn me_drive_root_children() -> String { format!("{GRAPH_V1}/me/drive/root/children") } @@ -456,6 +464,10 @@ pub fn shares_drive_item_content(sharing_token: &str) -> String { format!("{GRAPH_V1}/shares/{sharing_token}/driveItem/content") } +pub fn shares_drive_item_at(base: &str, sharing_token: &str) -> String { + format!("{base}/shares/{sharing_token}/driveItem") +} + #[cfg(test)] mod tests { use super::*; @@ -489,6 +501,23 @@ mod tests { ); } + #[test] + fn attachment_delete_urls() { + let url = "https://tenant-my.sharepoint.com/personal/user/Documents/Microsoft%20Teams%20Chat%20Files/NetskopeLogs.zip"; + assert_eq!( + shares_drive_item_at(GRAPH_V1, &sharing_url_token(url)), + "https://graph.microsoft.com/v1.0/shares/u!aHR0cHM6Ly90ZW5hbnQtbXkuc2hhcmVwb2ludC5jb20vcGVyc29uYWwvdXNlci9Eb2N1bWVudHMvTWljcm9zb2Z0JTIwVGVhbXMlMjBDaGF0JTIwRmlsZXMvTmV0c2tvcGVMb2dzLnppcA/driveItem" + ); + assert_eq!( + me_drive_at(GRAPH_V1), + "https://graph.microsoft.com/v1.0/me/drive" + ); + assert_eq!( + drive_item("d1", "i1"), + "https://graph.microsoft.com/v1.0/drives/d1/items/i1" + ); + } + #[test] fn hosted_content_value_encodes_id() { // Base64 IDs can contain '+', '/', and '=' which must be percent-encoded in a path. diff --git a/src/api/files.rs b/src/api/files.rs index dbecb5d..4ef6bf8 100644 --- a/src/api/files.rs +++ b/src/api/files.rs @@ -1,8 +1,8 @@ use crate::error::{Result, TeamsError}; use crate::models::common::PageResponse; use crate::models::file::{ - DriveInviteRequest, DriveItem, DrivePermission, DriveRecipient, FilesFolder, ShareLinkRequest, - ShareLinkResponse, + Drive, DriveInviteRequest, DriveItem, DrivePermission, DriveRecipient, FilesFolder, + ShareLinkRequest, ShareLinkResponse, }; use super::client::{GraphClient, PaginationOpts}; @@ -317,6 +317,59 @@ pub async fn delete_file( .await } +/// ID of the signed-in user's OneDrive. +pub async fn my_drive_id_at(client: &GraphClient, base: &str) -> Result { + let drive: Drive = client + .get(&endpoints::me_drive_at(base), &[]) + .await + .map_err(delete_scope_hint)?; + drive.id.ok_or_else(|| TeamsError::ApiError { + status: 500, + message: "/me/drive missing id".to_string(), + }) +} + +/// Resolve a shared web URL (an attachment's `contentUrl`) to its drive item. +pub async fn shared_drive_item_at( + client: &GraphClient, + base: &str, + content_url: &str, +) -> Result { + let token = endpoints::sharing_url_token(content_url); + client + .get(&endpoints::shares_drive_item_at(base, &token), &[]) + .await +} + +/// Delete an item from a drive. A 404 is returned as `NotFound` unchanged so +/// the caller can treat an item that is already gone as deleted. +pub async fn delete_drive_item_at( + client: &GraphClient, + base: &str, + drive_id: &str, + item_id: &str, +) -> Result<()> { + client + .delete(&endpoints::drive_item_at(base, drive_id, item_id)) + .await + .map_err(|e| match e { + TeamsError::PermissionDenied(_) => delete_scope_hint(e), + other => other, + }) +} + +/// Graph masks a drive the token cannot see as 404, so `/me/drive` failing +/// with 403 or 404 usually means the Files.ReadWrite scope is missing. +fn delete_scope_hint(err: TeamsError) -> TeamsError { + let hint = "\nHint: deleting attachment uploads needs the Files.ReadWrite delegated \ + scope. Add it to your profile's scopes and run `teams auth refresh`."; + match err { + TeamsError::PermissionDenied(msg) => TeamsError::PermissionDenied(format!("{msg}{hint}")), + TeamsError::NotFound(msg) => TeamsError::NotFound(format!("{msg}{hint}")), + other => other, + } +} + /// Grant `recipients` read access to an item in the signed-in user's OneDrive, /// without sending them an email. Returns the permissions that now exist for /// them. Needs the same `Files.ReadWrite` scope as the upload itself. diff --git a/src/cli/message.rs b/src/cli/message.rs index 421723b..e73f3e3 100644 --- a/src/cli/message.rs +++ b/src/cli/message.rs @@ -104,7 +104,7 @@ pub enum MessageCommand { #[arg(long)] with_attachments: bool, }, - /// List or download message attachments and inline images + /// List, download or delete message attachments and inline images Attachments { #[command(subcommand)] command: super::message_attachments::AttachmentsCommand, @@ -353,6 +353,14 @@ pub async fn run( "Deleting a message needs explicit confirmation: add --yes".into(), )); } + if let MessageCommand::Attachments { + command: super::message_attachments::AttachmentsCommand::Delete { yes: false, .. }, + } = &cmd + { + return Err(TeamsError::InvalidInput( + "Deleting attachments needs explicit confirmation: add --yes".into(), + )); + } let token = auth::resolve_token(profile).await?; let client = GraphClient::new(token, &config.network)?; diff --git a/src/cli/message_attachments.rs b/src/cli/message_attachments.rs index 624dfde..3468ce5 100644 --- a/src/cli/message_attachments.rs +++ b/src/cli/message_attachments.rs @@ -4,7 +4,7 @@ use std::time::Instant; use clap::Subcommand; use crate::api::messages::MessageRef; -use crate::api::{self, GraphClient}; +use crate::api::{self, endpoints, GraphClient}; use crate::error::{Result, TeamsError}; use crate::models::attachment_inventory::{build_inventory, AttachmentItem, ItemKind}; use crate::output::{self, OutputFormat}; @@ -74,6 +74,41 @@ pub enum AttachmentsCommand { #[arg(long, requires = "index")] path: Option, }, + /// Delete files you attached from your OneDrive (the message itself is kept) + #[command( + override_usage = "teams message attachments delete (--team --channel [--reply ] | --chat ) (MESSAGE_ID | --message ) [--index ] --yes" + )] + Delete { + /// Team ID (for channel messages; requires --channel) + #[arg(long, requires = "channel", conflicts_with = "chat")] + team: Option, + /// Channel ID (for channel messages; requires --team) + #[arg(long, requires = "team", conflicts_with = "chat")] + channel: Option, + /// Chat ID (for chat messages) + #[arg(long, required_unless_present = "team")] + chat: Option, + /// Reply ID (for a reply within a channel thread) + #[arg(long, requires = "team", conflicts_with = "chat")] + reply: Option, + /// Message ID + #[arg(required_unless_present = "message", conflicts_with = "message")] + message_id: Option, + /// Message ID + #[arg( + long = "message", + alias = "message-id", + required_unless_present = "message_id", + conflicts_with = "message_id" + )] + message: Option, + /// Delete only the item with this index (from `attachments list`) + #[arg(long)] + index: Option, + /// Confirm the deletion; without it the command exits with code 2 and sends nothing + #[arg(long)] + yes: bool, + }, } pub async fn run( @@ -177,6 +212,169 @@ pub async fn run( output::print_success(format, &result, start); Ok(()) } + + AttachmentsCommand::Delete { + team, + channel, + chat, + reply, + message_id, + message, + index, + yes: _, + } => { + let start = Instant::now(); + let message_id = + super::message::resolve_id(message_id, message, "--message or ")?; + let message_ref = resolve_message_ref(team, channel, chat, reply, message_id.clone())?; + let items = fetch_inventory(client, &message_ref).await?; + let selected = select_for_delete(&items, index)?; + let results = delete_items_at(client, endpoints::GRAPH_V1, &selected).await?; + let result = serde_json::json!({ + "message_id": message_id, + "items": results, + }); + output::print_success(format, &result, start); + Ok(()) + } + } +} + +/// Like `select_items`, but every kind is selectable: items that are not +/// uploads are reported as skipped rather than refused. +fn select_for_delete( + items: &[AttachmentItem], + index: Option, +) -> Result> { + match index { + Some(i) => { + let item = items.iter().find(|it| it.index == i).ok_or_else(|| { + TeamsError::InvalidInput(format!( + "No attachment item with index {i}; the message has {} item(s). \ + Run `teams message attachments list` to see them.", + items.len() + )) + })?; + Ok(vec![item]) + } + None if items.is_empty() => Err(TeamsError::NotFound( + "Message has no attachments or inline images".into(), + )), + None => Ok(items.iter().collect()), + } +} + +/// Delete each selected file attachment that lives in the caller's own +/// OneDrive. Everything else is skipped with a reason. Stops at the first +/// Graph failure, noting what was already deleted. +async fn delete_items_at( + client: &GraphClient, + base: &str, + items: &[&AttachmentItem], +) -> Result> { + let mut my_drive: Option = None; + let mut results = Vec::new(); + for item in items { + match delete_item_at(client, base, item, &mut my_drive).await { + Ok(r) => results.push(r), + Err(e) => { + let deleted: Vec = results + .iter() + .filter(|r| r["deleted"] == true) + .map(|r| r["index"].to_string()) + .collect(); + if deleted.is_empty() { + return Err(e); + } + let note = format!( + " Deleted before the failure: item(s) {}.", + deleted.join(", ") + ); + // Rate-limit and network errors carry no message to extend. + tracing::warn!("{}", note.trim()); + return Err(match e { + TeamsError::ServerError { status, message } => TeamsError::ServerError { + status, + message: format!("{message}{note}"), + }, + TeamsError::AuthError(m) => TeamsError::AuthError(format!("{m}{note}")), + other => augment_error(other, ¬e), + }); + } + } + } + Ok(results) +} + +async fn delete_item_at( + client: &GraphClient, + base: &str, + item: &AttachmentItem, + my_drive: &mut Option, +) -> Result { + let outcome = |deleted: bool, reason: Option<&str>| { + let mut v = serde_json::json!({ + "index": item.index, + "name": item.name, + "deleted": deleted, + }); + if let Some(reason) = reason { + v["reason"] = reason.into(); + } + v + }; + + if item.kind != ItemKind::FileReference { + let reason = match item.kind { + ItemKind::HostedContent => "inline image, stored in the message, not an upload", + ItemKind::CodeSnippet => "code snippet, stored in the message, not an upload", + _ => "not a file attachment", + }; + return Ok(outcome(false, Some(reason))); + } + let Some(url) = item.content_url.as_deref() else { + return Ok(outcome(false, Some("file attachment has no contentUrl"))); + }; + + let drive_item = match api::files::shared_drive_item_at(client, base, url).await { + Ok(d) => d, + // Graph masks items the caller cannot see as 404, so a missing item + // cannot be told apart from someone else's file: never claim it deleted. + Err(TeamsError::NotFound(_)) | Err(TeamsError::PermissionDenied(_)) => { + return Ok(outcome( + false, + Some("not found or not accessible (already deleted, or not your file)"), + )); + } + Err(e) => return Err(e), + }; + + let my_drive_id = match my_drive { + Some(id) => id.as_str(), + None => my_drive.insert(api::files::my_drive_id_at(client, base).await?), + }; + let item_drive = drive_item + .parent_reference + .as_ref() + .and_then(|r| r.drive_id.as_deref()); + if item_drive != Some(my_drive_id) { + return Ok(outcome( + false, + Some("not in your OneDrive (shared by someone else, or a channel file)"), + )); + } + // A folder delete is recursive; attachments are files. + if drive_item.file.is_none() { + return Ok(outcome(false, Some("not a file"))); + } + let Some(item_id) = drive_item.id.as_deref() else { + return Ok(outcome(false, Some("drive item has no id"))); + }; + + match api::files::delete_drive_item_at(client, base, my_drive_id, item_id).await { + Ok(()) => Ok(outcome(true, None)), + Err(TeamsError::NotFound(_)) => Ok(outcome(true, Some("already deleted"))), + Err(e) => Err(e), } } @@ -604,6 +802,290 @@ mod tests { assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1); } + mod delete { + use super::*; + use crate::models::message::{ChatMessage, ChatMessageHostedContent}; + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + const OWN_URL: &str = "https://tenant-my.sharepoint.com/personal/me/Documents/Microsoft%20Teams%20Chat%20Files/report.pdf"; + const FOREIGN_URL: &str = + "https://tenant.sharepoint.com/sites/team/Shared%20Documents/General/plan.docx"; + + fn test_client() -> GraphClient { + GraphClient::new( + crate::auth::token::TokenInfo { + access_token: "synthetic-test-token".into(), + expires_at: None, + token_type: "Bearer".into(), + scope: None, + refresh_token: None, + profile: "test".into(), + }, + &crate::config::NetworkConfig { + timeout: 3, + max_retries: 0, + retry_backoff_base: 1, + }, + ) + .unwrap() + } + + /// Inventory of a message with one inline image (index 0) and file + /// attachments for each of `urls` (index 1..). + fn inventory(urls: &[&str]) -> Vec { + let attachments: Vec = urls + .iter() + .enumerate() + .map(|(i, url)| { + serde_json::json!({ + "id": format!("att{i}"), + "contentType": "reference", + "contentUrl": url, + "name": format!("file{i}"), + }) + }) + .collect(); + let msg: ChatMessage = serde_json::from_value(serde_json::json!({ + "id": "m1", + "body": { + "contentType": "html", + "content": "", + }, + "attachments": attachments, + })) + .unwrap(); + let hosted: Vec = + serde_json::from_value(serde_json::json!([{"id": "aW1n"}])).unwrap(); + build_inventory(&msg, &hosted) + } + + async fn mount_me_drive(server: &MockServer, times: u64) { + Mock::given(method("GET")) + .and(path("/me/drive")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "my-drive", + }))) + .expect(times) + .mount(server) + .await; + } + + async fn mount_share(server: &MockServer, url: &str, drive_id: &str, item_id: &str) { + mount_share_body( + server, + url, + serde_json::json!({ + "id": item_id, + "name": "x", + "file": {"mimeType": "application/pdf"}, + "parentReference": {"driveId": drive_id, "id": "parent"}, + }), + ) + .await; + } + + async fn mount_share_body(server: &MockServer, url: &str, body: serde_json::Value) { + Mock::given(method("GET")) + .and(path(format!( + "/shares/{}/driveItem", + endpoints::sharing_url_token(url) + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(body)) + .expect(1) + .mount(server) + .await; + } + + #[tokio::test] + async fn own_folder_is_never_deleted() { + let server = MockServer::start().await; + mount_me_drive(&server, 1).await; + mount_share_body( + &server, + OWN_URL, + serde_json::json!({ + "id": "folder-item", + "folder": {"childCount": 3}, + "parentReference": {"driveId": "my-drive", "id": "root"}, + }), + ) + .await; + mount_delete(&server, "folder-item", 204, 0).await; + + let items = inventory(&[OWN_URL]); + let selected = select_for_delete(&items, Some(1)).unwrap(); + let results = delete_items_at(&test_client(), &server.uri(), &selected) + .await + .unwrap(); + assert_eq!(results[0]["deleted"], false); + assert_eq!(results[0]["reason"], "not a file"); + } + + #[tokio::test] + async fn server_error_after_a_delete_keeps_the_note_and_exit_code() { + let server = MockServer::start().await; + let second = "https://tenant-my.sharepoint.com/personal/me/Documents/Microsoft%20Teams%20Chat%20Files/c.pdf"; + mount_me_drive(&server, 1).await; + mount_share(&server, OWN_URL, "my-drive", "own-item").await; + mount_share(&server, second, "my-drive", "item-c").await; + mount_delete(&server, "own-item", 204, 1).await; + mount_delete(&server, "item-c", 503, 1).await; + + let items = inventory(&[OWN_URL, second]); + let selected = select_for_delete(&items, None).unwrap(); + let err = delete_items_at(&test_client(), &server.uri(), &selected) + .await + .unwrap_err(); + assert_eq!(err.exit_code(), 8); + assert!(err.to_string().contains("item(s) 1."), "{err}"); + } + + async fn mount_delete(server: &MockServer, item_id: &str, status: u16, times: u64) { + Mock::given(method("DELETE")) + .and(path(format!("/drives/my-drive/items/{item_id}"))) + .respond_with(ResponseTemplate::new(status)) + .expect(times) + .mount(server) + .await; + } + + #[tokio::test] + async fn own_upload_is_deleted_and_others_are_skipped() { + let server = MockServer::start().await; + mount_me_drive(&server, 1).await; + mount_share(&server, OWN_URL, "my-drive", "own-item").await; + mount_share(&server, FOREIGN_URL, "team-drive", "team-item").await; + mount_delete(&server, "own-item", 204, 1).await; + Mock::given(method("DELETE")) + .and(path("/drives/team-drive/items/team-item")) + .respond_with(ResponseTemplate::new(204)) + .expect(0) + .mount(&server) + .await; + + let items = inventory(&[OWN_URL, FOREIGN_URL]); + let selected = select_for_delete(&items, None).unwrap(); + let results = delete_items_at(&test_client(), &server.uri(), &selected) + .await + .unwrap(); + + assert_eq!(results.len(), 3); + assert_eq!(results[0]["index"], 0); + assert_eq!(results[0]["deleted"], false); + assert!(results[0]["reason"] + .as_str() + .unwrap() + .contains("inline image")); + assert_eq!(results[1]["name"], "file0"); + assert_eq!(results[1]["deleted"], true); + assert!(results[1].get("reason").is_none()); + assert_eq!(results[2]["deleted"], false); + assert!(results[2]["reason"] + .as_str() + .unwrap() + .contains("not in your OneDrive")); + } + + #[tokio::test] + async fn delete_404_counts_as_already_deleted() { + let server = MockServer::start().await; + mount_me_drive(&server, 1).await; + mount_share(&server, OWN_URL, "my-drive", "own-item").await; + mount_delete(&server, "own-item", 404, 1).await; + + let items = inventory(&[OWN_URL]); + let selected = select_for_delete(&items, Some(1)).unwrap(); + let results = delete_items_at(&test_client(), &server.uri(), &selected) + .await + .unwrap(); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["deleted"], true); + assert_eq!(results[0]["reason"], "already deleted"); + } + + #[tokio::test] + async fn unresolvable_share_is_skipped_without_delete() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path(format!( + "/shares/{}/driveItem", + endpoints::sharing_url_token(OWN_URL) + ))) + .respond_with(ResponseTemplate::new(404)) + .expect(1) + .mount(&server) + .await; + mount_me_drive(&server, 0).await; + mount_delete(&server, "own-item", 204, 0).await; + + let items = inventory(&[OWN_URL]); + let selected = select_for_delete(&items, Some(1)).unwrap(); + let results = delete_items_at(&test_client(), &server.uri(), &selected) + .await + .unwrap(); + assert_eq!(results[0]["deleted"], false); + assert!(results[0]["reason"].as_str().unwrap().contains("not found")); + } + + #[tokio::test] + async fn delete_failure_is_an_error_noting_earlier_deletes() { + let server = MockServer::start().await; + let second = "https://tenant-my.sharepoint.com/personal/me/Documents/Microsoft%20Teams%20Chat%20Files/b.pdf"; + mount_me_drive(&server, 1).await; + mount_share(&server, OWN_URL, "my-drive", "own-item").await; + mount_share(&server, second, "my-drive", "item-b").await; + mount_delete(&server, "own-item", 204, 1).await; + Mock::given(method("DELETE")) + .and(path("/drives/my-drive/items/item-b")) + .respond_with(ResponseTemplate::new(403)) + .expect(1) + .mount(&server) + .await; + + let items = inventory(&[OWN_URL, second]); + let selected = select_for_delete(&items, None).unwrap(); + let err = delete_items_at(&test_client(), &server.uri(), &selected) + .await + .unwrap_err(); + assert_eq!(err.exit_code(), 4); + let msg = err.to_string(); + assert!(msg.contains("Files.ReadWrite"), "{msg}"); + assert!( + msg.contains("Deleted before the failure: item(s) 1."), + "{msg}" + ); + } + + #[tokio::test] + async fn inline_image_only_selection_makes_no_requests() { + let server = MockServer::start().await; + let items = inventory(&[OWN_URL]); + let selected = select_for_delete(&items, Some(0)).unwrap(); + let results = delete_items_at(&test_client(), &server.uri(), &selected) + .await + .unwrap(); + assert_eq!(results.len(), 1); + assert_eq!(results[0]["deleted"], false); + assert!(server.received_requests().await.unwrap().is_empty()); + } + + #[test] + fn index_selection_matches_list_indexes() { + let items = inventory(&[OWN_URL, FOREIGN_URL]); + let picked = select_for_delete(&items, Some(2)).unwrap(); + assert_eq!(picked.len(), 1); + assert_eq!(picked[0].index, 2); + assert_eq!(picked[0].content_url.as_deref(), Some(FOREIGN_URL)); + + let err = select_for_delete(&items, Some(9)).unwrap_err(); + assert_eq!(err.exit_code(), 2); + assert!(err.to_string().contains("3 item(s)")); + + assert_eq!(select_for_delete(&[], None).unwrap_err().exit_code(), 5); + } + } + #[test] fn code_snippet_id_extracted_from_graph_url_only() { assert_eq!( diff --git a/src/models/file.rs b/src/models/file.rs index 130a848..9b0fa2e 100644 --- a/src/models/file.rs +++ b/src/models/file.rs @@ -79,6 +79,12 @@ pub struct FilesFolder { pub parent_reference: Option, } +/// A drive (`GET /me/drive`); only its ID is needed. +#[derive(Debug, Clone, Deserialize)] +pub struct Drive { + pub id: Option, +} + /// Request to create a sharing link #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] diff --git a/tests/cli.rs b/tests/cli.rs index a204250..7677008 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -948,6 +948,100 @@ fn message_delete_rejects_incomplete_or_mixed_targets() { .stderr(predicate::str::contains("--team")); } +/// Like `message delete`, attachment deletion is refused without `--yes` +/// before any token is resolved, so the exit code is 2, not an auth error. +#[test] +fn message_attachments_delete_requires_yes() { + teams() + .args([ + "message", + "attachments", + "delete", + "--chat", + "19:abc@thread.v2", + "--message", + "1700000000000", + "--index", + "1", + "--output", + "json", + ]) + .assert() + .code(2) + .stdout( + predicate::str::contains("\"success\": false") + .and(predicate::str::contains("INVALID_INPUT")) + .and(predicate::str::contains("--yes")), + ); +} + +#[test] +fn message_attachments_delete_rejects_incomplete_or_mixed_targets() { + teams() + .args([ + "message", + "attachments", + "delete", + "--team", + "team-id", + "--message", + "1", + "--yes", + ]) + .assert() + .code(2) + .stderr(predicate::str::contains("--channel")); + + teams() + .args([ + "message", + "attachments", + "delete", + "--chat", + "19:abc@thread.v2", + "--team", + "team-id", + "--channel", + "channel-id", + "--message", + "1", + "--yes", + ]) + .assert() + .code(2) + .stderr(predicate::str::contains("cannot be used with")); + + teams() + .args([ + "message", + "attachments", + "delete", + "--chat", + "19:abc@thread.v2", + "--reply", + "2", + "--message", + "1", + "--yes", + ]) + .assert() + .code(2) + .stderr(predicate::str::contains("cannot be used with")); + + teams() + .args([ + "message", + "attachments", + "delete", + "--message", + "1", + "--yes", + ]) + .assert() + .code(2) + .stderr(predicate::str::contains("--chat")); +} + #[test] fn message_documented_flags_are_available() { teams() From f5794c63b6e1a7e84dbeeee1a68bf71df3c0063a Mon Sep 17 00:00:00 2001 From: Sion Smith Date: Thu, 15 Oct 2026 11:16:56 +0100 Subject: [PATCH 10/11] Test empty pages and opaque Graph continuation tokens --- src/api/client.rs | 50 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/api/client.rs b/src/api/client.rs index c4f35ff..e8fef7e 100644 --- a/src/api/client.rs +++ b/src/api/client.rs @@ -1122,4 +1122,54 @@ mod tests { ); assert!(page.next_link.is_none()); } + #[tokio::test] + async fn review_empty_page_with_opaque_next_link_is_not_terminal() { + let server = MockServer::start().await; + let next = format!( + "{}/me/chats?$skiptoken=opaque%2Btoken%2F%3D&$top=7", + server.uri() + ); + Mock::given(method("GET")) + .and(path("/me/chats")) + .and(query_param_is_missing("$skiptoken")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [], "@odata.nextLink": next + }))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/me/chats")) + .and(query_param("$skiptoken", "opaque+token/=")) + .and(query_param("$top", "7")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [{"id": "last"}] + }))) + .mount(&server) + .await; + let url = format!("{}/me/chats", server.uri()); + let single = test_client() + .get_paged::(&url, &[], &one_page()) + .await + .unwrap(); + assert!(single.items.is_empty()); + assert_eq!(single.next_link.as_deref(), Some(next.as_str())); + let all = test_client() + .get_paged::( + &url, + &[], + &PaginationOpts { + page_size: 1, + all_pages: true, + }, + ) + .await + .unwrap(); + assert_eq!(all.items, vec![serde_json::json!({"id":"last"})]); + assert!(all.next_link.is_none()); + let requests = server.received_requests().await.unwrap(); + assert_eq!( + requests.last().unwrap().url.query(), + Some("$skiptoken=opaque%2Btoken%2F%3D&$top=7") + ); + } } From fb7173e1425f934b5d75189e8b35c4a4e803f68c Mon Sep 17 00:00:00 2001 From: Sion Smith Date: Thu, 15 Oct 2026 11:17:03 +0100 Subject: [PATCH 11/11] Skip remote drive references when deleting attachment uploads --- CHANGELOG.md | 2 ++ CLAUDE.md | 2 +- src/cli/message_attachments.rs | 39 ++++++++++++++++++++++++++++++++++ src/cli/message_media.rs | 1 + src/models/file.rs | 3 +++ 5 files changed, 46 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9537b13..3bdf7df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Fixed +- Attachment deletion also rejects `remoteItem` references, including local shortcuts into another drive. + - `message attachments download` and `file download --path` write through a uniquely named, exclusively created temp file in the target directory, then rename it into place. Attachments used a fixed `.part`, so a symlink there redirected the write, a stray `.part` was clobbered, and concurrent downloads to one target shared a temp file; `file download` wrote in place, through any symlink at the target, and left a truncated file on failure; a symlink at the target is now replaced. A failed write now leaves no temp file. An existing target is still overwritten and keeps its mode, and `message attachments download --path -` still streams to stdout. ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 966f413..d4e8677 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,7 +24,7 @@ Single-crate Rust binary. Key modules: - `auth.rs` — login (PKCE, device code, client credentials), status, list, switch, logout, token - `team.rs` — list, get, create, update, delete, clone, archive, unarchive, members - `channel.rs` — list, get, create, update, delete, members - - `message.rs` — send, list, get, reply, update, delete, undelete, react, unreact, pin, unpin + - `message.rs` — send, list, get, reply, update, delete, undelete, react, unreact, pin, unpin; attachments list/download/delete (delete requires --yes and skips remote items) - `chat.rs` — list, get, create, hide, unhide, members - `presence.rs` — get, set, set-preferred, clear, clear-preferred, status, get-batch - `search.rs` — messages, users, teams diff --git a/src/cli/message_attachments.rs b/src/cli/message_attachments.rs index 3468ce5..cd3684b 100644 --- a/src/cli/message_attachments.rs +++ b/src/cli/message_attachments.rs @@ -349,6 +349,10 @@ async fn delete_item_at( Err(e) => return Err(e), }; + if drive_item.remote_item.is_some() { + return Ok(outcome(false, Some("shared reference to another drive"))); + } + let my_drive_id = match my_drive { Some(id) => id.as_str(), None => my_drive.insert(api::files::my_drive_id_at(client, base).await?), @@ -897,6 +901,41 @@ mod tests { .await; } + #[tokio::test] + async fn remote_reference_in_own_drive_is_never_deleted() { + let server = MockServer::start().await; + mount_share_body( + &server, + OWN_URL, + serde_json::json!({ + "id": "local-shortcut", + "file": {}, + "parentReference": {"driveId": "my-drive"}, + "remoteItem": { + "id": "foreign-item", + "file": {}, + "parentReference": {"driveId": "foreign-drive"} + } + }), + ) + .await; + mount_me_drive(&server, 0).await; + mount_delete(&server, "local-shortcut", 204, 0).await; + let items = inventory(&[OWN_URL]); + let selected = select_for_delete(&items, Some(1)).unwrap(); + let results = delete_items_at(&test_client(), &server.uri(), &selected) + .await + .unwrap(); + assert_eq!(results[0]["deleted"], false); + assert_eq!(results[0]["reason"], "shared reference to another drive"); + assert!(server + .received_requests() + .await + .unwrap() + .iter() + .all(|r| r.method.as_str() != "DELETE")); + } + #[tokio::test] async fn own_folder_is_never_deleted() { let server = MockServer::start().await; diff --git a/src/cli/message_media.rs b/src/cli/message_media.rs index 10b28b6..55260cc 100644 --- a/src/cli/message_media.rs +++ b/src/cli/message_media.rs @@ -766,6 +766,7 @@ mod tests { last_modified_by: None, file: None, folder: None, + remote_item: None, download_url: None, parent_reference: None, }; diff --git a/src/models/file.rs b/src/models/file.rs index 9b0fa2e..afb687a 100644 --- a/src/models/file.rs +++ b/src/models/file.rs @@ -28,6 +28,9 @@ pub struct DriveItem { pub file: Option, #[serde(skip_serializing_if = "Option::is_none")] pub folder: Option, + /// A reference to an item in another drive, never an owned upload. + #[serde(skip_serializing_if = "Option::is_none")] + pub remote_item: Option, #[serde( rename = "@microsoft.graph.downloadUrl", skip_serializing_if = "Option::is_none"