Date: Wed, 7 Dec 2022 14:39:24 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: pgsql-hackers@postgresql.org
Subject: fix and document CLUSTER privileges
Message-ID: <20221207223924.GA4182184@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="mYCpIKhGyMATD0i+"
Content-Disposition: inline
Archived-At: 
 <https://www.postgresql.org/message-id/20221207223924.GA4182184%40nathanxps13>
Precedence: bulk


--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi hackers,

While looking into other opportunities for per-table permissions, I noticed
a weird discrepancy in CLUSTER.  When evaluating whether the current user
has permission to CLUSTER a table, we ordinarily just check for ownership.
However, the database owner is also allowed to CLUSTER all partitions that
are not shared.  This was added in 3f19e17, and I didn't see any discussion
about it in the corresponding thread [0].

My first instinct is that we should just remove the database ownership
check, which is what I've done in the attached patch.  I don't see any
strong reason to complicate matters with special
database-owner-but-not-shared checks like other commands (e.g., VACUUM).
But perhaps we should do so just for consistency's sake.  Thoughts?

It was also noted elsewhere [1] that the privilege requirements for CLUSTER
are not documented.  The attached patch adds such documentation.

[0] https://postgr.es/m/20220411140609.GF26620%40telsasoft.com
[1] https://postgr.es/m/661148f4-c7f1-dec1-2bc8-29f3bd58e242%40postgrespro.ru

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--mYCpIKhGyMATD0i+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="fix_cluster_privs.patch"

diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index c37f4236f1..feb61e6ec2 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -67,7 +67,8 @@ CLUSTER [VERBOSE]
   </para>
 
   <para>
-   <command>CLUSTER</command> without any parameter reclusters all the
+   <command>CLUSTER</command> without a
+   <replaceable class="parameter">table_name</replaceable> reclusters all the
    previously-clustered tables in the current database that the calling user
    owns, or all such tables if called by a superuser.  This
    form of <command>CLUSTER</command> cannot be executed inside a transaction
@@ -132,6 +133,13 @@ CLUSTER [VERBOSE]
  <refsect1>
   <title>Notes</title>
 
+   <para>
+    To cluster a table, one must be the table's owner or a superuser.
+    Database-wide clusters and clusters on partitioned tables will silently
+    skip over any tables that the calling user does not have permission to
+    cluster.
+   </para>
+
    <para>
     In cases where you are accessing single rows randomly
     within a table, the actual order of the data in the
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 07e091bb87..a8a51cc674 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1693,9 +1693,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
 			continue;
 
 		/* Silently skip partitions which the user has no access to. */
-		if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
-			(!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
-			 IsSharedRelation(relid)))
+		if (!object_ownercheck(RelationRelationId, relid, GetUserId()))
 			continue;
 
 		/* Use a permanent memory context for the result list */

--mYCpIKhGyMATD0i+--