What is meant by, "A user shouldn't decide whether it is an Admin or not. The Privileges or Security system should."

Question

The example used in the question pass bare minimum data to a function touches on the best way to determine whether the user is an administrator or not. One common answer was:

user.isAdmin()

This prompted a comment which was repeated several times and up-voted many times:

A user shouldn't decide whether it is an Admin or not. The Privileges or Security system should. Something being tightly coupled to a class doesn't mean it is a good idea to make it part of that class.

I replied,

The user isn't deciding anything. The User object/table stores data about each user. Actual users don't get to change everything about themselves.

But this was not productive. Clearly there is an underlying difference of perspective which is making communication difficult. Can someone explain to me why user.isAdmin() is bad, and paint a brief sketch of what it looks like done "right"?

Really, I fail to see the advantage of separating security from the system that it protects. Any security text will say that security needs to be designed into a system from the beginning and considered at every stage of development, deployment, maintenance, and even end-of-life. It is not something that can be bolted on the side. But 17 up-votes so far on this comment says that I'm missing something important.

Answer 1

Think about the User as a key, and the permissions as a value.

Different contexts might have different permissions for each user. As the permissions are context-relevant, you would have to change the user for each context. So it's better you put that logic somewhere else (e.g. the context class), and just look up the permissions where needed.

A side-effect is that it might make your system more secure, because you cannot forget to clear the admin flag for places where it's not relevant.

Answer 2

That comment was badly worded.

The point isn't whether the user object "decides" whether it is an admin, a trial user, a superuser or anything in or out of the ordinary. Obviously it doesn't decide any of those things, the software system at large, as implemented by you, does. The point is whether the "user" class should represent the roles of the principal that its objects represent, or whether this is a responsibility of another class.

Answer 3

I wouldn't have chosen to word the original comment the way it was worded, but it does identify a potentially legitimate issue.

Specifically, the concerns that warrant separation are authentication vs. authorization.

Authentication refers to the process of logging in and getting an identity. It is how the systems knows who you are, and used for things like personalization, object ownership, etc.

Authorization refers to what you are allowed to do, and this (generally) is not determined by who you are. Instead, it is determined by some security policy like roles or permissions, which don't care about things like your name or email address.

These two can change orthogonally to each other. For example, you might change the authentication model by adding OpenID/OpenAuth providers. And you might change the security policy by adding a new role, or changing from RBAC to ABAC.

If all of this goes into one class or abstraction, then your security code, which is one of your most important tools for risk mitigation, becomes, ironically, high-risk.

I have worked with systems where authentication and authorization were too tightly coupled. In one system, there were two parallel user databases, each for one type of "role". The person or team who designed it apparently never considered that a single physical user might be in both roles, or that there might be certain actions which were common to multiple roles, or that there could be problems with User ID collisions. This is an admittedly extreme example, but it was/is incredibly painful to work with.

Microsoft and Sun/Oracle (Java) refer to the aggregate of authentication and authorization information as the Security Principal. It's not perfect, but it works reasonably well. In .NET, for example, you have IPrincipal, which encapsulates the IIdentity - the former being a policy (authorization) object while the latter is an identity (authentication). You could reasonably question the decision to put one inside of the other, but the important thing is that most code you write will be for just one of the abstractions which means it is easy to test and refactor.

There's nothing wrong with a User.IsAdmin field... unless there is also a User.Name field. This would indicate that the "User" concept isn't properly defined and this is, sadly, a very common mistake among developers who are a bit wet behind the ears when it comes to security. Typically, the only thing that should be shared by identity and policy is the User ID, which, not coincidentally, is exactly how it is implemented in both the Windows and *nix security models.

It's completely acceptable to create wrapper objects that encapsulate both identity and policy. For example, it would facilitate the creation of a dashboard screen where you need to display a "hello" message in addition to various widgets or links that the current user is permitted to access. As long as this wrapper just wraps the identity and policy information, and doesn't claim to own it. In other words, as long as it's not being presented as an aggregate root.

A simplistic security model always seems like a good idea when you're first designing a new application, because of YAGNI and all that, but it almost always ends up coming back to bite you later, because, surprise surprise, new features get added!

So, if you know what's best for you, you'll keep authentication and authorization information separate. Even if the "authorization" right now is as simple as an "IsAdmin" flag, you'll still be better off if it's not part of the same class or table as the authentication information, so that if and when your security policy needs to change, you don't need to do reconstructive surgery on your authentication systems which already works fine.

Answer 4

Well, at the very least it violates single responsibility principle. Should there be changes(multiple admin levels, even more different roles?) this kind of design would be fairly messy and awful to maintain.

Consider the advantage of separating the security system from the user class, so that both can have single, well defined role. You end up with cleaner, easier to maintain design overall.

Answer 5

I think that the issue, perhaps phrased poorly, is that it puts too much weight on the User class. No, there is no security issue with having a method User.isAdmin(), as was suggested in those comments. After all, if someone is deep enough in your code to inject malicious code for one of your core classes, you have serious problems. On the other hand, it doesn't make sense for the User to decide if it is an administrator for every context. Imagine that you add different roles: moderators for your forum, editors who can publish posts to the front page, writers who can write content for the editors to publish, and so on. With the approach of putting it on the User object, you'll end up with too many methods and too much logic living on the User that isn't directly related to the class.

Instead, you could use an enum of different types of permissions, and a PermissionsManager class. Perhaps you could have a method like PermissionsManager.userHasPermission(User, Permission), returning a boolean value. In practice, it might look like (in java):

if (!PermissionsManager.userHasPermission(user, Permissions.EDITOR)) {
  Site.renderSecurityRejectionPage();
}

It's essentially equivalent to the Rails method before_filter method, which provides an example of this type of permissions check in the real world.

Answer 6

Object.isX() is used to represent a clear relationship between the object and X, that can be expressed as a boolean result, for example:

Water.isBoiling()

Circuit.isOpen()

User.isAdmin() assumes a single meaning of 'Administrator' within every context of the system, that a user is, in every part of the system, an administrator.

While it sounds simple enough, a real world program will almost never fit this model, there will most certainly be a requirement for a user to administer [X] resource but not [Y], or for more distinct types of administrators (a [project] administrator vs a [system] administrator).

This situation usually requires an investigation of the requirements. Rarely, if ever, will a client want the relationship that User.isAdmin() actually represents, so I would hesitate to implement any such solution without clarification.

Answer 7

The poster merely had a different and more complicated design in mind. In my opinion, User.isAdmin() is fine. Even if you later introduce some complicated permissions model, I see little reason to move User.isAdmin(). Later, you may want to split the User class into an object representing a logged-in user and a static object representing the data about the user. Or you may not. Save tomorrow for tomorrow.

Answer 8

Not really a problem...

I don't see a problem with User.isAdmin(). I certainly prefer it to something like PermissionManager.userHasPermission(user, permissions.ADMIN), which in the holy name of SRP makes the code less clear and does't add anything of value.

I think SRP is being interpreted a little to literally by some. I think it's fine, even preferable for a class to have a rich interface. SRP just means that an object must delegate to collaborators anything that doesn't fall within it's single responsibility. If the admin role of a user is something more involved than a boolean field, it might very well make good sense for the user object to delegate to a PermissionsManager. But that doesn't mean that it isn't also a good idea for a user to keep it's isAdmin method. In fact it means that when your application graduates from simple to complex, you want have to change the code that uses the User object. IOW, your client doesn't need to know what it really takes to answer the question of whether or not a user is an admin.

... but why do you really want to know?

That said, It seems to me like you rarely need to know if a User is an admin except to be able to answer some other question, like whether or not a user is allowed to perform some specific action, say like updating a Widget. If that is the case, I would prefer to have a method on Widget, like isUpdatableBy(User user):

boolean isUpdatableBy(User user) {
    return user.isAdmin();
}

That way a Widget is responsible for knowing what criteria must be satisfied for it to be updated by a user, and a user is responsible for knowing if it is an admin. This design communicates clearly about its intentions and makes it easier to graduate to more complex business logic if and when that time comes.

[Edit]

The problem with not having User.isAdmin() and using PermissionManager.userHasPermission(...)

I thought I would add to my answer to explain why I greatly prefer calling a method on the User object to calling a method on a PermissionManager object if I want to know if a user is an admin (or has an admin role).

I think it's fair to assume that you are always going to depend on the User class wherever you need to ask the question is this user an admin? That's a dependency you cannot get rid of. But if you need to pass the user to a different object to ask a question about it, that creates a new dependency on that object on top of the one you already have on the User. If the question gets asked a lot, that's a lot places where you create an extra dependency, and it's a lot of places you may have to change if either of dependency demand it.

Compare this with moving the dependency into the User class. Now, suddenly you have a system where client code (code that needs to ask the question is this user an admin) is not coupled to the implementation of how this question get answered. You are free to change the permission system completely, and you only have to update one method in one class to do it. All the client code remains the same.

Insisting on not having an isAdmin method on the User for fear of creating a dependency in the User class on the permissions subsystem, is to my mind akin to spending a dollar to earn a dime. Sure, you avoid one dependency in the User class, but at the cost of creating one in every place where you need to ask the question. Not a good bargain.

Answer 9

This discussion reminded me of a Blog Post by Eric Lippert, which was brought to my attention in a similar discussion about class design, security and correctness.

Why Are So Many Of The Framework Classes Sealed?

In particular, Eric raises the point:

4. Secure. the whole point of polymorphism is that you can pass around objects that look like Animals but are in fact Giraffes. There are potential security issues here.

Every time you implement a method which takes an instance of an unsealed type, you MUST write that method to be robust in the face of potentially hostile instances of that type. You cannot rely upon any invariants which you know to be true of YOUR implementations, because some hostile web page might subclass your implementation, override the virtual methods to do stuff that messes up your logic, and passes it in. Every time I seal a class, I can write methods that use that class with the confidence that I know what that class does.

This may seem like a tangent, but I think it dovetails with the point other posters have raised about the SOLID single-responsibility principle. Consider the following malicious class as a reason not to implement a User.IsAdmin method or property.

public class MyUser : User
{

    public new boolean IsAdmin()
    {
        // You know it!
        return true;
    }

    // Anything else we can break today?

}

Granted, it's a bit of a stretch: no one yet suggested making IsAmdmin a virtual method, but it could happen if your user/role architecture ended up being bizarrely complex. (Or perhaps not. Consider public class Admin : User { ... }: such a class might have exactly that code above in it.) Getting a malicious binary into place is not a common attack vector and has a lot more potential for chaos than an obscure user library - then again, this could be the Privilege Escalation bug that opens the door to the real shenanigans. Finally, if a malicious binary or object instance did find its way into the run time, it's not much more of a stretch to imagine the "Privilege or Security System" being replaced in a similar manner.

But taking Eric's point to heart, if you put your user code in a particular type of vulnerable system, maybe you did just lose the game.

Oh, and just to be precise for the record, I agree with the postulate in the question: “A user shouldn't decide whether it is an Admin or not. The Privileges or Security system should.” A User.IsAdmin method is a bad idea if you are running the code on system you do not remain in 100% control of the code - you should instead do Permissions.IsAdmin(User user).

Answer 10

The problem here is:

if (user.isAdmin()) {
   doPriviledgedOp();
} else {
   // maybe we can anyway!
   doPriviledgedOp();
}

Either you have set up your security properly in which case the privileged method should check if the caller has sufficient authority -- in which case the check is superfluous. Or you have not and are trusting an un-priviledged class to make its own decisions on security.

Answer 11

Security is a tricky topic. There's not always clean clear-cut answers.

In some very simplistic systems, user.isAdmin() may be a completely legitimate approach. This is especially true if

• There is only one meaning for "admin" in the context of the system
• A user either is an admin, or is not.

The latter is something that creates pitfalls. Consider the case of revoking admin access from a user. How is it done? Every user class needs to have been written in a way that handles all possible revocations. The worst case scenario is when an employee is fired, their admin is revoked, and after they have been fired, they have access to an old "user" object that still has admin.

This has the potential to make "User" a spider's nest of functionality, handling not just admin but other similar user attributes that may have different revocation rules managed by different groups. It's a pain.

That being said, I have seen a pattern where user.isAdmin() is a thin wrapper that just makes a call to the correct permissions system. And that can be fine too. However, you have now added security-critical elements to User, so you have to make sure all future updates to User are reviewed with a security mindset (and likely by individuals who can speak for a security organization). Patrick M points out an obvious example of what could go wrong here. Make sure it was worth it!