From 25f7e1321aa3a7a715e4119cf4b21ae651577b03 Mon Sep 17 00:00:00 2001 From: games647 Date: Fri, 18 Jun 2021 14:55:21 +0200 Subject: [PATCH 1/7] Check inventory authentication status only on cache Fixes #2212 In #2112, we found out that in configurations with disabled cache (like Bungee), this adapter will perform blocking queries on the main thread which impacts the performance. The original code (81cf14fbc16aaecf7ac72dfea9a8acbff21f4f6d) is in place to preserve the inventory for unregistered and configurations without enforcing a registration (#1830, #1752). Alternatives are: 1. Send the inventory on registration like p.updateInventory() * Still hides it before for unregistered 2. Checking for the enforce registration setting would mean it hides also the inventory for unregistered players 3. Get a notification on player status changes * Requires a complex setup to propagate the changes across spigot servers or different solution for web interfaces * Refresh events, when the status is updated within the plugin itself would be possible, however requires previous queries like registration/login requests. Instant reports about registration will still be complicated. Example: Every time we make queries, save the result locally and fire an event for our components to check for potential changes without making the same query again. Nevertheless this again delays changes if there is no need to that. --- .../protocollib/InventoryPacketAdapter.java | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java b/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java index bceeaca7a..ce9a0e03d 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java @@ -83,7 +83,20 @@ class InventoryPacketAdapter extends PacketAdapter { } private boolean shouldHideInventory(String playerName) { - return !playerCache.isAuthenticated(playerName) && dataSource.isAuthAvailable(playerName); + if (playerCache.isAuthenticated(playerName)) { + // fully logged in - no need to protect it + return false; + } + + if (dataSource.isCached()) { + // load from cache or only request once + return dataSource.isAuthAvailable(playerName); + } + + // data source is not cached - this means queries would run blocking + // Assume the player is registered would mean the inventory will be protected + // and any potential information leak can be prevented + return true; } public void unregister() { From a8ebe7090083dacd02f3c49689164472ccd048f2 Mon Sep 17 00:00:00 2001 From: games647 Date: Sat, 19 Jun 2021 11:46:12 +0200 Subject: [PATCH 2/7] Only leave inventory untouched if registration is not enforced --- .../protocollib/InventoryPacketAdapter.java | 20 ++++++++++++------- .../protocollib/ProtocolLibService.java | 5 ++++- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java b/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java index ce9a0e03d..7709bb747 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java @@ -24,20 +24,22 @@ import com.comphenix.protocol.events.PacketAdapter; import com.comphenix.protocol.events.PacketContainer; import com.comphenix.protocol.events.PacketEvent; import com.comphenix.protocol.reflect.StructureModifier; + import fr.xephi.authme.AuthMe; import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.output.ConsoleLoggerFactory; import fr.xephi.authme.service.BukkitService; -import org.bukkit.Material; -import org.bukkit.entity.Player; -import org.bukkit.inventory.ItemStack; import java.lang.reflect.InvocationTargetException; import java.util.Arrays; import java.util.List; +import org.bukkit.Material; +import org.bukkit.entity.Player; +import org.bukkit.inventory.ItemStack; + class InventoryPacketAdapter extends PacketAdapter { private static final int PLAYER_INVENTORY = 0; @@ -52,10 +54,14 @@ class InventoryPacketAdapter extends PacketAdapter { private final PlayerCache playerCache; private final DataSource dataSource; - InventoryPacketAdapter(AuthMe plugin, PlayerCache playerCache, DataSource dataSource) { + private final boolean isRegistrationForced; + + InventoryPacketAdapter(AuthMe plugin, PlayerCache playerCache, DataSource dataSource, + boolean isRegistrationForced) { super(plugin, PacketType.Play.Server.SET_SLOT, PacketType.Play.Server.WINDOW_ITEMS); this.playerCache = playerCache; this.dataSource = dataSource; + this.isRegistrationForced = isRegistrationForced; } @Override @@ -94,9 +100,9 @@ class InventoryPacketAdapter extends PacketAdapter { } // data source is not cached - this means queries would run blocking - // Assume the player is registered would mean the inventory will be protected - // and any potential information leak can be prevented - return true; + // If registration is enforced: **assume** player is registered to prevent any information leak + // If not, players could play even without a registration, so there is no need for protection + return isRegistrationForced; } public void unregister() { diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java index 024077570..19c1b26aa 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java @@ -9,6 +9,7 @@ import fr.xephi.authme.initialization.SettingsDependent; import fr.xephi.authme.output.ConsoleLoggerFactory; import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.settings.Settings; +import fr.xephi.authme.settings.properties.RegistrationSettings; import fr.xephi.authme.settings.properties.RestrictionSettings; import org.bukkit.entity.Player; @@ -26,6 +27,7 @@ public class ProtocolLibService implements SettingsDependent { /* Settings */ private boolean protectInvBeforeLogin; private boolean denyTabCompleteBeforeLogin; + private boolean isRegistrationForced; /* Service */ private boolean isEnabled; @@ -66,7 +68,7 @@ public class ProtocolLibService implements SettingsDependent { if (protectInvBeforeLogin) { if (inventoryPacketAdapter == null) { // register the packet listener and start hiding it for all already online players (reload) - inventoryPacketAdapter = new InventoryPacketAdapter(plugin, playerCache, dataSource); + inventoryPacketAdapter = new InventoryPacketAdapter(plugin, playerCache, dataSource, isRegistrationForced); inventoryPacketAdapter.register(bukkitService); } } else if (inventoryPacketAdapter != null) { @@ -120,6 +122,7 @@ public class ProtocolLibService implements SettingsDependent { this.protectInvBeforeLogin = settings.getProperty(RestrictionSettings.PROTECT_INVENTORY_BEFORE_LOGIN); this.denyTabCompleteBeforeLogin = settings.getProperty(RestrictionSettings.DENY_TABCOMPLETE_BEFORE_LOGIN); + this.isRegistrationForced = settings.getProperty(RegistrationSettings.FORCE); //it was true and will be deactivated now, so we need to restore the inventory for every player if (oldProtectInventory && !protectInvBeforeLogin && inventoryPacketAdapter != null) { From f52e6a1a2fb61b694eec05b15726f098d88472d2 Mon Sep 17 00:00:00 2001 From: games647 Date: Sat, 19 Jun 2021 11:53:29 +0200 Subject: [PATCH 3/7] Allow unregistered players in not enforced config to use tab --- .../protocollib/InventoryPacketAdapter.java | 27 ++++--------------- .../protocollib/ProtocolLibService.java | 21 +++++++++++++-- .../protocollib/TabCompletePacketAdapter.java | 11 ++++---- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java b/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java index 7709bb747..ec256d9b8 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java @@ -54,14 +54,14 @@ class InventoryPacketAdapter extends PacketAdapter { private final PlayerCache playerCache; private final DataSource dataSource; - private final boolean isRegistrationForced; + private final ProtocolLibService protocolLibService; InventoryPacketAdapter(AuthMe plugin, PlayerCache playerCache, DataSource dataSource, - boolean isRegistrationForced) { + ProtocolLibService protocolLibService) { super(plugin, PacketType.Play.Server.SET_SLOT, PacketType.Play.Server.WINDOW_ITEMS); this.playerCache = playerCache; this.dataSource = dataSource; - this.isRegistrationForced = isRegistrationForced; + this.protocolLibService = protocolLibService; } @Override @@ -70,7 +70,7 @@ class InventoryPacketAdapter extends PacketAdapter { PacketContainer packet = packetEvent.getPacket(); int windowId = packet.getIntegers().read(0); - if (windowId == PLAYER_INVENTORY && shouldHideInventory(player.getName())) { + if (windowId == PLAYER_INVENTORY && protocolLibService.shouldRestrictPlayer(player.getName())) { packetEvent.setCancelled(true); } } @@ -84,27 +84,10 @@ class InventoryPacketAdapter extends PacketAdapter { ProtocolLibrary.getProtocolManager().addPacketListener(this); bukkitService.getOnlinePlayers().stream() - .filter(player -> shouldHideInventory(player.getName())) + .filter(player -> protocolLibService.shouldRestrictPlayer(player.getName())) .forEach(this::sendBlankInventoryPacket); } - private boolean shouldHideInventory(String playerName) { - if (playerCache.isAuthenticated(playerName)) { - // fully logged in - no need to protect it - return false; - } - - if (dataSource.isCached()) { - // load from cache or only request once - return dataSource.isAuthAvailable(playerName); - } - - // data source is not cached - this means queries would run blocking - // If registration is enforced: **assume** player is registered to prevent any information leak - // If not, players could play even without a registration, so there is no need for protection - return isRegistrationForced; - } - public void unregister() { ProtocolLibrary.getProtocolManager().removePacketListener(this); } diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java index 19c1b26aa..c1afc54c6 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java @@ -68,7 +68,7 @@ public class ProtocolLibService implements SettingsDependent { if (protectInvBeforeLogin) { if (inventoryPacketAdapter == null) { // register the packet listener and start hiding it for all already online players (reload) - inventoryPacketAdapter = new InventoryPacketAdapter(plugin, playerCache, dataSource, isRegistrationForced); + inventoryPacketAdapter = new InventoryPacketAdapter(plugin, playerCache, dataSource, this); inventoryPacketAdapter.register(bukkitService); } } else if (inventoryPacketAdapter != null) { @@ -78,7 +78,7 @@ public class ProtocolLibService implements SettingsDependent { if (denyTabCompleteBeforeLogin) { if (tabCompletePacketAdapter == null) { - tabCompletePacketAdapter = new TabCompletePacketAdapter(plugin, playerCache); + tabCompletePacketAdapter = new TabCompletePacketAdapter(plugin, this); tabCompletePacketAdapter.register(); } } else if (tabCompletePacketAdapter != null) { @@ -116,6 +116,23 @@ public class ProtocolLibService implements SettingsDependent { } } + protected boolean shouldRestrictPlayer(String playerName) { + if (playerCache.isAuthenticated(playerName)) { + // fully logged in - no need to protect it + return false; + } + + if (dataSource.isCached()) { + // load from cache or only request once + return dataSource.isAuthAvailable(playerName); + } + + // data source is not cached - this means queries would run blocking + // If registration is enforced: **assume** player is registered to prevent any information leak + // If not, players could play even without a registration, so there is no need for protection + return isRegistrationForced; + } + @Override public void reload(Settings settings) { boolean oldProtectInventory = this.protectInvBeforeLogin; diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/TabCompletePacketAdapter.java b/src/main/java/fr/xephi/authme/listener/protocollib/TabCompletePacketAdapter.java index 3f0bb3161..f36d06b9c 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/TabCompletePacketAdapter.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/TabCompletePacketAdapter.java @@ -8,24 +8,25 @@ import com.comphenix.protocol.events.PacketEvent; import com.comphenix.protocol.reflect.FieldAccessException; import fr.xephi.authme.AuthMe; import fr.xephi.authme.ConsoleLogger; -import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.output.ConsoleLoggerFactory; class TabCompletePacketAdapter extends PacketAdapter { private final ConsoleLogger logger = ConsoleLoggerFactory.get(TabCompletePacketAdapter.class); - private final PlayerCache playerCache; - TabCompletePacketAdapter(AuthMe plugin, PlayerCache playerCache) { + private final ProtocolLibService protocolLibService; + + TabCompletePacketAdapter(AuthMe plugin, ProtocolLibService protocolLibService) { super(plugin, ListenerPriority.NORMAL, PacketType.Play.Client.TAB_COMPLETE); - this.playerCache = playerCache; + this.protocolLibService = protocolLibService; } @Override public void onPacketReceiving(PacketEvent event) { if (event.getPacketType() == PacketType.Play.Client.TAB_COMPLETE) { try { - if (!playerCache.isAuthenticated(event.getPlayer().getName())) { + String playerName = event.getPlayer().getName(); + if (protocolLibService.shouldRestrictPlayer(playerName)) { event.setCancelled(true); } } catch (FieldAccessException e) { From c04a72ba551ca5da3eadd560d905d88df3a0b846 Mon Sep 17 00:00:00 2001 From: games647 Date: Sat, 19 Jun 2021 11:53:29 +0200 Subject: [PATCH 4/7] Allow unregistered players in not enforced config to use tab --- .../listener/protocollib/ProtocolLibService.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java index c1afc54c6..20695e97b 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java @@ -116,6 +116,12 @@ public class ProtocolLibService implements SettingsDependent { } } + /** + * Should the given player need to be restricted + * + * @param playerName player that is about to prevented to do or see something + * @return true if restriction is necessary + */ protected boolean shouldRestrictPlayer(String playerName) { if (playerCache.isAuthenticated(playerName)) { // fully logged in - no need to protect it @@ -137,9 +143,9 @@ public class ProtocolLibService implements SettingsDependent { public void reload(Settings settings) { boolean oldProtectInventory = this.protectInvBeforeLogin; - this.protectInvBeforeLogin = settings.getProperty(RestrictionSettings.PROTECT_INVENTORY_BEFORE_LOGIN); - this.denyTabCompleteBeforeLogin = settings.getProperty(RestrictionSettings.DENY_TABCOMPLETE_BEFORE_LOGIN); this.isRegistrationForced = settings.getProperty(RegistrationSettings.FORCE); + this.denyTabCompleteBeforeLogin = settings.getProperty(RestrictionSettings.DENY_TABCOMPLETE_BEFORE_LOGIN); + this.protectInvBeforeLogin = settings.getProperty(RestrictionSettings.PROTECT_INVENTORY_BEFORE_LOGIN); //it was true and will be deactivated now, so we need to restore the inventory for every player if (oldProtectInventory && !protectInvBeforeLogin && inventoryPacketAdapter != null) { From b1a9abc019c80a582b5a45eb05d87f0215da6b50 Mon Sep 17 00:00:00 2001 From: games647 Date: Sat, 19 Jun 2021 17:47:30 +0200 Subject: [PATCH 5/7] Perform all restriction checks only on cache Cache registration status for online players. The cache stays valid until the player leaves the server or logs manual in. --- .../xephi/authme/data/auth/PlayerCache.java | 39 ++++++++++++++++- .../authme/datasource/CacheDataSource.java | 2 +- .../authme/listener/ListenerService.java | 43 +++++++++++-------- .../protocollib/InventoryPacketAdapter.java | 18 +++----- .../protocollib/ProtocolLibService.java | 39 +++-------------- .../protocollib/TabCompletePacketAdapter.java | 9 ++-- .../authme/process/join/AsynchronousJoin.java | 8 +++- .../authme/service/TeleportationService.java | 3 +- .../authme/listener/ListenerServiceTest.java | 10 +++-- 9 files changed, 97 insertions(+), 74 deletions(-) diff --git a/src/main/java/fr/xephi/authme/data/auth/PlayerCache.java b/src/main/java/fr/xephi/authme/data/auth/PlayerCache.java index 46fafbf77..7c21ee4b3 100644 --- a/src/main/java/fr/xephi/authme/data/auth/PlayerCache.java +++ b/src/main/java/fr/xephi/authme/data/auth/PlayerCache.java @@ -10,6 +10,7 @@ import java.util.concurrent.ConcurrentHashMap; public class PlayerCache { private final Map cache = new ConcurrentHashMap<>(); + private final Map registeredCache = new ConcurrentHashMap<>(); PlayerCache() { } @@ -20,6 +21,7 @@ public class PlayerCache { * @param auth the player auth object to save */ public void updatePlayer(PlayerAuth auth) { + registeredCache.put(auth.getNickname().toLowerCase(), RegistrationStatus.REGISTERED); cache.put(auth.getNickname().toLowerCase(), auth); } @@ -30,6 +32,7 @@ public class PlayerCache { */ public void removePlayer(String user) { cache.remove(user.toLowerCase()); + registeredCache.remove(user.toLowerCase()); } /** @@ -43,6 +46,35 @@ public class PlayerCache { return cache.containsKey(user.toLowerCase()); } + /** + * Add a registration entry to the cache for active use later like the player active playing. + * + * @param user player name + * @param status registration status + */ + public void addRegistrationStatus(String user, RegistrationStatus status) { + registeredCache.put(user.toLowerCase(), status); + } + + /** + * Update the status for existing entries like currently active users + * @param user player name + * @param status newest query result + */ + public void updateRegistrationStatus(String user, RegistrationStatus status) { + registeredCache.replace(user, status); + } + + /** + * Checks if there is cached result with the player having an account. + * Warning: This shouldn't be used for authentication, because the result could be outdated. + * @param user player name + * @return Cached result about being registered or unregistered and UNKNOWN if there is no cache entry + */ + public RegistrationStatus getRegistrationStatus(String user) { + return registeredCache.getOrDefault(user.toLowerCase(), RegistrationStatus.UNKNOWN); + } + /** * Returns the PlayerAuth associated with the given user, if available. * @@ -66,8 +98,13 @@ public class PlayerCache { * * @return all player auths inside the player cache */ - public Map getCache() { + public Map getAuthCache() { return this.cache; } + public enum RegistrationStatus { + REGISTERED, + UNREGISTERED, + UNKNOWN + } } diff --git a/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java b/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java index e1418dcaf..9b4fad54a 100644 --- a/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java +++ b/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java @@ -267,7 +267,7 @@ public class CacheDataSource implements DataSource { @Override public List getLoggedPlayersWithEmptyMail() { - return playerCache.getCache().values().stream() + return playerCache.getAuthCache().values().stream() .filter(auth -> Utils.isEmailEmpty(auth.getEmail())) .map(PlayerAuth::getRealName) .collect(Collectors.toList()); diff --git a/src/main/java/fr/xephi/authme/listener/ListenerService.java b/src/main/java/fr/xephi/authme/listener/ListenerService.java index d283f3e4e..4502a9e31 100644 --- a/src/main/java/fr/xephi/authme/listener/ListenerService.java +++ b/src/main/java/fr/xephi/authme/listener/ListenerService.java @@ -1,6 +1,7 @@ package fr.xephi.authme.listener; import fr.xephi.authme.data.auth.PlayerCache; +import fr.xephi.authme.data.auth.PlayerCache.RegistrationStatus; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.initialization.SettingsDependent; import fr.xephi.authme.service.ValidationService; @@ -17,7 +18,7 @@ import javax.inject.Inject; /** * Service class for the AuthMe listeners to determine whether an event should be canceled. */ -class ListenerService implements SettingsDependent { +public class ListenerService implements SettingsDependent { private final DataSource dataSource; private final PlayerCache playerCache; @@ -77,28 +78,32 @@ class ListenerService implements SettingsDependent { * @return true if the associated event should be canceled, false otherwise */ public boolean shouldCancelEvent(Player player) { - return player != null && !checkAuth(player.getName()) && !PlayerUtils.isNpc(player); + return player != null && !PlayerUtils.isNpc(player) && shouldRestrictPlayer(player.getName()); + } + + /** + * Check if restriction are required for the given player name. The check will be performed against the local + * cache. This means changes from other sources like web services will have a delay to it. + * + * @param name player name + * @return true if the player needs to be restricted + */ + public boolean shouldRestrictPlayer(String name) { + if (validationService.isUnrestricted(name) || playerCache.isAuthenticated(name)) { + return false; + } + + if (isRegistrationForced) { + // registration always required to play - so restrict everything + return true; + } + + // registration not enforced, but registered players needs to be restricted if not logged in + return playerCache.getRegistrationStatus(name) == RegistrationStatus.REGISTERED; } @Override public void reload(Settings settings) { isRegistrationForced = settings.getProperty(RegistrationSettings.FORCE); } - - /** - * Checks whether the player is allowed to perform actions (i.e. whether he is logged in - * or if other settings permit playing). - * - * @param name the name of the player to verify - * @return true if the player may play, false otherwise - */ - private boolean checkAuth(String name) { - if (validationService.isUnrestricted(name) || playerCache.isAuthenticated(name)) { - return true; - } - if (!isRegistrationForced && !dataSource.isAuthAvailable(name)) { - return true; - } - return false; - } } diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java b/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java index ec256d9b8..eded77c5b 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/InventoryPacketAdapter.java @@ -27,8 +27,7 @@ import com.comphenix.protocol.reflect.StructureModifier; import fr.xephi.authme.AuthMe; import fr.xephi.authme.ConsoleLogger; -import fr.xephi.authme.data.auth.PlayerCache; -import fr.xephi.authme.datasource.DataSource; +import fr.xephi.authme.listener.ListenerService; import fr.xephi.authme.output.ConsoleLoggerFactory; import fr.xephi.authme.service.BukkitService; @@ -51,17 +50,12 @@ class InventoryPacketAdapter extends PacketAdapter { private static final int HOTBAR_SIZE = 9; private final ConsoleLogger logger = ConsoleLoggerFactory.get(InventoryPacketAdapter.class); - private final PlayerCache playerCache; - private final DataSource dataSource; - private final ProtocolLibService protocolLibService; + private final ListenerService listenerService; - InventoryPacketAdapter(AuthMe plugin, PlayerCache playerCache, DataSource dataSource, - ProtocolLibService protocolLibService) { + InventoryPacketAdapter(AuthMe plugin, ListenerService listenerService) { super(plugin, PacketType.Play.Server.SET_SLOT, PacketType.Play.Server.WINDOW_ITEMS); - this.playerCache = playerCache; - this.dataSource = dataSource; - this.protocolLibService = protocolLibService; + this.listenerService = listenerService; } @Override @@ -70,7 +64,7 @@ class InventoryPacketAdapter extends PacketAdapter { PacketContainer packet = packetEvent.getPacket(); int windowId = packet.getIntegers().read(0); - if (windowId == PLAYER_INVENTORY && protocolLibService.shouldRestrictPlayer(player.getName())) { + if (windowId == PLAYER_INVENTORY && listenerService.shouldRestrictPlayer(player.getName())) { packetEvent.setCancelled(true); } } @@ -84,7 +78,7 @@ class InventoryPacketAdapter extends PacketAdapter { ProtocolLibrary.getProtocolManager().addPacketListener(this); bukkitService.getOnlinePlayers().stream() - .filter(player -> protocolLibService.shouldRestrictPlayer(player.getName())) + .filter(player -> listenerService.shouldRestrictPlayer(player.getName())) .forEach(this::sendBlankInventoryPacket); } diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java index 20695e97b..01d682ab3 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java @@ -6,10 +6,10 @@ import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.initialization.SettingsDependent; +import fr.xephi.authme.listener.ListenerService; import fr.xephi.authme.output.ConsoleLoggerFactory; import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.settings.Settings; -import fr.xephi.authme.settings.properties.RegistrationSettings; import fr.xephi.authme.settings.properties.RestrictionSettings; import org.bukkit.entity.Player; @@ -27,22 +27,21 @@ public class ProtocolLibService implements SettingsDependent { /* Settings */ private boolean protectInvBeforeLogin; private boolean denyTabCompleteBeforeLogin; - private boolean isRegistrationForced; /* Service */ private boolean isEnabled; private final AuthMe plugin; private final BukkitService bukkitService; + private final ListenerService listenerService; private final PlayerCache playerCache; - private final DataSource dataSource; @Inject - ProtocolLibService(AuthMe plugin, Settings settings, BukkitService bukkitService, PlayerCache playerCache, - DataSource dataSource) { + ProtocolLibService(AuthMe plugin, Settings settings, BukkitService bukkitService, ListenerService listenerService, + PlayerCache playerCache) { this.plugin = plugin; this.bukkitService = bukkitService; + this.listenerService = listenerService; this.playerCache = playerCache; - this.dataSource = dataSource; reload(settings); } @@ -68,7 +67,7 @@ public class ProtocolLibService implements SettingsDependent { if (protectInvBeforeLogin) { if (inventoryPacketAdapter == null) { // register the packet listener and start hiding it for all already online players (reload) - inventoryPacketAdapter = new InventoryPacketAdapter(plugin, playerCache, dataSource, this); + inventoryPacketAdapter = new InventoryPacketAdapter(plugin, listenerService); inventoryPacketAdapter.register(bukkitService); } } else if (inventoryPacketAdapter != null) { @@ -78,7 +77,7 @@ public class ProtocolLibService implements SettingsDependent { if (denyTabCompleteBeforeLogin) { if (tabCompletePacketAdapter == null) { - tabCompletePacketAdapter = new TabCompletePacketAdapter(plugin, this); + tabCompletePacketAdapter = new TabCompletePacketAdapter(plugin, listenerService); tabCompletePacketAdapter.register(); } } else if (tabCompletePacketAdapter != null) { @@ -116,34 +115,10 @@ public class ProtocolLibService implements SettingsDependent { } } - /** - * Should the given player need to be restricted - * - * @param playerName player that is about to prevented to do or see something - * @return true if restriction is necessary - */ - protected boolean shouldRestrictPlayer(String playerName) { - if (playerCache.isAuthenticated(playerName)) { - // fully logged in - no need to protect it - return false; - } - - if (dataSource.isCached()) { - // load from cache or only request once - return dataSource.isAuthAvailable(playerName); - } - - // data source is not cached - this means queries would run blocking - // If registration is enforced: **assume** player is registered to prevent any information leak - // If not, players could play even without a registration, so there is no need for protection - return isRegistrationForced; - } - @Override public void reload(Settings settings) { boolean oldProtectInventory = this.protectInvBeforeLogin; - this.isRegistrationForced = settings.getProperty(RegistrationSettings.FORCE); this.denyTabCompleteBeforeLogin = settings.getProperty(RestrictionSettings.DENY_TABCOMPLETE_BEFORE_LOGIN); this.protectInvBeforeLogin = settings.getProperty(RestrictionSettings.PROTECT_INVENTORY_BEFORE_LOGIN); diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/TabCompletePacketAdapter.java b/src/main/java/fr/xephi/authme/listener/protocollib/TabCompletePacketAdapter.java index f36d06b9c..4a67f550e 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/TabCompletePacketAdapter.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/TabCompletePacketAdapter.java @@ -8,17 +8,18 @@ import com.comphenix.protocol.events.PacketEvent; import com.comphenix.protocol.reflect.FieldAccessException; import fr.xephi.authme.AuthMe; import fr.xephi.authme.ConsoleLogger; +import fr.xephi.authme.listener.ListenerService; import fr.xephi.authme.output.ConsoleLoggerFactory; class TabCompletePacketAdapter extends PacketAdapter { private final ConsoleLogger logger = ConsoleLoggerFactory.get(TabCompletePacketAdapter.class); - private final ProtocolLibService protocolLibService; + private final ListenerService listenerService; - TabCompletePacketAdapter(AuthMe plugin, ProtocolLibService protocolLibService) { + TabCompletePacketAdapter(AuthMe plugin, ListenerService listenerService) { super(plugin, ListenerPriority.NORMAL, PacketType.Play.Client.TAB_COMPLETE); - this.protocolLibService = protocolLibService; + this.listenerService = listenerService; } @Override @@ -26,7 +27,7 @@ class TabCompletePacketAdapter extends PacketAdapter { if (event.getPacketType() == PacketType.Play.Client.TAB_COMPLETE) { try { String playerName = event.getPlayer().getName(); - if (protocolLibService.shouldRestrictPlayer(playerName)) { + if (listenerService.shouldRestrictPlayer(playerName)) { event.setCancelled(true); } } catch (FieldAccessException e) { 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 7b16e564a..d715759cf 100644 --- a/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java +++ b/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java @@ -2,6 +2,8 @@ package fr.xephi.authme.process.join; import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.data.ProxySessionManager; +import fr.xephi.authme.data.auth.PlayerCache; +import fr.xephi.authme.data.auth.PlayerCache.RegistrationStatus; import fr.xephi.authme.data.limbo.LimboService; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.events.ProtectInventoryEvent; @@ -73,6 +75,9 @@ public class AsynchronousJoin implements AsynchronousProcess { @Inject private SessionService sessionService; + @Inject + private PlayerCache playerCache; + @Inject private ProxySessionManager proxySessionManager; @@ -112,7 +117,8 @@ public class AsynchronousJoin implements AsynchronousProcess { } final boolean isAuthAvailable = database.isAuthAvailable(name); - + RegistrationStatus status = RegistrationStatus.UNREGISTERED; + playerCache.addRegistrationStatus(name, isAuthAvailable ? RegistrationStatus.REGISTERED : status); if (isAuthAvailable) { // Protect inventory if (service.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN)) { diff --git a/src/main/java/fr/xephi/authme/service/TeleportationService.java b/src/main/java/fr/xephi/authme/service/TeleportationService.java index f0eb7f851..ef84f2a41 100644 --- a/src/main/java/fr/xephi/authme/service/TeleportationService.java +++ b/src/main/java/fr/xephi/authme/service/TeleportationService.java @@ -3,6 +3,7 @@ package fr.xephi.authme.service; import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.data.auth.PlayerAuth; import fr.xephi.authme.data.auth.PlayerCache; +import fr.xephi.authme.data.auth.PlayerCache.RegistrationStatus; import fr.xephi.authme.data.limbo.LimboPlayer; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.events.AbstractTeleportEvent; @@ -112,7 +113,7 @@ public class TeleportationService implements Reloadable { return; } - if (!player.hasPlayedBefore() || !dataSource.isAuthAvailable(player.getName())) { + if (!player.hasPlayedBefore() || playerCache.getRegistrationStatus(player.getName()) == RegistrationStatus.UNREGISTERED) { logger.debug("Attempting to teleport player `{0}` to first spawn", player.getName()); performTeleportation(player, new FirstSpawnTeleportEvent(player, firstSpawn)); } diff --git a/src/test/java/fr/xephi/authme/listener/ListenerServiceTest.java b/src/test/java/fr/xephi/authme/listener/ListenerServiceTest.java index 76877fcac..6e71ab63d 100644 --- a/src/test/java/fr/xephi/authme/listener/ListenerServiceTest.java +++ b/src/test/java/fr/xephi/authme/listener/ListenerServiceTest.java @@ -130,7 +130,7 @@ public class ListenerServiceTest { // then assertThat(result, equalTo(false)); verify(playerCache).isAuthenticated(playerName); - verify(dataSource).isAuthAvailable(playerName); + verify(playerCache).getRegistrationStatus(playerName); } @Test @@ -154,10 +154,9 @@ public class ListenerServiceTest { public void shouldAllowNpcPlayer() { // given String playerName = "other_npc"; - Player player = mockPlayerWithName(playerName); + Player player = mockPlayerWithName(playerName, true); EntityEvent event = mock(EntityEvent.class); given(event.getEntity()).willReturn(player); - given(player.hasMetadata("NPC")).willReturn(true); // when boolean result = listenerService.shouldCancelEvent(event); @@ -214,8 +213,13 @@ public class ListenerServiceTest { } private static Player mockPlayerWithName(String name) { + return mockPlayerWithName(name,false); + } + + private static Player mockPlayerWithName(String name, boolean npc) { Player player = mock(Player.class); given(player.getName()).willReturn(name); + given(player.hasMetadata("NPC")).willReturn(npc); return player; } From e437321f8fa1f1ce36906aa7a7fbead1b1c23fb9 Mon Sep 17 00:00:00 2001 From: games647 Date: Sat, 19 Jun 2021 17:53:13 +0200 Subject: [PATCH 6/7] Code clean up --- .../protocollib/ProtocolLibService.java | 5 ++-- .../authme/process/join/AsynchronousJoin.java | 25 +++++++++++-------- .../authme/service/TeleportationService.java | 3 ++- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java index 01d682ab3..8399c189a 100644 --- a/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java +++ b/src/main/java/fr/xephi/authme/listener/protocollib/ProtocolLibService.java @@ -1,20 +1,21 @@ package fr.xephi.authme.listener.protocollib; import ch.jalu.injector.annotations.NoFieldScan; + import fr.xephi.authme.AuthMe; import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.data.auth.PlayerCache; -import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.initialization.SettingsDependent; import fr.xephi.authme.listener.ListenerService; import fr.xephi.authme.output.ConsoleLoggerFactory; import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.properties.RestrictionSettings; -import org.bukkit.entity.Player; import javax.inject.Inject; +import org.bukkit.entity.Player; + @NoFieldScan public class ProtocolLibService implements SettingsDependent { 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 d715759cf..e6dafca49 100644 --- a/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java +++ b/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java @@ -117,18 +117,9 @@ public class AsynchronousJoin implements AsynchronousProcess { } final boolean isAuthAvailable = database.isAuthAvailable(name); - RegistrationStatus status = RegistrationStatus.UNREGISTERED; - playerCache.addRegistrationStatus(name, isAuthAvailable ? RegistrationStatus.REGISTERED : status); + playerCache.addRegistrationStatus(name, isAuthAvailable ? RegistrationStatus.REGISTERED : RegistrationStatus.UNREGISTERED); if (isAuthAvailable) { - // Protect inventory - if (service.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN)) { - ProtectInventoryEvent ev = bukkitService.createAndCallEvent( - isAsync -> new ProtectInventoryEvent(player, isAsync)); - if (ev.isCancelled()) { - player.updateInventory(); - logger.fine("ProtectInventoryEvent has been cancelled for " + player.getName() + "..."); - } - } + protectInventory(player); // Session logic if (sessionService.canResumeSession(player)) { @@ -160,6 +151,18 @@ public class AsynchronousJoin implements AsynchronousProcess { processJoinSync(player, isAuthAvailable); } + private void protectInventory(Player player) { + // Protect inventory + if (service.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN)) { + ProtectInventoryEvent ev = bukkitService.createAndCallEvent( + isAsync -> new ProtectInventoryEvent(player, isAsync)); + if (ev.isCancelled()) { + player.updateInventory(); + logger.fine("ProtectInventoryEvent has been cancelled for " + player.getName() + "..."); + } + } + } + private void handlePlayerWithUnmetNameRestriction(Player player, String ip) { bukkitService.scheduleSyncTaskFromOptionallyAsyncTask(() -> { player.kickPlayer(service.retrieveSingleMessage(player, MessageKey.NOT_OWNER_ERROR)); diff --git a/src/main/java/fr/xephi/authme/service/TeleportationService.java b/src/main/java/fr/xephi/authme/service/TeleportationService.java index ef84f2a41..76bae2873 100644 --- a/src/main/java/fr/xephi/authme/service/TeleportationService.java +++ b/src/main/java/fr/xephi/authme/service/TeleportationService.java @@ -113,7 +113,8 @@ public class TeleportationService implements Reloadable { return; } - if (!player.hasPlayedBefore() || playerCache.getRegistrationStatus(player.getName()) == RegistrationStatus.UNREGISTERED) { + RegistrationStatus registrationStatus = playerCache.getRegistrationStatus(player.getName()); + if (!player.hasPlayedBefore() || registrationStatus == RegistrationStatus.UNREGISTERED) { logger.debug("Attempting to teleport player `{0}` to first spawn", player.getName()); performTeleportation(player, new FirstSpawnTeleportEvent(player, firstSpawn)); } From cc2128b612984f825e633f97022e32b5078ce4e0 Mon Sep 17 00:00:00 2001 From: games647 Date: Sat, 19 Jun 2021 18:22:54 +0200 Subject: [PATCH 7/7] Protect unknown data too --- src/main/java/fr/xephi/authme/listener/ListenerService.java | 4 +++- .../java/fr/xephi/authme/process/join/AsynchronousJoin.java | 3 ++- .../java/fr/xephi/authme/listener/ListenerServiceTest.java | 2 ++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/java/fr/xephi/authme/listener/ListenerService.java b/src/main/java/fr/xephi/authme/listener/ListenerService.java index 4502a9e31..866a98c36 100644 --- a/src/main/java/fr/xephi/authme/listener/ListenerService.java +++ b/src/main/java/fr/xephi/authme/listener/ListenerService.java @@ -99,7 +99,9 @@ public class ListenerService implements SettingsDependent { } // registration not enforced, but registered players needs to be restricted if not logged in - return playerCache.getRegistrationStatus(name) == RegistrationStatus.REGISTERED; + // if there is no data fall back to safer alternative to prevent any leakage + final RegistrationStatus status = playerCache.getRegistrationStatus(name); + return status != RegistrationStatus.UNREGISTERED; } @Override 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 e6dafca49..ee54d6381 100644 --- a/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java +++ b/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java @@ -117,7 +117,8 @@ public class AsynchronousJoin implements AsynchronousProcess { } final boolean isAuthAvailable = database.isAuthAvailable(name); - playerCache.addRegistrationStatus(name, isAuthAvailable ? RegistrationStatus.REGISTERED : RegistrationStatus.UNREGISTERED); + RegistrationStatus status = isAuthAvailable ? RegistrationStatus.REGISTERED : RegistrationStatus.UNREGISTERED; + playerCache.addRegistrationStatus(name, status); if (isAuthAvailable) { protectInventory(player); diff --git a/src/test/java/fr/xephi/authme/listener/ListenerServiceTest.java b/src/test/java/fr/xephi/authme/listener/ListenerServiceTest.java index 6e71ab63d..28e17c664 100644 --- a/src/test/java/fr/xephi/authme/listener/ListenerServiceTest.java +++ b/src/test/java/fr/xephi/authme/listener/ListenerServiceTest.java @@ -4,6 +4,7 @@ import ch.jalu.injector.testing.BeforeInjecting; import ch.jalu.injector.testing.DelayedInjectionRunner; import ch.jalu.injector.testing.InjectDelayed; import fr.xephi.authme.data.auth.PlayerCache; +import fr.xephi.authme.data.auth.PlayerCache.RegistrationStatus; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.service.ValidationService; import fr.xephi.authme.settings.Settings; @@ -119,6 +120,7 @@ public class ListenerServiceTest { String playerName = "myPlayer1"; Player player = mockPlayerWithName(playerName); given(playerCache.isAuthenticated(playerName)).willReturn(false); + given(playerCache.getRegistrationStatus(playerName)).willReturn(RegistrationStatus.UNREGISTERED); given(settings.getProperty(RegistrationSettings.FORCE)).willReturn(false); EntityEvent event = mock(EntityEvent.class); given(event.getEntity()).willReturn(player);