[fix](auth) Add missing privilege checks for several Nereids commands - #66218
Merged
Conversation
Several Nereids commands do not check privileges before executing. The Nereids path in StmtExecutor dispatches straight to Command.run(), so every command has to enforce its own privileges, and these ones were missed: - AdminSetEncryptionRootKeyCommand / AdminRotateTdeRootKeyCommand: now require ADMIN, like the other ADMIN SET statements. - DropCatalogRecycleBinCommand: now requires ADMIN. It takes a raw object id and erases instantly, so it cannot be authorized at db/table level. - CreateDictionaryCommand: requires CREATE on the target database plus SELECT on the source table, since the load task runs internally. DropDictionaryCommand requires DROP on the database. - AddConstraintCommand / DropConstraintCommand: require ALTER on the table. For a foreign key the referenced table is checked as well, because it gets a reverse reference registered on it. In DropConstraintCommand the check is placed after the name-based fallback so neither resolution path can skip it. - WarmUpClusterCommand / CancelWarmUpJobCommand / DropStageCommand: require ADMIN, matching CreateStageCommand which already did. Add auth_call regression cases for constraint, dictionary and recycle bin covering both the denied and the granted path.
CalvinKirs
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
July 29, 2026 06:52
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Member
Author
|
/review |
…ions Follow-up on the privilege checks added in the previous commit. Dictionary: use checkTblPriv on the dictionary itself instead of checkDbPriv on its database, matching CREATE/DROP TABLE and CREATE/DROP MTMV. checkDbPriv would reject a user who was granted the privilege on the dictionary name directly. Warm up: WARM UP CLUSTER now requires USAGE on the source and destination compute groups instead of global ADMIN, matching UseCloudClusterCommand, plus SELECT on each table named by WITH TABLE. CANCEL WARM UP JOB keeps ADMIN because CloudWarmUpJob records no owner, so a job cannot be scoped to the user who created it.
…intCommand WarmUpClusterOnTablesParseTest builds a bare ConnectContext with no user identity, so the privilege check added to WarmUpClusterCommand.validate() hit a NullPointerException in getRolesByUserWithLdap(). Neither checkGlobalPriv(ctx, ...) nor checkCloudPriv(ctx, ...) tolerates a null identity, which is fine for the real execution path since StmtExecutor always runs with an authenticated context. Give the test an identity and let it bypass the checks, since it covers ON TABLES parsing rather than authorization. Also make AddConstraintCommand use the ConnectContext passed to run() instead of the static ConnectContext.get(). DropConstraintCommand already did, and the static one is null on any path that has no thread local set.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29006 ms |
Contributor
TPC-DS: Total hot run time: 177551 ms |
Contributor
ClickBench: Total hot run time: 25.03 s |
Contributor
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29290 ms |
Contributor
TPC-DS: Total hot run time: 177166 ms |
Contributor
ClickBench: Total hot run time: 24.94 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Blocking authorization gaps remain in the constraint, dictionary, and cloud warm-up changes; the six inline comments contain the concrete reproductions and fixes.
Checkpoint conclusions:
- Goal and effective privileges: incomplete. The intended predicates are ALTER on constraint endpoints, CREATE/DROP for dictionaries plus source SELECT, global ADMIN for the administrative commands, and compute-group USAGE plus table SELECT where applicable. The accepted findings show cross-table mutation, pre-authorization metadata disclosure, row/data-policy bypass, and incomplete warm-up table authorization.
- Scope and parallel paths: the diff is small, but the review traced constraint cascades/replay, dictionary creation/load/refresh/read, all warm-up syntax modes and job lifecycle, forwarding, privilege inheritance, and the changed tests. No extra
review_focus.txtguidance was supplied, so the whole PR was reviewed. - Correctness, concurrency, lifecycle, and persistence: constraint metadata changes remain manager-locked and replay/MTMV invalidation are equivalent; dictionary loads and warm-up refreshes run asynchronously, and their missing caller identity is captured by the accepted policy/recheck findings. Warm-up journaling and forwarded context installation are otherwise consistent, with no separate lock, deadlock, atomicity, crash, or failover defect found.
- Configuration and compatibility: no config, FE-BE variable/contract, function-symbol, storage-format, or rolling-upgrade change is introduced.
- Performance and observability: the added privilege lookups are not a material hot-path cost. Existing logs identify the affected operations; no separate metrics gap was found, while unauthorized matched-table visibility is part of the accepted
ON TABLESissue. - Tests: the added suites cover only UNIQUE constraints, basic dictionary grants, and recycle-bin ADMIN, while the warm-up unit test explicitly bypasses auth. They do not cover FK cascades, metadata-order denial, row/mask policies, warm-up catalog mismatch, dynamic matches, or forwarding. Per the review contract, no local build or tests were run.
| TableNameInfo tableNameInfo = TableNameInfoUtils.fromCatalogDb( | ||
| table.getDatabase().getCatalog(), table.getDatabase(), table); | ||
| ImmutableList<String> columns = columnsAndTable.first; | ||
| checkAlterPriv(ctx, tableNameInfo); |
Contributor
There was a problem hiding this comment.
The relation and columns have already been bound by extractColumnsAndTable() before this ALTER gate; ANALYZED_PLAN returns before the rewrite-stage privilege rule. Consequently a caller without ALTER can distinguish missing tables/columns from valid protected ones, and the FK path similarly probes the referenced side before its second check. Normalize both table names and authorize them first, then bind columns; add no-grant existing/missing-object tests plus an ALTER-without-SELECT success case.
| throw new AnalysisException("Failed to create dictionary: " + e.getMessage()); | ||
| } | ||
|
|
||
| // 2. Check auth. Must run after validateAndSet(), which fills in the default catalog/db names. |
Contributor
There was a problem hiding this comment.
validateAndSet() has already resolved the source catalog/DB/table, read its full schema, and validated column names/types before either new check runs. A user without SELECT can therefore distinguish missing objects/columns and invalid types from the eventual access denial. Split default-name normalization from metadata binding, run the target/source checks on the normalized names, and only then resolve and validate the source.
| String srcCtl = createDictionaryInfo.getSourceCtlName(); | ||
| String srcDb = createDictionaryInfo.getSourceDbName(); | ||
| String srcTbl = createDictionaryInfo.getSourceTableName(); | ||
| if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx, srcCtl, srcDb, srcTbl, |
Contributor
There was a problem hiding this comment.
A plain table-level SELECT check does not establish that the creator may read the raw source rows. createDictionary() schedules dataLoad(null, ...), which builds an ADMIN context; LogicalCheckPolicy skips row filters and data masks for ADMIN, so a policy-restricted user can create a dictionary containing rows/unmasked values their own SELECT would hide and read them with dict_get. Preserve the effective principal/policies for the load (and refresh), or otherwise prevent policy-bound sources from being materialized this way.
| } | ||
| // must be checked on both paths above: table resolution failing (which includes an | ||
| // authorization failure) falls back to a name-only lookup that binds nothing. | ||
| if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx, tableNameInfo.getCtl(), |
Contributor
There was a problem hiding this comment.
This authorizes only the named table, but dropping a primary key calls ConstraintManager.cascadeDropForeignKeys() and removes the actual FK constraints from every table in foreignTableInfos. A user with ALTER on the parent but none on a child can therefore delete the child's constraint. Require ALTER on every referencing table before the atomic mutation (or reject the cascade and require separately authorized FK drops), and cover the cross-table PK/FK case rather than only UNIQUE.
|
|
||
| // check auth. warming up moves data between compute groups, so require USAGE on both ends | ||
| // instead of global ADMIN. Keep it aligned with UseCloudClusterCommand. | ||
| checkComputeGroupUsage(connectContext, dstCluster); |
Contributor
There was a problem hiding this comment.
This USAGE gate admits non-admin ON TABLES jobs, but that mode then scans every internal DB/table and stores matching IDs/names without table authorization; refreshAllTableFilters() adds future matches without an effective identity to reauthorize them. A user with USAGE on both compute groups can therefore warm protected tables. Either keep pattern jobs ADMIN-only, or require SELECT for every initial match and persist the principal so future matches are reauthorized.
| if (table == null) { | ||
| ErrorReport.reportAnalysisException(ErrorCode.ERR_BAD_TABLE_ERROR, tableNameInfo.getTbl()); | ||
| } | ||
| if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(connectContext, tableNameInfo.getCtl(), |
Contributor
There was a problem hiding this comment.
The object resolved and later warmed is always internal.db.table, but this check uses the catalog supplied in SQL and the downstream triple drops the catalog. Thus SELECT on ext.db.t can authorize warming the same-named internal.db.t. The internal lookup and OlapTable cast also happen before this check, leaking object/type existence to denied users. Reject non-internal catalogs and authorize the internal fully qualified name before lookup/cast, or carry one resolved catalog/object consistently end to end.
…ternal warm up target Three follow-ups on the privilege checks: - DROP CONSTRAINT on a primary key cascades into ConstraintManager.cascadeDropForeignKeys(), which deletes the foreign key constraints of every referencing table. Require ALTER on those tables too. - WARM UP ... ON TABLES selects tables by pattern over the whole internal catalog and keeps re-matching in the background with no identity, so compute group USAGE is not enough to bound it. Require global ADMIN for that mode. - WARM UP ... WITH TABLE always resolves and warms the internal catalog, but the SELECT check used the catalog written in the SQL, so SELECT on ext_ctl.db.tbl authorized warming internal.db.tbl. Authorize the internal name, and do it before the db/table lookup. Also extend the constraint regression case with the cross-table PK/FK drop.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review status: changes requested for head 5ac9cfcd467b3a9759157677358b50aeed58eb50.
I reviewed all 14 changed files plus command dispatch, forwarding, privilege resolution, dictionary load/consume lifecycle, warm-up job lifecycle, constraint locking/journaling, and the added tests. Three new inline issues remain: the PK/FK authorization TOCTOU, dictionary consumers with no query-time authorization, and the regression test that never reaches its claimed fallback path.
Existing-thread reconciliation:
- Still unresolved: AddConstraint binds before ALTER (r3679441033), CreateDictionary binds before authorization (r3679441037), and ADMIN dictionary loads/refreshes bypass creator policies (r3679441039).
- Addressed in the current head: ON TABLES is ADMIN-only (r3679441044) and explicit warm-up checks internal-catalog SELECT before lookup (r3679441046). The current child-table checks address the static cascade scope from r3679441042 but remain non-atomic as described inline.
Critical checkpoints:
- Goal and scope: this is a focused privilege-hardening change, but it is incomplete while the above RBAC gaps remain.
- Concurrency and atomicity: ConstraintManager serializes mutation, but authorization is over a live child set outside the lock; this is blocking. No other changed command introduces concurrent shared-state mutation.
- Lifecycle: dictionary initial/automatic loads, manual refresh, and consumers were traced; the missing durable policy principal is already covered by an existing thread, and query-time consumers add a distinct bypass. Warm-up rematching and cancellation are correctly scoped after the current fixes.
- Forwarding and parallel paths: the original caller identity is preserved to the master; no alternate public TDE, recycle-bin, stage, or warm-up mutation bypass was found.
- Compatibility, configuration, and protocol: no configuration, FE-BE protocol, storage-format, or rolling-upgrade change.
- Persistence and data writes: constraint logs replay deterministically, but replay cannot repair an unauthorized raced cascade; no other transaction or persistence defect was found.
- Tests: the new auth regressions cover basic denied/granted paths, but the fallback claim is unexercised and there is no two-user dictionary-consumer or latch-based constraint-race coverage. Per the runner contract, I performed static review only and did not execute tests.
- Performance and observability: the new checks are lightweight; no additional metric/logging requirement or material performance issue was found.
User focus: no additional focus was provided; the full PR was reviewed.
| // dropping a primary key cascades into ConstraintManager.cascadeDropForeignKeys(), which | ||
| // deletes the foreign key constraints of every referencing table, so those tables have to | ||
| // be authorized too. Checked before dropConstraint() because the cascade is atomic. | ||
| for (TableNameInfo fkTableInfo : ((PrimaryKeyConstraint) constraint).getForeignTableInfos()) { |
Contributor
There was a problem hiding this comment.
The child checks and cascade are separated by an unlocked TOCTOU window. getConstraint() releases the manager read lock and returns the live PrimaryKeyConstraint; getForeignTableInfos() is only an unmodifiable view of its mutable ArrayList. Another request can add an FK after this loop but before dropConstraint() takes the write lock, so the cascade removes that new child's FK even though this caller was never checked for ALTER there (and mutation during iteration can also throw ConcurrentModificationException). Snapshot the target set, authorize outside the lock, then make the write-locked drop conditional on the same constraint identity/version and exact child set; retry on mismatch. Please add a latch-based ADD-FK/drop-PK test.
| String srcCtl = createDictionaryInfo.getSourceCtlName(); | ||
| String srcDb = createDictionaryInfo.getSourceDbName(); | ||
| String srcTbl = createDictionaryInfo.getSourceTableName(); | ||
| if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx, srcCtl, srcDb, srcTbl, |
Contributor
There was a problem hiding this comment.
This checks the source only for the creator, but dictionary consumers are not checked. DictGet.customSignatureDict() and DictGetMany.customSignatureDict() resolve the materialized descriptor directly from DictionaryManager without consulting AccessControllerManager/ConnectContext, and a query such as SELECT dict_get('db.secret_dict', 'value', 1) references no source relation that normal SELECT analysis could protect. User B can therefore read a dictionary created by user A despite having no grant on its source. Enforce a query-time rule for both functions (at minimum SELECT on the persisted source, aligned with the effective-policy model from the existing load thread) and add a two-user regression test.
| sql """use ${dbName}""" | ||
| sql """ALTER TABLE ${tableName} ADD CONSTRAINT ${constraintName} UNIQUE (id)""" | ||
|
|
||
| // dropping a constraint is refused as well, including when the user can not even resolve |
Contributor
There was a problem hiding this comment.
This case does not exercise the fallback it claims to cover. The user still has SELECT_PRIV, the internal table exists, and extractTable() uses ANALYZED_PLAN, which returns before the rewrite-stage CheckPrivileges rule; the later no-SELECT PK/FK cases therefore do not force the catch branch either. A regression that moves checkAlterPriv() into only the successful-resolution branch, or otherwise skips it after fallback, can leave this suite green. Add a deterministic case where table resolution fails while the unresolved name still maps to stored constraint metadata, then verify both the denied and ALTER-authorized outcomes.
Contributor
TPC-H: Total hot run time: 29303 ms |
Contributor
TPC-DS: Total hot run time: 177311 ms |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes.
New findings in this review:
- Constraint cascade correctness and authorization still rely on a lossy per-child reverse index. With two distinct FKs from one child to the same PK, dropping either FK removes the sole reverse entry; a later PK or parent-table drop misses the surviving FK. See the new inline comment.
- The new two-table ADD FOREIGN KEY, warm-up/cancel, TDE root-key, and
DROP STAGEauthorization boundaries have no denied/granted execution coverage. See the four coverage comments.
Existing-thread status after deduplication:
- Still unresolved at this head: ADD CONSTRAINT and CREATE DICTIONARY bind protected metadata before their new gates; dictionary loads run as ADMIN without the creator's row/data-mask policy and consumers remain unchecked; the PK-drop authorization snapshot is not identity/exact-set fenced against concurrent FK or same-name replacement changes; and the DROP CONSTRAINT fallback remains unproved by a deterministic test. These are already covered by the live threads and were not repeated inline.
- The current head does address the two earlier warm-up table-identity threads: dynamic
ON TABLESjobs are ADMIN-only, and fixed-table authorization now uses the internal object before lookup.
Critical checkpoints:
- Goal and tests: the PR adds direct checks, but the outstanding production paths and missing authorization matrices mean the stated RBAC fix is not yet proved end to end.
- Scope and compatibility: the code is generally focused; no configuration, FE/BE protocol, storage-format, or rolling-upgrade change was introduced.
- Concurrency, lifecycle, and persistence: constraint authorization is still separated from the write-locked mutation, and the new deterministic reverse-index undercount persists in manager state; dictionary policy/consumer lifecycle remains covered by existing blockers. No additional warm-up persistence defect was substantiated beyond the covered threads.
- Performance and observability: the added predicate checks and snapshots introduce no material hot-path cost, and no new logging/metrics blocker was found.
- Testing performed: static review only, as required by the review runner; no builds or tests were run. The changed regressions cover constraint, dictionary, and recycle-bin basics, but not the accepted authorization matrices or two-FK invariant.
- User focus:
review_focus.txtcontains no additional focus, so the whole PR was reviewed.
| } | ||
| readLock(); | ||
| try { | ||
| return ImmutableList.copyOf(((PrimaryKeyConstraint) constraint).getForeignTableInfos()); |
Contributor
There was a problem hiding this comment.
This snapshot assumes foreignTableInfos enumerates every live FK target, but it is only a per-table reverse index. A child can have fk1(a) -> parent(id) and fk2(b) -> parent(id): the second registration is deduplicated by table, then dropping either FK unconditionally removes the child's sole reverse entry while the other FK remains. This method subsequently returns an empty target set, and cascadeDropForeignKeys() uses the same list, so dropping the PK neither authorizes that child nor removes its surviving FK; checkNoReferencingForeignKeys() can also allow the parent table to be dropped. Track exact FK references/refcounts (or verify remaining FKs before removing the child entry) and add a two-FKs-one-child regression.
| // context an identity and let it bypass the privilege checks in validate() | ||
| connectContext.setCurrentUserIdentity(UserIdentity.ROOT); | ||
| connectContext.setNoAuth(true); | ||
| connectContext.setSkipAuth(true); |
Contributor
There was a problem hiding this comment.
The fixture now makes every validate() call run as ROOT with both auth bypass flags, so it cannot catch regressions in any of the new security behavior: source/destination USAGE, fixed-table SELECT, ON TABLES ADMIN, or CANCEL WARM UP JOB ADMIN. The existing cloud warm-up suites also invoke these statements as the suite administrator. Please add denied and granted cases with non-root identities for each gate (including independently missing source vs destination USAGE); otherwise these checks can be removed or wired to the wrong object while all tests remain green.
|
|
||
| @Override | ||
| public void run(ConnectContext ctx, StmtExecutor executor) throws Exception { | ||
| validate(); |
Contributor
There was a problem hiding this comment.
Neither TDE root-key command has an execution-level authorization test for these new gates. Please exercise both AdminRotateTdeRootKeyCommand and AdminSetEncryptionRootKeyCommand with non-admin and ADMIN contexts (mocking the key manager), and assert that denial happens before rotateRootKey/setRootKey while ADMIN reaches the call. At present either check can be removed or wired to the wrong context without a test failing.
|
|
||
| @Override | ||
| public void run(ConnectContext ctx, StmtExecutor executor) throws Exception { | ||
| validate(); |
Contributor
There was a problem hiding this comment.
This new ADMIN boundary is not exercised by any non-admin test; existing stage tests run as the suite administrator. Add a denied/granted command or cloud regression test and assert that a denied caller never reaches CloudEnv.dropStage(). Otherwise this validate() call can regress without any coverage signal.
| PROPERTIES ("replication_num" = "1"); | ||
| """ | ||
| sql """ALTER TABLE ${dbName}.${pkTable} ADD CONSTRAINT ${pkName} PRIMARY KEY (id)""" | ||
| sql """ALTER TABLE ${dbName}.${fkTable} ADD CONSTRAINT ${fkName} FOREIGN KEY (pk_id) REFERENCES ${pkTable}(id)""" |
Contributor
There was a problem hiding this comment.
This FK is created as the suite administrator, so it never exercises the new ALTER check on the referenced table; the restricted user only adds a single-table UNIQUE constraint and later tests PK-drop cascade. Add child-only ALTER (deny), parent-only ALTER (deny), and both-granted (succeed) FK-creation cases, and verify neither denial leaves an FK behind. Otherwise the second gate can be removed or checked against the wrong table while this suite remains green.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28858 ms |
Contributor
TPC-DS: Total hot run time: 166561 ms |
Contributor
ClickBench: Total hot run time: 23.96 s |
924060929
reviewed
Aug 6, 2026
924060929
left a comment
924060929
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the full diff against the privilege-check conventions (CreateStageCommand, UseCloudClusterCommand, CheckPrivileges, Auth/Role). Overall: the direction is right and I did not find a security hole — every check sits on the real execution path (run()/validate()), forwarding re-executes with the reconstructed user identity, the internal VCG warm-up path (CloudInstanceStatusChecker) never calls validate(), global ADMIN satisfies every predicate via checkGlobalInternal, and PatternMatcher.match(null) returns false so null db/cluster never bypasses. The ConstraintManager snapshot under the read lock and the cascade TOCTOU reasoning are sound. Four minor comments inline.
| */ | ||
| public void validate() throws AnalysisException { | ||
| // check auth | ||
| if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ConnectContext.get(), PrivPredicate.ADMIN)) { |
Contributor
There was a problem hiding this comment.
Nit: use the ctx passed to run() instead of the static ConnectContext.get() here. Commit 3 of this PR already switched AddConstraintCommand to the passed context because the static one is null on any path with no thread-local set, but the same pattern remains in four new validate() methods (this file, AdminSetEncryptionRootKeyCommand, DropCatalogRecycleBinCommand, DropStageCommand). Equivalent on the real execution path, but inconsistent within the PR — suggest threading ctx through all four.
| */ | ||
| public void validate(ConnectContext ctx) throws AnalysisException { | ||
| // check auth | ||
| if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ctx, PrivPredicate.ADMIN)) { |
Contributor
There was a problem hiding this comment.
Nit: the ADMIN check now runs before the disk-mode check, so in disk mode a non-admin user gets a misleading "Access denied...ADMIN" error instead of "The sql is illegal in disk mode". WarmUpClusterCommand keeps the cloud-mode check first (checks are placed after it) — consider the same ordering here for consistency.
| dbName = ctx.getDatabase(); | ||
| } | ||
| // check auth. dictionaries always live in the internal catalog. | ||
| if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx, InternalCatalog.INTERNAL_CATALOG_NAME, |
Contributor
There was a problem hiding this comment.
Nit: when no default database is selected, dbName stays null and checkTblPriv(db=null) always returns false (PatternMatcher.match(null) is false, so no wildcard bypass either), so the user gets an access-denied error instead of the previous "no database selected" style error from the dictionary manager. Pure UX regression — consider reporting the missing-db case explicitly before the check.
| */ | ||
| public void validate() throws AnalysisException { | ||
| // check auth | ||
| if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ConnectContext.get(), PrivPredicate.ADMIN)) { |
Contributor
There was a problem hiding this comment.
Test coverage note: the two TDE commands (this one and AdminRotateTdeRootKeyCommand) have no regression case even though the denied path is pure FE and testable in disk mode. The cloud-only commands (WARM UP CLUSTER / CANCEL WARM UP JOB / DROP STAGE) are acknowledged as uncovered — acceptable. Also: the constraint suite's comment honestly states the name-based fallback branch is covered by inspection only.
924060929
approved these changes
Aug 6, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
gavinchou
approved these changes
Aug 7, 2026
linrrzqqq
pushed a commit
to linrrzqqq/doris
that referenced
this pull request
Aug 10, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66218 Problem Summary: Dictionary refresh rebuilt its internal INSERT SELECT statement by concatenating raw target and source object names. Identifier text containing SQL syntax could therefore change the parsed query structure. Quote and escape every identifier component with the existing Nereids helper, and add a parser-level unit test that verifies hostile-looking names remain one source relation. ### Release note Fix dictionary refresh for quoted object names. ### Check List (For Author) - Test: Unit test added; not run locally because the required thirdparty protoc binary is unavailable in the worktree - Behavior changed: Yes; dictionary load SQL now treats all metadata names strictly as identifiers - Does this need documentation: No
linrrzqqq
pushed a commit
to linrrzqqq/doris
that referenced
this pull request
Aug 10, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66218 Problem Summary: Dictionary refresh rebuilt its internal INSERT SELECT statement by concatenating raw target and source object names. Identifier text containing SQL syntax could therefore change the parsed query structure. Quote and escape every identifier component with the existing Nereids helper, and add a parser-level unit test that verifies hostile-looking names remain one source relation. ### Release note Fix dictionary refresh for quoted object names. ### Check List (For Author) - Test: Unit test added; not run locally because the required thirdparty protoc binary is unavailable in the worktree - Behavior changed: Yes; dictionary load SQL now treats all metadata names strictly as identifiers - Does this need documentation: No
wyxxxcat
pushed a commit
to wyxxxcat/doris
that referenced
this pull request
Aug 17, 2026
…apache#66218) ### What problem does this PR solve? Problem Summary: The Nereids path in `StmtExecutor` dispatches straight to `Command.run()`, so each command has to enforce its own privileges. A number of commands never got that check, and can currently be executed by any authenticated user regardless of their grants. This PR adds the missing checks: | Command | Required privilege | Follows | | --- | --- | --- | | `AdminSetEncryptionRootKeyCommand`, `AdminRotateTdeRootKeyCommand` | global `ADMIN` | `AdminSetFrontendConfig`, `AdminSetTableStatus`, `AdminSetReplicaStatus`, `AdminCleanTrash` | | `DropCatalogRecycleBinCommand` | global `ADMIN` | `SHOW CATALOG RECYCLE BIN` already requires global `ADMIN`. `RECOVER` uses `ALTER_CREATE`, but it is name-scoped while erasing takes a raw object id, so it cannot be authorized at db/table level | | `CreateDictionaryCommand` | `CREATE` on the dictionary **and** `SELECT` on the source table | `CREATE TABLE` / `CREATE MTMV`. The `SELECT` check is needed because the load task runs internally, and unlike an MTMV the source table is not bound by the planner | | `DropDictionaryCommand` | `DROP` on the dictionary | `DROP TABLE` / `DROP MTMV` | | `AddConstraintCommand`, `DropConstraintCommand` | `ALTER` on the table. For a foreign key, also on the referenced table; for dropping a primary key, also on every referencing table | the rest of `ALTER TABLE` | | `WarmUpClusterCommand` | `USAGE` on the source and destination compute groups, plus `SELECT` on each table named by `WITH TABLE`. `ON TABLES` additionally requires global `ADMIN` | `UseCloudClusterCommand` | | `CancelWarmUpJobCommand` | global `ADMIN` | `CloudWarmUpJob` records no owner, so a job cannot be scoped to the user who created it | | `DropStageCommand` | global `ADMIN` | `CreateStageCommand`, which already checked it | Note that `ADMIN_PRIV` satisfies every predicate used above, so admin users are unaffected by any of this. #### Where the check is placed, and why - `AddConstraintCommand`: for a foreign key the referenced table is checked as well, because adding the constraint registers a reverse reference on it. - `DropConstraintCommand`: the check sits after the two table-resolution paths converge. Resolution can fall back to a name-only lookup, and putting the check on the normal path only would let the fallback skip it. - `DropConstraintCommand`, primary keys: dropping one cascades into `ConstraintManager.cascadeDropForeignKeys()`, which deletes the foreign key constraint of every referencing table, so `ALTER` is required on each of those too. The cascade is atomic, so all of them are checked before `dropConstraint()`. - `CreateDictionaryCommand`: the check has to run after `validateAndSet()`, since that is what fills in the default catalog/db names. - `WarmUpClusterCommand`, `WITH TABLE`: the per-table `SELECT` check is inside the existing resolution loop, before the db/table lookup. It authorizes the *internal* fully qualified name, not the catalog written in the SQL, because the lookup and the `(db, table, partition)` triple stored for the job are internal-catalog only. - `WarmUpClusterCommand`, `ON TABLES`: this mode matches tables by glob over the whole internal catalog, and `CacheHotspotManager.refreshAllTableFilters()` keeps re-matching in the background with no identity available to re-authorize new matches. There is no fixed table set to authorize, so it requires global `ADMIN` on top of the compute group `USAGE`. #### Incidental changes reviewers should look at These are not privilege checks, but they fall out of adding them: 1. `AdminRotateTdeRootKeyCommand`, `DropCatalogRecycleBinCommand`, `DropStageCommand` had no `validate()` method at all. One was added to each and is called at the top of `run()`. 2. `CreateDictionaryCommand.run()` and `DropDictionaryCommand.run()` now declare `throws Exception`. 3. `CreateDictionaryCommand.run()`: the single try block that wrapped `validateAndSet()` + `createDictionary()` is split in two, with the check in between. Consequence: an access-denied error is **not** wrapped in the `"Failed to create dictionary: ..."` prefix, while the failure messages from `validateAndSet()` and `createDictionary()` are unchanged. 4. `WarmUpClusterCommand.validate()`: order is now cloud-mode → compute group `USAGE` (+ `ADMIN` for `ON TABLES`) → compute group existence/virtual-group validation → table resolution. The non-cloud error message is unchanged, but in cloud mode a user without `USAGE` now gets an access-denied error where they previously got "compute group doesn't exist". 5. `WarmUpClusterCommand`, `WITH TABLE`: the `SELECT` check runs before the db/table lookup, so a user without `SELECT` gets an access-denied error where they previously got "unknown database/table". This is deliberate: the error should not tell a user who cannot read the table whether it exists. 6. `DropConstraintCommand`: the two `ALTER` checks are factored into a private `checkAlterPriv()`, same shape as the one in `AddConstraintCommand`. #### Open questions - `WarmUpClusterCommand`: for `WITH TABLE`, global `ADMIN` felt too coarse for an operation scoped to compute groups the user already has `USAGE` on, so it uses `checkCloudPriv(..., ResourceTypeEnum.CLUSTER)` plus per-table `SELECT`. Happy to switch it back to `ADMIN` if the cloud maintainers prefer that. `ON TABLES` does require `ADMIN`, since a pattern job cannot be authorized per table. - `DropConstraintCommand`: requiring `ALTER` on the referencing tables means a user who owns the primary key table can no longer drop it once somebody else's table references it. Rejecting the cascade and asking for the foreign keys to be dropped separately would be the other option; happy to switch. - `CancelWarmUpJobCommand` keeps `ADMIN` only because there is nothing to scope it to. If `CloudWarmUpJob` recorded the submitting user, letting that user cancel their own job would be better. - Out of scope, but noted while looking around: `ShowWarmUpCommand` (`SHOW WARM UP JOB`) and `ShowDictionariesCommand` (`SHOW DICTIONARIES`) have no privilege check either. Happy to follow up in a separate PR. ### Release note Fix missing privilege checks on `ADMIN SET ENCRYPTION ROOT KEY`, `ADMIN ROTATE TDE ROOT KEY`, `DROP CATALOG RECYCLE BIN`, `CREATE/DROP DICTIONARY`, `ALTER TABLE ADD/DROP CONSTRAINT`, `WARM UP CLUSTER`, `CANCEL WARM UP JOB` and `DROP STAGE`. These statements previously ran for any authenticated user. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [x] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> New `auth_call` cases for constraint, dictionary and recycle bin, each covering both the denied and the granted path, including the cross-table primary key / foreign key cascade. The cloud-only commands (`WARM UP CLUSTER`, `CANCEL WARM UP JOB`, `DROP STAGE`) are not covered by a regression case. - Behavior changed: - [ ] No. - [x] Yes. Users without the privileges listed above can no longer run these statements; they now get an access-denied error. See "Incidental changes reviewers should look at" above for the error-message and ordering changes that come with it. - Does this need documentation? - [ ] No. - [x] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> The privilege documentation for these statements should list the required privileges.
8 tasks
mrhhsg
added a commit
to mrhhsg/doris
that referenced
this pull request
Aug 31, 2026
### 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`. 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 `SELECT` on the source table up front. These are the privileges the internal `INSERT INTO ... SELECT` already required, so nobody loses the ability to refresh; 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. 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), and both (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 and `SELECT` on the source table, 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
mrhhsg
added a commit
to mrhhsg/doris
that referenced
this pull request
Aug 31, 2026
### 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`, the same contract the generated `INSERT` enforces through `CheckPrivileges`, so column-level grants keep working). 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. - `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. - [ ] 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. - 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
added a commit
to mrhhsg/doris
that referenced
this pull request
Sep 1, 2026
### 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
added a commit
to mrhhsg/doris
that referenced
this pull request
Sep 1, 2026
### 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. The check runs once at bind time in `ExpressionAnalyzer`, before the dictionary is resolved (no existence probing) and only outside persisted view bodies: a view is authorized as a whole by `CheckPrivileges.visitLogicalView`, so a user holding `SELECT` on the view alone keeps reading through it, while direct calls, alias function bodies and `CREATE/ALTER VIEW` bodies are checked against the caller. - 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. Both marks are set on the `StatementContext` at bind time, so a read that constant folding later turns into a literal still counts. Dictionaries are a cache already, so the lost sql-cache hit is not a real cost. - `REFRESH DICTIONARY` authorizes the source columns against the resolved source table (catalog, database and table names as the load scan reports them), not the spelling persisted by `CREATE DICTIONARY`, which can differ under `lower_case_table_names`. - `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, a server prepared point query whose second EXECUTE is denied after the revoke (with a non-constant and, under `enable_fold_constant_by_be`, a literal-only dictionary read), view-only reads through views built on `dict_get()` / `dict_get_many()` with direct calls as negative controls, and a global alias function wrapping `dict_get()`. - [ ] 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. Expressions bound in the executing user's statement that call `dict_get()` — row policies, generated columns, sync materialized view definitions, alias function bodies — now require that user to hold `SELECT` on the dictionary, like any other read of it. - 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
added a commit
to mrhhsg/doris
that referenced
this pull request
Sep 1, 2026
### 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
The Nereids path in
StmtExecutordispatches straight toCommand.run(), so each command has to enforce its own privileges. A number of commands never got that check, and can currently be executed by any authenticated user regardless of their grants.This PR adds the missing checks:
AdminSetEncryptionRootKeyCommand,AdminRotateTdeRootKeyCommandADMINAdminSetFrontendConfig,AdminSetTableStatus,AdminSetReplicaStatus,AdminCleanTrashDropCatalogRecycleBinCommandADMINSHOW CATALOG RECYCLE BINalready requires globalADMIN.RECOVERusesALTER_CREATE, but it is name-scoped while erasing takes a raw object id, so it cannot be authorized at db/table levelCreateDictionaryCommandCREATEon the dictionary andSELECTon the source tableCREATE TABLE/CREATE MTMV. TheSELECTcheck is needed because the load task runs internally, and unlike an MTMV the source table is not bound by the plannerDropDictionaryCommandDROPon the dictionaryDROP TABLE/DROP MTMVAddConstraintCommand,DropConstraintCommandALTERon the table. For a foreign key, also on the referenced table; for dropping a primary key, also on every referencing tableALTER TABLEWarmUpClusterCommandUSAGEon the source and destination compute groups, plusSELECTon each table named byWITH TABLE.ON TABLESadditionally requires globalADMINUseCloudClusterCommandCancelWarmUpJobCommandADMINCloudWarmUpJobrecords no owner, so a job cannot be scoped to the user who created itDropStageCommandADMINCreateStageCommand, which already checked itNote that
ADMIN_PRIVsatisfies every predicate used above, so admin users are unaffected by any of this.Where the check is placed, and why
AddConstraintCommand: for a foreign key the referenced table is checked as well, because adding the constraint registers a reverse reference on it.DropConstraintCommand: the check sits after the two table-resolution paths converge. Resolution can fall back to a name-only lookup, and putting the check on the normal path only would let the fallback skip it.DropConstraintCommand, primary keys: dropping one cascades intoConstraintManager.cascadeDropForeignKeys(), which deletes the foreign key constraint of every referencing table, soALTERis required on each of those too. The cascade is atomic, so all of them are checked beforedropConstraint().CreateDictionaryCommand: the check has to run aftervalidateAndSet(), since that is what fills in the default catalog/db names.WarmUpClusterCommand,WITH TABLE: the per-tableSELECTcheck is inside the existing resolution loop, before the db/table lookup. It authorizes the internal fully qualified name, not the catalog written in the SQL, because the lookup and the(db, table, partition)triple stored for the job are internal-catalog only.WarmUpClusterCommand,ON TABLES: this mode matches tables by glob over the whole internal catalog, andCacheHotspotManager.refreshAllTableFilters()keeps re-matching in the background with no identity available to re-authorize new matches. There is no fixed table set to authorize, so it requires globalADMINon top of the compute groupUSAGE.Incidental changes reviewers should look at
These are not privilege checks, but they fall out of adding them:
AdminRotateTdeRootKeyCommand,DropCatalogRecycleBinCommand,DropStageCommandhad novalidate()method at all. One was added to each and is called at the top ofrun().CreateDictionaryCommand.run()andDropDictionaryCommand.run()now declarethrows Exception.CreateDictionaryCommand.run(): the single try block that wrappedvalidateAndSet()+createDictionary()is split in two, with the check in between. Consequence: an access-denied error is not wrapped in the"Failed to create dictionary: ..."prefix, while the failure messages fromvalidateAndSet()andcreateDictionary()are unchanged.WarmUpClusterCommand.validate(): order is now cloud-mode → compute groupUSAGE(+ADMINforON TABLES) → compute group existence/virtual-group validation → table resolution. The non-cloud error message is unchanged, but in cloud mode a user withoutUSAGEnow gets an access-denied error where they previously got "compute group doesn't exist".WarmUpClusterCommand,WITH TABLE: theSELECTcheck runs before the db/table lookup, so a user withoutSELECTgets an access-denied error where they previously got "unknown database/table". This is deliberate: the error should not tell a user who cannot read the table whether it exists.DropConstraintCommand: the twoALTERchecks are factored into a privatecheckAlterPriv(), same shape as the one inAddConstraintCommand.Open questions
WarmUpClusterCommand: forWITH TABLE, globalADMINfelt too coarse for an operation scoped to compute groups the user already hasUSAGEon, so it usescheckCloudPriv(..., ResourceTypeEnum.CLUSTER)plus per-tableSELECT. Happy to switch it back toADMINif the cloud maintainers prefer that.ON TABLESdoes requireADMIN, since a pattern job cannot be authorized per table.DropConstraintCommand: requiringALTERon the referencing tables means a user who owns the primary key table can no longer drop it once somebody else's table references it. Rejecting the cascade and asking for the foreign keys to be dropped separately would be the other option; happy to switch.CancelWarmUpJobCommandkeepsADMINonly because there is nothing to scope it to. IfCloudWarmUpJobrecorded the submitting user, letting that user cancel their own job would be better.ShowWarmUpCommand(SHOW WARM UP JOB) andShowDictionariesCommand(SHOW DICTIONARIES) have no privilege check either. Happy to follow up in a separate PR.Release note
Fix missing privilege checks on
ADMIN SET ENCRYPTION ROOT KEY,ADMIN ROTATE TDE ROOT KEY,DROP CATALOG RECYCLE BIN,CREATE/DROP DICTIONARY,ALTER TABLE ADD/DROP CONSTRAINT,WARM UP CLUSTER,CANCEL WARM UP JOBandDROP STAGE. These statements previously ran for any authenticated user.Check List (For Author)
New
auth_callcases for constraint, dictionary and recycle bin, each covering both the denied and the granted path, including the cross-table primary key / foreign key cascade. The cloud-only commands (WARM UP CLUSTER,CANCEL WARM UP JOB,DROP STAGE) are not covered by a regression case.Behavior changed:
Does this need documentation?
The privilege documentation for these statements should list the required privileges.