I'm relatively new to PHP and programming as whole. I'm sure my code could be better in a lot of ways, most of which I can't fully grasp at the moment. As it stands I'm trying to avoid the general noobie mistakes(whatever they may be). And also trying to code better and cleaner so that other people can understand without too much hassle.
This is a simple login system I've been working on for the past few days.
Be harsh, please, point out as many flaws as you can. And, if possible, offer solutions to fix said flaws.
The general area of interest would be the classes.
Login class:
<?php
/**
* Handles the login of the user
*/
/**
* TODO: Redesign this class. It's too bloated with functions and does waaay too many things for just one class!
*/
class Login
{
public function __construct(){
session_start();
/**
* TODO: Add remember me cookie
* uncomment this when it's done
if(isset($_POST['rememberMe'])){
$_COOKIE['rememberMe'] = TRUE;
}
*/
/**
* If a session exists
* Just redirect user to logged_in page.
* Else, do login
*/
if((isset($_SESSION['id'])) && (isset($_SESSION['username']))){
header('Location: logged_in.php');
}
if(isset($_POST['login'])){
$this->doLogin();
}
}
/**
* log in with post data
*/
private function doLogin(){
if( ! empty($_POST['username']) &&
! empty($_POST['password'])){
$username = ($_POST['username']);
$password = ($_POST['password']);
require_once 'db/db_connect.php';
require_once 'db/db_tables.php';
/**
* PHP Version 5.4.31 doesn't support password_hash()
* so an extension called password_compat is used
* Link to lib - https://github.com/ircmaxell/password_compat
*/
require_once 'password_compat/lib/password.php';
$sqlQuery = $dbPDO->prepare("SELECT $loginPassword
FROM $tableName
WHERE $loginUsername=:username");
$sqlQuery->execute(array(':username' => $username));
$hash = $sqlQuery->fetch();
$passwordVerify = password_verify($password, $hash['login_password']);
/**
* Note that the variables used here
* come from the db_tables.php file
* not from user input
*/
$sqlQuery = $dbPDO->prepare("SELECT $loginUsername, $loginId FROM $tableName WHERE $loginUsername=:username");
$sqlQuery->execute(array(':username' => $username));
$userVerify = $sqlQuery->fetch();
if(($passwordVerify == 1) && ($userVerify['login_username'] == $username)){
/**
* Great, the user's logged in
* Time to set the session and redirect him
*/
$_SESSION['id'] = $userVerify['login_id'];
$_SESSION['username'] = $userVerify['login_username'];
header('Location: logged_in.php');
} else {
echo '<p> The username or password do not match any registered users.</p>';
}
} else {
echo '<p> You must fill in all fields.</p>';
}
}
}
Register Class:
<?php
/**
* Handles the register of the user
*/
/**
* TODO: Redesign this class. It's too bloated with functions and does waaay too many things for just one class!
*/
class Register
{
private $errors = array();
private $username = NULL;
private $password = NULL;
public function __construct(){
require_once 'forms/register_form.php';
if(isset($_POST['register'])){
$this->doRegister();
}
}
/**
* @bool
*/
private function isEmailValid($email){
if (filter_var(trim($email), FILTER_VALIDATE_EMAIL)){
return true;
}
return false;
}
/**
* @bool regex match
* The username must contain alphanumeric chars
* and must be between 5 and 20 chars long
* Example of valid username: thebrownfox
*/
private function isUsernameValid($username){
return (preg_match('/^[a-z\d_]{5,20}$/i', trim($username)));
}
/**
* @bool regex match
* The password requires at least one uppercase latter, at least one lower case letter
* at least one number and at least any of the following characters: ! @ # $
* Length must be between 5 and 40 characters long.
* Example of valid password: Thequickbrown200!
*/
private function isPasswordValid($password){
return (preg_match('/^(?=.*\d)(?=.*[A-Za-z])[0-9A-Za-z!@#$%]{5,40}$/', trim($password)));
}
/**
* Here we register the user
* Add him to the database etc etc
*/
private function doRegister(){
if( ! empty($_POST['username']) &&
! empty($_POST['password']) &&
! empty($_POST['email']) &&
! empty($_POST['passwordAgain'])){
/**
* @bool
*/
if($this->isUsernameValid($_POST['username'])){
$username = trim($_POST['username']);
} else {
$errors[] = '<p>The username must be between 5 and 20 characters long. And it can only contain numbers and letters!</p>';
}
/**
* @bool
*/
if($this->isEmailValid($_POST['email'])){
$email = trim($_POST['email']);
} else {
$errors[] = '<p>Email is invalid</p>';
}
/**
* Check weather the password is valid
*/
if($this->isPasswordValid($_POST['password'])){
$password = trim($_POST['password']);
$passwordAgain = trim($_POST['passwordAgain']);
if($password != $passwordAgain){
$errors[] = '<p>The passwords do not match.<p>';
}
/**
* PHP Version 5.4.31 doesn't support password_hash()
* so I used an extension called password_compat
* Link to lib - https://github.com/ircmaxell/password_compat
*/
require 'password_compat/lib/password.php';
$password = password_hash($password, PASSWORD_BCRYPT);
} else {
$errors[] = '<p>The password must be between 5 and 40 characters long, must contain at least one number, at least one letter and at least one non Alphanumeric character.</p>';
}
} else {
$errors[] = '<p>Some fields are empty.</p>';
}
/**
* Check if there are any errors so far.
* I can't check after the queries because if some fields
* are empty PHP would slap me with an error
* Also this is bad coding, i need to split up this class
*/
if (!empty($errors)) {
foreach($errors as $error){
echo $error;
}
return;
}
require_once 'db/db_connect.php';
require_once 'db/db_tables.php';
/**
* The queries below are repetitive
* in order to display an error for
* username or email
* I think I could marked the columns in the DB
* as Unique to make things easier however I didn't get around to trying that
* and I'm not sure if it works
*/
$sqlQuery = $dbPDO->prepare("SELECT $loginUsername FROM $tableName WHERE $loginUsername=:username");
$sqlQuery->execute(array(':username' => $username));
$doesUsernameExist = $sqlQuery->fetch(PDO::FETCH_ASSOC);
if($doesUsernameExist['login_username'] == $username){
$errors[] = '<p>That username is already registered!</p>';
}
$sqlQuery = $dbPDO->prepare("SELECT $loginEmail FROM $tableName WHERE $loginEmail=:email");
$sqlQuery->execute(array(':email' => $email));
$doesEmailExist = $sqlQuery->fetch(PDO::FETCH_ASSOC);
/**
* Does email already exist?
*/
if($doesEmailExist['login_email'] == $email){
$errors[] = '<p>That email is already registered!</p>';
}
/**
* This foreach below is bad coding
* I need to split up my code into several
* classes, this class is doing too much
*/
if (!empty($errors)) {
foreach($errors as $error){
echo $error;
}
return;
}
/**
* Here we actually register the user
* He passed all the checks and we can safely
* insert him into the database
*/
$sqlQuery = $dbPDO->prepare("INSERT INTO $tableName($loginUsername, $loginPassword, $loginEmail)
VALUES (:username, :password, :email)");
if($sqlQuery->execute(array(':username' => $username,
':password' => $password,
':email' => $email))){
echo '<p>Registration successful!</p>';
} else {
die('<p> There was an error in the registration process.</p>');
}
}
}
Recover Password Class:
<?php
/**
* Handles the password recovery
*/
/**
* TODO: Redesign this class. It's too bloated with functions and does waaay too many things for just one class!
*/
class RecoverPassword
{
public function __construct(){
if(isset($_POST['recoverPassword'])){
$this->doRecover();
}
}
/**
* @bool
* Note: This has some flaws
* For example using numbers in the name of the email MAY give back some false negatives
* For more details see this question: https://stackoverflow.com/questions/3722831/does-phps-filter-var-filter-validate-email-actually-work
*/
private function isEmailValid($email){
if (filter_var(trim($email), FILTER_VALIDATE_EMAIL)){
return true;
}
return false;
}
private function doRecover(){
if( ! empty($_POST['email'])){
if($this->isEmailValid($_POST['email'])){
$email = $_POST['email'];
require_once 'db/db_connect.php';
require_once 'db/db_tables.php';
/**
* Note that the variables used here
* come from the db_tables.php file
* not from user input
*/
$sqlQuery = $dbPDO->prepare("SELECT $loginEmail FROM $tableName WHERE $loginEmail= :email");
$sqlQuery->execute(array(':email' => $email));
$doesEmailExist = $sqlQuery->fetch(PDO::FETCH_ASSOC);
/**
* Does the email exist?
*/
if($doesEmailExist){
//echo $doesEmailExist['login_email'];
echo '<p>The email exists. Currently the recover password email will not send due to not having a proper SMPT configured.</p>';
/**
* TODO: Make this work. Configure SMTP server -> SwitfMail
* Swift Mailer is downloaded and configured now.
* Uncomment this when I get a chance to test
* require_once 'SendMailRecoverPassword.php';
* $swift = new RecoverPasswordFunctions();
*/
} else {
echo '<p>Email doesn\'t exist!</p>';
}
} else {
echo '<p>Email is invalid</p>';
}
} else {
echo ('<p>Email field is empty!</p>');
}
}
}
Change Password Class:
<?php
/**
* Handles the password change
*/
/**
* TODO: Redesign this class. It's too bloated with functions and does waaay too many things for just one class!
*/
class ChangePassword
{
/**
* Instead of echoing each error on it's own
* the errors are added to an array and iterate
* thru the array before doing the updating
* in the database. If the array is not empty
* return to end the function essentially killing the script
*/
private $errors = array();
public function __construct(){
if(isset($_POST['resetPassword'])){
$this->doResetPassword();
}
}
private function doResetPassword(){
/**
* Here nothing "interesting"
* happens. Just some checks to make sure the user entered the right
* information into the fields. If not add said errors to previously
* mentioned array.
*/
if(empty($_POST['passwordCurrent']) &&
empty($_POST['passwordNew']) &&
empty($_POST['passwordNewAgain'])){
$errors[] = '<p>Some fields are empty</p>';
} else {
$passwordCurrent = trim($_POST['passwordCurrent']);
$passwordNew = trim($_POST['passwordNew']);
$passwordNewAgain = trim($_POST['passwordNewAgain']);
if($passwordNew != $passwordNewAgain){
$errors[] = '<p>Passwords do not match.</p>';
}
/**
* See further down some notes about
* this function
*/
if( ! $this->isPasswordValid($passwordNew)){
$errors[] = '<p>The password must be between 5 and 40 characters long, must contain at least one number, at least one letter and at least one non Alphanumeric character.</p>';
}
if($passwordNew == $passwordCurrent){
$errors[] = '<p>Your new password cannot be the same as your old password.</p>';
}
}
/**
* If this array of errors is empty the user passed all the checks.
* If not, a return happens, ending the script
*/
if( ! empty($errors)){
foreach($errors as $error){
echo $error;
}
return;
}
/**
* Here the user passed the checks
* It's safe to update the database
* with the new password
*/
$this->insertPassword($passwordNew);
$_SESSION = array();
session_destroy();
}
private function insertPassword($passwordNew){
/**
* Go up one directory.
* Conncatenate this var with paths from certain files
*/
$upOneDir = realpath(__DIR__ . '/..');
require_once $upOneDir . '/db/db_connect.php';
require_once $upOneDir . '/db/db_tables.php';
/**
* PHP Version 5.4.31 doesn't support password_hash()
* so I used an extension called password_compat
* Link to lib - https://github.com/ircmaxell/password_compat
*/
require $upOneDir . '/password_compat/lib/password.php';
/**
* NOTE: Hash strength is set to default(10)
*/
$password = password_hash($passwordNew, PASSWORD_BCRYPT);
/**
* The username ID is dependent on the Session id
* which is set to the username ID - in the database
* you can find the username ID as equal to login_id
* each ID is unique and each username is unique
*/
$usernameId = $_SESSION['id'];
$sqlQuery = $dbPDO->prepare("UPDATE $tableName SET $loginPassword=:passwordNew WHERE $loginId=:usernameId");
$result = $sqlQuery->execute(array(':passwordNew' => $password,
':usernameId' => $usernameId));
if($result){
echo '<p>Successfully changed the password</p>';
} else {
echo '<p>An error occurred while changing the password</p>';
}
}
/**
* @bool regex match
* The password requires at least one uppercase letter, at least one lower case letter
* at least one number and at least any of the following characters: ! @ # $
* Length must be between 5 and 40 characters long.
* Example of valid password: Thequickbrown200!
*/
private function isPasswordValid($password){
return (preg_match('/^(?=.*\d)(?=.*[A-Za-z])[0-9A-Za-z!@#$%]{5,40}$/', trim($password)));
}
}
Before anybody points it out:
The classes do way to many things. I realize that. In fact some kind enough people have pointed it out. I should redesign them and separate the responsibilities. But as it stands I don't know how to properly do that. It's more of "thinking"(if that makes sense) problem rather than a coding problem.
My comments may not be the best they can be. Again, I don't know any better as it stands. And I went with the idea that some comments are better than none.
Totally off-topic question. Feel free to ignore it completely, I honestly just don't know where else to ask. In about 3 days I'll be interviewing for a Junior PHP dev position(yeah, I know, I don't believe it either).
Judging purely by this code, where do I stand?
I realize this question is so open for interpretation and skewed by each persons point of view, that it doesn't even have a close-to-accurate answer. Hench the reason I suggested that it be ignored if it's too vague.