github-actions[bot] commented on code in PR #66218:
URL: https://github.com/apache/doris/pull/66218#discussion_r3679441033
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AddConstraintCommand.java:
##########
@@ -71,6 +74,7 @@ public void run(ConnectContext ctx, StmtExecutor executor)
throws Exception {
TableNameInfo tableNameInfo = TableNameInfoUtils.fromCatalogDb(
table.getDatabase().getCatalog(), table.getDatabase(), table);
ImmutableList<String> columns = columnsAndTable.first;
+ checkAlterPriv(ctx, tableNameInfo);
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateDictionaryCommand.java:
##########
@@ -60,12 +65,20 @@ public StmtType stmtType() {
}
@Override
- public void run(ConnectContext ctx, StmtExecutor executor) {
+ public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
try {
// 1. Validate the dictionary info. names and existence.
createDictionaryInfo.validateAndSet(ctx);
+ } catch (Exception e) {
+ LOG.warn("Failed to create dictionary: {}", e.getMessage());
+ throw new AnalysisException("Failed to create dictionary: " +
e.getMessage());
+ }
+
+ // 2. Check auth. Must run after validateAndSet(), which fills in the
default catalog/db names.
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropConstraintCommand.java:
##########
@@ -75,6 +78,14 @@ public void run(ConnectContext ctx, StmtExecutor executor)
throws Exception {
+ "falling back to name-based lookup: {}", name,
e.getMessage());
tableNameInfo = extractTableNameFromPlan(ctx);
}
+ // 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(),
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateDictionaryCommand.java:
##########
@@ -75,4 +88,25 @@ public void run(ConnectContext ctx, StmtExecutor executor) {
throw new AnalysisException("Failed to create dictionary: " +
e.getMessage());
}
}
+
+ /**
+ * A dictionary is created in the internal catalog and its data is loaded
by an internal task,
+ * so require CREATE on the dictionary itself and SELECT on the source
table. Without the latter
+ * the dictionary would expose data the creator can not read.
+ */
+ private void checkAuth(ConnectContext ctx) throws
org.apache.doris.common.AnalysisException {
+ if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx,
InternalCatalog.INTERNAL_CATALOG_NAME,
+ createDictionaryInfo.getDbName(),
createDictionaryInfo.getDictName(), PrivPredicate.CREATE)) {
+
ErrorReport.reportAnalysisException(ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR,
"CREATE");
+ }
+
+ String srcCtl = createDictionaryInfo.getSourceCtlName();
+ String srcDb = createDictionaryInfo.getSourceDbName();
+ String srcTbl = createDictionaryInfo.getSourceTableName();
+ if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ctx, srcCtl,
srcDb, srcTbl,
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/WarmUpClusterCommand.java:
##########
@@ -180,6 +190,13 @@ public void validate(ConnectContext connectContext) throws
UserException {
throw new UserException("The sql is just support in cloud mode");
}
+ // 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);
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/WarmUpClusterCommand.java:
##########
@@ -226,6 +243,12 @@ public void validate(ConnectContext connectContext) throws
UserException {
if (table == null) {
ErrorReport.reportAnalysisException(ErrorCode.ERR_BAD_TABLE_ERROR,
tableNameInfo.getTbl());
}
+ if
(!Env.getCurrentEnv().getAccessManager().checkTblPriv(connectContext,
tableNameInfo.getCtl(),
Review 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.
--
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]
