fixing logged_in result when using with an array. Making the $require_al...

[nyunyu95]

...l works too.

Feel free to contact me ! maybe i'm wrong !

best regards,

[bluesnowman]

Much appreciated. I will review the changes and merge them into the develop branch.

[bluesnowman]

I merged these changes into the 3.2/develop branch.

[bluesnowman]

@CubedEye Check out the changes to the Leap's Auth module for Kohana. I merged them into the 3.2/develop branch. Do you see any issues with merging them into the master?

[CubedEye]

@bluesnowman @nyunyu95 Hi all, I've looked at the changes, I just want clarification of what the issue was to begin with?

I've been using the Auth as for months in a production environment with a full suite of roles (being passed in as an array) and I haven't seen any issues with existing code.

Cheers.

[nyunyu95]

Hello CubedEye !

Well, I think (but I could may be wrong !!!) that you didn't see the issue but it exists ! Actually we have this : if (!$role || !$role || (($role instanceof $role_model){...}

As $role is a DB_ResultSet variable type, it'll ALWAYS exists. It doesn't matter if the query matched any records, you always will be enter the if statement.

That's why I got the idea to test $role->count() instead. Indeed that method tells us how many records were found and allows us to enter or not the statement.

What do you think ? :p

Best Regards,

[CubedEye]

Hi @nyunyu95 and @bluesnowman ,

I have checked the current version 'master' branch which is a a touch newer than my dev copy. But somewhere along the line, this (My Dev Copy):

$role = DB_ORM::select($this->models['role']) ->where($this->columns['role_name'],DB_SQL_Operator::_EQUALTO,$role) ->query(1)->fetch(0);

was changed to this (master branch):

$role = DB_ORM::select($this->models['role']) ->where($this->columns['role_name'], DB_SQL_Operator::_EQUALTO, $role) ->limit(1) ->query();

In 'my dev copy' I was using FETCH to get the first (and only) item, which should have been a Model_Leap_Role object.

@bluesnowman:

Unless I'm mistaken, doesn't '->limit(1)->query()' still return a DB_Result_Set? If so, then I question if the if statement is actually ever evaluating to true.

If not, then the $roles->count() is not going to be required because $role should actually be a single instance of Model_Leap_Role.

Thoughts?

Cheers.

[nyunyu95]

@CubedEye, I just tested it out and ->query() always return a DB_ResultSet ! ^^ That's why I'm trying to explain you ! (forgive my bad english... I'm french huhu)

[CubedEye]

@nyunyu95 Well if that's the case then:

if ( ! $role || (($role instanceof $role_model) && ! in_array($role->id, $user_roles)))

is never going to be true because it's expecting $role to be an instance of Model_Leap_Role.

My solution would actually be to replace:

$role = DB_ORM::select($this->models['role'])
->where($this->columns['role_name'], DB_SQL_Operator::_EQUAL_TO_ , $role)
->limit(1)
->query();

with the following:

$role = DB_ORM::select($this->models['role']) ->where($this->columns['role_name'],DB_SQL_Operator::_EQUAL_TO_ ,$role)
->query(1)->fetch(0);

To get an instance of Model_Leap_Role rather than count the number of results. I only say this because if somewhere along the line that makes that not return a role object then it's no longer checking the roles, but rather that the query only returned something.

[bluesnowman]

@CubedEye Thank you for debugging this issue. I will commit these changes shortly.

FYI, I went through the commit history on leap/classes/base/auth/leap.php to see if anything else changed as well. All I found was this commit (which shows my style changes to these lines of code):

https://github.com/CubedEye/kohana-orm-leap/commit/57b5c89cbc861604e5c9aef97f60c71198260fd3#diff-0

It looks like the fetch(0) was also missing from the original code. Would you mind comparing your local copy with the current version in the '3.2/develop' branch and let me know if there are any other differences that should be updated?

Again, I really appreciate all of what you have contributed.

[CubedEye]

@bluesnowman I've had a quick look through, I don't think the is anything else that could cause a problem. I'll keep an eye out though.

[nyunyu95]

Sorry to disturb you again guys but, this doesnt really fixes the problem. Indeed with the "->fetch(0)" solution if $require_all=TRUE .

For exemple : Auth_Leap::instance()->logged_in(array('somethingnotexisting', 'admin', 'user'), FALSE)

Will always return FALSE even if the user has the admin and user roles.

Regards,

[CubedEye]

Excuse my last comment that I have removed. I haven't been using the all required functionality so I can't confirm that it works, but as commented in the below I don't think there is any flaw in the logic (once the ->fetch(0) is put back in),

if you can find the issue then let me know and I'll have another look at it:

public function logged_in($roles = NULL, $all_required = TRUE) {
    $user = $this->get_user(); //Get the user

    //Settings
    $user_model = DB_ORM_Model::model_name($this->models['user']);
    $role_model = DB_ORM_Model::model_name($this->models['role']);

    //If there is a user, proceed
    if (($user instanceof $user_model) && $user->is_loaded()) {

        // If no roles defined then user is just logged in
        if ( ! $roles) {
            return TRUE;
        }

        // Make array of user roles ids.
        $user_roles = array();
        foreach ($user->user_roles as $user_role) {
            $_role = $user_role->role;
            if ($_role instanceof $role_model) {
                array_push($user_roles, $_role->id);
            }
        }

        if (is_array($roles)) { //Multiple roles passed
            $status = (bool) $all_required;

            foreach ($roles as $role) {

                //If you haven't passed in a role object then get it from the DB, by the name passed in the array.
                if ( ! is_object($role)) { // || !($role instanceof Model_Leap_Role))
                    $role = DB_ORM::select($this->models['role'])
                    ->where($this->columns['role_name'], DB_SQL_Operator::_EQUAL_TO_, $role)
                    ->query(1)
                    ->fetch(0);
                }

                // If it's NOT a role or (if it IS a role but NOT in the user's role list)
                if ( ! $role || (($role instanceof $role_model) && ! in_array($role->id, $user_roles))) {
                    $status = FALSE;
                    if ($all_required) {
                        break; //Break out of for loop
                    }
                }

                // If the role does exist in the users list and $all_required is false
                // Then user has at least one role, so set $status to TRUE and break out of for loop
                else if ( ! $all_required) {
                    $status = TRUE; 
                    break;
                }
            }
        }
        else { // Single Role
            if ( ! is_object($roles)) { // || ! ($role instanceof Model_Leap_Role)) {
                $role = DB_ORM::select($this->models['role'])
                ->where($this->columns['role_name'],DB_SQL_Operator::_EQUAL_TO_,$roles)
                ->query(1)
                ->fetch(0);

                $status = FALSE;

                if ($role && ($role instanceof $role_model)) {
                    if (in_array($role->id, $user_roles)) {
                        $status = TRUE; 
                    }
                }
            }
        }
        return $status;
    }
    return FALSE; //No user returned
}