Users management php class
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
2
down vote
favorite
After a little bit of troubleshoots, I've finished this simple users registration and login class. It uses PDO
to connect to a mysql database, I've tried also to manage the session when an user do the login or logout. The main objective of this code form me was to make an exercise for using correctly the PDO
and related functions, and to start improving my coding skills to manage account creations and login using PHP OOP.
<?php
reqiure_once 'config.php';
class User
private $db = null;
private $email;
private $username;
private $password;
private $uid;
private $sessioncode;
public function __construct(PDO $db)
$this->db = $db;
public function createUser($email,$username,$password)
if($this->checkEmail($email))
echo 'Email address already exsist.';
elseif($this->checkUsername($username))
echo 'Username already exsist.';
else
$this->password = password_hash($password,PASSWORD_BCRYPT);
$stmt = $this->db->prepare('INSERT INTO users (email,username,password) VALUES (?, ?, ? )');
if($stmt->execute(array($email,$username,$this->password)))
echo 'Account successful created';
public function loginUser($username,$password)
$stmt = $this->db->prepare('SELECT id,username,password FROM users WHERE username = ?');
$stmt->execute(array($username));
if($stmt->rowCount() > 0)
$result = $stmt->fetch(PDO::FETCH_OBJ);
if(password_verify($password, $result->password))
$this->setSession($result->username,$result->id);
else
echo 'wrong password';
else
echo 'Username or email does not exist in the database.';
public function logoutUser($uid)
$this->unsetSession($uid);
private function checkEmail($email)
$stmt = $this->db->prepare('SELECT email FROM users WHERE email = ?');
$stmt->execute(array($email));
if($stmt->rowCount() > 0)
return true;
else
return false;
private function checkUsername($username)
$stmt = $this->db->prepare('SELECT username FROM users WHERE username = ?');
$stmt->execute(array($username));
if($stmt->rowCount() > 0)
return true;
else
return false;
private function setSession($username,$uid)
$this->sessioncode = bin2hex(rand());
$_SESSION['id'] = $uid;
$_SESSION['username'] = $username;
$_SESSION['session_code'] = $this->sessioncode;
$stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
if($stmt->execute(array($this->sessioncode,$uid)))
header('Location: u/dashboard.php');
private function unsetSession($uid)
$this->uid = $uid;
$this->sessioncode = '';
$stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
if($stmt->execute(array($this->sessioncode,$this->uid)))
session_unset();
session_destroy();
header('Location: ../index.php');
?>
php object-oriented pdo php5
 |Â
show 2 more comments
up vote
2
down vote
favorite
After a little bit of troubleshoots, I've finished this simple users registration and login class. It uses PDO
to connect to a mysql database, I've tried also to manage the session when an user do the login or logout. The main objective of this code form me was to make an exercise for using correctly the PDO
and related functions, and to start improving my coding skills to manage account creations and login using PHP OOP.
<?php
reqiure_once 'config.php';
class User
private $db = null;
private $email;
private $username;
private $password;
private $uid;
private $sessioncode;
public function __construct(PDO $db)
$this->db = $db;
public function createUser($email,$username,$password)
if($this->checkEmail($email))
echo 'Email address already exsist.';
elseif($this->checkUsername($username))
echo 'Username already exsist.';
else
$this->password = password_hash($password,PASSWORD_BCRYPT);
$stmt = $this->db->prepare('INSERT INTO users (email,username,password) VALUES (?, ?, ? )');
if($stmt->execute(array($email,$username,$this->password)))
echo 'Account successful created';
public function loginUser($username,$password)
$stmt = $this->db->prepare('SELECT id,username,password FROM users WHERE username = ?');
$stmt->execute(array($username));
if($stmt->rowCount() > 0)
$result = $stmt->fetch(PDO::FETCH_OBJ);
if(password_verify($password, $result->password))
$this->setSession($result->username,$result->id);
else
echo 'wrong password';
else
echo 'Username or email does not exist in the database.';
public function logoutUser($uid)
$this->unsetSession($uid);
private function checkEmail($email)
$stmt = $this->db->prepare('SELECT email FROM users WHERE email = ?');
$stmt->execute(array($email));
if($stmt->rowCount() > 0)
return true;
else
return false;
private function checkUsername($username)
$stmt = $this->db->prepare('SELECT username FROM users WHERE username = ?');
$stmt->execute(array($username));
if($stmt->rowCount() > 0)
return true;
else
return false;
private function setSession($username,$uid)
$this->sessioncode = bin2hex(rand());
$_SESSION['id'] = $uid;
$_SESSION['username'] = $username;
$_SESSION['session_code'] = $this->sessioncode;
$stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
if($stmt->execute(array($this->sessioncode,$uid)))
header('Location: u/dashboard.php');
private function unsetSession($uid)
$this->uid = $uid;
$this->sessioncode = '';
$stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
if($stmt->execute(array($this->sessioncode,$this->uid)))
session_unset();
session_destroy();
header('Location: ../index.php');
?>
php object-oriented pdo php5
I've got a little suggestion for checkEmail function. as of the rest, did you actually try to implement this class in the actual code? All thoseecho
s look terribly misplaced. Where such an output is supposed to be shown?
â Your Common Sense
Jun 3 at 11:18
@YourCommonSense I wroteecho
s because for now I didn't write theAJAX
code to request the various class taskes suchcreateUser
ecc. so I need a simply output to show when something was wrong. I don't know if can be a good idea to sobstituteecho
s withreturn
for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacingecho
s will be appreciated. I'm also planning to add a method to send verification email, but for now I'm evaluting on how to proceed.
â user9741470
Jun 3 at 11:24
Like I suggested before, the best would be to throw a custom exception, then catch it in the AJAX handling code (outside the class). There is an explanation i wrote in the other question (ider Error reporting section) codereview.stackexchange.com/questions/193023/â¦
â Your Common Sense
Jun 3 at 11:33
1
yes, it's a mistake. like any other tool, try catch has its use, and this is a perfectly legitimate case for it. All other ways to check for error would be more tedious and your current approach is just no-go. Please read my article on MVC, it explains, why a code in Model should never output a single character - you just cannot tell in which context it will be used in the future.
â Your Common Sense
Jun 3 at 12:04
1
as of rowCount() for me it's a matter of sanity: you are selecting some data but not fetching it. For me, it makes no sense. At the same time rowCount() is not guaranteed to be available. After all, fetch() makes uniform processing
â Your Common Sense
Jun 3 at 14:08
 |Â
show 2 more comments
up vote
2
down vote
favorite
up vote
2
down vote
favorite
After a little bit of troubleshoots, I've finished this simple users registration and login class. It uses PDO
to connect to a mysql database, I've tried also to manage the session when an user do the login or logout. The main objective of this code form me was to make an exercise for using correctly the PDO
and related functions, and to start improving my coding skills to manage account creations and login using PHP OOP.
<?php
reqiure_once 'config.php';
class User
private $db = null;
private $email;
private $username;
private $password;
private $uid;
private $sessioncode;
public function __construct(PDO $db)
$this->db = $db;
public function createUser($email,$username,$password)
if($this->checkEmail($email))
echo 'Email address already exsist.';
elseif($this->checkUsername($username))
echo 'Username already exsist.';
else
$this->password = password_hash($password,PASSWORD_BCRYPT);
$stmt = $this->db->prepare('INSERT INTO users (email,username,password) VALUES (?, ?, ? )');
if($stmt->execute(array($email,$username,$this->password)))
echo 'Account successful created';
public function loginUser($username,$password)
$stmt = $this->db->prepare('SELECT id,username,password FROM users WHERE username = ?');
$stmt->execute(array($username));
if($stmt->rowCount() > 0)
$result = $stmt->fetch(PDO::FETCH_OBJ);
if(password_verify($password, $result->password))
$this->setSession($result->username,$result->id);
else
echo 'wrong password';
else
echo 'Username or email does not exist in the database.';
public function logoutUser($uid)
$this->unsetSession($uid);
private function checkEmail($email)
$stmt = $this->db->prepare('SELECT email FROM users WHERE email = ?');
$stmt->execute(array($email));
if($stmt->rowCount() > 0)
return true;
else
return false;
private function checkUsername($username)
$stmt = $this->db->prepare('SELECT username FROM users WHERE username = ?');
$stmt->execute(array($username));
if($stmt->rowCount() > 0)
return true;
else
return false;
private function setSession($username,$uid)
$this->sessioncode = bin2hex(rand());
$_SESSION['id'] = $uid;
$_SESSION['username'] = $username;
$_SESSION['session_code'] = $this->sessioncode;
$stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
if($stmt->execute(array($this->sessioncode,$uid)))
header('Location: u/dashboard.php');
private function unsetSession($uid)
$this->uid = $uid;
$this->sessioncode = '';
$stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
if($stmt->execute(array($this->sessioncode,$this->uid)))
session_unset();
session_destroy();
header('Location: ../index.php');
?>
php object-oriented pdo php5
After a little bit of troubleshoots, I've finished this simple users registration and login class. It uses PDO
to connect to a mysql database, I've tried also to manage the session when an user do the login or logout. The main objective of this code form me was to make an exercise for using correctly the PDO
and related functions, and to start improving my coding skills to manage account creations and login using PHP OOP.
<?php
reqiure_once 'config.php';
class User
private $db = null;
private $email;
private $username;
private $password;
private $uid;
private $sessioncode;
public function __construct(PDO $db)
$this->db = $db;
public function createUser($email,$username,$password)
if($this->checkEmail($email))
echo 'Email address already exsist.';
elseif($this->checkUsername($username))
echo 'Username already exsist.';
else
$this->password = password_hash($password,PASSWORD_BCRYPT);
$stmt = $this->db->prepare('INSERT INTO users (email,username,password) VALUES (?, ?, ? )');
if($stmt->execute(array($email,$username,$this->password)))
echo 'Account successful created';
public function loginUser($username,$password)
$stmt = $this->db->prepare('SELECT id,username,password FROM users WHERE username = ?');
$stmt->execute(array($username));
if($stmt->rowCount() > 0)
$result = $stmt->fetch(PDO::FETCH_OBJ);
if(password_verify($password, $result->password))
$this->setSession($result->username,$result->id);
else
echo 'wrong password';
else
echo 'Username or email does not exist in the database.';
public function logoutUser($uid)
$this->unsetSession($uid);
private function checkEmail($email)
$stmt = $this->db->prepare('SELECT email FROM users WHERE email = ?');
$stmt->execute(array($email));
if($stmt->rowCount() > 0)
return true;
else
return false;
private function checkUsername($username)
$stmt = $this->db->prepare('SELECT username FROM users WHERE username = ?');
$stmt->execute(array($username));
if($stmt->rowCount() > 0)
return true;
else
return false;
private function setSession($username,$uid)
$this->sessioncode = bin2hex(rand());
$_SESSION['id'] = $uid;
$_SESSION['username'] = $username;
$_SESSION['session_code'] = $this->sessioncode;
$stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
if($stmt->execute(array($this->sessioncode,$uid)))
header('Location: u/dashboard.php');
private function unsetSession($uid)
$this->uid = $uid;
$this->sessioncode = '';
$stmt = $this->db->prepare('UPDATE users SET session_code = ? WHERE id = ?');
if($stmt->execute(array($this->sessioncode,$this->uid)))
session_unset();
session_destroy();
header('Location: ../index.php');
?>
php object-oriented pdo php5
asked Jun 3 at 9:38
user9741470
1009
1009
I've got a little suggestion for checkEmail function. as of the rest, did you actually try to implement this class in the actual code? All thoseecho
s look terribly misplaced. Where such an output is supposed to be shown?
â Your Common Sense
Jun 3 at 11:18
@YourCommonSense I wroteecho
s because for now I didn't write theAJAX
code to request the various class taskes suchcreateUser
ecc. so I need a simply output to show when something was wrong. I don't know if can be a good idea to sobstituteecho
s withreturn
for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacingecho
s will be appreciated. I'm also planning to add a method to send verification email, but for now I'm evaluting on how to proceed.
â user9741470
Jun 3 at 11:24
Like I suggested before, the best would be to throw a custom exception, then catch it in the AJAX handling code (outside the class). There is an explanation i wrote in the other question (ider Error reporting section) codereview.stackexchange.com/questions/193023/â¦
â Your Common Sense
Jun 3 at 11:33
1
yes, it's a mistake. like any other tool, try catch has its use, and this is a perfectly legitimate case for it. All other ways to check for error would be more tedious and your current approach is just no-go. Please read my article on MVC, it explains, why a code in Model should never output a single character - you just cannot tell in which context it will be used in the future.
â Your Common Sense
Jun 3 at 12:04
1
as of rowCount() for me it's a matter of sanity: you are selecting some data but not fetching it. For me, it makes no sense. At the same time rowCount() is not guaranteed to be available. After all, fetch() makes uniform processing
â Your Common Sense
Jun 3 at 14:08
 |Â
show 2 more comments
I've got a little suggestion for checkEmail function. as of the rest, did you actually try to implement this class in the actual code? All thoseecho
s look terribly misplaced. Where such an output is supposed to be shown?
â Your Common Sense
Jun 3 at 11:18
@YourCommonSense I wroteecho
s because for now I didn't write theAJAX
code to request the various class taskes suchcreateUser
ecc. so I need a simply output to show when something was wrong. I don't know if can be a good idea to sobstituteecho
s withreturn
for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacingecho
s will be appreciated. I'm also planning to add a method to send verification email, but for now I'm evaluting on how to proceed.
â user9741470
Jun 3 at 11:24
Like I suggested before, the best would be to throw a custom exception, then catch it in the AJAX handling code (outside the class). There is an explanation i wrote in the other question (ider Error reporting section) codereview.stackexchange.com/questions/193023/â¦
â Your Common Sense
Jun 3 at 11:33
1
yes, it's a mistake. like any other tool, try catch has its use, and this is a perfectly legitimate case for it. All other ways to check for error would be more tedious and your current approach is just no-go. Please read my article on MVC, it explains, why a code in Model should never output a single character - you just cannot tell in which context it will be used in the future.
â Your Common Sense
Jun 3 at 12:04
1
as of rowCount() for me it's a matter of sanity: you are selecting some data but not fetching it. For me, it makes no sense. At the same time rowCount() is not guaranteed to be available. After all, fetch() makes uniform processing
â Your Common Sense
Jun 3 at 14:08
I've got a little suggestion for checkEmail function. as of the rest, did you actually try to implement this class in the actual code? All those
echo
s look terribly misplaced. Where such an output is supposed to be shown?â Your Common Sense
Jun 3 at 11:18
I've got a little suggestion for checkEmail function. as of the rest, did you actually try to implement this class in the actual code? All those
echo
s look terribly misplaced. Where such an output is supposed to be shown?â Your Common Sense
Jun 3 at 11:18
@YourCommonSense I wrote
echo
s because for now I didn't write the AJAX
code to request the various class taskes such createUser
ecc. so I need a simply output to show when something was wrong. I don't know if can be a good idea to sobstitute echo
s with return
for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacing echo
s will be appreciated. I'm also planning to add a method to send verification email, but for now I'm evaluting on how to proceed.â user9741470
Jun 3 at 11:24
@YourCommonSense I wrote
echo
s because for now I didn't write the AJAX
code to request the various class taskes such createUser
ecc. so I need a simply output to show when something was wrong. I don't know if can be a good idea to sobstitute echo
s with return
for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacing echo
s will be appreciated. I'm also planning to add a method to send verification email, but for now I'm evaluting on how to proceed.â user9741470
Jun 3 at 11:24
Like I suggested before, the best would be to throw a custom exception, then catch it in the AJAX handling code (outside the class). There is an explanation i wrote in the other question (ider Error reporting section) codereview.stackexchange.com/questions/193023/â¦
â Your Common Sense
Jun 3 at 11:33
Like I suggested before, the best would be to throw a custom exception, then catch it in the AJAX handling code (outside the class). There is an explanation i wrote in the other question (ider Error reporting section) codereview.stackexchange.com/questions/193023/â¦
â Your Common Sense
Jun 3 at 11:33
1
1
yes, it's a mistake. like any other tool, try catch has its use, and this is a perfectly legitimate case for it. All other ways to check for error would be more tedious and your current approach is just no-go. Please read my article on MVC, it explains, why a code in Model should never output a single character - you just cannot tell in which context it will be used in the future.
â Your Common Sense
Jun 3 at 12:04
yes, it's a mistake. like any other tool, try catch has its use, and this is a perfectly legitimate case for it. All other ways to check for error would be more tedious and your current approach is just no-go. Please read my article on MVC, it explains, why a code in Model should never output a single character - you just cannot tell in which context it will be used in the future.
â Your Common Sense
Jun 3 at 12:04
1
1
as of rowCount() for me it's a matter of sanity: you are selecting some data but not fetching it. For me, it makes no sense. At the same time rowCount() is not guaranteed to be available. After all, fetch() makes uniform processing
â Your Common Sense
Jun 3 at 14:08
as of rowCount() for me it's a matter of sanity: you are selecting some data but not fetching it. For me, it makes no sense. At the same time rowCount() is not guaranteed to be available. After all, fetch() makes uniform processing
â Your Common Sense
Jun 3 at 14:08
 |Â
show 2 more comments
2 Answers
2
active
oldest
votes
up vote
2
down vote
Whole user management system is very hard to do properly even with structural code. When you get a grasp of entire logic & security nuances OOP itself takes it to the next level, because this logic will be hard to encapsulate in objects of separate concerns - session, cookies, http request and response, domain models, view models, factories, routing... potential abstraction leaks are everywhere. That said, let's take a look at your class...
From OOP perspective
Output:
This class is not concerned about output - you don't know its format (json/html) nor channel/destination (http/console/log file) - you will validate passed values against your data model, but nothing more. For example when registering new user, the response should be the same user data (withut password - not needed anymore) with user id or class specific validation errors attached like (bool) usernameConflict
, (bool) emailConflict
- that's it. The rest is up to clients of this class (they might not want to reveal users' emails in this indirect way)
Database:
You should separate yourself from database - it's not relevant to your application where data comes from. Use port & adapter technique - interface to query methods (taking only required parameters) and its PDO implementation.
Session:
Session at this point is a global dependency and hidden structural dependency (there needs to be session_start()
somewhere). Remove it from this class and return what it needs instead. It should be something that provides user id & not only that. There will be other parties interested in authentication result, so return user data without password hash and other "secrets".
Handling hashes:
This is the only concern of your class: get credentials for concrete user (defined by username, email, cookie token id) and its hash signature (password hash, persistent cookie token, ...), compare result & return data useful to others - beyond this class hashes should be no use to anyone. You won't have methods covering entire login and logout procedure - just a part of it.
and side-effects...
There will be also some side-effect logic in this class like possible password rehashing on algorithm's change (after succesful match), removing cookie tokens (id+hash) on mismatch, hashing token (id+token is generated outside and passed as response cookie)... etc.
Some security concerns:
- After succesful login session id should be regenerated (see: session
fixation attack) - After failed login/registration - don't inform that same email is in database (mentioned in "output" section). Registering user with existing email might be handled by sending some (merge/restore) options in activation email.
- Session code doesn't protect you from anything - it
all happens inside application environment (only session id goes outside). Holding user's id for authentication (in case user is deactivated/removed) is enough - Make sure that
httpOnly
andSecure
flags are set for session-type cookies (https might be hard to setup for localhost env, but it's neccessary on remote) - All forms should be signed/marked with CSRF tokens - new session id = generate new token is most convenient approach (every form with new token doensn't add much to protection, but causes problems with multiple open tabs, back button...)
- More...
So you suggest to remove the session code and use instead the session id to identify the current active user session, plus renew thesession_id()
on success login right?About the output, I'm removing from the final version of the code all theecho
s, I want to usereturn
s and in case of error as suggested I'm working tocatch()
the errors.
â user9741470
Jun 5 at 19:32
1
@user9741470 I suggested removing session handling from this class but implementing in upper layers (the class that directly/indirectly usues yours).session_id()
is internal mechanism and it just works that way. I was saying that logged inuser_id
is enough to be saved in session storage (next to CSRF tokens) andsession_regenerate_id()
should be called right before that.
â shudder
Jun 5 at 19:54
Perfect, I will follow your suggestions.
â user9741470
Jun 6 at 7:14
add a comment |Â
up vote
1
down vote
Early Returns
I prefer early returns for example
if(!$stmt->rowCount())
echo 'Username or email does not exist in the database.';
return false;
$result = $stmt->fetch(PDO::FETCH_OBJ);
if(password_verify($password, $result->password) !== true)
echo 'wrong password';
return false;
$this->setSession($result->username,$result->id);
Session Ids
Its against best practise (as i understand it) to create your own session id you can very easily get the current sessions id by calling session_id()
PDO Is Not Injectable (without custom definitions)
I like dependency injection (especially auto wiring) so in the future if you were to use auto wiring you would have to add a definition some where for the PDO object so it might be better to put that pdo object in a class so you can get it in the constructor
public function __construct(database $database)
$this->db = $database->dbObject;
// Or
$this->db = $database->getSomeObjectOrThrowSomeException()
but thats just me
The last thing id say which yourcommonsense has already pointed out dont echo in the model
1
For most databases, PDOStatement::rowCount() does not return the number of rows affected by a SELECT statement.
â Your Common Sense
Jun 4 at 11:48
well you could have kept it, as it works for the OP. a good note on the injectability by the way
â Your Common Sense
Jun 4 at 12:57
The id is the user id and not thesession_id()
. I use it to manage the user logout. But I will add also thesession_id()
to the informations stored into the database to make more secure the app. For the Dependency injection, I've tried to Inject aPDO
object but without success, maybe it was a mistake of mine, so I've opted for this solution. See this question stackoverflow.com/questions/50641139/â¦
â user9741470
Jun 4 at 13:04
1
Yeah you wont be able to inject PDO without a definition telling the dependency injection container which PDO instance you want to use. if $this->sessioncode = bin2hex(rand()); is the uesr id you need to re-think your naming convention, you should just use the session id generated by php instead of this (imo)
â Dan
Jun 4 at 13:38
add a comment |Â
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
2
down vote
Whole user management system is very hard to do properly even with structural code. When you get a grasp of entire logic & security nuances OOP itself takes it to the next level, because this logic will be hard to encapsulate in objects of separate concerns - session, cookies, http request and response, domain models, view models, factories, routing... potential abstraction leaks are everywhere. That said, let's take a look at your class...
From OOP perspective
Output:
This class is not concerned about output - you don't know its format (json/html) nor channel/destination (http/console/log file) - you will validate passed values against your data model, but nothing more. For example when registering new user, the response should be the same user data (withut password - not needed anymore) with user id or class specific validation errors attached like (bool) usernameConflict
, (bool) emailConflict
- that's it. The rest is up to clients of this class (they might not want to reveal users' emails in this indirect way)
Database:
You should separate yourself from database - it's not relevant to your application where data comes from. Use port & adapter technique - interface to query methods (taking only required parameters) and its PDO implementation.
Session:
Session at this point is a global dependency and hidden structural dependency (there needs to be session_start()
somewhere). Remove it from this class and return what it needs instead. It should be something that provides user id & not only that. There will be other parties interested in authentication result, so return user data without password hash and other "secrets".
Handling hashes:
This is the only concern of your class: get credentials for concrete user (defined by username, email, cookie token id) and its hash signature (password hash, persistent cookie token, ...), compare result & return data useful to others - beyond this class hashes should be no use to anyone. You won't have methods covering entire login and logout procedure - just a part of it.
and side-effects...
There will be also some side-effect logic in this class like possible password rehashing on algorithm's change (after succesful match), removing cookie tokens (id+hash) on mismatch, hashing token (id+token is generated outside and passed as response cookie)... etc.
Some security concerns:
- After succesful login session id should be regenerated (see: session
fixation attack) - After failed login/registration - don't inform that same email is in database (mentioned in "output" section). Registering user with existing email might be handled by sending some (merge/restore) options in activation email.
- Session code doesn't protect you from anything - it
all happens inside application environment (only session id goes outside). Holding user's id for authentication (in case user is deactivated/removed) is enough - Make sure that
httpOnly
andSecure
flags are set for session-type cookies (https might be hard to setup for localhost env, but it's neccessary on remote) - All forms should be signed/marked with CSRF tokens - new session id = generate new token is most convenient approach (every form with new token doensn't add much to protection, but causes problems with multiple open tabs, back button...)
- More...
So you suggest to remove the session code and use instead the session id to identify the current active user session, plus renew thesession_id()
on success login right?About the output, I'm removing from the final version of the code all theecho
s, I want to usereturn
s and in case of error as suggested I'm working tocatch()
the errors.
â user9741470
Jun 5 at 19:32
1
@user9741470 I suggested removing session handling from this class but implementing in upper layers (the class that directly/indirectly usues yours).session_id()
is internal mechanism and it just works that way. I was saying that logged inuser_id
is enough to be saved in session storage (next to CSRF tokens) andsession_regenerate_id()
should be called right before that.
â shudder
Jun 5 at 19:54
Perfect, I will follow your suggestions.
â user9741470
Jun 6 at 7:14
add a comment |Â
up vote
2
down vote
Whole user management system is very hard to do properly even with structural code. When you get a grasp of entire logic & security nuances OOP itself takes it to the next level, because this logic will be hard to encapsulate in objects of separate concerns - session, cookies, http request and response, domain models, view models, factories, routing... potential abstraction leaks are everywhere. That said, let's take a look at your class...
From OOP perspective
Output:
This class is not concerned about output - you don't know its format (json/html) nor channel/destination (http/console/log file) - you will validate passed values against your data model, but nothing more. For example when registering new user, the response should be the same user data (withut password - not needed anymore) with user id or class specific validation errors attached like (bool) usernameConflict
, (bool) emailConflict
- that's it. The rest is up to clients of this class (they might not want to reveal users' emails in this indirect way)
Database:
You should separate yourself from database - it's not relevant to your application where data comes from. Use port & adapter technique - interface to query methods (taking only required parameters) and its PDO implementation.
Session:
Session at this point is a global dependency and hidden structural dependency (there needs to be session_start()
somewhere). Remove it from this class and return what it needs instead. It should be something that provides user id & not only that. There will be other parties interested in authentication result, so return user data without password hash and other "secrets".
Handling hashes:
This is the only concern of your class: get credentials for concrete user (defined by username, email, cookie token id) and its hash signature (password hash, persistent cookie token, ...), compare result & return data useful to others - beyond this class hashes should be no use to anyone. You won't have methods covering entire login and logout procedure - just a part of it.
and side-effects...
There will be also some side-effect logic in this class like possible password rehashing on algorithm's change (after succesful match), removing cookie tokens (id+hash) on mismatch, hashing token (id+token is generated outside and passed as response cookie)... etc.
Some security concerns:
- After succesful login session id should be regenerated (see: session
fixation attack) - After failed login/registration - don't inform that same email is in database (mentioned in "output" section). Registering user with existing email might be handled by sending some (merge/restore) options in activation email.
- Session code doesn't protect you from anything - it
all happens inside application environment (only session id goes outside). Holding user's id for authentication (in case user is deactivated/removed) is enough - Make sure that
httpOnly
andSecure
flags are set for session-type cookies (https might be hard to setup for localhost env, but it's neccessary on remote) - All forms should be signed/marked with CSRF tokens - new session id = generate new token is most convenient approach (every form with new token doensn't add much to protection, but causes problems with multiple open tabs, back button...)
- More...
So you suggest to remove the session code and use instead the session id to identify the current active user session, plus renew thesession_id()
on success login right?About the output, I'm removing from the final version of the code all theecho
s, I want to usereturn
s and in case of error as suggested I'm working tocatch()
the errors.
â user9741470
Jun 5 at 19:32
1
@user9741470 I suggested removing session handling from this class but implementing in upper layers (the class that directly/indirectly usues yours).session_id()
is internal mechanism and it just works that way. I was saying that logged inuser_id
is enough to be saved in session storage (next to CSRF tokens) andsession_regenerate_id()
should be called right before that.
â shudder
Jun 5 at 19:54
Perfect, I will follow your suggestions.
â user9741470
Jun 6 at 7:14
add a comment |Â
up vote
2
down vote
up vote
2
down vote
Whole user management system is very hard to do properly even with structural code. When you get a grasp of entire logic & security nuances OOP itself takes it to the next level, because this logic will be hard to encapsulate in objects of separate concerns - session, cookies, http request and response, domain models, view models, factories, routing... potential abstraction leaks are everywhere. That said, let's take a look at your class...
From OOP perspective
Output:
This class is not concerned about output - you don't know its format (json/html) nor channel/destination (http/console/log file) - you will validate passed values against your data model, but nothing more. For example when registering new user, the response should be the same user data (withut password - not needed anymore) with user id or class specific validation errors attached like (bool) usernameConflict
, (bool) emailConflict
- that's it. The rest is up to clients of this class (they might not want to reveal users' emails in this indirect way)
Database:
You should separate yourself from database - it's not relevant to your application where data comes from. Use port & adapter technique - interface to query methods (taking only required parameters) and its PDO implementation.
Session:
Session at this point is a global dependency and hidden structural dependency (there needs to be session_start()
somewhere). Remove it from this class and return what it needs instead. It should be something that provides user id & not only that. There will be other parties interested in authentication result, so return user data without password hash and other "secrets".
Handling hashes:
This is the only concern of your class: get credentials for concrete user (defined by username, email, cookie token id) and its hash signature (password hash, persistent cookie token, ...), compare result & return data useful to others - beyond this class hashes should be no use to anyone. You won't have methods covering entire login and logout procedure - just a part of it.
and side-effects...
There will be also some side-effect logic in this class like possible password rehashing on algorithm's change (after succesful match), removing cookie tokens (id+hash) on mismatch, hashing token (id+token is generated outside and passed as response cookie)... etc.
Some security concerns:
- After succesful login session id should be regenerated (see: session
fixation attack) - After failed login/registration - don't inform that same email is in database (mentioned in "output" section). Registering user with existing email might be handled by sending some (merge/restore) options in activation email.
- Session code doesn't protect you from anything - it
all happens inside application environment (only session id goes outside). Holding user's id for authentication (in case user is deactivated/removed) is enough - Make sure that
httpOnly
andSecure
flags are set for session-type cookies (https might be hard to setup for localhost env, but it's neccessary on remote) - All forms should be signed/marked with CSRF tokens - new session id = generate new token is most convenient approach (every form with new token doensn't add much to protection, but causes problems with multiple open tabs, back button...)
- More...
Whole user management system is very hard to do properly even with structural code. When you get a grasp of entire logic & security nuances OOP itself takes it to the next level, because this logic will be hard to encapsulate in objects of separate concerns - session, cookies, http request and response, domain models, view models, factories, routing... potential abstraction leaks are everywhere. That said, let's take a look at your class...
From OOP perspective
Output:
This class is not concerned about output - you don't know its format (json/html) nor channel/destination (http/console/log file) - you will validate passed values against your data model, but nothing more. For example when registering new user, the response should be the same user data (withut password - not needed anymore) with user id or class specific validation errors attached like (bool) usernameConflict
, (bool) emailConflict
- that's it. The rest is up to clients of this class (they might not want to reveal users' emails in this indirect way)
Database:
You should separate yourself from database - it's not relevant to your application where data comes from. Use port & adapter technique - interface to query methods (taking only required parameters) and its PDO implementation.
Session:
Session at this point is a global dependency and hidden structural dependency (there needs to be session_start()
somewhere). Remove it from this class and return what it needs instead. It should be something that provides user id & not only that. There will be other parties interested in authentication result, so return user data without password hash and other "secrets".
Handling hashes:
This is the only concern of your class: get credentials for concrete user (defined by username, email, cookie token id) and its hash signature (password hash, persistent cookie token, ...), compare result & return data useful to others - beyond this class hashes should be no use to anyone. You won't have methods covering entire login and logout procedure - just a part of it.
and side-effects...
There will be also some side-effect logic in this class like possible password rehashing on algorithm's change (after succesful match), removing cookie tokens (id+hash) on mismatch, hashing token (id+token is generated outside and passed as response cookie)... etc.
Some security concerns:
- After succesful login session id should be regenerated (see: session
fixation attack) - After failed login/registration - don't inform that same email is in database (mentioned in "output" section). Registering user with existing email might be handled by sending some (merge/restore) options in activation email.
- Session code doesn't protect you from anything - it
all happens inside application environment (only session id goes outside). Holding user's id for authentication (in case user is deactivated/removed) is enough - Make sure that
httpOnly
andSecure
flags are set for session-type cookies (https might be hard to setup for localhost env, but it's neccessary on remote) - All forms should be signed/marked with CSRF tokens - new session id = generate new token is most convenient approach (every form with new token doensn't add much to protection, but causes problems with multiple open tabs, back button...)
- More...
edited Jun 5 at 12:48
answered Jun 5 at 11:44
shudder
59668
59668
So you suggest to remove the session code and use instead the session id to identify the current active user session, plus renew thesession_id()
on success login right?About the output, I'm removing from the final version of the code all theecho
s, I want to usereturn
s and in case of error as suggested I'm working tocatch()
the errors.
â user9741470
Jun 5 at 19:32
1
@user9741470 I suggested removing session handling from this class but implementing in upper layers (the class that directly/indirectly usues yours).session_id()
is internal mechanism and it just works that way. I was saying that logged inuser_id
is enough to be saved in session storage (next to CSRF tokens) andsession_regenerate_id()
should be called right before that.
â shudder
Jun 5 at 19:54
Perfect, I will follow your suggestions.
â user9741470
Jun 6 at 7:14
add a comment |Â
So you suggest to remove the session code and use instead the session id to identify the current active user session, plus renew thesession_id()
on success login right?About the output, I'm removing from the final version of the code all theecho
s, I want to usereturn
s and in case of error as suggested I'm working tocatch()
the errors.
â user9741470
Jun 5 at 19:32
1
@user9741470 I suggested removing session handling from this class but implementing in upper layers (the class that directly/indirectly usues yours).session_id()
is internal mechanism and it just works that way. I was saying that logged inuser_id
is enough to be saved in session storage (next to CSRF tokens) andsession_regenerate_id()
should be called right before that.
â shudder
Jun 5 at 19:54
Perfect, I will follow your suggestions.
â user9741470
Jun 6 at 7:14
So you suggest to remove the session code and use instead the session id to identify the current active user session, plus renew the
session_id()
on success login right?About the output, I'm removing from the final version of the code all the echo
s, I want to use return
s and in case of error as suggested I'm working to catch()
the errors.â user9741470
Jun 5 at 19:32
So you suggest to remove the session code and use instead the session id to identify the current active user session, plus renew the
session_id()
on success login right?About the output, I'm removing from the final version of the code all the echo
s, I want to use return
s and in case of error as suggested I'm working to catch()
the errors.â user9741470
Jun 5 at 19:32
1
1
@user9741470 I suggested removing session handling from this class but implementing in upper layers (the class that directly/indirectly usues yours).
session_id()
is internal mechanism and it just works that way. I was saying that logged in user_id
is enough to be saved in session storage (next to CSRF tokens) and session_regenerate_id()
should be called right before that.â shudder
Jun 5 at 19:54
@user9741470 I suggested removing session handling from this class but implementing in upper layers (the class that directly/indirectly usues yours).
session_id()
is internal mechanism and it just works that way. I was saying that logged in user_id
is enough to be saved in session storage (next to CSRF tokens) and session_regenerate_id()
should be called right before that.â shudder
Jun 5 at 19:54
Perfect, I will follow your suggestions.
â user9741470
Jun 6 at 7:14
Perfect, I will follow your suggestions.
â user9741470
Jun 6 at 7:14
add a comment |Â
up vote
1
down vote
Early Returns
I prefer early returns for example
if(!$stmt->rowCount())
echo 'Username or email does not exist in the database.';
return false;
$result = $stmt->fetch(PDO::FETCH_OBJ);
if(password_verify($password, $result->password) !== true)
echo 'wrong password';
return false;
$this->setSession($result->username,$result->id);
Session Ids
Its against best practise (as i understand it) to create your own session id you can very easily get the current sessions id by calling session_id()
PDO Is Not Injectable (without custom definitions)
I like dependency injection (especially auto wiring) so in the future if you were to use auto wiring you would have to add a definition some where for the PDO object so it might be better to put that pdo object in a class so you can get it in the constructor
public function __construct(database $database)
$this->db = $database->dbObject;
// Or
$this->db = $database->getSomeObjectOrThrowSomeException()
but thats just me
The last thing id say which yourcommonsense has already pointed out dont echo in the model
1
For most databases, PDOStatement::rowCount() does not return the number of rows affected by a SELECT statement.
â Your Common Sense
Jun 4 at 11:48
well you could have kept it, as it works for the OP. a good note on the injectability by the way
â Your Common Sense
Jun 4 at 12:57
The id is the user id and not thesession_id()
. I use it to manage the user logout. But I will add also thesession_id()
to the informations stored into the database to make more secure the app. For the Dependency injection, I've tried to Inject aPDO
object but without success, maybe it was a mistake of mine, so I've opted for this solution. See this question stackoverflow.com/questions/50641139/â¦
â user9741470
Jun 4 at 13:04
1
Yeah you wont be able to inject PDO without a definition telling the dependency injection container which PDO instance you want to use. if $this->sessioncode = bin2hex(rand()); is the uesr id you need to re-think your naming convention, you should just use the session id generated by php instead of this (imo)
â Dan
Jun 4 at 13:38
add a comment |Â
up vote
1
down vote
Early Returns
I prefer early returns for example
if(!$stmt->rowCount())
echo 'Username or email does not exist in the database.';
return false;
$result = $stmt->fetch(PDO::FETCH_OBJ);
if(password_verify($password, $result->password) !== true)
echo 'wrong password';
return false;
$this->setSession($result->username,$result->id);
Session Ids
Its against best practise (as i understand it) to create your own session id you can very easily get the current sessions id by calling session_id()
PDO Is Not Injectable (without custom definitions)
I like dependency injection (especially auto wiring) so in the future if you were to use auto wiring you would have to add a definition some where for the PDO object so it might be better to put that pdo object in a class so you can get it in the constructor
public function __construct(database $database)
$this->db = $database->dbObject;
// Or
$this->db = $database->getSomeObjectOrThrowSomeException()
but thats just me
The last thing id say which yourcommonsense has already pointed out dont echo in the model
1
For most databases, PDOStatement::rowCount() does not return the number of rows affected by a SELECT statement.
â Your Common Sense
Jun 4 at 11:48
well you could have kept it, as it works for the OP. a good note on the injectability by the way
â Your Common Sense
Jun 4 at 12:57
The id is the user id and not thesession_id()
. I use it to manage the user logout. But I will add also thesession_id()
to the informations stored into the database to make more secure the app. For the Dependency injection, I've tried to Inject aPDO
object but without success, maybe it was a mistake of mine, so I've opted for this solution. See this question stackoverflow.com/questions/50641139/â¦
â user9741470
Jun 4 at 13:04
1
Yeah you wont be able to inject PDO without a definition telling the dependency injection container which PDO instance you want to use. if $this->sessioncode = bin2hex(rand()); is the uesr id you need to re-think your naming convention, you should just use the session id generated by php instead of this (imo)
â Dan
Jun 4 at 13:38
add a comment |Â
up vote
1
down vote
up vote
1
down vote
Early Returns
I prefer early returns for example
if(!$stmt->rowCount())
echo 'Username or email does not exist in the database.';
return false;
$result = $stmt->fetch(PDO::FETCH_OBJ);
if(password_verify($password, $result->password) !== true)
echo 'wrong password';
return false;
$this->setSession($result->username,$result->id);
Session Ids
Its against best practise (as i understand it) to create your own session id you can very easily get the current sessions id by calling session_id()
PDO Is Not Injectable (without custom definitions)
I like dependency injection (especially auto wiring) so in the future if you were to use auto wiring you would have to add a definition some where for the PDO object so it might be better to put that pdo object in a class so you can get it in the constructor
public function __construct(database $database)
$this->db = $database->dbObject;
// Or
$this->db = $database->getSomeObjectOrThrowSomeException()
but thats just me
The last thing id say which yourcommonsense has already pointed out dont echo in the model
Early Returns
I prefer early returns for example
if(!$stmt->rowCount())
echo 'Username or email does not exist in the database.';
return false;
$result = $stmt->fetch(PDO::FETCH_OBJ);
if(password_verify($password, $result->password) !== true)
echo 'wrong password';
return false;
$this->setSession($result->username,$result->id);
Session Ids
Its against best practise (as i understand it) to create your own session id you can very easily get the current sessions id by calling session_id()
PDO Is Not Injectable (without custom definitions)
I like dependency injection (especially auto wiring) so in the future if you were to use auto wiring you would have to add a definition some where for the PDO object so it might be better to put that pdo object in a class so you can get it in the constructor
public function __construct(database $database)
$this->db = $database->dbObject;
// Or
$this->db = $database->getSomeObjectOrThrowSomeException()
but thats just me
The last thing id say which yourcommonsense has already pointed out dont echo in the model
edited Jun 4 at 12:56
answered Jun 4 at 11:30
Dan
373211
373211
1
For most databases, PDOStatement::rowCount() does not return the number of rows affected by a SELECT statement.
â Your Common Sense
Jun 4 at 11:48
well you could have kept it, as it works for the OP. a good note on the injectability by the way
â Your Common Sense
Jun 4 at 12:57
The id is the user id and not thesession_id()
. I use it to manage the user logout. But I will add also thesession_id()
to the informations stored into the database to make more secure the app. For the Dependency injection, I've tried to Inject aPDO
object but without success, maybe it was a mistake of mine, so I've opted for this solution. See this question stackoverflow.com/questions/50641139/â¦
â user9741470
Jun 4 at 13:04
1
Yeah you wont be able to inject PDO without a definition telling the dependency injection container which PDO instance you want to use. if $this->sessioncode = bin2hex(rand()); is the uesr id you need to re-think your naming convention, you should just use the session id generated by php instead of this (imo)
â Dan
Jun 4 at 13:38
add a comment |Â
1
For most databases, PDOStatement::rowCount() does not return the number of rows affected by a SELECT statement.
â Your Common Sense
Jun 4 at 11:48
well you could have kept it, as it works for the OP. a good note on the injectability by the way
â Your Common Sense
Jun 4 at 12:57
The id is the user id and not thesession_id()
. I use it to manage the user logout. But I will add also thesession_id()
to the informations stored into the database to make more secure the app. For the Dependency injection, I've tried to Inject aPDO
object but without success, maybe it was a mistake of mine, so I've opted for this solution. See this question stackoverflow.com/questions/50641139/â¦
â user9741470
Jun 4 at 13:04
1
Yeah you wont be able to inject PDO without a definition telling the dependency injection container which PDO instance you want to use. if $this->sessioncode = bin2hex(rand()); is the uesr id you need to re-think your naming convention, you should just use the session id generated by php instead of this (imo)
â Dan
Jun 4 at 13:38
1
1
For most databases, PDOStatement::rowCount() does not return the number of rows affected by a SELECT statement.
â Your Common Sense
Jun 4 at 11:48
For most databases, PDOStatement::rowCount() does not return the number of rows affected by a SELECT statement.
â Your Common Sense
Jun 4 at 11:48
well you could have kept it, as it works for the OP. a good note on the injectability by the way
â Your Common Sense
Jun 4 at 12:57
well you could have kept it, as it works for the OP. a good note on the injectability by the way
â Your Common Sense
Jun 4 at 12:57
The id is the user id and not the
session_id()
. I use it to manage the user logout. But I will add also the session_id()
to the informations stored into the database to make more secure the app. For the Dependency injection, I've tried to Inject a PDO
object but without success, maybe it was a mistake of mine, so I've opted for this solution. See this question stackoverflow.com/questions/50641139/â¦â user9741470
Jun 4 at 13:04
The id is the user id and not the
session_id()
. I use it to manage the user logout. But I will add also the session_id()
to the informations stored into the database to make more secure the app. For the Dependency injection, I've tried to Inject a PDO
object but without success, maybe it was a mistake of mine, so I've opted for this solution. See this question stackoverflow.com/questions/50641139/â¦â user9741470
Jun 4 at 13:04
1
1
Yeah you wont be able to inject PDO without a definition telling the dependency injection container which PDO instance you want to use. if $this->sessioncode = bin2hex(rand()); is the uesr id you need to re-think your naming convention, you should just use the session id generated by php instead of this (imo)
â Dan
Jun 4 at 13:38
Yeah you wont be able to inject PDO without a definition telling the dependency injection container which PDO instance you want to use. if $this->sessioncode = bin2hex(rand()); is the uesr id you need to re-think your naming convention, you should just use the session id generated by php instead of this (imo)
â Dan
Jun 4 at 13:38
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f195738%2fusers-management-php-class%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
I've got a little suggestion for checkEmail function. as of the rest, did you actually try to implement this class in the actual code? All those
echo
s look terribly misplaced. Where such an output is supposed to be shown?â Your Common Sense
Jun 3 at 11:18
@YourCommonSense I wrote
echo
s because for now I didn't write theAJAX
code to request the various class taskes suchcreateUser
ecc. so I need a simply output to show when something was wrong. I don't know if can be a good idea to sobstituteecho
s withreturn
for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacingecho
s will be appreciated. I'm also planning to add a method to send verification email, but for now I'm evaluting on how to proceed.â user9741470
Jun 3 at 11:24
Like I suggested before, the best would be to throw a custom exception, then catch it in the AJAX handling code (outside the class). There is an explanation i wrote in the other question (ider Error reporting section) codereview.stackexchange.com/questions/193023/â¦
â Your Common Sense
Jun 3 at 11:33
1
yes, it's a mistake. like any other tool, try catch has its use, and this is a perfectly legitimate case for it. All other ways to check for error would be more tedious and your current approach is just no-go. Please read my article on MVC, it explains, why a code in Model should never output a single character - you just cannot tell in which context it will be used in the future.
â Your Common Sense
Jun 3 at 12:04
1
as of rowCount() for me it's a matter of sanity: you are selecting some data but not fetching it. For me, it makes no sense. At the same time rowCount() is not guaranteed to be available. After all, fetch() makes uniform processing
â Your Common Sense
Jun 3 at 14:08