fix: person management ids validation and permissions (#31902)

fix: person management ids validation and permissions (#31898)

(cherry picked from commit 4408f49d15d5b38eb2fbfa40b68cf697a0e16ca8)

Co-authored-by: Daniel Dietzler <36593685+danieldietzler@users.noreply.github.com>

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": {

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' }),
+        ]),
+      );
+    });
+  });
 });

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',

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 {

server/src/schema/migrations/1790693088454-AddPersonUserTableSharedBySharedWithConstraint.ts

@@ -0,0 +1,9 @@
+import { Kysely, sql } from 'kysely';
+
+export async function up(db: Kysely<any>): Promise<void> {
+  await sql`ALTER TABLE "person_user" ADD CONSTRAINT "person_user_sharedBy_sharedWith_chk" CHECK ("sharedById" != "sharedWithId");`.execute(db);
+}
+
+export async function down(db: Kysely<any>): Promise<void> {
+  await sql`ALTER TABLE "person_user" DROP CONSTRAINT "person_user_sharedBy_sharedWith_chk";`.execute(db);
+}

server/src/schema/migrations/ORDER

@@ -97,3 +97,4 @@
 1789419229196-ConvertUserOAuthIdEmptyStringToNull
 1790587508209-RenameGeoNamesCountries
 1790616293884-PersonSharing
+1790693088454-AddPersonUserTableSharedBySharedWithConstraint

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;

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();
     });
   });
 });

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) {

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');

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 () => {

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);
     });

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