[fix](auth) Add privilege checks to SHOW/EXPLAIN/REFRESH DICTIONARY - #67343
[fix](auth) Add privilege checks to SHOW/EXPLAIN/REFRESH DICTIONARY#67343mrhhsg wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes. The new checks close the original unauthenticated SHOW/EXPLAIN/REFRESH gap, but the reviewed head still has two authorization-boundary defects and two correctness/test issues:
- P1: the checks use the physical table privilege key even though dictionaries live in DictionaryManager and can share a name with a physical table. Existing SELECT/column grants on that table can therefore expose the dictionary through SHOW/EXPLAIN, and LOAD grants can authorize REFRESH of the distinct dictionary.
- P2: REFRESH checks only dictionary LOAD before dataLoad; source SELECT is checked after the shared status is set to LOADING, so an unauthorized request can transiently block concurrent authorized refreshes.
- P2: the positive regression REFRESH runs immediately after CREATE, whose initial load is asynchronous, so the test can fail on the LOADING guard rather than exercise authorization.
Checkpoint conclusions: the intended privilege goal is only partially met; the command changes are otherwise focused and preserve existing error propagation. The refresh path has a shared-state lifecycle race, while SHOW/EXPLAIN have no new lock change beyond the pre-existing manager snapshot behavior. Internal and Ranger privilege paths should share a dictionary-specific namespace or explicitly reject table/dictionary collisions. The regression test covers the main negative/positive contract but needs readiness synchronization. No protocol, persistence, or configuration changes are involved. Tests were not run because this review bundle forbids builds and test execution. User focus requested no additional focus; the full changed-file sweep found no other distinct issues.
| import org.apache.doris.common.ErrorCode; | ||
| import org.apache.doris.common.ErrorReport; | ||
| import org.apache.doris.datasource.InternalCatalog; | ||
| import org.apache.doris.dictionary.Dictionary; |
There was a problem hiding this 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).
There was a problem hiding this comment.
Dictionaries are authorized with the table privilege key of the internal catalog by design: CREATE/DROP DICTIONARY already use checkTblPriv(internal, db, dictName, ...) (#66218), and GRANT can only target db.<name> (there is no dictionary privilege object). This PR keeps that model and only adds the checks that were missing, so SHOW/EXPLAIN/REFRESH now behave consistently with CREATE/DROP. The name-collision problem is real, but it predates this change and affects CREATE/DROP the same way; fixing it means either rejecting table/dictionary name collisions in both CREATE DICTIONARY and CREATE TABLE, or introducing a dedicated privilege object for internal and Ranger controllers. That is out of scope here and tracked in #67345.
| import org.apache.doris.catalog.Env; | ||
| import org.apache.doris.common.ErrorCode; | ||
| import org.apache.doris.common.ErrorReport; | ||
| import org.apache.doris.datasource.InternalCatalog; |
There was a problem hiding this 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.
There was a problem hiding this comment.
[P1] Dictionaries are authorized with the table privilege key of the internal catalog by design: CREATE/DROP DICTIONARY already use checkTblPriv(internal, db, dictName, ...) (#66218), and GRANT can only target db.<name> (there is no dictionary privilege object). This PR keeps that model and only adds the checks that were missing, so SHOW/EXPLAIN/REFRESH now behave consistently with CREATE/DROP. The name-collision problem is real, but it predates this change and affects CREATE/DROP the same way; fixing it means either rejecting table/dictionary name collisions in both CREATE DICTIONARY and CREATE TABLE, or introducing a dedicated privilege object for internal and Ranger controllers. That is out of scope here and tracked in #67345.
[P2] Fixed in 9a1fbfe: after the LOAD check the command now also verifies SELECT on the source table (dictionary.getSourceCtlName/DbName/TableName, the same check CREATE DICTIONARY does) before calling dataLoad(), so a request lacking either privilege is rejected before the shared status is switched to LOADING. Covered by the new LOAD-without-source-SELECT case in test_ddl_dictionary_auth.
| connect(viewer, "${pwd}", context.config.jdbcUrl) { | ||
| sql """use ${dbName}""" | ||
| sql """REFRESH DICTIONARY ${dictName}""" | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this comment.
Fixed in 9a1fbfe: the suite now calls waitDictionaryReady after CREATE DICTIONARY and after each refresh, so the positive REFRESH DICTIONARY deterministically exercises authorization instead of the LOADING guard. It also adds the case where the viewer has LOAD but no SELECT on the source table, which is now rejected before the dictionary is switched to LOADING.
c9e968f to
9a1fbfe
Compare
|
/review |
There was a problem hiding this comment.
Automated review of 9a1fbfe84513a6635675d79d110bc0fa25d1bce4 — REQUEST_CHANGES
Three distinct issues are attached inline.
Critical checkpoint conclusions:
- Goal and proof: The SHOW/EXPLAIN/REFRESH checks improve whole-table authorization and the readiness waits make the intended regression paths deterministic, but the authorization goal is incomplete because scalar dictionary reads still bypass it, column-authorized refreshes are rejected, and an all-hidden SHOW invokes the all-dictionary status contract.
- Scope and focus: The diff is focused. There was no additional user-provided review focus. The existing same-name table/dictionary privilege thread, the earlier source-SELECT thread, and the readiness thread were treated as duplicate fences and were not repeated.
- Concurrency and lifecycle: The changed commands hold no metadata lock across authorization, planning, or RPC. Existing status CAS, failure rollback, concurrent drop, and forwarding behavior introduced no additional finding; the empty-visible-set status RPC is covered inline.
- Compatibility and parallel paths: No configuration, persistence, wire-format, rolling-upgrade, FE-BE variable, or public symbol compatibility change was found. Column-level native/Ranger grants regress, and the parallel
dict_get/dict_get_manyread paths remain unauthorized. - Tests and results: The new waits remove the initial-load race; the whole-table negative/positive and cloud cases reach their intended guards. Missing coverage includes column-only grants, scalar dictionary reads, and the empty-visible-set status path. No build or tests were run, as required by the review-only task.
- Observability and performance: An all-hidden SHOW still fans status RPCs to all alive BEs, emits misleading missing-dictionary warnings, and can fail on an unrelated response; this is covered inline. No separate logging or performance issue remains.
- Transactions, persistence, and data writes: No new transaction or persisted-state format is introduced. The preflight runs before LOADING publication, and later authorization failure retains the existing rollback behavior.
Review status: complete after two rounds. Both Round 2 normal reviewers and the separate risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS; three candidates were accepted, one real but pre-existing design issue was dismissed after exact base/head comparison, and all remaining candidates were duplicate-fenced or dismissed with evidence.
| // 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, |
There was a problem hiding this 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).
There was a problem hiding this comment.
Fixed in 00993be: SHOW DICTIONARIES now returns before status collection when the visible set is empty, so an all-hidden (or empty / non-matching LIKE) result no longer triggers the empty-id-list "ALL" semantics of get_dictionary_status, the per-BE fan-out, the misleading missing-dictionary warnings, or a command failure on a bad BE response.
| // 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(), |
There was a problem hiding this 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.
There was a problem hiding this comment.
Fixed in 00993be: the source preflight now uses the column-aware contract — it collects the dictionary's source column names and calls AccessControllerManager.checkColumnsPriv(..., SELECT), the same authorization the generated INSERT applies after BindSink projects the source to the dictionary schema. A caller with dictionary LOAD plus column-level SELECT on exactly the dictionary's source columns is authorized again (native and Ranger column policies alike), and a missing privilege is still rejected before LOADING is published. Added a column-only regression case (grant SELECT_PRIV(id)/(username) then REFRESH succeeds), and the negative cases now assert status NORMAL and an unchanged LastUpdateResult to prove the preflight, not the INSERT, rejected the request.
| 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, |
There was a problem hiding this 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.
There was a problem hiding this comment.
Fixed in 00993be: both DictGet.customSignatureDict and DictGetMany.customSignatureDict now check SELECT on the dictionary (internal catalog key, same as the commands) before DictionaryManager.getDictionary, so an unauthorized caller gets the same SELECT command denied whether or not the name exists — no existence or schema probing, and nothing reaches ExpressionTranslator. Internal paths without a ConnectContext are unaffected. Regression now covers: hidden user's dict_get denied; still denied with db-level SHOW_VIEW/LOAD plus source column grants; allowed and returning the value after SELECT on the database is granted (a table-level grant on the dictionary name itself is impossible today because GRANT validates table existence — the namespace issue tracked in #67345).
9a1fbfe to
00993be
Compare
|
/review |
There was a problem hiding this comment.
Requesting changes: three distinct authorization/lifecycle gaps remain on 00993be115ab21771fd9a2a04637e61fb5c8c6bd.
- Nereids SQL-cache replay does not record dictionary privilege or ID/version dependencies, so cached values can survive SELECT revocation or refresh.
- REFRESH preflight uses dictionary-DDL column spelling instead of the canonical source-column identity used by the load scan.
- Reusable server-prepared short-circuit point queries retain the translated dictionary expression without revalidating privilege or version.
Checkpoint conclusions:
- Goal, scope, and proof: the six-file change is focused, and direct SHOW/EXPLAIN/REFRESH/
dict_getbehavior is substantially improved, but the read-authorization and pre-LOADINGgoals are incomplete because of the three inline findings. Previously raised namespace, empty-status-RPC, table-wide-preflight, and async-wait points were treated as duplicate fences. - Concurrency and lifecycle: no additional lock-order, deadlock, drop/recreate, commit/abort, or rollback defect was found. The remaining lifecycle defects are retained SQL/prepared state outliving privilege/version changes and the mixed-case path allowing authorization failure after
LOADINGpublication. - Configuration and compatibility: no new configuration, storage format, EditLog schema, function symbol, Thrift field, or FE/BE variable is introduced. The prepared short-circuit issue is reachable under default session settings on a qualifying point table; the SQL-cache issue is reachable with supported BE folding plus the default SQL cache.
- Parallel and conditional paths:
dict_getanddict_get_manyshare both cache/reuse defects. Fresh direct, ordinary prepared, forwarded, Arrow Flight, internal, native, and Ranger paths were rechecked; no fourth distinct bypass or controller mismatch was found. - Tests and results: the regression covers native direct visibility/read/refresh paths, deterministic load waits, column grants, and denial before state mutation. It does not cover the three demonstrated triggers: cache revoke/refresh, mixed-case source columns, and reusable prepared point-query revoke/refresh. No build or test was run because the review prompt is review-only.
- Persistence, writes, failure handling, observability, and performance: REFRESH otherwise reuses the existing versioned load/commit/rollback flow and fails loudly; existing status/load/RPC diagnostics are adequate, and no separate material performance or persistence issue was found.
- User focus: no additional user-provided review focus was supplied.
| // 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, |
There was a problem hiding this 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.
There was a problem hiding this 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.
| // 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) |
There was a problem hiding this 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.
There was a problem hiding this 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.
| // 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, |
There was a problem hiding this 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.
There was a problem hiding this 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.
00993be to
c0c06d6
Compare
|
/review |
There was a problem hiding this comment.
Requesting changes on c0c06d6ea3686b1cd616adbc5a96617e6c96d4b1. Three distinct current-head issues remain:
- P1: BE constant folding can erase a dictionary dependency before the new prepared short-circuit guard, allowing reuse across privilege revocation or dictionary refresh.
- P1: the function-local read check runs while persisted views are rebound as the invoker, breaking Doris's established view-only privilege boundary.
- P2: refresh preflight uses the persisted source-table spelling rather than the resolved table identity, so case-insensitive lookup can disagree with downstream INSERT authorization.
Critical checkpoint conclusions:
- Goal, scope, and security model: The focused nine-file patch substantially improves dictionary visibility, direct-read authorization, refresh preflight, SQL-cache exclusion, and ordinary prepared revalidation, but the RBAC/revalidation goal remains incomplete through the three inline findings. Restricted authenticated users and privilege isolation are in scope under the repository threat model. No additional user review focus was supplied.
- Concurrency and lifecycle: Refresh otherwise authorizes before publishing
LOADING, retains downstream INSERT authorization, restores failures, and uses exact dictionary-generation fences; no separate lock-order, deadlock, drop/recreate, or status-RPC lifecycle issue remains. The retainedShortCircuitQueryContextlifecycle is the P1 exception. - Configuration and compatibility: No new configuration, storage/wire format, public symbol, edit-log schema, FE-BE variable, or mixed-version contract is introduced. The supported
enable_fold_constant_by_besetting exposes the folded-dependency path, and persisted-view behavior regresses relative to the existing view authorization contract. - Parallel paths and conditions: Both
dict_getanddict_get_many, direct/nested/predicate forms, FE/BE SQL cache, ordinary/server-prepared execution, CREATE/ALTER/persisted/nested views, native/external controllers, SHOW/EXPLAIN/REFRESH, follower forwarding, and lower-case table-name modes were traced. Remaining variants deduplicate into the three comments. - Tests and expected results: The regressions cover direct RBAC, SHOW/EXPLAIN visibility, pre-
LOADINGdenial, canonical source columns, SQL-cache exclusion, and a nonconstant prepared query. They miss literal folding plus revoke/refresh-after-prime, mixed-case source-table qualifiers, and view-only reads for both functions. Expected-result and cleanup logic were inspected; no build or tests were run because the authoritative review bundle forbids them. - Persistence, writes, failure handling, observability, and performance: No new persisted state or transaction protocol is added. Refresh reuses the existing load/rollback flow, although the qualifier mismatch can defer denial until after
LOADING. No distinct crash-safety, logging/metrics, or material performance issue remains.
Review convergence: complete after three rounds. Round 3's three normal full passes and separate risk-focused pass found no fourth distinct issue, so the final cap converged and is not capped/incomplete. Existing live threads were treated as hard duplicate fences.
| .when(this::scanMatchShortCircuitCondition) | ||
| ).when(this::filterMatchShortCircuitCondition))) | ||
| .thenApply(ctx -> { | ||
| if (projectReadsDictionary(ctx.root.child())) { |
There was a problem hiding this comment.
[P1] Preserve dictionary dependencies across constant folding
The existing prepared-reuse issue is still reachable when enable_fold_constant_by_be=true. For ResultSink -> Project(dict_get('db.d','v',1)) -> Filter(id = ?) -> Scan(point_table), plan normalization folds the all-literal dictionary call before this table/physical rule runs (FoldConstantRuleOnBE does not skip either dictionary function). This guard then sees only a Literal, marks the query short-circuit, and ShortCircuitQueryContext can reuse that serialized old value after the dictionary privilege or version changes. The new regression uses nonconstant id and never enables BE folding, so it cannot catch this. Please preserve a statement-level dictionary dependency through rewrites (or prevent these calls from folding) and cover literal dict_get/dict_get_many with both revoke-after-prime and refresh-after-prime prepared executions.
There was a problem hiding this 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.
| .map(definition -> definition.getOriginColumn().getName()) | ||
| .collect(Collectors.toCollection(LinkedHashSet::new)); | ||
| Env.getCurrentEnv().getAccessManager().checkColumnsPriv(ctx, dictionary.getSourceCtlName(), | ||
| dictionary.getSourceDbName(), dictionary.getSourceTableName(), sourceColumns, |
There was a problem hiding this comment.
[P2] Use the resolved source table name in this preflight
The column set is canonical now, but the table argument still comes from the spelling persisted by CREATE DICTIONARY. Under lower_case_table_names=1/2, that spelling can differ from the resolved Table.getName(): native ColPrivilegeKey compares the table string exactly, while the generated INSERT's UserAuthentication check uses the resolved table name. A canonical column grant can therefore be rejected here even though the INSERT would pass; the inverse spelling can pass here and fail only after dataLoad() publishes LOADING. Please authorize with the resolved source qualifiers (or persist them canonically) and add a mixed-case source-table regression with column-only grants.
There was a problem hiding this 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.
| // 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, |
There was a problem hiding this comment.
[P1] Preserve the persisted-view privilege boundary
This invocation-time check also runs while BindRelation reparses a persisted view definition under the querying user's ConnectContext, before the analyzed body is wrapped in LogicalView. A user granted only SELECT on db.v therefore cannot execute SELECT against a view created as SELECT dict_get('db.d','v',1), even though CheckPrivileges.visitLogicalView explicitly authorizes the view and stops before its protected children (the established behavior covered by test_select_view_auth). DictGetMany has the same check, and translation calls this helper again after view inlining. Please preserve the direct-call and CREATE/ALTER checks while carrying a trusted persisted-view scope through binding and translation, and add view-only regressions for both functions plus a direct-call negative control.
There was a problem hiding this 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/test_select_view_auth re-run as the existing view-boundary control.
c0c06d6 to
e99a7ec
Compare
### What problem does this PR solve? Issue Number: None Related PR: apache#66218 Problem Summary: `SHOW DICTIONARIES` and `EXPLAIN DICTIONARY` did not check any privilege. Any user who can `USE` a database (which only needs a privilege on some table of that database) could list every dictionary of the database together with its source table name, status and BE data distribution, and describe its columns. This is inconsistent with `SHOW TABLES`, which hides tables the user cannot show, and with `CREATE/DROP DICTIONARY`, which already require privileges on the dictionary name (apache#66218). Dictionaries are authorized like tables of the internal catalog, so: - `SHOW DICTIONARIES` skips dictionaries the user has no `SHOW` privilege on, the same way `SHOW TABLES` filters tables. When nothing is visible it returns before collecting BE status: an empty id list means "all dictionaries" to the `get_dictionary_status` RPC, so the previous code fanned status RPCs to every alive BE for that case (also reachable before this change through an empty database or a non-matching `LIKE`). - `EXPLAIN DICTIONARY` requires `SHOW` on the dictionary, like `DESCRIBE` on a table. - `REFRESH DICTIONARY` checks `LOAD` on the dictionary up front, like `DROP DICTIONARY` checks `DROP`. The internal `INSERT INTO` already required it, so nobody loses the ability to refresh; the check now happens before the dictionary is resolved and before its status is switched to `LOADING`. The checks run before the dictionary is looked up, so a denied user cannot probe whether a dictionary exists either. Reading dictionary values through `dict_get()` / `dict_get_many()` has never been authorized; that is a separate change and is tracked in apache#67345. ### Release note None ### Check List (For Author) - Test - [x] Regression test: `auth_call/test_ddl_dictionary_auth` now covers a user with a privilege on another table of the database (must not see, describe or refresh the dictionary), `SHOW_VIEW` on the database (sees the dictionary and its source table, may describe it, still cannot refresh, and the denied refresh leaves the dictionary untouched), and `LOAD` on the database (may refresh). - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - Behavior changed: - [ ] No. - [x] Yes. Users without `SHOW` on a dictionary no longer see it in `SHOW DICTIONARIES` and cannot `EXPLAIN DICTIONARY` it. `REFRESH DICTIONARY` still needs `LOAD` on the dictionary, but is now rejected before the dictionary is touched. - Does this need documentation? - [ ] No. - [x] Yes. The privilege requirements of the three statements should be documented. Claude-Session: https://claude.ai/code/session_01X9KukfTLYxHmP6iYyEnQtW
e99a7ec to
0b0428f
Compare
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16717 ms |
TPC-DS: Total hot run time: 81589 ms |
ClickBench: Total hot run time: 14.75 s |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: #66218
Problem Summary:
SHOW DICTIONARIESandEXPLAIN DICTIONARYdid not check any privilege. Anyuser who can
USEa database (which only needs a privilege on some table ofthat database) could list every dictionary of the database together with its
source table name, status and BE data distribution, and describe its columns.
REFRESH DICTIONARYonly failed inside the internalINSERT INTO, after thedictionary had been looked up and switched to
LOADING.This is inconsistent with
SHOW TABLES, which hides tables the user cannotshow, and with
CREATE/DROP DICTIONARY, which already require privileges on thedictionary name (#66218).
Dictionaries are authorized like tables of the internal catalog, so:
SHOW DICTIONARIESnow skips dictionaries the user has noSHOWprivilegeon, the same way
SHOW TABLESfilters tables.EXPLAIN DICTIONARYrequiresSHOWon the dictionary, likeDESCRIBEon atable.
REFRESH DICTIONARYchecksLOADon the dictionary up front. This is theprivilege the internal
INSERT INTOalready required, so nobody loses theability to refresh; the check now happens before the dictionary is resolved
and before its status is flipped to
LOADING.The checks run before the dictionary is looked up, so a denied user cannot
probe whether a dictionary exists either.
Release note
None
Check List (For Author)
Test
auth_call/test_ddl_dictionary_authnow covers auser with a privilege on another table of the database (must not see,
describe or refresh the dictionary),
SHOW_VIEWon the database (seesthe dictionary and its source table, may describe it, still cannot
refresh), and
LOADon the database (may refresh).Behavior changed:
SHOWon a dictionary no longer see it inSHOW DICTIONARIESand cannotEXPLAIN DICTIONARYit.REFRESH DICTIONARYstill needsLOADon the dictionary, but is now rejectedbefore the dictionary is touched.
Does this need documentation?
documented.
https://claude.ai/code/session_01X9KukfTLYxHmP6iYyEnQtW