Skip to content

Clean up login modal in Connect after changes to MFA prompts#47883

Merged
ravicious merged 6 commits intomasterfrom
r7s/login-modal
Oct 25, 2024
Merged

Clean up login modal in Connect after changes to MFA prompts#47883
ravicious merged 6 commits intomasterfrom
r7s/login-modal

Conversation

@ravicious
Copy link
Copy Markdown
Member

@ravicious ravicious commented Oct 24, 2024

Closes #47322.

What needed fixing

Before #47153, the way MFA with Webauthn worked in Connect was that after submitting the credentials, the login form would immediately shift to prompting you for a key tap. Underneath, tshd would initiate appropriate calls to actually prompt the key tap. The actual key tap was handled through tsh with a regular CLI prompt. The Electron app was just masquerading as if it was somehow done through the Electron app and not tsh.

#47153 made it so that the CLI prompt is no longer used. The login procedure still goes through tsh, but when tsh is about to prompt the key tap, it sends an RPC to the Electron app. The Electron app then shows another modal (on top of the login modal, there's a bit of tech debt there) to ask for a key tap.

Now, as you can see in the recording in the issue closed by this PR, #47322, this introduced a bit of redundancy. The login modal would immediately ask you for key tap and then after a short delay another modal would show up asking for a tap.

How it was fixed

I noticed in useClusterLogin that we'd always do setWebauthnLogin({ prompt: 'tap' }); within onLoginWithLocal. Before the prompt changes, it was guarded by a check for MFA method:

const onLoginWithLocal = (
username: string,
password: string,
token: string,
secondFactor?: types.Auth2faType
) => {
if (secondFactor === 'webauthn') {
setWebauthnLogin({ prompt: 'tap' });
}

The check was removed in #47153, causing any kind of local MFA to immediately switch to prompting for a key after credentials were submitted, followed by being overlaid with another modal asking for the proper MFA method.

I removed the call to setWebauthnLogin from there completely and adjusted <FormLocal> so that it shows a progress bar while processing. Previously it'd only disable the fields.

I couldn't remove <PromptWebauthn>, as the passwordless flow still uses it for its own prompts. But since <PromptWebauthn> was no longer used for anything other than passwordless login, I renamed it to <PromptPasswordless>. Similarly, I renamed webauthnLogin to passwordlessLoginState, followed by a couple of similar renames to related values and types.


I verified that the passwordless flow and the new PIV prompts work correctly after those changes.

A couple of nice things about the new implementation:

  • We prompt for the key tap when the key is actually waiting for a tap. In the past, we'd prompt the user for a tap immediately which could result in the user tapping the key when the key wouldn't be even blinking yet.
  • If there's an error during the login RPC, the password field doesn't get reset and the user can immediately attempt to resubmit the form.

Demo

Before:

login-mfa.mov

After:

after.mov

@ravicious ravicious added the no-changelog Indicates that a PR does not require a changelog entry label Oct 24, 2024
@ravicious ravicious requested a review from gzdunek October 24, 2024 09:09
@github-actions github-actions Bot requested review from kiosion and ryanclark October 24, 2024 09:10
@aws-amplify-us-west-2

This comment was marked as off-topic.

@aws-amplify-us-west-2

This comment was marked as off-topic.

@ravicious
Copy link
Copy Markdown
Member Author

@gzdunek @kiosion @ryanclark I'd like to ship it before the test plan starts if possible. Despite the number of changed lines, the actual changes are rather simple, but I renamed a bunch of stuff hence the numbers.

Comment on lines +22 to +24
transparentBackground = false,
absolute = true,
hidden = false,
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool that we don't need a type at all 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, though idk how good that is. If someone wants to add, say, an enum prop in the future, they'll have to add the whole type anyway instead of just adding one line to an existing type. ;f

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from kiosion October 24, 2024 16:25
@ravicious ravicious added this pull request to the merge queue Oct 25, 2024
Merged via the queue into master with commit 41ea7a5 Oct 25, 2024
@ravicious ravicious deleted the r7s/login-modal branch October 25, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean up login modal in Connect after MFA changes

3 participants