Fix WebUI Admin Action infinite retry with no MFA devices#51134
Fix WebUI Admin Action infinite retry with no MFA devices#51134
Conversation
Joerger
force-pushed
the
joerger/fix-webui-admin-action-retry
branch
from
21430bc to
61b4600
Compare
Joerger
added
backport/branch/v17
no-changelog
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
Looks good, but there are still a few failing tests, even after fixing those in api.test.ts.
Joerger
force-pushed
the
joerger/fix-webui-admin-action-retry
branch
from
61b4600 to
cbc4c2a
Compare
There was a problem hiding this comment.
Leaving a preemptive approval, but how do I even test it, bar what Xin described in the issue? If I set second factor to optional, I cannot even log in as a user with no MFA. The login form shows "t is undefined" error and I'm forced to select a multi-factor type anyway.
I managed to work around this by choosing an authenticator app as the MFA type and then providing a bogus code. But how is it supposed to work for normal users? 🤔
There was a problem hiding this comment.
You have to make an SSO user, which wouldn't have an MFA device until you add one
| return await api.fetch(url, customOptions, mfaResponse); | ||
| } catch (err) { | ||
| // error reading JSON | ||
| const message = response.ok | ||
| ? err.message | ||
| : `${response.status} - ${response.url}`; | ||
| throw new ApiError({ message, response, opts: { cause: err } }); | ||
| } | ||
|
|
||
| if (response.ok) { | ||
| return json; | ||
| } | ||
|
|
||
| /** This error can occur in the edge case where a role in the user's certificate was deleted during their session. */ | ||
| const isRoleNotFoundErr = isRoleNotFoundError(parseError(json)); | ||
| if (isRoleNotFoundErr) { | ||
| websession.logoutWithoutSlo({ | ||
| /* Don't remember location after login, since they may no longer have access to the page they were on. */ | ||
| rememberLocation: false, | ||
| /* Show "access changed" notice on login page. */ | ||
| withAccessChangedMessage: true, | ||
| }); | ||
| return; | ||
| // Retry with MFA if we get an admin action MFA error. | ||
| if (!mfaResponse && isAdminActionRequiresMfaError(err)) { | ||
| mfaResponse = await api.getAdminActionMfaResponse(); | ||
| return await api.fetch(url, customOptions, mfaResponse); |
There was a problem hiding this comment.
Yeah, it's not a big deal, but if you have a promise, you can return it from an async function without awaiting.
I'm not sure if there's a lint rule which would automatically take care of this. await-thenable and no-misused-promises seem to be about different cases entirely. typescript-eslint/typescript-eslint#8517
There was a problem hiding this comment.
I see, I wasn't sure how the try/catch would work without the first await
There was a problem hiding this comment.
No problem, reverted that one.
* 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.
…51513) * 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.
…nal#51134) * 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.
3 participants
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.
#49679 and subsequently #50570 introduced a change where
getMfaChallengeResponsecould returnnull | undefinedwhen the user had no MFA challenges (no devices or not required).fetchJsonWithMfaAuthnRetryexpectsgetMfaChallengeResponseto 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:
fetchJsonWithMfaAuthnRetrywhich could result in an infinite loop. It's no longer recursive so it will only retry once even if null or undefined is received.getMfaChallengeResponsereturn{}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).Closes #51105