• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 953
  • Last Modified:

ASP.NET MVC : How to prevent property injection

Hello,

I have been learning MVC and don't like a certain flaw that exists.
I understand that you create ModelViews that represent the data on the view.
I understand that you can create a model to mimic your domain objects, or not.

But how do you prevent someone from injecting form elements, updating properties on the model view that they should NOT be able to update?

Think of the following situation:
There are three users who have access to update a certain customer record.
They use three different views.
Person 1 has access to update FirstName, LastName
Person 2 has access to update FirstName, CompanyName
Person 3 has access to update LastName, CompanyName, AccessLevel

How do you properly build this so that you don't end up with an overwhelming amount of models mimicing domain objects, while still preventing them from updating objects they arn't supposed to have access to?

The best thing so far I can come up with is defining input models and map them to the domain object in the controller.
namespace Models.Customer1
class UpdateCustomer
{
FirstName,
LastName
}

namespace Models.Customer2
class UpdateCustomer
{
FirstName,
CompanyName
}

namespace Models.Customer3
class UpdateCustomer
{
LastName,
CompanyName,
AccessLevel
}

ActionResult (UpdateCustomer uc)
{
DomainCustomer c = _service.GetCustomer(c.CustomerID);
ModelHelper.MapTo(c, uc);
}

Open in new window

0
digitalpacman
Asked:
digitalpacman
  • 7
  • 3
  • 3
1 Solution
 
mrjoltcolaCommented:
Use white-lists with UpdateModel() call. It is a list of property names that are legal to update. See the MSDN docs on UpdateModel
0
 
digitalpacmanAuthor Commented:
You mean TryUpdateModel?
0
 
mrjoltcolaCommented:
Example:

 UpdateModel(model,
   new[]{ "Ssn", "Status", "Title", "FirstName", "MiddleName", "LastName", "Birthdate" }
 );

Open in new window

0
Concerto's Cloud Advisory Services

Want to avoid the missteps to gaining all the benefits of the cloud? Learn more about the different assessment options from our Cloud Advisory team.

 
mrjoltcolaCommented:
UpdateModel and TryUpdateModel both support white lists.
0
 
digitalpacmanAuthor Commented:
So that worked until I had to do child objects.

Now the goal of this was to get this working (which IS working.. but because UpdateModel fails it doesnt work):

UpdateModel<IndexViewModel>(model, x => x.Shipment.Title);

However I don't think this works. For some reason they didn't make UpdateModel traverse down into children.
public class IndexViewModel
    {
        public Uship.Mobile2.Shipment Shipment { get; set; }
    }

    public class Shipment
    {
        public string Title { get; set; }
        public int TotalBids { get; set; }
    }

            IndexViewModel model = new IndexViewModel();
            UpdateModel<IndexViewModel >(model, new[] { "Shipment.Title" });

Open in new window

0
 
Vipul Patel.NET ExpertCommented:
Best practices says do not use your entity/data/business model as a view model. Create separate view model.
Please refer below url for best practices:
http://codeclimber.net.nz/archive/2009/10/27/12-asp.net-mvc-best-practices.aspx

So now you have 2 classes/models for a view.
Let's say for Customer view;
we have 2 models;
First is entity model;
public class CustomerEntity
{
     public string FirstName{get;set;}
     public string LastName {get;set;}
     public string CompanyName {get;set;}
     
}
and last one is View model: CustomerModel
public class CustomerModel
{
     public string FirstName{get;set;}
     public string LastName {get;set;}
     public string CompanyName {get;set;}
     public void CleanUp(string AccessLevel)
    {
          //TODO: Write code as per access level
          //Access Level = 1 then only FirstName should be allowed to update and clear others fields
    }
    public void Update()
    {
          //TODO: Write code as per
         
    }
}

Now action body look like;
ActionResult UpdateCustomer(CustomerModel c)
{
    //TODO: Fetch accesslevel for current user from db/session
    c.CleanUp(accesslevel);
    c.Update();
}
0
 
digitalpacmanAuthor Commented:
Microsoft best practices say that update models (also known as input models) are completely optional and you should normally rely on domain objects for receiving input. Or taking input through your view model and either having BLL methods that accept view models. or translating the view model to business objects to then pass through the service layer.

Microsoft also highly suggests that whenever your UI matches your domain model, that you should probably simply use the domain model for the view model.

They say you can create update models, but they are purely optional and should not be forced to a 1:1 to the view because it becomes cumbersome on large applications.

They suggest using white lists, not a "CleanUp" method.

Infact, in a link to "best practices" it has domain objects, in the view model, which is what I am doing.
The example they give is when one domain object is being updated so they use the PREFIX bind attribute. Which is fine until you have to pass back all the domain objects for a complicated UI.

It is also common practice to get the object you are filling first, then call UpdateModel to fill in the rest, not the reverse (which would require custom code written).

Customer c = _service.GetCustomer(customerID)
UpdateModel(c);
_service.Update(c);
0
 
digitalpacmanAuthor Commented:
So what I tried out to see how I like it, is I made a method on a base class that will look through an object with a list of lambda expressions and invoke updatemodel multiple times on the same model, with different PREFIX values and white lists.

So for example with the object:
class Test {
public Test TestA;
public string B;
}

Invoking
UpdateModel2(testInstance, x => x.TestA.B, x => x.B);
or
UpdateModel2(testInstance, "TestA.B", "B");

Will run
UpdateModel(testInstance.TestA, "TestA", "B");
UpdateModel(testInstance, "B");
public abstract class BaseController : Controller
    {
        protected internal void UpdateModel2<T>(T model, params Expression<Func<T, object>>[] lambdas) where T : class
        {
            IEnumerable<string> whiteListProperties = from l in lambdas select GetFullExpression(l.Body);

            UpdateModel2<T>(model, whiteListProperties.ToArray());
        }

        protected internal void UpdateModel2<T>(T model, params string[] whitelist) where T : class
        {
            UpdateModel2<T>(model, string.Empty, whitelist);
        }

        protected internal void UpdateModel2<T>(T model, string prefix, params string[] whitelist) where T : class
        {
            Type modelType = model.GetType();
            foreach (PropertyInfo pi in modelType.GetProperties())
            {
                if (pi.PropertyType.IsClass && pi.PropertyType != typeof(string))
                {
                    dynamic child = pi.GetValue(model, null);
                    if (child != null)
                    {
                        if (!string.IsNullOrEmpty(prefix))
                            prefix += "." + pi.Name;
                        else
                            prefix = pi.Name;

                        string[] reducedWhitelist = (from s in whitelist
                                                    select s.Replace(prefix + ".", string.Empty)).ToArray();

                        UpdateModel(child, prefix, reducedWhitelist);
                        UpdateModel2(child, prefix, whitelist);
                    }
                }
            }
            UpdateModel(model, whitelist);
        }
}

Open in new window

0
 
Vipul Patel.NET ExpertCommented:
Good solution!

I have one simple query.

Could you give me the sample for parsing model based on access right as per asked question?
Can I reuse above implementation in different places? (DRY principle- Do Not Repeat Your self)

Thanks in advance.
0
 
Vipul Patel.NET ExpertCommented:
Reuse means;

Let's say in an action,
for x access right - UpdateModel(testInstance.TestA, "TestA", "B") will be executed.
for y access right - UpdateModel(testInstance, "B") will be executed.

Can I reuse above implementation in different controllers? If yes then how?
 
0
 
digitalpacmanAuthor Commented:
This is pulling the discussion in a security topic.

I was just using access rights as an example because it makes the most sense.

I am invoking access rights within the action, so if access rights don't match the user goes to the login page.

actionresult()
{
  if (!securitygroup.contains(securitylevel.basic)) return RedirectToAction()
 // code to update from view
}

The code you put as an example is not supposed to be shared because it is strictly bound to the view. It should never show up more than once because then you would re-use the same view.

0
 
digitalpacmanAuthor Commented:
I should also note, I have a view for each security level.
Because different items in the view are editable.
0
 
digitalpacmanAuthor Commented:
If you want to enforce security groups I'm pretty sure you can use a custom action invoker or something like that, with attributes on the action.

[SecurityRequired(Enum.SecurityLevel.Basic)]
public ActionResult Edit(int id)
{
}

0

Featured Post

Concerto Cloud for Software Providers & ISVs

Can Concerto Cloud Services help you focus on evolving your application offerings, while delivering the best cloud experience to your customers? From DevOps to revenue models and customer support, the answer is yes!

Learn how Concerto can help you.

  • 7
  • 3
  • 3
Tackle projects and never again get stuck behind a technical roadblock.
Join Now