Simple Data Access Control
Posted on 25/8/08 by Felix Geisendörfer
Hey folks,
this is post #6 of my 30 day challenge.
If your application is like most, then you have some basic permission requirements for your data. A simple scenario is the following. Blog posts can only be edited by their owners and administrators. Same goes for viewing unpublished blog posts. Given that requirement I usually wrote code like this in the past:
function view($id = null) {
$post = $this->Post->findById($post);
Assert::notEmpty($post, '404');
$ownPost = $post['Post']['user_id'] == User::get('id');
Assert::true($post['Post']['published'] || ($ownPost || User::isAdmin()), '403');
}
function edit($id = null) {
$post = $this->Post->findById($post);
Assert::notEmpty($post, '404');
$ownPost = $post['Post']['user_id'] == User::get('id');
Assert::true($ownPost || User::isAdmin(), '403');
}
}
Note: See this post about the Assert implementation. If you wonder about the User::get('id') call - that is part of our custom Auth system we hope to publish at some point.
One could argue that the above is not ideally DRY and therefor should be refactored. In the past I probably would have followed that logic. However, these days I'm more often like fuck DRY if a little dupplication here and there makes code easier to read and to maintain (yes, that is very much possible with certain one liners).
Anyway, what I don't like about the above is that I feel the logic for deciding record permissions falls into the Model realm. It simply makes more sense if you think of it as a business requirement which should be abstracted in the model layer. So here is how I deal with the problem these days:
static function can($action, $record, $options = array()) {
if (is_string($options)) {
$options = array('model' => $options);
}
$options = array_merge(array(
'action' => $action,
'model' => key($record)
), $options);
if (method_exists($options['model'], 'userCan')) {
return call_user_func(array($options['model'], $method), $record, $options);
}
return $record[$options['model']]['user_id'] == User::get('id');
}
}
class Post extends AppModel{
static function userCan($record, $options) {
if ($options['action'] == 'view' && $record['Post']['published']) {
return true;
}
return ($record['Post']['user_id'] == User::get('id')) || User::isAdmin();
}
}
class PostsController extends Controller{
function view($id = null) {
$post = $this->Post->findById($post);
Assert::notEmpty($post, '404');
Assert::true(User::can('view', $post), '403');
}
function edit($id = null) {
$post = $this->Post->findById($post);
Assert::notEmpty($post, '404');
Assert::true(User::can('edit', $post), '403');
}
}
This is quite some code for this refactoring, yet I found it extremly nice to work with this pattern torwards data access control. It is fairly light weight compared to most other approaches, yet gives you a per-Model and per-Context kind of flexibility on access control.
Let me know if you have any questions!
HTH,
-- Felix Geisendörfer aka the_undefined
Did you like this blog post? If so, please consider subscribing to the Blog RSS feed.
You can skip to the end and add a comment.
Thomas: Sry, forgot the 'extends AppModel'. I promote the idea of using Model classes as static namespaces for relevant functionality as well as gateways for the application state.
I think that what Thomas pointed out is interesting and valid, Models on CakePHP already represent two things: a record it self and the whole table - Daniel Hofstetter wrote about it : http://cakebaker.42dh.com/2008/04/22/thinking-about-the-model-ii/. Sure your approach is really nice, but you're giving User model one more role, so it now represents three things (2 + 1 = 3, I know math!), what it's a bit confusing, specially when working with UsersControllers, because you'll see User representing all this roles together... kind annoying and not much readable...
I think this is a sound approach and one I often implement. My only criticism is that your post assumes the reader is familiar with your previous posts. If they are not, your examples are more complicated to understand, as there is further reading required.
rafaelbandeira3: I've always thought that implementing simple access control is best done in the models. Otherwise you end up with this logic in your controllers / appController. Leaving you with a bulky appController or lots of requestAction() to perform simple access checks. Both of which are less appealing to me than fat models.
I am currently refactoring my project controllers and i am too asking myself how to get rid of repetive access logic. This post came to the right time. ;) anyway: I really like your approach, but there is one thing i feel problematic.
return ($record['Post']['user_id']...
It's not always said that the record is nested like that. (hasMany, etc..). Maybe a non-issue?
Mark Story: I agree with you, my only con to Felix's approach, although I thought it was nice, is that he has one class that does a lot of things, some of them wich aren't even in the same context. My solution to this kind of scenario was implementing a more flexible interface between AuthComponent - wich I extended with the name AppAuthComponent, and the User model class, then always I needed to check something I would pass user's data as a method param. I think this way things are kept in their "more logical" place.
But that's my opinion, not a law.
f@ck dry! classic.
I'm curious, don't many folks use ACL for such authentication? I've made numerous attempts but usually get super pissed off and end up using some code that looks like your first example.
@timmy
yep - i use ACL and access checks are gone from the actions (95% of them). Only 5% of actions need some kinf of $this->Acl->check($this->Auth->user(), 'controller/action/whatever/scheme/you/got', 'update') in them.
I tought that ACL is a too big canon before but... it really simplifies stuff when you get used to it.
Should $post in the findById calls in the PostsController be $id?
felix, i assumbed it. and i think, that is a realy, realy bad thinking of "practicism" architectur. like rafaelbandeira3 already explain.
i like the idea of simple, readbable oo-architecture. that includes the fact, that cone class only do this, what it should to do. not more. not less. a appmodel-representive has to access/manage data from a source (f.e. a database) but has not to decide over access-guidlines. nor do any other things.
i'm suprised that you do, assumed, fine test-driven development, but on the other side prefer such architecture guidlines.
maybe i'm wrong about your purpose here. if i misunderstanding something, please correct me.
That's very cunning. It's like a cut down version of ACL (which I find very over complicated for simple rights/roles).
class PostsController extends Controller{
function view($id = null) {
$post = $this->Post->findById($post /* mistake? */);
Assert::notEmpty($post, '404');
Assert::true(User::can('view', $post), '403');
}
function edit($id = null) {
$post = $this->Post->findById($post /* mistake? */);
Assert::notEmpty($post, '404');
Assert::true(User::can('edit', $post), '403');
}
}
doesn't all of this "where does this belong" stuff really depend on your business logic for your application? When I think of MVC, my models should encapsulate my business rules. If I a lot of rules concerning "users can do this, or that, but not that", then certainly it seems to follow that this logic should or could reside in my User class. To me, the controller should simply marshall data from my view to my model and from my model to my view, ensuring data integrity and validation. So, putting access control in the model makes perfect sense to me.
As always, I think we often look for what we think is the "right" way rather than just doing what is best for our situation. If we always looked for what was right, then we would follow all the Ruby guys that tell us, basically, it's wrong to use PHP :)
It's WAY faster to manually build a query which returns all allowed records instead of using ACL. this way, one can work around the issue that ACL ist url-based and not record-based. (this is a big problem on index pages)
This post is too old. We do not allow comments here anymore in order to fight spam. If you have real feedback or questions for the post, please contact us.
hm, i have a little understanding problem with your example. what exactly is the purpose of your "User"-class?
like i see it, it has 2 purposes to handle. one, the real one to handle/manage and another one to .. eh, represent some special user?