From c19dbd19c548961e6e428419d2447f64879481d2 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Tue, 29 Nov 2022 19:00:10 -0800 Subject: [PATCH 1/7] Added critical clarifiying comments and refactored some methods for conciseness combined some methods as split was unnecessary) and refactored for adherance to current engine behavior to not look for legacy claimTypes with URIs (ADFS/XML claimtypes) --- .../Authorization/AuthorizationResolver.cs | 80 +++++++++---------- 1 file changed, 37 insertions(+), 43 deletions(-) diff --git a/src/Service/Authorization/AuthorizationResolver.cs b/src/Service/Authorization/AuthorizationResolver.cs index 46c736c2bf..765536b8d7 100644 --- a/src/Service/Authorization/AuthorizationResolver.cs +++ b/src/Service/Authorization/AuthorizationResolver.cs @@ -161,8 +161,13 @@ public bool AreColumnsAllowedForOperation(string entityName, string roleName, Op public string TryProcessDBPolicy(string entityName, string roleName, Operation operation, HttpContext httpContext) { string dBpolicyWithClaimTypes = GetDBPolicyForRequest(entityName, roleName, operation); - return string.IsNullOrWhiteSpace(dBpolicyWithClaimTypes) ? string.Empty : - ProcessClaimsForPolicy(dBpolicyWithClaimTypes, httpContext); + + if (string.IsNullOrWhiteSpace(dBpolicyWithClaimTypes)) + { + return string.Empty; + } + + return GetPolicyWithClaimValues(dBpolicyWithClaimTypes, GetAllUserClaims(httpContext)); } /// @@ -420,26 +425,11 @@ public IEnumerable GetAllowedExposedColumns(string entityName, string ro } /// - /// Helper method to process the given policy obtained from config, and convert it into an injectable format in - /// the HttpContext object by substituting @claim.xyz claims with their values. - /// - /// The policy to be processed. - /// HttpContext object used to extract all the claims available in the request. - /// Processed policy string that can be injected into the HttpContext object. - private static string ProcessClaimsForPolicy(string policy, HttpContext context) - { - Dictionary claimsInRequestContext = GetAllUserClaims(context); - policy = GetPolicyWithClaimValues(policy, claimsInRequestContext); - return policy; - } - - /// - /// Helper method to extract all claims available in the HttpContext object and - /// add them all in the claimsInRequestContext dictionary which is used later for quick lookup - /// of different claimTypes and their corresponding claimValues. + /// Helper method to extract all claims available in the HttpContext's user object and add the claims + /// to the claimsInRequestContext dictionary to be used for claimType -> claim lookups. /// - /// HttpContext object used to extract all the claims available in the request. - /// Dictionary to hold all the claims available in the request. + /// HttpContext object used to extract the authenticated user's claims. + /// Dictionary with claimType -> claim mappings. private static Dictionary GetAllUserClaims(HttpContext context) { Dictionary claimsInRequestContext = new(); @@ -450,7 +440,6 @@ private static Dictionary GetAllUserClaims(HttpContext context) return claimsInRequestContext; } - string roleClaimShortName = string.Empty; foreach (Claim claim in identity.Claims) { /* @@ -459,29 +448,24 @@ private static Dictionary GetAllUserClaims(HttpContext context) * claim.Value: "authz@microsoft.com" * claim.ValueType: "string" */ - // If a claim has a short type name, use it (i.e. 'roles' instead of 'http://schemas.microsoft.com/ws/2008/06/identity/claims/role') - string type = claim.Properties.TryGetValue(SHORT_CLAIM_TYPE_NAME, out string? shortName) ? shortName : claim.Type; - // Don't add roles to the claims dictionary and don't throw an exception in the case of multiple role claims, - // since a user can have multiple roles assigned and role resolution happens beforehand - if (claim.Type is not AuthenticationConfig.ROLE_CLAIM_TYPE && !claimsInRequestContext.TryAdd(type, claim)) + // At this point, only add non-role claims to the collection and only throw an exception for duplicate non-role claims. + if (claim.Type is not AuthenticationConfig.ROLE_CLAIM_TYPE && !claimsInRequestContext.TryAdd(claim.Type, claim)) { // If there are duplicate claims present in the request, return an exception. throw new DataApiBuilderException( message: $"Duplicate claims are not allowed within a request.", - statusCode: System.Net.HttpStatusCode.Forbidden, + statusCode: HttpStatusCode.Forbidden, subStatusCode: DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed ); } - - if (claim.Type is ClaimTypes.Role) - { - roleClaimShortName = type; - } } - // Add role claim to the claimsInRequestContext as it is not added above. + // Only add a role claim which represents the role context evaluated for the request. string clientRoleHeader = context.Request.Headers[CLIENT_ROLE_HEADER].ToString(); - claimsInRequestContext.Add(roleClaimShortName, new Claim(roleClaimShortName, clientRoleHeader, ClaimValueTypes.String)); + if (identity.HasClaim(type: AuthenticationConfig.ROLE_CLAIM_TYPE, value: clientRoleHeader)) + { + claimsInRequestContext.Add(AuthenticationConfig.ROLE_CLAIM_TYPE, new Claim(AuthenticationConfig.ROLE_CLAIM_TYPE, clientRoleHeader, ClaimValueTypes.String)); + } return claimsInRequestContext; } @@ -505,7 +489,7 @@ private static string GetPolicyWithClaimValues(string policy, Dictionary GetClaimValueFromClaim(claimTypeMatch, claimsInRequestContext)); //Remove occurences of @item. directives - processedPolicy = processedPolicy.Replace(AuthorizationResolver.FIELD_PREFIX, ""); + processedPolicy = processedPolicy.Replace(FIELD_PREFIX, ""); return processedPolicy; } @@ -518,7 +502,8 @@ private static string GetPolicyWithClaimValues(string policy, Dictionary Throws exception when the user does not possess the given claim. private static string GetClaimValueFromClaim(Match claimTypeMatch, Dictionary claimsInRequestContext) { - string claimType = claimTypeMatch.Value.ToString().Substring(AuthorizationResolver.CLAIM_PREFIX.Length); + // Gets from @claims. + string claimType = claimTypeMatch.Value.ToString().Substring(CLAIM_PREFIX.Length); if (claimsInRequestContext.TryGetValue(claimType, out Claim? claim)) { return GetClaimValueByDataType(claim); @@ -535,19 +520,28 @@ private static string GetClaimValueFromClaim(Match claimTypeMatch, Dictionary - /// Helper function to return the claim value enclosed within a parenthesis alongwith the required additonal - /// quotes if required. This makes sure we adhere to JSON specifications where strings are enclosed in - /// single quotes while int,bool,double etc are not. + /// Using the input parameter claim, returns the primitive literal from claim.Value enclosed within parentheses: + /// e.g. @claims.idp resolves as ('azuread') + /// e.g. @claims.iat resolves as (1537231048) + /// e.g. @claims.email_verified resolves as (true) + /// To adhere with OData 4 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. + /// Note: With many access token issuers, token claims are strings or string representations + /// of other data types such as dates and GUIDs. /// /// The claim whose value is to be returned. /// Processed claim value based on its data type. /// Exception thrown when the claim's datatype is not supported. + /// + /// + /// private static string GetClaimValueByDataType(Claim claim) { - /* An example claim would be of format: + /* An example Claim object: * claim.Type: "user_email" * claim.Value: "authz@microsoft.com" - * claim.ValueType: "string" + * claim.ValueType: "http://www.w3.org/2001/XMLSchema#string" */ switch (claim.ValueType) @@ -563,7 +557,7 @@ private static string GetClaimValueByDataType(Claim claim) // One of the claims in the request had unsupported data type. throw new DataApiBuilderException( message: "One or more claims have data types which are not supported yet.", - statusCode: System.Net.HttpStatusCode.Forbidden, + statusCode: HttpStatusCode.Forbidden, subStatusCode: DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed ); } From c5fe6187757540cf2e9a601880401209e9774317 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Wed, 30 Nov 2022 12:27:06 -0800 Subject: [PATCH 2/7] unit test updaes. --- .../AuthorizationResolverUnitTests.cs | 40 ++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs index dd966c653f..1fb46a606f 100644 --- a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs +++ b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs @@ -970,6 +970,44 @@ public void ParseValidDbPolicy(string policy, string expectedParsedPolicy) Assert.AreEqual(parsedPolicy, expectedParsedPolicy); } + /// + /// Tests authorization resolver handling of claim value types present in HttpContext.User.Claims + /// + /// + [DataTestMethod] + [DataRow("@claims.user_email ne @item.col1", ClaimValueTypes.String, DisplayName = "string")] + [DataRow("", ClaimValueTypes.Boolean, DisplayName = "bool")] + [DataRow("", ClaimValueTypes.Integer, DisplayName = "int")] + //[DataRow("", ClaimValueTypes., DisplayName = "json object")] + //[DataRow("", ClaimValueTypes.null, DisplayName = "null")] + public void DbPolicy_ClaimValueTypeParsing(string policy, string claimValueType) + { + RuntimeConfig runtimeConfig = InitRuntimeConfig( + entityName: TEST_ENTITY, + roleName: TEST_ROLE, + operation: TEST_OPERATION, + includedCols: new HashSet { "col1", "col2", "col3" }, + databasePolicy: policy + ); + AuthorizationResolver authZResolver = AuthorizationHelpers.InitAuthorizationResolver(runtimeConfig); + + Mock context = new(); + + //Add identity object to the Mock context object. + ClaimsIdentity identity = new(TEST_AUTHENTICATION_TYPE, TEST_CLAIMTYPE_NAME, AuthenticationConfig.ROLE_CLAIM_TYPE); + identity.AddClaim(new Claim("user_email", "xyz@microsoft.com", ClaimValueTypes.String)); + identity.AddClaim(new Claim("name", "Aaron", ClaimValueTypes.String)); + identity.AddClaim(new Claim("contact_no", "1234", ClaimValueTypes.Integer64)); + identity.AddClaim(new Claim("isemployee", "true", ClaimValueTypes.Boolean)); + identity.AddClaim(new Claim("emprating", "4.2", ClaimValueTypes.Double)); + ClaimsPrincipal principal = new(identity); + context.Setup(x => x.User).Returns(principal); + context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); + + string parsedPolicy = authZResolver.TryProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + Assert.IsFalse(string.IsNullOrEmpty(parsedPolicy)); + } + /// /// Test to validate that we are correctly throwing an appropriate exception when the user request /// lacks a claim required by the policy. @@ -1017,7 +1055,7 @@ public void ParseInvalidDbPolicyWithUserNotPossessingAllClaims(string policy) /// duplicate role claims are ignored, so just checks policy is parsed as expected in this case /// /// Whether we expect an exception (403 forbidden) to be thrown while parsing policy - /// Parameter list of claim types/keys to add to the claims dictionary that can be accessed with @claims + /// Parameter list of claim types/keys to add to the claims dictionary that can be accessed with @claims [DataTestMethod] [DataRow(true, AuthenticationConfig.ROLE_CLAIM_TYPE, "username", "guid", "username", DisplayName = "duplicate claim expect exception")] From f3345161c5cd8b2b758e9bb93a69b7a56ee8c56c Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Wed, 30 Nov 2022 15:05:29 -0800 Subject: [PATCH 3/7] Rename TryProcessDBPolicy to ProcessDBPolicy because it did not follow behavior of a "Try" prefixed method name. Added unit tests for each JSON ClaimValue type that would be present after SecurityJwtHandler parses valid JWT token and validates how policies are processed into OData compatible strings. --- src/Auth/IAuthorizationResolver.cs | 2 +- .../AuthorizationResolverUnitTests.cs | 78 +++++++++++++------ .../REST/RestAuthorizationHandlerUnitTests.cs | 2 +- .../Authorization/AuthorizationResolver.cs | 23 ++++-- .../Resolvers/AuthorizationPolicyHelpers.cs | 2 +- src/Service/Services/RestService.cs | 2 +- 6 files changed, 75 insertions(+), 34 deletions(-) diff --git a/src/Auth/IAuthorizationResolver.cs b/src/Auth/IAuthorizationResolver.cs index f956338948..f0d1b4ea69 100644 --- a/src/Auth/IAuthorizationResolver.cs +++ b/src/Auth/IAuthorizationResolver.cs @@ -63,7 +63,7 @@ public interface IAuthorizationResolver /// 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 TryProcessDBPolicy(string entityName, string roleName, Operation operation, HttpContext httpContext); + public string ProcessDBPolicy(string entityName, string roleName, Operation 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/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs index 1fb46a606f..31e0fe417d 100644 --- a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs +++ b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs @@ -1,5 +1,7 @@ #nullable enable +using System; using System.Collections.Generic; +using System.Globalization; using System.Linq; using System.Net; using System.Security.Claims; @@ -10,6 +12,7 @@ using Azure.DataApiBuilder.Service.Exceptions; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Primitives; +using Microsoft.IdentityModel.JsonWebTokens; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; using PermissionOperation = Azure.DataApiBuilder.Config.PermissionOperation; @@ -966,46 +969,75 @@ 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.TryProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); Assert.AreEqual(parsedPolicy, expectedParsedPolicy); } /// - /// Tests authorization resolver handling of claim value types present in HttpContext.User.Claims + /// Tests authorization policy processing mechanism by validating value type compatibility + /// of claims present in HttpContext.User.Claims. /// - /// + /// Claim.ValueType which is a string, by definition. + /// Claim.Value which is a string, by definition. + /// Whether Claim.ValueType is supported by DAB engine + /// + /// + #pragma warning disable format [DataTestMethod] - [DataRow("@claims.user_email ne @item.col1", ClaimValueTypes.String, DisplayName = "string")] - [DataRow("", ClaimValueTypes.Boolean, DisplayName = "bool")] - [DataRow("", ClaimValueTypes.Integer, DisplayName = "int")] - //[DataRow("", ClaimValueTypes., DisplayName = "json object")] - //[DataRow("", ClaimValueTypes.null, DisplayName = "null")] - public void DbPolicy_ClaimValueTypeParsing(string policy, string claimValueType) + [DataRow(ClaimValueTypes.String, "StringLiteral", true, DisplayName = "string")] + [DataRow(ClaimValueTypes.Boolean, "true", true, DisplayName = "bool")] + [DataRow(ClaimValueTypes.Integer, "65535", true, DisplayName = "short")] + [DataRow(ClaimValueTypes.Integer, "-2147483648", true, DisplayName = "int - Scenario 1")] + [DataRow(ClaimValueTypes.Integer32, "2147483647", true, DisplayName = "int - Scenario 2")] + [DataRow(ClaimValueTypes.Integer64, "9223372036854775807", true, DisplayName = "long")] + [DataRow(ClaimValueTypes.UInteger32, "4294967295", true, DisplayName = "uint")] + [DataRow(ClaimValueTypes.UInteger64, "18446744073709551615", true, DisplayName = "ulong")] + [DataRow(ClaimValueTypes.Double, "12.34", true, DisplayName = "decimal")] + [DataRow(ClaimValueTypes.Double, "12.345", true, DisplayName = "double")] + [DataRow(JsonClaimValueTypes.JsonNull, "null", true, DisplayName = "Json null literal")] + [DataRow(ClaimValueTypes.DateTime, "2022-11-30T22:57:57.5847834Z", false, DisplayName = "DateTime")] + [DataRow(JsonClaimValueTypes.Json, "{\"\"ext1\"\":\"\"ext1Value\"\"}", false, DisplayName = "Json object")] + [DataRow(JsonClaimValueTypes.JsonArray, "[{\"\"ext1\"\":\"\"ext1Value\"\"}]", false, DisplayName = "Json array")] + #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 policyDefinition = "@claims.testClaim eq @item.col1"; + RuntimeConfig runtimeConfig = InitRuntimeConfig( entityName: TEST_ENTITY, roleName: TEST_ROLE, operation: TEST_OPERATION, - includedCols: new HashSet { "col1", "col2", "col3" }, - databasePolicy: policy - ); + includedCols: new HashSet { "col1" }, + databasePolicy: policyDefinition); + AuthorizationResolver authZResolver = AuthorizationHelpers.InitAuthorizationResolver(runtimeConfig); Mock context = new(); //Add identity object to the Mock context object. ClaimsIdentity identity = new(TEST_AUTHENTICATION_TYPE, TEST_CLAIMTYPE_NAME, AuthenticationConfig.ROLE_CLAIM_TYPE); - identity.AddClaim(new Claim("user_email", "xyz@microsoft.com", ClaimValueTypes.String)); - identity.AddClaim(new Claim("name", "Aaron", ClaimValueTypes.String)); - identity.AddClaim(new Claim("contact_no", "1234", ClaimValueTypes.Integer64)); - identity.AddClaim(new Claim("isemployee", "true", ClaimValueTypes.Boolean)); - identity.AddClaim(new Claim("emprating", "4.2", ClaimValueTypes.Double)); + identity.AddClaim(new Claim("testClaim", claimValue, claimValueType)); + ClaimsPrincipal principal = new(identity); context.Setup(x => x.User).Returns(principal); context.Setup(x => x.Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]).Returns(TEST_ROLE); - string parsedPolicy = authZResolver.TryProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); - Assert.IsFalse(string.IsNullOrEmpty(parsedPolicy)); + try + { + string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + Assert.IsTrue(supportedValueType); + Assert.AreEqual(expectedPolicy, parsedPolicy); + } + catch(DataApiBuilderException ex) + { + Assert.IsFalse(supportedValueType); + Assert.AreEqual(expected: AuthorizationResolver.INVALID_POLICY_CLAIM_MESSAGE, actual: ex.Message); + } } /// @@ -1041,7 +1073,7 @@ public void ParseInvalidDbPolicyWithUserNotPossessingAllClaims(string policy) try { - authZResolver.TryProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); } catch (DataApiBuilderException ex) { @@ -1093,7 +1125,7 @@ public void ParsePolicyWithDuplicateUserClaims(bool exceptionExpected, params st { try { - authZResolver.TryProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); Assert.Fail(); } catch (DataApiBuilderException ex) @@ -1106,7 +1138,7 @@ public void ParsePolicyWithDuplicateUserClaims(bool exceptionExpected, params st { // If the role claim was the only duplicate, simply verify policy parsed as expected string expectedPolicy = $"('{defaultClaimValue}') eq 1"; - string parsedPolicy = authZResolver.TryProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); + string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, TEST_ROLE, TEST_OPERATION, context.Object); Assert.AreEqual(expected: expectedPolicy, actual: parsedPolicy); } } @@ -1156,7 +1188,7 @@ 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.TryProcessDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object); + string parsedPolicy = authZResolver.ProcessDBPolicy(TEST_ENTITY, clientRole, requestOperation, context.Object); string errorMessage = "TryProcessDBPolicy returned unexpected value."; if (expectPolicy) { diff --git a/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs b/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs index 4b298f78c5..31d6d29a74 100644 --- a/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs +++ b/src/Service.Tests/Authorization/REST/RestAuthorizationHandlerUnitTests.cs @@ -96,7 +96,7 @@ public void TestWildcardPolicyResolvesToEmpty(string httpMethod) AuthorizationResolver authorizationResolver = SetupAuthResolverWithWildcardOperation(); HttpContext httpContext = CreateHttpContext(httpMethod: httpMethod, clientRole: "admin"); - Assert.AreEqual(expected: string.Empty, actual: authorizationResolver.TryProcessDBPolicy( + Assert.AreEqual(expected: string.Empty, actual: authorizationResolver.ProcessDBPolicy( entityName: AuthorizationHelpers.TEST_ENTITY, roleName: "admin", operation: RestService.HttpVerbToOperations(httpVerbName: httpMethod), diff --git a/src/Service/Authorization/AuthorizationResolver.cs b/src/Service/Authorization/AuthorizationResolver.cs index 765536b8d7..8f45809613 100644 --- a/src/Service/Authorization/AuthorizationResolver.cs +++ b/src/Service/Authorization/AuthorizationResolver.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.IdentityModel.Tokens.Jwt; using System.Linq; using System.Net; using System.Security.Claims; @@ -32,7 +33,7 @@ public class AuthorizationResolver : IAuthorizationResolver public const string CLIENT_ROLE_HEADER = "X-MS-API-ROLE"; public const string ROLE_ANONYMOUS = "anonymous"; public const string ROLE_AUTHENTICATED = "authenticated"; - private const string SHORT_CLAIM_TYPE_NAME = "http://schemas.xmlsoap.org/ws/2005/05/identity/claimproperties/ShortTypeName"; + public const string INVALID_POLICY_CLAIM_MESSAGE = "One or more claims referenced in an authorization policy have value types which are not supported."; public Dictionary EntityPermissionsMap { get; private set; } = new(); @@ -158,7 +159,7 @@ public bool AreColumnsAllowedForOperation(string entityName, string roleName, Op } /// - public string TryProcessDBPolicy(string entityName, string roleName, Operation operation, HttpContext httpContext) + public string ProcessDBPolicy(string entityName, string roleName, Operation operation, HttpContext httpContext) { string dBpolicyWithClaimTypes = GetDBPolicyForRequest(entityName, roleName, operation); @@ -453,7 +454,7 @@ private static Dictionary GetAllUserClaims(HttpContext context) { // If there are duplicate claims present in the request, return an exception. throw new DataApiBuilderException( - message: $"Duplicate claims are not allowed within a request.", + message: "Duplicate claims are not allowed within a request.", statusCode: HttpStatusCode.Forbidden, subStatusCode: DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed ); @@ -521,14 +522,16 @@ private static string GetClaimValueFromClaim(Match claimTypeMatch, Dictionary /// Using the input parameter claim, returns the primitive literal from claim.Value enclosed within parentheses: - /// e.g. @claims.idp resolves as ('azuread') - /// e.g. @claims.iat resolves as (1537231048) - /// e.g. @claims.email_verified resolves as (true) + /// 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 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. /// 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 mechanism that + /// created the claim does not explicitly provide a value type. /// /// The claim whose value is to be returned. /// Processed claim value based on its data type. @@ -536,6 +539,7 @@ private static string GetClaimValueFromClaim(Match claimTypeMatch, Dictionary /// /// + /// private static string GetClaimValueByDataType(Claim claim) { /* An example Claim object: @@ -549,14 +553,19 @@ private static string GetClaimValueByDataType(Claim claim) case ClaimValueTypes.String: return $"('{claim.Value}')"; 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: "One or more claims have data types which are not supported yet.", + message: INVALID_POLICY_CLAIM_MESSAGE, statusCode: HttpStatusCode.Forbidden, subStatusCode: DataApiBuilderException.SubStatusCodes.AuthorizationCheckFailed ); diff --git a/src/Service/Resolvers/AuthorizationPolicyHelpers.cs b/src/Service/Resolvers/AuthorizationPolicyHelpers.cs index 0761e47a05..70821e554c 100644 --- a/src/Service/Resolvers/AuthorizationPolicyHelpers.cs +++ b/src/Service/Resolvers/AuthorizationPolicyHelpers.cs @@ -44,7 +44,7 @@ public static void ProcessAuthorizationPolicies( string clientRoleHeader = roleHeaderValue.ToString(); - string dbQueryPolicy = authorizationResolver.TryProcessDBPolicy( + string dbQueryPolicy = authorizationResolver.ProcessDBPolicy( queryStructure.EntityName, clientRoleHeader, operationType, diff --git a/src/Service/Services/RestService.cs b/src/Service/Services/RestService.cs index fab1ec867b..b658918b15 100644 --- a/src/Service/Services/RestService.cs +++ b/src/Service/Services/RestService.cs @@ -161,7 +161,7 @@ RuntimeConfigProvider runtimeConfigProvider string role = GetHttpContext().Request.Headers[AuthorizationResolver.CLIENT_ROLE_HEADER]; Operation operation = HttpVerbToOperations(GetHttpContext().Request.Method); - string dbPolicy = _authorizationResolver.TryProcessDBPolicy(entityName, role, operation, GetHttpContext()); + string dbPolicy = _authorizationResolver.ProcessDBPolicy(entityName, role, operation, GetHttpContext()); if (!string.IsNullOrEmpty(dbPolicy)) { // Since dbPolicy is nothing but filters to be added by virtue of database policy, we prefix it with From 98256240bce1a9820307a8df5ad0ff28a58c6f71 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Wed, 30 Nov 2022 17:01:07 -0800 Subject: [PATCH 4/7] update failure messaging for unit tests. --- .../Authorization/AuthorizationResolverUnitTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs index 31e0fe417d..fa42d9a6e4 100644 --- a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs +++ b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs @@ -1035,8 +1035,8 @@ public void DbPolicy_ClaimValueTypeParsing(string claimValueType, string claimVa } catch(DataApiBuilderException ex) { - Assert.IsFalse(supportedValueType); - Assert.AreEqual(expected: AuthorizationResolver.INVALID_POLICY_CLAIM_MESSAGE, actual: ex.Message); + Assert.IsFalse(supportedValueType, message: ex.Message); + Assert.AreEqual(expected: AuthorizationResolver.INVALID_POLICY_CLAIM_MESSAGE, actual: ex.Message, message: ex.Message); } } From 3f30a9df4696cb2b2130f6f05bbbcb57e8b219ba Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Wed, 30 Nov 2022 17:02:01 -0800 Subject: [PATCH 5/7] Tidy up test names, add tests for double (C# CLR) -> float (SQL type). --- .../Unittests/ODataASTVisitorUnitTests.cs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Service.Tests/Unittests/ODataASTVisitorUnitTests.cs b/src/Service.Tests/Unittests/ODataASTVisitorUnitTests.cs index c768903899..78492840ec 100644 --- a/src/Service.Tests/Unittests/ODataASTVisitorUnitTests.cs +++ b/src/Service.Tests/Unittests/ODataASTVisitorUnitTests.cs @@ -102,9 +102,18 @@ public void VisitorLeftFieldGreaterThanRightNullFilterTest() ); } + /// + /// Tests processed authorization policies (@claims.claimName eq @item.columnName) -> ('UserName' eq ScreenName) + /// against the custom OData Filter parser resolver ClaimsTypeDataUriResolver. + /// The columns xyz_types are sourced from type_table. + /// + /// Filter parser input, the processed authorization policy + /// Whether an OData Filter parser error is expected + /// [DataTestMethod] // Constant on left side and OData EDM object on right side of binary operator. (L->R) - [DataRow("'1' eq int_types", false, DisplayName = "L->R: Cast token claim of type string to integer, left to right ")] + [DataRow("'1' eq int_types", false, DisplayName = "L->R: Cast token claim of type string to integer")] + [DataRow("12.24 eq float_types", false, DisplayName = "L->R: Cast token claim of type double to (SQL) float")] [DataRow("'13B4F4EC-C45B-46EC-99F2-77BC22A256A7' eq guid_types", false, DisplayName = "L->R: Cast token claim of type string to GUID")] [DataRow("'true' eq boolean_types", false, DisplayName = "L->R: Cast token claim of type string to bool (true)")] [DataRow("'false' eq boolean_types", false, DisplayName = "L->R: Cast token claim of type string to bool (false)")] @@ -112,6 +121,7 @@ public void VisitorLeftFieldGreaterThanRightNullFilterTest() [DataRow("true eq string_types", false, DisplayName = "L->R: Cast token claim of type bool to string")] // Constant on right side and OData EDM object on left side of binary operator. (R->L) [DataRow("int_types eq '1'", false, DisplayName = "R->L: Cast token claim of type string to integer")] + [DataRow("float_types eq 12.24", false, DisplayName = "R->L: Cast token claim of type double to (SQL) float")] [DataRow("guid_types eq '13B4F4EC-C45B-46EC-99F2-77BC22A256A7'", false, DisplayName = "R->L: Cast token claim of type string to GUID")] [DataRow("boolean_types eq 'true'", false, DisplayName = "R->L: Cast token claim of type string to bool (true)")] [DataRow("boolean_types eq 'false'", false, DisplayName = "R->L: Cast token claim of type string to bool (false)")] @@ -143,7 +153,7 @@ public void CustomODataUriParserResolverTest(string resolvedAuthZPolicyText, boo catch (Exception e) when (e is DataApiBuilderException || e is ODataException) { Assert.IsTrue(errorExpected, message: "Filter clause creation was not expected to fail."); - Assert.IsTrue(e.Message.Contains(expectedErrorMessageFragment)); + Assert.IsTrue(e.Message.Contains(expectedErrorMessageFragment), message: e.Message); } } #endregion From ba666681f6c6f9ec7d20fcc4c37d710fd1328c59 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Thu, 1 Dec 2022 17:49:12 -0800 Subject: [PATCH 6/7] Add comments and remove double parsing as it is handled better by base class. --- .../Unittests/ODataASTVisitorUnitTests.cs | 7 +++++-- src/Service/Authorization/AuthorizationResolver.cs | 10 +++++----- src/Service/Parsers/ClaimsTypeDataUriResolver.cs | 5 ++++- src/Service/Parsers/FilterParser.cs | 4 ++-- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/Service.Tests/Unittests/ODataASTVisitorUnitTests.cs b/src/Service.Tests/Unittests/ODataASTVisitorUnitTests.cs index 78492840ec..ae5c5c9321 100644 --- a/src/Service.Tests/Unittests/ODataASTVisitorUnitTests.cs +++ b/src/Service.Tests/Unittests/ODataASTVisitorUnitTests.cs @@ -106,6 +106,9 @@ public void VisitorLeftFieldGreaterThanRightNullFilterTest() /// Tests processed authorization policies (@claims.claimName eq @item.columnName) -> ('UserName' eq ScreenName) /// against the custom OData Filter parser resolver ClaimsTypeDataUriResolver. /// The columns xyz_types are sourced from type_table. + /// Constant values/literals in expressions are parsed by Microsoft.OData.UriParser.ExpressionLexer which + /// attempts to resolve value to its OData(EdmPrimitiveTypeKind) via + /// https://github.com/OData/odata.net/blob/f3bf65a74a7ed4028ff8074ccae31e4c2019772d/src/Microsoft.OData.Core/UriParser/ExpressionLexer.cs#L1206-L1221 /// /// Filter parser input, the processed authorization policy /// Whether an OData Filter parser error is expected @@ -113,7 +116,7 @@ public void VisitorLeftFieldGreaterThanRightNullFilterTest() [DataTestMethod] // Constant on left side and OData EDM object on right side of binary operator. (L->R) [DataRow("'1' eq int_types", false, DisplayName = "L->R: Cast token claim of type string to integer")] - [DataRow("12.24 eq float_types", false, DisplayName = "L->R: Cast token claim of type double to (SQL) float")] + [DataRow("12.24 eq float_types", false, DisplayName = "L->R: Cast token claim of type single to type double (CLR) which maps to (SQL) float")] [DataRow("'13B4F4EC-C45B-46EC-99F2-77BC22A256A7' eq guid_types", false, DisplayName = "L->R: Cast token claim of type string to GUID")] [DataRow("'true' eq boolean_types", false, DisplayName = "L->R: Cast token claim of type string to bool (true)")] [DataRow("'false' eq boolean_types", false, DisplayName = "L->R: Cast token claim of type string to bool (false)")] @@ -121,7 +124,7 @@ public void VisitorLeftFieldGreaterThanRightNullFilterTest() [DataRow("true eq string_types", false, DisplayName = "L->R: Cast token claim of type bool to string")] // Constant on right side and OData EDM object on left side of binary operator. (R->L) [DataRow("int_types eq '1'", false, DisplayName = "R->L: Cast token claim of type string to integer")] - [DataRow("float_types eq 12.24", false, DisplayName = "R->L: Cast token claim of type double to (SQL) float")] + [DataRow("float_types eq 12.24", false, DisplayName = "R->L: Cast token claim of type single to type double (CLR) which maps to (SQL) float")] [DataRow("guid_types eq '13B4F4EC-C45B-46EC-99F2-77BC22A256A7'", false, DisplayName = "R->L: Cast token claim of type string to GUID")] [DataRow("boolean_types eq 'true'", false, DisplayName = "R->L: Cast token claim of type string to bool (true)")] [DataRow("boolean_types eq 'false'", false, DisplayName = "R->L: Cast token claim of type string to bool (false)")] diff --git a/src/Service/Authorization/AuthorizationResolver.cs b/src/Service/Authorization/AuthorizationResolver.cs index 8f45809613..ceba4d9d01 100644 --- a/src/Service/Authorization/AuthorizationResolver.cs +++ b/src/Service/Authorization/AuthorizationResolver.cs @@ -507,7 +507,7 @@ private static string GetClaimValueFromClaim(Match claimTypeMatch, Dictionary /// The claim whose value is to be returned. /// Processed claim value based on its data type. @@ -540,7 +540,7 @@ private static string GetClaimValueFromClaim(Match claimTypeMatch, Dictionary /// /// - private static string GetClaimValueByDataType(Claim claim) + private static string GetODataCompliantClaimValue(Claim claim) { /* An example Claim object: * claim.Type: "user_email" diff --git a/src/Service/Parsers/ClaimsTypeDataUriResolver.cs b/src/Service/Parsers/ClaimsTypeDataUriResolver.cs index 672635a940..0c6f3740c9 100644 --- a/src/Service/Parsers/ClaimsTypeDataUriResolver.cs +++ b/src/Service/Parsers/ClaimsTypeDataUriResolver.cs @@ -47,7 +47,10 @@ public override void PromoteBinaryOperandTypes(BinaryOperatorKind binaryOperator } /// - /// Uses type specific parsers to attempt converting the supplied node to a new ConstantNode of type targetType. + /// 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. + /// When no explicit conversion is provided, no conversion occurs here and the conversion attempt is deferred + /// to base.PromoteBinaryOperandTypes(). /// /// Primitive type (string, bool, int, etc.) of the primary node's value. /// Node representing a constant value which should be converted to a ConstantNode of type targetType. diff --git a/src/Service/Parsers/FilterParser.cs b/src/Service/Parsers/FilterParser.cs index c800f0e15e..5331cfcd15 100644 --- a/src/Service/Parsers/FilterParser.cs +++ b/src/Service/Parsers/FilterParser.cs @@ -33,7 +33,7 @@ public void BuildModel(ISqlMetadataProvider sqlMetadataProvider) /// An AST FilterClause that represents the filter portion of the WHERE clause. public FilterClause GetFilterClause(string filterQueryString, string resourcePath, ODataUriResolver? customResolver = null) { - if (_model == null) + if (_model is null) { throw new DataApiBuilderException( @@ -45,7 +45,7 @@ public FilterClause GetFilterClause(string filterQueryString, string resourcePat try { Uri relativeUri = new(resourcePath + '/' + filterQueryString, UriKind.Relative); - ODataUriParser parser = new(_model!, relativeUri); + ODataUriParser parser = new(_model, relativeUri); if (customResolver is not null) { From 38fb4499604f9b657a9a8dd7f1e0b259cdbab604 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Thu, 1 Dec 2022 22:49:06 -0800 Subject: [PATCH 7/7] Fix spacing and usings --- .../Authorization/AuthorizationResolverUnitTests.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs index fa42d9a6e4..4120279d5b 100644 --- a/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs +++ b/src/Service.Tests/Authorization/AuthorizationResolverUnitTests.cs @@ -1,7 +1,5 @@ #nullable enable -using System; using System.Collections.Generic; -using System.Globalization; using System.Linq; using System.Net; using System.Security.Claims; @@ -1004,7 +1002,7 @@ public void DbPolicy_ClaimValueTypeParsing(string claimValueType, string claimVa // 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 odataClaimValue = (claimValueType == ClaimValueTypes.String) ? "'" + claimValue + "'" : claimValue; string expectedPolicy = "(" + odataClaimValue + ") eq col1"; string policyDefinition = "@claims.testClaim eq @item.col1"; @@ -1033,7 +1031,7 @@ public void DbPolicy_ClaimValueTypeParsing(string claimValueType, string claimVa Assert.IsTrue(supportedValueType); Assert.AreEqual(expectedPolicy, parsedPolicy); } - catch(DataApiBuilderException ex) + catch (DataApiBuilderException ex) { Assert.IsFalse(supportedValueType, message: ex.Message); Assert.AreEqual(expected: AuthorizationResolver.INVALID_POLICY_CLAIM_MESSAGE, actual: ex.Message, message: ex.Message);