Was haltet ihr von meinem Login/Session Script?

[NicNac]

Hi

Dieses Script programmierte ich für meine Page. Man soll sich einloggen können und dann soll man während dem Besuch auf der Homepage eingeloggt bleiben. Funzt das so? Irgend einen logischen Fehler? Verbesserungsvorschläge?

Und noch ne Frage..ich plane den Usernamen als Index in der Tabelle zu nehmen, das Passwort aber nicht. Das ist richtig so oder? Da man ja immer nur nach dem User und DANN nach dem Passwort sucht..also macht es keinen Sinn die Password Spalte zu indexieren!?

PHP-Code:

    <!-- User: Login or already logged in --> 
    <?php 
    function LOGIN($par1, $par2) {
    $file = ("login.csv");
    if($handler = fopen($file, "r")){
    $row = fgetcsv($handler, 1024, ","); //Data is now in $row[1], [2], and [3])
    fclose($handler);
    } else {
    die("<span class=\"inhaltext\">Couldn't open the login file!</span>");
    }
    $db = mysql_connect($row[0], $row[2], $row[3]) OR die("<span class=\"inhaltext\">Couldn't connect to MySQL!</span>");
    mysql_select_db($row[3]) OR die("<span class=\"inhaltext\">Couldn't select the Database!</span>");
    //Now we are logged in and ready to compare the form data with the database entries!
    
    if($par1 != ""){
    $test = 1;
    } else {
    echo("<span class=\"inhaltext\">Please fill in a username!</span><br>");
    include("login.php");
    }
    if($par2 != ""){
    $test++;
    } else {
    echo("<span class=\"inhaltext\">Please fill in a password!</span><br>");
    include("login.php");
    }
    
    if($test == 2){
    $select_user = "SELECT username, password FROM users WHERE username = '$par1'";
    while($row = mysql_fetch_array(mysql_query($select_user))){
    if($row['username'] == "NULL"){
    echo("<span class=\inhaltext\">This username doesn't exist!</span><br>");
    include("login.php");
    } else {
    if($par2 == $row['password']){
    $loggedin = "yes";
    session_register("loggedin");
    echo("<span class=\inhaltext\">Logged in!</span><br>");
    } else {
    $loggedin = "no";
    session_register("loggedin");
    echo("<span class=\inhaltext\">That's not the correct password!</span><br>");
    include("login.php");
    }
    }
    }
    }
    }
    ?>
    <!-- The function is closed -->
    
    <?php
    if($_POST["login"] == "Login"){
    LOGIN($_POST["user"], $_POST["password"]);
    } 
    elseif($_SESSION["loggedin"] == "yes"){
    echo("<span class=\inhaltext\">Still logged in!</span><br>");
    } else {
    include("login.php");
    }
    ?>
    <!-- End Login -->

[Stuck Mojo]

Was mir sofort auffällt:

1. Total unstrukturiert programmiert. Man rückt mit Absicht Code-Bereiche ein um eine Struktur zu bewahren.
2. Riesen Sicherheitsloch, wenn man den Datenbankzugang in einer CSV-Datei abspeichert! -> http://www.deinedomain.tld/login.csv ...schon hab ich deinen Datenbankzugang
3. Die Prüfung des Benutzers und Passwortes ist viel zu kompliziert. Warum machst du das nicht direkt im Query?

Gruss
Jan

[NicNac]

Was würdest Du mir denn konkret vorschlagen? Kapier nicht ganz was du sagst, besonders im Punkt 1..und username und pw für db direkt in PHP schreiben?

[Stuck Mojo]

zu 1.: findest du dich in diesem Code zurecht? Kannst du mir auf anhieb sagen, welche Klammer welche IF-Abfrage beendet? Das meine ich damit. Mann kann/sollte seinen Code einrücken um sich darin zurecht zu finden. (gutes Beispiel: http://pear.php.net/manual/en/standards.php)

zu 2.: Stell dir die Frage: Was ist sicherer? Ein Passwort in einer PHP-Datei, von der niemand den QT sehen kann, es sei denn aus irgendwelchen Gründen schmiert der PHP-Parser ab? Oder in einer Text-Datei, die JEDER zu jeder Zeit herunterladen kann?

Gruss
Jan

[NicNac]

Hm alles klar, ja das mit der CSV-Datei wusste ich nicht. In meinem Buch wurde das erwähnt, da dachte ich es sei gut. Ok aber sonst ist der Code ok? Also ich strukturiere ihn noch aber sonst?

Und momentan bin ich noch an nem Upload SCript...könnt ich dich dafür vielleicht auch noch später mal für Deine Bewertung kontaktieren?

[Stuck Mojo]

Original geschrieben von NicNac
Ok aber sonst ist der Code ok?

...Geschmackssache... Lange machst du das noch nicht, das sieht man
Abfragen auf != "" find ich ziemlich überflüssig, da man diese schon mit einem Leerzeichen "austricksen" kann... Aber das kommt mit der Zeit.

Naja und wie gesagt: Die Gültigkeitsprüfung ist zu kompliziert. Frag einfach den User ab, der das übergebene PW und den übergebenen Namen hat... Wenns ein Result gibt, haste deinen User. Wenn nicht, dann nicht...

Dann sollte man ja JA/NEIN-Variablen auch eine 1 (oder true) und eine 0 (oder false) verwenden, anstelle von "yes" und "no". Damit kannst du dann ganz leicht abfragen if($_SESSION["loggedin"]) oder if(!$_SESSION["loggedin"]).

Desweiteren würde ich keine includes oder echo-Ausgaben im Login vorgang machen, sondern mit Return-Werten arbeiten. Aber wie gesagt... das bekommt man mit der Zeit mit

Gruss
Jan

[NicNac]

Hi

Habe das ganze jetzt bisschen geändert. Die Abfrage ist (nach Meinung von Stuck Mojo sicher immer noch zu kompliziert, aber das mit den return Anweisungen habe ich geändert. Aber irgendwie funzt das ganze noch nicht so wie es sollte. Why? Hab jetzt sicher 1 Stunde den Fehler gesucht.

PHP-Code:

    <?php 
    function LOGIN($par1, $par2) {
      $db = mysql_connect("localhost", "root", "") OR die("<span class=\"inhaltext\">Couldn't connect to MySQL!</span>");
      mysql_select_db("test") OR die("<span class=\"inhaltext\">Couldn't select the Database!</span>");
      //Now we are logged in and ready to compare the form data with the database entries!
    
      if(strlen($par1) != 0){
        $test = 1;
      } else {
        $log = 2;
        return $log;
      }
      if(strlen($par2) != 0){
        $test++;
      } else {
        $log = 3;
        return $log;
      }
    
      if($test == 2){
      $select_user = "SELECT username, password FROM user WHERE username = '$par1'";
      $result = mysql_query($select_user);
        if(mysql_num_rows($result) == 0){
            $log = 4;
            return $log;
        } else {
            while($row = mysql_fetch_array($result)){
               if($par2 == $row["password"]){
                 $loggedin = 1;
                 session_register("loggedin");
                 $log = 1;
                 return $log;
               } else {
                  $log = 5;
                  return $log;
            }
          }
        }
      }
    }
    ?>
    <!-- The function is closed -->
    
    <?php
    if(@$_POST["login"] == "Login"){
      LOGIN($_POST["username"], $_POST["password"]);
      switch(@$log) {
      case 1:
       echo("<span class=\inhaltext\">Logged in!</span><br>");
       break;
      case 2: 
       echo("<span class=\"inhaltext\">Please fill in a username!</span><br>");
       include("login.php");
       break;
      case 3:
       echo("<span class=\"inhaltext\">Please fill in a password!</span><br>");
       include("login.php");
       break;
      case 4:  
       echo("<span class=\inhaltext\">This username doesn't exist!</span><br>");
       include("login.php");
       break;
      case 5:
       echo("<span class=\inhaltext\">That's not the correct password!</span><br>");
       include("login.php");
       break;
      } 
      } elseif(@$_SESSION["loggedin"]){
      echo("<span class=\inhaltext\">Still logged in!</span><br>");
      } else {
      include("login.php");
    }
    ?>

[Radoo.de]

.

[NicNac]

Sorry dass ich nicht mehr geantwortet habe, war eben 1 Woche in den Ferien.

Also es klappt (eigentlich) schon, denke ich zumindest, denn Fehlermeldungen kommen keine. Was allerdings nicht klappt ist vermutlich der Switch-Abschnitt. Denn ausgegeben wird gar nichts.

[anrebe]

Hallo.

Lösch mal die @ Zeichen im switch-abschnitt raus und probier das noch mal aus. Jetzt müsste eigentlich eine detailiertere Fehlermeldung kommen.