SNOW-352846 OAuth Authentication: #2 OAuth Support #537

sfc-gh-alhuang · 2023-06-21T20:03:41Z

This PR includes core and test code for OAuth authentication. For more details, please refer to this doc.

According to the discussion, the integration would be added in dew. All OAuth unit test would be perform by mock OAuth client. For detail of integration test, please refer to this PR.

sfc-gh-asen

Looks good, had bunch of nit comments mostly

src/main/java/net/snowflake/ingest/connection/OAuthManager.java

sfc-gh-asen · 2023-06-21T20:56:51Z

src/main/java/net/snowflake/ingest/connection/OAuthManager.java

+ }
+
+ /** refreshToken - Get new access token using refresh_token, client_id, client_secret */
+ void refreshToken() {


nit: it looks like this can be private

src/main/java/net/snowflake/ingest/utils/Utils.java

src/test/java/net/snowflake/ingest/streaming/internal/OAuthTest.java

sfc-gh-asen · 2023-06-21T21:55:55Z

src/main/java/net/snowflake/ingest/connection/RequestBuilder.java

@@ -279,6 +290,28 @@ public RequestBuilder(
 this.host = hostName;
 this.userAgentSuffix = userAgentSuffix;

+ // create our security/token manager
+ switch (credential.getClass().getSimpleName()) {


Using a getClass to do the type check is probably fine here but I'd rather we use instanceof check here instead. The main reason is that, getClass().getSimpleName() will break if someone creates their own credentials class but instanceof will work for inheritance without problem.

if (credential instanceof KeyPair) { ... } else if( credential instanceof OAuthCredential) { ... }

Fixed, thanks for the tip!

sfc-gh-asen · 2023-06-21T23:04:19Z

src/test/java/net/snowflake/ingest/connection/SecurityManagerTest.java

+ }
+
+ /** Test instantiate OAuth manager with invalid params */
+ @Test


You can use
@test(expected = IllegalArgumentException.class)
for test cases throws the correct exception

Fixed, thanks for the tip!

codecov · 2023-06-22T23:23:15Z

Codecov Report

Merging #537 (fad5a00) into master (7d01fb7) will decrease coverage by 0.08%.
The diff coverage is 78.98%.

@@            Coverage Diff             @@
##           master     #537      +/-   ##
==========================================
- Coverage   78.37%   78.30%   -0.08%     
==========================================
  Files          73       76       +3     
  Lines        4597     4734     +137     
  Branches      408      424      +16     
==========================================
+ Hits         3603     3707     +104     
- Misses        821      846      +25     
- Partials      173      181       +8

Impacted Files	Coverage Δ
...va/net/snowflake/ingest/connection/JWTManager.java	`86.04% <ø> (-4.66%)`	⬇️
...ain/java/net/snowflake/ingest/utils/Constants.java	`52.38% <ø> (ø)`
...wflake/ingest/connection/SnowflakeOAuthClient.java	`70.73% <70.73%> (ø)`
...et/snowflake/ingest/connection/RequestBuilder.java	`73.75% <72.72%> (-0.91%)`	⬇️
.../net/snowflake/ingest/connection/OAuthManager.java	`76.92% <76.92%> (ø)`
...ternal/SnowflakeStreamingIngestClientInternal.java	`86.92% <78.94%> (-1.60%)`	⬇️
...rc/main/java/net/snowflake/ingest/utils/Utils.java	`59.57% <86.66%> (+4.88%)`	⬆️
...t/snowflake/ingest/connection/OAuthCredential.java	`100.00% <100.00%> (ø)`
...ain/java/net/snowflake/ingest/utils/ErrorCode.java	`97.50% <100.00%> (+0.20%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sfc-gh-tzhang

Nice change! Left some comments, PTAL

src/main/java/net/snowflake/ingest/connection/OAuthClient.java

sfc-gh-tzhang · 2023-06-26T20:14:34Z

src/main/java/net/snowflake/ingest/connection/OAuthCredential.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2012-2023 Snowflake Computing Inc. All rights reserved.


sfc-gh-tzhang · 2023-06-26T20:18:35Z

src/main/java/net/snowflake/ingest/connection/OAuthManager.java

+ @Override
+ String getToken() {
+ if (refreshFailed.get()) {
+ LOGGER.error("getToken request failed due to token refresh failure");


which LOGGER are you using?

And do we need to log it since an exception will be thrown?

The LOGGER is from its super SecurityManager. Each client has exactly one SecurityManager which is either JWTManager or OAuthManager.

sfc-gh-tzhang · 2023-06-26T20:19:37Z

src/main/java/net/snowflake/ingest/connection/OAuthManager.java

+ String getToken() {
+ if (refreshFailed.get()) {
+ LOGGER.error("getToken request failed due to token refresh failure");
+ throw new SecurityException();


Should we add a message in the exception?

Moved error message into the exception.

src/main/java/net/snowflake/ingest/connection/OAuthManager.java

sfc-gh-tzhang · 2023-06-26T20:47:06Z

src/main/java/net/snowflake/ingest/connection/TelemetryService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 Snowflake Computing Inc. All rights reserved.
+ * Copyright (c) 2023 Snowflake Computing Inc. All rights reserved.


why updating the year?

Updated by mistake, fixed.

...ain/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestClientInternal.java

sfc-gh-tzhang · 2023-06-26T20:49:37Z

...ain/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestClientInternal.java

 channelStatus.getChannelName(),
 channelStatus.getChannelSequencer(),
 channelStatus.getStatusCode(),
- channelStatus.getMessage(),


why the message is removed?

Removed my mistake merge, reverted.

sfc-gh-tzhang · 2023-06-26T20:50:26Z

src/main/java/net/snowflake/ingest/utils/Constants.java

@@ -50,6 +56,7 @@ public class Constants {
 public static final int MAX_STREAMING_INGEST_API_CHANNEL_RETRY = 3;
 public static final int STREAMING_INGEST_TELEMETRY_UPLOAD_INTERVAL_IN_SEC = 10;
 public static final long EP_NDV_UNKNOWN = -1L;
+ public static final int MAX_REFRESH_TOKEN_RETRY = 3;


is this only used for OAuth? if yes, could you add it to the name to make it clear?

Yes, modified.

src/main/java/net/snowflake/ingest/utils/Utils.java

sfc-gh-tjones · 2023-06-30T16:58:53Z

src/main/java/net/snowflake/ingest/connection/OAuthCredential.java

+ this.refreshToken = refreshToken;
+ }
+
+ public void setExpires_in(int expires_in) {


This should be expiresIn.

sfc-gh-tjones · 2023-06-30T16:59:01Z

src/main/java/net/snowflake/ingest/connection/OAuthCredential.java

+ private final String clientSecret;
+ private String accessToken;
+ private String refreshToken;
+ private int expires_in;


Nit, expiresIn

sfc-gh-tjones · 2023-06-30T17:06:56Z

src/main/java/net/snowflake/ingest/connection/RequestBuilder.java

+ /**
+ * RequestBuilder - this constructor is for testing OAuth purposes only
+ *
+ * @param securityManager - security manager, either JWTManager or OAuthManager
+ */
+ public RequestBuilder(SecurityManager securityManager) {
+ this.port = DEFAULT_PORT;
+ this.scheme = DEFAULT_SCHEME;
+ this.host = DEFAULT_HOST_SUFFIX;
+ this.securityManager = securityManager;
+
+ this.userAgentSuffix = null;
+ this.authType = "OAuth";
+ this.telemetryService = null;
+ }


This belongs in a test class

Is it possible put a constructor of RequestBuilder in other class?

Just found out we actually don't need this constructor. Removed.

sfc-gh-tjones · 2023-06-30T17:07:40Z

src/main/java/net/snowflake/ingest/connection/RequestBuilder.java

+ // The authorization type used in auth header
+ public final String authType;


Could you infer this from the SecurityManager type?

sfc-gh-tjones · 2023-06-30T17:08:23Z

src/main/java/net/snowflake/ingest/connection/RequestBuilder.java

+ securityManager =
+ new OAuthManager(
+ accountName, userName, (OAuthCredential) credential, makeBaseURI(), telemetryService);
+ authType = Constants.OAUTH;


If we are assigning securityManager here could we simply call something like securityManager.getAuthType() and use when needed?

It turns out that we actually don't need authType, removed.

sfc-gh-tjones · 2023-06-30T17:08:48Z

src/main/java/net/snowflake/ingest/connection/SnowflakeOAuthClient.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SnowflakeOAuthClient implements OAuthClient {


Docstring needed.

sfc-gh-tzhang

LGTM! Left some minor comments, PTAL. Could you make sure the Code Coverage report won't regress? Looks like we're missing some test coverage

sfc-gh-tzhang · 2023-06-30T20:11:12Z

src/main/java/net/snowflake/ingest/connection/RequestBuilder.java

 throw new IllegalArgumentException();
 }

 // Set up the telemetry service if needed
 this.telemetryService =
- ENABLE_TELEMETRY_TO_SF
+ ENABLE_TELEMETRY_TO_SF && credential instanceof KeyPair


Please add a TODO in the comment with JIRA mentioning that OAuth support will come later

sfc-gh-tzhang · 2023-06-30T20:13:06Z

...ain/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestClientInternal.java

+ } catch (NoSuchAlgorithmException | InvalidKeySpecException e) {
+ throw new SFException(e, ErrorCode.KEYPAIR_CREATION_FAILURE);
+ }
+ logger.logInfo("Using JWT KeyPair for authorization");


nit: could we combine the log line with the one below and use the authType as an input?

sfc-gh-tzhang · 2023-06-30T20:15:05Z

src/main/java/net/snowflake/ingest/utils/ErrorCode.java

+ MAKE_URI_FAILURE("0032"),
+ OAUTH_REFRESH_TOKEN_ERROR("0033"),
+ INVALID_CONFIG_PARAMETER("0034");


I don't the corresponding error messages being added, did I miss anything?

@sfc-gh-alhuang Translations for these errors are missing, see https://github.com/snowflakedb/snowflake-ingest-java/blob/master/src/main/resources/net/snowflake/ingest/ingest_error_messages.properties#L36.

@sfc-gh-lsembera Thanks for the hint! I'll send a PR to add the error messages.

sfc-gh-tjones · 2023-07-12T20:51:31Z

src/main/java/net/snowflake/ingest/connection/OAuthManager.java

+ OAuthCredential oAuthCredential,
+ URIBuilder baseURIBuilder,
+ TelemetryService telemetryService) {
+ this(


sfc-gh-tjones · 2023-07-12T20:51:40Z

src/main/java/net/snowflake/ingest/connection/OAuthManager.java

+ baseURIBuilder,
+ DEFAULT_UPDATE_THRESHOLD_RATIO,
+ telemetryService);
+ }


sfc-gh-tjones · 2023-07-12T20:51:49Z

src/main/java/net/snowflake/ingest/connection/OAuthManager.java

+
+ // check if update threshold is within (0, 1)
+ if (updateThresholdRatio <= 0 || updateThresholdRatio >= 1) {
+ throw new IllegalArgumentException("updateThresholdRatio should fall in (0, 1)");


Missing tests

sfc-gh-tjones · 2023-07-12T20:52:00Z

src/main/java/net/snowflake/ingest/connection/OAuthManager.java

+ refreshToken();
+ }


Not covered by tests?

For this #L94-L95 and #L50-L57, those lines require actual snowflake oauth client to run. The lines required backdoor function which cannot be test in this repo, the tests are added in the dew test PR.

sfc-gh-tjones

Changes since last time look good! See my comments about some missing tests. Thanks!

sfc-gh-alhuang added 2 commits June 21, 2023 18:39

add oauth support

53626d6

format

f79eaa4

sfc-gh-alhuang requested review from a team and sfc-gh-tzhang as code owners June 21, 2023 20:03

sfc-gh-alhuang requested a review from sfc-gh-tjones June 21, 2023 20:03

sfc-gh-asen reviewed Jun 21, 2023

View reviewed changes

fix test

b836607

increase coverage

a8fa9af

sfc-gh-tzhang reviewed Jun 26, 2023

View reviewed changes

sfc-gh-alhuang added 2 commits June 26, 2023 22:45

add param tests & fix

690c3b9

disable telemetry service for OAuth

b7d5520

sfc-gh-tjones reviewed Jun 30, 2023

View reviewed changes

add doc string

2b7d20b

sfc-gh-tzhang approved these changes Jun 30, 2023

View reviewed changes

fix nit

6d1e967

sfc-gh-alhuang force-pushed the alhuang-oauth-2 branch from 2f5a0fa to 6d1e967 Compare June 30, 2023 21:56

sfc-gh-alhuang added 4 commits June 30, 2023 22:06

Merge branch 'master' into alhuang-oauth-2

7c58b82

fix test error

bab4a0c

fix

4850b3f

Merge branch 'master' into alhuang-oauth-2

a5df6b3

sfc-gh-tjones reviewed Jul 12, 2023

View reviewed changes

sfc-gh-tjones approved these changes Jul 12, 2023

View reviewed changes

Merge branch 'master' into alhuang-oauth-2

fad5a00

sfc-gh-alhuang merged commit 2999390 into master Jul 12, 2023
10 of 11 checks passed

sfc-gh-alhuang deleted the alhuang-oauth-2 branch July 12, 2023 21:54

sfc-gh-alhuang mentioned this pull request Jul 18, 2023

SNOW-352846 OAuth Authentication: #3 Add error message #553

Merged

		@@ -0,0 +1,55 @@
		/*
		* Copyright (c) 2012-2023 Snowflake Computing Inc. All rights reserved.

		// The authorization type used in auth header
		public final String authType;

SNOW-352846 OAuth Authentication: #2 OAuth Support #537

SNOW-352846 OAuth Authentication: #2 OAuth Support #537

Conversation

sfc-gh-alhuang commented Jun 21, 2023 • edited Loading

sfc-gh-asen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 22, 2023 • edited Loading

Codecov Report

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-alhuang Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

sfc-gh-tjones left a comment

Choose a reason for hiding this comment

sfc-gh-alhuang commented Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading

sfc-gh-alhuang Jul 12, 2023 •

edited

Loading