Merge pull request ClickHouse#70088 from ClickHouse/pufit/better-acce…

…ss-denied-error Better message for ACCESS_DENIED error.
bigo-sg · Sep 28, 2024 · e2cacd9 · e2cacd9
2 parents 924e21f + 1cfa8f6
commit e2cacd9
Show file tree

Hide file tree

Showing 8 changed files with 128 additions and 20 deletions.
diff --git a/src/Access/AccessRights.cpp b/src/Access/AccessRights.cpp
@@ -594,6 +594,14 @@ struct AccessRights::Node
         optimizeTree();
     }
 
+    void makeDifference(const Node & other)
+    {
+        Node rhs = other;
+        makeDifferenceRec(*this, rhs);
+        flags -= other.flags;
+        optimizeTree();
+    }
+
     ProtoElements getElements() const
     {
         ProtoElements res;
@@ -1109,6 +1117,32 @@ struct AccessRights::Node
         result.wildcard_grant = wildcard_grant || rhs.wildcard_grant;
     }
 
+    void makeDifferenceRec(Node & result, Node & rhs)
+    {
+        if (rhs.children)
+        {
+            for (auto & rhs_child : *rhs.children)
+            {
+                auto & result_child = result.getLeaf(rhs_child.node_name, rhs_child.level, !rhs_child.isLeaf());
+                auto & lhs_child = getLeaf(rhs_child.node_name, rhs_child.level, !rhs_child.isLeaf());
+                lhs_child.makeDifferenceRec(result_child, rhs_child);
+            }
+        }
+
+        if (children)
+        {
+            for (auto & lhs_child : *children)
+            {
+                auto & result_child = result.getLeaf(lhs_child.node_name, lhs_child.level, !lhs_child.isLeaf());
+                auto & rhs_child = rhs.getLeaf(lhs_child.node_name, lhs_child.level, !lhs_child.isLeaf());
+                lhs_child.makeDifferenceRec(result_child, rhs_child);
+            }
+        }
+
+        result.flags = flags - rhs.flags;
+        result.wildcard_grant = wildcard_grant || rhs.wildcard_grant;
+    }
+
     void modifyFlagsRec(const ModifyFlagsFunction & function, bool grant_option, bool & flags_added, bool & flags_removed)
     {
         if (children)
@@ -1581,6 +1615,22 @@ void AccessRights::makeIntersection(const AccessRights & other)
 }
 
 
+void AccessRights::makeDifference(const AccessRights & other)
+{
+    auto helper = [](std::unique_ptr<Node> & root_node, const std::unique_ptr<Node> & other_root_node)
+    {
+        if (!root_node || !other_root_node)
+            return;
+
+        root_node->makeDifference(*other_root_node);
+        if (!root_node->flags && !root_node->children)
+            root_node = nullptr;
+    };
+    helper(root, other.root);
+    helper(root_with_grant_option, other.root_with_grant_option);
+}
+
+
 void AccessRights::modifyFlags(const ModifyFlagsFunction & function)
 {
     if (!root)

diff --git a/src/Access/AccessRights.h b/src/Access/AccessRights.h
@@ -150,6 +150,9 @@ class AccessRights
     /// Makes an intersection of access rights.
     void makeIntersection(const AccessRights & other);
 
+    /// Makes a difference (relative complement) of access rights.
+    void makeDifference(const AccessRights & other);
+
     /// Traverse the tree and modify each access flags.
     using ModifyFlagsFunction = std::function<AccessFlags(
         const AccessFlags & flags,

diff --git a/src/Access/Common/AccessRightsElement.cpp b/src/Access/Common/AccessRightsElement.cpp
@@ -233,11 +233,6 @@ void AccessRightsElement::replaceEmptyDatabase(const String & current_database)
 
 String AccessRightsElement::toString() const { return toStringImpl(*this, true); }
 String AccessRightsElement::toStringWithoutOptions() const { return toStringImpl(*this, false); }
-String AccessRightsElement::toStringForAccessTypeSource() const
-{
-    String result{access_flags.toKeywords().front()};
-    return result + " ON *.*";
-}
 
 bool AccessRightsElements::empty() const { return std::all_of(begin(), end(), [](const AccessRightsElement & e) { return e.empty(); }); }
 

diff --git a/src/Access/Common/AccessRightsElement.h b/src/Access/Common/AccessRightsElement.h
@@ -92,7 +92,6 @@ struct AccessRightsElement
     /// Returns a human-readable representation like "GRANT SELECT, UPDATE(x, y) ON db.table".
     String toString() const;
     String toStringWithoutOptions() const;
-    String toStringForAccessTypeSource() const;
 
     void formatColumnNames(WriteBuffer & buffer) const;
     void formatONClause(WriteBuffer & buffer, bool hilite = false) const;

diff --git a/src/Access/ContextAccess.cpp b/src/Access/ContextAccess.cpp
@@ -645,9 +645,25 @@ bool ContextAccess::checkAccessImplHelper(const ContextPtr & context, AccessFlag
                     AccessRightsElement{access_flags, fmt_args...}.toStringWithoutOptions());
             }
 
+            AccessRights difference;
+            difference.grant(flags, fmt_args...);
+            AccessRights original_rights = difference;
+            difference.makeDifference(*getAccessRights());
+
+            if (difference == original_rights)
+            {
+                return access_denied(ErrorCodes::ACCESS_DENIED,
+                    "{}: Not enough privileges. To execute this query, it's necessary to have the grant {}",
+                    AccessRightsElement{access_flags, fmt_args...}.toStringWithoutOptions() + (grant_option ? " WITH GRANT OPTION" : ""));
+            }
+
+
             return access_denied(ErrorCodes::ACCESS_DENIED,
-                "{}: Not enough privileges. To execute this query, it's necessary to have the grant {}",
-                AccessRightsElement{access_flags, fmt_args...}.toStringWithoutOptions() + (grant_option ? " WITH GRANT OPTION" : ""));
+                "{}: Not enough privileges. To execute this query, it's necessary to have the grant {}. "
+                "(Missing permissions: {}){}",
+                AccessRightsElement{access_flags, fmt_args...}.toStringWithoutOptions() + (grant_option ? " WITH GRANT OPTION" : ""),
+                difference.getElements().toStringWithoutOptions(),
+                grant_option ? ". You can try to use the `GRANT CURRENT GRANTS(...)` statement" : "");
         };
 
         /// As we check the SOURCES from the Table Engine logic, direct prompt about Table Engine would be misleading
@@ -671,18 +687,7 @@ bool ContextAccess::checkAccessImplHelper(const ContextPtr & context, AccessFlag
             if (new_flags.isEmpty())
                 return access_denied_no_grant(flags, args...);
 
-            if (grant_option && acs->isGranted(flags, args...))
-            {
-                return access_denied(ErrorCodes::ACCESS_DENIED,
-                    "{}: Not enough privileges. "
-                    "The required privileges have been granted, but without grant option. "
-                    "To execute this query, it's necessary to have the grant {} WITH GRANT OPTION",
-                    AccessRightsElement{new_flags}.toStringForAccessTypeSource());
-            }
-
-            return access_denied(ErrorCodes::ACCESS_DENIED,
-                "{}: Not enough privileges. To execute this query, it's necessary to have the grant {}",
-                AccessRightsElement{new_flags}.toStringForAccessTypeSource() + (grant_option ? " WITH GRANT OPTION" : ""));
+            return access_denied_no_grant(new_flags);
         }
 
         return access_denied_no_grant(flags, args...);

diff --git a/src/Access/tests/gtest_access_rights_ops.cpp b/src/Access/tests/gtest_access_rights_ops.cpp
@@ -445,6 +445,48 @@ TEST(AccessRights, Intersection)
     ASSERT_EQ(lhs.toString(), "GRANT SELECT ON toaster.*");
 }
 
+TEST(AccessRights, Difference)
+{
+    AccessRights lhs, rhs;
+    lhs.grant(AccessType::SELECT);
+    rhs.grant(AccessType::SELECT);
+    rhs.revoke(AccessType::SELECT, "system");
+    lhs.makeDifference(rhs);
+    ASSERT_EQ(lhs.toString(), "GRANT SELECT ON system.*");
+
+    lhs = {};
+    rhs = {};
+    lhs.grantWildcard(AccessType::SELECT, "toast");
+    rhs.grant(AccessType::SELECT);
+    rhs.revoke(AccessType::SELECT, "toaster");
+    lhs.makeDifference(rhs);
+    ASSERT_EQ(lhs.toString(), "GRANT SELECT ON toaster.*");
+
+    lhs = {};
+    rhs = {};
+    lhs.grantWildcard(AccessType::SELECT, "toast");
+    lhs.grant(AccessType::CREATE_TABLE, "jam");
+    auto lhs_old = lhs;
+    lhs.makeDifference(rhs);
+    ASSERT_EQ(lhs, lhs_old);
+
+    lhs = {};
+    rhs = {};
+    lhs.grant(AccessType::SELECT, "toast");
+    rhs.grant(AccessType::CREATE_TABLE, "jam");
+    lhs_old = lhs;
+    lhs.makeDifference(rhs);
+    ASSERT_EQ(lhs, lhs_old);
+
+    lhs = {};
+    rhs = {};
+    lhs.grant(AccessType::ALL);
+    rhs.grant(AccessType::ALL);
+    rhs.revoke(AccessType::SELECT, "system");
+    lhs.makeDifference(rhs);
+    ASSERT_EQ(lhs.toString(), "GRANT SELECT ON system.*");
+}
+
 TEST(AccessRights, Contains)
 {
     AccessRights lhs, rhs;

diff --git a/tests/queries/0_stateless/03215_grant_current_grants.reference b/tests/queries/0_stateless/03215_grant_current_grants.reference
@@ -1,2 +1,5 @@
 GRANT SELECT, CREATE TABLE, CREATE VIEW ON default.*
 GRANT SELECT ON default.*
+OK
+GRANT SELECT ON *.*
+REVOKE SELECT ON system.zookeeper
diff --git a/tests/queries/0_stateless/03215_grant_current_grants.sh b/tests/queries/0_stateless/03215_grant_current_grants.sh
@@ -11,6 +11,7 @@ user3="user03215_3_${CLICKHOUSE_DATABASE}_$RANDOM"
 db=${CLICKHOUSE_DATABASE}
 
 
+${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS $user1, $user2, $user3";
 ${CLICKHOUSE_CLIENT} --query "CREATE USER $user1, $user2, $user3;";
 ${CLICKHOUSE_CLIENT} --query "GRANT SELECT, CREATE TABLE, CREATE VIEW ON $db.* TO $user1 WITH GRANT OPTION;";
 
@@ -23,4 +24,14 @@ ${CLICKHOUSE_CLIENT} --query "SHOW GRANTS FOR $user3" | sed 's/ TO.*//';
 ${CLICKHOUSE_CLIENT} --query "GRANT CURRENT GRANTS(SELECT ON $db.*) TO $user3" --user $user1;
 ${CLICKHOUSE_CLIENT} --query "SHOW GRANTS FOR $user3" | sed 's/ TO.*//';
 
+${CLICKHOUSE_CLIENT} --query "REVOKE ALL ON *.* FROM $user1";
+${CLICKHOUSE_CLIENT} --query "REVOKE ALL ON *.* FROM $user2";
+
+${CLICKHOUSE_CLIENT} --query "GRANT SELECT ON *.* TO $user1 WITH GRANT OPTION";
+${CLICKHOUSE_CLIENT} --query "REVOKE SELECT ON system.zookeeper FROM $user1";
+
+(( $(${CLICKHOUSE_CLIENT} --user $user1 --query "GRANT SELECT ON *.* TO $user2" 2>&1 | grep -c "(Missing permissions: SELECT ON system.*)") >= 1 )) && echo "OK" || echo "UNEXPECTED"
+${CLICKHOUSE_CLIENT} --query "GRANT CURRENT GRANTS(SELECT ON *.*) TO $user2" --user $user1;
+${CLICKHOUSE_CLIENT} --query "SHOW GRANTS FOR $user2" | sed 's/ TO.*//' | sed 's/ FROM.*//';
+
 ${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS $user1, $user2, $user3";