From 62aacc5539f4ba286d4a39905d00433fbba94390 Mon Sep 17 00:00:00 2001 From: pufit Date: Mon, 3 Jun 2024 18:43:08 -0400 Subject: [PATCH 1/6] Fix default database with grant on cluster --- src/Interpreters/Access/InterpreterGrantQuery.cpp | 9 +++++---- .../integration/test_access_control_on_cluster/test.py | 10 ++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/Interpreters/Access/InterpreterGrantQuery.cpp b/src/Interpreters/Access/InterpreterGrantQuery.cpp index a137404a669..6ad32ae5a31 100644 --- a/src/Interpreters/Access/InterpreterGrantQuery.cpp +++ b/src/Interpreters/Access/InterpreterGrantQuery.cpp @@ -438,6 +438,11 @@ BlockIO InterpreterGrantQuery::execute() RolesOrUsersSet roles_to_revoke; collectRolesToGrantOrRevoke(access_control, query, roles_to_grant, roles_to_revoke); + /// Check if the current user has corresponding access rights granted with grant option. + String current_database = getContext()->getCurrentDatabase(); + elements_to_grant.replaceEmptyDatabase(current_database); + elements_to_revoke.replaceEmptyDatabase(current_database); + /// Executing on cluster. if (!query.cluster.empty()) { @@ -452,10 +457,6 @@ BlockIO InterpreterGrantQuery::execute() return executeDDLQueryOnCluster(updated_query, getContext(), params); } - /// Check if the current user has corresponding access rights granted with grant option. - String current_database = getContext()->getCurrentDatabase(); - elements_to_grant.replaceEmptyDatabase(current_database); - elements_to_revoke.replaceEmptyDatabase(current_database); bool need_check_grantees_are_allowed = true; if (!query.current_grants) checkGrantOption(access_control, *current_user_access, grantees, need_check_grantees_are_allowed, elements_to_grant, elements_to_revoke); diff --git a/tests/integration/test_access_control_on_cluster/test.py b/tests/integration/test_access_control_on_cluster/test.py index 8dbb87c67d8..87298bcabd8 100644 --- a/tests/integration/test_access_control_on_cluster/test.py +++ b/tests/integration/test_access_control_on_cluster/test.py @@ -74,3 +74,13 @@ def test_grant_all_on_cluster(): assert ch2.query("SHOW GRANTS FOR Alex") == "GRANT ALL ON *.* TO Alex\n" ch1.query("DROP USER Alex ON CLUSTER 'cluster'") + + +def test_grant_current_database_on_cluster(): + ch1.query("CREATE DATABASE user_db ON CLUSTER 'cluster'") + ch1.query("CREATE USER IF NOT EXISTS test_user ON CLUSTER 'cluster' DEFAULT DATABASE user_db") + ch1.query("GRANT SELECT ON user_db.* TO test_user ON CLUSTER 'cluster' WITH GRANT OPTION") + + assert ch1.query("SHOW DATABASES", user="test_user") == "user_db\n" + ch1.query("GRANT SELECT ON * TO test_user ON CLUSTER 'cluster'", user="test_user") + assert ch1.query("SHOW DATABASES", user="test_user") == "user_db\n" From c6108cf8f5b919061b2fe2d5b9730e6d9d119013 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Mon, 3 Jun 2024 22:55:53 +0000 Subject: [PATCH 2/6] Automatic style fix --- tests/integration/test_access_control_on_cluster/test.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_access_control_on_cluster/test.py b/tests/integration/test_access_control_on_cluster/test.py index 87298bcabd8..73112b5deae 100644 --- a/tests/integration/test_access_control_on_cluster/test.py +++ b/tests/integration/test_access_control_on_cluster/test.py @@ -78,8 +78,12 @@ def test_grant_all_on_cluster(): def test_grant_current_database_on_cluster(): ch1.query("CREATE DATABASE user_db ON CLUSTER 'cluster'") - ch1.query("CREATE USER IF NOT EXISTS test_user ON CLUSTER 'cluster' DEFAULT DATABASE user_db") - ch1.query("GRANT SELECT ON user_db.* TO test_user ON CLUSTER 'cluster' WITH GRANT OPTION") + ch1.query( + "CREATE USER IF NOT EXISTS test_user ON CLUSTER 'cluster' DEFAULT DATABASE user_db" + ) + ch1.query( + "GRANT SELECT ON user_db.* TO test_user ON CLUSTER 'cluster' WITH GRANT OPTION" + ) assert ch1.query("SHOW DATABASES", user="test_user") == "user_db\n" ch1.query("GRANT SELECT ON * TO test_user ON CLUSTER 'cluster'", user="test_user") From abdf0d5b5896d87302156199b3fbaeddd32c1d14 Mon Sep 17 00:00:00 2001 From: pufit Date: Mon, 3 Jun 2024 21:29:08 -0400 Subject: [PATCH 3/6] fix test --- tests/integration/test_access_control_on_cluster/test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/test_access_control_on_cluster/test.py b/tests/integration/test_access_control_on_cluster/test.py index 73112b5deae..1b480a39768 100644 --- a/tests/integration/test_access_control_on_cluster/test.py +++ b/tests/integration/test_access_control_on_cluster/test.py @@ -84,6 +84,7 @@ def test_grant_current_database_on_cluster(): ch1.query( "GRANT SELECT ON user_db.* TO test_user ON CLUSTER 'cluster' WITH GRANT OPTION" ) + ch1.query("GRANT CLUSTER ON * TO test_user ON CLUSTER 'cluster'") assert ch1.query("SHOW DATABASES", user="test_user") == "user_db\n" ch1.query("GRANT SELECT ON * TO test_user ON CLUSTER 'cluster'", user="test_user") From e59d71be487378561826d49f48885bf83a27096d Mon Sep 17 00:00:00 2001 From: pufit Date: Mon, 3 Jun 2024 23:58:39 -0400 Subject: [PATCH 4/6] fix test --- tests/integration/test_access_control_on_cluster/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_access_control_on_cluster/test.py b/tests/integration/test_access_control_on_cluster/test.py index 1b480a39768..b12add7ad3f 100644 --- a/tests/integration/test_access_control_on_cluster/test.py +++ b/tests/integration/test_access_control_on_cluster/test.py @@ -84,7 +84,7 @@ def test_grant_current_database_on_cluster(): ch1.query( "GRANT SELECT ON user_db.* TO test_user ON CLUSTER 'cluster' WITH GRANT OPTION" ) - ch1.query("GRANT CLUSTER ON * TO test_user ON CLUSTER 'cluster'") + ch1.query("GRANT CLUSTER ON *.* TO test_user ON CLUSTER 'cluster'") assert ch1.query("SHOW DATABASES", user="test_user") == "user_db\n" ch1.query("GRANT SELECT ON * TO test_user ON CLUSTER 'cluster'", user="test_user") From c3fd58475a8ef619fa1ec119350330949c8c92b8 Mon Sep 17 00:00:00 2001 From: pufit Date: Tue, 4 Jun 2024 01:12:30 -0400 Subject: [PATCH 5/6] Add comment --- src/Interpreters/Access/InterpreterGrantQuery.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Interpreters/Access/InterpreterGrantQuery.cpp b/src/Interpreters/Access/InterpreterGrantQuery.cpp index 6ad32ae5a31..b75c0bfb1c7 100644 --- a/src/Interpreters/Access/InterpreterGrantQuery.cpp +++ b/src/Interpreters/Access/InterpreterGrantQuery.cpp @@ -438,7 +438,7 @@ BlockIO InterpreterGrantQuery::execute() RolesOrUsersSet roles_to_revoke; collectRolesToGrantOrRevoke(access_control, query, roles_to_grant, roles_to_revoke); - /// Check if the current user has corresponding access rights granted with grant option. + /// Replacing empty database with the default. This step must be done before replication to avoid privilege escalation. String current_database = getContext()->getCurrentDatabase(); elements_to_grant.replaceEmptyDatabase(current_database); elements_to_revoke.replaceEmptyDatabase(current_database); @@ -457,6 +457,7 @@ BlockIO InterpreterGrantQuery::execute() return executeDDLQueryOnCluster(updated_query, getContext(), params); } + /// Check if the current user has corresponding access rights granted with grant option. bool need_check_grantees_are_allowed = true; if (!query.current_grants) checkGrantOption(access_control, *current_user_access, grantees, need_check_grantees_are_allowed, elements_to_grant, elements_to_revoke); From e7b7c3aebefcdb2f5eb1d765a201193d75671e0a Mon Sep 17 00:00:00 2001 From: pufit Date: Wed, 5 Jun 2024 16:00:08 -0400 Subject: [PATCH 6/6] Update query before replication --- src/Interpreters/Access/InterpreterGrantQuery.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Interpreters/Access/InterpreterGrantQuery.cpp b/src/Interpreters/Access/InterpreterGrantQuery.cpp index b75c0bfb1c7..6a46ac9c330 100644 --- a/src/Interpreters/Access/InterpreterGrantQuery.cpp +++ b/src/Interpreters/Access/InterpreterGrantQuery.cpp @@ -442,6 +442,7 @@ BlockIO InterpreterGrantQuery::execute() String current_database = getContext()->getCurrentDatabase(); elements_to_grant.replaceEmptyDatabase(current_database); elements_to_revoke.replaceEmptyDatabase(current_database); + query.access_rights_elements.replaceEmptyDatabase(current_database); /// Executing on cluster. if (!query.cluster.empty())