#1141 2FA implementation fixes

- Merge TotpService into TotpAuthenticator
- Add missing tests
- Migrate old 2fa enabled key to new one

src/main/java/fr/xephi/authme/command/executable/totp/ConfirmTotpCommand.java

@@ -33,13 +33,17 @@ public class ConfirmTotpCommand extends PlayerCommand {
             messages.send(player, MessageKey.REGISTER_MESSAGE);
         } else if (auth.getTotpKey() != null) {
             messages.send(player, MessageKey.TWO_FACTOR_ALREADY_ENABLED);
+        } else {
+            verifyTotpCodeConfirmation(player, arguments.get(0));
         }
+    }
 
+    private void verifyTotpCodeConfirmation(Player player, String inputTotpCode) {
         final TotpGenerationResult totpDetails = generateTotpService.getGeneratedTotpKey(player);
         if (totpDetails == null) {
             messages.send(player, MessageKey.TWO_FACTOR_ENABLE_ERROR_NO_CODE);
         } else {
-            boolean isCodeValid = generateTotpService.isTotpCodeCorrectForGeneratedTotpKey(player, arguments.get(0));
+            boolean isCodeValid = generateTotpService.isTotpCodeCorrectForGeneratedTotpKey(player, inputTotpCode);
             if (isCodeValid) {
                 generateTotpService.removeGenerateTotpKey(player);
                 dataSource.setTotpKey(player.getName(), totpDetails.getTotpKey());

src/main/java/fr/xephi/authme/command/executable/totp/RemoveTotpCommand.java

@@ -5,7 +5,7 @@ import fr.xephi.authme.data.auth.PlayerAuth;
 import fr.xephi.authme.datasource.DataSource;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.message.Messages;
-import fr.xephi.authme.security.totp.TotpService;
+import fr.xephi.authme.security.totp.TotpAuthenticator;
 import org.bukkit.entity.Player;
 
 import javax.inject.Inject;
@@ -20,7 +20,7 @@ public class RemoveTotpCommand extends PlayerCommand {
     private DataSource dataSource;
 
     @Inject
-    private TotpService totpService;
+    private TotpAuthenticator totpAuthenticator;
 
     @Inject
     private Messages messages;
@@ -31,7 +31,7 @@ public class RemoveTotpCommand extends PlayerCommand {
         if (auth.getTotpKey() == null) {
             messages.send(player, MessageKey.TWO_FACTOR_NOT_ENABLED_ERROR);
         } else {
-            if (totpService.verifyCode(auth, arguments.get(0))) {
+            if (totpAuthenticator.checkCode(auth, arguments.get(0))) {
                 dataSource.removeTotpKey(auth.getNickname());
                 messages.send(player, MessageKey.TWO_FACTOR_REMOVED_SUCCESS);
             } else {

src/main/java/fr/xephi/authme/command/executable/totp/TotpCodeCommand.java

@@ -10,7 +10,7 @@ import fr.xephi.authme.datasource.DataSource;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.message.Messages;
 import fr.xephi.authme.process.login.AsynchronousLogin;
-import fr.xephi.authme.security.totp.TotpService;
+import fr.xephi.authme.security.totp.TotpAuthenticator;
 import org.bukkit.entity.Player;
 
 import javax.inject.Inject;
@@ -31,7 +31,7 @@ public class TotpCodeCommand extends PlayerCommand {
     private Messages messages;
 
     @Inject
-    private TotpService totpService;
+    private TotpAuthenticator totpAuthenticator;
 
     @Inject
     private DataSource dataSource;
@@ -61,7 +61,7 @@ public class TotpCodeCommand extends PlayerCommand {
     }
 
     private void processCode(Player player, PlayerAuth auth, String inputCode) {
-        boolean isCodeValid = totpService.verifyCode(auth, inputCode);
+        boolean isCodeValid = totpAuthenticator.checkCode(auth, inputCode);
         if (isCodeValid) {
             asynchronousLogin.performLogin(player, auth);
         } else {

src/main/java/fr/xephi/authme/message/updater/MessageUpdater.java

@@ -5,7 +5,7 @@ import ch.jalu.configme.configurationdata.ConfigurationData;
 import ch.jalu.configme.configurationdata.PropertyListBuilder;
 import ch.jalu.configme.properties.Property;
 import ch.jalu.configme.properties.StringProperty;
-import ch.jalu.configme.resource.YamlFileResource;
+import ch.jalu.configme.resource.PropertyResource;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.io.Files;
 import fr.xephi.authme.ConsoleLogger;
@@ -57,14 +57,16 @@ public class MessageUpdater {
      */
     private boolean migrateAndSave(File userFile, JarMessageSource jarMessageSource) {
         // YamlConfiguration escapes all special characters when saving, making the file hard to use, so use ConfigMe
-        YamlFileResource userResource = new MigraterYamlFileResource(userFile);
+        PropertyResource userResource = new MigraterYamlFileResource(userFile);
 
         // Step 1: Migrate any old keys in the file to the new paths
         boolean movedOldKeys = migrateOldKeys(userResource);
-        // Step 2: Take any missing messages from the message files shipped in the AuthMe JAR
+        // Step 2: Perform newer migrations
+        boolean movedNewerKeys = migrateKeys(userResource);
+        // Step 3: Take any missing messages from the message files shipped in the AuthMe JAR
         boolean addedMissingKeys = addMissingKeys(jarMessageSource, userResource);
 
-        if (movedOldKeys || addedMissingKeys) {
+        if (movedOldKeys || movedNewerKeys || addedMissingKeys) {
             backupMessagesFile(userFile);
 
             SettingsManager settingsManager = new SettingsManager(userResource, null, CONFIGURATION_DATA);
@@ -75,7 +77,19 @@ public class MessageUpdater {
         return false;
     }
 
-    private boolean migrateOldKeys(YamlFileResource userResource) {
+    private boolean migrateKeys(PropertyResource userResource) {
+        return moveIfApplicable(userResource, "misc.two_factor_create", MessageKey.TWO_FACTOR_CREATE.getKey());
+    }
+
+    private static boolean moveIfApplicable(PropertyResource resource, String oldPath, String newPath) {
+        if (resource.getString(newPath) == null && resource.getString(oldPath) != null) {
+            resource.setValue(newPath, resource.getString(oldPath));
+            return true;
+        }
+        return false;
+    }
+
+    private boolean migrateOldKeys(PropertyResource userResource) {
         boolean hasChange = OldMessageKeysMigrater.migrateOldPaths(userResource);
         if (hasChange) {
             ConsoleLogger.info("Old keys have been moved to the new ones in your messages_xx.yml file");
@@ -83,7 +97,7 @@ public class MessageUpdater {
         return hasChange;
     }
 
-    private boolean addMissingKeys(JarMessageSource jarMessageSource, YamlFileResource userResource) {
+    private boolean addMissingKeys(JarMessageSource jarMessageSource, PropertyResource userResource) {
         List<String> addedKeys = new ArrayList<>();
         for (Property<?> property : CONFIGURATION_DATA.getProperties()) {
             final String key = property.getPath();

src/main/java/fr/xephi/authme/security/totp/TotpAuthenticator.java

@@ -4,13 +4,14 @@ import com.warrenstrange.googleauth.GoogleAuthenticator;
 import com.warrenstrange.googleauth.GoogleAuthenticatorKey;
 import com.warrenstrange.googleauth.GoogleAuthenticatorQRGenerator;
 import com.warrenstrange.googleauth.IGoogleAuthenticator;
+import fr.xephi.authme.data.auth.PlayerAuth;
 import fr.xephi.authme.service.BukkitService;
 import org.bukkit.entity.Player;
 
 import javax.inject.Inject;
 
 /**
- * Provides rudimentary TOTP functions (wraps third-party TOTP implementation).
+ * Provides TOTP functions (wrapping a third-party TOTP implementation).
  */
 public class TotpAuthenticator {
 
@@ -30,6 +31,10 @@ public class TotpAuthenticator {
         return new GoogleAuthenticator();
     }
 
+    public boolean checkCode(PlayerAuth auth, String totpCode) {
+        return checkCode(auth.getTotpKey(), totpCode);
+    }
+
     /**
      * Returns whether the given input code matches for the provided TOTP key.
      *
@@ -58,7 +63,7 @@ public class TotpAuthenticator {
         private final String totpKey;
         private final String authenticatorQrCodeUrl;
 
-        TotpGenerationResult(String totpKey, String authenticatorQrCodeUrl) {
+        public TotpGenerationResult(String totpKey, String authenticatorQrCodeUrl) {
             this.totpKey = totpKey;
             this.authenticatorQrCodeUrl = authenticatorQrCodeUrl;
         }

src/main/java/fr/xephi/authme/security/totp/TotpService.java

@@ -1,18 +0,0 @@
-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);
-    }
-}

src/test/java/fr/xephi/authme/command/executable/totp/AddTotpCommandTest.java

@@ -0,0 +1,93 @@
+package fr.xephi.authme.command.executable.totp;
+
+import fr.xephi.authme.data.auth.PlayerAuth;
+import fr.xephi.authme.datasource.DataSource;
+import fr.xephi.authme.message.MessageKey;
+import fr.xephi.authme.message.Messages;
+import fr.xephi.authme.security.totp.GenerateTotpService;
+import fr.xephi.authme.security.totp.TotpAuthenticator.TotpGenerationResult;
+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.Collections;
+
+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 AddTotpCommand}.
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class AddTotpCommandTest {
+
+    @InjectMocks
+    private AddTotpCommand addTotpCommand;
+
+    @Mock
+    private GenerateTotpService generateTotpService;
+    @Mock
+    private DataSource dataSource;
+    @Mock
+    private Messages messages;
+
+    @Test
+    public void shouldHandleNonExistentUser() {
+        // given
+        Player player = mockPlayerWithName("bob");
+        given(dataSource.getAuth("bob")).willReturn(null);
+
+        // when
+        addTotpCommand.runCommand(player, Collections.emptyList());
+
+        // then
+        verify(messages).send(player, MessageKey.REGISTER_MESSAGE);
+        verifyZeroInteractions(generateTotpService);
+    }
+
+    @Test
+    public void shouldNotAddCodeForAlreadyExistingTotp() {
+        // given
+        Player player = mockPlayerWithName("arend");
+        PlayerAuth auth = PlayerAuth.builder().name("arend")
+            .totpKey("TOTP2345").build();
+        given(dataSource.getAuth("arend")).willReturn(auth);
+
+        // when
+        addTotpCommand.runCommand(player, Collections.emptyList());
+
+        // then
+        verify(messages).send(player, MessageKey.TWO_FACTOR_ALREADY_ENABLED);
+        verifyZeroInteractions(generateTotpService);
+    }
+
+    @Test
+    public void shouldGenerateTotpCode() {
+        // given
+        Player player = mockPlayerWithName("charles");
+        PlayerAuth auth = PlayerAuth.builder().name("charles").build();
+        given(dataSource.getAuth("charles")).willReturn(auth);
+
+        TotpGenerationResult generationResult = new TotpGenerationResult(
+            "777Key214", "http://example.org/qr-code/link");
+        given(generateTotpService.generateTotpKey(player)).willReturn(generationResult);
+
+        // when
+        addTotpCommand.runCommand(player, Collections.emptyList());
+
+        // then
+        verify(messages).send(player, MessageKey.TWO_FACTOR_CREATE, generationResult.getTotpKey(), generationResult.getAuthenticatorQrCodeUrl());
+        verify(messages).send(player, MessageKey.TWO_FACTOR_CREATE_CONFIRMATION_REQUIRED);
+    }
+
+    private static Player mockPlayerWithName(String name) {
+        Player player = mock(Player.class);
+        given(player.getName()).willReturn(name);
+        return player;
+    }
+}

src/test/java/fr/xephi/authme/command/executable/totp/ConfirmTotpCommandTest.java

@@ -0,0 +1,139 @@
+package fr.xephi.authme.command.executable.totp;
+
+import fr.xephi.authme.data.auth.PlayerAuth;
+import fr.xephi.authme.datasource.DataSource;
+import fr.xephi.authme.message.MessageKey;
+import fr.xephi.authme.message.Messages;
+import fr.xephi.authme.security.totp.GenerateTotpService;
+import fr.xephi.authme.security.totp.TotpAuthenticator.TotpGenerationResult;
+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.Collections;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.only;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyZeroInteractions;
+
+/**
+ * Test for {@link ConfirmTotpCommand}.
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class ConfirmTotpCommandTest {
+
+    @InjectMocks
+    private ConfirmTotpCommand command;
+
+    @Mock
+    private GenerateTotpService generateTotpService;
+    @Mock
+    private DataSource dataSource;
+    @Mock
+    private Messages messages;
+
+    @Test
+    public void shouldAddTotpCodeToUserAfterSuccessfulConfirmation() {
+        // given
+        Player player = mock(Player.class);
+        String playerName = "George";
+        given(player.getName()).willReturn(playerName);
+        PlayerAuth auth = PlayerAuth.builder().name(playerName).build();
+        given(dataSource.getAuth(playerName)).willReturn(auth);
+        given(generateTotpService.getGeneratedTotpKey(player)).willReturn(new TotpGenerationResult("totp-key", "url-not-relevant"));
+        String totpCode = "954321";
+        given(generateTotpService.isTotpCodeCorrectForGeneratedTotpKey(player, totpCode)).willReturn(true);
+
+        // when
+        command.runCommand(player, Collections.singletonList(totpCode));
+
+        // then
+        verify(generateTotpService).isTotpCodeCorrectForGeneratedTotpKey(player, totpCode);
+        verify(generateTotpService).removeGenerateTotpKey(player);
+        verify(dataSource).setTotpKey(playerName, "totp-key");
+        verify(messages).send(player, MessageKey.TWO_FACTOR_ENABLE_SUCCESS);
+    }
+
+    @Test
+    public void shouldHandleWrongTotpCode() {
+        // given
+        Player player = mock(Player.class);
+        String playerName = "George";
+        given(player.getName()).willReturn(playerName);
+        PlayerAuth auth = PlayerAuth.builder().name(playerName).build();
+        given(dataSource.getAuth(playerName)).willReturn(auth);
+        given(generateTotpService.getGeneratedTotpKey(player)).willReturn(new TotpGenerationResult("totp-key", "url-not-relevant"));
+        String totpCode = "754321";
+        given(generateTotpService.isTotpCodeCorrectForGeneratedTotpKey(player, totpCode)).willReturn(false);
+
+        // when
+        command.runCommand(player, Collections.singletonList(totpCode));
+
+        // then
+        verify(generateTotpService).isTotpCodeCorrectForGeneratedTotpKey(player, totpCode);
+        verify(generateTotpService, never()).removeGenerateTotpKey(any(Player.class));
+        verify(dataSource, only()).getAuth(playerName);
+        verify(messages).send(player, MessageKey.TWO_FACTOR_ENABLE_ERROR_WRONG_CODE);
+    }
+
+    @Test
+    public void shouldHandleMissingTotpKey() {
+        // given
+        Player player = mock(Player.class);
+        String playerName = "George";
+        given(player.getName()).willReturn(playerName);
+        PlayerAuth auth = PlayerAuth.builder().name(playerName).build();
+        given(dataSource.getAuth(playerName)).willReturn(auth);
+        given(generateTotpService.getGeneratedTotpKey(player)).willReturn(null);
+
+        // when
+        command.runCommand(player, Collections.singletonList("871634"));
+
+        // then
+        verify(generateTotpService, only()).getGeneratedTotpKey(player);
+        verify(dataSource, only()).getAuth(playerName);
+        verify(messages).send(player, MessageKey.TWO_FACTOR_ENABLE_ERROR_NO_CODE);
+    }
+
+    @Test
+    public void shouldStopForAlreadyExistingTotpKeyOnAccount() {
+        // given
+        Player player = mock(Player.class);
+        String playerName = "George";
+        given(player.getName()).willReturn(playerName);
+        PlayerAuth auth = PlayerAuth.builder().name(playerName).totpKey("A987234").build();
+        given(dataSource.getAuth(playerName)).willReturn(auth);
+
+        // when
+        command.runCommand(player, Collections.singletonList("871634"));
+
+        // then
+        verify(dataSource, only()).getAuth(playerName);
+        verifyZeroInteractions(generateTotpService);
+        verify(messages).send(player, MessageKey.TWO_FACTOR_ALREADY_ENABLED);
+    }
+
+    @Test
+    public void shouldHandleMissingAuthAccount() {
+        // given
+        Player player = mock(Player.class);
+        String playerName = "George";
+        given(player.getName()).willReturn(playerName);
+        given(dataSource.getAuth(playerName)).willReturn(null);
+
+        // when
+        command.runCommand(player, Collections.singletonList("984685"));
+
+        // then
+        verify(dataSource, only()).getAuth(playerName);
+        verifyZeroInteractions(generateTotpService);
+        verify(messages).send(player, MessageKey.REGISTER_MESSAGE);
+    }
+}

src/test/java/fr/xephi/authme/command/executable/totp/TotpBaseCommandTest.java

@@ -0,0 +1,47 @@
+package fr.xephi.authme.command.executable.totp;
+
+import fr.xephi.authme.command.CommandMapper;
+import fr.xephi.authme.command.FoundCommandResult;
+import fr.xephi.authme.command.help.HelpProvider;
+import org.bukkit.command.CommandSender;
+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.Collections;
+
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+/**
+ * Test for {@link TotpBaseCommand}.
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class TotpBaseCommandTest {
+
+    @InjectMocks
+    private TotpBaseCommand command;
+
+    @Mock
+    private CommandMapper mapper;
+    @Mock
+    private HelpProvider helpProvider;
+
+    @Test
+    public void shouldOutputHelp() {
+        // given
+        CommandSender sender = mock(CommandSender.class);
+        FoundCommandResult mappingResult = mock(FoundCommandResult.class);
+        given(mapper.mapPartsToCommand(sender, Collections.singletonList("totp"))).willReturn(mappingResult);
+
+        // when
+        command.executeCommand(sender, Collections.emptyList());
+
+        // then
+        verify(mapper).mapPartsToCommand(sender, Collections.singletonList("totp"));
+        verify(helpProvider).outputHelp(sender, mappingResult, HelpProvider.SHOW_CHILDREN);
+    }
+}

src/test/java/fr/xephi/authme/message/updater/MessageUpdaterTest.java

@@ -100,6 +100,23 @@ public class MessageUpdaterTest {
             equalTo("seconds in plural"));
     }
 
+    @Test
+    public void shouldPerformNewerMigrations() throws IOException {
+        // given
+        File messagesFile = temporaryFolder.newFile();
+        Files.copy(TestHelper.getJarFile(TestHelper.PROJECT_ROOT + "message/messages_test2.yml"), messagesFile);
+
+        // when
+        boolean wasChanged = messageUpdater.migrateAndSave(messagesFile, "messages/messages_en.yml", "messages/messages_en.yml");
+
+        // then
+        assertThat(wasChanged, equalTo(true));
+        FileConfiguration configuration = YamlConfiguration.loadConfiguration(messagesFile);
+        assertThat(configuration.getString(MessageKey.TWO_FACTOR_CREATE.getKey()), equalTo("Old 2fa create text"));
+        assertThat(configuration.getString(MessageKey.WRONG_PASSWORD.getKey()), equalTo("test2 - wrong password")); // from pre-5.5 key
+        assertThat(configuration.getString(MessageKey.SECOND.getKey()), equalTo("second")); // from messages_en.yml
+    }
+
     @Test
     public void shouldHaveAllKeysInConfigurationData() {
         // given

src/test/java/fr/xephi/authme/security/totp/TotpAuthenticatorTest.java

@@ -1,6 +1,7 @@
 package fr.xephi.authme.security.totp;
 
 import com.warrenstrange.googleauth.IGoogleAuthenticator;
+import fr.xephi.authme.data.auth.PlayerAuth;
 import fr.xephi.authme.security.totp.TotpAuthenticator.TotpGenerationResult;
 import fr.xephi.authme.service.BukkitService;
 import org.bukkit.entity.Player;
@@ -86,6 +87,25 @@ public class TotpAuthenticatorTest {
         verifyZeroInteractions(googleAuthenticator);
     }
 
+    @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 = totpAuthenticator.checkCode(auth, inputCode);
+
+        // then
+        assertThat(result, equalTo(true));
+        verify(googleAuthenticator).authorize(totpKey, 408435);
+    }
+
     private final class TotpAuthenticatorTestImpl extends TotpAuthenticator {
 
         TotpAuthenticatorTestImpl(BukkitService bukkitService) {

src/test/java/fr/xephi/authme/security/totp/TotpServiceTest.java

@@ -1,46 +0,0 @@
-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);
-    }
-
-}

src/test/resources/fr/xephi/authme/message/messages_test2.yml

@@ -4,3 +4,5 @@ unknown_user: 'Message from test2'
 login: 'test2 - login'
 not_logged_in: 'test2 - not logged in'
 wrong_pwd: 'test2 - wrong password'
+misc:
+  two_factor_create: 'Old 2fa create text'