Conversation
… is off
When MFA verification is disabled for WebAuthn, POST /factors/{id}/verify
returned the mfa_webauthn_enroll_not_enabled error code, even though the
message says verification is disabled. The challenge endpoint and the phone
and TOTP branches of verify already return their *_verify_not_enabled codes,
so clients checking error.code could not tell the two cases apart.
Return mfa_webauthn_verify_not_enabled instead, and add a test covering the
verify-disabled error code for phone, TOTP and WebAuthn factors.
Author
|
@fadymak this one is ready to review whenever you get somechance |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
When MFA verification is disabled for WebAuthn (
GOTRUE_MFA_WEB_AUTHN_VERIFY_ENABLED=false),POST /factors/{id}/verifyreturns:{ "code": 422, "error_code": "mfa_webauthn_enroll_not_enabled", "msg": "MFA verification is disabled for WebAuthn" }The message says verification is disabled, but the error code says enrollment is. Every other "verify disabled" path already returns the matching code:
POST /factors/{id}/challengemfa_phone_verify_not_enabledmfa_totp_verify_not_enabledmfa_webauthn_verify_not_enabledPOST /factors/{id}/verifymfa_phone_verify_not_enabledmfa_totp_verify_not_enabledmfa_webauthn_enroll_not_enabledBoth codes are part of the public
ErrorCodeunion in auth-js, so a client that branches onerror.codegets the wrong code at this one call site. It looks like a copy-paste slip from when WebAuthn MFA was added (#1775).What is the new behavior?
VerifyFactorreturnsmfa_webauthn_verify_not_enabledfor disabled WebAuthn verification, matchingChallengeFactorand the phone/TOTP branches. The message is unchanged.Additional context
TestVerifyFactorVerifyDisabled, a table test that checks the verify-disabled error code for phone, TOTP and WebAuthn factors. The WebAuthn case fails onmaster(expected: "mfa_webauthn_verify_not_enabled", actual: "mfa_webauthn_enroll_not_enabled") and passes with this change.go test ./internal/api/...(all packages pass),gofmt -s,go vet,staticcheck.error_codevalue on this path.