[GitHub] [pulsar] codelipenghui commented on a diff in pull request #19830: [improve][broker] Authorize originalPrincipal when provided

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

codelipenghui commented on code in PR #19830:
URL: https://github.com/apache/pulsar/pull/19830#discussion_r1139492653


##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -331,7 +327,9 @@ public boolean isValidOriginalPrincipal(String authenticatedPrincipal,
             }
         } else if (StringUtils.isNotBlank(originalPrincipal)
                 && !(allowNonProxyPrincipalsToBeEqual && originalPrincipal.equals(authenticatedPrincipal))) {
-            errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role.";
+                log.warn("[{}] Non-proxy role [{}] passed originalPrincipal [{}]. This behavior will not "

Review Comment:
   I think it's fine to add a warning log here. It will help the pulsar's administrator to consider correcting the configuration. And the warning logs will also not impact the alert too much.



##########
pulsar-broker-common/src/test/java/org/apache/pulsar/broker/authorization/AuthorizationServiceTest.java:
##########
@@ -0,0 +1,135 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.authorization;
+
+import static org.testng.AssertJUnit.assertFalse;
+import static org.testng.AssertJUnit.assertTrue;
+import java.util.HashSet;
+import org.apache.pulsar.broker.PulsarServerException;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.common.naming.NamespaceName;
+import org.apache.pulsar.common.naming.TopicName;
+import org.apache.pulsar.common.policies.data.NamespaceOperation;
+import org.apache.pulsar.common.policies.data.PolicyName;
+import org.apache.pulsar.common.policies.data.PolicyOperation;
+import org.apache.pulsar.common.policies.data.TenantOperation;
+import org.apache.pulsar.common.policies.data.TopicOperation;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class AuthorizationServiceTest {
+
+    AuthorizationService authorizationService;
+
+    @BeforeClass
+    void beforeClass() throws PulsarServerException {
+        ServiceConfiguration conf = new ServiceConfiguration();
+        conf.setAuthorizationEnabled(true);
+        // Consider both of these proxy roles to make testing more comprehensive
+        HashSet<String> proxyRoles = new HashSet<>();
+        proxyRoles.add("pass.proxy");
+        proxyRoles.add("fail.proxy");
+        conf.setProxyRoles(proxyRoles);
+        conf.setAuthorizationProvider(MockAuthorizationProvider.class.getName());
+        authorizationService = new AuthorizationService(conf, null);
+    }
+
+    /**
+     * See {@link MockAuthorizationProvider} for the implementation of the mock authorization provider.
+     */
+    @DataProvider(name = "roles")
+    public Object[][] encryptionProvider() {
+        return new Object[][]{
+                // Schema: role, originalRole, whether authorization should pass
+
+                // Client conditions where original role isn't passed or is blank
+                {"pass.client", null, Boolean.TRUE},
+                {"pass.client", " ", Boolean.TRUE},
+                {"fail.client", null, Boolean.FALSE},
+                {"fail.client", " ", Boolean.FALSE},
+
+                // Proxy conditions where original role isn't passed or is blank

Review Comment:
   We only need to ensure the existing behavior will not be changed even if it's not good enough. @nodece Do you think the list of potential combinations you have provided will be broken if we don't make more changes to this PR? I mean, if yes, we should provide the context of how https://github.com/apache/pulsar/pull/19455 break the potential combinations even if we have this PR.



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