HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Extremely basic authentication class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
extremelyclassauthenticationbasic

Problem

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...

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.

Solution

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.

Context

StackExchange Code Review Q#17650, answer score: 7

Revisions (0)

No revisions yet.