Give the test role schema rights only, never a database-level grant - #64
Open
jnasbyupgrade wants to merge 1 commit into
Open
Give the test role schema rights only, never a database-level grant#64jnasbyupgrade wants to merge 1 commit into
jnasbyupgrade wants to merge 1 commit into
Conversation
The docs say any role with rights on the target schema can install count_nulls, but the suite never tested that: use_test_user.sql granted the test role CREATE on the whole database, and the test role then created and owned every schema it used. A regression making the install depend on database-level CREATE would have passed unnoticed. The connecting role now creates every schema the harness needs and grants the test role USAGE and CREATE on that one schema, so the test role holds nothing on the database on any path. count_nulls_install_extension() is split from the schema creation, which moves to count_nulls_prepare_test_schema() on the connecting role's side of the switch; test/deps.sql likewise creates _null_count_test, which was the only remaining reason the test role needed database CREATE. Granting the target schema is what a caller must now supply, so the grant stays inside count_nulls_prepare_test_user(): that is where the role is guaranteed to exist, and a grant issued before it would fail on a cluster that has never run the suite. Installing with CREATE but not USAGE is silently useless rather than an error - CREATE EXTENSION puts "<target>, pg_temp" on search_path, search_path drops a schema the role cannot USAGE, and the whole extension is then built in pg_temp and lost at disconnect while CREATE EXTENSION still reports success. count_nulls_install_extension() now checks that the functions actually landed in the target schema, and the docs say USAGE and CREATE rather than CREATE alone. Co-Authored-By: Claude Opus 5 (1M context) <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: Team 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 |
|
Claude finished @jnasbyupgrade's task in 5m 37s —— View job Claude is reviewing this PR
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The docs claim any role with rights on the target schema can install count_nulls, but the suite never tested that:
use_test_user.sqlgranted the test roleCREATEon the whole database, and the test role then created and owned every schema it used, so a regression making the install depend on database-levelCREATEwould have passed unnoticed. The connecting role now creates every schema the harness needs — the extension's target schema and_null_count_test— and grants the test roleUSAGEandCREATEon that one schema, so the test role holds nothing on the database on any path.Granting only
CREATEturns out to be silently useless rather than an error:CREATE EXTENSIONputs"<target>", pg_temponsearch_path,search_pathskips a schema the role cannotUSAGE, and the extension is then built inpg_tempand lost at disconnect whileCREATE EXTENSIONstill reports success — socount_nulls_install_extension()now checks the functions actually landed in the target schema, and the docs sayUSAGEandCREATErather thanCREATEalone.