Re: table partitioning and access privileges

Amit Langote <[email protected]>
Newsgroups gmane.comp.db.postgresql.devel.general
Message-ID <CA+HiwqGCQLKizA+Voyw3GjRv0q8t9adS2YFGmw6GzAbRb+yazg@mail.gmail.com> 
On Fri, Dec 27, 2019 at 4:26 AM Tom Lane <[email protected]> wrote:
> Fujii Masao <[email protected]> writes:
> > My customer reported me that the queries through a partitioned table
> > ignore each partition's SELECT, INSERT, UPDATE, and DELETE privileges,
> > on the other hand, only TRUNCATE privilege specified for each partition
> > is applied. I'm not sure if this behavior is expected or not. But anyway
> > is it better to document that? For example,
>
> >     Access privileges may be defined and removed separately for each partition.
> >     But note that queries through a partitioned table ignore each partition's
> >     SELECT, INSERT, UPDATE and DELETE privileges, and apply only TRUNCATE one.
>
> I believe it's intentional that we only check access privileges on
> the table explicitly named in the query.  So I'd say SELECT etc
> are doing the right thing, and if TRUNCATE isn't in step with them
> that's a bug to fix, not something to document.

I tend to agree that TRUNCATE's permission model for inheritance
should be consistent with that for the other commands.  How about the
attached patch toward that end?

Thanks,
Amit
dont-check-child-truncate-perms.patch (text/plain, 5.2 KB) 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1c4394abea..43377e24c4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -298,6 +298,7 @@ struct DropRelationCallbackState
 	((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
 
 static void truncate_check_rel(Oid relid, Form_pg_class reltuple);
+static void truncate_check_perms(Oid relid, Form_pg_class reltuple);
 static void truncate_check_activity(Relation rel);
 static void RangeVarCallbackForTruncate(const RangeVar *relation,
 										Oid relId, Oid oldRelId, void *arg);
@@ -1575,6 +1576,10 @@ ExecuteTruncate(TruncateStmt *stmt)
 				}
 
 				truncate_check_rel(RelationGetRelid(rel), rel->rd_rel);
+				/*
+				 * We skip checking the child's permission, because we
+				 * already checked the parent's.
+				 */
 				truncate_check_activity(rel);
 
 				rels = lappend(rels, rel);
@@ -1660,6 +1665,7 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
 						(errmsg("truncate cascades to table \"%s\"",
 								RelationGetRelationName(rel))));
 				truncate_check_rel(relid, rel->rd_rel);
+				truncate_check_perms(relid, rel->rd_rel);
 				truncate_check_activity(rel);
 				rels = lappend(rels, rel);
 				relids = lappend_oid(relids, relid);
@@ -1910,7 +1916,6 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
 static void
 truncate_check_rel(Oid relid, Form_pg_class reltuple)
 {
-	AclResult	aclresult;
 	char	   *relname = NameStr(reltuple->relname);
 
 	/*
@@ -1924,12 +1929,6 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("\"%s\" is not a table", relname)));
 
-	/* Permissions checks */
-	aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
-	if (aclresult != ACLCHECK_OK)
-		aclcheck_error(aclresult, get_relkind_objtype(reltuple->relkind),
-					   relname);
-
 	if (!allowSystemTableMods && IsSystemClass(relid, reltuple))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -1939,6 +1938,22 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
 	InvokeObjectTruncateHook(relid);
 }
 
+/*
+ * Check that current user has the permission to truncate given relation.
+ */
+static void
+truncate_check_perms(Oid relid, Form_pg_class reltuple)
+{
+	char	   *relname = NameStr(reltuple->relname);
+	AclResult	aclresult;
+
+	/* Permissions checks */
+	aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
+	if (aclresult != ACLCHECK_OK)
+		aclcheck_error(aclresult, get_relkind_objtype(reltuple->relkind),
+					   relname);
+}
+
 /*
  * Set of extra sanity checks to check if a given relation is safe to
  * truncate.  This is split with truncate_check_rel() as
@@ -14799,6 +14814,7 @@ RangeVarCallbackForTruncate(const RangeVar *relation,
 		elog(ERROR, "cache lookup failed for relation %u", relId);
 
 	truncate_check_rel(relId, (Form_pg_class) GETSTRUCT(tuple));
+	truncate_check_perms(relId, (Form_pg_class) GETSTRUCT(tuple));
 
 	ReleaseSysCache(tuple);
 }
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 0ddbd8e89f..ac5c5e921c 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -695,6 +695,29 @@ SELECT tableoid FROM atestp2; -- ok
 ----------
 (0 rows)
 
+-- child's permissions do not apply when operating on parent
+SET SESSION AUTHORIZATION regress_priv_user1;
+REVOKE ALL ON atestc FROM regress_priv_user2;
+GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2;
+REVOKE TRUNCATE ON atestc FROM regress_priv_user2;
+GRANT ALL ON atestp1 TO regress_priv_user2;
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT f2 FROM atestp1; -- ok
+ f2 
+----
+(0 rows)
+
+SELECT f2 FROM atestc; -- fail
+ERROR:  permission denied for table atestc
+DELETE FROM atestp1; -- ok
+DELETE FROM atestc; -- fail
+ERROR:  permission denied for table atestc
+UPDATE atestp1 SET f1 = 1; -- ok
+UPDATE atestc SET f1 = 1; -- fail
+ERROR:  permission denied for table atestc
+TRUNCATE atestp1; -- ok
+TRUNCATE atestc; -- fail
+ERROR:  permission denied for table atestc
 -- privileges on functions, languages
 -- switch to superuser
 \c -
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index f15d1f3773..757bbc4983 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -446,6 +446,22 @@ SELECT fy FROM atestp2; -- ok
 SELECT atestp2 FROM atestp2; -- ok
 SELECT tableoid FROM atestp2; -- ok
 
+-- child's permissions do not apply when operating on parent
+SET SESSION AUTHORIZATION regress_priv_user1;
+REVOKE ALL ON atestc FROM regress_priv_user2;
+GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2;
+REVOKE TRUNCATE ON atestc FROM regress_priv_user2;
+GRANT ALL ON atestp1 TO regress_priv_user2;
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT f2 FROM atestp1; -- ok
+SELECT f2 FROM atestc; -- fail
+DELETE FROM atestp1; -- ok
+DELETE FROM atestc; -- fail
+UPDATE atestp1 SET f1 = 1; -- ok
+UPDATE atestc SET f1 = 1; -- fail
+TRUNCATE atestp1; -- ok
+TRUNCATE atestc; -- fail
+
 -- privileges on functions, languages
 
 -- switch to superuser
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.