From 1f81e43f1edf269249490d16fd9d6a90ca10d14d Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Tue, 1 Feb 2022 19:55:24 +0700 Subject: [PATCH 1/2] Fix checking grants for SHOW GRANTS. --- .../Access/InterpreterShowGrantsQuery.cpp | 43 ++++++++++++++++++- .../integration/test_grant_and_revoke/test.py | 9 ++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/Interpreters/Access/InterpreterShowGrantsQuery.cpp b/src/Interpreters/Access/InterpreterShowGrantsQuery.cpp index cd98d8d4575..e11843f52e1 100644 --- a/src/Interpreters/Access/InterpreterShowGrantsQuery.cpp +++ b/src/Interpreters/Access/InterpreterShowGrantsQuery.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -135,15 +136,53 @@ QueryPipeline InterpreterShowGrantsQuery::executeImpl() std::vector InterpreterShowGrantsQuery::getEntities() const { - const auto & show_query = query_ptr->as(); + const auto & access = getContext()->getAccess(); const auto & access_control = getContext()->getAccessControl(); + + const auto & show_query = query_ptr->as(); auto ids = RolesOrUsersSet{*show_query.for_roles, access_control, getContext()->getUserID()}.getMatchingIDs(access_control); + bool throw_if_access_denied = !show_query.for_roles->all; + std::optional show_users_check_result; + std::optional show_roles_check_result; + + auto has_grant_show_users = [&]() + { + if (show_users_check_result) + return *show_users_check_result; + if (throw_if_access_denied) + { + access->checkAccess(AccessType::SHOW_USERS); + show_users_check_result = true; + return true; + } + show_users_check_result = access->isGranted(AccessType::SHOW_USERS); + return *show_users_check_result; + }; + + auto has_grant_show_roles = [&]() + { + if (show_roles_check_result) + return *show_roles_check_result; + if (throw_if_access_denied) + { + access->checkAccess(AccessType::SHOW_ROLES); + show_roles_check_result = true; + return true; + } + show_roles_check_result = access->isGranted(AccessType::SHOW_ROLES); + return *show_roles_check_result; + }; + std::vector entities; for (const auto & id : ids) { auto entity = access_control.tryRead(id); - if (entity) + if (!entity) + continue; + if ((id == access->getUserID()) + || (entity->isTypeOf() && has_grant_show_users()) + || (entity->isTypeOf() && has_grant_show_roles())) entities.push_back(entity); } diff --git a/tests/integration/test_grant_and_revoke/test.py b/tests/integration/test_grant_and_revoke/test.py index b905e4df219..89e07fecb0a 100644 --- a/tests/integration/test_grant_and_revoke/test.py +++ b/tests/integration/test_grant_and_revoke/test.py @@ -250,6 +250,15 @@ def test_introspection(): assert instance.query("SHOW GRANTS", user='A') == TSV(["GRANT SELECT ON test.table TO A"]) assert instance.query("SHOW GRANTS", user='B') == TSV(["GRANT CREATE ON *.* TO B WITH GRANT OPTION"]) + assert instance.query("SHOW GRANTS FOR ALL", user='A') == TSV(["GRANT SELECT ON test.table TO A"]) + assert instance.query("SHOW GRANTS FOR ALL", user='B') == TSV(["GRANT CREATE ON *.* TO B WITH GRANT OPTION"]) + assert instance.query("SHOW GRANTS FOR ALL") == TSV(["GRANT SELECT ON test.table TO A", + "GRANT CREATE ON *.* TO B WITH GRANT OPTION", + "GRANT ALL ON *.* TO default WITH GRANT OPTION"]) + + expected_error = "necessary to have grant SHOW USERS" + assert expected_error in instance.query_and_get_error("SHOW GRANTS FOR B", user='A') + expected_access1 = "CREATE USER A\n" \ "CREATE USER B\n" \ "CREATE USER default IDENTIFIED WITH plaintext_password SETTINGS PROFILE default" From 30557aebfb5cdb0a114d09286bcb52692e629f9f Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 2 Feb 2022 21:29:47 +0700 Subject: [PATCH 2/2] Add helper class to cache the result of checking access. --- src/Access/CachedAccessChecking.cpp | 44 +++++++++++++++++++ src/Access/CachedAccessChecking.h | 29 ++++++++++++ .../Access/InterpreterShowGrantsQuery.cpp | 39 +++------------- 3 files changed, 79 insertions(+), 33 deletions(-) create mode 100644 src/Access/CachedAccessChecking.cpp create mode 100644 src/Access/CachedAccessChecking.h diff --git a/src/Access/CachedAccessChecking.cpp b/src/Access/CachedAccessChecking.cpp new file mode 100644 index 00000000000..aa8ef6073d3 --- /dev/null +++ b/src/Access/CachedAccessChecking.cpp @@ -0,0 +1,44 @@ +#include +#include + + +namespace DB +{ +CachedAccessChecking::CachedAccessChecking(const std::shared_ptr & access_, AccessFlags access_flags_) + : CachedAccessChecking(access_, AccessRightsElement{access_flags_}) +{ +} + +CachedAccessChecking::CachedAccessChecking(const std::shared_ptr & access_, const AccessRightsElement & element_) + : access(access_), element(element_) +{ +} + +CachedAccessChecking::~CachedAccessChecking() = default; + +bool CachedAccessChecking::checkAccess(bool throw_if_denied) +{ + if (checked) + return result; + if (throw_if_denied) + { + try + { + access->checkAccess(element); + result = true; + } + catch (...) + { + result = false; + throw; + } + } + else + { + result = access->isGranted(element); + } + checked = true; + return result; +} + +} diff --git a/src/Access/CachedAccessChecking.h b/src/Access/CachedAccessChecking.h new file mode 100644 index 00000000000..e87c28dd823 --- /dev/null +++ b/src/Access/CachedAccessChecking.h @@ -0,0 +1,29 @@ +#pragma once + +#include +#include + + +namespace DB +{ +class ContextAccess; + +/// Checks if the current user has a specified access type granted, +/// and if it's checked another time later, it will just return the first result. +class CachedAccessChecking +{ +public: + CachedAccessChecking(const std::shared_ptr & access_, AccessFlags access_flags_); + CachedAccessChecking(const std::shared_ptr & access_, const AccessRightsElement & element_); + ~CachedAccessChecking(); + + bool checkAccess(bool throw_if_denied = true); + +private: + const std::shared_ptr access; + const AccessRightsElement element; + bool checked = false; + bool result = false; +}; + +} diff --git a/src/Interpreters/Access/InterpreterShowGrantsQuery.cpp b/src/Interpreters/Access/InterpreterShowGrantsQuery.cpp index e11843f52e1..17d9f321b56 100644 --- a/src/Interpreters/Access/InterpreterShowGrantsQuery.cpp +++ b/src/Interpreters/Access/InterpreterShowGrantsQuery.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -142,37 +143,9 @@ std::vector InterpreterShowGrantsQuery::getEntities() const const auto & show_query = query_ptr->as(); auto ids = RolesOrUsersSet{*show_query.for_roles, access_control, getContext()->getUserID()}.getMatchingIDs(access_control); + CachedAccessChecking show_users(access, AccessType::SHOW_USERS); + CachedAccessChecking show_roles(access, AccessType::SHOW_ROLES); bool throw_if_access_denied = !show_query.for_roles->all; - std::optional show_users_check_result; - std::optional show_roles_check_result; - - auto has_grant_show_users = [&]() - { - if (show_users_check_result) - return *show_users_check_result; - if (throw_if_access_denied) - { - access->checkAccess(AccessType::SHOW_USERS); - show_users_check_result = true; - return true; - } - show_users_check_result = access->isGranted(AccessType::SHOW_USERS); - return *show_users_check_result; - }; - - auto has_grant_show_roles = [&]() - { - if (show_roles_check_result) - return *show_roles_check_result; - if (throw_if_access_denied) - { - access->checkAccess(AccessType::SHOW_ROLES); - show_roles_check_result = true; - return true; - } - show_roles_check_result = access->isGranted(AccessType::SHOW_ROLES); - return *show_roles_check_result; - }; std::vector entities; for (const auto & id : ids) @@ -180,9 +153,9 @@ std::vector InterpreterShowGrantsQuery::getEntities() const auto entity = access_control.tryRead(id); if (!entity) continue; - if ((id == access->getUserID()) - || (entity->isTypeOf() && has_grant_show_users()) - || (entity->isTypeOf() && has_grant_show_roles())) + if ((id == access->getUserID() /* Any user can see his own grants */) + || (entity->isTypeOf() && show_users.checkAccess(throw_if_access_denied)) + || (entity->isTypeOf() && show_roles.checkAccess(throw_if_access_denied))) entities.push_back(entity); }