views:

49

answers:

2

Hey, For some reason $password (the one being sent form the form) is coming up blank.

I am manually checking for this with this code:

echo "<p>$username does not match $firstandlast</p>";
echo "<p>$password does not match $dbpassword</p>";

here is my code...

            <?php
 if(isset ($_POST['submit'])) {
  $username = $_POST['name'];
  $password = $_POST['pw'];

  if ($username&&$password){

   require('includes/cxn.php');
   $login_query = "SELECT * FROM users WHERE username='$username'";
   $result = mysqli_query($cxn,$login_query) or die("Couldn't execute query.");
   $numrows = mysqli_num_rows($result);
   if($numrows!=0){

    while ($row = mysqli_fetch_assoc($result)) 
    {
     $firstandlast = $row['firstName'] . " " . $row['lastName']; 
     $dbpassword = $row['password'];
     $userID = $row['userid'];

     if ($username==$firstandlast&&$password==$dbpassword){
      echo "You are now logged in! <a href='profile.php?userID=" . $userID . "'>Click Here</a> to continue.";
      $_SESSION['username'] = $dbusername;
      $_SESSION['id'] = $userID;
     } 
     else {
      echo "<p>$username does not match $firstandlast</p>";
         echo "<p>$password does not match $dbpassword</p>";
         echo "Password does not exist.";
     }
    }
   } else {
    echo "This user doesn't exist.";
   }
  } else {
   die("You Must Enter both a username and password");
  }
} else {
 echo "
 <div id='login'>
 <form action='login.php' method='post' id='loginform'>
 <table>
   <tr>
  <td width='90'>Name:</td>
  <td width='825'><input type='text' name='name' /></td>
   </tr>
   <tr>
  <td>Password:</td>
  <td><input type='password' name='pw' /></td>
   </tr>
   <tr>
  <td>&nbsp;</td>
  <td><button type='submit' name='submit'>Sumbit</button></td>
   </tr>
 </table></form></div>";
}

?>
A: 

Looking at your code, I think there may be a problem with your formatting and syntax. Learn to use spaces in your code. When you write something like: $username&&$password PHP may be using &$password as a reference to the variable instead of $password the variable. So go through your code and add spaces:

if ($username && $password){

if ($username == $firstandlast && $password == $dbpassword){

This will probably fix your problem.

cdburgess
+2  A: 

Some things to consider:

1:

if(isset ($_POST['submit'])) {

This only works if your submit button in the form has name="submit" input element, which may not always be the case. You might forget to add it to the submit button, for one. A better check is:

if ($_SERVER['REQUEST_METHOD'] == 'POST') {

This is always true if the form was POSTed, regardless of what fields were submitted or buttons clicked.

2:

if ($username&&$password){

could be better rewritten as:

if(!empty($username) && !empty($password)) {

Best to explicitly state that you want them to be non-empty, which makes for easier comprehension by others later on.

3:

$login_query = "SELECT * FROM users WHERE username='$username'";

This is just begging for some SQL injection goodness to subvert your system. There's lots of resources out there on what SQL injection is, and how to get around it, so I won't rehash it here. Please fix before your roll this out (and think of what happens if someone tries to log in with ' OR 1=1 as their username.

4:

while ($row = mysqli_fetch_assoc($result)) 

Assuming your table is built correctly, there's only two possible results from the query. Nothing, because the username doesn't exist; or ONE row, representing that user's data. There's no need to do a loop to fetch all matching usernames. Of course, perhaps you allow multiple "John Smith" accounts, and identify between them by their password, but that's rather insecure. What if John Smith #1's password is very similar to John Smith #2's password, and one of the two "guesses" the other's by making a typo? Now you've got one person logged into another's account.

5:

Instead of doing a die() if anything's mis-matched, it's considered polite practice to re-display the form that was submitted so the user can try again. Your system requires the user to hit the back back to get back to the login form before they can try again. You should restructure the code so that if there is an error, that error gets displayed, ALONG with the login form.

6:

echo "
<div id='login'>
....
</table></form></div>";

Generating a long text string like this is painful, especially if you have to use quotes within. Use a HEREDOC instead, which is specifically intended for building multi-line strings. As a bonus, it acts like a double-quoted string, so you can interpolate variables into the string as it's being built:

echo <<<EOL
<div id="login">
....
</table></form></div>
EOF

7:

Instead of displaying "You are now logged in", with the link to the profile page, why not just redirect the user to the profile page automatically?

So, after all that, your script would look something like this:

<?php

$error = NULL;

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    $username = $_POST['name'];
    $password = $_POST['pw'];

    if (!empty($username) && !empty($password) {
        $qusername = mysql_real_escape_string($username);
        $qpassword = mysql_real_escape_string($password);

        $query = <<<EOL
SELECT *
FROM users
WHERE (username = $qusername) AND (password = $qpassword)
EOL;
        $stmt = mysql_query($query);
        if (mysql_num_rows($stmt) == 1) {
             // store user details in session here
             redirect("profile.php?userID=$id");
        } else {
             $error = "Invalid username or password";
        }
    } else {
        $error = "Must specify both username and password";
    }
}

?>
<html>

<body>

<?php if (!empty($error)) { ?>
<h3><?php echo $error ?></h3>
<? } ?>

<form ...>
<table ...>
<tr>
   <td><input type="text" name="name" value="<?php echo htmlspecialchars($name) ?>" /></td>
</tr>
<tr>
   <td><input type="password" name="pw" /></td>
</tr>
<table>
<input type="submit" value="Login" />

</form>
Marc B