4
\$\begingroup\$

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Includes should go at the top of the PHP file, before the class declaration, not inside it. Also, use require_once rather than include - for one require_once performs better in that it throws a fatal error if the file is not found and stops the script (rather than letting it continue as include_once or include would, and for two, it checks that the file has not already been included and if it has been included will not require it again. Mind that require() executes faster than require_once(), so if you're sure there are no duplicated includes use require(). \$\endgroup\$
    – jsanc623
    Oct 17 '12 at 20:24
  • \$\begingroup\$ Is using includes/requires the best way to grab other classes? \$\endgroup\$
    – ohiock
    Oct 17 '12 at 20:25
  • 3
    \$\begingroup\$ Your code has at least 5 syntax errors in it. Code Review is for reviewing existing code, so please fix those syntax errors. (And @tbowman, your hunch is correct. You should typically avoid explicitly including classes. Autoloading is usually a better approach.) \$\endgroup\$
    – Corbin
    Oct 17 '12 at 20:28
5
\$\begingroup\$

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(); }
}
\$\endgroup\$
4
  • \$\begingroup\$ Wow, I couldn't ask for a better response. Thank you so much for the effort. This is certainly going to help me understand things better. \$\endgroup\$
    – ohiock
    Oct 17 '12 at 21:04
  • \$\begingroup\$ Not a problem. Just make sure to always check your code for syntax errors as Corbin pointed out. \$\endgroup\$
    – jsanc623
    Oct 17 '12 at 21:30
  • \$\begingroup\$ Tiny nitpick: return $error = "Username or password incorrect." is still a syntax error (and the assignment is pointless in a return). \$\endgroup\$
    – Corbin
    Oct 17 '12 at 22:05
  • \$\begingroup\$ Woops! Missing the semicolon and yep, can just return the string...thanks @Corbin! \$\endgroup\$
    – jsanc623
    Oct 18 '12 at 4:47
7
\$\begingroup\$

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.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ The problem with looking at other frameworks is that they have SO much more functionality added into it than I need in order to understand basic concepts, which is why I am here. Honestly, this isn't even a piece of code that is being utilized in a real application yet. It's just something I started to put together so that I can better understand how to arrange the components. \$\endgroup\$
    – ohiock
    Oct 17 '12 at 22:57
  • 1
    \$\begingroup\$ @tbowman ZF is more of a library than a framework, but yes, it is a bit much to jump into just to look at a few classes. \$\endgroup\$
    – Corbin
    Oct 18 '12 at 4:36
2
\$\begingroup\$

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.

\$\endgroup\$
11
  • \$\begingroup\$ That red bull high didn't help me catch errors in the code yesterday. Ech. But as with Corbin - great points pointed out by mseancole. tbowman - would suggest you take the advice of both mseancole and Corbin to heart. \$\endgroup\$
    – jsanc623
    Oct 18 '12 at 15:35
  • \$\begingroup\$ I apologize for submitting non-working code, but I do greatly appreciate everyones effort here. So are you saying that creating my sessions in a front controller is ideal? Maybe I'm misunderstanding. It seems like they should be started in the Auth controller considering the two go hand in hand. \$\endgroup\$
    – ohiock
    Oct 18 '12 at 15:48
  • \$\begingroup\$ @tbowman: No, they do belong in the Auth controller. I briefly mentioned a router because some people have adopted an MVCR (or MVRC) where the controller requests GET, POST, SESSION, and database connections from the router so that those common commands can be reused between controllers. Its just another level of abstraction that's sometimes helpful. \$\endgroup\$
    – mseancole
    Oct 18 '12 at 16:29
  • 1
    \$\begingroup\$ "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. " I meant to comment on this, but did not clarify. In its current state, this is not a controller. It's an authentication adapater. The most controller-y thing about is that it has controller in the name. \$\endgroup\$
    – Corbin
    Oct 18 '12 at 21:33
  • \$\begingroup\$ As for why sessions do not belong in there... Know those globals you mentioned? Know what $_SESSION is? :D. $_SESSION is basically meeting the need of persisting the authentication. This class has two responsibilities though: authenticating, and persisting authentication. Though almost always closely related, they do not have to be, and should not be tied together. Hard coding the session stuff ties them together. \$\endgroup\$
    – Corbin
    Oct 18 '12 at 21:35

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.