Merge pull request #10277 from very-ape/authenticate-hook

Bug fix: allow authentication addons to create users again.

src/Model/User.php

@@ -515,7 +515,27 @@ 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) {
+			$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 (empty($username) || is_numeric($username)) {
+				throw $e;
+			}
+
+			return self::getIdFromAuthenticateHooks($username, $password);
+		}
 
 		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 +565,40 @@ class User
 
 			return $user['uid'];
 		} else {
-			$addon_auth = [
+			return self::getIdFromAuthenticateHooks($user['nickname'], $password); // throws
-				'username'      => $user['nickname'],
+		}
-				'password'      => $password,
-				'authenticated' => 0,
-				'user_record'   => null
-			];
 
-			/*
+		throw new HTTPException\ForbiddenException(DI::l10n()->t('Login failed'));
-			 * 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'];
+	 * 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 $addon_auth['user_record']['uid'];
 		}
 
 		throw new HTTPException\ForbiddenException(DI::l10n()->t('Login failed'));