diff --git a/crates/config/src/sections/upstream_oauth2.rs b/crates/config/src/sections/upstream_oauth2.rs index cf02aaa15..aa6a27254 100644 --- a/crates/config/src/sections/upstream_oauth2.rs +++ b/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 diff --git a/crates/data-model/src/upstream_oauth2/session.rs b/crates/data-model/src/upstream_oauth2/session.rs index ef3623cfc..c5b45234c 100644 --- a/crates/data-model/src/upstream_oauth2/session.rs +++ b/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, - pub nonce: String, + pub nonce: Option, pub created_at: DateTime, } diff --git a/crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs b/crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs index 0c270c64b..403c1bc33 100644 --- a/crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs +++ b/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(); diff --git a/crates/handlers/src/upstream_oauth2/callback.rs b/crates/handlers/src/upstream_oauth2/callback.rs index f9b85adc8..75e8d63a0 100644 --- a/crates/handlers/src/upstream_oauth2/callback.rs +++ b/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); } diff --git a/crates/handlers/src/upstream_oauth2/link.rs b/crates/handlers/src/upstream_oauth2/link.rs index ce2138b2a..d95854faa 100644 --- a/crates/handlers/src/upstream_oauth2/link.rs +++ b/crates/handlers/src/upstream_oauth2/link.rs @@ -998,7 +998,7 @@ mod tests { &provider, "state".to_owned(), None, - "nonce".to_owned(), + None, ) .await .unwrap(); diff --git a/crates/oidc-client/src/requests/authorization_code.rs b/crates/oidc-client/src/requests/authorization_code.rs index 198846876..9271fe33c 100644 --- a/crates/oidc-client/src/requests/authorization_code.rs +++ b/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, /// 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 { diff --git a/crates/oidc-client/tests/it/requests/authorization_code.rs b/crates/oidc-client/tests/it/requests/authorization_code.rs index 131db26e9..d0eb2a8c1 100644 --- a/crates/oidc-client/tests/it/requests/authorization_code.rs +++ b/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()), }; diff --git a/crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json b/crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json index eac08aed7..0e28ac022 100644 --- a/crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json +++ b/crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json @@ -80,7 +80,7 @@ true, false, true, - false, + true, true, true, true, diff --git a/crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql b/crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql new file mode 100644 index 000000000..1b637c91b --- /dev/null +++ b/crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql @@ -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; diff --git a/crates/storage-pg/src/upstream_oauth2/mod.rs b/crates/storage-pg/src/upstream_oauth2/mod.rs index 2b2662df7..a5cda570b 100644 --- a/crates/storage-pg/src/upstream_oauth2/mod.rs +++ b/crates/storage-pg/src/upstream_oauth2/mod.rs @@ -108,7 +108,7 @@ mod tests { &provider, "some-state".to_owned(), None, - "some-nonce".to_owned(), + Some("some-nonce".to_owned()), ) .await .unwrap(); diff --git a/crates/storage-pg/src/upstream_oauth2/session.rs b/crates/storage-pg/src/upstream_oauth2/session.rs index d9cad86a7..594f3be4c 100644 --- a/crates/storage-pg/src/upstream_oauth2/session.rs +++ b/crates/storage-pg/src/upstream_oauth2/session.rs @@ -38,7 +38,7 @@ struct SessionLookup { upstream_oauth_link_id: Option, state: String, code_challenge_verifier: Option, - nonce: String, + nonce: Option, id_token: Option, userinfo: Option, created_at: DateTime, @@ -191,7 +191,7 @@ impl UpstreamOAuthSessionRepository for PgUpstreamOAuthSessionRepository<'_> { upstream_oauth_provider: &UpstreamOAuthProvider, state_str: String, code_challenge_verifier: Option, - nonce: String, + nonce: Option, ) -> Result { let created_at = clock.now(); let id = Ulid::from_datetime_with_source(created_at.into(), rng); diff --git a/crates/storage/src/upstream_oauth2/session.rs b/crates/storage/src/upstream_oauth2/session.rs index d87ee7d3a..c563fce5e 100644 --- a/crates/storage/src/upstream_oauth2/session.rs +++ b/crates/storage/src/upstream_oauth2/session.rs @@ -48,7 +48,7 @@ pub trait UpstreamOAuthSessionRepository: Send + Sync { /// upstream OAuth provider /// * `code_challenge_verifier`: the code challenge verifier used in this /// session, if PKCE is being used - /// * `nonce`: the `nonce` used in this session + /// * `nonce`: the `nonce` used in this session if in OIDC mode /// /// # Errors /// @@ -60,7 +60,7 @@ pub trait UpstreamOAuthSessionRepository: Send + Sync { upstream_oauth_provider: &UpstreamOAuthProvider, state: String, code_challenge_verifier: Option, - nonce: String, + nonce: Option, ) -> Result; /// Mark a session as completed and associate the given link @@ -122,7 +122,7 @@ repository_impl!(UpstreamOAuthSessionRepository: upstream_oauth_provider: &UpstreamOAuthProvider, state: String, code_challenge_verifier: Option, - nonce: String, + nonce: Option, ) -> Result; async fn complete_with_link( diff --git a/docs/config.schema.json b/docs/config.schema.json index 06fa0e768..def447302 100644 --- a/docs/config.schema.json +++ b/docs/config.schema.json @@ -1974,7 +1974,6 @@ "required": [ "client_id", "id", - "scope", "token_endpoint_auth_method" ], "properties": { @@ -2044,7 +2043,7 @@ ] }, "scope": { - "description": "The scopes to request from the provider", + "description": "The scopes to request from the provider\n\nDefaults to `openid`.", "type": "string" }, "discovery_mode": {