diff --git a/crates/handlers/src/oauth2/authorization/complete.rs b/crates/handlers/src/oauth2/authorization/complete.rs index b8a03159..6a67cd14 100644 --- a/crates/handlers/src/oauth2/authorization/complete.rs +++ b/crates/handlers/src/oauth2/authorization/complete.rs @@ -16,23 +16,24 @@ use std::sync::Arc; use axum::{ extract::{Path, State}, - response::{IntoResponse, Response}, + response::{Html, IntoResponse, Response}, }; use axum_extra::extract::PrivateCookieJar; use hyper::StatusCode; -use mas_axum_utils::SessionInfoExt; +use mas_axum_utils::{csrf::CsrfExt, SessionInfoExt}; use mas_data_model::{AuthorizationGrant, BrowserSession, Client, Device}; use mas_keystore::{Encrypter, Keystore}; -use mas_policy::PolicyFactory; +use mas_policy::{EvaluationResult, PolicyFactory}; use mas_router::{PostAuthAction, Route, UrlBuilder}; use mas_storage::{ oauth2::{OAuth2AuthorizationGrantRepository, OAuth2ClientRepository, OAuth2SessionRepository}, user::BrowserSessionRepository, - BoxClock, BoxRepository, BoxRng, RepositoryAccess, + BoxClock, BoxRepository, BoxRng, Clock, RepositoryAccess, }; -use mas_templates::Templates; +use mas_templates::{PolicyViolationContext, TemplateContext, Templates}; use oauth2_types::requests::AuthorizationResponse; use thiserror::Error; +use tracing::warn; use ulid::Ulid; use super::callback::CallbackDestination; @@ -74,6 +75,7 @@ impl IntoResponse for RouteError { } 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::InstanciateError); impl_from_error_for_route!(mas_policy::EvaluationError); @@ -87,7 +89,7 @@ impl_from_error_for_route!(super::callback::CallbackDestinationError); err, )] pub(crate) async fn get( - rng: BoxRng, + mut rng: BoxRng, clock: BoxClock, State(policy_factory): State>, State(templates): State, @@ -123,15 +125,15 @@ pub(crate) async fn get( .ok_or(RouteError::NoSuchClient)?; match complete( - rng, - clock, + &mut rng, + &clock, repo, key_store, &policy_factory, url_builder, grant, - client, - session, + &client, + &session, ) .await { @@ -144,10 +146,22 @@ pub(crate) async fn get( mas_router::Reauth::and_then(continue_grant).go(), ) .into_response()), - Err(GrantCompletionError::RequiresConsent | GrantCompletionError::PolicyViolation) => { + Err(GrantCompletionError::RequiresConsent) => { let next = mas_router::Consent(grant_id); Ok((cookie_jar, next.go()).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::new(grant, client) + .with_session(session) + .with_csrf(csrf_token.form_value()); + + let content = templates.render_policy_violation(&ctx).await?; + + Ok((cookie_jar, Html(content)).into_response()) + } Err(GrantCompletionError::NotPending) => Err(RouteError::NotPending), Err(GrantCompletionError::Internal(e)) => Err(RouteError::Internal(e)), } @@ -168,7 +182,7 @@ pub enum GrantCompletionError { RequiresConsent, #[error("denied by the policy")] - PolicyViolation, + PolicyViolation(AuthorizationGrant, EvaluationResult), } impl_from_error_for_route!(GrantCompletionError: mas_storage::RepositoryError); @@ -179,15 +193,15 @@ impl_from_error_for_route!(GrantCompletionError: mas_policy::EvaluationError); impl_from_error_for_route!(GrantCompletionError: super::super::IdTokenSignatureError); pub(crate) async fn complete( - mut rng: BoxRng, - clock: BoxClock, + rng: &mut (impl rand::RngCore + rand::CryptoRng + Send), + clock: &impl Clock, mut repo: BoxRepository, key_store: Keystore, policy_factory: &PolicyFactory, url_builder: UrlBuilder, grant: AuthorizationGrant, - client: Client, - browser_session: BrowserSession, + client: &Client, + browser_session: &BrowserSession, ) -> Result { // Verify that the grant is in a pending stage if !grant.stage.is_pending() { @@ -197,7 +211,7 @@ pub(crate) async fn complete( // Check if the authentication is fresh enough let authentication = repo .browser_session() - .get_last_authentication(&browser_session) + .get_last_authentication(browser_session) .await?; let authentication = authentication.filter(|auth| auth.created_at > grant.max_auth_time()); @@ -209,16 +223,16 @@ pub(crate) async fn complete( // Run through the policy let mut policy = policy_factory.instantiate().await?; let res = policy - .evaluate_authorization_grant(&grant, &client, &browser_session.user) + .evaluate_authorization_grant(&grant, client, &browser_session.user) .await?; if !res.valid() { - return Err(GrantCompletionError::PolicyViolation); + return Err(GrantCompletionError::PolicyViolation(grant, res)); } let current_consent = repo .oauth2_client() - .get_consent_for_user(&client, &browser_session.user) + .get_consent_for_user(client, &browser_session.user) .await?; let lacks_consent = grant @@ -236,18 +250,12 @@ pub(crate) async fn complete( // All good, let's start the session let session = repo .oauth2_session() - .add( - &mut rng, - &clock, - &client, - &browser_session, - grant.scope.clone(), - ) + .add(rng, clock, client, browser_session, grant.scope.clone()) .await?; let grant = repo .oauth2_authorization_grant() - .fulfill(&clock, &session, grant) + .fulfill(clock, &session, grant) .await?; // Yep! Let's complete the auth now @@ -256,13 +264,13 @@ pub(crate) async fn complete( // Did they request an ID token? if grant.response_type_id_token { params.id_token = Some(generate_id_token( - &mut rng, - &clock, + rng, + clock, &url_builder, &key_store, - &client, + client, &grant, - &browser_session, + browser_session, None, Some(&valid_authentication), )?); diff --git a/crates/handlers/src/oauth2/authorization/mod.rs b/crates/handlers/src/oauth2/authorization/mod.rs index 1230362d..1fd4b52c 100644 --- a/crates/handlers/src/oauth2/authorization/mod.rs +++ b/crates/handlers/src/oauth2/authorization/mod.rs @@ -16,11 +16,11 @@ use std::sync::Arc; use axum::{ extract::{Form, State}, - response::{IntoResponse, Response}, + response::{Html, IntoResponse, Response}, }; use axum_extra::extract::PrivateCookieJar; use hyper::StatusCode; -use mas_axum_utils::SessionInfoExt; +use mas_axum_utils::{csrf::CsrfExt, SessionInfoExt}; use mas_data_model::{AuthorizationCode, Pkce}; use mas_keystore::{Encrypter, Keystore}; use mas_policy::PolicyFactory; @@ -29,7 +29,7 @@ use mas_storage::{ oauth2::{OAuth2AuthorizationGrantRepository, OAuth2ClientRepository}, BoxClock, BoxRepository, BoxRng, }; -use mas_templates::Templates; +use mas_templates::{PolicyViolationContext, TemplateContext, Templates}; use oauth2_types::{ errors::{ClientError, ClientErrorCode}, pkce, @@ -39,6 +39,7 @@ use oauth2_types::{ use rand::{distributions::Alphanumeric, Rng}; use serde::Deserialize; use thiserror::Error; +use tracing::warn; use self::{callback::CallbackDestination, complete::GrantCompletionError}; use crate::impl_from_error_for_route; @@ -91,6 +92,7 @@ impl IntoResponse for RouteError { } impl_from_error_for_route!(mas_storage::RepositoryError); +impl_from_error_for_route!(mas_templates::TemplateError); impl_from_error_for_route!(self::callback::CallbackDestinationError); impl_from_error_for_route!(mas_policy::LoadError); impl_from_error_for_route!(mas_policy::InstanciateError); @@ -170,6 +172,7 @@ pub(crate) async fn get( // Get the session info from the cookie let (session_info, cookie_jar) = cookie_jar.session_info(); + let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng); // One day, we will have try blocks let res: Result = ({ @@ -340,15 +343,15 @@ pub(crate) async fn get( Some(user_session) if prompt.contains(&Prompt::None) => { // With prompt=none, we should get back to the client immediately match self::complete::complete( - rng, - clock, + &mut rng, + &clock, repo, key_store, &policy_factory, url_builder, grant, - client, - user_session, + &client, + &user_session, ) .await { @@ -369,7 +372,7 @@ pub(crate) async fn get( ) .await? } - Err(GrantCompletionError::PolicyViolation) => { + Err(GrantCompletionError::PolicyViolation(_grant, _res)) => { callback_destination .go(&templates, ClientError::from(ClientErrorCode::AccessDenied)) .await? @@ -387,29 +390,32 @@ pub(crate) async fn get( let grant_id = grant.id; // Else, we show the relevant reauth/consent page if necessary match self::complete::complete( - rng, - clock, + &mut rng, + &clock, repo, key_store, &policy_factory, url_builder, grant, - client, - user_session, + &client, + &user_session, ) .await { Ok(params) => callback_destination.go(&templates, params).await?, - Err( - GrantCompletionError::RequiresConsent - | GrantCompletionError::PolicyViolation, - ) => { - // We're redirecting to the consent URI in both 'consent required' and - // 'policy violation' cases, because we reevaluate the policy on this - // page, and show the error accordingly - // XXX: is this the right approach? + Err(GrantCompletionError::RequiresConsent) => { mas_router::Consent(grant_id).go().into_response() } + Err(GrantCompletionError::PolicyViolation(grant, res)) => { + warn!(violation = ?res, "Authorization grant for client {} denied by policy", client.id); + + let ctx = PolicyViolationContext::new(grant, client) + .with_session(user_session) + .with_csrf(csrf_token.form_value()); + + let content = templates.render_policy_violation(&ctx).await?; + Html(content).into_response() + } Err(GrantCompletionError::RequiresReauth) => { mas_router::Reauth::and_then(continue_grant) .go() diff --git a/crates/storage-pg/src/repository.rs b/crates/storage-pg/src/repository.rs index d5a42792..3e749d97 100644 --- a/crates/storage-pg/src/repository.rs +++ b/crates/storage-pg/src/repository.rs @@ -31,6 +31,7 @@ use mas_storage::{ Repository, RepositoryAccess, RepositoryTransaction, }; use sqlx::{PgPool, Postgres, Transaction}; +use tracing::Instrument; use crate::{ compat::{ @@ -78,11 +79,21 @@ impl RepositoryTransaction for PgRepository { type Error = DatabaseError; fn save(self: Box) -> BoxFuture<'static, Result<(), Self::Error>> { - self.txn.commit().map_err(DatabaseError::from).boxed() + let span = tracing::info_span!("db.save"); + self.txn + .commit() + .map_err(DatabaseError::from) + .instrument(span) + .boxed() } fn cancel(self: Box) -> BoxFuture<'static, Result<(), Self::Error>> { - self.txn.rollback().map_err(DatabaseError::from).boxed() + let span = tracing::info_span!("db.cancel"); + self.txn + .rollback() + .map_err(DatabaseError::from) + .instrument(span) + .boxed() } }