Re: [PATCH] Simplify permission checking logic in user.c

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