-
Notifications
You must be signed in to change notification settings - Fork 35
[FEATURE] Adding token cache to u2m oauth #429
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
Changes from 7 commits
eb0fad0
e8080b3
b57f6e2
6f1b69d
d8c366e
3dad770
c9fc008
c4433de
9ce45ef
174c3c2
ab64d0f
23b5bd1
ce75413
ef3c000
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,13 +4,17 @@ | |
| import com.databricks.sdk.core.http.HttpClient; | ||
| import com.databricks.sdk.core.http.Request; | ||
| import com.databricks.sdk.core.http.Response; | ||
| import com.databricks.sdk.core.oauth.FileTokenCache; | ||
| import com.databricks.sdk.core.oauth.OpenIDConnectEndpoints; | ||
| import com.databricks.sdk.core.oauth.TokenCache; | ||
| import com.databricks.sdk.core.oauth.TokenCacheUtils; | ||
| import com.databricks.sdk.core.utils.Cloud; | ||
| import com.databricks.sdk.core.utils.Environment; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.lang.reflect.Field; | ||
| import java.nio.file.Path; | ||
| import java.util.*; | ||
| import org.apache.http.HttpMessage; | ||
|
|
||
|
|
@@ -141,6 +145,9 @@ public class DatabricksConfig { | |
|
|
||
| private DatabricksEnvironment databricksEnvironment; | ||
|
|
||
| // Lazily initialized OAuth token cache | ||
| private transient TokenCache tokenCache; | ||
|
|
||
| public Environment getEnv() { | ||
| return env; | ||
| } | ||
|
|
@@ -669,4 +676,41 @@ public DatabricksConfig newWithWorkspaceHost(String host) { | |
| "headerFactory")); | ||
| return clone(fieldsToSkip).setHost(host); | ||
| } | ||
|
|
||
| /** | ||
| * Sets a custom TokenCache implementation. | ||
| * | ||
| * @param tokenCache the TokenCache implementation to use | ||
| * @return this config instance | ||
| */ | ||
| public DatabricksConfig setTokenCache(TokenCache tokenCache) { | ||
| this.tokenCache = tokenCache; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the default OAuth redirect URL. If one is not provided explicitly, uses | ||
| * http://localhost:8080/callback | ||
| * | ||
| * @return The OAuth redirect URL to use | ||
| */ | ||
| public String getEffectiveOAuthRedirectUrl() { | ||
| return redirectUrl != null ? redirectUrl : "http://localhost:8080/callback"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is hardcoded at 2 places, making this error prone for future
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i made this change to remove the hardcoded value from 2 places. where else do you mean is this hardcoded?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we separate unrelated refactors such as this into separate PRs?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually added code in ExternalBrowserCredentialsProvider where we would have to define this string "http://localhost:8080/callback" again without the refactor. Thought it would be in scope for this change? |
||
| } | ||
|
|
||
| /** | ||
| * Gets the TokenCache instance for the current configuration. | ||
| * | ||
| * <p>If a custom TokenCache has been set, it will be returned. Otherwise, a SimpleFileTokenCache | ||
| * will be created based on the configuration properties. | ||
| * | ||
| * @return A TokenCache instance | ||
| */ | ||
| public synchronized TokenCache getTokenCache() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just talking with @renaudhartert-db. Is it possible to inline this token cache entirely into ExternalBrowserCredentialsProvider? We're trying to break apart DatabricksConfig and stop using it as a dependency injection mechanism. I think you could do this by
|
||
| if (tokenCache == null) { | ||
|
vikrantpuppala marked this conversation as resolved.
Outdated
|
||
| Path cachePath = TokenCacheUtils.getCacheFilePath(getHost(), getClientId(), getScopes()); | ||
| tokenCache = new FileTokenCache(cachePath); | ||
| } | ||
| return tokenCache; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,12 +5,17 @@ | |
| import com.databricks.sdk.core.DatabricksException; | ||
| import com.databricks.sdk.core.HeaderFactory; | ||
| import java.io.IOException; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * A {@code CredentialsProvider} which implements the Authorization Code + PKCE flow by opening a | ||
| * browser for the user to authorize the application. | ||
| * browser for the user to authorize the application. Uses the cache from {@link | ||
| * DatabricksConfig#getTokenCache()}, tokens will be cached to avoid repeated authentication. | ||
| */ | ||
| public class ExternalBrowserCredentialsProvider implements CredentialsProvider { | ||
| private static final Logger LOGGER = | ||
| LoggerFactory.getLogger(ExternalBrowserCredentialsProvider.class); | ||
|
|
||
| @Override | ||
| public String authType() { | ||
|
|
@@ -19,16 +24,57 @@ public String authType() { | |
|
|
||
| @Override | ||
| public HeaderFactory configure(DatabricksConfig config) { | ||
| if (config.getHost() == null || config.getAuthType() != "external-browser") { | ||
| if (config.getHost() == null | ||
| || config.getClientId() == null | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? Are customers explicitly requesting external-browser but also configuring client IDs and that's causing issues?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the testing I did, we do need the client ID for external browser auth?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Take a look at how it is done in the Python SDK: https://github.com/databricks/databricks-sdk-py/blob/main/databricks/sdk/credentials_provider.py#L213-L220. Let's preserve that behavior, defaulting to |
||
| || !config.getAuthType().equals("external-browser")) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| OAuthClient client = new OAuthClient(config); | ||
| Consent consent = client.initiateConsent(); | ||
| SessionCredentials creds = consent.launchExternalBrowser(); | ||
| return creds.configure(config); | ||
| // Get the token cache from config | ||
| TokenCache tokenCache = config.getTokenCache(); | ||
|
vikrantpuppala marked this conversation as resolved.
Outdated
|
||
|
|
||
| // First try to use the cached token if available (will return null if disabled) | ||
| Token cachedToken = tokenCache.load(); | ||
| if (cachedToken != null && cachedToken.getRefreshToken() != null) { | ||
| LOGGER.debug("Found cached token for {}:{}", config.getHost(), config.getClientId()); | ||
|
|
||
| try { | ||
| // Create SessionCredentials with the cached token and try to refresh if needed | ||
| SessionCredentials cachedCreds = | ||
| new SessionCredentials.Builder() | ||
| .withToken(cachedToken) | ||
| .withHttpClient(config.getHttpClient()) | ||
| .withClientId(config.getClientId()) | ||
| .withClientSecret(config.getClientSecret()) | ||
| .withTokenUrl(config.getOidcEndpoints().getTokenEndpoint()) | ||
| .withRedirectUrl(config.getEffectiveOAuthRedirectUrl()) | ||
| .build(); | ||
|
|
||
| LOGGER.debug("Using cached token, will immediately refresh"); | ||
| cachedCreds.token = cachedCreds.refresh(); | ||
| tokenCache.save(cachedCreds.getToken()); | ||
| return cachedCreds.configure(config); | ||
| } catch (Exception e) { | ||
| // If token refresh fails, log and continue to browser auth | ||
| LOGGER.info("Token refresh failed: {}, falling back to browser auth", e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| // If no cached token or refresh failed, perform browser auth | ||
| SessionCredentials credentials = performBrowserAuth(config); | ||
| tokenCache.save(credentials.getToken()); | ||
| return credentials.configure(config); | ||
| } catch (IOException | DatabricksException e) { | ||
| LOGGER.error("Failed to authenticate: {}", e.getMessage()); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| SessionCredentials performBrowserAuth(DatabricksConfig config) throws IOException { | ||
| LOGGER.debug("Performing browser authentication"); | ||
| OAuthClient client = new OAuthClient(config); | ||
| Consent consent = client.initiateConsent(); | ||
| return consent.launchExternalBrowser(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| package com.databricks.sdk.core.oauth; | ||
|
|
||
| import com.databricks.sdk.core.utils.SerDeUtils; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import java.io.File; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.util.Objects; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** A TokenCache implementation that stores tokens as plain files. */ | ||
| public class FileTokenCache implements TokenCache { | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(FileTokenCache.class); | ||
|
|
||
| private final Path cacheFile; | ||
| private final ObjectMapper mapper; | ||
|
|
||
| /** | ||
| * Constructs a new SimpleFileTokenCache instance. | ||
| * | ||
| * @param cacheFilePath The path where the token cache will be stored | ||
| */ | ||
| public FileTokenCache(Path cacheFilePath) { | ||
| Objects.requireNonNull(cacheFilePath, "cacheFilePath must be defined"); | ||
|
|
||
| this.cacheFile = cacheFilePath; | ||
| this.mapper = SerDeUtils.createMapper(); | ||
| } | ||
|
|
||
| @Override | ||
| public void save(Token token) { | ||
| try { | ||
| Files.createDirectories(cacheFile.getParent()); | ||
|
|
||
| // Serialize token to JSON | ||
| String json = mapper.writeValueAsString(token); | ||
| byte[] dataToWrite = json.getBytes(StandardCharsets.UTF_8); | ||
|
|
||
| Files.write(cacheFile, dataToWrite); | ||
| // Set file permissions to be readable only by the owner (equivalent to 0600) | ||
| File file = cacheFile.toFile(); | ||
| file.setReadable(false, false); | ||
| file.setReadable(true, true); | ||
| file.setWritable(false, false); | ||
| file.setWritable(true, true); | ||
|
|
||
| LOGGER.debug("Successfully saved token to cache: {}", cacheFile); | ||
| } catch (Exception e) { | ||
| LOGGER.warn("Failed to save token to cache: {}", cacheFile, e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Token load() { | ||
| try { | ||
| if (!Files.exists(cacheFile)) { | ||
| LOGGER.debug("No token cache file found at: {}", cacheFile); | ||
| return null; | ||
| } | ||
|
|
||
| byte[] fileContent = Files.readAllBytes(cacheFile); | ||
|
|
||
| // Deserialize token from JSON | ||
| String json = new String(fileContent, StandardCharsets.UTF_8); | ||
| Token token = mapper.readValue(json, Token.class); | ||
| LOGGER.debug("Successfully loaded token from cache: {}", cacheFile); | ||
| return token; | ||
| } catch (Exception e) { | ||
| // If there's any issue loading the token, return null | ||
| // to allow a fresh token to be obtained | ||
| LOGGER.warn("Failed to load token from cache: {}", e.getMessage()); | ||
| return null; | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package com.databricks.sdk.core.oauth; | ||
|
|
||
| /** | ||
| * TokenCache interface for storing and retrieving OAuth tokens. Implementations can use different | ||
| * storage mechanisms and security approaches. | ||
| */ | ||
| public interface TokenCache { | ||
| /** | ||
| * Saves a Token to the cache. | ||
| * | ||
| * @param token The Token to save | ||
| */ | ||
| void save(Token token); | ||
|
|
||
| /** | ||
| * Loads a Token from the cache. | ||
| * | ||
| * @return The Token from the cache or null if the cache doesn't exist or is invalid | ||
| */ | ||
| Token load(); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.