-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SNOW-1831099: OAuth Client Credentials Flow Implementation #1993
Conversation
@@ -16,7 +16,7 @@ public class AssertUtil { | |||
* @param internalErrorMesg The error message to display if condition is false | |||
* @throws SFException Will be thrown if condition is false | |||
*/ | |||
static void assertTrue(boolean condition, String internalErrorMesg) throws SFException { | |||
public static void assertTrue(boolean condition, String internalErrorMesg) throws SFException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed.
@@ -842,6 +842,7 @@ private static String executeRequestInternal( | |||
SnowflakeUtil.logResponseDetails(response, logger); | |||
|
|||
if (response != null) { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's minimize such unnecessary changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
String oauthAccessToken = accessTokenProvider.getAccessToken(loginInput); | ||
loginInput.setAuthenticator(AuthenticatorType.OAUTH.name()); | ||
loginInput.setToken(oauthAccessToken); | ||
loginInput.setUserName("0oalpyiuy8rmozhjZ5d7"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the secret and git guardian should complain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't though 🤷 Removed it.
@@ -98,10 +97,11 @@ private String exchangeAuthorizationCodeForAccessToken( | |||
TokenRequest request = buildTokenRequest(loginInput, authorizationCode, pkceVerifier); | |||
URI requestUri = request.getEndpointURI(); | |||
logger.debug( | |||
"Requesting access token from: {}", requestUri.getAuthority() + requestUri.getPath()); | |||
"Requesting OAuth access token from: {}", | |||
requestUri.getAuthority() + requestUri.getPath()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here also let's use parameters without concatenation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
loginInput.getLoginTimeout(), | ||
loginInput.getAuthTimeout(), | ||
loginInput.getSocketTimeoutInMillis(), | ||
0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the 0 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's retry count. In case of SAML it's also 0 so I wanted this to be consistent.
} else if (loginInput | ||
.getAuthenticator() | ||
.equalsIgnoreCase(AuthenticatorType.OAUTH_CLIENT_CREDENTIALS.name())) { | ||
// OAuth authorization code flow authentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it accurate? Do we need this?
|
||
private static final SFLogger logger = | ||
SFLoggerFactory.getLogger(AccessTokenProviderFactory.class); | ||
private static final AuthenticatorType[] ELIGIBLE_AUTH_TYPES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why array? Maybe it can be a set?
} | ||
|
||
public static Set<AuthenticatorType> getEligible() { | ||
return new HashSet<>(Arrays.asList(ELIGIBLE_AUTH_TYPES)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can create this value once, right?
import org.apache.http.entity.StringEntity; | ||
|
||
@SnowflakeJdbcInternalApi | ||
public class OAuthUtil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need it to be public?
Overview
SNOW-1831099
Pre-review self checklist
master
branchmvn -P check-style validate
)mvn verify
and inspecttarget/japicmp/japicmp.html
)SNOW-XXXX:
External contributors - please answer these questions before submitting a pull request. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Issue: #NNNN
Fill out the following pre-review checklist:
@SnowflakeJdbcInternalApi
(note that public/protected methods/fields in classes marked with this annotation are already internal)Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.