From 8c59ebe7591c9e4843d8a2ff8687efbf8057dafd Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Mon, 16 Dec 2024 10:32:53 +0100 Subject: [PATCH 1/6] Allow response_mode to be null and if so do not add the query param --- crates/cli/src/sync.rs | 16 +++-- crates/config/src/sections/upstream_oauth2.rs | 14 +---- .../src/upstream_oauth2/provider.rs | 2 +- .../handlers/src/oauth2/authorization/mod.rs | 23 ++++--- .../handlers/src/upstream_oauth2/authorize.rs | 9 ++- crates/handlers/src/upstream_oauth2/cache.rs | 2 +- .../handlers/src/upstream_oauth2/callback.rs | 62 +++++++++++++------ crates/handlers/src/upstream_oauth2/link.rs | 2 +- crates/handlers/src/views/login.rs | 4 +- ...60b382b7881e9c4ead31ff257ff5ff4414b6e.json | 2 +- ...4be94dc893df9107e982583aa954b5067dfd1.json | 2 +- ...241212154426_oauth2_response_mode_null.sql | 1 + crates/storage-pg/src/upstream_oauth2/mod.rs | 4 +- .../src/upstream_oauth2/provider.rs | 22 ++++--- .../storage/src/upstream_oauth2/provider.rs | 2 +- crates/templates/src/context.rs | 6 +- 16 files changed, 104 insertions(+), 69 deletions(-) create mode 100644 crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql diff --git a/crates/cli/src/sync.rs b/crates/cli/src/sync.rs index 124ceb530..d9febd78b 100644 --- a/crates/cli/src/sync.rs +++ b/crates/cli/src/sync.rs @@ -235,13 +235,17 @@ pub async fn config_sync( } }; - let response_mode = match provider.response_mode { - mas_config::UpstreamOAuth2ResponseMode::Query => { - mas_data_model::UpstreamOAuthProviderResponseMode::Query - } - mas_config::UpstreamOAuth2ResponseMode::FormPost => { - mas_data_model::UpstreamOAuthProviderResponseMode::FormPost + let response_mode = if let Some(response_mode) = provider.response_mode { + match response_mode { + mas_config::UpstreamOAuth2ResponseMode::Query => { + Some(mas_data_model::UpstreamOAuthProviderResponseMode::Query) + } + mas_config::UpstreamOAuth2ResponseMode::FormPost => { + Some(mas_data_model::UpstreamOAuthProviderResponseMode::FormPost) + } } + } else { + None }; if discovery_mode.is_disabled() { diff --git a/crates/config/src/sections/upstream_oauth2.rs b/crates/config/src/sections/upstream_oauth2.rs index 4db5b4121..5025b077a 100644 --- a/crates/config/src/sections/upstream_oauth2.rs +++ b/crates/config/src/sections/upstream_oauth2.rs @@ -106,12 +106,11 @@ impl ConfigurationSection for UpstreamOAuth2Config { } /// The response mode we ask the provider to use for the callback -#[derive(Debug, Clone, Copy, Serialize, Deserialize, Default, JsonSchema)] +#[derive(Debug, Clone, Copy, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] pub enum ResponseMode { /// `query`: The provider will send the response as a query string in the /// URL search parameters - #[default] Query, /// `form_post`: The provider will send the response as a POST request with @@ -121,13 +120,6 @@ pub enum ResponseMode { FormPost, } -impl ResponseMode { - #[allow(clippy::trivially_copy_pass_by_ref)] - const fn is_default(&self) -> bool { - matches!(self, ResponseMode::Query) - } -} - /// Authentication methods used against the OAuth 2.0 provider #[derive(Debug, Clone, Copy, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "snake_case")] @@ -550,8 +542,8 @@ pub struct Provider { pub jwks_uri: Option, /// The response mode we ask the provider to use for the callback - #[serde(default, skip_serializing_if = "ResponseMode::is_default")] - pub response_mode: ResponseMode, + #[serde(skip_serializing_if = "Option::is_none")] + pub response_mode: Option, /// How claims should be imported from the `id_token` provided by the /// provider diff --git a/crates/data-model/src/upstream_oauth2/provider.rs b/crates/data-model/src/upstream_oauth2/provider.rs index bfbbb8d9f..bee551f2d 100644 --- a/crates/data-model/src/upstream_oauth2/provider.rs +++ b/crates/data-model/src/upstream_oauth2/provider.rs @@ -236,7 +236,7 @@ pub struct UpstreamOAuthProvider { pub token_endpoint_signing_alg: Option, pub token_endpoint_auth_method: TokenAuthMethod, pub id_token_signed_response_alg: JsonWebSignatureAlg, - pub response_mode: ResponseMode, + pub response_mode: Option, pub created_at: DateTime, pub disabled_at: Option>, pub claims_imports: ClaimsImports, diff --git a/crates/handlers/src/oauth2/authorization/mod.rs b/crates/handlers/src/oauth2/authorization/mod.rs index f56f06133..35a075412 100644 --- a/crates/handlers/src/oauth2/authorization/mod.rs +++ b/crates/handlers/src/oauth2/authorization/mod.rs @@ -106,18 +106,25 @@ fn resolve_response_mode( ) -> Result { use ResponseMode as M; - // If the response type includes either "token" or "id_token", the default - // response mode is "fragment" and the response mode "query" must not be - // used if response_type.has_token() || response_type.has_id_token() { - match suggested_response_mode { - None => Ok(M::Fragment), - Some(M::Query) => Err(RouteError::InvalidResponseMode), - Some(mode) => Ok(mode), + // If the response type includes either "token" or "id_token", the default + // response mode is "fragment" and the response mode "query" must not be + // used + if let Some(suggested_response_mode) = suggested_response_mode { + match suggested_response_mode { + M::Query => Err(RouteError::InvalidResponseMode), + mode => Ok(mode), + } + } else { + Ok(M::Fragment) } } else { // In other cases, all response modes are allowed, defaulting to "query" - Ok(suggested_response_mode.unwrap_or(M::Query)) + if let Some(suggested_response_mode) = suggested_response_mode { + Ok(suggested_response_mode) + } else { + Ok(M::Query) + } } } diff --git a/crates/handlers/src/upstream_oauth2/authorize.rs b/crates/handlers/src/upstream_oauth2/authorize.rs index 67d4d9a7d..c1dd3f342 100644 --- a/crates/handlers/src/upstream_oauth2/authorize.rs +++ b/crates/handlers/src/upstream_oauth2/authorize.rs @@ -83,12 +83,15 @@ pub(crate) async fn get( let redirect_uri = url_builder.upstream_oauth_callback(provider.id); - let data = AuthorizationRequestData::new( + let mut data = AuthorizationRequestData::new( provider.client_id.clone(), provider.scope.clone(), redirect_uri, - ) - .with_response_mode(provider.response_mode.into()); + ); + + if let Some(response_mode) = provider.response_mode { + data = data.with_response_mode(response_mode.into()); + } let data = if let Some(methods) = lazy_metadata.pkce_methods().await? { data.with_code_challenge_methods_supported(methods) diff --git a/crates/handlers/src/upstream_oauth2/cache.rs b/crates/handlers/src/upstream_oauth2/cache.rs index 838a82f5b..a19d1a746 100644 --- a/crates/handlers/src/upstream_oauth2/cache.rs +++ b/crates/handlers/src/upstream_oauth2/cache.rs @@ -411,8 +411,8 @@ mod tests { encrypted_client_secret: None, token_endpoint_signing_alg: None, token_endpoint_auth_method: UpstreamOAuthProviderTokenAuthMethod::None, - response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query, id_token_signed_response_alg: JsonWebSignatureAlg::Rs256, + response_mode: None, created_at: clock.now(), disabled_at: None, claims_imports: UpstreamOAuthProviderClaimsImports::default(), diff --git a/crates/handlers/src/upstream_oauth2/callback.rs b/crates/handlers/src/upstream_oauth2/callback.rs index 1e3e2990e..af4438010 100644 --- a/crates/handlers/src/upstream_oauth2/callback.rs +++ b/crates/handlers/src/upstream_oauth2/callback.rs @@ -109,10 +109,13 @@ pub(crate) enum RouteError { MissingFormParams, #[error("Invalid response mode, expected '{expected}'")] - InvalidParamsMode { + InvalidResponseMode { expected: UpstreamOAuthProviderResponseMode, }, + #[error("Invalid request method '{method}'")] + InvalidReqMethod { method: Method }, + #[error(transparent)] Internal(Box), } @@ -184,25 +187,46 @@ pub(crate) async fn handler( // The `Form` extractor will use the body of the request for POST requests and // the query parameters for GET requests. We need to then look at the method do // make sure it matches the expected `response_mode` - match (provider.response_mode, method) { - (UpstreamOAuthProviderResponseMode::Query, Method::GET) => {} - (UpstreamOAuthProviderResponseMode::FormPost, Method::POST) => { - // We set the cookies with a `Same-Site` policy set to `Lax`, so because this is - // usually a cross-site form POST, we need to render a form with the - // same values, which posts back to the same URL. However, there are - // other valid reasons for the cookie to be missing, so to track whether we did - // this POST ourselves, we set a flag. - if sessions_cookie.is_empty() && !params.did_mas_repost_to_itself { - let params = Params { - did_mas_repost_to_itself: true, - ..params - }; - let context = FormPostContext::new_for_current_url(params).with_language(&locale); - let html = templates.render_form_post(&context)?; - return Ok(Html(html).into_response()); - } + match method { + Method::GET => { + match provider.response_mode { + Some(UpstreamOAuthProviderResponseMode::Query) | None => {} + Some(UpstreamOAuthProviderResponseMode::FormPost) => { + return Err(RouteError::InvalidResponseMode { + expected: UpstreamOAuthProviderResponseMode::Query, + }) + } + }; + } + Method::POST => { + match provider.response_mode { + Some(UpstreamOAuthProviderResponseMode::FormPost) => { + // We set the cookies with a `Same-Site` policy set to `Lax`, so because this is + // usually a cross-site form POST, we need to render a form with the + // same values, which posts back to the same URL. However, there are + // other valid reasons for the cookie to be missing, so to track whether we did + // this POST ourselves, we set a flag. + if sessions_cookie.is_empty() && !params.did_mas_repost_to_itself { + let params = Params { + did_mas_repost_to_itself: true, + ..params + }; + let context = + FormPostContext::new_for_current_url(params).with_language(&locale); + let html = templates.render_form_post(&context)?; + return Ok(Html(html).into_response()); + } + } + Some(UpstreamOAuthProviderResponseMode::Query) | None => { + return Err(RouteError::InvalidResponseMode { + expected: UpstreamOAuthProviderResponseMode::FormPost, + }) + } + }; + } + method => { + return Err(RouteError::InvalidReqMethod { method }); } - (expected, _) => return Err(RouteError::InvalidParamsMode { expected }), } let (session_id, _post_auth_action) = sessions_cookie diff --git a/crates/handlers/src/upstream_oauth2/link.rs b/crates/handlers/src/upstream_oauth2/link.rs index b183de977..a784a1536 100644 --- a/crates/handlers/src/upstream_oauth2/link.rs +++ b/crates/handlers/src/upstream_oauth2/link.rs @@ -934,7 +934,7 @@ mod tests { jwks_uri_override: None, discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc, pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto, - response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query, + response_mode: None, additional_authorization_parameters: Vec::new(), }, ) diff --git a/crates/handlers/src/views/login.rs b/crates/handlers/src/views/login.rs index e42f8d643..2fd9adf27 100644 --- a/crates/handlers/src/views/login.rs +++ b/crates/handlers/src/views/login.rs @@ -416,7 +416,7 @@ mod test { jwks_uri_override: None, discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc, pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto, - response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query, + response_mode: None, additional_authorization_parameters: Vec::new(), }, ) @@ -456,7 +456,7 @@ mod test { jwks_uri_override: None, discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc, pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto, - response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query, + response_mode: None, additional_authorization_parameters: Vec::new(), }, ) diff --git a/crates/storage-pg/.sqlx/query-1d758df58ccfead4cb39ee8f88f60b382b7881e9c4ead31ff257ff5ff4414b6e.json b/crates/storage-pg/.sqlx/query-1d758df58ccfead4cb39ee8f88f60b382b7881e9c4ead31ff257ff5ff4414b6e.json index 31d0bcb48..12872d4b6 100644 --- a/crates/storage-pg/.sqlx/query-1d758df58ccfead4cb39ee8f88f60b382b7881e9c4ead31ff257ff5ff4414b6e.json +++ b/crates/storage-pg/.sqlx/query-1d758df58ccfead4cb39ee8f88f60b382b7881e9c4ead31ff257ff5ff4414b6e.json @@ -146,7 +146,7 @@ true, false, false, - false, + true, true ] }, diff --git a/crates/storage-pg/.sqlx/query-27d6f228a9a608b5d03d30cb4074be94dc893df9107e982583aa954b5067dfd1.json b/crates/storage-pg/.sqlx/query-27d6f228a9a608b5d03d30cb4074be94dc893df9107e982583aa954b5067dfd1.json index 77d46d050..f91f999b1 100644 --- a/crates/storage-pg/.sqlx/query-27d6f228a9a608b5d03d30cb4074be94dc893df9107e982583aa954b5067dfd1.json +++ b/crates/storage-pg/.sqlx/query-27d6f228a9a608b5d03d30cb4074be94dc893df9107e982583aa954b5067dfd1.json @@ -144,7 +144,7 @@ true, false, false, - false, + true, true ] }, diff --git a/crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql b/crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql new file mode 100644 index 000000000..9e3c19eb8 --- /dev/null +++ b/crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql @@ -0,0 +1 @@ +ALTER TABLE "upstream_oauth_providers" ALTER COLUMN "response_mode" DROP NOT NULL; diff --git a/crates/storage-pg/src/upstream_oauth2/mod.rs b/crates/storage-pg/src/upstream_oauth2/mod.rs index bdaac8b26..358620ace 100644 --- a/crates/storage-pg/src/upstream_oauth2/mod.rs +++ b/crates/storage-pg/src/upstream_oauth2/mod.rs @@ -74,7 +74,7 @@ mod tests { jwks_uri_override: None, discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc, pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto, - response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query, + response_mode: None, additional_authorization_parameters: Vec::new(), }, ) @@ -320,7 +320,7 @@ mod tests { jwks_uri_override: None, discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc, pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto, - response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query, + response_mode: None, additional_authorization_parameters: Vec::new(), }, ) diff --git a/crates/storage-pg/src/upstream_oauth2/provider.rs b/crates/storage-pg/src/upstream_oauth2/provider.rs index b9befe59c..05694de88 100644 --- a/crates/storage-pg/src/upstream_oauth2/provider.rs +++ b/crates/storage-pg/src/upstream_oauth2/provider.rs @@ -68,7 +68,7 @@ struct ProviderLookup { userinfo_endpoint_override: Option, discovery_mode: String, pkce_mode: String, - response_mode: String, + response_mode: Option, additional_parameters: Option>>, } @@ -177,12 +177,16 @@ impl TryFrom for UpstreamOAuthProvider { .source(e) })?; - let response_mode = value.response_mode.parse().map_err(|e| { - DatabaseInconsistencyError::on("upstream_oauth_providers") - .column("response_mode") - .row(id) - .source(e) - })?; + let response_mode = value + .response_mode + .map(|x| x.parse()) + .transpose() + .map_err(|e| { + DatabaseInconsistencyError::on("upstream_oauth_providers") + .column("response_mode") + .row(id) + .source(e) + })?; let additional_authorization_parameters = value .additional_parameters @@ -370,7 +374,7 @@ impl UpstreamOAuthProviderRepository for PgUpstreamOAuthProviderRepository<'_> { params.jwks_uri_override.as_ref().map(ToString::to_string), params.discovery_mode.as_str(), params.pkce_mode.as_str(), - params.response_mode.as_str(), + params.response_mode.as_ref().map(ToString::to_string), created_at, ) .traced() @@ -576,7 +580,7 @@ impl UpstreamOAuthProviderRepository for PgUpstreamOAuthProviderRepository<'_> { params.jwks_uri_override.as_ref().map(ToString::to_string), params.discovery_mode.as_str(), params.pkce_mode.as_str(), - params.response_mode.as_str(), + params.response_mode.as_ref().map(ToString::to_string), Json(¶ms.additional_authorization_parameters) as _, created_at, ) diff --git a/crates/storage/src/upstream_oauth2/provider.rs b/crates/storage/src/upstream_oauth2/provider.rs index e2271e818..6206eb9a8 100644 --- a/crates/storage/src/upstream_oauth2/provider.rs +++ b/crates/storage/src/upstream_oauth2/provider.rs @@ -91,7 +91,7 @@ pub struct UpstreamOAuthProviderParams { pub pkce_mode: UpstreamOAuthProviderPkceMode, /// What response mode it should ask - pub response_mode: UpstreamOAuthProviderResponseMode, + pub response_mode: Option, /// Additional parameters to include in the authorization request pub additional_authorization_parameters: Vec<(String, String)>, diff --git a/crates/templates/src/context.rs b/crates/templates/src/context.rs index 6a20a4502..865b3ce78 100644 --- a/crates/templates/src/context.rs +++ b/crates/templates/src/context.rs @@ -22,8 +22,8 @@ use mas_data_model::{ AuthorizationGrant, BrowserSession, Client, CompatSsoLogin, CompatSsoLoginState, DeviceCodeGrant, UpstreamOAuthLink, UpstreamOAuthProvider, UpstreamOAuthProviderClaimsImports, UpstreamOAuthProviderDiscoveryMode, UpstreamOAuthProviderPkceMode, - UpstreamOAuthProviderResponseMode, UpstreamOAuthProviderTokenAuthMethod, User, UserAgent, - UserEmail, UserEmailVerification, UserRecoverySession, + UpstreamOAuthProviderTokenAuthMethod, User, UserAgent, UserEmail, UserEmailVerification, + UserRecoverySession, }; use mas_i18n::DataLocale; use mas_iana::jose::JsonWebSignatureAlg; @@ -1408,7 +1408,7 @@ impl TemplateContext for UpstreamRegister { userinfo_signed_response_alg: None, discovery_mode: UpstreamOAuthProviderDiscoveryMode::Oidc, pkce_mode: UpstreamOAuthProviderPkceMode::Auto, - response_mode: UpstreamOAuthProviderResponseMode::Query, + response_mode: None, additional_authorization_parameters: Vec::new(), created_at: now, disabled_at: None, From 67ce0ce297270f828bce37a0aa72bbc96bdf8afe Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 18 Dec 2024 12:26:35 +0100 Subject: [PATCH 2/6] Address comments --- .../handlers/src/oauth2/authorization/mod.rs | 17 ++---- .../handlers/src/upstream_oauth2/callback.rs | 61 ++++++------------- 2 files changed, 24 insertions(+), 54 deletions(-) diff --git a/crates/handlers/src/oauth2/authorization/mod.rs b/crates/handlers/src/oauth2/authorization/mod.rs index 35a075412..eb750f038 100644 --- a/crates/handlers/src/oauth2/authorization/mod.rs +++ b/crates/handlers/src/oauth2/authorization/mod.rs @@ -106,25 +106,18 @@ fn resolve_response_mode( ) -> Result { use ResponseMode as M; - if response_type.has_token() || response_type.has_id_token() { // If the response type includes either "token" or "id_token", the default // response mode is "fragment" and the response mode "query" must not be // used - if let Some(suggested_response_mode) = suggested_response_mode { + if response_type.has_token() || response_type.has_id_token() { match suggested_response_mode { - M::Query => Err(RouteError::InvalidResponseMode), - mode => Ok(mode), - } - } else { - Ok(M::Fragment) + None => Ok(M::Fragment), + Some(M::Query) => Err(RouteError::InvalidResponseMode), + Some(mode) => Ok(mode), } } else { // In other cases, all response modes are allowed, defaulting to "query" - if let Some(suggested_response_mode) = suggested_response_mode { - Ok(suggested_response_mode) - } else { - Ok(M::Query) - } + Ok(suggested_response_mode.unwrap_or(M::Query)) } } diff --git a/crates/handlers/src/upstream_oauth2/callback.rs b/crates/handlers/src/upstream_oauth2/callback.rs index af4438010..034c81af3 100644 --- a/crates/handlers/src/upstream_oauth2/callback.rs +++ b/crates/handlers/src/upstream_oauth2/callback.rs @@ -113,9 +113,6 @@ pub(crate) enum RouteError { expected: UpstreamOAuthProviderResponseMode, }, - #[error("Invalid request method '{method}'")] - InvalidReqMethod { method: Method }, - #[error(transparent)] Internal(Box), } @@ -187,46 +184,26 @@ pub(crate) async fn handler( // The `Form` extractor will use the body of the request for POST requests and // the query parameters for GET requests. We need to then look at the method do // make sure it matches the expected `response_mode` - match method { - Method::GET => { - match provider.response_mode { - Some(UpstreamOAuthProviderResponseMode::Query) | None => {} - Some(UpstreamOAuthProviderResponseMode::FormPost) => { - return Err(RouteError::InvalidResponseMode { - expected: UpstreamOAuthProviderResponseMode::Query, - }) - } - }; - } - Method::POST => { - match provider.response_mode { - Some(UpstreamOAuthProviderResponseMode::FormPost) => { - // We set the cookies with a `Same-Site` policy set to `Lax`, so because this is - // usually a cross-site form POST, we need to render a form with the - // same values, which posts back to the same URL. However, there are - // other valid reasons for the cookie to be missing, so to track whether we did - // this POST ourselves, we set a flag. - if sessions_cookie.is_empty() && !params.did_mas_repost_to_itself { - let params = Params { - did_mas_repost_to_itself: true, - ..params - }; - let context = - FormPostContext::new_for_current_url(params).with_language(&locale); - let html = templates.render_form_post(&context)?; - return Ok(Html(html).into_response()); - } - } - Some(UpstreamOAuthProviderResponseMode::Query) | None => { - return Err(RouteError::InvalidResponseMode { - expected: UpstreamOAuthProviderResponseMode::FormPost, - }) - } - }; - } - method => { - return Err(RouteError::InvalidReqMethod { method }); + match (provider.response_mode, method) { + (Some(UpstreamOAuthProviderResponseMode::Query) | None, Method::GET) => {} + (Some(UpstreamOAuthProviderResponseMode::FormPost) | None, Method::POST) => { + // We set the cookies with a `Same-Site` policy set to `Lax`, so because this is + // usually a cross-site form POST, we need to render a form with the + // same values, which posts back to the same URL. However, there are + // other valid reasons for the cookie to be missing, so to track whether we did + // this POST ourselves, we set a flag. + if sessions_cookie.is_empty() && !params.did_mas_repost_to_itself { + let params = Params { + did_mas_repost_to_itself: true, + ..params + }; + let context = FormPostContext::new_for_current_url(params).with_language(&locale); + let html = templates.render_form_post(&context)?; + return Ok(Html(html).into_response()); + } } + (None, _) => {} + (Some(expected), _) => return Err(RouteError::InvalidResponseMode { expected }), } let (session_id, _post_auth_action) = sessions_cookie From d617774a37442ffdbfd8bf8799189618fce54c57 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 18 Dec 2024 14:09:15 +0100 Subject: [PATCH 3/6] Fixes --- crates/cli/src/sync.rs | 14 ++++++-------- crates/handlers/src/oauth2/authorization/mod.rs | 8 ++++---- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/crates/cli/src/sync.rs b/crates/cli/src/sync.rs index d9febd78b..b61cc6935 100644 --- a/crates/cli/src/sync.rs +++ b/crates/cli/src/sync.rs @@ -235,18 +235,16 @@ pub async fn config_sync( } }; - let response_mode = if let Some(response_mode) = provider.response_mode { - match response_mode { + let response_mode = provider + .response_mode + .map(|response_mode| match response_mode { mas_config::UpstreamOAuth2ResponseMode::Query => { - Some(mas_data_model::UpstreamOAuthProviderResponseMode::Query) + mas_data_model::UpstreamOAuthProviderResponseMode::Query } mas_config::UpstreamOAuth2ResponseMode::FormPost => { - Some(mas_data_model::UpstreamOAuthProviderResponseMode::FormPost) + mas_data_model::UpstreamOAuthProviderResponseMode::FormPost } - } - } else { - None - }; + }); if discovery_mode.is_disabled() { if provider.authorization_endpoint.is_none() { diff --git a/crates/handlers/src/oauth2/authorization/mod.rs b/crates/handlers/src/oauth2/authorization/mod.rs index eb750f038..f56f06133 100644 --- a/crates/handlers/src/oauth2/authorization/mod.rs +++ b/crates/handlers/src/oauth2/authorization/mod.rs @@ -106,11 +106,11 @@ fn resolve_response_mode( ) -> Result { use ResponseMode as M; - // If the response type includes either "token" or "id_token", the default - // response mode is "fragment" and the response mode "query" must not be - // used + // If the response type includes either "token" or "id_token", the default + // response mode is "fragment" and the response mode "query" must not be + // used if response_type.has_token() || response_type.has_id_token() { - match suggested_response_mode { + match suggested_response_mode { None => Ok(M::Fragment), Some(M::Query) => Err(RouteError::InvalidResponseMode), Some(mode) => Ok(mode), From ee84e6886a88dfa9431f5768e4aeba81e143503c Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 18 Dec 2024 14:12:21 +0100 Subject: [PATCH 4/6] Add header --- .../migrations/20241212154426_oauth2_response_mode_null.sql | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql b/crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql index 9e3c19eb8..c6a6b7b4e 100644 --- a/crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql +++ b/crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql @@ -1 +1,7 @@ +-- Copyright 2024 New Vector Ltd. +-- +-- SPDX-License-Identifier: AGPL-3.0-only +-- Please see LICENSE in the repository root for full details. + +-- Drop not null requirement on response mode, so we can ignore this query parameter. ALTER TABLE "upstream_oauth_providers" ALTER COLUMN "response_mode" DROP NOT NULL; From 6d921d6ebfe56a5217bea3837a0617ce50289e55 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 18 Dec 2024 17:22:53 +0100 Subject: [PATCH 5/6] Fix --- crates/handlers/src/upstream_oauth2/callback.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/handlers/src/upstream_oauth2/callback.rs b/crates/handlers/src/upstream_oauth2/callback.rs index 034c81af3..7baa033fc 100644 --- a/crates/handlers/src/upstream_oauth2/callback.rs +++ b/crates/handlers/src/upstream_oauth2/callback.rs @@ -185,7 +185,7 @@ pub(crate) async fn handler( // the query parameters for GET requests. We need to then look at the method do // make sure it matches the expected `response_mode` match (provider.response_mode, method) { - (Some(UpstreamOAuthProviderResponseMode::Query) | None, Method::GET) => {} + (None, _) | (Some(UpstreamOAuthProviderResponseMode::Query) | None, Method::GET) => {} (Some(UpstreamOAuthProviderResponseMode::FormPost) | None, Method::POST) => { // We set the cookies with a `Same-Site` policy set to `Lax`, so because this is // usually a cross-site form POST, we need to render a form with the @@ -202,7 +202,6 @@ pub(crate) async fn handler( return Ok(Html(html).into_response()); } } - (None, _) => {} (Some(expected), _) => return Err(RouteError::InvalidResponseMode { expected }), } From c1e51c333bc7db5ae4337ec9351204e4bc16eec4 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 18 Dec 2024 17:44:40 +0100 Subject: [PATCH 6/6] Fix --- crates/handlers/src/upstream_oauth2/callback.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/handlers/src/upstream_oauth2/callback.rs b/crates/handlers/src/upstream_oauth2/callback.rs index 7baa033fc..d35f2a83b 100644 --- a/crates/handlers/src/upstream_oauth2/callback.rs +++ b/crates/handlers/src/upstream_oauth2/callback.rs @@ -185,7 +185,6 @@ pub(crate) async fn handler( // the query parameters for GET requests. We need to then look at the method do // make sure it matches the expected `response_mode` match (provider.response_mode, method) { - (None, _) | (Some(UpstreamOAuthProviderResponseMode::Query) | None, Method::GET) => {} (Some(UpstreamOAuthProviderResponseMode::FormPost) | None, Method::POST) => { // We set the cookies with a `Same-Site` policy set to `Lax`, so because this is // usually a cross-site form POST, we need to render a form with the @@ -202,6 +201,7 @@ pub(crate) async fn handler( return Ok(Html(html).into_response()); } } + (None, _) | (Some(UpstreamOAuthProviderResponseMode::Query), Method::GET) => {} (Some(expected), _) => return Err(RouteError::InvalidResponseMode { expected }), }