Group based permissions

[paddy_fields]

Hi, I've written the function below to check the permissions of a user based on the table structure of....

 

staff

staff_roles_id (foreign)

 

staff_roles

id

roleName

 

staff_roles_permissions

staff_roles_id (foreign)

staff_permissions_id (foreign)

 

staff_permissions

id

permissionName

function checkPerm($permission){

        global $db;

        $user = $_SESSION['userid'];

        if(!$stmt = $db->prepare("SELECT * FROM staff LEFT JOIN staff_roles ON staff.staff_roles_id = staff_roles.id LEFT JOIN staff_role_permissions ON staff_roles.id = staff_role_permissions.staff_roles_id LEFT JOIN staff_permissions ON staff_role_permissions.staff_permissions_id = staff_permissions.id WHERE staff.id = ? AND staff_permissions.permissionsName = ?")){
            echo $db->error;
            exit;
        }

        $stmt->bind_param('is',$user,$permission);
        if(!$stmt->execute()){
            echo $db->error();
            exit;
        };
        $stmt->store_result();
        $authenticate = $stmt->num_rows;
        $stmt->close();

        return $authenticate;

}

So for example if I then had the permission of 'adminAccess', I would use the code below to check access, referring to the 'staff_permissions' table

if(checkPerm('adminAccess')){
echo 'you are authorised';
exit;
}
else
{
echo 'you are not authorised';
exit;
}

This works, but Is this the correct way to be going about access for group based permissions or am I missing a glaring security vulnerability here?

 

If this is suitable then I intend to turn it into a class, and add checkRole() which would just check the user against the 'staff_roles' table.  This would then be called by $security->checkRole('example');

$security->checkPermission('example'). Would that be a good idea?

 

Any advice would be great - I'm not great with functions (and just starting to learn classes!)

 

Cheers.

 

[kicken]

The security stuff is fine. The one suggestion I would make is to change your function so that rather than query for individual permissions it queries for all permissions a user has and stores that in a variable cache somewhere, then when you need to know if a user has a permission you check that cache. That will prevent you from needing to run a bunch of queries if you need to do a number of different permission checks on the same request.

 

Other than that, you just need some general code clean up and re-factoring to get rid of things like global $db and using echo and exit for error handling. I am going to assume this will be handled when you convert it to a class later as mentioned.

[paddy_fields]

Should I be using 'private' instead of 'global'...? This is where my knowledge of functions is quite limited. From what I've read I needed to use 'global' as $db is defined outside of the function itself

 

Thanks for the variable cache suggestion. I could possibly do that query in the script that processes the users log-in request

[paddy_fields]

I've given this another go, using a session to store an array of user permissions during the login script - so there is only one query as suggested;

 

The function then uses 'in_array' to see if the permission is allowed.

/* permissions */

// find the staff members permissions and store as session array
if(!$stmt = $db->prepare("SELECT permissionsName FROM staff_permissions LEFT JOIN staff_role_permissions ON staff_role_permissions.staff_permissions_id = staff_permissions.id LEFT JOIN staff_roles ON staff_role_permissions.staff_roles_id = staff_roles.id LEFT JOIN staff ON staff.staff_roles_id = staff_roles.id WHERE staff.id = ?")){
    echo $db->error;
    exit;
}

$stmt->bind_param('i',$user_id);
$stmt->execute();
$result = $stmt->get_result();
$stmt->close();

$permissionsArray[] = NULL;

while($row = $result->fetch_assoc()){

    $permissionsArray[] = $row['permissionsName'];

}

$_SESSION['permissionsArray'] = $permissionsArray;

function checkPerm($permission){

    if(in_array($permission, $_SESSION['permissionsArray']))
    {
        $auth = TRUE;
    }

    else
    {
        $auth = FALSE;
    }

    // return true if authenticated, otherwise false
    return $auth;

}

This works ok... can anyone see any security issues with this at all?

Edited July 24, 2014 by paddy_fields

[Jacques1]

Caching the permissions in the session is a very bad idea, because changes only take effect after the session has ended. If you take away a permission from a user, they still have it for as long as they can keep the session alive, and this may be forever.

 

I think the whole caching idea doesn't make a lot of sense and is downright harmful. The point of caching is to improve performance, but you haven't said anything about performance issues. So you're effectively replacing a virtual issue with a real issue: Now you have to worry about cache invalidation, and if you screw this up, the whole permission system falls apart.

 

So throw away the caching stuff and just use a plain function. Should you indeed experience performance issues, approach this in a smart way:

• Is the permissions function actually the cause?
• If that's the case, is the performance advantage worth the security disadvantage?
• If both is the case, then it's time to think about the best approach. And that's certainly not a session-based cache, because you need to be able to invalidate the cache at any time. 

Besides that, you've developed some strange coding habits:

• Do you really think it's a good idea to dump all MySQL error messages on the screen for the whole world to see? Why even mess with manual error handling? Turn on error reporting in the mysqli_driver class, and MySQLi will either trigger an error or throw an exception whenever something goes wrong.
• Why do you fetch the entire permissions rows to count them in PHP? Are you aware that MySQL can do the counting itself? You can also use an EXISTS query, because you only care if there's any row.
• You need to stop polluting your code with exit statements. This makes the control flow extremely hard to understand and is also very error-prone, because who knows what your code does if you forget one of those statements? Create a proper structure which runs from the top to the bottom without any tricks.
• What exactly is the point of mapping the return value of in_array() to a boolean with an if statement? The return value already is a boolean, and it doesn't get “truer” or “falser”. So why not just return it?
• Please start formatting your queries. Those endless one-liners are really a pain to read.
• Doing a left join is nonsensical, because all additional NULL rows immediately get rejected by the WHERE clause. You need an inner join.
• The staff_roles table is useless in your query.
• Don't mix different naming schemes in your table. Drop the camelCase notation and always use underscores.

<?php

function checkUserPermission($permission)
{
	global $db;

	$hasPermission = false;

	if (isset($_SESSION['userid']))
	{
		$permissionCheckStmt = $db->prepare('
			SELECT EXISTS (
				SELECT 1
				FROM
					staff
					JOIN staff_roles_permissions ON staff.staff_roles_id = staff_roles_permissions.staff_roles_id
					JOIN staff_permissions ON staff_roles_permissions.staff_permissions_id = staff_permissions.id
				WHERE
					staff_id = ?
					AND staff_permissions.permissions_name = ?
			)
		');
		$permissionCheckStmt->bind_param('is', $permission, $_SESSION['userid']);
		$permissionCheckStmt->execute();
		$permissionCheckStmt->bind_result($permissionAssignmentExists);
		$permissionCheckStmt->fetch();

		$hasPermission = (boolean) $permissionAssignmentExists;

	}

	return $hasPermission;
}

(completely untested)

 

 

[paddy_fields]

Thank you very much, I really appreciate your help with this. Now you've pointed out the flaw with using sessions for this I do feel quite stupid for not realising that to start with...

 

I've tweaked your code slightly and got it working like a charm and I've learned a lot from the way you've written it, cheers buddy.

 

I wasn't aware of the EXISTS query in MySQL but I'll read up in more detail now. I'm sure there is some code I can go back to and clean up with that in mind. I've got a lot of learning to do.

 

The MySQL error reporting and use of 'exit' was just something I had in there for testing purposes while I was writing/playing with it, it was going to be taken out as soon as it was working as expected. But yes you're right of course I should just turn on error reporting!

 

Also I didn't realise in_array returned a boolan... face palm. I've never used it before but I obviously should have referred to the manual.

 

Thanks again.

Edited July 24, 2014 by paddy_fields

[kicken]

Regarding the caching, what I typically will do is cache them in a variable. They are still loaded upon each request so a change in permissions is reflected immediately but several permission inquiries within the same request will not result in several separate queries.

 

eg:

function checkUserPermission($permission)
{
	global $db;
	static $cache;

	$hasPermission = false;	
	if (isset($_SESSION['userid']))
	{
		$uid = $_SESSION['userid'];
		if (!isset($cache[$uid])){
			$permissionCheckStmt = $db->prepare('
			SELECT 
				staff_permission.permission_name
			FROM
				staff
				JOIN staff_roles_permissions ON staff.staff_roles_id = staff_roles_permissions.staff_roles_id
				JOIN staff_permissions ON staff_roles_permissions.staff_permissions_id = staff_permissions.id
			WHERE
				staff_id = ?
			');
			$permissionCheckStmt->bind_param('i', $uid);
			$permissionCheckStmt->execute();
			$permissionCheckStmt->bind_result($permissionName);
			
			$cache[$uid] = array();
			while ($permissionCheckStmt->fetch()){
				$cache[$uid][] = $permissionName;
			}
		}

		$hasPermission = in_array($permission, $cache[$uid]);
	}

	return $hasPermission;
}