diff --git a/scripts-available/CDB_FederatedServer.sql b/scripts-available/CDB_FederatedServer.sql index f5971d5..b3b821a 100644 --- a/scripts-available/CDB_FederatedServer.sql +++ b/scripts-available/CDB_FederatedServer.sql @@ -26,15 +26,20 @@ AS $$ DECLARE internal_server_name text := format('%s%s', @extschema@.__CDB_FS_Name_Pattern(), input_name); BEGIN - -- We discard anything that would be truncated - IF (char_length(internal_server_name) < 64) THEN - IF (check_existence AND (NOT EXISTS (SELECT * FROM pg_foreign_server WHERE srvname = internal_server_name))) THEN - RAISE EXCEPTION 'Server "%" does not exist', input_name; - END IF; - RETURN internal_server_name::name; - ELSE - RAISE EXCEPTION 'Server name is too long to be used as identifier'; + IF input_name IS NULL THEN + RAISE EXCEPTION 'Server name cannot be NULL'; END IF; + + -- We discard anything that would be truncated + IF (char_length(internal_server_name) >= 64) THEN + RAISE EXCEPTION 'Server name (%) is too long to be used as identifier', input_name; + END IF; + + IF (check_existence AND (NOT EXISTS (SELECT * FROM pg_foreign_server WHERE srvname = internal_server_name))) THEN + RAISE EXCEPTION 'Server "%" does not exist', input_name; + END IF; + + RETURN internal_server_name::name; END $$ LANGUAGE PLPGSQL IMMUTABLE PARALLEL SAFE; @@ -315,3 +320,52 @@ BEGIN END $$ LANGUAGE PLPGSQL IMMUTABLE PARALLEL SAFE; + + +-- +-- Grant access to a server +-- +CREATE OR REPLACE FUNCTION @extschema@.CDB_Federated_Server_Grant_Access(server TEXT, usernames text[]) +RETURNS void +AS $$ +DECLARE + server_internal text := @extschema@.__CDB_FS_Generate_Server_Name(input_name := server, check_existence := true); + server_role_name name := @extschema@.__CDB_FS_Generate_Server_Role_Name(server_internal); + user_role TEXT; + username TEXT; +BEGIN + FOREACH username IN ARRAY usernames + LOOP + user_role := @extschema@._CDB_User_RoleFromUsername(username); + IF (user_role IS NULL) THEN + RAISE EXCEPTION 'User role "%" does not exists', username; + END IF; + EXECUTE format('GRANT %I TO %I', server_role_name, user_role); + END loop; +END +$$ +LANGUAGE PLPGSQL VOLATILE PARALLEL UNSAFE; + +-- +-- Revoke access to a server +-- +CREATE OR REPLACE FUNCTION @extschema@.CDB_Federated_Server_Revoke_Access(server TEXT, usernames text[]) +RETURNS void +AS $$ +DECLARE + server_internal text := @extschema@.__CDB_FS_Generate_Server_Name(input_name := server, check_existence := true); + server_role_name name := @extschema@.__CDB_FS_Generate_Server_Role_Name(server_internal); + user_role TEXT; + username TEXT; +BEGIN + FOREACH username IN ARRAY usernames + LOOP + user_role := @extschema@._CDB_User_RoleFromUsername(username); + IF (user_role IS NULL) THEN + RAISE EXCEPTION 'User role "%" does not exists', username; + END IF; + EXECUTE format('REVOKE %I FROM %I', server_role_name, user_role); + END loop; +END +$$ +LANGUAGE PLPGSQL VOLATILE PARALLEL UNSAFE; diff --git a/test/CDB_FederatedServer.sql b/test/CDB_FederatedServer.sql index ffce997..69c07a5 100644 --- a/test/CDB_FederatedServer.sql +++ b/test/CDB_FederatedServer.sql @@ -72,6 +72,7 @@ SELECT '6.1', cartodb.CDB_Federated_Server_Unregister(server := 'myRemote2'::tex SELECT '6.2', cartodb.CDB_Federated_Server_List_Servers(); -- Test empty config +SELECT '7.0', cartodb.CDB_Federated_Server_Register_PG(server := NULL::text, config := '{ "server": {}, "credentials" : {}}'); SELECT '7.1', cartodb.CDB_Federated_Server_Register_PG(server := 'empty'::text, config := '{}'); -- Test without passing credentials SELECT '7.2', cartodb.CDB_Federated_Server_Register_PG(server := 'empty'::text, config := '{ @@ -130,8 +131,67 @@ SELECT '8.3', cartodb.CDB_Federated_Server_Unregister(server := 'myRemote" or''n -- Should throw when trying to unregistering a server that doesn't exists SELECT '8.4', cartodb.CDB_Federated_Server_Unregister(server := 'Does not exist'::text); +-- Test permissions +\set QUIET on + +-- We create a username following the same steps as organization members +CREATE ROLE cdb_fs_tester LOGIN PASSWORD 'cdb_fs_passwd'; +GRANT CONNECT ON DATABASE contrib_regression TO cdb_fs_tester; +CREATE SCHEMA cdb_fs_tester AUTHORIZATION cdb_fs_tester; +SELECT cartodb.CDB_Organization_Create_Member('cdb_fs_tester'); +ALTER ROLE cdb_fs_tester SET search_path TO cdb_fs_tester,cartodb,public; + +\set QUIET off + +SELECT '9.1', cartodb.CDB_Federated_Server_Register_PG(server := 'myRemote3'::text, config := '{ + "server": { + "host": "localhost", + "port": @@PGPORT@@ + }, + "credentials": { + "username": "fdw_user", + "password": "foobarino" + } +}'::jsonb); + +\c contrib_regression cdb_fs_tester + +-- A normal user can list existing servers +SELECT '9.2', cartodb.CDB_Federated_Server_List_Servers(); +-- Creating a server without superadmin should fail +SELECT '9.3', cartodb.CDB_Federated_Server_Register_PG(server := 'myRemote4'::text, config := '{ + "server": { + "host": "localhost", + "port": @@PGPORT@@ + }, + "credentials": { + "username": "fdw_user", + "password": "foobarino" + } +}'::jsonb); + + +\c contrib_regression postgres + +SELECT '9.5', cartodb.CDB_Federated_Server_Grant_Access(server := 'myRemote3', usernames := ARRAY['cdb_fs_tester']); +SELECT '9.6', cartodb.CDB_Federated_Server_Grant_Access(server := 'does not exist', usernames := ARRAY['cdb_fs_tester']); +SELECT '9.7', cartodb.CDB_Federated_Server_Grant_Access(server := 'myRemote3', usernames := ARRAY['does not exist']); + +-- Grant again raises a notice +SELECT '9.8', cartodb.CDB_Federated_Server_Grant_Access(server := 'myRemote3', usernames := ARRAY['cdb_fs_tester']); + +-- Revoke works +SELECT '9.9', cartodb.CDB_Federated_Server_Revoke_Access(server := 'myRemote3', usernames := ARRAY['cdb_fs_tester']); +SELECT '9.10', cartodb.CDB_Federated_Server_Grant_Access(server := 'myRemote3', usernames := ARRAY['cdb_fs_tester']); + +-- Dropping the server without revoking access works +SELECT '9.11', cartodb.CDB_Federated_Server_Unregister(server := 'myRemote3'::text); + -- Cleanup \set QUIET on +DROP SCHEMA cdb_fs_tester CASCADE; +REVOKE CONNECT ON DATABASE contrib_regression FROM cdb_fs_tester; +DROP ROLE cdb_fs_tester; DROP EXTENSION postgres_fdw; \set QUIET off diff --git a/test/CDB_FederatedServer_expect b/test/CDB_FederatedServer_expect index 68972b1..3c11692 100644 --- a/test/CDB_FederatedServer_expect +++ b/test/CDB_FederatedServer_expect @@ -11,6 +11,7 @@ 4.2|(myRemote2,postgres_fdw,localhost,5432,fdw_target,read-only,other_remote_user) ERROR: Server "doesNotExist" does not exist 6.1| +ERROR: Server name cannot be NULL ERROR: Server information is mandatory ERROR: Credentials are mandatory 7.3| @@ -21,3 +22,17 @@ ERROR: Server information is mandatory 8.2|("myRemote"" or'not",postgres_fdw,localhost,5432,"fdw target",read-only,"fdw user") 8.3| ERROR: Server "Does not exist" does not exist + +9.1| +You are now connected to database "contrib_regression" as user "cdb_fs_tester". +9.2|(myRemote3,postgres_fdw,localhost,5432,,read-only,) +ERROR: Could not create server myRemote4: permission denied for foreign-data wrapper postgres_fdw +You are now connected to database "contrib_regression" as user "postgres". +9.5| +ERROR: Server "does not exist" does not exist +ERROR: User role "does not exist" does not exists +NOTICE: role "cdb_fs_tester" is already a member of role "cdb_fs_role_95b63382aabca4433e7bd9cba6c30368" +9.8| +9.9| +9.10| +9.11|