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.

Sign up for GitHub

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

Jump to bottom

[C++] Handle OAuth 2.0 exceptional cases gracefully #12335

Merged
merged 9 commits into from
Oct 13, 2021
Merged

[C++] Handle OAuth 2.0 exceptional cases gracefully #12335

merged 9 commits into from
Oct 13, 2021

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Oct 12, 2021 •
edited
Loading

Fixes #12324

Motivation

Currently if any error happened during OAuth 2.0 authentication in C++ client, a runtime error would be thrown and could only be caught when creating an AuthOauth object, but could not be caught in Client's method like createProducer. It's not graceful. What's worse, there's no way for Python client that is a wrapper of C++ client to caught this exception.

Modifications

When ClientCredentialFlow::authenticate returns an invalid Oauth2TokenResult, catch the runtime_error thrown in Oauth2CachedToken's constructor and returns ResultAuthenticationError as AuthOauth2::getAuthData's returned value. Since getAuthData always returns ResultOk before this PR, the related docs are also modified.

Then when a CONNECT or AUTH_RESPONSE command is created, expose the result of getAuthData. If it's not ResultOk, close the connection and complete the connection's future with the result. After that, the Client's API will be completed with the result.

In addition, this PR also makes the error code of libcurl human readable by configuring CURLOPT_ERRORBUFFER.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests AuthPluginTest.testOauth2Failure to verify when OAuth 2.0 authentication failed, the createProducer would return ResultAuthenticationError without any exception thrown.

@BewareMyPower BewareMyPower self-assigned this Oct 12, 2021
@BewareMyPower BewareMyPower added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Oct 12, 2021
@BewareMyPower BewareMyPower merged commit 06b68bb into apache:master Oct 13, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/cpp-oauth-error branch October 13, 2021 03:05
tuteng pushed a commit to AmateurEvents/pulsar that referenced this pull request Oct 13, 2021
@BewareMyPower @tuteng
[C++] Handle OAuth 2.0 exceptional cases gracefully (apache#12335)
522af14 
Fixes apache#12324

Currently if any error happened during OAuth 2.0 authentication in C++ client, a runtime error would be thrown and could only be caught when creating an `AuthOauth` object, but could not be caught in `Client`'s method like `createProducer`. It's not graceful. What's worse, there's no way for Python client that is a wrapper of C++ client to caught this exception.

When `ClientCredentialFlow::authenticate` returns an invalid `Oauth2TokenResult`, catch the `runtime_error` thrown in `Oauth2CachedToken`'s constructor and returns `ResultAuthenticationError` as `AuthOauth2::getAuthData`'s returned value. Since `getAuthData` always returns `ResultOk` before this PR, the related docs are also modified.

Then when a CONNECT or AUTH_RESPONSE command is created, expose the result of `getAuthData`. If it's not `ResultOk`, close the connection and complete the connection's future with the result. After that, the `Client`'s API will be completed with the result.

In addition, this PR also makes the error code of libcurl human readable by configuring `CURLOPT_ERRORBUFFER`.

- [x] Make sure that the change passes the CI checks.

This change added tests `AuthPluginTest.testOauth2Failure` to verify when OAuth 2.0 authentication failed, the `createProducer` would return `ResultAuthenticationError` without any exception thrown.

(cherry picked from commit 06b68bb)
@hangc0276 hangc0276 added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Oct 13, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Oct 14, 2021
Merge branch 'up/master' into website/branch-2.7.2-chapter-2
8b9e28c 
* up/master: (26 commits)
  [pulsar-admin] Allow setting --forward-source-message-property to false when updating a pulsar function (apache#12128)
  [website][upgrade]feat: docs migration - Development (apache#12320)
  Update delete inactive topic configuration documentation (apache#12350)
  [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol (apache#12056)
  Added Debezium Source for MS SQL Server (apache#12256)
  Fix: flaky oracle tests (apache#12306)
  [C++] Use URL encoded content type for OAuth 2.0 authentication (apache#12341)
  [C++] Handle OAuth 2.0 exceptional cases gracefully (apache#12335)
  feat(cli): add restart command to pulsar-daemon (apache#12279)
  [client-tools] Remove redundant initial value (apache#12296)
  Make AuthenticationTokenTest to run on windows (apache#12329)
  [offload] fix FileSystemManagedLedgerOffloader can not cleanup outdated ledger data (apache#12309)
  [Doc]--Update contents for Pulsar adaptor for Apache Spark (apache#12338)
  [PIP 95][Issue 12040][broker] Improved multi-listener in standalone mode (apache#12066)
  [website][upgrade]feat: docs migration - Cookbooks (apache#12319)
  [testclient] Make --payload-file take effect in PerformanceClient (apache#12187)
  [website][upgrade]feat: docs migration - adaptor (apache#12318)
  [pulsar-client] Add partition-change api for producer/consumer interceptors (apache#12287)
  [Transaction]Fix lowWaterMark of TopicTransactionBuffer (apache#12312)
  [pulsar-admin] New option takes precedence over deprecated option (apache#12260)
  ...

# Conflicts:
#	site2/website-next/docusaurus.config.js
#	site2/website-next/versions.json
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Oct 14, 2021
Merge branch 'up/master' into website/branch-2.7.2
82208c9 
* up/master: (37 commits)
  re-enabling integration tests for Sinks (apache#12307)
  [PIP 95][Issue 12040][web] Topic lookup with listener header (apache#12072)
  Fix the master CI broken with update dispatch rate block issue (apache#12360)
  Fix message being ignored when the non-persistent topic reader reconnect. (apache#12348)
  Fix log format. (apache#12346)
  [website][upgrade]feat: docs migration - version-2.7.2 Concepts and Architecture (apache#12354)
  [website][upgrade] feat: full docs migration for version 2.8.0 (apache#12359)
  [website][upgrade]feat: dynamic replace version info before build (apache#12337)
  Fix flaky tests: ElasticSearchClientTests (apache#12347)
  Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete (apache#12113)
  fix-npe-ZkBookieRackAffinityMapping (apache#11947)
  [pulsar-admin] Allow setting --forward-source-message-property to false when updating a pulsar function (apache#12128)
  [website][upgrade]feat: docs migration - Development (apache#12320)
  Update delete inactive topic configuration documentation (apache#12350)
  [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol (apache#12056)
  Added Debezium Source for MS SQL Server (apache#12256)
  Fix: flaky oracle tests (apache#12306)
  [C++] Use URL encoded content type for OAuth 2.0 authentication (apache#12341)
  [C++] Handle OAuth 2.0 exceptional cases gracefully (apache#12335)
  feat(cli): add restart command to pulsar-daemon (apache#12279)
  ...

# Conflicts:
#	site2/website-next/docusaurus.config.js
#	site2/website-next/versioned_sidebars/version-2.7.2-sidebars.json
#	site2/website-next/versions.json
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@BewareMyPower @ciaocloud
[C++] Handle OAuth 2.0 exceptional cases gracefully (apache#12335)
b159b68 
Fixes apache#12324

### Motivation

Currently if any error happened during OAuth 2.0 authentication in C++ client, a runtime error would be thrown and could only be caught when creating an `AuthOauth` object, but could not be caught in `Client`'s method like `createProducer`. It's not graceful. What's worse, there's no way for Python client that is a wrapper of C++ client to caught this exception.

### Modifications

When `ClientCredentialFlow::authenticate` returns an invalid `Oauth2TokenResult`, catch the `runtime_error` thrown in `Oauth2CachedToken`'s constructor and returns `ResultAuthenticationError` as `AuthOauth2::getAuthData`'s returned value. Since `getAuthData` always returns `ResultOk` before this PR, the related docs are also modified.

Then when a CONNECT or AUTH_RESPONSE command is created, expose the result of `getAuthData`. If it's not `ResultOk`, close the connection and complete the connection's future with the result. After that, the `Client`'s API will be completed with the result.

In addition, this PR also makes the error code of libcurl human readable by configuring `CURLOPT_ERRORBUFFER`.

### Verifying this change

- [x] Make sure that the change passes the CI checks.

This change added tests `AuthPluginTest.testOauth2Failure` to verify when OAuth 2.0 authentication failed, the `createProducer` would return `ResultAuthenticationError` without any exception thrown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuteng tuteng tuteng approved these changes

@merlimat merlimat merlimat approved these changes

@jiazhai jiazhai Awaiting requested review from jiazhai

@aahmed-se aahmed-se Awaiting requested review from aahmed-se

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@sijie sijie Awaiting requested review from sijie

Assignees

@BewareMyPower BewareMyPower

Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/2.8.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C++] Segfault and terrible error handling when OAuth 2.0 authentication failed
4 participants
@BewareMyPower @merlimat @tuteng @hangc0276