From 38d8a470bc104c7f2cc403380f5b5b8191463748 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 14 Nov 2019 13:21:25 +0100 Subject: [PATCH 01/10] Federated Server: Grant all over postgis and cartodb schemas --- scripts-available/CDB_FederatedServer.sql | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index c496e59..1948481 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -238,8 +238,14 @@ BEGIN EXECUTE FORMAT('CREATE ROLE %I NOLOGIN', role_name); END IF; EXECUTE FORMAT('GRANT ALL PRIVILEGES ON DATABASE %I TO %I', current_database(), role_name); - EXECUTE FORMAT('GRANT ALL ON SCHEMA %I TO %I', '@extschema@', role_name); - EXECUTE FORMAT('GRANT EXECUTE ON ALL FUNCTIONS in SCHEMA %I TO %I', '@extschema@', role_name); + + EXECUTE FORMAT('GRANT USAGE ON SCHEMA %I TO %I', '@extschema@', role_name); + EXECUTE FORMAT('GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA %I TO %I', '@extschema@', role_name); + + EXECUTE FORMAT('GRANT USAGE ON SCHEMA %I TO %I', '@postgisschema@', role_name); + EXECUTE FORMAT('GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA %I TO %I', '@postgisschema@', role_name); + EXECUTE FORMAT('GRANT SELECT ON ALL TABLES IN SCHEMA %I TO %I', '@postgisschema@', role_name); + EXECUTE FORMAT('GRANT USAGE ON FOREIGN DATA WRAPPER postgres_fdw TO %I', role_name); EXECUTE FORMAT('GRANT USAGE ON FOREIGN DATA WRAPPER postgres_fdw TO %I', role_name); EXECUTE FORMAT('GRANT USAGE ON FOREIGN SERVER %I TO %I', server_internal, role_name); From 64e185b8415b91a794ff84b45028868181b74572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 14 Nov 2019 13:43:28 +0100 Subject: [PATCH 02/10] Go back to public user mappings --- scripts-available/CDB_FederatedServer.sql | 10 +++++----- scripts-available/CDB_FederatedServerTables.sql | 10 +++++++--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index 1948481..6562756 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -250,7 +250,7 @@ BEGIN EXECUTE FORMAT('GRANT USAGE ON FOREIGN DATA WRAPPER postgres_fdw TO %I', role_name); EXECUTE FORMAT('GRANT USAGE ON FOREIGN SERVER %I TO %I', server_internal, role_name); EXECUTE FORMAT('ALTER SERVER %I OWNER TO %I', server_internal, role_name); - EXECUTE FORMAT ('CREATE USER MAPPING FOR %I SERVER %I', role_name, server_internal); + EXECUTE FORMAT ('CREATE USER MAPPING FOR PUBLIC SERVER %I', server_internal); EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION 'Could not create server %: %', server, SQLERRM USING HINT = 'Please clean the left over objects'; @@ -276,12 +276,12 @@ BEGIN LOOP IF NOT EXISTS ( WITH a AS ( - SELECT split_part(unnest(umoptions), '=', 1) as options from pg_user_mappings WHERE srvname = server_internal AND usename = role_name + SELECT split_part(unnest(umoptions), '=', 1) as options from pg_user_mappings WHERE srvname = server_internal AND usename = 'public' ) SELECT * from a where options = option.key) THEN - EXECUTE FORMAT('ALTER USER MAPPING FOR %I SERVER %I OPTIONS (ADD %I %L)', role_name, server_internal, option.key, option.value); + EXECUTE FORMAT('ALTER USER MAPPING FOR PUBLIC SERVER %I OPTIONS (ADD %I %L)', server_internal, option.key, option.value); ELSE - EXECUTE FORMAT('ALTER USER MAPPING FOR %I SERVER %I OPTIONS (SET %I %L)', role_name, server_internal, option.key, option.value); + EXECUTE FORMAT('ALTER USER MAPPING FOR PUBLIC SERVER %I OPTIONS (SET %I %L)', server_internal, option.key, option.value); END IF; END LOOP; END @@ -300,7 +300,7 @@ DECLARE BEGIN SET client_min_messages = ERROR; BEGIN - EXECUTE FORMAT ('DROP USER MAPPING FOR %I SERVER %I', role_name, server_internal); + EXECUTE FORMAT ('DROP USER MAPPING FOR PUBLIC SERVER %I', server_internal); EXECUTE FORMAT ('DROP OWNED BY %I', role_name); EXECUTE FORMAT ('DROP ROLE %I', role_name); EXCEPTION WHEN OTHERS THEN diff --git a/scripts-available/CDB_FederatedServerTables.sql b/scripts-available/CDB_FederatedServerTables.sql index 8356340..468bc1c 100644 --- a/scripts-available/CDB_FederatedServerTables.sql +++ b/scripts-available/CDB_FederatedServerTables.sql @@ -306,9 +306,13 @@ BEGIN END IF; END; - EXECUTE format('ALTER VIEW %1$I OWNER TO %I', - local_name, - cartodb.__CDB_FS_Generate_Server_Role_Name(server_internal)); + BEGIN + EXECUTE format('ALTER VIEW %1$I OWNER TO %I', + local_name, + cartodb.__CDB_FS_Generate_Server_Role_Name(server_internal)); + EXCEPTION WHEN OTHERS THEN + RAISE WARNING 'View "%" could not be shared with other users: %', local_name, SQLERRM; + END; END $$ LANGUAGE PLPGSQL VOLATILE PARALLEL UNSAFE; From 1b8651700710984b13f4a61ca0dfe8c9451f9117 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 14 Nov 2019 14:07:41 +0100 Subject: [PATCH 03/10] CDB_Federated_Server_Unregister: Drop owned in cascade Since now views might be owned by other roles, we need to drop them in cascade to force deletion --- scripts-available/CDB_FederatedServer.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index 6562756..29e36ae 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -301,7 +301,7 @@ BEGIN SET client_min_messages = ERROR; BEGIN EXECUTE FORMAT ('DROP USER MAPPING FOR PUBLIC SERVER %I', server_internal); - EXECUTE FORMAT ('DROP OWNED BY %I', role_name); + EXECUTE FORMAT ('DROP OWNED BY %I CASCADE', role_name); EXECUTE FORMAT ('DROP ROLE %I', role_name); EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION 'Not enough permissions to drop the server "%"', server; From 6e455efcddf973ef154d965cc3b54105da5c82f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 14 Nov 2019 14:15:51 +0100 Subject: [PATCH 04/10] Improve warning --- scripts-available/CDB_FederatedServerTables.sql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts-available/CDB_FederatedServerTables.sql b/scripts-available/CDB_FederatedServerTables.sql index 468bc1c..553afca 100644 --- a/scripts-available/CDB_FederatedServerTables.sql +++ b/scripts-available/CDB_FederatedServerTables.sql @@ -311,7 +311,8 @@ BEGIN local_name, cartodb.__CDB_FS_Generate_Server_Role_Name(server_internal)); EXCEPTION WHEN OTHERS THEN - RAISE WARNING 'View "%" could not be shared with other users: %', local_name, SQLERRM; + RAISE WARNING 'View "%" could not be shared with other users: %. Execute "ALTER VIEW %1$I OWNER TO %I" to share it', + local_name, SQLERRM, local_name, cartodb.__CDB_FS_Generate_Server_Role_Name(server_internal); END; END $$ From e69d3b1c30c6e178c79df4c9d6d0691e2aa2897e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 14 Nov 2019 18:45:09 +0100 Subject: [PATCH 05/10] Place all views in a shared schema --- scripts-available/CDB_FederatedServer.sql | 14 ++++++-- .../CDB_FederatedServerTables.sql | 35 +++++++++---------- test/CDB_FederatedServerTables.sql | 26 ++++++++------ test/CDB_FederatedServerTables_expect | 28 ++++++++------- 4 files changed, 57 insertions(+), 46 deletions(-) diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index 29e36ae..bdf8d3a 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -5,9 +5,16 @@ -- -- This function is just a placement to store and use the pattern for -- foreign object names --- Servers: cdb_fs_$(server_name) --- Schemas: cdb_fs_schema_$(md5sum(server_name || remote_schema_name)) --- Owner role: cdb_fs_$(md5sum(current_database() || server_name) +-- Servers: cdb_fs_$(server_name) +-- View schema: cdb_fs_$(server_name) +-- > This is where all views created when importing tables are placed +-- > One server has only one view schema +-- Import Schemas: cdb_fs_schema_$(md5sum(server_name || remote_schema_name)) +-- > This is where the foreign tables are placed +-- > One server has one import schema per remote schema plus auxiliar ones used +-- to access the remote catalog (pg_catalog, information_schema...) +-- Owner role: cdb_fs_$(md5sum(current_database() || server_name) +-- > This is the role than owns all schemas and tables related to the server -- CREATE OR REPLACE FUNCTION @extschema@.__CDB_FS_Name_Pattern() RETURNS TEXT @@ -19,6 +26,7 @@ LANGUAGE SQL IMMUTABLE PARALLEL SAFE; -- -- Produce a valid DB name for servers generated for the Federated Server -- If check_existence is true, it'll throw if the server doesn't exists +-- This name is also used to -- CREATE OR REPLACE FUNCTION @extschema@.__CDB_FS_Generate_Server_Name(input_name TEXT, check_existence BOOL) RETURNS NAME diff --git a/scripts-available/CDB_FederatedServerTables.sql b/scripts-available/CDB_FederatedServerTables.sql index 553afca..013de5e 100644 --- a/scripts-available/CDB_FederatedServerTables.sql +++ b/scripts-available/CDB_FederatedServerTables.sql @@ -173,6 +173,8 @@ LANGUAGE PLPGSQL VOLATILE PARALLEL UNSAFE; -- can be used through SQL and Maps API's. -- If the table was already exported, it will be dropped and re-imported -- +-- The view is placed under the server's view schema (cdb_fs_$(server_name)) +-- -- E.g: -- SELECT cartodb.CDB_SetUp_PG_Federated_Table( -- 'amazon', -- mandatory, name of the federated server @@ -198,7 +200,10 @@ AS $$ DECLARE server_internal name := @extschema@.__CDB_FS_Generate_Server_Name(input_name => server, check_existence => false); local_schema name := @extschema@.__CDB_FS_Create_Schema(server_internal, remote_schema); - src_table REGCLASS; + role_name name := @extschema@.__CDB_FS_Generate_Server_Role_Name(server_internal); + + src_table REGCLASS; -- import_schema.remote_table - import_schema is local_schema + local_view text; -- view_schema.local_name - view_schema is server_internal rest_of_cols TEXT[]; geom_expression TEXT; @@ -284,36 +289,28 @@ BEGIN webmercator_expression ]; - -- To create the view we switch to the caller role to make sure we have permissions - -- to write in the destination schema - RESET ROLE; + -- Create view schema if it doesn't exist + IF NOT EXISTS (SELECT oid FROM pg_namespace WHERE nspname = server_internal) THEN + EXECUTE 'CREATE SCHEMA ' || quote_ident(server_internal) || ' AUTHORIZATION ' || quote_ident(role_name); + END IF; -- Create a view with homogeneous CDB fields BEGIN EXECUTE format( - 'CREATE OR REPLACE VIEW %1$I AS - SELECT %2s - FROM %3$s t', - local_name, + 'CREATE OR REPLACE VIEW %1$I.%2$I AS + SELECT %3s + FROM %4$s t', + server_internal, local_name, array_to_string(carto_columns_expression || rest_of_cols, ','), src_table ); EXCEPTION WHEN OTHERS THEN - IF EXISTS (SELECT to_regclass(local_name)) THEN - RAISE EXCEPTION 'Could not import table "%" as "%" already exists: %', remote_table, local_name, SQLERRM; + IF EXISTS (SELECT to_regclass(format('%1$I.%2$I', server_internal, local_name))) THEN + RAISE EXCEPTION 'Could not import table "%" as "%.%" already exists', remote_table, server_internal, local_name; ELSE RAISE EXCEPTION 'Could not import table "%" as "%": %', remote_table, local_name, SQLERRM; END IF; END; - - BEGIN - EXECUTE format('ALTER VIEW %1$I OWNER TO %I', - local_name, - cartodb.__CDB_FS_Generate_Server_Role_Name(server_internal)); - EXCEPTION WHEN OTHERS THEN - RAISE WARNING 'View "%" could not be shared with other users: %. Execute "ALTER VIEW %1$I OWNER TO %I" to share it', - local_name, SQLERRM, local_name, cartodb.__CDB_FS_Generate_Server_Role_Name(server_internal); - END; END $$ LANGUAGE PLPGSQL VOLATILE PARALLEL UNSAFE; diff --git a/test/CDB_FederatedServerTables.sql b/test/CDB_FederatedServerTables.sql index 328a05b..fdd1d7a 100644 --- a/test/CDB_FederatedServerTables.sql +++ b/test/CDB_FederatedServerTables.sql @@ -71,7 +71,8 @@ SELECT 'R1', cartodb.CDB_Federated_Table_Register( geom_column => 'geom' ); -SELECT 'S1', cartodb_id, ST_AsText(the_geom), another_field FROM remote_geom; +SELECT 'S1', cartodb_id, ST_AsText(the_geom), another_field FROM cdb_fs_loopback.remote_geom; + Select * FROM CDB_Federated_Server_List_Remote_Tables( server => 'loopback', remote_schema => 'remote_schema' @@ -88,7 +89,7 @@ SELECT 'R2', cartodb.CDB_Federated_Table_Register( local_name => 'myFullTable' ); -SELECT 'S2', cartodb_id, ST_AsText(the_geom), another_field FROM "myFullTable"; +SELECT 'S2', cartodb_id, ST_AsText(the_geom), another_field FROM cdb_fs_loopback."myFullTable"; Select * FROM CDB_Federated_Server_List_Remote_Tables( server => 'loopback', remote_schema => 'remote_schema' @@ -107,9 +108,9 @@ SELECT 'R3', cartodb.CDB_Federated_Table_Register( ); -- The old view should dissapear -SELECT 'S3_old', cartodb_id, another_field FROM "myFullTable"; +SELECT 'S3_old', cartodb_id, another_field FROM cdb_fs_loopback."myFullTable"; -- And the new appear -SELECT 'S3_new', cartodb_id, another_field FROM different_name; +SELECT 'S3_new', cartodb_id, another_field FROM cdb_fs_loopback.different_name; \echo '## Unregistering works' -- Deregistering the first table @@ -255,7 +256,7 @@ SELECT cartodb.CDB_Federated_Table_Unregister( -- =================================================================== \echo '## Target conflict is handled nicely: Table' -CREATE TABLE localtable (a integer); +CREATE TABLE cdb_fs_loopback.localtable (a integer); SELECT cartodb.CDB_Federated_Table_Register( server => 'loopback', remote_schema => 'remote_schema', @@ -265,7 +266,7 @@ SELECT cartodb.CDB_Federated_Table_Register( local_name => 'localtable'); \echo '## Target conflict is handled nicely: View' -CREATE VIEW localtable2 AS Select * from localtable; +CREATE VIEW cdb_fs_loopback.localtable2 AS Select * from cdb_fs_loopback.localtable; SELECT cartodb.CDB_Federated_Table_Register( server => 'loopback', remote_schema => 'remote_schema', @@ -274,8 +275,8 @@ SELECT cartodb.CDB_Federated_Table_Register( geom_column => 'geom', local_name => 'localtable2'); -DROP VIEW localtable2; -DROP TABLE localtable; +DROP VIEW cdb_fs_loopback.localtable2; +DROP TABLE cdb_fs_loopback.localtable; -- =================================================================== -- Test permissions @@ -292,6 +293,9 @@ SELECT cartodb.CDB_Federated_Table_Register( geom_column => 'geom', local_name => 'localtable'); +\echo '## Normal users can not write in the import schema' +CREATE TABLE cdb_fs_loopback.localtable (a integer); + \echo '## Listing remote tables does not work without permissions' Select * FROM cartodb.CDB_Federated_Server_List_Remote_Tables(server => 'loopback', remote_schema => 'remote_schema'); @@ -311,7 +315,7 @@ SELECT cartodb.CDB_Federated_Table_Register( Select * FROM cartodb.CDB_Federated_Server_List_Remote_Tables(server => 'loopback', remote_schema => 'remote_schema'); \echo '## Selecting from a registered table with granted permissions works' -Select cartodb_id, ST_AsText(the_geom) from localtable; +Select cartodb_id, ST_AsText(the_geom) from cdb_fs_loopback.localtable; \echo '## Selecting from a registered table without permissions does not work' \c contrib_regression cdb_fs_tester2 @@ -329,7 +333,7 @@ EXCEPTION RETURN FALSE; END $$ LANGUAGE 'plpgsql'; -Select catch_permission_error($$SELECT cartodb_id, ST_AsText(the_geom) from localtable$$); +Select catch_permission_error($$SELECT cartodb_id, ST_AsText(the_geom) from cdb_fs_loopback.localtable$$); DROP FUNCTION catch_permission_error(text); \echo '## Deleting a registered table without permissions does not work' @@ -347,7 +351,7 @@ SELECT cartodb.CDB_Federated_Server_Grant_Access(server := 'loopback', db_role : SELECT cartodb.CDB_Federated_Server_Grant_Access(server := 'loopback', db_role := 'cdb_fs_tester2'::name); \c contrib_regression cdb_fs_tester2 Select * FROM cartodb.CDB_Federated_Server_List_Remote_Tables(server => 'loopback', remote_schema => 'remote_schema'); -Select cartodb_id, ST_AsText(the_geom) from localtable; +Select cartodb_id, ST_AsText(the_geom) from cdb_fs_loopback.localtable; \echo '## A different user can unregister a table' SELECT cartodb.CDB_Federated_Table_Unregister( diff --git a/test/CDB_FederatedServerTables_expect b/test/CDB_FederatedServerTables_expect index b8f9abc..f8dd882 100644 --- a/test/CDB_FederatedServerTables_expect +++ b/test/CDB_FederatedServerTables_expect @@ -3,24 +3,24 @@ C1| R1| S1|1|POINT(1 1)|patata S1|2|POINT(2 2)|patata2 -t|remote_geom|public.remote_geom|id|geom|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] +t|remote_geom|cdb_fs_loopback.remote_geom|id|geom|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] f|remote_geom2|||||[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] f|remote_other|||||[{"Name" : "field", "Type" : "text"}, {"Name" : "field2", "Type" : "text"}, {"Name" : "id", "Type" : "bigint"}] ## Registering another existing table works R2| S2|3|POINT(3 3)|patata -t|remote_geom|public.remote_geom|id|geom|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] -t|remote_geom2|public."myFullTable"|id|geom|geom_mercator|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] +t|remote_geom|cdb_fs_loopback.remote_geom|id|geom|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] +t|remote_geom2|cdb_fs_loopback."myFullTable"|id|geom|geom_mercator|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] f|remote_other|||||[{"Name" : "field", "Type" : "text"}, {"Name" : "field2", "Type" : "text"}, {"Name" : "id", "Type" : "bigint"}] ## Re-registering a table works R3| -ERROR: relation "myFullTable" does not exist at character 49 +ERROR: relation "cdb_fs_loopback.myFullTable" does not exist at character 49 S3_new|3|patata ## Unregistering works U1| ERROR: relation "remote_geom" does not exist at character 71 f|remote_geom|||||[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] -t|remote_geom2|public.different_name|id|geom_mercator|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] +t|remote_geom2|cdb_fs_loopback.different_name|id|geom_mercator|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] f|remote_other|||||[{"Name" : "field", "Type" : "text"}, {"Name" : "field2", "Type" : "text"}, {"Name" : "id", "Type" : "bigint"}] ## Registering a table: Invalid server fails ERROR: Server "Does not exist" does not exist @@ -50,15 +50,17 @@ ERROR: non geometry column "Does not exists" ## Target conflict is handled nicely: Table CREATE TABLE -ERROR: Could not import table "remote_geom" as "localtable" already exists: "localtable" is not a view +ERROR: Could not import table "remote_geom" as "cdb_fs_loopback.localtable" already exists ## Target conflict is handled nicely: View CREATE VIEW -ERROR: Could not import table "remote_geom" as "localtable2" already exists: cannot change name of view column "a" to "cartodb_id" +ERROR: Could not import table "remote_geom" as "cdb_fs_loopback.localtable2" already exists DROP VIEW DROP TABLE ## Registering tables does not work without permissions You are now connected to database "contrib_regression" as user "cdb_fs_tester". ERROR: Not enough permissions to access the server "loopback" +## Normal users can not write in the import schema +ERROR: permission denied for schema cdb_fs_loopback at character 14 ## Listing remote tables does not work without permissions ERROR: Not enough permissions to access the server "loopback" ## Registering tables works with granted permissions @@ -67,8 +69,8 @@ You are now connected to database "contrib_regression" as user "postgres". You are now connected to database "contrib_regression" as user "cdb_fs_tester". ## Listing remote tables works with granted permissions -t|remote_geom|public.localtable|id|geom|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] -t|remote_geom2|public.different_name|id|geom_mercator|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] +t|remote_geom|cdb_fs_loopback.localtable|id|geom|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] +t|remote_geom2|cdb_fs_loopback.different_name|id|geom_mercator|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] f|remote_other|||||[{"Name" : "field", "Type" : "text"}, {"Name" : "field2", "Type" : "text"}, {"Name" : "id", "Type" : "bigint"}] ## Selecting from a registered table with granted permissions works 1|POINT(1 1) @@ -86,16 +88,16 @@ ERROR: You do not have rights to grant access on "loopback" You are now connected to database "contrib_regression" as user "postgres". You are now connected to database "contrib_regression" as user "cdb_fs_tester2". -t|remote_geom|public.localtable|id|geom|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] -t|remote_geom2|public.different_name|id|geom_mercator|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] +t|remote_geom|cdb_fs_loopback.localtable|id|geom|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] +t|remote_geom2|cdb_fs_loopback.different_name|id|geom_mercator|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] f|remote_other|||||[{"Name" : "field", "Type" : "text"}, {"Name" : "field2", "Type" : "text"}, {"Name" : "id", "Type" : "bigint"}] 1|POINT(1 1) 2|POINT(2 2) ## A different user can unregister a table -NOTICE: drop cascades to view localtable +NOTICE: drop cascades to view cdb_fs_loopback.localtable f|remote_geom|||||[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "id", "Type" : "integer"}] -t|remote_geom2|public.different_name|id|geom_mercator|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] +t|remote_geom2|cdb_fs_loopback.different_name|id|geom_mercator|geom|[{"Name" : "another_field", "Type" : "text"}, {"Name" : "geom", "Type" : "GEOMETRY,4326"}, {"Name" : "geom_mercator", "Type" : "GEOMETRY,3857"}, {"Name" : "id", "Type" : "bigint"}] f|remote_other|||||[{"Name" : "field", "Type" : "text"}, {"Name" : "field2", "Type" : "text"}, {"Name" : "id", "Type" : "bigint"}] ## Only the owner can revoke permissions over the server ERROR: You do not have rights to revoke access on "loopback" From 0893fae0257763cdf909ff64262ff38e4b58eb5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 14 Nov 2019 18:46:02 +0100 Subject: [PATCH 06/10] Revert "CDB_Federated_Server_Unregister: Drop owned in cascade" This reverts commit 1b8651700710984b13f4a61ca0dfe8c9451f9117. --- scripts-available/CDB_FederatedServer.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index bdf8d3a..51baa20 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -309,7 +309,7 @@ BEGIN SET client_min_messages = ERROR; BEGIN EXECUTE FORMAT ('DROP USER MAPPING FOR PUBLIC SERVER %I', server_internal); - EXECUTE FORMAT ('DROP OWNED BY %I CASCADE', role_name); + EXECUTE FORMAT ('DROP OWNED BY %I', role_name); EXECUTE FORMAT ('DROP ROLE %I', role_name); EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION 'Not enough permissions to drop the server "%"', server; From adca72716979970cbb94f56772874c1df8545e07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 14 Nov 2019 18:46:45 +0100 Subject: [PATCH 07/10] Revert "Go back to public user mappings" This reverts commit 64e185b8415b91a794ff84b45028868181b74572. --- scripts-available/CDB_FederatedServer.sql | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index 51baa20..008bc4f 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -258,7 +258,7 @@ BEGIN EXECUTE FORMAT('GRANT USAGE ON FOREIGN DATA WRAPPER postgres_fdw TO %I', role_name); EXECUTE FORMAT('GRANT USAGE ON FOREIGN SERVER %I TO %I', server_internal, role_name); EXECUTE FORMAT('ALTER SERVER %I OWNER TO %I', server_internal, role_name); - EXECUTE FORMAT ('CREATE USER MAPPING FOR PUBLIC SERVER %I', server_internal); + EXECUTE FORMAT ('CREATE USER MAPPING FOR %I SERVER %I', role_name, server_internal); EXCEPTION WHEN OTHERS THEN RAISE EXCEPTION 'Could not create server %: %', server, SQLERRM USING HINT = 'Please clean the left over objects'; @@ -284,12 +284,12 @@ BEGIN LOOP IF NOT EXISTS ( WITH a AS ( - SELECT split_part(unnest(umoptions), '=', 1) as options from pg_user_mappings WHERE srvname = server_internal AND usename = 'public' + SELECT split_part(unnest(umoptions), '=', 1) as options from pg_user_mappings WHERE srvname = server_internal AND usename = role_name ) SELECT * from a where options = option.key) THEN - EXECUTE FORMAT('ALTER USER MAPPING FOR PUBLIC SERVER %I OPTIONS (ADD %I %L)', server_internal, option.key, option.value); + EXECUTE FORMAT('ALTER USER MAPPING FOR %I SERVER %I OPTIONS (ADD %I %L)', role_name, server_internal, option.key, option.value); ELSE - EXECUTE FORMAT('ALTER USER MAPPING FOR PUBLIC SERVER %I OPTIONS (SET %I %L)', server_internal, option.key, option.value); + EXECUTE FORMAT('ALTER USER MAPPING FOR %I SERVER %I OPTIONS (SET %I %L)', role_name, server_internal, option.key, option.value); END IF; END LOOP; END @@ -308,7 +308,7 @@ DECLARE BEGIN SET client_min_messages = ERROR; BEGIN - EXECUTE FORMAT ('DROP USER MAPPING FOR PUBLIC SERVER %I', server_internal); + EXECUTE FORMAT ('DROP USER MAPPING FOR %I SERVER %I', role_name, server_internal); EXECUTE FORMAT ('DROP OWNED BY %I', role_name); EXECUTE FORMAT ('DROP ROLE %I', role_name); EXCEPTION WHEN OTHERS THEN From fa5f0bcbc12ba70cc84079a226867a93bbca22fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 14 Nov 2019 18:55:38 +0100 Subject: [PATCH 08/10] CDB_Federated_Server_Grant_Access: comment about search_path --- scripts-available/CDB_FederatedServer.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index 008bc4f..49a7de0 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -362,6 +362,8 @@ LANGUAGE PLPGSQL IMMUTABLE PARALLEL SAFE; -- -- Grant access to a server +-- In the future we might consider adding the server's view schema to the role search_path +-- to make it easier to access the created views -- CREATE OR REPLACE FUNCTION @extschema@.CDB_Federated_Server_Grant_Access(server TEXT, db_role NAME) RETURNS void From e99e4a02c9f43b73761cd8c52a8b5965156b632b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 14 Nov 2019 19:00:09 +0100 Subject: [PATCH 09/10] Remove TODO Since the server registration requires superadmin priviledges it doesn't make sense to over protect the role creation --- scripts-available/CDB_FederatedServer.sql | 4 ---- 1 file changed, 4 deletions(-) diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index 49a7de0..6d6fb08 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -238,10 +238,6 @@ BEGIN IF NOT EXISTS (SELECT * FROM pg_foreign_server WHERE srvname = server_internal) THEN BEGIN EXECUTE FORMAT('CREATE SERVER %I FOREIGN DATA WRAPPER postgres_fdw', server_internal); - -- TODO: Delete this IF before merging to make sure nobody creates a role - -- that is later used automatically by us granting them all permissions in the foreign server - -- TODO: This is here to help debugging during development (so failures to destroy objects are allowed) - -- TODO IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = role_name) THEN EXECUTE FORMAT('CREATE ROLE %I NOLOGIN', role_name); END IF; From fde12c818a77241a7f75685c2fe1ef46f247e0b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Fri, 15 Nov 2019 13:09:52 +0100 Subject: [PATCH 10/10] Update comments --- scripts-available/CDB_FederatedServer.sql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index 6d6fb08..bf5a841 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -26,7 +26,7 @@ LANGUAGE SQL IMMUTABLE PARALLEL SAFE; -- -- Produce a valid DB name for servers generated for the Federated Server -- If check_existence is true, it'll throw if the server doesn't exists --- This name is also used to +-- This name is also used as the schema to store views -- CREATE OR REPLACE FUNCTION @extschema@.__CDB_FS_Generate_Server_Name(input_name TEXT, check_existence BOOL) RETURNS NAME @@ -243,9 +243,10 @@ BEGIN END IF; EXECUTE FORMAT('GRANT ALL PRIVILEGES ON DATABASE %I TO %I', current_database(), role_name); + -- These grants over `@extschema@` and `@postgisschema@` are necessary for the cases + -- where the schemas aren't accessible to PUBLIC, which is what happens in a CARTO database EXECUTE FORMAT('GRANT USAGE ON SCHEMA %I TO %I', '@extschema@', role_name); EXECUTE FORMAT('GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA %I TO %I', '@extschema@', role_name); - EXECUTE FORMAT('GRANT USAGE ON SCHEMA %I TO %I', '@postgisschema@', role_name); EXECUTE FORMAT('GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA %I TO %I', '@postgisschema@', role_name); EXECUTE FORMAT('GRANT SELECT ON ALL TABLES IN SCHEMA %I TO %I', '@postgisschema@', role_name);