From e0dacf0761669fc2e22da2315829f70cfed34dab Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 11 Apr 2025 14:08:28 +0200 Subject: [PATCH] Remove the `complete` handler, make it go through the consent page --- crates/handlers/src/lib.rs | 7 +- .../src/oauth2/authorization/callback.rs | 2 +- .../src/oauth2/authorization/complete.rs | 271 ------------------ .../src/oauth2/{ => authorization}/consent.rs | 116 ++++++-- .../handlers/src/oauth2/authorization/mod.rs | 110 +++---- crates/handlers/src/oauth2/mod.rs | 1 - crates/router/src/endpoints.rs | 19 +- 7 files changed, 142 insertions(+), 384 deletions(-) delete mode 100644 crates/handlers/src/oauth2/authorization/complete.rs rename crates/handlers/src/oauth2/{ => authorization}/consent.rs (69%) diff --git a/crates/handlers/src/lib.rs b/crates/handlers/src/lib.rs index cbf12ad50..4d610482f 100644 --- a/crates/handlers/src/lib.rs +++ b/crates/handlers/src/lib.rs @@ -405,13 +405,10 @@ where mas_router::OAuth2AuthorizationEndpoint::route(), get(self::oauth2::authorization::get), ) - .route( - mas_router::ContinueAuthorizationGrant::route(), - get(self::oauth2::authorization::complete::get), - ) .route( mas_router::Consent::route(), - get(self::oauth2::consent::get).post(self::oauth2::consent::post), + get(self::oauth2::authorization::consent::get) + .post(self::oauth2::authorization::consent::post), ) .route( mas_router::CompatLoginSsoComplete::route(), diff --git a/crates/handlers/src/oauth2/authorization/callback.rs b/crates/handlers/src/oauth2/authorization/callback.rs index b76722d5a..beb0868d7 100644 --- a/crates/handlers/src/oauth2/authorization/callback.rs +++ b/crates/handlers/src/oauth2/authorization/callback.rs @@ -101,7 +101,7 @@ impl CallbackDestination { }) } - pub async fn go( + pub fn go( self, templates: &Templates, locale: &DataLocale, diff --git a/crates/handlers/src/oauth2/authorization/complete.rs b/crates/handlers/src/oauth2/authorization/complete.rs deleted file mode 100644 index ea534df2c..000000000 --- a/crates/handlers/src/oauth2/authorization/complete.rs +++ /dev/null @@ -1,271 +0,0 @@ -// Copyright 2024 New Vector Ltd. -// Copyright 2022-2024 The Matrix.org Foundation C.I.C. -// -// SPDX-License-Identifier: AGPL-3.0-only -// Please see LICENSE in the repository root for full details. - -use axum::{ - extract::{Path, State}, - response::{Html, IntoResponse, Response}, -}; -use axum_extra::TypedHeader; -use hyper::StatusCode; -use mas_axum_utils::{SessionInfoExt, cookies::CookieJar, csrf::CsrfExt, sentry::SentryEventID}; -use mas_data_model::{AuthorizationGrant, BrowserSession, Client}; -use mas_keystore::Keystore; -use mas_policy::{EvaluationResult, Policy}; -use mas_router::{PostAuthAction, UrlBuilder}; -use mas_storage::{ - BoxClock, BoxRepository, BoxRng, Clock, RepositoryAccess, - oauth2::{OAuth2AuthorizationGrantRepository, OAuth2ClientRepository, OAuth2SessionRepository}, - user::BrowserSessionRepository, -}; -use mas_templates::{PolicyViolationContext, TemplateContext, Templates}; -use oauth2_types::requests::AuthorizationResponse; -use thiserror::Error; -use tracing::warn; -use ulid::Ulid; - -use super::callback::CallbackDestination; -use crate::{ - BoundActivityTracker, PreferredLanguage, impl_from_error_for_route, oauth2::generate_id_token, -}; - -#[derive(Debug, Error)] -pub enum RouteError { - #[error(transparent)] - Internal(Box), - - #[error("authorization grant was not found")] - NotFound, - - #[error("authorization grant is not in a pending state")] - NotPending, - - #[error("failed to load client")] - NoSuchClient, -} - -impl IntoResponse for RouteError { - fn into_response(self) -> axum::response::Response { - let event = sentry::capture_error(&self); - // TODO: better error pages - let response = match self { - RouteError::NotFound => { - (StatusCode::NOT_FOUND, "authorization grant was not found").into_response() - } - RouteError::NotPending => ( - StatusCode::BAD_REQUEST, - "authorization grant not in a pending state", - ) - .into_response(), - RouteError::Internal(_) | Self::NoSuchClient => { - (StatusCode::INTERNAL_SERVER_ERROR, self.to_string()).into_response() - } - }; - - (SentryEventID::from(event), response).into_response() - } -} - -impl_from_error_for_route!(mas_storage::RepositoryError); -impl_from_error_for_route!(mas_templates::TemplateError); -impl_from_error_for_route!(mas_policy::LoadError); -impl_from_error_for_route!(mas_policy::EvaluationError); -impl_from_error_for_route!(super::callback::IntoCallbackDestinationError); -impl_from_error_for_route!(super::callback::CallbackDestinationError); - -#[tracing::instrument( - name = "handlers.oauth2.authorization_complete.get", - fields(grant.id = %grant_id), - skip_all, - err, -)] -pub(crate) async fn get( - mut rng: BoxRng, - clock: BoxClock, - PreferredLanguage(locale): PreferredLanguage, - State(templates): State, - State(url_builder): State, - State(key_store): State, - policy: Policy, - activity_tracker: BoundActivityTracker, - user_agent: Option>, - mut repo: BoxRepository, - cookie_jar: CookieJar, - Path(grant_id): Path, -) -> Result { - let (session_info, cookie_jar) = cookie_jar.session_info(); - - let maybe_session = session_info.load_active_session(&mut repo).await?; - - let user_agent = user_agent.map(|TypedHeader(ua)| ua.to_string()); - - let grant = repo - .oauth2_authorization_grant() - .lookup(grant_id) - .await? - .ok_or(RouteError::NotFound)?; - - let callback_destination = CallbackDestination::try_from(&grant)?; - let continue_grant = PostAuthAction::continue_grant(grant.id); - - let Some(session) = maybe_session else { - // If there is no session, redirect to the login screen, redirecting here after - // logout - return Ok(( - cookie_jar, - url_builder.redirect(&mas_router::Login::and_then(continue_grant)), - ) - .into_response()); - }; - - activity_tracker - .record_browser_session(&clock, &session) - .await; - - let client = repo - .oauth2_client() - .lookup(grant.client_id) - .await? - .ok_or(RouteError::NoSuchClient)?; - - match complete( - &mut rng, - &clock, - &activity_tracker, - user_agent, - repo, - key_store, - policy, - &url_builder, - grant, - &client, - &session, - ) - .await - { - Ok(params) => { - let res = callback_destination.go(&templates, &locale, params).await?; - Ok((cookie_jar, res).into_response()) - } - Err(GrantCompletionError::PolicyViolation(grant, res)) => { - warn!(violation = ?res, "Authorization grant for client {} denied by policy", client.id); - - let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng); - let ctx = PolicyViolationContext::for_authorization_grant(grant, client) - .with_session(session) - .with_csrf(csrf_token.form_value()) - .with_language(locale); - - let content = templates.render_policy_violation(&ctx)?; - - Ok((cookie_jar, Html(content)).into_response()) - } - Err(GrantCompletionError::NotPending) => Err(RouteError::NotPending), - Err(GrantCompletionError::Internal(e)) => Err(RouteError::Internal(e)), - } -} - -#[derive(Debug, Error)] -pub enum GrantCompletionError { - #[error(transparent)] - Internal(Box), - - #[error("authorization grant is not in a pending state")] - NotPending, - - #[error("denied by the policy")] - PolicyViolation(AuthorizationGrant, EvaluationResult), -} - -impl_from_error_for_route!(GrantCompletionError: mas_storage::RepositoryError); -impl_from_error_for_route!(GrantCompletionError: super::callback::IntoCallbackDestinationError); -impl_from_error_for_route!(GrantCompletionError: mas_policy::LoadError); -impl_from_error_for_route!(GrantCompletionError: mas_policy::EvaluationError); -impl_from_error_for_route!(GrantCompletionError: super::super::IdTokenSignatureError); - -pub(crate) async fn complete( - rng: &mut (impl rand::RngCore + rand::CryptoRng + Send), - clock: &impl Clock, - activity_tracker: &BoundActivityTracker, - user_agent: Option, - mut repo: BoxRepository, - key_store: Keystore, - mut policy: Policy, - url_builder: &UrlBuilder, - grant: AuthorizationGrant, - client: &Client, - browser_session: &BrowserSession, -) -> Result { - // Verify that the grant is in a pending stage - if !grant.stage.is_pending() { - return Err(GrantCompletionError::NotPending); - } - - // Run through the policy - let res = policy - .evaluate_authorization_grant(mas_policy::AuthorizationGrantInput { - user: Some(&browser_session.user), - client, - scope: &grant.scope, - grant_type: mas_policy::GrantType::AuthorizationCode, - requester: mas_policy::Requester { - ip_address: activity_tracker.ip(), - user_agent, - }, - }) - .await?; - - if !res.valid() { - return Err(GrantCompletionError::PolicyViolation(grant, res)); - } - - // All good, let's start the session - let session = repo - .oauth2_session() - .add_from_browser_session(rng, clock, client, browser_session, grant.scope.clone()) - .await?; - - let grant = repo - .oauth2_authorization_grant() - .fulfill(clock, &session, grant) - .await?; - - // Yep! Let's complete the auth now - let mut params = AuthorizationResponse::default(); - - // Did they request an ID token? - if grant.response_type_id_token { - // Fetch the last authentication - let last_authentication = repo - .browser_session() - .get_last_authentication(browser_session) - .await?; - - params.id_token = Some(generate_id_token( - rng, - clock, - url_builder, - &key_store, - client, - Some(&grant), - browser_session, - None, - last_authentication.as_ref(), - )?); - } - - // Did they request an auth code? - if let Some(code) = grant.code { - params.code = Some(code.code); - } - - repo.save().await?; - - activity_tracker - .record_oauth2_session(clock, &session) - .await; - - Ok(params) -} diff --git a/crates/handlers/src/oauth2/consent.rs b/crates/handlers/src/oauth2/authorization/consent.rs similarity index 69% rename from crates/handlers/src/oauth2/consent.rs rename to crates/handlers/src/oauth2/authorization/consent.rs index 6912fd213..273542383 100644 --- a/crates/handlers/src/oauth2/consent.rs +++ b/crates/handlers/src/oauth2/authorization/consent.rs @@ -16,6 +16,7 @@ use mas_axum_utils::{ sentry::SentryEventID, }; use mas_data_model::AuthorizationGrantStage; +use mas_keystore::Keystore; use mas_policy::Policy; use mas_router::{PostAuthAction, UrlBuilder}; use mas_storage::{ @@ -23,11 +24,14 @@ use mas_storage::{ oauth2::{OAuth2AuthorizationGrantRepository, OAuth2ClientRepository}, }; use mas_templates::{ConsentContext, PolicyViolationContext, TemplateContext, Templates}; +use oauth2_types::requests::AuthorizationResponse; use thiserror::Error; use ulid::Ulid; +use super::callback::CallbackDestination; use crate::{ BoundActivityTracker, PreferredLanguage, impl_from_error_for_route, + oauth2::generate_id_token, session::{SessionOrFallback, load_session_or_fallback}, }; @@ -45,9 +49,6 @@ pub enum RouteError { #[error("Authorization grant already used")] GrantNotPending, - #[error("Policy violation")] - PolicyViolation, - #[error("Failed to load client")] NoSuchClient, } @@ -57,20 +58,24 @@ impl_from_error_for_route!(mas_storage::RepositoryError); impl_from_error_for_route!(mas_policy::LoadError); impl_from_error_for_route!(mas_policy::EvaluationError); impl_from_error_for_route!(crate::session::SessionLoadError); +impl_from_error_for_route!(crate::oauth2::IdTokenSignatureError); +impl_from_error_for_route!(super::callback::IntoCallbackDestinationError); +impl_from_error_for_route!(super::callback::CallbackDestinationError); impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { let event_id = sentry::capture_error(&self); ( - SentryEventID::from(event_id), StatusCode::INTERNAL_SERVER_ERROR, + SentryEventID::from(event_id), + self.to_string(), ) .into_response() } } #[tracing::instrument( - name = "handlers.oauth2.consent.get", + name = "handlers.oauth2.authorization.consent.get", fields(grant.id = %grant_id), skip_all, err, @@ -142,17 +147,7 @@ pub(crate) async fn get( }, }) .await?; - - if res.valid() { - let ctx = ConsentContext::new(grant, client) - .with_session(session) - .with_csrf(csrf_token.form_value()) - .with_language(locale); - - let content = templates.render_consent(&ctx)?; - - Ok((cookie_jar, Html(content)).into_response()) - } else { + if !res.valid() { let ctx = PolicyViolationContext::for_authorization_grant(grant, client) .with_session(session) .with_csrf(csrf_token.form_value()) @@ -160,12 +155,21 @@ pub(crate) async fn get( let content = templates.render_policy_violation(&ctx)?; - Ok((cookie_jar, Html(content)).into_response()) + return Ok((cookie_jar, Html(content)).into_response()); } + + let ctx = ConsentContext::new(grant, client) + .with_session(session) + .with_csrf(csrf_token.form_value()) + .with_language(locale); + + let content = templates.render_consent(&ctx)?; + + Ok((cookie_jar, Html(content)).into_response()) } #[tracing::instrument( - name = "handlers.oauth2.consent.post", + name = "handlers.oauth2.authorization.consent.post", fields(grant.id = %grant_id), skip_all, err, @@ -175,6 +179,7 @@ pub(crate) async fn post( clock: BoxClock, PreferredLanguage(locale): PreferredLanguage, State(templates): State, + State(key_store): State, mut policy: Policy, mut repo: BoxRepository, activity_tracker: BoundActivityTracker, @@ -199,6 +204,8 @@ pub(crate) async fn post( SessionOrFallback::Fallback { response } => return Ok(response), }; + let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng); + let user_agent = user_agent.map(|ua| ua.to_string()); let grant = repo @@ -206,15 +213,16 @@ pub(crate) async fn post( .lookup(grant_id) .await? .ok_or(RouteError::GrantNotFound)?; - let next = PostAuthAction::continue_grant(grant_id); + let callback_destination = CallbackDestination::try_from(&grant)?; - let Some(session) = maybe_session else { + let Some(browser_session) = maybe_session else { + let next = PostAuthAction::continue_grant(grant_id); let login = mas_router::Login::and_then(next); return Ok((cookie_jar, url_builder.redirect(&login)).into_response()); }; activity_tracker - .record_browser_session(&clock, &session) + .record_browser_session(&clock, &browser_session) .await; let client = repo @@ -225,7 +233,7 @@ pub(crate) async fn post( let res = policy .evaluate_authorization_grant(mas_policy::AuthorizationGrantInput { - user: Some(&session.user), + user: Some(&browser_session.user), client: &client, scope: &grant.scope, grant_type: mas_policy::GrantType::AuthorizationCode, @@ -237,10 +245,70 @@ pub(crate) async fn post( .await?; if !res.valid() { - return Err(RouteError::PolicyViolation); + let ctx = PolicyViolationContext::for_authorization_grant(grant, client) + .with_session(browser_session) + .with_csrf(csrf_token.form_value()) + .with_language(locale); + + let content = templates.render_policy_violation(&ctx)?; + + return Ok((cookie_jar, Html(content)).into_response()); + } + + // All good, let's start the session + let session = repo + .oauth2_session() + .add_from_browser_session( + &mut rng, + &clock, + &client, + &browser_session, + grant.scope.clone(), + ) + .await?; + + let grant = repo + .oauth2_authorization_grant() + .fulfill(&clock, &session, grant) + .await?; + + let mut params = AuthorizationResponse::default(); + + // Did they request an ID token? + if grant.response_type_id_token { + // Fetch the last authentication + let last_authentication = repo + .browser_session() + .get_last_authentication(&browser_session) + .await?; + + params.id_token = Some(generate_id_token( + &mut rng, + &clock, + &url_builder, + &key_store, + &client, + Some(&grant), + &browser_session, + None, + last_authentication.as_ref(), + )?); + } + + // Did they request an auth code? + if let Some(code) = grant.code { + params.code = Some(code.code); } repo.save().await?; - Ok((cookie_jar, next.go_next(&url_builder)).into_response()) + activity_tracker + .record_oauth2_session(&clock, &session) + .await; + + Ok(( + cookie_jar, + callback_destination.go(&templates, &locale, params)?, + ) + .into_response()) } diff --git a/crates/handlers/src/oauth2/authorization/mod.rs b/crates/handlers/src/oauth2/authorization/mod.rs index c953f965b..7e131f812 100644 --- a/crates/handlers/src/oauth2/authorization/mod.rs +++ b/crates/handlers/src/oauth2/authorization/mod.rs @@ -31,7 +31,7 @@ use self::callback::CallbackDestination; use crate::{BoundActivityTracker, PreferredLanguage, impl_from_error_for_route}; mod callback; -pub mod complete; +pub(crate) mod consent; #[derive(Debug, Error)] pub enum RouteError { @@ -172,80 +172,66 @@ pub(crate) async fn get( // Check if the request/request_uri/registration params are used. If so, reply // with the right error since we don't support them. if params.auth.request.is_some() { - return Ok(callback_destination - .go( - &templates, - &locale, - ClientError::from(ClientErrorCode::RequestNotSupported), - ) - .await?); + return Ok(callback_destination.go( + &templates, + &locale, + ClientError::from(ClientErrorCode::RequestNotSupported), + )?); } if params.auth.request_uri.is_some() { - return Ok(callback_destination - .go( - &templates, - &locale, - ClientError::from(ClientErrorCode::RequestUriNotSupported), - ) - .await?); + return Ok(callback_destination.go( + &templates, + &locale, + ClientError::from(ClientErrorCode::RequestUriNotSupported), + )?); } // Check if the client asked for a `token` response type, and bail out if it's // the case, since we don't support them if response_type.has_token() { - return Ok(callback_destination - .go( - &templates, - &locale, - ClientError::from(ClientErrorCode::UnsupportedResponseType), - ) - .await?); + return Ok(callback_destination.go( + &templates, + &locale, + ClientError::from(ClientErrorCode::UnsupportedResponseType), + )?); } // If the client asked for a `id_token` response type, we must check if it can // use the `implicit` grant type if response_type.has_id_token() && !client.grant_types.contains(&GrantType::Implicit) { - return Ok(callback_destination - .go( - &templates, - &locale, - ClientError::from(ClientErrorCode::UnauthorizedClient), - ) - .await?); + return Ok(callback_destination.go( + &templates, + &locale, + ClientError::from(ClientErrorCode::UnauthorizedClient), + )?); } if params.auth.registration.is_some() { - return Ok(callback_destination - .go( - &templates, - &locale, - ClientError::from(ClientErrorCode::RegistrationNotSupported), - ) - .await?); + return Ok(callback_destination.go( + &templates, + &locale, + ClientError::from(ClientErrorCode::RegistrationNotSupported), + )?); } // Fail early if prompt=none; we never let it go through if prompt.contains(&Prompt::None) { - return Ok(callback_destination - .go( - &templates, - &locale, - ClientError::from(ClientErrorCode::LoginRequired), - ) - .await?); + return Ok(callback_destination.go( + &templates, + &locale, + ClientError::from(ClientErrorCode::LoginRequired), + )?); } let code: Option = if response_type.has_code() { // Check if it is allowed to use this grant type if !client.grant_types.contains(&GrantType::AuthorizationCode) { - return Ok(callback_destination - .go( - &templates, - &locale, - ClientError::from(ClientErrorCode::UnauthorizedClient), - ) - .await?); + return Ok(callback_destination.go( + &templates, + &locale, + ClientError::from(ClientErrorCode::UnauthorizedClient), + )?); } // 32 random alphanumeric characters, about 190bit of entropy @@ -265,13 +251,11 @@ pub(crate) async fn get( // If the request had PKCE params but no code asked, it should get back with an // error if params.pkce.is_some() { - return Ok(callback_destination - .go( - &templates, - &locale, - ClientError::from(ClientErrorCode::InvalidRequest), - ) - .await?); + return Ok(callback_destination.go( + &templates, + &locale, + ClientError::from(ClientErrorCode::InvalidRequest), + )?); } None @@ -336,13 +320,11 @@ pub(crate) async fn get( Ok(r) => r, Err(err) => { tracing::error!(%err); - callback_destination - .go( - &templates, - &locale, - ClientError::from(ClientErrorCode::ServerError), - ) - .await? + callback_destination.go( + &templates, + &locale, + ClientError::from(ClientErrorCode::ServerError), + )? } }; diff --git a/crates/handlers/src/oauth2/mod.rs b/crates/handlers/src/oauth2/mod.rs index c80d7212b..f15a1ae9d 100644 --- a/crates/handlers/src/oauth2/mod.rs +++ b/crates/handlers/src/oauth2/mod.rs @@ -23,7 +23,6 @@ use mas_storage::{Clock, RepositoryAccess}; use thiserror::Error; pub mod authorization; -pub mod consent; pub mod device; pub mod discovery; pub mod introspection; diff --git a/crates/router/src/endpoints.rs b/crates/router/src/endpoints.rs index ceead6d12..89a9d726e 100644 --- a/crates/router/src/endpoints.rs +++ b/crates/router/src/endpoints.rs @@ -60,9 +60,7 @@ impl PostAuthAction { pub fn go_next(&self, url_builder: &UrlBuilder) -> axum::response::Redirect { match self { - Self::ContinueAuthorizationGrant { id } => { - url_builder.redirect(&ContinueAuthorizationGrant(*id)) - } + Self::ContinueAuthorizationGrant { id } => url_builder.redirect(&Consent(*id)), Self::ContinueDeviceCodeGrant { id } => { url_builder.redirect(&DeviceCodeConsent::new(*id)) } @@ -521,21 +519,6 @@ impl SimpleRoute for AccountPasswordChange { const PATH: &'static str = "/account/password/change"; } -/// `GET /authorize/{grant_id}` -#[derive(Debug, Clone)] -pub struct ContinueAuthorizationGrant(pub Ulid); - -impl Route for ContinueAuthorizationGrant { - type Query = (); - fn route() -> &'static str { - "/authorize/{grant_id}" - } - - fn path(&self) -> std::borrow::Cow<'static, str> { - format!("/authorize/{}", self.0).into() - } -} - /// `GET /consent/{grant_id}` #[derive(Debug, Clone)] pub struct Consent(pub Ulid);