Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix useMfa error handling #50844

Merged
merged 6 commits into from
Jan 16, 2025
Merged

Fix useMfa error handling #50844

merged 6 commits into from
Jan 16, 2025

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Jan 7, 2025

The original error is now displayed in the MFA dialog, and the MFA canclled by user error is displayed at the top level after the MFA dialog is closed:

Screenshot 2025-01-07 at 12 22 32 PM
Screenshot 2025-01-07 at 12 23 37 PM

Fixes #50582

This will be backported with #50529

@Joerger Joerger added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Jan 7, 2025
@github-actions github-actions bot requested review from kimlisa and kiosion January 7, 2025 20:25
@Joerger Joerger requested a review from gzdunek January 7, 2025 20:26
@Joerger
Copy link
Contributor Author

Joerger commented Jan 7, 2025

@gzdunek This is the same issue you pointed out here. I just separated the fix 93ec6b7 into this new PR, in case #50373 is scrapped.

@zmb3
Copy link
Collaborator

zmb3 commented Jan 8, 2025

Can we use the American-English spelling (canceled) instead of the British-English spelling (cancelled)?

@Joerger Joerger force-pushed the joerger/fix-useMfa-error-state branch from 01d553d to dd33f6a Compare January 8, 2025 17:47
@Joerger Joerger force-pushed the joerger/fix-useMfa-error-state branch from dd33f6a to 6de6710 Compare January 8, 2025 20:27
@Joerger Joerger requested a review from gzdunek January 13, 2025 19:37
web/packages/teleport/src/lib/useMfa.ts Outdated Show resolved Hide resolved
web/packages/teleport/src/lib/useMfa.ts Outdated Show resolved Hide resolved
@Joerger Joerger requested a review from gzdunek January 14, 2025 18:15
web/packages/teleport/src/lib/useMfa.test.tsx Outdated Show resolved Hide resolved
@Joerger
Copy link
Contributor Author

Joerger commented Jan 15, 2025

@kimlisa @kiosion Friendly ping to review

}: Props) {
if (!challenge && attempt.status !== 'error') return;
const showMfaDialog =
!!challenge ||
Copy link
Contributor

Choose a reason for hiding this comment

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

i just want to make sure this is show mfa dialog if there IS a challenge? i think that not is throwing me off (brain farting)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't know why I thought it was more readable this way, updated to:

  const showError =
    attempt.status === 'error' && !(attempt.error instanceof MfaCanceledError);

  if (!challenge && !showError) return;

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from kiosion January 16, 2025 18:12
@Joerger Joerger enabled auto-merge January 16, 2025 18:35
@Joerger Joerger added this pull request to the merge queue Jan 16, 2025
Merged via the queue into master with commit c8a6728 Jan 16, 2025
41 checks passed
@Joerger Joerger deleted the joerger/fix-useMfa-error-state branch January 16, 2025 19:11
@public-teleport-github-review-bot

@Joerger See the table below for backport results.

Branch Result
branch/v17 Failed

Joerger added a commit that referenced this pull request Jan 16, 2025
* Fix useMfa cancel logic to avoid duplicate error messages across dialog layers.

* Add MfaCanceledError and use promise.reject for mfa cancel.

* Address comments.

* Fix test.

* Remove outdated comment.

* Add comment; Simplify who dialog logic.
mvbrock pushed a commit that referenced this pull request Jan 18, 2025
* Fix useMfa cancel logic to avoid duplicate error messages across dialog layers.

* Add MfaCanceledError and use promise.reject for mfa cancel.

* Address comments.

* Fix test.

* Remove outdated comment.

* Add comment; Simplify who dialog logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 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.

web: canceling MFA dialog shows same error twice
4 participants