Move privilege check for SET SESSION AUTHORIZATION.

Presently, the privilege check for SET SESSION AUTHORIZATION is
performed in session_authorization's assign_hook.  A relevant
comment states, "It's OK because the check does not require catalog
access and can't fail during an end-of-transaction GUC
reversion..."  However, we plan to add a catalog lookup to this
privilege check in a follow-up commit.

This commit moves this privilege check to the check_hook for
session_authorization.  Like check_role(), we do not throw a hard
error for insufficient privileges when the source is PGC_S_TEST.

Author: Joseph Koshakow
Discussion: https://postgr.es/m/CAAvxfHc-HHzONQ2oXdvhFF9ayRnidPwK%2BfVBhRzaBWYYLVQL-g%40mail.gmail.com

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e076552410aa8170e64cc979b10b61b66ec3..071bef6375401bb5f0a8268d1c4600cd889f2dde 100644 (file)
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -821,14 +821,16 @@ check_session_authorization(char **newval, void **extra, GucSource source)
                return false;
        }
 
+       /*
+        * When source == PGC_S_TEST, we don't throw a hard error for a
+        * nonexistent user name or insufficient privileges, only a NOTICE. See
+        * comments in guc.h.
+        */
+
        /* Look up the username */
        roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
        if (!HeapTupleIsValid(roleTup))
        {
-               /*
-                * When source == PGC_S_TEST, we don't throw a hard error for a
-                * nonexistent user name, only a NOTICE.  See comments in guc.h.
-                */
                if (source == PGC_S_TEST)
                {
                        ereport(NOTICE,
@@ -846,6 +848,28 @@ check_session_authorization(char **newval, void **extra, GucSource source)
 
        ReleaseSysCache(roleTup);
 
+       /*
+        * Only superusers may SET SESSION AUTHORIZATION a role other than itself.
+        * Note that in case of multiple SETs in a single session, the original
+        * authenticated user's superuserness is what matters.
+        */
+       if (roleid != GetAuthenticatedUserId() &&
+               !GetAuthenticatedUserIsSuperuser())
+       {
+               if (source == PGC_S_TEST)
+               {
+                       ereport(NOTICE,
+                                       (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                                        errmsg("permission will be denied to set session authorization \"%s\"",
+                                                       *newval)));
+                       return true;
+               }
+               GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
+               GUC_check_errmsg("permission denied to set session authorization \"%s\"",
+                                                *newval);
+               return false;
+       }
+
        /* Set up "extra" struct for assign_session_authorization to use */
        myextra = (role_auth_extra *) guc_malloc(LOG, sizeof(role_auth_extra));
        if (!myextra)

diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index a604432126c82ee1ac459cc1388d59dedcb0c2bc..64545bc373891d7149c14963d04f3d81887ee609 100644 (file)
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -582,6 +582,16 @@ GetAuthenticatedUserId(void)
        return AuthenticatedUserId;
 }
 
+/*
+ * Return whether the authenticated user was superuser at connection start.
+ */
+bool
+GetAuthenticatedUserIsSuperuser(void)
+{
+       Assert(OidIsValid(AuthenticatedUserId));
+       return AuthenticatedUserIsSuperuser;
+}
+
 
 /*
  * GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
@@ -889,28 +899,12 @@ system_user(PG_FUNCTION_ARGS)
 /*
  * Change session auth ID while running
  *
- * Only a superuser may set auth ID to something other than himself.  Note
- * that in case of multiple SETs in a single session, the original userid's
- * superuserness is what matters.  But we set the GUC variable is_superuser
- * to indicate whether the *current* session userid is a superuser.
- *
- * Note: this is not an especially clean place to do the permission check.
- * It's OK because the check does not require catalog access and can't
- * fail during an end-of-transaction GUC reversion, but we may someday
- * have to push it up into assign_session_authorization.
+ * Note that we set the GUC variable is_superuser to indicate whether the
+ * current role is a superuser.
  */
 void
 SetSessionAuthorization(Oid userid, bool is_superuser)
 {
-       /* Must have authenticated already, else can't make permission check */
-       Assert(OidIsValid(AuthenticatedUserId));
-
-       if (userid != AuthenticatedUserId &&
-               !AuthenticatedUserIsSuperuser)
-               ereport(ERROR,
-                               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                                errmsg("permission denied to set session authorization")));
-
        SetSessionUserId(userid, is_superuser);
 
        SetConfigOption("is_superuser",

diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 14bd574fc24ef2e64ce75ed8432942d9108dce12..11d6e6869deee3900a18ba7f2f9ceb99f14f0a01 100644 (file)
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -357,6 +357,7 @@ extern Oid  GetUserId(void);
 extern Oid     GetOuterUserId(void);
 extern Oid     GetSessionUserId(void);
 extern Oid     GetAuthenticatedUserId(void);
+extern bool GetAuthenticatedUserIsSuperuser(void);
 extern void GetUserIdAndSecContext(Oid *userid, int *sec_context);
 extern void SetUserIdAndSecContext(Oid userid, int sec_context);
 extern bool InLocalUserIdChange(void);