3
\$\begingroup\$

I have this class, which helps me to connect to LDAP:

    class adUser {

        private $_username;
        private $_password;

        public function __construct($username, $password)
        { 
          $this->_username = $username;
          $this->_password = $password;
        }

        public function connectAD() 
        {

            # Connect to Domain Controller
            $ldap = ldap_connect("censored");

            # Case / When to see if user and password match
            if ($bind = ldap_bind($ldap, $this->_username, $this->_password)) 
                {
                  # todo: Successful
                  return $this->_username;
                } 
            else 
                {
                  # todo: Failure
                  echo "Invalid login.";
                  header('Location: index.php?eid=1');
                }

            # Close connection
            ldap_close($ldap);
        }

        public function getFullName()
        {

            # Initialize the connection
            $ldap = ldap_connect("censored");
            ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, 3);
            ldap_set_option($ldap, LDAP_OPT_REFERRALS, 0);

            # Parameters bound
            $dn = "censored";
            $bind = ldap_bind($ldap, $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 = array("cn");
            $sr=ldap_search($ldap, $dn, $filter, $justthese);
            $info = ldap_get_entries($ldap, $sr);

            # Loop through results - although we only have one entry at this time.
            for ($i=0; $i<$info["count"]; $i++) {
                        //echo "dn is: ". $data[$i]["dn"] ."<br />";
                         echo $info[$i]["cn"][0];
                    }

            # Close the connection
            ldap_close($ldap);
        }
    }

It's basically the second time I've tried OOP PHP - is there any way where I could significantly compress this class? Did I get this whole OOP thing right?

This is how I call the functions:

include_once("functions/LDAP.php");

$curUser = new adUser($_POST['username'], $_POST['password']);

echo "logged in as: ".$curUser->connectAD();
echo "<br>";

$curUser->getFullName();
\$\endgroup\$
4
\$\begingroup\$

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();
\$\endgroup\$
  • \$\begingroup\$ Wow, thanks, that's quite a new level for me. I've just went it through, will spend some time on it to better understand. One question though: "always have documentation" what should be put in there? A basic description of the function, and what the arguments are for? \$\endgroup\$ – Alex Szabó Sep 18 '14 at 16:14
  • 1
    \$\begingroup\$ A basic description of the function, as well as any parameters it takes, and what it returns at the very least. Take a look at my PHPBenchTime library to see comments: github.com/jsanc623/PHPBenchTime/blob/master/src/Timer.php as well as the PHPDoc wikipedia entry: en.wikipedia.org/wiki/PHPDoc \$\endgroup\$ – jsanc623 Sep 18 '14 at 16:16

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.