From 73a4007c18325f3840e51fe2f2bde9369c6472a9 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 10 Apr 2025 19:57:45 +0200 Subject: [PATCH] Always ask for consent, never for reauth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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! --- .../src/oauth2/authorization_grant.rs | 19 +-- .../src/oauth2/authorization/complete.rs | 54 +----- .../handlers/src/oauth2/authorization/mod.rs | 156 ++---------------- crates/handlers/src/oauth2/consent.rs | 24 +-- crates/handlers/src/oauth2/discovery.rs | 2 +- crates/handlers/src/oauth2/token.rs | 4 - ...1aafadb0f4e98caeaba01597c6f62875ae691.json | 29 ---- ...18b8817d1bb511e08ee761a031fa12d27251.json} | 30 +--- ...6f7ae39bc5cb1572e3a2dcfcd67f196a1fa39.json | 23 --- ...c1eb529d321d0aa543be70f86ef96d0d8ff28.json | 27 +++ ...2ce7e16bc5df0677c7cd4ecb4fdbc5ee86395.json | 18 -- ...f0168a63e278d8f8c947cb3ab65173f92ae4.json} | 30 +--- ...56d3688851c36a48a50aa6104e8291e73630d.json | 14 -- ...authorization_default_requires_consent.sql | 9 + .../src/oauth2/authorization_grant.rs | 74 +-------- crates/storage-pg/src/oauth2/client.rs | 100 +---------- crates/storage-pg/src/oauth2/mod.rs | 25 --- .../storage/src/oauth2/authorization_grant.rs | 30 ---- crates/storage/src/oauth2/client.rs | 58 +------ 19 files changed, 88 insertions(+), 638 deletions(-) delete mode 100644 crates/storage-pg/.sqlx/query-854cc8cd3c1fc3dbbdf4ce81b561aafadb0f4e98caeaba01597c6f62875ae691.json rename crates/storage-pg/.sqlx/{query-1d9c478c7a5e3a672610376a290b9a1afaaa6fa2fb137f7307002f058b206dbd.json => query-890516adeeaf2d6a4fe83a69e42e18b8817d1bb511e08ee761a031fa12d27251.json} (75%) delete mode 100644 crates/storage-pg/.sqlx/query-8b7297c263336d70c2b647212b16f7ae39bc5cb1572e3a2dcfcd67f196a1fa39.json create mode 100644 crates/storage-pg/.sqlx/query-96208afd8b81e00f2700b0ded40c1eb529d321d0aa543be70f86ef96d0d8ff28.json delete mode 100644 crates/storage-pg/.sqlx/query-9a6c197ff4ad80217262d48f8792ce7e16bc5df0677c7cd4ecb4fdbc5ee86395.json rename crates/storage-pg/.sqlx/{query-e0d3be7e741581430e3e4719c7e19596837234c94a398570bdac42652c2c4652.json => query-bf6d1e3e3145438c988b1a47fc13f0168a63e278d8f8c947cb3ab65173f92ae4.json} (75%) delete mode 100644 crates/storage-pg/.sqlx/query-d83421d4a16f4ad084dd0db5abb56d3688851c36a48a50aa6104e8291e73630d.json create mode 100644 crates/storage-pg/migrations/20250410174306_oauth2_authorization_default_requires_consent.sql diff --git a/crates/data-model/src/oauth2/authorization_grant.rs b/crates/data-model/src/oauth2/authorization_grant.rs index a1b321e4c..170e476d0 100644 --- a/crates/data-model/src/oauth2/authorization_grant.rs +++ b/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, pub nonce: Option, - pub max_age: Option, pub response_mode: ResponseMode, pub response_type_id_token: bool, pub created_at: DateTime, - pub requires_consent: bool, pub login_hint: Option, } @@ -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 { - 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")), } } diff --git a/crates/handlers/src/oauth2/authorization/complete.rs b/crates/handlers/src/oauth2/authorization/complete.rs index bfd07531b..ea534df2c 100644 --- a/crates/handlers/src/oauth2/authorization/complete.rs +++ b/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(¤t_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(), )?); } diff --git a/crates/handlers/src/oauth2/authorization/mod.rs b/crates/handlers/src/oauth2/authorization/mod.rs index 54d0641e3..c953f965b 100644 --- a/crates/handlers/src/oauth2/authorization/mod.rs +++ b/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, - State(key_store): State, State(url_builder): State, - policy: Policy, - user_agent: Option>, 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 = ({ @@ -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) diff --git a/crates/handlers/src/oauth2/consent.rs b/crates/handlers/src/oauth2/consent.rs index 599ba080d..6912fd213 100644 --- a/crates/handlers/src/oauth2/consent.rs +++ b/crates/handlers/src/oauth2/consent.rs @@ -15,7 +15,7 @@ use mas_axum_utils::{ csrf::{CsrfExt, ProtectedForm}, sentry::SentryEventID, }; -use mas_data_model::{AuthorizationGrantStage, Device}; +use mas_data_model::AuthorizationGrantStage; use mas_policy::Policy; use mas_router::{PostAuthAction, UrlBuilder}; use mas_storage::{ @@ -240,28 +240,6 @@ pub(crate) async fn post( return Err(RouteError::PolicyViolation); } - // Do not consent for the "urn:matrix:org.matrix.msc2967.client:device:*" scope - let scope_without_device = grant - .scope - .iter() - .filter(|s| Device::from_scope_token(s).is_none()) - .cloned() - .collect(); - - repo.oauth2_client() - .give_consent_for_user( - &mut rng, - &clock, - &client, - &session.user, - &scope_without_device, - ) - .await?; - - repo.oauth2_authorization_grant() - .give_consent(grant) - .await?; - repo.save().await?; Ok((cookie_jar, next.go_next(&url_builder)).into_response()) diff --git a/crates/handlers/src/oauth2/discovery.rs b/crates/handlers/src/oauth2/discovery.rs index e72ebb358..bdefa3e62 100644 --- a/crates/handlers/src/oauth2/discovery.rs +++ b/crates/handlers/src/oauth2/discovery.rs @@ -132,7 +132,7 @@ pub(crate) async fn get( let request_uri_parameter_supported = Some(false); let prompt_values_supported = Some({ - let mut v = vec![Prompt::None, Prompt::Login]; + let mut v = vec![Prompt::Login]; // Advertise for prompt=create if password registration is enabled // TODO: we may want to be able to forward that to upstream providers if they // support it diff --git a/crates/handlers/src/oauth2/token.rs b/crates/handlers/src/oauth2/token.rs index c1a842be7..d361bcc99 100644 --- a/crates/handlers/src/oauth2/token.rs +++ b/crates/handlers/src/oauth2/token.rs @@ -978,10 +978,8 @@ mod tests { }), Some("state".to_owned()), Some("nonce".to_owned()), - None, ResponseMode::Query, false, - false, None, ) .await @@ -1079,10 +1077,8 @@ mod tests { }), Some("state".to_owned()), Some("nonce".to_owned()), - None, ResponseMode::Query, false, - false, None, ) .await diff --git a/crates/storage-pg/.sqlx/query-854cc8cd3c1fc3dbbdf4ce81b561aafadb0f4e98caeaba01597c6f62875ae691.json b/crates/storage-pg/.sqlx/query-854cc8cd3c1fc3dbbdf4ce81b561aafadb0f4e98caeaba01597c6f62875ae691.json deleted file mode 100644 index 0e114d418..000000000 --- a/crates/storage-pg/.sqlx/query-854cc8cd3c1fc3dbbdf4ce81b561aafadb0f4e98caeaba01597c6f62875ae691.json +++ /dev/null @@ -1,29 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n INSERT INTO oauth2_authorization_grants (\n oauth2_authorization_grant_id,\n oauth2_client_id,\n redirect_uri,\n scope,\n state,\n nonce,\n max_age,\n response_mode,\n code_challenge,\n code_challenge_method,\n response_type_code,\n response_type_id_token,\n authorization_code,\n requires_consent,\n login_hint,\n created_at\n )\n VALUES\n ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16)\n ", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Uuid", - "Uuid", - "Text", - "Text", - "Text", - "Text", - "Int4", - "Text", - "Text", - "Text", - "Bool", - "Bool", - "Text", - "Bool", - "Text", - "Timestamptz" - ] - }, - "nullable": [] - }, - "hash": "854cc8cd3c1fc3dbbdf4ce81b561aafadb0f4e98caeaba01597c6f62875ae691" -} diff --git a/crates/storage-pg/.sqlx/query-1d9c478c7a5e3a672610376a290b9a1afaaa6fa2fb137f7307002f058b206dbd.json b/crates/storage-pg/.sqlx/query-890516adeeaf2d6a4fe83a69e42e18b8817d1bb511e08ee761a031fa12d27251.json similarity index 75% rename from crates/storage-pg/.sqlx/query-1d9c478c7a5e3a672610376a290b9a1afaaa6fa2fb137f7307002f058b206dbd.json rename to crates/storage-pg/.sqlx/query-890516adeeaf2d6a4fe83a69e42e18b8817d1bb511e08ee761a031fa12d27251.json index 3f0d2177d..d8fd25487 100644 --- a/crates/storage-pg/.sqlx/query-1d9c478c7a5e3a672610376a290b9a1afaaa6fa2fb137f7307002f058b206dbd.json +++ b/crates/storage-pg/.sqlx/query-890516adeeaf2d6a4fe83a69e42e18b8817d1bb511e08ee761a031fa12d27251.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT oauth2_authorization_grant_id\n , created_at\n , cancelled_at\n , fulfilled_at\n , exchanged_at\n , scope\n , state\n , redirect_uri\n , response_mode\n , nonce\n , max_age\n , oauth2_client_id\n , authorization_code\n , response_type_code\n , response_type_id_token\n , code_challenge\n , code_challenge_method\n , requires_consent\n , login_hint\n , oauth2_session_id\n FROM\n oauth2_authorization_grants\n\n WHERE authorization_code = $1\n ", + "query": "\n SELECT oauth2_authorization_grant_id\n , created_at\n , cancelled_at\n , fulfilled_at\n , exchanged_at\n , scope\n , state\n , redirect_uri\n , response_mode\n , nonce\n , oauth2_client_id\n , authorization_code\n , response_type_code\n , response_type_id_token\n , code_challenge\n , code_challenge_method\n , login_hint\n , oauth2_session_id\n FROM\n oauth2_authorization_grants\n\n WHERE authorization_code = $1\n ", "describe": { "columns": [ { @@ -55,51 +55,41 @@ }, { "ordinal": 10, - "name": "max_age", - "type_info": "Int4" - }, - { - "ordinal": 11, "name": "oauth2_client_id", "type_info": "Uuid" }, { - "ordinal": 12, + "ordinal": 11, "name": "authorization_code", "type_info": "Text" }, { - "ordinal": 13, + "ordinal": 12, "name": "response_type_code", "type_info": "Bool" }, { - "ordinal": 14, + "ordinal": 13, "name": "response_type_id_token", "type_info": "Bool" }, { - "ordinal": 15, + "ordinal": 14, "name": "code_challenge", "type_info": "Text" }, { - "ordinal": 16, + "ordinal": 15, "name": "code_challenge_method", "type_info": "Text" }, { - "ordinal": 17, - "name": "requires_consent", - "type_info": "Bool" - }, - { - "ordinal": 18, + "ordinal": 16, "name": "login_hint", "type_info": "Text" }, { - "ordinal": 19, + "ordinal": 17, "name": "oauth2_session_id", "type_info": "Uuid" } @@ -120,17 +110,15 @@ false, false, true, - true, false, true, false, false, true, true, - false, true, true ] }, - "hash": "1d9c478c7a5e3a672610376a290b9a1afaaa6fa2fb137f7307002f058b206dbd" + "hash": "890516adeeaf2d6a4fe83a69e42e18b8817d1bb511e08ee761a031fa12d27251" } diff --git a/crates/storage-pg/.sqlx/query-8b7297c263336d70c2b647212b16f7ae39bc5cb1572e3a2dcfcd67f196a1fa39.json b/crates/storage-pg/.sqlx/query-8b7297c263336d70c2b647212b16f7ae39bc5cb1572e3a2dcfcd67f196a1fa39.json deleted file mode 100644 index 0389ab030..000000000 --- a/crates/storage-pg/.sqlx/query-8b7297c263336d70c2b647212b16f7ae39bc5cb1572e3a2dcfcd67f196a1fa39.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n SELECT scope_token\n FROM oauth2_consents\n WHERE user_id = $1 AND oauth2_client_id = $2\n ", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "scope_token", - "type_info": "Text" - } - ], - "parameters": { - "Left": [ - "Uuid", - "Uuid" - ] - }, - "nullable": [ - false - ] - }, - "hash": "8b7297c263336d70c2b647212b16f7ae39bc5cb1572e3a2dcfcd67f196a1fa39" -} diff --git a/crates/storage-pg/.sqlx/query-96208afd8b81e00f2700b0ded40c1eb529d321d0aa543be70f86ef96d0d8ff28.json b/crates/storage-pg/.sqlx/query-96208afd8b81e00f2700b0ded40c1eb529d321d0aa543be70f86ef96d0d8ff28.json new file mode 100644 index 000000000..2f372898b --- /dev/null +++ b/crates/storage-pg/.sqlx/query-96208afd8b81e00f2700b0ded40c1eb529d321d0aa543be70f86ef96d0d8ff28.json @@ -0,0 +1,27 @@ +{ + "db_name": "PostgreSQL", + "query": "\n INSERT INTO oauth2_authorization_grants (\n oauth2_authorization_grant_id,\n oauth2_client_id,\n redirect_uri,\n scope,\n state,\n nonce,\n response_mode,\n code_challenge,\n code_challenge_method,\n response_type_code,\n response_type_id_token,\n authorization_code,\n login_hint,\n created_at\n )\n VALUES\n ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14)\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Uuid", + "Uuid", + "Text", + "Text", + "Text", + "Text", + "Text", + "Text", + "Text", + "Bool", + "Bool", + "Text", + "Text", + "Timestamptz" + ] + }, + "nullable": [] + }, + "hash": "96208afd8b81e00f2700b0ded40c1eb529d321d0aa543be70f86ef96d0d8ff28" +} diff --git a/crates/storage-pg/.sqlx/query-9a6c197ff4ad80217262d48f8792ce7e16bc5df0677c7cd4ecb4fdbc5ee86395.json b/crates/storage-pg/.sqlx/query-9a6c197ff4ad80217262d48f8792ce7e16bc5df0677c7cd4ecb4fdbc5ee86395.json deleted file mode 100644 index 07d5aa55f..000000000 --- a/crates/storage-pg/.sqlx/query-9a6c197ff4ad80217262d48f8792ce7e16bc5df0677c7cd4ecb4fdbc5ee86395.json +++ /dev/null @@ -1,18 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n INSERT INTO oauth2_consents\n (oauth2_consent_id, user_id, oauth2_client_id, scope_token, created_at)\n SELECT id, $2, $3, scope_token, $5 FROM UNNEST($1::uuid[], $4::text[]) u(id, scope_token)\n ON CONFLICT (user_id, oauth2_client_id, scope_token) DO UPDATE SET refreshed_at = $5\n ", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "UuidArray", - "Uuid", - "Uuid", - "TextArray", - "Timestamptz" - ] - }, - "nullable": [] - }, - "hash": "9a6c197ff4ad80217262d48f8792ce7e16bc5df0677c7cd4ecb4fdbc5ee86395" -} diff --git a/crates/storage-pg/.sqlx/query-e0d3be7e741581430e3e4719c7e19596837234c94a398570bdac42652c2c4652.json b/crates/storage-pg/.sqlx/query-bf6d1e3e3145438c988b1a47fc13f0168a63e278d8f8c947cb3ab65173f92ae4.json similarity index 75% rename from crates/storage-pg/.sqlx/query-e0d3be7e741581430e3e4719c7e19596837234c94a398570bdac42652c2c4652.json rename to crates/storage-pg/.sqlx/query-bf6d1e3e3145438c988b1a47fc13f0168a63e278d8f8c947cb3ab65173f92ae4.json index 485b7fd9d..7a52e4781 100644 --- a/crates/storage-pg/.sqlx/query-e0d3be7e741581430e3e4719c7e19596837234c94a398570bdac42652c2c4652.json +++ b/crates/storage-pg/.sqlx/query-bf6d1e3e3145438c988b1a47fc13f0168a63e278d8f8c947cb3ab65173f92ae4.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT oauth2_authorization_grant_id\n , created_at\n , cancelled_at\n , fulfilled_at\n , exchanged_at\n , scope\n , state\n , redirect_uri\n , response_mode\n , nonce\n , max_age\n , oauth2_client_id\n , authorization_code\n , response_type_code\n , response_type_id_token\n , code_challenge\n , code_challenge_method\n , requires_consent\n , login_hint\n , oauth2_session_id\n FROM\n oauth2_authorization_grants\n\n WHERE oauth2_authorization_grant_id = $1\n ", + "query": "\n SELECT oauth2_authorization_grant_id\n , created_at\n , cancelled_at\n , fulfilled_at\n , exchanged_at\n , scope\n , state\n , redirect_uri\n , response_mode\n , nonce\n , oauth2_client_id\n , authorization_code\n , response_type_code\n , response_type_id_token\n , code_challenge\n , code_challenge_method\n , login_hint\n , oauth2_session_id\n FROM\n oauth2_authorization_grants\n\n WHERE oauth2_authorization_grant_id = $1\n ", "describe": { "columns": [ { @@ -55,51 +55,41 @@ }, { "ordinal": 10, - "name": "max_age", - "type_info": "Int4" - }, - { - "ordinal": 11, "name": "oauth2_client_id", "type_info": "Uuid" }, { - "ordinal": 12, + "ordinal": 11, "name": "authorization_code", "type_info": "Text" }, { - "ordinal": 13, + "ordinal": 12, "name": "response_type_code", "type_info": "Bool" }, { - "ordinal": 14, + "ordinal": 13, "name": "response_type_id_token", "type_info": "Bool" }, { - "ordinal": 15, + "ordinal": 14, "name": "code_challenge", "type_info": "Text" }, { - "ordinal": 16, + "ordinal": 15, "name": "code_challenge_method", "type_info": "Text" }, { - "ordinal": 17, - "name": "requires_consent", - "type_info": "Bool" - }, - { - "ordinal": 18, + "ordinal": 16, "name": "login_hint", "type_info": "Text" }, { - "ordinal": 19, + "ordinal": 17, "name": "oauth2_session_id", "type_info": "Uuid" } @@ -120,17 +110,15 @@ false, false, true, - true, false, true, false, false, true, true, - false, true, true ] }, - "hash": "e0d3be7e741581430e3e4719c7e19596837234c94a398570bdac42652c2c4652" + "hash": "bf6d1e3e3145438c988b1a47fc13f0168a63e278d8f8c947cb3ab65173f92ae4" } diff --git a/crates/storage-pg/.sqlx/query-d83421d4a16f4ad084dd0db5abb56d3688851c36a48a50aa6104e8291e73630d.json b/crates/storage-pg/.sqlx/query-d83421d4a16f4ad084dd0db5abb56d3688851c36a48a50aa6104e8291e73630d.json deleted file mode 100644 index 2d671aa52..000000000 --- a/crates/storage-pg/.sqlx/query-d83421d4a16f4ad084dd0db5abb56d3688851c36a48a50aa6104e8291e73630d.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n UPDATE oauth2_authorization_grants AS og\n SET\n requires_consent = 'f'\n WHERE\n og.oauth2_authorization_grant_id = $1\n ", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Uuid" - ] - }, - "nullable": [] - }, - "hash": "d83421d4a16f4ad084dd0db5abb56d3688851c36a48a50aa6104e8291e73630d" -} diff --git a/crates/storage-pg/migrations/20250410174306_oauth2_authorization_default_requires_consent.sql b/crates/storage-pg/migrations/20250410174306_oauth2_authorization_default_requires_consent.sql new file mode 100644 index 000000000..05960c32e --- /dev/null +++ b/crates/storage-pg/migrations/20250410174306_oauth2_authorization_default_requires_consent.sql @@ -0,0 +1,9 @@ +-- Copyright 2025 New Vector Ltd. +-- +-- SPDX-License-Identifier: AGPL-3.0-only +-- Please see LICENSE in the repository root for full details. + +-- We stopped reading/writing to this column, but it's not nullable. +-- So we need to add a default value, and drop it in the next release +ALTER TABLE oauth2_authorization_grants + ALTER COLUMN requires_consent SET DEFAULT false; diff --git a/crates/storage-pg/src/oauth2/authorization_grant.rs b/crates/storage-pg/src/oauth2/authorization_grant.rs index 7a6162171..d619573e7 100644 --- a/crates/storage-pg/src/oauth2/authorization_grant.rs +++ b/crates/storage-pg/src/oauth2/authorization_grant.rs @@ -4,8 +4,6 @@ // SPDX-License-Identifier: AGPL-3.0-only // Please see LICENSE in the repository root for full details. -use std::num::NonZeroU32; - use async_trait::async_trait; use chrono::{DateTime, Utc}; use mas_data_model::{ @@ -48,13 +46,11 @@ struct GrantLookup { nonce: Option, redirect_uri: String, response_mode: String, - max_age: Option, response_type_code: bool, response_type_id_token: bool, authorization_code: Option, code_challenge: Option, code_challenge_method: Option, - requires_consent: bool, login_hint: Option, oauth2_client_id: Uuid, oauth2_session_id: Option, @@ -153,25 +149,6 @@ impl TryFrom for AuthorizationGrant { .source(e) })?; - let max_age = value - .max_age - .map(u32::try_from) - .transpose() - .map_err(|e| { - DatabaseInconsistencyError::on("oauth2_authorization_grants") - .column("max_age") - .row(id) - .source(e) - })? - .map(NonZeroU32::try_from) - .transpose() - .map_err(|e| { - DatabaseInconsistencyError::on("oauth2_authorization_grants") - .column("max_age") - .row(id) - .source(e) - })?; - Ok(AuthorizationGrant { id, stage, @@ -180,12 +157,10 @@ impl TryFrom for AuthorizationGrant { scope, state: value.state, nonce: value.nonce, - max_age, response_mode, redirect_uri, created_at: value.created_at, response_type_id_token: value.response_type_id_token, - requires_consent: value.requires_consent, login_hint: value.login_hint, }) } @@ -216,10 +191,8 @@ impl OAuth2AuthorizationGrantRepository for PgOAuth2AuthorizationGrantRepository code: Option, state: Option, nonce: Option, - max_age: Option, response_mode: ResponseMode, response_type_id_token: bool, - requires_consent: bool, login_hint: Option, ) -> Result { let code_challenge = code @@ -230,8 +203,6 @@ impl OAuth2AuthorizationGrantRepository for PgOAuth2AuthorizationGrantRepository .as_ref() .and_then(|c| c.pkce.as_ref()) .map(|p| p.challenge_method.to_string()); - // TODO: this conversion is a bit ugly - let max_age_i32 = max_age.map(|x| i32::try_from(u32::from(x)).unwrap_or(i32::MAX)); let code_str = code.as_ref().map(|c| &c.code); let created_at = clock.now(); @@ -247,19 +218,17 @@ impl OAuth2AuthorizationGrantRepository for PgOAuth2AuthorizationGrantRepository scope, state, nonce, - max_age, response_mode, code_challenge, code_challenge_method, response_type_code, response_type_id_token, authorization_code, - requires_consent, login_hint, created_at ) VALUES - ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16) + ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) "#, Uuid::from(id), Uuid::from(client.id), @@ -267,14 +236,12 @@ impl OAuth2AuthorizationGrantRepository for PgOAuth2AuthorizationGrantRepository scope.to_string(), state, nonce, - max_age_i32, response_mode.to_string(), code_challenge, code_challenge_method, code.is_some(), response_type_id_token, code_str, - requires_consent, login_hint, created_at, ) @@ -291,11 +258,9 @@ impl OAuth2AuthorizationGrantRepository for PgOAuth2AuthorizationGrantRepository scope, state, nonce, - max_age, response_mode, created_at, response_type_id_token, - requires_consent, login_hint, }) } @@ -323,14 +288,12 @@ impl OAuth2AuthorizationGrantRepository for PgOAuth2AuthorizationGrantRepository , redirect_uri , response_mode , nonce - , max_age , oauth2_client_id , authorization_code , response_type_code , response_type_id_token , code_challenge , code_challenge_method - , requires_consent , login_hint , oauth2_session_id FROM @@ -374,14 +337,12 @@ impl OAuth2AuthorizationGrantRepository for PgOAuth2AuthorizationGrantRepository , redirect_uri , response_mode , nonce - , max_age , oauth2_client_id , authorization_code , response_type_code , response_type_id_token , code_challenge , code_challenge_method - , requires_consent , login_hint , oauth2_session_id FROM @@ -480,37 +441,4 @@ impl OAuth2AuthorizationGrantRepository for PgOAuth2AuthorizationGrantRepository Ok(grant) } - - #[tracing::instrument( - name = "db.oauth2_authorization_grant.give_consent", - skip_all, - fields( - db.query.text, - %grant.id, - client.id = %grant.client_id, - ), - err, - )] - async fn give_consent( - &mut self, - mut grant: AuthorizationGrant, - ) -> Result { - sqlx::query!( - r#" - UPDATE oauth2_authorization_grants AS og - SET - requires_consent = 'f' - WHERE - og.oauth2_authorization_grant_id = $1 - "#, - Uuid::from(grant.id), - ) - .traced() - .execute(&mut *self.conn) - .await?; - - grant.requires_consent = false; - - Ok(grant) - } } diff --git a/crates/storage-pg/src/oauth2/client.rs b/crates/storage-pg/src/oauth2/client.rs index b846e80d4..02e57a01a 100644 --- a/crates/storage-pg/src/oauth2/client.rs +++ b/crates/storage-pg/src/oauth2/client.rs @@ -6,20 +6,15 @@ use std::{ collections::{BTreeMap, BTreeSet}, - str::FromStr, string::ToString, }; use async_trait::async_trait; -use mas_data_model::{Client, JwksOrJwksUri, User}; +use mas_data_model::{Client, JwksOrJwksUri}; use mas_iana::{jose::JsonWebSignatureAlg, oauth::OAuthClientAuthenticationMethod}; use mas_jose::jwk::PublicJsonWebKeySet; use mas_storage::{Clock, oauth2::OAuth2ClientRepository}; -use oauth2_types::{ - oidc::ApplicationType, - requests::GrantType, - scope::{Scope, ScopeToken}, -}; +use oauth2_types::{oidc::ApplicationType, requests::GrantType}; use opentelemetry_semantic_conventions::attribute::DB_QUERY_TEXT; use rand::RngCore; use sqlx::PgConnection; @@ -698,97 +693,6 @@ impl OAuth2ClientRepository for PgOAuth2ClientRepository<'_> { .collect() } - #[tracing::instrument( - name = "db.oauth2_client.get_consent_for_user", - skip_all, - fields( - db.query.text, - %user.id, - %client.id, - ), - err, - )] - async fn get_consent_for_user( - &mut self, - client: &Client, - user: &User, - ) -> Result { - let scope_tokens: Vec = sqlx::query_scalar!( - r#" - SELECT scope_token - FROM oauth2_consents - WHERE user_id = $1 AND oauth2_client_id = $2 - "#, - Uuid::from(user.id), - Uuid::from(client.id), - ) - .fetch_all(&mut *self.conn) - .await?; - - let scope: Result = scope_tokens - .into_iter() - .map(|s| ScopeToken::from_str(&s)) - .collect(); - - let scope = scope.map_err(|e| { - DatabaseInconsistencyError::on("oauth2_consents") - .column("scope_token") - .source(e) - })?; - - Ok(scope) - } - - #[tracing::instrument( - name = "db.oauth2_client.give_consent_for_user", - skip_all, - fields( - db.query.text, - %user.id, - %client.id, - %scope, - ), - err, - )] - async fn give_consent_for_user( - &mut self, - rng: &mut (dyn RngCore + Send), - clock: &dyn Clock, - client: &Client, - user: &User, - scope: &Scope, - ) -> Result<(), Self::Error> { - let now = clock.now(); - let (tokens, ids): (Vec, Vec) = scope - .iter() - .map(|token| { - ( - token.to_string(), - Uuid::from(Ulid::from_datetime_with_source(now.into(), rng)), - ) - }) - .unzip(); - - sqlx::query!( - r#" - INSERT INTO oauth2_consents - (oauth2_consent_id, user_id, oauth2_client_id, scope_token, created_at) - SELECT id, $2, $3, scope_token, $5 FROM UNNEST($1::uuid[], $4::text[]) u(id, scope_token) - ON CONFLICT (user_id, oauth2_client_id, scope_token) DO UPDATE SET refreshed_at = $5 - "#, - &ids, - Uuid::from(user.id), - Uuid::from(client.id), - &tokens, - now, - ) - .traced() - .execute(&mut *self.conn) - .await?; - - Ok(()) - } - #[tracing::instrument( name = "db.oauth2_client.delete_by_id", skip_all, diff --git a/crates/storage-pg/src/oauth2/mod.rs b/crates/storage-pg/src/oauth2/mod.rs index a3aadc2ad..5968e625d 100644 --- a/crates/storage-pg/src/oauth2/mod.rs +++ b/crates/storage-pg/src/oauth2/mod.rs @@ -135,10 +135,8 @@ mod tests { }), Some("state".to_owned()), Some("nonce".to_owned()), - None, ResponseMode::Query, true, - false, None, ) .await @@ -175,29 +173,6 @@ mod tests { .await .unwrap(); - // Lookup the consent the user gave to the client - let consent = repo - .oauth2_client() - .get_consent_for_user(&client, &user) - .await - .unwrap(); - assert!(consent.is_empty()); - - // Give consent to the client - let scope = Scope::from_iter([OPENID]); - repo.oauth2_client() - .give_consent_for_user(&mut rng, &clock, &client, &user, &scope) - .await - .unwrap(); - - // Lookup the consent the user gave to the client - let consent = repo - .oauth2_client() - .get_consent_for_user(&client, &user) - .await - .unwrap(); - assert_eq!(scope, consent); - // Lookup a non-existing session let session = repo.oauth2_session().lookup(Ulid::nil()).await.unwrap(); assert_eq!(session, None); diff --git a/crates/storage/src/oauth2/authorization_grant.rs b/crates/storage/src/oauth2/authorization_grant.rs index cff0ba418..7724ace87 100644 --- a/crates/storage/src/oauth2/authorization_grant.rs +++ b/crates/storage/src/oauth2/authorization_grant.rs @@ -4,8 +4,6 @@ // SPDX-License-Identifier: AGPL-3.0-only // Please see LICENSE in the repository root for full details. -use std::num::NonZeroU32; - use async_trait::async_trait; use mas_data_model::{AuthorizationCode, AuthorizationGrant, Client, Session}; use oauth2_types::{requests::ResponseMode, scope::Scope}; @@ -37,12 +35,9 @@ pub trait OAuth2AuthorizationGrantRepository: Send + Sync { /// `response_type` was requested /// * `state`: The state the client sent, if set /// * `nonce`: The nonce the client sent, if set - /// * `max_age`: The maximum age since the user last authenticated, if asked - /// by the client /// * `response_mode`: The response mode the client requested /// * `response_type_id_token`: Whether the `id_token` `response_type` was /// requested - /// * `requires_consent`: Whether the client explicitly requested consent /// * `login_hint`: The login_hint the client sent, if set /// /// # Errors @@ -59,10 +54,8 @@ pub trait OAuth2AuthorizationGrantRepository: Send + Sync { code: Option, state: Option, nonce: Option, - max_age: Option, response_mode: ResponseMode, response_type_id_token: bool, - requires_consent: bool, login_hint: Option, ) -> Result; @@ -131,22 +124,6 @@ pub trait OAuth2AuthorizationGrantRepository: Send + Sync { clock: &dyn Clock, authorization_grant: AuthorizationGrant, ) -> Result; - - /// Unset the `requires_consent` flag on an authorization grant - /// - /// Returns the updated authorization grant - /// - /// # Parameters - /// - /// * `authorization_grant`: The authorization grant to update - /// - /// # Errors - /// - /// Returns [`Self::Error`] if the underlying repository fails - async fn give_consent( - &mut self, - authorization_grant: AuthorizationGrant, - ) -> Result; } repository_impl!(OAuth2AuthorizationGrantRepository: @@ -160,10 +137,8 @@ repository_impl!(OAuth2AuthorizationGrantRepository: code: Option, state: Option, nonce: Option, - max_age: Option, response_mode: ResponseMode, response_type_id_token: bool, - requires_consent: bool, login_hint: Option, ) -> Result; @@ -184,9 +159,4 @@ repository_impl!(OAuth2AuthorizationGrantRepository: clock: &dyn Clock, authorization_grant: AuthorizationGrant, ) -> Result; - - async fn give_consent( - &mut self, - authorization_grant: AuthorizationGrant, - ) -> Result; ); diff --git a/crates/storage/src/oauth2/client.rs b/crates/storage/src/oauth2/client.rs index 113fe3e37..aa5a82a2a 100644 --- a/crates/storage/src/oauth2/client.rs +++ b/crates/storage/src/oauth2/client.rs @@ -7,10 +7,10 @@ use std::collections::{BTreeMap, BTreeSet}; use async_trait::async_trait; -use mas_data_model::{Client, User}; +use mas_data_model::Client; use mas_iana::{jose::JsonWebSignatureAlg, oauth::OAuthClientAuthenticationMethod}; use mas_jose::jwk::PublicJsonWebKeySet; -use oauth2_types::{oidc::ApplicationType, requests::GrantType, scope::Scope}; +use oauth2_types::{oidc::ApplicationType, requests::GrantType}; use rand_core::RngCore; use ulid::Ulid; use url::Url; @@ -171,45 +171,6 @@ pub trait OAuth2ClientRepository: Send + Sync { /// Returns [`Self::Error`] if the underlying repository fails async fn all_static(&mut self) -> Result, Self::Error>; - /// Get the list of scopes that the user has given consent for the given - /// client - /// - /// # Parameters - /// - /// * `client`: The client to get the consent for - /// * `user`: The user to get the consent for - /// - /// # Errors - /// - /// Returns [`Self::Error`] if the underlying repository fails - async fn get_consent_for_user( - &mut self, - client: &Client, - user: &User, - ) -> Result; - - /// Give consent for a set of scopes for the given client and user - /// - /// # Parameters - /// - /// * `rng`: The random number generator to use - /// * `clock`: The clock used to generate timestamps - /// * `client`: The client to give the consent for - /// * `user`: The user to give the consent for - /// * `scope`: The scope to give consent for - /// - /// # Errors - /// - /// Returns [`Self::Error`] if the underlying repository fails - async fn give_consent_for_user( - &mut self, - rng: &mut (dyn RngCore + Send), - clock: &dyn Clock, - client: &Client, - user: &User, - scope: &Scope, - ) -> Result<(), Self::Error>; - /// Delete a client /// /// # Parameters @@ -288,19 +249,4 @@ repository_impl!(OAuth2ClientRepository: async fn delete(&mut self, client: Client) -> Result<(), Self::Error>; async fn delete_by_id(&mut self, id: Ulid) -> Result<(), Self::Error>; - - async fn get_consent_for_user( - &mut self, - client: &Client, - user: &User, - ) -> Result; - - async fn give_consent_for_user( - &mut self, - rng: &mut (dyn RngCore + Send), - clock: &dyn Clock, - client: &Client, - user: &User, - scope: &Scope, - ) -> Result<(), Self::Error>; );