diff --git a/server/src/nest/database/database.service.ts b/server/src/nest/database/database.service.ts index 0d18ea549..103f4c8e1 100644 --- a/server/src/nest/database/database.service.ts +++ b/server/src/nest/database/database.service.ts @@ -56,6 +56,25 @@ export class DatabaseService { return isOwner(tripId, userId); } + /** + * The user ids a trip may refer to: its members plus the owner. Guests count — + * a guest is a credential-less users row joined into trip_members, and #1362 + * makes it assignable everywhere a real member is. + * + * This is canAccessTrip's question asked as a set rather than a predicate, so + * it lives beside it. Domains that accept user ids in a write body intersect + * against it: holding the permission says you may edit the trip, not that any + * id you name belongs to it. Runs on the connection rather than delegating to + * db/database, because it has no legacy export to stay parity with. + */ + rosterUserIds(tripId: number | string): Set { + const rows = this.all<{ user_id: number }>( + 'SELECT user_id FROM trip_members WHERE trip_id = ? UNION SELECT user_id FROM trips WHERE id = ?', + tripId, tripId, + ); + return new Set(rows.map(r => r.user_id)); + } + getPlaceWithTags(placeId: number | string): PlaceWithTags | null { return getPlaceWithTags(placeId); } diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index 1fef2f8a3..01c169ccb 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -231,7 +231,8 @@ export class PackingService { // "Shared with specific people" — record the recipients it covers. if (data.visibility === 'shared' && Array.isArray(data.recipient_ids)) { const ins = this.db.prepare('INSERT OR IGNORE INTO packing_item_recipients (item_id, user_id) VALUES (?, ?)'); - for (const uid of data.recipient_ids) if (uid !== ownerId) ins.run(id, uid); + const roster = this.tripRosterIds(tripId); + for (const uid of data.recipient_ids) if (uid !== ownerId && roster.has(uid)) ins.run(id, uid); } return id; }); @@ -350,7 +351,8 @@ export class PackingService { if (visibility === 'shared') { const ins = this.db.prepare('INSERT OR IGNORE INTO packing_item_recipients (item_id, user_id) VALUES (?, ?)'); const owner = item.owner_id ?? actingUserId; - for (const uid of recipientIds) if (uid !== owner) ins.run(id, uid); + const roster = this.tripRosterIds(tripId); + for (const uid of recipientIds) if (uid !== owner && roster.has(uid)) ins.run(id, uid); } // Leaving the Common tier drops any co-contributors (they only apply to Common). if (visibility !== 'common') this.db.run('DELETE FROM packing_item_contributors WHERE item_id = ?', id); @@ -479,10 +481,14 @@ export class PackingService { })); } - /** Owner + collaborators of a trip — the only user ids that may be assigned to a bag. */ + /** + * Owner + collaborators of a trip, guests included — the only user ids + * assignable anywhere on it, not just to a bag. The wording used to say + * "assigned to a bag", which is why the category and recipient writes below + * grew up without it. + */ private tripRosterIds(tripId: string | number): Set { - const rows = this.db.all<{ user_id: number }>('SELECT user_id FROM trip_members WHERE trip_id = ? UNION SELECT user_id FROM trips WHERE id = ?', tripId, tripId); - return new Set(rows.map(r => r.user_id)); + return this.db.rosterUserIds(tripId); } setBagMembers(tripId: string | number, bagId: string | number, userIds: number[]) { @@ -665,7 +671,9 @@ export class PackingService { if (Array.isArray(userIds) && userIds.length > 0) { const insert = this.db.prepare('INSERT OR IGNORE INTO packing_category_assignees (trip_id, category_name, user_id) VALUES (?, ?, ?)'); - for (const uid of userIds) insert.run(tripId, categoryName, uid); + // Same rule as setBagMembers: only people on this trip may be assigned. + const roster = this.tripRosterIds(tripId); + for (const uid of userIds) if (roster.has(uid)) insert.run(tripId, categoryName, uid); } }); diff --git a/server/src/nest/todo/todo.service.ts b/server/src/nest/todo/todo.service.ts index 8d2025ba0..31bf80953 100644 --- a/server/src/nest/todo/todo.service.ts +++ b/server/src/nest/todo/todo.service.ts @@ -136,7 +136,12 @@ export class TodoService { if (Array.isArray(userIds) && userIds.length > 0) { const insert = this.db.prepare('INSERT OR IGNORE INTO todo_category_assignees (trip_id, category_name, user_id) VALUES (?, ?, ?)'); - for (const uid of userIds) insert.run(tripId, categoryName, uid); + // Only people on this trip may be assigned, the way packing filters bag + // members and reservations filter travellers. Dropped rather than + // rejected: a copied trip carries assignee ids across before its members + // exist, and a 400 would make the picker unusable there. + const roster = this.db.rosterUserIds(tripId); + for (const uid of userIds) if (roster.has(uid)) insert.run(tripId, categoryName, uid); } }); diff --git a/server/tests/unit/nest/packing.service.test.ts b/server/tests/unit/nest/packing.service.test.ts index 44571e593..649db5838 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -515,6 +515,7 @@ describe('three-tier packing sharing (#858)', () => { const { user: friend } = createUser(testDb); const { user: stranger } = createUser(testDb); const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, friend.id); const item = svc.createItem(trip.id, { name: 'Power bank', visibility: 'shared', recipient_ids: [friend.id] }, owner.id) as any; expect(item.is_private).toBe(1); @@ -539,6 +540,7 @@ describe('three-tier packing sharing (#858)', () => { const { user: owner } = createUser(testDb); const { user: friend } = createUser(testDb); const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, friend.id); const item = svc.createItem(trip.id, { name: 'First aid', visibility: 'personal' }, owner.id) as any; // A non-owner who cannot see the item at all gets the missing-item answer, so @@ -1007,6 +1009,8 @@ describe('packing item object-level authorization', () => { const { user: intruder } = createUser(testDb); const { user: friend } = createUser(testDb); const trip = createTrip(testDb, owner.id); + // The recipient is a fellow traveller; the intruder stays off the trip on purpose. + addTripMember(testDb, trip.id, friend.id); const personal = svc.createItem(trip.id, { name: 'Diary', visibility: 'personal' }, owner.id) as any; const shared = svc.createItem(trip.id, { name: 'Power bank', visibility: 'shared', recipient_ids: [friend.id] }, owner.id) as any; const common = svc.createItem(trip.id, { name: 'Tent', visibility: 'common' }, owner.id) as any; diff --git a/server/tests/unit/nest/todo.service.test.ts b/server/tests/unit/nest/todo.service.test.ts index b52e7931a..b3fcc351e 100644 --- a/server/tests/unit/nest/todo.service.test.ts +++ b/server/tests/unit/nest/todo.service.test.ts @@ -270,6 +270,35 @@ describe('getCategoryAssignees / updateCategoryAssignees', () => { expect(assignees['Logistics']).toHaveLength(1); }); + // Holding todo_edit says you may edit this trip's list, not that any user id + // you name belongs to it. The read-back joins users and hands the caller a + // display name and an avatar, so an unfiltered id turns the endpoint into a + // lookup for every account on the instance. + it('TODO-SVC-017a: drops a user who is not on the trip, and says nothing about them', () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + + const rows = svc.updateCategoryAssignees(trip.id, 'Packing', [owner.id, stranger.id, member.id]) as { user_id: number }[]; + + expect(rows.map(r => r.user_id).sort()).toEqual([owner.id, member.id].sort()); + expect(JSON.stringify(rows)).not.toContain(stranger.username); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM todo_category_assignees WHERE user_id = ?').get(stranger.id)).toEqual({ n: 0 }); + }); + + it('TODO-SVC-017b: keeps a guest, who is a trip member like any other', () => { + const { user: owner } = createUser(testDb); + const { user: guest } = createUser(testDb); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, guest.id); + + const rows = svc.updateCategoryAssignees(trip.id, 'Packing', [guest.id]) as { user_id: number }[]; + expect(rows.map(r => r.user_id)).toEqual([guest.id]); + }); + it('TODO-SVC-020: updateCategoryAssignees replaces existing assignees (not append)', () => { const { user: owner } = createUser(testDb); const { user: member } = createUser(testDb);