letro/letro-authentication-service
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

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

Browse Source
This commit is contained in:
Mathieu Velten
2024-12-18 17:18:39 +00:00
committed by GitHub
parent 57fe6ca0c2
commit 33e1cdbf16
15 changed files with 56 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
crates/cli/src/sync.rs
View File

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

14
crates/config/src/sections/upstream_oauth2.rs
View File

@@ -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

2
crates/data-model/src/upstream_oauth2/provider.rs
View File

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

9
crates/handlers/src/upstream_oauth2/authorize.rs
View File

@@ -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)

2
crates/handlers/src/upstream_oauth2/cache.rs
View File

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

8
crates/handlers/src/upstream_oauth2/callback.rs
View File

@@ -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

2
crates/handlers/src/upstream_oauth2/link.rs
View File

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

4
crates/handlers/src/views/login.rs
View File

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

2
crates/storage-pg/.sqlx/query-1d758df58ccfead4cb39ee8f88f60b382b7881e9c4ead31ff257ff5ff4414b6e.json generated
View File

@@ -146,7 +146,7 @@
true,
false,
false,
false,
true,
true
]
},

2
crates/storage-pg/.sqlx/query-27d6f228a9a608b5d03d30cb4074be94dc893df9107e982583aa954b5067dfd1.json generated
View File

@@ -144,7 +144,7 @@
true,
false,
false,
false,
true,
true
]
},

7
crates/storage-pg/migrations/20241212154426_oauth2_response_mode_null.sql Normal file
View File

@@ -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;

4
crates/storage-pg/src/upstream_oauth2/mod.rs
View File

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

22
crates/storage-pg/src/upstream_oauth2/provider.rs
View File

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

2
crates/storage/src/upstream_oauth2/provider.rs
View File

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

6
crates/templates/src/context.rs
View File

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