Skip to content
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

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

Merged

Conversation

BewareMyPower
Copy link
Contributor

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

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

@BewareMyPower BewareMyPower added component/c++ doc-not-needed Your PR changes do not impact docs labels Oct 8, 2021
@BewareMyPower BewareMyPower self-assigned this Oct 8, 2021
@BewareMyPower BewareMyPower merged commit 44dcc04 into apache:master Oct 9, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/cpp-oauth2-scope branch October 9, 2021 00:14
@tuteng tuteng added this to the 2.9.0 milestone Oct 9, 2021
@BewareMyPower BewareMyPower added doc-required Your PR changes impact docs and you will update later. and removed doc-not-needed Your PR changes do not impact docs labels Oct 9, 2021
@tuteng tuteng added doc-not-needed Your PR changes do not impact docs release/2.8.2 and removed doc-required Your PR changes impact docs and you will update later. doc-not-needed Your PR changes do not impact docs labels Oct 9, 2021
@BewareMyPower BewareMyPower added the doc-required Your PR changes impact docs and you will update later. label Oct 9, 2021
tuteng pushed a commit that referenced this pull request Oct 11, 2021
…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)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Oct 11, 2021
codelipenghui pushed a commit that referenced this pull request Oct 11, 2021
…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)
@Anonymitaet Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Oct 18, 2021
@Anonymitaet Anonymitaet added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Oct 28, 2021
@github-actions
Copy link

@BewareMyPower:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-label-missing labels Nov 11, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life doc-complete Your PR changes impact docs and the related docs have been already added. release/2.8.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants