Skip to content

Commit

Permalink
sql: Drop Role does not properly check if target user does not exist
Browse files Browse the repository at this point in the history
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: 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.
  • Loading branch information
Dedej-Bergin committed Nov 11, 2024
1 parent 8d75ef8 commit 696869a
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 0 deletions.
11 changes: 11 additions & 0 deletions pkg/sql/drop_role.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 696869a

Please sign in to comment.