diff --git a/README.md b/README.md index e041cd2..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 -- if you have multiple unnamed sessions on different sites, set `DD_SITE` to pick one. +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/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 diff --git a/src/auth/storage.rs b/src/auth/storage.rs index 4d95d4d..26ed0d6 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,24 @@ 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 +1723,28 @@ 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 +1764,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 +1785,18 @@ 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..915e31e 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())); @@ -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(())