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

Author: MatMaul

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() {

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

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,

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)

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(),

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

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

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