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

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

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")]
@@ -522,8 +514,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

@@ -234,7 +234,7 @@ pub struct UpstreamOAuthProvider {
     pub encrypted_client_secret: Option<String>,
     pub token_endpoint_signing_alg: Option<JsonWebSignatureAlg>,
     pub token_endpoint_auth_method: TokenAuthMethod,
-    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/oauth2/authorization/mod.rs

@@ -106,18 +106,25 @@ fn resolve_response_mode(
 ) -> Result<ResponseMode, RouteError> {
     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)
+        }
     }
 }
 

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

@@ -409,7 +409,7 @@ mod tests {
             encrypted_client_secret: None,
             token_endpoint_signing_alg: None,
             token_endpoint_auth_method: UpstreamOAuthProviderTokenAuthMethod::None,
-            response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query,
+            response_mode: None,
             created_at: clock.now(),
             disabled_at: None,
             claims_imports: UpstreamOAuthProviderClaimsImports::default(),

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<dyn std::error::Error + Send + Sync + 'static>),
 }
@@ -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

crates/handlers/src/upstream_oauth2/link.rs

@@ -932,7 +932,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

@@ -413,7 +413,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(),
                 },
             )
@@ -451,7 +451,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(),
                 },
             )

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;

crates/storage-pg/src/upstream_oauth2/mod.rs

@@ -71,7 +71,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(),
                 },
             )
@@ -315,7 +315,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/storage-pg/src/upstream_oauth2/provider.rs

@@ -66,7 +66,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)>>>,
 }
 
@@ -157,12 +157,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
@@ -339,7 +343,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()
@@ -534,7 +538,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,
         )

crates/storage/src/upstream_oauth2/provider.rs

@@ -78,7 +78,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)>,

crates/templates/src/context.rs

@@ -22,7 +22,7 @@ use mas_data_model::{
     AuthorizationGrant, BrowserSession, Client, CompatSsoLogin, CompatSsoLoginState,
     DeviceCodeGrant, UpstreamOAuthLink, UpstreamOAuthProvider, UpstreamOAuthProviderClaimsImports,
     UpstreamOAuthProviderDiscoveryMode, UpstreamOAuthProviderPkceMode,
-    UpstreamOAuthProviderResponseMode, UpstreamOAuthProviderTokenAuthMethod, User, UserAgent,
+    UpstreamOAuthProviderTokenAuthMethod, User, UserAgent,
     UserEmail, UserEmailVerification, UserRecoverySession,
 };
 use mas_i18n::DataLocale;
@@ -1405,7 +1405,7 @@ impl TemplateContext for UpstreamRegister {
                 fetch_userinfo: false,
                 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,