Skip to content

Conversation

@vadeveka
Copy link
Contributor

@vadeveka vadeveka commented Jul 30, 2025

Why make this change?

Closes #2776
Ensure authorization error thrown if fields in the groupBy argument or in the aggregation function are not allowed for the current role.

What is this change?

During groupBy argument parsing, check if the field is allowed access for current role.
During aggregation function argument parsing, check if the field is allowed access for current role
If no access, then throw authorization error

How was this tested?

  • Integration Tests

Sample Request(s)

Samples from development mode (stack traces will not be show in production mode)
image
image

@vadeveka
Copy link
Contributor Author

/azp run

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This change implements authorization checks for GraphQL aggregation operations to ensure users can only group by and aggregate on fields they have permission to access. When unauthorized fields are used in groupBy arguments or aggregation functions, the system now throws an appropriate authorization error.

  • Added authorization validation for fields in groupBy arguments
  • Added authorization validation for fields in aggregation functions
  • Added new error message constants for aggregation authorization failures

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
GraphQLAuthorizationHandlerTests.cs Added integration tests for unauthorized groupBy and aggregation field access scenarios
SqlQueryStructure.cs Implemented authorization checks in ProcessGroupByField and ProcessAggregations methods
DataApiBuilderException.cs Added new error message constants for groupBy and aggregation authorization failures
mssql-commands.txt Added test role configuration with excluded publisher_id field for aggregation tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@vadeveka
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@vadeveka
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@vadeveka
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@vadeveka
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@vadeveka
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@vadeveka
Copy link
Contributor Author

vadeveka commented Aug 1, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@vadeveka vadeveka enabled auto-merge (squash) August 1, 2025 00:04
@vadeveka
Copy link
Contributor Author

vadeveka commented Aug 1, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@vadeveka vadeveka merged commit 37343c0 into main Aug 1, 2025
11 checks passed
@vadeveka vadeveka deleted the aggregationRbac branch August 1, 2025 01:08
@Aniruddh25 Aniruddh25 added this to the 1.6 milestone Aug 6, 2025
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.

[Bug]: Aggregations does not respect column level security

5 participants