From 500c2259992ca6ea166ae7cd698e0dfcf76f77ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabr=C3=ADzio=20de=20Royes=20Mello?= Date: Fri, 5 Aug 2022 15:19:24 -0300 Subject: [PATCH] Handle properly default privileges on CAggs If a default privilege is configured and applied to a given Continuous Aggregate during it creation just the user view has the ACL properly configured but the underlying materialization hypertable no leading to permission errors. Fixed it by copying the privileges from the user view to the materialization hypertable during the Continous Aggregate creation. Fixes #4555 --- CHANGELOG.md | 2 + src/chunk.c | 66 +----------------------- src/utils.c | 70 ++++++++++++++++++++++++++ src/utils.h | 2 + tsl/src/continuous_aggs/create.c | 5 +- tsl/test/expected/cagg_permissions.out | 37 +++++++++++++- tsl/test/sql/cagg_permissions.sql | 32 +++++++++++- 7 files changed, 145 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63466f20998..4f4ab5a3924 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,11 +13,13 @@ accidentally triggering the load of a previous DB version.** **Bugfixes** * #4486 Adding boolean column with default value doesn't work on compressed table +* #4555 Handle properly default privileges on Continuous Aggregates * #4575 Fix use of `get_partition_hash` and `get_partition_for_key` inside an IMMUTABLE function **Thanks** @janko for reporting @AlmiS for reporting error on `get_partition_hash` executed inside an IMMUTABLE function +@michaelkitson for reporting permission errors using default privileges on Continuous Aggregates ## 2.7.2 (2022-07-26) diff --git a/src/chunk.c b/src/chunk.c index 4539dc571b6..7b24e7f9f56 100644 --- a/src/chunk.c +++ b/src/chunk.c @@ -737,71 +737,9 @@ get_am_name_for_rel(Oid relid) } static void -copy_hypertable_acl_to_relid(const Hypertable *ht, Oid owner_id, Oid relid) +copy_hypertable_acl_to_relid(const Hypertable *ht, const Oid owner_id, const Oid relid) { - HeapTuple ht_tuple; - bool is_null; - Datum acl_datum; - Relation class_rel; - - /* We open it here since there is no point in trying to update the tuples - * if we cannot open the Relation catalog table */ - class_rel = table_open(RelationRelationId, RowExclusiveLock); - - ht_tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(ht->main_table_relid)); - Assert(HeapTupleIsValid(ht_tuple)); - - /* We only bother about setting the chunk ACL if the hypertable ACL is - * non-null */ - acl_datum = SysCacheGetAttr(RELOID, ht_tuple, Anum_pg_class_relacl, &is_null); - if (!is_null) - { - HeapTuple chunk_tuple, newtuple; - Datum new_val[Natts_pg_class] = { 0 }; - bool new_null[Natts_pg_class] = { false }; - bool new_repl[Natts_pg_class] = { false }; - Acl *acl = DatumGetAclP(acl_datum); - - new_repl[Anum_pg_class_relacl - 1] = true; - new_val[Anum_pg_class_relacl - 1] = PointerGetDatum(acl); - - /* Find the tuple for the chunk in `pg_class` */ - chunk_tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); - Assert(HeapTupleIsValid(chunk_tuple)); - - /* Update the relacl for the chunk tuple to use the acl from the hypertable */ - newtuple = heap_modify_tuple(chunk_tuple, - RelationGetDescr(class_rel), - new_val, - new_null, - new_repl); - CatalogTupleUpdate(class_rel, &newtuple->t_self, newtuple); - - /* We need to update the shared dependencies as well to indicate that - * the chunk is dependent on any roles that the hypertable is - * dependent on. */ - Oid *newmembers; - int nnewmembers = aclmembers(acl, &newmembers); - - /* The list of old members is intentionally empty since we are using - * updateAclDependencies to set the ACL for the chunk. We can use NULL - * because getOidListDiff, which is called from updateAclDependencies, - * can handle that. */ - updateAclDependencies(RelationRelationId, - relid, - 0, - owner_id, - 0, - NULL, - nnewmembers, - newmembers); - - heap_freetuple(newtuple); - ReleaseSysCache(chunk_tuple); - } - - ReleaseSysCache(ht_tuple); - table_close(class_rel, RowExclusiveLock); + ts_copy_relation_acl(ht->main_table_relid, relid, owner_id); } /* diff --git a/src/utils.c b/src/utils.c index dda245b8413..d10101cb06f 100644 --- a/src/utils.c +++ b/src/utils.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -1197,3 +1198,72 @@ ts_alter_table_with_event_trigger(Oid relid, Node *cmd, List *cmds, bool recurse AlterTableInternal(relid, cmds, recurse); EventTriggerAlterTableEnd(); } + +void +ts_copy_relation_acl(const Oid source_relid, const Oid target_relid, const Oid owner_id) +{ + HeapTuple source_tuple; + bool is_null; + Datum acl_datum; + Relation class_rel; + + /* We open it here since there is no point in trying to update the tuples + * if we cannot open the Relation catalog table */ + class_rel = table_open(RelationRelationId, RowExclusiveLock); + + source_tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(source_relid)); + Assert(HeapTupleIsValid(source_tuple)); + + /* We only bother about setting the ACL if the source relation ACL is + * non-null */ + acl_datum = SysCacheGetAttr(RELOID, source_tuple, Anum_pg_class_relacl, &is_null); + + if (!is_null) + { + HeapTuple target_tuple, newtuple; + Datum new_val[Natts_pg_class] = { 0 }; + bool new_null[Natts_pg_class] = { false }; + bool new_repl[Natts_pg_class] = { false }; + Acl *acl = DatumGetAclP(acl_datum); + + new_repl[Anum_pg_class_relacl - 1] = true; + new_val[Anum_pg_class_relacl - 1] = PointerGetDatum(acl); + + /* Find the tuple for the target in `pg_class` */ + target_tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(target_relid)); + Assert(HeapTupleIsValid(target_tuple)); + + /* Update the relacl for the target tuple to use the acl from the source */ + newtuple = heap_modify_tuple(target_tuple, + RelationGetDescr(class_rel), + new_val, + new_null, + new_repl); + CatalogTupleUpdate(class_rel, &newtuple->t_self, newtuple); + + /* We need to update the shared dependencies as well to indicate that + * the target is dependent on any roles that the source is + * dependent on. */ + Oid *newmembers; + int nnewmembers = aclmembers(acl, &newmembers); + + /* The list of old members is intentionally empty since we are using + * updateAclDependencies to set the ACL for the target. We can use NULL + * because getOidListDiff, which is called from updateAclDependencies, + * can handle that. */ + updateAclDependencies(RelationRelationId, + target_relid, + 0, + owner_id, + 0, + NULL, + nnewmembers, + newmembers); + + heap_freetuple(newtuple); + ReleaseSysCache(target_tuple); + } + + ReleaseSysCache(source_tuple); + table_close(class_rel, RowExclusiveLock); +} diff --git a/src/utils.h b/src/utils.h index 66a3b11590b..a94017b03d8 100644 --- a/src/utils.h +++ b/src/utils.h @@ -198,5 +198,7 @@ extern TSDLLEXPORT const char *ts_get_node_name(Node *node); extern TSDLLEXPORT int ts_get_relnatts(Oid relid); extern TSDLLEXPORT void ts_alter_table_with_event_trigger(Oid relid, Node *cmd, List *cmds, bool recurse); +extern TSDLLEXPORT void ts_copy_relation_acl(const Oid source_relid, const Oid target_relid, + const Oid owner_id); #endif /* TIMESCALEDB_UTILS_H */ diff --git a/tsl/src/continuous_aggs/create.c b/tsl/src/continuous_aggs/create.c index 6001d41b0d7..06c2a6c4d8c 100644 --- a/tsl/src/continuous_aggs/create.c +++ b/tsl/src/continuous_aggs/create.c @@ -47,6 +47,7 @@ #include #include #include +#include #include #include #include @@ -2257,7 +2258,9 @@ cagg_create(const CreateTableAsStmt *create_stmt, ViewStmt *stmt, Query *panquer panquery, materialize_hypertable_id); - create_view_for_query(final_selquery, stmt->view); + /* copy view acl to materialization hypertable */ + ObjectAddress view_address = create_view_for_query(final_selquery, stmt->view); + ts_copy_relation_acl(view_address.objectId, mataddress.objectId, GetUserId()); /* Step 3: create the internal view with select partialize(..) */ diff --git a/tsl/test/expected/cagg_permissions.out b/tsl/test/expected/cagg_permissions.out index 1996adbd96a..b3575369d90 100644 --- a/tsl/test/expected/cagg_permissions.out +++ b/tsl/test/expected/cagg_permissions.out @@ -65,8 +65,8 @@ insert into conditions select generate_series(0, 50, 10), 'NYC', 55, 75, 40, 70, NULL; CALL refresh_continuous_aggregate(' mat_refresh_test', NULL, NULL); SELECT id as cagg_job_id FROM _timescaledb_config.bgw_job order by id desc limit 1 \gset -SELECT format('%I.%I', materialization_hypertable_schema, materialization_hypertable_name ) as materialization_hypertable -FROM timescaledb_information.continuous_aggregates +SELECT format('%I.%I', materialization_hypertable_schema, materialization_hypertable_name ) as materialization_hypertable +FROM timescaledb_information.continuous_aggregates WHERE view_name = 'mat_refresh_test' \gset SELECT mat_hypertable_id FROM _timescaledb_catalog.continuous_agg WHERE user_view_name = 'mat_refresh_test' \gset SELECT schema_name as mat_chunk_schema, table_name as mat_chunk_table @@ -268,3 +268,36 @@ partial_view | _timescaledb_internal._partial_view_10 partial_view_perm | {default_perm_user_2=arwdDxt/default_perm_user_2,default_perm_user=adDxt/default_perm_user_2} \x off +-- Check for default privilege permissions get propagated to the materialization hypertable +\c :TEST_DBNAME :ROLE_SUPERUSER +CREATE SCHEMA test_default_privileges; +GRANT USAGE ON SCHEMA "test_default_privileges" TO :ROLE_DEFAULT_PERM_USER; +ALTER DEFAULT PRIVILEGES IN SCHEMA "test_default_privileges" GRANT SELECT ON TABLES TO :ROLE_DEFAULT_PERM_USER; +CREATE TABLE test_default_privileges.devices ( + time TIMESTAMPTZ NOT NULL, + device INT, + temp DOUBLE PRECISION NULL, + PRIMARY KEY(time, device) +); +SELECT create_hypertable('test_default_privileges.devices', 'time'); + create_hypertable +---------------------------------------- + (11,test_default_privileges,devices,t) +(1 row) + +CREATE MATERIALIZED VIEW test_default_privileges.devices_summary +WITH (timescaledb.continuous) +AS +SELECT time_bucket('1 day', time) AS bucket, device, MAX(temp) +FROM test_default_privileges.devices +GROUP BY bucket, device +WITH NO DATA; +-- check if user view perms have been propagated to the mat-ht +SELECT user_view_perm IS NOT DISTINCT FROM mat_table_perm +FROM cagg_info +WHERE user_view = 'test_default_privileges.devices_summary'::regclass; + ?column? +---------- + t +(1 row) + diff --git a/tsl/test/sql/cagg_permissions.sql b/tsl/test/sql/cagg_permissions.sql index 37778ca12bc..a340128747c 100644 --- a/tsl/test/sql/cagg_permissions.sql +++ b/tsl/test/sql/cagg_permissions.sql @@ -62,8 +62,8 @@ select generate_series(0, 50, 10), 'NYC', 55, 75, 40, 70, NULL; CALL refresh_continuous_aggregate(' mat_refresh_test', NULL, NULL); SELECT id as cagg_job_id FROM _timescaledb_config.bgw_job order by id desc limit 1 \gset -SELECT format('%I.%I', materialization_hypertable_schema, materialization_hypertable_name ) as materialization_hypertable -FROM timescaledb_information.continuous_aggregates +SELECT format('%I.%I', materialization_hypertable_schema, materialization_hypertable_name ) as materialization_hypertable +FROM timescaledb_information.continuous_aggregates WHERE view_name = 'mat_refresh_test' \gset SELECT mat_hypertable_id FROM _timescaledb_catalog.continuous_agg WHERE user_view_name = 'mat_refresh_test' \gset @@ -223,3 +223,31 @@ SELECT * FROM cagg_info WHERE user_view::text = 'devices_summary'; REVOKE SELECT, UPDATE ON devices_summary FROM :ROLE_DEFAULT_PERM_USER; SELECT * FROM cagg_info WHERE user_view::text = 'devices_summary'; \x off + +-- Check for default privilege permissions get propagated to the materialization hypertable +\c :TEST_DBNAME :ROLE_SUPERUSER +CREATE SCHEMA test_default_privileges; +GRANT USAGE ON SCHEMA "test_default_privileges" TO :ROLE_DEFAULT_PERM_USER; +ALTER DEFAULT PRIVILEGES IN SCHEMA "test_default_privileges" GRANT SELECT ON TABLES TO :ROLE_DEFAULT_PERM_USER; + +CREATE TABLE test_default_privileges.devices ( + time TIMESTAMPTZ NOT NULL, + device INT, + temp DOUBLE PRECISION NULL, + PRIMARY KEY(time, device) +); + +SELECT create_hypertable('test_default_privileges.devices', 'time'); + +CREATE MATERIALIZED VIEW test_default_privileges.devices_summary +WITH (timescaledb.continuous) +AS +SELECT time_bucket('1 day', time) AS bucket, device, MAX(temp) +FROM test_default_privileges.devices +GROUP BY bucket, device +WITH NO DATA; + +-- check if user view perms have been propagated to the mat-ht +SELECT user_view_perm IS NOT DISTINCT FROM mat_table_perm +FROM cagg_info +WHERE user_view = 'test_default_privileges.devices_summary'::regclass;