2
\$\begingroup\$

This is where I got the idea for most of the code.

I think this is pretty decent. I want this reviewed for security. Any criticism is welcome.

<?php
require_once('db.php');
//Create new Authentication Class
class Auth{
    //Single underscore indicates variable privacy
    private $_siteKey;
    private $_dbh;


    /* Constructor function is passed a single database
     * dependancy variable to extend the PDO connection
     * started in db.php without using a singleton or global
     */
    public function __construct(connection $con){
        //Declaring _siteKey and extending the PDO conenction
        //SiteKey is an salt dedicated to the site to manage the salting of hashed information
        // SiteKey definition may be found inside the config.php file
        $this->_siteKey = SITE_KEY;
        $this->_dbh = $con->con;

    }//__construct()

    /* Random string function generates random strings which
     * may be appended onto various salts. Caries a $length
     * variable default length of 50, may be overridden
     */
    private function randomString($length = 50){
        //A list of possible characters for the $string to be made form
        $characters = '0123456790abcdefghijklmnopqrstuvwxyz!@#^_';
        $string = '';
        //For loop to build string
        for($p = 0; $p < $length; $p++){
            $string .= $characters[mt_rand(0, strlen($characters)-1)];
        }

        return $string;
    }//randomString()

    /* Hash Data function will be used to hash
     * data that will be inputed into the database
     */
    protected function hashData($data){
        return hash_hmac('sha512', $data, $this->_siteKey);
    }//hashData()

    /* Is Admin function is currently not implemented but
     * exists for future purposes to verify whether a logged
     * in user is of admin status or not
     */
    public function isAdmin(){
        /* Selection being the row of the user returned
         * There are no admin pages yet so the function
         * useless and unfinished
         */

        //PDO Selection Work Here
        /*
        if($selection['is-admin'] == 1){
            return true;
        }

        return false;
        */
    }//isAdmin()

    /* Create user function creates user
     * when passed the specified parameters
     */
    public function createUser($firstname, $lastname, $email, $username, $password){

        //Generate User Salt
        $user_salt = $this->randomString();

        //Salt and Hash the password for storing
        $laterpassword = $password;
        $password = $user_salt.$password.$user_salt;
        $password = $this->hashData($password);

        //Create a verification code for email
        $code = $this->randomString();

        /* Second level assurance to make sure
         * that the inputed user has unique information
         * in the event that the front end verification fails
         * or if the user to be inputed has been inputed at the
         * the same time as the other user and the front end
         * can't catch it
         */
        //Build the SQL & Execute
        $ensure = "SELECT * FROM users WHERE username = :username OR email = :email";
        $ensure = $this->_dbh->prepare($ensure);
        $ensure->execute(array(':username' => $username,
                                ':email' => $email
        ));
        //If a row exists with that user information already, return false and make user try again
        if($ensure->rowCount()){
            echo "false";
            return false;
        }

        //Commit values to database - Prepare the SQL
        $created = "INSERT INTO users (id, firstname, lastname, email, username, password, user_salt, is_verified, is_admin, verification_code) VALUES 
                                        (null, :firstname, :lastname, :email, :username, :password, :user_salt, :is_verified, :is_admin, :code)";

        $created = $this->_dbh->prepare($created);
        $created->execute(array(':firstname' => $firstname,
                                ':lastname' => $lastname,
                                ':email' => $email,
                                ':username' => $username,
                                ':password' => $password,
                                ':user_salt' => $user_salt,
                                ':is_verified' => '1',
                                ':is_admin' => '0',
                                ':code' => $code
                                ));
        //If user has been inserted
        if($created != false){
            //Immediately log the newly created user in
            //$laterpassword, being the unhashed password for the login() method
            $this->login($username, $laterpassword);

            /* Build message for the verification email
             * later an email() method will be created and called
             * to delegate to the work to another function
             */
            $message = "Thanks for signing up, $firstname.
                        Your account has been created with the username, $username,
                        and to login to be able to use your account, please click the activation link below:
                        http://www.yourwebsite.com/verify.php?email='.$email.'&hash='.$code.'
                        ";
            $headers = 'From:noreply@yourwebsite.com' . "\r\n"; // Set from headers
            mail($email, "Account Verification", $message, $headers); // Send our email
            //Return "true" to the ajax function for front end verification
            echo "true";
            return true;
        }

        return false;
    }//createUser()

    /* login function requires username and password
     * as parameter. Hashes the given password and
     * inserts the user into the database
     */
    public function login($username, $password){

        //Find user with that username in the database
        $sql_1 = ("SELECT * FROM users WHERE username = :username");
        $selection_1 = $this->_dbh->prepare($sql_1);
        $selection_1->execute(array(':username' => $username));

        //If the users is found, register the information for testing
        if($selection_1->rowCount()){

        //Fetch data into associative array 
        $row = $selection_1->fetch(PDO::FETCH_ASSOC);


        //Declaring the database user-salt
        $dbsalt = $row['user_salt'];

        //Build the Salted and Hashed Password for checking
        $password = $dbsalt.$password.$dbsalt;
        $password = $this->hashData($password);

        //Extract for comparison, the databse password for the selected user
        $db_password = $row['password'];

        //Convert verified to boolean
        $verified  = (boolean) $row['is_verified'];

        }
                //Restart the user checking - If user exists
                if($selection_1->rowCount()){
                        //If user is verified
                    if($verified == true){
                            //If passwords do match
                        if($password == $db_password){

                        //Generate random string
                        $random = $this->randomString();

                        //Build the token
                        $token = $_SERVER['HTTP_USER_AGENT'].$random;
                        $token = $this->hashData($token);

                        //Setup Session Variables
                        session_start();
                        $_SESSION['user_id']   =  $row['id'];
                        $_SESSION['firstname'] =  $row['firstname'];
                        $_SESSION['lastname']  =  $row['lastname'];
                        $_SESSION['email']     =  $row['email'];
                        $_SESSION['username']  =  $row['username'];

                        //SESSION token variable to be inputed into logged_in_users table
                        $_SESSION['token'] = $token;

                        //Select and delete old logged_in_member records for user
                        $sql_2 = ("DELETE FROM logged_in_users WHERE user_id = :user_id");
                        $selection_2 = $this->_dbh->prepare($sql_2);
                        $selection_2->execute(array(':user_id' => $_SESSION['user_id']));

                        //Insert the newly logged in user into the logged_in_users table
                        $sql_3 = ("INSERT INTO logged_in_users (id, user_id, session_id, token) VALUES (null, :user_id, :session_id, :token)");
                        $selection_3 = $this->_dbh->prepare($sql_3);
                        $selection_3->execute(array(':user_id' => $_SESSION['user_id'],
                                                    ':session_id' => session_id(),
                                                    ':token' => $_SESSION['token']
                                              ));

                        if($selection_3 != false){
                            //Echo Logged In for Ajax Response Text Testing
                            echo "Logged In";
                            return 0;
                        } else {
                            //Something went wrong
                            echo "Could Not Be Logged In";
                        }
                                //Echo Password Is Wrong for Ajax Response Text Testing
                        } else {echo "Password Is Wrong";}
                            //Echo Not Verified for Ajax Response Text Testing
                    } else {echo "Not Verified"; }
                        //Echo User Not Found for Ajax Response Text Testing
                } else {echo "User Not Found";}
    }//login()

    /* Check Session function checks the session to make
     * sure that the user's session is continuous with each
     * page request or the user must log in again to restart session
     */
    public function checkSession(){
        //Selection of user that we're checking
        $sql = "SELECT * FROM logged_in_users WHERE user_id = :user_id";
        $selection = $this->_dbh->prepare($sql);
        $selection->execute(array(':user_id' => $_SESSION['user_id']));

        //If exists
        if($selection->rowCount()){
            //Fetch logged_in_users information into associative array
            $row = $selection->fetch(PDO::FETCH_ASSOC);
            //Session validity check
                if(session_id() == $row['session_id'] && $_SESSION['token'] == $row['token']){
                    $this->refreshSession();
                    return true;
                }
        }
        return false;
    }//checkSession()

    /* Refresh Session is called each time
     * checkSession() is run to ensure the security
     * of each session
     */
    private function refreshSession(){
        //Regenerate session id
        session_regenerate_id();

        //Regenerate our token
        $random = $this->randomString();

        //Build the token
        $token = $_SERVER['HTTP_USER_AGENT'].$random;
        $token = $this->hashData($token);

        //Store in session
        $_SESSION['token'] = $token;

        //Database work to update the SessionID and Token in the DB
        $deletesql = "DELETE FROM logged_in_users WHERE user_id = :user_id";
        $deletesql = $this->_dbh->prepare($deletesql);
        $deletesql->execute(array(':user_id' => $_SESSION['user_id']));

        $insertsql = "INSERT INTO logged_in_users (id, user_id, session_id, token) VALUES (null, :user_id, :session_id, :token)";
        $insertsql = $this->_dbh->prepare($insertsql);
        $insertsql->execute(array(':user_id' => $_SESSION['user_id'],
                                    ':session_id' => session_id(),
                                    ':token' => $_SESSION['token']
                              ));


    }//refreshSession()

    /* Basic logout() function to delete user from
     * from logged_in_user table and destroy the session
     */
    public function logout(){
        $deletesql = "DELETE FROM logged_in_users WHERE user_id = :user_id";
        $deletesql = $this->_dbh->prepare($deletesql);
        $deletesql->execute(array(':user_id' => $_SESSION['user_id']));

        session_destroy();
    }
}
?>
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't know PHP, but if the code works nice question, Let me criticize : you have too much comments, some of the comments even explain obvious things. You don't need to mark end of each function/method/constructor with a comment \$\endgroup\$ – bhathiya-perera Aug 21 '14 at 12:08
1
\$\begingroup\$

Nice Code, just a couple of points for now:

Security

I didn't see anything else right now, but that doesn't mean that this is all.

Other

  • why are you checking if the user exists manually (via $ensure)? Shouldn't your database schema take care of this?
  • updating logged_in_member in login: couldn't you use update instead of delete and then insert?
  • select * is bad practice and bad for performance, it is better to specify which columns you want.
  • indentation and spacing: sometimes, they are off (eg if($selection_1->rowCount()){)
  • Comments: I generally like your comments, but as @Bhathiya-JaDogg-Perera noted, they are a bit much (eg //For loop to build string, //Generate User Salt, Create user function creates user, etc). What I would rather want to know: hashData: what hashing method is used? createUser: does it only create the user? Or does it also send out mail?
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback, how could the schema take care of my $ensure method? I'm just wondering. Other than that thank's a lot \$\endgroup\$ – Dominic Farolino Aug 21 '14 at 16:15
  • \$\begingroup\$ @DomFarolino you are just checking if the username or the email already exists, right?. So you could just add a unique constraints on both columns (seperately, as together would be username and email unique), then dublicate entries would not be allowed \$\endgroup\$ – tim Aug 22 '14 at 8:09
1
\$\begingroup\$

I find it helpful to separate SQL calls into their own class, rather than embedding them with the logic, even though it's related. It's easier to find and make changes to queries when needed.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! We're looking for in-depth responses. Can you expand your answer? \$\endgroup\$ – mleyfman Aug 22 '14 at 4:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.