Skip to content

A wizard for adding authentication devices#38100

Merged
bl-nero merged 3 commits intomasterfrom
bl-nero/new-auth-device-dialog-2
Feb 13, 2024
Merged

A wizard for adding authentication devices#38100
bl-nero merged 3 commits intomasterfrom
bl-nero/new-auth-device-dialog-2

Conversation

@bl-nero
Copy link
Copy Markdown
Contributor

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

Contributes to #36232
Contributes to #37616

This is a first pass at implementing the new designs. This PR covers all of the logic and interactions needed for the passkey dialog. Follow-up PRs will address layout and styling, as well as adding MFAs.

Figma designs

Demo

@bl-nero bl-nero added the no-changelog Indicates that a PR does not require a changelog entry label Feb 12, 2024
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Code and tests look good, but I'm not sure we should merge this until the styling is fixed. It's a relatively small PR as-is, and by merging what you have here we introduce a chance that we fail to backport styling updates and show users a set of screens that look half done.

Comment thread Makefile Outdated
@bl-nero
Copy link
Copy Markdown
Contributor Author

bl-nero commented Feb 13, 2024

Code and tests look good, but I'm not sure we should merge this until the styling is fixed. It's a relatively small PR as-is, and by merging what you have here we introduce a chance that we fail to backport styling updates and show users a set of screens that look half done.

@zmb3 Good catch, I forgot to put this change behind the flag that I prepared last week. Fixed it (it was just one statement that triggers the flow; the rest of the logic just lives there in parallel). The thing is, I'd still like to push this change through, since it's sizable enough, and I'd like to first integrate both passkeys and MFAs to the new flow, and only then work on the styling details.

@bl-nero bl-nero requested a review from zmb3 February 13, 2024 12:06
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

Looks good, I left some code-style comments.

@bl-nero
Copy link
Copy Markdown
Contributor Author

bl-nero commented Feb 13, 2024

@gzdunek FYI, I decided to leave the error message in the backend as it is, since it's consistent with CLI, and instead translate it in the UI code (and clearly mark dependencies on both sides so it's visible where we depend on it).

@bl-nero bl-nero enabled auto-merge February 13, 2024 15:53
@bl-nero bl-nero added this pull request to the merge queue Feb 13, 2024
Merged via the queue into master with commit 9684b50 Feb 13, 2024
@bl-nero bl-nero deleted the bl-nero/new-auth-device-dialog-2 branch February 13, 2024 16:23
bl-nero added a commit that referenced this pull request Feb 22, 2024
* A wizard for adding authentication devices

* Hide changes between localStorage flag

* Review
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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants