patternphpMinor
Extremely basic authentication class
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...
Is this typically how a class like this should work? I'm still very new to this stuff.
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:
These concerns should be separated.
The logic of "is
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.
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.