Registration Script

[DexterMorgan]

I think this is finished, could anyone have a look at any problems or anything that can be added etc. Thanks in advance.

<?php
	/*
		*	Register Process
		*	Version 0.4
		*	15 May 2010
		*
	*/
	session_start();
	session_regenerate_id();
	require('../includes/config.php');
	require('mysql.php');

function create_verification($username,$password, $email){
	/* Function to create verification email and add to the database. */

	/* Create a random string. */
	$key = ""; $i = 0;

	while ($i < 32) {
		$key .= chr(rand(97, 122));
		$i++;
	}
	/* $key = Random 32 character string. */

	/* Check if this username has already been used.*/	
	$query = sprintf('DELETE FROM pending_activations WHERE username = "%s"',$username);
	mysql_query($query);

	/* Insert into database. */
	$timestamp = time()+86400;
	$query = sprintf('INSERT INTO pending_activations (username,password,email,activationkey,timestamp) VALUES ("%s","%s","%s","%s","%u")',$username,$password,$email,$key,$timestamp);
	mysql_query($query);

	/* Email information */
	$link = 'http://www.upsidedowndog.co.cc/myclan/activate.php?key=';	// CHANGE WHEN MOVED		
	$subject = 'MyClan activation email.'; // CHANGE WHEN FOUND A NAME
	$body	 = 'Please visit this link to activate your account: '.$link.$key;
	if(mail($email,$subject,$body)){
		$_SESSION['notice'] = 'Thanks for registering, we have sent you an activation email. you have to activate your account before use. You have 24 hours to activate this account or it will be deleted. Thanks';
		header('location:../index.php');
	}else{
		$delete = sprintf('DELETE FROM pending_activations WHERE username="%s" LIMIT 1',$username);
		mysql_query($delete);
		$_SESSION['notice'] = 'Error 6: Activation email failed. Registration aborted.';
		header('location:../index.php');
	}		
}
function detail_available($table,$detail,$value){
	/* Function for checking dupes. */
	$query 	= "SELECT * FROM $table WHERE $detail =  '$value'";
	$result = mysql_query($query);
	$information = mysql_fetch_assoc($result);
	if(!$information){
		return true;
	}else{
		return false;
	}
}
if(($_POST['username']) &&($_POST['password']) && ($_POST['email']) ){

	foreach($_POST as $key => $value){ /*escaped all posts*/
		if($_POST[$key] != ""){//if POST contains nothing
			$_POST[$key] = mysql_real_escape_string($value);//escape strings
		}else{
			$_SESSION['notice'] = 'Error 1';
			header('location:../index.php');
		}
	}
	/* Secure input function is strip_tags and trim etc*/
	$username 	= secureinput($_POST['username']);
	$password 	= sha512($_POST['password']);
	$email		= secureinput($_POST['email']);

	if(detail_available('users','username',$username)){
		if(filter_var($email, FILTER_VALIDATE_EMAIL)){
			if(detail_available('users','email',$email)){//everything is fine.
				/* Check the pending table */
				if(detail_available('pending_activations','email',$email)){
					if(detail_available('pending_activations','username',$username)){
						/* Everything is fine. */
						create_verification($username,$password,$email);
					}else{
						$_SESSION['notice'] = 'Error 7: This username is awaiting activation.';
						header('location:../index.php');
					}
				}else{
					$_SESSION['notice'] = 'Error 6: This email address is awaiting activation.';
					header('location:../index.php');
				}					
			}else{
				$_SESSION['notice'] = 'Error 5: This email address has already been registered.';
				header('location:../index.php');
			}
		}else{
			$_SESSION['notice'] = 'Error 4: Email address is not valid';
			header('location:../index.php');
		}			
	}else{
		$_SESSION['notice'] = 'Error 3: Username is already taken.';
		header('location:../index.php');
	}		
	mysql_close($con);
}else{//not all details have been entered	
	$_SESSION['notice'] = 'Error 2: All fields are required.';
	header('location:../index.php');
}
?>

[gavshouse]

Alot of it looks okay, your detail_available() function isnt really a good way of checking this would be alot better

If you are sending static text to the function which will need editing and isnt dynamic then why send it to the function, just enter the text. This will also allow you to use sprinft(). Another thing you are trying to return the row..why all you need to do is check if there is a row. You also want to limit the query to 1 (LIMIT 1)

<?php 
	function detail_available($value){
        /* Function for checking dupes. */
        $query	= sprintf("SELECT * FROM users WHERE username='%s' LIMIT 1", mysql_real_escape_string($value));
        $result	= @mysql_query($query);//suppress mysql error

	if($result){//if query failed ?
		if(mysql_num_rows($result) == 0){//if number of rows is 0 
			return true;
		}else{//they have a DB row must already be taken
			return false;
		}
	}else{
		die('Database Error');//good for debugging
	}
}
?>

[DexterMorgan]

I use this function to check whether the email address is available and I use it on other tables.

Yeah good idea about checking the result instead of the row, I will change that.

[laserlight]

DexterMorgan wrote:

I think this is finished, could anyone have a look at any problems or anything that can be added etc.

It seems a little strange that in the code following the comment "Check if this username has already been used", you delete from the table instead of checking it. Normally, the way to handle the possibility of duplicate usernames is to declare the username column as unique. You then insert into the table, and check for a unique constraint violation.

Three comments on gavshouse's suggestion:

• Use the error suppression operator @ wisely. In this case, you want database query related errors to be logged in production, not suppressed, and you want them to be displayed in development. Therefore, turn off display_errors in production, but keep error_reporting say, E_ALL, and do not use @.

• The use of die is indeed good for debugging, but you should handle errors more gracefully in production.

• Instead of having an if statement for which one branch returns true and the other branch returns false, write: return mysql_num_rows($result) == 0;

[DexterMorgan]

laserlight;10953448 wrote:

You then insert into the table, and check for a unique constraint violation.

I have never done this before, I will look into it, thanks.

It was you who was explaining the whole salt on passwords thing, I have not yet implemented this but im planning on doing.

Thanks for the comments