On Tue, Dec 29, 2020 at 02:26:19PM -0600, Paul Martinez wrote: > The checks for whether the current user can create a user with the SUPERUSER, > REPLICATION, or BYPASSRLS attributes are chained together using if/else-if, > before finally checking whether the user has CREATEROLE privileges in a > final else. This construction is error-prone, since once one branch is > visited, > later ones are skipped, and it implicitly assumes that the permissions needed > for each subsequent action are subsets of the permissions needed for the > previous action. Since each branch requires SUPERUSER this is fine, but > changing one of the checks could inadvertently allow users without the > CREATEROLE permission to create users.
Hmm. I agree with your position here. A careless change could easily
miss that all those if branches have to use a superuser check.
+ if (isreplication)
{
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to create replication users")));
}
}
It would be more readable to combine both conditions together, no?
Your change also means that we may call superuser() more times than
necessary, but last_roleid_is_super leverages that, and this code path
is not performance-critical either.
I am adding Stephen in CC, just in case he has some comments to
share.
--
Michael
signature.asc
Description: PGP signature
