Skip to content

Remove the old AddDevice dialog and substitute it with the new wizard#38393

Merged
bl-nero merged 6 commits intomasterfrom
bl-nero/new-auth-device-dialog-5
Feb 21, 2024
Merged

Remove the old AddDevice dialog and substitute it with the new wizard#38393
bl-nero merged 6 commits intomasterfrom
bl-nero/new-auth-device-dialog-5

Conversation

@bl-nero
Copy link
Copy Markdown
Contributor

@bl-nero bl-nero commented Feb 19, 2024

Fixes #37616

Changelog: Updated the dialog for adding new authentication methods in the account settings screen.

Also cleans up the accompanying local storage flag.
@bl-nero bl-nero requested a review from kimlisa February 19, 2024 17:24
@github-actions
Copy link
Copy Markdown
Contributor

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 changelog: followed by the changelog entries for the PR.

${'on'} | ${false} | ${false} | ${true}
${'optional'} | ${true} | ${true} | ${true}
${'optional'} | ${false} | ${false} | ${true}
${'otp'} | ${true} | ${false} | ${true}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why was this test case removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zmb3 It got broken when I removed a redundant check, but it's also impossible in real life: the OTP option implies passwordless being disabled. The auth server refuses to start if these options are in conflict. Similarly, we don't check what happens if 2fa is set to "off" and passwordless is set to true.

If you still think it's good to check these seemingly impossible conditions, I can bring back this case (and extend the "off" case) to verify that we always enable passwordless when the backend tells us so, regardless of the MFA setting itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(On the second thought, I just went on and included these anyway.)

Comment thread web/packages/teleport/src/Account/Account.tsx Outdated
Comment thread web/packages/teleport/src/Account/Account.tsx Outdated
bl-nero and others added 3 commits February 20, 2024 13:33
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
@bl-nero bl-nero requested a review from zmb3 February 20, 2024 12:40
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Feb 20, 2024

Looking good. The only issue I noticed is that I can't remove a passkey, even though I have other non-passwordless methods for logging in.

image

(This is also not a great UX, because the dialog continues to show me a "remove" button over and over again, but attempts to click it will always fail with the same outcome)

@bl-nero
Copy link
Copy Markdown
Contributor Author

bl-nero commented Feb 21, 2024

The only issue I noticed is that I can't remove a passkey, even though I have other non-passwordless methods for logging in.

Filed #38433 for it, as this is not something introduced by this PR.

isSso={isSso}
canAddPasskeys={canAddPasskeys}
canAddMFA={canAddMFA}
canAddMFA={canAddMfa}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: we can change it here as well:

Suggested change
canAddMFA={canAddMfa}
canAddMfa={canAddMfa}

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from ryanclark February 21, 2024 17:40
@bl-nero bl-nero added this pull request to the merge queue Feb 21, 2024
Merged via the queue into master with commit 8d8a379 Feb 21, 2024
@bl-nero bl-nero deleted the bl-nero/new-auth-device-dialog-5 branch February 21, 2024 18:14
bl-nero added a commit that referenced this pull request Feb 22, 2024
…#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>
github-merge-queue Bot pushed a commit that referenced this pull request Feb 23, 2024
)

* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refresh "add new passkey" dialog

4 participants