On Sun, Jul 9, 2023 at 12:47 AM Nathan Bossart <
nathandbossart@gmail.com> wrote:
> I think we should split this into two patches: one to move the permission
> check to check_session_authorization() and another for the behavior change.
> I've attached an attempt at the first one (that borrows heavily from your
> latest patch). AFAICT the only reason that the permission check lives in
> SetSessionAuthorization() is because AuthenticatedUserIsSuperuser is static
> to miscinit.c and doesn't have an accessor function. I added one, but it
> would probably just be removed by the following patch. WDYT?
I think that's a good idea. We could even keep around the accessor
function as a good place to bundle the calls to
Assert(OidIsValid(AuthenticatedUserId))
and
superuser_arg(AuthenticatedUserId)
> * Only a superuser may set auth ID to something other than himself
Is "auth ID" the right term here? Maybe something like "Only a
superuser may set their session authorization/ID to something other
than their authenticated ID."
> But we set the GUC variable
> * is_superuser to indicate whether the *current* session userid is a
> * superuser.
Just a small correction here, I believe the is_superuser GUC is meant
to indicate whether the current user id is a superuser, not the current
session user id. We only update is_superuser in SetSessionAuthorization
because we are also updating the current user id in SetSessionUserId.
For example,
test=# CREATE ROLE r1 SUPERUSER;
CREATE ROLE
test=# CREATE ROLE r2;
CREATE ROLE
test=# SET SESSION AUTHORIZATION r1;
SET
test=# SET ROLE r2;
SET
test=> SELECT session_user, current_user;
session_user | current_user
--------------+--------------
r1 | r2
(1 row)
test=> SHOW is_superuser;
is_superuser
--------------
off
(1 row)
Which has also made me realize that the comment on is_superuser in
guc_tables.c is incorrect:
> /* Not for general use --- used by SET SESSION AUTHORIZATION */
Additionally the C variable name for is_superuser is fairly misleading:
> session_auth_is_superuser
The documentation for this GUC in show.sgml is correct:
> True if the current role has superuser privileges.
As an aside, I'm starting to think we should consider removing this
GUC. It sometimes reports an incorrect value [0], and potentially is
not used internally for anything.
I've rebased my changes over your patch and attached them both.
[0]
https://www.postgresql.org/message-id/CAAvxfHcxH-hLndty6CRThGXL1hLsgCn%2BE3QuG_4Qi7GxrHmgKg%40mail.gmail.com