mrhhsg commented on code in PR #67343:
URL: https://github.com/apache/doris/pull/67343#discussion_r3900843564
##########
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 c0c06d6: statements that bind `dict_get` / `dict_get_many` are now
marked `cannotProcessExpression` in `ExpressionAnalyzer` (next to the UDF
exclusion), so they never enter the FE or BE sql cache — `supportSqlCache()`
gates both `tryAddFeSqlCache` and `tryAddBeCache`. That covers the BE-folded
literal-only case as well as the default-settings case with a table scan, and
also removes the pre-existing staleness after `REFRESH DICTIONARY` (a refresh
changes no table version, so a cached result was never invalidated). Regression
`test_dictionary_read_auth_cache` primes the sql
(`cache_last_version_interval_second=0`, `enable_sql_cache=true`), checks
`explain physical plan` shows no `PhysicalSqlCache`, revokes `SELECT` on the
dictionary and expects `SELECT command denied` on the identical statement.
##########
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:
Fixed in c0c06d6: the preflight now derives the column set from
`DictionaryColumnDefinition.getOriginColumn().getName()` — the source column's
own spelling that `BindSink` / `CheckPrivileges` compare — instead of the DDL
spelling. `originColumn` has been persisted since the feature's first commit,
so no migration concern. Regression adds a dictionary declared as `(ID KEY,
USERNAME VALUE)` over a source table with `id` / `username`, grants
`SELECT_PRIV(id)` / `SELECT_PRIV(username)` plus `LOAD`, and refreshes
successfully.
##########
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:
Fixed in c0c06d6: `LogicalResultSinkToShortCircuitPointQuery` no longer sets
the short-circuit flag when the projection contains `dict_get` /
`dict_get_many`, so such point queries take the normal path and every EXECUTE
of a server prepared statement is analyzed — and authorized — again; nothing
about the dictionary ID/version is retained in a `ShortCircuitQueryContext`.
Regression `test_dictionary_read_auth_cache` prepares `SELECT dict_get(...)
FROM <MoW row-store point table> WHERE id = ?` over `useServerPrepStmts=true`,
executes once, revokes `SELECT` on the dictionary from an admin connection and
expects the second EXECUTE of the same statement to be denied.
--
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]
