[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)

["michaeljmarshall (via GitHub)" <gi...@apache.org>]

michaeljmarshall commented on code in PR #19820:
URL: https://github.com/apache/pulsar/pull/19820#discussion_r1137209272


##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -292,53 +293,19 @@ public CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName,
         return provider.allowSinkOpsAsync(namespaceName, role, authenticationData);
     }
 
-    /**
-     * Whether the authenticatedPrincipal and the originalPrincipal form a valid pair. This method assumes that
-     * authenticatedPrincipal and originalPrincipal can be equal, as long as they are not a proxy role. This use
-     * case is relvant for the admin server because of the way the proxy handles authentication. The binary protocol
-     * should not use this method.
-     * @return true when roles are a valid combination and false when roles are an invalid combination
-     */
-    public boolean isValidOriginalPrincipal(String authenticatedPrincipal,
-                                            String originalPrincipal,
-                                            AuthenticationDataSource authDataSource) {
-        SocketAddress remoteAddress = authDataSource != null ? authDataSource.getPeerAddress() : null;
-        return isValidOriginalPrincipal(authenticatedPrincipal, originalPrincipal, remoteAddress, true);
-    }
-
-    /**
-     * Validates that the authenticatedPrincipal and the originalPrincipal are a valid combination.
-     * Valid combinations fulfill one of the following two rules:
-     * <p>
-     * 1. The authenticatedPrincipal is in {@link ServiceConfiguration#getProxyRoles()}, if, and only if,
-     * the originalPrincipal is set to a role that is not also in {@link ServiceConfiguration#getProxyRoles()}.
-     * <p>
-     * 2. The authenticatedPrincipal and the originalPrincipal are the same, but are not a proxyRole, when
-     * allowNonProxyPrincipalsToBeEqual is true.
-     *
-     * @return true when roles are a valid combination and false when roles are an invalid combination
-     */
-    public boolean isValidOriginalPrincipal(String authenticatedPrincipal,
-                                            String originalPrincipal,
-                                            SocketAddress remoteAddress,
-                                            boolean allowNonProxyPrincipalsToBeEqual) {
-        String errorMsg = null;
-        if (conf.getProxyRoles().contains(authenticatedPrincipal)) {
+    private static void validateOriginalPrincipal(Set<String> proxyRoles, String authenticatedPrincipal,
+                                                  String originalPrincipal) {
+        if (proxyRoles.contains(authenticatedPrincipal)) {
+            // Request has come from a proxy
             if (StringUtils.isBlank(originalPrincipal)) {
-                errorMsg = "originalPrincipal must be provided when connecting with a proxy role.";
-            } else if (conf.getProxyRoles().contains(originalPrincipal)) {
-                errorMsg = "originalPrincipal cannot be a proxy role.";
+                log.warn("Original principal empty in request authenticated as {}", authenticatedPrincipal);
+                throw new RestException(Response.Status.UNAUTHORIZED, "Original principal cannot be empty if the "
+                        + "request is via proxy.");
+            }
+            if (proxyRoles.contains(originalPrincipal)) {
+                log.warn("Original principal {} cannot be a proxy role ({})", originalPrincipal, proxyRoles);
+                throw new RestException(Response.Status.UNAUTHORIZED, "Original principal cannot be a proxy role");
             }
-        } else if (StringUtils.isNotBlank(originalPrincipal)
-                && !(allowNonProxyPrincipalsToBeEqual && originalPrincipal.equals(authenticatedPrincipal))) {
-            errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role.";
-        }

Review Comment:
   These are the lines that are under question.



-- 
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