Add functions to grant and revoke permissions over federated servers

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 input_name IS NULL THEN
-    IF (char_length(internal_server_name) < 64) THEN
+        RAISE EXCEPTION 'Server name cannot be NULL';
-        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';
     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;

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
 

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|