Login Klasse bewerten

[darton]

Hallo!
Ich habe mir für meine Webseite mal ein Login Skript gebastelt. Für meine Bedürfnisse funktioniert es im Moment perfekt. Aber vielleicht findet ihr ja noch ein paar Fehler oder eventuelle Sicherheitslücken.
So sieht meine Klasse aus.

class Login {

    public function __construct() {
        session_start();
    }

    public function login($username, $password) {
        try {
            //Datenbankverbindung aufbauen
            $db = new PDO('mysql:host='.MYSQL_HOST.';dbname='.MYSQL_DB, MYSQL_USER, MYSQL_PASSWORD);
            //Error-Modus auf Exception umstellen
            $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $stmt = $db->prepare("SELECT id, user, password FROM benutzer WHERE user = :form_username AND password = :form_password");
            //Variablen im prepared Statement belegen und nach User und Password suchen
            $stmt->bindParam(':form_username', $username, PDO::PARAM_STR);
            //Passwort mit dem sha1 Hash verschlüsseln
            $stmt->bindParam(':form_password', sha1($password), PDO::PARAM_STR, 40);
            $stmt->execute();
            $user_id = $stmt->fetchColumn();
            $db = null;
            if($user_id) { //falls eine ID zu User und Passwort gefunden wurde
                $_SESSION['user_id'] = $user_id;
                session_regenerate_id(); //SID regenerieren, um Session Fixation zu verhindern
            } else {
                $_SESSION['error'] = "Benutzername oder Passwort falsch"; //Errorcode in Session ablegen
            }
        } catch (PDOException $e) {
            $_SESSION['error'] = "Verbindung zu Datenbank fehlgeschlagen";
        }
    }

    public function checkLogin() {
        if(!isset($_SESSION['user_id'])) { //wenn nicht authorisiert
            $referrer = basename($_SERVER['PHP_SELF']); //zwischenspeichern, welche Seite aufgerufen werden sollte
            $error = "";
            if ($_SERVER['QUERY_STRING'] != "") {
                $referrer .= "?" . $_SERVER['QUERY_STRING']; //auch den Query-String zwischenspeichern
            }
            if (isset($_SESSION['error']))
                $error = $_SESSION['error'];
            unset($_SESSION['error']); //Errorcode wieder zurücksetzen
            $vars = compact('referrer', 'error');
            Haanga::Load('login.html', $vars); //Template aufrufen
            exit; //alle weiteren Skripte werden gestoppt, da man nicht eingeloggt ist
        }
    }

    public function logout() {
        setcookie(session_name(), '', time()-3600,'/'); 
        session_destroy();
    }
}

if(isset($_POST['login_submit'])) {
    $login = new Login();
    //GET-Variablen von HTML-Tags befreien
    $user = filter_input(INPUT_POST, 'user', FILTER_SANITIZE_STRING);
    $pass = filter_input(INPUT_POST, 'user_pass', FILTER_SANITIZE_STRING);
    $login->login($user, $pass); //einloggen, falls Daten stimmen
    header("Location: ". $_POST['referrer'] .""); //zu der Seite umleiten, die ursprünglich hätte aufgerufen werden sollte
}
if (isset($_POST['logout'])) {
    $login = new Login();
    $login->logout();
    header("Location: index.php"); //zur Startseite umleiten
} 

Also da ist jetzt schon eine Umleitung mit eingebaut, die ich gleich noch erkläre. Außerdem benutze ich die Template-Engine Haanga. Das Formular, was in der login.html steht, sieht einfach so aus:

<div id="login">
{% if error %}
<div id="login_error">{{ error }}</div>
{% endif %}
<form action="login.php" method="post">
<p><label for="user">Benutzername:</label><br />
<input name="user" id="user" type="text" /></p>
<p><label for="user_pass">Passwort:</label><br />
<input name ="user_pass" id="user_pass" type="password" /></p>
<input name="login_submit" id="submit_button" type="submit" value="Einloggen" />
<input name="referrer" type="hidden" value="{{ referrer }}">
</div>

Wenn ich eine Seite also durch diese Login Klasse schützen möchte schreibe ich einfach folgendes ganz als erstes in den PHP-Code rein.

require_once "login.php";
$login = new Login();
$login->checkLogin(); 

Wenn nun beispielsweise die Seite index.php aufgerufen wird, so wird in der check-Login Funktion zwischengespeichert, von welcher Seite man gekommen ist, also hier die index.php. Nach erfolgreichem Login wird man dann einfach zu der Seite umgeleitet, die man angucken wollte. Was haltet ihr von der Klasse?

[AmicaNoctis]

Hallo,

das sieht soweit gut aus und ist auch SQL-Injection-sicher.

Wenn es irgendwann mal Benutzernamen mit Sonderzeichen geben sollte, wäre es sinnvoll, nach dem Aufbau der DB-Verbindung einen Zeichensatz für die Kommunikation zwischen Script und DBMS festzulegen.

Das Zusammenbasteln der $referrer-Variablen verstehe ich nicht ganz. Es sieht so aus, als ob du dort genau das reinschreibst, was ohnehin in $_SERVER["REQUEST_URI"] stehen müsste.

Die Fehlerbehandlung ist soweit gut aufgebaut, aber ich würde bei den PDOExceptions noch unterscheiden, ob wirklich der Verbindungsaufbau gescheitert ist (wie du einfach immer annimmst), oder ob es doch einen Fehler bei der Abfrage gab, was zugegebenermaßen recht unwahrscheinlich ist und vermutlich nur passiert, wenn in der DB etwas umbenannt wird.

Aus OOP-Sicht würde ich bemängeln, dass die Klasse nicht durch Abbildung der Entitäten der Realität entstanden ist, sondern auf einem Use-Case basiert. „Login“ ist eine Aktion und kein Akteur oder Gegenstand. Ich hätte die Methoden eher in der Klasse User/Visitor/Member untergebracht.

Gruß,

Amica

[darton]

Danke für die Antwort. Du hast eigentlich in allen Punkten Recht. Und ja, in der $_SERVER["REQUEST_URI"] steht tatsächlich das, was ich mir selber zusammengebaut habe. Wusste ich gar nicht.
Wie sieht das denn aus mit Brute-Force-Attacken? Sollte man die im Code berücksichtigen?

[AmicaNoctis]

Kann man machen, nur erinnere ich mich an meine Studienzeit im Wohnheim, wo 200 Studenten nach außen dieselbe IP-Adresse hatten. IP-basierte Brute-Force-Strategien gehen meist davon aus, dass das eine Person ist, die da plötzlich massiv Requests absetzt. Und Hacker, die es ernst meinen, führen dann ihre Brute Force Attack eben über unzählige verschiedene Anon-Proxies durch. Das kannst du dann wieder nicht erkennen. Wenn du das auch noch verhindern willst, musst du ganze IP-Bereiche sperren, womit du wieder andere User verärgern kannst (vorausgesetzt deine Seite ist international).

Was du aber tun kannst, um die Sicherheit der Passwörter noch zu erhöhen, wäre die Verwendung von Salted Passwords. Damit kannst du Rainbow-Table-basierte Angriffe verhindern, die ich – wenn ich Hacker wäre – probieren würde, bevor ich mich mit Brute Force abmühe.

[litterauspirna]

Naja meine erhlriche Meinung dazu, ich finde die Klasse nicht gut.

Es geht um ein Login, du aber hast da drin eine logout Methode, die gehört da drin nicht rein.
Das session_start() ist genauso fehlt am Platz, denn ein Login benutzt eine Session aber stellt sie nicht zur Verfügung.
Die Datenbankinstanz gehört da nicht rein sondern maximal per DI übergeben.
Diese $_SESSION Sachen würde ich so da auch nicht rein schreiben, sondern in ein Beispielsweiße SessionSaveHandler mit Sessionnamespaces unterbringen und auch dieses nicht in eine direkte Abhängigkeit bringen sondern maximal per DI übergeben.
Die ganzen Seesionaktionen gehören da überhaupt nicht rein.

Ich würde das agnze auch nicht unbedingt Login nenne sondern eher Auth, steht für Authentification und die an die richtige Steller deines Application Aufbaus einsetzen sodas es auch überall zur Verfügung stellen.

Du hast da viel zu viele direkte Abhängigkeiten drin, die so auf keinen Fall zustande kommen dürften, im Sinne der OOP.

Gruß der Litter

[AmicaNoctis]

Da hat der Litter in allen Punkten recht.

[darton]

OK, da liegt ihr wahrscheinlich sogar richtig. Ich habe jetzt mal alle $_SESSION Sachen ausgelagert und nun sieht es so aus:

class Auth {

    private $db;
    private $sh;

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

    public function login($username, $password) {
        try {            
            //Error-Modus auf Exception umstellen
            $this->db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $stmt = $this->db->prepare("SELECT id, user, password FROM benutzer WHERE user = :form_username AND password = :form_password");
            //Variablen im prepared Statement belegen und nach User und Password suchen
            $stmt->bindParam(':form_username', $username, PDO::PARAM_STR);
            //Passwort mit dem sha1 Hash verschlüsseln
            $stmt->bindParam(':form_password', sha1($password), PDO::PARAM_STR, 40);
            $stmt->execute();
            $user_id = $stmt->fetchColumn();               
            if($user_id) { //falls eine ID zu User und Passwort gefunden wurde                
                $this->sh->setAuth(true);
                session_regenerate_id(); //SID regenerieren, um Session Fixation zu verhindern
            } else {                
                $this->sh->setError("Benutzername oder Passwort falsch"); //Errorcode in Session ablegen
            }
        } catch (PDOException $e) {
            $this->sh->setError("Fehler in SQL Anweisung");
        }
    }

    public function checkLogin() {        
        if (!$this->sh->isAuth()) { //wenn nicht authorisiert            
            $error = $this->sh->getError();            
            $referrer = $this->sh->getReferrer();
            $vars = compact('referrer', 'error');
            Haanga::Load('login.html', $vars); //Template aufrufen
            exit; //alle weiteren Skripte werden gestoppt, da man nicht eingeloggt ist
        }
    }

    public function logout() {
        $this->sh->destroy();
    }
}

class SessionHandler {

    private $referrer;    

    public function __construct() {
        $this->referrer = basename($_SERVER["REQUEST_URI"]);
        session_start();
    }
    
    public function destroy() {
        setcookie(session_name(), '', time()-3600,'/'); 
        session_destroy();
    }
    
    public function setAuth($auth) {
        if ($auth) {
            $_SESSION['auth'] = 1;
        }
    }
    
    public function isAuth() {
        if (isset($_SESSION['auth']))
            return true;
    }
    
    public function setError($error) {
        $_SESSION['error'] = $error;
    }
    
    public function getError() {
        if (isset($_SESSION['error']))
            return $_SESSION['error'];
    }
    
    public function getReferrer() {
        return $this->referrer;
    }

}

if(isset($_POST['login_submit'])) {
    //Datenbankverbindung aufbauen    
    $sh = new SessionHandler();
    $db = new PDO('mysql:host='.MYSQL_HOST.';dbname='.MYSQL_DB, MYSQL_USER, MYSQL_PASSWORD);
    $login = new Auth($sh, $db);
    $db = null;
    //GET-Variablen von HTML-Tags befreien
    $user = filter_input(INPUT_POST, 'user', FILTER_SANITIZE_STRING);
    $pass = filter_input(INPUT_POST, 'user_pass', FILTER_SANITIZE_STRING);
    $login->login($user, $pass); //einloggen, falls Daten stimmen
    header("Location: ". $_POST['referrer'] .""); //zu der Seite umleiten, die ursprünglich hätte aufgerufen werden sollte
}
else if (isset($_POST['logout'])) {   
    $sh = new SessionHandler();
    $login = new Auth($sh);    
    $login->logout();
    header("Location: index.php"); //zur Startseite umleiten
} 

Und um eine Seite zu schützen, muss dann folgender Aufruf ganz oben auf die Seite:

$sh = new SessionHandler();
$db = new PDO('mysql:host='.MYSQL_HOST.';dbname='.MYSQL_DB, MYSQL_USER, MYSQL_PASSWORD);
$login = new Auth($sh, $db);
$login->checkLogin(); 

Sieht jetzt insgesamt etwas aufgeräumter aus, wobei ich das Gefühl habe, dass so Funktionen wie isAuth() eigentlich in die Auth-Klasse gehören. Und leider kann ich jetzt nicht mehr innerhalb der Klasse abfangen, wenn die Datenbankverbindung nicht funktionieren sollte, da das Objekt dazu ja nun außerhalb der Klasse erstellt wird. Hmm...naja, was haltet ihr jetzt davon?

[AmicaNoctis]

Und leider kann ich jetzt nicht mehr innerhalb der Klasse abfangen, wenn die Datenbankverbindung nicht funktionieren sollte, da das Objekt dazu ja nun außerhalb der Klasse erstellt wird.

Wie du merkst, übernimmt deine Klasse eine Verantwortung für die korrekte Datenbankabfrage. Verantwortungen kann man aber delegieren. Das try…catch könnte jetzt also genausogut außerhalb der Klasse untergebracht werden.

[litterauspirna]

Hallo,

du bist mit deinem Auth Objekt nun immer noch furchtbar unflexibel, du beschrämkst es schier auf eine Datenbank Authentifizierung.
Ziel der OOP ist aber der Mehrwert durch Wiederverwendbarkeit und der ist bei dir überhaupt nicht gewährleistet.

Was wenn du mal ein Projekt hast und sollst ein LDAP Login schaffen? Dann fängst den ganzen Plunder neu zu schreiben nur dafür.

Also schreibe dein Authobjekt so, dass es nur Hauptfunktionalität bietet und die tatsächlichen Kommunikatonen mit Speichermedien lagerst du in Adapter aus.

So bist du mit diesem Authobjekt immer flexibel und übergibst mittles DI einfach ne Adapterinstanz.

[darton]

OK, was sagst du hier zu?

class Auth {
    
    private $login;
    private $sh; 

    public function __construct($sh, $login = null) {       
        $this->login = $login;
        $this->sh = $sh;        
    }

    public function login($username, $password) {        
        if($this->login->dbLogin($username, $password)) { //falls eine ID zu User und Passwort gefunden wurde                
            $this->sh->setLogin(true);
            session_regenerate_id(); //SID regenerieren, um Session Fixation zu verhindern
        }        
    }

    public function checkAuth() {  
        if (!$this->sh->logged_in()) { //wenn nicht authorisiert            
            $error = $this->sh->getError();            
            $referrer = $this->sh->getReferrer();
            $vars = compact('referrer', 'error');
            Haanga::Load('login.html', $vars); //Template aufrufen
            exit; //alle weiteren Skripte werden gestoppt, da man nicht eingeloggt ist
        }
    }

    public function logout() {
        $this->sh->free();
    }
}

class DBLogin {

    private $db;
    private $sh;

    public function __construct($sh) {       
        $this->sh = $sh;        
    }
    
    public function dbLogin($username, $password) {
        try {
            $this->db = new PDO('mysql:host='.MYSQL_HOST.';dbname='.MYSQL_DB, MYSQL_USER, MYSQL_PASSWORD);
            //Error-Modus auf Exception umstellen
            $this->db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $stmt = $this->db->prepare("SELECT id, user, password FROM benutzer WHERE user = :form_username AND password = :form_password");
            //Variablen im prepared Statement belegen und nach User und Password suchen
            $stmt->bindParam(':form_username', $username, PDO::PARAM_STR);
            //Passwort mit dem sha1 Hash verschlüsseln
            $stmt->bindParam(':form_password', sha1($password), PDO::PARAM_STR, 40);
            $stmt->execute();
            $user_id = $stmt->fetchColumn();
            $db = null;            
            if ($user_id) { //falls eine ID zu User und Passwort gefunden wurde
                return true;
            } else {
                $this->sh->setError("Benutzername oder Passwort falsch");
                return false; 
            }
        } catch (PDOException $e) {
            $this->sh->setError("Fehler bei der Verbindung mit der SQL Datenbank");
        }
    }

}

class SessionHandler {

    private $referrer;    

    public function __construct() {
        $this->referrer = basename($_SERVER["REQUEST_URI"]);
        session_start();
    }
    
    public function free() {
        setcookie(session_name(), '', time()-3600,'/'); 
        session_destroy();
    }
    
    public function setLogin($auth) {
        if ($auth) {
            $_SESSION['auth'] = 1;
        }
    }
    
    public function logged_in() {
        if (isset($_SESSION['auth']))
            return true;
    }
    
    public function setError($error) {
        $_SESSION['error'] = $error;
    }
    
    public function getError() {
        if (isset($_SESSION['error']))
            return $_SESSION['error'];
    }
    
    public function getReferrer() {
        return $this->referrer;
    }

} 

Muss allerdings sagen, dass fast alle Login Skripte im Internet vom OOP-Gedanken her ziemlich schlecht sind.

[litterauspirna]

Das sieht schon besser aus, eine Sache gefällt mir persönlich nicht, du hast in der DB Login schon wieder ne direkte Abhängigkeit zum PDO Objekt, warum übergibst das nicht via DI?

Direkte Abhängigkeiten sollten so oft und so gut wie möglich vermieden werden.

Dieses session_regenerate_id() gehört nicht ins Auth Objekt sondern bei erfolgreicher Authentifizierung vom Session Objekt heraus aufgerufen.

Muss allerdings sagen, dass fast alle Login Skripte im Internet vom OOP-Gedanken her ziemlich schlecht sind.

Nunja das ist im Netz nun mal so, man findet mal gute und mal weniger gute Sachen.
Man muss sich, denke ich mal, von dem Gedanken Login trennen. Ein Login ist eine Aktion eines Visitors und kein Objekt.
Ein Authentifizierung ist aber ein Objekt was ein Login verarbeiten kann und eben noch viel mehr. Mit gescheiten Adaptern kann man das dann für alle möglichen Art und Weisen einer Authentifizierung nutzen, vollkommen flexibel und losgelöst.
Ich würde meinen Beispielcode gern hier posten, nur ist das auch noch in der Entwicklung und läuft nicht Astrein, von daher will ich keine halben Sachen hier drin posten.

In deinem Session Handler hast du Sachen wie setLogin(). Das ist aber, wie du vom Namen und dem Bezug des Session Objektes schon raus lesen kannst auch falsch.
Allgemein ist die SessionHandler Geschichte eigentlich Mist.

Ich mache das so.

class SessionHandler
{
    protected $_namespace;
    
    public function __construct($pNamespace = 'Default')
    {
        $this->_namespace = $pNamespace;
    }
    
    public function getNameSpace()
    {
        return $this->_namespace;
    }
    
    public function getSession()
    {
        return $_SESSION[$this->_namespace];
    }
    
    public function __set($pKey, $pValue)
    {
        $_SESSION[$this->_namespace][$pKey] = $pValue;
    }
    
    public function __get($pKey)
    {
        if($_SESSION[$this->_namespace][$pKey] != '') {
            return $_SESSION[$this->_namespace][$pKey];
        }
    }
    
    public function resetNamespace($pKey)
    {
        unset($_SESSION[$pKey]);
    }
} 

Sieh das bitte auf keinen Fall als den Maßstab an, dass ist nur eine Möglichkeit.
Da wird bei der Instanz ein Namespace angelegt und über die magischen __set() und __get() Methoden schreibe ich Werte in einem benutzenden Objekt in meinen Namespace hinein und kann über alle Objekte die in meinem FrontController laufen darauf zugreifen.
Das heißt das mein Auth Objekt dieses Session Objekt benutzt und erst da im Auth Objekt bekommt ein Session Namespace Werte zugewiesen.
Ich habe zusätzlich noch ne Klassen die mir die Instanzen speichert und festhält und übergebe diese Klasse bzw. das daraus resultierende Objekt mit DI.

Gruß Litter

[unset]

Also, wenn man Realitäten abbilden will und es ganz streng nehmen möchte, dann ist auch "Auth" eine bananige Klasse. Im Grunde ist es die Entität User, die gegen eine wie auch immer geartete Instanz authentifiziert wird. Das ist eine Interaktion zweier Entitäten und diese wird –*jedenfalls im MVC-Modell –*durch eine Action eines Controller gesteuert.

Aber im Grunde ist der von Anja und Litter (ich nenne dich jetzt einfach mal so) "geforderte" Ansatz erprobt, durchaus verbreitet und völlig ok.