Skip to content

[fix](auth) Add privilege checks to SHOW/EXPLAIN/REFRESH DICTIONARY - #67343

Draft
mrhhsg wants to merge 1 commit into
apache:masterfrom
mrhhsg:fix/dictionary-command-privileges
Draft

[fix](auth) Add privilege checks to SHOW/EXPLAIN/REFRESH DICTIONARY#67343
mrhhsg wants to merge 1 commit into
apache:masterfrom
mrhhsg:fix/dictionary-command-privileges

Conversation

@mrhhsg

@mrhhsg mrhhsg commented Aug 31, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: None

Related PR: #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.
REFRESH DICTIONARY only failed inside the internal INSERT INTO, after the
dictionary had been looked up and switched to LOADING.

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 (#66218).

Dictionaries are authorized like tables of the internal catalog, so:

  • SHOW DICTIONARIES now skips dictionaries the user has no SHOW privilege
    on, the same way SHOW TABLES filters tables.
  • EXPLAIN DICTIONARY requires SHOW on the dictionary, like DESCRIBE on a
    table.
  • REFRESH DICTIONARY checks LOAD on the dictionary up front. This is the
    privilege the internal INSERT INTO already required, so nobody loses the
    ability 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

    • 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 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.
    • 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.
    • Yes. The privilege requirements of the three statements should be
      documented.

https://claude.ai/code/session_01X9KukfTLYxHmP6iYyEnQtW

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@mrhhsg

mrhhsg commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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}"""
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mrhhsg

mrhhsg commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review of 9a1fbfe84513a6635675d79d110bc0fa25d1bce4REQUEST_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_many read 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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@mrhhsg
mrhhsg force-pushed the fix/dictionary-command-privileges branch from 9a1fbfe to 00993be Compare August 31, 2026 14:13
@mrhhsg

mrhhsg commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: three distinct authorization/lifecycle gaps remain on 00993be115ab21771fd9a2a04637e61fb5c8c6bd.

  1. Nereids SQL-cache replay does not record dictionary privilege or ID/version dependencies, so cached values can survive SELECT revocation or refresh.
  2. REFRESH preflight uses dictionary-DDL column spelling instead of the canonical source-column identity used by the load scan.
  3. 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_get behavior is substantially improved, but the read-authorization and pre-LOADING goals 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 LOADING publication.
  • 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_get and dict_get_many share 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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

### 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.
`REFRESH DICTIONARY` only failed inside the internal `INSERT INTO`, after the
dictionary had been looked up and switched to `LOADING`. Reading values through
`dict_get()` / `dict_get_many()` never checked any privilege at all.

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` now skips dictionaries the user has no `SHOW` privilege
  on, the same way `SHOW TABLES` filters tables.
- `EXPLAIN DICTIONARY` requires `SHOW` on the dictionary, like `DESCRIBE` on a
  table.
- `REFRESH DICTIONARY` checks `LOAD` on the dictionary and column-aware
  `SELECT` on the dictionary's source columns up front (`checkColumnsPriv` on
  the source columns' own names, the same contract the generated `INSERT`
  enforces through `CheckPrivileges`, so column-level grants keep working even
  when the dictionary definition spells the columns differently). These are the
  privileges the reload already required; the checks now happen before the
  dictionary is resolved and before its status is flipped to `LOADING`, so an
  unauthorized request can no longer block concurrent refreshes while its
  INSERT is being planned.
- `dict_get()` / `dict_get_many()` require `SELECT` on the dictionary before
  resolving it, so a user who cannot see a dictionary cannot read its values or
  probe whether it exists.
- Statements that read a dictionary are kept out of the sql cache and are not
  short-circuited as reusable point-query plans of a server prepared statement:
  neither mechanism records the dictionary's privilege or version, so a cached
  result / reused plan could survive a revoke or a refresh. Dictionaries are a
  cache already, so the lost sql-cache hit is not a real cost.
- `SHOW DICTIONARIES` returns early when the visible set is empty: an empty id
  list means "all dictionaries" to the BE status RPC (`get_dictionary_status`),
  so the previous code fanned status RPCs to every alive BE (also reachable
  before this change via an empty database or a non-matching `LIKE`).

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
    - [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), `LOAD` on the database without `SELECT` on the source table
      (rejected before the dictionary starts loading), column-level `SELECT`
      on the source columns (may refresh), and `dict_get()` denied until
      `SELECT` on the dictionary is granted. New
      `auth_call/test_dictionary_read_auth_cache` (nonConcurrent) covers a
      dictionary spelling its columns differently from the source table, a
      primed sql-cache statement that is denied right after the revoke, and a
      server prepared point query whose second EXECUTE is denied after the
      revoke.
    - [ ] 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 and (column-aware)
      `SELECT` on the source columns, but is now rejected before the dictionary
      is touched. `dict_get()` / `dict_get_many()` now require `SELECT` on the
      dictionary; internal paths without a user context are unaffected.
      Statements reading a dictionary no longer use the sql cache or a reused
      short-circuit point-query plan.

- 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
@mrhhsg
mrhhsg force-pushed the fix/dictionary-command-privileges branch from 00993be to c0c06d6 Compare September 1, 2026 04:44
@mrhhsg

mrhhsg commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

/review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants