Apply suggestions from code review

Also clean up some code, make it less needlessly verbose.

Co-authored-by: Hypolite Petovan <hypolite@mrpetovan.com>

diff --git a/src/Model/User.php b/src/Model/User.php
index 8a9f0a93042737cb1efa63952c10f1fbb05b4421..a0a40069f9a2c76308671da9b882b702363174eb 100644 (file)
--- 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
+                       $username = (is_string($user_info) ? $user_info : $user_info['nickname'] ?? '');
+
+                       // 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 (is_numeric($user_info) || is_numeric($user_info['nickname'] ?? '')) {
+                       if (empty($username) || is_numeric($user_info) || is_numeric($user_info['nickname'] ?? '')) {
                                throw $e;
                        }
 
-                       $username = (is_string($user_info) ? $user_info : $user_info['nickname'] ?? '');
-
-                       if (!$username) {
-                               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,