Registration Script Optimization

[sikuneh]

I want to make sure there is nothing substantial that I'm missing, security or performance, on my registration script.

PHP scripts (adduser.php)

<?php ## Validate Registration ##
session_start();
include("../../scripts/modules.php");


$vars = array(
'username'=>$_POST['username'],
'password'=>$_POST['password'],
'confirmPassword'=>$_POST['confPass'],
'email'=>$_POST['email'],
'confirmEmail'=>$_POST['confEmail'],
'gender'=>$_POST['gender'],
'firstName'=>$_POST['firstName'],
'lastName'=>$_POST['lastName'],
'b-m'=>$_POST['bday-m'],
'b-d'=>$_POST['bday-d'],
'b-y'=>$_POST['bday-y']);

$errors = array();

## Check for empty fields ##
foreach($vars as $values=>$label) {
	if(empty($values)) {
		$errors[]="$label is invalid";
	}
}

## Validate entries ##

	## Check for matching passwords ##
if($vars['password'] != $vars['confirmPassword']) {
	$errors[]="Passwords do not match.";
}

	## Check for matching emails ##
if($vars['email'] != $vars['confirmEmail']) {
	$errors[]="Emails do not match.";
}

	## Check for valid username ##
	$getUsername = $db->prepare("
		SELECT username 
		FROM users
		WHERE username = :name");
	
	$getUsername->bindValue(":name",$_POST['username']);
	$getUsername->execute();
	
	if($getUsername->rowcount() !== 0) { $errors[]="Username is taken"; }

## Combine birthday to full mm-dd-yyyy format ##
$bday = $_POST['bday-m'].
		"-".
		$_POST['bday-d'].
		"-".
		$_POST['bday-y'];

## Encrypt password ##
$password = hash("sha256",$vars['password']);		
		
## If any errors ##
if($errors) {
	userError($errors);
	echo "<a href=\"../../index.php?q=users/register\">Try Again</a>";
}

## Otherwise continue ##
else {
	try {
		$addUser = $db->prepare("
			INSERT INTO users(id,username,password,email,gender,firstname,lastname,birthday,status)
			VALUES(0,:user,:pass,:email,:gender,:firstName,:lastName,:bday,0)
		");
			$addUser->bindValue(":user",$_POST['username']);
			$addUser->bindValue(":pass",$password);
			$addUser->bindValue(":email",$_POST['email']);
			$addUser->bindValue(":gender",$_POST['gender']);
			$addUser->bindValue(":firstName",$_POST['firstName']);
			$addUser->bindValue(":lastName",$_POST['lastName']);
			$addUser->bindValue(":bday",$bday);
		$addUser->execute();
		
		## Start Session ##
			$_SESSION['loggedin'] = $_POST['username'];
		 
		## Redirect to welcome page ##
		header("Location:../../index.php?q=users/welcome");
	
	} catch(PDOException $e) { $entry = uniqID();
		reportError(
			$e->getMessage(),
			"Cannot add user to database. If you feel you have reached this in error, please contact the administrator and reference this ID: $entry", 
			"../../logs/errors.log",
			"adduser.php",
			$entry
		);
	}
}
?>

Database Structure:

id
username
password
email
gender
firstname
lastname
birthday
status /* Active */

If there is a better way to structure this that I'm not using, please tell me, because I want this to be as high quality as it can be.

 

[Skizzerz]

1. Please post the code for bindValue. I can't tell if you have major SQL injection issues unless I can see that function or not. What happens when I sign up with the username foo"; DROP TABLE users;-- or foo'; DROP TABLE users;--
2. Try to modify bindValue or introduce a new function bindValues so that you can pass in an array of what to search for and replace. This will reduce the number of function calls you'll need to perform, which will speed up your code (very) slightly.
3. Your empty fields check is wrong. It appears that if a field is empty, you'd be printing out " is invalid" because you're printing $label (which is the array value) instead of $values (which is the array key).

 

[misson]

Note PDOStatement::bindValue is a library function. Regarding point 2, PDOStatement::execute can take an array of values to bind to query parameters.

As for a better way to structure user account creation, the professional way would be to separate different concerns into different modules, where the exact mechanisms would be abstracted away and they could then be tested independently and easily replaced with other mechanisms. For example, the hashing of a password would be handled by an authentication module, and you'd use a data access layer to handle data persistence. See "10 ORM patterns" for an overview of data access patterns.

When it comes to modeling names (currently done with the "firstname" and "lastname" columns in the table), the approach that works best across cultures is to store a full name and a short name, which is the name someone is generally addressed by (I don't know of a better term).

 

[sikuneh]

So something to this effect:

Registration Script (addUser.php)

<?php ## Validate Registration ##
session_start();
## Base Modules ##
include("../../scripts/modules.php");
## Registration Modules ##
include("../../scripts/registration_modules.php");


$vars = array(
'username'=>$_POST['username'],
'password'=>$_POST['password'],
'confirmPassword'=>$_POST['confPass'],
'email'=>$_POST['email'],
'confirmEmail'=>$_POST['confEmail'],
'gender'=>$_POST['gender'],
'fullName'=>$_POST['fullname'],
'shortName'=>$_POST['shortname'],
'b-m'=>$_POST['bday-m'],
'b-d'=>$_POST['bday-d'],
'b-y'=>$_POST['bday-y']);

$errors = array();

## Revised: ##
## Run Authentication ##
checkEmpty($vars);
matching($vars['password'],$vars['confirmPassword'],"Passwords");
matching($vars['email'],$vars['confirmEmail'],"Emails");
varify($username,$password);
	
## If any errors ##
if($errors) {
	userError($errors);
	echo "<a href=\"../../index.php?q=users/register\">Try Again</a>";
}

## Otherwise continue ##
else {
	try {
		$addUser = $db->prepare("
			INSERT INTO users(id,username,password,email,gender,firstname,lastname,birthday,status)
			VALUES(0,?,?,?,?,?,?,?,0)
		");
		$addUser->execute(array($_POST['username'],$password,$_POST['email'],$_POST['gender'],$_POST['firstname'],$_POST['lastname'],$bday));
		
		## Start Session ##
			$_SESSION['loggedin'] = $_POST['username'];
		 
		## Redirect to welcome page ##
		header("Location:../../index.php?q=users/welcome");
	
	} catch(PDOException $e) { $entry = uniqID();
		reportError(
			$e->getMessage(),
			"Cannot add user to database. If you feel you have reached this in error, please contact the administrator and reference this ID: $entry", 
			"../../logs/errors.log",
			"adduser.php",
			$entry
		);
	}
}
?>

Registration Modules (registration_modules.php)

<?php
## Modules for Registration ##

## Check for empty fields ##
	function checkEmpty($array) {
		foreach($vars as $values=>$label) {
			if(empty($values)) {
				$errors[]="$label is invalid";
			}
		}
	}
	
	## Matching Values ##
	function matching($value1,$value2,$input) {
		if($value1 != $value2) {
			$errors[]="$input"."s do not match.";
		}
	}
	
	## Varify information ##
	function varify($username,$password) {
		$getUsername = $db->prepare("
		SELECT username 
		FROM users
		WHERE username = :name");
	
	$getUsername->bindValue(":name",$_POST['username']);
	$getUsername->execute();
	
	if($getUsername->rowcount() !== 0) { $errors[]="Username is taken"; }
	
	$password = hash("sha256",$password);
}	

	## Combine birthday to full mm-dd-yyyy format ##
	$bday = $_POST['bday-m'].
			"-".
			$_POST['bday-d'].
			"-".
			$_POST['bday-y'];		
?>

 

[misson]

Closer. The registration script might look something like:

<?php
$userForm = new UserForm;
if (($errors = $userForm->validate($_POST))) {
    # UserForm::display takes keyword arguments that affect display.
    $userForm->display(array('errors' => $errors));
    ...
} else {
    # the following step may or may not be necessary
    $data = $userForm->prepareData($_POST);
    try {
        # Attempt to create a user. UserData is a manifestation of the Data Mapper pattern
        $user = UserData::create($data);

        # Creation successful.
        Auth::default()->login($user->name, $_POST['password']);
        
        header("Location: ../../index.php?q=users/welcome");
    } catch (UserInputException $exc) {
        # This includes the case where a username is already in use. 
        # UserInputException::errors is an array of errors, indexed by the 
        # field name the error applies to or an integer index if the error doesn't
        # apply to a specific field.
        $userForm->display(array('errors' => $exc->errors));
    } catch (DataException $exc) {
        # Persistance layer error.
        reportError(...);
    } catch (Exception $exc) {
        # System error.
        reportError(...);
    }
}

Or:

<?php
$userForm = new UserForm;
if ($userForm->validate($_POST)) {
    # the following step may or may not be necessary
    $data = $userForm->prepareData($_POST);
    try {
        ... /* Same as above */
    } catch (UserInputException $exc) {
        $userForm->errors = $exc->errors;
        $userForm->display();
    } catch (DataException $exc) {
        ... /* Same as above */
    }
} else {
    # $userForm will display errors from a failed validation
    $userForm->display();
}

Or:

<?php
... /* Same as above */
    try {
        # The data access layer may allow "creation" of user objects
        # for existing users by turning a "create" into an "update".
        # Since this isn't what we want when adding a user, we first check 
        # for existence.
        if (UserData::exists($data)) {
            $userForm->errors = $exc->errors;
            $userForm->display();
        } else {
            # Attempt to create a user
            $user = UserData::create($data);
            # Creation successful.
            Auth::default()->login($user->name, $_POST['password']);
            
            header("Location: ../../index.php?q=users/welcome");
        }
    } catch (DataException $exc) {
        ... /* Same as above */
    }
...

Note how high-level the task are in these scripts, which (hopefully) make them easier to follow.

You may or may not need the data preparation step (the call to UserForm:repareData) before attempting to create a user. It will pre-process POST data into a format suitable to be passed to UserData::create; if no pre-processing is necessary, the step can be skipped, or (better yet) UserForm:repareData can return its argument unprocessed. The latter is better because other implementations may need pre-processing, or pre-processing may need to be added in the future; with the call in place, all that would need to be done is to fill out the method body. The method could also be a part of another module (such as UserData), but since it involves two different layers (presentation and persistence), it's a bit of an odd man out. The technical name for this type of thing is a cross cutting concern.

You don't necessarily need a special class (UserForm in the example) for the registration form, though you should have some way of defining the form in just one place. There could be a generic form that you pass a UserData instance or property to. Validation could be handled by a form class or a validator class, or the form class could use a validator class (the form defines the fields, field formats and which fields are required and which optional, passing this information to the validator).

Also, rather than having the script body creating objects (either with new or by getting an instance from a class method), put the script body in a function and pass it the instances, a pattern called dependency injection. The use of Auth::default() in the samples accomplishes the same goal (dependency inversion); the UserData::create call may achieve this while new UserForm certainly doesn't.

Note that these sample (especially UserForm) are only some ways of doing it. The best way of approaching this is to describe what the add user script does at a high level, then dole out those tasks to different modules, depending on what each task is primarily concerned with. As for modules based on classes, each class should have a single responsibility, which is one of the five SOLID design principles.

 

[sikuneh]

I am doing some research into PHP Classes but I'm not very experienced with them. Could you give me an example of what you're trying to do with

$userForm = new UserForm; 
if (($errors = $userForm->validate($_POST))) { 
    # UserForm::display takes keyword arguments that affect display. 
    $userForm->display(array('errors' => $errors));

Like what should the class look like?

I was trying to, like you said, send a general form through a reusable class

Example: (modules.php)

## Class Validation ##
class validation {
	function valUsername($name,$db,$type,$errors) {
		$sql = $db->prepare("SELECT username FROM users WHERE username = ?");
		$sql->execute(array($name));
		
		$typeNum = ($type == "login") ? 0 : 1;
		
		if($sql->rowcount()==$typeNum) { $errors[]="Invalid Username"; }
		return $errors;
	}
	
	function valPassword($pass,$db,$type,$errors) {
		$password = hash("sha256",$pass);
		
		$sql = $db->prepare("SELECT password FROM users WHERE password = ?");
		$sql->execute(array($password));
		
		
		$typeNum = ($type == "login") ? 0 : 1;
		
		if($sql->rowcount()==$typeNum) { $errors[]="Invalid Password"; }
		return $errors;
	}
}

Login Validation: (validate.php)

$errors = validation::valUsername($_POST['username'],$db,"login",$errors);
$errors = validation::valPassword($_POST['password'],$db,"login",$errors);

$errors is the array of errors, I don't know if this was the right way to do that.

 

[misson]

My original post was much more detailed, but got et by a gremlin.

I am doing some research into PHP Classes but I'm not very experienced with them. Could you give me an example of what you're trying to do with

$userForm = new UserForm; 
if (($errors = $userForm->validate($_POST))) { 
    # UserForm::display takes keyword arguments that affect display. 
    $userForm->display(array('errors' => $errors));

This topic is bigger than can be covered in a forum thread. Find a good book on OOP. StackOverflow has many questions with book recommendations; search there.

For the immediate problem, stick with the S in SOLID. UserForm is responsible for the user registration form. It records the input fields, including their formats (which are specified in a special purpose language you pick as a format language, which could be regex or some other pattern language), types (HTML, DB, what-have-you), and which are necessary and which optional.

To follow the single responsibility principle completely, you'd also have a FormView class to handle form display rather than a UserForm::display method (given a form, a FormView renders it as HTML). I didn't want to overwhelm you with too many classes, so I didn't have anything like it in my previous post.

For the same reason, you could have a FormValidator class (which was mentioned in the previous post, but only in passing). From another point of view, "validating" is a verb and thus should correspond to a method, not a class. This last point is part of a much more advanced topic that you don't need to worry about now, but take a look at how Common Lisp handles OOP (which is the Common Lisp Object System) if you're curious to walk that path.

The other technique that should help is continue with the top-down design: for each method (which are implementations of tasks), write it in terms of sub-tasks.

Like what should the class look like?

Are you asking for the interface or the implementation? The phrasing of the question suggests the former.

When it comes to interface, the filter functions show one way of approaching validation.

class validation {

Style varies, but most of them require class names to be capitalized. This doesn't matter to the PHP interpreter (which is case insensitive, when it comes to class and function names), but is important for anyone reading your code. (The main point of contention when it comes to naming is whether to use underscores or CamelCase.)

Login Validation: (validate.php)

$errors = validation::valUsername($_POST['username'],$db,"login",$errors);
$errors = validation::valPassword($_POST['password'],$db,"login",$errors);

Did you mean to write $errors[] = validation::...?

Shortening "validate" to "val" decreases readability. I recommend writing it out in full, and not having the methods static.

valUsername is too specific. Better is to have more generic validation methods that apply to fields based on type or format. If your valUsername checks whether a username is taken, that's not a validation task. Validation is checking that all required fields are defined, and all input has the correct format.

Passwords are special enough that it makes sense to have a method that applies to them, if you have password requirements such as mixed case and at least one numeric and symbolic character, especially since these requirements are messy to specify in a format language that isn't a Turing complete programming language.

$errors is the array of errors, I don't know if this was the right way to do that.

It looks fine. You need some data structure to store validation errors. Which one depends on how you use it. I typically use an array indexed by field name (for errors that apply to specific fields), or indexed numerically for general errors. The field-specific errors get displayed next to the corresponding field, and general errors are displayed at the top of the form.

 

[creastery]

Seems like you forgot to check if the email is in a right format.

function verifyEmail($email) {
	return ( ! preg_match("/^([a-z0-9\+_\-]+)(\.[a-z0-9\+_\-]+)*@([a-z0-9\-]+\.)+[a-z]{2,6}$/ix", $str)) ? FALSE : TRUE;
}

Call it when you need to verify it