From f3f8376f5fca815c8e1f75b74c781a27d5904124 Mon Sep 17 00:00:00 2001 From: Ashutosh Sharma Date: Tue, 18 Feb 2025 09:11:50 +0000 Subject: [PATCH] Add role dependency check before dropping the role Before dropping a role, check for any dependencies. If dependencies exist, raise an error to prevent the role from being dropped, and include a list of dependent roles in the error message. 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 | 113 ++++++++++++++++++++++ src/test/regress/expected/create_role.out | 29 ++++++ src/test/regress/sql/create_role.sql | 15 +++ 3 files changed, 157 insertions(+) diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 0db174e6f10..7d86f3373ad 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -124,6 +124,116 @@ have_createrole_privilege(void) return has_createrole_privilege(GetUserId()); } +/* + * check_drop_role_dependency + * + * Check if there are any dependencies related to the role that is being + * dropped. + * + * This function scans the pg_auth_members table to find roles that are either + * admins of the role being dropped, or have the role being dropped as an admin + * member. If both conditions are true, it indicates that there are dependencies + * on the role being dropped. In such cases, an error is raised 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 *cell; + StringInfoData dependent_roles_list; + + /* Initialize StringInfo to build the list string */ + initStringInfo(&dependent_roles_list); + + foreach(cell, admin_members_of_drop_role) + { + Oid role_oid = lfirst_oid(cell); + + if (dependent_roles_list.len > 0) + appendStringInfo(&dependent_roles_list, ", "); + + appendStringInfo(&dependent_roles_list, "%s", + GetUserNameFromId(role_oid, false)); + } + + ereport(ERROR, + (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST), + errmsg("role \"%s\" cannot be dropped because some role(s) depend on it", + GetUserNameFromId(roleid, false)), + errdetail("Dependent role(s): %s", dependent_roles_list.data))); + + pfree(dependent_roles_list.data); + } +} /* * CREATE ROLE @@ -1178,6 +1288,9 @@ DropRole(DropRoleStmt *stmt) errdetail("Only roles with the %s attribute and the %s option on role \"%s\" may drop this role.", "CREATEROLE", "ADMIN", NameStr(roleform->rolname)))); + /* Check for role dependencies before dropping the role. */ + check_drop_role_dependency(pg_auth_members_rel, roleid); + /* DROP hook for the role being removed */ InvokeObjectDropHook(AuthIdRelationId, roleid, 0); diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out index 46d4f9efe99..4f051821991 100644 --- a/src/test/regress/expected/create_role.out +++ b/src/test/regress/expected/create_role.out @@ -225,6 +225,34 @@ 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 role(s) depend on it +DETAIL: Dependent role(s): regress_role_admin +DROP ROLE regress_createrole; +ERROR: role "regress_createrole" cannot be dropped because some role(s) depend on it +DETAIL: Dependent role(s): regress_role_admin +DROP ROLE regress_login; +ERROR: role "regress_login" cannot be dropped because some role(s) depend on it +DETAIL: Dependent role(s): regress_role_admin +DROP ROLE regress_inherit; +ERROR: role "regress_inherit" cannot be dropped because some role(s) depend on it +DETAIL: Dependent role(s): regress_role_admin +DROP ROLE regress_connection_limit; +ERROR: role "regress_connection_limit" cannot be dropped because some role(s) depend on it +DETAIL: Dependent role(s): regress_role_admin +DROP ROLE regress_encrypted_password; +ERROR: role "regress_encrypted_password" cannot be dropped because some role(s) depend on it +DETAIL: Dependent role(s): regress_role_admin +DROP ROLE regress_password_null; +ERROR: role "regress_password_null" cannot be dropped because some role(s) depend on it +DETAIL: Dependent role(s): regress_role_admin +-- 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 +263,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