Madara Uchiha sums up some problems.
Here are some more:
A methods containing multiple if's smells. It makes testing harder since there are more edge cases. It also creates a mess where none should be.
For instance your
getError method. A better approach would be:
And if you want it to return
false if no error has been set:
private $error = false; //defaults to false
This makes it a private variable (as it should be) instead of it being added on run-time (what you are doing now).
userLogin method also contains a lot of
if's. And no, you are giving away to much info. Don't tell what was wrong (username or password). Only tell that the combination is wrong. This way, you don't give away any information. So:
public function userLogin($user, $pass)
if( $this->checkUser($user) && $this->checkPass($pass) )
$_SESSION['memberName'] = $this->user;
I also change
return true. What you are doing is evaluating true. Pointless.
Only take what you need
Every time you write
SELECT *, a cute small kitten dies. You only need the
SELECT userName, hash, salt FROM members ...
Then you should add some parameter binding spices and your all good to go!
Then, what happens after is weird. We know that if a user exists, only 1 should. So we can add a
LIMIT 1 statement to the SQL. This way, the database will stop searching once it has found 1 member instead of continuing to search the entire members table.
After that simply doing this:
if ( null === ($userData = $result->fetch_assoc()) )
$this->user = $userData['userName'];
$this->user = $userData['salt'];
$this->user = $userData['hash'];
is cleaner, better to read and does the exact same thing.
checkPass function has the same problem: you evaluate something to a boolean. Then you evaluate that boolean in the
if-condition and then you return an evaluated boolean. A better method is:
public function checkPass($password)
return (hash_hmac("sha256", $password, $this->salt) === $this->hash);
And that is the code review.
Your code is OO, you are using Objects and stuff and the class keyword. But I think what you are asking is: is it good OO?
No it isn't
A good place to start is SOLID. Starting with Single responsibility.
Every class should have one single responsibility. Is it authentication? or is it retrieving user-data? or it it connection to a database? Your class does a little bit of all three.
A good place to start when writing OO-code is in starting with your controllers first. Here you should write how you would like to react with you application. for instance:
if ( $auth->login($username, $password) )
print 'welcome master';
Here we know that we want an interface with 2 methods. login and getError()
public function login($uuid, $pass);
public function getError();
and then implement that interface. For instance a UserAuthentication class that authenticates using users. Later on you could change that with a LDAPAuthentication class without having to worry that the rest of your app breaks. As long as the interface is the same ;)