Copilot commented on code in PR #1441:
URL: https://github.com/apache/pulsar-client-go/pull/1441#discussion_r2564301378
##########
pulsaradmin/pkg/admin/auth/oauth2.go:
##########
@@ -130,44 +97,22 @@ func NewAuthenticationOAuth2WithParams(
Audience: audience,
}
- keyringStore, err := MakeKeyringStore()
- if err != nil {
- return nil, err
- }
-
p := &OAuth2Provider{
clock: clock2.RealClock{},
issuer: issuer,
- store: keyringStore,
defaultTransport: transport,
}
Review Comment:
The `flow` field is nil when `NewAuthenticationOAuth2WithParams` is called,
but `initCache()` requires a non-nil flow. This will cause a nil pointer
dereference when the cache tries to use the flow.
This function should initialize the flow similar to how
`NewAuthenticationOAuth2WithFlow` does, or it should be removed if it's no
longer used.
##########
oauth2/cache/cache.go:
##########
@@ -77,56 +71,21 @@ func (t *tokenCache) Token() (*xoauth2.Token, error) {
return t.token, nil
}
- // load from the store and use the access token if it isn't expired
- grant, err := t.store.LoadGrant(t.audience)
+ grant, err := t.flow.Authorize(t.audience)
if err != nil {
- return nil, fmt.Errorf("LoadGrant: %w", err)
+ return nil, err
}
t.token = grant.Token
- if t.token != nil && t.validateAccessToken(*t.token) {
- return t.token, nil
- }
-
- // obtain and cache a fresh access token
- grant, err = t.refresher.Refresh(grant)
- if err != nil {
- return nil, fmt.Errorf("RefreshGrant: %w", err)
- }
- t.token = grant.Token
- err = t.store.SaveGrant(t.audience, *grant)
- if err != nil {
- // TODO log rather than throw
- return nil, fmt.Errorf("SaveGrant: %w", err)
- }
return t.token, nil
}
// InvalidateToken clears the access token (likely due to a response from the
resource server).
Review Comment:
The comment is now inaccurate. The old comment mentioned "Note that the
token within the grant may contain a refresh token which should survive," but
the new implementation simply clears the entire token without persisting
anything.
Consider updating it to:
```go
// InvalidateToken clears the cached access token (likely due to a response
from the resource server).
// The next call to Token() will obtain a fresh token by calling the flow's
Authorize method.
```
```suggestion
// InvalidateToken clears the cached access token (likely due to a response
from the resource server).
// The next call to Token() will obtain a fresh token by calling the flow's
Authorize method.
```
##########
pulsaradmin/pkg/admin/auth/oauth2_test.go:
##########
@@ -99,37 +98,16 @@ func TestOauth2(t *testing.T) {
Audience: server.URL,
}
- memoryStore := store.NewMemoryStore()
- err = saveGrant(memoryStore, kf, issuer.Audience)
- if err != nil {
- t.Fatal(err)
- }
-
- auth, err := NewAuthenticationOAuth2(issuer, memoryStore)
+ auth, err := NewAuthenticationOAuth2WithFlow(issuer,
oauth2.ClientCredentialsFlowOptions{
+ KeyFile: kf,
+ })
if err != nil {
t.Fatal(err)
}
- token, err := auth.source.Token()
+ token, err := auth.(*OAuth2Provider).source.Token()
Review Comment:
The type assertion `auth.(*OAuth2Provider)` will panic if the returned type
doesn't match. While this is a test and the type should be correct, it's better
practice to use the comma-ok idiom for type assertions to avoid panics:
```go
provider, ok := auth.(*OAuth2Provider)
if !ok {
t.Fatal("unexpected provider type")
}
token, err := provider.source.Token()
```
Alternatively, since this is accessing an internal field (`source`),
consider adding a public method to retrieve the token for testing purposes.
```suggestion
provider, ok := auth.(*OAuth2Provider)
if !ok {
t.Fatal("unexpected provider type")
}
token, err := provider.source.Token()
```
##########
oauth2/cache/cache.go:
##########
@@ -77,56 +71,21 @@ func (t *tokenCache) Token() (*xoauth2.Token, error) {
return t.token, nil
}
- // load from the store and use the access token if it isn't expired
- grant, err := t.store.LoadGrant(t.audience)
+ grant, err := t.flow.Authorize(t.audience)
if err != nil {
- return nil, fmt.Errorf("LoadGrant: %w", err)
+ return nil, err
}
t.token = grant.Token
Review Comment:
Potential nil pointer dereference: If `flow.Authorize()` returns a grant
with a nil `Token` field, line 78 will assign nil to `t.token`, but the
function will return it without an error. This could cause issues for callers
expecting a valid token.
Consider adding a check after line 78:
```go
if t.token == nil {
return nil, errors.New("authorization succeeded but no token was
returned")
}
```
##########
oauth2/cache/cache.go:
##########
@@ -43,24 +40,21 @@ const (
)
// tokenCache implements a cache for the token associated with a specific
audience.
-// it interacts with the store when the access token is near expiration or
invalidated.
// it is advisable to use a token cache instance per audience.
Review Comment:
The documentation comment should be updated to reflect the new behavior.
Since the old implementation interacted with a store for persistence, and the
new implementation calls the flow to refresh tokens dynamically (enabling key
file reloading), the comment should mention this:
```go
// tokenCache implements a cache for the token associated with a specific
audience.
// When the cached token expires or is invalidated, it obtains a fresh token
by
// calling the flow's Authorize method, which allows for dynamic reloading
of credentials.
// It is advisable to use a token cache instance per audience.
```
```suggestion
// When the cached token expires or is invalidated, it obtains a fresh token
by
// calling the flow's Authorize method, which allows for dynamic reloading
of credentials.
// It is advisable to use a token cache instance per audience.
```
##########
oauth2/cache/cache.go:
##########
@@ -43,24 +40,21 @@ const (
)
// tokenCache implements a cache for the token associated with a specific
audience.
-// it interacts with the store when the access token is near expiration or
invalidated.
// it is advisable to use a token cache instance per audience.
type tokenCache struct {
- clock clock.Clock
- lock sync.Mutex
- store store.Store
- audience string
- refresher oauth2.AuthorizationGrantRefresher
- token *xoauth2.Token
+ clock clock.Clock
+ lock sync.Mutex
+ audience string
+ token *xoauth2.Token
+ flow *oauth2.ClientCredentialsFlow
}
-func NewDefaultTokenCache(store store.Store, audience string,
- refresher oauth2.AuthorizationGrantRefresher) (CachingTokenSource,
error) {
+func NewDefaultTokenCache(audience string,
+ flow *oauth2.ClientCredentialsFlow) (CachingTokenSource, error) {
cache := &tokenCache{
- clock: clock.RealClock{},
- store: store,
- audience: audience,
- refresher: refresher,
+ clock: clock.RealClock{},
+ audience: audience,
+ flow: flow,
}
return cache, nil
Review Comment:
Missing input validation: The function should validate that required
parameters are non-nil. If `flow` is nil, it will cause a panic when `Token()`
is called at line 74.
Consider adding validation:
```go
if flow == nil {
return nil, errors.New("flow cannot be nil")
}
if audience == "" {
return nil, errors.New("audience cannot be empty")
}
```
##########
pulsar/auth/oauth2.go:
##########
@@ -69,46 +66,26 @@ func NewAuthenticationOAuth2WithParams(params
map[string]string) (Provider, erro
if err != nil {
return nil, err
}
- grant, err := flow.Authorize(issuer.Audience)
- if err != nil {
- return nil, err
- }
- err = st.SaveGrant(issuer.Audience, *grant)
- if err != nil {
- return nil, err
- }
+ return NewAuthenticationOAuth2(issuer, flow), nil
default:
return nil, fmt.Errorf("unsupported authentication type: %s",
params[ConfigParamType])
}
- return NewAuthenticationOAuth2(issuer, st), nil
}
Review Comment:
The empty line after the closing brace is unnecessary since the function
ends immediately after. This is a minor style issue but removing it would
improve consistency.
```suggestion
```
##########
pulsar/auth/oauth2_test.go:
##########
@@ -142,3 +161,71 @@ 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")
+ _, err = auth.GetData()
+ require.Error(t, err) // The token refresh should be failed after
updating the client-secret
+
+ // now update the key file to have different client credentials
+ keyFile, err := os.OpenFile(kf, os.O_CREATE|os.O_WRONLY|os.O_TRUNC,
0644)
+ require.NoError(t, err)
+ _, err = keyFile.WriteString(fmt.Sprintf(`{
+ "type":"resource",
+ "client_id":"client-id",
+ "client_secret":"new-client-secret",
+ "client_email":"[email protected]",
+ "issuer_url":"%s"
+}`, server.URL))
+ require.NoError(t, err)
+ require.NoError(t, keyFile.Close())
+
+ token, err = auth.GetData()
+ if err != nil {
+ t.Fatal(err)
+ }
+ assert.Equal(t, "token-content", string(token))
+}
+
+func TestGrantProviderScopes(t *testing.T) {
Review Comment:
The test `TestGrantProviderScopes` creates a mock server without
initializing `expectedClientID` and `expectedClientSecret` atomic values. While
this test doesn't appear to make actual token requests, if it did, the handler
at line 60 would panic with a nil pointer dereference when calling
`.Load().(string)` on uninitialized atomic.Values.
Consider either:
1. Initializing the atomic values before calling `mockOAuthServer()`, or
2. Adding nil checks in the mock token handler to handle uninitialized
values gracefully.
```suggestion
func TestGrantProviderScopes(t *testing.T) {
expectedClientID.Store("client-id")
expectedClientSecret.Store("client-secret")
```
--
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]
