From 8ce4f20061e9b3a85edc06ac1e1568682cd86c05 Mon Sep 17 00:00:00 2001 From: Halil Ozan Akgul Date: Wed, 5 Feb 2020 16:17:58 +0300 Subject: [PATCH] Fixes the bug of grants on public schema propagation --- .../distributed/commands/dependencies.c | 6 -- .../distributed/metadata/metadata_sync.c | 5 -- .../expected/grant_on_schema_propagation.out | 77 +++++++++++++++++++ ...lation_ensure_dependency_activate_node.out | 30 ++++++++ .../expected/isolation_extension_commands.out | 28 +++---- .../expected/multi_cluster_management.out | 2 + .../multi_transactional_drop_shards.out | 9 +-- .../sql/grant_on_schema_propagation.sql | 42 ++++++++++ .../regress/sql/multi_cluster_management.sql | 3 + .../sql/multi_transactional_drop_shards.sql | 3 +- 10 files changed, 170 insertions(+), 35 deletions(-) diff --git a/src/backend/distributed/commands/dependencies.c b/src/backend/distributed/commands/dependencies.c index c65f2a3c8..d13105763 100644 --- a/src/backend/distributed/commands/dependencies.c +++ b/src/backend/distributed/commands/dependencies.c @@ -173,12 +173,6 @@ GetDependencyCreateDDLCommands(const ObjectAddress *dependency) { char *schemaDDLCommand = CreateSchemaDDLCommand(dependency->objectId); - if (schemaDDLCommand == NULL) - { - /* no schema to create */ - return NIL; - } - List *DDLCommands = list_make1(schemaDDLCommand); List *grantDDLCommands = GrantOnSchemaDDLCommands(dependency->objectId); diff --git a/src/backend/distributed/metadata/metadata_sync.c b/src/backend/distributed/metadata/metadata_sync.c index acff0f6ed..b0f5fb139 100644 --- a/src/backend/distributed/metadata/metadata_sync.c +++ b/src/backend/distributed/metadata/metadata_sync.c @@ -1101,11 +1101,6 @@ CreateSchemaDDLCommand(Oid schemaId) { char *schemaName = get_namespace_name(schemaId); - if (strncmp(schemaName, "public", NAMEDATALEN) == 0) - { - return NULL; - } - StringInfo schemaNameDef = makeStringInfo(); const char *quotedSchemaName = quote_identifier(schemaName); const char *ownerName = quote_identifier(SchemaOwnerName(schemaId)); diff --git a/src/test/regress/expected/grant_on_schema_propagation.out b/src/test/regress/expected/grant_on_schema_propagation.out index 58266e44a..9c7affd46 100644 --- a/src/test/regress/expected/grant_on_schema_propagation.out +++ b/src/test/regress/expected/grant_on_schema_propagation.out @@ -356,6 +356,83 @@ SELECT run_command_on_coordinator_and_workers('DROP SCHEMA dist_schema CASCADE') (1 row) +-- test grants on public schema +-- first remove one of the worker nodes +SET citus.shard_replication_factor TO 1; +SELECT master_remove_node('localhost', :worker_2_port); + master_remove_node +--------------------------------------------------------------------- + +(1 row) + +-- distribute the public schema (it has to be distributed by now but just in case) +CREATE TABLE public_schema_table (id INT); +SELECT create_distributed_table('public_schema_table', 'id'); + create_distributed_table +--------------------------------------------------------------------- + +(1 row) + +-- give cascading permissions +GRANT USAGE, CREATE ON SCHEMA PUBLIC TO role_1 WITH GRANT OPTION; +SET ROLE role_1; +GRANT USAGE ON SCHEMA PUBLIC TO PUBLIC; +RESET ROLE; +-- check if the grants are propagated correctly +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; + nspname | nspacl +--------------------------------------------------------------------- + public | {postgres=UC/postgres,=UC/postgres,role_1=U*C*/postgres,=U/role_1} +(1 row) + +\c - - - :worker_1_port +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; + nspname | nspacl +--------------------------------------------------------------------- + public | {postgres=UC/postgres,=UC/postgres,role_1=U*C*/postgres,=U/role_1} +(1 row) + +\c - - - :master_port +-- add the previously removed node +SELECT 1 FROM master_add_node('localhost', :worker_2_port); + ?column? +--------------------------------------------------------------------- + 1 +(1 row) + +-- check if the grants are propagated correctly +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; + nspname | nspacl +--------------------------------------------------------------------- + public | {postgres=UC/postgres,=UC/postgres,role_1=U*C*/postgres,=U/role_1} +(1 row) + +\c - - - :worker_2_port +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; + nspname | nspacl +--------------------------------------------------------------------- + public | {postgres=UC/postgres,=UC/postgres,role_1=U*C*/postgres,=U/role_1} +(1 row) + +\c - - - :master_port +-- revoke those new permissions +REVOKE CREATE, USAGE ON SCHEMA PUBLIC FROM role_1 CASCADE; +-- check if the grants are propagated correctly +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; + nspname | nspacl +--------------------------------------------------------------------- + public | {postgres=UC/postgres,=UC/postgres} +(1 row) + +\c - - - :worker_1_port +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; + nspname | nspacl +--------------------------------------------------------------------- + public | {postgres=UC/postgres,=UC/postgres} +(1 row) + +\c - - - :master_port +DROP TABLE public_schema_table; SELECT run_command_on_coordinator_and_workers('DROP ROLE role_1, role_2, role_3'); run_command_on_coordinator_and_workers --------------------------------------------------------------------- diff --git a/src/test/regress/expected/isolation_ensure_dependency_activate_node.out b/src/test/regress/expected/isolation_ensure_dependency_activate_node.out index cf12b2d2e..a17b47667 100644 --- a/src/test/regress/expected/isolation_ensure_dependency_activate_node.out +++ b/src/test/regress/expected/isolation_ensure_dependency_activate_node.out @@ -24,6 +24,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -89,6 +90,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -139,6 +141,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -210,6 +213,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -260,6 +264,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -331,6 +336,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -381,6 +387,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -448,6 +455,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address (schema,{myschema},{}) +(schema,{public},{}) count 1 @@ -498,6 +506,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -571,6 +580,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address (schema,{myschema},{}) +(schema,{public},{}) count 1 @@ -621,6 +631,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -694,6 +705,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address (schema,{myschema},{}) +(schema,{public},{}) count 1 @@ -744,6 +756,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -836,6 +849,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address (schema,{myschema},{}) +(schema,{public},{}) count 1 @@ -886,6 +900,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -971,6 +986,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address (schema,{myschema},{}) +(schema,{public},{}) count 1 @@ -1021,6 +1037,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -1115,6 +1132,7 @@ pg_identify_object_as_address (schema,{myschema},{}) (schema,{myschema2},{}) +(schema,{public},{}) count 1 @@ -1165,6 +1183,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -1223,6 +1242,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address +(schema,{public},{}) (type,{public.tt1},{}) count @@ -1274,6 +1294,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -1331,6 +1352,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address +(schema,{public},{}) (type,{public.tt1},{}) count @@ -1382,6 +1404,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -1458,6 +1481,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address (schema,{myschema},{}) +(schema,{public},{}) (type,{myschema.tt1},{}) count @@ -1509,6 +1533,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -1584,6 +1609,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address (function,"{public,add}","{integer,integer}") +(schema,{public},{}) count 0 @@ -1634,6 +1660,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -1714,6 +1741,7 @@ step s2-print-distributed-objects: pg_identify_object_as_address (function,"{public,add}","{integer,integer}") +(schema,{public},{}) count 0 @@ -1764,6 +1792,7 @@ step s1-print-distributed-objects: 1 pg_identify_object_as_address +(schema,{public},{}) count 0 @@ -1846,6 +1875,7 @@ pg_identify_object_as_address (function,"{myschema,add}","{integer,integer}") (schema,{myschema},{}) +(schema,{public},{}) count 1 diff --git a/src/test/regress/expected/isolation_extension_commands.out b/src/test/regress/expected/isolation_extension_commands.out index 7e2176b71..7af348d84 100644 --- a/src/test/regress/expected/isolation_extension_commands.out +++ b/src/test/regress/expected/isolation_extension_commands.out @@ -26,7 +26,7 @@ step s1-print: count -1 +2 extname extversion nspname seg 1.1 public @@ -73,7 +73,7 @@ step s1-print: count -1 +2 extname extversion nspname seg 1.2 public @@ -126,7 +126,7 @@ step s1-print: count -0 +1 extname extversion nspname run_command_on_workers @@ -168,7 +168,7 @@ step s1-print: count -2 +3 extname extversion nspname seg 1.3 schema1 @@ -215,7 +215,7 @@ step s1-print: count -1 +2 extname extversion nspname run_command_on_workers @@ -270,7 +270,7 @@ step s1-print: count -4 +5 extname extversion nspname seg 1.3 schema3 @@ -322,7 +322,7 @@ step s1-print: count -4 +5 extname extversion nspname seg 1.3 schema1 @@ -379,7 +379,7 @@ step s1-print: count -3 +4 extname extversion nspname seg 1.1 public @@ -444,7 +444,7 @@ step s1-print: count -4 +5 extname extversion nspname seg 1.2 public @@ -497,7 +497,7 @@ step s1-print: count -3 +4 extname extversion nspname run_command_on_workers @@ -538,7 +538,7 @@ step s1-print: count -3 +4 extname extversion nspname seg 1.3 schema1 @@ -597,7 +597,7 @@ step s1-print: count -4 +5 extname extversion nspname seg 1.3 schema2 @@ -648,7 +648,7 @@ step s1-print: count -3 +4 extname extversion nspname seg 1.1 public @@ -709,7 +709,7 @@ step s1-print: count -3 +4 extname extversion nspname run_command_on_workers diff --git a/src/test/regress/expected/multi_cluster_management.out b/src/test/regress/expected/multi_cluster_management.out index 1ddd8a458..67cb4b8ba 100644 --- a/src/test/regress/expected/multi_cluster_management.out +++ b/src/test/regress/expected/multi_cluster_management.out @@ -149,6 +149,8 @@ GRANT EXECUTE ON FUNCTION master_add_secondary_node(text,int,text,int,name) TO n GRANT EXECUTE ON FUNCTION master_disable_node(text,int) TO node_metadata_user; GRANT EXECUTE ON FUNCTION master_remove_node(text,int) TO node_metadata_user; GRANT EXECUTE ON FUNCTION master_update_node(int,text,int,bool,int) TO node_metadata_user; +-- Removing public schema from pg_dist_object because it breaks the next tests +DELETE FROM citus.pg_dist_object WHERE objid = 'public'::regnamespace::oid; -- try to manipulate node metadata via non-super user SET ROLE non_super_user; SELECT 1 FROM master_add_inactive_node('localhost', :worker_2_port + 1); diff --git a/src/test/regress/expected/multi_transactional_drop_shards.out b/src/test/regress/expected/multi_transactional_drop_shards.out index 867e434c0..31094b237 100644 --- a/src/test/regress/expected/multi_transactional_drop_shards.out +++ b/src/test/regress/expected/multi_transactional_drop_shards.out @@ -688,7 +688,6 @@ SELECT stop_metadata_sync_to_node('localhost', :worker_1_port); CREATE USER try_drop_table WITH LOGIN; NOTICE: not propagating CREATE ROLE/USER commands to worker nodes HINT: Connect to worker nodes directly to manually create all necessary users and roles. -GRANT ALL ON SCHEMA public TO try_drop_table; SELECT run_command_on_workers('CREATE USER try_drop_table WITH LOGIN'); run_command_on_workers --------------------------------------------------------------------- @@ -696,13 +695,7 @@ SELECT run_command_on_workers('CREATE USER try_drop_table WITH LOGIN'); (localhost,57638,t,"CREATE ROLE") (2 rows) -SELECT run_command_on_workers('GRANT ALL ON SCHEMA public TO try_drop_table'); - run_command_on_workers ---------------------------------------------------------------------- - (localhost,57637,t,GRANT) - (localhost,57638,t,GRANT) -(2 rows) - +GRANT ALL ON SCHEMA public TO try_drop_table; \c - try_drop_table - :master_port BEGIN; CREATE TABLE temp_dist_table (x int, y int); diff --git a/src/test/regress/sql/grant_on_schema_propagation.sql b/src/test/regress/sql/grant_on_schema_propagation.sql index 41da0e640..7d0d8072f 100644 --- a/src/test/regress/sql/grant_on_schema_propagation.sql +++ b/src/test/regress/sql/grant_on_schema_propagation.sql @@ -174,4 +174,46 @@ SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'dist_schema' ORDER BY DROP TABLE dist_schema.dist_table; SELECT run_command_on_coordinator_and_workers('DROP SCHEMA dist_schema CASCADE'); + +-- test grants on public schema +-- first remove one of the worker nodes +SET citus.shard_replication_factor TO 1; +SELECT master_remove_node('localhost', :worker_2_port); + +-- distribute the public schema (it has to be distributed by now but just in case) +CREATE TABLE public_schema_table (id INT); +SELECT create_distributed_table('public_schema_table', 'id'); + +-- give cascading permissions +GRANT USAGE, CREATE ON SCHEMA PUBLIC TO role_1 WITH GRANT OPTION; +SET ROLE role_1; +GRANT USAGE ON SCHEMA PUBLIC TO PUBLIC; +RESET ROLE; + +-- check if the grants are propagated correctly +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; +\c - - - :worker_1_port +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; +\c - - - :master_port + +-- add the previously removed node +SELECT 1 FROM master_add_node('localhost', :worker_2_port); + +-- check if the grants are propagated correctly +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; +\c - - - :worker_2_port +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; +\c - - - :master_port + +-- revoke those new permissions +REVOKE CREATE, USAGE ON SCHEMA PUBLIC FROM role_1 CASCADE; + +-- check if the grants are propagated correctly +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; +\c - - - :worker_1_port +SELECT nspname, nspacl FROM pg_namespace WHERE nspname = 'public' ORDER BY nspname; +\c - - - :master_port + +DROP TABLE public_schema_table; + SELECT run_command_on_coordinator_and_workers('DROP ROLE role_1, role_2, role_3'); diff --git a/src/test/regress/sql/multi_cluster_management.sql b/src/test/regress/sql/multi_cluster_management.sql index c0627ab4b..f31fff389 100644 --- a/src/test/regress/sql/multi_cluster_management.sql +++ b/src/test/regress/sql/multi_cluster_management.sql @@ -68,6 +68,9 @@ GRANT EXECUTE ON FUNCTION master_disable_node(text,int) TO node_metadata_user; GRANT EXECUTE ON FUNCTION master_remove_node(text,int) TO node_metadata_user; GRANT EXECUTE ON FUNCTION master_update_node(int,text,int,bool,int) TO node_metadata_user; +-- Removing public schema from pg_dist_object because it breaks the next tests +DELETE FROM citus.pg_dist_object WHERE objid = 'public'::regnamespace::oid; + -- try to manipulate node metadata via non-super user SET ROLE non_super_user; SELECT 1 FROM master_add_inactive_node('localhost', :worker_2_port + 1); diff --git a/src/test/regress/sql/multi_transactional_drop_shards.sql b/src/test/regress/sql/multi_transactional_drop_shards.sql index 68931354d..f574dedcf 100644 --- a/src/test/regress/sql/multi_transactional_drop_shards.sql +++ b/src/test/regress/sql/multi_transactional_drop_shards.sql @@ -377,9 +377,8 @@ SELECT stop_metadata_sync_to_node('localhost', :worker_1_port); -- test DROP TABLE as a non-superuser in a transaction block CREATE USER try_drop_table WITH LOGIN; -GRANT ALL ON SCHEMA public TO try_drop_table; SELECT run_command_on_workers('CREATE USER try_drop_table WITH LOGIN'); -SELECT run_command_on_workers('GRANT ALL ON SCHEMA public TO try_drop_table'); +GRANT ALL ON SCHEMA public TO try_drop_table; \c - try_drop_table - :master_port