Correct revoke for the partially granted rights. (#61115)

* Correct revoke for the partially granted rights.

src/Access/AccessRights.cpp

@@ -408,6 +408,65 @@ struct AccessRights::Node
 
     friend bool operator!=(const Node & left, const Node & right) { return !(left == right); }
 
+    bool contains(const Node & other)
+    {
+        if (min_flags_with_children.contains(other.max_flags_with_children))
+            return true;
+
+        if (!flags.contains(other.flags))
+            return false;
+
+        /// Let's assume that the current node has the following rights:
+        ///
+        /// SELECT ON *.* TO user1;
+        /// REVOKE SELECT ON system.* FROM user1;
+        /// REVOKE SELECT ON mydb.* FROM user1;
+        ///
+        /// And the other node has the rights:
+        ///
+        /// SELECT ON *.* TO user2;
+        /// REVOKE SELECT ON system.* FROM user2;
+        ///
+        /// First, we check that each child from the other node is present in the current node:
+        ///
+        /// SELECT ON *.* TO user1;  -- checked
+        /// REVOKE SELECT ON system.* FROM user1; -- checked
+        if (other.children)
+        {
+            for (const auto & [name, node] : *other.children)
+            {
+                const auto & child = tryGetChild(name);
+                if (child == nullptr)
+                {
+                    if (!flags.contains(node.flags))
+                        return false;
+                }
+                else
+                {
+                    if (!child->contains(node))
+                        return false;
+                }
+            }
+        }
+
+        if (!children)
+            return true;
+
+        /// Then we check that each of our children has no other rights revoked.
+        ///
+        /// REVOKE SELECT ON mydb.* FROM user1; -- check failed, returning false
+        for (const auto & [name, node] : *children)
+        {
+            if (other.children && other.children->contains(name))
+                continue;
+
+            if (!node.flags.contains(other.flags))
+                return false;
+        }
+
+        return true;
+    }
+
     void makeUnion(const Node & other)
     {
         makeUnionRec(other);
@@ -1004,6 +1063,24 @@ bool AccessRights::isGrantedImpl(const AccessFlags & flags, const Args &... args
         return helper(root);
 }
 
+template <bool grant_option>
+bool AccessRights::containsImpl(const AccessRights & other) const
+{
+    auto helper = [&](const std::unique_ptr<Node> & root_node) -> bool
+    {
+        if (!root_node)
+            return !other.root;
+        if (!other.root)
+            return true;
+        return root_node->contains(*other.root);
+    };
+    if constexpr (grant_option)
+        return helper(root_with_grant_option);
+    else
+        return helper(root);
+}
+
+
 template <bool grant_option>
 bool AccessRights::isGrantedImplHelper(const AccessRightsElement & element) const
 {
@@ -1068,6 +1145,8 @@ bool AccessRights::hasGrantOption(const AccessFlags & flags, std::string_view da
 bool AccessRights::hasGrantOption(const AccessRightsElement & element) const { return isGrantedImpl<true>(element); }
 bool AccessRights::hasGrantOption(const AccessRightsElements & elements) const { return isGrantedImpl<true>(elements); }
 
+bool AccessRights::contains(const AccessRights & access_rights) const { return containsImpl<false>(access_rights); }
+bool AccessRights::containsWithGrantOption(const AccessRights & access_rights) const { return containsImpl<true>(access_rights); }
 
 bool operator ==(const AccessRights & left, const AccessRights & right)
 {

src/Access/AccessRights.h

@@ -95,6 +95,10 @@ class AccessRights
     bool hasGrantOption(const AccessRightsElement & element) const;
     bool hasGrantOption(const AccessRightsElements & elements) const;
 
+    /// Checks if a given `access_rights` is a subset for the current access rights.
+    bool contains(const AccessRights & access_rights) const;
+    bool containsWithGrantOption(const AccessRights & access_rights) const;
+
     /// Merges two sets of access rights together.
     /// It's used to combine access rights from multiple roles.
     void makeUnion(const AccessRights & other);
@@ -153,6 +157,9 @@ class AccessRights
     template <bool grant_option>
     bool isGrantedImpl(const AccessRightsElements & elements) const;
 
+    template <bool grant_option>
+    bool containsImpl(const AccessRights & other) const;
+
     template <bool grant_option>
     bool isGrantedImplHelper(const AccessRightsElement & element) const;
 

src/Interpreters/Access/InterpreterGrantQuery.cpp

@@ -178,6 +178,22 @@ namespace
                 elements_to_revoke.emplace_back(std::move(element_to_revoke));
         }
 
+        /// Additional check for REVOKE
+        ///
+        /// If user1 has the rights
+        /// GRANT SELECT ON *.* TO user1;
+        /// REVOKE SELECT ON system.* FROM user1;
+        /// REVOKE SELECT ON mydb.* FROM user1;
+        ///
+        /// And user2 has the rights
+        /// GRANT SELECT ON *.* TO user2;
+        /// REVOKE SELECT ON system.* FROM user2;
+        ///
+        /// the query `REVOKE SELECT ON *.* FROM user1` executed by user2 should succeed.
+        if (current_user_access.getAccessRights()->containsWithGrantOption(access_to_revoke))
+            return;
+
+        /// Technically, this check always fails if `containsWithGrantOption` returns `false`. But we still call it to get a nice exception message.
         current_user_access.checkGrantOption(elements_to_revoke);
     }
 

tests/queries/0_stateless/03006_correct_revoke_for_partial_rights.reference

@@ -0,0 +1 @@
+2

tests/queries/0_stateless/03006_correct_revoke_for_partial_rights.sh

@@ -0,0 +1,33 @@
+#!/usr/bin/env bash
+
+CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CURDIR"/../shell_config.sh
+
+db=${CLICKHOUSE_DATABASE}
+user1="user1_03006_$db_$RANDOM"
+user2="user2_03006_$db_$RANDOM"
+
+${CLICKHOUSE_CLIENT} --multiquery <<EOF
+DROP DATABASE IF EXISTS $db;
+CREATE DATABASE $db;
+CREATE USER $user1, $user2;
+
+GRANT SELECT ON *.* TO $user2 WITH GRANT OPTION;
+REVOKE SELECT ON system.* FROM $user2;
+EOF
+
+${CLICKHOUSE_CLIENT} --user $user2 --query "GRANT CURRENT GRANTS ON *.* TO $user1"
+${CLICKHOUSE_CLIENT} --user $user2 --query "REVOKE ALL ON *.* FROM $user1"
+${CLICKHOUSE_CLIENT} --query "SHOW GRANTS FOR $user1"
+
+${CLICKHOUSE_CLIENT} --user $user2 --query "GRANT CURRENT GRANTS ON *.* TO $user1"
+${CLICKHOUSE_CLIENT} --query "REVOKE ALL ON $db.* FROM $user1"
+${CLICKHOUSE_CLIENT} --user $user2 --query "REVOKE ALL ON *.* FROM $user1"
+${CLICKHOUSE_CLIENT} --query "SHOW GRANTS FOR $user1"
+
+${CLICKHOUSE_CLIENT} --user $user2 --query "GRANT CURRENT GRANTS ON *.* TO $user1"
+${CLICKHOUSE_CLIENT} --query "REVOKE ALL ON $db.* FROM $user2"
+${CLICKHOUSE_CLIENT} --user $user2 --query "REVOKE ALL ON *.* FROM $user1" 2>&1 | grep -c "ACCESS_DENIED"
+
+${CLICKHOUSE_CLIENT} --query "DROP DATABASE IF EXISTS $db"