From b4e362cf4840345b3ea37b7cbf1804329b6c026d Mon Sep 17 00:00:00 2001 From: James Mortemore Date: Tue, 30 Dec 2025 18:12:08 +0000 Subject: [PATCH] fix: generate new pins if old expired fixes #18 --- .../common/commands/PinCommand.java | 9 ++ .../common/storage/PlayerPinStorage.java | 62 ++++++++-- common/src/main/resources/messages.yml | 2 +- e2e/tests/src/pin-cache.test.ts | 116 ++++++++---------- e2e/tests/src/pin-command.test.ts | 78 ++++++++---- e2e/tests/src/pin-storage.test.ts | 41 ++++--- 6 files changed, 193 insertions(+), 115 deletions(-) diff --git a/common/src/main/java/me/confuser/banmanager/webenhancer/common/commands/PinCommand.java b/common/src/main/java/me/confuser/banmanager/webenhancer/common/commands/PinCommand.java index 9ce2a27..16944c7 100644 --- a/common/src/main/java/me/confuser/banmanager/webenhancer/common/commands/PinCommand.java +++ b/common/src/main/java/me/confuser/banmanager/webenhancer/common/commands/PinCommand.java @@ -27,6 +27,15 @@ public boolean onCommand(CommonSender sender, CommandParser parser) { if (sender.isConsole()) return false; if (parser.getArgs().length != 0) return false; + // Check rate limit before async work + if (plugin.getPlayerPinStorage().isRateLimited(sender.getData().getUUID())) { + long remaining = plugin.getPlayerPinStorage().getRateLimitRemaining(sender.getData().getUUID()); + Message.get("pin.rateLimited") + .set("seconds", String.valueOf(remaining)) + .sendTo(sender); + return true; + } + getPlugin().getScheduler().runAsync(() -> { PlayerData player = null; diff --git a/common/src/main/java/me/confuser/banmanager/webenhancer/common/storage/PlayerPinStorage.java b/common/src/main/java/me/confuser/banmanager/webenhancer/common/storage/PlayerPinStorage.java index cb04868..8436860 100644 --- a/common/src/main/java/me/confuser/banmanager/webenhancer/common/storage/PlayerPinStorage.java +++ b/common/src/main/java/me/confuser/banmanager/webenhancer/common/storage/PlayerPinStorage.java @@ -2,8 +2,8 @@ import me.confuser.banmanager.webenhancer.common.google.guava.cache.Cache; import me.confuser.banmanager.webenhancer.common.google.guava.cache.CacheBuilder; -import me.confuser.banmanager.common.data.PlayerWarnData; import me.confuser.banmanager.common.ormlite.dao.BaseDaoImpl; +import me.confuser.banmanager.common.ormlite.stmt.DeleteBuilder; import me.confuser.banmanager.common.ormlite.support.ConnectionSource; import me.confuser.banmanager.common.ormlite.table.DatabaseTableConfig; import me.confuser.banmanager.common.ormlite.table.TableUtils; @@ -17,9 +17,10 @@ import java.util.concurrent.TimeUnit; public class PlayerPinStorage extends BaseDaoImpl { - private Cache pins = CacheBuilder.newBuilder() - .expireAfterWrite(5, TimeUnit.MINUTES) - .concurrencyLevel(2) + private static final int RATE_LIMIT_SECONDS = 30; + + private Cache rateLimitCache = CacheBuilder.newBuilder() + .expireAfterWrite(RATE_LIMIT_SECONDS, TimeUnit.SECONDS) .maximumSize(200) .build(); @@ -38,14 +39,37 @@ public PlayerPinStorage(ConnectionSource connection) throws SQLException { } } - public PlayerPinData generate(PlayerData player) { + /** + * Checks if a player is rate limited from generating a new pin. + * + * @param playerId the player's UUID + * @return true if the player must wait before generating a new pin + */ + public boolean isRateLimited(UUID playerId) { + return rateLimitCache.getIfPresent(playerId) != null; + } + + /** + * Gets the number of seconds remaining until rate limit expires. + * + * @param playerId the player's UUID + * @return seconds remaining, or 0 if not rate limited + */ + public long getRateLimitRemaining(UUID playerId) { + Long lastGenerated = rateLimitCache.getIfPresent(playerId); + if (lastGenerated == null) { + return 0; + } + long elapsed = (System.currentTimeMillis() - lastGenerated) / 1000; + return Math.max(0, RATE_LIMIT_SECONDS - elapsed); + } + + private PlayerPinData generate(PlayerData player) { PlayerPinData pin = null; try { pin = new PlayerPinData(player); if (create(pin) != 1) { pin = null; - } else { - pins.put(player.getUUID(), pin); } } catch (NoSuchAlgorithmException | SQLException e) { e.printStackTrace(); @@ -54,11 +78,29 @@ public PlayerPinData generate(PlayerData player) { return pin; } + /** + * Gets a valid pin for the player, always generating a fresh one. + * Any existing pins for this player are deleted first to prevent stale pins. + * Rate limiting should be checked via isRateLimited() before calling this method. + * + * @param player the player to generate a pin for + * @return the newly generated pin, or null if generation failed + */ public PlayerPinData getValidPin(PlayerData player) { - PlayerPinData pin = pins.getIfPresent(player.getUUID()); + try { + DeleteBuilder deleteBuilder = deleteBuilder(); + deleteBuilder.where().eq("player_id", player.getId()); + deleteBuilder.delete(); + } catch (SQLException e) { + e.printStackTrace(); + } + + // Generate fresh pin + PlayerPinData pin = generate(player); - if (pin == null) { - pin = generate(player); + // Update rate limit cache on successful generation + if (pin != null) { + rateLimitCache.put(player.getUUID(), System.currentTimeMillis()); } return pin; diff --git a/common/src/main/resources/messages.yml b/common/src/main/resources/messages.yml index 5738200..023dec2 100644 --- a/common/src/main/resources/messages.yml +++ b/common/src/main/resources/messages.yml @@ -2,4 +2,4 @@ messages: pin: notify: '&6Your pin expires in [expires]' pin: '[pin]' - + rateLimited: '&cPlease wait [seconds] seconds before generating a new pin' diff --git a/e2e/tests/src/pin-cache.test.ts b/e2e/tests/src/pin-cache.test.ts index ddf8f66..e512f0b 100644 --- a/e2e/tests/src/pin-cache.test.ts +++ b/e2e/tests/src/pin-cache.test.ts @@ -5,45 +5,36 @@ import { opPlayer, sleep, getPlayerPinCount, - getAllPlayerPins, clearPlayerPins, closeDatabase } from './helpers' -describe('Pin Cache Behavior', () => { +describe('Pin Rate Limiting Behavior', () => { let playerBot: TestBot - beforeAll(async () => { - playerBot = await createBot('PinCachePlayer') - await sleep(2000) - await opPlayer('PinCachePlayer') - await sleep(500) - await clearPlayerPins() - }) - afterAll(async () => { await playerBot?.disconnect() await disconnectRcon() await closeDatabase() }) - beforeEach(async () => { - await clearPlayerPins() + test('rate limit prevents rapid pin generation', async () => { + playerBot = await createBot('RateLimitTest') + await sleep(2000) + await opPlayer('RateLimitTest') await sleep(500) - }) - test('same pin is returned within 5-minute cache window', async () => { - // First /bmpin call + // First /bmpin call - should succeed playerBot.clearSystemMessages() await playerBot.sendChat('/bmpin') - // Wait for the expiry message first + // Wait for the expiry message (indicates success) const firstResponse = await playerBot.waitForSystemMessage('expires', 10000) console.log(`First call - Received: ${firstResponse.message}`) await sleep(500) - // Get all system messages and find the 6-digit pin + // Get the first pin const firstMessages = playerBot.getSystemMessages() let actualFirstPin: string | undefined for (const msg of firstMessages) { @@ -54,39 +45,34 @@ describe('Pin Cache Behavior', () => { } } console.log(`First call - Actual pin: ${actualFirstPin ?? 'not found'}`) + expect(actualFirstPin).toBeDefined() await sleep(1000) - // Second /bmpin call (should return cached pin) + // Second /bmpin call immediately - should be rate limited playerBot.clearSystemMessages() await playerBot.sendChat('/bmpin') - const secondResponse = await playerBot.waitForSystemMessage('expires', 10000) - console.log(`Second call - Received: ${secondResponse.message}`) + // Should receive rate limit message + const rateLimitResponse = await playerBot.waitForSystemMessage('wait', 10000) + console.log(`Second call - Rate limit message: ${rateLimitResponse.message}`) - await sleep(500) + expect(rateLimitResponse.message).toContain('wait') + expect(rateLimitResponse.message).toContain('seconds') - // Get all system messages and find the 6-digit pin - const secondMessages = playerBot.getSystemMessages() - let actualSecondPin: string | undefined - for (const msg of secondMessages) { - const match = msg.message.match(/^\d{6}$/) - if (match != null) { - actualSecondPin = match[0] - break - } - } - console.log(`Second call - Actual pin: ${actualSecondPin ?? 'not found'}`) - - // The pins should be the same (cached) - if (actualFirstPin != null && actualSecondPin != null) { - expect(actualFirstPin).toBe(actualSecondPin) - console.log(`Pins match as expected (cache working): ${actualFirstPin}`) - } + await playerBot.disconnect() }, 30000) test('pin is stored in database on first request', async () => { - const initialCount = await getPlayerPinCount('PinCachePlayer') + // Use a unique player for this test (max 16 chars) + playerBot = await createBot('PinDbTest') + await sleep(2000) + await opPlayer('PinDbTest') + await sleep(500) + await clearPlayerPins() + await sleep(500) + + const initialCount = await getPlayerPinCount('PinDbTest') console.log(`Initial pin count: ${initialCount}`) playerBot.clearSystemMessages() @@ -94,44 +80,48 @@ describe('Pin Cache Behavior', () => { await playerBot.waitForSystemMessage('expires', 10000) await sleep(1000) - const newCount = await getPlayerPinCount('PinCachePlayer') - // Pin count should increase by at least 1 (could be more if previous test created one) - expect(newCount).toBeGreaterThanOrEqual(initialCount) + const newCount = await getPlayerPinCount('PinDbTest') + // Pin count should be exactly 1 (old pins deleted before new one created) + expect(newCount).toBe(1) console.log(`Pin count after /bmpin: ${newCount}`) - }, 20000) - test('only one pin is stored for multiple cached requests', async () => { - // Clear and wait for cache to be empty + await playerBot.disconnect() + }, 30000) + + test('new pin generation deletes old pins for same player', async () => { + // Use a unique player for this test (max 16 chars) + playerBot = await createBot('PinDelTest') + await sleep(2000) + await opPlayer('PinDelTest') + await sleep(500) await clearPlayerPins() await sleep(500) - const initialCount = await getPlayerPinCount('PinCachePlayer') - console.log(`Initial pin count after clear: ${initialCount}`) - // First call creates a pin playerBot.clearSystemMessages() await playerBot.sendChat('/bmpin') await playerBot.waitForSystemMessage('expires', 10000) await sleep(500) - const afterFirstCall = await getPlayerPinCount('PinCachePlayer') + const afterFirstCall = await getPlayerPinCount('PinDelTest') console.log(`Pin count after first call: ${afterFirstCall}`) + expect(afterFirstCall).toBe(1) - // Make two more /bmpin calls (should use cache) - for (let i = 0; i < 2; i++) { - playerBot.clearSystemMessages() - await playerBot.sendChat('/bmpin') - await playerBot.waitForSystemMessage('expires', 10000) - await sleep(500) - } + // Wait for rate limit to expire (30 seconds) + console.log('Waiting 31 seconds for rate limit to expire...') + await sleep(31000) - await sleep(1000) + // Second call should delete old pin and create new one + playerBot.clearSystemMessages() + await playerBot.sendChat('/bmpin') + await playerBot.waitForSystemMessage('expires', 10000) + await sleep(500) - // Should still have the same count (cached, not generating new ones) - const finalCount = await getPlayerPinCount('PinCachePlayer') - console.log(`Final pin count: ${finalCount}`) + // Should still have only 1 pin (old one was deleted) + const finalCount = await getPlayerPinCount('PinDelTest') + console.log(`Pin count after second call: ${finalCount}`) + expect(finalCount).toBe(1) - // Final count should equal the count after first call (no new pins generated) - expect(finalCount).toBe(afterFirstCall) - }, 30000) + await playerBot.disconnect() + }, 70000) }) diff --git a/e2e/tests/src/pin-command.test.ts b/e2e/tests/src/pin-command.test.ts index 883126c..ca74ad4 100644 --- a/e2e/tests/src/pin-command.test.ts +++ b/e2e/tests/src/pin-command.test.ts @@ -1,49 +1,77 @@ -import { TestBot, createBot, sendCommand, disconnectRcon, opPlayer, sleep } from './helpers' +import { TestBot, createBot, disconnectRcon, opPlayer, sleep, clearPlayerPins } from './helpers' describe('Pin Command', () => { let playerBot: TestBot - beforeAll(async () => { - playerBot = await createBot('PinTestPlayer') - await sleep(2000) - await opPlayer('PinTestPlayer') - await sleep(500) - }) - afterAll(async () => { await playerBot?.disconnect() await disconnectRcon() }) test('bmpin command returns a 6-digit pin', async () => { + // Use a unique player for this test + playerBot = await createBot('PinCmdPlayer1') + await sleep(2000) + await opPlayer('PinCmdPlayer1') + await sleep(500) + playerBot.clearSystemMessages() await playerBot.sendChat('/bmpin') - const response = await playerBot.waitForSystemMessage('pin expires', 10000) - expect(response.message).toMatch(/\d{6}|expires/) + const response = await playerBot.waitForSystemMessage('expires', 10000) + expect(response.message).toMatch(/expires/) console.log(`Received pin message: ${response.message}`) - }) - test('bmpin command generates a new pin each time', async () => { + // Also verify we got a 6-digit pin + const allMessages = playerBot.getSystemMessages() + const pinMessage = allMessages.find(m => /^\d{6}$/.test(m.message)) + expect(pinMessage).toBeDefined() + console.log(`Pin: ${pinMessage?.message}`) + + await playerBot.disconnect() + }, 20000) + + test('bmpin command generates a new pin after rate limit expires', async () => { + // Use a unique player for this test + playerBot = await createBot('PinCmdPlayer2') + await sleep(2000) + await opPlayer('PinCmdPlayer2') + await sleep(500) + await clearPlayerPins() + await sleep(500) + + // First pin request playerBot.clearSystemMessages() await playerBot.sendChat('/bmpin') - const firstResponse = await playerBot.waitForSystemMessage('pin', 10000) - const firstPin = firstResponse.message.match(/\d{6}/)?.[0] + await playerBot.waitForSystemMessage('expires', 10000) + + const firstMessages = playerBot.getSystemMessages() + const firstPin = firstMessages.find(m => /^\d{6}$/.test(m.message))?.message - await sleep(1000) + expect(firstPin).toBeDefined() + expect(firstPin).toMatch(/^\d{6}$/) + console.log(`First pin: ${firstPin}`) + // Wait for rate limit to expire (30 seconds) + console.log('Waiting 31 seconds for rate limit to expire...') + await sleep(31000) + + // Second pin request playerBot.clearSystemMessages() await playerBot.sendChat('/bmpin') - const secondResponse = await playerBot.waitForSystemMessage('pin', 10000) - const secondPin = secondResponse.message.match(/\d{6}/)?.[0] + await playerBot.waitForSystemMessage('expires', 10000) - if (firstPin != null) { - expect(firstPin).toMatch(/^\d{6}$/) - } - if (secondPin != null) { - expect(secondPin).toMatch(/^\d{6}$/) - } + const secondMessages = playerBot.getSystemMessages() + const secondPin = secondMessages.find(m => /^\d{6}$/.test(m.message))?.message - console.log(`First pin: ${firstPin ?? 'not found'}, Second pin: ${secondPin ?? 'not found'}`) - }) + expect(secondPin).toBeDefined() + expect(secondPin).toMatch(/^\d{6}$/) + console.log(`Second pin: ${secondPin}`) + + // Pins should be different (fresh each time) + expect(secondPin).not.toBe(firstPin) + console.log(`First pin: ${firstPin}, Second pin: ${secondPin} - different as expected`) + + await playerBot.disconnect() + }, 70000) }) diff --git a/e2e/tests/src/pin-storage.test.ts b/e2e/tests/src/pin-storage.test.ts index 941f454..bdd545d 100644 --- a/e2e/tests/src/pin-storage.test.ts +++ b/e2e/tests/src/pin-storage.test.ts @@ -12,13 +12,6 @@ import { describe('Pin Storage', () => { let playerBot: TestBot - beforeAll(async () => { - playerBot = await createBot('PinStoragePlayer') - await sleep(2000) - await opPlayer('PinStoragePlayer') - await sleep(500) - }) - afterAll(async () => { await playerBot?.disconnect() await disconnectRcon() @@ -26,36 +19,52 @@ describe('Pin Storage', () => { }) test('pin is stored in database after /bmpin command', async () => { - const initialCount = await getPlayerPinCount('PinStoragePlayer') + // Use a unique player for this test (max 16 chars) + playerBot = await createBot('PinStoreTest1') + await sleep(2000) + await opPlayer('PinStoreTest1') + await sleep(500) + + const initialCount = await getPlayerPinCount('PinStoreTest1') playerBot.clearSystemMessages() await playerBot.sendChat('/bmpin') - const response = await playerBot.waitForSystemMessage('pin', 10000) + const response = await playerBot.waitForSystemMessage('expires', 10000) console.log(`Received pin message: ${response.message}`) await sleep(1000) - const newCount = await getPlayerPinCount('PinStoragePlayer') - expect(newCount).toBeGreaterThan(initialCount) - console.log(`Pin count increased from ${initialCount} to ${newCount}`) + const newCount = await getPlayerPinCount('PinStoreTest1') + expect(newCount).toBeGreaterThanOrEqual(1) + console.log(`Pin count: ${newCount}`) - const latestPin = await getLatestPlayerPin('PinStoragePlayer') + const latestPin = await getLatestPlayerPin('PinStoreTest1') expect(latestPin).not.toBeNull() expect(latestPin?.expires).toBeGreaterThan(Date.now() / 1000) console.log(`Pin expires at ${new Date((latestPin?.expires ?? 0) * 1000)}`) + + await playerBot.disconnect() }, 20000) test('pin hash is stored securely with argon2', async () => { + // Use a unique player for this test (max 16 chars) + playerBot = await createBot('PinStoreTest2') + await sleep(2000) + await opPlayer('PinStoreTest2') + await sleep(500) + playerBot.clearSystemMessages() await playerBot.sendChat('/bmpin') - await playerBot.waitForSystemMessage('pin', 10000) + await playerBot.waitForSystemMessage('expires', 10000) await sleep(1000) - const latestPin = await getLatestPlayerPin('PinStoragePlayer') + const latestPin = await getLatestPlayerPin('PinStoreTest2') expect(latestPin).not.toBeNull() expect(latestPin?.pin).toContain('$argon2') console.log(`Pin is hashed with argon2: ${latestPin?.pin.substring(0, 30)}...`) - }, 15000) + + await playerBot.disconnect() + }, 20000) })