xiangfu0 commented on code in PR #19664:
URL: https://github.com/apache/pinot/pull/19664#discussion_r4201467195
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -133,6 +144,27 @@ public TableAuthorizationResult
authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public AuthorizationResult authorizeDeleteRows(RequesterIdentity
requesterIdentity,
+ @Nullable HttpHeaders httpHeaders, String tableName) {
+ Optional<BasicAuthPrincipal> principalOpt =
getPrincipalOpt(requesterIdentity);
+ if (principalOpt.isEmpty()) {
+ return new BasicAuthorizationResultImpl(false, "Missing or invalid
credentials");
+ }
+ BasicAuthPrincipal principal = principalOpt.get();
+ // The table checks match the name as given, while `excludeTables` lists
raw names: check both, so that a name
+ // with a type suffix cannot bypass an excluded table
+ if (!principal.hasTable(tableName) ||
!principal.hasTable(TableNameBuilder.extractRawTableName(tableName))) {
Review Comment:
Split it in d5065b2950: `BasicAuthPrincipal` gets `isTableExcluded`, and the
raw-name check here only applies the exclusion list. A `typedDeleter` principal
with `tables=lessImportantStuff_OFFLINE, permissions=read,delete` is now
allowed on the typed name and denied on the raw name and the other type. That
relaxation exposed a second problem on the same path: such a principal then
reached the row-level-security lookup, which consulted the raw name first and
tripped `getRowColFilters`'s `hasTable` precondition, turning the DELETE into a
500. `hasRowFilters` now looks up the typed name first and only consults the
raw name when the caller is authorized for it, and a `PinotClientRequestTest`
case runs the real `BasicAuthAccessControlFactory` with that principal, with
RLS enabled, asserting the DELETE executes, and that a filter keyed on the
typed name still refuses it.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -669,7 +669,7 @@ private void setUpPinotController() {
new SegmentCompletionManager(_helixParticipantManager,
_pinotLLCRealtimeSegmentManager, _controllerMetrics,
_leadControllerManager, _config.getSegmentCommitTimeoutSeconds(),
segmentCompletionConfig);
- _sqlQueryExecutor = new SqlQueryExecutor(_config.generateVipUrl());
+ _sqlQueryExecutor = createSqlQueryExecutor();
Review Comment:
Moved the call down to just before the binder registration in d5065b2950,
after `setupControllerPeriodicTasks()` so `_taskManager` and
`_connectionManager` exist, and the hook's Javadoc now lists what is
initialized at that point and points at the broker-side hook.
--
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]
