cjones/immich
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

chore(server): Use access core for person permissions (#4138)

Browse Source 
* use access core for all person methods

* minor fixes, feedback

* reorder assignments

* remove unnecessary permission requirement

* unify naming of tests

* reorder variables
This commit is contained in:
Daniel Dietzler
2023-09-18 23:22:44 +02:00
committed by GitHub
parent dda735ec51
commit 1a633f3fca
8 changed files with 171 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/domain/person/person.service.spec.ts
+105 -12
View File
@@ -1,8 +1,10 @@
import { BadRequestException, NotFoundException } from '@nestjs/common';
import {
IAccessRepositoryMock,
assetStub,
authStub,
faceStub,
newAccessRepositoryMock,
newJobRepositoryMock,
newPersonRepositoryMock,
newStorageRepositoryMock,
@@ -26,18 +28,20 @@ const responseDto: PersonResponseDto = {
};
describe(PersonService.name, () => {
let sut: PersonService;
let personMock: jest.Mocked<IPersonRepository>;
let accessMock: IAccessRepositoryMock;
let configMock: jest.Mocked<ISystemConfigRepository>;
let storageMock: jest.Mocked<IStorageRepository>;
let jobMock: jest.Mocked<IJobRepository>;
let personMock: jest.Mocked<IPersonRepository>;
let storageMock: jest.Mocked<IStorageRepository>;
let sut: PersonService;
beforeEach(async () => {
accessMock = newAccessRepositoryMock();
personMock = newPersonRepositoryMock();
storageMock = newStorageRepositoryMock();
configMock = newSystemConfigRepositoryMock();
jobMock = newJobRepositoryMock();
sut = new PersonService(personMock, configMock, storageMock, jobMock);
sut = new PersonService(accessMock, personMock, configMock, storageMock, jobMock);
});
it('should be defined', () => {
@@ -93,74 +97,124 @@ describe(PersonService.name, () => {
});
describe('getById', () => {
it('should require person.read permission', async () => {
personMock.getById.mockResolvedValue(personStub.withName);
accessMock.person.hasOwnerAccess.mockResolvedValue(false);
await expect(sut.getById(authStub.admin, 'person-1')).rejects.toBeInstanceOf(BadRequestException);
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should throw a bad request when person is not found', async () => {
personMock.getById.mockResolvedValue(null);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.getById(authStub.admin, 'person-1')).rejects.toBeInstanceOf(BadRequestException);
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should get a person by id', async () => {
personMock.getById.mockResolvedValue(personStub.withName);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.getById(authStub.admin, 'person-1')).resolves.toEqual(responseDto);
expect(personMock.getById).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
expect(personMock.getById).toHaveBeenCalledWith('person-1');
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
});
describe('getThumbnail', () => {
it('should require person.read permission', async () => {
personMock.getById.mockResolvedValue(personStub.noName);
accessMock.person.hasOwnerAccess.mockResolvedValue(false);
await expect(sut.getThumbnail(authStub.admin, 'person-1')).rejects.toBeInstanceOf(BadRequestException);
expect(storageMock.createReadStream).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should throw an error when personId is invalid', async () => {
personMock.getById.mockResolvedValue(null);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.getThumbnail(authStub.admin, 'person-1')).rejects.toBeInstanceOf(NotFoundException);
expect(storageMock.createReadStream).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should throw an error when person has no thumbnail', async () => {
personMock.getById.mockResolvedValue(personStub.noThumbnail);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.getThumbnail(authStub.admin, 'person-1')).rejects.toBeInstanceOf(NotFoundException);
expect(storageMock.createReadStream).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should serve the thumbnail', async () => {
personMock.getById.mockResolvedValue(personStub.noName);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await sut.getThumbnail(authStub.admin, 'person-1');
expect(storageMock.createReadStream).toHaveBeenCalledWith('/path/to/thumbnail.jpg', 'image/jpeg');
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
});
describe('getAssets', () => {
it('should require person.read permission', async () => {
personMock.getAssets.mockResolvedValue([assetStub.image, assetStub.video]);
accessMock.person.hasOwnerAccess.mockResolvedValue(false);
await expect(sut.getAssets(authStub.admin, 'person-1')).rejects.toBeInstanceOf(BadRequestException);
expect(personMock.getAssets).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it("should return a person's assets", async () => {
personMock.getAssets.mockResolvedValue([assetStub.image, assetStub.video]);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await sut.getAssets(authStub.admin, 'person-1');
expect(personMock.getAssets).toHaveBeenCalledWith('admin_id', 'person-1');
expect(personMock.getAssets).toHaveBeenCalledWith('person-1');
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
});
describe('update', () => {
it('should throw an error when personId is invalid', async () => {
personMock.getById.mockResolvedValue(null);
it('should require person.write permission', async () => {
personMock.getById.mockResolvedValue(personStub.noName);
accessMock.person.hasOwnerAccess.mockResolvedValue(false);
await expect(sut.update(authStub.admin, 'person-1', { name: 'Person 1' })).rejects.toBeInstanceOf(
BadRequestException,
);
expect(personMock.update).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should throw an error when personId is invalid', async () => {
personMock.getById.mockResolvedValue(null);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.update(authStub.admin, 'person-1', { name: 'Person 1' })).rejects.toBeInstanceOf(
BadRequestException,
);
expect(personMock.update).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it("should update a person's name", async () => {
personMock.getById.mockResolvedValue(personStub.noName);
personMock.update.mockResolvedValue(personStub.withName);
personMock.getAssets.mockResolvedValue([assetStub.image]);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.update(authStub.admin, 'person-1', { name: 'Person 1' })).resolves.toEqual(responseDto);
expect(personMock.getById).toHaveBeenCalledWith('admin_id', 'person-1');
expect(personMock.getById).toHaveBeenCalledWith('person-1');
expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', name: 'Person 1' });
expect(jobMock.queue).toHaveBeenCalledWith({
name: JobName.SEARCH_INDEX_ASSET,
data: { ids: [assetStub.image.id] },
});
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it("should update a person's date of birth", async () => {
personMock.getById.mockResolvedValue(personStub.noBirthDate);
personMock.update.mockResolvedValue(personStub.withBirthDate);
personMock.getAssets.mockResolvedValue([assetStub.image]);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.update(authStub.admin, 'person-1', { birthDate: new Date('1976-06-30') })).resolves.toEqual({
id: 'person-1',
@@ -170,35 +224,39 @@ describe(PersonService.name, () => {
isHidden: false,
});
expect(personMock.getById).toHaveBeenCalledWith('admin_id', 'person-1');
expect(personMock.getById).toHaveBeenCalledWith('person-1');
expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', birthDate: new Date('1976-06-30') });
expect(jobMock.queue).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should update a person visibility', async () => {
personMock.getById.mockResolvedValue(personStub.hidden);
personMock.update.mockResolvedValue(personStub.withName);
personMock.getAssets.mockResolvedValue([assetStub.image]);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.update(authStub.admin, 'person-1', { isHidden: false })).resolves.toEqual(responseDto);
expect(personMock.getById).toHaveBeenCalledWith('admin_id', 'person-1');
expect(personMock.getById).toHaveBeenCalledWith('person-1');
expect(personMock.update).toHaveBeenCalledWith({ id: 'person-1', isHidden: false });
expect(jobMock.queue).toHaveBeenCalledWith({
name: JobName.SEARCH_INDEX_ASSET,
data: { ids: [assetStub.image.id] },
});
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it("should update a person's thumbnailPath", async () => {
personMock.getById.mockResolvedValue(personStub.withName);
personMock.getFaceById.mockResolvedValue(faceStub.face1);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(
sut.update(authStub.admin, 'person-1', { featureFaceAssetId: faceStub.face1.assetId }),
).resolves.toEqual(responseDto);
expect(personMock.getById).toHaveBeenCalledWith('admin_id', 'person-1');
expect(personMock.getById).toHaveBeenCalledWith('person-1');
expect(personMock.getFaceById).toHaveBeenCalledWith({
assetId: faceStub.face1.assetId,
personId: 'person-1',
@@ -218,25 +276,31 @@ describe(PersonService.name, () => {
imageWidth: faceStub.face1.imageWidth,
},
});
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should throw an error when the face feature assetId is invalid', async () => {
personMock.getById.mockResolvedValue(personStub.withName);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.update(authStub.admin, 'person-1', { featureFaceAssetId: '-1' })).rejects.toThrow(
BadRequestException,
);
expect(personMock.update).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
});
describe('updateAll', () => {
it('should throw an error when personId is invalid', async () => {
personMock.getById.mockResolvedValue(null);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(
sut.updatePeople(authStub.admin, { people: [{ id: 'person-1', name: 'Person 1' }] }),
).resolves.toEqual([{ error: BulkIdErrorReason.UNKNOWN, id: 'person-1', success: false }]);
expect(personMock.update).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
});
@@ -255,11 +319,31 @@ describe(PersonService.name, () => {
});
describe('mergePerson', () => {
it('should require person.write and person.merge permission', async () => {
personMock.getById.mockResolvedValueOnce(personStub.primaryPerson);
personMock.getById.mockResolvedValueOnce(personStub.mergePerson);
personMock.prepareReassignFaces.mockResolvedValue([]);
personMock.delete.mockResolvedValue(personStub.mergePerson);
accessMock.person.hasOwnerAccess.mockResolvedValue(false);
await expect(sut.mergePerson(authStub.admin, 'person-1', { ids: ['person-2'] })).rejects.toBeInstanceOf(
BadRequestException,
);
expect(personMock.prepareReassignFaces).not.toHaveBeenCalled();
expect(personMock.reassignFaces).not.toHaveBeenCalled();
expect(personMock.delete).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should merge two people', async () => {
personMock.getById.mockResolvedValueOnce(personStub.primaryPerson);
personMock.getById.mockResolvedValueOnce(personStub.mergePerson);
personMock.prepareReassignFaces.mockResolvedValue([]);
personMock.delete.mockResolvedValue(personStub.mergePerson);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.mergePerson(authStub.admin, 'person-1', { ids: ['person-2'] })).resolves.toEqual([
{ id: 'person-2', success: true },
@@ -276,12 +360,14 @@ describe(PersonService.name, () => {
});
expect(personMock.delete).toHaveBeenCalledWith(personStub.mergePerson);
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should delete conflicting faces before merging', async () => {
personMock.getById.mockResolvedValue(personStub.primaryPerson);
personMock.getById.mockResolvedValue(personStub.mergePerson);
personMock.prepareReassignFaces.mockResolvedValue([assetStub.image.id]);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.mergePerson(authStub.admin, 'person-1', { ids: ['person-2'] })).resolves.toEqual([
{ id: 'person-2', success: true },
@@ -296,21 +382,25 @@ describe(PersonService.name, () => {
name: JobName.SEARCH_REMOVE_FACE,
data: { assetId: assetStub.image.id, personId: personStub.mergePerson.id },
});
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should throw an error when the primary person is not found', async () => {
personMock.getById.mockResolvedValue(null);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.mergePerson(authStub.admin, 'person-1', { ids: ['person-2'] })).rejects.toBeInstanceOf(
BadRequestException,
);
expect(personMock.delete).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should handle invalid merge ids', async () => {
personMock.getById.mockResolvedValueOnce(personStub.primaryPerson);
personMock.getById.mockResolvedValueOnce(null);
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.mergePerson(authStub.admin, 'person-1', { ids: ['person-2'] })).resolves.toEqual([
{ id: 'person-2', success: false, error: BulkIdErrorReason.NOT_FOUND },
@@ -319,6 +409,7 @@ describe(PersonService.name, () => {
expect(personMock.prepareReassignFaces).not.toHaveBeenCalled();
expect(personMock.reassignFaces).not.toHaveBeenCalled();
expect(personMock.delete).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
it('should handle an error reassigning faces', async () => {
@@ -326,12 +417,14 @@ describe(PersonService.name, () => {
personMock.getById.mockResolvedValue(personStub.mergePerson);
personMock.prepareReassignFaces.mockResolvedValue([assetStub.image.id]);
personMock.reassignFaces.mockRejectedValue(new Error('update failed'));
accessMock.person.hasOwnerAccess.mockResolvedValue(true);
await expect(sut.mergePerson(authStub.admin, 'person-1', { ids: ['person-2'] })).resolves.toEqual([
{ id: 'person-2', success: false, error: BulkIdErrorReason.UNKNOWN },
]);
expect(personMock.delete).not.toHaveBeenCalled();
expect(accessMock.person.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'person-1');
});
});
});