github-actions[bot] commented on code in PR #66218: URL: https://github.com/apache/doris/pull/66218#discussion_r3679832494
########## regression-test/suites/auth_call/test_ddl_constraint_auth.groovy: ########## @@ -0,0 +1,127 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_ddl_constraint_auth", "p0,auth_call") { + String user = 'test_ddl_constraint_auth_user' + String pwd = 'C123_567p' + String dbName = 'test_ddl_constraint_auth_db' + String tableName = 'test_ddl_constraint_auth_tb' + String constraintName = 'test_ddl_constraint_auth_uk' + + try_sql("DROP USER ${user}") + try_sql """drop database if exists ${dbName}""" + sql """CREATE USER '${user}' IDENTIFIED BY '${pwd}'""" + sql """grant select_priv on regression_test to ${user}""" + //cloud-mode + if (isCloudMode()) { + def clusters = sql " SHOW CLUSTERS; " + assertTrue(!clusters.isEmpty()) + def validCluster = clusters[0][0] + sql """GRANT USAGE_PRIV ON CLUSTER `${validCluster}` TO ${user}"""; + } + + sql """create database ${dbName}""" + sql """ + CREATE TABLE IF NOT EXISTS ${dbName}.${tableName} ( + id BIGINT, + username VARCHAR(30) + ) + DISTRIBUTED BY HASH(id) BUCKETS 2 + PROPERTIES ("replication_num" = "1"); + """ + + // SELECT alone must not be enough to change constraints + sql """grant SELECT_PRIV on ${dbName}.${tableName} to ${user}""" + connect(user, "${pwd}", context.config.jdbcUrl) { + sql """use ${dbName}""" + test { + sql """ALTER TABLE ${tableName} ADD CONSTRAINT ${constraintName} UNIQUE (id)""" + exception "denied" + } + } + + 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 Review 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. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropConstraintCommand.java: ########## @@ -75,17 +79,38 @@ 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. + checkAlterPriv(ctx, tableNameInfo); Constraint constraint = Env.getCurrentEnv().getConstraintManager().getConstraint(tableNameInfo, name); if (constraint == null) { throw new AnalysisException( String.format("Unknown constraint %s on table %s.", name, tableNameInfo)); } + if (constraint instanceof PrimaryKeyConstraint) { + // 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()) { Review 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. ########## 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: 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. -- 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]
