User class: Getting user data, Loggin in, secure CSRF session handling, Logout - A class working example...
2
I wrote this class a few months ago and noticed from a few examples that it's better to break down these classes and separate them.
I am not so sure what is the proper way to break if to parts.
It currently includes a creation of a System_user obj based on user id (fetching user data), login validation, logout, storing user data to session(more specifically CSRF), and I think that's all..
This is my working code:
<?php
namespace MyAppModels;
use Exception;
use MyAppCoreDatabase;
use MyAppCoreConfig;
use MyAppHelpersSession;
use MyAppHelpersCookie;
use MyAppHelpersToken;
use MyAppHelpersGeneral;
use MyAppHelpersHash;
/**
*
* System User Class
*
*/
class System_user
{
/*=================================
= Variables =
=================================*/
# @object database Database instance
private $db;
# Users data
private $data;
# User user ID name
public $user_id;
# User first name
public $first_name;
# User last name
public $last_name;
# Username
public $user_name;
# User Email
public $email;
# User Last logged in
public $last_login;
# is user logged in
public $isLoggedIn;
# is user logged in
public $login_timestamp;
# is user IP
private $user_ip;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct($system_user = NULL)
{
# Get database instance
$this->db = Database::getInstance();
# If system_user isn't passed as a variable
if ( !$system_user ) {
# ...so check if there is a session user id set
if (Session::exists(Config::$session_name)) {
# Insert session data to system_user variable
$system_user = Session::get(Config::$session_name);
# Get user data
$this->find($system_user);
}
} else {
$this->find($system_user);
}
}
/**
*
* Find method: Find user by id or by username
* @param $user String/Init A username or user ID
*
*/
public function find($system_user = NULL)
{
if ($system_user) {
// Enable search for a system_user by a string name or if numeric - so by id.
$field = ( is_numeric($system_user) ) ? 'system_user_id' : 'uname';
// Search for the system_user in the Database 'system_users' table.
$data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $system_user));
// If there is a result
if ( $data ) {
// Set data
$this->setUserData($data);
return $this;
} else {
return false;
}
}
else{
return false;
}
}
/**
*
* Check if user exist in 'system_users' table
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Array/Boolian Is this a signed System user?
*
*/
private function system_user_login_validation($username, $password)
{
$user_data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));
if ($user_data)
return $user_data;
else
return false;
}
/**
*
* Login method
* @param $customer_name String Get a customer_name user input
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Boolian Is this a signed System user?
*
*/
public function login($customer_name, $username, $password)
{
# Create a Customer Obj
$customer = new MyAppModelsCustomer($customer_name);
try {
# Check if the result is an array
# OR there is no row result:
if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
throw new MyAppCoreExceptionHandlerLoginException("Bad company name: {$customer_name}");
# Change localhost string to 127.0.0.1 (prevent dns lookup)
$customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;
# Connect to new database
$new_connection = $this->db->customer_connect($customer->host, $customer->dbName);
# If status is connected
if ($new_connection) {
# Check for user credentials data
$user_data = $this->system_user_login_validation($username, $password);
# If the result isn't a valid array - EXEPTION
if ( (!is_array($user_data)) || (empty($user_data)) )
throw new MyAppCoreExceptionHandlerLoginException("Customer: '{$customer_name}' - Invalid username ({$username}) or password ({$password})");
# Store Customer in the sesison
Session::put(Config::$customer, serialize($customer));
# Update host and db for the db object
# $this->db->update_host_and_db($customer->host, $customer->dbName);
# Set data for this System_user object
$this->setUserData($user_data);
# Set a login session for the user id:
Session::put(Config::$session_name, $this->user_id);
# Set logged in user sessions
$this->set_loggedin_user_sessions();
return $this;
} else {
# Connect back to backoffice (current db set)
$this->db->connect_to_current_set_db();
throw new MyAppCoreExceptionHandlerLoginException('User does not exist');
return false;
}
} catch (MyAppCoreExceptionHandlerLoginException $e) {
$e->log($e);
return false;
// die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
}
}
/**
*
* Set sessions for the logged in user.
* Tutorial: http://forums.devshed.com/php-faqs-stickies/953373-php-sessions-secure-post2921620.html
*
*/
public function set_loggedin_user_sessions()
{
# Generate security sessions
$this->generate_security_sessions();
# Set login timestamp
Session::put(Config::$login_timestamp, $this->login_timestamp);
# Set login flag to true
Session::put(Config::$is_logged_in, true);
# Set login IP
Session::put(Config::$login_user_ip, $this->user_ip);
}
/**
*
* Generate system user security sessions
* @param $new_session Boolean (optinal) Dedices if to delete the cookie session id [default is set to true]
*
*/
public function generate_security_sessions($new_session = true)
{
if ($new_session)
# Generate a new session ID
session_regenerate_id(true);
# Fetch cookie session ID
$session_id = session_id();
# Set the session id to the session
Session::put(Config::$session_id, $session_id);
# Create a secret token
# Set it in session (does them both)
$secret = Token::generate_login_token();
# Combine secret and session_id and create a hash
$combined = Hash::make_from_array(array($secret, $session_id, $this->user_ip));
# Add combined to session
Session::put(Config::$combined, $combined);
}
/**
*
* Check if there is a logged in user
*
*/
public function check_logged_in()
{
if ( Session::exists(Config::$secret) && # Secret session exists
Session::exists(Config::$session_id) && # Session_id session exists
Session::exists(Config::$session_name) && # User session exists
Session::exists(Config::$is_logged_in) && # Check if 'logged in' session exists
Session::exists(Config::$session_name) # Check if sys_user id is set in session
)
{
# Get users ip
$ip = $this->get_system_user_ip();
# if the saved bombined session
if (
(Session::get(Config::$combined) === Hash::make_from_array(array(Session::get(Config::$secret), session_id()), $ip)) &&
(Session::get(Config::$is_logged_in) === true )
)
{
# Set ip to system user object
$this->user_ip = $ip;
return true;
} else {
return false;
}
}
else {
return false;
}
}
/**
*
* Check if loggin session is timeout
*
*/
public function check_timeout()
{
if (Session::exists(Config::$login_timestamp)){
# Calculate time
$session_lifetime_seconds = time() - Session::get(Config::$login_timestamp) ;
if ($session_lifetime_seconds > Config::MAX_TIME){
$this->logout();
return true;
} else {
return false;
}
} else {
$this->logout();
return false;
}
}
/**
*
* Get user IP
*
*/
private function get_system_user_ip()
{
if (!empty($_SERVER['HTTP_CLIENT_IP']))
$ip = $_SERVER['HTTP_CLIENT_IP'];
elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
else
$ip = $_SERVER['REMOTE_ADDR'];
return $ip;
}
/**
*
* Set User data to (this) System_user object
* @param $user_data Array User data fetched from the db (usually by the find method)
*
*/
private function setUserData($user_data)
{
// Set data for this user object
$this->user_id = $user_data['system_user_id'];
$this->first_name = $user_data['fname'];
$this->last_name = $user_data['lname'];
$this->user_name = $user_data['uname'];
$this->email = $user_data['email'];
$this->last_login = $user_data['last_login'];
$this->isLoggedIn = true;
$this->user_ip = $this->get_system_user_ip();
$this->login_timestamp = time();
}
/**
*
* Logout: Now guess what this method does..
*
*/
public function logout()
{
$this->isLoggedIn = false;
Cookie::eat_cookies();
Session::kill_session();
session_destroy();
session_write_close();
}
}
I would like to get suggestions about my current code, and if possible, about structuring it differently with more than one class. (class SystemUser, class systemUserLogin, class systemUserAuthenticator, ect')
ps: In general, the webapp by default logs in to a general database. when a user inserts his company_name, username and password, I check if the company name actually exist, if if does, I disconnect from the general db and connect to the customers database and validate his username & password.
This is the new class I started writing (not tested, so I cant assure this is a working code) with more classes following this example & inspired by this post I found, while tring to follow the SOLID principals and PSR standards, focusing on the structure and architecture.
<?php
namespace MyAppModels;
use MyAppCoreConfig;
use MyAppHelpersSession;
use MyAppCoreDatabase;
/**
*
* System User Class
*
*/
class SystemUser
{
/*=================================
= Variables =
=================================*/
# @obj SystemUser profile information (fullname, profile picture... etc')
protected $systemUserDetatils;
# @obj SystemUser Login data
protected $systemUserLogin;
# @obj SystemUser Authenticator
protected $systemUserAuthenticator;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct($systemUserId = NULL)
{
# If system_user passed
if ( $systemUserId ) {
# Create systemUserDedatils obj
$this->systemUserDetatils = new MyAppModelsSystemUserSystemUserDetatils();
# Get SysUser data
$this->systemUserDetatils->get($systemUserId);
} else {
# Check for sysUser id in the session:
$systemUserId = $this->systemUserDetatils->getUserFromSession();
# Get user data from session
if ( $systemUserId ) {
# Create systemUserDedatils obj
$this->systemUserDetatils = new MyAppModelsSystemUserSystemUserDetatils();
# Get SysUser data
$this->systemUserDetatils->get($systemUserId);
}
}
}
/**
*
* Set Login: Sets the SystemUserLogin object to $systemUserLogin variable
* @param $_systemUserLogin SystemUserLogin Gets a SystemUserLogin object
*
*/
public function setSystemUserLogin(SystemUserLogin $_systemUserLogin)
{
$this->systemUserLogin = $_systemUserLogin;
}
/**
*
* Login
*
*/
public function login()
{
$this->systemUserAuthenticator($this);
}
}
<?php
namespace MyAppModelsSystemUser;
use MyAppCoreConfig;
use MyAppHelpersSession;
/**
*
* System User Details Class
*
*/
class SystemUserDetails
{
/*=================================
= Variables =
=================================*/
# @object database Database instance
private $db;
# Users data
private $data;
# User user ID name
public $userId;
# User first name
public $firstName;
# User last name
public $lastName;
# Username
public $userName;
# User Email
public $email;
# User Last logged in
public $lastLogin;
/*# is user logged in
public $isLoggedIn;
# is user logged in
public $login_timestamp;*/
# is user IP
private $user_ip;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct()
{
# Get database instance
$this->db = Database::getInstance();
}
/**
*
* Find method: Find user by id or by username
* @param $user String / Init A username or user ID
* @return
*
*/
public function get(Int $systemUserId)
{
if ($systemUserId) {
# Enable search for a system_user by a string name or if numeric - so by id.
$field = ( is_numeric($systemUserId) ) ? 'system_user_id' : 'uname';
# Search for the system_user in the Database 'system_users' table.
$data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $systemUserId));
# If there is a result
if ( $data ) {
# Set data
$this->setUserData($data);
return $this;
} else {
return false;
}
}
else {
return false;
}
}
/**
*
* Set User data to $this obj
* @param $userData Array User data fetched from the db (usually by the find method)
* @return
*
*/
public function set(Array $userData)
{
// Set data for this user object
$this->userId = $userData['system_user_id'];
$this->firstName = $userData['fname'];
$this->lastName = $userData['lname'];
$this->userName = $userData['uname'];
$this->email = $userData['email'];
$this->lastLogin = $userData['last_login'];
}
/**
*
* Get User from session
* @param
* @return
*
*/
public function getUserFromSession()
{
# Check if there is a session user id set
if (Session::exists(Config::$session_name)) {
# Insert session data to system_user variable
return Session::get(Config::$session_name);
} else {
# Returning false cause there is no user id session
return false;
}
}
}
<?php
namespace MyAppModelsSystemUser;
/**
*
* System User Details Class
*
*/
class systemUserLogin
{
/*=================================
= Variables =
=================================*/
# @str Customer name
public $customerName;
# @str UserName
public $userName;
# @str Password
public $password;
# @str user IP
public $userIp;
/*===============================
= Methods =
================================*/
/**
*
* Construct - Set customer, username and password
* @param $_customerName String
* @param $_userName String
* @param $_password String
*
*/
public function __construct(String $_customerName, String $_userName, String $_password)
{
$this->customerName = $_customerName;
$this->userName = $_userName;
$this->password = $_password;
$this->userIp = $this->getSystemUserIp();
}
/**
*
* Get user IP
* @return String Returns the user IP that is trying to connect.
*
*/
private function getSystemUserIp()
{
if (!empty($_SERVER['HTTP_CLIENT_IP']))
$ip = $_SERVER['HTTP_CLIENT_IP'];
elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
else
$ip = $_SERVER['REMOTE_ADDR'];
return $ip;
}
}
<?php
namespace MyAppModelsSystemUser;
/**
*
* System User Details Class
*
*/
class systemUserAuthenticator
{
/*=================================
= Variables =
=================================*/
# @object Database instance
private $db;
# @bool Is logged in
public $isLoggedIn = false;
# @str Login Timestamp
public $loginTimestamp;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct()
{
# Get database instance
$this->db = Database::getInstance();
}
/**
*
* Login method
* @param $customer_name String Get a customer_name user input
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Boolian Is this a signed System user?
*
*/
public function login(User $user)
{
# Create a Customer Obj
$customer = new MyAppModelsCustomer($user->SystemUserLogin->customerName);
try {
# Check if the result is an array
# OR there is no row result:
if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
throw new MyAppCoreExceptionHandlerLoginException("Bad company name: {$user->SystemUserLogin->customerName}");
# Change localhost string to 127.0.0.1 (prevent dns lookup)
$customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;
# Connect to new database
$new_connection = $this->db->customer_connect($customer->host, $customer->dbName);
# If status is connected
if ($new_connection) {
# Check for user credentials data
$user_data = $this->system_user_login_validation($user->SystemUserLogin->userName, $user->SystemUserLogin->password);
# If the result isn't a valid array - EXEPTION
if ( (!is_array($user_data)) || (empty($user_data)) )
throw new MyAppCoreExceptionHandlerLoginException("Customer: '{$user->SystemUserLogin->customerName}' - Invalid username ({$user->SystemUserLogin->userName}) or password ({$user->SystemUserLogin->password})");
# Store Customer in the sesison
Session::put(Config::$customer, serialize($customer));
# Update host and db for the db object
# $this->db->update_host_and_db($customer->host, $customer->dbName);
# Set data for this System_user object
$this->setUserData($user_data);
# Set a login session for the user id:
Session::put(Config::$session_name, $this->user_id);
# Set logged in user sessions
$this->set_loggedin_user_sessions();
return $this;
} else {
# Connect back to backoffice (current db set)
$this->db->connect_to_current_set_db();
throw new MyAppCoreExceptionHandlerLoginException('User does not exist');
return false;
}
} catch (MyAppCoreExceptionHandlerLoginException $e) {
$e->log($e);
return false;
// die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
}
}
/**
*
* Check if user exist in 'system_users' table
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Array/Boolian Is this a signed System user?
*
*/
private function systemUserLoginValidation($username, $password)
{
$userData = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));
if ($userData)
return $userData;
else
return false;
}
}
php object-oriented design-patterns
asked 2 days ago
potatoguypotatoguy
375
add a comment |
2
I wrote this class a few months ago and noticed from a few examples that it's better to break down these classes and separate them.
I am not so sure what is the proper way to break if to parts.
It currently includes a creation of a System_user obj based on user id (fetching user data), login validation, logout, storing user data to session(more specifically CSRF), and I think that's all..
This is my working code:
<?php
namespace MyAppModels;
use Exception;
use MyAppCoreDatabase;
use MyAppCoreConfig;
use MyAppHelpersSession;
use MyAppHelpersCookie;
use MyAppHelpersToken;
use MyAppHelpersGeneral;
use MyAppHelpersHash;
/**
*
* System User Class
*
*/
class System_user
{
/*=================================
= Variables =
=================================*/
# @object database Database instance
private $db;
# Users data
private $data;
# User user ID name
public $user_id;
# User first name
public $first_name;
# User last name
public $last_name;
# Username
public $user_name;
# User Email
public $email;
# User Last logged in
public $last_login;
# is user logged in
public $isLoggedIn;
# is user logged in
public $login_timestamp;
# is user IP
private $user_ip;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct($system_user = NULL)
{
# Get database instance
$this->db = Database::getInstance();
# If system_user isn't passed as a variable
if ( !$system_user ) {
# ...so check if there is a session user id set
if (Session::exists(Config::$session_name)) {
# Insert session data to system_user variable
$system_user = Session::get(Config::$session_name);
# Get user data
$this->find($system_user);
}
} else {
$this->find($system_user);
}
}
/**
*
* Find method: Find user by id or by username
* @param $user String/Init A username or user ID
*
*/
public function find($system_user = NULL)
{
if ($system_user) {
// Enable search for a system_user by a string name or if numeric - so by id.
$field = ( is_numeric($system_user) ) ? 'system_user_id' : 'uname';
// Search for the system_user in the Database 'system_users' table.
$data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $system_user));
// If there is a result
if ( $data ) {
// Set data
$this->setUserData($data);
return $this;
} else {
return false;
}
}
else{
return false;
}
}
/**
*
* Check if user exist in 'system_users' table
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Array/Boolian Is this a signed System user?
*
*/
private function system_user_login_validation($username, $password)
{
$user_data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));
if ($user_data)
return $user_data;
else
return false;
}
/**
*
* Login method
* @param $customer_name String Get a customer_name user input
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Boolian Is this a signed System user?
*
*/
public function login($customer_name, $username, $password)
{
# Create a Customer Obj
$customer = new MyAppModelsCustomer($customer_name);
try {
# Check if the result is an array
# OR there is no row result:
if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
throw new MyAppCoreExceptionHandlerLoginException("Bad company name: {$customer_name}");
# Change localhost string to 127.0.0.1 (prevent dns lookup)
$customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;
# Connect to new database
$new_connection = $this->db->customer_connect($customer->host, $customer->dbName);
# If status is connected
if ($new_connection) {
# Check for user credentials data
$user_data = $this->system_user_login_validation($username, $password);
# If the result isn't a valid array - EXEPTION
if ( (!is_array($user_data)) || (empty($user_data)) )
throw new MyAppCoreExceptionHandlerLoginException("Customer: '{$customer_name}' - Invalid username ({$username}) or password ({$password})");
# Store Customer in the sesison
Session::put(Config::$customer, serialize($customer));
# Update host and db for the db object
# $this->db->update_host_and_db($customer->host, $customer->dbName);
# Set data for this System_user object
$this->setUserData($user_data);
# Set a login session for the user id:
Session::put(Config::$session_name, $this->user_id);
# Set logged in user sessions
$this->set_loggedin_user_sessions();
return $this;
} else {
# Connect back to backoffice (current db set)
$this->db->connect_to_current_set_db();
throw new MyAppCoreExceptionHandlerLoginException('User does not exist');
return false;
}
} catch (MyAppCoreExceptionHandlerLoginException $e) {
$e->log($e);
return false;
// die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
}
}
/**
*
* Set sessions for the logged in user.
* Tutorial: http://forums.devshed.com/php-faqs-stickies/953373-php-sessions-secure-post2921620.html
*
*/
public function set_loggedin_user_sessions()
{
# Generate security sessions
$this->generate_security_sessions();
# Set login timestamp
Session::put(Config::$login_timestamp, $this->login_timestamp);
# Set login flag to true
Session::put(Config::$is_logged_in, true);
# Set login IP
Session::put(Config::$login_user_ip, $this->user_ip);
}
/**
*
* Generate system user security sessions
* @param $new_session Boolean (optinal) Dedices if to delete the cookie session id [default is set to true]
*
*/
public function generate_security_sessions($new_session = true)
{
if ($new_session)
# Generate a new session ID
session_regenerate_id(true);
# Fetch cookie session ID
$session_id = session_id();
# Set the session id to the session
Session::put(Config::$session_id, $session_id);
# Create a secret token
# Set it in session (does them both)
$secret = Token::generate_login_token();
# Combine secret and session_id and create a hash
$combined = Hash::make_from_array(array($secret, $session_id, $this->user_ip));
# Add combined to session
Session::put(Config::$combined, $combined);
}
/**
*
* Check if there is a logged in user
*
*/
public function check_logged_in()
{
if ( Session::exists(Config::$secret) && # Secret session exists
Session::exists(Config::$session_id) && # Session_id session exists
Session::exists(Config::$session_name) && # User session exists
Session::exists(Config::$is_logged_in) && # Check if 'logged in' session exists
Session::exists(Config::$session_name) # Check if sys_user id is set in session
)
{
# Get users ip
$ip = $this->get_system_user_ip();
# if the saved bombined session
if (
(Session::get(Config::$combined) === Hash::make_from_array(array(Session::get(Config::$secret), session_id()), $ip)) &&
(Session::get(Config::$is_logged_in) === true )
)
{
# Set ip to system user object
$this->user_ip = $ip;
return true;
} else {
return false;
}
}
else {
return false;
}
}
/**
*
* Check if loggin session is timeout
*
*/
public function check_timeout()
{
if (Session::exists(Config::$login_timestamp)){
# Calculate time
$session_lifetime_seconds = time() - Session::get(Config::$login_timestamp) ;
if ($session_lifetime_seconds > Config::MAX_TIME){
$this->logout();
return true;
} else {
return false;
}
} else {
$this->logout();
return false;
}
}
/**
*
* Get user IP
*
*/
private function get_system_user_ip()
{
if (!empty($_SERVER['HTTP_CLIENT_IP']))
$ip = $_SERVER['HTTP_CLIENT_IP'];
elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
else
$ip = $_SERVER['REMOTE_ADDR'];
return $ip;
}
/**
*
* Set User data to (this) System_user object
* @param $user_data Array User data fetched from the db (usually by the find method)
*
*/
private function setUserData($user_data)
{
// Set data for this user object
$this->user_id = $user_data['system_user_id'];
$this->first_name = $user_data['fname'];
$this->last_name = $user_data['lname'];
$this->user_name = $user_data['uname'];
$this->email = $user_data['email'];
$this->last_login = $user_data['last_login'];
$this->isLoggedIn = true;
$this->user_ip = $this->get_system_user_ip();
$this->login_timestamp = time();
}
/**
*
* Logout: Now guess what this method does..
*
*/
public function logout()
{
$this->isLoggedIn = false;
Cookie::eat_cookies();
Session::kill_session();
session_destroy();
session_write_close();
}
}
I would like to get suggestions about my current code, and if possible, about structuring it differently with more than one class. (class SystemUser, class systemUserLogin, class systemUserAuthenticator, ect')
ps: In general, the webapp by default logs in to a general database. when a user inserts his company_name, username and password, I check if the company name actually exist, if if does, I disconnect from the general db and connect to the customers database and validate his username & password.
This is the new class I started writing (not tested, so I cant assure this is a working code) with more classes following this example & inspired by this post I found, while tring to follow the SOLID principals and PSR standards, focusing on the structure and architecture.
<?php
namespace MyAppModels;
use MyAppCoreConfig;
use MyAppHelpersSession;
use MyAppCoreDatabase;
/**
*
* System User Class
*
*/
class SystemUser
{
/*=================================
= Variables =
=================================*/
# @obj SystemUser profile information (fullname, profile picture... etc')
protected $systemUserDetatils;
# @obj SystemUser Login data
protected $systemUserLogin;
# @obj SystemUser Authenticator
protected $systemUserAuthenticator;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct($systemUserId = NULL)
{
# If system_user passed
if ( $systemUserId ) {
# Create systemUserDedatils obj
$this->systemUserDetatils = new MyAppModelsSystemUserSystemUserDetatils();
# Get SysUser data
$this->systemUserDetatils->get($systemUserId);
} else {
# Check for sysUser id in the session:
$systemUserId = $this->systemUserDetatils->getUserFromSession();
# Get user data from session
if ( $systemUserId ) {
# Create systemUserDedatils obj
$this->systemUserDetatils = new MyAppModelsSystemUserSystemUserDetatils();
# Get SysUser data
$this->systemUserDetatils->get($systemUserId);
}
}
}
/**
*
* Set Login: Sets the SystemUserLogin object to $systemUserLogin variable
* @param $_systemUserLogin SystemUserLogin Gets a SystemUserLogin object
*
*/
public function setSystemUserLogin(SystemUserLogin $_systemUserLogin)
{
$this->systemUserLogin = $_systemUserLogin;
}
/**
*
* Login
*
*/
public function login()
{
$this->systemUserAuthenticator($this);
}
}
<?php
namespace MyAppModelsSystemUser;
use MyAppCoreConfig;
use MyAppHelpersSession;
/**
*
* System User Details Class
*
*/
class SystemUserDetails
{
/*=================================
= Variables =
=================================*/
# @object database Database instance
private $db;
# Users data
private $data;
# User user ID name
public $userId;
# User first name
public $firstName;
# User last name
public $lastName;
# Username
public $userName;
# User Email
public $email;
# User Last logged in
public $lastLogin;
/*# is user logged in
public $isLoggedIn;
# is user logged in
public $login_timestamp;*/
# is user IP
private $user_ip;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct()
{
# Get database instance
$this->db = Database::getInstance();
}
/**
*
* Find method: Find user by id or by username
* @param $user String / Init A username or user ID
* @return
*
*/
public function get(Int $systemUserId)
{
if ($systemUserId) {
# Enable search for a system_user by a string name or if numeric - so by id.
$field = ( is_numeric($systemUserId) ) ? 'system_user_id' : 'uname';
# Search for the system_user in the Database 'system_users' table.
$data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $systemUserId));
# If there is a result
if ( $data ) {
# Set data
$this->setUserData($data);
return $this;
} else {
return false;
}
}
else {
return false;
}
}
/**
*
* Set User data to $this obj
* @param $userData Array User data fetched from the db (usually by the find method)
* @return
*
*/
public function set(Array $userData)
{
// Set data for this user object
$this->userId = $userData['system_user_id'];
$this->firstName = $userData['fname'];
$this->lastName = $userData['lname'];
$this->userName = $userData['uname'];
$this->email = $userData['email'];
$this->lastLogin = $userData['last_login'];
}
/**
*
* Get User from session
* @param
* @return
*
*/
public function getUserFromSession()
{
# Check if there is a session user id set
if (Session::exists(Config::$session_name)) {
# Insert session data to system_user variable
return Session::get(Config::$session_name);
} else {
# Returning false cause there is no user id session
return false;
}
}
}
<?php
namespace MyAppModelsSystemUser;
/**
*
* System User Details Class
*
*/
class systemUserLogin
{
/*=================================
= Variables =
=================================*/
# @str Customer name
public $customerName;
# @str UserName
public $userName;
# @str Password
public $password;
# @str user IP
public $userIp;
/*===============================
= Methods =
================================*/
/**
*
* Construct - Set customer, username and password
* @param $_customerName String
* @param $_userName String
* @param $_password String
*
*/
public function __construct(String $_customerName, String $_userName, String $_password)
{
$this->customerName = $_customerName;
$this->userName = $_userName;
$this->password = $_password;
$this->userIp = $this->getSystemUserIp();
}
/**
*
* Get user IP
* @return String Returns the user IP that is trying to connect.
*
*/
private function getSystemUserIp()
{
if (!empty($_SERVER['HTTP_CLIENT_IP']))
$ip = $_SERVER['HTTP_CLIENT_IP'];
elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
else
$ip = $_SERVER['REMOTE_ADDR'];
return $ip;
}
}
<?php
namespace MyAppModelsSystemUser;
/**
*
* System User Details Class
*
*/
class systemUserAuthenticator
{
/*=================================
= Variables =
=================================*/
# @object Database instance
private $db;
# @bool Is logged in
public $isLoggedIn = false;
# @str Login Timestamp
public $loginTimestamp;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct()
{
# Get database instance
$this->db = Database::getInstance();
}
/**
*
* Login method
* @param $customer_name String Get a customer_name user input
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Boolian Is this a signed System user?
*
*/
public function login(User $user)
{
# Create a Customer Obj
$customer = new MyAppModelsCustomer($user->SystemUserLogin->customerName);
try {
# Check if the result is an array
# OR there is no row result:
if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
throw new MyAppCoreExceptionHandlerLoginException("Bad company name: {$user->SystemUserLogin->customerName}");
# Change localhost string to 127.0.0.1 (prevent dns lookup)
$customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;
# Connect to new database
$new_connection = $this->db->customer_connect($customer->host, $customer->dbName);
# If status is connected
if ($new_connection) {
# Check for user credentials data
$user_data = $this->system_user_login_validation($user->SystemUserLogin->userName, $user->SystemUserLogin->password);
# If the result isn't a valid array - EXEPTION
if ( (!is_array($user_data)) || (empty($user_data)) )
throw new MyAppCoreExceptionHandlerLoginException("Customer: '{$user->SystemUserLogin->customerName}' - Invalid username ({$user->SystemUserLogin->userName}) or password ({$user->SystemUserLogin->password})");
# Store Customer in the sesison
Session::put(Config::$customer, serialize($customer));
# Update host and db for the db object
# $this->db->update_host_and_db($customer->host, $customer->dbName);
# Set data for this System_user object
$this->setUserData($user_data);
# Set a login session for the user id:
Session::put(Config::$session_name, $this->user_id);
# Set logged in user sessions
$this->set_loggedin_user_sessions();
return $this;
} else {
# Connect back to backoffice (current db set)
$this->db->connect_to_current_set_db();
throw new MyAppCoreExceptionHandlerLoginException('User does not exist');
return false;
}
} catch (MyAppCoreExceptionHandlerLoginException $e) {
$e->log($e);
return false;
// die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
}
}
/**
*
* Check if user exist in 'system_users' table
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Array/Boolian Is this a signed System user?
*
*/
private function systemUserLoginValidation($username, $password)
{
$userData = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));
if ($userData)
return $userData;
else
return false;
}
}
php object-oriented design-patterns
asked 2 days ago
potatoguypotatoguy
375
add a comment |
2
2
2
I wrote this class a few months ago and noticed from a few examples that it's better to break down these classes and separate them.
I am not so sure what is the proper way to break if to parts.
It currently includes a creation of a System_user obj based on user id (fetching user data), login validation, logout, storing user data to session(more specifically CSRF), and I think that's all..
This is my working code:
<?php
namespace MyAppModels;
use Exception;
use MyAppCoreDatabase;
use MyAppCoreConfig;
use MyAppHelpersSession;
use MyAppHelpersCookie;
use MyAppHelpersToken;
use MyAppHelpersGeneral;
use MyAppHelpersHash;
/**
*
* System User Class
*
*/
class System_user
{
/*=================================
= Variables =
=================================*/
# @object database Database instance
private $db;
# Users data
private $data;
# User user ID name
public $user_id;
# User first name
public $first_name;
# User last name
public $last_name;
# Username
public $user_name;
# User Email
public $email;
# User Last logged in
public $last_login;
# is user logged in
public $isLoggedIn;
# is user logged in
public $login_timestamp;
# is user IP
private $user_ip;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct($system_user = NULL)
{
# Get database instance
$this->db = Database::getInstance();
# If system_user isn't passed as a variable
if ( !$system_user ) {
# ...so check if there is a session user id set
if (Session::exists(Config::$session_name)) {
# Insert session data to system_user variable
$system_user = Session::get(Config::$session_name);
# Get user data
$this->find($system_user);
}
} else {
$this->find($system_user);
}
}
/**
*
* Find method: Find user by id or by username
* @param $user String/Init A username or user ID
*
*/
public function find($system_user = NULL)
{
if ($system_user) {
// Enable search for a system_user by a string name or if numeric - so by id.
$field = ( is_numeric($system_user) ) ? 'system_user_id' : 'uname';
// Search for the system_user in the Database 'system_users' table.
$data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $system_user));
// If there is a result
if ( $data ) {
// Set data
$this->setUserData($data);
return $this;
} else {
return false;
}
}
else{
return false;
}
}
/**
*
* Check if user exist in 'system_users' table
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Array/Boolian Is this a signed System user?
*
*/
private function system_user_login_validation($username, $password)
{
$user_data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));
if ($user_data)
return $user_data;
else
return false;
}
/**
*
* Login method
* @param $customer_name String Get a customer_name user input
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Boolian Is this a signed System user?
*
*/
public function login($customer_name, $username, $password)
{
# Create a Customer Obj
$customer = new MyAppModelsCustomer($customer_name);
try {
# Check if the result is an array
# OR there is no row result:
if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
throw new MyAppCoreExceptionHandlerLoginException("Bad company name: {$customer_name}");
# Change localhost string to 127.0.0.1 (prevent dns lookup)
$customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;
# Connect to new database
$new_connection = $this->db->customer_connect($customer->host, $customer->dbName);
# If status is connected
if ($new_connection) {
# Check for user credentials data
$user_data = $this->system_user_login_validation($username, $password);
# If the result isn't a valid array - EXEPTION
if ( (!is_array($user_data)) || (empty($user_data)) )
throw new MyAppCoreExceptionHandlerLoginException("Customer: '{$customer_name}' - Invalid username ({$username}) or password ({$password})");
# Store Customer in the sesison
Session::put(Config::$customer, serialize($customer));
# Update host and db for the db object
# $this->db->update_host_and_db($customer->host, $customer->dbName);
# Set data for this System_user object
$this->setUserData($user_data);
# Set a login session for the user id:
Session::put(Config::$session_name, $this->user_id);
# Set logged in user sessions
$this->set_loggedin_user_sessions();
return $this;
} else {
# Connect back to backoffice (current db set)
$this->db->connect_to_current_set_db();
throw new MyAppCoreExceptionHandlerLoginException('User does not exist');
return false;
}
} catch (MyAppCoreExceptionHandlerLoginException $e) {
$e->log($e);
return false;
// die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
}
}
/**
*
* Set sessions for the logged in user.
* Tutorial: http://forums.devshed.com/php-faqs-stickies/953373-php-sessions-secure-post2921620.html
*
*/
public function set_loggedin_user_sessions()
{
# Generate security sessions
$this->generate_security_sessions();
# Set login timestamp
Session::put(Config::$login_timestamp, $this->login_timestamp);
# Set login flag to true
Session::put(Config::$is_logged_in, true);
# Set login IP
Session::put(Config::$login_user_ip, $this->user_ip);
}
/**
*
* Generate system user security sessions
* @param $new_session Boolean (optinal) Dedices if to delete the cookie session id [default is set to true]
*
*/
public function generate_security_sessions($new_session = true)
{
if ($new_session)
# Generate a new session ID
session_regenerate_id(true);
# Fetch cookie session ID
$session_id = session_id();
# Set the session id to the session
Session::put(Config::$session_id, $session_id);
# Create a secret token
# Set it in session (does them both)
$secret = Token::generate_login_token();
# Combine secret and session_id and create a hash
$combined = Hash::make_from_array(array($secret, $session_id, $this->user_ip));
# Add combined to session
Session::put(Config::$combined, $combined);
}
/**
*
* Check if there is a logged in user
*
*/
public function check_logged_in()
{
if ( Session::exists(Config::$secret) && # Secret session exists
Session::exists(Config::$session_id) && # Session_id session exists
Session::exists(Config::$session_name) && # User session exists
Session::exists(Config::$is_logged_in) && # Check if 'logged in' session exists
Session::exists(Config::$session_name) # Check if sys_user id is set in session
)
{
# Get users ip
$ip = $this->get_system_user_ip();
# if the saved bombined session
if (
(Session::get(Config::$combined) === Hash::make_from_array(array(Session::get(Config::$secret), session_id()), $ip)) &&
(Session::get(Config::$is_logged_in) === true )
)
{
# Set ip to system user object
$this->user_ip = $ip;
return true;
} else {
return false;
}
}
else {
return false;
}
}
/**
*
* Check if loggin session is timeout
*
*/
public function check_timeout()
{
if (Session::exists(Config::$login_timestamp)){
# Calculate time
$session_lifetime_seconds = time() - Session::get(Config::$login_timestamp) ;
if ($session_lifetime_seconds > Config::MAX_TIME){
$this->logout();
return true;
} else {
return false;
}
} else {
$this->logout();
return false;
}
}
/**
*
* Get user IP
*
*/
private function get_system_user_ip()
{
if (!empty($_SERVER['HTTP_CLIENT_IP']))
$ip = $_SERVER['HTTP_CLIENT_IP'];
elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
else
$ip = $_SERVER['REMOTE_ADDR'];
return $ip;
}
/**
*
* Set User data to (this) System_user object
* @param $user_data Array User data fetched from the db (usually by the find method)
*
*/
private function setUserData($user_data)
{
// Set data for this user object
$this->user_id = $user_data['system_user_id'];
$this->first_name = $user_data['fname'];
$this->last_name = $user_data['lname'];
$this->user_name = $user_data['uname'];
$this->email = $user_data['email'];
$this->last_login = $user_data['last_login'];
$this->isLoggedIn = true;
$this->user_ip = $this->get_system_user_ip();
$this->login_timestamp = time();
}
/**
*
* Logout: Now guess what this method does..
*
*/
public function logout()
{
$this->isLoggedIn = false;
Cookie::eat_cookies();
Session::kill_session();
session_destroy();
session_write_close();
}
}
I would like to get suggestions about my current code, and if possible, about structuring it differently with more than one class. (class SystemUser, class systemUserLogin, class systemUserAuthenticator, ect')
ps: In general, the webapp by default logs in to a general database. when a user inserts his company_name, username and password, I check if the company name actually exist, if if does, I disconnect from the general db and connect to the customers database and validate his username & password.
This is the new class I started writing (not tested, so I cant assure this is a working code) with more classes following this example & inspired by this post I found, while tring to follow the SOLID principals and PSR standards, focusing on the structure and architecture.
<?php
namespace MyAppModels;
use MyAppCoreConfig;
use MyAppHelpersSession;
use MyAppCoreDatabase;
/**
*
* System User Class
*
*/
class SystemUser
{
/*=================================
= Variables =
=================================*/
# @obj SystemUser profile information (fullname, profile picture... etc')
protected $systemUserDetatils;
# @obj SystemUser Login data
protected $systemUserLogin;
# @obj SystemUser Authenticator
protected $systemUserAuthenticator;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct($systemUserId = NULL)
{
# If system_user passed
if ( $systemUserId ) {
# Create systemUserDedatils obj
$this->systemUserDetatils = new MyAppModelsSystemUserSystemUserDetatils();
# Get SysUser data
$this->systemUserDetatils->get($systemUserId);
} else {
# Check for sysUser id in the session:
$systemUserId = $this->systemUserDetatils->getUserFromSession();
# Get user data from session
if ( $systemUserId ) {
# Create systemUserDedatils obj
$this->systemUserDetatils = new MyAppModelsSystemUserSystemUserDetatils();
# Get SysUser data
$this->systemUserDetatils->get($systemUserId);
}
}
}
/**
*
* Set Login: Sets the SystemUserLogin object to $systemUserLogin variable
* @param $_systemUserLogin SystemUserLogin Gets a SystemUserLogin object
*
*/
public function setSystemUserLogin(SystemUserLogin $_systemUserLogin)
{
$this->systemUserLogin = $_systemUserLogin;
}
/**
*
* Login
*
*/
public function login()
{
$this->systemUserAuthenticator($this);
}
}
<?php
namespace MyAppModelsSystemUser;
use MyAppCoreConfig;
use MyAppHelpersSession;
/**
*
* System User Details Class
*
*/
class SystemUserDetails
{
/*=================================
= Variables =
=================================*/
# @object database Database instance
private $db;
# Users data
private $data;
# User user ID name
public $userId;
# User first name
public $firstName;
# User last name
public $lastName;
# Username
public $userName;
# User Email
public $email;
# User Last logged in
public $lastLogin;
/*# is user logged in
public $isLoggedIn;
# is user logged in
public $login_timestamp;*/
# is user IP
private $user_ip;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct()
{
# Get database instance
$this->db = Database::getInstance();
}
/**
*
* Find method: Find user by id or by username
* @param $user String / Init A username or user ID
* @return
*
*/
public function get(Int $systemUserId)
{
if ($systemUserId) {
# Enable search for a system_user by a string name or if numeric - so by id.
$field = ( is_numeric($systemUserId) ) ? 'system_user_id' : 'uname';
# Search for the system_user in the Database 'system_users' table.
$data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $systemUserId));
# If there is a result
if ( $data ) {
# Set data
$this->setUserData($data);
return $this;
} else {
return false;
}
}
else {
return false;
}
}
/**
*
* Set User data to $this obj
* @param $userData Array User data fetched from the db (usually by the find method)
* @return
*
*/
public function set(Array $userData)
{
// Set data for this user object
$this->userId = $userData['system_user_id'];
$this->firstName = $userData['fname'];
$this->lastName = $userData['lname'];
$this->userName = $userData['uname'];
$this->email = $userData['email'];
$this->lastLogin = $userData['last_login'];
}
/**
*
* Get User from session
* @param
* @return
*
*/
public function getUserFromSession()
{
# Check if there is a session user id set
if (Session::exists(Config::$session_name)) {
# Insert session data to system_user variable
return Session::get(Config::$session_name);
} else {
# Returning false cause there is no user id session
return false;
}
}
}
<?php
namespace MyAppModelsSystemUser;
/**
*
* System User Details Class
*
*/
class systemUserLogin
{
/*=================================
= Variables =
=================================*/
# @str Customer name
public $customerName;
# @str UserName
public $userName;
# @str Password
public $password;
# @str user IP
public $userIp;
/*===============================
= Methods =
================================*/
/**
*
* Construct - Set customer, username and password
* @param $_customerName String
* @param $_userName String
* @param $_password String
*
*/
public function __construct(String $_customerName, String $_userName, String $_password)
{
$this->customerName = $_customerName;
$this->userName = $_userName;
$this->password = $_password;
$this->userIp = $this->getSystemUserIp();
}
/**
*
* Get user IP
* @return String Returns the user IP that is trying to connect.
*
*/
private function getSystemUserIp()
{
if (!empty($_SERVER['HTTP_CLIENT_IP']))
$ip = $_SERVER['HTTP_CLIENT_IP'];
elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
else
$ip = $_SERVER['REMOTE_ADDR'];
return $ip;
}
}
<?php
namespace MyAppModelsSystemUser;
/**
*
* System User Details Class
*
*/
class systemUserAuthenticator
{
/*=================================
= Variables =
=================================*/
# @object Database instance
private $db;
# @bool Is logged in
public $isLoggedIn = false;
# @str Login Timestamp
public $loginTimestamp;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct()
{
# Get database instance
$this->db = Database::getInstance();
}
/**
*
* Login method
* @param $customer_name String Get a customer_name user input
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Boolian Is this a signed System user?
*
*/
public function login(User $user)
{
# Create a Customer Obj
$customer = new MyAppModelsCustomer($user->SystemUserLogin->customerName);
try {
# Check if the result is an array
# OR there is no row result:
if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
throw new MyAppCoreExceptionHandlerLoginException("Bad company name: {$user->SystemUserLogin->customerName}");
# Change localhost string to 127.0.0.1 (prevent dns lookup)
$customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;
# Connect to new database
$new_connection = $this->db->customer_connect($customer->host, $customer->dbName);
# If status is connected
if ($new_connection) {
# Check for user credentials data
$user_data = $this->system_user_login_validation($user->SystemUserLogin->userName, $user->SystemUserLogin->password);
# If the result isn't a valid array - EXEPTION
if ( (!is_array($user_data)) || (empty($user_data)) )
throw new MyAppCoreExceptionHandlerLoginException("Customer: '{$user->SystemUserLogin->customerName}' - Invalid username ({$user->SystemUserLogin->userName}) or password ({$user->SystemUserLogin->password})");
# Store Customer in the sesison
Session::put(Config::$customer, serialize($customer));
# Update host and db for the db object
# $this->db->update_host_and_db($customer->host, $customer->dbName);
# Set data for this System_user object
$this->setUserData($user_data);
# Set a login session for the user id:
Session::put(Config::$session_name, $this->user_id);
# Set logged in user sessions
$this->set_loggedin_user_sessions();
return $this;
} else {
# Connect back to backoffice (current db set)
$this->db->connect_to_current_set_db();
throw new MyAppCoreExceptionHandlerLoginException('User does not exist');
return false;
}
} catch (MyAppCoreExceptionHandlerLoginException $e) {
$e->log($e);
return false;
// die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
}
}
/**
*
* Check if user exist in 'system_users' table
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Array/Boolian Is this a signed System user?
*
*/
private function systemUserLoginValidation($username, $password)
{
$userData = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));
if ($userData)
return $userData;
else
return false;
}
}
php object-oriented design-patterns
asked 2 days ago
potatoguypotatoguy
375
I wrote this class a few months ago and noticed from a few examples that it's better to break down these classes and separate them.
I am not so sure what is the proper way to break if to parts.
It currently includes a creation of a System_user obj based on user id (fetching user data), login validation, logout, storing user data to session(more specifically CSRF), and I think that's all..
This is my working code:
<?php
namespace MyAppModels;
use Exception;
use MyAppCoreDatabase;
use MyAppCoreConfig;
use MyAppHelpersSession;
use MyAppHelpersCookie;
use MyAppHelpersToken;
use MyAppHelpersGeneral;
use MyAppHelpersHash;
/**
*
* System User Class
*
*/
class System_user
{
/*=================================
= Variables =
=================================*/
# @object database Database instance
private $db;
# Users data
private $data;
# User user ID name
public $user_id;
# User first name
public $first_name;
# User last name
public $last_name;
# Username
public $user_name;
# User Email
public $email;
# User Last logged in
public $last_login;
# is user logged in
public $isLoggedIn;
# is user logged in
public $login_timestamp;
# is user IP
private $user_ip;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct($system_user = NULL)
{
# Get database instance
$this->db = Database::getInstance();
# If system_user isn't passed as a variable
if ( !$system_user ) {
# ...so check if there is a session user id set
if (Session::exists(Config::$session_name)) {
# Insert session data to system_user variable
$system_user = Session::get(Config::$session_name);
# Get user data
$this->find($system_user);
}
} else {
$this->find($system_user);
}
}
/**
*
* Find method: Find user by id or by username
* @param $user String/Init A username or user ID
*
*/
public function find($system_user = NULL)
{
if ($system_user) {
// Enable search for a system_user by a string name or if numeric - so by id.
$field = ( is_numeric($system_user) ) ? 'system_user_id' : 'uname';
// Search for the system_user in the Database 'system_users' table.
$data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $system_user));
// If there is a result
if ( $data ) {
// Set data
$this->setUserData($data);
return $this;
} else {
return false;
}
}
else{
return false;
}
}
/**
*
* Check if user exist in 'system_users' table
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Array/Boolian Is this a signed System user?
*
*/
private function system_user_login_validation($username, $password)
{
$user_data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));
if ($user_data)
return $user_data;
else
return false;
}
/**
*
* Login method
* @param $customer_name String Get a customer_name user input
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Boolian Is this a signed System user?
*
*/
public function login($customer_name, $username, $password)
{
# Create a Customer Obj
$customer = new MyAppModelsCustomer($customer_name);
try {
# Check if the result is an array
# OR there is no row result:
if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
throw new MyAppCoreExceptionHandlerLoginException("Bad company name: {$customer_name}");
# Change localhost string to 127.0.0.1 (prevent dns lookup)
$customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;
# Connect to new database
$new_connection = $this->db->customer_connect($customer->host, $customer->dbName);
# If status is connected
if ($new_connection) {
# Check for user credentials data
$user_data = $this->system_user_login_validation($username, $password);
# If the result isn't a valid array - EXEPTION
if ( (!is_array($user_data)) || (empty($user_data)) )
throw new MyAppCoreExceptionHandlerLoginException("Customer: '{$customer_name}' - Invalid username ({$username}) or password ({$password})");
# Store Customer in the sesison
Session::put(Config::$customer, serialize($customer));
# Update host and db for the db object
# $this->db->update_host_and_db($customer->host, $customer->dbName);
# Set data for this System_user object
$this->setUserData($user_data);
# Set a login session for the user id:
Session::put(Config::$session_name, $this->user_id);
# Set logged in user sessions
$this->set_loggedin_user_sessions();
return $this;
} else {
# Connect back to backoffice (current db set)
$this->db->connect_to_current_set_db();
throw new MyAppCoreExceptionHandlerLoginException('User does not exist');
return false;
}
} catch (MyAppCoreExceptionHandlerLoginException $e) {
$e->log($e);
return false;
// die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
}
}
/**
*
* Set sessions for the logged in user.
* Tutorial: http://forums.devshed.com/php-faqs-stickies/953373-php-sessions-secure-post2921620.html
*
*/
public function set_loggedin_user_sessions()
{
# Generate security sessions
$this->generate_security_sessions();
# Set login timestamp
Session::put(Config::$login_timestamp, $this->login_timestamp);
# Set login flag to true
Session::put(Config::$is_logged_in, true);
# Set login IP
Session::put(Config::$login_user_ip, $this->user_ip);
}
/**
*
* Generate system user security sessions
* @param $new_session Boolean (optinal) Dedices if to delete the cookie session id [default is set to true]
*
*/
public function generate_security_sessions($new_session = true)
{
if ($new_session)
# Generate a new session ID
session_regenerate_id(true);
# Fetch cookie session ID
$session_id = session_id();
# Set the session id to the session
Session::put(Config::$session_id, $session_id);
# Create a secret token
# Set it in session (does them both)
$secret = Token::generate_login_token();
# Combine secret and session_id and create a hash
$combined = Hash::make_from_array(array($secret, $session_id, $this->user_ip));
# Add combined to session
Session::put(Config::$combined, $combined);
}
/**
*
* Check if there is a logged in user
*
*/
public function check_logged_in()
{
if ( Session::exists(Config::$secret) && # Secret session exists
Session::exists(Config::$session_id) && # Session_id session exists
Session::exists(Config::$session_name) && # User session exists
Session::exists(Config::$is_logged_in) && # Check if 'logged in' session exists
Session::exists(Config::$session_name) # Check if sys_user id is set in session
)
{
# Get users ip
$ip = $this->get_system_user_ip();
# if the saved bombined session
if (
(Session::get(Config::$combined) === Hash::make_from_array(array(Session::get(Config::$secret), session_id()), $ip)) &&
(Session::get(Config::$is_logged_in) === true )
)
{
# Set ip to system user object
$this->user_ip = $ip;
return true;
} else {
return false;
}
}
else {
return false;
}
}
/**
*
* Check if loggin session is timeout
*
*/
public function check_timeout()
{
if (Session::exists(Config::$login_timestamp)){
# Calculate time
$session_lifetime_seconds = time() - Session::get(Config::$login_timestamp) ;
if ($session_lifetime_seconds > Config::MAX_TIME){
$this->logout();
return true;
} else {
return false;
}
} else {
$this->logout();
return false;
}
}
/**
*
* Get user IP
*
*/
private function get_system_user_ip()
{
if (!empty($_SERVER['HTTP_CLIENT_IP']))
$ip = $_SERVER['HTTP_CLIENT_IP'];
elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
else
$ip = $_SERVER['REMOTE_ADDR'];
return $ip;
}
/**
*
* Set User data to (this) System_user object
* @param $user_data Array User data fetched from the db (usually by the find method)
*
*/
private function setUserData($user_data)
{
// Set data for this user object
$this->user_id = $user_data['system_user_id'];
$this->first_name = $user_data['fname'];
$this->last_name = $user_data['lname'];
$this->user_name = $user_data['uname'];
$this->email = $user_data['email'];
$this->last_login = $user_data['last_login'];
$this->isLoggedIn = true;
$this->user_ip = $this->get_system_user_ip();
$this->login_timestamp = time();
}
/**
*
* Logout: Now guess what this method does..
*
*/
public function logout()
{
$this->isLoggedIn = false;
Cookie::eat_cookies();
Session::kill_session();
session_destroy();
session_write_close();
}
}
I would like to get suggestions about my current code, and if possible, about structuring it differently with more than one class. (class SystemUser, class systemUserLogin, class systemUserAuthenticator, ect')
ps: In general, the webapp by default logs in to a general database. when a user inserts his company_name, username and password, I check if the company name actually exist, if if does, I disconnect from the general db and connect to the customers database and validate his username & password.
This is the new class I started writing (not tested, so I cant assure this is a working code) with more classes following this example & inspired by this post I found, while tring to follow the SOLID principals and PSR standards, focusing on the structure and architecture.
<?php
namespace MyAppModels;
use MyAppCoreConfig;
use MyAppHelpersSession;
use MyAppCoreDatabase;
/**
*
* System User Class
*
*/
class SystemUser
{
/*=================================
= Variables =
=================================*/
# @obj SystemUser profile information (fullname, profile picture... etc')
protected $systemUserDetatils;
# @obj SystemUser Login data
protected $systemUserLogin;
# @obj SystemUser Authenticator
protected $systemUserAuthenticator;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct($systemUserId = NULL)
{
# If system_user passed
if ( $systemUserId ) {
# Create systemUserDedatils obj
$this->systemUserDetatils = new MyAppModelsSystemUserSystemUserDetatils();
# Get SysUser data
$this->systemUserDetatils->get($systemUserId);
} else {
# Check for sysUser id in the session:
$systemUserId = $this->systemUserDetatils->getUserFromSession();
# Get user data from session
if ( $systemUserId ) {
# Create systemUserDedatils obj
$this->systemUserDetatils = new MyAppModelsSystemUserSystemUserDetatils();
# Get SysUser data
$this->systemUserDetatils->get($systemUserId);
}
}
}
/**
*
* Set Login: Sets the SystemUserLogin object to $systemUserLogin variable
* @param $_systemUserLogin SystemUserLogin Gets a SystemUserLogin object
*
*/
public function setSystemUserLogin(SystemUserLogin $_systemUserLogin)
{
$this->systemUserLogin = $_systemUserLogin;
}
/**
*
* Login
*
*/
public function login()
{
$this->systemUserAuthenticator($this);
}
}
<?php
namespace MyAppModelsSystemUser;
use MyAppCoreConfig;
use MyAppHelpersSession;
/**
*
* System User Details Class
*
*/
class SystemUserDetails
{
/*=================================
= Variables =
=================================*/
# @object database Database instance
private $db;
# Users data
private $data;
# User user ID name
public $userId;
# User first name
public $firstName;
# User last name
public $lastName;
# Username
public $userName;
# User Email
public $email;
# User Last logged in
public $lastLogin;
/*# is user logged in
public $isLoggedIn;
# is user logged in
public $login_timestamp;*/
# is user IP
private $user_ip;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct()
{
# Get database instance
$this->db = Database::getInstance();
}
/**
*
* Find method: Find user by id or by username
* @param $user String / Init A username or user ID
* @return
*
*/
public function get(Int $systemUserId)
{
if ($systemUserId) {
# Enable search for a system_user by a string name or if numeric - so by id.
$field = ( is_numeric($systemUserId) ) ? 'system_user_id' : 'uname';
# Search for the system_user in the Database 'system_users' table.
$data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $systemUserId));
# If there is a result
if ( $data ) {
# Set data
$this->setUserData($data);
return $this;
} else {
return false;
}
}
else {
return false;
}
}
/**
*
* Set User data to $this obj
* @param $userData Array User data fetched from the db (usually by the find method)
* @return
*
*/
public function set(Array $userData)
{
// Set data for this user object
$this->userId = $userData['system_user_id'];
$this->firstName = $userData['fname'];
$this->lastName = $userData['lname'];
$this->userName = $userData['uname'];
$this->email = $userData['email'];
$this->lastLogin = $userData['last_login'];
}
/**
*
* Get User from session
* @param
* @return
*
*/
public function getUserFromSession()
{
# Check if there is a session user id set
if (Session::exists(Config::$session_name)) {
# Insert session data to system_user variable
return Session::get(Config::$session_name);
} else {
# Returning false cause there is no user id session
return false;
}
}
}
<?php
namespace MyAppModelsSystemUser;
/**
*
* System User Details Class
*
*/
class systemUserLogin
{
/*=================================
= Variables =
=================================*/
# @str Customer name
public $customerName;
# @str UserName
public $userName;
# @str Password
public $password;
# @str user IP
public $userIp;
/*===============================
= Methods =
================================*/
/**
*
* Construct - Set customer, username and password
* @param $_customerName String
* @param $_userName String
* @param $_password String
*
*/
public function __construct(String $_customerName, String $_userName, String $_password)
{
$this->customerName = $_customerName;
$this->userName = $_userName;
$this->password = $_password;
$this->userIp = $this->getSystemUserIp();
}
/**
*
* Get user IP
* @return String Returns the user IP that is trying to connect.
*
*/
private function getSystemUserIp()
{
if (!empty($_SERVER['HTTP_CLIENT_IP']))
$ip = $_SERVER['HTTP_CLIENT_IP'];
elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
$ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
else
$ip = $_SERVER['REMOTE_ADDR'];
return $ip;
}
}
<?php
namespace MyAppModelsSystemUser;
/**
*
* System User Details Class
*
*/
class systemUserAuthenticator
{
/*=================================
= Variables =
=================================*/
# @object Database instance
private $db;
# @bool Is logged in
public $isLoggedIn = false;
# @str Login Timestamp
public $loginTimestamp;
/*===============================
= Methods =
================================*/
/**
*
* Construct
*
*/
public function __construct()
{
# Get database instance
$this->db = Database::getInstance();
}
/**
*
* Login method
* @param $customer_name String Get a customer_name user input
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Boolian Is this a signed System user?
*
*/
public function login(User $user)
{
# Create a Customer Obj
$customer = new MyAppModelsCustomer($user->SystemUserLogin->customerName);
try {
# Check if the result is an array
# OR there is no row result:
if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
throw new MyAppCoreExceptionHandlerLoginException("Bad company name: {$user->SystemUserLogin->customerName}");
# Change localhost string to 127.0.0.1 (prevent dns lookup)
$customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;
# Connect to new database
$new_connection = $this->db->customer_connect($customer->host, $customer->dbName);
# If status is connected
if ($new_connection) {
# Check for user credentials data
$user_data = $this->system_user_login_validation($user->SystemUserLogin->userName, $user->SystemUserLogin->password);
# If the result isn't a valid array - EXEPTION
if ( (!is_array($user_data)) || (empty($user_data)) )
throw new MyAppCoreExceptionHandlerLoginException("Customer: '{$user->SystemUserLogin->customerName}' - Invalid username ({$user->SystemUserLogin->userName}) or password ({$user->SystemUserLogin->password})");
# Store Customer in the sesison
Session::put(Config::$customer, serialize($customer));
# Update host and db for the db object
# $this->db->update_host_and_db($customer->host, $customer->dbName);
# Set data for this System_user object
$this->setUserData($user_data);
# Set a login session for the user id:
Session::put(Config::$session_name, $this->user_id);
# Set logged in user sessions
$this->set_loggedin_user_sessions();
return $this;
} else {
# Connect back to backoffice (current db set)
$this->db->connect_to_current_set_db();
throw new MyAppCoreExceptionHandlerLoginException('User does not exist');
return false;
}
} catch (MyAppCoreExceptionHandlerLoginException $e) {
$e->log($e);
return false;
// die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
}
}
/**
*
* Check if user exist in 'system_users' table
* @param $username String Get a username user input
* @param $password String Get a password user input
* @throws Array/Boolian Is this a signed System user?
*
*/
private function systemUserLoginValidation($username, $password)
{
$userData = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));
if ($userData)
return $userData;
else
return false;
}
}
php object-oriented design-patterns
php object-oriented design-patterns
asked 2 days ago
potatoguypotatoguy
375
asked 2 days ago
potatoguypotatoguy
375
edited 2 days ago
potatoguy
asked 2 days ago
potatoguypotatoguy
375
asked 2 days ago
potatoguypotatoguy
375
asked 2 days ago
potatoguypotatoguy
375
375
add a comment |
add a comment |
1 Answer
1
active
oldest
votes
1
In my last answer for you I mention how you should use dependency injection to avoid tight coupling and promote testability. Then in the comments I go further to mention to take it a step further and use a container like Pimple. Since I don't see those changes here I'll show the container example here since I showed the basic dependency injection in the other answer.
Using Pimple, the Dependency Injection Container
I'll assume you will have installed Pimple already and have included it in your application. Their documentation covers that so I won't get into it here.
use PimpleContainer;
use MyAppCoreDatabase;
$container = new Container();
$container['db'] = function ($c) {
return Database::getInstance();
};
The above code simply:
- Creates a container
- Defines a service called
db
- Instantiates your database class
- Places it in your container
You can add your session logic and other shared objects at this time as well. This is typically contained in its own file but where you put this ins entirely up to you as long as it executes as part of your bootstrap process (i.e. before your business logic).
From here you only need to include Pimple as an argument of the constructor of objects that need to use something in your container.
class System_user
{
public function __construct(Pimple $container, $system_user = NULL)
{
$this->db = $container['db'];
}
}
Now you can easily make sure all of your classes are working with the same objects, eliminate dependencies in your code, and your code is testable.
Good job with not putting login info into the User object
A common pitfall many developers fall into is to put the login logic into a user object because the user is the one who logs in. You pass the User object into the login functionality which is a much better way to do this. An area for improvement is you place the validation and the login logic all in one method. You could break out the validation into it's own method so you separate the two concerns. You also do this like work with IP addresses again which should be separated out into its own logic.
Getting IP addresses is kind of common
You have a private method for getting the user's IP address (systemUserLogin::getSystemUserIp()). That actually is something not directly related to a user and may be something you eventually wish to use elsewhere. That probably should be broken out into its own function or into another helper class.
FYI Stuff
sha1() is obsolete for hashing passwords and should not be used. PHP provides password_hash() and password_verify(), please use them. And here are some good ideas about passwords. If you are using a PHP version prior to 5.5 there is a compatibility pack available here.
answered 2 days ago
John CondeJohn Conde
25018
New contributor
John Conde is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
I made a little research about pimple and it's amazing! Thank you! I am mainly expecting a review regarding the re-construction of the class (/classes).
– potatoguy
yesterday
1
I added some more info directly related to how you architected your classes. I'll add more if I think of anything through the day.
– John Conde
yesterday
Since when didsha1()become obsolete and why? :O I am continuing to write the new classes and I came across a problem - Do I need to set the properties$this->user_id | $this->first_name | $this->last_name | $this->user_name | $this->email | $this->last_login | $this->isLoggedIn | $this->user_ip | $this->login_timestamp(see first "old" code part) to he SystemUser class? or should I add it to the SystemUserDetails class? (so to fetch it i will have to$user->systemUserDetails->userId&$user->systemUserAuthenticator->isLoggedIn&$user->systemUserDetails->userIdect?
– potatoguy
20 hours ago
1
SHA1 has been obsolete since 2005 when it was first successfully attacked by researchers.
– John Conde
16 hours ago
1
Those properties look like they belong ot the SystemUser class as they are properties of that user.
– John Conde
14 hours ago
|
show 4 more comments
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
draft saved
draft discarded
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211040%2fuser-class-getting-user-data-loggin-in-secure-csrf-session-handling-logout%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
1
In my last answer for you I mention how you should use dependency injection to avoid tight coupling and promote testability. Then in the comments I go further to mention to take it a step further and use a container like Pimple. Since I don't see those changes here I'll show the container example here since I showed the basic dependency injection in the other answer.
Using Pimple, the Dependency Injection Container
I'll assume you will have installed Pimple already and have included it in your application. Their documentation covers that so I won't get into it here.
use PimpleContainer;
use MyAppCoreDatabase;
$container = new Container();
$container['db'] = function ($c) {
return Database::getInstance();
};
The above code simply:
- Creates a container
- Defines a service called
db
- Instantiates your database class
- Places it in your container
You can add your session logic and other shared objects at this time as well. This is typically contained in its own file but where you put this ins entirely up to you as long as it executes as part of your bootstrap process (i.e. before your business logic).
From here you only need to include Pimple as an argument of the constructor of objects that need to use something in your container.
class System_user
{
public function __construct(Pimple $container, $system_user = NULL)
{
$this->db = $container['db'];
}
}
Now you can easily make sure all of your classes are working with the same objects, eliminate dependencies in your code, and your code is testable.
Good job with not putting login info into the User object
A common pitfall many developers fall into is to put the login logic into a user object because the user is the one who logs in. You pass the User object into the login functionality which is a much better way to do this. An area for improvement is you place the validation and the login logic all in one method. You could break out the validation into it's own method so you separate the two concerns. You also do this like work with IP addresses again which should be separated out into its own logic.
Getting IP addresses is kind of common
You have a private method for getting the user's IP address (systemUserLogin::getSystemUserIp()). That actually is something not directly related to a user and may be something you eventually wish to use elsewhere. That probably should be broken out into its own function or into another helper class.
FYI Stuff
sha1() is obsolete for hashing passwords and should not be used. PHP provides password_hash() and password_verify(), please use them. And here are some good ideas about passwords. If you are using a PHP version prior to 5.5 there is a compatibility pack available here.
answered 2 days ago
John CondeJohn Conde
25018
New contributor
John Conde is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
I made a little research about pimple and it's amazing! Thank you! I am mainly expecting a review regarding the re-construction of the class (/classes).
– potatoguy
yesterday
1
I added some more info directly related to how you architected your classes. I'll add more if I think of anything through the day.
– John Conde
yesterday
Since when didsha1()become obsolete and why? :O I am continuing to write the new classes and I came across a problem - Do I need to set the properties$this->user_id | $this->first_name | $this->last_name | $this->user_name | $this->email | $this->last_login | $this->isLoggedIn | $this->user_ip | $this->login_timestamp(see first "old" code part) to he SystemUser class? or should I add it to the SystemUserDetails class? (so to fetch it i will have to$user->systemUserDetails->userId&$user->systemUserAuthenticator->isLoggedIn&$user->systemUserDetails->userIdect?
– potatoguy
20 hours ago
1
SHA1 has been obsolete since 2005 when it was first successfully attacked by researchers.
– John Conde
16 hours ago
1
Those properties look like they belong ot the SystemUser class as they are properties of that user.
– John Conde
14 hours ago
|
show 4 more comments
1
In my last answer for you I mention how you should use dependency injection to avoid tight coupling and promote testability. Then in the comments I go further to mention to take it a step further and use a container like Pimple. Since I don't see those changes here I'll show the container example here since I showed the basic dependency injection in the other answer.
Using Pimple, the Dependency Injection Container
I'll assume you will have installed Pimple already and have included it in your application. Their documentation covers that so I won't get into it here.
use PimpleContainer;
use MyAppCoreDatabase;
$container = new Container();
$container['db'] = function ($c) {
return Database::getInstance();
};
The above code simply:
- Creates a container
- Defines a service called
db
- Instantiates your database class
- Places it in your container
You can add your session logic and other shared objects at this time as well. This is typically contained in its own file but where you put this ins entirely up to you as long as it executes as part of your bootstrap process (i.e. before your business logic).
From here you only need to include Pimple as an argument of the constructor of objects that need to use something in your container.
class System_user
{
public function __construct(Pimple $container, $system_user = NULL)
{
$this->db = $container['db'];
}
}
Now you can easily make sure all of your classes are working with the same objects, eliminate dependencies in your code, and your code is testable.
Good job with not putting login info into the User object
A common pitfall many developers fall into is to put the login logic into a user object because the user is the one who logs in. You pass the User object into the login functionality which is a much better way to do this. An area for improvement is you place the validation and the login logic all in one method. You could break out the validation into it's own method so you separate the two concerns. You also do this like work with IP addresses again which should be separated out into its own logic.
Getting IP addresses is kind of common
You have a private method for getting the user's IP address (systemUserLogin::getSystemUserIp()). That actually is something not directly related to a user and may be something you eventually wish to use elsewhere. That probably should be broken out into its own function or into another helper class.
FYI Stuff
sha1() is obsolete for hashing passwords and should not be used. PHP provides password_hash() and password_verify(), please use them. And here are some good ideas about passwords. If you are using a PHP version prior to 5.5 there is a compatibility pack available here.
answered 2 days ago
John CondeJohn Conde
25018
New contributor
John Conde is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
I made a little research about pimple and it's amazing! Thank you! I am mainly expecting a review regarding the re-construction of the class (/classes).
– potatoguy
yesterday
1
I added some more info directly related to how you architected your classes. I'll add more if I think of anything through the day.
– John Conde
yesterday
Since when didsha1()become obsolete and why? :O I am continuing to write the new classes and I came across a problem - Do I need to set the properties$this->user_id | $this->first_name | $this->last_name | $this->user_name | $this->email | $this->last_login | $this->isLoggedIn | $this->user_ip | $this->login_timestamp(see first "old" code part) to he SystemUser class? or should I add it to the SystemUserDetails class? (so to fetch it i will have to$user->systemUserDetails->userId&$user->systemUserAuthenticator->isLoggedIn&$user->systemUserDetails->userIdect?
– potatoguy
20 hours ago
1
SHA1 has been obsolete since 2005 when it was first successfully attacked by researchers.
– John Conde
16 hours ago
1
Those properties look like they belong ot the SystemUser class as they are properties of that user.
– John Conde
14 hours ago
|
show 4 more comments
1
1
1
In my last answer for you I mention how you should use dependency injection to avoid tight coupling and promote testability. Then in the comments I go further to mention to take it a step further and use a container like Pimple. Since I don't see those changes here I'll show the container example here since I showed the basic dependency injection in the other answer.
Using Pimple, the Dependency Injection Container
I'll assume you will have installed Pimple already and have included it in your application. Their documentation covers that so I won't get into it here.
use PimpleContainer;
use MyAppCoreDatabase;
$container = new Container();
$container['db'] = function ($c) {
return Database::getInstance();
};
The above code simply:
- Creates a container
- Defines a service called
db
- Instantiates your database class
- Places it in your container
You can add your session logic and other shared objects at this time as well. This is typically contained in its own file but where you put this ins entirely up to you as long as it executes as part of your bootstrap process (i.e. before your business logic).
From here you only need to include Pimple as an argument of the constructor of objects that need to use something in your container.
class System_user
{
public function __construct(Pimple $container, $system_user = NULL)
{
$this->db = $container['db'];
}
}
Now you can easily make sure all of your classes are working with the same objects, eliminate dependencies in your code, and your code is testable.
Good job with not putting login info into the User object
A common pitfall many developers fall into is to put the login logic into a user object because the user is the one who logs in. You pass the User object into the login functionality which is a much better way to do this. An area for improvement is you place the validation and the login logic all in one method. You could break out the validation into it's own method so you separate the two concerns. You also do this like work with IP addresses again which should be separated out into its own logic.
Getting IP addresses is kind of common
You have a private method for getting the user's IP address (systemUserLogin::getSystemUserIp()). That actually is something not directly related to a user and may be something you eventually wish to use elsewhere. That probably should be broken out into its own function or into another helper class.
FYI Stuff
sha1() is obsolete for hashing passwords and should not be used. PHP provides password_hash() and password_verify(), please use them. And here are some good ideas about passwords. If you are using a PHP version prior to 5.5 there is a compatibility pack available here.
answered 2 days ago
John CondeJohn Conde
25018
New contributor
John Conde is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
In my last answer for you I mention how you should use dependency injection to avoid tight coupling and promote testability. Then in the comments I go further to mention to take it a step further and use a container like Pimple. Since I don't see those changes here I'll show the container example here since I showed the basic dependency injection in the other answer.
Using Pimple, the Dependency Injection Container
I'll assume you will have installed Pimple already and have included it in your application. Their documentation covers that so I won't get into it here.
use PimpleContainer;
use MyAppCoreDatabase;
$container = new Container();
$container['db'] = function ($c) {
return Database::getInstance();
};
The above code simply:
- Creates a container
- Defines a service called
db
- Instantiates your database class
- Places it in your container
You can add your session logic and other shared objects at this time as well. This is typically contained in its own file but where you put this ins entirely up to you as long as it executes as part of your bootstrap process (i.e. before your business logic).
From here you only need to include Pimple as an argument of the constructor of objects that need to use something in your container.
class System_user
{
public function __construct(Pimple $container, $system_user = NULL)
{
$this->db = $container['db'];
}
}
Now you can easily make sure all of your classes are working with the same objects, eliminate dependencies in your code, and your code is testable.
Good job with not putting login info into the User object
A common pitfall many developers fall into is to put the login logic into a user object because the user is the one who logs in. You pass the User object into the login functionality which is a much better way to do this. An area for improvement is you place the validation and the login logic all in one method. You could break out the validation into it's own method so you separate the two concerns. You also do this like work with IP addresses again which should be separated out into its own logic.
Getting IP addresses is kind of common
You have a private method for getting the user's IP address (systemUserLogin::getSystemUserIp()). That actually is something not directly related to a user and may be something you eventually wish to use elsewhere. That probably should be broken out into its own function or into another helper class.
FYI Stuff
sha1() is obsolete for hashing passwords and should not be used. PHP provides password_hash() and password_verify(), please use them. And here are some good ideas about passwords. If you are using a PHP version prior to 5.5 there is a compatibility pack available here.
answered 2 days ago
John CondeJohn Conde
25018
New contributor
John Conde is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited yesterday
answered 2 days ago
John CondeJohn Conde
25018
New contributor
John Conde is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
answered 2 days ago
John CondeJohn Conde
25018
answered 2 days ago
John CondeJohn Conde
25018
25018
New contributor
John Conde is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
John Conde is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
John Conde is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
I made a little research about pimple and it's amazing! Thank you! I am mainly expecting a review regarding the re-construction of the class (/classes).
– potatoguy
yesterday
1
I added some more info directly related to how you architected your classes. I'll add more if I think of anything through the day.
– John Conde
yesterday
Since when didsha1()become obsolete and why? :O I am continuing to write the new classes and I came across a problem - Do I need to set the properties$this->user_id | $this->first_name | $this->last_name | $this->user_name | $this->email | $this->last_login | $this->isLoggedIn | $this->user_ip | $this->login_timestamp(see first "old" code part) to he SystemUser class? or should I add it to the SystemUserDetails class? (so to fetch it i will have to$user->systemUserDetails->userId&$user->systemUserAuthenticator->isLoggedIn&$user->systemUserDetails->userIdect?
– potatoguy
20 hours ago
1
SHA1 has been obsolete since 2005 when it was first successfully attacked by researchers.
– John Conde
16 hours ago
1
Those properties look like they belong ot the SystemUser class as they are properties of that user.
– John Conde
14 hours ago
|
show 4 more comments
I made a little research about pimple and it's amazing! Thank you! I am mainly expecting a review regarding the re-construction of the class (/classes).
– potatoguy
yesterday
1
I added some more info directly related to how you architected your classes. I'll add more if I think of anything through the day.
– John Conde
yesterday
Since when didsha1()become obsolete and why? :O I am continuing to write the new classes and I came across a problem - Do I need to set the properties$this->user_id | $this->first_name | $this->last_name | $this->user_name | $this->email | $this->last_login | $this->isLoggedIn | $this->user_ip | $this->login_timestamp(see first "old" code part) to he SystemUser class? or should I add it to the SystemUserDetails class? (so to fetch it i will have to$user->systemUserDetails->userId&$user->systemUserAuthenticator->isLoggedIn&$user->systemUserDetails->userIdect?
– potatoguy
20 hours ago
1
SHA1 has been obsolete since 2005 when it was first successfully attacked by researchers.
– John Conde
16 hours ago
1
Those properties look like they belong ot the SystemUser class as they are properties of that user.
– John Conde
14 hours ago
I made a little research about pimple and it's amazing! Thank you! I am mainly expecting a review regarding the re-construction of the class (/classes).
– potatoguy
yesterday
I made a little research about pimple and it's amazing! Thank you! I am mainly expecting a review regarding the re-construction of the class (/classes).
– potatoguy
yesterday
1
1
I added some more info directly related to how you architected your classes. I'll add more if I think of anything through the day.
– John Conde
yesterday
I added some more info directly related to how you architected your classes. I'll add more if I think of anything through the day.
– John Conde
yesterday
Since when did
sha1() become obsolete and why? :O I am continuing to write the new classes and I came across a problem - Do I need to set the properties $this->user_id | $this->first_name | $this->last_name | $this->user_name | $this->email | $this->last_login | $this->isLoggedIn | $this->user_ip | $this->login_timestamp (see first "old" code part) to he SystemUser class? or should I add it to the SystemUserDetails class? (so to fetch it i will have to $user->systemUserDetails->userId & $user->systemUserAuthenticator->isLoggedIn & $user->systemUserDetails->userId ect?– potatoguy
20 hours ago
Since when did
sha1() become obsolete and why? :O I am continuing to write the new classes and I came across a problem - Do I need to set the properties $this->user_id | $this->first_name | $this->last_name | $this->user_name | $this->email | $this->last_login | $this->isLoggedIn | $this->user_ip | $this->login_timestamp (see first "old" code part) to he SystemUser class? or should I add it to the SystemUserDetails class? (so to fetch it i will have to $user->systemUserDetails->userId & $user->systemUserAuthenticator->isLoggedIn & $user->systemUserDetails->userId ect?– potatoguy
20 hours ago
1
1
SHA1 has been obsolete since 2005 when it was first successfully attacked by researchers.
– John Conde
16 hours ago
SHA1 has been obsolete since 2005 when it was first successfully attacked by researchers.
– John Conde
16 hours ago
1
1
Those properties look like they belong ot the SystemUser class as they are properties of that user.
– John Conde
14 hours ago
Those properties look like they belong ot the SystemUser class as they are properties of that user.
– John Conde
14 hours ago
|
show 4 more comments
draft saved
draft discarded
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Some of your past answers have not been well-received, and you're in danger of being blocked from answering.
Please pay close attention to the following guidance:
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
draft saved
draft discarded
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f211040%2fuser-class-getting-user-data-loggin-in-secure-csrf-session-handling-logout%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
