Copilot commented on code in PR #1463:
URL: https://github.com/apache/pulsar-client-go/pull/1463#discussion_r2734781716
##########
oauth2/client_credentials_flow.go:
##########
@@ -70,7 +71,18 @@ func (p *DefaultGrantProvider) GetGrant(audience string,
options *ClientCredenti
return nil, errors.Wrap(err, "could not get client credentials")
}
- wellKnownEndpoints, err :=
GetOIDCWellKnownEndpointsFromIssuerURL(keyFile.IssuerURL)
+ issuerURL := ""
+ if options != nil {
+ issuerURL = options.IssuerURL
+ }
+ if issuerURL == "" {
+ issuerURL = keyFile.IssuerURL
+ }
+ if issuerURL == "" {
+ return nil, errors.New("issuer url is required for client
credentials flow")
+ }
+
+ wellKnownEndpoints, err :=
GetOIDCWellKnownEndpointsFromIssuerURL(issuerURL)
if err != nil {
Review Comment:
Behavior was added here to (1) prefer
`ClientCredentialsFlowOptions.IssuerURL` over the key file issuer and (2) error
when neither is provided. The oauth2 package has a dedicated
`client_credentials_flow_test.go` suite, but this new branch in
`DefaultGrantProvider.GetGrant` isn’t covered there. Please add unit tests in
`oauth2/client_credentials_flow_test.go` (or a new test file) to cover issuer
override precedence and the missing-issuer error path.
##########
pulsaradmin/pkg/admin/auth/oauth2.go:
##########
@@ -52,7 +52,8 @@ type OAuth2Provider struct {
// NewAuthenticationOAuth2WithDefaultFlow uses memory to save the grant
func NewAuthenticationOAuth2WithDefaultFlow(issuer oauth2.Issuer, keyFile
string) (Provider, error) {
return NewAuthenticationOAuth2WithFlow(issuer,
oauth2.ClientCredentialsFlowOptions{
- KeyFile: keyFile,
+ KeyFile: keyFile,
+ IssuerURL: issuer.IssuerEndpoint,
})
Review Comment:
The PR description is still the unfilled template (placeholders like `Fixes
#<xyz>`, `Master Issue: #<xyz>`, and empty sections). Please update the PR
description to include the actual issue link (or remove the placeholders) and a
brief motivation/verification summary so reviewers have context.
##########
oauth2/client_credentials_flow.go:
##########
@@ -70,7 +71,18 @@ func (p *DefaultGrantProvider) GetGrant(audience string,
options *ClientCredenti
return nil, errors.Wrap(err, "could not get client credentials")
}
- wellKnownEndpoints, err :=
GetOIDCWellKnownEndpointsFromIssuerURL(keyFile.IssuerURL)
+ issuerURL := ""
+ if options != nil {
+ issuerURL = options.IssuerURL
+ }
+ if issuerURL == "" {
Review Comment:
The new `options != nil` guard suggests `GetGrant` can handle a nil
`options`, but the function dereferences `options` earlier (e.g., for
`options.KeyFile` / `options.AdditionalScopes`). This means a nil `options`
would still panic and the nil check is ineffective. Consider either making
`options` a non-pointer parameter, or returning a clear error immediately when
`options == nil` before any dereference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
