1

As a part of my training, my team leader asked me to make a simple login in PHP, I have created a controller that receives the username and the password from a JavaScript file, and then triggers an SQL query using parameters, that's where my first problem starts. I execute the statement but then I can't manage to store the query dataset in my $stored variable. I'm using store_result(), and neither have any rows being returned ($queryRows variable).

What am I doing wrong?

<?php
include('database.php');
session_start();

//Fetch the user data
$user = $_POST['user'];
$password = $_POST['pass'];
$_SESSION['user'] = $user;


//Checks for empty values 
if(empty($user)||empty($password)){
    header("Location: ../views/login.php?error=emptyfields");
    exit();
}

//DB query to validate the user/password combination
$validationQuery = "SELECT userName, password FROM users WHERE userName =? AND password =?";

//Prevents SQLInjection by using parameters

$statement = $connection->prepare($validationQuery);
$statement->bind_param("ss",$user,$password);
$executed = $statement->execute();
$stored = $statement->store_result($statement);
$queryRows = $statement->num_rows($statement);


if ($stored) {
    header("Location: ../views/home.php");
    exit();
} else {
    header("Location: ../views/login.php?error=userError");
    exit();
}
mysqli_stmt_close($statement);
mysqli_close($connection);
Dharman
  • 30,962
  • 25
  • 85
  • 135
jak2wook
  • 13
  • 4
  • `from a js file`...you mean via Ajax? If the request is coming via Ajax then redirecting won't work - Ajax clients don't honour redirect headers sent by the server, because, after all, the entire point of using Ajax is to allow the browser to stay on the same page afterwards. If you want to use redirects then don't use Ajax. – ADyson Jun 27 '21 at 23:39
  • P.s. why on earth are you putting the user value into the session before you've validated them? I don't know how you're check the user's logged in status on other pages but it very much looks like you're only setting that one session value, so presumably you intended to use that to determine the status on pages which require login to access them. But you're setting it whether or not the user exists, so it won't provide any security. – ADyson Jun 27 '21 at 23:43
  • 2
    Also you should really be hashing passwords, not storing them in plain text – ADyson Jun 27 '21 at 23:43
  • As for your database problem...have you got PHP errors and warnings switched on? Habe you got mysqli's specific error reporting switched on? And what values are you sending for username and password? Which row in your database are you expecting that to match? We can't see any of your data so we can't be certain whether your query ought to return any rows or not – ADyson Jun 27 '21 at 23:44
  • Thank you for your advice, i'll be taking care of those issues you mentioned, also you're right, Ï'm using Ajax , so that explains the redirect problem. Thank you so much! – jak2wook Jun 27 '21 at 23:46
  • 1
    **Never store passwords in clear text or using MD5/SHA1!** Only store password hashes created using PHP's [`password_hash()`](https://php.net/manual/en/function.password-hash.php), which you can then verify using [`password_verify()`](https://php.net/manual/en/function.password-verify.php). Take a look at this post: [How to use password_hash](https://stackoverflow.com/q/30279321/1839439) and learn more about [bcrypt & password hashing in PHP](https://stackoverflow.com/a/6337021/1839439) – Dharman Jun 28 '21 at 10:04
  • I don't know your company but as a general rule you should avoid mysqli. You are clearly confused by it. Use PDO instead as it is much easier – Dharman Jun 28 '21 at 10:07
  • `include` is not a function, it doesn't need parentheses – Dharman Jun 28 '21 at 10:08
  • 1
    What **exactly** is not working with the given code? What have you tried to make it work? Is this a problem of gathering the data, or of senidng the response? Have you checked whether executing the query throws any kind of error message? – Nico Haase Jun 28 '21 at 10:08

1 Answers1

1

You are doing really well by using prepared statements. However, mysqli is a very complicated extension. I highly recommend looking up each function in the documentation carefully before using it.

In your case, your code looks like it makes sense, but when you look closer it's pure nonsense.

The first 3 lines work fine. It prepares and executes a statement on the MySQL server.

$statement = $connection->prepare($validationQuery);
$statement->bind_param("ss",$user,$password);
$executed = $statement->execute();

mysqli_stmt::store_result() is used to store the data in the internal buffer. It returns true when the data was buffered properly. It doesn't take any arguments!

$stored = $statement->store_result($statement);

mysqli_stmt::num_rows() returns the number of rows buffered. It doesn't take any arguments!

$queryRows = $statement->num_rows($statement);

After this code will be executed, $stored will always be true. Even if the result set has 0 rows. This means that the next statement will be executed.

if ($stored) {
    header("Location: ../views/home.php");
    exit();
}

Here, your script ends. You redirect to another page and exit the current one. The internal buffer is cleared, the statement is cleaned up, and the connection is closed.

The following two lines are no-op code:

mysqli_stmt_close($statement);
mysqli_close($connection);

I am not sure what exactly your intentions were, but given that this is a login script, I would say that you want to retrieve the data from the statement, not the number of rows. I would remove store_result() and use get_result() instead.

$statement = $connection->prepare($validationQuery);
$statement->bind_param("ss",$user,$password);
$executed = $statement->execute();

$result = $statement->get_result();
$userRecord = $result->fetch_assoc();
if($userRecord && password_verify($password, $userRecord['hash']) {
    $_SESSION['userId'] = $userRecord['id'];

    header("Location: ../views/home.php");
    exit();
} else {
    header("Location: ../views/login.php?error=userError");
    exit();
}
// NO MORE CODE SHOULD GO HERE. NO PHP, no HTML, nothing. This is the end of the script!

My example of course uses password hashing like every login script should. You need to adjust your SQL statement to account for that.

Dharman
  • 30,962
  • 25
  • 85
  • 135