CorsMiddleware should resolve dependencies dynamically

[aspnet-hello]

From @brockallen on Sunday, March 19, 2017 11:32:55 AM

The dependency on ICorsService and ICorsPolicyProvider to the ctor for the middleware means the use of these services are in effect singletons. This means implementations that require scoping in DI (e.g. to hit a database) will not work properly and the one instance will be used concurrently. I'd suggest a change in Invoke to resolve these dynamically.

Copied from original issue: aspnet/CORS#105

[aspnet-hello]

From @davidfowl on Sunday, March 19, 2017 3:38:12 PM

Are you using a database to resolve your cors policy? Only one of those interfaces are async. So I'd opt for only making one of them scoped (if any)

[aspnet-hello]

From @brockallen on Sunday, March 19, 2017 4:49:06 PM

I'm building a ICorsPolicyProvider, yes, and will typically consult a DB.

https://github.com/IdentityServer/IdentityServer4/blob/dev/src/IdentityServer4/Infrastructure/CorsPolicyProvider.cs

Tangent: this also shows another place where decorator pattern would be nice to be built into DI in AspNetCore (because our middleware only should respond to CORS requests for endpoints our middleware "owns" -- we need to compose with other CORS policies in the hosting app).

Tangent2: I think our use of the CORS plumbing shows how the design is difficult to use with a DB (as our design needs to query the DB with the origin to see if it's allowed).

[aspnet-hello]

From @Eilon on Wednesday, July 5, 2017 9:55:23 AM

This sounds like a good feature request, but we are not currently planning to implement this.

[aspnet-hello]

From @henkmollema on Thursday, July 6, 2017 1:21:28 PM

@Eilon PR #106 already implements this. You might want to take a look.

[aspnet-hello]

From @Eilon on Thursday, July 6, 2017 10:36:02 PM

@henkmollema ah I hadn't noticed that, thanks! We're right now in the last days of stabilization for the 2.0.0 RTM release so we're trying to trim down as much as we can, but we'll definitely take a look for 2.1.0.

[aspnet-hello]

From @brockallen on Monday, August 28, 2017 9:10:52 AM

we'll definitely take a look for 2.1.0

Can we get confirmation that this will be addressed in 2.1? I'd like it to not slip thru the cracks.

[aspnet-hello]

From @Eilon on Monday, August 28, 2017 11:11:59 AM

@brockallen I've flagged it for discussion.

[aspnet-hello]

From @Eilon on Tuesday, September 5, 2017 1:40:49 PM

This would be a breaking change - not only to the API, but to change the lifecycle/scope of a service.

Could the implementation of the ICorsPolicyProvider be made such that it uses service locator (which isn't pretty, but will work fine)?

[aspnet-hello]

From @brockallen on Tuesday, September 5, 2017 2:02:24 PM

This would be a breaking change

Why can't the MW just accept the ICorsPolicyProvider as a param to Invoke or resolve the ICorsPolicyProvider itself via service locator in Invoke?

[aspnet-hello]

From @Eilon on Tuesday, September 5, 2017 3:38:00 PM

Logically it certainly can, but the change would be the scope of the service, which is a breaking change. Anyone currently implementing the interface expects a particular lifetime.

We could conceivable add a new interface with a different lifetime that had the new behavior. I'm hoping that if we just add a new overload to Invoke() with the new interface as a parameter that the middleware invoker will choose the longer overload...

@davidfowl - thoughts on that?

[aspnet-hello]

From @brockallen on Tuesday, September 5, 2017 3:43:38 PM

which is a breaking change. Anyone currently implementing the interface expects a particular lifetime.

Just keep in mind that as of today, given the singleton nature of the MW, the scope that anyone configures for their custom implementation is currently not being honored.

[aspnet-hello]

From @Eilon on Tuesday, September 5, 2017 3:49:14 PM

The scope of a service is defined by the service interface (although not formally; though I wish it was). An implementation cannot (read: must not) choose a different scope.

Or maybe I misunderstood the statement?

[aspnet-hello]

From @brockallen on Tuesday, September 5, 2017 6:47:47 PM

Or maybe I misunderstood the statement?

No, that's what I meant. But this design:

An implementation cannot (read: must not) choose a different scope.

seems bizarre to me. I'd think this would always be the decision of the implementation. This is the first I've heard of that...

[aspnet-hello]

From @davidfowl on Tuesday, September 5, 2017 10:07:30 PM

seems bizarre to me. I'd think this would always be the decision of the implementation. This is the first I've heard of that...

@Eilon is right. You can't externally control the lifetime of a dependency if the usage was never intended for that lifetime. There are so many things that can go wrong if you for example changed a lifetime from scoped or transient to a singleton (e.g. thread safety issues).

[aspnet-hello]

From @brockallen on Wednesday, September 6, 2017 5:04:23 AM

Would going from singleton to scoped/transient be a breaking change?

[aspnet-hello]

From @brockallen on Wednesday, September 6, 2017 5:12:35 AM

The other idea is to put the default impl into DI as a singleton, and then resolve it as a param to Invoke. Then it'll still be a singleton, but then custom implementation can break the rules and re-register with a more appropriate scope (so they can hit the DB).

All in all, this is still a bug -- by accepting the dependency in a MW ctor, that in effect has broken your own rule about changing the lifetime. It's in DI as transient: https://github.com/aspnet/CORS/blob/dev/src/Microsoft.AspNetCore.Cors/CorsServiceCollectionExtensions.cs#L30

[aspnet-hello]

From @Eilon on Wednesday, September 6, 2017 9:45:50 AM

@brockallen yes all changes to scopes are breaking changes.

If a service was assumed to be singleton but is now scoped, other services which are also singletons could have a dependency on it and inadvertently extend the lifetime of that service (to infinity), and perhaps worse, would get one random request's service instance, and try using that with another request.

[mkArtakMSFT]

Sorry for our late response, @brockallen.
We're going to take this change for 3.0.

[brockallen]

Thanks