2
\$\begingroup\$

In your opinion, do I need to do all these validations? I thought to do well and stay safe but it seriously raises the question if it isn't slower.

jQuery

$(document).ready(function () {

        // jQuery validate plugin
        $('#login > form').validate({

            rules: {
                username: { required: true },
                password: { required: true }
            },

            submitHandler: function () {

                var credentials = {
                    "username": $('#username').val(),
                    "password": $('#password').val()
                };

                $.post('ajax/tryLogin.php', credentials)
                    .done(function (data) {

                        if (data.hasOwnProperty('success') &&
                            data['success']) {

                            window.location = 'catalog.php';

                        } else if (data.hasOwnProperty('message')) {

                            alert(data['message']);
                        } else {
                            alert('Communication with the server failed.');
                        }
                    })
                    .fail(function () {
                        alert('Communication with the server failed.');
                    })
            }
        });
    });

Ajax PHP

if (empty($_POST['username']) || empty($_POST['password'])) {
    $data['success'] = false;
    $data['message'] = 'Username and password are required.';

} else {

    try {
        $data['success'] = Security::TryLogin($_POST['username'], $_POST['password']);
    } catch (Exception $e) {

        $data['success'] = false;
        $data['message'] = $e->getMessage();
    }
}
header('Content-type: application/json');
echo json_encode($data);

PHP

class Security 
{

    //...

    public static function TryLogin($username, $password)
    {
        $username = strtolower($username);
        $password = sha1($password . $username);

        try {
            $user = Users::FindByUsernameAndPassword($username, $password);

            if (session_id() == '') {
                session_start();
            }

            $_SESSION[self::USER_IDENTIFIER] = $user;

            return true;

        } catch (Exception $e) {
            throw new Exception('Username of password incorrect.');
        }
    }

    //...

}
\$\endgroup\$
  • \$\begingroup\$ Good question! :) I have a few comments. No, a couple of if won't slow your code down. Yes, it helps to catch errors early, especially to give feedback to your users. And no, alert() is not a nice way to give feedback. \$\endgroup\$ – Quentin Pradet May 7 '13 at 8:10
  • \$\begingroup\$ @QuentinPradet What alternative do you propose to alerts? \$\endgroup\$ – hlapointe May 18 '13 at 17:03
  • 1
    \$\begingroup\$ One example: github.com/stanlemon/jGrowl \$\endgroup\$ – Quentin Pradet May 19 '13 at 9:47
2
\$\begingroup\$

Since throw interrupts the program flow, you don't need to provide an else branch. That way, you also visibly follow the good practice of using 'watchdogs':

<?php

include_once('config.php');
include_once(ROOT . 'libs/database.php');
include_once(ROOT . 'libs/models/user.php');

class Security
{

    //...

    public static function TryLogin($username, $password)
    {
        $username = strtolower($username);
        $password = sha1($password . $username);

        $conn = Database::getConnection();
        if (empty($conn)) {
            throw new Exception('The connection to the database failed.');
        }

        $result = odbc_exec($conn, '{CALL [BruPartsOrderDb].[dbo].[tryLogin]("' . $username . '", "' . $password . '")}');
        if (empty($result)) {
            throw new Exception('The execution of the query failed.');
        }

        $row = odbc_fetch_row($result);
        if (empty($row)) {
            throw new Exception('Username or password incorrect.');
        }

        if (session_id() == '') {
            session_start();
        }

        $_SESSION['user'] = new User(odbc_result($result, 'id'), $username);

        return true;
    }

    //...
}

Suddenly the code looks much more handy.

\$\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.