Show and log the policy violations better

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<Arc<PolicyFactory>>,
     State(templates): State<Templates>,
@@ -123,15 +125,15 @@ pub(crate) async fn get(
         .ok_or(RouteError::NoSuchClient)?;
 
     match complete(
-        rng,
+        &mut rng,
-        clock,
+        &clock,
         repo,
         key_store,
         &policy_factory,
         url_builder,
         grant,
-        client,
+        &client,
-        session,
+        &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,
+    rng: &mut (impl rand::RngCore + rand::CryptoRng + Send),
-    clock: BoxClock,
+    clock: &impl Clock,
     mut repo: BoxRepository,
     key_store: Keystore,
     policy_factory: &PolicyFactory,
     url_builder: UrlBuilder,
     grant: AuthorizationGrant,
-    client: Client,
+    client: &Client,
-    browser_session: BrowserSession,
+    browser_session: &BrowserSession,
 ) -> Result<AuthorizationResponse, GrantCompletionError> {
     // 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(
+        .add(rng, clock, client, browser_session, grant.scope.clone())
-            &mut 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,
+            rng,
-            &clock,
+            clock,
             &url_builder,
             &key_store,
-            &client,
+            client,
             &grant,
-            &browser_session,
+            browser_session,
             None,
             Some(&valid_authentication),
         )?);

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<Response, RouteError> = ({
@@ -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,
+                        &mut rng,
-                        clock,
+                        &clock,
                         repo,
                         key_store,
                         &policy_factory,
                         url_builder,
                         grant,
-                        client,
+                        &client,
-                        user_session,
+                        &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,
+                        &mut rng,
-                        clock,
+                        &clock,
                         repo,
                         key_store,
                         &policy_factory,
                         url_builder,
                         grant,
-                        client,
+                        &client,
-                        user_session,
+                        &user_session,
                     )
                     .await
                     {
                         Ok(params) => callback_destination.go(&templates, params).await?,
-                        Err(
+                        Err(GrantCompletionError::RequiresConsent) => {
-                            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?
                             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()

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<Self>) -> 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<Self>) -> 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()
     }
 }