aspnet / Identity

[Archived] ASP.NET Core Identity is the membership system for building ASP.NET Core web applications, including membership, login, and user data. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
1.96k stars 869 forks source link

UserManager is always ServiceLiftime.Scoped #1919

Closed ghost closed 6 years ago

ghost commented 6 years ago
services.TryAddScoped<UserManager<TUser>>();

In practice this presents a problem because the database context is transient. UserManager creates an instance that may be shared among threads that should get a transient dbcontext and uses a scoped instance of something that should be transient. We have in fact caused an error in openidconnectserver if the logins are close enough together as it uses usermanager to verify the login information.

The option to inject a transient usermanager should exist.

blowdart commented 6 years ago

@HaoK

brockallen commented 6 years ago

IIRC, the decision to make the lifetime scoped was to try to protect people who didn't understand DI... at least that's how it was explained when I opened a similar issue back in 1.0 :)

blowdart commented 6 years ago

UserManager does not create contexts which are shared between threads. UserManager is not transient by default either. You can override this yourself as transient, and take the unnecessary overhead :)

(And what Brock said is true. Although I wouldn't phrase it like that.)

ghost commented 6 years ago

I've verified that different threads do in fact get the same DBContext instance when injecting using AddIdentity() (A second operation started on this DBContext…)

This is because UserStore is also injected with Scoped lifetime. Suffice to say, we need transient scope on the dbcontext used by UserStore. The default injection scope for the required identity services doesn't respect the transient nature of the dbcontext that is injected into them.

We have of course copied AddIdentity() code and adjusted the scopes accordingly. I understand DI well enough to know how many instances I have, and why I need transient as opposed to scoped.

brockallen commented 6 years ago

And what Brock said is true. Although I wouldn't phrase it like that.

That's the difference between us: decorum :)

ghost commented 6 years ago

Finally found my answer. It is specific to OpenIdConnectServer.

https://kevinchalet.com/2016/07/13/creating-your-own-openid-connect-server-with-asos-creating-your-own-authorization-provider/

It's important to note that the authorization provider is always a singleton: don't try to inject scoped dependencies in its constructor. To resolve scoped dependencies (e.g an Entity Framework DbContext), use the context.HttpContext.RequestServices property to access the scoped container. You can read this thread for more information about this limitation/design choice, which is not specific to ASOS and impacts all the security middleware sharing the same events model. It might be fixed in a future version, though.

This explains the behavior I'm seeing. Scoped UserManager should work fine now that I know the OpenIdConnectServerProvider is a singleton but can still access the scoped services from within its methods.