From 3eb681e847e9c55eaaa69c22bea4f4e66c7eac36 Mon Sep 17 00:00:00 2001 From: Dan Ribbens Date: Fri, 26 Jan 2024 13:39:45 -0500 Subject: [PATCH] fix: afterLogin hook write conflicts (#4904) * fix: afterLogin hook conflict * test: afterLogin hook returns for assertion * chore: commit increment login attempt --- packages/payload/src/auth/operations/login.ts | 14 ++--- .../local/incrementLoginAttempts.ts | 1 + .../strategies/local/resetLoginAttempts.ts | 2 + test/admin/collections/Users.ts | 2 +- .../hooks/collections/Users/afterLoginHook.ts | 12 +++++ test/hooks/collections/Users/index.ts | 14 +++-- test/hooks/int.spec.ts | 54 +++++++++++-------- 7 files changed, 64 insertions(+), 35 deletions(-) create mode 100644 test/hooks/collections/Users/afterLoginHook.ts diff --git a/packages/payload/src/auth/operations/login.ts b/packages/payload/src/auth/operations/login.ts index ab0e574640..2ba8273350 100644 --- a/packages/payload/src/auth/operations/login.ts +++ b/packages/payload/src/auth/operations/login.ts @@ -18,8 +18,8 @@ import sanitizeInternalFields from '../../utilities/sanitizeInternalFields' import isLocked from '../isLocked' import { authenticateLocalStrategy } from '../strategies/local/authenticate' import { incrementLoginAttempts } from '../strategies/local/incrementLoginAttempts' +import { resetLoginAttempts } from '../strategies/local/resetLoginAttempts' import { getFieldsToSign } from './getFieldsToSign' -import unlock from './unlock' export type Result = { exp?: number @@ -115,16 +115,16 @@ async function login( }) } + if (shouldCommit) await commitTransaction(req) + throw new AuthenticationError(req.t) } if (maxLoginAttemptsEnabled) { - await unlock({ - collection: { - config: collectionConfig, - }, - data, - overrideAccess: true, + await resetLoginAttempts({ + collection: collectionConfig, + doc: user, + payload: req.payload, req, }) } diff --git a/packages/payload/src/auth/strategies/local/incrementLoginAttempts.ts b/packages/payload/src/auth/strategies/local/incrementLoginAttempts.ts index 99472a64d8..3f5c338fc8 100644 --- a/packages/payload/src/auth/strategies/local/incrementLoginAttempts.ts +++ b/packages/payload/src/auth/strategies/local/incrementLoginAttempts.ts @@ -52,5 +52,6 @@ export const incrementLoginAttempts = async ({ id: doc.id, collection: collection.slug, data, + req, }) } diff --git a/packages/payload/src/auth/strategies/local/resetLoginAttempts.ts b/packages/payload/src/auth/strategies/local/resetLoginAttempts.ts index f58dc202fb..fa5490f96c 100644 --- a/packages/payload/src/auth/strategies/local/resetLoginAttempts.ts +++ b/packages/payload/src/auth/strategies/local/resetLoginAttempts.ts @@ -15,6 +15,7 @@ export const resetLoginAttempts = async ({ payload, req, }: Args): Promise => { + if (!('lockUntil' in doc && typeof doc.lockUntil === 'string') || doc.loginAttempts === 0) return await payload.update({ id: doc.id, collection: collection.slug, @@ -22,6 +23,7 @@ export const resetLoginAttempts = async ({ lockUntil: null, loginAttempts: 0, }, + overrideAccess: true, req, }) } diff --git a/test/admin/collections/Users.ts b/test/admin/collections/Users.ts index 732691a0fd..8644d27f12 100644 --- a/test/admin/collections/Users.ts +++ b/test/admin/collections/Users.ts @@ -4,10 +4,10 @@ import { usersCollectionSlug } from '../slugs' export const Users: CollectionConfig = { slug: usersCollectionSlug, - auth: true, admin: { useAsTitle: 'email', }, + auth: true, fields: [ { name: 'textField', diff --git a/test/hooks/collections/Users/afterLoginHook.ts b/test/hooks/collections/Users/afterLoginHook.ts new file mode 100644 index 0000000000..097dc5a2b3 --- /dev/null +++ b/test/hooks/collections/Users/afterLoginHook.ts @@ -0,0 +1,12 @@ +import type { AfterLoginHook } from '../../../../packages/payload/src/collections/config/types' + +export const afterLoginHook: AfterLoginHook = async ({ req, user }) => { + return req.payload.update({ + id: user.id, + collection: 'hooks-users', + data: { + afterLoginHook: true, + }, + req, + }) +} diff --git a/test/hooks/collections/Users/index.ts b/test/hooks/collections/Users/index.ts index dc86c468af..f6a7bcbd42 100644 --- a/test/hooks/collections/Users/index.ts +++ b/test/hooks/collections/Users/index.ts @@ -6,8 +6,9 @@ import type { Payload } from '../../../../packages/payload/src/payload' import { AuthenticationError } from '../../../../packages/payload/src/errors' import { devUser, regularUser } from '../../../credentials' +import { afterLoginHook } from './afterLoginHook' -const beforeLoginHook: BeforeLoginHook = ({ user, req }) => { +const beforeLoginHook: BeforeLoginHook = ({ req, user }) => { const isAdmin = user.roles.includes('admin') ? user : undefined if (!isAdmin) { throw new AuthenticationError(req.t) @@ -33,16 +34,21 @@ const Users: CollectionConfig = { fields: [ { name: 'roles', - label: 'Role', type: 'select', - options: ['admin', 'user'], defaultValue: 'user', + hasMany: true, + label: 'Role', + options: ['admin', 'user'], required: true, saveToJWT: true, - hasMany: true, + }, + { + name: 'afterLoginHook', + type: 'checkbox', }, ], hooks: { + afterLogin: [afterLoginHook], beforeLogin: [beforeLoginHook], }, } diff --git a/test/hooks/int.spec.ts b/test/hooks/int.spec.ts index dd7e58856f..2dfcf36ee4 100644 --- a/test/hooks/int.spec.ts +++ b/test/hooks/int.spec.ts @@ -27,7 +27,7 @@ describe('Hooks', () => { beforeAll(async () => { const { serverURL } = await initPayloadTest({ __dirname, init: { local: false } }) const config = await configPromise - client = new RESTClient(config, { serverURL, defaultSlug: transformSlug }) + client = new RESTClient(config, { defaultSlug: transformSlug, serverURL }) apiUrl = `${serverURL}/api` }) @@ -43,8 +43,8 @@ describe('Hooks', () => { const doc = await payload.create({ collection: transformSlug, data: { - transform: [2, 8], localizedTransform: [2, 8], + transform: [2, 8], }, }) @@ -59,15 +59,15 @@ describe('Hooks', () => { doc = await payload.create({ collection: hooksSlug, data: { - fieldBeforeValidate: false, - collectionBeforeValidate: false, - fieldBeforeChange: false, - collectionBeforeChange: false, - fieldAfterChange: false, collectionAfterChange: false, - collectionBeforeRead: false, - fieldAfterRead: false, collectionAfterRead: false, + collectionBeforeChange: false, + collectionBeforeRead: false, + collectionBeforeValidate: false, + fieldAfterChange: false, + fieldAfterRead: false, + fieldBeforeChange: false, + fieldBeforeValidate: false, }, }) @@ -84,10 +84,10 @@ describe('Hooks', () => { const document: NestedAfterReadHook = await payload.create({ collection: nestedAfterReadHooksSlug, data: { - text: 'ok', group: { array: [{ input: 'input' }], }, + text: 'ok', }, }) @@ -106,7 +106,6 @@ describe('Hooks', () => { const document = await payload.create({ collection: nestedAfterReadHooksSlug, data: { - text: 'ok', group: { array: [ { @@ -117,12 +116,13 @@ describe('Hooks', () => { shouldPopulate: relation.id, }, }, + text: 'ok', }, }) const retrievedDoc = await payload.findByID({ - collection: nestedAfterReadHooksSlug, id: document.id, + collection: nestedAfterReadHooksSlug, }) expect(retrievedDoc.group.array[0].shouldPopulate.title).toEqual(relation.title) @@ -138,8 +138,8 @@ describe('Hooks', () => { }) const retrievedDoc = await payload.findByID({ - collection: chainingHooksSlug, id: document.id, + collection: chainingHooksSlug, }) expect(retrievedDoc.text).toEqual('ok!!') @@ -189,15 +189,15 @@ describe('Hooks', () => { const [updatedDoc1, updatedDoc2] = await Promise.all([ await payload.update({ - collection: afterOperationSlug, id: doc1.id, + collection: afterOperationSlug, data: { title: 'Title', }, }), await payload.update({ - collection: afterOperationSlug, id: doc2.id, + collection: afterOperationSlug, data: { title: 'Title', }, @@ -225,8 +225,8 @@ describe('Hooks', () => { }) const retrievedDoc = await payload.findByID({ - collection: contextHooksSlug, id: document.id, + collection: contextHooksSlug, }) expect(retrievedDoc.value).toEqual('secret') @@ -235,17 +235,17 @@ describe('Hooks', () => { it('should pass context from local API to hooks', async () => { const document = await payload.create({ collection: contextHooksSlug, - data: { - value: 'wrongvalue', - }, context: { secretValue: 'data from local API', }, + data: { + value: 'wrongvalue', + }, }) const retrievedDoc = await payload.findByID({ - collection: contextHooksSlug, id: document.id, + collection: contextHooksSlug, }) expect(retrievedDoc.value).toEqual('data from local API') @@ -282,8 +282,8 @@ describe('Hooks', () => { const document = (await response.json()).doc const retrievedDoc = await payload.findByID({ - collection: contextHooksSlug, id: document.id, + collection: contextHooksSlug, }) expect(retrievedDoc.value).toEqual('data from rest API') @@ -291,7 +291,7 @@ describe('Hooks', () => { }) describe('auth collection hooks', () => { - it('allow admin login', async () => { + it('should call afterLogin hook', async () => { const { user } = await payload.login({ collection: hooksUsersSlug, data: { @@ -299,7 +299,15 @@ describe('Hooks', () => { password: devUser.password, }, }) + + const result = await payload.findByID({ + id: user.id, + collection: hooksUsersSlug, + }) + expect(user).toBeDefined() + expect(user.afterLoginHook).toStrictEqual(true) + expect(result.afterLoginHook).toStrictEqual(true) }) it('deny user login', async () => { @@ -342,8 +350,8 @@ describe('Hooks', () => { // BeforeRead is only run for find operations const foundDoc = await payload.findByID({ - collection: dataHooksSlug, id: doc.id, + collection: dataHooksSlug, }) expect(JSON.parse(foundDoc.collection_beforeRead_collection)).toStrictEqual(