debuggable

 
Contact Us
 

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:

class PostsController extends Controller{
  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:

class User extends AppModel{
  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

 
&nsbp;

You can skip to the end and add a comment.

Thomas  said on Aug 25, 2008:

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?

Felix Geisendörfer said on Aug 25, 2008:

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.

rafaelbandeira3 said on Aug 25, 2008:

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

Mark Story said on Aug 25, 2008:

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.

Kjell  said on Aug 25, 2008:

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?

rafaelbandeira3 said on Aug 25, 2008:

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.

timmy said on Aug 25, 2008:

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.

kabturek said on Aug 25, 2008:

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

Neil Crookes said on Aug 25, 2008:

Should $post in the findById calls in the PostsController be $id?

Thomas  said on Aug 26, 2008:

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.

Richard@Home said on Aug 26, 2008:

That's very cunning. It's like a cut down version of ACL (which I find very over complicated for simple rights/roles).

ring0  said on Aug 26, 2008:

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

}

}

brian  said on Aug 29, 2008:

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

Pfadi Frauenfeld said on Nov 12, 2008:

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.