Storing and confirming user input

Question

I am a beginner, and I'm trying to secure a sign-login system on my website. Is my code good/enough to prevent SQL injection?

THIS IS THE SIGN FILES

This is the index.php that takes user input:

<?php

require('some_path\autoload_class.php');

$pass = $_POST['passwordd'];
$card = $_POST['cardd'];

$hashed_pass = password_hash($pass, PASSWORD_DEFAULT);
$hashed_card = password_hash($card, PASSWORD_DEFAULT);

$user = new User($_POST['emaill'], $hashed_pass, $hashed_card);
$user->insert();

echo("Your <b>Email</b> is: {$_POST['emaill']}<br>");
echo("Your <b>Password</b> is: {$pass}<br>");
echo("Your <b>Card's number</b> is: {$card}");
?>

This is User.php Class:

<?php

class User {
    //User and database info
    private $email;
    private $password;
    private $card;
    private $host = "localhost";
    private $user = "root";
    private $dbpassword = "x";
    private $dbName = "database";

    public function __construct(string $user_email, string $user_password, string $user_card) {
        $this->email = $user_email;
        $this->password = $user_password;
        $this->card = $user_card;
    }

    public function insert(){
        try{

            //The connection to the Database

            $dsn = "mysql:host={$this->host};dbname={$this->dbName}";
            $pdo = new PDO($dsn, $this->user, $this->dbpassword);
            $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
            $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            echo("Connected successfully to database!<br>");

            //The code to insert user input into database (table user)

            $sql = "INSERT INTO user(email, password, card) VALUES(?, ?, ?)";
            $prepared_statement = $pdo->prepare($sql);
            $prepared_statement->execute([$this->email, $this->password, $this->card]);
            echo("User input stored with success!<br>");

            //I did read that this is good practice for closing the connection

            $pdo = null;
            $prepared_statement = null;

            //if there's an error, it will be printed out

        }catch (PDOException $e) {
            echo("Connection failed!<br>");
            echo($e->getMessage());
        }
    }
}

THIS IS THE LOGIN FILES

This is index.php that takes user input and confirm if password is correct:

<?php

require('some_path\autoload_class.php');

$user = new login($_POST['emaill'], $_POST['passwordd']);
$user->fetch();

This is Login.php class:

<?php

class login {
    private $email;
    private $password;
    private $host = "localhost";
    private $user = "root";
    private $dbpassword = "x";
    private $dbName = "database";

    public function __construct(string $user_email, string $user_password) {
        $this->email = $user_email;
        $this->password = $user_password;
    }

    public function fetch(){
        try{

            //The connection to the database

            $dsn = "mysql:host={$this->host};dbname={$this->dbName}";
            $pdo = new PDO($dsn, $this->user, $this->dbpassword);
            $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
            $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
            echo("Connected successfully to database!<br>");

            //The query
            $sql = "SELECT * FROM user WHERE email = ?";
            $prepared_statement = $pdo->prepare($sql);
            $prepared_statement->execute([$this->email]);
            $user = $prepared_statement->fetchAll();

            //Because $user is an assocc array, I use password_verify inside this foreach
            foreach ($user as $key) {
                //if user password == to hashed password in database where email == user email
                if (password_verify($this->password, $key['password'])) {
                    echo("User Indentity Confirmed with Success!<br>");
                    echo("<h2>INFO</h2><hr><hr>");
                    echo("<b>User Id:</b> {$key['user_id']}<br>");
                    echo("<b>User Email:</b> {$key['email']}<br>");
                    echo("<b>User Password:</b> {$key['password']}<br>");
                    echo("<b>User Card number:</b> {$key['card']}");
                }else{
                    echo("<h2>User Indentity Not Confirmed.</h2>");
                }
            }
            $pdo = null;
            $prepared_statement = null;

        }catch (PDOException $e) {
            echo("Oops...Something Went Wrong!<br>");
            echo($e->getMessage());
        }
    }
}

In the sign files, I'm trying to get user input in the most secure way I possible, hashing password and credit card number, using pdo with prepared statements, and then inserting the user input into database.

In the login files, I'm trying to use the email provided by user, find the row in user table where email = user email provided, and then comparing the hashed password with password provided.

Answer 1

Your code is good enough at preventing sql injection. But it sucks (pardon me) in everything else.

Don't print database error to the user.

Don't put echoes into your logic.

Don't double last letters of post fields names.

Don't repeat yourself by integrating your database connection to every class that has nothing to do with db. Prefer composition. User should not insert itself into db. Login should not fetch itself from db. How are you going to connect to a different db without changing every class in your project?

EDIT: To clarify the last point, your code should look more like this:

// definitions 
function connect() {
  return new \PDO(/*maybe take it from environemt variables rather then hardcoding it*/);
}

// services instantiation (your bootstrap, DI container or so...)
$pdo = connect();
$users = new UsersService($pdo);


// create new user
$users->createUser($userCreateRequestData);

// to login
$user = $users->login($userName, $password);
...

Or in other words, there should be separate objects to represent a user (its data) and to manipulate user objects (ie, creating new users, verifying their password, obtaining their data, updating their data, removing them, etc...). User data dont need to know how, where and when they are persisted. And the persister dont need to carry arund data of a specific user.

You may actually want to create a separate class for authentication as that's really a bit different layer then the persistance of user data - which is db related, but auth processes dont really need to know the details. On other hand, it may need to access session which is not really a concern of the data access layer. So they should be separate - Auth layer knows session and DA layer but it doesnt care for the actual connection, DA layer knows about the connection but it doesnt care if session or auth layer exists at all.

Actually creating a user should also be a separate class. Because creating a user often involved other things then just insert a row to database (ie send an email) and those things are again no concern of DA layer.

Eventually you might end up having more and more one purpose classes (which is good).

// bootstrap
$pdo = connect();
$users = new UsersRepository($pdo);

// create user
$userFactory = new UserFactory($users, $mailService);
$userFactory->createUser($userCreateRequestData);

// login user
$auth = new Authenticator($users, $sessionService);
$user = $auth->login($userName, $password);
```