Date: Sat, 10 Dec 2022 14:17:53 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Ian Lawrence Barwick <barwick@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, David Rowley <dgrowleyml@gmail.com>,
	Greg Stark <stark@mit.edu>, Julien Rouhaud <rjuju123@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
Message-ID: <20221210201753.GA27893@telsasoft.com>
References: 
 <CA+HiwqHQeooqvXy+FSXYOq0wajGtC5-TBj0=ZW1=qVq5Pm2ycw@mail.gmail.com>
 <20221129092708.uooopfzpiwrhgedl@alvherre.pgsql>
 <CA+HiwqGFCs2uq7VRKi7g+FFKbP6Ea_2_HkgZb2HPhUfaAKT3ng@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+HiwqGFCs2uq7VRKi7g+FFKbP6Ea_2_HkgZb2HPhUfaAKT3ng@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20221210201753.GA27893%40telsasoft.com>
Precedence: bulk

On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> 0002 contains changes that has to do with changing how we access
> checkAsUser in some foreign table planning/execution code sites.
> Thought it might be better to describe it separately too.

This was committed as 599b33b94:
    Stop accessing checkAsUser via RTE in some cases

Which does this in a couple places in selfuncs.c:

                                if (!vardata->acl_ok &&
                                    root->append_rel_array != NULL)
                                {   
                                    AppendRelInfo *appinfo;
                                    Index       varno = index->rel->relid;

                                    appinfo = root->append_rel_array[varno];
                                    while (appinfo &&
                                           planner_rt_fetch(appinfo->parent_relid,
                                                            root)->rtekind == RTE_RELATION)
                                    {   
                                        varno = appinfo->parent_relid;
                                        appinfo = root->append_rel_array[varno];
                                    }
                                    if (varno != index->rel->relid)
                                    {   
                                        /* Repeat access check on this rel */
                                        rte = planner_rt_fetch(varno, root);
                                        Assert(rte->rtekind == RTE_RELATION);

-                                       userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
+                                       userid = OidIsValid(onerel->userid) ?
+                                           onerel->userid : GetUserId();

                                        vardata->acl_ok =
                                            rte->securityQuals == NIL &&
                                            (pg_class_aclcheck(rte->relid,
                                                               userid,
                                                               ACL_SELECT) == ACLCHECK_OK);
                                    }
                                }


The original code rechecks rte->checkAsUser with the rte of the parent
rel.  The patch changed to access onerel instead, but that's not updated
after looping to find the parent.

Is that okay ?  It doesn't seem intentional, since "userid" is still
being recomputed, but based on onerel, which hasn't changed.  The
original intent (since 553d2ec27) is to recheck the parent's
"checkAsUser".  

It seems like this would matter for partitioned tables, when the
partition isn't readable, but its parent is, and accessed via a view
owned by another user?

-- 
Justin