From d268cd07cb11249ebb61c9f2ff40e08a9352ed23 Mon Sep 17 00:00:00 2001 From: Paul Ramsey Date: Thu, 13 Aug 2015 15:59:45 -0700 Subject: [PATCH] Fix many tests and handle ownership issues involved with Cartodbfy being invoked by schema triggers. Some issues with regclass interpretation in tests still remain. Some issues with slightly different behavior to old version remain. Some issues with error messages / notification messages changing a little still remain. --- scripts-available/CDB_CartodbfyTable.sql | 9 ++++ scripts-available/CDB_DDLTriggers.sql | 66 ++++++++++++++++-------- sql/test_ddl_triggers.sql | 1 + sql/test_setup.sql | 2 +- test/CDB_CartodbfyTableTest.sql | 7 ++- test/CDB_QuotaTest.sql | 12 ++--- test/CDB_QuotaTest_expect | 2 +- 7 files changed, 68 insertions(+), 31 deletions(-) diff --git a/scripts-available/CDB_CartodbfyTable.sql b/scripts-available/CDB_CartodbfyTable.sql index 2707810..91b04a2 100644 --- a/scripts-available/CDB_CartodbfyTable.sql +++ b/scripts-available/CDB_CartodbfyTable.sql @@ -973,6 +973,15 @@ BEGIN sql := Format('DROP TABLE %s', reloid::text); PERFORM _CDB_SQL(sql, '_CDB_Rewrite_Table'); + -- If the table is being created by a SECURITY DEFINER function + -- make sure the user is set back to the user who is connected + IF current_user != session_user THEN + sql := Format('ALTER TABLE IF EXISTS %s OWNER TO %s', copyname, session_user); + PERFORM _CDB_SQL(sql, '_CDB_Rewrite_Table'); + sql := Format('ALTER SEQUENCE IF EXISTS %s OWNER TO %s', destseq, session_user); + PERFORM _CDB_SQL(sql, '_CDB_Rewrite_Table'); + END IF; + -- If we used a temporary destination table -- we can now rename it into place IF destschema = relschema THEN diff --git a/scripts-available/CDB_DDLTriggers.sql b/scripts-available/CDB_DDLTriggers.sql index 81db820..7453e88 100644 --- a/scripts-available/CDB_DDLTriggers.sql +++ b/scripts-available/CDB_DDLTriggers.sql @@ -4,17 +4,23 @@ CREATE OR REPLACE FUNCTION cartodb.cdb_handle_create_table () RETURNS event_trigger SECURITY DEFINER LANGUAGE plpgsql AS $$ DECLARE event_info RECORD; - rel_namespace TEXT; + rel RECORD; + newtable REGCLASS; BEGIN event_info := schema_triggers.get_relation_create_eventinfo(); -- We're only interested in real relations IF (event_info.new).relkind != 'r' THEN RETURN; END IF; - SELECT nspname FROM pg_namespace WHERE oid=(event_info.new).relnamespace INTO rel_namespace; + SELECT c.relname, c.relnamespace, c.relkind, n.nspname + FROM pg_class c + JOIN pg_namespace n + ON c.relnamespace = n.oid + WHERE c.oid = event_info.relation + INTO rel; RAISE DEBUG 'Relation % of kind % created in table % namespace % (oid %)', - event_info.relation, (event_info.new).relkind, (event_info.new).relname::TEXT, rel_namespace, (event_info.new).relnamespace; + event_info.relation, rel.relkind, rel.relname, rel.nspname, rel.relnamespace; -- We don't want to react to alters triggered by superuser, IF current_setting('is_superuser') = 'on' THEN @@ -25,15 +31,17 @@ BEGIN PERFORM cartodb.cdb_disable_ddl_hooks(); -- CDB_CartodbfyTable must not create tables, or infinite loop will happen - PERFORM cartodb.CDB_CartodbfyTable(rel_namespace, event_info.relation); + PERFORM cartodb.CDB_CartodbfyTable(rel.nspname, event_info.relation); PERFORM cartodb.cdb_enable_ddl_hooks(); RAISE DEBUG 'Inserting into cartodb.CDB_TableMetadata'; + newtable := Format('%s.%s', rel.nspname, rel.relname)::regclass; + -- Add entry to CDB_TableMetadata (should CartodbfyTable do this?) - INSERT INTO cartodb.CDB_TableMetadata(tabname,updated_at) - VALUES (event_info.relation, now()); + INSERT INTO cartodb.CDB_TableMetadata(tabname, updated_at) + VALUES (newtable, now()); END; $$; -- } @@ -66,14 +74,19 @@ RETURNS event_trigger SECURITY DEFINER LANGUAGE plpgsql AS $$ DECLARE event_info RECORD; rel RECORD; - rel_namespace TEXT; + newtable REGCLASS; BEGIN event_info := schema_triggers.get_column_alter_eventinfo(); - SELECT oid,* FROM pg_class WHERE oid = event_info.relation INTO rel; + SELECT c.relname, c.relnamespace, c.relkind, n.nspname + FROM pg_class c + JOIN pg_namespace n + ON c.relnamespace = n.oid + WHERE c.oid = event_info.relation + INTO rel; RAISE DEBUG 'Column % altered by % (superuser? %) in relation % of kind %', - (event_info.old).attname, current_user, current_setting('is_superuser'), event_info.relation::regclass, rel.relkind; + (event_info.old).attname, current_user, current_setting('is_superuser'), rel.relname, rel.relkind; -- We're only interested in real relations IF rel.relkind != 'r' THEN RETURN; END IF; @@ -84,17 +97,17 @@ BEGIN RETURN; END IF; - SELECT nspname FROM pg_namespace WHERE oid = rel.relnamespace INTO rel_namespace; - PERFORM cartodb.cdb_disable_ddl_hooks(); - PERFORM cartodb.CDB_CartodbfyTable(rel_namespace, event_info.relation); + PERFORM cartodb.CDB_CartodbfyTable(rel.nspname, event_info.relation); PERFORM cartodb.cdb_enable_ddl_hooks(); + newtable := Format('%s.%s', rel.nspname, rel.relname)::regclass; + -- update CDB_TableMetadata.updated_at (should invalidate varnish) UPDATE cartodb.CDB_TableMetadata SET updated_at = NOW() - WHERE tabname = event_info.relation; + WHERE tabname = newtable; END; $$; -- } @@ -106,14 +119,19 @@ RETURNS event_trigger SECURITY DEFINER LANGUAGE plpgsql AS $$ DECLARE event_info RECORD; rel RECORD; - rel_namespace TEXT; + newtable REGCLASS; BEGIN event_info := schema_triggers.get_column_drop_eventinfo(); - SELECT oid,* FROM pg_class WHERE oid = event_info.relation INTO rel; + SELECT c.relname, c.relnamespace, c.relkind, n.nspname + FROM pg_class c + JOIN pg_namespace n + ON c.relnamespace = n.oid + WHERE c.oid = event_info.relation + INTO rel; RAISE DEBUG 'Column % drop by % (superuser? %) in relation % of kind %', - (event_info.old).attname, current_user, current_setting('is_superuser'), event_info.relation::regclass, rel.relkind; + (event_info.old).attname, current_user, current_setting('is_superuser'), rel.relname, rel.relkind; -- We're only interested in real relations IF rel.relkind != 'r' THEN RETURN; END IF; @@ -124,17 +142,18 @@ BEGIN RETURN; END IF; - SELECT nspname FROM pg_namespace WHERE oid = rel.relnamespace INTO rel_namespace; PERFORM cartodb.cdb_disable_ddl_hooks(); - PERFORM cartodb.CDB_CartodbfyTable(rel_namespace, event_info.relation); + PERFORM cartodb.CDB_CartodbfyTable(rel.nspname, event_info.relation); PERFORM cartodb.cdb_enable_ddl_hooks(); + newtable := Format('%s.%s', rel.nspname, rel.relname)::regclass; + -- update CDB_TableMetadata.updated_at (should invalidate varnish) UPDATE cartodb.CDB_TableMetadata SET updated_at = NOW() - WHERE tabname = event_info.relation; + WHERE tabname = newtable; END; $$; -- } @@ -149,10 +168,15 @@ DECLARE BEGIN event_info := schema_triggers.get_column_add_eventinfo(); - SELECT oid,* FROM pg_class WHERE oid = event_info.relation INTO rel; + SELECT c.relname, c.relnamespace, c.relkind, n.nspname + FROM pg_class c + JOIN pg_namespace n + ON c.relnamespace = n.oid + WHERE c.oid = event_info.relation + INTO rel; RAISE DEBUG 'Column % added by % (superuser? %) in relation % of kind %', - (event_info.new).attname, current_user, current_setting('is_superuser'), event_info.relation::regclass, rel.relkind; + (event_info.new).attname, current_user, current_setting('is_superuser'), rel.relname, rel.relkind; -- We're only interested in real relations IF rel.relkind != 'r' THEN RETURN; END IF; diff --git a/sql/test_ddl_triggers.sql b/sql/test_ddl_triggers.sql index 8d7bbaf..f4b83dc 100644 --- a/sql/test_ddl_triggers.sql +++ b/sql/test_ddl_triggers.sql @@ -10,6 +10,7 @@ create schema c; SELECT CDB_SetUserQuotaInBytes('c', 0); +DROP USER IF EXISTS cartodb_postgresql_unpriv_user; CREATE USER cartodb_postgresql_unpriv_user; GRANT ALL ON SCHEMA c to cartodb_postgresql_unpriv_user; SET SESSION AUTHORIZATION 'cartodb_postgresql_unpriv_user'; diff --git a/sql/test_setup.sql b/sql/test_setup.sql index c05769f..67fe684 100644 --- a/sql/test_setup.sql +++ b/sql/test_setup.sql @@ -7,4 +7,4 @@ RETURNS void AS $$ BEGIN RAISE NOTICE 'cdb_invalidate_varnish(%) called', table_name; END; -$$ LANGUAGE 'plpgsql'; +$$ LANGUAGE 'plpgsql'; \ No newline at end of file diff --git a/test/CDB_CartodbfyTableTest.sql b/test/CDB_CartodbfyTableTest.sql index eafb79b..0b6bcab 100644 --- a/test/CDB_CartodbfyTableTest.sql +++ b/test/CDB_CartodbfyTableTest.sql @@ -12,6 +12,7 @@ DECLARE tmp INTEGER; ogc_geom geometry_columns; -- old the_geom record in geometry_columns ogc_merc geometry_columns; -- old the_geom_webmercator record in geometry_columns + tabtext TEXT; BEGIN -- Save current constraints on geometry columns, if any @@ -30,7 +31,10 @@ BEGIN END IF; END LOOP; + tabtext := Format('%s.%s','public',tabname); + RAISE NOTICE 'CARTODBFYING % !!!!', tabtext; PERFORM CDB_CartodbfyTable('public', tabname); + tabname := tabtext::regclass; sql := 'INSERT INTO ' || tabname::text || '(the_geom) values ( CDB_LatLng(2,1) ) RETURNING cartodb_id'; EXECUTE sql INTO STRICT id; @@ -121,9 +125,8 @@ $$ LANGUAGE 'plpgsql'; -- table with single non-geometrical column -CREATE TABLE t AS SELECT 1::int as a; -SELECT CDB_CartodbfyTable('public', 't'); -- should fail SELECT CDB_SetUserQuotaInBytes(0); -- Set user quota to infinite +CREATE TABLE t AS SELECT 1::int as a; SELECT CDB_CartodbfyTableCheck('t', 'single non-geometrical column'); DROP TABLE t; diff --git a/test/CDB_QuotaTest.sql b/test/CDB_QuotaTest.sql index e54a74d..6a253af 100644 --- a/test/CDB_QuotaTest.sql +++ b/test/CDB_QuotaTest.sql @@ -7,16 +7,16 @@ CREATE TABLE big(a int); CREATE TRIGGER test_quota BEFORE UPDATE OR INSERT ON big EXECUTE PROCEDURE CDB_CheckQuota(1, 1, 'public'); INSERT INTO big VALUES (1); -- allowed, check runs before -INSERT INTO big VALUES (1); -- disallowed, quota exceeds before +INSERT INTO big VALUES (2); -- disallowed, quota exceeds before SELECT CDB_SetUserQuotaInBytes(0); SELECT CDB_CartodbfyTable('big'); -INSERT INTO big SELECT generate_series(1,2048); -INSERT INTO big SELECT generate_series(1,2048); -INSERT INTO big SELECT generate_series(1,2048); +INSERT INTO big SELECT generate_series(2049,4096); +INSERT INTO big SELECT generate_series(4097,6144); +INSERT INTO big SELECT generate_series(6145,8192); SELECT CDB_SetUserQuotaInBytes(2); -INSERT INTO big VALUES (1); +INSERT INTO big VALUES (8193); SELECT CDB_SetUserQuotaInBytes(0); -INSERT INTO big VALUES (1); +INSERT INTO big VALUES (8194); DROP TABLE big; set client_min_messages to NOTICE; DROP FUNCTION _CDB_UserQuotaInBytes(); diff --git a/test/CDB_QuotaTest_expect b/test/CDB_QuotaTest_expect index 72397a9..7249c63 100644 --- a/test/CDB_QuotaTest_expect +++ b/test/CDB_QuotaTest_expect @@ -9,7 +9,7 @@ INSERT 0 2048 INSERT 0 2048 INSERT 0 2048 2 -ERROR: Quota exceeded by 519.998046875KB +ERROR: Quota exceeded by 443.998046875KB 0 INSERT 0 1 DROP TABLE