From a5d79b230699b6df3e2ebc9fdc8746877388bf47 Mon Sep 17 00:00:00 2001 From: flashwave Date: Wed, 27 May 2020 17:05:23 +0000 Subject: [PATCH] Moved user validation code into User object. --- misuzu.php | 1 - public/auth/password.php | 48 +++++----- public/auth/register.php | 8 +- public/manage/users/user.php | 13 ++- public/settings/account.php | 11 ++- src/Comments/CommentsParser.php | 2 +- src/Users/User.php | 135 ++++++++++++++++++++++++++--- src/Users/user_legacy.php | 50 ----------- src/Users/validation.php | 96 -------------------- templates/auth/password_reset.twig | 4 +- 10 files changed, 169 insertions(+), 199 deletions(-) delete mode 100644 src/Users/validation.php diff --git a/misuzu.php b/misuzu.php index 529f74ae..50049d5c 100644 --- a/misuzu.php +++ b/misuzu.php @@ -88,7 +88,6 @@ require_once 'src/Users/relations.php'; require_once 'src/Users/role.php'; require_once 'src/Users/session.php'; require_once 'src/Users/user_legacy.php'; -require_once 'src/Users/validation.php'; require_once 'src/Users/warning.php'; $dbConfig = parse_ini_file(MSZ_ROOT . '/config/config.ini', true, INI_SCANNER_TYPED); diff --git a/public/auth/password.php b/public/auth/password.php index 3c1962fc..584bb5a5 100644 --- a/public/auth/password.php +++ b/public/auth/password.php @@ -5,6 +5,7 @@ use UnexpectedValueException; use Misuzu\AuditLog; use Misuzu\Net\IPAddress; use Misuzu\Users\User; +use Misuzu\Users\UserNotFoundException; use Misuzu\Users\UserLoginAttempt; use Misuzu\Users\UserSession; @@ -20,12 +21,14 @@ $forgot = !empty($_POST['forgot']) && is_array($_POST['forgot']) ? $_POST['forgo $userId = !empty($reset['user']) ? (int)$reset['user'] : ( !empty($_GET['user']) ? (int)$_GET['user'] : 0 ); -$username = $userId > 0 ? user_username_from_id($userId) : ''; -if($userId > 0 && empty($username)) { - url_redirect('auth-forgot'); - return; -} +if($userId > 0) + try { + $userInfo = User::byId($userId); + } catch(UserNotFoundException $ex) { + url_redirect('auth-forgot'); + return; + } $notices = []; $siteIsPrivate = Config::get('private.enable', Config::TYPE_BOOL); @@ -57,18 +60,16 @@ while($canResetPassword) { break; } - if(user_validate_password($passwordNew) !== '') { + if(User::validatePassword($passwordNew) !== '') { $notices[] = 'Your password is too weak!'; break; } - if(user_password_set($userId, $passwordNew)) { - AuditLog::create(AuditLog::PASSWORD_RESET, [], User::byId($userId)); - } else { - throw new UnexpectedValueException('Password reset failed.'); - } + $userInfo->setPassword($passwordNew); + AuditLog::create(AuditLog::PASSWORD_RESET, [], $userInfo); // disable two factor auth to prevent getting locked out of account entirely + // this behaviour should really be replaced with recovery keys... user_totp_update($userId, null); user_recovery_token_invalidate($userId, $verificationCode); @@ -93,49 +94,52 @@ while($canResetPassword) { break; } - $forgotUser = user_find_for_reset($forgot['email']); + try { + $forgotUser = User::byEMailAddress($forgot['email']); + } catch(UserNotFoundException $ex) { + unset($forgotUser); + } - if(empty($forgotUser)) { + if(empty($forgotUser) || $forgotUser->isDeleted()) { $notices[] = "This e-mail address is not registered with us."; break; } - if(!user_recovery_token_sent($forgotUser['user_id'], $ipAddress)) { - $verificationCode = user_recovery_token_create($forgotUser['user_id'], $ipAddress); + if(!user_recovery_token_sent($forgotUser->getId(), $ipAddress)) { + $verificationCode = user_recovery_token_create($forgotUser->getId(), $ipAddress); if(empty($verificationCode)) { throw new UnexpectedValueException('A verification code failed to insert.'); } $recoveryMessage = Mailer::template('password-recovery', [ - 'username' => $forgotUser['username'], + 'username' => $forgotUser->getUsername(), 'token' => $verificationCode, ]); $recoveryMail = Mailer::sendMessage( - [$forgotUser['email'] => $forgotUser['username']], + [$forgotUser->getEMailAddress() => $forgotUser->getUsername()], $recoveryMessage['subject'], $recoveryMessage['message'] ); if(!$recoveryMail) { $notices[] = "Failed to send reset email, please contact the administrator."; - user_recovery_token_invalidate($forgotUser['user_id'], $verificationCode); + user_recovery_token_invalidate($forgotUser->getId(), $verificationCode); break; } } - url_redirect('auth-reset', ['user' => $forgotUser['user_id']]); + url_redirect('auth-reset', ['user' => $forgotUser->getId()]); return; } break; } -Template::render($userId > 0 ? 'auth.password_reset' : 'auth.password_forgot', [ +Template::render(isset($userInfo) ? 'auth.password_reset' : 'auth.password_forgot', [ 'password_notices' => $notices, 'password_email' => !empty($forget['email']) && is_string($forget['email']) ? $forget['email'] : '', 'password_attempts_remaining' => $remainingAttempts, - 'password_user_id' => $userId, - 'password_username' => $username, + 'password_user' => $userInfo ?? null, 'password_verification' => $verificationCode ?? '', ]); diff --git a/public/auth/register.php b/public/auth/register.php index b6f68ca6..11ef4fc9 100644 --- a/public/auth/register.php +++ b/public/auth/register.php @@ -48,12 +48,12 @@ while(!$restricted && !empty($register)) { break; } - $usernameValidation = user_validate_username($register['username'], true); + $usernameValidation = User::validateUsername($register['username']); if($usernameValidation !== '') { - $notices[] = MSZ_USER_USERNAME_VALIDATION_STRINGS[$usernameValidation]; + $notices[] = User::usernameValidationErrorString($usernameValidation); } - $emailValidation = user_validate_email($register['email'], true); + $emailValidation = User::validateEMailAddress($register['email']); if($emailValidation !== '') { $notices[] = $emailValidation === 'in-use' ? 'This e-mail address has already been used!' @@ -64,7 +64,7 @@ while(!$restricted && !empty($register)) { $notices[] = 'The given passwords don\'t match.'; } - if(user_validate_password($register['password']) !== '') { + if(User::validatePassword($register['password']) !== '') { $notices[] = 'Your password is too weak!'; } diff --git a/public/manage/users/user.php b/public/manage/users/user.php index f9e73df7..66fefca7 100644 --- a/public/manage/users/user.php +++ b/public/manage/users/user.php @@ -111,15 +111,14 @@ if(CSRF::validateRequest() && $canEdit) { $setUserInfo['display_role'] = $displayRole; } - $usernameValidation = user_validate_username($setUserInfo['username']); - $emailValidation = user_validate_email($setUserInfo['email']); + $usernameValidation = User::validateUsername($setUserInfo['username']); + $emailValidation = User::validateEMailAddress($setUserInfo['email']); $countryValidation = strlen($setUserInfo['user_country']) === 2 && ctype_alpha($setUserInfo['user_country']) && ctype_upper($setUserInfo['user_country']); - if(!empty($usernameValidation)) { - $notices[] = MSZ_USER_USERNAME_VALIDATION_STRINGS[$usernameValidation]; - } + if(!empty($usernameValidation)) + $notices[] = User::usernameValidationErrorString($usernameValidation); if(!empty($emailValidation)) { $notices[] = $emailValidation === 'in-use' @@ -163,10 +162,10 @@ if(CSRF::validateRequest() && $canEdit) { if(!empty($passwordNewValue)) { if($passwordNewValue !== $passwordConfirmValue) { $notices[] = 'Confirm password does not match.'; - } elseif(!empty(user_validate_password($passwordNewValue))) { + } elseif(!empty(User::validatePassword($passwordNewValue))) { $notices[] = 'New password is too weak.'; } else { - $setUserInfo['password'] = user_password_hash($passwordNewValue); + $setUserInfo['password'] = User::hashPassword($passwordNewValue); } } } diff --git a/public/settings/account.php b/public/settings/account.php index 479ef0fd..bc81b67b 100644 --- a/public/settings/account.php +++ b/public/settings/account.php @@ -17,7 +17,6 @@ if(!UserSession::hasCurrent()) { $errors = []; $currentUser = User::getCurrent(); $currentUserId = $currentUser->getId(); -$currentEmail = user_email_get($currentUserId); $isRestricted = user_warning_check_restriction($currentUserId); $twoFactorInfo = user_totp_info($currentUserId); $isVerifiedRequest = CSRF::validateRequest(); @@ -77,10 +76,10 @@ if($isVerifiedRequest && !empty($_POST['current_password'])) { if(!empty($_POST['email']['new'])) { if(empty($_POST['email']['confirm']) || $_POST['email']['new'] !== $_POST['email']['confirm']) { $errors[] = 'The addresses you entered did not match each other.'; - } elseif($currentEmail === mb_strtolower($_POST['email']['confirm'])) { + } elseif($currentUser->getEMailAddress() === mb_strtolower($_POST['email']['confirm'])) { $errors[] = 'This is already your e-mail address!'; } else { - $checkMail = user_validate_email($_POST['email']['new'], true); + $checkMail = User::validateEMailAddress($_POST['email']['new'], true); if($checkMail !== '') { switch($checkMail) { @@ -113,12 +112,12 @@ if($isVerifiedRequest && !empty($_POST['current_password'])) { if(empty($_POST['password']['confirm']) || $_POST['password']['new'] !== $_POST['password']['confirm']) { $errors[] = 'The new passwords you entered did not match each other.'; } else { - $checkPassword = user_validate_password($_POST['password']['new']); + $checkPassword = User::validatePassword($_POST['password']['new']); if($checkPassword !== '') { $errors[] = 'The given passwords was too weak.'; } else { - user_password_set($currentUserId, $_POST['password']['new']); + $currentUser->setPassword($_POST['password']['new']); AuditLog::create(AuditLog::PERSONAL_PASSWORD_CHANGE); } } @@ -130,7 +129,7 @@ $userRoles = user_role_all_user($currentUserId); Template::render('settings.account', [ 'errors' => $errors, - 'current_email' => $currentEmail, + 'current_email' => $currentUser->getEMailAddress(), 'user_roles' => $userRoles, 'user_display_role' => user_role_get_display($currentUserId), 'is_restricted' => $isRestricted, diff --git a/src/Comments/CommentsParser.php b/src/Comments/CommentsParser.php index 34b9e3fd..9602315f 100644 --- a/src/Comments/CommentsParser.php +++ b/src/Comments/CommentsParser.php @@ -5,7 +5,7 @@ use Misuzu\DB; use Misuzu\Users\User; class CommentsParser { - private const MARKUP_USERNAME = '#\B(?:@{1}(' . MSZ_USERNAME_REGEX . '))#u'; + private const MARKUP_USERNAME = '#\B(?:@{1}(' . User::NAME_REGEX . '))#u'; private const MARKUP_USERID = '#\B(?:@{2}([0-9]+))#u'; public static function parseForStorage(string $text): string { diff --git a/src/Users/User.php b/src/Users/User.php index 1b89526b..c56bb4cd 100644 --- a/src/Users/User.php +++ b/src/Users/User.php @@ -11,6 +11,16 @@ class UserException extends UsersException {} // this naming definitely won't le class UserNotFoundException extends UserException {} class User { + public const NAME_MIN_LENGTH = 3; // Minimum username length + public const NAME_MAX_LENGTH = 16; // Maximum username length, unless your name is Flappyzor(WorldwideOnline2018through2019through2020) + public const NAME_REGEX = '[A-Za-z0-9-_]+'; // Username character constraint + + // Minimum amount of unique characters for passwords + public const PASSWORD_UNIQUE = 6; + + // Password hashing algorithm + public const PASSWORD_ALGO = PASSWORD_ARGON2ID; + // Database fields // TODO: update all references to use getters and setters and mark all of these as private public $user_id = -1; @@ -39,6 +49,20 @@ class User { private $totp = null; + public const TABLE = 'users'; + private const QUERY_SELECT = 'SELECT %1$s FROM `' . DB::PREFIX . self::TABLE . '` AS '. self::TABLE; + private const SELECT = '%1$s.`user_id`, %1$s.`username`, %1$s.`password`, %1$s.`email`, %1$s.`user_super`, %1$s.`user_title`, ' + . ', %1$s.`user_country`, %1$s.`user_colour`, %1$s.`display_role`, %1$s.`user_totp_key`' + . ', %1$s.`user_about_content`, %1$s.`user_about_parser`' + . ', %1$s.`user_signature_content`, %1$s.`user_signature_parser`' + . ', %1$s.`user_birthdate`, %1$s.`user_background_settings`' + . ', INET6_NTOA(%1$s.`register_ip`) AS `register_ip`' + . ', INET6_NTOA(%1$s.`last_ip`) AS `last_ip`' + . ', UNIX_TIMESTAMP(%1$s.`user_created`) AS `user_created`' + . ', UNIX_TIMESTAMP(%1$s.`user_active`) AS `user_active`' + . ', UNIX_TIMESTAMP(%1$s.`user_deleted`) AS `user_deleted`'; + + // Stop using this one and use the one above private const USER_SELECT = ' SELECT u.`user_id`, u.`username`, u.`password`, u.`email`, u.`user_super`, u.`user_title`, u.`user_country`, u.`user_colour`, u.`display_role`, u.`user_totp_key`, @@ -79,6 +103,14 @@ class User { return $this->email; } + public function getRegisterRemoteAddress(): string { + return $this->register_ip; + } + + public function getLastRemoteAddress(): string { + return $this->last_ip; + } + public function getHierarchy(): int { return ($userId = $this->getId()) < 1 ? 0 : user_get_hierarchy($userId); } @@ -90,15 +122,12 @@ class User { return $this->hasPassword() && password_verify($password, $this->password); } public function passwordNeedsRehash(): bool { - return password_needs_rehash($this->password, MSZ_USERS_PASSWORD_HASH_ALGO); + return password_needs_rehash($this->password, self::PASSWORD_ALGO); } public function setPassword(string $password): void { - if(($userId = $this->getId()) < 1) - return; - DB::prepare('UPDATE `msz_users` SET `password` = :password WHERE `user_id` = :user_id') - ->bind('password', password_hash($password, MSZ_USERS_PASSWORD_HASH_ALGO)) - ->bind('user_id', $userId) + ->bind('password', $this->password = self::hashPassword($password)) + ->bind('user_id', $this->getId()) ->execute(); } @@ -180,6 +209,79 @@ class User { return self::$localUser !== null; } + public static function validateUsername(string $name): string { + if($name !== trim($name)) + return 'trim'; + + $length = mb_strlen($name); + if($length < self::NAME_MIN_LENGTH) + return 'short'; + if($length > self::NAME_MAX_LENGTH) + return 'long'; + + if(!preg_match('#^' . self::NAME_REGEX . '$#u', $name)) + return 'invalid'; + + $userId = (int)DB::prepare( + 'SELECT `user_id`' + . ' FROM `' . DB::PREFIX . self::TABLE . '`' + . ' WHERE LOWER(`username`) = LOWER(:username)' + ) ->bind('username', $name) + ->fetchColumn(); + if($userId > 0) + return 'in-use'; + + return ''; + } + + public static function usernameValidationErrorString(string $error): string { + switch($error) { + case 'trim': + return 'Your username may not start or end with spaces!'; + case 'short': + return sprintf('Your username is too short, it has to be at least %d characters!', self::NAME_MIN_LENGTH); + case 'long': + return sprintf("Your username is too long, it can't be longer than %d characters!", self::NAME_MAX_LENGTH); + case 'invalid': + return 'Your username contains invalid characters.'; + case 'in-use': + return 'This username is already taken!'; + case '': + return 'This username is correctly formatted!'; + default: + return 'This username is incorrectly formatted.'; + } + } + + public static function validateEMailAddress(string $address): string { + if(filter_var($address, FILTER_VALIDATE_EMAIL) === false) + return 'format'; + if(!checkdnsrr(mb_substr(mb_strstr($address, '@'), 1), 'MX')) + return 'dns'; + + $userId = (int)DB::prepare( + 'SELECT `user_id`' + . ' FROM `' . DB::PREFIX . self::TABLE . '`' + . ' WHERE LOWER(`email`) = LOWER(:email)' + ) ->bind('email', $address) + ->fetchColumn(); + if($userId > 0) + return 'in-use'; + + return ''; + } + + public static function validatePassword(string $password): string { + if(unique_chars($password) < self::PASSWORD_UNIQUE) + return 'weak'; + + return ''; + } + + public static function hashPassword(string $password): string { + return password_hash($password, self::PASSWORD_ALGO); + } + public static function create( string $username, string $password, @@ -196,7 +298,7 @@ class User { ) ') ->bind('username', $username)->bind('email', $email) ->bind('register_ip', $ipAddress)->bind('last_ip', $ipAddress) - ->bind('password', user_password_hash($password)) + ->bind('password', self::hashPassword($password)) ->bind('user_country', IPAddress::country($ipAddress)) ->executeGetId(); @@ -217,7 +319,20 @@ class User { return self::getMemoizer()->find($userId, function() use ($userId) { $user = DB::prepare(self::USER_SELECT . 'WHERE `user_id` = :user_id') ->bind('user_id', $userId) - ->fetchObject(User::class); + ->fetchObject(self::class); + if(!$user) + throw new UserNotFoundException; + return $user; + }); + } + public static function byEMailAddress(string $address): ?self { + $address = mb_strtolower($address); + return self::getMemoizer()->find(function($user) use ($address) { + return $user->getEmailAddress() === $address; + }, function() use ($address) { + $user = DB::prepare(self::USER_SELECT . 'WHERE LOWER(`email`) = :email') + ->bind('email', $address) + ->fetchObject(self::class); if(!$user) throw new UserNotFoundException; return $user; @@ -232,7 +347,7 @@ class User { $user = DB::prepare(self::USER_SELECT . 'WHERE LOWER(`email`) = LOWER(:email) OR LOWER(`username`) = LOWER(:username)') ->bind('email', $usernameOrEmail) ->bind('username', $usernameOrEmail) - ->fetchObject(User::class); + ->fetchObject(self::class); if(!$user) throw new UserNotFoundException; return $user; @@ -246,7 +361,7 @@ class User { $user = DB::prepare(self::USER_SELECT . 'WHERE `user_id` = :user_id OR LOWER(`username`) = LOWER(:username)') ->bind('user_id', (int)$userIdOrName) ->bind('username', (string)$userIdOrName) - ->fetchObject(User::class); + ->fetchObject(self::class); if(!$user) throw new UserNotFoundException; return $user; diff --git a/src/Users/user_legacy.php b/src/Users/user_legacy.php index b8bb6af2..8b6d87ea 100644 --- a/src/Users/user_legacy.php +++ b/src/Users/user_legacy.php @@ -3,43 +3,6 @@ // Never ever EVER use it for ANYTHING other than determining display colours, there's a small chance that it might not be accurate. // And even if it were, roles properties are aggregated and thus must all be accounted for. -define( - 'MSZ_USERS_PASSWORD_HASH_ALGO', - defined('PASSWORD_ARGON2ID') - ? PASSWORD_ARGON2ID - : ( - defined('PASSWORD_ARGON2I') - ? PASSWORD_ARGON2I - : PASSWORD_BCRYPT - ) -); - -function user_find_for_reset(string $email): array { - $getUser = \Misuzu\DB::prepare(' - SELECT `user_id`, `username`, `email` - FROM `msz_users` - WHERE LOWER(`email`) = LOWER(:email) - AND `user_deleted` IS NULL - '); - $getUser->bind('email', $email); - return $getUser->fetch(); -} - -function user_password_hash(string $password): string { - return password_hash($password, MSZ_USERS_PASSWORD_HASH_ALGO); -} - -function user_password_set(int $userId, string $password): bool { - $updatePassword = \Misuzu\DB::prepare(' - UPDATE `msz_users` - SET `password` = :password - WHERE `user_id` = :user - '); - $updatePassword->bind('user', $userId); - $updatePassword->bind('password', user_password_hash($password)); - return $updatePassword->execute(); -} - function user_totp_info(int $userId): array { if($userId < 1) return []; @@ -72,19 +35,6 @@ function user_totp_update(int $userId, ?string $key): void { $updateTotpKey->execute(); } -function user_email_get(int $userId): string { - if($userId < 1) - return ''; - - $fetchMail = \Misuzu\DB::prepare(' - SELECT `email` - FROM `msz_users` - WHERE `user_id` = :user_id - '); - $fetchMail->bind('user_id', $userId); - return (string)$fetchMail->fetchColumn(0, ''); -} - function user_email_set(int $userId, string $email): bool { $updateMail = \Misuzu\DB::prepare(' UPDATE `msz_users` diff --git a/src/Users/validation.php b/src/Users/validation.php deleted file mode 100644 index e6f6573e..00000000 --- a/src/Users/validation.php +++ /dev/null @@ -1,96 +0,0 @@ - MSZ_USERNAME_MAX_LENGTH) { - return 'long'; - } - - if(!preg_match(MSZ_USERNAME_REGEX_FULL, $username)) { - return 'invalid'; - } - - if($checkInUse) { - $getUser = \Misuzu\DB::prepare(' - SELECT COUNT(`user_id`) - FROM `msz_users` - WHERE LOWER(`username`) = LOWER(:username) - '); - $getUser->bind('username', $username); - $userId = $getUser->fetchColumn(0, 0); - - if($userId > 0) { - return 'in-use'; - } - } - - return ''; -} - -function user_validate_check_mx_record(string $email): bool { - $domain = mb_substr(mb_strstr($email, '@'), 1); - return checkdnsrr($domain, 'MX') || checkdnsrr($domain, 'A'); -} - -function user_validate_email(string $email, bool $checkInUse = false): string { - if(filter_var($email, FILTER_VALIDATE_EMAIL) === false) { - return 'format'; - } - - if(!user_validate_check_mx_record($email)) { - return 'dns'; - } - - if($checkInUse) { - $getUser = \Misuzu\DB::prepare(' - SELECT COUNT(`user_id`) - FROM `msz_users` - WHERE LOWER(`email`) = LOWER(:email) - '); - $getUser->bind('email', $email); - $userId = $getUser->fetchColumn(0, 0); - - if($userId > 0) { - return 'in-use'; - } - } - - return ''; -} - -function user_validate_password(string $password): string { - if(unique_chars($password) < MSZ_PASSWORD_MIN_UNIQUE) { - return 'weak'; - } - - return ''; -} - -define('MSZ_USER_USERNAME_VALIDATION_STRINGS', [ - 'trim' => 'Your username may not start or end with spaces!', - 'short' => sprintf('Your username is too short, it has to be at least %d characters!', MSZ_USERNAME_MIN_LENGTH), - 'long' => sprintf("Your username is too long, it can't be longer than %d characters!", MSZ_USERNAME_MAX_LENGTH), - 'invalid' => 'Your username contains invalid characters.', - 'in-use' => 'This username is already taken!', -]); diff --git a/templates/auth/password_reset.twig b/templates/auth/password_reset.twig index c51adbb6..b22ce287 100644 --- a/templates/auth/password_reset.twig +++ b/templates/auth/password_reset.twig @@ -6,9 +6,9 @@ {% block content %}
- {{ container_title(' Resetting password for ' ~ password_username) }} + {{ container_title(' Resetting password for ' ~ password_user.username) }} - {{ input_hidden('reset[user]', password_user_id) }} + {{ input_hidden('reset[user]', password_user.id) }} {{ input_csrf() }} {% if password_notices|length > 0 %}