-
Notifications
You must be signed in to change notification settings - Fork 136
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
OAuth client: add support for custom request parameters #10154
Conversation
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.
Nice improvement! 👍 Just one minor comment.
api/client/src/main/java/org/projectnessie/client/auth/oauth2/OAuth2AuthenticatorConfig.java
Outdated
Show resolved
Hide resolved
@@ -312,12 +313,12 @@ static Stream<Arguments> testFromConfig() { | |||
ImmutableMap.of( | |||
CONF_NESSIE_OAUTH2_TOKEN_ENDPOINT, "https://example.com/token", | |||
CONF_NESSIE_OAUTH2_CLIENT_SECRET, "s3cr3t", | |||
CONF_NESSIE_OAUTH2_REFRESH_SAFETY_WINDOW, "PT10S", |
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.
The previous test for empty client ID was actually a test for the @Check
method and is already covered above (cf. line125), so I removed it.
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!
Maybe just add an example specific for Auth0
* sign. The values must NOT be URL-encoded. Example: | ||
* | ||
* <pre>{@code | ||
* nessie.authentication.oauth2.extra-params = "custom_param1=custom_value1,custom_param2=custom_value2" |
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.
Should there be an example for the audience
field, mentioning Auth0?
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.
Maybe an Auth0 guide in the site docs (not in javadoc)?
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.
In this case I think we need to extract the authentication stuff out of the "Client Configuration" page and create a dedicated "Client Authentication" page, wdyt? Then we could add instructions for Keycloak, Auth0, etc.
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.
I think having a dedicated "Client Authentication" is great. I think it would make it easier for end users to find relevant examples. From Zulip feedback it feels like existing docs are a bit too "technical" :)
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.
Okay, I'm adding that to my todo list.
void testExtraRequestParameters() throws Exception { | ||
|
||
try (HttpTestServer server = new HttpTestServer(handler(), true)) { | ||
|
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.
Nit-troll: superfluous new-line :disappear: ;) ;)
For context, this change is motivated by the fact that Auth0 requires an
audience
field to be included inclient_credentials
requests to the token endpoint:https://auth0.com/docs/get-started/authentication-and-authorization-flow/client-credentials-flow/call-your-api-using-the-client-credentials-flow#request-tokens
The audience value must correspond to a declared "API identifier", and is mandatory.
This deviates from standard OAuth 2.0 specs and have annoyed other client implementors, e.g. this one.
Rather than creating a specific
audience
field just for Auth0, I chose instead to open up for arbitrary custom parameters to be passed.Ironically, Iceberg REST client is not affected by this issue, because if the
audience
field is present, it is (mistakenly) included in all requests (and ignored by the auth server).