current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

I'm a little unsure as to what code belongs in the services and what belongs in the controllers.

Lets say I'm creating a website that requires a fine grained level of permissions (e.g. a user has permissions based on their user level (if they're currently logged in) determining whether they can view, edit, add entities and each entity has it's own set of requirements).

The process is pretty much the same for all entities:

  • Get the current user, either a registered one or an anonymous one
  • Get the requested entity
    • Check that it is not null
    • Check it isn't locked by the system
  • Check that the user has permissions to view/edit on the entity, as well as all parent entities

So, what should go where?

Right now, my controller makes a call to the service to get the entity and passes in the user (or null for anonymous).

The service makes a call to the repository which gets the entity by the id. Then it makes sure that the user has permission to view it. The code is basically:

// Controller
public ActionResult Get(int id)
{
    string error;
    var entity = EntityService.Get(id, GetUser(), out error);
    if(entity == null)
      // Display error
    else
      // Display entity
}

// Service
public Entity Get(int id, User user, out string error)
{
   var entity = Repository.Get(id);
   // null check
   // permissions check
   // if it passes, return the entity, otherwise assign error and return null
}

I don't really like this way of doing it because if I need to do something like redirect the user or display a custom view, I can't just return an error string, I need an enum or something to tell me exactly why the Get failed so I can handle the case properly. Plus (and I'm not sure if this is actually considered a bad thing) whatever is going on in the services method is hidden from the controller.

Any suggestions on a better design?

share|improve this question

closed as too broad by Malachi, Jamal♦ Nov 25 '13 at 4:44

There are either too many possible answers, or good answers would be too long for this format. Please add details to narrow the answer set or to isolate an issue that can be answered in a few paragraphs. If this question can be reworded to fit the rules in the help center, please edit the question.
    
Clarifying question: are these WCF services or Web/ASMX services? Are they also written in C# or are they in another language? –  neontapir Oct 31 '11 at 14:47
    
Sorry, my terminology is probably incorrect. When I say services project, I mean a project that implements the service layer of an application. It's just a C# class library. –  Brandon Oct 31 '11 at 15:34
    
Makes sense. I agree, services is an overloaded word in programming. –  neontapir Oct 31 '11 at 17:17

3 Answers 3

up vote 1 down vote accepted

Could be wrong but I'm thinking you are taking to issue the controller/service pattern in general, not just for Roles. For all of my projects except the most trivial I end up with a Policy (basically bl) layer between the controller and the service. The controller action instantiates a policy class and executes it, inside the policy it calls services to get data etc... Dependency inject everything into your policy so you can mock those services. Your controllers end up very small which is the way I like them!

share|improve this answer
    
So the policy is responsible for filtering out the valid/invalid results by handling all the permissions check? Is its only purpose to keep that logic outside of the service class? That seems like a nice and clean idea. Thanks for the suggestion. –  Brandon Apr 4 '11 at 16:36
    
I always see these MVC patterns discussed where they have 1 model class that pulls data and they use it in their controller and bang we have a web page. In reality, lets say using a PRG pattern, creating timecards with state by state OT rules and approval chains, that simple model doesn't last a minute. In this application I have shared Policy (BL layer), individual policies that call the shared, and throw in a low level service layer, called by either shared or individual. It's essentially a 3 layer Model... –  gt124 Apr 4 '11 at 20:02
up vote 1 down vote

My first instinct is to suggest using built-in ASP.NET Roles. This is handled at the ASP.NET level, so in general you can redirect to the appropriate View. You may have roles for 'Anon' 'Read' and 'ReadWrite' and possibly more. This will help with the first pass authorization, not at the specific database record level, but at a high level based on user type, aka role. However, its not required and the per record, per user access will work just as well.

If you have specific access requirements on a 'per record, per user' basis, you have a couple options.

A) Do what you're doing now, pass the User object to the Services layer, and let the services sort it out and return an appropriate error code. In this case, the service probably selects all of the data required to service the request, so if you expect a high level of authorization failures, this is probably not ideal because you'll waste a lot of database IO on requests that are going to fail. If in your list views, you arleady eliminate records the user doesn't have access to, and are your record level security is to prevent people from Uri snoopting (/Entity/Detail/#### for example) this might be sufficient.

B) Implement a method or two in your services layer which accepts a UserID and an EntityID. It then does the minimum database selection to determin if the UserID provided has access to the EntityID and it simply returns true/false. This is a good option if you expect that high number of requests for entities will fail authorization, since in the failed instances, you'll only select a few bits of data from the database and not the entire request; however, for requests that are permitted, you end up doing two selections. Only you can decide if this is worth it or not. But it does make the code a bit more simple:

if(! services.UserEntityAccess(userID, entityID))
{
    return View("NoAccessView");
}

return View(services.ReadEntity(entityID));

C) Hybrid of A and B, when you initialize a new instance of your service layer, make sure that your User object is a parameter in the constructor of the service, then your service can is always operating from within the "context" of a specific user. This is probably the way I would go, and internally, you can decide which of option A or B works best, based mostly on how many failed authorizations you expect, the advantage here is that if you guess wrong, it's easy to change, because its handeled internally to the class, and you wont need to change any calling code.

public ActionResult Get(int id)
{
    var svc = new ServicesLayer(GetUser());
    var entResponse = svc.GetEntity(id);

    if (entResponse.Error != null)
    {   
        return View("ErrorView",entResponse.Error);
    }

    return View(entResponse.Entity);
}
share|improve this answer
    
This is a great answer, thanks. I'm leaning towards Option C, but I'd like to see if anyone else has any other suggestions. –  Brandon Apr 1 '11 at 17:36
up vote 1 down vote

You can probably refactor a lot of the repetitive functionality to Action Filters. For example, when you are checking for permissions you could use an AuthorizationFilter, that checks for the permission before the Action event executes, and if it is not satisfied, a RedirectToAction is called. The reason I said probably, is because it depends on you being able to manage permissions on a controller action level instead of an individual entity level, and there wasn't enough information in your question to know this for sure. Refactoring these concerns to action filters that return redirect results will help you to determine the cause of failures (invalid, unauthorized, etc), because you will redirect the user to a different page depending on which condition failed.

share|improve this answer

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