5
\$\begingroup\$

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("admin@abc.com", toEmail, "New Password", mailBody);
}

Is there a better approach to avoid so many ifs?

\$\endgroup\$
4
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ Thanks to review my code. I have changed my logic and now I send a link to user to change password. \$\endgroup\$ – Kashif Oct 28 '13 at 18:10
  • \$\begingroup\$ I agree SilverlightFox .... \$\endgroup\$ – Malachi Oct 28 '13 at 18:17
  • \$\begingroup\$ @Kashif if you're still supporting this system, please move to the built in asp.net membership system. It handles all these nitty gritty details for you. Much more secure than anything we could write ourselves. It behaves exactly as SilverloghtFox suggested it should. \$\endgroup\$ – RubberDuck Jan 2 '16 at 12:11
3
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ It would be even cleaner if the btnSubmit_Click function decided which one of these functions to call (and you could omit both null checks and coalescing either way; I wrote these functions independent of knowledge of the callers). \$\endgroup\$ – Brian Oct 28 '13 at 17:38
  • \$\begingroup\$ Thanks for the effort. Your suggestions are good and I just missed this ?? operator. I have changed my logic to email a change password link to user. \$\endgroup\$ – Kashif Oct 28 '13 at 18:12
  • \$\begingroup\$ In hindsight, warning the user if no e-mail was sent may be considered a security hole. Depending on the value of your site, you may want it to be difficult for malicious users to detect which users have an account. \$\endgroup\$ – Brian Mar 2 '18 at 14:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.