search

Sustainsys / Saml2

Saml2 Authentication services for ASP.NET
Other
940 stars 606 forks source link

Use an IOptionsMonitor in the authentication handler #1411

Closed guillaume86 closed 8 months ago

guillaume86 commented 9 months ago

Use an IOptionsMonitor in place of manually calling IOptionsFactory + IOptionsMonitorCache.

The change allows consumers to inject a custom IOptionsMonitor to implement for example multi tenancy. The official handlers all follow that pattern.

Here's the function in the default options monitor to reassure that this don't change existing behavior: https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs#L90

AndersAbel commented 9 months ago

I agree that following the standard pattern would be better - that's what I do in v3.

But as I am focusing on v3, I try to not do any non-security fixes to v1 or v2. To persuade me, can you tell me why you would need a custom IOptionsMonitor for multi tenancy?

guillaume86 commented 9 months ago

Hey, sure I understand. I need to be able to change the options depending on a value in the HttpRequest, with a custom IOptionsMonitor it is pretty clean and easy to do and make it work with the caching by appending the tenant name to the cache key. But with the current implementation I don't have access to the cache key so it's basically impossible to implement. Have a nice day, thanks for the great library!

AndersAbel commented 8 months ago

But I still don't understand why you need to customize the options for multi tenancy. The common approaches are to either register a scheme per tenant, or to have one scheme with multiple IdentityProviders in the collection?

guillaume86 commented 8 months ago

Your solutions only work when you have a static list of tenants at startup AFAIK. It's not my case I can add a new tenant without restart and a tenant can change its SSO settings at runtime.

AndersAbel commented 8 months ago

Look at the SelectIdentityProvider and GetIdentityProvider notifications. If you implement both of those, you can read the Idp information from the DB.

It's also possible to keep a reference to the IdentityProviders collection around. Then add and remove providers at runtime.

If you read metadata for any of your Idps, you should cache the IdentityProvider objects in memory.

AndersAbel commented 8 months ago

Can we close this as won't fix then?