From d66f1e30ae04f37ba4e27124dec4b936d66baa85 Mon Sep 17 00:00:00 2001 From: very-ape <84299128+very-ape@users.noreply.github.com> Date: Thu, 20 May 2021 11:05:48 -0700 Subject: [PATCH] Apply suggestions from code review Also clean up some code, make it less needlessly verbose. Co-authored-by: Hypolite Petovan --- src/Model/User.php | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Model/User.php b/src/Model/User.php index 8a9f0a9304..a0a40069f9 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -523,19 +523,18 @@ class User try { $user = self::getAuthenticationInfo($user_info); } catch (Exception $e) { - // Addons can create users, and creating a numeric username would create - // abiguity with user IDs, possibly opening up an attack vector. - // So let's be very careful about that. - if (is_numeric($user_info) || is_numeric($user_info['nickname'] ?? '')) { - throw $e; - } - $username = (is_string($user_info) ? $user_info : $user_info['nickname'] ?? ''); - if (!$username) { + // Addons can create users, and since this 'catch' branch should only + // execute if getAuthenticationInfo can't find an existing user, that's + // exactly what will happen here. Creating a numeric username would create + // abiguity with user IDs, possibly opening up an attack vector. + // So let's be very careful about that. + if (empty($username) || is_numeric($user_info) || is_numeric($user_info['nickname'] ?? '')) { throw $e; } - return self::getIdFromAuthenticateHooks($user_info, $password); + + return self::getIdFromAuthenticateHooks($username, $password); } if ($third_party && DI::pConfig()->get($user['uid'], '2fa', 'verified')) { @@ -582,7 +581,8 @@ class User * @return int User Id if authentication is successful * @throws HTTPException\ForbiddenException */ - public static function getIdFromAuthenticateHooks($username, $password) { + public static function getIdFromAuthenticateHooks($username, $password) + { $addon_auth = [ 'username' => $username, 'password' => $password,