Copilot commented on code in PR #17295:
URL: https://github.com/apache/pinot/pull/17295#discussion_r2613239574
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java:
##########
@@ -122,10 +122,14 @@ public static List<ZkBasicAuthPrincipal>
extractBasicAuthPrincipals(List<UserCon
.orElseGet(() -> Collections.emptyList())
.stream().map(x -> x.toString())
.collect(Collectors.toSet());
- //todo: Handle RLS filters properly
+
+ // Extract RLS filters from UserConfig
Review Comment:
Consider adding a brief inline comment explaining that RLS filters are now
being extracted from UserConfig, since this is a key change from the previous
implementation where an empty map was passed. This would help future readers
understand the significance of this change.
```suggestion
// Extract RLS filters from UserConfig.
// Note: This is a key change from the previous implementation
where an empty map was always passed.
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -120,6 +124,26 @@ public TableAuthorizationResult
authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAccessResult getRowColFilters(RequesterIdentity
requesterIdentity, String table) {
+ Optional<ZkBasicAuthPrincipal> principalOpt =
getPrincipalAuth(requesterIdentity);
+
+ Preconditions.checkState(principalOpt.isPresent(), "Principal is not
authorized");
+ Preconditions.checkState(table != null, "Table cannot be null");
+
+ TableRowColAccessResult tableRowColAccessResult = new
TableRowColAccessResultImpl();
+ ZkBasicAuthPrincipal principal = principalOpt.get();
+
+ //precondition: The principal should have the table.
Review Comment:
The comment has a typo: 'precondition' should be capitalized and formatted
consistently with the Preconditions.checkArgument below it. Consider changing
this to a standard Javadoc comment or removing it since the
Preconditions.checkArgument already expresses this requirement clearly.
```suggestion
```
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/AccessControlUserConfigUtils.java:
##########
@@ -48,13 +56,32 @@ public static UserConfig fromZNRecord(ZNRecord znRecord) {
List<String> tableList = znRecord.getListField(UserConfig.TABLES_KEY);
List<String> excludeTableList =
znRecord.getListField(UserConfig.EXCLUDE_TABLES_KEY);
- List<String> permissionListFromZNRecord =
znRecord.getListField(UserConfig.PERMISSIONS_KEY);
+ List<String> permissionListFromZNRecord = znRecord.getListField(
+ UserConfig.PERMISSIONS_KEY);
List<AccessType> permissionList = null;
if (permissionListFromZNRecord != null) {
permissionList = permissionListFromZNRecord.stream()
- .map(x -> AccessType.valueOf(x)).collect(Collectors.toList());
+ .map(x -> AccessType.valueOf(x))
+ .collect(Collectors.toList());
+ }
+
+ // Extract RLS filters from simple fields (stored as JSON)
+ Map<String, List<String>> rlsFilters = null;
+ String rlsFiltersJson = simpleFields.get(UserConfig.RLS_FILTERS_KEY);
+ if (rlsFiltersJson != null && !rlsFiltersJson.isEmpty()) {
+ try {
+ rlsFilters = JsonUtils.stringToObject(
+ rlsFiltersJson, new TypeReference<Map<String,
List<String>>>() {
+ }
+ );
+ } catch (IOException e) {
+ // Log error but continue - RLS filters are optional
+ LOGGER.error("Failed to deserialize RLS filters for user: {}",
username, e);
+ }
}
Review Comment:
Consider adding a comment explaining why RLS filters are stored as JSON in
simple fields rather than using list fields like other list-based data. This
design decision would be helpful for future maintainers to understand the
serialization approach.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RowLevelSecurityZkAuthIntegrationTest.java:
##########
@@ -0,0 +1,209 @@
+/**
+ * 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.integration.tests;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Row Level Security integration test using ZkAuth (ZooKeeper-based
authentication).
+ * Users and permissions are created dynamically via REST API calls to the
controller.
+ * This approach allows for dynamic user management without requiring cluster
restarts.
+ */
+public class RowLevelSecurityZkAuthIntegrationTest extends
RowLevelSecurityIntegrationTestBase {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(RowLevelSecurityZkAuthIntegrationTest.class);
+
+ @Override
+ protected void overrideControllerConf(Map<String, Object> properties) {
+ properties.put("controller.segment.fetcher.auth.token", AUTH_TOKEN);
+ properties.put("controller.admin.access.control.factory.class",
+
"org.apache.pinot.controller.api.access.ZkBasicAuthAccessControlFactory");
+ properties.put("access.control.init.username", ADMIN_USER);
+ properties.put("access.control.init.password", ADMIN_PASSWORD);
+ }
+
+ @Override
+ protected void overrideBrokerConf(PinotConfiguration brokerConf) {
+ brokerConf.setProperty("pinot.broker.enable.row.column.level.auth",
"true");
+ brokerConf.setProperty("pinot.broker.access.control.class",
+ "org.apache.pinot.broker.broker.ZkBasicAuthAccessControlFactory");
+ }
+
+ @Override
+ protected void overrideServerConf(PinotConfiguration serverConf) {
+ serverConf.setProperty("pinot.server.segment.fetcher.auth.token",
AUTH_TOKEN);
+ serverConf.setProperty("pinot.server.segment.uploader.auth.token",
AUTH_TOKEN);
+ serverConf.setProperty("pinot.server.instance.auth.token", AUTH_TOKEN);
+ }
+
+ /**
+ * Initialize users via REST API after the cluster has started.
+ * This creates the same users with the same permissions and RLS filters as
BasicAuth test.
+ */
+ @Override
+ protected void initializeUsers()
+ throws Exception {
+ LOGGER.info("Initializing users via REST API for ZkAuth test");
+
+ // Wait for controller to be ready
+ waitForControllerReady();
+
+ // Create users for BROKER component with RLS filters
+ createBrokerUsers();
+
+ // Create users for CONTROLLER component
+ createControllerUsers();
+
+ // Create users for SERVER component
+ createServerUsers();
+
+ LOGGER.info("Successfully initialized all users for ZkAuth test");
+ }
+
+ private void waitForControllerReady()
+ throws Exception {
+ int maxRetries = 30;
+ int retryCount = 0;
+ String healthUrl = "http://localhost:"; + getControllerPort() + "/health";
+ while (retryCount < maxRetries) {
+ try {
+ String response = sendGetRequest(healthUrl);
+ if (response != null && !response.isEmpty()) {
+ LOGGER.info("Controller is ready");
+ return;
+ }
+ } catch (Exception e) {
+ // Controller not ready yet
Review Comment:
Consider logging the exception message to help with debugging when the
controller fails to become ready. This would provide more context about why the
health check failed.
```suggestion
// Controller not ready yet
LOGGER.debug("Controller health check failed on attempt {}: {}",
retryCount + 1, e.getMessage(), e);
```
--
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]
