From f03a817738d91f4bd35051397b11ff1ec0a7cd9d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 15 Jan 2025 12:45:48 +0100 Subject: [PATCH] Move the finishing of registration to a dedicated view --- crates/handlers/src/lib.rs | 4 + .../handlers/src/views/register/password.rs | 6 +- .../src/views/register/steps/finish.rs | 174 ++++++++++++++++++ .../handlers/src/views/register/steps/mod.rs | 1 + .../src/views/register/steps/verify_email.rs | 97 ++-------- crates/router/src/endpoints.rs | 30 ++- 6 files changed, 225 insertions(+), 87 deletions(-) create mode 100644 crates/handlers/src/views/register/steps/finish.rs diff --git a/crates/handlers/src/lib.rs b/crates/handlers/src/lib.rs index 39e45a8e5..66876a25b 100644 --- a/crates/handlers/src/lib.rs +++ b/crates/handlers/src/lib.rs @@ -383,6 +383,10 @@ where get(self::views::register::steps::verify_email::get) .post(self::views::register::steps::verify_email::post), ) + .route( + mas_router::RegisterFinish::route(), + get(self::views::register::steps::finish::get), + ) .route( mas_router::AccountRecoveryStart::route(), get(self::views::recovery::start::get).post(self::views::recovery::start::post), diff --git a/crates/handlers/src/views/register/password.rs b/crates/handlers/src/views/register/password.rs index 6853f43c9..37cfd861b 100644 --- a/crates/handlers/src/views/register/password.rs +++ b/crates/handlers/src/views/register/password.rs @@ -362,7 +362,7 @@ pub(crate) async fn post( repo.save().await?; Ok(url_builder - .redirect(&mas_router::RegisterVerifyEmail::new(registration.id)) + .redirect(&mas_router::RegisterFinish::new(registration.id)) .into_response()) } @@ -479,12 +479,12 @@ mod tests { response.assert_status(StatusCode::SEE_OTHER); let location = response.headers().get(LOCATION).unwrap(); - // The handler redirects with the ID as the last portion of the path + // The handler redirects with the ID as the second to last portion of the path let id = location .to_str() .unwrap() .rsplit('/') - .next() + .nth(1) .unwrap() .parse() .unwrap(); diff --git a/crates/handlers/src/views/register/steps/finish.rs b/crates/handlers/src/views/register/steps/finish.rs new file mode 100644 index 000000000..55db0a6c5 --- /dev/null +++ b/crates/handlers/src/views/register/steps/finish.rs @@ -0,0 +1,174 @@ +// Copyright 2025 New Vector Ltd. +// +// SPDX-License-Identifier: AGPL-3.0-only +// Please see LICENSE in the repository root for full details. + +use anyhow::Context as _; +use axum::{ + extract::{Path, State}, + response::IntoResponse, +}; +use axum_extra::TypedHeader; +use mas_axum_utils::{cookies::CookieJar, FancyError, SessionInfoExt as _}; +use mas_data_model::UserAgent; +use mas_router::{PostAuthAction, UrlBuilder}; +use mas_storage::{ + queue::{ProvisionUserJob, QueueJobRepositoryExt as _}, + user::UserEmailFilter, + BoxClock, BoxRepository, BoxRng, +}; +use ulid::Ulid; + +use crate::{views::shared::OptionalPostAuthAction, BoundActivityTracker}; + +#[tracing::instrument( + name = "handlers.views.register.steps.finish.get", + fields(user_registration.id = %id), + skip_all, + err, +)] +pub(crate) async fn get( + mut rng: BoxRng, + clock: BoxClock, + mut repo: BoxRepository, + activity_tracker: BoundActivityTracker, + user_agent: Option>, + State(url_builder): State, + cookie_jar: CookieJar, + Path(id): Path, +) -> Result { + let user_agent = user_agent.map(|ua| UserAgent::parse(ua.as_str().to_owned())); + let registration = repo + .user_registration() + .lookup(id) + .await? + .context("User registration not found")?; + + // If the registration is completed, we can go to the registration destination + // XXX: this might not be the right thing to do? Maybe an error page would be + // better? + if registration.completed_at.is_some() { + let post_auth_action: Option = registration + .post_auth_action + .map(serde_json::from_value) + .transpose()?; + + return Ok(( + cookie_jar, + OptionalPostAuthAction::from(post_auth_action).go_next(&url_builder), + )); + } + + // Let's perform last minute checks on the registration, especially to avoid + // race conditions where multiple users register with the same username or email + // address + + if repo.user().exists(®istration.username).await? { + return Err(FancyError::from(anyhow::anyhow!( + "Username is already taken" + ))); + } + + // TODO: query the homeserver + + // For now, we require an email address on the registration, but this might + // change in the future + let email_authentication_id = registration + .email_authentication_id + .context("No email authentication started for this registration")?; + let email_authentication = repo + .user_email() + .lookup_authentication(email_authentication_id) + .await? + .context("Could not load the email authentication")?; + + // Check that the email authentication has been completed + if email_authentication.completed_at.is_none() { + return Ok(( + cookie_jar, + url_builder.redirect(&mas_router::RegisterVerifyEmail::new(id)), + )); + } + + // Check that the email address isn't already used + if repo + .user_email() + .count(UserEmailFilter::new().for_email(&email_authentication.email)) + .await? + > 0 + { + return Err(FancyError::from(anyhow::anyhow!( + "Email address is already used" + ))); + } + + // Everuthing is good, let's complete the registration + let registration = repo + .user_registration() + .complete(&clock, registration) + .await?; + + // Now we can start the user creation + let user = repo + .user() + .add(&mut rng, &clock, registration.username) + .await?; + // Also create a browser session which will log the user in + let user_session = repo + .browser_session() + .add(&mut rng, &clock, &user, user_agent) + .await?; + + repo.user_email() + .add(&mut rng, &clock, &user, email_authentication.email) + .await?; + + if let Some(password) = registration.password { + let user_password = repo + .user_password() + .add( + &mut rng, + &clock, + &user, + password.version, + password.hashed_password, + None, + ) + .await?; + + repo.browser_session() + .authenticate_with_password(&mut rng, &clock, &user_session, &user_password) + .await?; + } + + if let Some(terms_url) = registration.terms_url { + repo.user_terms() + .accept_terms(&mut rng, &clock, &user, terms_url) + .await?; + } + + let mut job = ProvisionUserJob::new(&user); + if let Some(display_name) = registration.display_name { + job = job.set_display_name(display_name); + } + repo.queue_job().schedule_job(&mut rng, &clock, job).await?; + + repo.save().await?; + + activity_tracker + .record_browser_session(&clock, &user_session) + .await; + + let post_auth_action: Option = registration + .post_auth_action + .map(serde_json::from_value) + .transpose()?; + + // Login the user with the session we just created + let cookie_jar = cookie_jar.set_session(&user_session); + + return Ok(( + cookie_jar, + OptionalPostAuthAction::from(post_auth_action).go_next(&url_builder), + )); +} diff --git a/crates/handlers/src/views/register/steps/mod.rs b/crates/handlers/src/views/register/steps/mod.rs index 8d658ed85..4d479c352 100644 --- a/crates/handlers/src/views/register/steps/mod.rs +++ b/crates/handlers/src/views/register/steps/mod.rs @@ -3,4 +3,5 @@ // SPDX-License-Identifier: AGPL-3.0-only // Please see LICENSE in the repository root for full details. +pub(crate) mod finish; pub(crate) mod verify_email; diff --git a/crates/handlers/src/views/register/steps/verify_email.rs b/crates/handlers/src/views/register/steps/verify_email.rs index b5a0e4664..4ae18d777 100644 --- a/crates/handlers/src/views/register/steps/verify_email.rs +++ b/crates/handlers/src/views/register/steps/verify_email.rs @@ -8,19 +8,13 @@ use axum::{ extract::{Form, Path, State}, response::{Html, IntoResponse, Response}, }; -use axum_extra::TypedHeader; use mas_axum_utils::{ cookies::CookieJar, csrf::{CsrfExt, ProtectedForm}, - FancyError, SessionInfoExt, + FancyError, }; -use mas_data_model::UserAgent; use mas_router::{PostAuthAction, UrlBuilder}; -use mas_storage::{ - queue::{ProvisionUserJob, QueueJobRepositoryExt as _}, - user::UserEmailRepository, - BoxClock, BoxRepository, BoxRng, RepositoryAccess, -}; +use mas_storage::{user::UserEmailRepository, BoxClock, BoxRepository, BoxRng, RepositoryAccess}; use mas_templates::{ FieldError, RegisterStepsVerifyEmailContext, RegisterStepsVerifyEmailFormField, TemplateContext, Templates, ToFormState, @@ -28,7 +22,7 @@ use mas_templates::{ use serde::{Deserialize, Serialize}; use ulid::Ulid; -use crate::{views::shared::OptionalPostAuthAction, BoundActivityTracker, PreferredLanguage}; +use crate::{views::shared::OptionalPostAuthAction, PreferredLanguage}; #[derive(Serialize, Deserialize, Debug)] pub struct CodeForm { @@ -72,8 +66,12 @@ pub(crate) async fn get( .map(serde_json::from_value) .transpose()?; - return Ok(OptionalPostAuthAction::from(post_auth_action) - .go_next(&url_builder) + return Ok(( + cookie_jar, + OptionalPostAuthAction::from(post_auth_action) + .go_next(&url_builder) + .into_response(), + ) .into_response()); } @@ -115,13 +113,10 @@ pub(crate) async fn post( State(templates): State, mut repo: BoxRepository, cookie_jar: CookieJar, - user_agent: Option>, State(url_builder): State, - activity_tracker: BoundActivityTracker, Path(id): Path, Form(form): Form>, ) -> Result { - let user_agent = user_agent.map(|ua| UserAgent::parse(ua.as_str().to_owned())); let form = cookie_jar.verify_form(&clock, form)?; let registration = repo @@ -139,8 +134,10 @@ pub(crate) async fn post( .map(serde_json::from_value) .transpose()?; - return Ok(OptionalPostAuthAction::from(post_auth_action) - .go_next(&url_builder) + return Ok(( + cookie_jar, + OptionalPostAuthAction::from(post_auth_action).go_next(&url_builder), + ) .into_response()); } @@ -180,74 +177,12 @@ pub(crate) async fn post( return Ok((cookie_jar, Html(content)).into_response()); }; - let email_authentication = repo - .user_email() - .complete_authentication(&clock, email_authentication, &code) - .await?; - - let registration = repo - .user_registration() - .complete(&clock, registration) - .await?; - - // XXX: this should move somewhere else, and it doesn't check for uniqueness - let user = repo - .user() - .add(&mut rng, &clock, registration.username) - .await?; - let user_session = repo - .browser_session() - .add(&mut rng, &clock, &user, user_agent) - .await?; - repo.user_email() - .add(&mut rng, &clock, &user, email_authentication.email) - .await?; - - if let Some(password) = registration.password { - let user_password = repo - .user_password() - .add( - &mut rng, - &clock, - &user, - password.version, - password.hashed_password, - None, - ) - .await?; - - repo.browser_session() - .authenticate_with_password(&mut rng, &clock, &user_session, &user_password) - .await?; - } - - if let Some(terms_url) = registration.terms_url { - repo.user_terms() - .accept_terms(&mut rng, &clock, &user, terms_url) - .await?; - } - - repo.queue_job() - .schedule_job(&mut rng, &clock, ProvisionUserJob::new(&user)) + .complete_authentication(&clock, email_authentication, &code) .await?; repo.save().await?; - activity_tracker - .record_browser_session(&clock, &user_session) - .await; - - let post_auth_action: Option = registration - .post_auth_action - .map(serde_json::from_value) - .transpose()?; - - let cookie_jar = cookie_jar.set_session(&user_session); - - return Ok(( - cookie_jar, - OptionalPostAuthAction::from(post_auth_action).go_next(&url_builder), - ) - .into_response()); + let destination = mas_router::RegisterFinish::new(registration.id); + return Ok((cookie_jar, url_builder.redirect(&destination)).into_response()); } diff --git a/crates/router/src/endpoints.rs b/crates/router/src/endpoints.rs index b46de76f5..54b1a5cd1 100644 --- a/crates/router/src/endpoints.rs +++ b/crates/router/src/endpoints.rs @@ -444,7 +444,7 @@ impl From> for PasswordRegister { } } -/// `GET|POST /register/steps/verify-email/:id` +/// `GET|POST /register/steps/:id/verify-email` #[derive(Debug, Clone)] pub struct RegisterVerifyEmail { id: Ulid, @@ -460,11 +460,35 @@ impl RegisterVerifyEmail { impl Route for RegisterVerifyEmail { type Query = (); fn route() -> &'static str { - "/register/steps/verify-email/:id" + "/register/steps/:id/verify-email" } fn path(&self) -> std::borrow::Cow<'static, str> { - format!("/register/steps/verify-email/{}", self.id).into() + format!("/register/steps/{}/verify-email", self.id).into() + } +} + +/// `GET /register/steps/:id/finish` +#[derive(Debug, Clone)] +pub struct RegisterFinish { + id: Ulid, +} + +impl RegisterFinish { + #[must_use] + pub const fn new(id: Ulid) -> Self { + Self { id } + } +} + +impl Route for RegisterFinish { + type Query = (); + fn route() -> &'static str { + "/register/steps/:id/finish" + } + + fn path(&self) -> std::borrow::Cow<'static, str> { + format!("/register/steps/{}/finish", self.id).into() } }