From f3f596fbb2ff4cf975692d7826910849cfd42e9e Mon Sep 17 00:00:00 2001 From: Baozier Date: Thu, 26 Sep 2024 18:06:37 -0400 Subject: [PATCH] Allow resend sms mfa or fallback to email (#172) --- docs/auth-server.md | 2 +- .../__tests__/normal/identity-mfa.test.tsx | 334 ++++++++++++++++++ server/src/configs/locale.ts | 12 + server/src/configs/route.ts | 1 + server/src/handlers/identity/mfa.tsx | 69 +++- server/src/routes/identity.tsx | 10 +- server/src/services/kv.ts | 16 +- server/src/views/AuthorizeOtpMfa.tsx | 1 - server/src/views/AuthorizeSmsMfa.tsx | 95 ++++- 9 files changed, 521 insertions(+), 19 deletions(-) diff --git a/docs/auth-server.md b/docs/auth-server.md index f91fec3..d6ee4d3 100644 --- a/docs/auth-server.md +++ b/docs/auth-server.md @@ -318,7 +318,7 @@ npm run prod:deploy ### ALLOW_EMAIL_MFA_AS_BACKUP - **Default:** true -- **Description:** This setting allows users to use email-based MFA as an alternative method for signing in if they are enrolled in OTP MFA and not enrolled in email MFA. +- **Description:** This setting allows users to use email-based MFA as an alternative method for signing in if they are enrolled in OTP MFA or SMS MFA and not enrolled in email MFA. [Email functionality setup required](#email-functionality-setup) ### ACCOUNT_LOCKOUT_THRESHOLD diff --git a/server/src/__tests__/normal/identity-mfa.test.tsx b/server/src/__tests__/normal/identity-mfa.test.tsx index 96728cf..ffc2a42 100644 --- a/server/src/__tests__/normal/identity-mfa.test.tsx +++ b/server/src/__tests__/normal/identity-mfa.test.tsx @@ -717,6 +717,8 @@ describe( expect(document.getElementsByTagName('select').length).toBe(1) expect(document.getElementsByName('phoneNumber').length).toBe(1) expect(document.getElementsByTagName('form').length).toBe(1) + expect(document.getElementById('resend-btn')?.classList).toContain('hidden') + expect(document.getElementById('switch-to-email')).toBeFalsy() const code = getCodeFromParams(params) expect((await mockedKV.get(`${adapterConfig.BaseKVKey.SmsMfaCode}-${code}`) ?? '').length).toBeFalsy() @@ -803,6 +805,8 @@ describe( expect(phoneNumberEl.disabled).toBeTruthy() expect(document.getElementsByName('code').length).toBe(1) expect(document.getElementsByTagName('form').length).toBe(1) + expect(document.getElementById('resend-btn')?.classList).not.toContain('hidden') + expect(document.getElementById('switch-to-email')).toBeTruthy() const code = getCodeFromParams(params) const mfaCode = await mockedKV.get(`${adapterConfig.BaseKVKey.SmsMfaCode}-${code}`) ?? '' @@ -823,6 +827,58 @@ describe( }, ) + test( + 'could disable fall back to email mfa', + async () => { + process.env.SMS_MFA_IS_REQUIRED = true as unknown as string + process.env.TWILIO_ACCOUNT_ID = '123' + process.env.TWILIO_AUTH_TOKEN = 'abc' + process.env.TWILIO_SENDER_NUMBER = '+1231231234' + process.env.ALLOW_EMAIL_MFA_AS_BACKUP = false as unknown as string + + const mockFetch = vi.fn(async () => { + return Promise.resolve({ ok: true }) + }) as Mock + global.fetch = mockFetch + + await insertUsers( + db, + false, + ) + + await db.prepare('update "user" set "smsPhoneNumber" = ?, "smsPhoneNumberVerified" = ?').run( + '+16471231234', + 1, + ) + + const params = await prepareFollowUpParams(db) + + const res = await app.request( + `${routeConfig.IdentityRoute.AuthorizeSmsMfa}${params}`, + {}, + mock(db), + ) + const html = await res.text() + const dom = new JSDOM(html) + const document = dom.window.document + expect(document.getElementsByTagName('select').length).toBe(1) + const phoneNumberEl = document.getElementById('form-phoneNumber') as HTMLInputElement + expect(phoneNumberEl.value).toBe('********1234') + expect(phoneNumberEl.disabled).toBeTruthy() + expect(document.getElementsByName('code').length).toBe(1) + expect(document.getElementsByTagName('form').length).toBe(1) + expect(document.getElementById('resend-btn')?.classList).not.toContain('hidden') + expect(document.getElementById('switch-to-email')).toBeFalsy() + + process.env.SMS_MFA_IS_REQUIRED = false as unknown as string + process.env.TWILIO_ACCOUNT_ID = '' + process.env.TWILIO_AUTH_TOKEN = '' + process.env.TWILIO_SENDER_NUMBER = '' + global.fetch = fetchMock + process.env.ALLOW_EMAIL_MFA_AS_BACKUP = true as unknown as string + }, + ) + test( 'should pass through if request failed', async () => { @@ -1152,6 +1208,125 @@ describe( }, ) +describe( + 'post /resend-sms-mfa', + () => { + test( + 'could resend mfa', + async () => { + process.env.SMS_MFA_IS_REQUIRED = true as unknown as string + process.env.TWILIO_ACCOUNT_ID = '123' + process.env.TWILIO_AUTH_TOKEN = 'abc' + process.env.TWILIO_SENDER_NUMBER = '+1231231234' + + const mockFetch = vi.fn(async () => { + return Promise.resolve({ ok: true }) + }) as Mock + global.fetch = mockFetch + + await insertUsers( + db, + false, + ) + + await db.prepare('update "user" set "smsPhoneNumber" = ?, "smsPhoneNumberVerified" = ?').run( + '+16471231234', + 1, + ) + + const reqBody = await prepareFollowUpBody(db) + + const res = await app.request( + `${routeConfig.IdentityRoute.ResendSmsMfa}`, + { + method: 'POST', + body: JSON.stringify(reqBody), + }, + mock(db), + ) + expect(res.status).toBe(200) + + const mfaCode = await mockedKV.get(`${adapterConfig.BaseKVKey.SmsMfaCode}-${reqBody.code}`) ?? '' + expect(mfaCode.length).toBe(8) + + const callArgs = mockFetch.mock.calls[0] as any[] + const body = (callArgs[1] as unknown as { body: any }).body + expect(callArgs[0]).toBe('https://api.twilio.com/2010-04-01/Accounts/123/Messages.json') + expect(body.get('To')).toBe('+16471231234') + expect(body.get('From')).toBe('+1231231234') + expect(body.get('Body')).toBe(`${localeConfig.smsMfaMsg.body.en}: ${mfaCode}`) + + process.env.SMS_MFA_IS_REQUIRED = false as unknown as string + process.env.TWILIO_ACCOUNT_ID = '' + process.env.TWILIO_AUTH_TOKEN = '' + process.env.TWILIO_SENDER_NUMBER = '' + global.fetch = fetchMock + }, + ) + + test( + 'should throw error if user has not setup sms', + async () => { + await insertUsers( + db, + false, + ) + await enrollSmsMfa(db) + await db.prepare('update "user" set "smsPhoneNumber" = ?').run('+16471231234') + + const body = await prepareFollowUpBody(db) + + const res = await app.request( + `${routeConfig.IdentityRoute.ResendSmsMfa}`, + { + method: 'POST', + body: JSON.stringify(body), + }, + mock(db), + ) + expect(res.status).toBe(400) + }, + ) + + test( + 'should throw error if code is wrong', + async () => { + process.env.SMS_MFA_IS_REQUIRED = true as unknown as string + process.env.TWILIO_ACCOUNT_ID = '123' + process.env.TWILIO_AUTH_TOKEN = 'abc' + process.env.TWILIO_SENDER_NUMBER = '+1231231234' + + await insertUsers( + db, + false, + ) + await enrollSmsMfa(db) + + const reqBody = await prepareFollowUpBody(db) + const res = await app.request( + `${routeConfig.IdentityRoute.ResendSmsMfa}`, + { + method: 'POST', + body: JSON.stringify({ + ...reqBody, + code: 'abc', + phoneNumber: '+6471111111', + }), + }, + mock(db), + ) + expect(res.status).toBe(400) + expect(await res.text()).toBe(localeConfig.Error.WrongAuthCode) + + process.env.SMS_MFA_IS_REQUIRED = false as unknown as string + process.env.TWILIO_ACCOUNT_ID = '' + process.env.TWILIO_AUTH_TOKEN = '' + process.env.TWILIO_SENDER_NUMBER = '' + }, + ) + }, +) + describe( 'post /authorize-sms-mfa', () => { @@ -1561,6 +1736,165 @@ describe( }, ) + test( + 'should throw error if not enrolled with email mfa and fallback is not allowed', + async () => { + process.env.ALLOW_EMAIL_MFA_AS_BACKUP = false as unknown as string + + await insertUsers( + db, + false, + ) + await enrollOtpMfa(db) + const params = await prepareFollowUpParams(db) + + const res = await app.request( + `${routeConfig.IdentityRoute.AuthorizeEmailMfa}${params}`, + {}, + mock(db), + ) + expect(res.status).toBe(400) + + const code = getCodeFromParams(params) + const mfaCode = await mockedKV.get(`${adapterConfig.BaseKVKey.EmailMfaCode}-${code}`) + expect(mfaCode).toBeFalsy() + + const postRes = await app.request( + routeConfig.IdentityRoute.AuthorizeEmailMfa, + { + method: 'POST', + body: JSON.stringify({ + code, + locale: 'en', + mfaCode: 'abc', + }), + }, + mock(db), + ) + expect(postRes.status).toBe(401) + + process.env.ALLOW_EMAIL_MFA_AS_BACKUP = true as unknown as string + }, + ) + + test( + 'could use as otp mfa fallback', + async () => { + const mockFetch = vi.fn(async () => { + return Promise.resolve({ ok: true }) + }) + global.fetch = mockFetch as Mock + + await insertUsers( + db, + false, + ) + await enrollOtpMfa(db) + const params = await prepareFollowUpParams(db) + + await app.request( + `${routeConfig.IdentityRoute.AuthorizeEmailMfa}${params}`, + {}, + mock(db), + ) + const code = getCodeFromParams(params) + + const mfaCode = await mockedKV.get(`${adapterConfig.BaseKVKey.EmailMfaCode}-${code}`) + expect(mfaCode?.length).toBe(8) + expect(mockFetch).toBeCalledTimes(1) + + global.fetch = fetchMock + + const res = await app.request( + routeConfig.IdentityRoute.AuthorizeEmailMfa, + { + method: 'POST', + body: JSON.stringify({ + code, + locale: 'en', + mfaCode: await mockedKV.get(`${adapterConfig.BaseKVKey.EmailMfaCode}-${code}`), + }), + }, + mock(db), + ) + const json = await res.json() as { code: string } + expect(json).toStrictEqual({ + code: expect.any(String), + redirectUri: 'http://localhost:3000/en/dashboard', + state: '123', + scopes: ['profile', 'openid', 'offline_access'], + requireConsent: false, + requireMfaEnroll: false, + requireEmailMfa: false, + requireOtpSetup: false, + requireOtpMfa: false, + requireSmsMfa: false, + }) + expect(await mockedKV.get(`${adapterConfig.BaseKVKey.OtpMfaCode}-${json.code}`)).toBe('1') + }, + ) + + test( + 'could use as sms mfa fallback', + async () => { + const mockFetch = vi.fn(async () => { + return Promise.resolve({ ok: true }) + }) + global.fetch = mockFetch as Mock + + await insertUsers( + db, + false, + ) + await db.prepare('update "user" set "smsPhoneNumber" = ?, "smsPhoneNumberVerified" = ?').run( + '+16471231234', + 1, + ) + await enrollSmsMfa(db) + const params = await prepareFollowUpParams(db) + + await app.request( + `${routeConfig.IdentityRoute.AuthorizeEmailMfa}${params}`, + {}, + mock(db), + ) + const code = getCodeFromParams(params) + + const mfaCode = await mockedKV.get(`${adapterConfig.BaseKVKey.EmailMfaCode}-${code}`) + expect(mfaCode?.length).toBe(8) + expect(mockFetch).toBeCalledTimes(1) + + global.fetch = fetchMock + + const res = await app.request( + routeConfig.IdentityRoute.AuthorizeEmailMfa, + { + method: 'POST', + body: JSON.stringify({ + code, + locale: 'en', + mfaCode: await mockedKV.get(`${adapterConfig.BaseKVKey.EmailMfaCode}-${code}`), + }), + }, + mock(db), + ) + const json = await res.json() as { code: string } + expect(json).toStrictEqual({ + code: expect.any(String), + redirectUri: 'http://localhost:3000/en/dashboard', + state: '123', + scopes: ['profile', 'openid', 'offline_access'], + requireConsent: false, + requireMfaEnroll: false, + requireEmailMfa: false, + requireOtpSetup: false, + requireOtpMfa: false, + requireSmsMfa: false, + }) + expect(await mockedKV.get(`${adapterConfig.BaseKVKey.SmsMfaCode}-${json.code}`)).toBe('1') + }, + ) + test( 'should throw error if auth code is wrong', async () => { diff --git a/server/src/configs/locale.ts b/server/src/configs/locale.ts index b347a5c..1b1b938 100644 --- a/server/src/configs/locale.ts +++ b/server/src/configs/locale.ts @@ -292,6 +292,18 @@ export const authorizeSmsMfa = Object.freeze({ en: 'Verify', fr: 'Vérifier', }, + resend: { + en: 'Resend a new code', + fr: 'Renvoyer un nouveau code', + }, + resent: { + en: 'New code sent.', + fr: 'Nouveau code envoyé.', + }, + switchToEmail: { + en: 'Receive MFA Code by Email', + fr: 'Recevoir le code MFA par e-mail', + }, }) export const authorizeEmailMfa = Object.freeze({ diff --git a/server/src/configs/route.ts b/server/src/configs/route.ts index 841f3ac..ae44a47 100644 --- a/server/src/configs/route.ts +++ b/server/src/configs/route.ts @@ -22,6 +22,7 @@ export enum IdentityRoute { AuthorizeOtpSetup = `${InternalRoute.Identity}/authorize-otp-setup`, AuthorizeOtpMfa = `${InternalRoute.Identity}/authorize-otp-mfa`, AuthorizeSmsMfa = `${InternalRoute.Identity}/authorize-sms-mfa`, + ResendSmsMfa = `${InternalRoute.Identity}/resend-sms-mfa`, SetupSmsMfa = `${InternalRoute.Identity}/setup-sms-mfa`, AuthorizeEmailMfa = `${InternalRoute.Identity}/authorize-email-mfa`, ResendEmailMfa = `${InternalRoute.Identity}/resend-email-mfa`, diff --git a/server/src/handlers/identity/mfa.tsx b/server/src/handlers/identity/mfa.tsx index dfba5ab..d2ac693 100644 --- a/server/src/handlers/identity/mfa.tsx +++ b/server/src/handlers/identity/mfa.tsx @@ -51,12 +51,16 @@ const handleSendEmailMfa = async ( if (!authCodeBody) return false const requireEmailMfa = enableEmailMfa || authCodeBody.user.mfaTypes.includes(userModel.MfaType.Email) - const couldFallback = allowOtpSwitchToEmailMfa( + const couldFallbackAsOtp = allowOtpSwitchToEmailMfa( + c, + authCodeBody, + ) + const couldFallbackAsSms = allowSmsSwitchToEmailMfa( c, authCodeBody, ) - if (!requireEmailMfa && !couldFallback) throw new errorConfig.Forbidden() + if (!requireEmailMfa && !couldFallbackAsOtp && !couldFallbackAsSms) throw new errorConfig.Forbidden() const mfaCode = await emailService.sendEmailMfa( c, @@ -75,6 +79,23 @@ const handleSendEmailMfa = async ( return true } +const allowSmsSwitchToEmailMfa = ( + c: Context, + authCodeStore: AuthCodeBody, +) => { + if (!authCodeStore.user.smsPhoneNumber || !authCodeStore.user.smsPhoneNumberVerified) return false + + const { + SMS_MFA_IS_REQUIRED: enableSmsMfa, + EMAIL_MFA_IS_REQUIRED: enableEmailMfa, + ALLOW_EMAIL_MFA_AS_BACKUP: allowFallback, + } = env(c) + const notEnrolledEmail = !enableEmailMfa && !authCodeStore.user.mfaTypes.includes(userModel.MfaType.Email) + const enrolledSms = enableSmsMfa || authCodeStore.user.mfaTypes.includes(userModel.MfaType.Sms) + + return allowFallback && notEnrolledEmail && enrolledSms +} + const handleSendSmsMfa = async ( c: Context, phoneNumber: string, @@ -321,11 +342,17 @@ export const getAuthorizeSmsMfa = async (c: Context) => { ? '*'.repeat(phoneNumber.length - 4) + phoneNumber.slice(-4) : null + const allowSwitch = allowSmsSwitchToEmailMfa( + c, + authCodeBody, + ) + return c.html() } @@ -407,6 +434,34 @@ export const postSetupSmsMfa = async (c: Context) => { return c.json({ success: true }) } +export const resendSmsMfa = async (c: Context) => { + const reqBody = await c.req.json() + + const { SUPPORTED_LOCALES: locales } = env(c) + + const bodyDto = new identityDto.PostAuthorizeFollowUpReqDto(reqBody) + await validateUtil.dto(bodyDto) + + const authCodeBody = await kvService.getAuthCodeBody( + c.env.KV, + bodyDto.code, + ) + if (!authCodeBody) throw new errorConfig.Forbidden(localeConfig.Error.WrongAuthCode) + + if (!authCodeBody.user.smsPhoneNumber) throw new errorConfig.Forbidden() + + const smsRes = await handleSendSmsMfa( + c, + authCodeBody.user.smsPhoneNumber, + bodyDto.code, + authCodeBody, + bodyDto.locale || locales[0], + ) + if (!smsRes) throw new errorConfig.Forbidden() + + return c.json({ success: true }) +} + export const getAuthorizeEmailMfa = async (c: Context) => { const queryDto = await identityDto.parseGetAuthorizeFollowUpReq(c) await validateUtil.dto(queryDto) @@ -445,7 +500,12 @@ export const postAuthorizeEmailMfa = async (c: Context) => { ) if (!authCodeStore) throw new errorConfig.Forbidden(localeConfig.Error.WrongAuthCode) - const isFallback = allowOtpSwitchToEmailMfa( + const isOtpFallback = allowOtpSwitchToEmailMfa( + c, + authCodeStore, + ) + + const isSmsFallback = allowSmsSwitchToEmailMfa( c, authCodeStore, ) @@ -455,7 +515,8 @@ export const postAuthorizeEmailMfa = async (c: Context) => { bodyDto.code, bodyDto.mfaCode, expiresIn, - isFallback, + isOtpFallback, + isSmsFallback, ) if (!isValid) throw new errorConfig.UnAuthorized(localeConfig.Error.WrongMfaCode) diff --git a/server/src/routes/identity.tsx b/server/src/routes/identity.tsx index 81881a6..adab3b2 100644 --- a/server/src/routes/identity.tsx +++ b/server/src/routes/identity.tsx @@ -74,6 +74,12 @@ identityRoutes.get( identityHandler.getAuthorizeSmsMfa, ) +identityRoutes.post( + routeConfig.IdentityRoute.SetupSmsMfa, + setupMiddleware.validOrigin, + identityHandler.postSetupSmsMfa, +) + identityRoutes.post( routeConfig.IdentityRoute.AuthorizeSmsMfa, setupMiddleware.validOrigin, @@ -81,9 +87,9 @@ identityRoutes.post( ) identityRoutes.post( - routeConfig.IdentityRoute.SetupSmsMfa, + routeConfig.IdentityRoute.ResendSmsMfa, setupMiddleware.validOrigin, - identityHandler.postSetupSmsMfa, + identityHandler.resendSmsMfa, ) identityRoutes.get( diff --git a/server/src/services/kv.ts b/server/src/services/kv.ts index 7d7cd79..099caf6 100644 --- a/server/src/services/kv.ts +++ b/server/src/services/kv.ts @@ -132,7 +132,8 @@ export const stampEmailMfaCode = async ( authCode: string, mfaCode: string, expiresIn: number, - isFallbackOfOtp: boolean, + isOtpFallback: boolean, + isSmsFallback: boolean, ) => { const key = adapterConfig.getKVKey( adapterConfig.BaseKVKey.EmailMfaCode, @@ -143,12 +144,19 @@ export const stampEmailMfaCode = async ( const isValid = storedCode && storedCode === mfaCode if (isValid) { - const stampKey = isFallbackOfOtp - ? adapterConfig.getKVKey( + let stampKey = key + if (isOtpFallback) { + stampKey = adapterConfig.getKVKey( adapterConfig.BaseKVKey.OtpMfaCode, authCode, ) - : key + } else if (isSmsFallback) { + stampKey = adapterConfig.getKVKey( + adapterConfig.BaseKVKey.SmsMfaCode, + authCode, + ) + } + await kv.put( stampKey, '1', diff --git a/server/src/views/AuthorizeOtpMfa.tsx b/server/src/views/AuthorizeOtpMfa.tsx index c2462b5..e6965e3 100644 --- a/server/src/views/AuthorizeOtpMfa.tsx +++ b/server/src/views/AuthorizeOtpMfa.tsx @@ -49,7 +49,6 @@ const AuthorizeOtpMfa = ({ /> {showEmailMfaBtn && ( + {showEmailMfaBtn && ( + + )} ${resetErrorScript.resetCodeError()} ${resetErrorScript.resetPhoneNumberError()} + function switchToEmail() { + var queryString = "?code=${queryDto.code}&locale=${queryDto.locale}"; + var url = "${routeConfig.IdentityRoute.AuthorizeEmailMfa}" + queryString + window.location.href = url; + } + function resendCode() { + var containNumber = "${phoneNumber}"; + if (containNumber) { + fetch('${routeConfig.IdentityRoute.ResendSmsMfa}', { + method: 'POST', + headers: { + 'Accept': 'application/json', + 'Content-Type': 'application/json' + }, + body: JSON.stringify({ + locale: "${queryDto.locale}", + code: "${queryDto.code}", + }) + }) + .then((response) => { + if (response.ok) { + var resendBtn = document.getElementById("resend-btn") + resendBtn.disabled = true; + resendBtn.innerHTML = "${localeConfig.authorizeSmsMfa.resent[queryDto.locale]}" + } else { + return response.text().then(text => { + throw new Error(text); + }); + } + }) + .catch((error) => { + ${responseScript.handleSubmitError(queryDto.locale)} + }); + } else { + fetch('${routeConfig.IdentityRoute.SetupSmsMfa}', { + method: 'POST', + headers: { + 'Accept': 'application/json', + 'Content-Type': 'application/json' + }, + body: JSON.stringify({ + code: "${queryDto.code}", + locale: "${queryDto.locale}", + phoneNumber: document.getElementById('form-phoneNumber').value, + }) + }) + .then((response) => { + if (response.ok) { + var resendBtn = document.getElementById("resend-btn") + resendBtn.disabled = true; + resendBtn.innerHTML = "${localeConfig.authorizeSmsMfa.resent[queryDto.locale]}" + } else { + return response.text().then(text => { + throw new Error(text); + }); + } + }) + .catch((error) => { + ${responseScript.handleSubmitError(queryDto.locale)} + }); + } + } function handleSubmit(e) { e.preventDefault(); var containNumber = document.getElementById('form-phoneNumber').disabled @@ -85,12 +166,12 @@ const AuthorizeSmsMfa = ({ }) }) .then((response) => { - ${responseScript.parseRes()} - }) - .then((data) => { - document.getElementById('form-phoneNumber').disabled = true; - document.getElementById('code-row').classList.remove('hidden'); - document.getElementById('submit-button').innerHTML = "${localeConfig.authorizeSmsMfa.verify[queryDto.locale]}" + if (response.ok) { + document.getElementById('form-phoneNumber').disabled = true; + document.getElementById('code-row').classList.remove('hidden'); + document.getElementById('resend-btn').classList.remove('hidden'); + document.getElementById('submit-button').innerHTML = "${localeConfig.authorizeSmsMfa.verify[queryDto.locale]}" + } }) .catch((error) => { ${responseScript.handleSubmitError(queryDto.locale)}