From 696869afeb369f3999384e2637610f5fee147362 Mon Sep 17 00:00:00 2001
From: Bergin Dedej <bergin.dedej@cockroachlabs.com>
Date: Mon, 11 Nov 2024 13:12:08 -0500
Subject: [PATCH] sql: Drop Role does not properly check if target user does
 not exist

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.

Fixes: #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.
---
 pkg/sql/drop_role.go                          | 11 +++++++++
 .../drop_role_with_default_privileges         | 24 +++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go
index 2cedbda46537..2b446664c7ba 100644
--- 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
index d6f1d959e918..e884c5b9f1dc 100644
--- 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