From bb069909571f4e514e7d050ddf588c017ee5a029 Mon Sep 17 00:00:00 2001 From: timonrieger Date: Thu, 30 Apr 2026 13:11:44 +0200 Subject: [PATCH] fix(server): use 403 instead of 400 for access-denied errors requireAccess threw BadRequestException which is incorrect HTTP semantics. Access denial is a client authorization problem (403 Forbidden), not a malformed request (400 Bad Request). Keep the descriptive permission name in the message since the full permission set is public API surface. --- e2e/src/specs/server/api/activity.e2e-spec.ts | 2 +- e2e/src/specs/server/api/album.e2e-spec.ts | 8 +++---- e2e/src/specs/server/api/asset.e2e-spec.ts | 2 +- e2e/src/specs/server/api/session.e2e-spec.ts | 2 +- e2e/src/specs/server/api/tag.e2e-spec.ts | 2 +- server/src/services/activity.service.spec.ts | 8 +++---- server/src/services/album.service.spec.ts | 22 +++++++++---------- .../src/services/asset-media.service.spec.ts | 7 +++--- server/src/services/asset.service.spec.ts | 16 +++++++------- server/src/services/memory.service.spec.ts | 12 +++++----- server/src/services/partner.service.spec.ts | 4 ++-- server/src/services/person.service.spec.ts | 22 +++++++++---------- .../src/services/shared-link.service.spec.ts | 4 ++-- server/src/services/stack.service.spec.ts | 14 ++++++------ server/src/services/tag.service.spec.ts | 8 +++---- server/src/services/trash.service.spec.ts | 4 ++-- server/src/utils/access.ts | 4 ++-- 17 files changed, 71 insertions(+), 70 deletions(-) diff --git a/e2e/src/specs/server/api/activity.e2e-spec.ts b/e2e/src/specs/server/api/activity.e2e-spec.ts index 9ce9b4b916..94f47a7a40 100644 --- a/e2e/src/specs/server/api/activity.e2e-spec.ts +++ b/e2e/src/specs/server/api/activity.e2e-spec.ts @@ -324,7 +324,7 @@ describe('/activities', () => { .set('Authorization', `Bearer ${nonOwner.accessToken}`); expect(status).toBe(400); - expect(body).toEqual(errorDto.badRequest('Not found or no activity.delete access')); + expect(body).toEqual(errorDto.noPermission); }); it('should let a non-owner remove their own comment', async () => { diff --git a/e2e/src/specs/server/api/album.e2e-spec.ts b/e2e/src/specs/server/api/album.e2e-spec.ts index e1e5178476..a8b32b58aa 100644 --- a/e2e/src/specs/server/api/album.e2e-spec.ts +++ b/e2e/src/specs/server/api/album.e2e-spec.ts @@ -481,7 +481,7 @@ describe('/albums', () => { .send({ ids: [asset.id] }); expect(status).toBe(400); - expect(body).toEqual(errorDto.badRequest('Not found or no albumAsset.create access')); + expect(body).toEqual(errorDto.noPermission); }); it('should add duplicate assets only once', async () => { @@ -527,7 +527,7 @@ describe('/albums', () => { .send({ albumName: 'New album name' }); expect(status).toBe(400); - expect(body).toEqual(errorDto.badRequest('Not found or no album.update access')); + expect(body).toEqual(errorDto.noPermission); }); it('should be able to update as an editor', async () => { @@ -615,7 +615,7 @@ describe('/albums', () => { .send({ ids: [user1Asset1.id] }); expect(status).toBe(400); - expect(body).toEqual(errorDto.badRequest('Not found or no albumAsset.delete access')); + expect(body).toEqual(errorDto.noPermission); }); it('should remove duplicate assets only once', async () => { @@ -729,7 +729,7 @@ describe('/albums', () => { .send({ role: AlbumUserRole.Editor }); expect(status).toBe(400); - expect(body).toEqual(errorDto.badRequest('Not found or no album.share access')); + expect(body).toEqual(errorDto.noPermission); }); }); }); diff --git a/e2e/src/specs/server/api/asset.e2e-spec.ts b/e2e/src/specs/server/api/asset.e2e-spec.ts index 3fbacd5bf6..cf54e8366b 100644 --- a/e2e/src/specs/server/api/asset.e2e-spec.ts +++ b/e2e/src/specs/server/api/asset.e2e-spec.ts @@ -610,7 +610,7 @@ describe('/asset', () => { .set('Authorization', `Bearer ${admin.accessToken}`); expect(status).toBe(400); - expect(body).toEqual(errorDto.badRequest('Not found or no asset.delete access')); + expect(body).toEqual(errorDto.noPermission); }); it('should move an asset to trash', async () => { diff --git a/e2e/src/specs/server/api/session.e2e-spec.ts b/e2e/src/specs/server/api/session.e2e-spec.ts index 0b632f78ba..e6b290b7ab 100644 --- a/e2e/src/specs/server/api/session.e2e-spec.ts +++ b/e2e/src/specs/server/api/session.e2e-spec.ts @@ -53,7 +53,7 @@ describe('/sessions', () => { .delete(`/sessions/${uuidDto.notFound}`) .set('Authorization', `Bearer ${admin.accessToken}`); expect(status).toBe(400); - expect(body).toEqual(errorDto.badRequest('Not found or no authDevice.delete access')); + expect(body).toEqual(errorDto.noPermission); }); it('should logout a device', async () => { diff --git a/e2e/src/specs/server/api/tag.e2e-spec.ts b/e2e/src/specs/server/api/tag.e2e-spec.ts index 7b5a2f16de..64e3b1734d 100644 --- a/e2e/src/specs/server/api/tag.e2e-spec.ts +++ b/e2e/src/specs/server/api/tag.e2e-spec.ts @@ -512,7 +512,7 @@ describe('/tags', () => { .send({ ids: [userAsset.id] }); expect(status).toBe(400); - expect(body).toEqual(errorDto.badRequest('Not found or no tag.asset access')); + expect(body).toEqual(errorDto.noPermission); }); it('should add duplicate assets only once', async () => { diff --git a/server/src/services/activity.service.spec.ts b/server/src/services/activity.service.spec.ts index 03cd0132c1..e8bf12cdcd 100644 --- a/server/src/services/activity.service.spec.ts +++ b/server/src/services/activity.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException } from '@nestjs/common'; +import { ForbiddenException } from '@nestjs/common'; import { ReactionType } from 'src/dtos/activity.dto'; import { ActivityService } from 'src/services/activity.service'; import { ActivityFactory } from 'test/factories/activity.factory'; @@ -78,7 +78,7 @@ describe(ActivityService.name, () => { await expect( sut.create(AuthFactory.create(), { albumId, assetId, type: ReactionType.COMMENT, comment: 'comment' }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); }); it('should create a comment', async () => { @@ -113,7 +113,7 @@ describe(ActivityService.name, () => { await expect( sut.create(AuthFactory.create(), { albumId, assetId, type: ReactionType.COMMENT, comment: 'comment' }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); }); it('should create a like', async () => { @@ -145,7 +145,7 @@ describe(ActivityService.name, () => { describe('delete', () => { it('should require access', async () => { - await expect(sut.delete(AuthFactory.create(), newUuid())).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.delete(AuthFactory.create(), newUuid())).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.activity.delete).not.toHaveBeenCalled(); }); diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index 24e28a9701..446877aedc 100644 --- a/server/src/services/album.service.spec.ts +++ b/server/src/services/album.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { BulkIdErrorReason } from 'src/dtos/asset-ids.response.dto'; import { AlbumUserRole, AssetOrder, UserMetadataKey } from 'src/enum'; import { AlbumService } from 'src/services/album.service'; @@ -323,7 +323,7 @@ describe(AlbumService.name, () => { sut.update(AuthFactory.create(), 'invalid-id', { albumName: 'Album', }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.album.update).not.toHaveBeenCalled(); }); @@ -334,7 +334,7 @@ describe(AlbumService.name, () => { mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set()); await expect( sut.update(AuthFactory.create(owner), album.id, { albumName: 'new album name' }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); }); it('should require a valid thumbnail asset id', async () => { @@ -376,7 +376,7 @@ describe(AlbumService.name, () => { const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set()); - await expect(sut.delete(AuthFactory.create(owner), album.id)).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.delete(AuthFactory.create(owner), album.id)).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.album.delete).not.toHaveBeenCalled(); }); @@ -387,7 +387,7 @@ describe(AlbumService.name, () => { mocks.album.getById.mockResolvedValue(getForAlbum(album)); mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set()); - await expect(sut.delete(AuthFactory.create(owner), album.id)).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.delete(AuthFactory.create(owner), album.id)).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.album.delete).not.toHaveBeenCalled(); }); @@ -412,7 +412,7 @@ describe(AlbumService.name, () => { mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set()); await expect( sut.addUsers(AuthFactory.create(user), album.id, { albumUsers: [{ userId: newUuid() }] }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.album.update).not.toHaveBeenCalled(); }); @@ -512,7 +512,7 @@ describe(AlbumService.name, () => { mocks.album.getById.mockResolvedValue(getForAlbum(album)); await expect(sut.removeUser(AuthFactory.create(user1), album.id, user2.id)).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.albumUser.delete).not.toHaveBeenCalled(); @@ -655,7 +655,7 @@ describe(AlbumService.name, () => { it('should throw an error for no access', async () => { const auth = AuthFactory.create(); - await expect(sut.get(auth, 'album-123')).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.get(auth, 'album-123')).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.access.album.checkOwnerAccess).toHaveBeenCalledWith(auth.user.id, new Set(['album-123'])); expect(mocks.access.album.checkSharedAlbumAccess).toHaveBeenCalledWith( @@ -764,7 +764,7 @@ describe(AlbumService.name, () => { await expect( sut.addAssets(AuthFactory.create(user), album.id, { ids: [asset1.id, asset2.id, asset3.id] }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.album.update).not.toHaveBeenCalled(); }); @@ -833,7 +833,7 @@ describe(AlbumService.name, () => { mocks.album.getById.mockResolvedValue(getForAlbum(album)); await expect(sut.addAssets(AuthFactory.create(user), album.id, { ids: [asset.id] })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.access.album.checkOwnerAccess).toHaveBeenCalled(); @@ -847,7 +847,7 @@ describe(AlbumService.name, () => { await expect( sut.addAssets(AuthFactory.from().sharedLink({ allowUpload: true }).build(), album.id, { ids: [asset.id] }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.access.album.checkSharedLinkAccess).toHaveBeenCalled(); }); diff --git a/server/src/services/asset-media.service.spec.ts b/server/src/services/asset-media.service.spec.ts index cb3cc4d62a..6f54d77f12 100644 --- a/server/src/services/asset-media.service.spec.ts +++ b/server/src/services/asset-media.service.spec.ts @@ -1,5 +1,6 @@ import { BadRequestException, + ForbiddenException, InternalServerErrorException, NotFoundException, UnauthorizedException, @@ -459,7 +460,7 @@ describe(AssetMediaService.name, () => { describe('downloadOriginal', () => { it('should require the asset.download permission', async () => { - await expect(sut.downloadOriginal(authStub.admin, 'asset-1', {})).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.downloadOriginal(authStub.admin, 'asset-1', {})).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.access.asset.checkOwnerAccess).toHaveBeenCalledWith( authStub.admin.user.id, @@ -567,7 +568,7 @@ describe(AssetMediaService.name, () => { describe('viewThumbnail', () => { it('should require asset.view permissions', async () => { - await expect(sut.viewThumbnail(authStub.admin, 'id', {})).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.viewThumbnail(authStub.admin, 'id', {})).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.access.asset.checkOwnerAccess).toHaveBeenCalledWith(userStub.admin.id, new Set(['id']), undefined); expect(mocks.access.asset.checkAlbumAccess).toHaveBeenCalledWith(userStub.admin.id, new Set(['id'])); @@ -710,7 +711,7 @@ describe(AssetMediaService.name, () => { describe('playbackVideo', () => { it('should require asset.view permissions', async () => { - await expect(sut.playbackVideo(authStub.admin, 'id')).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.playbackVideo(authStub.admin, 'id')).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.access.asset.checkOwnerAccess).toHaveBeenCalledWith(userStub.admin.id, new Set(['id']), undefined); expect(mocks.access.asset.checkAlbumAccess).toHaveBeenCalledWith(userStub.admin.id, new Set(['id'])); diff --git a/server/src/services/asset.service.spec.ts b/server/src/services/asset.service.spec.ts index 13462a3246..e5fc98db27 100755 --- a/server/src/services/asset.service.spec.ts +++ b/server/src/services/asset.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { DateTime } from 'luxon'; import { AssetJobName, AssetStatsResponseDto } from 'src/dtos/asset.dto'; import { AssetEditAction } from 'src/dtos/editing.dto'; @@ -136,14 +136,14 @@ describe(AssetService.name, () => { }); it('should throw an error for no access', async () => { - await expect(sut.get(authStub.admin, AssetFactory.create().id)).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.get(authStub.admin, AssetFactory.create().id)).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.asset.getById).not.toHaveBeenCalled(); }); it('should throw an error for an invalid shared link', async () => { await expect(sut.get(authStub.adminSharedLink, AssetFactory.create().id)).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.access.asset.checkOwnerAccess).not.toHaveBeenCalled(); @@ -162,7 +162,7 @@ describe(AssetService.name, () => { it('should require asset write access for the id', async () => { await expect( sut.update(authStub.admin, 'asset-1', { visibility: AssetVisibility.Timeline }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.asset.update).not.toHaveBeenCalled(); }); @@ -357,7 +357,7 @@ describe(AssetService.name, () => { describe('updateAll', () => { it('should require asset write access for all ids', async () => { const auth = AuthFactory.create(); - await expect(sut.updateAll(auth, { ids: ['asset-1'] })).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.updateAll(auth, { ids: ['asset-1'] })).rejects.toBeInstanceOf(ForbiddenException); }); it('should update all assets', async () => { @@ -461,7 +461,7 @@ describe(AssetService.name, () => { sut.deleteAll(authStub.user1, { ids: ['asset-1'], }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); }); it('should force delete a batch of assets', async () => { @@ -612,7 +612,7 @@ describe(AssetService.name, () => { it('should require asset read permission', async () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set()); - await expect(sut.getOcr(authStub.admin, 'asset-1')).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.getOcr(authStub.admin, 'asset-1')).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.ocr.getByAssetId).not.toHaveBeenCalled(); }); @@ -736,7 +736,7 @@ describe(AssetService.name, () => { }, ], }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.assetEdit.replaceAll).not.toHaveBeenCalled(); }); diff --git a/server/src/services/memory.service.spec.ts b/server/src/services/memory.service.spec.ts index 0445cf892b..23c5c13c4a 100644 --- a/server/src/services/memory.service.spec.ts +++ b/server/src/services/memory.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { MemoryService } from 'src/services/memory.service'; import { OnThisDayData } from 'src/types'; import { AssetFactory } from 'test/factories/asset.factory'; @@ -53,7 +53,7 @@ describe(MemoryService.name, () => { describe('get', () => { it('should throw an error when no access', async () => { - await expect(sut.get(factory.auth(), 'not-found')).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.get(factory.auth(), 'not-found')).rejects.toBeInstanceOf(ForbiddenException); }); it('should throw an error when the memory is not found', async () => { @@ -151,7 +151,7 @@ describe(MemoryService.name, () => { describe('update', () => { it('should require access', async () => { await expect(sut.update(factory.auth(), 'not-found', { isSaved: true })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.memory.update).not.toHaveBeenCalled(); @@ -171,7 +171,7 @@ describe(MemoryService.name, () => { describe('remove', () => { it('should require access', async () => { - await expect(sut.remove(factory.auth(), newUuid())).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.remove(factory.auth(), newUuid())).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.memory.delete).not.toHaveBeenCalled(); }); @@ -193,7 +193,7 @@ describe(MemoryService.name, () => { const [memoryId, assetId] = newUuids(); await expect(sut.addAssets(factory.auth(), memoryId, { ids: [assetId] })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.memory.addAssetIds).not.toHaveBeenCalled(); @@ -251,7 +251,7 @@ describe(MemoryService.name, () => { describe('removeAssets', () => { it('should require memory access', async () => { await expect(sut.removeAssets(factory.auth(), 'not-found', { ids: ['asset1'] })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.memory.removeAssetIds).not.toHaveBeenCalled(); diff --git a/server/src/services/partner.service.spec.ts b/server/src/services/partner.service.spec.ts index 029462a865..2470912716 100644 --- a/server/src/services/partner.service.spec.ts +++ b/server/src/services/partner.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { PartnerDirection } from 'src/repositories/partner.repository'; import { PartnerService } from 'src/services/partner.service'; import { AuthFactory } from 'test/factories/auth.factory'; @@ -109,7 +109,7 @@ describe(PartnerService.name, () => { const user2 = UserFactory.create(); const auth = AuthFactory.create(); - await expect(sut.update(auth, user2.id, { inTimeline: false })).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.update(auth, user2.id, { inTimeline: false })).rejects.toBeInstanceOf(ForbiddenException); }); it('should update partner', async () => { diff --git a/server/src/services/person.service.spec.ts b/server/src/services/person.service.spec.ts index 8b303d04f6..ed61638686 100644 --- a/server/src/services/person.service.spec.ts +++ b/server/src/services/person.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException, NotFoundException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException, NotFoundException } from '@nestjs/common'; import { BulkIdErrorReason } from 'src/dtos/asset-ids.response.dto'; import { mapFaces, mapPerson } from 'src/dtos/person.dto'; import { AssetFileType, CacheControl, JobName, JobStatus, SourceType, SystemMetadataKey } from 'src/enum'; @@ -95,7 +95,7 @@ describe(PersonService.name, () => { const auth = AuthFactory.create(); const person = PersonFactory.create(); mocks.person.getById.mockResolvedValue(person); - await expect(sut.getById(auth, person.id)).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.getById(auth, person.id)).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.access.person.checkOwnerAccess).toHaveBeenCalledWith(auth.user.id, new Set([person.id])); }); @@ -124,7 +124,7 @@ describe(PersonService.name, () => { const person = PersonFactory.create(); mocks.person.getById.mockResolvedValue(person); - await expect(sut.getThumbnail(auth, person.id)).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.getThumbnail(auth, person.id)).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.storage.createReadStream).not.toHaveBeenCalled(); expect(mocks.access.person.checkOwnerAccess).toHaveBeenCalledWith(auth.user.id, new Set([person.id])); }); @@ -172,7 +172,7 @@ describe(PersonService.name, () => { const person = PersonFactory.create(); mocks.person.getById.mockResolvedValue(person); - await expect(sut.update(auth, person.id, { name: 'Person 1' })).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.update(auth, person.id, { name: 'Person 1' })).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.person.update).not.toHaveBeenCalled(); expect(mocks.access.person.checkOwnerAccess).toHaveBeenCalledWith(auth.user.id, new Set([person.id])); }); @@ -180,7 +180,7 @@ describe(PersonService.name, () => { it('should throw an error when personId is invalid', async () => { mocks.access.person.checkOwnerAccess.mockResolvedValue(new Set()); await expect(sut.update(authStub.admin, 'person-1', { name: 'Person 1' })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.person.update).not.toHaveBeenCalled(); expect(mocks.access.person.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['person-1'])); @@ -286,7 +286,7 @@ describe(PersonService.name, () => { mocks.person.getById.mockResolvedValue(person); mocks.access.person.checkOwnerAccess.mockResolvedValue(new Set([person.id])); - await expect(sut.update(auth, person.id, { featureFaceAssetId: '-1' })).rejects.toThrow(BadRequestException); + await expect(sut.update(auth, person.id, { featureFaceAssetId: '-1' })).rejects.toThrow(ForbiddenException); expect(mocks.person.update).not.toHaveBeenCalled(); expect(mocks.access.person.checkOwnerAccess).toHaveBeenCalledWith(auth.user.id, new Set([person.id])); }); @@ -312,7 +312,7 @@ describe(PersonService.name, () => { sut.reassignFaces(AuthFactory.create(), 'person-id', { data: [{ personId: 'asset-face-1', assetId: '' }], }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.job.queue).not.toHaveBeenCalledWith(); expect(mocks.job.queueAll).not.toHaveBeenCalledWith(); }); @@ -371,7 +371,7 @@ describe(PersonService.name, () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set()); mocks.person.getFaces.mockResolvedValue([getForAssetFace(face)]); await expect(sut.getFacesById(AuthFactory.create(), { id: face.assetId })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); }); }); @@ -507,7 +507,7 @@ describe(PersonService.name, () => { sut.reassignFacesById(AuthFactory.create(), person.id, { id: face.id, }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.job.queue).not.toHaveBeenCalledWith(); expect(mocks.job.queueAll).not.toHaveBeenCalledWith(); @@ -1189,7 +1189,7 @@ describe(PersonService.name, () => { mocks.person.getById.mockResolvedValueOnce(mergePerson); await expect(sut.mergePerson(auth, person.id, { ids: [mergePerson.id] })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.person.reassignFaces).not.toHaveBeenCalled(); @@ -1313,7 +1313,7 @@ describe(PersonService.name, () => { const person = PersonFactory.create(); mocks.person.getById.mockResolvedValue(person); - await expect(sut.getStatistics(auth, person.id)).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.getStatistics(auth, person.id)).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.access.person.checkOwnerAccess).toHaveBeenCalledWith(auth.user.id, new Set([person.id])); }); }); diff --git a/server/src/services/shared-link.service.spec.ts b/server/src/services/shared-link.service.spec.ts index 684d15bf7c..070a80cfe5 100644 --- a/server/src/services/shared-link.service.spec.ts +++ b/server/src/services/shared-link.service.spec.ts @@ -123,7 +123,7 @@ describe(SharedLinkService.name, () => { it('should not allow non-owners to create album shared links', async () => { await expect( sut.create(authStub.admin, { type: SharedLinkType.Album, assetIds: [], albumId: 'album-1' }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); }); it('should not allow individual shared links with no assets', async () => { @@ -135,7 +135,7 @@ describe(SharedLinkService.name, () => { it('should require asset ownership to make an individual shared link', async () => { await expect( sut.create(authStub.admin, { type: SharedLinkType.Individual, assetIds: ['asset-1'] }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); }); it('should create an album shared link', async () => { diff --git a/server/src/services/stack.service.spec.ts b/server/src/services/stack.service.spec.ts index d47d634f4f..ae0f5046b0 100644 --- a/server/src/services/stack.service.spec.ts +++ b/server/src/services/stack.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { StackService } from 'src/services/stack.service'; import { AssetFactory } from 'test/factories/asset.factory'; import { AuthFactory } from 'test/factories/auth.factory'; @@ -43,7 +43,7 @@ describe(StackService.name, () => { const [primaryAsset, asset] = [AssetFactory.create(), AssetFactory.create()]; await expect(sut.create(auth, { assetIds: [primaryAsset.id, asset.id] })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.access.asset.checkOwnerAccess).toHaveBeenCalled(); @@ -77,7 +77,7 @@ describe(StackService.name, () => { describe('get', () => { it('should require stack.read permissions', async () => { - await expect(sut.get(authStub.admin, 'stack-id')).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.get(authStub.admin, 'stack-id')).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.access.stack.checkOwnerAccess).toHaveBeenCalled(); expect(mocks.stack.getById).not.toHaveBeenCalled(); @@ -115,7 +115,7 @@ describe(StackService.name, () => { describe('update', () => { it('should require stack.update permissions', async () => { - await expect(sut.update(AuthFactory.create(), 'stack-id', {})).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.update(AuthFactory.create(), 'stack-id', {})).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.stack.getById).not.toHaveBeenCalled(); expect(mocks.stack.update).not.toHaveBeenCalled(); @@ -179,7 +179,7 @@ describe(StackService.name, () => { describe('delete', () => { it('should require stack.delete permissions', async () => { - await expect(sut.delete(AuthFactory.create(), 'stack-id')).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.delete(AuthFactory.create(), 'stack-id')).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.stack.delete).not.toHaveBeenCalled(); expect(mocks.event.emit).not.toHaveBeenCalled(); @@ -203,7 +203,7 @@ describe(StackService.name, () => { describe('deleteAll', () => { it('should require stack.delete permissions', async () => { - await expect(sut.deleteAll(authStub.admin, { ids: ['stack-id'] })).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.deleteAll(authStub.admin, { ids: ['stack-id'] })).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.stack.deleteAll).not.toHaveBeenCalled(); expect(mocks.event.emit).not.toHaveBeenCalled(); @@ -226,7 +226,7 @@ describe(StackService.name, () => { describe('removeAsset', () => { it('should require stack.update permissions', async () => { await expect(sut.removeAsset(authStub.admin, { id: 'stack-id', assetId: 'asset-id' })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.stack.getForAssetRemoval).not.toHaveBeenCalled(); diff --git a/server/src/services/tag.service.spec.ts b/server/src/services/tag.service.spec.ts index 6fc472bb87..c7b455688a 100644 --- a/server/src/services/tag.service.spec.ts +++ b/server/src/services/tag.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { BulkIdErrorReason } from 'src/dtos/asset-ids.response.dto'; import { JobStatus } from 'src/enum'; import { TagService } from 'src/services/tag.service'; @@ -45,7 +45,7 @@ describe(TagService.name, () => { it('should throw an error for no parent tag access', async () => { mocks.access.tag.checkOwnerAccess.mockResolvedValue(new Set()); await expect(sut.create(authStub.admin, { name: 'tag', parentId: 'tag-parent' })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.tag.create).not.toHaveBeenCalled(); }); @@ -105,7 +105,7 @@ describe(TagService.name, () => { it('should throw an error for no update permission', async () => { mocks.access.tag.checkOwnerAccess.mockResolvedValue(new Set()); await expect(sut.update(authStub.admin, 'tag-1', { color: '#000000' })).rejects.toBeInstanceOf( - BadRequestException, + ForbiddenException, ); expect(mocks.tag.update).not.toHaveBeenCalled(); }); @@ -165,7 +165,7 @@ describe(TagService.name, () => { describe('remove', () => { it('should throw an error for an invalid id', async () => { mocks.access.tag.checkOwnerAccess.mockResolvedValue(new Set()); - await expect(sut.remove(authStub.admin, 'tag-1')).rejects.toBeInstanceOf(BadRequestException); + await expect(sut.remove(authStub.admin, 'tag-1')).rejects.toBeInstanceOf(ForbiddenException); expect(mocks.tag.delete).not.toHaveBeenCalled(); }); diff --git a/server/src/services/trash.service.spec.ts b/server/src/services/trash.service.spec.ts index e43c49e543..1d0e9fc22a 100644 --- a/server/src/services/trash.service.spec.ts +++ b/server/src/services/trash.service.spec.ts @@ -1,4 +1,4 @@ -import { BadRequestException } from '@nestjs/common'; +import { ForbiddenException } from '@nestjs/common'; import { JobName, JobStatus } from 'src/enum'; import { TrashService } from 'src/services/trash.service'; import { authStub } from 'test/fixtures/auth.stub'; @@ -29,7 +29,7 @@ describe(TrashService.name, () => { sut.restoreAssets(authStub.user1, { ids: ['asset-1'], }), - ).rejects.toBeInstanceOf(BadRequestException); + ).rejects.toBeInstanceOf(ForbiddenException); }); it('should handle an empty list', async () => { diff --git a/server/src/utils/access.ts b/server/src/utils/access.ts index b8e7733772..154bca4b9d 100644 --- a/server/src/utils/access.ts +++ b/server/src/utils/access.ts @@ -1,4 +1,4 @@ -import { BadRequestException, UnauthorizedException } from '@nestjs/common'; +import { ForbiddenException, UnauthorizedException } from '@nestjs/common'; import { AuthSharedLink } from 'src/database'; import { AuthDto } from 'src/dtos/auth.dto'; import { AlbumUserRole, Permission } from 'src/enum'; @@ -37,7 +37,7 @@ export const requireUploadAccess = (auth: AuthDto | null): AuthDto => { export const requireAccess = async (access: AccessRepository, request: AccessRequest) => { const allowedIds = await checkAccess(access, request); if (!setIsEqual(new Set(request.ids), allowedIds)) { - throw new BadRequestException(`Not found or no ${request.permission} access`); + throw new ForbiddenException(`Not found or no ${request.permission} access`); } };