Make sure to prevent unauthorized users to drop sequences in Citus MX

src/backend/distributed/master/master_delete_protocol.c

@@ -58,6 +58,9 @@
 #include "utils/elog.h"
 #include "utils/errcodes.h"
 #include "utils/lsyscache.h"
+#if (PG_VERSION_NUM >= 100000)
+#include "utils/varlena.h"
+#endif
 
 
 /* Local functions forward declarations */
@@ -265,29 +268,51 @@ master_drop_sequences(PG_FUNCTION_ARGS)
 {
 	ArrayType *sequenceNamesArray = PG_GETARG_ARRAYTYPE_P(0);
 	ArrayIterator sequenceIterator = NULL;
-	Datum sequenceText = 0;
+	Datum sequenceNameDatum = 0;
 	bool isNull = false;
 	StringInfo dropSeqCommand = makeStringInfo();
-	bool coordinator = IsCoordinator();
 
 	CheckCitusVersion(ERROR);
 
-	/* do nothing if DDL propagation is switched off or this is not the coordinator */
+	/*
-	if (!EnableDDLPropagation || !coordinator)
+	 * Do nothing if DDL propagation is switched off or we're not on
+	 * the coordinator. Here we prefer to not error out on the workers
+	 * because this function is called on every dropped sequence and
+	 * we don't want to mess up the sequences that are not associated
+	 * with distributed tables.
+	 */
+	if (!EnableDDLPropagation || !IsCoordinator())
 	{
 		PG_RETURN_VOID();
 	}
 
 	/* iterate over sequence names to build single command to DROP them all */
 	sequenceIterator = array_create_iterator(sequenceNamesArray, 0, NULL);
-	while (array_iterate(sequenceIterator, &sequenceText, &isNull))
+	while (array_iterate(sequenceIterator, &sequenceNameDatum, &isNull))
 	{
+		text *sequenceNameText = NULL;
+		Oid sequenceOid = InvalidOid;
+
 		if (isNull)
 		{
 			ereport(ERROR, (errmsg("unexpected NULL sequence name"),
 							errcode(ERRCODE_INVALID_PARAMETER_VALUE)));
 		}
 
+		sequenceNameText = DatumGetTextP(sequenceNameDatum);
+		sequenceOid = ResolveRelationId(sequenceNameText, true);
+		if (OidIsValid(sequenceOid))
+		{
+			/*
+			 * This case (e.g., OID is valid) could only happen when a user manually calls
+			 * the UDF. So, ensure that the user has right to drop the sequence.
+			 *
+			 * In case the UDF is called via the DROP trigger, the OID wouldn't be valid since
+			 * the trigger is called after DROP happens.
+			 */
+			EnsureSequenceOwner(sequenceOid);
+		}
+
 		/* append command portion if we haven't added any sequence names yet */
 		if (dropSeqCommand->len == 0)
 		{
@@ -299,7 +324,7 @@ master_drop_sequences(PG_FUNCTION_ARGS)
 			appendStringInfoChar(dropSeqCommand, ',');
 		}
 
-		appendStringInfo(dropSeqCommand, " %s", TextDatumGetCString(sequenceText));
+		appendStringInfo(dropSeqCommand, " %s", TextDatumGetCString(sequenceNameText));
 	}
 
 	if (dropSeqCommand->len != 0)

src/backend/distributed/master/master_metadata_utility.c

@@ -1346,6 +1346,21 @@ EnsureTableOwner(Oid relationId)
 }
 
 
+/*
+ * Check that the current user has owner rights to sequenceRelationId, error out if
+ * not. Superusers are regarded as owners.
+ */
+void
+EnsureSequenceOwner(Oid sequenceOid)
+{
+	if (!pg_class_ownercheck(sequenceOid, GetUserId()))
+	{
+		aclcheck_error(ACLCHECK_NOT_OWNER, ACLCHECK_OBJECT_SEQUENCE,
+					   get_rel_name(sequenceOid));
+	}
+}
+
+
 /*
  * EnsureSuperUser check that the current user is a superuser and errors out if not.
  */

src/include/distributed/master_metadata_utility.h

@@ -155,6 +155,7 @@ extern void CreateTruncateTrigger(Oid relationId);
 extern char * TableOwner(Oid relationId);
 extern void EnsureTablePermissions(Oid relationId, AclMode mode);
 extern void EnsureTableOwner(Oid relationId);
+extern void EnsureSequenceOwner(Oid sequenceOid);
 extern void EnsureSuperUser(void);
 extern void EnsureReplicationSettings(Oid relationId, char replicationModel);
 extern bool RegularTable(Oid relationId);

src/include/distributed/version_compat.h

@@ -67,6 +67,8 @@ struct QueryEnvironment; /* forward-declare to appease compiler */
 #define ACLCHECK_OBJECT_TABLE ACL_KIND_CLASS
 #define ACLCHECK_OBJECT_SCHEMA ACL_KIND_NAMESPACE
 #define ACLCHECK_OBJECT_INDEX ACL_KIND_CLASS
+#define ACLCHECK_OBJECT_SEQUENCE ACL_KIND_CLASS
+
 
 static inline int
 BasicOpenFilePerm(FileName fileName, int fileFlags, int fileMode)
@@ -158,6 +160,8 @@ canonicalize_qual_compat(Expr *qual, bool is_check)
 #define ACLCHECK_OBJECT_TABLE OBJECT_TABLE
 #define ACLCHECK_OBJECT_SCHEMA OBJECT_SCHEMA
 #define ACLCHECK_OBJECT_INDEX OBJECT_INDEX
+#define ACLCHECK_OBJECT_SEQUENCE OBJECT_SEQUENCE
+
 
 #define ConstraintRelidIndexId ConstraintRelidTypidNameIndexId
 

src/test/regress/expected/multi_mx_metadata.out

@@ -7,12 +7,25 @@ SELECT pg_reload_conf();
  t
 (1 row)
 
+-- pg 10 and pg 11 has different error messages for acl checks
+-- so catch them and print only one type of error message to prevent
+-- multiple output test files
+CREATE OR REPLACE FUNCTION raise_failed_aclcheck(query text) RETURNS void AS $$
+BEGIN
+    EXECUTE query;
+    EXCEPTION WHEN OTHERS THEN
+    IF SQLERRM LIKE 'must be owner of%' THEN
+        RAISE 'must be owner of the object';
+    END IF;
+END;
+$$LANGUAGE plpgsql;
 -- get rid of the previously created entries in pg_dist_transaction
 -- for the sake of getting consistent results in this test file
 TRUNCATE pg_dist_transaction;
 CREATE TABLE distributed_mx_table (
     key text primary key,
-    value jsonb
+    value jsonb,
+    some_val bigserial
 );
 CREATE INDEX ON distributed_mx_table USING GIN (value);
 SET citus.shard_replication_factor TO 1;
@@ -48,10 +61,11 @@ SELECT count(*) FROM pg_dist_transaction;
 \c - - - :worker_1_port
 SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass;
   Column  |  Type  |                                Modifiers                                
---------+-------+-----------
+----------+--------+-------------------------------------------------------------------------
  key      | text   | not null
  value    | jsonb  | 
-(2 rows)
+ some_val | bigint | not null default nextval('distributed_mx_table_some_val_seq'::regclass)
+(3 rows)
 
 SELECT "Column", "Type", "Definition" FROM index_attrs WHERE
     relid = 'distributed_mx_table_pkey'::regclass;
@@ -84,10 +98,11 @@ WHERE logicalrelid = 'distributed_mx_table'::regclass;
 \c - - - :worker_2_port
 SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass;
   Column  |  Type  |                                Modifiers                                
---------+-------+-----------
+----------+--------+-------------------------------------------------------------------------
  key      | text   | not null
  value    | jsonb  | 
-(2 rows)
+ some_val | bigint | not null default nextval('distributed_mx_table_some_val_seq'::regclass)
+(3 rows)
 
 SELECT "Column", "Type", "Definition" FROM index_attrs WHERE
     relid = 'distributed_mx_table_pkey'::regclass;
@@ -299,25 +314,120 @@ SELECT run_command_on_workers($$CREATE USER no_access_mx;$$);
 (2 rows)
 
 SET ROLE no_access_mx;
-DROP TABLE distributed_mx_table;
+SELECT raise_failed_aclcheck($$ 
-ERROR:  must be owner of table distributed_mx_table
+    DROP TABLE distributed_mx_table;
-SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+$$);
-ERROR:  must be owner of table distributed_mx_table
+ERROR:  must be owner of the object
-SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+CONTEXT:  PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE
-ERROR:  must be owner of table distributed_mx_table
+SELECT raise_failed_aclcheck($$ 
-SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+    SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
-ERROR:  must be owner of table distributed_mx_table
+ $$);
+ERROR:  must be owner of the object
+CONTEXT:  PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+$$);
+ERROR:  must be owner of the object
+CONTEXT:  PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+$$);
+ERROR:  must be owner of the object
+CONTEXT:  PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']);
+$$);
+ERROR:  must be owner of the object
+CONTEXT:  PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_drop_sequences(ARRAY['distributed_mx_table_some_val_seq']);
+$$);
+ERROR:  must be owner of the object
+CONTEXT:  PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE
+SELECT master_drop_sequences(ARRAY['non_existing_schema.distributed_mx_table_some_val_seq']);
+ master_drop_sequences 
+-----------------------
+ 
+(1 row)
+
+SELECT master_drop_sequences(ARRAY['']);
+ERROR:  invalid name syntax
+SELECT master_drop_sequences(ARRAY['public.']);
+ERROR:  invalid name syntax
+SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq_not_existing']);
+ master_drop_sequences 
+-----------------------
+ 
+(1 row)
+
+-- make sure that we can drop unrelated tables/sequences
+CREATE TABLE unrelated_table(key serial);
+DROP TABLE unrelated_table;
+-- doesn't error out but it has no effect, so no need to error out
+SELECT master_drop_sequences(NULL);
+ master_drop_sequences 
+-----------------------
+ 
+(1 row)
+
+\c - postgres - :master_port
+-- finally make sure that the sequence remains
+SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass;
+  Column  |  Type  |                                Modifiers                                
+----------+--------+-------------------------------------------------------------------------
+ key      | text   | not null
+ value    | jsonb  | 
+ some_val | bigint | not null default nextval('distributed_mx_table_some_val_seq'::regclass)
+(3 rows)
+
 \c - no_access_mx - :worker_1_port
-DROP TABLE distributed_mx_table;
+-- see the comment in the top of the file
-ERROR:  must be owner of table distributed_mx_table
+CREATE OR REPLACE FUNCTION raise_failed_aclcheck(query text) RETURNS void AS $$
-SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+BEGIN
-ERROR:  must be owner of table distributed_mx_table
+    EXECUTE query;
+    EXCEPTION WHEN OTHERS THEN
+    IF SQLERRM LIKE 'must be owner of%' THEN
+        RAISE 'must be owner of the object';
+    END IF;
+END;
+$$LANGUAGE plpgsql;
+SELECT raise_failed_aclcheck($$ 
+    DROP TABLE distributed_mx_table;
+$$);
+ERROR:  must be owner of the object
+CONTEXT:  PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+ $$);
+ERROR:  must be owner of the object
+CONTEXT:  PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']);
+$$);
+ raise_failed_aclcheck 
+-----------------------
+ 
+(1 row)
+
 SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
 ERROR:  operation is not allowed on this node
 HINT:  Connect to the coordinator and run it again.
 SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
 ERROR:  operation is not allowed on this node
 HINT:  Connect to the coordinator and run it again.
+-- make sure that we can drop unrelated tables/sequences
+CREATE TABLE unrelated_table(key serial);
+DROP TABLE unrelated_table;
+\c - postgres - :worker_1_port
+-- finally make sure that the sequence remains
+SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass;
+  Column  |  Type  |                                Modifiers                                
+----------+--------+-------------------------------------------------------------------------
+ key      | text   | not null
+ value    | jsonb  | 
+ some_val | bigint | not null default nextval('distributed_mx_table_some_val_seq'::regclass)
+(3 rows)
+
 -- Resume ordinary recovery
 \c - postgres - :master_port
 ALTER SYSTEM RESET citus.recover_2pc_interval;

src/test/regress/sql/multi_mx_metadata.sql

@@ -4,13 +4,27 @@
 ALTER SYSTEM SET citus.recover_2pc_interval TO -1;
 SELECT pg_reload_conf();
 
+-- pg 10 and pg 11 has different error messages for acl checks
+-- so catch them and print only one type of error message to prevent
+-- multiple output test files
+CREATE OR REPLACE FUNCTION raise_failed_aclcheck(query text) RETURNS void AS $$
+BEGIN
+    EXECUTE query;
+    EXCEPTION WHEN OTHERS THEN
+    IF SQLERRM LIKE 'must be owner of%' THEN
+        RAISE 'must be owner of the object';
+    END IF;
+END;
+$$LANGUAGE plpgsql;
+
 -- get rid of the previously created entries in pg_dist_transaction
 -- for the sake of getting consistent results in this test file
 TRUNCATE pg_dist_transaction;
 
 CREATE TABLE distributed_mx_table (
     key text primary key,
-    value jsonb
+    value jsonb,
+    some_val bigserial
 );
 CREATE INDEX ON distributed_mx_table USING GIN (value);
 
@@ -181,16 +195,81 @@ CREATE USER no_access_mx;
 SELECT run_command_on_workers($$CREATE USER no_access_mx;$$);
 
 SET ROLE no_access_mx;
-DROP TABLE distributed_mx_table;
+
-SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+SELECT raise_failed_aclcheck($$ 
+    DROP TABLE distributed_mx_table;
+$$);
+
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+ $$);
+
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+$$);
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+$$);
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']);
+$$);
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_drop_sequences(ARRAY['distributed_mx_table_some_val_seq']);
+$$);
+
+SELECT master_drop_sequences(ARRAY['non_existing_schema.distributed_mx_table_some_val_seq']);
+SELECT master_drop_sequences(ARRAY['']);
+SELECT master_drop_sequences(ARRAY['public.']);
+SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq_not_existing']);
+
+-- make sure that we can drop unrelated tables/sequences
+CREATE TABLE unrelated_table(key serial);
+DROP TABLE unrelated_table;
+
+-- doesn't error out but it has no effect, so no need to error out
+SELECT master_drop_sequences(NULL);
+
+\c - postgres - :master_port
+
+-- finally make sure that the sequence remains
+SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass;
+
+\c - no_access_mx - :worker_1_port
+
+-- see the comment in the top of the file
+CREATE OR REPLACE FUNCTION raise_failed_aclcheck(query text) RETURNS void AS $$
+BEGIN
+    EXECUTE query;
+    EXCEPTION WHEN OTHERS THEN
+    IF SQLERRM LIKE 'must be owner of%' THEN
+        RAISE 'must be owner of the object';
+    END IF;
+END;
+$$LANGUAGE plpgsql;
+
+SELECT raise_failed_aclcheck($$ 
+    DROP TABLE distributed_mx_table;
+$$);
+
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+ $$);
+
+SELECT raise_failed_aclcheck($$ 
+    SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']);
+$$);
+
 SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
 SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
 
-\c - no_access_mx - :worker_1_port
+-- make sure that we can drop unrelated tables/sequences
-DROP TABLE distributed_mx_table;
+CREATE TABLE unrelated_table(key serial);
-SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+DROP TABLE unrelated_table;
-SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+
-SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table');
+\c - postgres - :worker_1_port
+
+-- finally make sure that the sequence remains
+SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass;
 
 -- Resume ordinary recovery
 \c - postgres - :master_port