Date: Wed, 14 Dec 2022 09:34:35 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
	Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@postgresql.org
Subject: Re: fix and document CLUSTER privileges
Message-ID: <20221214173435.GA690225@nathanxps13>
References: <20221207223924.GA4182184@nathanxps13>
 <20221208022559.GA27893@telsasoft.com>
 <20221208041313.GA216874@nathanxps13>
 <b4d634cf-d893-f0f1-74ce-07675317575c@dunslane.net>
 <20221208181324.GA4385@nathanxps13>
 <CA+TgmoaWhAjTP3a9xjA2P=xkF-RV9UOYT11JVWgQq5A8AB9ZrA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="IS0zKkzwUGydFO0o"
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmoaWhAjTP3a9xjA2P=xkF-RV9UOYT11JVWgQq5A8AB9ZrA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221214173435.GA690225%40nathanxps13>
Precedence: bulk


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

On Thu, Dec 08, 2022 at 04:08:40PM -0500, Robert Haas wrote:
> On Thu, Dec 8, 2022 at 1:13 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Currently, CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX (minus REINDEX
>> SCHEMA|DATABASE|SYSTEM) require ownership of the relation or superuser.  In
>> fact, all three use the same RangeVarCallbackOwnsTable() callback function.
>> My current thinking is that this is good enough.  I don't sense any strong
>> demand for allowing database owners to run these commands on all non-shared
>> relations, and there's ongoing work to break out the privileges to GRANT
>> and predefined roles.
> 
> +1.
> 
> I don't see why being the database owner should give you the right to
> run a random subset of commands on any table in the database. Tables
> have their own system for access privileges; we should use that, or
> extend it as required.

Here is a rebased version of the patch.

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

--IS0zKkzwUGydFO0o
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="fix_cluster_privs_v2.patch"

diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 145101e6a5..d6b2651657 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 has the <literal>MAINTAIN</literal> privilege for, or all such tables
    if called by a superuser or a role with privileges of the
@@ -134,6 +135,16 @@ CLUSTER [VERBOSE]
  <refsect1>
   <title>Notes</title>
 
+   <para>
+    To cluster a table, one must have the <literal>MAINTAIN</literal> privilege
+    on the table or be the table's owner, a superuser, or a role with
+    privileges of the
+    <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link>
+    role.  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 8966b75bd1..8140a90699 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1697,9 +1697,7 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
 
 		/* Silently skip partitions which the user has no access to. */
 		if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
-			pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
-			(!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
-			 IsSharedRelation(relid)))
+			pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
 			continue;
 
 		/* Use a permanent memory context for the result list */

--IS0zKkzwUGydFO0o--