[CLI-2816] On-prem OAuth SSO support #2652
Conversation
Would it make sense to say |
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
| @@ -141,13 +145,74 @@ func (a *AuthTokenHandlerImpl) refreshCCloudSSOToken(client *ccloudv1.Client, re | |||
| return res.GetToken(), refreshToken, err | |||
| } | |||
|
|
|||
| func (a *AuthTokenHandlerImpl) GetConfluentToken(mdsClient *mdsv1.APIClient, credentials *Credentials) (string, error) { | |||
| func (a *AuthTokenHandlerImpl) GetConfluentToken(mdsClient *mdsv1.APIClient, credentials *Credentials, noBrowser bool) (string, string, error) { | |||
| ctx := utils.GetContext() | |||
There was a problem hiding this comment.
Didn't know we had this function... maybe we should try to deprecate/remove it? The standard approach to getting the context is from the (un)authenticated command struct
There was a problem hiding this comment.
This isn't a config.Context, but a context.Context.
There was a problem hiding this comment.
Oh, meaning from the stdlib "context" package?
There was a problem hiding this comment.
Huh, I'm looking at the implementation of this. Why do we need to log DNS/TLS information here?
There was a problem hiding this comment.
Hmm. I'm not sure. Maybe we can replace both calls to this w/ background context and remove it like you suggested at the start.
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
| GetOnPremSsoCredentials(url, caCertPath string, unsafeTrace bool) func() (*Credentials, error) | ||
| GetOnPremSsoCredentialsFromConfig(*config.Config, bool) func() (*Credentials, error) |
There was a problem hiding this comment.
I think these functions should be able to be combined into one, right?
There was a problem hiding this comment.
Probably, yeah, I'll give it a try.
There was a problem hiding this comment.
Not sure how to do this neatly, actually.
The issue is that when you first do confluent login --url ..., the context is empty so you need to pass in the url and ca-cert-path values from the flag. But afterwards, when using the refresh token, I need to use the url and ca-cert-path from the Context (because some on-prem commands don't support those flags).
So a combined function needs to pass in url, ca-cert-path, and config/context (which I also need for auth and refresh token in addition to url and ca-cert-path) to account for both cases.
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
Release Notes
Breaking Changes
New Features
confluent loginconfluent iam user describeto display username and authentication tokenBug Fixes
Checklist
What
Add support for on-premises SSO authentication when
oidc.login.device.1.enabledis enabled.When both SSO and LDAP are enabled (which is possible for on-prem), we should prefer SSO. So the current order of precedence is
prompt flag > environment variables (SSO > LDAP) > LDAP (keychain > config > netrc) > SSO > LDAP (prompt).References
Test & Review
Manual testing