Disclose that email is already in use after verification

Author: sandhose

crates/handlers/src/graphql/mutations/user_email.rs

@@ -242,7 +242,7 @@ enum StartEmailAuthenticationStatus {
     RateLimited,
     /// The email address isn't allowed by the policy
     Denied,
-    /// The email address is already in use
+    /// The email address is already in use on this account
     InUse,
 }
 
@@ -308,6 +308,7 @@ enum CompleteEmailAuthenticationPayload {
     Completed,
     InvalidCode,
     CodeExpired,
+    InUse,
     RateLimited,
 }
 
@@ -322,6 +323,8 @@ enum CompleteEmailAuthenticationStatus {
     CodeExpired,
     /// Too many attempts to complete an email authentication
     RateLimited,
+    /// The email address is already in use
+    InUse,
 }
 
 #[Object(use_type_description)]
@@ -332,6 +335,7 @@ impl CompleteEmailAuthenticationPayload {
             Self::Completed => CompleteEmailAuthenticationStatus::Completed,
             Self::InvalidCode => CompleteEmailAuthenticationStatus::InvalidCode,
             Self::CodeExpired => CompleteEmailAuthenticationStatus::CodeExpired,
+            Self::InUse => CompleteEmailAuthenticationStatus::InUse,
             Self::RateLimited => CompleteEmailAuthenticationStatus::RateLimited,
         }
     }
@@ -588,10 +592,17 @@ impl UserEmailMutations {
 
         let mut repo = state.repository().await?;
 
-        // Check if the email address is already in use
+        // Check if the email address is already in use by the same user
+        // We don't report here if the email address is already in use by another user,
+        // because we don't want to leak information about other users. We will do that
+        // only when the user enters the right verification code
         let count = repo
             .user_email()
-            .count(UserEmailFilter::new().for_email(&input.email))
+            .count(
+                UserEmailFilter::new()
+                    .for_email(&input.email)
+                    .for_user(&browser_session.user),
+            )
             .await?;
         if count > 0 {
             return Ok(StartEmailAuthenticationPayload::InUse);
@@ -742,19 +753,23 @@ impl UserEmailMutations {
             return Ok(CompleteEmailAuthenticationPayload::CodeExpired);
         }
 
-        // Check that we can add the email address to the user
+        let authentication = repo
+            .user_email()
+            .complete_authentication(&clock, authentication, &code)
+            .await?;
+
+        // Check the email is not already in use by anyone, including the current user
         let count = repo
             .user_email()
             .count(UserEmailFilter::new().for_email(&authentication.email))
             .await?;
+
         if count > 0 {
-            return Ok(CompleteEmailAuthenticationPayload::CodeExpired);
-        }
+            // We still want to consume the code so that it can't be reused
+            repo.save().await?;
 
-        let authentication = repo
-            .user_email()
-            .complete_authentication(&clock, authentication, &code)
-            .await?;
+            return Ok(CompleteEmailAuthenticationPayload::InUse);
+        }
 
         repo.user_email()
             .add(

crates/handlers/src/views/register/password.rs

@@ -25,7 +25,7 @@ use mas_policy::Policy;
 use mas_router::UrlBuilder;
 use mas_storage::{
     queue::{QueueJobRepositoryExt as _, SendEmailAuthenticationCodeJob},
-    user::{UserEmailFilter, UserEmailRepository, UserRepository},
+    user::{UserEmailRepository, UserRepository},
     BoxClock, BoxRepository, BoxRng, RepositoryAccess,
 };
 use mas_templates::{
@@ -191,17 +191,13 @@ pub(crate) async fn post(
             homeserver_denied_username = true;
         }
 
+        // Note that we don't check here if the email is already taken here, as
+        // we don't want to leak the information about other users. Instead, we will
+        // show an error message once the user confirmed their email address.
         if form.email.is_empty() {
             state.add_error_on_field(RegisterFormField::Email, FieldError::Required);
         } else if Address::from_str(&form.email).is_err() {
             state.add_error_on_field(RegisterFormField::Email, FieldError::Invalid);
-        } else if repo
-            .user_email()
-            .count(UserEmailFilter::new().for_email(&form.email))
-            .await?
-            > 0
-        {
-            state.add_error_on_field(RegisterFormField::Email, FieldError::Exists);
         }
 
         if form.password.is_empty() {

crates/handlers/src/views/register/steps/finish.rs

@@ -6,7 +6,7 @@
 use anyhow::Context as _;
 use axum::{
     extract::{Path, State},
-    response::IntoResponse,
+    response::{Html, IntoResponse, Response},
 };
 use axum_extra::TypedHeader;
 use chrono::Duration;
@@ -19,10 +19,11 @@ use mas_storage::{
     user::UserEmailFilter,
     BoxClock, BoxRepository, BoxRng,
 };
+use mas_templates::{RegisterStepsEmailInUseContext, TemplateContext as _, Templates};
 use ulid::Ulid;
 
 use super::super::cookie::UserRegistrationSessions;
-use crate::{views::shared::OptionalPostAuthAction, BoundActivityTracker};
+use crate::{views::shared::OptionalPostAuthAction, BoundActivityTracker, PreferredLanguage};
 
 #[tracing::instrument(
     name = "handlers.views.register.steps.finish.get",
@@ -38,9 +39,11 @@ pub(crate) async fn get(
     user_agent: Option<TypedHeader<headers::UserAgent>>,
     State(url_builder): State<UrlBuilder>,
     State(homeserver): State<BoxHomeserverConnection>,
+    State(templates): State<Templates>,
+    PreferredLanguage(lang): PreferredLanguage,
     cookie_jar: CookieJar,
     Path(id): Path<Ulid>,
-) -> Result<impl IntoResponse, FancyError> {
+) -> Result<Response, FancyError> {
     let user_agent = user_agent.map(|ua| UserAgent::parse(ua.as_str().to_owned()));
     let registration = repo
         .user_registration()
@@ -60,7 +63,8 @@ pub(crate) async fn get(
         return Ok((
             cookie_jar,
             OptionalPostAuthAction::from(post_auth_action).go_next(&url_builder),
-        ));
+        )
+            .into_response());
     }
 
     // Make sure the registration session hasn't expired
@@ -117,29 +121,42 @@ pub(crate) async fn get(
         return Ok((
             cookie_jar,
             url_builder.redirect(&mas_router::RegisterVerifyEmail::new(id)),
-        ));
+        )
+            .into_response());
     }
 
     // Check that the email address isn't already used
+    // It is important to do that here, as we we're not checking during the
+    // registration, because we don't want to disclose whether an email is
+    // already being used or not before we verified it
     if repo
         .user_email()
         .count(UserEmailFilter::new().for_email(&email_authentication.email))
         .await?
         > 0
     {
-        // XXX: this could have a better error message, but as this is unlikely to
-        // happen, we're fine with a vague message for now
-        return Err(FancyError::from(anyhow::anyhow!(
-            "Email address is already used"
-        )));
+        let action = registration
+            .post_auth_action
+            .map(serde_json::from_value)
+            .transpose()?;
+
+        let ctx = RegisterStepsEmailInUseContext::new(email_authentication.email, action)
+            .with_language(lang);
+
+        return Ok((
+            cookie_jar,
+            Html(templates.render_register_steps_email_in_use(&ctx)?),
+        )
+            .into_response());
     }
 
     // Check that the display name is set
     if registration.display_name.is_none() {
         return Ok((
             cookie_jar,
             url_builder.redirect(&mas_router::RegisterDisplayName::new(registration.id)),
-        ));
+        )
+            .into_response());
     }
 
     // Everuthing is good, let's complete the registration
@@ -215,5 +232,6 @@ pub(crate) async fn get(
     return Ok((
         cookie_jar,
         OptionalPostAuthAction::from(post_auth_action).go_next(&url_builder),
-    ));
+    )
+        .into_response());
 }

crates/templates/src/context.rs

@@ -1000,6 +1000,32 @@ impl TemplateContext for RegisterStepsVerifyEmailContext {
     }
 }
 
+/// Context used by the `pages/register/steps/email_in_use.html` template
+#[derive(Serialize)]
+pub struct RegisterStepsEmailInUseContext {
+    email: String,
+    action: Option<PostAuthAction>,
+}
+
+impl RegisterStepsEmailInUseContext {
+    /// Constructs a context for the email in use page
+    #[must_use]
+    pub fn new(email: String, action: Option<PostAuthAction>) -> Self {
+        Self { email, action }
+    }
+}
+
+impl TemplateContext for RegisterStepsEmailInUseContext {
+    fn sample(_now: chrono::DateTime<Utc>, _rng: &mut impl Rng) -> Vec<Self>
+    where
+        Self: Sized,
+    {
+        let email = "[email protected]".to_owned();
+        let action = PostAuthAction::continue_grant(Ulid::nil());
+        vec![Self::new(email, Some(action))]
+    }
+}
+
 /// Fields for the display name form
 #[derive(Serialize, Deserialize, Debug, Clone, Copy, Hash, PartialEq, Eq)]
 #[serde(rename_all = "snake_case")]

crates/templates/src/lib.rs

@@ -42,10 +42,10 @@ pub use self::{
         RecoveryFinishContext, RecoveryFinishFormField, RecoveryProgressContext,
         RecoveryStartContext, RecoveryStartFormField, RegisterContext, RegisterFormField,
         RegisterStepsDisplayNameContext, RegisterStepsDisplayNameFormField,
-        RegisterStepsVerifyEmailContext, RegisterStepsVerifyEmailFormField, SiteBranding,
-        SiteConfigExt, SiteFeatures, TemplateContext, UpstreamExistingLinkContext,
-        UpstreamRegister, UpstreamRegisterFormField, UpstreamSuggestLink, WithCaptcha, WithCsrf,
-        WithLanguage, WithOptionalSession, WithSession,
+        RegisterStepsEmailInUseContext, RegisterStepsVerifyEmailContext,
+        RegisterStepsVerifyEmailFormField, SiteBranding, SiteConfigExt, SiteFeatures,
+        TemplateContext, UpstreamExistingLinkContext, UpstreamRegister, UpstreamRegisterFormField,
+        UpstreamSuggestLink, WithCaptcha, WithCsrf, WithLanguage, WithOptionalSession, WithSession,
     },
     forms::{FieldError, FormError, FormField, FormState, ToFormState},
 };
@@ -336,6 +336,9 @@ register_templates! {
     /// Render the email verification page
     pub fn render_register_steps_verify_email(WithLanguage<WithCsrf<RegisterStepsVerifyEmailContext>>) { "pages/register/steps/verify_email.html" }
 
+    /// Render the email in use page
+    pub fn render_register_steps_email_in_use(WithLanguage<RegisterStepsEmailInUseContext>) { "pages/register/steps/email_in_use.html" }
+
     /// Render the display name page
     pub fn render_register_steps_display_name(WithLanguage<WithCsrf<RegisterStepsDisplayNameContext>>) { "pages/register/steps/display_name.html" }
 
@@ -432,6 +435,7 @@ impl Templates {
         check::render_register(self, now, rng)?;
         check::render_password_register(self, now, rng)?;
         check::render_register_steps_verify_email(self, now, rng)?;
+        check::render_register_steps_email_in_use(self, now, rng)?;
         check::render_register_steps_display_name(self, now, rng)?;
         check::render_consent(self, now, rng)?;
         check::render_policy_violation(self, now, rng)?;

frontend/locales/en.json

@@ -78,6 +78,9 @@
       "tablet": "Tablet",
       "unknown": "Unknown device type"
     },
+    "email_in_use": {
+      "heading": "The email address {{email}} is already in use."
+    },
     "end_session_button": {
       "confirmation_modal_title": "Are you sure you want to end this session?",
       "text": "Sign out"
@@ -266,6 +269,10 @@
       }
     },
     "verify_email": {
+      "code_expired_alert": {
+        "description": "The code has expired. Please request a new code.",
+        "title": "Code expired"
+      },
       "code_field_error": "Code not recognised",
       "code_field_label": "6-digit code",
       "code_field_wrong_shape": "Code must be 6 digits",

frontend/schema.graphql

@@ -528,6 +528,10 @@ enum CompleteEmailAuthenticationStatus {
   Too many attempts to complete an email authentication
   """
   RATE_LIMITED
+  """
+  The email address is already in use
+  """
+  IN_USE
 }
 
 """
@@ -1650,7 +1654,7 @@ enum StartEmailAuthenticationStatus {
   """
   DENIED
   """
-  The email address is already in use
+  The email address is already in use on this account
   """
   IN_USE
 }

frontend/src/gql/graphql.ts

@@ -349,6 +349,8 @@ export type CompleteEmailAuthenticationStatus =
   | 'COMPLETED'
   /** The authentication code is invalid */
   | 'INVALID_CODE'
+  /** The email address is already in use */
+  | 'IN_USE'
   /** Too many attempts to complete an email authentication */
   | 'RATE_LIMITED';
 
@@ -1207,7 +1209,7 @@ export type StartEmailAuthenticationStatus =
   | 'DENIED'
   /** The email address is invalid */
   | 'INVALID_EMAIL_ADDRESS'
-  /** The email address is already in use */
+  /** The email address is already in use on this account */
   | 'IN_USE'
   /** Too many attempts to start an email authentication */
   | 'RATE_LIMITED'