# PHP - First Login System

I am quite new to PHP, and with the help of a few tutorials have made this login system.

However I know its not as efficient and compact as it could be, also I am looking for ways to improve the security of this system. As I noticed when testing that If I simply changed the url from the login page, I could bypass the whole login process without having to login. I'm not really sure how I would go about fixing this, I tried creating session variables at the bottom of the script but not really sure if this is the way to go about it.

I would be very grateful for any feedback and if someone could help me improve this code, to make it more secure and maybe more efficient?

<?php

session_start();

if (isset($_POST['submit'])) { require_once 'config.php';$username = mysqli_real_escape_string($connect,$_POST['username']);
$password = mysqli_real_escape_string($connect, $_POST['password']); //Error handlers //Check if inputs are empty if (empty($username) || empty($password)) { header("Location: ../index.php?login=empty"); exit(); } else {$sql = "SELECT * FROM users WHERE username='$username'";$search = mysqli_query($connect,$sql);
$validate = mysqli_num_rows($search);
if ($validate < 1) { header("Location: ../index.php?login=error"); exit(); } else { if ($row = mysqli_fetch_assoc($search)) { //De-hashing the password$hashcheck = password_verify($password,$row['password']);
if ($hashcheck == false) { header("Location: ../index.php?login=error"); exit(); } elseif ($hashcheck == true) {
$_SESSION['u_id'] =$row['userid'];
$_SESSION['u_first'] =$row['firstname'];
$_SESSION['u_last'] =$row['lastname'];
$_SESSION['u_email'] =$row['email'];
$_SESSION['u_uid'] =$row['username'];
exit();
}
}
}
}
} else {
exit();
}
?>


Security

You are escaping user input, which prevents SQL injection. It's good enough, but really not the recommended way to do this as it's too error-prone (whenever a project does this, it's basically guaranteed that you can find injections). Use prepared statements instead. It's not only more secure, but also leads to nicer code.

Guard Clauses

You have a lot of nested ifs, which makes it difficult to see what happens when (for example, I have to scroll all the way down to see what happens if it's a GET request).

If you use guard clauses / return early instead, your code will be much more readable. It might look like this:

if (!isset($_POST['submit'])) { header("Location: ../index.php?login=error"); exit(); } if (empty($username) || empty($password)) { header("Location: ../index.php?login=empty"); exit(); } // select the user if (!$userExists) {
if ($validPassword) { header("Location: ../index.php?login=error"); exit(); } // set the session, redirect  Now it's very clear what happens when. Duplication and functions Following a header redirect with exit is very good practice. You can make sure that you always do this, simplify the code, and make it easier to modify later by extracting that to it's own function (eg redirectError(Error error)). True or false Something is either true or false. There is no reason to do an else check on true if you already did one on false. Note though that some equals checks can be unexpected, so you should always use strong comparison (===) if you don't have a reason not to. • Thank you for the feedback, it really helps! just a few questions with the guard clauses, with the$userexists and \$validpassword can I just re-arrange my code to create those two variables, or would I have to create seperate code for those two variables if that makes any sense? – TessRay97 Feb 9 '18 at 20:49