Skip to content

Fixed relation privilege checks - #3451

Merged
Hydrocharged merged 2 commits into
mainfrom
daylon/issue-3448
Sep 30, 2026
Merged

Hydrocharged merged 2 commits into
mainfrom
daylon/issue-3448

Conversation

@Hydrocharged

@Hydrocharged Hydrocharged commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes:

Lets all roles read from the public system tables, and corrects table and view permissions. Some tests are skipped due to pre-existing issues that are outside the scope of this PR.

Builds on:

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor
Main PR
Total 42090 42090
Successful 20253 20253
Failures 21837 21837
Partial Successes1 5406 5406
Main PR
Successful 48.1183% 48.1183%
Failures 51.8817% 51.8817%

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct. ↩

@itoqa

itoqa Bot commented Sep 29, 2026

Copy link
Copy Markdown

Ito QA test results
Commit: efc1a35: 11 test cases ran, 2 failed ❌, 9 passed ✅.

Summary

Coverage spans database authorization happy paths and adversarial access attempts, including protected reads and writes, nested queries, concurrent access, connection recovery, and granted views across different schemas. Most permission boundaries behave correctly, but view authorization behavior remains faulty for both reads and denial reporting.

Merge with caution — this PR introduces a medium-severity authorization regression that makes explicitly granted views unusable, plus a minor issue where denied view writes identify the underlying table instead of the view. Unauthorized writes remain blocked, but the broken granted-view behavior affects intended access and warrants follow-up before treating the change as fully safe.

Tests run by Ito

View full run

Result Severity Type Description
❌ Medium severity Rev The first SELECT from reporting.order_view was denied with permission denied for table orders even though analyst had SELECT on the view. After the grant was moved to public.order_view, the qualified reporting query still failed with SQLSTATE 42501 and still named orders instead of the reporting view.
❌ Minor severity Rev Each write was rejected, but the error identified table items rather than view items_view.
✅ — Authorization The reader could read the authorized edges table. The protected secret table was denied with a relation-specific insufficient-privilege error, and the session remained usable.
✅ — General The authorized table can be read, but a CTE that also reads the protected table is denied before any protected rows are returned.
✅ — General The denied database operation returned PostgreSQL error 42501, and the same connection successfully ran a follow-up query.
✅ — General A minimally granted reader can view ordinary catalog metadata, but cannot read the protected role catalog or the protected user table.
✅ — General The reader can open the allowed table, but changing the schema name or using the search path does not expose protected data.
✅ — General Across 20 rounds, the authorized table query returned its rows and the unauthorized view query stayed denied with view-specific permission text. The two results did not affect each other.
✅ — Cte The authorized reader completed ordinary, recursive, nested, and self-joined CTE queries over the granted edges table. CTE-based writes to the granted target table also completed, while a CTE over the protected secret table was denied.
✅ — Database The reader could not create a database and received SQLSTATE 42501 with a clear permission message. The same connection then completed a follow-up query.
✅ — Privilege The reader can inspect public catalog information and read the allowed edges table. Attempts to read, update, or delete the protected secret table are denied with insufficient-privilege errors.

Tip

Reply with @itoqa to send us feedback on this test run.

Comment thread server/auth/auth_handler.go
Comment thread server/auth/auth_handler.go
@coffeegoddd

coffeegoddd commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

@Hydrocharged DOLT

read_tests from_latency to_latency percent_change
covering_index_scan_postgres 2.48 2.43 -2.02
groupby_scan_postgres 77.19 75.82 -1.77
index_join_postgres 2.3 2.26 -1.74
index_join_scan_postgres 1.64 1.61 -1.83
index_scan_postgres 458.96 475.79 3.67
oltp_point_select 0.37 0.37 0.0
oltp_read_only 6.43 6.32 -1.71
select_random_points 0.73 0.73 0.0
select_random_ranges 1.06 1.04 -1.89
table_scan_postgres 467.3 467.3 0.0
types_table_scan_postgres 1170.65 1191.92 1.82
write_tests from_latency to_latency percent_change
oltp_delete_insert_postgres 6.79 6.67 -1.77
oltp_insert 3.36 3.36 0.0
oltp_read_write 13.46 13.46 0.0
oltp_update_index 3.62 3.55 -1.93
oltp_update_non_index 3.3 3.25 -1.52
oltp_write_only 7.04 7.04 0.0
types_delete_insert_postgres 7.17 7.17 0.0

@itoqa

itoqa Bot commented Sep 29, 2026

Copy link
Copy Markdown

Ito QA test results
Ito Diff Report — efc1a35 → 3b7a063: 9 test cases ran, 1 fixed ✅, 7 passing ✅, 1 additional finding ⚠️.

Diff Summary

Coverage spans normal authorized database reads, denied access and write attempts, nested queries and views, transaction rollback, connection recovery, and clear permission errors. Overall, the exercised authorization behavior remains healthy: allowed operations work, protected data and changes stay blocked, and failed operations do not alter stored data.

Safe to merge — the only failure is a minor, pre-existing error-message issue unrelated to this PR, while the tested authorization behavior shows no PR-attributable regressions or new failures. The unrelated issue is suitable for follow-up rather than a merge blocker.

Tests run by Ito

View full run

Result State Severity Type Description
✅ ❌->✅ Fixed — Rev All three write attempts were rejected, and the original row stayed in the base table.
✅ Passing — General The allowed update ran first, then the denied operation returned a PostgreSQL permission error. After recovery, the original row was still unchanged.
✅ Passing — Cte The authorized query completed successfully and returned rows from the granted table.
✅ Passing — Cte The query was rejected because the role had no permission to read the physical table inside the CTE. PostgreSQL returned the expected insufficient-privilege error with SQLSTATE 42501.
✅ Passing — Error A role without access received the expected PostgreSQL privilege error, including SQLSTATE 42501 and the denied table name.
✅ Passing — Privilege The allowed database read returned the seeded row. The update without the required privilege was blocked with the expected PostgreSQL error.
✅ Passing — Relation The authorized analyst connected to the local database and read the reporting view successfully. The query returned the seeded row with id 1.
✅ Passing — Relation INSERT, UPDATE, and DELETE through items_view were denied with the expected permission error, and the underlying row stayed unchanged.
⏸️ Skipped — Authorization The reader could read the authorized edges table. The protected secret table was denied with a relation-specific insufficient-privilege error, and the session remained usable.
⏸️ Skipped — General The authorized table can be read, but a CTE that also reads the protected table is denied before any protected rows are returned.
⏸️ Skipped — General The denied database operation returned PostgreSQL error 42501, and the same connection successfully ran a follow-up query.
⏸️ Skipped — General A minimally granted reader can view ordinary catalog metadata, but cannot read the protected role catalog or the protected user table.
⏸️ Skipped — General The reader can open the allowed table, but changing the schema name or using the search path does not expose protected data.
⏸️ Skipped — General Across 20 rounds, the authorized table query returned its rows and the unauthorized view query stayed denied with view-specific permission text. The two results did not affect each other.
⏸️ Skipped — Database The reader could not create a database and received SQLSTATE 42501 with a clear permission message. The same connection then completed a follow-up query.
⚠️ Additional Finding Minor severity Rev Each write was rejected, but the error said permission denied for table items instead of permission denied for view items_view.
Additional Findings Details

These findings are unrelated to the current changes but were observed during testing.

⚪ View write errors name the wrong relation
  • Severity: Minor Minor severity
  • Description: Each write was rejected, but the error said permission denied for table items instead of permission denied for view items_view.
  • Impact: Users who try to write through the view receive an error naming the underlying table instead of the view they used. The write is still blocked and the data remains unchanged, but the misleading name can make permission problems harder to diagnose.
  • Steps to Reproduce:
    1. Create an items table with one row and an items_view view over that table.
    2. Grant the writer role SELECT on items_view but no write privileges.
    3. As writer, run INSERT INTO items_view VALUES (2), UPDATE items_view SET id = 3, and DELETE FROM items_view WHERE id = 1 as separate statements.
    4. Inspect each error message and confirm the base table still contains only id 1.
  • Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code Analysis: The production authorization entry point is AuthorizationHandler.HandleResolvedTableAuth in server/auth/auth_handler.go:314-330. It changes auth.TargetType to AuthTargetType_ViewIdentifiers only when the resolved node is a *plan.SubqueryAlias at lines 325-328; otherwise it keeps table authorization and forwards the original resolved relation metadata to HandleAuth. HandleAuth maps table/view authorization to checkPrivilegeOnTable at lines 193-212, and checkPrivilegeOnTable constructs the final PostgreSQL error from relationKind and tableName at lines 362-379. The runtime result (permission denied for table items) is consistent with this path reaching checkPrivilegeOnTable as relationKind=table and tableName=items even though the user wrote to items_view. The smallest fix is to preserve the resolved view name and view relation kind for this DML path before calling HandleAuth, then add or enable the three assertions as a regression check; no change is needed to the existing privilege denial itself.
Evidence Package

Tip

Reply with @itoqa to send us feedback on this test run.

@zachmu zachmu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@itoqa

itoqa Bot commented Sep 30, 2026

Copy link
Copy Markdown

Ito QA test results

History reset (rebase or force-push detected). Starting test narrative over.

Commit: b2c4d44: 14 test cases ran, 13 passed ✅, 1 additional finding ⚠️.

Summary

Coverage focused on database access control: permitted reads and writes, denied operations, restricted metadata, views, upserts, nested queries, and recovery after access errors. It exercised both expected user flows and adversarial permission-boundary and data-integrity cases, with the application behavior broadly matching the intended safeguards.

Safe to merge — the only observed failure is a medium-severity, pre-existing authorization error that is explicitly not attributable to this PR, and no regression or new PR-related failure was identified. It is a flag for later rather than a merge blocker.

Tests run by Ito

View full run

Result Severity Type Description
✅ — General Public catalog metadata was available to the reader role, but every working way to query pg_authid was denied without returning restricted rows.
✅ — General The local test setup did not provide the database role and fixtures needed to check view and table permissions. No application authorization result or changed row was produced.
✅ — General The upsert was rejected because the role lacked update permission. The existing value and the number of rows stayed unchanged.
✅ — General A denied table read returned the expected permission error. The same connection then returned the catalog row and both edge rows from the permitted queries.
✅ — General The database correctly denied an existing table without permission and used a different error only for a table that did not exist.
✅ — General The source rows could be read, but the write to the ungranted target was rejected and the target stayed unchanged.
✅ — General A reader could query public catalog metadata, but both direct and aliased access to pg_authid were denied without exposing rows.
✅ — General A reader could alternate denied secret-table queries with permitted edges-table queries on one connection. Each denied query returned SQLSTATE 42501, and each permitted query returned rows.
✅ — Catalog The reader can see ordinary catalog metadata, while access to the restricted pg_authid data is denied with PostgreSQL error 42501.
✅ — Cte All four query forms returned the expected rows through the permitted edges table. The CTE names stayed local and did not need separate table grants.
✅ — Privilege A role could read the table it was granted access to, while reads and changes to another table were rejected with the expected permission error.
✅ — Upsert An INSERT-only role was blocked from changing an existing row, and the same upsert succeeded after UPDATE permission was granted.
✅ — View The local test environment stopped before the view authorization check could run. The repository's view authorization code and regression coverage define the granted-read and denied-read behavior without showing a product defect.
⚠️ Medium severity General The permitted table and denied table checks behaved as expected, but the denied view returned SQLSTATE XX000 with the message 'only a single statement at a time is currently supported' instead of SQLSTATE 42501 identifying the view.
Additional Findings Details

These findings are unrelated to the current changes but were observed during testing.

🟡 Denied view returns the wrong database error
  • Severity: Medium Medium severity
  • Description: The permitted table and denied table checks behaved as expected, but the denied view returned SQLSTATE XX000 with the message 'only a single statement at a time is currently supported' instead of SQLSTATE 42501 identifying the view.
  • Impact: Applications querying a view they cannot access receive a generic database error instead of a permission-denied error. The view stays protected, but clients may not handle or explain the denial correctly.
  • Steps to Reproduce:
    1. Connect as a role that can read one table but has no privilege on a view over another table.
    2. Read the permitted table and confirm that it returns rows.
    3. Read the denied view in the same client flow.
    4. Check the returned SQLSTATE and error message.
  • Stub / mock content: The run used isolated local database fixtures, roles, and grants to exercise the authorization flow. No external service mocks were applied.
  • Code Analysis: The production authorization path is designed to preserve the relation kind for views. In server/auth/auth_handler.go:313-330, HandleResolvedTableAuth changes a resolved plan SubqueryAlias to AuthTargetType_ViewIdentifiers before calling HandleAuth. HandleAuth then routes both table and view identifiers through server/auth/auth_handler.go:193-212, and checkPrivilegeOnTable at lines 362-381 formats an insufficient-privilege error using the relationKind. For a denied view, that path should therefore reach server/auth/auth_handler.go:378 with relationKind set to 'view' and return PostgreSQL SQLSTATE 42501. The recorded response instead came back from the generic single-statement guard in postgres/parser/parser/sql/sql_parser.go:73-80, before the authorization error described by the handler. This establishes a production-code failure in the tested view request path, but the available diff does not prove that the parser response was introduced by this PR. The smallest practical fix is to trace the denied-view request through parsing and resolved-table authorization, then ensure a single denied-view query reaches HandleResolvedTableAuth and returns the existing view-specific pgerror rather than the generic XX000 parser error.
Evidence Package

Tip

Reply with @itoqa to send us feedback on this test run.

@Hydrocharged
Hydrocharged merged commit e02f66a into main Sep 30, 2026
25 checks passed
@Hydrocharged
Hydrocharged deleted the daylon/issue-3448 branch September 30, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A role with SELECT on a table is refused any CTE: "permission denied for table <cte name>"

3 participants