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 2021/09/22 10:55:24 UTC
[GitHub] [pulsar] RobertIndie opened a new pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
RobertIndie opened a new pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132
### Motivation
Currently, the setting prefix for JWT authentication does not work because the code does not specify the property name for the token setting prefix.
### Modifications
* Add token setting prefix property name: `tokenSettingPrefix`.
* Avoid multi calls for the getProperty in JWT authn.
### Verifying this change
This change is already covered by the existing test, such as *testTokenSettingPrefix*.
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [x] `doc-required`
--
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] 315157973 commented on a change in pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#discussion_r736127056
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -54,7 +54,7 @@
static final String HTTP_HEADER_VALUE_PREFIX = "Bearer ";
// When symmetric key is configured
- static final String CONF_TOKEN_SETTING_PREFIX = "";
+ static final String CONF_TOKEN_SETTING_PREFIX = "tokenSettingPrefix";
Review comment:
Will there be compatibility issues here? After the upgrade, the previous token cannot be used
--
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] 315157973 merged pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
315157973 merged pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132
--
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] RobertIndie commented on pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#issuecomment-925562469
> @RobertIndie 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.
I have created a PR: #12152 for doc. PTAL
--
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] RobertIndie commented on a change in pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#discussion_r740209089
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -54,7 +54,7 @@
static final String HTTP_HEADER_VALUE_PREFIX = "Bearer ";
// When symmetric key is configured
- static final String CONF_TOKEN_SETTING_PREFIX = "";
+ static final String CONF_TOKEN_SETTING_PREFIX = "tokenSettingPrefix";
Review comment:
No compatibility issues are introduced here. The `tokenSettingPrefix` is empty by default.
--
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] 315157973 commented on pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#issuecomment-956132219
@RobertIndie PTAL
--
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] 315157973 merged pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
315157973 merged pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132
--
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] RobertIndie commented on pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#issuecomment-925683140
/pulsarbot run-failure-checks
--
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] RobertIndie commented on pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#issuecomment-948174595
/pulsarbot run-failure-checks
--
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] 315157973 merged pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
315157973 merged pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132
--
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 commented on a change in pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#discussion_r719467355
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -293,19 +293,19 @@ private SignatureAlgorithm getPublicKeyAlgType(ServiceConfiguration conf) throws
// get Token Audience Claim from configuration, if not configured return null.
private String getTokenAudienceClaim(ServiceConfiguration conf) throws IllegalArgumentException {
- if (conf.getProperty(confTokenAudienceClaimSettingName) != null
- && StringUtils.isNotBlank((String) conf.getProperty(confTokenAudienceClaimSettingName))) {
- return (String) conf.getProperty(confTokenAudienceClaimSettingName);
+ Object tokenAudienceClaim = conf.getProperty(confTokenAudienceClaimSettingName);
+ if (tokenAudienceClaim != null && StringUtils.isNotBlank((String) tokenAudienceClaim)) {
+ return (String) tokenAudienceClaim;
} else {
return null;
}
}
// get Token Audience that stands for this broker from configuration, if not configured return null.
private String getTokenAudience(ServiceConfiguration conf) throws IllegalArgumentException {
- if (conf.getProperty(confTokenAudienceSettingName) != null
- && StringUtils.isNotBlank((String) conf.getProperty(confTokenAudienceSettingName))) {
- return (String) conf.getProperty(confTokenAudienceSettingName);
+ Object tokenAudience = conf.getProperty(confTokenAudienceSettingName);
Review comment:
Same as the above comment
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -293,19 +293,19 @@ private SignatureAlgorithm getPublicKeyAlgType(ServiceConfiguration conf) throws
// get Token Audience Claim from configuration, if not configured return null.
private String getTokenAudienceClaim(ServiceConfiguration conf) throws IllegalArgumentException {
- if (conf.getProperty(confTokenAudienceClaimSettingName) != null
- && StringUtils.isNotBlank((String) conf.getProperty(confTokenAudienceClaimSettingName))) {
- return (String) conf.getProperty(confTokenAudienceClaimSettingName);
+ Object tokenAudienceClaim = conf.getProperty(confTokenAudienceClaimSettingName);
+ if (tokenAudienceClaim != null && StringUtils.isNotBlank((String) tokenAudienceClaim)) {
+ return (String) tokenAudienceClaim;
Review comment:
```suggestion
String tokenAudienceClaim = (String) conf.getProperty(confTokenAudienceClaimSettingName);
if (tokenAudienceClaim != null && StringUtils.isNotBlank(tokenAudienceClaim)) {
return tokenAudienceClaim;
```
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -293,19 +293,19 @@ private SignatureAlgorithm getPublicKeyAlgType(ServiceConfiguration conf) throws
// get Token Audience Claim from configuration, if not configured return null.
private String getTokenAudienceClaim(ServiceConfiguration conf) throws IllegalArgumentException {
- if (conf.getProperty(confTokenAudienceClaimSettingName) != null
- && StringUtils.isNotBlank((String) conf.getProperty(confTokenAudienceClaimSettingName))) {
- return (String) conf.getProperty(confTokenAudienceClaimSettingName);
+ Object tokenAudienceClaim = conf.getProperty(confTokenAudienceClaimSettingName);
+ if (tokenAudienceClaim != null && StringUtils.isNotBlank((String) tokenAudienceClaim)) {
+ return (String) tokenAudienceClaim;
Review comment:
We should avoid cast to String twice.
--
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] RobertIndie commented on a change in pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#discussion_r740209089
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -54,7 +54,7 @@
static final String HTTP_HEADER_VALUE_PREFIX = "Bearer ";
// When symmetric key is configured
- static final String CONF_TOKEN_SETTING_PREFIX = "";
+ static final String CONF_TOKEN_SETTING_PREFIX = "tokenSettingPrefix";
Review comment:
No compatibility issues will be introduced here. The `tokenSettingPrefix` is empty by default.
--
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] RobertIndie commented on a change in pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#discussion_r731791613
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -293,19 +293,19 @@ private SignatureAlgorithm getPublicKeyAlgType(ServiceConfiguration conf) throws
// get Token Audience Claim from configuration, if not configured return null.
private String getTokenAudienceClaim(ServiceConfiguration conf) throws IllegalArgumentException {
- if (conf.getProperty(confTokenAudienceClaimSettingName) != null
- && StringUtils.isNotBlank((String) conf.getProperty(confTokenAudienceClaimSettingName))) {
- return (String) conf.getProperty(confTokenAudienceClaimSettingName);
+ Object tokenAudienceClaim = conf.getProperty(confTokenAudienceClaimSettingName);
+ if (tokenAudienceClaim != null && StringUtils.isNotBlank((String) tokenAudienceClaim)) {
+ return (String) tokenAudienceClaim;
Review comment:
Thanks. I fixed it.
--
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] Anonymitaet commented on pull request #12132: [Broker] Fix prefix setting in JWT authn and avoid multi calls for the getProperty
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #12132:
URL: https://github.com/apache/pulsar/pull/12132#issuecomment-925424490
@RobertIndie 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.
--
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