Follow-on to #18: run application-level tests as a genuine non-superuser - #45
jnasbyupgrade wants to merge 4 commits into
Conversation
…low-on) test/helpers/create.sql now uses SET SESSION AUTHORIZATION instead of SET ROLE to switch into test_role. SET ROLE only changes current_user; a further SET ROLE's own permission check (like the one test_factory's install performs, and like issue #14's bug) is based on session_user, which SET ROLE leaves untouched. Under pg_regress's superuser connection, that means SET ROLE alone silently leaves this whole class of check bypassed for the rest of the file -- SET SESSION AUTHORIZATION actually drops it. New test/sql/security.sql proves the public tf.* API needs nothing beyond what a freshly-created, unprivileged role gets by default (no owned schema, no explicit grants, not a member of test_factory__owner): register/get work end to end, and the role still can't SET ROLE into test_factory__owner. The non-superuser CREATE EXTENSION repro this PR originally added separately (a disposable test_factory_installer role in test/sql/install.sql) is dropped: test/install/load.sql's fresh/update branch now does exactly this, as the real install for the whole suite, making that separate repro redundant. test/sql/install.sql itself was already deleted upstream for the same reason. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Matches test_role/installer_role's existing pattern in test/roles.sql: a role name that other files might need should be defined there once, not inlined, so a rename only touches one place. Also converts the resulting/existing 2+-line -- comment stacks in roles.sql and security.sql to /* */, per the comment-stacked-dashes lint rule tightened in the linter submodule after this branch's original commits were written (2026-08-12, well after this branch's last real CI run on 2026-07-30) -- not a rule violation introduced by this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jnasbyupgrade
changed the title
Hand-copied from test/results/ rather than via `make results`: that target's verify-results-pgtap.sh safeguard can't accept a brand-new test file's first baseline (any diff from no-baseline-yet trips its regression.diffs fallback check, even with zero real `not ok` lines) -- filed as Postgres-Extensions/pgxntool#119. Content verified directly: 4/4 real pgtap assertions pass, nothing else in the file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| \i test/helpers/setup.sql | ||
|
|
||
| /* | ||
| * Prove the public tf.* API needs nothing beyond what a freshly-created, |
There was a problem hiding this comment.
Why is this a separate test? Can't we just have all the existing tests ensure they're using a bare role?
There was a problem hiding this comment.
Good catch, you're right — checked it concretely: test_role owns nothing test_factory-related (no tf/_tf beyond what PUBLIC gets, and deliberately no test_factory__owner membership), so base.sql/pgtap.sql already proved the positive claim this file existed for. The only real gap was that nothing actually asserted the negative claim the comment next to the GRANT only stated as intent.
Fixed in 14c070c: added isnt_member_of('test_factory__owner', :test_role, ...) right at that comment in create.sql (pgtap's purpose-built membership check, cleaner than throws_ok on a live SET ROLE and version-independent), and deleted security.sql/bare_role entirely as redundant.
test_role owns nothing test_factory-related (no tf/_tf ownership beyond what PUBLIC already gets, and -- per create.sql's own comment -- deliberately no test_factory__owner membership), so base.sql/pgtap.sql running under test_role already proved the positive claim security.sql existed for (register/get works with nothing beyond default + tap grants). The only gap was that nothing actually asserted the negative claim the comment only stated as intent. Closed with pgtap's isnt_member_of(), which checks the pg_auth_members row directly -- simpler than throws_ok on a live SET ROLE, and version-independent (no PG16 SET-option branching needed). security.sql, its expected output, and the now-unused bare_role are removed as redundant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Follow-on to #18: run more of the application-level test suite as a genuine non-superuser.
(Replaces #30, which developed a stuck head/diff computation on GitHub's side that survived a
force-push, a base-branch retarget, and a close/reopen — confirmed by opening this PR against the
exact same commits, which computed correctly immediately. Same content, same history.)
Background
While updating #18, the question came up: could we run more of this test suite as a non-superuser, given #18 was exactly the kind of bug (
SET ROLEpermission check) that a superuser-run test suite can't naturally catch?test/helpers/create.sqlswitches into a non-superusertest_rolefor most ofbase.sql/pgtap.sql-- but viaSET ROLE, notSET SESSION AUTHORIZATION. That distinction matters:SET ROLEonly changescurrent_user; Postgres's own permission check for a furtherSET ROLE(the exact class of check #18's bug was in) is based onsession_user, whichSET ROLEleaves untouched. Under pg_regress's superuser connection, that silently bypassed this whole class of check for the rest of the file. Fixed by switching toSET SESSION AUTHORIZATION. Also addedtest/sql/security.sql, proving the publictf.*API needs nothing beyond a freshly-created, unprivileged role -- using a newbare_roleentry intest/roles.sql(this repo's single-source-of-truth pattern for test-only role names) rather than a literal role name.Scope note: an earlier version of this PR also added a separate non-superuser
CREATE EXTENSIONrepro (a disposable installer role intest/sql/install.sql). That's dropped --test/install/load.sql's fresh/update branch (landed as part of #18's own review iterations, now onmaster) already does exactly this as the real install for the whole suite, making the separate repro redundant;test/sql/install.sqlitself was deleted upstream for the same reason. This PR is based on currentmasterand contains only the still-missing piece.Test plan
make test--load/base/pgtappass;securityruns correctly (4/4 pgtap assertions pass) but needsmake resultsto accept its expected output, per this repo's human-verifies-expected-output conventionmake test-update-- same resultmake lint-- cleantest/expected/base.out/pgtap.outunchanged🤖 Generated with Claude Code