Improve code coverage for totp mfa util change

indigo-iam · Dec 6, 2023 · 550a8da · 550a8da
1 parent 8dfb7c4
commit 550a8da
Show file tree

Hide file tree

Showing 14 changed files with 389 additions and 181 deletions.
diff --git a/...java/it/infn/mw/iam/api/account/multi_factor_authentication/DefaultIamTotpMfaService.java b/...java/it/infn/mw/iam/api/account/multi_factor_authentication/DefaultIamTotpMfaService.java
@@ -19,8 +19,6 @@
 import java.util.Optional;
 import java.util.Set;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.context.ApplicationEventPublisherAware;
@@ -43,12 +41,11 @@
 import it.infn.mw.iam.persistence.model.IamTotpRecoveryCode;
 import it.infn.mw.iam.persistence.repository.IamTotpMfaRepository;
 import it.infn.mw.iam.util.mfa.IamTotpMfaEncryptionAndDecryptionUtil;
+import it.infn.mw.iam.util.mfa.IamTotpMfaInvalidArgumentError;
 
 @Service
 public class DefaultIamTotpMfaService implements IamTotpMfaService, ApplicationEventPublisherAware {
 
-  private static final Logger LOG = LoggerFactory.getLogger(DefaultIamTotpMfaService.class);
-
   public static final int RECOVERY_CODE_QUANTITY = 6;
 
   private final IamAccountService iamAccountService;
@@ -103,7 +100,7 @@ public void setApplicationEventPublisher(ApplicationEventPublisher applicationEv
    * @return the new TOTP secret
    */
   @Override
-  public IamTotpMfa addTotpMfaSecret(IamAccount account) {
+  public IamTotpMfa addTotpMfaSecret(IamAccount account) throws IamTotpMfaInvalidArgumentError {
     Optional<IamTotpMfa> totpMfaOptional = totpMfaRepository.findByAccount(account);
     if (totpMfaOptional.isPresent()) {
       if (totpMfaOptional.get().isActive()) {
@@ -116,24 +113,16 @@ public IamTotpMfa addTotpMfaSecret(IamAccount account) {
 
     // Generate secret
     IamTotpMfa totpMfa = new IamTotpMfa(account);
-    String sharedSecretToken = secretGenerator.generate();
-
-    try {
-      String cipherText = IamTotpMfaEncryptionAndDecryptionUtil.encryptSecretOrRecoveryCode(
-          sharedSecretToken, iamTotpMfaProperties.getPasswordToEncryptOrDecrypt());
 
-      totpMfa.setSecret(cipherText);
-      totpMfa.setAccount(account);
+    totpMfa.setSecret(IamTotpMfaEncryptionAndDecryptionUtil.encryptSecretOrRecoveryCode(
+        secretGenerator.generate(), iamTotpMfaProperties.getPasswordToEncryptOrDecrypt()));
+    totpMfa.setAccount(account);
 
-      Set<IamTotpRecoveryCode> recoveryCodes = generateRecoveryCodes(totpMfa);
-      totpMfa.setRecoveryCodes(recoveryCodes);
-      totpMfaRepository.save(totpMfa);
+    Set<IamTotpRecoveryCode> recoveryCodes = generateRecoveryCodes(totpMfa);
+    totpMfa.setRecoveryCodes(recoveryCodes);
+    totpMfaRepository.save(totpMfa);
 
-      return totpMfa;
-    } catch (Exception iamTotpMfaInvalidArgumentErrorMsg) {
-      LOG.error(iamTotpMfaInvalidArgumentErrorMsg.getMessage());
-      throw iamTotpMfaInvalidArgumentErrorMsg;
-    }
+    return totpMfa;
   }
 
   /**
@@ -213,30 +202,24 @@ public IamTotpMfa disableTotpMfa(IamAccount account) {
    * Verifies a provided TOTP against an account multi-factor secret
    * 
    * @param account the account whose secret we will check against
-   * @param totp the TOTP to validate
+   * @param totp    the TOTP to validate
    * @return true if valid, false otherwise
    */
   @Override
-  public boolean verifyTotp(IamAccount account, String totp) {
+  public boolean verifyTotp(IamAccount account, String totp) throws IamTotpMfaInvalidArgumentError {
     Optional<IamTotpMfa> totpMfaOptional = totpMfaRepository.findByAccount(account);
     if (!totpMfaOptional.isPresent()) {
       throw new MfaSecretNotFoundException("No multi-factor secret is attached to this account");
     }
 
     IamTotpMfa totpMfa = totpMfaOptional.get();
+    String mfaSecret = IamTotpMfaEncryptionAndDecryptionUtil.decryptSecretOrRecoveryCode(
+        totpMfa.getSecret(), iamTotpMfaProperties.getPasswordToEncryptOrDecrypt());
 
-    try {
-      String mfaSecret = IamTotpMfaEncryptionAndDecryptionUtil.decryptSecretOrRecoveryCode(
-          totpMfa.getSecret(), iamTotpMfaProperties.getPasswordToEncryptOrDecrypt());
-
-      // Verify provided TOTP
-      if (codeVerifier.isValidCode(mfaSecret, totp)) {
-        totpVerifiedEvent(account, totpMfa);
-        return true;
-      }
-    } catch (Exception iamTotpMfaInvalidArgumentErrorMsg) {
-      LOG.error(iamTotpMfaInvalidArgumentErrorMsg.getMessage());
-      throw iamTotpMfaInvalidArgumentErrorMsg;
+    // Verify provided TOTP
+    if (codeVerifier.isValidCode(mfaSecret, totp)) {
+      totpVerifiedEvent(account, totpMfa);
+      return true;
     }
 
     return false;
@@ -264,40 +247,31 @@ public boolean verifyRecoveryCode(IamAccount account, String recoveryCode) {
     // Check for a matching recovery code
     Set<IamTotpRecoveryCode> accountRecoveryCodes = totpMfa.getRecoveryCodes();
 
-    try {
-      for (IamTotpRecoveryCode recoveryCodeObject : accountRecoveryCodes) {
-        String recoveryCodeEncrypted = recoveryCodeObject.getCode();
-        String recoveryCodeString = IamTotpMfaEncryptionAndDecryptionUtil.decryptSecretOrRecoveryCode(
-            recoveryCodeEncrypted, iamTotpMfaProperties.getPasswordToEncryptOrDecrypt());
+    for (IamTotpRecoveryCode recoveryCodeObject : accountRecoveryCodes) {
+      String recoveryCodeEncrypted = recoveryCodeObject.getCode();
+      String recoveryCodeString = IamTotpMfaEncryptionAndDecryptionUtil.decryptSecretOrRecoveryCode(
+          recoveryCodeEncrypted, iamTotpMfaProperties.getPasswordToEncryptOrDecrypt());
 
-        if (recoveryCode.equals(recoveryCodeString)) {
-          recoveryCodeVerifiedEvent(account, totpMfa);
-          return true;
-        }
+      if (recoveryCode.equals(recoveryCodeString)) {
+        recoveryCodeVerifiedEvent(account, totpMfa);
+        return true;
       }
-    } catch (Exception iamTotpMfaInvalidArgumentErrorMsg) {
-      throw iamTotpMfaInvalidArgumentErrorMsg;
     }
 
     return false;
   }
 
 
-  private Set<IamTotpRecoveryCode> generateRecoveryCodes(IamTotpMfa totpMfa) {
+  private Set<IamTotpRecoveryCode> generateRecoveryCodes(IamTotpMfa totpMfa) throws IamTotpMfaInvalidArgumentError {
     String[] recoveryCodeStrings = recoveryCodeGenerator.generateCodes(RECOVERY_CODE_QUANTITY);
     Set<IamTotpRecoveryCode> recoveryCodes = new HashSet<>();
 
-    try {
-      for (String code : recoveryCodeStrings) {
-        IamTotpRecoveryCode recoveryCode = new IamTotpRecoveryCode(totpMfa);
+    for (String code : recoveryCodeStrings) {
+      IamTotpRecoveryCode recoveryCode = new IamTotpRecoveryCode(totpMfa);
 
-        recoveryCode.setCode(IamTotpMfaEncryptionAndDecryptionUtil.encryptSecretOrRecoveryCode(
-            code, iamTotpMfaProperties.getPasswordToEncryptOrDecrypt()));
-        recoveryCodes.add(recoveryCode);
-      }
-    } catch (Exception iamTotpMfaInvalidArgumentErrorMsg) {
-      LOG.error(iamTotpMfaInvalidArgumentErrorMsg.getMessage());
-      throw iamTotpMfaInvalidArgumentErrorMsg;
+      recoveryCode.setCode(IamTotpMfaEncryptionAndDecryptionUtil.encryptSecretOrRecoveryCode(
+          code, iamTotpMfaProperties.getPasswordToEncryptOrDecrypt()));
+      recoveryCodes.add(recoveryCode);
     }
 
     return recoveryCodes;

diff --git a/...w/iam/api/account/multi_factor_authentication/DefaultIamTotpRecoveryCodeResetService.java b/...w/iam/api/account/multi_factor_authentication/DefaultIamTotpRecoveryCodeResetService.java
@@ -36,6 +36,7 @@
 import it.infn.mw.iam.persistence.repository.IamAccountRepository;
 import it.infn.mw.iam.persistence.repository.IamTotpMfaRepository;
 import it.infn.mw.iam.util.mfa.IamTotpMfaEncryptionAndDecryptionUtil;
+import it.infn.mw.iam.util.mfa.IamTotpMfaInvalidArgumentError;
 
 @Service
 public class DefaultIamTotpRecoveryCodeResetService
@@ -72,7 +73,7 @@ public void setApplicationEventPublisher(ApplicationEventPublisher applicationEv
    * @param account - the account to regenerate codes on
    */
   @Override
-  public IamAccount resetRecoveryCodes(IamAccount account) {
+  public IamAccount resetRecoveryCodes(IamAccount account) throws IamTotpMfaInvalidArgumentError {
     Optional<IamTotpMfa> totpMfaOptional = totpMfaRepository.findByAccount(account);
     if (!totpMfaOptional.isPresent()) {
       throw new MfaSecretNotFoundException("No multi-factor secret is attached to this account");
@@ -82,26 +83,22 @@ public IamAccount resetRecoveryCodes(IamAccount account) {
     String[] recoveryCodeStrings = recoveryCodeGenerator.generateCodes(RECOVERY_CODE_QUANTITY);
     Set<IamTotpRecoveryCode> recoveryCodes = new HashSet<>();
 
-    try {
-      for (String code : recoveryCodeStrings) {
-        IamTotpRecoveryCode recoveryCode = new IamTotpRecoveryCode(totpMfa);
+    for (String code : recoveryCodeStrings) {
+      IamTotpRecoveryCode recoveryCode = new IamTotpRecoveryCode(totpMfa);
 
-        recoveryCode.setCode(IamTotpMfaEncryptionAndDecryptionUtil.encryptSecretOrRecoveryCode(
-            code, iamTotpMfaProperties.getPasswordToEncryptOrDecrypt()));
-        recoveryCodes.add(recoveryCode);
-      }
+      recoveryCode.setCode(IamTotpMfaEncryptionAndDecryptionUtil.encryptSecretOrRecoveryCode(
+          code, iamTotpMfaProperties.getPasswordToEncryptOrDecrypt()));
+      recoveryCodes.add(recoveryCode);
+    }
 
-      // Attach to account
-      totpMfa.setRecoveryCodes(recoveryCodes);
-      totpMfa.touch();
-      account.touch();
-      accountRepository.save(account);
-      totpMfaRepository.save(totpMfa);
-      recoveryCodesResetEvent(account, totpMfa);
+    // Attach to account
+    totpMfa.setRecoveryCodes(recoveryCodes);
+    totpMfa.touch();
+    account.touch();
+    accountRepository.save(account);
+    totpMfaRepository.save(totpMfa);
+    recoveryCodesResetEvent(account, totpMfa);
 
-      return account;
-    } catch (Exception iamTotpMfaInvalidArgumentErrorMsg) {
-      throw iamTotpMfaInvalidArgumentErrorMsg;
-    }
+    return account;
   }
 }
diff --git a/...unt/multi_factor_authentication/authenticator_app/AuthenticatorAppSettingsController.java b/...unt/multi_factor_authentication/authenticator_app/AuthenticatorAppSettingsController.java
@@ -51,6 +51,7 @@
 import it.infn.mw.iam.persistence.model.IamTotpMfa;
 import it.infn.mw.iam.persistence.repository.IamAccountRepository;
 import it.infn.mw.iam.util.mfa.IamTotpMfaEncryptionAndDecryptionUtil;
+import it.infn.mw.iam.util.mfa.IamTotpMfaInvalidArgumentError;
 
 /**
  * Controller for customising user's authenticator app MFA settings Can enable or disable the
@@ -90,20 +91,14 @@ public AuthenticatorAppSettingsController(IamTotpMfaService service,
   @RequestMapping(value = ADD_SECRET_URL, method = RequestMethod.PUT,
       produces = MediaType.APPLICATION_JSON_VALUE)
   @ResponseBody
-  public SecretAndDataUriDTO addSecret() {
+  public SecretAndDataUriDTO addSecret() throws IamTotpMfaInvalidArgumentError {
     final String username = getUsernameFromSecurityContext();
     IamAccount account = accountRepository.findByUsername(username)
       .orElseThrow(() -> NoSuchAccountError.forUsername(username));
 
     IamTotpMfa totpMfa = service.addTotpMfaSecret(account);
-    String mfaSecret = "";
-
-    try {
-      mfaSecret = IamTotpMfaEncryptionAndDecryptionUtil.decryptSecretOrRecoveryCode(
-          totpMfa.getSecret(), iamTotpMfaProperties.getPasswordToEncryptOrDecrypt());
-    } catch (Exception iamTotpMfaInvalidArgumentErrorMsg) {
-      throw iamTotpMfaInvalidArgumentErrorMsg;
-    }
+    String mfaSecret = IamTotpMfaEncryptionAndDecryptionUtil.decryptSecretOrRecoveryCode(
+        totpMfa.getSecret(), iamTotpMfaProperties.getPasswordToEncryptOrDecrypt());
 
     try {
       SecretAndDataUriDTO dto = new SecretAndDataUriDTO(mfaSecret);

diff --git a/...authn/multi_factor_authentication/authenticator_app/RecoveryCodeManagementController.java b/...authn/multi_factor_authentication/authenticator_app/RecoveryCodeManagementController.java
@@ -43,6 +43,7 @@
 import it.infn.mw.iam.persistence.model.IamTotpRecoveryCode;
 import it.infn.mw.iam.persistence.repository.IamTotpMfaRepository;
 import it.infn.mw.iam.util.mfa.IamTotpMfaEncryptionAndDecryptionUtil;
+import it.infn.mw.iam.util.mfa.IamTotpMfaInvalidArgumentError;
 
 /**
  * Provides webpages related to recovery codes. Most of this appears if the user chooses to use a
@@ -61,7 +62,6 @@ public class RecoveryCodeManagementController {
   private final IamTotpRecoveryCodeResetService recoveryCodeResetService;
   private final IamTotpMfaProperties iamTotpMfaProperties;
 
-
   @Autowired
   public RecoveryCodeManagementController(AccountUtils accountUtils,
       IamTotpMfaRepository totpMfaRepository,
@@ -105,7 +105,7 @@ public String viewRecoveryCodes(ModelMap model) {
    */
   @PreAuthorize("hasRole('USER')")
   @RequestMapping(method = RequestMethod.GET, path = RECOVERY_CODE_GET_URL)
-  public @ResponseBody String[] getRecoveryCodes() {
+  public @ResponseBody String[] getRecoveryCodes() throws IamTotpMfaInvalidArgumentError {
     IamAccount account = accountUtils.getAuthenticatedUserAccount()
       .orElseThrow(() -> new MultiFactorAuthenticationError("Account not found"));
 
@@ -122,16 +122,12 @@ public String viewRecoveryCodes(ModelMap model) {
     List<IamTotpRecoveryCode> recs = new ArrayList<>(totpMfa.getRecoveryCodes());
     String[] codes = new String[recs.size()];
 
-    try {
-      for (int i = 0; i < recs.size(); i++) {
-        codes[i] = IamTotpMfaEncryptionAndDecryptionUtil.decryptSecretOrRecoveryCode(
-            recs.get(i).getCode(), iamTotpMfaProperties.getPasswordToEncryptOrDecrypt());
-      }
-
-      return codes;
-    } catch (Exception iamTotpMfaInvalidArgumentErrorMsg) {
-      throw iamTotpMfaInvalidArgumentErrorMsg;
+    for (int i = 0; i < recs.size(); i++) {
+      codes[i] = IamTotpMfaEncryptionAndDecryptionUtil.decryptSecretOrRecoveryCode(
+          recs.get(i).getCode(), iamTotpMfaProperties.getPasswordToEncryptOrDecrypt());
     }
+
+    return codes;
   }
 
   /**

diff --git a/...ervice/src/main/java/it/infn/mw/iam/util/mfa/IamTotpMfaEncryptionAndDecryptionHelper.java b/...ervice/src/main/java/it/infn/mw/iam/util/mfa/IamTotpMfaEncryptionAndDecryptionHelper.java
@@ -19,9 +19,16 @@ public class IamTotpMfaEncryptionAndDecryptionHelper {
 
   private String encryptionAlgorithm = "AES";
   private String modeOfOperation = "AES/CBC/PKCS5Padding";
-  private int keySize = 256;
+
+  // AES `keySize` has 3 options: 128, 192, or 256 bits.
+  private int keySize = 128;
+
   private int ivSize = 16;
+
+  // Multiples of 8
   private int saltSize = 16;
+
+  // The higher value the better
   private int iterations = 65536;
 
   private static IamTotpMfaEncryptionAndDecryptionHelper instance;
@@ -42,10 +49,6 @@ public int getIvSize() {
     return ivSize;
   }
 
-  public void setIvSize(int ivSize) {
-    this.ivSize = ivSize;
-  }
-
   public int getSaltSize() {
     return saltSize;
   }
@@ -66,18 +69,10 @@ public String getEncryptionAlgorithm() {
     return encryptionAlgorithm;
   }
 
-  public void setEncryptionAlgorithm(String encryptionAlgorithm) {
-    this.encryptionAlgorithm = encryptionAlgorithm;
-  }
-
   public String getModeOfOperation() {
     return modeOfOperation;
   }
 
-  public void setModeOfOperation(String modeOfOperation) {
-    this.modeOfOperation = modeOfOperation;
-  }
-
   /**
    * Helper to get the instance instead of creating new objects,
    * acts like a singleton pattern.

diff --git a/...-service/src/main/java/it/infn/mw/iam/util/mfa/IamTotpMfaEncryptionAndDecryptionUtil.java b/...-service/src/main/java/it/infn/mw/iam/util/mfa/IamTotpMfaEncryptionAndDecryptionUtil.java
@@ -40,8 +40,8 @@ private IamTotpMfaEncryptionAndDecryptionUtil() {
   }
 
   /**
-   * This process requires a password for encrypting the plaintext. Ensure to use
-   * the same password for decryption as well.
+   * This helper method requires a password for encrypting the plaintext.
+   * Ensure to use the same password for decryption as well.
    *
    * @param plaintext plaintext to encrypt.
    * @param password  Provided by the admin through the environment
@@ -129,7 +129,7 @@ public static String decryptSecretOrRecoveryCode(String cipherText, String passw
       return new String(decryptedTextBytes);
     } catch (Exception exp) {
       throw new IamTotpMfaInvalidArgumentError(
-          "Please use the same password and mode of operation which you used for encryption");
+          "Please use the same password and mode of operation which you used for encryption", exp);
     }
   }
 
@@ -171,10 +171,9 @@ private static SecretKey getKeyFromPassword(String password, byte[] salt, String
     SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
     KeySpec spec = new PBEKeySpec(password.toCharArray(), salt, defaultModel.getIterations(),
         defaultModel.getKeySize());
-    SecretKey secretKey = new SecretKeySpec(factory.generateSecret(spec)
-        .getEncoded(), algorithm);
 
-    return secretKey;
+    return new SecretKeySpec(factory.generateSecret(spec)
+        .getEncoded(), algorithm);
   }
 
   /**