vrajat commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2141774043
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -919,6 +922,20 @@ private CompileResult compileRequest(long requestId,
String query, SqlNodeAndOpt
throwAccessDeniedError(requestId, query, requestContext, tableName,
authorizationResult);
}
+ //get RLS/CLS filters now
Review Comment:
This is in the wrong place. This code block should be moved to
`doHandleRequest`
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -135,6 +138,32 @@ public TableAuthorizationResult
authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity
requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt =
getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
+ throw new NotAuthorizedException("Basic");
+ }
+
+ if (table == null) {
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ TableRowColAuthResult tableRowColAuthResult = new
TableRowColAuthResultImpl();
+
+ BasicAuthPrincipal principal = principalOpt.get();
Review Comment:
This should be moved earlier in the function.?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -304,6 +304,17 @@ private void checkAuthorization(RequesterIdentity
requesterIdentity, RequestCont
if (!tableAuthorizationResult.hasAccess()) {
throwTableAccessError(tableAuthorizationResult);
}
+ AccessControl accessControl = _accessControlFactory.create();
+ for (String tableName : tables) {
+ accessControl.getRowColFilters(requesterIdentity,
tableName).getRLSFilters()
+ .ifPresent(rowFilters -> {
+ String combinedFilters = String.join(" AND ", rowFilters);
+ String key = String.format("%s-%s", CommonConstants.RLS_FILTERS,
tableName);
Review Comment:
Is there a better way to store RLS filters for every table instead of
mangling string keys ? @yashmayya do you have a better suggestion ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -129,12 +132,43 @@ public TableAuthorizationResult
authorize(RequesterIdentity requesterIdentity, S
failedTables.add(table);
}
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
+// return TableAuthorizationResult.success();
+// }
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity
requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt =
getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
Review Comment:
Another option is to check using `Preconditions`.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -31,14 +34,17 @@ public class BasicAuthPrincipal {
private final Set<String> _tables;
private final Set<String> _excludeTables;
private final Set<String> _permissions;
+ //key: table name, val: list of RLS filters applicable for that table.
+ private final Map<String, List<String>> _rlsFilters;
public BasicAuthPrincipal(String name, String token, Set<String> tables,
Set<String> excludeTables,
- Set<String> permissions) {
+ Set<String> permissions, Map<String, List<String>> rlsFilters) {
Review Comment:
Can you add a constructor that does not have `rlsFilters` as a param ?
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -65,13 +71,27 @@ public boolean hasPermission(String permission) {
return _permissions.isEmpty() ||
_permissions.contains(permission.toLowerCase());
}
+ /**
+ * Gets the Row-Level Security (RLS) filter configured for the given table.
+ * The RLS filter is applied only if the user has access to the table
+ * (as determined by {@link #hasTable(String)}).
+ *
+ * @param tableName The name of the table.
+ * @return An {@link java.util.Optional} containing the RLS filter string if
configured for this principal and table,
+ * otherwise {@link java.util.Optional#empty()}.
+ */
+ public Optional<List<String>> getRLSFilters(String tableName) {
Review Comment:
Nit: Return null instead of `Optional` ?
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java:
##########
@@ -76,8 +79,26 @@ public static List<BasicAuthPrincipal>
extractBasicAuthPrincipals(PinotConfigura
Set<String> excludeTables = extractSet(configuration, prefix + "." +
name + "." + EXCLUDE_TABLES);
Set<String> permissions = extractSet(configuration, prefix + "." + name
+ "." + PERMISSIONS);
+ // Extract RLS filters
Review Comment:
Can you add a unit test for this block ?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -76,19 +78,26 @@ private ServerPlanRequestUtils() {
private static final int DEFAULT_LEAF_NODE_LIMIT = Integer.MAX_VALUE;
private static final List<String> QUERY_REWRITERS_CLASS_NAMES =
ImmutableList.of(PredicateComparisonRewriter.class.getName(),
- NonAggregationGroupByToDistinctQueryRewriter.class.getName());
+ NonAggregationGroupByToDistinctQueryRewriter.class.getName(),
RlsFiltersRewriter.class.getName());
private static final List<QueryRewriter> QUERY_REWRITERS =
new
ArrayList<>(QueryRewriterFactory.getQueryRewriters(QUERY_REWRITERS_CLASS_NAMES));
private static final QueryOptimizer QUERY_OPTIMIZER = new QueryOptimizer();
+ public static OpChain compileLeafStage(OpChainExecutionContext
executionContext, StagePlan stagePlan,
+ QueryExecutor leafQueryExecutor, ExecutorService executorService,
Map<String, String> rowFilters) {
+ return compileLeafStage(executionContext, stagePlan, leafQueryExecutor,
executorService,
+ (planNode, multiStageOperator) -> {
+ }, false, rowFilters);
+ }
+
public static OpChain compileLeafStage(
OpChainExecutionContext executionContext,
StagePlan stagePlan,
QueryExecutor leafQueryExecutor,
ExecutorService executorService) {
return compileLeafStage(executionContext, stagePlan, leafQueryExecutor,
executorService,
(planNode, multiStageOperator) -> {
- }, false);
+ }, false, Map.of());
Review Comment:
nit: send null instead of Map.of
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
