Backport #71958 to 24.10: Correct access rights check for RESTORE

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)
+                    if (!element.is_partial_revoke)
-                        continue;
+                        element.grant_option = true;
-                    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)
+                    if (!element.is_partial_revoke)
-                        continue;
+                        element.grant_option = true;
-                    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;
+            return !other_root_node;
-        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/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.

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>
@@ -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
+    /// We convert to AccessRights to check if we have the rights contained in our current access.
-    /// (some elements could be duplicated or not sorted).
+    /// This handles the situation when restoring partial revoked grants:
-    required_access = AccessRights{required_access}.getElements();
+    /// 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

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(