7
\$\begingroup\$

I have an application which uses ASP.net Identity as for its user authentication.

I do not wish for users to reset their own password (for reasons which are not relevant here), so I have developed the following back end system to allow system administrators to reset the users password.

I have the following implementation:

Password Reset View:

@using (Html.BeginForm()) 
{
    @Html.AntiForgeryToken()

    <div class="form-horizontal">
        <h4>ResetPasswordViewModel</h4>
        <hr />
        @Html.ValidationSummary(true, "", new { @class = "text-danger" })
        @Html.HiddenFor(model => model.Id)

        <div class="form-group">
            @Html.LabelFor(model => model.NewPassword, htmlAttributes: new { @class = "control-label col-md-2" })
            <div class="col-md-10">
                @Html.EditorFor(model => model.NewPassword, new { htmlAttributes = new { @class = "form-control" } })
                @Html.ValidationMessageFor(model => model.NewPassword, "", new { @class = "text-danger" })
            </div>
        </div>

        <div class="form-group">
            @Html.LabelFor(model => model.ConfirmPassword, htmlAttributes: new { @class = "control-label col-md-2" })
            <div class="col-md-10">
                @Html.EditorFor(model => model.ConfirmPassword, new { htmlAttributes = new { @class = "form-control" } })
                @Html.ValidationMessageFor(model => model.ConfirmPassword, "", new { @class = "text-danger" })
            </div>
        </div>

        <div class="form-group">
            <div class="col-md-offset-2 col-md-10">
                <input type="submit" value="Reset Password" class="btn btn-default" />
            </div>
        </div>
    </div>
}

AccountAdmin Controller

    // GET: /AccountAdmin/ResetPassword
    public ActionResult ResetPassword(string id)
    {
        if (id == null)
        {
            return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
        }
        ResetPasswordViewModel model = new ResetPasswordViewModel() { Id = id };
        return View(model);
    }

    //
    // POST: /AccountAdmin/ResetPassword
    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult ResetPassword(ResetPasswordViewModel model)
    {
        if (!ModelState.IsValid)
        {
            return View(model);
        }

        var removePassword = UserManager.RemovePassword(model.Id);
        if (removePassword.Succeeded)
        {
            //Removed Password Success
            var AddPassword = UserManager.AddPassword(model.Id, model.NewPassword);
            if (AddPassword.Succeeded)
            {
                return View("PasswordResetConfirm");
            }
        }

        return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
    }

ResetPassword View Model

public class ResetPasswordViewModel
{
    public string Id { get; set; }

    [Required]
    [StringLength(100, ErrorMessage = "The {0} must be at least {2} characters long.", MinimumLength = 6)]
    [DataType(DataType.Password)]
    [Display(Name = "New password")]
    public string NewPassword { get; set; }

    [DataType(DataType.Password)]
    [Display(Name = "Confirm new password")]
    [Compare("NewPassword", ErrorMessage = "The new password and confirmation password do not match.")]
    public string ConfirmPassword { get; set; }
}
\$\endgroup\$
4
  • 3
    \$\begingroup\$ Although, as you say, "not relevant" I would love to know why you aren't going to let people reset their own passwords. \$\endgroup\$ – RobH Jun 5 '15 at 11:25
  • \$\begingroup\$ @RobH: One of the main reasons is a large portion of our users will not have email addresses to follow down the usual password reset email route, additionally, our Email Policy's prevent the use of external email servers yet make it very difficult to use the approved one(For such things as SMTP Email sending) All of this coupled with the amount of potential users Vs Time it will take to implement a self service password reset makes it not high on the agenda to implement. \$\endgroup\$ – tornup Jun 5 '15 at 11:36
  • \$\begingroup\$ Fair enough - unfortunate, but understandable. \$\endgroup\$ – RobH Jun 5 '15 at 11:45
  • 1
    \$\begingroup\$ Anyone Reviewing the code should also note the Unfortunate missing functionality in the Identity Framework that does not let you simply reset a password, the only work around i found was to remove the users password and then add a new one.... I would be interested in your views of how I handle this, and if the methods i am using is considered secure and free of any potential failure. \$\endgroup\$ – tornup Jun 5 '15 at 12:00
3
\$\begingroup\$

One thing you should keep in mind is that when the Add fails, your user is left without a password as it's already been removed. When will this fail? Usually when your password does not pass the validation.

Of course, seeing how an admin is doing it, there's probably not so much of a problem as they can simply add a different one - as long as the message is clear enough to show there actually is a problem with adding the new password.

The way Microsoft wants you to tackle this is by using a PasswordResetToken.

var code = await _userManager.GeneratePasswordResetTokenAsync(user);
await _userManager.ResetPasswordAsync(user, code, newPassword);

For this you'll have to fiddle around a bit with the token generators that Identity provides.

\$\endgroup\$
1
\$\begingroup\$

My main objection was against AccountAdmin Controller. Your latest comment makes my point a bit moot, but here it is anyway.

This controller knows too much about the details of account management. It's not cool that you have to remove and re-add a password to reset it, and these operations pollute your code. A useful approach is to create a helper class to collect all the dirty elements in one place, until you find a better solution. For example, create a UserManagerHelper class with a ResetPassword method. This way your controller code can remain clean, and the strange remove password + re-add password stuff could be encapsulated and hidden in the helper.

Another thing that's not cool about this class is the two variants of the ResetPassword. The inspection from one to the other is confusing, and it seems to result in an unnecessary validity check when coming from a GET request. It would be better to factor out the common logic to a third method, and these two method should not call each other.

\$\endgroup\$

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.