2

What is the best practice to protect model against unwanted parse/update after post in MVC3

Controller action called at HttpGet-> Product/Edit:

 public ActionResult Edit()
        {
          Product p = new Product();
          p.Id = 1;
          p.Name = "PC";
          Category cat = new Category();
          cat.Id = 1;
          cat.Name = "Non food";
          p.Category = cat;

          return View(p);
        }

This is the Edit View:

@model MvcApplication3.Models.Product
@using (Html.BeginForm("Edit", "Product", FormMethod.Post))
{
  @Html.HiddenFor(model => model.Id)
  @Html.EditorFor(model => model.Name)
  <input type="submit" value="Submit" name="go" />
}

After the browser gets the response, the user inserts the following html segment into the page:

<input type="text" value="5" name="Category.Id" id="Category_Id"/>

He posts the form, and the following controller action gets the "Product" parameter.

    //
    // POST: /Class1/Edit/5

    [HttpPost]
    public ActionResult Edit(Product p)
    {
      //Here: p.Company.Id is 5    !!!
      db.Save(p);
      return null;
    }

The problem is that the user should not be allowed to post/update the c.Company.Id. I would not like to check the whole parameter structure hunting for unwanted values. Im seeking for the best practice to solve the problem.

Any help is appreciated!

Bests,

Boolish

Boolish
  • 35
  • 5

2 Answers2

1

You could separate the received entity type (i.e. the ViewModel) from the entity type persisted to the database, as described in this recent blog post by Josh Bush. Well worth a read - topical too as it stems from the recent similar problem experienced by GitHub.

e.g.

public ActionResult Edit(ProductModel p)
{
    // Map ProductModel -> a Product instance
    // Then save
}
AdaTheDev
  • 142,592
  • 28
  • 206
  • 200
  • Thank You for reply! I agree in that the view models are reasonable in a complex system case. If the view model requires aggregated/transformed data from the db model it should be reasonable as well ... etc. reasons. But if Id like to create a quite simple system, just five to ten tables with the basic CRUD actions, it can be a huge effort to create several view models and bidirectional mapping (Automapper Ignor statements in db model to view model direction) "just" to protect the db model. ... – Boolish Mar 07 '12 at 15:58
  • When the posted data arrives to the data binder, data binder creates a new ViewModel and fills the properties. (reflection) Then I should read the db model and a "simmilar" model is created (same structure, except protected props at view model), and map the view model to db model. (By AutoMapper - reflection again. By hand it is not time effective.) ... – Boolish Mar 07 '12 at 15:59
  • I thought some kind of... Developer says somewhere/somehow: - Whole db (the only one) model and all of its properties are protected.(eg: hardcoded rule) - Protection exceptions depending on actual context. (eg: in session) - Binding posted data directly into the db model (if it is possible, otherwise new model) according to the protection rules/exceptions. (reflection usage only once) What is Your opinion? – Boolish Mar 07 '12 at 15:59
  • 1
    @boolish.dev: "Just" to protect the model? Aren't that the most important aspect of programming? To protect the model? Your worries seems like premature optimization to me. – jgauffin Mar 07 '12 at 16:14
  • @jgauffin: Yes, this is one of the most important aspect of programming, that is why I wrote it between ""-s. I just wanted to solve the problem with the minimum effort. – Boolish Mar 12 '12 at 11:38
  • AdaTheDev's and jgauffin's answers are equivalently appropriates but I can mark only one reply as answer. Thanks guys! – Boolish Mar 12 '12 at 11:57
1

That's why you should use view models and not db entities in your views

http://blog.gauffin.org/2011/07/three-reasons-to-why-you-should-use-view-models/

jgauffin
  • 99,844
  • 45
  • 235
  • 372
  • Dear jgauffin, Thank You for your reply. Please see the comment I wrote to AdaTheDev's reply. Same question for you. What is your opinion? – Boolish Mar 07 '12 at 16:02