Looking for elegant and secure way for security-trimming "edit" views

Question

Looking for a clean, secure way to only allow certain users (by role) to edit certain fields in a view. The key word here is edit, because it's one thing to just show/hide parts of a view (buttons, links, etc.), but another thing how to handle "protected" fields as they're being posted back to the controller.

For example, I could do this:

View Model

public class CustomerEditViewModel
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    // Other properties...

    public string Email { get; set; }
    public string Phone { get; set; }

    public bool CanEditContactInfo { get; set; } 
}

View

@Html.EditorFor(m => m.FirstName)
@Html.EditorFor(m => m.LastName)

@if (Model.CanEditContactInfo)
{
    @Html.EditorFor(m => m.Email)
    @Html.EditorFor(m => m.Phone)
}

Controller

[HttpGet]
public ActionResult Edit(int id)
{
    var customer = _customerService.GetCustomerById(id);
    var model = Mapper.Map<CustomerEditViewModel>(customer);
    model.CanEditContactInfo = User.IsInRole("Admin");
    return View(model);
}

[HttpPost]
public ActionResult Edit(CustomerEditViewModel model)
{
    var customer = _customerRepo.GetCustomerById(model.Id);
    Mapper.Map(model, customer);
    _customerService.UpdateCustomer(customer);
    // return to Index view
}

But the problem is that when a non-admin user is editing the customer, the fields Email and Phone are never rendered on the view, so they will be NULL when the form is posted back to the controller, and would further overwrite the actual values in the database with NULLs.

I could solve this like so:

@Html.EditorFor(m => m.FirstName)
@Html.EditorFor(m => m.LastName)

@if (Model.CanEditContactInfo)
{
    @Html.EditorFor(m => m.Email)
    @Html.EditorFor(m => m.Phone)
}
else
{
    @Html.HiddenFor(m => m.Email)
    @Html.HiddenFor(m => m.Phone)
}

That would preseve the original Email/Phone values even when a non-admin user is editing the record, but then the problem is that Email/Phone fields are available in the rendered HTML as hidden fields and could easily be manipulated by a browser tool before posting back.

I do have some ideas, but they are getting messy. So I would like to know what successful approaches might already be out there for something like this.

Why don't just ignore those fields if the user doesn't have the role? — Camilo Terevinto, Jan 10 '18 at 17:23
Depends on what you mean by "ignore." I already explained that if simply left out of the view (i.e. one way of "ignoring them") they will be NULL when posted back to the controller and will save those NULLs to the database (overwriting the actual email/phone values). Unless you mean something else by "ignore?" — Jiveman, Jan 10 '18 at 18:30
I meant ignore them wherever you update the entity's data (I'd guess on `UpdateCustomer`) — Camilo Terevinto, Jan 10 '18 at 18:31
I see. I suppose that makes sense, but I have to push the user's role down to that layer (my "service" layer), so that it can check whether the person making an update is an admin or not. Alternatively, I could have two separate service calls, something like `UpdateCustomer` and `UpdateCustomerWithContactInfo`, but not sure if that feels right. — Jiveman, Jan 10 '18 at 18:35
Seems to me your service layer should be enforcing entitlements/permissions of this kind already. — John Wu, Jan 10 '18 at 18:46

score 2 · Answer 1 · answered Jan 10 '18 at 18:10

The first rule of thumb in secure coding is that the client input cannot be trusted, which means you MUST apply your checks and validations in the server side. In your case, the HttpPost controller action should check the user role again. If user is not authorized to update all properties, then you could:

1- Load the original data again and overwrite the properties that can be updated with user input and then save this updated copy:

[HttpPost]
public ActionResult Edit(CustomerEditViewModel model)
{
    var customer = _customerRepo.GetCustomerById(model.Id);
    // Check the current user role.    
    If (!User.IsInRole("Admin"))
    {
       customer.FirstName = model.FirstName;
       customer.LastName = model.LastName;
    }
    else
    {
       Mapper.Map(model, customer);
    }
    _customerService.UpdateCustomer(customer);
    // return to Index view
}

2- Create another mapping method that ignores the properties that can not be updated from user input and call the right mapper based on the user permission

public static Customer ToEntity(this CustomerEditViewModel model, Customer destination)
{
    return Mapper.CreateMap<CustomerEditViewModel, Customer>()
                 .ForMember(dest => dest.Email, opt => opt.Ignore()
                 .ForMember(dest => dest.Phone, opt => opt.Ignore()
                );
}

I was starting to go down something like this, but wasn't sure whether it's getting too messy. Not sure if I prefer additional manual mapping in the controller, or an alternate mapping in AutoMapper. I am also thinking about two separate view models (one for admins and one for non-admins), but I think that has potential to get messy, as well. — Jiveman, Jan 10 '18 at 18:33
Having two separate view models or even two separate views is definitely more secure; but it adds to the code redundancy (in other words, it has some additional cost). At the end of the date, you need to find the right balance between the cost and benefit and find out what level of security is good enough for your application. — Sparrow, Jan 10 '18 at 18:50

Looking for elegant and secure way for security-trimming "edit" views

1 Answers1