Skip to content

fix(oauth): send client_id on token revocation - #249

Open
big-kahuna-burger wants to merge 7 commits into
mainfrom
oauth-revocation-includes-client-id
Open

fix(oauth): send client_id on token revocation#249
big-kahuna-burger wants to merge 7 commits into
mainfrom
oauth-revocation-includes-client-id

Conversation

@big-kahuna-burger

@big-kahuna-burger big-kahuna-burger commented Aug 14, 2026

Copy link
Copy Markdown

Summary

Send client_id on OAuth token revocation by going through OAuthClient.RevokeToken instead of a raw form POST.

Depends on dash0hq/dash0-api-client-go#31, which adds ClientId to OAuthRevocationRequest. Until that ships, go.mod has a local redirect:

replace github.com/dash0hq/dash0-api-client-go => ../dash0-api-client-go

CI cannot resolve that path and will fail. Drop the replace and bump the client version once PR 31 is released on upstream.

Why

The authorization server now requires client_id on POST /oauth/revoke (RFC 7009 §2.1). Without it, logout, re-login, and config profiles update --oauth=false left the refresh token valid server-side.

Also in this PR

  • oauth.RevokeRequest so API URL / client ID / refresh token cannot be swapped at call sites
  • httptest handlers no longer FailNow on the server goroutine
  • SamplingModeAdaptive / SamplingModeDisabled (needed to compile against the client PR, which is based on current client main)

Tests

  • Revoke request body includes client_id
  • --oauth=false revoke path
  • Re-login revokes the old token with the old client's ID, not the new one
  • Session-expired error message, interactive + agent mode

@big-kahuna-burger
big-kahuna-burger requested a review from a team as a code owner August 14, 2026 17:55

@mmanciop mmanciop left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at least parts of this change needs to go into dash0-api-client-go so that the Terraform provider can also benefit from it

@big-kahuna-burger

Copy link
Copy Markdown
Author

I think at least parts of this change needs to go into dash0-api-client-go so that the Terraform provider can also benefit from it

Agree, let me handle that

Comment thread internal/oauth/revoke.go Outdated
Comment thread internal/oauth/revoke.go Outdated
Comment thread internal/oauth/revoke.go Outdated
Comment thread internal/oauth/revoke.go Outdated
Comment thread internal/config/config_cmd_test.go Outdated
Comment thread internal/login/integration_test.go Outdated
@big-kahuna-burger

Copy link
Copy Markdown
Author

Thanks @mmanciop for guidance on how to land this and @nicolastakashi for detailed review. Addressed everything as far as I can see.

Both:

go test -v ./internal/oauth/
go test -v ./internal/config/ -run 'OAuthFalse|Revoke'
go test -v -tags=integration ./internal/login/ -run 'Revoke|Logout'

and

go test ./...
go test -tags=integration ./...
make lint-go
make build

Exit with 0 - logs not shown here for brevity, but you can simply checkout and run next to a local version of api client from the upstream PR: dash0hq/dash0-api-client-go#31

@big-kahuna-burger big-kahuna-burger self-assigned this Aug 17, 2026
@mmanciop
mmanciop marked this pull request as draft August 18, 2026 08:42
@mmanciop

Copy link
Copy Markdown
Member

Converting to draft until dash0hq/dash0-api-client-go#31 ships

@big-kahuna-burger
big-kahuna-burger force-pushed the oauth-revocation-includes-client-id branch from 6e17928 to f42c684 Compare August 31, 2026 14:42
@big-kahuna-burger
big-kahuna-burger marked this pull request as ready for review August 31, 2026 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants