Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have created a PHP login system based off of this site. My main concern is: Is it secure? I chose that because to me it looked secure, but a 2nd or 3rd opinion never hurts. The idea of the project is to have something I can just pull into a new project directory and start off from there depending on my needs so it so it does have a few unused files.

Authentification Class

<?php
class authentification extends SQLQUERY
{

    /*
     * Hash code taken from:
     * Password hashing with PBKDF2.
     * Author: havoc AT defuse.ca
     * www: https://defuse.ca/php-pbkdf2.htm
     */

    /*
     * Authentifications EXCEPTIONS (1000)
     * 1001 : Login already exists
     * 1002 : Email already exists
     * 1003 : Current user is not a guest (for registration)
     * 1004 : Submitted registration password is too common
     * 1005 : Submitted registration password is too short
     * 1100 : User password not in correct format (when this is raised, we force user to enter a new password, if it was valid to start with) 
     *          This allows for including this code with an existing database
     * 1101 : User password is expired
     * 1102 : User is not properly activated
     * 1200 : Access violation - User is not admin
     * 
     */

    // These constants may be changed without breaking existing hashes.
    const PBKDF2_HASH_ALGORITHM = "sha256";
    const PBKDF2_ITERATIONS = 1000;
    const PBKDF2_SALT_BYTES = 24;
    const PBKDF2_HASH_BYTES = 24;

    const HASH_SECTIONS = 4;
    const HASH_ALGORITHM_INDEX = 0;
    const HASH_ITERATION_INDEX = 1;
    const HASH_SALT_INDEX = 2;
    const HASH_PBKDF2_INDEX = 3;     

    const MINIMUM_PASS_LENGTH = 1;
    const ADMIN_CAN_LOGIN = 1; //DISABLED RETURNS WRONG PASSWORD ON ALL ADMIN LOGIN ATTEMPTS       
    const SESSION_TIMEOUT = 86400; //IN SECONDS       
    const DEFAULT_LANG_ID = "dl_basicl"; //Session key for default language     
    public $login = '';
    public $email = '';
    public $pass = '';
    private $sess_id = '';
    private $user_id = -1;

    public function __construct($db, $login, $email, $pass, $user_id = -1)
    {
        SQLQUERY::__construct($db);
        $this->login = $login;
        $this->email = $email;
        $this->pass = $pass;
        //for when it is already known from a global
        $this->user_id = $user_id;

        //set user session
        $this->sess_id = session_id();
        if(!$this->sess_id) {
            $this->sess_id = $PHPSESSID;      
        }
    }

    /**
     * 
     * @return boolean checks if CheckCredentials are valid. 
     */
    public function login() 
    {
        $this->sess_clear();
        if($this->CheckCredentials()){

            $this->sess_write();
            $this->auth_user_update_date();          
            $this->auth_user_login_count();          
            return true;
        }
        return false;
    }
    /**
     * 
     * @return boolean Logs the user out
     */
    public function logout()
    {
        return $this->sess_delete();
    }

    /**
     *   
     * @return array returns the user infos
     * Array returns id, login, user_level, default_lang, email
     */
    public function checkuser()
    {
        $this->sess_clear();

        $id = $this->sess_read();
        $this->user_id = $id ? $id : -1;

        $this->auth_user_update_date(); 

        return $this->get_user_infos();
    }


    /**
     * 
     * @return  boolean Gets whether user should login or if it is first admin setup
     */
    public function shouldLogAdmin()
    {
        if($this->IsLoginAdmin())
        {
            return $this->HasPassword();
        }
        else 
        {
            //if it is not an admin, return false
            throw new Exception("Current user is not an admin", 1200);
        }
    }

    /**
     * 
     * @return  boolean Sets the first admin password
     */
    public function setAdminPassword()
    {
        $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthUpdateFirstAdminPass()), array($this->create_hash(), $this->login));     
        return true;
    }

    /**
     * Updates password through user id from the panel
     * @return  boolean updates a new password based on the user id
     */
    public function updatePassword()
    {
        $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthUpdatePassword()), array($this->create_hash(), $this->user_id));     
        return true;
    }

    /**
     * 
     * @return  boolean Sets a new password for an expired account
     */
    public function setNewPassword()
    {
        if($this->is_common_pass($this->pass))
        {
            throw new Exception("Submitted registration password is too common", 1004);
        }
        if(strlen($pass) < self::MINIMUM_PASS_LENGTH)
        {
            throw new Exception("Submitted registration password is too short", 1005);
        }
        $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthUpdateNewPassword()), array($this->create_hash(), $this->login));     
        return true;
    }

    /**
     * 
     * @return  boolean True on user registration
     */
    public function registerUserWithAdmin( $login, $email, $pass)
    {
        $register = $this->create_new_user($login, $email, $pass);
        return $register;
    }

    /**
     * 
     * @param int $active Default new user active status - 1 if unset anywhere.
     * @return  boolean 
     */
    public function registerUser($active = 1)
    {
        //if not, check if user can register (is not logged in already)
        if($this->user_id != -1)
        {
            throw new Exception("Current user is not a guest (for registration)", 1003);
        }
        //if not register a new user
        $register = $this->create_new_user($active, $this->login, $this->email, $this->pass);
        if($register)
        {
            $this->user_id = $register;
        }


        return true;
    }

    /**
     * Checks if a given email actually exists for an active non-admin user in the system
     * @return boolean 
     */
    public function emailExists()
    {
        $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthUsersByEmail()), array($this->email));
        while(isset($rs->fields) && !$rs->EOF)
        {
            if($rs->fields[1] > 0)
            {
                return true;
            }
            $rs->MoveNext();
        }        
        return false;
    }
    /**
     * @TODO email can login
     * Allows the user to login through a mail sent in his email
     * This is no less secure than letting them reset their password
     * Login count validation blocks further login attempts
     * @return boolean 
     */
    public function emailLogin()
    {
        /*$this->sess_clear();
        if( @TODO email can login ){

            $this->sess_write();
            $this->auth_user_update_date();          
            $this->auth_user_login_count();          
            return true;
        }*/
        return false;
    }
/***********************************
 * NON-PUBLIC FUNCTIONS START HERE *
 ***********************************/    

    /**
     * 
     * @return String The encoded password with the format: algorithm:iterations:salt:hash
     */
    private function create_hash()
    {
        $pass = $this->pass;
        //create unique salt
        $salt = base64_encode(mcrypt_create_iv(self::PBKDF2_SALT_BYTES, MCRYPT_DEV_URANDOM));

        return self::PBKDF2_HASH_ALGORITHM . ":" . self::PBKDF2_ITERATIONS . ":" .  $salt . ":" .
            base64_encode( $this->pbkdf2(self::PBKDF2_HASH_ALGORITHM,$pass,$salt,self::PBKDF2_ITERATIONS,self::PBKDF2_HASH_BYTES,true) );
    }

    /**
     * 
     * @param String $good_hash
     * @return boolean Validates the given password
     */
    private function validate_password($good_hash)
    {
        $params = explode(":", $good_hash);

        if(count($params) < self::HASH_SECTIONS)
        {
           return false;
        }

        $pbkdf2 = base64_decode($params[self::HASH_PBKDF2_INDEX]);

        return $this->slow_equals( $pbkdf2,  $this->pbkdf2($params[self::HASH_ALGORITHM_INDEX],$this->pass, $params[self::HASH_SALT_INDEX], intval($params[self::HASH_ITERATION_INDEX]),strlen($pbkdf2),true));
    }

    /**
     * 
     * @param String $a
     * @param String $b
     * @return boolean Compares two strings $a and $b in length-constant time.
     */
    private function slow_equals($a, $b)
    {
        $diff = strlen($a) ^ strlen($b);
        for($i = 0; $i < strlen($a) && $i < strlen($b); $i++)
        {
            $diff |= ord($a[$i]) ^ ord($b[$i]);
        }
        return $diff === 0;
    }

    /**
     * PBKDF2 key derivation function as defined by RSA's PKCS #5: https://www.ietf.org/rfc/rfc2898.txt
     * @param String $algorithm - The hash algorithm to use. Recommended: SHA256
     * @param String $password - The password.
     * @param String $salt - A salt that is unique to the password.
     * @param int $count - Iteration count. Higher is better, but slower. Recommended: At least 1000.
     * @param int $key_length - The length of the derived key in bytes.
     * @param boolean $raw_output - If true, the key is returned in raw binary format. Hex encoded otherwise.
     * @return String A $key_length-byte key derived from the password and salt.
     *
     * Test vectors can be found here: https://www.ietf.org/rfc/rfc6070.txt
     *
     * This implementation of PBKDF2 was originally created by https://defuse.ca
     * With improvements by http://www.variations-of-shadow.com
     */
    private function pbkdf2($algorithm, $password, $salt, $count, $key_length, $raw_output = false)
    {
        $algorithm = strtolower($algorithm);
        if(!in_array($algorithm, hash_algos(), true))
        {
            die('PBKDF2 ERROR: Invalid hash algorithm.');
        }
        if($count <= 0 || $key_length <= 0)
        {
            die('PBKDF2 ERROR: Invalid parameters.');
        }
        $hash_length = strlen(hash($algorithm, "", true));
        $block_count = ceil($key_length / $hash_length);

        $output = "";
        for($i = 1; $i <= $block_count; $i++) {
            // $i encoded as 4 bytes, big endian.
            $last = $salt . pack("N", $i);
            // first iteration
            $last = $xorsum = hash_hmac($algorithm, $last, $password, true);
            // perform the other $count - 1 iterations
            for ($j = 1; $j < $count; $j++) {
                $xorsum ^= ($last = hash_hmac($algorithm, $last, $password, true));
            }
            $output .= $xorsum;
        }

        if($raw_output)
        {
            return substr($output, 0, $key_length);
        }  
        else
        {
            return bin2hex(substr($output, 0, $key_length));
        }

    }

    /**
     * 
     * @return integer Checks password and username for login method. If found, returns the id
     */
    private function CheckCredentials() 
    {   
        if($this->CanLogin())
        {
            $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthGetUserPassword()), array($this->login));

            if(!$rs->EOF){            
                if($this->validate_password($rs->fields[1]) && $rs->fields[2] > 0)
                { //check if password is good and user is not inactive
                    $this->user_id = $rs->fields[0];
                    if($rs->fields[2] == 2)
                    {//if pass is correct but was set to expire, provoke change
                        throw new Exception("Password has expired", 1101);
                    }
                    if($rs->fields[2] >= 3)
                    {//if pass is correct but user is not yet verified
                        throw new Exception("User is not properly activated", 1102);
                    }
                    return true;
                }
                elseif($rs->fields[1] == $this->pass ) 
                {//if pass is in plain text in db then it must be changed
                    throw new Exception("Password format is incorrect", 1100);
                }

            }
        }
        return false;
    }

    /**
     * Checks if attempting to log in as admin and returns false if cannot admin login
     * Normal users can still log in  
     * 
     * @return boolean Returns if the login can login and always fails if ADMIN_CAN_LOGIN is 0
     */
    private function CanLogin()
    {
        $alogin = $this->IsLoginAdmin();     

        if(self::ADMIN_CAN_LOGIN == 0 && $alogin)
        {
            return false;
        }  

        return true;
    }

    /**
     * 
     * @return boolean Returns whether the login is admin or not
     */
    private function IsLoginAdmin()
    {
        $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthGetRootUsers()), array());

        while($rs->fields && !$rs->EOF)
        {
            if($this->login == $rs->fields[0])
            {
                return true;
            }
            $rs->MoveNext();
        }   

        return false;
    }


    /**
     * 
     * @return boolean Returns whether the login has a password
     */
    private function HasPassword()
    {
        $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthGetUserPassword()), array($this->login));

        if(strlen($rs->fields[1]) > 0)
        {
            return true;
        }

        return false;
    }

    /**
     * 
     * @return integer Returns the id_user from active sessions table accordingly his session id
     */
    private function sess_read()
    {
        $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthUserActiveSession()), array($this->sess_id));

        if(!$rs->EOF)
        {
            return $rs->fields[0];
        }
        return false;
    }

    /**
     * 
     * @return boolean Writes the session id and user id to the table
     */
    private function sess_write()
    {
        $ip_address = $_SERVER["REMOTE_ADDR"];
        $file = $_SERVER["REQUEST_URI"];
        if(!$file) { 
            $file= $_SERVER["SCRIPT_NAME"];
        }

        SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthDeleteActiveSession()), array($this->sess_id));

        //insert
        SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthInsertSession()), array($this->user_id, $this->sess_id, $file));

        return true;
    }

    /**
     * 
     * @return boolean Flushes a session
     */
    private function sess_delete()
    {
        SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthDeleteActiveSession()), array($this->sess_id));
        return true;
    }

    /**
     * 
     * @param date $time
     * @return boolean Clears expired sessions
     */
    private function sess_clear()
    {
        SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthDeleteOldSession()), array(self::SESSION_TIMEOUT));
        return true;
    }

    /**
     * Updates the user_id variable only for the object
     * 
     * @return boolean true when the user is not a guest
     */
    private function update_user()
    {
        $this->user_id = sess_read();
        if(!$this->user_id)
        {
            $this->user_id = -1;
            return false;
        }

        return true;
    }

    /**
     * 
     * @return boolean Updates user session date
     */
    private function auth_user_update_date()
    {
        $date = date("YmdHis");

        $file = $_SERVER["REQUEST_URI"];
        if(!$file) { 
            $file= $_SERVER["SCRIPT_NAME"];
        }

        SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthUpdateSession()), array($file, $this->sess_id, $this->user_id));        

        //if not a guest
        if($this->user_id > 0){
            SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthUpdateUserLastSeen()), array($this->user_id));
            //SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthUpdateUserActive()), array(1, $this->user_id));
        }
        return true;
    }    

    /**
     * 
     * @return boolean Updates user login count
     */
    private function auth_user_login_count()
    {
        //if not a guest
        if($this->user_id > 0){
            SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthUpdateUserLoginCount()), array($this->user_id));
        }
        return true;
    }    

    /**
     * 
     * @return array The user infos
     */
    private function get_user_infos()
    {global $lang;

        $arr = array();

        if($this->user_id == -1)
        {
            //load guest infos
            $arr["id"] = -1;
            $arr["login"] = isset($lang) ? $lang["guest"] : "guest";
            $arr["user_level"] = 0;
            $arr["default_lang"] = isset($_SESSION["lrdl"]) ? $_SESSION["lrdl"] : 1;
            $arr["email"] = "";            
        }
        else
        {
            $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthGetUserInfos()), array($this->user_id));
            //rs has id, login, user_level, default_lang, email
            while(isset($rs->fields) && !$rs->EOF)
            {
                $arr["id"] = $rs->fields[0];
                $arr["login"] = $rs->fields[1];
                $arr["user_level"] = $rs->fields[2];
                $arr["default_lang"] = $rs->fields[3];
                $arr["email"] = $rs->fields[4];

                $rs->MoveNext();
            }
        }


        return $arr;
    }

    /**
     * 
     * @return integer The newly create user ID. False if none.
     * @throws Exception 1001, 1002, 1004
     */
    private function create_new_user($active, $login, $email, $pass = "")
    {
        $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthGetDupeRegistrations()), array($this->login, $this->email));

        if($rs->fields && !$rs->EOF){

            if($rs->fields[1] == $this->login)
            {
                throw new Exception("Login already exists", 1001);
            }
            else
            {
                throw new Exception("Email already exists", 1002); 
            }
        }
        else
        {
            if($pass == "")
            {
                $this->pass = $this->random_password();
            }
            else 
            {
                if($this->is_common_pass($pass))
                {
                    throw new Exception("Submitted registration password is too common", 1004);
                }
                if(strlen($pass) < self::MINIMUM_PASS_LENGTH)
                {
                    throw new Exception("Submitted registration password is too short", 1005);
                }
            }
            $pass = $this->create_hash();
            $default_lang = isset($_SESSION[self::DEFAULT_LANG_ID]) ? $_SESSION[self::DEFAULT_LANG_ID] : 1;
            $rs = SQLQUERY::Execute(SQLQUERY::Prepare(SQLQUERY::AuthInsertNewUser()), array($active, $default_lang, $login, $pass, $email));
            return SQLQUERY::LastID();
        }
        return false;

    }

    /**
     * 
     * @return string   A random password
     */
    private function random_password()
    {
        //make sure random pass is at least 8 characters long
        $min = self::MINIMUM_PASS_LENGTH;
        if($min < 8)
        {
            $min = 8;
        }
        $alphabet = "abcdefghijklmnopqrstuwxyzABCDEFGHIJKLMNOPQRSTUWXYZ0123456789";
        $pass = array(); //remember to declare $pass as an array
        $alphaLength = strlen($alphabet) - 1; //put the length -1 in cache
        for ($i = 0; $i < $min; $i++) 
        {
            $n = rand(0, $alphaLength);
            $pass[] = $alphabet[$n];
        }

        if($this->is_common_pass($pass))
        {
            return $this->random_password();
        }
        else
        {
            return implode($pass); //turn the array into a string    
        } 
    }

    /**
     * Checks if a password is in the top 1050 common passwords list
     * @param string password The password to check
     * @return bool True if the password is common
     */
    private function is_common_pass($pass)
    {
        $common_password = array("char limit");

        if( in_array($pass, $common_password) )
        {
            return true;
        }        
        return false;
    }
}
?> 

SQL Query Class

<?php
/**
 * SQL Queries
 * This is meant to be used with ADODB which allows setting params
 */

class SQLQUERY {

    private $db;

    public function __construct($db) {
        $this->db = $db;
    }

    /**
     * 
     * @param String $sql
     * @return Object   Returns object created by prepare statement
     */
    public function Prepare($sql)
    {
        return $this->db->Prepare($sql);
    }
    /**
     * 
     * @param Object $o Object that has been prepared
     * @param array $array Array of parameters
     * @return RecordSet The result of the query.
     */
    public function Execute($o, $array)
    {
        return $this->db->Execute($o, $array);
    }

    /**
     * @return RecordSet The last inserted id.
     */
    public function LastID()
    {
        return $this->db->Insert_ID();
    }

    /********************************************
     * START OF QUERIES LIST                    *
     ********************************************/

    protected function AuthGetUserPassword()
    {
        return "SELECT id, password, active FROM ".USERS_TABLE." WHERE login = ".$this->db->Param('a')."";
    }

    protected function AuthUpdateUserLastSeen()
    {
        return  "UPDATE ".USERS_TABLE." SET date_last_seen = NOW() WHERE id = ".$this->db->Param('a')."" ;
    }

    protected function AuthUpdateUserLoginCount()
    {
        return  "UPDATE ".USERS_TABLE." SET login_count = login_count + 1 WHERE id = ".$this->db->Param('a')."" ;
    }

    protected function AuthUpdateUserActive()
    {
        return  "UPDATE ".USERS_TABLE." SET active = ".$this->db->Param('a')." WHERE id = ".$this->db->Param('b')." and active = ".$this->db->Param('c')." ";
    }

    protected function AuthUserActiveSession()
    {
        return  "SELECT id_user FROM ".ACTIVE_SESSIONS_TABLE." WHERE session = ".$this->db->Param('a')."" ;
    }

    protected function AuthDeleteActiveSession()
    {
        return  "DELETE FROM ".ACTIVE_SESSIONS_TABLE." WHERE session = ".$this->db->Param('a')." " ;
    }

    protected function AuthDeleteOldSession()
    {
        return  "DELETE FROM ".ACTIVE_SESSIONS_TABLE." WHERE UNIX_TIMESTAMP(update_date) < UNIX_TIMESTAMP(now())-".$this->db->Param('a')." " ;
    }

    protected function AuthInsertSession()
    {
        return  "INSERT INTO ".ACTIVE_SESSIONS_TABLE." (id_user, session, file, update_date)
                    VALUES ( ".$this->db->Param('a').", ".$this->db->Param('b').", ".$this->db->Param('c').", NOW()) ";
    }

    protected function AuthUpdateSession()
    {
        return  "UPDATE ".ACTIVE_SESSIONS_TABLE." 
                 SET update_date = NOW(), file=".$this->db->Param('a')."  
                 WHERE session=".$this->db->Param('b')." AND id_user = ".$this->db->Param('c')."";
    }

    protected function AuthGetRootUsers()
    {
        return  "SELECT login FROM ".USERS_TABLE." WHERE user_level = 100";
    }   

    protected function AuthGetUserInfos()
    {
        return  "SELECT id, login, user_level, default_lang, email FROM ".USERS_TABLE." WHERE id = ".$this->db->Param('a')." ";
    }   

    protected function AuthUpdateFirstAdminPass()
    {
        return  "UPDATE ".USERS_TABLE." SET date_registration = NOW(), password=".$this->db->Param('a')." WHERE login=".$this->db->Param('b')." AND user_level = 100 AND password = '' ";
    }

    protected function AuthUpdatePassword()
    {//updates through ID
        return  "UPDATE ".USERS_TABLE." SET password=".$this->db->Param('a')." WHERE id=".$this->db->Param('b')." ";
    }

    protected function AuthUpdateNewPassword()
    {//updates through login (which is unique key in DB) and user cannot have been deactivated purposefully
        return  "UPDATE ".USERS_TABLE." SET password=".$this->db->Param('a').", active = 1  WHERE login=".$this->db->Param('b')." AND active > 1 ";
    }


    protected function AuthUsersByEmail()
    {
        return  "SELECT login, active FROM ".USERS_TABLE." WHERE email = ".$this->db->Param('a')." AND user_level < 100";
    }  

    protected function AuthGetDupeRegistrations()
    {
        return  "SELECT id, login, email FROM ".USERS_TABLE." WHERE login = ".$this->db->Param('a')." OR email = ".$this->db->Param('b')." ";
    }  

    protected function AuthInsertNewUser()
    {
        //                                      a           b                                       c       d        e
        return  "INSERT INTO ".USERS_TABLE." (active, default_lang, date_registration, user_level, login, password, email)
                    VALUES ( ".$this->db->Param('a').", ".$this->db->Param('b').", NOW(), 1, ".$this->db->Param('c').", ".$this->db->Param('d').", ".$this->db->Param('e').") ";
    }    
}
?>

Most of the above is called by the auth.php script in the root directory.

share|improve this question
3  
Hi @MrJack, welcome to Code Review! You've posted a lot of code here! I suggest that you break this code down to manageable modules you want us to review, and put each in a different question. No one will read all this code, and give you a coherent answer. –  Uri Agassi Mar 3 at 17:18
1  
Oh, i'm reading it alright, and there's a lot to say... –  Amelia Mar 3 at 17:19
    
@UriAgassi - I agree, it is big, but I couldn't decide what to chop off. I figured with the whole thing, anyone could paste it into an editor. The 30 000 character limit really limited my options to have both specific areas and the whole class. –  Mr Jack Mar 3 at 17:35
2  
@MrJack the fact that you have so much in one class is another issue. You should split your classes up under the SoC principle and use a PSR0/4 autoloader. –  Amelia Mar 3 at 17:38

1 Answer 1

up vote 12 down vote accepted

Preamble

As a matter of security, I can safely say that you would fail a professional security audit in seconds. You should be including the hashing library as an external component. You also need to forget everything you know about querying databases and start again.

Also, look at Model-View-Controller. The fact you have a templating engine and separate classes for Database work implies that you are attempting MVC, but the implementation itself is nothing like MVC.

SQLQUERY

As for the code here itself, your SQLQUERY class leaves a lot to be desired.

First: the naming. SQLQUERY makes me instantly think that this is a subclass of PDOStatement. But it isn't. It's a wrapper around a PDO object.

Consider the following:

class DatabaseWrapper {

    private $db;
    const USERS_TABLE = "`users`";

    public function __construct(PDO $db) {
        $this->setInstance($db);
    }

    public function &getInstance() {
        return $this->db;
    }

    protected function setInstance(PDO $db) {
        $this->db = $db;
    }

}
  1. It type-hints the database object in the constructor for dependency injection.
  2. It allows you to get a direct reference to the PDO object that is injected.
  3. Likewise, it also allows to you set it later (and allows classes that extend it to do the same)
  4. The users table is a class constant.

As for your queries themselves...

You're going to have to never, under any circumstances, use string interpolation (which is not too much of an issue in your implementation, except for the fact that you are using your controller as a model and your model as an array...)

Remove the ->Param() method and instead use $this->db->prepare().

This means that:

protected function AuthUsersByEmail()
{
    return  "SELECT login, active FROM ".USERS_TABLE." WHERE email = ".$this->db->Param('a')." AND user_level < 100";
}

Would instead become:

protected function getUsersByEmail($email, $level = 100) {
    $sql = "SELECT login, active FROM ". static::USERS_TABLE . " WHERE email = ? AND user_level < ?";
    return $this->prepare($sql, [$email, $level]);
}

Where prepare is the following:

protected function prepare($sql, array $bindings = []) {
    try {
        $prepare = $this->db->prepare($sql);
        $prepare->execute($bindings);
        return $prepare->fetchAll();
    } catch (PDOException $e) {
        throw new RuntimeException("prepare failed: $sql, " . $e->getMessage());
    }
}

In short, you should make your SQLQUERY class into a model that fetches data for you.

Also something I've noticed is that your entire project will be generating notices like crazy because of poor PHP.

  1. parent::__construct() should be used to call a parent constructor. using SQLQUERY::__construct() implies that it is a static method, and PHP will complain.
  2. protected functions are not static! In your entire authentication class, you are simply trying to call protected (and hence internal) functions from the global scope as public static methods.

This system would need a fairly hefty redesign in order to be considered for use in production environments.

You need to split up your files, and huge files are pretty much unreadable. Look at composer for autoloading your classes.

Also, have you tried using the built-in password hashing functions that PHP provides? (password_hash, password_verify, password_needs_rehash). You should really stick with vetted implementations of cryptographic functions that have been extensively peer-reviewed.

I strongly, strongly recommend that you consult PSR0, PSR1 and PSR2.

share|improve this answer
    
Thanks for the feedback, will definitely change the SQLQUERY class ASAP - makes a lot of sense. Will also look at composer, however, I have not been getting any notices so I will look if my error settings fail to trigger that before changing it. The main reason I haven’t looked into the built-in functions too much is that I read they require php 5.5 which is not what the prod server this might eventually run on uses (5.3). –  Mr Jack Mar 3 at 18:28
    
@MrJack composer has a package for php5.5 password functions already. it's ircmaxell/password-compat. –  Amelia Mar 3 at 20:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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