Address comments

Author: MatMaul

Cargo.lock

@@ -399,13 +399,25 @@ 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 +497,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 +535,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,62 +278,53 @@ 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,
-                    lazy_metadata.jwks_uri().await?,
-                )
+        jwks = Some(
+            mas_oidc_client::requests::jose::fetch_jwks(&client, lazy_metadata.jwks_uri().await?)
                 .await?,
-            );
-
-            let id_token_verification_data = JwtVerificationData {
-                issuer: &provider.issuer,
-                jwks: &jwks.clone().unwrap(),
-                signing_algorithm: signed_response_alg,
-                client_id: &provider.client_id,
-            };
-
-            // Decode and verify the ID token
-            let id_token = mas_oidc_client::requests::jose::verify_id_token(
-                id_token,
-                id_token_verification_data,
-                None,
-                clock.now(),
-            )?;
-
-            let (_headers, mut claims) = id_token.into_parts();
-
-            // Access token hash must match.
-            mas_jose::claims::AT_HASH
-                .extract_optional_with_options(
-                    &mut claims,
-                    TokenHash::new(
-                        id_token_verification_data.signing_algorithm,
-                        &token_response.access_token,
-                    ),
-                )
-                .map_err(mas_oidc_client::error::IdTokenError::from)?;
+        );
 
-            // Code hash must match.
-            mas_jose::claims::C_HASH
-                .extract_optional_with_options(
-                    &mut claims,
-                    TokenHash::new(id_token_verification_data.signing_algorithm, &code),
-                )
-                .map_err(mas_oidc_client::error::IdTokenError::from)?;
-
-            // Nonce must match.
-            mas_jose::claims::NONCE
-                .extract_required_with_options(&mut claims, session.nonce.as_str())
-                .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);
-        }
+        let id_token_verification_data = JwtVerificationData {
+            issuer: &provider.issuer,
+            jwks: &jwks.clone().unwrap(),
+            signing_algorithm: &provider.id_token_signed_response_alg,
+            client_id: &provider.client_id,
+        };
+
+        // Decode and verify the ID token
+        let id_token = mas_oidc_client::requests::jose::verify_id_token(
+            id_token,
+            id_token_verification_data,
+            None,
+            clock.now(),
+        )?;
+
+        let (_headers, mut claims) = id_token.into_parts();
+
+        // Access token hash must match.
+        mas_jose::claims::AT_HASH
+            .extract_optional_with_options(
+                &mut claims,
+                TokenHash::new(
+                    id_token_verification_data.signing_algorithm,
+                    &token_response.access_token,
+                ),
+            )
+            .map_err(mas_oidc_client::error::IdTokenError::from)?;
+
+        // Code hash must match.
+        mas_jose::claims::C_HASH
+            .extract_optional_with_options(
+                &mut claims,
+                TokenHash::new(id_token_verification_data.signing_algorithm, &code),
+            )
+            .map_err(mas_oidc_client::error::IdTokenError::from)?;
+
+        // Nonce must match.
+        mas_jose::claims::NONCE
+            .extract_required_with_options(&mut claims, session.nonce.as_str())
+            .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.