Skip to content

Commit 8c59ebe

Browse files
MatMaulMatMaul
committed
Allow response_mode to be null and if so do not add the query param
1 parent 80903ed commit 8c59ebe

File tree

16 files changed

+104
-69
lines changed

16 files changed

+104
-69
lines changed

‎crates/cli/src/sync.rs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -235,13 +235,17 @@ pub async fn config_sync(
235235
}
236236
};
237237

238-
let response_mode = match provider.response_mode {
239-
mas_config::UpstreamOAuth2ResponseMode::Query => {
240-
mas_data_model::UpstreamOAuthProviderResponseMode::Query
241-
}
242-
mas_config::UpstreamOAuth2ResponseMode::FormPost => {
243-
mas_data_model::UpstreamOAuthProviderResponseMode::FormPost
238+
let response_mode = if let Some(response_mode) = provider.response_mode {
239+
match response_mode {
240+
mas_config::UpstreamOAuth2ResponseMode::Query => {
241+
Some(mas_data_model::UpstreamOAuthProviderResponseMode::Query)
242+
}
243+
mas_config::UpstreamOAuth2ResponseMode::FormPost => {
244+
Some(mas_data_model::UpstreamOAuthProviderResponseMode::FormPost)
245+
}
244246
}
247+
} else {
248+
None
245249
};
246250

247251
if discovery_mode.is_disabled() {

‎crates/config/src/sections/upstream_oauth2.rs‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -106,12 +106,11 @@ impl ConfigurationSection for UpstreamOAuth2Config {
106106
}
107107

108108
/// The response mode we ask the provider to use for the callback
109-
#[derive(Debug, Clone, Copy, Serialize, Deserialize, Default, JsonSchema)]
109+
#[derive(Debug, Clone, Copy, Serialize, Deserialize, JsonSchema)]
110110
#[serde(rename_all = "snake_case")]
111111
pub enum ResponseMode {
112112
/// `query`: The provider will send the response as a query string in the
113113
/// URL search parameters
114-
#[default]
115114
Query,
116115

117116
/// `form_post`: The provider will send the response as a POST request with
@@ -121,13 +120,6 @@ pub enum ResponseMode {
121120
FormPost,
122121
}
123122

124-
impl ResponseMode {
125-
#[allow(clippy::trivially_copy_pass_by_ref)]
126-
const fn is_default(&self) -> bool {
127-
matches!(self, ResponseMode::Query)
128-
}
129-
}
130-
131123
/// Authentication methods used against the OAuth 2.0 provider
132124
#[derive(Debug, Clone, Copy, Serialize, Deserialize, JsonSchema)]
133125
#[serde(rename_all = "snake_case")]
@@ -550,8 +542,8 @@ pub struct Provider {
550542
pub jwks_uri: Option<Url>,
551543

552544
/// The response mode we ask the provider to use for the callback
553-
#[serde(default, skip_serializing_if = "ResponseMode::is_default")]
554-
pub response_mode: ResponseMode,
545+
#[serde(skip_serializing_if = "Option::is_none")]
546+
pub response_mode: Option<ResponseMode>,
555547

556548
/// How claims should be imported from the `id_token` provided by the
557549
/// provider

‎crates/data-model/src/upstream_oauth2/provider.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ pub struct UpstreamOAuthProvider {
236236
pub token_endpoint_signing_alg: Option<JsonWebSignatureAlg>,
237237
pub token_endpoint_auth_method: TokenAuthMethod,
238238
pub id_token_signed_response_alg: JsonWebSignatureAlg,
239-
pub response_mode: ResponseMode,
239+
pub response_mode: Option<ResponseMode>,
240240
pub created_at: DateTime<Utc>,
241241
pub disabled_at: Option<DateTime<Utc>>,
242242
pub claims_imports: ClaimsImports,

‎crates/handlers/src/oauth2/authorization/mod.rs‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -106,18 +106,25 @@ fn resolve_response_mode(
106106
) -> Result<ResponseMode, RouteError> {
107107
use ResponseMode as M;
108108

109-
// If the response type includes either "token" or "id_token", the default
110-
// response mode is "fragment" and the response mode "query" must not be
111-
// used
112109
if response_type.has_token() || response_type.has_id_token() {
113-
match suggested_response_mode {
114-
None => Ok(M::Fragment),
115-
Some(M::Query) => Err(RouteError::InvalidResponseMode),
116-
Some(mode) => Ok(mode),
110+
// If the response type includes either "token" or "id_token", the default
111+
// response mode is "fragment" and the response mode "query" must not be
112+
// used
113+
if let Some(suggested_response_mode) = suggested_response_mode {
114+
match suggested_response_mode {
115+
M::Query => Err(RouteError::InvalidResponseMode),
116+
mode => Ok(mode),
117+
}
118+
} else {
119+
Ok(M::Fragment)
117120
}
118121
} else {
119122
// In other cases, all response modes are allowed, defaulting to "query"
120-
Ok(suggested_response_mode.unwrap_or(M::Query))
123+
if let Some(suggested_response_mode) = suggested_response_mode {
124+
Ok(suggested_response_mode)
125+
} else {
126+
Ok(M::Query)
127+
}
121128
}
122129
}
123130

‎crates/handlers/src/upstream_oauth2/authorize.rs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,12 +83,15 @@ pub(crate) async fn get(
8383

8484
let redirect_uri = url_builder.upstream_oauth_callback(provider.id);
8585

86-
let data = AuthorizationRequestData::new(
86+
let mut data = AuthorizationRequestData::new(
8787
provider.client_id.clone(),
8888
provider.scope.clone(),
8989
redirect_uri,
90-
)
91-
.with_response_mode(provider.response_mode.into());
90+
);
91+
92+
if let Some(response_mode) = provider.response_mode {
93+
data = data.with_response_mode(response_mode.into());
94+
}
9295

9396
let data = if let Some(methods) = lazy_metadata.pkce_methods().await? {
9497
data.with_code_challenge_methods_supported(methods)

‎crates/handlers/src/upstream_oauth2/cache.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -411,8 +411,8 @@ mod tests {
411411
encrypted_client_secret: None,
412412
token_endpoint_signing_alg: None,
413413
token_endpoint_auth_method: UpstreamOAuthProviderTokenAuthMethod::None,
414-
response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query,
415414
id_token_signed_response_alg: JsonWebSignatureAlg::Rs256,
415+
response_mode: None,
416416
created_at: clock.now(),
417417
disabled_at: None,
418418
claims_imports: UpstreamOAuthProviderClaimsImports::default(),

‎crates/handlers/src/upstream_oauth2/callback.rs‎

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,13 @@ pub(crate) enum RouteError {
109109
MissingFormParams,
110110

111111
#[error("Invalid response mode, expected '{expected}'")]
112-
InvalidParamsMode {
112+
InvalidResponseMode {
113113
expected: UpstreamOAuthProviderResponseMode,
114114
},
115115

116+
#[error("Invalid request method '{method}'")]
117+
InvalidReqMethod { method: Method },
118+
116119
#[error(transparent)]
117120
Internal(Box<dyn std::error::Error + Send + Sync + 'static>),
118121
}
@@ -184,25 +187,46 @@ pub(crate) async fn handler(
184187
// The `Form` extractor will use the body of the request for POST requests and
185188
// the query parameters for GET requests. We need to then look at the method do
186189
// make sure it matches the expected `response_mode`
187-
match (provider.response_mode, method) {
188-
(UpstreamOAuthProviderResponseMode::Query, Method::GET) => {}
189-
(UpstreamOAuthProviderResponseMode::FormPost, Method::POST) => {
190-
// We set the cookies with a `Same-Site` policy set to `Lax`, so because this is
191-
// usually a cross-site form POST, we need to render a form with the
192-
// same values, which posts back to the same URL. However, there are
193-
// other valid reasons for the cookie to be missing, so to track whether we did
194-
// this POST ourselves, we set a flag.
195-
if sessions_cookie.is_empty() && !params.did_mas_repost_to_itself {
196-
let params = Params {
197-
did_mas_repost_to_itself: true,
198-
..params
199-
};
200-
let context = FormPostContext::new_for_current_url(params).with_language(&locale);
201-
let html = templates.render_form_post(&context)?;
202-
return Ok(Html(html).into_response());
203-
}
190+
match method {
191+
Method::GET => {
192+
match provider.response_mode {
193+
Some(UpstreamOAuthProviderResponseMode::Query) | None => {}
194+
Some(UpstreamOAuthProviderResponseMode::FormPost) => {
195+
return Err(RouteError::InvalidResponseMode {
196+
expected: UpstreamOAuthProviderResponseMode::Query,
197+
})
198+
}
199+
};
200+
}
201+
Method::POST => {
202+
match provider.response_mode {
203+
Some(UpstreamOAuthProviderResponseMode::FormPost) => {
204+
// We set the cookies with a `Same-Site` policy set to `Lax`, so because this is
205+
// usually a cross-site form POST, we need to render a form with the
206+
// same values, which posts back to the same URL. However, there are
207+
// other valid reasons for the cookie to be missing, so to track whether we did
208+
// this POST ourselves, we set a flag.
209+
if sessions_cookie.is_empty() && !params.did_mas_repost_to_itself {
210+
let params = Params {
211+
did_mas_repost_to_itself: true,
212+
..params
213+
};
214+
let context =
215+
FormPostContext::new_for_current_url(params).with_language(&locale);
216+
let html = templates.render_form_post(&context)?;
217+
return Ok(Html(html).into_response());
218+
}
219+
}
220+
Some(UpstreamOAuthProviderResponseMode::Query) | None => {
221+
return Err(RouteError::InvalidResponseMode {
222+
expected: UpstreamOAuthProviderResponseMode::FormPost,
223+
})
224+
}
225+
};
226+
}
227+
method => {
228+
return Err(RouteError::InvalidReqMethod { method });
204229
}
205-
(expected, _) => return Err(RouteError::InvalidParamsMode { expected }),
206230
}
207231

208232
let (session_id, _post_auth_action) = sessions_cookie

‎crates/handlers/src/upstream_oauth2/link.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -934,7 +934,7 @@ mod tests {
934934
jwks_uri_override: None,
935935
discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc,
936936
pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto,
937-
response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query,
937+
response_mode: None,
938938
additional_authorization_parameters: Vec::new(),
939939
},
940940
)

‎crates/handlers/src/views/login.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -416,7 +416,7 @@ mod test {
416416
jwks_uri_override: None,
417417
discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc,
418418
pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto,
419-
response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query,
419+
response_mode: None,
420420
additional_authorization_parameters: Vec::new(),
421421
},
422422
)
@@ -456,7 +456,7 @@ mod test {
456456
jwks_uri_override: None,
457457
discovery_mode: mas_data_model::UpstreamOAuthProviderDiscoveryMode::Oidc,
458458
pkce_mode: mas_data_model::UpstreamOAuthProviderPkceMode::Auto,
459-
response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query,
459+
response_mode: None,
460460
additional_authorization_parameters: Vec::new(),
461461
},
462462
)

‎crates/storage-pg/.sqlx/query-1d758df58ccfead4cb39ee8f88f60b382b7881e9c4ead31ff257ff5ff4414b6e.json‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)