Re: [PATCH] Simplify permission checking logic in user.c - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: [PATCH] Simplify permission checking logic in user.c
Date
Msg-id X+wx4rQvHM3Br1Ub@paquier.xyz
Whole thread Raw
In response to [PATCH] Simplify permission checking logic in user.c  (Paul Martinez <paulmtz@google.com>)
Responses Re: [PATCH] Simplify permission checking logic in user.c
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: Weird failure in explain.out with OpenBSD
Next
From: Denis Smirnov
Date:
Subject: Re: PoC Refactor AM analyse API