"Forgot password" functionality

Question

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

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

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

Answer 1

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.

Answer 2

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.