Usage of Auth::check in AuthRbac makes signup forms log in user #21

Open
mariano opened this Issue Feb 27, 2012 · 3 comments

Comments

Projects
None yet
3 participants
Contributor

mariano commented Feb 27, 2012

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?

Contributor

mariano commented Mar 1, 2012

@tmaiaroto any feedback on this? thanks :)

Owner

tmaiaroto commented Mar 5, 2012

Sorry, just got back from vacation. I need to wrap my head around this. I also don't use the Auth in this manner, so I will need to play around and see the results to fully understand. I kinda hear what you're saying here from quick glance.

Contributor

Ciaro commented Mar 7, 2012

@mariano $requester -> 'This is an optional parameter, because we will fetch the users data through Auth seperately'.
The variable doesn't seem to be taken in account in the method, though, so nice catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment