From 9ab57de3562a584519485579e85a45bedc9352c6 Mon Sep 17 00:00:00 2001 From: very-ape Date: Thu, 20 May 2021 00:16:08 -0700 Subject: [PATCH 1/4] Bug fix: allow authentication addons to create users again. --- src/Model/User.php | 61 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 16 deletions(-) diff --git a/src/Model/User.php b/src/Model/User.php index 3b11ee6ce..4b716f260 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -515,7 +515,20 @@ class User */ public static function getIdFromPasswordAuthentication($user_info, $password, $third_party = false) { - $user = self::getAuthenticationInfo($user_info); + // Addons registered with the "authenticate" hook may create the user on the + // fly. `getAuthenticationInfo` will fail if the user doesn't exist yet. If + // the user doesn't exist, we should give the addons a chance to create the + // user in our database, if applicable, before re-throwing the exception if + // they fail. + try { + $user = self::getAuthenticationInfo($user_info); + } catch (Exception $e) { + if (is_string($user_info)) { + return self::getIdFromAuthenticateHooks($user_info, $password); + } else { + throw $e; + } + } if ($third_party && DI::pConfig()->get($user['uid'], '2fa', 'verified')) { // Third-party apps can't verify two-factor authentication, we use app-specific passwords instead @@ -545,23 +558,39 @@ class User return $user['uid']; } else { - $addon_auth = [ - 'username' => $user['nickname'], - 'password' => $password, - 'authenticated' => 0, - 'user_record' => null - ]; + return self::getIdFromAuthenticateHooks($user['nickname'], $password); // throws + } - /* - * An addon indicates successful login by setting 'authenticated' to non-zero value and returning a user record - * Addons should never set 'authenticated' except to indicate success - as hooks may be chained - * and later addons should not interfere with an earlier one that succeeded. - */ - Hook::callAll('authenticate', $addon_auth); + throw new HTTPException\ForbiddenException(DI::l10n()->t('Login failed')); + } - if ($addon_auth['authenticated'] && $addon_auth['user_record']) { - return $user['uid']; - } + /** + * Try to obtain a user ID via "authenticate" hook addons + * + * Returns the user id associated with a successful password authentication + * + * @param string $username + * @param string $password + * @return int User Id if authentication is successful + * @throws HTTPException\ForbiddenException + */ + public static function getIdFromAuthenticateHooks($username, $password) { + $addon_auth = [ + 'username' => $username, + 'password' => $password, + 'authenticated' => 0, + 'user_record' => null + ]; + + /* + * An addon indicates successful login by setting 'authenticated' to non-zero value and returning a user record + * Addons should never set 'authenticated' except to indicate success - as hooks may be chained + * and later addons should not interfere with an earlier one that succeeded. + */ + Hook::callAll('authenticate', $addon_auth); + + if ($addon_auth['authenticated'] && $addon_auth['user_record']) { + return $user['uid']; } throw new HTTPException\ForbiddenException(DI::l10n()->t('Login failed')); From c89241dbd80b1b0503e239f8b468e6efbcb79802 Mon Sep 17 00:00:00 2001 From: very-ape Date: Thu, 20 May 2021 00:19:09 -0700 Subject: [PATCH 2/4] Bug fix: add missing changes. --- src/Model/User.php | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Model/User.php b/src/Model/User.php index 4b716f260..8a9f0a930 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -523,11 +523,19 @@ class User try { $user = self::getAuthenticationInfo($user_info); } catch (Exception $e) { - if (is_string($user_info)) { - return self::getIdFromAuthenticateHooks($user_info, $password); - } else { + // 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) { + throw $e; + } + return self::getIdFromAuthenticateHooks($user_info, $password); } if ($third_party && DI::pConfig()->get($user['uid'], '2fa', 'verified')) { @@ -590,7 +598,7 @@ class User Hook::callAll('authenticate', $addon_auth); if ($addon_auth['authenticated'] && $addon_auth['user_record']) { - return $user['uid']; + return $addon_auth['user_record']['uid']; } throw new HTTPException\ForbiddenException(DI::l10n()->t('Login failed')); 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 3/4] 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 8a9f0a930..a0a40069f 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, From e2d93b57da1b83bdde21f21aa478a9e319f230c4 Mon Sep 17 00:00:00 2001 From: very-ape <84299128+very-ape@users.noreply.github.com> Date: Thu, 20 May 2021 11:54:30 -0700 Subject: [PATCH 4/4] Update src/Model/User.php Co-authored-by: Hypolite Petovan --- src/Model/User.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Model/User.php b/src/Model/User.php index a0a40069f..38a22f70f 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -530,7 +530,7 @@ class User // 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'] ?? '')) { + if (empty($username) || is_numeric($username)) { throw $e; }