tmaiaroto / li3_access Goto Github PK

This is a biggie. parseMatch()'s regex can't deal with namespaces, which makes (for example) the test suite impossible to add a rule for. Ideally you want to be able to do:

Access::config(array('default' => array(
    'adapter' => 'AuthRbac',
    'roles' => array(
        array(
            'requesters' => 'devs'
            'match' => 'lithium\test\Controller::*'
    )
));

But naturally this affects any namespace you wish to use, making it impossible to deal with more than one controller named the same way, but with different namespace.

Also, on a small note, the preg_match() can take the controller and action right from the match, without the need for the extra explode() call.

Coming up a pull request with a fix and tests for the fix.

Ok consider this scenario:

1. You build a signup form where you allow users to specify email and password:

<h1>Signup</h1>
<?php echo $this->form->create(isset($user) ? $user : null); ?>
    <?php echo $this->form->field('email'); ?>
    <?php echo $this->form->field('password', array('type' => 'password')); ?>
    <?php echo $this->form->field('name'); ?>
    <?php echo $this->form->submit('Signup'); ?>
<?php echo $this->form->end(); ?>

1. You configure Auth so fields is array('email', 'password'), and set it to use theForm` adapter:

Auth::config(array('default' => array(
    'adapter' => 'Form',
    'model' => 'app\models\User',
    'fields' => array('email', 'password')
)));

1. You add an AuthRbac rule so that the signup form is only accessible by non-logged-in users (this step is not really necessary to make the point this ticket is making, but just for illustration purposes):

Access::config(array('default' => array(
    array(
        'requesters' => '*',
        'match' => 'Users::signup',
        'allow' => function() {
            return !Auth::check('default');
        }
    )
)));

Now access the signup form, and enter an existing email & password into the corresponding fields. If you have proper validation in place, you'd get your validation fields (because of duplicated email). However, you may notice that you are now logged in.

This is because in AuthRbac::_getRolesByAuth(), when calling Auth::check() the actual request object is being passed on to Auth. When you pass the request object to Auth, then the Form adapter can check for the submitted fields and look for a valid record, thus logging you in right from a signup form. We obviously don't want that.

A way to avoid this is to "emptying" the data array from the Request before doing the access check. So when filtering Dispatcher::_callable you can do:

$_data = $params['request']->data;
$params['request']->data = array();
$access = Access::check('default', null, $params);
$params['request']->data = $_data;

But obviously this is not so clean. I could easily make a pull request to avoid passing the Request instance to Auth::check() from the AuthRbac adapter, but I wanted to double check with you before doing so, because you may have had a purpose for passing it which I might be missing.

Also, is there any reason why the $requester argument to AuthRbac::check() is not being used?

$rules = (isset($options['rules']['rule'])) ? array($options['rules']) : $options['rules'];

So if $options['rules']['rule'] exists, which means $options['rules'] is an array, then cast $options['rules'] to an array and pass that value to $rules.

Otherwise, clearly $options['rules'] is not an array, therefore just pass it to $rules.

Isn't that the same as:

$rules = $options['rules']

?

Am I missing something?

(Sometimes my tone gets misunderstood on the Internet so to be clear, I'm not being snarky. Just want to clarify.)

I have an admin route and sometimes the admin param isn't specified (admin = null vs admin = true) in the route. With AuthRbac, this causes an issue when parsing the match. My fix is as follows but I haven't thoroughly tested it:

Line 147 AuthRbac.php, change

        if (!$exists_in_request || $value !== Inflector::underscore($request->params[$type])) {
            return false;
        }

to:

        if($value == null && !$exists_in_request) {
          continue;
        }
        if(!$exists_in_request || $value !== (is_string($request->params[$type]) ? Inflector::underscore($request->params[$type]) : $request->params[$type])) {
          return false;
        }

I didn't create a pull request because I also want to verify that there isn't an alternate way of doing this that I haven't realized?

More often than not when you use the 'allow' option in an AuthRbac rule to specify your own closure, you only use one closure. Currently such a rule has to be written as:

array(
    'requesters' => '*',
    'match' => 'Users::signup',
    'allow' => array(
        function($request, $options) {
            return !Auth::check('default');
        }
    )
)

Ideally you want to write this without the array, as such:

array(
    'requesters' => '*',
    'match' => 'Users::signup',
    'allow' => function($request, $options) {
        return !Auth::check('default');
    }
)

Coming up a pull request with change + tests to improve this.

Here's the scenario. If you have defined, somewhere in your bootstrap config, rules of Dispatcher configurations, like so:

Dispatcher::config(array('rules' => array(
    'admin' => array('action' => 'admin_{:action}'
)));

Access, and all the access adapters, do not take into account any modifications made to the route parameters. This is because Dispatcher::applyRules() makes the modifications to the $params array, instead of Request::$params. This is not going to change (double checked it with @nateabele), so we need a way for these dispatcher rules to be taken into account.

Before proceeding with the solution I implemented (coming up in a pull request in just a few minutes), let me give you a use case. You define a modification just as the one above so when the 'admin' route parameter is one, the action gets prefixed with admin_. You then add the appropriate route continuation so that this route parameter is set when an URL is accessed with /admin as a prefix:

Router::connect('/admin/{:args}', array('admin' => true), array('continue' => true));

Let us now add an auth rule (for the AuthRbac adapter), so that all admin prefixed accesses are only allowed access when the appropriate prefix is set in Auth. Now, a note here. I could easily change the following check to use $request->params['admin'] which would work just fine, however I'm trying to showcase a scenario where you would need to do something with the params after they have been modified by a dispatcher rule. Also, you'll see that I'm using a closure (instead of array of closures) directly in the match setting, the fix for this is also part of the pull request:

Auth::config(array('default' => array(
    'adapter' => 'AuthRbac',
    'roles' => array(
        array(
            'requesters' => '*',
            'match' => function($request) {
                return (strpos('admin_', $request->params['action']) === 0);
            },
            'allow' => function() {
                return Auth::check('admin');
            }
        )
    )
)));

The above code would never match since the $request->params used are the parameters parsed before applying any dispatcher rules.

To fix this, and as per @nateabele suggestion, I refactored li3_access so instead of passing the request when filtering Dispatcher::_callable, one can directly pass the $params. However, I made it backwards compatible, so code passing the Request instance should still work.

The match closure now receive an extra parameter which is the whole $params array. So after applying the pull request, you would first change how you call Access when filtering Dispatcher::_callable:

Dispatcher::applyFilter('_callable', function($self, $params, $chain) {
    $access = Access::check('default', null, $params);
    // ...
});

and the above 'match' would be fixed by changing it to:

'match' => function($request, $params) {
    return (strpos('admin_', $params['action']) === 0);
}
)));

As part of the pull request I also had to adapt the test cases that were using adapter methods directly.