CDAP-21261 : Implementing Refresh Token Rotation Oauth by sahusanket · Pull Request #16206 · cdapio/cdap

sahusanket · 2026-08-24T18:54:50Z

This PR introduces support for Refresh Token Rotation (RTR) within the CDAP OAuth token management flow. To ensure safe token rotation in a distributed environment, it leverages distributed leasing/locking via SecureStoreManager to prevent race conditions where multiple pipeline workers might attempt to concurrently rotate the same refresh token.

Key Changes:

RTR Configuration: Introduced RefreshType (with STANDARD and RTR support) to OAuthProvider and PutOAuthProviderRequest.
Concurrency Control: Updated OAuthHandler to utilize distributed leases (acquireLease / releaseLease via OAuthStore) when fetching access tokens. This prevents concurrent token refreshes across multiple worker instances from invalidating each other.
Polling & Recovery: Added polling mechanisms to OAuthHandler so instances blocked by a lock wait for the active worker to finish the refresh and then fetch the newly rotated access token.
State Persistence: Updated OAuthStore to persist the new RefreshType and handle the new lease states.
Configurability: Added new properties to cdap-default.xml for fine-tuning RTR behavior:
- security.auth.oauth.rtr.access.token.safety.buffer.ms
- security.auth.oauth.rtr.wait.timeout.ms
- security.auth.oauth.rtr.wait.poll.interval.ms
- security.auth.oauth.rtr.lease.expiration.timeout.ms
Testing: Updated OAuthStoreTest to mock and validate the new leasing implementation and token deserialization logic.

gemini-code-assist

Code Review

This pull request introduces support for OAuth 2.0 Refresh Token Rotation (RTR) by implementing distributed lease locking in OAuthStore and updating OAuthHandler to coordinate concurrent token refreshes. Feedback on these changes highlights a compilation failure in OAuthStoreTest due to mismatched method signatures for lease operations, a concurrency issue where InterruptedException is swallowed while waiting for a new token, and a potential logical bug in the RTR flow where a process should double-check for a newly published token after acquiring a lock to prevent redundant refreshes.

gemini-code-assist · 2026-08-24T18:58:15Z

+      // 4. Winner (either initial or fallback after timeout) executes token refresh and persistence
+      RefreshTokenResponse tokenResponse = executeTokenRefresh(oauthProvider, provider, credentialId);


When implementing Refresh Token Rotation (RTR), if a process times out waiting for the lock winner and subsequently acquires the lease lock itself, it should double-check if a valid access token has already been published in the store before executing executeTokenRefresh. Without this check, the process might perform a redundant token refresh. In RTR, redundant refreshes are not only wasteful but can also trigger security mechanisms on the authorization server (which detects reuse of rotated refresh tokens and invalidates all active tokens).

Suggested change

// 4. Winner (either initial or fallback after timeout) executes token refresh and persistence

RefreshTokenResponse tokenResponse = executeTokenRefresh(oauthProvider, provider, credentialId);

// Double-check if a valid token was published while we were waiting or acquiring the lock

Optional<OAuthAccessToken> currentToken = getAccessToken(provider, credentialId);

if (currentToken.isPresent() && isAccessTokenValid(currentToken.get())) {

return new GetAccessTokenResponse(currentToken.get().getAccessToken(), "");

}

// 4. Winner (either initial or fallback after timeout) executes token refresh and persistence

RefreshTokenResponse tokenResponse = executeTokenRefresh(oauthProvider, provider, credentialId);

gemini-code-assist · 2026-08-24T18:58:15Z

+import io.cdap.cdap.api.security.store.SecureStoreLease;
 import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken;
 import io.cdap.cdap.datapipeline.oauth.OAuthProvider;
+import io.cdap.cdap.datapipeline.oauth.AuthType;
+import io.cdap.cdap.datapipeline.oauth.RefreshType;


Since SecureStoreLease is no longer used in the test after fixing the compilation error, we should remove its import.

Suggested change

import io.cdap.cdap.api.security.store.SecureStoreLease;

import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken;

import io.cdap.cdap.datapipeline.oauth.OAuthProvider;

import io.cdap.cdap.datapipeline.oauth.AuthType;

import io.cdap.cdap.datapipeline.oauth.RefreshType;

import io.cdap.cdap.datapipeline.oauth.OAuthAccessToken;

import io.cdap.cdap.datapipeline.oauth.OAuthProvider;

import io.cdap.cdap.datapipeline.oauth.AuthType;

import io.cdap.cdap.datapipeline.oauth.RefreshType;

vsethi09 · 2026-08-25T05:53:54Z

  }

+  public SecureStoreInfo getStoreInfo() throws OAuthStoreException {
+    if (secureStoreInfo == null) {


Use synchronized to initialize once.

This locking till doesn't guarantee multiple initialization.

This code would initialise only once, there is a doubt check of if (secureStoreInfo == null) { on line 94 .

vsethi09 · 2026-08-25T05:58:37Z


  public AuthType getAuthType() {
-    return authType;
+    return authType == null ? AuthType.STANDARD : authType;


Can this be null? It is now handled in the constructor.

GSON.fromJson(jsonString, PutOAuthProviderRequest.class) is used,
the value will be null . ( it by passes the constructor )

vsethi09 · 2026-08-25T05:58:42Z

+  }
+
+  public RefreshType getRefreshType() {
+    return refreshType == null ? RefreshType.STANDARD : refreshType;


Can this be null? It is now handled in the constructor.

GSON.fromJson(jsonString, PutOAuthProviderRequest.class) is used,
the value will be null . ( it by passes the constructor )

vsethi09 · 2026-08-25T05:59:05Z

+          AuthType authType,
+          RefreshType refreshType) {


Add @Nullable annotations.

vsethi09 · 2026-08-25T06:01:03Z

  @SerializedName("issued_at")
  private final String issuedAt;
+  @SerializedName("expires_in")
+  private final long expiresIn;


What does this indicate? Epoch time in ms or seconds?

(Optional) You may consider changing this to String expiresAt similar to String issuedAt, if it makes sense.

All the fields are either according to Oauth 2.0 standards or based on Salesforce Standard.

The expiresIn is in accordance to Oauth 2.0 : https://www.rfc-editor.org/info/rfc6749/#appendix-A.14

It is mandated to be in seconds.

Good to mention in the comments or variable name to be clear.

vsethi09 · 2026-08-25T06:01:32Z


 import io.cdap.cdap.api.security.store.SecureStore;
 import io.cdap.cdap.api.security.store.SecureStoreManager;
+


Remove extra line

vsethi09 · 2026-08-25T06:02:22Z

  }
+
+  @Test
+  public void testAcquireAndReleaseDatabaseLease() throws Exception {


Database lease or secret lease?

vsethi09 · 2026-08-25T06:10:36Z

  </property>

+  <property>
+    <name>security.auth.oauth.rtr.access.token.safety.buffer.ms</name>


nit: Please use better property name.

Same for other properties below.

vsethi09 · 2026-08-25T06:13:37Z

+      try {
+        instanceName = java.net.InetAddress.getLocalHost().getHostName();
+      } catch (java.net.UnknownHostException e) {
+        instanceName = "cdf";


Is this intended?

changed to cdap

vsethi09 · 2026-08-25T06:14:13Z

-      // If no long-lived access token was found, request a short-lived access token from the 3rd-party API using the
-      // stored refresh token
-      OAuthRefreshToken refreshToken = getRefreshToken(provider, credentialId);
+  private RefreshTokenResponse executeTokenRefresh(OAuthProvider oauthProvider, String provider, String credentialId) 


Seems like newly added code in this file can be simplified. PTAL.

I have tried to simply the code twice from the initial version. We can discuss over a meet to go over it.

vsethi09

Add unit tests.

vsethi09 · 2026-08-27T09:24:45Z

  public OAuthAccessToken(String accessToken) {
+    this(accessToken, 0L, null);
+  }
+
+  public OAuthAccessToken(String accessToken, long expiresAt) {
+    this(accessToken, expiresAt, null);
+  }
+
+  public OAuthAccessToken(String accessToken, long expiresAt, String identityUrl) {
    this.accessToken = accessToken;
+    this.expiresAt = expiresAt;
+    this.identityUrl = identityUrl;
  }


This class has Builder pattern for creation and multiple parameterized constructors, which is an anti-pattern.

vsethi09 · 2026-08-27T09:25:21Z

  public OAuthProvider(String name,
                       String loginURL,
                       String tokenRefreshURL,
                       @Nullable OAuthClientCredentials clientCreds,
                       @Nullable CredentialEncodingStrategy strategy,
                       @Nullable String userAgent,
                       @Nullable AuthType authType) {
+    this(name, loginURL, tokenRefreshURL, clientCreds, strategy, userAgent, authType, RefreshType.STANDARD);
+  }
+
+  public OAuthProvider(String name,
+                       String loginURL,
+                       String tokenRefreshURL,
+                       @Nullable OAuthClientCredentials clientCreds,
+                       @Nullable CredentialEncodingStrategy strategy,
+                       @Nullable String userAgent,
+                       @Nullable AuthType authType,
+                       @Nullable RefreshType refreshType) {


This class has Builder pattern for creation and multiple parameterized constructors, which is an anti-pattern.

vsethi09 · 2026-08-27T09:25:42Z


 package io.cdap.cdap.datapipeline.oauth;

+import javax.annotation.Nullable;


Missing new line after this line.

vsethi09 · 2026-08-27T09:26:44Z

+         RefreshType.STANDARD);
+  }
+
+  public PutOAuthProviderRequest(


Can this new parameterized constructor be merged with the previous constructor or Builder pattern be used?

refactored to builder pattern

vsethi09 · 2026-08-27T11:38:46Z

    }
  }
+
+  private static String generateWorkerIdPrefix() {


Use thread id instead.

gemini-code-assist Bot reviewed Aug 24, 2026

View reviewed changes

sahusanket force-pushed the CDAP-21261_RTR_handling branch 3 times, most recently from 249faaf to 501504e Compare August 24, 2026 20:05

vsethi09 reviewed Aug 25, 2026

View reviewed changes

vsethi09 requested changes Aug 25, 2026

View reviewed changes

sahusanket force-pushed the CDAP-21261_RTR_handling branch 2 times, most recently from 1d5d428 to 24ec486 Compare August 25, 2026 13:00

vsethi09 reviewed Aug 27, 2026

View reviewed changes

CDAP-21261 : Implementing Refresh Token Rotation Oauth

a8619d3

sahusanket force-pushed the CDAP-21261_RTR_handling branch 2 times, most recently from c30463e to ffdb1aa Compare August 28, 2026 09:23

Simpliyfying OauthHandler Error Handling

e69ba08

sahusanket force-pushed the CDAP-21261_RTR_handling branch from ffdb1aa to e69ba08 Compare August 28, 2026 10:45

		// 4. Winner (either initial or fallback after timeout) executes token refresh and persistence
		RefreshTokenResponse tokenResponse = executeTokenRefresh(oauthProvider, provider, credentialId);


		import io.cdap.cdap.api.security.store.SecureStore;
		import io.cdap.cdap.api.security.store.SecureStoreManager;


		package io.cdap.cdap.datapipeline.oauth;

		import javax.annotation.Nullable;

Uh oh!

Conversation

sahusanket commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsethi09 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sahusanket commented Aug 24, 2026 •

edited

Loading