Merge pull request #2563 from citusdata/transactions_pgmonitor

Restrict visibility of get_*_active_transactions functions to pg_monitor

src/backend/distributed/transaction/backend_data.c

@@ -15,6 +15,7 @@
 
 #include "funcapi.h"
 #include "access/htup_details.h"
+#include "catalog/pg_authid.h"
 #include "catalog/pg_type.h"
 #include "datatype/timestamp.h"
 #include "distributed/backend_data.h"
@@ -94,6 +95,8 @@ PG_FUNCTION_INFO_V1(get_all_active_transactions);
 Datum
 assign_distributed_transaction_id(PG_FUNCTION_ARGS)
 {
+	Oid userId = GetUserId();
+
 	CheckCitusVersion(ERROR);
 
 	/* MyBackendData should always be avaliable, just out of paranoia */
@@ -120,6 +123,7 @@ assign_distributed_transaction_id(PG_FUNCTION_ARGS)
 	}
 
 	MyBackendData->databaseId = MyDatabaseId;
+	MyBackendData->userId = userId;
 
 	MyBackendData->transactionId.initiatorNodeIdentifier = PG_GETARG_INT32(0);
 	MyBackendData->transactionId.transactionNumber = PG_GETARG_INT64(1);
@@ -385,6 +389,8 @@ StoreAllActiveTransactions(Tuplestorestate *tupleStore, TupleDesc tupleDescripto
 
 	Datum values[ACTIVE_TRANSACTION_COLUMN_COUNT];
 	bool isNulls[ACTIVE_TRANSACTION_COLUMN_COUNT];
+	bool showAllTransactions = superuser();
+	const Oid userId = GetUserId();
 
 	/*
 	 * We don't want to initialize memory while spinlock is held so we
@@ -394,6 +400,11 @@ StoreAllActiveTransactions(Tuplestorestate *tupleStore, TupleDesc tupleDescripto
 	memset(values, 0, sizeof(values));
 	memset(isNulls, false, sizeof(isNulls));
 
+	if (is_member_of_role(userId, DEFAULT_ROLE_MONITOR))
+	{
+		showAllTransactions = true;
+	}
+
 	/* we're reading all distributed transactions, prevent new backends */
 	LockBackendSharedMemory(LW_SHARED);
 
@@ -412,6 +423,16 @@ StoreAllActiveTransactions(Tuplestorestate *tupleStore, TupleDesc tupleDescripto
 			continue;
 		}
 
+		/*
+		 * Unless the user has a role that allows seeing all transactions (superuser,
+		 * pg_monitor), skip over transactions belonging to other users.
+		 */
+		if (!showAllTransactions && currentBackend->userId != userId)
+		{
+			SpinLockRelease(&currentBackend->mutex);
+			continue;
+		}
+
 		values[0] = ObjectIdGetDatum(currentBackend->databaseId);
 		values[1] = Int32GetDatum(ProcGlobal->allProcs[backendIndex].pid);
 		values[2] = Int32GetDatum(currentBackend->citusBackend.initiatorNodeIdentifier);
@@ -693,6 +714,7 @@ UnSetDistributedTransactionId(void)
 		SpinLockAcquire(&MyBackendData->mutex);
 
 		MyBackendData->databaseId = 0;
+		MyBackendData->userId = 0;
 		MyBackendData->transactionId.initiatorNodeIdentifier = 0;
 		MyBackendData->transactionId.transactionOriginator = false;
 		MyBackendData->transactionId.transactionNumber = 0;
@@ -781,10 +803,12 @@ AssignDistributedTransactionId(void)
 	uint64 nextTransactionNumber = pg_atomic_fetch_add_u64(transactionNumberSequence, 1);
 	int localGroupId = GetLocalGroupId();
 	TimestampTz currentTimestamp = GetCurrentTimestamp();
+	Oid userId = GetUserId();
 
 	SpinLockAcquire(&MyBackendData->mutex);
 
 	MyBackendData->databaseId = MyDatabaseId;
+	MyBackendData->userId = userId;
 
 	MyBackendData->transactionId.initiatorNodeIdentifier = localGroupId;
 	MyBackendData->transactionId.transactionOriginator = true;

src/include/distributed/backend_data.h

@@ -47,6 +47,7 @@ typedef struct CitusInitiatedBackend
 typedef struct BackendData
 {
 	Oid databaseId;
+	Oid userId;
 	slock_t mutex;
 	bool cancelledDueToDeadlock;
 	CitusInitiatedBackend citusBackend;

src/test/regress/expected/isolation_get_all_active_transactions.out

@@ -0,0 +1,87 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1-grant s1-begin-insert s2-begin-insert s3-as-admin s3-as-user-1 s3-as-readonly s3-as-monitor s1-commit s2-commit
+run_command_on_workers
+
+(localhost,57637,t,"GRANT ROLE")
+(localhost,57638,t,"GRANT ROLE")
+step s1-grant: 
+	GRANT ALL ON test_table TO test_user_1;
+	SELECT bool_and(success) FROM run_command_on_placements('test_table', 'GRANT ALL ON TABLE %s TO test_user_1');
+
+	GRANT ALL ON test_table TO test_user_2;
+	SELECT bool_and(success) FROM run_command_on_placements('test_table', 'GRANT ALL ON TABLE %s TO test_user_2');
+
+bool_and       
+
+t              
+bool_and       
+
+t              
+step s1-begin-insert: 
+	BEGIN;
+	SET ROLE test_user_1;
+	INSERT INTO test_table VALUES (100, 100);
+
+step s2-begin-insert: 
+	BEGIN;
+	SET ROLE test_user_2;
+	INSERT INTO test_table VALUES (200, 200);
+
+step s3-as-admin: 
+	-- Admin should be able to see all transactions
+	SELECT count(*) FROM get_all_active_transactions();
+	SELECT count(*) FROM get_global_active_transactions();
+
+count          
+
+2              
+count          
+
+4              
+step s3-as-user-1: 
+	-- User should only be able to see its own transactions
+	SET ROLE test_user_1;
+	SELECT count(*) FROM get_all_active_transactions();
+	SELECT count(*) FROM get_global_active_transactions();
+
+count          
+
+1              
+count          
+
+2              
+step s3-as-readonly: 
+	-- Other user should not see transactions
+	SET ROLE test_readonly;
+	SELECT count(*) FROM get_all_active_transactions();
+	SELECT count(*) FROM get_global_active_transactions();
+
+count          
+
+0              
+count          
+
+0              
+step s3-as-monitor: 
+	-- Monitor should see all transactions
+	SET ROLE test_monitor;
+	SELECT count(*) FROM get_all_active_transactions();
+	SELECT count(*) FROM get_global_active_transactions();
+
+count          
+
+2              
+count          
+
+4              
+step s1-commit: 
+	COMMIT;
+
+step s2-commit: 
+	COMMIT;
+
+run_command_on_workers
+
+(localhost,57637,f,"ERROR:  role ""test_user_1"" cannot be dropped because some objects depend on it")
+(localhost,57638,f,"ERROR:  role ""test_user_1"" cannot be dropped because some objects depend on it")

src/test/regress/isolation_schedule

@@ -43,6 +43,7 @@ test: isolation_truncate_vs_all
 test: isolation_drop_vs_all
 test: isolation_ddl_vs_all
 test: isolation_citus_dist_activity
+test: isolation_get_all_active_transactions
 test: isolation_validate_vs_insert
 test: isolation_insert_select_conflict
 

src/test/regress/specs/isolation_get_all_active_transactions.spec

@@ -0,0 +1,102 @@
+setup
+{
+	SET citus.shard_replication_factor TO 1;
+
+	CREATE TABLE test_table(column1 int, column2 int);
+	SELECT create_distributed_table('test_table', 'column1');
+
+	CREATE USER test_user_1;
+	SELECT run_command_on_workers('CREATE USER test_user_1');
+
+	CREATE USER test_user_2;
+	SELECT run_command_on_workers('CREATE USER test_user_2');
+
+	CREATE USER test_readonly;
+	SELECT run_command_on_workers('CREATE USER test_readonly');
+
+	CREATE USER test_monitor;
+	SELECT run_command_on_workers('CREATE USER test_monitor');
+
+	GRANT pg_monitor TO test_monitor;
+	SELECT run_command_on_workers('GRANT pg_monitor TO test_monitor');
+}
+
+teardown
+{
+	DROP TABLE test_table;
+	DROP USER test_user_1, test_user_2, test_readonly, test_monitor;
+	SELECT run_command_on_workers('DROP USER test_user_1, test_user_2, test_readonly, test_monitor');
+}
+
+session "s1"
+
+# run_command_on_placements is done in a separate step because the setup is executed as a single transaction
+step "s1-grant"
+{
+	GRANT ALL ON test_table TO test_user_1;
+	SELECT bool_and(success) FROM run_command_on_placements('test_table', 'GRANT ALL ON TABLE %s TO test_user_1');
+
+	GRANT ALL ON test_table TO test_user_2;
+	SELECT bool_and(success) FROM run_command_on_placements('test_table', 'GRANT ALL ON TABLE %s TO test_user_2');
+}
+
+step "s1-begin-insert"
+{
+	BEGIN;
+	SET ROLE test_user_1;
+	INSERT INTO test_table VALUES (100, 100);
+}
+
+step "s1-commit"
+{
+	COMMIT;
+}
+
+session "s2"
+
+step "s2-begin-insert"
+{
+	BEGIN;
+	SET ROLE test_user_2;
+	INSERT INTO test_table VALUES (200, 200);
+}
+
+step "s2-commit"
+{
+	COMMIT;
+}
+
+session "s3"
+
+step "s3-as-admin"
+{
+	-- Admin should be able to see all transactions
+	SELECT count(*) FROM get_all_active_transactions();
+	SELECT count(*) FROM get_global_active_transactions();
+}
+
+step "s3-as-user-1"
+{
+	-- User should only be able to see its own transactions
+	SET ROLE test_user_1;
+	SELECT count(*) FROM get_all_active_transactions();
+	SELECT count(*) FROM get_global_active_transactions();
+}
+
+step "s3-as-readonly"
+{
+	-- Other user should not see transactions
+	SET ROLE test_readonly;
+	SELECT count(*) FROM get_all_active_transactions();
+	SELECT count(*) FROM get_global_active_transactions();
+}
+
+step "s3-as-monitor"
+{
+	-- Monitor should see all transactions
+	SET ROLE test_monitor;
+	SELECT count(*) FROM get_all_active_transactions();
+	SELECT count(*) FROM get_global_active_transactions();
+}
+
+permutation "s1-grant" "s1-begin-insert" "s2-begin-insert" "s3-as-admin" "s3-as-user-1" "s3-as-readonly" "s3-as-monitor" "s1-commit" "s2-commit"