5

I'm building a simple multi-user (multi-tenant?) App with ASP.NET MVC3 and EF4, one database, one code base, all users access the app using the same URL. Once a User is logged in they should only have access to their data, I'm using the default asp.NET membership provider and have added a ‘UserId’ Guid field on each of the data tables. Obviously I don't want user A to have any access to user B’s data so I have been adding the following to nearly every action on my controllers.

public ActionResult EditStatus(int id)
    {
        if (!Request.IsAuthenticated)
            return RedirectToAction("Index", "Home");

        var status = sService.GetStatusById(id);

        // check if the logged in user has access to this status
        if (status.UserId != GetUserId())
            return RedirectToAction("Index", "Home");
    .
    .
    .
    }

    private Guid GetUserId()
    {
        if (Membership.GetUser() != null)
        {
            MembershipUser member = Membership.GetUser();
            Guid id = new Guid(member.ProviderUserKey.ToString());
            return id;
        }
        return Guid.Empty;
    }

This repetition is definitely feeling wrong and there must be a more elegant way of ensuring my users can't access each other's data – what am I missing?

Mogsdad
  • 44,709
  • 21
  • 151
  • 275
Simon Owen
  • 202
  • 3
  • 10

2 Answers2

12

what am I missing?

A custom model binder:

public class StatusModelBinder : DefaultModelBinder
{
    public override object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext)
    {
        // Fetch the id from the RouteData
        var id = controllerContext.RouteData.Values["id"] as string;

        // TODO: Use constructor injection to pass the service here
        var status = sService.GetStatusById(id);

        // Compare whether the id passed in the request belongs to 
        // the currently logged in user
        if (status.UserId != GetUserId())
        {
            throw new HttpException(403, "Forbidden");
        }
        return status;
    }

    private Guid GetUserId()
    {
        if (Membership.GetUser() != null)
        {
            MembershipUser member = Membership.GetUser();
            Guid id = new Guid(member.ProviderUserKey.ToString());
            return id;
        }
        return Guid.Empty;
    }
}

and then you would register this model binder in Application_Start:

// Could use constructor injection to pass the repository to the model binder
ModelBinders.Binders.Add(typeof(Status), new StatusModelBinder());

and finally

// The authorize attribute ensures that a user is authenticated. 
// If you want it to redirect to /Home/Index as in your original
// example if the user is not authenticated you could write a custom
// Authorize attribute and do the job there
[Authorize]
public ActionResult EditStatus(Status status)
{
    // if we got that far it means that the user has access to this resource
    // TODO: do something with the status and return some view
    ...
}

Conclusion: We've put this controller on a diet which is the way controllers should be :-)

Darin Dimitrov
  • 1,023,142
  • 271
  • 3,287
  • 2,928
  • Very cool, was not aware of this functionality. How would redirections be performed using this method? – Jon Mar 04 '11 at 14:44
  • 1
    @Jonathan Freeland, what redirections? In a RESTful application you should use proper status codes to indicate the intent, not redirections. Then you could have a global error handler which would trap those errors and render respective views, but don't redirect. I mean sending 200 status code when the user is denied access is simply wrong. – Darin Dimitrov Mar 04 '11 at 14:47
  • Darin - thats fantastic - thanks!! "Then you could have a global error handler which would trap those errors and render respective views" Do you recommend any links I could follow to learn more about doing things this way, more RESTful? – Simon Owen Mar 04 '11 at 16:13
  • @Simon Owen, you may take a look at the [following question](http://stackoverflow.com/questions/1171035/asp-net-mvc-custom-error-handling-application-error-global-asax). – Darin Dimitrov Mar 04 '11 at 18:05
1

Trying to get my head around this implementation (I'm having exactly the same question), I found a similar approach described in Scott Hanselman't post

http://www.hanselman.com/blog/IPrincipalUserModelBinderInASPNETMVCForEasierTesting.aspx


    public class IPrincipalModelBinder : IModelBinder
    {    
        public object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext)    
        {        
            if (controllerContext == null) 
            {            
            throw new ArgumentNullException("controllerContext");        
            }        
            if (bindingContext == null) 
            {            
            throw new ArgumentNullException("bindingContext");        
            }        
            IPrincipal p = controllerContext.HttpContext.User;        
            return p;    
        }
    }



    void Application_Start() 
    {    
        RegisterRoutes(RouteTable.Routes); //unrelated, don't sweat this line.    
        ModelBinders.Binders[typeof(IPrincipal)] = new IPrincipalModelBinder();
    }

    [Authorize]
    public ActionResult Edit(int id, IPrincipal user) 
    {     
        Dinner dinner = dinnerRepository.FindDinner(id);     

        if (dinner.HostedBy != user.Identity.Name)        
            return View("InvalidOwner");     

        var viewModel = new DinnerFormViewModel {        
            Dinner = dinner,        
            Countries = new SelectList(PhoneValidator.Countries, dinner.Country)    
        };     
        return View(viewModel);
    }

For a total MVC noob as myself, that was somewhat easier to understand.

Evgeny
  • 3,320
  • 7
  • 39
  • 50