From 81b43d13b0c86d469f91b4fd7aa6b44d64108315 Mon Sep 17 00:00:00 2001 From: David Perez Date: Mon, 31 Mar 2025 14:36:01 -0500 Subject: [PATCH] PM-19404: Improve email validation and validation error parsing (#4945) --- .../SendVerificationEmailResponseJson.kt | 16 ++++++--- .../ui/platform/base/util/StringExtensions.kt | 11 ++++-- .../auth/repository/AuthRepositoryTest.kt | 3 +- .../base/util/StringExtensionsTest.kt | 34 +++++++++---------- 4 files changed, 38 insertions(+), 26 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/network/model/SendVerificationEmailResponseJson.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/network/model/SendVerificationEmailResponseJson.kt index 5d5650993f..ddf460ddc6 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/network/model/SendVerificationEmailResponseJson.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/network/model/SendVerificationEmailResponseJson.kt @@ -1,7 +1,9 @@ package com.x8bit.bitwarden.data.auth.datasource.network.model +import kotlinx.serialization.ExperimentalSerializationApi import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable +import kotlinx.serialization.json.JsonNames /** * The response body for sending a verification email. @@ -26,20 +28,24 @@ sealed class SendVerificationEmailResponseJson { * The values in the array should be used for display to the user, since the keys tend to come * back as nonsense. (eg: empty string key) */ + @OptIn(ExperimentalSerializationApi::class) @Serializable data class Invalid( - @SerialName("message") - private val invalidMessage: String? = null, - + @JsonNames("message") @SerialName("Message") private val errorMessage: String? = null, @SerialName("validationErrors") - val validationErrors: Map>?, + private val validationErrors: Map>?, ) : SendVerificationEmailResponseJson() { /** * A generic error message. */ - val message: String? get() = invalidMessage ?: errorMessage + val message: String? + get() = validationErrors + ?.values + ?.firstOrNull() + ?.firstOrNull() + ?: errorMessage } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/base/util/StringExtensions.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/base/util/StringExtensions.kt index 968c8e6691..45df2e74f7 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/base/util/StringExtensions.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/base/util/StringExtensions.kt @@ -43,9 +43,16 @@ fun String?.orZeroWidthSpace(): String = this.orNullIfBlank() ?: ZERO_WIDTH_CHAR /** * Whether or not string is a valid email address. * - * This just checks if the string contains the "@" symbol. + * This validates that the email is valid by asserting that: + * * The string starts with a string of characters including periods, underscores, percent symbols, + * plus's, minus's, and alphanumeric characters. + * * Followed by an '@' symbol. + * * Followed by a string of characters including periods, minus's, and alphanumeric characters. + * * Followed by a period. + * * Followed by at least 2 more alphanumeric characters. */ -fun String.isValidEmail(): Boolean = contains("@") +fun String.isValidEmail(): Boolean = + this.matches(regex = "^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\\.[A-Za-z]{2,}$".toRegex()) /** * Returns `true` if the given [String] is a non-blank, valid URI and `false` otherwise. diff --git a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt index c0ca0e1e0c..7ab7557073 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt @@ -5474,7 +5474,6 @@ class AuthRepositoryTest { assertEquals(PrevalidateSsoResult.Success(token = "token"), result) } - @Suppress("MaxLineLength") @Test fun `logout for an inactive account should call logout on the UserLogoutManager`() { val userId = USER_ID_2 @@ -6229,7 +6228,7 @@ class AuthRepositoryTest { ), ) } returns SendVerificationEmailResponseJson - .Invalid(invalidMessage = errorMessage, validationErrors = null) + .Invalid(errorMessage = errorMessage, validationErrors = null) .asSuccess() val result = repository.sendVerificationEmail( diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/base/util/StringExtensionsTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/base/util/StringExtensionsTest.kt index 10a56c373d..000fc0aaa3 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/platform/base/util/StringExtensionsTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/base/util/StringExtensionsTest.kt @@ -9,26 +9,26 @@ import org.junit.jupiter.api.Test class StringExtensionsTest { @Test - fun `emails without an @ character should be invalid`() { + fun `isValidEmail should return appropriate value for possible email addresses`() { val invalidEmails = listOf( - "", - " ", - "test.com", + "" to false, + " " to false, + "test.com" to false, + "@" to false, + "@." to false, + "@.aa" to false, + "a@.aa" to false, + "test@test.com" to true, + " test@test " to false, + "test@test.c" to false, + "a@a.aa" to true, + "test@test.com" to true, + "test@test.test.com" to true, + "test.test@test.com" to true, + "test.test@test.test.com" to true, ) invalidEmails.forEach { - assertFalse(it.isValidEmail()) - } - } - - @Test - fun `emails with an @ character should be valid`() { - val validEmails = listOf( - "@", - "test@test.com", - " test@test ", - ) - validEmails.forEach { - assertTrue(it.isValidEmail()) + assertEquals(it.first.isValidEmail(), it.second) } }