IdentityServer / IdentityServer3.AspNetIdentity

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

AspNetIdentityUserService tries to create a new user if external login is not found #33

Closed mderriey closed 9 years ago

mderriey commented 9 years ago

Alright, the title may not be as clear as I want, but I couldn't figure out a clearer way without it being too long.

The current implementation doesn't allow a user to have multiple external logins. Say I first login with Facebook. My external login won't be found, so a new User will be created.

Then I log in with LinkedIn; LinkedIn external login doesn't exist but I don't want a new User to be created, I'd like to have the chance to match an existing user, say by the email address I got back from LinkedIn.

Thus, I'd like another method to (try to) match an existing user based on the claims I got back from the external identity provider. This method would be called in the ProcessNewExternalAccountAsync. If no existing account was found, then only we create the new user with InstantiateNewUserFromExternalProviderAsync and persist it in the DB.

Following this, it could also allow a user to have both a local and an external account, which doesn't look possible either for the same reasons.

I'll be happy to submit a PR if it's OK with you. Just let me know.

brockallen commented 9 years ago

It's a very hard problem to solve and different apps might have different ideas on how to reconcile this. We didn't want to impose one approach or another, so you can derive (or just build your own from scratch).

mderriey commented 9 years ago

OK then, I guess I don't have as much background on the subject than you guys. I'll override the specific parts, even though duplicating that much logic isn't very appealing due to possible future changes in your base implementations.

Thanks anyway!

brockallen commented 9 years ago

In the future, our implementation will be delivered as code, not as an assembly (partially for this reason).

mderriey commented 9 years ago

I may have closed the issue too quickly. What about an in-between solution where the ProcessNewExternalAccountAsync function would call, let's say, TryFindUserFromExternalClaimsAsync which default implementation would return null? This would maintain the current behavior, and users willing to connect external user to local users could override that one function to query the user store.

Once again, if you agree, I'll be happy to submit a PR.

brockallen commented 9 years ago

For our 2.0.0 release we're planning on reworking the identity provider implementations -- we're goign to release them as code nugets (meaning the nuget just has a .cs file and no assembly). This way people can 1) see the code and 2) change it as needed.

mderriey commented 9 years ago

Hey @brockallen, thanks for the heads up. I don't know how far the 2.0.0 release is - if you have some kind of planning I'm interested.

For the time being, what about my previous proposal? It wouldn't alter the default behavior but would allow people to opt-in to link the external identity with the local account?

Thanks

brockallen commented 9 years ago

Well, sure -- throw me a PR and I can add that. I agree, your idea would be fine. Make sure to send it on the dev branch. Thx.

mderriey commented 9 years ago

Perfect, I'll do that tomorrow.

brockallen commented 9 years ago

Merged.