Use Web MFA dialog for admin actions

[Joerger]

Changelog: Add MFA dialog in the WebUI for Admin actions instead of automatically opening up a webauthn/sso pop up.

Adds a global MFA context for prompting MFA from non-react contexts. Currently this is only used for admin actions, which prompts for MFA from basic API requests.

Depends on #49794

[gzdunek]

Three levels of dialogs 💀

Maybe this one should be closed as soon as the user selects a method? I see an error in the background anyways and here I can't take any action.

[Joerger]

@gzdunek

Three levels of dialogs 💀

Yeah, it might be nice to do some type of moving flow like we do for device management. e.g. edit user -> verify your identity in the same dialog, with option to continue (mfa challenge) or back (edit user page with error telling user mfa is required).

Maybe this one should be closed as soon as the user selects a method? I see an error in the background anyways and here I can't take any action.

Thanks for pointing this out, the cancellation logic turned out to be pretty fragile and only worked well for per-session MFA with SSH sessions. I've fixed it so you should now see just one error at a time - 0149c12

Also, you should be able to retry or cancel in the mfa dialog. Let me know if you still get locked with my changes. useMfa isn't safe to be called multiple times at the same time, so it's possible you're running into the consequences of that somehow?

[avatus]

@bl-nero take a look at this one when you get some time please

[ryanclark]

Should this unset the mfaContext in auth/api on unmount? (I'm not sure if that matters)

[Joerger]

I'm not sure how unmounting in React works, how would I go about this?

[bl-nero]

@Joerger To execute code on unmount, you can add an useEffect that will only execute once and return a cleanup function. The callback will be executed when component is mounted, and then the returned cleanup function will be executed when it's being unmounted. Example:

useEffect(
  () => {
    return () => {
      console.log('unmounted!');
    }
  },
  []
);

The cleanup function is executed by React every time when the useEffect dependencies change, just before the next effect callback is called or when component is unmounted. This is typically used to cancel network requests, making sure that no excess bandwidth is consumed, but most of all to protect out-of-order responses to force the component into an unexpected state. But here, since there are no dependencies, the cleanup function will only be executed once.

(Also note that the cleanup function is the very reason why you can't pass an async function directly as an useEffect callback, and you'd have to wrap it into a synchronous one. React would treat the returned promise object as a function and try to call it, with rather predictable effect.)

[ryanclark]

Does this surface to the user? Should we be more helpful than "This is a bug"?

[ryanclark]

Hm, looking at this again - this error doesn't go anywhere, as it's thrown within a setTimeout

[Joerger]

You're right, for now I'll remove the timeout and just return a better error message:

Failed to set up MFA prompt for admin action. Please try refreshing the page to try again. If the issue persists, contact support as this is likely a bug.

[bl-nero]

Consider using an initializer function. It will save you a re-render and make the state more consistent.

Suggested change

	const [mfaCtx, setMfaCtx] = useState<MfaContextValue>();
	if (!mfaCtx) {
	const mfaCtx = { getMfaChallengeResponse };
	setMfaCtx(mfaCtx);
	auth.setMfaContext(mfaCtx);
	api.setMfaContext(mfaCtx);
	}
	const [mfaCtx, setMfaCtx] = useState<MfaContextValue>(()=> {
	const mfaCtx = { getMfaChallengeResponse };
	auth.setMfaContext(mfaCtx);
	api.setMfaContext(mfaCtx);
	return mfaCtx;
	});

[bl-nero]

Hmm. This is quite a hack. The fact that auth service started to depend on an MFA context that is injected on the top level by context provider makes me think that it should be turned into a class (just like the MFA service) that is instantiated by TeleportContext. This would allow establishing a link to the MFA service/context/whatever when constructing the auth service. Will it be a big refactoring?

[Joerger]

It does sound like a big refactor to me, but I'm not really in a position with my level of typescript/React experience to judge or carry out this type of refactor. I also have some other important work to move onto. Do you think it's something we can live with today and fix tomorrow, or do we need to scrap this approach for now?

Note that without this change, we still support SSO MFA for admin actions, we just don't provide the user with the choice between SSO MFA and Webauthn when applicable. Instead we automatically open the Webauthn or SSO MFA pop up without a dialog, which may be jarring, but not the end of the world. In the long term we should find a solution.

[bl-nero]

@Joerger I think that given above, it would be best to continue with the PR as is, and treat this part as a technical debt that we take to improve UX. Can you please create a tracking issue for this refactoring?

[Joerger]

#50882

[avatus]

I think that converting the auth and api objects into services/classes wouldn't be too garly. the biggest issue i see is they they depend on each other, so we'd want to handle that circular dependency somehow. The easiest way would be to store a reference to the teleport context inside each of these classes, then update any calls like auth.myMethod to this.ctx.authServices.myMethod for example.

then just instantiate each of the new services inside the Teleport context like all our other services. We could pass the mfaService as an argument to the authService constructor if we super care about order on instantiation but tbh, if we are passing the entire teleport context itself, it doesnt matter.

should we reference the parent Teleport Context object inside these two new services? eh, maybe maybe not. I dont see a problem with it.

then we can just update every callsite of api and auth to instead reference ctx.apiService and ctx.authService.

Thats my initial thoughts off the dome. I converted auth and api to classes (id only look at auth tbh, api is a mess still) and i would need to check the validity/add the comments back, but this is when i discovered the circular dep.

[avatus]

Ill try the Context reference thing first and see how it works and maybe take it from there (separate PRs of course)

[rosstimothy]

Let's hold off on this and try to implement a less hacky solution from the start.