Add capability of adding MFA to the new auth device wizard#38260
Add capability of adding MFA to the new auth device wizard#38260
Conversation
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
zmb3
left a comment
zmb3
left a comment
There was a problem hiding this comment.
Approved with a few small comments.
| <ButtonPrimary type="submit"> | ||
| {usage === 'passwordless' | ||
| ? 'Save the Passkey' | ||
| : 'Save the MFA method'} |
There was a problem hiding this comment.
Can the button just say "Save" ??
The title of the dialog box already says "Save the MFA method" so I don't see why we need to repeat that much text twice.
There was a problem hiding this comment.
@zmb3 That was what we settled down on with the UX team. @rishibarbhaya-design, could you confirm?
There was a problem hiding this comment.
For what it's worth, when you edit an auth connector or a role, the save button says "SAVE CHANGES" - it does not say "Save the role" or "Save the auth connector"
| } | ||
|
|
||
| function SavePasskeyStep({ | ||
| export function SaveDeviceStep({ |
There was a problem hiding this comment.
| export function SaveDeviceStep({ | |
| export function SaveMfaStep({ |
Perhaps a more generic name? (Since some would argue that TOTP is not a "device")
There was a problem hiding this comment.
I'm using an "auth device" as an abstract term for all of these, the entire wizard is called AddAuthDeviceWizard, and MFA is actually a less generic name (since it explicitly excludes passkeys). Note that I inherited the "device" terminology from existing code (AddDevice.tsx, useAddDevice, etc.). If we want to rename the concept of "device" to something else, perhaps we could consider "auth method", and rename the appropriate elements to AddAuthMethodWizard, SaveMethodStep, etc. However, as the "device" terminology is also used across our backend codebase, I'd say this would end up being more confusing than useful.
There was a problem hiding this comment.
I still think SaveMfaStep is best.
"Devices" is an overloaded term in Teleport, because we also have Device Trust which keeps an inventory of trusted devices in the backend.
There was a problem hiding this comment.
OK, I can rename it, but I'd like to know your ideas about renaming CreateMfaBox, then. SInce with SaveMfaStep, MFA will actually "MFA or a passkey", while in case of CreateMfaBox, MFA will mean exactly MFA. Is there any specific name I could then put on "MFA, but really MFA"?
There was a problem hiding this comment.
(This, plus a bunch of other variables with mfa in name and whose names will then be confusing.)
| <br /> | ||
| We recommend{' '} | ||
| <Link href="https://authy.com/download/" target="_blank"> | ||
| Authy |
There was a problem hiding this comment.
I know this has always been here, but I wonder if it should stay.
Do we actually recommend authy? If so, why? I don't know that anyone can answer these questions.
cc @xinding33 @roraback what do you guys think about this?
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
Looks good! Thanks for leaving JSDocs on the new props that you added.
public-teleport-github-review-bot
Bot
removed the request for review
from ibeckermayer
bl-nero
added
the
no-changelog
* Add capability of adding MFA to the new auth device wizard * Review * Review * License
) * Add an option to enable a new Add Device dialog design (#37747) * Add an option to enable a new Add Device dialog design Also deduplicate code from storageService functions that parse JSON. * lint * A wizard for adding authentication devices (#38100) * A wizard for adding authentication devices * Hide changes between localStorage flag * Review * Add capability of adding MFA to the new auth device wizard (#38260) * Add capability of adding MFA to the new auth device wizard * Review * Review * License * Make the auth device wizard look pretty (#38353) * Add capability of adding MFA to the new auth device wizard * Review * Review * Make the auth device wizard look pretty * License * Review * Fix lint * Remove the old AddDevice dialog and substitute it with the new wizard (#38393) * Remove the old AddDevice dialog Also cleans up the accompanying local storage flag. * Rename `canAddMFA` Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * Update a success message Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * review * Review --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
4 participants
Contributes to #36232
Contributes to #37616
This PR doesn't include proper styling; a follow-up PR will make it pretty.
This UI is hidden until ready; to view it, set
grv_new_add_auth_device_dialogtotruein the app's local storage.Figma designs
Demo