Skip to content

Conversation

@gaoran10
Copy link
Contributor

@gaoran10 gaoran10 commented Jun 4, 2022

Motivation

Currently, the well-known OpenID configuration URL generated by issuer_url and suffix (/.well-known/openid-configuration), if the issurer_url end with a slash, the well-known OpenID configuration URL will be not correct.

Demo

issuer_url: https://dev-kt-aa9ne.us.auth0.com/
openid_url: https://dev-kt-aa9ne.us.auth0.com//.well-known/openid-configuration

Making the issuer_url as below could work well.

issuer_url: https://dev-kt-aa9ne.us.auth0.com/
issuer_url: https://dev-kt-aa9ne.us.auth0.com

Modifications

Trim the slash if the issuer_url end with a slash.

Verifying this change

Add a unit test to verify the method ClientCredentialFlow::initialize.

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): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@gaoran10 gaoran10 self-assigned this Jun 4, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 4, 2022
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTGM

Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In C++, returning a const object is meaningless. I think you want to make the method const to ensure the internal state doesn't change in the implementation.

@gaoran10 gaoran10 changed the title [fix][auth] Generate correct well-known OpenID configuration URL [WIP] [fix][auth] Generate correct well-known OpenID configuration URL Jun 6, 2022
…the method `ClientCredentialFlow::initialize`.
@gaoran10 gaoran10 changed the title [WIP] [fix][auth] Generate correct well-known OpenID configuration URL [fix][auth] Generate correct well-known OpenID configuration URL Jun 6, 2022
@BewareMyPower BewareMyPower added type/bug The PR fixed a bug or issue reported a bug release/2.9.3 release/2.8.4 release/2.10.1 labels Jun 6, 2022
@BewareMyPower
Copy link
Contributor

Please apply the following patch to fix the code format.

diff --git a/pulsar-client-cpp/lib/auth/AuthOauth2.cc b/pulsar-client-cpp/lib/auth/AuthOauth2.cc
index 4bc5e8c814b2..c7f944da75b1 100644
--- a/pulsar-client-cpp/lib/auth/AuthOauth2.cc
+++ b/pulsar-client-cpp/lib/auth/AuthOauth2.cc
@@ -143,9 +143,7 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
       audience_(params["audience"]),
       scope_(params["scope"]) {}
 
-std::string ClientCredentialFlow::getTokenEndPoint() const {
-    return tokenEndPoint_;
-}
+std::string ClientCredentialFlow::getTokenEndPoint() const { return tokenEndPoint_; }
 
 static size_t curlWriteCallback(void* contents, size_t size, size_t nmemb, void* responseDataPtr) {
     ((std::string*)responseDataPtr)->append((char*)contents, size * nmemb);
@@ -177,7 +175,7 @@ void ClientCredentialFlow::initialize() {
         wellKnownUrl.pop_back();
     }
     wellKnownUrl.append("/.well-known/openid-configuration");
-    curl_easy_setopt(handle, CURLOPT_URL,wellKnownUrl.c_str());
+    curl_easy_setopt(handle, CURLOPT_URL, wellKnownUrl.c_str());
 
     // Write callback
     curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, curlWriteCallback);

@codelipenghui codelipenghui merged commit 304b03e into apache:master Jun 7, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jun 7, 2022
codelipenghui pushed a commit that referenced this pull request Jun 10, 2022
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jun 10, 2022
codelipenghui pushed a commit that referenced this pull request Jun 12, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 13, 2022
BewareMyPower pushed a commit that referenced this pull request Jul 27, 2022
@BewareMyPower BewareMyPower added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/authn area/security cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.8.4 release/2.9.3 release/2.10.1 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants