Correct access rights check when performing RESTORE for users/roles

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())

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 grant_option>
 bool AccessRights::containsImpl(const AccessRights & other) const
 {
-    auto helper = [&](const std::unique_ptr<Node> & root_node) -> bool
+    auto helper = [&](const std::unique_ptr<Node> & root_node, const std::unique_ptr<Node> & 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);
 }
 
 

src/Backups/RestorerFromBackup.cpp

@@ -8,6 +8,7 @@
 #include <Backups/DDLAdjustingForBackupVisitor.h>
 #include <Access/AccessBackup.h>
 #include <Access/AccessRights.h>
+#include <Access/ContextAccess.h>
 #include <Parsers/ParserCreateQuery.h>
 #include <Parsers/parseQuery.h>
 #include <Parsers/formatAST.h>
@@ -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);
 }

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(