[fix](auth)Disable column auth temporarily (#23295)

- add config `enable_col_auth` to temporarily disable column permissions(because old/new planner has bug when select from view) - Restore the old optimizer to the previous authentication method - Support for new optimizer authentication（Legacy issue: When querying the view, the permissions of the base table will be authenticated. The view's own permissions should be authenticated and processed after the new optimizer is improved） - fix: show grants for non-existent users - fix: role:`admin` can not grant/revoke to/from user
apache · Aug 25, 2023 · 945a8ec · 945a8ec
1 parent bc514ef
commit 945a8ec
Show file tree

Hide file tree

Showing 13 changed files with 39 additions and 84 deletions.
diff --git a/fe/fe-common/src/main/java/org/apache/doris/common/Config.java b/fe/fe-common/src/main/java/org/apache/doris/common/Config.java
@@ -2093,6 +2093,11 @@ public class Config extends ConfigBase {
             "Whether to use mysql's bigint type to return Doris's largeint type"})
     public static boolean use_mysql_bigint_for_largeint = false;
 
+    @ConfField(description = {
+            "是否开启列权限",
+            "Whether to enable col auth"})
+    public static boolean enable_col_auth = false;
+
     @ConfField
     public static boolean forbid_running_alter_job = false;
 }
diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/GrantStmt.java
@@ -21,6 +21,7 @@
 import org.apache.doris.catalog.Env;
 import org.apache.doris.cluster.ClusterNamespace;
 import org.apache.doris.common.AnalysisException;
+import org.apache.doris.common.Config;
 import org.apache.doris.common.ErrorCode;
 import org.apache.doris.common.ErrorReport;
 import org.apache.doris.common.FeNameFormat;
@@ -149,7 +150,7 @@ public void analyze(Analyzer analyzer) throws UserException {
         } else if (roles != null) {
             for (int i = 0; i < roles.size(); i++) {
                 String originalRoleName = roles.get(i);
-                FeNameFormat.checkRoleName(originalRoleName, false /* can not be admin */, "Can not grant role");
+                FeNameFormat.checkRoleName(originalRoleName, true /* can be admin */, "Can not grant role");
                 roles.set(i, ClusterNamespace.getFullName(analyzer.getClusterName(), originalRoleName));
             }
         }
@@ -181,7 +182,8 @@ public void analyze(Analyzer analyzer) throws UserException {
     public static void checkAccessPrivileges(
             List<AccessPrivilegeWithCols> accessPrivileges) throws AnalysisException {
         for (AccessPrivilegeWithCols access : accessPrivileges) {
-            if (!access.getAccessPrivilege().canHasColPriv() && !CollectionUtils.isEmpty(access.getCols())) {
+            if ((!access.getAccessPrivilege().canHasColPriv() || !Config.enable_col_auth) && !CollectionUtils
+                    .isEmpty(access.getCols())) {
                 throw new AnalysisException(
                         String.format("%s do not support col auth.", access.getAccessPrivilege().name()));
             }

diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/RevokeStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/RevokeStmt.java
@@ -133,7 +133,7 @@ public void analyze(Analyzer analyzer) throws AnalysisException {
         } else if (roles != null) {
             for (int i = 0; i < roles.size(); i++) {
                 String originalRoleName = roles.get(i);
-                FeNameFormat.checkRoleName(originalRoleName, false /* can not be admin */, "Can not revoke role");
+                FeNameFormat.checkRoleName(originalRoleName, true /* can be admin */, "Can not revoke role");
                 roles.set(i, ClusterNamespace.getFullName(analyzer.getClusterName(), originalRoleName));
             }
         }

diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java
@@ -24,6 +24,7 @@
 import org.apache.doris.catalog.AggregateFunction;
 import org.apache.doris.catalog.Column;
 import org.apache.doris.catalog.DatabaseIf;
+import org.apache.doris.catalog.Env;
 import org.apache.doris.catalog.FunctionSet;
 import org.apache.doris.catalog.OlapTable;
 import org.apache.doris.catalog.PrimitiveType;
@@ -44,6 +45,7 @@
 import org.apache.doris.common.UserException;
 import org.apache.doris.common.util.SqlUtils;
 import org.apache.doris.common.util.ToSqlContext;
+import org.apache.doris.mysql.privilege.PrivPredicate;
 import org.apache.doris.qe.ConnectContext;
 import org.apache.doris.rewrite.ExprRewriter;
 import org.apache.doris.rewrite.mvrewrite.MVSelectFailedException;
@@ -392,6 +394,13 @@ public void getTables(Analyzer analyzer, boolean expandView, Map<Long, TableIf>
                     View view = (View) table;
                     view.getQueryStmt().getTables(analyzer, expandView, tableMap, parentViewNameSet);
                 } else {
+                    // check auth
+                    if (!Env.getCurrentEnv().getAccessManager()
+                            .checkTblPriv(ConnectContext.get(), tblRef.getName(), PrivPredicate.SELECT)) {
+                        ErrorReport.reportAnalysisException(ErrorCode.ERR_TABLEACCESS_DENIED_ERROR, "SELECT",
+                                ConnectContext.get().getQualifiedUser(), ConnectContext.get().getRemoteIP(),
+                                dbName + "." + tableName);
+                    }
                     tableMap.put(table.getId(), table);
                 }
             }

diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowGrantsStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/ShowGrantsStmt.java
@@ -89,6 +89,9 @@ public void analyze(Analyzer analyzer) throws AnalysisException {
                 ErrorReport.reportAnalysisException(ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR, "GRANT");
             }
         }
+        if (userIdent != null && !Env.getCurrentEnv().getAccessManager().getAuth().doesUserExist(userIdent)) {
+            throw new AnalysisException(String.format("User: %s does not exist", userIdent));
+        }
     }
 
     @Override

diff --git a/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java b/fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java
@@ -663,7 +663,7 @@ private void grantInternal(UserIdentity userIdent, List<String> roles, boolean i
 
 
     // return true if user ident exist
-    private boolean doesUserExist(UserIdentity userIdent) {
+    public boolean doesUserExist(UserIdentity userIdent) {
         return userManager.userIdentityExist(userIdent, false);
     }
 

diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/UserAuthentication.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/UserAuthentication.java
@@ -73,4 +73,5 @@ private Plan checkPermission(CatalogRelation relation, ConnectContext connectCon
         }
         return null;
     }
+
 }
diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/OriginalPlanner.java b/fe/fe-core/src/main/java/org/apache/doris/planner/OriginalPlanner.java
@@ -31,23 +31,15 @@
 import org.apache.doris.analysis.SlotRef;
 import org.apache.doris.analysis.StatementBase;
 import org.apache.doris.analysis.StorageBackend;
-import org.apache.doris.analysis.TableName;
 import org.apache.doris.analysis.TupleDescriptor;
 import org.apache.doris.catalog.Column;
 import org.apache.doris.catalog.Env;
 import org.apache.doris.catalog.OlapTable;
 import org.apache.doris.catalog.PrimitiveType;
 import org.apache.doris.catalog.ScalarType;
-import org.apache.doris.catalog.Table;
-import org.apache.doris.catalog.TableIf;
 import org.apache.doris.catalog.Type;
-import org.apache.doris.catalog.external.ExternalTable;
-import org.apache.doris.cluster.ClusterNamespace;
-import org.apache.doris.common.AnalysisException;
 import org.apache.doris.common.Config;
 import org.apache.doris.common.UserException;
-import org.apache.doris.datasource.InternalCatalog;
-import org.apache.doris.mysql.privilege.PrivPredicate;
 import org.apache.doris.qe.ConnectContext;
 import org.apache.doris.rewrite.mvrewrite.MVSelectFailedException;
 import org.apache.doris.statistics.query.StatsDelta;
@@ -56,10 +48,7 @@
 import org.apache.doris.thrift.TRuntimeFilterMode;
 
 import com.google.common.base.Preconditions;
-import com.google.common.base.Strings;
-import com.google.common.collect.HashMultimap;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
@@ -173,8 +162,6 @@ public void createPlanFragments(StatementBase statement, Analyzer analyzer, TQue
             insertStmt.prepareExpressions();
         }
 
-        checkColumnPrivileges(singleNodePlan);
-
         // TODO chenhao16 , no used materialization work
         // compute referenced slots before calling computeMemLayout()
         //analyzer.markRefdSlots(analyzer, singleNodePlan, resultExprs, null);
@@ -313,66 +300,6 @@ public void createPlanFragments(StatementBase statement, Analyzer analyzer, TQue
         }
     }
 
-    private void checkColumnPrivileges(PlanNode singleNodePlan) throws UserException {
-        if (ConnectContext.get() == null) {
-            return;
-        }
-        // 1. collect all columns from all scan nodes
-        List<ScanNode> scanNodes = Lists.newArrayList();
-        singleNodePlan.collect((PlanNode planNode) -> planNode instanceof ScanNode, scanNodes);
-        // catalog : <db.table : column>
-        Map<String, HashMultimap<TableName, String>> ctlToTableColumnMap = Maps.newHashMap();
-        for (ScanNode scanNode : scanNodes) {
-            if (!scanNode.needToCheckColumnPriv()) {
-                continue;
-            }
-            TupleDescriptor tupleDesc = scanNode.getTupleDesc();
-            TableIf table = tupleDesc.getTable();
-            if (table == null) {
-                continue;
-            }
-            TableName tableName = getFullQualifiedTableNameFromTable(table);
-            for (SlotDescriptor slotDesc : tupleDesc.getSlots()) {
-                if (!slotDesc.isMaterialized()) {
-                    continue;
-                }
-                Column column = slotDesc.getColumn();
-                if (column == null) {
-                    continue;
-                }
-                HashMultimap<TableName, String> tableColumnMap = ctlToTableColumnMap.get(tableName.getCtl());
-                if (tableColumnMap == null) {
-                    tableColumnMap = HashMultimap.create();
-                    ctlToTableColumnMap.put(tableName.getCtl(), tableColumnMap);
-                }
-                tableColumnMap.put(tableName, column.getName());
-                LOG.debug("collect column {} in {}", column.getName(), tableName);
-            }
-        }
-        // 2. check privs
-        // TODO: only support SELECT_PRIV now
-        PrivPredicate wanted = PrivPredicate.SELECT;
-        for (Map.Entry<String, HashMultimap<TableName, String>> entry : ctlToTableColumnMap.entrySet()) {
-            Env.getCurrentEnv().getAccessManager().checkColumnsPriv(ConnectContext.get().getCurrentUserIdentity(),
-                    entry.getKey(), entry.getValue(), wanted);
-        }
-    }
-
-    private TableName getFullQualifiedTableNameFromTable(TableIf table) throws AnalysisException {
-        if (table instanceof Table) {
-            String dbName = ClusterNamespace.getNameFromFullName(((Table) table).getQualifiedDbName());
-            if (Strings.isNullOrEmpty(dbName)) {
-                throw new AnalysisException("failed to get db name from table " + table.getName());
-            }
-            return new TableName(InternalCatalog.INTERNAL_CATALOG_NAME, dbName, table.getName());
-        } else if (table instanceof ExternalTable) {
-            ExternalTable extTable = (ExternalTable) table;
-            return new TableName(extTable.getCatalog().getName(), extTable.getDbName(), extTable.getName());
-        } else {
-            throw new AnalysisException("table " + table.getName() + " is not internal or external table instance");
-        }
-    }
-
     /**
      * If there are unassigned conjuncts, returns a SelectNode on top of root that evaluate those conjuncts; otherwise
      * returns root unchanged.

diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/ColumnPrivTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/ColumnPrivTest.java
@@ -48,12 +48,15 @@
 import com.google.common.collect.Maps;
 import org.junit.Assert;
 import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+// when `select` suppport `col auth`,will open ColumnPrivTest
+@Disabled
 public class ColumnPrivTest extends TestWithFeService {
     private static Auth auth;
     private static Env env;

diff --git a/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java b/fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/AuthTest.java
@@ -34,6 +34,7 @@
 import org.apache.doris.catalog.DomainResolver;
 import org.apache.doris.catalog.Env;
 import org.apache.doris.common.AnalysisException;
+import org.apache.doris.common.Config;
 import org.apache.doris.common.DdlException;
 import org.apache.doris.common.ExceptionChecker;
 import org.apache.doris.common.UserException;
@@ -153,6 +154,7 @@ public void setUp() throws NoSuchMethodException, SecurityException {
         };
 
         resolver = new MockDomainResolver(auth);
+        Config.enable_col_auth = true;
     }
 
     @Test

diff --git a/regression-test/data/account_p0/test_account.out b/regression-test/data/account_p0/test_account.out
diff --git a/regression-test/suites/account_p0/test_account.groovy b/regression-test/suites/account_p0/test_account.groovy
@@ -18,5 +18,11 @@ suite("test_account") {
     // todo: test account management, such as role, user, grant, revoke ...
     sql "show roles"
 
-    qt_show_grants1 "show grants for 'non_existent_user_1'"
+    try {
+        sql "show grants for 'non_existent_user_1'"
+        fail()
+    } catch (Exception e) {
+        log.info(e.getMessage())
+        assertTrue(e.getMessage().contains('not exist'))
+    }
 }
diff --git a/regression-test/suites/account_p0/test_nereids_authentication.groovy b/regression-test/suites/account_p0/test_nereids_authentication.groovy
@@ -28,6 +28,7 @@ suite("test_nereids_authentication", "query") {
     }
 
     sql "set enable_nereids_planner = true"
+    sql "set enable_fallback_to_original_planner = false"
 
     def dbName = "nereids_authentication"
     sql "DROP DATABASE IF EXISTS ${dbName}"
@@ -57,7 +58,7 @@ suite("test_nereids_authentication", "query") {
             fail()
         } catch (Exception e) {
             log.info(e.getMessage())
-            assertTrue(e.getMessage().contains('Permission denied'))
+            assertTrue(e.getMessage().contains('denied to user'))
         }
     }
 
@@ -67,7 +68,7 @@ suite("test_nereids_authentication", "query") {
             fail()
         } catch (Exception e) {
             log.info(e.getMessage())
-            assertTrue(e.getMessage().contains('Permission denied'))
+            assertTrue(e.getMessage().contains('denied to user'))
         }
     }