Registering usernames and passwords to a database

Question 1

Here is my script that registers a user to a database. I am still very much a newbie when it comes to this topic so any guidance would be greatly appreciated.

<html>
<head>
<title>Register</title>
</head>
<body>
<?php
require ("opendboLogin.php");

main();

function main()
{
    if(!isset($_POST['submit'])) //if user tries to access page without hitting submit button
    {
        header("location:loginRegister.html");
        die();
    }

    $date = date("Y-m-d H:i:s"); // 2001-03-10 17:16:18 (the MySQL DATETIME format)

    $username = $_POST["username"]; //these come from "loginRegister.html"

    $username = strtolower($username);

    $email = $_POST['email'];

    global $conn; //this is from "openDboLogin.php"

    $email    = $conn->real_escape_string($email);

    $password = $_POST["password"];

    if(!usernameAndPassIsValid($username, $password, $email)) //if username/password function returns false
    {
        header('Refresh: 5;url=loginRegister.html');
        die('Username, password, or email does not meet requirements. Try again. <br>
            <b>You will be re-directed to the registration page in 5 seconds.</b>');
    }

    if(!usernameAndEmailIsUnique($username, $email)) //if username and email is not unique to Db
    {   
        header('Refresh: 5;url=loginRegister.html');
        die('The username or email you provided is already in use, please use another. <br>
            <b>You will be re-directed to the registration page in 5 seconds.</b>');
    }

    if(!insertInfo($username, $password, $date, $email)) //if theres an error instering info into DB
    {
        header('Refresh: 5;url=loginRegister.html');
        die('Error occurred inserting info, try again later. <br>
            <b>You will be re-directed to the registration page in 5 seconds.</b>');
    }
}

function usernameAndPassIsValid($username, $password, $email)
{
    $usernameRegEx = '/^[w]+$/';
    $emailRegEx = '/^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+.[a-zA-Z0-9-.]+$/';
    $passwordRegEx = '/S*(?=S{8,})(?=S*[a-z])(?=S*[A-Z])(?=S*[d])(?=S*[W])S*$/';

    if(preg_match($usernameRegEx, $username) && preg_match($passwordRegEx, $password) && preg_match($emailRegEx, $email))
    {
        return true;
    }
    return false;
}

function usernameAndEmailIsUnique($username, $email)
{
    global $conn; //this is from "opendboLogin.php
    $stmt1 = $conn->stmt_init();
    $stmt1 = $conn->prepare("SELECT * FROM login WHERE auser = ? OR email = ?");
    $stmt1->bind_param("ss", $username, $email);
    $stmt1->execute();
    $stmt1->store_result();
    $numberofrows = $stmt1->num_rows; 
    $stmt1 -> close();

    if($numberofrows == 0) //if username and email IS unique to DB, return true
    {
        return true;
    }
    return false;
}

function insertInfo($username, $password, $date, $email)
{
    global $conn; //this is from "openDboLogin.php"
    $stmt = $conn->stmt_init();
    if ($stmt->prepare("INSERT INTO login (adate, auser, email, apassword) VALUES (?,?,?,?)")) 
    {   
        $encryptedPassword = password_hash($password, PASSWORD_DEFAULT);
        if ($encryptedPassword === false){
            die('There was an error encrypting your password, please try again later.');
        }
        $stmt->bind_param("ssss", $date, $username, $email, $encryptedPassword);
        $execute = $stmt->execute();
        if($execute === false){
            die('Execution of prepared statement failed: ' . htmlspecialchars($stmt->error));
        }
        print("Account info added successfully <br> Your username is:<b> $username </b><br>
             The email you registered with is:<b> $email </b><br> 
             <a href='loginLogin.html'>Click here</a> to login.");
        $stmt->close();
        return true;
    }
    return false;
}



?>
</body>
</html>

As you can see, I split the script into functions and I call all of them within the main() function to see if they return false. Is this good practice?

Also within my insertInfo() function, am I error-checking the prepared statement correctly?

My regular expressions check for:

Username: Only letters, numbers, and underscores allowed
Password: At least 8 chars, one uppercase, one lowercase, one number, and one symbol

Question 2

First of all, I generally like your code. It is generally readable, and your use of functions is great.

Just a list of small stuff that I noticed:

don’t echo user supplied data without encoding it. Your regexes might change in the future, which would open you up to XSS.
don’t echo db messages to the enduser. It’s not user friendly, and it might reveal some information
don’t die or print in functions, it makes them hard to reuse.
you use prepared statements – which is great – so why use real_escape_string?
your main function has too many newlines
usernameAndPassIsValid is odd phrasing. I would go with isValidUsernameAndPassword, or just create two functions isValidUsername and isValidPassword.
splitting this up also allows you to report to the user what exactly went wrong. It’s really annoying to read that the form is wrong, but then not to get information on why.
if (cond) return true else return false can be rewritten as return cond;
having functions and other code in the same file makes it impossible to reuse the functions, which is why you shouldn’t mix this. Maybe you want to check the validity of a username somewhere else (eg when updating a username).
I didn’t check your regex, but if your description is accurate, you are way too restrictive. 8 chars is not enough for a password, and there really shouldn’t be a reason to restrict the length at all.
personally, I don’t really like global variables. It makes code a bit messy and hard to test. I would pass the db info on via function parameters
except for the date explanation and the reference to the html page, your comments only describe what the code does. Since I already know that from reading the code, you should remove them. Too many such comments lead to people not reading your important comments anymore.

Question 3

I’ve a few points to point out, for the moment:

if(preg_match($usernameRegEx, $username) && preg_match($passwordRegEx, $password) && preg_match($emailRegEx, $email))
{
    return true;
}
return false;

You can directly return the variable:

return (preg_match($usernameRegEx, $username) && preg_match($passwordRegEx, $password) && preg_match($emailRegEx, $email));

Same thing here:

if($numberofrows == 0) //if username and email IS unique to DB, return true
{
    return true;
}
return false;

return ($numberofrows == 0);

print("Account info added successfully <br> Your username is:<b> $username </b><br>
         The email you registered with is:<b> $email </b><br> 
         <a href='loginLogin.html'>Click here</a> to login.");

You should be wrapping PHP variables inside strings like: {$string} instead of just $string.

You don’t need all this extraneous stuff around the PHP.

If you echo "<title>Register</title> at the top of your code, the rest of the HTML would build by itself.

Otherwise, your HTML isn’t indented correctly. Four spaces plus the parent’s indention is where the children elements should be.

Leave a Reply Cancel reply