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
