From 235ea220ec80b88182dde9206ded1240901d982c Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 18 Dec 2020 23:09:39 +0300 Subject: [PATCH] SELECT count() FROM table now requires at least one column to be accessible in the table. --- src/Interpreters/InterpreterSelectQuery.cpp | 34 ++++++++++++++++++- src/Interpreters/TreeRewriter.cpp | 6 ++-- src/Interpreters/TreeRewriter.h | 7 ++++ .../test_select_access_rights/test.py | 22 ++++++++++++ 4 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/Interpreters/InterpreterSelectQuery.cpp b/src/Interpreters/InterpreterSelectQuery.cpp index dd9df3f36a0..8cc4b08c61b 100644 --- a/src/Interpreters/InterpreterSelectQuery.cpp +++ b/src/Interpreters/InterpreterSelectQuery.cpp @@ -13,6 +13,7 @@ #include #include +#include #include @@ -99,6 +100,7 @@ namespace ErrorCodes extern const int PARAMETER_OUT_OF_BOUND; extern const int INVALID_LIMIT_EXPRESSION; extern const int INVALID_WITH_FILL_EXPRESSION; + extern const int ACCESS_DENIED; } /// Assumes `storage` is set and the table filter (row-level security) is not empty. @@ -211,6 +213,36 @@ static void rewriteMultipleJoins(ASTPtr & query, const TablesWithColumns & table JoinToSubqueryTransformVisitor(join_to_subs_data).visit(query); } +/// Checks that the current user has the SELECT privilege. +static void checkAccessRightsForSelect( + const Context & context, + const StorageID & table_id, + const StorageMetadataPtr & table_metadata, + const Strings & required_columns, + const TreeRewriterResult & syntax_analyzer_result) +{ + if (!syntax_analyzer_result.has_explicit_columns && table_metadata && !table_metadata->getColumns().empty()) + { + /// For a trivial query like "SELECT count() FROM table" access is granted if at least + /// one column is accessible. + /// In this case just checking access for `required_columns` doesn't work correctly + /// because `required_columns` will contain the name of a column of minimum size (see TreeRewriterResult::collectUsedColumns()) + /// which is probably not the same column as the column the current user has access to. + auto access = context.getAccess(); + for (const auto & column : table_metadata->getColumns()) + { + if (access->isGranted(AccessType::SELECT, table_id.database_name, table_id.table_name, column.name)) + return; + } + throw Exception(context.getUserName() + " : Not enough privileges. " + "To execute this query it's necessary to have grant SELECT for at least one column on " + table_id.getFullTableName(), + ErrorCodes::ACCESS_DENIED); + } + + /// General check. + context.checkAccess(AccessType::SELECT, table_id, required_columns); +} + /// Returns true if we should ignore quotas and limits for a specified table in the system database. static bool shouldIgnoreQuotaAndLimits(const StorageID & table_id) { @@ -466,7 +498,7 @@ InterpreterSelectQuery::InterpreterSelectQuery( { /// 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); + checkAccessRightsForSelect(*context, table_id, metadata_snapshot, required_columns, *syntax_analyzer_result); /// Remove limits for some tables in the `system` database. if (shouldIgnoreQuotaAndLimits(table_id) && (joined_tables.tablesCount() <= 1)) diff --git a/src/Interpreters/TreeRewriter.cpp b/src/Interpreters/TreeRewriter.cpp index 2b801500958..29a6b102fc9 100644 --- a/src/Interpreters/TreeRewriter.cpp +++ b/src/Interpreters/TreeRewriter.cpp @@ -588,11 +588,13 @@ void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select required.insert(column_name_type.name); } - /// You need to read at least one column to find the number of rows. - if (is_select && required.empty()) + /// Figure out if we're able to use the trivial count optimization. + has_explicit_columns = !required.empty(); + if (is_select && !has_explicit_columns) { optimize_trivial_count = true; + /// You need to read at least one column to find the number of rows. /// We will find a column with minimum . /// Because it is the column that is cheapest to read. struct ColumnSizeTuple diff --git a/src/Interpreters/TreeRewriter.h b/src/Interpreters/TreeRewriter.h index d9f98ee40bd..35f82d2505f 100644 --- a/src/Interpreters/TreeRewriter.h +++ b/src/Interpreters/TreeRewriter.h @@ -53,6 +53,13 @@ struct TreeRewriterResult /// Predicate optimizer overrides the sub queries bool rewrite_subqueries = false; + /// Whether the query contains explicit columns like "SELECT column1 + column2 FROM table1". + /// Queries like "SELECT count() FROM table1", "SELECT 1" don't contain explicit columns. + bool has_explicit_columns = false; + + /// Whether it's possible to use the trivial count optimization, + /// i.e. use a fast call of IStorage::totalRows() (or IStorage::totalRowsByPartitionPredicate()) + /// instead of actual retrieving columns and counting rows. bool optimize_trivial_count = false; /// Cache isRemote() call for storage, because it may be too heavy. diff --git a/tests/integration/test_select_access_rights/test.py b/tests/integration/test_select_access_rights/test.py index ccea77d6fb7..14eae1700d0 100644 --- a/tests/integration/test_select_access_rights/test.py +++ b/tests/integration/test_select_access_rights/test.py @@ -155,3 +155,25 @@ def test_select_union(): 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') + + +def test_select_count(): + instance.query("CREATE TABLE table1(x String, y UInt8) ENGINE = MergeTree ORDER BY tuple()") + + select_query = "SELECT count() FROM table1" + assert "it's necessary to have grant SELECT for at least one column on default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(x) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "0\n" + + instance.query("REVOKE SELECT(x) ON default.table1 FROM A") + assert "it's necessary to have grant SELECT for at least one column on default.table1" in instance.query_and_get_error(select_query, user = 'A') + + instance.query("GRANT SELECT(y) ON default.table1 TO A") + assert instance.query(select_query, user = 'A') == "0\n" + + instance.query("REVOKE SELECT(y) ON default.table1 FROM A") + assert "it's necessary to have grant SELECT for at least one column 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') == "0\n"