On 2022-Dec-11, Amit Langote wrote:
> 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.
I gave this a look and I thought it was clearer to have the new
condition depend on rel->reloptkind instead parent or no.
I tried a few things for a new test case, but I was unable to find
anything useful. Maybe an intermediate view, I thought; no dice.
Maybe one with a security barrier would do? Anyway, for now I just kept
what you added in v2.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
>From 423416f41247d5e4fa2e99de411ffa3b5e09cc8e Mon Sep 17 00:00:00 2001
From: amitlan <amitlangot...@gmail.com>
Date: Wed, 18 Jan 2023 16:49:49 +0900
Subject: [PATCH v3] Correctly set userid of subquery rel's child rels
For a RTE_SUBQUERY parent baserel's child relation that itself is a
RTE_RELATION rel, build_simple_rel() should explicitly look up the
latter's RTEPermissionInfo instead of copying the parent rel's
userid, which would be 0 given that it's a subquery rel.
---
src/backend/optimizer/util/relnode.c | 18 ++++++++++---
src/test/regress/expected/inherit.out | 39 +++++++++++++++++++++++++--
src/test/regress/sql/inherit.sql | 28 +++++++++++++++++--
3 files changed, 77 insertions(+), 8 deletions(-)
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index a70a16238a..6c4550b90f 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -233,12 +233,22 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
rel->serverid = InvalidOid;
if (rte->rtekind == RTE_RELATION)
{
+ Assert(parent == NULL ||
+ parent->rtekind == RTE_RELATION ||
+ parent->rtekind == RTE_SUBQUERY);
+
/*
- * Get the userid from the relation's RTEPermissionInfo, though only
- * the tables mentioned in query are assigned RTEPermissionInfos.
- * Child relations (otherrels) simply use the parent's value.
+ * For any RELATION rte, we need a userid with which to check
+ * permission access. Baserels simply use their own RTEPermissionInfo's
+ * checkAsUser.
+ *
+ * For otherrels normally there's no RTEPermissionInfo, so we use the
+ * parent's, which normally has one. The exceptional case is that the
+ * parent is a subquery, in which case the otherrel will have its own.
*/
- if (parent == NULL)
+ if (rel->reloptkind == RELOPT_BASEREL ||
+ (rel->reloptkind == RELOPT_OTHER_MEMBER_REL &&
+ parent->rtekind == RTE_SUBQUERY))
{
RTEPermissionInfo *perminfo;
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 2d49e765de..143271cd62 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2512,9 +2512,44 @@ explain (costs off)
(6 rows)
reset session authorization;
-revoke all on permtest_parent from regress_no_child_access;
-drop role regress_no_child_access;
+-- Check with a view over permtest_parent and a UNION ALL over the view,
+-- especially that permtest_parent's permissions are checked with the role
+-- owning the view as permtest_parent's RTE's checkAsUser.
+create role regress_permtest_view_owner;
+revoke all on permtest_grandchild from regress_permtest_view_owner;
+grant select(a, c) on permtest_parent to regress_permtest_view_owner;
+create view permtest_parent_view as
+ select a, c from permtest_parent;
+alter view permtest_parent_view owner to regress_permtest_view_owner;
+-- Like above, the 2nd arm of UNION ALL gets a hash join due to lack of access
+-- to the expression index's stats
+revoke select on permtest_parent_view from regress_no_child_access;
+grant select(a,c) on permtest_parent_view to regress_no_child_access;
+set session authorization regress_no_child_access;
+explain (costs off)
+ select p1.a, p2.c from
+ (select * from permtest_parent_view
+ union all
+ select * from permtest_parent_view) p1
+ inner join permtest_parent p2 on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
+ QUERY PLAN
+---------------------------------------------------------------------
+ Hash Join
+ Hash Cond: (p2.a = permtest_parent.a)
+ -> Seq Scan on permtest_grandchild p2
+ -> Hash
+ -> Append
+ -> Seq Scan on permtest_grandchild permtest_parent
+ Filter: ("left"(c, 3) ~ 'a1$'::text)
+ -> Seq Scan on permtest_grandchild permtest_parent_1
+ Filter: ("left"(c, 3) ~ 'a1$'::text)
+(9 rows)
+
+reset session authorization;
+drop view permtest_parent_view;
drop table permtest_parent;
+drop role regress_permtest_view_owner;
+drop role regress_no_child_access;
-- Verify that constraint errors across partition root / child are
-- handled correctly (Bug #16293)
CREATE TABLE errtst_parent (
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 195aedb5ff..73b4a26f50 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -907,9 +907,33 @@ explain (costs off)
select p2.a, p1.c from permtest_parent p1 inner join permtest_parent p2
on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
reset session authorization;
-revoke all on permtest_parent from regress_no_child_access;
-drop role regress_no_child_access;
+
+-- Check with a view over permtest_parent and a UNION ALL over the view,
+-- especially that permtest_parent's permissions are checked with the role
+-- owning the view as permtest_parent's RTE's checkAsUser.
+create role regress_permtest_view_owner;
+revoke all on permtest_grandchild from regress_permtest_view_owner;
+grant select(a, c) on permtest_parent to regress_permtest_view_owner;
+create view permtest_parent_view as
+ select a, c from permtest_parent;
+alter view permtest_parent_view owner to regress_permtest_view_owner;
+-- Like above, the 2nd arm of UNION ALL gets a hash join due to lack of access
+-- to the expression index's stats
+revoke select on permtest_parent_view from regress_no_child_access;
+grant select(a,c) on permtest_parent_view to regress_no_child_access;
+set session authorization regress_no_child_access;
+explain (costs off)
+ select p1.a, p2.c from
+ (select * from permtest_parent_view
+ union all
+ select * from permtest_parent_view) p1
+ inner join permtest_parent p2 on p1.a = p2.a and left(p1.c, 3) ~ 'a1$';
+
+reset session authorization;
+drop view permtest_parent_view;
drop table permtest_parent;
+drop role regress_permtest_view_owner;
+drop role regress_no_child_access;
-- Verify that constraint errors across partition root / child are
-- handled correctly (Bug #16293)
--
2.30.2
