From 9a0cdbf5af88234732176ea2e5da65b45bf9d518 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrkan=20=C4=B0ndibay?= Date: Mon, 19 Feb 2024 15:44:21 +0300 Subject: [PATCH] Fixes granted by cascade/restrict statements for revoke (#7517) DESCRIPTION: Fixes incorrect propagating of `GRANTED BY` and `CASCADE/RESTRICT` clauses for `REVOKE` statements There are two issues fixed in this PR 1. granted by statement will appear for revoke statements as well 2. revoke/cascade statement will appear after granted by Since granted by statements does not appear in statements, this bug hasn't been visible until now. However, after activating the granted by statement for revoke, order problem arised and this issue was fixed order problem for cascade/revoke as well In summary, this PR provides usage of granted by statements properly now with the correct order of statements. We can verify the both errors, fixed with just single statement REVOKE dist_role_3 from non_dist_role_3 granted by test_admin_role cascade; --- .../distributed/deparser/citus_grantutils.c | 2 +- .../distributed/deparser/deparse_role_stmts.c | 2 +- .../expected/create_role_propagation.out | 37 ++++++++++++++++++- .../regress/sql/create_role_propagation.sql | 24 +++++++++++- 4 files changed, 59 insertions(+), 6 deletions(-) diff --git a/src/backend/distributed/deparser/citus_grantutils.c b/src/backend/distributed/deparser/citus_grantutils.c index c944013f6..8354e0479 100644 --- a/src/backend/distributed/deparser/citus_grantutils.c +++ b/src/backend/distributed/deparser/citus_grantutils.c @@ -74,7 +74,7 @@ AppendGrantRestrictAndCascade(StringInfo buf, GrantStmt *stmt) void AppendGrantedByInGrantForRoleSpec(StringInfo buf, RoleSpec *grantor, bool isGrant) { - if (isGrant && grantor) + if (grantor) { appendStringInfo(buf, " GRANTED BY %s", RoleSpecString(grantor, true)); } diff --git a/src/backend/distributed/deparser/deparse_role_stmts.c b/src/backend/distributed/deparser/deparse_role_stmts.c index 0a2319f0d..a4a085026 100644 --- a/src/backend/distributed/deparser/deparse_role_stmts.c +++ b/src/backend/distributed/deparser/deparse_role_stmts.c @@ -486,8 +486,8 @@ AppendGrantRoleStmt(StringInfo buf, GrantRoleStmt *stmt) appendStringInfo(buf, "%s ", stmt->is_grant ? " TO " : " FROM "); AppendRoleList(buf, stmt->grantee_roles); AppendGrantWithAdminOption(buf, stmt); - AppendGrantRestrictAndCascadeForRoleSpec(buf, stmt->behavior, stmt->is_grant); AppendGrantedByInGrantForRoleSpec(buf, stmt->grantor, stmt->is_grant); + AppendGrantRestrictAndCascadeForRoleSpec(buf, stmt->behavior, stmt->is_grant); appendStringInfo(buf, ";"); } diff --git a/src/test/regress/expected/create_role_propagation.out b/src/test/regress/expected/create_role_propagation.out index 5e2777a4d..90f2690ce 100644 --- a/src/test/regress/expected/create_role_propagation.out +++ b/src/test/regress/expected/create_role_propagation.out @@ -259,7 +259,24 @@ SELECT result FROM run_command_on_all_nodes( {"member":"test_admin_role","role":"dist_role_3","grantor":"postgres","admin_option":true}] (3 rows) -REVOKE dist_role_3 from dist_role_4 granted by test_admin_role; +REVOKE dist_role_3 from dist_role_4 granted by test_admin_role cascade; +SELECT result FROM run_command_on_all_nodes( + $$ + SELECT json_agg(q.* ORDER BY member) FROM ( + SELECT member::regrole::text, roleid::regrole::text AS role, grantor::regrole::text, admin_option + FROM pg_auth_members WHERE roleid::regrole::text = 'dist_role_3' + order by member::regrole::text + ) q; + $$ +); + result +--------------------------------------------------------------------- + [{"member":"non_dist_role_3","role":"dist_role_3","grantor":"test_admin_role","admin_option":false}, + + {"member":"test_admin_role","role":"dist_role_3","grantor":"postgres","admin_option":true}] + [{"member":"test_admin_role","role":"dist_role_3","grantor":"postgres","admin_option":true}] + [{"member":"test_admin_role","role":"dist_role_3","grantor":"postgres","admin_option":true}] +(3 rows) + SELECT roleid::regrole::text AS role, member::regrole::text, (grantor::regrole::text IN ('postgres', 'non_dist_role_1', 'dist_role_1','test_admin_role')) AS grantor, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE '%dist\_%' ORDER BY 1, 2; role | member | grantor | admin_option --------------------------------------------------------------------- @@ -282,7 +299,23 @@ SELECT objid::regrole FROM pg_catalog.pg_dist_object WHERE classid='pg_authid':: non_dist_role_4 (5 rows) -REVOKE dist_role_3 from non_dist_role_3 granted by test_admin_role; +REVOKE dist_role_3 from non_dist_role_3 granted by test_admin_role cascade; +SELECT result FROM run_command_on_all_nodes( + $$ + SELECT json_agg(q.* ORDER BY member) FROM ( + SELECT member::regrole::text, roleid::regrole::text AS role, grantor::regrole::text, admin_option + FROM pg_auth_members WHERE roleid::regrole::text = 'dist_role_3' + order by member::regrole::text + ) q; + $$ +); + result +--------------------------------------------------------------------- + [{"member":"test_admin_role","role":"dist_role_3","grantor":"postgres","admin_option":true}] + [{"member":"test_admin_role","role":"dist_role_3","grantor":"postgres","admin_option":true}] + [{"member":"test_admin_role","role":"dist_role_3","grantor":"postgres","admin_option":true}] +(3 rows) + revoke dist_role_3,dist_role_1 from test_admin_role cascade; drop role test_admin_role; \c - - - :worker_1_port diff --git a/src/test/regress/sql/create_role_propagation.sql b/src/test/regress/sql/create_role_propagation.sql index cc98b1091..bd2951b17 100644 --- a/src/test/regress/sql/create_role_propagation.sql +++ b/src/test/regress/sql/create_role_propagation.sql @@ -132,12 +132,32 @@ SELECT result FROM run_command_on_all_nodes( $$ ); -REVOKE dist_role_3 from dist_role_4 granted by test_admin_role; +REVOKE dist_role_3 from dist_role_4 granted by test_admin_role cascade; + +SELECT result FROM run_command_on_all_nodes( + $$ + SELECT json_agg(q.* ORDER BY member) FROM ( + SELECT member::regrole::text, roleid::regrole::text AS role, grantor::regrole::text, admin_option + FROM pg_auth_members WHERE roleid::regrole::text = 'dist_role_3' + order by member::regrole::text + ) q; + $$ +); SELECT roleid::regrole::text AS role, member::regrole::text, (grantor::regrole::text IN ('postgres', 'non_dist_role_1', 'dist_role_1','test_admin_role')) AS grantor, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE '%dist\_%' ORDER BY 1, 2; SELECT objid::regrole FROM pg_catalog.pg_dist_object WHERE classid='pg_authid'::regclass::oid AND objid::regrole::text LIKE '%dist\_%' ORDER BY 1; -REVOKE dist_role_3 from non_dist_role_3 granted by test_admin_role; +REVOKE dist_role_3 from non_dist_role_3 granted by test_admin_role cascade; + +SELECT result FROM run_command_on_all_nodes( + $$ + SELECT json_agg(q.* ORDER BY member) FROM ( + SELECT member::regrole::text, roleid::regrole::text AS role, grantor::regrole::text, admin_option + FROM pg_auth_members WHERE roleid::regrole::text = 'dist_role_3' + order by member::regrole::text + ) q; + $$ +); revoke dist_role_3,dist_role_1 from test_admin_role cascade; drop role test_admin_role;