diff --git a/client/src/mobile/screens/collections/MCollections.tsx b/client/src/mobile/screens/collections/MCollections.tsx index 5a8abdae1..0d1c7c3ef 100644 --- a/client/src/mobile/screens/collections/MCollections.tsx +++ b/client/src/mobile/screens/collections/MCollections.tsx @@ -383,7 +383,7 @@ export default function MCollections() { - {c.canEdit && ( + {c.canDelete && ( diff --git a/client/src/pages/CollectionsPage.tsx b/client/src/pages/CollectionsPage.tsx index 0c1acc090..2aab1b5fe 100644 --- a/client/src/pages/CollectionsPage.tsx +++ b/client/src/pages/CollectionsPage.tsx @@ -277,7 +277,7 @@ function CollectionsPageDesktop(): React.ReactElement { {t('collections.labels.assign')} )} - {c.canEdit && ( + {c.canDelete && ( diff --git a/client/src/pages/CollectionsPage.wiring.test.tsx b/client/src/pages/CollectionsPage.wiring.test.tsx index 194be364e..80a7a6579 100644 --- a/client/src/pages/CollectionsPage.wiring.test.tsx +++ b/client/src/pages/CollectionsPage.wiring.test.tsx @@ -479,6 +479,15 @@ describe('CollectionsPage — select mode', () => { expect(screen.getByRole('button', { name: /collections.copyToTrip/ })).toBeInTheDocument() }) + it('FE-PAGE-COLLPAGE-021b: an editor keeps the edit actions but cannot move places out of the list', () => { + renderPage({ ...selecting, canEdit: true, canDelete: false }) + expect(screen.queryByRole('button', { name: /collections.moveToList/ })).toBeNull() + expect(screen.queryByRole('button', { name: /common.delete/ })).toBeNull() + expect(screen.getByRole('button', { name: /collections.labels.assign/ })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /collections.duplicateToList/ })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /collections.copyToTrip/ })).toBeInTheDocument() + }) + it('FE-PAGE-COLLPAGE-022: bulk buttons are disabled while nothing is selected', () => { renderPage({ selectMode: true, selectedIds: [] }) expect(screen.getByRole('button', { name: /collections.moveToList/ })).toBeDisabled() diff --git a/client/tests/unit/mobile/collections/MCollections.test.tsx b/client/tests/unit/mobile/collections/MCollections.test.tsx index 0a54d38fc..1f90cd9ac 100644 --- a/client/tests/unit/mobile/collections/MCollections.test.tsx +++ b/client/tests/unit/mobile/collections/MCollections.test.tsx @@ -435,6 +435,16 @@ describe('MCollections', () => { expect(screen.getByRole('button', { name: /collections.duplicateToList/ })).toBeInTheDocument() }) + it('FE-MOB-COLSCR-017b: an editor keeps copy and duplicate but cannot move places out of the list', () => { + mocks.coll = makeHook({ selectMode: true, selectedIds: [10], canEdit: true, canDelete: false, isOwner: false }) + render() + + expect(screen.queryByRole('button', { name: /collections.moveToList/ })).not.toBeInTheDocument() + expect(screen.queryByRole('button', { name: /common.delete/ })).not.toBeInTheDocument() + expect(screen.getByRole('button', { name: /collections.duplicateToList/ })).toBeInTheDocument() + expect(screen.getByRole('button', { name: /collections.copyToTrip/ })).toBeInTheDocument() + }) + it('FE-MOB-COLSCR-018: the list view renders rows, the loading state and both empty states', () => { render() expect(screen.getByText('Elbphilharmonie')).toBeInTheDocument() diff --git a/server/src/nest/collections/collections.mcp.ts b/server/src/nest/collections/collections.mcp.ts index c2f8d2c3d..5efcab16e 100644 --- a/server/src/nest/collections/collections.mcp.ts +++ b/server/src/nest/collections/collections.mcp.ts @@ -304,7 +304,7 @@ export class CollectionsMcp { @Tool({ name: 'update_collection_place', description: - "Update a saved place's name, address, coordinates (lat/lng), description, notes, status, category, links, tags, labels, image, or move it to another collection (set collection_id).", + "Update a saved place's name, address, coordinates (lat/lng), description, notes, status, category, links, tags, labels, image, or move it to another collection (set collection_id). Moving a place out of a list needs the same role there as deleting it (owner or admin).", inputSchema: { placeId: idSchema, ...collectionPlaceUpdateRequestSchema.shape, links: linksInput }, annotations: TOOL_ANNOTATIONS_WRITE, when: collectionsAddonOn, diff --git a/server/src/nest/collections/collections.service.ts b/server/src/nest/collections/collections.service.ts index 9defcfc5a..7f5da39b4 100644 --- a/server/src/nest/collections/collections.service.ts +++ b/server/src/nest/collections/collections.service.ts @@ -198,9 +198,19 @@ export class CollectionsService { /** Delete a place — owner or admin only. */ async assertCanDelete(userId: number, collectionId: number): Promise { + await this.assertOwnerOrAdmin(userId, collectionId, 'Only an admin can delete places from this list'); + } + + /** Move a place out of a list to another one. It leaves the list just as a + * delete would, so it takes the same role. */ + private async assertCanMoveOut(userId: number, collectionId: number): Promise { + await this.assertOwnerOrAdmin(userId, collectionId, 'Only an admin can move places out of this list'); + } + + private async assertOwnerOrAdmin(userId: number, collectionId: number, refusal: string): Promise { const r = await this.roleOf(userId, collectionId); if (r === null) httpError(404, 'Collection not found'); - if (r !== 'owner' && r !== 'admin') httpError(403, 'Only an admin can delete places from this list'); + if (r !== 'owner' && r !== 'admin') httpError(403, refusal); } private async ownerOf(collectionId: number): Promise { @@ -919,6 +929,7 @@ export class CollectionsService { let movedTo: number | null = null; if (body.collection_id !== undefined && body.collection_id !== currentCollection) { + await this.assertCanMoveOut(userId, currentCollection); await this.assertCanEdit(userId, body.collection_id); write.collection_id = body.collection_id; write.owner_id = await this.ownerOf(body.collection_id); diff --git a/server/tests/e2e/collections.e2e.test.ts b/server/tests/e2e/collections.e2e.test.ts index 200eacf38..6fb4dc49c 100644 --- a/server/tests/e2e/collections.e2e.test.ts +++ b/server/tests/e2e/collections.e2e.test.ts @@ -711,6 +711,65 @@ describe('Collections e2e (real auth guard + real service + temp SQLite)', () => ).toBe(400); }); + // ── move out of a shared list ──────────────────────────────────────────── + // Moving a place out removes it from the source list, so it takes the same + // role there as deleting it: an editor is refused, an admin is not. + it('COLLECTIONS-E2E-051: moving a place out of a shared list needs admin, not editor', async () => { + const shared = ( + await request(server) + .post('/api/addons/collections') + .set('Cookie', sessionCookie(ownerId)) + .send({ name: 'Shared moves' }) + ).body; + const editorId = createUser(db as never).user.id; + const adminId = createUser(db as never).user.id; + const join = async (userId: number, role: 'editor' | 'admin') => { + await request(server) + .post('/api/addons/collections/invite') + .set('Cookie', sessionCookie(ownerId)) + .send({ collection_id: shared.id, user_id: userId, role }); + await request(server) + .post('/api/addons/collections/invite/accept') + .set('Cookie', sessionCookie(userId)) + .send({ collection_id: shared.id }); + }; + await join(editorId, 'editor'); + await join(adminId, 'admin'); + const place = ( + await request(server) + .post('/api/addons/collections/places') + .set('Cookie', sessionCookie(ownerId)) + .send({ collection_id: shared.id, name: 'Pergamon' }) + ).body.place; + const editorList = ( + await request(server) + .post('/api/addons/collections') + .set('Cookie', sessionCookie(editorId)) + .send({ name: 'Editor own' }) + ).body; + const adminList = ( + await request(server) + .post('/api/addons/collections') + .set('Cookie', sessionCookie(adminId)) + .send({ name: 'Admin own' }) + ).body; + + const refused = await request(server) + .patch(`/api/addons/collections/places/${place.id}`) + .set('Cookie', sessionCookie(editorId)) + .send({ collection_id: editorList.id }); + expect(refused.status).toBe(403); + expect(refused.body.error).toBe('Only an admin can move places out of this list'); + expect((await findRow(orm, CollectionPlaces, { id: place.id }))?.collection_id).toBe(shared.id); + + const moved = await request(server) + .patch(`/api/addons/collections/places/${place.id}`) + .set('Cookie', sessionCookie(adminId)) + .send({ collection_id: adminList.id }); + expect(moved.status).toBe(200); + expect((await findRow(orm, CollectionPlaces, { id: place.id }))?.collection_id).toBe(adminList.id); + }); + // ── delete ─────────────────────────────────────────────────────────────── it('COLLECTIONS-E2E-050: owner deletes; non-owner member cannot (403)', async () => { const col = ( diff --git a/server/tests/unit/mcp/tools-collections.test.ts b/server/tests/unit/mcp/tools-collections.test.ts index 7b641ac9c..47767ddc9 100644 --- a/server/tests/unit/mcp/tools-collections.test.ts +++ b/server/tests/unit/mcp/tools-collections.test.ts @@ -477,6 +477,24 @@ describe('Tool: update_collection_place / set_collection_place_status / rate_col expect(cleared.place.rating_count).toBe(0); }); }); + + it('update_collection_place refuses an editor moving a place out of a shared list', async () => { + const { user: owner } = createUser(testDb); + const { user: editor } = createUser(testDb); + const shared = await seedCollection(owner.id, 'Shared'); + await addMember(shared, editor.id, 'editor'); + const own = await seedCollection(editor.id, 'Mine'); + const pid = await seedPlace(shared, owner.id, 'Gate'); + await withHarness(editor.id, async (h) => { + const res = await h.client.callTool({ + name: 'update_collection_place', + arguments: { placeId: pid, collection_id: own }, + }); + expect(res.isError).toBe(true); + expect(errorText(res)).toBe('Only an admin can move places out of this list'); + }); + expect((await findRow(orm, CollectionPlaces, { id: pid }))?.collection_id).toBe(shared); + }); }); // --------------------------------------------------------------------------- diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index 5ed845b62..bd0c0cec6 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/server/tests/unit/nest/collections.service.test.ts @@ -461,6 +461,64 @@ describe('status + updatePlace move', () => { await expect(svc.updatePlace(owner.id, p.id, { collection_id: b.id })).rejects.toThrow(); }); + // Moving a place out takes it away from the source list, so the source needs + // the same role as deleting it there (owner or admin), not just edit rights. + it('COLLECTIONS-SVC-018b: an editor of the source list cannot move a place out of it', async () => { + const owner = createUser(testDb).user; + const shared = await svc.createCollection(owner.id, { name: 'Shared' }); + const editor = createUser(testDb).user; + await joinCollection(shared.id, editor.id, 'editor'); + const own = await svc.createCollection(editor.id, { name: 'Mine' }); + const l = await svc.createLabel(owner.id, shared.id, 'Berlin'); + const { place: p } = await svc.savePlace(owner.id, { collection_id: shared.id, name: 'Kept' }); + await svc.updatePlace(owner.id, p.id, { label_ids: [l.id] }); + + await expect(svc.updatePlace(editor.id, p.id, { collection_id: own.id })).rejects.toThrow( + 'Only an admin can move places out of this list', + ); + const row = await findRow(await orm(), CollectionPlaces, { id: p.id }); + expect(row?.collection_id).toBe(shared.id); + expect(row?.owner_id).toBe(owner.id); + expect((await svc.getCollection(owner.id, shared.id)).places.find(x => x.id === p.id)?.label_ids).toEqual([l.id]); + }); + + it('COLLECTIONS-SVC-018c: an admin of the source list may move a place out of it', async () => { + const owner = createUser(testDb).user; + const shared = await svc.createCollection(owner.id, { name: 'Shared' }); + const admin = createUser(testDb).user; + await joinCollection(shared.id, admin.id, 'admin'); + const own = await svc.createCollection(admin.id, { name: 'Mine' }); + const { place: p } = await svc.savePlace(owner.id, { collection_id: shared.id, name: 'Movable' }); + + const moved = await svc.updatePlace(admin.id, p.id, { collection_id: own.id }); + expect(moved.collection_id).toBe(own.id); + }); + + it('COLLECTIONS-SVC-018d: an editor may still edit a place when collection_id names the list it is in', async () => { + const owner = createUser(testDb).user; + const shared = await svc.createCollection(owner.id, { name: 'Shared' }); + const editor = createUser(testDb).user; + await joinCollection(shared.id, editor.id, 'editor'); + const { place: p } = await svc.savePlace(owner.id, { collection_id: shared.id, name: 'Old' }); + + const res = await svc.updatePlace(editor.id, p.id, { collection_id: shared.id, name: 'New' }); + expect(res.name).toBe('New'); + expect(res.collection_id).toBe(shared.id); + }); + + it('COLLECTIONS-SVC-018e: a viewer of the source list still gets the read-only refusal on a move', async () => { + const owner = createUser(testDb).user; + const shared = await svc.createCollection(owner.id, { name: 'Shared' }); + const viewer = createUser(testDb).user; + await joinCollection(shared.id, viewer.id, 'viewer'); + const own = await svc.createCollection(viewer.id, { name: 'Mine' }); + const { place: p } = await svc.savePlace(owner.id, { collection_id: shared.id, name: 'X' }); + + await expect(svc.updatePlace(viewer.id, p.id, { collection_id: own.id })).rejects.toThrow( + 'You have read-only access to this list', + ); + }); + // #1870: the address column was missing from the UPDATE set, so a typo or a // moved restaurant could only be fixed by deleting and re-adding the place. it('COLLECTIONS-SVC-019: updatePlace corrects the address and clears it with null', async () => { diff --git a/wiki/Collections.md b/wiki/Collections.md index e5875c1c1..d21b77e45 100644 --- a/wiki/Collections.md +++ b/wiki/Collections.md @@ -69,7 +69,7 @@ Above the places sit compact filters: by **status**, by **category**, by average - **Select all** the currently filtered places. - **Assign label**: add one or more of the list's labels to every selected place at once. - **Copy to trip**: copies the selected places into any of your trips (carrying their name, description, category, notes, price, coordinates, photo and tags). -- **Move to list** or **Duplicate to list**: the selection goes into another of your lists. +- **Move to list** or **Duplicate to list**: the selection goes into another of your lists. Moving takes the places out of the current list, so in a shared list it needs the same role as deleting them (Admin or owner). - **Delete** the selection. ## Custom labels @@ -94,7 +94,7 @@ When sharing, the owner assigns each member a permission role, and can change it |---|---| | **Viewer** | View the list, cast their own star rating on its places, copy its places into their own trips, and filter by label; no other changes to the list. | | **Editor** *(default)* | Add new places and edit existing ones, and manage + assign the list's labels. | -| **Admin** | Everything an editor can, plus delete places. | +| **Admin** | Everything an editor can, plus delete places and move them out to another list. | The owner always has full control. The owner can also remove a member, and a member can leave a shared list themselves. Permissions are enforced on the server, so a role can only ever do what it is allowed to. diff --git a/wiki/MCP-Addon-Tools.md b/wiki/MCP-Addon-Tools.md index 3fb26f1fb..192d945df 100644 --- a/wiki/MCP-Addon-Tools.md +++ b/wiki/MCP-Addon-Tools.md @@ -107,7 +107,7 @@ Requires `collections:read` or `collections:write` scope. | `reorder_collections` | Reorder the user's collections — pass every collection id in the desired order. | | `save_place_to_collection` | Save a place into a collection from a raw payload. Returns a duplicate marker instead of saving when a similar place already exists, unless `force` is true. | | `save_trip_places_to_collection` | Copy one or more existing trip places into a collection. Duplicates are skipped unless `force` is true. | -| `update_collection_place` | Update a saved place's name, address, coordinates, description, notes, status, category, links, tags, labels, image, or move it to another collection. | +| `update_collection_place` | Update a saved place's name, address, coordinates, description, notes, status, category, links, tags, labels, image, or move it to another collection (moving out of a list needs the owner or admin role there). | | `set_collection_place_status` | Set a saved place's status: `idea`, `want`, or `visited`. | | `set_collection_place_status_from_trip` | Set a status on every saved copy of the given trip places, in every list they are on, for marking places visited after a day out. Ids are trip place ids. Lists you may only read are skipped. Returns how many saved places changed and how many of the trip places were found in at least one list. | | `rate_collection_place` | Set or clear the current user's 1-5 star rating on a saved place. Every member rates independently; pass `null` to remove the vote. |