From eb9cd31a65e064acaf607e66fc44cfacec9513ea Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sat, 10 Mar 2018 16:21:53 +0100 Subject: [PATCH] #1141 Split TOTP permissions for add/remove, refactor TOTP services - Split TotpService further into GenerateTotpService and TotpAuthenticator, which wraps the GoogleAuthenticator impl - Add missing tests for the services - Change GenerateTotpService's interface to behave like a collection for more intuitive method behavior --- .../authme/command/CommandInitializer.java | 89 +++++++------- .../executable/totp/AddTotpCommand.java | 8 +- .../executable/totp/ConfirmTotpCommand.java | 16 +-- .../executable/totp/RemoveTotpCommand.java | 2 +- .../authme/permission/PlayerPermission.java | 9 +- .../process/login/AsynchronousLogin.java | 2 +- .../fr/xephi/authme/security/TotpService.java | 113 ------------------ .../security/totp/GenerateTotpService.java | 69 +++++++++++ .../security/totp/TotpAuthenticator.java | 67 +++++++++++ .../authme/security/totp/TotpService.java | 18 +++ src/main/resources/plugin.yml | 14 ++- .../authme/security/TotpServiceTest.java | 51 -------- .../totp/GenerateTotpServiceTest.java | 113 ++++++++++++++++++ .../security/totp/TotpAuthenticatorTest.java | 84 +++++++++++++ .../authme/security/totp/TotpServiceTest.java | 46 +++++++ ...lpTranslationGeneratorIntegrationTest.java | 2 +- .../fr/xephi/authme/message/help_test.yml | 3 + 17 files changed, 478 insertions(+), 228 deletions(-) delete mode 100644 src/main/java/fr/xephi/authme/security/TotpService.java create mode 100644 src/main/java/fr/xephi/authme/security/totp/GenerateTotpService.java create mode 100644 src/main/java/fr/xephi/authme/security/totp/TotpAuthenticator.java create mode 100644 src/main/java/fr/xephi/authme/security/totp/TotpService.java delete mode 100644 src/test/java/fr/xephi/authme/security/TotpServiceTest.java create mode 100644 src/test/java/fr/xephi/authme/security/totp/GenerateTotpServiceTest.java create mode 100644 src/test/java/fr/xephi/authme/security/totp/TotpAuthenticatorTest.java create mode 100644 src/test/java/fr/xephi/authme/security/totp/TotpServiceTest.java diff --git a/src/main/java/fr/xephi/authme/command/CommandInitializer.java b/src/main/java/fr/xephi/authme/command/CommandInitializer.java index dd3cc37b..e1890626 100644 --- a/src/main/java/fr/xephi/authme/command/CommandInitializer.java +++ b/src/main/java/fr/xephi/authme/command/CommandInitializer.java @@ -59,6 +59,9 @@ import java.util.List; */ public class CommandInitializer { + private static final boolean OPTIONAL = true; + private static final boolean MANDATORY = false; + private List commands; public CommandInitializer() { @@ -88,8 +91,8 @@ public class CommandInitializer { .labels("login", "l", "log") .description("Login command") .detailedDescription("Command to log in using AuthMeReloaded.") - .withArgument("password", "Login password", false) - .withArgument("2facode", "TOTP code", true) + .withArgument("password", "Login password", MANDATORY) + .withArgument("2facode", "TOTP code", OPTIONAL) .permission(PlayerPermission.LOGIN) .executableCommand(LoginCommand.class) .register(); @@ -110,8 +113,8 @@ public class CommandInitializer { .labels("register", "reg") .description("Register an account") .detailedDescription("Command to register using AuthMeReloaded.") - .withArgument("password", "Password", true) - .withArgument("verifyPassword", "Verify password", true) + .withArgument("password", "Password", OPTIONAL) + .withArgument("verifyPassword", "Verify password", OPTIONAL) .permission(PlayerPermission.REGISTER) .executableCommand(RegisterCommand.class) .register(); @@ -122,7 +125,7 @@ public class CommandInitializer { .labels("unregister", "unreg") .description("Unregister an account") .detailedDescription("Command to unregister using AuthMeReloaded.") - .withArgument("password", "Password", false) + .withArgument("password", "Password", MANDATORY) .permission(PlayerPermission.UNREGISTER) .executableCommand(UnregisterCommand.class) .register(); @@ -133,8 +136,8 @@ public class CommandInitializer { .labels("changepassword", "changepass", "cp") .description("Change password of an account") .detailedDescription("Command to change your password using AuthMeReloaded.") - .withArgument("oldPassword", "Old password", false) - .withArgument("newPassword", "New password", false) + .withArgument("oldPassword", "Old password", MANDATORY) + .withArgument("newPassword", "New password", MANDATORY) .permission(PlayerPermission.CHANGE_PASSWORD) .executableCommand(ChangePasswordCommand.class) .register(); @@ -148,7 +151,7 @@ public class CommandInitializer { .labels("captcha") .description("Captcha command") .detailedDescription("Captcha command for AuthMeReloaded.") - .withArgument("captcha", "The Captcha", false) + .withArgument("captcha", "The Captcha", MANDATORY) .permission(PlayerPermission.CAPTCHA) .executableCommand(CaptchaCommand.class) .register(); @@ -159,7 +162,7 @@ public class CommandInitializer { .labels("verification") .description("Verification command") .detailedDescription("Command to complete the verification process for AuthMeReloaded.") - .withArgument("code", "The code", false) + .withArgument("code", "The code", MANDATORY) .permission(PlayerPermission.VERIFICATION_CODE) .executableCommand(VerificationCommand.class) .register(); @@ -191,8 +194,8 @@ public class CommandInitializer { .labels("register", "reg", "r") .description("Register a player") .detailedDescription("Register the specified player with the specified password.") - .withArgument("player", "Player name", false) - .withArgument("password", "Password", false) + .withArgument("player", "Player name", MANDATORY) + .withArgument("password", "Password", MANDATORY) .permission(AdminPermission.REGISTER) .executableCommand(RegisterAdminCommand.class) .register(); @@ -203,7 +206,7 @@ public class CommandInitializer { .labels("unregister", "unreg", "unr") .description("Unregister a player") .detailedDescription("Unregister the specified player.") - .withArgument("player", "Player name", false) + .withArgument("player", "Player name", MANDATORY) .permission(AdminPermission.UNREGISTER) .executableCommand(UnregisterAdminCommand.class) .register(); @@ -214,7 +217,7 @@ public class CommandInitializer { .labels("forcelogin", "login") .description("Enforce login player") .detailedDescription("Enforce the specified player to login.") - .withArgument("player", "Online player name", true) + .withArgument("player", "Online player name", OPTIONAL) .permission(AdminPermission.FORCE_LOGIN) .executableCommand(ForceLoginCommand.class) .register(); @@ -225,8 +228,8 @@ public class CommandInitializer { .labels("password", "changepassword", "changepass", "cp") .description("Change a player's password") .detailedDescription("Change the password of a player.") - .withArgument("player", "Player name", false) - .withArgument("pwd", "New password", false) + .withArgument("player", "Player name", MANDATORY) + .withArgument("pwd", "New password", MANDATORY) .permission(AdminPermission.CHANGE_PASSWORD) .executableCommand(ChangePasswordAdminCommand.class) .register(); @@ -237,7 +240,7 @@ public class CommandInitializer { .labels("lastlogin", "ll") .description("Player's last login") .detailedDescription("View the date of the specified players last login.") - .withArgument("player", "Player name", true) + .withArgument("player", "Player name", OPTIONAL) .permission(AdminPermission.LAST_LOGIN) .executableCommand(LastLoginCommand.class) .register(); @@ -248,7 +251,7 @@ public class CommandInitializer { .labels("accounts", "account") .description("Display player accounts") .detailedDescription("Display all accounts of a player by his player name or IP.") - .withArgument("player", "Player name or IP", true) + .withArgument("player", "Player name or IP", OPTIONAL) .permission(AdminPermission.ACCOUNTS) .executableCommand(AccountsCommand.class) .register(); @@ -259,7 +262,7 @@ public class CommandInitializer { .labels("email", "mail", "getemail", "getmail") .description("Display player's email") .detailedDescription("Display the email address of the specified player if set.") - .withArgument("player", "Player name", true) + .withArgument("player", "Player name", OPTIONAL) .permission(AdminPermission.GET_EMAIL) .executableCommand(GetEmailCommand.class) .register(); @@ -270,8 +273,8 @@ public class CommandInitializer { .labels("setemail", "setmail", "chgemail", "chgmail") .description("Change player's email") .detailedDescription("Change the email address of the specified player.") - .withArgument("player", "Player name", false) - .withArgument("email", "Player email", false) + .withArgument("player", "Player name", MANDATORY) + .withArgument("email", "Player email", MANDATORY) .permission(AdminPermission.CHANGE_EMAIL) .executableCommand(SetEmailCommand.class) .register(); @@ -282,7 +285,7 @@ public class CommandInitializer { .labels("getip", "ip") .description("Get player's IP") .detailedDescription("Get the IP address of the specified online player.") - .withArgument("player", "Player name", false) + .withArgument("player", "Player name", MANDATORY) .permission(AdminPermission.GET_IP) .executableCommand(GetIpCommand.class) .register(); @@ -333,7 +336,7 @@ public class CommandInitializer { .labels("purge", "delete") .description("Purge old data") .detailedDescription("Purge old AuthMeReloaded data longer than the specified number of days ago.") - .withArgument("days", "Number of days", false) + .withArgument("days", "Number of days", MANDATORY) .permission(AdminPermission.PURGE) .executableCommand(PurgeCommand.class) .register(); @@ -344,8 +347,8 @@ public class CommandInitializer { .labels("purgeplayer") .description("Purges the data of one player") .detailedDescription("Purges data of the given player.") - .withArgument("player", "The player to purge", false) - .withArgument("options", "'force' to run without checking if player is registered", true) + .withArgument("player", "The player to purge", MANDATORY) + .withArgument("options", "'force' to run without checking if player is registered", OPTIONAL) .permission(AdminPermission.PURGE_PLAYER) .executableCommand(PurgePlayerCommand.class) .register(); @@ -367,7 +370,7 @@ public class CommandInitializer { "resetlastposition", "resetlastpos") .description("Purge player's last position") .detailedDescription("Purge the last know position of the specified player or all of them.") - .withArgument("player/*", "Player name or * for all players", false) + .withArgument("player/*", "Player name or * for all players", MANDATORY) .permission(AdminPermission.PURGE_LAST_POSITION) .executableCommand(PurgeLastPositionCommand.class) .register(); @@ -388,7 +391,7 @@ public class CommandInitializer { .labels("switchantibot", "toggleantibot", "antibot") .description("Switch AntiBot mode") .detailedDescription("Switch or toggle the AntiBot mode to the specified state.") - .withArgument("mode", "ON / OFF", true) + .withArgument("mode", "ON / OFF", OPTIONAL) .permission(AdminPermission.SWITCH_ANTIBOT) .executableCommand(SwitchAntiBotCommand.class) .register(); @@ -419,7 +422,7 @@ public class CommandInitializer { .description("Converter command") .detailedDescription("Converter command for AuthMeReloaded.") .withArgument("job", "Conversion job: xauth / crazylogin / rakamak / " - + "royalauth / vauth / sqliteToSql / mysqlToSqlite / loginsecurity", true) + + "royalauth / vauth / sqliteToSql / mysqlToSqlite / loginsecurity", OPTIONAL) .permission(AdminPermission.CONVERTER) .executableCommand(ConverterCommand.class) .register(); @@ -447,9 +450,9 @@ public class CommandInitializer { .labels("debug", "dbg") .description("Debug features") .detailedDescription("Allows various operations for debugging.") - .withArgument("child", "The child to execute", true) - .withArgument("arg", "argument (depends on debug section)", true) - .withArgument("arg", "argument (depends on debug section)", true) + .withArgument("child", "The child to execute", OPTIONAL) + .withArgument("arg", "argument (depends on debug section)", OPTIONAL) + .withArgument("arg", "argument (depends on debug section)", OPTIONAL) .permission(DebugSectionPermissions.DEBUG_COMMAND) .executableCommand(DebugCommand.class) .register(); @@ -488,8 +491,8 @@ public class CommandInitializer { .labels("add", "addemail", "addmail") .description("Add Email") .detailedDescription("Add a new email address to your account.") - .withArgument("email", "Email address", false) - .withArgument("verifyEmail", "Email address verification", false) + .withArgument("email", "Email address", MANDATORY) + .withArgument("verifyEmail", "Email address verification", MANDATORY) .permission(PlayerPermission.ADD_EMAIL) .executableCommand(AddEmailCommand.class) .register(); @@ -500,8 +503,8 @@ public class CommandInitializer { .labels("change", "changeemail", "changemail") .description("Change Email") .detailedDescription("Change an email address of your account.") - .withArgument("oldEmail", "Old email address", false) - .withArgument("newEmail", "New email address", false) + .withArgument("oldEmail", "Old email address", MANDATORY) + .withArgument("newEmail", "New email address", MANDATORY) .permission(PlayerPermission.CHANGE_EMAIL) .executableCommand(ChangeEmailCommand.class) .register(); @@ -513,7 +516,7 @@ public class CommandInitializer { .description("Recover password using email") .detailedDescription("Recover your account using an Email address by sending a mail containing " + "a new password.") - .withArgument("email", "Email address", false) + .withArgument("email", "Email address", MANDATORY) .permission(PlayerPermission.RECOVER_EMAIL) .executableCommand(RecoverEmailCommand.class) .register(); @@ -524,7 +527,7 @@ public class CommandInitializer { .labels("code") .description("Submit code to recover password") .detailedDescription("Recover your account by submitting a code delivered to your email.") - .withArgument("code", "Recovery code", false) + .withArgument("code", "Recovery code", MANDATORY) .permission(PlayerPermission.RECOVER_EMAIL) .executableCommand(ProcessCodeCommand.class) .register(); @@ -535,7 +538,7 @@ public class CommandInitializer { .labels("setpassword") .description("Set new password after recovery") .detailedDescription("Set a new password after successfully recovering your account.") - .withArgument("password", "New password", false) + .withArgument("password", "New password", MANDATORY) .permission(PlayerPermission.RECOVER_EMAIL) .executableCommand(SetPasswordCommand.class) .register(); @@ -564,7 +567,7 @@ public class CommandInitializer { .labels("add") .description("Enables TOTP") .detailedDescription("Enables two-factor authentication for your account.") - .permission(PlayerPermission.TOGGLE_TOTP_STATUS) + .permission(PlayerPermission.ENABLE_TWO_FACTOR_AUTH) .executableCommand(AddTotpCommand.class) .register(); @@ -574,8 +577,8 @@ public class CommandInitializer { .labels("confirm") .description("Enables TOTP after successful code") .detailedDescription("Saves the generated TOTP secret after confirmation.") - .withArgument("code", "Code from the given secret from /totp add", false) - .permission(PlayerPermission.TOGGLE_TOTP_STATUS) + .withArgument("code", "Code from the given secret from /totp add", MANDATORY) + .permission(PlayerPermission.ENABLE_TWO_FACTOR_AUTH) .executableCommand(ConfirmTotpCommand.class) .register(); @@ -585,8 +588,8 @@ public class CommandInitializer { .labels("remove") .description("Removes TOTP") .detailedDescription("Disables two-factor authentication for your account.") - .withArgument("code", "Current 2FA code", false) - .permission(PlayerPermission.TOGGLE_TOTP_STATUS) + .withArgument("code", "Current 2FA code", MANDATORY) + .permission(PlayerPermission.DISABLE_TWO_FACTOR_AUTH) .executableCommand(RemoveTotpCommand.class) .register(); @@ -607,7 +610,7 @@ public class CommandInitializer { .labels(helpCommandLabels) .description("View help") .detailedDescription("View detailed help for /" + base.getLabels().get(0) + " commands.") - .withArgument("query", "The command or query to view help for.", true) + .withArgument("query", "The command or query to view help for.", OPTIONAL) .executableCommand(HelpCommand.class) .register(); } diff --git a/src/main/java/fr/xephi/authme/command/executable/totp/AddTotpCommand.java b/src/main/java/fr/xephi/authme/command/executable/totp/AddTotpCommand.java index 44eef5d8..8fc9aaf1 100644 --- a/src/main/java/fr/xephi/authme/command/executable/totp/AddTotpCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/totp/AddTotpCommand.java @@ -4,8 +4,8 @@ import fr.xephi.authme.command.PlayerCommand; import fr.xephi.authme.data.auth.PlayerAuth; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.message.MessageKey; -import fr.xephi.authme.security.TotpService; -import fr.xephi.authme.security.TotpService.TotpGenerationResult; +import fr.xephi.authme.security.totp.GenerateTotpService; +import fr.xephi.authme.security.totp.TotpAuthenticator.TotpGenerationResult; import fr.xephi.authme.service.CommonService; import org.bukkit.ChatColor; import org.bukkit.entity.Player; @@ -19,7 +19,7 @@ import java.util.List; public class AddTotpCommand extends PlayerCommand { @Inject - private TotpService totpService; + private GenerateTotpService generateTotpService; @Inject private DataSource dataSource; @@ -31,7 +31,7 @@ public class AddTotpCommand extends PlayerCommand { protected void runCommand(Player player, List arguments) { PlayerAuth auth = dataSource.getAuth(player.getName()); if (auth.getTotpKey() == null) { - TotpGenerationResult createdTotpInfo = totpService.generateTotpKey(player); + TotpGenerationResult createdTotpInfo = generateTotpService.generateTotpKey(player); commonService.send(player, MessageKey.TWO_FACTOR_CREATE, createdTotpInfo.getTotpKey(), createdTotpInfo.getAuthenticatorQrCodeUrl()); } else { diff --git a/src/main/java/fr/xephi/authme/command/executable/totp/ConfirmTotpCommand.java b/src/main/java/fr/xephi/authme/command/executable/totp/ConfirmTotpCommand.java index 1bf59f7d..34bdf56d 100644 --- a/src/main/java/fr/xephi/authme/command/executable/totp/ConfirmTotpCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/totp/ConfirmTotpCommand.java @@ -2,7 +2,8 @@ package fr.xephi.authme.command.executable.totp; import fr.xephi.authme.command.PlayerCommand; import fr.xephi.authme.datasource.DataSource; -import fr.xephi.authme.security.TotpService; +import fr.xephi.authme.security.totp.GenerateTotpService; +import fr.xephi.authme.security.totp.TotpAuthenticator.TotpGenerationResult; import org.bukkit.entity.Player; import javax.inject.Inject; @@ -14,7 +15,7 @@ import java.util.List; public class ConfirmTotpCommand extends PlayerCommand { @Inject - private TotpService totpService; + private GenerateTotpService generateTotpService; @Inject private DataSource dataSource; @@ -23,13 +24,14 @@ public class ConfirmTotpCommand extends PlayerCommand { protected void runCommand(Player player, List arguments) { // TODO #1141: Check if player already has TOTP - final String totpKey = totpService.retrieveGeneratedSecret(player); - if (totpKey == null) { + final TotpGenerationResult totpDetails = generateTotpService.getGeneratedTotpKey(player); + if (totpDetails == null) { player.sendMessage("No TOTP key has been generated for you or it has expired. Please run /totp add"); } else { - boolean isTotpCodeValid = totpService.confirmCodeForGeneratedTotpKey(player, arguments.get(0)); - if (isTotpCodeValid) { - dataSource.setTotpKey(player.getName(), totpKey); + boolean isCodeValid = generateTotpService.isTotpCodeCorrectForGeneratedTotpKey(player, arguments.get(0)); + if (isCodeValid) { + generateTotpService.removeGenerateTotpKey(player); + dataSource.setTotpKey(player.getName(), totpDetails.getTotpKey()); player.sendMessage("Successfully enabled two-factor authentication for your account"); } else { player.sendMessage("Wrong code or code has expired. Please use /totp add again"); diff --git a/src/main/java/fr/xephi/authme/command/executable/totp/RemoveTotpCommand.java b/src/main/java/fr/xephi/authme/command/executable/totp/RemoveTotpCommand.java index abf0b79c..0a2a371d 100644 --- a/src/main/java/fr/xephi/authme/command/executable/totp/RemoveTotpCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/totp/RemoveTotpCommand.java @@ -3,7 +3,7 @@ package fr.xephi.authme.command.executable.totp; import fr.xephi.authme.command.PlayerCommand; import fr.xephi.authme.data.auth.PlayerAuth; import fr.xephi.authme.datasource.DataSource; -import fr.xephi.authme.security.TotpService; +import fr.xephi.authme.security.totp.TotpService; import org.bukkit.entity.Player; import javax.inject.Inject; diff --git a/src/main/java/fr/xephi/authme/permission/PlayerPermission.java b/src/main/java/fr/xephi/authme/permission/PlayerPermission.java index 12a4a422..33b5fd1e 100644 --- a/src/main/java/fr/xephi/authme/permission/PlayerPermission.java +++ b/src/main/java/fr/xephi/authme/permission/PlayerPermission.java @@ -71,9 +71,14 @@ public enum PlayerPermission implements PermissionNode { VERIFICATION_CODE("authme.player.security.verificationcode"), /** - * Permission to enable and disable TOTP. + * Permission to enable two-factor authentication. */ - TOGGLE_TOTP_STATUS("authme.player.totp"); + ENABLE_TWO_FACTOR_AUTH("authme.player.totpadd"), + + /** + * Permission to disable two-factor authentication. + */ + DISABLE_TWO_FACTOR_AUTH("authme.player.totpremove"); /** * The permission node. diff --git a/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java b/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java index 6cadf820..629ce1aa 100644 --- a/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java +++ b/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java @@ -18,7 +18,7 @@ import fr.xephi.authme.permission.PlayerStatePermission; import fr.xephi.authme.process.AsynchronousProcess; import fr.xephi.authme.process.SyncProcessManager; import fr.xephi.authme.security.PasswordSecurity; -import fr.xephi.authme.security.TotpService; +import fr.xephi.authme.security.totp.TotpService; import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.service.CommonService; import fr.xephi.authme.service.SessionService; diff --git a/src/main/java/fr/xephi/authme/security/TotpService.java b/src/main/java/fr/xephi/authme/security/TotpService.java deleted file mode 100644 index dd8257b6..00000000 --- a/src/main/java/fr/xephi/authme/security/TotpService.java +++ /dev/null @@ -1,113 +0,0 @@ -package fr.xephi.authme.security; - -import com.warrenstrange.googleauth.GoogleAuthenticator; -import com.warrenstrange.googleauth.GoogleAuthenticatorKey; -import com.warrenstrange.googleauth.GoogleAuthenticatorQRGenerator; -import fr.xephi.authme.data.auth.PlayerAuth; -import fr.xephi.authme.initialization.HasCleanup; -import fr.xephi.authme.service.BukkitService; -import fr.xephi.authme.util.expiring.ExpiringMap; -import org.bukkit.entity.Player; - -import javax.inject.Inject; -import java.util.concurrent.TimeUnit; - -/** - * Service for TOTP actions. - */ -public class TotpService implements HasCleanup { - - private static final int NEW_TOTP_KEY_EXPIRATION_MINUTES = 5; - - private final ExpiringMap totpKeys; - private final GoogleAuthenticator authenticator; - private final BukkitService bukkitService; - - @Inject - TotpService(BukkitService bukkitService) { - this.bukkitService = bukkitService; - this.totpKeys = new ExpiringMap<>(NEW_TOTP_KEY_EXPIRATION_MINUTES, TimeUnit.MINUTES); - this.authenticator = new GoogleAuthenticator(); - } - - /** - * Generates a new TOTP key and returns the corresponding QR code. The generated key is saved temporarily - * for the user and can be later retrieved with a confirmation code from {@link #confirmCodeForGeneratedTotpKey}. - * - * @param player the player to save the TOTP key for - * @return TOTP generation result - */ - public TotpGenerationResult generateTotpKey(Player player) { - GoogleAuthenticatorKey credentials = authenticator.createCredentials(); - totpKeys.put(player.getName().toLowerCase(), credentials.getKey()); - String qrCodeUrl = GoogleAuthenticatorQRGenerator.getOtpAuthURL( - bukkitService.getIp(), player.getName(), credentials); - return new TotpGenerationResult(credentials.getKey(), qrCodeUrl); - } - - /** - * Returns the generated TOTP secret of a player, if available and not yet expired. - * - * @param player the player to retrieve the TOTP key for - * @return the totp secret - */ - public String retrieveGeneratedSecret(Player player) { - return totpKeys.get(player.getName().toLowerCase()); - } - - /** - * Returns if the new totp code matches the newly generated totp key. - * - * @param player the player to retrieve the code for - * @param totpCodeConfirmation the input code confirmation - * @return the TOTP secret that was generated for the player, or null if not available or if the code is incorrect - */ - // Maybe by allowing to retrieve without confirmation and exposing verifyCode(String, String) - public boolean confirmCodeForGeneratedTotpKey(Player player, String totpCodeConfirmation) { - String totpSecret = totpKeys.get(player.getName().toLowerCase()); - if (totpSecret != null) { - if (checkCode(totpSecret, totpCodeConfirmation)) { - totpKeys.remove(player.getName().toLowerCase()); - return true; - } - } - return false; - } - - public boolean verifyCode(PlayerAuth auth, String totpCode) { - return checkCode(auth.getTotpKey(), totpCode); - } - - private boolean checkCode(String totpKey, String inputCode) { - try { - Integer totpCode = Integer.valueOf(inputCode); - return authenticator.authorize(totpKey, totpCode); - } catch (NumberFormatException e) { - // ignore - } - return false; - } - - @Override - public void performCleanup() { - totpKeys.removeExpiredEntries(); - } - - public static final class TotpGenerationResult { - private final String totpKey; - private final String authenticatorQrCodeUrl; - - TotpGenerationResult(String totpKey, String authenticatorQrCodeUrl) { - this.totpKey = totpKey; - this.authenticatorQrCodeUrl = authenticatorQrCodeUrl; - } - - public String getTotpKey() { - return totpKey; - } - - public String getAuthenticatorQrCodeUrl() { - return authenticatorQrCodeUrl; - } - } -} diff --git a/src/main/java/fr/xephi/authme/security/totp/GenerateTotpService.java b/src/main/java/fr/xephi/authme/security/totp/GenerateTotpService.java new file mode 100644 index 00000000..14a6a6bb --- /dev/null +++ b/src/main/java/fr/xephi/authme/security/totp/GenerateTotpService.java @@ -0,0 +1,69 @@ +package fr.xephi.authme.security.totp; + +import fr.xephi.authme.initialization.HasCleanup; +import fr.xephi.authme.security.totp.TotpAuthenticator.TotpGenerationResult; +import fr.xephi.authme.util.expiring.ExpiringMap; +import org.bukkit.entity.Player; + +import javax.inject.Inject; +import java.util.concurrent.TimeUnit; + +/** + * Handles the generation of new TOTP secrets for players. + */ +public class GenerateTotpService implements HasCleanup { + + private static final int NEW_TOTP_KEY_EXPIRATION_MINUTES = 5; + + private final ExpiringMap totpKeys; + + @Inject + private TotpAuthenticator totpAuthenticator; + + GenerateTotpService() { + this.totpKeys = new ExpiringMap<>(NEW_TOTP_KEY_EXPIRATION_MINUTES, TimeUnit.MINUTES); + } + + /** + * Generates a new TOTP key and returns the corresponding QR code. + * + * @param player the player to save the TOTP key for + * @return TOTP generation result + */ + public TotpGenerationResult generateTotpKey(Player player) { + TotpGenerationResult credentials = totpAuthenticator.generateTotpKey(player); + totpKeys.put(player.getName().toLowerCase(), credentials); + return credentials; + } + + /** + * Returns the generated TOTP secret of a player, if available and not yet expired. + * + * @param player the player to retrieve the TOTP key for + * @return TOTP generation result + */ + public TotpGenerationResult getGeneratedTotpKey(Player player) { + return totpKeys.get(player.getName().toLowerCase()); + } + + public void removeGenerateTotpKey(Player player) { + totpKeys.remove(player.getName().toLowerCase()); + } + + /** + * Returns whether the given totp code is correct for the generated TOTP key of the player. + * + * @param player the player to verify the code for + * @param totpCode the totp code to verify with the generated secret + * @return true if the input code is correct, false if the code is invalid or no unexpired totp key is available + */ + public boolean isTotpCodeCorrectForGeneratedTotpKey(Player player, String totpCode) { + TotpGenerationResult totpDetails = totpKeys.get(player.getName().toLowerCase()); + return totpDetails != null && totpAuthenticator.checkCode(totpDetails.getTotpKey(), totpCode); + } + + @Override + public void performCleanup() { + totpKeys.removeExpiredEntries(); + } +} diff --git a/src/main/java/fr/xephi/authme/security/totp/TotpAuthenticator.java b/src/main/java/fr/xephi/authme/security/totp/TotpAuthenticator.java new file mode 100644 index 00000000..ed31da3a --- /dev/null +++ b/src/main/java/fr/xephi/authme/security/totp/TotpAuthenticator.java @@ -0,0 +1,67 @@ +package fr.xephi.authme.security.totp; + +import com.google.common.annotations.VisibleForTesting; +import com.warrenstrange.googleauth.GoogleAuthenticator; +import com.warrenstrange.googleauth.GoogleAuthenticatorKey; +import com.warrenstrange.googleauth.GoogleAuthenticatorQRGenerator; +import com.warrenstrange.googleauth.IGoogleAuthenticator; +import fr.xephi.authme.service.BukkitService; +import org.bukkit.entity.Player; + +import javax.inject.Inject; + +/** + * Provides rudimentary TOTP functions (wraps third-party TOTP implementation). + */ +public class TotpAuthenticator { + + private final IGoogleAuthenticator authenticator; + private final BukkitService bukkitService; + + + @Inject + TotpAuthenticator(BukkitService bukkitService) { + this(new GoogleAuthenticator(), bukkitService); + } + + @VisibleForTesting + TotpAuthenticator(IGoogleAuthenticator authenticator, BukkitService bukkitService) { + this.authenticator = authenticator; + this.bukkitService = bukkitService; + } + + public boolean checkCode(String totpKey, String inputCode) { + try { + Integer totpCode = Integer.valueOf(inputCode); + return authenticator.authorize(totpKey, totpCode); + } catch (NumberFormatException e) { + // ignore + } + return false; + } + + public TotpGenerationResult generateTotpKey(Player player) { + GoogleAuthenticatorKey credentials = authenticator.createCredentials(); + String qrCodeUrl = GoogleAuthenticatorQRGenerator.getOtpAuthURL( + bukkitService.getIp(), player.getName(), credentials); + return new TotpGenerationResult(credentials.getKey(), qrCodeUrl); + } + + public static final class TotpGenerationResult { + private final String totpKey; + private final String authenticatorQrCodeUrl; + + TotpGenerationResult(String totpKey, String authenticatorQrCodeUrl) { + this.totpKey = totpKey; + this.authenticatorQrCodeUrl = authenticatorQrCodeUrl; + } + + public String getTotpKey() { + return totpKey; + } + + public String getAuthenticatorQrCodeUrl() { + return authenticatorQrCodeUrl; + } + } +} diff --git a/src/main/java/fr/xephi/authme/security/totp/TotpService.java b/src/main/java/fr/xephi/authme/security/totp/TotpService.java new file mode 100644 index 00000000..15e3381f --- /dev/null +++ b/src/main/java/fr/xephi/authme/security/totp/TotpService.java @@ -0,0 +1,18 @@ +package fr.xephi.authme.security.totp; + +import fr.xephi.authme.data.auth.PlayerAuth; + +import javax.inject.Inject; + +/** + * Service for TOTP actions. + */ +public class TotpService { + + @Inject + private TotpAuthenticator totpAuthenticator; + + public boolean verifyCode(PlayerAuth auth, String totpCode) { + return totpAuthenticator.checkCode(auth.getTotpKey(), totpCode); + } +} diff --git a/src/main/resources/plugin.yml b/src/main/resources/plugin.yml index e60721de..bee65078 100644 --- a/src/main/resources/plugin.yml +++ b/src/main/resources/plugin.yml @@ -23,7 +23,7 @@ commands: usage: /email show|add|change|recover|code|setpassword login: description: Login command - usage: /login + usage: /login [2facode] aliases: - l - log @@ -48,7 +48,7 @@ commands: - cp totp: description: TOTP commands - usage: /totp add|remove + usage: /totp add|confirm|remove aliases: - 2fa captcha: @@ -238,7 +238,8 @@ permissions: authme.player.register: true authme.player.security.verificationcode: true authme.player.seeownaccounts: true - authme.player.totp: true + authme.player.totpadd: true + authme.player.totpremove: true authme.player.unregister: true authme.player.canbeforced: description: Permission for users a login can be forced to. @@ -283,8 +284,11 @@ permissions: authme.player.seeownaccounts: description: Permission to use to see own other accounts. default: true - authme.player.totp: - description: Permission to enable and disable TOTP. + authme.player.totpadd: + description: Permission to enable two-factor authentication. + default: true + authme.player.totpremove: + description: Permission to disable two-factor authentication. default: true authme.player.unregister: description: Command permission to unregister. diff --git a/src/test/java/fr/xephi/authme/security/TotpServiceTest.java b/src/test/java/fr/xephi/authme/security/TotpServiceTest.java deleted file mode 100644 index 6350b4c4..00000000 --- a/src/test/java/fr/xephi/authme/security/TotpServiceTest.java +++ /dev/null @@ -1,51 +0,0 @@ -package fr.xephi.authme.security; - -import fr.xephi.authme.security.TotpService.TotpGenerationResult; -import fr.xephi.authme.service.BukkitService; -import org.bukkit.entity.Player; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; - -import static fr.xephi.authme.AuthMeMatchers.stringWithLength; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.startsWith; -import static org.junit.Assert.assertThat; -import static org.mockito.BDDMockito.given; -import static org.mockito.Mockito.mock; - -/** - * Test for {@link TotpService}. - */ -@RunWith(MockitoJUnitRunner.class) -public class TotpServiceTest { - - @InjectMocks - private TotpService totpService; - - @Mock - private BukkitService bukkitService; - - @Test - public void shouldGenerateTotpKey() { - // given - Player player = mock(Player.class); - given(player.getName()).willReturn("Bobby"); - given(bukkitService.getIp()).willReturn("127.48.44.4"); - - // when - TotpGenerationResult key1 = totpService.generateTotpKey(player); - TotpGenerationResult key2 = totpService.generateTotpKey(player); - - // then - assertThat(key1.getTotpKey(), stringWithLength(16)); - assertThat(key2.getTotpKey(), stringWithLength(16)); - assertThat(key1.getAuthenticatorQrCodeUrl(), startsWith("https://chart.googleapis.com/chart?chs=200x200")); - assertThat(key2.getAuthenticatorQrCodeUrl(), startsWith("https://chart.googleapis.com/chart?chs=200x200")); - assertThat(key1.getTotpKey(), not(equalTo(key2.getTotpKey()))); - } - -} diff --git a/src/test/java/fr/xephi/authme/security/totp/GenerateTotpServiceTest.java b/src/test/java/fr/xephi/authme/security/totp/GenerateTotpServiceTest.java new file mode 100644 index 00000000..1a22d26e --- /dev/null +++ b/src/test/java/fr/xephi/authme/security/totp/GenerateTotpServiceTest.java @@ -0,0 +1,113 @@ +package fr.xephi.authme.security.totp; + +import fr.xephi.authme.ReflectionTestUtils; +import fr.xephi.authme.security.totp.TotpAuthenticator.TotpGenerationResult; +import fr.xephi.authme.util.expiring.ExpiringMap; +import org.bukkit.entity.Player; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import java.util.concurrent.TimeUnit; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +/** + * Test for {@link GenerateTotpService}. + */ +@RunWith(MockitoJUnitRunner.class) +public class GenerateTotpServiceTest { + + @InjectMocks + private GenerateTotpService generateTotpService; + + @Mock + private TotpAuthenticator totpAuthenticator; + + @Test + public void shouldGenerateTotpKey() { + // given + TotpGenerationResult givenGenerationResult = new TotpGenerationResult("1234", "http://example.com/link/to/chart"); + Player player = mockPlayerWithName("Spencer"); + given(totpAuthenticator.generateTotpKey(player)).willReturn(givenGenerationResult); + + // when + TotpGenerationResult result = generateTotpService.generateTotpKey(player); + + // then + assertThat(result, equalTo(givenGenerationResult)); + assertThat(generateTotpService.getGeneratedTotpKey(player), equalTo(givenGenerationResult)); + } + + @Test + public void shouldRemoveGeneratedTotpKey() { + // given + TotpGenerationResult givenGenerationResult = new TotpGenerationResult("1234", "http://example.com/link/to/chart"); + Player player = mockPlayerWithName("Hanna"); + given(totpAuthenticator.generateTotpKey(player)).willReturn(givenGenerationResult); + generateTotpService.generateTotpKey(player); + + // when + generateTotpService.removeGenerateTotpKey(player); + + // then + assertThat(generateTotpService.getGeneratedTotpKey(player), nullValue()); + } + + @Test + public void shouldCheckGeneratedTotpKey() { + // given + String generatedKey = "ASLO43KDF2J"; + TotpGenerationResult givenGenerationResult = new TotpGenerationResult(generatedKey, "url"); + Player player = mockPlayerWithName("Aria"); + given(totpAuthenticator.generateTotpKey(player)).willReturn(givenGenerationResult); + generateTotpService.generateTotpKey(player); + String validCode = "928374"; + given(totpAuthenticator.checkCode(generatedKey, validCode)).willReturn(true); + + // when + boolean invalidCodeResult = generateTotpService.isTotpCodeCorrectForGeneratedTotpKey(player, "000000"); + boolean validCodeResult = generateTotpService.isTotpCodeCorrectForGeneratedTotpKey(player, validCode); + boolean unknownPlayerResult = generateTotpService.isTotpCodeCorrectForGeneratedTotpKey(mockPlayerWithName("other"), "299874"); + + // then + assertThat(invalidCodeResult, equalTo(false)); + assertThat(validCodeResult, equalTo(true)); + assertThat(unknownPlayerResult, equalTo(false)); + verify(totpAuthenticator).checkCode(generatedKey, "000000"); + verify(totpAuthenticator).checkCode(generatedKey, validCode); + } + + @Test + public void shouldRemoveExpiredEntries() throws InterruptedException { + // given + TotpGenerationResult generationResult = new TotpGenerationResult("key", "url"); + ExpiringMap generatedKeys = + ReflectionTestUtils.getFieldValue(GenerateTotpService.class, generateTotpService, "totpKeys"); + generatedKeys.setExpiration(1, TimeUnit.MILLISECONDS); + generatedKeys.put("ghost", generationResult); + generatedKeys.setExpiration(5, TimeUnit.MINUTES); + generatedKeys.put("ezra", generationResult); + + // when + Thread.sleep(2L); + generateTotpService.performCleanup(); + + // then + assertThat(generateTotpService.getGeneratedTotpKey(mockPlayerWithName("Ezra")), equalTo(generationResult)); + assertThat(generateTotpService.getGeneratedTotpKey(mockPlayerWithName("ghost")), nullValue()); + } + + private static Player mockPlayerWithName(String name) { + Player player = mock(Player.class); + given(player.getName()).willReturn(name); + return player; + } +} diff --git a/src/test/java/fr/xephi/authme/security/totp/TotpAuthenticatorTest.java b/src/test/java/fr/xephi/authme/security/totp/TotpAuthenticatorTest.java new file mode 100644 index 00000000..eb2746fa --- /dev/null +++ b/src/test/java/fr/xephi/authme/security/totp/TotpAuthenticatorTest.java @@ -0,0 +1,84 @@ +package fr.xephi.authme.security.totp; + +import com.warrenstrange.googleauth.IGoogleAuthenticator; +import fr.xephi.authme.security.totp.TotpAuthenticator.TotpGenerationResult; +import fr.xephi.authme.service.BukkitService; +import org.bukkit.entity.Player; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import static fr.xephi.authme.AuthMeMatchers.stringWithLength; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.startsWith; +import static org.junit.Assert.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; + +/** + * Test for {@link TotpAuthenticator}. + */ +@RunWith(MockitoJUnitRunner.class) +public class TotpAuthenticatorTest { + + @InjectMocks + private TotpAuthenticator totpAuthenticator; + + @Mock + private BukkitService bukkitService; + + @Mock + private IGoogleAuthenticator googleAuthenticator; + + @Test + public void shouldGenerateTotpKey() { + // given + // Use the GoogleAuthenticator instance the TotpAuthenticator normally creates to test its parameters + totpAuthenticator = new TotpAuthenticator(bukkitService); + + Player player = mock(Player.class); + given(player.getName()).willReturn("Bobby"); + given(bukkitService.getIp()).willReturn("127.48.44.4"); + + // when + TotpGenerationResult key1 = totpAuthenticator.generateTotpKey(player); + TotpGenerationResult key2 = totpAuthenticator.generateTotpKey(player); + + // then + assertThat(key1.getTotpKey(), stringWithLength(16)); + assertThat(key2.getTotpKey(), stringWithLength(16)); + assertThat(key1.getAuthenticatorQrCodeUrl(), startsWith("https://chart.googleapis.com/chart?chs=200x200")); + assertThat(key2.getAuthenticatorQrCodeUrl(), startsWith("https://chart.googleapis.com/chart?chs=200x200")); + assertThat(key1.getTotpKey(), not(equalTo(key2.getTotpKey()))); + } + + @Test + public void shouldCheckCode() { + // given + String secret = "the_secret"; + int code = 21398; + given(googleAuthenticator.authorize(secret, code)).willReturn(true); + + // when + boolean result = totpAuthenticator.checkCode(secret, Integer.toString(code)); + + // then + assertThat(result, equalTo(true)); + verify(googleAuthenticator).authorize(secret, code); + } + + @Test + public void shouldHandleInvalidNumberInput() { + // given / when + boolean result = totpAuthenticator.checkCode("Some_Secret", "123ZZ"); + + // then + assertThat(result, equalTo(false)); + verifyZeroInteractions(googleAuthenticator); + } +} diff --git a/src/test/java/fr/xephi/authme/security/totp/TotpServiceTest.java b/src/test/java/fr/xephi/authme/security/totp/TotpServiceTest.java new file mode 100644 index 00000000..7d321cf1 --- /dev/null +++ b/src/test/java/fr/xephi/authme/security/totp/TotpServiceTest.java @@ -0,0 +1,46 @@ +package fr.xephi.authme.security.totp; + +import fr.xephi.authme.data.auth.PlayerAuth; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.verify; + +/** + * Test for {@link TotpService}. + */ +@RunWith(MockitoJUnitRunner.class) +public class TotpServiceTest { + + @InjectMocks + private TotpService totpService; + + @Mock + private TotpAuthenticator totpAuthenticator; + + @Test + public void shouldVerifyCode() { + // given + String totpKey = "ASLO43KDF2J"; + PlayerAuth auth = PlayerAuth.builder() + .name("Maya") + .totpKey(totpKey) + .build(); + String inputCode = "408435"; + given(totpAuthenticator.checkCode(totpKey, inputCode)).willReturn(true); + + // when + boolean result = totpService.verifyCode(auth, inputCode); + + // then + assertThat(result, equalTo(true)); + verify(totpAuthenticator).checkCode(totpKey, inputCode); + } + +} diff --git a/src/test/java/fr/xephi/authme/service/HelpTranslationGeneratorIntegrationTest.java b/src/test/java/fr/xephi/authme/service/HelpTranslationGeneratorIntegrationTest.java index 3e094cc2..9159b424 100644 --- a/src/test/java/fr/xephi/authme/service/HelpTranslationGeneratorIntegrationTest.java +++ b/src/test/java/fr/xephi/authme/service/HelpTranslationGeneratorIntegrationTest.java @@ -126,7 +126,7 @@ public class HelpTranslationGeneratorIntegrationTest { // Check /login checkDescription(configuration.get("commands.login"), "Login command", "/login detailed desc."); - checkArgs(configuration.get("commands.login"), arg("loginArg", "Login password")); + checkArgs(configuration.get("commands.login"), arg("loginArg", "Login password"), arg("2faArg", "TOTP code")); // Check /unregister checkDescription(configuration.get("commands.unregister"), "unreg_desc", "unreg_detail_desc"); diff --git a/src/test/resources/fr/xephi/authme/message/help_test.yml b/src/test/resources/fr/xephi/authme/message/help_test.yml index d6970daa..9647ecd7 100644 --- a/src/test/resources/fr/xephi/authme/message/help_test.yml +++ b/src/test/resources/fr/xephi/authme/message/help_test.yml @@ -65,6 +65,9 @@ commands: arg1: label: loginArg nonExistent: does not exist + arg2: + label: 2faArg + rhetorical: false someProp: label: '''someProp'' does not exist' description: idk