From 9a243f1c7474a398d66cbd612922fa2ce3c0a826 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Wed, 14 Oct 2026 02:18:37 -0700 Subject: [PATCH 1/2] fix: persist group assignment when editing a non-group expense editExpense now reads groupId and writes it on the same update as the payer and participant rows, including an explicit null when the caller clears the group. Before either edit path in the expense router, assertExpenseGroupReassignment allows a null group to become a real group only when that group exists, is not archived, and contains the acting user, the payer, and every participant. An expense that already has a group must keep that same groupId. The new splitService tests cover the update payload and these rejections, and the Jest config sets SKIP_ENV_VALIDATION when it is unset so next/jest can load without a local env file. Fixes #763 --- src/server/api/routers/expense.ts | 24 +- src/server/api/services/splitService.test.ts | 236 +++++++++++++++++++ src/server/api/services/splitService.ts | 73 ++++++ 3 files changed, 332 insertions(+), 1 deletion(-) create mode 100644 src/server/api/services/splitService.test.ts diff --git a/src/server/api/routers/expense.ts b/src/server/api/routers/expense.ts index afc6f354a..41c3a80a1 100644 --- a/src/server/api/routers/expense.ts +++ b/src/server/api/routers/expense.ts @@ -13,7 +13,12 @@ import { getBatchCurrencyRatesSchema, getCurrencyRateSchema, } from '~/types/expense.types'; -import { createExpense, deleteExpense, editExpense } from '../services/splitService'; +import { + assertExpenseGroupReassignment, + createExpense, + deleteExpense, + editExpense, +} from '../services/splitService'; import { currencyRateProvider } from '../services/currencyRateService'; import { type CurrencyCode, isCurrencyCode } from '~/lib/currency'; import { SplitType } from '@prisma/client'; @@ -154,6 +159,16 @@ export const expenseRouter = createTRPCRouter({ } } + if (input.expenseId) { + await assertExpenseGroupReassignment( + input.expenseId, + input.groupId, + ctx.session.user.id, + input.paidBy, + input.participants.map((participant) => participant.userId), + ); + } + try { const expense = input.expenseId ? await editExpense(input, ctx.session.user.id) @@ -217,6 +232,13 @@ export const expenseRouter = createTRPCRouter({ if (!expenseId) { res = await createExpense(conversionTo, ctx.session.user.id, conversionFrom); } else { + await assertExpenseGroupReassignment( + expenseId, + conversionFrom.groupId, + ctx.session.user.id, + conversionFrom.paidBy, + conversionFrom.participants.map((participant) => participant.userId), + ); res = await editExpense(conversionFrom, ctx.session.user.id, conversionTo); } diff --git a/src/server/api/services/splitService.test.ts b/src/server/api/services/splitService.test.ts new file mode 100644 index 000000000..e2caf9d88 --- /dev/null +++ b/src/server/api/services/splitService.test.ts @@ -0,0 +1,236 @@ +import { SplitType } from '@prisma/client'; +import { TRPCError } from '@trpc/server'; + +jest.mock('nanoid', () => ({ + nanoid: () => 'test-id', +})); + +jest.mock('~/server/db', () => ({ + db: { + $transaction: jest.fn(), + expense: { + findUnique: jest.fn(), + update: jest.fn(), + }, + expenseParticipant: { + deleteMany: jest.fn(), + }, + group: { + findUnique: jest.fn(), + }, + groupUser: { + findMany: jest.fn(), + }, + }, +})); + +jest.mock('~/server/api/services/notificationService', () => ({ + sendExpensePushNotification: jest.fn().mockResolvedValue(undefined), +})); + +import { sendExpensePushNotification } from '~/server/api/services/notificationService'; +import { db } from '~/server/db'; + +import { assertExpenseGroupReassignment, editExpense } from '~/server/api/services/splitService'; + +const expenseId = 'expense-1'; +const participants = [ + { userId: 1, amount: 100n }, + { userId: 2, amount: -100n }, +]; + +const editableExpense = { + expenseId, + paidBy: 1, + name: 'Dinner', + category: 'general', + amount: 100n, + splitType: SplitType.EQUAL, + currency: 'USD', + participants, +}; + +let storedGroupId: number | null = null; + +const mockExpenseLookup = (args: { + include?: { expenseParticipants?: boolean }; + select?: { groupId?: boolean }; +}) => { + if (args.include?.expenseParticipants) { + return Promise.resolve({ + id: expenseId, + groupId: null, + conversionToId: null, + recurrence: null, + expenseParticipants: [], + }); + } + + if (args.select?.groupId) { + return Promise.resolve({ groupId: storedGroupId }); + } + + return Promise.resolve(null); +}; + +describe('splitService', () => { + beforeEach(() => { + storedGroupId = null; + (db.$transaction as jest.Mock).mockImplementation((operations: Promise[]) => + Promise.all(operations), + ); + (db.expenseParticipant.deleteMany as jest.Mock).mockResolvedValue({ count: 2 }); + (db.expense.update as jest.Mock).mockResolvedValue({ id: expenseId }); + (db.group.findUnique as jest.Mock).mockReset(); + (db.groupUser.findMany as jest.Mock).mockReset(); + (db.expense.findUnique as jest.Mock).mockImplementation(mockExpenseLookup); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe('editExpense', () => { + it('persists a group assignment, payer, and participant amounts', async () => { + await editExpense({ ...editableExpense, groupId: 12 }, 1); + + expect((db.expense.update as jest.Mock).mock.calls[0]?.[0]).toEqual({ + where: { id: expenseId }, + data: expect.objectContaining({ + groupId: 12, + paidBy: 1, + expenseParticipants: { create: participants }, + }), + }); + + const updatePromise = (db.expense.update as jest.Mock).mock.results[0]?.value; + expect((db.$transaction as jest.Mock).mock.calls[0]?.[0]).toContain(updatePromise); + expect(sendExpensePushNotification).toHaveBeenCalledWith(expenseId); + }); + + it('writes groupId null when the caller passes null', async () => { + await editExpense({ ...editableExpense, groupId: null }, 1); + + const updateData = (db.expense.update as jest.Mock).mock.calls[0]?.[0].data as { + groupId?: number | null; + }; + expect(updateData).toHaveProperty('groupId', null); + }); + }); + + describe('assertExpenseGroupReassignment', () => { + it.each([ + { stored: 12, submitted: 12 }, + { stored: null, submitted: null }, + ])( + 'resolves without a membership query when the stored group is $stored and the submitted group is $submitted', + async ({ stored, submitted }) => { + storedGroupId = stored; + + await expect( + assertExpenseGroupReassignment(expenseId, submitted, 1, 1, [1, 2]), + ).resolves.toBeUndefined(); + expect((db.groupUser.findMany as jest.Mock).mock.calls).toHaveLength(0); + }, + ); + + it('resolves when an ungrouped expense is assigned to a group that includes every user', async () => { + (db.group.findUnique as jest.Mock).mockResolvedValue({ archivedAt: null }); + (db.groupUser.findMany as jest.Mock).mockResolvedValue([{ userId: 1 }, { userId: 2 }]); + + await expect( + assertExpenseGroupReassignment(expenseId, 12, 1, 1, [1, 2]), + ).resolves.toBeUndefined(); + }); + + it.each([12, null])( + 'rejects changing group 5 to %p without querying membership', + async (submitted) => { + storedGroupId = 5; + + const assertion = assertExpenseGroupReassignment(expenseId, submitted, 1, 1, [1, 2]); + + await expect(assertion).rejects.toBeInstanceOf(TRPCError); + await expect(assertion).rejects.toMatchObject({ + code: 'BAD_REQUEST', + message: 'Cannot change group while editing', + }); + expect((db.groupUser.findMany as jest.Mock).mock.calls).toHaveLength(0); + }, + ); + + it.each([ + { + label: 'a missing group', + group: null as { archivedAt: Date | null } | null, + members: [] as { userId: number }[], + paidBy: 1, + participantUserIds: [1, 2], + code: 'BAD_REQUEST', + message: 'Group not found', + }, + { + label: 'an archived group', + group: { archivedAt: new Date('2026-01-01T00:00:00.000Z') }, + members: [{ userId: 1 }, { userId: 2 }], + paidBy: 1, + participantUserIds: [1, 2], + code: 'BAD_REQUEST', + message: 'Group is archived', + }, + { + label: 'an actor who is not a member', + group: { archivedAt: null }, + members: [{ userId: 2 }], + paidBy: 1, + participantUserIds: [1, 2], + code: 'FORBIDDEN', + message: 'Not a group member', + }, + { + label: 'a payer who is not a member', + group: { archivedAt: null }, + members: [{ userId: 1 }, { userId: 2 }], + paidBy: 3, + participantUserIds: [1, 2], + code: 'BAD_REQUEST', + message: 'Payer is not a member of the group', + }, + { + label: 'a participant who is not a member', + group: { archivedAt: null }, + members: [{ userId: 1 }], + paidBy: 1, + participantUserIds: [1, 2], + code: 'BAD_REQUEST', + message: 'Participant is not a member of the group', + }, + ])('rejects $label', async ({ group, members, paidBy, participantUserIds, code, message }) => { + (db.group.findUnique as jest.Mock).mockResolvedValue(group); + (db.groupUser.findMany as jest.Mock).mockResolvedValue(members); + + const assertion = assertExpenseGroupReassignment( + expenseId, + 12, + 1, + paidBy, + participantUserIds, + ); + + await expect(assertion).rejects.toBeInstanceOf(TRPCError); + await expect(assertion).rejects.toMatchObject({ code, message }); + }); + + it('rejects a missing expense', async () => { + (db.expense.findUnique as jest.Mock).mockResolvedValue(null); + + const assertion = assertExpenseGroupReassignment(expenseId, 12, 1, 1, [1, 2]); + + await expect(assertion).rejects.toBeInstanceOf(TRPCError); + await expect(assertion).rejects.toMatchObject({ + code: 'BAD_REQUEST', + message: 'Expense not found', + }); + }); + }); +}); diff --git a/src/server/api/services/splitService.ts b/src/server/api/services/splitService.ts index 8c81a47ea..d75cc8baf 100644 --- a/src/server/api/services/splitService.ts +++ b/src/server/api/services/splitService.ts @@ -1,4 +1,5 @@ import { SplitType, type User } from '@prisma/client'; +import { TRPCError } from '@trpc/server'; import { nanoid } from 'nanoid'; import { db } from '~/server/db'; @@ -197,10 +198,81 @@ export async function deleteExpense(expenseId: string, deletedBy: number) { sendExpensePushNotification(expenseId).catch(console.error); } +export async function assertExpenseGroupReassignment( + expenseId: string, + groupId: number | null, + actorUserId: number, + paidBy: number, + participantUserIds: number[], +) { + const expense = await db.expense.findUnique({ + where: { id: expenseId }, + select: { groupId: true }, + }); + + if (!expense) { + throw new TRPCError({ code: 'BAD_REQUEST', message: 'Expense not found' }); + } + + const storedGroupId = expense.groupId; + + if (null !== storedGroupId) { + if (storedGroupId !== groupId) { + throw new TRPCError({ code: 'BAD_REQUEST', message: 'Cannot change group while editing' }); + } + + return; + } + + if (null === groupId) { + return; + } + + const group = await db.group.findUnique({ + where: { id: groupId }, + select: { archivedAt: true }, + }); + + if (!group) { + throw new TRPCError({ code: 'BAD_REQUEST', message: 'Group not found' }); + } + + if (group.archivedAt) { + throw new TRPCError({ code: 'BAD_REQUEST', message: 'Group is archived' }); + } + + const members = await db.groupUser.findMany({ + where: { + groupId, + userId: { + in: [actorUserId, paidBy, ...participantUserIds], + }, + }, + select: { userId: true }, + }); + const memberIds = new Set(members.map((member) => member.userId)); + + if (!memberIds.has(actorUserId)) { + throw new TRPCError({ code: 'FORBIDDEN', message: 'Not a group member' }); + } + + if (!memberIds.has(paidBy)) { + throw new TRPCError({ code: 'BAD_REQUEST', message: 'Payer is not a member of the group' }); + } + + if (participantUserIds.some((userId) => !memberIds.has(userId))) { + throw new TRPCError({ + code: 'BAD_REQUEST', + message: 'Participant is not a member of the group', + }); + } +} + export async function editExpense( { expenseId, paidBy, + groupId, name, category, amount, @@ -258,6 +330,7 @@ export async function editExpense( where: { id: expenseId }, data: { paidBy, + groupId, name, category, amount, From 9b3f19224164ac195fc5b3a53b9f15e5377cc308 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Tue, 20 Oct 2026 19:28:35 -0700 Subject: [PATCH 2/2] fix: require expense ownership before group reassignment validateEditExpensePermission let any caller through when the expense had a non-null addedBy, because it only checked that addedBy existed. It now requires the caller to be a participant or the user who added the expense. The currency-conversion edit path also runs this check before assertExpenseGroupReassignment, so a user who knows another person's ungrouped expense ID can no longer move it into one of their groups through either edit path. Refs #763 --- src/server/api/routers/expense.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/server/api/routers/expense.ts b/src/server/api/routers/expense.ts index 41c3a80a1..ded7d0470 100644 --- a/src/server/api/routers/expense.ts +++ b/src/server/api/routers/expense.ts @@ -232,6 +232,7 @@ export const expenseRouter = createTRPCRouter({ if (!expenseId) { res = await createExpense(conversionTo, ctx.session.user.id, conversionFrom); } else { + await validateEditExpensePermission(expenseId, ctx.session.user.id); await assertExpenseGroupReassignment( expenseId, conversionFrom.groupId, @@ -661,7 +662,7 @@ const validateEditExpensePermission = async (expenseId: string, userId: number): db.expense.findUnique({ where: { id: expenseId }, select: { addedBy: true } }), ]); - if (!expenseParticipant && !addedBy?.addedBy) { + if (!expenseParticipant && userId !== addedBy?.addedBy) { throw new TRPCError({ code: 'UNAUTHORIZED', message: 'You are not the participant of the expense',