IdentityServer / IdentityServer3.AspNetIdentity

ASP.NET Identity support for Thinktecture IdentityServer3
Apache License 2.0
64 stars 51 forks source link

AspNetIdentityUserService's userManager is single instance. Causes caching issues. #77

Closed evdzhan closed 7 years ago

evdzhan commented 7 years ago

In AspNetIdentityUserService there is a field of type <Microsoft.AspNet.Identity.UserManager<TUser, TKey>. In normal ASP.NET web app, such object is not singleton/global. Rather it is recreated for each request.

In the source code of the said file that field is injected in the constructor, and that's all. It stays the same for the entire duration of the identity server lifecycle,. This is quite odd and I've found out that if I edid user data from another web app, using the same database, the changes are gone on next user sign in.

For example I have asp.net identity user Name: Bob Password: 01234

I login with that user through identity server3, and it all works.

But consider that I change the users password from another web app, to 11111. Then I try to log in again using the new password, but it doesnt work. If I try to log in with the old one it works. The issue here is that the cache in UserManager object, overrides all the changes for some reason.

I have found the following stackoverflow question that closely resembles my scenario: http://stackoverflow.com/questions/25701942/odd-behavior-by-usermanager-in-net-identity

So my question is, why is that UserManager a single instance within this library for IdentityServer3? Is this a flaw in the design of the library or flaw in my understanding of the whole technology?

By the way, many thanks for IdentyServer3. Works so well. :)

brockallen commented 7 years ago

I don't understand why the user manager is cached. Where did that get configured.

evdzhan commented 7 years ago

Going to this file https://github.com/IdentityServer/IdentityServer3.AspNetIdentity/blob/master/source/IdentityServer3.AspNetIdentity/IdentityServer3.AspNetIdentity.cs

which contains the AspNetIdentityUserService class, one can find the userManager field. The field is of type Microsoft.AspNet.Identity.UserManager<TUser, TKey> . That field is set at construction time, and is a read only. That is, cannot be changed. By definition, the User Managers have to be used per request -- for each request a new User Manager has to be constructed and used. In the AspNetIdentityUserService class, the User Manager is set once, and that's it.

evdzhan commented 7 years ago

By the way, the way I fixed the caching issue is to replace the usages of that field with a new User Manager every time. This way any external changes to the user are visible and there is no caching issue.

brockallen commented 7 years ago

Sounds like you're doing some caching somewhere in the DI system.

evdzhan commented 7 years ago

I never understood the entire depedency injection system within IdentityServer registration phase. Currently I have the following:

factory.UserService = new Registration<IUserService>(CreateUserService()); CreateUserService() looks like:

private static AspNetIdentityUserService<ApplicationUser, string> CreateUserService() 
{
                var context = new ApplicationDbContext();
                var userStore = new UserStore<ApplicationUser>(context);
                var userManager = new CustomUserManager(userStore);          
                return new AspNetIdentityUserService(userManager);

}
brockallen commented 7 years ago
factory.UserService = new Registration<IUserService>(CreateUserService());

This registers a singleton.

evdzhan commented 7 years ago

Gosh, this makes so much sense now. I looked at the signature of the Registration constructor that the above calls. Indeed, it treats the object I pass as singleton.

I changed the above to: factory.UserService = new Registration<IUserService>(resolver => { return CreateUserService(); });

Making it so, causes the UseManager ( infact the entire UserService ) to be properly recreated for each request.

I am yet to test and see if the caching behaviour is gone.

Many thanks for pointing out my flaw. Also, this question could be moved to the IdentityServer3 issue tracker, incase anyone gets the same incorrect set up in place.

Many many many thanks again.

Regards, Evdzhan Mustafa

P.S. Keep up the good work :)