ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.31k stars 1.63k forks source link

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

Closed MayorSheFF closed 7 months ago

MayorSheFF commented 8 months ago

Closes #1580 #740

Proposed Changes

MayorSheFF commented 8 months ago

@raman-m,

That is a new PR. Is it OK for you?

raman-m commented 8 months ago

@MayorSheFF commented on Dec 28

Thank you very much!

raman-m commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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

raman-m commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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

raman-m commented 7 months ago

@ggnaegi I have done feature design. And added major required acceptance tests 👉https://github.com/ThreeMammals/Ocelot/pull/1870/commits/0f0a856b44770d6d0dbfcc5c8c31445a0d40b5b8 related to Identity Server 4. Could you review please? And if no objections then I'm going to merge... tomorrow.

raman-m commented 7 months ago

The build is 🟢

Ready for delivery! ✔️