michaeljmarshall commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073818425
##########
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:
> one note is the previous code perform the checks only if authorization is
also enabled, but I think it was wrong since these checks are about
authentication
Good point. The code path checks for authentication but not authorization.
I'll fix that. (As an aside, I wonder who is running with authentication and
without authorization.)
--
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]
