From 813e216398768738de6521ec709cdcd1b96aaeb7 Mon Sep 17 00:00:00 2001 From: Xiao Yijun Date: Thu, 1 Jun 2023 12:18:49 +0800 Subject: [PATCH] fix(core): trigger reset password hook (#3916) --- .changeset/silly-impalas-pump.md | 5 ++ .../core/src/libraries/hook/index.test.ts | 22 ++----- packages/core/src/libraries/hook/index.ts | 44 ++++++++----- .../actions/submit-interaction.test.ts | 1 + .../interaction/actions/submit-interaction.ts | 8 ++- .../core/src/routes/interaction/consent.ts | 3 +- packages/core/src/routes/interaction/index.ts | 2 +- .../middleware/koa-interaction-details.ts | 15 +++-- .../middleware/koa-interaction-hooks.ts | 62 +++++++++++++++---- .../middleware/koa-interaction-sie.ts | 10 ++- .../mandatory-user-profile-validation.ts | 4 +- .../src/helpers/interactions.ts | 23 ++++++- .../src/tests/api/hook/hook.trigger.test.ts | 56 ++++++++++++++++- 13 files changed, 188 insertions(+), 67 deletions(-) create mode 100644 .changeset/silly-impalas-pump.md diff --git a/.changeset/silly-impalas-pump.md b/.changeset/silly-impalas-pump.md new file mode 100644 index 000000000..8022ca976 --- /dev/null +++ b/.changeset/silly-impalas-pump.md @@ -0,0 +1,5 @@ +--- +"@logto/core": patch +--- + +Bug fix: reset password webhook should be triggered when the user resets password diff --git a/packages/core/src/libraries/hook/index.test.ts b/packages/core/src/libraries/hook/index.test.ts index 977e69bf8..c6cc902f6 100644 --- a/packages/core/src/libraries/hook/index.test.ts +++ b/packages/core/src/libraries/hook/index.test.ts @@ -5,7 +5,6 @@ import { createMockUtils } from '@logto/shared/esm'; import { mockHook } from '#src/__mocks__/hook.js'; import RequestError from '#src/errors/RequestError/index.js'; -import type { Interaction } from './index.js'; import { generateHookTestPayload, parseResponse } from './utils.js'; const { jest } = import.meta; @@ -50,7 +49,7 @@ const findAllHooks = jest.fn().mockResolvedValue([hook]); const findHookById = jest.fn().mockResolvedValue(hook); const { createHookLibrary } = await import('./index.js'); -const { triggerInteractionHooksIfNeeded, attachExecutionStatsToHook, testHook } = createHookLibrary( +const { triggerInteractionHooks, attachExecutionStatsToHook, testHook } = createHookLibrary( new MockQueries({ users: { findUserById: jest.fn().mockReturnValue({ @@ -67,28 +66,17 @@ const { triggerInteractionHooksIfNeeded, attachExecutionStatsToHook, testHook } }) ); -describe('triggerInteractionHooksIfNeeded()', () => { +describe('triggerInteractionHooks()', () => { afterEach(() => { jest.clearAllMocks(); }); - it('should return if no user ID found', async () => { - await triggerInteractionHooksIfNeeded(InteractionEvent.SignIn); - - expect(findAllHooks).not.toBeCalled(); - }); - it('should set correct payload when hook triggered', async () => { jest.useFakeTimers().setSystemTime(100_000); - await triggerInteractionHooksIfNeeded( - InteractionEvent.SignIn, - // @ts-expect-error - { - jti: 'some_jti', - result: { login: { accountId: '123' } }, - params: { client_id: 'some_client' }, - } as Interaction + await triggerInteractionHooks( + { event: InteractionEvent.SignIn, sessionId: 'some_jti', applicationId: 'some_client' }, + { userId: '123' } ); expect(findAllHooks).toHaveBeenCalled(); diff --git a/packages/core/src/libraries/hook/index.ts b/packages/core/src/libraries/hook/index.ts index 89a8d5218..1d7c400aa 100644 --- a/packages/core/src/libraries/hook/index.ts +++ b/packages/core/src/libraries/hook/index.ts @@ -11,7 +11,6 @@ import { import { generateStandardId } from '@logto/shared'; import { conditional, pick, trySafe } from '@silverhand/essentials'; import { HTTPError } from 'got'; -import type Provider from 'oidc-provider'; import RequestError from '#src/errors/RequestError/index.js'; import { LogEntry } from '#src/middleware/koa-audit-log.js'; @@ -20,14 +19,32 @@ import { consoleLog } from '#src/utils/console.js'; import { generateHookTestPayload, parseResponse, sendWebhookRequest } from './utils.js'; +/** + * The context for triggering interaction hooks by `triggerInteractionHooks`. + * In the `koaInteractionHooks` middleware, + * we will store the context before processing the interaction and consume it after the interaction is processed if needed. + */ +export type InteractionHookContext = { + event: InteractionEvent; + sessionId?: string; + applicationId?: string; +}; + +/** + * The interaction hook result for triggering interaction hooks by `triggerInteractionHooks`. + * In the `koaInteractionHooks` middleware, + * if we get an interaction hook result after the interaction is processed, related hooks will be triggered. + */ +export type InteractionHookResult = { + userId: string; +}; + const eventToHook: Record = { [InteractionEvent.Register]: HookEvent.PostRegister, [InteractionEvent.SignIn]: HookEvent.PostSignIn, [InteractionEvent.ForgotPassword]: HookEvent.PostResetPassword, }; -export type Interaction = Awaited>; - export const createHookLibrary = (queries: Queries) => { const { applications: { findApplicationById }, @@ -37,18 +54,13 @@ export const createHookLibrary = (queries: Queries) => { hooks: { findAllHooks, findHookById }, } = queries; - const triggerInteractionHooksIfNeeded = async ( - event: InteractionEvent, - details?: Interaction, + const triggerInteractionHooks = async ( + interactionContext: InteractionHookContext, + interactionResult: InteractionHookResult, userAgent?: string ) => { - const userId = details?.result?.login?.accountId; - const sessionId = details?.jti; - const applicationId = details?.params.client_id; - - if (!userId) { - return; - } + const { userId } = interactionResult; + const { event, sessionId, applicationId } = interactionContext; const hookEvent = eventToHook[event]; const found = await findAllHooks(); @@ -63,9 +75,7 @@ export const createHookLibrary = (queries: Queries) => { const [user, application] = await Promise.all([ trySafe(findUserById(userId)), - trySafe(async () => - conditional(typeof applicationId === 'string' && (await findApplicationById(applicationId))) - ), + trySafe(async () => conditional(applicationId && (await findApplicationById(applicationId)))), ]); const payload = { @@ -155,7 +165,7 @@ export const createHookLibrary = (queries: Queries) => { }); return { - triggerInteractionHooksIfNeeded, + triggerInteractionHooks, attachExecutionStatsToHook, testHook, }; 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 d366c5430..f26f7fcea 100644 --- a/packages/core/src/routes/interaction/actions/submit-interaction.test.ts +++ b/packages/core/src/routes/interaction/actions/submit-interaction.test.ts @@ -64,6 +64,7 @@ describe('submit action', () => { ...createMockLogContext(), // eslint-disable-next-line @typescript-eslint/consistent-type-assertions interactionDetails: { params: {} } as Awaited>, + assignInteractionHookResult: jest.fn(), }; const profile = { username: 'username', diff --git a/packages/core/src/routes/interaction/actions/submit-interaction.ts b/packages/core/src/routes/interaction/actions/submit-interaction.ts index c15897caa..d89d2f852 100644 --- a/packages/core/src/routes/interaction/actions/submit-interaction.ts +++ b/packages/core/src/routes/interaction/actions/submit-interaction.ts @@ -17,11 +17,12 @@ import { EnvSet } from '#src/env-set/index.js'; import type { ConnectorLibrary } from '#src/libraries/connector.js'; import { assignInteractionResults } from '#src/libraries/session.js'; import { encryptUserPassword } from '#src/libraries/user.js'; -import type { LogEntry } from '#src/middleware/koa-audit-log.js'; +import type { LogEntry, WithLogContext } from '#src/middleware/koa-audit-log.js'; import type TenantContext from '#src/tenants/TenantContext.js'; import { getTenantId } from '#src/utils/tenant.js'; import type { WithInteractionDetailsContext } from '../middleware/koa-interaction-details.js'; +import { type WithInteractionHooksContext } from '../middleware/koa-interaction-hooks.js'; import type { Identifier, VerifiedInteractionResult, @@ -165,7 +166,7 @@ const parseUserProfile = async ( export default async function submitInteraction( interaction: VerifiedInteractionResult, - ctx: WithInteractionDetailsContext, + ctx: WithLogContext & WithInteractionDetailsContext & WithInteractionHooksContext, { provider, libraries, connectors, queries }: TenantContext, log?: LogEntry ) { @@ -211,6 +212,7 @@ export default async function submitInteraction( } await assignInteractionResults(ctx, provider, { login: { accountId: id } }); + ctx.assignInteractionHookResult({ userId: id }); log?.append({ userId: id }); appInsights.client?.trackEvent({ name: getEventName(Component.Core, CoreEvent.Register) }); @@ -227,6 +229,7 @@ export default async function submitInteraction( await updateUserById(accountId, updateUserProfile); await assignInteractionResults(ctx, provider, { login: { accountId } }); + ctx.assignInteractionHookResult({ userId: accountId }); appInsights.client?.trackEvent({ name: getEventName(Component.Core, CoreEvent.SignIn) }); @@ -239,6 +242,7 @@ export default async function submitInteraction( ); await updateUserById(accountId, { passwordEncrypted, passwordEncryptionMethod }); + ctx.assignInteractionHookResult({ userId: accountId }); await clearInteractionStorage(ctx, provider); ctx.status = 204; } diff --git a/packages/core/src/routes/interaction/consent.ts b/packages/core/src/routes/interaction/consent.ts index 4ded333c1..c3ace0838 100644 --- a/packages/core/src/routes/interaction/consent.ts +++ b/packages/core/src/routes/interaction/consent.ts @@ -1,5 +1,6 @@ import { conditional } from '@silverhand/essentials'; import type Router from 'koa-router'; +import { type IRouterParamContext } from 'koa-router'; import { z } from 'zod'; import { assignInteractionResults, saveUserFirstConsentedAppId } from '#src/libraries/session.js'; @@ -9,7 +10,7 @@ import assertThat from '#src/utils/assert-that.js'; import { interactionPrefix } from './const.js'; import type { WithInteractionDetailsContext } from './middleware/koa-interaction-details.js'; -export default function consentRoutes( +export default function consentRoutes( router: Router>, { provider, libraries, queries }: TenantContext ) { diff --git a/packages/core/src/routes/interaction/index.ts b/packages/core/src/routes/interaction/index.ts index d07b5b62a..ff024d83f 100644 --- a/packages/core/src/routes/interaction/index.ts +++ b/packages/core/src/routes/interaction/index.ts @@ -285,7 +285,7 @@ export default function interactionRoutes( router.post( `${interactionPrefix}/submit`, koaInteractionSie(queries), - koaInteractionHooks(tenant), + koaInteractionHooks(libraries), async (ctx, next) => { const { interactionDetails, createLog } = ctx; const interactionStorage = getInteractionStorage(interactionDetails.result); diff --git a/packages/core/src/routes/interaction/middleware/koa-interaction-details.ts b/packages/core/src/routes/interaction/middleware/koa-interaction-details.ts index 63eca5ed0..fff0efa55 100644 --- a/packages/core/src/routes/interaction/middleware/koa-interaction-details.ts +++ b/packages/core/src/routes/interaction/middleware/koa-interaction-details.ts @@ -1,15 +1,18 @@ import type { MiddlewareType } from 'koa'; +import { type IRouterParamContext } from 'koa-router'; import type Provider from 'oidc-provider'; -import type { WithLogContext } from '#src/middleware/koa-audit-log.js'; - -export type WithInteractionDetailsContext = ContextT & { +export type WithInteractionDetailsContext< + ContextT extends IRouterParamContext = IRouterParamContext +> = ContextT & { interactionDetails: Awaited>; }; -export default function koaInteractionDetails( - provider: Provider -): MiddlewareType> { +export default function koaInteractionDetails< + StateT, + ContextT extends IRouterParamContext, + ResponseT +>(provider: Provider): MiddlewareType, ResponseT> { return async (ctx, next) => { ctx.interactionDetails = await provider.interactionDetails(ctx.req, ctx.res); diff --git a/packages/core/src/routes/interaction/middleware/koa-interaction-hooks.ts b/packages/core/src/routes/interaction/middleware/koa-interaction-hooks.ts index cdff6ee97..6669b866b 100644 --- a/packages/core/src/routes/interaction/middleware/koa-interaction-hooks.ts +++ b/packages/core/src/routes/interaction/middleware/koa-interaction-hooks.ts @@ -1,31 +1,69 @@ -import { trySafe } from '@silverhand/essentials'; +import { type Optional, trySafe, conditionalString } from '@silverhand/essentials'; import type { MiddlewareType } from 'koa'; import type { IRouterParamContext } from 'koa-router'; -import type TenantContext from '#src/tenants/TenantContext.js'; +import { + type InteractionHookContext, + type InteractionHookResult, +} from '#src/libraries/hook/index.js'; +import type Libraries from '#src/tenants/Libraries.js'; import { getInteractionStorage } from '../utils/interaction.js'; import type { WithInteractionDetailsContext } from './koa-interaction-details.js'; +type AssignInteractionHookResult = (result: InteractionHookResult) => void; + +export type WithInteractionHooksContext< + ContextT extends IRouterParamContext = IRouterParamContext +> = ContextT & { assignInteractionHookResult: AssignInteractionHookResult }; + +/** + * The factory to create a new interaction hook middleware function. + * Interaction related event hooks will be triggered once we got the interaction hook result. + * Use `assignInteractionHookResult` to assign the interaction hook result. + */ export default function koaInteractionHooks< StateT, - ContextT extends WithInteractionDetailsContext, + ContextT extends WithInteractionDetailsContext, ResponseT >({ - provider, - libraries: { - hooks: { triggerInteractionHooksIfNeeded }, - }, -}: TenantContext): MiddlewareType { + hooks: { triggerInteractionHooks }, +}: Libraries): MiddlewareType, ResponseT> { return async (ctx, next) => { const { event } = getInteractionStorage(ctx.interactionDetails.result); + const { + interactionDetails, + header: { 'user-agent': userAgent }, + } = ctx; + + // Predefined interaction hook context + const interactionHookContext: InteractionHookContext = { + event, + sessionId: interactionDetails.jti, + applicationId: conditionalString(interactionDetails.params.client_id), + }; + + // eslint-disable-next-line @silverhand/fp/no-let + let interactionHookResult: Optional; + + /** + * Assign an interaction hook result to trigger webhook. + * Calling it multiple times will overwrite the original result, but only one webhook will be triggered. + * @param result The result to assign. + */ + ctx.assignInteractionHookResult = (result) => { + // eslint-disable-next-line @silverhand/fp/no-mutation + interactionHookResult = result; + }; await next(); - // Get up-to-date interaction details - const details = await trySafe(provider.interactionDetails(ctx.req, ctx.res)); - // Hooks should not crash the app - void trySafe(triggerInteractionHooksIfNeeded(event, details, ctx.header['user-agent'])); + if (interactionHookResult) { + // Hooks should not crash the app + void trySafe( + triggerInteractionHooks(interactionHookContext, interactionHookResult, userAgent) + ); + } }; } diff --git a/packages/core/src/routes/interaction/middleware/koa-interaction-sie.ts b/packages/core/src/routes/interaction/middleware/koa-interaction-sie.ts index bc66794aa..8754a48ec 100644 --- a/packages/core/src/routes/interaction/middleware/koa-interaction-sie.ts +++ b/packages/core/src/routes/interaction/middleware/koa-interaction-sie.ts @@ -1,15 +1,13 @@ import type { SignInExperience } from '@logto/schemas'; import type { MiddlewareType } from 'koa'; +import { type IRouterParamContext } from 'koa-router'; import type Queries from '#src/tenants/Queries.js'; -import type { WithInteractionDetailsContext } from './koa-interaction-details.js'; +export type WithInteractionSieContext = + ContextT & { signInExperience: SignInExperience }; -export type WithInteractionSieContext = WithInteractionDetailsContext & { - signInExperience: SignInExperience; -}; - -export default function koaInteractionSie({ +export default function koaInteractionSie({ signInExperiences: { findDefaultSignInExperience }, }: Queries): MiddlewareType, ResponseT> { return async (ctx, next) => { diff --git a/packages/core/src/routes/interaction/verifications/mandatory-user-profile-validation.ts b/packages/core/src/routes/interaction/verifications/mandatory-user-profile-validation.ts index ed24c439f..59519d030 100644 --- a/packages/core/src/routes/interaction/verifications/mandatory-user-profile-validation.ts +++ b/packages/core/src/routes/interaction/verifications/mandatory-user-profile-validation.ts @@ -2,12 +2,12 @@ import type { Profile, SignInExperience, User } from '@logto/schemas'; import { InteractionEvent, MissingProfile, SignInIdentifier } from '@logto/schemas'; import type { Nullable } from '@silverhand/essentials'; import { conditional } from '@silverhand/essentials'; -import type { Context } from 'koa'; import RequestError from '#src/errors/RequestError/index.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; +import { type WithInteractionDetailsContext } from '../middleware/koa-interaction-details.js'; import type { WithInteractionSieContext } from '../middleware/koa-interaction-sie.js'; import type { SocialIdentifier, @@ -213,7 +213,7 @@ const fillMissingProfileWithSocialIdentity = async ( export default async function validateMandatoryUserProfile( userQueries: Queries['users'], - ctx: WithInteractionSieContext, + ctx: WithInteractionSieContext & WithInteractionDetailsContext, interaction: MandatoryProfileValidationInteraction ) { const { signUp } = ctx.signInExperience; diff --git a/packages/integration-tests/src/helpers/interactions.ts b/packages/integration-tests/src/helpers/interactions.ts index 13a4c7776..f21b76e3c 100644 --- a/packages/integration-tests/src/helpers/interactions.ts +++ b/packages/integration-tests/src/helpers/interactions.ts @@ -10,11 +10,12 @@ import { createSocialAuthorizationUri, patchInteractionIdentifiers, putInteractionProfile, + sendVerificationCode, } from '#src/api/index.js'; import { generateUserId } from '#src/utils.js'; import { initClient, processSession, logoutClient } from './client.js'; -import { expectRejects } from './index.js'; +import { expectRejects, readVerificationCode } from './index.js'; import { enableAllPasswordSignInMethods } from './sign-in-experience.js'; import { generateNewUser } from './user.js'; @@ -85,3 +86,23 @@ export const createNewSocialUserWithUsernameAndPassword = async (connectorId: st return processSession(client, redirectTo); }; + +export const resetPassword = async ( + profile: { email: string } | { phone: string }, + newPassword: string +) => { + const client = await initClient(); + + await client.successSend(putInteraction, { event: InteractionEvent.ForgotPassword }); + await client.successSend(sendVerificationCode, { + ...profile, + }); + + const { code: verificationCode } = await readVerificationCode(); + await client.successSend(patchInteractionIdentifiers, { + ...profile, + verificationCode, + }); + await client.successSend(putInteractionProfile, { password: newPassword }); + await client.submitInteraction(); +}; diff --git a/packages/integration-tests/src/tests/api/hook/hook.trigger.test.ts b/packages/integration-tests/src/tests/api/hook/hook.trigger.test.ts index 02acf7ef8..f1a7b6c79 100644 --- a/packages/integration-tests/src/tests/api/hook/hook.trigger.test.ts +++ b/packages/integration-tests/src/tests/api/hook/hook.trigger.test.ts @@ -8,16 +8,26 @@ import { LogResult, SignInIdentifier, type Log, + ConnectorType, } from '@logto/schemas'; import { deleteUser } from '#src/api/admin-user.js'; import { authedAdminApi } from '#src/api/api.js'; import { getLogs } from '#src/api/logs.js'; +import { + clearConnectorsByTypes, + setEmailConnector, + setSmsConnector, +} from '#src/helpers/connector.js'; import { getHookCreationPayload } from '#src/helpers/hook.js'; import { createMockServer } from '#src/helpers/index.js'; -import { registerNewUser, signInWithPassword } from '#src/helpers/interactions.js'; -import { enableAllPasswordSignInMethods } from '#src/helpers/sign-in-experience.js'; +import { registerNewUser, resetPassword, signInWithPassword } from '#src/helpers/interactions.js'; +import { + enableAllPasswordSignInMethods, + enableAllVerificationCodeSignInMethods, +} from '#src/helpers/sign-in-experience.js'; import { generateNewUser, generateNewUserProfile } from '#src/helpers/user.js'; +import { generatePassword, waitFor } from '#src/utils.js'; type HookSecureData = { signature: string; @@ -182,4 +192,46 @@ describe('trigger hooks', () => { await deleteUser(userId); }); + + it('should trigger reset password hook and record properly when interaction finished', async () => { + await clearConnectorsByTypes([ConnectorType.Email, ConnectorType.Sms]); + await setEmailConnector(); + await setSmsConnector(); + await enableAllVerificationCodeSignInMethods({ + identifiers: [SignInIdentifier.Email, SignInIdentifier.Phone], + password: true, + verify: true, + }); + // Create a reset password hook + const resetPasswordHook = await authedAdminApi + .post('hooks', { + json: getHookCreationPayload(HookEvent.PostResetPassword, 'http://localhost:9999'), + }) + .json(); + const logKey: LogKey = 'TriggerHook.PostResetPassword'; + + const { user, userProfile } = await generateNewUser({ + primaryPhone: true, + primaryEmail: true, + password: true, + }); + // Reset Password by Email + await resetPassword({ email: userProfile.primaryEmail }, generatePassword()); + // Reset Password by Phone + await resetPassword({ phone: userProfile.primaryPhone }, generatePassword()); + // Wait for the hook to be trigged + await waitFor(1000); + + const logs = await getLogs(new URLSearchParams({ logKey, page_size: '100' })); + const relatedLogs = logs.filter( + ({ payload: { hookId, result } }) => + hookId === resetPasswordHook.id && result === LogResult.Success + ); + + expect(relatedLogs).toHaveLength(2); + + await authedAdminApi.delete(`hooks/${resetPasswordHook.id}`); + await deleteUser(user.id); + await clearConnectorsByTypes([ConnectorType.Email, ConnectorType.Sms]); + }); });