Minor refactoring in listeners; create listener consistency test

- Export duplicated code into a service
- Remove canConnect attribute from AuthMe class - was unused and doesn't seem sensible
- Add consistency test for listener classes
This commit is contained in:
ljacqu 2015-12-20 20:26:01 +01:00
parent 51d1705b1f
commit f809d91c45
9 changed files with 170 additions and 329 deletions

View File

@ -107,10 +107,6 @@ public class AuthMe extends JavaPlugin {
public final ConcurrentHashMap<String, String> cap = new ConcurrentHashMap<>();
public final ConcurrentHashMap<String, String> realIp = new ConcurrentHashMap<>();
// If cache is enabled, prevent any connection before the players data caching is completed.
// TODO: Move somewhere
private boolean canConnect = true;
private CommandHandler commandHandler = null;
private PermissionsManager permsMan = null;
private Settings settings;
@ -168,38 +164,10 @@ public class AuthMe extends JavaPlugin {
*
* @return Messages
*/
public Messages getMessages() {
return messages;
}
/**
* Set the Messages instance.
*
* @param m Messages
*/
public void setMessages(Messages m) {
this.messages = m;
}
/**
* Returns if players are allowed to join the server.
*
* @return boolean
*/
public boolean canConnect() {
return canConnect;
}
/**
* Define if players are allowed to join the server.
*
* @param canConnect boolean
*/
public void setCanConnect(boolean canConnect) {
this.canConnect = canConnect;
}
// Get version and build number of the plugin
// TODO: enhance this
private void setupConstants() {

View File

@ -2,7 +2,6 @@ package fr.xephi.authme.listener;
import fr.xephi.authme.AuthMe;
import fr.xephi.authme.util.Utils;
import org.bukkit.entity.Entity;
import org.bukkit.entity.LivingEntity;
import org.bukkit.entity.Player;
import org.bukkit.entity.Projectile;
@ -14,6 +13,8 @@ import org.bukkit.projectiles.ProjectileSource;
import java.lang.reflect.Method;
import static fr.xephi.authme.listener.ListenerService.shouldCancelEvent;
/**
*/
public class AuthMeEntityListener implements Listener {
@ -22,11 +23,6 @@ public class AuthMeEntityListener implements Listener {
private static boolean shooterIsProjectileSource;
public final AuthMe instance;
/**
* Constructor for AuthMeEntityListener.
*
* @param instance AuthMe
*/
public AuthMeEntityListener(AuthMe instance) {
this.instance = instance;
try {
@ -36,179 +32,61 @@ public class AuthMeEntityListener implements Listener {
}
}
// TODO: npc status can be used to bypass security!!!
/**
* Method onEntityDamage.
*
* @param event EntityDamageEvent
*/
// TODO #360: npc status can be used to bypass security!!!
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onEntityDamage(EntityDamageEvent event) {
Entity entity = event.getEntity();
if (entity == null || !(entity instanceof Player)) {
return;
}
Player player = (Player) entity;
if (Utils.checkAuth(player) ) {
return;
}
if (Utils.isNPC(player)) {
return;
}
player.setFireTicks(0);
if (shouldCancelEvent(event)) {
event.getEntity().setFireTicks(0);
event.setDamage(0);
event.setCancelled(true);
}
}
/**
* Method onEntityTarget.
*
* @param event EntityTargetEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onEntityTarget(EntityTargetEvent event) {
Entity entity = event.getTarget();
if (entity == null || !(entity instanceof Player)) {
return;
}
if (Utils.checkAuth((Player) entity)) {
return;
}
if (Utils.isNPC((Player) entity)) {
return;
}
if (shouldCancelEvent(event)) {
event.setTarget(null);
event.setCancelled(true);
}
}
/**
* Method onDmg.
*
* @param event EntityDamageByEntityEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onDamage(EntityDamageByEntityEvent event) {
Entity entity = event.getDamager();
if (entity == null || !(entity instanceof Player)) {
return;
}
Player player = (Player) entity;
if (Utils.checkAuth(player)) {
return;
}
if (Utils.isNPC(player)) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
/**
* Method onFoodLevelChange.
*
* @param event FoodLevelChangeEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onFoodLevelChange(FoodLevelChangeEvent event) {
Entity entity = event.getEntity();
if (entity == null || !(entity instanceof Player)) {
return;
}
if (Utils.checkAuth((Player) entity)) {
return;
}
if (Utils.isNPC((Player) entity)) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
/**
* Method entityRegainHealthEvent.
*
* @param event EntityRegainHealthEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void entityRegainHealthEvent(EntityRegainHealthEvent event) {
Entity entity = event.getEntity();
if (entity == null || !(entity instanceof Player)) {
return;
}
if (Utils.checkAuth((Player) entity)) {
return;
}
if (Utils.isNPC((Player) entity)) {
return;
}
if (shouldCancelEvent(event)) {
event.setAmount(0);
event.setCancelled(true);
}
}
/**
* Method onEntityInteract.
*
* @param event EntityInteractEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.HIGHEST)
public void onEntityInteract(EntityInteractEvent event) {
Entity entity = event.getEntity();
if (entity == null || !(entity instanceof Player)) {
return;
}
if (Utils.checkAuth((Player) entity)) {
return;
}
if (Utils.isNPC((Player) entity)) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
/**
* Method onLowestEntityInteract.
*
* @param event EntityInteractEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onLowestEntityInteract(EntityInteractEvent event) {
Entity entity = event.getEntity();
if (entity == null || !(entity instanceof Player)) {
return;
}
if (Utils.checkAuth((Player) entity)) {
return;
}
if (Utils.isNPC((Player) entity)) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
// TODO: Need to check this, player can't throw snowball but the item is taken.
/**
* Method onProjectileLaunch.
*
* @param event ProjectileLaunchEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onProjectileLaunch(ProjectileLaunchEvent event) {
if (event.getEntity() == null) {
@ -224,6 +102,7 @@ public class AuthMeEntityListener implements Listener {
}
player = (Player) shooter;
} else {
// TODO ljacqu 20151220: Invoking getShooter() with null but method isn't static
try {
if (getShooter == null) {
getShooter = Projectile.class.getMethod("getShooter");
@ -245,28 +124,11 @@ public class AuthMeEntityListener implements Listener {
event.setCancelled(true);
}
/**
* Method onShoot.
*
* @param event EntityShootBowEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.NORMAL)
public void onShoot(EntityShootBowEvent event) {
Entity entity = event.getEntity();
if (entity == null || !(entity instanceof Player)) {
return;
}
Player player = (Player) entity;
if (Utils.checkAuth(player)) {
return;
}
if (Utils.isNPC(player)) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
}

View File

@ -46,22 +46,10 @@ public class AuthMeInventoryPacketAdapter extends PacketAdapter {
private static final int MAIN_SIZE = 27;
private static final int HOTBAR_SIZE = 9;
/**
* Constructor for AuthMeInventoryPacketAdapter.
*
* @param plugin AuthMe
*/
public AuthMeInventoryPacketAdapter(AuthMe plugin) {
super(plugin, PacketType.Play.Server.SET_SLOT, PacketType.Play.Server.WINDOW_ITEMS);
}
/**
* Method onPacketSending.
*
* @param packetEvent PacketEvent
*
* @see com.comphenix.protocol.events.PacketListener#onPacketSending(PacketEvent)
*/
@Override
public void onPacketSending(PacketEvent packetEvent) {
Player player = packetEvent.getPlayer();
@ -82,11 +70,6 @@ public class AuthMeInventoryPacketAdapter extends PacketAdapter {
ProtocolLibrary.getProtocolManager().removePacketListener(this);
}
/**
* Method sendInventoryPacket.
*
* @param player Player
*/
public void sendInventoryPacket(Player player) {
ProtocolManager protocolManager = ProtocolLibrary.getProtocolManager();
PacketContainer inventoryPacket = protocolManager.createPacket(PacketType.Play.Server.WINDOW_ITEMS);

View File

@ -50,6 +50,8 @@ import org.bukkit.event.player.PlayerShearEntityEvent;
import java.util.concurrent.ConcurrentHashMap;
import static fr.xephi.authme.listener.ListenerService.shouldCancelEvent;
/**
* Listener class for player's events
*/
@ -185,7 +187,7 @@ public class AuthMePlayerListener implements Listener {
player.teleport(spawn);
return;
}
if ((spawn.distance(player.getLocation()) > Settings.getMovementRadius)) {
if (spawn.distance(player.getLocation()) > Settings.getMovementRadius) {
player.teleport(spawn);
}
}
@ -219,12 +221,6 @@ public class AuthMePlayerListener implements Listener {
@EventHandler(priority = EventPriority.HIGHEST)
public void onPreLogin(AsyncPlayerPreLoginEvent event) {
if (!plugin.canConnect()) {
event.setLoginResult(AsyncPlayerPreLoginEvent.Result.KICK_OTHER);
event.setKickMessage("Server is loading, please wait before joining!");
return;
}
PlayerAuth auth = plugin.database.getAuth(event.getName());
if (auth != null && !auth.getRealName().equals("Player") && !auth.getRealName().equals(event.getName())) {
event.setLoginResult(AsyncPlayerPreLoginEvent.Result.KICK_OTHER);
@ -308,7 +304,6 @@ public class AuthMePlayerListener implements Listener {
final String name = player.getName().toLowerCase();
boolean isAuthAvailable = plugin.database.isAuthAvailable(name);
// TODO: Add message to the messages file!!!
if (Settings.isKickNonRegisteredEnabled && !isAuthAvailable) {
if (Settings.antiBotInAction) {
event.setKickMessage(m.retrieveSingle(MessageKey.KICK_ANTIBOT));
@ -376,39 +371,30 @@ public class AuthMePlayerListener implements Listener {
/*
* <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
* TODO: npc status can be used to bypass security!!!
* TODO #360: npc status can be used to bypass security!!!
* <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.HIGHEST)
public void onPlayerPickupItem(PlayerPickupItemEvent event) {
Player player = event.getPlayer();
if (Utils.checkAuth(player) || Utils.isNPC(player)) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onPlayerInteract(PlayerInteractEvent event) {
Player player = event.getPlayer();
if (Utils.checkAuth(player) || Utils.isNPC(player)) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
@EventHandler(ignoreCancelled = true, priority = EventPriority.NORMAL)
public void onPlayerConsumeItem(PlayerItemConsumeEvent event) {
Player player = event.getPlayer();
if (Utils.checkAuth(player) || Utils.isNPC(player)) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
@EventHandler(ignoreCancelled = true, priority = EventPriority.HIGHEST)
public void onPlayerInventoryOpen(InventoryOpenEvent event) {
@ -461,36 +447,24 @@ public class AuthMePlayerListener implements Listener {
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onPlayerInteractEntity(PlayerInteractEntityEvent event) {
if (Utils.checkAuth(event.getPlayer())) {
return;
}
if (Utils.isNPC(event.getPlayer())) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onPlayerDropItem(PlayerDropItemEvent event) {
if (Utils.checkAuth(event.getPlayer())) {
return;
}
if (Utils.isNPC(event.getPlayer())) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onPlayerBedEnter(PlayerBedEnterEvent event) {
if (Utils.checkAuth(event.getPlayer())) {
return;
}
if (Utils.isNPC(event.getPlayer())) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onSignChange(SignChangeEvent event) {
@ -505,10 +479,7 @@ public class AuthMePlayerListener implements Listener {
@EventHandler(ignoreCancelled = true, priority = EventPriority.HIGHEST)
public void onPlayerRespawn(PlayerRespawnEvent event) {
if (Utils.checkAuth(event.getPlayer())) {
return;
}
if (Utils.isNPC(event.getPlayer())) {
if (!shouldCancelEvent(event)) {
return;
}
@ -526,10 +497,7 @@ public class AuthMePlayerListener implements Listener {
@EventHandler(ignoreCancelled = true, priority = EventPriority.HIGHEST)
public void onPlayerGameModeChange(PlayerGameModeChangeEvent event) {
if (Utils.checkAuth(event.getPlayer())) {
return;
}
if (Utils.isNPC(event.getPlayer())) {
if (!shouldCancelEvent(event)) {
return;
}
@ -548,23 +516,15 @@ public class AuthMePlayerListener implements Listener {
@EventHandler(ignoreCancelled = true, priority = EventPriority.NORMAL)
public void onPlayerShear(PlayerShearEntityEvent event) {
if (Utils.checkAuth(event.getPlayer())) {
return;
}
if (Utils.isNPC(event.getPlayer())) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
@EventHandler(ignoreCancelled = true, priority = EventPriority.NORMAL)
public void onPlayerFish(PlayerFishEvent event) {
if (Utils.checkAuth(event.getPlayer())) {
return;
}
if (Utils.isNPC(event.getPlayer())) {
return;
}
if (shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
}

View File

@ -13,31 +13,15 @@ public class AuthMePlayerListener16 implements Listener {
public final AuthMe plugin;
/**
* Constructor for AuthMePlayerListener16.
*
* @param plugin AuthMe
*/
public AuthMePlayerListener16(AuthMe plugin) {
this.plugin = plugin;
}
// TODO: npc status can be used to bypass security!!!
/**
* Method onPlayerEditBook.
*
* @param event PlayerEditBookEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.NORMAL)
public void onPlayerEditBook(PlayerEditBookEvent event) {
if (Utils.checkAuth(event.getPlayer())) {
return;
}
if (Utils.isNPC(event.getPlayer())) {
return;
}
if (ListenerService.shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
}

View File

@ -13,31 +13,15 @@ public class AuthMePlayerListener18 implements Listener {
public final AuthMe plugin;
/**
* Constructor for AuthMePlayerListener18.
*
* @param plugin AuthMe
*/
public AuthMePlayerListener18(AuthMe plugin) {
this.plugin = plugin;
}
// TODO: npc status can be used to bypass security!!!
/**
* Method onPlayerInteractAtEntity.
*
* @param event PlayerInteractAtEntityEvent
*/
@EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST)
public void onPlayerInteractAtEntity(PlayerInteractAtEntityEvent event) {
if (Utils.checkAuth(event.getPlayer())) {
return;
}
if (Utils.isNPC(event.getPlayer())) {
return;
}
if (ListenerService.shouldCancelEvent(event)) {
event.setCancelled(true);
}
}
}

View File

@ -21,21 +21,11 @@ public class AuthMeServerListener implements Listener {
private final AuthMe plugin;
private final Messages m;
/**
* Constructor for AuthMeServerListener.
*
* @param plugin AuthMe
*/
public AuthMeServerListener(AuthMe plugin) {
this.m = plugin.getMessages();
this.plugin = plugin;
}
/**
* Method onServerPing.
*
* @param event ServerListPingEvent
*/
@EventHandler(priority = EventPriority.HIGHEST)
public void onServerPing(ServerListPingEvent event) {
if (!Settings.enableProtection) {
@ -53,11 +43,6 @@ public class AuthMeServerListener implements Listener {
}
}
/**
* Method onPluginDisable.
*
* @param event PluginDisableEvent
*/
@EventHandler(priority = EventPriority.HIGHEST)
public void onPluginDisable(PluginDisableEvent event) {
// Make sure the plugin instance isn't null
@ -103,11 +88,6 @@ public class AuthMeServerListener implements Listener {
}
}
/**
* Method onPluginEnable.
*
* @param event PluginEnableEvent
*/
@EventHandler(priority = EventPriority.HIGHEST)
public void onPluginEnable(PluginEnableEvent event) {
// Call the onPluginEnable method in the permissions manager

View File

@ -0,0 +1,44 @@
package fr.xephi.authme.listener;
import fr.xephi.authme.util.Utils;
import org.bukkit.entity.Entity;
import org.bukkit.entity.Player;
import org.bukkit.event.entity.EntityEvent;
import org.bukkit.event.player.PlayerEvent;
/**
* Service class for the AuthMe listeners.
*/
final class ListenerService {
private ListenerService() {
}
/**
* Return whether an event should be canceled (for unauthenticated, non-NPC players).
*
* @param event The event to process
* @return True if the event should be canceled, false otherwise
*/
public static boolean shouldCancelEvent(EntityEvent event) {
Entity entity = event.getEntity();
if (entity == null || !(entity instanceof Player)) {
return false;
}
Player player = (Player) entity;
return !Utils.checkAuth(player) && !Utils.isNPC(player);
}
/**
* Return whether an event should be canceled (for unauthenticated, non-NPC players).
*
* @param event The event to process
* @return True if the event should be canceled, false otherwise
*/
public static boolean shouldCancelEvent(PlayerEvent event) {
Player player = event.getPlayer();
return player != null && !Utils.checkAuth(player) && !Utils.isNPC(player);
}
}

View File

@ -0,0 +1,76 @@
package fr.xephi.authme.listener;
import com.google.common.collect.Sets;
import org.bukkit.event.EventHandler;
import org.junit.Test;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.Set;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
/**
* Test for verifying that AuthMe listener methods are well-formed.
*/
public final class ListenerConsistencyTest {
private static final Class<?>[] LISTENERS = { AuthMeBlockListener.class, AuthMeEntityListener.class,
AuthMePlayerListener.class, AuthMePlayerListener16.class, AuthMePlayerListener18.class,
AuthMeServerListener.class };
// TODO #368: Ensure that these exceptions are really intentional, if not fix them and remove them here
private static final Set<String> CANCELED_EXCEPTIONS = Sets.newHashSet("AuthMePlayerListener#onPlayerJoin",
"AuthMePlayerListener#onPreLogin", "AuthMePlayerListener#onPlayerLogin",
"AuthMePlayerListener#onPlayerQuit", "AuthMeServerListener#onPluginDisable",
"AuthMeServerListener#onServerPing", "AuthMeServerListener#onPluginEnable");
@Test
public void shouldSetIgnoreCancelledToTrue() {
for (Class<?> listener : LISTENERS) {
checkCanceledAttribute(listener);
}
}
@Test
public void shouldHaveOnlyEventListenersAsPublicMembers() {
for (Class<?> listener : LISTENERS) {
checkPublicMethodsAreListeners(listener);
}
}
private static void checkCanceledAttribute(Class<?> listenerClass) {
final String clazz = listenerClass.getSimpleName();
Method[] methods = listenerClass.getDeclaredMethods();
for (Method method : methods) {
if (isTestableMethod(method) && method.isAnnotationPresent(EventHandler.class)) {
if (CANCELED_EXCEPTIONS.contains(clazz + "#" + method.getName())) {
continue;
}
EventHandler eventHandlerAnnotation = method.getAnnotation(EventHandler.class);
assertThat("Method " + clazz + "#" + method.getName() + " should have ignoreCancelled = true",
eventHandlerAnnotation.ignoreCancelled(), equalTo(true));
}
}
}
private static void checkPublicMethodsAreListeners(Class<?> listenerClass) {
final String clazz = listenerClass.getSimpleName();
Method[] methods = listenerClass.getDeclaredMethods();
for (Method method : methods) {
if (isTestableMethod(method) && !method.isAnnotationPresent(EventHandler.class)) {
fail("Expected @EventHandler annotation on non-private method " + clazz + "#" + method.getName());
}
}
}
private static boolean isTestableMethod(Method method) {
// A method like "access$000" is created by the compiler when a private member is being accessed by an inner
// class, so we need to ignore such methods
return !Modifier.isPrivate(method.getModifiers()) && !method.getName().startsWith("access$");
}
}