3
\$\begingroup\$
<?php
$page = 'Login';
session_start();
include 'header.php';
$user_error = '';
$pass_error = '';
$login_error = '';

if(isset($_SESSION['username'])){
header('location: control-painel.php'); 
}
if(isset($_POST['login'])){
$username = $_POST['username'];
$password = $_POST['password'];
$cost = '11';
$salt = 'Cf1f11ePArKlBJomM0F6aJ';
$password_hash = crypt($password, '$2a$' . $cost . '$' . $salt . '$');
$id = 0;

if(empty($username)){
    $user_error = 'Please insert a username';
}
if(empty($password)){
    $pass_error = 'Please insert a password';
}
elseif(!empty($username) && !empty($password)){
    $stmt = $mysqli -> prepare('SELECT id FROM user WHERE username = ? AND password = ?');
    $stmt -> bind_param('ss', $username, $password_hash);
    $stmt -> execute();
    $stmt -> bind_result($id);
    $stmt -> fetch();

    if($id){
        $_SESSION['username'] = $username;
        $_SESSION['email'] = $email;
        header('location: control-painel.php');
        die();
    }
    else{
        $login_error = 'User or Password invalid';
    }        
}
}
?>

<div class="message">
       <br><br>
    <?php echo $user_error; ?><br><br>
    <?php echo $pass_error; ?><br><br>
    <?php echo $login_error; ?><br><br>
    <br><br>
</div>
<div id="form" class="bradius">
    <div class="content">
        <form method="post">
            <label>Username: </label>
            <input type="text" name="username" class="text bradius">
            <label>Password: </label>
            <input type="password" name="password" class="text bradius">
            <input type="submit" class="submitbutton bradius" name="login" value="Login">
        </form>
    </div>
</div>

<?php
include "footer.php";
?>
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Not strictly an answer but it could do some accurate indenting! \$\endgroup\$ – dougajmcdonald May 28 '14 at 19:22
3
\$\begingroup\$

The first thing I spotted was this: control-painel.php. I'm not sure if that's an error with your page name or code, but whichever it is needs to be fixed.

Can it be optimized?

The best optimisation is only doing what's necessary. When checking the login, you don't check whether they've even been specified until after you hash it, consider reordering it like so:

$username = $_POST['username'];
$password = $_POST['password'];
if(empty($username))
{
    $user_error = 'Please insert a username';
}
if(empty($password))
{
    $pass_error = 'Please insert a password';
}
elseif(!$user_error) //Password can't be empty here, don't need to check again
{
    $cost = '11';
    $salt = 'Cf1f11ePArKlBJomM0F6aJ';
    $password_hash = crypt($password, '$2a$' . $cost . '$' . $salt . '$');
    $id = 0;
    $stmt = $mysqli->prepare('SELECT id FROM user WHERE username = ? AND password = ?');
    $stmt->bind_param('ss', $username, $password_hash);
    $stmt->execute();
    $stmt->bind_result($id);
    $stmt->fetch();
    if($id)
    {
        $_SESSION['username'] = $username;
        $_SESSION['email'] = $email;
        header('location: control-painel.php');
        die();
    }
    else
    {
        $login_error = 'User or Password invalid';
    }
}

So the hashing is only performed when you have a username and password.

\$\endgroup\$
1
\$\begingroup\$

Not really related to how secure your login page is, but more on the displaying of the error message. The current code can end up looking like so:

<div class="message">
    <br><br>
    <br><br>
    <br><br>
    User or Password invalid<br><br>
    <br><br>
</div>

The excess spacing will appear inconsistent if the user keeps on having different errors everytime. You can opt to have a single variable for the error messages, separated by line breaks:

<?php
    $error_message = "";
    if(empty($username)){
        $error_message .= 'Please insert a username<br><br>';
    }
    if(empty($password)){
        $error_message .= 'Please insert a password<br><br>';
    }
    elseif(!empty($username) && !empty($password)){
        /*error in login*/
        $error_message .= 'User or Password invalid<br><br>';
    }
?>
<div class="message"><?php echo $error_message; ?></div>

This lessens the excessive <br> on the <div class="message"></div> part. But to handle the line breaks better, you can use an array instead:

<?php
    $error_message = array();
    if(empty($username)){
        $error_message[] = 'Please insert a username';
    }
    if(empty($password)){
        $error_message[]= 'Please insert a password';
    }
    elseif(!empty($username) && !empty($password)){
        /*error in login*/
        $error_message[]= 'User or Password invalid';
    }

    $error_message = implode("<br><br>", $error_message); //turns the $error_message into a string, with line breaks as separators
?>

However, relying on line breaks for spacing/padding purposes is generally a bad practice. You can use CSS for that purpose, and perhaps use a single error message every time.

I hope this is still helpful. Let me know if you have questions!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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