brockallen / BrockAllen.MembershipReboot

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

Simpler account verification ux #520

Closed CrescentFresh closed 7 years ago

CrescentFresh commented 9 years ago

I've noticed a strange effect when certain conditions are true of a user account when said account attempts to reset their [forgotten] password.

At this point, assume the user forgets their password.

Resetting their password generates a "confirm your email" email (EmailChangeRequestedEvent triggers this). By design the url embedded is required to prompt for the user's password in order to accept the verification key. This is required due to an implementation detail in VerifyEmailFromKey():

if (account.HasPassword() && String.IsNullOrWhiteSpace(password))
{
    // ...
    throw new ValidationException(...);
}

So back to the example. By clicking on this link the user is implicitly verifying her account, however in order to proceed with resetting their password is required to enter their current password, which they have forgotten.

After looking at the code I have a hunch this happens even without the 4th condition being true as well:

if (!account.IsAccountVerified)
{
    // if they've not yet verified then don't allow password reset
    if (account.IsNew())
    {
        // instead request an initial account verification
        // ...
        this.AddEvent(new AccountCreatedEvent<TAccount> { ... });
    }
    else
    {
        // ...
        this.AddEvent(new EmailChangeRequestedEvent<TAccount> { ... });
    }
}

The UX this results in (asking the user for their password when they are trying to reset it) is weird.

brockallen commented 9 years ago

The RequireAccountVerification flag is a bit misleading -- it means that the user can login even if their email is not verified. We never allow a password to be reset if the email is not confirmed. So if you let them login without email confirmation and they forget the password they're in trouble.

CrescentFresh commented 9 years ago

Yes I understand what MR is doing.

However, what I'm arguing for: clicking a link in an email implicitly confirms the user's identity (in so far as we're talking email access == verified identity) - yet is prompted for a password simply because user.HasPassword().

What about new account creation? The same logic sits at the end of the verification ux there too.

I believe that the logic in ResetPassword() should be symmetrical to that of VerifyEmailFromKey():

To recap ResetPassword() contains the following logic:

ResetPassword():

if (!account.IsAccountVerified)
{
    // if they've not yet verified then don't allow password reset
    if (account.IsNew())
    {
        // ... account created event -> sends email
    }
    else
    {
        // ... email change event -> sends email
    }
}

Could it not follow that the logic should be reflected in VerifyEmailFromKey()?

VerifyEmailFromKey():

if (account.IsAccountVerified)
{
    // as before, validate given password
}
else
{
    // if they've not yet verified then implicitly verify without password requirement
    if (account.IsNew())
    {
        // check VerificationPurpose == VerificationKeyPurpose.VerifyAccount?
    }
    else
    {
        // check VerificationPurpose == VerificationKeyPurpose.ResetPassword?
    }
}

Otherwise, why support resetting of password for unverified accounts at all (provided RequireAccountVerification = true)? Or if you don't like that logic, why require new account registrations to have to enter their password to verify their account? It's a small but meaningful UX change that may reduce the "barrier of entry" to the application.

brockallen commented 9 years ago

Sorry -- was traveling so I got behind on these issues.

However, what I'm arguing for: clicking a link in an email implicitly confirms the user's identity (in so far as we're talking email access == verified identity) - yet is prompted for a password simply because user.HasPassword().

I'd disagree with this statement. Just because you click a link in your email does not confirm your identity. It simply confirms that you control the email that MR sent the message to. Forcing a user to re-enter their password is what really confirms their identity. This protects against incorrect/typo'd email addresses. And that happens all the time.

jeremycook commented 9 years ago

I'm running into this password reset conundrum myself. In my case I require email verification, and getting the same outcome. I'm throwing in my 2p to weigh in that this really is problematic.

Here's a scenario:

  1. A user registers. The site doesn't error but for whatever reason the user never gets (or finds) the confirmation email.
  2. The user moves on and forgets about the site for a while. It wasn't a high priority at the time, they got distracted by some other site/event/etc..
  3. The user comes back to the site and tries registering again some time later (they forgot that they ever tried registering).
  4. The user is told "that email address is already in use."
  5. Next they try signing in a couple times to no avail. They can't remember the password they set.
  6. So they try the reset password feature, and receive an email with a link.
  7. They follow the link and are prompted for their password.
  8. They give up and never try again, or they contact support and complain heartily.

Another scenario is the user simply fat fingered their password, and need to go back to zero because they don't know what they did wrong (yes, twice in a row, maybe caps lock was bumped).

How can this scenario be handled? Right now the user is stuck unless they reach out to support.

brockallen commented 9 years ago

Yea, at this point it's safe to delete the account because: 1) the account has never been logged into (IsNew extension method can check this), and 2) You don't let them login to the app until email has been confirmed (via RequireAccount Confirmation in MR Config).

jeremycook commented 9 years ago

Thanks @brockallen I've updated my register action to delete a new account with matching email before allowing CreateAccount to be called in the normal way. The one downside I ran into is that an account closed email is sent at the same time as the account created email. To avoid that email being sent I can delete the user account from the repository and basically replicate DeleteAccount, but was wondering, is there a more elegant way to handle this?

An example of deleting a new account with the same email as part of the user registration process:

// If a new, unconfirmed user already exists with the same email
// address delete the user account and then permit new account creation
// to proceed normally.
var account = this.userAccountService.GetByEmail(model.Email);
if (account != null && account.IsNew())
{
    this.userAccountService.DeleteAccount(account.ID);
}

// Register like normal:
account = this.userAccountService.CreateAccount(model.Username, model.Password, model.Email);
// ...
jeremycook commented 9 years ago

@brockallen if you have any suggestions for handling this more elegantly I'm all ears but in order to keep things simple I decided to use DeleteAccount as in my previous comment, and display a message to the user letting them know what happened. That kind of double registration should be fairly rare and I guess I'll find out if the closed account notice causes confusion.

Here's how I implemented it for anyone interested. You'll notice that I only do this if account verification is required. I wouldn't want someone to accidentally close their own account because they re-register when they already have an active, valid account. You may notice that this doesn't solve the password reset when account verification is not required problem that started this discussion, unfortunately. This solution is good enough for me for now.

if (this.userAccountService.Configuration.RequireAccountVerification)
{
    // If a new, unconfirmed user already exists with the same email
    // address delete the user account and then permit new account creation
    // to proceed normally.
    var existingAccount = this.userAccountService.GetByEmail(model.Email);
    if (existingAccount != null && existingAccount.IsNew())
    {
        var created = existingAccount.Created;
        this.userAccountService.DeleteAccount(existingAccount.ID);
        ViewBag.AccountDeletedMessage = string.Format("A previous registration was started {0:g} but never finished for an account with the email address you entered. The old account had to be closed before creating the new account you just registered. You will receive an account closed email very soon that you can safely ignore. Be sure to confirm your email address to finish registering this account.",
            created.ToLocalTime());
    }
}

var account = this.userAccountService.CreateAccount(model.Username, model.Password, model.Email);
brockallen commented 9 years ago

You can just configure different MR configs for the delete scenario and don't add an email event handler.

jeremycook commented 9 years ago

Thank you for the suggestion! In case others will benefit, here's what I ended up with. Note that I'm using a custom user account service and configuration class.

if (this.userAccountService.Configuration.RequireAccountVerification)
{
    // If a new, unconfirmed user already exists with the same email
    // address delete the user account before continuing with account creation.
    var existingAccount = this.userAccountService.GetByEmail(model.Email);
    if (existingAccount != null && existingAccount.IsNew())
    {
        // Use a user account service that is not configured to send emails
        // when the account is deleted.
        var tempUserAccountService = new CityUserAccountService(new CityMembershipRebootConfiguration(), this.userRepository);
        tempUserAccountService.DeleteAccount(existingAccount.ID);

        ViewBag.AccountDeletedMessage = string.Format("A previous registration was started {0:g} but never finished for an account with the email address you entered. The old account had to be closed before creating the new account you just registered. Be sure to check your email, and verify your account to finish registering this account.",
            existingAccount.Created.ToLocalTime());
    }
}
geetar85 commented 8 years ago

We never allow a password to be reset if the email is not confirmed. So if you let them login without email confirmation and they forget the password they're in trouble.

@brockallen - adding to @CrescentFresh's initial question. Since the user initiated reset password because they have forgotten their password, why even kick off the EmailChangeRequestedEvent if it will never work? Is there an alternate use case where sending the email verification makes sense in this scenario? Even when they are new, they still initiated reset password, so kicking off the AccountCreatedEvent doesn't seem to be helpful either.

CrescentFresh commented 8 years ago

@brockallen:

Just because you click a link in your email does not confirm your identity

Yes, it does. That's the point of verification urls via email. You (or Dominik, I'll track it down) even say this in a talk you give. Yes it's one of many verification methods (passwords is one, 2fa being another, security questions being another), but in this particular flow access to the email link is implicitly confirming identity.

Otherwise why use email at all for the initial verification email?

brockallen commented 8 years ago

What is the email address on record is incorrect -- for example if you self-register and enter the wrong email (or have a typo)? That's the context for that statement.

brockallen commented 8 years ago

BTW, this is why email confirmation in MR also requires re-entering the password. That really confirms that the person whose email was used is the same person that owns the account. Once that's confirmed, then the email can be trusted for things like password reset.

geetar85 commented 8 years ago

But in the scenario where a password reset is initiated and the email has not been verified, sending the email verification email is pointless as it will leave the user stuck. If you really want to keep the password requirement on verification, doing a password reset when not verified should throw an error, I would think.

brockallen commented 8 years ago

Right, and MR does not do that (unless there's a bug). Perhaps that's what some of this thread is getting at?

geetar85 commented 8 years ago

which "that" are you referring to? It does send the email verification on password reset for an unverified email, are you saying that is the bug?

brockallen commented 8 years ago

I'll have to go back thru the code then to trace the logic.

I do recall some path where an email reset is requested but the email is not yet verified. What should happen is the trigger the email that is the initial email (which at this point might be the email verification)... so yes, there's a state you can get into where the user is stuck if they never confirm their email and forget their password. It's a hard problem to solve (if you agree the possibility of invalid emails entering the system).

If the emails being provided are already confirmed (from some other source), then you can just mark the email for that account as being confirmed (there's an API for that).

geetar85 commented 8 years ago

Ah didn't notice that API - so it looks like we could actually build our own Email Verification without requiring a password (if we disagree with your assessment on requiring password) by calling GetByVerificationKey and SetConfirmedEmail...

brockallen commented 8 years ago

absolutely

brockallen commented 8 years ago

But I do caution you about making the assumption that an end user will enter the right email address. They often get something like that wrong (believe it or not).

geetar85 commented 8 years ago

Yep - definitely get that. One other thought on this scenario - I think it would be useful to have a different Event raised when a user is not verified but they do a password reset. I would want to tailor my message to the user differently in that scenario because a standard email verification email will likely be confusing to the user when they have just selected the forgot password option. Something to think about - I'll see if I can find some time to do a pull request on it.

Thanks as always!

brockallen commented 8 years ago

Not a bad idea. I think what I've done in the past is in the email verification I check IsNew() to know if the user has logged in or not. I think some of the samples might even show this... but yes, certainly room for improvement and/or clarification.

brockallen commented 7 years ago

Given that I don't foresee making this change, I'll close this issue. Thanks.