Address comments

Author: MatMaul

Cargo.lock

@@ -399,13 +399,23 @@ fn is_default_true(value: &bool) -> bool {
 }
 
 #[allow(clippy::ref_option)]
-fn is_signed_response_alg_default(signed_response_alg: &Option<JsonWebSignatureAlg>) -> bool {
+fn is_signed_response_alg_default(signed_response_alg: &JsonWebSignatureAlg) -> bool {
     *signed_response_alg == signed_response_alg_default()
 }
 
+#[allow(clippy::ref_option)]
+fn is_optional_signed_response_alg_default(optional_signed_response_alg: &Option<JsonWebSignatureAlg>) -> bool {
+    *optional_signed_response_alg == optional_signed_response_alg_default()
+}
+
 #[allow(clippy::unnecessary_wraps)]
-fn signed_response_alg_default() -> Option<JsonWebSignatureAlg> {
-    Some(JsonWebSignatureAlg::Rs256)
+fn signed_response_alg_default() -> JsonWebSignatureAlg {
+    JsonWebSignatureAlg::Rs256
+}
+
+#[allow(clippy::unnecessary_wraps)]
+fn optional_signed_response_alg_default() -> Option<JsonWebSignatureAlg> {
+    Some(signed_response_alg_default())
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
@@ -485,13 +495,12 @@ pub struct Provider {
     /// Expected signature for the JWT payload returned by the token
     /// authentication endpoint.
     ///
-    /// If null, the response is expected to be an unsigned JSON payload.
     /// Defaults to `RS256`.
     #[serde(
         default = "signed_response_alg_default",
         skip_serializing_if = "is_signed_response_alg_default"
     )]
-    pub id_token_signed_response_alg: Option<JsonWebSignatureAlg>,
+    pub id_token_signed_response_alg: JsonWebSignatureAlg,
 
     /// The scopes to request from the provider
     pub scope: String,
@@ -524,8 +533,8 @@ pub struct Provider {
     /// If null, the response is expected to be an unsigned JSON payload.
     /// Defaults to `RS256`.
     #[serde(
-        default = "signed_response_alg_default",
-        skip_serializing_if = "is_signed_response_alg_default"
+        default = "optional_signed_response_alg_default",
+        skip_serializing_if = "is_optional_signed_response_alg_default"
     )]
     pub userinfo_signed_response_alg: Option<JsonWebSignatureAlg>,
 

crates/config/src/sections/upstream_oauth2.rs

@@ -235,7 +235,7 @@ pub struct UpstreamOAuthProvider {
     pub encrypted_client_secret: Option<String>,
     pub token_endpoint_signing_alg: Option<JsonWebSignatureAlg>,
     pub token_endpoint_auth_method: TokenAuthMethod,
-    pub id_token_signed_response_alg: Option<JsonWebSignatureAlg>,
+    pub id_token_signed_response_alg: JsonWebSignatureAlg,
     pub response_mode: ResponseMode,
     pub created_at: DateTime<Utc>,
     pub disabled_at: Option<DateTime<Utc>>,

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

@@ -289,6 +289,7 @@ mod tests {
     use mas_data_model::{
         UpstreamOAuthProviderClaimsImports, UpstreamOAuthProviderTokenAuthMethod,
     };
+    use mas_iana::jose::JsonWebSignatureAlg;
     use mas_storage::{clock::MockClock, Clock};
     use oauth2_types::scope::{Scope, OPENID};
     use ulid::Ulid;
@@ -411,7 +412,7 @@ mod tests {
             token_endpoint_signing_alg: None,
             token_endpoint_auth_method: UpstreamOAuthProviderTokenAuthMethod::None,
             response_mode: mas_data_model::UpstreamOAuthProviderResponseMode::Query,
-            id_token_signed_response_alg: None,
+            id_token_signed_response_alg: JsonWebSignatureAlg::Rs256,
             created_at: clock.now(),
             disabled_at: None,
             claims_imports: UpstreamOAuthProviderClaimsImports::default(),

crates/handlers/src/upstream_oauth2/cache.rs

@@ -278,7 +278,6 @@ pub(crate) async fn handler(
 
     let mut context = AttributeMappingContext::new();
     if let Some(id_token) = token_response.id_token.as_ref() {
-        if let Some(signed_response_alg) = &provider.id_token_signed_response_alg {
             jwks = Some(
                 mas_oidc_client::requests::jose::fetch_jwks(
                     &client,
@@ -290,7 +289,7 @@ pub(crate) async fn handler(
             let id_token_verification_data = JwtVerificationData {
                 issuer: &provider.issuer,
                 jwks: &jwks.clone().unwrap(),
-                signing_algorithm: signed_response_alg,
+                signing_algorithm: &provider.id_token_signed_response_alg,
                 client_id: &provider.client_id,
             };
 
@@ -329,11 +328,6 @@ pub(crate) async fn handler(
                 .map_err(mas_oidc_client::error::IdTokenError::from)?;
 
             context = context.with_id_token_claims(claims);
-        } else {
-            let claims = serde_json::from_str(id_token)
-                .map_err(mas_oidc_client::error::IdTokenError::from)?;
-            context = context.with_id_token_claims(claims);
-        }
     }
 
     if let Some(extra_callback_parameters) = extra_callback_parameters.clone() {

crates/handlers/src/upstream_oauth2/callback.rs

@@ -922,7 +922,7 @@ mod tests {
                     scope: Scope::from_iter([OPENID]),
                     token_endpoint_auth_method: UpstreamOAuthProviderTokenAuthMethod::None,
                     token_endpoint_signing_alg: None,
-                    id_token_signed_response_alg: None,
+                    id_token_signed_response_alg: JsonWebSignatureAlg::Rs256,
                     client_id: "client".to_owned(),
                     encrypted_client_secret: None,
                     claims_imports,

crates/handlers/src/upstream_oauth2/link.rs

@@ -346,6 +346,7 @@ mod test {
     use mas_data_model::{
         UpstreamOAuthProviderClaimsImports, UpstreamOAuthProviderTokenAuthMethod,
     };
+    use mas_iana::jose::JsonWebSignatureAlg;
     use mas_router::Route;
     use mas_storage::{
         upstream_oauth2::{UpstreamOAuthProviderParams, UpstreamOAuthProviderRepository},
@@ -403,7 +404,7 @@ mod test {
                     scope: [OPENID].into_iter().collect(),
                     token_endpoint_auth_method: UpstreamOAuthProviderTokenAuthMethod::None,
                     token_endpoint_signing_alg: None,
-                    id_token_signed_response_alg: None,
+                    id_token_signed_response_alg: JsonWebSignatureAlg::Rs256,
                     fetch_userinfo: false,
                     userinfo_signed_response_alg: None,
                     client_id: "client".to_owned(),
@@ -443,7 +444,7 @@ mod test {
                     scope: [OPENID].into_iter().collect(),
                     token_endpoint_auth_method: UpstreamOAuthProviderTokenAuthMethod::None,
                     token_endpoint_signing_alg: None,
-                    id_token_signed_response_alg: None,
+                    id_token_signed_response_alg: JsonWebSignatureAlg::Rs256,
                     fetch_userinfo: false,
                     userinfo_signed_response_alg: None,
                     client_id: "client".to_owned(),

crates/handlers/src/views/login.rs

@@ -200,10 +200,6 @@ pub enum IdTokenError {
     /// one we got before.
     #[error("wrong authentication time")]
     WrongAuthTime,
-
-    #[error(transparent)]
-    /// TODO
-    Deserialize(#[from] serde_json::Error),
 }
 
 /// All errors that can occur when adding client credentials to the request.