8
\$\begingroup\$

This PHP User System was built with MySQLi and I also used Composer. I'm planning to improve this, and add more stuff and release it as a sort of a module for Composer.

User.php (Controller File in the src/Controller) folder.

<?php
namespace Application\Controllers;

use Http\Request;
use Http\Response;
use Application\Users\Mapper;

class User
{
    private $request;
    private $response;
    private $userMapper;

    public function __construct(Request $request, Response $response, Mapper $mapper)
    {
        $this->request = $request;
        $this->response = $response;
        $this->userMapper = $mapper;
    }

    public function login()
    {
        $username = $this->request->getParameter('username');
        $password = $this->request->getParameter('password');


        if($this->userMapper->authenticateLoginDetails($username, $password)) {
            $this->userMapper->createSession($username);
            $this->userMapper->fetchUserDataAndPopulateUserObject($username);
            $this->response->redirect('/dashboard');
            return true;
        }
        $this->response->redirect('/');
        return false;
    }

    public function logout()
    {
        $this->userMapper->destroySession();
        $this->userMapper->cleanUserObject();
        $this->response->redirect('/');
    }
}

Module Files:

User.php (src/Users/User.php)

<?php
namespace Application\Users;

use \mysqli;

abstract class User
{
    protected $database;
    private $id;
    private $username;
    private $password;
    private $email;

    public function __construct(mysqli $database)
    {
        $this->database = $database;
    }

    public function setID($value)
    {
        $this->id= $value;
    }

    public function getID()
    {
        return $this->id;
    }

    public function setUsername($value)
    {
        $this->username = $value;
    }

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

    public function updateUsername($newUsername)
    {
        $this->setUsername($newUsername);

        $query = $this->database->prepare("UPDATE users SET username=? WHERE id=?");
        $query->bind_param("ss", $newUsername, $this->getID());
        $query->execute();
        $query->close();
    }

    public function setPassword($value)
    {
        $this->password = $value;
    }

    public function getPassword()
    {
        return $this->password;
    }

    public function updatePassword($newPassword)
    {
        $this->setPassword($newPassword);

        $query = $this->database->prepare("UPDATE users SET password=? WHERE id=?");
        $query->bind_param("ss", $newPassword, $this->getID());
        $query->execute();
        $query->close();
    }

    public function setEmail($value)
    {
        $this->email = $value;
    }

    public function getEmail()
    {
        return $this->email;
    }

    public function updateEmail($newEmail)
    {
        $this->setEmail($newEmail);

        $query = $this->database->prepare("UPDATE users SET email=? WHERE id=?");
        $query->bind_param("ss", $newEmail, $this->getID());
        $query->execute();
        $query->close();
    }
}

ApplicationUser.php:

<?php
namespace Application\Users;

class ApplicationUser extends User {
    private $rank;
    private $balance;
    private $participatingTournaments;
    private $points;

    public function clean()
    {
        $this->setID(null);
        $this->setUsername(null);
        $this->setPassword(null);
        $this->setEmail(null);
        $this->setRank(null);
        $this->setPoints(null);
        $this->setBalance(null);
        $this->setParticipatingTournaments(null);

        return true;
    }

    public function setRank($value)
    {
        $this->rank = $value;
    }

    public function getRank()
    {
        return $this->rank;
    }

    public function setBalance($value)
    {
        $this->balance = $value;
    }

    public function getBalance()
    {
        return $this->balance;
    }

    public function updateBalance($newBalance)
    {
        $this->setBalance($newBalance);

        $query = $this->database->prepare("UPDATE users SET balance=? WHERE id=?");
        $query->bind_param("ss", $newBalance, $this->getID());
        $query->execute();
        $query->close();
    }

    public function setParticipatingTournaments($value)
    {
        $this->participatingTournaments = $value;
    }

    public function getParticipatingTournaments()
    {
        return $this->participatingTournaments;
    }

    public function setPoints($value)
    {
        $this->points = $value;
    }

    public function getPoints()
    {
        return $this->points;
    }

    public function updatePoints($newPoints)
    {
        $this->setPoints($newPoints);

        $query = $this->database->prepare("UPDATE users SET points=? WHERE id=?");
        $query->bind_param("ss", $newPoints, $this->getID());
        $query->execute();
        $query->close();
    }
}

Mapper.php:

<?php
namespace Application\Users;

use \mysqli;

class Mapper
{
    private $database;
    private $userObject;

    public function __construct(mysqli $database, ApplicationUser $userObject)
    {
        $this->database = $database;
        $this->userObject = $userObject;
    }

    public function authenticateLoginDetails($username, $rawPassword)
    {
        $query = $this->database->prepare("SELECT password FROM users WHERE username=?");
        $query->bind_param("s", $username);
        $query->execute();
        $query->bind_result($databasePassword);
        $query->store_result();
        while($query->fetch()) {
            if(password_verify($rawPassword, $databasePassword) == true) {
                $query->close();
                return true;
            }
            $query->close();
            return false;
        }
    }

    public function createSession($username)
    {
        session_start();
        $_SESSION['username'] = $username;
        $_SESSION['logoutTime'] = time() + 3600;
    }

    public function destroySession()
    {
        session_start();

        unset($_SESSION['username']);
        unset($_SESSION['logoutTime']);

        session_destroy();

        session_start();
        session_destroy();
    }

    public function fetchUserDataAndPopulateUserObject($username)
    {
        $query = $this->database->prepare("SELECT id, password, email, rank, balance, points FROM users WHERE username=?");
        $query->bind_param("s", $username);
        $query->execute();
        $query->bind_result($id, $password, $email, $rank, $balance, $points);
        while($query->fetch()) {
            $this->userObject->setID($id);
            $this->userObject->setUsername($username);
            $this->userObject->setPassword($password);
            $this->userObject->setEmail($email);
            $this->userObject->setRank($rank);
            $this->userObject->setBalance($balance);
            $this->userObject->setPoints($points);
        }
        $query->close();

        return true;
    }

    public function cleanUserObject()
    {
        $this->userObject->clean();

        return true;
    }

    public function isUserOnline()
    {
        session_start();
        if(isset($_SESSION["username"]) && strlen($_SESSION["username"]) > 0) {
            return true;
        }
        return false;
    }
}

Usage of the above script in my script is as follows:

User fills in a form like this:

<form method="post" action="user/login">
    <input type="text" name="username">
    <input type="password" name="password">
    <input type="submit" name="Login">
</form>

I use a FastRoute library where I have bind the /user/login URI like this:

['POST', '/user/login', ['Application\Controllers\User', 'login']],

This calls my Controller class. I've placed the source above, if you scroll up.

I'm looking for a top-notch code review which also provides an answer for the following questions:

  • Is my code flexible? How may I make it more flexible?
  • Is my code efficient? How may I make it more efficient?
  • Do you have any recommendations for my code? If so, please state.
  • Have I written SOLID code? If not, what principles have I breached and how and how may make them adhere to the SOLID principles.
  • Do you see any flaws in my code? If so please point out.
  • Is the system secure? If not, how may I make it more secure?

Recommendations From Chat

  • Change the form action attribute value to /user/login from user/login
\$\endgroup\$
  • 2
    \$\begingroup\$ Are you okay with book recommendations instead of code-review? Because you ask really a lot, most of the questions can't even be objectively answered. For example take the part where you ask about SOLID principles. Its missing the important context why you can't say that your own. But it starts already at the very first question if the code is flexible. It highly depends on flexible for what. What are your plans to refactor it and where? As long as you don't say, you might end up adding flexibility at the wrong places which wouldn't be helpful to gain more. \$\endgroup\$ – hakre Apr 5 '15 at 7:13
  • \$\begingroup\$ For some insightful examples with the database access and your classes I recommend the Patterns of Enterprise Application Architecture - depending of the level of data-access you need, you can find patterns in there. After you've located one that pleases you, search for an existing library that works that way and use it. Best code is code you don't need to write and maintain yourself. The usage scenario you paint here doesn't look like it hasn't been already solved. So you should (again) add more context why right now you need a review. \$\endgroup\$ – hakre Apr 5 '15 at 7:18
5
+50
\$\begingroup\$

Misc

  • password_verify returns a boolean value, so there is no need to do == true, just ask if(password_verify($rawPassword, $databasePassword)).
  • why have while($query->fetch()) in authenticateLoginDetails? There should only exist one user with that name, but even if there are more, you return after checking the first users password (you return if it is false or true). You do the same again in fetchUserDataAndPopulateUserObject, only that there, you do not take the values of the first found user, but the last (which could be considered a bug, because it will cause problems if you ever decide that you do allow duplicate usernames).
  • it's not clear why you start and destroy the session again after you destroyed it. If there is a specific reason, I would add a comment to make it clear.
  • you sometimes have unnecessary return values, eg in fetchUserDataAndPopulateUserObject or cleanUserObject. If the only possible return value is true, then it's not needed.
  • instead of isset($_SESSION["username"]) && strlen($_SESSION["username"]) > 0, I would go with !empty($_SESSION["username"]), it's behaviour will be the same for your case, but it's a lot shorter. And then I would transform the if(boolean) { return true; } else { return false; } into a direct return: return !empty($_SESSION["username"])
  • InauthenticateLoginDetails, if a user is not found, it would return nothing, and the calling if would work only because of the fact that if(null) is the same as if(false). I would make this more explicit by returning false at the end.
  • It's unclear where you are calling password_hash on a new password, I would have expected this to take place in updatePassword.

Naming

  • isUserOnline is misleading, as it implies that it also checks the logoutTime (or anything to that effect, which would tell you that the user is still active), which it doesn't. isUserLoggedIn might be an alternative.
  • fetchUserDataAndPopulateUserObject is very descriptive, but also a bit long. I think just populateUserObject would be fine, or alternatively maybe fetchAndPopulateUser.
  • I would probably rename User to UserController, so it's easier to differentiate it from the other User.
  • I would rename $query to $statement, because it's a statement.
  • in updatePassword, I would change $newPassword to $passwordHash. It's obvious from the context that it's the new password, but it's not obvious that it is a hash, not plaintext.

Structure

  • I like that you separated the User and the ApplicationUser, it adds flexibility and reusability.
  • Your controller looks very clean, that's nice.
  • It's not all that clear to me what the responsibility of the Mapper class is. What does it map? User to something? It seems to be a mixture of DAO and session handler, which doesn't seem ideal to me. I would probably move the database methods into it's own DAO class, or move it in with the User class, and then also create a specific Session class, which handles sessions (I think it's already enough code for a class, but I think that there will be more code coming, eg updating the logoutTime, regenerating the session id, etc).

Error Handling

Currently, you don't really have any error handling (that you have posted). It seems that incorrect user credentials would result in the same page for the user as an incorrectly written query, or problems with the database. I would either check the return values of execute, etc, or manage this with exceptions, so that you at least have the option to report to the user if it's their problem, or a server problem.

Security

Your system seems secure. You use prepared statements (and you use them correctly), and you hash your passwords using bcrypt, which is also recommended. There also do not seem to be any logical errors with your login process or your session management.

It's generally recommended to regenerate the session id when session data changes (such as on logins), although that's not as important as it used to be (as use_trans_sid is false by default). Still, regenerating session ids is good practice to limit the dangers of session fixation and session highjacking.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I am pretty impressed by your answer, however, I do not understand the meaning of 'DAO' and you have not answered my questions.. \$\endgroup\$ – Abandoned Account Apr 13 '15 at 18:20
  • \$\begingroup\$ @HassanAlthaf DAO = data access object (example code) (although I used the term a bit more losely, as in, perform database operations for a specific class). I think I answered most of your questions, with the exception of efficiency (I didn't see anything extremely bad (you do perform two queries for login + populating user where one would be enough, but I think the code clarity is worth it). I would wait until I notice performance problems and profile those), and parts of SOLID (I did mention Mapper, which has more than one responsibility). \$\endgroup\$ – tim Apr 13 '15 at 18:34
  • \$\begingroup\$ Alright, thanks @tim I will wait for about 48 hours and see if I get more answers. I hope you are alright with it. But, I'll upvote your post. \$\endgroup\$ – Abandoned Account Apr 13 '15 at 18:38
  • \$\begingroup\$ @HassanAlthaf yes, of course. I would also wait for a while for more answers. \$\endgroup\$ – tim Apr 13 '15 at 18:40
  • \$\begingroup\$ Return nothing is not recommended in Robert Martin's book Clean Code. Instead a default value of the same type is preferred. \$\endgroup\$ – Dmitri Zaitsev Apr 18 '15 at 2:23
7
\$\begingroup\$

- How may I make it more flexible?

Depend on abstractions not on concretions (SOLI D )

__construct(Request $request, Response $response, Mapper $mapper)

Each concrete (Request, Response, Mapper) could be replaced with a Interface making the code more flexible for changes in the dependency and not avoiding tight coupling.

- Is my code efficient? How may I make it more efficient?

When I read efficient, I understand that as fast, if that is the case, do not worry about it, that could be considered as premature optimization. There might be some places where one might reconsider if the logic is really justified, readable and easy to refactor, again, it really has nothing to do with speed.

    session_start();
    unset($_SESSION['username']);
    unset($_SESSION['logoutTime']);
    session_destroy();
    session_start();
    session_destroy();

Reconsider the logic in this code, take a extra look at session_destroy and the example on removing a session.

Also single $_SESSION = []; will remove any data.

- Do you have any recommendations for my code?

Access to the $_SESSION should be abstracted away into a class.

A couple of places in your code return a boolean, consider using no return value when none is needed and istead throw a Exception if something went wrong.

- Have I written SOLID code?

Single responsibility principle - There seems to be abit of trouble adhering to this principle especially in the Mapper, it acts both as a proxy to the user, wrapper for the session. It should take a map a user not depend on one via. the constructor.

Dependency inversion principle - Remove your hard dependencies from the constructors and add interfaces instead.

  __construct(FooInterface $foo = null)
  {
    $this->foo = $foo ?: new MyDefaultFoo;
  }

Take care, as it is very hard to forfill these principles at all times, I would recommend to strive for them but accept it's not always what you end up with. Also there are other 3 and 5 letter acronyms also worth remembering and striving for. =]

- Do you see any flaws in my code? If so please point out.

At multiple places in the codebase do you run the session_start() this function should only be run once, and only if the session is not running, calling isUserOnline() and destroySession() would result in a PHP Notice: A session had already been started - ignoring session_start() in php shell code on line 1, other combinations of session related method would also end up in this. Consider reading this question about checking if the session is running

Consider the difference between private and protected. Also keep in mind why you close of access to modification, if you make both a setValue($x) { $this->x = $x } and getValue($x) { return $this->x; } having a private $x; has no use and might as well be a public $x.

- Is the system secure? If not, how may I make it more secure?

I am not a security expert and will leave any real security advice to a expert any day of the week. If you need security review I would recommend a question with a really specific context. The only things I can take away from the code and give some comments one is

  • In any security/permission related context always use === instead of == regarding password_verify($rawPassword, $databasePassword) == true, but in this case it does not really matter, just do not make it a habit.

  • Watch out for while($query->fetch() only one user should be fetched and the username should be a unique value.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot! Both yours and tims post are awesome. Also, can you tell me how to make non-SOLID areas to SOLID code? Especially the Mapper story, I do not understand what you say. \$\endgroup\$ – Abandoned Account Apr 13 '15 at 19:42

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