User class: getting user data, logging in, secure CSRF session handling, logging out

Question

In reference to my previous code: User class: getting user data, logging in, secure CSRF session handling

I re-wrote my SystemUser class, without the DI container (for now), I'm still trying to figure out how to structure my code to work with dependency injections container rather than a singletone.

I wrote my code basically after looking at this example and inspired by this post I found, while trying to follow the SOLID principals and PSR standards, focusing on the structure and architecture (I would rather prefer to focus on the code architecture, since I was told that my code "is far from being good" without explaining why :[ )

SystemUser.php

<?php
namespace MyApp\Models;

class SystemUser
{
    public $id;
    public $firstName;
    public $lastName;
    public $userName;
    public $email;
    public $lastLogin;
    public $customerName;
    public $password; 
    public $ip;
    public $loginTimestamp;
    public $isLoggedIn;

    # @obj SystemUser profile information (fullname, email, last_login, profile picture, etc')
    public $systemUserDetatils;
    # @obj SystemUser Login data (a template for a login insert)
    public $systemUserLogin;
    # @obj SystemUser Authenticator object
    protected $systemUserAuthenticator;
    # @obj SystemUser Logout handling 
    protected $systemUserLogout;
    # @obj Handle SystemUser Sessions (Sets user sessions, Check if sessions are set, Check timeout, ect')
    public $systemUserSessions;

/*===============================
=            Methods            =
================================*/


    /**
     *
     *  Construct
     *  @param      $systemUserId   Int    (optional) User Id 
     *
     */
    public function __construct($systemUserId = NULL)
    {
        # Create systemUserDedatils obj
        $this->systemUserDetatils   = new \MyApp\Models\SystemUser\SystemUserDetails();

        # If system_user passed
        if ( $systemUserId ) {

            # Set system user ID
            $this->id                   = $systemUserId;

            # Get SysUser data
            $this->systemUserDetatils->get($this);

        } else {

            # Check for sysUser id in the session:
            $systemUserId                   = $this->systemUserDetatils->getUserFromSession();

            # Get user data from session 
            if ( $systemUserId ) {

                # Set system user ID
                $this->id                   = $systemUserId;

                # Get SysUser data
                $this->systemUserDetatils->get($this);
            }
        }
    }


    /**
     *
     *  Set Login: Sets the SystemUserLogin object to $systemUserLogin variable. 
     *  @param     $_systemUserLogin   SystemUserLogin     Gets a SystemUserLogin object
     *
     */
    public function setSystemUserLogin(\MyApp\Models\SystemUser\SystemUserLogin $_systemUserLogin)
    {
        $this->systemUserLogin = $_systemUserLogin; 
    }


    /**
     *
     *  System User Login 
     *  @return     
     *
     */
    public function login()
    {
        $this->systemUserAuthenticator = new \MyApp\Models\SystemUser\SystemUserAuthenticator(); 
        return $this->systemUserAuthenticator->login($this);
    }


    /**
     *
     *  Logout: Now guess what this method does.. 
     *
     */
    public function logout()
    {
        $this->systemUserLogout = new \MyApp\Models\SystemUser\SystemUserLogout($this);
    }


    /**
     *
     *  Checks if a user is logged in
     *  @return     
     *
     */
    public function isLoggedIn()
    {
        return $this->systemUserSessions->isLoggedIn($this);
    }


    /**
     *
     *  Checks if a system user has a timeout
     *  @return     
     *
     */
    public function checkTimeout()
    {
        return $this->systemUserSessions->isTimeout($this);
    }
}

SystemUserDetails.php

<?php 
namespace MyApp\Models\SystemUser;

use MyApp\Core\Database;
use MyApp\Core\Config;
use MyApp\Helpers\Session;

class SystemUserDetails 
{

    private $db;


    /**
     *
     *  Construct
     *
     */
    public function __construct(/*Database $db*/)
    {
        # Get database instance
        $this->db           = Database::getInstance();
        // $this->db           = $db;
    }


    /**
     *
     *  Find method: Find user by id or by username 
     *  @param      $user   String / Init     A username or user ID
     *  @return 
     *
     */
    public function get(\MyApp\Models\SystemUser $systemUser)
    {
        if ($systemUser->id) {

            # Enable search for a system_user by a string name or if numeric - so by id
            $field  = ( is_numeric($systemUser->id) ) ? 'system_user_id' : 'uname';

            # Search for the system_user in the Database 'system_users' table. 
            $result = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $systemUser->id));

            # If there is a result
            if ( $result ) {

                # Set result
                $this->set($systemUser, $result);

                return true;
            } 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(\MyApp\Models\SystemUser $systemUser, Array $userData)
    {
        $systemUser->id         = $userData['system_user_id'];
        $systemUser->firstName  = $userData['fname'];
        $systemUser->lastName   = $userData['lname'];
        $systemUser->userName   = $userData['uname'];
        $systemUser->email      = $userData['email'];
        $systemUser->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::$systemUserId)) {

            # Insert session data to system_user variable
            return Session::get(Config::$systemUserId);

        } else {
            # Returning false cause there is no user id session
            return false; 
        }
    }
}

SystemUserLogin.php

<?php 
namespace MyApp\Models\SystemUser;

/**
 *
 *  System User Login - Prepare a user for login with this class.
 *
 */
class SystemUserLogin 
{    
    public $customerName;
    public $userName;
    public $password;
    public $userIp;

    /**
     *
     *  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       = \MyApp\Helpers\General::getIp();
    }

}

SystemUserAuthenticator.php:

    <?php 
    namespace MyApp\Models\SystemUser;

    use MyApp\Core\Database;
    use MyApp\Core\Config;
    use MyApp\Helpers\Session;

   class SystemUserAuthenticator 
    {

        private $db;


        /**
         *
         *  Construct
         *
         */
        public function __construct(/*Database $db*/)
        {
            # Get database instance
            $this->db           = Database::getInstance();
            // $this->db           = $db;
        }



        /**
         *
         *  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
         *  @return                     Boolian     Is this a signed System user?
         *
         */
        public function login(\MyApp\Models\SystemUser $systemUser)
        {
            # Create a Customer Obj
            $customer = new \MyApp\Models\Customer($systemUser->systemUserLogin->customerName);

            try {

                # Check customer result
                if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
                    throw new \MyApp\Core\Exception\Handler\LoginException("Bad company name: {$systemUser->systemUserLogin->customerName}");

                # Connect to new database
                $newConnection = $this->db->customer_connect($customer->host, $customer->dbName);

                # If status is connected 
                if ($newConnection) {

                    # Check for user credentials data 
                    $userData = $this->systemUserLoginValidation($systemUser->systemUserLogin->userName, $systemUser->systemUserLogin->password); 

                    # If the result isn't a valid array - EXEPTION  
                    if ( (!is_array($userData)) || (empty($userData)) )
                        throw new \MyApp\Core\Exception\Handler\LoginException("Customer: '{$systemUser->SystemUserLogin->customerName}' - Invalid username ({$systemUser->SystemUserLogin->userName}) or password ({$systemUser->SystemUserLogin->password})");

                    # Store Customer in the sesison
                    Session::put(Config::$customer, serialize($customer));

                    # Set data for this System_user object
                    $this->set($systemUser, $userData);

                    # Set a login session for the user id: 
                    Session::put(Config::$systemUserId, $systemUser->id);

                    # Set logged in user sessions
                    // $this->setLoggedinUserSessions($systemUser);
                    $systemUser->systemUserSessions = new \MyApp\Models\SystemUser\SystemUserSessions();

                    $systemUser->systemUserSessions->setSecuritySession($systemUser);
                    # Update last_login for this user
                    $this->updateLastLogin($systemUser->id, $systemUser->loginTimestamp);

                    // return $this;
                    return true;

                } else {
                    # Connect back to backoffice (current db set)
                    $this->db->connect_to_current_set_db();
                    throw new \MyApp\Core\Exception\Handler\LoginException('User does not exist');
                    return false;
                }

            } catch (\MyApp\Core\Exception\Handler\LoginException $e) {
                $e->log($e);
                return false;
                // die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
            }
        }


        /**
         *
         *  Set User data to $this obj
         *  @param      $userData       Array   User data fetched from the db (usually by the find method)
         *  @return  
         *
         */
        public function set(\MyApp\Models\SystemUser $systemUser, Array $userData)
        {
            # Sets basic user data using SystemUserDetails
            $systemUser->systemUserDetatils->set($systemUser, $userData);

            # Set Login data 
            $systemUser->loginTimestamp = date("Y-m-d H:i:s");
            $systemUser->isLoggedIn     = true;
            $systemUser->ip             = $systemUser->systemUserLogin->userIp;
        }


        /**
         *
         *  Check if user exist in 'system_users' table
         *  @param      $username       String              Get a username user input
         *  @param      $password       String              Get a password user input
         *  @return                     Array/Boolian       Is this a signed System user?
         *
         */
        private function systemUserLoginValidation(String $username, String $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;
        }


        /**
         *
         *  Updates the system users "last logged in" field in db
         *  @param      $id     Int     System User ID
         *  @param      $date   String  Current login timestamp (set to $systemUser->loginTimestamp)
         *
         */
        private function updateLastLogin(Int $id, String $date)
        {
            $this->db->row("UPDATE system_users SET last_login = :newLastLogin WHERE system_user_id = :systemUserId", array('newLastLogin' => $date, 'systemUserId' => $id));
        }
    }

SystemUserSessions.php :

<?php 
namespace MyApp\Models\SystemUser;

use MyApp\Core\Config;
use MyApp\Helpers\Session;
use MyApp\Helpers\Token;
use MyApp\Helpers\Hash;
use MyApp\Helpers\General;
use MyApp\Models\SystemUser;

/**
 *
 *  System User Security Session: Handle the system user security session / token / secret / ect.
 *
 */
class SystemUserSessions
{

    /**
     *
     *  Sets SystemUser security session
     *  @param      $ystemUser      Obj     SystemUser object
     *
     */
    public function setSecuritySession(SystemUser $systemUser)
    {
        $this->setLoggedinUserSessions($systemUser);
    }


    /**
     *
     *  Check if there is a logged in user
     *  @param      $ystemUser      Obj     SystemUser object
     *
     */
    public function isLoggedIn(SystemUser $systemUser)
    {
        if ( Session::exists(Config::$secret)       &&  # Secret session exists
             Session::exists(Config::$session_id)   &&  # Session_id session exists
             Session::exists(Config::$systemUserId) &&  # User session exists 
             Session::exists(Config::$is_logged_in)     # Check if 'logged in' session exists
            )
        {
            # Get users ip
            $ip = General::getIp();

            # 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
                $systemUser->ip = $ip;

                return true;

            } else {
                return false;
            }
        }
        else {
            return false; 
        }
    }


    /**
     *
     *  Check if loggin session is timeout
     *  @param      $ystemUser      Obj     SystemUser object
     *
     */
    public function isTimeout(SystemUser $systemUser)
    {
        if (Session::exists(Config::$login_timestamp)){

            # Calculate time 
            $session_lifetime_seconds = time() - Session::get(Config::$login_timestamp) ; 

            if ($session_lifetime_seconds > Config::MAX_TIME){
                $systemUser->logout();
                return true;
            } else {
                return false;
            }

        } else {
            $systemUser->logout();
            return false;
        }
    }















    /**
     *
     *  Set sessions for the logged in user. 
     *  Tutorial: http://forums.devshed.com/php-faqs-stickies/953373-php-sessions-secure-post2921620.html
     *  @param  $systemUser      Object      Gets the main SystemUser class object
     *  
     */
    private function setLoggedinUserSessions(SystemUser $systemUser)
    {
        # Generate security sessions
        $this->generateSecuritySessions($systemUser);

        # Set login timestamp 
        Session::put(Config::$login_timestamp, $systemUser->loginTimestamp);

        # Set login flag to true
        Session::put(Config::$is_logged_in, true);

        # Set login IP 
        Session::put(Config::$login_user_ip, $systemUser->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 generateSecuritySessions(SystemUser $systemUser, bool $newSession = true)
    {
        if ($newSession)
            # Generate a new session ID
            session_regenerate_id(true);

        # Fetch cookie session ID 
        $sessionId  = session_id();
        # Set the session id to the session
        Session::put(Config::$session_id, $sessionId);

        # 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, $sessionId, $systemUser->ip));
        # Add combined to session
        Session::put(Config::$combined, $combined);
    }



}

SystemUserLogout.php:

<?php 
namespace MyApp\Models\SystemUser;

use MyApp\Helpers\Session;
use MyApp\Helpers\Cookie;

class SystemUserLogout 
{
    /**
     *
     *  Construct - Set customer, username and password
     *
     */
    public function __construct()
    {
        # Delete cookies 
        Cookie::eat_cookies();
        # Delete all sessions 
        Session::kill_session();
        # Re-generate SESSIONID
        session_regenerate_id(true);
    }


}

Huge thank you to @John Conde

Hi Rick, im not sure how best to help you - the code is really convoluted, so its hard to work out whats going on, or indeed what the overall purpose is. The main issue i think is that you are writing procedural code, and wrapping it in classes. — Steve, Commented Jan 16, 2019 at 13:44
Is your main objective to learn OOP, or create a usable product? Are you new to just OOP, or programming in general — Steve, Commented Jan 16, 2019 at 13:46
@Steve I'm not sure what's the exact definition of "procedural code". My main objective is to learn OOP. I'm new to OOP in a certain way, I used CodeIgniter most of the time, but never got into writing an MVC or understanding how it works. Most tutorials/courses (Udemy/Blogs/CodeAcademy/Lynda) use weird examples like "cars" / "clothes", and the more practical examples don't use OOP, or at least not in a "serious" way. Or just teach a framework where all is already built. Please do try to review my code. — potatoguy, Commented Jan 16, 2019 at 13:59
Ok, thanks for answering - i was just checking that you werent trying to create an actual product, else i would suggest picking up symfony or laravel and utilizing the huge communities and documentation. — Steve, Commented Jan 16, 2019 at 14:09
To take a step back, are you using any 3rd party libraries, or trying to build a complete system from scratch? Do you have a single entrypoint (usually an index.php) the bootstraps the application? — Steve, Commented Jan 16, 2019 at 14:11

Your Common Sense · Accepted Answer · 2019-01-16 13:30:22Z

For me, there is way too much code to review. So I could make just a brief outline.

first of all, I think here you inclined to the opposite extremity, creating classes with no methods other than constructor. I don't think it's a good move. So I would take Login and Logout back into the main class as methods.

There are many places that can be written more efficiently. For example,

        $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;

is the same as

return $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)));

on a side note, as you can see, it is very hard to read (and review) such big lines. You must split your code so it would fit for the screen:

$sql = "SELECT system_user_id, fname, lname, uname, email, last_login 
        FROM system_users WHERE uname = :username AND password = :password";
$data = array('username' => $username, 'password' => sha1($password));
return $this->db->row($sql, $data);

and now it was revealed that you are using an outdated and insecure approach to hashing passwords. You must hash them using password_hash() and verify using password_verify(). I've got a handy example for this, Authenticating a user using PDO and password_verify().

Another example of rather heavily duplicated code, a literal example of the WET acronym (Write Everything Twice) is SystemUser's constructor. Why not to make it this way

public function __construct($systemUserId = NULL)
{
    $this->id = $systemUserId ?? $this->systemUserDetatils->getUserFromSession();
    $this->systemUserDetatils   = new \MyApp\Models\SystemUser\SystemUserDetails();
    if ( $this->id ) {
        $this->systemUserDetatils->get($this);
    }
}

So here we are effectively writing everything once, making our code DRY (Don't Repeat Yourself) by fist setting $this->id to either $systemUserId if it's set or whatever $this->systemUserDetatils->getUserFromSession() returns otherwise, and then populating the user details.

Thanks a lot for your review, you are one of my favorite users on codereview! :) — potatoguy, Commented Jan 16, 2019 at 13:45
I split the code for the login because I have a validation in the controller (before passing the variables to the login model), and thing I'm going to move my validation to there - so I wont have only one function for the login. For the Logout, I wrote it that way because I'm sure I'm going to add and run more functions to run before I log a user out, so this is just the infrastructure for a bigger thing I'm planning on. for the rest of your comments I'll have to review later in compare to my code, so if i'll have anything to comment on, you'll know.. — potatoguy, Commented Jan 16, 2019 at 13:48

Stack Exchange Network

User class: getting user data, logging in, secure CSRF session handling, logging out

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged
php
object-oriented
design-patterns
or ask your own question.

Linked

Hot Network Questions

User class: getting user data, logging in, secure CSRF session handling, logging out

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged phpobject-orienteddesign-patterns or ask your own question.

Linked

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
php
object-oriented
design-patterns
or ask your own question.