#761 Fix removal and restoration of primary permission group

- Improve how a player is being switched between permission groups (add new group before removing old one)
- Remove group handling logic from LimboCache: AuthGroupHandler is now solely responsible for changing the player's permission group
This commit is contained in:
ljacqu 2017-02-05 13:12:04 +01:00
parent 49f7e47645
commit 2b1a97e959
6 changed files with 78 additions and 84 deletions

View File

@ -1,10 +1,8 @@
package fr.xephi.authme.data.limbo;
import fr.xephi.authme.ConsoleLogger;
import fr.xephi.authme.permission.PermissionsManager;
import fr.xephi.authme.settings.Settings;
import fr.xephi.authme.settings.SpawnLoader;
import fr.xephi.authme.settings.properties.PluginSettings;
import fr.xephi.authme.util.StringUtils;
import org.bukkit.Location;
import org.bukkit.entity.Player;
@ -22,14 +20,11 @@ public class LimboCache {
private final Map<String, LimboPlayer> cache = new ConcurrentHashMap<>();
private LimboPlayerStorage limboPlayerStorage;
private Settings settings;
private PermissionsManager permissionsManager;
private SpawnLoader spawnLoader;
@Inject
LimboCache(Settings settings, PermissionsManager permissionsManager,
SpawnLoader spawnLoader, LimboPlayerStorage limboPlayerStorage) {
this.settings = settings;
LimboCache(PermissionsManager permissionsManager, SpawnLoader spawnLoader, LimboPlayerStorage limboPlayerStorage) {
this.permissionsManager = permissionsManager;
this.spawnLoader = spawnLoader;
this.limboPlayerStorage = limboPlayerStorage;
@ -51,6 +46,7 @@ public class LimboCache {
if (permissionsManager.hasGroupSupport()) {
playerGroup = permissionsManager.getPrimaryGroup(player);
}
ConsoleLogger.debug("Player `{0}` has primary group `{1}`", player.getName(), playerGroup);
if (limboPlayerStorage.hasData(player)) {
LimboPlayer cache = limboPlayerStorage.readData(player);
@ -83,15 +79,14 @@ public class LimboCache {
float walkSpeed = data.getWalkSpeed();
float flySpeed = data.getFlySpeed();
// Reset the speed value if it was 0
if(walkSpeed < 0.01f) {
if (walkSpeed < 0.01f) {
walkSpeed = 0.2f;
}
if(flySpeed < 0.01f) {
if (flySpeed < 0.01f) {
flySpeed = 0.2f;
}
player.setWalkSpeed(walkSpeed);
player.setFlySpeed(flySpeed);
restoreGroup(player, data.getGroup());
data.clearTasks();
}
}
@ -153,11 +148,4 @@ public class LimboCache {
removeFromCache(player);
addPlayerData(player);
}
private void restoreGroup(Player player, String group) {
if (!StringUtils.isEmpty(group) && permissionsManager.hasGroupSupport()
&& settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)) {
permissionsManager.setGroup(player, group);
}
}
}

View File

@ -13,6 +13,15 @@ import javax.inject.Inject;
/**
* Changes the permission group according to the auth status of the player and the configuration.
* <p>
* If this feature is enabled, the <i>primary permissions group</i> of a player is replaced until he has
* logged in. Some permission plugins have a notion of a primary group; for other permission plugins the
* first group is simply taken.
* <p>
* The groups that are used as replacement until the player logs in is configurable and depends on if
* the player is registered or not. Note that some (all?) permission systems require the group to actually
* exist for the replacement to take place. Furthermore, since some permission groups require that players
* be in at least one group, this will mean that the player is not removed from his primary group.
*/
public class AuthGroupHandler implements Reloadable {
@ -36,11 +45,49 @@ public class AuthGroupHandler implements Reloadable {
*
* @param player the player
* @param groupType the group type
*
* @return True upon success, false otherwise. False is also returned if groups aren't supported
* with the current permissions system.
*/
public boolean setGroup(Player player, AuthGroupType groupType) {
public void setGroup(Player player, AuthGroupType groupType) {
if (!useAuthGroups()) {
return;
}
String primaryGroup = "";
LimboPlayer limboPlayer = limboCache.getPlayerData(player.getName());
if (limboPlayer != null) {
primaryGroup = limboPlayer.getGroup();
}
switch (groupType) {
// Implementation note: some permission systems don't support players not being in any group,
// so add the new group before removing the old ones
case UNREGISTERED:
permissionsManager.addGroup(player, unregisteredGroup);
permissionsManager.removeGroups(player, registeredGroup, primaryGroup);
break;
case REGISTERED_UNAUTHENTICATED:
permissionsManager.addGroup(player, registeredGroup);
permissionsManager.removeGroups(player, unregisteredGroup, primaryGroup);
break;
case LOGGED_IN:
restoreGroup(player);
break;
default:
throw new IllegalStateException("Encountered unhandled auth group type '" + groupType + "'");
}
ConsoleLogger.debug(
() -> player.getName() + " changed to " + groupType + ": has groups " + permissionsManager.getGroups(player));
}
/**
* Returns whether the auth permissions group function should be used.
*
* @return true if should be used, false otherwise
*/
private boolean useAuthGroups() {
// Check whether the permissions check is enabled
if (!settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)) {
return false;
@ -51,39 +98,21 @@ public class AuthGroupHandler implements Reloadable {
ConsoleLogger.warning("The current permissions system doesn't have group support, unable to set group!");
return false;
}
switch (groupType) {
case UNREGISTERED:
// Remove the other group, set the current group
permissionsManager.removeGroups(player, registeredGroup);
return permissionsManager.addGroup(player, unregisteredGroup);
case REGISTERED_UNAUTHENTICATED:
// Remove the other group, set the current group
permissionsManager.removeGroups(player, unregisteredGroup);
return permissionsManager.addGroup(player, registeredGroup);
case LOGGED_IN:
return restoreGroup(player);
default:
throw new IllegalStateException("Encountered unhandled auth group type '" + groupType + "'");
}
return true;
}
private boolean restoreGroup(Player player) {
// Get the player's LimboPlayer
/**
* Restores the player's original primary group (taken from LimboPlayer).
*
* @param player the player to process
*/
private void restoreGroup(Player player) {
LimboPlayer limbo = limboCache.getPlayerData(player.getName());
if (limbo == null) {
return false;
if (limbo != null) {
String primaryGroup = limbo.getGroup();
permissionsManager.addGroup(player, primaryGroup);
}
// Get the players group
String realGroup = limbo.getGroup();
// Remove the other group types groups, set the real group
permissionsManager.removeGroups(player, unregisteredGroup, registeredGroup);
return permissionsManager.addGroup(player, realGroup);
}
@Override

View File

@ -1,6 +1,5 @@
package fr.xephi.authme.process.login;
import fr.xephi.authme.AuthMe;
import fr.xephi.authme.data.auth.PlayerAuth;
import fr.xephi.authme.data.limbo.LimboCache;
import fr.xephi.authme.data.limbo.LimboPlayer;
@ -8,17 +7,17 @@ import fr.xephi.authme.datasource.DataSource;
import fr.xephi.authme.events.LoginEvent;
import fr.xephi.authme.events.RestoreInventoryEvent;
import fr.xephi.authme.listener.PlayerListener;
import fr.xephi.authme.permission.AuthGroupType;
import fr.xephi.authme.process.SynchronousProcess;
import fr.xephi.authme.service.BukkitService;
import fr.xephi.authme.service.BungeeService;
import fr.xephi.authme.service.CommonService;
import fr.xephi.authme.service.TeleportationService;
import fr.xephi.authme.settings.Settings;
import fr.xephi.authme.settings.WelcomeMessageConfiguration;
import fr.xephi.authme.settings.commandconfig.CommandManager;
import fr.xephi.authme.settings.properties.RegistrationSettings;
import fr.xephi.authme.util.StringUtils;
import org.bukkit.entity.Player;
import org.bukkit.plugin.PluginManager;
import org.bukkit.potion.PotionEffectType;
import javax.inject.Inject;
@ -28,9 +27,6 @@ import static fr.xephi.authme.settings.properties.RestrictionSettings.PROTECT_IN
public class ProcessSyncPlayerLogin implements SynchronousProcess {
@Inject
private AuthMe plugin;
@Inject
private BungeeService bungeeService;
@ -40,9 +36,6 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess {
@Inject
private BukkitService bukkitService;
@Inject
private PluginManager pluginManager;
@Inject
private TeleportationService teleportationService;
@ -53,7 +46,7 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess {
private CommandManager commandManager;
@Inject
private Settings settings;
private CommonService commonService;
@Inject
private WelcomeMessageConfiguration welcomeMessageConfiguration;
@ -63,7 +56,7 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess {
private void restoreInventory(Player player) {
RestoreInventoryEvent event = new RestoreInventoryEvent(player);
pluginManager.callEvent(event);
bukkitService.callEvent(event);
if (!event.isCancelled()) {
player.updateInventory();
}
@ -80,8 +73,9 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess {
// do we really need to use location from database for now?
// because LimboCache#restoreData teleport player to last location.
}
commonService.setGroup(player, AuthGroupType.LOGGED_IN);
if (settings.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN)) {
if (commonService.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN)) {
restoreInventory(player);
}
@ -98,7 +92,7 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess {
}
}
if (settings.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)) {
if (commonService.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)) {
player.removePotionEffect(PotionEffectType.BLINDNESS);
}
@ -108,8 +102,8 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess {
// Login is done, display welcome message
List<String> welcomeMessage = welcomeMessageConfiguration.getWelcomeMessage(player);
if (settings.getProperty(RegistrationSettings.USE_WELCOME_MESSAGE)) {
if (settings.getProperty(RegistrationSettings.BROADCAST_WELCOME_MESSAGE)) {
if (commonService.getProperty(RegistrationSettings.USE_WELCOME_MESSAGE)) {
if (commonService.getProperty(RegistrationSettings.BROADCAST_WELCOME_MESSAGE)) {
welcomeMessage.forEach(bukkitService::broadcastMessage);
} else {
welcomeMessage.forEach(player::sendMessage);

View File

@ -101,10 +101,9 @@ public class CommonService {
*
* @param player the player to process
* @param group the group to add the player to
* @return true on success, false otherwise
*/
public boolean setGroup(Player player, AuthGroupType group) {
return authGroupHandler.setGroup(player, group);
public void setGroup(Player player, AuthGroupType group) {
authGroupHandler.setGroup(player, group);
}
}

View File

@ -2,9 +2,7 @@ package fr.xephi.authme.data.limbo;
import fr.xephi.authme.ReflectionTestUtils;
import fr.xephi.authme.permission.PermissionsManager;
import fr.xephi.authme.settings.Settings;
import fr.xephi.authme.settings.SpawnLoader;
import fr.xephi.authme.settings.properties.PluginSettings;
import org.bukkit.Location;
import org.bukkit.entity.Player;
import org.junit.Test;
@ -33,9 +31,6 @@ public class LimboCacheTest {
@InjectMocks
private LimboCache limboCache;
@Mock
private Settings settings;
@Mock
private PermissionsManager permissionsManager;
@ -118,11 +113,7 @@ public class LimboCacheTest {
given(limboPlayer.isCanFly()).willReturn(true);
float flySpeed = 1.0f;
given(limboPlayer.getFlySpeed()).willReturn(flySpeed);
String group = "primary-group";
given(limboPlayer.getGroup()).willReturn(group);
getCache().put(name.toLowerCase(), limboPlayer);
given(settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)).willReturn(true);
given(permissionsManager.hasGroupSupport()).willReturn(true);
// when
limboCache.restoreData(player);
@ -132,7 +123,6 @@ public class LimboCacheTest {
verify(player).setWalkSpeed(walkSpeed);
verify(player).setAllowFlight(true);
verify(player).setFlySpeed(flySpeed);
verify(permissionsManager).setGroup(player, group);
verify(limboPlayer).clearTasks();
}
@ -147,11 +137,7 @@ public class LimboCacheTest {
given(limboPlayer.getWalkSpeed()).willReturn(0f);
given(limboPlayer.isCanFly()).willReturn(true);
given(limboPlayer.getFlySpeed()).willReturn(0f);
String group = "primary-group";
given(limboPlayer.getGroup()).willReturn(group);
getCache().put(name.toLowerCase(), limboPlayer);
given(settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)).willReturn(true);
given(permissionsManager.hasGroupSupport()).willReturn(true);
// when
limboCache.restoreData(player);

View File

@ -134,13 +134,11 @@ public class CommonServiceTest {
// given
Player player = mock(Player.class);
AuthGroupType type = AuthGroupType.LOGGED_IN;
given(authGroupHandler.setGroup(player, type)).willReturn(true);
// when
boolean result = commonService.setGroup(player, type);
commonService.setGroup(player, type);
// then
verify(authGroupHandler).setGroup(player, type);
assertThat(result, equalTo(true));
}
}