From aec7c0d97bd6679b2b300e72e870b83c5ef13c85 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Tue, 29 Sep 2026 18:12:25 -0400 Subject: [PATCH] feat: auto-sync name changes to connected users (#31923) * feat: auto-sync name changes to connected users * fix: rename to faceId * fix: pr feedback * fix: mobile build --- i18n/en.json | 4 + mobile/lib/utils/openapi_patching.dart | 3 +- open-api/immich-openapi-specs.json | 21 +++- packages/sdk/src/fetch-client.ts | 10 +- .../src/controllers/person.controller.spec.ts | 17 +++ server/src/dtos/person.dto.ts | 13 +- server/src/dtos/user-preferences.dto.ts | 4 +- server/src/enum.ts | 10 ++ server/src/queries/person.repository.sql | 17 +++ server/src/repositories/person.repository.ts | 20 +++ server/src/services/person.service.spec.ts | 106 ++++++++++++++-- server/src/services/person.service.ts | 27 ++-- server/src/types.ts | 2 + server/src/utils/misc.ts | 2 + server/src/utils/preferences.ts | 3 +- .../specs/services/person.service.spec.ts | 117 ++++++++++++++++++ .../specs/services/user.service.spec.ts | 16 ++- web/src/lib/modals/PersonEditModal.svelte | 24 ++-- web/src/lib/services/person.service.ts | 59 ++++----- web/src/routes/(user)/people/+page.svelte | 25 +--- .../[[assetId=id]]/+page.svelte | 9 +- .../user-settings/FeatureSettings.svelte | 19 ++- .../factories/preferences-factory.ts | 3 +- 23 files changed, 416 insertions(+), 115 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index 532f675902..fc9c03338e 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1679,6 +1679,10 @@ "person_read_access_message": "You only have read access to this person.", "person_recognized": "Person recognized", "person_share_no_users": "Every user in your cluster group already has access to this person.", + "person_update_strategy": "Apply changes to", + "person_update_strategy_description": "Which people to update when you change a name or date of birth", + "person_update_strategy_everyone": "Everyone with shared access", + "person_update_strategy_self": "Only me", "photo_shared_all_users": "Looks like you shared your photos with all users or you don't have any user to share with.", "photos": "Photos", "photos_and_videos": "Photos & Videos", diff --git a/mobile/lib/utils/openapi_patching.dart b/mobile/lib/utils/openapi_patching.dart index 1ac6aa1740..cca48c8b2d 100644 --- a/mobile/lib/utils/openapi_patching.dart +++ b/mobile/lib/utils/openapi_patching.dart @@ -21,7 +21,7 @@ final Map> openApiPatches = { 'folders': FoldersResponse(enabled: false, sidebarWeb: false).toJson(), 'memories': MemoriesResponse(enabled: true, duration: 5, sidebarWeb: false).toJson(), 'ratings': RatingsResponse(enabled: false).toJson(), - 'people': PeopleResponse(enabled: true, sidebarWeb: false).toJson(), + 'people': PeopleResponse(enabled: true, sidebarWeb: false, updateStrategy: PersonUpdateStrategy.everyone).toJson(), 'tags': TagsResponse(enabled: false, sidebarWeb: false).toJson(), 'sharedLinks': SharedLinksResponse(enabled: true, sidebarWeb: false).toJson(), 'cast': CastResponse(gCastEnabled: false).toJson(), @@ -42,6 +42,7 @@ final Map> openApiPatches = { 'ServerFeaturesDto': {'ocr': false, 'realtimeTranscoding': false}, 'SearchAssetResponseDto': {'nextCursor': null}, 'MemoriesResponse': {'duration': 5, 'sidebarWeb': false}, + 'PeopleResponse': {'updateStrategy': 'everyone'}, 'PersonResponseDto': {'otherPeople': const [], 'sharedBy': const [], 'sharedWith': const []}, 'WorkflowResponseDto': {'logging': false}, }; diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index c81a3dbba1..f0e2ee24b1 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -24944,11 +24944,15 @@ "sidebarWeb": { "description": "Whether people appear in web sidebar", "type": "boolean" + }, + "updateStrategy": { + "$ref": "#/components/schemas/PersonUpdateStrategy" } }, "required": [ "enabled", - "sidebarWeb" + "sidebarWeb", + "updateStrategy" ], "type": "object" }, @@ -25011,6 +25015,9 @@ "sidebarWeb": { "description": "Whether people appear in web sidebar", "type": "boolean" + }, + "updateStrategy": { + "$ref": "#/components/schemas/PersonUpdateStrategy" } }, "type": "object" @@ -25069,7 +25076,7 @@ "type": "string" }, "userId": { - "description": "User ID", + "description": "Restrict the update to the person record of this User ID", "format": "uuid", "pattern": "^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-8][0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}|00000000-0000-0000-0000-000000000000|ffffffff-ffff-ffff-ffff-ffffffffffff)$", "type": "string" @@ -25527,7 +25534,7 @@ "type": "string" }, "userId": { - "description": "User ID", + "description": "Restrict the update to the person record of this User ID", "format": "uuid", "pattern": "^([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-8][0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}|00000000-0000-0000-0000-000000000000|ffffffff-ffff-ffff-ffff-ffffffffffff)$", "type": "string" @@ -25535,6 +25542,14 @@ }, "type": "object" }, + "PersonUpdateStrategy": { + "description": "Which person records to update when editing a person", + "enum": [ + "self", + "everyone" + ], + "type": "string" + }, "PersonUserRole": { "description": "Levels of access for managing people resources on behalf of another user.", "enum": [ diff --git a/packages/sdk/src/fetch-client.ts b/packages/sdk/src/fetch-client.ts index 1b3e029b91..b8439e823f 100644 --- a/packages/sdk/src/fetch-client.ts +++ b/packages/sdk/src/fetch-client.ts @@ -688,6 +688,7 @@ export type PeopleResponse = { minimumFaces?: number; /** Whether people appear in web sidebar */ sidebarWeb: boolean; + updateStrategy: PersonUpdateStrategy; }; export type PurchaseResponse = { /** Date until which to hide buy button */ @@ -774,6 +775,7 @@ export type PeopleUpdate = { minimumFaces?: number; /** Whether people appear in web sidebar */ sidebarWeb?: boolean; + updateStrategy?: PersonUpdateStrategy; }; export type PurchaseUpdate = { /** Date until which to hide buy button */ @@ -2033,7 +2035,7 @@ export type PeopleUpdateItem = { isHidden?: boolean; /** Person name */ name?: string; - /** User ID */ + /** Restrict the update to the person record of this User ID */ userId?: string; }; export type PeopleUpdateDto = { @@ -2090,7 +2092,7 @@ export type PersonUpdateDto = { isHidden?: boolean; /** Person name */ name?: string; - /** User ID */ + /** Restrict the update to the person record of this User ID */ userId?: string; }; export type AssetFaceUpdateItem = { @@ -8061,6 +8063,10 @@ export enum AssetOrder { Asc = "asc", Desc = "desc" } +export enum PersonUpdateStrategy { + Self = "self", + Everyone = "everyone" +} export enum AssetVisibility { Archive = "archive", Timeline = "timeline", diff --git a/server/src/controllers/person.controller.spec.ts b/server/src/controllers/person.controller.spec.ts index b4af29ae54..3d798d67c5 100644 --- a/server/src/controllers/person.controller.spec.ts +++ b/server/src/controllers/person.controller.spec.ts @@ -91,6 +91,23 @@ describe(PersonController.name, () => { expect(body).toEqual(errorDto.validationError([{ path: ['featureFaceAssetId'], message: 'Invalid UUID' }])); }); + it('should require at least one property to update', async () => { + const { status, body } = await request(ctx.getHttpServer()) + .put(`/people/${factory.uuid()}`) + .send({ userId: factory.uuid() }) + .set('Authorization', `Bearer token`); + expect(status).toBe(400); + expect(body).toEqual( + errorDto.validationError([ + { + path: [], + message: + 'At least one of the following fields is required: name, birthDate, isHidden, isFavorite, color, featureFaceAssetId', + }, + ]), + ); + }); + it(`should require isFavorite to be a boolean`, async () => { const { status, body } = await request(ctx.getHttpServer()) .put(`/people/${factory.uuid()}`) diff --git a/server/src/dtos/person.dto.ts b/server/src/dtos/person.dto.ts index 19a5d145f3..6f61ce1c74 100644 --- a/server/src/dtos/person.dto.ts +++ b/server/src/dtos/person.dto.ts @@ -29,10 +29,17 @@ const PersonCreateSchema = z }) .meta({ id: 'PersonCreateDto' }); -const PersonUpdateSchema = PersonCreateSchema.extend({ +const PersonUpdateBaseSchema = PersonCreateSchema.extend({ featureFaceAssetId: z.uuidv4().optional().describe('Asset ID used for feature face thumbnail'), - userId: z.uuid().optional().describe('User ID'), -}).meta({ id: 'PersonUpdateDto' }); +}); + +const PersonUpdateSchema = PersonUpdateBaseSchema.extend({ + userId: z.uuid().optional().describe('Restrict the update to the person record of this User ID'), +}) + .refine((dto) => Object.entries(dto).some(([key, value]) => key !== 'userId' && value !== undefined), { + message: `At least one of the following fields is required: ${Object.keys(PersonUpdateBaseSchema.shape).join(', ')}`, + }) + .meta({ id: 'PersonUpdateDto' }); const PeopleUpdateItemSchema = PersonUpdateSchema.extend({ id: z.uuidv4().describe('Person ID'), diff --git a/server/src/dtos/user-preferences.dto.ts b/server/src/dtos/user-preferences.dto.ts index 57dd33d046..341f524f0c 100644 --- a/server/src/dtos/user-preferences.dto.ts +++ b/server/src/dtos/user-preferences.dto.ts @@ -1,7 +1,7 @@ import { createZodDto } from 'nestjs-zod'; import z from 'zod'; import type { UserPreferences } from 'src/types.js'; -import { AssetOrderSchema, UserAvatarColorSchema } from 'src/enum.js'; +import { AssetOrderSchema, PersonUpdateStrategySchema, UserAvatarColorSchema } from 'src/enum.js'; const AlbumsUpdateSchema = z .object({ @@ -47,6 +47,7 @@ const PeopleUpdateSchema = z enabled: z.boolean().optional().describe('Whether people are enabled'), sidebarWeb: z.boolean().optional().describe('Whether people appear in web sidebar'), minimumFaces: z.int().min(1).optional().describe('People face threshold'), + updateStrategy: PersonUpdateStrategySchema.optional(), }) .optional() .meta({ id: 'PeopleUpdate' }); @@ -150,6 +151,7 @@ const PeopleResponseSchema = z enabled: z.boolean().describe('Whether people are enabled'), sidebarWeb: z.boolean().describe('Whether people appear in web sidebar'), minimumFaces: z.int().min(1).optional().describe('People face threshold'), + updateStrategy: PersonUpdateStrategySchema, }) .meta({ id: 'PeopleResponse' }); diff --git a/server/src/enum.ts b/server/src/enum.ts index 6dd5ee36c0..41178f4e40 100644 --- a/server/src/enum.ts +++ b/server/src/enum.ts @@ -417,6 +417,16 @@ export const UserAvatarColorSchema = z .describe('User avatar color') .meta({ id: 'UserAvatarColor' }); +export enum PersonUpdateStrategy { + Self = 'self', + Everyone = 'everyone', +} + +export const PersonUpdateStrategySchema = z + .enum(PersonUpdateStrategy) + .describe('Which person records to update when editing a person') + .meta({ id: 'PersonUpdateStrategy' }); + export enum UserStatus { Active = 'active', Removing = 'removing', diff --git a/server/src/queries/person.repository.sql b/server/src/queries/person.repository.sql index 08e76d68c9..b9d42755e3 100644 --- a/server/src/queries/person.repository.sql +++ b/server/src/queries/person.repository.sql @@ -1116,6 +1116,23 @@ from 1 ) as "dummy" +-- PersonRepository.updateForWritableOwners +update "person" +set + "name" = $1 +where + "person"."personGroupId" = $2 + and "person"."ownerId" in ( + select + "person_user"."sharedById" + from + "person_user" + where + "person_user"."personGroupId" = $3 + and "person_user"."sharedWithId" = $4 + and "person_user"."role" in ($5, $6) + ) + -- PersonRepository.getFacesByIds select "asset_face".*, diff --git a/server/src/repositories/person.repository.ts b/server/src/repositories/person.repository.ts index 6f0c6bffc4..01b0306977 100644 --- a/server/src/repositories/person.repository.ts +++ b/server/src/repositories/person.repository.ts @@ -933,6 +933,26 @@ export class PersonRepository { .executeTakeFirstOrThrow(); } + @GenerateSql({ params: [{ userId: DummyValue.UUID, personGroupId: DummyValue.UUID }, { name: DummyValue.STRING }] }) + async updateForWritableOwners( + { userId, personGroupId }: { userId: string; personGroupId: string }, + person: Pick, 'name' | 'birthDate'>, + ): Promise { + await this.db + .updateTable('person') + .set(person) + .where('person.personGroupId', '=', personGroupId) + .where('person.ownerId', 'in', (eb) => + eb + .selectFrom('person_user') + .select('person_user.sharedById') + .where('person_user.personGroupId', '=', personGroupId) + .where('person_user.sharedWithId', '=', userId) + .where('person_user.role', 'in', [PersonUserRole.Write, PersonUserRole.Admin]), + ) + .execute(); + } + async updateAll(people: Insertable[]): Promise { if (people.length === 0) { return; diff --git a/server/src/services/person.service.spec.ts b/server/src/services/person.service.spec.ts index f44e7f9b6a..6791d37130 100644 --- a/server/src/services/person.service.spec.ts +++ b/server/src/services/person.service.spec.ts @@ -122,8 +122,8 @@ describe(PersonService.name, () => { const person = PersonFactory.create(); const ids = [{ personGroupId: person.personGroupId, ownerId: auth.user.id }]; - mocks.person.getForUser.mockResolvedValue(getDehydrated(person)); mocks.access.person.checkAccess.mockResolvedValue(new Set(ids)); + mocks.person.getForUser.mockResolvedValue(getDehydrated(person)); await expect(sut.getById(auth, person.personGroupId)).resolves.toEqual( expect.objectContaining({ id: person.personGroupId }), ); @@ -221,7 +221,7 @@ describe(PersonService.name, () => { const auth = AuthFactory.create(); const person = PersonFactory.create({ ownerId: auth.user.id }); - await expect(sut.update(auth, person.personGroupId, { name: 'Person 1' })).rejects.toBeInstanceOf( + await expect(sut.update(auth, person.personGroupId, { isHidden: true })).rejects.toBeInstanceOf( BadRequestException, ); expect(mocks.person.update).not.toHaveBeenCalled(); @@ -237,28 +237,46 @@ describe(PersonService.name, () => { mocks.access.person.checkAccess.mockResolvedValue(new Set()); await expect(sut.update(auth, 'person-1', { name: 'Person 1' })).rejects.toBeInstanceOf(BadRequestException); - expect(mocks.person.update).not.toHaveBeenCalled(); expect(mocks.access.person.checkAccess).toHaveBeenCalledWith( auth.user.id, new Set([{ personGroupId: 'person-1', ownerId: auth.user.id }]), PERSON_WRITE_ROLES, ); + expect(mocks.person.updateForWritableOwners).not.toHaveBeenCalled(); }); - it("should update a person's name", async () => { + it('should throw an error when personal properties are updated for another user', async () => { + const auth = AuthFactory.create(); + const person = PersonFactory.create(); + + await expect( + sut.update(auth, person.personGroupId, { userId: person.ownerId, name: 'Person 1', isFavorite: true }), + ).rejects.toThrow('Only name and birthDate can be updated for other users'); + expect(mocks.access.person.checkAccess).not.toHaveBeenCalled(); + expect(mocks.person.update).not.toHaveBeenCalled(); + expect(mocks.person.updateForWritableOwners).not.toHaveBeenCalled(); + }); + + it("should update a person's name for every user with write access", async () => { const auth = AuthFactory.create(); const person = PersonFactory.create({ ownerId: auth.user.id, name: 'Person 1' }); const ids = [{ personGroupId: person.personGroupId, ownerId: auth.user.id }]; - mocks.person.update.mockResolvedValue(person); mocks.access.person.checkAccess.mockResolvedValue(new Set(ids)); + mocks.person.update.mockResolvedValue(person); await expect(sut.update(auth, person.personGroupId, { name: 'Person 1' })).resolves.toEqual( expect.objectContaining({ id: person.personGroupId, name: 'Person 1' }), ); + expect(mocks.person.updateForWritableOwners).toHaveBeenCalledWith( + { userId: auth.user.id, personGroupId: person.personGroupId }, + { + name: 'Person 1', + }, + ); expect(mocks.person.update).toHaveBeenCalledWith({ - ownerId: person.ownerId, + ownerId: auth.user.id, personGroupId: person.personGroupId, name: 'Person 1', }); @@ -282,17 +300,41 @@ describe(PersonService.name, () => { personGroupId: person.personGroupId, name: 'Person 1', }); + expect(mocks.person.updateForWritableOwners).not.toHaveBeenCalled(); expect(mocks.access.person.checkAccess).toHaveBeenCalledWith(auth.user.id, new Set(ids), PERSON_WRITE_ROLES); }); - it("should update a person's date of birth", async () => { + it("should only update the user's own person when userId is the current user", async () => { const auth = AuthFactory.create(); - const person = PersonFactory.create({ ownerId: auth.user.id, birthDate: new Date('1976-06-30') }); + const person = PersonFactory.create({ ownerId: auth.user.id, name: 'Person 1' }); const ids = [{ personGroupId: person.personGroupId, ownerId: auth.user.id }]; mocks.person.update.mockResolvedValue(person); mocks.access.person.checkAccess.mockResolvedValue(new Set(ids)); + await expect( + sut.update(auth, person.personGroupId, { name: 'Person 1', isFavorite: true, userId: auth.user.id }), + ).resolves.toEqual(expect.objectContaining({ id: person.personGroupId, name: 'Person 1' })); + + expect(mocks.person.update).toHaveBeenCalledWith({ + ownerId: auth.user.id, + personGroupId: person.personGroupId, + name: 'Person 1', + isFavorite: true, + }); + expect(mocks.person.updateForWritableOwners).not.toHaveBeenCalled(); + expect(mocks.access.person.checkAccess).toHaveBeenCalledWith(auth.user.id, new Set(ids), PERSON_WRITE_ROLES); + }); + + it("should update a person's date of birth for every user with write access", async () => { + const auth = AuthFactory.create(); + const person = PersonFactory.create({ ownerId: auth.user.id, birthDate: new Date('1976-06-30') }); + + mocks.access.person.checkAccess.mockResolvedValue( + new Set([{ personGroupId: person.personGroupId, ownerId: auth.user.id }]), + ); + mocks.person.update.mockResolvedValue(person); + await expect(sut.update(auth, person.personGroupId, { birthDate: '1976-06-30' })).resolves.toEqual({ id: person.personGroupId, name: person.name, @@ -305,14 +347,19 @@ describe(PersonService.name, () => { sharedWith: [], updatedAt: expect.any(String), }); + expect(mocks.person.updateForWritableOwners).toHaveBeenCalledWith( + { userId: auth.user.id, personGroupId: person.personGroupId }, + { + birthDate: '1976-06-30', + }, + ); expect(mocks.person.update).toHaveBeenCalledWith({ - ownerId: person.ownerId, + ownerId: auth.user.id, personGroupId: person.personGroupId, birthDate: '1976-06-30', }); expect(mocks.job.queue).not.toHaveBeenCalled(); expect(mocks.job.queueAll).not.toHaveBeenCalled(); - expect(mocks.access.person.checkAccess).toHaveBeenCalledWith(auth.user.id, new Set(ids), PERSON_WRITE_ROLES); }); it('should update a person visibility', async () => { @@ -332,6 +379,7 @@ describe(PersonService.name, () => { personGroupId: person.personGroupId, isHidden: true, }); + expect(mocks.person.updateForWritableOwners).not.toHaveBeenCalled(); expect(mocks.access.person.checkAccess).toHaveBeenCalledWith(auth.user.id, new Set(ids), PERSON_WRITE_ROLES); }); @@ -352,9 +400,42 @@ describe(PersonService.name, () => { personGroupId: person.personGroupId, isFavorite: true, }); + expect(mocks.person.updateForWritableOwners).not.toHaveBeenCalled(); expect(mocks.access.person.checkAccess).toHaveBeenCalledWith(auth.user.id, new Set(ids), PERSON_WRITE_ROLES); }); + it('should update shared and personal properties together', async () => { + const auth = AuthFactory.create(); + const person = PersonFactory.create({ ownerId: auth.user.id, name: 'Person 1', isFavorite: true }); + + mocks.access.person.checkAccess.mockResolvedValue( + new Set([{ personGroupId: person.personGroupId, ownerId: auth.user.id }]), + ); + mocks.person.update.mockResolvedValue(person); + + await expect(sut.update(auth, person.personGroupId, { name: 'Person 1', isFavorite: true })).resolves.toEqual( + expect.objectContaining({ name: 'Person 1', isFavorite: true }), + ); + + expect(mocks.person.updateForWritableOwners).toHaveBeenCalledWith( + { userId: auth.user.id, personGroupId: person.personGroupId }, + { + name: 'Person 1', + }, + ); + expect(mocks.person.update).toHaveBeenCalledWith({ + ownerId: auth.user.id, + personGroupId: person.personGroupId, + name: 'Person 1', + isFavorite: true, + }); + expect(mocks.access.person.checkAccess).toHaveBeenCalledWith( + auth.user.id, + new Set([{ personGroupId: person.personGroupId, ownerId: auth.user.id }]), + PERSON_WRITE_ROLES, + ); + }); + it("should update a person's thumbnailPath", async () => { const face = AssetFaceFactory.create(); const auth = AuthFactory.create(); @@ -389,9 +470,7 @@ describe(PersonService.name, () => { it('should throw an error when the face feature assetId is invalid', async () => { const auth = AuthFactory.create(); const person = PersonFactory.create({ ownerId: auth.user.id }); - const ids = [{ personGroupId: person.personGroupId, ownerId: auth.user.id }]; - mocks.access.person.checkAccess.mockResolvedValue(new Set(ids)); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['-1'])); mocks.person.getForFeatureFaceUpdate.mockResolvedValue(undefined); @@ -399,7 +478,7 @@ describe(PersonService.name, () => { BadRequestException, ); expect(mocks.person.update).not.toHaveBeenCalled(); - expect(mocks.access.person.checkAccess).toHaveBeenCalledWith(auth.user.id, new Set(ids), PERSON_WRITE_ROLES); + expect(mocks.person.updateForWritableOwners).not.toHaveBeenCalled(); }); }); @@ -411,6 +490,7 @@ describe(PersonService.name, () => { { error: BulkIdErrorReason.UNKNOWN, id: 'person-1', success: false }, ]); expect(mocks.person.update).not.toHaveBeenCalled(); + expect(mocks.person.updateForWritableOwners).not.toHaveBeenCalled(); expect(mocks.access.person.checkAccess).toHaveBeenCalledWith( authStub.admin.user.id, new Set([{ personGroupId: 'person-1', ownerId: authStub.admin.user.id }]), diff --git a/server/src/services/person.service.ts b/server/src/services/person.service.ts index d4d6b73fd4..2b616a9d7d 100644 --- a/server/src/services/person.service.ts +++ b/server/src/services/person.service.ts @@ -52,7 +52,7 @@ import { getDimensions, getMyPartnerIds } from 'src/utils/asset.util.js'; import { ImmichFileResponse } from 'src/utils/file.js'; import { isHttpException } from 'src/utils/logger.js'; import { mimeTypes } from 'src/utils/mime-types.js'; -import { batched, findOrFail, isFacialRecognitionEnabled } from 'src/utils/misc.js'; +import { batched, findOrFail, hasSomeDefined, isFacialRecognitionEnabled } from 'src/utils/misc.js'; import { Point, transformPoints } from 'src/utils/transform.js'; const personKey = ({ ownerId, personGroupId }: PersonId) => `${ownerId}/${personGroupId}`; @@ -247,17 +247,27 @@ export class PersonService extends BaseService { } async update(auth: AuthDto, personGroupId: string, dto: PersonUpdateDto): Promise { - const ownerId = dto.userId ?? auth.user.id; + const { userId: userIdOverride, name, birthDate, isHidden, featureFaceAssetId: assetId, isFavorite, color } = dto; + const targetOwnerId = userIdOverride ?? auth.user.id; + const hasSharedProperties = hasSomeDefined([name, birthDate]); + const hasPersonalProperties = hasSomeDefined([isHidden, isFavorite, color, assetId]); + + if (targetOwnerId !== auth.user.id && hasPersonalProperties) { + throw new BadRequestException('Only name and birthDate can be updated for other users'); + } await this.requirePersonAccess({ auth, permission: Permission.PersonUpdate, - ids: [{ personGroupId, ownerId }], + ids: [{ personGroupId, ownerId: targetOwnerId }], }); - const { name, birthDate, isHidden, featureFaceAssetId: assetId, isFavorite, color } = dto; - // TODO: set by faceId directly + if (!userIdOverride && hasSharedProperties) { + await this.personRepository.updateForWritableOwners({ userId: auth.user.id, personGroupId }, { name, birthDate }); + } + let faceId: string | undefined; + if (assetId) { await this.requireAccess({ auth, permission: Permission.AssetRead, ids: [assetId] }); const face = await this.personRepository.getForFeatureFaceUpdate({ personGroupId, assetId }); @@ -269,7 +279,7 @@ export class PersonService extends BaseService { } const person = await this.personRepository.update({ - ownerId, + ownerId: targetOwnerId, personGroupId, faceAssetId: faceId, name, @@ -280,7 +290,10 @@ export class PersonService extends BaseService { }); if (assetId) { - await this.jobRepository.queue({ name: JobName.PersonGenerateThumbnail, data: { ownerId, personGroupId } }); + await this.jobRepository.queue({ + name: JobName.PersonGenerateThumbnail, + data: { ownerId: targetOwnerId, personGroupId }, + }); } return mapPerson(person); diff --git a/server/src/types.ts b/server/src/types.ts index b8036a5de9..194b6cef99 100644 --- a/server/src/types.ts +++ b/server/src/types.ts @@ -24,6 +24,7 @@ import { IntegrityReport, JobName, MemoryType, + PersonUpdateStrategy, QueueName, StorageFolder, SyncEntityType, @@ -580,6 +581,7 @@ export type UserPreferences = { enabled: boolean; sidebarWeb: boolean; minimumFaces: number; + updateStrategy: PersonUpdateStrategy; }; ratings: { enabled: boolean; diff --git a/server/src/utils/misc.ts b/server/src/utils/misc.ts index fbb64c53a8..dd2b5763bc 100644 --- a/server/src/utils/misc.ts +++ b/server/src/utils/misc.ts @@ -108,6 +108,8 @@ export const handlePromiseError = (promise: Promise, logger: LoggingReposi promise.catch((error: Error | any) => logger.error(`Promise error: ${error}`, error?.stack)); }; +export const hasSomeDefined = (values: unknown[]) => values.some((value) => value !== undefined); + export const findOrFail = async (find: () => Promise, entity: string): Promise> => { const value = await find(); if (!value) { diff --git a/server/src/utils/preferences.ts b/server/src/utils/preferences.ts index 1f363de868..4ecc324a4d 100644 --- a/server/src/utils/preferences.ts +++ b/server/src/utils/preferences.ts @@ -1,7 +1,7 @@ import { get, isEqual, set } from 'lodash-es'; import type { DeepPartial, UserMetadataItem, UserPreferences } from 'src/types.js'; import { UserPreferencesUpdateDto } from 'src/dtos/user-preferences.dto.js'; -import { AssetOrder, UserMetadataKey } from 'src/enum.js'; +import { AssetOrder, PersonUpdateStrategy, UserMetadataKey } from 'src/enum.js'; import { HumanReadableSize } from 'src/utils/bytes.js'; import { getKeysDeep } from 'src/utils/misc.js'; @@ -23,6 +23,7 @@ const getDefaultPreferences = (): UserPreferences => { enabled: true, sidebarWeb: false, minimumFaces: 3, + updateStrategy: PersonUpdateStrategy.Everyone, }, sharedLinks: { enabled: true, diff --git a/server/test/medium/specs/services/person.service.spec.ts b/server/test/medium/specs/services/person.service.spec.ts index 6d5afdda64..212b725480 100644 --- a/server/test/medium/specs/services/person.service.spec.ts +++ b/server/test/medium/specs/services/person.service.spec.ts @@ -303,6 +303,123 @@ describe(PersonService.name, () => { }); }); + describe('update', () => { + it('should throw an error when there is no access', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + const { user: user2 } = await ctx.newUser(); + const { person } = await ctx.newPerson({ ownerId: user2.id }); + + await expect(sut.update(factory.auth({ user }), person.personGroupId, { name: 'New name' })).rejects.toThrow( + 'Not found or no person.update access', + ); + }); + + it('should update the name and birth date for every user with write access', async () => { + const { ctx, sut } = setup(); + const personRepo = ctx.get(PersonRepository); + const { user } = await ctx.newUser(); + const { user: writer } = await ctx.newUser(); + const { user: reader } = await ctx.newUser(); + const { person } = await ctx.newPerson({ ownerId: writer.id, name: 'Old name' }); + const { personGroupId } = person; + await ctx.newPerson({ ownerId: reader.id, personGroupId, name: 'Old name' }); + await ctx.newPersonUser({ + personGroupId, + sharedById: writer.id, + sharedWithId: user.id, + role: PersonUserRole.Write, + }); + await ctx.newPersonUser({ + personGroupId, + sharedById: reader.id, + sharedWithId: user.id, + role: PersonUserRole.Read, + }); + + await expect( + sut.update(factory.auth({ user }), personGroupId, { name: 'New name', birthDate: '2000-01-01' }), + ).resolves.toEqual(expect.objectContaining({ name: 'New name', birthDate: '2000-01-01' })); + + await expect(personRepo.getForUser({ userId: user.id, personGroupId })).resolves.toEqual( + expect.objectContaining({ name: 'New name', birthDate: '2000-01-01' }), + ); + await expect(personRepo.getForUser({ userId: writer.id, personGroupId })).resolves.toEqual( + expect.objectContaining({ name: 'New name', birthDate: '2000-01-01' }), + ); + await expect(personRepo.getForUser({ userId: reader.id, personGroupId })).resolves.toEqual( + expect.objectContaining({ name: 'Old name', birthDate: null }), + ); + }); + + it('should only update the specified user when userId is provided', async () => { + const { ctx, sut } = setup(); + const personRepo = ctx.get(PersonRepository); + const { user } = await ctx.newUser(); + const { user: writer } = await ctx.newUser(); + const { person } = await ctx.newPerson({ ownerId: writer.id, name: 'Old name' }); + const { personGroupId } = person; + await ctx.newPersonUser({ + personGroupId, + sharedById: writer.id, + sharedWithId: user.id, + role: PersonUserRole.Write, + }); + + await expect( + sut.update(factory.auth({ user }), personGroupId, { name: 'New name', userId: writer.id }), + ).resolves.toEqual(expect.objectContaining({ name: 'New name' })); + + await expect(personRepo.getForUser({ userId: writer.id, personGroupId })).resolves.toEqual( + expect.objectContaining({ name: 'New name' }), + ); + await expect(personRepo.getForUser({ userId: user.id, personGroupId })).resolves.toEqual( + expect.objectContaining({ name: 'Old name' }), + ); + }); + + it('should only update personal properties for the current user', async () => { + const { ctx, sut } = setup(); + const personRepo = ctx.get(PersonRepository); + const { user } = await ctx.newUser(); + const { user: writer } = await ctx.newUser(); + const { person } = await ctx.newPerson({ ownerId: writer.id, name: 'Old name' }); + const { personGroupId } = person; + await ctx.newPersonUser({ + personGroupId, + sharedById: writer.id, + sharedWithId: user.id, + role: PersonUserRole.Write, + }); + + await expect( + sut.update(factory.auth({ user }), personGroupId, { name: 'New name', isFavorite: true, isHidden: true }), + ).resolves.toEqual(expect.objectContaining({ name: 'New name', isFavorite: true, isHidden: true })); + + await expect(personRepo.getForUser({ userId: writer.id, personGroupId })).resolves.toEqual( + expect.objectContaining({ name: 'New name', isFavorite: false, isHidden: false }), + ); + }); + + it('should not allow personal properties to be updated for another user', async () => { + const { ctx, sut } = setup(); + const { user } = await ctx.newUser(); + const { user: writer } = await ctx.newUser(); + const { person } = await ctx.newPerson({ ownerId: writer.id }); + const { personGroupId } = person; + await ctx.newPersonUser({ + personGroupId, + sharedById: writer.id, + sharedWithId: user.id, + role: PersonUserRole.Write, + }); + + await expect( + sut.update(factory.auth({ user }), personGroupId, { isFavorite: true, userId: writer.id }), + ).rejects.toThrow('Only name and birthDate can be updated for other users'); + }); + }); + describe('delete', () => { it('should throw an error when there is no access', async () => { const { sut } = setup(); diff --git a/server/test/medium/specs/services/user.service.spec.ts b/server/test/medium/specs/services/user.service.spec.ts index 09b3268333..1177922282 100644 --- a/server/test/medium/specs/services/user.service.spec.ts +++ b/server/test/medium/specs/services/user.service.spec.ts @@ -1,6 +1,6 @@ import { Kysely } from 'kysely'; import { DateTime } from 'luxon'; -import { ImmichEnvironment, JobName, JobStatus, UserAvatarColor } from 'src/enum.js'; +import { ImmichEnvironment, JobName, JobStatus, PersonUpdateStrategy, UserAvatarColor } from 'src/enum.js'; import { ClusterGroupRepository } from 'src/repositories/cluster-group.repository.js'; import { ConfigRepository } from 'src/repositories/config.repository.js'; import { CryptoRepository } from 'src/repositories/crypto.repository.js'; @@ -263,6 +263,20 @@ describe(UserService.name, () => { await expect(sut.updateMyPreferences(auth, dto)).resolves.toMatchObject(dto); await expect(sut.getMyPreferences(auth)).resolves.toMatchObject(dto); }); + + it('should update the person update strategy', async () => { + const { sut, ctx } = setup(); + const { user } = await ctx.newUser(); + const auth = factory.auth({ user: { id: user.id } }); + + const dto = { people: { updateStrategy: PersonUpdateStrategy.Self } }; + + await expect(sut.getMyPreferences(auth)).resolves.toMatchObject({ + people: { updateStrategy: PersonUpdateStrategy.Everyone }, + }); + await expect(sut.updateMyPreferences(auth, dto)).resolves.toMatchObject(dto); + await expect(sut.getMyPreferences(auth)).resolves.toMatchObject(dto); + }); }); describe('setLicense', () => { diff --git a/web/src/lib/modals/PersonEditModal.svelte b/web/src/lib/modals/PersonEditModal.svelte index 6e447948a2..c1d9b98d81 100644 --- a/web/src/lib/modals/PersonEditModal.svelte +++ b/web/src/lib/modals/PersonEditModal.svelte @@ -1,9 +1,9 @@