Merge cockroachdb#134850

134850: sql: Drop Role does not properly check if target user does not exist r=Dedej-Bergin a=Dedej-Bergin Previously when we are a non-admin user and we run drop role if exists on a user that does not exist we would get an error that the user does not exist. This is incosistent with other if exists commands and it does not make sense to get an error since by typing if exists we expect that the user may not exist. These code changes take care of that. Loom Video Description: https://www.loom.com/share/cd75d692ef3940be9b3fd395dc911f71?sid=ad6b27e1-620f-45b1-be10-9b733408fae5 Fixes: cockroachdb#134538 Release note (bug fix): When you are a non-admin user and you run drop role if exists on a user that does not exist you no longer get an error message. Co-authored-by: Bergin Dedej <[email protected]>
herkolategan · Nov 12, 2024 · 62c52f4 · 62c52f4
2 parents e4be042 + 696869a
commit 62c52f4
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 0 deletions.
diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go
@@ -104,8 +104,18 @@ func (n *DropRoleNode) startExec(params runParams) error {
 		if name.IsReserved() {
 			return pgerror.Newf(pgcode.ReservedName, "role name %q is reserved", name.Normalized())
 		}
+
 		// Non-admin users cannot drop admins.
 		if !hasAdmin {
+			roleExists, err := RoleExists(params.ctx, params.p.InternalSQLTxn(), name)
+			if err != nil {
+				return err
+			}
+			if !roleExists {
+				// If the role does not exist, we can skip the check for targetIsAdmin.
+				continue
+			}
+
 			targetIsAdmin, err := params.p.UserHasAdminRole(params.ctx, name)
 			if err != nil {
 				return err
@@ -114,6 +124,7 @@ func (n *DropRoleNode) startExec(params runParams) error {
 				return pgerror.New(pgcode.InsufficientPrivilege, "must be superuser to drop superusers")
 			}
 		}
+
 	}
 
 	privilegeObjectFormatter := tree.NewFmtCtx(tree.FmtSimple)

diff --git a/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges b/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges
@@ -159,3 +159,27 @@ ALTER DEFAULT PRIVILEGES FOR ALL ROLES IN SCHEMA public REVOKE ALL ON SEQUENCES
 
 statement ok
 DROP ROLE testuser4;
+
+subtest fix_for_regression_bug_134538
+
+statement ok
+CREATE USER not_admin WITH PASSWORD '123';
+GRANT SYSTEM CREATEROLE TO not_admin;
+SET ROLE not_admin;
+
+statement error pq: role/user "a_user_that_does_not_exist" does not exist
+DROP USER a_user_that_does_not_exist;
+
+statement ok
+DROP USER IF EXISTS a_user_that_does_not_exist;
+
+statement ok
+SET ROLE admin;
+
+statement error pq: role/user "a_user_that_does_not_exist" does not exist
+DROP USER a_user_that_does_not_exist;
+
+statement ok
+DROP USER IF EXISTS a_user_that_does_not_exist;
+
+subtest end