Allow response_mode to be null and if so do not add the query param (#…

…3700)
element-hq · Dec 18, 2024 · af1282b · af1282b
1 parent 0bbf0a0
commit af1282b
Show file tree

Hide file tree

Showing 15 changed files with 56 additions and 48 deletions.
diff --git a/crates/cli/src/sync.rs b/crates/cli/src/sync.rs
@@ -235,14 +235,16 @@ 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 = provider
+                .response_mode
+                .map(|response_mode| match response_mode {
+                    mas_config::UpstreamOAuth2ResponseMode::Query => {
+                        mas_data_model::UpstreamOAuthProviderResponseMode::Query
+                    }
+                    mas_config::UpstreamOAuth2ResponseMode::FormPost => {
+                        mas_data_model::UpstreamOAuthProviderResponseMode::FormPost
+                    }
+                });
 
             if discovery_mode.is_disabled() {
                 if provider.authorization_endpoint.is_none() {

diff --git a/crates/config/src/sections/upstream_oauth2.rs b/crates/config/src/sections/upstream_oauth2.rs
@@ -114,12 +114,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
@@ -129,13 +128,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")]
@@ -561,8 +553,8 @@ pub struct Provider {
     pub jwks_uri: Option<Url>,
 
     /// 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<ResponseMode>,
 
     /// 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
@@ -236,7 +236,7 @@ pub struct UpstreamOAuthProvider {
     pub token_endpoint_signing_alg: Option<JsonWebSignatureAlg>,
     pub token_endpoint_auth_method: TokenAuthMethod,
     pub id_token_signed_response_alg: JsonWebSignatureAlg,
-    pub response_mode: ResponseMode,
+    pub response_mode: Option<ResponseMode>,
     pub created_at: DateTime<Utc>,
     pub disabled_at: Option<DateTime<Utc>>,
     pub claims_imports: ClaimsImports,

diff --git 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
@@ -417,8 +417,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
@@ -109,7 +109,7 @@ pub(crate) enum RouteError {
     MissingFormParams,
 
     #[error("Invalid response mode, expected '{expected}'")]
-    InvalidParamsMode {
+    InvalidResponseMode {
         expected: UpstreamOAuthProviderResponseMode,
     },
 
@@ -185,8 +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) {
-        (UpstreamOAuthProviderResponseMode::Query, Method::GET) => {}
-        (UpstreamOAuthProviderResponseMode::FormPost, Method::POST) => {
+        (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
@@ -202,7 +201,8 @@ pub(crate) async fn handler(
                 return Ok(Html(html).into_response());
             }
         }
-        (expected, _) => return Err(RouteError::InvalidParamsMode { expected }),
+        (None, _) | (Some(UpstreamOAuthProviderResponseMode::Query), Method::GET) => {}
+        (Some(expected), _) => return Err(RouteError::InvalidResponseMode { 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
@@ -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
@@ -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/...rage-pg/.sqlx/query-1d758df58ccfead4cb39ee8f88f60b382b7881e9c4ead31ff257ff5ff4414b6e.json b/...rage-pg/.sqlx/query-1d758df58ccfead4cb39ee8f88f60b382b7881e9c4ead31ff257ff5ff4414b6e.json
diff --git a/...rage-pg/.sqlx/query-27d6f228a9a608b5d03d30cb4074be94dc893df9107e982583aa954b5067dfd1.json b/...rage-pg/.sqlx/query-27d6f228a9a608b5d03d30cb4074be94dc893df9107e982583aa954b5067dfd1.json
diff --git a/crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql b/crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql
@@ -0,0 +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;
diff --git 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(),
                 },
             )
@@ -319,7 +319,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
@@ -68,7 +68,7 @@ struct ProviderLookup {
     userinfo_endpoint_override: Option<String>,
     discovery_mode: String,
     pkce_mode: String,
-    response_mode: String,
+    response_mode: Option<String>,
     additional_parameters: Option<Json<Vec<(String, String)>>>,
 }
 
@@ -177,12 +177,16 @@ impl TryFrom<ProviderLookup> 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(&params.additional_authorization_parameters) as _,
             created_at,
         )

diff --git 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<UpstreamOAuthProviderResponseMode>,
 
     /// 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
@@ -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,