MVC4 Razor Looses UserID if Unbound

Question

I'm creating some user profile edit forms in MVC4 at the moment and for testing I was rendering the UserId property into a readonly textbox on the form like this:

<li>
    @Html.LabelFor(model => model.UserId)
    @Html.TextBoxFor(model => model.UserId, new { @readonly="readonly"})
</li>

As I'm nearing completion of the edit form I removed this textbox as it's just using up real estate. Once I had done this the model sent back to the controller when saving had the integer default value of 0 and then the Entity Framework blows up as it cannot update any rows. So I added this to the form:

<li>
    @Html.HiddenFor(model => model.UserId, new { @readonly="readonly"})
</li>

Is this a safe move? Should I be using the ViewBag for things like this? On the profile details page I render an edit button like this:

@Html.ActionLink("Edit", "Edit", new { id=Model.UserId })

Meaning that the UserId is rendered in the link. Is this safe and secure or do I need to rethink how I move the models and ids around the UI?

TIA,

Answer 1

Is this a safe move?

This will do the job of sending the id to the server. Just get rid of the readonly="readonly" attribute which makes very little sense for a hidden input.

Should I be using the ViewBag for things like this?

This doesn't change anything in terms of security. Any user could still put whatever id he wants. Whether you are using a hidden field or an ActionLink you are still sending the id as plain text to the server and anyone could forge a request and put whatever id he wants. So if you site uses some form of authentication you must absolutely check on the server side that the id that you received actually is a resource that belongs to the currently authenticated user before attempting to perform any actions on it. Otherwise some authenticated user could supply the id of a resource that belongs to another user and be able to update it. Of course that's just a hypothetical scenario, it's not clear at all if this is your case and whether this id needs to be secured.

Answer 2

If UserId is sensitive, then there are other options

• Keep UserId server side only with Session state (if your architecture allows for Session)
• Put it in an encrypted cookie. Note as per Darin, that these can be compromised.

If it isn't sensitive, then your HiddenFor is fine - post it back with the rest of the form. Don't put it in your ActionLink Querystring unless this is part of your route (i.e. /Controller/Action/id)

Answer 3

I would strongly suggest using ValueInjecter. Here is a code snippet doing the same thing

[HttpGet]
    public new ActionResult Profile()
    {
        var model = new ProfileModel();

        model.InjectFrom<UnflatLoopValueInjection>(this.GetCurrentUser());

        return View(model);
    }

    [HttpPost]
    public new ActionResult Profile(ProfileModel model)
    {
        if (ModelState.IsValid)
        {
            this.GetCurrentUser().InjectFrom<UnflatLoopValueInjection>(model);

            try
            {
                _userService.SaveOrUpdate(this.GetCurrentUser());
                TempData["Success"] = "User was successfully updated.";
                return RedirectToAction("Profile");
            }
            catch (Exception)
            {
                ModelState.AddModelError("Exception", "Unexpected error");
            }
        }

        return View(model);
    }

And here is the view...

    @using (Html.BeginForm("Profile", "Account", FormMethod.Post, new { @class = "form-horizontal" }))
{
    @Html.ValidationSummary(true, "Unable to update profile. Please correct the errors and try again.", new { @class = "alert alert-block alert-error" })

    @Html.EditorForModel()

    <div class="form-actions">
        <input type="submit" value="Update" class="btn btn-primary" />
    </div>        
}