Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }

?>
share|improve this question

3 Answers 3

up vote 6 down vote accepted

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;
    }
share|improve this answer
1  
I could not have asked for anything more! I'm going to take to the time to really take this in and study it. By the way, the only reason I had exit("") was because I was getting errors with just exit().. Also I completely forgot about the ? operator; now I can put it to some use! Time to learn about sql injection, this is something I was avoiding till the last part. On a side note, Is there a better way to approach(ehoing) my way of taking away the login form if the user is logged in? Should I just use css to remove it? –  Lemony-Andrew May 5 at 13:15
2  
it also looks like the password is stored in plain text. one should never do this. use something like blowfish to hash it and compare it with the hashed password taken from $_POST –  Joshua May 5 at 14:56
    
@Lemony-Andrew Do some work on your code. Improve the parts that are not so good, then post it again (in another question), and we will review it again. –  bumperbox May 5 at 17:28

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

    http://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

share|improve this answer
2  
I disagree with your "connection phase and call die" recommendation. What the OP is doing is good practice. Why are you suggesting they use the deprecated mysql_ extension instead of mysqli_ ? –  bumperbox May 5 at 6:12
    
Thanks! Very good suggestions. I did not know that it was bad to just check ($userName && $userPass ) and I didn't know including the data-base in the mysql connect would be bad. Also, this was just a way for me to learn more about php and mysql .. Like stated in the beginning, I only have roughly 2 days of experience with php and mysql. –  Lemony-Andrew May 5 at 6:12
4  
The isset recommendation nonsense, those two variables are guaranteed to be set, no need to check them. What you do need to check are the $_POST variables/indices. –  deceze May 5 at 6:27
1  
@Lemony Better read kunststube.net/isset –  deceze May 5 at 6:31
3  
The provided phpeasystep example is even worse! don't use it –  Pinoniq May 5 at 12:17
  • $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.)

share|improve this answer
    
I agree that it's better to just keep conditions the way they are and not reverse them with the ! operator. And I completely forgot that what I wrote $userName = $_POST['name'] or ""; was completely redundant. –  Lemony-Andrew May 5 at 13:17

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.