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

tuteng · 2021-09-06T09:37:33Z

Motivation

In some scenarios (e.g. azure cloud), when the client exchanges tokens with the server, an optional scope parameter is required, this pr fixes this issue, to ensure compatibility, when the user does not fill in this parameter, all behavior is the same as before.

Modifications

Add an optional parameter scope when exchanges token

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

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

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API: (yes / no)
The schema: (yes / no / don't know)
The default values of configurations: (yes / no)
The wire protocol: (yes / no)
The rest endpoints: (yes / no)
The admin cli options: (yes / no)
Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

doc-required
Pulsar client oauth2 client support optional parameter scope #11932

(If you need help on updating docs, create a doc issue)
no-need-doc

(Please explain why)
doc

(If this PR contains doc changes)

tuteng · 2021-09-06T14:00:49Z

/pulsarbot run-failure-checks

Anonymitaet · 2021-09-07T01:17:20Z

@tuteng Thanks for your contribution. Please do not forget to add docs accordingly to allow users to know your great code changes. And you can ping me to review the docs, thanks.

tuteng · 2021-09-07T02:18:04Z

/pulsarbot run-failure-checks

eolivelli

very useful, I left a comment about the test.
please take a look

eolivelli · 2021-09-07T06:14:59Z

...client/src/test/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2Test.java

@@ -86,6 +86,7 @@ public void testConfigure() throws Exception {
        params.put("privateKey", "data:base64,e30=");
        params.put("issuerUrl", "http://localhost");
        params.put("audience", "http://localhost");
+        params.put("scope", "test-scope");


can you please add more testing about this value ?
I see this only added here and there is not follow up verification

@addisonj I have added some tests, please take a look at this, thanks

michaeljmarshall

Good addition. Left some comments that I think we should address before merging this.

...client/src/test/java/org/apache/pulsar/client/impl/auth/oauth2/protocol/TokenClientTest.java

michaeljmarshall · 2021-09-07T18:59:15Z

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

@@ -76,6 +77,9 @@ public TokenResult exchangeClientCredentials(ClientCredentialsExchangeRequest re
        bodyMap.put("client_id", req.getClientId());
        bodyMap.put("client_secret", req.getClientSecret());
        bodyMap.put("audience", req.getAudience());


It seems to me that the audience field originates from the Auth0 implementation, and is not necessary in every case. Since it is optional and since the bodyMap does not allow for null values, I think we should only put this field in the bodyMap if it is non empty and non null.

I haven't seen the audience related specification yet, if I want to change this configuration, there might be other parts that need to be updated, for example https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/FlowBase.java#L61, maybe other language need to be updated as well, I'm worried about breaking client compatibility, I want to fixed it in the next pr

I'll submit a PR to change the behavior. My main point is that the RFC for this flow (https://datatracker.ietf.org/doc/html/rfc6749#section-4.4.2) does not mention audience. The only IDP I know of that requires the audience field is Auth0. As such, we shouldn't require that audience be set. I don't think it will break client compatibility to demote a field from being required to optional.

I submitted the PR here: #11988

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

michaeljmarshall · 2021-09-07T19:15:31Z

...ent/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationFactoryOAuth2.java

+     * @param audience the audience identifier
+     * @param scope the scope


I think it'd be helpful to update these comments. The audience field seems tied to auth0, not oauth2.0 itself, so you might even reference the auth0 spec: https://auth0.com/docs/authorization/flows/call-your-api-using-the-client-credentials-flow. The scope is an optional part of the oauth2.0 spec defined here: https://datatracker.ietf.org/doc/html/rfc6749#section-4.4.2.

It'd also be helpful to know what is optional and what is required.

Fixed scope, but without changing the audience

Makes sense, thanks.

sijie · 2021-09-08T09:04:56Z

@eolivelli please review @tuteng 's latest change.

tuteng · 2021-09-08T09:09:28Z

/pulsarbot run-failure-checks

codelipenghui · 2021-09-08T10:14:20Z

...client/src/test/java/org/apache/pulsar/client/impl/auth/oauth2/protocol/TokenClientTest.java

+        when(defaultAsyncHttpClient.preparePost(url.toString())).thenReturn(boundRequestBuilder);
+        when(boundRequestBuilder.setHeader("Accept", "application/json")).thenReturn(boundRequestBuilder);
+        when(boundRequestBuilder.setHeader("Content-Type", "application/x-www-form-urlencoded")).thenReturn(boundRequestBuilder);
+        when(boundRequestBuilder.setBody(body)).thenReturn(boundRequestBuilder);


@tuteng Could you please provide more context about this test? I do not fully understand the purpose of this test. Here we only return the boundRequestBuilder if the body equals the body map, but the request always has the scope, so that we will always return null here. But looks like the test will not touch any response handling steps.

The purpose of this test is to test that the scope is correctly encoded in the request body, because it is a unit test, so there is no way to get the real response, In this unit test, only this part of the logic in this function exchangeClientCredentials can be tested

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

because it has no way to actually send an http request to a server, all http-related operations are mocked,
So there is the following mock call:

when(boundRequestBuilder.setBody(body)).thenReturn(boundRequestBuilder);

Note that there is a body that is mocked here, and the test will pass when it has the same value as the body in the following function, otherwise an exception will be thrown, the body in the following function is generated in the exchangeClientCredentials function:

Response res = httpClient.preparePost(tokenUrl.toString()) .setHeader("Accept", "application/json") .setHeader("Content-Type", "application/x-www-form-urlencoded") .setBody(body) .execute() .get();

This is the current way to verify that the scope is encoded correctly

@codelipenghui

I see. Thanks for the explanation

codelipenghui · 2021-09-08T10:16:04Z

...client/src/test/java/org/apache/pulsar/client/impl/auth/oauth2/protocol/TokenClientTest.java

+        bodyMap.put("client_id", request.getClientId());
+        bodyMap.put("client_secret", request.getClientSecret());
+        bodyMap.put("audience", request.getAudience());
+        if (!StringUtils.isBlank(request.getScope())) {


nit: the request.getScope() is always "test-scope", so looks like a unnecessary check.

lhotari

LGTM

eolivelli

LGTM

michaeljmarshall

LGTM

…11931) ### Motivation In some scenarios (e.g. azure cloud), when the client exchanges tokens with the server, an optional scope parameter is required, this pr fixes this issue, to ensure compatibility, when the user does not fill in this parameter, all behavior is the same as before. ### Modifications * Add an optional parameter scope when exchanges token ### Verifying this change - [x] Make sure that the change passes the CI checks. (cherry picked from commit ac5114f)

…ion (#12305) ### Motivation It's a C++ client catchup for #11931. ### Modifications - Add a `scope_` field to `ClientCredentialFlow` and load it from `ParamMap` object whose key is `scope`. - Refactor `ClientCredentialFlow` to simplify code and make it testable: - Use only one constructor instead of two overloaded constructors that might look confused - Add a `generateJsonBody` public method for generating JSON body for post fields in `authenticate` so that it can be tested. - Add a `KeyFile` class like what Java client does to load client id and client secret from `ParamMap` or file. ### Verifying this change - [x] Make sure that the change passes the CI checks. This change added test `AuthPluginTest.testOauth2RequestBody` for the cases that scope exists or doesn't exist.

…ion (#12305) It's a C++ client catchup for #11931. - Add a `scope_` field to `ClientCredentialFlow` and load it from `ParamMap` object whose key is `scope`. - Refactor `ClientCredentialFlow` to simplify code and make it testable: - Use only one constructor instead of two overloaded constructors that might look confused - Add a `generateJsonBody` public method for generating JSON body for post fields in `authenticate` so that it can be tested. - Add a `KeyFile` class like what Java client does to load client id and client secret from `ParamMap` or file. - [x] Make sure that the change passes the CI checks. This change added test `AuthPluginTest.testOauth2RequestBody` for the cases that scope exists or doesn't exist. (cherry picked from commit 44dcc04)

…pache#11931) ### Motivation In some scenarios (e.g. azure cloud), when the client exchanges tokens with the server, an optional scope parameter is required, this pr fixes this issue, to ensure compatibility, when the user does not fill in this parameter, all behavior is the same as before. ### Modifications * Add an optional parameter scope when exchanges token ### Verifying this change - [x] Make sure that the change passes the CI checks.

…ion (apache#12305) ### Motivation It's a C++ client catchup for apache#11931. ### Modifications - Add a `scope_` field to `ClientCredentialFlow` and load it from `ParamMap` object whose key is `scope`. - Refactor `ClientCredentialFlow` to simplify code and make it testable: - Use only one constructor instead of two overloaded constructors that might look confused - Add a `generateJsonBody` public method for generating JSON body for post fields in `authenticate` so that it can be tested. - Add a `KeyFile` class like what Java client does to load client id and client secret from `ParamMap` or file. ### Verifying this change - [x] Make sure that the change passes the CI checks. This change added test `AuthPluginTest.testOauth2RequestBody` for the cases that scope exists or doesn't exist.

Add a optional params scope for pulsar oauth2 client

22432d4

tuteng requested review from jiazhai, merlimat, addisonj, sijie and codelipenghui September 6, 2021 09:38

tuteng self-assigned this Sep 6, 2021

tuteng added release/2.8.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages doc-required Your PR changes impact docs and you will update later. labels Sep 6, 2021

tuteng added this to the 2.9.0 milestone Sep 6, 2021

eolivelli requested changes Sep 7, 2021

View reviewed changes

tuteng added 2 commits September 7, 2021 21:06

Add test for scope field, move httpClient to default

d8e801e

Add license for test file

c1d2657

michaeljmarshall requested changes Sep 7, 2021

View reviewed changes

Fixed comment, add comment

c89abf6

sijie approved these changes Sep 8, 2021

View reviewed changes

codelipenghui reviewed Sep 8, 2021

View reviewed changes

tuteng added 3 commits September 8, 2021 19:08

Fixed scope is empty

c9c732e

Fixed scope check

7025200

Fixed scope is empty

3f28fe3

codelipenghui approved these changes Sep 8, 2021

View reviewed changes

codelipenghui requested review from eolivelli and sijie September 8, 2021 11:16

lhotari approved these changes Sep 8, 2021

View reviewed changes

eolivelli approved these changes Sep 8, 2021

View reviewed changes

michaeljmarshall approved these changes Sep 8, 2021

View reviewed changes

tuteng merged commit ac5114f into apache:master Sep 9, 2021

codelipenghui added cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.7.4 labels Sep 9, 2021

codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Sep 9, 2021

michaeljmarshall mentioned this pull request Sep 9, 2021

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

Merged

lhotari mentioned this pull request Sep 15, 2021

Pulsar client oauth2 client support optional parameter scope #11932

Closed

eolivelli added the release/2.9.0 label Oct 6, 2021

BewareMyPower mentioned this pull request Oct 8, 2021

[C++] Support configuring optional scope field for OAuth2 authentication #12305

Merged

1 task

Anonymitaet mentioned this pull request Oct 15, 2021

[Doc] add scope explanations #12380

Merged

Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Oct 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tuteng commented Sep 6, 2021 •

edited

Loading

tuteng commented Sep 6, 2021

Anonymitaet commented Sep 7, 2021

tuteng commented Sep 7, 2021

eolivelli left a comment

eolivelli Sep 7, 2021

tuteng Sep 7, 2021

michaeljmarshall left a comment

michaeljmarshall Sep 7, 2021

tuteng Sep 8, 2021 •

edited

Loading

michaeljmarshall Sep 8, 2021

michaeljmarshall Sep 9, 2021

michaeljmarshall Sep 7, 2021

tuteng Sep 8, 2021

michaeljmarshall Sep 8, 2021

sijie commented Sep 8, 2021

tuteng commented Sep 8, 2021

codelipenghui Sep 8, 2021

tuteng Sep 8, 2021 •

edited

Loading

codelipenghui Sep 8, 2021

codelipenghui Sep 8, 2021

lhotari left a comment

eolivelli left a comment

michaeljmarshall left a comment

		* @param audience the audience identifier
		* @param scope the scope

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

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

Conversation

tuteng commented Sep 6, 2021 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

tuteng commented Sep 6, 2021

Anonymitaet commented Sep 7, 2021

tuteng commented Sep 7, 2021

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuteng Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sijie commented Sep 8, 2021

tuteng commented Sep 8, 2021

Choose a reason for hiding this comment

tuteng Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhotari left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

tuteng commented Sep 6, 2021 •

edited

Loading

tuteng Sep 8, 2021 •

edited

Loading

tuteng Sep 8, 2021 •

edited

Loading