2
\$\begingroup\$

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, and I think that's all.

This is my working code:

<?php
namespace MyApp\Models;
use \Exception;
use MyApp\Core\Database;
use MyApp\Core\Config;
use MyApp\Helpers\Session;
use MyApp\Helpers\Cookie;
use MyApp\Helpers\Token;
use MyApp\Helpers\General;
use MyApp\Helpers\Hash;



/**
 *
 *  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 \MyApp\Models\Customer($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 \MyApp\Core\Exception\Handler\LoginException("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 \MyApp\Core\Exception\Handler\LoginException("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 \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 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.

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question instead, although I'd recommend waiting at least a day before doing so. More answers might be incoming. \$\endgroup\$ – Mast Jan 6 at 16:49
  • \$\begingroup\$ @Mast I was composing a meta question ask about this as, being a new member here, I wasn't sure what the proper protocol was. Thank you for clarifying. \$\endgroup\$ – John Conde Jan 6 at 16:54
  • \$\begingroup\$ @JohnConde No problem. Feel free to ask if anything else is unclear, me and other regulars can usually be found in The 2nd Monitor. \$\endgroup\$ – Mast Jan 6 at 16:59
  • \$\begingroup\$ @Mast I didn't edit my original code (which is a working code) - I have just added an other code (the one i started to write, which I didn't test and don't know if it works). the questions still refers to the first code. the second code block is just for ref. \$\endgroup\$ – potatoguy Jan 7 at 8:19
  • \$\begingroup\$ I know, but that doesn't change anything. Don't add/change code after the first answer comes in, so all answerers see the same code. Should you have improved code, feel free to post a follow-up question linking back to this one. Make sure you tell a bit about what you changed and why. \$\endgroup\$ – Mast Jan 7 at 9:23
3
\$\begingroup\$

Interestingly enough we just had another question where there was a large user class doing a lot. It was correctly pointed out that is not a good thing as it violates the Single Responsibility Principle. To summarize it, a class should have one and only one responsibility. If your user class is handling the user properties, login, and other actions it is doing too much.

You should familiarize yourself with Dependency Injection. In your constructor you instantiate a database class and then use it to get your database abstraction object. Now you cannot unit test this class because you cannot mock that object. (You can still do an integration test, though). "Dependency injection allows a client to remove all knowledge of a concrete implementation that it needs to use. This helps isolate the client from the impact of design changes and defects. It promotes reusability, testability and maintainability". (source) In other words, your user class has a dependency on the Database class and is at risk if backwards incompatible changes are made to it.

A high level explanation of what you would want to do here to improve this is:

  1. Create an interface that your database implements. This will enforce that any database objects in your code will adhere to the same contract (assuming they all implemnt this interface).
  2. Instantiate the database object in the client code (the code that calls the user class).
  3. Pass it as a parameter to your constructor and then assign it to your User::db property. Make sure you type hint that parameter using the name of the interface you created in step 1 so if a different database object is created and used it will have to adhere to the same contract or else your code will blow up (in testing before it ever goes live).

Here's some simple code to get you started:

The Database Interface

This is just a stub. You will need to complete it.

interface iDatabase
{
    public function row($sql);
    public function customer_connect($host, $dbName);
}

Implement the interface

class Database implements iDatabase

Make your database object a parameter of your contstructor

// Put optional parameters after required parameters
public function __construct(iDatabase $db, $system_user = NULL)

Instantiate your class passing the database object as a parameter

$db = Database::getInstance();
$this->user = new User($db);

You would follow the same example above for any other logic that you pull out of your user class and into its own object. Now your User class does only one thing and does it well and it testable.

Some little stuff

Put a line between your namespace and use statements

PSR-2 coding standards say there should be a line between the namespace declaration and your use statements.

namespace MyApp\Models;

use \Exception;

Class names should be camel case

The PSR-1 standards say class names should be camel case and should not use underscores:

class SystemUser

The PHP community prefers // to # for comments

Although # is a valid syntax for a one line comment in PHP, it is common practice to use //. This came out as a result of the PEAR coding standards.

No need to point out your class' "variables"

Besides the fact that they aren't technically variables but "class members", convention says they go at the top of the class so it is already clear what they are. No need to add unnecessary comments pointing out the obvious. Save your comments for anything that might be ambiguous or needs explanation because it isn't clear from reading the code.

Don't mix coding styles

Your class properties you use both underscores and camel case in your names. Use one or the other but not both.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hello John Conde and thankyou so much for your attention. It's weird for me to pass the DB variable to every class which uses it, are you sure this the the best practice in these situations? I didn't really understand why it's not right from your explanation. + I added my current attempt to re-write this class following a different post from the one you have mentioned. I would be glad if I can get your opinion about it too. ~ still didn't finish the last class... \$\endgroup\$ – potatoguy Jan 6 at 15:44
  • \$\begingroup\$ Yes, I am sure. It seems like a lot but it is very common. This promotes test-ability and re-usability. If you wanted to take it a step further, and make life easier for yourself, you can use a container like Pimple, create all of your shared objects once, and then pass that container to your other objects. That way you only have to write it once and if you add other shared objects to it any classes that need them will get them automatically since they already have that container. This is how enterprise applications are coded. \$\endgroup\$ – John Conde Jan 6 at 15:50
  • 1
    \$\begingroup\$ I have rolled back both the question and answer to avoid a big mess being created. Should a follow-up question arise, feel free to move the comments you've made to the new question instead. They are still accessible in the edit history. \$\endgroup\$ – Mast Jan 6 at 16:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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