#746 Multiple values in static claims #2131

asrasya-kosha · 2024-07-31T17:33:01Z

Closes #746

Multiple values for single key in RouteClaimsRequirement #746

Proposed Changes

Changed RouteClaimsRequirements to accept multiple values for a single key
Changed ClaimsAuthorizer to validate with any of the given values for a key for a static claim
Adjusted ClaimsAuthorizer for dynamic claims to only accept the first value for a given key
Added test cases

Note : The proposed changed will only work for static claims. There shouldn't be any effect on the dynamic claims.

raman-m · 2024-07-31T18:04:30Z

Thank you for the PR!
Please note that the development process mandates the inclusion of both unit and acceptance tests for every new feature and bug fix. New features should be accompanied by tests that cover the new functionality. Additionally, merely fixing existing tests does not suffice to warrant approval of the PR.

ggnaegi · 2024-08-08T11:33:57Z

@raman-m reviewing it right now...

ggnaegi

@raman-m @asrasya-kosha Ok for me, but the ClaimsAuthorizer code is difficult to read. I think we could probably refactor it a bit.

ggnaegi · 2024-08-08T12:33:19Z

src/Ocelot/Authorization/ClaimsAuthorizer.cs

@@ -32,7 +32,7 @@ List<PlaceholderNameAndValue> urlPathPlaceholderNameAndValues
                if (values.Data != null)
                {
                    // dynamic claim
-                    var match = Regex.Match(required.Value, @"^{(?<variable>.+)}$");
+                    var match = Regex.Match(required.Value!.FirstOrDefault() ?? string.Empty, "^{(?<variable>.+)}$");


That's not your code, I agree, Maybe we could use a compiled Regex and enhance security by specifying a timeout, 10 seconds...

[GeneratedRegex("^{(?<variable>.+)}$", RegexOptions.Compiled, 10000)] private static partial Regex DynamicClaimRegex();

in GenerateRegex, RegexOptions.Compiled is ignored

We have well known open PR by Mohsen who followed Regex best practices, see #1348

raman-m · 2024-08-09T10:29:21Z

@ggnaegi reviewed on Aug 8

Thanks for code review! But you know I will return to feature PRs after v23.3 Hotfixes release.
Also this PR has a lot of fake changes probably because of line endings problem so it's hard to read real changes.

asrasya-kosha · 2024-08-20T18:12:51Z

Thank you for the PR! Please note that the development process mandates the inclusion of both unit and acceptance tests for every new feature and bug fix. New features should be accompanied by tests that cover the new functionality. Additionally, merely fixing existing tests does not suffice to warrant approval of the PR.

@raman-m I'll add acceptance tests and new unit tests (I thought I added them but I'll have a look once again) this weekend and update the PR. The line ending changes are perhaps a GitHub issue? because I don't see them in rider when I do sa side by side compare. Thanks

RaynaldM · 2024-08-21T06:23:42Z

src/Ocelot/Authorization/ClaimsAuthorizer.cs

@@ -32,7 +32,7 @@ List<PlaceholderNameAndValue> urlPathPlaceholderNameAndValues
                if (values.Data != null)
                {
                    // dynamic claim
-                    var match = Regex.Match(required.Value, @"^{(?<variable>.+)}$");
+                    var match = Regex.Match(required.Value!.FirstOrDefault() ?? string.Empty, "^{(?<variable>.+)}$");


in GenerateRegex, RegexOptions.Compiled is ignored

RaynaldM · 2024-08-21T06:24:19Z

src/Ocelot/Authorization/IClaimsAuthorizer.cs

@@ -8,7 +8,7 @@ public interface IClaimsAuthorizer
    {
        Response<bool> Authorize(
            ClaimsPrincipal claimsPrincipal,
-            Dictionary<string, string> routeClaimsRequirement,
+            Dictionary<string, string[]> routeClaimsRequirement,


Suggested change

Dictionary<string, string[]> routeClaimsRequirement,

IDictionary<string, string[]> routeClaimsRequirement,

RaynaldM · 2024-08-21T06:24:46Z

src/Ocelot/Configuration/Builder/DownstreamRouteBuilder.cs

@@ -126,7 +126,7 @@ public DownstreamRouteBuilder WithClaimsToClaims(List<ClaimToThing> input)
        return this;
    }

-    public DownstreamRouteBuilder WithRouteClaimsRequirement(Dictionary<string, string> input)
+    public DownstreamRouteBuilder WithRouteClaimsRequirement(Dictionary<string, string[]> input)


Suggested change

public DownstreamRouteBuilder WithRouteClaimsRequirement(Dictionary<string, string[]> input)

public DownstreamRouteBuilder WithRouteClaimsRequirement(IDictionary<string, string[]> input)

RaynaldM · 2024-08-21T06:25:49Z

src/Ocelot/Configuration/File/FileRoute.cs

        public int Priority { get; set; }
        public FileQoSOptions QoSOptions { get; set; }
        public FileRateLimitRule RateLimitOptions { get; set; }
        public string RequestIdKey { get; set; }
-        public Dictionary<string, string> RouteClaimsRequirement { get; set; }
+        public Dictionary<string, string[]> RouteClaimsRequirement { get; set; }


Suggested change

public Dictionary<string, string[]> RouteClaimsRequirement { get; set; }

public IDictionary<string, string[]> RouteClaimsRequirement { get; set; }

RaynaldM · 2024-08-21T06:26:28Z

src/Ocelot/Configuration/DownstreamRoute.cs

@@ -100,7 +100,7 @@ public DownstreamRoute(
        public CacheOptions CacheOptions { get; }
        public LoadBalancerOptions LoadBalancerOptions { get; }
        public RateLimitOptions RateLimitOptions { get; }
-        public Dictionary<string, string> RouteClaimsRequirement { get; }
+        public Dictionary<string, string[]> RouteClaimsRequirement { get; }


Suggested change

public Dictionary<string, string[]> RouteClaimsRequirement { get; }

public IDictionary<string, string[]> RouteClaimsRequirement { get; }

RaynaldM · 2024-08-21T06:26:51Z

src/Ocelot/Configuration/Builder/DownstreamRouteBuilder.cs

@@ -13,7 +13,7 @@ public class DownstreamRouteBuilder
    private bool _isAuthenticated;
    private List<ClaimToThing> _claimsToHeaders;
    private List<ClaimToThing> _claimToClaims;
-    private Dictionary<string, string> _routeClaimRequirement;
+    private Dictionary<string, string[]> _routeClaimRequirement;


Suggested change

private Dictionary<string, string[]> _routeClaimRequirement;

private IDictionary<string, string[]> _routeClaimRequirement;

RaynaldM · 2024-08-21T06:27:23Z

src/Ocelot/Authorization/ClaimsAuthorizer.cs

@@ -16,7 +16,7 @@ public ClaimsAuthorizer(IClaimsParser claimsParser)

        public Response<bool> Authorize(
            ClaimsPrincipal claimsPrincipal,
-            Dictionary<string, string> routeClaimsRequirement,
+            Dictionary<string, string[]> routeClaimsRequirement,


Suggested change

Dictionary<string, string[]> routeClaimsRequirement,

IDictionary<string, string[]> routeClaimsRequirement,

raman-m · 2024-08-30T10:22:34Z

@asrasya-kosha Please work on code review issues.
Also, we have to add at least one acceptance test.

raman-m · 2024-08-30T10:27:42Z

@asrasya-kosha commented on Aug 20, 2024

The line ending changes are perhaps a GitHub issue?

The changes in line endings are not due to GitHub; rather, they are caused by your IDE. GitHub simply identifies the different line endings and highlights those lines.

because I don't see them in Rider when I do so side by side compare.

We have Visual Studio IDE settings for line endings here: .editorconfig#L19-L21 (common settings), *.{cs,vb} (C# CRLF ending). However, there is no settings file for Rider IDE.
The main issue is the .gitattributes file for Git, which means our settings do not override EOL and commits are made as-is (both CRLF for Windows and LF for Linux). Local/global Git settings may sometimes override EOL in this file.
Since you're using Rider IDE, ensure that your Git environment does not override EOL (a mixed mode is preferred).

removed redundant tests

asrasya-kosha mentioned this pull request Jul 31, 2024

Multiple values for single key in RouteClaimsRequirement #746

Open

raman-m changed the title ~~Multiple values in static claims~~ #746 Multiple values in static claims Jul 31, 2024

raman-m assigned asrasya-kosha Jul 31, 2024

raman-m added feature A new feature Configuration Ocelot feature: Configuration Authorization Ocelot feature: Authorization Oct'24 October 2024 release labels Jul 31, 2024

raman-m added this to the Summer'24 milestone Jul 31, 2024

raman-m added the high High priority label Jul 31, 2024

raman-m requested review from RaynaldM, raman-m and ggnaegi July 31, 2024 17:57

ggnaegi reviewed Aug 8, 2024

View reviewed changes

RaynaldM approved these changes Aug 21, 2024

View reviewed changes

raman-m force-pushed the feature/multiple-values-in-routeclaims branch from 8204bbd to d2b9a2b Compare October 18, 2024 16:59

raman-m added Autumn'24 Autumn 2024 release and removed Oct'24 October 2024 release labels Oct 26, 2024

raman-m modified the milestones: October'24, Autumn'24 Oct 26, 2024

Multiple values in static claims

a6eae47

removed redundant tests

raman-m force-pushed the feature/multiple-values-in-routeclaims branch from d2b9a2b to a6eae47 Compare November 2, 2024 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#746 Multiple values in static claims #2131

#746 Multiple values in static claims #2131

asrasya-kosha commented Jul 31, 2024 •

edited by raman-m

Loading

raman-m commented Jul 31, 2024

ggnaegi commented Aug 8, 2024

ggnaegi left a comment

ggnaegi Aug 8, 2024

RaynaldM Aug 21, 2024

raman-m Aug 30, 2024 •

edited

Loading

raman-m commented Aug 9, 2024

asrasya-kosha commented Aug 20, 2024

RaynaldM Aug 21, 2024

RaynaldM Aug 21, 2024

RaynaldM Aug 21, 2024

RaynaldM Aug 21, 2024

RaynaldM Aug 21, 2024

RaynaldM Aug 21, 2024

RaynaldM Aug 21, 2024

raman-m commented Aug 30, 2024

raman-m commented Aug 30, 2024

	Dictionary<string, string[]> routeClaimsRequirement,
	IDictionary<string, string[]> routeClaimsRequirement,

	public DownstreamRouteBuilder WithRouteClaimsRequirement(Dictionary<string, string[]> input)
	public DownstreamRouteBuilder WithRouteClaimsRequirement(IDictionary<string, string[]> input)

	public Dictionary<string, string[]> RouteClaimsRequirement { get; set; }
	public IDictionary<string, string[]> RouteClaimsRequirement { get; set; }

	private Dictionary<string, string[]> _routeClaimRequirement;
	private IDictionary<string, string[]> _routeClaimRequirement;

#746 Multiple values in static claims #2131

Are you sure you want to change the base?

#746 Multiple values in static claims #2131

Conversation

asrasya-kosha commented Jul 31, 2024 • edited by raman-m Loading

Closes #746

Proposed Changes

raman-m commented Jul 31, 2024

ggnaegi commented Aug 8, 2024

ggnaegi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m commented Aug 9, 2024

asrasya-kosha commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raman-m commented Aug 30, 2024

raman-m commented Aug 30, 2024

asrasya-kosha commented Jul 31, 2024 •

edited by raman-m

Loading

raman-m Aug 30, 2024 •

edited

Loading