0
\$\begingroup\$

Now i have a fully functional PHP codes for login and registeration i was wondering if anyone could offer improvements to the code

The code:

Config.php:

<?php
return [
    'db' => [
        'hostname' => 'localhost',
        'username' => 'Bebo',
        'password' => 'Bebo',
        'database' => 'Bebo',
        'port' => 3306,
    ],
    'db_charset' => 'utf8mb4',
];

init.php:

<?php
date_default_timezone_set('Asia/Riyadh');
$error = ['Username' => '', 'Email' => '', 'Password' => ''];
$input = ['Username' => '', 'Email' => '', 'Password' => ''];
session_start();
$config = require 'Config.php';
$db = new mysqli(...$config['db']);
$db->set_charset($config['db_charset']);

Registeration.php:

<?php
require 'init.php';

if ($_SERVER['REQUEST_METHOD'] === 'POST') {
    // input validation

    $Username = $input['Username'] = trim(filter_input(INPUT_POST, 'Username'));
    if (mb_strlen($Username) < 3 || mb_strlen($Username) > 30) {
        $error['Username'] = 'Please enter your name, it must be from 3 to 30 charaters long.';
        echo "<p class='Center'> <font color=White  size='50pt'>Username should be at least 3 characters long!</font> </p>";
    }

    $Email = $input['Email'] = trim(filter_input(INPUT_POST, 'Email'));
    if (!filter_var($Email, FILTER_VALIDATE_EMAIL)) {
        $error['Email'] = 'Please enter a valid email address.';
        echo "<p class='Center'> <font color=White  size='50pt'>Please enter a valid email address!</font> </p>";

    } else {
        $result = $db->execute_query("SELECT 1 FROM users WHERE email = ?", [$Email]);
        if ($result->fetch_row()) {
            $error['Email'] = 'Email address already taken.';
            echo "<p class='Center'> <font color=White  size='50pt'>Email address already taken.Please Login!</font> </p>";

        }
    }

    $Password = $input['Password'] = filter_input(INPUT_POST, 'Password');
    if (strlen($Password) < 3 || strlen($Password) > 72) {
        $error['Password'] = 'Please enter password, it must be from 3 to 72 characters long.';
        echo "<p class='Center'> <font color=White  size='50pt'>Password should be at least 3 characters long!</font> </p>";
    }
        // if no errors
        if (implode("", $error) === '')
        {
            // Password MUST be hashed using the dedicated function
            $Password = password_hash($input['Password'], PASSWORD_DEFAULT);
            $VIP= "NO";
            $Admin = "NO";
            $Creation_date = date('d-M-Y h:i:s A');
            $Last_Login = date('d-M-Y h:i:s A');
            $Login_Times=1;
            // a parameterized query MUST be used to avoid errors and injections
            $stmt = $db->prepare("INSERT INTO Users (Username, Email, Password, VIP, Admin, Creation_Date, Last_login, Login_Times) VALUES (?,?,?,?,?,?,?,?)");
            $stmt->execute([
                $Username, 
                $Email,
                $Password,
                $VIP,
                $Admin,
                $Creation_date,
                $Last_Login,
                $Login_Times,
            ]);
            echo "<p class='Center'> <font color=White  size='50pt'>Registeration successful</font> </p>";
            $_SESSION['Email'] = $Email;
            header("Location: ../home.php");
            die;
        }
    }
?>
<!DOCTYPE html>
<html>
<head>
<meta http-equiv="refresh" content="4;URL=../login.php">
    <link rel="stylesheet" href="../Styles/General.css">
    <link rel="stylesheet" href="../Styles/Background.css">
    <link rel="icon" href ="favicon.ico" type="image/x-icon" /> <link rel="shortcut icon" href="favicon.ico" type="image/x-icon" />

    <title>Register & Login</title>
</head>

<body class="Blue-Black">
    <h1 class="Center">Please wait, you will be automatically redirected to the login & registeration page.</h1>
</body>
</html>

Validation.php:

<?php
require 'init.php';

$Email = $input['Email'] = trim(filter_input(INPUT_POST, 'Email'));
$Password = $input['Password'] = filter_input(INPUT_POST, 'Password');
$result = $db->execute_query("SELECT Email FROM Users WHERE Email = ?", [$Email]);
    if ($result->fetch_row()) {
        $select = "SELECT Password FROM Users WHERE Email = ?;";
        $result2 = $db ->execute_query($select, [$Email]) ; 
        $Get_hash = $result2 ->fetch_assoc();
        $hash = $Get_hash['Password'];
            if (password_verify($Password, $hash)) {
            $_SESSION['Email'] = $Email;
            $Date = date('d-M-Y h:i A');
            $Update = "UPDATE Users SET Last_Login = ?, Login_Times = Login_Times + 1 WHERE Users.Email = ?";
            $stmt = $db->execute_query($Update, [$Date, $Email]);
            header('location:../home.php');
            }else{
                echo "<p class='Center'> <font color=White  size='50pt'>Invalid Password. Try again!</font> </p>";
            }
    }else{
        echo "<p class='Center'> <font color=White  size='50pt'>There is no account associated with this email address please sign up!</font> </p>";
    }
?>
<!DOCTYPE html>
<html>
<head>
<meta http-equiv="refresh" content="4;URL=../login.php">
    <link rel="stylesheet" href="../Styles/General.css">
    <link rel="stylesheet" href="../Styles/Background.css">
    <link rel="icon" href ="favicon.ico" type="image/x-icon" /> <link rel="shortcut icon" href="favicon.ico" type="image/x-icon" />

    <title>Register & Login</title>
</head>
<body class="Blue-Black">
    <h1 class="Center">Please wait, you will be automatically redirected to the login & registeration page.</h1>
</body>
</html>

Any suggestions and improvements associated with explanation would be appreciated!!

\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Nice to hear you got your code working. I will help you a bit with the validation script. These are the points of improvement:

  • Your input filtering is not doing anything because you haven't specified the type, I added FILTER_VALIDATE_EMAIL, and used it to give feedback.
  • You defined $input['Email'] and $input['Password'] but never use them. I removed them.
  • As I said before, one of the SELECT queries is not needed. I removed one.
  • You can get the current time in MySQL itself, see NOW(), no need to do it in PHP.
  • You output HTML outside of the HTML document body when there's an error.
  • I really don't like the forced redirection to the login page when an error occurs. This also contradicts the header you set when the login is successful, which makes it unpredictable what will actually happen. I use a "Retry" button to go to the login page when an error occurs.

This all results in the following code:

<?php

require 'init.php';

$userEmail    = trim(filter_input(INPUT_POST, 'Email', FILTER_VALIDATE_EMAIL));
$userPassword = filter_input(INPUT_POST, 'Password', FILTER_UNSAFE_RAW);
$errorMessage = '';
if ($userEmail) {
    $select = "SELECT Password FROM Users WHERE Email = ?";
    $result = $db->execute_query($select, [$userEmail]);
    if ($result->field_count == 1) {
        $passwordHash = $result->fetch_assoc()['Password'];
        if (password_verify($userPassword, $passwordHash)) {
            $_SESSION['Email'] = $userEmail;
            $update = "UPDATE Users SET Last_Login = NOW(), Login_Times = Login_Times + 1 WHERE Email = ?";
            $db->execute_query($Update, [$userEmail]);
            header('location:../home.php');
        } else {
            $errorMessage = "This password is not correct. Please try again!";
        }
    } else {
        if ($result->field_count > 1) {
            $errorMessage = "Multiple accounts are associated with this email address. Please contact us!";
        } else {
            $errorMessage = "No account is associated with this email address. Please sign up!";
        }
    }
} else {
    $errorMessage = "This was not a valid email address. Please try again!";
}
    
?>
<!DOCTYPE html>
<html>
<head>
    <link rel="stylesheet" href="../Styles/General.css">
    <link rel="stylesheet" href="../Styles/Background.css">
    <link rel="icon" href ="favicon.ico" type="image/x-icon" /> <link rel="shortcut icon" href="favicon.ico" type="image/x-icon" />
    <title>Register & Login</title>
</head>
<body class="Blue-Black">
    <h1 class="Center">
<?php
 
if (empty($errorMessage)) {
    echo "Please wait, you will be automatically redirected to the login & registeration page.";
} else {
    echo '<font color="white" size="50pt">' . $errorMessage . ' <a href="../login.php">Go to the login page</a>.</font>';
}

?>
    </h1>
</body>
</html>

Is this code perfect? No. But a lot of problems are resolved. There is still the problem that you ask people to sign up, but redirect them to the login page.

Then there also still the other pieces of code to work on.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks I'll be using most of these ideas but just to clarify smth the login page has both login and sign up options 😀 \$\endgroup\$ Commented Sep 16, 2023 at 5:16

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.