4
\$\begingroup\$

This is a Login Authentication / Sign-up Models for my class project in PHP. I would really appreciate criticisms and any suggestions to improve security, code quality, etc.

Hashing.php

class Hashing{
    public static function getHashedPassword($username, $password, $regTime){
        //Prepending Username and Appending Registration time as Salts before hashing & storing the password
        return password_hash($username.$password.$regTime, PASSWORD_DEFAULT);
    }

    public static function verifyHash($password, $hash){
        return password_verify($password,$hash);
    }
}

LoginService.php

require_once("UserInfo.php");
require_once("../../model/db/DbService.php");
include("../../model/login/Roles.php");
include("../../model/login/Hashing.php");

class LoginService{
    private $dbConnection;
    public static $USER_NOT_FOUND=-1;
    public static $PASSWORD_MISMATCH=0;
    public static $USER_FOUND=1;
    //Retrieve a PDO Connection 
    function __construct(DbService $dbService){$this->dbConnection=$dbService->getDbConnection();}

    public function verify($username, $password){
        try{
            /*Given a username and password,
              We will query the Database to get the user with that username;
            */
            $SQL="SELECT * FROM users WHERE username=:username";

            $statement=$this->dbConnection->prepare($SQL);
            $statement->bindParam(":username", $username);
            $statement->execute();

            // If no rows were returned, that means there exists no such user.
            if ($statement->rowCount()==0) return LoginService::$USER_NOT_FOUND;
            // If user exists, we construct a user object.
            $user=$statement->fetch(PDO::FETCH_OBJ);
            // Verify 
            if(Hashing::verifyHash($username.$password.$user->reg_time, $user->password)) return LoginService::$USER_FOUND;
            // If we failed to verify
            return LoginService::$PASSWORD_MISMATCH;

        }catch(Exception $e){
            // In any other event, throw an exception
            throw $e;
        }
    }

}

SignUpService.php

require_once("UserInfo.php");
require_once("../../model/db/DbService.php");
include("../../model/login/Roles.php");
include("../../model/login/Hashing.php");

class SignUpService{
    private $userInfo;
    private $dbConnection;

    // On init, we recieve a UserInfo Object and store it.
    // Next we retrieve a PDO Connection from DbService().getDbConnection.
    function __construct(DbService $dbService){$this->dbConnection=$dbService->getDbConnection();}

    // We try to store the new user in the database, 
    // Throws PDO Exception
    public function register(UserInfo $userInfo){
        $this->userInfo=$userInfo;
        try{
            $this->execute();
            return true;
        }catch(Exception $e){throw $e;}
    }


    private function execute(){
        $SQL="INSERT INTO users (username,password,f_name, l_name, email, phone, reg_time, role) VALUES (:username, :password, :f_name, :l_name, :email, :phone, :reg_time, :role)";
        $statement=$this->dbConnection->prepare($SQL);
        $statement->bindParam(":username", $this->userInfo->getUsername());
        $statement->bindParam(":password", $this->getHashedPassword(new Hashing()));
        $statement->bindParam(":f_name",   $this->userInfo->getFName());
        $statement->bindParam(":l_name",   $this->userInfo->getLName());
        $statement->bindParam(":email" ,   $this->userInfo->getEmail());
        $statement->bindParam(":phone",    $this->userInfo->getPhone());
        $statement->bindParam(":reg_time", $this->userInfo->getRegTime());
        $statement->bindParam(":role", Roles::$STUDENT);
        $statement->execute();
    }

    // Salt & Hash
    private function getHashedPassword(){   
        $username=$this->userInfo->getUsername();
        $password=$this->userInfo->getPassword();
        $regTime=$this->userInfo->getRegTime();
        return Hashing::getHashedPassword($username, $password,$regTime);
    }

}

DbService.php

require_once("DbLoginConsts.php");

// Whenever a PDO connection is required, we aqquire the connection by calling new DbSerive()->getDbConnection();
// This reassures that we are not creating multiple open connections. 
// Use closeDbConnection() to deallocate resources. 
class DbService{
    private $dbConnection=null;

    public function getDbConnection(){
        //Retreiving Settings from Configs/mysql.xml file
        $dbLoginInfo=new DbLoginConsts();

        if (is_null($this->dbConnection)){
            $this->dbConnection=new PDO($dbLoginInfo->getUrl(), $dbLoginInfo->getUsername(), $dbLoginInfo->getPassword());
            // Allowing PDO to throw Exceptions
            $this->dbConnection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }
        //Returning an Instance of a PDO Object. 
        return $this->dbConnection;
    }

    public function closeDbConnection(){
        $this->dbConnection=null;
    }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Tl:DR;

  • Use a coding style. We have some really decent ones defined by the php-fig: psr-1 and psr-2. Why? this makes reading code a lot easier.
  • Add documentation explaining the why: more on this later.
  • Don't just use fancy words like class and Service because
  • Don't re-invent the security wheel: more on this later.
  • Don't write code that doesn't solve a problem.

Author note: I might come over as harsh, and maybe I am. So if I have offended you, well sorry. That was no my intention when writing this. Try and look beyond the rant and learn a couple of things ;)


Use a decent conding-style.

This is a no brainer. Don't aggree with me? We will talk again once you need to debug that old legacy application.

Oh, and while your at it, use PSR-0. This adds the benefit of not needing all those requires in your code. It will happen auto-magically ;). Need integration with existing libraries? Composer works really great with the pgp-fig autoload standards.


Decent documentation

You code has comments, but they are useless and only clutter the screen. Making your app harder to read instead of explaining it. Some examples:

//Prepending Username and Appending Registration time as Salts before hashing & storing the password
return password_hash($username.$password.$regTime, PASSWORD_DEFAULT);

I can read you know? I even have some knowledge of PHP, so I know what the . means. No nead explaining it. The only thing I just can't understand is why? enlighten me. (See the security part bellow for a more detailed rant)$

catch(Exception $e){
    // In any other event, throw an exception
    throw $e;
}

Good you tell me you are throwing an exception. I had no idea... But why catch it to only throw it? What am I missing here?

// On init, we recieve a UserInfo Object and store it.
// Next we retrieve a PDO Connection from DbService().getDbConnection.
function __construct(DbService $dbService){$this->dbConnection=$dbService->getDbConnection();}

Where is the UserInfo object? Why ask for a DbService if we actually need a PDO object? Why one line? To save on those key-strokes? If you enter-key is hard to press, buy a new keyboard.

// Allowing PDO to throw Exceptions
$this->dbConnection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

Captain obvious...

Comments demistified

Comments are the most important part of your code. The best advice I got in Uni, was the guy in front asking us to start with the comments. In fact, the entire task didnt require any code at all. As long as the comments were there. That was enough. This because everyone can write code. There is even a saying:

Put a thousand monkeys in front of a thousand keyboards and eventually one will write a valid java program. The rest will all write Perl programs

Let's just redo your DbService:

<?php

use DbLoginConsts;
use PDO;

/**
 * Whenever a PDO connection is required, we aqquire the connection by calling new DbSerive()->getDbConnection();
 * This reassures that we are not creating multiple open connections.
 * Class DbService
 */
class DbService
{
    /**
     * @var PDO
     */
    private $dbConnection;

    /**
     * Return a PDO object.
     * If it is not yet created, we create one with the settings defined in DbLoginConsts
     *
     * @return PDO
     */
    public function getDbConnection()
    {
        if (!$this->dbConnection) {
            $dbLoginInfo = new DbLoginConsts();
            $this->dbConnection = new PDO($dbLoginInfo->getUrl(), $dbLoginInfo->getUsername(), $dbLoginInfo->getPassword());
            $this->dbConnection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }

        return $this->dbConnection;
    }

    /**
     * Close the current PDO connection by setting it to null.
     * We rely on garbage collection to remove the object from memory
     */
    public function closeDbConnection()
    {
        $this->dbConnection = null;
    }
}

So, what did I do here? I opened my IDE, and have it format the code for me. Then I added doc blocks. I also removed all useless comments.

I also took the liberty to add @return and @var statements. At the top, I added 2 use statement. This way it is clear what we need for this class to function.

Now we have some proper comments, the code allready looks better. So let's start reading the actual comments to understand what the code is actually doing

This reassures that we are not creating multiple open connections.

You sure? what about:

$connA = new DbService();
$connb = new DbService();

$connA->getDbConnection() =?= $connB->getDbConnection();

Ow, snap. #ToSmartForMyDbService So it seams, or our class it not usefull, or our comments are incorrect. In this case, I fear it is simply a useless class. We tried solving the 'one connection per request' problem with Singletons. But the fact remains: If you only want to create it once, simply only create it once.

$connection = new PDO(); beats $connection = new DbService(); $connection->getDbConnection(); in every aspect. There is noy a single metric where your DbService can beat PDO. It doesn't solve a problem, it only clutters your code with bizar getDbConnection() calls. So, let's redo your DbService properly:

$dbLoginInfo = new DbLoginConsts();
$dbConnection = new PDO($dbLoginInfo->getDsn(), $dbLoginInfo->getUsername(), $dbLoginInfo->getPassword());

I took the liberty of chaing your ->getUrl to a getDsn. PDO doesn't need a url, an url doesn't make any sense here. It does however need a DSN.

I also removed the Exception throwing mode. The only thing you are doing with those exceptions is try - fake catch - throw. This only adds extra white space and key strokes.


Fancy words

The keyword class is one of the most misunderstood tools of about every language that can handle objects. Let's look the word up on wikipedia:

In object-oriented programming, a class is an extensible program-code-template for creating objects, providing initial values for state (member variables) and implementations of behavior

I secretly put the most important parts in bold. Jokes aside, this is really really important. A class is a code-template implementing behavior.

Instead of looking at what you did, let's just take a step back and think about the problem you are solving: Login and Sign-up. Ow snap, there is an and. The english language is telling me there are in fact 2 problems to solve. Let's start by solving the first one:

The User authentication problem

We start by defining some actors in our story:

  • The user: A user has atleast a username and a password
  • The user provider: We need some kind of interface to retrieve users
  • The encoder: We need to store our passwords, but we should encode them somehow.

Now that we have our different actors, lets define the interfaces:

Because I don't like reinventing the wheel, I used a couple if interfaces from the Security component of Symfony

<?php

/**
 * Represents the interface that all user classes must implement.
 *
 * This interface is useful because the authentication layer can deal with
 * the object through its lifecycle, using the object to get the encoded
 * password (for checking against a submitted password), assigning roles
 * and so on.
 *
 * Regardless of how your user are loaded or where they come from (a database,
 * configuration, web service, etc), you will have a class that implements
 * this interface. Objects that implement this interface are created and
 * loaded by different objects that implement UserProviderInterface
 *
 * @see UserProviderInterface
 * @see AdvancedUserInterface
 *
 * @author Fabien Potencier <[email protected]>
 */
interface UserInterface
{

    /**
     * Returns the password used to authenticate the user.
     *
     * This should be the encoded password. On authentication, a plain-text
     * password will be salted, encoded, and then compared to this value.
     *
     * @return string The password
     */
    public function getPassword();

    /**
     * Returns the username used to authenticate the user.
     *
     * @return string The username
     */
    public function getUsername();
}

<?php

/**
 * Represents a class that loads UserInterface objects from some source for the authentication system.
 *
 * In a typical authentication configuration, a username (i.e. some unique
 * user identifier) credential enters the system (via form login, or any
 * method). The user provider that is configured with that authentication
 * method is asked to load the UserInterface object for the given username
 * (via loadUserByUsername) so that the rest of the process can continue.
 *
 * Internally, a user provider can load users from any source (databases,
 * configuration, web service). This is totally independent of how the authentication
 * information is submitted or what the UserInterface object looks like.
 *
 * @see UserInterface
 *
 * @author Fabien Potencier <[email protected]>
 */
interface UserProviderInterface
{
    /**
     * Loads the user for the given username.
     *
     * This method must throw UsernameNotFoundException if the user is not
     * found.
     *
     * @param string $username The username
     *
     * @return UserInterface
     *
     * @see UsernameNotFoundException
     *
     * @throws UsernameNotFoundException if the user is not found
     */
    public function loadUserByUsername($username);
}

<?php

/**
 * PasswordEncoderInterface is the interface for all encoders.
 *
 * @author Fabien Potencier <[email protected]>
 */
interface PasswordEncoderInterface
{
    /**
     * Encodes the raw password.
     *
     * @param string $raw  The password to encode
     * @param string $salt The salt
     *
     * @return string The encoded password
     */
    public function encodePassword($raw, $salt);
    /**
     * Checks a raw password against an encoded password.
     *
     * @param string $encoded An encoded password
     * @param string $raw     A raw password
     * @param string $salt    The salt
     *
     * @return bool true if the password is valid, false otherwise
     */
    public function isPasswordValid($encoded, $raw, $salt);
}

Now that we have our interfaces defined, we can start bringing everything together. Let's start with our UserAuthenticationManager. As the name says it, it's a code-template for a object that has the following behavior: It can Authenticate Users. It will probably look something like this:

class AuthenticationManager
{

    /**
     * @var UserProviderInterface
     */
    private $userProvider;

    /**
     * @var PasswordEncoderInterface
     */
    private $passwordEncoder;

    /**
     * @param UserProviderInterface $userProvider
     * @param PasswordEncoderInterface $passwordEncoder
     */
    public function __construct(UserProviderInterface $userProvider, PasswordEncoderInterface $passwordEncoder)
    {
        $this->userProvider = $userProvider;
        $this->passwordEncoder = $passwordEncoder;
    }

    /**
     * Attempts to authenticate a UserInterface object.
     *
     * @param UserInterface $userToAuthenticate
     *
     * @return UserInterface the authenticated user object
     *
     * @throws AuthenticationException if the authentication fails
     */
    public function authenticate(UserInterface $userToAuthenticate)
    {
        $user = $this->userProvider->loadUserByUsername($userToAuthenticate->getUsername());

        //If no user with this username is found, authentication has failed.
        if (!$user) {
            throw new AuthenticationException('Username not found');
        }

        if (!$this->passwordEncoder->isPasswordValid($user->getPassword(), $userToAuthenticate->getPassword())) {
            throw new AuthenticationException('Password invalid');
        }

        return $user;
    }

}

Things to note here is that our AuthenticationManager litteraly acts like a real-world manager. It knows nothing about the objects he uses to reach his goal. The only thing the manager does is glue the different things together. IT relys on abstraction. I'll leave the implementation of the UserProvider and UserInterface up to you.


Security is not an invention

You specifically talk about security in your question, yet you use some semi half baken mechanism to construct the password that will be hashed. Before you continue, burn that piece of code. Burn the hard drive you wrote it on.

NEVER USE YOUR OWN SECURITY MECHANISM

Let me rephase:

DON'T USE YOUR OWN SECURITY MECHANISM

Now that we have established some ground rules, let's analyse your password hashing 'library'.

password_hash($username.$password.$regTime, PASSWORD_DEFAULT);

Good, you use password_hash. Nice. This is indeed the way to go. Ow, but wait, what is that $username.$password.$regTime thing? I mean, what is it doing there? if we needed all that information, we should have used the username_password_time_hash function.

Let's say I'm a hacker, and I want to crack your password's. The $username can be found in the column next to password, it is nicely named username. The regTime, well. This is a little harder. But after some sniffing around i notice the 'reg_time' column. aaah, the old camelCase to snake_case.

Now that we know that the added security benefit is 0. In fact, when you use for instance BCRYPT as your hashing algo, you have even made your mechanism less safe.

BCRYPT accepts a password of max 72. In your algo this is suddenly 72 - length(reg_time) - length(username). This is probably no big deal for you 6 character passwords. But hey, theoretically your algo is weakened.

So my question remains: why?


No propblem? no code

If you don't have a problem, don't code. Hashing has allready been solved by password_hash and password_verify. Rendering your entire Hahsing class obsolete. It has no behavior, no state and doest act as a template for objects. The only thing it does is delegation to the built in functions.

Im tired now. Even ranting geeks like me need sleep. I might edit/add stuf tomorrow, I might not. I just hope you learned a couple of things with this post

\$\endgroup\$
5
  • 2
    \$\begingroup\$ one of the most hilarious-yet-educative code review I have ever read \$\endgroup\$ Sep 14, 2015 at 23:53
  • \$\begingroup\$ This was very helpful indeed. I will keep the suggestions in mind and redo my code. Thank you. \$\endgroup\$ Sep 16, 2015 at 5:35
  • \$\begingroup\$ Hi, just wanted to discuss a few things 1) Regarding my DbService.php class, the member variable $dbConnection must be static. That prohibits a user from accidentally creating multiple connections, does it not? \$\endgroup\$ Sep 28, 2015 at 17:20
  • \$\begingroup\$ 2)You said “If you only want to create it once, simply only create it once.” – What if you are working along with other developers. Wouldn’t it make it easier since you (or your fellow developers) do not need to worry about duplicate connections? \$\endgroup\$ Sep 28, 2015 at 17:21
  • \$\begingroup\$ Use a dependency injection container for this if it becomes really hard. Symfony has a good one: symfony.com/doc/current/components/dependency_injection/… others will do just fine too. \$\endgroup\$
    – Pinoniq
    Sep 29, 2015 at 8:24

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.