Dedicated method vs method with parameter

Question

Suppose we've a permissionManager which tell us if user has some privileges to some action or not. We've quite a lot of them(dozen, maybe more).

I see two ways to implement checks for a user, like a:

1. Dedicated method: permissionManager.canAdminister(user);

2. Method with a parameter: permissionManager.hasPermission(Permissions.ADMINISTER, user);

(where Permissions is a class with a bunch of static fields for each permission or a namespace with global constants)

There are pros and cons for both ways, I see these:

1. Dedicated method

   • Tracking usages and adding additional logging/validation/etc to method is simpler(we need just to modify specific method).

   • No additional entities and global variables.

2. Method with a parameter:

   • Multiple checks for permissions is easier(just a loop with sequential validation of each permission).

   • Ability to have a combination of permissions(bit-or style ADMINISTER | VIEW).

Any additional cons/pros for each implementation?

What is more maintainable and preferable?

PS. maybe moving functionality to User is even better, like: user.hasPermission(Permissions.ADMINISTER) but this breaks Single responsibility principle.

Answer 1

Sometimes, the fact that you have this choice to make is itself a smell. It tells you that perhaps you are missing an abstraction, or that behavior is not sitting in the proper place. That may or may not be true in your particular case, so I'll address your question directly.

The preferred choice depends on what the callers (will) look like. If the extra parameter type would be passed in as a variable, then use the extra parameter. For example:

if (permissionsManager.hasPermission(requiredPermission, user)) {
    doSomething();
}

Since the permission is a variable, we'd have to use something like a case-statement to know which method to call if there were separate methods, and that would be much less readable.

Equally, if you do expect that you would need to test for an arbitrary combination of permissions, use the extra parameter. A separate method like hasPermissions could take something like an EnumSet in that case. The extra parameter for hasPermission would improve readability through consistency of always testing for permissions in a similar style.

On the other hand, if you are only ever going to pass in single hard-coded values, go with the separate method. It more clearly states your intention in that case. For example:

if (permissionsManager.hasPermission(Permission.ADMINISTER, user)) {
    doSomething();
}

is not as good as:

if (permissionsManager.canAdminister(user)) {
    doSomething();
}

Answer 2

In the main logic I would want to see

if ( user.HasPermission(Permission.Delete) )
{
    DeleteSomething();
}

In the HasPermission method you would have something less readable, more technical, low-level like

return PermissionManager.UserHasPermission(this.Id, permission);

Answer 3

I always like to sort out issues like this by looking at using code:

sc = new SecuredCommands( permissions.for(user) );
sc.delete("foo.txt");

That seems nicely decoupled and doesn't require responsibility mixing.

All we care about when trying to issue the command is if it's permitted. We don't HAVE to know who the user is at that point. So all we are directly dependent on is the permissions.

I'll demonstrate how this decoupling helps. Watch as I add the concept of roles without it causing a lot of pain:

sc = new SecuredCommands(  permissions.for( rolls.for(user) )  );
sc.delete("foo.txt");

This, of course, assumes you didn't do something silly like scatter sc construction everywhere.

Oh, and delete should be a method if the commands don't change. If there are likely to be new commands added later then make it an object.

Answer 4

I'm a fan of attribute-based access control because it is extremely flexible and doesn't depend on explicitly enumerated permissions or roles. A simple form of this kind of access control would look like this:

interface Policy {
    /**
     * Returns true if the given subject is allowed to perform the given 
     * action on the given resource in the given environment
     */
    def isAuthorized(
        subject: User, 
        action: Action, 
        resource: Resource, 
        environment: Environment
    ): Boolean
}

Here are some examples of policies

/**
 * Policy that grants access to everything if user has ADMIN role
 */
class AdministerPolicy extends Policy {
    override def isAuthorized(
        subject: User, 
        action: Action, 
        resource: Resource, 
        environment: Environment
    ): Boolean = subject.roles.contains(ADMIN)
}

 

/**
 * Policy that grants read-only access to anyone
 */
class ReadPolicy extends Policy {
    override def isAuthorized(
        subject: User, 
        action: Action, 
        resource: Resource, 
        environment: Environment
    ): Boolean = action == READ
}

 

/**
 * Collection of policies that grants access if one of the child policies
 * grants access
 */
class PolicyCollection(policies: List[Policy]) extends Policy {
    override def isAuthorized(
        subject: User, 
        action: Action, 
        resource: Resource, 
        environment: Environment
    ): Boolean = policies.find(
        p => p.isAuthorized(
            subject, 
            action, 
            resource,  
            environment
        )
    ).map(_ => true).getOrElse(false)
}

At your authorization enforcement point, you can inject a Policy, and invoke isAuthorized before performing the action.

This authorization strategy also allows you to extract authorization enforcement from your application code. The reason why you might want to do that is that it makes your application code less readable when littered with authorization calls. IMO it's better to keep authorization logic out of business logic. Instead authorization should be handled and tested separately.

For example if you're controlling access to a web service, it would be easy to implement a HTTP filter that takes HTTP method -> action and URI -> resource and makes the authorization decision without each endpoint making authorization calls explicitly.

Answer 5

If the sort of permission is just something you pass around as data, and you don't actually have any specific calls in your code that go with it, and/or permissions can be defined by end users and need to be flexible and extensible, I would provide the argument and use a generalized function call.

If the sort of permission you are attempting to check for has specific compile-time implications (e.g. a certain interface is supported) then a compile-time, dedicated function or method is suitable.

For example, if your permissions correspond to objects (like documents or web pages) or enumerable actions (like delegates, stored procedures, or navigation paths), and that list of objects/actions grows over time, you will want a generalized function with an argument.

But if you permission relates to an implementation, meaning administration functions and perhaps a IAdministration interface, by all means, hardcode the check. After all, the interface is hardcoded too, so it's not like you're decreasing the system's flexibility.

You might even try something like this:

var user = GetUserSomehow();
var admin = user.GetPolicy().GetAdminInterface();
if (admin!= null) 
    admin.DoSomethingSerious();

This sort of pattern would prevent you from accidentally calling administration functions for a user that can't have them, e.g. with this sort of (erroneous) code:

var user = GetUserSomehow();
if (user != null)
{
    var admin = GetAdminInterfaceSomehow();
    if (admin != null)
        if (user.CanAdminister)
             log.Debug("Doing something serious");
             admin.DoSomethingSerious(); //Do you see the problem?
}