diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 9e525641d..c81a3dbba 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -10128,7 +10128,7 @@ "state": "Stable" } ], - "x-immich-permission": "person.delete", + "x-immich-permission": "person.update", "x-immich-state": "Stable" }, "get": { diff --git a/server/src/controllers/person.controller.spec.ts b/server/src/controllers/person.controller.spec.ts index f8058ddd4..b4af29ae5 100644 --- a/server/src/controllers/person.controller.spec.ts +++ b/server/src/controllers/person.controller.spec.ts @@ -1,9 +1,10 @@ import request from 'supertest'; import { PersonController } from 'src/controllers/person.controller.js'; +import { PersonUserRole } from 'src/dtos/person.dto.js'; import { LoggingRepository } from 'src/repositories/logging.repository.js'; import { PersonService } from 'src/services/person.service.js'; import { errorDto } from 'test/medium/responses.js'; -import { factory } from 'test/small.factory.js'; +import { factory, newUuid } from 'test/small.factory.js'; import { ControllerContext, automock, controllerSetup, mockBaseService } from 'test/utils.js'; describe(PersonController.name, () => { @@ -160,4 +161,21 @@ describe(PersonController.name, () => { expect(service.delete).toHaveBeenCalled(); }); }); + + describe('PUT /people/users', () => { + it('should reject duplicate personIds as well as duplicate userIds', async () => { + const userId = newUuid(); + const personId = newUuid(); + const { status, body } = await request(ctx.getHttpServer()) + .put('/people/users') + .send({ personIds: [personId, personId], sharedWithIds: [userId, userId], role: PersonUserRole.Write }); + expect(status).toBe(400); + expect(body.errors).toEqual( + expect.arrayContaining([ + expect.objectContaining({ path: ['personIds'], message: 'Items must be unique' }), + expect.objectContaining({ path: ['sharedWithIds'], message: 'Items must be unique' }), + ]), + ); + }); + }); }); diff --git a/server/src/controllers/person.controller.ts b/server/src/controllers/person.controller.ts index bd3d7ae1f..ea125e429 100644 --- a/server/src/controllers/person.controller.ts +++ b/server/src/controllers/person.controller.ts @@ -249,7 +249,7 @@ export class PersonController { } @Delete('users') - @Authenticated({ permission: Permission.PersonDelete }) + @Authenticated({ permission: Permission.PersonUpdate }) @Endpoint({ summary: 'Remove users from people', description: 'Remove user access to people', diff --git a/server/src/dtos/person.dto.ts b/server/src/dtos/person.dto.ts index 5bc4d406f..b2de5bc46 100644 --- a/server/src/dtos/person.dto.ts +++ b/server/src/dtos/person.dto.ts @@ -5,14 +5,13 @@ import type { ImageDimensions, MaybeDehydrated } from 'src/types.js'; import { AssetFace, Person, PersonUser, User } from 'src/database.js'; import { HistoryBuilder } from 'src/decorators.js'; import { BulkIdsSchema } from 'src/dtos/asset-ids.response.dto.js'; -import { AuthDto } from 'src/dtos/auth.dto.js'; import { AssetEditActionItem } from 'src/dtos/editing.dto.js'; import { UserResponseSchema, mapUser } from 'src/dtos/user.dto.js'; import { SharingDirectionSchema, SourceTypeSchema } from 'src/enum.js'; import { AssetFaceTable } from 'src/schema/tables/asset-face.table.js'; import { asDateString, asDateTimeString } from 'src/utils/date.js'; import { transformFaceBoundingBox } from 'src/utils/transform.js'; -import { hexColor, stringToBool } from 'src/validation.js'; +import { hexColor, stringToBool, uniqueIds } from 'src/validation.js'; const PersonCreateSchema = z .object({ @@ -223,8 +222,8 @@ const PersonUsersSearchSchema = z const PersonUsersCreateSchema = z .object({ - personIds: z.array(z.uuid()).describe('Person IDs'), - sharedWithIds: z.array(z.uuid()).describe('User IDs that should be given access to the person'), + personIds: uniqueIds.describe('Person IDs'), + sharedWithIds: uniqueIds.describe('User IDs that should be given access to the person'), role: PersonUserRoleSchema.describe('Role that should be applied'), }) .meta({ id: 'PersonUsersCreateDto' }); @@ -334,7 +333,6 @@ function mapFacesWithoutPerson( export function mapFaces( face: AssetFace, - auth: AuthDto, edits?: AssetEditActionItem[], assetDimensions?: ImageDimensions, ): AssetFaceResponseDto { diff --git a/server/src/schema/migrations/1790693088454-AddPersonUserTableSharedBySharedWithConstraint.ts b/server/src/schema/migrations/1790693088454-AddPersonUserTableSharedBySharedWithConstraint.ts new file mode 100644 index 000000000..0c3113c74 --- /dev/null +++ b/server/src/schema/migrations/1790693088454-AddPersonUserTableSharedBySharedWithConstraint.ts @@ -0,0 +1,9 @@ +import { Kysely, sql } from 'kysely'; + +export async function up(db: Kysely): Promise { + await sql`ALTER TABLE "person_user" ADD CONSTRAINT "person_user_sharedBy_sharedWith_chk" CHECK ("sharedById" != "sharedWithId");`.execute(db); +} + +export async function down(db: Kysely): Promise { + await sql`ALTER TABLE "person_user" DROP CONSTRAINT "person_user_sharedBy_sharedWith_chk";`.execute(db); +} diff --git a/server/src/schema/migrations/ORDER b/server/src/schema/migrations/ORDER index 64e7f323e..9f46fc499 100644 --- a/server/src/schema/migrations/ORDER +++ b/server/src/schema/migrations/ORDER @@ -97,3 +97,4 @@ 1789419229196-ConvertUserOAuthIdEmptyStringToNull 1790587508209-RenameGeoNamesCountries 1790616293884-PersonSharing +1790693088454-AddPersonUserTableSharedBySharedWithConstraint diff --git a/server/src/schema/tables/person-user.table.ts b/server/src/schema/tables/person-user.table.ts index b8fb2ec67..baaf4f8c7 100644 --- a/server/src/schema/tables/person-user.table.ts +++ b/server/src/schema/tables/person-user.table.ts @@ -1,3 +1,4 @@ +import { Check } from '@immich/sql-tools'; import { AfterInsertTrigger, Column, @@ -32,6 +33,7 @@ import { UserTable } from 'src/schema/tables/user.table.js'; onUpdate: 'CASCADE', onDelete: 'CASCADE', }) +@Check({ name: 'person_user_sharedBy_sharedWith_chk', expression: `"sharedById" != "sharedWithId"` }) export class PersonUserTable { @PrimaryColumn({ type: 'uuid' }) personGroupId!: string; diff --git a/server/src/services/person.service.spec.ts b/server/src/services/person.service.spec.ts index ee0c8d436..f44e7f9b6 100644 --- a/server/src/services/person.service.spec.ts +++ b/server/src/services/person.service.spec.ts @@ -479,7 +479,7 @@ describe(PersonService.name, () => { mocks.person.getFaces.mockResolvedValue([getForAssetFace(face)]); mocks.asset.getForFaces.mockResolvedValue({ edits: [], ...asset.exifInfo }); await expect(sut.getFacesById(auth, { id: face.assetId })).resolves.toStrictEqual([ - mapFaces(getForAssetFace(face), auth), + mapFaces(getForAssetFace(face)), ]); }); @@ -1444,11 +1444,10 @@ describe(PersonService.name, () => { describe('mapFace', () => { it('should map a face', () => { const user = UserFactory.create(); - const auth = AuthFactory.create({ id: user.id }); const person = PersonFactory.create({ ownerId: user.id }); const face = AssetFaceFactory.from().person(person).build(); - expect(mapFaces(getForAssetFace(face), auth)).toEqual({ + expect(mapFaces(getForAssetFace(face))).toEqual({ boundingBoxX1: 100, boundingBoxX2: 200, boundingBoxY1: 100, @@ -1462,7 +1461,7 @@ describe(PersonService.name, () => { }); it('should not map person if person is null', () => { - expect(mapFaces(getForAssetFace(AssetFaceFactory.create()), AuthFactory.create()).person).toBeNull(); + expect(mapFaces(getForAssetFace(AssetFaceFactory.create())).person).toBeNull(); }); }); }); diff --git a/server/src/services/person.service.ts b/server/src/services/person.service.ts index 13dc96351..d4d6b73fd 100644 --- a/server/src/services/person.service.ts +++ b/server/src/services/person.service.ts @@ -163,7 +163,7 @@ export class PersonService extends BaseService { const asset = await this.assetRepository.getForFaces(dto.id); const assetDimensions = getDimensions(asset); - return faces.map((face) => mapFaces(face, auth, asset.edits, assetDimensions)); + return faces.map((face) => mapFaces(face, asset.edits, assetDimensions)); } async createNewFeaturePhoto(changeFeaturePhoto: PersonId[]) { @@ -829,6 +829,10 @@ export class PersonService extends BaseService { } async addUsersToPeople(auth: AuthDto, dto: PersonUsersCreateDto) { + if (dto.sharedWithIds.includes(auth.user.id)) { + throw new BadRequestException('Cannot share a person with yourself'); + } + await this.requirePersonAccess({ auth, permission: Permission.PersonUpdate, @@ -847,7 +851,7 @@ export class PersonService extends BaseService { for (const sharedWithId of dto.sharedWithIds) { if (!clusterGroupUserIds.has(sharedWithId)) { - continue; + throw new BadRequestException('All users must be in the same cluster group'); } for (const personGroupId of dto.personIds) { diff --git a/server/src/validation.ts b/server/src/validation.ts index 7c8121578..3a90fd4cc 100644 --- a/server/src/validation.ts +++ b/server/src/validation.ts @@ -248,3 +248,5 @@ export const hexColor = z .transform((val) => (val.startsWith('#') ? val : `#${val}`)); export const sanitizeFilename = z.string().transform((val) => sanitize(val.replaceAll('.', ''))); + +export const uniqueIds = z.array(z.uuid()).refine((ids) => ids.length === new Set(ids).size, 'Items must be unique'); diff --git a/server/test/medium/specs/repositories/person-user.repository.spec.ts b/server/test/medium/specs/repositories/person-user.repository.spec.ts index 58ce21c7e..0413a8285 100644 --- a/server/test/medium/specs/repositories/person-user.repository.spec.ts +++ b/server/test/medium/specs/repositories/person-user.repository.spec.ts @@ -70,6 +70,27 @@ const newShares = async () => { }; describe(PersonUserRepository.name, () => { + describe('table constraints', () => { + it('should reject a row with sharedWithId = sharedById', async () => { + const { ctx } = setup(); + const { user } = await ctx.newUser(); + const { person } = await ctx.newPerson({ ownerId: user.id }); + await expect( + ctx.database + .insertInto('person_user') + .values({ + personGroupId: person.personGroupId, + sharedById: user.id, + sharedWithId: user.id, + role: PersonUserRole.Write, + }) + .execute(), + ).rejects.toThrow( + 'new row for relation "person_user" violates check constraint "person_user_sharedBy_sharedWith_chk"', + ); + }); + }); + describe('createAll', () => { describe(person_user_after_insert.name, () => { it('should create automatically person for the user it was shared with', async () => { diff --git a/server/test/medium/specs/services/person.service.spec.ts b/server/test/medium/specs/services/person.service.spec.ts index e76582f0b..6d5afdda6 100644 --- a/server/test/medium/specs/services/person.service.spec.ts +++ b/server/test/medium/specs/services/person.service.spec.ts @@ -1245,18 +1245,20 @@ describe(PersonService.name, () => { }); describe('addUsersToPeople', () => { - it('should skip sharedWith users that are not in the same cluster group', async () => { + it('should throw error for sharedWith users that are not in the same cluster group', async () => { const { sut, ctx } = setup(); const { user: owner } = await ctx.newUser(); const { user: user1 } = await ctx.newUser(); const { person } = await ctx.newPerson({ ownerId: owner.id }); const auth = factory.auth({ user: owner }); - await sut.addUsersToPeople(auth, { - personIds: [person.personGroupId], - sharedWithIds: [user1.id], - role: PersonUserRole.Read, - }); + await expect( + sut.addUsersToPeople(auth, { + personIds: [person.personGroupId], + sharedWithIds: [user1.id], + role: PersonUserRole.Read, + }), + ).rejects.toThrow('All users must be in the same cluster group'); await expect(sut.getUsersForPeople(auth, {})).resolves.toHaveLength(0); }); diff --git a/server/test/utils.ts b/server/test/utils.ts index fe9a33603..9d2f35b08 100644 --- a/server/test/utils.ts +++ b/server/test/utils.ts @@ -140,7 +140,10 @@ export const controllerSetup = async (controller: ControllerClass | ControllerCl .overrideInterceptor(AssetUploadInterceptor) .useValue(noopInterceptor) .compile(); - const app = moduleRef.createNestApplication(); + const app = moduleRef.createNestApplication({ + routeConflictPolicy: { duplicate: 'error' }, + routeResolutionStrategy: 'specificity', + }); await app.init(); // allow the AuthController to override the AuthService itself