From c79ba49ca88e6835afa9299581d87ef795aa7242 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sat, 11 Mar 2017 08:47:58 +0100 Subject: [PATCH] #1113 Don't restore OP to unregistered player; add tests for LimboService --- .../xephi/authme/data/limbo/LimboService.java | 8 +- .../authme/process/join/AsynchronousJoin.java | 1 - .../authme/data/limbo/LimboServiceTest.java | 224 ++++++++++++++++++ 3 files changed, 229 insertions(+), 4 deletions(-) create mode 100644 src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java diff --git a/src/main/java/fr/xephi/authme/data/limbo/LimboService.java b/src/main/java/fr/xephi/authme/data/limbo/LimboService.java index 90b96d37..775337a8 100644 --- a/src/main/java/fr/xephi/authme/data/limbo/LimboService.java +++ b/src/main/java/fr/xephi/authme/data/limbo/LimboService.java @@ -105,7 +105,7 @@ public class LimboService { ConsoleLogger.debug("LimboPlayer for `{0}` was already present", name); } - LimboPlayer limboPlayer = newLimboPlayer(player); + LimboPlayer limboPlayer = newLimboPlayer(player, isRegistered); taskManager.registerMessageTask(player, limboPlayer, isRegistered); taskManager.registerTimeoutTask(player, limboPlayer); revokeLimboStates(player); @@ -157,11 +157,13 @@ public class LimboService { * Creates a LimboPlayer with the given player's details. * * @param player the player to process + * @param isRegistered whether the player is registered * @return limbo player with the player's data */ - private LimboPlayer newLimboPlayer(Player player) { + private LimboPlayer newLimboPlayer(Player player, boolean isRegistered) { Location location = spawnLoader.getPlayerLocationOrSpawn(player); - boolean isOperator = player.isOp(); + // For safety reasons an unregistered player should not have OP status after registration + boolean isOperator = isRegistered && player.isOp(); boolean flyEnabled = player.getAllowFlight(); float walkSpeed = player.getWalkSpeed(); float flySpeed = player.getFlySpeed(); diff --git a/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java b/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java index 20c2ddd4..eed9933f 100644 --- a/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java +++ b/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java @@ -148,7 +148,6 @@ public class AsynchronousJoin implements AsynchronousProcess { final int registrationTimeout = service.getProperty(RestrictionSettings.TIMEOUT) * TICKS_PER_SECOND; bukkitService.scheduleSyncTaskFromOptionallyAsyncTask(() -> { - // TODO #1113: Find an elegant way to deop unregistered players (and disable fly status etc.?) limboService.createLimboPlayer(player, isAuthAvailable); player.setNoDamageTicks(registrationTimeout); diff --git a/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java b/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java new file mode 100644 index 00000000..69d3c370 --- /dev/null +++ b/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java @@ -0,0 +1,224 @@ +package fr.xephi.authme.data.limbo; + +import fr.xephi.authme.ReflectionTestUtils; +import fr.xephi.authme.TestHelper; +import fr.xephi.authme.permission.PermissionsManager; +import fr.xephi.authme.settings.Settings; +import fr.xephi.authme.settings.SpawnLoader; +import fr.xephi.authme.settings.properties.RestrictionSettings; +import org.bukkit.Location; +import org.bukkit.entity.Player; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; + +import java.util.Map; + +import static org.hamcrest.Matchers.anEmptyMap; +import static org.hamcrest.Matchers.both; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.sameInstance; +import static org.junit.Assert.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.only; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; + +/** + * Test for {@link LimboService}. + */ +@RunWith(MockitoJUnitRunner.class) +public class LimboServiceTest { + + @InjectMocks + private LimboService limboService; + + @Mock + private SpawnLoader spawnLoader; + + @Mock + private PermissionsManager permissionsManager; + + @Mock + private Settings settings; + + @Mock + private LimboPlayerTaskManager taskManager; + + @BeforeClass + public static void initLogger() { + TestHelper.setupLogger(); + } + + @Before + public void mockSettings() { + given(settings.getProperty(RestrictionSettings.ALLOW_UNAUTHED_MOVEMENT)).willReturn(false); + given(settings.getProperty(RestrictionSettings.REMOVE_SPEED)).willReturn(true); + } + + @Test + public void shouldCreateLimboPlayer() { + // given + Player player = newPlayer("Bobby", true, 0.3f, false, 0.2f); + Location playerLoc = mock(Location.class); + given(spawnLoader.getPlayerLocationOrSpawn(player)).willReturn(playerLoc); + given(permissionsManager.hasGroupSupport()).willReturn(true); + given(permissionsManager.getPrimaryGroup(player)).willReturn("permgrwp"); + + // when + limboService.createLimboPlayer(player, true); + + // then + verify(taskManager).registerMessageTask(eq(player), any(LimboPlayer.class), eq(true)); + verify(taskManager).registerTimeoutTask(eq(player), any(LimboPlayer.class)); + verify(player).setAllowFlight(false); + verify(player).setFlySpeed(0.0f); + verify(player).setWalkSpeed(0.0f); + + LimboPlayer limbo = getLimboMap().get("bobby"); + assertThat(limbo, not(nullValue())); + assertThat(limbo.isOperator(), equalTo(true)); + assertThat(limbo.getWalkSpeed(), equalTo(0.3f)); + assertThat(limbo.isCanFly(), equalTo(false)); + assertThat(limbo.getFlySpeed(), equalTo(0.2f)); + assertThat(limbo.getLocation(), equalTo(playerLoc)); + assertThat(limbo.getGroup(), equalTo("permgrwp")); + } + + @Test + public void shouldNotKeepOpStatusForUnregisteredPlayer() { + // given + Player player = newPlayer("CharleS", true, 0.1f, true, 0.4f); + Location playerLoc = mock(Location.class); + given(spawnLoader.getPlayerLocationOrSpawn(player)).willReturn(playerLoc); + given(permissionsManager.hasGroupSupport()).willReturn(false); + + // when + limboService.createLimboPlayer(player, false); + + // then + verify(taskManager).registerMessageTask(eq(player), any(LimboPlayer.class), eq(false)); + verify(taskManager).registerTimeoutTask(eq(player), any(LimboPlayer.class)); + verify(permissionsManager, only()).hasGroupSupport(); + verify(player).setAllowFlight(false); + verify(player).setFlySpeed(0.0f); + verify(player).setWalkSpeed(0.0f); + + LimboPlayer limbo = getLimboMap().get("charles"); + assertThat(limbo, not(nullValue())); + assertThat(limbo.isOperator(), equalTo(false)); + assertThat(limbo.getWalkSpeed(), equalTo(0.1f)); + assertThat(limbo.isCanFly(), equalTo(true)); + assertThat(limbo.getFlySpeed(), equalTo(0.4f)); + assertThat(limbo.getLocation(), equalTo(playerLoc)); + assertThat(limbo.getGroup(), equalTo("")); + } + + @Test + public void shouldClearTasksOnAlreadyExistingLimbo() { + // given + LimboPlayer limbo = mock(LimboPlayer.class); + getLimboMap().put("carlos", limbo); + Player player = newPlayer("Carlos"); + + // when + limboService.createLimboPlayer(player, false); + + // then + verify(limbo).clearTasks(); + assertThat(getLimboMap().get("carlos"), both(not(sameInstance(limbo))).and(not(nullValue()))); + } + + @Test + public void shouldRestoreData() { + // given + Player player = newPlayer("John", true, 0.4f, false, 0.2f); + LimboPlayer limbo = Mockito.spy(convertToLimboPlayer(player, null, "")); + getLimboMap().put("john", limbo); + + // when + limboService.restoreData(player); + + // then + verify(player).setOp(true); + verify(player).setWalkSpeed(0.4f); + verify(player).setAllowFlight(false); + verify(player).setFlySpeed(0.2f); + verify(limbo).clearTasks(); + assertThat(getLimboMap(), anEmptyMap()); + } + + @Test + public void shouldHandleMissingLimboPlayerWhileRestoring() { + // given + Player player = newPlayer("Test"); + + // when + limboService.restoreData(player); + + // then + verify(player, only()).getName(); + } + + @Test + public void shouldReplaceTasks() { + // given + LimboPlayer limbo = mock(LimboPlayer.class); + getLimboMap().put("jeff", limbo); + Player player = newPlayer("JEFF"); + + + // when + limboService.replaceTasksAfterRegistration(player); + + // then + verify(taskManager).registerTimeoutTask(player, limbo); + verify(taskManager).registerMessageTask(player, limbo, true); + } + + @Test + public void shouldHandleMissingLimboForReplaceTasks() { + // given + Player player = newPlayer("ghost"); + + // when + limboService.replaceTasksAfterRegistration(player); + + // then + verifyZeroInteractions(taskManager); + } + + private static Player newPlayer(String name) { + Player player = mock(Player.class); + given(player.getName()).willReturn(name); + return player; + } + + private static Player newPlayer(String name, boolean isOp, float walkSpeed, boolean canFly, float flySpeed) { + Player player = newPlayer(name); + given(player.isOp()).willReturn(isOp); + given(player.getWalkSpeed()).willReturn(walkSpeed); + given(player.getAllowFlight()).willReturn(canFly); + given(player.getFlySpeed()).willReturn(flySpeed); + return player; + } + + private static LimboPlayer convertToLimboPlayer(Player player, Location location, String group) { + return new LimboPlayer(location, player.isOp(), group, player.getAllowFlight(), + player.getWalkSpeed(), player.getFlySpeed()); + } + + private Map getLimboMap() { + return ReflectionTestUtils.getFieldValue(LimboService.class, limboService, "entries"); + } +}