Make the id_token optional on upstream OAuth 2.0 providers

This makes it possible to use non-OIDC providers as upstream OAuth 2.0 providers, like GitHub.

Author: sandhose

crates/cli/src/sync.rs

@@ -251,7 +251,7 @@ pub async fn config_sync(
                 }
 
                 if provider.jwks_uri.is_none() {
-                    error!("Provider has discovery disabled but no JWKS URI set");
+                    warn!("Provider has discovery disabled but no JWKS URI set");
                 }
             }
 

crates/handlers/src/upstream_oauth2/callback.rs

@@ -14,10 +14,9 @@ use axum_extra::response::Html;
 use hyper::StatusCode;
 use mas_axum_utils::{cookies::CookieJar, sentry::SentryEventID};
 use mas_data_model::{UpstreamOAuthProvider, UpstreamOAuthProviderResponseMode};
+use mas_jose::claims::TokenHash;
 use mas_keystore::{Encrypter, Keystore};
-use mas_oidc_client::requests::{
-    authorization_code::AuthorizationValidationData, jose::JwtVerificationData,
-};
+use mas_oidc_client::requests::jose::JwtVerificationData;
 use mas_router::UrlBuilder;
 use mas_storage::{
     upstream_oauth2::{
@@ -27,7 +26,7 @@ use mas_storage::{
     BoxClock, BoxRepository, BoxRng, Clock,
 };
 use mas_templates::{FormPostContext, Templates};
-use oauth2_types::errors::ClientErrorCode;
+use oauth2_types::{errors::ClientErrorCode, requests::AccessTokenRequest};
 use serde::{Deserialize, Serialize};
 use serde_json::json;
 use thiserror::Error;
@@ -88,9 +87,6 @@ pub(crate) enum RouteError {
     #[error("State parameter mismatch")]
     StateMismatch,
 
-    #[error("Missing ID token")]
-    MissingIDToken,
-
     #[error("Could not extract subject from ID token")]
     ExtractSubject(#[source] minijinja::Error),
 
@@ -125,7 +121,8 @@ impl_from_error_for_route!(mas_templates::TemplateError);
 impl_from_error_for_route!(mas_storage::RepositoryError);
 impl_from_error_for_route!(mas_oidc_client::error::DiscoveryError);
 impl_from_error_for_route!(mas_oidc_client::error::JwksError);
-impl_from_error_for_route!(mas_oidc_client::error::TokenAuthorizationCodeError);
+impl_from_error_for_route!(mas_oidc_client::error::TokenRequestError);
+impl_from_error_for_route!(mas_oidc_client::error::IdTokenError);
 impl_from_error_for_route!(mas_oidc_client::error::UserInfoError);
 impl_from_error_for_route!(super::ProviderCredentialsError);
 impl_from_error_for_route!(super::cookie::UpstreamSessionNotFound);
@@ -253,11 +250,6 @@ pub(crate) async fn handler(
 
     let mut lazy_metadata = LazyProviderInfos::new(&metadata_cache, &provider, &client);
 
-    // Fetch the JWKS
-    let jwks =
-        mas_oidc_client::requests::jose::fetch_jwks(&client, lazy_metadata.jwks_uri().await?)
-            .await?;
-
     // Figure out the client credentials
     let client_credentials = client_credentials_for_provider(
         &provider,
@@ -268,41 +260,72 @@ pub(crate) async fn handler(
 
     let redirect_uri = url_builder.upstream_oauth_callback(provider.id);
 
-    // TODO: all that should be borrowed
-    let validation_data = AuthorizationValidationData {
-        state: session.state_str.clone(),
-        nonce: session.nonce.clone(),
-        code_challenge_verifier: session.code_challenge_verifier.clone(),
-        redirect_uri,
-    };
-
-    let verification_data = JwtVerificationData {
-        issuer: &provider.issuer,
-        jwks: &jwks,
-        // TODO: make that configurable
-        signing_algorithm: &mas_iana::jose::JsonWebSignatureAlg::Rs256,
-        client_id: &provider.client_id,
-    };
+    let token_response = mas_oidc_client::requests::token::request_access_token(
+        &client,
+        client_credentials,
+        lazy_metadata.token_endpoint().await?,
+        AccessTokenRequest::AuthorizationCode(oauth2_types::requests::AuthorizationCodeGrant {
+            code: code.clone(),
+            redirect_uri: Some(redirect_uri),
+            code_verifier: session.code_challenge_verifier.clone(),
+        }),
+        clock.now(),
+        &mut rng,
+    )
+    .await?;
+
+    let mut context = AttributeMappingContext::new();
+    if let Some(id_token) = token_response.id_token.as_ref() {
+        // Fetch the JWKS
+        let jwks =
+            mas_oidc_client::requests::jose::fetch_jwks(&client, lazy_metadata.jwks_uri().await?)
+                .await?;
+
+        let verification_data = JwtVerificationData {
+            issuer: &provider.issuer,
+            jwks: &jwks,
+            // TODO: make that configurable
+            signing_algorithm: &mas_iana::jose::JsonWebSignatureAlg::Rs256,
+            client_id: &provider.client_id,
+        };
 
-    let (response, id_token_map) =
-        mas_oidc_client::requests::authorization_code::access_token_with_authorization_code(
-            &client,
-            client_credentials,
-            lazy_metadata.token_endpoint().await?,
-            code,
-            validation_data,
-            Some(verification_data),
+        // Decode and verify the ID token
+        let id_token = mas_oidc_client::requests::jose::verify_id_token(
+            id_token,
+            verification_data,
+            None,
             clock.now(),
-            &mut rng,
-        )
-        .await?;
+        )?;
+
+        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(
+                    verification_data.signing_algorithm,
+                    &token_response.access_token,
+                ),
+            )
+            .map_err(mas_oidc_client::error::IdTokenError::from)?;
 
-    let (_header, id_token) = id_token_map
-        .clone()
-        .ok_or(RouteError::MissingIDToken)?
-        .into_parts();
+        // Code hash must match.
+        mas_jose::claims::C_HASH
+            .extract_optional_with_options(
+                &mut claims,
+                TokenHash::new(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);
+    }
 
-    let mut context = AttributeMappingContext::new().with_id_token_claims(id_token);
     if let Some(extra_callback_parameters) = extra_callback_parameters.clone() {
         context = context.with_extra_callback_parameters(extra_callback_parameters);
     }
@@ -312,9 +335,8 @@ pub(crate) async fn handler(
             mas_oidc_client::requests::userinfo::fetch_userinfo(
                 &client,
                 lazy_metadata.userinfo_endpoint().await?,
-                response.access_token.as_str(),
-                Some(verification_data),
-                &id_token_map.ok_or(RouteError::MissingIDToken)?,
+                token_response.access_token.as_str(),
+                None,
             )
             .await?
         ))
@@ -364,7 +386,7 @@ pub(crate) async fn handler(
             &clock,
             session,
             &link,
-            response.id_token,
+            token_response.id_token,
             extra_callback_parameters,
             userinfo,
         )

crates/oidc-client/src/requests/token.rs

@@ -7,7 +7,9 @@
 //! Requests for the Token endpoint.
 
 use chrono::{DateTime, Utc};
+use http::header::ACCEPT;
 use mas_http::RequestBuilderExt;
+use mime::APPLICATION_JSON;
 use oauth2_types::requests::{AccessTokenRequest, AccessTokenResponse};
 use rand::Rng;
 use url::Url;
@@ -48,7 +50,9 @@ pub async fn request_access_token(
 ) -> Result<AccessTokenResponse, TokenRequestError> {
     tracing::debug!(?request, "Requesting access token...");
 
-    let token_request = http_client.post(token_endpoint.as_str());
+    let token_request = http_client
+        .post(token_endpoint.as_str())
+        .header(ACCEPT, APPLICATION_JSON.as_ref());
 
     let token_response = client_credentials
         .authenticated_form(token_request, &request, now, rng)?

crates/oidc-client/src/requests/userinfo.rs

@@ -13,7 +13,6 @@ use std::collections::HashMap;
 use headers::{ContentType, HeaderMapExt, HeaderValue};
 use http::header::ACCEPT;
 use mas_http::RequestBuilderExt;
-use mas_jose::claims;
 use mime::Mime;
 use serde_json::Value;
 use url::Url;
@@ -22,7 +21,6 @@ use super::jose::JwtVerificationData;
 use crate::{
     error::{IdTokenError, ResponseExt, UserInfoError},
     requests::jose::verify_signed_jwt,
-    types::IdToken,
 };
 
 /// Obtain information about an authenticated end-user.
@@ -59,7 +57,6 @@ pub async fn fetch_userinfo(
     userinfo_endpoint: &Url,
     access_token: &str,
     jwt_verification_data: Option<JwtVerificationData<'_>>,
-    auth_id_token: &IdToken<'_>,
 ) -> Result<HashMap<String, Value>, UserInfoError> {
     tracing::debug!("Obtaining user info…");
 
@@ -94,7 +91,7 @@ pub async fn fetch_userinfo(
         });
     }
 
-    let mut claims = if let Some(verification_data) = jwt_verification_data {
+    let claims = if let Some(verification_data) = jwt_verification_data {
         let response_body = userinfo_response.text().await?;
         verify_signed_jwt(&response_body, verification_data)
             .map_err(IdTokenError::from)?
@@ -104,18 +101,5 @@ pub async fn fetch_userinfo(
         userinfo_response.json().await?
     };
 
-    let mut auth_claims = auth_id_token.payload().clone();
-
-    // Subject identifier must always be the same.
-    let sub = claims::SUB
-        .extract_required(&mut claims)
-        .map_err(IdTokenError::from)?;
-    let auth_sub = claims::SUB
-        .extract_required(&mut auth_claims)
-        .map_err(IdTokenError::from)?;
-    if sub != auth_sub {
-        return Err(IdTokenError::WrongSubjectIdentifier.into());
-    }
-
     Ok(claims)
 }

crates/oidc-client/tests/it/requests/userinfo.rs

@@ -4,24 +4,19 @@
 // SPDX-License-Identifier: AGPL-3.0-only
 // Please see LICENSE in the repository root for full details.
 
-use assert_matches::assert_matches;
-use mas_oidc_client::{
-    error::{IdTokenError, UserInfoError},
-    requests::userinfo::fetch_userinfo,
-};
+use mas_oidc_client::requests::userinfo::fetch_userinfo;
 use serde_json::json;
 use wiremock::{
     matchers::{header, method, path},
     Mock, ResponseTemplate,
 };
 
-use crate::{id_token, init_test, ACCESS_TOKEN, SUBJECT_IDENTIFIER};
+use crate::{init_test, ACCESS_TOKEN, SUBJECT_IDENTIFIER};
 
 #[tokio::test]
 async fn pass_fetch_userinfo() {
     let (http_client, mock_server, issuer) = init_test().await;
     let userinfo_endpoint = issuer.join("userinfo").unwrap();
-    let (auth_id_token, _) = id_token(issuer.as_str());
 
     Mock::given(method("GET"))
         .and(path("/userinfo"))
@@ -36,50 +31,9 @@ async fn pass_fetch_userinfo() {
         .mount(&mock_server)
         .await;
 
-    let claims = fetch_userinfo(
-        &http_client,
-        &userinfo_endpoint,
-        ACCESS_TOKEN,
-        None,
-        &auth_id_token,
-    )
-    .await
-    .unwrap();
+    let claims = fetch_userinfo(&http_client, &userinfo_endpoint, ACCESS_TOKEN, None)
+        .await
+        .unwrap();
 
     assert_eq!(claims.get("email").unwrap(), "[email protected]");
 }
-
-#[tokio::test]
-async fn fail_wrong_subject_identifier() {
-    let (http_client, mock_server, issuer) = init_test().await;
-    let userinfo_endpoint = issuer.join("userinfo").unwrap();
-    let (auth_id_token, _) = id_token(issuer.as_str());
-
-    Mock::given(method("GET"))
-        .and(path("/userinfo"))
-        .and(header(
-            "authorization",
-            format!("Bearer {ACCESS_TOKEN}").as_str(),
-        ))
-        .respond_with(ResponseTemplate::new(200).set_body_json(json!({
-            "sub": "wrong_subject_identifier",
-            "email": "[email protected]",
-        })))
-        .mount(&mock_server)
-        .await;
-
-    let error = fetch_userinfo(
-        &http_client,
-        &userinfo_endpoint,
-        ACCESS_TOKEN,
-        None,
-        &auth_id_token,
-    )
-    .await
-    .unwrap_err();
-
-    assert_matches!(
-        error,
-        UserInfoError::IdToken(IdTokenError::WrongSubjectIdentifier)
-    );
-}