Core: Introduce AuthConfig #10161
Core: Introduce AuthConfig #10161
Conversation
nastra
force-pushed
the
authconfig
branch
4 times, most recently
from
e89bede to
70d3ef8
Compare
| AuthConfig.builder() | ||
| .token(token) | ||
| .credential(credential()) | ||
| .scope(SCOPE) |
There was a problem hiding this comment.
I'd consider SCOPE as one of the optional params. It is optional in both token exchange flow and client credential flow.
There was a problem hiding this comment.
yes SCOPE is optional, but the signer client uses a custom scope, which we need to provide here
There was a problem hiding this comment.
We can get the scope from the optional OAuth parameters for signer. Also a rest catalog isn't necessary with signer. But I'm OK with either one.
| AuthConfig.builder() | ||
| .token(token) | ||
| .credential(credential()) | ||
| .scope(SCOPE) |
There was a problem hiding this comment.
We can get the scope from the optional OAuth parameters for signer. Also a rest catalog isn't necessary with signer. But I'm OK with either one.
dramaticlly
left a comment
dramaticlly
left a comment
There was a problem hiding this comment.
Thank you Eduard, looks promising but have some question hope you can answer on
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Overall, this seems like a good improvement but I think the change removes a public constructor for AuthSession that we cannot remove; I think we'll need to address that before merging. Lmk if I'm missing something!
| public AuthSession( | ||
| Map<String, String> baseHeaders, | ||
| String token, | ||
| String tokenType, | ||
| String credential, | ||
| String scope, | ||
| String oauth2ServerUri, | ||
| Map<String, String> optionalOAuthParams) { | ||
| this.headers = RESTUtil.merge(baseHeaders, authHeaders(token)); | ||
| this.token = token; | ||
| this.tokenType = tokenType; | ||
| this.expiresAtMillis = OAuth2Util.expiresAtMillis(token); | ||
| this.credential = credential; | ||
| this.scope = scope; | ||
| this.oauth2ServerUri = oauth2ServerUri; | ||
| this.optionalOAuthParams = optionalOAuthParams; | ||
| public AuthSession(Map<String, String> baseHeaders, AuthConfig config) { | ||
| this.headers = RESTUtil.merge(baseHeaders, authHeaders(config.token())); | ||
| this.config = config; |
There was a problem hiding this comment.
Since it's public, unfortunately I don't think we can remove this constructor just yet, I think we'll have to deprecate like the other constructors.
There was a problem hiding this comment.
@amogh-jahagirdar that constructor was introduced with 5f655a3 which didn't make it into any release and only exists on main
4 participants
Over time it became quite painful to add new parameters/functionality to
AuthSessionas can be seen with the recent changes from #8976 and from #9839.Hence I'm proposing an
AuthConfigclass that carries all of the relevant configuration for anAuthSession.