From 8f2bfa099e9302866e3ffda6941ed58a3eb0894b Mon Sep 17 00:00:00 2001 From: bo0tzz Date: Wed, 28 Oct 2026 12:53:36 +0100 Subject: [PATCH] fix(server): scope duplicate group resolution to the caller's assets (#32603) (cherry picked from commit 7bb91afbe15e82502f8f6c92b61edb3865ce76f3) --- server/src/queries/duplicate.repository.sql | 3 +- .../src/repositories/duplicate.repository.ts | 3 +- server/src/services/duplicate.service.spec.ts | 22 +++++++++++ server/src/services/duplicate.service.ts | 2 +- .../specs/services/duplicate.service.spec.ts | 38 ++++++++++++++----- 5 files changed, 55 insertions(+), 13 deletions(-) diff --git a/server/src/queries/duplicate.repository.sql b/server/src/queries/duplicate.repository.sql index 24a02e0f2..a25323d14 100644 --- a/server/src/queries/duplicate.repository.sql +++ b/server/src/queries/duplicate.repository.sql @@ -121,7 +121,8 @@ from ) as "asset2" on true where "asset"."visibility" in ('archive', 'timeline') - and "asset"."duplicateId" = $1::uuid + and "asset"."ownerId" = $1::uuid + and "asset"."duplicateId" = $2::uuid and "asset"."deletedAt" is null and "asset"."stackId" is null group by diff --git a/server/src/repositories/duplicate.repository.ts b/server/src/repositories/duplicate.repository.ts index 7b4682e85..2ed6cc272 100644 --- a/server/src/repositories/duplicate.repository.ts +++ b/server/src/repositories/duplicate.repository.ts @@ -112,7 +112,7 @@ export class DuplicateRepository { } @GenerateSql({ params: [DummyValue.UUID, DummyValue.UUID] }) - async get(duplicateId: string): Promise<{ duplicateId: string; assets: MapAsset[] } | undefined> { + async get(userId: string, duplicateId: string): Promise<{ duplicateId: string; assets: MapAsset[] } | undefined> { const result = await this.db .selectFrom('asset') .$call(withDefaultVisibility) @@ -141,6 +141,7 @@ export class DuplicateRepository { ) .select('asset.duplicateId') .select((eb) => eb.fn.jsonAgg('asset2').orderBy('asset.localDateTime', 'asc').$castTo().as('assets')) + .where('asset.ownerId', '=', asUuid(userId)) .where('asset.duplicateId', '=', asUuid(duplicateId)) .where('asset.deletedAt', 'is', null) .where('asset.stackId', 'is', null) diff --git a/server/src/services/duplicate.service.spec.ts b/server/src/services/duplicate.service.spec.ts index b41f3a07e..da33f8e5d 100644 --- a/server/src/services/duplicate.service.spec.ts +++ b/server/src/services/duplicate.service.spec.ts @@ -233,6 +233,28 @@ describe(DuplicateService.name, () => { ]); }); + it('should only load the assets owned by the user', async () => { + const asset = AssetFactory.create(); + mocks.access.duplicate.checkOwnerAccess.mockResolvedValue(new Set(['group-1'])); + mocks.duplicateRepository.get.mockResolvedValue({ + duplicateId: 'group-1', + assets: [asset as unknown as MapAsset], + }); + + mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([asset.id])); + + const result = await sut.resolve(authStub.admin, { + groups: [{ duplicateId: 'group-1', keepAssetIds: ['other-user-asset'], trashAssetIds: [asset.id] }], + }); + + expect(mocks.duplicateRepository.get).toHaveBeenCalledWith(authStub.admin.user.id, 'group-1'); + expect(result[0].success).toBe(true); + expect(mocks.asset.updateAllExif).not.toHaveBeenCalled(); + for (const [ids] of mocks.asset.updateAll.mock.calls) { + expect(ids).toEqual([asset.id]); + } + }); + it('should skip when keepAssetIds contains non-member', async () => { const asset = AssetFactory.create(); mocks.access.duplicate.checkOwnerAccess.mockResolvedValue(new Set(['group-1'])); diff --git a/server/src/services/duplicate.service.ts b/server/src/services/duplicate.service.ts index 1d01e195a..b876a8a68 100644 --- a/server/src/services/duplicate.service.ts +++ b/server/src/services/duplicate.service.ts @@ -112,7 +112,7 @@ export class DuplicateService extends BaseService { private async resolveGroup(auth: AuthDto, group: DuplicateResolveGroupDto): Promise { const { duplicateId, keepAssetIds, trashAssetIds } = group; - const duplicateGroup = await this.duplicateRepository.get(duplicateId); + const duplicateGroup = await this.duplicateRepository.get(auth.user.id, duplicateId); if (!duplicateGroup) { return { id: duplicateId, success: false, error: BulkIdErrorReason.NOT_FOUND }; } diff --git a/server/test/medium/specs/services/duplicate.service.spec.ts b/server/test/medium/specs/services/duplicate.service.spec.ts index 8f90d0a6f..a3005558f 100644 --- a/server/test/medium/specs/services/duplicate.service.spec.ts +++ b/server/test/medium/specs/services/duplicate.service.spec.ts @@ -289,7 +289,7 @@ describe(DuplicateService.name, () => { } }); - it('should fail without permission to delete an asset owned by someone else', async () => { + it('should ignore an asset owned by someone else', async () => { const { sut, ctx } = setup(); const { user } = await ctx.newUser(); const { user: other } = await ctx.newUser(); @@ -301,16 +301,12 @@ describe(DuplicateService.name, () => { const auth = factory.auth({ user: { id: user.id } }); await expect( sut.resolve(auth, { groups: [{ duplicateId, keepAssetIds: [mine.id], trashAssetIds: [theirs.id] }] }), - ).resolves.toEqual([ - { - id: duplicateId, - success: false, - error: BulkIdErrorReason.NO_PERMISSION, - errorMessage: 'No permission to delete assets', - }, - ]); + ).resolves.toEqual([{ id: duplicateId, success: true }]); - await expect(ctx.get(AssetRepository).getById(theirs.id)).resolves.toMatchObject({ deletedAt: null }); + await expect(ctx.get(AssetRepository).getById(theirs.id)).resolves.toMatchObject({ + deletedAt: null, + duplicateId, + }); }); it('should resolve several groups in one request', async () => { @@ -475,6 +471,28 @@ describe(DuplicateService.name, () => { expect(result?.exifInfo?.description?.split('\n').sort()).toEqual(['from keeper', 'from trashed']); }); + it('should not merge metadata into an asset owned by someone else', async () => { + const { sut, ctx } = setup(); + const { user } = await ctx.newUser(); + const { user: other } = await ctx.newUser(); + const duplicateId = factory.uuid(); + + const theirs = await newDuplicateAsset(ctx, { ownerId: other.id, duplicateId }, { description: 'theirs' }); + const mine = await newDuplicateAsset(ctx, { ownerId: user.id, duplicateId }, { description: 'mine' }); + + const auth = factory.auth({ user: { id: user.id } }); + expectSuccess( + await sut.resolve(auth, { + groups: [{ duplicateId, keepAssetIds: [theirs.id], trashAssetIds: [mine.id] }], + }), + duplicateId, + ); + + const result = await ctx.get(AssetRepository).getById(theirs.id, { exifInfo: true }); + expect(result).toMatchObject({ duplicateId, deletedAt: null }); + expect(result?.exifInfo?.description).toBe('theirs'); + }); + it('should merge a location that both assets agree on', async () => { const { sut, ctx } = setup(); const { user } = await ctx.newUser();