Skip to main content
added 55 characters in body
Source Link
Mike Brant
  • 9.6k
  • 13
  • 23
  • Consider user experience around registration. Why would you want to send them to login form after registration? Presumably if you had valid information for a registration, then you could just put user directly into logged in state, rather than forcing them to then log in.
  • Don't pass log in/account creation state via GET. This is way to easy to hack. This information should always be in session. For example, say you had successful registration and wanted to then redirect user to home page (rather than homelogin page). You could store an account created flag either in session or in the theoretical UserSessionManager class which could be checked to see if any messaging should be displayed to the user and then flag turned off.
  • You should consider more significant validation of user input fields:
  • Do fields like username and password need to be a minimum length or have any constraints on characters it can use?
  • Don't nest validation in if-else conditionals. Why not validate all field with every pass so you can give user full feedback at once.
  • For registration, consider adding password verification field so you can help user mitigate typos in their password.
  • Consider using more robust validation class, or perhaps have static validation methods on user class, so that you can simply call centralized validation functions rather than having validation logic exist on every page.
  • Consider whether you REALLY want to tell users if there name or email matches/doesn't match the system on failed registration/login attempts. Knowing whether a user account exists or not is a valuable piece of potential information for an attacker. Many developers prefer the more vague "user account could not be create" or "login not successful" which are more non-specific.
  • If you don't REALLY need the functionality noted above, you can do away with needing to try perform a select before andthe insert when registering an account. If your table has proper unique indexedindex on user name and user email, an insert that would violate that uniqueness constraint will be unsuccessful. You just need to be able to handle that use case in your method that inserts the user record. This eliminates 50% of your SQL calls for registration use case.
  • If you choose to keep the potentially unnecessary select before insert, you should at least move the functionality to lookup a user by email or user name into a class method and out of the logic of the registration page.
  • Consider using empty() or !empty to check "truthiness"/"falsiness" of variables for validation rather than == ''. You should ideally only use loose comparison for very specific reasons, defaulting your code writing approach to using strict comparisons as a default. This makes your code less fragile to "truthiness"/"falsiness" bugs and clearer to read with regards to intent. For me, if I run into a case where it makes sense to use a loose comparison, I make sure to add a comment in code as to why I this made decision.
  • Consider user experience around registration. Why would you want to send them to login form after registration? Presumably if you had valid information for a registration, then you could just put user directly into logged in state, rather than forcing them to then log in.
  • Don't pass log in/account creation state via GET. This is way to easy to hack. This information should always be in session. For example, say you had successful registration and wanted to then redirect user to home page (rather than home page). You could store an account created flag either in session or in the theoretical UserSessionManager class which could be checked to see if any messaging should be displayed to the user and then turned off.
  • You should consider more significant validation of user input fields:
  • Do fields like username and password need to be a minimum length or have any constraints on characters it can use?
  • Don't nest validation in if-else conditionals. Why not validate all field with every pass so you can give user full feedback at once.
  • For registration, consider adding password verification field so you can help user mitigate typos in their password.
  • Consider using more robust validation class, or perhaps have static validation methods on user class, so that you can simply call centralized validation functions rather than having validation logic exist on every page.
  • Consider whether you REALLY want to tell users if there name or email matches/doesn't match the system on failed registration/login attempts. Knowing whether a user account exists or not is a valuable piece of potential information for an attacker. Many developers prefer the more vague "user account could not be create" or "login not successful" which are more non-specific.
  • If you don't REALLY need the functionality noted above, you can do away with needing to try perform a select before and insert when registering an account. If your table has proper unique indexed on user name and user email, an insert that would violate that uniqueness constraint will be unsuccessful. You just need to be able to handle that use case in your method that inserts the user record.
  • If you choose to keep the potentially unnecessary select before insert, you should at least move the functionality to lookup a user by email or user name into a class method and out of the logic of the registration page.
  • Consider using empty() or !empty to check "truthiness"/"falsiness" of variables for validation rather than == ''. You should ideally only use loose comparison for very specific reasons, defaulting your code writing approach to using strict comparisons as a default. This makes your code less fragile to "truthiness"/"falsiness" bugs and clearer to read with regards to intent. For me, if I run into a case where it makes sense to use a loose comparison, I make sure to add a comment in code as to why I this made decision.
  • Consider user experience around registration. Why would you want to send them to login form after registration? Presumably if you had valid information for a registration, then you could just put user directly into logged in state, rather than forcing them to then log in.
  • Don't pass log in/account creation state via GET. This is way to easy to hack. This information should always be in session. For example, say you had successful registration and wanted to then redirect user to home page (rather than login page). You could store an account created flag either in session or in the theoretical UserSessionManager class which could be checked to see if any messaging should be displayed to the user and then flag turned off.
  • You should consider more significant validation of user input fields:
  • Do fields like username and password need to be a minimum length or have any constraints on characters it can use?
  • Don't nest validation in if-else conditionals. Why not validate all field with every pass so you can give user full feedback at once.
  • For registration, consider adding password verification field so you can help user mitigate typos in their password.
  • Consider using more robust validation class, or perhaps have static validation methods on user class, so that you can simply call centralized validation functions rather than having validation logic exist on every page.
  • Consider whether you REALLY want to tell users if there name or email matches/doesn't match the system on failed registration/login attempts. Knowing whether a user account exists or not is a valuable piece of potential information for an attacker. Many developers prefer the more vague "user account could not be create" or "login not successful" which are more non-specific.
  • If you don't REALLY need the functionality noted above, you can do away with needing to perform a select before the insert when registering an account. If your table has proper unique index on user name and user email, an insert that would violate that uniqueness constraint will be unsuccessful. You just need to be able to handle that use case in your method that inserts the user record. This eliminates 50% of your SQL calls for registration use case.
  • If you choose to keep the unnecessary select before insert, you should at least move the functionality to lookup a user by email or user name into a class method and out of the logic of the registration page.
  • Consider using empty() or !empty to check "truthiness"/"falsiness" of variables for validation rather than == ''. You should ideally only use loose comparison for very specific reasons, defaulting your code writing approach to using strict comparisons as a default. This makes your code less fragile to "truthiness"/"falsiness" bugs and clearer to read with regards to intent. For me, if I run into a case where it makes sense to use a loose comparison, I make sure to add a comment in code as to why I this made decision.
Source Link
Mike Brant
  • 9.6k
  • 13
  • 23

Good points by @insertusernamehere

I would like to strongly emphasize his points on thinking about what values in your code should be configuration driven vs. actually hardcoded, as well as the thought to look at some existing frameworks to understand how they approach these problems.

So, on to my thoughts:

Database table

  • Consider dropping user_* prefixes on your field names. You table is called users, so you should already have this context.
  • Don't truncate field names just for shortness sake. It is is more clear to call a password field password than it is pass. It would actually be better to call this field password_hash.
  • You likely want unique indexes on both user name and user email fields.

config.php

  • A file called config should have only config in it - no classes, no application bootstrapping.
  • Don't put all your classes into one file. Split them up into their own individual files. This makes your application more maintainable.
  • Indentation is inconsistent (this is pretty much a problem throughout all the code). Pick a style and stick with it, though I would suggest that single space indentation is not really indentation at all, just confusing.
  • Line length gets above ~ 80 characters (a good rule-of-thumb limit) in many case (typical throughout all code). Break you code up across lines as needed to help keep this line length down and make your code more readable.

Database Class

Your database class really holds little value. Typically one might implement a database class to do things such as:

  • Manage connections - something your implementation falls short of doing
  • Abstract away implementation details - something not happening here at all since classes working with the DB connection must understand that PDO is being used
  • Provide user-friendly query interfaces - again something not approached here.

About the only value your class has is in allowing calling classes to not have to understand how to instantiate a PDO object to work with. If you want to enforce that only a single connection is used for each script execution pass, then consider implementing a singleton here.

Even if you don't want to implement singleton, consider implementing the method to get the PDO object as a static method. There really is no reason at at all that calling code should have to first instantiate a Database object and then call the method to get the actual PDO object that it needs to work with.

Also consider naming this class and method more specifically to what it does. PDOProvider::getPDO() or PDOSingleton::getPDO() (if you try to go singleton route) would be much clearer usage from calling code.

Don't echo out Exception information. In fact, if you are not planning on actually doing something meaningful with an Exception (i.e. logging, wrapping in more general exception, rethrowing, or some combination of the above), you might be best not to put code that could throw Exception in try-catch at all. It would be better to let the Exception bubble up the call stack than to not do anything meaningful with it. This class should not care about end use messaging for the exception condition. This class just needs to do whatever is appropriate to a) log exception information and b) handle the exception in whatever manner is most meaningful for calling code to understand whatever it needs to know to determine how to handle the situation.

User class

  • Name your class User not USER.
  • Consider whether your User class itself should just describe the properties and behaviors of a User itself, or also have to have knowledge on how to create/find a user. You might have a helper class or classes which have the responsibility for creating / searching for users and instantiating valid user objects and limit the functionality of a user class to truly what the properties and behaviors of a user are. This class, as it stands, almost seems like more of a helper class (which shouldn't be named User at all) than an actual user class as there is really no user information stored on the class at all.
  • Consider passing a valid PDO object to the class as parameter in constructor. This class should not need to understand how to instantiate a PDO object. It should be given the dependencies it needs to operate (a concept called "Dependency Injection" which you should familiarize yourself with).
  • runQuery looks like a general PDO functionality that should not reside in a User class. This method is also improperly named, as it does not execute a query at all, just prepares one and returns PDOStatement object if successful. I honestly don't see what value this method is adding anyway.
  • I have same concerns in this class with basically meaningless try-catch exception handling blocks and improper responsibility for messaging end user.
  • In register function:
  • You should validate that you have reasonable parameters to work with (i.e. non-zero length strings) and fail out immediately with InvalidArgumentException or similar before executing any further. This is especially important since this is publicly exposed method.
  • $new_password is a really poor variable name. How about call it what it is - $password_hash. Try to always useful meaningful variables that are specific to a) what the content of the variable is intended to be and/or b) how the content of the variable is intended to be used.
  • This code only considers happy path. What happens if insert fails?
  • Why return PDOStatement object? What do you expect the caller to do with it? Perhaps this method either just returns boolean on failure/success, or perhaps returns the autoincrement id generated for the inserted record.
  • doLogin() seems odd to me. To me perhaps this method should return a valid User object (against which user can derive whatever information it wants) or null/false on failure. This relates to my question as to whether this sort of method should be part of user class. Right now it does things like set session values, which should not be within the responsibility of a user class IMO. I also don't really understand the user name OR user email login capability here. It seems like you could potentialy get multiple records in your result set. If you need to support this use case, I would have two different methods like getUserByName and getUserbyEmail (possibly static methods) and call which is appropriate by whatever the user provides (don't have them provide both). Leave the "log in" aspect of the logic outside the user class. As it stands, this method also lacks input validation and only really considers happy path (what if empty result set is received from query?)
  • Other methods in this class: Do they really belong here? Why would a redirect function be in a user class? Why are methods on this class interacting with session rather than actual properties on the class? True logout functionality may require a lot more than simply unsetting a session variable. Do you regenerate session id, do you reset cookies? How much d you really need to reset the application back to a state where the a brand new user can sit down on that machine and not know anything about previous user's session? Again this should all likely not be managed in a "user" class.

index.php

  • You can see just how poorly your User class is defined in this section of code (see my added comments)

Your code:

 // why would you call this variable $session if you were truly getting
 // a user object here?
 $session = new USER();
 // Why include hard-coded link to redirection page?  If you ever want
 // to change this, you have to go to every page where this code is used
 // to change it, rather than having someplace where you have configured
 // in your application where the user should be directed for login screen.
 if(!$session->is_loggedin()) {$session->redirect('login.php');}

  // why should you need to re-instantiate the user again here?
  $auth_user = new USER();
  // if you had a meaningful User class, I would think the id would
  // be stored on the object rather than needing to be derived from
  // session variable.
  $user_id = $_SESSION['user_session'];
  // Why should calling code here need to know how to get user info from DB?
  // You should be able to pass in an id to instantiate user class and
  // get an object with all appropriate properties filled out
  $stmt = $auth_user->runQuery("SELECT * FROM users WHERE user_id=:user_id");
  $stmt->execute(array(":user_id"=>$user_id));  
  $userRow=$stmt->fetch(PDO::FETCH_ASSOC);

If you follow some of the suggestions I have noted thus far, you might get to some code like this:

 // this code somewhere in bootstrap file (not config)
 // setup PDO dependency
 try {
     $pdo = PDOProvider::getPDO();
 } catch (Exception $e) {
     // perhaps log exception here
     // then redirect to error page or message user
 }

 // set up session
 session_start()
 
 // instantiate object to manage session behaviors -
 // log in, session regeneration, redirection,
 // interact with $_SESSION variables, etc.
 // Here I am using a theoretically named UserSessionManager class
 // perhaps it needs PDO as a dependency to do login operations and such.
 // The theoretical methods on this class are for informational purposes
 // only.  I honestly don't know if you want to have to instantiate an
 // object here, as some of the methods may be good for static usage.
 $user_session_manager = new UserSessionManager($pdo);


 // now the code below could be in any page where you needed to
 // enforce logged in users and work with user information

 // See if there is logged in user
 if($user_session_manager.isUserLoggedIn() === false) {
     $user_session_manager.redirectToLogin();
 }

 // since we know we have a valid user login at this point,
 // let us retrieve a user object
 $user = $user_session_manager.getUser();

When you take out the comments from the suggested coding approach I noted above, you can see that the calling code (index.php page) would now need to know very little about the underlying mechanisms for managing the user login, the database, etc. and now only contains a handful of lines of code to get your user information set up in a state that is meaningful for the page.

Login/registration

  • Consider user experience around registration. Why would you want to send them to login form after registration? Presumably if you had valid information for a registration, then you could just put user directly into logged in state, rather than forcing them to then log in.
  • Don't pass log in/account creation state via GET. This is way to easy to hack. This information should always be in session. For example, say you had successful registration and wanted to then redirect user to home page (rather than home page). You could store an account created flag either in session or in the theoretical UserSessionManager class which could be checked to see if any messaging should be displayed to the user and then turned off.
  • You should consider more significant validation of user input fields:
  • Do fields like username and password need to be a minimum length or have any constraints on characters it can use?
  • Don't nest validation in if-else conditionals. Why not validate all field with every pass so you can give user full feedback at once.
  • For registration, consider adding password verification field so you can help user mitigate typos in their password.
  • Consider using more robust validation class, or perhaps have static validation methods on user class, so that you can simply call centralized validation functions rather than having validation logic exist on every page.
  • Consider whether you REALLY want to tell users if there name or email matches/doesn't match the system on failed registration/login attempts. Knowing whether a user account exists or not is a valuable piece of potential information for an attacker. Many developers prefer the more vague "user account could not be create" or "login not successful" which are more non-specific.
  • If you don't REALLY need the functionality noted above, you can do away with needing to try perform a select before and insert when registering an account. If your table has proper unique indexed on user name and user email, an insert that would violate that uniqueness constraint will be unsuccessful. You just need to be able to handle that use case in your method that inserts the user record.
  • If you choose to keep the potentially unnecessary select before insert, you should at least move the functionality to lookup a user by email or user name into a class method and out of the logic of the registration page.
  • Consider using empty() or !empty to check "truthiness"/"falsiness" of variables for validation rather than == ''. You should ideally only use loose comparison for very specific reasons, defaulting your code writing approach to using strict comparisons as a default. This makes your code less fragile to "truthiness"/"falsiness" bugs and clearer to read with regards to intent. For me, if I run into a case where it makes sense to use a loose comparison, I make sure to add a comment in code as to why I this made decision.

Logout

  • You should not send user to index page here if index is then going to redirect user to login page anyway. Just redirect to login page.

  • I think this could be simplified to just:

    // not shown - include bootstrap file

    $user_session_manager->logout();

Where within the UserSessionManager class the logout method might call methods to unset session, regenerate session id, unset current User object, clear cookies, etc. and then finally call it's own redirectToLogin() method. This takes the logout logic out of individual pages and into a class where it can be centrally maintained.