Jackie-Jiang commented on code in PR #19664:
URL: https://github.com/apache/pinot/pull/19664#discussion_r4253443736
##########
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:
This overload is a cluster-wide authorization check, not an
authentication-only check. Both controller BasicAuth implementations inherit
BaseBasicAuthAccessControl.hasAccess(AccessType, ...), which rejects any
principal with a table allow-list or exclude-list. A principal with tables=t
and permissions=read,delete therefore gets ACCESS_DENIED for DELETE FROM t
WHERE id = 1 before the resolved-table checks or executor run, although it can
execute the same DELETE through the broker. Please use an authentication-only
precheck (while preserving authentication before the cache lookup), followed by
the existing resolved-table authorization. A controller test using a real
table-scoped BasicAuth principal would cover this; the unrestricted
integration-test administrator and mocked access controls miss it.
##########
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:
Non-blocking performance suggestion for existing queries: return immediately
when _excludeTables is empty, and only perform the second contains check when
the extracted raw name differs from tableName. As written, every successful
exact-name exclusion check proceeds to suffix extraction and another lookup,
even with no exclusions configured; typed names also allocate a substring.
Callers that already pass raw names repeat the same lookup. Keep the raw-name
exclusion enforcement itself, since removing it would reopen the type-suffix
bypass.
##########
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:
Non-blocking performance suggestion: preserve the single regex pass for
legacy IGNORE mode. Previously that mode only called removeOptionsFromSql; it
now allocates the extraction list on every parse and, when an OPTION suffix
exists, scans the SQL again to remove it. One matcher can detect and strip the
end-anchored suffix, retaining its presence so DELETE options are still
rejected under IGNORE. Avoid materializing an options list when no suffix
exists. The default ALLOW path already did both scans before this PR, so this
is specifically added work for existing IGNORE-mode traffic, not a demonstrated
regression for all queries.
--
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]
