-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Java Client] Make Audience Field Optional in OAuth2 Client Credentials #11988
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 all commits
327fbc1
02edee2
d03b008
dfdf255
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 |
|---|---|---|
|
|
@@ -73,10 +73,21 @@ public void close() throws Exception { | |
|
|
||
| /** | ||
| * Constructing http request parameters. | ||
| * @param bodyMap List of parameters to be requested. | ||
| * @param req object with relevant request parameters | ||
| * @return Generate the final request body from a map. | ||
| */ | ||
| String buildClientCredentialsBody(Map<String, String> bodyMap) { | ||
| String buildClientCredentialsBody(ClientCredentialsExchangeRequest req) { | ||
| Map<String, String> bodyMap = new TreeMap<>(); | ||
|
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. Could you explain why do you use a
Member
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. It's only a |
||
| bodyMap.put("grant_type", "client_credentials"); | ||
| bodyMap.put("client_id", req.getClientId()); | ||
| bodyMap.put("client_secret", req.getClientSecret()); | ||
| // Only set audience and scope if they are non-empty. | ||
| if (!StringUtils.isBlank(req.getAudience())) { | ||
| bodyMap.put("audience", req.getAudience()); | ||
| } | ||
| if (!StringUtils.isBlank(req.getScope())) { | ||
| bodyMap.put("scope", req.getScope()); | ||
| } | ||
| return bodyMap.entrySet().stream() | ||
| .map(e -> { | ||
| try { | ||
|
|
@@ -96,15 +107,7 @@ String buildClientCredentialsBody(Map<String, String> bodyMap) { | |
| */ | ||
| public TokenResult exchangeClientCredentials(ClientCredentialsExchangeRequest req) | ||
| throws TokenExchangeException, IOException { | ||
| Map<String, String> bodyMap = new TreeMap<>(); | ||
| bodyMap.put("grant_type", "client_credentials"); | ||
| bodyMap.put("client_id", req.getClientId()); | ||
| bodyMap.put("client_secret", req.getClientSecret()); | ||
| bodyMap.put("audience", req.getAudience()); | ||
| if (!StringUtils.isBlank(req.getScope())) { | ||
| bodyMap.put("scope", req.getScope()); | ||
| } | ||
| String body = buildClientCredentialsBody(bodyMap); | ||
| String body = buildClientCredentialsBody(req); | ||
|
|
||
| try { | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,7 @@ The following table lists parameters supported for the `client credentials` auth | |
| | `type` | Oauth 2.0 authentication type. | `client_credentials` (default) | Optional | | ||
| | `issuerUrl` | URL of the authentication provider which allows the Pulsar client to obtain an access token | `https://accounts.google.com` | Required | | ||
| | `privateKey` | URL to a JSON credentials file | Support the following pattern formats: <br> <li> `file:///path/to/file` <li>`file:/path/to/file` <li> `data:application/json;base64,<base64-encoded value>` | Required | | ||
| | `audience` | An OAuth 2.0 "resource server" identifier for the Pulsar cluster | `https://broker.example.com` | Required | | ||
| | `audience` | An OAuth 2.0 "resource server" identifier for the Pulsar cluster | `https://broker.example.com` | Optional | | ||
|
Member
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. Not sure if updating this field to be optional will cause confusion for users using other languages, one suggestion I have is to keep this field as required until other languages implement this change |
||
| | `scope` | Scope of an access request. <br />For more more information, see [access token scope](https://datatracker.ietf.org/doc/html/rfc6749#section-3.3). | api://pulsar-cluster-1/.default | Optional | | ||
|
|
||
| The credentials file contains service account credentials used with the client authentication type. The following shows an example of a credentials file `credentials_file.json`. | ||
|
|
@@ -64,7 +64,7 @@ In the above example, the mapping relationship is shown as below. | |
|
|
||
| - The `issuerUrl` parameter in this plugin is mapped to `--url https://dev-kt-aa9ne.us.auth0.com`. | ||
| - The `privateKey` file parameter in this plugin should at least contains the `client_id` and `client_secret` fields. | ||
| - The `audience` parameter in this plugin is mapped to `"audience":"https://dev-kt-aa9ne.us.auth0.com/api/v2/"`. | ||
| - The `audience` parameter in this plugin is mapped to `"audience":"https://dev-kt-aa9ne.us.auth0.com/api/v2/"`. This field is only used by some identity providers. | ||
|
|
||
| ## Client Configuration | ||
|
|
||
|
|
||
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.
Optional for client.=>Optional for java client.?Just a note, we also need to make this field optional for other language clients that support oauth authentication (e.g. cpp, python, golang), otherwise, there may be inconsistent behavior, e.g. for java language this field is optional and for cpp this field is required, can you create several issues, in order for us to track this thing