[Java Client] Make Audience Field Optional in OAuth2 Client Credentials #11988

michaeljmarshall · 2021-09-09T20:08:03Z

Motivation

When reviewing #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.

BewareMyPower · 2021-09-10T03:14:28Z

Good job! I also found this issue when I learn Azure OAuth 2.0 long time ago but forgot to fix it.

Anonymitaet · 2021-09-10T03:20:43Z

@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.

tuteng · 2021-09-11T10:07:18Z

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` |


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

tuteng · 2021-09-11T10:09:45Z

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 |


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

tuteng · 2021-09-11T10:09:56Z

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 |


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

I don't understand what would be the benefit of this suggestion. This change looks good to me.

michaeljmarshall · 2021-10-04T14:52:24Z

@eolivelli - PTAL

EronWright · 2021-10-04T18:17:27Z

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).

michaeljmarshall · 2021-10-05T03:41:08Z

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?

EronWright · 2021-10-07T16:13:39Z

@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). I mention it to raise awareness.

Anonymitaet · 2021-10-15T04:06:44Z

Hi @michaeljmarshall would you like to add docs to Pulsar? Something like:

BewareMyPower · 2021-10-15T05:07:56Z

@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). I mention it to raise awareness.

I think it doesn't make a difference. If Golang client also makes audience field optional in future, the Audience field will still exist.

type Issuer struct {
    IssuerEndpoint string
    ClientID       string
    Audience       string
}

The only affected code might be here that audience could be nil.

func (f *MemoryStore) SaveGrant(audience string, grant oauth2.AuthorizationGrant) error {
	f.lock.Lock()
	defer f.lock.Unlock()
	f.grants[audience] = &grant
	return nil
}

I think adding a null check will be okay. @EronWright Please confirm it.

lhotari

LGTM

michaeljmarshall · 2021-11-23T22:11:48Z

@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.

BewareMyPower · 2021-11-24T02:10:40Z

I'd like the rebase way.

michaeljmarshall · 2021-11-24T05:01:36Z

@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!

michaeljmarshall · 2021-12-01T18:25:35Z

@lhotari, @tuteng, @BewareMyPower @EronWright @Anonymitaet @wolfstudy - PTAL. Thanks!

BewareMyPower · 2021-12-02T02:46:03Z

LGTM, but I'm not sure if it's proper to merge this PR at this moment.

lhotari · 2021-12-02T04:12:25Z

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.

michaeljmarshall · 2021-12-02T04:23:38Z

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.

BewareMyPower · 2021-12-02T04:35:47Z

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.

…ls (#11988) * [Java Client] Make Audience Field in Client Credentials Optional * Update site2/website-next/docs * Remove update to 2.8.1 docs * Update more docs based on code review (cherry picked from commit b2b5463)

…ls (apache#11988) * [Java Client] Make Audience Field in Client Credentials Optional * Update site2/website-next/docs * Remove update to 2.8.1 docs * Update more docs based on code review

BewareMyPower · 2022-04-27T07:03:52Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/protocol/TokenClient.java

     * @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<>();


Could you explain why do you use a TreeMap here? I'm not sure if the order makes a difference.

It's only a TreeMap because I copied it from exchangeClientCredentials. I don't remember any specific reason for using this data structure.

…rom apache#12448, apache#12641, and apache#12786

…, and apache#11681

michaeljmarshall mentioned this pull request Sep 9, 2021

[pulsar-client]Add a optional params scope for pulsar oauth2 client #11931

Merged

4 tasks

michaeljmarshall changed the title ~~[Java Client] Make Audience Field in Client Credentials Optional~~ [Java Client] Make Audience Field Optional in OAuth2 Client Credentials Sep 9, 2021

BewareMyPower requested review from tuteng, wolfstudy and eolivelli September 10, 2021 03:12

BewareMyPower assigned michaeljmarshall Sep 10, 2021

BewareMyPower added the area/client label Sep 10, 2021

BewareMyPower approved these changes Sep 10, 2021

View reviewed changes

Anonymitaet added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 10, 2021

tuteng reviewed Sep 11, 2021

View reviewed changes

tuteng approved these changes Oct 4, 2021

View reviewed changes

lhotari approved these changes Nov 8, 2021

View reviewed changes

michaeljmarshall added 2 commits November 23, 2021 22:52

[Java Client] Make Audience Field in Client Credentials Optional

327fbc1

Update site2/website-next/docs

02edee2

michaeljmarshall force-pushed the client-creds-flow-make-audience-optional branch from 5fe6804 to 02edee2 Compare November 24, 2021 04:54

michaeljmarshall added 2 commits November 23, 2021 22:56

Remove update to 2.8.1 docs

d03b008

Update more docs based on code review

dfdf255

lhotari closed this Dec 2, 2021

lhotari reopened this Dec 2, 2021

lhotari approved these changes Dec 2, 2021

View reviewed changes

lhotari closed this Dec 2, 2021

lhotari reopened this Dec 2, 2021

lhotari merged commit b2b5463 into apache:master Dec 3, 2021

michaeljmarshall deleted the client-creds-flow-make-audience-optional branch December 3, 2021 05:01

lhotari added cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life release/2.8.2 release/2.9.1 labels Dec 3, 2021

lhotari added this to the 2.10.0 milestone Dec 3, 2021

BewareMyPower reviewed Apr 27, 2022

View reviewed changes

momo-jun added a commit to momo-jun/pulsar that referenced this pull request Aug 4, 2022

Merge 2.9.1-specific docs from apache#11988 and 2.9.2-specific docs f…

b034618

…rom apache#12448, apache#12641, and apache#12786

momo-jun added a commit to momo-jun/pulsar that referenced this pull request Aug 4, 2022

Merge 2.8.2-specific docs from apache#12175, apache#11988, apache#11828…

8869ffe

…, and apache#11681

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java Client] Make Audience Field Optional in OAuth2 Client Credentials #11988

[Java Client] Make Audience Field Optional in OAuth2 Client Credentials #11988

michaeljmarshall commented Sep 9, 2021

BewareMyPower commented Sep 10, 2021

Anonymitaet commented Sep 10, 2021

tuteng Sep 11, 2021

tuteng Sep 11, 2021

tuteng Sep 11, 2021

EronWright Oct 4, 2021

michaeljmarshall commented Oct 4, 2021

EronWright commented Oct 4, 2021

michaeljmarshall commented Oct 5, 2021

EronWright commented Oct 7, 2021

Anonymitaet commented Oct 15, 2021

BewareMyPower commented Oct 15, 2021 •

edited

Loading

lhotari left a comment

michaeljmarshall commented Nov 23, 2021

BewareMyPower commented Nov 24, 2021

michaeljmarshall commented Nov 24, 2021

michaeljmarshall commented Dec 1, 2021

BewareMyPower commented Dec 2, 2021

lhotari commented Dec 2, 2021

michaeljmarshall commented Dec 2, 2021

BewareMyPower commented Dec 2, 2021

BewareMyPower Apr 27, 2022

michaeljmarshall Jun 14, 2022

[Java Client] Make Audience Field Optional in OAuth2 Client Credentials #11988

[Java Client] Make Audience Field Optional in OAuth2 Client Credentials #11988

Conversation

michaeljmarshall commented Sep 9, 2021

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

BewareMyPower commented Sep 10, 2021

Anonymitaet commented Sep 10, 2021

tuteng Sep 11, 2021

Choose a reason for hiding this comment

tuteng Sep 11, 2021

Choose a reason for hiding this comment

tuteng Sep 11, 2021

Choose a reason for hiding this comment

EronWright Oct 4, 2021

Choose a reason for hiding this comment

michaeljmarshall commented Oct 4, 2021

EronWright commented Oct 4, 2021

michaeljmarshall commented Oct 5, 2021

EronWright commented Oct 7, 2021

Anonymitaet commented Oct 15, 2021

BewareMyPower commented Oct 15, 2021 • edited Loading

lhotari left a comment

Choose a reason for hiding this comment

michaeljmarshall commented Nov 23, 2021

BewareMyPower commented Nov 24, 2021

michaeljmarshall commented Nov 24, 2021

michaeljmarshall commented Dec 1, 2021

BewareMyPower commented Dec 2, 2021

lhotari commented Dec 2, 2021

michaeljmarshall commented Dec 2, 2021

BewareMyPower commented Dec 2, 2021

BewareMyPower Apr 27, 2022

Choose a reason for hiding this comment

michaeljmarshall Jun 14, 2022

Choose a reason for hiding this comment

BewareMyPower commented Oct 15, 2021 •

edited

Loading