From f1e9e3dec0d154b41fab9699369720377317a281 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 6 Mar 2020 20:13:28 +0300 Subject: [PATCH] Improve access rights: KILL_MUTATION deleted, rights for corresponding ALTER commands are checked instead. --- dbms/src/Access/AccessFlags.h | 4 +- dbms/src/Access/AccessRightsContext.cpp | 2 +- dbms/src/Access/AccessType.h | 4 - .../Interpreters/InterpreterAlterQuery.cpp | 301 +++++++++--------- dbms/src/Interpreters/InterpreterAlterQuery.h | 3 + .../InterpreterKillQueryQuery.cpp | 35 +- .../0_stateless/00834_kill_mutation.reference | 4 +- ...ll_mutation_replicated_zookeeper.reference | 4 +- 8 files changed, 189 insertions(+), 168 deletions(-) diff --git a/dbms/src/Access/AccessFlags.h b/dbms/src/Access/AccessFlags.h index 2cf59b4886d..61a160fc69a 100644 --- a/dbms/src/Access/AccessFlags.h +++ b/dbms/src/Access/AccessFlags.h @@ -349,9 +349,7 @@ private: ext::push_back(all, std::move(optimize)); auto kill_query = std::make_unique("KILL QUERY", next_flag++, GLOBAL); - auto kill_mutation = std::make_unique("KILL MUTATION", next_flag++, TABLE); - auto kill = std::make_unique("KILL", std::move(kill_query), std::move(kill_mutation)); - ext::push_back(all, std::move(kill)); + ext::push_back(all, std::move(kill_query)); auto create_user = std::make_unique("CREATE USER", next_flag++, GLOBAL); auto alter_user = std::make_unique("ALTER USER", next_flag++, GLOBAL); diff --git a/dbms/src/Access/AccessRightsContext.cpp b/dbms/src/Access/AccessRightsContext.cpp index f847e353a41..6eff1f050b3 100644 --- a/dbms/src/Access/AccessRightsContext.cpp +++ b/dbms/src/Access/AccessRightsContext.cpp @@ -436,7 +436,7 @@ boost::shared_ptr AccessRightsContext::calculateResultAccess | AccessType::DROP_ROLE | AccessType::DROP_POLICY | AccessType::DROP_QUOTA | AccessType::ROLE_ADMIN; if (readonly_) - result.revoke(write_table_access | all_dcl | AccessType::SYSTEM | AccessType::KILL); + result.revoke(write_table_access | all_dcl | AccessType::SYSTEM | AccessType::KILL_QUERY); if (readonly_ || !allow_ddl_) result.revoke(table_and_dictionary_ddl); diff --git a/dbms/src/Access/AccessType.h b/dbms/src/Access/AccessType.h index ea51d66451a..4084d180013 100644 --- a/dbms/src/Access/AccessType.h +++ b/dbms/src/Access/AccessType.h @@ -79,8 +79,6 @@ enum class AccessType OPTIMIZE, /// allows to execute OPTIMIZE TABLE KILL_QUERY, /// allows to kill a query started by another user (anyone can kill his own queries) - KILL_MUTATION, /// allows to kill a mutation - KILL, /// allows to execute KILL {MUTATION|QUERY} CREATE_USER, ALTER_USER, @@ -244,8 +242,6 @@ namespace impl ACCESS_TYPE_TO_KEYWORD_CASE(OPTIMIZE); ACCESS_TYPE_TO_KEYWORD_CASE(KILL_QUERY); - ACCESS_TYPE_TO_KEYWORD_CASE(KILL_MUTATION); - ACCESS_TYPE_TO_KEYWORD_CASE(KILL); ACCESS_TYPE_TO_KEYWORD_CASE(CREATE_USER); ACCESS_TYPE_TO_KEYWORD_CASE(ALTER_USER); diff --git a/dbms/src/Interpreters/InterpreterAlterQuery.cpp b/dbms/src/Interpreters/InterpreterAlterQuery.cpp index 5462fc16a81..315527765ef 100644 --- a/dbms/src/Interpreters/InterpreterAlterQuery.cpp +++ b/dbms/src/Interpreters/InterpreterAlterQuery.cpp @@ -13,7 +13,7 @@ #include #include #include - +#include #include @@ -125,155 +125,162 @@ AccessRightsElements InterpreterAlterQuery::getRequiredAccess() const { AccessRightsElements required_access; const auto & alter = query_ptr->as(); - for (ASTAlterCommand * command_ast : alter.command_list->commands) - { - auto column_name = [&]() -> String { return getIdentifierName(command_ast->column); }; - auto column_name_from_col_decl = [&]() -> std::string_view { return command_ast->col_decl->as().name; }; - auto column_names_from_update_assignments = [&]() -> std::vector - { - std::vector column_names; - for (const ASTPtr & assignment_ast : command_ast->update_assignments->children) - column_names.emplace_back(assignment_ast->as().column_name); - return column_names; - }; + for (ASTAlterCommand * command : alter.command_list->commands) + boost::range::push_back(required_access, getRequiredAccessForCommand(*command, alter.database, alter.table)); + return required_access; +} - switch (command_ast->type) + +AccessRightsElements InterpreterAlterQuery::getRequiredAccessForCommand(const ASTAlterCommand & command, const String & database, const String & table) +{ + AccessRightsElements required_access; + + auto column_name = [&]() -> String { return getIdentifierName(command.column); }; + auto column_name_from_col_decl = [&]() -> std::string_view { return command.col_decl->as().name; }; + auto column_names_from_update_assignments = [&]() -> std::vector + { + std::vector column_names; + for (const ASTPtr & assignment_ast : command.update_assignments->children) + column_names.emplace_back(assignment_ast->as().column_name); + return column_names; + }; + + switch (command.type) + { + case ASTAlterCommand::UPDATE: { - case ASTAlterCommand::UPDATE: - { - required_access.emplace_back(AccessType::UPDATE, alter.database, alter.table, column_names_from_update_assignments()); - break; - } - case ASTAlterCommand::DELETE: - { - required_access.emplace_back(AccessType::DELETE, alter.database, alter.table); - break; - } - case ASTAlterCommand::ADD_COLUMN: - { - required_access.emplace_back(AccessType::ADD_COLUMN, alter.database, alter.table, column_name_from_col_decl()); - break; - } - case ASTAlterCommand::DROP_COLUMN: - { - if (command_ast->clear_column) - required_access.emplace_back(AccessType::CLEAR_COLUMN, alter.database, alter.table, column_name()); - else - required_access.emplace_back(AccessType::DROP_COLUMN, alter.database, alter.table, column_name()); - break; - } - case ASTAlterCommand::MODIFY_COLUMN: - { - required_access.emplace_back(AccessType::MODIFY_COLUMN, alter.database, alter.table, column_name_from_col_decl()); - break; - } - case ASTAlterCommand::COMMENT_COLUMN: - { - required_access.emplace_back(AccessType::COMMENT_COLUMN, alter.database, alter.table, column_name()); - break; - } - case ASTAlterCommand::MODIFY_ORDER_BY: - { - required_access.emplace_back(AccessType::ALTER_ORDER_BY, alter.database, alter.table); - break; - } - case ASTAlterCommand::ADD_INDEX: - { - required_access.emplace_back(AccessType::ADD_INDEX, alter.database, alter.table); - break; - } - case ASTAlterCommand::DROP_INDEX: - { - if (command_ast->clear_index) - required_access.emplace_back(AccessType::CLEAR_INDEX, alter.database, alter.table); - else - required_access.emplace_back(AccessType::DROP_INDEX, alter.database, alter.table); - break; - } - case ASTAlterCommand::MATERIALIZE_INDEX: - { - required_access.emplace_back(AccessType::MATERIALIZE_INDEX, alter.database, alter.table); - break; - } - case ASTAlterCommand::ADD_CONSTRAINT: - { - required_access.emplace_back(AccessType::ADD_CONSTRAINT, alter.database, alter.table); - break; - } - case ASTAlterCommand::DROP_CONSTRAINT: - { - required_access.emplace_back(AccessType::DROP_CONSTRAINT, alter.database, alter.table); - break; - } - case ASTAlterCommand::MODIFY_TTL: - { - required_access.emplace_back(AccessType::MODIFY_TTL, alter.database, alter.table); - break; - } - case ASTAlterCommand::MATERIALIZE_TTL: - { - required_access.emplace_back(AccessType::MATERIALIZE_TTL, alter.database, alter.table); - break; - } - case ASTAlterCommand::MODIFY_SETTING: - { - required_access.emplace_back(AccessType::MODIFY_SETTING, alter.database, alter.table); - break; - } - case ASTAlterCommand::ATTACH_PARTITION: - { - required_access.emplace_back(AccessType::INSERT, alter.database, alter.table); - break; - } - case ASTAlterCommand::DROP_PARTITION: [[fallthrough]]; - case ASTAlterCommand::DROP_DETACHED_PARTITION: - { - required_access.emplace_back(AccessType::DELETE, alter.database, alter.table); - break; - } - case ASTAlterCommand::MOVE_PARTITION: - { - if ((command_ast->move_destination_type == PartDestinationType::DISK) - || (command_ast->move_destination_type == PartDestinationType::VOLUME)) - { - required_access.emplace_back(AccessType::MOVE_PARTITION, alter.database, alter.table); - } - else if (command_ast->move_destination_type == PartDestinationType::TABLE) - { - required_access.emplace_back(AccessType::SELECT | AccessType::DELETE, alter.database, alter.table); - required_access.emplace_back(AccessType::INSERT, command_ast->to_database, command_ast->to_table); - } - break; - } - case ASTAlterCommand::REPLACE_PARTITION: - { - required_access.emplace_back(AccessType::SELECT, command_ast->from_database, command_ast->from_table); - required_access.emplace_back(AccessType::DELETE | AccessType::INSERT, alter.database, alter.table); - break; - } - case ASTAlterCommand::FETCH_PARTITION: - { - required_access.emplace_back(AccessType::FETCH_PARTITION, alter.database, alter.table); - break; - } - case ASTAlterCommand::FREEZE_PARTITION: [[fallthrough]]; - case ASTAlterCommand::FREEZE_ALL: - { - required_access.emplace_back(AccessType::FREEZE_PARTITION, alter.database, alter.table); - break; - } - case ASTAlterCommand::MODIFY_QUERY: - { - required_access.emplace_back(AccessType::MODIFY_VIEW_QUERY, alter.database, alter.table); - break; - } - case ASTAlterCommand::LIVE_VIEW_REFRESH: - { - required_access.emplace_back(AccessType::REFRESH_VIEW, alter.database, alter.table); - break; - } - case ASTAlterCommand::NO_TYPE: break; + required_access.emplace_back(AccessType::UPDATE, database, table, column_names_from_update_assignments()); + break; } + case ASTAlterCommand::DELETE: + { + required_access.emplace_back(AccessType::DELETE, database, table); + break; + } + case ASTAlterCommand::ADD_COLUMN: + { + required_access.emplace_back(AccessType::ADD_COLUMN, database, table, column_name_from_col_decl()); + break; + } + case ASTAlterCommand::DROP_COLUMN: + { + if (command.clear_column) + required_access.emplace_back(AccessType::CLEAR_COLUMN, database, table, column_name()); + else + required_access.emplace_back(AccessType::DROP_COLUMN, database, table, column_name()); + break; + } + case ASTAlterCommand::MODIFY_COLUMN: + { + required_access.emplace_back(AccessType::MODIFY_COLUMN, database, table, column_name_from_col_decl()); + break; + } + case ASTAlterCommand::COMMENT_COLUMN: + { + required_access.emplace_back(AccessType::COMMENT_COLUMN, database, table, column_name()); + break; + } + case ASTAlterCommand::MODIFY_ORDER_BY: + { + required_access.emplace_back(AccessType::ALTER_ORDER_BY, database, table); + break; + } + case ASTAlterCommand::ADD_INDEX: + { + required_access.emplace_back(AccessType::ADD_INDEX, database, table); + break; + } + case ASTAlterCommand::DROP_INDEX: + { + if (command.clear_index) + required_access.emplace_back(AccessType::CLEAR_INDEX, database, table); + else + required_access.emplace_back(AccessType::DROP_INDEX, database, table); + break; + } + case ASTAlterCommand::MATERIALIZE_INDEX: + { + required_access.emplace_back(AccessType::MATERIALIZE_INDEX, database, table); + break; + } + case ASTAlterCommand::ADD_CONSTRAINT: + { + required_access.emplace_back(AccessType::ADD_CONSTRAINT, database, table); + break; + } + case ASTAlterCommand::DROP_CONSTRAINT: + { + required_access.emplace_back(AccessType::DROP_CONSTRAINT, database, table); + break; + } + case ASTAlterCommand::MODIFY_TTL: + { + required_access.emplace_back(AccessType::MODIFY_TTL, database, table); + break; + } + case ASTAlterCommand::MATERIALIZE_TTL: + { + required_access.emplace_back(AccessType::MATERIALIZE_TTL, database, table); + break; + } + case ASTAlterCommand::MODIFY_SETTING: + { + required_access.emplace_back(AccessType::MODIFY_SETTING, database, table); + break; + } + case ASTAlterCommand::ATTACH_PARTITION: + { + required_access.emplace_back(AccessType::INSERT, database, table); + break; + } + case ASTAlterCommand::DROP_PARTITION: [[fallthrough]]; + case ASTAlterCommand::DROP_DETACHED_PARTITION: + { + required_access.emplace_back(AccessType::DELETE, database, table); + break; + } + case ASTAlterCommand::MOVE_PARTITION: + { + if ((command.move_destination_type == PartDestinationType::DISK) + || (command.move_destination_type == PartDestinationType::VOLUME)) + { + required_access.emplace_back(AccessType::MOVE_PARTITION, database, table); + } + else if (command.move_destination_type == PartDestinationType::TABLE) + { + required_access.emplace_back(AccessType::SELECT | AccessType::DELETE, database, table); + required_access.emplace_back(AccessType::INSERT, command.to_database, command.to_table); + } + break; + } + case ASTAlterCommand::REPLACE_PARTITION: + { + required_access.emplace_back(AccessType::SELECT, command.from_database, command.from_table); + required_access.emplace_back(AccessType::DELETE | AccessType::INSERT, database, table); + break; + } + case ASTAlterCommand::FETCH_PARTITION: + { + required_access.emplace_back(AccessType::FETCH_PARTITION, database, table); + break; + } + case ASTAlterCommand::FREEZE_PARTITION: [[fallthrough]]; + case ASTAlterCommand::FREEZE_ALL: + { + required_access.emplace_back(AccessType::FREEZE_PARTITION, database, table); + break; + } + case ASTAlterCommand::MODIFY_QUERY: + { + required_access.emplace_back(AccessType::MODIFY_VIEW_QUERY, database, table); + break; + } + case ASTAlterCommand::LIVE_VIEW_REFRESH: + { + required_access.emplace_back(AccessType::REFRESH_VIEW, database, table); + break; + } + case ASTAlterCommand::NO_TYPE: break; } return required_access; diff --git a/dbms/src/Interpreters/InterpreterAlterQuery.h b/dbms/src/Interpreters/InterpreterAlterQuery.h index fd395a0de52..a7609eb81f1 100644 --- a/dbms/src/Interpreters/InterpreterAlterQuery.h +++ b/dbms/src/Interpreters/InterpreterAlterQuery.h @@ -8,6 +8,7 @@ namespace DB { class Context; class AccessRightsElements; +class ASTAlterCommand; /** Allows you add or remove a column in the table. @@ -20,6 +21,8 @@ public: BlockIO execute() override; + static AccessRightsElements getRequiredAccessForCommand(const ASTAlterCommand & command, const String & database, const String & table); + private: AccessRightsElements getRequiredAccess() const; diff --git a/dbms/src/Interpreters/InterpreterKillQueryQuery.cpp b/dbms/src/Interpreters/InterpreterKillQueryQuery.cpp index 81a093f4eae..dc365990794 100644 --- a/dbms/src/Interpreters/InterpreterKillQueryQuery.cpp +++ b/dbms/src/Interpreters/InterpreterKillQueryQuery.cpp @@ -6,6 +6,10 @@ #include #include #include +#include +#include +#include +#include #include #include #include @@ -55,7 +59,7 @@ struct QueryDescriptor size_t source_num; bool processed = false; - QueryDescriptor(String && query_id_, String && user_, size_t source_num_, bool processed_ = false) + QueryDescriptor(String query_id_, String user_, size_t source_num_, bool processed_ = false) : query_id(std::move(query_id_)), user(std::move(user_)), source_num(source_num_), processed(processed_) {} }; @@ -81,6 +85,7 @@ static QueryDescriptors extractQueriesExceptMeAndCheckAccess(const Block & proce const ClientInfo & my_client = context.getProcessListElement()->getClientInfo(); std::optional can_kill_query_started_by_another_user; String query_user; + bool access_denied = false; for (size_t i = 0; i < num_processes; ++i) { @@ -95,14 +100,17 @@ static QueryDescriptors extractQueriesExceptMeAndCheckAccess(const Block & proce { if (!can_kill_query_started_by_another_user) can_kill_query_started_by_another_user = context.getAccessRights()->isGranted(&Poco::Logger::get("InterpreterKillQueryQuery"), AccessType::KILL_QUERY); - if (!can_kill_query_started_by_another_user.value()) + if (!*can_kill_query_started_by_another_user) + { + access_denied = true; continue; + } } - res.emplace_back(std::move(query_id), std::move(query_user), i, false); + res.emplace_back(std::move(query_id), query_user, i, false); } - if (res.empty() && !query_user.empty()) // NOLINT + if (res.empty() && access_denied) throw Exception("User " + my_client.current_user + " attempts to kill query created by " + query_user, ErrorCodes::ACCESS_DENIED); return res; @@ -221,19 +229,22 @@ BlockIO InterpreterKillQueryQuery::execute() } case ASTKillQueryQuery::Type::Mutation: { - Block mutations_block = getSelectResult("database, table, mutation_id", "system.mutations"); + Block mutations_block = getSelectResult("database, table, mutation_id, command", "system.mutations"); if (!mutations_block) return res_io; const ColumnString & database_col = typeid_cast(*mutations_block.getByName("database").column); const ColumnString & table_col = typeid_cast(*mutations_block.getByName("table").column); const ColumnString & mutation_id_col = typeid_cast(*mutations_block.getByName("mutation_id").column); + const ColumnString & command_col = typeid_cast(*mutations_block.getByName("command").column); auto header = mutations_block.cloneEmpty(); header.insert(0, {ColumnString::create(), std::make_shared(), "kill_status"}); MutableColumns res_columns = header.cloneEmptyColumns(); auto table_id = StorageID::createEmpty(); + AccessRightsElements required_access_rights; + bool access_denied = false; for (size_t i = 0; i < mutations_block.rows(); ++i) { @@ -248,8 +259,14 @@ BlockIO InterpreterKillQueryQuery::execute() code = CancellationCode::NotFound; else { - if (!context.getAccessRights()->isGranted(&Poco::Logger::get("InterpreterKillQueryQuery"), AccessType::KILL_MUTATION, table_id.database_name, table_id.table_name)) + ParserAlterCommand parser; + auto command_ast = parseQuery(parser, command_col.getDataAt(i).toString(), 0); + required_access_rights = InterpreterAlterQuery::getRequiredAccessForCommand(command_ast->as(), table_id.database_name, table_id.table_name); + if (!context.getAccessRights()->isGranted(&Poco::Logger::get("InterpreterKillQueryQuery"), required_access_rights)) + { + access_denied = true; continue; + } code = storage->killMutation(mutation_id); } } @@ -257,9 +274,9 @@ BlockIO InterpreterKillQueryQuery::execute() insertResultRow(i, code, mutations_block, header, res_columns); } - if (res_columns[0]->empty() && table_id) + if (res_columns[0]->empty() && access_denied) throw Exception( - "Not allowed to kill mutation on " + table_id.getNameForLogs(), + "Not allowed to kill mutation. To execute this query it's necessary to have the grant " + required_access_rights.toString(), ErrorCodes::ACCESS_DENIED); res_io.in = std::make_shared(header.cloneWithColumns(std::move(res_columns))); @@ -295,7 +312,7 @@ AccessRightsElements InterpreterKillQueryQuery::getRequiredAccessForDDLOnCluster if (query.type == ASTKillQueryQuery::Type::Query) required_access.emplace_back(AccessType::KILL_QUERY); else if (query.type == ASTKillQueryQuery::Type::Mutation) - required_access.emplace_back(AccessType::KILL_MUTATION); + required_access.emplace_back(AccessType::UPDATE | AccessType::DELETE | AccessType::MATERIALIZE_INDEX | AccessType::MATERIALIZE_TTL); return required_access; } diff --git a/dbms/tests/queries/0_stateless/00834_kill_mutation.reference b/dbms/tests/queries/0_stateless/00834_kill_mutation.reference index cbee44069d8..1e4a67b66ea 100644 --- a/dbms/tests/queries/0_stateless/00834_kill_mutation.reference +++ b/dbms/tests/queries/0_stateless/00834_kill_mutation.reference @@ -1,7 +1,7 @@ *** Create and kill a single invalid mutation *** 1 -waiting test kill_mutation mutation_3.txt +waiting test kill_mutation mutation_3.txt DELETE WHERE toUInt32(s) = 1 *** Create and kill invalid mutation that blocks another mutation *** 1 -waiting test kill_mutation mutation_4.txt +waiting test kill_mutation mutation_4.txt DELETE WHERE toUInt32(s) = 1 2001-01-01 2 b diff --git a/dbms/tests/queries/0_stateless/00834_kill_mutation_replicated_zookeeper.reference b/dbms/tests/queries/0_stateless/00834_kill_mutation_replicated_zookeeper.reference index a997ebe1dc9..d6a82e48836 100644 --- a/dbms/tests/queries/0_stateless/00834_kill_mutation_replicated_zookeeper.reference +++ b/dbms/tests/queries/0_stateless/00834_kill_mutation_replicated_zookeeper.reference @@ -1,9 +1,9 @@ *** Create and kill a single invalid mutation *** 1 Mutation 0000000000 was killed -waiting test kill_mutation_r1 0000000000 +waiting test kill_mutation_r1 0000000000 DELETE WHERE toUInt32(s) = 1 0 *** Create and kill invalid mutation that blocks another mutation *** 1 -waiting test kill_mutation_r1 0000000001 +waiting test kill_mutation_r1 0000000001 DELETE WHERE toUInt32(s) = 1 2001-01-01 2 b