feat: OAuth support for all Logs commands#639
Conversation
Adds logs_write_pipelines to default_scopes() (not read_only_scopes(), it's a write scope). Needed for pipeline create/update/delete once the server-side route accepts OAuth (dogweb#170410). Also fixes commands::logs_restriction (the pup logs-restriction CRUD subcommand): it used make_api_no_auth!, which explicitly skips the bearer token and always falls back to API+App key regardless of server support. Swapped to make_api! so it actually uses OAuth like the rest of the logs commands. Addresses DataDog#608.
srosenthal-dd
changed the title
|
Validated e2e against sites: staging, US5, and EU. US1 support should land within the next ~hour as the server-side deployment completes. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f1005d20f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex flagged that logs-restriction create/update/delete/roles-add need user_access_manage, which is high-privilege enough that it shouldn't be in default_scopes() for every login. Rather than hardcode it into a client-side allowlist, resolve_login_scopes() now passes explicit --scopes/--extra-scopes/config scopes through unvalidated -- the OAuth server is the source of truth for which scopes this client can request, and already rejects the rest with invalid_scope. This means any scope enabled server-side works immediately via an explicit opt-in, with no pup release needed to add it to a list. Also adds --extra-scopes, which adds to the normal scope set (defaults, config, or read-only scopes) instead of replacing it like --scopes does, so opting into one extra scope doesn't require retyping every other scope you still want. Mutually exclusive with --scopes. E2E verified on staging: `pup auth login --extra-scopes user_access_manage` grants it alongside normal defaults, and `pup logs-restriction create/get/delete` round-trips successfully against a real (cleaned-up) restriction query.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba1c955b21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex caught that the AUTHENTICATION example still used --scopes with an ellipsis, which the previous commit made a full replacement of the scope set passed through unvalidated -- copying it literally would send "..." to the OAuth server and fail with invalid_scope. Point at --extra-scopes instead, which is what this opt-in is actually for.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Addresses #608.
Summary
logs_write_pipelinestodefault_scopes()(write scope, not added toread_only_scopes()). Needed for pipeline create/update/delete once the server-side route accepts OAuth (recently updated).commands::logs_restriction(thepup logs-restrictionCRUD subcommand: list/get/create/update/delete/roles): it usedmake_api_no_auth!, which explicitly skips the bearer token and always falls back to API+App key regardless of server support. Swapped tomake_api!so it uses OAuth like the rest of the logs commands.Note:
pup logs restriction-queries list/get(the separate, older dedicated subcommand) and genericpup apipassthrough already send the OAuth bearer correctly for these paths — this PR only fixes thelogs-restrictionCRUD surface.Test plan
cargo build,cargo test --bin pup auth::types,cargo test --bin pup logs_restriction,cargo clippy --all-targets -- -D warnings,cargo fmt --checkall cleanpup logs-restriction list/get/create/update/deleteandpup api v1/logs/config/pipelines