[fix](auth) Add missing privilege checks for several Nereids commands - #66218
Open
CalvinKirs wants to merge 3 commits into
Open
[fix](auth) Add missing privilege checks for several Nereids commands#66218CalvinKirs wants to merge 3 commits into
CalvinKirs wants to merge 3 commits into
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 |
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
3 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?
Issue Number: close #xxx
Related PR: #xxx
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 tableALTER TABLEWarmUpClusterCommandUSAGEon the source and destination compute groups, plusSELECTon each table named byWITH TABLEUseCloudClusterCommandCancelWarmUpJobCommandADMINCloudWarmUpJobrecords 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.CreateDictionaryCommand: the check has to run aftervalidateAndSet(), since that is what fills in the default catalog/db names.WarmUpClusterCommand: the per-tableSELECTcheck is inside the existingWITH TABLEresolution loop, so it reuses the table that was just resolved.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→ 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".Open questions
WarmUpClusterCommand: globalADMINfelt too coarse for an operation scoped to compute groups the user already hasUSAGEon, so it usescheckCloudPriv(..., ResourceTypeEnum.CLUSTER)instead. Happy to switch it back toADMINif the cloud maintainers prefer that.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. 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.