From 55739d420383326ed435386c9d0d19a46613028a Mon Sep 17 00:00:00 2001 From: Giuseppe Perrotta Date: Sun, 16 Aug 2026 20:36:35 +0200 Subject: [PATCH] fix: skip premium username migration when the username already matches Concurrent logins of a renamed premium account all read the pre-rename state, so every connection after the first asked UserService to migrate a row another connection had already migrated. migrateUsernameNoTx rejects that with IllegalArgumentException, which escaped the PreLoginEvent handler and left the account stuck in a kick loop. --- CHANGELOG.md | 1 + .../username/UsernameResolutionService.kt | 23 ++++-- ...ernameResolutionServiceIntegrationTests.kt | 79 +++++++++++++++++++ 3 files changed, 95 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49eb1da..17d23d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ ### Fixes - fix register command always using BCrypt instead of the selected algo - fix different letter case same username migration +- fix concurrent logins of a renamed premium account throwing out of the PreLoginEvent handler ### API ```diff diff --git a/navauth-common/src/main/kotlin/pl/spcode/navauth/common/application/auth/username/UsernameResolutionService.kt b/navauth-common/src/main/kotlin/pl/spcode/navauth/common/application/auth/username/UsernameResolutionService.kt index 6a0043e..5a65b88 100644 --- a/navauth-common/src/main/kotlin/pl/spcode/navauth/common/application/auth/username/UsernameResolutionService.kt +++ b/navauth-common/src/main/kotlin/pl/spcode/navauth/common/application/auth/username/UsernameResolutionService.kt @@ -41,15 +41,22 @@ constructor(private val userService: UserService, private val profileService: Pr if (existingUserIgnoreCase == null && isPremiumNickname) { val userByMojangUuid = userService.findUserByMojangUuid(correspondingPremiumProfile.uuid) if (userByMojangUuid != null) { - // user with the same mojang uuid exists, but with different nickname - try { - userService.migrateUsername(userByMojangUuid, correspondingPremiumProfile.name) - } catch (e: UsernameAlreadyTakenException) { - return failure( - UsernameResFailureReason.UsernameMigrationFailedUsernameAlreadyTaken( - correspondingPremiumProfile.name.value + // user with the same mojang uuid exists, but usually with a different nickname. + // The stored username can already be the profile one when a concurrent login for the same + // account committed the migration between the caller's lookup and this branch: then + // `existingUserIgnoreCase` is a stale null while the row is already migrated. There is + // nothing left to migrate, and asking UserService to do it anyway would throw + // IllegalArgumentException out of the login pipeline. + if (userByMojangUuid.username != correspondingPremiumProfile.name) { + try { + userService.migrateUsername(userByMojangUuid, correspondingPremiumProfile.name) + } catch (e: UsernameAlreadyTakenException) { + return failure( + UsernameResFailureReason.UsernameMigrationFailedUsernameAlreadyTaken( + correspondingPremiumProfile.name.value + ) ) - ) + } } if (connUsername != correspondingPremiumProfile.name) { return failure( diff --git a/navauth-common/src/test/kotlin/integration/auth/UsernameResolutionServiceIntegrationTests.kt b/navauth-common/src/test/kotlin/integration/auth/UsernameResolutionServiceIntegrationTests.kt index 0e886dd..02edee6 100644 --- a/navauth-common/src/test/kotlin/integration/auth/UsernameResolutionServiceIntegrationTests.kt +++ b/navauth-common/src/test/kotlin/integration/auth/UsernameResolutionServiceIntegrationTests.kt @@ -22,6 +22,11 @@ import com.google.inject.Inject import extension.app.UsernameResolutionTestExtension import fake.FakeProfileService import java.util.UUID +import java.util.concurrent.Callable +import java.util.concurrent.CountDownLatch +import java.util.concurrent.CyclicBarrier +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit import kotlin.test.assertEquals import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test @@ -249,6 +254,80 @@ class UsernameResolutionServiceIntegrationTests { ) } + @Test + fun `premium user already renamed by a concurrent login resolves without failing`() { + val mojangId = MojangId(UUID.randomUUID()) + val userUuid = UserUuid(UUID.randomUUID()) + val username = Username(generateRandomString(10)) + // State left behind by a login that already committed the rename. + userRepository.save(User.premium(userUuid, username, mojangId)) + fakeProfileService.addProfile(username, MojangProfile(mojangId, username)) + + // The caller looked the user up before that rename was committed, so it hands over a stale + // null while the row already carries the Mojang username. + val result = usernameResolutionService.resolveUsernameConflicts(username, null) + + assertEquals( + UsernameResResult.Success( + EncryptionType.ENFORCE_PREMIUM, + PostUsernameResolutionState.PREMIUM_USERNAME_MIGRATED, + ), + result, + ) + assertEquals(username, userRepository.findByUserUuid(userUuid)!!.username) + } + + @Test + fun `concurrent logins of a renamed premium user never fail the resolution`() { + val executor = Executors.newFixedThreadPool(2) + try { + repeat(32) { + val mojangId = MojangId(UUID.randomUUID()) + val userUuid = UserUuid(UUID.randomUUID()) + val oldUsername = Username(generateRandomString(10)) + val newUsername = Username(generateRandomString(10)) + userRepository.save(User.premium(userUuid, oldUsername, mojangId)) + fakeProfileService.addProfile(newUsername, MojangProfile(mojangId, newUsername)) + + // Two PreLoginEvents racing for the same account. Both read the pre-rename state, then + // the second one resolves after the first has committed the migration, so it works on a + // stale null while the row already carries the new username. The latch only makes that + // interleaving deterministic; on a live proxy it is what two reconnects milliseconds + // apart produce. + val bothLookedUp = CyclicBarrier(2) + val firstResolved = CountDownLatch(1) + val logins = + (1..2).map { login -> + executor.submit( + Callable { + val existingUser = userService.findUserByUsernameIgnoreCase(newUsername.value) + bothLookedUp.await(20, TimeUnit.SECONDS) + if (login == 2) check(firstResolved.await(20, TimeUnit.SECONDS)) + try { + usernameResolutionService.resolveUsernameConflicts(newUsername, existingUser) + } finally { + if (login == 1) firstResolved.countDown() + } + } + ) + } + + logins.forEach { login -> + assertEquals( + UsernameResResult.Success( + EncryptionType.ENFORCE_PREMIUM, + PostUsernameResolutionState.PREMIUM_USERNAME_MIGRATED, + ), + login.get(30, TimeUnit.SECONDS), + ) + } + assertEquals(newUsername, userRepository.findByUserUuid(userUuid)!!.username) + } + } finally { + executor.shutdownNow() + } + } + @Test fun `existing premium user with different case mojang username found by case-insensitive lookup migrates successfully`() { val mojangId = MojangId(UUID.randomUUID())