Don't automatically insert the openid scope on upstream providers (#4517)

Author: sandhose

crates/config/src/sections/upstream_oauth2.rs

@@ -400,6 +400,14 @@ pub struct SignInWithApple {
     pub key_id: String,
 }
 
+fn default_scope() -> String {
+    "openid".to_owned()
+}
+
+fn is_default_scope(scope: &str) -> bool {
+    scope == default_scope()
+}
+
 /// Configuration for one upstream OAuth 2 provider.
 #[skip_serializing_none]
 #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
@@ -495,6 +503,9 @@ pub struct Provider {
     pub id_token_signed_response_alg: JsonWebSignatureAlg,
 
     /// The scopes to request from the provider
+    ///
+    /// Defaults to `openid`.
+    #[serde(default = "default_scope", skip_serializing_if = "is_default_scope")]
     pub scope: String,
 
     /// How to discover the provider's configuration

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

@@ -250,7 +250,7 @@ pub struct UpstreamOAuthAuthorizationSession {
     pub provider_id: Ulid,
     pub state_str: String,
     pub code_challenge_verifier: Option<String>,
-    pub nonce: String,
+    pub nonce: Option<String>,
     pub created_at: DateTime<Utc>,
 }
 

crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs

@@ -108,14 +108,7 @@ mod tests {
         // Pretend it was linked by an authorization session
         let session = repo
             .upstream_oauth_session()
-            .add(
-                &mut rng,
-                &state.clock,
-                &provider,
-                String::new(),
-                None,
-                String::new(),
-            )
+            .add(&mut rng, &state.clock, &provider, String::new(), None, None)
             .await
             .unwrap();
 

crates/handlers/src/upstream_oauth2/callback.rs

@@ -356,10 +356,12 @@ pub(crate) async fn handler(
             )
             .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)?;
+        // Nonce must match if present.
+        if let Some(nonce) = session.nonce.as_deref() {
+            mas_jose::claims::NONCE
+                .extract_required_with_options(&mut claims, nonce)
+                .map_err(mas_oidc_client::error::IdTokenError::from)?;
+        }
 
         context = context.with_id_token_claims(claims);
     }

crates/handlers/src/upstream_oauth2/link.rs

@@ -998,7 +998,7 @@ mod tests {
                 &provider,
                 "state".to_owned(),
                 None,
-                "nonce".to_owned(),
+                None,
             )
             .await
             .unwrap();

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

@@ -191,7 +191,9 @@ pub struct AuthorizationValidationData {
     pub state: String,
 
     /// A string to mitigate replay attacks.
-    pub nonce: String,
+    /// Used when the `openid` scope is set (and therefore we are using OpenID
+    /// Connect).
+    pub nonce: Option<String>,
 
     /// The URI where the end-user will be redirected after authorization.
     pub redirect_uri: Url,
@@ -216,7 +218,7 @@ fn build_authorization_request(
 ) -> Result<(FullAuthorizationRequest, AuthorizationValidationData), AuthorizationError> {
     let AuthorizationRequestData {
         client_id,
-        mut scope,
+        scope,
         redirect_uri,
         code_challenge_methods_supported,
         display,
@@ -229,9 +231,13 @@ fn build_authorization_request(
         response_mode,
     } = authorization_data;
 
+    let is_openid = scope.contains(&OPENID);
+
     // Generate a random CSRF "state" token and a nonce.
     let state = Alphanumeric.sample_string(rng, 16);
-    let nonce = Alphanumeric.sample_string(rng, 16);
+
+    // Generate a random nonce if we're in 'OpenID Connect' mode
+    let nonce = is_openid.then(|| Alphanumeric.sample_string(rng, 16));
 
     // Use PKCE, whenever possible.
     let (pkce, code_challenge_verifier) = if code_challenge_methods_supported
@@ -255,8 +261,6 @@ fn build_authorization_request(
         (None, None)
     };
 
-    scope.insert(OPENID);
-
     let auth_request = FullAuthorizationRequest {
         inner: AuthorizationRequest {
             response_type: OAuthAuthorizationEndpointResponseType::Code.into(),
@@ -265,7 +269,7 @@ fn build_authorization_request(
             scope,
             state: Some(state.clone()),
             response_mode,
-            nonce: Some(nonce.clone()),
+            nonce: nonce.clone(),
             display,
             prompt,
             max_age,
@@ -442,10 +446,12 @@ pub async fn access_token_with_authorization_code(
             .extract_optional_with_options(&mut claims, TokenHash::new(signing_alg, &code))
             .map_err(IdTokenError::from)?;
 
-        // Nonce must match.
-        claims::NONCE
-            .extract_required_with_options(&mut claims, validation_data.nonce.as_str())
-            .map_err(IdTokenError::from)?;
+        // Nonce must match if we have one.
+        if let Some(nonce) = validation_data.nonce.as_deref() {
+            claims::NONCE
+                .extract_required_with_options(&mut claims, nonce)
+                .map_err(IdTokenError::from)?;
+        }
 
         Some(id_token.into_owned())
     } else {

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

@@ -186,7 +186,7 @@ async fn pass_access_token_with_authorization_code() {
     let redirect_uri = Url::parse(REDIRECT_URI).unwrap();
     let validation_data = AuthorizationValidationData {
         state: "some_state".to_owned(),
-        nonce: NONCE.to_owned(),
+        nonce: Some(NONCE.to_owned()),
         redirect_uri,
         code_challenge_verifier: Some(CODE_VERIFIER.to_owned()),
     };
@@ -244,7 +244,7 @@ async fn fail_access_token_with_authorization_code_wrong_nonce() {
     let redirect_uri = Url::parse(REDIRECT_URI).unwrap();
     let validation_data = AuthorizationValidationData {
         state: "some_state".to_owned(),
-        nonce: "wrong_nonce".to_owned(),
+        nonce: Some("wrong_nonce".to_owned()),
         redirect_uri,
         code_challenge_verifier: Some(CODE_VERIFIER.to_owned()),
     };
@@ -306,7 +306,7 @@ async fn fail_access_token_with_authorization_code_no_id_token() {
     let nonce = "some_nonce".to_owned();
     let validation_data = AuthorizationValidationData {
         state: "some_state".to_owned(),
-        nonce: nonce.clone(),
+        nonce: Some(nonce.clone()),
         redirect_uri,
         code_challenge_verifier: Some(CODE_VERIFIER.to_owned()),
     };

crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json

@@ -0,0 +1,8 @@
+-- Copyright 2025 New Vector Ltd.
+--
+-- SPDX-License-Identifier: AGPL-3.0-only
+-- Please see LICENSE in the repository root for full details.
+
+-- Make the nonce column optional on the upstream oauth sessions
+ALTER TABLE "upstream_oauth_authorization_sessions"
+    ALTER COLUMN "nonce" DROP NOT NULL;

crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql

@@ -108,7 +108,7 @@ mod tests {
                 &provider,
                 "some-state".to_owned(),
                 None,
-                "some-nonce".to_owned(),
+                Some("some-nonce".to_owned()),
             )
             .await
             .unwrap();