JudahGabriel / RavenDB.Identity

RavenDB Identity provider for ASP.NET Core. Let RavenDB manage your users and logins.
https://www.nuget.org/packages/RavenDB.Identity/1.0.0
MIT License
61 stars 29 forks source link

Clash between expected User.Id "Collection/HiLo" and resulted "Collection/Email" - Error on UserManager.ResetPasswordAsync #26

Closed TheMadKow closed 4 years ago

TheMadKow commented 4 years ago

When calling for _userManager.ResetPasswordAsync(storeUser, token, newPassword)

I get failed result with the following error :

"ConcurrencyFailure"
"Unable to update user email."

The actual PassworsReset works, but error is thrown.

I think I use the same session to get the user, is it what's happening here ? Why would it want to update user email ? :)

It was working fine a while back. But I can recall if bug happened after asp.core 3 or RavenDB.Identity 7.x

Thanks

JudahGabriel commented 4 years ago

Thanks for the bug report. I'll look into this today and get back to you.

JudahGabriel commented 4 years ago

@TheMadKow can you send me the full stack trace you're getting?

TheMadKow commented 4 years ago

Sorry, I've missed it. Will do that soon! :)

TheMadKow commented 4 years ago

@JudahGabriel It's not throwing an error for me, but yielding a valid failed result. Example 1

TheMadKow commented 4 years ago

I'm also having issues with another project - when calling UserManager.CreateAsync(user, pass), it fails (without exception) and says email is duplicate, but database is clean.

Adding new user https://github.com/SuperTeam-IL/DragonCon/blob/14125147b72faf032044c062a4e7a7d8b62b9a23/DragonCon.Gateway.RavenDB/Identities/RavenIdentityFacade.cs#L62

Creating User https://github.com/SuperTeam-IL/DragonCon/blob/14125147b72faf032044c062a4e7a7d8b62b9a23/Dragoncon.App/Startup.cs#L122

DB Records Example 2 Contains no "Long Term Participant" (users). (short term are not identities)

Both of the issues arise on code bases that uses the new version, and initialize in that way -

            services.AddSingleton(holder);  // As Store Holder.
            services 
                .AddRavenDbAsyncSession(holder.Store)
                .AddIdentity<USER, ROLE>(identityOptions => ..... 

It might be the same issue, or something wrong on my site.

Please let me know if you wish more data, and how to obtain it. Also - Thanks again for helping out and for creating this NuGet :)

TheMadKow commented 4 years ago

Update: On the participant issue, the participant was deleted, but the compare exchange value was not removed.

On the ResetPasswordAsync. My users are Users/HiLo and not Users/email

Could it be related to the issue ?

My Guess is -- Updating the user, will also force it to compare-exchange the id vs existing emails.

But I do wish to keep my regular id ("User/A-44" for example, easier to handle on other items referencing users)

EDIT : I personally think it may be advisable to allow "session.store(user)" to generate HiLo ID instead of using the "User/Email" approach.

My personal solution is on https://github.com/TheMadKow/RavenDB.Identity/blob/master/RavenDB.Identity/UserStore.cs

Probably there's a nicer way to use this setting :)

JudahGabriel commented 4 years ago

Please open a separate ticket for the other issue; it likely has to do with the user already being there, inside CompareExchange items. Don't delete users manually; use the UserManager to delete them, which will properly remove the compare exchange values. If you need further help with that, please open a separate issue.

For the ResetPassword issue, what's the user's ID when attempting to reset the password?

JudahGabriel commented 4 years ago

RavenDB.Identity isn't designed to work with hi/lo user IDs. This library achieves high performance ACID queries by looking up users by email. Querying for users via an index isn't ACID -- as RavenDB indexes are not ACID -- thus, you could get stale results.

This library avoids stale results by utilizing well-known IDs based on email addresses.

If you've hacked RavenDB.Identity to use hi/lo IDs instead, you're gonna have a rough time; it's likely the cause of the above described issues.

My recommendation is to use this library as-is, allowing it to use well-known email address IDs, e.g. AppUsers/foo@test.com

JudahGabriel commented 4 years ago

Looking at your personal fork of UserStore, you have bugs related to querying for users by index, e.g. here. Since indexes are not ACID, it's possible and even likely you're going to get stale results, which will wreak all kinds of havoc.

This is why we use well-known user IDs based on email address; it allows us to skip potentially stale indexes and go right to the Raven's ACID storage engine.

Since your bugs here are caused by your personal fork of this library, I'm closing this issue. My recommendation is avoid using hi/lo user IDs; this library doesn't support that, and more importantly, your code will hit issues related to stale indexes.

TheMadKow commented 4 years ago

OK, Thanks.

TheMadKow commented 4 years ago

@JudahGabriel

Just regarding the code location, it's the same as the original project. https://github.com/JudahGabriel/RavenDB.Identity/blob/74f5bdcda1ca24af4970f89c580e301a0a1f9b65/RavenDB.Identity/UserStore.cs#L279

What am I missing ?

JudahGabriel commented 4 years ago

You're not missing anything. It's a bug! 😧 Looks like that was introduced last month from a user contribution.

It should be using .LoadAsync, which is ACID and skips indexes, but it's using .Query, which hits the non-ACID indexes.

I'm committing a fix for this. Thanks for the catch.

JudahGabriel commented 4 years ago

Actually, FindByName is OK - it's FindByEmail that was the issue, as that's the unique key. I've fixed this for FindByEmail to avoid using the potentially stale .Query and instead using the ACID .Load.

TheMadKow commented 4 years ago

@JudahGabriel - Glad to be of assistance after all :)