3
\$\begingroup\$

I'm just looking for some feedback on my User class. The class is designed to handle users being able to register, login, and logout of the site. I'll be using it in conjunction with a forum I'm also working on.

I'm mostly curious how I've handled validation of input, MySQLi interactivity, and whether my code is redundant and horribly structured.

<?php

// Include the MySQLi connection file containing functions to quickly
// setup a connection to the MySQL database.
require_once 'mysqli_login.php';

class User {

// Class variables.
private $username;
private $password;
private $session;
private $salt;

// Constructor
public function __construct() {
    // Initialize all values as if for a guest.
    $this->username = 'Guest';
    $this->password = '';
    $this->session = '';
    $this->salt = '';   

    // Checks to see if a cookie is set. If so, examines contents of the
    // cookie to see if the username and session ID match the set stored
    // in the database. If a match is found, current Object is initilized
    // for that user.
    if(isset($_COOKIE['user'])) {
        $user_details = explode("|", $_COOKIE['user'], 2);
        $this->username = $user_details[0];
        // First, check that the username is registered in the database. If
        // the user exists, compare the session ID's and ensure a match. If
        // one is found, set Object for that user, if not set the Object as a
        // guest and delete the cookie.
        if($this->user_exists()) {
            $connection = user_connection();
            $sql = "SELECT session FROM users WHERE username='$this->username'";
            $result = $connection->query($sql);
            $connection->close();
            $row = $result->fetch_assoc();
            if($row['session'] === $user_details[1]) {
                $this->session = $row['session'];
                $_SESSION['user'] = $this;
            }
            else {
                $this->username = 'Guest';
                setcookie('user', '', time()-1);
            }
        }
        else {
            $this->username = 'Guest';
            echo "Cookie Login Failed: No such user.<br />";
        }
    }
    // If no cookie is set, check if a session exists. If so, set the current
    // Object to that user's username and session ID.
    else if(isset($_SESSION['user'])) {
        $this->set_username($_SESSION['user']->get_username());
        $this->set_session($_SESSION['user']->get_session());
    }
}

// Outputs the current values assigned to the Object.
public function show_info() {
    echo "Username: $this->username<br />";
    echo "Password: $this->password<br />";
    echo "Session: $this->session<br />";
    echo "Salt: $this->salt<br />";
}

// Ensures a username is set, and that it is only composed of alpha-numerical
// characters. If so, set's the current Object username and return true, else
// returns false.
private function set_username($username) {
    if(!$username || preg_match("/[^]1234567890abcdefghijklmnopqrstuvwxyz]/i", $username)) {
        return 0;
    }
    else {
        $this->username = $username;
        return 1;
    }
}

public function get_username() {
    return $this->username;
}

// Ensures the password is over 6 characters long and only made of acceptable characters.
// If so, sets the Object's password and returns true, else returns false.
private function set_password($password) {
    if(strlen($password) < 6) {
        return 0;
    }
    else if(preg_match("/[^1234567890!@#$%^&*()abcdefghijklmnopqrstuvwxyz]/i", $password)) {
        return 0;
    }
    else {
        $this->password = $password;
        return 1;
    }
}

private function set_session($session) {
    $this->session = $session;
}

public function get_session() {
    return $this->session;
}

// Generates a random 128 character string and sets it to the Object's salt.
private function generate_salt() {
    $char_set = '1234567890!@#$%^&*()abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
    for($i = 0; $i < 128; $i++) {
        $this->salt .= $char_set[rand(0, strlen($char_set)-1)];
    }
}

// Generates a random 128 character string and hashes it, then sets it to the Object's session.
private function generate_session() {
    $char_set = '1234567890!@#$%^&*()abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
    for($i = 0; $i < 128; $i++) {
        $this->session .= $char_set[rand(0, strlen($char_set)-1)];
    }
    $this->session = hash('sha512', $this->session);
}

// Returns true if current User exists in the database. Returns false if not.
private function user_exists() {
    $connection = check_login();
    $sql = "SELECT username FROM users WHERE username = '$this->username'";
    $result = $connection->query($sql);
    $connection->close();
    if($result->num_rows > 0) {
        return TRUE;
    }
    return FALSE;
}

// Adss the current User to the database.
public function register($new_user, $new_pass1, $new_pass2) {
    // Ensures both supplied passwords match. Returns false if not.
    if($new_pass1 != $new_pass2) {
        echo "Passwords must match.<br />";
        return 0;
    }
    // Ensures username is made of valid characters. Returns false if not.
    else if(!$this->set_username($new_user)) {
        echo "Invalid username. Usernames may only contain alpha-numeric characters.<br />";
        return 0;
    }
    // Ensures password is long enough and made of valid characters.
    else if(!$this->set_password($new_pass1)) {
        echo "Invalid password. Passwords must be at least 6 characters long, and may only contain alpha-numeric characters, as well as the following special characters:<br />";
        echo "! @ # $ % ^ & * ( ) <br />";
        return 0;
    }
    // Checks to make sure the current username is not already taken. Returns
    // false if it is.
    else if($this->user_exists()) {
        echo "This username is already registered.<br />";
        return 0;
    }
    // If all conditions pass, generates salt, hashes password with salt appended, then adds
    // current User to the database.
    else {
        $this->generate_salt();
        $this->password = hash('sha512', $this->password . $this->salt);
        $connection = user_connection();
        $sql = "INSERT INTO users(username, password, salt) VALUES('$this->username', '$this->password', '$this->salt')";
        $connection->query($sql);
        $connection->close();
        return TRUE;
    }
}

// Deletes selected user from the database.
public function delete($username) {
    $this->set_username($username);
    // Ensure the selected user is in the database.
    if(!$this->user_exists()) {
        echo "That user doesn't exist.<br />";
        return 0;
    }
    $connection = user_connection();
    $sql = "DELETE FROM users WHERE username='$this->username'";
    if($connection->query($sql)) {
        echo "$username was deleted from the database.<br />";
        return 1;
    }
    return 0;
}

public static function show_users() {
    $connection = user_connection();
    $sql = "SELECT * FROM users";
    $result = $connection->query($sql);
    echo "<style>#show_users { border-collapse: collapse; } #show_users td { border: 1px solid black; max-width: 200px; padding: 3px; overflow: hidden; } </style>";
    echo "<table id='show_users'>";
    while($rows = $result->fetch_assoc()) {
        echo "<tr><td>" . $rows['username'] . "</td>";
        echo "<td>" . $rows['password'] . "</td>";
        echo "<td>" . $rows['salt'] . "</td>";
        echo "<td>" . $rows['session'] . "</td></tr>";
    }
    echo "</table>";
    $connection->close();
}

// Allows a user to login to the site using their username and password.
public function login($username, $password) {
    // Ensures both the username and password are valid entries.
    if($this->set_username($username) && $this->set_password($password)) {
        // Ensures the user is registered in the database.
        if(!$this->user_exists()) {
            return 0;
        }
        $connection = user_connection();
        $sql = "SELECT password, salt FROM users WHERE username='$this->username'";
        $result = $connection->query($sql);
        $row = $result->fetch_assoc();
        // Append user's unique salt to supplied password and hash, then compare to
        // stored password for a match.
        if(hash('sha512', $this->password . $row['salt']) !== $row['password']) {
            $connection->close();
            return 0;
        }
        // If the username and password match, generate a session ID, empty the
        // Object's password, then update the user's entry in the database with
        // the session ID.
        else {
            $this->generate_session();
            $this->password = '';
            $_SESSION['user'] = $this;

            $sql = "UPDATE users SET session='$this->session' WHERE username='$this->username'";
            $connection->query($sql);
            $connection->close();
            // If the "Remember Me" option is checked, set a cookie containing the user's
            // username and session ID.
            if(isset($_POST['remember'])) {
                setcookie('user', $this->username . "|" . $this->session, time() + 60);
            }
            return 1;
        }
    }
    else {
        return 0;
    }
}

// Logs out the user by destroying the session, and deleting the cookie.
public function logout() {
    $connection = user_connection();
    $sql = "UPDATE users SET session=NULL WHERE username='$this->username'";
    $connection->query($sql);
    $connection->close();
    session_destroy();
    setcookie('user', '', time()-1);
}
}

?>
\$\endgroup\$
1
  • \$\begingroup\$ I think this class has way too many responsibilities! It's responsible for representing a User, logging in a User, logging out a User, registering a User, generating salts and sessions... While, really, the job of a User class is just representing a user column from the database ;) This idea is called The Single Responsibility Principle. See if you can put some separate tasks in separate classes (or functions). \$\endgroup\$
    – britishtea
    Feb 17, 2015 at 10:46

1 Answer 1

2
\$\begingroup\$

Comments

There is such a thing as having too many comments. Most readers will read the first two or three, realize that they were not particularly useful to them, and skip the rest.

You should not document how the language works (eg // Class variables. or // Constructor).

Some of your comments also don't really add any information (eg Ensure the selected user is in the database. or // Include the MySQLi connection file containing functions to quickly setup a connection to the MySQL database.).

Generally, in-code comments should only explain why you do something, not what or how you do it. What you do should be expressed with good code, good function names, and possibly good PHPDoc function-level comments.

For function-level comments I would choose some organized comment style like PHPDoc:

 /**
  * sets the given username if it only contains alpha-numerical characters.
  *
  * @param string $username username
  *
  * @return int 1 if the username was set, 0 if it was not set because it contains invalid characters
  */
  private function set_password($password) {
  }

Your function level comments could sometimes be more concrete. For example register: Adss the current User to the database. Always? No. Then when not? Why are two passwords passed? Does it hash the password? If so, how? Does it echo something to the user? When does it return what?

Naming

Your names do not always express what the function does:

  • set_username: sets the username sometimes, but also checks the username.
  • construct: construct this instance, but also performs database interactions, echoes, etc

Functions

Some of your functions do too much (see Naming). I would for example extract the filtering from the setting of the username.

Another example is your constructor, which is very long and nested. I would at least extract the database check to its own function, as well as the cookie check.

Reduce nesting

In addition to extracting code to functions, you can also return early to reduce nesting. For example:

if(!$this->set_username($username) || !$this->set_password($password) || !$this->user_exists()) {
    return 0;
}

Echoing

Your class should not echo, because it makes it very static and non-reusable. What if the calling class wants to handle it differently? It can't. Either handle these cases with a return, or throw an exception.

Class size

In my opinion, your class does too much (user model, handle sessions, register, delete, login, logout, etc). I would think about splitting some of this in different classes.

Security: Cookies

Important cookies should be httpOnly, so that they cannot be read out via JavaScript. This limits the damage an XSS attack can do.

Security: SQL injection

Filtering is not the solution to SQL injection. Right now, it actually works fine, but what if your website grows larger, you get more developers on board, and then a user requests that they can be named "Jack Smith" or "super_awesome" or "Jack O'Rily"? As your filtering is very restrictive, a developer might "improve" it, or just remove it completely. And then you are in trouble.

The way to prevent SQL injection is to use prepared statements always, everywhere, for all variable data.

Security: XSS

Same problem as above. Currently, you are save, but for how long? When echoing variable data, always protect against XSS.

Security: Hashing

sha512 is better than md5, but both are not ideal. If you still can migrate easily, use bcrypt.

Inconsistent return values

Sometimes, you use 0/1 as return values for success/failure, and sometimes TRUE/FALSE (sometimes these even mix in the same function). Try to be more consistent.

Misc

  • if (cond) {return true; } else {return false; } can be written as return cond.
  • your CSS should not be part of your HTML/PHP code, but stored inside an external CSS file.
  • HTML doesn't belong in your User class (because its a model class, not a view).
  • I don't like restricting a users password, and there doesn't seem to be a good reason to do it here. Why can't a user eg use ? or ß in their password?
  • generate salt and generate session share a lot of code, which could be extracted to a function like generateString.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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