nodece commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073283182
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -395,17 +395,32 @@ public void exceptionCaught(ChannelHandlerContext ctx,
Throwable cause) throws E
ctx.close();
}
- /*
- * If authentication and authorization is enabled(and not sasl) and
- * if the authRole is one of proxyRoles we want to enforce
- * - the originalPrincipal is given while connecting
- * - originalPrincipal is not blank
- * - originalPrincipal is not a proxy principal
+ /**
+ * When transitioning from Connecting to Connected, this class validates
that the {@link #authRole} and the
+ * {@link #originalPrincipal} are a valid combination. Valid combinations
fulfill the following rule:
+ * <p>
+ * The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the
{@link #originalPrincipal} is set to a role
+ * that is not also in {@link #proxyRoles}.
*/
- private boolean invalidOriginalPrincipal(String originalPrincipal) {
- return (service.isAuthenticationEnabled() &&
service.isAuthorizationEnabled()
- && proxyRoles.contains(authRole) &&
(StringUtils.isBlank(originalPrincipal)
- || proxyRoles.contains(originalPrincipal)));
+ private void validateRoleAndOriginalPrincipal() {
+ String errorMsg = null;
+ if (proxyRoles.contains(authRole)) {
+ if (StringUtils.isBlank(originalPrincipal)) {
+ errorMsg = "originalPrincipal must be provided when connecting
with a proxy role.";
+ } else if (proxyRoles.contains(originalPrincipal)) {
+ errorMsg = "originalPrincipal cannot be a proxy role.";
+ }
+ } else if (StringUtils.isNotBlank(originalPrincipal)) {
+ errorMsg = "cannot specify originalPrincipal when connecting
without valid proxy role.";
+ }
+ if (errorMsg != null) {
+ service.getPulsarStats().recordConnectionCreateFail();
Review Comment:
I agreed with @nicoloboschi.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -395,17 +395,32 @@ public void exceptionCaught(ChannelHandlerContext ctx,
Throwable cause) throws E
ctx.close();
}
- /*
- * If authentication and authorization is enabled(and not sasl) and
- * if the authRole is one of proxyRoles we want to enforce
- * - the originalPrincipal is given while connecting
- * - originalPrincipal is not blank
- * - originalPrincipal is not a proxy principal
+ /**
+ * When transitioning from Connecting to Connected, this class validates
that the {@link #authRole} and the
+ * {@link #originalPrincipal} are a valid combination. Valid combinations
fulfill the following rule:
+ * <p>
+ * The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the
{@link #originalPrincipal} is set to a role
+ * that is not also in {@link #proxyRoles}.
*/
- private boolean invalidOriginalPrincipal(String originalPrincipal) {
- return (service.isAuthenticationEnabled() &&
service.isAuthorizationEnabled()
- && proxyRoles.contains(authRole) &&
(StringUtils.isBlank(originalPrincipal)
- || proxyRoles.contains(originalPrincipal)));
+ private void validateRoleAndOriginalPrincipal() {
Review Comment:
See line 837, checked.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
