From f246aff81ee36967067e8408a0acbf7ccaf0d28c Mon Sep 17 00:00:00 2001 From: Baozier Date: Fri, 22 Nov 2024 22:08:45 -0500 Subject: [PATCH] Simplify authorize page redirecting conditions (#194) --- .../__tests__/normal/identity-main.test.tsx | 81 ++++--------------- .../__tests__/normal/identity-mfa.test.tsx | 66 +-------------- .../__tests__/normal/identity-policy.test.tsx | 18 +---- .../__tests__/normal/identity-social.test.tsx | 18 +---- server/src/handlers/identity/social.tsx | 2 +- server/src/utils/identity.ts | 77 +++++++++++++----- server/src/views/scripts/response.ts | 26 +----- 7 files changed, 82 insertions(+), 206 deletions(-) diff --git a/server/src/__tests__/normal/identity-main.test.tsx b/server/src/__tests__/normal/identity-main.test.tsx index 2f4d738..dc4ecf3 100644 --- a/server/src/__tests__/normal/identity-main.test.tsx +++ b/server/src/__tests__/normal/identity-main.test.tsx @@ -334,14 +334,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: false, - requireMfaEnroll: true, - requireEmailMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeMfaEnroll, }) const { code } = json as { code: string } const codeStore = JSON.parse(await mockedKV.get(`AC-${code}`) ?? '') @@ -674,6 +667,7 @@ describe( test( 'should get auth code after sign up', async () => { + process.env.ENABLE_USER_APP_CONSENT = false as unknown as string const mockFetch = vi.fn(async () => { return Promise.resolve({ ok: true }) }) @@ -686,14 +680,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: true, - requireMfaEnroll: true, - requireEmailMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeMfaEnroll, }) const appRecord = await getApp(db) const { code } = json as { code: string } @@ -715,6 +702,7 @@ describe( expect(body).toContain(`${routeConfig.IdentityRoute.VerifyEmail}?id=${codeStore.user.authId}&locale=en`) global.fetch = fetchMock + process.env.ENABLE_USER_APP_CONSENT = true as unknown as string }, ) @@ -743,6 +731,7 @@ describe( 'could force otp mfa', async () => { global.process.env.OTP_MFA_IS_REQUIRED = true as unknown as string + global.process.env.ENABLE_USER_APP_CONSENT = false as unknown as string const res = await postAuthorizeAccount() const json = await res.json() expect(json).toStrictEqual({ @@ -750,16 +739,10 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: true, - requireMfaEnroll: false, - requireEmailMfa: false, - requireOtpSetup: true, - requireOtpMfa: true, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeOtpSetup, }) global.process.env.OTP_MFA_IS_REQUIRED = false as unknown as string + global.process.env.ENABLE_USER_APP_CONSENT = true as unknown as string }, ) @@ -767,6 +750,7 @@ describe( 'could force email mfa', async () => { global.process.env.EMAIL_MFA_IS_REQUIRED = true as unknown as string + global.process.env.ENABLE_USER_APP_CONSENT = false as unknown as string const res = await postAuthorizeAccount() const json = await res.json() expect(json).toStrictEqual({ @@ -774,16 +758,10 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: true, - requireMfaEnroll: false, - requireEmailMfa: true, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeEmailMfa, }) global.process.env.EMAIL_MFA_IS_REQUIRED = false as unknown as string + global.process.env.ENABLE_USER_APP_CONSENT = true as unknown as string }, ) @@ -791,6 +769,7 @@ describe( 'could force sms mfa', async () => { global.process.env.SMS_MFA_IS_REQUIRED = true as unknown as string + global.process.env.ENABLE_USER_APP_CONSENT = false as unknown as string const res = await postAuthorizeAccount() const json = await res.json() expect(json).toStrictEqual({ @@ -798,16 +777,10 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: true, - requireMfaEnroll: false, - requireEmailMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: true, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeSmsMfa, }) global.process.env.SMS_MFA_IS_REQUIRED = false as unknown as string + global.process.env.ENABLE_USER_APP_CONSENT = true as unknown as string }, ) @@ -815,6 +788,7 @@ describe( 'could skip mfa', async () => { global.process.env.ENFORCE_ONE_MFA_ENROLLMENT = [] as unknown as string + global.process.env.ENABLE_USER_APP_CONSENT = false as unknown as string const res = await postAuthorizeAccount() const json = await res.json() expect(json).toStrictEqual({ @@ -822,16 +796,9 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: true, - requireMfaEnroll: false, - requireEmailMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, }) global.process.env.ENFORCE_ONE_MFA_ENROLLMENT = ['email', 'otp'] as unknown as string + global.process.env.ENABLE_USER_APP_CONSENT = true as unknown as string }, ) @@ -846,14 +813,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: false, - requireMfaEnroll: true, - requireEmailMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeMfaEnroll, }) global.process.env.ENABLE_USER_APP_CONSENT = true as unknown as string }, @@ -1014,14 +974,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: false, - requireMfaEnroll: true, - requireEmailMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeMfaEnroll, }) const consent = db.prepare('SELECT * from user_app_consent WHERE "userId" = 1 AND "appId" = 1').get() expect(consent).toBeTruthy() diff --git a/server/src/__tests__/normal/identity-mfa.test.tsx b/server/src/__tests__/normal/identity-mfa.test.tsx index f1c3671..f1edec8 100644 --- a/server/src/__tests__/normal/identity-mfa.test.tsx +++ b/server/src/__tests__/normal/identity-mfa.test.tsx @@ -225,14 +225,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: false, - requireMfaEnroll: false, - requireEmailMfa: true, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeEmailMfa, }) const user = await db.prepare('SELECT * from "user" WHERE id = 1').get() as userModel.Raw @@ -318,14 +311,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: false, - requireMfaEnroll: false, - requireEmailMfa: false, - requireOtpSetup: true, - requireOtpMfa: true, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeOtpSetup, }) const user = await db.prepare('SELECT * from "user" WHERE id = 1').get() as userModel.Raw @@ -542,14 +528,6 @@ describe( 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, - requireChangePassword: false, - requireChangeEmail: false, }) expect(await mockedKV.get(`${adapterConfig.BaseKVKey.OtpMfaCode}-${json.code}`)).toBe('1') }, @@ -1516,14 +1494,6 @@ describe( 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, - requireChangePassword: false, - requireChangeEmail: false, }) expect(await mockedKV.get(`${adapterConfig.BaseKVKey.SmsMfaCode}-${json.code}`)).toBe('1') @@ -1940,14 +1910,6 @@ describe( 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, - requireChangePassword: false, - requireChangeEmail: false, }) expect(await mockedKV.get(`${adapterConfig.BaseKVKey.EmailMfaCode}-${json.code}`)).toBe('1') }, @@ -2040,14 +2002,6 @@ describe( 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, - requireChangePassword: false, - requireChangeEmail: false, }) expect(await mockedKV.get(`${adapterConfig.BaseKVKey.OtpMfaCode}-${json.code}`)).toBe('1') }, @@ -2103,14 +2057,6 @@ describe( 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, - requireChangePassword: false, - requireChangeEmail: false, }) expect(await mockedKV.get(`${adapterConfig.BaseKVKey.SmsMfaCode}-${json.code}`)).toBe('1') }, @@ -2285,14 +2231,6 @@ describe( 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, - requireChangePassword: false, - requireChangeEmail: false, }) expect(await mockedKV.get(`${adapterConfig.BaseKVKey.EmailMfaCode}-${json.code}`)).toBe('1') }, diff --git a/server/src/__tests__/normal/identity-policy.test.tsx b/server/src/__tests__/normal/identity-policy.test.tsx index 0ea7895..7117a1c 100644 --- a/server/src/__tests__/normal/identity-policy.test.tsx +++ b/server/src/__tests__/normal/identity-policy.test.tsx @@ -143,14 +143,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: true, - requireMfaEnroll: true, - requireEmailMfa: false, - requireSmsMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeConsent, }) }, ) @@ -423,14 +416,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: true, - requireMfaEnroll: true, - requireEmailMfa: false, - requireSmsMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeConsent, }) }, ) diff --git a/server/src/__tests__/normal/identity-social.test.tsx b/server/src/__tests__/normal/identity-social.test.tsx index 967bd5a..ac07c31 100644 --- a/server/src/__tests__/normal/identity-social.test.tsx +++ b/server/src/__tests__/normal/identity-social.test.tsx @@ -86,14 +86,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: true, - requireMfaEnroll: false, - requireEmailMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeConsent, }) } @@ -277,14 +270,7 @@ describe( redirectUri: 'http://localhost:3000/en/dashboard', state: '123', scopes: ['profile', 'openid', 'offline_access'], - requireConsent: true, - requireMfaEnroll: false, - requireEmailMfa: false, - requireOtpSetup: false, - requireOtpMfa: false, - requireSmsMfa: false, - requireChangePassword: false, - requireChangeEmail: false, + nextPage: routeConfig.IdentityRoute.AuthorizeConsent, }) } diff --git a/server/src/handlers/identity/social.tsx b/server/src/handlers/identity/social.tsx index 70b6ccc..29d317f 100644 --- a/server/src/handlers/identity/social.tsx +++ b/server/src/handlers/identity/social.tsx @@ -190,7 +190,7 @@ export const getAuthorizeGithub = async (c: Context) => { ) const qs = `?state=${detail.state}&code=${detail.code}&locale=${bodyDto.locale}` - const url = detail.requireConsent + const url = detail.nextPage === routeConfig.IdentityRoute.AuthorizeConsent ? `${routeConfig.IdentityRoute.AuthorizeConsent}${qs}&redirect_uri=${detail.redirectUri}` : `${detail.redirectUri}${qs}` return c.redirect(url) diff --git a/server/src/utils/identity.ts b/server/src/utils/identity.ts index 089c9a2..390b18b 100644 --- a/server/src/utils/identity.ts +++ b/server/src/utils/identity.ts @@ -7,6 +7,7 @@ import { import { AuthCodeBody } from 'configs/type' import { userModel } from 'models' import { Policy } from 'dtos/oauth' +import { IdentityRoute } from 'configs/route' export enum AuthorizeStep { Account = 0, @@ -27,11 +28,23 @@ export const processPostAuthorize = async ( authCode: string, authCodeBody: AuthCodeBody, ) => { + const basicInfo = { + code: authCode, + redirectUri: authCodeBody.request.redirectUri, + state: authCodeBody.request.state, + scopes: authCodeBody.request.scopes, + } + const requireConsent = step < 1 && await consentService.shouldCollectConsent( c, authCodeBody.user.id, authCodeBody.appId, ) + if (requireConsent) { + return { + ...basicInfo, nextPage: IdentityRoute.AuthorizeConsent, + } + } const isSocialLogin = !!authCodeBody.user.socialAccountId @@ -52,61 +65,83 @@ export const processPostAuthorize = async ( !enableOtpMfa && !enableSmsMfa && !authCodeBody.user.mfaTypes.length + if (requireMfaEnroll) { + return { + ...basicInfo, nextPage: IdentityRoute.AuthorizeMfaEnroll, + } + } const requireOtpMfa = step < 3 && !isSocialLogin && (enableOtpMfa || authCodeBody.user.mfaTypes.includes(userModel.MfaType.Otp)) const requireOtpSetup = requireOtpMfa && !authCodeBody.user.otpVerified + if (requireOtpSetup) { + return { + ...basicInfo, nextPage: IdentityRoute.AuthorizeOtpSetup, + } + } + if (requireOtpMfa) { + return { + ...basicInfo, nextPage: IdentityRoute.AuthorizeOtpMfa, + } + } const requireSmsMfa = step < 4 && !isSocialLogin && (enableSmsMfa || authCodeBody.user.mfaTypes.includes(userModel.MfaType.Sms)) + if (requireSmsMfa) { + return { + ...basicInfo, nextPage: IdentityRoute.AuthorizeSmsMfa, + } + } const requireEmailMfa = step < 5 && !isSocialLogin && (enableEmailMfa || authCodeBody.user.mfaTypes.includes(userModel.MfaType.Email)) + if (requireEmailMfa) { + return { + ...basicInfo, nextPage: IdentityRoute.AuthorizeEmailMfa, + } + } const requireChangePassword = step < 6 && !isSocialLogin && enablePasswordReset && authCodeBody.request.policy === Policy.ChangePassword + if (requireChangePassword) { + return { + ...basicInfo, nextPage: IdentityRoute.ChangePassword, + } + } const requireChangeEmail = step < 6 && !isSocialLogin && enableEmailVerification && authCodeBody.request.policy === Policy.ChangeEmail - - if ( - !requireChangePassword && !requireChangeEmail && - !requireConsent && !requireMfaEnroll && - !requireOtpMfa && !requireEmailMfa && !requireSmsMfa - ) { - sessionService.setAuthInfoSession( - c, - authCodeBody.appId, - authCodeBody.appName, - authCodeBody.user, - authCodeBody.request, - ) + if (requireChangeEmail) { + return { + ...basicInfo, nextPage: IdentityRoute.ChangeEmail, + } } + sessionService.setAuthInfoSession( + c, + authCodeBody.appId, + authCodeBody.appName, + authCodeBody.user, + authCodeBody.request, + ) + return { code: authCode, redirectUri: authCodeBody.request.redirectUri, state: authCodeBody.request.state, scopes: authCodeBody.request.scopes, - requireConsent, - requireMfaEnroll, - requireEmailMfa, - requireSmsMfa, - requireOtpSetup, - requireOtpMfa, - requireChangePassword, - requireChangeEmail, + nextPage: undefined, } } diff --git a/server/src/views/scripts/response.ts b/server/src/views/scripts/response.ts index c08e615..2d9aa3b 100644 --- a/server/src/views/scripts/response.ts +++ b/server/src/views/scripts/response.ts @@ -15,30 +15,8 @@ export const parseRes = () => html` export const handleAuthorizeFormRedirect = (locale: typeConfig.Locale) => html` var queryString = "?state=" + data.state + "&code=" + data.code + "&locale=" + "${locale}"; - var innerQueryString = queryString += "&redirect_uri=" + data.redirectUri; - if (data.requireConsent) { - var url = "${routeConfig.IdentityRoute.AuthorizeConsent}" + innerQueryString - window.location.href = url; - } else if (data.requireMfaEnroll) { - var url = "${routeConfig.IdentityRoute.AuthorizeMfaEnroll}" + innerQueryString - window.location.href = url; - } else if (data.requireOtpSetup) { - var url = "${routeConfig.IdentityRoute.AuthorizeOtpSetup}" + innerQueryString - window.location.href = url; - } else if (data.requireOtpMfa) { - var url = "${routeConfig.IdentityRoute.AuthorizeOtpMfa}" + innerQueryString - window.location.href = url; - } else if (data.requireSmsMfa) { - var url = "${routeConfig.IdentityRoute.AuthorizeSmsMfa}" + innerQueryString - window.location.href = url; - } else if (data.requireEmailMfa) { - var url = "${routeConfig.IdentityRoute.AuthorizeEmailMfa}" + innerQueryString - window.location.href = url; - } else if (data.requireChangePassword) { - var url = "${routeConfig.IdentityRoute.ChangePassword}" + innerQueryString - window.location.href = url; - } else if (data.requireChangeEmail) { - var url = "${routeConfig.IdentityRoute.ChangeEmail}" + innerQueryString + if (data.nextPage) { + var url = data.nextPage + queryString + "&redirect_uri=" + data.redirectUri window.location.href = url; } else { var url = data.redirectUri + queryString;