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 0e29f740..96e7f389 100644 --- a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.test.ts +++ b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.test.ts @@ -19,10 +19,30 @@ vi.mock('../../../../shared/rust-api/api', () => ({ api: {} })); const sessionId = 'session-1'; const authorizeResult = { deadline_timestamp: 1_900_000_000, recovery_codes: [] }; +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: [], + mfa_capabilities: { + setup_methods: [MfaMethod.Totp, MfaMethod.Fido2], + authorize_methods: [MfaMethod.Totp], + }, +}; + describe('applyAuthorization', () => { beforeEach(() => { useConfigureMfaStore.getState().reset(); useConfigureMfaStore.setState({ + instance, sessionId, configuredMethods: [MfaMethod.Email], verificationMethods: [MfaMethod.Email], @@ -79,19 +99,12 @@ describe('applyAuthorization', () => { }); 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: [], + const startResult = { + session_id: 'session-2', + available_methods: [MfaMethod.Totp], + configured_methods: [MfaMethod.Totp], + email_fallback: false, + deadline_timestamp: 1_900_000_000, }; it('waits for the previous session to end before starting a new one', async () => { @@ -99,12 +112,7 @@ describe('startMfaConfiguration', () => { 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, - }); + const mfaConfigStart = vi.fn().mockResolvedValue(startResult); Object.assign(api, { mfaConfigCancel: vi.fn(() => ended), mfaConfigStart }); useConfigureMfaStore.setState({ sessionId }); @@ -118,4 +126,30 @@ describe('startMfaConfiguration', () => { expect(mfaConfigStart).toHaveBeenCalledOnce(); expect(useConfigureMfaStore.getState().sessionId).toBe('session-2'); }); + + it('drops preselected factors the instance cannot set up', async () => { + Object.assign(api, { mfaConfigStart: vi.fn().mockResolvedValue(startResult) }); + + await startMfaConfiguration(instance, { + preselectedMethods: [MfaMethod.Email, MfaMethod.Fido2], + }); + + expect(useConfigureMfaStore.getState().initialSelection).toEqual([MfaMethod.Fido2]); + }); + + it('does not offer a configured factor the instance cannot authorize with', async () => { + Object.assign(api, { + mfaConfigStart: vi.fn().mockResolvedValue({ + ...startResult, + available_methods: [MfaMethod.Fido2], + configured_methods: [MfaMethod.Totp, MfaMethod.Fido2], + }), + }); + + await startMfaConfiguration(instance, { preselectedMethods: [MfaMethod.Totp] }); + + const state = useConfigureMfaStore.getState(); + expect(state.configuredMethods).toContain(MfaMethod.Totp); + expect(state.initialSelection).toEqual([]); + }); }); diff --git a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.tsx b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.tsx index 855baf99..6fab9a33 100644 --- a/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.tsx +++ b/new-ui/src/pages/full/ConfigureMfaPage/hooks/useConfigureMfaStore.tsx @@ -13,6 +13,7 @@ import { type MfaMethodValue, } from '../../../../shared/rust-api/types'; import { isPresent } from '../../../../shared/utils/isPresent'; +import { setupMethodsOf } from '../../../../shared/utils/mfa'; import { dismissEdgeComsError } from '../components/EdgeComsError/useEdgeComsErrorStore'; import { ConfigureMfaStep, @@ -62,7 +63,11 @@ type StoreValues = { type FlowState = Pick< StoreValues, - 'configuredMethods' | 'completedMethods' | 'selectedMethods' | 'recoveryCodes' + | 'instance' + | 'configuredMethods' + | 'completedMethods' + | 'selectedMethods' + | 'recoveryCodes' >; /** Picked factors still to set up, in wizard order. */ @@ -71,7 +76,11 @@ const pendingMethods = (state: FlowState): MfaMethodValue[] => (method) => !state.completedMethods.includes(method) && // Guards against a pick the selection screen should already have refused. - isMfaFactorOfferable(method, state.configuredMethods), + isMfaFactorOfferable( + method, + state.configuredMethods, + setupMethodsOf(state.instance), + ), ) ?? []; /** Setup steps with a factor still pending, plus the closing steps that have something to show. */ @@ -146,10 +155,14 @@ export const useConfigureMfaStore = create()( const sessionMethods = response.email_fallback ? [MfaMethod.Email] : response.available_methods; + // a factor the instance cannot authorize with is still configured + const sessionConfiguredMethods = response.email_fallback + ? [MfaMethod.Email] + : response.configured_methods; // the session and the snapshot may both list FIDO2 const configuredMethods = [ ...new Set([ - ...sessionMethods, + ...sessionConfiguredMethods, ...(instance.mfa_configured_methods ?? []).filter( (method) => !isCodeMfaMethod(method), ), @@ -249,7 +262,7 @@ export const useConfigureMfaStore = create()( name: 'configure-mfa-store', storage: createJSONStorage(() => sessionStorage), // Bumped on every shape change: a stored session is never resumable across one. - version: 12, + version: 13, }, ), ); @@ -276,8 +289,9 @@ export const startMfaConfiguration = async ( dismissEdgeComsError(); useConfigureMfaStore.getState().start(instance, response, { source, location }); // Only pre-ticks, the user still confirms in the selection step. + const { configuredMethods } = useConfigureMfaStore.getState(); const initialSelection = preselectedMethods.filter((method) => - isMfaFactorOfferable(method, useConfigureMfaStore.getState().configuredMethods), + isMfaFactorOfferable(method, configuredMethods, setupMethodsOf(instance)), ); useConfigureMfaStore.setState({ initialSelection }); }; diff --git a/new-ui/src/pages/full/ConfigureMfaPage/utils.ts b/new-ui/src/pages/full/ConfigureMfaPage/utils.ts index a2c3c2f8..95ee508a 100644 --- a/new-ui/src/pages/full/ConfigureMfaPage/utils.ts +++ b/new-ui/src/pages/full/ConfigureMfaPage/utils.ts @@ -20,19 +20,20 @@ const WIZARD_FACTORS: Record }; /** In selection and wizard order. */ -export const MFA_CONFIGURABLE_FACTORS: MfaFactor[] = CLIENT_CONFIGURABLE_METHODS.map( - (method) => ({ method, ...WIZARD_FACTORS[method] }), -); +const configurableFactors: MfaFactor[] = CLIENT_CONFIGURABLE_METHODS.map((method) => ({ + method, + ...WIZARD_FACTORS[method], +})); export const mfaFactor = (method: MfaMethodValue): MfaFactor | undefined => - MFA_CONFIGURABLE_FACTORS.find((factor) => factor.method === method); + configurableFactors.find((factor) => factor.method === method); export const mfaFactorStep = ( method: MfaMethodValue, ): ConfigureMfaStepValue | undefined => mfaFactor(method)?.step; export const isMfaSetupStep = (step: ConfigureMfaStepValue): boolean => - MFA_CONFIGURABLE_FACTORS.some((factor) => factor.step === step); + configurableFactors.some((factor) => factor.step === step); export const mfaStepsOf = (methods: MfaMethodValue[]): ConfigureMfaStepValue[] => MFA_WIZARD_STEPS.filter((step) => @@ -42,9 +43,10 @@ export const mfaStepsOf = (methods: MfaMethodValue[]): ConfigureMfaStepValue[] = export const isMfaFactorOfferable = ( method: MfaMethodValue, configuredMethods: MfaMethodValue[], + setupMethods: MfaMethodValue[], ): boolean => { const factor = mfaFactor(method); - if (!factor) return false; + if (!factor || !setupMethods.includes(method)) return false; return factor.repeatable || !configuredMethods.includes(method); }; diff --git a/new-ui/src/pages/full/ConfigureMfaPage/verify/ConfigureSelectMethodsStep/ConfigureSelectMethodsStep.tsx b/new-ui/src/pages/full/ConfigureMfaPage/verify/ConfigureSelectMethodsStep/ConfigureSelectMethodsStep.tsx index e3f70323..9647ab4f 100644 --- a/new-ui/src/pages/full/ConfigureMfaPage/verify/ConfigureSelectMethodsStep/ConfigureSelectMethodsStep.tsx +++ b/new-ui/src/pages/full/ConfigureMfaPage/verify/ConfigureSelectMethodsStep/ConfigureSelectMethodsStep.tsx @@ -15,15 +15,18 @@ import { import { ThemeSpacing } from '../../../../../shared/types'; import { isPresent } from '../../../../../shared/utils/isPresent'; import { + CLIENT_CONFIGURABLE_METHODS, + isClientConfigurableMethod, isDesktopDrivableMethod, mfaStepsOf as locationMfaSteps, mfaToText, + setupMethodsOf, } from '../../../../../shared/utils/mfa'; import { discardMfaConfiguration, useConfigureMfaStore, } from '../../hooks/useConfigureMfaStore'; -import { isMfaFactorOfferable, MFA_CONFIGURABLE_FACTORS } from '../../utils'; +import { isMfaFactorOfferable } from '../../utils'; import '../style.scss'; import './style.scss'; import { MethodRow, type MethodRowState } from './components/MethodRow'; @@ -32,19 +35,18 @@ interface Props { onCancel: () => void; } -const CLIENT_CONFIGURABLE_METHODS = MFA_CONFIGURABLE_FACTORS.map( - (factor) => factor.method, -); - export const ConfigureSelectMethodsStep = ({ onCancel }: Props) => { const configuredMethods = useConfigureMfaStore((s) => s.configuredMethods); const emailFallback = useConfigureMfaStore((s) => s.emailFallback); const location = useConfigureMfaStore((s) => s.location); const initialSelection = useConfigureMfaStore((s) => s.initialSelection); + const instance = useConfigureMfaStore((s) => s.instance); + + const setupMethods = useMemo(() => setupMethodsOf(instance), [instance]); // Listing order, matching what `toggle` keeps. const [selected, setSelected] = useState(() => - CLIENT_CONFIGURABLE_METHODS.filter((method) => initialSelection.includes(method)), + setupMethods.filter((method) => initialSelection.includes(method)), ); const [error, setError] = useState(null); @@ -74,7 +76,7 @@ export const ConfigureSelectMethodsStep = ({ onCancel }: Props) => { ); const groups = useMemo(() => { - let result = [CLIENT_CONFIGURABLE_METHODS]; + let result: MfaMethodValue[][] = [[...CLIENT_CONFIGURABLE_METHODS]]; if (isLocationAware) { result = locationSteps.map((step) => step.methods.map((entry) => entry.method)); } @@ -84,18 +86,18 @@ export const ConfigureSelectMethodsStep = ({ onCancel }: Props) => { return [ [...result[0]].sort( (a, b) => - Number(!isMfaFactorOfferable(a, configuredMethods)) - - Number(!isMfaFactorOfferable(b, configuredMethods)), + Number(!isMfaFactorOfferable(a, configuredMethods, setupMethods)) - + Number(!isMfaFactorOfferable(b, configuredMethods, setupMethods)), ), ]; } return result; - }, [isLocationAware, locationSteps, configuredMethods]); + }, [isLocationAware, locationSteps, configuredMethods, setupMethods]); const describeMethod = useCallback( (method: MfaMethodValue): MethodRowState => { // A repeatable factor stays offerable once configured, keeping its badge and its pick. - const disabled = !isMfaFactorOfferable(method, configuredMethods); + const disabled = !isMfaFactorOfferable(method, configuredMethods, setupMethods); const configured = accountConfigured.includes(method); const satisfied = configuredMethods.includes(method); @@ -104,6 +106,8 @@ export const ConfigureSelectMethodsStep = ({ onCancel }: Props) => { if (satisfied) { // Reached only via the email fallback, which registers the factor as it verifies. hint = `${mfaToText(method)} is required to continue.`; + } else if (isClientConfigurableMethod(method)) { + hint = `This Defguard instance does not support configuring ${mfaToText(method)} from the desktop client.`; } else { hint = `${mfaToText(method)} cannot be configured in the desktop client.`; } @@ -121,7 +125,7 @@ export const ConfigureSelectMethodsStep = ({ onCancel }: Props) => { hint, }; }, - [accountConfigured, configuredMethods, selected], + [accountConfigured, configuredMethods, setupMethods, selected], ); const { mutate: cancel, isPending: isCancelling } = useMutation({ @@ -129,18 +133,21 @@ export const ConfigureSelectMethodsStep = ({ onCancel }: Props) => { onSettled: onCancel, }); - const toggle = useCallback((method: MfaMethodValue) => { - setError(null); - // Kept in listing order, which is the order the wizard sets them up in. - setSelected((current) => { - if (current.includes(method)) { - return current.filter((picked) => picked !== method); - } - return CLIENT_CONFIGURABLE_METHODS.filter( - (candidate) => candidate === method || current.includes(candidate), - ); - }); - }, []); + const toggle = useCallback( + (method: MfaMethodValue) => { + setError(null); + // Kept in listing order, which is the order the wizard sets them up in. + setSelected((current) => { + if (current.includes(method)) { + return current.filter((picked) => picked !== method); + } + return setupMethods.filter( + (candidate) => candidate === method || current.includes(candidate), + ); + }); + }, + [setupMethods], + ); const handleSubmit = useCallback(() => { if (isLocationAware) { diff --git a/new-ui/src/pages/playground/components/PlaygroundTestMfaSettingsSection/PlaygroundTestMfaSettingsSection.tsx b/new-ui/src/pages/playground/components/PlaygroundTestMfaSettingsSection/PlaygroundTestMfaSettingsSection.tsx index f8a78081..6efea708 100644 --- a/new-ui/src/pages/playground/components/PlaygroundTestMfaSettingsSection/PlaygroundTestMfaSettingsSection.tsx +++ b/new-ui/src/pages/playground/components/PlaygroundTestMfaSettingsSection/PlaygroundTestMfaSettingsSection.tsx @@ -53,6 +53,10 @@ const singleStepLocation: MfaSettingsLocation = { const instance: MfaSettingsInstance = { mfa_configured_methods: [MfaMethod.Totp, MfaMethod.Oidc, MfaMethod.Fido2], + mfa_capabilities: { + setup_methods: [MfaMethod.Totp, MfaMethod.Email, MfaMethod.Fido2], + authorize_methods: [MfaMethod.Totp, MfaMethod.Email, MfaMethod.Fido2, MfaMethod.Oidc], + }, openid_display_name: null, }; diff --git a/new-ui/src/shared/components/MfaSettingsSection/types.ts b/new-ui/src/shared/components/MfaSettingsSection/types.ts index d9647c63..07535d5d 100644 --- a/new-ui/src/shared/components/MfaSettingsSection/types.ts +++ b/new-ui/src/shared/components/MfaSettingsSection/types.ts @@ -8,7 +8,7 @@ export type MfaSettingsLocation = Pick< export type MfaSettingsInstance = Pick< InstanceInfo, - 'mfa_configured_methods' | 'openid_display_name' + 'mfa_configured_methods' | 'mfa_capabilities' | 'openid_display_name' >; /** What clicking a factor row does. */ diff --git a/new-ui/src/shared/components/MfaSettingsSection/utils.test.ts b/new-ui/src/shared/components/MfaSettingsSection/utils.test.ts index 68c6697c..de3e7e3e 100644 --- a/new-ui/src/shared/components/MfaSettingsSection/utils.test.ts +++ b/new-ui/src/shared/components/MfaSettingsSection/utils.test.ts @@ -28,8 +28,14 @@ const threeSteps = [ step([MfaMethod.Fido2, true], [MfaMethod.MobileApprove, false]), ]; +const capabilities = { + setup_methods: [MfaMethod.Totp, MfaMethod.Email, MfaMethod.Fido2], + authorize_methods: [MfaMethod.Totp, MfaMethod.Email, MfaMethod.Fido2, MfaMethod.Oidc], +}; + const instance = { mfa_configured_methods: [MfaMethod.Totp, MfaMethod.Oidc, MfaMethod.Fido2], + mfa_capabilities: capabilities, openid_display_name: null, }; @@ -99,10 +105,22 @@ describe('mfaSettingsStepsOf', () => { expect(steps[2].factors[1].action).toBe(MfaFactorAction.None); }); - it('never offers configuration on an instance that does not report its factors', () => { + it('never offers configuration on an instance that cannot configure from the client', () => { + const steps = mfaSettingsStepsOf({ + location: locationOf(threeSteps), + instance: { ...instance, mfa_capabilities: null }, + configurable: true, + }); + expect(steps[0].factors[1].action).toBe(MfaFactorAction.None); + }); + + it('never offers a factor the instance cannot set up', () => { const steps = mfaSettingsStepsOf({ location: locationOf(threeSteps), - instance: { mfa_configured_methods: null, openid_display_name: null }, + instance: { + ...instance, + mfa_capabilities: { ...capabilities, setup_methods: [MfaMethod.Totp] }, + }, configurable: true, }); expect(steps[0].factors[1].action).toBe(MfaFactorAction.None); diff --git a/new-ui/src/shared/components/MfaSettingsSection/utils.ts b/new-ui/src/shared/components/MfaSettingsSection/utils.ts index ea30887e..f8b5b038 100644 --- a/new-ui/src/shared/components/MfaSettingsSection/utils.ts +++ b/new-ui/src/shared/components/MfaSettingsSection/utils.ts @@ -1,8 +1,7 @@ import type { MfaStepMethod } from '../../rust-api/types'; import { isPresent } from '../../utils/isPresent'; import { - isClientConfigurableMethod, - isDesktopDrivableMethod, + canSetUpMfaMethod, isMfaMethodConfigured, isMfaMethodUsable, mfaStepsOf, @@ -24,14 +23,9 @@ const mfaFactorActionOf = ( ): MfaFactorActionValue => { if (isMfaMethodUsable(entry, instance)) return MfaFactorAction.Pick; - // Like `connectionAbilityOf`, an instance without reported factors cannot configure. - const canConfigure = - configurable && - isPresent(instance?.mfa_configured_methods) && - isDesktopDrivableMethod(entry.method) && - isClientConfigurableMethod(entry.method); - - return canConfigure ? MfaFactorAction.Configure : MfaFactorAction.None; + return configurable && canSetUpMfaMethod(entry.method, instance) + ? MfaFactorAction.Configure + : MfaFactorAction.None; }; export const mfaSettingsStepsOf = ({ diff --git a/new-ui/src/shared/hooks/useConnectionAbility.ts b/new-ui/src/shared/hooks/useConnectionAbility.ts index 450369cd..48701a37 100644 --- a/new-ui/src/shared/hooks/useConnectionAbility.ts +++ b/new-ui/src/shared/hooks/useConnectionAbility.ts @@ -6,6 +6,6 @@ import { type ConnectionAbilityValue, connectionAbilityOf } from '../utils/mfa'; * as `connectionAbility`; cards outside that context call this directly. */ export const useConnectionAbility = ( location: Pick, - instance?: Pick, + instance?: Pick, ): ConnectionAbilityValue => useMemo(() => connectionAbilityOf(location, instance), [location, instance]); diff --git a/new-ui/src/shared/rust-api/query.ts b/new-ui/src/shared/rust-api/query.ts index 1d8a69c4..a820b4d8 100644 --- a/new-ui/src/shared/rust-api/query.ts +++ b/new-ui/src/shared/rust-api/query.ts @@ -1,6 +1,7 @@ import { queryOptions, skipToken } from '@tanstack/react-query'; import { isPresent } from '../utils/isPresent'; +import { setupMethodsOf } from '../utils/mfa'; import { api } from './api'; import type { ConnectionArgs, @@ -16,9 +17,8 @@ import type { export const tunnelsDisabled = (instances: InstanceInfo[]): boolean => instances.some((i) => i.disable_tunnels); -/** An instance that never reported its MFA state predates the configuration API. */ export const supportsMfaConfiguration = (instance: InstanceInfo): boolean => - isPresent(instance.mfa_configured_methods); + setupMethodsOf(instance).length > 0; /** Shared by the Add page, its route guard and the instance picker, so all three count alike. */ export const mfaConfigurableInstances = (instances: InstanceInfo[]): InstanceInfo[] => diff --git a/new-ui/src/shared/rust-api/types.ts b/new-ui/src/shared/rust-api/types.ts index 0c58f2f3..d410922f 100644 --- a/new-ui/src/shared/rust-api/types.ts +++ b/new-ui/src/shared/rust-api/types.ts @@ -291,6 +291,14 @@ export type InstanceInfo = { openid_display_name: string | null; /** Factors set up on the account, as last reported. Null when the instance predates the API. */ mfa_configured_methods: MfaMethodValue[] | null; + /** Null when this Core cannot configure MFA from the client. */ + mfa_capabilities: MfaCapabilities | null; +}; + +/** What Core accepts, static per Core version. Offer only what this client also drives. */ +export type MfaCapabilities = { + setup_methods: MfaMethodValue[]; + authorize_methods: MfaMethodValue[]; }; export type MfaStepMethod = { @@ -558,10 +566,12 @@ export type MfaSetupFinishResult = { recovery_codes: string[]; }; -/** Result from mfa_config_start. `available_methods` holds only factors that can authorize. */ +/** Result from mfa_config_start. `available_methods` holds only factors that can authorize, + * `configured_methods` every factor Core reported, whether or not it can authorize. */ export type MfaConfigStartResult = { session_id: string; available_methods: MfaMethodValue[]; + configured_methods: MfaMethodValue[]; email_fallback: boolean; deadline_timestamp: number; }; diff --git a/new-ui/src/shared/utils/mfa.test.ts b/new-ui/src/shared/utils/mfa.test.ts index 7d84e41d..e79b49d5 100644 --- a/new-ui/src/shared/utils/mfa.test.ts +++ b/new-ui/src/shared/utils/mfa.test.ts @@ -1,11 +1,18 @@ import { describe, expect, it } from 'vitest'; import { ConnectionType, + type MfaCapabilities, MfaMethod, type MfaMethodValue, type MfaStep, } from '../rust-api/types'; -import { hasMfaMethodChoice } from './mfa'; +import { + ConnectionAbility, + canSetUpMfaMethod, + connectionAbilityOf, + hasMfaMethodChoice, + setupMethodsOf, +} from './mfa'; const step = (...methods: MfaMethodValue[]): MfaStep => ({ methods: methods.map((method) => ({ method, configured: true })), @@ -47,3 +54,66 @@ describe('hasMfaMethodChoice', () => { ).toBe(false); }); }); + +const capabilitiesOf = (...setup_methods: MfaMethodValue[]): MfaCapabilities => ({ + setup_methods, + authorize_methods: [], +}); + +describe('setupMethodsOf', () => { + it('is empty for an instance that cannot configure from the client', () => { + expect(setupMethodsOf({ mfa_capabilities: null })).toEqual([]); + expect(setupMethodsOf(undefined)).toEqual([]); + }); + + it('keeps the client order and drops what the client cannot set up', () => { + const instance = { + mfa_capabilities: capabilitiesOf( + MfaMethod.Fido2, + MfaMethod.MobileApprove, + MfaMethod.Totp, + ), + }; + expect(setupMethodsOf(instance)).toEqual([MfaMethod.Totp, MfaMethod.Fido2]); + }); +}); + +describe('canSetUpMfaMethod', () => { + it('needs both the client and the instance', () => { + const instance = { mfa_capabilities: capabilitiesOf(MfaMethod.Totp, MfaMethod.Oidc) }; + expect(canSetUpMfaMethod(MfaMethod.Totp, instance)).toBe(true); + expect(canSetUpMfaMethod(MfaMethod.Email, instance)).toBe(false); + expect(canSetUpMfaMethod(MfaMethod.Oidc, instance)).toBe(false); + }); +}); + +describe('connectionAbilityOf', () => { + const unconfiguredStep: MfaStep = { + methods: [ + { method: MfaMethod.Email, configured: false }, + { method: MfaMethod.Biometric, configured: false }, + ], + }; + const location = locationOf([unconfiguredStep]); + + it('is configurable when the instance can set up a blocking factor', () => { + const instance = { + mfa_configured_methods: [], + mfa_capabilities: capabilitiesOf(MfaMethod.Email), + }; + expect(connectionAbilityOf(location, instance)).toBe(ConnectionAbility.Configurable); + }); + + it('is unavailable when the instance cannot set up any blocking factor', () => { + const instance = { + mfa_configured_methods: [], + mfa_capabilities: capabilitiesOf(MfaMethod.Totp), + }; + expect(connectionAbilityOf(location, instance)).toBe(ConnectionAbility.Unavailable); + }); + + it('is unavailable when the instance cannot configure from the client', () => { + const instance = { mfa_configured_methods: [], mfa_capabilities: null }; + expect(connectionAbilityOf(location, instance)).toBe(ConnectionAbility.Unavailable); + }); +}); diff --git a/new-ui/src/shared/utils/mfa.ts b/new-ui/src/shared/utils/mfa.ts index 86941b11..2f356c83 100644 --- a/new-ui/src/shared/utils/mfa.ts +++ b/new-ui/src/shared/utils/mfa.ts @@ -153,14 +153,31 @@ export const isClientConfigurableMethod = ( ): method is ClientConfigurableMethod => CLIENT_CONFIGURABLE_METHODS.some((candidate) => candidate === method); +type MfaCapabilitiesInstance = Pick; + +/** in client order, which the picker and wizard rely on */ +export const setupMethodsOf = ( + instance?: MfaCapabilitiesInstance | null, +): ClientConfigurableMethod[] => { + const coreSetupMethods = instance?.mfa_capabilities?.setup_methods ?? []; + return CLIENT_CONFIGURABLE_METHODS.filter((method) => + coreSetupMethods.includes(method), + ); +}; + +export const canSetUpMfaMethod = ( + method: MfaMethodValue, + instance?: MfaCapabilitiesInstance | null, +): method is ClientConfigurableMethod => + setupMethodsOf(instance).some((candidate) => candidate === method); + /** How far the user can get connecting this location with the factors they hold. */ export const ConnectionAbility = { /** A whole path through the steps runs on factors already on the account. */ Available: 'available', - /** Blocked, but every blocking step offers a factor this client can set up. */ + /** Blocked, but every blocking step offers a factor this client can set up there. */ Configurable: 'configurable', - /** Blocked on a factor the client cannot set up - the mobile client's, or an - * instance too old to configure factors from here. */ + /** Blocked on a factor that cannot be set up from this client on this instance. */ Unavailable: 'unavailable', } as const; @@ -174,20 +191,15 @@ export type ConnectionAbilityValue = */ export const connectionAbilityOf = ( location: Pick, - instance?: Pick, + instance?: Pick, ): ConnectionAbilityValue => { const blockedSteps = mfaStepsOf(location).filter( (step) => usableMfaMethods(step, instance).length === 0, ); if (blockedSteps.length === 0) return ConnectionAbility.Available; - // An instance that never reported its factors cannot configure them from here. - if (!isPresent(instance?.mfa_configured_methods)) return ConnectionAbility.Unavailable; - const isFixable = (step: MfaStep): boolean => - step.methods.some( - (entry) => isDesktopDrivable(entry) && isClientConfigurableMethod(entry.method), - ); + step.methods.some((entry) => canSetUpMfaMethod(entry.method, instance)); return blockedSteps.every(isFixable) ? ConnectionAbility.Configurable diff --git a/src-tauri/.sqlx/query-4ea55b4f46a6cc640b716409281a951d680b843a85abe8d9363fad9d3c9e950b.json b/src-tauri/.sqlx/query-5333542a69d9004f67066ddaa449291d867c36314b2d31b073b538b0b12bea8c.json similarity index 66% rename from src-tauri/.sqlx/query-4ea55b4f46a6cc640b716409281a951d680b843a85abe8d9363fad9d3c9e950b.json rename to src-tauri/.sqlx/query-5333542a69d9004f67066ddaa449291d867c36314b2d31b073b538b0b12bea8c.json index f6d818bd..e7c65a02 100644 --- a/src-tauri/.sqlx/query-4ea55b4f46a6cc640b716409281a951d680b843a85abe8d9363fad9d3c9e950b.json +++ b/src-tauri/.sqlx/query-5333542a69d9004f67066ddaa449291d867c36314b2d31b073b538b0b12bea8c.json @@ -1,6 +1,6 @@ { "db_name": "SQLite", - "query": "INSERT INTO instance (name, uuid, url, proxy_url, username, token, client_traffic_policy , enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) RETURNING id;", + "query": "INSERT INTO instance (name, uuid, url, proxy_url, username, token, client_traffic_policy , enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods, mfa_capabilities) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) RETURNING id;", "describe": { "columns": [ { @@ -16,11 +16,11 @@ } ], "parameters": { - "Right": 11 + "Right": 12 }, "nullable": [ false ] }, - "hash": "4ea55b4f46a6cc640b716409281a951d680b843a85abe8d9363fad9d3c9e950b" + "hash": "5333542a69d9004f67066ddaa449291d867c36314b2d31b073b538b0b12bea8c" } diff --git a/src-tauri/.sqlx/query-4bad70df5cb3d65e0f10c5125c8650f9f86ce7a8140d667433e402f0a62d096f.json b/src-tauri/.sqlx/query-603ec52d065a9601fbd370e37941b2aa7c56b5dcff39ac3d7c4b6624fbb7ceec.json similarity index 62% rename from src-tauri/.sqlx/query-4bad70df5cb3d65e0f10c5125c8650f9f86ce7a8140d667433e402f0a62d096f.json rename to src-tauri/.sqlx/query-603ec52d065a9601fbd370e37941b2aa7c56b5dcff39ac3d7c4b6624fbb7ceec.json index f4da2e5f..f17100bb 100644 --- a/src-tauri/.sqlx/query-4bad70df5cb3d65e0f10c5125c8650f9f86ce7a8140d667433e402f0a62d096f.json +++ b/src-tauri/.sqlx/query-603ec52d065a9601fbd370e37941b2aa7c56b5dcff39ac3d7c4b6624fbb7ceec.json @@ -1,12 +1,12 @@ { "db_name": "SQLite", - "query": "UPDATE instance SET name = $1, uuid = $2, url = $3, proxy_url = $4, username = $5, client_traffic_policy = $6, enterprise_enabled = $7, disable_tunnels = $8, token = $9, openid_display_name = $10, mfa_configured_methods = $11 WHERE id = $12;", + "query": "UPDATE instance SET name = $1, uuid = $2, url = $3, proxy_url = $4, username = $5, client_traffic_policy = $6, enterprise_enabled = $7, disable_tunnels = $8, token = $9, openid_display_name = $10, mfa_configured_methods = $11, mfa_capabilities = $12 WHERE id = $13;", "describe": { "columns": [], "parameters": { - "Right": 12 + "Right": 13 }, "nullable": [] }, - "hash": "4bad70df5cb3d65e0f10c5125c8650f9f86ce7a8140d667433e402f0a62d096f" + "hash": "603ec52d065a9601fbd370e37941b2aa7c56b5dcff39ac3d7c4b6624fbb7ceec" } diff --git a/src-tauri/.sqlx/query-881b9e283868c50ba854bc884283bbfa5d0ec541b947f44a1c6bf5a49e50bdb3.json b/src-tauri/.sqlx/query-77fcdba9db256f8a424c1d3193dc4658f6a14ea548db376552d84b229a22849d.json similarity index 87% rename from src-tauri/.sqlx/query-881b9e283868c50ba854bc884283bbfa5d0ec541b947f44a1c6bf5a49e50bdb3.json rename to src-tauri/.sqlx/query-77fcdba9db256f8a424c1d3193dc4658f6a14ea548db376552d84b229a22849d.json index 952a82bc..39a4eaee 100644 --- a/src-tauri/.sqlx/query-881b9e283868c50ba854bc884283bbfa5d0ec541b947f44a1c6bf5a49e50bdb3.json +++ b/src-tauri/.sqlx/query-77fcdba9db256f8a424c1d3193dc4658f6a14ea548db376552d84b229a22849d.json @@ -1,6 +1,6 @@ { "db_name": "SQLite", - "query": "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token, client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods \"mfa_configured_methods: _\" FROM instance WHERE token IS NOT NULL ORDER BY name ASC;", + "query": "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token, client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods \"mfa_configured_methods: _\", mfa_capabilities \"mfa_capabilities: _\" FROM instance WHERE token IS NOT NULL ORDER BY name ASC;", "describe": { "columns": [ { @@ -134,6 +134,17 @@ "name": "mfa_configured_methods" } } + }, + { + "name": "mfa_capabilities: _", + "ordinal": 12, + "type_info": "Text", + "origin": { + "Table": { + "table": "instance", + "name": "mfa_capabilities" + } + } } ], "parameters": { @@ -151,8 +162,9 @@ false, false, true, + true, true ] }, - "hash": "881b9e283868c50ba854bc884283bbfa5d0ec541b947f44a1c6bf5a49e50bdb3" + "hash": "77fcdba9db256f8a424c1d3193dc4658f6a14ea548db376552d84b229a22849d" } diff --git a/src-tauri/.sqlx/query-7387f9763fa13b500aed26f00f18bedcb246c3741ff77d33e7fb04452e15e633.json b/src-tauri/.sqlx/query-7ab70302d037b1322ad5c6df8070637c0772ca20b69c9515397c2c09b447cf41.json similarity index 88% rename from src-tauri/.sqlx/query-7387f9763fa13b500aed26f00f18bedcb246c3741ff77d33e7fb04452e15e633.json rename to src-tauri/.sqlx/query-7ab70302d037b1322ad5c6df8070637c0772ca20b69c9515397c2c09b447cf41.json index 405e7ac4..3f9f8ba2 100644 --- a/src-tauri/.sqlx/query-7387f9763fa13b500aed26f00f18bedcb246c3741ff77d33e7fb04452e15e633.json +++ b/src-tauri/.sqlx/query-7ab70302d037b1322ad5c6df8070637c0772ca20b69c9515397c2c09b447cf41.json @@ -1,6 +1,6 @@ { "db_name": "SQLite", - "query": "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token \"token?\", client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods \"mfa_configured_methods: _\" FROM instance WHERE id = $1;", + "query": "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token \"token?\", client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods \"mfa_configured_methods: _\", mfa_capabilities \"mfa_capabilities: _\" FROM instance WHERE name = $1;", "describe": { "columns": [ { @@ -134,6 +134,17 @@ "name": "mfa_configured_methods" } } + }, + { + "name": "mfa_capabilities: _", + "ordinal": 12, + "type_info": "Text", + "origin": { + "Table": { + "table": "instance", + "name": "mfa_capabilities" + } + } } ], "parameters": { @@ -151,8 +162,9 @@ false, false, true, + true, true ] }, - "hash": "7387f9763fa13b500aed26f00f18bedcb246c3741ff77d33e7fb04452e15e633" + "hash": "7ab70302d037b1322ad5c6df8070637c0772ca20b69c9515397c2c09b447cf41" } diff --git a/src-tauri/.sqlx/query-43bfb3fcdf996a7009ee23bd0a0e7f1f5ac113740d3e2b420eeaad3473f8c678.json b/src-tauri/.sqlx/query-92b79687976be57157424962deaee17b6c51789d2987bb8649298f2daf74a744.json similarity index 88% rename from src-tauri/.sqlx/query-43bfb3fcdf996a7009ee23bd0a0e7f1f5ac113740d3e2b420eeaad3473f8c678.json rename to src-tauri/.sqlx/query-92b79687976be57157424962deaee17b6c51789d2987bb8649298f2daf74a744.json index 4c7843c6..ddf01bc9 100644 --- a/src-tauri/.sqlx/query-43bfb3fcdf996a7009ee23bd0a0e7f1f5ac113740d3e2b420eeaad3473f8c678.json +++ b/src-tauri/.sqlx/query-92b79687976be57157424962deaee17b6c51789d2987bb8649298f2daf74a744.json @@ -1,6 +1,6 @@ { "db_name": "SQLite", - "query": "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token \"token?\", client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods \"mfa_configured_methods: _\" FROM instance WHERE name = $1;", + "query": "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token \"token?\", client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods \"mfa_configured_methods: _\", mfa_capabilities \"mfa_capabilities: _\" FROM instance WHERE id = $1;", "describe": { "columns": [ { @@ -134,6 +134,17 @@ "name": "mfa_configured_methods" } } + }, + { + "name": "mfa_capabilities: _", + "ordinal": 12, + "type_info": "Text", + "origin": { + "Table": { + "table": "instance", + "name": "mfa_capabilities" + } + } } ], "parameters": { @@ -151,8 +162,9 @@ false, false, true, + true, true ] }, - "hash": "43bfb3fcdf996a7009ee23bd0a0e7f1f5ac113740d3e2b420eeaad3473f8c678" + "hash": "92b79687976be57157424962deaee17b6c51789d2987bb8649298f2daf74a744" } diff --git a/src-tauri/.sqlx/query-714e5a3371be2563aa5c77071c222b8378f7e414be257ec0a2614808b4daaec9.json b/src-tauri/.sqlx/query-b45d49dbb89d03082def98a25d6d412b2732e3c0c4662de610279af25fe75095.json similarity index 88% rename from src-tauri/.sqlx/query-714e5a3371be2563aa5c77071c222b8378f7e414be257ec0a2614808b4daaec9.json rename to src-tauri/.sqlx/query-b45d49dbb89d03082def98a25d6d412b2732e3c0c4662de610279af25fe75095.json index ca9cde7b..8bf69395 100644 --- a/src-tauri/.sqlx/query-714e5a3371be2563aa5c77071c222b8378f7e414be257ec0a2614808b4daaec9.json +++ b/src-tauri/.sqlx/query-b45d49dbb89d03082def98a25d6d412b2732e3c0c4662de610279af25fe75095.json @@ -1,6 +1,6 @@ { "db_name": "SQLite", - "query": "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token \"token?\", client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods \"mfa_configured_methods: _\" FROM instance ORDER BY name ASC;", + "query": "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token \"token?\", client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, mfa_configured_methods \"mfa_configured_methods: _\", mfa_capabilities \"mfa_capabilities: _\" FROM instance ORDER BY name ASC;", "describe": { "columns": [ { @@ -134,6 +134,17 @@ "name": "mfa_configured_methods" } } + }, + { + "name": "mfa_capabilities: _", + "ordinal": 12, + "type_info": "Text", + "origin": { + "Table": { + "table": "instance", + "name": "mfa_capabilities" + } + } } ], "parameters": { @@ -151,8 +162,9 @@ false, false, true, + true, true ] }, - "hash": "714e5a3371be2563aa5c77071c222b8378f7e414be257ec0a2614808b4daaec9" + "hash": "b45d49dbb89d03082def98a25d6d412b2732e3c0c4662de610279af25fe75095" } diff --git a/src-tauri/client-cli/src/commands/instance.rs b/src-tauri/client-cli/src/commands/instance.rs index 7ad65434..dd86f8f0 100644 --- a/src-tauri/client-cli/src/commands/instance.rs +++ b/src-tauri/client-cli/src/commands/instance.rs @@ -143,6 +143,7 @@ mod tests { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } } diff --git a/src-tauri/client-cli/src/commands/list.rs b/src-tauri/client-cli/src/commands/list.rs index a509f4f6..bcfc847c 100644 --- a/src-tauri/client-cli/src/commands/list.rs +++ b/src-tauri/client-cli/src/commands/list.rs @@ -220,6 +220,7 @@ mod tests { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } } diff --git a/src-tauri/client-cli/src/resolve.rs b/src-tauri/client-cli/src/resolve.rs index 2b70361f..e791c625 100644 --- a/src-tauri/client-cli/src/resolve.rs +++ b/src-tauri/client-cli/src/resolve.rs @@ -188,6 +188,7 @@ mod tests { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } } diff --git a/src-tauri/client-cli/src/tests_daemon.rs b/src-tauri/client-cli/src/tests_daemon.rs index 4a986c3f..bd5e2ebe 100644 --- a/src-tauri/client-cli/src/tests_daemon.rs +++ b/src-tauri/client-cli/src/tests_daemon.rs @@ -173,6 +173,7 @@ async fn test_active_state_lists_interfaces(pool: DbPool) { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } .save(&pool) .await diff --git a/src-tauri/client-proto/build.rs b/src-tauri/client-proto/build.rs index 58a7bb1a..7ae82a41 100644 --- a/src-tauri/client-proto/build.rs +++ b/src-tauri/client-proto/build.rs @@ -80,6 +80,10 @@ fn main() -> Result<(), Box> { "#[serde(default)]", ) .type_attribute(".defguard.client_types.InstanceInfo", "#[serde(default)]") + .type_attribute( + ".defguard.client_types.MfaCapabilities", + "#[serde(default)]", + ) .type_attribute( ".defguard.client_types.EnrollmentStartResponse", "#[serde(default)]", diff --git a/src-tauri/core/src/database/models/connection.rs b/src-tauri/core/src/database/models/connection.rs index 9486c0ff..105a032c 100644 --- a/src-tauri/core/src/database/models/connection.rs +++ b/src-tauri/core/src/database/models/connection.rs @@ -185,6 +185,7 @@ mod tests { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } .save(pool) .await diff --git a/src-tauri/core/src/database/models/instance.rs b/src-tauri/core/src/database/models/instance.rs index 0440385e..0fbee7b5 100644 --- a/src-tauri/core/src/database/models/instance.rs +++ b/src-tauri/core/src/database/models/instance.rs @@ -19,8 +19,38 @@ pub struct Instance { pub enterprise_enabled: bool, pub disable_tunnels: bool, pub openid_display_name: Option, - /// `None` when the proxy never sent `MfaUserState`, so factors cannot be configured there. + /// None when the proxy never sent MfaUserState. pub mfa_configured_methods: Option>>, + /// None when this Core cannot configure MFA from the client. + pub mfa_capabilities: Option>, +} + +/// Factors a Core accepts in an MFA configuration session, static per Core version. Only +/// factors this client also drives may be offered, so callers intersect with their own lists. +#[derive(Clone, Debug, Default, Serialize, Deserialize, PartialEq, Eq)] +pub struct MfaCapabilities { + pub setup_methods: Vec, + pub authorize_methods: Vec, +} + +impl MfaCapabilities { + #[must_use] + pub fn can_set_up(&self, method: LocationMfaMethod) -> bool { + self.setup_methods.contains(&method) + } + + #[must_use] + pub fn can_authorize(&self, method: LocationMfaMethod) -> bool { + self.authorize_methods.contains(&method) + } +} + +fn sorted_methods( + methods: impl Iterator, +) -> Vec { + let mut methods: Vec<_> = methods.map(LocationMfaMethod::from).collect(); + methods.sort_unstable(); + methods } /// Keeps "not reported" distinct from "reported as none", and sorts so a reordered report is @@ -29,14 +59,38 @@ pub struct Instance { pub fn mfa_configured_methods( instance_info: &proto::client_types::InstanceInfo, ) -> Option> { - instance_info.mfa_user_state.as_ref().map(|state| { - let mut methods: Vec<_> = state - .configured_methods() - .map(LocationMfaMethod::from) - .collect(); - methods.sort_unstable(); - methods - }) + instance_info + .mfa_user_state + .as_ref() + .map(|state| sorted_methods(state.configured_methods())) +} + +/// Same contract as [mfa_configured_methods]. Methods this client does not know are dropped. +#[must_use] +pub fn mfa_capabilities( + instance_info: &proto::client_types::InstanceInfo, +) -> Option { + instance_info + .mfa_capabilities + .as_ref() + .map(|capabilities| MfaCapabilities { + setup_methods: sorted_methods(capabilities.setup_methods()), + authorize_methods: sorted_methods(capabilities.authorize_methods()), + }) +} + +impl Instance { + /// Returns whether either MFA field changed. + pub fn sync_mfa_state(&mut self, instance_info: &proto::client_types::InstanceInfo) -> bool { + let configured_methods = mfa_configured_methods(instance_info); + let capabilities = mfa_capabilities(instance_info); + let changed = self.mfa_configured_methods.as_ref().map(|json| &json.0) + != configured_methods.as_ref() + || self.mfa_capabilities.as_ref().map(|json| &json.0) != capabilities.as_ref(); + self.mfa_configured_methods = configured_methods.map(Json); + self.mfa_capabilities = capabilities.map(Json); + changed + } } impl fmt::Display for Instance { @@ -49,6 +103,7 @@ impl From for Instance { fn from(instance_info: proto::client_types::InstanceInfo) -> Self { let client_traffic_policy = ClientTrafficPolicy::from(&instance_info); let mfa_configured_methods = mfa_configured_methods(&instance_info).map(Json); + let mfa_capabilities = mfa_capabilities(&instance_info).map(Json); Self { id: NoId, name: instance_info.name, @@ -62,6 +117,7 @@ impl From for Instance { disable_tunnels: instance_info.disable_tunnels.unwrap_or(false), openid_display_name: instance_info.openid_display_name, mfa_configured_methods, + mfa_capabilities, } } } @@ -74,8 +130,8 @@ impl Instance { query!( "UPDATE instance SET name = $1, uuid = $2, url = $3, proxy_url = $4, username = $5, \ client_traffic_policy = $6, enterprise_enabled = $7, disable_tunnels = $8, token = $9, \ - openid_display_name = $10, mfa_configured_methods = $11 \ - WHERE id = $12;", + openid_display_name = $10, mfa_configured_methods = $11, mfa_capabilities = $12 \ + WHERE id = $13;", self.name, self.uuid, self.url, @@ -87,6 +143,7 @@ impl Instance { self.token, self.openid_display_name, self.mfa_configured_methods, + self.mfa_capabilities, self.id ) .execute(executor) @@ -102,7 +159,8 @@ impl Instance { Self, "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token \"token?\", \ client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, \ - mfa_configured_methods \"mfa_configured_methods: _\" \ + mfa_configured_methods \"mfa_configured_methods: _\", \ + mfa_capabilities \"mfa_capabilities: _\" \ FROM instance ORDER BY name ASC;" ) .fetch_all(executor) @@ -118,7 +176,8 @@ impl Instance { Self, "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token \"token?\", \ client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, \ - mfa_configured_methods \"mfa_configured_methods: _\" \ + mfa_configured_methods \"mfa_configured_methods: _\", \ + mfa_capabilities \"mfa_capabilities: _\" \ FROM instance WHERE id = $1;", id ) @@ -135,7 +194,8 @@ impl Instance { Self, "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token \"token?\", \ client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, \ - mfa_configured_methods \"mfa_configured_methods: _\" \ + mfa_configured_methods \"mfa_configured_methods: _\", \ + mfa_capabilities \"mfa_capabilities: _\" \ FROM instance WHERE name = $1;", name ) @@ -171,7 +231,8 @@ impl Instance { Self, "SELECT id \"id: _\", name, uuid, url, proxy_url, username, token, \ client_traffic_policy, enterprise_enabled, disable_tunnels, openid_display_name, \ - mfa_configured_methods \"mfa_configured_methods: _\" \ + mfa_configured_methods \"mfa_configured_methods: _\", \ + mfa_capabilities \"mfa_capabilities: _\" \ FROM instance \ WHERE token IS NOT NULL ORDER BY name ASC;" ) @@ -220,6 +281,8 @@ impl PartialEq for Instance { && self.openid_display_name == other.openid_display_name && self.mfa_configured_methods.as_ref().map(|json| &json.0) == mfa_configured_methods(other).as_ref() + && self.mfa_capabilities.as_ref().map(|json| &json.0) + == mfa_capabilities(other).as_ref() } } @@ -233,8 +296,8 @@ impl Instance { let result = query!( "INSERT INTO instance (name, uuid, url, proxy_url, username, token, \ client_traffic_policy , enterprise_enabled, disable_tunnels, openid_display_name, \ - mfa_configured_methods) \ - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) RETURNING id;", + mfa_configured_methods, mfa_capabilities) \ + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) RETURNING id;", self.name, self.uuid, url, @@ -245,7 +308,8 @@ impl Instance { self.enterprise_enabled, self.disable_tunnels, self.openid_display_name, - self.mfa_configured_methods + self.mfa_configured_methods, + self.mfa_capabilities ) .fetch_one(executor) .await?; @@ -262,6 +326,7 @@ impl Instance { disable_tunnels: self.disable_tunnels, openid_display_name: self.openid_display_name, mfa_configured_methods: self.mfa_configured_methods, + mfa_capabilities: self.mfa_capabilities, }) } } @@ -279,9 +344,9 @@ pub struct InstanceInfo { pub enterprise_enabled: bool, pub disable_tunnels: bool, pub openid_display_name: Option, - /// `None` when the instance never reported its MFA state, which the frontend reads as - /// "cannot configure factors here". pub mfa_configured_methods: Option>, + /// None when this Core cannot configure MFA from the client. + pub mfa_capabilities: Option, } impl fmt::Display for InstanceInfo { @@ -363,6 +428,7 @@ mod tests { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } } @@ -590,6 +656,7 @@ mod tests { disable_tunnels: false, openid_display_name: info.openid_display_name.clone(), mfa_configured_methods: None, + mfa_capabilities: None, }; // Model has false, proto has true → not equal. assert_ne!(instance, info); @@ -638,6 +705,7 @@ mod tests { disable_tunnels: false, openid_display_name: info.openid_display_name.clone(), mfa_configured_methods: None, + mfa_capabilities: None, }; // Never reported vs reported as [totp], a change the poller has to persist. assert_ne!(instance, info); @@ -671,8 +739,120 @@ mod tests { LocationMfaMethod::Totp, LocationMfaMethod::Fido2, ])), + mfa_capabilities: None, }; // The same factors in another order are not a change worth a full config update. assert_eq!(instance, info); } + + fn proto_capabilities( + setup_methods: &[i32], + authorize_methods: &[i32], + ) -> proto::client_types::MfaCapabilities { + proto::client_types::MfaCapabilities { + setup_methods: setup_methods.to_vec(), + authorize_methods: authorize_methods.to_vec(), + } + } + + #[test] + fn test_instance_from_proto_mfa_capabilities_absent() { + let instance: Instance = base_info().into(); + assert!(instance.mfa_capabilities.is_none()); + } + + #[test] + fn test_instance_from_proto_mfa_capabilities_sorts_and_drops_unknown_methods() { + let mut info = base_info(); + info.mfa_capabilities = Some(proto_capabilities( + &[ + proto::client_types::MfaMethod::Fido2 as i32, + 99, + proto::client_types::MfaMethod::Totp as i32, + ], + &[proto::client_types::MfaMethod::Oidc as i32, 99], + )); + let instance: Instance = info.into(); + assert_eq!( + instance.mfa_capabilities.map(|json| json.0), + Some(MfaCapabilities { + setup_methods: vec![LocationMfaMethod::Totp, LocationMfaMethod::Fido2], + authorize_methods: vec![LocationMfaMethod::Oidc], + }) + ); + } + + #[test] + fn test_instance_from_proto_mfa_capabilities_empty_is_not_absent() { + let mut info = base_info(); + info.mfa_capabilities = Some(proto_capabilities(&[], &[])); + let instance: Instance = info.into(); + assert_eq!( + instance.mfa_capabilities.map(|json| json.0), + Some(MfaCapabilities::default()) + ); + } + + #[test] + fn test_instance_partial_eq_detects_mfa_capabilities_change() { + let mut info = base_info(); + info.mfa_capabilities = Some(proto_capabilities( + &[ + proto::client_types::MfaMethod::Fido2 as i32, + proto::client_types::MfaMethod::Totp as i32, + ], + &[], + )); + let mut instance = Instance:: { + id: 1, + name: info.name.clone(), + uuid: info.id.clone(), + url: info.url.clone(), + proxy_url: info.proxy_url.clone(), + username: info.username.clone(), + token: Some("tok".into()), + client_traffic_policy: ClientTrafficPolicy::None, + enterprise_enabled: info.enterprise_enabled, + disable_tunnels: false, + openid_display_name: info.openid_display_name.clone(), + mfa_configured_methods: None, + mfa_capabilities: None, + }; + // a Core upgraded to configure MFA from the client + assert_ne!(instance, info); + + instance.mfa_capabilities = Some(Json(MfaCapabilities { + setup_methods: vec![LocationMfaMethod::Totp, LocationMfaMethod::Fido2], + authorize_methods: Vec::new(), + })); + assert_eq!(instance, info); + } + + #[sqlx::test(migrations = "../migrations")] + async fn test_mfa_capabilities_round_trip(pool: SqlitePool) { + let capabilities = MfaCapabilities { + setup_methods: vec![LocationMfaMethod::Totp, LocationMfaMethod::Fido2], + authorize_methods: vec![LocationMfaMethod::Email, LocationMfaMethod::Oidc], + }; + let mut instance = new_instance(); + instance.mfa_capabilities = Some(Json(capabilities.clone())); + let mut saved = instance.save(&pool).await.unwrap(); + + let persisted = Instance::find_by_id(&pool, saved.id) + .await + .unwrap() + .expect("instance should exist"); + assert_eq!( + persisted.mfa_capabilities.map(|json| json.0), + Some(capabilities) + ); + + saved.mfa_capabilities = None; + saved.save(&pool).await.unwrap(); + let persisted = Instance::find_by_id(&pool, saved.id) + .await + .unwrap() + .expect("instance should exist"); + assert!(persisted.mfa_capabilities.is_none()); + } } diff --git a/src-tauri/core/src/database/models/location.rs b/src-tauri/core/src/database/models/location.rs index 8138050f..1bfaddc9 100644 --- a/src-tauri/core/src/database/models/location.rs +++ b/src-tauri/core/src/database/models/location.rs @@ -743,6 +743,7 @@ mod tests { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } } diff --git a/src-tauri/core/src/database/models/location_stats.rs b/src-tauri/core/src/database/models/location_stats.rs index 3bb93964..04c66946 100644 --- a/src-tauri/core/src/database/models/location_stats.rs +++ b/src-tauri/core/src/database/models/location_stats.rs @@ -259,6 +259,7 @@ mod tests { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } .save(pool) .await diff --git a/src-tauri/core/src/mfa_config.rs b/src-tauri/core/src/mfa_config.rs index 4a473a67..8b06b296 100644 --- a/src-tauri/core/src/mfa_config.rs +++ b/src-tauri/core/src/mfa_config.rs @@ -20,7 +20,7 @@ use tokio::{ use tokio_util::sync::CancellationToken; use crate::{ - database::models::Id, + database::models::{instance::MfaCapabilities, Id}, mfa::{OIDC_POLL_INTERVAL, OIDC_POLL_TIMEOUT}, proxy::{post_with_headers, read_error_message}, }; @@ -34,7 +34,7 @@ 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 +// what this client can authorize with, narrowed per instance by MfaCapabilities pub const AUTHORIZING_METHODS: &[MfaMethod] = &[ MfaMethod::Totp, MfaMethod::Email, @@ -52,7 +52,7 @@ const ALREADY_AUTHORIZED_MESSAGE: &str = "session already authorized"; // a prefix, Core's login flow words it "OIDC authentication not completed yet" const OIDC_PENDING_MESSAGE: &str = "OIDC authentication not completed"; -// Mirrors the methods Core accepts in `mfa_setup_start` / `mfa_setup_finish`, keep in step. +// what this client can set up, narrowed per instance by MfaCapabilities pub const CONFIGURABLE_METHODS: &[MfaMethod] = &[MfaMethod::Totp, MfaMethod::Email, MfaMethod::Fido2]; @@ -94,6 +94,7 @@ pub struct MfaConfigSession { pub proxy_url: Url, pub session_token: String, pub deadline_timestamp: i64, + pub capabilities: MfaCapabilities, } impl MfaConfigSession { @@ -111,6 +112,7 @@ impl fmt::Debug for MfaConfigSession { .field("proxy_url", &self.proxy_url) .field("session_token", &"") .field("deadline_timestamp", &self.deadline_timestamp) + .field("capabilities", &self.capabilities) .finish() } } @@ -185,8 +187,11 @@ fn method_name(method: MfaMethod) -> &'static str { } } -fn ensure_can_authorize(proof: &AuthorizeProof) -> Result<(), MfaConfigError> { - let method = proof.method(); +/// Lets a caller refuse before a key prompt or browser login the request would waste. +pub fn ensure_can_authorize_method( + method: MfaMethod, + capabilities: &MfaCapabilities, +) -> Result<(), MfaConfigError> { if !AUTHORIZING_METHODS.contains(&method) { return Err(MfaConfigError::UnsupportedMethod { message: format!( @@ -195,6 +200,23 @@ fn ensure_can_authorize(proof: &AuthorizeProof) -> Result<(), MfaConfigError> { ), }); } + if !capabilities.can_authorize(method.into()) { + return Err(MfaConfigError::UnsupportedMethod { + message: format!( + "This Defguard instance does not accept a {} to authorize MFA configuration.", + method_name(method) + ), + }); + } + Ok(()) +} + +fn ensure_can_authorize( + proof: &AuthorizeProof, + capabilities: &MfaCapabilities, +) -> Result<(), MfaConfigError> { + let method = proof.method(); + ensure_can_authorize_method(method, capabilities)?; if matches!(proof, AuthorizeProof::Code { .. }) && !CODE_METHODS.contains(&method) { return Err(MfaConfigError::UnsupportedMethod { message: format!( @@ -206,22 +228,34 @@ fn ensure_can_authorize(proof: &AuthorizeProof) -> Result<(), MfaConfigError> { Ok(()) } -fn ensure_can_configure(method: MfaMethod) -> Result<(), MfaConfigError> { - if CONFIGURABLE_METHODS.contains(&method) { - Ok(()) - } else { - Err(MfaConfigError::UnsupportedMethod { +fn ensure_can_configure( + method: MfaMethod, + capabilities: &MfaCapabilities, +) -> Result<(), MfaConfigError> { + if !CONFIGURABLE_METHODS.contains(&method) { + return Err(MfaConfigError::UnsupportedMethod { message: format!( "Configuring a {} from the desktop client is not supported.", method_name(method) ), - }) + }); } + if !capabilities.can_set_up(method.into()) { + return Err(MfaConfigError::UnsupportedMethod { + message: format!( + "This Defguard instance does not support configuring a {} from the desktop client.", + method_name(method) + ), + }); + } + Ok(()) } -/// Unknown method numbers are dropped so a newer Core cannot break an older client. +/// Every factor Core reported for the account, including ones this instance cannot authorize +/// with, so the wizard does not offer them for setup again. Unknown method numbers are dropped so +/// a newer Core cannot break an older client. #[must_use] -pub fn authorizing_methods(response: &MfaConfigStartResponse) -> Vec { +pub fn session_methods(response: &MfaConfigStartResponse) -> Vec { response .available_methods .iter() @@ -230,6 +264,17 @@ pub fn authorizing_methods(response: &MfaConfigStartResponse) -> Vec .collect() } +#[must_use] +pub fn authorizing_methods( + response: &MfaConfigStartResponse, + capabilities: &MfaCapabilities, +) -> Vec { + session_methods(response) + .into_iter() + .filter(|method| capabilities.can_authorize((*method).into())) + .collect() +} + fn build_url(proxy_url: &Url, endpoint: &str) -> Result { proxy_url .join(endpoint) @@ -332,8 +377,9 @@ pub async fn mfa_config_authorize( proxy_url: Url, session_token: String, proof: AuthorizeProof, + capabilities: &MfaCapabilities, ) -> Result { - ensure_can_authorize(&proof)?; + ensure_can_authorize(&proof, capabilities)?; debug!("Authorizing MFA configuration session"); let method = proof.method() as i32; let (code, signature, auth_data, credential_id) = match proof { @@ -367,6 +413,7 @@ pub async fn mfa_config_poll_oidc( proxy_url: Url, session_token: String, deadline_timestamp: i64, + capabilities: &MfaCapabilities, cancel: CancellationToken, ) -> Result { let session_left = u64::try_from(deadline_timestamp - Utc::now().timestamp()).unwrap_or(0); @@ -393,6 +440,7 @@ pub async fn mfa_config_poll_oidc( proxy_url.clone(), session_token.clone(), AuthorizeProof::Oidc, + capabilities, ) .await { @@ -406,8 +454,9 @@ pub async fn mfa_config_setup_start( proxy_url: Url, session_token: String, method: MfaMethod, + capabilities: &MfaCapabilities, ) -> Result { - ensure_can_configure(method)?; + ensure_can_configure(method, capabilities)?; debug!("Starting MFA factor setup"); let request = CodeMfaSetupStartRequest { method: method as i32, @@ -422,8 +471,9 @@ pub async fn mfa_config_setup_finish( session_token: String, method: MfaMethod, proof: SetupProof, + capabilities: &MfaCapabilities, ) -> Result { - ensure_can_configure(method)?; + ensure_can_configure(method, capabilities)?; debug!("Finishing MFA factor setup"); // The proto spells the unused half as empty rather than absent. let (code, name, fido2_attestation) = match proof { diff --git a/src-tauri/core/src/mfa_config/tests.rs b/src-tauri/core/src/mfa_config/tests.rs index 620f4060..c1052072 100644 --- a/src-tauri/core/src/mfa_config/tests.rs +++ b/src-tauri/core/src/mfa_config/tests.rs @@ -1,3 +1,5 @@ +use std::sync::LazyLock; + use reqwest::Url; use serde_json::json; use tokio_util::sync::CancellationToken; @@ -7,6 +9,7 @@ use wiremock::{ }; use super::*; +use crate::database::models::location::LocationMfaMethod; const SESSION_TOKEN: &str = "mfa-config-session"; /// Stand-ins for the WebAuthn JSON the client and Core exchange verbatim. @@ -16,6 +19,17 @@ const ATTESTATION: &str = r#"{"id":"cred"}"#; /// far enough out that only OIDC_POLL_TIMEOUT bounds a poll const LIVE_DEADLINE: i64 = 4_000_000_000; +static ALL_CAPABILITIES: LazyLock = LazyLock::new(|| MfaCapabilities { + setup_methods: CONFIGURABLE_METHODS + .iter() + .map(|&method| method.into()) + .collect(), + authorize_methods: AUTHORIZING_METHODS + .iter() + .map(|&method| method.into()) + .collect(), +}); + fn mock_url(server: &MockServer) -> Url { Url::parse(&server.uri()).expect("MockServer URI should be valid") } @@ -132,17 +146,24 @@ async fn test_method_is_sent_as_a_number() { url.clone(), SESSION_TOKEN.into(), code(MfaMethod::Email, "123456"), + &ALL_CAPABILITIES, + ) + .await + .unwrap(); + mfa_config_setup_start( + url.clone(), + SESSION_TOKEN.into(), + MfaMethod::Totp, + &ALL_CAPABILITIES, ) .await .unwrap(); - mfa_config_setup_start(url.clone(), SESSION_TOKEN.into(), MfaMethod::Totp) - .await - .unwrap(); mfa_config_setup_finish( url, SESSION_TOKEN.into(), MfaMethod::Totp, SetupProof::Code("654321".into()), + &ALL_CAPABILITIES, ) .await .unwrap(); @@ -184,9 +205,14 @@ async fn test_setup_start_returns_totp_secret() { ) .await; - let response = mfa_config_setup_start(mock_url(&server), SESSION_TOKEN.into(), MfaMethod::Totp) - .await - .unwrap(); + let response = mfa_config_setup_start( + mock_url(&server), + SESSION_TOKEN.into(), + MfaMethod::Totp, + &ALL_CAPABILITIES, + ) + .await + .unwrap(); assert_eq!(response.totp_secret.as_deref(), Some("JBSWY3DPEHPK3PXP")); } @@ -201,10 +227,14 @@ async fn test_setup_start_secret_is_absent_for_email() { ) .await; - let response = - mfa_config_setup_start(mock_url(&server), SESSION_TOKEN.into(), MfaMethod::Email) - .await - .unwrap(); + let response = mfa_config_setup_start( + mock_url(&server), + SESSION_TOKEN.into(), + MfaMethod::Email, + &ALL_CAPABILITIES, + ) + .await + .unwrap(); assert!(response.totp_secret.is_none()); } @@ -225,6 +255,7 @@ async fn test_setup_finish_returns_recovery_codes() { SESSION_TOKEN.into(), MfaMethod::Totp, SetupProof::Code("654321".into()), + &ALL_CAPABILITIES, ) .await .unwrap(); @@ -248,6 +279,7 @@ async fn test_setup_finish_accepts_empty_recovery_codes() { SESSION_TOKEN.into(), MfaMethod::Email, SetupProof::Code("111111".into()), + &ALL_CAPABILITIES, ) .await .unwrap(); @@ -275,7 +307,7 @@ async fn test_one_session_configures_two_factors() { let url = mock_url(&server); for (method, code) in [(MfaMethod::Totp, "654321"), (MfaMethod::Email, "111111")] { - mfa_config_setup_start(url.clone(), SESSION_TOKEN.into(), method) + mfa_config_setup_start(url.clone(), SESSION_TOKEN.into(), method, &ALL_CAPABILITIES) .await .unwrap(); mfa_config_setup_finish( @@ -283,6 +315,7 @@ async fn test_one_session_configures_two_factors() { SESSION_TOKEN.into(), method, SetupProof::Code(code.into()), + &ALL_CAPABILITIES, ) .await .unwrap(); @@ -307,9 +340,14 @@ async fn test_not_found_off_the_start_route_is_a_proxy_error() { let server = MockServer::start().await; mount(&server, SETUP_START, ResponseTemplate::new(404)).await; - let err = mfa_config_setup_start(mock_url(&server), SESSION_TOKEN.into(), MfaMethod::Totp) - .await - .unwrap_err(); + let err = mfa_config_setup_start( + mock_url(&server), + SESSION_TOKEN.into(), + MfaMethod::Totp, + &ALL_CAPABILITIES, + ) + .await + .unwrap_err(); assert!(matches!( err, @@ -331,6 +369,7 @@ async fn test_unauthorized_means_session_expired() { mock_url(&server), "stale".into(), code(MfaMethod::Email, "000000"), + &ALL_CAPABILITIES, ) .await .unwrap_err(); @@ -349,9 +388,14 @@ async fn test_unauthorized_invalid_code_is_an_invalid_code() { ) .await; - let err = mfa_config_authorize(mock_url(&server), SESSION_TOKEN.into(), fido2_proof()) - .await - .unwrap_err(); + let err = mfa_config_authorize( + mock_url(&server), + SESSION_TOKEN.into(), + fido2_proof(), + &ALL_CAPABILITIES, + ) + .await + .unwrap_err(); assert!(matches!(err, MfaConfigError::InvalidCode { .. })); } @@ -383,9 +427,14 @@ async fn test_other_forbidden_carries_the_core_message() { ) .await; - let err = mfa_config_authorize(mock_url(&server), SESSION_TOKEN.into(), fido2_proof()) - .await - .unwrap_err(); + let err = mfa_config_authorize( + mock_url(&server), + SESSION_TOKEN.into(), + fido2_proof(), + &ALL_CAPABILITIES, + ) + .await + .unwrap_err(); match err { MfaConfigError::Forbidden { message } => assert_eq!(message, "user is inactive"), @@ -403,9 +452,14 @@ async fn test_no_fido2_challenge_is_a_failed_precondition() { ) .await; - let err = mfa_config_authorize(mock_url(&server), SESSION_TOKEN.into(), fido2_proof()) - .await - .unwrap_err(); + let err = mfa_config_authorize( + mock_url(&server), + SESSION_TOKEN.into(), + fido2_proof(), + &ALL_CAPABILITIES, + ) + .await + .unwrap_err(); match err { MfaConfigError::FailedPrecondition { message } => { @@ -430,6 +484,7 @@ async fn test_bad_request_carries_the_proxy_message() { SESSION_TOKEN.into(), MfaMethod::Totp, SetupProof::Code("000000".into()), + &ALL_CAPABILITIES, ) .await .unwrap_err(); @@ -482,13 +537,18 @@ async fn test_unsupported_methods_are_rejected_before_the_request() { MfaMethod::MobileApprove, ] { assert!(matches!( - mfa_config_authorize(url.clone(), "t".into(), code(unsupported, "1")) - .await - .unwrap_err(), + mfa_config_authorize( + url.clone(), + "t".into(), + code(unsupported, "1"), + &ALL_CAPABILITIES + ) + .await + .unwrap_err(), MfaConfigError::UnsupportedMethod { .. } )); assert!(matches!( - mfa_config_setup_start(url.clone(), "t".into(), unsupported) + mfa_config_setup_start(url.clone(), "t".into(), unsupported, &ALL_CAPABILITIES) .await .unwrap_err(), MfaConfigError::UnsupportedMethod { .. } @@ -498,7 +558,8 @@ async fn test_unsupported_methods_are_rejected_before_the_request() { url.clone(), "t".into(), unsupported, - SetupProof::Code("1".into()) + SetupProof::Code("1".into()), + &ALL_CAPABILITIES ) .await .unwrap_err(), @@ -518,15 +579,66 @@ async fn test_fido2_cannot_authorize_with_a_code() { .await; assert!(matches!( - mfa_config_authorize(mock_url(&server), "t".into(), code(MfaMethod::Fido2, "1")) - .await - .unwrap_err(), + mfa_config_authorize( + mock_url(&server), + "t".into(), + code(MfaMethod::Fido2, "1"), + &ALL_CAPABILITIES + ) + .await + .unwrap_err(), MfaConfigError::UnsupportedMethod { .. } )); assert!(CONFIGURABLE_METHODS.contains(&MfaMethod::Fido2)); assert!(AUTHORIZING_METHODS.contains(&MfaMethod::Fido2)); } +/// an older Core must not be sent a factor only a newer client knows how to drive +#[tokio::test] +async fn test_methods_the_instance_lacks_are_rejected_before_the_request() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; + let capabilities = MfaCapabilities { + setup_methods: vec![LocationMfaMethod::Totp], + authorize_methods: vec![LocationMfaMethod::Totp], + }; + + let url = mock_url(&server); + assert!(matches!( + mfa_config_authorize( + url.clone(), + "t".into(), + code(MfaMethod::Email, "1"), + &capabilities + ) + .await + .unwrap_err(), + MfaConfigError::UnsupportedMethod { .. } + )); + assert!(matches!( + mfa_config_setup_start(url.clone(), "t".into(), MfaMethod::Fido2, &capabilities) + .await + .unwrap_err(), + MfaConfigError::UnsupportedMethod { .. } + )); + assert!(matches!( + mfa_config_setup_finish( + url, + "t".into(), + MfaMethod::Email, + SetupProof::Code("1".into()), + &capabilities + ) + .await + .unwrap_err(), + MfaConfigError::UnsupportedMethod { .. } + )); +} + fn fido2_proof() -> AuthorizeProof { AuthorizeProof::Fido2 { signature: vec![6, 7], @@ -595,9 +707,14 @@ async fn test_fido2_authorize_sends_the_assertion() { .mount(&server) .await; - let response = mfa_config_authorize(mock_url(&server), SESSION_TOKEN.into(), fido2_proof()) - .await - .unwrap(); + let response = mfa_config_authorize( + mock_url(&server), + SESSION_TOKEN.into(), + fido2_proof(), + &ALL_CAPABILITIES, + ) + .await + .unwrap(); assert_eq!(response.deadline_timestamp, 7); assert!(response.recovery_codes.is_empty()); @@ -626,6 +743,7 @@ async fn test_oidc_pending_matches_both_wordings() { mock_url(&server), SESSION_TOKEN.into(), AuthorizeProof::Oidc, + &ALL_CAPABILITIES, ) .await .unwrap_err(); @@ -662,6 +780,7 @@ async fn test_oidc_poll_keeps_going_until_the_login_completes() { mock_url(&server), SESSION_TOKEN.into(), LIVE_DEADLINE, + &ALL_CAPABILITIES, CancellationToken::new(), ) .await @@ -688,6 +807,7 @@ async fn test_oidc_poll_stops_on_session_already_authorized() { mock_url(&server), SESSION_TOKEN.into(), LIVE_DEADLINE, + &ALL_CAPABILITIES, CancellationToken::new(), ) .await @@ -715,6 +835,7 @@ async fn test_oidc_poll_keeps_an_answer_that_lands_after_cancel() { mock_url(&server), SESSION_TOKEN.into(), LIVE_DEADLINE, + &ALL_CAPABILITIES, cancel.clone(), )); @@ -746,6 +867,7 @@ async fn test_oidc_poll_stops_when_the_session_ends() { mock_url(&server), SESSION_TOKEN.into(), LIVE_DEADLINE, + &ALL_CAPABILITIES, CancellationToken::new(), ) .await @@ -763,6 +885,7 @@ async fn test_oidc_poll_times_out() { mock_url(&server), SESSION_TOKEN.into(), LIVE_DEADLINE, + &ALL_CAPABILITIES, CancellationToken::new(), ) .await @@ -784,6 +907,7 @@ async fn test_oidc_poll_is_bounded_by_the_session_deadline() { mock_url(&server), SESSION_TOKEN.into(), 0, + &ALL_CAPABILITIES, CancellationToken::new(), ) .await @@ -807,6 +931,7 @@ async fn test_oidc_poll_stops_on_cancel() { mock_url(&server), SESSION_TOKEN.into(), LIVE_DEADLINE, + &ALL_CAPABILITIES, cancel, ) .await @@ -842,6 +967,7 @@ async fn test_setup_finish_sends_the_fido2_attestation() { name: "Yubikey".into(), attestation: ATTESTATION.into(), }, + &ALL_CAPABILITIES, ) .await .unwrap(); @@ -859,10 +985,14 @@ async fn test_setup_start_returns_the_fido2_creation_challenge() { ) .await; - let response = - mfa_config_setup_start(mock_url(&server), SESSION_TOKEN.into(), MfaMethod::Fido2) - .await - .unwrap(); + let response = mfa_config_setup_start( + mock_url(&server), + SESSION_TOKEN.into(), + MfaMethod::Fido2, + &ALL_CAPABILITIES, + ) + .await + .unwrap(); assert_eq!( response.fido2_creation_challenge.as_deref(), @@ -887,7 +1017,7 @@ fn test_authorizing_methods_drops_unknown_and_non_authorizing_entries() { }; assert_eq!( - authorizing_methods(&response), + authorizing_methods(&response, &ALL_CAPABILITIES), vec![ MfaMethod::Totp, MfaMethod::Fido2, @@ -906,7 +1036,72 @@ fn test_authorizing_methods_is_empty_for_the_email_fallback() { deadline_timestamp: 0, }; - assert!(authorizing_methods(&response).is_empty()); + assert!(authorizing_methods(&response, &ALL_CAPABILITIES).is_empty()); +} + +#[test] +fn test_authorizing_methods_keeps_only_what_the_instance_accepts() { + let response = MfaConfigStartResponse { + session_token: SESSION_TOKEN.into(), + available_methods: vec![MfaMethod::Totp as i32, MfaMethod::Oidc as i32], + email_fallback: false, + deadline_timestamp: 0, + }; + let capabilities = MfaCapabilities { + setup_methods: Vec::new(), + authorize_methods: vec![LocationMfaMethod::Totp], + }; + + assert_eq!( + authorizing_methods(&response, &capabilities), + vec![MfaMethod::Totp] + ); +} + +#[test] +fn test_session_methods_keeps_what_the_instance_cannot_authorize_with() { + let response = MfaConfigStartResponse { + session_token: SESSION_TOKEN.into(), + available_methods: vec![ + MfaMethod::Totp as i32, + MfaMethod::Fido2 as i32, + MfaMethod::Biometric as i32, + 99, + ], + email_fallback: false, + deadline_timestamp: 0, + }; + let capabilities = MfaCapabilities { + setup_methods: Vec::new(), + authorize_methods: vec![LocationMfaMethod::Fido2], + }; + + assert_eq!( + authorizing_methods(&response, &capabilities), + vec![MfaMethod::Fido2] + ); + assert_eq!( + session_methods(&response), + vec![MfaMethod::Totp, MfaMethod::Fido2] + ); +} + +#[test] +fn test_ensure_can_authorize_method_needs_the_client_and_the_instance() { + let capabilities = MfaCapabilities { + setup_methods: Vec::new(), + authorize_methods: vec![LocationMfaMethod::Fido2, LocationMfaMethod::MobileApprove], + }; + + assert!(ensure_can_authorize_method(MfaMethod::Fido2, &capabilities).is_ok()); + assert!(matches!( + ensure_can_authorize_method(MfaMethod::Oidc, &capabilities), + Err(MfaConfigError::UnsupportedMethod { .. }) + )); + assert!(matches!( + ensure_can_authorize_method(MfaMethod::MobileApprove, &capabilities), + Err(MfaConfigError::UnsupportedMethod { .. }) + )); } /// A proxy URL may carry a base path, which a leading slash on the endpoint would discard. diff --git a/src-tauri/enterprise/config-sync/src/commands.rs b/src-tauri/enterprise/config-sync/src/commands.rs index 51c5ea35..969de102 100644 --- a/src-tauri/enterprise/config-sync/src/commands.rs +++ b/src-tauri/enterprise/config-sync/src/commands.rs @@ -7,7 +7,7 @@ use defguard_client_core::{ use defguard_client_core::{ database::{ models::{ - instance::{mfa_configured_methods, ClientTrafficPolicy, Instance}, + instance::{ClientTrafficPolicy, Instance}, location::{infer_mfa_method, Location}, Id, NoId, }, @@ -22,7 +22,7 @@ use defguard_client_proto::defguard::client::v1::{ }; use defguard_client_proto::defguard::client_types::DeviceConfigResponse; use defguard_client_service_locations::to_service_location; -use sqlx::{types::Json, Sqlite, SqliteExecutor, Transaction}; +use sqlx::{Sqlite, SqliteExecutor, Transaction}; pub async fn locations_changed( transaction: &mut Transaction<'_, Sqlite>, @@ -59,9 +59,8 @@ pub async fn do_update_instance( let instance_info = response .instance .expect("Missing instance info in device config response"); - // Read before the struct is picked apart below. Stays `None` when the proxy reports no - // state at all, so "unsupported" is not confused with an account that has no factors. - let configured_methods = mfa_configured_methods(&instance_info).map(Json); + // before the struct is picked apart below + instance.sync_mfa_state(&instance_info); instance.name = instance_info.name; instance.url = instance_info.url; instance.proxy_url = instance_info.proxy_url; @@ -76,7 +75,6 @@ pub async fn do_update_instance( instance.client_traffic_policy = instance_info.client_traffic_policy.into(); instance.openid_display_name = instance_info.openid_display_name; instance.disable_tunnels = instance_info.disable_tunnels.unwrap_or(false); - instance.mfa_configured_methods = configured_methods; instance.uuid = instance_info.id; if response.token.is_some() { instance.token = response.token; diff --git a/src-tauri/enterprise/config-sync/src/lib.rs b/src-tauri/enterprise/config-sync/src/lib.rs index 0830ffc1..b4b6b8e1 100644 --- a/src-tauri/enterprise/config-sync/src/lib.rs +++ b/src-tauri/enterprise/config-sync/src/lib.rs @@ -7,10 +7,7 @@ pub mod commands; use defguard_client_core::{ database::{ - models::{ - instance::{mfa_configured_methods, Instance}, - Id, - }, + models::{instance::Instance, Id}, DbPool, }, error::Error, @@ -22,7 +19,7 @@ use futures_util::future::join_all; use reqwest::{StatusCode, Url}; use semver::Version; use serde::Serialize; -use sqlx::{types::Json, Sqlite, Transaction}; +use sqlx::{Sqlite, Transaction}; use crate::commands::{ disable_enterprise_features, do_update_instance, sync_service_locations_best_effort, @@ -224,16 +221,12 @@ async fn apply_fetched_config( } // Says nothing about the tunnel, and deferring it would keep the instance unable to // configure MFA for as long as the VPN stayed up. - let configured_methods = mfa_configured_methods(info).map(Json); - if instance.mfa_configured_methods.as_ref().map(|json| &json.0) - != configured_methods.as_ref().map(|json| &json.0) - { + if instance.sync_mfa_state(info) { debug!( "MFA state changed for instance {}({}) while a connection is active, \ persisting the snapshot immediately.", instance.name, instance.id ); - instance.mfa_configured_methods = configured_methods; instance_updated = true; } if instance_updated { diff --git a/src-tauri/enterprise/config-sync/src/tests.rs b/src-tauri/enterprise/config-sync/src/tests.rs index 7ad68afa..b90aa403 100644 --- a/src-tauri/enterprise/config-sync/src/tests.rs +++ b/src-tauri/enterprise/config-sync/src/tests.rs @@ -7,12 +7,13 @@ use std::{ }; use defguard_client_core::database::models::{ - instance::ClientTrafficPolicy, - location::{Location, LocationMfaMode, ServiceLocationMode}, + instance::{ClientTrafficPolicy, MfaCapabilities}, + location::{Location, LocationMfaMethod, LocationMfaMode, ServiceLocationMode}, NoId, }; use defguard_client_proto::defguard::client_types::{ - DeviceConfig, DeviceConfigResponse, InstanceInfo, MfaUserState, + DeviceConfig, DeviceConfigResponse, InstanceInfo, MfaCapabilities as ProtoMfaCapabilities, + MfaMethod, MfaUserState, }; use sqlx::SqlitePool; @@ -113,6 +114,7 @@ fn instance_with_token(token: Option<&str>) -> Instance { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } } @@ -192,6 +194,7 @@ async fn seed_instance( disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } .save(pool) .await @@ -361,19 +364,23 @@ async fn test_poll_instance_changed_while_active_does_not_update_db(pool: Sqlite assert_eq!(location.endpoint, "1.2.3.4:51820"); } -/// The migration leaves a null snapshot, which the frontend reads as "cannot configure MFA", +/// The migrations leave a null snapshot, which the frontend reads as "cannot configure MFA", /// so deferring it behind an active connection would hide the instance until the VPN dropped. #[sqlx::test(migrations = "../../migrations")] async fn test_poll_instance_persists_mfa_snapshot_while_active(pool: SqlitePool) { let mut instance = seed_instance(&pool, "acme", "https://proxy.example", Some("tok")).await; seed_location(&pool, instance.id, 1, "office", "1.2.3.4:51820").await; assert!(instance.mfa_configured_methods.is_none()); + assert!(instance.mfa_capabilities.is_none()); let mut response = device_config_response(&instance, device_config(1, "office", "5.6.7.8:51820")); - // An account with no factors still reports state, which is what tells the client the - // proxy speaks the API at all. - response.instance.as_mut().unwrap().mfa_user_state = Some(MfaUserState::default()); + let info = response.instance.as_mut().unwrap(); + info.mfa_user_state = Some(MfaUserState::default()); + info.mfa_capabilities = Some(ProtoMfaCapabilities { + setup_methods: vec![MfaMethod::Totp as i32], + authorize_methods: vec![MfaMethod::Email as i32], + }); let server = MockPollServer::new(vec![poll_response(response)]); instance.proxy_url = server.url(); instance.save(&pool).await.unwrap(); @@ -399,6 +406,13 @@ async fn test_poll_instance_persists_mfa_snapshot_while_active(pool: SqlitePool) stored.mfa_configured_methods.map(|json| json.0), Some(Vec::new()) ); + assert_eq!( + stored.mfa_capabilities.map(|json| json.0), + Some(MfaCapabilities { + setup_methods: vec![LocationMfaMethod::Totp], + authorize_methods: vec![LocationMfaMethod::Email], + }) + ); // The rest of the config still waits for the disconnect. let location = Location::find_by_instance_id(&pool, instance.id, true) .await diff --git a/src-tauri/migrations/20261002120000_add_instance_mfa_capabilities.sql b/src-tauri/migrations/20261002120000_add_instance_mfa_capabilities.sql new file mode 100644 index 00000000..ab5b5dd5 --- /dev/null +++ b/src-tauri/migrations/20261002120000_add_instance_mfa_capabilities.sql @@ -0,0 +1,2 @@ +-- NULL marks a core that cannot configure MFA from the client, so no factor is offered there. +ALTER TABLE instance ADD COLUMN mfa_capabilities TEXT; diff --git a/src-tauri/proto b/src-tauri/proto index 1fa1c70b..eb31e528 160000 --- a/src-tauri/proto +++ b/src-tauri/proto @@ -1 +1 @@ -Subproject commit 1fa1c70be598c3987ba62391a9a7b44dd1017c69 +Subproject commit eb31e528ddcc30311029232a262ad533bf71b5a5 diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index e6899648..cb9208d6 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -573,6 +573,7 @@ pub(crate) async fn build_instance_info( disable_tunnels: instance.disable_tunnels, openid_display_name: instance.openid_display_name, mfa_configured_methods: instance.mfa_configured_methods.map(|json| json.0), + mfa_capabilities: instance.mfa_capabilities.map(|json| json.0), }) } @@ -2375,6 +2376,7 @@ pub async fn cancel_mfa(task_id: String, state: State<'_, AppState>) -> Result<( pub struct MfaConfigStartResult { session_id: String, available_methods: Vec, + configured_methods: Vec, email_fallback: bool, deadline_timestamp: i64, } @@ -2422,6 +2424,10 @@ pub async fn mfa_config_start( .clone() .filter(|token| !token.is_empty()) .ok_or_else(|| err_to_json(MfaConfigError::NoToken))?; + let capabilities = instance + .mfa_capabilities + .map(|json| json.0) + .ok_or_else(|| err_to_json(MfaConfigError::Unsupported))?; let proxy_url = Url::parse(&instance.proxy_url) .map_err(|e| mfa_config_other(format!("Invalid proxy URL: {e}")))?; @@ -2429,7 +2435,11 @@ pub async fn mfa_config_start( .await .map_err(err_to_json)?; - let available_methods = mfa_config::authorizing_methods(&response) + let available_methods = mfa_config::authorizing_methods(&response, &capabilities) + .into_iter() + .map(LocationMfaMethod::from) + .collect(); + let configured_methods = mfa_config::session_methods(&response) .into_iter() .map(LocationMfaMethod::from) .collect(); @@ -2438,6 +2448,7 @@ pub async fn mfa_config_start( proxy_url, session_token: response.session_token, deadline_timestamp: response.deadline_timestamp, + capabilities, }; let session_uuid = Uuid::new_v4(); @@ -2455,6 +2466,7 @@ pub async fn mfa_config_start( Ok(MfaConfigStartResult { session_id: session_uuid.to_string(), available_methods, + configured_methods, email_fallback: response.email_fallback, deadline_timestamp: response.deadline_timestamp, }) @@ -2487,6 +2499,7 @@ pub async fn mfa_config_authorize( session.proxy_url, session.session_token, AuthorizeProof::Code { method, code }, + &session.capabilities, ) .await .map_err(err_to_json)?; @@ -2529,6 +2542,8 @@ pub async fn mfa_config_authorize_fido2( let uid = parse_mfa_config_session_id(&session_id)?; let ceremony = CeremonyGuard::register(&state, uid).map_err(err_to_json)?; let session = get_mfa_config_session(&state, &session_id)?; + mfa_config::ensure_can_authorize_method(MfaMethod::Fido2, &session.capabilities) + .map_err(err_to_json)?; let instance = Instance::find_by_id(&*DB_POOL, session.instance_id) .await .map_err(|err| mfa_config_other(err.to_string()))? @@ -2584,6 +2599,7 @@ pub async fn mfa_config_authorize_fido2( auth_data: assertion.authenticator_data, credential_id: assertion.credential_id, }, + &session.capabilities, ) .await .map_err(err_to_json)?; @@ -2601,6 +2617,8 @@ pub async fn mfa_config_oidc_url( state: State<'_, AppState>, ) -> Result { let session = get_mfa_config_session(&state, &session_id)?; + mfa_config::ensure_can_authorize_method(MfaMethod::Oidc, &session.capabilities) + .map_err(err_to_json)?; let mut url = session .proxy_url .join("openid/mfa") @@ -2625,6 +2643,7 @@ pub async fn mfa_config_authorize_oidc( session.proxy_url, session.session_token, session.deadline_timestamp, + &session.capabilities, attempt.token(), ) .await @@ -2644,9 +2663,14 @@ pub async fn mfa_config_setup_start( debug!("Starting MFA factor setup"); let method = parse_mfa_method(&method)?; let session = get_mfa_config_session(&state, &session_id)?; - mfa_config::mfa_config_setup_start(session.proxy_url, session.session_token, method) - .await - .map_err(err_to_json) + mfa_config::mfa_config_setup_start( + session.proxy_url, + session.session_token, + method, + &session.capabilities, + ) + .await + .map_err(err_to_json) } /// Keeps the session alive so the user can configure another factor without authorizing again. @@ -2666,6 +2690,7 @@ pub async fn mfa_config_setup_finish( session.session_token, method, SetupProof::Code(code), + &session.capabilities, ) .await .map_err(err_to_json)?; @@ -2724,6 +2749,7 @@ pub async fn mfa_config_setup_fido2( session.proxy_url.clone(), session.session_token.clone(), MfaMethod::Fido2, + &session.capabilities, ) .await .map_err(err_to_json)?; @@ -2769,6 +2795,7 @@ pub async fn mfa_config_setup_fido2( session.session_token, MfaMethod::Fido2, SetupProof::Fido2 { name, attestation }, + &session.capabilities, ) .await .map_err(err_to_json)?; @@ -2848,7 +2875,10 @@ pub async fn mfa_config_cancel( #[cfg(test)] mod tests { - use defguard_client_core::version::{CORE_VERSION_HEADER, PROXY_VERSION_HEADER}; + use defguard_client_core::{ + database::models::instance::MfaCapabilities, + version::{CORE_VERSION_HEADER, PROXY_VERSION_HEADER}, + }; use defguard_client_proto::defguard::client_types::{ MfaAdvanced, MfaAwaitingExternal, MfaCompleted, }; @@ -3228,6 +3258,7 @@ mod tests { proxy_url: Url::parse("https://proxy.example.com").expect("valid proxy URL"), session_token: "session-token".into(), deadline_timestamp, + capabilities: MfaCapabilities::default(), } } diff --git a/src-tauri/src/session_state.rs b/src-tauri/src/session_state.rs index 6bfd37c0..c9be0bfc 100644 --- a/src-tauri/src/session_state.rs +++ b/src-tauri/src/session_state.rs @@ -259,6 +259,7 @@ mod tests { disable_tunnels: false, openid_display_name: None, mfa_configured_methods: None, + mfa_capabilities: None, } .save(pool) .await