Always ask for consent, never for reauth

Now that we have deduplicated clients, we're in this weird situation
where authorization grants just… go through.

This is because 4 years ago, I designed it to support prompt=consent and
prompt=none, but that never ended up being used/mentioned in the MSCs.

We also had support for max_age, but that required reauthing, which
doesn't work well with upstream providers.

So this removes support for prompt=consent|none and max_age, and makes
sure we always go through the consent page.

Lots of code deleted, yay!

Author: sandhose

crates/data-model/src/oauth2/authorization_grant.rs

@@ -4,9 +4,7 @@
 // SPDX-License-Identifier: AGPL-3.0-only
 // Please see LICENSE in the repository root for full details.
 
-use std::num::NonZeroU32;
-
-use chrono::{DateTime, Duration, Utc};
+use chrono::{DateTime, Utc};
 use mas_iana::oauth::PkceCodeChallengeMethod;
 use oauth2_types::{
     pkce::{CodeChallengeError, CodeChallengeMethodExt},
@@ -158,11 +156,9 @@ pub struct AuthorizationGrant {
     pub scope: Scope,
     pub state: Option<String>,
     pub nonce: Option<String>,
-    pub max_age: Option<NonZeroU32>,
     pub response_mode: ResponseMode,
     pub response_type_id_token: bool,
     pub created_at: DateTime<Utc>,
-    pub requires_consent: bool,
     pub login_hint: Option<String>,
 }
 
@@ -174,18 +170,7 @@ impl std::ops::Deref for AuthorizationGrant {
     }
 }
 
-const DEFAULT_MAX_AGE: Duration = Duration::microseconds(3600 * 24 * 365 * 1000 * 1000);
-
 impl AuthorizationGrant {
-    #[must_use]
-    pub fn max_auth_time(&self) -> DateTime<Utc> {
-        let max_age = self
-            .max_age
-            .and_then(|x| Duration::try_seconds(x.get().into()))
-            .unwrap_or(DEFAULT_MAX_AGE);
-        self.created_at - max_age
-    }
-
     #[must_use]
     pub fn parse_login_hint(&self, homeserver: &str) -> LoginHint {
         let Some(login_hint) = &self.login_hint else {
@@ -274,11 +259,9 @@ impl AuthorizationGrant {
             scope: Scope::from_iter([OPENID, PROFILE]),
             state: Some(Alphanumeric.sample_string(rng, 10)),
             nonce: Some(Alphanumeric.sample_string(rng, 10)),
-            max_age: None,
             response_mode: ResponseMode::Query,
             response_type_id_token: false,
             created_at: now,
-            requires_consent: false,
             login_hint: Some(String::from("mxid:@example-user:example.com")),
         }
     }

crates/handlers/src/oauth2/authorization/complete.rs

@@ -11,7 +11,7 @@ use axum::{
 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, Device};
+use mas_data_model::{AuthorizationGrant, BrowserSession, Client};
 use mas_keystore::Keystore;
 use mas_policy::{EvaluationResult, Policy};
 use mas_router::{PostAuthAction, UrlBuilder};
@@ -149,15 +149,6 @@ pub(crate) async fn get(
             let res = callback_destination.go(&templates, &locale, params).await?;
             Ok((cookie_jar, res).into_response())
         }
-        Err(GrantCompletionError::RequiresReauth) => Ok((
-            cookie_jar,
-            url_builder.redirect(&mas_router::Reauth::and_then(continue_grant)),
-        )
-            .into_response()),
-        Err(GrantCompletionError::RequiresConsent) => {
-            let next = mas_router::Consent(grant_id);
-            Ok((cookie_jar, url_builder.redirect(&next)).into_response())
-        }
         Err(GrantCompletionError::PolicyViolation(grant, res)) => {
             warn!(violation = ?res, "Authorization grant for client {} denied by policy", client.id);
 
@@ -184,12 +175,6 @@ pub enum GrantCompletionError {
     #[error("authorization grant is not in a pending state")]
     NotPending,
 
-    #[error("user needs to reauthenticate")]
-    RequiresReauth,
-
-    #[error("client lacks consent")]
-    RequiresConsent,
-
     #[error("denied by the policy")]
     PolicyViolation(AuthorizationGrant, EvaluationResult),
 }
@@ -218,18 +203,6 @@ pub(crate) async fn complete(
         return Err(GrantCompletionError::NotPending);
     }
 
-    // Check if the authentication is fresh enough
-    let authentication = repo
-        .browser_session()
-        .get_last_authentication(browser_session)
-        .await?;
-    let authentication = authentication.filter(|auth| auth.created_at > grant.max_auth_time());
-
-    let Some(valid_authentication) = authentication else {
-        repo.save().await?;
-        return Err(GrantCompletionError::RequiresReauth);
-    };
-
     // Run through the policy
     let res = policy
         .evaluate_authorization_grant(mas_policy::AuthorizationGrantInput {
@@ -248,23 +221,6 @@ pub(crate) async fn complete(
         return Err(GrantCompletionError::PolicyViolation(grant, res));
     }
 
-    let current_consent = repo
-        .oauth2_client()
-        .get_consent_for_user(client, &browser_session.user)
-        .await?;
-
-    let lacks_consent = grant
-        .scope
-        .difference(&current_consent)
-        .filter(|scope| Device::from_scope_token(scope).is_none())
-        .any(|_| true);
-
-    // Check if the client lacks consent *or* if consent was explicitly asked
-    if lacks_consent || grant.requires_consent {
-        repo.save().await?;
-        return Err(GrantCompletionError::RequiresConsent);
-    }
-
     // All good, let's start the session
     let session = repo
         .oauth2_session()
@@ -281,6 +237,12 @@ pub(crate) async fn complete(
 
     // 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,
@@ -290,7 +252,7 @@ pub(crate) async fn complete(
             Some(&grant),
             browser_session,
             None,
-            Some(&valid_authentication),
+            last_authentication.as_ref(),
         )?);
     }
 

crates/handlers/src/oauth2/authorization/mod.rs

@@ -6,20 +6,17 @@
 
 use axum::{
     extract::{Form, State},
-    response::{Html, IntoResponse, Response},
+    response::{IntoResponse, Response},
 };
-use axum_extra::TypedHeader;
 use hyper::StatusCode;
-use mas_axum_utils::{SessionInfoExt, cookies::CookieJar, csrf::CsrfExt, sentry::SentryEventID};
+use mas_axum_utils::{SessionInfoExt, cookies::CookieJar, sentry::SentryEventID};
 use mas_data_model::{AuthorizationCode, Pkce};
-use mas_keystore::Keystore;
-use mas_policy::Policy;
 use mas_router::{PostAuthAction, UrlBuilder};
 use mas_storage::{
     BoxClock, BoxRepository, BoxRng,
     oauth2::{OAuth2AuthorizationGrantRepository, OAuth2ClientRepository},
 };
-use mas_templates::{PolicyViolationContext, TemplateContext, Templates};
+use mas_templates::Templates;
 use oauth2_types::{
     errors::{ClientError, ClientErrorCode},
     pkce,
@@ -29,9 +26,8 @@ use oauth2_types::{
 use rand::{Rng, distributions::Alphanumeric};
 use serde::Deserialize;
 use thiserror::Error;
-use tracing::warn;
 
-use self::{callback::CallbackDestination, complete::GrantCompletionError};
+use self::callback::CallbackDestination;
 use crate::{BoundActivityTracker, PreferredLanguage, impl_from_error_for_route};
 
 mod callback;
@@ -134,10 +130,7 @@ pub(crate) async fn get(
     clock: BoxClock,
     PreferredLanguage(locale): PreferredLanguage,
     State(templates): State<Templates>,
-    State(key_store): State<Keystore>,
     State(url_builder): State<UrlBuilder>,
-    policy: Policy,
-    user_agent: Option<TypedHeader<headers::UserAgent>>,
     activity_tracker: BoundActivityTracker,
     mut repo: BoxRepository,
     cookie_jar: CookieJar,
@@ -166,9 +159,6 @@ 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);
-
-    let user_agent = user_agent.map(|TypedHeader(ua)| ua.to_string());
 
     // One day, we will have try blocks
     let res: Result<Response, RouteError> = ({
@@ -235,8 +225,8 @@ pub(crate) async fn get(
                     .await?);
             }
 
-            // Fail early if prompt=none and there is no active session
-            if prompt.contains(&Prompt::None) && maybe_session.is_none() {
+            // Fail early if prompt=none; we never let it go through
+            if prompt.contains(&Prompt::None) {
                 return Ok(callback_destination
                     .go(
                         &templates,
@@ -287,8 +277,6 @@ pub(crate) async fn get(
                 None
             };
 
-            let requires_consent = prompt.contains(&Prompt::Consent);
-
             let grant = repo
                 .oauth2_authorization_grant()
                 .add(
@@ -300,151 +288,43 @@ pub(crate) async fn get(
                     code,
                     params.auth.state.clone(),
                     params.auth.nonce,
-                    params.auth.max_age,
                     response_mode,
                     response_type.has_id_token(),
-                    requires_consent,
                     params.auth.login_hint,
                 )
                 .await?;
             let continue_grant = PostAuthAction::continue_grant(grant.id);
 
             let res = match maybe_session {
-                // Cases where there is no active session, redirect to the relevant page
-                None if prompt.contains(&Prompt::None) => {
-                    // This case should already be handled earlier
-                    unreachable!();
-                }
                 None if prompt.contains(&Prompt::Create) => {
                     // Client asked for a registration, show the registration prompt
                     repo.save().await?;
 
-                    url_builder.redirect(&mas_router::Register::and_then(continue_grant))
+                    url_builder
+                        .redirect(&mas_router::Register::and_then(continue_grant))
                         .into_response()
                 }
+
                 None => {
                     // Other cases where we don't have a session, ask for a login
                     repo.save().await?;
 
-                    url_builder.redirect(&mas_router::Login::and_then(continue_grant))
+                    url_builder
+                        .redirect(&mas_router::Login::and_then(continue_grant))
                         .into_response()
                 }
 
-                // Special case when we already have a session but prompt=login|select_account
-                Some(session)
-                    if prompt.contains(&Prompt::Login)
-                        || prompt.contains(&Prompt::SelectAccount) =>
-                {
-                    // TODO: better pages here
+                Some(user_session) => {
+                    // TODO: better support for prompt=create when we have a session
                     repo.save().await?;
 
-                    activity_tracker.record_browser_session(&clock, &session).await;
-
-                    url_builder.redirect(&mas_router::Reauth::and_then(continue_grant))
+                    activity_tracker
+                        .record_browser_session(&clock, &user_session)
+                        .await;
+                    url_builder
+                        .redirect(&mas_router::Consent(grant.id))
                         .into_response()
                 }
-
-                // Else, we immediately try to complete the authorization grant
-                Some(user_session) if prompt.contains(&Prompt::None) => {
-                    activity_tracker.record_browser_session(&clock, &user_session).await;
-
-                    // With prompt=none, we should get back to the client immediately
-                    match self::complete::complete(
-                        &mut rng,
-                        &clock,
-                        &activity_tracker,
-                        user_agent,
-                        repo,
-                        key_store,
-                        policy,
-                        &url_builder,
-                        grant,
-                        &client,
-                        &user_session,
-                    )
-                    .await
-                    {
-                        Ok(params) => callback_destination.go(&templates, &locale, params).await?,
-                        Err(GrantCompletionError::RequiresConsent) => {
-                            callback_destination
-                                .go(
-                                    &templates,
-                                    &locale,
-                                    ClientError::from(ClientErrorCode::ConsentRequired),
-                                )
-                                .await?
-                        }
-                        Err(GrantCompletionError::RequiresReauth) => {
-                            callback_destination
-                                .go(
-                                    &templates,
-                                    &locale,
-                                    ClientError::from(ClientErrorCode::InteractionRequired),
-                                )
-                                .await?
-                        }
-                        Err(GrantCompletionError::PolicyViolation(_grant, _res)) => {
-                            callback_destination
-                                .go(&templates, &locale, ClientError::from(ClientErrorCode::AccessDenied))
-                                .await?
-                        }
-                        Err(GrantCompletionError::Internal(e)) => {
-                            return Err(RouteError::Internal(e))
-                        }
-                        Err(e @ GrantCompletionError::NotPending) => {
-                            // This should never happen
-                            return Err(RouteError::Internal(Box::new(e)));
-                        }
-                    }
-                }
-                Some(user_session) => {
-                    activity_tracker.record_browser_session(&clock, &user_session).await;
-
-                    let grant_id = grant.id;
-                    // Else, we show the relevant reauth/consent page if necessary
-                    match self::complete::complete(
-                        &mut rng,
-                        &clock,
-                        &activity_tracker,
-                        user_agent,
-                        repo,
-                        key_store,
-                        policy,
-                        &url_builder,
-                        grant,
-                        &client,
-                        &user_session,
-                    )
-                    .await
-                    {
-                        Ok(params) => callback_destination.go(&templates, &locale, params).await?,
-                        Err(GrantCompletionError::RequiresConsent) => {
-                            url_builder.redirect(&mas_router::Consent(grant_id)).into_response()
-                        }
-                        Err(GrantCompletionError::PolicyViolation(grant, res)) => {
-                            warn!(violation = ?res, "Authorization grant for client {} denied by policy", client.id);
-
-                            let ctx = PolicyViolationContext::for_authorization_grant(grant, client)
-                                .with_session(user_session)
-                                .with_csrf(csrf_token.form_value())
-                                .with_language(locale);
-
-                            let content = templates.render_policy_violation(&ctx)?;
-                            Html(content).into_response()
-                        }
-                        Err(GrantCompletionError::RequiresReauth) => {
-                            url_builder.redirect(&mas_router::Reauth::and_then(continue_grant))
-                                .into_response()
-                        }
-                        Err(GrantCompletionError::Internal(e)) => {
-                            return Err(RouteError::Internal(e))
-                        }
-                        Err(e @ GrantCompletionError::NotPending) => {
-                            // This should never happen
-                            return Err(RouteError::Internal(Box::new(e)));
-                        }
-                    }
-                }
             };
 
             Ok(res)