Mein Loginsystem - (Communitysystem)

php

php

coolsascha
ID: 146779
L
20 April 2006
982
48
Hallo,
da ich jetzt anfangen will PHP mal richtig zu lernen, hatte ich mir gedacht fange ich mal an ein einfaches kleines Loginsystem zu basteln. :biggrin:
Nun hab ich das auch gemacht, nur ich bin mir da nicht so ganz sicher ob ich das auch sauber und sicher gecodet habe. :LOL:
Ich fände es total nett wenn jemand mal drüber gucken würde und mich mit Tips und meinen Fehlern zuballert.
Danke schon mal jetzt!

Hier zum Downloaden, die aktuellste BETA:
:ugly: [LOGIN VER 2.0.4]:ugly:

Aktuelle Funktionen:
  • Registrieren, einloggen, passwort vergessen..
  • Profil, kleine Nickpage..
  • Userliste, einfache Usersuche..
  • Kleines PN System..
  • Es wird ständig erweitert.
Am besten geht zur letzten Seite um mein neustes Problem zu finden oder um mir einfach einen Hinweis zu geben was ich besser machen kann.. ;)
 
Zuletzt bearbeitet:
Ein paar Anregungen zu dem Code:

:arrow: Zahlen sind keine Strings
:arrow: Code einrücken
:arrow: Statt $foo == "" einfach empty($foo) verwenden
:arrow: session_destroy() verwenden
:arrow: Nur tw. Vorbeugung gegen SQL-Injections (siehe z.B. reg.php)
:arrow: Code-Design zusammengelegt
:arrow: Keine Kommentare und in meinen Augen wirres Codedesign (OOP hilft da sehr viel weiter)

Das war´s so auf den ersten Blick.

Gruß
 
Naja sauber ist denke ich was anderes. Keine einzige stelle im Code sehe ich eingerückt wie es sich gehört. In der reg.php findet man sowas
PHP: 
$aktivierungscode = "1000";
}
$passwortmd = md5($passwort1);
}
}
}
}
}
}
}

if($fehler == ""){
Ich würde stunden brauchen um mich da durchzuwühlen. Dann SQL Injektion schau mal bei Wikipedia. Sehr Interessant das Thema.

Sicher das dies funktioniert?
PHP: 
$query = "SELECT mail FROM Login WHERE mail = '$mail'";
Schaut meiner Meninung nach komisch aus.

Kleiner Tip am Rand bevor du das hier schreibst:
PHP: 
  Nickname: <input type="text" name="nickname" <? print 'value="'.$_POST['nickname'].'"'; ?> size="20"></p>
kannst auch folgendes schreiben:
PHP: 
  Nickname: <input type="text" name="nickname" value="<?php echo $_POST['nickname']; ?>" size="20"></p>
oder:
PHP: 
  Nickname: <input type="text" name="nickname" value="<?= $_POST['nickname']; ?>" size="20"></p>
das verwirrt nicht so sehr mit den ganzen ' ". Letzteres setzt aber Shorttags On vorraus.

Für die Logout würde ich anstatt
PHP: 
session_start();
unset($_SESSION['nickname']);
unset($_SESSION['pw']);
include('include/config.php');
print 'Sie werden weitergeleitet... <meta http-equiv="refresh"content="0;URL=https://'.$url.'/index.php?action=logout">';
session_destroy(); verwenden, dann frage ich mich warum du nochmal die config includest und auf einen Link mit ?action=logout weiterleitest.
 
PHP: 
$hostnew = mysql_escape_string($host);
$usrnew = mysql_escape_string($usr);
$pwnew = mysql_escape_string($pw);
$dbnew = mysql_escape_string($db);
mysql_connect($hostnew,$usrnew,$pwnew) or die ("Keine Verbindung moeglich");
mysql_select_db($dbnew) or die ("Die Datenbank existiert nicht");

Da muss nicht escaped werden, wie gesagt, mal SQL-Injections ansehen, dann weisst du wo man escapen muss und wo nicht.

Sonst - normaler Anfaenger Code, du bist jedenfalls auf dem richtigen Weg. :)
 
Hellimai schrieb:
in meinen Augen, auf den ersten und zweiten Blick, sieht der Code supper aus :)
Zum Vergrößern anklicken....
Sorry, kann ich nicht bestätigen :nö:
Auf den ersten Blick krieg ich Augenkrebs und auf dem zweiten eigentlich auch noch 8O ...und ich hab nur 10min hingeguckt.

Erstmal zur Sauberkeit (die wohl das schlimmere von beiden is):
  • EEIINNRRÜÜCCKKEENN !!!!!!1111111elf :evil: :evil:
    motz.gif
    motz.gif
  • Fünfmal mit ?> zumachen und direkt darauf mit <?php wieder zu öffnen is wohl für die Katz :roll:
  • SELECT * :-?
  • Nicht-numerische Array-Indizes in Hochkommata
  • Konstanten in Uppercases. Die Konstante beim Setzen in Hochkommata.
  • Einheitlicher Programmstil, z.B. Leerzeichen, nach Argument bei Funktionsaufruf
  • Mischung von Inhalt und Script: Templatesystem benutzen. Vorteil, dass auch Fehlermeldungen etc., die nicht das ganze Script laufen lassen, eine komplette Seite mit kompletten Layout haben.
  • Tabellenlayout ist out ! Mach ein CSS-Layout.
Zum Inhalt:
  • Die Datenbank nicht unnötig mit String/Integer-Konversionen aufhalten. Das gilt für PHP wie auch für MySQL (in MySQL kann es schlimme Folgen haben).
  • ... or die() tuts zwar, aber für den Besucher sicherlich nicht besonders toll ;)
  • Query-Ergebnisse freigeben, wirklich alle Daten im Querystring, die nicht konstant sind, escapen. Um das am einfachsten zu handeln, empfiehlt sich eine Klasse.
  • passende Feldlängen in der DB verwenden:
    • Schon mal einen Vornamen mit 65535 Zeichen gesehen ? :roll: Nachnamen und Mailadresse analog.
    • Eine IP mit 18 Stellen ? :hö: Laut deinem Code hat der String höchstens 15 Stellen (xxx.xxx.xxx.xxx). Zusätzlich könnte man noch Platz einsparen durch Umwandlung in eine Long-Zahl (im Vergleich: 18 (deins) > 15 (entspr. deinem Code) > 4 (Long))
    • `aktiviert` is INT(1). Wieso 32 Bits reservieren, wenn du eh nur 2 brauchst ? Besser: ENUM-Datentyp.
  • Kein DOCTYPE vorhanden
Hinzu kommen natürlich alle schon genannten Punkte ;)

Fazit: Mit ein paar mysql_escape_string()-Aufrufen zwar ein Schritt in die richtige Richtung, aber dennoch weit vom Ziel entfernt.

Mein Tip: Bau dieses Script nicht weiter, sondern fang nochmal neu an und versuche, alle hier genannten Punkte abzuhaken. Dann poste erneut und stell dein Werk erneut zur Diskussion :)
 
Ich werde das erstmal bis dahin lösen (dacht ich es mir doch, das dort eine Menge falsch dran ist)..
Erstmal pause bis ichs gelöst habe :biggrin:
 
ActionScripter schrieb:
da würde ich gern einspruch erheben. enum ist imho auch nicht die optimale lösung. entweder macht man das (wenns denn unbedingt string sein soll) mit char(1) oder besser mit tinyint(1)...
Zum Vergrößern anklicken....
Auf den Einspruch hab ich gewartet :biggrin:
Bei wenigen Bits empfiehlt sich aber der ENUM-Datentyp, da der afaik wirklich bitweise gespeichert wird. Ein CHAR(1) oder der üblichere TINYINT(1) verbraucht aber 8 Bits.
Obiges hätte ich abgeschickt, wenn ich nicht nochmal nachgelesen hätte: Klick

ENUM frisst trotzdem 8 Bits, auch wenn nur eines benutzt wird. Im Vergleich zur TINYINT(1)-Variante also speicherplatztechnisch gesehen gleich gut.
Allerdings hast du halt mit ENUM den Vorteil, dass du die Konstanten direkt in MySQL hast und auch in der Query benutzen darfst. Bei einem numerischen Datenfeld musst du die Konstanten in PHP definieren und umsetzen.
 
theHacker schrieb:
Auf den Einspruch hab ich gewartet :
Zum Vergrößern anklicken....
kannst mal sehen, auf mich ist verlass :)
theHacker schrieb:
ENUM frisst trotzdem 8 Bits, auch wenn nur eines benutzt wird. Im Vergleich zur TINYINT(1)-Variante also speicherplatztechnisch gesehen gleich gut.
Allerdings hast du halt mit ENUM den Vorteil, dass du die Konstanten direkt in MySQL hast und auch in der Query benutzen darfst. Bei einem numerischen Datenfeld musst du die Konstanten in PHP definieren und umsetzen.
Zum Vergrößern anklicken....

jaaaaaaa, soweit korrekt, aber: der name der konstanten eines enum-feldes wird auch in mysql abgelegt ... und der verbraucht auch speicher :) also wenn du schlauerweise nicht "y" und "n" (oder 1/0) benutzt, sondern "das ding ist eingeschaltet" und "dat teil is deaktiviert", dann musst du das auch mit zum speicher rechnen ... bin aber zu faul, das jetzt auszurechnen, wieviel das mehr ist ... das kannst du viel besser *g*
 
ActionScripter schrieb:
jaaaaaaa, soweit korrekt, aber: der name der konstanten eines enum-feldes wird auch in mysql abgelegt ... und der verbraucht auch speicher :)
Zum Vergrößern anklicken....
Aber doch nur einmal, nämlich in der Struktur :doh:
ActionScripter schrieb:
also wenn du schlauerweise nicht "y" und "n" (oder 1/0) benutzt, sondern "das ding ist eingeschaltet" und "dat teil is deaktiviert", dann musst du das auch mit zum speicher rechnen ...
Zum Vergrößern anklicken....
Denkfehler ;) Ob du dem Typ dem Typ
ENUM('n','y') oder ENUM('ja, der User ist freigeschaltet','nein, der User muss seinen Account noch per eMail verifizieren') gibst, macht bei einem Datensatz genauso viel aus, wie bei einer Million Datensätzen.

MySQL assoziert die Konstanten als die Werte. Wenn ich 'y' speichere, wird 1 abgespeichert und wenn ich 'ja, der User ist freigeschaltet' in der Query übergebe, wird auch nur ne 1 gespeichert.
 
theHacker schrieb:
Aber doch nur einmal, nämlich in der Struktur :doh: Denkfehler ;)
Zum Vergrößern anklicken....

ja meinte ich ja ... aber ist halt eben trotzdem mehr als nur tinyint() ... nu hab dich nicht so und lass mich auch mal pingelig sein, wenn ice-breaker das schon nicht macht ;)

aber um beim thema zu bleiben ... klar ist es geschmackssache, ob man nun enum oder tinyint benutzt. ich mags halt lieber "mathematischer" also als zahl und definiere im programm, was die zahl zu bedeuten hat. ein anderer mag seinen query vielleicht "schöner lesbar" haben und machts daher mit enum ...
 
drotzdem würde ich aus übersichtlichkeits gründen hier defintiv zu enum greifen... ist zwar geschmackssache mann kanns auch mit char oder tinyint machen. Aber ich bin ein fan von übersichtlichkeit... lieber verschwend ich 1 2 byte mehr als am ende ne spalte zu ham in der nur 1,2,3,4,5 drin steht und man erst nachschauen muss was was ist.
und man weiß auch nie was die zukunft bringt... in mysql wurden schon datentypen optimiert.
 
ActionScripter schrieb:
ja meinte ich ja ... aber ist halt eben trotzdem mehr als nur tinyint() ... nu hab dich nicht so und lass mich auch mal pingelig sein, wenn ice-breaker das schon nicht macht ;)
Zum Vergrößern anklicken....
Ich dachte schon, du hast nicht kapiert, wie der Wert gespeichert wird, drum ;)
ActionScripter schrieb:
ein anderer mag seinen query vielleicht "schöner lesbar" haben und machts daher mit enum ...
Zum Vergrößern anklicken....
Es ist Geschmackssache korrekt, allerdings ist die Gefahr hoch, dass im Script dann am Ende sowas steht:
PHP: 
if($do=="speeren")
{
  mysql_query("UPDATE user SET status=4, adminflags=49");
}
Weniger verbreitet wird wohl sowas sein:
PHP: 
if($do=="speeren")
{
  mysql_query("UPDATE user SET status=".USERSTATE_BANNED.", adminflags=".(ADMINFLAG_FAKER | ADMINFLAG_TOCHECK | ADMINFLAG_NOTIFYONLOGIN));
}
 
Weniger verbreitet wird wohl sowas sein:
PHP: 
if($do=="speeren")
{
  mysql_query("UPDATE user SET status=".USERSTATE_BANNED.", adminflags=".(ADMINFLAG_FAKER | ADMINFLAG_TOCHECK | ADMINFLAG_NOTIFYONLOGIN));
}
Zum Vergrößern anklicken....

naja die flags werden natürlich in variablen gespeichert und im query steht dann nur die variable. aber ja, so mag ich das :)
 
Du musst dich einloggen oder registrieren, um hier zu antworten.
Zurück
Oben