From 9c0e68f0ef7cb5f3ef23851f9903a191b95c920b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Villemain?= Date: Wed, 26 Mar 2025 10:03:38 +0100 Subject: [PATCH] Add finer control in EnsureTablePermission Previously this function was only checking ACL at table level, however for propagating GRANT on attributes, it is required to lock the shard while the user may not have table privilege at all (but attribute level ones). In order to solve that, I add a new parameter to the function: the ACL mask to be used when checking attributes acl: ----- Check that the current user has `mode` permissions on relationId. If not, also check relationId's attributes with `mask`, error out privileges are not defined. ACL mask is used because we assume that user has enought privilege to distribute a table when either ACL_INSERT on the TABLE or ACL_INSERT on ALL attributes. In other situations, having a single attribute privilege is enough. ----- --- src/backend/distributed/metadata/metadata_utility.c | 11 ++++++++--- src/backend/distributed/metadata/node_metadata.c | 2 +- src/backend/distributed/operations/stage_protocol.c | 2 +- src/backend/distributed/utils/resource_lock.c | 8 +++++--- src/include/distributed/metadata_utility.h | 2 +- 5 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/backend/distributed/metadata/metadata_utility.c b/src/backend/distributed/metadata/metadata_utility.c index 7a8e4f7d8..fede8f752 100644 --- a/src/backend/distributed/metadata/metadata_utility.c +++ b/src/backend/distributed/metadata/metadata_utility.c @@ -2428,12 +2428,17 @@ UpdateNoneDistTableMetadata(Oid relationId, char replicationModel, uint32 coloca /* - * Check that the current user has `mode` permissions on relationId or on at - * least one relationId's attribute, error out if not. + * Check that the current user has `mode` permissions on relationId. + * If not, also check relationId's attributes with `mask`, error out + * privileges are not defined. + * ACL mask is used because we assume that user has enought privilege + * to distribute a table when either ACL_INSERT on the TABLE or + * ACL_INSERT on ALL attributes. + * In other situations, having a single attribute privilege is enough. * Superusers always have such permissions. */ void -EnsureTablePermissions(Oid relationId, AclMode mode) +EnsureTablePermissions(Oid relationId, AclMode mode, AclMaskHow mask) { AclResult aclresult = pg_class_aclcheck(relationId, GetUserId(), mode); diff --git a/src/backend/distributed/metadata/node_metadata.c b/src/backend/distributed/metadata/node_metadata.c index d031f4d2c..c2a2abd60 100644 --- a/src/backend/distributed/metadata/node_metadata.c +++ b/src/backend/distributed/metadata/node_metadata.c @@ -1522,7 +1522,7 @@ get_shard_id_for_distribution_column(PG_FUNCTION_ARGS) } Oid relationId = PG_GETARG_OID(0); - EnsureTablePermissions(relationId, ACL_SELECT); + EnsureTablePermissions(relationId, ACL_SELECT, ACLMASK_ANY); if (!IsCitusTable(relationId)) { diff --git a/src/backend/distributed/operations/stage_protocol.c b/src/backend/distributed/operations/stage_protocol.c index 9881d8775..976f1549f 100644 --- a/src/backend/distributed/operations/stage_protocol.c +++ b/src/backend/distributed/operations/stage_protocol.c @@ -108,7 +108,7 @@ master_create_empty_shard(PG_FUNCTION_ARGS) Oid relationId = ResolveRelationId(relationNameText, false); - EnsureTablePermissions(relationId, ACL_INSERT); + EnsureTablePermissions(relationId, ACL_INSERT, ACLMASK_ALL); CheckDistributedTable(relationId); /* diff --git a/src/backend/distributed/utils/resource_lock.c b/src/backend/distributed/utils/resource_lock.c index 3f50b682e..012dc1079 100644 --- a/src/backend/distributed/utils/resource_lock.c +++ b/src/backend/distributed/utils/resource_lock.c @@ -222,10 +222,12 @@ lock_shard_resources(PG_FUNCTION_ARGS) * on the executor. However, for INSERTs, the user might have only * INSERTs granted, so add a special case for it. */ - AclMode aclMask = ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE; + AclMode aclMode = ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE; + AclMaskHow aclMaskHow = ACLMASK_ANY; + if (lockMode == RowExclusiveLock) { - aclMask |= ACL_INSERT; + aclMode |= ACL_INSERT; } for (int shardIdIndex = 0; shardIdIndex < shardIdCount; shardIdIndex++) @@ -254,7 +256,7 @@ lock_shard_resources(PG_FUNCTION_ARGS) if (!SkipAdvisoryLockPermissionChecks) { - EnsureTablePermissions(relationId, aclMask); + EnsureTablePermissions(relationId, aclMode, aclMaskHow); } LockShardResource(shardId, lockMode); diff --git a/src/include/distributed/metadata_utility.h b/src/include/distributed/metadata_utility.h index 737e1283b..38c13eb51 100644 --- a/src/include/distributed/metadata_utility.h +++ b/src/include/distributed/metadata_utility.h @@ -400,7 +400,7 @@ extern bool ShouldPropagateAnyObject(List *addresses); /* Remaining metadata utility functions */ extern Oid TableOwnerOid(Oid relationId); extern char * TableOwner(Oid relationId); -extern void EnsureTablePermissions(Oid relationId, AclMode mode); +extern void EnsureTablePermissions(Oid relationId, AclMode mode, AclMaskHow mask); extern void EnsureTableOwner(Oid relationId); extern void EnsureHashDistributedTable(Oid relationId); extern void EnsureHashOrSingleShardDistributedTable(Oid relationId);