diff --git a/src/backend/distributed/metadata/dependency.c b/src/backend/distributed/metadata/dependency.c index 2f0120cbd67..5a097d8bae2 100644 --- a/src/backend/distributed/metadata/dependency.c +++ b/src/backend/distributed/metadata/dependency.c @@ -187,10 +187,13 @@ static void ApplyAddCitusDependedObjectsToDependencyList(ObjectAddressCollector static List * GetViewRuleReferenceDependencyList(Oid relationId); static List * ExpandCitusSupportedTypes(ObjectAddressCollector *collector, ObjectAddress target); +static List * ExpandCitusSupportedTypesForNodeActivation(ObjectAddressCollector * + collector, + ObjectAddress target); static List * ExpandForPgVanilla(ObjectAddressCollector *collector, ObjectAddress target); static List * GetDependentRoleIdsFDW(Oid FDWOid); -static List * ExpandRolesToGroups(Oid roleid); +static List * ExpandRolesToGroups(Oid roleid, bool includeGrantors); static ViewDependencyNode * BuildViewDependencyGraph(Oid relationId, HTAB *nodeMap); static bool IsObjectAddressOwnedByExtension(const ObjectAddress *target, ObjectAddress *extensionAddress); @@ -344,7 +347,7 @@ OrderObjectAddressListInDependencyOrder(List *objectAddressList) } RecurseObjectDependencies(*objectAddress, - &ExpandCitusSupportedTypes, + &ExpandCitusSupportedTypesForNodeActivation, &FollowAllSupportedDependencies, &ApplyAddToDependencyList, &collector); @@ -1545,9 +1548,15 @@ ExpandCitusSupportedTypes(ObjectAddressCollector *collector, ObjectAddress targe { /* * Roles are members of other roles. These relations are not recorded directly - * but can be deduced from pg_auth_members + * but can be deduced from pg_auth_members. + * + * Note: we intentionally do NOT include grantors here. Grantors are + * only relevant for ordering role creation during node activation + * (see ExpandCitusSupportedTypesForNodeActivation). Including them + * in the generic dependency graph would produce false-positive + * circular-dependency errors for legitimate mutual GRANTs. */ - return ExpandRolesToGroups(target.objectId); + return ExpandRolesToGroups(target.objectId, false); } case ExtensionRelationId: @@ -1737,6 +1746,32 @@ ExpandCitusSupportedTypes(ObjectAddressCollector *collector, ObjectAddress targe } +/* + * ExpandCitusSupportedTypesForNodeActivation is a variant of + * ExpandCitusSupportedTypes used only when ordering distributed objects for + * propagation to a newly-activated node. For roles, it additionally treats + * grantors of membership tuples as dependencies so that a role used as a + * grantor is created on the new node before the grantee role. This is what + * fixes issue #8425. + * + * This expansion must NOT be used by the generic dependency-collection paths + * (creation, cycle-detection, DDL propagation) because mutually-granted roles + * would appear to form a cycle and be rejected by + * DeferErrorIfCircularDependencyExists. + */ +static List * +ExpandCitusSupportedTypesForNodeActivation(ObjectAddressCollector *collector, + ObjectAddress target) +{ + if (target.classId == AuthIdRelationId) + { + return ExpandRolesToGroups(target.objectId, true); + } + + return ExpandCitusSupportedTypes(collector, target); +} + + /* * ExpandForPgVanilla only expands only comosite types because other types * will find their dependencies in pg_depend. The method should only be called by @@ -1800,10 +1835,19 @@ GetDependentRoleIdsFDW(Oid FDWOid) /* * ExpandRolesToGroups returns a list of object addresses pointing to roles that roleid - * depends on. + * depends on. This always includes: + * 1. Roles that roleid is a member of (membership->roleid) + * + * When includeGrantors is true, it additionally includes: + * 2. Roles that are used as grantors for roleid's memberships (membership->grantor) + * + * The grantor dependency is only used for ordering role propagation during node + * activation (see ExpandCitusSupportedTypesForNodeActivation). It must NOT be used + * in the generic dependency graph because legitimate mutual GRANTs between roles + * would otherwise be reported as circular dependencies. */ static List * -ExpandRolesToGroups(Oid roleid) +ExpandRolesToGroups(Oid roleid, bool includeGrantors) { Relation pgAuthMembers = table_open(AuthMemRelationId, AccessShareLock); HeapTuple tuple = NULL; @@ -1819,15 +1863,47 @@ ExpandRolesToGroups(Oid roleid) true, NULL, scanKeyCount, scanKey); List *roles = NIL; + + /* + * Track all role OIDs we have already emitted as dependencies so that + * parent roles and grantors are de-duplicated through a single set. + * A role can appear multiple times in pg_auth_members for the same + * member (different grantors), and the same OID may show up as both a + * parent role and a grantor; one DependencyDefinition per OID is enough. + * + * Note: For roles with many memberships this O(n) membership check could + * be replaced with a hash set, but in practice the number of memberships + * per role is small. + */ + List *seenRoleIds = NIL; while ((tuple = systable_getnext(scanDescriptor)) != NULL) { Form_pg_auth_members membership = (Form_pg_auth_members) GETSTRUCT(tuple); - DependencyDefinition *definition = palloc0(sizeof(DependencyDefinition)); - definition->mode = DependencyObjectAddress; - ObjectAddressSet(definition->data.address, AuthIdRelationId, membership->roleid); + Oid candidates[2] = { membership->roleid, membership->grantor }; + int numCandidates = includeGrantors ? 2 : 1; + for (int i = 0; i < numCandidates; i++) + { + Oid candidateOid = candidates[i]; - roles = lappend(roles, definition); + /* + * Skip self-references: a role cannot depend on itself (the + * parent-role case cannot hit this because pg_auth_members does + * not allow roleid == member, but the grantor case can). + */ + if (candidateOid == roleid || + !OidIsValid(candidateOid) || + list_member_oid(seenRoleIds, candidateOid)) + { + continue; + } + + DependencyDefinition *definition = palloc0(sizeof(DependencyDefinition)); + definition->mode = DependencyObjectAddress; + ObjectAddressSet(definition->data.address, AuthIdRelationId, candidateOid); + roles = lappend(roles, definition); + seenRoleIds = lappend_oid(seenRoleIds, candidateOid); + } } systable_endscan(scanDescriptor); diff --git a/src/test/regress/expected/create_role_propagation.out b/src/test/regress/expected/create_role_propagation.out index 4d594ddab6c..6a5ff20e5cb 100644 --- a/src/test/regress/expected/create_role_propagation.out +++ b/src/test/regress/expected/create_role_propagation.out @@ -337,10 +337,11 @@ SELECT rolname FROM pg_authid WHERE rolname LIKE '%dist\_%' ORDER BY 1; \c - - - :worker_2_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 + role | member | grantor | admin_option --------------------------------------------------------------------- - non_dist_role_4 | dist_role_4 | postgres | f -(1 row) + dist_role_1 | non_dist_role_1 | postgres | t + non_dist_role_4 | dist_role_4 | postgres | f +(2 rows) SELECT rolname FROM pg_authid WHERE rolname LIKE '%dist\_%' ORDER BY 1; rolname @@ -349,8 +350,9 @@ SELECT rolname FROM pg_authid WHERE rolname LIKE '%dist\_%' ORDER BY 1; dist_role_2 dist_role_3 dist_role_4 + non_dist_role_1 non_dist_role_4 -(5 rows) +(6 rows) \c - - - :master_port DROP ROLE dist_role_3, non_dist_role_3, dist_role_4, non_dist_role_4; @@ -750,4 +752,62 @@ SELECT rolname FROM pg_authid WHERE rolname LIKE '%existing%' ORDER BY 1; (0 rows) \c - - - :master_port +-- test interdependent roles with grantor dependencies +-- This test recreates the issue where role1 is used as a grantor for role2's grants, +-- but role1 hasn't been granted admin option on the parent role yet. +SELECT master_remove_node('localhost', :worker_2_port); + master_remove_node +--------------------------------------------------------------------- + +(1 row) + +CREATE ROLE read_only_role; +CREATE ROLE interdep_role1; +CREATE ROLE interdep_role2; +-- Grant read_only_role to interdep_role1 WITH ADMIN OPTION +-- This allows interdep_role1 to grant read_only_role to other roles +GRANT read_only_role TO interdep_role1 WITH ADMIN OPTION; +-- Grant read_only_role to interdep_role2, using interdep_role1 as the grantor +-- Also make interdep_role1 a member of interdep_role2 with admin option +GRANT read_only_role TO interdep_role2 GRANTED BY interdep_role1; +GRANT interdep_role1 TO interdep_role2 WITH ADMIN OPTION; +-- Verify the grant relationships on coordinator +SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option +FROM pg_auth_members +WHERE roleid::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2') + OR member::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2') +ORDER BY role, member; + role | member | grantor | admin_option +--------------------------------------------------------------------- + interdep_role1 | interdep_role2 | postgres | t + read_only_role | interdep_role1 | postgres | t + read_only_role | interdep_role2 | interdep_role1 | f +(3 rows) + +-- Add worker_2 back - this should succeed with our fix +-- Before the fix, this would fail because interdep_role2 would be propagated before +-- interdep_role1's admin option on read_only_role was set up +SELECT 1 FROM master_add_node('localhost', :worker_2_port); + ?column? +--------------------------------------------------------------------- + 1 +(1 row) + +-- Verify the grants were properly propagated to worker_2 +\c - - - :worker_2_port +SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option +FROM pg_auth_members +WHERE roleid::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2') + OR member::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2') +ORDER BY role, member; + role | member | grantor | admin_option +--------------------------------------------------------------------- + interdep_role1 | interdep_role2 | postgres | t + read_only_role | interdep_role1 | postgres | t + read_only_role | interdep_role2 | interdep_role1 | f +(3 rows) + +\c - - - :master_port +-- Clean up interdependent roles +DROP ROLE interdep_role2, interdep_role1, read_only_role; DROP ROLE nondist_cascade_1, nondist_cascade_2, nondist_cascade_3, dist_cascade; diff --git a/src/test/regress/sql/create_role_propagation.sql b/src/test/regress/sql/create_role_propagation.sql index bd2951b175c..84a54fa1f79 100644 --- a/src/test/regress/sql/create_role_propagation.sql +++ b/src/test/regress/sql/create_role_propagation.sql @@ -320,4 +320,48 @@ SELECT rolname FROM pg_authid WHERE rolname LIKE '%existing%' ORDER BY 1; SELECT rolname FROM pg_authid WHERE rolname LIKE '%existing%' ORDER BY 1; \c - - - :master_port +-- test interdependent roles with grantor dependencies +-- This test recreates the issue where role1 is used as a grantor for role2's grants, +-- but role1 hasn't been granted admin option on the parent role yet. + +SELECT master_remove_node('localhost', :worker_2_port); + +CREATE ROLE read_only_role; +CREATE ROLE interdep_role1; +CREATE ROLE interdep_role2; + +-- Grant read_only_role to interdep_role1 WITH ADMIN OPTION +-- This allows interdep_role1 to grant read_only_role to other roles +GRANT read_only_role TO interdep_role1 WITH ADMIN OPTION; + +-- Grant read_only_role to interdep_role2, using interdep_role1 as the grantor +-- Also make interdep_role1 a member of interdep_role2 with admin option +GRANT read_only_role TO interdep_role2 GRANTED BY interdep_role1; +GRANT interdep_role1 TO interdep_role2 WITH ADMIN OPTION; + +-- Verify the grant relationships on coordinator +SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option +FROM pg_auth_members +WHERE roleid::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2') + OR member::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2') +ORDER BY role, member; + +-- Add worker_2 back - this should succeed with our fix +-- Before the fix, this would fail because interdep_role2 would be propagated before +-- interdep_role1's admin option on read_only_role was set up +SELECT 1 FROM master_add_node('localhost', :worker_2_port); + +-- Verify the grants were properly propagated to worker_2 +\c - - - :worker_2_port +SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option +FROM pg_auth_members +WHERE roleid::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2') + OR member::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2') +ORDER BY role, member; + +\c - - - :master_port + +-- Clean up interdependent roles +DROP ROLE interdep_role2, interdep_role1, read_only_role; + DROP ROLE nondist_cascade_1, nondist_cascade_2, nondist_cascade_3, dist_cascade;