You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/09 20:08:11 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #11988: [Java Client] Make Audience Field in Client Credentials Optional
michaeljmarshall opened a new pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988
### Motivation
When reviewing https://github.com/apache/pulsar/pull/11931, I noticed that the `audience` field on the `ClientCredentialsExchangeRequest` is not optional, but it should be. Only some Identity Providers require this field. Auth0 is one such provider. Azure AD, for example, does not require this field. Thus, we shouldn't require the field.
### Modifications
* Modify `buildClientCredentialsBody` to only add `audience` when it is non empty and non null. Note that previously, the `audience` field was not allowed to be empty or null. Thus, only setting it on the `bodyMap` when it is non empty and non null will result in backwards compatible changes for all clients.
* Modify `buildClientCredentialsBody` to reduce code duplication in tests
* Updated documentation
* Updated 2 tests and added another to ensure coverage
### Verifying this change
Added new test and modified two existing tests.
### Does this pull request potentially affect one of the following parts:
It modifies the public api in that it allows for the `audience` field to now be null or the empty string. In both cases, the field will not be sent to the Identity Provider. This not a breaking change.
### Documentation
I updated the latest docs. If this change gets cherry picked to `branch-2.8` or `branch-2.7`, we will need to update the docs for those versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-977534854
@BewareMyPower - thank you for your input. I went ahead and rebased my branch.
@Anonymitaet - in my most recent commit, I updated the section of the documentation you highlighted above. Thank you for pointing out that section!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-984241895
LGTM, but I'm not sure if it's proper to merge this PR at this moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] EronWright commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
EronWright commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-933735225
Something to be aware of, some of the client implementations use audience field as a key into the token persistence layer. I suppose the service URL should be used instead. It is important to isolate the tokens for each resource server (Pulsar cluster).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-983937665
@lhotari, @tuteng, @BewareMyPower @EronWright @Anonymitaet @wolfstudy - PTAL. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on a change in pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
tuteng commented on a change in pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#discussion_r706593281
##########
File path: site2/docs/security-oauth2.md
##########
@@ -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 |
Review comment:
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
##########
File path: site2/website/versioned_docs/version-2.8.1/security-oauth2.md
##########
@@ -29,7 +29,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. Required by some Identity Providers. | `https://broker.example.com` | Optional |
Review comment:
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
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/README.md
##########
@@ -46,7 +46,7 @@ The following parameters are supported:
| `type` | Oauth 2.0 auth type. Optional. | default: `client_credentials` |
| `issuerUrl` | URL of the provider which allows Pulsar to obtain an access token. Required. | `https://accounts.google.com` |
| `privateKey` | URL to a JSON credentials file (in JSON format; see below). Required. | See "Supported Pattern Formats" |
-| `audience` | An OAuth 2.0 "resource server" identifier for the Pulsar cluster. Required. | `https://broker.example.com` |
+| `audience` | An OAuth 2.0 "resource server" identifier for the Pulsar cluster. Required by some Identity Providers. Optional for client. | `https://broker.example.com` |
Review comment:
`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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-916600155
@michaeljmarshall thanks for adding docs for master! Please do not forget to add docs to other versions if applicable. Then you can ping me to review, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-984280427
> LGTM, but I'm not sure if it's proper to merge this PR at this moment.
@BewareMyPower why wouldn't it be proper? I think I might be missing some context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-916598048
Good job! I also found this issue when I learn Azure OAuth 2.0 long time ago but forgot to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari closed pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-984276112
Closing and re-opening to run against latest master branch changes. GitHub creates a new merge commit internally when that happens. Another option would be to rebase or push new commits to the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] EronWright commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
EronWright commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-937942057
@michaeljmarshall I don't know of any Java implementations that do, but I was thinking about `pulsarctl` since it maintains a token cache based on audience ([example](https://github.com/streamnative/pulsarctl/blob/master/pkg/auth/oauth2.go#L63)). I mention it to raise awareness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] EronWright commented on a change in pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#discussion_r721153523
##########
File path: site2/website/versioned_docs/version-2.8.1/security-oauth2.md
##########
@@ -29,7 +29,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. Required by some Identity Providers. | `https://broker.example.com` | Optional |
Review comment:
I don't understand what would be the benefit of this suggestion. This change looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-984285271
Just a concern about backward compatibility like what @EronWright mentioned. From my perspective, it doesn't break the compatibility. So I think we can merge it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-977399155
I'd like the rebase way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] EronWright commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
EronWright commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-937942057
@michaeljmarshall I don't know of any Java implementations that do, but I was thinking about `pulsarctl` since it maintains a token cache based on audience ([example](https://github.com/streamnative/pulsarctl/blob/master/pkg/auth/oauth2.go#L63)). I mention it to raise awareness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari closed pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-933564428
@eolivelli - PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-977215586
@lhotari, @tuteng, @BewareMyPower - what is the preferred way to resolve conflicts here? The contributor's guide advises against pushing with force and thus against rebasing, but merging master into my branch will add a ton of extra commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11988: [Java Client] Make Audience Field Optional in OAuth2 Client Credentials
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11988:
URL: https://github.com/apache/pulsar/pull/11988#issuecomment-934032355
> Something to be aware of, some of the client implementations use audience field as a key into the token persistence layer. I suppose the service URL should be used instead. It is important to isolate the tokens for each resource server (Pulsar cluster).
@EronWright - can you clarify which client implementations you're referring to? Since the field will still exist, I don't think we're breaking any compatibility here. Is there documentation you think we should update?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org