feat: expose MFA context on token error cause with isMfaRequiredError type guard#169
feat: expose MFA context on token error cause with isMfaRequiredError type guard#169subhankarmaiti wants to merge 2 commits into
cause with isMfaRequiredError type guard#169Conversation
MfaRequiredError class for detecting MFA challenges from token endpointscause with isMfaRequiredError type guard
| error_description: err.error_description ?? '', | ||
| message: (e as { message?: string }).message, | ||
| }; | ||
| if (err.error === 'mfa_required' && err.cause) { |
There was a problem hiding this comment.
Here we are just checking if err.cause exists or not.
Should this function validate that err.cause is actually an object (not just truthy) before accessing properties on it ?
What happens if cause is a string or number, wouldn't err.cause.mfa_token throw or return unexpected values ?
Since e is unknown from the catch block, the as cast on line 143 doesn't provide runtime safety.
| message: (e as { message?: string }).message, | ||
| }; | ||
| if (err.error === 'mfa_required' && err.cause) { | ||
| base.mfa_token = err.cause.mfa_token as string | undefined; |
There was a problem hiding this comment.
Nit:
Can we add a typeof check here instead of casting ? Something like typeof err.cause.mfa_token === 'string' ? err.cause.mfa_token : undefined ?
This way we don't silently assign a non-string value if the server response shape changes.
| * Describes which MFA factors the user must challenge or enroll. | ||
| */ | ||
| export interface MfaRequirements { | ||
| challenge?: Array<{ type: string }>; |
There was a problem hiding this comment.
Should type be a string literal union (e.g. otp' | 'oob' | 'webauthn-roaming' | 'webauthn-platform' | 'push-notification' | 'phone' | 'email') instead of plain string ?
That way consumers get autocomplete and don't need to cast when branching on the type.
| return ( | ||
| error instanceof Error && | ||
| (error as { cause?: OAuth2Error }).cause?.error === 'mfa_required' && | ||
| typeof (error as { cause?: OAuth2Error }).cause?.mfa_token === 'string' |
There was a problem hiding this comment.
The return type says { cause: OAuth2Error & { error: 'mfa_required'; mfa_token: string } } & Error but should this also include mfa_requirements as optional ?
Without it, consumers who destructure error.cause won't see mfa_requirements in the narrowed type unless they separately cast it.
|
|
||
| return TokenResponse.fromTokenEndpointResponse(tokenEndpointResponse); | ||
| } catch (e) { | ||
| throw new TokenByCodeError('There was an error while trying to request a token.', e as OAuth2Error); |
There was a problem hiding this comment.
getTokenByCode still uses e as OAuth2Error instead of toOAuth2Error(e). Is there a reason this method is excluded ? If a user hits step-up MFA after a code exchange, the MFA context on the error would be lost. Even if it's not expected today, using toOAuth2Error here would keep all catch blocks consistent.
|
|
||
| return TokenResponse.fromTokenEndpointResponse(tokenEndpointResponse); | ||
| } catch (e) { | ||
| throw new TokenByClientCredentialsError('There was an error while trying to request a token.', e as OAuth2Error); |
There was a problem hiding this comment.
| */ | ||
| export function isMfaRequiredError( | ||
| error: unknown | ||
| ): error is { cause: OAuth2Error & { error: 'mfa_required'; mfa_token: string } } & Error { |
There was a problem hiding this comment.
The return type narrows to { cause: OAuth2Error & { ... } } & Error. But ApiError declares cause as public cause?: OAuth2Error (optional).
After the guard passes, TypeScript will believe cause is definitely defined but the narrowed type is & Error, not & ApiError. This means consumers lose access to .code, .name, and other ApiError properties after narrowing.
Was this intentional ? Would narrowing to & ApiError (or a union of the known error classes) be better for DX ?
|
We may need to add coverage for:
|
|
closing this as we have all change sin a single PR #164 |
Summary
OAuth2Errorinterface with optionalmfa_tokenandmfa_requirementsfieldsMfaRequirementsinterface describing which factors the user must challenge or enrollisMfaRequiredError(error)type guard that narrows any caught error to one withcause.mfa_token(string) andcause.mfa_requirementsguaranteed presentcauseof existing error classes via atoOAuth2ErrorhelperMotivation
When the Auth0 server requires MFA, it returns a 403 with
error: "mfa_required",mfa_token, and optionallymfa_requirements. Previously these fields werediscarded when the response was wrapped into
TokenByPasswordError,TokenByRefreshTokenError, etc.This change is non-breaking:
instanceof TokenByPasswordErrorcatches still work identicallycausegains two new optional fields (mfa_token,mfa_requirements) that areundefinedfor non-MFA errorsisMfaRequiredErrortype guard is additive — consumers opt-in to the new behaviorHow it works
Server returns 403:
{ error: "mfa_required", error_description: "...", mfa_token: "Fe26...", mfa_requirements: {...} }
openid-client throws ResponseBodyError with full response body in .cause
auth-client.ts catch block calls toOAuth2Error(e):
→ detects mfa_required, extracts mfa_token and mfa_requirements from response body
→ returns OAuth2Error with all fields populated
Existing error class (e.g. TokenByPasswordError) is thrown with enriched cause
Consumer catches error and uses isMfaRequiredError(error) to detect + narrow
Examples
Basic: detect MFA and start challenge