10
\$\begingroup\$

I'm learning about OOP and putting my knowledge into practice. I created a login system in PHP, so I wish someone could tell me if I'm on the right track. I feel like I'm getting better at coding, but it's never good enough, so I want someone more experienced (especially in OOP) to look at my code and tell me if I'm doing it right and what I can improve.

Just a note, I recognize that the PHP code contained in HTML is not something appropriate, but I just put it to make it easier to understand how the system works. Therefore, I ask you to focus on the login system part, more precisely on OOP.

Tree

Login(directory):.
│   composer.json
│   Dashboard.php
│   Main.php
│   Signin.php
│
├───.idea
│       .gitignore
│       Login.iml
│       modules.xml
│       php.xml
│       workspace.xml
│
├───src
│       HttpTransport.php
│       Login.php
│       Session.php
│       UserAccount.php
│
└───vendor
    │   autoload.php
    │
    └───composer
            autoload_classmap.php
            autoload_namespaces.php
            autoload_psr4.php
            autoload_real.php
            autoload_static.php
            ClassLoader.php
            LICENSE

UserAccount.php

<?php

declare(strict_types = 1);

namespace Login;

class UserAccount
{
    private string $email;
    private string $password;

    public function setEmail(string $email): void
    {
        $this->email = $email;
    }

    public function getEmail(): string
    {
        return $this->email;
    }

    public function setPassword(string $password): void
    {
        $this->password = $password;
    }

    public function getPassword(): string
    {
        return $this->password;
    }
}

Session.php

<?php

declare(strict_types = 1);

namespace Login;

class Session
{
    public static function startSession(string $sessionName, string $value)
    {
        return $_SESSION[$sessionName] = $value;
    }
}

HttpTransport

<?php

declare(strict_types = 1);

namespace Login;

class HttpTransport
{
    public static function setFlashMessage(string $message): string
    {
        return $message;
    }

    public static function redirect(string $path): void
    {
        header('Location: ' . $path);
        exit;
    }
}

Login.php

<?php

declare(strict_types = 1);

namespace Login;

use \Login\HttpTransport;
use \Login\Session;

class Login
{
    public function __construct(public \PDO $conn)
    {
    }

    public function login(UserAccount $user)
    {
        $sql = "SELECT * FROM users WHERE email = ?";
        $statement = $this->conn->prepare($sql);
        $statement->execute([
            $user->getEmail()
        ]);

        if ($statement->rowCount() > 0)
        {
            $rows = $statement->fetchAll(\PDO::FETCH_ASSOC);

            foreach ($rows as $row)
            {
                $passwordHash = $row['passwordHash'];
                $hash = password_verify($user->getPassword(), $passwordHash);

                if ($hash)
                {
                    Session::startSession('username', $row['username']);
                    HttpTransport::redirect('/login/dashboard.php');
                }
                else
                {
                    $flashMessage = HttpTransport::setFlashMessage('<p>Incorrect e-mail or password.</p>');
                    Session::startSession('error', $flashMessage);
                    HttpTransport::redirect('/login/signin.php');
                }
            }
        }
        else
        {
            $flashMessage = HttpTransport::setFlashMessage('<p>Incorrect e-mail or password.</p>');
            Session::startSession('error', $flashMessage);
            HttpTransport::redirect('/login/signin.php');
        }
    }
}

Signin.php

<?php
    session_start();
    session_regenerate_id(true);

    if (isset($_SESSION['username']))
    {
        header('Location: dashboard.php');
        exit;
    }
?>

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Login</title>
</head>
<body>
    <form method="POST" action="Main.php">
        <?php
            if (isset($_SESSION['error']))
            {
                echo $_SESSION['error'];
                unset($_SESSION['error']);
            }
        ?>

        <p>E-mail</p>
        <input type="email" placeholder="Enter e-mail" name="email" required autofocus><br><br>
        <p>Password</p>
        <input type="password" placeholder="Enter password" name="password" required><br><br>

        <input type="submit" value="Login">
    </form>
</body>
</html>

Dashboard.php

<?php

session_start();
session_regenerate_id(true);

$username = $_SESSION['username'];

if (isset($username))
{
    echo "Logged in!";
    echo "<br><br>";
    echo "Welcome, " . htmlentities($username);
}
else
{
    header('Location: signin.php');
    exit;
}

Main.php

<?php

declare(strict_types = 1);

require_once __DIR__ . '/vendor/autoload.php';

session_start();

try
{
    $username = 'root';
    $password = '';

    $conn = new \PDO("mysql:host=localhost;dbname=customers;charset=utf8mb4", $username, $password, [
        \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION,
        \PDO::ATTR_EMULATE_PREPARES => false
    ]);
}
catch (\PDOException $e)
{
    throw new \PDOException($e->getMessage(), (int) $e->getCode());
}

$user = new \Login\UserAccount();
$user->setEmail(filter_input(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL));
$user->setPassword(filter_input(INPUT_POST, 'password', FILTER_SANITIZE_STRING));

(new \Login\Login($conn))->login($user);

SQL

-- phpMyAdmin SQL Dump
-- version 5.1.1
-- https://www.phpmyadmin.net/
--
-- Host: 127.0.0.1
-- Generation Time: Nov 11, 2021 at 07:49 PM
-- Server version: 10.4.21-MariaDB
-- PHP Version: 8.0.12

SET SQL_MODE = "NO_AUTO_VALUE_ON_ZERO";
START TRANSACTION;
SET time_zone = "+00:00";


/*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */;
/*!40101 SET @OLD_CHARACTER_SET_RESULTS=@@CHARACTER_SET_RESULTS */;
/*!40101 SET @OLD_COLLATION_CONNECTION=@@COLLATION_CONNECTION */;
/*!40101 SET NAMES utf8mb4 */;

--
-- Database: `customers`
--

-- --------------------------------------------------------

--
-- Table structure for table `users`
--

CREATE TABLE `users` (
  `id` int(11) NOT NULL,
  `username` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
  `passwordHash` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,
  `email` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;

--
-- Dumping data for table `users`
--

INSERT INTO `users` (`id`, `username`, `passwordHash`, `email`) VALUES
(1, 'PHP', '$2y$10$sJgO/trlE5Ik1PuIEgzKkuur2V3vpNJ1kQAZfkyjURQD2AWuF3IWK', 'php@mail.com'),
(3, 'Warlock', '$2y$10$B3VHrEmkjUUk2yncfQs.V.lR8FXat0tBj.LhxWEU9U5fuXhkYcSAi', 'warlock@mail.com');

--
-- Indexes for dumped tables
--

--
-- Indexes for table `users`
--
ALTER TABLE `users`
  ADD PRIMARY KEY (`id`),
  ADD UNIQUE KEY `username` (`username`),
  ADD UNIQUE KEY `email` (`email`);

--
-- AUTO_INCREMENT for dumped tables
--

--
-- AUTO_INCREMENT for table `users`
--
ALTER TABLE `users`
  MODIFY `id` int(11) NOT NULL AUTO_INCREMENT, AUTO_INCREMENT=4;
COMMIT;

/*!40101 SET CHARACTER_SET_CLIENT=@OLD_CHARACTER_SET_CLIENT */;
/*!40101 SET CHARACTER_SET_RESULTS=@OLD_CHARACTER_SET_RESULTS */;
/*!40101 SET COLLATION_CONNECTION=@OLD_COLLATION_CONNECTION */;

If you want to make login:

E-mail: warlock@mail.com
Pass: Rptw36VWBU%7DF

composer.json

{
  "autoload":
  {
    "psr-4":
    {
      "Login\\" : "src/"
    }
  }
}

Questions

  1. Is my code clean?
  2. Am I following the sole responsibility principle that a class should only do one thing?
  3. Regarding naming, do you think I'm naming things correctly?
  4. What do you suggest for me to become a better developer?

I appreciate any help, if you can give tips on how to improve the code or how to learn more, something like recommending books, I would be happy to. My goal is to learn, I believe the code is well structured and easy to read, but maybe you have more experience and can find something wrong, however, my goal is to learn, so feel free to criticize and give tips.

\$\endgroup\$
8
  • \$\begingroup\$ The question I'd ask you is why do you want to use OOP? Just because you read about something doesn't mean it's a good idea. I would think about what exactly you're trying to achieve using it, and if you don't know, try and learn that too rather than focusing on 'best practices' for no clear reason. \$\endgroup\$
    – rak1507
    Nov 11 at 20:39
  • 1
    \$\begingroup\$ @KIKOSoftware Yes, the code works. The constructor method takes a PDO connection, you can check this in the Main.php file. Obs: I'm using PHP version 8.0.12 \$\endgroup\$
    – Warlock
    Nov 11 at 21:36
  • 3
    \$\begingroup\$ Ah, Constructor Promotion. I missed that trick, still working in PHP 7 for now. \$\endgroup\$ Nov 11 at 21:45
  • 5
    \$\begingroup\$ Personal opinion: class UserAccount should simply have public variables. Classes with only variables which all have getters and setters are useless imo. (Though I feel like a lot of people don't agree with me. @those people: I come from a C++ background, and would make this a struct instead of a class.) \$\endgroup\$ Nov 12 at 4:59
  • 1
    \$\begingroup\$ What's the purpose of setFlashMessage? \$\endgroup\$
    – user253751
    Nov 12 at 13:27
18
\$\begingroup\$

This is a pretty good attempt at a login system. The code looks much better than 99% of code that I see on Stack Overflow. Keep up the good work!

Your code is almost clean. Your classes do so little that I would consider it too little (more on this later). The naming is almost perfect IMHO. Nonetheless, I have noticed a number of smaller issues that I think you ought to know to become a better developer.

Redirects

You follow the general rule for 302 redirects: header and exit. Once you switch over to PHP 8.1 I highly recommend using the never type. Take a look at your Login::login() method. You have redirect() followed by else block. That's unnecessary. The code after redirect will never be executed.

Login::login() needs refactoring

It does what it was supposed to do, but the code is overly complex. Despite calling fetchAll() your foreach loop will never iterate more than one row. Either way the code exits due to redirects. password_verify() does not return hash, so you named the variable incorrectly. There's also no reason for the temporary variable. if($statement->rowCount() > 0 is generally considered an antipattern and is unnecessary in this code. SELECT * is also an antipattern you should avoid.

Consider how it could be simplified and still do the same:

public function login(UserAccount $user):void
{
    $sql = "SELECT passwordHash, username FROM users WHERE email = ?";
    $statement = $this->conn->prepare($sql);
    $statement->execute([
        $user->getEmail()
    ]);

    if ($row = $statement->fetch(\PDO::FETCH_ASSOC)) {
        if (password_verify($user->getPassword(), $row['passwordHash'])) {
            Session::startSession('username', $row['username']);
            HttpTransport::redirect('/login/dashboard.php');
        }
    }
    $flashMessage = HttpTransport::setFlashMessage('<p>Incorrect e-mail or password.</p>');
    Session::startSession('error', $flashMessage);
    HttpTransport::redirect('/login/signin.php');
}

I also added void return type, but once you move to PHP 8.1 you should use never.

Session class

At the moment this class does nothing. You could remove it. However, I think it's a good idea for a class like this with methods to start, regenerate and kill the session. In the start method, you should ensure you use the right storage method, secure cookies, HTTP only. Use session_set_cookie_params() to set these options. You should also set the session name and start it (session_start() should belong in this class).

Also, session_regenerate_id() after session_start() is not such a good idea. You should regenerate it after successful login and at regular intervals, but not every time the page is loaded.

FILTER_SANITIZE_STRING is deprecated in 8.1

Do not use this filter. You simply don't need it. Passwords should not be modified in any way. Whatever the value you got from the user, should be the value used in password_verify() without any modification. You can use FILTER_UNSAFE_RAW which leaves the value unchanged.

Database credentials are hardcoded

Database credentials should never be present in the code. Use config file (do not store it in VCS). You can use any format you want for the config file, PHP file, JSON, INI, YAML, NEON, etc. Just do not put it in the code!

Document root

It looks like your entry points are stored in the main directory. You should create public directory that will be accessible from the internet and store your entry points there. All other code should be inaccessible from the outside.

Login does too little

This is the last point as this is just an opinion. You don't need to listen to this, but in my opinion the class is too restricted. I would call the class Auth and put all your authentication-related functionality there. There could be a method called logout, a method that rehashes passwords, a method that stores invalid attempts in the database (without passwords) so that you can rate limit or show captcha.

On the other hand, I would also move all password-related functionality to a separate class. There could be a method that calls https://api.pwnedpasswords.com/range API to check whether the password is compromised. Another method that checks whether the password needs to be rehashed (see. password_needs_rehash()). And of course, a method to generate a hash (remember to forbid empty passwords and ones that contain NUL bytes; throw an exception if such password is provided).

Conclusion

You are very knowledgeable and you are using the latest PHP 8 features. I see you have also read good online resources, e.g. https://phpdelusions.net/. The code is a very good start. You need a lot more security considerations if this is to become a real login system, e.g. secure sessions/cookies, password rehashing, checking passwords against leaked passwords.

You have also avoided many pitfalls common to beginners. You don't have SQL injection, you avoid XSS, you catch PDO exception to prevent accidental credential leaks in error logs, you use strict types, utf8mb4 in the database, password hashing, and you didn't add arbitrary password restrictions.

I'd consider you an experienced developer and a valuable asset to any team.

\$\endgroup\$
0
7
\$\begingroup\$

A few other more minor bits not mentioned by Dharman, but agree 100% that the code is very good to start with, the Post - Redirect - Get pattern stands out many login systems do not have that implemented well.

Consider 2fa

There are plenty of packages which implement totp or u2f use one of them for an additional security boost

Structure of file system

It's hard to be sure, but looking at the structure, that whole structure is exposed to the world.

This means composer.json for example is exposed to the world, in your case currently, that's not a major issue as there are no dependencies, but imagine you use somePackage later on. somePackage then gets a CVE assigned, you have stated you are using the package and from the version you can see if your server is vulnerable. Vendor is also open.

Most projects use a public folder in the root and have a single entrypoint php file, which prevents anything like this occurring. Your code is private as its not exposed, and the only public PHP is the index.php file entrypoint which will do things like set up composer and then call the appropriate php functions. Other public assets like images and stylesheets etc would go in this public directory then too.

The hash chosen

While there is nothing wrong with bcrypt (it is not mentioned here but clear from the test credentials), for new code I would suggest using Argon2id (https://www.php.net/manual/en/function.password-hash.php)

Too much info? Possibly just a bad user experiance

Some day, your database will stop responding. When that happens you do have code for it.

catch (\PDOException $e)
{
    throw new \PDOException($e->getMessage(), (int) $e->getCode());
}

That code will catch and then rethrow, in a prod enviroment where (hopefully) display errors is set to 0, which will only leave this with a blank page. if that variable ever accidentally gets set to 1, your database credentials will likely be part of the output.

In either case, handling that message with a simple error page to the user is much preferred. Log to a file / some sort of service to alert you, and display an error that you choose, not the standard PHP one.

There are a few ways you can do this, either wrapping everything in a try catch, or using set_error_handler are common ways to do this.

Future code duplication

    public function setEmail(string $email): void
    {
        $this->email = $email;
    }

    public function getEmail(): string
    {
        return $this->email;
    }

The above implies that there is no checking at all of email.

in which case it could / should just be a public attribute, but calling it has this code

$user->setEmail(filter_input(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL));

In another place in later development, you now need to copy paste this, the validation should really be inside of the method. eg

public function setEmail(string $email): void
{
    $email = filter_var($email, FILTER_SANITIZE_EMAIL);
    if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
        // probably throw some sort of exception here as it's invalid input.
    }
    $this->email = $email;
}

This would be picked up by static analysis too as the filter_input command returns mixed, you are passing that output to a function which accepts string.

https://phpstan.org/r/6f7d1925-ddb8-4a42-add3-d1581feed3ef

phpstan is great and I would recommend running your code though that

The above code also helps with unit testing, as now your handle code is separate from your input code, so you can test submitting bad input without needing to manipulate the global post variables.

elses

The else statement is never necessary and code is always more simple without. Consider guard conditions or similar

if (isset($username))
{
    echo "Logged in!";
    echo "<br><br>";
    echo "Welcome, " . htmlentities($username);
}
else
{
    header('Location: signin.php');
    exit;
}

Could be

if (empty($username))
{
    header('Location: signin.php');
    exit;

}
echo "Logged in!";
echo "<br><br>";
echo "Welcome, " . htmlentities($username);

For example which removes one level of indentation for your "happy path"

The re written code in the above answer does the same thing, but not mentioning the else removal explicitly.

Schema

`passwordHash` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL,

You hash will never need that charset, super minor but could use a much more limiting charset there such as latin1 to save some space, if you have millions of users, these things start to make a difference.

Password in memory

This really is a minor point (and 99% of systems will not have this), when handling passwords, I prefer to use something like sodium_memzero or just an unset as soon as I have used the secret. This reduces the amount of time in memory which a password lives. The current way lets php decide when to release that memory. This would include the post variable and the UserAccount::$password variable.

Final words

As mentioned above, despite the number of small issues here, it's actually good code, I have looked at many login systems in use, and yours even as it was originally posted is ranked high on that list.

Using tools like phpcs, phpstan, phpmd and phpmnd will help you a huge amount. Get comfortable using them also phpunit for writing tests will be a great thing to learn.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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