diff --git a/server/src/domain/access/access.core.ts b/server/src/domain/access/access.core.ts index fc170386d1..fe9d972239 100644 --- a/server/src/domain/access/access.core.ts +++ b/server/src/domain/access/access.core.ts @@ -140,6 +140,20 @@ export class AccessCore { private async checkAccessOther(auth: AuthDto, permission: Permission, ids: Set) { switch (permission) { + // uses album id + case Permission.ACTIVITY_CREATE: + return await this.repository.activity.checkCreateAccess(auth.user.id, ids); + + // uses activity id + case Permission.ACTIVITY_DELETE: { + const isOwner = await this.repository.activity.checkOwnerAccess(auth.user.id, ids); + const isAlbumOwner = await this.repository.activity.checkAlbumOwnerAccess( + auth.user.id, + setDifference(ids, isOwner), + ); + return setUnion(isOwner, isAlbumOwner); + } + case Permission.ASSET_READ: { const isOwner = await this.repository.asset.checkOwnerAccess(auth.user.id, ids); const isAlbum = await this.repository.asset.checkAlbumAccess(auth.user.id, setDifference(ids, isOwner)); @@ -249,41 +263,16 @@ export class AccessCore { return await this.repository.person.checkOwnerAccess(auth.user.id, ids); case Permission.PERSON_CREATE: - return this.repository.person.hasFaceOwnerAccess(auth.user.id, ids); + return this.repository.person.checkFaceOwnerAccess(auth.user.id, ids); case Permission.PERSON_REASSIGN: - return this.repository.person.hasFaceOwnerAccess(auth.user.id, ids); + return this.repository.person.checkFaceOwnerAccess(auth.user.id, ids); case Permission.PARTNER_UPDATE: return await this.repository.partner.checkUpdateAccess(auth.user.id, ids); - } - - const allowedIds = new Set(); - for (const id of ids) { - const hasAccess = await this.hasOtherAccess(auth, permission, id); - if (hasAccess) { - allowedIds.add(id); - } - } - return allowedIds; - } - - // TODO: Migrate logic to checkAccessOther to evaluate permissions in bulk. - private async hasOtherAccess(auth: AuthDto, permission: Permission, id: string) { - switch (permission) { - // uses album id - case Permission.ACTIVITY_CREATE: - return await this.repository.activity.hasCreateAccess(auth.user.id, id); - - // uses activity id - case Permission.ACTIVITY_DELETE: - return ( - (await this.repository.activity.hasOwnerAccess(auth.user.id, id)) || - (await this.repository.activity.hasAlbumOwnerAccess(auth.user.id, id)) - ); default: - return false; + return new Set(); } } } diff --git a/server/src/domain/activity/activity.spec.ts b/server/src/domain/activity/activity.spec.ts index 659718bede..79c466b922 100644 --- a/server/src/domain/activity/activity.spec.ts +++ b/server/src/domain/activity/activity.spec.ts @@ -93,7 +93,7 @@ describe(ActivityService.name, () => { }); it('should create a comment', async () => { - accessMock.activity.hasCreateAccess.mockResolvedValue(true); + accessMock.activity.checkCreateAccess.mockResolvedValue(new Set(['album-id'])); activityMock.create.mockResolvedValue(activityStub.oneComment); await sut.create(authStub.admin, { @@ -114,7 +114,6 @@ describe(ActivityService.name, () => { it('should fail because activity is disabled for the album', async () => { accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id'])); - accessMock.activity.hasCreateAccess.mockResolvedValue(false); activityMock.create.mockResolvedValue(activityStub.oneComment); await expect( @@ -128,7 +127,7 @@ describe(ActivityService.name, () => { }); it('should create a like', async () => { - accessMock.activity.hasCreateAccess.mockResolvedValue(true); + accessMock.activity.checkCreateAccess.mockResolvedValue(new Set(['album-id'])); activityMock.create.mockResolvedValue(activityStub.liked); activityMock.search.mockResolvedValue([]); @@ -148,7 +147,7 @@ describe(ActivityService.name, () => { it('should skip if like exists', async () => { accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id'])); - accessMock.activity.hasCreateAccess.mockResolvedValue(true); + accessMock.activity.checkCreateAccess.mockResolvedValue(new Set(['album-id'])); activityMock.search.mockResolvedValue([activityStub.liked]); await sut.create(authStub.admin, { @@ -163,19 +162,18 @@ describe(ActivityService.name, () => { describe('delete', () => { it('should require access', async () => { - accessMock.activity.hasOwnerAccess.mockResolvedValue(false); await expect(sut.delete(authStub.admin, activityStub.oneComment.id)).rejects.toBeInstanceOf(BadRequestException); expect(activityMock.delete).not.toHaveBeenCalled(); }); it('should let the activity owner delete a comment', async () => { - accessMock.activity.hasOwnerAccess.mockResolvedValue(true); + accessMock.activity.checkOwnerAccess.mockResolvedValue(new Set(['activity-id'])); await sut.delete(authStub.admin, 'activity-id'); expect(activityMock.delete).toHaveBeenCalledWith('activity-id'); }); it('should let the album owner delete a comment', async () => { - accessMock.activity.hasAlbumOwnerAccess.mockResolvedValue(true); + accessMock.activity.checkAlbumOwnerAccess.mockResolvedValue(new Set(['activity-id'])); await sut.delete(authStub.admin, 'activity-id'); expect(activityMock.delete).toHaveBeenCalledWith('activity-id'); }); diff --git a/server/src/domain/person/person.service.spec.ts b/server/src/domain/person/person.service.spec.ts index 5154bd43de..484ba165fe 100644 --- a/server/src/domain/person/person.service.spec.ts +++ b/server/src/domain/person/person.service.spec.ts @@ -360,7 +360,7 @@ describe(PersonService.name, () => { it('should reassign a face', async () => { accessMock.person.checkOwnerAccess.mockResolvedValue(new Set([personStub.withName.id])); personMock.getById.mockResolvedValue(personStub.noName); - accessMock.person.hasFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id])); + accessMock.person.checkFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id])); personMock.getFacesByIds.mockResolvedValue([faceStub.face1]); personMock.reassignFace.mockResolvedValue(1); personMock.getRandomFace.mockResolvedValue(faceStub.primaryFace1); @@ -415,7 +415,7 @@ describe(PersonService.name, () => { describe('reassignFacesById', () => { it('should create a new person', async () => { accessMock.person.checkOwnerAccess.mockResolvedValue(new Set([personStub.noName.id])); - accessMock.person.hasFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id])); + accessMock.person.checkFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id])); personMock.getFaceById.mockResolvedValue(faceStub.face1); personMock.reassignFace.mockResolvedValue(1); personMock.getById.mockResolvedValue(personStub.noName); @@ -437,7 +437,6 @@ describe(PersonService.name, () => { it('should fail if user has not the correct permissions on the asset', async () => { accessMock.person.checkOwnerAccess.mockResolvedValue(new Set([personStub.noName.id])); - accessMock.person.hasFaceOwnerAccess.mockResolvedValue(new Set()); personMock.getFaceById.mockResolvedValue(faceStub.face1); personMock.reassignFace.mockResolvedValue(1); personMock.getById.mockResolvedValue(personStub.noName); @@ -456,7 +455,7 @@ describe(PersonService.name, () => { it('should create a new person', async () => { personMock.create.mockResolvedValue(personStub.primaryPerson); personMock.getFaceById.mockResolvedValue(faceStub.face1); - accessMock.person.hasFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id])); + accessMock.person.checkFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id])); await expect(sut.createPerson(authStub.admin)).resolves.toBe(personStub.primaryPerson); }); diff --git a/server/src/domain/repositories/access.repository.ts b/server/src/domain/repositories/access.repository.ts index a501e4d6d4..6aa70a2123 100644 --- a/server/src/domain/repositories/access.repository.ts +++ b/server/src/domain/repositories/access.repository.ts @@ -2,9 +2,9 @@ export const IAccessRepository = 'IAccessRepository'; export interface IAccessRepository { activity: { - hasOwnerAccess(userId: string, activityId: string): Promise; - hasAlbumOwnerAccess(userId: string, activityId: string): Promise; - hasCreateAccess(userId: string, albumId: string): Promise; + checkOwnerAccess(userId: string, activityIds: Set): Promise>; + checkAlbumOwnerAccess(userId: string, activityIds: Set): Promise>; + checkCreateAccess(userId: string, albumIds: Set): Promise>; }; asset: { @@ -34,7 +34,7 @@ export interface IAccessRepository { }; person: { - hasFaceOwnerAccess(userId: string, assetFaceId: Set): Promise>; + checkFaceOwnerAccess(userId: string, assetFaceId: Set): Promise>; checkOwnerAccess(userId: string, personIds: Set): Promise>; }; diff --git a/server/src/infra/repositories/access.repository.ts b/server/src/infra/repositories/access.repository.ts index 5a3f9925b1..eb01c93bed 100644 --- a/server/src/infra/repositories/access.repository.ts +++ b/server/src/infra/repositories/access.repository.ts @@ -27,41 +27,64 @@ export class AccessRepository implements IAccessRepository { ) {} activity = { - hasOwnerAccess: (userId: string, activityId: string): Promise => { - return this.activityRepository.exist({ - where: { - id: activityId, - userId, - }, - }); - }, - hasAlbumOwnerAccess: (userId: string, activityId: string): Promise => { - return this.activityRepository.exist({ - where: { - id: activityId, - album: { - ownerId: userId, + checkOwnerAccess: async (userId: string, activityIds: Set): Promise> => { + if (activityIds.size === 0) { + return new Set(); + } + + return this.activityRepository + .find({ + select: { id: true }, + where: { + id: In([...activityIds]), + userId, }, - }, - }); + }) + .then((activities) => new Set(activities.map((activity) => activity.id))); }, - hasCreateAccess: (userId: string, albumId: string): Promise => { - return this.albumRepository.exist({ - where: [ - { - id: albumId, - isActivityEnabled: true, - sharedUsers: { - id: userId, + + checkAlbumOwnerAccess: async (userId: string, activityIds: Set): Promise> => { + if (activityIds.size === 0) { + return new Set(); + } + + return this.activityRepository + .find({ + select: { id: true }, + where: { + id: In([...activityIds]), + album: { + ownerId: userId, }, }, - { - id: albumId, - isActivityEnabled: true, - ownerId: userId, - }, - ], - }); + }) + .then((activities) => new Set(activities.map((activity) => activity.id))); + }, + + checkCreateAccess: async (userId: string, albumIds: Set): Promise> => { + if (albumIds.size === 0) { + return new Set(); + } + + return this.albumRepository + .find({ + select: { id: true }, + where: [ + { + id: In([...albumIds]), + isActivityEnabled: true, + sharedUsers: { + id: userId, + }, + }, + { + id: In([...albumIds]), + isActivityEnabled: true, + ownerId: userId, + }, + ], + }) + .then((albums) => new Set(albums.map((album) => album.id))); }, }; @@ -320,7 +343,8 @@ export class AccessRepository implements IAccessRepository { }) .then((persons) => new Set(persons.map((person) => person.id))); }, - hasFaceOwnerAccess: async (userId: string, assetFaceIds: Set): Promise> => { + + checkFaceOwnerAccess: async (userId: string, assetFaceIds: Set): Promise> => { if (assetFaceIds.size === 0) { return new Set(); } diff --git a/server/test/repositories/access.repository.mock.ts b/server/test/repositories/access.repository.mock.ts index 52fa21252f..a1f09aa75c 100644 --- a/server/test/repositories/access.repository.mock.ts +++ b/server/test/repositories/access.repository.mock.ts @@ -18,9 +18,9 @@ export const newAccessRepositoryMock = (reset = true): IAccessRepositoryMock => return { activity: { - hasOwnerAccess: jest.fn(), - hasAlbumOwnerAccess: jest.fn(), - hasCreateAccess: jest.fn(), + checkOwnerAccess: jest.fn().mockResolvedValue(new Set()), + checkAlbumOwnerAccess: jest.fn().mockResolvedValue(new Set()), + checkCreateAccess: jest.fn().mockResolvedValue(new Set()), }, asset: { @@ -50,7 +50,7 @@ export const newAccessRepositoryMock = (reset = true): IAccessRepositoryMock => }, person: { - hasFaceOwnerAccess: jest.fn(), + checkFaceOwnerAccess: jest.fn().mockResolvedValue(new Set()), checkOwnerAccess: jest.fn().mockResolvedValue(new Set()), },