fix(oauth): send client_id on token revocation

.chloggen/oauth-revocation-includes-client-id.yaml

@@ -0,0 +1,29 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern (e.g. dashboards, config, apply)
+component: login
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: "Send `client_id` on OAuth token revocation requests"
+
+# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
+issues: [249]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: |
+  `dash0 logout`, `dash0 login` (on re-login), and `dash0 config profiles update --oauth=false` now
+  include the profile's client_id when revoking a refresh token, matching the authorization server's
+  requirement. Without it, revocation silently failed and the old refresh token stayed valid.
+
+# If your change doesn't affect end users or the exported elements of any package,
+# you should instead start your pull request title with "chore" or use the "Skip Changelog" label.
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Default: '[user]'
+change_logs: []

internal/client/client_test.go

@@ -2,6 +2,7 @@ package client
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"os"
 	"strings"
@@ -619,3 +620,70 @@ func TestCheckOAuthOnOtlp_AgentModeMessage(t *testing.T) {
 		t.Errorf("agent-mode error must surface the profile conversion path: %q", err.Error())
 	}
 }
+
+// revokedRefreshTokenErrorBody is the exact response body observed from the
+// token endpoint when exchanging a refresh token that has already been
+// revoked (e.g. by a prior `dash0 logout`).
+const revokedRefreshTokenErrorBody = `{"error":"invalid_grant","error_description":"The refresh token has already been used. The entire token family has been revoked."}`
+
+// revokedRefreshTokenError decodes revokedRefreshTokenErrorBody the same way
+// the SDK's token-exchange path would, and wraps it the same way
+// NewClientFromContext's OAuth-refresh failure does, so the test starts
+// from the literal bytes the authorization server sends rather than a
+// hand-built struct.
+func revokedRefreshTokenError(t *testing.T) error {
+	t.Helper()
+	var parsed dash0api.OAuthTokenErrorResponse
+	require.NoError(t, json.Unmarshal([]byte(revokedRefreshTokenErrorBody), &parsed))
+	description := ""
+	if parsed.ErrorDescription != nil {
+		description = *parsed.ErrorDescription
+	}
+	oauthErr := &dash0api.OAuthTokenError{
+		StatusCode:  400,
+		Code:        parsed.Error,
+		Description: description,
+	}
+	return fmt.Errorf("failed to refresh OAuth access token: %w", oauthErr)
+}
+
+// TestTranslateConfigError_RevokedRefreshToken asserts that when the
+// authorization server rejects a refresh token as revoked, the CLI
+// surfaces the friendly re-login hint instead of leaking the wrapped SDK
+// error.
+func TestTranslateConfigError_RevokedRefreshToken(t *testing.T) {
+	err := translateConfigError(context.Background(), revokedRefreshTokenError(t))
+	if err == nil {
+		t.Fatalf("expected a translated error, got nil")
+	}
+	if !strings.Contains(err.Error(), "your Dash0 session has expired or was revoked") {
+		t.Errorf("error does not name the root cause: %q", err.Error())
+	}
+	if !strings.Contains(err.Error(), "dash0 login") {
+		t.Errorf("error does not point at `dash0 login`: %q", err.Error())
+	}
+}
+
+// TestTranslateConfigError_RevokedRefreshToken_AgentMode asserts the
+// agent-mode branch of the same translation does not tell the caller to
+// run `dash0 login` (it may still explain that login is unavailable) and
+// instead surfaces the DASH0_AUTH_TOKEN / --oauth=false escape hatches.
+func TestTranslateConfigError_RevokedRefreshToken_AgentMode(t *testing.T) {
+	prev := agentmode.Enabled
+	agentmode.Enabled = true
+	defer func() { agentmode.Enabled = prev }()
+
+	err := translateConfigError(context.Background(), revokedRefreshTokenError(t))
+	if err == nil {
+		t.Fatalf("expected a translated error, got nil")
+	}
+	if strings.Contains(err.Error(), "Run `dash0 login") {
+		t.Errorf("agent-mode error must not instruct the caller to run `dash0 login`: %q", err.Error())
+	}
+	if !strings.Contains(err.Error(), "DASH0_AUTH_TOKEN") {
+		t.Errorf("agent-mode error must surface DASH0_AUTH_TOKEN: %q", err.Error())
+	}
+	if !strings.Contains(err.Error(), "--oauth=false") {
+		t.Errorf("agent-mode error must surface the profile conversion path: %q", err.Error())
+	}
+}

internal/config/config_cmd.go

@@ -527,7 +527,11 @@ are mutually exclusive.`,
 				// after the update so the user knows the AS still holds
 				// the token.
 				if existing.OAuth != nil {
-					if !oauthpkg.Revoke(existing.ApiUrl, existing.OAuth.RefreshToken) {
+					if !oauthpkg.Revoke(oauthpkg.RevokeRequest{
+						APIURL:       existing.ApiUrl,
+						ClientID:     existing.OAuth.ClientID,
+						RefreshToken: existing.OAuth.RefreshToken,
+					}) {
 						defer fmt.Println("Note: server-side refresh-token revocation failed; the token will remain valid on the authorization server until natural expiry.")
 					}
 				}

internal/config/config_cmd_test.go

@@ -5,6 +5,9 @@ import (
 	"context"
 	"encoding/json"
 	"io"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"os"
 	"testing"
 
@@ -1214,3 +1217,68 @@ func TestUpdateProfileCmdAuthTokenOnOAuthActiveErrors(t *testing.T) {
 		t.Errorf("expected error to suggest --oauth=false, got: %v", err)
 	}
 }
+
+func TestUpdateProfileCmdOAuthFalseRevokesWithClientID(t *testing.T) {
+	_ = setupTestConfigDir(t)
+
+	var gotForm url.Values
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/oauth/revoke" {
+			t.Errorf("unexpected request to %s", r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		if err := r.ParseForm(); err != nil {
+			t.Errorf("failed to parse revoke form: %v", err)
+			w.WriteHeader(http.StatusBadRequest)
+			return
+		}
+		gotForm = r.Form
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	store, _ := profiles.NewStore()
+	if err := store.AddProfile(profiles.Profile{
+		Name: "oauth-active",
+		Configuration: profiles.Configuration{
+			ApiUrl:    server.URL,
+			AuthToken: "dash0_at_xxxxxxxxxxxx",
+			OAuth: &profiles.OAuthState{
+				ClientID:     "cli-client-id-abc",
+				RefreshToken: "dash0_rt_yyy",
+			},
+		},
+	}); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	rootCmd := &cobra.Command{Use: "dash0"}
+	rootCmd.AddCommand(NewConfigCmd())
+
+	if _, err := executeCommand(rootCmd, "config", "profiles", "update", "oauth-active",
+		"--oauth=false", "--auth-token", "auth_new_xxxxxx", "--force"); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if gotForm == nil {
+		t.Fatalf("expected a revoke request to reach the server")
+	}
+	if got := gotForm.Get("client_id"); got != "cli-client-id-abc" {
+		t.Errorf("expected client_id %q on the revoke request, got %q", "cli-client-id-abc", got)
+	}
+	if got := gotForm.Get("token"); got != "dash0_rt_yyy" {
+		t.Errorf("expected token %q on the revoke request, got %q", "dash0_rt_yyy", got)
+	}
+
+	all, _ := store.GetProfiles()
+	if len(all) != 1 {
+		t.Fatalf("expected 1 profile, got %d", len(all))
+	}
+	if all[0].Configuration.AuthToken != "auth_new_xxxxxx" {
+		t.Errorf("expected new AuthToken to be set, got %q", all[0].Configuration.AuthToken)
+	}
+	if all[0].Configuration.OAuth != nil {
+		t.Errorf("expected OAuth block cleared after --oauth=false, got %+v", all[0].Configuration.OAuth)
+	}
+}

internal/login/integration_test.go

@@ -34,11 +34,12 @@ type fakeOAuthServer struct {
 	server   *httptest.Server
 	clientID string
 
-	issuedCode      atomic.Value // string
-	issuedChallenge atomic.Value // string -- PKCE code_challenge stored on /authorize
-	revokeCount     atomic.Int32
-	revoked         sync.Mutex
-	revokedList     []string
+	issuedCode       atomic.Value // string
+	issuedChallenge  atomic.Value // string -- PKCE code_challenge stored on /authorize
+	revokeCount      atomic.Int32
+	revoked          sync.Mutex
+	revokedList      []string
+	revokedClientIDs map[string]string // token -> client_id
 
 	// tokenCounter lets us hand out distinct access/refresh tokens so the
 	// re-login test can prove the old refresh was revoked.
@@ -80,6 +81,12 @@ func (f *fakeOAuthServer) Revoked() []string {
 	return out
 }
 
+func (f *fakeOAuthServer) RevokedClientID(token string) string {
+	f.revoked.Lock()
+	defer f.revoked.Unlock()
+	return f.revokedClientIDs[token]
+}
+
 func (f *fakeOAuthServer) handleDiscovery(w http.ResponseWriter, _ *http.Request) {
 	w.Header().Set("Content-Type", "application/json")
 	_ = json.NewEncoder(w).Encode(map[string]any{
@@ -205,12 +212,21 @@ func (f *fakeOAuthServer) handleToken(w http.ResponseWriter, r *http.Request) {
 }
 
 func (f *fakeOAuthServer) handleRevoke(w http.ResponseWriter, r *http.Request) {
-	require.NoError(f.t, r.ParseForm())
+	if !assert.NoError(f.t, r.ParseForm()) {
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
 	token := r.Form.Get("token")
-	require.NotEmpty(f.t, token, "revoke requires a token")
+	assert.NotEmpty(f.t, token, "revoke requires a token")
+	clientID := r.Form.Get("client_id")
+	assert.NotEmpty(f.t, clientID, "revoke requires a client_id")
 	f.revokeCount.Add(1)
 	f.revoked.Lock()
 	f.revokedList = append(f.revokedList, token)
+	if f.revokedClientIDs == nil {
+		f.revokedClientIDs = make(map[string]string)
+	}
+	f.revokedClientIDs[token] = clientID
 	f.revoked.Unlock()
 	w.WriteHeader(http.StatusOK)
 }
@@ -399,6 +415,43 @@ func TestRunLogin_OnOAuthActive_RevokesOldRefresh(t *testing.T) {
 	require.Equal(t, "dash0_rt_OLD_to_be_revoked", revoked[0])
 }
 
+func TestRunLogin_OnOAuthActive_RevokesOldRefreshWithOldClientID(t *testing.T) {
+	forceInteractive(t)
+	t.Setenv("DASH0_CONFIG_DIR", t.TempDir())
+
+	server := newFakeOAuthServer(t)
+	defer server.Close()
+	server.clientID = "client-freshly-registered"
+
+	store, _ := profiles.NewStore()
+	require.NoError(t, store.AddProfile(profiles.Profile{
+		Name: "active-prof",
+		Configuration: profiles.Configuration{
+			ApiUrl:    server.URL(),
+			AuthToken: "dash0_at_old_xxxxxxxxxxxx",
+			OAuth: &profiles.OAuthState{
+				ClientID:     "client-that-issued-the-old-token",
+				RefreshToken: "dash0_rt_OLD_to_be_revoked",
+				ExpiresAt:    time.Now().Add(time.Hour),
+			},
+		},
+	}))
+
+	defer driveBrowserOnce(t)()
+
+	err := runLogin(context.Background(), loginOptions{
+		ProfileName: "active-prof",
+		Timeout:     5 * time.Second,
+	})
+	require.NoError(t, err)
+
+	revoked := server.Revoked()
+	require.Len(t, revoked, 1)
+	require.Equal(t, "dash0_rt_OLD_to_be_revoked", revoked[0])
+	require.Equal(t, "client-that-issued-the-old-token", server.RevokedClientID("dash0_rt_OLD_to_be_revoked"),
+		"the old refresh token must be revoked using the client_id that issued it, not the freshly-registered one")
+}
+
 func TestRunLogin_RejectsInAgentMode(t *testing.T) {
 	prev := isTerminal
 	isTerminal = func() bool { return false }