Need Opinion!

[Trium918]

How can "I" improve my login script below.

 

<?php
session_start();

#Include function files for this application
require_once("superiun_container_fns.php");

// connect to db
$conn = db_connect();
if (!$conn)
    return "Could not connect to database server - please try later.";

if(isset($_POST['sublogin'])){
$user_name = trim($_POST['user_name']);
$password = trim($_POST['password']);

$query = "SELECT * FROM members_info WHERE  user_name='$user_name' 
AND password=MD5('$password') LIMIT 1";
  
if ($result = mysql_query($query)) {
  if (mysql_num_rows($result) > 0) {
    while ($row = mysql_fetch_assoc($result)) {  
  
$_SESSION['membersid'] = $row['members_id'];
$_SESSION['last']= $row['last_visit'];
    $_SESSION['valid_user']= $row['user_name'];; 

/*if (isset($_SESSION['membersid'])) {
      echo "Session is set";
  exit;
    }else{ echo "No";}*/
  
#Display date as 5/8/2007 format
    $_SESSION['last']= date('n/d/Y', strtotime($_SESSION['last']));  
      
  $sql="UPDATE members_info SET last_visit=NOW() 
          WHERE members_id='{$_SESSION['membersid']}'";   //now only update it
      $result2=mysql_query($sql); 
    }
  } 
  else {
     // unsuccessful login
   
      echo "<p class='genmed'>You could not be logged in. 
            You must be logged in to view this page.</p>";
  echo "<p class='genmed'> $user_name $password</p>";
   header("Location: login :-\.php");
  }
  header("Location: member.php"); 
} 
else {
    echo "Query failed<br />$query<br />" . mysql_error();
}// End if() statement

}
?>

[pocobueno1388]

Why do you have the while loop? You don't need one, as your query should only return ONE result.

[Trium918]

Why do you have the while loop? You don't need one, as your query should only return ONE result.

 

That's a start. Thanks!

 

I actually have a two different scrips. One with and the other with out

the while loop. This is way I needed you all opinion. Thanks again!

[trq]

For starters you need to escape the data coming through the form. The way you've got it setup now it would be quit easy to apply sql injection. Look into using mysql_real_escape_string.

 

Secondly, you don't need the while loop, your limiting your query to one record.

 

And thirdly, remove this....

 

echo "<p class='genmed'>You could not be logged in. 
            You must be logged in to view this page.</p>";
  echo "<p class='genmed'> $user_name $password</p>";

 

It serves no purpose as you are redirecting straight after it. In fact, it should generate a header error.

[Trium918]

What about placing all of the sessions in some type of

if() statement? Do you think I need one are is it fine the

way it is?

[trq]

Why would you need to place the sessions in some type of if() statment?

[Trium918]

Why would you need to place the sessions in some type of if() statment?

 

As you can see, I need your opinion. I am trying to follow you

lead by wrapping what ever can be wrapped in an if() statement.

[trq]

If I have said that anywhere I would have been refering to function calls. Basicaly, functions (usually) return false on failure, so its a good idea to check they succeed before attmpting to use any results they produce. eg;

 

Instead of simply calling....

 

<?php

  $result = mysql_query($sql);

?>

 

then simply going about using $result blindly, check your query succeeded firstly. eg;

 

<?php

  if ($result = mysql_query($sql)) {
    // $result holds a valid resource.
  }

?>

[per1os]

<?php
session_start();

#Include function files for this application
require_once("superiun_container_fns.php");

// connect to db
$conn = db_connect();
if (!$conn)
    return "Could not connect to database server - please try later.";

if(isset($_POST['sublogin'])){
$user_name = get_magic_quotes_gpc()?mysql_real_escape_string(trim(stripslashes($_POST['user_name']))):mysql_real_escape_string(trim($_POST['user_name'])); // escape string properly
$password = md5(trim($_POST['password']));

$query = "SELECT * FROM members_info WHERE  user_name='$user_name' LIMIT 1";
$result = mysql_query($query);
if (mysql_error()) {
	die("Query failed<br />$query<br />" . mysql_error());
}

if (mysql_num_rows($result) == 1) {
    $row = mysql_fetch_array($result);
    
    	if ($row['password'] == $password) {  
	$_SESSION['membersid'] = $row['members_id'];
	$_SESSION['last']= $row['last_visit'];
	$_SESSION['valid_user']= $row['user_name'];; 
	$_SESSION['last']= date('n/d/Y', strtotime($_SESSION['last']));  

	$sql="UPDATE members_info SET last_visit=NOW() WHERE members_id='{$_SESSION['membersid']}'";   //now only update it
	mysql_query($sql); // no need to assign a variable for an update statement.
	header("Location: member.php"); 
    }else {
	// invalid password
	// unsuccessful login   
	echo "<p class='genmed'>You could not be logged in. 
            You must be logged in to view this page.</p>";
	echo "<p class='genmed'> $user_name $password</p>";
	header("Location: login :-\.php");
    }
}else {
	// unable to find user
	// unsuccessful login   
	echo "<p class='genmed'>You could not be logged in. 
            You must be logged in to view this page.</p>";
	echo "<p class='genmed'> $user_name $password</p>";
	header("Location: login :-\.php");
}
}
?>

[Trium918]

I undestand. When you refere to the while loop as limiting the

query to one record. Do you mean that only one user will be able

to login at a time?  

[per1os]

I undestand. When you refere to the while loop as limiting the

query to one record. Do you mean that only one user will be able

to login at a time? 

 

No, You have a limit on the query which only returns 1 record. Therefore there is no reason to set a while loop. Remember each page call is unique to a user. So since you are only expecting 1 record you can just call mysql_fetch_assoc($result) once and that is it. The while loop would be for if you expect more than one record returned. In this case we are not, we only want 1 record.