Fix oidc auto-register not cleaning up new user on errors #4563

server/auth/OidcAuthStrategy.js

@@ -110,6 +110,8 @@ class OidcAuthStrategy {
    * @param {Function} done - Passport callback
    */
   async verifyCallback(tokenset, userinfo, done) {
+    let isNewUser = false
+    let user = null
     try {
       Logger.debug(`[OidcAuth] openid callback userinfo=`, JSON.stringify(userinfo, null, 2))
 
@@ -121,9 +123,24 @@ class OidcAuthStrategy {
         throw new Error(`Group claim ${Database.serverSettings.authOpenIDGroupClaim} not found or empty in userinfo`)
       }
 
-      let user = await Database.userModel.findOrCreateUserFromOpenIdUserInfo(userinfo)
+      user = await Database.userModel.findUserFromOpenIdUserInfo(userinfo)
 
-      if (!user?.isActive) {
+      if (user?.error) {
+        throw new Error('Invalid userinfo or already linked')
+      }
+
+      if (!user) {
+        // If no existing user was matched, auto-register if configured
+        if (global.ServerSettings.authOpenIDAutoRegister) {
+          Logger.info(`[User] openid: Auto-registering user with sub "${userinfo.sub}"`, userinfo)
+          user = await Database.userModel.createUserFromOpenIdUserInfo(userinfo)
+          isNewUser = true
+        } else {
+          Logger.warn(`[User] openid: User not found and auto-register is disabled`)
+        }
+      }
+
+      if (!user.isActive) {
         throw new Error('User not active or not found')
       }
 
@@ -136,6 +153,10 @@ class OidcAuthStrategy {
       return done(null, user)
     } catch (error) {
       Logger.error(`[OidcAuth] openid callback error: ${error?.message}\n${error?.stack}`)
+      // Remove new user if an error occurs
+      if (isNewUser && user) {
+        await user.destroy()
+      }
       return done(null, null, 'Unauthorized')
     }
   }

server/models/User.js

@@ -211,18 +211,18 @@ class User extends Model {
   }
 
   /**
-   * Finds an existing user by OpenID subject identifier, or by email/username based on server settings,
+   * Finds an existing user by OpenID subject identifier, or by email/username based on server settings
-   * or creates a new user if configured to do so.
+   * Returns null if no user is found
    *
    * @param {Object} userinfo
-   * @returns {Promise<User>}
+   * @returns {Promise<User|{error: string}>}
    */
-  static async findOrCreateUserFromOpenIdUserInfo(userinfo) {
+  static async findUserFromOpenIdUserInfo(userinfo) {
     let user = await this.getUserByOpenIDSub(userinfo.sub)
 
     // Matched by sub
     if (user) {
-      Logger.debug(`[User] openid: User found by sub`)
+      Logger.debug(`[User] openid: User found by sub "${userinfo.sub}"`)
       return user
     }
 
@@ -232,20 +232,27 @@ class User extends Model {
         // Only disallow when email_verified explicitly set to false (allow both if not set or true)
         if (userinfo.email_verified === false) {
           Logger.warn(`[User] openid: User not found and email "${userinfo.email}" is not verified`)
-          return null
+          return {
+            error: 'Email not verified'
+          }
         } else {
           Logger.info(`[User] openid: User not found, checking existing with email "${userinfo.email}"`)
           user = await this.getUserByEmail(userinfo.email)
 
           if (user?.authOpenIDSub) {
             Logger.warn(`[User] openid: User found with email "${userinfo.email}" but is already matched with sub "${user.authOpenIDSub}"`)
-            return null // User is linked to a different OpenID subject; do not proceed.
+            // User is linked to a different OpenID subject; do not proceed.
+            return {
+              error: 'User already linked to a different OpenID subject'
+            }
           }
         }
       } else {
         Logger.warn(`[User] openid: User not found and no email in userinfo`)
         // We deny login, because if the admin whishes to match email, it makes sense to require it
-        return null
+        return {
+          error: 'No email in userinfo'
+        }
       }
     }
     // Match existing user by username
@@ -260,43 +267,40 @@ class User extends Model {
         username = userinfo.username
       } else {
         Logger.warn(`[User] openid: User not found and neither preferred_username nor username in userinfo`)
-        return null
+        return {
+          error: 'No username in userinfo'
+        }
       }
 
       user = await this.getUserByUsername(username)
 
       if (user?.authOpenIDSub) {
         Logger.warn(`[User] openid: User found with username "${username}" but is already matched with sub "${user.authOpenIDSub}"`)
-        return null // User is linked to a different OpenID subject; do not proceed.
+        // User is linked to a different OpenID subject; do not proceed.
+        return {
+          error: 'User already linked to a different OpenID subject'
+        }
       }
     }
 
+    if (!user) {
+      return null
+    }
+
     // Found existing user via email or username
-    if (user) {
+    if (!user.isActive) {
-      if (!user.isActive) {
+      Logger.warn(`[User] openid: User found but is not active`)
-        Logger.warn(`[User] openid: User found but is not active`)
-        return null
-      }
-
-      // Update user with OpenID sub
-      if (!user.extraData) user.extraData = {}
-      user.extraData.authOpenIDSub = userinfo.sub
-      user.changed('extraData', true)
-      await user.save()
-
-      Logger.debug(`[User] openid: User found by email/username`)
       return user
     }
 
-    // If no existing user was matched, auto-register if configured
+    // Update user with OpenID sub
-    if (global.ServerSettings.authOpenIDAutoRegister) {
+    if (!user.extraData) user.extraData = {}
-      Logger.info(`[User] openid: Auto-registering user with sub "${userinfo.sub}"`, userinfo)
+    user.extraData.authOpenIDSub = userinfo.sub
-      user = await this.createUserFromOpenIdUserInfo(userinfo)
+    user.changed('extraData', true)
-      return user
+    await user.save()
-    }
 
-    Logger.warn(`[User] openid: User not found and auto-register is disabled`)
+    Logger.debug(`[User] openid: User found by email/username`)
-    return null
+    return user
   }
 
   /**