HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

User Authentication/Permissions for PHP MySQL CRUD

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
permissionsuserphpauthenticationmysqlforcrud

Problem

I took my first stab at a user authentication function. I invented my own way of doing this. Is this a safe and efficient way of giving a user Read/Edit/Delete permission on tables in a MySQL database?

The Class

class Personnel {
//set tables that will need permissions and actions
protected static $actions = array("read", "edit", "delete");
protected static $table_names = array("species", "users", "news");

//this will generate a form with checkbox for each position in the permissions array
public static function set_permissions_form($current_permissions) {

    for($i=0; $i";
        for($z=0; $z";
            $form .= "";
        }
    }
    return $form;
}
}


The Form

If this is a new user or an existing user with no permissions set then $current_permissions will be NULL. If a permissions array does exist in the database unserialize it and check any checkbox when name = array key.

$current_permissions = unserialize($personnel->permissions);
$form = "";
$form .= Personnel::set_permissions_form($current_permissions);
$form .= "";
$form .= "";
echo $form;


Save

Once the form is submitted each box checked will be a "1" in the permissions array. This next snippet serializes the array and stores it in the database. I did not include code for $personnel->save() but you know what it does. Create user if does not exist, update user if it does.

$personnel = new Personnel();
$personnel->permissions = serialize($_POST['permissions']);
if($personnel->save()) {
    redirect_to("manage_personnel.php");
}


Check for Permission

The next steps I haven't completed yet but basically it will pull $personnel->permissions from database and unserialize it. Then say it is on a page where user is trying to edit species, $permissions["species"]["read"] == '1' ? you shall pass : you shall not pass;.

Am I heading in the right direction with this or am I over complicating?

Solution

Loops

Why are you using a for loop? I would just use a foreach loop, its much more efficient, especially for what you are doing here.

foreach(self::$table_names as $table_name) {
    //etc...
}


Strings

The following are micro-optimizations, but ones that are usually accepted as standard practice.

Single quotes should be used if no PHP processing is to be performed on a string. In other words, there are no PHP variables in the string. This is a micro optimization because it doesn't really add any noticeable overhead to your script. However, doing this you'll notice that you wont have to escape quotes nearly as frequently as you would have to if you started with single quotes. This is especially handy when dealing with HTML or XML attributes, which I'll get into soon.

$form .= $table_name . ':';


Double quotes should be used for strings that require PHP processing.

$form .= "$table_name:";


HTML/XML Attributes

While most browsers accept it, it is still improper syntax to use single quotes on HTML/XML tag attributes. XML will actually produce errors. Either escape the double quotes or escape the string and only go back into PHP for the variables.

$form .= $action . ": ][]"


Sanitizing and Validating

You shouldn't handle raw POST data. Always sanitize and validate it. Check out PHP's filter_input() function. That goes right along with what Shaad said about passing serialized data to your tables too. Its not a good idea, especially if it hasn't been sanitized and validated.

Comparisons

I would use absolute equality comparisons when checking for current permissions rather than loose comparison as your $actions table should only have boolean values. Also the quotes around an integer is unnecessary, unless you are comparing a string with absolute equality.

$form .= ($current_permissions[$table_name][$action] === TRUE ? " checked =\"checked\"" : "");

Code Snippets

foreach(self::$table_names as $table_name) {
    //etc...
}
$form .= $table_name . ':<br />';
$form .= "$table_name:<br />";
$form .= $action . ": <input type=\"checkbox\" name=\"permissions[$table_name][$action ]\"";
//OR
<input type="checkbox" name="permissions[<?php echo $table_name; ?>][<?php echo $action; ?>]"
$form .= ($current_permissions[$table_name][$action] === TRUE ? " checked =\"checked\"" : "");

Context

StackExchange Code Review Q#12092, answer score: 2

Revisions (0)

No revisions yet.