Skip to content

[v17] Fix WebUI Admin Action infinite retry with no MFA devices#51513

Merged
Joerger merged 1 commit intobranch/v17from
joerger/v17/fix-webui-admin-action-retry
Jan 30, 2025
Merged

[v17] Fix WebUI Admin Action infinite retry with no MFA devices#51513
Joerger merged 1 commit intobranch/v17from
joerger/v17/fix-webui-admin-action-retry

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Jan 27, 2025

Changelog: Fixed a bug where performing an admin action in the WebUI would hang indefinitely instead of getting an actionable error if the user has no MFA devices registered.

Backport #51134 to branch/v17

* Fix admin action retry method resulting in an infinite loop.

* Return an empty challenge and challenge response instead of undefined.

* Add api.getAdminActionMfaResponse and add new custom error message.

* Resolve comments.

* Don't swallow non-admin-action-required errors.

* Remove unnecessary awaits.

* Fix try/catch.
@Joerger Joerger added backport no-changelog Indicates that a PR does not require a changelog entry labels Jan 27, 2025
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Jan 27, 2025

Why no-changelog? This seems like a UX-impacting bug.

@ryanclark
Copy link
Copy Markdown
Member

Does the no-changelog label need to be removed?

@zmb3 zmb3 removed the no-changelog Indicates that a PR does not require a changelog entry label Jan 29, 2025
challengeScope: MfaChallengeScope.CHANGE_PASSWORD,
onMfaResponse: async mfaResponse =>
setWebauthnResponse(mfaResponse?.webauthn_response),
setWebauthnResponse(mfaResponse.webauthn_response),
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 this change?

Before we were tolerant of null/undefined values, now we are not. Is that intended?

Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Jan 29, 2025

Choose a reason for hiding this comment

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

Yes, returning null/undefined is what caused this bug - causing the admin mfa retry logic to think we haven't tried MFA already. Here's the original PR description:

#49679 and subsequently #50570 introduced a change where getMfaChallengeResponse could return null | undefined when the user had no MFA challenges (no devices or not required). fetchJsonWithMfaAuthnRetry expects getMfaChallengeResponse to return {} in these cases, and will retry with MFA until it gets either an empty or non-empty object.

This PR fixes the issue by:

  1. addressing the recursive fetchJsonWithMfaAuthnRetry which could result in an infinite loop. It's no longer recursive so it will only retry once even if null or undefined is received.
  2. Reverting part of https://github.com/gravitational/teleport/pull/50570/files and instead making getMfaChallengeResponse return {} so we can properly determine at any point whether an mfa response is undefined or an empty response resulting from a no-op challenge attempt (no devices or not required).

@Joerger Joerger requested a review from zmb3 January 29, 2025 19:08
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from ryanclark January 30, 2025 00:12
@Joerger Joerger added this pull request to the merge queue Jan 30, 2025
Merged via the queue into branch/v17 with commit 757a571 Jan 30, 2025
@Joerger Joerger deleted the joerger/v17/fix-webui-admin-action-retry branch January 30, 2025 23:35
@camscale camscale mentioned this pull request Feb 13, 2025
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.

5 participants