From 93f4ae10ec69f731e26ef6d83854621727c4e92a Mon Sep 17 00:00:00 2001 From: Gao Sun Date: Sat, 17 Dec 2022 22:59:10 +0800 Subject: [PATCH] refactor: update comments and fix tests --- packages/core/src/middleware/koa-audit-log.ts | 61 +++++++++++++------ packages/core/src/oidc/init.ts | 4 ++ packages/core/src/routes/init.ts | 3 - .../actions/submit-interaction.test.ts | 4 +- .../core/src/routes/interaction/index.test.ts | 24 +++++--- packages/core/src/routes/interaction/index.ts | 22 ++++++- .../src/routes/interaction/types/index.ts | 5 +- .../identifier-payload-verification.test.ts | 13 ++-- .../identifier-payload-verification.ts | 4 +- ...ofile-verification-forgot-password.test.ts | 3 +- ...profile-verification-profile-exist.test.ts | 3 +- ...le-verification-profile-registered.test.ts | 3 +- ...-verification-protected-identifier.test.ts | 3 +- .../user-identity-verification.test.ts | 2 + packages/core/src/routes/types.ts | 12 ++-- packages/core/src/test-utils/koa-log.ts | 9 ++- .../src/tests/api/logs-legacy.test.ts | 4 +- packages/schemas/src/types/log/interaction.ts | 2 - 18 files changed, 120 insertions(+), 61 deletions(-) diff --git a/packages/core/src/middleware/koa-audit-log.ts b/packages/core/src/middleware/koa-audit-log.ts index c9319059f..50916395a 100644 --- a/packages/core/src/middleware/koa-audit-log.ts +++ b/packages/core/src/middleware/koa-audit-log.ts @@ -21,6 +21,15 @@ export class LogEntry { }; } + /** Update payload by spreading `data` first, then spreading `this.payload`. */ + prepend(data: Readonly) { + this.payload = { + ...removeUndefinedKeys(data), + ...this.payload, + }; + } + + /** Update payload by spreading `this.payload` first, then spreading `data`. */ append(data: Readonly) { this.payload = { ...this.payload, @@ -33,6 +42,7 @@ export type LogPayload = Partial & Record; export type LogContext = { createLog: (key: LogKey) => LogEntry; + prependAllLogEntries: (payload: LogPayload) => void; }; export type WithLogContext = ContextT & @@ -40,46 +50,54 @@ export type WithLogContext( - dumpLogContext?: (ctx: ContextT) => Promise> | Record -): MiddlewareType, ResponseBodyT> { +export default function koaAuditLog< + StateT, + ContextT extends IRouterParamContext, + ResponseBodyT +>(): MiddlewareType, ResponseBodyT> { return async (ctx, next) => { const entries: LogEntry[] = []; @@ -91,6 +109,12 @@ export default function koaAuditLog { + for (const entry of entries) { + entry.prepend(payload); + } + }; + try { await next(); } catch (error: unknown) { @@ -111,13 +135,12 @@ export default function koaAuditLog { return insertLog({ id: nanoid(), type: payload.key, - payload: { ...logContext, ...payload }, + payload: { ip, userAgent, ...payload }, }); }) ); diff --git a/packages/core/src/oidc/init.ts b/packages/core/src/oidc/init.ts index ed9967f7b..cf58f11ba 100644 --- a/packages/core/src/oidc/init.ts +++ b/packages/core/src/oidc/init.ts @@ -12,6 +12,7 @@ import snakecaseKeys from 'snakecase-keys'; import envSet from '#src/env-set/index.js'; import { addOidcEventListeners } from '#src/event-listeners/index.js'; +import koaAuditLog from '#src/middleware/koa-audit-log.js'; import postgresAdapter from '#src/oidc/adapter.js'; import { isOriginAllowed, validateCustomClientMetadata } from '#src/oidc/utils.js'; import { findApplicationById } from '#src/queries/application.js'; @@ -188,6 +189,9 @@ export default async function initOidc(app: Koa): Promise { addOidcEventListeners(oidc); + // Provide audit log context for event listeners + oidc.use(koaAuditLog()); + app.use(mount('/oidc', oidc.app)); return oidc; diff --git a/packages/core/src/routes/init.ts b/packages/core/src/routes/init.ts index 7fd54a489..b8a70d671 100644 --- a/packages/core/src/routes/init.ts +++ b/packages/core/src/routes/init.ts @@ -4,9 +4,7 @@ import mount from 'koa-mount'; import Router from 'koa-router'; import type { Provider } from 'oidc-provider'; -import { extractInteractionContext } from '#src/event-listeners/utils.js'; import koaAuditLogLegacy from '#src/middleware/koa-audit-log-legacy.js'; -import koaAuditLog from '#src/middleware/koa-audit-log.js'; import koaAuth from '../middleware/koa-auth.js'; import koaLogSessionLegacy from '../middleware/koa-log-session-legacy.js'; @@ -36,7 +34,6 @@ const createRouters = (provider: Provider) => { sessionRoutes(sessionRouter, provider); const interactionRouter: AnonymousRouter = new Router(); - interactionRouter.use(koaAuditLog(extractInteractionContext)); interactionRoutes(interactionRouter, provider); const managementRouter: AuthedRouter = new Router(); diff --git a/packages/core/src/routes/interaction/actions/submit-interaction.test.ts b/packages/core/src/routes/interaction/actions/submit-interaction.test.ts index 33884a4b2..e3994823e 100644 --- a/packages/core/src/routes/interaction/actions/submit-interaction.test.ts +++ b/packages/core/src/routes/interaction/actions/submit-interaction.test.ts @@ -1,6 +1,7 @@ import { Event } from '@logto/schemas'; import { mockEsm, pickDefault } from '@logto/shared/esm'; +import { createMockLogContext } from '#src/test-utils/koa-log.js'; import { createMockProvider } from '#src/test-utils/oidc-provider.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; @@ -51,10 +52,9 @@ jest.useFakeTimers().setSystemTime(now); describe('submit action', () => { const provider = createMockProvider(); - const log = jest.fn(); const ctx: InteractionContext = { ...createContextWithRouteParameters(), - log, + ...createMockLogContext(), interactionPayload: { event: Event.SignIn }, }; const profile = { diff --git a/packages/core/src/routes/interaction/index.test.ts b/packages/core/src/routes/interaction/index.test.ts index ca0e34c18..8df4aaa0e 100644 --- a/packages/core/src/routes/interaction/index.test.ts +++ b/packages/core/src/routes/interaction/index.test.ts @@ -5,6 +5,8 @@ import { mockEsm, mockEsmDefault, mockEsmWithActual, pickDefault } from '@logto/ import { mockSignInExperience } from '#src/__mocks__/sign-in-experience.js'; import RequestError from '#src/errors/RequestError/index.js'; +import type koaAuditLog from '#src/middleware/koa-audit-log.js'; +import { createMockLogContext } from '#src/test-utils/koa-log.js'; import { createMockProvider } from '#src/test-utils/oidc-provider.js'; import { createRequester } from '#src/utils/test-utils.js'; @@ -76,7 +78,17 @@ const { getInteractionStorage } = mockEsm('./utils/interaction.js', () => ({ getInteractionStorage: jest.fn(), })); -const log = jest.fn(); +const { createLog, prependAllLogEntries } = createMockLogContext(); +mockEsmDefault( + '#src/middleware/koa-audit-log.js', + // eslint-disable-next-line unicorn/consistent-function-scoping + (): typeof koaAuditLog => () => async (ctx, next) => { + ctx.createLog = createLog; + ctx.prependAllLogEntries = prependAllLogEntries; + + return next(); + } +); const koaInteractionBodyGuard = await pickDefault( import('./middleware/koa-interaction-body-guard.js') @@ -107,14 +119,6 @@ describe('session -> interactionRoutes', () => { provider: createMockProvider( jest.fn().mockResolvedValue({ params: {}, jti: 'jti', client_id: demoAppApplicationId }) ), - middlewares: [ - async (ctx, next) => { - ctx.addLogContext = jest.fn(); - ctx.log = log; - - return next(); - }, - ], }); afterEach(() => { @@ -244,7 +248,7 @@ describe('session -> interactionRoutes', () => { }; const response = await sessionRequest.post(path).send(body); - expect(sendPasscodeToIdentifier).toBeCalledWith(body, 'jti', log); + expect(sendPasscodeToIdentifier).toBeCalledWith(body, 'jti', createLog); expect(response.status).toEqual(204); }); }); diff --git a/packages/core/src/routes/interaction/index.ts b/packages/core/src/routes/interaction/index.ts index 5c7ef9bae..57c357704 100644 --- a/packages/core/src/routes/interaction/index.ts +++ b/packages/core/src/routes/interaction/index.ts @@ -1,9 +1,11 @@ import type { LogtoErrorCode } from '@logto/phrases'; import { Event } from '@logto/schemas'; +import { conditional } from '@silverhand/essentials'; import type { Provider } from 'oidc-provider'; import RequestError from '#src/errors/RequestError/index.js'; import { assignInteractionResults } from '#src/libraries/session.js'; +import koaAuditLog from '#src/middleware/koa-audit-log.js'; import koaGuard from '#src/middleware/koa-guard.js'; import assertThat from '#src/utils/assert-that.js'; @@ -28,6 +30,24 @@ export default function interactionRoutes( router: T, provider: Provider ) { + router.use(koaAuditLog(), async (ctx, next) => { + await next(); + + // Prepend interaction context to log entries + try { + const { + jti, + params: { client_id }, + } = await provider.interactionDetails(ctx.req, ctx.res); + ctx.prependAllLogEntries({ + sessionId: jti, + applicationId: conditional(typeof client_id === 'string' && client_id), + }); + } catch (error: unknown) { + console.error(`Failed to get oidc provider interaction details`, error); + } + }); + router.put( interactionPrefix, koaInteractionBodyGuard(), @@ -117,7 +137,7 @@ export default function interactionRoutes( async (ctx, next) => { // Check interaction session const { jti } = await provider.interactionDetails(ctx.req, ctx.res); - await sendPasscodeToIdentifier(ctx.guard.body, jti, ctx.log); + await sendPasscodeToIdentifier(ctx.guard.body, jti, ctx.createLog); ctx.status = 204; diff --git a/packages/core/src/routes/interaction/types/index.ts b/packages/core/src/routes/interaction/types/index.ts index 8ff0f49e8..f53c3139b 100644 --- a/packages/core/src/routes/interaction/types/index.ts +++ b/packages/core/src/routes/interaction/types/index.ts @@ -11,6 +11,7 @@ import type { IRouterParamContext } from 'koa-router'; import type { z } from 'zod'; import type { SocialUserInfo } from '#src/connectors/types.js'; +import type { WithLogContext } from '#src/middleware/koa-audit-log.js'; import type { WithGuardedIdentifierPayloadContext } from '../middleware/koa-interaction-body-guard.js'; import type { @@ -108,7 +109,9 @@ export type VerifiedInteractionResult = | VerifiedSignInInteractionResult | VerifiedForgotPasswordInteractionResult; -export type InteractionContext = WithGuardedIdentifierPayloadContext; +export type InteractionContext = WithGuardedIdentifierPayloadContext< + WithLogContext +>; export type UserIdentity = | { username: string } diff --git a/packages/core/src/routes/interaction/verifications/identifier-payload-verification.test.ts b/packages/core/src/routes/interaction/verifications/identifier-payload-verification.test.ts index d9a360cd2..97ef5094e 100644 --- a/packages/core/src/routes/interaction/verifications/identifier-payload-verification.test.ts +++ b/packages/core/src/routes/interaction/verifications/identifier-payload-verification.test.ts @@ -2,6 +2,7 @@ import { Event } from '@logto/schemas'; import { mockEsm, mockEsmDefault, mockEsmWithActual, pickDefault } from '@logto/shared/esm'; import RequestError from '#src/errors/RequestError/index.js'; +import { createMockLogContext } from '#src/test-utils/koa-log.js'; import { createMockProvider } from '#src/test-utils/oidc-provider.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; @@ -31,10 +32,10 @@ const identifierPayloadVerification = await pickDefault( import('./identifier-payload-verification.js') ); -const log = jest.fn(); +const logContext = createMockLogContext(); describe('identifier verification', () => { - const baseCtx = { ...createContextWithRouteParameters(), log }; + const baseCtx = { ...createContextWithRouteParameters(), ...logContext }; afterEach(() => { jest.clearAllMocks(); @@ -152,7 +153,7 @@ describe('identifier verification', () => { expect(verifyIdentifierByPasscode).toBeCalledWith( { ...identifier, event: Event.SignIn }, 'jti', - log + logContext.createLog ); expect(result).toEqual({ @@ -176,7 +177,7 @@ describe('identifier verification', () => { expect(verifyIdentifierByPasscode).toBeCalledWith( { ...identifier, event: Event.SignIn }, 'jti', - log + logContext.createLog ); expect(result).toEqual({ @@ -198,7 +199,7 @@ describe('identifier verification', () => { const result = await identifierPayloadVerification(ctx, createMockProvider()); - expect(verifySocialIdentity).toBeCalledWith(identifier, log); + expect(verifySocialIdentity).toBeCalledWith(identifier, logContext.createLog); expect(findUserByIdentifier).not.toBeCalled(); expect(result).toEqual({ @@ -323,7 +324,7 @@ describe('identifier verification', () => { expect(verifyIdentifierByPasscode).toBeCalledWith( { ...identifier, event: Event.SignIn }, 'jti', - log + logContext.createLog ); expect(result).toEqual({ diff --git a/packages/core/src/routes/interaction/verifications/identifier-payload-verification.ts b/packages/core/src/routes/interaction/verifications/identifier-payload-verification.ts index bbf6c7a13..1c8224778 100644 --- a/packages/core/src/routes/interaction/verifications/identifier-payload-verification.ts +++ b/packages/core/src/routes/interaction/verifications/identifier-payload-verification.ts @@ -46,7 +46,7 @@ const verifyPasscodeIdentifier = async ( ): Promise => { const { jti } = await provider.interactionDetails(ctx.req, ctx.res); - await verifyIdentifierByPasscode({ ...identifier, event }, jti, ctx.log); + await verifyIdentifierByPasscode({ ...identifier, event }, jti, ctx.createLog); return 'email' in identifier ? { key: 'emailVerified', value: identifier.email } @@ -57,7 +57,7 @@ const verifySocialIdentifier = async ( identifier: SocialConnectorPayload, ctx: InteractionContext ): Promise => { - const userInfo = await verifySocialIdentity(identifier, ctx.log); + const userInfo = await verifySocialIdentity(identifier, ctx.createLog); return { key: 'social', connectorId: identifier.connectorId, userInfo }; }; diff --git a/packages/core/src/routes/interaction/verifications/profile-verification-forgot-password.test.ts b/packages/core/src/routes/interaction/verifications/profile-verification-forgot-password.test.ts index 8647d8aff..f23fd90a7 100644 --- a/packages/core/src/routes/interaction/verifications/profile-verification-forgot-password.test.ts +++ b/packages/core/src/routes/interaction/verifications/profile-verification-forgot-password.test.ts @@ -2,6 +2,7 @@ import { Event } from '@logto/schemas'; import { mockEsm, mockEsmWithActual, pickDefault } from '@logto/shared/esm'; import RequestError from '#src/errors/RequestError/index.js'; +import { createMockLogContext } from '#src/test-utils/koa-log.js'; import { createMockProvider } from '#src/test-utils/oidc-provider.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; @@ -25,7 +26,7 @@ const verifyProfile = await pickDefault(import('./profile-verification.js')); describe('forgot password interaction profile verification', () => { const provider = createMockProvider(); - const baseCtx = createContextWithRouteParameters(); + const baseCtx = { ...createContextWithRouteParameters(), ...createMockLogContext() }; const interaction = { event: Event.ForgotPassword, diff --git a/packages/core/src/routes/interaction/verifications/profile-verification-profile-exist.test.ts b/packages/core/src/routes/interaction/verifications/profile-verification-profile-exist.test.ts index 992b361cb..afd947782 100644 --- a/packages/core/src/routes/interaction/verifications/profile-verification-profile-exist.test.ts +++ b/packages/core/src/routes/interaction/verifications/profile-verification-profile-exist.test.ts @@ -2,6 +2,7 @@ import { Event } from '@logto/schemas'; import { mockEsm, mockEsmWithActual, pickDefault } from '@logto/shared/esm'; import RequestError from '#src/errors/RequestError/index.js'; +import { createMockLogContext } from '#src/test-utils/koa-log.js'; import { createMockProvider } from '#src/test-utils/oidc-provider.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; @@ -32,7 +33,7 @@ const verifyProfile = await pickDefault(import('./profile-verification.js')); describe('Should throw when providing existing identifiers in profile', () => { const provider = createMockProvider(); - const baseCtx = createContextWithRouteParameters(); + const baseCtx = { ...createContextWithRouteParameters(), ...createMockLogContext() }; const identifiers: Identifier[] = [ { key: 'accountId', value: 'foo' }, { key: 'emailVerified', value: 'email' }, diff --git a/packages/core/src/routes/interaction/verifications/profile-verification-profile-registered.test.ts b/packages/core/src/routes/interaction/verifications/profile-verification-profile-registered.test.ts index 295af32bd..cc7b5aebd 100644 --- a/packages/core/src/routes/interaction/verifications/profile-verification-profile-registered.test.ts +++ b/packages/core/src/routes/interaction/verifications/profile-verification-profile-registered.test.ts @@ -2,6 +2,7 @@ import { Event } from '@logto/schemas'; import { mockEsm, mockEsmWithActual, pickDefault } from '@logto/shared/esm'; import RequestError from '#src/errors/RequestError/index.js'; +import { createMockLogContext } from '#src/test-utils/koa-log.js'; import { createMockProvider } from '#src/test-utils/oidc-provider.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; @@ -34,7 +35,7 @@ mockEsm('#src/connectors/index.js', () => ({ const verifyProfile = await pickDefault(import('./profile-verification.js')); -const baseCtx = createContextWithRouteParameters(); +const baseCtx = { ...createContextWithRouteParameters(), ...createMockLogContext() }; const identifiers: Identifier[] = [ { key: 'accountId', value: 'foo' }, { key: 'emailVerified', value: 'email@logto.io' }, diff --git a/packages/core/src/routes/interaction/verifications/profile-verification-protected-identifier.test.ts b/packages/core/src/routes/interaction/verifications/profile-verification-protected-identifier.test.ts index 571a94aad..790456d1f 100644 --- a/packages/core/src/routes/interaction/verifications/profile-verification-protected-identifier.test.ts +++ b/packages/core/src/routes/interaction/verifications/profile-verification-protected-identifier.test.ts @@ -2,6 +2,7 @@ import { Event } from '@logto/schemas'; import { mockEsm, mockEsmWithActual, pickDefault } from '@logto/shared/esm'; import RequestError from '#src/errors/RequestError/index.js'; +import { createMockLogContext } from '#src/test-utils/koa-log.js'; import { createMockProvider } from '#src/test-utils/oidc-provider.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; @@ -29,7 +30,7 @@ mockEsm('#src/connectors/index.js', () => ({ const verifyProfile = await pickDefault(import('./profile-verification.js')); describe('profile protected identifier verification', () => { - const baseCtx = createContextWithRouteParameters(); + const baseCtx = { ...createContextWithRouteParameters(), ...createMockLogContext() }; const interaction = { event: Event.SignIn, accountId: 'foo' }; const provider = createMockProvider(); diff --git a/packages/core/src/routes/interaction/verifications/user-identity-verification.test.ts b/packages/core/src/routes/interaction/verifications/user-identity-verification.test.ts index 5d63f98eb..dc6efd648 100644 --- a/packages/core/src/routes/interaction/verifications/user-identity-verification.test.ts +++ b/packages/core/src/routes/interaction/verifications/user-identity-verification.test.ts @@ -2,6 +2,7 @@ import { Event } from '@logto/schemas'; import { mockEsm, mockEsmDefault, mockEsmWithActual, pickDefault } from '@logto/shared/esm'; import RequestError from '#src/errors/RequestError/index.js'; +import { createMockLogContext } from '#src/test-utils/koa-log.js'; import { createMockProvider } from '#src/test-utils/oidc-provider.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; @@ -26,6 +27,7 @@ describe('userAccountVerification', () => { const ctx: InteractionContext = { ...createContextWithRouteParameters(), + ...createMockLogContext(), interactionPayload: { event: Event.SignIn, }, diff --git a/packages/core/src/routes/types.ts b/packages/core/src/routes/types.ts index 05be53ea6..70803b5ff 100644 --- a/packages/core/src/routes/types.ts +++ b/packages/core/src/routes/types.ts @@ -1,17 +1,17 @@ +import type { ExtendableContext } from 'koa'; import type Router from 'koa-router'; -import type { KoaContextWithOIDC } from 'oidc-provider'; import type { WithLogContextLegacy } from '#src/middleware/koa-audit-log-legacy.js'; import type { WithLogContext } from '#src/middleware/koa-audit-log.js'; import type { WithAuthContext } from '#src/middleware/koa-auth.js'; import type { WithI18nContext } from '#src/middleware/koa-i18next.js'; -export type AnonymousRouter = Router< - unknown, - WithLogContext & WithI18nContext & KoaContextWithOIDC ->; +export type AnonymousRouter = Router; /** @deprecated This will be removed soon. Use `kua-log-session.js` instead. */ export type AnonymousRouterLegacy = Router; -export type AuthedRouter = Router; +export type AuthedRouter = Router< + unknown, + WithAuthContext & WithLogContext & WithI18nContext & ExtendableContext +>; diff --git a/packages/core/src/test-utils/koa-log.ts b/packages/core/src/test-utils/koa-log.ts index 68970549b..5052a0f89 100644 --- a/packages/core/src/test-utils/koa-log.ts +++ b/packages/core/src/test-utils/koa-log.ts @@ -1,3 +1,4 @@ +import type { LogContext } from '#src/middleware/koa-audit-log.js'; import { LogEntry } from '#src/middleware/koa-audit-log.js'; const { jest } = import.meta; @@ -6,8 +7,12 @@ class MockLogEntry extends LogEntry { append = jest.fn(); } -export const createMockLogContext = () => { +export const createMockLogContext = (): LogContext & { mockAppend: jest.Mock } => { const mockLogEntry = new MockLogEntry('Unknown'); - return { createLog: jest.fn(() => mockLogEntry), mockAppend: mockLogEntry.append }; + return { + createLog: jest.fn(() => mockLogEntry), + prependAllLogEntries: jest.fn(), + mockAppend: mockLogEntry.append, + }; }; diff --git a/packages/integration-tests/src/tests/api/logs-legacy.test.ts b/packages/integration-tests/src/tests/api/logs-legacy.test.ts index 8c5407219..c970ad27b 100644 --- a/packages/integration-tests/src/tests/api/logs-legacy.test.ts +++ b/packages/integration-tests/src/tests/api/logs-legacy.test.ts @@ -20,9 +20,7 @@ describe('admin console logs (legacy)', () => { const logs = await getLogs(); const registerLog = logs.filter( - ({ type, payload }) => - type === 'RegisterUsernamePassword' && - (payload as Record).username === username + ({ type, payload }) => type === 'RegisterUsernamePassword' && payload.username === username ); expect(registerLog.length).toBeGreaterThan(0); diff --git a/packages/schemas/src/types/log/interaction.ts b/packages/schemas/src/types/log/interaction.ts index c76f7ebc7..bfcecb20e 100644 --- a/packages/schemas/src/types/log/interaction.ts +++ b/packages/schemas/src/types/log/interaction.ts @@ -1,7 +1,5 @@ import type { Event } from '../interactions.js'; -export { Event } from '../interactions.js'; - export type Prefix = 'Interaction'; export const prefix: Prefix = 'Interaction';