Skip to content

Commit

Permalink
Handle properly default privileges on CAggs
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@fabriziomello
fabriziomello committed Aug 12, 2022
1 parent 16fdb6c commit 500c225
Show file tree
Hide file tree
Showing 7 changed files with 145 additions and 69 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -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)

Expand Down
66 changes: 2 additions & 64 deletions src/chunk.c
View file
Expand Up @@ -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);
}

/*
Expand Down
70 changes: 70 additions & 0 deletions src/utils.c
View file
Expand Up @@ -24,6 +24,7 @@
#include <parser/parse_coerce.h>
#include <parser/parse_func.h>
#include <parser/scansup.h>
#include <utils/acl.h>
#include <utils/builtins.h>
#include <utils/catcache.h>
#include <utils/date.h>
Expand Down Expand Up @@ -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);
}
2 changes: 2 additions & 0 deletions src/utils.h
View file
Expand Up @@ -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 */
5 changes: 4 additions & 1 deletion tsl/src/continuous_aggs/create.c
View file
Expand Up @@ -47,6 +47,7 @@
#include <parser/parse_type.h>
#include <rewrite/rewriteHandler.h>
#include <rewrite/rewriteManip.h>
#include <utils/acl.h>
#include <utils/rel.h>
#include <utils/builtins.h>
#include <utils/catcache.h>
Expand Down Expand Up @@ -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(..)
*/
Expand Down
37 changes: 35 additions & 2 deletions tsl/test/expected/cagg_permissions.out
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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)

32 changes: 30 additions & 2 deletions tsl/test/sql/cagg_permissions.sql
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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;

0 comments on commit 500c225

Please sign in to comment.