First PHP login system

Question

This is my first attempt at a login system! I've only had roughly 2 days of experience with MySQL and PHP so far and this is what I came up with:

<?php
    session_start();

    //Start Database
        $IP = "";
        $user = "";
        $pass = "";
        $db = "";
        $con = mysqli_connect($IP, $user, $pass, $db);

        // Check connection
        if ( mysqli_connect_errno() ) {
            echo "<div>";
            echo "Failed to connect to MySQL: " . mysqli_connect_error();
            echo "</div>";
        }

    // Pretty much kicks out a user once they revisit this page and is logged in
    if( $_SESSION["name"] )
    {
        echo "You are already logged in, ".$_SESSION['name']."! <br> I'm Loggin you out M.R ..";
        unset( $_SESSION );
        session_destroy();
        exit("");
    }

    $loggedIn = false;
    $userName = $_POST["name"] or "";
    $userPass = $_POST["pass"] or "";

    if ($userName && $userPass )
    {
        // User Entered fields
        $query = "SELECT name FROM Clients WHERE name = '$userName' AND password = '$userPass'";// AND password = $userPass";

        $result = mysqli_query( $con, $query);
        $row = mysqli_fetch_array($result);

        if(!$row){
            echo "<div>";
            echo "No existing user or wrong password.";
            echo "</div>";
        }
        else
            $loggedIn = true;
    }

    if ( !$loggedIn )
    {
        echo "
            <form action='logmein.php' method='post'>
                Name: <input type='text' name='name' value='$userName'><br>
                Password: <input type='password' name='pass' value='$userPass'><br>
                <input type='submit' value='log in'>
            </form>
        ";
    }
    else{
        echo "<div>";
        echo "You have been logged in as $userName!";
        echo "</div>";
        $_SESSION["name"] = $userName;
    }

?>

Answer 1

First up, that's a pretty good effort for a couple days of experience, you have obviously done some reading yourself.

To be honest it is, what I call spaghetti code, echo html statements in between logic. While it works fine (and is how most people learn), it becomes a pain to maintain in the long term.

Rather then straighten out the spaghetti, I have made some minor comments, so that your code it still recognizable, and you can see the changes.

I have put some inline comments describing the changes.

<?php
    // turn error reporting on, it makes life easier if you make typo in a variable name etc
    error_reporting(E_ALL);

    session_start();

    //Start Database
    $IP = "";
    $user = "";
    $pass = "";
    $db = "";
    $con = mysqli_connect($IP, $user, $pass, $db);

    // Check connection
    if (!$con) {
        echo "<div>";
        echo "Failed to connect to MySQL: " . mysqli_connect_error();
        echo "</div>";

        // *** what happens here, you let the script continue regardless of the error?
    }

    // Pretty much kicks out a user once they revisit this age and is logged in

    // *** It is best to test isset($_SESSION["name"]), otherwise php will generate a warning if 'name' index is not set.
    // you can also test for !empty($_SESSION["name"]), as empty detects if a value is not set, but it will also detect 0 as empty, so use with caution
    // if( $_SESSION["name"] )
    if( isset($_SESSION["name"]) && $_SESSION["name"] )
    {
        echo "You are already logged in, ".$_SESSION['name']."! <br> I'm Loggin you out M.R ..";
        unset( $_SESSION );
        session_destroy();

        // *** The empty quotes do nothing
        // exit("");
        exit;
    }

    $loggedIn = false;

    // *** While or is nice solution, it doesn't take into account when the 'name' index is not set, which generates a php warning
    // $userName = $_POST["name"] or "";
    $userName = isset($_POST["name"]) ? $_POST["name"] : null;

    // *** same change as above
    // $userPass = $_POST["pass"] or "";
    $userPass = isset($_POST["pass"]) ? $_POST["pass"] : null;

    // *** This test really comes down to, what if username or password is evaluated to false.
    // have a good think about what it is you are actually testing
    // php casts strings and numeric values to boolean, so something that you don't think is false could be cast as false, eg a string containing "0"
    if ($userName && $userPass )
    {
        // User Entered fields
        // *** This is dangerous, it is subject to sql injection, given you wrote this code in 2 days, i am sure you can find
        // plenty of info on sql injection and mysqli and improve it
        $query = "SELECT name FROM Clients WHERE name = '$userName' AND password = '$userPass'";// AND password = $userPass";

        $result = mysqli_query( $con, $query);

        // *** Error checking, what if !$result? eg query is broken

        $row = mysqli_fetch_array($result);

        if(!$row){
            echo "<div>";
            echo "No existing user or wrong password.";
            echo "</div>";
        }
        else {
            // *** My PERSONAL preference is to use {} every where, it just makes it easier if you add  
            // code into the condition later
            $loggedIn = true;
        }
    }

    if ( !$loggedIn )
    {
        echo "
                <form action='logmein.php' method='post'>
                    Name: <input type='text' name='name' value='$userName'><br>
                    Password: <input type='password' name='pass' value='$userPass'><br>
                    <input type='submit' value='log in'>
                </form>
            ";
    }
    else{
        echo "<div>";
        echo "You have been logged in as $userName!";
        echo "</div>";
        $_SESSION["name"] = $userName;
    }

Answer 2

At a quick look:

• Your code is vulnerable to SQL Injection: assume the user wants to hurt you, so always parse superglobals $_GET and $_POST

  https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php

• To check if variable have values:

  // good practice
  if (isset($userName, $userPass))
  // bad practice
  if ($userName && $userPass )
  

• More important:

  Don't reinvent the wheel unless you plan on learning more about wheels.

  A simple search on google for PHP login system will give you a limitless number of examples from where you can learn how to build a proper system:

http://www.phpeasystep.com/workshopview.php?id=6

Answer 3

• $userName = $_POST['name'] or ""; doesn't do what it looks like it does.

  or has a lower precedence than just about any other operator in PHP, including =. So that line is equivalent to ($userName = $_POST['name']) or "";. Basically, the or "" part has no effect. $userName will be whatever is in $_POST['name'], or null if nothing was there.

  You can't just fix it with parentheses either. or is a boolean operator, and the result will be either true or false. And in any case, you're still getting a variable that might be undefined, and will trigger notices in that case.

  Unfortunately, there's no good shorthand way to access a maybe-undefined variable without potentially triggering notices (there's @, but it's advised against for several reasons, a couple of which i agree with). You need to be a bit wordier.

  $userName = isset($_POST['name']) ? $_POST['name'] : "";
  

• Try to make your conditions positive. For example,

  if(!$row){
      echo "<div>";
      echo "No existing user or wrong password.";
      echo "</div>";
  }
  else 
      $loggedIn = true;
  

  is often less confusing as

  if($row)
      $loggedIn = true;
  else {
      echo "<div>";
      echo "No existing user or wrong password.";
      echo "</div>";
  }
  

  (By the way, bumperbox has a bit of a point about braces. I'm not as adamant about them, but i would suggest including them if the code will span multiple lines. If the code fits on the same line as the if, like this:

  if ($row) $loggedIn = true;
  else {
      echo "<div>";
      echo "No existing user or wrong password.";
      echo "</div>";
  }
  

  the intent is clear, and someone adding code should already know to add the braces as well. With the code indented on the next line the way you have it, it's easy to miss that there are no braces.)