Cherry pick #3756 into release/2.1 - #3794
Open
aaronburtle wants to merge 1 commit into
Open
Conversation
## Why make this change? Closes #3755 ## What is this change? - Replaces claim references in database policies with inert OData parameter aliases. - Keeps claim values separate from policy text in a `ResolvedDatabasePolicy`. - Injects claim values into the parsed OData AST as typed `ConstantNode` values before operand type promotion. - Converts supported primitive claim types to CLR values using invariant parsing and fails closed when a claim does not match its declared type. - Continues using database parameters for SQL predicates and now parameterizes Cosmos DB policy constants instead of inlining them. - Preserves legitimate claim values containing apostrophes, percent characters, or encoded text without decoding or rewriting them. Database policy resolution now returns a `ResolvedDatabasePolicy` rather than a string. This keeps trusted OData policy syntax separate from untrusted claim values: the policy contains inert aliases, while typed values are carried independently and bound during OData AST processing. This prevents URI decoding or escaping behavior from turning claim data into policy syntax. Relevant specification: - [OData 4.01 URL Conventions: Parameter Aliases](https://docs.oasis-open.org/odata/odata/v4.01/cs02/part2-url-conventions/odata-v4.01-cs02-part2-url-conventions.html#sec_ParameterAliases) ```mermaid flowchart TD A["Trusted configured policy<br/>@item.ownerId eq @claims.userId"] B["Untrusted authenticated claim<br/>alice%27 or 1 eq 1 or %27"] A --> C["AuthorizationResolver"] B --> C C --> D["ResolvedDatabasePolicy"] D --> E["Policy:<br/>ownerId eq @dabClaim0"] D --> F["ClaimValues:<br/>@dabClaim0 maps to raw CLR string"] E --> G["ODataParser"] F --> H["ConstantNode map"] H --> G G --> I["ClaimsTypeDataUriResolver<br/>Resolves aliases before type promotion"] I --> J["ParameterAliasRewriter<br/>Resolves remaining aliases and Boolean contexts"] J --> K["Typed FilterClause AST"] K --> L["ODataASTVisitor"] K --> M["ODataASTCosmosVisitor"] L --> N["SQL predicate and provider parameters"] M --> O["Cosmos SQL predicate and provider parameters"] ``` ## How was this tested? - [x] Integration Tests - [x] Unit Tests Focused unit-test coverage includes: - Literal apostrophes in string claims. - Percent-encoded text. - Double-encoded and mixed-encoded text. - Legitimate percent characters. - Typed boolean, integer, floating-point, and null claims. - Malformed primitive claims failing closed. - Cosmos DB policy constants being emitted as query parameters. Integration tests use an authenticated REST test in FindApiTestBase, so it will run against MSSQL, PGSQL, MySQL, and DWSQL, along with a cosmos specific authenticated GQL integration test. ## Sample Request(s) No client-facing request contract changes are introduced. Example database policy: @item.ownerId eq @claims.userId Example request: GET /api/Note Authorization: Bearer <token-with-userId-claim> X-MS-API-ROLE: authenticated A legitimate claim such as `O'Brien` or `50% complete` is preserved exactly and bound as a database parameter. It is never inserted into or reinterpreted as OData policy syntax.
Contributor
There was a problem hiding this comment.
Pull request overview
Cherry-picks #3756 into release/2.1 to harden database-policy claim handling by keeping untrusted claim values out of OData policy text and binding them as typed constants/parameters during OData AST processing, including Cosmos DB parameterization.
Changes:
- Replace
ProcessDBPolicy(string substitution) withResolveDBPolicyreturning aResolvedDatabasePolicy(inert aliases + typed claim values). - Bind claim values via OData parameter aliases/AST rewriting and parameterize Cosmos DB policy constants (no inline string literals).
- Add/extend unit + integration coverage and update test configs/snapshots for a new
claim_policy_testerrole.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Service.Tests/UnitTests/EdmModelBuilderTests.cs | Adds unit coverage for Cosmos @model(name: ...) alias being available as an OData entity set and for alias binding in filters. |
| src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs | Updates mocks to the new ResolveDBPolicy API returning ResolvedDatabasePolicy.Empty. |
| src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs | New end-to-end unit tests validating typed claim binding through SQL and Cosmos policy pipelines. |
| src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs | Adds authenticated REST integration test ensuring encoded quotes in claims remain data (not policy syntax). |
| src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForPostgreSql.verified.txt | Snapshot update adding claim_policy_tester permissions/policy. |
| src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForMySql.verified.txt | Snapshot update adding claim_policy_tester permissions/policy. |
| src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForMsSql.verified.txt | Snapshot update adding claim_policy_tester permissions/policy. |
| src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForCosmos.verified.txt | Snapshot update adding claim_policy_tester permissions/policy (Cosmos variant). |
| src/Service.Tests/dab-config.PostgreSql.json | Adds claim_policy_tester role with database policy using @claims.userId. |
| src/Service.Tests/dab-config.MySql.json | Adds claim_policy_tester role with database policy using @claims.userId. |
| src/Service.Tests/dab-config.MsSql.json | Adds claim_policy_tester role with database policy using @claims.userId. |
| src/Service.Tests/dab-config.DwSql.json | Adds claim_policy_tester role with database policy using @claims.userId. |
| src/Service.Tests/dab-config.CosmosDb_NoSql.json | Adds claim_policy_tester role with database policy using @claims.userId (Cosmos). |
| src/Service.Tests/CosmosTests/QueryFilterTests.cs | Adds authenticated GraphQL integration test for Cosmos ensuring encoded-quote claims remain bound values. |
| src/Service.Tests/Caching/DabCacheServiceIntegrationTests.cs | Updates authorization resolver mocks for ResolveDBPolicy. |
| src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs | Updates assertions to use ResolvedDatabasePolicy.Policy. |
| src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs | Updates/extends tests for parameter-alias based claim handling, typed parsing, and fail-closed behavior. |
| src/Core/Resolvers/CosmosQueryStructure.cs | Removes redundant MakeDbConnectionParam override (now relies on base implementation). |
| src/Core/Resolvers/AuthorizationPolicyHelpers.cs | Switches to ResolvedDatabasePolicy, binds parameter aliases into OData parsing, and fixes Cosmos root path prefixing. |
| src/Core/Parsers/ParameterAliasRewriter.cs | New rewriter to replace parameter aliases post-parse and normalize boolean predicate positions. |
| src/Core/Parsers/ODataASTCosmosVisitor.cs | Parameterizes Cosmos policy constants using query-structure parameters (no inline quoting). |
| src/Core/Parsers/FilterParser.cs | Extends GetFilterClause to accept parameter alias nodes and applies alias rewriting when provided. |
| src/Core/Parsers/EdmModelBuilder.cs | Adds entity sets for Cosmos @model(name: ...) aliases so policies can parse against configured entity names. |
| src/Core/Parsers/ClaimsTypeDataUriResolver.cs | Resolves claim aliases before operand type promotion to keep claim data out of URI text. |
| src/Core/Authorization/AuthorizationResolver.cs | Introduces typed claim parsing (invariant) + fail-closed validation; returns ResolvedDatabasePolicy with inert aliases. |
| src/Auth/ResolvedDatabasePolicy.cs | New type representing policy text + immutable typed claim-value snapshot. |
| src/Auth/IAuthorizationResolver.cs | Updates interface to expose ResolveDBPolicy returning ResolvedDatabasePolicy. |
| config-generators/postgresql-commands.txt | Adds generator command for claim_policy_tester database policy. |
| config-generators/mysql-commands.txt | Adds generator command for claim_policy_tester database policy. |
| config-generators/mssql-commands.txt | Adds generator command for claim_policy_tester database policy. |
| config-generators/dwsql-commands.txt | Adds generator command for claim_policy_tester database policy. |
| config-generators/cosmosdb_nosql-commands.txt | Adds generator command for claim_policy_tester database policy (Cosmos). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+444
to
+450
| private static void AssertParameterValues(BaseQueryStructure structure, params object?[] expectedValues) | ||
| { | ||
| object?[] actualValues = structure.Parameters.Values | ||
| .Select(parameter => parameter.Value) | ||
| .ToArray(); | ||
| CollectionAssert.AreEqual(expectedValues, actualValues); | ||
| } |
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
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.
Why make this change?
Closes #3793
What is this change?
Cherry picks #3756 into the
release/2.1branch.How was this tested?
Sample Request(s)
N/A