Skip to content

Commit b6ba9dd

Browse files
authored
misc email-related bugfixes (#2832)
* make smtp auth form fields nullable * add test to reproduce setting clearing bug * add a workaround for optional settings not being cleared * add missing snackbars * handle smtp user & pass as trulty optional * make resend button actually resent the email * disable submit button until a valid code is entered * add missing name in MFA activated email * handle mfa code errors * add better error messages for code inputs
1 parent 38d5365 commit b6ba9dd

14 files changed

Lines changed: 173 additions & 22 deletions

File tree

crates/defguard_common/src/db/models/settings.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use rsa::{
1010
traits::PublicKeyParts,
1111
};
1212
use secrecy::ExposeSecret;
13-
use serde::{Deserialize, Serialize};
13+
use serde::{Deserialize, Deserializer, Serialize};
1414
use sqlx::{PgExecutor, PgPool, Type, query, query_as};
1515
use struct_patch::Patch;
1616
use thiserror::Error;
@@ -146,6 +146,27 @@ impl LdapSyncStatus {
146146
}
147147
}
148148

149+
/// Custom deserializer for `Option<T>` fields in patch structs.
150+
///
151+
/// By default serde cannot distinguish between a missing JSON key and an explicit `null` value
152+
/// when deserializing into `Option<Option<T>>` (the shape struct-patch generates for nullable
153+
/// fields) — both map to the outer `None`, causing a PATCH with `"field": null` to be silently
154+
/// ignored instead of clearing the field.
155+
///
156+
/// This deserializer always wraps the result in `Some(...)`, so:
157+
/// - JSON key absent → `None` (handled by `#[serde(default)]` before this is called)
158+
/// - `"field": null` → `Some(None)` → field gets cleared
159+
/// - `"field": "val"` → `Some(Some(v))` → field gets updated
160+
///
161+
/// See <https://github.com/serde-rs/serde/issues/1042> and the struct-patch test suite.
162+
fn deserialize_optional_field<'de, T, D>(deserializer: D) -> Result<Option<Option<T>>, D::Error>
163+
where
164+
D: Deserializer<'de>,
165+
T: Deserialize<'de>,
166+
{
167+
Ok(Some(Option::deserialize(deserializer)?))
168+
}
169+
149170
#[derive(Clone, Deserialize, PartialEq, Patch, Serialize, Default)]
150171
#[patch(attribute(derive(Deserialize, Serialize, Debug)))]
151172
pub struct Settings {
@@ -164,7 +185,9 @@ pub struct Settings {
164185
pub smtp_server: Option<String>,
165186
pub smtp_port: Option<i32>,
166187
pub smtp_encryption: SmtpEncryption,
188+
#[patch(attribute(serde(deserialize_with = "deserialize_optional_field", default)))]
167189
pub smtp_user: Option<String>,
190+
#[patch(attribute(serde(deserialize_with = "deserialize_optional_field", default)))]
168191
pub smtp_password: Option<SecretStringWrapper>,
169192
pub smtp_sender: Option<String>,
170193
// Enrollment
@@ -202,6 +225,7 @@ pub struct Settings {
202225
// Additional object classes for users which determine the added attributes
203226
pub ldap_user_auxiliary_obj_classes: Vec<String>,
204227
// The attribute which is used to map LDAP usernames to Defguard usernames
228+
#[patch(attribute(serde(deserialize_with = "deserialize_optional_field", default)))]
205229
pub ldap_user_rdn_attr: Option<String>,
206230
pub ldap_sync_groups: Vec<String>,
207231
pub ldap_remote_enrollment_enabled: bool,

crates/defguard_core/src/handlers/auth.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -496,6 +496,7 @@ pub async fn webauthn_finish(
496496
&mut conn,
497497
Some(&session.session.into()),
498498
&MFAMethod::Webauthn,
499+
&user.first_name,
499500
)
500501
.await?;
501502
user.set_mfa_method(&mut *conn, MFAMethod::Webauthn).await?;
@@ -658,6 +659,7 @@ pub async fn totp_enable(
658659
&mut conn,
659660
Some(&session.session.into()),
660661
&MFAMethod::OneTimePassword,
662+
&user.first_name,
661663
)
662664
.await?;
663665
user.set_mfa_method(&mut *conn, MFAMethod::OneTimePassword)
@@ -838,6 +840,7 @@ pub async fn email_mfa_enable(
838840
&mut conn,
839841
Some(&session.session.into()),
840842
&MFAMethod::Email,
843+
&user.first_name,
841844
)
842845
.await?;
843846
user.set_mfa_method(&mut *conn, MFAMethod::Email).await?;

crates/defguard_core/tests/integration/api/settings.rs

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,92 @@ async fn test_settings(_: PgPoolOptions, options: PgConnectOptions) {
4848
assert!(new_settings.wireguard_enabled);
4949
}
5050

51+
#[sqlx::test]
52+
async fn test_patch_settings_clears_optional_fields(_: PgPoolOptions, options: PgConnectOptions) {
53+
let pool = setup_pool(options).await;
54+
let (client, _client_state) = make_test_client(pool.clone()).await;
55+
56+
let auth = Auth::new("admin", "pass123");
57+
let response = client.post("/api/v1/auth").json(&auth).send().await;
58+
assert_eq!(response.status(), StatusCode::OK);
59+
60+
// --- smtp_user & smtp_password ---
61+
62+
// set smtp_user and smtp_password (include the required trio so validation passes)
63+
let patch: SettingsPatch = serde_json::from_str(
64+
r#"{
65+
"smtp_server": "smtp.example.com",
66+
"smtp_port": 587,
67+
"smtp_sender": "noreply@example.com",
68+
"smtp_user": "testuser",
69+
"smtp_password": "testpass"
70+
}"#,
71+
)
72+
.unwrap();
73+
let response = client.patch("/api/v1/settings").json(&patch).send().await;
74+
assert_eq!(response.status(), StatusCode::OK);
75+
76+
// verify fields are set
77+
let response = client.get("/api/v1/settings").send().await;
78+
assert_eq!(response.status(), StatusCode::OK);
79+
let settings: Settings = response.json().await;
80+
assert_eq!(
81+
settings.smtp_user,
82+
Some("testuser".to_string()),
83+
"smtp_user should be set after initial PATCH"
84+
);
85+
// smtp_password is redacted in the API response; verify via DB
86+
let from_db = Settings::get(&pool).await.unwrap().unwrap();
87+
assert!(
88+
from_db.smtp_password.is_some(),
89+
"smtp_password should be set in DB after initial PATCH"
90+
);
91+
92+
// clear smtp_user and smtp_password by sending null
93+
let patch: SettingsPatch =
94+
serde_json::from_str(r#"{ "smtp_user": null, "smtp_password": null }"#).unwrap();
95+
let response = client.patch("/api/v1/settings").json(&patch).send().await;
96+
assert_eq!(response.status(), StatusCode::OK);
97+
98+
// assert both fields are cleared in the DB
99+
let from_db = Settings::get(&pool).await.unwrap().unwrap();
100+
assert!(
101+
from_db.smtp_user.is_none(),
102+
"smtp_user should be cleared to None after PATCH with null"
103+
);
104+
assert!(
105+
from_db.smtp_password.is_none(),
106+
"smtp_password should be cleared to None after PATCH with null"
107+
);
108+
109+
// --- ldap_user_rdn_attr ---
110+
111+
// set ldap_user_rdn_attr
112+
let patch: SettingsPatch = serde_json::from_str(r#"{ "ldap_user_rdn_attr": "uid" }"#).unwrap();
113+
let response = client.patch("/api/v1/settings").json(&patch).send().await;
114+
assert_eq!(response.status(), StatusCode::OK);
115+
116+
// verify field is set
117+
let from_db = Settings::get(&pool).await.unwrap().unwrap();
118+
assert_eq!(
119+
from_db.ldap_user_rdn_attr,
120+
Some("uid".to_string()),
121+
"ldap_user_rdn_attr should be set after PATCH"
122+
);
123+
124+
// clear ldap_user_rdn_attr by sending null
125+
let patch: SettingsPatch = serde_json::from_str(r#"{ "ldap_user_rdn_attr": null }"#).unwrap();
126+
let response = client.patch("/api/v1/settings").json(&patch).send().await;
127+
assert_eq!(response.status(), StatusCode::OK);
128+
129+
// assert field is cleared in the DB
130+
let from_db = Settings::get(&pool).await.unwrap().unwrap();
131+
assert!(
132+
from_db.ldap_user_rdn_attr.is_none(),
133+
"ldap_user_rdn_attr should be cleared to None after PATCH with null"
134+
);
135+
}
136+
51137
// JSON fragment containing all required LDAP fields except ldap_url (add that at the call site).
52138
const VALID_LDAP_FIELDS_NO_URL: &str = r#"
53139
"ldap_bind_username": "cn=admin,dc=example,dc=com",

crates/defguard_mail/src/lib.rs

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,29 +21,28 @@ pub(crate) struct SmtpSettings {
2121
server: String,
2222
port: u16,
2323
encryption: SmtpEncryption,
24-
user: String,
25-
password: String,
24+
user: Option<String>,
25+
password: Option<String>,
2626
sender: String,
2727
}
2828

2929
impl SmtpSettings {
3030
/// Constructs `SmtpSettings` from `Settings`. Returns error if `SmtpSettings` are incomplete.
3131
pub(crate) fn from_settings(settings: Settings) -> Result<SmtpSettings, MailError> {
32-
if let (Some(server), Some(port), encryption, Some(user), Some(password), Some(sender)) = (
32+
if let (Some(server), Some(port), Some(sender)) = (
3333
settings.smtp_server,
3434
settings.smtp_port,
35-
settings.smtp_encryption,
36-
settings.smtp_user,
37-
settings.smtp_password,
3835
settings.smtp_sender,
3936
) {
4037
let port = port.try_into().map_err(|_| MailError::InvalidPort(port))?;
4138
Ok(Self {
4239
server,
4340
port,
44-
encryption,
45-
user,
46-
password: password.expose_secret().to_string(),
41+
encryption: settings.smtp_encryption,
42+
user: settings.smtp_user,
43+
password: settings
44+
.smtp_password
45+
.map(|p| p.expose_secret().to_string()),
4746
sender,
4847
})
4948
} else {

crates/defguard_mail/src/mail.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -260,11 +260,11 @@ impl Mail {
260260
.timeout(Some(SMTP_TIMEOUT));
261261

262262
// Skip credentials if any of them is empty
263-
let builder = if settings.user.is_empty() || settings.password.is_empty() {
263+
let builder = if let (Some(user), Some(password)) = (settings.user, settings.password) {
264+
builder.credentials(Credentials::new(user, password))
265+
} else {
264266
debug!("SMTP credentials were not provided, skipping username/password authentication");
265267
builder
266-
} else {
267-
builder.credentials(Credentials::new(settings.user, settings.password))
268268
};
269269

270270
Ok(builder.build())

crates/defguard_mail/src/templates.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -393,9 +393,11 @@ pub async fn mfa_configured_mail(
393393
conn: &mut PgConnection,
394394
session: Option<&SessionContext>,
395395
method: &MFAMethod,
396+
first_name: &str,
396397
) -> Result<(), TemplateError> {
397398
let (mut tera, mut context) = get_base_tera_mjml(Context::new(), session, None, None)?;
398399

400+
context.insert("username", first_name);
399401
context.insert("mfa_method", &method);
400402

401403
let message = MailMessage::MFAConfigured { method: *method };

crates/defguard_mail/src/tests.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,7 @@ fn send_mfa_configured_mail(_: PgPoolOptions, options: PgConnectOptions) {
258258
&mut conn,
259259
None,
260260
&MFAMethod::Email,
261+
"Test",
261262
)
262263
.await
263264
.unwrap();
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
{{ title }}
1+
{{ title }}{% if username %} {{ username }}{% endif %}
22
{% if subtitle %}{{ subtitle }}{% endif %}
33

44
{{ mfa_method_label }} {{ mfa_method }}

crates/defguard_proxy_manager/src/servers/enrollment.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1074,7 +1074,10 @@ impl EnrollmentServer {
10741074
.map_err(|_| Status::internal("Failed to get recovery codes.".to_string()))?
10751075
.ok_or_else(|| Status::internal("Recovery codes not found".to_string()))?;
10761076
if let Ok(mut conn) = self.pool.begin().await {
1077-
if let Err(err) = mfa_configured_mail(&user.email, &mut conn, None, &mfa_method).await {
1077+
if let Err(err) =
1078+
mfa_configured_mail(&user.email, &mut conn, None, &mfa_method, &user.first_name)
1079+
.await
1080+
{
10781081
error!("Failed to send MFA configured email\nReason: {err}");
10791082
}
10801083
} else {

web/src/pages/auth/LoginEmail/LoginEmail.tsx

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
import { useMutation, useQuery } from '@tanstack/react-query';
2+
import type { AxiosError } from 'axios';
23
import type z from 'zod';
34
import { m } from '../../../paraglide/messages';
45
import api from '../../../shared/api/api';
6+
import type { ApiError } from '../../../shared/api/types';
57
import { LoginPage } from '../../../shared/components/LoginPage/LoginPage';
68
import { SizedBox } from '../../../shared/defguard-ui/components/SizedBox/SizedBox';
79
import { ThemeSpacing } from '../../../shared/defguard-ui/types';
10+
import { isPresent } from '../../../shared/defguard-ui/utils/isPresent';
811
import { useAppForm } from '../../../shared/form';
912
import { formChangeLogic } from '../../../shared/formLogic';
1013
import { useAuth } from '../../../shared/hooks/useAuth';
@@ -35,6 +38,18 @@ export const LoginEmail = () => {
3538
onSuccess: (response) => {
3639
useAuth.getState().authSubject.next(response.data);
3740
},
41+
onError: (e: AxiosError<ApiError>) => {
42+
const respCode = e.response?.status;
43+
if (isPresent(respCode) && respCode < 500) {
44+
form.setErrorMap({
45+
onSubmit: {
46+
fields: {
47+
code: respCode === 429 ? m.login_main_attempts_info() : m.form_error_code(),
48+
},
49+
},
50+
});
51+
}
52+
},
3853
});
3954

4055
const form = useAppForm({
@@ -65,6 +80,7 @@ export const LoginEmail = () => {
6580
<form.AppField name="code">
6681
{(field) => (
6782
<field.FormInput
83+
notNull
6884
size="lg"
6985
label={m.form_label_auth_code()}
7086
helper={m.form_helper_auth_code()}

0 commit comments

Comments
 (0)