From 5dff521ba8bb0cd79ab48d59832785a4cd0de557 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Thu, 18 Jun 2026 07:55:08 -0700 Subject: [PATCH 1/4] docs: remove stale multi-session guidance PR #596 enforced a single-slot invariant for unnamed sessions -- `prune_other_default_sessions` ensures a bare `pup auth login` always overwrites the existing default session. Two doc sites hadn't caught up: - docs/OAUTH2.md logout section: remove the DD_SITE logout example and the note that `pup auth logout` doesn't accept `--site` - docs/OAUTH2.md site selection: drop the "multiple unnamed sessions on different sites, set DD_SITE to pick one" sentence - README.md site selection: same sentence removed --- README.md | 2 +- docs/OAUTH2.md | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index e041cd2..337a7d9 100644 --- a/README.md +++ b/README.md @@ -285,7 +285,7 @@ Note: `pup auth logout` (default session) also deletes the shared DCR client cre `pup auth login` and `pup auth status` additionally accept `--site`, which wins over the above for those two commands. -If multiple sessions share the same org name on different sites, step 2 is skipped (ambiguous) and pup warns to stderr; pass `DD_SITE` to disambiguate. An unnamed (default) session can't be selected by `--org` at all -- if you have multiple unnamed sessions on different sites, set `DD_SITE` to pick one. +If multiple sessions share the same org name on different sites, step 2 is skipped (ambiguous) and pup warns to stderr; pass `DD_SITE` to disambiguate. An unnamed (default) session can't be selected by `--org` at all -- it has no name to look up. **Token Storage**: By default, OAuth tokens and DCR client credentials are stored in your platform's secure store: macOS Keychain (via Apple's Security framework, with Touch ID prompts), Linux Secret Service (via the `keyring` crate), or Windows Credential Manager (via the `keyring` crate; sharded across multiple WinCred entries to stay within WinCred's per-record size limit). When no secure store is available, pup falls back to JSON files under `~/.config/pup/` with `0600` permissions; in file mode tokens and client credentials are kept in separate files (`tokens_.json`, `client_.json`). Set `DD_TOKEN_STORAGE=file` to force file storage. In either mode, all tokens for a given site share one tokens entry, keyed internally by org name. diff --git a/docs/OAUTH2.md b/docs/OAUTH2.md index 6243f3e..df50cc6 100644 --- a/docs/OAUTH2.md +++ b/docs/OAUTH2.md @@ -66,14 +66,10 @@ Manually refresh your access token using the refresh token. This happens automat ### 4. Logout ```bash -pup auth logout # default session for the current site -DD_SITE=datadoghq.eu pup auth logout # default session for a non-default site +pup auth logout # default session pup auth logout --org staging-child # one named session, leaves others intact ``` -`pup auth logout` itself doesn't accept a `--site` flag; use `DD_SITE` to -pick which default session to clear. - **Side effect on sibling sessions:** logging out the default (unnamed) session for a site also deletes that site's shared DCR client credentials. Any named-org sessions on the same site will still hold @@ -415,8 +411,7 @@ accepts `--site`. If multiple sessions share the same org name on different sites, step 2 is skipped (ambiguous) and pup warns to stderr; pass `DD_SITE` to disambiguate. An unnamed (default) session can't be selected by `--org` -at all (it has no name to look up), so if you have multiple unnamed -sessions on different sites, set `DD_SITE` to pick one. +at all -- it has no name to look up. ### Session registry From 1715cf642bdd15d4bce8e10af522b524a3a27194 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Thu, 18 Jun 2026 08:08:45 -0700 Subject: [PATCH 2/4] feat(auth): enforce one session per org name Previously the session registry key was (site, org), so the same org name could appear on multiple sites as separate entries. This caused ambiguity in find_session_site when a user logged in as the same name on datadoghq.com and datadoghq.eu. The unnamed (default) session already had a single-slot invariant enforced by prune_other_default_sessions. This extends the same invariant to named sessions: save_session now deduplicates on org alone, so logging in as --org foo on a new site silently replaces the prior entry rather than accumulating a second one. find_session now looks up by org alone (no site parameter), and find_session_site is simplified to a single .find() since the registry is guaranteed to hold at most one entry per name. Legacy sessions.json files with duplicate named-org rows self-heal on the next pup auth login for that org name. --- README.md | 4 +- src/auth/storage.rs | 135 ++++++++++++++++++------------------------- src/commands/auth.rs | 4 +- 3 files changed, 61 insertions(+), 82 deletions(-) diff --git a/README.md b/README.md index 337a7d9..f67ffe6 100644 --- a/README.md +++ b/README.md @@ -280,12 +280,12 @@ Note: `pup auth logout` (default session) also deletes the shared DCR client cre **Site selection rules** (when pup resolves a site for a non-auth command): 1. `DD_SITE` env var (or `site:` in `~/.config/pup/config.yaml`), if set. -2. The site recorded in `~/.config/pup/sessions.json` for the named `--org` / `DD_ORG`, when the lookup is unambiguous. +2. The site recorded in `~/.config/pup/sessions.json` for the named `--org` / `DD_ORG`. 3. Default: `datadoghq.com`. `pup auth login` and `pup auth status` additionally accept `--site`, which wins over the above for those two commands. -If multiple sessions share the same org name on different sites, step 2 is skipped (ambiguous) and pup warns to stderr; pass `DD_SITE` to disambiguate. An unnamed (default) session can't be selected by `--org` at all -- it has no name to look up. +Each org name maps to exactly one session, so step 2 is always unambiguous. An unnamed (default) session can't be selected by `--org` at all -- it has no name to look up. **Token Storage**: By default, OAuth tokens and DCR client credentials are stored in your platform's secure store: macOS Keychain (via Apple's Security framework, with Touch ID prompts), Linux Secret Service (via the `keyring` crate), or Windows Credential Manager (via the `keyring` crate; sharded across multiple WinCred entries to stay within WinCred's per-record size limit). When no secure store is available, pup falls back to JSON files under `~/.config/pup/` with `0600` permissions; in file mode tokens and client credentials are kept in separate files (`tokens_.json`, `client_.json`). Set `DD_TOKEN_STORAGE=file` to force file storage. In either mode, all tokens for a given site share one tokens entry, keyed internally by org name. diff --git a/src/auth/storage.rs b/src/auth/storage.rs index 4d95d4d..418804e 100644 --- a/src/auth/storage.rs +++ b/src/auth/storage.rs @@ -883,64 +883,47 @@ pub fn list_sessions() -> Result> { } } -/// Upsert a session entry into the registry. Dedups on `(site, org)`; the -/// new entry's `org_uuid` wins so re-auth refreshes the stored UUID. +/// Upsert a session entry into the registry. Dedups on `org` alone; the +/// new entry's site and `org_uuid` win so re-auth to a different site +/// replaces the existing entry rather than accumulating a second one. #[cfg(not(target_arch = "wasm32"))] pub fn save_session(entry: &SessionEntry) -> Result<()> { let mut sessions = list_sessions()?; - sessions.retain(|s| !(s.site == entry.site && s.org == entry.org)); + sessions.retain(|s| s.org != entry.org); sessions.push(entry.clone()); write_sessions(&sessions) } /// Remove a session entry from the registry. #[cfg(not(target_arch = "wasm32"))] -pub fn remove_session(site: &str, org: Option<&str>) -> Result<()> { +pub fn remove_session(_site: &str, org: Option<&str>) -> Result<()> { let mut sessions = list_sessions()?; - sessions.retain(|s| !(s.site == site && s.org.as_deref() == org)); + sessions.retain(|s| s.org.as_deref() != org); write_sessions(&sessions) } -/// Look up a single session entry by `(site, org)`. +/// Look up a single session entry by org name. #[cfg(not(target_arch = "wasm32"))] -pub fn find_session(site: &str, org: Option<&str>) -> Option { +pub fn find_session(org: Option<&str>) -> Option { list_sessions() .ok()? .into_iter() - .find(|s| s.site == site && s.org.as_deref() == org) + .find(|s| s.org.as_deref() == org) } /// Look up the site for a named org session. Returns None if no session exists -/// for that org, or if multiple sessions share the same org name on different -/// sites (ambiguous — caller must pass DD_SITE explicitly). On the ambiguous -/// path, prints a one-line warning to stderr naming the conflicting sites so -/// the user knows the auto-resolution gave up. +/// for that org. The save_session invariant ensures at most one session per org +/// name, so the lookup is always unambiguous for current data. Legacy sessions.json +/// files written by older pup versions could contain two rows for the same named +/// org on different sites; in that case we return the first match and self-heal +/// on the next `pup auth login --org `. #[cfg(not(target_arch = "wasm32"))] pub fn find_session_site(org: &str) -> Option { - let sessions = list_sessions().ok()?; - let mut sites: Vec = sessions + list_sessions() + .ok()? .into_iter() - .filter(|s| s.org.as_deref() == Some(org)) + .find(|s| s.org.as_deref() == Some(org)) .map(|s| s.site) - .collect(); - sites.sort(); - sites.dedup(); - match sites.len() { - 0 => None, - 1 => sites.pop(), - _ => { - // The caller (Config::from_env / apply_org_override) handles the - // resulting None by leaving cfg.site at whatever it was — which - // may be a default, an env-set site, or a previously-resolved - // org's site — so we do not promise "falling back to default" here. - eprintln!( - "Warning: org '{org}' has saved sessions on multiple sites ({}); \ - not auto-selecting one. Set DD_SITE to disambiguate.", - sites.join(", ") - ); - None - } - } } /// Return the site for the single no-org ("default") session, if there is @@ -982,6 +965,13 @@ pub fn find_default_session_site() -> Option { /// so that switching regions replaces the previous unnamed default rather /// than accumulating ambiguous entries that confuse `find_default_session_site`. /// +/// Note: `save_session(None)` now also removes all prior no-org rows as part +/// of the single-slot invariant, so in the normal bare-login flow `to_prune` +/// will be empty when this runs. This function remains as a defensive guard +/// for legacy on-disk data (sessions.json written by older pup versions) +/// that may have multiple no-org rows for different sites, and to clean up +/// any orphaned tokens from the displaced session. +/// /// Named-org sessions on any site are never touched. #[cfg(not(target_arch = "wasm32"))] pub fn prune_other_default_sessions(keep_site: &str) -> Result<()> { @@ -1507,29 +1497,33 @@ mod tests { } #[test] - fn test_find_session_site_ambiguous_returns_none() { + fn test_save_session_same_name_different_site_overwrites() { let _lock = crate::test_utils::ENV_LOCK.blocking_lock(); - let tmp = TempDir::new("find_sess_amb"); + let tmp = TempDir::new("find_sess_overwrite"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - // Same org name registered against two different sites → caller must - // disambiguate via DD_SITE rather than us picking one. + // First login: org "myorg" on .com. save_session(&SessionEntry { site: "datadoghq.com".into(), - org: Some("shared-name".into()), + org: Some("myorg".into()), org_uuid: None, }) .unwrap(); + // Re-login: same org name but different site → overwrites the first. save_session(&SessionEntry { site: "datadoghq.eu".into(), - org: Some("shared-name".into()), + org: Some("myorg".into()), org_uuid: None, }) .unwrap(); - let result = find_session_site("shared-name"); + + let sessions = list_sessions().unwrap(); + let result = find_session_site("myorg"); std::env::remove_var("PUP_CONFIG_DIR"); - assert!(result.is_none()); + // Only one session remains, on the new site. + assert_eq!(sessions.len(), 1); + assert_eq!(result.as_deref(), Some("datadoghq.eu")); } #[test] @@ -1588,21 +1582,16 @@ mod tests { #[test] fn test_find_default_session_site_multiple_no_org_rows_returns_none() { - // Legacy data: two no-org rows → ambiguous; warn + return None. + // Legacy on-disk data: two no-org rows written by an older pup version → + // ambiguous; warn + return None. Construct directly via write_sessions + // because save_session now enforces the single-slot invariant. let _lock = crate::test_utils::ENV_LOCK.blocking_lock(); let tmp = TempDir::new("fds_multi"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session(&SessionEntry { - site: "datadoghq.com".into(), - org: None, - org_uuid: None, - }) - .unwrap(); - save_session(&SessionEntry { - site: "datadoghq.eu".into(), - org: None, - org_uuid: None, - }) + write_sessions(&[ + SessionEntry { site: "datadoghq.com".into(), org: None, org_uuid: None }, + SessionEntry { site: "datadoghq.eu".into(), org: None, org_uuid: None }, + ]) .unwrap(); let result = find_default_session_site(); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1726,25 +1715,20 @@ mod tests { #[test] fn test_prune_self_heals_legacy_multiple_no_org_rows() { - // The central migration promise: legacy data with two no-org rows is - // ambiguous (find_default_session_site warns + returns None), but a bare + // The central migration promise: legacy on-disk data with two no-org rows + // is ambiguous (find_default_session_site warns + returns None), but a bare // login's prune collapses it to one, after which resolution recovers. + // Construct directly via write_sessions because save_session now enforces + // the single-slot invariant. let _lock = crate::test_utils::ENV_LOCK.blocking_lock(); let tmp = TempDir::new("prune_self_heal"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); std::env::set_var("DD_TOKEN_STORAGE", "file"); - save_session(&SessionEntry { - site: "datadoghq.com".into(), - org: None, - org_uuid: None, - }) - .unwrap(); - save_session(&SessionEntry { - site: "datadoghq.eu".into(), - org: None, - org_uuid: None, - }) + write_sessions(&[ + SessionEntry { site: "datadoghq.com".into(), org: None, org_uuid: None }, + SessionEntry { site: "datadoghq.eu".into(), org: None, org_uuid: None }, + ]) .unwrap(); // Ambiguous before: two no-org rows → None. @@ -1764,6 +1748,8 @@ mod tests { fn test_prune_deletes_only_other_no_org_token() { // Token hygiene + safety: prune deletes the displaced site's no-org token // but leaves the kept site's no-org token and any named-org token intact. + // Construct two no-org rows directly via write_sessions because save_session + // now enforces the single-slot invariant (this is the legacy-data path). let _lock = crate::test_utils::ENV_LOCK.blocking_lock(); let tmp = TempDir::new("prune_tokens"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); @@ -1783,17 +1769,10 @@ mod tests { .save_tokens("datadoghq.eu", None, &make_token("eu-default")) .unwrap(); } - save_session(&SessionEntry { - site: "datadoghq.com".into(), - org: None, - org_uuid: None, - }) - .unwrap(); - save_session(&SessionEntry { - site: "datadoghq.eu".into(), - org: None, - org_uuid: None, - }) + write_sessions(&[ + SessionEntry { site: "datadoghq.com".into(), org: None, org_uuid: None }, + SessionEntry { site: "datadoghq.eu".into(), org: None, org_uuid: None }, + ]) .unwrap(); prune_other_default_sessions("datadoghq.eu").unwrap(); diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 0b9cb3d..4ad9e3b 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -44,9 +44,9 @@ pub async fn login( let org = cfg.org.as_deref(); // Resolve effective org_uuid: CLI flag wins; otherwise recall the UUID - // stored on the matching `(site, org)` session so re-auth keeps emitting + // stored on the matching org session so re-auth keeps emitting // `dd_oid` without re-passing the flag. - let stored_session = storage::find_session(site, org); + let stored_session = storage::find_session(org); let effective_org_uuid: Option = org_uuid .map(String::from) .or_else(|| stored_session.as_ref().and_then(|s| s.org_uuid.clone())); From 406b9adb85c37b42750e9f639a41a15a6d05d4d0 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Thu, 18 Jun 2026 08:38:06 -0700 Subject: [PATCH 3/4] fix: cargo fmt --- src/auth/storage.rs | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src/auth/storage.rs b/src/auth/storage.rs index 418804e..26ed0d6 100644 --- a/src/auth/storage.rs +++ b/src/auth/storage.rs @@ -1589,8 +1589,16 @@ mod tests { let tmp = TempDir::new("fds_multi"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); write_sessions(&[ - SessionEntry { site: "datadoghq.com".into(), org: None, org_uuid: None }, - SessionEntry { site: "datadoghq.eu".into(), org: None, org_uuid: None }, + SessionEntry { + site: "datadoghq.com".into(), + org: None, + org_uuid: None, + }, + SessionEntry { + site: "datadoghq.eu".into(), + org: None, + org_uuid: None, + }, ]) .unwrap(); let result = find_default_session_site(); @@ -1726,8 +1734,16 @@ mod tests { std::env::set_var("DD_TOKEN_STORAGE", "file"); write_sessions(&[ - SessionEntry { site: "datadoghq.com".into(), org: None, org_uuid: None }, - SessionEntry { site: "datadoghq.eu".into(), org: None, org_uuid: None }, + SessionEntry { + site: "datadoghq.com".into(), + org: None, + org_uuid: None, + }, + SessionEntry { + site: "datadoghq.eu".into(), + org: None, + org_uuid: None, + }, ]) .unwrap(); @@ -1770,8 +1786,16 @@ mod tests { .unwrap(); } write_sessions(&[ - SessionEntry { site: "datadoghq.com".into(), org: None, org_uuid: None }, - SessionEntry { site: "datadoghq.eu".into(), org: None, org_uuid: None }, + SessionEntry { + site: "datadoghq.com".into(), + org: None, + org_uuid: None, + }, + SessionEntry { + site: "datadoghq.eu".into(), + org: None, + org_uuid: None, + }, ]) .unwrap(); From c459df6d6a65a7f54c5cf111f4d621e30c37daff Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Thu, 18 Jun 2026 08:54:41 -0700 Subject: [PATCH 4/4] fix(auth): delete tokens for all matching sessions on logout --- src/commands/auth.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 4ad9e3b..915e31e 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -366,18 +366,32 @@ pub async fn login( #[cfg(not(target_arch = "wasm32"))] pub async fn logout(cfg: &Config) -> Result<()> { - let site = &cfg.site; let org = cfg.org.as_deref(); + // Collect all sessions matching this org name before removing them. Under + // the one-per-name invariant there is normally only one, but legacy + // sessions.json files may have two rows for the same org on different sites. + // Delete tokens for each so none are orphaned in keychain/file storage. + let sessions_to_remove: Vec<_> = storage::list_sessions() + .unwrap_or_default() + .into_iter() + .filter(|s| s.org.as_deref() == org) + .collect(); with_storage(|store| { - store.delete_tokens(site, org)?; - // Only delete client credentials when logging out the default (no-org) session; - // client credentials are site-scoped and shared across orgs - if org.is_none() { - store.delete_client_credentials(site)?; + for s in &sessions_to_remove { + store.delete_tokens(&s.site, org)?; + // Client credentials are site-scoped and shared across orgs; only + // delete them when logging out the default (no-org) session. + if org.is_none() { + store.delete_client_credentials(&s.site)?; + } } Ok(()) })?; - storage::remove_session(site, org)?; + storage::remove_session(&cfg.site, org)?; + let site = sessions_to_remove + .first() + .map(|s| s.site.as_str()) + .unwrap_or(&cfg.site); let org_label = org_suffix(org); eprintln!("Logged out from {site}{org_label}. Tokens removed."); Ok(())