Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down Expand Up @@ -79,32 +99,20 @@ 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 () => {
let endPrevious = () => {};
const ended = new Promise<void>((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 });

Expand All @@ -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([]);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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. */
Expand All @@ -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. */
Expand Down Expand Up @@ -146,10 +155,14 @@ export const useConfigureMfaStore = create<Store>()(
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),
),
Expand Down Expand Up @@ -249,7 +262,7 @@ export const useConfigureMfaStore = create<Store>()(
name: 'configure-mfa-store',
storage: createJSONStorage(() => sessionStorage),
// Bumped on every shape change: a stored session is never resumable across one.
version: 12,
version: 13,
},
),
);
Expand All @@ -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 });
};
Expand Down
14 changes: 8 additions & 6 deletions new-ui/src/pages/full/ConfigureMfaPage/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,20 @@ const WIZARD_FACTORS: Record<ClientConfigurableMethod, Omit<MfaFactor, 'method'>
};

/** 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) =>
Expand All @@ -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);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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<MfaMethodValue[]>(() =>
CLIENT_CONFIGURABLE_METHODS.filter((method) => initialSelection.includes(method)),
setupMethods.filter((method) => initialSelection.includes(method)),
);
const [error, setError] = useState<string | null>(null);

Expand Down Expand Up @@ -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));
}
Expand All @@ -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);

Expand All @@ -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.`;
}
Expand All @@ -121,26 +125,29 @@ export const ConfigureSelectMethodsStep = ({ onCancel }: Props) => {
hint,
};
},
[accountConfigured, configuredMethods, selected],
[accountConfigured, configuredMethods, setupMethods, selected],
);

const { mutate: cancel, isPending: isCancelling } = useMutation({
mutationFn: discardMfaConfiguration,
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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down
2 changes: 1 addition & 1 deletion new-ui/src/shared/components/MfaSettingsSection/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
22 changes: 20 additions & 2 deletions new-ui/src/shared/components/MfaSettingsSection/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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);
Expand Down
Loading
Loading