Skip to content

Commit 74832ad

Browse files
Ivan-ShamlIvan-Shaml
authored
feat(registration): do not expose email is taken (#376)
1 parent 37399a6 commit 74832ad

File tree

14 files changed

+164
-27
lines changed

14 files changed

+164
-27
lines changed

‎src/main/bundles/dev.bundle

2.06 MB
Binary file not shown.

‎src/main/kotlin/com/faforever/userservice/backend/account/RegistrationService.kt

+28-19
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,10 @@ enum class UsernameStatus {
2222
USERNAME_TAKEN, USERNAME_RESERVED, USERNAME_AVAILABLE,
2323
}
2424

25-
enum class EmailStatus {
26-
EMAIL_TAKEN, EMAIL_BLACKLISTED, EMAIL_AVAILABLE,
25+
sealed interface EmailStatusResponse {
26+
data object EmailBlackListed : EmailStatusResponse
27+
data object EmailAvailable : EmailStatusResponse
28+
data class EmailTaken(val existingUsername: String) : EmailStatusResponse
2729
}
2830

2931
data class RegisteredUser(
@@ -49,10 +51,22 @@ class RegistrationService(
4951
}
5052

5153
fun register(username: String, email: String) {
52-
checkUsernameAndEmail(username, email)
54+
checkUsername(username)
55+
56+
when (val emailStatus = emailAvailable(email)) {
57+
is EmailStatusResponse.EmailBlackListed -> throw IllegalStateException("Email provider is blacklisted")
58+
is EmailStatusResponse.EmailTaken -> onEmailTaken(username, emailStatus.existingUsername, email)
59+
is EmailStatusResponse.EmailAvailable -> {
60+
sendActivationEmail(username, email)
61+
metricHelper.incrementUserRegistrationCounter()
62+
}
63+
}
64+
}
5365

54-
sendActivationEmail(username, email)
55-
metricHelper.incrementUserRegistrationCounter()
66+
private fun onEmailTaken(desiredUsername: String, existingUsername: String, email: String) {
67+
val passwordResetUrl =
68+
fafProperties.account().passwordReset().passwordResetInitiateEmailUrlFormat().format(email)
69+
emailService.sendEmailAlreadyTakenMail(desiredUsername, existingUsername, email, passwordResetUrl)
5670
}
5771

5872
private fun sendActivationEmail(username: String, email: String) {
@@ -84,14 +98,14 @@ class RegistrationService(
8498
}
8599

86100
@Transactional
87-
fun emailAvailable(email: String): EmailStatus {
101+
fun emailAvailable(email: String): EmailStatusResponse {
88102
val onBlacklist = domainBlacklistRepository.existsByDomain(email.substring(email.lastIndexOf('@') + 1))
89103
if (onBlacklist) {
90-
return EmailStatus.EMAIL_BLACKLISTED
104+
return EmailStatusResponse.EmailBlackListed
91105
}
92106

93-
val exists = userRepository.existsByEmail(email)
94-
return if (exists) EmailStatus.EMAIL_TAKEN else EmailStatus.EMAIL_AVAILABLE
107+
val user = userRepository.findByEmail(email)
108+
return if (user != null) EmailStatusResponse.EmailTaken(user.username) else EmailStatusResponse.EmailAvailable
95109
}
96110

97111
fun validateRegistrationToken(registrationToken: String): RegisteredUser {
@@ -115,7 +129,9 @@ class RegistrationService(
115129
val email = registeredUser.email
116130
val encodedPassword = passwordEncoder.encode(password)
117131

118-
checkUsernameAndEmail(username, email)
132+
checkUsername(username)
133+
val emailStatus = emailAvailable(email)
134+
require(emailStatus is EmailStatusResponse.EmailAvailable) { "Email unavailable" }
119135

120136
val user = User(
121137
username = username,
@@ -134,15 +150,8 @@ class RegistrationService(
134150
return user
135151
}
136152

137-
private fun checkUsernameAndEmail(username: String, email: String) {
153+
private fun checkUsername(username: String) {
138154
val usernameStatus = usernameAvailable(username)
139-
if (usernameStatus != UsernameStatus.USERNAME_AVAILABLE) {
140-
throw IllegalArgumentException("Username unavailable")
141-
}
142-
143-
val emailStatus = emailAvailable(email)
144-
if (emailStatus != EmailStatus.EMAIL_AVAILABLE) {
145-
throw IllegalArgumentException("Email unavailable")
146-
}
155+
require(usernameStatus == UsernameStatus.USERNAME_AVAILABLE) { "Username unavailable" }
147156
}
148157
}

‎src/main/kotlin/com/faforever/userservice/backend/domain/User.kt

+3
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ class UserRepository : PanacheRepositoryBase<User, Int> {
6464
fun findByUsernameOrEmail(usernameOrEmail: String): User? =
6565
find("username = ?1 or email = ?1", usernameOrEmail).firstResult()
6666

67+
fun findByEmail(email: String): User? =
68+
find("email = ?1", email).firstResult()
69+
6770
fun findUserPermissions(userId: Int): List<Permission> =
6871
getEntityManager().createNativeQuery(
6972
"""

‎src/main/kotlin/com/faforever/userservice/backend/email/EmailService.kt

+10
Original file line numberDiff line numberDiff line change
@@ -66,4 +66,14 @@ class EmailService(
6666
val mailBody = mailBodyBuilder.buildPasswordResetBody(username, passwordResetUrl)
6767
mailSender.sendMail(email, properties.account().passwordReset().subject(), mailBody, ContentType.HTML)
6868
}
69+
70+
fun sendEmailAlreadyTakenMail(
71+
desiredUsername: String,
72+
existingUsername: String,
73+
email: String,
74+
passwordResetUrl: String,
75+
) {
76+
val mailBody = mailBodyBuilder.buildEmailTakenBody(desiredUsername, existingUsername, passwordResetUrl)
77+
mailSender.sendMail(email, properties.account().registration().emailTakenSubject(), mailBody, ContentType.HTML)
78+
}
6979
}

‎src/main/kotlin/com/faforever/userservice/backend/email/MailBodyBuilder.kt

+12
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ class MailBodyBuilder(private val properties: FafProperties) {
2222
ACCOUNT_ACTIVATION("username", "activationUrl"),
2323
WELCOME_TO_FAF("username"),
2424
PASSWORD_RESET("username", "passwordResetUrl"),
25+
EMAIL_ALREADY_TAKEN("desiredUsername", "existingUsername", "passwordResetUrl"),
2526
;
2627

2728
val variables: Set<String>
@@ -36,6 +37,7 @@ class MailBodyBuilder(private val properties: FafProperties) {
3637
Template.ACCOUNT_ACTIVATION -> properties.account().registration().activationMailTemplatePath()
3738
Template.WELCOME_TO_FAF -> properties.account().registration().welcomeMailTemplatePath()
3839
Template.PASSWORD_RESET -> properties.account().passwordReset().mailTemplatePath()
40+
Template.EMAIL_ALREADY_TAKEN -> properties.account().registration().emailTakenMailTemplatePath()
3941
}
4042
return Path.of(path)
4143
}
@@ -125,4 +127,14 @@ class MailBodyBuilder(private val properties: FafProperties) {
125127
"passwordResetUrl" to passwordResetUrl,
126128
),
127129
)
130+
131+
fun buildEmailTakenBody(desiredUsername: String, existingUsername: String, passwordResetUrl: String) =
132+
populate(
133+
Template.EMAIL_ALREADY_TAKEN,
134+
mapOf(
135+
"desiredUsername" to desiredUsername,
136+
"existingUsername" to existingUsername,
137+
"passwordResetUrl" to passwordResetUrl,
138+
),
139+
)
128140
}

‎src/main/kotlin/com/faforever/userservice/config/FafProperties.kt

+10-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import io.smallrye.config.WithName
66
import jakarta.validation.constraints.NotBlank
77
import jakarta.validation.constraints.NotNull
88
import java.net.URI
9-
import java.util.*
9+
import java.util.Optional
1010

1111
@ConfigMapping(prefix = "faf")
1212
interface FafProperties {
@@ -106,6 +106,12 @@ interface FafProperties {
106106
@NotBlank
107107
fun welcomeMailTemplatePath(): String
108108

109+
@NotBlank
110+
fun emailTakenMailTemplatePath(): String
111+
112+
@NotBlank
113+
fun emailTakenSubject(): String
114+
109115
@NotBlank
110116
fun termsOfServiceUrl(): String
111117

@@ -123,6 +129,9 @@ interface FafProperties {
123129
@NotBlank
124130
fun passwordResetUrlFormat(): String
125131

132+
@NotBlank
133+
fun passwordResetInitiateEmailUrlFormat(): String
134+
126135
@NotBlank
127136
fun subject(): String
128137

‎src/main/kotlin/com/faforever/userservice/ui/view/recovery/RecoverViaEmailView.kt

+12-1
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@ import com.vaadin.flow.component.orderedlayout.HorizontalLayout
1717
import com.vaadin.flow.component.orderedlayout.VerticalLayout
1818
import com.vaadin.flow.component.textfield.TextField
1919
import com.vaadin.flow.data.value.ValueChangeMode
20+
import com.vaadin.flow.router.BeforeEnterEvent
21+
import com.vaadin.flow.router.BeforeEnterObserver
2022
import com.vaadin.flow.router.Route
2123

2224
@Route("/recover-account/email", layout = CardLayout::class)
2325
class RecoverViaEmailView(
2426
private val recoveryService: RecoveryService,
25-
) : CompactVerticalLayout() {
27+
) : CompactVerticalLayout(), BeforeEnterObserver {
2628

2729
private val usernameOrEmailDescription =
2830
Paragraph(
@@ -81,4 +83,13 @@ class RecoverViaEmailView(
8183
open()
8284
}
8385
}
86+
87+
override fun beforeEnter(event: BeforeEnterEvent?) {
88+
val possibleIdentifier = event?.location?.queryParameters?.parameters?.get("identifier")?.get(0)
89+
if (!possibleIdentifier.isNullOrBlank()) {
90+
usernameOrEmail.value = possibleIdentifier
91+
usernameOrEmail.isReadOnly = true
92+
requestEmail()
93+
}
94+
}
8495
}

‎src/main/kotlin/com/faforever/userservice/ui/view/registration/RegisterView.kt

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
package com.faforever.userservice.ui.view.registration
22

3-
import com.faforever.userservice.backend.account.EmailStatus
3+
import com.faforever.userservice.backend.account.EmailStatusResponse
44
import com.faforever.userservice.backend.account.RegistrationService
55
import com.faforever.userservice.backend.account.UsernameStatus
66
import com.faforever.userservice.backend.recaptcha.RecaptchaService
@@ -141,7 +141,7 @@ class RegisterView(
141141
).bind("username")
142142

143143
binder.forField(email).withValidator(EmailValidator(getTranslation("register.email.invalid"))).withValidator(
144-
{ email -> registrationService.emailAvailable(email) != EmailStatus.EMAIL_BLACKLISTED },
144+
{ email -> registrationService.emailAvailable(email) !is EmailStatusResponse.EmailBlackListed },
145145
getTranslation("register.email.blacklisted"),
146146
).bind("email")
147147

‎src/main/mjml/dummy/test-email-taken.html

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
To generate proper html code use the template in src/main/mjml with an mjml parser (e.g. use the IntelliJ plugin for MJML or follow the instructions on https://documentation.mjml.io/#usage
2+
Following variables are available:
3+
desiredUsername: {{desiredUsername}}
4+
existingUsername: {{existingUsername}}
5+
passwordResetUrl: {{passwordResetUrl}}

‎src/main/mjml/email-taken.mjml

+60
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
<mjml>
2+
<mj-head>
3+
<mj-include path="_style.mjml"/>
4+
<mj-preview>
5+
Account exists notice
6+
</mj-preview>
7+
</mj-head>
8+
<mj-body background-color="#fafafa">
9+
<mj-include path="_header.mjml"/>
10+
11+
<mj-section background-color="#ffffff">
12+
<mj-column>
13+
<mj-text>
14+
<h1>Account exists notice</h1>
15+
<p>Dear {{existingUsername}},</p>
16+
<p>We noticed someone tried to register an account with the username "{{desiredUsername}}"
17+
using your email address. However, your email is already associated with an existing
18+
account.
19+
</p>
20+
</mj-text>
21+
</mj-column>
22+
</mj-section>
23+
24+
<mj-section>
25+
<mj-column>
26+
<mj-text>
27+
<p>According to our rules each user may have only have one (1) FAF account.
28+
Maybe you forgot, that you already registered in the past, thus we send you this notice.
29+
</p>
30+
<p>If you have no more access to this account, you can reset the password right here:</p>
31+
</mj-text>
32+
<mj-button href="{{passwordResetUrl}}">
33+
Reset Password
34+
</mj-button>
35+
</mj-column>
36+
</mj-section>
37+
38+
<mj-section background-color="gray" padding="0">
39+
<mj-column>
40+
<mj-text color="#d3d3d3">
41+
<p>If you did not attempt to register a new account, please ignore this email.</p>
42+
</mj-text>
43+
</mj-column>
44+
</mj-section>
45+
46+
<mj-include path="_footer.mjml"/>
47+
48+
<mj-section padding="0">
49+
<mj-column>
50+
<mj-text color="gray">
51+
<p>If the button above doesn't work, you can enter the following URL manually in your
52+
browser:
53+
</p>
54+
<p>{{passwordResetUrl}}</p>
55+
</mj-text>
56+
</mj-column>
57+
</mj-section>
58+
59+
</mj-body>
60+
</mjml>

‎src/main/resources/application.yaml

+4
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,15 @@ faf:
1212
subject: ${REGISTRATION_EMAIL_SUBJECT:FAF user registration}
1313
activation-mail-template-path: ${ACCOUNT_ACTIVATION_MAIL_TEMPLATE_PATH:/config/mail/account-activation.html}
1414
welcome-subject: ${WELCOME_MAIL_SUBJECT:Welcome to FAF}
15+
email-taken-subject: ${EMAIL_TAKEN_SUBJECT:Account exists notice}
1516
welcome-mail-template-path: ${WELCOME_MAIL_TEMPLATE_PATH:/config/mail/welcome-to-faf.html}
17+
email-taken-mail-template-path: ${EMAIL_TAKEN_MAIL_TEMPLATE_PATH:/config/mail/email-taken.html}
1618
terms-of-service-url: ${FAF_TERMS_OF_SERVICE:https://faforever.com/tos}
1719
privacy-statement-url: ${FAF_PRIVACY_STATEMENT:https://faforever.com/privacy}
1820
rules-url: ${FAF_RULES:https://faforever.com/rules}
1921
password-reset:
2022
password-reset-url-format: ${faf.self-url}/recover-account/set-password?token=%s
23+
password-reset-initiate-email-url-format: ${faf.self-url}/recover-account/email?identifier=%s
2124
subject: ${PASSWORD_RESET_EMAIL_SUBJECT:FAF password reset}
2225
mail-template-path: ${PASSWORD_RESET_MAIL_TEMPLATE_PATH:/config/mail/password-reset.html}
2326
username:
@@ -103,6 +106,7 @@ quarkus:
103106
registration:
104107
activation-mail-template-path: ${ACCOUNT_ACTIVATION_MAIL_TEMPLATE_PATH:../../../../src/main/mjml/dummy/test-account-activation.html}
105108
welcome-mail-template-path: ${WELCOME_MAIL_TEMPLATE_PATH:../../../../src/main/mjml/dummy/test-welcome-to-faf.html}
109+
email-taken-mail-template-path: ${EMAIL_TAKEN_MAIL_TEMPLATE_PATH:../../../../src/main/mjml/dummy/test-email-taken.html}
106110
password-reset:
107111
mail-template-path: ${PASSWORD_RESET_MAIL_TEMPLATE_PATH:../../../../src/main/mjml/dummy/test-password-reset.html}
108112
irc:

‎src/test/kotlin/com/faforever/userservice/backend/account/RegistrationServiceTest.kt

+14-4
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@ import com.faforever.userservice.backend.domain.IpAddress
55
import com.faforever.userservice.backend.domain.NameRecordRepository
66
import com.faforever.userservice.backend.domain.User
77
import com.faforever.userservice.backend.domain.UserRepository
8+
import com.faforever.userservice.backend.email.EmailService
89
import com.faforever.userservice.backend.security.FafTokenService
910
import com.faforever.userservice.config.FafProperties
1011
import io.quarkus.mailer.MockMailbox
1112
import io.quarkus.test.InjectMock
1213
import io.quarkus.test.junit.QuarkusTest
14+
import io.quarkus.test.junit.mockito.InjectSpy
1315
import jakarta.inject.Inject
1416
import org.hamcrest.MatcherAssert.assertThat
1517
import org.hamcrest.Matchers.hasSize
@@ -20,6 +22,7 @@ import org.junit.jupiter.api.Test
2022
import org.junit.jupiter.api.assertThrows
2123
import org.mockito.ArgumentMatchers.anyString
2224
import org.mockito.kotlin.any
25+
import org.mockito.kotlin.times
2326
import org.mockito.kotlin.verify
2427
import org.mockito.kotlin.whenever
2528

@@ -35,6 +38,9 @@ class RegistrationServiceTest {
3538
private val user = User(1, username, password, email, null)
3639
}
3740

41+
@InjectSpy
42+
private lateinit var emailService: EmailService
43+
3844
@Inject
3945
private lateinit var registrationService: RegistrationService
4046

@@ -87,16 +93,20 @@ class RegistrationServiceTest {
8793

8894
@Test
8995
fun registerEmailTaken() {
90-
whenever(userRepository.existsByEmail(anyString())).thenReturn(true)
96+
whenever(userRepository.findByEmail(anyString())).thenReturn(user)
97+
val newTestUsername = "newUsername"
9198

92-
assertThrows<IllegalArgumentException> { registrationService.register(username, email) }
99+
registrationService.register(newTestUsername, email)
100+
101+
val expectedLink = fafProperties.account().passwordReset().passwordResetInitiateEmailUrlFormat().format(email)
102+
verify(emailService, times(1)).sendEmailAlreadyTakenMail(newTestUsername, username, email, expectedLink)
93103
}
94104

95105
@Test
96106
fun registerEmailBlacklisted() {
97107
whenever(domainBlacklistRepository.existsByDomain(anyString())).thenReturn(true)
98108

99-
assertThrows<IllegalArgumentException> { registrationService.register(username, email) }
109+
assertThrows<IllegalStateException> { registrationService.register(username, email) }
100110
}
101111

102112
@Test
@@ -131,7 +141,7 @@ class RegistrationServiceTest {
131141

132142
@Test
133143
fun activateEmailTaken() {
134-
whenever(userRepository.existsByEmail(anyString())).thenReturn(true)
144+
whenever(userRepository.findByEmail(anyString())).thenReturn(user)
135145

136146
assertThrows<IllegalArgumentException> {
137147
registrationService.activate(RegisteredUser(username, email), ipAddress, password)

‎src/test/resources/application.yaml

+1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ faf:
55
registration:
66
activation-mail-template-path: src/test/resources/mail/account-activation.html
77
welcome-mail-template-path: src/test/resources/mail/welcome-to-faf.html
8+
email-taken-mail-template-path: src/test/resources/mail/email-taken.html
89

910
irc:
1011
fixed:

‎src/test/resources/mail/email-taken.html

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
{{desiredUsername}}
2+
{{existingUsername}}
3+
{{passwordResetUrl}}

0 commit comments

Comments
 (0)