Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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; }
}
share|improve this question
3  
Although, as you say, "not relevant" I would love to know why you aren't going to let people reset their own passwords. – RobH Jun 5 '15 at 11:25
    
@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. – tornup Jun 5 '15 at 11:36
    
Fair enough - unfortunate, but understandable. – RobH Jun 5 '15 at 11:45
1  
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. – tornup Jun 5 '15 at 12:00

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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