Merge pull request ClickHouse#60117 from jsc0218/TableEngineGrant_ver…

…sion2

TableEngineGrant_version2

docs/en/sql-reference/statements/grant.md

@@ -209,6 +209,7 @@ Hierarchy of privileges:
     - `SHOW NAMED COLLECTIONS`
     - `SHOW NAMED COLLECTIONS SECRETS`
     - `NAMED COLLECTION`
+- [TABLE ENGINE](#grant-table-engine)
 
 Examples of how this hierarchy is treated:
 
@@ -505,6 +506,7 @@ and
 [`format_display_secrets_in_show_and_select` format setting](../../operations/settings/formats#format_display_secrets_in_show_and_select)
 are turned on.
 
+
 ### NAMED COLLECTION ADMIN
 
 Allows a certain operation on a specified named collection. Before version 23.7 it was called NAMED COLLECTION CONTROL, and after 23.7 NAMED COLLECTION ADMIN was added and NAMED COLLECTION CONTROL is preserved as an alias.
@@ -524,6 +526,17 @@ Unlike all other grants (CREATE, DROP, ALTER, SHOW) grant NAMED COLLECTION was a
 Assuming a named collection is called abc, we grant privilege CREATE NAMED COLLECTION to user john.
 - `GRANT CREATE NAMED COLLECTION ON abc TO john`
 
+
+### TABLE ENGINE
+
+Allows using a specified table engine when creating a table. Applies to [table engines](../../engines/table-engines/index.md).
+
+**Examples**
+
+- `GRANT TABLE ENGINE ON * TO john`
+- `GRANT TABLE ENGINE ON TinyLog TO john`
+
+
 ### ALL
 
 Grants all the privileges on regulated entity to a user account or a role.

programs/server/config.xml

@@ -742,6 +742,10 @@
              It also enables 'changeable_in_readonly' constraint type -->
         <settings_constraints_replace_previous>true</settings_constraints_replace_previous>
 
+        <!-- By default, for backward compatibility create table with a specific table engine ignores grant,
+             however you can change this behaviour by setting this to true -->
+        <table_engines_require_grant>true</table_engines_require_grant>
+
         <!-- Number of seconds since last access a role is stored in the Role Cache -->
         <role_cache_expiration_time_seconds>600</role_cache_expiration_time_seconds>
     </access_control_improvements>

src/Access/AccessControl.cpp

@@ -285,6 +285,7 @@ void AccessControl::setUpFromMainConfig(const Poco::Util::AbstractConfiguration
     setSelectFromSystemDatabaseRequiresGrant(config_.getBool("access_control_improvements.select_from_system_db_requires_grant", false));
     setSelectFromInformationSchemaRequiresGrant(config_.getBool("access_control_improvements.select_from_information_schema_requires_grant", false));
     setSettingsConstraintsReplacePrevious(config_.getBool("access_control_improvements.settings_constraints_replace_previous", false));
+    setTableEnginesRequireGrant(config_.getBool("access_control_improvements.table_engines_require_grant", false));
 
     addStoragesFromMainConfig(config_, config_path_, get_zookeeper_function_);
 

src/Access/AccessControl.h

@@ -182,6 +182,9 @@ class AccessControl : public MultipleAccessStorage
     void setSettingsConstraintsReplacePrevious(bool enable) { settings_constraints_replace_previous = enable; }
     bool doesSettingsConstraintsReplacePrevious() const { return settings_constraints_replace_previous; }
 
+    void setTableEnginesRequireGrant(bool enable) { table_engines_require_grant = enable; }
+    bool doesTableEnginesRequireGrant() const { return table_engines_require_grant; }
+
     std::shared_ptr<const ContextAccess> getContextAccess(const ContextAccessParams & params) const;
 
     std::shared_ptr<const EnabledRoles> getEnabledRoles(
@@ -258,6 +261,7 @@ class AccessControl : public MultipleAccessStorage
     std::atomic_bool select_from_system_db_requires_grant = false;
     std::atomic_bool select_from_information_schema_requires_grant = false;
     std::atomic_bool settings_constraints_replace_previous = false;
+    std::atomic_bool table_engines_require_grant = false;
     std::atomic_int bcrypt_workfactor = 12;
     std::atomic<AuthenticationType> default_password_type = AuthenticationType::SHA256_PASSWORD;
 };

src/Access/Common/AccessFlags.cpp

@@ -101,8 +101,9 @@ namespace
         const Flags & getTableFlags() const { return all_flags_for_target[TABLE]; }
         const Flags & getColumnFlags() const { return all_flags_for_target[COLUMN]; }
         const Flags & getDictionaryFlags() const { return all_flags_for_target[DICTIONARY]; }
-        const Flags & getNamedCollectionFlags() const { return all_flags_for_target[NAMED_COLLECTION]; }
+        const Flags & getTableEngineFlags() const { return all_flags_for_target[TABLE_ENGINE]; }
         const Flags & getUserNameFlags() const { return all_flags_for_target[USER_NAME]; }
+        const Flags & getNamedCollectionFlags() const { return all_flags_for_target[NAMED_COLLECTION]; }
         const Flags & getAllFlagsGrantableOnGlobalLevel() const { return getAllFlags(); }
         const Flags & getAllFlagsGrantableOnGlobalWithParameterLevel() const { return getGlobalWithParameterFlags(); }
         const Flags & getAllFlagsGrantableOnDatabaseLevel() const { return all_flags_grantable_on_database_level; }
@@ -122,6 +123,7 @@ namespace
             DICTIONARY,
             NAMED_COLLECTION,
             USER_NAME,
+            TABLE_ENGINE,
         };
 
         struct Node;
@@ -301,7 +303,7 @@ namespace
                 collectAllFlags(child.get());
 
             all_flags_grantable_on_table_level = all_flags_for_target[TABLE] | all_flags_for_target[DICTIONARY] | all_flags_for_target[COLUMN];
-            all_flags_grantable_on_global_with_parameter_level = all_flags_for_target[NAMED_COLLECTION] | all_flags_for_target[USER_NAME];
+            all_flags_grantable_on_global_with_parameter_level = all_flags_for_target[NAMED_COLLECTION] | all_flags_for_target[USER_NAME] | all_flags_for_target[TABLE_ENGINE];
             all_flags_grantable_on_database_level = all_flags_for_target[DATABASE] | all_flags_grantable_on_table_level;
         }
 
@@ -352,7 +354,7 @@ namespace
         std::unordered_map<std::string_view, Flags> keyword_to_flags_map;
         std::vector<Flags> access_type_to_flags_mapping;
         Flags all_flags;
-        Flags all_flags_for_target[static_cast<size_t>(USER_NAME) + 1];
+        Flags all_flags_for_target[static_cast<size_t>(TABLE_ENGINE) + 1];
         Flags all_flags_grantable_on_database_level;
         Flags all_flags_grantable_on_table_level;
         Flags all_flags_grantable_on_global_with_parameter_level;
@@ -376,7 +378,11 @@ std::unordered_map<AccessFlags::ParameterType, AccessFlags> AccessFlags::splitIn
     if (user_flags)
         result.emplace(ParameterType::USER_NAME, user_flags);
 
-    auto other_flags = (~named_collection_flags & ~user_flags) & *this;
+    auto table_engine_flags = AccessFlags::allTableEngineFlags() & *this;
+    if (table_engine_flags)
+        result.emplace(ParameterType::TABLE_ENGINE, table_engine_flags);
+
+    auto other_flags = (~named_collection_flags & ~user_flags & ~table_engine_flags) & *this;
     if (other_flags)
         result.emplace(ParameterType::NONE, other_flags);
 
@@ -395,6 +401,10 @@ AccessFlags::ParameterType AccessFlags::getParameterType() const
     if (AccessFlags::allUserNameFlags().contains(*this))
         return AccessFlags::USER_NAME;
 
+    /// All flags refer to TABLE ENGINE access type.
+    if (AccessFlags::allTableEngineFlags().contains(*this))
+        return AccessFlags::TABLE_ENGINE;
+
     throw Exception(ErrorCodes::MIXED_ACCESS_PARAMETER_TYPES, "Having mixed parameter types: {}", toString());
 }
 
@@ -414,6 +424,7 @@ AccessFlags AccessFlags::allColumnFlags() { return Helper::instance().getColumnF
 AccessFlags AccessFlags::allDictionaryFlags() { return Helper::instance().getDictionaryFlags(); }
 AccessFlags AccessFlags::allNamedCollectionFlags() { return Helper::instance().getNamedCollectionFlags(); }
 AccessFlags AccessFlags::allUserNameFlags() { return Helper::instance().getUserNameFlags(); }
+AccessFlags AccessFlags::allTableEngineFlags() { return Helper::instance().getTableEngineFlags(); }
 AccessFlags AccessFlags::allFlagsGrantableOnGlobalLevel() { return Helper::instance().getAllFlagsGrantableOnGlobalLevel(); }
 AccessFlags AccessFlags::allFlagsGrantableOnGlobalWithParameterLevel() { return Helper::instance().getAllFlagsGrantableOnGlobalWithParameterLevel(); }
 AccessFlags AccessFlags::allFlagsGrantableOnDatabaseLevel() { return Helper::instance().getAllFlagsGrantableOnDatabaseLevel(); }

src/Access/Common/AccessFlags.h

@@ -56,6 +56,7 @@ class AccessFlags
     enum ParameterType
     {
         NONE,
+        TABLE_ENGINE,
         NAMED_COLLECTION,
         USER_NAME,
     };
@@ -107,6 +108,9 @@ class AccessFlags
     /// Returns all the flags related to a user.
     static AccessFlags allUserNameFlags();
 
+    /// Returns all the flags related to a table engine.
+    static AccessFlags allTableEngineFlags();
+
     /// Returns all the flags which could be granted on the global level.
     /// The same as allFlags().
     static AccessFlags allFlagsGrantableOnGlobalLevel();

src/Access/Common/AccessType.h

@@ -12,7 +12,7 @@ enum class AccessType
 /// Macro M should be defined as M(name, aliases, node_type, parent_group_name)
 /// where name is identifier with underscores (instead of spaces);
 /// aliases is a string containing comma-separated list;
-/// node_type either specifies access type's level (GLOBAL/NAMED_COLLECTION/USER_NAME/DATABASE/TABLE/DICTIONARY/VIEW/COLUMNS),
+/// node_type either specifies access type's level (GLOBAL/NAMED_COLLECTION/USER_NAME/TABLE_ENGINE/DATABASE/TABLE/DICTIONARY/VIEW/COLUMNS),
 /// or specifies that the access type is a GROUP of other access types;
 /// parent_group_name is the name of the group containing this access type (or NONE if there is no such group).
 /// NOTE A parent group must be declared AFTER all its children.
@@ -153,6 +153,8 @@ enum class AccessType
     M(NAMED_COLLECTION_ADMIN, "NAMED COLLECTION CONTROL", NAMED_COLLECTION, ALL) \
     M(SET_DEFINER, "", USER_NAME, ALL) \
     \
+    M(TABLE_ENGINE, "TABLE ENGINE", TABLE_ENGINE, ALL) \
+    \
     M(SYSTEM_SHUTDOWN, "SYSTEM KILL, SHUTDOWN", GLOBAL, SYSTEM) \
     M(SYSTEM_DROP_DNS_CACHE, "SYSTEM DROP DNS, DROP DNS CACHE, DROP DNS", GLOBAL, SYSTEM_DROP_CACHE)  \
     M(SYSTEM_DROP_CONNECTIONS_CACHE, "SYSTEM DROP CONNECTIONS CACHE, DROP CONNECTIONS CACHE", GLOBAL, SYSTEM_DROP_CACHE)  \

src/Access/ContextAccess.cpp

@@ -204,6 +204,52 @@ namespace
             res.grant(AccessType::SELECT, DatabaseCatalog::INFORMATION_SCHEMA_UPPERCASE);
         }
 
+        /// 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())
+        {
+            for (const auto & source_and_table_engine : source_and_table_engines)
+            {
+                const auto & source = std::get<0>(source_and_table_engine);
+                if (res.isGranted(source))
+                {
+                    const auto & table_engine = std::get<1>(source_and_table_engine);
+                    res.grant(AccessType::TABLE_ENGINE, table_engine);
+                }
+            }
+        }
+        else
+        {
+            /// Add TABLE_ENGINE on * and then remove TABLE_ENGINE on particular engines.
+            res.grant(AccessType::TABLE_ENGINE);
+            for (const auto & source_and_table_engine : source_and_table_engines)
+            {
+                const auto & source = std::get<0>(source_and_table_engine);
+                if (!res.isGranted(source))
+                {
+                    const auto & table_engine = std::get<1>(source_and_table_engine);
+                    res.revoke(AccessType::TABLE_ENGINE, table_engine);
+                }
+            }
+        }
+
         return res;
     }
 

src/Access/UsersConfigAccessStorage.cpp

@@ -371,6 +371,7 @@ namespace
             if (databases)
             {
                 user->access.revoke(AccessFlags::allFlags() - AccessFlags::allGlobalFlags());
+                user->access.grantWithGrantOption(AccessType::TABLE_ENGINE);
                 user->access.grantWithGrantOption(AccessFlags::allDictionaryFlags(), IDictionary::NO_DATABASE_TAG);
                 for (const String & database : *databases)
                     user->access.grantWithGrantOption(AccessFlags::allFlags(), database);

src/Access/tests/gtest_access_rights_ops.cpp

@@ -54,7 +54,7 @@ TEST(AccessRights, Union)
               "SYSTEM MOVES, SYSTEM PULLING REPLICATION LOG, SYSTEM CLEANUP, SYSTEM VIEWS, SYSTEM SENDS, SYSTEM REPLICATION QUEUES, SYSTEM VIRTUAL PARTS UPDATE, "
               "SYSTEM DROP REPLICA, SYSTEM SYNC REPLICA, SYSTEM RESTART REPLICA, "
               "SYSTEM RESTORE REPLICA, SYSTEM WAIT LOADING PARTS, SYSTEM SYNC DATABASE REPLICA, SYSTEM FLUSH DISTRIBUTED, dictGet ON db1.*, "
-              "GRANT SET DEFINER ON db1, GRANT NAMED COLLECTION ADMIN ON db1");
+              "GRANT TABLE ENGINE ON db1, GRANT SET DEFINER ON db1, GRANT NAMED COLLECTION ADMIN ON db1");
 }
 
 

src/Interpreters/InterpreterCreateQuery.cpp

@@ -732,11 +732,7 @@ InterpreterCreateQuery::TableProperties InterpreterCreateQuery::getTableProperti
 
     /// We have to check access rights again (in case engine was changed).
     if (create.storage && create.storage->engine)
-    {
-        auto source_access_type = StorageFactory::instance().getSourceAccessType(create.storage->engine->name);
-        if (source_access_type != AccessType::NONE)
-            getContext()->checkAccess(source_access_type);
-    }
+        getContext()->checkAccess(AccessType::TABLE_ENGINE, create.storage->engine->name);
 
     TableProperties properties;
     TableLockHolder as_storage_lock;
@@ -1842,11 +1838,7 @@ AccessRightsElements InterpreterCreateQuery::getRequiredAccess() const
         required_access.emplace_back(AccessType::SELECT | AccessType::INSERT, create.to_table_id.database_name, create.to_table_id.table_name);
 
     if (create.storage && create.storage->engine)
-    {
-        auto source_access_type = StorageFactory::instance().getSourceAccessType(create.storage->engine->name);
-        if (source_access_type != AccessType::NONE)
-            required_access.emplace_back(source_access_type);
-    }
+        required_access.emplace_back(AccessType::TABLE_ENGINE, create.storage->engine->name);
 
     return required_access;
 }

src/Storages/System/StorageSystemPrivileges.cpp

@@ -30,6 +30,7 @@ namespace
         COLUMN,
         NAMED_COLLECTION,
         USER_NAME,
+        TABLE_ENGINE,
     };
 
     DataTypeEnum8::Values getLevelEnumValues()
@@ -43,6 +44,7 @@ namespace
         enum_values.emplace_back("COLUMN", static_cast<Int8>(COLUMN));
         enum_values.emplace_back("NAMED_COLLECTION", static_cast<Int8>(NAMED_COLLECTION));
         enum_values.emplace_back("USER_NAME", static_cast<Int8>(USER_NAME));
+        enum_values.emplace_back("TABLE_ENGINE", static_cast<Int8>(TABLE_ENGINE));
         return enum_values;
     }
 }

tests/config/config.d/enable_access_control_improvements.xml

@@ -5,5 +5,6 @@
         <select_from_system_db_requires_grant>true</select_from_system_db_requires_grant>
         <select_from_information_schema_requires_grant>true</select_from_information_schema_requires_grant>
         <settings_constraints_replace_previous>true</settings_constraints_replace_previous>
+        <table_engines_require_grant>true</table_engines_require_grant>
     </access_control_improvements>
 </clickhouse>

tests/integration/test_backup_restore_new/test.py

@@ -1036,6 +1036,7 @@ def test_required_privileges():
     )
 
     instance.query("GRANT INSERT, CREATE ON test.table2 TO u1")
+    instance.query("GRANT TABLE ENGINE ON MergeTree TO u1")
     instance.query(
         f"RESTORE TABLE test.table AS test.table2 FROM {backup_name}", user="u1"
     )

tests/integration/test_distributed_ddl/test.py

@@ -300,11 +300,13 @@ def test_allowed_databases(test_cluster):
             "CREATE TABLE db2.t2 ON CLUSTER cluster (i Int8) ENGINE = Memory",
             settings={"user": "restricted_user"},
         )
+
     with pytest.raises(Exception):
         instance.query(
             "CREATE TABLE t3 ON CLUSTER cluster (i Int8) ENGINE = Memory",
             settings={"user": "restricted_user"},
         )
+
     with pytest.raises(Exception):
         instance.query(
             "DROP DATABASE db2 ON CLUSTER cluster", settings={"user": "restricted_user"}

tests/integration/test_grant_and_revoke/configs/config.xml

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