patternphpModerate
PHP Authentication Class
Viewed 0 times
phpclassauthentication
Problem
I could use some review on this Auth class, as I'm sure it could use many improvements!
I wrote this fairly quickly, so please forgive any overseen bugs.
Example usage:
Source code:
I'm also not sure if using all static methods in the class could lead to problems later on?
I know there is currently no password hashing - I'm still working on the hashing method.
I wrote this fairly quickly, so please forgive any overseen bugs.
Example usage:
// Login a user
if (Auth::attempt($_POST['username'], $_POST['password']))
{
echo 'You have successfully logged in.';
}
// Check if the user is a guest
if (Auth::guest())
{
echo 'Please log in to see this page.';
}
// Logout a user
Auth::logout();Source code:
prepare('SELECT * FROM `users` WHERE `username` = :username AND `password` = :password');
$stmt->execute(array(':username' => $username, ':password' => $password));
if ($user = $stmt->fetch(PDO::FETCH_ASSOC))
{
static::$user = $user;
static::login($user['id']);
return true;
}
return false;
}
/**
* Login the user with the given id
*
* @param int $user_id
* @return void
*/
public static function login($user_id)
{
if ($user_id)
{
$_SESSION['user_id'] = $user_id;
}
}
/**
* Log out the current user
*
* @return void
*/
public static function logout()
{
static::$user = null;
$_SESSION = array();
unset($_SESSION['user_id']);
session_destroy();
}
/**
* Checks if a user is currently logged in
*
* @return bool
*/
public static function check()
{
return isset($_SESSION['user_id']);
}
/**
* Checks if the current user is a guest
*
* @return bool
*/
public static function guest()
{
return ! isset($_SESSION['user_id']);
}
}I'm also not sure if using all static methods in the class could lead to problems later on?
I know there is currently no password hashing - I'm still working on the hashing method.
Solution
What's up with static?
Your class is build like a utility class:
UtilityClasses are classes that have only static methods that perform some operation on the objects passed as parameters. Such classes typically have no state.
but it's not really one as it has state, you have a
The only benefit of using static like this is namespacing your functions, but that's not a real benefit as PHP supports namespaces. You should either write your class as an object oriented class, or a set of namespaced utility functions, everything in between is just bad form.
Since you mentioned execution speed, yes calling a static function is faster than instantiating an object and then calling the function, but the difference is in the range of a fraction of a millisecond. A more convincing argument for using static would be memory use, not execution speed, as the instantiated object will take up some memory. Still the argument is moot as there's absolutely no reason to write procedural code with classes, other than perhaps a misguided notion that everything needs to be object oriented.
Public properties
Assuming you rewrite this to be an object oriented class you should avoid public properties, or you'd be breaking encapsulation. All your class properties should be private and accessible through getters and setters, you should even avoid protected if you are not going to extend the class.
Global
Don't. Please forget the
Note the added benefit of type hinting. If you need that PDO object anywhere else in your class, then you should really transform this into an object oriented class and feed the dependency through its constructor:
Session
will fail. Either call
Further reading
Your class is build like a utility class:
UtilityClasses are classes that have only static methods that perform some operation on the objects passed as parameters. Such classes typically have no state.
but it's not really one as it has state, you have a
$user property you need to pass around, and your functions depend on it. You are writing procedural code using classes, and there really isn't any reason for that, if procedural programming feels more natural with you, go with it, object orientation is not the one true paradigm. PHP supports both paradigms equally, and they are both equally valid.The only benefit of using static like this is namespacing your functions, but that's not a real benefit as PHP supports namespaces. You should either write your class as an object oriented class, or a set of namespaced utility functions, everything in between is just bad form.
Since you mentioned execution speed, yes calling a static function is faster than instantiating an object and then calling the function, but the difference is in the range of a fraction of a millisecond. A more convincing argument for using static would be memory use, not execution speed, as the instantiated object will take up some memory. Still the argument is moot as there's absolutely no reason to write procedural code with classes, other than perhaps a misguided notion that everything needs to be object oriented.
Public properties
public static $user;Assuming you rewrite this to be an object oriented class you should avoid public properties, or you'd be breaking encapsulation. All your class properties should be private and accessible through getters and setters, you should even avoid protected if you are not going to extend the class.
Global
global $db;Don't. Please forget the
global keyword even exists, and pass your dependencies through method parameters:public static function attempt(PDO $db, $username, $password) {
...
}Note the added benefit of type hinting. If you need that PDO object anywhere else in your class, then you should really transform this into an object oriented class and feed the dependency through its constructor:
class Auth {
private $pdo;
public function __construct(PDO $pdo) {
$this->setPDO($pdo);
}
public function setPDO(PDO $pdo) {
$this->pdo = $pdo;
return $this;
}
public function getPDO() {
return $this->pdo;
}
...
}Auth::setPDO() and Auth::getPDO() are mutator methods, keeping the class in line with encapsulation and any half decent IDE can generate them automatically for you. Notice that I return $this; in the setter? That's because I like chaining my methods, up to you if you want to follow my style.Session
$_SESSION is also a global, and suffers from everything global variables do, but given that it's a native (almost) always on global, we can live with it. Obviously your class would be useless if at some point you decide to store session information in a different storage, but let's worry about than when and if it happens. However you need to keep in mind that $_SESSION is an external dependency, and your class needs to compensate for the rare occasion that you forgot to call session_start(). For example this:public static function login($user_id)
{
if ($user_id)
{
$_SESSION['user_id'] = $user_id;
}
}will fail. Either call
session_start() before your class definition or in the constructor. In either case, do check if $_SESSION already exists before calling it.Further reading
- Why is Global State so Evil?
- SOLID - Don't cheat, read all the linked articles.
- Dependency inversion principle
- Dependency injection
- Inversion of Control Containers and the Dependency Injection pattern
- When to Use Static Classes in C# - Not really C# specific, at least not the top voted answer.
Code Snippets
public static $user;global $db;public static function attempt(PDO $db, $username, $password) {
...
}class Auth {
private $pdo;
public function __construct(PDO $pdo) {
$this->setPDO($pdo);
}
public function setPDO(PDO $pdo) {
$this->pdo = $pdo;
return $this;
}
public function getPDO() {
return $this->pdo;
}
...
}public static function login($user_id)
{
if ($user_id)
{
$_SESSION['user_id'] = $user_id;
}
}Context
StackExchange Code Review Q#15828, answer score: 13
Revisions (0)
No revisions yet.