nicoloboschi commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073208937
##########
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
##########
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:
what about throwing an exception in order to let the caller code to catch
it?
In this case there's the exactly same code in case of Exception
https://github.com/apache/pulsar/blob/a0e603a3eaf822cb0c41f292ad5845ee54df0ccf/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L968-L973
--
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]
