924060929 commented on code in PR #66218:
URL: https://github.com/apache/doris/pull/66218#discussion_r3727551397
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CancelWarmUpJobCommand.java:
##########
@@ -59,6 +63,11 @@ public long getJobId() {
* @throws AnalysisException check whether this sql is legal
*/
public void validate(ConnectContext ctx) throws AnalysisException {
+ // check auth
+ if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ctx,
PrivPredicate.ADMIN)) {
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AdminSetEncryptionRootKeyCommand.java:
##########
@@ -63,6 +67,11 @@ public void run(ConnectContext ctx, StmtExecutor executor)
throws Exception {
* validate
*/
public void validate() throws AnalysisException {
+ // check auth
+ if
(!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ConnectContext.get(),
PrivPredicate.ADMIN)) {
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AdminRotateTdeRootKeyCommand.java:
##########
@@ -60,6 +65,16 @@ public void run(ConnectContext ctx, StmtExecutor executor)
throws Exception {
}
}
+ /**
+ * validate
+ */
+ public void validate() throws AnalysisException {
+ // check auth
+ if
(!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ConnectContext.get(),
PrivPredicate.ADMIN)) {
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropDictionaryCommand.java:
##########
@@ -57,10 +62,15 @@ public <R, C> R accept(PlanVisitor<R, C> visitor, C
context) {
}
@Override
- public void run(ConnectContext ctx, StmtExecutor executor) {
+ public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
if (dbName == null) { // use current database
dbName = ctx.getDatabase();
}
+ // check auth. dictionaries always live in the internal catalog.
+ if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx,
InternalCatalog.INTERNAL_CATALOG_NAME,
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
