Separation of concerns and security

Question

The system I am developing is designed to have multiple organizations, with users and roles for each organization. Some organizations can interact, some can not, and generally organizations are not allowed to see or modify each others data with some exceptions.

I have a model class A that manipulates the data storage in a permanent storage container of some kind. I have a controller class B that validates input (including access rights verification) to objects of class A.

The controller classes are in a separate library from the model classes.

All user interaction is performed from view classes which are again in a separated library (in this case exposed as web services).

However, it seems that there is a possible security risk in keeping access control logic separate from the model, since no security checks will be performed if the model is not accessed by its associated controller and is accessed instead by some other code due to some malicious attacker or programmer error.

Should I place the security logic in the same model classes weaving these concerns together or should I keep the separation as I have now?

Answer 1

I would prefer to see those two concepts in two separate classes. The reason is that it's a simpler and cleaner design, which usually means that it will have less bugs (including security bugs). The some design is simpler, it will usually also have less problems (including security based problems).

The attack you are protecting against can only happen through programmer error. I don't see a way that a malicious user can circumvent your controller and hit the model directly, unless you are doing something very weird in your system. The programming error in question is using the model directly and not through a control. I think this is a fairly obviously the "wrong" thing to do and most developers wouldn't do something like that. However, you should be doing code reviews for security critical code, and that's the best place to catch these sorts of issues.

Remember a developer can destroy the security controls in many different ways. They have full code access, and there's countless ways to screw something up. Your best bet is to write the code in the cleanest way possible and enforce practices like code reviews.

Answer 2

IMO, your instincts are good. To me it's basic, duh OOP, to put some kind of access-control on the class/object that controls the data. That data-controlling unit might do something as simple as telling the asker to go validate itself somewhere else and provide valid credentials if you feel validation logic needs to be split out but IMO, it is much more secure in the long haul that access control is at least initiated from the object that hands out the potentially sensitive data before doing so.

If you split it out, there's greater risk of somebody re-using code or making modifications and simply assuming it's being handled in some other fashion they might be used to dealing with. Or they might simply not care. Putting that call for validation on that data-managing object forces them to at the very least prove they don't care by disabling the call for validation and leaving evidence of that in source control.

It's okay for classes to get a little big as long as they're serving said class's purpose. I've seen far too many codebases busted into 10,000 1 or 2 method classes which is really no different than 10,000 functions in old-school badly-designed procedural code, which OOP is meant to help people improve on. Line-count doesn't matter if you're isolating responsibilities according to design so you don't potentially have thousands of hands on the same persistent data (real fun for debug) or an infinite number of ways for some crummy outsourced outfit to expose confidential data by accident long after you've left that codebase behind.