github-actions[bot] commented on code in PR #67343:
URL: https://github.com/apache/doris/pull/67343#discussion_r3894193276
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/refresh/RefreshDictionaryCommand.java:
##########
@@ -45,7 +50,24 @@ 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.
dataLoad() publishes the
+ // LOADING status before that INSERT is analyzed, so reject a missing
SELECT here instead of
+ // letting an unauthorized request block concurrent refreshes until
planning fails.
+ if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx,
dictionary.getSourceCtlName(),
Review Comment:
[P2] Use the column-aware source authorization here. The generated
dictionary load does not require table-wide SELECT:
`BindSink.bindDictionarySink` projects the source to
`dictionary.getFullSchema()`, then `CheckPrivileges` authorizes the actually
required slots through `checkColumnsPriv`. A caller with dictionary LOAD plus
`SELECT_PRIV(id)` and `SELECT_PRIV(value)` for a dictionary containing those
source columns is authorized by that load plan (and equivalent Ranger column
policies work too), but `checkTblPriv(..., SELECT)` rejects it before planning.
Please preflight the actual dictionary source-column set through the same
column-aware contract, or centralize planning/authorization before publishing
`LOADING`, and add a column-only regression case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExplainDictionaryCommand.java:
##########
@@ -62,11 +68,17 @@ public ShowResultSetMetaData getMetaData() {
}
@Override
- public ShowResultSet doRun(ConnectContext ctx, StmtExecutor executor)
throws DdlException {
+ public ShowResultSet doRun(ConnectContext ctx, StmtExecutor executor)
throws DdlException, AnalysisException {
List<List<String>> rows = Lists.newArrayList();
DictionaryManager dictionaryManager =
ctx.getEnv().getDictionaryManager();
String db = dbName == null ? ctx.getDatabase() : dbName;
+ // Describing a dictionary exposes its schema, so require SHOW on it
like DESCRIBE on a table.
+ if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx,
InternalCatalog.INTERNAL_CATALOG_NAME,
Review Comment:
[P1] This authorization is bypassed by the actual dictionary read path.
`DictGet.customSignatureDict` and `DictGetMany.customSignatureDict` call
`DictionaryManager.getDictionary` directly, inspect its schema, and let
`ExpressionTranslator` send its ID/version to the BE without any access-manager
check; because `SELECT dict_get(...)` needs no relation, `CheckPrivileges`
cannot catch it. A user for whom this command returns `denied` and `SHOW
DICTIONARIES` hides the row can still guess `db.dict` and read its values (or
distinguish dictionary/column errors). Apply the same dictionary read
authorization to both scalar functions before lookup/translation, return the
same denial for missing and unauthorized names, and add hidden-user read tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowDictionariesCommand.java:
##########
@@ -73,14 +76,23 @@ public ShowResultSet doRun(ConnectContext ctx, StmtExecutor
executor) throws Ana
DictionaryManager dictionaryManager =
ctx.getEnv().getDictionaryManager();
List<Dictionary> queryDicts = Lists.newArrayList();
+ String dbName = ctx.getDatabase();
// getDictionaries() already have read lock
- Map<String, Dictionary> dbDictionaries =
dictionaryManager.getDictionaries(ctx.getDatabase());
+ Map<String, Dictionary> dbDictionaries =
dictionaryManager.getDictionaries(dbName);
for (Map.Entry<String, Dictionary> entry : dbDictionaries.entrySet()) {
String dictionaryName = entry.getKey();
// Apply wild condition filtering if wild pattern is provided
- if (wild == null || matcher.match(dictionaryName)) {
- queryDicts.add(entry.getValue());
+ if (wild != null && !matcher.match(dictionaryName)) {
+ continue;
}
+ // Dictionaries are authorized like tables of the internal
catalog. Hide the ones the user
+ // may not show, the same way SHOW TABLES hides tables, so the
source table name, status
+ // and data distribution are not exposed to users without
privileges on the dictionary.
+ if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx,
InternalCatalog.INTERNAL_CATALOG_NAME,
Review Comment:
[P2] Short-circuit when this filter leaves `queryDicts` empty.
`collectDictionaryStatus(emptyList())` does not mean ‘collect none’: the
Thrift/BE contract treats an empty ID list as ALL, so a low-privilege user
whose correct result is empty still fans status RPCs to every alive BE, logs
every returned dictionary as missing from the requested set, and can have `SHOW
DICTIONARIES` fail because of a failed/null response from a contacted alive BE.
Return the empty result before status collection (or preserve an explicit
none-vs-all distinction).
--
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]
