As a self-taught reasonably new programmer the thing I find most daunting is security specifically security involving databases, I have written a user auth class but
I'm struggling to find a way to validate that it is secure. Is it ok to post it here for a peer review type thing? I only really need to know if there is any massive holes
and I don't want you guys to fix them I just want them pointed out!
I only can recommend you to use proven and stable auth package, framework or service.
They will receive future updates and are most times battle proven.
If it's for learning I can try to help you if it's PHP. Have you implemented any standard? Or only a simple self thought implementation?
Its mostly for learning its pretty simple self implementation ill get some code posted!
require 'db.php';
session_start(); ///needs an open session to set the logged in value
class User {
public $username = null;
public $password = null;
public $name = null;
public $email = null;
public $ftp = null;
public $connection = null;
public $errors = array();
public $errorFlag = false;
public $lastInsertedID = null;
function __construct() {
$this->connection = connect_db(); ///this is the basic database connection
}
//Get the errors array.
public function getErrors() {
return $this->errors;
}
public function storeFormValues($username, $password, $name, $email) {
if (empty($password)) {
array_push($this->errors, "Please Enter a Password!");
$this->errorFlag = true;
} else {
if (!preg_match('/(?=.*[a-z])(?=.*\d).{5,}/i', $password)) {
array_push($this->errors, "Passwords must have a minimum of 5 characters and contain at least one letter and at least on number");
$this->errorFlag = true;
} else {
$this->password = password_hash($password, PASSWORD_DEFAULT);
}
}
if (empty($username)) {
array_push($this->errors, "Please Enter a Username!");
$this->errorFlag = true;
} else {
if (!preg_match("/^[a-zA-Z ]*$/", $username)) {
array_push($this->errors, "Only letters and white space allowed in Username");
$this->errorFlag = true;
} else {
$this->username = $username;
}
}
if (empty($name)) {
array_push($this->errors, "Please Enter a name!");
$this->errorFlag = true;
} else {
if (!preg_match("/^[a-zA-Z ]*$/", $name)) {
array_push($this->errors, "Only letters and white space allowed in Name!");
$this->errorFlag = true;
} else {
$this->name = $name;
}
}
if (empty($email)) {
array_push($this->errors, "Please Enter a Email!");
$this->errorFlag = true;
} else {
if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
array_push($this->errors, "Invalid Email format");
$this->errorFlag = true;
} else {
$this->email = $email;
}
}
if ($this->errorFlag === false) {
$this->ftp = hash('sha512', rand());
return true;
} else {
return false;
}
}
public function logout() {
session_destroy();
header("Location: ../../login.php"); ///destroys all session data and redirects to login
}
public function userLogin($email, $password) {
try {
$sql = "SELECT * FROM accounts where account_email = ?";
$stmt = $this->connection->prepare($sql);
$stmt->execute(array($email));
if ($stmt->rowCount() > 0) {
$output = $stmt->fetch();
$hash = $output['account_password'];
if (password_verify($password, $hash)) {
header_remove();
header('Location: ./home.php');
$_SESSION['loggedIn'] = 1;
$_SESSION['username'] = $output['account_username'];
$_SESSION['id'] = $output['account_id'];
return;
} else {
$_SESSION['loggedIn'] = 0;
array_push($this->errors, "User password didn't match!");
return false;
}
} else {
array_push($this->errors, "User Not Found!");
return false;
}
}
catch(PDOException $e) {
//$e->getMessage();
array_push($this->errors, "Unknown Error");
return false;
}
}
public function userRegister() {
//below is the basic code to add a user to the database
try {
$sql = "INSERT INTO accounts (account_name, account_password, account_username, account_email, ftp_password, is_admin, is_enabled, is_ftp_enabled) VALUES (?,?,?,?,?,?,?,?)";
$stmt = $this->connection->prepare($sql);
$stmt->execute([$this->name, $this->password, $this->username, $this->email, $this->ftp, "0", "1", "1"]);
$this->lastInsertedID = $this->connection->lastInsertId(); //this gets the id of thew last inserted row so it can then init all the other needed databases
}
catch(PDOException $e) {
array_push($this->errors, "We already have an account with that email!");
$this->errorFlag = true;
}
//The two insert statments below setup the default values for the ship station config saving
try {
$sql = "INSERT INTO configuration (`account_id`, `shipstationPublic`, `shipstationPrivate`) VALUES (?,?,?)"; //used backticks for escaping reserved words
$stmt = $this->connection->prepare($sql);
$stmt->execute([ $this->lastInsertedID, "0", "0"]);
}
catch(PDOException $e) {
if ($e->errorInfo[1] == 1062) {
array_push($this->errors, "Configuration Already set!");
$this->errorFlag = true;
} else {
array_push($this->errors, $e);
$this->errorFlag = true;
}
}
if ($this->errorFlag === false) {
return true;
} else {
return false;
}
}
public function getUserData($userID){
try{
$sql = "SELECT * FROM accounts where account_id = ?";
$stmt = $this->connection->prepare($sql);
$stmt->execute(array($userID));
if ($stmt->rowCount() > 0) {
$output = $stmt->fetch();
return array("email"=>$output['account_email'], "username"=>$output['account_username'], "name"=>$output['account_name']);;
}else{
return false;
}
}
catch(PDOException $e) {
array_push($this->errors, "Could not get user information");
return false;
}
}
}At super first a simple cleaned-up and fixed code:
<?php
require 'db.php';
// needs an open session to set the logged in value
session_start();
class User
{
private $username = null;
private $password = null;
private $name = null;
private $email = null;
private $ftp = null;
private $connection = null;
private $errors = [];
private $lastInsertedID = null;
public function __construct()
{
$this->connection = connect_db(); // this is the basic database connection
}
public function getErrors(): array
{
return $this->errors;
}
public function storeFormValues($username, $password, $name, $email): bool
{
if (empty($password)) {
array_push($this->errors, "Please Enter a Password!");
} else {
if (!preg_match('/(?=.*[a-z])(?=.*\d).{5,}/i', $password)) {
array_push($this->errors, "Passwords must have a minimum of 5 characters and contain at least one letter and at least on number");
} else {
$this->password = password_hash($password, PASSWORD_DEFAULT);
}
}
if (empty($username)) {
array_push($this->errors, "Please Enter a Username!");
} else {
if (!preg_match("/^[a-zA-Z ]*$/", $username)) {
array_push($this->errors, "Only letters and white space allowed in Username");
} else {
$this->username = $username;
}
}
if (empty($name)) {
array_push($this->errors, "Please Enter a name!");
} else {
if (!preg_match("/^[a-zA-Z ]*$/", $name)) {
array_push($this->errors, "Only letters and white space allowed in Name!");
} else {
$this->name = $name;
}
}
if (empty($email)) {
array_push($this->errors, "Please Enter a Email!");
} else {
if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
array_push($this->errors, "Invalid Email format");
} else {
$this->email = $email;
}
}
if (empty($this->errors)) {
$this->ftp = hash('sha512', rand());
return true;
}
return false;
}
public function logout()
{
session_destroy();
header("Location: ../../login.php"); // destroys all session data and redirects to login
}
public function userLogin($email, $password): bool
{
try {
$sql = "SELECT * FROM accounts where account_email = ?";
$stmt = $this->connection->prepare($sql);
$stmt->execute([$email]);
if ($stmt->rowCount() > 0) {
$output = $stmt->fetch();
$hash = $output['account_password'];
if (password_verify($password, $hash)) {
header_remove();
header('Location: ./home.php');
$_SESSION['loggedIn'] = 1;
$_SESSION['username'] = $output['account_username'];
$_SESSION['id'] = $output['account_id'];
return true;
} else {
$_SESSION['loggedIn'] = 0;
array_push($this->errors, "User password didn't match!");
return false;
}
} else {
array_push($this->errors, "User Not Found!");
return false;
}
} catch (PDOException $e) {
array_push($this->errors, "Unknown Error");
return false;
}
}
public function userRegister(): bool
{
// below is the basic code to add a user to the database
try {
$sql = "INSERT INTO accounts (account_name, account_password, account_username, account_email, ftp_password, is_admin, is_enabled, is_ftp_enabled) VALUES (?,?,?,?,?,?,?,?)";
$stmt = $this->connection->prepare($sql);
$stmt->execute([$this->name, $this->password, $this->username, $this->email, $this->ftp, "0", "1", "1"]);
$this->lastInsertedID = $this->connection->lastInsertId(); //this gets the id of thew last inserted row so it can then init all the other needed databases
} catch (PDOException $e) {
array_push($this->errors, "We already have an account with that email!");
}
// The two insert statements below setup the default values for the ship station config saving
try {
$sql = "INSERT INTO configuration (`account_id`, `shipstationPublic`, `shipstationPrivate`) VALUES (?,?,?)"; //used backticks for escaping reserved words
$stmt = $this->connection->prepare($sql);
$stmt->execute([$this->lastInsertedID, "0", "0"]);
} catch (PDOException $e) {
if ($e->errorInfo[1] == 1062) {
array_push($this->errors, "Configuration Already set!");
} else {
array_push($this->errors, $e);
}
}
return empty($this->errors);
}
public function getUserData($userID): ?array
{
try {
$sql = "SELECT * FROM accounts where account_id = ?";
$stmt = $this->connection->prepare($sql);
$stmt->execute(array($userID));
if ($stmt->rowCount() > 0) {
$output = $stmt->fetch();
return [
"email" => $output['account_email'],
"username" => $output['account_username'],
"name" => $output['account_name']
];
}
} catch (PDOException $e) {
array_push($this->errors, "Could not get user information");
}
return null;
}
}You should only store the user_id in the session - nothing more auth related. To verify if the user is logged in you should query your database with this ID.
At the end this part, which is the most important one, is missing. How you verify that the user is logged in or not.
Thanks I appreciate it!
So Essentially store a "logged in true/false" flag in the database and use the user idea to check that?
Also, the only thing I'm a little confused about with the code is:
public function getUserData($userID): ?arrayNo flag at all - you store the user ID in the session $_SESSION['user_id'] = $user['id']. If the user comes back and you verify the user logged in state you take the ID that you've stored in the session and query your DB if an user with given ID exists.
The ?array says that this method will return an array or null. The ? is a nullable type hint introduced in PHP7.1
If you want to see a battle proven solution in full OOP you should check out the Laravel Session Auth process.
Login process:
The Laravel example gives you also some insights and ideas how to split your OOP code and get near SRP. So the differentiation of request/form data, session, guard, user model, middleware and so on. If your project grows it gets more and more important to have splitted code/classes to call them independently and also replace single parts without updating all usage lines. Like the session or guard driver. That's what Laravel uses interfaces, service container and manager driver pattern for.
Just to get it straight in my head, I store the user id in the session and redirect them to the "logged in" page, then if they come back and the session still had a user ID I check the user id is valid and redirect to the logged in page if it is. Then I guess I set an expiry time?
Sorry if the questions quite simple I just wanna make sure I understand correctly before I move on, no good learning something half heartedly!
