PHP/MySQLi code for registration/login form

Question

The following code works, I just want to know if there are any suggestions as to how I can make it better or more secure.

config.php:

<?php
define('DB_HOST', 'localhost');
define('DB_USER', 'root');
define('DB_PASS', '');
define('DB_NAME', 'database');
?>

connect.php:

<?php
class DB_Connect {
    private $con;

    public function connect() {
        require_once dirname(__FILE__) . '/config.php';

        $this->con = new mysqli(DB_HOST, DB_USER, DB_PASS, DB_NAME);

        return $this->con;
    }
}
?>

functions.php:

<?php

class DB_Functions {

    private $con;

    function __construct() {
        require_once dirname(__FILE__) . '/../database/connect.php';

        $db = new DB_Connect();
        $this->con = $db->connect();
    }

    function __destruct() {

    }

    public function storeUser($username, $email, $password) {
        $uuid = uniqid('', true);
        $email_code = md5($_POST['username'] + microtime());
        $hash = $this->hashSSHA($password);
        $encrypted_password = $hash['encrypted'];
        $salt = $hash['salt'];

        $stmt = $this->con->prepare("INSERT INTO users(unique_id, username, email, email_code, encrypted_password, salt, created_at) VALUES(?, ?, ?, ?, ?, ?, NOW())");

        $stmt->bind_param('ssssss', $uuid, $username, $email, $email_code, $encrypted_password, $salt);

        $result = $stmt->execute();

        $stmt->close();

        if ($result) {
            $stmt = $this->con->prepare("SELECT * FROM users WHERE email = ?");
            $stmt->bind_param('s', $email);
            $stmt->execute();
            $user = $stmt->get_result()->fetch_assoc();
            $stmt->close();

            return $user;
        } else {
            return false;
        }
    }

    public function getUserByUsernameAndPassword($username, $password) {
        $stmt = $this->con->prepare("SELECT * FROM users WHERE username = ?");

        $stmt->bind_param('s', $username);

        if ($stmt->execute()) {
            $user = $stmt->get_result()->fetch_assoc();
            $stmt->close();

            $salt = $user['salt'];
            $encrypted_password = $user['encrypted_password'];
            $hash = $this->checkhashSSHA($salt, $password);

            if ($encrypted_password == $hash) {
                return $user;
            }
        } else {
            return NULL;
        }
    }

    public function doesUserEmailExist($email) {
        $stmt = $this->con->prepare("SELECT email FROM users WHERE email = ?");

        $stmt->bind_param('s', $email);

        $stmt->execute();

        $stmt->store_result();

        if ($stmt->num_rows > 0) {
            $stmt->close();
            return true;
        } else {
            $stmt->close();
            return false;
        }
    }

    public function doesUsernameExist($username) {
        $stmt = $this->con->prepare("SELECT username FROM users WHERE username = ?");

        $stmt->bind_param('s', $username);

        $stmt->execute();

        $stmt->store_result();

        if ($stmt->num_rows > 0) {
            $stmt->close();
            return true;
        } else {
            $stmt->close();
            return false;
        }
    }

    public function isActive($username) {
        $stmt = $this->con->prepare("SELECT COUNT user_id FROM users WHERE username = ? AND active = 1");

        $stmt->bind_param('s', $username);

        $stmt->execute();

        $stmt->store_result();

        if ($stmt->num_rows > 0) {
            $stmt->close();
            return true;
        } else {
            $stmt->close();
            return false;
        }
    }

    public function hashSSHA($password) {
        $salt = sha1(rand());
        $salt = substr($salt, 0, 10);
        $encrypted = base64_encode(sha1($password . $salt, true) . $salt);
        $hash = array('salt' => $salt, 'encrypted' => $encrypted);
        return $hash;
    }

    public function checkhashSSHA($salt, $password) {
        $hash = base64_encode(sha1($password . $salt, true) . $salt);

        return $hash;
    }

}

?>

index.php:

<?php
?>
<!doctype html>
<html>
    <head>
        <meta name="viewport" content="width=device-width, initial-scale=1">
        <title>Website</title>
        <link href='https://fonts.googleapis.com/css?family=Oswald' rel='stylesheet' type='text/css'>
        <link rel="stylesheet" href="css/main.css">
    </head>
    <body background='img/main_bg.jpg'>
        <div id="header">
            <div id="login-div">
                <form action="login.php" method="post">
                    <ul id="login">
                        <li>
                            <input type="text" name="username" placeholder="Username">
                        </li>
                        <li>
                            <input type="password" name="password" placeholder="Password">
                        </li>
                        <li>
                            <input type="submit" value="Login">
                        </li>
                    </ul>
                </form>
            </div>
        </div>
        <div id="container">
            <h1 id="main-title">Website</h1>
            <div id="register-div">
                <form action="register.php" method="post">
                    <ul id="register">
                        <li>
                            <input type="text" name="username" placeholder="Username">
                        </li>
                        <li>
                            <input type="text" name="email" placeholder="Email">
                        </li>
                        <li>
                            <input type="password" name="password" placeholder="Password">
                        </li>
                        <li>
                            <input type="password" name="confirm-password" placeholder="Confirm Password">
                        </li>
                        <li>
                            <input type="checkbox" id="agreement" name="agreement" value="agreement">
                            <label for="agreement" id="agreement-label">I have read and agree to the terms of service.</label>
                        </li>
                        <li>
                            <input type="submit" value="Register">
                        </li>
                    </ul>
                </form>
            </div>
        </div>
        <div id="footer">
            FOOTER
        </div>
    </body>
</html>

register.php:

<?php

require_once dirname(__FILE__) . '/includes/functions/functions.php';
$db = new DB_Functions();

$errors = array();

if (!empty($_POST['username']) && !empty($_POST['email']) && !empty($_POST['password']) && !empty($_POST['confirm-password'])) {
    if (isset($_POST['agreement'])) {
        $username = $_POST['username'];
        $email = $_POST['email'];
        $password = $_POST['password'];
        $confirm_password = $_POST['confirm-password'];

        if ($password == $confirm_password) {
            if (filter_var($email, FILTER_VALIDATE_EMAIL) === false) {
                $errors[] = 'You must use a valid email address.';
            }
            if ($db->doesUserEmailExist($email)) {
                $errors[] = 'The email ' . $email . ' is already in use.';
            }
            if (preg_match("/\\s/", $username) == true) {
                $errors[] = 'Your username cannot contain spaces.';
            }
            if ($db->doesUsernameExist($username)) {
                $errors[] = 'The username ' . $username . ' is already in use.';
            }
            if (strlen($password) < 6) {
                $errors[] = 'Password must contain at least 6 characters.';
            }
        } else {
            $errors[] = 'Your passwords do not match.';
        }
    } else {
        $errors[] = 'You must accept terms agreement before registering.';
    }
} else {
    $errors[] = 'All fields are required.';
}

if (isset($_GET['success']) && empty($_GET['success'])) {
    echo 'YOU HAVE REGISTERED SUCCESSFULLY';
} else {
    if (!empty($_POST) && empty($errors)) {
        $user = $db->storeUser($username, $email, $password);
        if ($user) {
            header('Location: register.php?success');
            exit();
        }
    } else if (!empty($errors)) {
        echo json_encode($errors);
    }
}

?>

login.php:

<?php

require_once dirname(__FILE__) . '/includes/functions/functions.php';
$db = new DB_Functions();

$errors = array();

if (!empty($_POST['username']) && !empty($_POST['password'])) {
    if (!$db->doesUsernameExist($_POST['username'])) {
        $errors[] = 'We can\'t find this username. Have you registered?';
    } else {
        if (!$db->getUserByUsernameAndPassword($_POST['username'], $_POST['password'])) {
            $errors[] = 'The username/password combination is incorrect.';
        } else {
            if (isActive($username)) {
                $user = $db->getUserByUsernameAndPassword($_POST['username'], $_POST['password']);
                $_SESSION['user_id'] = $user['user_id'];
                header('Location: home.php');
                exit();
            } else {
                $errors[] = 'You have not activated your account.';
            }
        }
    }
} else {
    $errors[] = 'Username and password are required.';
}

echo json_encode($errors);

?>

Mike Brant · Accepted Answer · 2016-08-05 21:56:51Z

Config.php

Don't use root MySQL user. Define a user for your application with appropriate permissions.
Don't use empty string password.
Ideally, inject DB credentials from outside your code base (i.e. environmental configuration).

Connect.php

It looks like you are trying to implement a singleton here, but your implementation is a little off. You will always instantiate and replace a new connection regardless as to whether you have one. Also, method to supply the connection is typically static.

Instead you might consider something like:

<?php
class DB_Connect {
    // I most always use protected visibility when starting a class
    // unless I know for sure I will not ever extend it.
    // This is static property as it will only be called statically
    // Name the variable to be clear as to what it will hold - in 
    // this case a mysqli object.
    protected static $mysqli;

    // Make a private constructor as you do not want to allow
    // a concrete instantiation of this class
    private function __construct() {}

    // make static function to get mysqli instance 
    public static function get_mysqli() {
        // do we already have a connection available?
        if(self::$mysqli instanceof mysqli) {
            // No we don't, so let's instantiate.
            // Note that I have removed your require for config.
            // If you are going to have a common app config file it
            // should be included up the call stack, not have
            // some class require it.
            $mysqli = new mysqli(DB_HOST, DB_USER, DB_PASS, DB_NAME);
            // throw Exception if connection failed
            if($mysqli->connect_error) {
                thrown new Exception(
                    'MySQL connection failed with "' .
                    $mysqli->connect_error . '"'
                );
            }
            // Set the instance to class property.
            // This will only happen once per code execution
            // regardless as to how many times this method is
            // called.
            self::$mysqli = $mysqli;
        }
        // return mysqli object to caller
        return self::$mysqli;
    }
}
?>

DB_Functions

Why do you have DB connection logic in two places? To me this class might better be called User_Login or something similar since it has nothing to do with general DB functions at all.
Consider passing DB connection to class constructor as dependency rather than having this class have to understand how to instantiate it. You can enforce that a valid mysqli object is passed by specifying this by type hinting the parameter.
Don't roll your own hashing functions - use PHP's password_hash and password_verify functions. Note this will also require change in your DB table schema since you now longer would need separate column for salt.
You have no input validation on any of your methods. You should validate input and fail fast if necessary before working with it.
Your DB interactions only consider happy path. What if a prepare fails? What if query fails?
Don't use SELECT * it is wasteful of bandwidth and makes it unclear to someone reading the code as to what values they are expecting to be returned.
When you "store user". Why make a second call to select out user information? You just inserted all that same information a few lines before that. This query is totally unnecessary.
You might consider having a proper User class rather than just passing around associative arrays of user information.

Register.php

Your first few lines of code might be better formulated like this:

<?php
// Not shown - require config here
// Not shown - require all classes here

// instantiate DB connection
$mysqli = DB_Connect::get_mysqli();
// instantiate your user login object, passing it the mysqli object
$user_login = new User_Login($mysqli);
...

Here you make all your dependencies clear up front, not hidden away in other class files. You also set up your single DB connection and pass it to the User_Login upon instantiation. You are injecting your dependencies to the objects that need them, allowing you to remove all logic from classes on how to set up their dependencies.

Consider using exact comparisons === instead of loose comparisons == as a matter of course, unless you have a really good reason to do loose comparison. This will save you a lot of debugging time down the line from unexpected truthy/falsey behaviors.
Why redirect the users back to the same page with passing a parameter on success? Why not just indicate success directly? This seems totally unnecessary.

login.php

Same comments here about setting up your dependencies up front.
Consider not exposing "username not found" type of messaging. This helps in two ways. One, you can reduce down to a single database call here to validate credentials - one call to get user info based on username, then you password_verify() the results. Two, it is generally considered bad practice to indicate that a username doesn't exist, as you make your application less secure. You have given a potential attacker a significant piece of information in telling them that a username does or does not exist, rather just saying login failed.
Not sure why isActive() is a function at all. If you call database with username and say return a user object or array, you should just be able to derive active status based on that information. Why have yet another database access to get this information that you could have gotten with your first and only database call?

Stack Exchange Network

PHP/MySQLi code for registration/login form

1 Answer 1

You must log in to answer this question.

Hot Network Questions

PHP/MySQLi code for registration/login form

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions