Merge #151472

151472: sql: fix DROP USER to detect default privilege dependencies r=rafiss a=spilchen

Prior to this fix, DROP USER would succeed even when a role had created default privilege rules, leaving orphaned default privilege entries that referenced non-existent roles.

The issue occurred because the dependency detection logic only checked for roles that had privileges granted to them, but missed the case where a role was the creator of default privileges through REVOKE operations. When default privileges are created with REVOKE (e.g., "ALTER DEFAULT PRIVILEGES FOR ROLE user REVOKE EXECUTE ON ROUTINES FROM public"), the grantee list becomes empty, so the original logic didn't detect the dependency.

This fix adds explicit detection of creator dependencies for default privileges, ensuring that roles cannot be dropped if they own default privilege rules.

Fixes #150543.

Release note (bug fix): Fixed DROP USER succeeding when a role owned default privileges, which could leave invalid privilege entries in the system.

Epic: None

Co-authored-by: Matt Spilchen <[email protected]>

Author: craig[bot]

pkg/cmd/roachtest/testdata/pg_regress/privileges.diffs

@@ -4497,7 +4497,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/privileges.out --
  DROP TABLE atest1;
  DROP TABLE atest2;
  DROP TABLE atest3;
-@@ -2680,108 +4209,214 @@
+@@ -2680,108 +4209,219 @@
  DROP TABLE atest5;
  DROP TABLE atest6;
  DROP TABLE atestc;
@@ -4527,15 +4527,20 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/privileges.out --
 +HINT:  try \h REVOKE
  DROP OWNED BY regress_priv_user1;
  DROP USER regress_priv_user1;
++ERROR:  role regress_priv_user1 cannot be dropped because some objects depend on it
++owner of default privileges that revokes privileges of routines from public in database root
++owner of default privileges that revokes privileges of types from public in database root
++HINT:  USE root; ALTER DEFAULT PRIVILEGES FOR ROLE regress_priv_user1 GRANT EXECUTE ON ROUTINES TO public;
++USE root; ALTER DEFAULT PRIVILEGES FOR ROLE regress_priv_user1 GRANT USAGE ON TYPES TO public;
  DROP USER regress_priv_user2;
 +ERROR:  role regress_priv_user2 cannot be dropped because some objects depend on it
-+privileges for default privileges on new  belonging to role test_admin in database root
 +privileges for default privileges on new relations belonging to role test_admin in database root
++privileges for default privileges on new routines belonging to role test_admin in database root
 +privileges for default privileges on new schemas belonging to role test_admin in database root
 +privileges for default privileges on new sequences belonging to role test_admin in database root
 +privileges for default privileges on new types belonging to role test_admin in database root
-+HINT:  USE root; ALTER DEFAULT PRIVILEGES FOR ROLE test_admin REVOKE ALL ON ROUTINES FROM regress_priv_user2;
-+USE root; ALTER DEFAULT PRIVILEGES FOR ROLE test_admin REVOKE ALL ON TABLES FROM regress_priv_user2;
++HINT:  USE root; ALTER DEFAULT PRIVILEGES FOR ROLE test_admin REVOKE ALL ON TABLES FROM regress_priv_user2;
++USE root; ALTER DEFAULT PRIVILEGES FOR ROLE test_admin REVOKE ALL ON ROUTINES FROM regress_priv_user2;
 +USE root; ALTER DEFAULT PRIVILEGES FOR ROLE test_admin REVOKE ALL ON SCHEMAS FROM regress_priv_user2;
 +USE root; ALTER DEFAULT PRIVILEGES FOR ROLE test_admin REVOKE ALL ON SEQUENCES FROM regress_priv_user2;
 +USE root; ALTER DEFAULT PRIVILEGES FOR ROLE test_admin REVOKE ALL ON TYPES FROM regress_priv_user2;
@@ -4725,7 +4730,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/privileges.out --
  -- clean up
  DROP TABLE lock_table;
  DROP USER regress_locktable_user;
-@@ -2791,24 +4426,17 @@
+@@ -2791,24 +4431,17 @@
  \c -
  CREATE ROLE regress_readallstats;
  SELECT has_table_privilege('regress_readallstats','pg_backend_memory_contexts','SELECT'); -- no
@@ -4754,7 +4759,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/privileges.out --
  SELECT has_table_privilege('regress_readallstats','pg_shmem_allocations','SELECT'); -- yes
   has_table_privilege 
  ---------------------
-@@ -2818,11 +4446,7 @@
+@@ -2818,11 +4451,7 @@
  -- run query to ensure that functions within views can be executed
  SET ROLE regress_readallstats;
  SELECT COUNT(*) >= 0 AS ok FROM pg_backend_memory_contexts;
@@ -4767,7 +4772,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/privileges.out --
  SELECT COUNT(*) >= 0 AS ok FROM pg_shmem_allocations;
   ok 
  ----
-@@ -2838,28 +4462,48 @@
+@@ -2838,28 +4467,48 @@
  CREATE ROLE regress_group_indirect_manager;
  CREATE ROLE regress_group_member;
  GRANT regress_group TO regress_group_direct_manager WITH INHERIT FALSE, ADMIN TRUE;
@@ -4826,7 +4831,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/privileges.out --
  DROP ROLE regress_group;
  DROP ROLE regress_group_direct_manager;
  DROP ROLE regress_group_indirect_manager;
-@@ -2871,22 +4515,59 @@
+@@ -2871,22 +4520,59 @@
  CREATE SCHEMA regress_roleoption;
  GRANT CREATE, USAGE ON SCHEMA regress_roleoption TO PUBLIC;
  GRANT regress_roleoption_donor TO regress_roleoption_protagonist WITH INHERIT TRUE, SET FALSE;

pkg/cmd/roachtest/tests/pg_regress.go

@@ -516,8 +516,6 @@ func registerPGRegress(r registry.Registry) {
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			runPGRegress(ctx, t, c)
 		},
-		// TODO(#150543): remove this.
-		SkipPostValidations: registry.PostValidationInvalidDescriptors,
 	})
 }
 

pkg/sql/catalog/catpb/privilege.proto

@@ -62,6 +62,8 @@ message DefaultPrivilegesForRole {
     // for the role causes it to "own" default privileges as it is no longer
     // the "default" case and the role cannot be dropped until the default case
     // is met.
+    // Note: if adding any public_has_* fields in the future, be sure to update
+    // checking of these default privileges in addDependentPrivileges.
     optional bool public_has_usage_on_types = 4 [(gogoproto.nullable) = false];
     optional bool role_has_all_privileges_on_tables = 5 [(gogoproto.nullable) = false];
     optional bool role_has_all_privileges_on_sequences = 6 [(gogoproto.nullable) = false];

pkg/sql/drop_role.go

@@ -563,16 +563,40 @@ func accumulateDependentDefaultPrivileges(
 			role.ForAllRoles = true
 		}
 		for object, defaultPrivs := range defaultPrivilegesForRole.DefaultPrivilegesPerObject {
-			addDependentPrivileges(object, defaultPrivs, role, userNames, dbName, schemaName)
+			addDependentPrivileges(object, defaultPrivs, role, defaultPrivilegesForRole.GetExplicitRole(),
+				userNames, dbName, schemaName)
 		}
 		return nil
 	})
 }
 
+// addDependentPrivileges checks if a specific default privilege creates dependencies
+// that would prevent dropping any of the target roles.
+//
+// This function examines one default privilege rule and determines if:
+//  1. The role that created the default privilege is being dropped (creator dependency).
+//  2. Any roles that were granted privileges by this default privilege are being
+//     dropped (grantee dependency).
+//
+// Parameters:
+//   - object: The type of database object this default privilege applies to
+//     (e.g., privilege.Tables, privilege.Sequences, privilege.Routines, etc.).
+//   - defaultPrivs: The privilege descriptor containing the actual privilege
+//     grants/revokes for this specific default privilege rule.
+//   - role: The role information for who created this default privilege rule.
+//     Contains either a specific role (role.Role) or indicates it applies to all roles
+//     (role.ForAllRoles)
+//   - explicitRole: This is passed in if the default privilege is for a specific role.
+//   - userNames: Map of roles being dropped -> their accumulated dependency objects.
+//     Used both for lookup (is this role being dropped?) and for recording dependencies.
+//   - dbName: Name of the database where this default privilege is defined.
+//   - schemaName: Name of the schema where this default privilege is defined,
+//     or empty string if this is a database-level default privilege.
 func addDependentPrivileges(
 	object privilege.TargetObjectType,
 	defaultPrivs catpb.PrivilegeDescriptor,
 	role catpb.DefaultPrivilegesRole,
+	explicitRole *catpb.DefaultPrivilegesForRole_ExplicitRole,
 	userNames map[username.SQLUsername][]objectAndType,
 	dbName string,
 	schemaName string,
@@ -587,6 +611,10 @@ func addDependentPrivileges(
 		objectType = "types"
 	case privilege.Schemas:
 		objectType = "schemas"
+	case privilege.Routines:
+		objectType = "routines"
+	default:
+		objectType = object.String()
 	}
 
 	inSchemaMsg := ""
@@ -597,22 +625,34 @@ func addDependentPrivileges(
 	createHint := func(
 		role catpb.DefaultPrivilegesRole,
 		grantee username.SQLUsername,
+		isRevoke bool,
+		privilegeKind privilege.Kind,
 	) string {
 
 		roleString := "ALL ROLES"
 		if !role.ForAllRoles {
 			roleString = fmt.Sprintf("ROLE %s", role.Role.SQLIdentifier())
 		}
 
-		return fmt.Sprintf("USE %s; ALTER DEFAULT PRIVILEGES FOR %s%s REVOKE ALL ON %s FROM %s;",
-			dbName, roleString, strings.ToUpper(inSchemaMsg), strings.ToUpper(object.String()), grantee.SQLIdentifier())
+		var action, targetKeyword string
+		if isRevoke {
+			action = "REVOKE"
+			targetKeyword = "FROM"
+		} else {
+			action = "GRANT"
+			targetKeyword = "TO"
+		}
+
+		return fmt.Sprintf("USE %s; ALTER DEFAULT PRIVILEGES FOR %s%s %s %s ON %s %s %s;",
+			dbName, roleString, strings.ToUpper(inSchemaMsg), action, string(privilegeKind.DisplayName()),
+			strings.ToUpper(object.String()), targetKeyword, grantee.SQLIdentifier())
 	}
 
 	for _, privs := range defaultPrivs.Users {
 		grantee := privs.User()
 		if !role.ForAllRoles {
 			if _, ok := userNames[role.Role]; ok {
-				hint := createHint(role, grantee)
+				hint := createHint(role, grantee, true /* isRevoke */, privilege.ALL)
 				userNames[role.Role] = append(userNames[role.Role],
 					objectAndType{
 						IsDefaultPrivilege: true,
@@ -625,7 +665,7 @@ func addDependentPrivileges(
 			}
 		}
 		if _, ok := userNames[grantee]; ok {
-			hint := createHint(role, grantee)
+			hint := createHint(role, grantee, true /* isRevoke */, privilege.ALL)
 			var err error
 			if role.ForAllRoles {
 				err = errors.Newf(
@@ -645,6 +685,32 @@ func addDependentPrivileges(
 				})
 		}
 	}
+
+	// New roles grant EXECUTE on functions and USAGE on types by default. If any
+	// of those two were revoked, the privilege needs to be granted again to
+	// remove any dependencies.
+	if explicitRole != nil {
+		if _, ok := userNames[role.Role]; ok {
+			var hint string
+			if !explicitRole.PublicHasUsageOnTypes && object == privilege.Types {
+				hint = createHint(role, username.PublicRoleName(), false /* isRevoke */, privilege.USAGE)
+			}
+			if !explicitRole.PublicHasExecuteOnFunctions && object == privilege.Routines {
+				hint = createHint(role, username.PublicRoleName(), false /* isRevoke */, privilege.EXECUTE)
+			}
+			if hint != "" {
+				userNames[role.Role] = append(userNames[role.Role],
+					objectAndType{
+						IsDefaultPrivilege: true,
+						ErrorMessage: errors.WithHint(
+							errors.Newf(
+								"owner of default privileges that revokes privileges of %s from public in database %s%s",
+								objectType, dbName, inSchemaMsg,
+							), hint),
+					})
+			}
+		}
+	}
 }
 
 func addDependentPrivilegesFromSystemPrivileges(

pkg/sql/logictest/testdata/logic_test/drop_user

@@ -215,3 +215,78 @@ statement error role schema_owner cannot be dropped because some objects depend
 DROP ROLE schema_owner
 
 subtest end
+
+# Regression test for #150543. Ensure that DROP USER is blocked when role has
+# default privileges. 
+subtest default_privileges_dependency
+
+statement ok
+CREATE USER default_priv_user
+
+# Create default privileges with REVOKE. Target routines and types initially. These undo
+# the implicitly created default privileges for new roles.
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user REVOKE EXECUTE ON ROUTINES FROM public
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user REVOKE USAGE ON TYPES FROM public
+
+# The next threes REVOKE's are essentially no-op since they don't change anything
+# as nothing was implicitly granted for them.
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user REVOKE USAGE ON SEQUENCES FROM public
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user REVOKE ALL ON TABLES FROM public
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user REVOKE ALL ON SCHEMAS FROM public
+
+statement error role default_priv_user cannot be dropped because some objects depend on it\nowner of default privileges that revokes privileges of routines from public in database test\nowner of default privileges that revokes privileges of types from public in database test\nHINT: USE test; ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user GRANT EXECUTE ON ROUTINES TO public;\nUSE test; ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user GRANT USAGE ON TYPES TO public;
+DROP USER default_priv_user
+
+# Test with GRANT case 
+statement ok
+CREATE USER default_priv_user2
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user2 GRANT ALL ON TABLES TO public
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user2 GRANT ALL ON SCHEMAS TO public
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user2 GRANT USAGE ON SEQUENCES TO public
+
+statement error role default_priv_user2 cannot be dropped because some objects depend on it\nowner of default privileges on new relations belonging to role default_priv_user2 in database test\nowner of default privileges on new schemas belonging to role default_priv_user2 in database test\nowner of default privileges on new sequences belonging to role default_priv_user2 in database test\nHINT: USE test; ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user2 REVOKE ALL ON TABLES FROM public;\nUSE test; ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user2 REVOKE ALL ON SCHEMAS FROM public;\nUSE test; ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user2 REVOKE ALL ON SEQUENCES FROM public;
+DROP USER default_priv_user2
+
+# Clean up for successful drop
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user GRANT EXECUTE ON FUNCTIONS TO public
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user GRANT USAGE ON TYPES TO public
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user2 REVOKE ALL ON TABLES FROM public
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user2 REVOKE ALL ON SCHEMAS FROM public
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE default_priv_user2 REVOKE USAGE ON SEQUENCES FROM public
+
+statement ok
+DROP USER default_priv_user
+
+statement ok
+DROP USER default_priv_user2
+
+# Ensure there are no invalid objects left around. This was how the issue was
+# found prior to #150543.
+query TTTT
+SELECT database_name, schema_name, obj_name, error FROM crdb_internal.invalid_objects;
+----
+
+subtest end

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go

@@ -101,6 +101,8 @@ func DropOwnedBy(b BuildCtx, n *tree.DropOwnedBy) {
 		}
 	})
 
+	// TODO(151425): we should cleanup default privileges for the user too like postgres
+
 	b.IncrementSubWorkID()
 	b.IncrementDropOwnedByCounter()