From 21e649af81cf62215c59117d07e2513ae0c5cf25 Mon Sep 17 00:00:00 2001 From: Charles Zhao Date: Fri, 11 Nov 2022 19:12:18 +0800 Subject: [PATCH] refactor(core): change session health validation method for session APIs --- packages/core/src/lib/session.ts | 43 +++++++------------ .../core/src/routes/session/continue.test.ts | 12 ++---- .../core/src/routes/session/password.test.ts | 12 ++---- .../src/routes/session/passwordless.test.ts | 24 ++++------- .../core/src/routes/session/social.test.ts | 16 ++----- .../core/src/routes/session/utils.test.ts | 3 +- 6 files changed, 37 insertions(+), 73 deletions(-) diff --git a/packages/core/src/lib/session.ts b/packages/core/src/lib/session.ts index 7d2508675..5a475c500 100644 --- a/packages/core/src/lib/session.ts +++ b/packages/core/src/lib/session.ts @@ -1,10 +1,10 @@ -import { conditional } from '@silverhand/essentials'; import { getUnixTime } from 'date-fns'; import type { Context } from 'koa'; import type { InteractionResults, Provider } from 'oidc-provider'; import RequestError from '@/errors/RequestError'; import { findUserById, updateUserById } from '@/queries/user'; +import assertThat from '@/utils/assert-that'; export const assignInteractionResults = async ( ctx: Context, @@ -17,32 +17,20 @@ export const assignInteractionResults = async ( // have to do it manually // refer to: https://github.com/panva/node-oidc-provider/blob/c243bf6b6663c41ff3e75c09b95fb978eba87381/lib/actions/authorization/interactions.js#L106 const details = merge ? await provider.interactionDetails(ctx.req, ctx.res) : undefined; - const ts = getUnixTime(new Date()); - const mergedResult = { - // Merge with current result - ...details?.result, - ...result, - }; + const redirectTo = await provider.interactionResult( ctx.req, ctx.res, { - ...mergedResult, - ...conditional( - mergedResult.login && { - login: { - ...mergedResult.login, - // Update ts(timestamp) if the accountId has been set in result - ts: result.login?.accountId ? ts : mergedResult.login.ts, - }, - } - ), + // Merge with current result + ...details?.result, + ...result, }, { mergeWithLastSubmission: merge, } ); - ctx.body = { redirectTo, ts }; + ctx.body = { redirectTo }; }; export const checkSessionHealth = async ( @@ -50,23 +38,24 @@ export const checkSessionHealth = async ( provider: Provider, tolerance = 10 * 60 // 10 mins ) => { - const { result } = await provider.interactionDetails(ctx.req, ctx.res); + const { accountId, loginTs } = await provider.Session.get(ctx); - if (!result?.login?.accountId) { - throw new RequestError('auth.unauthorized'); - } + assertThat( + accountId, + new RequestError({ code: 'auth.unauthorized', id: accountId, status: 401 }) + ); - if (!result.login.ts || result.login.ts < getUnixTime(new Date()) - tolerance) { - const { passwordEncrypted, primaryPhone, primaryEmail } = await findUserById( - result.login.accountId - ); + if (!loginTs || loginTs < getUnixTime(new Date()) - tolerance) { + const { passwordEncrypted, primaryPhone, primaryEmail } = await findUserById(accountId); // No authenticated method configured for this user. Pass! if (!passwordEncrypted && !primaryPhone && !primaryEmail) { return; } - throw new RequestError('auth.require_re_authentication'); + throw new RequestError({ code: 'auth.require_re_authentication', status: 422 }); } + + return accountId; }; export const saveUserFirstConsentedAppId = async (userId: string, applicationId: string) => { diff --git a/packages/core/src/routes/session/continue.test.ts b/packages/core/src/routes/session/continue.test.ts index 05ae2f17b..76d4456fe 100644 --- a/packages/core/src/routes/session/continue.test.ts +++ b/packages/core/src/routes/session/continue.test.ts @@ -89,8 +89,7 @@ describe('session -> continueRoutes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: mockUser.id } }), expect.anything() ); }); @@ -122,8 +121,7 @@ describe('session -> continueRoutes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: mockUser.id } }), expect.anything() ); }); @@ -158,8 +156,7 @@ describe('session -> continueRoutes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: mockUser.id } }), expect.anything() ); }); @@ -191,8 +188,7 @@ describe('session -> continueRoutes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: mockUser.id } }), expect.anything() ); }); diff --git a/packages/core/src/routes/session/password.test.ts b/packages/core/src/routes/session/password.test.ts index 0eaf50aae..18b3010fa 100644 --- a/packages/core/src/routes/session/password.test.ts +++ b/packages/core/src/routes/session/password.test.ts @@ -111,8 +111,7 @@ describe('session -> password routes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: mockUser.id } }), expect.anything() ); }); @@ -128,8 +127,7 @@ describe('session -> password routes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: mockUser.id } }), expect.anything() ); }); @@ -145,8 +143,7 @@ describe('session -> password routes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: mockUser.id } }), expect.anything() ); }); @@ -175,8 +172,7 @@ describe('session -> password routes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: 'user1', ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: 'user1' } }), expect.anything() ); jest.useRealTimers(); diff --git a/packages/core/src/routes/session/passwordless.test.ts b/packages/core/src/routes/session/passwordless.test.ts index 3fdfaa0a3..34882ddcb 100644 --- a/packages/core/src/routes/session/passwordless.test.ts +++ b/packages/core/src/routes/session/passwordless.test.ts @@ -396,8 +396,7 @@ describe('session -> passwordlessRoutes', () => { expect.anything(), expect.anything(), expect.objectContaining({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - login: { accountId: mockUser.id, ts: expect.any(Number) }, + login: { accountId: mockUser.id }, }), expect.anything() ); @@ -419,8 +418,7 @@ describe('session -> passwordlessRoutes', () => { expect.anything(), expect.anything(), expect.objectContaining({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - login: { accountId: mockUser.id, ts: expect.any(Number) }, + login: { accountId: mockUser.id }, }), expect.anything() ); @@ -562,8 +560,7 @@ describe('session -> passwordlessRoutes', () => { expect.anything(), expect.anything(), expect.objectContaining({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - login: { accountId: mockUser.id, ts: expect.any(Number) }, + login: { accountId: mockUser.id }, }), expect.anything() ); @@ -587,8 +584,7 @@ describe('session -> passwordlessRoutes', () => { expect.anything(), expect.anything(), expect.objectContaining({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - login: { accountId: mockUser.id, ts: expect.any(Number) }, + login: { accountId: mockUser.id }, }), expect.anything() ); @@ -698,8 +694,7 @@ describe('session -> passwordlessRoutes', () => { expect.anything(), expect.anything(), expect.objectContaining({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - login: { accountId: 'user1', ts: expect.any(Number) }, + login: { accountId: 'user1' }, }), expect.anything() ); @@ -721,8 +716,7 @@ describe('session -> passwordlessRoutes', () => { expect.anything(), expect.anything(), expect.objectContaining({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - login: { accountId: 'user1', ts: expect.any(Number) }, + login: { accountId: 'user1' }, }), expect.anything() ); @@ -828,8 +822,7 @@ describe('session -> passwordlessRoutes', () => { expect.anything(), expect.anything(), expect.objectContaining({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - login: { accountId: 'user1', ts: expect.any(Number) }, + login: { accountId: 'user1' }, }), expect.anything() ); @@ -851,8 +844,7 @@ describe('session -> passwordlessRoutes', () => { expect.anything(), expect.anything(), expect.objectContaining({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - login: { accountId: 'user1', ts: expect.any(Number) }, + login: { accountId: 'user1' }, }), expect.anything() ); diff --git a/packages/core/src/routes/session/social.test.ts b/packages/core/src/routes/session/social.test.ts index d67f2b3f9..e00a6a8cd 100644 --- a/packages/core/src/routes/session/social.test.ts +++ b/packages/core/src/routes/session/social.test.ts @@ -1,4 +1,3 @@ -/* eslint-disable max-lines */ import { ConnectorType } from '@logto/connector-kit'; import type { User } from '@logto/schemas'; import { SignUpIdentifier } from '@logto/schemas'; @@ -234,8 +233,7 @@ describe('session -> socialRoutes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: mockUser.id } }), expect.anything() ); }); @@ -318,8 +316,7 @@ describe('session -> socialRoutes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: 'user1', ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: 'user1' } }), expect.anything() ); }); @@ -353,8 +350,7 @@ describe('session -> socialRoutes', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: 'user1', ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: 'user1' } }), expect.anything() ); }); @@ -368,10 +364,7 @@ describe('session -> socialRoutes', () => { }); it('throw error if result parsing fails', async () => { - interactionDetails.mockResolvedValueOnce({ - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - result: { login: { accountId: mockUser.id, ts: expect.any(Number) } }, - }); + interactionDetails.mockResolvedValueOnce({ result: { login: { accountId: mockUser.id } } }); const response = await sessionRequest .post(`${registerRoute}`) .send({ connectorId: 'connectorId' }); @@ -443,4 +436,3 @@ describe('session -> socialRoutes', () => { }); }); }); -/* eslint-enable max-lines */ diff --git a/packages/core/src/routes/session/utils.test.ts b/packages/core/src/routes/session/utils.test.ts index 49d6e2255..3bceb3391 100644 --- a/packages/core/src/routes/session/utils.test.ts +++ b/packages/core/src/routes/session/utils.test.ts @@ -121,8 +121,7 @@ describe('signInWithPassword()', () => { expect(interactionResult).toHaveBeenCalledWith( expect.anything(), expect.anything(), - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - expect.objectContaining({ login: { accountId: mockUser.id, ts: expect.any(Number) } }), + expect.objectContaining({ login: { accountId: mockUser.id } }), expect.anything() ); });