From 67663c79aa48cab8f3d8dd87b7a2a62a157bf0c3 Mon Sep 17 00:00:00 2001
From: Rafa de la Torre <rtorre@carto.com>
Date: Mon, 15 Jul 2019 15:50:33 +0200
Subject: [PATCH] Check removal of SECURITY DEFINER

This is just a test to see how feasible is to remove the SECURITY
DEFINER and have regular users setup their FDW.

There are still problems with this approach:
- need to grant the usage of postgres_fdw (no big issue)
- need CREATEROLE privilege. A problem in itself (see the NOTES
https://www.postgresql.org/docs/current/sql-createrole.html)

Aside from those, there are still practical problems:
```
> Executing query 'SELECT cartodb.CDB_SetUp_User_Foreign_Server('test_user_fdw', '{
   "server": {
     "extensions": "postgis",
     "dbname": "fdw_target",
     "host": "localhost",
     "port": 5432
   },
   "user_mapping": {
     "user": "fdw_user",
     "password": "foobarino"
   }
}');' as cdb_testmember_1
ERROR:  permission denied for foreign-data wrapper postgres_fdw
CONTEXT:  SQL statement "ALTER SERVER test_user_fdw OWNER TO test_user_fdw"
PL/pgSQL function cdb_setup_user_foreign_server(name,json) line 32 at EXECUTE
```
---
 scripts-available/CDB_ForeignTable.sql | 14 ++++----------
 test/extension/test.sh                 |  7 +++++++
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/scripts-available/CDB_ForeignTable.sql b/scripts-available/CDB_ForeignTable.sql
index e4cfc40..b266615 100644
--- a/scripts-available/CDB_ForeignTable.sql
+++ b/scripts-available/CDB_ForeignTable.sql
@@ -175,12 +175,6 @@ DECLARE
   row record;
   option record;
 BEGIN
-  -- TODO: refactor with original function
-  -- This function tries to be as idempotent as possible, by not creating anything more than once
-  -- (not even using IF NOT EXIST to avoid throwing warnings)
-  IF NOT EXISTS ( SELECT * FROM pg_extension WHERE extname = 'postgres_fdw') THEN
-    CREATE EXTENSION postgres_fdw;
-  END IF;
   -- Create FDW first if it does not exist
   IF NOT EXISTS ( SELECT * FROM pg_foreign_server WHERE srvname = fdw_name)
     THEN
@@ -203,6 +197,9 @@ BEGIN
        EXECUTE format('CREATE ROLE %I NOLOGIN', fdw_name);
     END IF;
 
+    -- Grant the fdw role to the caller, and permissions to grant it to others
+    EXECUTE FORMAT ('GRANT %I TO %I WITH ADMIN OPTION', fdw_name, session_user);
+
     -- Transfer ownership of the server to the fdw role
     EXECUTE format('ALTER SERVER %I OWNER TO %I', fdw_name, fdw_name);
 
@@ -234,12 +231,9 @@ BEGIN
     -- Give the fdw role ownership over the schema
     EXECUTE FORMAT ('ALTER SCHEMA %I OWNER TO %I', fdw_name, fdw_name);
 
-    -- Grant the fdw role to the caller, and permissions to grant it to others
-    EXECUTE FORMAT ('GRANT %I TO %I WITH ADMIN OPTION', fdw_name, session_user);
-
     -- TODO: Bring here the remote cdb_tablemetadata
 END
-$$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER;
+$$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE;
 
 
 -- Set up a user foreign table
diff --git a/test/extension/test.sh b/test/extension/test.sh
index 58b2ead..3462354 100755
--- a/test/extension/test.sh
+++ b/test/extension/test.sh
@@ -592,6 +592,11 @@ test_extension|public|"local-table-with-dashes"'
 
 
     # Check user-defined FDW's
+
+    # Grant the user permissions to use the postgres_fdw
+    sql postgres "GRANT USAGE ON FOREIGN DATA WRAPPER postgres_fdw TO cdb_testmember_1;"
+    sql postgres "ALTER ROLE cdb_testmember_1 WITH CREATEROLE;"
+
     # Set up a user foreign server
     read -d '' ufdw_config <<- EOF
 {
@@ -629,6 +634,8 @@ EOF
     sql postgres 'DROP schema test_user_fdw;'
     sql postgres 'DROP USER MAPPING FOR public SERVER test_user_fdw;'
     sql postgres 'DROP SERVER test_user_fdw;'
+    sql postgres 'REVOKE USAGE ON FOREIGN DATA WRAPPER postgres_fdw FROM test_user_fdw;'
+    sql postgres 'DROP ROLE test_user_fdw;'
 
     sql postgres "select pg_terminate_backend(pid) from pg_stat_activity where datname='fdw_target';"
     DATABASE=fdw_target tear_down_database