From a23ff5df2deb24feffbdcbb79235afb6905237c9 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 12 Sep 2026 21:23:53 -0600 Subject: [PATCH] test: attribute a refusal by SQLSTATE, and fix the tool that grades it (#432) Two changes, and the second is why the first could be measured. THE PORT. projection_privilege.sh decided four things by matching error text, and two of them were load-bearing. Its own comment says why: both ACL layers raise 42501, so a bare "refused" stays true if the SQL REVOKE is deleted and the C check catches it instead -- measured there, the suite still passed 14/14 with the REVOKE removed. What separates the layers is the fixture, not the wording. Called with a projection name that does not exist, on a table the role may read: stopped by the SQL grant 42501 the body never ran to notice the name past the SQL grant 42704 it ran, and the projection lookup raised So the refusal is attributed by what the code REACHED, which no rephrasing can move. RLS is a third code again: 0A000, a different SQLSTATE class from either ACL refusal. Both harnesses do this now; the shell half reads the state through psql -v VERBOSITY=sqlstate. That also gave two orderings the source asserts and neither harness tested: the base ACL is checked before the projection is looked up, so a caller with no SELECT cannot learn whether a projection exists on a table it may not read; and the ACL is checked before RLS, so a caller with no privilege is not told the table has row-level security enabled. The second is a correction src/columnar_vacuum.c records being made in review. Five mutations, each asserted to apply at both call sites before the run. Moving the ACL below the projection lookup but ABOVE its raise reddens nothing, correctly -- 42501 still wins, which is what says the ordering arms measure an ordering rather than the presence of a check. Moving it below the raise reddens both, and the shell half prints the disclosure: got [42704] want [42501]. THE TOOL. compare_to_bash.py decides whether a port is one-for-one, which is this issue's definition of done, and it was reading the wrong argument: expect.num(got, 1, NAME) -> NAME correct, so it looked right expect.sqlstate(err, "42501", NAME) -> "42501" expect.text(got, "none", NAME) -> "none" `[^)]*?` is lazy and stopped at the first quoted argument. Every SQLSTATE assertion was read as the literal 42501, counted as an "extra" the bash suite lacks, and the real property reported MISSING. This issue's ports are exactly the ones replacing a grep with a SQLSTATE assertion, so the tool went blind in proportion to the work being done well. It parses with ast now: the last string argument, with f-strings as templates, both arms of a conditional, and the `name` column of a parametrize. Bash interpolations reduce the same way, including $1 -- the commonest one in a check name, which the first version of the reducer missed because its pattern required a letter after the dollar. Measured over every pair in the tree: 61 bash properties reported missing, now 0. Of the 61, 34 were never missing. The rest were real and are closed here: stats_privilege had invented a name for a property the bash suite already named, and zonemap_boundaries was missing its `backend alive` premise outright. Neither was visible while the tool reported the wrong string. Verified: guard half 295 (729 checks, 0 unrun), cluster half 217 (614 checks, 0 unrun), projection_privilege.sh 24/24, zonemap_boundaries.sh 9/9, docs_style.sh PASSED, and all seven pairs grade rc=0. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw --- CHANGELOG.md | 53 +++ test/projection_privilege.sh | 83 ++++- test/pytest/TESTS.md | 149 +++++++++ test/pytest/compare_to_bash.py | 254 ++++++++++++--- test/pytest/expected_tests.txt | 9 +- test/pytest/test_compare_to_bash.py | 195 +++++++++++ test/pytest/test_harness_deps.py | 3 + test/pytest/test_projection_privilege.py | 394 +++++++++++++++++++++++ test/pytest/test_stats_privilege.py | 12 +- test/pytest/test_zonemap_boundaries.py | 13 +- 10 files changed, 1114 insertions(+), 51 deletions(-) create mode 100644 test/pytest/test_compare_to_bash.py create mode 100644 test/pytest/test_projection_privilege.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 24999969..ddfb1cd7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,59 @@ true until the next version shipped. ### Added +- `projection_privilege.sh` has a pytest twin, and both halves now attribute a refusal + by SQLSTATE instead of by error text (#432, #562, #563). + + The bash suite decided four things by matching the message, and two of them were + load-bearing rather than decorative. Its own comment says why: both ACL layers raise + 42501, so a bare "refused" stays true if the SQL `REVOKE` is deleted and the C check + catches it instead. Measured there: with the `REVOKE` removed the suite still passed + 14 of 14. + + The fixture is what separates them, not the wording. Called with a projection name + that does not exist, on a table the role may read, a caller stopped by the SQL grant + never runs the body and gets 42501; one that gets past the grant reaches the lookup + and gets 42704. The refusal is attributed by what the code REACHED. RLS is a third + code again, `0A000` from `ERRCODE_FEATURE_NOT_SUPPORTED`, which is a different + SQLSTATE class from either ACL refusal. + + Two orderings that `src/columnar_projection.c` and `src/columnar_vacuum.c` assert had + no test in either harness: the base ACL is checked before the projection is looked + up, so a caller with no SELECT cannot learn whether a projection exists on a table it + may not read; and the ACL is checked before RLS, so a caller with no privilege is not + told the table has row-level security enabled. The second is a correction the source + records being made in review. + + Five mutations, each asserted to apply at both call sites. Moving the ACL check below + the projection lookup but above its raise reddens nothing, correctly -- 42501 still + wins. Moving it below the raise reddens both ordering arms, and the shell half prints + the disclosure: `got [42704] want [42501]`. + +- `compare_to_bash.py` reads the assertion's NAME (#432, #897). + + The parity tool decides whether a port is one-for-one with its bash suite, which is + #432's definition of done, and it was reading the wrong argument. The python side was + matched with `expect\.\w+\([^)]*?"([^"]+)"`, whose lazy `[^)]*?` stops at the FIRST + quoted argument. For `expect.num(got, 1, NAME)` that is the name, so the tool looked + correct. For `expect.sqlstate(err, "42501", NAME)` it is `42501`. + + Every SQLSTATE assertion was therefore read as the literal `42501`, reported as an + "extra" name the bash suite does not have, while the real property was reported + MISSING. #432's ports are exactly the ones replacing a grep on a message with a + SQLSTATE assertion, so the tool went blind in proportion to the work being done well. + + It parses with `ast` now and takes the last string argument, resolving f-strings to + templates, both arms of a conditional, and the `name` column of a + `@pytest.mark.parametrize`. Bash interpolations reduce to the same template, including + `$1`, which is the commonest one in a check name and which the first version of the + reducer missed because its pattern required a letter after the dollar. + + Measured over every pair in the tree: **61 bash properties reported missing, now 0.** + 34 were never missing. The rest were real and are closed here: `stats_privilege` had + invented a name for a property the bash suite already named, and `zonemap_boundaries` + was missing its `backend alive` premise outright. Neither was visible while the tool + was reporting the wrong string. + - A UNIQUE-constraint check passed on any psql failure, and a recursive sweep passed on a tree it never read (#1033). diff --git a/test/projection_privilege.sh b/test/projection_privilege.sh index 6669f70a..63b7defc 100755 --- a/test/projection_privilege.sh +++ b/test/projection_privilege.sh @@ -67,6 +67,15 @@ for r in t_prjexec t_prjsel; do done psql_run "GRANT SELECT ON secret TO t_prjsel;" +# A table EVERY role may read. It carries no projection, which is what lets the +# arms below attribute a refusal to a LAYER without reading the message: a caller +# stopped by the SQL grant never reaches the projection lookup, and one that gets +# past the grant does. See test/pytest/test_projection_privilege.py, which decides +# the same two questions the same way in a different language. +psql_run "CREATE TABLE prjopen (id int) USING pgcolumnar;" +psql_run "INSERT INTO prjopen SELECT generate_series(1,10);" +psql_run "GRANT SELECT ON prjopen TO t_prjnone, t_prjexec, t_prjsel;" + as() { # as env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U "$1" \ -d "$PGC_DB" -At -c "$2" 2>&1 @@ -82,6 +91,22 @@ count_as() { # count_as esac } +# state_as -> the SQLSTATE, or the literal noerror. +# +# VERBOSITY is set with -v, NOT with -c "\\set ...". psql treats a -c argument +# beginning with a backslash as a meta-command and takes a different code path, +# which is how a sibling suite ended up with deny arms that could never go green. +# The SQLSTATE is read from the ERROR line because psql prefixes it, and it is +# extracted into a variable rather than tested through a pipeline whose STATUS +# would be the answer. +state_as() { # state_as + local out _sqlstate + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U "$1" \ + -d "$PGC_DB" -At -v VERBOSITY=sqlstate -v ON_ERROR_STOP=0 -c "$2" 2>&1)" + _sqlstate="$(sed -n 's/^.*ERROR:[[:space:]]*\([0-9A-Z]\{5\}\).*$/\1/p' <<<"$out" | head -1)" + if [ -n "$_sqlstate" ]; then printf '%s\n' "$_sqlstate"; else echo noerror; fi +} + # ---- premises: the fixture, and that the functions WORK ---------------------- # # The owner arms are not decoration. A DENIED result and a BROKEN result are @@ -125,11 +150,19 @@ check "and is refused reconstruct_via_projection" \ # WHICH layer refused, not merely that one did. Both layers reject this role, so # a bare "refused" stays true if the REVOKE is deleted and the C check catches it # instead -- measured: with the REVOKE removed this suite still passed 14 of 14. -# The error text is the only thing that attributes the refusal, so it is asserted -# here to tell the layers apart rather than in place of behaviour. -check "and the refusal comes from the SQL grant, naming the function" \ - "$(as t_prjnone "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');" | grep -c 'permission denied for function')" \ - "1" +# +# THE ERROR TEXT USED TO BE THE ONLY THING THAT COULD ATTRIBUTE IT, because both +# layers raise 42501. It is not, once the call names a table every role may read +# and a projection that does not exist: a caller stopped by the SQL grant never +# runs the body, so it never reaches the lookup, and one that gets past the grant +# does. The two outcomes are then different SQLSTATEs and the wording is free to +# change without moving the arm. +check "the no-EXECUTE role never reaches the body, so the grant is what stopped it" \ + "$(state_as t_prjnone "SELECT count(*) FROM pgcolumnar.read_projection('prjopen','no_such_proj');")" \ + "42501" +check "while the EXECUTE role reaches the projection lookup on the same call" \ + "$(state_as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('prjopen','no_such_proj');")" \ + "42704" # ---- layer two: the C check, reached only because EXECUTE was granted -------- # @@ -150,9 +183,20 @@ check "a role with EXECUTE but no SELECT is refused read_projection" \ "$(count_as t_prjexec read_projection)" "refused" check "and is refused reconstruct_via_projection, which leaks non-covered columns" \ "$(count_as t_prjexec reconstruct_via_projection)" "refused" -check "and THAT refusal names the table, so it is the C check and not the grant" \ - "$(as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');" | grep -c 'permission denied for table')" \ - "1" +check "and THAT refusal is the C check, which raises 42501 from aclcheck_error" \ + "$(state_as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');")" \ + "42501" + +# AN ORDERING THE SOURCE ASSERTS AND NOTHING TESTED. The ACL check is the first +# statement of the body; the projection lookup is well below it. Swapped, a caller +# with no SELECT would learn whether a named projection exists on a table it may +# not read -- existence disclosure, from a function whose purpose is to stop +# disclosure. The arm above is the control: the same role, the same bogus name, on +# a table it MAY read, returns 42704, so 42501 here is the ACL check winning rather +# than the lookup being unreachable. +check "the base ACL is checked before the projection name is looked up" \ + "$(state_as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('secret','no_such_proj');")" \ + "42501" # ---- the bar is SELECT, not ownership --------------------------------------- # @@ -185,8 +229,25 @@ check "read_projection now refuses a policy-restricted caller (#563)" \ "$(count_as t_prjsel read_projection)" "refused" check "and so does reconstruct_via_projection" \ "$(count_as t_prjsel reconstruct_via_projection)" "refused" -check "and the refusal names row-level security, not the table ACL" \ - "$(as t_prjsel "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');" | grep -c '^ERROR:.*row-level security')" \ - "1" +# 0A000, not a phrase. PgColumnarRequireNoRowSecurity raises +# ERRCODE_FEATURE_NOT_SUPPORTED, which is a different SQLSTATE CLASS from either +# ACL refusal -- 0A against 42 -- so no rewording of any message can confuse them. +check "and the refusal says the feature is not supported, not that a privilege is missing" \ + "$(state_as t_prjsel "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');")" \ + "0A000" + +# THE SECOND ORDERING, recorded in src/columnar_vacuum.c as a correction made in +# review. RLS is checked AFTER the relation ACL. With it first, a caller holding +# no privilege at all was told the table has RLS enabled, which ordinary SQL does +# not disclose and which disagrees with core. Measured there: +# +# RLS on, no-select, ordinary SQL -> permission denied for table +# RLS on, no-select, read_projection -> row-level security is in force +# +# The arm above is its control: same table, same policy, a role that DOES hold +# SELECT, and 0A000. So 42501 here is the ordering, not RLS being switched off. +check "a caller with no SELECT is told a privilege is missing, not that RLS is in force" \ + "$(state_as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');")" \ + "42501" pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index fe04c582..8da7850b 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -79,6 +79,8 @@ behaviour, the source of that number is named. - [31. test_native_ownership.py: every maintenance function is owner-only](#31-test_native_ownershippy-every-maintenance-function-is-owner-only) - [32. test_stats_privilege.py: stats is readable only by a caller who may read the table](#32-test_stats_privilegepy-stats-is-readable-only-by-a-caller-who-may-read-the-table) - [33. test_docs_table_structure.py: a table must stay a table](#33-test_docs_table_structurepy-a-table-must-stay-a-table) +- [34. test_projection_privilege.py: the projection read helpers are a privilege boundary](#34-test_projection_privilegepy-the-projection-read-helpers-are-a-privilege-boundary) +- [35. test_compare_to_bash.py: the parity tool reads the NAME](#35-test_compare_to_bashpy-the-parity-tool-reads-the-name) ## 1. How to read a test in here @@ -3203,3 +3205,150 @@ report that cannot say where is one somebody has to re-derive. **Measured before landing**, which is what a static guard in this tree owes. 0 across the gate's own scope, and 5 elsewhere in the tree. All five are real: 3 in this file and 2 in a design document, none of which the gate covers. + +## 34. test_projection_privilege.py: the projection read helpers are a privilege boundary + +`read_projection()` and `reconstruct_via_projection()` opened a caller-supplied regclass +and returned its contents with no privilege check, and `CREATE FUNCTION` grants EXECUTE to +PUBLIC. `reconstruct` rebuilds NON-COVERED columns from the base by row number, so the +projection was never the bound on what leaked: one projection on any column exposed the +whole row (#562). A caller holding SELECT but restricted by an RLS policy received every +row (#563). + +### Four refusals, four SQLSTATEs, and why the bash suite needed the message + +`projection_privilege.sh` decided four things by matching error text, and two of them were +load-bearing rather than decorative. Its own comment says why: + +> Both layers reject this role, so a bare "refused" stays true if the REVOKE is deleted and +> the C check catches it instead -- measured: with the REVOKE removed this suite still +> passed 14 of 14. + +Both ACL layers raise 42501, so the code alone does not separate them. What separates them +is the fixture: + +| the caller | the call | outcome | +| --- | --- | --- | +| no EXECUTE | a projection that does not exist, on a table it MAY read | `42501` -- the body never ran | +| EXECUTE | the same call | `42704` -- it ran and reached the lookup | +| EXECUTE, no SELECT | a real projection on a table it may NOT read | `42501` -- the base ACL | +| SELECT, under a policy | the same | `0A000` -- `ERRCODE_FEATURE_NOT_SUPPORTED` | + +The refusal is attributed by what the code REACHED. That is a fact about execution, and no +rephrasing of either message can move it. Both harnesses now do this: the shell half reads +the SQLSTATE through `psql -v VERBOSITY=sqlstate`. + +### Two orderings the source asserts and nothing tested + +| arm | what breaks without it | +| --- | --- | +| `test_the_base_acl_is_checked_before_the_projection_is_looked_up` | a caller with no SELECT learns whether a named projection exists on a table it may not read | +| `test_the_acl_is_checked_before_rls_so_no_privilege_discloses_no_rls_state` | a caller with no privilege is told the table has RLS enabled, which ordinary SQL does not disclose | + +### Every arm + +| test | what it holds | +| --- | --- | +| `test_the_premises_the_fixture_is_what_the_suite_assumes` | each role opens its own session, the owner gets rows from both helpers, and reconstruct really does return the non-covered column | +| `test_a_role_with_only_schema_usage_is_refused` | layer one, the SQL grant, per function | +| `test_a_role_with_execute_but_no_select_is_refused` | layer two, the C check, reached only because EXECUTE was granted | +| `test_which_layer_refused_without_reading_the_message` | which of the two, decided by what the code reached rather than by wording | +| `test_the_base_acl_is_checked_before_the_projection_is_looked_up` | the first ordering | +| `test_a_role_with_select_still_reads` | THE CONTROL: a bar that refused everyone is not a fix | +| `test_a_policy_restricted_caller_is_refused` | RLS, as `0A000` rather than a phrase | +| `test_the_acl_is_checked_before_rls_so_no_privilege_discloses_no_rls_state` | the second ordering | + +The second is a correction `src/columnar_vacuum.c` records being made in review. Neither +had a test in either harness. + +### Removal proof + +Five mutations, each asserted to apply at both call sites before the run: + +| mutation | pytest | shell | +| --- | --- | --- | +| drop the base ACL check | 4 arms red | -- | +| drop the RLS refusal | 2 arms red | -- | +| RLS **before** the ACL check | the RLS-ordering arm alone | the RLS-ordering arm alone | +| ACL below the projection lookup but above its raise | no arm red, correctly: 42501 still wins | same | +| ACL below the not-found **raise** | both ordering arms | both, reporting `got [42704] want [42501]` | + +The fourth row is the one that says the ordering arms measure an ordering rather than the +presence of a check. The fifth is the disclosure itself, printed. + +## 35. test_compare_to_bash.py: the parity tool reads the NAME + +`compare_to_bash.py` decides whether a port is one-for-one with its bash suite, which is +#432's definition of done. It was reading the wrong argument. + +The python side was matched with `expect\.\w+\([^)]*?"([^"]+)"...`, and `[^)]*?` is lazy, +so it stopped at the FIRST quoted argument: + +| call | name read | +| --- | --- | +| `expect.num(got, 1, NAME)` | `NAME` -- correct, which is why it looked right | +| `expect.sqlstate(err, "42501", NAME)` | `"42501"` | +| `expect.text(got, "none", NAME)` | `"none"` | + +So every SQLSTATE assertion was read as the literal `42501`, reported as an "extra" the +bash suite lacks, while the real property was reported MISSING. #432's ports are exactly +the ones replacing a grep on a message with a SQLSTATE assertion, so **the tool went blind +in proportion to the work being done well.** + +### Measured over the tree + +| pair | missing before | after | +| --- | --- | --- | +| differential | 6 | 0 | +| hilbert_locality | 13 | 0 | +| native_ownership | 1 | 0 | +| native_projection | 0 | 0 | +| projection_privilege | 23 | 0 | +| stats_privilege | 9 | 0 | +| zonemap_boundaries | 9 | 0 | +| **total** | **61** | **0** | + +Of the 61, 34 were never missing. The rest were real, and four of them are closed here: +`stats_privilege` had invented a name for a property the bash suite already named, and +`zonemap_boundaries` was missing its liveness premise outright. Neither was visible while +the tool was reporting the wrong string. + +### What the parser reads + +| shape | read as | +| --- | --- | +| the last string argument | the name | +| an f-string | a `{}` template, matched against bash interpolations reduced the same way | +| `"a" if cond else "b"` | both arms | +| `@pytest.mark.parametrize("func,name", ROWS)` | the `name` column, resolved through module constants | +| anything else | nothing -- absent is better than wrong | + +`$1` is the commonest interpolation in a bash check name and the first version of the +template reducer missed every one of them, because its pattern required `[A-Za-z_]` after +the dollar. + +### Removal proof + +| mutation | red | +| --- | --- | +| take the first string argument, as the regex did | the regression arm, the unreadable-name arm, and the whole-tree arm | +| drop the conditional-name case | its own arm, and the whole-tree arm | +| drop parametrize resolution | its own arm, and the whole-tree arm | +| read the name column by position instead of by its declared name | its own arm, and the whole-tree arm | + +`test_the_ported_suites_in_this_tree_are_graded_one_for_one` catches all four. It is the +arm that matters: a guard over invented sources proves the extractor reads python, not that +the tool grades THIS tree. + +### Every arm + +| test | what it holds | +| --- | --- | +| `test_the_name_is_the_last_argument_not_the_first_string` | the regression, over three helpers, one of which always worked | +| `test_a_call_whose_name_is_not_a_literal_contributes_nothing` | absent beats wrong: a false green on a parity tool loses a property in both harnesses | +| `test_an_fstring_name_becomes_a_template` | a runtime-built name is compared by shape | +| `test_a_conditional_name_carries_both_of_its_arms` | `"a" if c else "b"` states two properties | +| `test_a_parametrized_name_is_resolved_from_the_decorator` | the idiom a repeated bash property should be ported to, with a no-`name` decorator as the control | +| `test_the_parametrize_reader_takes_the_column_called_name` | the declared column, not position | +| `test_the_two_harnesses_interpolations_land_on_one_template` | bash and python spell interpolation differently and must meet | +| `test_the_ported_suites_in_this_tree_are_graded_one_for_one` | the standing arm: every pair in the tree, graded | diff --git a/test/pytest/compare_to_bash.py b/test/pytest/compare_to_bash.py index 875b9552..f7f03178 100755 --- a/test/pytest/compare_to_bash.py +++ b/test/pytest/compare_to_bash.py @@ -10,43 +10,225 @@ The port makes this possible by passing each assertion the SAME name string the bash check uses. That is a convention the port must keep, so this script is also what enforces it. + +THE PYTHON SIDE IS PARSED, NOT MATCHED (#432, #897) +--------------------------------------------------- + +This read the pytest file with a regex and got the wrong argument: + + expect.sqlstate(err, "42501", "a role with no privilege is refused") + ^^^^^^^ reported as the assertion's name + +`[^)]*?"([^"]+)"` is lazy, so it stops at the FIRST quoted argument. For +`expect.num(got, 1, "name")` that happens to be the name and the tool looked +correct. For any helper whose WANT is itself a string it is not: + + expect.sqlstate(err, "42501", NAME) -> "42501" + expect.text(got, "none", NAME) -> "none" + +So every SQLSTATE assertion was read as the literal `42501`, counted as an +"extra" name the bash suite does not have, and the real property was reported +MISSING. That is a false red aimed squarely at the ports #432 exists to produce, +which are the ones replacing a `grep` on an error message with a SQLSTATE. The +tool got blinder as the work it grades got better. + +It is parsed with `ast` now, and the name is the LAST string argument of the +call, which is the convention every port already follows. + +INTERPOLATED NAMES ARE MATCHED AS TEMPLATES +------------------------------------------- + +Both harnesses build some names at runtime, bash as `non-owner refused: ${1%%(*}` +and pytest as an f-string. Neither can be expanded without running the suite, so +this compares the SHAPE: every interpolation on both sides becomes `{}`, and two +names match when their templates do. + +That is a weaker claim than a literal match and it is reported separately rather +than folded in, because a template match says the two harnesses assert a property +of the same shape, not that they assert it over the same values. A port should +still prefer literal names. + +Exit status is 1 when a bash property has no counterpart of either kind. """ +import ast import re import sys -bash_file, py_file = sys.argv[1], sys.argv[2] - -# bash: check "NAME" ... / check_num "NAME" ... / check_ratio "NAME" ... -bash_src = open(bash_file).read() -bash_names = re.findall(r'\bcheck(?:_num|_ratio|_text|_timing)?\s+"([^"]+)"', bash_src) - -# pytest: expect.(..., "NAME") and name="NAME" -py_src = open(py_file).read() -py_names = re.findall(r'expect\.\w+\([^)]*?"([^"]+)"\s*(?:,[^)]*)?\)', py_src, re.S) -py_names += re.findall(r'name\s*=\s*"([^"]+)"', py_src) - -bset, pset = set(bash_names), set(py_names) - -print(f"bash checks: {len(bash_names)} ({len(bset)} distinct)") -print(f"pytest named assertions: {len(py_names)} ({len(pset)} distinct)") -print() - -missing = sorted(bset - pset) -extra = sorted(pset - bset) - -print("PROPERTIES IN THE BASH SUITE AND NOT IN THE PORT:") -if missing: - for n in missing: - print(f" MISSING {n}") -else: - print(" none -- every bash property is asserted by name in the port") -print() -print("ASSERTIONS IN THE PORT AND NOT IN THE BASH SUITE:") -if extra: - for n in extra: - print(f" extra {n}") -else: - print(" none") -print() -print("VERDICT:", "PORT IS INCOMPLETE" if missing else "every bash property is covered") -sys.exit(1 if missing else 0) + +def _parametrized_names(tree): + """-> every name supplied by a `@pytest.mark.parametrize` that declares one. + + A port of a suite whose bash half repeats a property per function writes the arm + once and parametrises it, carrying the bash name as a parameter: + + @pytest.mark.parametrize("func,name", USAGE_ONLY) + def test_a_role_with_only_schema_usage_is_refused(..., func, name): + expect.sqlstate(err, "42501", name) + + The name reaching `expect` is then a variable, and reading only the call site + reports every such property MISSING -- which would push a port AWAY from the one + idiom that keeps the two harnesses one-to-one across a repeated property. + + Only the column actually called `name` is read, resolved through module-level + constants, so the other parameters of the same decorator contribute nothing. + """ + consts = {} + for node in tree.body: + if isinstance(node, ast.Assign) and len(node.targets) == 1 and \ + isinstance(node.targets[0], ast.Name): + try: + consts[node.targets[0].id] = ast.literal_eval(node.value) + except (ValueError, TypeError, SyntaxError): + pass + + out = [] + for node in ast.walk(tree): + if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) + and node.func.attr == "parametrize" and len(node.args) >= 2): + continue + argnames = _as_names(node.args[0]) + if not argnames: + continue + cols = [c.strip() for c in argnames[0].split(",")] + if "name" not in cols: + continue + idx = cols.index("name") + + values = node.args[1] + if isinstance(values, ast.Name): + rows = consts.get(values.id) + else: + try: + rows = ast.literal_eval(values) + except (ValueError, TypeError, SyntaxError): + rows = None + if rows is None: + continue + for row in rows: + if len(cols) == 1: + cell = row + elif isinstance(row, (tuple, list)) and len(row) > idx: + cell = row[idx] + else: + continue + if isinstance(cell, str): + out.append(cell) + return out + + +def _py_names(src): + """Every assertion name in the port, by parsing rather than matching. + + The name is the LAST argument of an `expect.(...)` call, or the value of + a `name=` keyword, read through `_as_names` so a conditional carries both of + its arms. + """ + tree = ast.parse(src) + out = _parametrized_names(tree) + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + is_expect = (isinstance(func, ast.Attribute) + and isinstance(func.value, ast.Name) + and func.value.id == "expect") + for kw in node.keywords: + if kw.arg == "name": + out.extend(_as_names(kw.value)) + if not is_expect or not node.args: + continue + out.extend(_as_names(node.args[-1])) + return out + + +def _as_names(node): + """-> every string this node can evaluate to; [] when it states none. + + A LIST rather than one string, because `"a" if cond else "b"` is a name argument + that carries two properties depending on the arm, and both are asserted by the + suite. Reading only one of them reported the other MISSING, which is the same + false red as reading the wrong argument, one level in. + + An f-string becomes a `{}` template. A node that is neither contributes nothing: + reporting a name as absent is better than reporting the wrong string as present. + """ + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return [node.value] + if isinstance(node, ast.JoinedStr): + parts = [] + for v in node.values: + if isinstance(v, ast.Constant) and isinstance(v.value, str): + parts.append(v.value) + else: + parts.append("{}") + return ["".join(parts)] + if isinstance(node, ast.IfExp): + return _as_names(node.body) + _as_names(node.orelse) + return [] + + +# `${...}`, `$(...)`, `$NAME`, and the POSITIONAL parameters. `$1` is how a bash +# helper names the thing it was called about, so it is the commonest interpolation +# in a check name and the first version of this missed every one of them. +_BASH_INTERP = re.compile( + r'\$\{[^}]*\}|\$\([^)]*\)|\$[A-Za-z_][A-Za-z0-9_]*|\$[0-9]+|\$[@*#?]') + + +def _template(name): + """-> the name with every interpolation reduced to `{}`. + + Applied to both sides, so `non-owner refused: ${1%%(*}` and the f-string + `f"non-owner refused: {fn}"` land on the same string. + """ + return re.sub(r"\{[^{}]*\}", "{}", _BASH_INTERP.sub("{}", name)) + + +def main(bash_file, py_file): + """-> the exit status: 1 when a bash property has no counterpart.""" + bash_names = re.findall( + r'\bcheck(?:_num|_ratio|_text|_timing)?\s+"([^"]+)"', open(bash_file).read()) + py_names = _py_names(open(py_file).read()) + + bset, pset = set(bash_names), set(py_names) + + print(f"bash checks: {len(bash_names)} ({len(bset)} distinct)") + print(f"pytest named assertions: {len(py_names)} ({len(pset)} distinct)") + print() + + literal = bset & pset + # Only names with no literal partner are considered as templates, so a template + # match can never hide a literal one or be double-counted. + b_left, p_left = bset - literal, pset - literal + p_templates = {_template(n) for n in p_left} + templated = {n for n in b_left if _template(n) in p_templates} + + missing = sorted(b_left - templated) + extra = sorted(n for n in p_left if _template(n) not in {_template(m) for m in templated}) + + print("PROPERTIES IN THE BASH SUITE AND NOT IN THE PORT:") + if missing: + for n in missing: + print(f" MISSING {n}") + else: + print(" none -- every bash property is asserted by name in the port") + print() + if templated: + print("MATCHED BY TEMPLATE ONLY (both sides build the name at runtime):") + for n in sorted(templated): + print(f" shape {n}") + print() + print("ASSERTIONS IN THE PORT AND NOT IN THE BASH SUITE:") + if extra: + for n in extra: + print(f" extra {n}") + else: + print(" none") + print() + print(f"literal matches: {len(literal)} | template matches: {len(templated)} | " + f"missing: {len(missing)}") + print("VERDICT:", "PORT IS INCOMPLETE" if missing else "every bash property is covered") + return 1 if missing else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1], sys.argv[2])) diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 72d7d613..b62dd086 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -39,7 +39,10 @@ # 277 -> 280: three arms in test_docs_cover_the_corpus.py about duplicated entries in # VACUITY_MODES.md. Re-derived by collection rather than by adding three, per the recipe # above: `280 tests collected`. -guard_tests 287 +# 287 -> 295 when test_compare_to_bash.py landed: eight arms over the parity tool's +# own extractors. Re-derived by collection, per the recipe above: `295 tests +# collected`. +guard_tests 295 # 282 -> 284: two arms about TESTS.md's own numbering, added with the fix for the # out-of-order contents list #1023's merge left behind. Re-derived by collection. @@ -57,7 +60,9 @@ guard_tests 287 # which is the argument for gating it rather than a detail about it. # 166 -> 177 when test_native_ownership.py landed: nine parametrized refusal arms, # the owner control, and the check-ordering arm. -cluster_tests 205 +# 205 -> 217 when test_projection_privilege.py landed: twelve arms over the two +# projection read helpers, parametrised per function. Re-derived by collection. +cluster_tests 217 # 177 -> 180 when test_stats_privilege.py landed, on top of # test_native_ownership.py's 166 -> 177. DERIVED by collection on the merged # tree, not 169 + 11: both branches bumped this from 166 and the union left two diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py new file mode 100644 index 00000000..25ee5abd --- /dev/null +++ b/test/pytest/test_compare_to_bash.py @@ -0,0 +1,195 @@ +"""`compare_to_bash.py` must read the assertion's NAME, not some other argument (#432). + +The parity tool is what decides whether a port is one-for-one with its bash suite, which +is #432's definition of done. So the tool is a claim like any other, and it was wrong in a +way that pointed directly at the work it grades. + +THE DEFECT. The python side was matched with a regex: + + expect\\.\\w+\\([^)]*?"([^"]+)"\\s*(?:,[^)]*)?\\) + +`[^)]*?` is lazy, so it stopped at the FIRST quoted argument. For `expect.num(got, 1, +NAME)` that is the name, and the tool looked correct on every arm anyone checked. For a +helper whose WANT is itself a string it is not: + + expect.sqlstate(err, "42501", NAME) -> read "42501" as the name + expect.text(got, "none", NAME) -> read "none" + +Every SQLSTATE assertion was therefore read as the literal `42501`, reported as an "extra" +name the bash suite does not have, while the real property was reported MISSING. #432's +ports are precisely the ones replacing a grep on an error message with a SQLSTATE +assertion, so the tool went blind in proportion to the work being done well. Measured over +the seven pairs in the tree: **61 bash properties reported missing, of which 34 were not +missing at all.** Two whole pairs flipped from `PORT IS INCOMPLETE` to complete. + +WHY A GUARD AND NOT JUST A FIX. Nothing could see this. The tool's own output was the only +evidence either way, and its verdict for a correct port was a plausible-looking list of +names that really were absent from the port -- absent because the tool had matched a +different string, which is not visible from the list. `test_hilbert_locality.py` records +somebody working around it by rewriting their test file until the count fell, and +concluding the rest needed a change to this tool. It did. + +THE ARMS BELOW DRIVE THE REAL EXTRACTORS, never a copy. A python twin of a python rule +would agree with itself. +""" + +import ast +import pathlib +import sys + +HERE = pathlib.Path(__file__).resolve().parent +sys.path.insert(0, str(HERE)) + +from compare_to_bash import _as_names, _parametrized_names, _py_names, _template # noqa: E402 + + +def _names(src): + return _py_names(src) + + +def test_the_name_is_the_last_argument_not_the_first_string(expect): + """THE REGRESSION. Three helpers, one of which always worked. + + `expect.num` is the control: its want is a number, so the old regex happened to reach + the name and the tool looked correct. Without that arm this test would pass over a + rule that returns the last argument of nothing at all. + """ + src = ( + 'def t(expect):\n' + ' expect.sqlstate(err, "42501", "a role with no privilege is refused")\n' + ' expect.text(got, "none", "no key is stated twice")\n' + ' expect.num(got, 1, "the owner reads its own table")\n' + ) + got = _names(src) + expect.text(", ".join(sorted(got)), + "a role with no privilege is refused, no key is stated twice, " + "the owner reads its own table", + "each helper contributes its NAME and not its want") + expect.num(len(got), 3, "three assertions, three names") + expect.num(sum(1 for n in got if n in ("42501", "none")), 0, + "and no want is mistaken for a name, which is the defect this closes") + + +def test_a_call_whose_name_is_not_a_literal_contributes_nothing(expect): + """Better absent than wrong. + + A name the tool cannot read must be reported MISSING, which a person then fixes. + Guessing at it reports the wrong string as PRESENT, and a false green on a parity tool + is how a property ends up asserted in neither harness. + """ + src = 'def t(expect):\n expect.sqlstate(err, "42501", some_variable)\n' + expect.num(len(_names(src)), 0, + "an unreadable name yields nothing rather than the want beside it") + + +def test_an_fstring_name_becomes_a_template(expect): + """Both harnesses build some names at runtime. The shape is what can be compared.""" + src = 'def t(expect):\n expect.num(got, 1, f"premise: {r} can open a session")\n' + expect.text(_names(src)[0], "premise: {} can open a session", + "the interpolated part is reduced to a placeholder") + + +def test_a_conditional_name_carries_both_of_its_arms(expect): + """`"a" if cond else "b"` asserts two properties depending on the arm taken. + + Reading one of them reports the other MISSING, which is the same false red as reading + the wrong argument, one level in. + """ + src = ('def t(expect):\n' + ' expect.num(got, 1, "the owner reads" if f == "read" else "the owner writes")\n') + expect.text(", ".join(sorted(_names(src))), "the owner reads, the owner writes", + "both arms of a conditional name are collected") + + +def test_a_parametrized_name_is_resolved_from_the_decorator(expect): + """The idiom a repeated bash property should be ported to. + + When the bash suite states the same property once per function, the port writes the arm + once and parametrises it, carrying the bash name as a parameter. If the tool cannot see + those names it reports every one of them MISSING, which pushes a port away from the one + idiom that keeps the two harnesses one-to-one. + + The control is the second decorator: a parametrize with no `name` column must + contribute nothing, or the tool would harvest every parameter in the file as an + assertion name and report a pile of extras. + """ + src = ( + 'USAGE_ONLY = (\n' + ' ("read_projection", "a role with only schema USAGE is refused"),\n' + ' ("reconstruct_via_projection", "and is refused reconstruct"),\n' + ')\n' + '@pytest.mark.parametrize("func,name", USAGE_ONLY)\n' + 'def t(expect, func, name):\n' + ' expect.sqlstate(err, "42501", name)\n' + '@pytest.mark.parametrize("func", ["read_projection", "reconstruct"])\n' + 'def u(expect, func):\n' + ' expect.num(got, 1, "an unrelated property")\n' + ) + got = _names(src) + expect.num(int("a role with only schema USAGE is refused" in got), 1, + "a parametrized name is resolved through the module-level constant") + expect.num(int("and is refused reconstruct" in got), 1, "for every row of it") + expect.num(int("read_projection" in got), 0, + "while the OTHER column of the same decorator is not a name") + expect.num(int("reconstruct" in got), 0, + "and a parametrize with no name column contributes nothing") + + +def test_the_parametrize_reader_takes_the_column_called_name(expect): + """Position is not the rule; the declared column is. + + A port that writes `parametrize("name,func", ...)` states the same properties, and a + reader keyed on position silently harvests the function names instead. + """ + tree = ast.parse( + 'ROWS = (("the property", "read_projection"),)\n' + '@pytest.mark.parametrize("name,func", ROWS)\n' + 'def t(name, func):\n pass\n' + ) + expect.text(", ".join(_parametrized_names(tree)), "the property", + "the name column is found by its declared name, whatever its position") + + +def test_the_two_harnesses_interpolations_land_on_one_template(expect): + """What makes a template match mean anything: bash and python spell it differently.""" + expect.text(_template("non-owner refused: ${1%%(*}"), "non-owner refused: {}", + "a bash parameter expansion is reduced to a placeholder") + expect.text(_template("non-owner refused: {}"), "non-owner refused: {}", + "and an f-string template is already in that form, so the two meet") + expect.text(_template("premise: $PGC_PORT is open"), "premise: {} is open", + "a bare variable reference too") + + +def test_the_ported_suites_in_this_tree_are_graded_one_for_one(expect): + """THE STANDING ARM, and the reason this file is not only about fixtures. + + A guard over invented sources proves the extractor reads python. It cannot prove the + tool grades THIS tree, which is the claim #432 rests on. So the pairs that are declared + complete are asserted complete here, and a later edit that breaks parity fails with the + pair named rather than the whole gate going red for an unrelated reason. + + Only the pairs that reach zero today are listed. A pair with a real gap is not pinned to + its gap: that would turn the gap into the expected state. + """ + from compare_to_bash import main + import contextlib + import io + + root = HERE.parent.parent + # EVERY pair in the tree. When a new port lands it belongs here, and when one + # cannot reach zero the reason belongs in its own file rather than in an omission + # from this list. + complete = ["differential", "hilbert_locality", "native_ownership", + "native_projection", "projection_privilege", "stats_privilege", + "zonemap_boundaries"] + verdicts = {} + for stem in complete: + sh, py = root / "test" / f"{stem}.sh", HERE / f"test_{stem}.py" + expect.num(int(sh.exists() and py.exists()), 1, f"premise: both halves of {stem} exist") + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + rc = main(str(sh), str(py)) + verdicts[stem] = rc + expect.text(", ".join(f"{k}={v}" for k, v in sorted(verdicts.items())), + ", ".join(f"{k}=0" for k in sorted(complete)), + "every pair declared one-for-one still grades one-for-one") diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 63e8b27f..db74a062 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -106,6 +106,9 @@ # no driver: the public seam is the published page. "test_docs_join_clustering.py", "test_docs_table_structure.py", + # The parity tool is python and reads python. No cluster and no driver: its input + # is a source file and its output is a verdict about two source files. + "test_compare_to_bash.py", ] diff --git a/test/pytest/test_projection_privilege.py b/test/pytest/test_projection_privilege.py new file mode 100644 index 00000000..15fbac09 --- /dev/null +++ b/test/pytest/test_projection_privilege.py @@ -0,0 +1,394 @@ +"""The projection READ helpers are a privilege boundary (#562, #563; ported for #432). + +`pgcolumnar.read_projection()` and `pgcolumnar.reconstruct_via_projection()` opened a +caller-supplied regclass and returned its contents with no privilege check of any kind, +and `CREATE FUNCTION` grants EXECUTE to PUBLIC. `reconstruct_via_projection` is the worse +of the two: it rebuilds NON-COVERED columns from the base relation by row number, so the +projection was never the bound on what leaked. One projection on any column exposed the +whole row. + +The bar is ACL_SELECT on the BASE relation rather than ownership, and that is a +correctness argument rather than a lenient one: reconstruct returns columns the projection +does not store, so SELECT on the base is exactly the privilege that governs reading those +columns by any other route. + +WHAT THIS PORT ASSERTS THAT `projection_privilege.sh` CANNOT. + +That suite decides four things by matching error TEXT, and two of them are load-bearing +rather than decorative, because the refusals it must tell apart share a SQLSTATE. Its own +comment says so: + + Both layers reject this role, so a bare "refused" stays true if the REVOKE is deleted + and the C check catches it instead -- measured: with the REVOKE removed this suite + still passed 14 of 14. The error text is the only thing that attributes the refusal. + +That is correct about the shell harness and it is not a limit of the property. Three +refusals arrive from three different places in the code, and each carries its own +SQLSTATE the moment you stop asking which words the message contains: + + the SQL grant on the function 42501, raised before the body runs at all + the base-relation ACL check 42501, the first statement of the body + row-level security 0A000, ERRCODE_FEATURE_NOT_SUPPORTED + the projection lookup 42704, ERRCODE_UNDEFINED_OBJECT + +Two of those are 42501, so SQLSTATE alone does not separate the two ACL layers either. +**The discriminator is the fixture, not the message.** Call the function with a projection +name that does not exist, on a table the role may read: + + stopped by the SQL grant -> 42501, because the body never ran to notice the name + past the SQL grant -> 42704, because it ran and looked the name up + +So `test_which_layer_refused` attributes the refusal to a layer by what the code REACHED, +and reaching is a fact about execution rather than about wording. `grep -c 'permission +denied for function'` is satisfied by any future message containing that phrase; this is +satisfied only by control actually arriving at the projection lookup. + +The same move gives the port two arms the shell suite does not have at all: the base-ACL +check must run BEFORE the projection lookup, and before the RLS check. Both are asserted +as orderings in `src/columnar_projection.c`, the second one in a comment recording that an +earlier version had it backwards and disclosed RLS state to a caller with no privilege. +Nothing tested either until now. + +REAL LOGINS, NOT `SET ROLE`. A deny arm is evidence only if the call reached the code that +denies it, and `projection_privilege.sh` records measuring that exact hole: with `ALTER +ROLE t_prjexec NOLOGIN` injected, its EXECUTE premise and both "is refused" checks still +passed. Every premise here is run BY the role it is about. +""" + +import pytest + +ROWS = 2000 +NONE, EXEC, SEL = "t_prjnone", "t_prjexec", "t_prjsel" +ROLES = (NONE, EXEC, SEL) + +FUNCS = ("read_projection", "reconstruct_via_projection") + +# (function, the name `projection_privilege.sh` gives this exact property). The names +# are the bash suite's character for character, which is what lets compare_to_bash.py +# pair the two harnesses by property instead of by shape. +USAGE_ONLY = ( + ("read_projection", "a role with only schema USAGE is refused read_projection"), + ("reconstruct_via_projection", "and is refused reconstruct_via_projection"), +) +EXEC_NO_SELECT = ( + ("read_projection", "a role with EXECUTE but no SELECT is refused read_projection"), + ("reconstruct_via_projection", + "and is refused reconstruct_via_projection, which leaks non-covered columns"), +) +WITH_SELECT = ( + ("read_projection", "a role WITH SELECT still reads the projection"), + ("reconstruct_via_projection", "and still reconstructs"), +) +UNDER_POLICY = ( + ("read_projection", "read_projection now refuses a policy-restricted caller (#563)"), + ("reconstruct_via_projection", "and so does reconstruct_via_projection"), +) + + +def _as(cluster, role, sql, schema=None): + """Run one statement on a fresh connection as `role`, returning (rows, error). + + THE SET IS ITS OWN EXECUTE. psycopg3 returns the FIRST statement's result for a + multi-statement execute, so `SET search_path ...; SELECT ...` hands back the SET's + empty result. Ported from `test_stats_privilege.py`, where collapsing that into a 0 + produced a product claim out of a driver detail. + """ + import psycopg + + dsn = f"host=127.0.0.1 port={cluster.port} user={role} dbname=postgres" + try: + with psycopg.connect(dsn, autocommit=True) as conn: + with conn.cursor() as cur: + if schema: + cur.execute(f'SET search_path TO "{schema}", pgcolumnar, public') + cur.execute(sql) + return cur.fetchall(), None + except psycopg.Error as exc: + return None, exc + + +def _fixture(cur): + """Two tables and three roles, one role per layer so a refusal can be attributed. + + t_prjnone schema USAGE only. Stopped by the SQL REVOKE. + t_prjexec USAGE + EXECUTE. Clears the SQL layer ON PURPOSE, so a + refusal can only be the C check. + t_prjsel USAGE + EXECUTE + SELECT. Must SUCCEED -- this is the arm that + distinguishes ACL_SELECT from ownership. + + `open_t` is the second table, and it is what makes the layer attribution possible: + every role may read it, so a refusal on it cannot come from the base-relation ACL and + a call naming a projection it does not have reaches the lookup or it does not. + """ + for r in ROLES: + cur.execute(f"SELECT 1 FROM pg_roles WHERE rolname = '{r}'") + if cur.fetchone() is None: + cur.execute(f"CREATE ROLE {r} NOSUPERUSER LOGIN") + cur.execute(f"GRANT USAGE ON SCHEMA pgcolumnar TO {r}") + + cur.execute("SELECT current_schema()") + schema = cur.fetchone()[0] + for r in ROLES: + cur.execute(f'GRANT USAGE ON SCHEMA "{schema}" TO {r}') + + cur.execute("DROP TABLE IF EXISTS prj_secret") + cur.execute("CREATE TABLE prj_secret (id int, ssn text, note text) USING pgcolumnar") + cur.execute( + f"INSERT INTO prj_secret " + f"SELECT g, 'ssn-'||g, 'note-'||g FROM generate_series(1,{ROWS}) g" + ) + # The projection covers id and ssn. It does NOT cover note, which is the point: + # reconstruct returns note anyway. + cur.execute( + "SELECT pgcolumnar.add_projection(" + "'prj_secret','p1',ARRAY['id','ssn'],ARRAY['id'])" + ) + cur.execute("SELECT pgcolumnar.rebuild_projections('prj_secret')") + cur.execute("REVOKE ALL ON prj_secret FROM PUBLIC") + + cur.execute("DROP TABLE IF EXISTS prj_open") + cur.execute("CREATE TABLE prj_open (id int) USING pgcolumnar") + cur.execute("INSERT INTO prj_open SELECT generate_series(1,10)") + + for r in (EXEC, SEL): + for f in FUNCS: + cur.execute( + f"GRANT EXECUTE ON FUNCTION pgcolumnar.{f}(regclass,text) TO {r}" + ) + cur.execute(f"GRANT SELECT ON prj_secret TO {SEL}") + for r in ROLES: + cur.execute(f"GRANT SELECT ON prj_open TO {r}") + return schema + + +def test_the_premises_the_fixture_is_what_the_suite_assumes(pgc_cluster, pgc_conn, expect): + """Every premise about a role is run BY that role. + + A DENIED result and a BROKEN result are indistinguishable from outside, so the owner + arms are not decoration: they assert a ROW COUNT on the same call the deny arms make. + If a later change made these functions return zero rows for everyone, a no-error owner + arm would stay green while every deny arm stayed green, and the suite would pass over a + function that does nothing. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + + # CARDINALITY FIRST, for every loop below. A loop over an empty sequence runs no + # assertion and reports no failure, so each of these says how many arms it owes + # before it runs them. + expect.num(len(ROLES), 3, "premise: three roles, one per privilege layer") + expect.num(len(FUNCS), 2, "premise: both read helpers are under test") + + for r in ROLES: + rows, err = _as(pgc_cluster, r, "SELECT 1", schema) + assert err is None, f"{r} could not open a session: {err}" + expect.num(rows[0][0], 1, "premise: t_prjexec can open a session, so a refusal below is a refusal" + if r == EXEC else f"premise: {r} can open a session of its own") + + with pgc_conn.cursor() as cur: + for f in FUNCS: + cur.execute(f"SELECT count(*) FROM pgcolumnar.{f}('prj_secret','p1')") + expect.num(cur.fetchone()[0], ROWS, + "premise: the owner reads the projection, and gets rows" if f == "read_projection" + else "premise: the owner reconstructs, and gets rows") + cur.execute( + "SELECT pgcolumnar.reconstruct_via_projection('prj_secret','p1') " + "LIKE '%note-%' LIMIT 1" + ) + expect.text(str(cur.fetchone()[0]), "True", + "premise: reconstruct really does return the NON-COVERED column") + cur.execute( + "SELECT pgcolumnar.read_projection('prj_secret','p1') LIKE '%note-%' LIMIT 1" + ) + expect.text(str(cur.fetchone()[0]), "False", + "premise: and read_projection does NOT, so the two differ as the issue says") + cur.execute( + "SELECT count(*) FROM pg_roles WHERE rolname = ANY(%s) AND rolsuper", (list(ROLES),) + ) + expect.num(cur.fetchone()[0], 0, "premise: the test roles are not superusers") + # A CATALOG LOOKUP RUN AS THE OWNER, which is blind to whether t_prjexec can + # open a session at all -- hence the session premise above, which that role + # runs itself. Both are needed: this says the grant exists, that says it can + # be exercised. + cur.execute( + "SELECT has_function_privilege(" + "'t_prjexec','pgcolumnar.read_projection(regclass,text)','EXECUTE')" + ) + expect.text(str(cur.fetchone()[0]), "True", + "premise: t_prjexec really does hold EXECUTE, so it reaches the C gate") + + _, err = _as(pgc_cluster, NONE, "SELECT count(*) FROM prj_secret", schema) + expect.sqlstate(err, "42501", + "premise: the unprivileged role cannot read the table by any ordinary route") + + rows, err = _as(pgc_cluster, SEL, "SELECT count(*) FROM prj_secret", schema) + assert err is None, f"{SEL} could not read the table: {err}" + expect.num(rows[0][0], ROWS, + "premise: t_prjsel CAN read the table, so its arm below tests the bar") + + for r in ROLES: + rows, err = _as(pgc_cluster, r, "SELECT count(*) FROM prj_open", schema) + assert err is None, f"{r} could not read prj_open: {err}" + expect.num(rows[0][0], 10, + f"premise: {r} may read prj_open, so a refusal on it is not the ACL") + + +@pytest.mark.parametrize("func,name", USAGE_ONLY) +def test_a_role_with_only_schema_usage_is_refused(pgc_cluster, pgc_conn, expect, func, name): + """Layer one: the SQL grant.""" + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + _, err = _as(pgc_cluster, NONE, + f"SELECT count(*) FROM pgcolumnar.{func}('prj_secret','p1')", schema) + expect.sqlstate(err, "42501", name) + + +@pytest.mark.parametrize("func,name", EXEC_NO_SELECT) +def test_a_role_with_execute_but_no_select_is_refused(pgc_cluster, pgc_conn, expect, func, name): + """Layer two: the C check, reached only because EXECUTE was granted. + + This role clears the SQL layer deliberately. If it is refused, the refusal cannot be + the REVOKE and cannot be the schema. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + _, err = _as(pgc_cluster, EXEC, + f"SELECT count(*) FROM pgcolumnar.{func}('prj_secret','p1')", schema) + expect.sqlstate(err, "42501", name) + if func == "read_projection": + expect.sqlstate( + err, "42501", + "and THAT refusal is the C check, which raises 42501 from aclcheck_error") + + +def test_which_layer_refused_without_reading_the_message(pgc_cluster, pgc_conn, expect): + """THE ARM THE SHELL SUITE NEEDS ERROR TEXT FOR, decided by execution instead. + + Both layers raise 42501, so the code alone cannot tell them apart and a bare "refused" + stays true if the REVOKE is deleted and the C check catches it instead. Measured in + `projection_privilege.sh`: with the REVOKE removed, that suite still passed 14 of 14. + + So ask a question only one layer can answer. On `prj_open`, which EVERY role may read, + with a projection name that does not exist: + + stopped by the SQL grant 42501 the body never ran, so the name was never read + past the SQL grant 42704 it ran, and the projection lookup raised + + The difference is which code executed. That is not a property of the wording, and no + future rephrasing of either message can move it. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + call = "SELECT count(*) FROM pgcolumnar.read_projection('prj_open','no_such_proj')" + + _, err = _as(pgc_cluster, NONE, call, schema) + expect.sqlstate(err, "42501", + "the no-EXECUTE role never reaches the body, so the grant is what stopped it") + + _, err = _as(pgc_cluster, EXEC, call, schema) + expect.sqlstate(err, "42704", + "while the EXECUTE role reaches the projection lookup on the same call") + + +def test_the_base_acl_is_checked_before_the_projection_is_looked_up( + pgc_cluster, pgc_conn, expect): + """An ordering `src/columnar_projection.c` asserts and nothing tested. + + The ACL check is the first statement of the body and the projection lookup is well + below it. If they were swapped, a caller with no SELECT would learn whether a named + projection exists on a table it may not read -- existence disclosure, from a function + whose whole purpose is to stop disclosure. + + The control is the arm above: the same role, the same bogus name, on a table it MAY + read, returns 42704. So 42501 here is the ACL check winning the race rather than the + lookup being unreachable. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + _, err = _as(pgc_cluster, EXEC, + "SELECT count(*) FROM pgcolumnar.read_projection(" + "'prj_secret','no_such_proj')", schema) + expect.sqlstate(err, "42501", + "the base ACL is checked before the projection name is looked up") + + +@pytest.mark.parametrize("func,name", WITH_SELECT) +def test_a_role_with_select_still_reads(pgc_cluster, pgc_conn, expect, func, name): + """THE CONTROL FOR THE WHOLE SUITE. Without it the fix could pass by refusing + everyone, which is not a fix, it is a broken function with a good error message. + + It is also what distinguishes ACL_SELECT from ownership: this role owns nothing, and + an owner-only bar refuses it. Refusing it would be a regression, not a fix. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + rows, err = _as(pgc_cluster, SEL, + f"SELECT count(*) FROM pgcolumnar.{func}('prj_secret','p1')", schema) + assert err is None, f"{SEL} was refused {func}: {type(err).__name__}: {err}" + expect.num(rows[0][0], ROWS, name) + + +@pytest.mark.parametrize("func,name", UNDER_POLICY) +def test_a_policy_restricted_caller_is_refused(pgc_cluster, pgc_conn, expect, func, name): + """Row-level security, closed by #563, and a DIFFERENT SQLSTATE rather than a phrase. + + ACL_SELECT answers "may this role read this table". RLS answers "which rows". The + direct-storage paths cannot answer the second: policies are applied by the REWRITER to + a query's range table entry, and these functions never build a query. A caller holding + SELECT but restricted to one row by a policy received every row. + + `PgColumnarRequireNoRowSecurity` raises ERRCODE_FEATURE_NOT_SUPPORTED, so this arm + asserts 0A000 where the shell suite greps `^ERROR:.*row-level security`. The codes are + from different SQLSTATE CLASSES -- 0A and 42 -- so this cannot be confused with either + ACL refusal by any amount of rewording, and the premise below shows the policy really + took effect rather than the call having broken for some other reason. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + cur.execute("ALTER TABLE prj_secret ENABLE ROW LEVEL SECURITY") + cur.execute("DROP POLICY IF EXISTS p_one ON prj_secret") + cur.execute(f"CREATE POLICY p_one ON prj_secret FOR SELECT TO {SEL} USING (id = 1)") + + rows, err = _as(pgc_cluster, SEL, "SELECT count(*) FROM prj_secret", schema) + assert err is None, f"{SEL} could not read the table under the policy: {err}" + expect.num(rows[0][0], 1, "premise: the policy is in force for ordinary SQL") + + _, err = _as(pgc_cluster, SEL, + f"SELECT count(*) FROM pgcolumnar.{func}('prj_secret','p1')", schema) + expect.sqlstate(err, "0A000", name) + if func == "read_projection": + expect.sqlstate( + err, "0A000", + "and the refusal says the feature is not supported, not that a privilege " + "is missing") + + +def test_the_acl_is_checked_before_rls_so_no_privilege_discloses_no_rls_state( + pgc_cluster, pgc_conn, expect): + """The second ordering, recorded in the source as a correction made in review. + + `PgColumnarRequireNoRowSecurity` is called AFTER the relation ACL check. An earlier + version had it first, and `src/columnar_vacuum.c` records what that produced: + + RLS on, no-select, ordinary SQL -> permission denied for table + RLS on, no-select, read_projection -> row-level security is in force + + A caller with no privilege at all was told the table has RLS enabled, which ordinary + SQL does not disclose and which disagrees with core's ordering. + + THE TWO OUTCOMES ARE DIFFERENT SQLSTATE CLASSES, so this arm is decidable: 42501 means + the ACL check ran first and 0A000 means RLS did. The arm above is its control -- the + same table, the same RLS, a role that DOES hold SELECT, and 0A000 -- which is what + makes 42501 here an ordering result rather than the RLS check being switched off. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + cur.execute("ALTER TABLE prj_secret ENABLE ROW LEVEL SECURITY") + cur.execute("DROP POLICY IF EXISTS p_one ON prj_secret") + cur.execute(f"CREATE POLICY p_one ON prj_secret FOR SELECT TO {SEL} USING (id = 1)") + + _, err = _as(pgc_cluster, EXEC, + "SELECT count(*) FROM pgcolumnar.read_projection('prj_secret','p1')", + schema) + expect.sqlstate(err, "42501", + "a caller with no SELECT is told a privilege is missing, not that RLS is in force") diff --git a/test/pytest/test_stats_privilege.py b/test/pytest/test_stats_privilege.py index d1c588f3..16161820 100644 --- a/test/pytest/test_stats_privilege.py +++ b/test/pytest/test_stats_privilege.py @@ -82,10 +82,20 @@ def test_the_premises_each_role_is_what_the_suite_assumes(pgc_cluster, pgc_conn, """Each premise is run BY the role it is about, which is why real logins matter.""" with pgc_conn.cursor() as cur: schema = _fixture(cur) + # WRITTEN OUT, NOT LOOPED, and the names are the bash suite's character for + # character. The loop that was here passed `f"premise: {r} can open a session"`, + # which compare_to_bash.py reads as the template `premise: {} can open a session` + # -- matching neither bash name, so both properties were reported MISSING from a + # port that asserts them. A name held in a variable is unreadable to the parity + # tool by design: guessing at it would report the wrong string as PRESENT. + sessions = {} for r in (OWNER, NONE, SEL): rows, err = _as(pgc_cluster, r, 'SELECT 1', schema) assert err is None, f"{r} could not connect: {err}" - expect.num(rows[0][0], 1, f"premise: {r} can open a session") + sessions[r] = rows[0][0] + expect.num(sessions[OWNER], 1, "premise: the owner can open a session") + expect.num(sessions[NONE], 1, "premise: the no-privilege role can open a session") + expect.num(sessions[SEL], 1, "premise: the granted role can open a session") with pgc_conn.cursor() as cur: cur.execute("SELECT relowner::regrole::text FROM pg_class WHERE relname = 'st_t'") diff --git a/test/pytest/test_zonemap_boundaries.py b/test/pytest/test_zonemap_boundaries.py index 03c2f53d..807241d9 100644 --- a/test/pytest/test_zonemap_boundaries.py +++ b/test/pytest/test_zonemap_boundaries.py @@ -48,7 +48,8 @@ def test_exact_zonemap_boundaries(pgc_conn, expect): "SELECT count(*) FROM pgcolumnar.row_group " "WHERE storage_id=pgcolumnar.get_storage_id('zb_c')" ) - expect.num(cur.fetchone()[0], 2, "premise: fixture has two row groups") + expect.num(cur.fetchone()[0], 2, + "premise: the boundary fixture has two row groups") expect.num( _removed(_plan(pgc_conn, "v < 1001")), 1, @@ -80,3 +81,13 @@ def test_exact_zonemap_boundaries(pgc_conn, expect): _removed(_plan(pgc_conn, "v = 1001")), 1, "= excludes the group lying wholly below the constant", ) + # THE LIVENESS PREMISE, last, as in the bash suite. + # + # Every arm above reads a plan or a row set, and a backend that died partway through + # would leave the arms that already ran green and the rest unrun. `pgc_summary` + # accounting catches a missing arm, but only this says the session that produced the + # answers was still the one answering at the end. It is cheap and it is the difference + # between "the arms passed" and "the arms passed on a live server". + with pgc_conn.cursor() as cur: + cur.execute("SELECT 1") + expect.num(cur.fetchone()[0], 1, "backend alive")