fix: generate new pins if old expired (#26)

confuser · web-flow · commit 8d521f52d0f4 · 2025-12-30T19:18:50.000+01:00
fixes #18
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
@@ -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
@@ -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<PlayerPinData, Integer> {
-  private Cache<UUID, PlayerPinData> pins = CacheBuilder.newBuilder()
-      .expireAfterWrite(5, TimeUnit.MINUTES)
-      .concurrencyLevel(2)
+  private static final int RATE_LIMIT_SECONDS = 30;
+
+  private Cache<UUID, Long> 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<PlayerPinData, Integer> 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
@@ -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
@@ -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,84 +45,83 @@ 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()
     await playerBot.sendChat('/bmpin')
     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
diff --git a/e2e/tests/src/pin-storage.test.ts b/e2e/tests/src/pin-storage.test.ts
