I've commented through your existing code below and noted what was removed/replaced/added (my comments are denoted by double #
's):
## Let's modify the class name 'adUser' to something more
## informative, like 'ActiveDirectoryUser'
class ActiveDirectoryUser {
private $connection;
## The use of an underscore is a leftover from a past convention
## it is ok to not have an underscore on private properties
private $username;
private $password;
private $ldap_db = "censored";
private $ldap_connection; ## We moved the connection to here for state\
/**
* Always have documentation
* @param $username string
* @param $password string
*/
public function __construct($username, $password) {
$this->username = $username;
$this->password = $password;
}
/**
* Always have documentation
*/
public function __destruct(){
ldap_close($this->ldap_connection);
}
/**
* Always have documentation
* @param $username string
* @param $password string
*/
## We'll rename connectAD to connect - it should be simple.
public function connect() {
## Get rid of this comment - it doesn't tell us anything informative
# Connect to Domain Controller
$this->ldap_connection = ldap_connect($this->ldap_db); ## Replaced the string with a property for reuse
## Again with the uninformative comment - it
## doesn't tell us anything that the code already does
# Case / When to see if user and password match
if ($bind = ldap_bind($this->ldap_connection, $this->username, $this->password)) {
## We'll return true if it connects, otherwise throw an exception.
## Our connect() function shouldn't ever return a username or data
return True
# return $this->username;
# Your bracketing in else is weird and unconventional
} else {
# todo: Failure
#echo "Invalid login.";
# Don't handle your redirect logic in your class - do so at call level
# Instead, throw an exception or return False
#header('Location: index.php?eid=1');
throw new Exception("Invalid login.");
}
## Why are we closing the connection in the connect function? Move this to the destruct()
# Close connection
#ldap_close($ldap);
}
/**
* Always have documentation
*/
## Renamed from getFullName to getName
public function getName(){
# Initialize the connection
## No need to call LDAP connection again since we already have the connection stored in a property
# $ldap = ldap_connect($this->ldap_db);
ldap_set_option($this->ldap_connection, LDAP_OPT_PROTOCOL_VERSION, 3);
ldap_set_option($this->ldap_connection, LDAP_OPT_REFERRALS, 0);
# Parameters bound
$bind = ldap_bind($this->ldap_connection, $this->username, $this->password);
# Will trim domain and slash from username
$person = substr($this->username,strpos($this->username,"\\")+1,strlen($this->username) - strpos($this->username,"\\"));
# Set search criteria (search on username -> return cn aka full name)
$filter="(sAMAccountName=".$person.")";
## justthese can be moved to the ldap_search function since its inline and only called once
## additionally, we can use the shorthand [] array initiator
#$justthese = array("cn");
$sr = ldap_search($this->ldap_connection, $this->ldap_db, $filter, ['cn']);
$info = ldap_get_entries($ldap, $sr);
# Loop through results - although we only have one entry at this time.
## We'll have a returnData array to return rather than echo out.
$returnData = [];
for ($i=0; $i<$info["count"]; $i++) {
//echo "dn is: ". $data[$i]["dn"] ."<br />";
$returnData[] = $info[$i]["cn"][0];
}
## Don't need to close it as it's closed on class destruct
# Close the connection
#ldap_close($ldap);
return $returnData;
}
}
and here is the same, with my comments removed:
/**
* Always have documentation here
*/
class ActiveDirectoryUser {
private $connection;
private $username;
private $password;
private $ldap_db = "censored";
private $ldap_connection;
/**
* Always have documentation
* @param $username string
* @param $password string
*/
public function __construct($username, $password) {
$this->username = $username;
$this->password = $password;
}
/**
* Always have documentation
*/
public function __destruct(){
ldap_close($this->ldap_connection);
}
/**
* Always have documentation
* @param $username string
* @param $password string
*/
public function connect() {
$this->ldap_connection = ldap_connect($this->ldap_db);
if ($bind = ldap_bind($this->ldap_connection, $this->username, $this->password)) {
return True
} else {
throw new Exception("Invalid login.");
}
}
/**
* Always have documentation
*/
public function getName(){
ldap_set_option($this->ldap_connection, LDAP_OPT_PROTOCOL_VERSION, 3);
ldap_set_option($this->ldap_connection, LDAP_OPT_REFERRALS, 0);
$bind = ldap_bind($this->ldap_connection, $this->username, $this->password);
# Will trim domain and slash from username
$person = substr($this->username, strpos($this->username, "\\") + 1,
strlen($this->username) - strpos($this->username,"\\"));
$filter="(sAMAccountName=" . $person.")";
$sr = ldap_search($this->ldap_connection, $this->ldap_db, $filter, ['cn']);
$info = ldap_get_entries($ldap, $sr);
$returnData = [];
for ($i = 0; $i < $info["count"]; $i++) {
$returnData[] = $info[$i]["cn"][0];
}
return $returnData;
}
}
And finally, the usage (note we have to try/catch for Exception):
include_once("functions/LDAP.php");
$ActiveDirectoryUser = new ActiveDirectoryUser($_POST['username'], $_POST['password']);
try{
$ActiveDirectoryUser->connect();
} catch (Exception $e){
# Do header redirect here
}
$user = $ActiveDirectoryUser->getName();