Model Ownership Checking

Question

In my Controller before a Model is modified (updated or deleted) I am trying to verify that the User performing the action actually owns the object they are trying to modify.

I am currently doing this at the method level and it seems a bit redundant.

[HttpPost]
public ActionResult Edit(Notebook notebook)
{
    if (notebook.UserProfileId != WebSecurity.CurrentUserId) { return HttpNotFound(); }

    if (ModelState.IsValid)
    {
        db.Entry(notebook).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index");
    }
    return View(notebook);
}

Is there a generic way of doing this that could be reusable across various models?

Is it possible to do this with an ActionFilter?

Answer 1

A filter approach might look like:

public class VerifyOwnership : IActionFilter
{
    public void OnActionExecuting(ActionExecutingContext filterContext)
    {
        foreach(var parameter in filterContext.ActionParameters)
        {
            var owned  = paramter.Value as IHaveAnOwner;
            if(owned != null)
            {                    
                if(owned.OwnerId != WebSecurity.CurrentUserId)
                {
                    // ... not found or access denied
                }
            }
        }
    }

    public void OnActionExecuted(ActionExecutedContext filterContext)
    {

    }
}

That assumes models like Notebook implement a specific interface.

public interface IHaveAnOwner
{
    int OwnerId { get; set; }
}

Blowdart has a good point that a user could tamper with the OwnerId in a post. I'm sure they could tamper with their auth ticket, too, but they'd have to know the other user's ticket and tamper with both to get the IDs to match for another user, I believe.

Answer 2

I can see one problem with what you have - you are relying on user input to perform the security check.

Consider your code

if (notebook.UserProfileId != WebSecurity.CurrentUserId)

Notebook has come from model binding. So UserProfileId has come from model binding. And you can quite happily fake that - for example I use Firefox's TamperData to change the value of the hidden UserProfileId to match my login and away I go.

What I end up doing (in a service, rather than the controller) is on a post pulling back the record from the database based on the unique id passed (Edit/2 for example would use 2), and then checking User.Identity.Name (well, the passed identity parameter) against the current owner field I have in my returned database record.

Because I pull back from the database (repository, whatever) an attribute isn't going to work for this, and I'm not sure you could be generic enough in an attribute's approach anyway.

Answer 3

The filter sounds like an ok approach, but it's somewhat limited. It would be nice if you could have a filter like this:

[RequireOwnership<Notebook>(n => n.UserProfileId)]

...but Attributes are limited in which data types are allowed, and I don't think that generics are allowed either. So you could have a [RequireOwnership] attribute that works by inspecting model properties using reflection, or you could create a custom validator instead, where your model looks like this:

public class Notebook
{
    [MatchesCurrentUserId]
    public int UserProfileId { get; set; }
}

Then your ModelState.IsValid check should suffice.

Edit:

Another option occurred to me. You could use a filter in conjunction with an attribute on your model (doesn't have to be a ValidationAttribute). The filter could inspect your request models and check for properties with [MatchesCurrentUserId], comparing with the current user ID.