From 939242d2f6bc71e6b0f8af1cde93bca38ec5ba6f Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Fri, 6 Jun 2025 00:05:19 -0400 Subject: [PATCH 01/21] Allow saving a new refresh token sent from OAuth APIs when requesting access tokens --- .../datapipeline/service/OAuthHandler.java | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 7d578dc2d4a7..10f93661a96d 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -45,6 +45,7 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.Base64; +import java.util.Objects; import java.util.Optional; import javax.ws.rs.DefaultValue; import javax.ws.rs.GET; @@ -264,9 +265,29 @@ public void getOAuthCredential(HttpServiceRequest request, HttpServiceResponder } catch (JsonSyntaxException e) { throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Error parsing JSON response", e); } - if (refreshTokenResponse.getAccessToken() == null || refreshTokenResponse.getAccessToken().isEmpty()) { + + boolean hasRefreshToken = refreshTokenResponse.getRefreshToken() != null + && !refreshTokenResponse.getRefreshToken().isEmpty(); + boolean hasAccessToken = refreshTokenResponse.getAccessToken() != null + && !refreshTokenResponse.getAccessToken().isEmpty(); + + if (!hasAccessToken) { throw new OAuthServiceException( - HttpURLConnection.HTTP_INTERNAL_ERROR, "Refresh token response body does not have refresh token"); + HttpURLConnection.HTTP_INTERNAL_ERROR, "Access token response body does not have access token"); + } + + // API has given us a new refresh token + if (hasRefreshToken && !refreshToken.getRefreshToken().equals(refreshTokenResponse.getRefreshToken())) { + OAuthRefreshToken newRefreshToken = OAuthRefreshToken.newBuilder() + .withRefreshToken(refreshTokenResponse.getRefreshToken()) + .withRedirectURI(refreshToken.getRedirectURI()) + .build(); + + try { + oauthStore.writeRefreshToken(provider, credentialId, newRefreshToken); + } catch (OAuthStoreException e) { + LOG.error("An error occurred while writing the new refresh token", e); + } } responder.sendString(GSON.toJson( From 25acc52706616d7466caafdfbdd3bb39fb4c2e92 Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Thu, 5 Jun 2025 13:49:03 -0400 Subject: [PATCH 02/21] Add support for saving & getting access tokens in OAuthStore --- .../datapipeline/oauth/OAuthAccessToken.java | 68 +++++++++++++++++++ .../cdap/datapipeline/oauth/OAuthStore.java | 56 +++++++++++++++ .../cdap/datapipeline/OAuthStoreTest.java | 40 ++++++++++- 3 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java new file mode 100644 index 000000000000..5f8d37ad19f5 --- /dev/null +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java @@ -0,0 +1,68 @@ +/* + * Copyright © 2021 Cask Data, Inc. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + * + */ + +package io.cdap.cdap.datapipeline.oauth; + +import com.google.common.base.Preconditions; + +/** + * OAuth access token, with related metadata required to retrieve a long-lived access token. + */ +public class OAuthAccessToken { + private final String accessToken; + private final String redirectURI; + + public OAuthAccessToken(String accessToken, String redirectURI) { + this.accessToken = accessToken; + this.redirectURI = redirectURI; + } + + public String getAccessToken() { + return accessToken; + } + + public String getRedirectURI() { + return redirectURI; + } + + public static Builder newBuilder() { + return new Builder(); + } + + /** + * Builder class for {@link OAuthAccessToken}. + */ + public static class Builder { + private String accessToken; + private String redirectURI; + + public Builder() {} + + public Builder withAccessToken(String accessToken) { + this.accessToken = accessToken; + return this; + } + + public Builder withRedirectURI(String redirectURI) { + this.redirectURI = redirectURI; + return this; + } + + public OAuthAccessToken build() { + Preconditions.checkNotNull(accessToken, "OAuth access token missing"); + Preconditions.checkNotNull(redirectURI, "OAuth redirect URI missing"); + return new OAuthAccessToken(accessToken, redirectURI); + } + } +} diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthStore.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthStore.java index d0aac9bb2a21..35a8d371ed68 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthStore.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthStore.java @@ -47,6 +47,7 @@ public class OAuthStore { private static final String CREDENTIAL_ENCODING_STRATEGY_COL = "credentialencodingstrategy"; private static final String USER_AGENT_COL = "useragent"; private static final String CLIENT_CREDS_KEY_PREFIX = "oauthclientcreds"; + private static final String ACCESS_TOKEN_KEY_PREFIX = "oauthaccesstoken"; private static final String REFRESH_TOKEN_KEY_PREFIX = "oauthrefreshtoken"; private static final Gson GSON = new Gson(); private final TransactionRunner transactionRunner; @@ -203,6 +204,57 @@ public Optional getRefreshToken(String oauthProvider, String } } + /** + * Write an OAuth access token for the given provider and credential. This is used for providers which do not provide + * a refresh token and instead opt for a permanent access token. + * + * @param oauthProvider name of OAuth provider the access token is sourced from + * @param credentialId ID used to identify this credential + * @param token the {@link OAuthAccessToken} to write + * @throws OAuthStoreException if the write fails + */ + public void writeAccessToken( + String oauthProvider, + String credentialId, + OAuthAccessToken token) throws OAuthStoreException { + String namespace = NamespaceId.SYSTEM.getNamespace(); + try { + secureStoreManager.put( + namespace, + getAccessTokenKey(oauthProvider, credentialId), + GSON.toJson(token), + "OAuth access token", + Collections.emptyMap()); + } catch (IOException e) { + throw new OAuthStoreException("Failed to write OAuth access token", e); + } catch (Exception e) { + throw new OAuthStoreException("Namespace \"" + namespace + "\" does not exist", e); + } + } + + /** + * Retrieve the {@link OAuthAccessToken} associated with the given OAuth provider and credential + * + * @param oauthProvider name of the OAuth provider the access token is sourced from + * @param credentialId ID used to identify this credential + * @throws OAuthStoreException if the read fails + */ + public Optional getAccessToken(String oauthProvider, String credentialId) + throws OAuthStoreException { + try { + String tokenJson = new String( + secureStore.getData(NamespaceId.SYSTEM.getNamespace(), getAccessTokenKey(oauthProvider, credentialId)), + StandardCharsets.UTF_8); + return Optional.of(GSON.fromJson(tokenJson, OAuthAccessToken.class)); + } catch (IOException e) { + throw new OAuthStoreException("Failed to read from OAuth access token secure storage", e); + } catch (JsonSyntaxException e) { + throw new OAuthStoreException("Invalid JSON for OAuth access token", e); + } catch (Exception e) { + return Optional.empty(); + } + } + private static String getClientCredsKey(String oauthProvider) { return String.format("%s-%s", CLIENT_CREDS_KEY_PREFIX, oauthProvider.toLowerCase()); } @@ -211,6 +263,10 @@ private static String getRefreshTokenKey(String oauthProvider, String credential return String.format("%s-%s-%s", REFRESH_TOKEN_KEY_PREFIX, oauthProvider.toLowerCase(), credentialId.toLowerCase()); } + private static String getAccessTokenKey(String oauthProvider, String credentialId) { + return String.format("%s-%s-%s", ACCESS_TOKEN_KEY_PREFIX, oauthProvider.toLowerCase(), credentialId.toLowerCase()); + } + private static List> getKey(String name) { List> keyFields = new ArrayList<>(1); keyFields.add(Fields.stringField(OAUTH_PROVIDER_COL, name)); diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java index 919a6550877e..80f4a60e3875 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java @@ -20,7 +20,9 @@ import io.cdap.cdap.api.security.store.SecureStore; import io.cdap.cdap.api.security.store.SecureStoreManager; +import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken; import io.cdap.cdap.datapipeline.oauth.OAuthProvider; +import io.cdap.cdap.datapipeline.oauth.OAuthRefreshToken; import io.cdap.cdap.datapipeline.oauth.OAuthStore; import io.cdap.cdap.spi.data.StructuredRow; import io.cdap.cdap.spi.data.StructuredTable; @@ -30,13 +32,17 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class OAuthStoreTest { - private OAuthStore oauthStore; private TransactionRunner mockTransactionRunner; private SecureStore mockSecureStore; @@ -88,4 +94,32 @@ public void testGetProviderWithNullCredentialStrategy() throws Exception { assertEquals(provider.get().getCredentialEncodingStrategy(), OAuthProvider.CredentialEncodingStrategy.FORM_BODY); } + + @Test + public void testWriteRefreshToken() throws Exception { + doNothing().when(mockSecureStoreManager).put(anyString(), anyString(), any(), anyString(), any()); + + OAuthRefreshToken token = OAuthRefreshToken.newBuilder() + .withRefreshToken("muhtoken") + .withRedirectURI("uri") + .build(); + oauthStore.writeRefreshToken("Provider", "ID0", token); + + verify(mockSecureStoreManager, times(1)) + .put(eq("system"), eq("oauthrefreshtoken-provider-id0"), any(), eq("OAuth refresh token"), any()); + } + + @Test + public void testWriteAccessToken() throws Exception { + doNothing().when(mockSecureStoreManager).put(anyString(), anyString(), any(), anyString(), any()); + + OAuthAccessToken token = OAuthAccessToken.newBuilder() + .withAccessToken("muhtoken") + .withRedirectURI("uri") + .build(); + oauthStore.writeAccessToken("Provider", "ID0", token); + + verify(mockSecureStoreManager, times(1)) + .put(eq("system"), eq("oauthaccesstoken-provider-id0"), any(), eq("OAuth access token"), any()); + } } From 9a10a16afd8cf2a75dbcbd080dec070f6680a071 Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Thu, 5 Jun 2025 20:00:15 -0400 Subject: [PATCH 03/21] Add support for OAuth APIs initially returning only access tokens rather than refresh tokens --- .../datapipeline/service/OAuthHandler.java | 48 ++++++++++++++----- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 7d578dc2d4a7..882bd140938a 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -26,6 +26,7 @@ import io.cdap.cdap.api.service.http.SystemHttpServiceContext; import io.cdap.cdap.datapipeline.oauth.CredentialIsValidResponse; import io.cdap.cdap.datapipeline.oauth.GetAccessTokenResponse; +import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken; import io.cdap.cdap.datapipeline.oauth.OAuthClientCredentials; import io.cdap.cdap.datapipeline.oauth.OAuthProvider; import io.cdap.cdap.datapipeline.oauth.OAuthProvider.CredentialEncodingStrategy; @@ -208,21 +209,44 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to parse JSON: " + e.getMessage(), e); } - if (refreshTokenResponse.getRefreshToken() == null || refreshTokenResponse.getRefreshToken().isEmpty()) { + boolean hasRefreshToken = refreshTokenResponse.getRefreshToken() != null + && !refreshTokenResponse.getRefreshToken().isEmpty(); + boolean hasAccessToken = refreshTokenResponse.getAccessToken() != null + && !refreshTokenResponse.getAccessToken().isEmpty(); + + if (!hasAccessToken && !hasRefreshToken) { throw new OAuthServiceException( - HttpURLConnection.HTTP_INTERNAL_ERROR, "Refresh token response body did not contain refresh token"); + HttpURLConnection.HTTP_INTERNAL_ERROR, + "Refresh token response body did not contain a refresh token or access token"); } - try { - OAuthRefreshToken refreshToken = OAuthRefreshToken.newBuilder() - .withRefreshToken(refreshTokenResponse.getRefreshToken()) - .withRedirectURI(putOAuthCredentialRequest.getRedirectURI()) - .build(); - oauthStore.writeRefreshToken(provider, credentialId, refreshToken); - } catch (NullPointerException e) { - throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, e.getMessage(), e); - } catch (OAuthStoreException e) { - throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to write refresh token", e); + if (hasRefreshToken) { + try { + OAuthRefreshToken refreshToken = OAuthRefreshToken.newBuilder() + .withRefreshToken(refreshTokenResponse.getRefreshToken()) + .withRedirectURI(putOAuthCredentialRequest.getRedirectURI()) + .build(); + oauthStore.writeRefreshToken(provider, credentialId, refreshToken); + } catch (NullPointerException e) { + throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, e.getMessage(), e); + } catch (OAuthStoreException e) { + throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to write refresh token", e); + } + } else { + // Refresh token call gave us an access token without a refresh token. + // Store the access token instead. + + try { + OAuthAccessToken accessToken = OAuthAccessToken.newBuilder() + .withAccessToken(refreshTokenResponse.getAccessToken()) + .withRedirectURI(putOAuthCredentialRequest.getRedirectURI()) + .build(); + oauthStore.writeAccessToken(provider, credentialId, accessToken); + } catch (NullPointerException e) { + throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, e.getMessage(), e); + } catch (OAuthStoreException e) { + throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to write access token", e); + } } responder.sendStatus(HttpURLConnection.HTTP_OK); From b8a34bee3558bc27603e6b5ed6e148e59eafab12 Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Mon, 9 Jun 2025 13:28:52 -0700 Subject: [PATCH 04/21] Throw `HTTP_BAD_REQUEST` rather than `HTTP_INTERNAL_ERROR` when no access token or refresh token is returned. --- .../java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java | 2 +- .../java/io/cdap/cdap/datapipeline/service/OAuthHandler.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java index 5f8d37ad19f5..af10312e56c8 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java @@ -1,5 +1,5 @@ /* - * Copyright © 2021 Cask Data, Inc. + * Copyright © 2025 Cask Data, Inc. * Licensed under the Apache License, Version 2.0 (the "License"); you may not * use this file except in compliance with the License. You may obtain a copy of * the License at diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 882bd140938a..fa7b41d8850b 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -216,7 +216,7 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder if (!hasAccessToken && !hasRefreshToken) { throw new OAuthServiceException( - HttpURLConnection.HTTP_INTERNAL_ERROR, + HttpURLConnection.HTTP_BAD_REQUEST, "Refresh token response body did not contain a refresh token or access token"); } From 68e804d59fce037105bbf00a93522c50ac78fcd0 Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Mon, 9 Jun 2025 13:41:22 -0700 Subject: [PATCH 05/21] Add unit test for case where response contains both a refresh token and an access token. --- .../io/cdap/cdap/datapipeline/OAuthStoreTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java index 80f4a60e3875..97d4193244ba 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java @@ -122,4 +122,19 @@ public void testWriteAccessToken() throws Exception { verify(mockSecureStoreManager, times(1)) .put(eq("system"), eq("oauthaccesstoken-provider-id0"), any(), eq("OAuth access token"), any()); } + + @Test + public void testWriteRefreshAndAccessToken() throws Exception { + doNothing().when(mockSecureStoreManager).put(anyString(), anyString(), any(), anyString(), any()); + + OAuthAccessToken token = OAuthAccessToken.newBuilder() + .withAccessToken("badtoken") + .withRefreshToken("muhtoken") + .withRedirectURI("uri") + .build(); + oauthStore.writeAccessToken("Provider", "ID0", token); + + verify(mockSecureStoreManager, times(1)) + .put(eq("system"), eq("oauthrefreshtoken-provider-id0"), any(), eq("OAuth refresh token"), any()); + } } From 12ff6dab26ab4523fb54be0beadc133c5676618f Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Mon, 9 Jun 2025 15:00:50 -0700 Subject: [PATCH 06/21] Throw an error when refresh token does not write successfully. --- .../java/io/cdap/cdap/datapipeline/service/OAuthHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 10f93661a96d..7437464b60e8 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -45,7 +45,6 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.Base64; -import java.util.Objects; import java.util.Optional; import javax.ws.rs.DefaultValue; import javax.ws.rs.GET; @@ -286,7 +285,8 @@ public void getOAuthCredential(HttpServiceRequest request, HttpServiceResponder try { oauthStore.writeRefreshToken(provider, credentialId, newRefreshToken); } catch (OAuthStoreException e) { - LOG.error("An error occurred while writing the new refresh token", e); + throw new OAuthServiceException( + HttpURLConnection.HTTP_INTERNAL_ERROR, "Unable to write refresh token."); } } From e55d77aff88824e6e78b9aeb6c22ae7a690a5f38 Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Wed, 11 Jun 2025 09:44:48 -0700 Subject: [PATCH 07/21] Add refreshTokenResponse to error message when response does not have an access or refresh token. --- .../java/io/cdap/cdap/datapipeline/service/OAuthHandler.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index fa7b41d8850b..3a722b5b012c 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -217,7 +217,9 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder if (!hasAccessToken && !hasRefreshToken) { throw new OAuthServiceException( HttpURLConnection.HTTP_BAD_REQUEST, - "Refresh token response body did not contain a refresh token or access token"); + "Refresh token response body did not contain a refresh token or access token. \n" + + refreshTokenResponse + ); } if (hasRefreshToken) { From 661e8541e844ff6b32d11fec8665a02e77432623 Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Wed, 11 Jun 2025 11:58:52 -0700 Subject: [PATCH 08/21] Add refreshTokenResponse to error message when response does not have an access or refresh token. --- .../java/io/cdap/cdap/datapipeline/service/OAuthHandler.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 3a722b5b012c..945083a8689a 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -217,8 +217,7 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder if (!hasAccessToken && !hasRefreshToken) { throw new OAuthServiceException( HttpURLConnection.HTTP_BAD_REQUEST, - "Refresh token response body did not contain a refresh token or access token. \n" + - refreshTokenResponse + String.format("Refresh token response is missing the required access token or refresh token. See the full response body: %s", refreshTokenResponse); ); } From 7cff1c8c1a1931163a436e9093887075a43eeacb Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Thu, 5 Jun 2025 13:49:03 -0400 Subject: [PATCH 09/21] Add support for saving & getting access tokens in OAuthStore --- .../datapipeline/oauth/OAuthAccessToken.java | 68 +++++++++++++++++++ .../cdap/datapipeline/oauth/OAuthStore.java | 56 +++++++++++++++ .../cdap/datapipeline/OAuthStoreTest.java | 40 ++++++++++- 3 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java new file mode 100644 index 000000000000..5f8d37ad19f5 --- /dev/null +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java @@ -0,0 +1,68 @@ +/* + * Copyright © 2021 Cask Data, Inc. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + * + */ + +package io.cdap.cdap.datapipeline.oauth; + +import com.google.common.base.Preconditions; + +/** + * OAuth access token, with related metadata required to retrieve a long-lived access token. + */ +public class OAuthAccessToken { + private final String accessToken; + private final String redirectURI; + + public OAuthAccessToken(String accessToken, String redirectURI) { + this.accessToken = accessToken; + this.redirectURI = redirectURI; + } + + public String getAccessToken() { + return accessToken; + } + + public String getRedirectURI() { + return redirectURI; + } + + public static Builder newBuilder() { + return new Builder(); + } + + /** + * Builder class for {@link OAuthAccessToken}. + */ + public static class Builder { + private String accessToken; + private String redirectURI; + + public Builder() {} + + public Builder withAccessToken(String accessToken) { + this.accessToken = accessToken; + return this; + } + + public Builder withRedirectURI(String redirectURI) { + this.redirectURI = redirectURI; + return this; + } + + public OAuthAccessToken build() { + Preconditions.checkNotNull(accessToken, "OAuth access token missing"); + Preconditions.checkNotNull(redirectURI, "OAuth redirect URI missing"); + return new OAuthAccessToken(accessToken, redirectURI); + } + } +} diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthStore.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthStore.java index d0aac9bb2a21..35a8d371ed68 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthStore.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthStore.java @@ -47,6 +47,7 @@ public class OAuthStore { private static final String CREDENTIAL_ENCODING_STRATEGY_COL = "credentialencodingstrategy"; private static final String USER_AGENT_COL = "useragent"; private static final String CLIENT_CREDS_KEY_PREFIX = "oauthclientcreds"; + private static final String ACCESS_TOKEN_KEY_PREFIX = "oauthaccesstoken"; private static final String REFRESH_TOKEN_KEY_PREFIX = "oauthrefreshtoken"; private static final Gson GSON = new Gson(); private final TransactionRunner transactionRunner; @@ -203,6 +204,57 @@ public Optional getRefreshToken(String oauthProvider, String } } + /** + * Write an OAuth access token for the given provider and credential. This is used for providers which do not provide + * a refresh token and instead opt for a permanent access token. + * + * @param oauthProvider name of OAuth provider the access token is sourced from + * @param credentialId ID used to identify this credential + * @param token the {@link OAuthAccessToken} to write + * @throws OAuthStoreException if the write fails + */ + public void writeAccessToken( + String oauthProvider, + String credentialId, + OAuthAccessToken token) throws OAuthStoreException { + String namespace = NamespaceId.SYSTEM.getNamespace(); + try { + secureStoreManager.put( + namespace, + getAccessTokenKey(oauthProvider, credentialId), + GSON.toJson(token), + "OAuth access token", + Collections.emptyMap()); + } catch (IOException e) { + throw new OAuthStoreException("Failed to write OAuth access token", e); + } catch (Exception e) { + throw new OAuthStoreException("Namespace \"" + namespace + "\" does not exist", e); + } + } + + /** + * Retrieve the {@link OAuthAccessToken} associated with the given OAuth provider and credential + * + * @param oauthProvider name of the OAuth provider the access token is sourced from + * @param credentialId ID used to identify this credential + * @throws OAuthStoreException if the read fails + */ + public Optional getAccessToken(String oauthProvider, String credentialId) + throws OAuthStoreException { + try { + String tokenJson = new String( + secureStore.getData(NamespaceId.SYSTEM.getNamespace(), getAccessTokenKey(oauthProvider, credentialId)), + StandardCharsets.UTF_8); + return Optional.of(GSON.fromJson(tokenJson, OAuthAccessToken.class)); + } catch (IOException e) { + throw new OAuthStoreException("Failed to read from OAuth access token secure storage", e); + } catch (JsonSyntaxException e) { + throw new OAuthStoreException("Invalid JSON for OAuth access token", e); + } catch (Exception e) { + return Optional.empty(); + } + } + private static String getClientCredsKey(String oauthProvider) { return String.format("%s-%s", CLIENT_CREDS_KEY_PREFIX, oauthProvider.toLowerCase()); } @@ -211,6 +263,10 @@ private static String getRefreshTokenKey(String oauthProvider, String credential return String.format("%s-%s-%s", REFRESH_TOKEN_KEY_PREFIX, oauthProvider.toLowerCase(), credentialId.toLowerCase()); } + private static String getAccessTokenKey(String oauthProvider, String credentialId) { + return String.format("%s-%s-%s", ACCESS_TOKEN_KEY_PREFIX, oauthProvider.toLowerCase(), credentialId.toLowerCase()); + } + private static List> getKey(String name) { List> keyFields = new ArrayList<>(1); keyFields.add(Fields.stringField(OAUTH_PROVIDER_COL, name)); diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java index 919a6550877e..80f4a60e3875 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java @@ -20,7 +20,9 @@ import io.cdap.cdap.api.security.store.SecureStore; import io.cdap.cdap.api.security.store.SecureStoreManager; +import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken; import io.cdap.cdap.datapipeline.oauth.OAuthProvider; +import io.cdap.cdap.datapipeline.oauth.OAuthRefreshToken; import io.cdap.cdap.datapipeline.oauth.OAuthStore; import io.cdap.cdap.spi.data.StructuredRow; import io.cdap.cdap.spi.data.StructuredTable; @@ -30,13 +32,17 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class OAuthStoreTest { - private OAuthStore oauthStore; private TransactionRunner mockTransactionRunner; private SecureStore mockSecureStore; @@ -88,4 +94,32 @@ public void testGetProviderWithNullCredentialStrategy() throws Exception { assertEquals(provider.get().getCredentialEncodingStrategy(), OAuthProvider.CredentialEncodingStrategy.FORM_BODY); } + + @Test + public void testWriteRefreshToken() throws Exception { + doNothing().when(mockSecureStoreManager).put(anyString(), anyString(), any(), anyString(), any()); + + OAuthRefreshToken token = OAuthRefreshToken.newBuilder() + .withRefreshToken("muhtoken") + .withRedirectURI("uri") + .build(); + oauthStore.writeRefreshToken("Provider", "ID0", token); + + verify(mockSecureStoreManager, times(1)) + .put(eq("system"), eq("oauthrefreshtoken-provider-id0"), any(), eq("OAuth refresh token"), any()); + } + + @Test + public void testWriteAccessToken() throws Exception { + doNothing().when(mockSecureStoreManager).put(anyString(), anyString(), any(), anyString(), any()); + + OAuthAccessToken token = OAuthAccessToken.newBuilder() + .withAccessToken("muhtoken") + .withRedirectURI("uri") + .build(); + oauthStore.writeAccessToken("Provider", "ID0", token); + + verify(mockSecureStoreManager, times(1)) + .put(eq("system"), eq("oauthaccesstoken-provider-id0"), any(), eq("OAuth access token"), any()); + } } From 79d7c1843a97e6356fa2c7709bbfd088549744ab Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Thu, 5 Jun 2025 20:00:15 -0400 Subject: [PATCH 10/21] Add support for OAuth APIs initially returning only access tokens rather than refresh tokens --- .../datapipeline/service/OAuthHandler.java | 48 ++++++++++++++----- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 7437464b60e8..3f56dbb62bee 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -26,6 +26,7 @@ import io.cdap.cdap.api.service.http.SystemHttpServiceContext; import io.cdap.cdap.datapipeline.oauth.CredentialIsValidResponse; import io.cdap.cdap.datapipeline.oauth.GetAccessTokenResponse; +import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken; import io.cdap.cdap.datapipeline.oauth.OAuthClientCredentials; import io.cdap.cdap.datapipeline.oauth.OAuthProvider; import io.cdap.cdap.datapipeline.oauth.OAuthProvider.CredentialEncodingStrategy; @@ -208,21 +209,44 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to parse JSON: " + e.getMessage(), e); } - if (refreshTokenResponse.getRefreshToken() == null || refreshTokenResponse.getRefreshToken().isEmpty()) { + boolean hasRefreshToken = refreshTokenResponse.getRefreshToken() != null + && !refreshTokenResponse.getRefreshToken().isEmpty(); + boolean hasAccessToken = refreshTokenResponse.getAccessToken() != null + && !refreshTokenResponse.getAccessToken().isEmpty(); + + if (!hasAccessToken && !hasRefreshToken) { throw new OAuthServiceException( - HttpURLConnection.HTTP_INTERNAL_ERROR, "Refresh token response body did not contain refresh token"); + HttpURLConnection.HTTP_INTERNAL_ERROR, + "Refresh token response body did not contain a refresh token or access token"); } - try { - OAuthRefreshToken refreshToken = OAuthRefreshToken.newBuilder() - .withRefreshToken(refreshTokenResponse.getRefreshToken()) - .withRedirectURI(putOAuthCredentialRequest.getRedirectURI()) - .build(); - oauthStore.writeRefreshToken(provider, credentialId, refreshToken); - } catch (NullPointerException e) { - throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, e.getMessage(), e); - } catch (OAuthStoreException e) { - throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to write refresh token", e); + if (hasRefreshToken) { + try { + OAuthRefreshToken refreshToken = OAuthRefreshToken.newBuilder() + .withRefreshToken(refreshTokenResponse.getRefreshToken()) + .withRedirectURI(putOAuthCredentialRequest.getRedirectURI()) + .build(); + oauthStore.writeRefreshToken(provider, credentialId, refreshToken); + } catch (NullPointerException e) { + throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, e.getMessage(), e); + } catch (OAuthStoreException e) { + throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to write refresh token", e); + } + } else { + // Refresh token call gave us an access token without a refresh token. + // Store the access token instead. + + try { + OAuthAccessToken accessToken = OAuthAccessToken.newBuilder() + .withAccessToken(refreshTokenResponse.getAccessToken()) + .withRedirectURI(putOAuthCredentialRequest.getRedirectURI()) + .build(); + oauthStore.writeAccessToken(provider, credentialId, accessToken); + } catch (NullPointerException e) { + throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, e.getMessage(), e); + } catch (OAuthStoreException e) { + throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to write access token", e); + } } responder.sendStatus(HttpURLConnection.HTTP_OK); From 20b116372546bef7c02546b09b330cd97800891b Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Mon, 9 Jun 2025 13:28:52 -0700 Subject: [PATCH 11/21] Throw `HTTP_BAD_REQUEST` rather than `HTTP_INTERNAL_ERROR` when no access token or refresh token is returned. --- .../java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java | 2 +- .../java/io/cdap/cdap/datapipeline/service/OAuthHandler.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java index 5f8d37ad19f5..af10312e56c8 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/oauth/OAuthAccessToken.java @@ -1,5 +1,5 @@ /* - * Copyright © 2021 Cask Data, Inc. + * Copyright © 2025 Cask Data, Inc. * Licensed under the Apache License, Version 2.0 (the "License"); you may not * use this file except in compliance with the License. You may obtain a copy of * the License at diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 3f56dbb62bee..d342f84d66a5 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -216,7 +216,7 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder if (!hasAccessToken && !hasRefreshToken) { throw new OAuthServiceException( - HttpURLConnection.HTTP_INTERNAL_ERROR, + HttpURLConnection.HTTP_BAD_REQUEST, "Refresh token response body did not contain a refresh token or access token"); } From 4b790d94edea84ab59ac0f4692b6bdc3076bb0d0 Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Mon, 9 Jun 2025 13:41:22 -0700 Subject: [PATCH 12/21] Add unit test for case where response contains both a refresh token and an access token. --- .../io/cdap/cdap/datapipeline/OAuthStoreTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java index 80f4a60e3875..97d4193244ba 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java @@ -122,4 +122,19 @@ public void testWriteAccessToken() throws Exception { verify(mockSecureStoreManager, times(1)) .put(eq("system"), eq("oauthaccesstoken-provider-id0"), any(), eq("OAuth access token"), any()); } + + @Test + public void testWriteRefreshAndAccessToken() throws Exception { + doNothing().when(mockSecureStoreManager).put(anyString(), anyString(), any(), anyString(), any()); + + OAuthAccessToken token = OAuthAccessToken.newBuilder() + .withAccessToken("badtoken") + .withRefreshToken("muhtoken") + .withRedirectURI("uri") + .build(); + oauthStore.writeAccessToken("Provider", "ID0", token); + + verify(mockSecureStoreManager, times(1)) + .put(eq("system"), eq("oauthrefreshtoken-provider-id0"), any(), eq("OAuth refresh token"), any()); + } } From 6b866630d08db42f4af8970a1c282eae589eaa8b Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Wed, 11 Jun 2025 09:44:48 -0700 Subject: [PATCH 13/21] Add refreshTokenResponse to error message when response does not have an access or refresh token. --- .../java/io/cdap/cdap/datapipeline/service/OAuthHandler.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index d342f84d66a5..337dac645dda 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -217,7 +217,9 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder if (!hasAccessToken && !hasRefreshToken) { throw new OAuthServiceException( HttpURLConnection.HTTP_BAD_REQUEST, - "Refresh token response body did not contain a refresh token or access token"); + "Refresh token response body did not contain a refresh token or access token. \n" + + refreshTokenResponse + ); } if (hasRefreshToken) { From 244042a51aefc7745a79797aac5d9f77e5eca2eb Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Wed, 11 Jun 2025 11:58:52 -0700 Subject: [PATCH 14/21] Add refreshTokenResponse to error message when response does not have an access or refresh token. --- .../java/io/cdap/cdap/datapipeline/service/OAuthHandler.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 337dac645dda..0fa61149310d 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -217,8 +217,7 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder if (!hasAccessToken && !hasRefreshToken) { throw new OAuthServiceException( HttpURLConnection.HTTP_BAD_REQUEST, - "Refresh token response body did not contain a refresh token or access token. \n" + - refreshTokenResponse + String.format("Refresh token response is missing the required access token or refresh token. See the full response body: %s", refreshTokenResponse); ); } From a9343850b689e51d865a5a92953a7f58153a6833 Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Thu, 5 Jun 2025 13:49:03 -0400 Subject: [PATCH 15/21] Add support for saving & getting access tokens in OAuthStore --- .../cdap/datapipeline/OAuthStoreTest.java | 27 +++---------------- 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java index 97d4193244ba..596234860bc3 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java @@ -20,9 +20,7 @@ import io.cdap.cdap.api.security.store.SecureStore; import io.cdap.cdap.api.security.store.SecureStoreManager; -import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken; import io.cdap.cdap.datapipeline.oauth.OAuthProvider; -import io.cdap.cdap.datapipeline.oauth.OAuthRefreshToken; import io.cdap.cdap.datapipeline.oauth.OAuthStore; import io.cdap.cdap.spi.data.StructuredRow; import io.cdap.cdap.spi.data.StructuredTable; @@ -32,17 +30,13 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.anyString; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.doAnswer; public class OAuthStoreTest { + private OAuthStore oauthStore; private TransactionRunner mockTransactionRunner; private SecureStore mockSecureStore; @@ -122,19 +116,4 @@ public void testWriteAccessToken() throws Exception { verify(mockSecureStoreManager, times(1)) .put(eq("system"), eq("oauthaccesstoken-provider-id0"), any(), eq("OAuth access token"), any()); } - - @Test - public void testWriteRefreshAndAccessToken() throws Exception { - doNothing().when(mockSecureStoreManager).put(anyString(), anyString(), any(), anyString(), any()); - - OAuthAccessToken token = OAuthAccessToken.newBuilder() - .withAccessToken("badtoken") - .withRefreshToken("muhtoken") - .withRedirectURI("uri") - .build(); - oauthStore.writeAccessToken("Provider", "ID0", token); - - verify(mockSecureStoreManager, times(1)) - .put(eq("system"), eq("oauthrefreshtoken-provider-id0"), any(), eq("OAuth refresh token"), any()); - } } From f0c8ee0757a0c96f84fc8fb886f56ecfe6c07312 Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Thu, 5 Jun 2025 20:00:15 -0400 Subject: [PATCH 16/21] Add support for OAuth APIs initially returning only access tokens rather than refresh tokens Also change 500 errors to 400 --- .../datapipeline/service/OAuthHandler.java | 34 ++++--------------- 1 file changed, 6 insertions(+), 28 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 0fa61149310d..970ad6c572b8 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -26,7 +26,6 @@ import io.cdap.cdap.api.service.http.SystemHttpServiceContext; import io.cdap.cdap.datapipeline.oauth.CredentialIsValidResponse; import io.cdap.cdap.datapipeline.oauth.GetAccessTokenResponse; -import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken; import io.cdap.cdap.datapipeline.oauth.OAuthClientCredentials; import io.cdap.cdap.datapipeline.oauth.OAuthProvider; import io.cdap.cdap.datapipeline.oauth.OAuthProvider.CredentialEncodingStrategy; @@ -229,9 +228,9 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder .build(); oauthStore.writeRefreshToken(provider, credentialId, refreshToken); } catch (NullPointerException e) { - throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, e.getMessage(), e); + throw new OAuthServiceException(HttpURLConnection.HTTP_BAD_REQUEST, e.getMessage(), e); } catch (OAuthStoreException e) { - throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to write refresh token", e); + throw new OAuthServiceException(HttpURLConnection.HTTP_BAD_REQUEST, "Failed to write refresh token", e); } } else { // Refresh token call gave us an access token without a refresh token. @@ -244,9 +243,9 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder .build(); oauthStore.writeAccessToken(provider, credentialId, accessToken); } catch (NullPointerException e) { - throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, e.getMessage(), e); + throw new OAuthServiceException(HttpURLConnection.HTTP_BAD_REQUEST, e.getMessage(), e); } catch (OAuthStoreException e) { - throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to write access token", e); + throw new OAuthServiceException(HttpURLConnection.HTTP_BAD_REQUEST, "Failed to write access token", e); } } @@ -289,30 +288,9 @@ public void getOAuthCredential(HttpServiceRequest request, HttpServiceResponder } catch (JsonSyntaxException e) { throw new OAuthServiceException(HttpURLConnection.HTTP_INTERNAL_ERROR, "Error parsing JSON response", e); } - - boolean hasRefreshToken = refreshTokenResponse.getRefreshToken() != null - && !refreshTokenResponse.getRefreshToken().isEmpty(); - boolean hasAccessToken = refreshTokenResponse.getAccessToken() != null - && !refreshTokenResponse.getAccessToken().isEmpty(); - - if (!hasAccessToken) { + if (refreshTokenResponse.getAccessToken() == null || refreshTokenResponse.getAccessToken().isEmpty()) { throw new OAuthServiceException( - HttpURLConnection.HTTP_INTERNAL_ERROR, "Access token response body does not have access token"); - } - - // API has given us a new refresh token - if (hasRefreshToken && !refreshToken.getRefreshToken().equals(refreshTokenResponse.getRefreshToken())) { - OAuthRefreshToken newRefreshToken = OAuthRefreshToken.newBuilder() - .withRefreshToken(refreshTokenResponse.getRefreshToken()) - .withRedirectURI(refreshToken.getRedirectURI()) - .build(); - - try { - oauthStore.writeRefreshToken(provider, credentialId, newRefreshToken); - } catch (OAuthStoreException e) { - throw new OAuthServiceException( - HttpURLConnection.HTTP_INTERNAL_ERROR, "Unable to write refresh token."); - } + HttpURLConnection.HTTP_INTERNAL_ERROR, "Refresh token response body does not have refresh token"); } responder.sendString(GSON.toJson( From c83b0127abe299974dd9961054e1e438618fd18b Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Thu, 5 Jun 2025 13:49:03 -0400 Subject: [PATCH 17/21] Add support for saving & getting access tokens in OAuthStore --- .../io/cdap/cdap/datapipeline/OAuthStoreTest.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java index 596234860bc3..80f4a60e3875 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java @@ -20,7 +20,9 @@ import io.cdap.cdap.api.security.store.SecureStore; import io.cdap.cdap.api.security.store.SecureStoreManager; +import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken; import io.cdap.cdap.datapipeline.oauth.OAuthProvider; +import io.cdap.cdap.datapipeline.oauth.OAuthRefreshToken; import io.cdap.cdap.datapipeline.oauth.OAuthStore; import io.cdap.cdap.spi.data.StructuredRow; import io.cdap.cdap.spi.data.StructuredTable; @@ -30,13 +32,17 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class OAuthStoreTest { - private OAuthStore oauthStore; private TransactionRunner mockTransactionRunner; private SecureStore mockSecureStore; From bb29488c08b9ee360a44b47c02e4cb5ee156cbdf Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Thu, 5 Jun 2025 20:00:15 -0400 Subject: [PATCH 18/21] Add support for OAuth APIs initially returning only access tokens rather than refresh tokens --- .../io/cdap/cdap/datapipeline/service/OAuthHandler.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 970ad6c572b8..414e4f1c028e 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -208,12 +208,7 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to parse JSON: " + e.getMessage(), e); } - boolean hasRefreshToken = refreshTokenResponse.getRefreshToken() != null - && !refreshTokenResponse.getRefreshToken().isEmpty(); - boolean hasAccessToken = refreshTokenResponse.getAccessToken() != null - && !refreshTokenResponse.getAccessToken().isEmpty(); - - if (!hasAccessToken && !hasRefreshToken) { + if (refreshTokenResponse.getRefreshToken() == null || refreshTokenResponse.getRefreshToken().isEmpty()) { throw new OAuthServiceException( HttpURLConnection.HTTP_BAD_REQUEST, String.format("Refresh token response is missing the required access token or refresh token. See the full response body: %s", refreshTokenResponse); From 928ea3d8104db2598b01c2615eb80150af993eb1 Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Mon, 9 Jun 2025 13:28:52 -0700 Subject: [PATCH 19/21] Throw `HTTP_BAD_REQUEST` rather than `HTTP_INTERNAL_ERROR` when no access token or refresh token is returned. --- .../io/cdap/cdap/datapipeline/service/OAuthHandler.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java index 414e4f1c028e..b998a499bbf0 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/main/java/io/cdap/cdap/datapipeline/service/OAuthHandler.java @@ -26,6 +26,7 @@ import io.cdap.cdap.api.service.http.SystemHttpServiceContext; import io.cdap.cdap.datapipeline.oauth.CredentialIsValidResponse; import io.cdap.cdap.datapipeline.oauth.GetAccessTokenResponse; +import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken; import io.cdap.cdap.datapipeline.oauth.OAuthClientCredentials; import io.cdap.cdap.datapipeline.oauth.OAuthProvider; import io.cdap.cdap.datapipeline.oauth.OAuthProvider.CredentialEncodingStrategy; @@ -208,7 +209,12 @@ public void putOAuthCredential(HttpServiceRequest request, HttpServiceResponder HttpURLConnection.HTTP_INTERNAL_ERROR, "Failed to parse JSON: " + e.getMessage(), e); } - if (refreshTokenResponse.getRefreshToken() == null || refreshTokenResponse.getRefreshToken().isEmpty()) { + boolean hasRefreshToken = refreshTokenResponse.getRefreshToken() != null + && !refreshTokenResponse.getRefreshToken().isEmpty(); + boolean hasAccessToken = refreshTokenResponse.getAccessToken() != null + && !refreshTokenResponse.getAccessToken().isEmpty(); + + if (!hasAccessToken && !hasRefreshToken) { throw new OAuthServiceException( HttpURLConnection.HTTP_BAD_REQUEST, String.format("Refresh token response is missing the required access token or refresh token. See the full response body: %s", refreshTokenResponse); From 30bdb5c0a79e304b512a4897f98bf4734e977c1b Mon Sep 17 00:00:00 2001 From: wclaiborne Date: Mon, 9 Jun 2025 13:41:22 -0700 Subject: [PATCH 20/21] Add unit test for case where response contains both a refresh token and an access token. --- .../io/cdap/cdap/datapipeline/OAuthStoreTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java index 80f4a60e3875..97d4193244ba 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java @@ -122,4 +122,19 @@ public void testWriteAccessToken() throws Exception { verify(mockSecureStoreManager, times(1)) .put(eq("system"), eq("oauthaccesstoken-provider-id0"), any(), eq("OAuth access token"), any()); } + + @Test + public void testWriteRefreshAndAccessToken() throws Exception { + doNothing().when(mockSecureStoreManager).put(anyString(), anyString(), any(), anyString(), any()); + + OAuthAccessToken token = OAuthAccessToken.newBuilder() + .withAccessToken("badtoken") + .withRefreshToken("muhtoken") + .withRedirectURI("uri") + .build(); + oauthStore.writeAccessToken("Provider", "ID0", token); + + verify(mockSecureStoreManager, times(1)) + .put(eq("system"), eq("oauthrefreshtoken-provider-id0"), any(), eq("OAuth refresh token"), any()); + } } From 53b0e65ca4758d211bf91fa1a1dfedca60748d85 Mon Sep 17 00:00:00 2001 From: Andrew Koroluk Date: Thu, 5 Jun 2025 13:49:03 -0400 Subject: [PATCH 21/21] Add support for saving & getting access tokens in OAuthStore --- .../cdap/datapipeline/OAuthStoreTest.java | 27 +++---------------- 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java index 97d4193244ba..596234860bc3 100644 --- a/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java +++ b/cdap-app-templates/cdap-etl/cdap-data-pipeline-base/src/test/java/io/cdap/cdap/datapipeline/OAuthStoreTest.java @@ -20,9 +20,7 @@ import io.cdap.cdap.api.security.store.SecureStore; import io.cdap.cdap.api.security.store.SecureStoreManager; -import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken; import io.cdap.cdap.datapipeline.oauth.OAuthProvider; -import io.cdap.cdap.datapipeline.oauth.OAuthRefreshToken; import io.cdap.cdap.datapipeline.oauth.OAuthStore; import io.cdap.cdap.spi.data.StructuredRow; import io.cdap.cdap.spi.data.StructuredTable; @@ -32,17 +30,13 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.anyString; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.doAnswer; public class OAuthStoreTest { + private OAuthStore oauthStore; private TransactionRunner mockTransactionRunner; private SecureStore mockSecureStore; @@ -122,19 +116,4 @@ public void testWriteAccessToken() throws Exception { verify(mockSecureStoreManager, times(1)) .put(eq("system"), eq("oauthaccesstoken-provider-id0"), any(), eq("OAuth access token"), any()); } - - @Test - public void testWriteRefreshAndAccessToken() throws Exception { - doNothing().when(mockSecureStoreManager).put(anyString(), anyString(), any(), anyString(), any()); - - OAuthAccessToken token = OAuthAccessToken.newBuilder() - .withAccessToken("badtoken") - .withRefreshToken("muhtoken") - .withRedirectURI("uri") - .build(); - oauthStore.writeAccessToken("Provider", "ID0", token); - - verify(mockSecureStoreManager, times(1)) - .put(eq("system"), eq("oauthrefreshtoken-provider-id0"), any(), eq("OAuth refresh token"), any()); - } }