[KingCuddles]

Hello DIC, I have been learning PHP for about a month, I have been following guides and reading books. I have now built my own login system with bits from various guides and books.

It is only basic at the minute - I am planning to add features such as last login time, last IP, browser.

But for now I would like you guys to look at my code and see if I have made any mistakes, or if there is anything you could improve, Security or otherwise.

<h3>Login</h3>

<form method="POST">
Username: <input type="text" name="username" /><br />
Password: <input type="password" name="password" /><br /><br />
<?php

//set vars
$dbhost = "host";
$dbuser = "username";
$dbpass = "password";
$dbtable = "table";
$username = addslashes(strip_tags(strtolower($_POST['username'])));
$password = addslashes(strip_tags($_POST['password']));;
$encpassword = sha1($password);

//if login button is pushed
if ($_POST['login'])
{
//if $username and $encpassword contain values continue
if ($username&&$password)
{
//connect to the database
$connect = @mysql_connect("$dbhost","$dbuser","$dbpass") or die("Error connecting to server, please try again later.");
@mysql_select_db("$dbtable") or die("Error connecting to server, please try again later.");

//find all users with the username matching $username_clean
$query = mysql_query("SELECT * FROM users WHERE username ='$username'");

//count the usernames matching $username
$numrows = mysql_num_rows($query);

//if any matches are found
if ($numrows!=0)
{
//put username and password from database into varables 
while ($row = mysql_fetch_assoc($query))
{
$dbusername = $row['username_clean'];
$dbpassword = $row['password'];
}

//if both $username and $password match the database values then login
if (($username==$dbusername)&&($encpassword==$dbpassword))
{
//set session variable
$_SESSION['username']=$username;
//redirect the user, and make sure script ends
header("Location: index.php");
exit();
}
//password dosn't match
else
{
echo "Incorrect login details, please try again.<br /><br />";
}

}
//if username isnt found
else
{
echo "Incorrect login details, please try again.<br /><br />";
}
//end ($username&&$password) if statement
}
//if $username and $encpassword does not contain values continue
else 
{
echo "Please fill in both fields.<br /><br />";
}
}

?>

<input type="submit" name="login" value="Login!" />

This post has been edited by KingCuddles: 03 January 2010 - 10:08 AM

[moopet]

It's basically ok. I'm not sure where you're going with mentioning variables called "username" and "username_clean"? Looks like you either changed your mind or you're keeping a second, clean version of the username in the database, which is weird. It should be clean when it goes into the db in the first place!

What I would suggest is not using PHP to encrypt your password. While that makes it nice and portable between databases, it's generally a lot faster to let the database handle it. MySQL handles it fine using its built-in PASSWORD() function:

$query = mysql_query("SELECT * FROM users WHERE username='{$username}'" AND password=PASSWORD('{$password}')");

Obviously when setting the password you have to use the same function


Note that I put my variables in braces in the double-quoted string? It's certainly not essential, but I find it helps a lot - if the characters following the variable name could constitute another part of it you'd have to break out of the quotes otherwise:

$thing = "there are several {$item}s here";
// is nicer to me than:
$thing = "there are several ".$item."s here";
// and of course this wouldn't work:
$thing = "there are several $items here"; // $items not found

[noorahmad]

I found a few bugs in your code,

• You didn't closed the form tag </form>
  
• You declared $username and $password before checking the if form is posted or not
  
• You didn't check if username or password is empty or not and just assigned values to $username and $password
  
• When you are using die() @ sign is not necessary and not affecting on your code, You used in your code
  
• don't user double quotes when you are passing only variable name, [i]You used in mysql_connect() and mysql_select_db()
  
• Don't use loop to check the username and password, try to check in compare username and password in sql example: SELECT * FROM `users` WHERE `username`='$username' AND `password`='$encpassword'
  
• use isset function to validate the fields

Here is your code clean from bugs:

<?php
//set vars
$dbhost = "host";
$dbuser = "username";
$dbpass = "password";
$dbtable = "table";
//if login button is pushed
if (isset($_POST['login'])){

	if(isset($_POST['username']) && !empty($_POST['username'])){
		$username = addslashes(strip_tags(strtolower($_POST['username'])));
	}else{
		$username = NULL;
	}
	
	if(isset($_POST['password']) && !empty($_POST['password'])){
		$password = addslashes(strip_tags($_POST['password']));
		$encpassword = sha1($password);
	}else{
		$password = NULL;
	}
	//if $username and $encpassword contain values continue
	if (!is_null($username) && !is_null($password)){
	//connect to the database
	$connect = mysql_connect($dbhost,$dbuser,$dbpass) or die("Error connecting to server, please try again later. ".mysql_error());
	mysql_select_db($dbtable) or die("Error connecting to server, please try again later. ".mysql_error());
	
	//find all users with the username matching $username_clean
	$query = mysql_query("SELECT * FROM users WHERE username ='$username' and `password`='$encpassword'") or die(mysql_error());
	
	//count the usernames matching $username
	$numrows = mysql_num_rows($query);
	
		//if any matches are found
		if ($numrows>0){
			//set session variable
			$_SESSION['username']=$username;
			//redirect the user, and make sure script ends
			header("Location: index.php");
			exit();
		}else{//password dosn't match
			echo "Incorrect login details, please try again.<br /><br />";
		}	
	}
	//if username isnt found
	else{
		echo "Incorrect login details, please try again.<br /><br />";
	}
	//end ($username&&$password) if statement

}else{//if $username and $encpassword does not contain values continue
	echo "Please fill in both fields.<br /><br />";
}
?>

Hope it helps

This post has been edited by noorahmad: 03 January 2010 - 11:03 AM