From 7f1b3866bac93d4d257c072c0b997bb01d17ec2d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 23 Jan 2025 18:18:19 +0100 Subject: [PATCH] Disclose that email is already in use after verification --- .../src/graphql/mutations/user_email.rs | 39 +++++++++---- .../handlers/src/views/register/password.rs | 12 ++-- .../src/views/register/steps/finish.rs | 42 ++++++++++---- crates/templates/src/context.rs | 26 +++++++++ crates/templates/src/lib.rs | 12 ++-- frontend/locales/en.json | 7 +++ frontend/schema.graphql | 6 +- frontend/src/gql/graphql.ts | 4 +- frontend/src/routeTree.gen.ts | 26 +++++++++ frontend/src/routes/emails.$id.in-use.tsx | 56 +++++++++++++++++++ .../src/routes/emails.$id.verify.lazy.tsx | 13 +++++ .../pages/register/steps/email_in_use.html | 30 ++++++++++ translations/en.json | 12 +++- 13 files changed, 246 insertions(+), 39 deletions(-) create mode 100644 frontend/src/routes/emails.$id.in-use.tsx create mode 100644 templates/pages/register/steps/email_in_use.html diff --git a/crates/handlers/src/graphql/mutations/user_email.rs b/crates/handlers/src/graphql/mutations/user_email.rs index 41ec2f01c..38048d39a 100644 --- a/crates/handlers/src/graphql/mutations/user_email.rs +++ b/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,20 +753,24 @@ impl UserEmailMutations { return Ok(CompleteEmailAuthenticationPayload::CodeExpired); } - // Check that we can add the email address to the user - let count = repo - .user_email() - .count(UserEmailFilter::new().for_email(&authentication.email)) - .await?; - if count > 0 { - return Ok(CompleteEmailAuthenticationPayload::CodeExpired); - } - 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 { + // We still want to consume the code so that it can't be reused + repo.save().await?; + + return Ok(CompleteEmailAuthenticationPayload::InUse); + } + repo.user_email() .add( &mut rng, diff --git a/crates/handlers/src/views/register/password.rs b/crates/handlers/src/views/register/password.rs index 624b22508..c2177c484 100644 --- a/crates/handlers/src/views/register/password.rs +++ b/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() { diff --git a/crates/handlers/src/views/register/steps/finish.rs b/crates/handlers/src/views/register/steps/finish.rs index 21e382682..b460d6aed 100644 --- a/crates/handlers/src/views/register/steps/finish.rs +++ b/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>, State(url_builder): State, State(homeserver): State, + State(templates): State, + PreferredLanguage(lang): PreferredLanguage, cookie_jar: CookieJar, Path(id): Path, -) -> Result { +) -> Result { 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,21 +121,33 @@ 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 @@ -139,7 +155,8 @@ pub(crate) async fn get( 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()); } diff --git a/crates/templates/src/context.rs b/crates/templates/src/context.rs index f5e652ed4..7fcfcf8f6 100644 --- a/crates/templates/src/context.rs +++ b/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, +} + +impl RegisterStepsEmailInUseContext { + /// Constructs a context for the email in use page + #[must_use] + pub fn new(email: String, action: Option) -> Self { + Self { email, action } + } +} + +impl TemplateContext for RegisterStepsEmailInUseContext { + fn sample(_now: chrono::DateTime, _rng: &mut impl Rng) -> Vec + where + Self: Sized, + { + let email = "hello@example.com".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")] diff --git a/crates/templates/src/lib.rs b/crates/templates/src/lib.rs index 07e67cc63..60482f792 100644 --- a/crates/templates/src/lib.rs +++ b/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>) { "pages/register/steps/verify_email.html" } + /// Render the email in use page + pub fn render_register_steps_email_in_use(WithLanguage) { "pages/register/steps/email_in_use.html" } + /// Render the display name page pub fn render_register_steps_display_name(WithLanguage>) { "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)?; diff --git a/frontend/locales/en.json b/frontend/locales/en.json index cc977da72..72df9d5fe 100644 --- a/frontend/locales/en.json +++ b/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", diff --git a/frontend/schema.graphql b/frontend/schema.graphql index 1602835a6..58678a73b 100644 --- a/frontend/schema.graphql +++ b/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 } diff --git a/frontend/src/gql/graphql.ts b/frontend/src/gql/graphql.ts index fbc5e83b2..c342bd3f4 100644 --- a/frontend/src/gql/graphql.ts +++ b/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' diff --git a/frontend/src/routeTree.gen.ts b/frontend/src/routeTree.gen.ts index bafc24851..2c412c68c 100644 --- a/frontend/src/routeTree.gen.ts +++ b/frontend/src/routeTree.gen.ts @@ -25,6 +25,7 @@ import { Route as PasswordRecoveryIndexImport } from './routes/password.recovery import { Route as PasswordChangeIndexImport } from './routes/password.change.index' import { Route as AccountSessionsIndexImport } from './routes/_account.sessions.index' import { Route as EmailsIdVerifyImport } from './routes/emails.$id.verify' +import { Route as EmailsIdInUseImport } from './routes/emails.$id.in-use' import { Route as AccountSessionsBrowsersImport } from './routes/_account.sessions.browsers' import { Route as AccountSessionsIdImport } from './routes/_account.sessions.$id' @@ -127,6 +128,12 @@ const EmailsIdVerifyRoute = EmailsIdVerifyImport.update({ import('./routes/emails.$id.verify.lazy').then((d) => d.Route), ) +const EmailsIdInUseRoute = EmailsIdInUseImport.update({ + id: '/emails/$id/in-use', + path: '/emails/$id/in-use', + getParentRoute: () => rootRoute, +} as any) + const AccountSessionsBrowsersRoute = AccountSessionsBrowsersImport.update({ id: '/sessions/browsers', path: '/sessions/browsers', @@ -217,6 +224,13 @@ declare module '@tanstack/react-router' { preLoaderRoute: typeof AccountSessionsBrowsersImport parentRoute: typeof AccountImport } + '/emails/$id/in-use': { + id: '/emails/$id/in-use' + path: '/emails/$id/in-use' + fullPath: '/emails/$id/in-use' + preLoaderRoute: typeof EmailsIdInUseImport + parentRoute: typeof rootRoute + } '/emails/$id/verify': { id: '/emails/$id/verify' path: '/emails/$id/verify' @@ -300,6 +314,7 @@ export interface FileRoutesByFullPath { '/reset-cross-signing/': typeof ResetCrossSigningIndexRoute '/sessions/$id': typeof AccountSessionsIdRoute '/sessions/browsers': typeof AccountSessionsBrowsersRoute + '/emails/$id/in-use': typeof EmailsIdInUseRoute '/emails/$id/verify': typeof EmailsIdVerifyRoute '/password/change/success': typeof PasswordChangeSuccessLazyRoute '/sessions': typeof AccountSessionsIndexRoute @@ -316,6 +331,7 @@ export interface FileRoutesByTo { '/reset-cross-signing': typeof ResetCrossSigningIndexRoute '/sessions/$id': typeof AccountSessionsIdRoute '/sessions/browsers': typeof AccountSessionsBrowsersRoute + '/emails/$id/in-use': typeof EmailsIdInUseRoute '/emails/$id/verify': typeof EmailsIdVerifyRoute '/password/change/success': typeof PasswordChangeSuccessLazyRoute '/sessions': typeof AccountSessionsIndexRoute @@ -335,6 +351,7 @@ export interface FileRoutesById { '/reset-cross-signing/': typeof ResetCrossSigningIndexRoute '/_account/sessions/$id': typeof AccountSessionsIdRoute '/_account/sessions/browsers': typeof AccountSessionsBrowsersRoute + '/emails/$id/in-use': typeof EmailsIdInUseRoute '/emails/$id/verify': typeof EmailsIdVerifyRoute '/password/change/success': typeof PasswordChangeSuccessLazyRoute '/_account/sessions/': typeof AccountSessionsIndexRoute @@ -355,6 +372,7 @@ export interface FileRouteTypes { | '/reset-cross-signing/' | '/sessions/$id' | '/sessions/browsers' + | '/emails/$id/in-use' | '/emails/$id/verify' | '/password/change/success' | '/sessions' @@ -370,6 +388,7 @@ export interface FileRouteTypes { | '/reset-cross-signing' | '/sessions/$id' | '/sessions/browsers' + | '/emails/$id/in-use' | '/emails/$id/verify' | '/password/change/success' | '/sessions' @@ -387,6 +406,7 @@ export interface FileRouteTypes { | '/reset-cross-signing/' | '/_account/sessions/$id' | '/_account/sessions/browsers' + | '/emails/$id/in-use' | '/emails/$id/verify' | '/password/change/success' | '/_account/sessions/' @@ -400,6 +420,7 @@ export interface RootRouteChildren { ResetCrossSigningRoute: typeof ResetCrossSigningRouteWithChildren ClientsIdRoute: typeof ClientsIdRoute DevicesSplatRoute: typeof DevicesSplatRoute + EmailsIdInUseRoute: typeof EmailsIdInUseRoute EmailsIdVerifyRoute: typeof EmailsIdVerifyRoute PasswordChangeSuccessLazyRoute: typeof PasswordChangeSuccessLazyRoute PasswordChangeIndexRoute: typeof PasswordChangeIndexRoute @@ -411,6 +432,7 @@ const rootRouteChildren: RootRouteChildren = { ResetCrossSigningRoute: ResetCrossSigningRouteWithChildren, ClientsIdRoute: ClientsIdRoute, DevicesSplatRoute: DevicesSplatRoute, + EmailsIdInUseRoute: EmailsIdInUseRoute, EmailsIdVerifyRoute: EmailsIdVerifyRoute, PasswordChangeSuccessLazyRoute: PasswordChangeSuccessLazyRoute, PasswordChangeIndexRoute: PasswordChangeIndexRoute, @@ -431,6 +453,7 @@ export const routeTree = rootRoute "/reset-cross-signing", "/clients/$id", "/devices/$", + "/emails/$id/in-use", "/emails/$id/verify", "/password/change/success", "/password/change/", @@ -484,6 +507,9 @@ export const routeTree = rootRoute "filePath": "_account.sessions.browsers.tsx", "parent": "/_account" }, + "/emails/$id/in-use": { + "filePath": "emails.$id.in-use.tsx" + }, "/emails/$id/verify": { "filePath": "emails.$id.verify.tsx" }, diff --git a/frontend/src/routes/emails.$id.in-use.tsx b/frontend/src/routes/emails.$id.in-use.tsx new file mode 100644 index 000000000..c12120f5c --- /dev/null +++ b/frontend/src/routes/emails.$id.in-use.tsx @@ -0,0 +1,56 @@ +// Copyright 2025 New Vector Ltd. +// +// SPDX-License-Identifier: AGPL-3.0-only +// Please see LICENSE in the repository root for full details. + +import { useSuspenseQuery } from "@tanstack/react-query"; +import { createFileRoute, notFound, redirect } from "@tanstack/react-router"; +import IconArrowLeft from "@vector-im/compound-design-tokens/assets/web/icons/arrow-left"; +import IconError from "@vector-im/compound-design-tokens/assets/web/icons/error"; +import { useTranslation } from "react-i18next"; +import { ButtonLink } from "../components/ButtonLink"; +import Layout from "../components/Layout"; +import PageHeading from "../components/PageHeading"; +import { query } from "./emails.$id.verify"; + +export const Route = createFileRoute("/emails/$id/in-use")({ + async loader({ context, params }): Promise { + const data = await context.queryClient.ensureQueryData(query(params.id)); + if (!data.userEmailAuthentication) { + throw notFound(); + } + + // If the user has not completed the verification process, it means they got + // to this page by mistake + if (!data.userEmailAuthentication.completedAt) { + throw redirect({ to: "/emails/$id/verify", params }); + } + }, + + component: EmailInUse, +}); + +function EmailInUse(): React.ReactElement { + const { id } = Route.useParams(); + const { + data: { userEmailAuthentication }, + } = useSuspenseQuery(query(id)); + if (!userEmailAuthentication) throw notFound(); + const { t } = useTranslation(); + + return ( + + + + + {t("action.back")} + + + ); +} diff --git a/frontend/src/routes/emails.$id.verify.lazy.tsx b/frontend/src/routes/emails.$id.verify.lazy.tsx index 480109ef0..71c6f3d6f 100644 --- a/frontend/src/routes/emails.$id.verify.lazy.tsx +++ b/frontend/src/routes/emails.$id.verify.lazy.tsx @@ -59,6 +59,8 @@ function EmailVerify(): React.ReactElement { if (data.completeEmailAuthentication.status === "COMPLETED") { await navigate({ to: "/" }); + } else if (data.completeEmailAuthentication.status === "IN_USE") { + await navigate({ to: "/emails/$id/in-use", params: { id } }); } }, }); @@ -99,6 +101,8 @@ function EmailVerify(): React.ReactElement { "RESENT"; const invalidCode = verifyEmail.data?.completeEmailAuthentication.status === "INVALID_CODE"; + const codeExpired = + verifyEmail.data?.completeEmailAuthentication.status === "CODE_EXPIRED"; const rateLimited = verifyEmail.data?.completeEmailAuthentication.status === "RATE_LIMITED"; @@ -135,6 +139,15 @@ function EmailVerify(): React.ReactElement { )} + {codeExpired && ( + + {t("frontend.verify_email.code_expired_alert.description")} + + )} + {rateLimited && ( +
+
+ {{ icon.error() }} +
+ +
+

+ {{ _("mas.email_in_use.title", email=email) }} +

+

+ {{ _("mas.email_in_use.description") }} +

+
+
+ + {% set params = action | default({}) | to_params(prefix="?") %} + {{ button.link_outline(text=_("action.start_over"), href="/register" ~ params) }} + +{% endblock %} diff --git a/translations/en.json b/translations/en.json index c3797507a..0da0ccf85 100644 --- a/translations/en.json +++ b/translations/en.json @@ -30,7 +30,7 @@ }, "start_over": "Start over", "@start_over": { - "context": "pages/recovery/consumed.html:22:32-54, pages/recovery/expired.html:30:32-54" + "context": "pages/recovery/consumed.html:22:32-54, pages/recovery/expired.html:30:32-54, pages/register/steps/email_in_use.html:28:32-54" } }, "app": { @@ -232,6 +232,16 @@ } } }, + "email_in_use": { + "description": "If you have forgotten your account credentials, you can recover your account. You can also start over and use a different email address.", + "@description": { + "context": "pages/register/steps/email_in_use.html:22:13-46" + }, + "title": "The email address %(email)s is already in use", + "@title": { + "context": "pages/register/steps/email_in_use.html:19:13-53" + } + }, "emails": { "greeting": "Hello %(username)s,", "@greeting": {