[SOLVED] Redundant login check?

[soycharliente]

Is this double check necessary or is it just redundant? I've been using this code for ages now and never really noticed it.

 

<?php
if ($_POST["Submit"] == "Login")
{
foreach ($_POST as $key => $val) { $_POST[$key] = myEscape($val); }
$un = $_POST['Username'];
$pw = md5($_POST['Password']);
dbconnect();
$sql = "SELECT * FROM `users` WHERE `username`='{$un}' AND `password`='{$pw}'";
$result = mysql_query($sql) OR DIE ("Unable to validate login.");
if (mysql_num_rows($result) > 0)
{
	$r = mysql_fetch_assoc($result);
	$user = $r['username'];
	$pass = $r['password'];
	if ($un == $user && $pw == $pass) //THIS CHECK RIGHT HERE
	{
		// do stuff
	}
}
dbclose();
}
?>

[PFMaBiSmAd]

It's not necessary (edit: assuming that you have prevented sql injection, depending on what myEscape() is.) Only rows that match the query are returned, so the values in $r will match what the query asked for in $un and $pw.

[soycharliente]

<?php
function myEscape($string)
{
dbconnect();
$new = get_magic_quotes_gpc() ? stripslashes($string) : $string;
$safe = mysql_real_escape_string($new);
dbclose();
return $safe;
}
?>

 

So I could just have this and it would work jsut the same?

<?php
if ($_POST["Submit"] == "Login")
{
   foreach ($_POST as $key => $val) { $_POST[$key] = myEscape($val); }
   $un = $_POST['Username'];
   $pw = md5($_POST['Password']);
   dbconnect();
   $sql = "SELECT * FROM `users` WHERE `username`='{$un}' AND `password`='{$pw}'";
   $result = mysql_query($sql) OR DIE ("Unable to validate login.");
   if (mysql_num_rows($result) > 0)
   {
      // do stuff
   }
   dbclose();
}
?>

 

[PFMaBiSmAd]

That will work.

 

I recommend not opening and closing the database connection for each operation. Your code will run significantly faster if you just open a connection at the start of the script and close it at the end or let it be closed automatically when the script ends.

 

If you have 5 $_POST variables, the posted code is connecting - dbconnect(); and disconnecting - dbclose(); 6 times.

[soycharliente]

I sometimes run myEscape outside the dbconnect. I don't remember why. I had to do it that way because I had to be connected to run the function. I guess I should just move it inside  the dbconnect() and use it that way from now on.

 

That makes a lot of sense. Thanks.

[PFMaBiSmAd]

mysql_real_escape_string() should only be used with mysql queries, so calls to myEscape() should only be done when you are going to be doing a query and you have already established a connection to the database server.

[soycharliente]

So instead of doing a forloop and wrapping the post array with that function, I should just wrap my $sql variable with it?