Merge pull request #43993 from evillique/fix_settings_constraints

Fix CREATE query constraints

src/Access/SettingsConstraints.cpp

@@ -86,6 +86,49 @@ void SettingsConstraints::merge(const SettingsConstraints & other)
 }
 
 
+void SettingsConstraints::check(const Settings & current_settings, const SettingsProfileElements & profile_elements) const
+{
+    for (const auto & element : profile_elements)
+    {
+        if (SettingsProfileElements::isAllowBackupSetting(element.setting_name))
+            continue;
+
+        if (!element.value.isNull())
+        {
+            SettingChange value(element.setting_name, element.value);
+            check(current_settings, value);
+        }
+
+        if (!element.min_value.isNull())
+        {
+            SettingChange value(element.setting_name, element.min_value);
+            check(current_settings, value);
+        }
+
+        if (!element.max_value.isNull())
+        {
+            SettingChange value(element.setting_name, element.max_value);
+            check(current_settings, value);
+        }
+
+        SettingConstraintWritability new_value = SettingConstraintWritability::WRITABLE;
+        SettingConstraintWritability old_value = SettingConstraintWritability::WRITABLE;
+
+        if (element.writability)
+            new_value = *element.writability;
+
+        auto it = constraints.find(element.setting_name);
+        if (it != constraints.end())
+            old_value = it->second.writability;
+
+        if (new_value != old_value)
+        {
+            if (old_value == SettingConstraintWritability::CONST)
+                throw Exception("Setting " + element.setting_name + " should not be changed", ErrorCodes::SETTING_CONSTRAINT_VIOLATION);
+        }
+    }
+}
+
 void SettingsConstraints::check(const Settings & current_settings, const SettingChange & change) const
 {
     checkImpl(current_settings, const_cast<SettingChange &>(change), THROW_ON_VIOLATION);

src/Access/SettingsConstraints.h

@@ -73,6 +73,7 @@ public:
     void merge(const SettingsConstraints & other);
 
     /// Checks whether `change` violates these constraints and throws an exception if so.
+    void check(const Settings & current_settings, const SettingsProfileElements & profile_elements) const;
     void check(const Settings & current_settings, const SettingChange & change) const;
     void check(const Settings & current_settings, const SettingsChanges & changes) const;
     void check(const Settings & current_settings, SettingsChanges & changes) const;

src/Access/SettingsProfileElement.cpp

@@ -248,4 +248,9 @@ bool SettingsProfileElements::isBackupAllowed() const
     return true;
 }
 
+bool SettingsProfileElements::isAllowBackupSetting(const String & setting_name)
+{
+    return setting_name == ALLOW_BACKUP_SETTING_NAME;
+}
+
 }

src/Access/SettingsProfileElement.h

@@ -71,6 +71,8 @@ public:
     std::vector<UUID> toProfileIDs() const;
 
     bool isBackupAllowed() const;
+
+    static bool isAllowBackupSetting(const String & setting_name);
 };
 
 }

src/Interpreters/Access/InterpreterCreateRoleQuery.cpp

@@ -40,13 +40,18 @@ BlockIO InterpreterCreateRoleQuery::execute()
     else
         getContext()->checkAccess(AccessType::CREATE_ROLE);
 
-    if (!query.cluster.empty())
-        return executeDDLQueryOnCluster(query_ptr, getContext());
-
     std::optional<SettingsProfileElements> settings_from_query;
     if (query.settings)
+    {
         settings_from_query = SettingsProfileElements{*query.settings, access_control};
 
+        if (!query.attach)
+            getContext()->checkSettingsConstraints(*settings_from_query);
+    }
+
+    if (!query.cluster.empty())
+        return executeDDLQueryOnCluster(query_ptr, getContext());
+
     if (query.alter)
     {
         auto update_func = [&](const AccessEntityPtr & entity) -> AccessEntityPtr

src/Interpreters/Access/InterpreterCreateSettingsProfileQuery.cpp

@@ -48,16 +48,21 @@ BlockIO InterpreterCreateSettingsProfileQuery::execute()
     else
         getContext()->checkAccess(AccessType::CREATE_SETTINGS_PROFILE);
 
+    std::optional<SettingsProfileElements> settings_from_query;
+    if (query.settings)
+    {
+        settings_from_query = SettingsProfileElements{*query.settings, access_control};
+
+        if (!query.attach)
+            getContext()->checkSettingsConstraints(*settings_from_query);
+    }
+
     if (!query.cluster.empty())
     {
         query.replaceCurrentUserTag(getContext()->getUserName());
         return executeDDLQueryOnCluster(query_ptr, getContext());
     }
 
-    std::optional<SettingsProfileElements> settings_from_query;
-    if (query.settings)
-        settings_from_query = SettingsProfileElements{*query.settings, access_control};
-
     std::optional<RolesOrUsersSet> roles_from_query;
     if (query.to_roles)
         roles_from_query = RolesOrUsersSet{*query.to_roles, access_control, getContext()->getUserID()};

src/Interpreters/Access/InterpreterCreateUserQuery.cpp

@@ -124,12 +124,19 @@ BlockIO InterpreterCreateUserQuery::execute()
                 access->checkAdminOption(role);
         }
     }
-    if (!query.cluster.empty())
-        return executeDDLQueryOnCluster(query_ptr, getContext());
+
     std::optional<SettingsProfileElements> settings_from_query;
     if (query.settings)
+    {
         settings_from_query = SettingsProfileElements{*query.settings, access_control};
 
+        if (!query.attach)
+            getContext()->checkSettingsConstraints(*settings_from_query);
+    }
+
+    if (!query.cluster.empty())
+        return executeDDLQueryOnCluster(query_ptr, getContext());
+
     if (query.alter)
     {
         std::optional<RolesOrUsersSet> grantees_from_query;

src/Interpreters/Context.cpp

@@ -1412,6 +1412,11 @@ void Context::applySettingsChanges(const SettingsChanges & changes)
 }
 
 
+void Context::checkSettingsConstraints(const SettingsProfileElements & profile_elements) const
+{
+    getSettingsConstraintsAndCurrentProfiles()->constraints.check(settings, profile_elements);
+}
+
 void Context::checkSettingsConstraints(const SettingChange & change) const
 {
     getSettingsConstraintsAndCurrentProfiles()->constraints.check(settings, change);

src/Interpreters/Context.h

@@ -110,6 +110,7 @@ class AccessControl;
 class Credentials;
 class GSSAcceptorContext;
 struct SettingsConstraintsAndProfileIDs;
+class SettingsProfileElements;
 class RemoteHostFilter;
 struct StorageID;
 class IDisk;
@@ -658,6 +659,7 @@ public:
     void applySettingsChanges(const SettingsChanges & changes);
 
     /// Checks the constraints.
+    void checkSettingsConstraints(const SettingsProfileElements & profile_elements) const;
     void checkSettingsConstraints(const SettingChange & change) const;
     void checkSettingsConstraints(const SettingsChanges & changes) const;
     void checkSettingsConstraints(SettingsChanges & changes) const;

tests/integration/test_create_query_constraints/test.py

@@ -0,0 +1,92 @@
+import pytest
+import asyncio
+import re
+import random
+import os.path
+from helpers.cluster import ClickHouseCluster
+from helpers.test_tools import assert_eq_with_retry, TSV
+
+cluster = ClickHouseCluster(__file__)
+instance = cluster.add_instance("instance")
+
+
+@pytest.fixture(scope="module", autouse=True)
+def start_cluster():
+    try:
+        cluster.start()
+        yield cluster
+    finally:
+        cluster.shutdown()
+
+
+def test_create_query_const_constraints():
+
+    instance.query("CREATE USER u_const SETTINGS max_threads = 1 CONST")
+    instance.query("GRANT ALL ON *.* TO u_const")
+
+    expected_error = "Setting max_threads should not be changed"
+
+    assert expected_error in instance.query_and_get_error(
+        "CREATE USER inner_user SETTINGS max_threads = 1", user="u_const"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE USER inner_user SETTINGS max_threads MIN 0 MAX 2", user="u_const"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE USER inner_user SETTINGS max_threads WRITABLE", user="u_const"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE ROLE inner_role SETTINGS max_threads = 1", user="u_const"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE SETTINGS PROFILE inner_profile SETTINGS max_threads = 1", user="u_const"
+    )
+
+    instance.query(
+        "CREATE USER inner_user_1 SETTINGS max_threads CONST", user="u_const"
+    )
+    instance.query(
+        "CREATE USER inner_user_2 SETTINGS max_threads = 1 CONST", user="u_const"
+    )
+    instance.query("DROP USER u_const, inner_user_1, inner_user_2")
+
+
+def test_create_query_minmax_constraints():
+
+    instance.query("CREATE USER u_minmax SETTINGS max_threads = 4 MIN 2 MAX 6")
+    instance.query("GRANT ALL ON *.* TO u_minmax")
+
+    expected_error = "Setting max_threads shouldn't be less than"
+
+    assert expected_error in instance.query_and_get_error(
+        "CREATE USER inner_user SETTINGS max_threads = 1", user="u_minmax"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE USER inner_user SETTINGS max_threads MIN 1 MAX 3", user="u_minmax"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE ROLE inner_role SETTINGS max_threads MIN 1 MAX 3", user="u_minmax"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE SETTINGS PROFILE inner_profile SETTINGS max_threads MIN 1 MAX 3",
+        user="u_minmax",
+    )
+
+    expected_error = "Setting max_threads shouldn't be greater than"
+
+    assert expected_error in instance.query_and_get_error(
+        "CREATE USER inner_user SETTINGS max_threads = 8", user="u_minmax"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE USER inner_user SETTINGS max_threads MIN 4 MAX 8", user="u_minmax"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE ROLE inner_role SETTINGS max_threads MIN 4 MAX 8", user="u_minmax"
+    )
+    assert expected_error in instance.query_and_get_error(
+        "CREATE SETTINGS PROFILE inner_profile SETTINGS max_threads MIN 4 MAX 8",
+        user="u_minmax",
+    )
+
+    instance.query("CREATE USER inner_user SETTINGS max_threads = 3", user="u_minmax")
+    instance.query("DROP USER u_minmax, inner_user")