Generating and sending new password to user

Question

Generate new password and send to user, if user is registered.

• Two TextBox on page, txtUserName & txtEamil.
• User can provide either UserName or Email to get new password.
• If UserName is provided then find User on username, if exists then generate new password and Email.
• If user provided email then check if any user exists with provided email then generate new password and Email.

protected void btnSubmit_Click(object sender, EventArgs e)
{
    string userName = txtUserName.Text.Trim();
    string email = txtEmail.Text.Trim();

    //
    if (string.IsNullOrEmpty(userName) && string.IsNullOrEmpty(email))
    {
        lblMessage.Text = "Please provide at least either of the one, User Name or Email.";
    }
    else
    {
        SendNewPasswordToUser(userName, email);
    }
}

private void SendNewPasswordToUser(string userName, string email)
{
    if (!string.IsNullOrEmpty(userName))
    {
        MembershipUser mu = Membership.GetUser(userName);
        if (mu != null)
        {
            string password = mu.ResetPassword();
            EmailPassword(password, mu.Email);
        }
    }
    else if (!string.IsNullOrEmpty(email))
    {
        userName = Membership.GetUserNameByEmail(email);
        if (!string.IsNullOrEmpty(userName))
        {
            MembershipUser mu = Membership.GetUser(userName);
            string password = mu.ResetPassword();
            EmailPassword(password, mu.Email);
        }
    }
}

private void EmailPassword(string password, string toEmail)
{
    string mailBody = string.Format("Your new password is {0}", password);
    Mail.SendMail("[email protected]", toEmail, "New Password", mailBody);
}

Is there a better approach to avoid so many ifs?

Answer 1

Before reviewing the code, the concept itself has some flaws:

• It is possible to reset the password of another user by entering their email address or username.
• The password is provided directly in the email which is bad practise (not only is this in clear text but it will be retained in the user's inbox which is not a good place to store passwords).

Entering the email or username should display the same message to the website user regardless of whether the user exists or not. This would stop an attacker from enumerating usernames (i.e. finding whether a particular username or email address is registered on your system). In the email itself include either the details of a password reset link or an account not found message as appropriate for that username/email - this way only the email address owner can see whether they have an account on your system.

The password reset link should be include a time limited reset token that will allow the user to change their password to one of their choosing and it should not then email this out anywhere. Make sure the token is securely generated and does not use a predictable sequence (i.e. a cryptographically secure sequence and not one generated using a Random).

Answer 2

The only thing that I can think of right now is making SendNewPasswordToUser two separate methods.

but this would just move the code around a little bit, on the other hand the way you have it written right now, you are sending an Empty variable into the Method whether the users gives an E-mail or a Username, and that seems a little messy to me.

personally, I think that I would split it up as two separate methods so that if the user inputs a Username it sends a Username into the proper method, and if the user inputs an E-mail it sends the E-mail into the proper method.

I think that this would also help with maintainability further down the road.

if you want to do some checking on the username or the E-mail address, you can work on that stuff separately, it makes things easier.

Answer 3

Something like this? The booleans are there so you can warn the user if you didn't send an email. That said, I agree with SilverlightFox's answer; allowing users to reset each other's passwords is probably a bad idea.

private bool SendNewPasswordToUserByEmail(string email)
{
    return SendNewPasswordToUser(Membership.GetUserNameByEmail(email ?? String.Empty));
}

private bool SendNewPasswordToUser(string userName)
{
    MembershipUser mu = Membership.GetUser(userName ?? String.Empty);
    if (mu == null)
    {
        return false;
    }
    string password = mu.ResetPassword();
    EmailPassword(password, mu.Email);
    return true;
}

Personally, I'd add another if to check for null rather than using ??, but you said to reduce use of conditionals.