search

tugberkugurlu / AspNet.Identity.RavenDB

Fully asynchronous, new and sweet ASP.NET Identity implementation for RavenDB
MIT License
42 stars 28 forks source link

Do not set User.Id in RavenUserStore.CreateAsync #22

Open SlyNet opened 10 years ago

SlyNet commented 10 years ago
  1. Id is generated in format RavenUsers/{0}/Login.
  2. Do not set it if it is defined by client. I want to have predefined userId that I'm creating.
tugberkugurlu commented 10 years ago
  1. It's already fixed and it will be available in the next release.
  2. No, it's not going to be possible. You cannot set the id because username uniqueness needs to be ensured and I'm doing it by setting it to the id. Even further: in the next release, you will see that RavenUserStore will require an optimistic concurrency enabled IAsyncDocumentSession.

What do u want to set as the id?

tugberkugurlu commented 10 years ago

new version is released: https://www.nuget.org/packages/AspNet.Identity.RavenDB/2.0.0-pre-03 FYI.

SlyNet commented 10 years ago

Thanks for new version!

I'm trying to integrate asp.net identity to existing project, and backend code requires GUID as user id. I'm not sure that its correct, but I've put UserId GUID as a separate claim in User entity. But such approach may lead to non efficient queries in Raven. Uniqueness check if not as frequent operation as getting user by id. Do you know how it is implemented in entity framework provider?

tugberkugurlu commented 10 years ago

In EF, they ensure the uniqueness by putting a unique index on the UserName and we don't have that in RavenDB.

You are doing it correct as setting it as a claim. Just index it correctly to perform efficient queries.

Just to be sure: you don't have a scenario where you allow duplicate usernames in your case, right?

SlyNet commented 10 years ago

Yes, you are right. I don't need duplicate user names.

marisks commented 10 years ago

Wouldn't it be better to use RavenDB IdConvention for custom Id? http://ravendb.net/docs/2.5/client-api/basic-operations/saving-new-document

P.S. I would like to use default RavenDB Id generation - then Ids will be unique.

tugberkugurlu commented 10 years ago

@marisks yes, IdConvention can be used. If you have a chance to send a PR for that, please do. However, username needs to be part of the id as we need to ensure the uniqueness of the username.

This rule also applies for E-Mail address and UserLogin which are also stored inside other collections to ensure uniqueness.

petedavis commented 10 years ago

I have been making some changes to try and make the code far more portable from EF to RavenDB. Given that we can now enforce unique constraint on the username and email with the unique bundle http://ravendb.net/docs/2.0/server/extending/bundles/unique-constraints

This will make it possible for the ID to be anything from a GUID, or leave null and have the Hilo generated ID.

I also have also got most of the updated identity sample site working with the RavenDB provider. I used the process that they used in racoonBlog to use document ids in the routes.

One issue i had was that the raven implementation was more restrictive than others and makes it hard to port an existing app over. Things like using the username as the document ID. Some sites allow for changing the username, and that would not be possible with how the raven implementation works, but will with EF. Also what if two people do have the same phone number? It is perfectly plausible that you could have members from the same family with the same home phone number.

I think it would be best if it was possible to derive from the base Raven provider and then enforce these things. A framework version should be as flexible as possible, and not make any assumptions.

Thoughts? Id dont think this is a great generic implementation. Maybe more of a custom version built for a specific set of rules? Im sure you could derive from the changes I made and add those rules back, which would hence make it more of a flexible solution?

tugberkugurlu commented 10 years ago

thanks for your opinions. I agree that usename change is a valid scenario here and this library should provide support for that. I'll make this (setting the username as the document id) configurable. The question:

Thoughts?

petedavis commented 10 years ago

I have been working on this in my fork. There are two solutions.

  1. Use the unique bundle. This will require the bundle to be installed on the raven server, which means that it will not be unique by default.
  2. Manually implement the similar behavior of he unique constraint by storing the username like you do with the email as a separate document. So if there was a IdentityUsername document that is stored when the user is attempted to be created that has the username as the id. Then delete this thing when the user is deleted and delete/create when you change the username.

The second option will take a bit more work. I am looking at doing this at the moment with the email as an idea i had last night, but the update option requires that you load the user document first and then check for the email property change, and if so, then store a new email document to lock it in the raven database.

I still think it is valid to modify other documents when a user update is handed to the UserStore as this is the way that the user store in this case enforces uniqueness, and these documents almost never need to be seen by anyone outside of the store.

I am just wondering what the guys in Microsoft are thinking regarding use of updating a user, rather than the explicitly calling SetEmail. In the EF implementation it works both ways, but a bit more work to ensure this extra document is maintained correctly.