From ad0e76600b41c6f42e1650303f0ceaaacc843ae1 Mon Sep 17 00:00:00 2001 From: mrflick72 Date: Sat, 27 Jul 2024 16:56:53 +0200 Subject: [PATCH] mfa association bug: mfa channel was hardcoded as main email --- .../server/mfa/domain/MfaSenderAndVerifier.kt | 4 +- .../DynamoMfaAccountMethodsRepository.kt | 12 ++++- .../domain/AccountAwareOtpMfaVerifierTest.kt | 44 +++++++++---------- 3 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/main/kotlin/com/vauthenticator/server/mfa/domain/MfaSenderAndVerifier.kt b/src/main/kotlin/com/vauthenticator/server/mfa/domain/MfaSenderAndVerifier.kt index 48696978..683ea964 100644 --- a/src/main/kotlin/com/vauthenticator/server/mfa/domain/MfaSenderAndVerifier.kt +++ b/src/main/kotlin/com/vauthenticator/server/mfa/domain/MfaSenderAndVerifier.kt @@ -51,7 +51,7 @@ class AccountAwareOtpMfaVerifier( mfaChannel: String, challenge: MfaChallenge ) { - mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) + mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, mfaChannel) .map { val account = accountRepository.accountFor(userName).get() if (!it.associated) { @@ -68,7 +68,7 @@ class AccountAwareOtpMfaVerifier( mfaChannel: String, challenge: MfaChallenge ) { - mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) + mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, mfaChannel) .map { val account = accountRepository.accountFor(userName).get() if (it.associated) { diff --git a/src/main/kotlin/com/vauthenticator/server/mfa/repository/DynamoMfaAccountMethodsRepository.kt b/src/main/kotlin/com/vauthenticator/server/mfa/repository/DynamoMfaAccountMethodsRepository.kt index 301641e5..9bee03e1 100644 --- a/src/main/kotlin/com/vauthenticator/server/mfa/repository/DynamoMfaAccountMethodsRepository.kt +++ b/src/main/kotlin/com/vauthenticator/server/mfa/repository/DynamoMfaAccountMethodsRepository.kt @@ -24,7 +24,7 @@ class DynamoMfaAccountMethodsRepository( mfaMfaMethod: MfaMethod, mfaChannel: String ): Optional = - Optional.ofNullable(findAll(userName).find { it.method == mfaMfaMethod }) + Optional.ofNullable(findAll(userName).find { it.method == mfaMfaMethod && it.mfaChannel == mfaChannel}) override fun findAll(userName: String): List = @@ -42,6 +42,16 @@ class DynamoMfaAccountMethodsRepository( QueryRequest.builder().tableName(tableName).keyConditionExpression("user_name=:email") .expressionAttributeValues(mapOf(":email" to email.asDynamoAttribute())).build() ).items() + private fun getFromDynamo(email: String, mfaChannel : String) = dynamoDbClient.query( + QueryRequest.builder().tableName(tableName).keyConditionExpression("user_name=:email AND mfa_channel=:mfaChannel") + .expressionAttributeValues( + mapOf( + ":email" to email.asDynamoAttribute(), + ":mfaChannel" to mfaChannel.asDynamoAttribute(), + ) + ) + .build() + ).items() override fun save( userName: String, diff --git a/src/test/kotlin/com/vauthenticator/server/mfa/domain/AccountAwareOtpMfaVerifierTest.kt b/src/test/kotlin/com/vauthenticator/server/mfa/domain/AccountAwareOtpMfaVerifierTest.kt index 6bdf7e18..8fe50550 100644 --- a/src/test/kotlin/com/vauthenticator/server/mfa/domain/AccountAwareOtpMfaVerifierTest.kt +++ b/src/test/kotlin/com/vauthenticator/server/mfa/domain/AccountAwareOtpMfaVerifierTest.kt @@ -21,7 +21,7 @@ internal class AccountAwareOtpMfaVerifierTest { private val account = anAccount() private val userName = account.email - private val email = userName + private val email = "a_new_email@email.com" private val challenge = MfaChallenge("AN_MFA_CHALLENGE") @@ -45,7 +45,7 @@ internal class AccountAwareOtpMfaVerifierTest { internal fun `when associated mfa challenge is successfully verified`() { val challenge = MfaChallenge("AN_MFA_CHALLENGE") - every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } returns + every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } returns Optional.of( MfaAccountMethod( userName, @@ -56,16 +56,16 @@ internal class AccountAwareOtpMfaVerifierTest { ) ) every { accountRepository.accountFor(userName) } returns Optional.of(account) - every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, userName, challenge) } just runs + every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, email, challenge) } just runs - underTest.verifyAssociatedMfaChallengeFor(userName, MfaMethod.EMAIL_MFA_METHOD, userName, challenge) + underTest.verifyAssociatedMfaChallengeFor(userName, MfaMethod.EMAIL_MFA_METHOD, email, challenge) - verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } + verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } } @Test internal fun `when not associated mfa challenge fails on verification`() { - every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } returns + every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } returns Optional.of( MfaAccountMethod( userName, @@ -76,25 +76,25 @@ internal class AccountAwareOtpMfaVerifierTest { ) ) every { accountRepository.accountFor(userName) } returns Optional.of(account) - every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, userName, challenge) } just runs + every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, email, challenge) } just runs assertThrows(UnAssociatedMfaVerificationException::class.java) { underTest.verifyAssociatedMfaChallengeFor( userName, MfaMethod.EMAIL_MFA_METHOD, - userName, + email, challenge ) } - verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } + verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } } @Test internal fun `when associated mfa challenge fails on verification`() { every { accountRepository.accountFor(account.email) } returns Optional.of(account) - every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, account.email, challenge) } throws MfaException("") - every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } returns + every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, email, challenge) } throws MfaException("") + every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } returns Optional.of( MfaAccountMethod( userName, @@ -106,14 +106,14 @@ internal class AccountAwareOtpMfaVerifierTest { ) assertThrows(MfaException::class.java) { underTest.verifyAssociatedMfaChallengeFor( - account.email, + userName, MfaMethod.EMAIL_MFA_METHOD, - account.email, + email, challenge ) } - verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } + verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } } @@ -121,8 +121,8 @@ internal class AccountAwareOtpMfaVerifierTest { @Test internal fun `when not associated mfa verification succeed on association verification`() { every { accountRepository.accountFor(account.email) } returns Optional.of(account) - every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, account.email, challenge) } throws MfaException("") - every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } returns + every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, email, challenge) } throws MfaException("") + every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } returns Optional.of( MfaAccountMethod( userName, @@ -136,19 +136,19 @@ internal class AccountAwareOtpMfaVerifierTest { underTest.verifyMfaChallengeToBeAssociatedFor( account.email, MfaMethod.EMAIL_MFA_METHOD, - account.email, + email, challenge ) } - verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } + verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } } @Test internal fun `when associated mfa verification succeed on association verification`() { every { accountRepository.accountFor(account.email) } returns Optional.of(account) - every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, account.email, challenge) } throws MfaException("") - every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } returns + every { otpMfa.verify(account, MfaMethod.EMAIL_MFA_METHOD, email, challenge) } throws MfaException("") + every { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } returns Optional.of( MfaAccountMethod( userName, @@ -162,11 +162,11 @@ internal class AccountAwareOtpMfaVerifierTest { underTest.verifyMfaChallengeToBeAssociatedFor( account.email, MfaMethod.EMAIL_MFA_METHOD, - account.email, + email, challenge ) } - verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, userName) } + verify { mfaAccountMethodsRepository.findOne(userName, MfaMethod.EMAIL_MFA_METHOD, email) } } } \ No newline at end of file