From 72c2c301a5624973b254ceab31216764ce880869 Mon Sep 17 00:00:00 2001 From: bilogic <946010+bilogic@users.noreply.github.com> Date: Sat, 8 Aug 2026 10:12:58 +0800 Subject: [PATCH] get refreshable tokens --- internal/http/mcp_oauth.go | 43 +++++++++++++++++++++--------- internal/http/mcp_oauth_test.go | 31 ++++++++++++++++++++++ internal/mcp/oauth/flow.go | 17 +++++++++++- internal/mcp/oauth/flow_test.go | 47 +++++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 13 deletions(-) diff --git a/internal/http/mcp_oauth.go b/internal/http/mcp_oauth.go index 4f2b15fb..daf83f71 100644 --- a/internal/http/mcp_oauth.go +++ b/internal/http/mcp_oauth.go @@ -7,6 +7,7 @@ import ( "html" "log/slog" "net/http" + "net/url" "strings" "time" @@ -39,12 +40,12 @@ type MCPOAuthHandler struct { // MCPOAuthHandlerDeps contains all dependencies for the OAuth handler. type MCPOAuthHandlerDeps struct { - MCPStore store.MCPServerStore - OAuthStore store.MCPOAuthTokenStore - Discoverer *mcpoauth.Discoverer - FlowMgr *mcpoauth.FlowManager - Refresher *mcpoauth.Refresher - EventBus bus.EventPublisher + MCPStore store.MCPServerStore + OAuthStore store.MCPOAuthTokenStore + Discoverer *mcpoauth.Discoverer + FlowMgr *mcpoauth.FlowManager + Refresher *mcpoauth.Refresher + EventBus bus.EventPublisher PublicURL string Port int Evictor MCPPoolEvictor @@ -54,12 +55,12 @@ type MCPOAuthHandlerDeps struct { // NewMCPOAuthHandler creates an MCPOAuthHandler. func NewMCPOAuthHandler(deps MCPOAuthHandlerDeps) *MCPOAuthHandler { return &MCPOAuthHandler{ - mcpStore: deps.MCPStore, - oauthStore: deps.OAuthStore, - discoverer: deps.Discoverer, - flowMgr: deps.FlowMgr, - refresher: deps.Refresher, - eventBus: deps.EventBus, + mcpStore: deps.MCPStore, + oauthStore: deps.OAuthStore, + discoverer: deps.Discoverer, + flowMgr: deps.FlowMgr, + refresher: deps.Refresher, + eventBus: deps.EventBus, publicURL: deps.PublicURL, port: deps.Port, evictor: deps.Evictor, @@ -150,6 +151,23 @@ type startOAuthResp struct { Completed bool `json:"completed,omitempty"` // true for client_credentials — token already minted, no redirect needed } +// dropboxOfflineAuthParam returns the authorization-request parameter Dropbox +// requires to issue a refresh token. Dropbox's default is an online-only token +// (expires ~4h, cannot be refreshed), so every Dropbox authorize URL must carry +// token_access_type=offline for goclaw's auto-refresh to work. Returns nil for +// every other authorization server — unknown params could break stricter ASes. +func dropboxOfflineAuthParam(authEndpoint string) url.Values { + u, err := url.Parse(authEndpoint) + if err != nil { + return nil + } + host := u.Hostname() + if host == "www.dropbox.com" || strings.HasSuffix(host, ".dropbox.com") { + return url.Values{"token_access_type": {"offline"}} + } + return nil +} + // authorizeOAuthScope enforces who may operate on a given OAuth token scope. // A per-user token (non-empty targetUserID) may be managed by its owner without // admin rights — self-service, matching the per-user MCP credentials flow and the @@ -377,6 +395,7 @@ func (h *MCPOAuthHandler) handleStart(w http.ResponseWriter, r *http.Request) { RedirectURI: callbackURI, Scopes: scopes, GrantType: oauthSettings.OAuth.GrantType, + ExtraAuthParams: dropboxOfflineAuthParam(disc.AuthorizationEndpoint), }) if err != nil { writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) diff --git a/internal/http/mcp_oauth_test.go b/internal/http/mcp_oauth_test.go index d34588ea..7044ed18 100644 --- a/internal/http/mcp_oauth_test.go +++ b/internal/http/mcp_oauth_test.go @@ -264,6 +264,37 @@ func newTestMCPOAuthHandler( // callbackURL helper tests // -------------------------------------------------------------------------- +func TestDropboxOfflineAuthParam(t *testing.T) { + tests := []struct { + name string + endpt string + wantK string + wantV string + }{ + {"dropbox www host", "https://www.dropbox.com/oauth2/authorize", "token_access_type", "offline"}, + {"dropbox api subdomain", "https://api.dropbox.com/oauth2/authorize", "token_access_type", "offline"}, + {"non-dropbox returns nil", "https://auth.example.com/authorize", "", ""}, + {"malformed url returns nil", "://not-a-url", "", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := dropboxOfflineAuthParam(tt.endpt) + if tt.wantK == "" { + if got != nil { + t.Errorf("dropboxOfflineAuthParam(%q) = %v, want nil", tt.endpt, got) + } + return + } + if got == nil { + t.Fatalf("dropboxOfflineAuthParam(%q) = nil, want %s=%s", tt.endpt, tt.wantK, tt.wantV) + } + if v := got.Get(tt.wantK); v != tt.wantV { + t.Errorf("dropboxOfflineAuthParam(%q)[%s] = %q, want %q", tt.endpt, tt.wantK, v, tt.wantV) + } + }) + } +} + func TestCallbackURLPublicURL(t *testing.T) { h := &MCPOAuthHandler{publicURL: "https://goclaw.example.com"} r := httptest.NewRequest(http.MethodGet, "/", nil) diff --git a/internal/mcp/oauth/flow.go b/internal/mcp/oauth/flow.go index eb55f26e..85c47550 100644 --- a/internal/mcp/oauth/flow.go +++ b/internal/mcp/oauth/flow.go @@ -40,7 +40,7 @@ type PendingFlow struct { UserID string // empty = global/tenant-level, non-empty = per-user InitiatingUserID string // admin who called /start — used for WS event routing CodeVerifier string - UsePKCE bool // true → send code_verifier on token exchange + UsePKCE bool // true → send code_verifier on token exchange RedirectURI string TokenEndpoint string ClientID string @@ -66,6 +66,13 @@ type StartFlowParams struct { // "authorization_code" — Authorization Code + PKCE S256, confidential client (sends ClientSecret). // "client_credentials" — not handled by StartFlow; use ClientCredentials() instead. GrantType string + + // ExtraAuthParams carries provider-specific authorization-request parameters + // that goclaw's generic flow does not otherwise set (e.g. Dropbox's + // token_access_type=offline, which the AS requires to issue a refresh token; + // without it Dropbox returns an online-only token that cannot be refreshed). + // Standard OAuth keys already set by StartFlow are never overridden. + ExtraAuthParams url.Values } // FlowManager manages in-progress OAuth authorization code flows. @@ -144,6 +151,14 @@ func (fm *FlowManager) StartFlow(ctx context.Context, p StartFlowParams) (authUR if p.DiscoveryResult.ResourceURI != "" { q.Set("resource", p.DiscoveryResult.ResourceURI) // RFC 8707 } + for k, vs := range p.ExtraAuthParams { + if q.Has(k) { + continue // standard OAuth params win — never overridden + } + for _, v := range vs { + q.Add(k, v) + } + } authURL = p.DiscoveryResult.AuthorizationEndpoint + "?" + q.Encode() return authURL, state, nil diff --git a/internal/mcp/oauth/flow_test.go b/internal/mcp/oauth/flow_test.go index 3c8daa16..d2367924 100644 --- a/internal/mcp/oauth/flow_test.go +++ b/internal/mcp/oauth/flow_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "net/url" "strings" "testing" "time" @@ -78,6 +79,52 @@ func TestStartFlowReturnsPKCEAuthURL(t *testing.T) { } } +func TestStartFlowIncludesExtraAuthParams(t *testing.T) { + fm := NewFlowManager(http.DefaultClient) + disc := &DiscoveryResult{ + AuthorizationEndpoint: "https://auth.example.com/authorize", + TokenEndpoint: "https://auth.example.com/token", + } + authURL, _, err := fm.StartFlow(context.Background(), StartFlowParams{ + ServerID: uuid.New(), + TenantID: uuid.New(), + DiscoveryResult: disc, + ClientID: "test-client", + RedirectURI: "https://goclaw.example.com/v1/mcp/oauth/callback", + GrantType: "pkce", + ExtraAuthParams: url.Values{"token_access_type": {"offline"}}, + }) + if err != nil { + t.Fatalf("StartFlow() error: %v", err) + } + if !strings.Contains(authURL, "token_access_type=offline") { + t.Errorf("authURL missing token_access_type=offline: %s", authURL) + } +} + +func TestStartFlowDoesNotOverrideStandardParams(t *testing.T) { + fm := NewFlowManager(http.DefaultClient) + disc := &DiscoveryResult{ + AuthorizationEndpoint: "https://auth.example.com/authorize", + TokenEndpoint: "https://auth.example.com/token", + } + authURL, _, err := fm.StartFlow(context.Background(), StartFlowParams{ + ServerID: uuid.New(), + TenantID: uuid.New(), + DiscoveryResult: disc, + ClientID: "test-client", + RedirectURI: "https://goclaw.example.com/v1/mcp/oauth/callback", + GrantType: "pkce", + ExtraAuthParams: url.Values{"client_id": {"evil"}}, + }) + if err != nil { + t.Fatalf("StartFlow() error: %v", err) + } + if strings.Contains(authURL, "client_id=evil") { + t.Errorf("ExtraAuthParams must not override standard client_id: %s", authURL) + } +} + func TestStartFlowStoresInPending(t *testing.T) { fm := NewFlowManager(http.DefaultClient) disc := &DiscoveryResult{