github-actions[bot] commented on code in PR #67343:
URL: https://github.com/apache/doris/pull/67343#discussion_r3893132612
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExplainDictionaryCommand.java:
##########
@@ -18,10 +18,16 @@
package org.apache.doris.nereids.trees.plans.commands;
import org.apache.doris.catalog.Column;
+import org.apache.doris.catalog.Env;
import org.apache.doris.catalog.ScalarType;
+import org.apache.doris.common.AnalysisException;
import org.apache.doris.common.DdlException;
+import org.apache.doris.common.ErrorCode;
+import org.apache.doris.common.ErrorReport;
+import org.apache.doris.datasource.InternalCatalog;
import org.apache.doris.dictionary.Dictionary;
Review Comment:
[P1] This check uses the physical table privilege namespace, but
dictionaries are stored outside Database's table map and can coexist with a
same-name table. For example, after creating `db.foo` and dictionary `db.foo`,
a user with only `SELECT` (or a column grant) on physical table `foo` passes
`PrivPredicate.SHOW` here because `Role.checkTblPriv` treats any matching
table/column privilege as SHOW. The command then returns the full dictionary
schema (and `SHOW DICTIONARIES` exposes its source/status), despite no
privilege on the dictionary. Please reject collisions or authorize dictionaries
in a distinct namespace (and cover both internal and Ranger controllers).
##########
regression-test/suites/auth_call/test_ddl_dictionary_auth.groovy:
##########
@@ -85,6 +85,56 @@ suite("test_ddl_dictionary_auth", "p0,auth_call") {
def dictRes = sql """SHOW DICTIONARIES"""
assertTrue(dictRes.size() == 1)
+ // A user with privileges on another object of the database can USE the
database, but must
+ // not learn about dictionaries it has no privilege on, the same way SHOW
TABLES hides tables.
+ String viewer = 'test_ddl_dictionary_auth_viewer'
+ try_sql("DROP USER ${viewer}")
+ sql """CREATE USER '${viewer}' IDENTIFIED BY '${pwd}'"""
+ sql """grant select_priv on regression_test to ${viewer}"""
+ sql """grant SELECT_PRIV on ${dbName}.${tableName} to ${viewer}"""
+ if (isCloudMode()) {
+ def clusters = sql " SHOW CLUSTERS; "
+ def validCluster = clusters[0][0]
+ sql """GRANT USAGE_PRIV ON CLUSTER `${validCluster}` TO ${viewer}""";
+ }
+ connect(viewer, "${pwd}", context.config.jdbcUrl) {
+ sql """use ${dbName}"""
+ def hiddenDicts = sql """SHOW DICTIONARIES"""
+ assertEquals(0, hiddenDicts.size())
+ test {
+ sql """EXPLAIN DICTIONARY ${dictName}"""
+ exception "denied"
+ }
+ test {
+ sql """REFRESH DICTIONARY ${dictName}"""
+ exception "denied"
+ }
+ }
+
+ // SHOW_VIEW makes the dictionary visible, including its source table, but
refreshing still
+ // needs LOAD on the dictionary. Dictionaries are not tables of the
database, so these
+ // privileges have to be granted on the database.
+ sql """grant SHOW_VIEW_PRIV on ${dbName}.* to ${viewer}"""
+ connect(viewer, "${pwd}", context.config.jdbcUrl) {
+ sql """use ${dbName}"""
+ def visibleDicts = sql """SHOW DICTIONARIES"""
+ assertEquals(1, visibleDicts.size())
+ assertEquals(dictName, visibleDicts[0][1])
+ assertEquals("internal.${dbName}.${tableName}".toString(),
visibleDicts[0][2])
+ def dictColumns = sql """EXPLAIN DICTIONARY ${dictName}"""
+ assertEquals(2, dictColumns.size())
+ test {
+ sql """REFRESH DICTIONARY ${dictName}"""
+ exception "denied"
+ }
+ }
+
+ sql """grant LOAD_PRIV on ${dbName}.* to ${viewer}"""
+ connect(viewer, "${pwd}", context.config.jdbcUrl) {
+ sql """use ${dbName}"""
+ sql """REFRESH DICTIONARY ${dictName}"""
+ }
Review Comment:
[P2] `CREATE DICTIONARY` queues its initial `dataLoad` asynchronously, but
this positive `REFRESH DICTIONARY` runs without waiting for the dictionary to
reach `NORMAL`. If the initial task is still `LOADING`, `dataLoad` rejects this
refresh with a status-conflict error even though the viewer now has `LOAD` and
source `SELECT`. Add `waitDictionaryReady`/`waitAllDictionariesReady` after
creation (and poll after refresh if needed) so this test deterministically
exercises authorization.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/refresh/RefreshDictionaryCommand.java:
##########
@@ -18,8 +18,13 @@
package org.apache.doris.nereids.trees.plans.commands.refresh;
import org.apache.doris.analysis.StmtType;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.common.ErrorCode;
+import org.apache.doris.common.ErrorReport;
+import org.apache.doris.datasource.InternalCatalog;
Review Comment:
[P1] `checkTblPriv(..., LOAD)` also resolves the target solely by the
physical table name. Because `DictionaryManager` allows `db.foo` (table) and
`db.foo` (dictionary) to coexist, a user granted `LOAD` on the table `foo` can
pass this check and refresh the dictionary `foo` without a dictionary grant
(provided they can SELECT the source). Please use a dictionary-specific
authorization key or disallow table/dictionary name collisions so privileges
cannot cross object types.
[P2] The preflight only checks `LOAD` on the dictionary, but the refresh's
generated INSERT also requires `SELECT` on the source.
`DictionaryManager.dataLoad()` sets the shared dictionary status to `LOADING`
before `CheckPrivileges` analyzes that source; a user with database `LOAD` but
no source `SELECT` is rejected only after planning starts, and the catch
restores the status afterward. During that window concurrent legitimate
refreshes get `cannot load now`, and repeated requests can starve refreshes on
a slow source. Validate source SELECT (or perform deterministic
authorization/planning) before publishing `LOADING`.
--
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]
