search

brockallen / BrockAllen.MembershipReboot

MembershipReboot is a user identity management and authentication library.
Other
742 stars 238 forks source link

Cancel verification key being overwritten during external linking #652

Closed jonbarthol closed 7 years ago

jonbarthol commented 8 years ago

When I login with an external provider (e.g. Facebook), I get two simultaneous emails--an "account created" email and an "account linked" email. My "account created" email has a link to cancel the verification. However, the verification key is no longer valid, since it's been overwritten by a new one from the account linking.

brockallen commented 8 years ago

You can always customize those emails, and maybe just for that scenario (meaning logging in via external provider).

jonbarthol commented 8 years ago

I'm not sure this will work. The "account created" event happens before the "linked account added" event. So there's no way to know if I should hide the cancel link in the "account created" email. Also, is there a way to include the verification key in the "linked account added" email? The verification key that's included in the UserAccountEvent object is the hashed value. Thanks.

jonbarthol commented 8 years ago

Sorry, I misinterpreted the behavior I'm seeing. The problem isn't that the verification key is overwritten. It is in fact the correct verification key. The problem is that there is no way to delete the newly added account by clicking on the "Cancel request" link in the "account created" email. This is because the "linked account added" event causes the LastLogin on the user record to be updated. So the user is no longer considered a new account.

brockallen commented 8 years ago

Ah ok... good catch. Do you have a suggestion on how to fix it?

jonbarthol commented 8 years ago

Maybe you could re-implement the VerificationKeyPurpose value of "VerifyAccount." It's currently commented out in VerificationKeyPurpose.cs. You could delete the user record if IsAccountVerified is false and VerificationKeyPurpose is "VerifyAccount." A side note to this is our company has "RequireAccountVerfication" set to false, so a user is immediately logged in. But it seems a little odd (bad?) that LastLogin on the user record isn't set, even though the user is actually logged in. It's not set until they log in a second time.

brockallen commented 8 years ago

Or add a new column for "has logged in" (or "is new" or somesuch) that specifically tracks this scenario. I used to have this distinction a while back... and of course i forgot why i changed it.

jonbarthol commented 8 years ago

But wouldn't a new "has logged in" column serve the same purpose as the LastLogin column being set to a non-null value? I guess we need to determine in what state the user record can be deleted via a cancel request. I could imagine a scenario where a user has never verified his email, has logged in a hundred times, but then goes back to his original "account created" email and attempts to cancel the request. Should his user record be deleted?

brockallen commented 7 years ago

Yes, agreed. But given that I don't foresee making this change, I'll close this issue. Thanks.