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)";
$result = ldap_search($ldap,"dc=MYDC,dc=NET",$filter);
ldap_sort($ldap,$result,"sn");
$info = ldap_get_entries($ldap, $result);
@ldap_close($ldap);
$fullName = $info[0]["displayname"][0];
$dn = $info[0]["dn"];
$groups = array();
if (array_key_exists("memberof", $info[0])){
$x = $info[0]["memberof"];
$groups = array();
foreach ($x as $key){
if(strlen($key)>1){
array_push($groups, $this->get_string_between($key, "CN=", ",") );
}
}
}
@ldap_close($ldap);
$_SESSION['userID'] = bin2hex($info[0]["objectguid"][0]);
$_SESSION['fullName'] = $fullName;
$_SESSION['userGroups'] = $groups;
$_SESSION['loggedOnStatus'] = "1";
if(in_array('Dept_Directors', $groups)){
$_SESSION['userLevel'] = 10;
}elseif(in_array('Dept_Admin', $groups)){
$_SESSION['userLevel'] = 7;
}else{
$_SESSION['userLevel'] = 0;
}
if (strpos($dn, 'OU=MYOU') !== false) {
$_SESSION['userType'] = 2;
}elseif(strpos($dn, 'OU=MYOU') !== false){
$_SESSION['userType'] = 1;
}
if($this->checkUserProfileExsists() == false){
if(!($this->createUserProfile())){
$msg="Error Creating User Profile ";
echo $msg;
}
}
} else {
@ldap_close($ldap);
$this->errors[] = "Wrong Username or Password";
}
} else {
@ldap_close($ldap);
echo "Must Enter UserName & Password Try again.";
}
}
Now the code does fire of to some other functions that check the database to see if the user's ID (GUID object in AD) has been updated (I'm not a great AD administrator sometimes you gota to what you gota do), or if its a new user who gets authenticated with LDAP and just needs a profile setting up.
My main concern is that script isn't secure, I use HTTPS as standard. But apart from the fact there is no effort made to sanitize the user input (LDAP injection?) is there anything "wrong" or insecure about my code? It is partially based of the The PHP Login Project