xiangfu0 commented on code in PR #19664:
URL: https://github.com/apache/pinot/pull/19664#discussion_r4253629704
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +459,163 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, and the caller
authorized to delete rows from the resolved
+ /// table. Only then is the table checked not to be a logical table and to
exist, and every error message names the
+ /// table as the caller wrote it, so that a caller who is not authorized for
a table learns neither whether it exists
+ /// nor the case it is defined with, nor whether the name is a logical
table. The executor deletes rows from the
+ /// exact resolved table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @param request request the statement came with, whose remote address the
log line names; null when the
+ /// statement did not come with one
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#QUERY_VALIDATION] if the table is
a logical table, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders,
+ @Nullable Request request) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
Review Comment:
You're right, that overload is the cluster-wide gate and it denied every
table-scoped principal on the controller DELETE path. Fixed in 7c3949e686: the
precheck is gone, and the first check that authenticates the caller is the
table-level READ check of `authorizeDelete` on the resolved table (basic auth
throws 401 for a missing principal there). Running the table-cache lookup
before it leaks nothing, since every table-dependent outcome (logical table,
unknown table, denial) comes after authorization and names the table as the
caller wrote it; the Javadoc says so. `PinotQueryResourceTest` now runs the
real controller `BasicAuthAccessControlFactory` with a principal `tables=t,
permissions=read,delete` and asserts the DELETE on `t` reaches the executor,
the same principal is denied on another table, and a wrong password is
unauthorized.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -93,8 +94,11 @@ private boolean isTableIncluded(String tableName) {
return _tables.isEmpty() || _tables.contains(tableName);
}
+ /// The `excludeTables` list holds raw table names, so a name with a type
suffix is also checked by its raw name:
+ /// with `excludeTables=[t]`, `t_OFFLINE` is excluded as `t` is, and a type
suffix cannot bypass an excluded table.
private boolean isTableNotExcluded(String tableName) {
- return !_excludeTables.contains(tableName);
+ return !_excludeTables.contains(tableName)
+ &&
!_excludeTables.contains(TableNameBuilder.extractRawTableName(tableName));
Review Comment:
Done in 7c3949e686: `isTableNotExcluded` returns immediately when no
exclusions are configured and only looks up the raw name when it differs from
the name given, so raw-name callers and principals without exclusions keep a
single lookup and no substring is allocated; the raw-name enforcement itself is
unchanged.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -122,19 +123,14 @@ public static SqlNodeAndOptions
compileToSqlNodeAndOptions(String sql)
sql = ParserUtils.sanitizeSql(sql);
// extract and remove OPTIONS string
- List<String> options = List.of();
SqlOptionsMode legacyOptionSyntaxMode =
QueryOptionsUtils.getLegacyOptionSyntaxMode();
- if (legacyOptionSyntaxMode == SqlOptionsMode.IGNORE) {
- sql = removeOptionsFromSql(sql);
- } else {
- options = extractOptionsFromSql(sql);
- if (!options.isEmpty()) {
- if (legacyOptionSyntaxMode == SqlOptionsMode.REJECT) {
- throw new SqlCompilationException("Legacy OPTION(...) query options
are not allowed on this cluster, use "
- + "'SET <key> = <value>;' statements instead: " + options);
- }
- sql = removeOptionsFromSql(sql);
+ List<String> options = extractOptionsFromSql(sql);
+ if (!options.isEmpty()) {
+ if (legacyOptionSyntaxMode == SqlOptionsMode.REJECT) {
+ throw new SqlCompilationException("Legacy OPTION(...) query options
are not allowed on this cluster, use "
+ + "'SET <key> = <value>;' statements instead: " + options);
}
+ sql = removeOptionsFromSql(sql);
Review Comment:
Done in 7c3949e686: in legacy IGNORE mode one matcher now detects and strips
the suffix in a single pass and only records whether a suffix was present, so
no options list is materialized and no second scan runs; a DELETE carrying a
legacy suffix is still rejected in that mode. ALLOW and REJECT keep their
existing extract-and-remove path.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
