Give the test role schema rights only, never a database-level grant

CLAUDE.md

@@ -1,7 +1,8 @@
 # Claude Code Instructions for pg_count_nulls
 
-See [`../ai/CLAUDE.md`](../ai/CLAUDE.md) (or
-https://github.com/Postgres-Extensions/ai/blob/main/CLAUDE.md if that path
-doesn't exist locally — clone it to `../ai/` per its own first instruction)
-for CI-monitoring conventions, including handling of `claude-code-review`
-findings.
+@../ai/CLAUDE.md
+
+If `../ai/CLAUDE.md` doesn't exist locally (see
+https://github.com/Postgres-Extensions/ai), clone it now per its own
+first instruction, then re-read it — do not rely on the import above
+alone to have picked it up in that case.

HISTORY.md

@@ -3,11 +3,7 @@
 stable
 ------
 
-== CREATE EXTENSION no longer requires a superuser
-count_nulls is pure SQL functions, so the control file now says
-`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.
+- Extension no longer requires superuser to install
 
 1.0.0
 -----

README.md

@@ -52,9 +52,6 @@ running:
 
     CREATE EXTENSION count_nulls;
 
-No superuser required: count_nulls is nothing but SQL functions, so any role
-with `CREATE` on the target schema can add it.
-
 If you've upgraded your cluster to PostgreSQL 9.1 and already had count_nulls
 installed, you can upgrade it to a properly packaged extension with:
 

count_nulls.control

@@ -2,8 +2,9 @@
 comment = 'Count the number of null arguments'
 default_version = 'stable'
 relocatable = false
-# Pure SQL functions, nothing privileged - anyone with CREATE on the target
-# schema can install it. Enforced by the suite running as a non-superuser
+# Pure SQL functions, nothing privileged - anyone with USAGE and CREATE on the
+# target schema can install it, holding nothing on the database itself.
+# Enforced by the suite running as exactly such a role
 # (test/helpers/use_test_user.sql), which fails outright if this reverts to the
 # default.
 superuser = false

test/README.md

@@ -12,16 +12,17 @@ just defines it, leaving the caller to decide when to use it.
 - `install/load.sql` — installs count_nulls once, committed, before the
   `test/sql/` schedule, per `TEST_LOAD_SOURCE`. Its output isn't tracked; it
   fails loudly instead.
-- `deps.sql` — per-test-session setup; drops the session to the test user.
+- `deps.sql` — per-test-session setup; creates `_null_count_test` for the
+  `test__*` library to live in, then drops the session to the test user.
 - `core/functions.sql` — `ncs()`, plus the shared `test__*` library.
 - `sql/extension_tests.sql` — adds `test__check_ncs` and
   `test__shutdown__drop_all`, then calls `runtests()`.
 - `helpers/use_test_user.sql` — switches the session to the non-superuser
-  role.
+  role, granting it its rights on `:count_nulls_grant_schema` first.
 - `helpers/extension_installer.sql` — defines the functions that clean up
-  leftover test schemas and install count_nulls at a given version into a
-  fresh, randomly named one.
-- `helpers/create_test_schema.sql` — calls both, with `:version`; a file
+  leftover test schemas, produce a fresh, randomly named one, and install
+  count_nulls at a given version into it.
+- `helpers/create_test_schema.sql` — calls all three, with `:version`; a file
   only because `bin/test_existing`'s `prepare-old` runs it standalone.
 - `helpers/find_test_schema.sql` — finds that schema again, from a session
   that didn't create it.
@@ -36,6 +37,12 @@ count_nulls`). That's what makes `superuser = false` in
 `helpers/use_test_user.sql`, including why it deliberately does *not* switch
 when a real `pg_upgrade` has left the extension owned by someone else.
 
+That role holds **no privilege on the database** — every schema it works in is
+created by the connecting role, which grants it `USAGE` and `CREATE` on that
+one schema and nothing else. So a regression that made installing count_nulls
+depend on database-level rights fails the suite instead of passing on a
+privilege the test role happened to have.
+
 **count_nulls is installed into a randomly named schema**, never a fixed one
 and never the default. That's what gives `core/functions.sql`'s
 `%I`-qualified calls their meaning: the extension's schema can never

test/core/functions.sql

@@ -1,6 +1,4 @@
-CREATE SCHEMA _null_count_test;
-
--- See bottom as well!
+-- Schema created by test/deps.sql, as the connecting role. See bottom as well!
 SET SEARCH_PATH = _null_count_test, tap;
 
 CREATE FUNCTION ncs() RETURNS name IMMUTABLE LANGUAGE sql AS $$

test/deps.sql

@@ -11,4 +11,14 @@
 SELECT current_setting('count_nulls.test_load_mode') AS count_nulls_load_mode
 \gset
 
+/*
+ * Where the test__* functions and their fixtures live (test/core/functions.sql
+ * puts it on search_path). Created here, by the connecting role, so that the
+ * test user needs no privilege on the database - only what use_test_user.sql
+ * grants it below on this one schema. Rolled back with the rest of the
+ * session, like every other object a test/sql/ session makes.
+ */
+\set count_nulls_grant_schema _null_count_test
+CREATE SCHEMA :"count_nulls_grant_schema";
+
 \i test/helpers/use_test_user.sql

test/helpers/create_test_schema.sql

@@ -4,11 +4,12 @@
  * standalone (psql -v version=<VERSION> -f); the work itself lives in
  * test/helpers/extension_installer.sql.
  *
- * The install itself runs as a non-superuser (see
- * test/helpers/use_test_user.sql), which is what proves count_nulls doesn't
- * need superuser to install. Cleanup happens before that switch: a leftover
- * schema can belong to any role, and only the connecting one is sure to be
- * able to drop it.
+ * The install itself runs as a non-superuser holding nothing but a grant on
+ * the target schema (see test/helpers/use_test_user.sql), which is what
+ * proves count_nulls needs neither superuser nor any database-level
+ * privilege. Cleanup and the schema creation happen before that switch, as
+ * the connecting role: a leftover schema can belong to any role, and only
+ * the connecting one is sure to be able to drop it.
  */
 \i test/helpers/extension_installer.sql
 
@@ -18,6 +19,9 @@
  */
 SELECT pg_temp.count_nulls_cleanup_test_schemas('fresh');
 
+SELECT pg_temp.count_nulls_prepare_test_schema('fresh') AS count_nulls_grant_schema
+\gset
+
 /*
  * :count_nulls_load_mode must already be set by the caller
  * (bin/test_existing's -v on the command line - see the file header) -
@@ -26,7 +30,9 @@ SELECT pg_temp.count_nulls_cleanup_test_schemas('fresh');
  */
 \i test/helpers/use_test_user.sql
 
-SELECT pg_temp.count_nulls_install_extension(:'version') AS count_nulls_test_schema
+SELECT pg_temp.count_nulls_install_extension(
+    :'count_nulls_grant_schema', :'version'
+  ) AS count_nulls_installed
 \gset
 
 -- vi: expandtab sw=2 ts=2

test/helpers/extension_installer.sql

@@ -1,11 +1,13 @@
 /*
  * Defines, but does not call, the functions that find-and-drop leftover test
- * schemas and install count_nulls into a fresh one. Split from calling them,
- * and from each other, so test/install/load.sql can decide server-side
- * whether to install at all, and so every caller can run cleanup before
- * switching to the test user (test/helpers/use_test_user.sql) while
- * installing after - a leftover schema can belong to any role, and only the
- * connecting one is sure to be able to drop it.
+ * schemas, produce the schema count_nulls goes into, and install it there.
+ * Split from calling them, and from each other, so test/install/load.sql can
+ * decide server-side whether to install at all, and so every caller can run
+ * the first two as the connecting role and only the install as the test user
+ * (test/helpers/use_test_user.sql). Cleanup has to be the connecting role's
+ * because a leftover schema can belong to any role; the schema creation has
+ * to be, because the whole point is that the test user holds nothing but
+ * what it was granted on that one schema.
  */
 CREATE OR REPLACE FUNCTION pg_temp.count_nulls_cleanup_test_schemas(
   p_mode text
@@ -25,7 +27,7 @@ BEGIN
   /*
    * A run that died before its own teardown leaves a schema nothing else
    * knows the name of, so match the prefix (see
-   * count_nulls_install_extension()'s c_prefix below) and drop whatever's
+   * count_nulls_prepare_test_schema()'s c_prefix below) and drop whatever's
    * there.
    */
   FOR r IN SELECT nspname FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' LOOP
@@ -34,8 +36,16 @@ BEGIN
 END
 $body$;
 
-CREATE OR REPLACE FUNCTION pg_temp.count_nulls_install_extension(
-  p_version text
+/*
+ * Names the schema count_nulls is about to be installed into, creating it as
+ * the connecting role. 'existing' instead finds the schema a prior, separate
+ * prepare-old run installed into: that installation is the thing the mode
+ * exists to test, so creating a second schema here would both destroy the
+ * one-schema invariant test/helpers/find_test_schema.sql relies on and leave
+ * the real one untested.
+ */
+CREATE OR REPLACE FUNCTION pg_temp.count_nulls_prepare_test_schema(
+  p_mode text
 ) RETURNS name LANGUAGE plpgsql AS $body$
 DECLARE
   /*
@@ -46,6 +56,48 @@ DECLARE
    */
   c_prefix CONSTANT text := 'count_nulls test schema ';
   v_schema name;
+BEGIN
+  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;
+
+  IF p_mode = 'existing' THEN
+    SELECT nspname INTO v_schema
+      FROM pg_namespace n JOIN pg_extension x ON n.oid = x.extnamespace
+     WHERE extname = 'count_nulls'
+    ;
+
+    IF v_schema IS NULL THEN
+      RAISE EXCEPTION
+        'count_nulls is not installed, so mode ''%'' has no test schema to find'
+        , p_mode
+      ;
+    END IF;
+
+    RETURN v_schema;
+  END IF;
+
+  v_schema := c_prefix || substr(md5(random()::text), 1, 12);
+  EXECUTE format('CREATE SCHEMA %I', v_schema);
+
+  RETURN v_schema;
+END
+$body$;
+
+/*
+ * Runs as the test user, on a schema the connecting role made and granted it
+ * rights on (test/helpers/use_test_user.sql) - which is what makes a
+ * successful install prove count_nulls needs no privilege on the database
+ * itself, only on its target schema.
+ */
+CREATE OR REPLACE FUNCTION pg_temp.count_nulls_install_extension(
+  p_schema name
+  , p_version text
+) RETURNS void LANGUAGE plpgsql AS $body$
 BEGIN
   /*
    * 'current' means whatever the control file's default_version is, matching
@@ -56,9 +108,6 @@ BEGIN
     RAISE EXCEPTION $$p_version must be set explicitly, or 'current'$$;
   END IF;
 
-  v_schema := c_prefix || substr(md5(random()::text), 1, 12);
-  EXECUTE format('CREATE SCHEMA %I', v_schema);
-
   /*
    * WITH SCHEMA rather than arranging search_path first: this way a
    * successful install proves the install script doesn't depend on
@@ -67,11 +116,29 @@ BEGIN
    */
   EXECUTE format(
     'CREATE EXTENSION count_nulls WITH SCHEMA %I%s'
-    , v_schema
+    , p_schema
     , CASE WHEN p_version = 'current' THEN '' ELSE format(' VERSION %L', p_version) END
   );
 
-  RETURN v_schema;
+  /*
+   * CREATE EXTENSION runs the install script with search_path set to
+   * "<target>, pg_temp", and search_path silently drops a schema the role
+   * lacks USAGE on - so a role holding only CREATE builds the entire
+   * extension in pg_temp, where it disappears at disconnect, and CREATE
+   * EXTENSION still reports success. pg_extension.extnamespace says the
+   * target either way, so the only way to tell is to look for the objects.
+   */
+  IF NOT EXISTS(
+    SELECT 1
+      FROM pg_depend d
+      JOIN pg_proc p ON p.oid = d.objid AND d.classid = 'pg_proc'::regclass
+      JOIN pg_namespace n ON n.oid = p.pronamespace
+     WHERE d.deptype = 'e'
+       AND d.refobjid = (SELECT oid FROM pg_extension WHERE extname = 'count_nulls')
+       AND n.nspname = p_schema
+  ) THEN
+    RAISE EXCEPTION 'count_nulls installed no functions into schema %', p_schema;
+  END IF;
 END
 $body$;
 

test/helpers/use_test_user.sql

@@ -10,6 +10,9 @@
  * test/deps.sql (each test/sql/ session, via pgxntool's setup.sql),
  * test/install/load.sql (the install session) and
  * test/helpers/create_test_schema.sql (bin/test_existing's prepare-old).
+ * Each of them creates the schema its session works in first, as the
+ * connecting role, and names it in :count_nulls_grant_schema - the test user
+ * ends up with rights on that schema and nothing else.
  *
  * RESET ROLE first, so what this file does depends only on how the session
  * connected and not on anything an earlier \i of it already did - it needs
@@ -35,15 +38,17 @@ RESET ROLE;
  * p_load_mode must come from the caller, not from reading
  * count_nulls.test_load_mode in here: the Makefile only exports that GUC via
  * PGOPTIONS for pg_regress sessions, and bin/test_existing's prepare-old
- * invokes psql directly without it. Every includer sets the psql variable
- * count_nulls_load_mode before \i-ing this file.
+ * invokes psql directly without it. Every includer sets the psql variables
+ * count_nulls_load_mode and count_nulls_grant_schema before \i-ing this
+ * file.
  *
  * TODO: collapse this into a \gset + \if once 10 is the oldest version
  * supported - the plpgsql is only here to work around \if's absence.
  */
 CREATE OR REPLACE FUNCTION pg_temp.count_nulls_prepare_test_user(
   p_test_user name
   , p_load_mode text
+  , p_grant_schema name
 ) RETURNS name LANGUAGE plpgsql AS $body$
 DECLARE
   /*
@@ -118,15 +123,15 @@ BEGIN
     END IF;
 
     /*
-     * Can't fold into the CREATE ROLE above: the role is cluster-wide and
-     * outlives any one run, but this grant lives in the current database's
-     * ACL, so a run against a new database still has to issue it.
+     * The test user's entire footprint: rights on the one schema its caller
+     * already created for it, and nothing on the database. USAGE as well as
+     * CREATE because search_path skips a schema the role can't use at all.
      *
      * These two grants are all the suite gets. Anything else turning out to
      * be necessary is a finding about count_nulls, not something to grant.
      */
     EXECUTE format(
-      'GRANT CREATE ON DATABASE %I TO %I', current_database(), p_test_user
+      'GRANT USAGE, CREATE ON SCHEMA %I TO %I', p_grant_schema, p_test_user
     );
 
     /*
@@ -164,7 +169,11 @@ BEGIN
 END
 $body$;
 
-SELECT pg_temp.count_nulls_prepare_test_user(:'test_user', :'count_nulls_load_mode') AS count_nulls_run_as
+SELECT pg_temp.count_nulls_prepare_test_user(
+    :'test_user'
+    , :'count_nulls_load_mode'
+    , :'count_nulls_grant_schema'
+  ) AS count_nulls_run_as
 \gset
 
 SET ROLE :"count_nulls_run_as";