Should auth options be using IOptionsMonitor?

[HaoK]

Currently we are using IOptionsSnapshots which have scoped lifetimes, so the instances will get recreated every request via IOptionsFactory. If we want longer lifetimes, we could switch to IOptionsMonitor, which would would leave lifetime up to the IOptionsMonitorCache.

Downside is, scoped IConfigureOptions won't be run every request for auth which was also something we wanted to support.

[RickBlouch]

Please keep in mind that this functionality is desirable for those of us doing multitenancy. See this comment from the auth redesign issue for the specific strategy that I am referring to.

[HaoK]

What functionality is desirable? That you want to reconfigure options for every request?

[kevinchalet]

Currently we are using IOptionsSnapshots which have scoped lifetimes, so the instances will get recreated every request via IOptionsFactory.

WTF? This is a HUGE regression! In practice, this means that options configuration classes will be run for every handler+requests even when options are supposed to be static (I just confirmed this new behavior when moving my ASOS port to the latest bits including the events changes...)

Related posts:

• [Design] Authentication 2.0 #1065 (comment)
• [Design] Authentication 2.0 #1065 (comment)

[RickBlouch]

What functionality is desirable? That you want to reconfigure options for every request?

I have not been able to follow the changes super close, but I thought IOptionsSnapshot was the go forward strategy (at least in 2.0) to switch the security settings per tenant on each request. That's what the link I pointed to showed. Has that changed? If so what's the new plan?

[HaoK]

@RickBlouch even if we switch to IOptionsMonitor, you could still get similar behavior to the IOptionsSnapshot by replacing IOptionsMontiorCache with a scoped instance, that would result in in the options being recomputed every request.

[ajcvickers]

@Eilon Copying you per discussion today.

[HaoK]

After discussing with @Tratcher we'll switch Auth back to the old behavior (meaning switching to IOptionsMonitor). OpenIdConnect metadata + RemoteAuthentication.Backchannel both will fall over if new instances are created every request. There will still be a way for developers to opt into per request options if they really want, they could do this by changing IOptionsMonitorCache<TOptions> to be a scoped lifetime for the specific handler type they wanted to recreate per request.

[kevinchalet]

After discussing with @Tratcher we'll switch Auth back to the old behavior (meaning switching to IOptionsMonitor).

This should have never changed in the first place, specially since this issue had been clearly identified during the initial 2.0 design. I'm really surprised this change passed the review stage...

[HaoK]

Yeah there's a test hole as there's no formal requirement in the form of tests that enforce this requirement for the auth handlers, so the options breaking change to snapshot didn't cause any issues. But I did file this issue for followup :)

[kevinchalet]

Yeah there's a test hole

"hole" is an euphemism, as many aspects of the OIDC handler are not tested at all (https://github.com/aspnet/Security/issues/772)

[HaoK]

Yup, I've pointed that out in the past :)