From c1e465b6fb6543e9cc72c2d434da73b9837e2ea8 Mon Sep 17 00:00:00 2001 From: pufit Date: Tue, 30 Apr 2024 17:00:05 -0400 Subject: [PATCH 01/14] Correct load for SQL security defaults during startup --- src/Databases/DatabaseOnDisk.cpp | 6 +++ ...mpty_sql_security_in_create_view_query.xml | 3 ++ .../test_ignore_empty_sql_security.py | 48 +++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 tests/integration/test_sql_security/configs/ignore_empty_sql_security_in_create_view_query.xml create mode 100644 tests/integration/test_sql_security/test_ignore_empty_sql_security.py diff --git a/src/Databases/DatabaseOnDisk.cpp b/src/Databases/DatabaseOnDisk.cpp index 674e9afa8ac..93bc119009f 100644 --- a/src/Databases/DatabaseOnDisk.cpp +++ b/src/Databases/DatabaseOnDisk.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -67,6 +68,11 @@ std::pair createTableFromAST( ast_create_query.attach = true; ast_create_query.setDatabase(database_name); + if (!ast_create_query.sql_security && ast_create_query.supportSQLSecurity() && !context->getServerSettings().ignore_empty_sql_security_in_create_view_query) + { + ast_create_query.sql_security = std::make_shared(); + InterpreterCreateQuery::processSQLSecurityOption(context, ast_create_query.sql_security->as(), true, ast_create_query.is_materialized_view); + } if (ast_create_query.select && ast_create_query.isView()) ApplyWithSubqueryVisitor::visit(*ast_create_query.select); diff --git a/tests/integration/test_sql_security/configs/ignore_empty_sql_security_in_create_view_query.xml b/tests/integration/test_sql_security/configs/ignore_empty_sql_security_in_create_view_query.xml new file mode 100644 index 00000000000..99819f58630 --- /dev/null +++ b/tests/integration/test_sql_security/configs/ignore_empty_sql_security_in_create_view_query.xml @@ -0,0 +1,3 @@ + + 1 + diff --git a/tests/integration/test_sql_security/test_ignore_empty_sql_security.py b/tests/integration/test_sql_security/test_ignore_empty_sql_security.py new file mode 100644 index 00000000000..cf6b29ec717 --- /dev/null +++ b/tests/integration/test_sql_security/test_ignore_empty_sql_security.py @@ -0,0 +1,48 @@ +#!/usr/bin/env python3 +import pytest +from helpers.cluster import ClickHouseCluster + +cluster = ClickHouseCluster(__file__) +node = cluster.add_instance( + "node1", + main_configs=["configs/ignore_empty_sql_security_in_create_view_query.xml"], + stay_alive=True, +) + + +@pytest.fixture(scope="module") +def started_cluster(): + try: + cluster.start() + + yield cluster + + finally: + cluster.shutdown() + + +def test_load_mv_with_security_none(started_cluster: ClickHouseCluster): + node.query( + "CREATE TABLE test_table (s String) ENGINE = MergeTree ORDER BY s" + ) + node.query( + "CREATE MATERIALIZED VIEW test_mv_1 (s String) ENGINE = MergeTree ORDER BY s AS SELECT * FROM test_table" + ) + node.query("INSERT INTO test_table VALUES ('foo'), ('bar')") + + node.query("CREATE USER test_user") + node.query("GRANT SELECT ON test_mv_1 TO test_user") + + with pytest.raises(Exception, match="Not enough privileges"): + node.query("SELECT count() FROM test_mv_1", user="test_user") + + node.replace_in_config( + "/etc/clickhouse-server/config.d/ignore_empty_sql_security_in_create_view_query.xml", + "1", + "0", + ) + + node.restart_clickhouse() + + assert node.query("SELECT count() FROM test_mv_1", user="test_user") == "2\n" + From a27f29f10c46784773779d92c39e27b0ac12c1e0 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Tue, 30 Apr 2024 21:38:56 +0000 Subject: [PATCH 02/14] Automatic style fix --- .../test_sql_security/test_ignore_empty_sql_security.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/integration/test_sql_security/test_ignore_empty_sql_security.py b/tests/integration/test_sql_security/test_ignore_empty_sql_security.py index cf6b29ec717..d6349242cfd 100644 --- a/tests/integration/test_sql_security/test_ignore_empty_sql_security.py +++ b/tests/integration/test_sql_security/test_ignore_empty_sql_security.py @@ -22,9 +22,7 @@ def started_cluster(): def test_load_mv_with_security_none(started_cluster: ClickHouseCluster): - node.query( - "CREATE TABLE test_table (s String) ENGINE = MergeTree ORDER BY s" - ) + node.query("CREATE TABLE test_table (s String) ENGINE = MergeTree ORDER BY s") node.query( "CREATE MATERIALIZED VIEW test_mv_1 (s String) ENGINE = MergeTree ORDER BY s AS SELECT * FROM test_table" ) @@ -45,4 +43,3 @@ def test_load_mv_with_security_none(started_cluster: ClickHouseCluster): node.restart_clickhouse() assert node.query("SELECT count() FROM test_mv_1", user="test_user") == "2\n" - From 3a6136d0e650dee0a660ca9d14d47b77585df3fc Mon Sep 17 00:00:00 2001 From: pufit Date: Tue, 30 Apr 2024 18:42:12 -0400 Subject: [PATCH 03/14] fix style --- tests/integration/test_sql_security/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 tests/integration/test_sql_security/__init__.py diff --git a/tests/integration/test_sql_security/__init__.py b/tests/integration/test_sql_security/__init__.py new file mode 100644 index 00000000000..e69de29bb2d From 642e225fdbee5f7d3b7b14f368c72d8feb20a8d9 Mon Sep 17 00:00:00 2001 From: pufit Date: Wed, 1 May 2024 12:36:48 -0400 Subject: [PATCH 04/14] Fix review --- src/Databases/DatabaseOnDisk.cpp | 6 ------ src/Storages/StorageMaterializedView.cpp | 10 +++++++++- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/Databases/DatabaseOnDisk.cpp b/src/Databases/DatabaseOnDisk.cpp index 93bc119009f..674e9afa8ac 100644 --- a/src/Databases/DatabaseOnDisk.cpp +++ b/src/Databases/DatabaseOnDisk.cpp @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -68,11 +67,6 @@ std::pair createTableFromAST( ast_create_query.attach = true; ast_create_query.setDatabase(database_name); - if (!ast_create_query.sql_security && ast_create_query.supportSQLSecurity() && !context->getServerSettings().ignore_empty_sql_security_in_create_view_query) - { - ast_create_query.sql_security = std::make_shared(); - InterpreterCreateQuery::processSQLSecurityOption(context, ast_create_query.sql_security->as(), true, ast_create_query.is_materialized_view); - } if (ast_create_query.select && ast_create_query.isView()) ApplyWithSubqueryVisitor::visit(*ast_create_query.select); diff --git a/src/Storages/StorageMaterializedView.cpp b/src/Storages/StorageMaterializedView.cpp index 696865dfa2f..8025cae512f 100644 --- a/src/Storages/StorageMaterializedView.cpp +++ b/src/Storages/StorageMaterializedView.cpp @@ -97,7 +97,15 @@ StorageMaterializedView::StorageMaterializedView( storage_metadata.columns, local_context->getGlobalContext()); - if (query.sql_security) + ASTPtr sql_security = query.sql_security; + if (!sql_security && query.supportSQLSecurity() && !getContext()->getServerSettings().ignore_empty_sql_security_in_create_view_query) + { + /// This is hack which allows to load materialized views during startup with default SQL security NONE for backward compatability. + sql_security = std::make_shared(); + InterpreterCreateQuery::processSQLSecurityOption(getContext(), sql_security->as(), true, query.is_materialized_view); + } + + if (sql_security) storage_metadata.setSQLSecurity(query.sql_security->as()); if (storage_metadata.sql_security_type == SQLSecurityType::INVOKER) From 8f95ca23f6636a9bb3a0c9517741eab422adebe3 Mon Sep 17 00:00:00 2001 From: pufit Date: Wed, 1 May 2024 14:00:12 -0400 Subject: [PATCH 05/14] Fix typo --- src/Storages/StorageMaterializedView.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/StorageMaterializedView.cpp b/src/Storages/StorageMaterializedView.cpp index 8025cae512f..f1564d93c01 100644 --- a/src/Storages/StorageMaterializedView.cpp +++ b/src/Storages/StorageMaterializedView.cpp @@ -100,7 +100,7 @@ StorageMaterializedView::StorageMaterializedView( ASTPtr sql_security = query.sql_security; if (!sql_security && query.supportSQLSecurity() && !getContext()->getServerSettings().ignore_empty_sql_security_in_create_view_query) { - /// This is hack which allows to load materialized views during startup with default SQL security NONE for backward compatability. + /// This is hack which allows to load materialized views during startup with default SQL security NONE for backward compatibility. sql_security = std::make_shared(); InterpreterCreateQuery::processSQLSecurityOption(getContext(), sql_security->as(), true, query.is_materialized_view); } From 92dee331f157f05338b1a5d543a8af574e19eb25 Mon Sep 17 00:00:00 2001 From: pufit Date: Wed, 1 May 2024 15:54:41 -0400 Subject: [PATCH 06/14] fix --- src/Storages/StorageMaterializedView.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Storages/StorageMaterializedView.cpp b/src/Storages/StorageMaterializedView.cpp index f1564d93c01..3ee461a2185 100644 --- a/src/Storages/StorageMaterializedView.cpp +++ b/src/Storages/StorageMaterializedView.cpp @@ -98,15 +98,15 @@ StorageMaterializedView::StorageMaterializedView( local_context->getGlobalContext()); ASTPtr sql_security = query.sql_security; - if (!sql_security && query.supportSQLSecurity() && !getContext()->getServerSettings().ignore_empty_sql_security_in_create_view_query) + if (!sql_security && !getContext()->getServerSettings().ignore_empty_sql_security_in_create_view_query) { /// This is hack which allows to load materialized views during startup with default SQL security NONE for backward compatibility. sql_security = std::make_shared(); - InterpreterCreateQuery::processSQLSecurityOption(getContext(), sql_security->as(), true, query.is_materialized_view); + InterpreterCreateQuery::processSQLSecurityOption(getContext(), sql_security->as(), true, true); } if (sql_security) - storage_metadata.setSQLSecurity(query.sql_security->as()); + storage_metadata.setSQLSecurity(sql_security->as()); if (storage_metadata.sql_security_type == SQLSecurityType::INVOKER) throw Exception(ErrorCodes::QUERY_IS_NOT_SUPPORTED_IN_MATERIALIZED_VIEW, "SQL SECURITY INVOKER can't be specified for MATERIALIZED VIEW"); From 08d6fa03b69fb3b8bcd53b6847c2723a1275cb81 Mon Sep 17 00:00:00 2001 From: pufit Date: Wed, 1 May 2024 16:00:55 -0400 Subject: [PATCH 07/14] fix comment --- src/Storages/StorageMaterializedView.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Storages/StorageMaterializedView.cpp b/src/Storages/StorageMaterializedView.cpp index 3ee461a2185..24c3778670a 100644 --- a/src/Storages/StorageMaterializedView.cpp +++ b/src/Storages/StorageMaterializedView.cpp @@ -100,7 +100,8 @@ StorageMaterializedView::StorageMaterializedView( ASTPtr sql_security = query.sql_security; if (!sql_security && !getContext()->getServerSettings().ignore_empty_sql_security_in_create_view_query) { - /// This is hack which allows to load materialized views during startup with default SQL security NONE for backward compatibility. + /// This allows materialized views to be loaded during startup with default SQL security for backward compatibility. + /// If ClickHouse loads an old materialized view created without SQL security, it will use the default `SQL SECURITY NONE` sql_security = std::make_shared(); InterpreterCreateQuery::processSQLSecurityOption(getContext(), sql_security->as(), true, true); } From be02a16599367cafdfa912aaf64c6b8992f0d2cc Mon Sep 17 00:00:00 2001 From: pufit Date: Thu, 2 May 2024 09:30:46 -0400 Subject: [PATCH 08/14] better --- src/Core/ServerSettings.h | 1 + src/Storages/StorageMaterializedView.cpp | 2 +- .../attach_materialized_views_with_sql_security_none.xml | 3 +++ .../ignore_empty_sql_security_in_create_view_query.xml | 3 --- .../test_sql_security/test_ignore_empty_sql_security.py | 4 ++-- 5 files changed, 7 insertions(+), 6 deletions(-) create mode 100644 tests/integration/test_sql_security/configs/attach_materialized_views_with_sql_security_none.xml delete mode 100644 tests/integration/test_sql_security/configs/ignore_empty_sql_security_in_create_view_query.xml diff --git a/src/Core/ServerSettings.h b/src/Core/ServerSettings.h index f41c596282f..cd616414016 100644 --- a/src/Core/ServerSettings.h +++ b/src/Core/ServerSettings.h @@ -61,6 +61,7 @@ namespace DB M(UInt64, async_insert_threads, 16, "Maximum number of threads to actually parse and insert data in background. Zero means asynchronous mode is disabled", 0) \ M(Bool, async_insert_queue_flush_on_shutdown, true, "If true queue of asynchronous inserts is flushed on graceful shutdown", 0) \ M(Bool, ignore_empty_sql_security_in_create_view_query, true, "If true, ClickHouse doesn't write defaults for empty SQL security statement in CREATE VIEW queries. This setting is only necessary for the migration period and will become obsolete in 24.4", 0) \ + M(Bool, attach_materialized_views_with_sql_security_none, true, "If true, all materialized views loaded without SQL security statement (e.g. `ignore_empty_sql_security_in_create_view_query = false` or when loading old created views) will use SQL security NONE.", 0) \ \ M(UInt64, max_concurrent_queries, 0, "Maximum number of concurrently executed queries. Zero means unlimited.", 0) \ M(UInt64, max_concurrent_insert_queries, 0, "Maximum number of concurrently INSERT queries. Zero means unlimited.", 0) \ diff --git a/src/Storages/StorageMaterializedView.cpp b/src/Storages/StorageMaterializedView.cpp index 24c3778670a..97e2cf6ef4d 100644 --- a/src/Storages/StorageMaterializedView.cpp +++ b/src/Storages/StorageMaterializedView.cpp @@ -98,7 +98,7 @@ StorageMaterializedView::StorageMaterializedView( local_context->getGlobalContext()); ASTPtr sql_security = query.sql_security; - if (!sql_security && !getContext()->getServerSettings().ignore_empty_sql_security_in_create_view_query) + if (!sql_security && query.attach && getContext()->getServerSettings().attach_materialized_views_with_sql_security_none) { /// This allows materialized views to be loaded during startup with default SQL security for backward compatibility. /// If ClickHouse loads an old materialized view created without SQL security, it will use the default `SQL SECURITY NONE` diff --git a/tests/integration/test_sql_security/configs/attach_materialized_views_with_sql_security_none.xml b/tests/integration/test_sql_security/configs/attach_materialized_views_with_sql_security_none.xml new file mode 100644 index 00000000000..556d845d43e --- /dev/null +++ b/tests/integration/test_sql_security/configs/attach_materialized_views_with_sql_security_none.xml @@ -0,0 +1,3 @@ + + 0 + diff --git a/tests/integration/test_sql_security/configs/ignore_empty_sql_security_in_create_view_query.xml b/tests/integration/test_sql_security/configs/ignore_empty_sql_security_in_create_view_query.xml deleted file mode 100644 index 99819f58630..00000000000 --- a/tests/integration/test_sql_security/configs/ignore_empty_sql_security_in_create_view_query.xml +++ /dev/null @@ -1,3 +0,0 @@ - - 1 - diff --git a/tests/integration/test_sql_security/test_ignore_empty_sql_security.py b/tests/integration/test_sql_security/test_ignore_empty_sql_security.py index d6349242cfd..c2e167700a2 100644 --- a/tests/integration/test_sql_security/test_ignore_empty_sql_security.py +++ b/tests/integration/test_sql_security/test_ignore_empty_sql_security.py @@ -5,7 +5,7 @@ from helpers.cluster import ClickHouseCluster cluster = ClickHouseCluster(__file__) node = cluster.add_instance( "node1", - main_configs=["configs/ignore_empty_sql_security_in_create_view_query.xml"], + main_configs=["configs/attach_materialized_views_with_sql_security_none.xml"], stay_alive=True, ) @@ -36,8 +36,8 @@ def test_load_mv_with_security_none(started_cluster: ClickHouseCluster): node.replace_in_config( "/etc/clickhouse-server/config.d/ignore_empty_sql_security_in_create_view_query.xml", - "1", "0", + "1", ) node.restart_clickhouse() From f685eea7c545b00af1249e62e38d1aa5e03ae13b Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Thu, 2 May 2024 16:59:19 +0200 Subject: [PATCH 09/14] Get rid of fallbacks, `all` is too far away in 2022 --- tests/ci/download_release_packages.py | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/tests/ci/download_release_packages.py b/tests/ci/download_release_packages.py index 550301e8fa2..b9376497258 100755 --- a/tests/ci/download_release_packages.py +++ b/tests/ci/download_release_packages.py @@ -1,10 +1,9 @@ #!/usr/bin/env python3 -import os import logging +import os import requests - from requests.adapters import HTTPAdapter # type: ignore from urllib3.util.retry import Retry # type: ignore @@ -19,10 +18,8 @@ CLICKHOUSE_COMMON_STATIC_PACKAGE_NAME = "clickhouse-common-static_{version}_amd6 CLICKHOUSE_COMMON_STATIC_DBG_PACKAGE_NAME = ( "clickhouse-common-static-dbg_{version}_amd64.deb" ) -CLICKHOUSE_SERVER_PACKAGE_NAME = "clickhouse-server_{version}_amd64.deb" -CLICKHOUSE_SERVER_PACKAGE_FALLBACK = "clickhouse-server_{version}_all.deb" CLICKHOUSE_CLIENT_PACKAGE_NAME = "clickhouse-client_{version}_amd64.deb" -CLICKHOUSE_CLIENT_PACKAGE_FALLBACK = "clickhouse-client_{version}_all.deb" +CLICKHOUSE_SERVER_PACKAGE_NAME = "clickhouse-server_{version}_amd64.deb" PACKAGES_DIR = "previous_release_package_folder/" VERSION_PATTERN = r"((?:\d+\.)?(?:\d+\.)?(?:\d+\.)?\d+-[a-zA-Z]*)" @@ -59,26 +56,13 @@ def download_packages(release, dest_path=PACKAGES_DIR): for pkg in ( CLICKHOUSE_COMMON_STATIC_PACKAGE_NAME, CLICKHOUSE_COMMON_STATIC_DBG_PACKAGE_NAME, + CLICKHOUSE_SERVER_PACKAGE_NAME, + CLICKHOUSE_CLIENT_PACKAGE_NAME, ): url = (DOWNLOAD_PREFIX + pkg).format(version=release.version, type=release.type) pkg_name = get_dest_path(pkg.format(version=release.version)) download_package(url, pkg_name) - for pkg, fallback in ( - (CLICKHOUSE_SERVER_PACKAGE_NAME, CLICKHOUSE_SERVER_PACKAGE_FALLBACK), - (CLICKHOUSE_CLIENT_PACKAGE_NAME, CLICKHOUSE_CLIENT_PACKAGE_FALLBACK), - ): - url = (DOWNLOAD_PREFIX + pkg).format(version=release.version, type=release.type) - pkg_name = get_dest_path(pkg.format(version=release.version)) - try: - download_package(url, pkg_name) - except Exception: - url = (DOWNLOAD_PREFIX + fallback).format( - version=release.version, type=release.type - ) - pkg_name = get_dest_path(fallback.format(version=release.version)) - download_package(url, pkg_name) - def download_last_release(dest_path): current_release = get_previous_release(None) From fb9b2be2f9692776e6ad9b57c1f1177d50c41aaa Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Thu, 2 May 2024 17:00:48 +0200 Subject: [PATCH 10/14] Leftover to fix after #62114 --- tests/ci/download_release_packages.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/ci/download_release_packages.py b/tests/ci/download_release_packages.py index b9376497258..1ba4ff8ff2e 100755 --- a/tests/ci/download_release_packages.py +++ b/tests/ci/download_release_packages.py @@ -19,6 +19,8 @@ CLICKHOUSE_COMMON_STATIC_DBG_PACKAGE_NAME = ( "clickhouse-common-static-dbg_{version}_amd64.deb" ) CLICKHOUSE_CLIENT_PACKAGE_NAME = "clickhouse-client_{version}_amd64.deb" +CLICKHOUSE_LIBRARY_BRIDGE_PACKAGE_NAME = "clickhouse-library-bridge_{version}_amd64.deb" +CLICKHOUSE_ODBC_BRIDGE_PACKAGE_NAME = "clickhouse-odbc-bridge_{version}_amd64.deb" CLICKHOUSE_SERVER_PACKAGE_NAME = "clickhouse-server_{version}_amd64.deb" PACKAGES_DIR = "previous_release_package_folder/" @@ -56,8 +58,10 @@ def download_packages(release, dest_path=PACKAGES_DIR): for pkg in ( CLICKHOUSE_COMMON_STATIC_PACKAGE_NAME, CLICKHOUSE_COMMON_STATIC_DBG_PACKAGE_NAME, - CLICKHOUSE_SERVER_PACKAGE_NAME, CLICKHOUSE_CLIENT_PACKAGE_NAME, + CLICKHOUSE_LIBRARY_BRIDGE_PACKAGE_NAME, + CLICKHOUSE_ODBC_BRIDGE_PACKAGE_NAME, + CLICKHOUSE_SERVER_PACKAGE_NAME, ): url = (DOWNLOAD_PREFIX + pkg).format(version=release.version, type=release.type) pkg_name = get_dest_path(pkg.format(version=release.version)) From 426f5a0434d8f042747a6daca80a58a25ece6ed2 Mon Sep 17 00:00:00 2001 From: pufit Date: Thu, 2 May 2024 18:56:54 -0400 Subject: [PATCH 11/14] Use INVOKER as a default for old MV --- src/Core/ServerSettings.h | 1 - .../Transforms/buildPushingToViewsChain.cpp | 3 +- src/Storages/StorageMaterializedView.cpp | 13 +++--- .../integration/test_sql_security/__init__.py | 0 ...erialized_views_with_sql_security_none.xml | 3 -- .../test_ignore_empty_sql_security.py | 45 ------------------- ...te_view_with_sql_security_option.reference | 2 + ...84_create_view_with_sql_security_option.sh | 39 ++++++++++++++++ 8 files changed, 48 insertions(+), 58 deletions(-) delete mode 100644 tests/integration/test_sql_security/__init__.py delete mode 100644 tests/integration/test_sql_security/configs/attach_materialized_views_with_sql_security_none.xml delete mode 100644 tests/integration/test_sql_security/test_ignore_empty_sql_security.py diff --git a/src/Core/ServerSettings.h b/src/Core/ServerSettings.h index cd616414016..f41c596282f 100644 --- a/src/Core/ServerSettings.h +++ b/src/Core/ServerSettings.h @@ -61,7 +61,6 @@ namespace DB M(UInt64, async_insert_threads, 16, "Maximum number of threads to actually parse and insert data in background. Zero means asynchronous mode is disabled", 0) \ M(Bool, async_insert_queue_flush_on_shutdown, true, "If true queue of asynchronous inserts is flushed on graceful shutdown", 0) \ M(Bool, ignore_empty_sql_security_in_create_view_query, true, "If true, ClickHouse doesn't write defaults for empty SQL security statement in CREATE VIEW queries. This setting is only necessary for the migration period and will become obsolete in 24.4", 0) \ - M(Bool, attach_materialized_views_with_sql_security_none, true, "If true, all materialized views loaded without SQL security statement (e.g. `ignore_empty_sql_security_in_create_view_query = false` or when loading old created views) will use SQL security NONE.", 0) \ \ M(UInt64, max_concurrent_queries, 0, "Maximum number of concurrently executed queries. Zero means unlimited.", 0) \ M(UInt64, max_concurrent_insert_queries, 0, "Maximum number of concurrently INSERT queries. Zero means unlimited.", 0) \ diff --git a/src/Processors/Transforms/buildPushingToViewsChain.cpp b/src/Processors/Transforms/buildPushingToViewsChain.cpp index aa5a1c0cc1a..91e43dc8134 100644 --- a/src/Processors/Transforms/buildPushingToViewsChain.cpp +++ b/src/Processors/Transforms/buildPushingToViewsChain.cpp @@ -361,7 +361,8 @@ std::optional generateViewChain( } InterpreterInsertQuery interpreter(nullptr, insert_context, false, false, false); - out = interpreter.buildChain(inner_table, inner_metadata_snapshot, insert_columns, thread_status_holder, view_counter_ms, !materialized_view->hasInnerTable()); + bool check_access = !materialized_view->hasInnerTable() && materialized_view->getInMemoryMetadataPtr()->sql_security_type != SQLSecurityType::INVOKER; + out = interpreter.buildChain(inner_table, inner_metadata_snapshot, insert_columns, thread_status_holder, view_counter_ms, check_access); if (interpreter.shouldAddSquashingFroStorage(inner_table)) { diff --git a/src/Storages/StorageMaterializedView.cpp b/src/Storages/StorageMaterializedView.cpp index 97e2cf6ef4d..4c3f90e23ed 100644 --- a/src/Storages/StorageMaterializedView.cpp +++ b/src/Storages/StorageMaterializedView.cpp @@ -98,20 +98,17 @@ StorageMaterializedView::StorageMaterializedView( local_context->getGlobalContext()); ASTPtr sql_security = query.sql_security; - if (!sql_security && query.attach && getContext()->getServerSettings().attach_materialized_views_with_sql_security_none) + if (!sql_security) { /// This allows materialized views to be loaded during startup with default SQL security for backward compatibility. - /// If ClickHouse loads an old materialized view created without SQL security, it will use the default `SQL SECURITY NONE` + /// If ClickHouse loads an old materialized view created without SQL security, it will use the default `SQL SECURITY INVOKER` sql_security = std::make_shared(); - InterpreterCreateQuery::processSQLSecurityOption(getContext(), sql_security->as(), true, true); + sql_security->as().type = SQLSecurityType::INVOKER; } if (sql_security) storage_metadata.setSQLSecurity(sql_security->as()); - if (storage_metadata.sql_security_type == SQLSecurityType::INVOKER) - throw Exception(ErrorCodes::QUERY_IS_NOT_SUPPORTED_IN_MATERIALIZED_VIEW, "SQL SECURITY INVOKER can't be specified for MATERIALIZED VIEW"); - if (!query.select) throw Exception(ErrorCodes::INCORRECT_QUERY, "SELECT query is not specified for {}", getName()); @@ -229,7 +226,7 @@ void StorageMaterializedView::read( auto storage_id = storage->getStorageID(); /// We don't need to check access if the inner table was created automatically. - if (!has_inner_table && !storage_id.empty()) + if (!has_inner_table && !storage_id.empty() && getInMemoryMetadataPtr()->sql_security_type != SQLSecurityType::INVOKER) context->checkAccess(AccessType::SELECT, storage_id, column_names); storage->read(query_plan, column_names, target_storage_snapshot, query_info, context, processed_stage, max_block_size, num_streams); @@ -278,7 +275,7 @@ SinkToStoragePtr StorageMaterializedView::write(const ASTPtr & query, const Stor auto storage_id = storage->getStorageID(); /// We don't need to check access if the inner table was created automatically. - if (!has_inner_table && !storage_id.empty()) + if (!has_inner_table && !storage_id.empty() && getInMemoryMetadataPtr()->sql_security_type != SQLSecurityType::INVOKER) { auto query_sample_block = InterpreterInsertQuery::getSampleBlock(query->as(), storage, metadata_snapshot, context); context->checkAccess(AccessType::INSERT, storage_id, query_sample_block.getNames()); diff --git a/tests/integration/test_sql_security/__init__.py b/tests/integration/test_sql_security/__init__.py deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/tests/integration/test_sql_security/configs/attach_materialized_views_with_sql_security_none.xml b/tests/integration/test_sql_security/configs/attach_materialized_views_with_sql_security_none.xml deleted file mode 100644 index 556d845d43e..00000000000 --- a/tests/integration/test_sql_security/configs/attach_materialized_views_with_sql_security_none.xml +++ /dev/null @@ -1,3 +0,0 @@ - - 0 - diff --git a/tests/integration/test_sql_security/test_ignore_empty_sql_security.py b/tests/integration/test_sql_security/test_ignore_empty_sql_security.py deleted file mode 100644 index c2e167700a2..00000000000 --- a/tests/integration/test_sql_security/test_ignore_empty_sql_security.py +++ /dev/null @@ -1,45 +0,0 @@ -#!/usr/bin/env python3 -import pytest -from helpers.cluster import ClickHouseCluster - -cluster = ClickHouseCluster(__file__) -node = cluster.add_instance( - "node1", - main_configs=["configs/attach_materialized_views_with_sql_security_none.xml"], - stay_alive=True, -) - - -@pytest.fixture(scope="module") -def started_cluster(): - try: - cluster.start() - - yield cluster - - finally: - cluster.shutdown() - - -def test_load_mv_with_security_none(started_cluster: ClickHouseCluster): - node.query("CREATE TABLE test_table (s String) ENGINE = MergeTree ORDER BY s") - node.query( - "CREATE MATERIALIZED VIEW test_mv_1 (s String) ENGINE = MergeTree ORDER BY s AS SELECT * FROM test_table" - ) - node.query("INSERT INTO test_table VALUES ('foo'), ('bar')") - - node.query("CREATE USER test_user") - node.query("GRANT SELECT ON test_mv_1 TO test_user") - - with pytest.raises(Exception, match="Not enough privileges"): - node.query("SELECT count() FROM test_mv_1", user="test_user") - - node.replace_in_config( - "/etc/clickhouse-server/config.d/ignore_empty_sql_security_in_create_view_query.xml", - "0", - "1", - ) - - node.restart_clickhouse() - - assert node.query("SELECT count() FROM test_mv_1", user="test_user") == "2\n" diff --git a/tests/queries/0_stateless/02884_create_view_with_sql_security_option.reference b/tests/queries/0_stateless/02884_create_view_with_sql_security_option.reference index 6d9d1f07ec2..9ba927fa201 100644 --- a/tests/queries/0_stateless/02884_create_view_with_sql_security_option.reference +++ b/tests/queries/0_stateless/02884_create_view_with_sql_security_option.reference @@ -32,3 +32,5 @@ OK 2 2 6 6 9 9 +1000 +1000 diff --git a/tests/queries/0_stateless/02884_create_view_with_sql_security_option.sh b/tests/queries/0_stateless/02884_create_view_with_sql_security_option.sh index bead7db8450..48d9a3e220b 100755 --- a/tests/queries/0_stateless/02884_create_view_with_sql_security_option.sh +++ b/tests/queries/0_stateless/02884_create_view_with_sql_security_option.sh @@ -222,4 +222,43 @@ EOF ${CLICKHOUSE_CLIENT} --user $user2 --query "SELECT * FROM $db.test_mv_row_2" +${CLICKHOUSE_CLIENT} --multiquery < Date: Fri, 3 May 2024 10:23:45 -0400 Subject: [PATCH 12/14] Fix issues --- src/Databases/DatabasesCommon.cpp | 15 ++++++++++----- src/Storages/StorageMaterializedView.cpp | 13 +++++++++++-- .../02884_create_view_with_sql_security_option.sh | 2 +- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/Databases/DatabasesCommon.cpp b/src/Databases/DatabasesCommon.cpp index fc75f8e44b9..d31581bc7fe 100644 --- a/src/Databases/DatabasesCommon.cpp +++ b/src/Databases/DatabasesCommon.cpp @@ -73,13 +73,18 @@ void applyMetadataChangesToCreateQuery(const ASTPtr & query, const StorageInMemo if (metadata.sql_security_type) { - auto new_sql_security = std::make_shared(); - new_sql_security->type = metadata.sql_security_type; + /// TODO: remove after we turn `ignore_empty_sql_security_in_create_view_query=false` + /// If materialized view has SQL SECURITY INVOKER we shouldn't dump it on disk. + if (!ast_create_query.is_materialized_view || metadata.sql_security_type != SQLSecurityType::INVOKER) + { + auto new_sql_security = std::make_shared(); + new_sql_security->type = metadata.sql_security_type; - if (metadata.definer) - new_sql_security->definer = std::make_shared(*metadata.definer); + if (metadata.definer) + new_sql_security->definer = std::make_shared(*metadata.definer); - ast_create_query.sql_security = std::move(new_sql_security); + ast_create_query.sql_security = std::move(new_sql_security); + } } /// MaterializedView, Dictionary are types of CREATE query without storage. diff --git a/src/Storages/StorageMaterializedView.cpp b/src/Storages/StorageMaterializedView.cpp index 4c3f90e23ed..2bf48e759da 100644 --- a/src/Storages/StorageMaterializedView.cpp +++ b/src/Storages/StorageMaterializedView.cpp @@ -98,6 +98,12 @@ StorageMaterializedView::StorageMaterializedView( local_context->getGlobalContext()); ASTPtr sql_security = query.sql_security; + + /// Materialized view doesn't support SQL SECURITY INVOKER. It's reserved type for backward compatibility + if (sql_security && sql_security->as().type == SQLSecurityType::INVOKER) + throw Exception(ErrorCodes::QUERY_IS_NOT_SUPPORTED_IN_MATERIALIZED_VIEW, "SQL SECURITY INVOKER can't be specified for MATERIALIZED VIEW"); + + // TODO: remove after we turn `ignore_empty_sql_security_in_create_view_query=false` if (!sql_security) { /// This allows materialized views to be loaded during startup with default SQL security for backward compatibility. @@ -106,8 +112,7 @@ StorageMaterializedView::StorageMaterializedView( sql_security->as().type = SQLSecurityType::INVOKER; } - if (sql_security) - storage_metadata.setSQLSecurity(sql_security->as()); + storage_metadata.setSQLSecurity(sql_security->as()); if (!query.select) throw Exception(ErrorCodes::INCORRECT_QUERY, "SELECT query is not specified for {}", getName()); @@ -225,6 +230,8 @@ void StorageMaterializedView::read( context->checkAccess(AccessType::SELECT, getInMemoryMetadataPtr()->select.select_table_id, column_names); auto storage_id = storage->getStorageID(); + + /// TODO: remove INVOKER check after we turn `ignore_empty_sql_security_in_create_view_query=false` /// We don't need to check access if the inner table was created automatically. if (!has_inner_table && !storage_id.empty() && getInMemoryMetadataPtr()->sql_security_type != SQLSecurityType::INVOKER) context->checkAccess(AccessType::SELECT, storage_id, column_names); @@ -274,6 +281,8 @@ SinkToStoragePtr StorageMaterializedView::write(const ASTPtr & query, const Stor auto metadata_snapshot = storage->getInMemoryMetadataPtr(); auto storage_id = storage->getStorageID(); + + /// TODO: remove INVOKER check after we turn `ignore_empty_sql_security_in_create_view_query=false` /// We don't need to check access if the inner table was created automatically. if (!has_inner_table && !storage_id.empty() && getInMemoryMetadataPtr()->sql_security_type != SQLSecurityType::INVOKER) { diff --git a/tests/queries/0_stateless/02884_create_view_with_sql_security_option.sh b/tests/queries/0_stateless/02884_create_view_with_sql_security_option.sh index 48d9a3e220b..9c9df120298 100755 --- a/tests/queries/0_stateless/02884_create_view_with_sql_security_option.sh +++ b/tests/queries/0_stateless/02884_create_view_with_sql_security_option.sh @@ -259,6 +259,6 @@ EOF ${CLICKHOUSE_CLIENT} --user $user3 --query "INSERT INTO $db.session_events SELECT * FROM generateRandom('clientId UUID, sessionId UUID, pageId UUID, timestamp DateTime, type Enum(\'type1\', \'type2\')', 1, 10, 2) LIMIT 1000" ${CLICKHOUSE_CLIENT} --user $user3 --query "SELECT count(*) FROM session_events" -${CLICKHOUSE_CLIENT} --user $user3 --query "SELECT count(*) FROM materialized_events" +${CLICKHOUSE_CLIENT} --query "SELECT count(*) FROM materialized_events" ${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS $user1, $user2, $user3"; From 8732855d27d577db11f80c046d66bf5805106d01 Mon Sep 17 00:00:00 2001 From: pufit Date: Fri, 3 May 2024 10:24:57 -0400 Subject: [PATCH 13/14] additional todo --- src/Processors/Transforms/buildPushingToViewsChain.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Processors/Transforms/buildPushingToViewsChain.cpp b/src/Processors/Transforms/buildPushingToViewsChain.cpp index 91e43dc8134..d5a3591eaa7 100644 --- a/src/Processors/Transforms/buildPushingToViewsChain.cpp +++ b/src/Processors/Transforms/buildPushingToViewsChain.cpp @@ -361,6 +361,8 @@ std::optional generateViewChain( } InterpreterInsertQuery interpreter(nullptr, insert_context, false, false, false); + + /// TODO: remove INVOKER check after we turn `ignore_empty_sql_security_in_create_view_query=false` bool check_access = !materialized_view->hasInnerTable() && materialized_view->getInMemoryMetadataPtr()->sql_security_type != SQLSecurityType::INVOKER; out = interpreter.buildChain(inner_table, inner_metadata_snapshot, insert_columns, thread_status_holder, view_counter_ms, check_access); From c63cc7eb0c4771be6c63f9e34d4183e2d7411cf7 Mon Sep 17 00:00:00 2001 From: pufit Date: Fri, 3 May 2024 16:50:49 -0400 Subject: [PATCH 14/14] better --- src/Databases/DatabasesCommon.cpp | 15 ++++------- .../Transforms/buildPushingToViewsChain.cpp | 4 +-- src/Storages/StorageMaterializedView.cpp | 26 ++++++------------- 3 files changed, 15 insertions(+), 30 deletions(-) diff --git a/src/Databases/DatabasesCommon.cpp b/src/Databases/DatabasesCommon.cpp index d31581bc7fe..fc75f8e44b9 100644 --- a/src/Databases/DatabasesCommon.cpp +++ b/src/Databases/DatabasesCommon.cpp @@ -73,18 +73,13 @@ void applyMetadataChangesToCreateQuery(const ASTPtr & query, const StorageInMemo if (metadata.sql_security_type) { - /// TODO: remove after we turn `ignore_empty_sql_security_in_create_view_query=false` - /// If materialized view has SQL SECURITY INVOKER we shouldn't dump it on disk. - if (!ast_create_query.is_materialized_view || metadata.sql_security_type != SQLSecurityType::INVOKER) - { - auto new_sql_security = std::make_shared(); - new_sql_security->type = metadata.sql_security_type; + auto new_sql_security = std::make_shared(); + new_sql_security->type = metadata.sql_security_type; - if (metadata.definer) - new_sql_security->definer = std::make_shared(*metadata.definer); + if (metadata.definer) + new_sql_security->definer = std::make_shared(*metadata.definer); - ast_create_query.sql_security = std::move(new_sql_security); - } + ast_create_query.sql_security = std::move(new_sql_security); } /// MaterializedView, Dictionary are types of CREATE query without storage. diff --git a/src/Processors/Transforms/buildPushingToViewsChain.cpp b/src/Processors/Transforms/buildPushingToViewsChain.cpp index d5a3591eaa7..5e8ecdca95e 100644 --- a/src/Processors/Transforms/buildPushingToViewsChain.cpp +++ b/src/Processors/Transforms/buildPushingToViewsChain.cpp @@ -362,8 +362,8 @@ std::optional generateViewChain( InterpreterInsertQuery interpreter(nullptr, insert_context, false, false, false); - /// TODO: remove INVOKER check after we turn `ignore_empty_sql_security_in_create_view_query=false` - bool check_access = !materialized_view->hasInnerTable() && materialized_view->getInMemoryMetadataPtr()->sql_security_type != SQLSecurityType::INVOKER; + /// TODO: remove sql_security_type check after we turn `ignore_empty_sql_security_in_create_view_query=false` + bool check_access = !materialized_view->hasInnerTable() && materialized_view->getInMemoryMetadataPtr()->sql_security_type; out = interpreter.buildChain(inner_table, inner_metadata_snapshot, insert_columns, thread_status_holder, view_counter_ms, check_access); if (interpreter.shouldAddSquashingFroStorage(inner_table)) diff --git a/src/Storages/StorageMaterializedView.cpp b/src/Storages/StorageMaterializedView.cpp index 2bf48e759da..7e27b1d5005 100644 --- a/src/Storages/StorageMaterializedView.cpp +++ b/src/Storages/StorageMaterializedView.cpp @@ -97,23 +97,13 @@ StorageMaterializedView::StorageMaterializedView( storage_metadata.columns, local_context->getGlobalContext()); - ASTPtr sql_security = query.sql_security; + if (query.sql_security) + storage_metadata.setSQLSecurity(query.sql_security->as()); - /// Materialized view doesn't support SQL SECURITY INVOKER. It's reserved type for backward compatibility - if (sql_security && sql_security->as().type == SQLSecurityType::INVOKER) + /// Materialized view doesn't support SQL SECURITY INVOKER. + if (storage_metadata.sql_security_type == SQLSecurityType::INVOKER) throw Exception(ErrorCodes::QUERY_IS_NOT_SUPPORTED_IN_MATERIALIZED_VIEW, "SQL SECURITY INVOKER can't be specified for MATERIALIZED VIEW"); - // TODO: remove after we turn `ignore_empty_sql_security_in_create_view_query=false` - if (!sql_security) - { - /// This allows materialized views to be loaded during startup with default SQL security for backward compatibility. - /// If ClickHouse loads an old materialized view created without SQL security, it will use the default `SQL SECURITY INVOKER` - sql_security = std::make_shared(); - sql_security->as().type = SQLSecurityType::INVOKER; - } - - storage_metadata.setSQLSecurity(sql_security->as()); - if (!query.select) throw Exception(ErrorCodes::INCORRECT_QUERY, "SELECT query is not specified for {}", getName()); @@ -231,9 +221,9 @@ void StorageMaterializedView::read( auto storage_id = storage->getStorageID(); - /// TODO: remove INVOKER check after we turn `ignore_empty_sql_security_in_create_view_query=false` + /// TODO: remove sql_security_type check after we turn `ignore_empty_sql_security_in_create_view_query=false` /// We don't need to check access if the inner table was created automatically. - if (!has_inner_table && !storage_id.empty() && getInMemoryMetadataPtr()->sql_security_type != SQLSecurityType::INVOKER) + if (!has_inner_table && !storage_id.empty() && getInMemoryMetadataPtr()->sql_security_type) context->checkAccess(AccessType::SELECT, storage_id, column_names); storage->read(query_plan, column_names, target_storage_snapshot, query_info, context, processed_stage, max_block_size, num_streams); @@ -282,9 +272,9 @@ SinkToStoragePtr StorageMaterializedView::write(const ASTPtr & query, const Stor auto storage_id = storage->getStorageID(); - /// TODO: remove INVOKER check after we turn `ignore_empty_sql_security_in_create_view_query=false` + /// TODO: remove sql_security_type check after we turn `ignore_empty_sql_security_in_create_view_query=false` /// We don't need to check access if the inner table was created automatically. - if (!has_inner_table && !storage_id.empty() && getInMemoryMetadataPtr()->sql_security_type != SQLSecurityType::INVOKER) + if (!has_inner_table && !storage_id.empty() && getInMemoryMetadataPtr()->sql_security_type) { auto query_sample_block = InterpreterInsertQuery::getSampleBlock(query->as(), storage, metadata_snapshot, context); context->checkAccess(AccessType::INSERT, storage_id, query_sample_block.getNames());