6
\$\begingroup\$

I want to implement the "forgot password" functionality in Asp.Net MVC 5. Here is the code flow:

  1. Take user email ID
  2. Let the user enter the token received in his email inbox
  3. If the token matches then redirect for password reset page
  4. Set the new password

Please tell me where I can improve.

public ActionResult ForgotPassword()
{
    return View();
}

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult ForgotPassword(ForgotPasswordMV viewModel)
{
    if (ModelState.IsValid)
    {
        if (SecurityHelper.SendEmail(viewModel.Email))
            return RedirectToAction("VerifyToken", new { email = viewModel.Email });
        ModelState.AddModelError("Email", "Email not found");
    }

    return View();
}

public ActionResult VerifyToken(string email = null)
{
    VerifyTokenMV viewModel = new VerifyTokenMV
    {
        Email = email,
        Token = ""
    };

    return View(viewModel);
}

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult VerifyToken(VerifyTokenMV viewModel)
{
    if (ModelState.IsValid)
    {
        if (SecurityHelper.VerifyToken(viewModel.Token, viewModel.Email))
            return RedirectToAction("ConfirmPassword", new { email = viewModel.Email });
        ModelState.AddModelError("Token", "Token does not match");

    }

    return View();
}

public ActionResult ConfirmPassword(string email = null)
{
    ConfirmPasswordMV viewModel = new ConfirmPasswordMV
    {
        EmailID = email
    };
    return View(viewModel);
}

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult ConfirmPassword(ConfirmPasswordMV viewModel)
{
    if (ModelState.IsValid)
    {
        if (viewModel.Password == viewModel.ConfirmPassword)
        {
            SecurityHelper.UpdatePassword(viewModel.EmailID, viewModel.Password);
            TempData["Message"] = "Your password has been updated.";
            return RedirectToAction(action, homeController);
        }
        ModelState.AddModelError("", "Password does not match.");
    }

    return View();
}

View Model

public class ConfirmPasswordMV
{
    [Display(Name = "Enter your new password"), Required]
    public string Password { get; set; }

    [Display(Name = "Confirm Password"), Required]
    public string ConfirmPassword { get; set; }

    public string EmailID { get; set; }
}

public class VerifyTokenMV
{
    [Display(Name = "Enter Verification Token sent to your email"), Required]
    public string Token { get; set; }

    public string Email { get; set; }
}

    public class ForgotPasswordMV
{
    [Display(Name = "Enter your email:"), Required]
    public string Email { get; set; }
}

Views

@using (@Html.BeginForm("VerifyToken", "Security"))
{
    @Html.AntiForgeryToken()
    @Html.HiddenFor(m => m.Email)
    @Html.ValidationSummary(true, "Please fix below errors")
    <div class="form-group">
        @Html.LabelFor(m => m.Token)
        @Html.TextBoxFor(m => m.Token, new { @class = "form-control" })
        @Html.ValidationMessageFor(m => m.Token)
    </div>
    <button class="btn btn-primary">Continue</button>
}

View 2

@using (@Html.BeginForm("ConfirmPassword", "Security"))
{
    @Html.AntiForgeryToken()
    @Html.HiddenFor(m => m.EmailID)
    @Html.ValidationSummary(true, "Please fix below errors")
    <div class="form-group">
        @Html.LabelFor(m => m.Password)
        @Html.TextBoxFor(m => m.Password, new { @class = "form-control" })
        @Html.ValidationMessageFor(m => m.Password)
     </div>
    <div class="form-group">
        @Html.LabelFor(m => m.ConfirmPassword)
        @Html.TextBoxFor(m => m.ConfirmPassword, new { @class = "form-control" })
        @Html.ValidationMessageFor(m => m.ConfirmPassword)
    </div>
    <button class="btn btn-primary">Save Password</button>
}

View 3

<p>@ViewBag.ErrorMessage</p>

@using (@Html.BeginForm("ForgotPassword", "Security"))
{
    @Html.ValidationSummary(true, "Please fix below error")
    @Html.AntiForgeryToken()
    <div class="form-group">
        @Html.LabelFor(m => m.Email)
        @Html.TextBoxFor(m => m.Email, new { @class = "form-control" })
        @Html.ValidationMessageFor(m => m.Email)
    </div>
    <button class="btn btn-primary">Continue</button>
}

Note

  1. Kindly note that I am passing email to every single screen as hidden field and thus in controller too so that I can verify the correct user. I have passed in using an anonymous object. It's my novice attempt, so please tell me the flaws/improvements I can make.
  2. Can I have just one view and make all of these as partial view?
  3. Do I really need three separate view models for these pages?
  4. I do not have direct interaction with models as code hits API written in VB.NET code.

EDIT

  1. I have three columns in DB a. token b. emailId c. TimeStamp

  2. I expire token if time is more than 5 minutes.

\$\endgroup\$
4
  • \$\begingroup\$ Could you clarify the 4th note? You write about VB.NET code but there is only C# and you put a question mark at the end of the sentence but it doesn't sound like a question. \$\endgroup\$
    – t3chb0t
    Apr 23, 2017 at 6:38
  • \$\begingroup\$ added the full stop. \$\endgroup\$ Apr 23, 2017 at 6:45
  • \$\begingroup\$ Actually my code hits, backend code written in VB.Net. \$\endgroup\$ Apr 23, 2017 at 6:45
  • \$\begingroup\$ The dekstop version of this web app is written in Vb.net and hence the common code (Model and all is used from Desktop version) \$\endgroup\$ Apr 23, 2017 at 6:47

2 Answers 2

3
\$\begingroup\$

Is this secure?

No. It isn't secure because you can change anyone's email address on the last step. To do so, all I need to do is change the emailId stored in the hidden field (for example, using my browser tools).

In the final step you must verify the token. You don't need step 2 at all.

There are two requests you need once you have your reset token. Typically you'd click a link in the email.

GET passwordReset (pass in the reset token)
POST passwordReset (pass in the reset token and the new password)

Use the reset token to look up the email that you're changing the password for. The token should be time limited, long and random to avoid guessing.

\$\endgroup\$
11
  • \$\begingroup\$ You mean GET passwordRest(pass in the email address) right? Also, to expand on this, you need to store the email and token you sent on the server side somewhere. Never Ever trust the client, especially for something like this. \$\endgroup\$ Apr 24, 2017 at 14:23
  • \$\begingroup\$ @ImperialJustinian I've been unhelpfully vague here. I meant that you only need 2 steps once you have the token. I was thinking of it as coming from a link in an email... I'll edit to explain myself better. \$\endgroup\$
    – RobH
    Apr 24, 2017 at 15:02
  • \$\begingroup\$ I have added some edits to be more clear. \$\endgroup\$ Apr 24, 2017 at 15:40
  • \$\begingroup\$ @Unbreakable - so is emailId not an email address? Is it random and long enough not to be guessable? \$\endgroup\$
    – RobH
    Apr 25, 2017 at 7:35
  • \$\begingroup\$ I am really sorry for being not clear. Back in my country we call Email same as EmailId same as Email Address. So three columns are "token" "email address" aka emailID and timestamp \$\endgroup\$ Apr 25, 2017 at 15:09
1
\$\begingroup\$

I have a few suggestions.

First, you are suffering from some code duplication in the controller with the repetitive checks to the model state, take a look at Jimmy Bogard's post for one way to resolve this problem. You don't have to follow his recommendations exactly, the following is a variant I use in some of my projects.

public abstract class DefaultController : Controller
  {
    internal IActionResult Form<TForm>(TForm form, IFormResultRequest<TForm> request, Func<IActionResult> success, Func<IActionResult> failure)
    {
      if (!ModelState.IsValid)
        return failure();
      var formResult = request.Handle(form);
      if (!formResult.Success)
      {
        ModelState.AddModelError("error", formResult.ErrorMessage);
        failure();
      }
      return success();
    }

Then have your controller inherit from this default controller and do something like:

[HttpPost]
[ValidateAntiForgeryToken]
public IActionResult Validate(ValidateEditModel form, IFormResultRequest<ValidateEditModel> request)
{
  return Form(form, request,
    success: () => RedirectToAction(...),
    failure: () => View(form as ValidateViewModel));
}

This action method has the IFormResultRequest<ValidateEditModel> injected from the DI container. If you do not want to use dependency injection you should still use some type of controller base class to reduce the repetitive model validation checks.

Second, this controller is pretty 'obese', meaning it contains too much code, thus causing it not to be very extensible, and it makes it harder to read. Code that is harder to read is harder to debug. Additionally, you are verifying the token and changing the password from within your controller, this is business logic and business logic should not be contained inside the controller. I suggest using some type of software design pattern to separate this logic out (eg: CQRS with dependency injection).

These recommendations only apply to your backend code and are contingent on the scale of your application. If your application is small enough, CQRS with dependency injection would most likely not be ideal because the payoff it would provide wouldn't be worth the effort. Either way, your business logic needs to be contained within its own layer.

\$\endgroup\$
1
  • \$\begingroup\$ I am a beginner John, Above mentioned solution seems nice and generic but it will take time for me to sink this information. Thank you so much for helping and guiding a newbie. :) \$\endgroup\$ Apr 27, 2017 at 13:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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