From e1848dddef48d0e33d45ee6966d529a4c2cf52bb Mon Sep 17 00:00:00 2001 From: Ashutosh Sharma Date: Thu, 6 Mar 2025 10:19:06 +0000 Subject: [PATCH] Add role dependency check before dropping the role Before dropping a role, check for any existing dependencies. If dependencies are found, raise an error to prevent the role from being dropped, and and provide a detailed error message outlining the role dependencies. Additionally, update the existing test case in create_role.sql to verify the new functionality and ensure proper handling of role dependencies. --- src/backend/commands/user.c | 131 ++++++++++++++++++++++ src/test/regress/expected/create_role.out | 32 ++++++ src/test/regress/sql/create_role.sql | 15 +++ 3 files changed, 178 insertions(+) diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 8ae510c623b..db3207428c2 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -125,6 +125,131 @@ have_createrole_privilege(void) return has_createrole_privilege(GetUserId()); } +/* + * check_drop_role_dependency + * + * Check if there are any dependencies associated with the role being dropped. + * + * This function scans the pg_auth_members table to find: 1) roles that are + * admins of the role being dropped, and 2) roles where the role being dropped + * is an admin. If both conditions are met, it means the roles that are admins + * of the role being dropped rely on it for admin privileges on roles where the + * role being dropped is an admin. + * + * For example, if userA creates userB, and userB creates userC, both userB and + * userA would have admin privileges on userC. UserB has admin privileges on + * userC because it created userC, while userA inherits those admin privileges + * from userB. This means userA relies on userB for admin privileges on userC. + * Therefore, if userB is dropped, userA will lose its admin privileges on + * userC. + * + * If such a dependency exists, this function raises an error to prevent the + * role from being dropped. + * + * pg_auth_members_rel: relation object representing the pg_auth_members system + * catalog. + * roleid: Oid of the role that is being dropped. + */ +static void +check_drop_role_dependency(Relation pg_auth_members_rel, Oid roleid) +{ + ScanKeyData scankey; + SysScanDesc sscan; + HeapTuple tmp_tuple; + List *admin_members_of_drop_role = NULL; /* List of roles that are admin members of the role to drop */ + List *roles_with_drop_role_as_admin_member = NULL; /* List of roles where the role to drop is an admin member */ + + ScanKeyInit(&scankey, + Anum_pg_auth_members_roleid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(roleid)); + + sscan = systable_beginscan(pg_auth_members_rel, + AuthMemRoleMemIndexId, + true, NULL, 1, &scankey); + + while (HeapTupleIsValid(tmp_tuple = systable_getnext(sscan))) + { + Form_pg_auth_members authmem_form; + authmem_form = (Form_pg_auth_members) GETSTRUCT(tmp_tuple); + + if (authmem_form->admin_option) + { + if (!admin_members_of_drop_role) + admin_members_of_drop_role = + list_make1_oid(authmem_form->member); + else + admin_members_of_drop_role = + lappend_oid(admin_members_of_drop_role, + authmem_form->member); + } + } + + systable_endscan(sscan); + + ScanKeyInit(&scankey, + Anum_pg_auth_members_member, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(roleid)); + + sscan = systable_beginscan(pg_auth_members_rel, + AuthMemRoleMemIndexId, + true, NULL, 1, &scankey); + + while (HeapTupleIsValid(tmp_tuple = systable_getnext(sscan))) + { + Form_pg_auth_members authmem_form; + authmem_form = (Form_pg_auth_members) GETSTRUCT(tmp_tuple); + + if (authmem_form->admin_option) + { + if (!roles_with_drop_role_as_admin_member) + roles_with_drop_role_as_admin_member = + list_make1_oid(authmem_form->roleid); + else + roles_with_drop_role_as_admin_member = + lappend_oid(roles_with_drop_role_as_admin_member, + authmem_form->roleid); + } + } + + systable_endscan(sscan); + + /* If there are dependencies, raise an error */ + if (admin_members_of_drop_role && roles_with_drop_role_as_admin_member) + { + ListCell *dependent_role; + ListCell *referenced_role; + StringInfoData detail_log; + + /* Initialize StringInfo to build the detailed error log message */ + initStringInfo(&detail_log); + + foreach(dependent_role, admin_members_of_drop_role) + { + Oid dependent_role_oid = lfirst_oid(dependent_role); + + foreach(referenced_role, roles_with_drop_role_as_admin_member) + { + Oid referenced_role_oid = lfirst_oid(referenced_role); + + if (detail_log.len > 0) + appendStringInfoChar(&detail_log, '\n'); + + appendStringInfo(&detail_log, "role %s inherits ADMIN privileges on role %s through role %s", + GetUserNameFromId(dependent_role_oid, false), + GetUserNameFromId(referenced_role_oid, false), + GetUserNameFromId(roleid, false)); + } + } + + ereport(ERROR, + (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST), + errmsg("role \"%s\" cannot be dropped because some objects depend on it", + GetUserNameFromId(roleid, false)), + errdetail("%s", detail_log.data))); + } +} /* * CREATE ROLE @@ -1190,6 +1315,12 @@ DropRole(DropRoleStmt *stmt) */ LockSharedObject(AuthIdRelationId, roleid, 0, AccessExclusiveLock); + /* + * Check if there are any existing dependencies on the role before dropping + * it. + */ + check_drop_role_dependency(pg_auth_members_rel, roleid); + /* * If there is a pg_auth_members entry that has one of the roles to be * dropped as the roleid or member, it should be silently removed, but diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out index 46d4f9efe99..c16fabd2a95 100644 --- a/src/test/regress/expected/create_role.out +++ b/src/test/regress/expected/create_role.out @@ -225,6 +225,37 @@ REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE; DROP ROLE regress_replication_bypassrls; DROP ROLE regress_replication; DROP ROLE regress_bypassrls; +-- should fail, as some roles have dependency on these roles +DROP ROLE regress_createdb; +ERROR: role "regress_createdb" cannot be dropped because some objects depend on it +DETAIL: role regress_role_admin inherits ADMIN privileges on role regress_adminroles through role regress_createdb +DROP ROLE regress_createrole; +ERROR: role "regress_createrole" cannot be dropped because some objects depend on it +DETAIL: role regress_role_admin inherits ADMIN privileges on role regress_adminroles through role regress_createrole +role regress_role_admin inherits ADMIN privileges on role regress_rolecreator through role regress_createrole +role regress_role_admin inherits ADMIN privileges on role regress_tenant through role regress_createrole +role regress_role_admin inherits ADMIN privileges on role regress_tenant2 through role regress_createrole +DROP ROLE regress_login; +ERROR: role "regress_login" cannot be dropped because some objects depend on it +DETAIL: role regress_role_admin inherits ADMIN privileges on role regress_adminroles through role regress_login +DROP ROLE regress_inherit; +ERROR: role "regress_inherit" cannot be dropped because some objects depend on it +DETAIL: role regress_role_admin inherits ADMIN privileges on role regress_adminroles through role regress_inherit +DROP ROLE regress_connection_limit; +ERROR: role "regress_connection_limit" cannot be dropped because some objects depend on it +DETAIL: role regress_role_admin inherits ADMIN privileges on role regress_adminroles through role regress_connection_limit +DROP ROLE regress_encrypted_password; +ERROR: role "regress_encrypted_password" cannot be dropped because some objects depend on it +DETAIL: role regress_role_admin inherits ADMIN privileges on role regress_adminroles through role regress_encrypted_password +DROP ROLE regress_password_null; +ERROR: role "regress_password_null" cannot be dropped because some objects depend on it +DETAIL: role regress_role_admin inherits ADMIN privileges on role regress_adminroles through role regress_password_null +-- remove any dependencies associated with the role before dropping it +-- should pass +RESET SESSION AUTHORIZATION; +REVOKE regress_createdb, regress_createrole, regress_login, + regress_inherit, regress_connection_limit, regress_encrypted_password, regress_password_null +FROM regress_role_admin; DROP ROLE regress_createdb; DROP ROLE regress_createrole; DROP ROLE regress_login; @@ -235,6 +266,7 @@ DROP ROLE regress_password_null; DROP ROLE regress_noiseword; DROP ROLE regress_inroles; DROP ROLE regress_adminroles; +SET SESSION AUTHORIZATION regress_role_admin; -- fail, cannot drop ourself, nor superusers or roles we lack ADMIN for DROP ROLE regress_role_super; ERROR: permission denied to drop role diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql index 4491a28a8ae..312495c7df8 100644 --- a/src/test/regress/sql/create_role.sql +++ b/src/test/regress/sql/create_role.sql @@ -183,6 +183,20 @@ REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE; DROP ROLE regress_replication_bypassrls; DROP ROLE regress_replication; DROP ROLE regress_bypassrls; +-- should fail, as some roles have dependency on these roles +DROP ROLE regress_createdb; +DROP ROLE regress_createrole; +DROP ROLE regress_login; +DROP ROLE regress_inherit; +DROP ROLE regress_connection_limit; +DROP ROLE regress_encrypted_password; +DROP ROLE regress_password_null; +-- remove any dependencies associated with the role before dropping it +-- should pass +RESET SESSION AUTHORIZATION; +REVOKE regress_createdb, regress_createrole, regress_login, + regress_inherit, regress_connection_limit, regress_encrypted_password, regress_password_null +FROM regress_role_admin; DROP ROLE regress_createdb; DROP ROLE regress_createrole; DROP ROLE regress_login; @@ -193,6 +207,7 @@ DROP ROLE regress_password_null; DROP ROLE regress_noiseword; DROP ROLE regress_inroles; DROP ROLE regress_adminroles; +SET SESSION AUTHORIZATION regress_role_admin; -- fail, cannot drop ourself, nor superusers or roles we lack ADMIN for DROP ROLE regress_role_super; -- 2.17.1