feat(auth): --org-uuid hint, mismatch handling, and richer callback page#480
Merged
platinummonkey merged 11 commits intoDataDog:mainfrom May 8, 2026
Merged
Conversation
…compat Carries the authoritative `dd_oid` returned by the OAuth issuer alongside the existing free-form `org` label. `#[serde(default)]` lets sessions.json files written before this field existed deserialize cleanly, and `skip_serializing_if = "Option::is_none"` keeps the on-disk shape byte-stable for sessions that were never tagged with a UUID. Adds `find_session(site, org)` so login can recall a previously-stored UUID without searching the registry by hand. Existing callers of `save_session` pass `None` for the new third arg; the upcoming login-path change will populate it from the callback.
Adds a `--org-uuid <UUID>` flag on `pup auth login` that emits the value as `dd_oid` in the OAuth authorize URL. The hint lets the authorize endpoint skip the org switcher when the existing browser session already matches, and pre-routes SAML/SSO routing for first-time logins. Resolution order for the effective UUID: 1. CLI flag `--org-uuid` if provided 2. Stored `SessionEntry.org_uuid` from the session registry 3. None (no hint) After token exchange, the callback's authoritative `dd_oid` is persisted on the session record so subsequent invocations re-emit it without the flag. The callback parser now also captures `dd_org_name` for use in the mismatch path (next commit). Empty UUID strings are coerced to "no hint" (mirrors the empty-subdomain behaviour) so an empty `--org-uuid ""` falls back rather than emitting a malformed `dd_oid=`.
When the user hints a UUID via --org-uuid (or via a stored session) but the OAuth server returns a different `dd_oid` (the user picked a different org in the switcher, or hit an SSO routing override), saving the resulting token under the user-supplied --org label would mislabel it. Refusing to save would also throw away the user's click since authorization codes are single-use. The chosen middle ground: warn on stderr and switch the save target to the actual org's name (`dd_org_name` from the callback, falling back to the first 8 chars of the UUID). The user-supplied --org entry is left alone, so a future re-auth against the requested org still gets a clean slot. The selection logic lives in a pure helper (`resolve_save_target`) so the unit tests can pin the four cases (no hint, match, mismatch with name, mismatch without name) without touching env state or storage.
The local listener's success page used to say only "Authentication Successful" — handy as a "you can close this tab" signal but offered no way to confirm the user consented for the org they intended. With the dd_oid hint flow, the consented org can diverge silently (logged-out login redirect drops the hint, SSO override, switcher pick), so showing the org name + UUID alongside the success message lets users catch the divergence at the moment of consent rather than later. Tests stub `success_page` directly with synthetic inputs (hostile strings included to verify HTML escaping). Real UUIDs that snuck into unit tests during the prior commits are swapped for placeholder values.
Iterating on the success page based on live preview feedback: - Lead line is now "Connected to Datadog (datadoghq.com)" — names the OAuth provider explicitly and links the site URL extracted from the callback's `site=` param. Avoids the dual "Authenticated"/"Authenticated to" repetition the prior copy had. - Org line reads "in org <Name>" instead of "as <Name>" — the org-as- context framing parses better than person-as-org. - UUID is now always-visible fine-print on its own line, wrapped in parens so it reads as supplementary annotation. Fine-print styling (small, gray, monospaced) demotes it visually but keeps it selectable and copy-pasteable. The earlier <details>/disclosure variant felt cluttered; hover-tooltip alternatives weren't copyable. - Defensive: the link href is only emitted when `site` is an https URL, and the displayed text uses the bare `domain` to avoid surfacing a full URL into the rendered text. Test fixtures use Star Wars placeholders (Mos Eisley Cantina, tatooine.example, Rebel Alliance, deathstar.tatooine.example) so no real Datadog org names or hosts leak into the codebase.
- Extract `org_suffix(Option<&str>) -> String` helper to replace the
`org.map(|o| format!(" (org: {o})")).unwrap_or_default()` boilerplate
that was duplicated across `login`, `logout`, `status`, `refresh`, and
the new mismatch path.
- Replace the single-field `SaveTarget { org }` struct with an enum
carrying both branches and pull the eprintln! out of the helper into
the caller, so `resolve_save_target` becomes genuinely pure (matching
the docstring) and the warning text stays testable separately if
needed.
- `save_session(&SessionEntry)` instead of three loose Option-shaped
args: a slip between `org` and `org_uuid` would have been silent
before; the struct field names make the call sites clearer.
- Move the empty-`--org-uuid`-string coercion from
`dcr::build_authorization_url` up to `main.rs` where the raw clap
value lives. Drops the `.filter(|u| !u.is_empty())` from the
URL-builder API and the now-redundant dcr test.
- Trim narration comments that restated what the code did or what was
changing in this PR. Kept genuine WHY comments (security defense,
OAuth-code-single-use rationale).
…smatch
Codex review flagged two real risks in the previous mismatch handling:
1. **Relabel could overwrite an unrelated existing session**. Saving
under just `dd_org_name` meant a fresh login that resolved to "Foo
HQ" would silently replace any existing `(site, "Foo HQ")` session
on that site, even if that session belonged to a different
underlying org UUID. Org display names are not stable identifiers,
so the relabel now always embeds the actual UUID's 8-char prefix:
`"Foo HQ [11111111]"`. Collisions become impossible.
2. **The user's pre-existing `--org foo` session was left untouched
silently**. The warning told users their token was being saved
somewhere else but didn't mention that the requested-label session
was preserved as-is and may serve stale tokens on future commands.
The warning now suggests `pup auth logout --org foo` so users can
actively clear the prior session if it's stale.
Also tightens `SaveTarget::Reassigned.label` from `Option<String>` to
`String` since the resolution path is now total, and drops a dead
`code{...}` CSS rule on the success page that wasn't matching any
rendered element.
…d_oid Two more issues caught in the iter-2 review pass: - **Default-org reauth was silently leaving the unnamed session stale** on dd_oid mismatch. `pup auth login` (no `--org`) is supposed to be idempotent — refresh the default session. Previously, a recalled stored UUID that no longer matched the issuer's response would route through `Reassigned`, save under a phantom labeled slot, and leave the unnamed session pointing at old credentials. Future commands without `--org` would still load the stale token. `resolve_save_target` now only relabels when `--org` was explicitly set; when it wasn't, the default session is overwritten in place with the new UUID, so the user gets the idempotent refresh they expected. - **Callback `dd_oid` is now shape-validated** before being persisted. In the stdin-paste fallback path the user pastes a URL whose query parameters could carry an arbitrary string. Anything that doesn't match the canonical `8-4-4-4-12` hex-with-dashes UUID shape is treated as missing, so a tampered URL can't seed garbage into `sessions.json` or rendered hints. Also filters empty `dd_org_name` out of the warning text so we don't render `OAuth returned <uuid> ()` with a dangling empty paren.
Codex flagged that on a successful login where the issuer omitted (or sent a malformed) `dd_oid`, the previous code carried the already-recalled UUID forward into the saved session. That meant a user could authenticate into a different org but `sessions.json` would still be tagged with the old org UUID, and subsequent logins would re-emit that stale hint. The fallback chain is now: callback-validated `dd_oid` → user's explicit `--org-uuid` flag → None. The recalled-from-storage value is only used to construct the in-flight authorize URL; it is no longer written back unless the callback confirms it.
srosenthal-dd
changed the title
platinummonkey
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
--org-uuid <UUID>topup auth login, sent asdd_oidin the OAuth authorize URL. Lets the authorize endpoint skip the org switcher when the existing browser session matches and pre-route SAML/SSO routing for first-time logins. The UUID is persisted on the session record so subsequent logins re-emit it automatically.Behavior
Changes
SessionEntrygains an optionalorg_uuidfield with#[serde(default)]+skip_serializing_if, keeping existingsessions.jsonbyte-stable.build_authorization_urlappendsdd_oidwhen the caller supplies it (empty-string coercion happens up at the CLI layer inmain.rs).CallbackResultparsesdd_oidanddd_org_namefrom both the HTTP listener and the stdin-paste path.commands::auth::loginresolves the effective UUID as CLI flag → stored hint → None, builds the URL, then on callback compares hinted vs returneddd_oid.--orgset, relabels the saved token as"{dd_org_name} [{uuid_prefix}]"(always embeds a UUID prefix so the relabel can't collide with an unrelated existing session of the same display name) and warns on stderr with apup auth logout --org Xcleanup hint. With no--org, default-session reauth stays idempotent and updates in place.dd_oidis shape-validated as8-4-4-4-12hex before being persisted, so a tampered stdin-paste URL can't seed garbage intosessions.json.org_uuidfalls back only to the user's explicit--org-uuidflag if the callback didn't echo one — the recalled-from-storage value is used to build the in-flight URL but never re-persisted unconfirmed.Test plan
cargo test --bin pup— 86 auth-related tests pass, including 7 cases forresolve_save_targetand 4 forsuccess_pagerendering / HTML escaping. Pre-existing parallel-run flakes in unrelated modules are unaffected.cargo clippy --bin pup --all-targets -- -D warningscleancargo fmt --checkcleandd_oidin authorize URL, saved cleanly under requested labelDatadog HQ [8dee7c38]+ cleanup hint + exit 0Out of scope
dd_oidmatch — rejected; would clobber unrelated browser tabs since sessions are one-per-domain. Also a separate web-ui change.Jira: https://datadoghq.atlassian.net/browse/DAL-670