Copilot commented on code in PR #1441:
URL: https://github.com/apache/pulsar-client-go/pull/1441#discussion_r2564487567
##########
pulsaradmin/pkg/admin/auth/oauth2.go:
##########
@@ -130,44 +97,28 @@ func NewAuthenticationOAuth2WithParams(
Audience: audience,
}
- keyringStore, err := MakeKeyringStore()
+ flow, err :=
oauth2.NewDefaultClientCredentialsFlow(oauth2.ClientCredentialsFlowOptions{})
Review Comment:
Creating a flow with empty `ClientCredentialsFlowOptions{}` will cause the
flow to have no KeyFile path. When `GetGrant()` is later called on this flow,
it will fail because `NewClientCredentialsProviderFromKeyFile("")` with an
empty string will not be able to load credentials. This function appears to be
incomplete - it should either populate the KeyFile in the options or use a
different approach to initialize the flow for this use case.
```suggestion
flow, err :=
oauth2.NewDefaultClientCredentialsFlow(oauth2.ClientCredentialsFlowOptions{
KeyFile: paramsJSON.PrivateKey,
})
```
##########
pulsar/auth/oauth2_test.go:
##########
@@ -142,3 +161,73 @@ func TestNewAuthenticationOAuth2WithParams(t *testing.T) {
assert.Equal(t, "token-content", string(token))
}
}
+
+func TestOAuth2KeyFileReloading(t *testing.T) {
+ server := mockOAuthServer()
+ defer server.Close()
+ expectedClientID.Store("client-id")
+ expectedClientSecret.Store("client-secret")
+ kf, err := mockKeyFile(server.URL)
+ defer os.Remove(kf)
+ require.NoError(t, err)
+
+ params := map[string]string{
+ ConfigParamType: ConfigParamTypeClientCredentials,
+ ConfigParamIssuerURL: server.URL,
+ ConfigParamClientID: "client-id",
+ ConfigParamAudience: "audience",
+ ConfigParamKeyFile: fmt.Sprintf("file://%s", kf),
+ ConfigParamScope: "profile",
+ }
+
+ auth, err := NewAuthenticationOAuth2WithParams(params)
+ require.NoError(t, err)
+ err = auth.Init()
+ require.NoError(t, err)
+
+ token, err := auth.GetData()
+ require.NoError(t, err)
+ assert.Equal(t, "token-content", string(token))
+
+ expectedClientSecret.Store("new-client-secret")
Review Comment:
After changing `expectedClientSecret` on line 192, the next `GetData()` call
on line 193 will likely return the cached token from line 188-190 without
triggering a refresh (since the token hasn't expired yet). This means the test
may not actually verify that token refresh fails with incorrect credentials. To
properly test the key file reloading behavior, you should either:
1. Invalidate the cached token before line 193 to force a refresh
2. Wait for the token to expire
3. Explicitly call a method that triggers token refresh
Consider adding something like:
```go
// Force token invalidation to trigger refresh on next GetData()
auth.(*oauth2AuthProvider).source.InvalidateToken()
```
```suggestion
expectedClientSecret.Store("new-client-secret")
// Force token invalidation to trigger refresh on next GetData()
auth.(*oauth2AuthProvider).source.InvalidateToken()
```
##########
oauth2/client_credentials_flow.go:
##########
@@ -47,29 +46,20 @@ type ClientCredentialsExchanger interface {
ExchangeClientCredentials(req ClientCredentialsExchangeRequest)
(*TokenResult, error)
}
Review Comment:
The `GrantProvider` interface is missing a documentation comment. Public
interfaces should have GoDoc comments explaining their purpose. Consider adding:
```go
// GrantProvider abstracts the creation of authorization grants from
credentials
type GrantProvider interface {
GetGrant(audience string, options *ClientCredentialsFlowOptions)
(*AuthorizationGrant, error)
}
```
```suggestion
// GrantProvider abstracts the creation of authorization grants from
credentials
```
##########
pulsar/auth/oauth2_test.go:
##########
@@ -44,7 +50,17 @@ func mockOAuthServer() *httptest.Server {
}`, server.URL, server.URL, server.URL, server.URL)
fmt.Fprintln(writer, s)
})
- mockedHandler.HandleFunc("/oauth/token", func(writer
http.ResponseWriter, _ *http.Request) {
+ mockedHandler.HandleFunc("/oauth/token", func(writer
http.ResponseWriter, r *http.Request) {
+ if err := r.ParseForm(); err != nil {
+ http.Error(writer, "invalid form",
http.StatusBadRequest)
+ return
+ }
+ clientID := r.FormValue("client_id")
+ clientSecret := r.FormValue("client_secret")
+ if clientID != expectedClientID.Load().(string) || clientSecret
!= expectedClientSecret.Load().(string) {
Review Comment:
The type assertion `expectedClientID.Load().(string)` will panic if
`expectedClientID` hasn't been initialized yet. If tests run in parallel or if
`mockOAuthServer()` is called before a test initializes the atomic values, this
will crash. Consider adding a nil check or ensuring atomic values are
initialized in the server setup:
```go
if val := expectedClientID.Load(); val != nil {
clientID := r.FormValue("client_id")
clientSecret := r.FormValue("client_secret")
if clientID != val.(string) || clientSecret !=
expectedClientSecret.Load().(string) {
http.Error(writer, "invalid client credentials",
http.StatusUnauthorized)
return
}
}
```
```suggestion
expectedID := expectedClientID.Load()
expectedSecret := expectedClientSecret.Load()
if expectedID == nil || expectedSecret == nil {
http.Error(writer, "server misconfiguration: expected
client credentials not set", http.StatusInternalServerError)
return
}
if clientID != expectedID.(string) || clientSecret !=
expectedSecret.(string) {
```
##########
oauth2/client_credentials_flow.go:
##########
@@ -47,29 +46,20 @@ type ClientCredentialsExchanger interface {
ExchangeClientCredentials(req ClientCredentialsExchangeRequest)
(*TokenResult, error)
}
+type GrantProvider interface {
+ GetGrant(audience string, options *ClientCredentialsFlowOptions)
(*AuthorizationGrant, error)
+}
+
type ClientCredentialsFlowOptions struct {
KeyFile string
AdditionalScopes []string
}
-func newClientCredentialsFlow(
- options ClientCredentialsFlowOptions,
- keyfile *KeyFile,
- oidcWellKnownEndpoints OIDCWellKnownEndpoints,
- exchanger ClientCredentialsExchanger,
- clock clock.Clock) *ClientCredentialsFlow {
- return &ClientCredentialsFlow{
- options: options,
- oidcWellKnownEndpoints: oidcWellKnownEndpoints,
- keyfile: keyfile,
- exchanger: exchanger,
- clock: clock,
- }
+type DefaultGrantProvider struct {
}
Review Comment:
The `GetGrant` method is missing a documentation comment. Public methods on
exported types should have GoDoc comments. Consider adding:
```go
// GetGrant creates an authorization grant by loading credentials from the
key file and
// merging the scopes from both the options and the key file configuration
func (p *DefaultGrantProvider) GetGrant(audience string, options
*ClientCredentialsFlowOptions) (
*AuthorizationGrant, error) {
```
```suggestion
// GetGrant creates an authorization grant by loading credentials from the
key file and
// merging the scopes from both the options and the key file configuration.
```
##########
oauth2/client_credentials_flow.go:
##########
@@ -80,39 +70,58 @@ func NewDefaultClientCredentialsFlow(options
ClientCredentialsFlowOptions) (*Cli
if err != nil {
return nil, err
}
+ // Merge the scopes of the options AdditionalScopes with the scopes
read from the keyFile config
+ var scopesToAdd []string
+ if len(options.AdditionalScopes) > 0 {
+ scopesToAdd = append(scopesToAdd, options.AdditionalScopes...)
+ }
+
+ if keyFile.Scope != "" {
+ scopesSplit := strings.Split(keyFile.Scope, " ")
Review Comment:
Using `strings.Split(keyFile.Scope, " ")` will create empty string elements
if there are multiple consecutive spaces in the scope string. This could lead
to invalid scope values being sent to the OAuth server. Consider using
`strings.Fields(keyFile.Scope)` instead, which handles multiple whitespace
characters correctly and filters out empty strings:
```go
scopesSplit := strings.Fields(keyFile.Scope)
scopesToAdd = append(scopesToAdd, scopesSplit...)
```
```suggestion
scopesSplit := strings.Fields(keyFile.Scope)
```
##########
oauth2/client_credentials_flow.go:
##########
@@ -47,29 +46,20 @@ type ClientCredentialsExchanger interface {
ExchangeClientCredentials(req ClientCredentialsExchangeRequest)
(*TokenResult, error)
}
+type GrantProvider interface {
+ GetGrant(audience string, options *ClientCredentialsFlowOptions)
(*AuthorizationGrant, error)
+}
+
type ClientCredentialsFlowOptions struct {
KeyFile string
AdditionalScopes []string
}
Review Comment:
The `DefaultGrantProvider` struct is missing a documentation comment. Public
types should have GoDoc comments. Consider adding:
```go
// DefaultGrantProvider provides authorization grants by loading credentials
from a key file
type DefaultGrantProvider struct {
}
```
```suggestion
// DefaultGrantProvider provides authorization grants by loading credentials
from a key file
```
--
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]
