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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
