mrhhsg commented on code in PR #67343:
URL: https://github.com/apache/doris/pull/67343#discussion_r3902022520
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/LogicalResultSinkToShortCircuitPointQuery.java:
##########
@@ -98,8 +109,10 @@ public List<Rule> buildRules() {
.when(this::scanMatchShortCircuitCondition)
).when(this::filterMatchShortCircuitCondition)))
.thenApply(ctx -> {
+ if (projectReadsDictionary(ctx.root.child())) {
Review Comment:
Fixed in e99a7ec: the reuse decision no longer inspects the plan.
`ExpressionAnalyzer` sets `StatementContext.hasDictionaryRead` when it binds
`dict_get` / `dict_get_many` (bind time precedes every rewrite, including
`FoldConstantRuleOnBE`), and
`LogicalResultSinkToShortCircuitPointQuery.shortCircuit` returns without
setting the short-circuit flag whenever that mark is present. The same
statement-level mark approach also covers alias function bodies (found in my
own pre-push review: `AliasUdfBuilder` analyzes the body without a
`CascadesContext`, so the analyzer now falls back to the current statement's
`ConnectContext`). Regression: prepared `SELECT dict_get('db.d','username',1)
FROM <point table> WHERE id = ?` with `enable_fold_constant_by_be=true`,
executed once, `SELECT` on the dictionary revoked from an admin connection,
second EXECUTE denied.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/refresh/RefreshDictionaryCommand.java:
##########
@@ -45,7 +54,28 @@ public RefreshDictionaryCommand(String dbName, String
dicName) {
public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
DictionaryManager dictionaryManager =
ctx.getEnv().getDictionaryManager();
String db = dbName == null ? ctx.getDatabase() : dbName;
+ // The reload is an INSERT INTO the dictionary executed as the current
user, which requires
+ // LOAD on the dictionary (and SELECT on the source table). Check LOAD
up front so a user
+ // without it can neither probe whether the dictionary exists nor flip
it to LOADING.
+ if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx,
InternalCatalog.INTERNAL_CATALOG_NAME,
+ db, dictionaryName, PrivPredicate.LOAD)) {
+
ErrorReport.reportAnalysisException(ErrorCode.ERR_TABLEACCESS_DENIED_ERROR,
"LOAD",
+ ctx.getQualifiedUser(), ctx.getRemoteIP(), db + ": " +
dictionaryName);
+ }
Dictionary dictionary = dictionaryManager.getDictionary(db,
dictionaryName);
+ // The reload also reads the source table as the current user, and its
INSERT authorizes
+ // exactly the dictionary's source columns through the column-aware
contract (BindSink
+ // projects the source to the dictionary schema before CheckPrivileges
runs). Use the same
+ // contract and the same column spelling here: the dictionary
definition may spell a column
+ // differently from the source table, but column grants are compared
against the source
+ // column's own name. Reject a missing privilege before dataLoad()
publishes the LOADING
+ // status and blocks concurrent refreshes.
+ Set<String> sourceColumns = dictionary.getDicColumns().stream()
+ .map(definition -> definition.getOriginColumn().getName())
+ .collect(Collectors.toCollection(LinkedHashSet::new));
+ Env.getCurrentEnv().getAccessManager().checkColumnsPriv(ctx,
dictionary.getSourceCtlName(),
+ dictionary.getSourceDbName(), dictionary.getSourceTableName(),
sourceColumns,
Review Comment:
Fixed in e99a7ec: the preflight resolves the source through
`RelationUtil.getTable(dictionary.getSourceQualifiedName(), ...)` (as
`Dictionary.hasNewerSourceVersion` does) and authorizes with the resolved
catalog name, `DatabaseIf.getFullName()` and `TableIf.getName()` — the
identifiers the load scan reports to `CheckPrivileges` — together with the
source columns' own names. A dropped/renamed source now fails there, before
`LOADING` is published. `lower_case_table_names` is a bootstrap-time FE config
the regression environment cannot switch, so this one is covered by the
resolved-identity code path rather than a dedicated case; the mixed-case
*column* case from the previous round remains in
`test_dictionary_read_auth_cache`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/DictGet.java:
##########
@@ -93,6 +96,16 @@ public Pair<FunctionSignature, Dictionary>
customSignatureDict() {
String dictName = firstNames[1];
String colName = ((Literal) getArgument(1)).getStringValue();
+ // Reading dictionary values must be authorized like reading its
source data. Check before
+ // the lookup so a caller without the privilege cannot even probe
whether the dictionary
+ // exists. ConnectContext may be absent on internal paths, which carry
no user to check.
+ ConnectContext connectContext = ConnectContext.get();
+ if (connectContext != null &&
!Env.getCurrentEnv().getAccessManager().checkTblPriv(connectContext,
Review Comment:
Fixed in e99a7ec: the check moved out of `customSignatureDict()` (which
`ExpressionTranslator` calls again after view inlining) into
`ExpressionAnalyzer.visitUnboundFunction`, through a new
`DictionaryReadFunction.checkReadPrivilege(ConnectContext)` implemented by both
functions. It runs once at bind time, right after `builder.build` and before
signature computation resolves the dictionary (so a missing and an unauthorized
name still produce the same denial), and only when
`StatementContext.isAnalyzingView()` is false —
`BindRelation.parseAndAnalyzeView` now increments/decrements that depth around
the view body's analysis (try/finally), matching the boundary
`CheckPrivileges.visitLogicalView` enforces. Direct calls, alias function
bodies and `CREATE/ALTER VIEW` bodies are still checked against the caller.
Regression: views over `dict_get` and `dict_get_many` readable by a user
holding `SELECT` on the view only, with direct calls of both functions as
negative controls; `auth_p0/t
est_select_view_auth` re-run as the existing view-boundary control.
--
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]
