github-actions[bot] commented on code in PR #66218:
URL: https://github.com/apache/doris/pull/66218#discussion_r3711175322
##########
fe/fe-core/src/test/java/org/apache/doris/cloud/WarmUpClusterOnTablesParseTest.java:
##########
@@ -66,6 +67,11 @@ public static void init() throws Exception {
originalSystemInfo = getField(env, Env.class, "systemInfo");
connectContext = new ConnectContext();
connectContext.setEnv(env);
+ // this test covers ON TABLES parsing and validation, not
authorization, so give the
+ // context an identity and let it bypass the privilege checks in
validate()
+ connectContext.setCurrentUserIdentity(UserIdentity.ROOT);
+ connectContext.setNoAuth(true);
+ connectContext.setSkipAuth(true);
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AdminRotateTdeRootKeyCommand.java:
##########
@@ -52,6 +56,7 @@ public AdminRotateTdeRootKeyCommand(Map<String, String>
properties) {
@Override
public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
+ validate();
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java:
##########
@@ -126,6 +126,24 @@ public void addConstraint(TableNameInfo tableNameInfo,
String constraintName,
}
}
+ /**
+ * Snapshot the tables whose foreign keys would be cascade-dropped along
with the given primary
+ * key constraint. Taken under the read lock: {@link
PrimaryKeyConstraint#getForeignTableInfos()}
+ * is only a view over a list that {@link #addConstraint} mutates under
the write lock, so callers
+ * outside the lock must not iterate it directly. Returns an empty list
for other constraint types.
+ */
+ public List<TableNameInfo> getCascadeDropTables(Constraint constraint) {
+ if (!(constraint instanceof PrimaryKeyConstraint)) {
+ return ImmutableList.of();
+ }
+ readLock();
+ try {
+ return ImmutableList.copyOf(((PrimaryKeyConstraint)
constraint).getForeignTableInfos());
Review 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.
##########
regression-test/suites/auth_call/test_ddl_constraint_auth.groovy:
##########
@@ -0,0 +1,129 @@
+// 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. Note this goes through the
normal resolution path:
+ // the name-based fallback in DropConstraintCommand only triggers when
resolution throws (e.g. an
+ // external table removed out of band), which is not reproducible from a
suite, so the check on
+ // that branch is covered by inspection only.
+ connect(user, "${pwd}", context.config.jdbcUrl) {
+ sql """use ${dbName}"""
+ test {
+ sql """ALTER TABLE ${tableName} DROP CONSTRAINT
${constraintName}"""
+ exception "denied"
+ }
+ }
+ def constraints = sql """SHOW CONSTRAINTS FROM ${dbName}.${tableName}"""
+ assertTrue(constraints.size() == 1)
+
+ sql """grant ALTER_PRIV on ${dbName}.${tableName} to ${user}"""
+ connect(user, "${pwd}", context.config.jdbcUrl) {
+ sql """use ${dbName}"""
+ sql """ALTER TABLE ${tableName} DROP CONSTRAINT ${constraintName}"""
+ sql """ALTER TABLE ${tableName} ADD CONSTRAINT ${constraintName}
UNIQUE (id)"""
+ }
+ constraints = sql """SHOW CONSTRAINTS FROM ${dbName}.${tableName}"""
+ assertTrue(constraints.size() == 1)
+
+ // dropping a primary key cascades into the foreign keys of every
referencing table, so ALTER on
+ // the referencing tables is required as well
+ String pkTable = 'test_ddl_constraint_auth_pk_tb'
+ String fkTable = 'test_ddl_constraint_auth_fk_tb'
+ String pkName = 'test_ddl_constraint_auth_pk'
+ String fkName = 'test_ddl_constraint_auth_fk'
+ sql """
+ CREATE TABLE IF NOT EXISTS ${dbName}.${pkTable} (
+ id BIGINT NOT NULL,
+ username VARCHAR(30)
+ )
+ DISTRIBUTED BY HASH(id) BUCKETS 2
+ PROPERTIES ("replication_num" = "1");
+ """
+ sql """
+ CREATE TABLE IF NOT EXISTS ${dbName}.${fkTable} (
+ id BIGINT NOT NULL,
+ pk_id BIGINT NOT NULL
+ )
+ DISTRIBUTED BY HASH(id) BUCKETS 2
+ 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)"""
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropStageCommand.java:
##########
@@ -45,9 +49,20 @@ public <R, C> R accept(PlanVisitor<R, C> visitor, C context)
{
@Override
public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
+ validate();
Review 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.
--
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]
