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