diff --git a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.test.ts b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.test.ts index 9ec75d0f..0e29f740 100644 --- a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.test.ts +++ b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.test.ts @@ -1,7 +1,17 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { MfaMethod } from '../../../../shared/rust-api/types'; +import { api } from '../../../../shared/rust-api/api'; +import { + ClientTrafficPolicy, + type InstanceInfo, + MfaMethod, +} from '../../../../shared/rust-api/types'; import { ConfigureMfaStep } from '../types'; -import { applyAuthorization, useConfigureMfaStore } from './useConfigureMfaStore'; +import { + applyAuthorization, + discardMfaConfiguration, + startMfaConfiguration, + useConfigureMfaStore, +} from './useConfigureMfaStore'; vi.mock('@tauri-apps/plugin-log', () => ({ error: vi.fn() })); vi.mock('../../../../shared/rust-api/api', () => ({ api: {} })); @@ -67,3 +77,45 @@ describe('applyAuthorization', () => { expect(useConfigureMfaStore.getState().authorized).toBe(false); }); }); + +describe('startMfaConfiguration', () => { + const instance: InstanceInfo = { + id: 1, + name: 'instance', + uuid: 'instance-uuid', + url: 'https://core.example', + proxy_url: 'https://proxy.example', + active: false, + pubkey: 'pubkey', + client_traffic_policy: ClientTrafficPolicy.None, + enterprise_enabled: false, + disable_tunnels: false, + openid_display_name: null, + mfa_configured_methods: [], + }; + + it('waits for the previous session to end before starting a new one', async () => { + let endPrevious = () => {}; + const ended = new Promise((resolve) => { + endPrevious = resolve; + }); + const mfaConfigStart = vi.fn().mockResolvedValue({ + session_id: 'session-2', + available_methods: [MfaMethod.Totp], + email_fallback: false, + deadline_timestamp: 1_900_000_000, + }); + Object.assign(api, { mfaConfigCancel: vi.fn(() => ended), mfaConfigStart }); + useConfigureMfaStore.setState({ sessionId }); + + void discardMfaConfiguration(); + const started = startMfaConfiguration(instance); + await Promise.resolve(); + expect(mfaConfigStart).not.toHaveBeenCalled(); + + endPrevious(); + await started; + expect(mfaConfigStart).toHaveBeenCalledOnce(); + expect(useConfigureMfaStore.getState().sessionId).toBe('session-2'); + }); +}); diff --git a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.tsx b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.tsx index a71e4b63..855baf99 100644 --- a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.tsx +++ b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.tsx @@ -260,6 +260,8 @@ export const selectPendingMethod = (state: Store): MfaMethodValue | undefined => pendingMethods(state).find((method) => mfaFactorStep(method) === step); +let pendingEnd: Promise = Promise.resolve(); + type StartOptions = Partial & { preselectedMethods?: MfaMethodValue[]; }; @@ -268,6 +270,8 @@ export const startMfaConfiguration = async ( instance: InstanceInfo, { preselectedMethods = [], source = null, location = null }: StartOptions = {}, ): Promise => { + // Core ends every session of the user, so an end still in flight would take this one with it. + await pendingEnd; const response = await api.mfaConfigStart(instance.id); dismissEdgeComsError(); useConfigureMfaStore.getState().start(instance, response, { source, location }); @@ -289,14 +293,20 @@ export const applyAuthorization = ( store.authorize(result); }; -/** A copy the proxy still holds expires on its own, so a failed cancel is not worth raising. */ -export const discardMfaConfiguration = async (): Promise => { - const { sessionId } = useConfigureMfaStore.getState(); - useConfigureMfaStore.getState().reset(); - if (!isPresent(sessionId)) return; +const endSession = async (sessionId: string): Promise => { try { await api.mfaConfigCancel(sessionId); } catch (err) { void logError(`Failed to cancel MFA configuration session: ${err}`); } }; + +/** Also ends the session on Core. One the proxy still holds expires on its own, so a failure is + * not worth raising. */ +export const discardMfaConfiguration = async (): Promise => { + const { sessionId } = useConfigureMfaStore.getState(); + useConfigureMfaStore.getState().reset(); + if (!isPresent(sessionId)) return; + pendingEnd = endSession(sessionId); + await pendingEnd; +}; diff --git a/src-tauri/core/src/mfa_config.rs b/src-tauri/core/src/mfa_config.rs index eb9b3630..4a473a67 100644 --- a/src-tauri/core/src/mfa_config.rs +++ b/src-tauri/core/src/mfa_config.rs @@ -7,8 +7,8 @@ use chrono::Utc; use defguard_client_proto::defguard::client_types::{ CodeMfaSetupFinishRequest, CodeMfaSetupFinishResponse, CodeMfaSetupStartRequest, CodeMfaSetupStartResponse, MfaConfigAuthorizeRequest, MfaConfigAuthorizeResponse, - MfaConfigFido2ChallengeRequest, MfaConfigFido2ChallengeResponse, MfaConfigSendCodeRequest, - MfaConfigStartRequest, MfaConfigStartResponse, MfaMethod, + MfaConfigEndRequest, MfaConfigFido2ChallengeRequest, MfaConfigFido2ChallengeResponse, + MfaConfigSendCodeRequest, MfaConfigStartRequest, MfaConfigStartResponse, MfaMethod, }; use reqwest::{Response, StatusCode, Url}; use serde::{de::DeserializeOwned, Serialize}; @@ -32,6 +32,7 @@ const AUTHORIZE: &str = "api/v1/mfa-config/authorize"; const FIDO2_CHALLENGE: &str = "api/v1/mfa-config/fido2-challenge"; const SETUP_START: &str = "api/v1/mfa-config/setup/start"; const SETUP_FINISH: &str = "api/v1/mfa-config/setup/finish"; +const END: &str = "api/v1/mfa-config/end"; // mirrors the methods Core accepts in mfa_config_authorize, keep in step pub const AUTHORIZING_METHODS: &[MfaMethod] = &[ @@ -439,5 +440,13 @@ pub async fn mfa_config_setup_finish( parse(post(&proxy_url, SETUP_FINISH, &request).await?).await } +/// Lets Core drop the session before its deadline. The proxy answers with an empty 200. +pub async fn mfa_config_end(proxy_url: Url, session_token: String) -> Result<(), MfaConfigError> { + debug!("Ending MFA configuration session"); + let request = MfaConfigEndRequest { session_token }; + post(&proxy_url, END, &request).await?; + Ok(()) +} + #[cfg(test)] mod tests; diff --git a/src-tauri/core/src/mfa_config/tests.rs b/src-tauri/core/src/mfa_config/tests.rs index a76e333c..620f4060 100644 --- a/src-tauri/core/src/mfa_config/tests.rs +++ b/src-tauri/core/src/mfa_config/tests.rs @@ -158,6 +158,22 @@ async fn test_send_code_tolerates_empty_body() { .unwrap(); } +#[tokio::test] +async fn test_end_sends_session_token() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path(format!("/{END}"))) + .and(body_partial_json(json!({ "session_token": SESSION_TOKEN }))) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + + mfa_config_end(mock_url(&server), SESSION_TOKEN.into()) + .await + .unwrap(); +} + #[tokio::test] async fn test_setup_start_returns_totp_secret() { let server = MockServer::start().await; diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 15a24199..e6899648 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -2802,13 +2802,14 @@ fn abort_mfa_config_attempt(state: &AppState, uid: Uuid) { } } -fn cancel_mfa_config_session(state: &AppState, uid: Uuid) { - state +fn cancel_mfa_config_session(state: &AppState, uid: Uuid) -> Option { + let session = state .mfa_config_sessions .lock() .expect("mfa_config_sessions mutex poisoned") .remove(&uid); abort_mfa_config_attempt(state, uid); + session } /// unlike mfa_config_cancel, the session survives for another method @@ -2823,6 +2824,8 @@ pub async fn mfa_config_abort_attempt( Ok(()) } +/// Awaited rather than spawned: Core ends every MFA configuration session of the user, so an +/// end landing after the next `mfa_config_start` would kill that session too. #[tauri::command(async)] pub async fn mfa_config_cancel( session_id: String, @@ -2830,7 +2833,16 @@ pub async fn mfa_config_cancel( ) -> Result<(), String> { debug!("Cancelling MFA configuration session"); let uid = parse_mfa_config_session_id(&session_id)?; - cancel_mfa_config_session(&state, uid); + let Some(session) = cancel_mfa_config_session(&state, uid) else { + return Ok(()); + }; + if session.is_expired(Utc::now().timestamp()) { + return Ok(()); + } + // Best effort, an older proxy lacks the route and the session dies at its deadline anyway. + if let Err(err) = mfa_config::mfa_config_end(session.proxy_url, session.session_token).await { + warn!("Failed to end MFA configuration session: {err}"); + } Ok(()) }