Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion sdk/go/openshell/v1/oidc/credentials_auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Comment on lines +74 to +80

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does context.WithTimeout() interpret a 0 value as no deadline, or does it explicitly need this handling?

@rhuss rhuss Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context.WithTimeout(parent, 0) doesn't mean "no deadline". It's defined as WithDeadline(parent, time.Now().Add(timeout)), so a 0 (or negative) timeout sets the deadline to now and the context is born already-expired; any call using it returns context.DeadlineExceeded immediately. So the explicit timeout > 0 guard is needed to actually get "no deadline" behavior, and it mirrors what Login and DeviceFlow already do. Without it, WithTimeout(0) would break the client-credentials exchange instead of disabling the timeout (covered by TestClientCredentialsAuthZeroTimeoutHasNoDeadline).

defer cancel()
token, err := exchangeClientCredentials(exchangeCtx, a.cfg, true)
if err != nil {
Expand Down
34 changes: 34 additions & 0 deletions sdk/go/openshell/v1/oidc/credentials_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions sdk/go/openshell/v1/oidc/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}
}
Expand Down Expand Up @@ -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
}
}

Expand Down
20 changes: 20 additions & 0 deletions sdk/go/openshell/v1/oidc/options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading