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);