rambleraptor commented on code in PR #579:
URL: https://github.com/apache/iceberg-go/pull/579#discussion_r2673685538
##########
catalog/rest/rest.go:
##########
@@ -587,23 +503,37 @@ func (r *Catalog) createSession(ctx context.Context, opts
*options) (*http.Clien
}
cl := &http.Client{Transport: session}
- token := opts.oauthToken
- if token == "" && opts.credential != "" {
- var err error
- if token, err = r.fetchAccessToken(cl, opts.credential, opts);
err != nil {
- return nil, fmt.Errorf("auth error: %w", err)
- }
- }
+ // This creates an OAuth2Manager if no auth manager is provided.
+ // This goal is to replicate existing behavior.
+ if opts.credential != "" {
+ if _, ok := opts.authManager.(*Oauth2AuthManager); !ok {
+ authURI := opts.authUri
+ if authURI == nil {
+ authURI = r.baseURI.JoinPath("oauth/tokens")
+ }
- if token != "" {
- session.defaultHeaders.Set(authorizationHeader, bearerPrefix+"
"+token)
+ opts.authManager = &Oauth2AuthManager{
+ Credential: opts.credential,
+ AuthURI: authURI,
+ Scope: opts.scope,
+ Client: cl,
+ }
+ }
Review Comment:
I agree with everything you're saying. Sorry this took so long to get back
to you. My notification got lost in the winter holidays.
I'm really not a fan of adding more methods to the AuthManager interface.
Credential, AuthURI...they're all implementation details for authentication. A
user could create an auth manager that just returns a static value and none of
these other methods would be useful.
I changed up the defaults. If no AuthManager is set, we create an
OAuth2AuthManager using the options they provided (token, credential). This
gets us the same behaviors we have right now, cleans up this part of the code
immensely, and still lets us punt all the authentication details to the
AuthManager (where it belongs)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
