What I don't care about, is how secure this login class is, because I know it's not. But it keeps out the rare unwanted guests because I do not want to have my information public. I made this mainly for learning purposes. I want to know how "OOP" this is and what I could do differently.

    if($_SERVER['REQUEST_METHOD'] == 'POST') {$user       = $_POST['username'];$pass       = $_POST['password']; if($login->userLogin($user,$pass))
{
echo "Successfully logged in!";
}
else {
echo $login->getError(); } }  Login Class: class UserLogin { private$id;
private $user; private$pass;
private $salt; private$hash;
private $error; protected$mysqli;

public function __construct()
{
$this->mysqli = mysqliSingleton::init(); } function setError($val)
{
$this->error =$val;
}

function getError()
{
return $this->error; } public function userLogin($user, $pass) { if($this->checkUser($user) &&$this->checkPass($pass)) {$_SESSION['memberID']   = $this->id;$_SESSION['memberName'] = $this->user; return true; } else {$this->setError("Invalid username or password");
}
}

public function checkUser($user) {$stmt = $this->mysqli->prepare("SELECT memberID, userName, salt, hash FROM members WHERE userName = ? LIMIT 1");$stmt->bind_param("s",$user);$stmt->execute();
$stmt->bind_result($this->id, $this->user,$this->salt, $this->hash); if (null === ($stmt->fetch()))
{
return false;
}
return true;
}

public function checkPass($password) { return (hash_hmac("sha256",$password, $this->salt) ===$this->hash);
}
}

-

Madara Uchiha sums up some problems.

Here are some more:

## To many if

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:

function getError()
{
return $this->error; }  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).

Then, your 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;
return true;
}
else
{
{
return (hash_hmac("sha256", $password,$this->salt) === $this->hash); }  And that is the code review. # OO 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';
}
else
{
print $auth->getError(); }  Here we know that we want an interface with 2 methods. login and getError() interface Authentication { 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 ;) - Lovely, thanks :) I am already checking the prepared statements for now, and I will check your feedback soon as well to learn more, and I will change my script accordingly :) Thanks a lot! – Ron Brouwers Jul 31 '14 at 8:06 I have changed the class a bit after reading up on some advice that you and Madara Uchiha posted. Using a prepared statement now and removed a lot of "ifs". Unfortunately I cannot use password_hash yet since my server isn't running PHP 5.5 yet. – Ron Brouwers Jul 31 '14 at 10:54 looks a lot cleaner now :) – Pinoniq Jul 31 '14 at 11:47 RE: SELECT *. No kittens were harmed in the making of this SQL statement. Although in the future when you add a column to that table and forget to add the column name to your one or more SELECT statements, a constant stream of profanity will flow from you mouth as you try tracking down why on Earth would this new column not show up on your web site. I personally group the argument to not use SELECT * into the "premature optimizations" department. I'll only limit the columns I select if and only if I really find a performance or architectural issue. – Greg Burghardt Jan 16 at 13:28 It's not about prematuer optimization but about contracts. If you don't need it, dont ask for it. If you add a field 'age' to your user table, you AUTH library still doesn't need it. So don't query it. – Pinoniq Jan 16 at 13:39 Yes, there are several things I can see with your implementation, from most critical to less critical: • SQL Injection - You has it! Since you're already using mysqli, learn about prepared statements with mysqli and apply them. You are very vulnerable to Little Bobby Tables. • Your password hashing is not secure - sha256 is insecure for password hashing in on itself, you're also apply a $this->salt which isn't defined anywhere. See password_hash() and password_verify() for better, more secure password hashing, which includes iteration over the hash, use of proper hashing algorithms (such as CRYPT_BLOWFISH), and simpler and more readable UI.
• Your class does too many things - Why is it the business of a login class, that's supposed to handle users, to create a database connection instance? Why not just pass it in?

public function __construct(mysqli $databaseConnection)  This way, if you ever change your credentials or even move to a different server, you won't need to change your class's code, but only the code which is responsible for creating the connection. That makes your class reusable. • Naming conventions - It's common to use CapitalCase for class and object names, and camelCase for variables and functions. - the salt is defined in the checkUser() method. But all the rest is true! – Pinoniq Jul 31 '14 at 7:38 Thanks, This is useful and I can work with this :) I will take a look into everything you said. Thanks a lot :) – Ron Brouwers Jul 31 '14 at 7:39 @RonBrouwers "Why is it the business of a login class, that's supposed to handle users, to create a database connection instance? Why not just pass it in?" <- In programming parlance this is referred to as 'Dependency Injection'. Just a quick tip FYI. – Mark Jul 31 '14 at 15:20 The LIMIT 1 on your query looks suspicious. LIMIT is rarely useful without ORDER BY, and you didn't specify ORDER BY. Have you either made the username column the primary key of the members table, or put a UNIQUE constraint on the column? If you haven't already done so, I recommend it, to let the database help you enforce data integrity. - Yes, the I have a "memberID" which is unique. – Ron Brouwers Jul 31 '14 at 10:20 I also add a LIMIT 1 to some queries, just so that I don't have to worry about an edge-case, where someone disables the primary-key constrained, or does something evil with data-load and deferred constraints... - LIMIT 1 clearly signals to the reader you only get one row and don't have to check. I think it's ok together with a primary key – Falco Jul 31 '14 at 11:57 A couple of things I smell: • Returning multiple values at once (aka returning a "Tuple") means your methods won't need as many side-effects. • In PHP, you accomplish this by returning an array and using the list function to assign the return value to multiple variables at once. • Keep your method implementations to a Single Level of Abstraction (Clean Code, p. 36). • For example, in your "userLogin" method, you're using both higher-level abstractions (checkUser() and checkPass()) and low-level abstractions ($_SESSION[]).
• To fix this, use the Extract Method refactor.

For example:

//Login form:
...
list($success,$error) = $login->userLogin($user, $pass); echo$success ? 'Success!' : $error; ... //Login class: ... /** * Attempts a user login. * Side effects: if successful, saves the current user in$_SESSION.
* @return A tuple of [ $successful:BOOLEAN,$error:string ]
*/
public function userLogin($user,$pass) {
if ($this->checkUser($user) && $this->checkPass($pass)) {
$this->somethingThatSetsUpTheCurrentSessionMember();$successful = TRUE;
$error = NULL; } else {$successful = FALSE;
$error = 'Invalid username or password'; } return array($successful, \$error);
}
...

-
Wow thanks, didn't know this :) It's good to learn this stuff. –  Ron Brouwers Jul 31 '14 at 16:43