Merge pull request #34855 from vitlibar/ignore-obsolete-grants-in-attach-grants

Ignore obsolete grants in ATTACH GRANT statements

src/Access/AccessEntityIO.cpp

@@ -91,7 +91,7 @@ String serializeAccessEntity(const IAccessEntity & entity)
     return buf.str();
 }
 
-AccessEntityPtr deserializeAccessEntity(const String & definition, const String & path)
+AccessEntityPtr deserializeAccessEntityImpl(const String & definition)
 {
     ASTs queries;
     ParserAttachAccessEntity parser;
@@ -118,43 +118,42 @@ AccessEntityPtr deserializeAccessEntity(const String & definition, const String
         if (auto * create_user_query = query->as<ASTCreateUserQuery>())
         {
             if (res)
-                throw Exception("Two access entities attached in " + path, ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
+                throw Exception("Two access entities attached in the same file", ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
             res = user = std::make_unique<User>();
             InterpreterCreateUserQuery::updateUserFromQuery(*user, *create_user_query);
         }
         else if (auto * create_role_query = query->as<ASTCreateRoleQuery>())
         {
             if (res)
-                throw Exception("Two access entities attached in " + path, ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
+                throw Exception("Two access entities attached in the same file", ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
             res = role = std::make_unique<Role>();
             InterpreterCreateRoleQuery::updateRoleFromQuery(*role, *create_role_query);
         }
         else if (auto * create_policy_query = query->as<ASTCreateRowPolicyQuery>())
         {
             if (res)
-                throw Exception("Two access entities attached in " + path, ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
+                throw Exception("Two access entities attached in the same file", ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
             res = policy = std::make_unique<RowPolicy>();
             InterpreterCreateRowPolicyQuery::updateRowPolicyFromQuery(*policy, *create_policy_query);
         }
         else if (auto * create_quota_query = query->as<ASTCreateQuotaQuery>())
         {
             if (res)
-                throw Exception("Two access entities attached in " + path, ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
+                throw Exception("Two access entities attached in the same file", ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
             res = quota = std::make_unique<Quota>();
             InterpreterCreateQuotaQuery::updateQuotaFromQuery(*quota, *create_quota_query);
         }
         else if (auto * create_profile_query = query->as<ASTCreateSettingsProfileQuery>())
         {
             if (res)
-                throw Exception("Two access entities attached in " + path, ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
+                throw Exception("Two access entities attached in the same file", ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
             res = profile = std::make_unique<SettingsProfile>();
             InterpreterCreateSettingsProfileQuery::updateSettingsProfileFromQuery(*profile, *create_profile_query);
         }
         else if (auto * grant_query = query->as<ASTGrantQuery>())
         {
             if (!user && !role)
-                throw Exception(
-                    "A user or role should be attached before grant in " + path, ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
+                throw Exception("A user or role should be attached before grant", ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
             if (user)
                 InterpreterGrantQuery::updateUserFromQuery(*user, *grant_query);
             else
@@ -165,9 +164,27 @@ AccessEntityPtr deserializeAccessEntity(const String & definition, const String
     }
 
     if (!res)
-        throw Exception("No access entities attached in " + path, ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
+        throw Exception("No access entities attached", ErrorCodes::INCORRECT_ACCESS_ENTITY_DEFINITION);
 
     return res;
 }
 
+
+AccessEntityPtr deserializeAccessEntity(const String & definition, const String & file_path)
+{
+    if (file_path.empty())
+        return deserializeAccessEntityImpl(definition);
+
+    try
+    {
+        return deserializeAccessEntityImpl(definition);
+    }
+    catch (Exception & e)
+    {
+        e.addMessage("Could not parse " + file_path);
+        e.rethrow();
+        __builtin_unreachable();
+    }
+}
+
 }

src/Access/AccessEntityIO.h

@@ -10,6 +10,6 @@ using AccessEntityPtr = std::shared_ptr<const IAccessEntity>;
 
 String serializeAccessEntity(const IAccessEntity & entity);
 
-AccessEntityPtr deserializeAccessEntity(const String & definition, const String & path);
+AccessEntityPtr deserializeAccessEntity(const String & definition, const String & file_path = "");
 
 }

src/Access/DiskAccessStorage.cpp

@@ -48,7 +48,7 @@ namespace
         }
         catch (...)
         {
-            tryLogCurrentException(&log, "Could not parse " + file_path);
+            tryLogCurrentException(&log);
             return nullptr;
         }
     }

src/Parsers/Access/ParserGrantQuery.cpp

@@ -156,7 +156,7 @@ namespace
     }
 
 
-    void eraseNonGrantable(AccessRightsElements & elements)
+    void throwIfNotGrantable(AccessRightsElements & elements)
     {
         boost::range::remove_erase_if(elements, [](AccessRightsElement & element)
         {
@@ -303,7 +303,12 @@ bool ParserGrantQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected)
     }
 
     if (!is_revoke)
-        eraseNonGrantable(elements);
+    {
+        if (attach_mode)
+            elements.eraseNonGrantable();
+        else
+            throwIfNotGrantable(elements);
+    }
 
     auto query = std::make_shared<ASTGrantQuery>();
     node = query;

tests/integration/test_access_for_functions/test.py

@@ -1,8 +1,9 @@
 import pytest
+import uuid
 from helpers.cluster import ClickHouseCluster
 
 cluster = ClickHouseCluster(__file__)
-instance = cluster.add_instance('instance')
+instance = cluster.add_instance('instance', stay_alive=True)
 
 
 @pytest.fixture(scope="module", autouse=True)
@@ -14,7 +15,8 @@ def started_cluster():
     finally:
         cluster.shutdown()
 
-def test_access_rights_for_funtion():
+
+def test_access_rights_for_function():
     create_function_query = "CREATE FUNCTION MySum AS (a, b) -> a + b"
 
     instance.query("CREATE USER A")
@@ -37,3 +39,19 @@ def test_access_rights_for_funtion():
 
     instance.query("DROP USER IF EXISTS A")
     instance.query("DROP USER IF EXISTS B")
+
+
+def test_ignore_obsolete_grant_on_database():
+    instance.stop_clickhouse()
+
+    user_id = uuid.uuid4()
+    instance.exec_in_container(["bash", "-c" , f"""
+        cat > /var/lib/clickhouse/access/{user_id}.sql << EOF
+ATTACH USER X;
+ATTACH GRANT CREATE FUNCTION, SELECT ON mydb.* TO X;
+EOF"""])
+
+    instance.exec_in_container(["bash", "-c" , "touch /var/lib/clickhouse/access/need_rebuild_lists.mark"])
+    instance.start_clickhouse()
+
+    assert instance.query("SHOW GRANTS FOR X") == "GRANT SELECT ON mydb.* TO X\n"