Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)

Hi,

On Sun, Dec 11, 2022 at 5:17 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> 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?

Thanks for pointing this out.

I think these blocks which are rewriting userid to basically the same
value should have been removed from these sites as part of 599b33b94.
Even before that commit, the checkAsUser value should have been the
same in the RTE of both the child relation passed to these functions
and that of the root parent that's looked up by looping. That's
because expand_single_inheritance_child(), which adds child RTEs,
copies the parent RTE's checkAsUser, that is, as of commit 599b33b94.
A later commit a61b1f74823c has removed the checkAsUser field from
RangeTblEntry.

Moreover, 599b33b94 adds some code in build_simple_rel() to set a
given rel's userid value by copying it from the parent rel, such that
the userid value would be the same in all relations in a given
inheritance tree.

I've attached 0001 to remove those extraneous code blocks and add a
comment mentioning that userid need not be recomputed.

While staring at the build_simple_rel() bit mentioned above, I
realized that this code fails to set userid correctly in the
inheritance parent rels that are child relations of subquery parent
relations, such as UNION ALL subqueries. In that case, instead of
copying the userid (= 0) of the parent rel, the child should look up
its own RTEPermissionInfo, which should be there, and use the
checkAsUser value from there. I've attached 0002 to fix this hole. I
am not sure whether there's a way to add a test case for this in the
core suite.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com