3
\$\begingroup\$

I've wrote this class in PHP for my future projects:

<?php

    /**
     * Auth
     * This class is used to securely authenticate and register users on a given website.
     * This class uses the crypt() function, and PDO database engine.
     * 
     * @package Class Library
     * @author Truth
     * @copyright 2011
     * @version 1.00
     * @access public
     */

    class Auth {
        const HOST = 'localhost';       //Holds the database host. For most cases that would be 'localhost'
        const NAME = 'users';           //Holds the database name for the class to use. Assums 'users', change if needed.
        const USER = 'root';            //Holds the username for the database. CHANGE IF NEEDED!!
        const PASS = '';         //Holds the password for the database. CHANGE IF NEEDED

        const TABLE = 'users';          //Holds the name of the table. CHANGE IF NEEDED

        #NOTE, THE USER/PASS COMBO MUST HAVE SUFFICIENT PRIVLIGES FOR THE DATABASE IN NAME.
        /**
         * @var Auth::$db
         * Holds pointer to the PDO object
         */
        protected static $db; 

        /**
         * @var Auth::$user
         * Holds username information in the instance object
         */
        protected $user;
        /**
         * @var Auth::$pass
         * Holds password information in the instance object
         */
        protected $pass;

        /**
         * @var Auth::$hash
         * Holds the hashed password ready for database storage
         */
         protected $hash;

        /**
         * Auth::__construct()
         * 
         * @param string $user
         * @param string $pass
         * @return void
         */
        public function __construct($user = "", $pass = "") {
            if (empty($user) || empty($pass)) {
                throw new Exception("Empty username or password.");
            }
            $this->user = $user;
            $this->pass = $pass;
        }

        /**
         * Auth::hash()
         * Pass a user/password combination through the crypt algorithm and return the result.
         * 
         * @return string hash   Returns the complete hashed string.
         */
        private function hash() {
            $this->hash = crypt($this->pass, '$2a$10$'.sha1($this->user));
            return $this->hash;
        }

        /**
         * Auth::getUname()
         * Return the username
         * 
         * @return string   The username
         */
        public function getUname() {
            return $this->user;
        }

        /**
         * Auth::getHash()
         * Check if there's a hash, if not, make one, then return the hash.
         * 
         * @return string   Hashed password
         */
        public function getHash() {
            if (empty($this->hash)) {
                $this->hash();
            }
            return $this->hash;
        }

        public function __toString() {
            return $this->getHash();
        }

        /**
         * Auth::databaseConnect()
         * Establish connection to database and store the connection on self::$db.
         * 
         * @param PDO $pdo an optional PDO object to have an existing connection instead of a new one
         * @return void
         */
        public static function databaseConnect($pdo = null) {
            #The class accepts an external 
            if (is_object(self::$db)) {
                #Should the connection already been established, an exception will be thrown.
                #If you want to start the connection yourself, make sure that this function is called before any other class functions.
                throw new Exception('Could not complete operation. Connection was already established.');
            }
            if (is_object($pdo)) {
                if (!($pdo instanceof PDO)) {
                    throw Exception('Illegal Argument: Supplied argument is not a PDO object.');
                }
                #The function accepts an external PDO object as an argument.
                #WARNING: Do not use an object other then a PDO object, as it might cause unexpected results.
                self::$db = $pdo;
                return 0;
            }
            $dsn = "mysql:host=".self::HOST.";dbname=".self::NAME;
            try {
                self::$db = new PDO($dsn, self::USER, self::PASS); //Connect to the database, and store the pdo object.
                self::$db->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);
            }
            catch (PDOException $e) {
                throw new Exception("There have been an error in the connection: ". $e->getMessage());
            }
            #Here, connection is established.
        }

        /**
         * Auth::databaseCreate()
         * Create a default table (named self::TABLE) on the database.
         * Only use from within an installation page or a try/catch statement
         * 
         * @return void
         */
        public static function databaseCreate() {
            if (!is_object(self::$db)) {
                self::databaseConnect();
            }
            $table = self::TABLE;
            $query = <<<EOQ
CREATE TABLE $table(
  `uname` varchar(255) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL COMMENT 'Holds usernames (who also act as salt)',
  `phash` varchar(255) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL COMMENT 'Holds hashed passwords',
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT 'Unique user ID',
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE utf8_bin COMMENT='Default user table' AUTO_INCREMENT=1 ;
EOQ;
            //var_dump(self::$db);
            $stmt = self::$db->prepare($query);
            $stmt->execute();

        }



        /**
         * Auth::registerUser()
         * Insert a user into the database.
         * 
         * @param Auth $auth  The $auth object in question. Escaping is not needed.
         * @return void
         */
        public static function registerUser(Auth $auth) {
            if (!is_object(self::$db)) {
                self::databaseConnect();
            }
            try {
                $query = "INSERT INTO ".self::TABLE." (`uname`, `phash`, `id`) VALUES (:uname, :phash, NULL)";
                $stmt = self::$db->prepare($query);
                $stmt->bindValue(':uname', $auth->getUname());
                $stmt->bindValue(':phash', $auth->getHash());
                $stmt->execute();
            }
            catch (PDOException $e) {
                echo "There has been an error registering: ". $e->getMessage();
            }

        }

        /**
         * Auth::validateAgainstDatabase()
         * Validate a user against the database.
         * Exceptions detailing the potential error will be thrown. Make sure to catch them!
         * 
         * @param Auth $auth   The Auth object in question. No escaping needed.
         * @return bool $success   Whether the user is valid or not. 
         */
        public static function validateAgainstDatabase(Auth $auth) {
            if (!is_object(self::$db)) {
                self::databaseConnect();
            }
            try {
                $query = "SELECT `uname`, `phash` FROM ".self::TABLE." WHERE `uname` = :uname";
                $stmt = self::$db->prepare($query);
                $stmt->bindValue(':uname', $auth->getUname());
                $stmt->execute();

                $row = $stmt->fetch(PDO::FETCH_ASSOC);
                if (!$row) {
                    throw new Exception('Username does not exist in the system.');
                }
                if ($row['phash'] != $auth->getHash()) {
                    throw new Exception('Password does not match username.');
                }
                return true;
            }
            catch (PDOException $e) {
                echo "There was an error during the fetching: ". $e->getMessage();
            }
        }

    }
?>

What do you guys think? Can it be improved? Does it look good? Anything to consider?

\$\endgroup\$
1
  • \$\begingroup\$ Can you descripe or give a sample how you use this class in your page. Maybe with the different pages for login and validate the user against the DB. And use with sessions would be nice \$\endgroup\$
    – user25497
    May 26 '13 at 21:10
2
\$\begingroup\$

I would never write static methods in OO code. Others argue that they are ok in certain circumstances, however I make different design decisions to them. At the very least you should have thought long and hard about why you are making it static and be prepared to be stuck with the static dependency created in every class that will call your static method (and the testing overhead that this will add).

  1. They are very hard to test. See Misko Hevery's article Static Methods are a death to testability.
  2. They create a tight coupling in your code. Read nikic's Don't be STUPID: GRASP SOLID (especially the STU part).
\$\endgroup\$
7
  • \$\begingroup\$ Static methods in and of themselves aren't bad, it depends on what they do. Unseen dependencies and unseen state are bad, not the static keyword. \$\endgroup\$
    – deceze
    Jan 22 '12 at 7:21
  • \$\begingroup\$ @deceze Calling a static method couples the calling code. Any calls to Auth::databaseConnect would couple the calling code making it depend on an Auth class with a databaseConnect method. There is no good reason to write a static method ever. Just write a global function instead. Give it a namespace if you want. What good can a static method do that a non-static one can't do? (nothing) \$\endgroup\$
    – Paul
    Jan 22 '12 at 7:29
  • \$\begingroup\$ A static method is supposedly a global function, it's just a way to make it more organized and contained in the single class, that's all. \$\endgroup\$ Jan 22 '12 at 18:13
  • 1
    \$\begingroup\$ "No good reason to write a static method ever"? What about alternative object constructors? For example a static method that initializes an instance of its own class and sets a bunch of private properties, for example by restoring the object state from a database record. The only real way to do that is with static methods. $foo = new Foo vs. $foo = Foo::fromDbRecord($pdo, 42). There's no more or less coupling going on here. \$\endgroup\$
    – deceze
    Jan 22 '12 at 23:08
  • \$\begingroup\$ You are correct, the coupling is only slightly higher with the static (which is a Foo class with method fromDbRecord). What I am arguing for is doing neither of those as both of them create a tight coupling: as Misko Hevery suggests \$\endgroup\$
    – Paul
    Jan 23 '12 at 0:05
2
\$\begingroup\$
public function __construct($user = "", $pass = "") {
    if (empty($user) || empty($pass)) {

Don't use empty when the variable is guaranteed to exist. It just makes it harder to catch typos, since it suppresses PHP's error mechanism when there's no need to. Since you also require arguments for $user and $pass, don't give them default values:

public function __construct($user, $pass) {
    if (!$user || !$pass) {

Also, hardcoding the database credentials as constants and the database schema is probably not good. That's something you'll want to configure dynamically without modifying your code. Just pass a PDO instance into the class, since you'll likely want to use that connection in more than just the Auth class. Don't let the Auth class instantiate its own connection or manage the database schema, that's not its job.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your comments! I've improved my code. Anything else you can see there? \$\endgroup\$ Jan 22 '12 at 18:21

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.