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

Support for stable (unchanging) User Ids #27

Closed esenciadev closed 4 years ago

esenciadev commented 4 years ago

This adds support for using a Raven-generated ID for Users, so that changing a Users email does not affect their Id. The default behavior should be unchanged from the existing Identity package (other than a few bug fixes mentioned below), so existing users of the package will not have anything break. By passing in a configuration option to RavenDb.Identity .AddRavenDbIdentityStores<AppUser>(o => o.StableUserId = true) the Stable User Id mode is enabled. With this enabled when a user changes their email address, the compare/exchange value for them is recreated with the new Email, and the Email property on their document is updated.

Also in this PR are some minor bug fixes for things I found while implementing this:

  1. FindUserByEmail now uses the compare/exchange value to get their actual UserId, then calls Load() with that Id. This resolves an issue where a user could change their email address, and another call very shortly after would get a stale result because it was querying an index.
  2. Update had error handling code that would attempt to recreate the compare/exchange value if an update failed, but the key value it was passing to the methods had already been formatted to the compare/exchange key, which resulted in the key being double-formatted (eg: emails/emails/foo@bar.com)
  3. A few places that could pass a CancellationToken but weren't.

@JudahGabriel - I know you prefer .ToLowerInvariant() and I agree with you on that preference, but the dotnet core Identity components which are calling into this UserStore, such as UserManager are expecting email/username to use ToUpperInvariant(). Given that, maybe Raven Identity should use the same approach for normalizing email & username? This UserStore could take a dependency on ILookupNormalizer from the framework and then have consistent behavior for however the user wants to configure normalization. If you are OK with this approach, I can make that change in a separate PR.

JudahGabriel commented 4 years ago

@esenciadev thanks for this - I added some comments above, can you have a look?

cadilhac commented 4 years ago

Thanks @JudahGabriel for writing this library and thanks to @esenciadev for this PR because I can not conceive this library with a user entity having an email as the ID. Users need to change their email without changing references everywhere in the DB and I must be able to easily load a user thanks to its integer ID (from a /user/33 web page for instance).

Question: what prevents it from already being merged? Not final yet?

esenciadev commented 4 years ago

@cadilhac It's my bad, I got distracted with other stuff and failed to make a few updates that needed to happen. I'll get those changes in this week

easily load user thanks to its integer ID

My changes to this do not result in a pure integer ID, it will create a hi/low ID like "users/123-A" and "users/543334-B". If you need a pure integer ID the best solution is to populate the ID property of your user entity before you add it to the store with a value that ends in the pipe character, like 'users|'. This will cause Raven to generate a cluster unique integer ID. Note that there are performance costs associated with using this technique.

cadilhac commented 4 years ago

My changes to this do not result in a pure integer ID, it will create a hi/low ID like "users/123-A" and "users/543334-B". If you need a pure integer ID the best solution is to populate the ID property of your user entity before you add it to the store with a value that ends in the pipe character, like 'users|'. This will cause Raven to generate a cluster unique integer ID. Note that there are performance costs associated with using this technique.

Don't worry for that. I already have a solution in place with a custom AsyncMultiDatabaseHiLoIdGenerator set in the conventions. Being on a single server, I don't need that node tag at the end of the id.

JudahGabriel commented 4 years ago

I'm going to merge this and do a few tweaks based on the above comments.

JudahGabriel commented 4 years ago

Looking at this PR closer, I think it conflates 2 things:

Those things can be different.

Uniqueness: it's entirely reasonable to enforce uniqueness by email; you don't want 2 users to register with the same email address.

ID generation: We may not want user IDs to be based on email address because users can change their email address.

I'm updating the code accordingly. This simplifies the PR.

JudahGabriel commented 4 years ago

Done. I've deployed Raven.Identity v8, which supports configuring how user IDs are generated.

// Use server-generated IDs, e.g. "AppUsers/0001-A"
services
    .AddRavenDbIdentityStores<AppUser>(o => o.UserIdType = UserIdType.ServerGenerated);

// Or, use username-based IDs, e.g. "AppUsers/johndoe"
services
    .AddRavenDbIdentityStores<AppUser>(o => o.UserIdType = UserIdType.UserName);

Regardless of how user IDs are generated, uniqueness is enforced by email address. (Meaning, you can't have 2 users with the same email address.)

Read more about it here.

JudahGabriel commented 4 years ago

Just a quick update to this: in Raven.Identity v8.0.2, we're Id-agnostic. Meaning, we don't care what ID you use. If you don't supply an ID when creating the user, Raven will assign one based on its own conventions.

// ID will be "AppUsers/1-a"; Raven's default.
var user = new AppUser(...) { Id = null };
userManager.CreateAsync(user, ...);

// ID will be incrementing, e.g. "AppUsers/42"
var user = new AppUser(...) { Id = "AppUsers|" }; // cluster-wide increment
userManager.CreateAsync(user, ...);

// ID will be server-generated, e.g. "AppUsers/0000001-A"
var user = new AppUser(...) { Id = "AppUsers/" };
userManager.CreateAsync(user);

// ID will be whatever we set it to
var user = new AppUser(...) { Id = "AppUsers/johndoe@email.com" };
userManager.CreateAsync(user);

In short, Raven.Identity doesn't care what your ID is. We use email for duplicate checking, but that is ID-agnostic. This leaves ID generation left to your app, and if your app doesn't care, ID generation is left to Raven itself.

I think this is the best of both worlds. 😎