From 218f7756c6d695e465c99a325478341977e5cfb3 Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Wed, 28 Oct 2026 13:01:12 +0100 Subject: [PATCH 1/4] handle MfaConfigError::SmtpNotConfigured error --- src-tauri/core/src/mfa_config.rs | 21 +++++++- src-tauri/core/src/mfa_config/tests.rs | 67 ++++++++++++++++++++++++++ src-tauri/proto | 2 +- 3 files changed, 87 insertions(+), 3 deletions(-) diff --git a/src-tauri/core/src/mfa_config.rs b/src-tauri/core/src/mfa_config.rs index 279cb2b2..1a7902de 100644 --- a/src-tauri/core/src/mfa_config.rs +++ b/src-tauri/core/src/mfa_config.rs @@ -22,7 +22,7 @@ use tokio_util::sync::CancellationToken; use crate::{ database::models::{instance::MfaCapabilities, Id}, mfa::{OIDC_POLL_INTERVAL, OIDC_POLL_TIMEOUT}, - proxy::{post_with_headers, read_error_message}, + proxy::post_with_headers, }; // No leading slash: it would make `Url::join` discard a proxy base path (`https://host/defguard/`). @@ -175,6 +175,9 @@ pub enum MfaConfigError { #[error("{message}")] NetworkError { message: String }, + #[error("SMTP is not configured on this Defguard instance")] + SmtpNotConfigured, + #[error("Proxy error (HTTP {status}): {message}")] ProxyError { status: u16, message: String }, @@ -300,7 +303,21 @@ async fn check_response(response: Response, endpoint: &str) -> Result().await.ok(); + if body + .as_ref() + .and_then(|body| body.get("code")) + .and_then(serde_json::Value::as_str) + == Some("smtp_not_configured") + { + return Err(MfaConfigError::SmtpNotConfigured); + } + let message = body + .as_ref() + .and_then(|body| body.get("error")) + .and_then(serde_json::Value::as_str) + .map(String::from) + .unwrap_or_else(|| format!("HTTP {status}")); match status { // a wrong code or FIDO2 assertion, any other 401 is a session Core no longer knows StatusCode::UNAUTHORIZED if message == INVALID_CODE_MESSAGE => { diff --git a/src-tauri/core/src/mfa_config/tests.rs b/src-tauri/core/src/mfa_config/tests.rs index 4bfa08f7..78a56aad 100644 --- a/src-tauri/core/src/mfa_config/tests.rs +++ b/src-tauri/core/src/mfa_config/tests.rs @@ -1175,3 +1175,70 @@ async fn test_proxy_base_path_is_preserved() { .await .unwrap(); } + +#[tokio::test] +async fn test_setup_start_preserves_smtp_error_code() { + for (status, body) in [ + ( + 500, + json!({ "error": "Internal server error", "code": "smtp_not_configured" }), + ), + (500, json!({ "code": "smtp_not_configured" })), + ( + 428, + json!({ "error": "different message", "code": "smtp_not_configured" }), + ), + ] { + let server = MockServer::start().await; + mount( + &server, + SETUP_START, + ResponseTemplate::new(status).set_body_json(body), + ) + .await; + let err = mfa_config_setup_start( + mock_url(&server), + SESSION_TOKEN.into(), + MfaMethod::Email, + &ALL_CAPABILITIES, + ) + .await + .unwrap_err(); + + assert!(matches!(err, MfaConfigError::SmtpNotConfigured)); + assert_eq!( + serde_json::to_value(&err).unwrap()["type"], + "smtp_not_configured" + ); + } +} + +#[tokio::test] +async fn test_setup_start_unrecognized_error_code_keeps_proxy_error() { + for body in [ + json!({ "error": "Internal server error" }), + json!({ "error": "Internal server error", "code": null }), + json!({ "error": "Internal server error", "code": "future_code" }), + json!({ "error": "Internal server error", "code": 123 }), + ] { + let server = MockServer::start().await; + mount( + &server, + SETUP_START, + ResponseTemplate::new(500).set_body_json(body), + ) + .await; + let err = mfa_config_setup_start( + mock_url(&server), + SESSION_TOKEN.into(), + MfaMethod::Email, + &ALL_CAPABILITIES, + ) + .await + .unwrap_err(); + + assert!( + matches!(err, MfaConfigError::ProxyError { status: 500, message } if message == "Internal server error") + ); + } +} diff --git a/src-tauri/proto b/src-tauri/proto index 2a39d0a0..6d99508f 160000 --- a/src-tauri/proto +++ b/src-tauri/proto @@ -1 +1 @@ -Subproject commit 2a39d0a09ebed4fe3ea08f1b54be5259ec63bed6 +Subproject commit 6d99508f5522dcfba90b171f1df64a483423cbbe From 798ce1949d10a2603647e40376f390736e382eff Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Wed, 28 Oct 2026 13:34:35 +0100 Subject: [PATCH 2/4] display error modal --- .../hooks/showMfaConfigErrorModal.test.tsx | 58 +++++++ .../hooks/showMfaConfigErrorModal.ts | 18 +++ .../hooks/useMfaConfigErrorHandler.test.ts | 146 ++++++++++++++++++ .../hooks/useMfaConfigErrorHandler.ts | 29 +++- new-ui/src/shared/rust-api/mfaError.ts | 3 + 5 files changed, 247 insertions(+), 7 deletions(-) create mode 100644 new-ui/src/pages/full/ConfigureMfaPage/hooks/showMfaConfigErrorModal.test.tsx create mode 100644 new-ui/src/pages/full/ConfigureMfaPage/hooks/showMfaConfigErrorModal.ts create mode 100644 new-ui/src/pages/full/ConfigureMfaPage/hooks/useMfaConfigErrorHandler.test.ts diff --git a/new-ui/src/pages/full/ConfigureMfaPage/hooks/showMfaConfigErrorModal.test.tsx b/new-ui/src/pages/full/ConfigureMfaPage/hooks/showMfaConfigErrorModal.test.tsx new file mode 100644 index 00000000..d0de2a62 --- /dev/null +++ b/new-ui/src/pages/full/ConfigureMfaPage/hooks/showMfaConfigErrorModal.test.tsx @@ -0,0 +1,58 @@ +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, fireEvent, render, screen } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ConfirmModal } from '../../../../shared/components/ConfirmModal/ConfirmModal'; +import { useConfirmModal } from '../../../../shared/hooks/confirmModal/useConfirmModal'; +import { MfaMethod } from '../../../../shared/rust-api/types'; +import { showMfaConfigErrorModal } from './showMfaConfigErrorModal'; +import { useConfigureMfaStore } from './useConfigureMfaStore'; + +vi.hoisted(() => { + document.body.innerHTML = '
'; +}); +vi.mock('@tauri-apps/plugin-log', () => ({ error: vi.fn() })); +vi.mock('../../../../shared/rust-api/api', () => ({ api: {} })); + +const message = 'Email MFA is unavailable because SMTP is not configured.'; + +describe('MFA configuration error modal', () => { + beforeEach(() => { + useConfirmModal.getState().reset(); + useConfigureMfaStore.getState().reset(); + useConfigureMfaStore.setState({ + sessionId: 'session-1', + selectedMethods: [MfaMethod.Email], + authorized: true, + }); + }); + + it.each(['Got it', 'close button'])( + 'dismisses via %s without resetting the flow', + (action) => { + render( + + + , + ); + act(() => { + showMfaConfigErrorModal(message); + }); + + expect(screen.getByText('MFA configuration error')).toBeTruthy(); + expect(screen.getByText(message)).toBeTruthy(); + expect(screen.queryByText('Cancel')).toBeNull(); + const button = + action === 'Got it' + ? screen.getByRole('button', { name: 'Got it' }) + : screen.getByRole('button', { name: '' }); + fireEvent.click(button); + + expect(useConfirmModal.getState().visible).toBe(false); + expect(useConfigureMfaStore.getState()).toMatchObject({ + sessionId: 'session-1', + selectedMethods: [MfaMethod.Email], + authorized: true, + }); + }, + ); +}); diff --git a/new-ui/src/pages/full/ConfigureMfaPage/hooks/showMfaConfigErrorModal.ts b/new-ui/src/pages/full/ConfigureMfaPage/hooks/showMfaConfigErrorModal.ts new file mode 100644 index 00000000..27d5aabc --- /dev/null +++ b/new-ui/src/pages/full/ConfigureMfaPage/hooks/showMfaConfigErrorModal.ts @@ -0,0 +1,18 @@ +import { ButtonVariant } from '../../../../shared/components/Button/types'; +import { useConfirmModal } from '../../../../shared/hooks/confirmModal/useConfirmModal'; + +export const showMfaConfigErrorModal = (message: string): (() => void) => { + const cancelProps = { text: 'Got it', variant: ButtonVariant.Primary }; + useConfirmModal.getState().open({ + title: 'MFA configuration error', + content: message, + cancelProps, + }); + + return () => { + // a later caller may have replaced this modal before the step unmounts + if (useConfirmModal.getState().cancelProps === cancelProps) { + useConfirmModal.setState({ visible: false }); + } + }; +}; diff --git a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useMfaConfigErrorHandler.test.ts b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useMfaConfigErrorHandler.test.ts new file mode 100644 index 00000000..ab59e1c2 --- /dev/null +++ b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useMfaConfigErrorHandler.test.ts @@ -0,0 +1,146 @@ +import { act, renderHook } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ButtonVariant } from '../../../../shared/components/Button/types'; +import { useConfirmModal } from '../../../../shared/hooks/confirmModal/useConfirmModal'; +import { showEdgeComsError } from '../components/EdgeComsError/useEdgeComsErrorStore'; +import { useMfaConfigErrorHandler } from './useMfaConfigErrorHandler'; + +vi.mock('@tauri-apps/plugin-log', () => ({ error: vi.fn() })); +vi.mock('../components/EdgeComsError/useEdgeComsErrorStore', () => ({ + showEdgeComsError: vi.fn(), +})); + +const renderHandler = (hasCodeInput = true) => { + const setError = vi.fn(); + const onSessionExpired = vi.fn(); + const hook = renderHook(() => + useMfaConfigErrorHandler({ + context: 'Email MFA configuration failed', + setError, + onSessionExpired, + fallback: 'Configuration failed', + hasCodeInput, + }), + ); + return { ...hook, setError, onSessionExpired }; +}; + +const error = (type: string, message?: string) => JSON.stringify({ type, message }); + +describe('useMfaConfigErrorHandler', () => { + beforeEach(() => { + vi.clearAllMocks(); + useConfirmModal.getState().reset(); + }); + + it('shows missing SMTP in an informational modal instead of the code input', () => { + const { result, setError, onSessionExpired } = renderHandler(); + const retry = vi.fn(); + act(() => result.current(error('smtp_not_configured'), retry)); + + expect(useConfirmModal.getState()).toMatchObject({ + visible: true, + title: 'MFA configuration error', + content: + 'Email MFA is unavailable because SMTP is not configured on this Defguard instance. Contact your administrator.', + cancelProps: { text: 'Got it', variant: ButtonVariant.Primary }, + submitProps: null, + }); + expect(setError).not.toHaveBeenCalled(); + expect(onSessionExpired).not.toHaveBeenCalled(); + expect(retry).not.toHaveBeenCalled(); + }); + + it('shows a safe modal for a server error without exposing backend details', () => { + const { result, setError } = renderHandler(); + act(() => + result.current( + JSON.stringify({ type: 'proxy_error', status: 500, message: 'private details' }), + ), + ); + + expect(useConfirmModal.getState()).toMatchObject({ + visible: true, + content: + 'Could not complete MFA configuration. Please try again or contact your administrator.', + }); + expect(setError).not.toHaveBeenCalled(); + }); + + it.each([new Error('private details'), error('future_error', 'private details')])( + 'uses the caller fallback in a modal for an unknown error', + (err) => { + const { result, setError } = renderHandler(); + act(() => result.current(err)); + + expect(useConfirmModal.getState()).toMatchObject({ + visible: true, + content: 'Configuration failed', + }); + expect(setError).not.toHaveBeenCalled(); + }, + ); + + it('keeps invalid codes inline', () => { + const { result, setError } = renderHandler(); + act(() => result.current(error('invalid_code', 'invalid code'))); + + expect(setError).toHaveBeenCalledWith('Invalid code'); + expect(useConfirmModal.getState().visible).toBe(false); + }); + + it('preserves rejections without a code input', () => { + const { result, setError } = renderHandler(false); + act(() => result.current(error('invalid_code', 'Verification rejected'))); + + expect(setError).toHaveBeenCalledWith('Verification rejected'); + expect(useConfirmModal.getState().visible).toBe(false); + }); + + it('preserves session expiration handling', () => { + const { result, onSessionExpired } = renderHandler(); + act(() => result.current(error('session_expired'))); + + expect(onSessionExpired).toHaveBeenCalledOnce(); + expect(useConfirmModal.getState().visible).toBe(false); + }); + + it('preserves the network error screen and retry callback', () => { + const { result, setError } = renderHandler(); + const retry = vi.fn(); + act(() => result.current(error('network_error'), retry)); + + expect(showEdgeComsError).toHaveBeenCalledWith(retry); + expect(setError).not.toHaveBeenCalled(); + expect(useConfirmModal.getState().visible).toBe(false); + }); + + it('ignores cancellation', () => { + const { result, setError, onSessionExpired } = renderHandler(); + act(() => result.current(error('cancelled'))); + + expect(setError).not.toHaveBeenCalled(); + expect(onSessionExpired).not.toHaveBeenCalled(); + expect(useConfirmModal.getState().visible).toBe(false); + }); + + it('dismisses its modal when the step unmounts', () => { + const { result, unmount } = renderHandler(); + act(() => result.current(error('smtp_not_configured'))); + unmount(); + + expect(useConfirmModal.getState().visible).toBe(false); + }); + + it('does not dismiss a modal opened by another caller', () => { + const { result, unmount } = renderHandler(); + act(() => result.current(error('smtp_not_configured'))); + act(() => useConfirmModal.getState().open({ title: 'Another modal' })); + unmount(); + + expect(useConfirmModal.getState()).toMatchObject({ + visible: true, + title: 'Another modal', + }); + }); +}); diff --git a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useMfaConfigErrorHandler.ts b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useMfaConfigErrorHandler.ts index ad95430c..1ef5a204 100644 --- a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useMfaConfigErrorHandler.ts +++ b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useMfaConfigErrorHandler.ts @@ -1,5 +1,5 @@ import { error as logError } from '@tauri-apps/plugin-log'; -import { useCallback } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import { isMfaConfigAlreadyAuthorized, isMfaConfigCancelled, @@ -11,10 +11,12 @@ import { isMfaConfigProxyError, isMfaConfigSecurityKeyError, isMfaConfigSessionExpired, + isMfaConfigSmtpNotConfigured, isMfaConfigTimeout, mfaErrorMessage, } from '../../../../shared/rust-api/mfaError'; import { showEdgeComsError } from '../components/EdgeComsError/useEdgeComsErrorStore'; +import { showMfaConfigErrorModal } from './showMfaConfigErrorModal'; type Options = { context: string; @@ -34,8 +36,16 @@ export const useMfaConfigErrorHandler = ({ onSessionExpired, fallback, hasCodeInput = true, -}: Options) => - useCallback( +}: Options) => { + const dismissModalRef = useRef<(() => void) | null>(null); + + useEffect(() => { + return () => { + dismissModalRef.current?.(); + }; + }, []); + + return useCallback( (err: unknown, retry?: () => void) => { // A cancel is the user's own doing, so it is neither logged nor shown. if (isMfaConfigCancelled(err)) { @@ -83,11 +93,16 @@ export const useMfaConfigErrorHandler = ({ showEdgeComsError(retry); return; } - if (isMfaConfigProxyError(err)) { - setError('Service temporarily unavailable, try again.'); - return; + let message = fallback; + if (isMfaConfigSmtpNotConfigured(err)) { + message = + 'Email MFA is unavailable because SMTP is not configured on this Defguard instance. Contact your administrator.'; + } else if (isMfaConfigProxyError(err)) { + message = + 'Could not complete MFA configuration. Please try again or contact your administrator.'; } - setError(fallback); + dismissModalRef.current = showMfaConfigErrorModal(message); }, [context, setError, onSessionExpired, fallback, hasCodeInput], ); +}; diff --git a/new-ui/src/shared/rust-api/mfaError.ts b/new-ui/src/shared/rust-api/mfaError.ts index 64411d43..cdaceb0a 100644 --- a/new-ui/src/shared/rust-api/mfaError.ts +++ b/new-ui/src/shared/rust-api/mfaError.ts @@ -119,6 +119,9 @@ export const isMfaConfigNetworkError = (err: unknown): boolean => export const isMfaConfigProxyError = (err: unknown): boolean => parseMfaError(err)?.type === 'proxy_error'; +export const isMfaConfigSmtpNotConfigured = (err: unknown): boolean => + parseMfaError(err)?.type === 'smtp_not_configured'; + /** Describes an OIDC polling error and its user-facing message. */ export type OidcPollFailure = { kind: From f6f837fa31577aabb456f367975504c5d051b924 Mon Sep 17 00:00:00 2001 From: Jacek Chmielewski Date: Wed, 28 Oct 2026 14:13:21 +0100 Subject: [PATCH 3/4] fix mobile approve error modal layout --- .../LocationCardConnectionErrorView.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/new-ui/src/shared/components/LocationCard/views/LocationCardConnectionErrorView/LocationCardConnectionErrorView.tsx b/new-ui/src/shared/components/LocationCard/views/LocationCardConnectionErrorView/LocationCardConnectionErrorView.tsx index ed3379b4..9f34bea9 100644 --- a/new-ui/src/shared/components/LocationCard/views/LocationCardConnectionErrorView/LocationCardConnectionErrorView.tsx +++ b/new-ui/src/shared/components/LocationCard/views/LocationCardConnectionErrorView/LocationCardConnectionErrorView.tsx @@ -23,13 +23,12 @@ export const LocationCardConnectionErrorView = () => { )}

{connectionError ?? DEFAULT_CONNECTION_ERROR}

- +