Login Klasse/Meinung/Verbesserungsvorschläge
2 0 0 0 0 0 
dedlfix
0
2 0
Login Klasse/Meinung/Verbesserungsvorschläge
Hi,
ich hab mir mal vor einiger Zeit eine Login Klasse erstellt bzw. von einem Tutorial übernommen und weiterverbreitet.
Jetzt würd mich mal interessieren ob man da noch was verbessern könnte sowohl an der performance als auch an der Sicherheit.
Kurz zur Klasse: das $Config Array hab ich in eine extra Datei gepackt, welche auch eingebunden wird. Unter der Klasse ist noch ein kleines Anwendungsbeispiel.
Ich würd mich über einen kurzen Blick darauf freuen.
Das wäre mal die login.class.php:
----------------------------------
<?php
class Login {
public $UserId = 0;
public $Username = "";
public $Password = "";
private $Salt = "";
private $Config = array();
private $DBConnection = NULL;
private $DBSelected = NULL;
private $DBError = "";
public function __construct($Config) {
$this->Config = $Config;
ini_set('session.use_only_cookies', '1');
ini_set('session.use_trans_sid', '0');
if ($this->Config['Notices']) {
error_reporting(E_ALL|E_STRICT);
ini_set('display_errors', '1');
} else {
error_reporting(0);
ini_set('display_errors', '0');
}
if($this->DBConnection === NULL) {
$this->DBConnect();
} else {
$this->DBCheckCon();
}
}
private function DBConnect() {
if($this->DBConnection !== NULL) {
$this->DBCheckCon();
return TRUE;
}
$this->DBConnection = new mysqli($this->Config['DBHost'], $this->Config['DBUser'], $this->Config['DBPass'], $this->Config['DBName']);
if (mysqli_connect_error()) {
$this->DBError = 'Connect Error (' . mysqli_connect_errno() . ') '.mysqli_connect_error()." in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>";
$this->DBConnection = NULL;
$this->DBDebug();
return FALSE;
}
return TRUE;
}
private function DBCheckCon() {
if(!$this->DBConnection->ping()) {
$this->DBConnection = NULL;
$this->DBConnect();
}
}
private function DBDebug() {
if($this->Config['Debug']) {
echo $this->DBError;
}
return TRUE;
}
function LoginUserAuth($User, $Pass, $FromCookie = FALSE) {
$this->DbCheckCon();
$Query = 'SELECT
`ID`,
`UserId`,
`alias`,
`password`,
`rights`,
`salt`,
`cookie_hash`
FROM
`'.$this->Config['TablePrefix'].'user`
WHERE
LOWER(`alias`) = LOWER(?)';
$Insert = $this->DBConnection->stmt_init();
if(!$Insert->prepare($Query)) {
$this->DBError = 'Query Error (' . $Insert->errno . ') '.$Insert->error." in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>";
$this->DBDebug();
}
$Insert->bind_param('s', $User);
$Insert->execute();
$Insert->store_result();
$NumRows = $Insert->num_rows;
if($NumRows == 1) {
$Insert->bind_result($DBID, $DBUserId, $DBAlias, $DBPass, $DBRights, $DBSalt, $DBCookieHash);
$Insert->fetch();
if($FromCookie) {
if($DBCookieHash != $Pass) {
return FALSE;
}
} else {
if($DBPass != md5($Pass.$DBSalt.$this->Config['LoginStaticSalt'])) {
return FALSE;
}
}
$this->UserId = $DBUserId;
$this->Username = $DBAlias;
$this->Password = $DBPass;
$this->Rights = $DBRights;
$this->Salt = $DBSalt;
$Insert->close();
return TRUE;
}
return FALSE;
}
function LoginUser($User, $Pass, $StayLoggedIn = FALSE) {
if(get_magic_quotes_gpc()) {
$User = stripslashes($User);
$Pass = stripslashes($Pass);
}
$User = trim($User);
$UserAuth = $this->LoginUserAuth($User, $Pass);
if($UserAuth) {
if($StayLoggedIn) {
$this->CookieSetup($this->Username, $this->GenerateHash(32));
}
$this->SessionSetup($User);
usleep(rand(500000,1500000));
return TRUE;
}
sleep(rand(2,4));
return FALSE;
}
function LoginCheck() {
if(isset($_COOKIE[$this->Config['CookieName'].'user_login'])) {
$String = $_COOKIE[$this->Config['CookieName'].'user_login'];
$Explode = explode($this->Config['CookieSeperator'], $String);
if(is_array($Explode)) {
$User = $Explode[0];
$Hash = $Explode[1];
} else {
echo "Fehler beim Auslesen des Cookies!";
return FALSE;
}
if($this->LoginUserAuth($User, $Hash, TRUE)) {
return TRUE;
}
}
if(isset($_SESSION['UserData'])) {
if(isset($_SESSION['UserData']['Name']) AND $_SESSION['UserData']['Name'] === $this->Username AND isset($_SESSION['UserData']['Login']) AND $_SESSION['UserData']['Login']) {
return TRUE;
}
}
return FALSE;
}
function Logout() {
if(isset($_COOKIE[$this->Config['CookieName'].'user_login'])) {
$this->CookiesDestroy();
}
$this->SessionDestroy();
header('Location: index.php');
}
function CookieSetup($User, $Hash) {
$CookieString = $User.$this->Config['CookieSeperator'].$Hash;
setcookie($this->Config['CookieName'].'user_login', $CookieString, time() + $this->Config['CookieTimeout'], $this->Config['CookiePath']);
$Query = 'UPDATE `'.$this->Config['TablePrefix'].'user` SET `cookie_hash` = ? WHERE LOWER(`alias`) = LOWER(?)';
$Insert = $this->DBConnection->stmt_init();
if(!$Insert->prepare($Query)) {
$this->DBError = "Query Error in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>";
$this->DBDebug();
}
$Insert->bind_param('ss', $Hash, $User);
$Insert->execute();
$Insert->close();
return TRUE;
}
function SessionSetup($User) {
$_SESSION['UserData']['Name'] = $User;
$_SESSION['UserData']['UserId'] = $this->UserId;
$_SESSION['UserData']['Rights'] = $this->Rights;
$_SESSION['UserData']['Login'] = TRUE;
return TRUE;
}
private function CookiesDestroy() {
setcookie($this->Config['CookieName'].'user_login', '', 0, $this->Config['CookiePath']);
return TRUE;
}
private function SessionDestroy() {
session_unset();
$_SESSION = array();
session_destroy();
return TRUE;
}
private function GenerateHash($HashSize) {
$Hash = "";
srand((double)microtime()*1000000);
for($i=0; $i < $HashSize; $i++) {
$Number = rand(48,120);
while (($Number >= 58 && $Number <= 64) || ($Number >= 91 && $Number <= 96)) {
$Number = rand(48,120);
}
$Hash .= chr($Number);
}
return $Hash;
}
}
?>
--------------------------------------------------------------------------
--------------------------------------------------------------------------
--------------------------------------------------------------------------
--------------------------------------------------------------------------
und hier das Ganze mal in Verwendung:
<?php
//Klasse einbinden
//$Config einbinden
ob_start();
$Login = new Login($Config);
session_start();
if (!isset( $_SESSION['server_SID'] )) {
session_unset();
$_SESSION = array();
session_destroy();
session_start();
session_regenerate_id();
$_SESSION['server_SID'] = TRUE;
}
if(isset($_GET['logout']) && ($_GET['logout'] == "true")) {
$Login->Logout();
}
if(isset($_POST['Submit'])) {
if($Login->LoginUser($_POST['UName'], $_POST['UPass'], TRUE)) {
//Login Erfolgreich
} else {
//Login nicht Erfolgreich
}
}
//Formular
ob_end_flush();
?>
-
Moin!
Jetzt würd mich mal interessieren ob man da noch was verbessern könnte sowohl an der performance als auch an der Sicherheit.
Man kann ganz viel verbessern.
1. Deine Klasse sollte allein die Aufgabe haben, über die Korrektheit von angegebenen User-Authentifizierungsdaten zu entscheiden. Sie hat aber zusätzlich auch noch eine DB-Connection intern eingebaut. Sowas ist schlechte Abstraktion - was machst du, wenn andere Teile deines Codes auch auf eine Datenbank zugreifen wollen?
2. Dein Code ist außerdem schlecht wiederverwendbar, weil alle wichtigen internen Eigenschaften und Methoden private sind, anstelle protected zu sein. Nur mit letzteren werden sie auch vererbt. Private sollte man sehr zurückhaltend und mit Verstand einsetzen, denn es behindert Vererbung - und Vererbung ist eines der Grundbausteine von OOP.
3. Vom Coding-Style her ist es ebenfalls unvorteilhaft, dass du deine public-Methoden nicht als solche gekennzeichnet hast. So wie das derzeit geschrieben ist, sieht das sehr nach einer Übernahme aus PHP 4 aus, selbst wenn das nicht stimmen sollte.
4. Ebenfalls Coding-Style-relevant ist, wie die Variablen benannt werden, die du verwendest. Es gibt natürlich unterschiedlichste Meinungen, wie das optischer Erscheinungsbild zu sein hat. Mein Favorit sind
$camelCaseVariablensowie$_protectedCamelCaseVars5. Übel ist, dass du noch innerhalb deiner Klasse Stripslashes entfernst. Sowas gehört hier nicht hinein, alle Variablenwerte, die an Methoden übergeben werden, sollten als "sauber" behandelt werden im Sinne von Stripslashes. D.H. hier kommen nur die reinen Strings an, keine Extra-Slashes. Denn diese Slash-Behandlung findet nur in einer einzigen Methode statt, alle anderen ignorieren die Slashes schon von vornherein.
6. Security: Das Seeden des Pseudo-Randomnumber-Generators ist seit PHP 4.2.0 überflüssig, und mit der von dir gewählten Methode (die man leider viel zu oft in irgendwelchen Tutorials sieht) ist sie auch noch ziemlich unsicher, weil vorhersagbar. Du benutzt die Microtime - ein Wert, den man mit etwas Aufwand auf eine Sekunde genau eingrenzen wird können, zumal Webserver oftmals diese Information auch freiwillig mit in ihre HTTP-Antwort packen. Richtig vernünftige Zufallswerte entnimmt man den Dateien /dev/random bzw. /dev/urandom, oder befragt die angebundene SSL-Bibliothek nach gutem Random. Alles andere selbstausgedachte führt vermutlich zu eher unzufälligen Resultaten. Insbesondere ist zu fragen, aus welchem Grund hier nochmals ein eigener Hash generiert werden muss, wenn doch die Session-ID von PHP im Prinzip ausreichend zufällig ist. Mehr Sicherheit wird dadurch jedenfalls nicht generiert, dass man nicht mehr nur genau ein Cookie kennen muss, sondern zwei.
7. session_unset() ist uralt-ekliger PHP 4.0-Session-Krams und gehört, gemeinsam mit session_register(), entsorgt. Ich bin mir auch nicht sicher, dass du weißt, was session_destroy() genau macht. Es ist ebenfalls überflüssig, die relevanten Lösch-Vorgänge löst man heutzutage mit entsprechenden Aktionen auf $_SESSION aus.
8. Dass dein Datenbankzugriff Prepared Statements benutzt, macht die Sache nicht wirklich einfach zu handhaben, oder? Performance gewinnst du damit sicherlich nicht, und das Rudel lokaler Variablen, die damit erzeugt werden, sind für meine Augen alles andere als übersichtlich. Du vermeindest damit natürlich vermeintlich, dass böse Userdaten dir dein SQL injizieren - allerdings setzt du trotzdem deine Querys dynamisch zusammen, d.h. wenn ein Angreifer dir in die Variable $this->Config['TablePrefix'] was einschmuggelt, bist du genauso schlecht dran, wie mit Escaping und der traditionellen Methode.
9. Eigenschaften einer Klasse public zu machen halte ich für sehr problematisch. Man gewinnt, dass der Zugriff auf diese Werte leicht von allen anderen Klassen aus möglich ist. Man verliert aber komplett die Kontrolle darüber, was andere Klassen mit diesen Werten machen können - beispielsweise auch Schreibzugriffe!
10. Der Zugriff auf superglobale Variablen innerhalb der Klasse ist hinsichtlich Wiederverwendbarkeit und Kapselung ebenfalls kritisch zu betrachten. Die superglobalen Variablen sind für manchen prozeduralen Code eine gute Erfindung gewesen, letztendlich aber sind es ganz normale globale Variablen - und globale Variable sind böse und sollten unbedingt vermieden werden. Auch wenn sie $_SESSION, $_COOKIE oder $_GET heißen.
und hier das Ganze mal in Verwendung:
Dem Code fehlt der Teil, der das früher erfolgte Login auch tatsächlich prüft... Wo ist der Zugangsschutz?
Die Abhängigkeit von ob_start() ist ebenfalls nicht sehr schön.
- Sven Rautenberg
-
Hello,
Man kann ganz viel verbessern.
-
Deine Klasse sollte allein die Aufgabe haben, über die Korrektheit von angegebenen User-Authentifizierungsdaten zu entscheiden. Sie hat aber zusätzlich auch noch eine DB-Connection intern eingebaut. Sowas ist schlechte Abstraktion - was machst du, wenn andere Teile deines Codes auch auf eine Datenbank zugreifen wollen?
-
Dein Code ist außerdem schlecht wiederverwendbar, weil alle wichtigen internen Eigenschaften und Methoden private sind, anstelle protected zu sein. Nur mit letzteren werden sie auch vererbt. Private sollte man sehr zurückhaltend und mit Verstand einsetzen, denn es behindert Vererbung - und Vererbung ist eines der Grundbausteine von OOP.
-
Vom Coding-Style her ist es ebenfalls unvorteilhaft, dass du deine public-Methoden nicht als solche gekennzeichnet hast. So wie das derzeit geschrieben ist, sieht das sehr nach einer Übernahme aus PHP 4 aus, selbst wenn das nicht stimmen sollte.
-
Ebenfalls Coding-Style-relevant ist, wie die Variablen benannt werden, die du verwendest. Es gibt natürlich unterschiedlichste Meinungen, wie das optischer Erscheinungsbild zu sein hat. Mein Favorit sind
$camelCaseVariablensowie$_protectedCamelCaseVars -
Übel ist, dass du noch innerhalb deiner Klasse Stripslashes entfernst. Sowas gehört hier nicht hinein, alle Variablenwerte, die an Methoden übergeben werden, sollten als "sauber" behandelt werden im Sinne von Stripslashes. D.H. hier kommen nur die reinen Strings an, keine Extra-Slashes. Denn diese Slash-Behandlung findet nur in einer einzigen Methode statt, alle anderen ignorieren die Slashes schon von vornherein.
-
Security: Das Seeden des Pseudo-Randomnumber-Generators ist seit PHP 4.2.0 überflüssig, und mit der von dir gewählten Methode (die man leider viel zu oft in irgendwelchen Tutorials sieht) ist sie auch noch ziemlich unsicher, weil vorhersagbar. Du benutzt die Microtime - ein Wert, den man mit etwas Aufwand auf eine Sekunde genau eingrenzen wird können, zumal Webserver oftmals diese Information auch freiwillig mit in ihre HTTP-Antwort packen. Richtig vernünftige Zufallswerte entnimmt man den Dateien /dev/random bzw. /dev/urandom, oder befragt die angebundene SSL-Bibliothek nach gutem Random. Alles andere selbstausgedachte führt vermutlich zu eher unzufälligen Resultaten. Insbesondere ist zu fragen, aus welchem Grund hier nochmals ein eigener Hash generiert werden muss, wenn doch die Session-ID von PHP im Prinzip ausreichend zufällig ist. Mehr Sicherheit wird dadurch jedenfalls nicht generiert, dass man nicht mehr nur genau ein Cookie kennen muss, sondern zwei.
-
session_unset() ist uralt-ekliger PHP 4.0-Session-Krams und gehört, gemeinsam mit session_register(), entsorgt. Ich bin mir auch nicht sicher, dass du weißt, was session_destroy() genau macht. Es ist ebenfalls überflüssig, die relevanten Lösch-Vorgänge löst man heutzutage mit entsprechenden Aktionen auf $_SESSION aus.
-
Dass dein Datenbankzugriff Prepared Statements benutzt, macht die Sache nicht wirklich einfach zu handhaben, oder? Performance gewinnst du damit sicherlich nicht, und das Rudel lokaler Variablen, die damit erzeugt werden, sind für meine Augen alles andere als übersichtlich. Du vermeindest damit natürlich vermeintlich, dass böse Userdaten dir dein SQL injizieren - allerdings setzt du trotzdem deine Querys dynamisch zusammen, d.h. wenn ein Angreifer dir in die Variable $this->Config['TablePrefix'] was einschmuggelt, bist du genauso schlecht dran, wie mit Escaping und der traditionellen Methode.
-
Eigenschaften einer Klasse public zu machen halte ich für sehr problematisch. Man gewinnt, dass der Zugriff auf diese Werte leicht von allen anderen Klassen aus möglich ist. Man verliert aber komplett die Kontrolle darüber, was andere Klassen mit diesen Werten machen können - beispielsweise auch Schreibzugriffe!
-
Der Zugriff auf superglobale Variablen innerhalb der Klasse ist hinsichtlich Wiederverwendbarkeit und Kapselung ebenfalls kritisch zu betrachten. Die superglobalen Variablen sind für manchen prozeduralen Code eine gute Erfindung gewesen, letztendlich aber sind es ganz normale globale Variablen - und globale Variable sind böse und sollten unbedingt vermieden werden. Auch wenn sie $_SESSION, $_COOKIE oder $_GET heißen.
11. Das Login als solches sollte als Singleton ausgeführt werden. Insbesondere die globalen Eigenschaften
ini_set('session.use_only_cookies', '1');
ini_set('session.use_trans_sid', '0');im Konstruktor der Klasse zu verwursten, halte ich für bedenklich.
12. Die Klasse sollte beim Login auch überprüfen, ob Cookies vom Client akzeptiert werden und alternativ Basic Authentication anbieten.
13. HTTP-Header, wie "header('Location: index.php');" sollten nach RFC ausgeführt werden
14. Mit fehlen hier Methoden für unterschiedliche Protection-Grade und die klare Aussage, ob es ein Login-Check pro Session oder ein Login-Check pro Request oder sogar ein Login-Check pro diskreter Anforderung (auf Methodenebene) sein soll.
Liebe Grüße aus dem schönen Oberharz
Tom vom Berg
-
Ahh, das sind ja schonmal einige Vorschläge.
Ich werde mal das ganze überarbeiten und hier noch mal posten.
Danke,
MfG
Naps-
Hello,
Ahh, das sind ja schonmal einige Vorschläge.
Und einen habe ich noch:
Ich würde das Anmeldeprocedere an der Datenbank mit einem Update vornehmen. Dann hast Du im Erfolgsfall auch gleich den aktuellen Timestamp und falls vorhanden, den zusätzlichen Kontrollcookie, in der DB drin.
Das ist sehr nützlich, wenn Du nachher nachschauen willst, welcher der User "angemeldet" ist, also innerhalb eines gewissen Zeitraumes einen Request abgesetzt hatte.
http://forum.de.selfhtml.org/archiv/2008/4/t170126/#m1111727
Nur im Misserfolgsfall musst Du dann ein Select absetzen, um nachzufragen, warum der User sich nicht anmelden kann.
Die Rechte eines Users sind vermutlich auch eher in einer eigenen Tabelle zu suchen und nicht in derjenigen, in der die Logindaten stehen, da sie zum Userdatensatz vertikal stehen sollten, also einen Datensatz pro Recht haben sollten.
Man könnte die ganze Anmeldung allerdings auch mit einem Bewegungsdatensatz aufbauen in einer speziellen Tabelle "Request". Dann könnte man ein Insert ... Select daraus machen.
Bitte bedenke bei der ganzen Sache aber auch die rechtliche Komponente. Deine Klasse sollte maximal exakt das abbilden, was rechtlich zulässig ist und auch Rücksicht auf eventuell notwenige Einverständnisse des Users nehmen.
Liebe Grüße aus dem schönen Oberharz
Tom vom Berg
-
-
- Die Klasse sollte beim Login auch überprüfen, ob Cookies vom Client akzeptiert werden und alternativ Basic Authentication anbieten.
Weist du da zufällig eine deutsche Seite wo man sich da ein bisschen einlesen kann?
- HTTP-Header, wie "header('Location: index.php');" sollten nach RFC ausgeführt werden
Was meinst du damit?
MfG
Naps -
Moin!
- Das Login als solches sollte als Singleton ausgeführt werden.
Negativ.
"Singleton" ist das einzige Pattern, das nennenswerten Bekanntheitsgrad erlangt hat, deshalb wird es von allen halbwissenden OOP-Programmierern als der heilige Gral erkannt und an allen möglichen und unmöglichen Stellen umgesetzt.
Dummerweise handelt man sich dadurch einige sehr negative Eigenschaften ein, die man bei genauerer Betrachtung nicht haben will:
a) Global State im Objekt: Eine neue Instanz des Objektes ist nicht neu, sondern exakt das alte, schon bisher verwendete Objekt - wenn es denn vorher schon mal verwendet wurde. Es hat also evtl. schon vom Initialzustand abweichende Inhalte, und insbesondere ändert die Nutzung des "anderen" Objekts auch dieses Objekt. Sowas ist schwierig zu debuggen.
b) Sehr eingeschränkte Testbarkeit aus demselben Grund.
c) Singletons sind in den meisten Fällen eigentlich gar nicht notwendig für das, was erreicht werden soll.Insbesondere die globalen Eigenschaften
ini_set('session.use_only_cookies', '1');
ini_set('session.use_trans_sid', '0');im Konstruktor der Klasse zu verwursten, halte ich für bedenklich.
Definitiv ja.
- Die Klasse sollte beim Login auch überprüfen, ob Cookies vom Client akzeptiert werden und alternativ Basic Authentication anbieten.
Funktioniert nur, wenn man den Client zu zwei Requests veranlasst, und zwar für jeden User, ohne wirklichen Mehrwert zu schaffen. Absolut verzichtbar.
- Mit fehlen hier Methoden für unterschiedliche Protection-Grade und die klare Aussage, ob es ein Login-Check pro Session oder ein Login-Check pro Request oder sogar ein Login-Check pro diskreter Anforderung (auf Methodenebene) sein soll.
Es geht um den Login, also die Authentifizierung, nicht um Autorisierung. Das ist ein komplett anderes Gebiet bzw. die deutlich größere Rückseite dieser selben Medaille.
- Sven Rautenberg
-
Hi!
- Das Login als solches sollte als Singleton ausgeführt werden.
Negativ.
Dummerweise handelt man sich dadurch einige sehr negative Eigenschaften ein, die man bei genauerer Betrachtung nicht haben will:
a) Global State im Objekt: Eine neue Instanz des Objektes ist nicht neu, sondern exakt das alte, schon bisher verwendete Objekt - wenn es denn vorher schon mal verwendet wurde. Es hat also evtl. schon vom Initialzustand abweichende Inhalte, und insbesondere ändert die Nutzung des "anderen" Objekts auch dieses Objekt. Sowas ist schwierig zu debuggen.
Genau das ist ja die besondere Eigenschaft eines Singleton, die man haben möchte, wenn man es einsetzt. Was sind die Alternativen? Diszipliniert nur ein einziges Objekt erstellen und dies per Dependency Injection herumzureichen? Das kommt zwar der Testbarkeit der Verwender zu Gute, aber wo lagert man dann dieses eine Objekt, wenn nicht irgendwo global? Seinen globalen Status, wie geöffnete Verbindungen, oder das Liefern von eindeutigen Werten, wozu ein Zählerstand oder eine Liste der bisherigen Werte gepflegt werden muss, soll es ja auch behalten. Warum sollte man und wie kann man den globalen Status wegbekommen?
b) Sehr eingeschränkte Testbarkeit aus demselben Grund.
Aus der Hinsicht sieht das wie ein Nachteil. Testbarkeit ist eine gute Sache, aber nicht für alle Anwendungsfälle verwendbar. Mit DI bekommt man zwar die Anwender testbarer hin, aber das Singleton selbst und seinen gewollten globalen Status ... wenn die verwendeten Testmethoden dafür nicht zielführend sind, sollte man dann nicht eher nach geeigneteren Testverfahren suchen?
c) Singletons sind in den meisten Fällen eigentlich gar nicht notwendig für das, was erreicht werden soll.
Das ist der entscheidende Punkt. Es kommt nicht darauf an, das Singleton-Pattern oder irgendein anderes Vorgehen für sich allein zu betrachten sondern seine Eigenschaften mit den Erfordernissen zu vergleichen. Erst dann kann man die Eigenschaften in positiv für das Vorhaben oder negativ für das Vorhaben einsortieren.
Kommen wir damit zu den wichtigen Fragen: Welche Aufgabe(n) genau soll das Singleton erfüllen (geht eher in Richtung Tom, weil er es vorschlug)? Welche Eigenschaften bringt es mit, die der Erfüllung der Aufgabe entgegenstehen und welche sonstigen Eigenschaften hat es, die sich in dem Fall ungünstig auswirken.
Insbesondere die globalen Eigenschaften
ini_set('session.use_only_cookies', '1');
ini_set('session.use_trans_sid', '0');
im Konstruktor der Klasse zu verwursten, halte ich für bedenklich.
Definitiv ja.Begründung und Alternativen wären sehr hilfreich.
Lo!
-
Hello,
Kommen wir damit zu den wichtigen Fragen: Welche Aufgabe(n) genau soll das Singleton erfüllen (geht eher in Richtung Tom, weil er es vorschlug)? Welche Eigenschaften bringt es mit, die der Erfüllung der Aufgabe entgegenstehen und welche sonstigen Eigenschaften hat es, die sich in dem Fall ungünstig auswirken.
Das ist jetzt wieder das philosophische Problem, ob man im Gesamtkunstwerk PHP überhaupt OOP auf der Anwendungsprogrammiererseite braucht... Meine Meinung dazu kennst Du ja. Man bräuchte sie nicht, wenn die Namensraumverwaltung (Thema Verdeckung) etwas geschickter aufgebaut wäre.
Wenn ich das Ganze erstmal ohne OOP betrachte, dann möchte ich erreichen, das eine Anmeldung innerhalb eines Scriptdurchlaufes (ein Request) eineindeutig ist. Ich muss also genau eine Stelle haben, an der ich mir den Zustand merke und/oder genau ein Verfahren, mit dem ich den Zustand ermitteln kann. Viel schlimmer noch, der Zustand muss sogar an den nächsten Request weitergereicht werden könen!
Üblicherweise speichere ich mir das dann 'irgendwie' in der Session oder einem aus der Session abgeleiteten Datensatz meiner Datenhaltung.
Betrachten wir das Ganze jetzt mal mit OOP. Da sind die Eigenschaften an die Objekte gebunden. Außerhalb der Objekte hat es keine Eigenschaften zu geben. Das wäre ein Bruch der Vorgaben für OOP.
Wenn ich aber nun nur genau einen Zustand haben will inerhalb eines Scriptes, (oder sogar einer Folge von Requests) das als Gesamtkunstwerk ja aus hunderten von Klassen/Includes bestehen kann, dann muss ich dafür sorgen, dass immer dasselbe Objekt benutzt wird für die Ermittlung des Zustandes. Um dies zu erreichen fällt mir nur "Singleton" ein. Ob man dies nun durch eine äußere Hilfskonstruktion erzeugt, oder sauber durchstrukturiert, ist für den Zweck unerheblich.
Insbesondere die globalen Eigenschaften
ini_set('session.use_only_cookies', '1');
ini_set('session.use_trans_sid', '0');
im Konstruktor der Klasse zu verwursten, halte ich für bedenklich.
Definitiv ja.Begründung und Alternativen wären sehr hilfreich.
Wenn ich diese Voreinstellungen nur als Direktive einsetze und nicht als Sensitive mit anschließender Direktive und Wiederherstellung des vorherigen Zustandes, dann halte ich das für bedenktlich. Entweder, sie gelten für die gesamte Abarbeitung des Request, oder wenn ich einräume, dass dies nicht der Fall sein könnte, muss ich die Änderung der Einstellung an die Nutzung der Methoden des Objektes binden und nicht an die Instantiierng. Es dürfte dann also immer nur für die Dauer der Nutzung einer Methode die gewünschte Eigenschaft eingestellt werden und bei Ende der Methode, diese wieder auf den vorherigen Stand zurückgestellt werden.
Liebe Grüße aus dem schönen Oberharz
Tom vom Berg
- Das Login als solches sollte als Singleton ausgeführt werden.
-
-
HI,
- Deine Klasse sollte allein die Aufgabe haben, über die Korrektheit von angegebenen User-Authentifizierungsdaten zu entscheiden. Sie hat aber zusätzlich auch noch eine DB-Connection intern eingebaut. Sowas ist schlechte Abstraktion - was machst du, wenn andere Teile deines Codes auch auf eine Datenbank zugreifen wollen?
Das mit der DB-Connection werd ich in eine andere Klasse packen bzw. hab ich eigentlich schon eine die ich so verwende.
Was meinst du genau mit "sollte allein die Aufgabe haben, über die Korrektheit zu entscheiden" ?
- Dein Code ist außerdem schlecht wiederverwendbar, weil alle wichtigen internen Eigenschaften und Methoden private sind, anstelle protected zu sein. Nur mit letzteren werden sie auch vererbt. Private sollte man sehr zurückhaltend und mit Verstand einsetzen, denn es behindert Vererbung - und Vererbung ist eines der Grundbausteine von OOP.
ok...
- Vom Coding-Style her ist es ebenfalls unvorteilhaft, dass du deine public-Methoden nicht als solche gekennzeichnet hast. So wie das derzeit geschrieben ist, sieht das sehr nach einer Übernahme aus PHP 4 aus, selbst wenn das nicht stimmen sollte.
Ja was das betrifft werd ich mir das nochmal anschauen, das hab ich glaub ich falsch verstanden.
- session_unset() ist uralt-ekliger PHP 4.0-Session-Krams und gehört, gemeinsam mit session_register(), entsorgt. Ich bin mir auch nicht sicher, dass du weißt, was session_destroy() genau macht. Es ist ebenfalls überflüssig, die relevanten Lösch-Vorgänge löst man heutzutage mit entsprechenden Aktionen auf $_SESSION aus.
Soll ich da einfach jeden Wert von $_SESSION löschen?
- Dass dein Datenbankzugriff Prepared Statements benutzt, macht die Sache nicht wirklich einfach zu handhaben, oder? Performance gewinnst du damit sicherlich nicht, und das Rudel lokaler Variablen, die damit erzeugt werden, sind für meine Augen alles andere als übersichtlich. Du vermeindest damit natürlich vermeintlich, dass böse Userdaten dir dein SQL injizieren - allerdings setzt du trotzdem deine Querys dynamisch zusammen, d.h. wenn ein Angreifer dir in die Variable $this->Config['TablePrefix'] was einschmuggelt, bist du genauso schlecht dran, wie mit Escaping und der traditionellen Methode.
ok, hab gedacht dass es die beste Lösung wäre, werd ich noch überarbeiten.
- Der Zugriff auf superglobale Variablen innerhalb der Klasse ist hinsichtlich Wiederverwendbarkeit und Kapselung ebenfalls kritisch zu betrachten. Die superglobalen Variablen sind für manchen prozeduralen Code eine gute Erfindung gewesen, letztendlich aber sind es ganz normale globale Variablen - und globale Variable sind böse und sollten unbedingt vermieden werden. Auch wenn sie $_SESSION, $_COOKIE oder $_GET heißen.
Was meinst du hier genau?
Nochmal danke für die Ausführlichen Vorschläge,
MfG Naps-
Hello,
- session_unset() ist uralt-ekliger PHP 4.0-Session-Krams und gehört, gemeinsam mit session_register(), entsorgt. Ich bin mir auch nicht sicher, dass du weißt, was session_destroy() genau macht. Es ist ebenfalls überflüssig, die relevanten Lösch-Vorgänge löst man heutzutage mit entsprechenden Aktionen auf $_SESSION aus.
Soll ich da einfach jeden Wert von $_SESSION löschen?
Ein
$_SESSION = array();
würde alle Werte im Sessionarray löschen, ohne das Array selber entgültig zu entfernen.
Aber mach Dir zu allererst mal Gedanken, ob Du das "Login" als Bestandteil der Session, oder die Session als Bestandteil des "Login" behandeln willst.
Das ist bei HTTP und den üblichen Webtechnologien durchaus ein philosphisches Problem.
Bei anderen Protokollen stellt sich die Frage gar nicht erst, da ein Client dort über seine Connection erkannt werden kann.
Liebe Grüße aus dem schönen Oberharz
Tom vom Berg
-
Ein
$_SESSION = array();
würde alle Werte im Sessionarray löschen, ohne das Array selber entgültig zu entfernen.
Aber mach Dir zu allererst mal Gedanken, ob Du das "Login" als Bestandteil der Session, oder die Session als Bestandteil des "Login" behandeln willst.
Das ist bei HTTP und den üblichen Webtechnologien durchaus ein philosphisches Problem.
Bei anderen Protokollen stellt sich die Frage gar nicht erst, da ein Client dort über seine Connection erkannt werden kann.
Kannst du darauf vielleicht ein bisschen tiefer eingehen?
MfG
Naps
-
Moin!
- Deine Klasse sollte allein die Aufgabe haben, über die Korrektheit von angegebenen User-Authentifizierungsdaten zu entscheiden. Sie hat aber zusätzlich auch noch eine DB-Connection intern eingebaut. Sowas ist schlechte Abstraktion - was machst du, wenn andere Teile deines Codes auch auf eine Datenbank zugreifen wollen?
Das mit der DB-Connection werd ich in eine andere Klasse packen bzw. hab ich eigentlich schon eine die ich so verwende.
Was meinst du genau mit "sollte allein die Aufgabe haben, über die Korrektheit zu entscheiden" ?
Eben genau das: Die Login-Klasse entscheidet (durch DB-Abfrage mithilfe eines ANDEREN Objekts), ob jemand dem System mit Username + Passwort bekannt ist, oder nicht. Und nur genau das, nicht mehr. Also insbesondere keine DB-Connection-Herstellung.
- session_unset() ist uralt-ekliger PHP 4.0-Session-Krams und gehört, gemeinsam mit session_register(), entsorgt. Ich bin mir auch nicht sicher, dass du weißt, was session_destroy() genau macht. Es ist ebenfalls überflüssig, die relevanten Lösch-Vorgänge löst man heutzutage mit entsprechenden Aktionen auf $_SESSION aus.
Soll ich da einfach jeden Wert von $_SESSION löschen?
$_SESSION = array();- fertig.Geht von der irrtümlichen Annahme weg, dass das Existieren einer Session irgendeine Aussage hinsichtlich des Login-Zustands haben würde. Sessions existieren per (vernünftiger) Definition ewig, werden aber eventuell bei zu langer Nichtbenutzung von der Garbage-Collection mal weggeschmissen. Das Session-Handling übernimmt aber komplett PHP.
Dort hinein setzt man sein Login-Handling. Eine brandneue Session enthält keinerlei Infos, also muss man offensichtlich den User erstmal nach seinen Login-Infos fragen. Aber auch eine zu lange nicht benutzte Session, die noch existiert, enthält keine brauchbaren Infos mehr: Der User sollte nach zu langer Zeit der Inaktivität als ausgeloggt betrachtet werden und sich neu anmelden müssen. Dieser Effekt darf aber keinesfalls durch die Garbage-Collection oder ein ungültig gewordenes, weil zeitlich ausgelaufenes Session-Cookie erzeugt werden, denn sowas ist userseitig sehr leicht zu manipulieren, bzw. serverseitig einfach zu unzuverlässig realisiert (das Löschen unbenutzter Sessions geschieht nur mit einer Wahrscheinlichkeit bei einem Session-Skriptaufruf - Server mit nur wenigen Besuchern räumen Sessions möglicherweise tagelang nicht weg).
- Der Zugriff auf superglobale Variablen innerhalb der Klasse ist hinsichtlich Wiederverwendbarkeit und Kapselung ebenfalls kritisch zu betrachten. Die superglobalen Variablen sind für manchen prozeduralen Code eine gute Erfindung gewesen, letztendlich aber sind es ganz normale globale Variablen - und globale Variable sind böse und sollten unbedingt vermieden werden. Auch wenn sie $_SESSION, $_COOKIE oder $_GET heißen.
Was meinst du hier genau?
Wenn du in deiner Klasse z.B. auf $_COOKIE mit bestimmten Parametern fix zugreifst, dann verbindest du deine Klasse unwiderruflich mit der Tatsache, dass a) immer Cookies genutzt werden und b) auch die Namen der Cookies immer gleichbleiben. Der Einsatz der Klasse in einem anderen Projekt wird dadurch behindert. Dabei kommt es zum korrekten Funktionieren der Prüfung von Username und Passwort bzw. dem Prüfen der Existenz einer Session nur drauf an, dass entsprechende Variablen, die als Parameter an die Methoden übergeben werden, die richtigen Inhalte haben. Die Auswahl, in welchen Request-Variablen wie $_COOKIE diese Info drinsteht, ist Aufgabe des umgebenden Programms, der die Login-Klasse aufruft.
- Sven Rautenberg
-
-
Hi,
ergänzend zu dem, was bereits gesagt wurde: Du solltest bei jedem erfolgreichen Login-Vorgang selbst per session_regenerate_id() aufrufen, um Session-Fixation-Angriffe zu verhindern. Es ist bereits etwas Code vorhanden, der bestimmte Angriffsvektoren ausschließt - aber eben nicht alle.
Faustregel: Jedes mal, wenn sich in einer Session die Privilegien erhöhen (Nicht eingloggt -> eingeloggt z.B.), sollte eine neue Session-ID erzeugt werden (session_regenerate_id), um Session-Fixation-Angriffe zu verhindern.
Viele Grüße,
Christian--
Mein "Weblog" [RSS]
Using XSLT to create JSON output (Saxon-B 9.0 for Java)
How to tell the difference between a science fan and a scientist.-
Hi,
ergänzend zu dem, was bereits gesagt wurde: Du solltest bei jedem erfolgreichen Login-Vorgang selbst per session_regenerate_id() aufrufen, um Session-Fixation-Angriffe zu verhindern. Es ist bereits etwas Code vorhanden, der bestimmte Angriffsvektoren ausschließt - aber eben nicht alle.
Faustregel: Jedes mal, wenn sich in einer Session die Privilegien erhöhen (Nicht eingloggt -> eingeloggt z.B.), sollte eine neue Session-ID erzeugt werden (session_regenerate_id), um Session-Fixation-Angriffe zu verhindern.
Das mach ich doch oder?
if (!isset( $_SESSION['server_SID'] )) { session_unset(); $_SESSION = array(); session_destroy(); session_start(); session_regenerate_id(); $_SESSION['server_SID'] = TRUE; }oder sollte ich das auch lieber in der Klasse machen?
MfG
Naps-
Hallo,
Faustregel: Jedes mal, wenn sich in einer Session die Privilegien erhöhen (Nicht eingloggt -> eingeloggt z.B.), sollte eine neue Session-ID erzeugt werden (session_regenerate_id), um Session-Fixation-Angriffe zu verhindern.
Das mach ich doch oder?
if (!isset( $_SESSION['server_SID'] )) {
session_unset();
$_SESSION = array();
session_destroy();
session_start();
session_regenerate_id();
$_SESSION['server_SID'] = TRUE;
}Nein, da machst Du das nur, wenn die Session nicht initialisiert war, \*nicht\* wenn die Privilegien erhöht werden. In dem Moment, in dem der User sich einloggt (d.h. die Passwortüberprüfung erfolgreich ist), \*genau dann\* muss session\_regenerate\_id() genutzt werden. Die Stelle, an der Du es verwendest, ist zwar nicht schädlich, aber auch nicht wirklich nützlich... (Wobei die Reihenfolge von den ganzen Session-Aufrufen an dieser Codestelle irgendwie seltsam ist... Was ist die Logik dahinter?) Viele Grüße, Christian -- [Mein "Weblog"](http://del.icio.us/chris_se/servertipps) [[RSS](http://del.icio.us/rss/chris_se/servertipps)] [Using XSLT to create JSON output](http://www.christian-seiler.de/projekte/xslt-json/) (Saxon-B 9.0 for Java) [How to tell the difference between a science fan and a scientist.](http://www.smbc-comics.com/index.php?db=comics&id=1777#comic)-
Nein, da machst Du das nur, wenn die Session nicht initialisiert war, *nicht* wenn die Privilegien erhöht werden. In dem Moment, in dem der User sich einloggt (d.h. die Passwortüberprüfung erfolgreich ist), *genau dann* muss session_regenerate_id() genutzt werden. Die Stelle, an der Du es verwendest, ist zwar nicht schädlich, aber auch nicht wirklich nützlich... (Wobei die Reihenfolge von den ganzen Session-Aufrufen an dieser Codestelle irgendwie seltsam ist... Was ist die Logik dahinter?)
*schäm* da hab ich ja so richtig was übersehn. Wegen der Reihenfolge hab ich mir da nicht wirklich Gedanken gemacht.
Das heißt es würde ja:
$_SESSION = array(); session_regenerate_id(); $_SESSION['server_SID'] = TRUE;reichen oder?
MfG
Naps
-
-
-
-
So, ich hab jetzt alles soweit mal angepasst und hab auch gleich noch einige Fragen:
Also hier die login.class.php .............................. ..............................
<?php require_once('mysql.class.php'); class Login extends mysql($config){ protected $UserId = 0; protected $Username = ""; protected $Password = ""; protected $Aktiv = FALSE; protected $Salt = ""; protected $config = array(); public function __construct($config) { $this->Config = $config; ini_set('session.use_only_cookies', '1'); ini_set('session.use_trans_sid', '0'); if ($this->Config['Notices']) { error_reporting(E_ALL|E_STRICT); ini_set('display_errors', '1'); } else { error_reporting(0); ini_set('display_errors', '0'); } } function loginSaveFails() { $this->DBCheckCon(); $query = "INSERT INTO `".$this->Config['TablePrefix']."failed_logins` (`ip`) VALUES (?)"; if(!$stmt = $this->DBConnection->prepare($query)) { $this->DBError = "query Error in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>"; $this->DBDebug(); } $remoteAddr = $_SERVER["REMOTE_ADDR"]; $stmt->bind_param('s', $remoteAddr); $stmt->execute(); if ($stmt->affected_rows != 1) { $this->DBError = 'query Error (' . $stmt->errno . ') '.$stmt->error." in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>"; $this->DBDebug(); } $stmt->close(); return TRUE; } function loginCheckFails() { $this->DBCheckCon(); $query = "SELECT `ip`, `time` FROM `".$this->Config['TablePrefix']."failed_logins` WHERE `ip` = ? AND `time` > ?"; $stmt = $this->DBConnection->stmt_init(); if(!$stmt->prepare($query)) { $this->DBError = "query Error in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>"; $this->DBDebug(); } $remoteAddr = $_SERVER["REMOTE_ADDR"]; $timeDif = (time() - $this->Config['LoginTimeout']); $stmt->bind_param('si', $remoteAddr, $timeDif); $stmt->execute(); $stmt->store_result(); if($stmt->errno != 0) { $this->DBError = 'query Error (' . $stmt->errno . ') '.$stmt->error." in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>"; $this->DBDebug(); } $numRows = $stmt->num_rows; $stmt->close(); return $numRows; } function loginTrialsAvailable() { if($this->loginCheckFails() < $this->Config['LoginRetrys']) { return TRUE; } else { return FALSE; } } function loginUserAuth($user, $pass, $fromCookie = FALSE) { $this->DbCheckCon(); $query = 'SELECT `ID`, `UserId`, `alias`, `password`, `aktiv`, `salt`, `cookie_hash` FROM `'.$this->Config['TablePrefix'].'user` WHERE LOWER(`alias`) = LOWER(?)'; $stmt = $this->DBConnection->stmt_init(); if(!$stmt->prepare($query)) { $this->DBError = 'query Error (' . $stmt->errno . ') '.$stmt->error." in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>"; $this->DBDebug(); } $stmt->bind_param('s', $user); $stmt->execute(); $stmt->store_result(); $numRows = $stmt->num_rows; if($numRows == 1) { $stmt->bind_result($dbId, $dbUserId, $dbAlias, $dbPass, $dbAktiv, $dbSalt, $dbCookieHash); $stmt->fetch(); if($fromCookie) { if($dbCookieHash != $pass) { return FALSE; } } else { if($dbPass != md5($pass.$dbSalt.$this->Config['LoginStaticSalt'])) { $this->loginSaveFails(); return FALSE; } } $this->UserId = $dbUserId; $this->Username = $dbAlias; $this->Password = $dbPass; $this->Aktiv = $dbAktiv; $this->Salt = $dbSalt; $stmt->close(); return TRUE; } return FALSE; } function loginUser($user, $pass, $stayLoggedIn = FALSE) { if(!loginTrialsAvailable()) { return FALSE; } if(!$this->Aktiv) { return FALSE; } $userAuth = $this->loginUserAuth($user, $pass); if($userAuth) { if($stayLoggedIn) { $this->cookieSetup($this->Username, $this->generateHash(32)); } $this->sessionSetup($user); usleep(rand(500000,1500000)); return TRUE; } sleep(rand(2,4)); return FALSE; } function loginCheck() { if(isset($_COOKIE[$this->Config['CookieName'].'user_login'])) { $string = $_COOKIE[$this->Config['CookieName'].'user_login']; $explode = explode($this->Config['CookieSeperator'], $string); if(is_array($explode)) { $user = $explode[0]; $hash = $explode[1]; } else { echo "Fehler beim Auslesen des Cookies!"; return FALSE; } if($this->loginUserAuth($user, $hash, TRUE)) { return TRUE; } } if(isset($_SESSION['UserData'])) { if(isset($_SESSION['UserData']['Name']) AND $_SESSION['UserData']['Name'] === $this->Username AND isset($_SESSION['UserData']['Login']) AND $_SESSION['UserData']['Login']) { return TRUE; } } return FALSE; } function logout() { if(isset($_COOKIE[$this->Config['CookieName'].'user_login'])) { $this->cookiesDestroy(); } $this->sessionDestroy(); header('Location: index.php'); } function cookieSetup($user, $hash) { $cookieString = $user.$this->Config['CookieSeperator'].$hash; setcookie($this->Config['CookieName'].'user_login', $cookieString, time() + $this->Config['CookieTimeout'], $this->Config['CookiePath']); $query = 'UPDATE `'.$this->Config['TablePrefix'].'user` SET `cookie_hash` = ? WHERE LOWER(`alias`) = LOWER(?)'; $stmt = $this->DBConnection->stmt_init(); if(!$stmt->prepare($query)) { $this->DBError = "query Error in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>"; $this->DBDebug(); } $stmt->bind_param('ss', $hash, $user); $stmt->execute(); $stmt->close(); return TRUE; } function sessionSetup($User) { $_SESSION['UserData']['Name'] = $user; $_SESSION['UserData']['UserId'] = $this->UserId; $_SESSION['UserData']['Login'] = TRUE; return TRUE; } function cookiesDestroy() { setcookie($this->Config['CookieName'].'user_login', '', 0, $this->Config['CookiePath']); return TRUE; } function sessionDestroy() { $_SESSION = array(); return TRUE; } function generateHash($hashSize) { $hash = ""; srand((double)microtime()*1000000); for($i=0; $i < $hashSize; $i++) { $Number = rand(48,120); while (($number >= 58 && $number <= 64) || ($number >= 91 && $number <= 96)) { $number = rand(48,120); } $hash .= chr($number); } return $hash; } } ?>
Die mysql.class.php ...................
<?php class mysql { protected $config = array(); protected $dbConnection = NULL; protected $DBSelected = NULL; protected $dbError = ""; protected function __construct($config) { $this->config = $config; if ($this->config['Notices']) { error_reporting(E_ALL|E_STRICT); ini_set('display_errors', '1'); } else { error_reporting(0); ini_set('display_errors', '0'); } if($this->dbConnection === NULL) { $this->dbConnect(); } else { $this->dbBCheckCon(); } } protected function dbConnect() { if($this->dbConnection !== NULL) { $this->dbBCheckCon(); return TRUE; } $this->dbConnection = new mysqli($this->config['DBHost'], $this->config['DBUser'], $this->config['DBPass'], $this->config['DBName']); if (mysqli_connect_error()) { $this->dbError = 'Connect Error (' . mysqli_connect_errno() . ') '.mysqli_connect_error()." in <b>". __CLASS__ ."::". __FUNCTION__ ."</b>"; $this->dbConnection = NULL; $this->dbDebug(); return FALSE; } return TRUE; } protected function dbBCheckCon() { if(!$this->dbConnection->ping()) { $this->dbConnection = NULL; $this->dbConnect(); } } protected function dbDebug() { if($this->config['Debug']) { echo $this->dbError; } return TRUE; } } ?>
und zu guter Letzt die login.php
<?php $config = array(); $config['TablePrefix'] = ''; $config['Debug'] = 1; $config['Notices'] = 1; $Config['DBHost'] = 'localhost'; $Config['DBUser'] = ''; $Config['DBPass'] = ''; $Config['DBName'] = ''; $config['CookieName'] = 'login_'; $config['CookieTimeout'] = 60*60*24*10; $config['CookieSeperator'] = '|.|?|.|'; $config['CookiePath'] = '/'; $config['LoginRetrys'] = 8; $config['LoginTimeout'] = 3600; require_once "login.class.php"; ob_start(); $Login = new login($config); session_start(); if (!isset( $_SESSION['server_SID'] )) { session_unset(); $_SESSION = array(); session_destroy(); session_start(); session_regenerate_id(); $_SESSION['server_SID'] = TRUE; } if(isset($_GET['logout']) && $_GET['logout'] == "true") { $Login->Logout(); } if(isset($_POST['Submit'])) { if(get_magic_quotes_gpc()) { $user = stripslashes($_POST['UName']); $pass = stripslashes($_POST['UPass']); } $user = trim(strtolower(($user)); $pass = trim($pass); if($Login->LoginUser($user, $pass, TRUE)) { echo "Erfolgreich eingeloggt!"; } else { echo "Einloggen fehlgeschlagen!"; } } if($Login->LoginCheck()) { echo "Du bist eingeloggt"; } else { echo "Du bist NICHT eingeloggt"; echo $Form; } ob_end_flush(); ?>So zu den Fragen:
-Ich hab da noch immer Probleme mit "protected", "public" usw. Passt das jetzt so wie ich es gemacht habe?
-Zu
ini_set('session.use_only_cookies', '1'); ini_set('session.use_trans_sid', '0');würd ich mich auch noch über einen Lösungsvorschlag freuen.
-Wegen dem Entfernen der Stripslashes: ich hab jetzt das ganze in der "login.php" gemacht nur frag ich mich ob es nicht doch besser wäre das mit einer Funktion in der "login.class.php" zu machen...
-die superglobalen Variablen: Ich greif doch eh mit $config darauf zu ?
-das mit dem Update beim Login werd ich noch machen
-Ich speicher jetzt noch zusätzlich fehlgeschlagene Anmeldungen in der DB, hab das aber noch nicht getestet, sollte aber hin hauen.
MfG Naps
-
Moin!
-Ich hab da noch immer Probleme mit "protected", "public" usw. Passt das jetzt so wie ich es gemacht habe?
Steht for allen Variablen und Funktionen entweder "protected" oder "public"? Nein. Schreibs hin.
Du hast eine ganz blöde Idee, indem du die Login-Klasse alles von der Mysql-Klasse ERBEN lässt. Das Login ist keine Datenbankklasse, muss also auch nichts erben. Das Login VERWENDET Datenbankzugriffe, benötigt also seinerseits im Konstruktor ein Datenbankobjekt übergeben, welches dann in einer protected-Variable gespeichert wird.
Dein Suchwort für weitere Infos zu diesem Thema lautet "Dependency Injection". Meine empfohlene Vorgehensweise, wenn man für sowas keine eigenen Frameworks verwenden will: Alle normalen Klassen kümmern sich um das Funktionieren hinsichtlich ihrer Aufgabe, und das Zusammenbauen von Objekten geschieht in Factory-Klassen, die wiederum ausschließlich nur Objekte korrekt zusammensetzen. In diesem Fall also eine Factory, die dir ein funktionierendes Login-Objekt zurückgibt, indem dem Konstruktor vom Login ein existierendes Mysql-Objekt übergeben wird - beide Objekte erstellt die Factory:
class Factory { static public function getLoginInstance() { $mysql = new Mysql($config); return new Login($mysql); } } // Verwendung: $login = Factors::getLoginInstance();Zu klärende Details: Wo kommt die Konfiguration her? Schlauerweise auch aus einer entsprechenden Klasse, die Alternative wäre alles, was global verfügbar ist, also z.B. Konstanten, globale Variablen. Singletons. Oder (das ist für derartige Dinge, wenn sie objektorientiert umgesetzt werden, der Favorit) eine Registry, also ein Objekt, welches Objekte entgegennimmt, deren Einmaligkeit in einem laufenden Skript garantiert, und sie an jeder beliebigen Stelle wieder zur Verfügung stellt.
Suchwort an dieser Stelle wäre "Registry pattern".
Kritiker würden natürlich anmerken, dass eine Registry nix anderes macht, sich für multiple Objekte als Singleton zu verhalten, aber der Unterschied zu einem echten Singleton ist, dass eine Registry sich nur um das Annehmen, Unique-Halten und Ausliefern von Objekten kümmert. Das ist ein sehr kleiner Aufgabenbereich, den man auch mal ohne automatisierten Test realisieren kann. Ein Singleton-Objekt hingegen tut ja in der Regel ganz viele nützliche Sachen - die Singleton-Eigenschaft aber untrennbar zu verbinden mit diesen wichtigen Dingen, die man unbedingt testen sollte, ist aber gerade hinderlich.
Einen Konstruktor, wie in der Mysql-Klasse geschehen, protected zu machen ist in der Regel eine ganz schlechte Idee, solange man kein Singleton bauen will (udn das will man in der Regel nicht).
PHP bietet für Funktionsparameter, die Objekte eines bestimmten Typs sein sollen, Typehinting an. Unbedingt benutzen! Kostet zwar winzig Performance, aber das ist viel besser, als wenn eine Klasse das falsche Objekt bekommt und dann in der Funktion komische Fehler wirft.
-Zu
~~~phpini_set('session.use_only_cookies', '1');
ini_set('session.use_trans_sid', '0');> > würd ich mich auch noch über einen Lösungsvorschlag freuen. Das ist PHP-Konfiguration, gehört wenn, dann zentral in eine überall eingebundene Konfigurationsdatei, oder besser noch in die Konfiguration deines PHP. Also php.ini, ggf. auch nur lokale php.ini oder .htaccess, je nach Art der Einbindung des PHP in deinen Server. > -Wegen dem Entfernen der Stripslashes: ich hab jetzt das ganze in der "login.php" gemacht nur frag ich mich ob es nicht doch besser wäre das mit einer Funktion in der "login.class.php" zu machen... > -die superglobalen Variablen: Ich greif doch eh mit $config darauf zu ? $\_SESSION, $\_COOKIE, $\_POST, $\_GET sind superglobale Variablen. Kommen die in deinen Klassen vor, oder nicht? > -das mit dem Update beim Login werd ich noch machen > > -Ich speicher jetzt noch zusätzlich fehlgeschlagene Anmeldungen in der DB, hab das aber noch nicht getestet, sollte aber hin hauen. - Sven Rautenberg
-