Give the test role schema rights only, never a database-level grant - #64
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: 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 |
|
Claude finished @jnasbyupgrade's task in 5m 37s —— View job Claude is reviewing this PR
|
| `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 |
There was a problem hiding this comment.
I missed this on the previous PR. This is waaay more detail than is warranted. Users don't care about the control file, and this being SQL functions has nothing to do with anything. This can literally just be - Extension no longer requires superuser to install
There was a problem hiding this comment.
Claude here — replaced the entry with exactly that line.
|
|
||
| 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 |
There was a problem hiding this comment.
WAY TOO MUCH DETAIL. In fact, I don't think it's even worth mentioning in the README that we don't require SU. Take the whole para out.
There was a problem hiding this comment.
Claude here — removed the whole paragraph.
…ed literal test/deps.sql declared count_nulls_grant_schema right after CREATE SCHEMA had already hardcoded the same name, so the two could silently drift apart. Setting the variable first and using :"count_nulls_grant_schema" in CREATE SCHEMA (as use_test_user.sql already does for the run-as role) makes the name single-sourced.
…tions Only 'existing' was special-cased; any other value - including a typo or an unpropagated GUC - fell through to creating a new schema unconditionally. count_nulls_load already rejects anything outside 'fresh'/'update'/'existing' with this same message; give the schema-naming function the identical check so a bad mode fails loudly there too instead of silently misbehaving.
Users installing the extension don't need the control-file mechanics or the search_path reasoning restated in two more places - drop the README paragraph entirely and cut the HISTORY entry down to the one-line fact.
A markdown link isn't Claude Code's @path import syntax, so nothing actually loaded ../ai/CLAUDE.md into context automatically - an agent had to notice and follow the pointer by hand, which is easy to skip. Switch to a real @../ai/CLAUDE.md import, keeping an explicit fallback note for the case where that file isn't cloned locally yet, since it's unclear whether a missing @-imported file fails loudly or is silently skipped.
Claude is reviewing this PR
|
…AISE Quote the bad value with %L instead of a bare %, assign ERRCODE invalid_parameter_value instead of leaving it generic, and phrase the primary message as a fixed input-domain violation (no trailing period, valid values moved to HINT) - matching real Postgres GUC-validation messages like "invalid value for parameter \"%s\": \"%s\"".
|
Claude finished @jnasbyupgrade's task in 3m 1s —— View job Claude finished the code review
SummaryReviewed the full diff (
One minor, non-blocking nit posted inline:
No correctness issues found beyond that — the privilege model (grant |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| IF p_mode NOT IN ('fresh', 'update', 'existing') THEN | ||
| RAISE EXCEPTION USING | ||
| MESSAGE = format('invalid value for parameter "count_nulls.test_load_mode": %L', p_mode) | ||
| , HINT = 'Valid values are ''fresh'', ''update'', and ''existing''.' | ||
| , ERRCODE = 'invalid_parameter_value' | ||
| ; | ||
| END IF; |
There was a problem hiding this comment.
Minor consistency nit: this validates the same value (count_nulls.test_load_mode, passed through as p_mode) that count_nulls_load in test/install/load.sql:72-77 also validates, but with a different error shape — RAISE EXCEPTION USING MESSAGE/HINT/ERRCODE here vs. a plain RAISE EXCEPTION '...got %' there. Both are reachable from the same test/install/load.sql run (this one via count_nulls_prepare_test_schema, called before count_nulls_load), so an invalid mode raises a differently-styled error depending on exactly where the validation happens to fire first. Worth either applying the same Postgres-error-style-guide shape to load.sql's check too, or a short comment noting the old one is intentionally left alone — not blocking, just flagging so it's a choice rather than an oversight.
There was a problem hiding this comment.
Claude here — correct, and already discussed on this PR: load.sql's check pre-dates this PR (see the reply on 3961370056) and is deliberately out of scope here per the maintainer - not brought in line with the new style in this PR.
1 participant
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.