Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow empty claim value to mimic behavior of ASP.NET AuthorizationPolicyBuilder.RequireClaim method #1337

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

iam-black
Copy link

@iam-black iam-black commented Sep 9, 2020

Ocelot Feature

Proposed Changes

  • Allow empty claim value to mimic behavior of ASP.NET RequireClaim(string)
  • Refactored the code to remove warnings and messages of compiler in the same file

Docs

Relevant topics

@raman-m raman-m changed the base branch from master to develop July 17, 2023 12:59
@raman-m
Copy link
Member

raman-m commented Jul 17, 2023

Hi Ihor!
Thanks for your interest in Ocelot!

Is this PR related to some issue in the backlog?

Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ihor,
I know, reviewing of this PR should be pretty easy from my side, but the case is quite specific.
The case should be covered by unit and/or acceptance tests.

${\color{orange}The\ case\ should\ be\ covered\ by\ unit\ and/or\ acceptance\ tests.}$

So, add test(s) please!

Also, I will appreciate if you update and write clear description of user scenario.
After reading of PR title I still cannot get the user scenario... 😞
In what cases the claim value will be empty?

Hope for the soonest reply from you...

@raman-m raman-m added question Initially seen a question could become a new feature or bug or closed ;) proposal Proposal for a new functionality in Ocelot needs feedback Issue is waiting on feedback before acceptance waiting Waiting for answer to question or feedback from issue raiser labels Jul 17, 2023
@raman-m raman-m force-pushed the allow-empty-required-claim-value branch from 5b7cf6c to 1a9dc97 Compare August 9, 2023 15:56
@raman-m
Copy link
Member

raman-m commented Aug 9, 2023

The feature branch has been rebased onto ThreeMammals:develop!
Code review has started...


@iam-black Ihor,
Did you create an issue for this?
Or, is the PR related to an existing issue?
Based on the title semantic, I would say that it should be related to standard behavior of ASP.NET pipeline...
Any quick links to MS Learn docs are welcome!


I don't see develop branch in your fork! Your fork is too old.

Could you Sync fork please? So, new develop branch will occur with all top commits!

Could you add me as collaborator to your forked repo please? I will create develop branch and make it default.

@raman-m raman-m added Claims Transform Ocelot feature: Claims Transformation Authorization Ocelot feature: Authorization and removed question Initially seen a question could become a new feature or bug or closed ;) needs feedback Issue is waiting on feedback before acceptance waiting Waiting for answer to question or feedback from issue raiser labels Nov 7, 2024
@raman-m raman-m force-pushed the allow-empty-required-claim-value branch from 1a9dc97 to 094b448 Compare November 7, 2024 17:16
@raman-m raman-m changed the title Allow empty claim value to mimic behavior of ASP.NET RequireClaim(type) Allow empty claim value to mimic behavior of ASP.NET AuthorizationPolicyBuilder.RequireClaim method Nov 7, 2024
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for Code Review❕

Given the tests that have passed, I'm evaluating whether this brief continue statement could pose a security risk. 😅
Nevertheless, further testing is necessary...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Authorization Ocelot feature: Authorization Claims Transform Ocelot feature: Claims Transformation proposal Proposal for a new functionality in Ocelot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants