Merge pull request #1510 from bilogic/dev

get refreshable tokens

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()})

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)

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

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{