[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16201: [fix][broker] Fix passing incorrect authentication data

[GitBox <gi...@apache.org>]

BewareMyPower commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r908019720


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -185,7 +185,7 @@ public class ServerCnx extends PulsarHandler implements TransportCnx {
     private String clientVersion = null;
     private int nonPersistentPendingMessages = 0;
     private final int maxNonPersistentPendingMessages;
-    private String originalPrincipal = null;
+    String originalPrincipal = null;

Review Comment:
   It's better to add a @VisibleForTesting annotation here. And we should only expose the getter, not the setter.
   
   ```java
   @VisibleForTesting
   String getOriginalPrincipal() {
       return originalPrincipal;
   }
   ```



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -411,9 +412,13 @@ private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName,
     private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, String subscriptionName,
                                                                TopicOperation operation) {
         if (service.isAuthorizationEnabled()) {
-            AuthenticationDataSource authData =
-                    new AuthenticationDataSubscription(getAuthenticationData(), subscriptionName);
-            return isTopicOperationAllowed(topicName, operation, authData);
+            AuthenticationDataSource authDataSource =
+                    new AuthenticationDataSubscription(authenticationData, subscriptionName);
+            AuthenticationDataSource originalAuthDataSource = null;
+            if (originalAuthData != null) {
+                originalAuthDataSource = new AuthenticationDataSubscription(originalAuthData, subscriptionName);
+            }
+            return isTopicOperationAllowed(topicName, operation, authDataSource, originalAuthDataSource);

Review Comment:
   I don't get why should it be so complicated? For the modified code,
   - if `originalAuthData` is not `null`, `originalAuthData` will be used to construct the `AuthenticationDataSubscription` object as the 4th argument that will be used in `isTopicOperationAllowed`.
   - Otherwise, the `authenticationData` will be used to construct the `AuthenticationDataSubscription` object (`authDataSource`) as the 3rd argument that will be used in `isTopicOperationAllowed`.
   
   I think it's equivalent to
   
   ```java
               return isTopicOperationAllowed(topicName, operation,
                       new AuthenticationDataSubscription(getAuthenticationData(), subscriptionName), null);
   ```
   
   After applying my change, `testVerifyOriginalPrincipalWithAuthDataForwardedFromProxy` will fail, but that's because you assume `allowTopicOperationAsync` will accept a non-null 4th argument.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -167,7 +167,7 @@ public class ServerCnx extends PulsarHandler implements TransportCnx {
     private State state;
     private volatile boolean isActive = true;
     String authRole = null;
-    private volatile AuthenticationDataSource authenticationData;
+    volatile AuthenticationDataSource authenticationData;

Review Comment:
   It's better to add a `@VisibleForTesting` annotation here.



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