Login Authentication & Sign Up

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;

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:

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

StackExchange Code Review Q#104545, answer score: 2