2
\$\begingroup\$

I developed this group of scripts to validate users againts a LDAP server and presenting the appropiate message to the user after sending the information.

I wonder if I'm handling everything properly according to best practices. I'll appreciate any advice on how to improve the code in order for it to be easier to maintain in the future.

LDAP connection script:

    <?php    
session_start();
unset($_SESSION["error"]);

$ldaprdn = 'mydomain\\' . trim($_POST["user"]); // will always be set coming from login form.
$ldappass = $_POST["pass"];
$ldapserver = 'aest-dc4.xxxxxxx.xxx.xxxxxxxx.xxx.xxx';

$ldapconn = ldap_connect($ldapserver);

if ($ldapconn) {
    $ldapbind = ldap_bind($ldapconn, $ldaprdn, $ldappass);
    $_SESSION["logged-in"] = $ldapbind;

    if (!$ldapbind) {
        $_SESSION["error"] = 2;
    } else {
        $_SESSION["error"] = 3;
    }
} else {
    $_SESSION["error"] = 4;
}

header("Location: http://" . $_SERVER["HTTP_HOST"] . "?/php/views/portal.php");

Login.php

<div id="form-container">
    <form action="/php/scripts/ldap_connect.php" onsubmit="return validateLoginForm(this);" method="POST" class="form-login">
        <h1>Login</h1>
        <label for="user">Usuario</label>
        <input type="text" name="user" id="user">
        <label for="password">Password</label>
        <input type="password" name="pass" id="pass">
        <button type="text" class="form-login-submit" name="form-login-submit">Login</button>
    </form>
</div>
<script>
    function validateLoginForm(form) {
        let input = [form["user"], form["pass"]];
        let $submit = true;

        input.forEach(element => {
            if (element.value.length === 0) {
                if (!element.className.includes("input-error")) {
                    element.className += " input-error";
                    element.className = element.className.trim();
                }

                element.addEventListener("keyup", function() {
                    element.className = element.className.replace("input-error", "");

                    if (element.className.length === 0)
                        element.removeAttribute("class");
                });

                $submit = false;
            }
        });

        return $submit;
    }
</script>

error-codes.php

<?php
$error_codes = [
    0 => "!Usuario incorrecto.",
    1 => "!Password incorrecta.",
    2 => "!Credenciales inválidas.",
    3 => "Logueado correctamente.",
    4 => "!No se pudo conectar al servidor LDAP para validar las credenciales."
];

/**
 * Looks up error in error code table and returns the appropriate message
 *
 * @param [int] $error the id of the error.
 * @return array(string, bool) the error message and true if it is an error or false if it a success.
 */
function getError($error)
{
    global $error_codes;

    foreach ($error_codes as $key => $value) {
        if ($key === $error) {
            $is_error = $value[0] === '!';
            $msg = $is_error ? substr($value, 1) : $value;
            return array($msg, $is_error);
        }
    }

    return array("Error desconocido", true);
}

Where I show the error:

<?php
session_start();
$link = isset($_GET["link"]) ? $_GET["link"] : "php/views/portal.php";
include("php/error-codes.php");
?>
<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta http-equiv="X-UA-Compatible" content="ie=edge">
    <link rel="stylesheet" href="/css/normalize.css">
    <link rel="stylesheet" href="/css/style.css">
    <link rel="icon" href="favicon.png">
    <script src="js/php_file_tree.js" type="text/javascript"></script>
    <script src="js/jquery.js"></script>
    <title>TecoDB</title>
</head>

<body>
    <div class="app">
        <?php
        if (isset($_SESSION["error"])) :
            $error = getError($_SESSION["error"]);
            $type = $error[1] ? "error" : "success";
            unset($_SESSION["error"]);
            echo ("<span class=\"error-msg $type\">" . $error[0] . "</span>");
        endif; ?>
        <?php include "php/views/header.php" ?>
        <div id="search-box">
            <section id="search-breadcrumbs">
                <?php
                if (substr($link, 0, 5) === 'docs/') {
                    // if it's not a doc, don't show path
                    echo "<p id=\"search-breadcrumbs-result\">$link</p>";
                }
                ?>
            </section>
\$\endgroup\$
  • 1
    \$\begingroup\$ Your javascript doesn't look Hungarian. Is there a significance to the $ on the $submit variable? What business does a correct login have being in the list of error codes? \$\endgroup\$ – mickmackusa Jun 13 at 11:10
4
\$\begingroup\$

My attention is really drawn to your error-codes script. I love a good lookup array, and you almost have one, but it needs some polishing.

  • Rather than declaring the lookup variable with global to put it in the function's scope, realize that you probably aren't going to be modifying the data during processing, so it is a perfect candidate for being a constant. A constant does not suffer scoping issues -- it is globally available.
  • Construct your lookup array to instantly deliver exactly what you need. Don't bother with performing string manipulations to the first character. Spend the few more characters in the array and just name boolean values in the second element of each subarray in the lookup.
  • error_codes is a strange name for data that contains both positive and negative responses. I wouldn't be saving any positive messages, but if you truly need to store the positive message, then it would be better to rename the file, the lookup, and the custom function.
  • The beauty of writing a lookup array with identifiable keys is that you can leverage a key check (isset() or similar) which will always perform better than an iterative process (even with an early break/return). This is purely a matter of how php arrays are designed and processed. See my snippet for how simple the function call can be after the lookup array is sweetened up.

Code: (Demo)

const response_codes = [
    ["Usuario incorrecto.", false],
    ["Password incorrecta.", false],
    ["Credenciales inválidas.", false],
    ["Logueado correctamente.", true],
    ["No se pudo conectar al servidor LDAP para validar las credenciales.", false]
];

function interpretResponse($id)
{
    return response_codes[$id] ?? ["Error desconocido", false];
}

With null coalescing operator in play, it's almost not worth declaring the custom function.

Beyond that...

  • I don't see the point in writing unset($_SESSION["error"]); near the top of your LDAP connection script because no matter which outcome is reached, the value will be overwritten.

  • When you want to check if a string begins with specific characters use:

    if (strpos($link, 'docs/') === 0) {
    

    instead of

    if (substr($link, 0, 5) === 'docs/') {
    

    because you don't need the string, you only need to check the location. It's not going to be a major performance booster, but I consider it to be more deliberate in coding intent. I have seen other cases where the substring is a bit longer (say 20 or more characters) and the developer would have to go to the trouble to count the characters each time the substring was changed -- increasing the risk of human error. strpos() -- when finding a substring from the start of the string (or near the start) spares that irritation.

  • You might also consider the slight shorting of conditional syntax from variable.length === 0 to !variable.length. They are equivalent.

  • Finally, you might like to check out this handy javascript feature: https://stackoverflow.com/a/14101453/2943403 element.classList.add("input-error"); This will spare you having the concat and trim stuff as you go.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for your time. I learned a lot! \$\endgroup\$ – Jh62 Jun 18 at 6:35

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.