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\$
  • \$\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 '17 at 6:38
  • \$\begingroup\$ added the full stop. \$\endgroup\$ – Unbreakable Apr 23 '17 at 6:45
  • \$\begingroup\$ Actually my code hits, backend code written in VB.Net. \$\endgroup\$ – Unbreakable Apr 23 '17 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\$ – Unbreakable Apr 23 '17 at 6:47
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\$
  • \$\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\$ – Imperial Justinian Apr 24 '17 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 '17 at 15:02
  • \$\begingroup\$ I have added some edits to be more clear. \$\endgroup\$ – Unbreakable Apr 24 '17 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 '17 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\$ – Unbreakable Apr 25 '17 at 15:09
2
\$\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\$
  • \$\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\$ – Unbreakable Apr 27 '17 at 13:55

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.