From 48c4c34e8808c453f6b78720aa5660309abbdb7f Mon Sep 17 00:00:00 2001 From: Olivier 'reivilibre Date: Thu, 25 Jul 2024 23:03:28 +0100 Subject: [PATCH] Remove the server-side rendered account recovery 'finish' form Replace with the React frontend form --- crates/handlers/src/lib.rs | 8 +- crates/handlers/src/views/app.rs | 15 ++ crates/handlers/src/views/recovery/finish.rs | 269 ------------------- crates/handlers/src/views/recovery/mod.rs | 1 - crates/router/src/endpoints.rs | 5 +- 5 files changed, 22 insertions(+), 276 deletions(-) delete mode 100644 crates/handlers/src/views/recovery/finish.rs diff --git a/crates/handlers/src/lib.rs b/crates/handlers/src/lib.rs index 74c68150..cafbae81 100644 --- a/crates/handlers/src/lib.rs +++ b/crates/handlers/src/lib.rs @@ -334,6 +334,10 @@ where mas_router::AccountWildcard::route(), get(self::views::app::get), ) + .route( + mas_router::AccountRecoveryFinish::route(), + get(self::views::app::get_anonymous), + ) .route( mas_router::ChangePasswordDiscovery::route(), get(|State(url_builder): State| async move { @@ -372,10 +376,6 @@ where mas_router::AccountRecoveryProgress::route(), get(self::views::recovery::progress::get).post(self::views::recovery::progress::post), ) - .route( - mas_router::AccountRecoveryFinish::route(), - get(self::views::recovery::finish::get).post(self::views::recovery::finish::post), - ) .route( mas_router::OAuth2AuthorizationEndpoint::route(), get(self::oauth2::authorization::get), diff --git a/crates/handlers/src/views/app.rs b/crates/handlers/src/views/app.rs index e8c0ae07..951a74db 100644 --- a/crates/handlers/src/views/app.rs +++ b/crates/handlers/src/views/app.rs @@ -58,3 +58,18 @@ pub async fn get( Ok((cookie_jar, Html(content)).into_response()) } + +/// Like `get`, but allow anonymous access. +/// Used for a subset of the account management paths. +/// Needed for e.g. account recovery. +#[tracing::instrument(name = "handlers.views.app.get_anonymous", skip_all, err)] +pub async fn get_anonymous( + PreferredLanguage(locale): PreferredLanguage, + State(templates): State, + State(url_builder): State, +) -> Result { + let ctx = AppContext::from_url_builder(&url_builder).with_language(locale); + let content = templates.render_app(&ctx)?; + + Ok(Html(content).into_response()) +} diff --git a/crates/handlers/src/views/recovery/finish.rs b/crates/handlers/src/views/recovery/finish.rs deleted file mode 100644 index 9881f8ac..00000000 --- a/crates/handlers/src/views/recovery/finish.rs +++ /dev/null @@ -1,269 +0,0 @@ -// Copyright 2024 The Matrix.org Foundation C.I.C. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -use anyhow::Context; -use axum::{ - extract::{Query, State}, - response::{Html, IntoResponse, Response}, - Form, -}; -use mas_axum_utils::{ - cookies::CookieJar, - csrf::{CsrfExt, ProtectedForm}, - FancyError, -}; -use mas_data_model::SiteConfig; -use mas_router::UrlBuilder; -use mas_storage::{BoxClock, BoxRepository, BoxRng}; -use mas_templates::{ - EmptyContext, ErrorContext, FieldError, FormState, RecoveryExpiredContext, - RecoveryFinishContext, RecoveryFinishFormField, TemplateContext, Templates, -}; -use serde::{Deserialize, Serialize}; -use zeroize::Zeroizing; - -use crate::{passwords::PasswordManager, PreferredLanguage}; - -#[derive(Deserialize)] -pub(crate) struct RouteQuery { - ticket: String, -} - -#[derive(Deserialize, Serialize)] -pub(crate) struct RouteForm { - new_password: String, - new_password_confirm: String, -} - -pub(crate) async fn get( - mut rng: BoxRng, - clock: BoxClock, - mut repo: BoxRepository, - State(site_config): State, - State(templates): State, - PreferredLanguage(locale): PreferredLanguage, - cookie_jar: CookieJar, - Query(query): Query, -) -> Result { - if !site_config.account_recovery_allowed { - let context = EmptyContext.with_language(locale); - let rendered = templates.render_recovery_disabled(&context)?; - return Ok((cookie_jar, Html(rendered)).into_response()); - } - - let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng); - - let ticket = repo - .user_recovery() - .find_ticket(&query.ticket) - .await? - .context("Unknown ticket")?; - - let session = repo - .user_recovery() - .lookup_session(ticket.user_recovery_session_id) - .await? - .context("Unknown session")?; - - if session.consumed_at.is_some() { - let context = EmptyContext.with_language(locale); - let rendered = templates.render_recovery_consumed(&context)?; - return Ok((cookie_jar, Html(rendered)).into_response()); - } - - if !ticket.active(clock.now()) { - let context = RecoveryExpiredContext::new(session) - .with_csrf(csrf_token.form_value()) - .with_language(locale); - let rendered = templates.render_recovery_expired(&context)?; - return Ok((cookie_jar, Html(rendered)).into_response()); - } - - let user_email = repo - .user_email() - .lookup(ticket.user_email_id) - .await? - // Only allow confirmed email addresses - .filter(|email| email.confirmed_at.is_some()) - .context("Unknown email address")?; - - let user = repo - .user() - .lookup(user_email.user_id) - .await? - .context("Invalid user")?; - - if !user.is_valid() { - // TODO: render a 'account locked' page - let rendered = templates.render_error( - &ErrorContext::new() - .with_code("Account locked") - .with_language(&locale), - )?; - return Ok((cookie_jar, Html(rendered)).into_response()); - } - - let context = RecoveryFinishContext::new(user) - .with_csrf(csrf_token.form_value()) - .with_language(locale); - - let rendered = templates.render_recovery_finish(&context)?; - - Ok((cookie_jar, Html(rendered)).into_response()) -} - -#[allow(clippy::too_many_lines)] -pub(crate) async fn post( - mut rng: BoxRng, - clock: BoxClock, - mut repo: BoxRepository, - State(site_config): State, - State(password_manager): State, - State(templates): State, - State(url_builder): State, - PreferredLanguage(locale): PreferredLanguage, - cookie_jar: CookieJar, - Query(query): Query, - Form(form): Form>, -) -> Result { - if !site_config.account_recovery_allowed { - let context = EmptyContext.with_language(locale); - let rendered = templates.render_recovery_disabled(&context)?; - return Ok((cookie_jar, Html(rendered)).into_response()); - } - - let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng); - - let ticket = repo - .user_recovery() - .find_ticket(&query.ticket) - .await? - .context("Unknown ticket")?; - - let session = repo - .user_recovery() - .lookup_session(ticket.user_recovery_session_id) - .await? - .context("Unknown session")?; - - if session.consumed_at.is_some() { - let context = EmptyContext.with_language(locale); - let rendered = templates.render_recovery_consumed(&context)?; - return Ok((cookie_jar, Html(rendered)).into_response()); - } - - if !ticket.active(clock.now()) { - let context = RecoveryExpiredContext::new(session) - .with_csrf(csrf_token.form_value()) - .with_language(locale); - let rendered = templates.render_recovery_expired(&context)?; - return Ok((cookie_jar, Html(rendered)).into_response()); - } - - let user_email = repo - .user_email() - .lookup(ticket.user_email_id) - .await? - // Only allow confirmed email addresses - .filter(|email| email.confirmed_at.is_some()) - .context("Unknown email address")?; - - let user = repo - .user() - .lookup(user_email.user_id) - .await? - .context("Invalid user")?; - - if !user.is_valid() { - // TODO: render a 'account locked' page - let rendered = templates.render_error( - &ErrorContext::new() - .with_code("Account locked") - .with_language(&locale), - )?; - return Ok((cookie_jar, Html(rendered)).into_response()); - } - - let form = cookie_jar.verify_form(&clock, form)?; - - // Check the form - let mut form_state = FormState::from_form(&form); - - if form.new_password.is_empty() { - form_state = form_state - .with_error_on_field(RecoveryFinishFormField::NewPassword, FieldError::Required); - } - - if form.new_password_confirm.is_empty() { - form_state = form_state.with_error_on_field( - RecoveryFinishFormField::NewPasswordConfirm, - FieldError::Required, - ); - } - - if form.new_password != form.new_password_confirm { - form_state = form_state - .with_error_on_field( - RecoveryFinishFormField::NewPassword, - FieldError::Unspecified, - ) - .with_error_on_field( - RecoveryFinishFormField::NewPasswordConfirm, - FieldError::PasswordMismatch, - ); - } - - if !password_manager.is_password_complex_enough(&form.new_password)? { - // TODO This error should be localised, - // but actually this entire form should be pushed down into the - // React frontend so user gets real-time feedback. - form_state = form_state.with_error_on_field( - RecoveryFinishFormField::NewPassword, - FieldError::Policy { - message: "Password is too weak".to_owned(), - }, - ); - } - - if !form_state.is_valid() { - let context = RecoveryFinishContext::new(user) - .with_form_state(form_state) - .with_csrf(csrf_token.form_value()) - .with_language(locale); - - let rendered = templates.render_recovery_finish(&context)?; - - return Ok((cookie_jar, Html(rendered)).into_response()); - } - - // Form is valid, change the password - let password = Zeroizing::new(form.new_password.into_bytes()); - let (version, hashed_password) = password_manager.hash(&mut rng, password).await?; - repo.user_password() - .add(&mut rng, &clock, &user, version, hashed_password, None) - .await?; - - // Mark the session as consumed - repo.user_recovery() - .consume_ticket(&clock, ticket, session) - .await?; - - repo.save().await?; - - Ok(( - cookie_jar, - url_builder.redirect(&mas_router::Login::default()), - ) - .into_response()) -} diff --git a/crates/handlers/src/views/recovery/mod.rs b/crates/handlers/src/views/recovery/mod.rs index 567fbe74..9a10d9dd 100644 --- a/crates/handlers/src/views/recovery/mod.rs +++ b/crates/handlers/src/views/recovery/mod.rs @@ -12,6 +12,5 @@ // See the License for the specific language governing permissions and // limitations under the License. -pub mod finish; pub mod progress; pub mod start; diff --git a/crates/router/src/endpoints.rs b/crates/router/src/endpoints.rs index 450a92c5..29ef5f1c 100644 --- a/crates/router/src/endpoints.rs +++ b/crates/router/src/endpoints.rs @@ -808,7 +808,8 @@ impl Route for AccountRecoveryProgress { } } -/// `GET|POST /recover/complete?ticket=:ticket` +/// `GET /account/password/recovery?ticket=:ticket` +/// Rendered by the React frontend #[derive(Default, Serialize, Deserialize, Debug, Clone)] pub struct AccountRecoveryFinish { ticket: String, @@ -825,7 +826,7 @@ impl Route for AccountRecoveryFinish { type Query = AccountRecoveryFinish; fn route() -> &'static str { - "/recover/complete" + "/account/password/recovery" } fn query(&self) -> Option<&Self::Query> {