patternphpModerate

Authentication library

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

authenticationlibrarystackoverflow

Problem

I am building a library and I need suggestions on how to improve it (security, performance, etc).

```
db = $db;
$this->PasswordLib = $PasswordLib;
}

public function login($username, $password) {
try {
$sth = $this->db->prepare("SELECT id, username, password, active FROM user WHERE username = ?");
$sth->setFetchMode(PDO::FETCH_OBJ);
$sth->execute(array($username));
$result = $sth->fetch();
} catch (PDOException $e) {
throw new Exception($e->getMessage());
}

if ($result) {
if ($this->PasswordLib->verifyPasswordHash($password, $result->password)) {
if ($result->active == 0) {
$this->setMessage("You must activate your account!");
return FALSE;
}
session_regenerate_id();
$_SESSION['userid'] = $result->userid;
return TRUE;
} else {
$this->addLoginAttempt($username);
$this->captchaCount++;
$this->setMessage("The password you entered is incorrect. Please try again.Forgot your password? Request a new one.");
return FALSE;
}
} else {
$this->addLoginAttempt($username);
$this->captchaCount++;
$this->setMessage("Incorrect username");
return FALSE;
}
}

public function IsLoggedIn() {
if (isset($_SESSION['userid'])) {
return TRUE;
}
}

public function getLoginAttempts() {
$ip_address = ip2long($_SERVER['REMOTE_ADDR']);
$sth = $this->db->prepare("SELECT COUNT(*) FROM login_attempts WHERE ip_address = ? AND attempted > DATE_SUB(NOW(), INTERVAL 15 minute)");
$sth->bindParam(1, $ip_address);
$sth->execute();

return $sth->fetchColumn();
}

private function addLoginAttempt($username) {
$ip_

Solution

Disclaimer: I am no security guru. So a lot of what I say in regards to security are assumptions. The code review is good, but you should get a second, third, maybe even fourth opinion on my suggestions regarding security. I will denote each "opinion" that way you know to take it with a grain of salt. That being said, on to the review!

Session Variables

Are these session variables, userid and username, not the same? Most people only use one or the other, but I'm assuming that since you use both that means userid is a numerical representation of that person. I would try finding some way of limiting the amount of dependencies you need, especially if those two are more or less the same. Code is like a machine. The more moving parts you have, the more things there are to break. If you need both username and userid in your code I would find some way of producing one fromt the other. Also, I do not see where you set these variables. If they are crucial for checking if a user is logged in I would set them inside the class that way there is no confusion about where they come from.

Logging In

When you are checking if a user is logged in you only return TRUE. What if they aren't logged in? You should return either TRUE or FALSE that way you don't have to write anything extra when checking for it.

This is one of those grain of salt moments, though I feel confident in what I'm saying, I may be missing a step or may have misunderstood my source. From what I've read the more secure way is to compare the current session id to that of the one last used to log in. If the user has not logged in in a while then it would force the user to the login page to refresh credentials. After verifying session id, it would THEN verify that userid and username are set.

Logging Out

When logging out, it is unnecessary to use session_unset(). At least from what I gather. session_destroy() destroys everything in the session so that it is completely empty, wheras session_unset() merely unsets everything but leaves information such as session id available meaning the session still technically exists. So, even if it was necessary to use both, you got them backwards as there would be nothing left to unset once everything was destroyed, but plenty to destroy after everything has been unset.

I also do not think that exit is necessary. After setting a header your script will stop running before even getting to it.

Expanding Variables

This is a preference, but one MANY agree with. Variables should be expanded so it is completely obvious at first glance what they are used for. I won't argue for expanding $db or any other commonly used abbreviation that everyone seems to know, but I would argue that you shouldn't go making your own abbreviations. Its just a matter of legibility. So, in this instance $dest should probably be expanded to $destination. Normally I would point out $sth but having seen it so many times recently, I think that is also one of those standardly accepted abbreviations.

Separating Code

I do not think that the create_account() and generate_salt() methods should be included in this class as it does not have anything to do with authentication. It does share a common resource, $db, but it does two completely different things with it. One class should only read it, while the other manipulates it. However, this might be considered one of those design preferences. So I leave this up to you. Just figured I'd point it out.

Also, I would group my public/private/protected methods separately. I personally separate my code with long comment breaks that visually as well as physically separate each section so that it is easier to find the beginning of each section when scrolling. I'm not saying you need to do that, but I would at least separate them from each other. Makes it much easier to find methods if they are sorted in some fashion.

Code Dependency

This is a common mistake. Classes, generally, should not be dependent on any other file. No outside magical constants, variables, or classes should find their way into your models or controllers. Even if you aren't using an MVC pattern, it is good practice to make your classes completely self dependent, that way if something breaks you wont have to go hunting for it, it will all be self contained. So SITE_KEY and, in my opinion, those session variables should be defined inside this class.

Braces

Please, please, please use braces {}. I say this a lot, so please excuse me if I rant a little. A matching brace pair only adds 2 bits to your file size so there is no reason to exclude them for speed or file size limitations, though you should never have a file size "limit" to begin with. Braces, when combined with brace-matching in IDE's make it easy to determine where one statement ends, and another begins. Not only in IDE's, but also just in plain reading. The fact that PHP allows this sadens me to no end. It just supports poor coding habits and I w

Code Snippets

$fields = array(
    'username' => $username,
    'password' => $password,
    'salt' => $salt,
    'email' => $email,
    'created' => $created,
);

$attributes = implode(', ', array_keys($fields));
$values = implode(', :', array_keys($fields));

$sth = $this->db->prepare("INSERT INTO `user` ($attributes) VALUES (:$values)");

protected function check_record($field, $select, $value) {
    $sth = $this->db->prepare("SELECT $select FROM user WHERE $field = ?");
    $sth->execute(array($value));
    return $sth;
}

protected function get_salt($username) {
    $sth = $this->check_record('username', 'salt', $username);

    if ($sth->rowCount() > 0) {
        $row = $sth->fetch();
        return $row['salt'];
    }
}

"{$array['element']} or {$this->property}"

$this->captchaCount++;
    $this->setMessage($message);
    return FALSE;

Context

StackExchange Code Review Q#11661, answer score: 11

Revisions (0)

No revisions yet.