PHP Login and registering system

Question

I am currently learning PHP and decided to write a login and registration system to learn about error handling, mysql, password hashing and basic queries.

I know there's standard authentication systems with frameworks such as Laravel, but I wanted to write one myself just for learning purposes.

I'd like to get some feedback, based on:

Security
Code efficiency
- Unneeded code
- Code that slows the script
- Wrong use of functions
- Bad indentation

Security is of course the main thing here, if you have any other feedback please post it as it is greatly appreciated!

SQL Injections
XSS
Session hijacking

My registering form

<?php

require('db.php');

if($_SERVER['REQUEST_METHOD'] == 'POST') {
  // Throw error if a field is not filled in
  if(empty($_POST['username']) || empty($_POST['password']) || empty($_POST['email'])) {
    $err = "Vul alle velden in";
    $class = "fail";
  } else {
    // If everything is filled in, prepare a statement
    $stmt = $connect->prepare('INSERT INTO `users` (user_name, user_password, user_email, user_perms, user_created_at) VALUES (?, ?, ?, ?, ?)');

    if($stmt) {
      // If statement gets sent correctly, create variables and add them
      $username = $_POST['username'];

      // Hash users' password
      $password = password_hash($_POST['password'], PASSWORD_DEFAULT);

      $email = $_POST['email'];

      // Timestamp
      $date = date('Y-m-d H:i:s');

      // Standard user permission level
      $perms = "Gebruiker";

      // Bind parameters
      $stmt->bind_param('sssss', $username, $password, $email, $perms, $date);

      // Check whether query ran succesfully or not
      if($stmt->execute()) {
        $err = "Het account is aangemaakt";
        $class = "succes";
      } else {
        $err = "Deze gebruikersnaam wordt al gebruikt";
        $class = "fail";
      }
    }
  }
}

?>

<!DOCTYPE html>
<html lang="en" dir="ltr" id="form-background">
    <head>
        <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta name="keywords" content="windows, windows server, linux, ubuntu, mint, install, guide, nnnext, click, tutorial, nextnextnext">
    <meta name="description" content="Leer veel voorkomende installaties makkelijk met nextnextnext.click!">

    <title>REGISTER</title>

    <link rel="stylesheet" href="./css/nnnext-main.css">
    <link href="https://fonts.googleapis.com/css?family=Lato" rel="stylesheet">
    <link rel="icon" href="./img/nnnext-favicon.ico">
    <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css">
        <link rel="stylesheet" href="./css/nnnext-backend.css">

    <link rel="stylesheet" type="text/css" href="./slick/slick.css"/>
    <link rel="stylesheet" type="text/css" href="./slick/slick-theme.css"/>
    </head>
    <body>
        <div class="admin-container form-background">
            <div class="centered-form">
                <form class="login-form" method="post">
                    <h3>REGISTER</h3>
                    <?php
                        if($_SERVER['REQUEST_METHOD'] == 'POST') {
                            echo "<div class='error-handling " . $class . "'" . ">";
                            echo "<p>" . $err . "</p>";
                            echo "</div>";
                        }
                    ?>
                    <input type="text" name="username" placeholder="Gebruikersnaam">
          <input type="email" name="email" placeholder="Email adres">
                    <input type="password" name="password" placeholder="Wachtwoord">
                    <input type="submit" name="submit" value="Account aanmaken">
          <p><a href="login.php">Ik heb al een account</a></p>
                </form>
            </div>
        </div>
    </body>
</html>

My login form

<?php
require('db.php');
// If the server receives a POST request
if($_SERVER['REQUEST_METHOD'] == 'POST') {
    if(empty($_POST['username']) || empty($_POST['password'])) {
        $err = "Vul beide velden in";
        $class = "fail";
    } else {
    // If everything has been filled in, send the query to the server
        $stmt = $connect->prepare('SELECT user_name, user_password, user_email, user_perms FROM `users` WHERE user_name = ?');

        if($stmt) {
            $username = $_POST['username'];
            $password = $_POST['password'];

      // Send parameters to the server
            $stmt->bind_param('s', $username);

      // Run statement
            $stmt->execute();

            // Get query result
            $result = $stmt->get_result();

            // Fetch the query result in a row and bind to variables
            while($row = $result->fetch_assoc()) {
                $hash = $row['user_password'];
                $username = $row['user_name'];
                $email = $row['user_email'];
                $userPerms = $row['user_perms'];
            }
            // If there's no hash, the user does not exists
            if(empty($hash)) {
                $err = "Sorry, maar deze gebruik bestaat niet";
                $class = "fail";
            } else {
                // Check whether password matches hash on server
                if(password_verify($password, $hash)) {
                    // Hash matches password
                    session_start();

                    // Bind session variables
                    $_SESSION['username'] = $username;
                    $_SESSION['email'] = $email;

                    // Login succesful, set message and class accordingly
                    $err = "Succesvol ingelogt. Redirecting...";
                    $class = "succes";

                    // Redirect to secured page
                  header("Location: admin.php");
                } else {
                    // Password doesn't match hash, set error message accordingly
                    $err = "Wachtwoord is incorrect";
                    $class = "fail";
                }
            }
        }
    }
}

?>

<!DOCTYPE html>
<html lang="en" dir="ltr" id="form-background">
    <head>
        <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta name="keywords" content="windows, windows server, linux, ubuntu, mint, install, guide, nnnext, click, tutorial, nextnextnext">
    <meta name="description" content="Leer veel voorkomende installaties makkelijk met nextnextnext.click!">

    <title>NNNEXT - INLOGGEN</title>

    <link rel="stylesheet" href="./css/nnnext-main.css">
    <link href="https://fonts.googleapis.com/css?family=Lato" rel="stylesheet">
    <link rel="icon" href="./img/nnnext-favicon.ico">
    <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css">
        <link rel="stylesheet" href="./css/nnnext-backend.css">

    <link rel="stylesheet" type="text/css" href="./slick/slick.css"/>
    <link rel="stylesheet" type="text/css" href="./slick/slick-theme.css"/>
    </head>
    <body>
        <div class="admin-container form-background">
            <div class="centered-form">
                <form class="login-form" method="post">
                    <form class="login-form" method="post">
                        <h3>Inloggen op nnnext.click</h3>
                        <?php
                            if($_SERVER['REQUEST_METHOD'] == 'POST') {
                                echo "<div class='error-handling " . $class . "'" . ">";
                                echo "<p>" . $err . "</p>";
                                echo "</div>";
                            }
                        ?>
                        <input type="text" name="username" placeholder="Gebruikersnaam">
                        <input type="password" name="password" placeholder="Wachtwoord">
                        <input type="submit" name="submit" value="Inloggen">
                        <p><a href="register.php">Ik heb nog geen account</a></p>
                    </form>
            </div>
        </div>
    </body>
</html>

My authentication file

<?php

// Check whether session started or not
if(!isset($_SESSION)) {
  session_start();
}

// Check whether user is logged in or not
if(!isset($_SESSION['username'])) {
  header("Location: login.php");
  exit();
}

?>

My logout file

<?php
// Check whether session started or not
if(!isset($_SESSION)) {
  session_start();
}

// Destroy session and redirect to login page
if(session_destroy()) {
  header("Location: login.php");
}

?>

"Security is of course the main thing here" What's your threat model? What does it have to be secure against? — Mast, May 12, 2018 at 19:51
@Mast In the first place SQL injections. After that session hijacking and XSS. I've been able to protect against SQL injections pretty well, using parameterized queries and bind_param. But I don't know as much about XSS and session hijacking. — rpm192, May 12, 2018 at 20:03
I think they mean such as are you coding for a small blog website without any products for sale or a bank website and account holder transactions — James, May 12, 2018 at 20:34
@James There's no transactions of any sort involved, it's not a webshop. It's just a simple webapplication where users can upload images and create posts with those images. The file uploading part has been secured properly but now I'd just like some feedback for the login part. — rpm192, May 12, 2018 at 20:39
I wrote similar(1) reviews(2) recently(3). Although not 100% applicable to your case but you can get some useful tips — Your Common Sense, May 13, 2018 at 9:21

James · Accepted Answer · 2018-05-12 22:51:55Z

I won't go into security (others would be better at that) but some general problems, or niggles, I can see.

Simple and clean code makes many things easier. Refactoring, bug finding, maintenance, etc.

For example, it's hard to read when you check something is true and then a huge load of code, with then an else at the end. Try really hard to keep if/elses short.

e.g. your if ($stmt), instead check if there is a problem and return early:

if (!$stmt) {
    $error = [
        'error' => 'We are having problems retrieving results. etc'
    ];
    // Then call or return to your view and tell them about the error
}

// No need for an else, just do the code
// as the above would have returned if there was a problem

That said, the above should be decoupled from a file claiming to be a login form. That is not a login form, that is a database query and a check to see if the query worked.

If you decouple your files into separate parts you can re-use things. For example, how many times in your different files do you connect to a database, check the connection was ok, then do something similar?
This can be centralised in one place and each place which needs to use it can call the one place where it's set.
https://en.wikipedia.org/wiki/Single_responsibility_principle
https://en.wikipedia.org/wiki/Separation_of_concerns

For example:
Your "login form" file should be a form file which is passed data from a controller. Not a DB call and error checking and data retrieval.

In your login and registration, you include the DB file and check some post data, and set an error and class if there's a problem. They both do the same. An example to get you thinking of how you can start to decouple your code to make your code easier to manage:

file formInputErrors.php (or whatever name suits):

function formInputErrors(array $formInput = [])
{
    foreach ($formInput as $inputValue) {

        if (empty($inputValue)) {
            return [
                'error' => 'Vul alle velden in',
                'class' => 'fail',
            ];
        }
    }

    return null;
}

Then in your login and registration file, you just do this:

include 'formInputErrors.php';

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    $formErrors = formInputErrors($_POST);
    if ($formErrors !== null) {
        // Call or return to your view and tell them about the error
    }
}

The checking is centralised and can be reused anywhere and will be the same (no tiresome copy and paste or paste fail and not the same checks etc). If you ever want to change it, such as perhaps all fields need to not be numeric, you change it in one place.

On that, you should be checking more than empty. which is where a class comes in with different methods (one for email check, one for name, etc).
But you can just put more functions in that separate file and call whichever one you need. such as function checkEmailAddress() etc.

In your "login form" code you have:

  $password = password_hash($_POST['password'], PASSWORD_DEFAULT);

You have literally defined how you are hashing passwords in a script that is supposedly for "login form". If you need to hash a password somewhere else, you'll have to copy it there, and then you have it defined in two places.

If you ever need to change how you hash passwords (unlikely but rule applies across the board however unlikely) then you need to seek out all the places you did the above and change it. Hoping you got them all and don't leave a bug or worse, a little security hole...

So another decouple example from the above:

In a separate file:

function hashPassword($passwordToHash)
{
    return password_hash($passwordToHash, PASSWORD_DEFAULT);
}

Then anywhere you need to know a password's hash value:

$password = hashPassword($_POST['password'];

if ($password === false) {
    return [
        'error' => 'Something went wrong in our system, try again, contact if persist',
    ];
    // Call or return to your view and tell them about the error
}

You use prepared statements, which is great, but it's not a magic wand. You don't check if the data the users enter is "valid". I can pass "meh" for an email address etc.

What about a username with non standard characters? What about duplicate usernames? If the field is unique, then you need to check it's not already used, rather than letting MySQL error out because of a duplicate insert error.

If your username field is varchar(25) and I send 30 chars, my username may be stored BUT will be truncated, meaning you let me register with my desired username with 5 chars cut off. Maybe I don't notice this and then struggle to login in next time.

How you do the username is down to what your system allows. Email is relatively simple:

In your function file for validation (or wherever):

function validateEmail($emailToValidate)
{
    return filter_var($emailToValidate, FILTER_VALIDATE_EMAIL);
}

In your (eg) registering file:

if (validateEmail($_POST['email']) === false) {
    // Return/call the view and pass in the error
}

Alright, I will definitely look into using functions, as they make things a lot less messy. I will also add the form validation. Can I put all of my functions into one file named functions.php for example, or is there a better way? — rpm192, May 12, 2018 at 23:06
You can, and at least the functionality itself is decoupled into separate functions which can be reused. But it's not hard to have a directory named eg "utilities" and have separate function files in there. That way you include what you need when you need it, rather than just allthethings in allthefiles :) but it's a start and if you feel more comfortable doing that you'll get to learn SRP/SoC etc at a better pace. — James, May 12, 2018 at 23:08
Might as well do it the right way the first time. Thanks for your efforts :) — rpm192, May 12, 2018 at 23:12
If I am allowed a little review for a review. if (!$stmt) { $error = ... is quite inefficient way to handle errors. Given almost every line of code may return a wrong value, it will make your code bloated with monotonous error handling snippets. Better to make your PHP report errors by itself and handle them in a single place uniformly. — Your Common Sense, May 13, 2018 at 9:06
@YourCommonSense I don't disagree, which is why I explained "he above should be decoupled from a file claiming to be a login form". Given the code they have and thus stage of learning they're at, it was more about showing the idea of writing it cleaner (rather than huge if/elses), and how to separate code out. Then the next stage would be classes with DI, factories/services and whatever ;) — James, May 13, 2018 at 12:52

Stack Exchange Network

PHP Login and registering system

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged
php
mysql
authentication
or ask your own question.

Linked

Hot Network Questions

PHP Login and registering system

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged phpmysqlauthentication or ask your own question.

Linked

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
php
mysql
authentication
or ask your own question.