#746 Multiple values in static claims by asrasya-kosha · Pull Request #2131 · ThreeMammals/Ocelot

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 changes will only apply to static claims and should not affect 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

                {
                    // 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

                {
                    // 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

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).

humbberto · 2025-12-05T18:47:00Z

@raman-m any idea when this will be deployed to production?

raman-m · 2025-12-06T14:55:56Z

@humbberto commented on December 5

I have no idea!
To be fair, the PR has merge conflicts, so it can't be reviewed yet, but it's in good shape and has been added to the Winter'26 milestone. The official feature in the NuGet package should be available after that milestone is released, hopefully in March 2026. Since we don't have the team capacity to deliver sooner, I might change the strategy to roll out minor 25.* versions to get the feature into production earlier.

raman-m · 2025-12-06T15:16:49Z

TO: @asrasya-kosha

Dear Asrasya,
After a year and a half, this PR has been abandoned. As our project has evolved, many commits have been merged into the develop branch, resulting in merge conflicts with your PR.

🟨 Yellow card penalty❗

Ignoring our code reviews, our requests, and questions shows that you are not following our development process and are neglecting your own open pull request. This situation is confusing me. Please start communicating with our team; otherwise, we will have to make a decision.

raman-m · 2025-12-06T15:33:05Z

@asrasya-kosha commented on Jul 31, 2024

Note: The proposed changes will only apply to static claims and should not affect dynamic claims.

What did you mean by the term "static claims"? You meant static routes, right? Got it!
First, we could discuss the additional feature of global configuration for static routes. Once the global configuration is correctly implemented in this PR, I will help with developing this feature for dynamic routes.

asrasya-kosha · 2026-01-21T18:35:05Z

@raman-m I have updated the PR. Have a look please and let me know your comments. Thanks

coveralls · 2026-01-22T12:54:44Z

coverage: 93.51% (+0.001%) from 93.509%
when pulling c533f85 on asrasya-kosha:feature/multiple-values-in-routeclaims
into 55095d8 on ThreeMammals:develop.

raman-m · 2026-01-22T13:14:35Z

@asrasya-kosha
Why did you update the PR? I had already resolved the previous merge conflicts, but after your force push, I can’t see the diff anymore, making it impossible to tell/understand what was changed. On top of that, you introduced a bunch of unnecessary changes aka EOL Gotchas during the rebase, like adding the docker/build.sh file. I am just wondering why you added this file? And should I be congratulating you on including multiple fake changes in the Files changed tab?

Please be honest with us—do you help in developing the product, or do you create problems in the project?

What is your LinkedIn profile?

asrasya-kosha · 2026-01-22T16:46:27Z

Thanks @raman-m, You did not have to be so arrogant about this. I do not work for you and am not answerable to you either. You can keep your project and your congratulations to yourself - try to be humble from next time and get some life instead of fussing about every small thing.
Simple advice - just use the setting to hide the whitespace changes while reviewing PRs instead of writing 200 lines of KB about EOLs. C# does not care about whitespaces, am sure you know.
Force pushes can be reverted, you simply had to ask properly, in-fact if you had properly replied, I would have reverted my force push.
Anyways - all the best for your projects

raman-m changed the title ~~Multiple values in static claims~~ #746 Multiple values in static claims 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, ggnaegi and raman-m 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 Summer'25 Summer 2025 release and removed Oct'24 October 2024 release labels Oct 26, 2024

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

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

raman-m force-pushed the develop branch 6 times, most recently from 4734c9d to b0cdbd6 Compare April 3, 2025 08:13

raman-m force-pushed the develop branch 8 times, most recently from 908d84f to 0678e7a Compare April 19, 2025 15:13

raman-m added Winter'26 Winter 2026 release and removed Summer'25 Summer 2025 release labels May 27, 2025

raman-m modified the milestones: Spring'25, Summer'25 May 27, 2025

raman-m added the conflicts Feature branch has merge conflicts label Dec 6, 2025

raman-m removed conflicts Feature branch has merge conflicts high High priority labels Jan 18, 2026

static route claims support multiple values

c533f85

ThreeMammals deleted a comment from RaynaldM Jan 22, 2026

Comments

Conversation

asrasya-kosha commented Jul 31, 2024 • edited by raman-m Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes #746

Proposed Changes

Uh oh!

raman-m commented Jul 31, 2024

Uh oh!

ggnaegi commented Aug 8, 2024

Uh oh!

ggnaegi left a comment

Choose a reason for hiding this comment

Uh oh!

ggnaegi Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

RaynaldM Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

raman-m Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raman-m commented Aug 9, 2024

Uh oh!

asrasya-kosha commented Aug 20, 2024

Uh oh!

RaynaldM Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

raman-m commented Aug 30, 2024

Uh oh!

raman-m commented Aug 30, 2024

Uh oh!

humbberto commented Dec 5, 2025

Uh oh!

raman-m commented Dec 6, 2025

Uh oh!

raman-m commented Dec 6, 2025

🟨 Yellow card penalty❗

Uh oh!

raman-m commented Dec 6, 2025

Uh oh!

asrasya-kosha commented Jan 21, 2026

Uh oh!

coveralls commented Jan 22, 2026

Uh oh!

raman-m commented Jan 22, 2026

Uh oh!

asrasya-kosha commented Jan 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asrasya-kosha commented Jul 31, 2024 •

edited by raman-m

Loading

raman-m Aug 30, 2024 •

edited

Loading