-
Notifications
You must be signed in to change notification settings - Fork 33
[PECOBLR-15] Respect OAuth Discovery URL for ExternalBrowserCredentialsStrategy #505
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
[PECOBLR-15] Respect OAuth Discovery URL for ExternalBrowserCredentialsStrategy #505
Conversation
mgyucht
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this PR, can we split out the fallback-to-default behavior to a separate discussion? Otherwise, the changes to the ExternalBrowserCredentialsStrategy and OAuthClient seem good to me.
databricks-sdk-java/src/main/java/com/databricks/sdk/core/DatabricksConfig.java
Outdated
Show resolved
Hide resolved
databricks-sdk-java/src/main/java/com/databricks/sdk/core/DatabricksConfig.java
Outdated
Show resolved
Hide resolved
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
mgyucht
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
) ## Description - This is to support Discovery URL for U2M AWS and GCP in OSS JDBC databricks/databricks-sdk-java#505 (Note azure is not supported as it has a different flow in OSS JDBC) ## Testing Tested that discovery URL is being honored in U2M as well. Tested this using the OSS JDBC with the following URL 1. Success flow test : ``` String jdbcUrl = "jdbc:databricks://e2-dogfood.staging.cloud.databricks.com:443/default;transportMode=http;ssl=1;httpPath=/sql/1.0/warehouses/dd43ee29fedd958d;" + "AuthMech=11;Auth_Flow=2;LogLevel=6;UseThriftClient=0;EnableTokenCache=0;OIDCDiscoveryEndpoint=https://e2-dogfood.staging.cloud.databricks.com/oidc/.well-known/oauth-authorization-server"; ``` 2. Failure flow test : ``` String jdbcUrl = "jdbc:databricks://e2-dogfood.staging.cloud.databricks.com:443/default;transportMode=http;ssl=1;httpPath=/sql/1.0/warehouses/dd43ee29fedd958d;" + "AuthMech=11;Auth_Flow=2;LogLevel=6;UseThriftClient=0;EnableTokenCache=0;OIDCDiscoveryEndpoint=https://google.com"; ``` Also tested using GCP : ``` "jdbc:databricks://6177827686947384.4.gcp.databricks.com:443/default;transportMode=http;ssl=1;EnableTokenCache=0;AuthMech=11;Auth_Flow=2;httpPath=/sql/1.0/warehouses/f5e97c8f8dfb56b5;OIDCDiscoveryEndpoint=https://6177827686947384.4.gcp.databricks.com/oidc/.well-known/oauth-authorization-server"; ```
What changes are proposed in this pull request?
I have also added a fallback to defaultOIDC flow to make this backward compatible too.skipped the fallback in this PR, will raise in a followup if I get approval from the SDK teamHow is this tested?
Tested that discovery URL is being honored in U2M as well. Tested this using the OSS JDBC with the following URL