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

#740 #1580 Support multiple authentication schemes in one route #1870

Merged
merged 66 commits into from
Feb 5, 2024
Merged

#740 #1580 Support multiple authentication schemes in one route #1870

merged 66 commits into from
Feb 5, 2024

Conversation

MayorSheFF
Copy link
Contributor

@MayorSheFF MayorSheFF commented Dec 28, 2023

Closes #1580 #740

Proposed Changes

  • opportunity to have the several authentication provider keys in Routes what allows to authenticate requests via different providers

Igor-Polishchuk and others added 30 commits June 5, 2023 10:30
Co-authored-by: Raynald Messié <redbird_project@yahoo.fr>
Co-authored-by: Raynald Messié <redbird_project@yahoo.fr>
Merge 'ThreeMammals:develop' into 'develop'
@raman-m
Copy link
Member

raman-m commented Jan 26, 2024

@ggnaegi @RaynaldM @MayorSheFF
I have the concern regarding new AuthenticationProviderKeys property.
We can reuse old AuthenticationProviderKey property and as a developer I haven't to update a lot of code lines.
Seems adding the new AuthenticationProviderKeys property is overhead design...

Please share your opinion ASAP
What is a benefit of adding new property? Seems zero!
Defining new property can be performed in case when there is no possibility to reuse old property somehow... But old property value can be parsed from the string making a collection of auth schemes. And backward compatibility of AuthenticationProviderKey property will be just excellent!
We don't add/remove props, we manipulate and parse the value of the current property. Seems, pretty clear thing...

@ggnaegi
Copy link
Member

ggnaegi commented Jan 26, 2024

@raman-m I'm not a fan of the language approach with delimiters. You will need a parser for that, with some risks. I would recommend keeping the variant proposed by @MayorSheFF and mark the property AuthenticationProviderKey as deprecated.

@RaynaldM
Copy link
Collaborator

@raman-m I don't think I'm qualified to give an opinion (we don't use auth through the gateway).

@raman-m
Copy link
Member

raman-m commented Jan 26, 2024

@ggnaegi Good, make sense... Everyone loves their own design... Seems it is too late to change design.

But the property is not deprecated. We still consider it as primary auth key, because of backward compatibility.
But what did you mean actually?
Marking the property with [Obsolete] attribute?

@ggnaegi
Copy link
Member

ggnaegi commented Jan 26, 2024

@ggnaegi Good, make sense... Everybody loves its own design... Seems it is to late to change design.

But the property is not deprecated. We still consider it as primary auth key, because of backward compatibility.
But what did you mean actually?
Marking the property with [Obsolete] attribute?

Yes mark it obsolete, so we promote the new approach but backward compatibility is ensured.

@ggnaegi
Copy link
Member

ggnaegi commented Feb 2, 2024

@MayorSheFF could you give me the permissions on your PR so I can finish the last steps? Thanks ggnaegi

@raman-m raman-m assigned raman-m and ggnaegi and unassigned MayorSheFF Feb 4, 2024
@raman-m
Copy link
Member

raman-m commented Feb 4, 2024

@ggnaegi I have done feature design. And added major required acceptance tests 👉0f0a856 related to Identity Server 4.
Could you review please? And if no objections then I'm going to merge... tomorrow.

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.

Code Complete! 🟢

  • Code review ➕➕➕
  • High quality source code ➕
  • Unit tests ➕
  • Acceptance tests ➕
  • Updated Docs ➕
  • Release notes ❔

@raman-m
Copy link
Member

raman-m commented Feb 5, 2024

The build is 🟢

Ready for delivery! ✔️

@raman-m raman-m merged commit c9510b1 into ThreeMammals:develop Feb 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature Nov'23 November 2023 release
Projects
None yet
5 participants