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..b686ed5f27 100644 --- a/sdk/go/openshell/v1/oidc/credentials_test.go +++ b/sdk/go/openshell/v1/oidc/credentials_test.go @@ -308,6 +308,84 @@ func TestClientCredentialsAuthLateFlightReusesCachedToken(t *testing.T) { assert.Equal(t, "cached-token", accessToken) } +// 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(timeout), // explicitly no timeout + ) + require.NoError(t, err) + + // 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"]) + }) + } +} + +// 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) { resetDiscoveryCache() var server *httptest.Server diff --git a/sdk/go/openshell/v1/oidc/device.go b/sdk/go/openshell/v1/oidc/device.go index ed73654181..9d62b9c4b0 100644 --- a/sdk/go/openshell/v1/oidc/device.go +++ b/sdk/go/openshell/v1/oidc/device.go @@ -74,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 7571de6b82..88ba324582 100644 --- a/sdk/go/openshell/v1/oidc/device_test.go +++ b/sdk/go/openshell/v1/oidc/device_test.go @@ -120,6 +120,169 @@ 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() + + scope := &capturedScope{} + srv := setupScopeCapturingDeviceProvider(t, scope) + + 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) + 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 916e31b78c..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" @@ -50,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. @@ -115,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 @@ -136,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 5e27ab9e9b..46f49c1069 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,143 @@ 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")) + }) + } +} + +// 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() + 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 d4d856ba4b..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. @@ -33,6 +37,7 @@ type loginConfig struct { scopesSet bool callbackPort int timeout time.Duration + timeoutSet bool keyboardFlow bool inMemory bool displayFunc func(verificationURL, userCode string) @@ -48,16 +53,39 @@ 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 } } +// 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) @@ -104,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)) @@ -120,11 +154,16 @@ 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 + c.timeoutSet = true } } @@ -179,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 28d17c06b4..af23b9cd62 100644 --- a/sdk/go/openshell/v1/oidc/options_test.go +++ b/sdk/go/openshell/v1/oidc/options_test.go @@ -82,6 +82,87 @@ 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 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)