github-actions[bot] commented on code in PR #67343:
URL: https://github.com/apache/doris/pull/67343#discussion_r3896156250
##########
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:
[P1] Revalidate dictionary access on SQL-cache hits
With `enable_fold_constant_by_be=true`, a literal-only `dict_get` is
authorized once, folded to a `Literal`, and stored by the FE SQL cache. This
relation-free expression records no table/view privilege and no dictionary
ID/version dependency, so the same user can repeat the identical SQL after
SELECT is revoked and `parseFromSqlCache` returns the cached value before this
check runs; refreshes can likewise leave a stale value reusable.
`dict_get_many` has the same path. Please record and revalidate the dictionary
privilege/version dependency, or exclude both functions from SQL-cache
insertion/reuse, and cover revoke-after-prime and refresh-after-prime cases.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/refresh/RefreshDictionaryCommand.java:
##########
@@ -45,7 +55,26 @@ 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 here so column-level grants keep working, and reject a
missing privilege before
+ // dataLoad() publishes the LOADING status and blocks concurrent
refreshes.
+ Set<String> sourceColumns = dictionary.getDicColumns().stream()
+ .map(DictionaryColumnDefinition::getName)
Review Comment:
[P2] Check the canonical source-column names
Dictionary definitions are matched to source columns case-insensitively, but
`DictionaryColumnDefinition.getName()` preserves the DDL spelling while
`originColumn`/`fullSchema` and the eventual scan use the source column's
canonical spelling. Native column grants are compared by exact string, so a
source column `ID` declared as dictionary column `id` can make this preflight
reject a valid canonical grant; a lower-case grant can instead pass here and
fail only after `dataLoad()` publishes `LOADING`. Please derive this set from
the canonical origin/full-schema columns shared with `BindSink`, and add a
mixed-case column-grant regression that preserves the pre-`LOADING` denial
invariant.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/DictGetMany.java:
##########
@@ -99,6 +102,16 @@ public Pair<FunctionSignature, Dictionary>
customSignatureDict() {
String dictName = firstNames[1];
List<Literal> colNames = ((ArrayLiteral) getArgument(1)).getValue();
+ // 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:
[P1] Revalidate dictionary access for prepared point-query reuse
A server-prepared point query can project `dict_get_many` (or `dict_get`)
from a qualifying point scan. Its first execution serializes the dictionary
ID/version into `ShortCircuitQueryContext`; later EXECUTEs take
`directExecuteShortCircuitQuery` without analysis whenever the point table's
name and base schema are unchanged. `isReusable` tracks neither dictionary
SELECT nor dictionary ID/version, and these functions are considered
deterministic, so revoking only the dictionary grant while retaining SELECT on
the point table still reuses the authorized output expression without reaching
this check. A refresh can likewise reuse the old version. Please revalidate
those dictionary dependencies before direct reuse, or make point-query plans
containing either function non-reusable, with revoke-after-prime and
refresh-after-prime prepared-statement regressions.
--
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]
