Extremely basic authentication class

Question

I'm in the process of building a small application and obviously I need some sort of way to authenticate users. I'm not sure if I am way off or if this is close to what I should be thinking. Please let me know what you think...

<?php
class Authentication_controller {
    private __constructor() {
        include(Authentication_model.php);

        $auth_model = new Authentication_Model();
        $auth_model->database_connect();
    }

    public login($username, $password) {
        if($auth_model->username($username)) {
            //Username exists, now check password
            if($auth_model->password($username, $password)) {
                //User is OK to login, send to designated page
                include(Page_controller.php);

                $page_controller = new Page_controller();
                $page_controller->go('home');

                //Create session
                session_start();

                //Set session variables
                $_SESSION['logged_in'] = true;
                $_SESSION['id'] = $auth_model->id($username);
                $_SESSION['access_level'] = $auth_model->access_level($username);
            } else {
                //Password didn't match
                return $error = "Username or password incorrect."
            }
        } else {
            //Username didn't match
            return $error = "Username or password incorrect."
        }
    }

    public permission($task_access_level) {
        $user_access_level = $_SESSION['access_level'];

        if($user_access_level >= $task_access_level) {
            //User has access level that is high enough for task
            return true;
        } else {
            //User does not have access level high enough for task
            return false;
        }
    }

    public logout() {
        session_destroy();
    }
}

Is this typically how a class like this should work? I'm still very new to this stuff.

Answer 1

I took the liberty of going through your code - see the comments preceded with MOD (modification), STAT (statement), QUES (question), SOL (solution) and REMOVED (removed)

# MOD: Moved all include() to here
# MOD: Changed from include() to require_once()
# MOD: Added "" around filename
require_once("Authentication_model.php");
require_once("Page_controller.php");

class Authentication_controller {
    # MOD: added 'function' 
    private function __constructor(){
        $auth_model = new Authentication_Model();
        $auth_model->database_connect();
    }

    # MOD: added 'function' 
    public function login($username, $password) {
        # QUES: Why are you doing two IF checks, with both returning the same error? 
        #       Let's make our life easier and do one IF check. 
        # STAT: We saved a few milliseconds per request!

        if($auth_model->username($username) && $auth_model->password($username, $password)){
            # User is OK to login, send to designated page
            $page_controller = new Page_controller();

            # STAT: Seems you're sending the user away to another page before initiating the session
            # SOL: Moved down
            # $page_controller->go('home');

            # Create session
            # MOD: Added '@' to prevent errors if session has already been started elsewhere
            @session_start();

            # Set session variables
            $_SESSION['logged_in'] = true;
            $_SESSION['id'] = $auth_model->id($username);
            $_SESSION['access_level'] = $auth_model->access_level($username);

            # ADD: Moved here
            $page_controller->go('home');
        } else{
            return $error = "Username or password incorrect."
        }

        /* MOD: OLD CODE
        if($auth_model->username($username)) {
            //Username exists, now check password
            if($auth_model->password($username, $password)) {
                //User is OK to login, send to designated page

                $page_controller = new Page_controller();
                $page_controller->go('home');

                //Create session
                session_start();

                //Set session variables
                $_SESSION['logged_in'] = true;
                $_SESSION['id'] = $auth_model->id($username);
                $_SESSION['access_level'] = $auth_model->access_level($username);
            } else {
                //Password didn't match
                return $error = "Username or password incorrect."
            }
        } else {
            //Username didn't match
            return $error = "Username or password incorrect."
        }*/
    }

    # MOD: added 'function' 
    public function permission($task_access_level) {
        # MOD: You're using $user_access_level once - why not just compare against $_SESSION?
        # REMOVED: $user_access_level = $_SESSION['access_level'];

        # Determine if user has access level high enough for task
        # MOD: Replaced $user_access_level with $_SESSION['access_level']
        if($_SESSION['access_level'] >= $task_access_level) return true; # High enough 
        else return false; # Not high enough
    }

    # MOD: added 'function' 
    public function logout() {
        session_destroy();
    }
}

And here's the same code with my comments removed

require_once("Authentication_model.php");
require_once("Page_controller.php");

class Authentication_controller {
    private function __constructor(){
        $auth_model = new Authentication_Model();
        $auth_model->database_connect();
    }

    public function login($username, $password) {
        if($auth_model->username($username) && $auth_model->password($username, $password)){
            # User is OK to login, send to designated page
            $page_controller = new Page_controller();

            @session_start(); # Create session

            # Set session variables
            $_SESSION['logged_in'] = true;
            $_SESSION['id'] = $auth_model->id($username);
            $_SESSION['access_level'] = $auth_model->access_level($username);

            $page_controller->go('home');
        } else{
            return $error = "Username or password incorrect."
        }
    }

    # Determine if user has access level high enough for task
    public function permission($task_access_level) {
        if($_SESSION['access_level'] >= $task_access_level) return true; # High enough 
        else return false; # Not high enough
    }

    public function logout(){ session_destroy(); }
}

Answer 2

jsanc623 had some good points, but I think he missed a lot too. In particular, I think you have a fairly major design problem. Without seeing all of the related classes, my comments are a bit limited, but there's a few things below.

---

There's a few different 'concerns' your code is handling here:

• Authenticating (is the username/password pair x, y valid?)
• Persisting an identity (remembering that the user is logged in under username x)
• Checking permissions

These concerns should be separated.

The logic of "is [x, y] a valid authentication pair?" and the logic of "can this person do z?" have no need to be coupled.

Additionally, there's no reason to hard code all of the session stuff. (The class should definitely not be responsible for starting the session.)

---

Based on the API seen, I have a suspicion that your controller and model classes have convoluted functionality and APIs.

You might want to consider looking into how a few different frameworks have handled authentication. Zend Framework is the only one I have experience with the authentication components. It has a few quirks, but overall, it has a good API.

Answer 3

I'm kind of sad that no one has commented on the global yet. Or the fact that, if the constructor is private, how is it being called?

Every time a programmer uses a global another programmer comes down off their red bull high, loses their wings, and misses a deadline. Globals are old and untrustworthy, especially in a security application such as an authenticator. Typically, if you need to share information between functions, you would pass those variables in as parameters. But with classes you can use properties instead.

private $auth_model;

private function __constructor(){
    $this->auth_model = new Authentication_Model();
    //etc...
}

public function login( $username, $password ) {
    if( $this->auth_model->username( $username ) ) {

As for the private constructor, I'm assuming you meant to use a Singleton pattern here? I don't have much experience with it myself, but I believe you will need to set up a static property and method that checks if the object has been instantiated, instantiates it, and returns a reference to it.

private static $instance;

//constructor, etc...

public static function getInstance() {
    if( ! self::$instance ) {
        self::$instance = new Authentication_controller();
    }

    return self::$instance; 
}

//usage
$authenticator = Authentication_controller::getInstance();
$authenticator->login();
//etc...

Clarification for jsan623's post

The reason that quotation marks are important around those includes, or any other strings for that matter, is that PHP is throwing warnings. If you had your error reporting turned on high enough you would notice the following warning: Authentication_model.php has been undefined, assuming "Authentication_model.php". Or something along those lines. This is just PHP's way of gracefully failing. You shouldn't rely on it. Turn up your error reporting so that you can see these kinds of things. Also, parenthesis aren't necessary around any of the include/require family, though that is preference.

The *_once versions are definitely slower than the non-*_once versions as they must make an additional request as to whether the content has already been included. This may not be all that noticeable with one or two includes, but when you start adding 10 or so it will quickly become apparent. So, don't use them unless you have to. You should be able to guarantee that your code wont repeat so this shouldn't be an issue. See Corbin's comment about autoloading.

Don't use error suppression. Do what ever checks you have to do to ensure that it works properly. In this case, start the session in the constructor and make sure it is the only session_start(). Since this appears to be the beginnings of an MVC framework the controller, or router if you decide to use one, should be in charge of sessions, so creating it in the constructor should ensure that it is only started once. I'm not sure why Corbin said the sessions shouldn't be here. Maybe I am wrong, but I would much rather not have my models dependent, or aware of, sessions. Create, manipulate, and read sessions in the controllers, neither your models, nor views need to know what's in the session. Unless someone can make a good point as to why they think they should be elsewhere?

Conclusion

As has already been pointed out. Code Review is for working code. You lucked out that someone decided to review this before it got closed. In the future, please take the time to write a working program before requesting a review. If it is a concept, still flesh it out enough so that it works.