1
\$\begingroup\$

I have a view that displays details of a work order request submitted by a user. As far as security and database querying is concerned, which one of the following options would be better if I'm going to update a table named "Requests" with some changes to the properties? Is the extra query to the dbcontext in option 2 worth the security benefit?

Note: some properties omitted to reduce code.

Option 1:

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Details(DetailsViewModel request)
{

    var model = new Request
    {
        Id = request.Id,
        RequestDate = request.RequestDate,
        Status = request.Status,
        FirstName = request.FirstName,
        LastName = request.LastName,
        LastModificationDate = DateTime.Now,
        AffiliationId = request.AffiliationId,
        IssueId = request.IssueId,
        RequestTypeId = request.RequestTypeId,

    };

    _context.Requests.Attach(model);
    var entry = _context.Entry(model);
    entry.Property(e => e.Status).IsModified = true;
    entry.Property(e => e.AffiliationId).IsModified = true;
    entry.Property(e => e.IssueId).IsModified = true;
    entry.Property(e => e.RequestTypeId).IsModified = true;
    entry.Property(e => e.LastModificationDate).IsModified = true;

    _context.SaveChanges();

    return RedirectToAction("Requests");

Option 2:

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Details(DetailsViewModel request)
{
    var model = _context.Requests.Find(request.Id);

    model.Status = request.Status;
    model.AffiliationId = request.AffiliationId;
    model.IssueId = request.IssueId;
    model.RequestTypeId = request.RequestTypeId;
    model.LastModificationDate = DateTime.Now;

    _context.Requests.Attach(model);
    var entry = _context.Entry(model);
    entry.Property(e => e.Status).IsModified = true;
    entry.Property(e => e.AffiliationId).IsModified = true;
    entry.Property(e => e.IssueId).IsModified = true;
    entry.Property(e => e.RequestTypeId).IsModified = true;
    entry.Property(e => e.LastModificationDate).IsModified = true;

    _context.SaveChanges();

    return RedirectToAction("Requests");
\$\endgroup\$
  • \$\begingroup\$ I cannot understand what is security benefit in option 2 ? \$\endgroup\$ – Disappointed Aug 17 '16 at 9:27
1
\$\begingroup\$

I find the second aproach if better for these reasons:

  • You dont't have to specify all properties but only those that change
  • You immediately know if the specified Id is correct because

If an entity with the given primary key values exists in the context, then it is returned immediately [..]. Otherwise, a request is made to the store for an entity with the given primary key values and this entity, if found, is [..] returned. If no entity is found in the context or the store, then null is returned.

  • You don't have to manually set the modified properties as modified (unless you've disabled tracking) becasue

this entity, if found, is attached to the context

so EF will figure out the properties that changed. All you need is:

var model = _context.Requests.Find(request.Id);

// check if model == null and handle it apropriately

// update the properties
model.Status = request.Status;
model.AffiliationId = request.AffiliationId;
model.IssueId = request.IssueId;
model.RequestTypeId = request.RequestTypeId;
model.LastModificationDate = DateTime.Now;

// no need to attach it or set properties as modified
// EF already knows this model and can track changes

// save changes
_context.SaveChanges();

I'd say this is more secure as you work with an entity that really exists, in case someone tried to post an invalid Id. Less work becasue EF handles tracking and ataching the entity thus less error prone.


source: DbSet.Find Method

\$\endgroup\$
0
\$\begingroup\$

My opinion is the following

  1. What is security benefit in option 2 ? As i understand you wanted to verify whether request really exist before updating. But for this case the both options will throw an exception(some entity framework exception in first option and NullReferenceException in second case.) So what is the difference ?!
  2. If you mention security then what about security attributes . Your method does not have it. Does controller have?
  3. And one more thing about security. Very often update methods have "insecure direct object references" issue. At first look you have the same issue.
\$\endgroup\$
  • \$\begingroup\$ 1. In Option 2, only the properties to be updated are passed over to my model class from the viewModel. Whereas all the properties are passed over in the first option. Both are working in my application but I just assumed there was a security benefit in the second option since for example FirstName and LastName don't have a way to be modified via a custom crafted HTTP request by a user. 2. Controller has security attributes to only allow certain roles to use the method. 3. I'm reading on up that, thanks for the heads up. Overall, your comments helped me clarify some aspects on my app. \$\endgroup\$ – Joe Cuevas Aug 17 '16 at 14:21
  • \$\begingroup\$ @JoeCuevas i saw about some property differences but i thought it because of Note: some properties omitted to reduce code. \$\endgroup\$ – Disappointed Aug 17 '16 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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