Merge pull request #65419 from jsc0218/ClearExceptionMsgTableEngineGrant

Clear Hint in Table Engine and Sources

src/Access/Common/AccessRightsElement.cpp

@@ -224,7 +224,11 @@ void AccessRightsElement::replaceEmptyDatabase(const String & current_database)
 
 String AccessRightsElement::toString() const { return toStringImpl(*this, true); }
 String AccessRightsElement::toStringWithoutOptions() const { return toStringImpl(*this, false); }
-
+String AccessRightsElement::toStringForAccessTypeSource() const
+{
+    String result{access_flags.toKeywords().front()};
+    return result + " ON *.*";
+}
 
 bool AccessRightsElements::empty() const { return std::all_of(begin(), end(), [](const AccessRightsElement & e) { return e.empty(); }); }
 

src/Access/Common/AccessRightsElement.h

@@ -89,6 +89,7 @@ struct AccessRightsElement
     /// Returns a human-readable representation like "GRANT SELECT, UPDATE(x, y) ON db.table".
     String toString() const;
     String toStringWithoutOptions() const;
+    String toStringForAccessTypeSource() const;
 };
 
 

src/Access/ContextAccess.cpp

@@ -38,6 +38,24 @@ namespace ErrorCodes
 
 namespace
 {
+    const std::vector<std::tuple<AccessFlags, std::string>> source_and_table_engines = {
+        {AccessType::FILE, "File"},
+        {AccessType::URL, "URL"},
+        {AccessType::REMOTE, "Distributed"},
+        {AccessType::MONGO, "MongoDB"},
+        {AccessType::REDIS, "Redis"},
+        {AccessType::MYSQL, "MySQL"},
+        {AccessType::POSTGRES, "PostgreSQL"},
+        {AccessType::SQLITE, "SQLite"},
+        {AccessType::ODBC, "ODBC"},
+        {AccessType::JDBC, "JDBC"},
+        {AccessType::HDFS, "HDFS"},
+        {AccessType::S3, "S3"},
+        {AccessType::HIVE, "Hive"},
+        {AccessType::AZURE, "AzureBlobStorage"}
+    };
+
+
     AccessRights mixAccessRightsFromUserAndRoles(const User & user, const EnabledRolesInfo & roles_info)
     {
         AccessRights res = user.access;
@@ -206,22 +224,6 @@ namespace
         }
 
         /// There is overlap between AccessType sources and table engines, so the following code avoids user granting twice.
-        static const std::vector<std::tuple<AccessFlags, std::string>> source_and_table_engines = {
-            {AccessType::FILE, "File"},
-            {AccessType::URL, "URL"},
-            {AccessType::REMOTE, "Distributed"},
-            {AccessType::MONGO, "MongoDB"},
-            {AccessType::REDIS, "Redis"},
-            {AccessType::MYSQL, "MySQL"},
-            {AccessType::POSTGRES, "PostgreSQL"},
-            {AccessType::SQLITE, "SQLite"},
-            {AccessType::ODBC, "ODBC"},
-            {AccessType::JDBC, "JDBC"},
-            {AccessType::HDFS, "HDFS"},
-            {AccessType::S3, "S3"},
-            {AccessType::HIVE, "Hive"},
-            {AccessType::AZURE, "AzureBlobStorage"}
-        };
 
         /// Sync SOURCE and TABLE_ENGINE, so only need to check TABLE_ENGINE later.
         if (access_control.doesTableEnginesRequireGrant())
@@ -267,6 +269,11 @@ namespace
 
     template <typename... OtherArgs>
     std::string_view getDatabase(std::string_view arg1, const OtherArgs &...) { return arg1; }
+
+    std::string_view getTableEngine() { return {}; }
+
+    template <typename... OtherArgs>
+    std::string_view getTableEngine(std::string_view arg1, const OtherArgs &...) { return arg1; }
 }
 
 
@@ -620,18 +627,58 @@ bool ContextAccess::checkAccessImplHelper(const ContextPtr & context, AccessFlag
 
     if (!granted)
     {
-        if (grant_option && acs->isGranted(flags, args...))
+        auto access_denied_no_grant = [&]<typename... FmtArgs>(AccessFlags access_flags, FmtArgs && ...fmt_args)
         {
+            if (grant_option && acs->isGranted(access_flags, fmt_args...))
+            {
+                return access_denied(ErrorCodes::ACCESS_DENIED,
+                    "{}: Not enough privileges. "
+                    "The required privileges have been granted, but without grant option. "
+                    "To execute this query, it's necessary to have the grant {} WITH GRANT OPTION",
+                    AccessRightsElement{access_flags, fmt_args...}.toStringWithoutOptions());
+            }
+
             return access_denied(ErrorCodes::ACCESS_DENIED,
-                "{}: Not enough privileges. "
-                "The required privileges have been granted, but without grant option. "
-                "To execute this query, it's necessary to have the grant {} WITH GRANT OPTION",
-                AccessRightsElement{flags, args...}.toStringWithoutOptions());
+                "{}: Not enough privileges. To execute this query, it's necessary to have the grant {}",
+                AccessRightsElement{access_flags, fmt_args...}.toStringWithoutOptions() + (grant_option ? " WITH GRANT OPTION" : ""));
+        };
+
+        /// As we check the SOURCES from the Table Engine logic, direct prompt about Table Engine would be misleading
+        /// since SOURCES is not granted actually. In order to solve this, turn the prompt logic back to Sources.
+        if (flags & AccessType::TABLE_ENGINE && !access_control->doesTableEnginesRequireGrant())
+        {
+            AccessFlags new_flags;
+
+            String table_engine_name{getTableEngine(args...)};
+            for (const auto & source_and_table_engine : source_and_table_engines)
+            {
+                const auto & table_engine = std::get<1>(source_and_table_engine);
+                if (table_engine != table_engine_name) continue;
+                const auto & source = std::get<0>(source_and_table_engine);
+                /// Set the flags from Table Engine to SOURCES so that prompts can be meaningful.
+                new_flags = source;
+                break;
+            }
+
+            /// Might happen in the case of grant Table Engine on A (but not source), then revoke A.
+            if (new_flags.isEmpty())
+                return access_denied_no_grant(flags, args...);
+
+            if (grant_option && acs->isGranted(flags, args...))
+            {
+                return access_denied(ErrorCodes::ACCESS_DENIED,
+                    "{}: Not enough privileges. "
+                    "The required privileges have been granted, but without grant option. "
+                    "To execute this query, it's necessary to have the grant {} WITH GRANT OPTION",
+                    AccessRightsElement{new_flags}.toStringForAccessTypeSource());
+            }
+
+            return access_denied(ErrorCodes::ACCESS_DENIED,
+                "{}: Not enough privileges. To execute this query, it's necessary to have the grant {}",
+                AccessRightsElement{new_flags}.toStringForAccessTypeSource() + (grant_option ? " WITH GRANT OPTION" : ""));
         }
 
-        return access_denied(ErrorCodes::ACCESS_DENIED,
-            "{}: Not enough privileges. To execute this query, it's necessary to have the grant {}",
-            AccessRightsElement{flags, args...}.toStringWithoutOptions() + (grant_option ? " WITH GRANT OPTION" : ""));
+        return access_denied_no_grant(flags, args...);
     }
 
     struct PrecalculatedFlags

tests/integration/test_grant_and_revoke/configs/config_without_table_engine_grant.xml

@@ -0,0 +1,5 @@
+<clickhouse>
+    <access_control_improvements>
+        <table_engines_require_grant>false</table_engines_require_grant>
+    </access_control_improvements>
+</clickhouse>

tests/integration/test_grant_and_revoke/test_with_table_engine_grant.py

@@ -5,7 +5,7 @@ from helpers.test_tools import TSV
 cluster = ClickHouseCluster(__file__)
 instance = cluster.add_instance(
     "instance",
-    main_configs=["configs/config.xml"],
+    main_configs=["configs/config_with_table_engine_grant.xml"],
     user_configs=["configs/users.d/users.xml"],
 )
 

tests/integration/test_grant_and_revoke/test_without_table_engine_grant.py

@@ -0,0 +1,82 @@
+import pytest
+from helpers.cluster import ClickHouseCluster
+from helpers.test_tools import TSV
+
+cluster = ClickHouseCluster(__file__)
+instance = cluster.add_instance(
+    "instance",
+    main_configs=["configs/config_without_table_engine_grant.xml"],
+    user_configs=["configs/users.d/users.xml"],
+)
+
+
+@pytest.fixture(scope="module", autouse=True)
+def start_cluster():
+    try:
+        cluster.start()
+
+        instance.query("CREATE DATABASE test")
+
+        yield cluster
+
+    finally:
+        cluster.shutdown()
+
+
+@pytest.fixture(autouse=True)
+def cleanup_after_test():
+    try:
+        yield
+    finally:
+        instance.query("DROP USER IF EXISTS A")
+        instance.query("DROP TABLE IF EXISTS test.table1")
+
+
+def test_table_engine_and_source_grant():
+    instance.query("DROP USER IF EXISTS A")
+    instance.query("CREATE USER A")
+    instance.query("GRANT CREATE TABLE ON test.table1 TO A")
+
+    instance.query("GRANT POSTGRES ON *.* TO A")
+
+    instance.query(
+        """
+        CREATE TABLE test.table1(a Integer)
+        engine=PostgreSQL('localhost:5432', 'dummy', 'dummy', 'dummy', 'dummy');
+        """,
+        user="A",
+    )
+
+    instance.query("DROP TABLE test.table1")
+
+    instance.query("REVOKE POSTGRES ON *.* FROM A")
+
+    assert "Not enough privileges" in instance.query_and_get_error(
+        """
+        CREATE TABLE test.table1(a Integer)
+        engine=PostgreSQL('localhost:5432', 'dummy', 'dummy', 'dummy', 'dummy');
+        """,
+        user="A",
+    )
+
+    # expecting grant POSTGRES instead of grant PostgreSQL due to discrepancy between source access type and table engine
+    # similarily, other sources should also use their own defined name instead of the name of table engine
+    assert "grant POSTGRES ON *.*" in instance.query_and_get_error(
+        """
+        CREATE TABLE test.table1(a Integer)
+        engine=PostgreSQL('localhost:5432', 'dummy', 'dummy', 'dummy', 'dummy');
+        """,
+        user="A",
+    )
+
+    instance.query("GRANT SOURCES ON *.* TO A")
+
+    instance.query(
+        """
+        CREATE TABLE test.table1(a Integer)
+        engine=PostgreSQL('localhost:5432', 'dummy', 'dummy', 'dummy', 'dummy');
+        """,
+        user="A",
+    )
+
+    instance.query("DROP TABLE test.table1")