LDAP Login Script

This is my first time ever having code peer reviewed! This is just the main function I'm concerned about.

Edit

Apologies for the initial vagueness of my question, this is the "login" part of my login script the classes is initial instantiated on the index.php which handles a $_POST request from login.php I wont paste them here because they are generic input fields.

The class construct looks something like (Removed the not so relevant parts)

public function __construct($DB_con)
{
    // create/read session, absolutely necessary
    session_start();

    //New method of database connection
    $this->db = $DB_con;

    // check the possible login actions:
    // if user tried to log out (happen when user clicks logout button)
    if (isset($_GET["logout"])) {
        $this->doLogout();
    }
    // login via post data (if user just submitted a login form)
    elseif (isset($_POST["login"])) {
        $this->dologinWithPostData();
    }
}

The logic of logging in does as follows;

```
private function dologinWithPostData()
{

if (empty($_POST['user_name'])) {
$this->errors[] = "Username field was empty.";
} elseif (empty($_POST['user_password'])) {
$this->errors[] = "Password field was empty.";
} elseif (!empty($_POST['user_name']) && !empty($_POST['user_password'])) {
//ldap_set_option(NULL, LDAP_OPT_DEBUG_LEVEL, 7);

$username = $_POST['user_name'];
$password = $_POST['user_password'];
$adServer = "ldaps://The Address";

$ldap = ldap_connect($adServer);

$ldaprdn = 'MYDN.net' . "\\" . $username;

ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, 3);
ldap_set_option($ldap, LDAP_OPT_REFERRALS, 0);

$bind = @ldap_bind($ldap, $ldaprdn, $password);

if ($bind) {

// Get user info by there account login
$filter="(sAMAccountName=$username)";

Logical inconsistencies and careless mistakes

Let's look at the outline:

if (empty($_POST['user_name'])) {
    $this->errors[] = "Username field was empty.";
} elseif (empty($_POST['user_password'])) {
    $this->errors[] = "Password field was empty.";
} elseif (!empty($_POST['username']) && !empty($_POST['user_password'])) {
    …
    $ldap = ldap_connect(…);
    …
} else {
@ldap_close($ldap);
echo "Wrong password. Try again.";
}

The third condition, if it ever reaches there, is always true. The else clause at the end is, therefore, never reachable. Good thing, too, because $ldap would not be defined if that dead code were ever executed.

Also, stare at these few lines for a while and tell me what is going on:

if (strpos($dn, 'OU=MYOU') !== false) {
    $_SESSION['userType'] = 2;
}elseif(strpos($dn, 'OU=MYOU') !== false){
    $_SESSION['userType'] = 1;
}

This method name is misspelled:

if($this->checkUserProfileExsists() == false){ … }

… and in any case it would be better named userProfileExists().

You called @ldap_close($ldap) too many times:

@ldap_close($ldap);

$fullName = $info[0]["displayname"][0];
$dn = $info[0]["dn"];
$groups = array();

if (array_key_exists("memberof", $info[0])){
    …
}

@ldap_close($ldap);

Suppressing errors using @ is OK with ldap_close(), since you don't really care much if it fails. But you shouldn't do it anywhere else.

Organization

This function never returns a value. It would be nice if it returned true if the login was successful and false if it wasn't.

In this code, and most code in general, there is only one way to succeed, and many ways to fail. Therefore, the idiom should be:

if (failure condition 1) {
    cleanup 1;
    return false;
}
if (failure condition 2) {
    cleanup 2;
    return false;
}
if (failure condition 3) {
    cleanup 3;
    return false;
}
run some infallible code;
return true;

The code would flow better mentally if you rearranged a few statements, such that there are "stanzas". Constructing the $groups array breaks up the flow; I would move that into a helper method. See the suggested implementation below.

LDAP

ldap_sort() is deprecated. In any case, I don't understand why you would need to sort the search results by surname. Shouldn't the "(sAMAccountName=$username)" filter uniquely identify an account? If not, then your directory is a horrible mess.

By the way, be careful when doing string interpolation like that without performing escaping. An attacker might try to confuse your code by posting user_name=*. In this case, you are probably fine, since you only do the search after successfully binding using the given username.

You refer to $info[0] a lot. It's probably worthwhile to reassign the variable to discard the rest of the array.

Suggested implementation

private function doLoginWithPostData() {
    // Input
    $username = $_POST['username'];
    $password = $_POST['username'];

    // Configuration
    $adServer = 'ldaps://The Address';
    $baseDN = "dc=MYDC,dc=NET";
    $ldapRDN = "MYDN.net\\$username";

    // Validate
    if (empty($username) || empty($password)) {
        if (empty($username)) { $this->errors[] = "Username field was empty."; }
        if (empty($password)) { $this->errors[] = "Password field was empty."; }
        return false;
    }

    // Connect
    if (!($ldap = ldap_connect($adServer))) {
        $this->errors[] = "Could not connect to authentication server.";
        return false;
    }
    ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, 3);
    ldap_set_option($ldap, LDAP_OPT_REFERRALS, 0);

    // Bind
    if (!($bind = ldap_bind($ldap, $ldapRDN, $password))) {
        $this->errors[] = "Wrong username or password";
        @ldap_close($ldap);
        return false;
    }

    // Search
    $result = ldap_search($ldap, $baseDN, "(sAMAccountName=$username)");
    $info = ldap_get_entries($ldap, $result);
    $info = $info[0];
    @ldap_close($ldap);

    // Session
    $_SESSION['userID'] = bin2hex($info["objectguid"][0]);
    $_SESSION['fullName'] = $info["displayname"][0];
    $_SESSION['loggedOnStatus'] = "1";
    $_SESSION['userGroups'] = $groups = $this->userGroups($info["memberof"]);
    $_SESSION['userLevel'] = in_array('Dept_Directors', $groups) ? 10 :
                            (in_array('Dept_Admin',     $groups) ?  7
                                                                 :  0);
    if (strpos($info["dn"], 'OU=MYOU') !== false) {
        $_SESSION['userType'] = 2;
    }

    if (!($this->userProfileExists()) && !($this->createUserProfile())) {
        // Succeed with a warning
        $this->errors[] = "Error creating user profile";
    }
    return true;
}

StackExchange Code Review Q#134079, answer score: 7