From 4475e2d18ec245c3190f8dd028d15818343f5a82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Wed, 9 Sep 2026 11:35:46 +0200 Subject: [PATCH 1/3] fix(sdk/go): honor explicitly-set zero option values in oidc defaults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `loginConfig.applyDefaults()` keyed off the zero value rather than set-ness, so it could not distinguish an unset field from one a caller explicitly set to its zero value: - `WithTimeout(0)` was silently replaced by the 2m default. - `WithScopes()` (explicit empty) was replaced by the default scopes, even though `WithScopes` already records `scopesSet`. Switch `applyDefaults` to consult the `*Set` sentinels, add a `timeoutSet` sentinel set by `WithTimeout`, and guard the client-credentials exchange so a zero timeout means "no deadline" instead of creating an already-expired context (matching Login and DeviceFlow). `WithTimeout(0)` now means "no timeout". Unset fields still receive their defaults; non-zero explicit values are unaffected. Signed-off-by: Roland Huß --- sdk/go/openshell/v1/oidc/credentials_auth.go | 8 ++++- sdk/go/openshell/v1/oidc/credentials_test.go | 34 ++++++++++++++++++++ sdk/go/openshell/v1/oidc/options.go | 8 +++-- sdk/go/openshell/v1/oidc/options_test.go | 20 ++++++++++++ 4 files changed, 67 insertions(+), 3 deletions(-) diff --git a/sdk/go/openshell/v1/oidc/credentials_auth.go b/sdk/go/openshell/v1/oidc/credentials_auth.go index 24012c8bf4..2c6d9a0e74 100644 --- a/sdk/go/openshell/v1/oidc/credentials_auth.go +++ b/sdk/go/openshell/v1/oidc/credentials_auth.go @@ -71,7 +71,13 @@ func (a *clientCredentialsAuth) accessTokenForExchange() (string, error) { return accessToken, nil } - exchangeCtx, cancel := context.WithTimeout(context.Background(), a.cfg.timeout) + // A zero timeout means "no deadline"; only bound the exchange when a + // positive timeout was configured (mirrors Login and DeviceFlow). + exchangeCtx := context.Background() + cancel := context.CancelFunc(func() {}) + if a.cfg.timeout > 0 { + exchangeCtx, cancel = context.WithTimeout(exchangeCtx, a.cfg.timeout) + } defer cancel() token, err := exchangeClientCredentials(exchangeCtx, a.cfg, true) if err != nil { diff --git a/sdk/go/openshell/v1/oidc/credentials_test.go b/sdk/go/openshell/v1/oidc/credentials_test.go index 3269c44d40..a2e11d1a80 100644 --- a/sdk/go/openshell/v1/oidc/credentials_test.go +++ b/sdk/go/openshell/v1/oidc/credentials_test.go @@ -308,6 +308,40 @@ func TestClientCredentialsAuthLateFlightReusesCachedToken(t *testing.T) { assert.Equal(t, "cached-token", accessToken) } +// A zero timeout means "no deadline". The exchange context must not be born +// expired, so the token exchange should still succeed. +func TestClientCredentialsAuthZeroTimeoutHasNoDeadline(t *testing.T) { + resetDiscoveryCache() + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/openid-configuration" { + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": server.URL, + "authorization_endpoint": server.URL + "/authorize", + "token_endpoint": server.URL + "/token", + }) + return + } + _, _ = w.Write([]byte(tokenResponseJSON("token", "", 3600))) + })) + t.Cleanup(server.Close) + + auth, err := NewClientCredentialsAuth( + WithIssuer(server.URL), + WithClientID("client"), + WithClientSecret("secret"), + WithTimeout(0), // explicitly no timeout + ) + require.NoError(t, err) + + // The explicit zero timeout must be preserved (not replaced by the default). + require.Zero(t, auth.(*clientCredentialsAuth).cfg.timeout) + + metadata, err := auth.GetRequestMetadata(context.Background()) + require.NoError(t, err) + assert.Equal(t, "Bearer token", metadata["authorization"]) +} + func TestClientCredentialsAuthCancellationDoesNotPoisonSharedExchange(t *testing.T) { resetDiscoveryCache() var server *httptest.Server diff --git a/sdk/go/openshell/v1/oidc/options.go b/sdk/go/openshell/v1/oidc/options.go index d4d856ba4b..141e964be5 100644 --- a/sdk/go/openshell/v1/oidc/options.go +++ b/sdk/go/openshell/v1/oidc/options.go @@ -33,6 +33,7 @@ type loginConfig struct { scopesSet bool callbackPort int timeout time.Duration + timeoutSet bool keyboardFlow bool inMemory bool displayFunc func(verificationURL, userCode string) @@ -48,12 +49,14 @@ type loginConfig struct { // applyDefaults fills in default values for fields that were not set // by any option function. func (c *loginConfig) applyDefaults() { - if len(c.scopes) == 0 { + // Check set-ness, not the zero value, so an explicitly-set empty scope + // list or zero timeout is honored instead of being replaced by defaults. + if !c.scopesSet { // Deep copy to avoid callers mutating the package-level slice. c.scopes = make([]string, len(defaultScopes)) copy(c.scopes, defaultScopes) } - if c.timeout == 0 { + if !c.timeoutSet { c.timeout = defaultTimeout } } @@ -125,6 +128,7 @@ func WithCallbackPort(port int) LoginOption { func WithTimeout(d time.Duration) LoginOption { return func(c *loginConfig) { c.timeout = d + c.timeoutSet = true } } diff --git a/sdk/go/openshell/v1/oidc/options_test.go b/sdk/go/openshell/v1/oidc/options_test.go index 28d17c06b4..8af2df7ff7 100644 --- a/sdk/go/openshell/v1/oidc/options_test.go +++ b/sdk/go/openshell/v1/oidc/options_test.go @@ -82,6 +82,26 @@ func TestWithTimeout(t *testing.T) { assert.Equal(t, 5*time.Minute, cfg.timeout) } +func TestWithScopes_ExplicitEmptyNotOverridden(t *testing.T) { + var cfg loginConfig + WithScopes()(&cfg) // caller explicitly requests no scopes + cfg.applyDefaults() + + // An explicitly-set empty scope list must be honored, not replaced by defaults. + assert.True(t, cfg.scopesSet) + assert.Empty(t, cfg.scopes) +} + +func TestWithTimeout_ExplicitZeroNotOverridden(t *testing.T) { + var cfg loginConfig + WithTimeout(0)(&cfg) // caller explicitly requests no timeout + cfg.applyDefaults() + + // An explicitly-set zero timeout must be honored, not replaced by the default. + assert.True(t, cfg.timeoutSet) + assert.Zero(t, cfg.timeout) +} + func TestWithKeyboardFlow(t *testing.T) { var cfg loginConfig WithKeyboardFlow()(&cfg) From 89a0f9ad52513b4f14e1a28ae57037ba5ad0cceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Fri, 11 Sep 2026 15:29:44 +0200 Subject: [PATCH 2/3] fix(sdk/go): always request the openid scope in interactive oidc flows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Login and DeviceLogin authenticate a user, so their requests must be OpenID Connect ones. Both passed the caller's scopes through verbatim, so WithScopes("profile") produced an authorization request without "openid", and buildAuthURL set the parameter unconditionally, so an explicitly-empty list emitted a bare "scope=". Normalize the scopes for both interactive flows: "openid" is placed first and a caller-supplied duplicate is dropped, with the remaining scopes keeping their order. This mirrors build_scopes in crates/openshell-cli/src/oidc_auth.rs, which the Go SDK did not follow. The client credentials grant is left untouched. It has no user and no ID token, so it keeps sending exactly what the caller asked for, matching build_ci_scopes. Also document the WithTimeout contract: the flows all gate on timeout > 0, so any non-positive duration means "no deadline". Signed-off-by: Roland Huß --- sdk/go/openshell/v1/oidc/credentials_test.go | 102 +++++++++++++------ sdk/go/openshell/v1/oidc/device.go | 3 + sdk/go/openshell/v1/oidc/device_test.go | 78 ++++++++++++++ sdk/go/openshell/v1/oidc/oidc.go | 3 + sdk/go/openshell/v1/oidc/oidc_test.go | 67 ++++++++++++ sdk/go/openshell/v1/oidc/options.go | 46 ++++++++- sdk/go/openshell/v1/oidc/options_test.go | 61 +++++++++++ 7 files changed, 329 insertions(+), 31 deletions(-) diff --git a/sdk/go/openshell/v1/oidc/credentials_test.go b/sdk/go/openshell/v1/oidc/credentials_test.go index a2e11d1a80..b686ed5f27 100644 --- a/sdk/go/openshell/v1/oidc/credentials_test.go +++ b/sdk/go/openshell/v1/oidc/credentials_test.go @@ -308,38 +308,82 @@ func TestClientCredentialsAuthLateFlightReusesCachedToken(t *testing.T) { assert.Equal(t, "cached-token", accessToken) } -// A zero timeout means "no deadline". The exchange context must not be born -// expired, so the token exchange should still succeed. -func TestClientCredentialsAuthZeroTimeoutHasNoDeadline(t *testing.T) { - resetDiscoveryCache() - var server *httptest.Server - server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/.well-known/openid-configuration" { - _ = json.NewEncoder(w).Encode(map[string]string{ - "issuer": server.URL, - "authorization_endpoint": server.URL + "/authorize", - "token_endpoint": server.URL + "/token", - }) - return - } - _, _ = w.Write([]byte(tokenResponseJSON("token", "", 3600))) - })) - t.Cleanup(server.Close) +// A non-positive timeout means "no deadline". The exchange context must not be +// born expired, so the token exchange should still succeed. +func TestClientCredentialsAuthNonPositiveTimeoutHasNoDeadline(t *testing.T) { + for _, timeout := range []time.Duration{0, -1 * time.Second} { + t.Run(timeout.String(), func(t *testing.T) { + resetDiscoveryCache() + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/openid-configuration" { + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": server.URL, + "authorization_endpoint": server.URL + "/authorize", + "token_endpoint": server.URL + "/token", + }) + return + } + _, _ = w.Write([]byte(tokenResponseJSON("token", "", 3600))) + })) + t.Cleanup(server.Close) - auth, err := NewClientCredentialsAuth( - WithIssuer(server.URL), - WithClientID("client"), - WithClientSecret("secret"), - WithTimeout(0), // explicitly no timeout - ) - require.NoError(t, err) + auth, err := NewClientCredentialsAuth( + WithIssuer(server.URL), + WithClientID("client"), + WithClientSecret("secret"), + WithTimeout(timeout), // explicitly no timeout + ) + require.NoError(t, err) - // The explicit zero timeout must be preserved (not replaced by the default). - require.Zero(t, auth.(*clientCredentialsAuth).cfg.timeout) + // The explicit timeout must be preserved (not replaced by the default). + require.Equal(t, timeout, auth.(*clientCredentialsAuth).cfg.timeout) - metadata, err := auth.GetRequestMetadata(context.Background()) - require.NoError(t, err) - assert.Equal(t, "Bearer token", metadata["authorization"]) + metadata, err := auth.GetRequestMetadata(context.Background()) + require.NoError(t, err) + assert.Equal(t, "Bearer token", metadata["authorization"]) + }) + } +} + +// The client credentials grant has no user and no ID token, so "openid" must +// never be forced onto it the way the interactive flows force it. +func TestClientCredentialsScopesAreNotNormalized(t *testing.T) { + tests := []struct { + name string + opts []LoginOption + want []string + }{ + { + name: "unset sends no scopes", + opts: nil, + want: nil, + }, + { + name: "explicit empty sends no scopes", + opts: []LoginOption{WithScopes()}, + want: []string{}, + }, + { + name: "application scopes are sent verbatim", + opts: []LoginOption{WithScopes("sandbox:read", "sandbox:write")}, + want: []string{"sandbox:read", "sandbox:write"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts := append([]LoginOption{ + WithIssuer("https://issuer.example.com"), + WithClientID("client"), + WithClientSecret("secret"), + }, tt.opts...) + + cfg, err := resolveClientCredentialsConfig(opts...) + require.NoError(t, err) + assert.Equal(t, tt.want, cfg.scopes) + }) + } } func TestClientCredentialsAuthCancellationDoesNotPoisonSharedExchange(t *testing.T) { diff --git a/sdk/go/openshell/v1/oidc/device.go b/sdk/go/openshell/v1/oidc/device.go index ed73654181..1c6b1e144c 100644 --- a/sdk/go/openshell/v1/oidc/device.go +++ b/sdk/go/openshell/v1/oidc/device.go @@ -50,6 +50,9 @@ func DeviceLogin(ctx context.Context, opts ...LoginOption) (*oauth2.Token, error opt(cfg) } cfg.applyDefaults() + // DeviceLogin authenticates a user, so the request must be an OIDC one even + // when the caller supplied its own scopes. + cfg.requireOpenIDScope() if _, hasDeadline := ctx.Deadline(); !hasDeadline && cfg.timeout > 0 { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, cfg.timeout) diff --git a/sdk/go/openshell/v1/oidc/device_test.go b/sdk/go/openshell/v1/oidc/device_test.go index 7571de6b82..f5dd450f35 100644 --- a/sdk/go/openshell/v1/oidc/device_test.go +++ b/sdk/go/openshell/v1/oidc/device_test.go @@ -120,6 +120,84 @@ func TestDeviceLogin_Success(t *testing.T) { // TestDeviceLogin_MissingIssuer verifies that DeviceLogin returns // ErrOIDCConfig when the issuer is not provided. +// DeviceLogin authenticates a user, so the device authorization request must +// carry "openid" on the wire even when the caller supplied its own scopes. +func TestDeviceLogin_AlwaysRequestsOpenIDScope(t *testing.T) { + tests := []struct { + name string + opts []LoginOption + want string + }{ + { + name: "unset scopes send the defaults", + opts: nil, + want: "openid profile email", + }, + { + name: "explicit empty still sends openid", + opts: []LoginOption{WithScopes()}, + want: "openid", + }, + { + name: "application scopes gain openid", + opts: []LoginOption{WithScopes("sandbox:read")}, + want: "openid sandbox:read", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resetDiscoveryCache() + + var requestedScope string + var scopeSeen atomic.Bool + mux := http.NewServeMux() + var srv *httptest.Server + + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": srv.URL, + "authorization_endpoint": srv.URL + "/authorize", + "token_endpoint": srv.URL + "/token", + "device_authorization_endpoint": srv.URL + "/device", + }) + }) + mux.HandleFunc("/device", func(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + requestedScope = r.Form.Get("scope") + scopeSeen.Store(true) + _ = json.NewEncoder(w).Encode(map[string]any{ + "device_code": "test-device-code", + "user_code": "ABCD-1234", + "verification_uri": "https://example.com/activate", + "expires_in": 300, + "interval": 1, + }) + }) + mux.HandleFunc("/token", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(tokenResponseJSON("device-access-token", "", 3600))) + }) + + srv = httptest.NewServer(mux) + t.Cleanup(srv.Close) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + opts := append([]LoginOption{ + WithIssuer(srv.URL), + WithClientID("device-client"), + WithDisplayFunc(func(_, _ string) {}), + }, tt.opts...) + + _, err := DeviceLogin(ctx, opts...) + require.NoError(t, err) + require.True(t, scopeSeen.Load(), "device authorization endpoint was not called") + assert.Equal(t, tt.want, requestedScope) + }) + } +} + func TestDeviceLogin_MissingIssuer(t *testing.T) { resetDiscoveryCache() diff --git a/sdk/go/openshell/v1/oidc/oidc.go b/sdk/go/openshell/v1/oidc/oidc.go index 916e31b78c..39d48df052 100644 --- a/sdk/go/openshell/v1/oidc/oidc.go +++ b/sdk/go/openshell/v1/oidc/oidc.go @@ -37,6 +37,9 @@ func Login(ctx context.Context, gatewayName string, opts ...LoginOption) (*oauth opt(cfg) } cfg.applyDefaults() + // Login authenticates a user, so the request must be an OIDC one even when + // the caller supplied its own scopes. + cfg.requireOpenIDScope() // Apply configured timeout if the caller's context has no deadline. if _, hasDeadline := ctx.Deadline(); !hasDeadline && cfg.timeout > 0 { diff --git a/sdk/go/openshell/v1/oidc/oidc_test.go b/sdk/go/openshell/v1/oidc/oidc_test.go index 5e27ab9e9b..3de4c48247 100644 --- a/sdk/go/openshell/v1/oidc/oidc_test.go +++ b/sdk/go/openshell/v1/oidc/oidc_test.go @@ -11,6 +11,7 @@ import ( "net" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "strings" @@ -190,6 +191,72 @@ func TestLogin_KeyboardFlow(t *testing.T) { assert.Equal(t, "login-access-token", diskTok.AccessToken) } +// Login authenticates a user, so the authorization URL must carry "openid" +// even when the caller supplied its own scopes. An explicitly-empty scope +// list must never produce a bare "scope=" parameter. +func TestLogin_AlwaysRequestsOpenIDScope(t *testing.T) { + tests := []struct { + name string + opts []LoginOption + want string + }{ + { + name: "unset scopes send the defaults", + opts: nil, + want: "openid profile email", + }, + { + name: "explicit empty still sends openid", + opts: []LoginOption{WithScopes()}, + want: "openid", + }, + { + name: "application scopes gain openid", + opts: []LoginOption{WithScopes("sandbox:read")}, + want: "openid sandbox:read", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resetDiscoveryCache() + + provider := setupMockProvider(t) + var prompt strings.Builder + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + opts := append([]LoginOption{ + WithIssuer(provider.URL), + WithClientID("test-client"), + WithInMemory(), + WithKeyboardFlow(), + withInput(strings.NewReader("keyboard-auth-code\n")), + withOutput(&prompt), + }, tt.opts...) + + _, err := Login(ctx, "", opts...) + require.NoError(t, err) + + // The keyboard flow prints the authorization URL it would open. + authURL := extractAuthURL(t, prompt.String()) + assert.Equal(t, tt.want, authURL.Query().Get("scope")) + }) + } +} + +// extractAuthURL pulls the authorization URL out of the keyboard-flow prompt. +func extractAuthURL(t *testing.T, prompt string) *url.URL { + t.Helper() + start := strings.Index(prompt, "http") + require.GreaterOrEqual(t, start, 0, "no authorization URL in prompt: %q", prompt) + raw := strings.Fields(prompt[start:])[0] + parsed, err := url.Parse(raw) + require.NoError(t, err) + return parsed +} + // TestLogin_InMemorySkipsPersistence verifies that WithInMemory() // returns a token without writing to disk. func TestLogin_InMemorySkipsPersistence(t *testing.T) { diff --git a/sdk/go/openshell/v1/oidc/options.go b/sdk/go/openshell/v1/oidc/options.go index 141e964be5..d73df2e862 100644 --- a/sdk/go/openshell/v1/oidc/options.go +++ b/sdk/go/openshell/v1/oidc/options.go @@ -11,9 +11,13 @@ import ( "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/gateway" ) +// scopeOpenID is the scope that turns an OAuth2 authorization request into +// an OpenID Connect one. Interactive flows always request it. +const scopeOpenID = "openid" + // defaultScopes are the OIDC scopes requested when no custom scopes // are specified via [WithScopes]. -var defaultScopes = []string{"openid", "profile", "email"} +var defaultScopes = []string{scopeOpenID, "profile", "email"} // defaultTimeout is the maximum duration for interactive login flows // (browser, keyboard, device code) when no custom timeout is set. @@ -61,6 +65,27 @@ func (c *loginConfig) applyDefaults() { } } +// requireOpenIDScope normalizes the scopes for an interactive flow so the +// request is always an OpenID Connect one. "openid" is placed first and any +// caller-supplied duplicate is dropped; the remaining scopes keep their order. +// +// Interactive flows authenticate a user, and the gateway requires a "sub" +// claim on the resulting token, so "openid" is not optional there. Callers +// remain free to request application scopes such as "sandbox:read". The +// client credentials grant has no user and is left untouched. +// +// This mirrors build_scopes in crates/openshell-cli/src/oidc_auth.rs. +func (c *loginConfig) requireOpenIDScope() { + normalized := make([]string, 0, len(c.scopes)+1) + normalized = append(normalized, scopeOpenID) + for _, scope := range c.scopes { + if scope != scopeOpenID { + normalized = append(normalized, scope) + } + } + c.scopes = normalized +} + // LoginOption configures a login attempt. Use the With* functions to // create option values. type LoginOption func(*loginConfig) @@ -107,6 +132,12 @@ func WithAudience(audience string) LoginOption { // WithScopes overrides the default scopes (openid, profile, email). // The provided scopes replace the defaults entirely. +// +// [Login] and [DeviceLogin] always request "openid" in addition to the +// provided scopes, so WithScopes("sandbox:read") sends "openid sandbox:read". +// [ClientCredentials] and [NewClientCredentialsAuth] send exactly what is +// provided, since that grant has no user and no ID token; calling +// WithScopes with no arguments there sends no scope parameter at all. func WithScopes(scopes ...string) LoginOption { return func(c *loginConfig) { c.scopes = make([]string, len(scopes)) @@ -123,8 +154,12 @@ func WithCallbackPort(port int) LoginOption { } } -// WithTimeout sets the maximum duration for interactive login flows. +// WithTimeout sets the maximum duration for a login flow. // The default is 2 minutes. +// +// A non-positive duration (d <= 0) means "no deadline": the flow runs until +// it completes or the caller's context is cancelled. A caller-supplied +// context that already carries a deadline always takes precedence. func WithTimeout(d time.Duration) LoginOption { return func(c *loginConfig) { c.timeout = d @@ -183,6 +218,13 @@ func withInput(r io.Reader) LoginOption { } } +// withOutput overrides the output writer for keyboard flow testing. +func withOutput(w io.Writer) LoginOption { + return func(c *loginConfig) { + c.output = w + } +} + // withGatewayResolver overrides the gateway.LoadConfig function for // testing. This allows tests to inject a fake gateway resolver // without filesystem setup. diff --git a/sdk/go/openshell/v1/oidc/options_test.go b/sdk/go/openshell/v1/oidc/options_test.go index 8af2df7ff7..af23b9cd62 100644 --- a/sdk/go/openshell/v1/oidc/options_test.go +++ b/sdk/go/openshell/v1/oidc/options_test.go @@ -102,6 +102,67 @@ func TestWithTimeout_ExplicitZeroNotOverridden(t *testing.T) { assert.Zero(t, cfg.timeout) } +func TestWithTimeout_NegativeMeansNoDeadline(t *testing.T) { + var cfg loginConfig + WithTimeout(-1 * time.Second)(&cfg) + cfg.applyDefaults() + + // Every flow gates on timeout > 0, so a negative duration means + // "no deadline" exactly as zero does. It is not replaced by the default. + assert.True(t, cfg.timeoutSet) + assert.Negative(t, cfg.timeout) +} + +// Interactive flows authenticate a user, so the request must carry the +// "openid" scope regardless of what the caller asked for. This mirrors +// build_scopes in crates/openshell-cli/src/oidc_auth.rs. +func TestRequireOpenIDScope(t *testing.T) { + tests := []struct { + name string + opts []LoginOption + want []string + }{ + { + name: "unset scopes keep the defaults", + opts: nil, + want: []string{"openid", "profile", "email"}, + }, + { + name: "explicit empty still requests openid", + opts: []LoginOption{WithScopes()}, + want: []string{"openid"}, + }, + { + name: "application scopes gain openid", + opts: []LoginOption{WithScopes("sandbox:read", "sandbox:write")}, + want: []string{"openid", "sandbox:read", "sandbox:write"}, + }, + { + name: "existing openid is not duplicated", + opts: []LoginOption{WithScopes("openid", "profile")}, + want: []string{"openid", "profile"}, + }, + { + name: "openid is normalized to the front", + opts: []LoginOption{WithScopes("profile", "openid", "email")}, + want: []string{"openid", "profile", "email"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var cfg loginConfig + for _, opt := range tt.opts { + opt(&cfg) + } + cfg.applyDefaults() + cfg.requireOpenIDScope() + + assert.Equal(t, tt.want, cfg.scopes) + }) + } +} + func TestWithKeyboardFlow(t *testing.T) { var cfg loginConfig WithKeyboardFlow()(&cfg) From 2dd564c1f8bf500ad80994e0fb94474a81e8cc09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Fri, 11 Sep 2026 15:55:02 +0200 Subject: [PATCH 3/3] fix(sdk/go): honor gateway-configured oidc scopes in interactive flows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A gateway's metadata.json carries oidc_scopes, and the Rust CLI feeds it into every flow it starts (commands/gateway.rs passes metadata.oidc_scopes to both the interactive and the client-credentials paths). The Go SDK read the field in resolveClientCredentialsConfig only, so Login and DeviceLogin silently ignored it and always used the built-in defaults. Resolve gateway scopes in both interactive flows using the same rule the client-credentials path already applies: they fill only genuinely-unset scopes, so an explicit WithScopes still wins. The openid normalization now runs after gateway resolution, so scopes coming from metadata.json are normalized too. Behavior change: a gateway with oidc_scopes set in metadata.json now affects Login and DeviceLogin from the Go SDK, matching the CLI. Signed-off-by: Roland Huß --- sdk/go/openshell/v1/oidc/device.go | 14 ++- sdk/go/openshell/v1/oidc/device_test.go | 151 ++++++++++++++++++------ sdk/go/openshell/v1/oidc/oidc.go | 17 ++- sdk/go/openshell/v1/oidc/oidc_test.go | 71 +++++++++++ 4 files changed, 212 insertions(+), 41 deletions(-) diff --git a/sdk/go/openshell/v1/oidc/device.go b/sdk/go/openshell/v1/oidc/device.go index 1c6b1e144c..9d62b9c4b0 100644 --- a/sdk/go/openshell/v1/oidc/device.go +++ b/sdk/go/openshell/v1/oidc/device.go @@ -50,9 +50,6 @@ func DeviceLogin(ctx context.Context, opts ...LoginOption) (*oauth2.Token, error opt(cfg) } cfg.applyDefaults() - // DeviceLogin authenticates a user, so the request must be an OIDC one even - // when the caller supplied its own scopes. - cfg.requireOpenIDScope() if _, hasDeadline := ctx.Deadline(); !hasDeadline && cfg.timeout > 0 { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, cfg.timeout) @@ -77,8 +74,19 @@ func DeviceLogin(ctx context.Context, opts ...LoginOption) (*oauth2.Token, error } cfg.issuer = gwCfg.OIDCIssuer cfg.clientID = gwCfg.OIDCClientID + // Gateway-configured scopes fill only genuinely-unset scopes, so an + // explicit WithScopes always wins. + if !cfg.scopesSet && gwCfg.OIDCScopes != "" { + cfg.scopes = strings.Fields(gwCfg.OIDCScopes) + cfg.scopesSet = true + } } + // DeviceLogin authenticates a user, so the request must be an OIDC one + // whether the scopes came from the caller, the gateway metadata, or the + // defaults. + cfg.requireOpenIDScope() + // Validate required configuration. if cfg.issuer == "" || cfg.clientID == "" { return nil, fmt.Errorf( diff --git a/sdk/go/openshell/v1/oidc/device_test.go b/sdk/go/openshell/v1/oidc/device_test.go index f5dd450f35..88ba324582 100644 --- a/sdk/go/openshell/v1/oidc/device_test.go +++ b/sdk/go/openshell/v1/oidc/device_test.go @@ -149,37 +149,8 @@ func TestDeviceLogin_AlwaysRequestsOpenIDScope(t *testing.T) { t.Run(tt.name, func(t *testing.T) { resetDiscoveryCache() - var requestedScope string - var scopeSeen atomic.Bool - mux := http.NewServeMux() - var srv *httptest.Server - - mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, _ *http.Request) { - _ = json.NewEncoder(w).Encode(map[string]any{ - "issuer": srv.URL, - "authorization_endpoint": srv.URL + "/authorize", - "token_endpoint": srv.URL + "/token", - "device_authorization_endpoint": srv.URL + "/device", - }) - }) - mux.HandleFunc("/device", func(w http.ResponseWriter, r *http.Request) { - _ = r.ParseForm() - requestedScope = r.Form.Get("scope") - scopeSeen.Store(true) - _ = json.NewEncoder(w).Encode(map[string]any{ - "device_code": "test-device-code", - "user_code": "ABCD-1234", - "verification_uri": "https://example.com/activate", - "expires_in": 300, - "interval": 1, - }) - }) - mux.HandleFunc("/token", func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(tokenResponseJSON("device-access-token", "", 3600))) - }) - - srv = httptest.NewServer(mux) - t.Cleanup(srv.Close) + scope := &capturedScope{} + srv := setupScopeCapturingDeviceProvider(t, scope) ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -192,12 +163,126 @@ func TestDeviceLogin_AlwaysRequestsOpenIDScope(t *testing.T) { _, err := DeviceLogin(ctx, opts...) require.NoError(t, err) - require.True(t, scopeSeen.Load(), "device authorization endpoint was not called") - assert.Equal(t, tt.want, requestedScope) + assert.Equal(t, tt.want, scope.get(t)) }) } } +// DeviceLogin must honor the gateway's configured oidc_scopes the way the +// Rust CLI does (see build_scopes in crates/openshell-cli/src/oidc_auth.rs), +// while an explicit WithScopes still wins. +func TestDeviceLogin_GatewayScopes(t *testing.T) { + tests := []struct { + name string + gatewayScope string + opts []LoginOption + want string + }{ + { + name: "gateway scopes are used when the caller sets none", + gatewayScope: "openid sandbox:read sandbox:write", + want: "openid sandbox:read sandbox:write", + }, + { + name: "gateway scopes gain openid", + gatewayScope: "sandbox:read", + want: "openid sandbox:read", + }, + { + name: "explicit scopes win over gateway scopes", + gatewayScope: "sandbox:read", + opts: []LoginOption{WithScopes("sandbox:admin")}, + want: "openid sandbox:admin", + }, + { + name: "empty gateway scopes fall back to the defaults", + gatewayScope: "", + want: "openid profile email", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resetDiscoveryCache() + + scope := &capturedScope{} + srv := setupScopeCapturingDeviceProvider(t, scope) + + fakeConfig := &gateway.Config{ + Name: "device-gw", + Endpoint: "gateway.example.com:443", + Dir: t.TempDir(), + OIDCIssuer: srv.URL, + OIDCClientID: "gw-device-client", + OIDCScopes: tt.gatewayScope, + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + opts := append([]LoginOption{ + WithGateway("device-gw"), + WithDisplayFunc(func(_, _ string) {}), + withGatewayResolver(func(string) (*gateway.Config, error) { + return fakeConfig, nil + }), + }, tt.opts...) + + _, err := DeviceLogin(ctx, opts...) + require.NoError(t, err) + assert.Equal(t, tt.want, scope.get(t)) + }) + } +} + +// capturedScope records the scope parameter seen by a mock provider. +type capturedScope struct { + value string + seen atomic.Bool +} + +func (c *capturedScope) get(t *testing.T) string { + t.Helper() + require.True(t, c.seen.Load(), "the authorization request was never made") + return c.value +} + +// setupScopeCapturingDeviceProvider serves a device-flow provider that records +// the scope parameter sent to the device authorization endpoint. +func setupScopeCapturingDeviceProvider(t *testing.T, scope *capturedScope) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + var srv *httptest.Server + + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": srv.URL, + "authorization_endpoint": srv.URL + "/authorize", + "token_endpoint": srv.URL + "/token", + "device_authorization_endpoint": srv.URL + "/device", + }) + }) + mux.HandleFunc("/device", func(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + scope.value = r.Form.Get("scope") + scope.seen.Store(true) + _ = json.NewEncoder(w).Encode(map[string]any{ + "device_code": "test-device-code", + "user_code": "ABCD-1234", + "verification_uri": "https://example.com/activate", + "expires_in": 300, + "interval": 1, + }) + }) + mux.HandleFunc("/token", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(tokenResponseJSON("device-access-token", "", 3600))) + }) + + srv = httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv +} + func TestDeviceLogin_MissingIssuer(t *testing.T) { resetDiscoveryCache() diff --git a/sdk/go/openshell/v1/oidc/oidc.go b/sdk/go/openshell/v1/oidc/oidc.go index 39d48df052..a64c04ffde 100644 --- a/sdk/go/openshell/v1/oidc/oidc.go +++ b/sdk/go/openshell/v1/oidc/oidc.go @@ -9,6 +9,7 @@ import ( "io" "os" "slices" + "strings" "golang.org/x/oauth2" @@ -37,9 +38,6 @@ func Login(ctx context.Context, gatewayName string, opts ...LoginOption) (*oauth opt(cfg) } cfg.applyDefaults() - // Login authenticates a user, so the request must be an OIDC one even when - // the caller supplied its own scopes. - cfg.requireOpenIDScope() // Apply configured timeout if the caller's context has no deadline. if _, hasDeadline := ctx.Deadline(); !hasDeadline && cfg.timeout > 0 { @@ -53,6 +51,9 @@ func Login(ctx context.Context, gatewayName string, opts ...LoginOption) (*oauth if err != nil { return nil, err } + // Login authenticates a user, so the request must be an OIDC one whether the + // scopes came from the caller, the gateway metadata, or the defaults. + cfg.requireOpenIDScope() // FR-019: Check for existing valid token on disk before starting // an interactive flow. @@ -118,8 +119,8 @@ func Login(ctx context.Context, gatewayName string, opts ...LoginOption) (*oauth return tok, nil } -// resolveOIDCConfig resolves the OIDC issuer and client ID either from -// the gateway metadata or from explicit options. Returns the token +// resolveOIDCConfig resolves the OIDC issuer, client ID, and scopes either +// from the gateway metadata or from explicit options. Returns the token // directory path (empty if in-memory or no directory available). func resolveOIDCConfig(cfg *loginConfig, gatewayName string) (string, error) { tokenDir := cfg.tokenDir @@ -139,6 +140,12 @@ func resolveOIDCConfig(cfg *loginConfig, gatewayName string) (string, error) { } cfg.issuer = gwCfg.OIDCIssuer cfg.clientID = gwCfg.OIDCClientID + // Gateway-configured scopes fill only genuinely-unset scopes, so an + // explicit WithScopes always wins. + if !cfg.scopesSet && gwCfg.OIDCScopes != "" { + cfg.scopes = strings.Fields(gwCfg.OIDCScopes) + cfg.scopesSet = true + } if tokenDir == "" { tokenDir = gwCfg.Dir } diff --git a/sdk/go/openshell/v1/oidc/oidc_test.go b/sdk/go/openshell/v1/oidc/oidc_test.go index 3de4c48247..46f49c1069 100644 --- a/sdk/go/openshell/v1/oidc/oidc_test.go +++ b/sdk/go/openshell/v1/oidc/oidc_test.go @@ -246,6 +246,77 @@ func TestLogin_AlwaysRequestsOpenIDScope(t *testing.T) { } } +// Login must honor the gateway's configured oidc_scopes the way the Rust CLI +// does (see commands/gateway.rs, which passes metadata.oidc_scopes into the +// interactive flow), while an explicit WithScopes still wins. +func TestLogin_GatewayScopes(t *testing.T) { + tests := []struct { + name string + gatewayScope string + opts []LoginOption + want string + }{ + { + name: "gateway scopes are used when the caller sets none", + gatewayScope: "openid sandbox:read sandbox:write", + want: "openid sandbox:read sandbox:write", + }, + { + name: "gateway scopes gain openid", + gatewayScope: "sandbox:read", + want: "openid sandbox:read", + }, + { + name: "explicit scopes win over gateway scopes", + gatewayScope: "sandbox:read", + opts: []LoginOption{WithScopes("sandbox:admin")}, + want: "openid sandbox:admin", + }, + { + name: "empty gateway scopes fall back to the defaults", + gatewayScope: "", + want: "openid profile email", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resetDiscoveryCache() + + provider := setupMockProvider(t) + var prompt strings.Builder + + fakeConfig := &gateway.Config{ + Name: "login-gw", + Endpoint: "gateway.example.com:443", + Dir: t.TempDir(), + OIDCIssuer: provider.URL, + OIDCClientID: "gw-login-client", + OIDCScopes: tt.gatewayScope, + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + opts := append([]LoginOption{ + WithInMemory(), + WithKeyboardFlow(), + withInput(strings.NewReader("keyboard-auth-code\n")), + withOutput(&prompt), + withGatewayResolver(func(string) (*gateway.Config, error) { + return fakeConfig, nil + }), + }, tt.opts...) + + _, err := Login(ctx, "login-gw", opts...) + require.NoError(t, err) + + authURL := extractAuthURL(t, prompt.String()) + assert.Equal(t, tt.want, authURL.Query().Get("scope")) + }) + } +} + // extractAuthURL pulls the authorization URL out of the keyboard-flow prompt. func extractAuthURL(t *testing.T, prompt string) *url.URL { t.Helper()