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.

• Dharman: echo is not a function. Adding parentheses is very confusing
• Dharman: Your PDO connection needs charset. Don't rely on the default one. Set it properly yourself to be utf8mb4
• irtexas19: @Dharman thank you, sorry I'm late to reply. You're always there! I will google for more information about charset. And I'm just used to use echo as a function because it looks like printf().

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);
```

• irtexas19: Thanks for the answer, i really understood everything except: "User class should not insert() itself... and Login class should not fetch() itself". What does it mean?
• slepic: @irtexas19 I have edited my answer with some more detail and code samples, have a look...