Re: [PR] [Enhance] Optimize user login [pinot]

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

Copilot commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2642366916


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java:
##########
@@ -81,7 +81,7 @@ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("
       @ApiParam(value = "API access type") @DefaultValue("READ") @QueryParam("accessType") AccessType accessType,
       @ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
     AccessControl accessControl = _accessControlFactory.create();
-    return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+    return accessControl.hasAccess(_httpHeaders, TargetType.CLUSTER);

Review Comment:
   The verify endpoint now ignores all its parameters (tableName, accessType, endpointUrl) and only checks cluster-level access. This breaks the documented API contract where callers expect table-specific and access-type-specific verification. Either update the endpoint's signature to remove unused parameters or implement the verification logic to actually use them.



##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -22,11 +22,11 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 import javax.ws.rs.NotAuthorizedException;

Review Comment:
   The removed import `java.util.Objects` appears to still be needed, as the code likely uses Objects.nonNull or similar elsewhere in this file. Verify that this import removal doesn't break compilation.



##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -122,24 +122,39 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
 
     private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity requesterIdentity) {
       Collection<String> tokens = extractAuthorizationTokens(requesterIdentity);
-      if (tokens.isEmpty()) {
+      if (tokens == null || tokens.isEmpty()) {
         return Optional.empty();
       }
 
-      _name2principal = BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
-          .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
+      Map<String, ZkBasicAuthPrincipal> name2principal =
+          BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
+              .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));

Review Comment:
   The name2principal map is reconstructed on every authentication attempt by calling getAllBrokerUserConfig() and processing it. This is inefficient for high-frequency authentication requests. Consider caching this map and only refreshing it when user configuration changes, similar to how the controller version uses the _name2principal field.



-- 
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@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org