Merged
Conversation
…ONDUCTORONE_OIDC_TOKEN
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds host normalization/resolution, new client entry points (access-token and OIDC exchange), a token-exchange oauth2.TokenSource implementation (RFC 8693), and simplifies NewC1TokenSource to return only an oauth2.TokenSource. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant NewWithOIDCToken
participant TokenExchangeSource
participant TokenEndpoint
participant HTTPClient
participant SDKClient
Caller->>NewWithOIDCToken: Call with OIDC token, clientID, tokenHost
NewWithOIDCToken->>NewWithOIDCToken: Resolve server host / validate inputs
NewWithOIDCToken->>TokenExchangeSource: NewTokenExchangeSource(ctx, oidcToken, clientID, tokenHost)
TokenExchangeSource->>HTTPClient: Build HTTP client (UA, logging, debug)
TokenExchangeSource->>TokenEndpoint: POST token exchange (subject_token)
TokenEndpoint-->>TokenExchangeSource: Return access_token + expiry
TokenExchangeSource-->>NewWithOIDCToken: oauth2.TokenSource (reuse wrapper)
NewWithOIDCToken->>SDKClient: newClientWithTokenSource(tokenSource, host, clientName)
SDKClient-->>Caller: Initialized authenticated client
sequenceDiagram
participant Caller
participant NewWithAccessToken
participant newClientWithTokenSource
participant HTTPClient
participant SDKClient
Caller->>NewWithAccessToken: Call with access token, clientID
NewWithAccessToken->>NewWithAccessToken: Resolve server host from clientID/env
NewWithAccessToken->>newClientWithTokenSource: oauth2.StaticTokenSource(token)
newClientWithTokenSource->>HTTPClient: Build HTTP client with token source
newClientWithTokenSource->>SDKClient: Initialize SDK client with server URL & UA
SDKClient-->>Caller: Return authenticated client
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@pkg/client/client.go`:
- Around line 173-185: NewWithOIDCToken must validate inputs early: check that
oidcToken and clientID are non-empty at the top of the function and return a
clear error (e.g., fmt.Errorf) if either is missing, before calling
parseClientID or NewTokenExchangeSource; this prevents downstream token-exchange
failures and gives a fast, descriptive failure point referencing
NewWithOIDCToken, oidcToken, clientID, parseClientID, and
NewTokenExchangeSource.
- Around line 138-147: The NewWithAccessToken function currently proceeds even
when accessToken is empty, causing confusing auth failures later; update
NewWithAccessToken to validate the accessToken argument at the start (check that
accessToken is non-empty) and return a clear error immediately if it's empty
(include the function name NewWithAccessToken and parameter accessToken in the
error message) so callers get immediate, actionable feedback instead of creating
a client with an invalid token.
- Around line 149-167: The tokenHost value may contain a scheme which will
produce invalid base URLs later; update the logic around tokenHost (used with
parseClientID and v.GetString("api-endpoint")) to normalize it by parsing with
url.Parse and extracting the Host (or Path for host-only inputs), stripping any
trailing slashes, and falling back to the original value if parsing fails;
ensure the error message using EnvAccessToken and EnvClientID remains unchanged.
In `@pkg/client/token_exchange.go`:
- Around line 19-22: The two OAuth2 URN constants grantTypeTokenExchange and
subjectTokenTypeJWT are being flagged by gosec G101 as false positives; add
explicit gosec suppression comments (e.g., append `// `#nosec` G101` or a scoped
Gosec suppression) directly above or on the same line as each constant
declaration to silence the false positive while keeping the constants unchanged,
ensuring CI stops failing for these known non-credential URNs.
- Around line 33-57: The Token() method on tokenExchangeSource currently uses
context.Background() and the http client may lack a total request timeout;
update Token() to accept/use the caller context (use the ctx passed into the
tokenExchangeSource constructor or a context param) when creating the request
(replace context.Background() with that context) so cancellations and deadlines
propagate, and ensure the http.Client stored on tokenExchangeSource has an
explicit Timeout set in the constructor (set http.Client.Timeout when creating
t.httpClient) so the token exchange cannot hang indefinitely while reading the
response body; adjust any constructor that builds t.httpClient (e.g., where
uhttp.NewClient is invoked) to accept the ctx and set the Timeout accordingly.
pkg/client/client.go
Outdated
Comment on lines
+138
to
+147
| func NewWithAccessToken( | ||
| ctx context.Context, | ||
| accessToken string, | ||
| clientID string, | ||
| v *viper.Viper, | ||
| cmdName string, | ||
| ) (C1Client, error) { | ||
| tokenSrc := oauth2.StaticTokenSource(&oauth2.Token{ | ||
| AccessToken: accessToken, | ||
| }) |
There was a problem hiding this comment.
Reject empty access tokens early.
This currently builds a client even if accessToken is empty, leading to confusing auth errors later.
🛠️ Proposed fix
func NewWithAccessToken(
ctx context.Context,
accessToken string,
clientID string,
v *viper.Viper,
cmdName string,
) (C1Client, error) {
+ if accessToken == "" {
+ return nil, fmt.Errorf("%s is required", EnvAccessToken)
+ }
tokenSrc := oauth2.StaticTokenSource(&oauth2.Token{
AccessToken: accessToken,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func NewWithAccessToken( | |
| ctx context.Context, | |
| accessToken string, | |
| clientID string, | |
| v *viper.Viper, | |
| cmdName string, | |
| ) (C1Client, error) { | |
| tokenSrc := oauth2.StaticTokenSource(&oauth2.Token{ | |
| AccessToken: accessToken, | |
| }) | |
| func NewWithAccessToken( | |
| ctx context.Context, | |
| accessToken string, | |
| clientID string, | |
| v *viper.Viper, | |
| cmdName string, | |
| ) (C1Client, error) { | |
| if accessToken == "" { | |
| return nil, fmt.Errorf("%s is required", EnvAccessToken) | |
| } | |
| tokenSrc := oauth2.StaticTokenSource(&oauth2.Token{ | |
| AccessToken: accessToken, | |
| }) |
🤖 Prompt for AI Agents
In `@pkg/client/client.go` around lines 138 - 147, The NewWithAccessToken function
currently proceeds even when accessToken is empty, causing confusing auth
failures later; update NewWithAccessToken to validate the accessToken argument
at the start (check that accessToken is non-empty) and return a clear error
immediately if it's empty (include the function name NewWithAccessToken and
parameter accessToken in the error message) so callers get immediate, actionable
feedback instead of creating a client with an invalid token.
Comment on lines
+33
to
+57
| func (t *tokenExchangeSource) Token() (*oauth2.Token, error) { | ||
| body := url.Values{ | ||
| "grant_type": []string{grantTypeTokenExchange}, | ||
| "subject_token": []string{t.oidcToken}, | ||
| "subject_token_type": []string{subjectTokenTypeJWT}, | ||
| "client_id": []string{t.clientID}, | ||
| } | ||
|
|
||
| tokenURL := url.URL{ | ||
| Scheme: "https", | ||
| Host: t.tokenHost, | ||
| Path: "auth/v1/token", | ||
| } | ||
|
|
||
| req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, tokenURL.String(), strings.NewReader(body.Encode())) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | ||
|
|
||
| resp, err := t.httpClient.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "NewClient|NewTransport|Timeout|ResponseHeaderTimeout|IdleConnTimeout" pkg/uhttp -C 3Repository: ConductorOne/cone
Length of output: 1939
🏁 Script executed:
cat -n pkg/uhttp/client.go | head -130Repository: ConductorOne/cone
Length of output: 3153
Propagate caller context and ensure timeout for token exchange requests.
Token() uses context.Background(), ignoring caller cancellations/timeouts. Additionally, uhttp.NewClient does not set http.Client.Timeout, only transport-level phase timeouts (dial, TLS handshake). This means requests can hang indefinitely while reading the response body. Wire the constructor's ctx through and add an explicit timeout to the http.Client.
🛠️ Proposed fix
type tokenExchangeSource struct {
+ ctx context.Context
oidcToken string
clientID string
tokenHost string
httpClient *http.Client
}
func (t *tokenExchangeSource) Token() (*oauth2.Token, error) {
+ ctx := t.ctx
+ if ctx == nil {
+ ctx = context.Background()
+ }
body := url.Values{
"grant_type": []string{grantTypeTokenExchange},
"subject_token": []string{t.oidcToken},
"subject_token_type": []string{subjectTokenTypeJWT},
"client_id": []string{t.clientID},
}
tokenURL := url.URL{
Scheme: "https",
Host: t.tokenHost,
Path: "auth/v1/token",
}
- req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, tokenURL.String(), strings.NewReader(body.Encode()))
+ req, err := http.NewRequestWithContext(ctx, http.MethodPost, tokenURL.String(), strings.NewReader(body.Encode()))
if err != nil {
return nil, err
} func NewTokenExchangeSource(ctx context.Context, oidcToken, clientID, tokenHost string, debug bool) (oauth2.TokenSource, error) {
+ if ctx == nil {
+ ctx = context.Background()
+ }
httpClient, err := uhttp.NewClient(ctx,
uhttp.WithLogger(true, ctxzap.Extract(ctx)),
uhttp.WithUserAgent("cone-wfe-credential-provider"),
uhttp.WithDebug(debug),
)
if err != nil {
return nil, err
}
+ httpClient.Timeout = 30 * time.Second
return oauth2.ReuseTokenSource(nil, &tokenExchangeSource{
+ ctx: ctx,
oidcToken: oidcToken,
clientID: clientID,
tokenHost: tokenHost,
httpClient: httpClient,
}), nil
}🤖 Prompt for AI Agents
In `@pkg/client/token_exchange.go` around lines 33 - 57, The Token() method on
tokenExchangeSource currently uses context.Background() and the http client may
lack a total request timeout; update Token() to accept/use the caller context
(use the ctx passed into the tokenExchangeSource constructor or a context param)
when creating the request (replace context.Background() with that context) so
cancellations and deadlines propagate, and ensure the http.Client stored on
tokenExchangeSource has an explicit Timeout set in the constructor (set
http.Client.Timeout when creating t.httpClient) so the token exchange cannot
hang indefinitely while reading the response body; adjust any constructor that
builds t.httpClient (e.g., where uhttp.NewClient is invoked) to accept the ctx
and set the Timeout accordingly.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/client/client.go (1)
199-217:⚠️ Potential issue | 🟠 MajorFail fast when server host cannot be resolved in New().
ResolveServerHostcan return an empty host (e.g., missing client ID and api-endpoint). That currently flows into client creation, yielding invalid base URLs later. Add a guard like the other entry points.🛠️ Proposed fix
func New( ctx context.Context, clientId string, clientSecret string, v *viper.Viper, cmdName string, ) (C1Client, error) { clientName, tokenHost, err := ResolveServerHost(clientId, v) if err != nil { return nil, err } + if tokenHost == "" { + return nil, fmt.Errorf("%s requires --client-id, %s, or --api-endpoint to determine the server", EnvClientSecret, EnvClientID) + } tokenSrc, err := NewC1TokenSource(ctx, clientId, clientSecret, tokenHost, v.GetBool("debug")) if err != nil { return nil, err }
robert-chiniquy
approved these changes
Feb 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Support
CONDUCTORONE_ACCESS_TOKENandCONDUCTORONE_OIDC_TOKENSummary by CodeRabbit