From c6f56d959185ee59a0c3c1d864a657af1b5197f9 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Sun, 17 Dec 2023 13:10:21 -0500 Subject: [PATCH] chore(server): Check activity permissions in bulk (#5775) Modify Access repository, to evaluate `asset` permissions in bulk. This is the last set of permission changes, to migrate all of them to run in bulk! Queries have been validated to match what they currently generate for single ids. Queries: * `activity` owner access: ```sql -- Before SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS ( SELECT 1 FROM "activity" "ActivityEntity" WHERE "ActivityEntity"."id" = $1 AND "ActivityEntity"."userId" = $2 ) LIMIT 1 -- After SELECT "ActivityEntity"."id" AS "ActivityEntity_id" FROM "activity" "ActivityEntity" WHERE "ActivityEntity"."id" IN ($1) AND "ActivityEntity"."userId" = $2 ``` * `activity` album owner access: ```sql -- Before SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS ( SELECT 1 FROM "activity" "ActivityEntity" LEFT JOIN "albums" "ActivityEntity__ActivityEntity_album" ON "ActivityEntity__ActivityEntity_album"."id"="ActivityEntity"."albumId" AND "ActivityEntity__ActivityEntity_album"."deletedAt" IS NULL WHERE "ActivityEntity"."id" = $1 AND "ActivityEntity__ActivityEntity_album"."ownerId" = $2 ) LIMIT 1 -- After SELECT "ActivityEntity"."id" AS "ActivityEntity_id" FROM "activity" "ActivityEntity" LEFT JOIN "albums" "ActivityEntity__ActivityEntity_album" ON "ActivityEntity__ActivityEntity_album"."id"="ActivityEntity"."albumId" AND "ActivityEntity__ActivityEntity_album"."deletedAt" IS NULL WHERE "ActivityEntity"."id" IN ($1) AND "ActivityEntity__ActivityEntity_album"."ownerId" = $2 ``` * `activity` create access: ```sql -- Before SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS ( SELECT 1 FROM "albums" "AlbumEntity" LEFT JOIN "albums_shared_users_users" "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers" ON "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."albumsId"="AlbumEntity"."id" LEFT JOIN "users" "AlbumEntity__AlbumEntity_sharedUsers" ON "AlbumEntity__AlbumEntity_sharedUsers"."id"="AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."usersId" AND "AlbumEntity__AlbumEntity_sharedUsers"."deletedAt" IS NULL WHERE ( ( "AlbumEntity"."id" = $1 AND "AlbumEntity"."isActivityEnabled" = $2 AND "AlbumEntity__AlbumEntity_sharedUsers"."id" = $3 ) OR ( "AlbumEntity"."id" = $4 AND "AlbumEntity"."isActivityEnabled" = $5 AND "AlbumEntity"."ownerId" = $6 ) ) AND "AlbumEntity"."deletedAt" IS NULL ) LIMIT 1 -- After SELECT "AlbumEntity"."id" AS "AlbumEntity_id" FROM "albums" "AlbumEntity" LEFT JOIN "albums_shared_users_users" "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers" ON "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."albumsId"="AlbumEntity"."id" LEFT JOIN "users" "AlbumEntity__AlbumEntity_sharedUsers" ON "AlbumEntity__AlbumEntity_sharedUsers"."id"="AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."usersId" AND "AlbumEntity__AlbumEntity_sharedUsers"."deletedAt" IS NULL WHERE ( ( "AlbumEntity"."id" IN ($1) AND "AlbumEntity"."isActivityEnabled" = $2 AND "AlbumEntity__AlbumEntity_sharedUsers"."id" = $3 ) OR ( "AlbumEntity"."id" IN ($4) AND "AlbumEntity"."isActivityEnabled" = $5 AND "AlbumEntity"."ownerId" = $6 ) ) AND "AlbumEntity"."deletedAt" IS NULL ``` --- server/src/domain/access/access.core.ts | 45 ++++------ server/src/domain/activity/activity.spec.ts | 12 ++- .../src/domain/person/person.service.spec.ts | 7 +- .../domain/repositories/access.repository.ts | 8 +- .../infra/repositories/access.repository.ts | 88 ++++++++++++------- .../repositories/access.repository.mock.ts | 8 +- 6 files changed, 89 insertions(+), 79 deletions(-) diff --git a/server/src/domain/access/access.core.ts b/server/src/domain/access/access.core.ts index fc170386d195a..fe9d972239eae 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 659718bede7b9..79c466b92266a 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 5154bd43de297..484ba165fe5c1 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 a501e4d6d45cd..6aa70a2123afa 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 5a3f9925b1afd..eb01c93bed8ad 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 52fa21252ffa2..a1f09aa75cb4d 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()), },