1
\$\begingroup\$

This code basically connects to a database, sets login success and failure pages, queries the database for user details, checks if user is active, sets session value and redirects accordingly.

Can you have a look? What do you think of it? Any suggestions?

<?php

session_start();

// Connect to the database
try {
    $db = new PDO('mysql:host=localhost; dbname=database', 'username', 'password');
    $db->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);
    $db->exec("SET NAMES 'utf8'");
} catch(Exception $e) {
    exit;
}

// Set URL for successful login and unsuccessful login
if($_POST['page']) {
    $success = $_POST['page'];
} else {
    $success = 'http://website.com/members';
}
$fail = 'http://website.com/login';

// Check if login came from my server
if($_SERVER['SERVER_NAME'] != 'website.com') {
    header('Location: ' . $fail);
}

// Check if a user is trying to login
if($_POST) {

    // Query the users details
    try {
        $user_query = $db->prepare('SELECT * FROM users LEFT JOIN zones ON user_timezone = zone_id WHERE user_email = ? AND user_pass = ?');
        $user_query->bindParam(1, $_POST['username'], PDO::PARAM_STR, 50);
        $user_query->bindParam(2, $_POST['password'], PDO::PARAM_STR, 15);
        $user = $user_query->execute();
        $user = $user_query->fetch(PDO::FETCH_ASSOC);
    } catch(Exception $e) {
        exit;
    }

    // Make sure account is active
    if($user['user_active'] != 1) {
        header('Location: ' . $fail . '?error=2');
        exit;
    }

    // Make sure user exists
    if($user != FALSE) {
        $_SESSION['uid'] = $user['user_id'];
        $_SESSION['utz'] = $user['zone_name'];
        header('Location: ' . $success);
    } else {
        header('Location: ' . $fail . '?error=1');
        exit;
    }
} else {
    header('Location: ' . $fail . '?error=4');
    exit;
}

?>
\$\endgroup\$
3
  • 1
    \$\begingroup\$ You mean improve a piece of code right? Or you're a code jazzman? \$\endgroup\$ Nov 27, 2013 at 20:29
  • \$\begingroup\$ where did you get this code at? \$\endgroup\$
    – Malachi
    Nov 27, 2013 at 20:40
  • 1
    \$\begingroup\$ Are you trying to improve someone else's code or this is the code which you implemented ? \$\endgroup\$
    – Ravi Kumar
    Dec 1, 2013 at 18:42

1 Answer 1

2
\$\begingroup\$

Just few quick things come to my mind:

  • The way try/catch is used is to exit on error, which would happen anyway. Maybe you need to send some info to the user here instead.

  • Are you passing raw user input to the database, without validation? This would be clear security issue.

  • Why not if($user) instead of if($user != FALSE)?

  • What is the purpose to set headers and then exit without any request?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Prepared statements mean sql injection is already accounted for. Setting location headers and exiting is the normal way to redirect a user. Wrapping the header-and-exit lines in a function is a simple example how the code could be improved. \$\endgroup\$
    – AD7six
    Dec 1, 2013 at 20:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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