feat(otdfctl): Auth provider and profile extensions#3736
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the extensibility of the Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. A CLI tool that's quite grand, With auth that you can command. Extensions now stored, With types unexplored, Extending the reach of the land. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a pluggable authentication mechanism and custom profile extensions to otdfctl, allowing external projects to register custom auth providers and store arbitrary JSON data in profiles. The review feedback focuses on robustness and thread safety, suggesting several nil checks for the profile store and its configuration to prevent nil pointer dereferences, as well as utilizing a sync.RWMutex to protect the global authProviders map from concurrent access issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func GetSDKAuthOptionFromProfile(profile *profiles.OtdfctlProfileStore) (sdk.Option, error) { | ||
| c := profile.GetAuthCredentials() | ||
|
|
||
| switch c.AuthType { | ||
| case profiles.AuthTypeClientCredentials: | ||
| return sdk.WithClientCredentials(c.ClientID, c.ClientSecret, NormalizeScopes(c.Scopes)), nil | ||
| case profiles.AuthTypeAccessToken: | ||
| tokenSource := oauth2.StaticTokenSource(buildToken(&c)) | ||
| return sdk.WithOAuthAccessTokenSource(tokenSource), nil | ||
| default: | ||
| return nil, ErrInvalidAuthType | ||
| provider, err := lookupProvider(profile.GetAuthCredentials().AuthType) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return provider.SDKAuthOption(profile) | ||
| } |
There was a problem hiding this comment.
Add a nil check for profile to prevent potential nil pointer dereference panics if the function is called with a nil profile store.
| func GetSDKAuthOptionFromProfile(profile *profiles.OtdfctlProfileStore) (sdk.Option, error) { | |
| c := profile.GetAuthCredentials() | |
| switch c.AuthType { | |
| case profiles.AuthTypeClientCredentials: | |
| return sdk.WithClientCredentials(c.ClientID, c.ClientSecret, NormalizeScopes(c.Scopes)), nil | |
| case profiles.AuthTypeAccessToken: | |
| tokenSource := oauth2.StaticTokenSource(buildToken(&c)) | |
| return sdk.WithOAuthAccessTokenSource(tokenSource), nil | |
| default: | |
| return nil, ErrInvalidAuthType | |
| provider, err := lookupProvider(profile.GetAuthCredentials().AuthType) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return provider.SDKAuthOption(profile) | |
| } | |
| func GetSDKAuthOptionFromProfile(profile *profiles.OtdfctlProfileStore) (sdk.Option, error) { | |
| if profile == nil { | |
| return nil, ErrProfileCredentialsNotFound | |
| } | |
| provider, err := lookupProvider(profile.GetAuthCredentials().AuthType) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return provider.SDKAuthOption(profile) | |
| } |
| func ValidateProfileAuthCredentials(ctx context.Context, profile *profiles.OtdfctlProfileStore) error { | ||
| c := profile.GetAuthCredentials() | ||
|
|
||
| switch c.AuthType { | ||
| case "": | ||
| authType := profile.GetAuthCredentials().AuthType | ||
| if authType == "" { | ||
| return ErrProfileCredentialsNotFound | ||
| case profiles.AuthTypeClientCredentials: | ||
| _, err := GetTokenWithClientCreds(ctx, profile.GetEndpoint(), c.ClientID, c.ClientSecret, profile.GetTLSNoVerify(), c.Scopes) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| case profiles.AuthTypeAccessToken: | ||
| if !buildToken(&c).Valid() { | ||
| return ErrAccessTokenExpired | ||
| } | ||
| default: | ||
| return ErrInvalidAuthType | ||
| } | ||
| return nil | ||
| provider, err := lookupProvider(authType) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return provider.Validate(ctx, profile) | ||
| } |
There was a problem hiding this comment.
Add a nil check for profile to prevent potential nil pointer dereference panics if the function is called with a nil profile store.
func ValidateProfileAuthCredentials(ctx context.Context, profile *profiles.OtdfctlProfileStore) error {
if profile == nil {
return ErrProfileCredentialsNotFound
}
authType := profile.GetAuthCredentials().AuthType
if authType == "" {
return ErrProfileCredentialsNotFound
}
provider, err := lookupProvider(authType)
if err != nil {
return err
}
return provider.Validate(ctx, profile)
}| func GetTokenWithProfile(ctx context.Context, profile *profiles.OtdfctlProfileStore) (*oauth2.Token, error) { | ||
| c := profile.GetAuthCredentials() | ||
|
|
||
| switch c.AuthType { | ||
| case profiles.AuthTypeClientCredentials: | ||
| return GetTokenWithClientCreds(ctx, profile.GetEndpoint(), c.ClientID, c.ClientSecret, profile.GetTLSNoVerify(), c.Scopes) | ||
| case profiles.AuthTypeAccessToken: | ||
| return buildToken(&c), nil | ||
| default: | ||
| return nil, ErrInvalidAuthType | ||
| provider, err := lookupProvider(profile.GetAuthCredentials().AuthType) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return provider.GetToken(ctx, profile) | ||
| } |
There was a problem hiding this comment.
Add a nil check for profile to prevent potential nil pointer dereference panics if the function is called with a nil profile store.
func GetTokenWithProfile(ctx context.Context, profile *profiles.OtdfctlProfileStore) (*oauth2.Token, error) {
if profile == nil {
return nil, ErrProfileCredentialsNotFound
}
provider, err := lookupProvider(profile.GetAuthCredentials().AuthType)
if err != nil {
return nil, err
}
return provider.GetToken(ctx, profile)
}| func GetExtension[T any](p *OtdfctlProfileStore, name string) (T, bool, error) { | ||
| var v T | ||
| raw, ok := p.getRawExtension(name) | ||
| if !ok { | ||
| return v, false, nil | ||
| } | ||
| if err := json.Unmarshal(raw, &v); err != nil { | ||
| return v, true, err | ||
| } | ||
| return v, true, nil | ||
| } |
There was a problem hiding this comment.
Add a nil check for p to prevent potential nil pointer dereference panics if the function is called with a nil profile store.
func GetExtension[T any](p *OtdfctlProfileStore, name string) (T, bool, error) {
var v T
if p == nil {
return v, false, nil
}
raw, ok := p.getRawExtension(name)
if !ok {
return v, false, nil
}
if err := json.Unmarshal(raw, &v); err != nil {
return v, true, err
}
return v, true, nil
}| func SetExtension[T any](p *OtdfctlProfileStore, name string, v T) error { | ||
| raw, err := json.Marshal(v) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return p.setRawExtension(name, raw) | ||
| } |
There was a problem hiding this comment.
Add a nil check for p to prevent potential nil pointer dereference panics if the function is called with a nil profile store.
func SetExtension[T any](p *OtdfctlProfileStore, name string, v T) error {
if p == nil {
return ErrProfileConfigEmpty
}
raw, err := json.Marshal(v)
if err != nil {
return err
}
return p.setRawExtension(name, raw)
}| func (p *OtdfctlProfileStore) ExtensionNames() []string { | ||
| if len(p.config.Extensions) == 0 { | ||
| return nil | ||
| } | ||
| names := make([]string, 0, len(p.config.Extensions)) | ||
| for name := range p.config.Extensions { | ||
| names = append(names, name) | ||
| } | ||
| sort.Strings(names) | ||
| return names | ||
| } |
There was a problem hiding this comment.
Add a nil check for p and p.config to prevent potential nil pointer dereference panics if the method is called on a nil profile store receiver or if the config is not initialized.
| func (p *OtdfctlProfileStore) ExtensionNames() []string { | |
| if len(p.config.Extensions) == 0 { | |
| return nil | |
| } | |
| names := make([]string, 0, len(p.config.Extensions)) | |
| for name := range p.config.Extensions { | |
| names = append(names, name) | |
| } | |
| sort.Strings(names) | |
| return names | |
| } | |
| func (p *OtdfctlProfileStore) ExtensionNames() []string { | |
| if p == nil || p.config == nil || len(p.config.Extensions) == 0 { | |
| return nil | |
| } | |
| names := make([]string, 0, len(p.config.Extensions)) | |
| for name := range p.config.Extensions { | |
| names = append(names, name) | |
| } | |
| sort.Strings(names) | |
| return names | |
| } |
| func (p *OtdfctlProfileStore) getRawExtension(name string) (json.RawMessage, bool) { | ||
| if p.config.Extensions == nil { | ||
| return nil, false | ||
| } | ||
| raw, ok := p.config.Extensions[name] | ||
| return raw, ok | ||
| } |
There was a problem hiding this comment.
Add a nil check for p and p.config to prevent potential nil pointer dereference panics if the method is called on a nil profile store receiver or if the config is not initialized.
func (p *OtdfctlProfileStore) getRawExtension(name string) (json.RawMessage, bool) {
if p == nil || p.config == nil {
return nil, false
}
if p.config.Extensions == nil {
return nil, false
}
raw, ok := p.config.Extensions[name]
return raw, ok
}| func (p *OtdfctlProfileStore) setRawExtension(name string, raw json.RawMessage) error { | ||
| if p.config.Extensions == nil { | ||
| p.config.Extensions = make(map[string]json.RawMessage) | ||
| } | ||
| p.config.Extensions[name] = raw | ||
| return p.store.Save() | ||
| } |
There was a problem hiding this comment.
Add a nil check for p and p.config to prevent potential nil pointer dereference panics if the method is called on a nil profile store receiver or if the config is not initialized.
| func (p *OtdfctlProfileStore) setRawExtension(name string, raw json.RawMessage) error { | |
| if p.config.Extensions == nil { | |
| p.config.Extensions = make(map[string]json.RawMessage) | |
| } | |
| p.config.Extensions[name] = raw | |
| return p.store.Save() | |
| } | |
| func (p *OtdfctlProfileStore) setRawExtension(name string, raw json.RawMessage) error { | |
| if p == nil || p.config == nil { | |
| return ErrProfileConfigEmpty | |
| } | |
| if p.config.Extensions == nil { | |
| p.config.Extensions = make(map[string]json.RawMessage) | |
| } | |
| p.config.Extensions[name] = raw | |
| return p.store.Save() | |
| } |
| import ( | ||
| "context" | ||
|
|
||
| "github.com/opentdf/platform/otdfctl/pkg/profiles" | ||
| "github.com/opentdf/platform/sdk" | ||
| "golang.org/x/oauth2" | ||
| ) |
There was a problem hiding this comment.
Import the sync package to support thread-safe access to the global authProviders map.
| import ( | |
| "context" | |
| "github.com/opentdf/platform/otdfctl/pkg/profiles" | |
| "github.com/opentdf/platform/sdk" | |
| "golang.org/x/oauth2" | |
| ) | |
| import ( | |
| "context" | |
| "sync" | |
| "github.com/opentdf/platform/otdfctl/pkg/profiles" | |
| "github.com/opentdf/platform/sdk" | |
| "golang.org/x/oauth2" | |
| ) |
| // authProviders maps an auth type to its provider. Registration is init-time and | ||
| // single-threaded, so no locking is required. | ||
| var authProviders = map[string]Provider{} | ||
|
|
||
| // RegisterProvider registers (or replaces) the provider for an auth type. | ||
| // Extending projects call this from init(). | ||
| func RegisterProvider(authType string, provider Provider) { | ||
| authProviders[authType] = provider | ||
| } | ||
|
|
||
| // lookupProvider returns the provider for authType, or ErrInvalidAuthType if none. | ||
| func lookupProvider(authType string) (Provider, error) { | ||
| provider, ok := authProviders[authType] | ||
| if !ok { | ||
| return nil, ErrInvalidAuthType | ||
| } | ||
| return provider, nil | ||
| } |
There was a problem hiding this comment.
Use a sync.RWMutex to protect the global authProviders map from concurrent read/write race conditions and potential panics if providers are registered or looked up concurrently at runtime.
var (
authProvidersMu sync.RWMutex
authProviders = map[string]Provider{}
)
// RegisterProvider registers (or replaces) the provider for an auth type.
// Extending projects call this from init().
func RegisterProvider(authType string, provider Provider) {
authProvidersMu.Lock()
defer authProvidersMu.Unlock()
authProviders[authType] = provider
}
// lookupProvider returns the provider for authType, or ErrInvalidAuthType if none.
func lookupProvider(authType string) (Provider, error) {
authProvidersMu.RLock()
defer authProvidersMu.RUnlock()
provider, ok := authProviders[authType]
if !ok {
return nil, ErrInvalidAuthType
}
return provider, nil
}
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
1 participant
Proposed Changes
Checklist
Testing Instructions