6
\$\begingroup\$

I have been working on creating my own implementation of an RBAC system for the purpose of:

  • learning to use an OOP approach instead of procedural
  • switch from using mysqli to PDO
  • and to learn how to use dependency injection

DB Connect File:

<?php
$config = include '../config.file.php';

try {
     $db = new PDO('mysql:host='.$config['dbhost'].';dbname='.$config['dbname'].';charset=utf8mb4', $config['dbusername'], $config['dbpass']);
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
} catch(PDOException $ex) {
    echo $ex->getMessage();
  }

Main Page:

<?php
session_start();

// Starts the Session Expire Checker
require 'SessionExpireChecker.php';

// Checks whether user is a valid user
if (isset($_SESSION['valid_user'])) {

    require_once './classes/Role.php';
    require_once './classes/EntitledUser.php';
    require_once '../DBConnect.php';

    $u = EntitledUser::getByUsername($db, $_SESSION['valid_user']);

    if ($u->isEntitled('can_view_testme')) {
        echo "You have permission to read.<br />";
    }

    if ($u->isEntitled('can_edit_testme')) {
        echo "You have permission to edit/update.<br />";
    }

    if ($u->isEntitled('can_delete_testme')) {
        echo "You have permission to delete.<br />";
    }

    if ($u->isEntitled('can_create_testme')) {
        echo "You have permission to create/add.<br />";
    }

}

EntitledUser Class:

<?php

class EntitledUser
{
    private $db;
    private $roles;

    public function __construct(PDO $db) {
        $this->db = $db;
    }

    public static function getByUsername(PDO $db, $username) {
        try {
            $stmt = $db->prepare("SELECT authorized_users.USERname FROM authorized_users WHERE authorized_users.USERname = :usernamevar");
            $stmt->bindValue(':usernamevar', $username, PDO::PARAM_STR);
            $stmt->execute();
        } catch(PDOException $ex) {
            echo $ex->getMessage();
        }
        $result = $stmt->fetchAll(PDO::FETCH_ASSOC);

        if (!empty($result)) {
            $entUser = new EntitledUser($db);
            $entUser->user_id = $result[0]['USERname'];
            $entUser->initializeRoles();
            return $entUser;
        } else {
            return false;
        }
    }

protected function initializeRoles() {
    $this->roles = array();

    try {
        $stmt2 = $this->db->prepare("SELECT rbac_user_roles.USERname, rbac_user_roles.role_id, rbac_roles.role_id, rbac_roles.role_name 
        FROM rbac_user_roles, rbac_roles 
        WHERE rbac_user_roles.role_id = rbac_roles.role_id
        AND rbac_user_roles.USERname = :user_idvar");

        $stmt2->bindValue(':user_idvar', $this->user_id, PDO::PARAM_STR);
        $stmt2->execute();
    } catch(PDOException $ex) {
        echo $ex->getMessage();
    }

    while($row = $stmt2->fetch(PDO::FETCH_ASSOC)) {
        $this->roles[$row['role_name']] = Role::getRolePermissions($this->db, $row['role_id']);
    }
}

// check if user has a specific entitlement
public function isEntitled($permission) {
    foreach ($this->roles as $role) {
        if ($role->hasPermission($permission)) {
            return true;
        }
    }
    return false;
}
}

Role Class:

<?php

class Role
{
    protected $permissions;

    protected function __construct() {
        $this->permissions = array();
    }

    // return a role object with permissions
    public static function getRolePermissions(PDO $db, $role_id) {
        $role = new Role();

        try {
            $stmt3 = $db->prepare("SELECT rbac_role_permissions.role_id, rbac_role_permissions.permission_id, rbac_permissions.permission_id, rbac_permissions.permission_description 
            FROM rbac_role_permissions, rbac_permissions 
            WHERE rbac_role_permissions.permission_id = rbac_permissions.permission_id
            AND rbac_role_permissions.role_id = :role_idvar");

            $stmt3->bindValue(':role_idvar', $role_id, PDO::PARAM_STR);
            $stmt3->execute();
        } catch(PDOException $ex) {
            echo $ex->getMessage();
        }


        while($row = $stmt3->fetch(PDO::FETCH_ASSOC)) {
            $role->permissions[$row['permission_description']] = true;
        }
        return $role;
    }

    // check if a permission is set
    public function hasPermission($permission) {
        return isset($this->permissions[$permission]);
    }
}

After some debugging in my IDE, the code ran successfully this morning and returned that the logged in user had each permission correctly. Since this is my first time using dependency injection I am wondering if I have implemented it correctly and passed the $db object where appropriate, and type hinted where applicable. I am also curious as to whether my code is creating duplicate instances of the database connection that I am not aware of.

\$\endgroup\$
3
\$\begingroup\$

My first impression on the code is that it looks messy. This is because you didn't let your IDE format and indent the code automatically. Doing that would have transformed this code:

     $db = new PDO('mysql:host='.$config['dbhost'].';dbname='.$config['dbname'].';charset=utf8mb4', $config['dbusername'], $config['dbpass']);
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

into:

$db = new PDO('mysql:host='.$config['dbhost'].';dbname='.$config['dbname'].';charset=utf8mb4', $config['dbusername'], $config['dbpass']);
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

Both lines now start in the same column, as they should. Only when a { or } or other hierarchical element appears in the source code, the indentation should change.

The PHP statement that executes the SQL statement should be written:

$stmt2 = $this->db->prepare("
    SELECT
        rbac_user_roles.USERname,
        rbac_user_roles.role_id,
        rbac_roles.role_id,
        rbac_roles.role_name 
    FROM
        rbac_user_roles,
        rbac_roles
    WHERE
        rbac_user_roles.role_id = rbac_roles.role_id
        AND rbac_user_roles.USERname = :user_idvar");

Sure, this needs a lot more vertical screen space than before. But in return, the list of selected columns is now very easy to read. Every reader can immediately say how many fields are returned and from which table each field is. The most important part though is that the SQL keywords start in a different column of the source code. As an extreme example, try to read the following code:

printf("%s%s", 'hello
printf("%s%s%s%s", ', 'arg2
exit();
return false; // ');

It is extremely confusing because all lines start in the same column. The meaning of each of the lines is totally different, though.

Since while is a keyword and not a function, there needs to be a space after it. It should be while (...) instead of while(...).

Apart from all these formatting and layout issues, the code looks reasonable. It's especially good that Little Bobby Tables has no chance of exploring your database.

You should take more care when naming your variable names. Why did you choose :user_idvar over the much simpler :username? I don't see any reason for that. Plus, the id part of the variable name is wrong. That variable contains the name, not the ID.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for this feedback and the time you took to be so thorough. I will definitely make all of the changes you mentioned as it relates to code formatting as it definitely looks much cleaner. The :user_idvar variable name I chose is due to my lack of experience with named placeholders. I was used to using ? in prepared statements with mysqli before and because I'm still getting used to the OOP approach and I wanted the names to be different to differentiate them from the parameter argument of the same name, the placeholders, and $entUser->user_id. \$\endgroup\$ – Statik Stasis Apr 24 '18 at 20:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.