You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/06/04 04:07:41 UTC
[GitHub] [pulsar] gaoran10 opened a new pull request, #15928: [fix][auth] Generate correct well-known OpenID configuration URL
gaoran10 opened a new pull request, #15928:
URL: https://github.com/apache/pulsar/pull/15928
### 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 generate the correct OpenID configuration URL.
### 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)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889707814
##########
pulsar-client-cpp/lib/auth/AuthOauth2.cc:
##########
@@ -143,6 +143,18 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
audience_(params["audience"]),
scope_(params["scope"]) {}
+const std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() {
Review Comment:
In C++, returning a const value is meaningless, for example, given the following function that returns a const `std::string`
```c++
const std::string f() { return "hello"; }
```
Then we have:
```c++
// It's an assignment by value, i.e. deep copy in Java, so s1 is non-const
auto s1 = f();
s1.append("world"); // OK because the type of s1 is non-const
// s2 is a const reference, but it's dangerous in C++ because f() returns a temporary variable.
auto& s2 = f();
// s2.append("world"); // Failed because s2 references to a const object
```
It's dangerous to use `s2` in the above example because it references a temporary variable. See following example:
```c++
#include <iostream>
using namespace std;
const std::string f() { return "hello"; }
const std::string& g() {
auto& s = f();
return s;
}
int main(int argc, char* argv[]) {
auto& s = g();
cout << s << endl;
return 0;
}
```
When you compile this program, you can see the warning from the Clang output:
```
1.cc:8:12: warning: returning reference to local temporary object [-Wreturn-stack-address]
return s;
^
1.cc:7:11: note: binding reference variable 's' here
auto& s = f();
```
And the output is undefined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889708710
##########
pulsar-client-cpp/lib/auth/AuthOauth2.cc:
##########
@@ -143,6 +143,18 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
audience_(params["audience"]),
scope_(params["scope"]) {}
+const std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() {
Review Comment:
BTW, usually, we should mark all methods that don't change the internal state as `const`. For example,
```c++
struct A {
int x;
int getX() {
x++; // a getter should not modify the internal state, but the compilation can pass
return x;
}
};
```
In this case, you should make `getX` a `const` method like:
```c++
int getX() const {
// x++; // Compilation error! Because it changes the value of the field of A
return 2;
}
```
The const qualifier makes your code clear (because we can know which methods don't change the state) and prevent unexpected changes (for example, modifying the state in a getter).
See https://google.github.io/styleguide/cppguide.html#Use_of_const
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889702235
##########
pulsar-client-cpp/tests/AuthPluginTest.cc:
##########
@@ -412,6 +412,20 @@ TEST(AuthPluginTest, testOauth2RequestBody) {
ASSERT_EQ(flow2.generateParamMap(), expectedResult2);
}
+TEST(AuthPluginTest, getWellKnownOpenIdConfigurationUrl) {
+ std::string issuerUrl = "https://dev-kt-aa9ne.us.auth0.com";
Review Comment:
Do you mean the case as flow2?
```
params["issuer_url"] = issuerUrl.append("/");
ClientCredentialFlow flow2(params);
ASSERT_EQ(flow2.getWellKnownOpenIdConfigurationUrl(), configurationUrl);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889708044
##########
pulsar-client-cpp/tests/AuthPluginTest.cc:
##########
@@ -412,6 +412,20 @@ TEST(AuthPluginTest, testOauth2RequestBody) {
ASSERT_EQ(flow2.generateParamMap(), expectedResult2);
}
+TEST(AuthPluginTest, getWellKnownOpenIdConfigurationUrl) {
+ std::string issuerUrl = "https://dev-kt-aa9ne.us.auth0.com";
Review Comment:
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889702560
##########
pulsar-client-cpp/lib/auth/AuthOauth2.cc:
##########
@@ -143,6 +143,18 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
audience_(params["audience"]),
scope_(params["scope"]) {}
+const std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() {
Review Comment:
I want the return value not to be changed. Maybe we don't need the const qualifier here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889495548
##########
pulsar-client-cpp/lib/auth/AuthOauth2.h:
##########
@@ -58,6 +58,7 @@ class ClientCredentialFlow : public Oauth2Flow {
void close();
ParamMap generateParamMap() const;
+ const std::string getWellKnownOpenIdConfigurationUrl();
Review Comment:
```suggestion
std::string getWellKnownOpenIdConfigurationUrl() const;
```
##########
pulsar-client-cpp/lib/auth/AuthOauth2.cc:
##########
@@ -143,6 +143,18 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
audience_(params["audience"]),
scope_(params["scope"]) {}
+const std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() {
Review Comment:
```suggestion
std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() const {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889708710
##########
pulsar-client-cpp/lib/auth/AuthOauth2.cc:
##########
@@ -143,6 +143,18 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
audience_(params["audience"]),
scope_(params["scope"]) {}
+const std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() {
Review Comment:
BTW, usually, we should mark all methods that don't change the internal state as `const`. For example,
```c++
struct A {
int x;
int getX() {
x++; // a getter should not modify the internal state, but the compilation can pass
return x;
}
};
```
In this case, you should make `getX` a `const` method like:
```c++
int getX() const {
// x++; // Compilation error! Because it changes a field of A
return 2;
}
```
The const qualifier makes your code clear (because we can know which methods don't change the state) and prevent unexpected changes (for example, modifying the state in a getter).
See https://google.github.io/styleguide/cppguide.html#Use_of_const
##########
pulsar-client-cpp/lib/auth/AuthOauth2.cc:
##########
@@ -143,6 +143,18 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
audience_(params["audience"]),
scope_(params["scope"]) {}
+const std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() {
Review Comment:
BTW, usually, we should mark all methods that don't change the internal state as `const`. For example,
```c++
struct A {
int x;
int getX() {
x++; // a getter should not modify the internal state, but the compilation can pass
return x;
}
};
```
In this case, you should make `getX` a `const` method like:
```c++
int getX() const {
// x++; // Compilation error! Because it changes a field of A
return x;
}
```
The const qualifier makes your code clear (because we can know which methods don't change the state) and prevent unexpected changes (for example, modifying the state in a getter).
See https://google.github.io/styleguide/cppguide.html#Use_of_const
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889707814
##########
pulsar-client-cpp/lib/auth/AuthOauth2.cc:
##########
@@ -143,6 +143,18 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
audience_(params["audience"]),
scope_(params["scope"]) {}
+const std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() {
Review Comment:
In C++, returning a const value is meaningless, for example, given the following function that returns a const `std::string`
```c++
const std::string f() { return "hello"; }
```
Then we have:
```c++
// It's an assignment, i.e. deep copy in Java, so s1 is non-const
auto s1 = f();
s1.append("world"); // OK because the type of s1 is non-const
// s2 is a const reference, but it's dangerous in C++ because f() returns a temporary variable.
auto& s2 = f();
// s2.append("world"); // Failed because s2 references to a const object
```
It's dangerous to use `s2` in the above example because it references a temporary variable. See following example:
```c++
#include <iostream>
using namespace std;
const std::string f() { return "hello"; }
const std::string& g() {
auto& s = f();
return s;
}
int main(int argc, char* argv[]) {
auto& s = g();
cout << s << endl;
return 0;
}
```
When you compile this program, you can see the warning from the Clang output:
```
1.cc:8:12: warning: returning reference to local temporary object [-Wreturn-stack-address]
return s;
^
1.cc:7:11: note: binding reference variable 's' here
auto& s = f();
```
And the output is undefined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889707814
##########
pulsar-client-cpp/lib/auth/AuthOauth2.cc:
##########
@@ -143,6 +143,18 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
audience_(params["audience"]),
scope_(params["scope"]) {}
+const std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() {
Review Comment:
In C++, returning a const value is meaningless, for example, given the following function that returns a const `std::string`
```c++
const std::string f() { return "hello"; }
```
Then we have:
```c++
// It's an assignment by value, i.e. deep copy in Java, so s1 is non-const
auto s1 = f();
s1.append("world"); // OK because the type of s1 is non-const
// s2 is a const reference, but it's dangerous in C++ because f() returns a temporary variable.
auto& s2 = f();
// s2.append("world"); // Failed because s2 references to a const object
// If we want the returned value is const, the only way is adding the const to the type qualifier of the returned value
const auto s3 = f(); // NOTE: it's not determined by whether f() returns a const value
// s3.append("world"); // Failed because s3 is const
```
It's dangerous to use `s2` in the above example because it references a temporary variable. See following example:
```c++
#include <iostream>
using namespace std;
const std::string f() { return "hello"; }
const std::string& g() {
auto& s = f();
return s;
}
int main(int argc, char* argv[]) {
auto& s = g();
cout << s << endl;
return 0;
}
```
When you compile this program, you can see the warning from the Clang output:
```
1.cc:8:12: warning: returning reference to local temporary object [-Wreturn-stack-address]
return s;
^
1.cc:7:11: note: binding reference variable 's' here
auto& s = f();
```
And the output is undefined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#issuecomment-1147251581
Please apply the following patch to fix the code format.
```diff
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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889471079
##########
pulsar-client-cpp/tests/AuthPluginTest.cc:
##########
@@ -412,6 +412,20 @@ TEST(AuthPluginTest, testOauth2RequestBody) {
ASSERT_EQ(flow2.generateParamMap(), expectedResult2);
}
+TEST(AuthPluginTest, getWellKnownOpenIdConfigurationUrl) {
+ std::string issuerUrl = "https://dev-kt-aa9ne.us.auth0.com";
Review Comment:
Nit: shouldn't we also test the case where there is a trailing `/` in the `issuerUrl`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889495341
##########
pulsar-client-cpp/tests/AuthPluginTest.cc:
##########
@@ -412,6 +412,20 @@ TEST(AuthPluginTest, testOauth2RequestBody) {
ASSERT_EQ(flow2.generateParamMap(), expectedResult2);
}
+TEST(AuthPluginTest, getWellKnownOpenIdConfigurationUrl) {
+ std::string issuerUrl = "https://dev-kt-aa9ne.us.auth0.com";
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #15928:
URL: https://github.com/apache/pulsar/pull/15928
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a diff in pull request #15928: [fix][auth] Generate correct well-known OpenID configuration URL
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15928:
URL: https://github.com/apache/pulsar/pull/15928#discussion_r889781947
##########
pulsar-client-cpp/lib/auth/AuthOauth2.cc:
##########
@@ -143,6 +143,18 @@ ClientCredentialFlow::ClientCredentialFlow(ParamMap& params)
audience_(params["audience"]),
scope_(params["scope"]) {}
+const std::string ClientCredentialFlow::getWellKnownOpenIdConfigurationUrl() {
Review Comment:
Thanks for your explanation, I'll fix this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org