nicht angemeldet
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 $camelCaseVariablen sowie $_protectedCamelCaseVars
5. Ü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