diff --git a/src/Interpreters/InterpreterSelectQuery.cpp b/src/Interpreters/InterpreterSelectQuery.cpp index 0b899fb8752..dd9df3f36a0 100644 --- a/src/Interpreters/InterpreterSelectQuery.cpp +++ b/src/Interpreters/InterpreterSelectQuery.cpp @@ -211,6 +211,18 @@ static void rewriteMultipleJoins(ASTPtr & query, const TablesWithColumns & table JoinToSubqueryTransformVisitor(join_to_subs_data).visit(query); } +/// Returns true if we should ignore quotas and limits for a specified table in the system database. +static bool shouldIgnoreQuotaAndLimits(const StorageID & table_id) +{ + if (table_id.database_name == DatabaseCatalog::SYSTEM_DATABASE) + { + static const boost::container::flat_set tables_ignoring_quota{"quotas", "quota_limits", "quota_usage", "quotas_usage", "one"}; + if (tables_ignoring_quota.count(table_id.table_name)) + return true; + } + return false; +} + InterpreterSelectQuery::InterpreterSelectQuery( const ASTPtr & query_ptr_, const Context & context_, @@ -255,14 +267,18 @@ InterpreterSelectQuery::InterpreterSelectQuery( JoinedTables joined_tables(getSubqueryContext(*context), getSelectQuery()); + bool got_storage_from_query = false; if (!has_input && !storage) + { storage = joined_tables.getLeftTableStorage(); + got_storage_from_query = true; + } if (storage) { table_lock = storage->lockForShare(context->getInitialQueryId(), context->getSettingsRef().lock_acquire_timeout); table_id = storage->getStorageID(); - if (metadata_snapshot == nullptr) + if (!metadata_snapshot) metadata_snapshot = storage->getInMemoryMetadataPtr(); } @@ -280,9 +296,10 @@ InterpreterSelectQuery::InterpreterSelectQuery( if (storage && joined_tables.isLeftTableSubquery()) { /// Rewritten with subquery. Free storage locks here. - storage = {}; + storage = nullptr; table_lock.reset(); table_id = StorageID::createEmpty(); + metadata_snapshot = nullptr; } } @@ -445,16 +462,14 @@ InterpreterSelectQuery::InterpreterSelectQuery( if (query.prewhere() && !query.where()) analysis_result.prewhere_info->need_filter = true; - const StorageID & left_table_id = joined_tables.leftTableID(); - - if (left_table_id) - context->checkAccess(AccessType::SELECT, left_table_id, required_columns); - - /// Remove limits for some tables in the `system` database. - if (left_table_id.database_name == "system") + if (table_id && got_storage_from_query && !joined_tables.isLeftTableFunction()) { - static const boost::container::flat_set system_tables_ignoring_quota{"quotas", "quota_limits", "quota_usage", "quotas_usage", "one"}; - if (system_tables_ignoring_quota.count(left_table_id.table_name)) + /// The current user should have the SELECT privilege. + /// If this table_id is for a table function we don't check access rights here because in this case they have been already checked in ITableFunction::execute(). + context->checkAccess(AccessType::SELECT, table_id, required_columns); + + /// Remove limits for some tables in the `system` database. + if (shouldIgnoreQuotaAndLimits(table_id) && (joined_tables.tablesCount() <= 1)) { options.ignore_quota = true; options.ignore_limits = true; diff --git a/src/Interpreters/JoinedTables.cpp b/src/Interpreters/JoinedTables.cpp index c0511122c1e..17d7949e478 100644 --- a/src/Interpreters/JoinedTables.cpp +++ b/src/Interpreters/JoinedTables.cpp @@ -161,6 +161,7 @@ StoragePtr JoinedTables::getLeftTableStorage() if (isLeftTableFunction()) return context.getQueryContext().executeTableFunction(left_table_expression); + StorageID table_id = StorageID::createEmpty(); if (left_db_and_table) { table_id = context.resolveStorageID(StorageID(left_db_and_table->database, left_db_and_table->table, left_db_and_table->uuid)); diff --git a/src/Interpreters/JoinedTables.h b/src/Interpreters/JoinedTables.h index 1e787ee4a65..812808fed61 100644 --- a/src/Interpreters/JoinedTables.h +++ b/src/Interpreters/JoinedTables.h @@ -43,8 +43,6 @@ public: bool isLeftTableFunction() const; size_t tablesCount() const { return table_expressions.size(); } - const StorageID & leftTableID() const { return table_id; } - void rewriteDistributedInAndJoins(ASTPtr & query); std::unique_ptr makeLeftTableSubquery(const SelectQueryOptions & select_options); @@ -57,9 +55,6 @@ private: /// Legacy (duplicated left table values) ASTPtr left_table_expression; std::optional left_db_and_table; - - /// left_db_and_table or 'system.one' - StorageID table_id = StorageID::createEmpty(); }; } diff --git a/tests/integration/test_select_access_rights/__init__.py b/tests/integration/test_select_access_rights/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/integration/test_select_access_rights/test.py b/tests/integration/test_select_access_rights/test.py new file mode 100644 index 00000000000..ccea77d6fb7 --- /dev/null +++ b/tests/integration/test_select_access_rights/test.py @@ -0,0 +1,157 @@ +import pytest +from helpers.cluster import ClickHouseCluster +from helpers.test_tools import TSV + +cluster = ClickHouseCluster(__file__) +instance = cluster.add_instance('instance') + + +@pytest.fixture(scope="module", autouse=True) +def started_cluster(): + try: + cluster.start() + yield cluster + finally: + cluster.shutdown() + + +@pytest.fixture(autouse=True) +def cleanup_after_test(): + instance.query("CREATE USER OR REPLACE A") + yield + instance.query("DROP TABLE IF EXISTS table1") + instance.query("DROP TABLE IF EXISTS table2") + + +def test_select_single_column(): + instance.query("CREATE TABLE table1(d DATE, a String, b UInt8) ENGINE = MergeTree ORDER BY d") + + select_query = "SELECT a FROM table1" + assert "it's necessary to have grant SELECT(a) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(a) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + instance.query("REVOKE SELECT(a) ON default.table1 FROM A") + assert "it's necessary to have grant SELECT(a) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + +def test_select_single_column_with_table_grant(): + instance.query("CREATE TABLE table1(d DATE, a String, b UInt8) ENGINE = MergeTree ORDER BY d") + + select_query = "SELECT a FROM table1" + assert "it's necessary to have grant SELECT(a) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + instance.query("REVOKE SELECT(a) ON default.table1 FROM A") + assert "it's necessary to have grant SELECT(a) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + +def test_select_all_columns(): + instance.query("CREATE TABLE table1(d DATE, a String, b UInt8) ENGINE = MergeTree ORDER BY d") + + select_query = "SELECT * FROM table1" + assert "it's necessary to have grant SELECT(d, a, b) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(d) ON default.table1 TO A") + assert "it's necessary to have grant SELECT(d, a, b) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(a) ON default.table1 TO A") + assert "it's necessary to have grant SELECT(d, a, b) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(b) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + +def test_select_all_columns_with_table_grant(): + instance.query("CREATE TABLE table1(d DATE, a String, b UInt8) ENGINE = MergeTree ORDER BY d") + + select_query = "SELECT * FROM table1" + assert "it's necessary to have grant SELECT(d, a, b) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + +def test_alias(): + instance.query("CREATE TABLE table1(x Int32, y Int32) ENGINE = MergeTree ORDER BY tuple()") + + select_query = "SELECT x, y, x + y AS s FROM table1" + assert "it's necessary to have grant SELECT(x, y) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(x, y) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + +def test_alias_columns(): + instance.query("CREATE TABLE table1(x Int32, y Int32, s Int32 ALIAS x + y) ENGINE = MergeTree ORDER BY tuple()") + + select_query = "SELECT * FROM table1" + assert "it's necessary to have grant SELECT(x, y) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(x,y) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + select_query = "SELECT s FROM table1" + assert "it's necessary to have grant SELECT(s) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(s) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + instance.query("REVOKE SELECT(x,y) ON default.table1 FROM A") + assert instance.query(select_query, user = 'A') == "" + + +def test_materialized_columns(): + instance.query("CREATE TABLE table1(x Int32, y Int32, p Int32 MATERIALIZED x * y) ENGINE = MergeTree ORDER BY tuple()") + + select_query = "SELECT * FROM table1" + assert "it's necessary to have grant SELECT(x, y) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(x,y) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + select_query = "SELECT p FROM table1" + assert "it's necessary to have grant SELECT(p) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(p) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + instance.query("REVOKE SELECT(x,y) ON default.table1 FROM A") + assert instance.query(select_query, user = 'A') == "" + + +def test_select_join(): + instance.query("CREATE TABLE table1(d DATE, a String, b UInt8) ENGINE = MergeTree ORDER BY d") + instance.query("CREATE TABLE table2(d DATE, x UInt32, y UInt8) ENGINE = MergeTree ORDER BY d") + + select_query = "SELECT * FROM table1 JOIN table2 USING(d)" + assert "it's necessary to have grant SELECT(d, x, y) ON default.table2" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(d, x, y) ON default.table2 TO A") + assert "it's necessary to have grant SELECT(d, a, b) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(d, a, b) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "" + + instance.query("REVOKE SELECT ON default.table2 FROM A") + assert "it's necessary to have grant SELECT(d, x, y) ON default.table2" in instance.query_and_get_error(select_query, user = 'A') + + +def test_select_union(): + instance.query("CREATE TABLE table1(a String, b UInt8) ENGINE = MergeTree ORDER BY tuple()") + instance.query("CREATE TABLE table2(a String, b UInt8) ENGINE = MergeTree ORDER BY tuple()") + + select_query = "SELECT * FROM table1 UNION ALL SELECT * FROM table2" + assert "it's necessary to have grant SELECT(a, b) ON default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(a, b) ON default.table1 TO A") + assert "it's necessary to have grant SELECT(a, b) ON default.table2" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(a, b) ON default.table2 TO A") + assert instance.query(select_query, user = 'A') == "" + + instance.query("REVOKE SELECT ON default.table1 FROM A") + assert "it's necessary to have grant SELECT(a, b) ON default.table1" in instance.query_and_get_error(select_query, user = 'A')