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

Greetings,

* Michael Paquier (michael(at)paquier(dot)xyz) wrote:
> 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.

That really strikes me as pretty darn unlikely to happen, it's not like
this code is really spread out across very many lines or that it's hard
to see the pretty clear pattern.

In thinking about these role attributes and the restrictions we have
about what attributes don't require superuser to set and what do, I do
feel like we're probably missing a bet regarding replication and
bypassrls.. In other words, I suspect people would be happier if we
provided a way for non-superusers a way to create replication roles and
bypassrls roles. Exactly how we do that is a bit tricky to figure out
but that's certainly a much more productive direction to be going in.

Thanks,

Stephen