From 978d6461c5d40647c1efcc2965d2f55ea1d43025 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Mon, 12 Jun 2023 12:49:22 +0200 Subject: [PATCH 1/7] Database: add fetch_object, queryObject, queryObjects methods --- models/Database.php | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/models/Database.php b/models/Database.php index 226cd74..8bc0fad 100644 --- a/models/Database.php +++ b/models/Database.php @@ -59,6 +59,14 @@ class Database return mysqli_fetch_assoc($resource); } + /** + * Fetches a row from a given recordset, encapsulating into an object. + */ + public function fetch_object($resource, $class) + { + return mysqli_fetch_object($resource, $class); + } + /** * Fetches a row from a given recordset, using numeric keys. */ @@ -341,6 +349,41 @@ class Database return $db_string; } + /** + * Executes a query, returning an object of the row it returns. + */ + public function queryObject($class, $db_string, $db_values = []) + { + $res = $this->query($db_string, $db_values); + + if (!$res || $this->num_rows($res) == 0) + return null; + + $object = $this->fetch_object($res, $class); + $this->free_result($res); + + return $object; + } + + /** + * Executes a query, returning an array of objects of all the rows returns. + */ + public function queryObjects($class, $db_string, $db_values = []) + { + $res = $this->query($db_string, $db_values); + + if (!$res || $this->num_rows($res) == 0) + return []; + + $rows = []; + while ($object = $this->fetch_object($res, $class)) + $rows[] = $object; + + $this->free_result($res); + + return $rows; + } + /** * Executes a query, returning an array of all the rows it returns. */ -- 2.46.0 From 3de4e9391c9930018dcf6008f4ce32106a88d023 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Tue, 5 Nov 2024 16:39:42 +0100 Subject: [PATCH 2/7] Authentication: reorder methods alphabetically --- models/Authentication.php | 86 +++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/models/Authentication.php b/models/Authentication.php index 55f9dc3..60c5af0 100644 --- a/models/Authentication.php +++ b/models/Authentication.php @@ -29,31 +29,24 @@ class Authentication } /** - * Finds the user id belonging to a certain emailaddress. + * Checks a password for a given username against the database. */ - public static function getUserId($emailaddress) + public static function checkPassword($emailaddress, $password) { - $res = Registry::get('db')->queryValue(' - SELECT id_user + // Retrieve password hash for user matching the provided emailaddress. + $password_hash = Registry::get('db')->queryValue(' + SELECT password_hash FROM users WHERE emailaddress = {string:emailaddress}', [ 'emailaddress' => $emailaddress, ]); - return empty($res) ? false : $res; - } + // If there's no hash, the user likely does not exist. + if (!$password_hash) + return false; - public static function setResetKey($id_user) - { - return Registry::get('db')->query(' - UPDATE users - SET reset_key = {string:key} - WHERE id_user = {int:id}', - [ - 'id' => $id_user, - 'key' => self::newActivationKey(), - ]); + return password_verify($password, $password_hash); } public static function checkResetKey($id_user, $reset_key) @@ -69,6 +62,33 @@ class Authentication return $key == $reset_key; } + /** + * Computes a password hash. + */ + public static function computeHash($password) + { + $hash = password_hash($password, PASSWORD_DEFAULT); + if (!$hash) + throw new Exception('Hash creation failed!'); + return $hash; + } + + /** + * Finds the user id belonging to a certain emailaddress. + */ + public static function getUserId($emailaddress) + { + $res = Registry::get('db')->queryValue(' + SELECT id_user + FROM users + WHERE emailaddress = {string:emailaddress}', + [ + 'emailaddress' => $emailaddress, + ]); + + return empty($res) ? false : $res; + } + /** * Verifies whether the user is currently logged in. */ @@ -99,36 +119,16 @@ class Authentication return $string; } - /** - * Checks a password for a given username against the database. - */ - public static function checkPassword($emailaddress, $password) + public static function setResetKey($id_user) { - // Retrieve password hash for user matching the provided emailaddress. - $password_hash = Registry::get('db')->queryValue(' - SELECT password_hash - FROM users - WHERE emailaddress = {string:emailaddress}', + return Registry::get('db')->query(' + UPDATE users + SET reset_key = {string:key} + WHERE id_user = {int:id}', [ - 'emailaddress' => $emailaddress, + 'id' => $id_user, + 'key' => self::newActivationKey(), ]); - - // If there's no hash, the user likely does not exist. - if (!$password_hash) - return false; - - return password_verify($password, $password_hash); - } - - /** - * Computes a password hash. - */ - public static function computeHash($password) - { - $hash = password_hash($password, PASSWORD_DEFAULT); - if (!$hash) - throw new Exception('Hash creation failed!'); - return $hash; } /** -- 2.46.0 From 9c86d2c475d1ce1b36a37fee4dea641896d0a011 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Tue, 5 Nov 2024 16:44:54 +0100 Subject: [PATCH 3/7] Authentication: replace getUserId with Member::fromEmailAddress --- controllers/Login.php | 4 +++- controllers/ResetPassword.php | 16 ++++++++-------- models/Authentication.php | 16 ---------------- models/Member.php | 11 ++++++++++- 4 files changed, 21 insertions(+), 26 deletions(-) diff --git a/controllers/Login.php b/controllers/Login.php index d91785d..62373ba 100644 --- a/controllers/Login.php +++ b/controllers/Login.php @@ -24,7 +24,9 @@ class Login extends HTMLController if (Authentication::checkPassword($_POST['emailaddress'], $_POST['password'])) { parent::__construct('Login'); - $_SESSION['user_id'] = Authentication::getUserId($_POST['emailaddress']); + + $user = Member::fromEmailAddress($_POST['emailaddress']); + $_SESSION['user_id'] = $user->getUserId(); if (isset($_POST['redirect_url'])) header('Location: ' . base64_decode($_POST['redirect_url'])); diff --git a/controllers/ResetPassword.php b/controllers/ResetPassword.php index 24fa7b6..2a6f500 100644 --- a/controllers/ResetPassword.php +++ b/controllers/ResetPassword.php @@ -18,12 +18,12 @@ class ResetPassword extends HTMLController if (isset($_GET['step'], $_GET['email'], $_GET['key']) && $_GET['step'] == 2) { $email = rawurldecode($_GET['email']); - $id_user = Authentication::getUserid($email); - if ($id_user === false) + $user = Member::fromEmailAddress($email); + if (!$user) throw new UserFacingException('Invalid email address. Please make sure you copied the full link in the email you received.'); $key = $_GET['key']; - if (!Authentication::checkResetKey($id_user, $key)) + if (!Authentication::checkResetKey($user->getUserId(), $key)) throw new UserFacingException('Invalid reset token. Please make sure you copied the full link in the email you received. Note: you cannot use the same token twice.'); parent::__construct('Reset password - ' . SITE_TITLE); @@ -42,7 +42,7 @@ class ResetPassword extends HTMLController // So, are we good to go? if (empty($missing)) { - Authentication::updatePassword($id_user, Authentication::computeHash($_POST['password1'])); + Authentication::updatePassword($user->getUserId(), Authentication::computeHash($_POST['password1'])); $_SESSION['login_msg'] = ['Your password has been reset', 'You can now use the form below to log in to your account.', 'success']; header('Location: ' . BASEURL . '/login/'); exit; @@ -60,15 +60,15 @@ class ResetPassword extends HTMLController // Have they submitted an email address yet? if (isset($_POST['emailaddress']) && preg_match('~^.+@.+\.[a-z]+$~', trim($_POST['emailaddress']))) { - $id_user = Authentication::getUserid(trim($_POST['emailaddress'])); - if ($id_user === false) + $user = Member::fromEmailAddress($_POST['emailaddress']); + if (!$user) { $form->adopt(new Alert('Invalid email address', 'The email address you provided could not be found in our system. Please try again.', 'danger')); return; } - Authentication::setResetKey($id_user); - Email::resetMail($id_user); + Authentication::setResetKey($user->getUserId()); + Email::resetMail($user->getUserId()); // Show the success message $this->page->clear(); diff --git a/models/Authentication.php b/models/Authentication.php index 60c5af0..c92f9af 100644 --- a/models/Authentication.php +++ b/models/Authentication.php @@ -73,22 +73,6 @@ class Authentication return $hash; } - /** - * Finds the user id belonging to a certain emailaddress. - */ - public static function getUserId($emailaddress) - { - $res = Registry::get('db')->queryValue(' - SELECT id_user - FROM users - WHERE emailaddress = {string:emailaddress}', - [ - 'emailaddress' => $emailaddress, - ]); - - return empty($res) ? false : $res; - } - /** * Verifies whether the user is currently logged in. */ diff --git a/models/Member.php b/models/Member.php index 84912a4..d276313 100644 --- a/models/Member.php +++ b/models/Member.php @@ -8,7 +8,7 @@ class Member extends User { - private function __construct($data) + private function __construct($data = []) { foreach ($data as $key => $value) $this->$key = $value; @@ -18,6 +18,15 @@ class Member extends User $this->is_admin = $this->is_admin == 1; } + public static function fromEmailAddress($email_address) + { + return Registry::get('db')->queryObject(static::class, ' + SELECT * + FROM users + WHERE emailaddress = {string:email_address}', + ['email_address' => $email_address]); + } + public static function fromId($id_user) { $row = Registry::get('db')->queryAssoc(' -- 2.46.0 From 8eaeb6c3328d077e6b49786f67a083a5f6558435 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Tue, 5 Nov 2024 16:45:40 +0100 Subject: [PATCH 4/7] Authentication: remove remnants of user agent checks --- models/Authentication.php | 9 --------- 1 file changed, 9 deletions(-) diff --git a/models/Authentication.php b/models/Authentication.php index c92f9af..afcc55d 100644 --- a/models/Authentication.php +++ b/models/Authentication.php @@ -78,15 +78,6 @@ class Authentication */ public static function isLoggedIn() { - // Check whether the active session matches the current user's environment. - if (isset($_SESSION['ip_address'], $_SESSION['user_agent']) && ( - (isset($_SERVER['REMOTE_ADDR']) && $_SESSION['ip_address'] != $_SERVER['REMOTE_ADDR']) || - (isset($_SERVER['HTTP_USER_AGENT']) && $_SESSION['user_agent'] != $_SERVER['HTTP_USER_AGENT']))) - { - session_destroy(); - return false; - } - // A user is logged in if a user id exists in the session and this id is (still) in the database. return isset($_SESSION['user_id']) && self::checkExists($_SESSION['user_id']); } -- 2.46.0 From 084658820ec4aa235303cbe643a894ef4c1cab5c Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Tue, 5 Nov 2024 16:46:53 +0100 Subject: [PATCH 5/7] Authentication: replace checkExists with Member::fromId --- models/Authentication.php | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/models/Authentication.php b/models/Authentication.php index afcc55d..8793282 100644 --- a/models/Authentication.php +++ b/models/Authentication.php @@ -12,22 +12,6 @@ */ class Authentication { - /** - * Checks whether a user still exists in the database. - */ - public static function checkExists($id_user) - { - $res = Registry::get('db')->queryValue(' - SELECT id_user - FROM users - WHERE id_user = {int:id}', - [ - 'id' => $id_user, - ]); - - return $res !== null; - } - /** * Checks a password for a given username against the database. */ @@ -78,8 +62,18 @@ class Authentication */ public static function isLoggedIn() { - // A user is logged in if a user id exists in the session and this id is (still) in the database. - return isset($_SESSION['user_id']) && self::checkExists($_SESSION['user_id']); + if (!isset($_SESSION['user_id'])) + return false; + + try + { + $exists = Member::fromId($_SESSION['user_id']); + return true; + } + catch (NotFoundException $e) + { + return false; + } } /** -- 2.46.0 From eb7a40a70d4844070fd57cac4e2eea14d619a7bd Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Tue, 5 Nov 2024 17:17:14 +0100 Subject: [PATCH 6/7] ResetPassword: introduce requestResetKey and verifyResetKey methods --- controllers/ResetPassword.php | 118 ++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 56 deletions(-) diff --git a/controllers/ResetPassword.php b/controllers/ResetPassword.php index 2a6f500..dc4c806 100644 --- a/controllers/ResetPassword.php +++ b/controllers/ResetPassword.php @@ -16,66 +16,72 @@ class ResetPassword extends HTMLController // Verifying an existing reset key? if (isset($_GET['step'], $_GET['email'], $_GET['key']) && $_GET['step'] == 2) - { - $email = rawurldecode($_GET['email']); - $user = Member::fromEmailAddress($email); - if (!$user) - throw new UserFacingException('Invalid email address. Please make sure you copied the full link in the email you received.'); - - $key = $_GET['key']; - if (!Authentication::checkResetKey($user->getUserId(), $key)) - throw new UserFacingException('Invalid reset token. Please make sure you copied the full link in the email you received. Note: you cannot use the same token twice.'); - - parent::__construct('Reset password - ' . SITE_TITLE); - $form = new PasswordResetForm($email, $key); - $this->page->adopt($form); - - // Are they trying to set something already? - if (isset($_POST['password1'], $_POST['password2'])) - { - $missing = []; - if (strlen($_POST['password1']) < 6 || !preg_match('~[^A-z]~', $_POST['password1'])) - $missing[] = 'Please fill in a password that is at least six characters long and contains at least one non-alphabetic character (e.g. a number or symbol).'; - if ($_POST['password1'] != $_POST['password2']) - $missing[] = 'The passwords you entered do not match.'; - - // So, are we good to go? - if (empty($missing)) - { - Authentication::updatePassword($user->getUserId(), Authentication::computeHash($_POST['password1'])); - $_SESSION['login_msg'] = ['Your password has been reset', 'You can now use the form below to log in to your account.', 'success']; - header('Location: ' . BASEURL . '/login/'); - exit; - } - else - $form->adopt(new Alert('Some fields require your attention', '
  • ' . implode('
  • ', $missing) . '
', 'danger')); - } - } + $this->verifyResetKey(); else + $this->requestResetKey(); + } + + private function requestResetKey() + { + parent::__construct('Reset password - ' . SITE_TITLE); + $form = new ForgotPasswordForm(); + $this->page->adopt($form); + + // Have they submitted an email address yet? + if (isset($_POST['emailaddress']) && preg_match('~^.+@.+\.[a-z]+$~', trim($_POST['emailaddress']))) { - parent::__construct('Reset password - ' . SITE_TITLE); - $form = new ForgotPasswordForm(); - $this->page->adopt($form); - - // Have they submitted an email address yet? - if (isset($_POST['emailaddress']) && preg_match('~^.+@.+\.[a-z]+$~', trim($_POST['emailaddress']))) + $user = Member::fromEmailAddress($_POST['emailaddress']); + if (!$user) { - $user = Member::fromEmailAddress($_POST['emailaddress']); - if (!$user) - { - $form->adopt(new Alert('Invalid email address', 'The email address you provided could not be found in our system. Please try again.', 'danger')); - return; - } - - Authentication::setResetKey($user->getUserId()); - Email::resetMail($user->getUserId()); - - // Show the success message - $this->page->clear(); - $box = new DummyBox('An email has been sent'); - $box->adopt(new Alert('', 'We have sent an email to ' . $_POST['emailaddress'] . ' containing details on how to reset your password.', 'success')); - $this->page->adopt($box); + $form->adopt(new Alert('Invalid email address', 'The email address you provided could not be found in our system. Please try again.', 'danger')); + return; } + + Authentication::setResetKey($user->getUserId()); + Email::resetMail($user->getUserId()); + + // Show the success message + $this->page->clear(); + $box = new DummyBox('An email has been sent'); + $box->adopt(new Alert('', 'We have sent an email to ' . $_POST['emailaddress'] . ' containing details on how to reset your password.', 'success')); + $this->page->adopt($box); + } + } + + private function verifyResetKey() + { + $email = rawurldecode($_GET['email']); + $user = Member::fromEmailAddress($email); + if (!$user) + throw new UserFacingException('Invalid email address. Please make sure you copied the full link in the email you received.'); + + $key = $_GET['key']; + if (!Authentication::checkResetKey($user->getUserId(), $key)) + throw new UserFacingException('Invalid reset token. Please make sure you copied the full link in the email you received. Note: you cannot use the same token twice.'); + + parent::__construct('Reset password - ' . SITE_TITLE); + $form = new PasswordResetForm($email, $key); + $this->page->adopt($form); + + // Are they trying to set something already? + if (isset($_POST['password1'], $_POST['password2'])) + { + $missing = []; + if (strlen($_POST['password1']) < 6 || !preg_match('~[^A-z]~', $_POST['password1'])) + $missing[] = 'Please fill in a password that is at least six characters long and contains at least one non-alphabetic character (e.g. a number or symbol).'; + if ($_POST['password1'] != $_POST['password2']) + $missing[] = 'The passwords you entered do not match.'; + + // So, are we good to go? + if (empty($missing)) + { + Authentication::updatePassword($user->getUserId(), Authentication::computeHash($_POST['password1'])); + $_SESSION['login_msg'] = ['Your password has been reset', 'You can now use the form below to log in to your account.', 'success']; + header('Location: ' . BASEURL . '/login/'); + exit; + } + else + $form->adopt(new Alert('Some fields require your attention', '
  • ' . implode('
  • ', $missing) . '
', 'danger')); } } } -- 2.46.0 From adfb5a2198a37409762e3ede4430366249cd7ceb Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Tue, 5 Nov 2024 17:19:59 +0100 Subject: [PATCH 7/7] ResetPassword: add time-out to password resets; prevent repeated mails --- controllers/ResetPassword.php | 22 ++++++++++++++++ migrations/2024-11-05.sql | 2 ++ models/Authentication.php | 48 ++++++++++++++++++++++++++++++++++- models/User.php | 1 + 4 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 migrations/2024-11-05.sql diff --git a/controllers/ResetPassword.php b/controllers/ResetPassword.php index dc4c806..cd0d56e 100644 --- a/controllers/ResetPassword.php +++ b/controllers/ResetPassword.php @@ -37,6 +37,24 @@ class ResetPassword extends HTMLController return; } + if (Authentication::getResetTimeOut($user->getUserId()) > 0) + { + // Update the reset time-out to prevent hammering + $resetTimeOut = Authentication::updateResetTimeOut($user->getUserId()); + + // Present it to the user in a readable way + if ($resetTimeOut > 3600) + $timeOut = sprintf('%d hours', ceil($resetTimeOut / 3600)); + elseif ($resetTimeOut > 60) + $timeOut = sprintf('%d minutes', ceil($resetTimeOut / 60)); + else + $timeOut = sprintf('%d seconds', $resetTimeOut); + + $form->adopt(new Alert('Password reset token already sent', 'We already sent a password reset token to this email address recently. ' . + 'If no email was received, please wait ' . $timeOut . ' to try again.', 'error')); + return; + } + Authentication::setResetKey($user->getUserId()); Email::resetMail($user->getUserId()); @@ -76,6 +94,10 @@ class ResetPassword extends HTMLController if (empty($missing)) { Authentication::updatePassword($user->getUserId(), Authentication::computeHash($_POST['password1'])); + + // Consume token, ensuring it isn't used again + Authentication::consumeResetKey($user->getUserId()); + $_SESSION['login_msg'] = ['Your password has been reset', 'You can now use the form below to log in to your account.', 'success']; header('Location: ' . BASEURL . '/login/'); exit; diff --git a/migrations/2024-11-05.sql b/migrations/2024-11-05.sql new file mode 100644 index 0000000..ee37d8f --- /dev/null +++ b/migrations/2024-11-05.sql @@ -0,0 +1,2 @@ +/* Add time-out to password reset keys, and prevent repeated mails */ +ALTER TABLE `users` ADD `reset_blocked_until` INT UNSIGNED NULL AFTER `reset_key`; diff --git a/models/Authentication.php b/models/Authentication.php index 8793282..324ad91 100644 --- a/models/Authentication.php +++ b/models/Authentication.php @@ -12,6 +12,8 @@ */ class Authentication { + const DEFAULT_RESET_TIMEOUT = 30; + /** * Checks a password for a given username against the database. */ @@ -57,6 +59,27 @@ class Authentication return $hash; } + public static function consumeResetKey($id_user) + { + return Registry::get('db')->query(' + UPDATE users + SET reset_key = NULL, + reset_blocked_until = NULL + WHERE id_user = {int:id_user}', + ['id_user' => $id_user]); + } + + public static function getResetTimeOut($id_user) + { + $resetTime = Registry::get('db')->queryValue(' + SELECT reset_blocked_until + FROM users + WHERE id_user = {int:id_user}', + ['id_user' => $id_user]); + + return max(0, $resetTime - time()); + } + /** * Verifies whether the user is currently logged in. */ @@ -92,7 +115,8 @@ class Authentication { return Registry::get('db')->query(' UPDATE users - SET reset_key = {string:key} + SET reset_key = {string:key}, + reset_blocked_until = UNIX_TIMESTAMP() + ' . static::DEFAULT_RESET_TIMEOUT . ' WHERE id_user = {int:id}', [ 'id' => $id_user, @@ -117,4 +141,26 @@ class Authentication 'blank' => '', ]); } + + public static function updateResetTimeOut($id_user) + { + $currentResetTimeOut = static::getResetTimeOut($id_user); + + // New timeout: between 30 seconds, double the current timeout, and a full day + $newResetTimeOut = min(max(static::DEFAULT_RESET_TIMEOUT, $currentResetTimeOut * 2), 60 * 60 * 24); + + $success = Registry::get('db')->query(' + UPDATE users + SET reset_blocked_until = {int:new_time_out} + WHERE id_user = {int:id_user}', + [ + 'id_user' => $id_user, + 'new_time_out' => time() + $newResetTimeOut, + ]); + + if (!$success) + throw new UnexpectedValueException('Could not set password reset timeout!'); + + return $newResetTimeOut; + } } diff --git a/models/User.php b/models/User.php index b69790d..a26241d 100644 --- a/models/User.php +++ b/models/User.php @@ -23,6 +23,7 @@ abstract class User protected $ip_address; protected $is_admin; protected $reset_key; + protected $reset_blocked_until; protected bool $is_logged; protected bool $is_guest; -- 2.46.0