[PHP] Feedback voor loginsysteem

[Josaus]

Beste tweakers,
Een tijdje geleden ben ik begonnen met het maken van een loginsysteem. Dit werkte goed. Nu heb ik het systeem herbouwd in een class met functies (OOP) en weet niet of dit wel de manier is. Ook weet ik niet of ik het echt goed gedaan heb. Dus zou ik graag feedback krijgen, als dat zou kunnen :3 De reden dat ik dit zo heb gedaan, is dat ik er nog veel dingen aan toe wil voegen, en dit het handig en overzichtelijk houdt. (hoop ik) Het systeem is nog niet helemaal af, maar wel operationeel. Ik weet ook niet of mijn SQL-variabel-namen wel handig zijn xd

PHP:

<?php
class LoginSaus {
    function __construct() {
        mysql_connect('localhost', '*username*', '*watchwoord*') or die('Unable to connect to database: ' . mysql_error());
        mysql_select_db('josaus');
        session_start();
    }
    function __destruct() {
        mysql_close();
    }
    function login($user, $password) {
        $sLogin = sprintf('
            SELECT id, login, permission
            FROM users
            WHERE login = "%s"
            AND password_md5 = "%s"',
            mysql_real_escape_string(strtolower($user)),
            mysql_real_escape_string(md5($password)));
        $qLogin = mysql_query($sLogin);
        if(mysql_num_rows($qLogin) == 1) {
            $userDetails = mysql_fetch_assoc($qLogin);
            $_SESSION['uid'] = $userDetails['id'];
            $_SESSION['user'] = $userDetails['login'];
            $_SESSION['permission'] = $userDetails['permission'];
            return true;
        }
        else
            return false;
    }
    function register($user, $password, $email) {
        $regReturn;
        if($this->checkEmail($email)) {
            $sCheck = sprintf('
                SELECT login, email
                FROM users
                WHERE login = "%s"
                OR name = "%s"
                OR email = "%s"',
                mysql_real_escape_string(strtolower($user)),
                mysql_real_escape_string($user),
                mysql_real_escape_string($email));
            $qCheck = mysql_query($sCheck);
            if(mysql_num_rows($qCheck) >= 1) {
                $regReturn .= '!user||email';
            }
            else {
                $sRegister = sprintf('
                    INSERT INTO users (name, login, password_md5, email, date)
                    VALUES ("%s", "%s", "%s", "%s", %d)',
                    mysql_real_escape_string($user),
                    mysql_real_escape_string(strtolower($user)),
                    mysql_real_escape_string(md5($password)),
                    mysql_real_escape_string($email),
                    time());
                $qRegister = mysql_query($sRegister);
                if(mysql_num_rows($qRegister) == 0) {
                    echo 'MySQL error: ' . mysql_error();
                    $regReturn .= '!mysql';
                }
                else {
                    $regReturn = true;
                }
            }
        }
        else {
            $regReturn .= '!email';
        }
        return $regReturn;
    }
    function logout() {
        if(isset($_SESSION['uid'], $_SESSION['user'], $_SESSION['permission'])) {
            session_unset();
            $loggedOut = true;
        }
        else {
            $loggedOut = false;
        }
        return $loggedOut;
    }
    
    function checkEmail($email) {
        $regex = '/^([*+!.&#$¦\'\\%\/0-9a-z^_`{}=?~:-]+)@(([0-9a-z-]+\.)+[0-9a-z]{2,4})$/i';
        $isValid = preg_match($regex, $email);
        if($isValid) {
            list( , $domain) = explode("@", $email);
            if (checkdnsrr($domain, "MX")) {
                return true;
            }
            else {
                return false;
            }
        }
        else
            return false;
    }
}
?>

SQL:

CREATE TABLE `users` (
  `id` int(12) NOT NULL AUTO_INCREMENT,
  `name` varchar(32) NOT NULL,
  `login` varchar(32) NOT NULL,
  `password_md5` varchar(32) NOT NULL,
  `email` varchar(64) NOT NULL,
  `status` varchar(32) NOT NULL DEFAULT 'activation',
  `date` int(10) NOT NULL,
  `permission` int(4) NOT NULL DEFAULT '0',
  PRIMARY KEY (`id`),
  UNIQUE KEY `name` (`name`,`login`)
) ENGINE=MyISAM  DEFAULT CHARSET=latin1 AUTO_INCREMENT=2 ;

Hopelijk kunnen jullie mij wat tips geven :3

Groeten, Kees

[ Voor 6% gewijzigd door Josaus op 17-10-2012 19:27 ]

[kwaakvaak_v2]

Tip1, kijk eens naar PDO. Prepared statements ipv je huidige old-skool mysql_escape gedoe..

[krvabo]

PDO heb je ook weer niet direct nodig. Overstappen naar in ieder geval mysqli (note the 'i') is al een stuk handiger gelet op de toekomst.

De TS moet er wel op letten dat zijn class elke keer een connectie maakt naar de database als hij hem aanmaakt. Deze connect kan beter buiten de klasse gebeuren. Verder is dat is natuurlijk prima, maar houd er rekening mee dat je met functions gaat werken, je de databaseconnectie steeds moet meegeven. Of je gaat met een state-machine of singleton werken. Over dit laatste is veel te zeggen en is een hele andere discussie op zich die al vaak is gevoerd

Dus: haal die databaseconnectie en session_start() buiten deze klasse indien je de databaseconnectie op meer plaatsen op je site gebruikt.

Je session_unset zou je opzich kunnen vervangen door een $_SESSION = array(); Daarnaast is het escapen van een md5-hash opzich niet nodig en moet je mysql-errors niet tonen, maar over het algemeen is je klasse best werkbaar.

Wel een tip: voeg nuttig commentaar toe.

[ Voor 10% gewijzigd door krvabo op 17-10-2012 21:49 ]

[pedorus]

md5 gebruiken voor een wachtwoordhash lijkt me onveilig/ouderwets, gebruik iets als bcrypt. Die emailvalidatie-regexp klopt niet -> gebruik een niet zelf-gemaakte of filter_var.

[RobIII]

We doen hier niet aan sitescriptchecks

Verder:
Secure Password Storage - Lots of Don'ts, a Few Dos
Storing Passwords Securely
Password Storage Cheat Sheet
etc. etc. etc.

[ Voor 57% gewijzigd door RobIII op 17-10-2012 23:17 ]