Web-Discover: Add support for connection testers with per-session MFA enabled

[kimlisa]

resolves #16702

Description

Implements the frontend part of RFD 0111
backend here: #22528

Before connection testing, we check if MFA is required.
If required:

• we render a MFA dialog that asks user to provide MFA credentials
• take this response and send it with the test connection request

Only supported for resource type node, kube, and database (no support for windows desktops yet)

Testing

Manually tested for a node, kube, and a database resource with a user assigned a role with option.require_session_mfa: true

If you want to test run yourself, this PR has all it needs, or I have a cloud cluster already running with changes if interested

Demo testing connection to a node

requiresessionmfademo.mov

[kimlisa]

@ibeckermayer @ryanclark friendly ping

[ibeckermayer]

Seems like something we should add to the manual test plan.

[ibeckermayer]

Suggested change

	mfaAuthnResponse: MfaAuthnResponse
	mfaAuthnResponse?: MfaAuthnResponse

[ibeckermayer]

Suggested change

	// runConnectionDiagnostic will initially make a call to check if
	// resource target requires MFA authentication. After this initial
	// check depending on if user successfully authenticated or not (
	// determined by the presence of the token field), will make a call
	// to test connection.
	// If mfaAuthnResponse is provided, or if it's undefined and this function determines
	// MFA authentication to be unnecessary for the given diagnostic request, it will run
	// the diagnostic.
	//
	// If mfaAuthnResponse is undefined, runConnectionDiagnostic will initially
	// make a call to check if resource target requires MFA authentication. If it does,
	// it will set showMfaDialog to true, and it is up the parent component to ensure that this
	// change in showMfaDialog's state to true causes the user to be prompted for MFA authentication,
	// and that the subsequent call to runConnectionDiagnostic is made with the mfaAuthnResponse
	// derived from that process.

[kimlisa]

i found your explanation confusing, so i cleared it up like this (i try to follow golang convention commenting so i don't have to think too hard when switching):

  // runConnectionDiagnostic depending on the value of `mfaAuthnResponse` does the following:
  //   1) If param `mfaAuthnResponse` is undefined or null, it will check if MFA is required.
  //      - If MFA is required, it sets a flag that indicates a users
  //        MFA credentials are required, and skips the request to test connection.
  //      - If MFA is NOT required, it makes the request to test connection.
  //   2) If param `mfaAuthnResponse` is defined, it skips checking if MFA is required,
  //      and makes the request to test connection.

[ibeckermayer]

How does setAttempt ever get set to success here?

[kimlisa]

it doesn't or rather it's not necessary. i think this component was created in such that after a successful "submit" the control is passed back to the parent (with the onAuthenticate or onMfaReponse func). I thought it made sense b/c there shouldn't be anything else that needs to be done after user submits. Perhaps it would've made more sense to call onClose before onAuthenticate/onMfaResponse. So if we are closing the dialog, it's clearer that we don't need anything to re-render, which is the purpose of setAttempt(success)

[ibeckermayer]

Maybe worth a concise comment explaining this because it's not the standard usage pattern.

[ibeckermayer]

A comment about the distinct prompt shapes and how they change the behavior of the hook would be helpful.

[kimlisa]

@ibeckermayer @ryanclark i made this commit as an example: 90e5050, where i am using snake cased fields outside the service layer...

was wondering if that was okay, the conversion of field names just to satisfy JS convention was getting tiring. will change back if requested.

context: #22528 (comment)

[kimlisa]

Seems like something we should add to the manual test plan.

I will do this in another PR. there is a lot that needs to be added/updated in the test plan

[ryanclark]

Can we name this something other than Default?

[kimlisa]

asked discussed in call, renamed to DefaultProps

[ibeckermayer]

@ibeckermayer @ryanclark i made this commit as an example: 90e5050, where i am using snake cased fields outside the service layer...

was wondering if that was okay, the conversion of field names just to satisfy JS convention was getting tiring. will change back if requested.

context: #22528 (comment)

My preference is that we change the backend's responses to use camelCase like you and Alexey were doing. AFAIK the web endpoints are all only used by the web client, and so there's no good reason not to tailor them for the web client in a way that reduces boilerplate.

I'd prefer to avoid doing it ad-hoc like this, as it will make the pattern inconsistent and therefore more difficult to change to the correct solution, whatever that is.

[kimlisa]

My preference is that we change the backend's responses to use camelCase like you and Alexey were doing. AFAIK the web endpoints are all only used by the web client, and so there's no good reason not to tailor them for the web client in a way that reduces boilerplate.

Pushing for camel consistency is a losing battle b/c we have a lot more devs touching the web layer. Hard to reinforce camel when everything else is snaked so folks will naturally default to snake.

I'd prefer to avoid doing it ad-hoc like this, as it will make the pattern inconsistent and therefore more difficult to change to the correct solution, whatever that is.

Most devs will default to snake, so if we all get onboard, eventually it'll become consistent? 🥲 But the correct solution is to transition into grpc.

Btw, I double checked that ryan was okay with my approach.

[public-teleport-github-review-bot]

@kimlisa See the table below for backport results.

Branch	Result
branch/v11	Failed
branch/v12	Failed