PHPhulp.nl
35k+ leden Over PHPhulp Offerte opdracht Contact

Review OO login-script

Door SilverWolf NL op 22-01-2010 18:52
1.932 views
Ik ben al een tijdje bezig met een (goed) login script, en dit is wat ik er tot nu toe van gemaakt heb. Het is 'mn eerste OO-project, dus wellicht zitten er nog wat foutjes in het "object-georiënteerd denken", maar dan hoor ik het graag!

Vragen zijn natuurlijk altijd welkom. De voornaamste vraag die ikzelf heb, is of dit een goed (en dus veilig) login script is.


De code:

<?PHP
//Define (session)settings
session_set_cookie_params(1814400);
session_start();
ini_set('display_errors',1);
error_reporting(E_ALL);

//Tabelstructuur
$databaseSQL="CREATE TABLE IF NOT EXISTS tblUsers (
  User varchar(24) NOT NULL,
  Pass varchar(128) NOT NULL,
  Active int(1) DEFAULT NULL,
  Level int(1) DEFAULT NULL,
  PRIMARY KEY (User),
  UNIQUE KEY Us3r (User)
) ENGINE=MyISAM DEFAULT CHARSET=latin1;

INSERT INTO tblUsers (User, Pass, Active, Level) VALUES
('Administrator', '18e3b68aebdcd361b0ed9c82f9d76ffd7f7c344e5b986030223beb1e51e88329b823c237c43512ba5ede056431d339505608eef747717afd8defe2eda4a69f18', 1, 1);";
//Het wachtwoord is PHPhulp, als je dit wil veranderen moet je even de Hash class gebruiken om een wachtwoord te genereren.

/*
 * Database Class
 * @return object Database;
 * @functions return:
 *   array getUserData($username, $password, mixed $fields);
 *   bool uploadUser(object $UserObject);
 */
class DBO{
	private $MySQLi_HOST="127.0.0.1";
	private $MySQLi_USER="root";
	private $MySQLi_PASS="";
	private $MySQLi_DB="cms_db";
	//-------------------------------------------------------------------------------------------------------------------//
	private $infoArrayFields=array("username"=>"User","password"=>"Pass","active"=>"Active","level"=>"Level");
	private $infoArrayTables=array("users"=>"tblUsers","online"=>"tblOnline");
	private $dbCon;
	public function __construct(){
		$this->dbCon=new MySQLi($this->MySQLi_HOST, $this->MySQLi_USER, $this->MySQLi_PASS, $this->MySQLi_DB);
		if(mysqli_connect_error()){
            Log::Log("Connection error (" . mysqli_connect_errno() . ") " . mysqli_connect_error());
			return false;
        }else{
			return true;
		}
	}
	public function getUserData($username,$password,$data){
		if(!is_array($data)){
			$sql="SELECT " . $data . " FROM tblUsers WHERE User='" . $username . "' AND Pass='" . $password . "'";
			$result=$this->dbCon->query($sql);
			if(($this->dbCon->affected_rows===0)||(mysqli_error($this->dbCon)!==false)){
				return false;
			}else{
				$return=mysqli_fetch_assoc($result);
				return $return[$data];
			}
		}else{
			foreach($data as $userVar){
				$array[]=$this->infoArrayFields[$userVar];
			}w
			$array=implode(", ",$array);
			$sql="SELECT " . $array . " FROM tblUsers WHERE LOWER(User)='" . strtolower($username) . "' AND Pass='" . $password . "'";
			$result=$this->dbCon->query($sql);
			if(($this->dbCon->affected_rows===0) || mysqli_error($this->dbCon)){
				return false;
			}else{
				$databaseData=mysqli_fetch_assoc($result);
				foreach(array_keys($databaseData) as $search){
					$key=array_search($search, $this->infoArrayFields);
					$return[$key]=$databaseData[$search];
				}
				return $return;
			}
		}
	}
	public function uploadUser($userData){
		//
		foreach(array_keys($userData) as $key){
			$fields[]=$key;
		}
		foreach($userData as $value){
			if(is_int($value)){
				$values[]=$value;
			}else{
				$values[]="'".$value."'";
			}
		}
		$fields=implode(",",$fields);
		$values=implode(",",$values);
		unset($userData);
		$sql="INSERT INTO tblUsers(" . $fields . ") VALUES(" . $values . ")";
		$result=$this->dbCon->query($sql);
		if(($this->dbCon->affected_rows===0)||(mysqli_error($this->dbCon)!==false)){
           	Log::Log("Connection error (" . $this->dbCon->errno . ") " . $this->dbCon->error);
			return false;
		}else{
			return true;
		}
	}
}
/*
 * User Class
 * @return object User;
 * @functions return:
 *   bool setUsername(string $username);
 *   bool setPassword(string $password);
 *   bool setLevel   (int $level      );
 *   bool setActive  (int $active     );
 *   string getUsername();
 *   string getPassword();
 *   int getActive();
 *   int getLevel();
 */
class User{
	private $username;
	private $password;
	private $level;
	private $active;
	
	public function setUsername($name){
		$this->username=$name;
	}
	public function setPassword($pass){
		$this->password=$pass;
	}
	public function setActive($a){
		$this->active=intval($a);
	}
	public function setLevel($l){
		$this->level=intval($l);
	}
	
	public function getUsername(){
		return $this->username;
	}
	public function getPassword(){
		return $this->password;
	}
	public function getActive(){
		return $this->active;
	}
	public function getLevel(){
		return $this->level;
	}
}
/*
 * UserHandler Class
 * @return object UserHandler;
 * @functions return:
 *   object getUserObject();
 *   bool login($username, $password [, $sessionName="userObject" [, array $data_to_get ]);
 *   bool logout([$sessionName="userObject"]);
 *   bool register($username, $password [, int $active [, int $level ]]);
 *   bool check([$sessionName="userObject"])
 */
class UserHandler{
	private $UserObject;
	private $DatabaseObject;
	private $EncryptionObject;
	private $SessionObject;
	public function __construct(){
		$this->DatabaseObject=new DBO;
		$this->SessionObject=new SessionHandler;
	}
	private function getUserData($username,$password,$fields=NULL){
		if(preg_match("/^[a-zA-Z][a-zA-Z0-9_]+[a-zA-Z0-9_]/",$username)===0){
			return false;
		}else{
			$password=Hash::HashString($password);
			if($fields==NULL){
				$fields=array("active","level");
			}
			$data=$this->DatabaseObject->getUserData($username,$password,$fields);
			if($data===false){
				return false;
			}else{
				$this->UserObject=new User;
				$this->UserObject->setUsername($username);
				$this->UserObject->setPassword($password);
				unset($username,$password);
				if($data===false){
					return false;
				}else{
					if(!is_array($fields)){
						$fields="set" . ucfirst(strtolower($fields));
						$this->UserObject->$fields($data);
					}else{
						foreach($data as $var=>$value){
							$fields="set" . ucfirst(strtolower($var));
							$this->UserObject->$fields($value);
						}
					}
					unset($data,$var,$value);
					return true;
				}
			}
		}
	}
	public function getUserObject(){
		return $this->UserObject;
	}
	public function login($username,$password,$sessionName=NULL,$UserVars=NULL){
		if(empty($sessionName)){
			$sessionName="userObject";
		}
		if($this->getUserData($username,$password,$UserVars)){
			unset($username,$password);
			$this->UserObject->setLevel($this->UserObject->getLevel());
			$this->SessionObject->setSession($this->UserObject,$sessionName);
			return true;
		}else{
			unset($username,$password);
			return false;
		}
	}
	public function logout($sessionName=NULL){
		if(empty($sessionName)){
			$sessionName="userObject";
		}
		$this->SessionObject->removeSession($sessionName);
		return true;
	}
	public function register($username,$password,$active=NULL,$level=NULL){
		if((preg_match("/^[a-zA-Z][a-zA-Z0-9_]+[a-zA-Z0-9_]/",$username)===0) || (strlen($username)>24)){
			return false;
		}else{
			$this->UserObject=new User;
			$this->UserObject->setUsername($username);
			$password=Hash::HashString($password);
			$this->UserObject->setPassword($password);
			$this->UserObject->setActive($active);
			if($level==NULL)$level=0;
			if($active=NULL)$active=0;
			$this->UserObject->setLevel($level);
			$data=array("username"=>$username, "password"=>$password, "active"=>$active, "level"=>$level);
			unset($username,$password,$level,$active);
			$succes=$this->DatabaseObject->uploadUser($data);
			if($succes){
				return true;
			}else{
				return false;
			}
		}
	}
	public function check($sessionName=NULL){
		if(empty($sessionName)){
			$sessionName="userObject";
		}
		
		if($this->SessionObject->instanceofClass($sessionName,"User")){
			$this->UserObject=$this->SessionObject->getSession($sessionName);
			return true;
		}else{
			return false;
		}
	}
}
/*
 * Log Class
 * @return object Log;
 */
class Log{
    private static $fileHandler;
    private static $logPath = 'errorLog.txt';

    public static function LogString($error){
        if(!isset(self::$fileHandler))
             self::$fileHandler=fopen(self::$logPath, "a");

        $error="[" . date("d-m-Y H:i:s") . "] -> '" . $error . "'\r\n";
        if(fwrite(self::$fileHandler,$error)){
            return true;
        }else{
            return false;
        }
    }
}
/*
 * Hash Class
 * @return object Hash;
 * @functions return:
 *   bool setKey($key);
 *   bool setInput($input);
 *   string getHash();
 */
class Hash{
	public static $_hash;
	private static $_key;
	private static $_hashMethod;
	public static function HashString($input,$method=NULL,$key=NULL){
		if($key==NULL&&empty(self::$_key))$key=trim(__FILE__,"./\\");
		if($method==NULL&&empty($_hashMethod)){
			switch(true){
				case (in_array("whirlpool",hash_algos())):
					$method="whirlpool";
				break;
				case (in_array("sha512",hash_algos())):
					$method="sha512";
				break;
				case (in_array("sha384",hash_algos())):
					$style="sha384";
				break;
				case (in_array("sha256",hash_algos())):
					$method="sha256";
				break;
				case (in_array("haval256,5",hash_algos())):
					$method="haval256,5";
				break;
				default:
					$method="md5";
				break;
			}
		}
		self::$_key=$key;
		self::$_hashMethod=$method;
		self::$_hash=hash_hmac(self::$_hashMethod,$input,self::$_key);
		return self::$_hash;
	}
	public static function getHash(){
		return self::$_hash;
	}
}

/*
 * SessionHandler Class
 * @return object SessionHandler;
 * @functions return:
 *   bool setSession(mixed $data, $sessionName [, time $time_to_live ]);
 *   bool removeSession($sessionName);
 *   mixed getSession($sessionName);
 *   bool instanceofClass($sessionName, $class);
 */
class SessionHandler{
	public function setSession($data,$sessionName){
		if(is_object($data)){
			$_SESSION[$sessionName]=base64_encode(serialize($data));
		}else{
			$SESSID=md5(time() . $_SERVER['REMOTE_ADDR'] . mt_rand());
			$_SESSION[$SESSID]=$data;
			setcookie($sessionName,$SESSID,0);
		}
		return true;
	}
	public function removeSession($sessionName){
		unset($_SESSION[$sessionName]);
		return true;
	}
	public function getSession($sessionName){
		if( (!empty($_SESSION[$sessionName])) && is_object(unserialize(base64_decode($_SESSION[$sessionName]))) ){
			return unserialize(base64_decode($_SESSION[$sessionName]));
		}elseif(!empty($_COOKIE[$sessionName])){
			$sessionName=$_COOKIE[$sessionName];
			return $_SESSION[$sessionName];
		}else{
			return false;
		}
	}
	public function instanceofClass($sessionName,$class){
		if((!empty($_SESSION[$sessionName]))){
			$temp=$_SESSION[$sessionName];
			$temp=unserialize(base64_decode($temp));
			if($temp instanceof $class){
				return true;
			}else{
				return false;
			}
		}else{
			return false;
		}
	}
}
?>
- Ariën -
22-01-2010 19:01
Us3r varchar(24) NOT NULL,
P4ss varchar(128) NOT NULL,
4ct1v3 int(1) DEFAULT NULL,
L3v31 int(1) DEFAULT NULL,

Vanwaar die belachelijke namen?
-
---- ----
22-01-2010 19:03
User
Pass
Active
Level

Zo kan het ook gewoon
Maarja dat is zijn eigen keus.. xD
?
Onbekende gebruiker
22-01-2010 19:04
Waarvoor moet je database object weten hoe hij gegevens van een user ophaalt? Dat klopt niet.
(En in principe kan je de mysqli class extenden.)
SilverWolf NL
22-01-2010 19:06
Aar schreef op 22.01.2010 19:01
Us3r varchar(24) NOT NULL,
P4ss varchar(128) NOT NULL,
4ct1v3 int(1) DEFAULT NULL,
L3v31 int(1) DEFAULT NULL,

Vanwaar die belachelijke namen?


Omdat je dan niet makkelijk kan raden wat de naam van een kolom is. Het voordeel hiervan is dat met SQL-injections, of iets in die vorm, moeilijker de data veranderd kan worden, omdat de tabelnamen moeilijker te raden zijn. Ik kan ze ook gewoon een standaard naam geven, mocht dat overzichtelijker zijn, maar ik heb het nu zo gedaan.
?
Onbekende gebruiker
22-01-2010 19:11
Edoxile schreef op 22.01.2010 19:06
[quote='Aar schreef op 22.01.2010 19:01'] Us3r varchar(24) NOT NULL,
P4ss varchar(128) NOT NULL,
4ct1v3 int(1) DEFAULT NULL,
L3v31 int(1) DEFAULT NULL,

Vanwaar die belachelijke namen?


Omdat je dan niet makkelijk kan raden wat de naam van een kolom is. Het voordeel hiervan is dat met SQL-injections, of iets in die vorm, moeilijker de data veranderd kan worden, omdat de tabelnamen moeilijker te raden zijn. Ik kan ze ook gewoon een standaard naam geven, mocht dat overzichtelijker zijn, maar ik heb het nu zo gedaan.[/quote]
Sommige sql injections kan gewoon achterhalen wat de tabel- en kolomnamen zijn. Dus zo'n rare naam heeft geen toegevoegde waarde.
Pim -
22-01-2010 19:25
Niet bij blinde injections, dan heeft het wel voordeel. Hoewel ik hier leesbaarheid toch belangrijker zou vinden. SQL Injections zijn makkelijk af te weren met prepared statements of escapes.
Maar waarom die session class, als je er niks bijzonders mee doet? Je kan er bijvoorbeeld een session hijacking preventie mee inbouwen oid.

EDIT: En nog wat, nu maak je bij elke log een nieuwe file verbinding. Je kan beter die verbinding static opslaan. Ook kan je die hele method beter static maken, het object zelf is toch leeg.
SilverWolf NL
23-01-2010 00:17
@Karl en Pim: dat wist ik niet, dan kan ik idd gewoon 'normale' namen doen.

@Pim, ik zal eens kijken naar die static method, dat is een goede tip

@Karl, dat databaseobject die de info van de gebruiker ophaalt valt me, nu je het zegt, ook op. Ik zal kijken wat ik eraan kan doen...
SilverWolf NL
24-01-2010 16:46
Ik heb geprobeerd het script aan te passen, zodat al het bovenstaande beter is. Zijn er nu nog tips of aanmerkingen? Dit zou mij heel erg helpen, dus b.v.d. ;)

PS: omdat ik niet 2x zo'n lange post wil neerzetten, heb ik de originele post aangepast.
Pim -
24-01-2010 16:59
Ik zou dit van je log class maken:
<?php
class Log{
private static $fileHandler;
private static $logPath = 'errorLog.txt';

public static function log($error){
if(!isset(self::$fileHandler))
self::$fileHandler=fopen(self::$logPath, "a");

$error="[".date("d-m-Y H:i:s")."] -> '".$error."'\r\n";
if(fwrite(self::$fileHandler,$error)){
return true;
}else{
return false;
}
}
}
?>

En van die hashclass kan je toch ook gewoon hetzelfde doen?
<?php
class Hash
{
private static $_defaultMethod = 'md5';
private static $_defaultKey = 'asdfla;sduf08';

public static function setMethod($method);
public static function getMethod();

public static function setKey($key);
public static function getKey();

public static hash($string, $key = null, $method = null);
}
?>
SilverWolf NL
24-01-2010 21:52
@Pim: jah, dankjewel. Ik had me nog niet echt verdiept in static methods en properties, maar dat komt bij deze twee inderdaad van pas. Ik heb het nu ongeveer zo aangepast, als jij had voorgesteld.

Verder, wil ik graag weten (dat was immers ook een vraag) in hoeverre dit veilig is. Ik moet nog iets verzinnen om de sessionid te koppelen aan iets, zodat deze niet gekopieerd kan worden. Nu is het volgens mij nog zo, dat als iemand de sessionid uit het koekje kopieert, dat je dan als iemand anders kan inloggen. Hoe kan ik dit het beste doen? Behalve een IP-adres link, wat door (bijna) iedereen wordt afgeraden, kan ik niets bedenken. Iemand ideeën?

Reageren

Inloggen om te reageren