diff --git a/src/main/java/fr/xephi/authme/service/RecoveryCodeService.java b/src/main/java/fr/xephi/authme/service/RecoveryCodeService.java index e7fa37ad0..fdc987a21 100644 --- a/src/main/java/fr/xephi/authme/service/RecoveryCodeService.java +++ b/src/main/java/fr/xephi/authme/service/RecoveryCodeService.java @@ -1,38 +1,38 @@ package fr.xephi.authme.service; -import com.google.common.annotations.VisibleForTesting; +import fr.xephi.authme.initialization.HasCleanup; import fr.xephi.authme.initialization.SettingsDependent; -import fr.xephi.authme.util.RandomStringUtils; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.properties.SecuritySettings; +import fr.xephi.authme.util.ExpiringMap; +import fr.xephi.authme.util.RandomStringUtils; import javax.inject.Inject; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; import static fr.xephi.authme.settings.properties.SecuritySettings.RECOVERY_CODE_HOURS_VALID; -import static fr.xephi.authme.util.Utils.MILLIS_PER_HOUR; /** * Manager for recovery codes. */ -public class RecoveryCodeService implements SettingsDependent { - - private Map recoveryCodes = new ConcurrentHashMap<>(); +public class RecoveryCodeService implements SettingsDependent, HasCleanup { + private final ExpiringMap recoveryCodes; private int recoveryCodeLength; - private long recoveryCodeExpirationMillis; + private int recoveryCodeExpiration; @Inject RecoveryCodeService(Settings settings) { - reload(settings); + recoveryCodeLength = settings.getProperty(SecuritySettings.RECOVERY_CODE_LENGTH); + recoveryCodeExpiration = settings.getProperty(SecuritySettings.RECOVERY_CODE_HOURS_VALID); + recoveryCodes = new ExpiringMap<>(recoveryCodeExpiration, TimeUnit.HOURS); } /** * @return whether recovery codes are enabled or not */ public boolean isRecoveryCodeNeeded() { - return recoveryCodeLength > 0 && recoveryCodeExpirationMillis > 0; + return recoveryCodeLength > 0 && recoveryCodeExpiration > 0; } /** @@ -43,7 +43,7 @@ public class RecoveryCodeService implements SettingsDependent { */ public String generateCode(String player) { String code = RandomStringUtils.generateHex(recoveryCodeLength); - recoveryCodes.put(player, new ExpiringEntry(code, System.currentTimeMillis() + recoveryCodeExpirationMillis)); + recoveryCodes.put(player, code); return code; } @@ -55,11 +55,8 @@ public class RecoveryCodeService implements SettingsDependent { * @return true if the code matches and has not expired, false otherwise */ public boolean isCodeValid(String player, String code) { - ExpiringEntry entry = recoveryCodes.get(player); - if (entry != null) { - return code != null && code.equals(entry.getCode()); - } - return false; + String storedCode = recoveryCodes.get(player); + return storedCode != null && storedCode.equals(code); } /** @@ -74,26 +71,12 @@ public class RecoveryCodeService implements SettingsDependent { @Override public void reload(Settings settings) { recoveryCodeLength = settings.getProperty(SecuritySettings.RECOVERY_CODE_LENGTH); - recoveryCodeExpirationMillis = settings.getProperty(RECOVERY_CODE_HOURS_VALID) * MILLIS_PER_HOUR; + recoveryCodeExpiration = settings.getProperty(RECOVERY_CODE_HOURS_VALID); + recoveryCodes.setExpiration(recoveryCodeExpiration, TimeUnit.HOURS); } - /** - * Entry with an expiration. - */ - @VisibleForTesting - static final class ExpiringEntry { - - private final String code; - private final long expiration; - - ExpiringEntry(String code, long expiration) { - this.code = code; - this.expiration = expiration; - } - - String getCode() { - return System.currentTimeMillis() < expiration ? code : null; - } + @Override + public void performCleanup() { + recoveryCodes.removeExpiredEntries(); } - } diff --git a/src/main/java/fr/xephi/authme/util/ExpiringMap.java b/src/main/java/fr/xephi/authme/util/ExpiringMap.java new file mode 100644 index 000000000..83beb37d6 --- /dev/null +++ b/src/main/java/fr/xephi/authme/util/ExpiringMap.java @@ -0,0 +1,114 @@ +package fr.xephi.authme.util; + +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; + +/** + * Map with expiring entries. Following a configured amount of time after + * an entry has been inserted, the map will act as if the entry does not + * exist. + *

+ * Time starts counting directly after insertion. Inserting a new entry with + * a key that already has a value will "reset" the expiration. Although the + * expiration can be redefined later on, only entries which are inserted + * afterwards will use the new expiration. + *

+ * An expiration of {@code <= 0} will make the map expire all entries + * immediately after insertion. Note that the map does not remove expired + * entries automatically; this is only done when calling + * {@link #removeExpiredEntries()}. + * + * @param the key type + * @param the value type + */ +public class ExpiringMap { + + private final Map> entries = new ConcurrentHashMap<>(); + private long expirationMillis; + + /** + * Constructor. + * + * @param duration the duration of time after which entries expire + * @param unit the time unit in which {@code duration} is expressed + */ + public ExpiringMap(long duration, TimeUnit unit) { + setExpiration(duration, unit); + } + + /** + * Returns the value associated with the given key, + * if available and not expired. + * + * @param key the key to look up + * @return the associated value, or {@code null} if not available + */ + public V get(K key) { + ExpiringEntry value = entries.get(key); + return value == null ? null : value.getValue(); + } + + /** + * Inserts a value for the given key. Overwrites a previous value + * for the key if it exists. + * + * @param key the key to insert a value for + * @param value the value to insert + */ + public void put(K key, V value) { + long expiration = System.currentTimeMillis() + expirationMillis; + entries.put(key, new ExpiringEntry<>(value, expiration)); + } + + /** + * Removes the value for the given key, if available. + * + * @param key the key to remove the value for + */ + public void remove(K key) { + entries.remove(key); + } + + /** + * Removes all entries which have expired from the internal structure. + */ + public void removeExpiredEntries() { + entries.entrySet().removeIf(entry -> System.currentTimeMillis() > entry.getValue().getExpiration()); + } + + /** + * Sets a new expiration duration. Note that already present entries + * will still make use of the old expiration. + * + * @param duration the duration of time after which entries expire + * @param unit the time unit in which {@code duration} is expressed + */ + public void setExpiration(long duration, TimeUnit unit) { + this.expirationMillis = unit.toMillis(duration); + } + + /** + * Class holding a value paired with an expiration timestamp. + * + * @param the value type + */ + private static final class ExpiringEntry { + + private final V value; + private final long expiration; + + ExpiringEntry(V value, long expiration) { + this.value = value; + this.expiration = expiration; + } + + V getValue() { + return System.currentTimeMillis() > expiration ? null : value; + } + + long getExpiration() { + return expiration; + } + } +} diff --git a/src/test/java/fr/xephi/authme/service/RecoveryCodeServiceTest.java b/src/test/java/fr/xephi/authme/service/RecoveryCodeServiceTest.java index b06a01f82..576c5da36 100644 --- a/src/test/java/fr/xephi/authme/service/RecoveryCodeServiceTest.java +++ b/src/test/java/fr/xephi/authme/service/RecoveryCodeServiceTest.java @@ -4,15 +4,13 @@ import ch.jalu.injector.testing.BeforeInjecting; import ch.jalu.injector.testing.DelayedInjectionRunner; import ch.jalu.injector.testing.InjectDelayed; import fr.xephi.authme.ReflectionTestUtils; -import fr.xephi.authme.service.RecoveryCodeService.ExpiringEntry; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.properties.SecuritySettings; +import fr.xephi.authme.util.ExpiringMap; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; -import java.util.Map; - import static fr.xephi.authme.AuthMeMatchers.stringWithLength; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.nullValue; @@ -60,22 +58,8 @@ public class RecoveryCodeServiceTest { recoveryCodeService.generateCode(name); // then - ExpiringEntry entry = getCodeMap().get(name); - assertThat(entry.getCode(), stringWithLength(5)); - } - - @Test - public void shouldNotConsiderExpiredCode() { - // given - String player = "Cat"; - String code = "11F235"; - setCodeInMap(player, code, System.currentTimeMillis() - 500); - - // when - boolean result = recoveryCodeService.isCodeValid(player, code); - - // then - assertThat(result, equalTo(false)); + String code = getCodeMap().get(name); + assertThat(code, stringWithLength(5)); } @Test @@ -106,12 +90,7 @@ public class RecoveryCodeServiceTest { } - private Map getCodeMap() { + private ExpiringMap getCodeMap() { return ReflectionTestUtils.getFieldValue(RecoveryCodeService.class, recoveryCodeService, "recoveryCodes"); } - - private void setCodeInMap(String player, String code, long expiration) { - Map map = getCodeMap(); - map.put(player, new ExpiringEntry(code, expiration)); - } } diff --git a/src/test/java/fr/xephi/authme/util/ExpiringMapTest.java b/src/test/java/fr/xephi/authme/util/ExpiringMapTest.java new file mode 100644 index 000000000..d4a3f8684 --- /dev/null +++ b/src/test/java/fr/xephi/authme/util/ExpiringMapTest.java @@ -0,0 +1,94 @@ +package fr.xephi.authme.util; + +import fr.xephi.authme.ReflectionTestUtils; +import org.junit.Test; + +import java.util.Map; +import java.util.concurrent.TimeUnit; + +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertThat; + +/** + * Test for {@link ExpiringMap}. + */ +public class ExpiringMapTest { + + @Test + public void shouldAddAndRetrieveEntries() { + // given + ExpiringMap map = new ExpiringMap<>(3, TimeUnit.MINUTES); + + // when / then + map.put("three", 3.0); + map.put("treefiddy", 3.50); + + assertThat(map.get("three"), equalTo(3.0)); + assertThat(map.get("treefiddy"), equalTo(3.50)); + } + + @Test + public void shouldRemoveEntry() { + // given + ExpiringMap map = new ExpiringMap<>(1, TimeUnit.HOURS); + map.put("hi", true); + map.put("ha", false); + + // when + map.remove("ha"); + + // then + assertThat(map.get("ha"), nullValue()); + assertThat(map.get("hi"), equalTo(true)); + } + + @Test + public void shouldUpdateExpiration() { + // given + ExpiringMap map = new ExpiringMap<>(2, TimeUnit.DAYS); + map.put(2, 4); + map.put(3, 9); + + // when + map.setExpiration(0, TimeUnit.SECONDS); + + // then + map.put(5, 25); + assertThat(map.get(2), equalTo(4)); + assertThat(map.get(3), equalTo(9)); + assertThat(map.get(5), nullValue()); + } + + @Test + public void shouldAcceptNegativeExpiration() { + // given / when + ExpiringMap map = new ExpiringMap<>(-3, TimeUnit.MINUTES); + map.put(3, "trois"); + + // then + assertThat(map.get(3), nullValue()); + } + + @Test + public void shouldCleanUpExpiredEntries() throws InterruptedException { + // given + ExpiringMap map = new ExpiringMap<>(400, TimeUnit.MILLISECONDS); + map.put(144, 12); + map.put(121, 11); + map.put(81, 9); + map.setExpiration(900, TimeUnit.MILLISECONDS); + map.put(64, 8); + map.put(25, 5); + + // when + Thread.sleep(400); + map.removeExpiredEntries(); + + // then + Map internalMap = ReflectionTestUtils.getFieldValue(ExpiringMap.class, map, "entries"); + assertThat(internalMap.keySet(), containsInAnyOrder(64, 25)); + } + +}