diff --git a/HISTORY.md b/HISTORY.md index 76c6d30..6d91925 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -5,9 +5,9 @@ stable == CREATE EXTENSION no longer requires a superuser count_nulls is pure SQL functions, so the control file now says -`superuser = false`: any role with `CREATE` on the target schema can install -it. The test suite runs as an ordinary role throughout, so this can't -silently regress. +`superuser = false`: any role with `USAGE` and `CREATE` on the target schema +can install it, and needs nothing at all on the database. The test suite runs +as exactly such a role throughout, so this can't silently regress. 1.0.0 ----- diff --git a/README.md b/README.md index cca31ee..00bc102 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,11 @@ running: CREATE EXTENSION count_nulls; No superuser required: count_nulls is nothing but SQL functions, so any role -with `CREATE` on the target schema can add it. +with `USAGE` and `CREATE` on the target schema can add it, without any +privilege on the database itself. `CREATE` alone is not enough - the install +script runs with the target schema on `search_path`, and `search_path` skips a +schema the role cannot `USAGE`, so the extension would be built in `pg_temp` +and lost at disconnect. If you've upgraded your cluster to PostgreSQL 9.1 and already had count_nulls installed, you can upgrade it to a properly packaged extension with: diff --git a/count_nulls.control b/count_nulls.control index 5c938a7..19fcff1 100644 --- a/count_nulls.control +++ b/count_nulls.control @@ -2,8 +2,9 @@ comment = 'Count the number of null arguments' default_version = 'stable' relocatable = false -# Pure SQL functions, nothing privileged - anyone with CREATE on the target -# schema can install it. Enforced by the suite running as a non-superuser +# Pure SQL functions, nothing privileged - anyone with USAGE and CREATE on the +# target schema can install it, holding nothing on the database itself. +# Enforced by the suite running as exactly such a role # (test/helpers/use_test_user.sql), which fails outright if this reverts to the # default. superuser = false diff --git a/test/README.md b/test/README.md index db93488..9c71b3f 100644 --- a/test/README.md +++ b/test/README.md @@ -12,16 +12,17 @@ just defines it, leaving the caller to decide when to use it. - `install/load.sql` — installs count_nulls once, committed, before the `test/sql/` schedule, per `TEST_LOAD_SOURCE`. Its output isn't tracked; it fails loudly instead. -- `deps.sql` — per-test-session setup; drops the session to the test user. +- `deps.sql` — per-test-session setup; creates `_null_count_test` for the + `test__*` library to live in, then drops the session to the test user. - `core/functions.sql` — `ncs()`, plus the shared `test__*` library. - `sql/extension_tests.sql` — adds `test__check_ncs` and `test__shutdown__drop_all`, then calls `runtests()`. - `helpers/use_test_user.sql` — switches the session to the non-superuser - role. + role, granting it its rights on `:count_nulls_grant_schema` first. - `helpers/extension_installer.sql` — defines the functions that clean up - leftover test schemas and install count_nulls at a given version into a - fresh, randomly named one. -- `helpers/create_test_schema.sql` — calls both, with `:version`; a file + leftover test schemas, produce a fresh, randomly named one, and install + count_nulls at a given version into it. +- `helpers/create_test_schema.sql` — calls all three, with `:version`; a file only because `bin/test_existing`'s `prepare-old` runs it standalone. - `helpers/find_test_schema.sql` — finds that schema again, from a session that didn't create it. @@ -36,6 +37,12 @@ count_nulls`). That's what makes `superuser = false` in `helpers/use_test_user.sql`, including why it deliberately does *not* switch when a real `pg_upgrade` has left the extension owned by someone else. +That role holds **no privilege on the database** — every schema it works in is +created by the connecting role, which grants it `USAGE` and `CREATE` on that +one schema and nothing else. So a regression that made installing count_nulls +depend on database-level rights fails the suite instead of passing on a +privilege the test role happened to have. + **count_nulls is installed into a randomly named schema**, never a fixed one and never the default. That's what gives `core/functions.sql`'s `%I`-qualified calls their meaning: the extension's schema can never diff --git a/test/core/functions.sql b/test/core/functions.sql index c5cd932..57031a6 100644 --- a/test/core/functions.sql +++ b/test/core/functions.sql @@ -1,6 +1,4 @@ -CREATE SCHEMA _null_count_test; - --- See bottom as well! +-- Schema created by test/deps.sql, as the connecting role. See bottom as well! SET SEARCH_PATH = _null_count_test, tap; CREATE FUNCTION ncs() RETURNS name IMMUTABLE LANGUAGE sql AS $$ diff --git a/test/deps.sql b/test/deps.sql index 01be477..19bb27c 100644 --- a/test/deps.sql +++ b/test/deps.sql @@ -11,4 +11,14 @@ SELECT current_setting('count_nulls.test_load_mode') AS count_nulls_load_mode \gset +/* + * Where the test__* functions and their fixtures live (test/core/functions.sql + * puts it on search_path). Created here, by the connecting role, so that the + * test user needs no privilege on the database - only what use_test_user.sql + * grants it below on this one schema. Rolled back with the rest of the + * session, like every other object a test/sql/ session makes. + */ +CREATE SCHEMA _null_count_test; +\set count_nulls_grant_schema _null_count_test + \i test/helpers/use_test_user.sql diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index 2e151b0..42e1edc 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -4,11 +4,12 @@ * standalone (psql -v version= -f); the work itself lives in * test/helpers/extension_installer.sql. * - * The install itself runs as a non-superuser (see - * test/helpers/use_test_user.sql), which is what proves count_nulls doesn't - * need superuser to install. Cleanup happens before that switch: a leftover - * schema can belong to any role, and only the connecting one is sure to be - * able to drop it. + * The install itself runs as a non-superuser holding nothing but a grant on + * the target schema (see test/helpers/use_test_user.sql), which is what + * proves count_nulls needs neither superuser nor any database-level + * privilege. Cleanup and the schema creation happen before that switch, as + * the connecting role: a leftover schema can belong to any role, and only + * the connecting one is sure to be able to drop it. */ \i test/helpers/extension_installer.sql @@ -18,6 +19,9 @@ */ SELECT pg_temp.count_nulls_cleanup_test_schemas('fresh'); +SELECT pg_temp.count_nulls_prepare_test_schema('fresh') AS count_nulls_grant_schema +\gset + /* * :count_nulls_load_mode must already be set by the caller * (bin/test_existing's -v on the command line - see the file header) - @@ -26,7 +30,9 @@ SELECT pg_temp.count_nulls_cleanup_test_schemas('fresh'); */ \i test/helpers/use_test_user.sql -SELECT pg_temp.count_nulls_install_extension(:'version') AS count_nulls_test_schema +SELECT pg_temp.count_nulls_install_extension( + :'count_nulls_grant_schema', :'version' + ) AS count_nulls_installed \gset -- vi: expandtab sw=2 ts=2 diff --git a/test/helpers/extension_installer.sql b/test/helpers/extension_installer.sql index 12cc161..9dfe13a 100644 --- a/test/helpers/extension_installer.sql +++ b/test/helpers/extension_installer.sql @@ -1,11 +1,13 @@ /* * Defines, but does not call, the functions that find-and-drop leftover test - * schemas and install count_nulls into a fresh one. Split from calling them, - * and from each other, so test/install/load.sql can decide server-side - * whether to install at all, and so every caller can run cleanup before - * switching to the test user (test/helpers/use_test_user.sql) while - * installing after - a leftover schema can belong to any role, and only the - * connecting one is sure to be able to drop it. + * schemas, produce the schema count_nulls goes into, and install it there. + * Split from calling them, and from each other, so test/install/load.sql can + * decide server-side whether to install at all, and so every caller can run + * the first two as the connecting role and only the install as the test user + * (test/helpers/use_test_user.sql). Cleanup has to be the connecting role's + * because a leftover schema can belong to any role; the schema creation has + * to be, because the whole point is that the test user holds nothing but + * what it was granted on that one schema. */ CREATE OR REPLACE FUNCTION pg_temp.count_nulls_cleanup_test_schemas( p_mode text @@ -25,7 +27,7 @@ BEGIN /* * A run that died before its own teardown leaves a schema nothing else * knows the name of, so match the prefix (see - * count_nulls_install_extension()'s c_prefix below) and drop whatever's + * count_nulls_prepare_test_schema()'s c_prefix below) and drop whatever's * there. */ FOR r IN SELECT nspname FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' LOOP @@ -34,8 +36,16 @@ BEGIN END $body$; -CREATE OR REPLACE FUNCTION pg_temp.count_nulls_install_extension( - p_version text +/* + * Names the schema count_nulls is about to be installed into, creating it as + * the connecting role. 'existing' instead finds the schema a prior, separate + * prepare-old run installed into: that installation is the thing the mode + * exists to test, so creating a second schema here would both destroy the + * one-schema invariant test/helpers/find_test_schema.sql relies on and leave + * the real one untested. + */ +CREATE OR REPLACE FUNCTION pg_temp.count_nulls_prepare_test_schema( + p_mode text ) RETURNS name LANGUAGE plpgsql AS $body$ DECLARE /* @@ -46,6 +56,40 @@ DECLARE */ c_prefix CONSTANT text := 'count_nulls test schema '; v_schema name; +BEGIN + IF p_mode = 'existing' THEN + SELECT nspname INTO v_schema + FROM pg_namespace n JOIN pg_extension x ON n.oid = x.extnamespace + WHERE extname = 'count_nulls' + ; + + IF v_schema IS NULL THEN + RAISE EXCEPTION + 'count_nulls is not installed, so mode ''%'' has no test schema to find' + , p_mode + ; + END IF; + + RETURN v_schema; + END IF; + + v_schema := c_prefix || substr(md5(random()::text), 1, 12); + EXECUTE format('CREATE SCHEMA %I', v_schema); + + RETURN v_schema; +END +$body$; + +/* + * Runs as the test user, on a schema the connecting role made and granted it + * rights on (test/helpers/use_test_user.sql) - which is what makes a + * successful install prove count_nulls needs no privilege on the database + * itself, only on its target schema. + */ +CREATE OR REPLACE FUNCTION pg_temp.count_nulls_install_extension( + p_schema name + , p_version text +) RETURNS void LANGUAGE plpgsql AS $body$ BEGIN /* * 'current' means whatever the control file's default_version is, matching @@ -56,9 +100,6 @@ BEGIN RAISE EXCEPTION $$p_version must be set explicitly, or 'current'$$; END IF; - v_schema := c_prefix || substr(md5(random()::text), 1, 12); - EXECUTE format('CREATE SCHEMA %I', v_schema); - /* * WITH SCHEMA rather than arranging search_path first: this way a * successful install proves the install script doesn't depend on @@ -67,11 +108,29 @@ BEGIN */ EXECUTE format( 'CREATE EXTENSION count_nulls WITH SCHEMA %I%s' - , v_schema + , p_schema , CASE WHEN p_version = 'current' THEN '' ELSE format(' VERSION %L', p_version) END ); - RETURN v_schema; + /* + * CREATE EXTENSION runs the install script with search_path set to + * ", pg_temp", and search_path silently drops a schema the role + * lacks USAGE on - so a role holding only CREATE builds the entire + * extension in pg_temp, where it disappears at disconnect, and CREATE + * EXTENSION still reports success. pg_extension.extnamespace says the + * target either way, so the only way to tell is to look for the objects. + */ + IF NOT EXISTS( + SELECT 1 + FROM pg_depend d + JOIN pg_proc p ON p.oid = d.objid AND d.classid = 'pg_proc'::regclass + JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE d.deptype = 'e' + AND d.refobjid = (SELECT oid FROM pg_extension WHERE extname = 'count_nulls') + AND n.nspname = p_schema + ) THEN + RAISE EXCEPTION 'count_nulls installed no functions into schema %', p_schema; + END IF; END $body$; diff --git a/test/helpers/use_test_user.sql b/test/helpers/use_test_user.sql index b8e1fad..f5cdcef 100644 --- a/test/helpers/use_test_user.sql +++ b/test/helpers/use_test_user.sql @@ -10,6 +10,9 @@ * test/deps.sql (each test/sql/ session, via pgxntool's setup.sql), * test/install/load.sql (the install session) and * test/helpers/create_test_schema.sql (bin/test_existing's prepare-old). + * Each of them creates the schema its session works in first, as the + * connecting role, and names it in :count_nulls_grant_schema - the test user + * ends up with rights on that schema and nothing else. * * RESET ROLE first, so what this file does depends only on how the session * connected and not on anything an earlier \i of it already did - it needs @@ -35,8 +38,9 @@ RESET ROLE; * p_load_mode must come from the caller, not from reading * count_nulls.test_load_mode in here: the Makefile only exports that GUC via * PGOPTIONS for pg_regress sessions, and bin/test_existing's prepare-old - * invokes psql directly without it. Every includer sets the psql variable - * count_nulls_load_mode before \i-ing this file. + * invokes psql directly without it. Every includer sets the psql variables + * count_nulls_load_mode and count_nulls_grant_schema before \i-ing this + * file. * * TODO: collapse this into a \gset + \if once 10 is the oldest version * supported - the plpgsql is only here to work around \if's absence. @@ -44,6 +48,7 @@ RESET ROLE; CREATE OR REPLACE FUNCTION pg_temp.count_nulls_prepare_test_user( p_test_user name , p_load_mode text + , p_grant_schema name ) RETURNS name LANGUAGE plpgsql AS $body$ DECLARE /* @@ -118,15 +123,15 @@ BEGIN END IF; /* - * Can't fold into the CREATE ROLE above: the role is cluster-wide and - * outlives any one run, but this grant lives in the current database's - * ACL, so a run against a new database still has to issue it. + * The test user's entire footprint: rights on the one schema its caller + * already created for it, and nothing on the database. USAGE as well as + * CREATE because search_path skips a schema the role can't use at all. * * These two grants are all the suite gets. Anything else turning out to * be necessary is a finding about count_nulls, not something to grant. */ EXECUTE format( - 'GRANT CREATE ON DATABASE %I TO %I', current_database(), p_test_user + 'GRANT USAGE, CREATE ON SCHEMA %I TO %I', p_grant_schema, p_test_user ); /* @@ -164,7 +169,11 @@ BEGIN END $body$; -SELECT pg_temp.count_nulls_prepare_test_user(:'test_user', :'count_nulls_load_mode') AS count_nulls_run_as +SELECT pg_temp.count_nulls_prepare_test_user( + :'test_user' + , :'count_nulls_load_mode' + , :'count_nulls_grant_schema' + ) AS count_nulls_run_as \gset SET ROLE :"count_nulls_run_as"; diff --git a/test/install/load.sql b/test/install/load.sql index 72b0664..23fc8d0 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -18,10 +18,11 @@ -- TODO: this file's mode branching could move back to \if once PG10 is the floor /* - * Definitions only - safe to load before the test-user switch below. - * Cleanup runs next, as the connecting role (a leftover schema can belong to - * any role, and only the connecting one is sure to be able to drop it), then - * the switch, then the actual install. + * Definitions only - safe to load before the test-user switch below. As the + * connecting role: cleanup (a leftover schema can belong to any role, and + * only the connecting one is sure to be able to drop it), then creating the + * schema count_nulls will go in. Only the install itself runs as the test + * user. */ \i test/helpers/extension_installer.sql @@ -37,6 +38,15 @@ SELECT pg_temp.count_nulls_cleanup_test_schemas( , current_setting('count_nulls.test_load_mode') AS count_nulls_load_mode \gset +/* + * use_test_user.sql grants the test user its rights on this schema, and the + * grant is all it gets - so it has to exist, owned by the connecting role, + * before the switch. + */ +SELECT pg_temp.count_nulls_prepare_test_schema(:'count_nulls_load_mode') + AS count_nulls_grant_schema +\gset + \i test/helpers/use_test_user.sql /* @@ -51,6 +61,7 @@ SELECT pg_temp.count_nulls_cleanup_test_schemas( CREATE OR REPLACE FUNCTION pg_temp.count_nulls_load( p_mode text , p_deploy text + , p_schema name ) RETURNS void LANGUAGE plpgsql AS $body$ DECLARE -- The oldest version we still ship a full install script for @@ -123,7 +134,7 @@ BEGIN END IF; IF p_mode = 'update' THEN - PERFORM pg_temp.count_nulls_install_extension(c_oldest_full_install); + PERFORM pg_temp.count_nulls_install_extension(p_schema, c_oldest_full_install); /* * Deliberately no client_min_messages suppression around this. Postgres @@ -134,7 +145,7 @@ BEGIN */ ALTER EXTENSION count_nulls UPDATE; ELSE - PERFORM pg_temp.count_nulls_install_extension('current'); + PERFORM pg_temp.count_nulls_install_extension(p_schema, 'current'); END IF; END $body$; @@ -147,6 +158,7 @@ $body$; SELECT pg_temp.count_nulls_load( current_setting('count_nulls.test_load_mode') , current_setting('count_nulls.test_existing_deploy') + , :'count_nulls_grant_schema' ) AS count_nulls_loaded \gset