Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
7  
I don't think its bad, especially if it's only a flag in the user db table. Should there be changes in the future, change it in the future. You may as well call the security system from the user object. I have once read about the design pattern that every db/resource access should go through the current-user object, to make sure you never try to do something that is not allowed. Might be a bit much, but the user could provide a "privileges" object that covers all security-related stuff. –  Arian Nov 4 '13 at 12:31
    
I think there was a misunderstanding. For you, "User" means the person who uses the software. For them, "User" means the code which uses your functionality. –  Philipp Nov 4 '13 at 12:35
2  
Not that you asked, but the thing I would most be careful of in this case is an IsAdmin() method at all, wherever you put it. I would usually recommend having an "Admin" not be hard-coded, but just have a set of possible permissions and an "Admin" just happens to have the whole set. That way when you need to split up the Admin role later, things are much easier. And it cuts down on special case logic. –  psr Nov 4 '13 at 17:46
1  
@Philipp I don't think so... Both sites are explicitly talking about a User object with an isAdmin() method (whatever it does behind the scenes) –  Izkata Nov 4 '13 at 20:41
    
Too late to edit; I meant "Both sides", not "Both sites"... =P –  Izkata Nov 5 '13 at 0:27
add comment

10 Answers

up vote 7 down vote accepted

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.

share|improve this answer
add comment

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.

share|improve this answer
5  
Ouch (it was my comment), but you do put it a lot better than I did. –  Marjan Venema Nov 4 '13 at 12:38
    
You are saying to model roles in a separate Role class, not on the User class? You are not recommending a separate system like LDAP? What if every single decision that the Role class makes requires querying the appropriate user record and requires no other information? Should it still be separate from the User and if so, why? –  GlenPeterson Nov 4 '13 at 12:41
2  
@GlenPeterson: not necessarily, just that classes <> tables and usually there are a lot more classes to be discerned than tables. Being data-oriented does lead to defining sublists on classes though, when many times these lists should be defined on another "ledger" (orders) or some such class passing in the user (or the order) for which to retrieve the... It is more a matter of responsibilities than of verbs and nouns. –  Marjan Venema Nov 4 '13 at 12:50
8  
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 either have to change the user for each context. So it's better you put that logic somewhere else (e.g. the context class). –  Wilbert Nov 4 '13 at 13:15
1  
@Wilbert: Context! That is particularly enlightening! Yes, if you have more than one context for those permissions then everything makes sense to me. Usually I see these permissions in a single context, and sticking them on the user is the simplest solution in that case. Clearly they don't belong there if they apply differently in a second context. If you write that as an answer, I am likely to accept it. Thank you. –  GlenPeterson Nov 4 '13 at 13:31
show 3 more comments

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.

share|improve this answer
2  
@GlenPeterson: Nope, users can very well exist without security. What about my name, my display name, my address, my e-mail address etc. Where would you put those? Identification (authentication) and authorization are different beasts altogether. –  Marjan Venema Nov 4 '13 at 12:53
2  
class User is a core component in the security triplet of Identification, Authentication, Authorization. These are intimately tied at design level, so it's not unreasonable for the code to reflect this interdependency. –  MSalters Nov 4 '13 at 12:54
1  
The question is, should the User class contain all the logic for handling the triplet! –  Zavior Nov 4 '13 at 12:57
3  
@MSalters: if it is an API to such logic, it knows about it even if it delegates elsewhere for the exact logic. The point being made is that Authorization (permissions) is about a combination of a user with something else and therefore should not be part of either the user or the something else, but part of the permissions systems that is rightly aware of both users and whatever is being given permission for. –  Marjan Venema Nov 4 '13 at 13:10
2  
Should there be changes - well, we don't know that there are going to be changes. YAGNI and all. Do those changes when it becomes necessary, right? –  Izkata Nov 4 '13 at 20:44
show 5 more comments

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.

share|improve this answer
10  
Oh I wish I had had the time to write this. Then again, I probably wouldn't have been able to put it as well as you did without a lot more thought on my part. A lot times my gut instinct tells me to go one way or the other, but explaining why - to myself or someone else - takes a lot more thought and effort. Thanks for this post. Would have multiple-plussed it, but SE won't let me... –  Marjan Venema Nov 4 '13 at 18:52
    
This sounds shockingly similar to a project I am working on and your answer gave me another perspective. Thank you so much! –  Brandon Nov 5 '13 at 2:50
    
"There's nothing wrong with a User.IsAdmin field... unless there is also a User.Name field" you write. But what isAdmin is a method? It might just delegate to an object whose responsibility it is to keep track of permissions. What is best practice in the design of a relational model (what fields an entity has) is not necessarily translatable to what is best practice in an OO model (what messages an object can respond to). –  KaptajnKold Nov 5 '13 at 12:07
    
@KaptajnKold: I keep reading this sentiment in various Q&A threads in this forum. If a method exists, it doesn't matter if it directly performs the operations or it delegates them, it still counts as a responsibility because other classes may rely on it. Your User.IsAdmin might very well be a one-liner that does nothing but call PermissionManager.IsAdmin(this.Id), but if it gets referenced by 30 other classes, you can't change or remove it. That's why the SRP exists. –  Aaronaught Nov 6 '13 at 0:34
    
And, @KaptajnKold, as far as relational models go, I'm not sure what you're getting at; it's even worse to have an IsAdmin field. It's the kind of field that tends to stick around long after the system has evolved into RBAC or something more complex, and gradually gets out of sync with the "real" permissions, and people forget that they aren't supposed to use it anymore, and... well, just say no. Even if you think you only have two roles, "admin" and "non-admin", don't store it as a boolean/bit field, normalize properly and have a permission table. –  Aaronaught Nov 6 '13 at 0:37
show 3 more comments

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.

share|improve this answer
6  
How is that different from user.hasPermission(Permissions.EDITOR), except that it is more verbose and procedural instead of OO? –  Arian Nov 4 '13 at 12:36
8  
@Arian: because user.hasPermissions puts too many responsibilities into the user class. It necessitates the user knowing about permissions, while a user (class) should be able to exists without such knowledge. You don't want a user class knowing about how to print or display (build a form) itself either, you leave that to a printer renderer/builder or display renderer/builder class. –  Marjan Venema Nov 4 '13 at 12:42
4  
user.hasPermission(P) is the correct API, but that's only the interface. The real decision must of course be made in a security subsystem. To address syrion's comment on testing, during testing you could swap out that subsystem. However, the benefit of the straightforward user.hasPermission(P) is that you do not pollute all code just so you can test class User. –  MSalters Nov 4 '13 at 12:52
3  
@MarjanVenema By that logic, User would remain a data object with no responsibilities at all. I'm not saying that the entire permission management should be implemented in the user class, if there is need for such a complex system. You say that a user doesn't need to know about permissions, but I don't see why every other class needs to know how to resolve permissions for a user. This makes mocking and testing much harder. –  Arian Nov 4 '13 at 12:53
6  
@Arian: while anemic classes are a smell, some classes just don't have any behaviour... User, imho, is one such class that is better used as a parameter to a whole bunch of other stuff (who want to do things/make decisions based on who is asking for them), than being used as a container for all kinds of behaviour just because it seems convenient to code it like that. –  Marjan Venema Nov 4 '13 at 12:58
show 2 more comments

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.

share|improve this answer
add comment

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.

share|improve this answer
4  
Save tomorrow for tomorrow. Yeah. When it you figure out that the tightly coupled code you just wrote has pigeonholed you and you have to rewrite it because you didn't take the extra 20 minutes to decouple it... –  MirroredFate Nov 4 '13 at 16:37
5  
@MirroredFate: It is entirely possible to have a User.isAdmin method connected to an arbitrary permission mechanism without coupling the User class to that specific mechanism. –  kevin cline Nov 4 '13 at 17:43
3  
For User.isAdmin to be coupled to an arbitrary permissions mechanism, even without coupling to a specific mechanism, requires... well... coupling. The minimum of which would be to an interface. And that interface should simply not be there. It is giving the user class (and interface) something for which it should simply not be responsible. Aaronaught explains it a lot better than I can (as does the combination of all other answers to this question). –  Marjan Venema Nov 4 '13 at 18:57
8  
@MirroredFate: I don't care if I have to rewrite a simple implementation to meet more complex requirements. But I have had really bad experiences with overly complex APIs designed to meet imagined future requirements that never arose. –  kevin cline Nov 4 '13 at 19:58
3  
@kevincline While overly-complex APIs are (by definition) a bad thing, I was not suggesting that one should write overly-complex APIs. I was stating that Save tomorrow for tomorrow is a terrible design approach because it makes problems that would be trivial if the system was implemented correctly far worse. In fact, that mentality is what results in overly-complex APIs, as layer upon layer is added to patch up the initial poor design. I guess my stance is measure twice, cut once. –  MirroredFate Nov 4 '13 at 22:33
show 2 more comments

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.

share|improve this answer
2  
"SRP just means that an object must delegate to collaborators anything that doesn't fall within it's single responsibility" - false. The definition of the SRP encompasses everything that an object does directly and everything it delegates. A class that only performs one task directly, but performs 50 other tasks indirectly via delegation, is still (probably) violating the SRP. –  Aaronaught Nov 4 '13 at 23:09
    
KaptajnKold: I +1'd because I agree with you and I feel a little guilty about that. Your post adds to the discussion, but it doesn't answer the question. –  GlenPeterson Nov 5 '13 at 0:28
    
@GlenPeterson Well, I tried to address the part of the question which was 'what it looks like done "right"' Also: You should absolutely not feel guilty about agreeing with me :) –  KaptajnKold Nov 5 '13 at 12:49
1  
@JamesSnell Maybe. If. But that's an implementation detail which I would still confine within the isAdmin method on User. That way your client code doesn't have to change when the "admin-ness" of a User evolves from being a boolean field to something more advanced. You might have many places where you need to know if a User is an admin and you don't want to have to change them every time your authorisation system changes. –  KaptajnKold Nov 5 '13 at 13:55
1  
@JamesSnell Maybe we misunderstand each other? The question of whether or not a user is an admin is not the same as the question of whether or not a user is permitted to perform a specific action. The answer to the first question is always independent from the context. The answer to the second is very much dependent on the context. This is what I tried to address in the second half of my original answer. –  KaptajnKold Nov 5 '13 at 15:23
show 7 more comments

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).

share|improve this answer
    
Huh? How is this pertinent? No matter where you put the security policy, you could always potentially subclass it with something insecure. It doesn't matter if that something is a User, Principal, Policy, PermissionSet, or whatever. It's a totally unrelated issue. –  Aaronaught Nov 4 '13 at 23:11
    
It's a security argument in favor of Single Responsibility. To directly answer your point, the security & permissions module is darn well going to be a sealed class, but certain designs might desire the User class to be virtual & inherited. It's the last point on the blog article (thus perhaps the least likely/important?), and perhaps I'm conflating the issues a bit, but given the source, it's clear that polymorphism can be a security threat and therefore limiting the responsibilities of a given class/object is more secure. –  Patrick M Nov 4 '13 at 23:31
    
I'm not sure why you say that. Lots of security and permissions frameworks are highly extensible. Most of the core .NET types related to security (IPrincipal, IIdentity, ClaimSet, etc.) are not only not sealed but are in fact interfaces or abstract classes so that you can plug in whatever you want. What Eric is talking about is that you wouldn't want something like System.String to be inheritable because all kinds of critical framework code makes very specific assumptions about how it works. In practice, most "user code" (including security) is inheritable to allow for test doubles. –  Aaronaught Nov 5 '13 at 0:15
1  
Anyway, polymorphism isn't mentioned anywhere in the question, and if you follow the author's link to where the original comment was made, it's clear that it wasn't referring to inheritance or even class invariants in general - it was about responsibilities. –  Aaronaught Nov 5 '13 at 0:17
    
I know it wasn't mentioned, but the principle of class responsibilities is directly related to polymorphism. If there isn't an IsAdmin method to override/co-opt, there isn't a potential security hole. Reviewing the methods of those system interfaces, IPrincipal and IIdentity, it's clear that the designers disagree with me, and so I concede the point. –  Patrick M Nov 5 '13 at 6:03
add comment

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.

share|improve this answer
4  
What is an unpriviledged class? –  Brandon Nov 5 '13 at 2:55
1  
There really is nothing you can do to prevent this sort of code from being written. No matter how you design your app, somewhere there is going to be an explicit check like this, and someone could write it to be insecure. Even if your permissions system is as streamlined as decorating a method with a role or permission attribute - somebody could incorrectly throw a "public" or "everyone" permission on it. So I don't really see how this makes a method like User.IsAdmin specifically a problem. –  Aaronaught Nov 6 '13 at 0:29
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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