Add a null check to getMfaChallengeResponse

[Joerger]

Changelog: Fixed a bug in the WebUI that could cause an access denied error when accessing application.

Fix a bug caused by #49679 which was meant to check for null/undefined.

Closes #50556

[ravicious]

Why the app access bug is present on branch/v17 but not on master

As discussed on Slack, on branch/v17, the faulty getMfaChallengeResponse is called every time an app session is created through the app launcher:

Details

teleport/web/packages/teleport/src/AppLauncher/AppLauncher.tsx

Line 104 in f4891b5

const session = await service.createAppSession(params);

teleport/web/packages/teleport/src/services/apps/apps.ts

Lines 51 to 60 in f4891b5

	// Prompt for MFA if per-session MFA is required for this app.
	const challenge = await auth.getMfaChallenge({
	scope: MfaChallengeScope.USER_SESSION,
	allowReuse: false,
	isMfaRequiredRequest: {
	app: resolveApp,
	},
	});
	const resp = await auth.getMfaChallengeResponse(challenge);

On master, that function is not called at all and the code for handling MFA returns early if challenge is falsy. That was implemented in #49794 which added support for SSO MFA.

Details

teleport/web/packages/teleport/src/AppLauncher/AppLauncher.tsx

Lines 121 to 123 in f9b61a0

	const createAppSessionParams = params as CreateAppSessionParams;
	createAppSessionParams.mfaResponse = await mfa.getChallengeResponse();
	const session = await service.createAppSession(createAppSessionParams);

teleport/web/packages/teleport/src/lib/useMfa.ts

Lines 86 to 90 in f9b61a0

	challenge = challenge ? challenge : await auth.getMfaChallenge(req);
	if (!challenge) {
	setMfaRequired(false);
	return;
	}

Broken password change flow with second_factor: "optional"

I wanted to find a way to reproduce the problem with that function on master, as it's still used in a couple of places. However, most of those places are triggered only when MFA is actually available. One of the places it's used is useReAuthenticate which is used in the password change flow.

I launched Teleport with TELEPORT_ALLOW_NO_SECOND_FACTOR=yes and second_factor: "optional", I created a new user with an MFA device, then I removed the device in hopes that I'd be able to trigger the bug during the password change. However, before I was able to trigger a call to getMfaChallengeResponse, I ran into another problem:

teleport/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx

Lines 164 to 165 in f9b61a0

	const reauthOptions = getReauthOptions(mfaOptions, hasPasswordless);
	setReauthMethod(reauthOptions[0].value);

This throws an error, as reauthOptions is empty if there's no MFA options available.

I don't know how many customers actually use TELEPORT_ALLOW_NO_SECOND_FACTOR, but it's an issue we should probably solve separately from this specific problem.

---

As it is, the easiest way to check if this PR fixes the problem is to cherry pick it on branch/v17.

[ravicious]

This fixes the problem on v17.

I wanted to suggest adding some tests, but SSO MFA is going to land in v17 and useMfa already has tests for when MFA is not required.

Since strict null checks are not enabled, another option to guarantee that the "no MFA challenge" is always handled would be to use a discriminated union, e.g. { kind: 'no-challenge' } | { kind: 'challenge', foo: Bar }. This would also help with adding null checks "just in case", as functions that actually need to work on a challenge could accept only the union variant with the challenge.

It's tempting to use { required: false } | { required: true, foo: Bar }. Unfortunately, discriminated unions on boolean fields don't work well with strict null checks turned off.

[zmb3]

Thanks @ravicious

[public-teleport-github-review-bot]

@Joerger See the table below for backport results.

Branch	Result
branch/v17	Create PR