Skip to content

Commit c6f56d9

Browse files
adamantikeadamantike
authored
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 ```
1 parent 691e205 commit c6f56d9

File tree

6 files changed

+89
-79
lines changed

6 files changed

+89
-79
lines changed

‎server/src/domain/access/access.core.ts

Lines changed: 17 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,20 @@ export class AccessCore {
140140

141141
private async checkAccessOther(auth: AuthDto, permission: Permission, ids: Set<string>) {
142142
switch (permission) {
143+
// uses album id
144+
case Permission.ACTIVITY_CREATE:
145+
return await this.repository.activity.checkCreateAccess(auth.user.id, ids);
146+
147+
// uses activity id
148+
case Permission.ACTIVITY_DELETE: {
149+
const isOwner = await this.repository.activity.checkOwnerAccess(auth.user.id, ids);
150+
const isAlbumOwner = await this.repository.activity.checkAlbumOwnerAccess(
151+
auth.user.id,
152+
setDifference(ids, isOwner),
153+
);
154+
return setUnion(isOwner, isAlbumOwner);
155+
}
156+
143157
case Permission.ASSET_READ: {
144158
const isOwner = await this.repository.asset.checkOwnerAccess(auth.user.id, ids);
145159
const isAlbum = await this.repository.asset.checkAlbumAccess(auth.user.id, setDifference(ids, isOwner));
@@ -249,41 +263,16 @@ export class AccessCore {
249263
return await this.repository.person.checkOwnerAccess(auth.user.id, ids);
250264

251265
case Permission.PERSON_CREATE:
252-
return this.repository.person.hasFaceOwnerAccess(auth.user.id, ids);
266+
return this.repository.person.checkFaceOwnerAccess(auth.user.id, ids);
253267

254268
case Permission.PERSON_REASSIGN:
255-
return this.repository.person.hasFaceOwnerAccess(auth.user.id, ids);
269+
return this.repository.person.checkFaceOwnerAccess(auth.user.id, ids);
256270

257271
case Permission.PARTNER_UPDATE:
258272
return await this.repository.partner.checkUpdateAccess(auth.user.id, ids);
259-
}
260-
261-
const allowedIds = new Set();
262-
for (const id of ids) {
263-
const hasAccess = await this.hasOtherAccess(auth, permission, id);
264-
if (hasAccess) {
265-
allowedIds.add(id);
266-
}
267-
}
268-
return allowedIds;
269-
}
270-
271-
// TODO: Migrate logic to checkAccessOther to evaluate permissions in bulk.
272-
private async hasOtherAccess(auth: AuthDto, permission: Permission, id: string) {
273-
switch (permission) {
274-
// uses album id
275-
case Permission.ACTIVITY_CREATE:
276-
return await this.repository.activity.hasCreateAccess(auth.user.id, id);
277-
278-
// uses activity id
279-
case Permission.ACTIVITY_DELETE:
280-
return (
281-
(await this.repository.activity.hasOwnerAccess(auth.user.id, id)) ||
282-
(await this.repository.activity.hasAlbumOwnerAccess(auth.user.id, id))
283-
);
284273

285274
default:
286-
return false;
275+
return new Set();
287276
}
288277
}
289278
}

‎server/src/domain/activity/activity.spec.ts

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ describe(ActivityService.name, () => {
9393
});
9494

9595
it('should create a comment', async () => {
96-
accessMock.activity.hasCreateAccess.mockResolvedValue(true);
96+
accessMock.activity.checkCreateAccess.mockResolvedValue(new Set(['album-id']));
9797
activityMock.create.mockResolvedValue(activityStub.oneComment);
9898

9999
await sut.create(authStub.admin, {
@@ -114,7 +114,6 @@ describe(ActivityService.name, () => {
114114

115115
it('should fail because activity is disabled for the album', async () => {
116116
accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id']));
117-
accessMock.activity.hasCreateAccess.mockResolvedValue(false);
118117
activityMock.create.mockResolvedValue(activityStub.oneComment);
119118

120119
await expect(
@@ -128,7 +127,7 @@ describe(ActivityService.name, () => {
128127
});
129128

130129
it('should create a like', async () => {
131-
accessMock.activity.hasCreateAccess.mockResolvedValue(true);
130+
accessMock.activity.checkCreateAccess.mockResolvedValue(new Set(['album-id']));
132131
activityMock.create.mockResolvedValue(activityStub.liked);
133132
activityMock.search.mockResolvedValue([]);
134133

@@ -148,7 +147,7 @@ describe(ActivityService.name, () => {
148147

149148
it('should skip if like exists', async () => {
150149
accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id']));
151-
accessMock.activity.hasCreateAccess.mockResolvedValue(true);
150+
accessMock.activity.checkCreateAccess.mockResolvedValue(new Set(['album-id']));
152151
activityMock.search.mockResolvedValue([activityStub.liked]);
153152

154153
await sut.create(authStub.admin, {
@@ -163,19 +162,18 @@ describe(ActivityService.name, () => {
163162

164163
describe('delete', () => {
165164
it('should require access', async () => {
166-
accessMock.activity.hasOwnerAccess.mockResolvedValue(false);
167165
await expect(sut.delete(authStub.admin, activityStub.oneComment.id)).rejects.toBeInstanceOf(BadRequestException);
168166
expect(activityMock.delete).not.toHaveBeenCalled();
169167
});
170168

171169
it('should let the activity owner delete a comment', async () => {
172-
accessMock.activity.hasOwnerAccess.mockResolvedValue(true);
170+
accessMock.activity.checkOwnerAccess.mockResolvedValue(new Set(['activity-id']));
173171
await sut.delete(authStub.admin, 'activity-id');
174172
expect(activityMock.delete).toHaveBeenCalledWith('activity-id');
175173
});
176174

177175
it('should let the album owner delete a comment', async () => {
178-
accessMock.activity.hasAlbumOwnerAccess.mockResolvedValue(true);
176+
accessMock.activity.checkAlbumOwnerAccess.mockResolvedValue(new Set(['activity-id']));
179177
await sut.delete(authStub.admin, 'activity-id');
180178
expect(activityMock.delete).toHaveBeenCalledWith('activity-id');
181179
});

‎server/src/domain/person/person.service.spec.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@ describe(PersonService.name, () => {
360360
it('should reassign a face', async () => {
361361
accessMock.person.checkOwnerAccess.mockResolvedValue(new Set([personStub.withName.id]));
362362
personMock.getById.mockResolvedValue(personStub.noName);
363-
accessMock.person.hasFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id]));
363+
accessMock.person.checkFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id]));
364364
personMock.getFacesByIds.mockResolvedValue([faceStub.face1]);
365365
personMock.reassignFace.mockResolvedValue(1);
366366
personMock.getRandomFace.mockResolvedValue(faceStub.primaryFace1);
@@ -415,7 +415,7 @@ describe(PersonService.name, () => {
415415
describe('reassignFacesById', () => {
416416
it('should create a new person', async () => {
417417
accessMock.person.checkOwnerAccess.mockResolvedValue(new Set([personStub.noName.id]));
418-
accessMock.person.hasFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id]));
418+
accessMock.person.checkFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id]));
419419
personMock.getFaceById.mockResolvedValue(faceStub.face1);
420420
personMock.reassignFace.mockResolvedValue(1);
421421
personMock.getById.mockResolvedValue(personStub.noName);
@@ -437,7 +437,6 @@ describe(PersonService.name, () => {
437437

438438
it('should fail if user has not the correct permissions on the asset', async () => {
439439
accessMock.person.checkOwnerAccess.mockResolvedValue(new Set([personStub.noName.id]));
440-
accessMock.person.hasFaceOwnerAccess.mockResolvedValue(new Set());
441440
personMock.getFaceById.mockResolvedValue(faceStub.face1);
442441
personMock.reassignFace.mockResolvedValue(1);
443442
personMock.getById.mockResolvedValue(personStub.noName);
@@ -456,7 +455,7 @@ describe(PersonService.name, () => {
456455
it('should create a new person', async () => {
457456
personMock.create.mockResolvedValue(personStub.primaryPerson);
458457
personMock.getFaceById.mockResolvedValue(faceStub.face1);
459-
accessMock.person.hasFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id]));
458+
accessMock.person.checkFaceOwnerAccess.mockResolvedValue(new Set([faceStub.face1.id]));
460459

461460
await expect(sut.createPerson(authStub.admin)).resolves.toBe(personStub.primaryPerson);
462461
});

‎server/src/domain/repositories/access.repository.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@ export const IAccessRepository = 'IAccessRepository';
22

33
export interface IAccessRepository {
44
activity: {
5-
hasOwnerAccess(userId: string, activityId: string): Promise<boolean>;
6-
hasAlbumOwnerAccess(userId: string, activityId: string): Promise<boolean>;
7-
hasCreateAccess(userId: string, albumId: string): Promise<boolean>;
5+
checkOwnerAccess(userId: string, activityIds: Set<string>): Promise<Set<string>>;
6+
checkAlbumOwnerAccess(userId: string, activityIds: Set<string>): Promise<Set<string>>;
7+
checkCreateAccess(userId: string, albumIds: Set<string>): Promise<Set<string>>;
88
};
99

1010
asset: {
@@ -34,7 +34,7 @@ export interface IAccessRepository {
3434
};
3535

3636
person: {
37-
hasFaceOwnerAccess(userId: string, assetFaceId: Set<string>): Promise<Set<string>>;
37+
checkFaceOwnerAccess(userId: string, assetFaceId: Set<string>): Promise<Set<string>>;
3838
checkOwnerAccess(userId: string, personIds: Set<string>): Promise<Set<string>>;
3939
};
4040

‎server/src/infra/repositories/access.repository.ts

Lines changed: 56 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -27,41 +27,64 @@ export class AccessRepository implements IAccessRepository {
2727
) {}
2828

2929
activity = {
30-
hasOwnerAccess: (userId: string, activityId: string): Promise<boolean> => {
31-
return this.activityRepository.exist({
32-
where: {
33-
id: activityId,
34-
userId,
35-
},
36-
});
37-
},
38-
hasAlbumOwnerAccess: (userId: string, activityId: string): Promise<boolean> => {
39-
return this.activityRepository.exist({
40-
where: {
41-
id: activityId,
42-
album: {
43-
ownerId: userId,
30+
checkOwnerAccess: async (userId: string, activityIds: Set<string>): Promise<Set<string>> => {
31+
if (activityIds.size === 0) {
32+
return new Set();
33+
}
34+
35+
return this.activityRepository
36+
.find({
37+
select: { id: true },
38+
where: {
39+
id: In([...activityIds]),
40+
userId,
4441
},
45-
},
46-
});
42+
})
43+
.then((activities) => new Set(activities.map((activity) => activity.id)));
4744
},
48-
hasCreateAccess: (userId: string, albumId: string): Promise<boolean> => {
49-
return this.albumRepository.exist({
50-
where: [
51-
{
52-
id: albumId,
53-
isActivityEnabled: true,
54-
sharedUsers: {
55-
id: userId,
45+
46+
checkAlbumOwnerAccess: async (userId: string, activityIds: Set<string>): Promise<Set<string>> => {
47+
if (activityIds.size === 0) {
48+
return new Set();
49+
}
50+
51+
return this.activityRepository
52+
.find({
53+
select: { id: true },
54+
where: {
55+
id: In([...activityIds]),
56+
album: {
57+
ownerId: userId,
5658
},
5759
},
58-
{
59-
id: albumId,
60-
isActivityEnabled: true,
61-
ownerId: userId,
62-
},
63-
],
64-
});
60+
})
61+
.then((activities) => new Set(activities.map((activity) => activity.id)));
62+
},
63+
64+
checkCreateAccess: async (userId: string, albumIds: Set<string>): Promise<Set<string>> => {
65+
if (albumIds.size === 0) {
66+
return new Set();
67+
}
68+
69+
return this.albumRepository
70+
.find({
71+
select: { id: true },
72+
where: [
73+
{
74+
id: In([...albumIds]),
75+
isActivityEnabled: true,
76+
sharedUsers: {
77+
id: userId,
78+
},
79+
},
80+
{
81+
id: In([...albumIds]),
82+
isActivityEnabled: true,
83+
ownerId: userId,
84+
},
85+
],
86+
})
87+
.then((albums) => new Set(albums.map((album) => album.id)));
6588
},
6689
};
6790

@@ -320,7 +343,8 @@ export class AccessRepository implements IAccessRepository {
320343
})
321344
.then((persons) => new Set(persons.map((person) => person.id)));
322345
},
323-
hasFaceOwnerAccess: async (userId: string, assetFaceIds: Set<string>): Promise<Set<string>> => {
346+
347+
checkFaceOwnerAccess: async (userId: string, assetFaceIds: Set<string>): Promise<Set<string>> => {
324348
if (assetFaceIds.size === 0) {
325349
return new Set();
326350
}

‎server/test/repositories/access.repository.mock.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@ export const newAccessRepositoryMock = (reset = true): IAccessRepositoryMock =>
1818

1919
return {
2020
activity: {
21-
hasOwnerAccess: jest.fn(),
22-
hasAlbumOwnerAccess: jest.fn(),
23-
hasCreateAccess: jest.fn(),
21+
checkOwnerAccess: jest.fn().mockResolvedValue(new Set()),
22+
checkAlbumOwnerAccess: jest.fn().mockResolvedValue(new Set()),
23+
checkCreateAccess: jest.fn().mockResolvedValue(new Set()),
2424
},
2525

2626
asset: {
@@ -50,7 +50,7 @@ export const newAccessRepositoryMock = (reset = true): IAccessRepositoryMock =>
5050
},
5151

5252
person: {
53-
hasFaceOwnerAccess: jest.fn(),
53+
checkFaceOwnerAccess: jest.fn().mockResolvedValue(new Set()),
5454
checkOwnerAccess: jest.fn().mockResolvedValue(new Set()),
5555
},
5656

0 commit comments

Comments
 (0)