Users management php class

The name of the pictureThe name of the pictureThe name of the pictureClash 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');




?>






share|improve this question



















  • 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 echos look terribly misplaced. Where such an output is supposed to be shown?
    – Your Common Sense
    Jun 3 at 11:18










  • @YourCommonSense I wrote echos 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 echos with return for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacing echos 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
















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');




?>






share|improve this question



















  • 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 echos look terribly misplaced. Where such an output is supposed to be shown?
    – Your Common Sense
    Jun 3 at 11:18










  • @YourCommonSense I wrote echos 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 echos with return for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacing echos 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












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');




?>






share|improve this question











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');




?>








share|improve this question










share|improve this question




share|improve this question









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 those echos look terribly misplaced. Where such an output is supposed to be shown?
    – Your Common Sense
    Jun 3 at 11:18










  • @YourCommonSense I wrote echos 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 echos with return for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacing echos 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 echos look terribly misplaced. Where such an output is supposed to be shown?
    – Your Common Sense
    Jun 3 at 11:18










  • @YourCommonSense I wrote echos 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 echos with return for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacing echos 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 echos 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 echos look terribly misplaced. Where such an output is supposed to be shown?
– Your Common Sense
Jun 3 at 11:18












@YourCommonSense I wrote echos 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 echos with return for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacing echos 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 echos 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 echos with return for now. The code will be implemented only for login scope but will no interact with other classes. any suggestion for replacing echos 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










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 and Secure 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...





share|improve this answer























  • 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 echos, I want to use returns and in case of error as suggested I'm working to catch() 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 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

















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






share|improve this answer



















  • 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 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




    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










Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















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






























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 and Secure 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...





share|improve this answer























  • 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 echos, I want to use returns and in case of error as suggested I'm working to catch() 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 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














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 and Secure 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...





share|improve this answer























  • 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 echos, I want to use returns and in case of error as suggested I'm working to catch() 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 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












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 and Secure 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...





share|improve this answer















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 and Secure 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...






share|improve this answer















share|improve this answer



share|improve this answer








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 the session_id() on success login right?About the output, I'm removing from the final version of the code all the echos, I want to use returns and in case of error as suggested I'm working to catch() 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 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
















  • 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 echos, I want to use returns and in case of error as suggested I'm working to catch() 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 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















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 echos, I want to use returns 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 echos, I want to use returns 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












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






share|improve this answer



















  • 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 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




    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














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






share|improve this answer



















  • 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 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




    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












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






share|improve this answer















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







share|improve this answer















share|improve this answer



share|improve this answer








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 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




    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




    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 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




    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












 

draft saved


draft discarded


























 


draft saved


draft discarded














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













































































Popular posts from this blog

Greedy Best First Search implementation in Rust

Function to Return a JSON Like Objects Using VBA Collections and Arrays

C++11 CLH Lock Implementation