xiangfu0 commented on code in PR #19234: URL: https://github.com/apache/pinot/pull/19234#discussion_r3798259647
########## pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BaseBasicAuthAccessControl.java: ########## @@ -0,0 +1,73 @@ +/** + * 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.pinot.controller.api.access; + +import java.util.Objects; +import java.util.Optional; +import javax.ws.rs.NotAuthorizedException; +import javax.ws.rs.core.HttpHeaders; +import org.apache.pinot.core.auth.BasicAuthPrincipal; +import org.apache.pinot.core.auth.TargetType; +import org.apache.pinot.spi.utils.builder.TableNameBuilder; + + +/// Shared controller BasicAuth policy, independent of how principals are loaded and credentials are verified. +/// This class is stateless and thread-safe; subclass principal resolution must also be thread-safe. +abstract class BaseBasicAuthAccessControl<P extends BasicAuthPrincipal> implements AccessControl { + @Override + public final boolean protectAnnotatedOnly() { + return false; + } + + @Override + public final boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders, + String endpointUrl) { + String rawTableName = TableNameBuilder.extractRawTableName(tableName); + return getPrincipal(httpHeaders) + .filter(p -> p.hasTable(rawTableName) && p.hasPermission(Objects.toString(accessType))).isPresent(); Review Comment: Fixed in d7cd1fd0cc. Table-scoped BasicAuth now authenticates first and returns 401 for missing, malformed, or incorrect credentials; authenticated table or CRUD denials remain 403. The shared unit matrix covers both implementations, and the controller HTTP test now verifies table-endpoint 401 behavior for static and ZooKeeper-backed BasicAuth. The deprecated auth probe retains its boolean false contract. ########## pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/LLCSegmentCompletionHandlers.java: ########## @@ -83,7 +83,8 @@ public static String getScheme() { // We don't want to document these in swagger since they are internal APIs @GET @Path(SegmentCompletionProtocol.MSG_TYPE_EXTEND_BUILD_TIME) - @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.GET_ADMIN_INFO) + @Authenticate(AccessType.CREATE) + @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.COMMIT_SEGMENT) Review Comment: Agreed. I added a Rolling upgrade requirement to the PR description: grant the server-to-controller segment uploader identity cluster CREATE and, for action-aware policies, COMMIT_SEGMENT before Controllers are upgraded. ########## pinot-common/src/main/java/org/apache/pinot/common/config/provider/AccessControlUserCache.java: ########## @@ -87,6 +87,13 @@ public List<UserConfig> getAllControllerUserConfig() { return _userControllerConfigMap.values().stream().collect(Collectors.toList()); } + /// Returns the cache-owned controller user config for the given raw username, or `null` when no such user exists. + /// Callers must treat the returned config as read-only. + @Nullable + public UserConfig getControllerUserConfigForUsername(String rawUsername) { + return _userControllerConfigMap.get(rawUsername + "_" + ComponentType.CONTROLLER); Review Comment: Fixed in d7cd1fd0cc. Lookup keeps the O(1) exact path, falls back to the trimmed stored username for legacy records, rejects presented whitespace aliases as before, and fails closed when multiple stored names normalize to the same identity. Tests cover stored whitespace, canonical precedence, presented-whitespace rejection, and ambiguous identities. ########## pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BaseBasicAuthAccessControl.java: ########## @@ -0,0 +1,73 @@ +/** + * 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.pinot.controller.api.access; + +import java.util.Objects; +import java.util.Optional; +import javax.ws.rs.NotAuthorizedException; +import javax.ws.rs.core.HttpHeaders; +import org.apache.pinot.core.auth.BasicAuthPrincipal; +import org.apache.pinot.core.auth.TargetType; +import org.apache.pinot.spi.utils.builder.TableNameBuilder; + + +/// Shared controller BasicAuth policy, independent of how principals are loaded and credentials are verified. +/// This class is stateless and thread-safe; subclass principal resolution must also be thread-safe. +abstract class BaseBasicAuthAccessControl<P extends BasicAuthPrincipal> implements AccessControl { + @Override + public final boolean protectAnnotatedOnly() { + return false; + } + + @Override + public final boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders, + String endpointUrl) { + String rawTableName = TableNameBuilder.extractRawTableName(tableName); + return getPrincipal(httpHeaders) + .filter(p -> p.hasTable(rawTableName) && p.hasPermission(Objects.toString(accessType))).isPresent(); Review Comment: Pinned in d7cd1fd0cc with an _OFFLINE assertion shared by the static and ZooKeeper-backed implementations. I also updated the PR description to disclose the static typed-name normalization alignment instead of claiming that all table behavior is unchanged. ########## pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerLogger.java: ########## @@ -141,7 +141,7 @@ public Set<String> getLocalLogFiles() { @Path("/loggers/download") @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.GET_LOG_FILE) @Produces(MediaType.APPLICATION_OCTET_STREAM) - @Authenticate(AccessType.DELETE) + @Authenticate(AccessType.READ) Review Comment: READ is intentional: these endpoints return data and do not mutate state, while UPDATE would route a read operation through a mutation bucket. Action-aware implementations still enforce GET_LOG_FILE for the operation. I added that distinction to the compatibility notes. -- 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]
