1

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.

  • 2
    Why not both? Have the dedicated method call the method with a parameter. – Andrew says Reinstate Monica Jun 13 '17 at 19:53
  • 1
    Just a note: admin is not a permission, it is a role comprising a number of permissions. The same goes for moderator. – Martin Maat Jun 13 '17 at 20:25
  • @MartinMaat yeah, my fault, you're right. I'll update the question. – Oleg Kovalov Jun 13 '17 at 20:55
  • 1
    @AndrewPiliser consistency? less distraction due different ways to do same stuff. – Oleg Kovalov Jun 13 '17 at 20:59
  • 1
    @olegkovalov, what Andrew is suggesting is called flexibility, and due to the many possible combinations of "permissions", that could be a good feature. – Laiv Jun 13 '17 at 21:11
0

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();
}
1

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);
1

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.

  • 2
    I do not see the point, this appears needlessly coupled to me. You couple commands with rights, through a new redundant class. Why would a command have to know about rights? Like making a tool aware of who is allowed to use it. The code is obscure, the logic is wrapped. To avoid an if? I want to see the if, there is a choice being made here and you are hiding it. – Martin Maat Jun 13 '17 at 20:40
  • @MartinMaat this isn't about avoiding an if. Hell you can use if's in sc if you like. This was about moving the user away from the commands and moving permissions away from use of the commands. This way, eventually, we get to place where we don't have to think about users or permissions. If you have a better idea feel free to post it. – candied_orange Jun 13 '17 at 20:50
  • What is there to decouple? Users do not have a dependency relationship with commands, nor do permissions. The only relationship is between users and permissions, the rest is code logic. I do not see your point. "Moving away the permission from the use of a command" seems backward because linking these two is what this is all about. – Martin Maat Jun 13 '17 at 21:11
  • @MartinMaat still waiting to see your better idea. – candied_orange Jun 13 '17 at 21:13
0

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.

0

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?
}    

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.