From f6d2c4a2c3943b48fee2135e02c6f56d16af6b57 Mon Sep 17 00:00:00 2001 From: Aaron Burtle Date: Wed, 5 Aug 2026 12:53:27 -0700 Subject: [PATCH 1/4] safely inject claims --- src/Auth/IAuthorizationResolver.cs | 7 +- src/Auth/ResolvedDatabasePolicy.cs | 23 ++++ .../Authorization/AuthorizationResolver.cs | 128 ++++++++++++------ src/Core/Parsers/ClaimsTypeDataUriResolver.cs | 23 ++++ src/Core/Parsers/FilterParser.cs | 15 +- src/Core/Parsers/ODataASTCosmosVisitor.cs | 7 +- .../Resolvers/AuthorizationPolicyHelpers.cs | 27 ++-- .../AuthorizationResolverUnitTests.cs | 116 ++++++++++------ .../REST/RestAuthorizationHandlerUnitTests.cs | 1 + .../DatabasePolicyClaimBindingUnitTests.cs | 85 ++++++++++++ .../UnitTests/DwSqlQueryBuilderUpsertTests.cs | 2 +- 11 files changed, 334 insertions(+), 100 deletions(-) create mode 100644 src/Auth/ResolvedDatabasePolicy.cs create mode 100644 src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs diff --git a/src/Auth/IAuthorizationResolver.cs b/src/Auth/IAuthorizationResolver.cs index 3a961ece4d..72ec1983fc 100644 --- a/src/Auth/IAuthorizationResolver.cs +++ b/src/Auth/IAuthorizationResolver.cs @@ -80,8 +80,11 @@ public interface IAuthorizationResolver /// Role defined in client role header. /// Operation type: Create, Read, Update, Delete. /// Contains token claims of the authenticated user used in policy evaluation. - /// Returns the parsed policy, if successfully processed, or an exception otherwise. - public string ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext); + /// + /// Returns policy text containing parameter aliases together with the typed claim values + /// that must be bound to those aliases, or an exception when processing fails. + /// + public ResolvedDatabasePolicy ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext); /// /// Get list of roles defined for entity within runtime configuration.. This is applicable for GraphQL when creating authorization diff --git a/src/Auth/ResolvedDatabasePolicy.cs b/src/Auth/ResolvedDatabasePolicy.cs new file mode 100644 index 0000000000..83ffefe104 --- /dev/null +++ b/src/Auth/ResolvedDatabasePolicy.cs @@ -0,0 +1,23 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +namespace Azure.DataApiBuilder.Auth; + +/// +/// A database authorization policy whose claim references have been replaced by +/// OData parameter aliases. Claim values remain separate from the policy text so +/// they can be injected into the parsed OData AST as typed constants. +/// +/// Policy text containing OData parameter aliases. +/// Typed claim values keyed by their parameter alias. +public sealed record ResolvedDatabasePolicy( + string Policy, + IReadOnlyDictionary ClaimValues) +{ + /// + /// Represents an operation without a database authorization policy. + /// + public static ResolvedDatabasePolicy Empty { get; } = new( + string.Empty, + new Dictionary()); +} diff --git a/src/Core/Authorization/AuthorizationResolver.cs b/src/Core/Authorization/AuthorizationResolver.cs index 205dc3d646..39a6671f42 100644 --- a/src/Core/Authorization/AuthorizationResolver.cs +++ b/src/Core/Authorization/AuthorizationResolver.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using System.Globalization; using System.Net; using System.Security.Claims; using System.Text.Json; @@ -205,13 +206,13 @@ public bool AreColumnsAllowedForOperation(string entityName, string roleName, En } /// - public string ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext) + public ResolvedDatabasePolicy ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext) { string dBpolicyWithClaimTypes = GetDBPolicyForRequest(entityName, roleName, operation); if (string.IsNullOrWhiteSpace(dBpolicyWithClaimTypes)) { - return string.Empty; + return ResolvedDatabasePolicy.Empty; } return GetPolicyWithClaimValues(dBpolicyWithClaimTypes, GetAllAuthenticatedUserClaims(httpContext)); @@ -759,26 +760,37 @@ public static Dictionary> GetAllAuthenticatedUserClaims(Http } /// - /// Helper method to substitute all the claimTypes(denoted with @claims.claimType) in - /// the policy string with their corresponding claimValues. + /// Replaces all claim references (denoted with @claims.claimType) in the policy + /// with OData parameter aliases and returns their typed values separately. + /// Claim values must never be inserted into URI text because URI parsing can decode + /// percent-encoded syntax after string escaping has already occurred. /// /// The policy to be processed. /// Dictionary holding all the claims available in the request. - /// Processed policy with claim values substituted for claim types. + /// Policy text containing aliases and the typed values bound to those aliases. /// - private static string GetPolicyWithClaimValues(string policy, Dictionary> claimsInRequestContext) + private static ResolvedDatabasePolicy GetPolicyWithClaimValues(string policy, Dictionary> claimsInRequestContext) { // Regex used to extract all claimTypes in policy. It finds all the substrings which are // of the form @claims.*** where *** contains characters from a-zA-Z0-9._ . string claimCharsRgx = @"@claims\.[a-zA-Z0-9_\.]*"; - // Find all the claimTypes from the policy + Dictionary claimValues = new(); + int claimIndex = 0; + + // Replace claim references with inert OData aliases. The raw values remain out of + // the policy URI and are later injected directly into the parsed AST. string processedPolicy = Regex.Replace(policy, claimCharsRgx, - (claimTypeMatch) => GetClaimValueFromClaim(claimTypeMatch, claimsInRequestContext)); + (claimTypeMatch) => + { + string claimAlias = $"@dabClaim{claimIndex++}"; + claimValues.Add(claimAlias, GetClaimValueFromClaim(claimTypeMatch, claimsInRequestContext)); + return claimAlias; + }); // Remove occurrences of @item. directives processedPolicy = processedPolicy.Replace(FIELD_PREFIX, ""); - return processedPolicy; + return new ResolvedDatabasePolicy(processedPolicy, claimValues); } /// @@ -786,9 +798,9 @@ private static string GetPolicyWithClaimValues(string policy, Dictionary /// The claimType present in policy with a prefix of @claims.. /// Dictionary populated with all the user claims. - /// The claim value of the first claim whose claimType matches 'claimTypeMatch'. + /// The typed value of the first claim whose claimType matches 'claimTypeMatch'. /// Throws exception when the user does not possess the given claim. - private static string GetClaimValueFromClaim(Match claimTypeMatch, Dictionary> claimsInRequestContext) + private static object? GetClaimValueFromClaim(Match claimTypeMatch, Dictionary> claimsInRequestContext) { // Gets from @claims. string claimType = claimTypeMatch.Value.ToString().Substring(CLAIM_PREFIX.Length); @@ -815,13 +827,12 @@ private static string GetClaimValueFromClaim(Match claimTypeMatch, Dictionary - /// Using the input parameter claim, returns the primitive literal from claim.Value: - /// e.g. @claims.idp (string) resolves as 'azuread' + /// Using the input parameter claim, returns the typed primitive value from claim.Value: + /// e.g. @claims.idp (string) resolves as azuread /// e.g. @claims.iat (int) resolves as 1537231048 /// e.g. @claims.email_verified (boolean) resolves as true - /// To adhere with OData 4.01 ABNF construction rules (Section 7: Literal Data Values) - /// - Primitive string literals in URLS must be enclosed within single quotes. - /// - Other primitive types are represented as plain values and do not require single quotes. + /// Values are returned as CLR primitives so the policy parser can bind them as typed + /// OData AST constants without serializing them into URI text. /// Note: With many access token issuers, token claims are strings or string representations /// of other data types such as dates and GUIDs. /// Note: System.Security.Claim.ValueType defaults to ClaimValueTypes.String if the code calling @@ -834,7 +845,7 @@ private static string GetClaimValueFromClaim(Match claimTypeMatch, Dictionary /// /// - private static string GetClaimValue(Claim claim) + private static object? GetClaimValue(Claim claim) { /* An example Claim object: * claim.Type: "user_email" @@ -842,33 +853,64 @@ private static string GetClaimValue(Claim claim) * claim.ValueType: "http://www.w3.org/2001/XMLSchema#string" */ - switch (claim.ValueType) - { - case ClaimValueTypes.String: - // Escape embedded single quotes per OData 4.01 ABNF (Section 7: Literal Data Values) - // by doubling them. This prevents an attacker-influenced claim value from breaking - // out of the string literal and injecting additional OData predicates into the - // database authorization policy expression. - // See: http://docs.oasis-open.org/odata/odata/v4.01/cs01/abnf/odata-abnf-construction-rules.txt - return $"'{claim.Value.Replace("'", "''")}'"; - case ClaimValueTypes.Boolean: - case ClaimValueTypes.Integer: - case ClaimValueTypes.Integer32: - case ClaimValueTypes.Integer64: - case ClaimValueTypes.UInteger32: - case ClaimValueTypes.UInteger64: - case ClaimValueTypes.Double: - return $"{claim.Value}"; - case JsonClaimValueTypes.JsonNull: - return $"null"; - default: - // One of the claims in the request had unsupported data type. - throw new DataApiBuilderException( - message: $"The claim value for claim: {claim.Type} belonging to the user has an unsupported data type.", - statusCode: HttpStatusCode.Forbidden, - subStatusCode: DataApiBuilderException.SubStatusCodes.UnsupportedClaimValueType - ); + try + { + switch (claim.ValueType) + { + case ClaimValueTypes.String: + return claim.Value; + case ClaimValueTypes.Boolean: + return bool.Parse(claim.Value); + case ClaimValueTypes.Integer: + return ParseIntegerClaimValue(claim.Value); + case ClaimValueTypes.Integer32: + return int.Parse(claim.Value, NumberStyles.Integer, CultureInfo.InvariantCulture); + case ClaimValueTypes.Integer64: + return long.Parse(claim.Value, NumberStyles.Integer, CultureInfo.InvariantCulture); + case ClaimValueTypes.UInteger32: + return (long)uint.Parse(claim.Value, NumberStyles.Integer, CultureInfo.InvariantCulture); + case ClaimValueTypes.UInteger64: + return (decimal)ulong.Parse(claim.Value, NumberStyles.Integer, CultureInfo.InvariantCulture); + case ClaimValueTypes.Double: + return double.Parse(claim.Value, NumberStyles.Float, CultureInfo.InvariantCulture); + case JsonClaimValueTypes.JsonNull: + return null; + default: + // One of the claims in the request had unsupported data type. + throw CreateUnsupportedClaimValueException(claim); + } + } + catch (Exception ex) when (ex is FormatException || ex is OverflowException) + { + throw CreateUnsupportedClaimValueException(claim, ex); + } + } + + private static DataApiBuilderException CreateUnsupportedClaimValueException(Claim claim, Exception? innerException = null) + { + return new DataApiBuilderException( + message: $"The claim value for claim: {claim.Type} belonging to the user has an unsupported data type.", + statusCode: HttpStatusCode.Forbidden, + subStatusCode: DataApiBuilderException.SubStatusCodes.UnsupportedClaimValueType, + innerException: innerException); + } + + /// + /// Parses an XML Schema integer claim into the narrowest OData-supported CLR integer type. + /// + private static object ParseIntegerClaimValue(string value) + { + if (int.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out int intValue)) + { + return intValue; + } + + if (long.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out long longValue)) + { + return longValue; } + + return decimal.Parse(value, NumberStyles.Integer, CultureInfo.InvariantCulture); } /// diff --git a/src/Core/Parsers/ClaimsTypeDataUriResolver.cs b/src/Core/Parsers/ClaimsTypeDataUriResolver.cs index c8ef174ecd..d1fdf6dc40 100644 --- a/src/Core/Parsers/ClaimsTypeDataUriResolver.cs +++ b/src/Core/Parsers/ClaimsTypeDataUriResolver.cs @@ -15,6 +15,13 @@ namespace Azure.DataApiBuilder.Core.Parsers /// public class ClaimsTypeDataUriResolver : ODataUriResolver { + private readonly IReadOnlyDictionary _claimValueNodes; + + public ClaimsTypeDataUriResolver(IReadOnlyDictionary? claimValueNodes = null) + { + _claimValueNodes = claimValueNodes ?? new Dictionary(); + } + /// /// Between two nodes in the filter clause, determine the: /// - PrimaryOperand: Node representing an OData EDM model object and has Kind == QueryNodeKind.SingleValuePropertyAccess. @@ -27,6 +34,9 @@ public class ClaimsTypeDataUriResolver : ODataUriResolver /// type reference for the result BinaryOperatorNode. public override void PromoteBinaryOperandTypes(BinaryOperatorKind binaryOperatorKind, ref SingleValueNode leftNode, ref SingleValueNode rightNode, out IEdmTypeReference typeReference) { + ResolveClaimAlias(ref leftNode); + ResolveClaimAlias(ref rightNode); + if (leftNode.TypeReference.PrimitiveKind() != rightNode.TypeReference.PrimitiveKind()) { if ((leftNode.Kind == QueryNodeKind.SingleValuePropertyAccess) && (rightNode is ConstantNode)) @@ -48,6 +58,19 @@ public override void PromoteBinaryOperandTypes(BinaryOperatorKind binaryOperator base.PromoteBinaryOperandTypes(binaryOperatorKind, ref leftNode, ref rightNode, out typeReference); } + /// + /// Replaces a policy parameter alias with its typed claim constant before OData + /// performs type promotion. The claim value therefore never enters URI text. + /// + private void ResolveClaimAlias(ref SingleValueNode node) + { + if (node is ParameterAliasNode aliasNode && + _claimValueNodes.TryGetValue(aliasNode.Alias, out SingleValueNode? claimValueNode)) + { + node = claimValueNode; + } + } + /// /// Uses type specific parsers to attempt converting the supplied node to a new ConstantNode of type targetType /// when the supplied node's type differs from the target's type. diff --git a/src/Core/Parsers/FilterParser.cs b/src/Core/Parsers/FilterParser.cs index c9cfc1eb53..dbad26ba20 100644 --- a/src/Core/Parsers/FilterParser.cs +++ b/src/Core/Parsers/FilterParser.cs @@ -39,8 +39,13 @@ public void BuildModel(DocumentNode graphQLSchemaRoot) /// Represents the $filter part of the query string /// Represents the resource path, in our case the entity name. /// ODataUriResolver resolving different kinds of Uri parsing context. + /// Typed AST values for parameter aliases referenced by the filter. /// An AST FilterClause that represents the filter portion of the WHERE clause. - public FilterClause GetFilterClause(string filterQueryString, string resourcePath, ODataUriResolver? customResolver = null) + public FilterClause GetFilterClause( + string filterQueryString, + string resourcePath, + ODataUriResolver? customResolver = null, + IReadOnlyDictionary? parameterAliasNodes = null) { if (_model is null) { @@ -60,6 +65,14 @@ public FilterClause GetFilterClause(string filterQueryString, string resourcePat parser.Resolver = customResolver; } + if (parameterAliasNodes is not null) + { + foreach ((string alias, SingleValueNode valueNode) in parameterAliasNodes) + { + parser.ParameterAliasNodes.Add(alias, valueNode); + } + } + return parser.ParseFilter(); } catch (ODataException e) diff --git a/src/Core/Parsers/ODataASTCosmosVisitor.cs b/src/Core/Parsers/ODataASTCosmosVisitor.cs index 1fca34d624..f32a60c1a5 100644 --- a/src/Core/Parsers/ODataASTCosmosVisitor.cs +++ b/src/Core/Parsers/ODataASTCosmosVisitor.cs @@ -12,14 +12,17 @@ namespace Azure.DataApiBuilder.Core.Parsers internal class ODataASTCosmosVisitor : QueryNodeVisitor { private string _prefix; + private readonly Func _makeDbConnectionParam; /// /// Constructor for the visitor to append prefix to the column names which would be the path from container to the column /// /// - public ODataASTCosmosVisitor(string prefix) + /// Creates a bound Cosmos DB query parameter. + public ODataASTCosmosVisitor(string prefix, Func makeDbConnectionParam) { this._prefix = prefix; + _makeDbConnectionParam = makeDbConnectionParam; } /// @@ -123,7 +126,7 @@ public override string Visit(ConstantNode nodeIn) return "NULL"; } - return $"'{nodeIn.Value}'"; + return _makeDbConnectionParam(nodeIn.Value); } /// diff --git a/src/Core/Resolvers/AuthorizationPolicyHelpers.cs b/src/Core/Resolvers/AuthorizationPolicyHelpers.cs index 58f4d1461d..093e8eefeb 100644 --- a/src/Core/Resolvers/AuthorizationPolicyHelpers.cs +++ b/src/Core/Resolvers/AuthorizationPolicyHelpers.cs @@ -105,13 +105,17 @@ public static void ProcessAuthorizationPolicies( cosmosQueryStructure.TableCounter.Next(); fromClause = pathConfig.JoinStatement; - predicates = filterClause?.Expression.Accept(new ODataASTCosmosVisitor(pathConfig.Alias)); + predicates = filterClause?.Expression.Accept(new ODataASTCosmosVisitor( + pathConfig.Alias, + value => cosmosQueryStructure.MakeDbConnectionParam(value))); existQuery = CosmosQueryBuilder.BuildExistsQueryForCosmos(fromClause, predicates); } else { - predicates = filterClause?.Expression.Accept(new ODataASTCosmosVisitor($"{pathConfig.Path}.{pathConfig.ColumnName}")); + predicates = filterClause?.Expression.Accept(new ODataASTCosmosVisitor( + $"{pathConfig.Path}.{pathConfig.ColumnName}", + value => cosmosQueryStructure.MakeDbConnectionParam(value))); } if (pathConfig.EntityName == entity.Key) @@ -160,7 +164,7 @@ private static List ProcessFilter( List filterClauses = new(); foreach (EntityActionOperation elementalOperation in elementalOperations) { - string dbQueryPolicy = authorizationResolver.ProcessDBPolicy( + ResolvedDatabasePolicy dbQueryPolicy = authorizationResolver.ProcessDBPolicy( entityName, clientRoleHeader, elementalOperation, @@ -179,31 +183,36 @@ private static List ProcessFilter( } /// - /// Given a dbPolicyClause string, appends the string formatting needed to be processed by ODataParser + /// Appends the filter query formatting to a resolved database policy and parses it with ODataParser. /// - /// string representation of a processed database authorization policy. + /// Database authorization policy text and separately bound claim values. /// Name of the entity. /// Name of the schema. e.g. `dbo` for MsSql. /// Provides helper method to process ODataFilterClause. public static FilterClause? GetDBPolicyClauseForQueryStructure( - string dbPolicyClause, + ResolvedDatabasePolicy dbPolicy, string entityName, string resourcePath, ISqlMetadataProvider sqlMetadataProvider) { - if (!string.IsNullOrEmpty(dbPolicyClause)) + if (!string.IsNullOrEmpty(dbPolicy.Policy)) { + Dictionary claimValueNodes = dbPolicy.ClaimValues.ToDictionary( + claimValue => claimValue.Key, + claimValue => (SingleValueNode)new ConstantNode(claimValue.Value)); + // Since dbPolicy is nothing but filters to be added by virtue of database policy, we prefix it with // ?$filter= so that it conforms with the format followed by other filter predicates. // This enables the ODataVisitor helpers to parse the policy text properly. - dbPolicyClause = $"?{RequestParser.FILTER_URL}={dbPolicyClause}"; + string dbPolicyClause = $"?{RequestParser.FILTER_URL}={dbPolicy.Policy}"; // Parse and save the values that are needed to later generate SQL query predicates // FilterClauseInDbPolicy is an Abstract Syntax Tree representing the parsed policy text. return sqlMetadataProvider.GetODataParser().GetFilterClause( filterQueryString: dbPolicyClause, resourcePath: resourcePath, - customResolver: new ClaimsTypeDataUriResolver()); + customResolver: new ClaimsTypeDataUriResolver(claimValueNodes), + parameterAliasNodes: claimValueNodes); } return null; diff --git a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs index a40839f8e8..c61031f722 100644 --- a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs +++ b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs @@ -1286,14 +1286,14 @@ public void AreColumnsAllowedForOperationWithRoleWithDifferentCasing( /// The policy which is expected to be generated after parsing. [DataTestMethod] [DataRow("@claims.user_email ne @item.col1 and @claims.contact_no eq @item.col2 and not(@claims.name eq @item.col3)", - "'xyz@microsoft.com' ne col1 and 1234 eq col2 and not('Aaron' eq col3)", + "@dabClaim0 ne col1 and @dabClaim1 eq col2 and not(@dabClaim2 eq col3)", DisplayName = "Valid policy parsing test for string and int64 claimvaluetypes.")] [DataRow("(@claims.isemployee eq @item.col1 and @item.col2 ne @claims.user_email) or" + - "('David' ne @item.col3 and @claims.contact_no ne @item.col3)", "(true eq col1 and col2 ne 'xyz@microsoft.com') or" + - "('David' ne col3 and 1234 ne col3)", DisplayName = "Valid policy parsing test for constant string and int64 claimvaluetypes.")] + "('David' ne @item.col3 and @claims.contact_no ne @item.col3)", "(@dabClaim0 eq col1 and col2 ne @dabClaim1) or" + + "('David' ne col3 and @dabClaim2 ne col3)", DisplayName = "Valid policy parsing test for constant string and int64 claimvaluetypes.")] [DataRow("(@item.rating gt @claims.emprating) and (@claims.isemployee eq true)", - "(rating gt 4.2) and (true eq true)", DisplayName = "Valid policy parsing test for double and boolean claimvaluetypes.")] - [DataRow("@item.rating eq @claims.emprating)", "rating eq 4.2)", DisplayName = "Valid policy parsing test for double claimvaluetype.")] + "(rating gt @dabClaim0) and (@dabClaim1 eq true)", DisplayName = "Valid policy parsing test for double and boolean claimvaluetypes.")] + [DataRow("@item.rating eq @claims.emprating)", "rating eq @dabClaim0)", DisplayName = "Valid policy parsing test for double claimvaluetype.")] public void ParseValidDbPolicy(string policy, string expectedParsedPolicy) { RuntimeConfig runtimeConfig = InitRuntimeConfig( @@ -1317,40 +1317,33 @@ public void ParseValidDbPolicy(string policy, string expectedParsedPolicy) context.Setup(x => x.User).Returns(principal); context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); - string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); - Assert.AreEqual(parsedPolicy, expectedParsedPolicy); + ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + Assert.AreEqual(parsedPolicy.Policy, expectedParsedPolicy); } /// - /// Validates that single quote characters embedded in a string-typed claim value are - /// escaped (doubled) per OData 4.01 ABNF when substituted into a database authorization - /// policy. Without escaping, an attacker who can influence a referenced JWT claim could - /// break out of the string literal and inject additional OData predicates - bypassing - /// row-level authorization. The substituted claim must remain enclosed in a single - /// string literal regardless of its contents. + /// Validates that a string claim is kept out of database authorization policy text and + /// associated with an inert OData parameter alias. This prevents claim contents from + /// being reinterpreted as policy syntax during URI parsing. /// /// The raw claim value (as it appears in the JWT) to substitute. - /// The parsed policy after safe substitution. [DataTestMethod] [DataRow( "alice' or 1 eq 1 or '", - "col1 eq 'alice'' or 1 eq 1 or '''", - DisplayName = "Injection attempt with OR predicate is neutralized by escaping single quotes")] + DisplayName = "Literal quote injection remains outside policy text")] [DataRow( "O'Brien", - "col1 eq 'O''Brien'", - DisplayName = "Legitimate single-quote-bearing value (e.g. surname) is safely escaped")] + DisplayName = "Legitimate single-quote-bearing value remains unchanged")] [DataRow( - "''", - "col1 eq ''''''", - DisplayName = "Value composed solely of single quotes is fully escaped")] + "alice%27 or 1 eq 1 or %27", + DisplayName = "Encoded quote injection remains outside policy text")] [DataRow( - "no quotes here", - "col1 eq 'no quotes here'", - DisplayName = "Value without single quotes is unchanged aside from enclosing quotes")] - public void DbPolicy_StringClaim_SingleQuotesEscaped_PreventsODataInjection( - string claimValue, - string expectedParsedPolicy) + "alice%2527 or 1 eq 1 or %2527", + DisplayName = "Double-encoded quote injection remains outside policy text")] + [DataRow( + "50% complete", + DisplayName = "Legitimate percent characters remain unchanged")] + public void DbPolicy_StringClaim_UsesTypedParameterAlias(string claimValue) { const string policyDefinition = "@item.col1 eq @claims.userId"; @@ -1370,9 +1363,10 @@ public void DbPolicy_StringClaim_SingleQuotesEscaped_PreventsODataInjection( context.Setup(x => x.User).Returns(principal); context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); - string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); - Assert.AreEqual(expectedParsedPolicy, parsedPolicy); + Assert.AreEqual("col1 eq @dabClaim0", parsedPolicy.Policy); + Assert.AreEqual(claimValue, parsedPolicy.ClaimValues["@dabClaim0"]); } /// @@ -1403,11 +1397,7 @@ public void DbPolicy_StringClaim_SingleQuotesEscaped_PreventsODataInjection( #pragma warning restore format public void DbPolicy_ClaimValueTypeParsing(string claimValueType, string claimValue, bool supportedValueType) { - // To adhere with OData 4 ABNF construction rules (Section 7: Literal Data Values) - // - Primitive string literals in URLS must be enclosed within single quotes. - // - http://docs.oasis-open.org/odata/odata/v4.01/cs01/abnf/odata-abnf-construction-rules.txt - string odataClaimValue = (claimValueType == ClaimValueTypes.String) ? "'" + claimValue + "'" : claimValue; - string expectedPolicy = odataClaimValue + " eq col1"; + string expectedPolicy = "@dabClaim0 eq col1"; string policyDefinition = "@claims.testClaim eq @item.col1"; RuntimeConfig runtimeConfig = InitRuntimeConfig( @@ -1431,9 +1421,21 @@ public void DbPolicy_ClaimValueTypeParsing(string claimValueType, string claimVa try { - string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); Assert.IsTrue(supportedValueType); - Assert.AreEqual(expectedPolicy, parsedPolicy); + Assert.AreEqual(expectedPolicy, parsedPolicy.Policy); + + object? typedClaimValue = parsedPolicy.ClaimValues["@dabClaim0"]; + if (claimValueType == JsonClaimValueTypes.JsonNull) + { + Assert.IsNull(typedClaimValue); + } + else + { + Assert.AreEqual( + claimValue.ToLowerInvariant(), + Convert.ToString(typedClaimValue, System.Globalization.CultureInfo.InvariantCulture)?.ToLowerInvariant()); + } } catch (DataApiBuilderException ex) { @@ -1446,6 +1448,34 @@ public void DbPolicy_ClaimValueTypeParsing(string claimValueType, string claimVa } } + /// + /// A claim whose value does not match its declared primitive type must fail before + /// policy parsing and must never be interpreted as OData syntax. + /// + [TestMethod] + public void DbPolicy_MalformedPrimitiveClaim_FailsClosed() + { + RuntimeConfig runtimeConfig = InitRuntimeConfig( + entityName: TEST_ENTITY, + roleName: TEST_ROLE, + operation: TEST_OPERATION, + includedCols: new HashSet { "col1" }, + databasePolicy: "@claims.testClaim eq @item.col1"); + AuthorizationResolver authZResolver = AuthorizationHelpers.InitAuthorizationResolver(runtimeConfig); + + Mock context = new(); + ClaimsIdentity identity = new(TEST_AUTHENTICATION_TYPE, TEST_CLAIMTYPE_NAME, AuthenticationOptions.ROLE_CLAIM_TYPE); + identity.AddClaim(new Claim("testClaim", "1 or 1 eq 1", ClaimValueTypes.Integer)); + context.Setup(x => x.User).Returns(new ClaimsPrincipal(identity)); + context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); + + DataApiBuilderException exception = Assert.ThrowsException(() => + authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object)); + + Assert.AreEqual(HttpStatusCode.Forbidden, exception.StatusCode); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.UnsupportedClaimValueType, exception.SubStatusCode); + } + /// /// Test to validate that we are correctly throwing an appropriate exception when the user request /// lacks a claim required by the policy. @@ -1531,11 +1561,13 @@ public void ParsePolicyWithDuplicateUserClaims() context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); // Act - string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); // Assert - string expectedPolicy = $"'profile' eq col2 and '1111' eq col3"; - Assert.AreEqual(expected: expectedPolicy, actual: parsedPolicy); + string expectedPolicy = "@dabClaim0 eq col2 and @dabClaim1 eq col3"; + Assert.AreEqual(expected: expectedPolicy, actual: parsedPolicy.Policy); + Assert.AreEqual("profile", parsedPolicy.ClaimValues["@dabClaim0"]); + Assert.AreEqual("1111", parsedPolicy.ClaimValues["@dabClaim1"]); } // Indirectly tests the AuthorizationResolver private method: @@ -1583,15 +1615,15 @@ public void GetDBPolicyTest( context.Setup(x => x.User).Returns(principal); context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(clientRole); - string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object); + ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object); string errorMessage = "TryProcessDBPolicy returned unexpected value."; if (expectPolicy) { - Assert.AreEqual(actual: parsedPolicy, expected: policy, message: errorMessage); + Assert.AreEqual(actual: parsedPolicy.Policy, expected: policy, message: errorMessage); } else { - Assert.AreEqual(actual: parsedPolicy, expected: string.Empty, message: errorMessage); + Assert.AreEqual(actual: parsedPolicy.Policy, expected: string.Empty, message: errorMessage); } } diff --git a/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs b/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs index d6e7d55bfe..69f71368dd 100644 --- a/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs +++ b/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs @@ -104,6 +104,7 @@ public void TestWildcardPolicyResolvesToEmpty(string httpMethod) roleName: "admin", operation: RestService.HttpVerbToOperations(httpVerbName: httpMethod), httpContext: httpContext) + .Policy ); } diff --git a/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs b/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs new file mode 100644 index 0000000000..417aa87e64 --- /dev/null +++ b/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs @@ -0,0 +1,85 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using Azure.DataApiBuilder.Core.Parsers; +using Microsoft.OData.Edm; +using Microsoft.OData.UriParser; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Azure.DataApiBuilder.Service.Tests.UnitTests +{ + /// + /// Tests that database policy claim values are bound as typed AST constants and + /// never interpreted as URI/OData syntax. + /// + [TestClass] + public class DatabasePolicyClaimBindingUnitTests + { + /// + /// Verifies that attacker-influenced claim values never enter the policy URI. The OData + /// parser must see one equality expression whose right operand is the original claim value, + /// regardless of percent encoding, and Cosmos DB must receive that value as a parameter. + /// + [DataTestMethod] + [DataRow("alice%27 or 1 eq 1 or %27", DisplayName = "Percent-encoded quote")] + [DataRow("alice%2527 or 1 eq 1 or %2527", DisplayName = "Double-encoded quote")] + [DataRow("alice%252527 or 1 eq 1 or %27", DisplayName = "Mixed nested encodings")] + [DataRow("alice' or 1 eq 1 or '", DisplayName = "Literal quote")] + [DataRow("50% complete", DisplayName = "Legitimate percent character")] + public void DatabasePolicyClaimValue_IsTypedAstConstantAndCosmosParameter(string claimValue) + { + const string claimAlias = "@dabClaim0"; + Dictionary claimValueNodes = new() + { + [claimAlias] = new ConstantNode(claimValue) + }; + + ODataUriParser parser = new( + BuildModel(), + new Uri($"Entities/?$filter=name eq {claimAlias}", UriKind.Relative)) + { + Resolver = new ClaimsTypeDataUriResolver(claimValueNodes) + }; + + foreach ((string alias, SingleValueNode valueNode) in claimValueNodes) + { + parser.ParameterAliasNodes.Add(alias, valueNode); + } + + FilterClause clause = parser.ParseFilter(); + BinaryOperatorNode comparison = (BinaryOperatorNode)clause.Expression; + Assert.AreEqual(BinaryOperatorKind.Equal, comparison.OperatorKind); + Assert.IsInstanceOfType(comparison.Right); + Assert.AreEqual(claimValue, ((ConstantNode)comparison.Right).Value); + + List parameterValues = new(); + string predicate = clause.Expression.Accept(new ODataASTCosmosVisitor( + "c", + value => + { + parameterValues.Add(value); + return "@param1"; + })); + + Assert.AreEqual("(c.name = @param1)", predicate); + CollectionAssert.AreEqual(new object?[] { claimValue }, parameterValues); + } + + private static IEdmModel BuildModel() + { + EdmModel model = new(); + EdmEntityType entityType = new("Dab", "Entity"); + EdmStructuralProperty id = entityType.AddStructuralProperty("id", EdmPrimitiveTypeKind.Int32, false); + entityType.AddKeys(id); + entityType.AddStructuralProperty("name", EdmPrimitiveTypeKind.String, true); + model.AddElement(entityType); + + EdmEntityContainer container = new("Dab", "Container"); + container.AddEntitySet("Entities", entityType); + model.AddElement(container); + return model; + } + } +} diff --git a/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs b/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs index 3d70162832..8a8a5f2341 100644 --- a/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs +++ b/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs @@ -144,7 +144,7 @@ private static SqlUpsertQueryStructure CreateUpsertStructure() Mock authorizationResolver = new(); authorizationResolver .Setup(x => x.ProcessDBPolicy(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns(string.Empty); + .Returns(ResolvedDatabasePolicy.Empty); RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestHelper.GetRuntimeConfigLoader()); Mock metadataProviderFactory = new(); From 170c2825df7823330f99f39c48d88c092d2ccc90 Mon Sep 17 00:00:00 2001 From: Aaron Burtle Date: Wed, 5 Aug 2026 14:17:06 -0700 Subject: [PATCH 2/4] fix some missing operator paths --- src/Auth/IAuthorizationResolver.cs | 12 +- src/Auth/IResolvedDatabasePolicyProvider.cs | 28 ++ src/Auth/ResolvedDatabasePolicy.cs | 35 +- .../Authorization/AuthorizationResolver.cs | 43 +- src/Core/Parsers/FilterParser.cs | 5 +- src/Core/Parsers/ODataASTCosmosVisitor.cs | 11 +- src/Core/Parsers/ParameterAliasRewriter.cs | 86 ++++ .../Resolvers/AuthorizationPolicyHelpers.cs | 51 +- src/Core/Resolvers/CosmosQueryStructure.cs | 8 - .../AuthorizationResolverUnitTests.cs | 65 ++- .../REST/RestAuthorizationHandlerUnitTests.cs | 3 +- .../DatabasePolicyClaimBindingUnitTests.cs | 445 ++++++++++++++++-- .../UnitTests/DwSqlQueryBuilderUpsertTests.cs | 7 +- 13 files changed, 700 insertions(+), 99 deletions(-) create mode 100644 src/Auth/IResolvedDatabasePolicyProvider.cs create mode 100644 src/Core/Parsers/ParameterAliasRewriter.cs diff --git a/src/Auth/IAuthorizationResolver.cs b/src/Auth/IAuthorizationResolver.cs index 72ec1983fc..27e3b330eb 100644 --- a/src/Auth/IAuthorizationResolver.cs +++ b/src/Auth/IAuthorizationResolver.cs @@ -80,11 +80,13 @@ public interface IAuthorizationResolver /// Role defined in client role header. /// Operation type: Create, Read, Update, Delete. /// Contains token claims of the authenticated user used in policy evaluation. - /// - /// Returns policy text containing parameter aliases together with the typed claim values - /// that must be bound to those aliases, or an exception when processing fails. - /// - public ResolvedDatabasePolicy ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext); + /// Returns the parsed policy, if safely representable as text, or an exception otherwise. + /// + /// Claim-bearing policies cannot be represented safely by this string-only contract. + /// Use instead. + /// + [Obsolete("Use IResolvedDatabasePolicyProvider.ResolveDBPolicy to keep claim values separate from policy text.")] + public string ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext); /// /// Get list of roles defined for entity within runtime configuration.. This is applicable for GraphQL when creating authorization diff --git a/src/Auth/IResolvedDatabasePolicyProvider.cs b/src/Auth/IResolvedDatabasePolicyProvider.cs new file mode 100644 index 0000000000..f816b15d99 --- /dev/null +++ b/src/Auth/IResolvedDatabasePolicyProvider.cs @@ -0,0 +1,28 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Azure.DataApiBuilder.Config.ObjectModel; +using Microsoft.AspNetCore.Http; + +namespace Azure.DataApiBuilder.Auth; + +/// +/// Resolves database authorization policies while keeping claim values separate +/// from policy text. +/// +public interface IResolvedDatabasePolicyProvider +{ + /// + /// Resolves claim references to parameter aliases and returns their typed values separately. + /// + /// Entity from the request. + /// Role defined in the client role header. + /// Operation type: Create, Read, Update, Delete. + /// Contains the authenticated user's token claims. + /// The policy text and typed claim values to bind to it. + public ResolvedDatabasePolicy ResolveDBPolicy( + string entityName, + string roleName, + EntityActionOperation operation, + HttpContext httpContext); +} diff --git a/src/Auth/ResolvedDatabasePolicy.cs b/src/Auth/ResolvedDatabasePolicy.cs index 83ffefe104..1e4e8a2676 100644 --- a/src/Auth/ResolvedDatabasePolicy.cs +++ b/src/Auth/ResolvedDatabasePolicy.cs @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using System.Collections.ObjectModel; + namespace Azure.DataApiBuilder.Auth; /// @@ -8,16 +10,37 @@ namespace Azure.DataApiBuilder.Auth; /// OData parameter aliases. Claim values remain separate from the policy text so /// they can be injected into the parsed OData AST as typed constants. /// -/// Policy text containing OData parameter aliases. -/// Typed claim values keyed by their parameter alias. -public sealed record ResolvedDatabasePolicy( - string Policy, - IReadOnlyDictionary ClaimValues) +public sealed record ResolvedDatabasePolicy { + /// + /// Initializes a resolved database policy and takes an immutable snapshot of its claim values. + /// + /// Policy text containing OData parameter aliases. + /// Typed claim values keyed by their parameter alias. + public ResolvedDatabasePolicy(string policy, IReadOnlyDictionary claimValues) + { + ArgumentNullException.ThrowIfNull(policy); + ArgumentNullException.ThrowIfNull(claimValues); + + Policy = policy; + ClaimValues = new ReadOnlyDictionary( + new Dictionary(claimValues, StringComparer.Ordinal)); + } + + /// + /// Policy text containing OData parameter aliases. + /// + public string Policy { get; } + + /// + /// Immutable snapshot of typed claim values keyed by parameter alias. + /// + public IReadOnlyDictionary ClaimValues { get; } + /// /// Represents an operation without a database authorization policy. /// public static ResolvedDatabasePolicy Empty { get; } = new( string.Empty, - new Dictionary()); + new ReadOnlyDictionary(new Dictionary())); } diff --git a/src/Core/Authorization/AuthorizationResolver.cs b/src/Core/Authorization/AuthorizationResolver.cs index 39a6671f42..4b896354f0 100644 --- a/src/Core/Authorization/AuthorizationResolver.cs +++ b/src/Core/Authorization/AuthorizationResolver.cs @@ -25,7 +25,7 @@ namespace Azure.DataApiBuilder.Core.Authorization; /// Authorization stages that require passing before a request is executed /// against a database. /// -public class AuthorizationResolver : IAuthorizationResolver +public class AuthorizationResolver : IAuthorizationResolver, IResolvedDatabasePolicyProvider { private readonly RuntimeConfigProvider _runtimeConfigProvider; private readonly IMetadataProviderFactory _metadataProviderFactory; @@ -206,7 +206,29 @@ public bool AreColumnsAllowedForOperation(string entityName, string roleName, En } /// - public ResolvedDatabasePolicy ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext) + [Obsolete("Use IResolvedDatabasePolicyProvider.ResolveDBPolicy to keep claim values separate from policy text.")] + public string ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext) + { + string dbPolicy = GetDBPolicyForRequest(entityName, roleName, operation); + + if (string.IsNullOrWhiteSpace(dbPolicy)) + { + return string.Empty; + } + + if (dbPolicy.Contains(CLAIM_PREFIX, StringComparison.Ordinal)) + { + throw new DataApiBuilderException( + message: "Claim-bearing database policies require typed claim binding.", + statusCode: HttpStatusCode.Forbidden, + subStatusCode: DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed); + } + + return dbPolicy.Replace(FIELD_PREFIX, string.Empty); + } + + /// + public ResolvedDatabasePolicy ResolveDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext) { string dBpolicyWithClaimTypes = GetDBPolicyForRequest(entityName, roleName, operation); @@ -872,7 +894,7 @@ private static ResolvedDatabasePolicy GetPolicyWithClaimValues(string policy, Di case ClaimValueTypes.UInteger64: return (decimal)ulong.Parse(claim.Value, NumberStyles.Integer, CultureInfo.InvariantCulture); case ClaimValueTypes.Double: - return double.Parse(claim.Value, NumberStyles.Float, CultureInfo.InvariantCulture); + return ParseFiniteDoubleClaimValue(claim.Value); case JsonClaimValueTypes.JsonNull: return null; default: @@ -886,6 +908,21 @@ private static ResolvedDatabasePolicy GetPolicyWithClaimValues(string policy, Di } } + /// + /// Parses a floating-point claim and rejects values that database providers cannot + /// represent consistently, including NaN and positive or negative infinity. + /// + private static double ParseFiniteDoubleClaimValue(string value) + { + double parsedValue = double.Parse(value, NumberStyles.Float, CultureInfo.InvariantCulture); + if (!double.IsFinite(parsedValue)) + { + throw new FormatException("The floating-point claim value must be finite."); + } + + return parsedValue; + } + private static DataApiBuilderException CreateUnsupportedClaimValueException(Claim claim, Exception? innerException = null) { return new DataApiBuilderException( diff --git a/src/Core/Parsers/FilterParser.cs b/src/Core/Parsers/FilterParser.cs index dbad26ba20..f261a6e197 100644 --- a/src/Core/Parsers/FilterParser.cs +++ b/src/Core/Parsers/FilterParser.cs @@ -73,7 +73,10 @@ public FilterClause GetFilterClause( } } - return parser.ParseFilter(); + FilterClause filterClause = parser.ParseFilter(); + return parameterAliasNodes is null + ? filterClause + : new ParameterAliasRewriter(parameterAliasNodes).Rewrite(filterClause); } catch (ODataException e) { diff --git a/src/Core/Parsers/ODataASTCosmosVisitor.cs b/src/Core/Parsers/ODataASTCosmosVisitor.cs index f32a60c1a5..1912972766 100644 --- a/src/Core/Parsers/ODataASTCosmosVisitor.cs +++ b/src/Core/Parsers/ODataASTCosmosVisitor.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using Azure.DataApiBuilder.Core.Resolvers; using Microsoft.OData.UriParser; /// @@ -12,17 +13,17 @@ namespace Azure.DataApiBuilder.Core.Parsers internal class ODataASTCosmosVisitor : QueryNodeVisitor { private string _prefix; - private readonly Func _makeDbConnectionParam; + private readonly BaseQueryStructure _queryStructure; /// /// Constructor for the visitor to append prefix to the column names which would be the path from container to the column /// /// - /// Creates a bound Cosmos DB query parameter. - public ODataASTCosmosVisitor(string prefix, Func makeDbConnectionParam) + /// Stores bound Cosmos DB query parameters. + public ODataASTCosmosVisitor(string prefix, BaseQueryStructure queryStructure) { this._prefix = prefix; - _makeDbConnectionParam = makeDbConnectionParam; + _queryStructure = queryStructure; } /// @@ -126,7 +127,7 @@ public override string Visit(ConstantNode nodeIn) return "NULL"; } - return _makeDbConnectionParam(nodeIn.Value); + return _queryStructure.MakeDbConnectionParam(nodeIn.Value); } /// diff --git a/src/Core/Parsers/ParameterAliasRewriter.cs b/src/Core/Parsers/ParameterAliasRewriter.cs new file mode 100644 index 0000000000..cfcef05dba --- /dev/null +++ b/src/Core/Parsers/ParameterAliasRewriter.cs @@ -0,0 +1,86 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.OData; +using Microsoft.OData.Edm; +using Microsoft.OData.UriParser; + +namespace Azure.DataApiBuilder.Core.Parsers; + +/// +/// Replaces OData parameter aliases with their typed AST values after parsing. +/// The resolver still supplies aliases during binary type promotion; this pass +/// also covers aliases in unary and root Boolean expressions. +/// +internal sealed class ParameterAliasRewriter +{ + private readonly IReadOnlyDictionary _parameterAliasNodes; + + public ParameterAliasRewriter(IReadOnlyDictionary parameterAliasNodes) + { + _parameterAliasNodes = parameterAliasNodes; + } + + /// + /// Rewrites all supported nodes in a filter clause and normalizes bare Boolean + /// values into comparisons that are valid SQL predicates across providers. + /// + public FilterClause Rewrite(FilterClause filterClause) + { + SingleValueNode expression = RewriteNode(filterClause.Expression, isPredicate: true); + return new FilterClause(expression, filterClause.RangeVariable); + } + + private SingleValueNode RewriteNode(SingleValueNode node, bool isPredicate) + { + return node switch + { + BinaryOperatorNode binaryNode => RewriteBinaryOperator(binaryNode), + UnaryOperatorNode unaryNode => new UnaryOperatorNode( + unaryNode.OperatorKind, + RewriteNode(unaryNode.Operand, isPredicate: true)), + ConvertNode convertNode => NormalizeBooleanPredicate( + new ConvertNode( + RewriteNode(convertNode.Source, isPredicate: false), + convertNode.TypeReference), + isPredicate), + ParameterAliasNode aliasNode => RewriteAlias(aliasNode, isPredicate), + ConstantNode constantNode => NormalizeBooleanPredicate(constantNode, isPredicate), + SingleValuePropertyAccessNode propertyNode => NormalizeBooleanPredicate(propertyNode, isPredicate), + _ => throw new ODataException( + $"Database policy expression node '{node.Kind}' is not supported for typed claim binding.") + }; + } + + private SingleValueNode RewriteBinaryOperator(BinaryOperatorNode node) + { + bool operandsArePredicates = node.OperatorKind is BinaryOperatorKind.And or BinaryOperatorKind.Or; + return new BinaryOperatorNode( + node.OperatorKind, + RewriteNode(node.Left, operandsArePredicates), + RewriteNode(node.Right, operandsArePredicates)); + } + + private SingleValueNode RewriteAlias(ParameterAliasNode aliasNode, bool isPredicate) + { + if (!_parameterAliasNodes.TryGetValue(aliasNode.Alias, out SingleValueNode? valueNode)) + { + throw new ODataException($"No value was supplied for database policy parameter alias '{aliasNode.Alias}'."); + } + + return RewriteNode(valueNode, isPredicate); + } + + private static SingleValueNode NormalizeBooleanPredicate(SingleValueNode node, bool isPredicate) + { + if (!isPredicate || node.TypeReference?.PrimitiveKind() is not EdmPrimitiveTypeKind.Boolean) + { + return node; + } + + return new BinaryOperatorNode( + BinaryOperatorKind.Equal, + node, + new ConstantNode(true)); + } +} diff --git a/src/Core/Resolvers/AuthorizationPolicyHelpers.cs b/src/Core/Resolvers/AuthorizationPolicyHelpers.cs index 093e8eefeb..7bfa9c8b5e 100644 --- a/src/Core/Resolvers/AuthorizationPolicyHelpers.cs +++ b/src/Core/Resolvers/AuthorizationPolicyHelpers.cs @@ -107,7 +107,7 @@ public static void ProcessAuthorizationPolicies( fromClause = pathConfig.JoinStatement; predicates = filterClause?.Expression.Accept(new ODataASTCosmosVisitor( pathConfig.Alias, - value => cosmosQueryStructure.MakeDbConnectionParam(value))); + cosmosQueryStructure)); existQuery = CosmosQueryBuilder.BuildExistsQueryForCosmos(fromClause, predicates); } @@ -115,7 +115,7 @@ public static void ProcessAuthorizationPolicies( { predicates = filterClause?.Expression.Accept(new ODataASTCosmosVisitor( $"{pathConfig.Path}.{pathConfig.ColumnName}", - value => cosmosQueryStructure.MakeDbConnectionParam(value))); + cosmosQueryStructure)); } if (pathConfig.EntityName == entity.Key) @@ -164,11 +164,12 @@ private static List ProcessFilter( List filterClauses = new(); foreach (EntityActionOperation elementalOperation in elementalOperations) { - ResolvedDatabasePolicy dbQueryPolicy = authorizationResolver.ProcessDBPolicy( - entityName, - clientRoleHeader, - elementalOperation, - context); + ResolvedDatabasePolicy dbQueryPolicy = ResolveDatabasePolicy( + authorizationResolver, + entityName, + clientRoleHeader, + elementalOperation, + context); FilterClause? filterClause = GetDBPolicyClauseForQueryStructure( dbQueryPolicy, @@ -182,6 +183,42 @@ private static List ProcessFilter( return filterClauses; } + /// + /// Uses typed claim binding when supported. Legacy authorization resolver implementations + /// remain compatible for static policies, but claim-bearing policies fail closed because + /// the legacy string contract cannot keep claim data separate from policy syntax. + /// + private static ResolvedDatabasePolicy ResolveDatabasePolicy( + IAuthorizationResolver authorizationResolver, + string entityName, + string roleName, + EntityActionOperation operation, + HttpContext context) + { + if (authorizationResolver is IResolvedDatabasePolicyProvider resolvedPolicyProvider) + { + return resolvedPolicyProvider.ResolveDBPolicy(entityName, roleName, operation, context); + } + + string policy = authorizationResolver.GetDBPolicyForRequest(entityName, roleName, operation); + if (string.IsNullOrWhiteSpace(policy)) + { + return ResolvedDatabasePolicy.Empty; + } + + if (policy.Contains(AuthorizationResolver.CLAIM_PREFIX, StringComparison.Ordinal)) + { + throw new DataApiBuilderException( + message: "The authorization resolver does not support typed database policy claims.", + statusCode: System.Net.HttpStatusCode.Forbidden, + subStatusCode: DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed); + } + + return new ResolvedDatabasePolicy( + policy.Replace(AuthorizationResolver.FIELD_PREFIX, string.Empty), + new Dictionary()); + } + /// /// Appends the filter query formatting to a resolved database policy and parses it with ODataParser. /// diff --git a/src/Core/Resolvers/CosmosQueryStructure.cs b/src/Core/Resolvers/CosmosQueryStructure.cs index 29c435d955..897345260a 100644 --- a/src/Core/Resolvers/CosmosQueryStructure.cs +++ b/src/Core/Resolvers/CosmosQueryStructure.cs @@ -67,14 +67,6 @@ public CosmosQueryStructure( Init(parameters); } - /// - public override string MakeDbConnectionParam(object? value, string? columnName = null, bool lengthOverride = false) - { - string encodedParamName = $"{PARAM_NAME_PREFIX}param{Counter.Next()}"; - Parameters.Add(encodedParamName, new(value)); - return encodedParamName; - } - private static IEnumerable GenerateQueryColumns(SelectionSetNode selectionSet, DocumentNode document, string tableName) { foreach (ISelectionNode selectionNode in selectionSet.Selections) diff --git a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs index c61031f722..93ef5a68a6 100644 --- a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs +++ b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs @@ -1317,7 +1317,7 @@ public void ParseValidDbPolicy(string policy, string expectedParsedPolicy) context.Setup(x => x.User).Returns(principal); context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); - ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + ResolvedDatabasePolicy parsedPolicy = authZResolver.ResolveDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); Assert.AreEqual(parsedPolicy.Policy, expectedParsedPolicy); } @@ -1363,12 +1363,46 @@ public void DbPolicy_StringClaim_UsesTypedParameterAlias(string claimValue) context.Setup(x => x.User).Returns(principal); context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); - ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + ResolvedDatabasePolicy parsedPolicy = authZResolver.ResolveDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); Assert.AreEqual("col1 eq @dabClaim0", parsedPolicy.Policy); Assert.AreEqual(claimValue, parsedPolicy.ClaimValues["@dabClaim0"]); } + /// + /// The legacy public API keeps its string return type for source and binary compatibility, + /// but fails closed because claim values cannot be represented safely in policy text. + /// + [TestMethod] + public void DbPolicy_LegacyStringContractWithClaims_FailsClosed() + { + RuntimeConfig runtimeConfig = InitRuntimeConfig( + entityName: TEST_ENTITY, + roleName: TEST_ROLE, + operation: TEST_OPERATION, + includedCols: new HashSet { "col1" }, + databasePolicy: "@item.col1 eq @claims.userId"); + AuthorizationResolver authZResolver = AuthorizationHelpers.InitAuthorizationResolver(runtimeConfig); + + Mock context = new(); + ClaimsIdentity identity = new(TEST_AUTHENTICATION_TYPE, TEST_CLAIMTYPE_NAME, AuthenticationOptions.ROLE_CLAIM_TYPE); + identity.AddClaim(new Claim("userId", "alice", ClaimValueTypes.String)); + context.Setup(x => x.User).Returns(new ClaimsPrincipal(identity)); + context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); + + Assert.AreEqual( + typeof(string), + typeof(IAuthorizationResolver).GetMethod(nameof(IAuthorizationResolver.ProcessDBPolicy))!.ReturnType); + + #pragma warning disable CS0618 // Explicitly validates the preserved legacy API contract. + DataApiBuilderException exception = Assert.ThrowsException(() => + authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object)); + #pragma warning restore CS0618 + + Assert.AreEqual(HttpStatusCode.Forbidden, exception.StatusCode); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed, exception.SubStatusCode); + } + /// /// Tests authorization policy processing mechanism by validating value type compatibility /// of claims present in HttpContext.User.Claims. @@ -1421,7 +1455,7 @@ public void DbPolicy_ClaimValueTypeParsing(string claimValueType, string claimVa try { - ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + ResolvedDatabasePolicy parsedPolicy = authZResolver.ResolveDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); Assert.IsTrue(supportedValueType); Assert.AreEqual(expectedPolicy, parsedPolicy.Policy); @@ -1452,8 +1486,13 @@ public void DbPolicy_ClaimValueTypeParsing(string claimValueType, string claimVa /// A claim whose value does not match its declared primitive type must fail before /// policy parsing and must never be interpreted as OData syntax. /// - [TestMethod] - public void DbPolicy_MalformedPrimitiveClaim_FailsClosed() + [DataTestMethod] + [DataRow(ClaimValueTypes.Integer, "1 or 1 eq 1", DisplayName = "Malformed integer")] + [DataRow(ClaimValueTypes.Double, "NaN", DisplayName = "NaN")] + [DataRow(ClaimValueTypes.Double, "Infinity", DisplayName = "Positive infinity")] + [DataRow(ClaimValueTypes.Double, "-Infinity", DisplayName = "Negative infinity")] + [DataRow(ClaimValueTypes.Double, "1e9999", DisplayName = "Exponent overflow")] + public void DbPolicy_InvalidPrimitiveClaim_FailsClosed(string claimValueType, string claimValue) { RuntimeConfig runtimeConfig = InitRuntimeConfig( entityName: TEST_ENTITY, @@ -1465,12 +1504,12 @@ public void DbPolicy_MalformedPrimitiveClaim_FailsClosed() Mock context = new(); ClaimsIdentity identity = new(TEST_AUTHENTICATION_TYPE, TEST_CLAIMTYPE_NAME, AuthenticationOptions.ROLE_CLAIM_TYPE); - identity.AddClaim(new Claim("testClaim", "1 or 1 eq 1", ClaimValueTypes.Integer)); + identity.AddClaim(new Claim("testClaim", claimValue, claimValueType)); context.Setup(x => x.User).Returns(new ClaimsPrincipal(identity)); context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); DataApiBuilderException exception = Assert.ThrowsException(() => - authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object)); + authZResolver.ResolveDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object)); Assert.AreEqual(HttpStatusCode.Forbidden, exception.StatusCode); Assert.AreEqual(DataApiBuilderException.SubStatusCodes.UnsupportedClaimValueType, exception.SubStatusCode); @@ -1509,7 +1548,7 @@ public void ParseInvalidDbPolicyWithUserNotPossessingAllClaims(string policy) try { - authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + authZResolver.ResolveDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); } catch (DataApiBuilderException ex) { @@ -1561,7 +1600,7 @@ public void ParsePolicyWithDuplicateUserClaims() context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); // Act - ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + ResolvedDatabasePolicy parsedPolicy = authZResolver.ResolveDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); // Assert string expectedPolicy = "@dabClaim0 eq col2 and @dabClaim1 eq col3"; @@ -1615,15 +1654,17 @@ public void GetDBPolicyTest( context.Setup(x => x.User).Returns(principal); context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(clientRole); - ResolvedDatabasePolicy parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object); + #pragma warning disable CS0618 // Explicitly validates the preserved legacy API contract. + string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object); + #pragma warning restore CS0618 string errorMessage = "TryProcessDBPolicy returned unexpected value."; if (expectPolicy) { - Assert.AreEqual(actual: parsedPolicy.Policy, expected: policy, message: errorMessage); + Assert.AreEqual(actual: parsedPolicy, expected: policy, message: errorMessage); } else { - Assert.AreEqual(actual: parsedPolicy.Policy, expected: string.Empty, message: errorMessage); + Assert.AreEqual(actual: parsedPolicy, expected: string.Empty, message: errorMessage); } } diff --git a/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs b/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs index 69f71368dd..26264741e0 100644 --- a/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs +++ b/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs @@ -99,13 +99,14 @@ public void TestWildcardPolicyResolvesToEmpty(string httpMethod) AuthorizationResolver authorizationResolver = SetupAuthResolverWithWildcardOperation(); HttpContext httpContext = CreateHttpContext(httpMethod: httpMethod, clientRole: "admin"); + #pragma warning disable CS0618 // Explicitly validates the preserved legacy API contract. Assert.AreEqual(expected: string.Empty, actual: authorizationResolver.ProcessDBPolicy( entityName: AuthorizationHelpers.TEST_ENTITY, roleName: "admin", operation: RestService.HttpVerbToOperations(httpVerbName: httpMethod), httpContext: httpContext) - .Policy ); + #pragma warning restore CS0618 } /// diff --git a/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs b/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs index 417aa87e64..6c7856de40 100644 --- a/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs +++ b/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs @@ -3,24 +3,41 @@ using System; using System.Collections.Generic; +using System.Data; +using System.Linq; +using System.Net; +using System.Security.Claims; +using Azure.DataApiBuilder.Auth; +using Azure.DataApiBuilder.Config.DatabasePrimitives; +using Azure.DataApiBuilder.Config.ObjectModel; +using Azure.DataApiBuilder.Core.Authorization; using Azure.DataApiBuilder.Core.Parsers; -using Microsoft.OData.Edm; +using Azure.DataApiBuilder.Core.Resolvers; +using Azure.DataApiBuilder.Core.Services; +using Azure.DataApiBuilder.Service.Exceptions; +using Azure.DataApiBuilder.Service.Tests.Authorization; +using Microsoft.AspNetCore.Http; +using Microsoft.IdentityModel.JsonWebTokens; using Microsoft.OData.UriParser; using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; namespace Azure.DataApiBuilder.Service.Tests.UnitTests { /// - /// Tests that database policy claim values are bound as typed AST constants and - /// never interpreted as URI/OData syntax. + /// Tests the complete database-policy claim binding path from an authenticated claim + /// through OData AST creation and SQL/Cosmos query parameter collection. /// [TestClass] public class DatabasePolicyClaimBindingUnitTests { + private const string ENTITY_NAME = AuthorizationHelpers.TEST_ENTITY; + private const string ROLE_NAME = AuthorizationHelpers.TEST_ROLE; + private const EntityActionOperation OPERATION = EntityActionOperation.Read; + /// - /// Verifies that attacker-influenced claim values never enter the policy URI. The OData - /// parser must see one equality expression whose right operand is the original claim value, - /// regardless of percent encoding, and Cosmos DB must receive that value as a parameter. + /// Verifies that encoded and literal syntax remains claim data throughout the complete + /// SQL and Cosmos policy pipelines. /// [DataTestMethod] [DataRow("alice%27 or 1 eq 1 or %27", DisplayName = "Percent-encoded quote")] @@ -28,58 +45,394 @@ public class DatabasePolicyClaimBindingUnitTests [DataRow("alice%252527 or 1 eq 1 or %27", DisplayName = "Mixed nested encodings")] [DataRow("alice' or 1 eq 1 or '", DisplayName = "Literal quote")] [DataRow("50% complete", DisplayName = "Legitimate percent character")] - public void DatabasePolicyClaimValue_IsTypedAstConstantAndCosmosParameter(string claimValue) + public void StringClaim_RemainsBoundParameterAcrossSqlAndCosmos(string claimValue) + { + const string policy = "@item.textCol eq @claims.value"; + (AuthorizationResolver resolver, DefaultHttpContext context) = CreateAuthorizationContext( + policy, + new Claim("value", claimValue, ClaimValueTypes.String)); + Mock metadataProvider = CreateMetadataProvider(); + + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver); + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver, + metadataProvider.Object); + + Assert.AreEqual("([textCol] = @param0)", sqlStructure.GetDbPolicyForOperation(OPERATION)); + AssertParameterValues(sqlStructure, claimValue); + + FilterClause filterClause = ResolveFilterClause(resolver, context, metadataProvider.Object); + TestQueryStructure cosmosStructure = new(metadataProvider.Object, resolver); + string cosmosPredicate = filterClause.Expression.Accept( + new ODataASTCosmosVisitor("c", cosmosStructure)); + + Assert.AreEqual("(c.textCol = @param0)", cosmosPredicate); + AssertParameterValues(cosmosStructure, claimValue); + } + + /// + /// Verifies aliases in root and unary Boolean positions are replaced throughout the AST + /// and generate executable, parameterized predicates for both SQL and Cosmos DB. + /// + [DataTestMethod] + [DataRow("@claims.value", "true", "(@param0 = @param1)", DisplayName = "Root Boolean claim")] + [DataRow("not @claims.value", "false", "(NOT (@param0 = @param1) )", DisplayName = "Unary Boolean claim")] + public void BooleanClaim_InRootOrUnaryPosition_IsResolvedAcrossSqlAndCosmos( + string policy, + string claimValue, + string expectedPredicate) + { + bool expectedValue = bool.Parse(claimValue); + (AuthorizationResolver resolver, DefaultHttpContext context) = CreateAuthorizationContext( + policy, + new Claim("value", claimValue, ClaimValueTypes.Boolean)); + Mock metadataProvider = CreateMetadataProvider(); + + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver); + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver, + metadataProvider.Object); + + Assert.AreEqual(expectedPredicate, sqlStructure.GetDbPolicyForOperation(OPERATION)); + AssertParameterValues(sqlStructure, expectedValue, true); + + FilterClause filterClause = ResolveFilterClause(resolver, context, metadataProvider.Object); + TestQueryStructure cosmosStructure = new(metadataProvider.Object, resolver); + string cosmosPredicate = filterClause.Expression.Accept( + new ODataASTCosmosVisitor("c", cosmosStructure)); + + Assert.AreEqual(expectedPredicate, cosmosPredicate); + AssertParameterValues(cosmosStructure, expectedValue, true); + } + + /// + /// Verifies aliases remain resolved when Boolean predicates are nested under logical operators. + /// + [TestMethod] + public void BooleanClaims_InNestedLogicalExpression_AreResolvedAcrossSqlAndCosmos() + { + const string policy = "@claims.first and not @claims.second"; + const string expectedPredicate = "((@param0 = @param1) AND (NOT (@param2 = @param3) ))"; + (AuthorizationResolver resolver, DefaultHttpContext context) = CreateAuthorizationContext( + policy, + new Claim("first", "true", ClaimValueTypes.Boolean), + new Claim("second", "false", ClaimValueTypes.Boolean)); + Mock metadataProvider = CreateMetadataProvider(); + + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver); + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver, + metadataProvider.Object); + + Assert.AreEqual(expectedPredicate, sqlStructure.GetDbPolicyForOperation(OPERATION)); + AssertParameterValues(sqlStructure, true, true, false, true); + + FilterClause filterClause = ResolveFilterClause(resolver, context, metadataProvider.Object); + TestQueryStructure cosmosStructure = new(metadataProvider.Object, resolver); + string cosmosPredicate = filterClause.Expression.Accept( + new ODataASTCosmosVisitor("c", cosmosStructure)); + + Assert.AreEqual(expectedPredicate, cosmosPredicate); + AssertParameterValues(cosmosStructure, true, true, false, true); + } + + /// + /// Verifies existing authorization resolver implementations remain usable for static policies. + /// + [TestMethod] + public void LegacyAuthorizationResolver_StaticPolicy_RemainsSupported() + { + Mock resolver = new(); + resolver + .Setup(instance => instance.GetDBPolicyForRequest(ENTITY_NAME, ROLE_NAME, OPERATION)) + .Returns("@item.intCol eq 42"); + Mock metadataProvider = CreateMetadataProvider(); + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver.Object); + DefaultHttpContext context = new(); + context.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER] = ROLE_NAME; + + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver.Object, + metadataProvider.Object); + + Assert.AreEqual("([intCol] = @param0)", sqlStructure.GetDbPolicyForOperation(OPERATION)); + AssertParameterValues(sqlStructure, 42); + } + + /// + /// Verifies existing string-only resolver implementations fail closed for claim-bearing policies. + /// + [TestMethod] + public void LegacyAuthorizationResolver_ClaimPolicy_FailsClosed() { - const string claimAlias = "@dabClaim0"; - Dictionary claimValueNodes = new() + Mock resolver = new(); + resolver + .Setup(instance => instance.GetDBPolicyForRequest(ENTITY_NAME, ROLE_NAME, OPERATION)) + .Returns("@item.textCol eq @claims.value"); + Mock metadataProvider = CreateMetadataProvider(); + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver.Object); + DefaultHttpContext context = new(); + context.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER] = ROLE_NAME; + + DataApiBuilderException exception = Assert.ThrowsException(() => + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver.Object, + metadataProvider.Object)); + + Assert.AreEqual(HttpStatusCode.Forbidden, exception.StatusCode); + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed, exception.SubStatusCode); + Assert.AreEqual(0, sqlStructure.Parameters.Count); + } + + /// + /// Verifies string claims are promoted to the target column's numeric type before + /// SQL and Cosmos parameters are created. + /// + [TestMethod] + public void StringClaim_IsPromotedToNumericColumnType() + { + const string policy = "@item.intCol eq @claims.value"; + (AuthorizationResolver resolver, DefaultHttpContext context) = CreateAuthorizationContext( + policy, + new Claim("value", "42", ClaimValueTypes.String)); + Mock metadataProvider = CreateMetadataProvider(); + + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver); + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver, + metadataProvider.Object); + + Assert.AreEqual("([intCol] = @param0)", sqlStructure.GetDbPolicyForOperation(OPERATION)); + AssertParameterValues(sqlStructure, 42); + Assert.AreEqual(DbType.Int32, sqlStructure.Parameters["@param0"].DbType); + + FilterClause filterClause = ResolveFilterClause(resolver, context, metadataProvider.Object); + TestQueryStructure cosmosStructure = new(metadataProvider.Object, resolver); + string cosmosPredicate = filterClause.Expression.Accept( + new ODataASTCosmosVisitor("c", cosmosStructure)); + + Assert.AreEqual("(c.intCol = @param0)", cosmosPredicate); + AssertParameterValues(cosmosStructure, 42); + } + + /// + /// Verifies null claims remain typed null AST constants and do not create provider parameters. + /// + [TestMethod] + public void NullClaim_ProducesNullPredicateWithoutParameter() + { + const string policy = "@item.textCol eq @claims.value"; + (AuthorizationResolver resolver, DefaultHttpContext context) = CreateAuthorizationContext( + policy, + new Claim("value", "null", JsonClaimValueTypes.JsonNull)); + Mock metadataProvider = CreateMetadataProvider(); + + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver); + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver, + metadataProvider.Object); + + Assert.AreEqual("([textCol] IS NULL)", sqlStructure.GetDbPolicyForOperation(OPERATION)); + Assert.AreEqual(0, sqlStructure.Parameters.Count); + + FilterClause filterClause = ResolveFilterClause(resolver, context, metadataProvider.Object); + TestQueryStructure cosmosStructure = new(metadataProvider.Object, resolver); + string cosmosPredicate = filterClause.Expression.Accept( + new ODataASTCosmosVisitor("c", cosmosStructure)); + + Assert.AreEqual("(c.textCol IS NULL)", cosmosPredicate); + Assert.AreEqual(0, cosmosStructure.Parameters.Count); + } + + /// + /// Verifies non-finite floating-point claims fail before either query structure can + /// collect a provider parameter. + /// + [DataTestMethod] + [DataRow("NaN")] + [DataRow("Infinity")] + [DataRow("-Infinity")] + [DataRow("1e9999")] + public void NonFiniteDoubleClaim_FailsBeforeParameterCollection(string claimValue) + { + const string policy = "@item.doubleCol eq @claims.value"; + (AuthorizationResolver resolver, DefaultHttpContext context) = CreateAuthorizationContext( + policy, + new Claim("value", claimValue, ClaimValueTypes.Double)); + Mock metadataProvider = CreateMetadataProvider(); + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver); + + DataApiBuilderException exception = Assert.ThrowsException(() => + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver, + metadataProvider.Object)); + + Assert.AreEqual(DataApiBuilderException.SubStatusCodes.UnsupportedClaimValueType, exception.SubStatusCode); + Assert.AreEqual(0, sqlStructure.Parameters.Count); + } + + /// + /// Verifies resolved policies own a read-only snapshot, including the shared empty value. + /// + [TestMethod] + public void ResolvedPolicyClaimValues_AreImmutableSnapshots() + { + Dictionary source = new() { ["@claim"] = "original" }; + ResolvedDatabasePolicy policy = new("value eq @claim", source); + source["@claim"] = "modified"; + + Assert.AreEqual("original", policy.ClaimValues["@claim"]); + Assert.IsInstanceOfType>(ResolvedDatabasePolicy.Empty.ClaimValues); + IDictionary emptyValues = (IDictionary)ResolvedDatabasePolicy.Empty.ClaimValues; + Assert.ThrowsException(() => emptyValues.Add("@claim", "value")); + } + + private static FilterClause ResolveFilterClause( + AuthorizationResolver resolver, + DefaultHttpContext context, + ISqlMetadataProvider metadataProvider) + { + ResolvedDatabasePolicy resolvedPolicy = resolver.ResolveDBPolicy( + ENTITY_NAME, + ROLE_NAME, + OPERATION, + context); + + return AuthorizationPolicyHelpers.GetDBPolicyClauseForQueryStructure( + resolvedPolicy, + ENTITY_NAME, + $"{ENTITY_NAME}.{metadataProvider.EntityToDatabaseObject[ENTITY_NAME].FullName}", + metadataProvider)!; + } + + private static (AuthorizationResolver Resolver, DefaultHttpContext Context) CreateAuthorizationContext( + string policy, + params Claim[] claims) + { + RuntimeConfig runtimeConfig = AuthorizationHelpers.InitRuntimeConfig( + entityName: ENTITY_NAME, + roleName: ROLE_NAME, + operation: OPERATION, + databasePolicy: policy); + AuthorizationResolver resolver = AuthorizationHelpers.InitAuthorizationResolver(runtimeConfig); + + ClaimsIdentity identity = new( + claims, + authenticationType: "TestAuth", + nameType: ClaimTypes.Name, + roleType: ClaimTypes.Role); + DefaultHttpContext context = new() { - [claimAlias] = new ConstantNode(claimValue) + User = new ClaimsPrincipal(identity) }; + context.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER] = ROLE_NAME; - ODataUriParser parser = new( - BuildModel(), - new Uri($"Entities/?$filter=name eq {claimAlias}", UriKind.Relative)) + return (resolver, context); + } + + private static Mock CreateMetadataProvider() + { + SourceDefinition sourceDefinition = new(); + sourceDefinition.Columns.Add("id", new ColumnDefinition(typeof(int)) { DbType = DbType.Int32 }); + sourceDefinition.Columns.Add("flag", new ColumnDefinition(typeof(bool)) { DbType = DbType.Boolean }); + sourceDefinition.Columns.Add("textCol", new ColumnDefinition(typeof(string)) { DbType = DbType.String }); + sourceDefinition.Columns.Add("intCol", new ColumnDefinition(typeof(int)) { DbType = DbType.Int32 }); + sourceDefinition.Columns.Add("doubleCol", new ColumnDefinition(typeof(double)) { DbType = DbType.Double }); + sourceDefinition.PrimaryKey.Add("id"); + + DatabaseObject databaseObject = new DatabaseTable(schemaName: "dbo", tableName: "PolicyTable"); + Dictionary entities = new() { - Resolver = new ClaimsTypeDataUriResolver(claimValueNodes) + [ENTITY_NAME] = databaseObject }; - foreach ((string alias, SingleValueNode valueNode) in claimValueNodes) + Mock metadataProvider = new(); + metadataProvider.SetupGet(provider => provider.EntityToDatabaseObject).Returns(entities); + metadataProvider.Setup(provider => provider.GetEntityNamesAndDbObjects()).Returns(entities); + metadataProvider.Setup(provider => provider.GetLinkingEntities()) + .Returns(new Dictionary()); + metadataProvider.Setup(provider => provider.GetSourceDefinition(ENTITY_NAME)).Returns(sourceDefinition); + metadataProvider.Setup(provider => provider.GetDatabaseType()).Returns(DatabaseType.MSSQL); + metadataProvider.Setup(provider => provider.GetQueryBuilder()).Returns(new MsSqlQueryBuilder()); + + string? exposedName; + metadataProvider + .Setup(provider => provider.TryGetExposedColumnName(It.IsAny(), It.IsAny(), out exposedName)) + .Callback(new ColumnNameCallback((string _, string column, out string? name) => name = column)) + .Returns(true); + + string? backingName; + metadataProvider + .Setup(provider => provider.TryGetBackingColumn(It.IsAny(), It.IsAny(), out backingName)) + .Callback(new ColumnNameCallback((string _, string column, out string? name) => name = column)) + .Returns(true); + + ODataParser parser = new(); + parser.BuildModel(metadataProvider.Object); + metadataProvider.Setup(provider => provider.GetODataParser()).Returns(parser); + return metadataProvider; + } + + private static void AssertParameterValues(BaseQueryStructure structure, params object?[] expectedValues) + { + object?[] actualValues = structure.Parameters.Values + .Select(parameter => parameter.Value) + .ToArray(); + CollectionAssert.AreEqual(expectedValues, actualValues); + } + + private delegate void ColumnNameCallback(string entity, string column, out string? name); + + private sealed class TestSqlQueryStructure : BaseSqlQueryStructure + { + public TestSqlQueryStructure( + ISqlMetadataProvider metadataProvider, + IAuthorizationResolver authorizationResolver) + : base( + metadataProvider, + authorizationResolver, + gQLFilterParser: null!, + entityName: ENTITY_NAME) { - parser.ParameterAliasNodes.Add(alias, valueNode); } - - FilterClause clause = parser.ParseFilter(); - BinaryOperatorNode comparison = (BinaryOperatorNode)clause.Expression; - Assert.AreEqual(BinaryOperatorKind.Equal, comparison.OperatorKind); - Assert.IsInstanceOfType(comparison.Right); - Assert.AreEqual(claimValue, ((ConstantNode)comparison.Right).Value); - - List parameterValues = new(); - string predicate = clause.Expression.Accept(new ODataASTCosmosVisitor( - "c", - value => - { - parameterValues.Add(value); - return "@param1"; - })); - - Assert.AreEqual("(c.name = @param1)", predicate); - CollectionAssert.AreEqual(new object?[] { claimValue }, parameterValues); } - private static IEdmModel BuildModel() + private sealed class TestQueryStructure : BaseQueryStructure { - EdmModel model = new(); - EdmEntityType entityType = new("Dab", "Entity"); - EdmStructuralProperty id = entityType.AddStructuralProperty("id", EdmPrimitiveTypeKind.Int32, false); - entityType.AddKeys(id); - entityType.AddStructuralProperty("name", EdmPrimitiveTypeKind.String, true); - model.AddElement(entityType); - - EdmEntityContainer container = new("Dab", "Container"); - container.AddEntitySet("Entities", entityType); - model.AddElement(container); - return model; + public TestQueryStructure( + ISqlMetadataProvider metadataProvider, + IAuthorizationResolver authorizationResolver) + : base( + metadataProvider, + authorizationResolver, + gQLFilterParser: null!, + entityName: ENTITY_NAME) + { + } } } } diff --git a/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs b/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs index 8a8a5f2341..5e4dc4a270 100644 --- a/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs +++ b/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs @@ -139,12 +139,9 @@ private static SqlUpsertQueryStructure CreateUpsertStructure() => _columnMapping.TryGetValue(field, out column))) .Returns((string entity, string field, string? column) => _columnMapping.ContainsKey(field)); - // The update policy is injected directly onto the structure, so the resolver only needs - // to return an empty policy (no throw) during construction. + // The update policy is injected directly onto the structure, so this test does not + // invoke the resolver while constructing the query structure. Mock authorizationResolver = new(); - authorizationResolver - .Setup(x => x.ProcessDBPolicy(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns(ResolvedDatabasePolicy.Empty); RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestHelper.GetRuntimeConfigLoader()); Mock metadataProviderFactory = new(); From 118417bf2211839efd447e6e4919a0f66cfef6e8 Mon Sep 17 00:00:00 2001 From: Aaron Burtle Date: Wed, 5 Aug 2026 15:30:59 -0700 Subject: [PATCH 3/4] fix bugs --- .../Authorization/AuthorizationResolver.cs | 6 +- src/Core/Parsers/ClaimsTypeDataUriResolver.cs | 17 +++-- src/Core/Parsers/FilterParser.cs | 4 +- src/Core/Parsers/ParameterAliasRewriter.cs | 20 +++++- .../AuthorizationResolverUnitTests.cs | 3 + .../DatabasePolicyClaimBindingUnitTests.cs | 67 +++++++++++++++++++ 6 files changed, 108 insertions(+), 9 deletions(-) diff --git a/src/Core/Authorization/AuthorizationResolver.cs b/src/Core/Authorization/AuthorizationResolver.cs index 4b896354f0..62cdfef745 100644 --- a/src/Core/Authorization/AuthorizationResolver.cs +++ b/src/Core/Authorization/AuthorizationResolver.cs @@ -925,8 +925,12 @@ private static double ParseFiniteDoubleClaimValue(string value) private static DataApiBuilderException CreateUnsupportedClaimValueException(Claim claim, Exception? innerException = null) { + string message = innerException is null + ? $"The claim value for claim: {claim.Type} belonging to the user has an unsupported data type." + : $"The claim value for claim: {claim.Type} belonging to the user is invalid for its declared data type."; + return new DataApiBuilderException( - message: $"The claim value for claim: {claim.Type} belonging to the user has an unsupported data type.", + message: message, statusCode: HttpStatusCode.Forbidden, subStatusCode: DataApiBuilderException.SubStatusCodes.UnsupportedClaimValueType, innerException: innerException); diff --git a/src/Core/Parsers/ClaimsTypeDataUriResolver.cs b/src/Core/Parsers/ClaimsTypeDataUriResolver.cs index d1fdf6dc40..f8f213dfa2 100644 --- a/src/Core/Parsers/ClaimsTypeDataUriResolver.cs +++ b/src/Core/Parsers/ClaimsTypeDataUriResolver.cs @@ -37,19 +37,26 @@ public override void PromoteBinaryOperandTypes(BinaryOperatorKind binaryOperator ResolveClaimAlias(ref leftNode); ResolveClaimAlias(ref rightNode); - if (leftNode.TypeReference.PrimitiveKind() != rightNode.TypeReference.PrimitiveKind()) + EdmPrimitiveTypeKind? leftPrimitiveKind = leftNode.TypeReference?.PrimitiveKind(); + EdmPrimitiveTypeKind? rightPrimitiveKind = rightNode.TypeReference?.PrimitiveKind(); + + if (leftPrimitiveKind != rightPrimitiveKind) { - if ((leftNode.Kind == QueryNodeKind.SingleValuePropertyAccess) && (rightNode is ConstantNode)) + if (leftPrimitiveKind.HasValue && + leftNode.Kind == QueryNodeKind.SingleValuePropertyAccess && + rightNode is ConstantNode) { TryConvertNodeToTargetType( - targetType: leftNode.TypeReference.PrimitiveKind(), + targetType: leftPrimitiveKind.Value, operandToConvert: ref rightNode ); } - else if (rightNode.Kind == QueryNodeKind.SingleValuePropertyAccess && leftNode is ConstantNode) + else if (rightPrimitiveKind.HasValue && + rightNode.Kind == QueryNodeKind.SingleValuePropertyAccess && + leftNode is ConstantNode) { TryConvertNodeToTargetType( - targetType: rightNode.TypeReference.PrimitiveKind(), + targetType: rightPrimitiveKind.Value, operandToConvert: ref leftNode ); } diff --git a/src/Core/Parsers/FilterParser.cs b/src/Core/Parsers/FilterParser.cs index f261a6e197..e79e559ee5 100644 --- a/src/Core/Parsers/FilterParser.cs +++ b/src/Core/Parsers/FilterParser.cs @@ -65,7 +65,7 @@ public FilterClause GetFilterClause( parser.Resolver = customResolver; } - if (parameterAliasNodes is not null) + if (parameterAliasNodes is { Count: > 0 }) { foreach ((string alias, SingleValueNode valueNode) in parameterAliasNodes) { @@ -74,7 +74,7 @@ public FilterClause GetFilterClause( } FilterClause filterClause = parser.ParseFilter(); - return parameterAliasNodes is null + return parameterAliasNodes is not { Count: > 0 } ? filterClause : new ParameterAliasRewriter(parameterAliasNodes).Rewrite(filterClause); } diff --git a/src/Core/Parsers/ParameterAliasRewriter.cs b/src/Core/Parsers/ParameterAliasRewriter.cs index cfcef05dba..22595cb7bb 100644 --- a/src/Core/Parsers/ParameterAliasRewriter.cs +++ b/src/Core/Parsers/ParameterAliasRewriter.cs @@ -73,7 +73,9 @@ private SingleValueNode RewriteAlias(ParameterAliasNode aliasNode, bool isPredic private static SingleValueNode NormalizeBooleanPredicate(SingleValueNode node, bool isPredicate) { - if (!isPredicate || node.TypeReference?.PrimitiveKind() is not EdmPrimitiveTypeKind.Boolean) + if (!isPredicate || + node.TypeReference?.PrimitiveKind() is not EdmPrimitiveTypeKind.Boolean || + IsPredicateExpression(node)) { return node; } @@ -83,4 +85,20 @@ private static SingleValueNode NormalizeBooleanPredicate(SingleValueNode node, b node, new ConstantNode(true)); } + + /// + /// Returns whether a Boolean node already represents a predicate rather than a bare value. + /// OData can wrap comparison predicates in one or more conversion nodes when binding logical + /// operators. Such predicates must not be rewritten as "predicate eq true", which is invalid SQL. + /// + private static bool IsPredicateExpression(SingleValueNode node) + { + return node switch + { + BinaryOperatorNode => true, + UnaryOperatorNode => true, + ConvertNode convertNode => IsPredicateExpression(convertNode.Source), + _ => false + }; + } } diff --git a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs index 93ef5a68a6..0639c03451 100644 --- a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs +++ b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs @@ -1513,6 +1513,9 @@ public void DbPolicy_InvalidPrimitiveClaim_FailsClosed(string claimValueType, st Assert.AreEqual(HttpStatusCode.Forbidden, exception.StatusCode); Assert.AreEqual(DataApiBuilderException.SubStatusCodes.UnsupportedClaimValueType, exception.SubStatusCode); + Assert.AreEqual( + "The claim value for claim: testClaim belonging to the user is invalid for its declared data type.", + exception.Message); } /// diff --git a/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs b/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs index 6c7856de40..1c29dac4fc 100644 --- a/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs +++ b/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs @@ -145,6 +145,73 @@ public void BooleanClaims_InNestedLogicalExpression_AreResolvedAcrossSqlAndCosmo AssertParameterValues(cosmosStructure, true, true, false, true); } + /// + /// Verifies ordinary comparison predicates are not rewritten as comparisons to Boolean true. + /// + [TestMethod] + public void StaticComparisonPolicy_RemainsValidAcrossSqlAndCosmos() + { + const string policy = "@item.intCol ne 6 and @item.doubleCol gt 0"; + const string expectedSqlPredicate = "(([intCol] != @param0) AND ([doubleCol] > @param1))"; + const string expectedCosmosPredicate = "((c.intCol != @param0) AND (c.doubleCol > @param1))"; + (AuthorizationResolver resolver, DefaultHttpContext context) = CreateAuthorizationContext(policy); + Mock metadataProvider = CreateMetadataProvider(); + + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver); + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver, + metadataProvider.Object); + + Assert.AreEqual(expectedSqlPredicate, sqlStructure.GetDbPolicyForOperation(OPERATION)); + AssertParameterValues(sqlStructure, 6, 0d); + + FilterClause filterClause = ResolveFilterClause(resolver, context, metadataProvider.Object); + TestQueryStructure cosmosStructure = new(metadataProvider.Object, resolver); + string cosmosPredicate = filterClause.Expression.Accept( + new ODataASTCosmosVisitor("c", cosmosStructure)); + + Assert.AreEqual(expectedCosmosPredicate, cosmosPredicate); + AssertParameterValues(cosmosStructure, 6, 0d); + } + + /// + /// Verifies a bare Boolean claim can be combined with an ordinary comparison without + /// rewriting the comparison predicate as "predicate equals true". + /// + [TestMethod] + public void BooleanClaim_CombinedWithComparison_OnlyNormalizesBareClaim() + { + const string policy = "@item.intCol ne 6 and @claims.allowed"; + const string expectedSqlPredicate = "(([intCol] != @param0) AND (@param1 = @param2))"; + const string expectedCosmosPredicate = "((c.intCol != @param0) AND (@param1 = @param2))"; + (AuthorizationResolver resolver, DefaultHttpContext context) = CreateAuthorizationContext( + policy, + new Claim("allowed", "true", ClaimValueTypes.Boolean)); + Mock metadataProvider = CreateMetadataProvider(); + + TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver); + AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( + OPERATION, + sqlStructure, + context, + resolver, + metadataProvider.Object); + + Assert.AreEqual(expectedSqlPredicate, sqlStructure.GetDbPolicyForOperation(OPERATION)); + AssertParameterValues(sqlStructure, 6, true, true); + + FilterClause filterClause = ResolveFilterClause(resolver, context, metadataProvider.Object); + TestQueryStructure cosmosStructure = new(metadataProvider.Object, resolver); + string cosmosPredicate = filterClause.Expression.Accept( + new ODataASTCosmosVisitor("c", cosmosStructure)); + + Assert.AreEqual(expectedCosmosPredicate, cosmosPredicate); + AssertParameterValues(cosmosStructure, 6, true, true); + } + /// /// Verifies existing authorization resolver implementations remain usable for static policies. /// From 60f660d103d739db0a52dc98886cf55c40500de5 Mon Sep 17 00:00:00 2001 From: Aaron Burtle Date: Wed, 5 Aug 2026 16:51:35 -0700 Subject: [PATCH 4/4] remove obsolete method --- src/Auth/IAuthorizationResolver.cs | 14 ++--- src/Auth/IResolvedDatabasePolicyProvider.cs | 28 ---------- src/Auth/ResolvedDatabasePolicy.cs | 20 +++---- .../Authorization/AuthorizationResolver.cs | 24 +------- .../Resolvers/AuthorizationPolicyHelpers.cs | 39 +------------ .../AuthorizationResolverUnitTests.cs | 46 ++-------------- .../REST/RestAuthorizationHandlerUnitTests.cs | 9 ++- .../DabCacheServiceIntegrationTests.cs | 14 +++++ .../DatabasePolicyClaimBindingUnitTests.cs | 55 ------------------- .../UnitTests/DwSqlQueryBuilderUpsertTests.cs | 10 +++- 10 files changed, 47 insertions(+), 212 deletions(-) delete mode 100644 src/Auth/IResolvedDatabasePolicyProvider.cs diff --git a/src/Auth/IAuthorizationResolver.cs b/src/Auth/IAuthorizationResolver.cs index 27e3b330eb..c990611f9a 100644 --- a/src/Auth/IAuthorizationResolver.cs +++ b/src/Auth/IAuthorizationResolver.cs @@ -72,21 +72,15 @@ public interface IAuthorizationResolver public string GetDBPolicyForRequest(string entityName, string roleName, EntityActionOperation operation); /// - /// Retrieves the policy of an operation within an entity's role entry - /// within the permissions section of the runtime config, and tries to process - /// the policy. + /// Resolves claim references in a database policy to parameter aliases and + /// returns their typed values separately from the policy text. /// /// Entity from request. /// Role defined in client role header. /// Operation type: Create, Read, Update, Delete. /// Contains token claims of the authenticated user used in policy evaluation. - /// Returns the parsed policy, if safely representable as text, or an exception otherwise. - /// - /// Claim-bearing policies cannot be represented safely by this string-only contract. - /// Use instead. - /// - [Obsolete("Use IResolvedDatabasePolicyProvider.ResolveDBPolicy to keep claim values separate from policy text.")] - public string ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext); + /// The policy text and typed claim values to bind to it. + public ResolvedDatabasePolicy ResolveDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext); /// /// Get list of roles defined for entity within runtime configuration.. This is applicable for GraphQL when creating authorization diff --git a/src/Auth/IResolvedDatabasePolicyProvider.cs b/src/Auth/IResolvedDatabasePolicyProvider.cs deleted file mode 100644 index f816b15d99..0000000000 --- a/src/Auth/IResolvedDatabasePolicyProvider.cs +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using Azure.DataApiBuilder.Config.ObjectModel; -using Microsoft.AspNetCore.Http; - -namespace Azure.DataApiBuilder.Auth; - -/// -/// Resolves database authorization policies while keeping claim values separate -/// from policy text. -/// -public interface IResolvedDatabasePolicyProvider -{ - /// - /// Resolves claim references to parameter aliases and returns their typed values separately. - /// - /// Entity from the request. - /// Role defined in the client role header. - /// Operation type: Create, Read, Update, Delete. - /// Contains the authenticated user's token claims. - /// The policy text and typed claim values to bind to it. - public ResolvedDatabasePolicy ResolveDBPolicy( - string entityName, - string roleName, - EntityActionOperation operation, - HttpContext httpContext); -} diff --git a/src/Auth/ResolvedDatabasePolicy.cs b/src/Auth/ResolvedDatabasePolicy.cs index 1e4e8a2676..75494b19f8 100644 --- a/src/Auth/ResolvedDatabasePolicy.cs +++ b/src/Auth/ResolvedDatabasePolicy.cs @@ -12,6 +12,16 @@ namespace Azure.DataApiBuilder.Auth; /// public sealed record ResolvedDatabasePolicy { + /// + /// Policy text containing OData parameter aliases. + /// + public string Policy { get; } + + /// + /// Immutable snapshot of typed claim values keyed by parameter alias. + /// + public IReadOnlyDictionary ClaimValues { get; } + /// /// Initializes a resolved database policy and takes an immutable snapshot of its claim values. /// @@ -27,16 +37,6 @@ public ResolvedDatabasePolicy(string policy, IReadOnlyDictionary(claimValues, StringComparer.Ordinal)); } - /// - /// Policy text containing OData parameter aliases. - /// - public string Policy { get; } - - /// - /// Immutable snapshot of typed claim values keyed by parameter alias. - /// - public IReadOnlyDictionary ClaimValues { get; } - /// /// Represents an operation without a database authorization policy. /// diff --git a/src/Core/Authorization/AuthorizationResolver.cs b/src/Core/Authorization/AuthorizationResolver.cs index 62cdfef745..fd0da59393 100644 --- a/src/Core/Authorization/AuthorizationResolver.cs +++ b/src/Core/Authorization/AuthorizationResolver.cs @@ -25,7 +25,7 @@ namespace Azure.DataApiBuilder.Core.Authorization; /// Authorization stages that require passing before a request is executed /// against a database. /// -public class AuthorizationResolver : IAuthorizationResolver, IResolvedDatabasePolicyProvider +public class AuthorizationResolver : IAuthorizationResolver { private readonly RuntimeConfigProvider _runtimeConfigProvider; private readonly IMetadataProviderFactory _metadataProviderFactory; @@ -205,28 +205,6 @@ public bool AreColumnsAllowedForOperation(string entityName, string roleName, En return false; } - /// - [Obsolete("Use IResolvedDatabasePolicyProvider.ResolveDBPolicy to keep claim values separate from policy text.")] - public string ProcessDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext) - { - string dbPolicy = GetDBPolicyForRequest(entityName, roleName, operation); - - if (string.IsNullOrWhiteSpace(dbPolicy)) - { - return string.Empty; - } - - if (dbPolicy.Contains(CLAIM_PREFIX, StringComparison.Ordinal)) - { - throw new DataApiBuilderException( - message: "Claim-bearing database policies require typed claim binding.", - statusCode: HttpStatusCode.Forbidden, - subStatusCode: DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed); - } - - return dbPolicy.Replace(FIELD_PREFIX, string.Empty); - } - /// public ResolvedDatabasePolicy ResolveDBPolicy(string entityName, string roleName, EntityActionOperation operation, HttpContext httpContext) { diff --git a/src/Core/Resolvers/AuthorizationPolicyHelpers.cs b/src/Core/Resolvers/AuthorizationPolicyHelpers.cs index 7bfa9c8b5e..f2efc35cf9 100644 --- a/src/Core/Resolvers/AuthorizationPolicyHelpers.cs +++ b/src/Core/Resolvers/AuthorizationPolicyHelpers.cs @@ -164,8 +164,7 @@ private static List ProcessFilter( List filterClauses = new(); foreach (EntityActionOperation elementalOperation in elementalOperations) { - ResolvedDatabasePolicy dbQueryPolicy = ResolveDatabasePolicy( - authorizationResolver, + ResolvedDatabasePolicy dbQueryPolicy = authorizationResolver.ResolveDBPolicy( entityName, clientRoleHeader, elementalOperation, @@ -183,42 +182,6 @@ private static List ProcessFilter( return filterClauses; } - /// - /// Uses typed claim binding when supported. Legacy authorization resolver implementations - /// remain compatible for static policies, but claim-bearing policies fail closed because - /// the legacy string contract cannot keep claim data separate from policy syntax. - /// - private static ResolvedDatabasePolicy ResolveDatabasePolicy( - IAuthorizationResolver authorizationResolver, - string entityName, - string roleName, - EntityActionOperation operation, - HttpContext context) - { - if (authorizationResolver is IResolvedDatabasePolicyProvider resolvedPolicyProvider) - { - return resolvedPolicyProvider.ResolveDBPolicy(entityName, roleName, operation, context); - } - - string policy = authorizationResolver.GetDBPolicyForRequest(entityName, roleName, operation); - if (string.IsNullOrWhiteSpace(policy)) - { - return ResolvedDatabasePolicy.Empty; - } - - if (policy.Contains(AuthorizationResolver.CLAIM_PREFIX, StringComparison.Ordinal)) - { - throw new DataApiBuilderException( - message: "The authorization resolver does not support typed database policy claims.", - statusCode: System.Net.HttpStatusCode.Forbidden, - subStatusCode: DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed); - } - - return new ResolvedDatabasePolicy( - policy.Replace(AuthorizationResolver.FIELD_PREFIX, string.Empty), - new Dictionary()); - } - /// /// Appends the filter query formatting to a resolved database policy and parses it with ODataParser. /// diff --git a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs index 0639c03451..e5896cf6ce 100644 --- a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs +++ b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs @@ -1369,40 +1369,6 @@ public void DbPolicy_StringClaim_UsesTypedParameterAlias(string claimValue) Assert.AreEqual(claimValue, parsedPolicy.ClaimValues["@dabClaim0"]); } - /// - /// The legacy public API keeps its string return type for source and binary compatibility, - /// but fails closed because claim values cannot be represented safely in policy text. - /// - [TestMethod] - public void DbPolicy_LegacyStringContractWithClaims_FailsClosed() - { - RuntimeConfig runtimeConfig = InitRuntimeConfig( - entityName: TEST_ENTITY, - roleName: TEST_ROLE, - operation: TEST_OPERATION, - includedCols: new HashSet { "col1" }, - databasePolicy: "@item.col1 eq @claims.userId"); - AuthorizationResolver authZResolver = AuthorizationHelpers.InitAuthorizationResolver(runtimeConfig); - - Mock context = new(); - ClaimsIdentity identity = new(TEST_AUTHENTICATION_TYPE, TEST_CLAIMTYPE_NAME, AuthenticationOptions.ROLE_CLAIM_TYPE); - identity.AddClaim(new Claim("userId", "alice", ClaimValueTypes.String)); - context.Setup(x => x.User).Returns(new ClaimsPrincipal(identity)); - context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); - - Assert.AreEqual( - typeof(string), - typeof(IAuthorizationResolver).GetMethod(nameof(IAuthorizationResolver.ProcessDBPolicy))!.ReturnType); - - #pragma warning disable CS0618 // Explicitly validates the preserved legacy API contract. - DataApiBuilderException exception = Assert.ThrowsException(() => - authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object)); - #pragma warning restore CS0618 - - Assert.AreEqual(HttpStatusCode.Forbidden, exception.StatusCode); - Assert.AreEqual(DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed, exception.SubStatusCode); - } - /// /// Tests authorization policy processing mechanism by validating value type compatibility /// of claims present in HttpContext.User.Claims. @@ -1614,7 +1580,7 @@ public void ParsePolicyWithDuplicateUserClaims() // Indirectly tests the AuthorizationResolver private method: // GetDBPolicyForRequest(string entityName, string roleName, string operation) - // by calling public method TryProcessDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object) + // by calling public method ResolveDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object) // The result of executing that method will determine whether execution behaves as expected. // When string.Empty is returned, // then no policy is found for the provided entity, role, and operation combination, therefore, @@ -1657,17 +1623,15 @@ public void GetDBPolicyTest( context.Setup(x => x.User).Returns(principal); context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(clientRole); - #pragma warning disable CS0618 // Explicitly validates the preserved legacy API contract. - string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object); - #pragma warning restore CS0618 - string errorMessage = "TryProcessDBPolicy returned unexpected value."; + ResolvedDatabasePolicy parsedPolicy = authZResolver.ResolveDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object); + string errorMessage = "ResolveDBPolicy returned unexpected value."; if (expectPolicy) { - Assert.AreEqual(actual: parsedPolicy, expected: policy, message: errorMessage); + Assert.AreEqual(actual: parsedPolicy.Policy, expected: policy, message: errorMessage); } else { - Assert.AreEqual(actual: parsedPolicy, expected: string.Empty, message: errorMessage); + Assert.AreEqual(actual: parsedPolicy.Policy, expected: string.Empty, message: errorMessage); } } diff --git a/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs b/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs index 26264741e0..8db19814d0 100644 --- a/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs +++ b/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs @@ -99,14 +99,13 @@ public void TestWildcardPolicyResolvesToEmpty(string httpMethod) AuthorizationResolver authorizationResolver = SetupAuthResolverWithWildcardOperation(); HttpContext httpContext = CreateHttpContext(httpMethod: httpMethod, clientRole: "admin"); - #pragma warning disable CS0618 // Explicitly validates the preserved legacy API contract. - Assert.AreEqual(expected: string.Empty, actual: authorizationResolver.ProcessDBPolicy( + ResolvedDatabasePolicy resolvedPolicy = authorizationResolver.ResolveDBPolicy( entityName: AuthorizationHelpers.TEST_ENTITY, roleName: "admin", operation: RestService.HttpVerbToOperations(httpVerbName: httpMethod), - httpContext: httpContext) - ); - #pragma warning restore CS0618 + httpContext: httpContext); + + Assert.AreEqual(expected: string.Empty, actual: resolvedPolicy.Policy); } /// diff --git a/src/Service.Tests/Caching/DabCacheServiceIntegrationTests.cs b/src/Service.Tests/Caching/DabCacheServiceIntegrationTests.cs index 3bf6e37012..2bcb97a931 100644 --- a/src/Service.Tests/Caching/DabCacheServiceIntegrationTests.cs +++ b/src/Service.Tests/Caching/DabCacheServiceIntegrationTests.cs @@ -717,6 +717,13 @@ private static Mock CreateMockSqlQueryStructure(string entity .Returns(entityToDatabaseObject); Mock mockMetadataProviderFactory = new(); Mock mockAuthorizationResolver = new(); + mockAuthorizationResolver + .Setup(resolver => resolver.ResolveDBPolicy( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(ResolvedDatabasePolicy.Empty); Mock mockRestRequestContext = new( entityName, new DatabaseTable()); @@ -817,6 +824,13 @@ private static SqlQueryEngine CreateQueryEngine(DabCacheService cache, string qu Mock mockMetadataProviderFactory = new(); Mock mockHttpContextAccessor = new(); Mock mockAuthorizationResolver = new(); + mockAuthorizationResolver + .Setup(resolver => resolver.ResolveDBPolicy( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(ResolvedDatabasePolicy.Empty); Mock> mockLogger = new(); Mock mockRuntimeConfigProvider = CreateMockRuntimeConfigProvider(entityName); Mock mockFilterParser = new(mockRuntimeConfigProvider.Object, mockMetadataProviderFactory.Object); diff --git a/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs b/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs index 1c29dac4fc..70d6622177 100644 --- a/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs +++ b/src/Service.Tests/UnitTests/DatabasePolicyClaimBindingUnitTests.cs @@ -5,7 +5,6 @@ using System.Collections.Generic; using System.Data; using System.Linq; -using System.Net; using System.Security.Claims; using Azure.DataApiBuilder.Auth; using Azure.DataApiBuilder.Config.DatabasePrimitives; @@ -212,60 +211,6 @@ public void BooleanClaim_CombinedWithComparison_OnlyNormalizesBareClaim() AssertParameterValues(cosmosStructure, 6, true, true); } - /// - /// Verifies existing authorization resolver implementations remain usable for static policies. - /// - [TestMethod] - public void LegacyAuthorizationResolver_StaticPolicy_RemainsSupported() - { - Mock resolver = new(); - resolver - .Setup(instance => instance.GetDBPolicyForRequest(ENTITY_NAME, ROLE_NAME, OPERATION)) - .Returns("@item.intCol eq 42"); - Mock metadataProvider = CreateMetadataProvider(); - TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver.Object); - DefaultHttpContext context = new(); - context.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER] = ROLE_NAME; - - AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( - OPERATION, - sqlStructure, - context, - resolver.Object, - metadataProvider.Object); - - Assert.AreEqual("([intCol] = @param0)", sqlStructure.GetDbPolicyForOperation(OPERATION)); - AssertParameterValues(sqlStructure, 42); - } - - /// - /// Verifies existing string-only resolver implementations fail closed for claim-bearing policies. - /// - [TestMethod] - public void LegacyAuthorizationResolver_ClaimPolicy_FailsClosed() - { - Mock resolver = new(); - resolver - .Setup(instance => instance.GetDBPolicyForRequest(ENTITY_NAME, ROLE_NAME, OPERATION)) - .Returns("@item.textCol eq @claims.value"); - Mock metadataProvider = CreateMetadataProvider(); - TestSqlQueryStructure sqlStructure = new(metadataProvider.Object, resolver.Object); - DefaultHttpContext context = new(); - context.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER] = ROLE_NAME; - - DataApiBuilderException exception = Assert.ThrowsException(() => - AuthorizationPolicyHelpers.ProcessAuthorizationPolicies( - OPERATION, - sqlStructure, - context, - resolver.Object, - metadataProvider.Object)); - - Assert.AreEqual(HttpStatusCode.Forbidden, exception.StatusCode); - Assert.AreEqual(DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed, exception.SubStatusCode); - Assert.AreEqual(0, sqlStructure.Parameters.Count); - } - /// /// Verifies string claims are promoted to the target column's numeric type before /// SQL and Cosmos parameters are created. diff --git a/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs b/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs index 5e4dc4a270..ac5de6109a 100644 --- a/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs +++ b/src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs @@ -139,9 +139,15 @@ private static SqlUpsertQueryStructure CreateUpsertStructure() => _columnMapping.TryGetValue(field, out column))) .Returns((string entity, string field, string? column) => _columnMapping.ContainsKey(field)); - // The update policy is injected directly onto the structure, so this test does not - // invoke the resolver while constructing the query structure. + // The update policy is injected directly onto the structure after construction. Mock authorizationResolver = new(); + authorizationResolver + .Setup(x => x.ResolveDBPolicy( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(ResolvedDatabasePolicy.Empty); RuntimeConfigProvider runtimeConfigProvider = TestHelper.GetRuntimeConfigProvider(TestHelper.GetRuntimeConfigLoader()); Mock metadataProviderFactory = new();