diff --git a/.gitignore b/.gitignore index 941a061..8122bbe 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,13 @@ control.mk # built targets # Note: Version-specific files (sql/*--*.sql) are now tracked in git and should be committed +# Exception: the `stable` pseudo-version (default_version between releases, +# see ../ai/RELEASE.md and ../ai/CLAUDE.md's "Version-specific SQL files") +# is permanently current, not a frozen release -- it would be regenerated +# and re-diffed on every source edit for zero test-coverage value if +# tracked. The update script *to* stable (sql/*----stable.sql) +# is not affected by this and must stay committed. +sql/*--stable.sql # Generated by asciidoctor from doc/*.asc; only the .asc source is tracked doc/*.html diff --git a/bin/test_existing b/bin/test_existing index a4f8a74..d92e4be 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -26,12 +26,12 @@ # # run-suite DB # Assert the installed version matches the current build (never -# hardcoded -- derived from `make -s print-PGXNVERSION`, so a broken -# extraction can't silently compare "" to ""), then run the suite in -# existing mode via --use-existing (pg_regress must not drop/recreate -# DB) and gate on verify-results, not a bare `make test` (pgxntool -# marks installcheck .IGNORE, so a plain test run exits 0 even when -# regression.diffs is nonempty). +# hardcoded -- derived from `make -s print-EXTENSION_test_factory_VERSION`, +# so a broken extraction can't silently compare "" to ""), then run +# the suite in existing mode via --use-existing (pg_regress must not +# drop/recreate DB) and gate on verify-results, not a bare `make test` +# (pgxntool marks installcheck .IGNORE, so a plain test run exits 0 +# even when regression.diffs is nonempty). # # No separate guard-planting subcommand: unlike cat_tools, test_factory's # dependency guard (a view in schema test_factory_drop_guard depending on @@ -57,8 +57,15 @@ psql_do() { psql -d "$db" -v ON_ERROR_STOP=1 "$@" } +# EXTENSION_test_factory_VERSION, not PGXNVERSION: the latter is the +# *distribution* version from META.json, which no longer tracks what +# CREATE EXTENSION actually installs now that test_factory.control's +# default_version sits at the literal 'stable' pseudo-version between +# releases (see ../ai/RELEASE.md) -- a bare CREATE EXTENSION always +# resolves to whatever default_version says, not the distribution's +# semver. current_version() { - make -s print-PGXNVERSION 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p' + make -s print-EXTENSION_test_factory_VERSION 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p' } installed_version() { diff --git a/sql/test_factory--0.5.0--stable.sql b/sql/test_factory--0.5.0--stable.sql new file mode 100644 index 0000000..7ba91f7 --- /dev/null +++ b/sql/test_factory--0.5.0--stable.sql @@ -0,0 +1,50 @@ +/* + * https://github.com/Postgres-Extensions/test_factory/issues/14: whoever + * originally installed 0.5.0 never automatically got SET-enabled (PG16+) + * or even plain (pre-16) membership in test_factory__owner -- that's the + * bug, and it applies just as much to an already-existing 0.5.0 install + * as to a fresh one (see sql/test_factory.sql's own comment for the fresh + * case). This grants it retroactively, so whoever runs this update -- and + * anyone else who later needs to SET ROLE test_factory__owner -- has it. + * + * No object in this extension changed between 0.5.0 and here, so unlike + * the fresh-install script, there's nothing to create or alter AS + * test_factory__owner, and so no role to switch to or restore. + */ +DO $body$ +BEGIN + IF current_setting('server_version_num')::int >= 160000 THEN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN + BEGIN + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + EXCEPTION + WHEN insufficient_privilege THEN + RAISE EXCEPTION + 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' + , current_user + USING ERRCODE = 'insufficient_privilege' + , HINT = format( + 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' + , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user) + ); + END; + END IF; + ELSIF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN + BEGIN + EXECUTE format('GRANT test_factory__owner TO %I', current_user); + EXCEPTION + WHEN insufficient_privilege THEN + RAISE EXCEPTION + 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' + , current_user + USING ERRCODE = 'insufficient_privilege' + , HINT = format( + 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' + , format('GRANT test_factory__owner TO %I;', current_user) + ); + END; + END IF; +END +$body$; + +-- vi: expandtab ts=2 sw=2 diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index 38db882..37e742b 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -6,7 +6,6 @@ * it's much lighter weight than creating a table. */ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); - DO $body$ BEGIN CREATE ROLE test_factory__owner; @@ -16,86 +15,6 @@ EXCEPTION END $body$; -/* - * CREATE ROLE alone never grants the creator any relationship to the role - * it just created -- on ANY version, confirmed directly against a genuine - * non-superuser CREATEROLE installer with zero prior grants: SET ROLE - * test_factory__owner below fails outright ("permission denied to set - * role") without an explicit GRANT first, even immediately after this - * same session created it. A real superuser bypasses the SET ROLE check - * entirely regardless of any of this, which is why this was easy to miss - * without testing against a genuine non-superuser role. - * - * As of PG16, plain membership isn't enough either -- SET ROLE requires - * the SET option specifically, which needs GRANT ... WITH SET TRUE. - * Before PG16, plain membership (no WITH SET syntax, which doesn't exist - * yet) already confers the ability to SET ROLE. pg_has_role's 'SET' - * privilege type is PG16+ only, but as a plain function argument (not a - * catalog column reference) it's safe to call inside this same - * version-gated branch, unlike a static reference to - * pg_auth_members.set_option, which would fail to parse on older servers - * even inside the gate. - * - * Either way, skip the GRANT when it's not actually needed: an installer - * might already have the membership it needs some other way -- e.g. a DBA - * pre-provisioned the role and granted it directly, deliberately - * withholding ADMIN OPTION as a least-privilege measure -- and GRANT ROLE - * requires ADMIN OPTION on the target role (or superuser) to run AT ALL, - * regardless of whether it would end up a no-op; forcing it unconditionally - * broke that already-working case (confirmed against a live non-superuser - * role with SET but not ADMIN OPTION: "ERROR: permission denied to grant - * role ... Only roles with the ADMIN option ... may grant this role"). - * - * If the installer has neither the membership it needs nor ADMIN OPTION to - * grant it themselves, installation genuinely cannot proceed -- nobody - * else can do it on their behalf from inside this script. Catch that - * specific case and say so plainly instead of surfacing Postgres's generic - * permission error. - */ -DO $body$ -BEGIN - IF current_setting('server_version_num')::int >= 160000 THEN - IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN - BEGIN - EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); - EXCEPTION - WHEN insufficient_privilege THEN - /* - * RAISE's own %-substitution is plain string - * interpolation, not format()'s %I/%L -- build the - * copy-pastable suggested command with format() first - * (so the role name is properly identifier-quoted), - * then substitute the whole result in with a single, - * ordinary %. - */ - RAISE EXCEPTION - 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' - , current_user - USING ERRCODE = 'insufficient_privilege' - , HINT = format( - 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' - , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user) - ); - END; - END IF; - ELSIF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN - BEGIN - EXECUTE format('GRANT test_factory__owner TO %I', current_user); - EXCEPTION - WHEN insufficient_privilege THEN - RAISE EXCEPTION - 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' - , current_user - USING ERRCODE = 'insufficient_privilege' - , HINT = format( - 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' - , format('GRANT test_factory__owner TO %I;', current_user) - ); - END; - END IF; -END -$body$; - CREATE SCHEMA tf AUTHORIZATION test_factory__owner; COMMENT ON SCHEMA tf IS $$Test factory. Tools for maintaining test data.$$; GRANT USAGE ON SCHEMA tf TO public; diff --git a/test/install/load.sql b/test/install/load.sql index 5c084f2..9f6ad7f 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -246,39 +246,30 @@ SELECT :'load_mode' = 'existing' AS is_existing DROP EXTENSION IF EXISTS test_factory_pgtap CASCADE; DROP EXTENSION IF EXISTS test_factory CASCADE; - -- Everything from here on -- the actual install this mode exists to - -- test -- runs as the installer instead. - SET SESSION AUTHORIZATION :installer_role; - - /* - * pgtap must land in a dedicated "tap" schema BEFORE test_factory_pgtap - * installs, not after. test_factory_pgtap.control declares pgtap as a - * requirement too, so the CREATE EXTENSION ... CASCADE (or plain CREATE - * EXTENSION, in update mode) below would otherwise cascade-install pgtap - * itself into whatever schema this session's ambient search_path - * resolves to (public, by default) -- and since that satisfies "pgtap is - * already installed", the main suite's own tap_setup.sql (which does - * CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap) then skips creating it - * in "tap" at all, leaving every pgTAP-based test file failing with - * "function no_plan() does not exist" (hit this for real writing this - * file). IF NOT EXISTS on both statements: harmless no-op on a rerun - * where a previous pass already did this and the drop-first reset above - * didn't touch it (cascading test_factory_pgtap's drop doesn't remove - * pgtap -- pgtap is its dependency, not the other way around). - */ - CREATE SCHEMA IF NOT EXISTS tap; - CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; - - -- Captured before either branch below runs CREATE EXTENSION, so the - -- role-restore proof after \endif covers whichever one actually ran. - SELECT current_user AS role_before_install - \gset - SELECT :'load_mode' = 'update' AS is_update \gset -- update-vs-fresh install \if :is_update + /* + * update mode runs entirely as the ambient role, not + * test_factory_installer: extension ownership can't be transferred + * (`ALTER EXTENSION ... OWNER TO` isn't valid syntax -- confirmed), + * so only the role that ran the ORIGINAL CREATE EXTENSION can ever + * run ALTER EXTENSION UPDATE on it. There's no reachable "a + * different non-superuser applies this later" scenario to test here + * -- whether the original install itself could have been done + * non-superuser is exactly what the fresh branch below already + * proves. + */ + CREATE SCHEMA IF NOT EXISTS tap; + CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; + + -- Captured before CREATE EXTENSION, so the role-restore proof below + -- covers it too. + SELECT current_user AS role_before_install + \gset + CREATE EXTENSION test_factory VERSION :'update_from'; SET client_min_messages = ERROR; -- suppress update-script deprecation NOTICEs \if :has_update_to @@ -297,12 +288,36 @@ SELECT :'load_mode' = 'existing' AS is_existing -- update-vs-fresh install \else + -- Everything from here on -- the actual install this mode exists to + -- test -- runs as the installer instead. + SET SESSION AUTHORIZATION :installer_role; + + /* + * pgtap must land in a dedicated "tap" schema BEFORE test_factory_pgtap + * installs, not after. test_factory_pgtap.control declares pgtap as a + * requirement too, so the CREATE EXTENSION ... CASCADE below would + * otherwise cascade-install pgtap itself into whatever schema this + * session's ambient search_path resolves to (public, by default) -- + * and since that satisfies "pgtap is already installed", the main + * suite's own tap_setup.sql (which does CREATE EXTENSION IF NOT + * EXISTS pgtap SCHEMA tap) then skips creating it in "tap" at all, + * leaving every pgTAP-based test file failing with "function + * no_plan() does not exist" (hit this for real writing this file). + * IF NOT EXISTS on both statements: harmless no-op on a rerun where a + * previous pass already did this and the drop-first reset above + * didn't touch it (cascading test_factory_pgtap's drop doesn't + * remove pgtap -- pgtap is its dependency, not the other way + * around). + */ + CREATE SCHEMA IF NOT EXISTS tap; + CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; + + -- Captured before CREATE EXTENSION, so the role-restore proof below + -- covers it too. + SELECT current_user AS role_before_install + \gset + /* - * fresh: install for real, right here, uniformly with update/existing -- - * so test/sql/base.sql and test/sql/pgtap.sql can assume both - * extensions are already present in every mode, instead of each mode - * needing its own install-or-skip dance (what test/helpers/ - * create_extension.sql used to do; deleted along with this change). * CASCADE proves test_factory_pgtap.control's "requires = 'pgtap, * test_factory'" line actually pulls test_factory in -- * test/sql/pgtap.sql separately proves the dependency is *enforced* @@ -310,6 +325,49 @@ SELECT :'load_mode' = 'existing' AS is_existing */ CREATE EXTENSION test_factory_pgtap CASCADE; + /* + * Regression test for https://github.com/Postgres-Extensions/test_factory/issues/14, + * checked from THIS session (still test_factory_installer, per the + * SET SESSION AUTHORIZATION above) rather than from + * test/sql/base.sql's own separate connection, which would only ever + * see whatever role CI or a developer happens to connect as -- + * pg_has_role(current_user, ...) is unconditionally true for a + * superuser regardless of whether the fix's GRANT logic in + * sql/test_factory.sql ever ran. test_factory_installer is freshly + * created every run with zero prior relationships, so it can only + * have this membership if that GRANT actually fired -- there's no + * other way it could already be there. Only checked here, not in + * update mode above: update mode never switches to + * test_factory_installer (see its own comment), so current_user + * there is whatever ambient role connected -- typically a superuser, + * for whom this would be checking nothing. + * + * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as + * sql/test_factory.sql's own comment); pre-16, plain membership is + * the closest equivalent ('SET' isn't a meaningful distinction yet -- + * membership itself already confers the ability to SET ROLE). + */ + SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus + \gset + -- pg16+ SET-enabled membership check + \if :pg16plus + DO $$ + BEGIN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN + RAISE EXCEPTION 'issue #14 regression: % lacks SET-enabled membership in test_factory__owner after install', current_user; + END IF; + END $$; + -- pg16+ SET-enabled membership check + \else + DO $$ + BEGIN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN + RAISE EXCEPTION 'issue #14 regression: % lacks membership in test_factory__owner after install', current_user; + END IF; + END $$; + -- pg16+ SET-enabled membership check + \endif + -- update-vs-fresh install \endif @@ -333,44 +391,6 @@ SELECT :'load_mode' = 'existing' AS is_existing DO $$ BEGIN RAISE EXCEPTION 'CREATE EXTENSION did not restore the calling role'; END $$; \endif - /* - * Regression test for https://github.com/Postgres-Extensions/test_factory/issues/14, - * checked from THIS session (still test_factory_installer, per the SET - * SESSION AUTHORIZATION above) rather than from test/sql/base.sql's own - * separate connection, which would only ever see whatever role CI or a - * developer happens to connect as -- pg_has_role(current_user, ...) is - * unconditionally true for a superuser regardless of whether the fix's - * GRANT logic in sql/test_factory.sql ever ran. test_factory_installer is - * freshly created every run with zero prior relationships, so it can only - * have this membership if that GRANT actually fired -- there's no other - * way it could already be there. - * - * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as - * sql/test_factory.sql's own comment); pre-16, plain membership is the - * closest equivalent ('SET' isn't a meaningful distinction yet -- - * membership itself already confers the ability to SET ROLE). - */ - SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus - \gset - -- pg16+ SET-enabled membership check - \if :pg16plus - DO $$ - BEGIN - IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN - RAISE EXCEPTION 'issue #14 regression: % lacks SET-enabled membership in test_factory__owner after install', current_user; - END IF; - END $$; - -- pg16+ SET-enabled membership check - \else - DO $$ - BEGIN - IF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN - RAISE EXCEPTION 'issue #14 regression: % lacks membership in test_factory__owner after install', current_user; - END IF; - END $$; - -- pg16+ SET-enabled membership check - \endif - -- existing-vs-fresh/update \endif diff --git a/test_factory.control b/test_factory.control index b05ee89..446c18a 100644 --- a/test_factory.control +++ b/test_factory.control @@ -1,5 +1,5 @@ comment = 'A framework for managing test data' -default_version = '0.5.0' +default_version = 'stable' relocatable = false # Not a security boundary weakening: test_factory__owner is a locked-down, # dedicated owner role, and every privileged function is SECURITY DEFINER diff --git a/test_factory_pgtap.control b/test_factory_pgtap.control index 0711698..f19f82f 100644 --- a/test_factory_pgtap.control +++ b/test_factory_pgtap.control @@ -1,5 +1,5 @@ comment = 'A framework for managing test data' -default_version = '0.1.0' +default_version = 'stable' relocatable = false requires = 'pgtap, test_factory' # See test_factory.control -- same rationale.