fix: aggregate result types respect column-level SELECT permission - #653
Open
philipsorensen wants to merge 1 commit into
Open
philipsorensen wants to merge 1 commit into
philipsorensen wants to merge 1 commit into
Conversation
The `*AggregateResult` object types (Sum/Avg/Min/Max) built by
`AggregateNumericType::fields()` listed every aggregatable column on a
table without filtering by `column.permissions.is_selectable`, unlike
every sibling builder — `FilterEntityType`, `OrderByEntityType`, the
Node column builder, `InsertInputType`, `UpdateSetInput` all filter on
the relevant permission flag.
As a result, a role with table-level SELECT but a column-level
`REVOKE SELECT` on a specific column still saw that column's name and
type exposed under `<Table>{Sum,Avg,Min,Max}AggregateResult` during
introspection. This aligns the aggregate builder with the rest of the
schema so a non-selectable column is omitted there too.
Adds `permissions_aggregate_column` regression test covering the
introspection of the aggregate result types under a column-level revoke.
Signed-off-by: Philip Sørensen <git@ppsa.dk>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
AggregateNumericType::fields()(src/graphql.rs) builds the*AggregateResultobject types (Sum/Avg/Min/Max) by iterating every aggregatable column, but — unlike every sibling builder — it does not filter oncolumn.permissions.is_selectable.The other field/type builders all filter on the relevant permission flag:
FilterEntityType::fields()→is_selectableOrderByEntityType::fields()→is_selectableis_selectableInsertInputType/UpdateSetInput→is_insertable/is_updatableBecause the aggregate builder skipped this check, a role with table-level
SELECTbut a column-levelREVOKE SELECTon a specific column still saw that column's name and type under<Table>{Sum,Avg,Min,Max}AggregateResultduring introspection, even though the column is otherwise omitted everywhere else in the schema for that role.Change
Add the
col.permissions.is_selectableguard to the aggregate column loop, matching the sibling builders. A non-selectable column is now omitted from the aggregate result types too.Test
Adds
permissions_aggregate_column, which enables aggregates on a table, revokesSELECTon one column from a role, and introspectsAccountSumAggregateResult/AccountMaxAggregateResultas that role — the revoked column no longer appears, while aggregating a permitted column still works. Full suite (./bin/installcheck, pg17) passes: 123/123.