From 4a507b22b26eefbb8d82fe9cd601fbeda16fc9cd Mon Sep 17 00:00:00 2001 From: pufit Date: Fri, 15 Nov 2024 02:39:30 -0500 Subject: [PATCH] Correct access rights check when performing `RESTORE` for users/roles --- src/Access/AccessBackup.cpp | 10 +++---- src/Access/AccessRights.cpp | 18 ++++++++----- src/Backups/RestorerFromBackup.cpp | 12 ++++++--- .../test_backup_restore_new/test.py | 27 ++++++++++++++++++- 4 files changed, 51 insertions(+), 16 deletions(-) diff --git a/src/Access/AccessBackup.cpp b/src/Access/AccessBackup.cpp index 3dee4447e8c..aabbec2c45f 100644 --- a/src/Access/AccessBackup.cpp +++ b/src/Access/AccessBackup.cpp @@ -358,9 +358,8 @@ AccessRightsElements AccessRestorerFromBackup::getRequiredAccess() const auto elements = user.access.getElements(); for (auto & element : elements) { - if (element.is_partial_revoke) - continue; - element.grant_option = true; + if (!element.is_partial_revoke) + element.grant_option = true; res.emplace_back(element); } if (!user.granted_roles.isEmpty()) @@ -375,9 +374,8 @@ AccessRightsElements AccessRestorerFromBackup::getRequiredAccess() const auto elements = role.access.getElements(); for (auto & element : elements) { - if (element.is_partial_revoke) - continue; - element.grant_option = true; + if (!element.is_partial_revoke) + element.grant_option = true; res.emplace_back(element); } if (!role.granted_roles.isEmpty()) diff --git a/src/Access/AccessRights.cpp b/src/Access/AccessRights.cpp index 854e2fea9c4..89379603eb6 100644 --- a/src/Access/AccessRights.cpp +++ b/src/Access/AccessRights.cpp @@ -1209,7 +1209,13 @@ AccessRights::AccessRights(const AccessRightsElement & element) AccessRights::AccessRights(const AccessRightsElements & elements) { - grant(elements); + for (const auto & element : elements) + { + if (element.is_partial_revoke) + revoke(element); + else + grant(element); + } } @@ -1457,18 +1463,18 @@ bool AccessRights::isGrantedImpl(const AccessFlags & flags, const Args &... args template bool AccessRights::containsImpl(const AccessRights & other) const { - auto helper = [&](const std::unique_ptr & root_node) -> bool + auto helper = [&](const std::unique_ptr & root_node, const std::unique_ptr & other_root_node) -> bool { if (!root_node) return !other.root; - if (!other.root) + if (!other_root_node) return true; - return root_node->contains(*other.root); + return root_node->contains(*other_root_node); }; if constexpr (grant_option) - return helper(root_with_grant_option); + return helper(root_with_grant_option, other.root); else - return helper(root); + return helper(root, other.root) && helper(root_with_grant_option, other.root_with_grant_option); } diff --git a/src/Backups/RestorerFromBackup.cpp b/src/Backups/RestorerFromBackup.cpp index 29579aa7348..21dcdccf7ca 100644 --- a/src/Backups/RestorerFromBackup.cpp +++ b/src/Backups/RestorerFromBackup.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -704,9 +705,14 @@ void RestorerFromBackup::checkAccessForObjectsFoundInBackup() const insertAtEnd(required_access, access_restorer->getRequiredAccess()); } - /// We convert to AccessRights and back to check access rights in a predictable way - /// (some elements could be duplicated or not sorted). - required_access = AccessRights{required_access}.getElements(); + /// We convert to AccessRights to check if we have the permissions contains in our access. + /// This handles the situation when restoring partial revoked grants: + /// GRANT SELECT ON *.* + /// REVOKE SELECT FROM systems.* + const auto & required_access_rights = AccessRights{required_access}; + const auto & access_rights = context->getAccess()->getAccessRights(); + if (access_rights->contains(required_access_rights)) + return; context->checkAccess(required_access); } diff --git a/tests/integration/test_backup_restore_new/test.py b/tests/integration/test_backup_restore_new/test.py index a7a22be1cf8..279ae61aafa 100644 --- a/tests/integration/test_backup_restore_new/test.py +++ b/tests/integration/test_backup_restore_new/test.py @@ -8,6 +8,7 @@ from collections import namedtuple import pytest +from helpers.client import QueryRuntimeException from helpers.cluster import ClickHouseCluster from helpers.test_tools import TSV, assert_eq_with_retry @@ -49,7 +50,7 @@ def cleanup_after_test(): instance.query("DROP DATABASE IF EXISTS test") instance.query("DROP DATABASE IF EXISTS test2") instance.query("DROP DATABASE IF EXISTS test3") - instance.query("DROP USER IF EXISTS u1") + instance.query("DROP USER IF EXISTS u1, u2") instance.query("DROP ROLE IF EXISTS r1, r2") instance.query("DROP SETTINGS PROFILE IF EXISTS prof1") instance.query("DROP ROW POLICY IF EXISTS rowpol1 ON test.table") @@ -1845,6 +1846,30 @@ def test_tables_dependency(): drop() +def test_restore_access(): + backup_name = new_backup_name() + instance.query("CREATE USER u1") + instance.query("GRANT SELECT ON *.* TO u1") + instance.query("REVOKE SELECT ON system.zookeeper* FROM u1") + instance.query("REVOKE SELECT ON foo.* FROM u1") + + instance.query(f"BACKUP ALL FROM system.users TO {backup_name}") + instance.query("DROP USER u1") + + instance.query("CREATE USER u2") + instance.query("GRANT SELECT ON *.* TO u2 WITH GRANT OPTION") + instance.query("GRANT CREATE USER ON *.* TO u2") + instance.query("REVOKE SELECT ON system.zookeeper* FROM u2") + instance.query("REVOKE SELECT ON foo.* FROM u2") + + instance.query(f"RESTORE ALL FROM {backup_name}", user="u2") + instance.query("DROP USER u1") + instance.query("REVOKE SELECT ON foo* FROM u2") + + with pytest.raises(QueryRuntimeException): + instance.query(f"RESTORE ALL FROM {backup_name}", user="u2") + + # Test for the "clickhouse_backupview" utility. test_backupview_dir = os.path.abspath(