4
\$\begingroup\$

I'm new to both PHP and OOP and would like some constructive feedback on a class I made.

I have a "main account" login system already setup and working; when the user logs in they're presented with a list of games. This code logs said user into a specific game.

I have a separate database for each game, plus one for the main account. Tables for the games include 'Players' 'Login History' and 'Player Inventory'

For security I choose to have both prepared statements and issue a unique token at login stored in the 'player' table; which will be checked anytime there is an interaction with the database.

Things I'm looking for:

  • Bad programming practices: I want to learn to do things the right way before my bad code becomes a bad habit.

  • Readability: Goes along with the bullet above; but the easier it is to read and maintain the better; and hints in this area are more than welcome.

  • Security: Being a game I want this as secure as possible. Any direction in this area is highly appreciated.

  • Better way of doing this: I often miss the obvious and with the amount of experience I have there could easily be a better way I haven't thought of.

<?php
namespace login;
error_reporting(E_ALL);
date_default_timezone_set('America/New_York');
session_start();

class Login
{
    //database variables
    protected $host;
    protected $database;
    protected $username;
    protected $password;

    //database connection
    protected $db_connect;

    //game variables
    protected $game_id;
    protected $game_name;

    //player variables
    protected $user_id;
    protected $player_id;
    protected $player_name;
    protected $token;
    protected $ip;

    public function __construct($game,$user_id)
    {
        $this->game_name = $game;
        $this->user_id = $user_id;

        switch($game)
        {
            case 'game_001' :
                //set up variables
                $this->game_id = 001;
                $this->host = 'host';
                $this->database = 'game_001';
                $this->username = 'dbUser';
                $this->password = 'dbPassword';
                break;
            default :
                die('No database exists.');
        }

        //delete this
        echo 'Login started...</br>';

        //login to game
        $this->login();
    }

    public function __destruct()
    {
        //close database connection
        $this->db_connect->close();

        //delete this
        echo 'Login has been closed.';
    }

    private function login()
    {
        //connect to database
        $this->db_connect();

        //get player id and name with user_id
        $this->get_player();

        //create token
        $this->generate_token();

        //get ip
        $this->get_ip();

        //record login
        $this->record_login();

        //log token in database
        $this->set_token();

        //log session variables
        $this->set_session();
    }

    private function set_session()
    {
        //delete this
        echo 'Setting session variables...</br>';

        $_SESSION[$this->game_name.'-token'] = $this->token;
        $_SESSION[$this->game_name.'-logged_in'] = true;
        $_SESSION[$this->game_name.'-player_id'] = $this->player_id;
        $_SESSION[$this->game_name.'-player_name'] = $this->player_name;

        //delete this
        echo 'Session has been set.</br>';
    }

    private function set_token()
    {
        //delete this
        echo 'Setting token in database...</br>';

        $db = $this->db_connect;
        $sql = "UPDATE `players` SET `token` = ? WHERE `players`.`player_id` = ?";

        //prepare the statement
        if(!$stmt = $db->prepare($sql))
        {
            die("Prepare failed: (".$db->errno.")".$db->error);
        }

        //bind variables
        if(!$stmt->bind_param("si",$this->token, $this->player_id))
        {
            die("Binding parameters failed: (".$db->errno.")".$db->error);
        }

        //execute the prepared statement
        if(!$stmt->execute())
        {
            die("Execution failed: (".$db->errno.")".$db->error);
        }

        //delete this
        echo 'Token set successfully.</br>';
    }

    private function get_player()
    {
        //delete this
        echo 'Searching for player information...</br>';

        $db = $this->db_connect;
        $user_id = $this->user_id;

        $sql = "SELECT player_id, player_name FROM players WHERE user_id = ?";

        //prepare the statement
        if(!$stmt = $db->prepare($sql)){
            die("Prepare failed: (".$db->errno.")".$db->error);
        }

        //bind variables
        if(!$stmt->bind_param("i",$user_id)){
            die("Binding parameters failed: (".$db->errno.")".$db->error);
        }

        //execute the prepared statement
        if(!$stmt->execute()){
            die("Execution failed: (".$db->errno.")".$db->error);
        }

        //bind result
        $stmt->bind_result($this->player_id,$this->player_name);

        if(!$stmt->fetch()) {
            die("Fetch result failed.");
        }

        //delete this
        echo 'Player information found: (#' . $this->player_id . ') ' . $this->player_name . '</br>';
    }

    private function record_login()
    {
        //delete this
        echo 'Recording log into database...</br>';

        $type = 'login';
        $note = '';
        $time= date('Y-m-d H:i:s');
        $player_id = $this->player_id;
        $player_name = $this->player_name;
        $db = $this->db_connect;

        $sql = "INSERT INTO login_history (game_num, player_id, user_id, log_type, timestamp, ip) VALUES (?,?,?,?,?,?)";

        //prepare the statement
        if(!$stmt = $db->prepare($sql))
        {
            die("Prepare failed: (".$db->errno.")".$db->error);
        }

        //bind variables
        if(!$stmt->bind_param("iiisss",$this->game_id, $this->player_id, $this->user_id, $type, $time, $this->ip))
        {
            die("Binding parameters failed: (".$db->errno.")".$db->error);
        }

        //execute the prepared statement
        if(!$stmt->execute())
        {
            die("Execution failed: (".$db->errno.")".$db->error);
        }

        //delete this
        echo 'Login recorded successfully.</br>';
    }

    private function generate_token()
    {
        //delete this
        echo 'Generating token...</br>';

        $this->token = md5(uniqid(rand(), TRUE));

        //delete this
        echo 'Token generated: ' . $this->token . '</br>';
    }

    private function get_ip()
    {
        //delete this
        echo 'Retreiving player ip address...</br>';

        $client  = @$_SERVER['HTTP_CLIENT_IP'];
        $forward = @$_SERVER['HTTP_X_FORWARDED_FOR'];
        $remote  = $_SERVER['REMOTE_ADDR'];

        if(filter_var($client, FILTER_VALIDATE_IP))
        {
            $ip = $client;
        }
        elseif(filter_var($forward, FILTER_VALIDATE_IP))
        {
            $ip = $forward;
        }
        else
        {
            $ip = $remote;
        }

        $this->ip = $ip;

        //delete this
        echo 'IP address found: ' . $this->ip . '</br>';
    }

    private function db_connect()
    {
        echo 'Establishing database connection...</br>';

        //the actual db connection (\ is used because mysqli is a global class)
        $db =  new \mysqli($this->host,$this->username,$this->password,$this->database);

        //error check and return is everything is good.
        if(mysqli_connect_error())
        {
            exit();
        }
        else
        {
            //delete this
            echo 'Database connection established.</br>';

            $this->db_connect = $db;
        }
    }
}
?>
\$\endgroup\$
2
\$\begingroup\$

Output of utility class

You are outputting error conditions and debugging statements to the output buffer on several occassions. This is an utility class, used by other code to log into a game. Error conditions should set a specific attribute that other code can use to detect an error and show an appropriate message such as "The server is currently under maintanance. Please try again later." when a database is not found, while you log useful information for developers only. Similarly, you can show a similar message when the player is, for example, already logged into the game.

Don't show sql errors to the user. They belong in a private error log somewhere. Give the user a user-friendly error message instead.

The ever-growing switch

In your constructor you have an ever-growing switch statement. These details do not belong in this class. You might choose to store them in their own table, or have a file with constants somewhere. You are setting yourself up for a world of hassle by duplicating login data, id's etc. over several files, then having to change them everywhere and potentially forgetting something.

Class naming

I don't think Login is a good name for this particular class. What you seem to try to emulate is a game session, so you probably should name it that instead.

Token generation

I am not sure what you are trying to accomplish with your token generation. uniqid() has two optional parameters, the first being a string containing a prefix, the second being a boolean to have more entropy. You seem desperate to get something unique, so you generate a random number between 0 and rand_max (which is then converted to a string for a prefix), and set "more entropy" to true, so you generate an even bigger token. Uniqueness is still not guaranteed though. Then you throw it through md5(), which is a hash function, which will thus increase the likelyhood that your token matches an other token.

You do not share why you are generating a token in the first place, or what problem you are trying to solve with it. If you just need a unique value to store game data with, why not use the player id? I assume a player has a saved game state per game, and can only play the same game one instance at a time. If you need to share a token with the client, why not use the session? The session token is already shared with the client, and you can just store your information in the session itself.

If you really need a token, try to use just uniqid("", TRUE); or uniqid("server-id", TRUE); if you are using some kind of load balancer set-up.

Orphaned sessions

Your login creates a game session, and stores it in the session variable. At no point you ever check if such a session already existed. You probably want to not automatically login in this class, but instead have both the login and logout procedures here. In the constructor retrieve any information you have on the current session, and let the login and logout procedure handle creation or destruction of the game session.

You can use this class in other operations too to check if the user is still logged in.

Use of comments

Don't use comments for single function calls. If your function names are to cryptic to understand what they are doing, you should rename the function. If a function has side-effects, you should rewrite it so those side-effects are eliminated. Otherwise, you are just stating the obvious. In other words: Don't do this.

//login to game
$this->login();

Whitespace

Your whitespace is mostly consistent. I would recommend adding a space after a comma, just like you would do in a normal sentence. The space makes argument lists a lot more readable.

date_default_timezone_set

I don't think this function call belongs in this class file. Instead set the default timezone in a config file. Consider using UTC instead of a random timezone somewhere in the world. You can convert this timestamp to a specific timezone if you need to display it to the user, hopefully based on some kind of preference.

\$\endgroup\$
  • \$\begingroup\$ Thank you for this; I'll be implementing a lot of your idea this weekend. I may have follow up questions and will update my code when I get a chance. \$\endgroup\$ – Dustin Snow Nov 17 '16 at 21:02
  • \$\begingroup\$ I am glad that my review was helpful to you. Please read this meta question if you want to post a follow-up. In particular, if you want a new review for your revised code, post a new question, linking this question. \$\endgroup\$ – Sumurai8 Nov 18 '16 at 9:49
1
\$\begingroup\$

This class is doing too much. Right now, this class:

  • stores DB connection config
    • a concern that should really be moved out into application or, ideally, environmental configuration
  • manages database connection instantiation
    • yet does nothing to promote connection reuse
  • reads request headers
  • manages player authentication
  • outputs end user messaging to standard output

This is ripe for refactoring. You could ideally end up with 5-6 classes that individually handle their piece of the work, with concrete objects being appropriately passed as dependencies to the classes needing them.

For example, perhaps you need to pass a valid Game object to the constructor, with that object providing an already-instantiated mysqli object pointing towards the proper database host and schema. This would remove all responsibility from this class to worry about such things. It just gets a properly set up DB connection that it can work with. This allows you to pass around one game (with its config and DB connection) to many players.

Perhaps this class itself does not need to exist at all.

Based on what you have shown, your application cries out for a Player class. This player class might hold the concerns for authenticating a player (or perhaps there is some player factory or provider class that is used to instantiate Player objects).

With the existence of such a class, you might simply provide a means to attach different games (and thus different DB connections) to the concrete Player object in order to build the connections between players and games.

That might give you a usage pattern like this:

try {
    $player = new Player::getById($player_id);
    $player->login();
} catch (Exception $e) {
    // log error using whatever approach you app uses
    // for example...
    error_log($e->getMessage());
    // maybe rethrow or invoke end user messaging mechanism
}

// if successful...
try {
    $game = new Game($game_id);
    $player->addGame($game);
} catch (Exception $e) {
    ...
}

Some other code comments in addition to those noted in great answer by @Sumurai8


You should begin thinking about adopting use of Exceptions when writing your classes. They are one of the primary methods for notifying calling code of errors in trying to perform the caller's requests. They can be used to convey appropriate information to let the caller make the decision on how to handle the issue (i.e. pass it along for end user messaging, try to recover, etc.) as calling code is almost always in a better position to know how to handle the issue globally than is the code being called.

You are not validating any input to your public methods, thus leaving your class vulnerable to being put in a bad state (which for your class, as written, pretty much means the application will die, perhaps without useful trace of the problem being recorded in your logs). Before doing things like opening up database resources and the other things you are doing in your login() method, you should be first validating that passed parameters are as expected (a non-zero-length string and an integer. If this simple conditions are not met, you should ideally throw and exception or otherwise immediately stop executing the method.

Ideally if you take on the suggestion to refactor, you being to get in the habit of passing around object which are easier to validate as parameters via type hinting. This means that if the objects are set up properly before being injected as a dependency into the class, the class can fully rely on the functionality exposed by the dependency to work properly.


get_player() should perhaps be set_player(), as most typically associate get* terminology with having some value returned to caller.


        case 'game_001' :
            //set up variables
            $this->game_id = 001;

I am not understanding your game id naming approach here. Why not just use integer game id value? What value does having both game_*** and *** formats serve? Why would you be switching logic on "name" text string value versus a (typically) more authoritative id.

Use of 001 integer value here seems programmatically vague and potentially problematic. Why not just 1? What happens when there are 1000+ games?


    $client  = @$_SERVER['HTTP_CLIENT_IP'];
    $forward = @$_SERVER['HTTP_X_FORWARDED_FOR'];

Stay away from error suppression. empty() is your friend.

\$\endgroup\$
  • \$\begingroup\$ As this was more of an expansion of Sumurai8's answer I gave them the 'accepted answer' (not sure what it's considered). Your response will be extremely helpful as it answers a few questions I had reading through the their answer. I will update my code and replies when I get a chance. I have a lot more to learn than I originally thought. \$\endgroup\$ – Dustin Snow Nov 17 '16 at 21:06

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.