From f0404e73c6c1ebc488935d0a5f989ad33411d3e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20W=C3=B3jcik?= Date: Mon, 17 Aug 2026 08:24:58 +0200 Subject: [PATCH 1/6] fix potential race in proxy test --- .../src/tests/proxy_manager/handler/lifecycle.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/lifecycle.rs b/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/lifecycle.rs index 214841042..6086f46e7 100644 --- a/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/lifecycle.rs +++ b/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/lifecycle.rs @@ -8,10 +8,13 @@ use crate::tests::common::{HandlerTestContext, reload_proxy}; async fn test_proxy_marked_connected_after_handshake(_: PgPoolOptions, options: PgConnectOptions) { let mut context = HandlerTestContext::new(options).await; - let proxy_before = context.reload_proxy().await; - // Proxy not yet connected: connected_at must be None or older than disconnected_at. + // Proxy row as created, snapshotted before the handler task was spawned: + // connected_at must be None or older than disconnected_at. Re-reading the + // row from the database here would race the handler's mark_connected() + // write, which happens as soon as the bidi stream is established - before + // the InitialInfo message that complete_proxy_handshake() waits for. assert!( - !proxy_before.is_connected(), + !context.proxy.is_connected(), "proxy should not be connected before handshake" ); From 0f76a9dc4bc73ad24511ce228f34c467ea430c5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20W=C3=B3jcik?= Date: Mon, 17 Aug 2026 10:01:41 +0200 Subject: [PATCH 2/6] display non-default values in enrollment email --- .../src/enrollment_management.rs | 4 + crates/defguard_core/src/mail/mod.rs | 7 ++ crates/defguard_core/src/mail/templates.rs | 112 +++++++++++++++++- crates/defguard_core/src/mail/tests.rs | 59 +++++++++ ...[2.1.0]_enrollment_email_timeouts.down.sql | 3 + ...9_[2.1.0]_enrollment_email_timeouts.up.sql | 5 + tools/defguard_generator/src/activity_log.rs | 4 +- 7 files changed, 188 insertions(+), 6 deletions(-) create mode 100644 migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.down.sql create mode 100644 migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.up.sql diff --git a/crates/defguard_core/src/enrollment_management.rs b/crates/defguard_core/src/enrollment_management.rs index b3e998b4a..946c3fe5d 100644 --- a/crates/defguard_core/src/enrollment_management.rs +++ b/crates/defguard_core/src/enrollment_management.rs @@ -1,3 +1,5 @@ +use std::time::Duration; + use defguard_common::db::{ Id, models::{Settings, user::User}, @@ -61,6 +63,7 @@ pub async fn start_user_enrollment( base_message_context, enrollment_service_url, &enrollment.id, + Duration::from_secs(token_timeout_seconds), ) .await; match result { @@ -200,6 +203,7 @@ pub async fn send_enrollment_invitation( base_message_context, enrollment_service_url, token_id, + Duration::from_secs((token.expires_at - token.created_at).num_seconds().max(0) as u64), ) .await { diff --git a/crates/defguard_core/src/mail/mod.rs b/crates/defguard_core/src/mail/mod.rs index 5064f6a7c..3f13b8330 100644 --- a/crates/defguard_core/src/mail/mod.rs +++ b/crates/defguard_core/src/mail/mod.rs @@ -152,6 +152,13 @@ impl Mail { &self.subject } + /// Getter for the plain text body. Used by tests to assert rendered content. + #[cfg(test)] + #[must_use] + pub(crate) fn text(&self) -> &str { + &self.text + } + /// Add to context. pub fn add_to_context(&mut self, key: K, value: &V) where diff --git a/crates/defguard_core/src/mail/templates.rs b/crates/defguard_core/src/mail/templates.rs index 2866ec331..0bdcc2823 100644 --- a/crates/defguard_core/src/mail/templates.rs +++ b/crates/defguard_core/src/mail/templates.rs @@ -15,7 +15,7 @@ use tera::{Context, Function, Tera}; use thiserror::Error; use tracing::{debug, warn}; -use super::{Attachment, MailError, MailMessage}; +use super::{Attachment, Mail, MailError, MailMessage}; pub(crate) const DEFAULT_LANG: &str = "en_US"; @@ -144,9 +144,33 @@ pub async fn new_account_mail( to: &str, conn: &mut PgConnection, context: Context, - mut enrollment_service_url: Url, + enrollment_service_url: Url, enrollment_token: &str, + token_timeout: Duration, ) -> Result<(), TemplateError> { + build_new_account_mail( + to, + conn, + context, + enrollment_service_url, + enrollment_token, + token_timeout, + ) + .await? + .send_and_forget(); + Ok(()) +} + +/// Build (but do not send) the enrollment start mail. Extracted so tests can assert the +/// rendered content without requiring an SMTP server. +pub(crate) async fn build_new_account_mail( + to: &str, + conn: &mut PgConnection, + context: Context, + mut enrollment_service_url: Url, + enrollment_token: &str, + token_timeout: Duration, +) -> Result { debug!("Render an enrollment start mail template for the user."); let (mut tera, mut context) = get_base_tera_mjml(context, None, None, None)?; @@ -166,9 +190,49 @@ pub async fn new_account_mail( let message = MailMessage::NewAccount; message.fill_context(conn, &mut context).await?; - message.mail(&mut tera, &context, to)?.send_and_forget(); - Ok(()) + // Inject the effective token/session timeouts so the email reflects the configured values + // instead of the hardcoded defaults. See https://github.com/DefGuard/defguard/issues/3518. + let settings = Settings::get_current_settings(); + context.insert("token_timeout", &format_timeout(token_timeout)); + context.insert( + "session_timeout", + &format_timeout(settings.enrollment_session_timeout()), + ); + + // The `token_info` section is admin-configurable text; render it as a template so it can + // reference the `token_timeout` / `session_timeout` variables above. + if let Some(Value::String(token_info)) = context.get("token_info").cloned() { + let mut info_tera = safe_tera(); + info_tera.add_raw_template("token_info", &token_info)?; + let rendered = info_tera.render("token_info", &context)?; + context.insert("token_info", &rendered); + } + + message.mail(&mut tera, &context, to) +} + +/// Format a timeout duration as a short human-readable string, e.g. "1 week", "1 day", +/// "24 hours", or "30 minutes". +fn format_timeout(duration: Duration) -> String { + let secs = duration.as_secs(); + let minute = 60; + let hour = 60 * minute; + let day = 24 * hour; + let week = 7 * day; + + let (value, unit) = if secs >= week && secs.is_multiple_of(week) { + (secs / week, "week") + } else if secs >= day && secs.is_multiple_of(day) { + (secs / day, "day") + } else if secs >= hour && secs.is_multiple_of(hour) { + (secs / hour, "hour") + } else if secs >= minute && secs.is_multiple_of(minute) { + (secs / minute, "minute") + } else { + (secs, "second") + }; + format!("{value} {unit}{}", if value == 1 { "" } else { "s" }) } // Mail with link to enrollment service. @@ -687,3 +751,43 @@ pub async fn certificate_expired_mail( Ok(()) } + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use super::format_timeout; + + #[test] + fn formats_weeks() { + assert_eq!(format_timeout(Duration::from_secs(7 * 24 * 3600)), "1 week"); + assert_eq!( + format_timeout(Duration::from_secs(14 * 24 * 3600)), + "2 weeks" + ); + } + + #[test] + fn formats_days() { + assert_eq!(format_timeout(Duration::from_secs(24 * 3600)), "1 day"); + assert_eq!(format_timeout(Duration::from_secs(2 * 24 * 3600)), "2 days"); + } + + #[test] + fn formats_hours() { + assert_eq!(format_timeout(Duration::from_secs(3600)), "1 hour"); + assert_eq!(format_timeout(Duration::from_secs(23 * 3600)), "23 hours"); + } + + #[test] + fn formats_minutes() { + assert_eq!(format_timeout(Duration::from_secs(60)), "1 minute"); + assert_eq!(format_timeout(Duration::from_secs(30 * 60)), "30 minutes"); + } + + #[test] + fn formats_seconds() { + assert_eq!(format_timeout(Duration::from_secs(1)), "1 second"); + assert_eq!(format_timeout(Duration::from_secs(90)), "90 seconds"); + } +} diff --git a/crates/defguard_core/src/mail/tests.rs b/crates/defguard_core/src/mail/tests.rs index 11e56a1e3..29e9f9f7e 100644 --- a/crates/defguard_core/src/mail/tests.rs +++ b/crates/defguard_core/src/mail/tests.rs @@ -33,6 +33,64 @@ fn dg25_8_server_side_template_injection() { assert!(tera.render("text", &Context::new()).is_err()); } +/// Override the enrollment token/session timeouts and reload the global settings. +async fn set_enrollment_timeouts(pool: &PgPool, token_hours: i32, session_minutes: i32) { + sqlx::query( + "UPDATE settings \ + SET enrollment_token_timeout_hours = $1, \ + enrollment_session_timeout_minutes = $2", + ) + .bind(token_hours) + .bind(session_minutes) + .execute(pool) + .await + .unwrap(); + + initialize_current_settings(pool).await.unwrap(); +} + +/// Regression test for https://github.com/DefGuard/defguard/issues/3518 +/// +/// The enrollment email must reflect the configured enrollment token and session timeouts +/// instead of the hardcoded defaults ("24 hours" / "10 minutes"). +#[sqlx::test] +async fn enrollment_email_reflects_configured_timeouts( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let pool = setup_pool(options).await; + initialize_current_settings(&pool).await.unwrap(); + + // Configure non-default timeouts: token valid for 1 week, session for 30 minutes. + set_enrollment_timeouts(&pool, 168, 30).await; + + let mut conn = pool.begin().await.unwrap(); + let url = Url::parse("http://localhost:8001").unwrap(); + let context = Context::new(); + let token = "zXc6N1ndXpWFeyBuogiFp1bD1UomAbZc"; + + let mail = templates::build_new_account_mail( + "user@example.com", + &mut conn, + context, + url, + token, + Duration::from_secs(168 * 3600), + ) + .await + .unwrap(); + + let text = mail.text(); + assert!( + text.contains("1 week"), + "enrollment email should show the configured token timeout, got: {text}" + ); + assert!( + text.contains("30 minutes"), + "enrollment email should show the configured session timeout, got: {text}" + ); +} + /// Delay, so send_and_forget() can process the message. async fn delay() { sleep(Duration::from_secs(2)).await; @@ -163,6 +221,7 @@ fn send_new_account(_: PgPoolOptions, options: PgConnectOptions) { context, url, token, + Duration::from_secs(24 * 3600), ) .await .unwrap(); diff --git a/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.down.sql b/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.down.sql new file mode 100644 index 000000000..f14e1e2b5 --- /dev/null +++ b/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.down.sql @@ -0,0 +1,3 @@ +UPDATE mail_context +SET text = 'The token is valid for 24 hours. Once the enrollment process starts, you have 10 minutes to complete it.' +WHERE template = 'new-account' AND section = 'token_info' AND language_tag = 'en_US'; diff --git a/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.up.sql b/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.up.sql new file mode 100644 index 000000000..9cc2553a7 --- /dev/null +++ b/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.up.sql @@ -0,0 +1,5 @@ +-- Render the enrollment token/session timeouts dynamically in the "new-account" email. +-- See https://github.com/DefGuard/defguard/issues/3518 +UPDATE mail_context +SET text = 'The token is valid for {{ token_timeout }}. Once the enrollment process starts, you have {{ session_timeout }} to complete it.' +WHERE template = 'new-account' AND section = 'token_info' AND language_tag = 'en_US'; diff --git a/tools/defguard_generator/src/activity_log.rs b/tools/defguard_generator/src/activity_log.rs index e0b8bca37..d1ed2b5c0 100644 --- a/tools/defguard_generator/src/activity_log.rs +++ b/tools/defguard_generator/src/activity_log.rs @@ -25,7 +25,7 @@ use defguard_event_logger::description::{ }; use rand::{Rng, rngs::ThreadRng, seq::SliceRandom}; -#[allow(dead_code)] +#[allow(dead_code, clippy::large_enum_variant)] enum DefguardEvent { UserLogin, UserLoginFailed { @@ -244,7 +244,7 @@ enum VpnEvent { }, } -#[allow(dead_code)] +#[allow(dead_code, clippy::large_enum_variant)] enum EnrollmentEvent { EnrollmentStarted, EnrollmentDeviceAdded { device: Device }, From 63616205ecb1e5ec50e467a4a4c2de5ddfb6884e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20W=C3=B3jcik?= Date: Mon, 17 Aug 2026 11:54:06 +0200 Subject: [PATCH 3/6] update UI submodule --- web/src/shared/defguard-ui | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/shared/defguard-ui b/web/src/shared/defguard-ui index b73914b9b..481b2e323 160000 --- a/web/src/shared/defguard-ui +++ b/web/src/shared/defguard-ui @@ -1 +1 @@ -Subproject commit b73914b9b32ded9a4e248def32a5b7fbe87708c3 +Subproject commit 481b2e323f22ff45908c2ffa2de6ccf52620da4b From 08d464c4daab51522afac07bfc0bcae7355f82c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20W=C3=B3jcik?= Date: Mon, 17 Aug 2026 12:17:15 +0200 Subject: [PATCH 4/6] fix provider edit endpoint --- .../enterprise/handlers/openid_providers.rs | 1 + .../tests/integration/api/openid_login.rs | 76 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/crates/defguard_core/src/enterprise/handlers/openid_providers.rs b/crates/defguard_core/src/enterprise/handlers/openid_providers.rs index 116030ff3..d4b258d9c 100644 --- a/crates/defguard_core/src/enterprise/handlers/openid_providers.rs +++ b/crates/defguard_core/src/enterprise/handlers/openid_providers.rs @@ -456,6 +456,7 @@ pub(crate) async fn modify_openid_provider( provider.directory_sync_group_match = group_match; provider.jumpcloud_api_key = provider_data.jumpcloud_api_key; provider.prefetch_users = provider_data.prefetch_users; + provider.disable_password_management = provider_data.disable_password_management; provider.directory_sync_user_groups = user_groups; provider.save(&mut *transaction).await?; transaction.commit().await?; diff --git a/crates/defguard_core/tests/integration/api/openid_login.rs b/crates/defguard_core/tests/integration/api/openid_login.rs index 40280fd17..32d6bef1a 100644 --- a/crates/defguard_core/tests/integration/api/openid_login.rs +++ b/crates/defguard_core/tests/integration/api/openid_login.rs @@ -25,6 +25,16 @@ struct UrlResponse { url: String, } +#[derive(Deserialize)] +struct CurrentProviderResponse { + provider: CurrentProvider, +} + +#[derive(Deserialize)] +struct CurrentProvider { + disable_password_management: bool, +} + #[sqlx::test] async fn test_openid_providers(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await; @@ -110,6 +120,72 @@ async fn test_openid_providers(_: PgPoolOptions, options: PgConnectOptions) { assert_eq!(response.status(), StatusCode::FORBIDDEN); } +#[sqlx::test] +async fn test_modify_openid_provider_persists_disable_password_management( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let pool = setup_pool(options).await; + let client = make_client(pool).await; + + let auth = Auth::new("admin", "pass123"); + let response = client.post("/api/v1/auth").json(&auth).send().await; + assert_eq!(response.status(), StatusCode::OK); + + exceed_enterprise_limits(&client).await; + + let mut provider_data = AddProviderData { + name: "test".to_owned(), + base_url: "https://accounts.google.com".to_owned(), + kind: OpenIdProviderKind::Google, + client_id: "client_id".to_owned(), + client_secret: "client_secret".to_owned(), + display_name: Some("display_name".to_owned()), + admin_email: None, + google_service_account_email: None, + google_service_account_key: None, + directory_sync_enabled: false, + directory_sync_interval: 100, + directory_sync_user_behavior: DirectorySyncUserBehavior::Keep.to_string(), + directory_sync_admin_behavior: DirectorySyncUserBehavior::Keep.to_string(), + directory_sync_target: DirectorySyncTarget::All.to_string(), + create_account: false, + okta_dirsync_client_id: None, + okta_private_jwk: None, + directory_sync_group_match: None, + username_handling: OpenIdUsernameHandling::PruneEmailDomain, + jumpcloud_api_key: None, + prefetch_users: false, + disable_password_management: false, + directory_sync_user_groups: None, + }; + + let response = client + .post("/api/v1/openid/provider") + .json(&provider_data) + .send() + .await; + assert_eq!(response.status(), StatusCode::CREATED); + + // Toggle the flag and update the provider via PUT. + provider_data.disable_password_management = true; + let response = client + .put("/api/v1/openid/provider/test") + .json(&provider_data) + .send() + .await; + assert_eq!(response.status(), StatusCode::OK); + + // Read back the current provider and assert the flag was persisted. + let response = client.get("/api/v1/openid/provider/current").send().await; + assert_eq!(response.status(), StatusCode::OK); + let body: CurrentProviderResponse = response.json().await; + assert!( + body.provider.disable_password_management, + "disable_password_management should be persisted as true after update" + ); +} + // FIXME: this test sometimes fails because of test_openid_providers. // The license state is possibly preserved between those two. This requires further research. #[sqlx::test] From 3daeaf70514b9a6ab1995cd7c5901a0c140d1a88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20W=C3=B3jcik?= Date: Mon, 17 Aug 2026 13:10:35 +0200 Subject: [PATCH 5/6] fix sending gw disconnect notifications --- .../defguard_gateway_manager/src/handler.rs | 203 +++++++++++++----- crates/defguard_gateway_manager/src/lib.rs | 95 ++++++++ .../src/tests/common/mod.rs | 83 ++++++- .../src/tests/gateway_manager/manager.rs | 26 +-- .../src/tests/gateway_manager/mod.rs | 1 + .../tests/gateway_manager/notifications.rs | 172 +++++++++++++++ 6 files changed, 508 insertions(+), 72 deletions(-) create mode 100644 crates/defguard_gateway_manager/src/tests/gateway_manager/notifications.rs diff --git a/crates/defguard_gateway_manager/src/handler.rs b/crates/defguard_gateway_manager/src/handler.rs index 8c9a89d6a..6e912c41e 100644 --- a/crates/defguard_gateway_manager/src/handler.rs +++ b/crates/defguard_gateway_manager/src/handler.rs @@ -6,12 +6,12 @@ use std::{ str::FromStr, sync::{ Arc, Mutex, - atomic::{AtomicU64, Ordering}, + atomic::{AtomicBool, AtomicU64, Ordering}, }, time::Duration, }; -use chrono::{DateTime, TimeDelta}; +use chrono::DateTime; use defguard_common::{ VERSION, db::{ @@ -94,6 +94,12 @@ pub(crate) struct GatewayHandler { peer_stats_tx: UnboundedSender, certs_rx: watch::Receiver>>, updates_handler_handle: Option>, + /// Disconnect email notification waiting out the inactivity threshold. Aborted when the + /// Gateway reconnects inside the window, so short outages produce no email at all. + pending_disconnect_notification: Option>, + /// Set by the pending task just before it sends the disconnect email. Guarantees a + /// reconnect email is sent if and only if a disconnect email went out for that outage. + disconnect_notification_sent: Arc, #[cfg(test)] test_transport: GatewayTestTransport, #[cfg(test)] @@ -126,6 +132,8 @@ impl GatewayHandler { peer_stats_tx, certs_rx, updates_handler_handle: None, + pending_disconnect_notification: None, + disconnect_notification_sent: Arc::new(AtomicBool::new(false)), #[cfg(test)] test_transport: GatewayTestTransport::default(), #[cfg(test)] @@ -138,6 +146,11 @@ impl GatewayHandler { TEN_SECS } + #[cfg(not(test))] + fn disconnect_notification_delay(&self, configured_delay: Duration) -> Duration { + configured_delay + } + #[cfg(not(test))] async fn connect_channel(&self, endpoint: &Endpoint) -> Result { self.connect_tls_channel(endpoint).await @@ -260,69 +273,121 @@ impl GatewayHandler { } } - /// Send Gateway disconnected notification. - /// Sends notification only if last notification time is bigger than specified in config. - async fn send_disconnect_notification(&self) { + /// Schedule a Gateway disconnected email notification. + /// + /// The email is delayed by the configured inactivity threshold instead of being sent + /// straight away. A reconnect inside that window aborts the pending task, so a short + /// outage produces no notification at all. + fn schedule_disconnect_notification(&mut self) { let settings = Settings::get_current_settings(); if !settings.gateway_disconnect_notifications_enabled { return; } - // Send email only if disconnection time is before the connection time. - if let (Some(connected_at), Some(disconnected_at)) = - (self.gateway.connected_at, self.gateway.disconnected_at) - && disconnected_at > connected_at - { - info!("{} disconnected; email notification not sent", self.gateway); - return; + if let Some(handle) = self.pending_disconnect_notification.take() { + warn!( + "Found a disconnect email notification already pending for {} while scheduling a \ + new one; aborting the old one", + self.gateway + ); + handle.abort(); } - debug!("Sending Gateway disconnect email notification"); - let name = match Gateway::find_by_id(&self.pool, self.gateway.id).await { - Ok(Some(gateway)) => gateway.name, - _ => self.gateway.name.clone(), - }; - let pool = self.pool.clone(); + // A threshold of 0 keeps the notification immediate, which is what the settings form + // allows as its minimum value. + let threshold_minutes = + u64::try_from(settings.gateway_disconnect_notifications_inactivity_threshold) + .unwrap_or_default(); + let delay = self.disconnect_notification_delay(Duration::from_secs(60 * threshold_minutes)); + + let gateway_id = self.gateway.id; + let location_id = self.gateway.location_id; let url = format!("{}:{}", self.gateway.address, self.gateway.port); + let pool = self.pool.clone(); + let notification_sent = Arc::clone(&self.disconnect_notification_sent); + #[cfg(test)] + let test_support = self.test_support.clone(); - let Ok(Some(network)) = - WireguardNetwork::find_by_id(&self.pool, self.gateway.location_id).await - else { - error!( - "Failed to fetch network ID {} from database", - self.gateway.location_id - ); - return; - }; + debug!( + "Scheduling Gateway disconnect email notification for {} in {delay:?}", + self.gateway + ); + let handle = tokio::spawn(async move { + sleep(delay).await; - // TODO: return result instead of logging. - if let Err(err) = send_gateway_disconnected_email(name, network.name, &url, &pool).await { - error!("Failed to send Gateway disconnect notification: {err}"); - } else { - info!("Sent email notification about Gateway being disconnected"); - } + // Re-read the current state, since the Gateway may have been removed from the + // database or reconnected without this task being aborted in time. + let gateway = match Gateway::find_by_id(&pool, gateway_id).await { + Ok(Some(gateway)) => gateway, + Ok(None) => { + info!( + "Gateway id={gateway_id} is no longer in the database; disconnect email \ + notification not sent" + ); + return; + } + Err(err) => { + error!( + "Failed to fetch Gateway id={gateway_id} from database, disconnect email \ + notification not sent: {err}" + ); + return; + } + }; + if gateway.is_connected() { + info!( + "{gateway} reconnected within the inactivity threshold; disconnect email \ + notification not sent" + ); + return; + } + + let Ok(Some(network)) = WireguardNetwork::find_by_id(&pool, location_id).await else { + error!("Failed to fetch network ID {location_id} from database"); + return; + }; + + // Record the notification as sent before awaiting the send, so an abort landing + // mid-send cannot leave a later reconnect notification unpaired. + notification_sent.store(true, Ordering::SeqCst); + #[cfg(test)] + note_disconnect_notification_for_tests(test_support.as_ref(), gateway_id); + + debug!("Sending Gateway disconnect email notification"); + // TODO: return result instead of logging. + if let Err(err) = + send_gateway_disconnected_email(gateway.name, network.name, &url, &pool).await + { + error!("Failed to send Gateway disconnect notification: {err}"); + } else { + info!("Sent email notification about Gateway being disconnected"); + } + }); + self.pending_disconnect_notification = Some(handle); } /// Send Gateway reconnected notification. + /// + /// Only sent when a disconnect notification actually went out for this outage, so admins + /// never receive a reconnect email without a matching disconnect email. fn send_reconnect_notification(&self, network_name: String) { - let settings = Settings::get_current_settings(); - if !settings.gateway_disconnect_notifications_reconnect_notification_enabled { + // Always clear the flag, even when reconnect notifications are turned off, so that a + // later outage cannot inherit it. + if !self + .disconnect_notification_sent + .swap(false, Ordering::SeqCst) + { return; } - let (Some(connected_at), Some(disconnected_at)) = - (self.gateway.connected_at, self.gateway.disconnected_at) - else { - return; - }; - let inactivity_threshold = TimeDelta::minutes(i64::from( - settings.gateway_disconnect_notifications_inactivity_threshold, - )); - if connected_at - disconnected_at <= inactivity_threshold { + let settings = Settings::get_current_settings(); + if !settings.gateway_disconnect_notifications_reconnect_notification_enabled { return; } debug!("Sending Gateway reconnect email notification"); + #[cfg(test)] + self.note_reconnect_notification_for_tests(); let gateway_id = self.gateway.id; let fallback_name = self.gateway.name.clone(); let pool = self.pool.clone(); @@ -355,12 +420,13 @@ impl GatewayHandler { } async fn handle_disconnection_error(&mut self) { - let was_connected = self.gateway.is_connected(); - if self.gateway.is_connected() { - self.send_disconnect_notification().await; + if !self.gateway.is_connected() { + return; } - if was_connected && self.mark_disconnected().await { + // Mark the Gateway disconnected before scheduling the notification: the delayed task + // re-reads the row when it fires and must not see a stale connected state. + if self.mark_disconnected().await { let _ = self .connection_events_tx .send(GatewayConnectionEvent::Disconnected { @@ -368,6 +434,8 @@ impl GatewayHandler { gateway_name: self.gateway.name.clone(), }); } + + self.schedule_disconnect_notification(); } async fn mark_connected_and_maybe_notify(&mut self, network_name: &str) { @@ -389,6 +457,15 @@ impl GatewayHandler { }); } + // A Gateway that came back this quickly should not be reported as down at all. + if let Some(handle) = self.pending_disconnect_notification.take() { + debug!( + "Cancelling pending disconnect email notification for {}", + self.gateway + ); + handle.abort(); + } + self.send_reconnect_notification(network_name.to_owned()); } @@ -588,6 +665,22 @@ impl Drop for GatewayHandler { if let Some(handle) = self.updates_handler_handle.take() { handle.abort(); } + if let Some(handle) = self.pending_disconnect_notification.take() { + handle.abort(); + } + } +} + +/// Records that a disconnect email notification was sent for the given Gateway. +/// A free function because the pending notification task only owns a clone of the test support, +/// not the handler itself. +#[cfg(test)] +fn note_disconnect_notification_for_tests( + test_support: Option<&GatewayManagerTestSupport>, + gateway_id: Id, +) { + if let Some(test_support) = test_support { + test_support.note_disconnect_notification_sent(gateway_id); } } @@ -624,12 +717,26 @@ impl GatewayHandler { } } + fn note_reconnect_notification_for_tests(&self) { + if let Some(test_support) = &self.test_support { + test_support.note_reconnect_notification_sent(self.gateway.id); + } + } + fn handler_retry_delay(&self) -> Duration { self.test_support .as_ref() .map_or(TEN_SECS, GatewayManagerTestSupport::handler_reconnect_delay) } + fn disconnect_notification_delay(&self, configured_delay: Duration) -> Duration { + self.test_support + .as_ref() + .map_or(configured_delay, |test_support| { + test_support.disconnect_notification_delay(configured_delay) + }) + } + async fn connect_channel(&self, endpoint: &Endpoint) -> Result { if let Some(socket_path) = self.test_transport.socket_path().cloned() { return Ok(endpoint.connect_with_connector_lazy(tower::service_fn( diff --git a/crates/defguard_gateway_manager/src/lib.rs b/crates/defguard_gateway_manager/src/lib.rs index 0ea94a548..6619421b0 100644 --- a/crates/defguard_gateway_manager/src/lib.rs +++ b/crates/defguard_gateway_manager/src/lib.rs @@ -79,9 +79,14 @@ struct GatewayManagerTestSupport { handler_connection_attempt_notify: Arc, gateway_notifications_by_gateway: Arc>>, gateway_notification_notify: Arc, + disconnect_notifications_by_gateway: Arc>>, + disconnect_notification_notify: Arc, + reconnect_notifications_by_gateway: Arc>>, + reconnect_notification_notify: Arc, listener_ready: Arc, listener_ready_notify: Arc, retry_delay_override: Arc>>, + disconnect_notification_delay_override: Arc>>, } #[cfg(test)] @@ -206,6 +211,79 @@ impl GatewayManagerTestSupport { } } + /// Records that a Gateway disconnect email notification was actually sent. Tests use this + /// instead of a mock SMTP server, because mail delivery itself is fire-and-forget. + fn note_disconnect_notification_sent(&self, gateway_id: Id) { + let mut disconnect_notifications = self + .disconnect_notifications_by_gateway + .lock() + .expect("Failed to lock GatewayManager disconnect notification registry"); + *disconnect_notifications.entry(gateway_id).or_default() += 1; + self.disconnect_notification_notify.notify_waiters(); + } + + #[cfg(test)] + fn disconnect_notification_count(&self, gateway_id: Id) -> u64 { + self.disconnect_notifications_by_gateway + .lock() + .expect("Failed to lock GatewayManager disconnect notification registry") + .get(&gateway_id) + .copied() + .unwrap_or_default() + } + + #[cfg(test)] + async fn wait_for_disconnect_notification_count(&self, gateway_id: Id, expected_count: u64) { + loop { + if self.disconnect_notification_count(gateway_id) >= expected_count { + return; + } + + let notified = self.disconnect_notification_notify.notified(); + if self.disconnect_notification_count(gateway_id) >= expected_count { + return; + } + + notified.await; + } + } + + /// Records that a Gateway reconnect email notification was actually sent. + fn note_reconnect_notification_sent(&self, gateway_id: Id) { + let mut reconnect_notifications = self + .reconnect_notifications_by_gateway + .lock() + .expect("Failed to lock GatewayManager reconnect notification registry"); + *reconnect_notifications.entry(gateway_id).or_default() += 1; + self.reconnect_notification_notify.notify_waiters(); + } + + #[cfg(test)] + fn reconnect_notification_count(&self, gateway_id: Id) -> u64 { + self.reconnect_notifications_by_gateway + .lock() + .expect("Failed to lock GatewayManager reconnect notification registry") + .get(&gateway_id) + .copied() + .unwrap_or_default() + } + + #[cfg(test)] + async fn wait_for_reconnect_notification_count(&self, gateway_id: Id, expected_count: u64) { + loop { + if self.reconnect_notification_count(gateway_id) >= expected_count { + return; + } + + let notified = self.reconnect_notification_notify.notified(); + if self.reconnect_notification_count(gateway_id) >= expected_count { + return; + } + + notified.await; + } + } + fn mark_listener_ready(&self) { self.listener_ready.store(true, Ordering::Release); self.listener_ready_notify.notify_waiters(); @@ -248,6 +326,23 @@ impl GatewayManagerTestSupport { .expect("Failed to lock GatewayManager retry delay override") .unwrap_or(TEN_SECS) } + + /// Overrides the inactivity threshold delay so tests do not have to wait out whole minutes. + #[cfg(test)] + fn set_disconnect_notification_delay(&self, delay: Duration) { + *self + .disconnect_notification_delay_override + .lock() + .expect("Failed to lock GatewayManager disconnect notification delay override") = + Some(delay); + } + + fn disconnect_notification_delay(&self, configured_delay: Duration) -> Duration { + self.disconnect_notification_delay_override + .lock() + .expect("Failed to lock GatewayManager disconnect notification delay override") + .unwrap_or(configured_delay) + } } pub struct GatewayManager { diff --git a/crates/defguard_gateway_manager/src/tests/common/mod.rs b/crates/defguard_gateway_manager/src/tests/common/mod.rs index dd9436d7c..eae01623f 100644 --- a/crates/defguard_gateway_manager/src/tests/common/mod.rs +++ b/crates/defguard_gateway_manager/src/tests/common/mod.rs @@ -15,7 +15,10 @@ use defguard_common::{ db::{ Id, NoId, models::{ - gateway::Gateway, settings::initialize_current_settings, wireguard::WireguardNetwork, + Settings, + gateway::Gateway, + settings::{initialize_current_settings, set_settings}, + wireguard::WireguardNetwork, }, setup_pool, }, @@ -23,7 +26,9 @@ use defguard_common::{ messages::peer_stats_update::PeerStatsUpdate, }; use defguard_core::events::GatewayConnectionEvent; -use defguard_proto::gateway::{CoreRequest, CoreResponse, PeerStats, core_request, gateway_server}; +use defguard_proto::gateway::{ + CoreRequest, CoreResponse, PeerStats, core_request, core_response, gateway_server, +}; use prost_types::Timestamp; use sqlx::{PgPool, postgres::PgConnectOptions}; use tokio::{ @@ -382,6 +387,42 @@ impl ManagerTestContext { self.control.gateway_notification_count(gateway_id) } + pub(crate) fn disconnect_notification_count(&self, gateway_id: Id) -> u64 { + self.control.disconnect_notification_count(gateway_id) + } + + pub(crate) fn reconnect_notification_count(&self, gateway_id: Id) -> u64 { + self.control.reconnect_notification_count(gateway_id) + } + + pub(crate) async fn wait_for_disconnect_notification_count( + &self, + gateway_id: Id, + expected_count: u64, + ) { + timeout( + TEST_TIMEOUT, + self.control + .wait_for_disconnect_notification_count(gateway_id, expected_count), + ) + .await + .expect("timed out waiting for gateway disconnect email notification"); + } + + pub(crate) async fn wait_for_reconnect_notification_count( + &self, + gateway_id: Id, + expected_count: u64, + ) { + timeout( + TEST_TIMEOUT, + self.control + .wait_for_reconnect_notification_count(gateway_id, expected_count), + ) + .await + .expect("timed out waiting for gateway reconnect email notification"); + } + pub(crate) async fn wait_for_handler_spawn_attempt_count( &self, gateway_id: Id, @@ -448,6 +489,10 @@ impl ManagerTestContext { self.control.set_retry_delay(retry_delay); } + pub(crate) fn set_disconnect_notification_delay(&self, delay: Duration) { + self.control.set_disconnect_notification_delay(delay); + } + pub(crate) async fn finish(mut self) { if let Some(manager_task) = self.manager_task.take() { manager_task.abort(); @@ -645,6 +690,25 @@ impl Drop for HandlerTestContext { } } +/// Drives the config handshake between the manager-owned handler and a mock Gateway, and waits +/// until the Gateway is recorded as connected in the database. +pub(crate) async fn complete_manager_handshake( + context: &ManagerTestContext, + gateway: &Gateway, + mock_gateway: &mut MockGatewayHarness, +) { + mock_gateway.wait_connected().await; + mock_gateway.send_config_request(); + let outbound = mock_gateway.recv_outbound().await; + assert!(matches!( + outbound.payload, + Some(core_response::Payload::Config(_)) + )); + + let gateway_after = wait_for_gateway_connection_state(&context.pool, gateway.id, true).await; + assert!(gateway_after.is_connected()); +} + pub(crate) async fn reload_gateway(pool: &PgPool, gateway_id: Id) -> Gateway { Gateway::find_by_id(pool, gateway_id) .await @@ -686,6 +750,21 @@ pub(crate) fn build_peer_stats(endpoint: &str) -> PeerStats { } } +/// Sets the Gateway notification settings in the process-global `SETTINGS` struct. Must be +/// called after a test context has been created, since that is what initializes the struct. +/// +/// `set_settings` is used directly rather than `update_current_settings` because the latter +/// validates that SMTP is configured, which these tests deliberately do not do. Because the +/// struct is process-global, tests touching it have to be run under nextest, which gives every +/// test its own process. +pub(crate) fn configure_gateway_notifications(enabled: bool, inactivity_threshold_minutes: i32) { + let mut settings = Settings::get_current_settings(); + settings.gateway_disconnect_notifications_enabled = enabled; + settings.gateway_disconnect_notifications_reconnect_notification_enabled = enabled; + settings.gateway_disconnect_notifications_inactivity_threshold = inactivity_threshold_minutes; + set_settings(Some(settings)); +} + pub(crate) async fn create_network(pool: &PgPool) -> WireguardNetwork { let network = WireguardNetwork::new( unique_name("network"), diff --git a/crates/defguard_gateway_manager/src/tests/gateway_manager/manager.rs b/crates/defguard_gateway_manager/src/tests/gateway_manager/manager.rs index d5cef0315..96232f260 100644 --- a/crates/defguard_gateway_manager/src/tests/gateway_manager/manager.rs +++ b/crates/defguard_gateway_manager/src/tests/gateway_manager/manager.rs @@ -1,31 +1,13 @@ -use defguard_common::db::{Id, models::gateway::Gateway}; -use defguard_proto::gateway::core_response; +use defguard_common::db::models::gateway::Gateway; use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; use tonic::Status; use crate::tests::common::{ - ManagerTestContext, MockGatewayHarness, build_gateway_with_enabled, create_gateway, - create_gateway_with_enabled, create_network, reload_gateway, unique_mock_gateway_socket_path, - wait_for_gateway_connection_state, + ManagerTestContext, MockGatewayHarness, build_gateway_with_enabled, complete_manager_handshake, + create_gateway, create_gateway_with_enabled, create_network, reload_gateway, + unique_mock_gateway_socket_path, wait_for_gateway_connection_state, }; -async fn complete_manager_handshake( - context: &ManagerTestContext, - gateway: &Gateway, - mock_gateway: &mut MockGatewayHarness, -) { - mock_gateway.wait_connected().await; - mock_gateway.send_config_request(); - let outbound = mock_gateway.recv_outbound().await; - assert!(matches!( - outbound.payload, - Some(core_response::Payload::Config(_)) - )); - - let gateway_after = wait_for_gateway_connection_state(&context.pool, gateway.id, true).await; - assert!(gateway_after.is_connected()); -} - #[sqlx::test] async fn test_starts_existing_enabled_gateway_on_startup( _: PgPoolOptions, diff --git a/crates/defguard_gateway_manager/src/tests/gateway_manager/mod.rs b/crates/defguard_gateway_manager/src/tests/gateway_manager/mod.rs index caf495ef2..12c24d23c 100644 --- a/crates/defguard_gateway_manager/src/tests/gateway_manager/mod.rs +++ b/crates/defguard_gateway_manager/src/tests/gateway_manager/mod.rs @@ -1,2 +1,3 @@ mod handler; mod manager; +mod notifications; diff --git a/crates/defguard_gateway_manager/src/tests/gateway_manager/notifications.rs b/crates/defguard_gateway_manager/src/tests/gateway_manager/notifications.rs new file mode 100644 index 000000000..b6a6c72d8 --- /dev/null +++ b/crates/defguard_gateway_manager/src/tests/gateway_manager/notifications.rs @@ -0,0 +1,172 @@ +//! Tests for the Gateway disconnect/reconnect email notification pairing. +//! +//! The disconnect email is delayed by the configured inactivity threshold, so these tests need +//! a Gateway that can actually go down and come back. That makes them manager tests rather than +//! handler tests: `HandlerTestContext` only handles a single connection attempt. +//! +//! Mail delivery itself is fire-and-forget, so the assertions count notification decisions +//! recorded through `GatewayManagerTestSupport` instead of intercepting SMTP traffic. + +use std::time::Duration; + +use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; +use tokio::time::sleep; + +use crate::tests::common::{ + ManagerTestContext, MockGatewayHarness, complete_manager_handshake, + configure_gateway_notifications, create_gateway, create_network, + wait_for_gateway_connection_state, +}; + +/// Stands in for the real inactivity threshold, which is configured in whole minutes. +const FAST_NOTIFICATION_DELAY: Duration = Duration::from_millis(50); +/// Longer than any of these tests can run, so a pending notification can only disappear by +/// being cancelled. +const NEVER_ELAPSING_NOTIFICATION_DELAY: Duration = Duration::from_secs(600); +/// How long to wait before concluding that no notification is going to be sent. +const NO_NOTIFICATION_GRACE_PERIOD: Duration = Duration::from_millis(200); + +#[sqlx::test] +async fn test_reconnect_inside_inactivity_threshold_sends_no_notifications( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let mut context = ManagerTestContext::new(options).await; + configure_gateway_notifications(true, 5); + context.set_disconnect_notification_delay(NEVER_ELAPSING_NOTIFICATION_DELAY); + + let network = create_network(&context.pool).await; + let gateway = create_gateway(&context.pool, network.id).await; + let mut mock_gateway = MockGatewayHarness::start().await; + context.register_gateway_mock(&gateway, &mock_gateway); + + context.start().await; + complete_manager_handshake(&context, &gateway, &mut mock_gateway).await; + + let reconnect_socket_path = mock_gateway.socket_path(); + mock_gateway.close_stream(); + let disconnected_gateway = + wait_for_gateway_connection_state(&context.pool, gateway.id, false).await; + assert!(disconnected_gateway.disconnected_at.is_some()); + mock_gateway.expect_server_finished().await; + + // The Gateway comes back well inside the inactivity threshold. + let mut replacement_mock_gateway = MockGatewayHarness::start_at(reconnect_socket_path).await; + replacement_mock_gateway.wait_for_connection_count(1).await; + complete_manager_handshake(&context, &gateway, &mut replacement_mock_gateway).await; + + sleep(NO_NOTIFICATION_GRACE_PERIOD).await; + assert_eq!( + context.disconnect_notification_count(gateway.id), + 0, + "a Gateway blip should not produce a disconnect email" + ); + assert_eq!( + context.reconnect_notification_count(gateway.id), + 0, + "a Gateway blip should not produce a reconnect email" + ); + + context.finish().await; +} + +#[sqlx::test] +async fn test_outage_past_inactivity_threshold_sends_disconnect_then_reconnect_notification( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let mut context = ManagerTestContext::new(options).await; + configure_gateway_notifications(true, 5); + context.set_disconnect_notification_delay(FAST_NOTIFICATION_DELAY); + + let network = create_network(&context.pool).await; + let gateway = create_gateway(&context.pool, network.id).await; + let mut mock_gateway = MockGatewayHarness::start().await; + context.register_gateway_mock(&gateway, &mock_gateway); + + context.start().await; + complete_manager_handshake(&context, &gateway, &mut mock_gateway).await; + + let reconnect_socket_path = mock_gateway.socket_path(); + mock_gateway.close_stream(); + let disconnected_gateway = + wait_for_gateway_connection_state(&context.pool, gateway.id, false).await; + assert!(disconnected_gateway.disconnected_at.is_some()); + mock_gateway.expect_server_finished().await; + + // Nothing is listening on the socket yet, so the Gateway stays down past the threshold and + // the disconnect email goes out. + context + .wait_for_disconnect_notification_count(gateway.id, 1) + .await; + assert_eq!( + context.reconnect_notification_count(gateway.id), + 0, + "reconnect email must not precede the Gateway coming back" + ); + + let mut replacement_mock_gateway = MockGatewayHarness::start_at(reconnect_socket_path).await; + replacement_mock_gateway.wait_for_connection_count(1).await; + complete_manager_handshake(&context, &gateway, &mut replacement_mock_gateway).await; + + context + .wait_for_reconnect_notification_count(gateway.id, 1) + .await; + assert_eq!( + context.disconnect_notification_count(gateway.id), + 1, + "the outage should have produced exactly one disconnect email" + ); + + context.finish().await; +} + +#[sqlx::test] +async fn test_disabled_notifications_send_nothing_across_a_full_outage( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let mut context = ManagerTestContext::new(options).await; + configure_gateway_notifications(false, 5); + // Same short delay as the outage test, so a scheduled notification would have fired. + context.set_disconnect_notification_delay(FAST_NOTIFICATION_DELAY); + + let network = create_network(&context.pool).await; + let gateway = create_gateway(&context.pool, network.id).await; + let mut mock_gateway = MockGatewayHarness::start().await; + context.register_gateway_mock(&gateway, &mock_gateway); + + context.start().await; + complete_manager_handshake(&context, &gateway, &mut mock_gateway).await; + + let reconnect_socket_path = mock_gateway.socket_path(); + let connection_attempts_before = context.handler_connection_attempt_count(gateway.id); + mock_gateway.close_stream(); + wait_for_gateway_connection_state(&context.pool, gateway.id, false).await; + mock_gateway.expect_server_finished().await; + + // Wait out a failed reconnect attempt plus the notification delay, so a notification would + // have had every chance to fire. + context + .wait_for_handler_connection_attempt_count(gateway.id, connection_attempts_before + 1) + .await; + sleep(NO_NOTIFICATION_GRACE_PERIOD).await; + assert_eq!( + context.disconnect_notification_count(gateway.id), + 0, + "disabled notifications should not schedule a disconnect email" + ); + + let mut replacement_mock_gateway = MockGatewayHarness::start_at(reconnect_socket_path).await; + replacement_mock_gateway.wait_for_connection_count(1).await; + complete_manager_handshake(&context, &gateway, &mut replacement_mock_gateway).await; + + sleep(NO_NOTIFICATION_GRACE_PERIOD).await; + assert_eq!( + context.reconnect_notification_count(gateway.id), + 0, + "disabled notifications should not produce a reconnect email" + ); + + context.finish().await; +} From 3906512cbefeb46a6b07ec2344971d766f248f26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20W=C3=B3jcik?= Date: Mon, 17 Aug 2026 17:26:16 +0200 Subject: [PATCH 6/6] address review feedback --- ...260a580f6f549f696f9d639093c8105d76d25.json | 15 ++ .../defguard_core/src/db/models/enrollment.rs | 9 + .../src/enrollment_management.rs | 6 +- crates/defguard_core/src/mail/templates.rs | 46 ++-- crates/defguard_core/src/mail/tests.rs | 10 +- .../defguard_gateway_manager/src/handler.rs | 82 ++++-- crates/defguard_gateway_manager/src/lib.rs | 247 ++++++++---------- .../src/tests/common/mod.rs | 17 +- .../tests/gateway_manager/notifications.rs | 53 +++- ...[2.1.0]_enrollment_email_timeouts.down.sql | 5 +- ...9_[2.1.0]_enrollment_email_timeouts.up.sql | 4 +- 11 files changed, 293 insertions(+), 201 deletions(-) create mode 100644 .sqlx/query-8b80c6e818d3ff25ad609bd592a260a580f6f549f696f9d639093c8105d76d25.json diff --git a/.sqlx/query-8b80c6e818d3ff25ad609bd592a260a580f6f549f696f9d639093c8105d76d25.json b/.sqlx/query-8b80c6e818d3ff25ad609bd592a260a580f6f549f696f9d639093c8105d76d25.json new file mode 100644 index 000000000..2da8736d1 --- /dev/null +++ b/.sqlx/query-8b80c6e818d3ff25ad609bd592a260a580f6f549f696f9d639093c8105d76d25.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE settings SET enrollment_token_timeout_hours = $1, enrollment_session_timeout_minutes = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Int4", + "Int4" + ] + }, + "nullable": [] + }, + "hash": "8b80c6e818d3ff25ad609bd592a260a580f6f549f696f9d639093c8105d76d25" +} diff --git a/crates/defguard_core/src/db/models/enrollment.rs b/crates/defguard_core/src/db/models/enrollment.rs index c02fc70ea..7d014dc69 100644 --- a/crates/defguard_core/src/db/models/enrollment.rs +++ b/crates/defguard_core/src/db/models/enrollment.rs @@ -1,3 +1,5 @@ +use std::time::Duration; + use chrono::{NaiveDateTime, TimeDelta, Utc}; use defguard_common::{ VERSION, @@ -111,6 +113,13 @@ impl Token { } } + /// Duration for which the token is valid, i.e. `expires_at - created_at`, clamped to zero. + #[must_use] + pub fn validity_duration(&self) -> Duration { + let seconds = (self.expires_at - self.created_at).num_seconds().max(0); + Duration::from_secs(u64::try_from(seconds).unwrap_or_default()) + } + pub async fn save<'e, E>(&self, executor: E) -> Result<(), TokenError> where E: PgExecutor<'e>, diff --git a/crates/defguard_core/src/enrollment_management.rs b/crates/defguard_core/src/enrollment_management.rs index 946c3fe5d..1fb8dfd9f 100644 --- a/crates/defguard_core/src/enrollment_management.rs +++ b/crates/defguard_core/src/enrollment_management.rs @@ -1,5 +1,3 @@ -use std::time::Duration; - use defguard_common::db::{ Id, models::{Settings, user::User}, @@ -63,7 +61,7 @@ pub async fn start_user_enrollment( base_message_context, enrollment_service_url, &enrollment.id, - Duration::from_secs(token_timeout_seconds), + enrollment.validity_duration(), ) .await; match result { @@ -203,7 +201,7 @@ pub async fn send_enrollment_invitation( base_message_context, enrollment_service_url, token_id, - Duration::from_secs((token.expires_at - token.created_at).num_seconds().max(0) as u64), + token.validity_duration(), ) .await { diff --git a/crates/defguard_core/src/mail/templates.rs b/crates/defguard_core/src/mail/templates.rs index 0bdcc2823..2c442c2f7 100644 --- a/crates/defguard_core/src/mail/templates.rs +++ b/crates/defguard_core/src/mail/templates.rs @@ -139,6 +139,11 @@ pub async fn user_import_blocked_mail( Ok(()) } +/// Placeholders substituted into the admin-configurable `token_info` mail text with the +/// configured enrollment timeouts. +const TOKEN_TIMEOUT_PLACEHOLDER: &str = "{{ token_timeout }}"; +const SESSION_TIMEOUT_PLACEHOLDER: &str = "{{ session_timeout }}"; + // Mail with link to enrollment service. pub async fn new_account_mail( to: &str, @@ -191,21 +196,26 @@ pub(crate) async fn build_new_account_mail( let message = MailMessage::NewAccount; message.fill_context(conn, &mut context).await?; - // Inject the effective token/session timeouts so the email reflects the configured values - // instead of the hardcoded defaults. See https://github.com/DefGuard/defguard/issues/3518. - let settings = Settings::get_current_settings(); - context.insert("token_timeout", &format_timeout(token_timeout)); - context.insert( - "session_timeout", - &format_timeout(settings.enrollment_session_timeout()), - ); + // The token timeout is per-enrollment (passed in), while the session timeout is a global + // setting read here so the email reflects the configured value. + let session_timeout = Settings::get_current_settings().enrollment_session_timeout(); - // The `token_info` section is admin-configurable text; render it as a template so it can - // reference the `token_timeout` / `session_timeout` variables above. + // Render the effective token/session timeouts into the admin-configurable `token_info` text. if let Some(Value::String(token_info)) = context.get("token_info").cloned() { - let mut info_tera = safe_tera(); - info_tera.add_raw_template("token_info", &token_info)?; - let rendered = info_tera.render("token_info", &context)?; + if !token_info.contains(TOKEN_TIMEOUT_PLACEHOLDER) + || !token_info.contains(SESSION_TIMEOUT_PLACEHOLDER) + { + warn!( + "mail_context 'new-account' token_info is missing the timeout placeholders; \ + the configured enrollment timeouts will not be shown" + ); + } + let rendered = token_info + .replace(TOKEN_TIMEOUT_PLACEHOLDER, &format_timeout(token_timeout)) + .replace( + SESSION_TIMEOUT_PLACEHOLDER, + &format_timeout(session_timeout), + ); context.insert("token_info", &rendered); } @@ -759,7 +769,7 @@ mod tests { use super::format_timeout; #[test] - fn formats_weeks() { + fn test_formats_weeks() { assert_eq!(format_timeout(Duration::from_secs(7 * 24 * 3600)), "1 week"); assert_eq!( format_timeout(Duration::from_secs(14 * 24 * 3600)), @@ -768,25 +778,25 @@ mod tests { } #[test] - fn formats_days() { + fn test_formats_days() { assert_eq!(format_timeout(Duration::from_secs(24 * 3600)), "1 day"); assert_eq!(format_timeout(Duration::from_secs(2 * 24 * 3600)), "2 days"); } #[test] - fn formats_hours() { + fn test_formats_hours() { assert_eq!(format_timeout(Duration::from_secs(3600)), "1 hour"); assert_eq!(format_timeout(Duration::from_secs(23 * 3600)), "23 hours"); } #[test] - fn formats_minutes() { + fn test_formats_minutes() { assert_eq!(format_timeout(Duration::from_secs(60)), "1 minute"); assert_eq!(format_timeout(Duration::from_secs(30 * 60)), "30 minutes"); } #[test] - fn formats_seconds() { + fn test_formats_seconds() { assert_eq!(format_timeout(Duration::from_secs(1)), "1 second"); assert_eq!(format_timeout(Duration::from_secs(90)), "90 seconds"); } diff --git a/crates/defguard_core/src/mail/tests.rs b/crates/defguard_core/src/mail/tests.rs index 29e9f9f7e..5a63e3551 100644 --- a/crates/defguard_core/src/mail/tests.rs +++ b/crates/defguard_core/src/mail/tests.rs @@ -35,13 +35,13 @@ fn dg25_8_server_side_template_injection() { /// Override the enrollment token/session timeouts and reload the global settings. async fn set_enrollment_timeouts(pool: &PgPool, token_hours: i32, session_minutes: i32) { - sqlx::query( + sqlx::query!( "UPDATE settings \ SET enrollment_token_timeout_hours = $1, \ enrollment_session_timeout_minutes = $2", + token_hours, + session_minutes, ) - .bind(token_hours) - .bind(session_minutes) .execute(pool) .await .unwrap(); @@ -54,7 +54,7 @@ async fn set_enrollment_timeouts(pool: &PgPool, token_hours: i32, session_minute /// The enrollment email must reflect the configured enrollment token and session timeouts /// instead of the hardcoded defaults ("24 hours" / "10 minutes"). #[sqlx::test] -async fn enrollment_email_reflects_configured_timeouts( +async fn test_enrollment_email_reflects_configured_timeouts( _: PgPoolOptions, options: PgConnectOptions, ) { @@ -62,6 +62,8 @@ async fn enrollment_email_reflects_configured_timeouts( initialize_current_settings(&pool).await.unwrap(); // Configure non-default timeouts: token valid for 1 week, session for 30 minutes. + // `build_new_account_mail` reads the session timeout from the global settings set here, + // while the token timeout is per-enrollment and passed explicitly. set_enrollment_timeouts(&pool, 168, 30).await; let mut conn = pool.begin().await.unwrap(); diff --git a/crates/defguard_gateway_manager/src/handler.rs b/crates/defguard_gateway_manager/src/handler.rs index 6e912c41e..18d58f61b 100644 --- a/crates/defguard_gateway_manager/src/handler.rs +++ b/crates/defguard_gateway_manager/src/handler.rs @@ -114,6 +114,7 @@ impl GatewayHandler { connection_events_tx: UnboundedSender, peer_stats_tx: UnboundedSender, certs_rx: watch::Receiver>>, + disconnect_notification_sent: Arc, ) -> Result { let url = Url::from_str(&gateway.url()).map_err(|err| { GatewayError::EndpointError(format!( @@ -133,7 +134,7 @@ impl GatewayHandler { certs_rx, updates_handler_handle: None, pending_disconnect_notification: None, - disconnect_notification_sent: Arc::new(AtomicBool::new(false)), + disconnect_notification_sent, #[cfg(test)] test_transport: GatewayTestTransport::default(), #[cfg(test)] @@ -295,9 +296,17 @@ impl GatewayHandler { // A threshold of 0 keeps the notification immediate, which is what the settings form // allows as its minimum value. - let threshold_minutes = - u64::try_from(settings.gateway_disconnect_notifications_inactivity_threshold) - .unwrap_or_default(); + let threshold = settings.gateway_disconnect_notifications_inactivity_threshold; + let threshold_minutes = match u64::try_from(threshold) { + Ok(minutes) => minutes, + Err(_) => { + warn!( + "Gateway disconnect notifications inactivity threshold {threshold} is \ + negative; treating it as 0 (immediate)" + ); + 0 + } + }; let delay = self.disconnect_notification_delay(Duration::from_secs(60 * threshold_minutes)); let gateway_id = self.gateway.id; @@ -366,17 +375,19 @@ impl GatewayHandler { self.pending_disconnect_notification = Some(handle); } - /// Send Gateway reconnected notification. - /// - /// Only sent when a disconnect notification actually went out for this outage, so admins - /// never receive a reconnect email without a matching disconnect email. - fn send_reconnect_notification(&self, network_name: String) { - // Always clear the flag, even when reconnect notifications are turned off, so that a - // later outage cannot inherit it. - if !self - .disconnect_notification_sent + /// Returns true if a disconnect email went out for this outage, and atomically clears the + /// flag so a later outage cannot inherit it. + fn take_disconnect_notification_sent(&self) -> bool { + self.disconnect_notification_sent .swap(false, Ordering::SeqCst) - { + } + + /// Send a Gateway reconnected notification, but only when a matching disconnect notification + /// actually went out for this outage. + fn maybe_send_reconnect_notification(&self, network_name: String) { + // Consume the flag even when reconnect notifications are turned off, so that a later + // outage cannot inherit it. + if !self.take_disconnect_notification_sent() { return; } @@ -425,7 +436,9 @@ impl GatewayHandler { } // Mark the Gateway disconnected before scheduling the notification: the delayed task - // re-reads the row when it fires and must not see a stale connected state. + // re-reads the row when it fires and must not see a stale connected state. If the DB + // write fails, skip the notification entirely rather than scheduling a task that would + // re-read the still-connected row and never fire. if self.mark_disconnected().await { let _ = self .connection_events_tx @@ -433,12 +446,23 @@ impl GatewayHandler { gateway_id: self.gateway.id, gateway_name: self.gateway.name.clone(), }); - } - self.schedule_disconnect_notification(); + self.schedule_disconnect_notification(); + } } async fn mark_connected_and_maybe_notify(&mut self, network_name: &str) { + // A Gateway that came back should not be reported as down at all. Cancel any pending + // disconnect email before touching the database, so a DB error cannot leave the task + // alive to send a false "disconnected" alert for a gateway that is actually back. + if let Some(handle) = self.pending_disconnect_notification.take() { + debug!( + "Cancelling pending disconnect email notification for {}", + self.gateway + ); + handle.abort(); + } + let was_connected = self.gateway.is_connected(); if let Err(err) = self.gateway.touch_connected(&self.pool).await { error!( @@ -457,16 +481,7 @@ impl GatewayHandler { }); } - // A Gateway that came back this quickly should not be reported as down at all. - if let Some(handle) = self.pending_disconnect_notification.take() { - debug!( - "Cancelling pending disconnect email notification for {}", - self.gateway - ); - handle.abort(); - } - - self.send_reconnect_notification(network_name.to_owned()); + self.maybe_send_reconnect_notification(network_name.to_owned()); } fn remove_client(&self, clients: &Arc>>) { @@ -686,6 +701,9 @@ fn note_disconnect_notification_for_tests( #[cfg(test)] impl GatewayHandler { + // Bundles the socket path and reconnect-pairing flag on top of the already-long + // `GatewayHandler::new` parameter list; splitting them out is not worth the churn. + #[allow(clippy::too_many_arguments)] pub(crate) fn new_with_test_socket( gateway: Gateway, pool: PgPool, @@ -694,6 +712,7 @@ impl GatewayHandler { peer_stats_tx: UnboundedSender, certs_rx: watch::Receiver>>, socket_path: PathBuf, + disconnect_notification_sent: Arc, ) -> Result { let mut handler = Self::new( gateway, @@ -702,6 +721,7 @@ impl GatewayHandler { connection_events_tx, peer_stats_tx, certs_rx, + disconnect_notification_sent, )?; handler.test_transport = GatewayTestTransport::with_socket_path(socket_path); Ok(handler) @@ -1243,7 +1263,12 @@ fn try_protos_into_stats_message( #[cfg(test)] mod tests { - use std::{collections::HashMap, net::IpAddr, str::FromStr, sync::Arc}; + use std::{ + collections::HashMap, + net::IpAddr, + str::FromStr, + sync::{Arc, atomic::AtomicBool}, + }; use chrono::{DateTime, Utc}; use defguard_common::{ @@ -1623,6 +1648,7 @@ mod tests { connection_events_tx, peer_stats_tx, certs_rx, + Arc::new(AtomicBool::new(false)), ) .unwrap(); let (tx, mut rx) = unbounded_channel(); diff --git a/crates/defguard_gateway_manager/src/lib.rs b/crates/defguard_gateway_manager/src/lib.rs index 6619421b0..c89b92209 100644 --- a/crates/defguard_gateway_manager/src/lib.rs +++ b/crates/defguard_gateway_manager/src/lib.rs @@ -1,13 +1,10 @@ use std::{ collections::HashMap, - sync::{Arc, Mutex}, + sync::{Arc, Mutex, atomic::AtomicBool}, time::Duration, }; #[cfg(test)] -use std::{ - path::PathBuf, - sync::atomic::{AtomicBool, Ordering}, -}; +use std::{path::PathBuf, sync::atomic::Ordering}; use defguard_common::{ db::{ChangeNotification, Id, TriggerOperation, models::gateway::Gateway}, @@ -69,20 +66,60 @@ impl Drop for AbortTaskOnDrop { } } +/// A per-Gateway counter that tests can increment, read, and block on. The map and its +/// `Notify` always travel together, so they are bundled into one type. +#[cfg(test)] +#[derive(Clone, Default)] +struct NotificationCounter { + counts: Arc>>, + notify: Arc, +} + +#[cfg(test)] +impl NotificationCounter { + fn note(&self, gateway_id: Id) { + let mut counts = self + .counts + .lock() + .expect("Failed to lock GatewayManager test notification counter"); + *counts.entry(gateway_id).or_default() += 1; + self.notify.notify_waiters(); + } + + fn count(&self, gateway_id: Id) -> u64 { + self.counts + .lock() + .expect("Failed to lock GatewayManager test notification counter") + .get(&gateway_id) + .copied() + .unwrap_or_default() + } + + async fn wait_for(&self, gateway_id: Id, expected_count: u64) { + loop { + if self.count(gateway_id) >= expected_count { + return; + } + + let notified = self.notify.notified(); + if self.count(gateway_id) >= expected_count { + return; + } + + notified.await; + } + } +} + #[cfg(test)] #[derive(Clone, Default)] struct GatewayManagerTestSupport { socket_paths_by_url: Arc>>, - handler_spawn_attempts_by_gateway: Arc>>, - handler_spawn_attempt_notify: Arc, - handler_connection_attempts_by_gateway: Arc>>, - handler_connection_attempt_notify: Arc, - gateway_notifications_by_gateway: Arc>>, - gateway_notification_notify: Arc, - disconnect_notifications_by_gateway: Arc>>, - disconnect_notification_notify: Arc, - reconnect_notifications_by_gateway: Arc>>, - reconnect_notification_notify: Arc, + handler_spawn_attempts: NotificationCounter, + handler_connection_attempts: NotificationCounter, + gateway_notifications: NotificationCounter, + disconnect_notifications: NotificationCounter, + reconnect_notifications: NotificationCounter, listener_ready: Arc, listener_ready_notify: Arc, retry_delay_override: Arc>>, @@ -107,181 +144,86 @@ impl GatewayManagerTestSupport { } fn note_handler_spawn_attempt(&self, gateway_id: Id) { - let mut handler_spawn_attempts = self - .handler_spawn_attempts_by_gateway - .lock() - .expect("Failed to lock GatewayManager handler spawn attempts registry"); - *handler_spawn_attempts.entry(gateway_id).or_default() += 1; - self.handler_spawn_attempt_notify.notify_waiters(); + self.handler_spawn_attempts.note(gateway_id); } #[cfg(test)] fn handler_spawn_attempt_count(&self, gateway_id: Id) -> u64 { - self.handler_spawn_attempts_by_gateway - .lock() - .expect("Failed to lock GatewayManager handler spawn attempts registry") - .get(&gateway_id) - .copied() - .unwrap_or_default() + self.handler_spawn_attempts.count(gateway_id) } #[cfg(test)] async fn wait_for_handler_spawn_attempt_count(&self, gateway_id: Id, expected_count: u64) { - loop { - if self.handler_spawn_attempt_count(gateway_id) >= expected_count { - return; - } - - let notified = self.handler_spawn_attempt_notify.notified(); - if self.handler_spawn_attempt_count(gateway_id) >= expected_count { - return; - } - - notified.await; - } + self.handler_spawn_attempts + .wait_for(gateway_id, expected_count) + .await; } fn note_handler_connection_attempt(&self, gateway_id: Id) { - let mut handler_connection_attempts = self - .handler_connection_attempts_by_gateway - .lock() - .expect("Failed to lock GatewayManager handler connection attempts registry"); - *handler_connection_attempts.entry(gateway_id).or_default() += 1; - self.handler_connection_attempt_notify.notify_waiters(); + self.handler_connection_attempts.note(gateway_id); } #[cfg(test)] fn handler_connection_attempt_count(&self, gateway_id: Id) -> u64 { - self.handler_connection_attempts_by_gateway - .lock() - .expect("Failed to lock GatewayManager handler connection attempts registry") - .get(&gateway_id) - .copied() - .unwrap_or_default() + self.handler_connection_attempts.count(gateway_id) } #[cfg(test)] async fn wait_for_handler_connection_attempt_count(&self, gateway_id: Id, expected_count: u64) { - loop { - if self.handler_connection_attempt_count(gateway_id) >= expected_count { - return; - } - - let notified = self.handler_connection_attempt_notify.notified(); - if self.handler_connection_attempt_count(gateway_id) >= expected_count { - return; - } - - notified.await; - } + self.handler_connection_attempts + .wait_for(gateway_id, expected_count) + .await; } fn note_gateway_notification(&self, gateway_id: Id) { - let mut gateway_notifications = self - .gateway_notifications_by_gateway - .lock() - .expect("Failed to lock GatewayManager gateway notification registry"); - *gateway_notifications.entry(gateway_id).or_default() += 1; - self.gateway_notification_notify.notify_waiters(); + self.gateway_notifications.note(gateway_id); } #[cfg(test)] fn gateway_notification_count(&self, gateway_id: Id) -> u64 { - self.gateway_notifications_by_gateway - .lock() - .expect("Failed to lock GatewayManager gateway notification registry") - .get(&gateway_id) - .copied() - .unwrap_or_default() + self.gateway_notifications.count(gateway_id) } #[cfg(test)] async fn wait_for_gateway_notification_count(&self, gateway_id: Id, expected_count: u64) { - loop { - if self.gateway_notification_count(gateway_id) >= expected_count { - return; - } - - let notified = self.gateway_notification_notify.notified(); - if self.gateway_notification_count(gateway_id) >= expected_count { - return; - } - - notified.await; - } + self.gateway_notifications + .wait_for(gateway_id, expected_count) + .await; } /// Records that a Gateway disconnect email notification was actually sent. Tests use this /// instead of a mock SMTP server, because mail delivery itself is fire-and-forget. fn note_disconnect_notification_sent(&self, gateway_id: Id) { - let mut disconnect_notifications = self - .disconnect_notifications_by_gateway - .lock() - .expect("Failed to lock GatewayManager disconnect notification registry"); - *disconnect_notifications.entry(gateway_id).or_default() += 1; - self.disconnect_notification_notify.notify_waiters(); + self.disconnect_notifications.note(gateway_id); } #[cfg(test)] fn disconnect_notification_count(&self, gateway_id: Id) -> u64 { - self.disconnect_notifications_by_gateway - .lock() - .expect("Failed to lock GatewayManager disconnect notification registry") - .get(&gateway_id) - .copied() - .unwrap_or_default() + self.disconnect_notifications.count(gateway_id) } #[cfg(test)] async fn wait_for_disconnect_notification_count(&self, gateway_id: Id, expected_count: u64) { - loop { - if self.disconnect_notification_count(gateway_id) >= expected_count { - return; - } - - let notified = self.disconnect_notification_notify.notified(); - if self.disconnect_notification_count(gateway_id) >= expected_count { - return; - } - - notified.await; - } + self.disconnect_notifications + .wait_for(gateway_id, expected_count) + .await; } /// Records that a Gateway reconnect email notification was actually sent. fn note_reconnect_notification_sent(&self, gateway_id: Id) { - let mut reconnect_notifications = self - .reconnect_notifications_by_gateway - .lock() - .expect("Failed to lock GatewayManager reconnect notification registry"); - *reconnect_notifications.entry(gateway_id).or_default() += 1; - self.reconnect_notification_notify.notify_waiters(); + self.reconnect_notifications.note(gateway_id); } #[cfg(test)] fn reconnect_notification_count(&self, gateway_id: Id) -> u64 { - self.reconnect_notifications_by_gateway - .lock() - .expect("Failed to lock GatewayManager reconnect notification registry") - .get(&gateway_id) - .copied() - .unwrap_or_default() + self.reconnect_notifications.count(gateway_id) } #[cfg(test)] async fn wait_for_reconnect_notification_count(&self, gateway_id: Id, expected_count: u64) { - loop { - if self.reconnect_notification_count(gateway_id) >= expected_count { - return; - } - - let notified = self.reconnect_notification_notify.notified(); - if self.reconnect_notification_count(gateway_id) >= expected_count { - return; - } - - notified.await; - } + self.reconnect_notifications + .wait_for(gateway_id, expected_count) + .await; } fn mark_listener_ready(&self) { @@ -349,6 +291,10 @@ pub struct GatewayManager { clients: Arc>>, pool: PgPool, handlers: JoinSet>, + /// Per-Gateway flag tracking whether a disconnect email went out and is still awaiting its + /// matching reconnect email. Owned by the manager rather than the handler so it survives a + /// handler rebuild when connection-relevant fields change. + disconnect_notification_sent_by_gateway: Arc>>>, #[cfg(test)] test_support: GatewayManagerTestSupport, tx: GatewayTxSet, @@ -362,6 +308,7 @@ impl GatewayManager { clients: Arc::default(), handlers: JoinSet::new(), pool, + disconnect_notification_sent_by_gateway: Arc::default(), tx, } } @@ -373,6 +320,7 @@ impl GatewayManager { clients: Arc::default(), handlers: JoinSet::new(), pool, + disconnect_notification_sent_by_gateway: Arc::default(), test_support, tx, } @@ -398,11 +346,34 @@ impl GatewayManager { } } + /// Returns the per-Gateway "disconnect email sent, awaiting reconnect" flag, creating it if + /// necessary. + fn disconnect_notification_flag(&self, gateway_id: Id) -> Arc { + self.disconnect_notification_sent_by_gateway + .lock() + .expect("Failed to lock GatewayManager disconnect notification flags") + .entry(gateway_id) + .or_insert_with(|| Arc::new(AtomicBool::new(false))) + .clone() + } + + /// Removes the per-Gateway "disconnect email sent" flag (on Gateway deletion). + fn clear_disconnect_notification_flag(&self, gateway_id: Id) { + self.disconnect_notification_sent_by_gateway + .lock() + .expect("Failed to lock GatewayManager disconnect notification flags") + .remove(&gateway_id); + } + fn build_handler( &self, gateway: Gateway, certs_rx: Receiver>>, ) -> Result { + // Reuse any outstanding "disconnect sent" flag for this Gateway so a reconnect email is + // not lost when the handler is rebuilt after connection-relevant fields change. + let disconnect_notification_sent = self.disconnect_notification_flag(gateway.id); + #[cfg(test)] { self.test_support.note_handler_spawn_attempt(gateway.id); @@ -417,6 +388,7 @@ impl GatewayManager { self.tx.peer_stats.clone(), certs_rx, socket_path, + disconnect_notification_sent, )? } else { GatewayHandler::new( @@ -426,6 +398,7 @@ impl GatewayManager { self.tx.connection_events.clone(), self.tx.peer_stats.clone(), certs_rx, + disconnect_notification_sent, )? }; gateway_handler.attach_test_support(self.test_support.clone()); @@ -441,6 +414,7 @@ impl GatewayManager { self.tx.connection_events.clone(), self.tx.peer_stats.clone(), certs_rx, + disconnect_notification_sent, ) } @@ -628,6 +602,9 @@ impl GatewayManager { ); } + // Drop the per-Gateway reconnect-pairing state. + self.clear_disconnect_notification_flag(gateway_id); + #[cfg(test)] self.note_gateway_notification_for_tests(gateway_id); } diff --git a/crates/defguard_gateway_manager/src/tests/common/mod.rs b/crates/defguard_gateway_manager/src/tests/common/mod.rs index eae01623f..e4f9285d4 100644 --- a/crates/defguard_gateway_manager/src/tests/common/mod.rs +++ b/crates/defguard_gateway_manager/src/tests/common/mod.rs @@ -6,7 +6,7 @@ use std::{ process, sync::{ Arc, Mutex, - atomic::{AtomicU16, AtomicU64, Ordering}, + atomic::{AtomicBool, AtomicU16, AtomicU64, Ordering}, }, time::Duration, }; @@ -556,6 +556,7 @@ impl HandlerTestContext { peer_stats_tx, certs_rx, mock_gateway.socket_path(), + Arc::new(AtomicBool::new(false)), ) .expect("failed to create gateway handler"); let handler_task = tokio::spawn(async move { handler.handle_connection_once().await }); @@ -754,13 +755,15 @@ pub(crate) fn build_peer_stats(endpoint: &str) -> PeerStats { /// called after a test context has been created, since that is what initializes the struct. /// /// `set_settings` is used directly rather than `update_current_settings` because the latter -/// validates that SMTP is configured, which these tests deliberately do not do. Because the -/// struct is process-global, tests touching it have to be run under nextest, which gives every -/// test its own process. -pub(crate) fn configure_gateway_notifications(enabled: bool, inactivity_threshold_minutes: i32) { +/// validates that SMTP is configured, which these tests deliberately do not do. +pub(crate) fn configure_gateway_notifications( + disconnect_enabled: bool, + reconnect_enabled: bool, + inactivity_threshold_minutes: i32, +) { let mut settings = Settings::get_current_settings(); - settings.gateway_disconnect_notifications_enabled = enabled; - settings.gateway_disconnect_notifications_reconnect_notification_enabled = enabled; + settings.gateway_disconnect_notifications_enabled = disconnect_enabled; + settings.gateway_disconnect_notifications_reconnect_notification_enabled = reconnect_enabled; settings.gateway_disconnect_notifications_inactivity_threshold = inactivity_threshold_minutes; set_settings(Some(settings)); } diff --git a/crates/defguard_gateway_manager/src/tests/gateway_manager/notifications.rs b/crates/defguard_gateway_manager/src/tests/gateway_manager/notifications.rs index b6a6c72d8..00abd7eb1 100644 --- a/crates/defguard_gateway_manager/src/tests/gateway_manager/notifications.rs +++ b/crates/defguard_gateway_manager/src/tests/gateway_manager/notifications.rs @@ -32,7 +32,7 @@ async fn test_reconnect_inside_inactivity_threshold_sends_no_notifications( options: PgConnectOptions, ) { let mut context = ManagerTestContext::new(options).await; - configure_gateway_notifications(true, 5); + configure_gateway_notifications(true, true, 5); context.set_disconnect_notification_delay(NEVER_ELAPSING_NOTIFICATION_DELAY); let network = create_network(&context.pool).await; @@ -76,7 +76,7 @@ async fn test_outage_past_inactivity_threshold_sends_disconnect_then_reconnect_n options: PgConnectOptions, ) { let mut context = ManagerTestContext::new(options).await; - configure_gateway_notifications(true, 5); + configure_gateway_notifications(true, true, 5); context.set_disconnect_notification_delay(FAST_NOTIFICATION_DELAY); let network = create_network(&context.pool).await; @@ -127,7 +127,7 @@ async fn test_disabled_notifications_send_nothing_across_a_full_outage( options: PgConnectOptions, ) { let mut context = ManagerTestContext::new(options).await; - configure_gateway_notifications(false, 5); + configure_gateway_notifications(false, false, 5); // Same short delay as the outage test, so a scheduled notification would have fired. context.set_disconnect_notification_delay(FAST_NOTIFICATION_DELAY); @@ -170,3 +170,50 @@ async fn test_disabled_notifications_send_nothing_across_a_full_outage( context.finish().await; } + +#[sqlx::test] +async fn test_reconnect_notification_disabled_sends_only_disconnect_email( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let mut context = ManagerTestContext::new(options).await; + configure_gateway_notifications(true, false, 5); + context.set_disconnect_notification_delay(FAST_NOTIFICATION_DELAY); + + let network = create_network(&context.pool).await; + let gateway = create_gateway(&context.pool, network.id).await; + let mut mock_gateway = MockGatewayHarness::start().await; + context.register_gateway_mock(&gateway, &mock_gateway); + + context.start().await; + complete_manager_handshake(&context, &gateway, &mut mock_gateway).await; + + let reconnect_socket_path = mock_gateway.socket_path(); + mock_gateway.close_stream(); + let disconnected_gateway = + wait_for_gateway_connection_state(&context.pool, gateway.id, false).await; + assert!(disconnected_gateway.disconnected_at.is_some()); + mock_gateway.expect_server_finished().await; + + context + .wait_for_disconnect_notification_count(gateway.id, 1) + .await; + + let mut replacement_mock_gateway = MockGatewayHarness::start_at(reconnect_socket_path).await; + replacement_mock_gateway.wait_for_connection_count(1).await; + complete_manager_handshake(&context, &gateway, &mut replacement_mock_gateway).await; + + sleep(NO_NOTIFICATION_GRACE_PERIOD).await; + assert_eq!( + context.disconnect_notification_count(gateway.id), + 1, + "the outage should have produced exactly one disconnect email" + ); + assert_eq!( + context.reconnect_notification_count(gateway.id), + 0, + "a reconnect email must not be sent when reconnect notifications are disabled" + ); + + context.finish().await; +} diff --git a/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.down.sql b/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.down.sql index f14e1e2b5..7564782b8 100644 --- a/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.down.sql +++ b/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.down.sql @@ -1,3 +1,6 @@ +-- Restore the pre-2.1 default text, but only for rows still carrying the placeholder text, so +-- any text customized after the upgrade is preserved. UPDATE mail_context SET text = 'The token is valid for 24 hours. Once the enrollment process starts, you have 10 minutes to complete it.' -WHERE template = 'new-account' AND section = 'token_info' AND language_tag = 'en_US'; +WHERE template = 'new-account' AND section = 'token_info' AND language_tag = 'en_US' + AND text = 'The token is valid for {{ token_timeout }}. Once the enrollment process starts, you have {{ session_timeout }} to complete it.'; diff --git a/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.up.sql b/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.up.sql index 9cc2553a7..77b588fb0 100644 --- a/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.up.sql +++ b/migrations/20260817091049_[2.1.0]_enrollment_email_timeouts.up.sql @@ -1,5 +1,7 @@ -- Render the enrollment token/session timeouts dynamically in the "new-account" email. -- See https://github.com/DefGuard/defguard/issues/3518 +-- Only migrate rows still carrying the pre-2.1 default text, so any hand-edited text is preserved. UPDATE mail_context SET text = 'The token is valid for {{ token_timeout }}. Once the enrollment process starts, you have {{ session_timeout }} to complete it.' -WHERE template = 'new-account' AND section = 'token_info' AND language_tag = 'en_US'; +WHERE template = 'new-account' AND section = 'token_info' AND language_tag = 'en_US' + AND text = 'The token is valid for 24 hours. Once the enrollment process starts, you have 10 minutes to complete it.';