diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 507bf1a..e73a52b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,22 +5,41 @@ # # test -- FRESH install: CREATE EXTENSION at the current # version, on every supported PostgreSQL major. The -# baseline a brand-new user gets. +# install itself runs as a disposable non-superuser +# role (test/install/load.sql's test_factory_installer +# -- see its own comment), not as whatever superuser +# CI happens to connect as: pg_has_role(current_user, +# ...) is unconditionally true for a superuser, so +# only a real non-superuser installer actually +# exercises https://github.com/Postgres-Extensions/test_factory/issues/14's +# fix. A separate superuser-install leg would add +# nothing real -- superuser bypasses every check that +# role is subject to, so if the non-superuser installer +# can do it, a superuser certainly can too. # pg-upgrade-test -- BINARY pg_upgrade: install the current version on an # OLD major, binary-upgrade the cluster to a NEWER # major, then run the suite against the migrated # objects in "existing" mode (test/install/load.sql). # Proves objects created on an old server still work # read back on a new one -- fresh-install testing -# never exercises this at all. +# never exercises this at all. Runs as the ambient +# superuser throughout (binary pg_upgrade itself needs +# OS/cluster-level superuser access anyway), not the +# non-superuser installer -- test/install/load.sql only +# switches to it in the fresh/update branch. # # test_factory has shipped only one version (0.5.0), so there is no # in-place `ALTER EXTENSION UPDATE` path to test yet (no extension-update-test # job) and the pg_upgrade job needs no bridge step (it always installs the # CURRENT version on the old cluster -- there's no older, pg_upgrade-unsafe -# version in the wild to carry forward). Both jobs derive their PostgreSQL -# major list from the single source of truth computed in the `changes` job -# below, so adding/dropping a supported major is a one-line edit there. +# version in the wild to carry forward). The `test` job derives its +# PostgreSQL major list from the single source of truth computed in the +# `changes` job below, so adding/dropping a supported major is a one-line +# edit there for that job. `pg-upgrade-test`'s matrix is NOT derived from +# that same source -- its old_pg/new_pg pairs are hardcoded literals below, +# deliberately chosen to straddle the PostgreSQL 16 boundary without ever +# crossing it (see that job's own comment for why), so bumping NEWEST/FLOOR +# does not automatically update it. name: CI on: # Post-merge CI only; pull_request already covers every PR commit. Without @@ -190,13 +209,12 @@ jobs: # older, pg_upgrade-unsafe install to carry forward -- every leg installs # the current version directly on the old cluster. # - # Two legs, not an every-major stepwise climb (deliberately out of scope -- - # see the PR description): the oldest-to-newest jump (widest catalog - # distance) and the newest-boundary jump (most likely to hit a *new* - # PostgreSQL major's catalog change first). test_factory has no views or - # functions touching catalog internals (no SELECT * over a system catalog), - # so the per-major-boundary risk a full stepwise climb protects against is - # low here. + # Two legs, deliberately chosen to each stay entirely on one side of the + # PostgreSQL 16 boundary (10->15 and 16->18), never crossing PG16 itself: + # binary pg_upgrade doesn't re-run the install script, so + # https://github.com/Postgres-Extensions/test_factory/issues/14's + # `GRANT ... WITH SET TRUE` fix can't reach a role grant that predates PG16 + # (see README's "Known limitation" section). pg-upgrade-test: # Also needs `test`, not just `changes`: without this, a trivially-broken # PR (fails the cheap fresh-install matrix) still burns the full, @@ -207,8 +225,8 @@ jobs: matrix: include: - old_pg: "10" - new_pg: "18" - - old_pg: "17" + new_pg: "15" + - old_pg: "16" new_pg: "18" name: 🔄 Binary pg_upgrade ${{ matrix.old_pg }} → ${{ matrix.new_pg }} runs-on: ubuntu-latest diff --git a/README.md b/README.md index fafece8..60bb071 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,25 @@ so: PGOPTIONS=--search_path=extensions psql -d mydb -f test_factory.sql +Known limitation: SET-enabled membership in test_factory__owner +------------------------------------------------------------------ + +`CREATE EXTENSION test_factory` doesn't require a superuser -- a role with +CREATEROLE (to create `test_factory__owner`) and CREATE on the target +database (for the schemas the install script creates) is enough. + +Installing this way grants the installing role membership in +`test_factory__owner` -- SET-enabled on PostgreSQL 16+ (needed to `SET ROLE +test_factory__owner`), plain membership pre-16 -- but only if the installer +already has that membership, or has ADMIN OPTION on `test_factory__owner` to +grant it themselves. If neither is true, `CREATE EXTENSION` fails +immediately with an error naming exactly who needs to run what. + +If you ever do need to grant this membership manually (e.g. it was revoked, +or a role was set up some other way): + + GRANT test_factory__owner TO WITH SET TRUE; + Copyright and License --------------------- diff --git a/sql/test_factory--0.5.0--stable.sql b/sql/test_factory--0.5.0--stable.sql index e7e34b8..7ba91f7 100644 --- a/sql/test_factory--0.5.0--stable.sql +++ b/sql/test_factory--0.5.0--stable.sql @@ -1,15 +1,50 @@ /* - * Genuine no-op: nothing in this extension has changed since 0.5.0 yet. - * This file exists purely so ALTER EXTENSION test_factory UPDATE has an - * edge to follow at all -- Postgres's version-graph resolution requires - * an actual sql/test_factory----.sql file to exist for a - * transition, regardless of whether its content would be a no-op - * (confirmed directly: without this file, ALTER EXTENSION UPDATE fails - * outright with "has no update path from version 0.5.0 to version - * stable", even though nothing would actually need to change). See - * ../ai/RELEASE.md's `stable` pseudo-version workflow: every subsequent - * SQL-touching PR adds whatever ALTER .../CREATE OR REPLACE ... statements - * are needed here to bring an install on 0.5.0 up to that change. + * 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.sql b/sql/test_factory.sql index 605c70b..49b53f9 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -5,6 +5,7 @@ * 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; @@ -14,6 +15,86 @@ 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 -- unquoted; the "" above is the only quoting + 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 -- unquoted; the "" above is the only quoting + 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/CLAUDE.md b/test/CLAUDE.md index b536df9..55410e1 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -70,6 +70,28 @@ extensions installed, `pgtap` in schema `tap`), so `base.sql`/`pgtap.sql` and their expected output are shared across all of them -- no per-mode alternate expected files. +### Non-superuser installer (`test_factory_installer`) + +Connecting as a superuser (CI's default, and most developers' local setup) +bypasses every role-membership check involved in +https://github.com/Postgres-Extensions/test_factory/issues/14's fix +(`pg_has_role(current_user, 'test_factory__owner', ...)` is unconditionally +true for a superuser). Asserting that property from `base.sql` while +staying connected as a superuser would prove nothing about whether the fix +works -- it's true either way. + +Rather than running the whole suite a second time as a separate disposable +role (which would only prove what's already implied: a superuser can do +anything a properly-privileged non-superuser can), `load.sql`'s own +fresh/update branch creates a non-login `test_factory_installer` role +(`CREATEROLE` only -- the exact privilege floor `CREATE EXTENSION` needs, +paired with `CREATE` granted on the test database) and switches to it with +`SET SESSION AUTHORIZATION` before running `CREATE EXTENSION`/`ALTER +EXTENSION UPDATE`. No separate login, password, or CI job needed: the +switch only affects `load.sql`'s own already-authenticated session, and +every mode this suite runs in (superuser CI, a developer's local +superuser) ends up exercising the exact same non-superuser install path. + ### Dependency Guard Planted only in `existing` mode (see `load.sql`): a view in schema diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index b1d0fc9..f014617 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,6 +1,3 @@ \set ECHO none -psql:sql/test_factory.sql:49: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:50: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:88: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text -psql:sql/test_factory.sql:110: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text -psql:sql/test_factory.sql:116: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text +psql:sql/test_factory.sql:130: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:131: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION diff --git a/test/build/syntax.sql b/test/build/syntax.sql index 3c002b8..056cb9b 100644 --- a/test/build/syntax.sql +++ b/test/build/syntax.sql @@ -34,6 +34,17 @@ BEGIN; +/* + * CREATE EXTENSION already forces client_min_messages to at least WARNING + * for the duration of an install script, restoring the caller's setting + * once it finishes -- so a real CREATE EXTENSION never shows the %TYPE + * NOTICE below on its own. Running the script bare via \i here gets none + * of that, so this harness sets it itself, immediately before the \i, to + * match what a real install actually looks like. ERROR is unaffected, so + * the two expected pg_extension_config_dump() errors below still show. + */ +SET LOCAL client_min_messages = WARNING; + -- test_factory first: test_factory_pgtap's file needs its "tf" schema/role. \i sql/test_factory.sql \i sql/test_factory_pgtap.sql diff --git a/test/install/load.sql b/test/install/load.sql index 7ee0c8b..85c7e27 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -58,6 +58,69 @@ SELECT NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'test_role') AS need_ CREATE ROLE :test_role; \endif +/* + * Same issue as test_factory__owner (see sql/test_factory.sql's comment): + * CREATE ROLE alone never grants the creator any relationship to the role + * it just created, on ANY version -- confirmed directly, this session's + * connecting role gets "permission denied to set role" on the very role + * it just created above, with zero prior grant. test/helpers/create.sql's + * later SET ROLE = test_role needs SET-enabled membership on PG16+, or + * plain membership pre-16 (no WITH SET syntax there yet). Either branch + * skips the GRANT when it's not actually needed, since GRANT ROLE needs + * ADMIN OPTION to run at all, even for a no-op re-grant. + * + * Plain SQL + \if, not a DO block: :test_role is a psql variable, and psql + * never substitutes :variables inside a dollar-quoted body (confirmed + * directly -- it sends the literal text ":test_role" to the server, + * producing a syntax error there instead of anywhere client-side that + * would point at the real problem). GRANT ... TO CURRENT_USER sidesteps + * needing the *installer's* name dynamically at all -- CURRENT_USER is a + * real keyword, not a value needing substitution. + */ +SELECT (current_setting('server_version_num')::int >= 160000) AS test_role_pg16plus +\gset +\if :test_role_pg16plus +SELECT NOT pg_has_role(current_user, :'test_role', 'SET') AS test_role_needs_grant +\gset +\if :test_role_needs_grant +GRANT :test_role TO CURRENT_USER WITH SET TRUE; +\endif +\else +SELECT NOT pg_has_role(current_user, :'test_role', 'MEMBER') AS test_role_needs_grant +\gset +\if :test_role_needs_grant +GRANT :test_role TO CURRENT_USER; +\endif +\endif + +/* + * test_factory_installer (see test/roles.sql): the fresh/update branch + * below runs CREATE EXTENSION as this role, not as whatever ambient role + * connected (CI's superuser, a developer's own local superuser) -- if a + * non-superuser with the exact privilege floor CREATE EXTENSION needs + * (CREATEROLE, CREATE on this database) can install it, a superuser + * certainly can too, since superuser bypasses every check that role is + * deliberately still subject to. This is what actually exercises + * https://github.com/Postgres-Extensions/test_factory/issues/14's fix -- + * running the *same* suite twice, once as a superuser and once as a + * separate non-superuser role, would just waste CI time proving the + * (already-guaranteed) superuser case a second time. + * + * No LOGIN attribute: unlike test_role, nothing ever connects AS this role + * directly -- it's only ever reached via SET SESSION AUTHORIZATION within + * this same already-authenticated session, further below. That sidesteps + * needing a password or relying on whatever auth method (peer, trust, + * scram) the connecting user happens to have configured for it. + */ +SELECT NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'installer_role') AS need_installer +\gset +\if :need_installer +CREATE ROLE :installer_role CREATEROLE; +\endif + +SELECT format('GRANT CREATE ON DATABASE %I TO %I', current_database(), :'installer_role') +\gexec + /* * psql's \if only accepts a plain boolean token, not a comparison * expression -- compute it via SQL first (\if :load_mode = 'existing' would @@ -175,42 +238,40 @@ SELECT :'load_mode' = 'existing' AS is_existing * bootstrapping (CREATE ROLE test_factory__owner, guarded by WHEN * duplicate_object in sql/test_factory.sql) already tolerates being * re-run, so unlike pgxntool's own drop-first example there's no - * separate role-drop step needed here. + * separate role-drop step needed here. Runs as the ambient role, not + * test_factory_installer: that's plausibly how the extension got here + * in the first place (a superuser), which the installer's narrower + * privileges might not be able to drop. */ DROP EXTENSION IF EXISTS test_factory_pgtap CASCADE; DROP EXTENSION IF EXISTS test_factory CASCADE; - /* - * 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 @@ -230,11 +291,39 @@ SELECT :'load_mode' = 'existing' AS is_existing \else /* - * 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). + * 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 + + /* * 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* @@ -242,6 +331,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 diff --git a/test/roles.sql b/test/roles.sql index a48f56f..4ab7010 100644 --- a/test/roles.sql +++ b/test/roles.sql @@ -7,4 +7,7 @@ */ \set test_role test_role +-- test/install/load.sql only -- see its own comment. +\set installer_role test_factory_installer + -- vi: expandtab ts=2 sw=2 diff --git a/test/sql/base.sql b/test/sql/base.sql index f41978c..84db8d5 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -1,7 +1,10 @@ \set ECHO none \i test/helpers/setup.sql --- test/install/load.sql already installed the extension, in every mode. +/* + * test/install/load.sql already installed the extension, in every mode + * (including the regression test for issue #14 -- see its own comment). + */ -- NOTE: This runs some tests itself \i test/helpers/create.sql diff --git a/test_factory.control b/test_factory.control index 565e4b6..446c18a 100644 --- a/test_factory.control +++ b/test_factory.control @@ -1,3 +1,11 @@ comment = 'A framework for managing test data' 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 +# with search_path=pg_catalog specifically so this is safe to install +# without a real superuser (e.g. RDS/Aurora's non-superuser master user). +# Installing still requires CREATEROLE (for test_factory__owner) plus +# CREATE on the target database -- an ordinary role gets neither by +# default. +superuser = false diff --git a/test_factory_pgtap.control b/test_factory_pgtap.control index be2217c..f19f82f 100644 --- a/test_factory_pgtap.control +++ b/test_factory_pgtap.control @@ -2,3 +2,5 @@ comment = 'A framework for managing test data' default_version = 'stable' relocatable = false requires = 'pgtap, test_factory' +# See test_factory.control -- same rationale. +superuser = false