diff --git a/forge-go/conf/oauth-providers.yaml b/forge-go/conf/oauth-providers.yaml index 19f7470..d71be42 100644 --- a/forge-go/conf/oauth-providers.yaml +++ b/forge-go/conf/oauth-providers.yaml @@ -13,6 +13,39 @@ # use_pkce: controls PKCE (S256 code challenge). Defaults to true. Set to # false for providers that do not support it. # +# prompt: consent is sent on every provider's authorization request by default. +# Servers that follow OIDC Core §11 strictly need it to issue a refresh token; +# the rest ignore it. Override or disable it via auth_params below. +# +# auth_params: extra parameters added to the authorization request only (never +# the token exchange or refresh). Needed because providers disagree on how to +# ask for a refresh token, and getting it wrong fails silently — you connect +# successfully and get an empty refresh_token: +# +# Authorization servers that follow OIDC Core §11 strictly +# scopes must include offline_access, and the request must carry +# prompt: consent (the default). Without the prompt the scope is stripped +# from the grant with no error and no refresh token is issued. +# Google +# ignores offline_access; wants access_type: offline. It also returns a +# refresh token only on the FIRST grant per (account, client_id) — a grant +# Google remembers long after Forge's stored token is gone — so the +# prompt: consent default is what makes reconnects work. +# Microsoft Entra v2.0, Okta, Auth0 +# offline_access in scopes is enough; the prompt is harmless. +# GitHub, Slack +# neither applies. See the entries below. +# +# A parameter with an empty value is omitted, which is how you switch off a +# default — set prompt: "" for a provider that rejects the parameter. +# Parameters the flow generates itself (state, code_challenge, redirect_uri, +# scope, resource, ...) are rejected at startup. +# +# Note the prompt default only earns you a refresh token in combination with +# offline_access in scopes, which stays explicit per provider: a server may +# reject an unknown scope outright (RFC 6749 §3.3), and turning "connects but +# cannot refresh" into "cannot connect" would be the worst failure. +# # resource_url: the canonical URI of the protected resource behind this # provider (e.g. an MCP server endpoint). When set, Forge sends it as the # RFC 8707 `resource` parameter on the authorization, token and refresh @@ -32,6 +65,9 @@ # editing this file. providers: +# # Classic GitHub OAuth apps issue a non-expiring access token and no refresh +# # token at all, so there is nothing to configure here. Refresh tokens require +# # a GitHub App with "expiring user tokens" enabled. # github: # display_name: GitHub # description: Access GitHub repositories and user profile for code reading and pull request automation. @@ -39,7 +75,14 @@ providers: # google-drive: # display_name: Google Drive # description: Read and write files in Google Drive for document-based workflows. +# scopes: [https://www.googleapis.com/auth/drive] +# # Google ignores offline_access and needs access_type for a refresh token. +# # The prompt: consent default is what makes reconnects get one too. +# auth_params: +# access_type: offline # +# # Slack has no offline_access and no prompt. Refresh tokens arrive only if +# # token rotation is enabled on the Slack app itself. # slack: # display_name: Slack # description: Send messages and read channel history to integrate agents with your Slack workspace. @@ -54,6 +97,10 @@ providers: # use_pkce: false # # Optional: ask for a token restricted to this resource (RFC 8707). # resource_url: https://api.example.com +# # Optional: extra authorization-request parameters. Setting prompt here +# # overrides the default; prompt: "" sends none. +# auth_params: +# access_type: offline # # Dynamic Client Registration example (e.g. a remote MCP server): # notion-mcp: @@ -61,3 +108,7 @@ providers: # description: Connect to the Notion MCP server. # resource_url: https://mcp.notion.com/mcp # use_dcrp: true +# # prompt: consent is applied automatically, but it only earns a refresh +# # token in combination with this scope. +# scopes: [offline_access] + diff --git a/forge-go/oauth/config.go b/forge-go/oauth/config.go index 0032978..79f92ff 100644 --- a/forge-go/oauth/config.go +++ b/forge-go/oauth/config.go @@ -2,9 +2,11 @@ package oauth import ( "fmt" + "maps" "net/url" "os" "regexp" + "slices" "gopkg.in/yaml.v3" ) @@ -22,6 +24,25 @@ type ProviderConfig struct { // Set to false for providers that do not support it. UsePKCE *bool `yaml:"use_pkce" json:"usePkce,omitempty"` + // AuthParams are extra parameters appended to the authorization request — + // and only that request, never the token exchange or the refresh. They exist + // for provider-specific knobs the generic flow does not model, because + // providers disagree on how to ask for a refresh token: + // + // prompt: consent // authorization servers that follow OIDC Core §11 + // // strictly silently drop the offline_access scope + // // without it, so no refresh token is ever issued. + // // Sent by default; see authParams. + // access_type: offline // Google's proprietary offline_access equivalent + // + // A parameter whose value is empty is left out entirely; that is how a + // built-in default is switched off (see authParams). + // + // Parameters the flow generates for itself — state, code_challenge, and the + // rest of reservedAuthParams — cannot be set here; they are rejected at + // load time. + AuthParams map[string]string `yaml:"auth_params" json:"authParams,omitempty"` + // ResourceURL is the canonical URI of the OAuth2 protected resource this // provider fronts (e.g. an MCP server endpoint). // @@ -51,15 +72,73 @@ func (p ProviderConfig) RequiresClientCredentials() bool { return !p.UseDCRP } +// reservedAuthParams are the authorization-request parameters the flow derives +// itself, so auth_params is not allowed to supply them. +// +// The restriction is load-bearing rather than tidiness: +// oauth2.Config.AuthCodeURL writes its own parameters first and then applies +// each AuthCodeOption with url.Values.Set, so a colliding auth_params entry +// would silently overwrite the generated value — including state, which is the +// CSRF defence, and code_challenge, which is what binds the code to this +// client. resource is reserved too: it is derived from ResourceURL and has to +// match on the exchange and refresh, which auth_params does not reach. +var reservedAuthParams = map[string]struct{}{ + "client_id": {}, + "client_secret": {}, + "code": {}, + "code_challenge": {}, + "code_challenge_method": {}, + "code_verifier": {}, + "grant_type": {}, + "redirect_uri": {}, + "resource": {}, + "response_type": {}, + "scope": {}, + "state": {}, +} + +// authParams returns the parameters to append to the authorization request. +// +// Every provider gets prompt=consent by default, so the behaviour is the same +// whether the far side is a static provider or one registered by DCR. It is the +// safe direction to be wrong in: a server that follows OIDC Core §11 strictly +// needs it to issue a refresh token at all, and one that does not simply +// ignores it. An explicit prompt in auth_params takes precedence; set it to the +// empty string to opt out, for a provider that rejects the parameter. +// +// The default only has an effect together with offline_access in Scopes, which +// stays explicit per provider: an authorization server may reject an unknown +// scope outright (RFC 6749 §3.3), and turning "connects but cannot refresh" into +// "cannot connect" would be the worse failure. +func (p ProviderConfig) authParams() map[string]string { + if _, set := p.AuthParams["prompt"]; set { + return p.AuthParams + } + out := make(map[string]string, len(p.AuthParams)+1) + maps.Copy(out, p.AuthParams) + out["prompt"] = "consent" + return out +} + // Validate reports configuration errors that would otherwise surface only when // an auth flow is started: DCR discovers its endpoints from the resource, so it // requires resource_url, and resource_url — which is sent as the RFC 8707 // `resource` parameter — must be a valid resource indicator: an absolute URI -// with no fragment (RFC 8707, section 2). +// with no fragment (RFC 8707, section 2). auth_params must not collide with the +// parameters the flow generates for itself. func (p ProviderConfig) Validate(id string) error { if p.UseDCRP && p.ResourceURL == "" { return fmt.Errorf("provider %q: use_dcrp requires resource_url", id) } + // Sorted so a config with several bad names always reports the same one. + for _, k := range slices.Sorted(maps.Keys(p.AuthParams)) { + if k == "" { + return fmt.Errorf("provider %q: auth_params has an empty parameter name", id) + } + if _, reserved := reservedAuthParams[k]; reserved { + return fmt.Errorf("provider %q: auth_params must not set %q; the OAuth2 flow generates it", id, k) + } + } if p.ResourceURL != "" { u, err := url.Parse(p.ResourceURL) if err != nil { @@ -118,6 +197,15 @@ func LoadProvidersConfig(path string) (*ProvidersConfig, error) { p.TokenURL = interpolateEnv(p.TokenURL) p.RedirectURL = interpolateEnv(p.RedirectURL) p.ResourceURL = interpolateEnv(p.ResourceURL) + if len(p.AuthParams) > 0 { + // A fresh map rather than an in-place rewrite, so the parsed config + // is not aliased by the interpolated one. + params := make(map[string]string, len(p.AuthParams)) + for k, v := range p.AuthParams { + params[k] = interpolateEnv(v) + } + p.AuthParams = params + } if err := p.Validate(id); err != nil { return nil, fmt.Errorf("parsing oauth providers config: %w", err) } diff --git a/forge-go/oauth/manager.go b/forge-go/oauth/manager.go index eda3019..85273b4 100644 --- a/forge-go/oauth/manager.go +++ b/forge-go/oauth/manager.go @@ -254,6 +254,14 @@ func (m *Manager) GetAuthURL(ctx context.Context, orgID, providerID, clientID, c } var authOpts []oauth2.AuthCodeOption + // Provider-specific extras first, so the generated PKCE and resource options + // below still win on any collision that got past validation. + for k, v := range cfg.authParams() { + if v == "" { + continue // an empty value switches a built-in default off + } + authOpts = append(authOpts, oauth2.SetAuthURLParam(k, v)) + } if usePKCE { authOpts = append(authOpts, oauth2.S256ChallengeOption(verifier)) } @@ -362,7 +370,8 @@ func (m *Manager) ExchangeCode(ctx context.Context, code, state string) (provide } // GetAccessToken returns a valid access token for the provider, refreshing it -// if it expires within 60 seconds. +// if it expires within 60 seconds. A token with no expiry never expires and is +// returned as-is. func (m *Manager) GetAccessToken(ctx context.Context, orgID, providerID string) (string, error) { m.mu.Lock() defer m.mu.Unlock() @@ -372,7 +381,12 @@ func (m *Manager) GetAccessToken(ctx context.Context, orgID, providerID string) return "", fmt.Errorf("provider %q not connected for org %q: %w", providerID, orgID, ErrNotConnected) } - if entry.token.Valid() && time.Until(entry.token.Expiry) > 60*time.Second { + // A zero Expiry means "never expires" (Token.Valid agrees), but time.Until + // on it is hugely negative, so it has to be checked separately or a + // non-expiring token — a Slack bot token, a classic GitHub token — would + // take the refresh path on every single call and rewrite the keychain entry + // for nothing. + if entry.token.Valid() && (entry.token.Expiry.IsZero() || time.Until(entry.token.Expiry) > 60*time.Second) { return entry.token.AccessToken, nil } diff --git a/forge-go/oauth/manager_test.go b/forge-go/oauth/manager_test.go index 6cdc01d..30d6553 100644 --- a/forge-go/oauth/manager_test.go +++ b/forge-go/oauth/manager_test.go @@ -2,6 +2,7 @@ package oauth import ( "context" + "net/url" "os" "strings" "testing" @@ -119,6 +120,111 @@ func TestGetAuthURL_DCRDiscoversAndUsesRegisteredClient(t *testing.T) { } } +func TestGetAuthURL_SendsAuthParams(t *testing.T) { + cfg := &ProvidersConfig{ + Providers: map[string]ProviderConfig{ + // A server that follows OIDC Core §11 strictly: without + // prompt=consent it drops offline_access and issues no refresh token. + "strict-oidc": { + AuthURL: "https://login.example.com/oidc/auth", + TokenURL: "https://login.example.com/oidc/token", + Scopes: []string{"openid", "offline_access"}, + ResourceURL: "https://api.example.com", + AuthParams: map[string]string{"prompt": "consent"}, + }, + // Google's dialect: access_type instead of offline_access, on top of + // the default prompt. + "google": { + AuthURL: "https://example.com/oauth/authorize", + TokenURL: "https://example.com/oauth/token", + AuthParams: map[string]string{"access_type": "offline"}, + }, + "plain": { + AuthURL: "https://example.com/oauth/authorize", + TokenURL: "https://example.com/oauth/token", + }, + "empty value": { + AuthURL: "https://example.com/oauth/authorize", + TokenURL: "https://example.com/oauth/token", + AuthParams: map[string]string{"prompt": ""}, + }, + }, + } + m := NewManager(cfg) + for id := range cfg.Providers { + m.CheckAndUpdateProvider(id, nil) + } + + t.Run("params are sent and do not disturb the generated ones", func(t *testing.T) { + authURL, state, err := m.GetAuthURL(context.Background(), "org1", "strict-oidc", "cid", "csecret", "https://example.com/cb") + if err != nil { + t.Fatalf("GetAuthURL failed: %v", err) + } + if got := queryParam(t, authURL, "prompt"); got != "consent" { + t.Errorf("prompt = %q, want %q", got, "consent") + } + // auth_params is applied before PKCE and resource, so the generated + // values must all still be intact and correct. + if got := queryParam(t, authURL, "state"); got != state { + t.Errorf("state = %q, want the returned state %q", got, state) + } + if queryParam(t, authURL, "code_challenge") == "" { + t.Error("code_challenge missing") + } + if got := queryParam(t, authURL, "code_challenge_method"); got != "S256" { + t.Errorf("code_challenge_method = %q, want S256", got) + } + if got := queryParam(t, authURL, "resource"); got != "https://api.example.com" { + t.Errorf("resource = %q", got) + } + if got := queryParam(t, authURL, "redirect_uri"); got != "https://example.com/cb" { + t.Errorf("redirect_uri = %q", got) + } + if got := queryParam(t, authURL, "scope"); got != "openid offline_access" { + t.Errorf("scope = %q", got) + } + }) + + t.Run("configured params sit alongside the default prompt", func(t *testing.T) { + authURL, _, err := m.GetAuthURL(context.Background(), "org1", "google", "cid", "csecret", "https://example.com/cb") + if err != nil { + t.Fatalf("GetAuthURL failed: %v", err) + } + if got := queryParam(t, authURL, "access_type"); got != "offline" { + t.Errorf("access_type = %q, want offline", got) + } + if got := queryParam(t, authURL, "prompt"); got != "consent" { + t.Errorf("prompt = %q, want consent", got) + } + }) + + t.Run("no auth_params still gets the default prompt", func(t *testing.T) { + authURL, _, err := m.GetAuthURL(context.Background(), "org1", "plain", "cid", "csecret", "https://example.com/cb") + if err != nil { + t.Fatalf("GetAuthURL failed: %v", err) + } + if got := queryParam(t, authURL, "prompt"); got != "consent" { + t.Errorf("prompt = %q, want consent", got) + } + }) + + t.Run("empty value opts out and is omitted entirely", func(t *testing.T) { + authURL, _, err := m.GetAuthURL(context.Background(), "org1", "empty value", "cid", "csecret", "https://example.com/cb") + if err != nil { + t.Fatalf("GetAuthURL failed: %v", err) + } + u, err := url.Parse(authURL) + if err != nil { + t.Fatal(err) + } + // Not just empty — the key must not be present at all, otherwise the + // provider receives a meaningless "prompt=". + if _, present := u.Query()["prompt"]; present { + t.Error("prompt should be absent, not sent empty") + } + }) +} + func TestCallbackURL(t *testing.T) { base := "https://forge.example.com/api" // A single constant callback for every provider (flow identified by state). @@ -181,6 +287,151 @@ func TestLoadProvidersConfig_RejectsInvalidResourceConfig(t *testing.T) { } } +// auth_params must not be able to overwrite what the flow generates. AuthCodeURL +// applies AuthCodeOptions after its own parameters, so an unchecked entry would +// win over the real state or code_challenge. +func TestLoadProvidersConfig_RejectsReservedAuthParams(t *testing.T) { + cases := map[string]string{ + "state": `providers: + broken: + auth_params: + state: attacker-chosen`, + "code_challenge": `providers: + broken: + auth_params: + code_challenge: attacker-chosen`, + "redirect_uri": `providers: + broken: + auth_params: + redirect_uri: https://evil.example.com/cb`, + // Derived from resource_url, and has to match on exchange and refresh — + // which auth_params does not reach. + "resource": `providers: + broken: + auth_params: + resource: https://evil.example.com`, + "scope": `providers: + broken: + auth_params: + scope: openid`, + "empty name": `providers: + broken: + auth_params: + "": consent`, + } + for name, yaml := range cases { + t.Run(name, func(t *testing.T) { + path := t.TempDir() + "/providers.yaml" + if err := os.WriteFile(path, []byte(yaml), 0644); err != nil { + t.Fatal(err) + } + if _, err := LoadProvidersConfig(path); err == nil { + t.Errorf("expected validation error for %q, got nil", name) + } + }) + } +} + +func TestLoadProvidersConfig_InterpolatesAuthParams(t *testing.T) { + t.Setenv("TEST_PROMPT_VALUE", "consent") + yaml := `providers: + api: + auth_url: https://example.com/oauth/authorize + token_url: https://example.com/oauth/token + auth_params: + prompt: ${TEST_PROMPT_VALUE} + access_type: offline` + + path := t.TempDir() + "/providers.yaml" + if err := os.WriteFile(path, []byte(yaml), 0644); err != nil { + t.Fatal(err) + } + cfg, err := LoadProvidersConfig(path) + if err != nil { + t.Fatalf("LoadProvidersConfig failed: %v", err) + } + p := cfg.Providers["api"] + if got := p.AuthParams["prompt"]; got != "consent" { + t.Errorf("prompt = %q, want %q", got, "consent") + } + if got := p.AuthParams["access_type"]; got != "offline" { + t.Errorf("access_type = %q, want %q", got, "offline") + } +} + +// prompt=consent is the default for every provider, static or DCR alike: a +// server that follows OIDC Core §11 strictly needs it to issue a refresh token, +// and one that does not ignores it. A DCR provider is registered against an +// authorization server we have no console for, so there is no other way to +// intervene there at all. +func TestAuthParams_DefaultsToPromptConsent(t *testing.T) { + cases := map[string]struct { + cfg ProviderConfig + want string // "" means the prompt parameter should be absent + }{ + "dcr gets the default": { + cfg: ProviderConfig{UseDCRP: true, ResourceURL: "https://mcp.example.com/mcp"}, + want: "consent", + }, + "explicit prompt wins": { + cfg: ProviderConfig{UseDCRP: true, ResourceURL: "https://mcp.example.com/mcp", + AuthParams: map[string]string{"prompt": "login"}}, + want: "login", + }, + "empty prompt switches the default off": { + cfg: ProviderConfig{UseDCRP: true, ResourceURL: "https://mcp.example.com/mcp", + AuthParams: map[string]string{"prompt": ""}}, + want: "", + }, + "static provider gets the default too": { + cfg: ProviderConfig{AuthURL: "https://example.com/a", TokenURL: "https://example.com/t"}, + want: "consent", + }, + "static provider can opt out": { + cfg: ProviderConfig{AuthURL: "https://example.com/a", TokenURL: "https://example.com/t", + AuthParams: map[string]string{"prompt": ""}}, + want: "", + }, + "other auth_params do not suppress the default": { + cfg: ProviderConfig{AuthURL: "https://example.com/a", TokenURL: "https://example.com/t", + AuthParams: map[string]string{"access_type": "offline"}}, + want: "consent", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + if got := tc.cfg.authParams()["prompt"]; got != tc.want { + t.Errorf("prompt = %q, want %q", got, tc.want) + } + }) + } +} + +// The DCR default must survive all the way into the real authorization URL, not +// just authParams(). +func TestGetAuthURL_DCRSendsPromptConsent(t *testing.T) { + cfg := &ProvidersConfig{ + Providers: map[string]ProviderConfig{ + "mcp": {ResourceURL: "https://mcp.example.com/mcp", UseDCRP: true, Scopes: []string{"offline_access"}}, + }, + } + m := NewManager(cfg) + m.CheckAndUpdateProvider("mcp", nil) + m.seedDiscovery("https://mcp.example.com/mcp", &resolvedProvider{ + endpoint: oauth2.Endpoint{AuthURL: "https://as.example.com/authorize", TokenURL: "https://as.example.com/token"}, + authMethods: []string{"none"}, + }) + _ = m.credStore.SaveCredentials("mcp", &clientCredentials{ClientID: "registered-id"}) + + authURL, _, err := m.GetAuthURL(context.Background(), "org1", "mcp", "", "", "https://example.com/cb") + if err != nil { + t.Fatalf("GetAuthURL failed: %v", err) + } + if got := queryParam(t, authURL, "prompt"); got != "consent" { + t.Errorf("prompt = %q, want %q", got, "consent") + } +} + // A static provider may declare resource_url purely to have the RFC 8707 // resource indicator sent; it is not DCR-only. func TestLoadProvidersConfig_ResourceURLWithoutDCR(t *testing.T) { @@ -300,3 +551,73 @@ func TestProviderConfig_ParsesScopes(t *testing.T) { t.Errorf("unexpected display name: %s", p.DisplayName) } } + +// countingTokenStore records how many times a token entry is written back, so a +// test can tell the cheap read path from the refresh path. +type countingTokenStore struct { + *InMemoryTokenStore + saves int +} + +func (s *countingTokenStore) Save(orgID, providerID string, entry *tokenEntry) error { + s.saves++ + return s.InMemoryTokenStore.Save(orgID, providerID, entry) +} + +// Tokens that need no refresh must be returned without touching the store. The +// zero-expiry case is the interesting one: it means "never expires", but +// time.Until on it is hugely negative, so a naive deadline comparison sends +// every call down the refresh path and rewrites the keychain entry for nothing. +func TestGetAccessToken_ReturnsUnexpiredTokenWithoutWriting(t *testing.T) { + cases := map[string]struct { + token *oauth2.Token + want string + wantErr bool + }{ + // What Slack stores for a bot token, and GitHub for a classic one. + "no expiry never expires": { + token: &oauth2.Token{AccessToken: "xoxb-static", TokenType: "bot"}, + want: "xoxb-static", + }, + "expiry far in the future": { + token: &oauth2.Token{AccessToken: "fresh", Expiry: time.Now().Add(time.Hour)}, + want: "fresh", + }, + // The guard above must not swallow tokens that genuinely need a refresh: + // this one is expired with nothing to refresh from, so it has to fail. + "expired with no refresh token": { + token: &oauth2.Token{AccessToken: "stale", Expiry: time.Now().Add(-time.Hour)}, + wantErr: true, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + store := &countingTokenStore{InMemoryTokenStore: NewInMemoryTokenStore()} + m := NewManagerWithStore(&ProvidersConfig{ + Providers: map[string]ProviderConfig{"api": {TokenURL: "https://example.com/token"}}, + }, store) + m.CheckAndUpdateProvider("api", nil) + if err := store.Save("org1", "api", &tokenEntry{token: tc.token}); err != nil { + t.Fatal(err) + } + store.saves = 0 // ignore the setup write + + got, err := m.GetAccessToken(context.Background(), "org1", "api") + if tc.wantErr { + if err == nil { + t.Fatalf("GetAccessToken succeeded with %q, want an error", got) + } + return + } + if err != nil { + t.Fatalf("GetAccessToken failed: %v", err) + } + if got != tc.want { + t.Errorf("token = %q, want %q", got, tc.want) + } + if store.saves != 0 { + t.Errorf("store written %d times, want 0 — the token needed no refresh", store.saves) + } + }) + } +} diff --git a/forge-go/version/version.go b/forge-go/version/version.go index d4b3ffb..c2b2e2b 100644 --- a/forge-go/version/version.go +++ b/forge-go/version/version.go @@ -1,7 +1,7 @@ package version var ( - Version = "0.4.5" + Version = "0.4.6" GitCommit = "none" BuildDate = "unknown" ) diff --git a/forge-python/pyproject.toml b/forge-python/pyproject.toml index 957fdcd..8b1a5b1 100644 --- a/forge-python/pyproject.toml +++ b/forge-python/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "rusticai-forge" -version = "0.4.5" +version = "0.4.6" description = "Python agent wrapper and execution engine for Forge" readme = "README.md" requires-python = ">=3.13,<3.14" diff --git a/forge-python/uv.lock b/forge-python/uv.lock index 61e49aa..b7ef707 100644 --- a/forge-python/uv.lock +++ b/forge-python/uv.lock @@ -837,7 +837,7 @@ wheels = [ [[package]] name = "rusticai-forge" -version = "0.4.5" +version = "0.4.6" source = { editable = "." } dependencies = [ { name = "httpx" },