Fixes invalid grantor field parsing in grant role propagation (#7451)

DESCRIPTION: Resolves an issue that disrupts distributed GRANT statements with the grantor option In this issue 3 issues are being solved: 1.Correcting the erroneous appending of multiple granted by in the deparser. 2Adding support for grantor (granted by) in grant role propagation. 3. Implementing grantor (granted by) support during the metadata sync grant role propagation phase. Limitations: Currently, the grantor must be created prior to the metadata sync phase. During metadata sync, both the creation of the grantor and the grants given by that role cannot be performed, as the grantor role is not detected during the dependency resolution phase. --------- Co-authored-by: Onur Tirtir <[email protected]>
citusdata · Feb 15, 2024 · 59da063 · 59da063
1 parent c665cb8
commit 59da063
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 17 deletions.
diff --git a/src/backend/distributed/commands/role.c b/src/backend/distributed/commands/role.c
@@ -886,6 +886,14 @@ GenerateGrantRoleStmtsOfRole(Oid roleid)
 	{
 		Form_pg_auth_members membership = (Form_pg_auth_members) GETSTRUCT(tuple);
 
+		ObjectAddress *roleAddress = palloc0(sizeof(ObjectAddress));
+		ObjectAddressSet(*roleAddress, AuthIdRelationId, membership->grantor);
+		if (!IsAnyObjectDistributed(list_make1(roleAddress)))
+		{
+			/* we only need to propagate the grant if the grantor is distributed */
+			continue;
+		}
+
 		GrantRoleStmt *grantRoleStmt = makeNode(GrantRoleStmt);
 		grantRoleStmt->is_grant = true;
 
@@ -901,7 +909,11 @@ GenerateGrantRoleStmtsOfRole(Oid roleid)
 		granteeRole->rolename = GetUserNameFromId(membership->member, true);
 		grantRoleStmt->grantee_roles = list_make1(granteeRole);
 
-		grantRoleStmt->grantor = NULL;
+		RoleSpec *grantorRole = makeNode(RoleSpec);
+		grantorRole->roletype = ROLESPEC_CSTRING;
+		grantorRole->location = -1;
+		grantorRole->rolename = GetUserNameFromId(membership->grantor, false);
+		grantRoleStmt->grantor = grantorRole;
 
 #if PG_VERSION_NUM >= PG_VERSION_16
 
@@ -1241,12 +1253,6 @@ PreprocessGrantRoleStmt(Node *node, const char *queryString,
 		return NIL;
 	}
 
-	/*
-	 * Postgres don't seem to use the grantor. Even dropping the grantor doesn't
-	 * seem to affect the membership. If this changes, we might need to add grantors
-	 * to the dependency resolution too. For now we just don't propagate it.
-	 */
-	stmt->grantor = NULL;
 	stmt->grantee_roles = distributedGranteeRoles;
 	char *sql = DeparseTreeNode((Node *) stmt);
 	stmt->grantee_roles = allGranteeRoles;

diff --git a/src/backend/distributed/deparser/deparse_role_stmts.c b/src/backend/distributed/deparser/deparse_role_stmts.c
@@ -486,7 +486,6 @@ AppendGrantRoleStmt(StringInfo buf, GrantRoleStmt *stmt)
 	appendStringInfo(buf, "%s ", stmt->is_grant ? " TO " : " FROM ");
 	AppendRoleList(buf, stmt->grantee_roles);
 	AppendGrantWithAdminOption(buf, stmt);
-	AppendGrantedByInGrantForRoleSpec(buf, stmt->grantor, stmt->is_grant);
 	AppendGrantRestrictAndCascadeForRoleSpec(buf, stmt->behavior, stmt->is_grant);
 	AppendGrantedByInGrantForRoleSpec(buf, stmt->grantor, 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
@@ -196,6 +196,7 @@ SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::t
 (1 row)
 
 \c - - - :master_port
+create role test_admin_role;
 -- test grants with distributed and non-distributed roles
 SELECT master_remove_node('localhost', :worker_2_port);
  master_remove_node
@@ -221,29 +222,55 @@ CREATE ROLE non_dist_role_4;
 NOTICE:  not propagating CREATE ROLE/USER commands to other nodes
 HINT:  Connect to other nodes directly to manually create all necessary users and roles.
 SET citus.enable_create_role_propagation TO ON;
+grant dist_role_3,dist_role_1 to test_admin_role with admin option;
 SET ROLE dist_role_1;
 GRANT non_dist_role_1 TO non_dist_role_2;
 SET citus.enable_create_role_propagation TO OFF;
+grant dist_role_1 to non_dist_role_1 with admin option;
 SET ROLE non_dist_role_1;
-GRANT dist_role_1 TO dist_role_2;
+GRANT dist_role_1 TO dist_role_2 granted by non_dist_role_1;
 RESET ROLE;
 SET citus.enable_create_role_propagation TO ON;
-GRANT dist_role_3 TO non_dist_role_3;
+GRANT dist_role_3 TO non_dist_role_3 granted by test_admin_role;
 GRANT non_dist_role_4 TO dist_role_4;
+GRANT dist_role_3 TO dist_role_4 granted by test_admin_role;
 SELECT 1 FROM master_add_node('localhost', :worker_2_port);
  ?column?
 ---------------------------------------------------------------------
         1
 (1 row)
 
-SELECT roleid::regrole::text AS role, member::regrole::text, (grantor::regrole::text IN ('postgres', 'non_dist_role_1', 'dist_role_1')) AS grantor, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE '%dist\_%' ORDER BY 1, 2;
+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'
+  ) q;
+  $$
+);
+                                                result
+---------------------------------------------------------------------
+ [{"member":"dist_role_4","role":"dist_role_3","grantor":"test_admin_role","admin_option":false},     +
+  {"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":"dist_role_4","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":"dist_role_4","role":"dist_role_3","grantor":"test_admin_role","admin_option":false},     +
+  {"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;
+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
 ---------------------------------------------------------------------
  dist_role_1     | dist_role_2     | t       | f
+ dist_role_1     | non_dist_role_1 | t       | t
+ dist_role_1     | test_admin_role | t       | t
  dist_role_3     | non_dist_role_3 | t       | f
+ dist_role_3     | test_admin_role | t       | t
  non_dist_role_1 | non_dist_role_2 | t       | f
  non_dist_role_4 | dist_role_4     | t       | f
-(4 rows)
+(7 rows)
 
 SELECT objid::regrole FROM pg_catalog.pg_dist_object WHERE classid='pg_authid'::regclass::oid AND objid::regrole::text LIKE '%dist\_%' ORDER BY 1;
       objid
@@ -255,6 +282,9 @@ 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,dist_role_1 from test_admin_role cascade;
+drop role test_admin_role;
 \c - - - :worker_1_port
 SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE '%dist\_%' ORDER BY 1, 2;
       role       |   member    | grantor  | admin_option
@@ -276,9 +306,8 @@ SELECT rolname FROM pg_authid WHERE rolname LIKE '%dist\_%' ORDER BY 1;
 SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE '%dist\_%' ORDER BY 1, 2;
       role       |   member    | grantor  | admin_option
 ---------------------------------------------------------------------
- dist_role_1     | dist_role_2 | postgres | f
  non_dist_role_4 | dist_role_4 | postgres | f
-(2 rows)
+(1 row)
 
 SELECT rolname FROM pg_authid WHERE rolname LIKE '%dist\_%' ORDER BY 1;
      rolname

diff --git a/src/test/regress/sql/create_role_propagation.sql b/src/test/regress/sql/create_role_propagation.sql
@@ -75,6 +75,8 @@ SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::t
 
 \c - - - :master_port
 
+create role test_admin_role;
+
 -- test grants with distributed and non-distributed roles
 
 SELECT master_remove_node('localhost', :worker_2_port);
@@ -84,6 +86,8 @@ CREATE ROLE dist_role_2;
 CREATE ROLE dist_role_3;
 CREATE ROLE dist_role_4;
 
+
+
 SET citus.enable_create_role_propagation TO OFF;
 
 CREATE ROLE non_dist_role_1 SUPERUSER;
@@ -93,28 +97,51 @@ CREATE ROLE non_dist_role_4;
 
 SET citus.enable_create_role_propagation TO ON;
 
+
+grant dist_role_3,dist_role_1 to test_admin_role with admin option;
+
 SET ROLE dist_role_1;
 
 GRANT non_dist_role_1 TO non_dist_role_2;
 
 SET citus.enable_create_role_propagation TO OFF;
 
+grant dist_role_1 to non_dist_role_1 with admin option;
 SET ROLE non_dist_role_1;
 
-GRANT dist_role_1 TO dist_role_2;
+GRANT dist_role_1 TO dist_role_2 granted by non_dist_role_1;
 
 RESET ROLE;
 
 SET citus.enable_create_role_propagation TO ON;
 
-GRANT dist_role_3 TO non_dist_role_3;
+
+GRANT dist_role_3 TO non_dist_role_3 granted by test_admin_role;
 GRANT non_dist_role_4 TO dist_role_4;
+GRANT dist_role_3 TO dist_role_4 granted by test_admin_role;
+
 
 SELECT 1 FROM master_add_node('localhost', :worker_2_port);
 
-SELECT roleid::regrole::text AS role, member::regrole::text, (grantor::regrole::text IN ('postgres', 'non_dist_role_1', 'dist_role_1')) AS grantor, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE '%dist\_%' ORDER BY 1, 2;
+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'
+  ) q;
+  $$
+);
+
+REVOKE dist_role_3 from dist_role_4 granted by test_admin_role;
+
+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,dist_role_1 from test_admin_role cascade;
+drop role test_admin_role;
+
 \c - - - :worker_1_port
 SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE '%dist\_%' ORDER BY 1, 2;
 SELECT rolname FROM pg_authid WHERE rolname LIKE '%dist\_%' ORDER BY 1;