github-actions[bot] commented on code in PR #63838: URL: https://github.com/apache/doris/pull/63838#discussion_r3339048398
########## fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/CatalogAccessControllerTest.java: ########## @@ -0,0 +1,245 @@ +// 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. + +package org.apache.doris.mysql.privilege; + +import org.apache.doris.analysis.ResourceTypeEnum; +import org.apache.doris.analysis.UserIdentity; +import org.apache.doris.common.AuthorizationException; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import org.junit.Assert; +import org.junit.Test; + +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; + +public class CatalogAccessControllerTest { + + private static class StubAccessController implements CatalogAccessController { + final AtomicBoolean ctlPrivCalled = new AtomicBoolean(false); + final AtomicBoolean dbPrivCalled = new AtomicBoolean(false); + final AtomicBoolean tblPrivCalled = new AtomicBoolean(false); + + private final boolean ctlResult; + private final boolean dbResult; + private final boolean tblResult; + + StubAccessController() { + this(false, false, false); + } + + StubAccessController(boolean ctlResult, boolean dbResult, boolean tblResult) { + this.ctlResult = ctlResult; + this.dbResult = dbResult; + this.tblResult = tblResult; + } + + @Override + public boolean checkGlobalPriv(UserIdentity currentUser, PrivPredicate wanted) { + return false; + } + + @Override + public boolean checkCtlPriv(UserIdentity currentUser, String ctl, PrivPredicate wanted) { + ctlPrivCalled.set(true); + return ctlResult; + } + + @Override + public boolean checkDbPriv(UserIdentity currentUser, String ctl, String db, PrivPredicate wanted) { + dbPrivCalled.set(true); + return dbResult; + } + + @Override + public boolean checkTblPriv(UserIdentity currentUser, String ctl, String db, String tbl, PrivPredicate wanted) { + tblPrivCalled.set(true); + return tblResult; + } + + @Override + public void checkColsPriv(UserIdentity currentUser, String ctl, String db, String tbl, + Set<String> cols, PrivPredicate wanted) throws AuthorizationException { + if (!tblResult) { + throw new AuthorizationException("denied"); + } + } + + @Override + public boolean checkResourcePriv(UserIdentity currentUser, String resourceName, PrivPredicate wanted) { + return false; + } + + @Override + public boolean checkWorkloadGroupPriv(UserIdentity currentUser, String workloadGroupName, + PrivPredicate wanted) { + return false; + } + + @Override + public boolean checkCloudPriv(UserIdentity currentUser, String cloudName, + PrivPredicate wanted, ResourceTypeEnum type) { + return false; + } + + @Override + public boolean checkStorageVaultPriv(UserIdentity currentUser, String storageVaultName, + PrivPredicate wanted) { + return false; + } + + @Override + public Optional<DataMaskPolicy> evalDataMaskPolicy(UserIdentity currentUser, String ctl, String db, + String tbl, String col) { + return Optional.empty(); + } + + @Override + public List<? extends RowFilterPolicy> evalRowFilterPolicies(UserIdentity currentUser, String ctl, + String db, String tbl) { + return ImmutableList.of(); + } + } + + @Test + public void testCheckCtlPrivShortCircuitOnHasGlobal() { + StubAccessController controller = new StubAccessController(); + UserIdentity user = UserIdentity.createAnalyzedUserIdentWithIp("test_user", "%"); + + boolean result = controller.checkCtlPriv(true, user, "ctl", PrivPredicate.SELECT); + + Assert.assertTrue(result); + Assert.assertFalse(controller.ctlPrivCalled.get()); + } + + @Test + public void testCheckCtlPrivFallsThroughWithoutHasGlobal() { + StubAccessController controller = new StubAccessController(true, false, false); + UserIdentity user = UserIdentity.createAnalyzedUserIdentWithIp("test_user", "%"); + + boolean result = controller.checkCtlPriv(false, user, "ctl", PrivPredicate.SELECT); + + Assert.assertTrue(result); + Assert.assertTrue(controller.ctlPrivCalled.get()); + } + + @Test + public void testCheckCtlPrivFallsThroughAndReturnsFalse() { + StubAccessController controller = new StubAccessController(false, false, false); + UserIdentity user = UserIdentity.createAnalyzedUserIdentWithIp("test_user", "%"); + + boolean result = controller.checkCtlPriv(false, user, "ctl", PrivPredicate.SELECT); + + Assert.assertFalse(result); + Assert.assertTrue(controller.ctlPrivCalled.get()); + } + + @Test + public void testCheckDbPrivShortCircuitOnHasGlobal() { + StubAccessController controller = new StubAccessController(); + UserIdentity user = UserIdentity.createAnalyzedUserIdentWithIp("test_user", "%"); + + boolean result = controller.checkDbPriv(true, user, "ctl", "db", PrivPredicate.SELECT); + + Assert.assertTrue(result); + Assert.assertFalse(controller.dbPrivCalled.get()); + } + + @Test + public void testCheckDbPrivFallsThroughWithoutHasGlobal() { + StubAccessController controller = new StubAccessController(false, true, false); + UserIdentity user = UserIdentity.createAnalyzedUserIdentWithIp("test_user", "%"); + + boolean result = controller.checkDbPriv(false, user, "ctl", "db", PrivPredicate.SELECT); + + Assert.assertTrue(result); + Assert.assertTrue(controller.dbPrivCalled.get()); + } + + @Test + public void testCheckDbPrivFallsThroughAndReturnsFalse() { + StubAccessController controller = new StubAccessController(false, false, false); + UserIdentity user = UserIdentity.createAnalyzedUserIdentWithIp("test_user", "%"); + + boolean result = controller.checkDbPriv(false, user, "ctl", "db", PrivPredicate.SELECT); + + Assert.assertFalse(result); + Assert.assertTrue(controller.dbPrivCalled.get()); + } + + @Test + public void testCheckTblPrivShortCircuitOnHasGlobal() { + StubAccessController controller = new StubAccessController(); + UserIdentity user = UserIdentity.createAnalyzedUserIdentWithIp("test_user", "%"); + + boolean result = controller.checkTblPriv(true, user, "ctl", "db", "tbl", PrivPredicate.SELECT); + + Assert.assertTrue(result); + Assert.assertFalse(controller.tblPrivCalled.get()); + } + + @Test + public void testCheckTblPrivFallsThroughWithoutHasGlobal() { + StubAccessController controller = new StubAccessController(false, false, true); + UserIdentity user = UserIdentity.createAnalyzedUserIdentWithIp("test_user", "%"); + + boolean result = controller.checkTblPriv(false, user, "ctl", "db", "tbl", PrivPredicate.SELECT); + + Assert.assertTrue(result); + Assert.assertTrue(controller.tblPrivCalled.get()); + } + + @Test + public void testCheckTblPrivFallsThroughAndReturnsFalse() { + StubAccessController controller = new StubAccessController(false, false, false); + UserIdentity user = UserIdentity.createAnalyzedUserIdentWithIp("test_user", "%"); + + boolean result = controller.checkTblPriv(false, user, "ctl", "db", "tbl", PrivPredicate.SELECT); + + Assert.assertFalse(result); + Assert.assertTrue(controller.tblPrivCalled.get()); + } + + @Test + public void testCheckColsPrivWithHasGlobalSuppressesException() throws AuthorizationException { Review Comment: This test currently preserves the old column behavior where `checkColsPriv(true, ...)` still calls the lower-level column check and only suppresses `AuthorizationException`. That leaves a functionally parallel path to the optimized catalog/db/table wrappers: `AccessControllerManager.checkColumnsPriv` computes the same `hasGlobal`, then `CatalogAccessController.checkColsPriv(boolean, ...)` still scans column/table privileges in `Auth.checkColsPriv` even though the global privilege already guarantees success. This means SELECT paths that call column privilege checks do not get the intended short-circuit benefit. Please make the column wrapper return immediately when `hasGlobal` is true and update this test to assert that the underlying column check was not called. -- 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]
