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..7a6014ceb9b 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) + return !other_root_node; + 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/Access/AccessRights.h b/src/Access/AccessRights.h index ea15338f6f3..e35fadbfcde 100644 --- a/src/Access/AccessRights.h +++ b/src/Access/AccessRights.h @@ -140,7 +140,14 @@ public: bool hasGrantOptionWildcard(const AccessFlags & flags, std::string_view database, std::string_view table, const Strings & columns) const; /// Checks if a given `access_rights` is a subset for the current access rights. + /// This function checks both access rights with and without grant option. + /// + /// root ⊇ other.root && root_with_grant_option ⊇ other.root_with_grant_option bool contains(const AccessRights & access_rights) const; + + /// Similar to `contains`, but checks that current access rights with grant option contain other access rights + /// + /// root_with_grant_option ⊇ other.root bool containsWithGrantOption(const AccessRights & access_rights) const; /// Merges two sets of access rights together. diff --git a/src/Backups/RestorerFromBackup.cpp b/src/Backups/RestorerFromBackup.cpp index eb4ba9424ff..a603ed0a027 100644 --- a/src/Backups/RestorerFromBackup.cpp +++ b/src/Backups/RestorerFromBackup.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -710,11 +711,16 @@ 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 rights contained in our current 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 & current_user_access_rights = context->getAccess()->getAccessRightsWithImplicit(); + if (current_user_access_rights->contains(required_access_rights)) + return; - context->checkAccess(required_access); + context->checkAccess(required_access_rights.getElements()); } AccessEntitiesToRestore RestorerFromBackup::getAccessEntitiesToRestore(const String & data_path_in_backup) const diff --git a/tests/integration/test_backup_restore_new/test.py b/tests/integration/test_backup_restore_new/test.py index 762a4de2fbe..33e89bbe891 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") @@ -1878,6 +1879,32 @@ def test_tables_dependency(): drop() +def test_required_privileges_with_partial_revokes(): + 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 TABLE 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 f* FROM u2") + # To restore the backup we should have the SELECT permission on any table except system.zookeeper* and foo.*, but now we don't have it on f*. + assert "Not enough privileges" in instance.query_and_get_error( + f"RESTORE ALL FROM {backup_name}", user="u2" + ) + + # Test for the "clickhouse_backupview" utility. test_backupview_dir = os.path.abspath(