diff --git a/server/src/cores/access.core.ts b/server/src/cores/access.core.ts index f836e97623..8d021031e9 100644 --- a/server/src/cores/access.core.ts +++ b/server/src/cores/access.core.ts @@ -84,7 +84,7 @@ export class AccessCore { * * @returns Set */ - async checkAccess(auth: AuthDto, permission: Permission, ids: Set | string[]) { + async checkAccess(auth: AuthDto, permission: Permission, ids: Set | string[]): Promise> { const idSet = Array.isArray(ids) ? new Set(ids) : ids; if (idSet.size === 0) { return new Set(); @@ -97,7 +97,11 @@ export class AccessCore { return this.checkAccessOther(auth, permission, idSet); } - private async checkAccessSharedLink(sharedLink: SharedLinkEntity, permission: Permission, ids: Set) { + private async checkAccessSharedLink( + sharedLink: SharedLinkEntity, + permission: Permission, + ids: Set, + ): Promise> { const sharedLinkId = sharedLink.id; switch (permission) { @@ -140,7 +144,7 @@ export class AccessCore { } } - private async checkAccessOther(auth: AuthDto, permission: Permission, ids: Set) { + private async checkAccessOther(auth: AuthDto, permission: Permission, ids: Set): Promise> { switch (permission) { // uses album id case Permission.ACTIVITY_CREATE: { diff --git a/server/src/interfaces/album.interface.ts b/server/src/interfaces/album.interface.ts index 230e94e2a2..48c728febc 100644 --- a/server/src/interfaces/album.interface.ts +++ b/server/src/interfaces/album.interface.ts @@ -1,4 +1,5 @@ import { AlbumEntity } from 'src/entities/album.entity'; +import { IBulkAsset } from 'src/utils/asset.util'; export const IAlbumRepository = 'IAlbumRepository'; @@ -23,15 +24,14 @@ export interface AlbumAssets { assetIds: string[]; } -export interface IAlbumRepository { +export interface IAlbumRepository extends IBulkAsset { getById(id: string, options: AlbumInfoOptions): Promise; getByIds(ids: string[]): Promise; getByAssetId(ownerId: string, assetId: string): Promise; - addAssets(assets: AlbumAssets): Promise; getAssetIds(albumId: string, assetIds?: string[]): Promise>; hasAsset(asset: AlbumAsset): Promise; removeAsset(assetId: string): Promise; - removeAssets(albumId: string, assetIds: string[]): Promise; + removeAssetIds(albumId: string, assetIds: string[]): Promise; getMetadataForIds(ids: string[]): Promise; getInvalidThumbnail(): Promise; getOwned(ownerId: string): Promise; diff --git a/server/src/queries/album.repository.sql b/server/src/queries/album.repository.sql index ddedc00959..50f775d2f5 100644 --- a/server/src/queries/album.repository.sql +++ b/server/src/queries/album.repository.sql @@ -590,7 +590,7 @@ DELETE FROM "albums_assets_assets" WHERE "albums_assets_assets"."assetsId" = $1 --- AlbumRepository.removeAssets +-- AlbumRepository.removeAssetIds DELETE FROM "albums_assets_assets" WHERE ( @@ -646,7 +646,7 @@ WHERE LIMIT 1 --- AlbumRepository.addAssets +-- AlbumRepository.addAssetIds INSERT INTO "albums_assets_assets" ("albumsId", "assetsId") VALUES diff --git a/server/src/repositories/album.repository.ts b/server/src/repositories/album.repository.ts index f98be21617..bbaab2a12b 100644 --- a/server/src/repositories/album.repository.ts +++ b/server/src/repositories/album.repository.ts @@ -5,13 +5,7 @@ import { dataSource } from 'src/database.config'; import { Chunked, ChunkedArray, DATABASE_PARAMETER_CHUNK_SIZE, DummyValue, GenerateSql } from 'src/decorators'; import { AlbumEntity } from 'src/entities/album.entity'; import { AssetEntity } from 'src/entities/asset.entity'; -import { - AlbumAsset, - AlbumAssetCount, - AlbumAssets, - AlbumInfoOptions, - IAlbumRepository, -} from 'src/interfaces/album.interface'; +import { AlbumAsset, AlbumAssetCount, AlbumInfoOptions, IAlbumRepository } from 'src/interfaces/album.interface'; import { Instrumentation } from 'src/utils/instrumentation'; import { setUnion } from 'src/utils/set'; import { DataSource, FindOptionsOrder, FindOptionsRelations, In, IsNull, Not, Repository } from 'typeorm'; @@ -203,7 +197,7 @@ export class AlbumRepository implements IAlbumRepository { @GenerateSql({ params: [DummyValue.UUID, [DummyValue.UUID]] }) @Chunked({ paramIndex: 1 }) - async removeAssets(albumId: string, assetIds: string[]): Promise { + async removeAssetIds(albumId: string, assetIds: string[]): Promise { await this.dataSource .createQueryBuilder() .delete() @@ -260,8 +254,8 @@ export class AlbumRepository implements IAlbumRepository { }); } - @GenerateSql({ params: [{ albumId: DummyValue.UUID, assetIds: [DummyValue.UUID] }] }) - async addAssets({ albumId, assetIds }: AlbumAssets): Promise { + @GenerateSql({ params: [DummyValue.UUID, [DummyValue.UUID]] }) + async addAssetIds(albumId: string, assetIds: string[]): Promise { await this.dataSource .createQueryBuilder() .insert() diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index 48462fac4c..02bb607b52 100644 --- a/server/src/services/album.service.spec.ts +++ b/server/src/services/album.service.spec.ts @@ -518,10 +518,7 @@ describe(AlbumService.name, () => { updatedAt: expect.any(Date), albumThumbnailAssetId: 'asset-1', }); - expect(albumMock.addAssets).toHaveBeenCalledWith({ - albumId: 'album-123', - assetIds: ['asset-1', 'asset-2', 'asset-3'], - }); + expect(albumMock.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']); }); it('should not set the thumbnail if the album has one already', async () => { @@ -539,7 +536,7 @@ describe(AlbumService.name, () => { updatedAt: expect.any(Date), albumThumbnailAssetId: 'asset-id', }); - expect(albumMock.addAssets).toHaveBeenCalled(); + expect(albumMock.addAssetIds).toHaveBeenCalled(); }); it('should allow a shared user to add assets', async () => { @@ -561,10 +558,7 @@ describe(AlbumService.name, () => { updatedAt: expect.any(Date), albumThumbnailAssetId: 'asset-1', }); - expect(albumMock.addAssets).toHaveBeenCalledWith({ - albumId: 'album-123', - assetIds: ['asset-1', 'asset-2', 'asset-3'], - }); + expect(albumMock.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']); }); it('should allow a shared link user to add assets', async () => { @@ -586,10 +580,7 @@ describe(AlbumService.name, () => { updatedAt: expect.any(Date), albumThumbnailAssetId: 'asset-1', }); - expect(albumMock.addAssets).toHaveBeenCalledWith({ - albumId: 'album-123', - assetIds: ['asset-1', 'asset-2', 'asset-3'], - }); + expect(albumMock.addAssetIds).toHaveBeenCalledWith('album-123', ['asset-1', 'asset-2', 'asset-3']); expect(accessMock.album.checkSharedLinkAccess).toHaveBeenCalledWith( authStub.adminSharedLink.sharedLink?.id, @@ -665,23 +656,23 @@ describe(AlbumService.name, () => { describe('removeAssets', () => { it('should allow the owner to remove assets', async () => { - accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123'])); - accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['asset-id'])); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-id'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); - albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); + albumMock.getAssetIds.mockResolvedValue(new Set(['asset-id'])); await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ { success: true, id: 'asset-id' }, ]); expect(albumMock.update).toHaveBeenCalledWith({ id: 'album-123', updatedAt: expect.any(Date) }); - expect(albumMock.removeAssets).toHaveBeenCalledWith('album-123', ['asset-id']); + expect(albumMock.removeAssetIds).toHaveBeenCalledWith('album-123', ['asset-id']); }); it('should skip assets not in the album', async () => { accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.empty)); - albumMock.getAssetIds.mockResolvedValueOnce(new Set()); + albumMock.getAssetIds.mockResolvedValue(new Set()); await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ { success: false, id: 'asset-id', error: BulkIdErrorReason.NOT_FOUND }, @@ -693,7 +684,7 @@ describe(AlbumService.name, () => { it('should skip assets without user permission to remove', async () => { accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); - albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); + albumMock.getAssetIds.mockResolvedValue(new Set(['asset-id'])); await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ { @@ -707,10 +698,10 @@ describe(AlbumService.name, () => { }); it('should reset the thumbnail if it is removed', async () => { - accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123'])); - accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['asset-id'])); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-id'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.twoAssets)); - albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); + albumMock.getAssetIds.mockResolvedValue(new Set(['asset-id'])); await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ { success: true, id: 'asset-id' }, diff --git a/server/src/services/album.service.ts b/server/src/services/album.service.ts index 483ddc3b02..df6c6b814c 100644 --- a/server/src/services/album.service.ts +++ b/server/src/services/album.service.ts @@ -12,7 +12,7 @@ import { mapAlbumWithAssets, mapAlbumWithoutAssets, } from 'src/dtos/album.dto'; -import { BulkIdErrorReason, BulkIdResponseDto, BulkIdsDto } from 'src/dtos/asset-ids.response.dto'; +import { BulkIdResponseDto, BulkIdsDto } from 'src/dtos/asset-ids.response.dto'; import { AuthDto } from 'src/dtos/auth.dto'; import { AlbumEntity } from 'src/entities/album.entity'; import { AssetEntity } from 'src/entities/asset.entity'; @@ -21,13 +21,13 @@ import { IAccessRepository } from 'src/interfaces/access.interface'; import { AlbumAssetCount, AlbumInfoOptions, IAlbumRepository } from 'src/interfaces/album.interface'; import { IAssetRepository } from 'src/interfaces/asset.interface'; import { IUserRepository } from 'src/interfaces/user.interface'; -import { setUnion } from 'src/utils/set'; +import { addAssets, removeAssets } from 'src/utils/asset.util'; @Injectable() export class AlbumService { private access: AccessCore; constructor( - @Inject(IAccessRepository) accessRepository: IAccessRepository, + @Inject(IAccessRepository) private accessRepository: IAccessRepository, @Inject(IAlbumRepository) private albumRepository: IAlbumRepository, @Inject(IAssetRepository) private assetRepository: IAssetRepository, @Inject(IUserRepository) private userRepository: IUserRepository, @@ -164,37 +164,20 @@ export class AlbumService { async addAssets(auth: AuthDto, id: string, dto: BulkIdsDto): Promise { const album = await this.findOrFail(id, { withAssets: false }); - await this.access.requirePermission(auth, Permission.ALBUM_READ, id); - const existingAssetIds = await this.albumRepository.getAssetIds(id, dto.ids); - const notPresentAssetIds = dto.ids.filter((id) => !existingAssetIds.has(id)); - const allowedAssetIds = await this.access.checkAccess(auth, Permission.ASSET_SHARE, notPresentAssetIds); + const results = await addAssets( + auth, + { accessRepository: this.accessRepository, repository: this.albumRepository }, + { id, assetIds: dto.ids }, + ); - const results: BulkIdResponseDto[] = []; - for (const assetId of dto.ids) { - const hasAsset = existingAssetIds.has(assetId); - if (hasAsset) { - results.push({ id: assetId, success: false, error: BulkIdErrorReason.DUPLICATE }); - continue; - } - - const hasAccess = allowedAssetIds.has(assetId); - if (!hasAccess) { - results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION }); - continue; - } - - results.push({ id: assetId, success: true }); - } - - const newAssetIds = results.filter(({ success }) => success).map(({ id }) => id); - if (newAssetIds.length > 0) { - await this.albumRepository.addAssets({ albumId: id, assetIds: newAssetIds }); + const { id: firstNewAssetId } = results.find(({ success }) => success) || {}; + if (firstNewAssetId) { await this.albumRepository.update({ id, updatedAt: new Date(), - albumThumbnailAssetId: album.albumThumbnailAssetId ?? newAssetIds[0], + albumThumbnailAssetId: album.albumThumbnailAssetId ?? firstNewAssetId, }); } @@ -206,31 +189,14 @@ export class AlbumService { await this.access.requirePermission(auth, Permission.ALBUM_READ, id); - const existingAssetIds = await this.albumRepository.getAssetIds(id, dto.ids); - const canRemove = await this.access.checkAccess(auth, Permission.ALBUM_REMOVE_ASSET, existingAssetIds); - const canShare = await this.access.checkAccess(auth, Permission.ASSET_SHARE, existingAssetIds); - const allowedAssetIds = setUnion(canRemove, canShare); - - const results: BulkIdResponseDto[] = []; - for (const assetId of dto.ids) { - const hasAsset = existingAssetIds.has(assetId); - if (!hasAsset) { - results.push({ id: assetId, success: false, error: BulkIdErrorReason.NOT_FOUND }); - continue; - } - - const hasAccess = allowedAssetIds.has(assetId); - if (!hasAccess) { - results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION }); - continue; - } - - results.push({ id: assetId, success: true }); - } + const results = await removeAssets( + auth, + { accessRepository: this.accessRepository, repository: this.albumRepository }, + { id, assetIds: dto.ids, permissions: [Permission.ASSET_SHARE, Permission.ALBUM_REMOVE_ASSET] }, + ); const removedIds = results.filter(({ success }) => success).map(({ id }) => id); if (removedIds.length > 0) { - await this.albumRepository.removeAssets(id, removedIds); await this.albumRepository.update({ id, updatedAt: new Date() }); if (album.albumThumbnailAssetId && removedIds.includes(album.albumThumbnailAssetId)) { await this.albumRepository.updateThumbnails(); diff --git a/server/src/utils/asset.util.ts b/server/src/utils/asset.util.ts new file mode 100644 index 0000000000..253073919f --- /dev/null +++ b/server/src/utils/asset.util.ts @@ -0,0 +1,91 @@ +import { AccessCore, Permission } from 'src/cores/access.core'; +import { BulkIdErrorReason, BulkIdResponseDto } from 'src/dtos/asset-ids.response.dto'; +import { AuthDto } from 'src/dtos/auth.dto'; +import { IAccessRepository } from 'src/interfaces/access.interface'; +import { setDifference, setUnion } from 'src/utils/set'; + +export interface IBulkAsset { + getAssetIds: (id: string, assetIds: string[]) => Promise>; + addAssetIds: (id: string, assetIds: string[]) => Promise; + removeAssetIds: (id: string, assetIds: string[]) => Promise; +} + +export const addAssets = async ( + auth: AuthDto, + repositories: { accessRepository: IAccessRepository; repository: IBulkAsset }, + dto: { id: string; assetIds: string[] }, +) => { + const { accessRepository, repository } = repositories; + const access = AccessCore.create(accessRepository); + + const existingAssetIds = await repository.getAssetIds(dto.id, dto.assetIds); + const notPresentAssetIds = dto.assetIds.filter((id) => !existingAssetIds.has(id)); + const allowedAssetIds = await access.checkAccess(auth, Permission.ASSET_SHARE, notPresentAssetIds); + + const results: BulkIdResponseDto[] = []; + for (const assetId of dto.assetIds) { + const hasAsset = existingAssetIds.has(assetId); + if (hasAsset) { + results.push({ id: assetId, success: false, error: BulkIdErrorReason.DUPLICATE }); + continue; + } + + const hasAccess = allowedAssetIds.has(assetId); + if (!hasAccess) { + results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION }); + continue; + } + + results.push({ id: assetId, success: true }); + } + + const newAssetIds = results.filter(({ success }) => success).map(({ id }) => id); + if (newAssetIds.length > 0) { + await repository.addAssetIds(dto.id, newAssetIds); + } + + return results; +}; + +export const removeAssets = async ( + auth: AuthDto, + repositories: { accessRepository: IAccessRepository; repository: IBulkAsset }, + dto: { id: string; assetIds: string[]; permissions: Permission[] }, +) => { + const { accessRepository, repository } = repositories; + const access = AccessCore.create(accessRepository); + + const existingAssetIds = await repository.getAssetIds(dto.id, dto.assetIds); + let allowedAssetIds = new Set(); + let remainingAssetIds = existingAssetIds; + + for (const permission of dto.permissions) { + const newAssetIds = await access.checkAccess(auth, permission, setDifference(remainingAssetIds, allowedAssetIds)); + remainingAssetIds = setDifference(remainingAssetIds, newAssetIds); + allowedAssetIds = setUnion(allowedAssetIds, newAssetIds); + } + + const results: BulkIdResponseDto[] = []; + for (const assetId of dto.assetIds) { + const hasAsset = existingAssetIds.has(assetId); + if (!hasAsset) { + results.push({ id: assetId, success: false, error: BulkIdErrorReason.NOT_FOUND }); + continue; + } + + const hasAccess = allowedAssetIds.has(assetId); + if (!hasAccess) { + results.push({ id: assetId, success: false, error: BulkIdErrorReason.NO_PERMISSION }); + continue; + } + + results.push({ id: assetId, success: true }); + } + + const removedIds = results.filter(({ success }) => success).map(({ id }) => id); + if (removedIds.length > 0) { + await repository.removeAssetIds(dto.id, removedIds); + } + + return results; +}; diff --git a/server/test/repositories/album.repository.mock.ts b/server/test/repositories/album.repository.mock.ts index 626358d811..38db70e4b8 100644 --- a/server/test/repositories/album.repository.mock.ts +++ b/server/test/repositories/album.repository.mock.ts @@ -14,9 +14,9 @@ export const newAlbumRepositoryMock = (): jest.Mocked => { softDeleteAll: jest.fn(), deleteAll: jest.fn(), getAll: jest.fn(), - addAssets: jest.fn(), + addAssetIds: jest.fn(), removeAsset: jest.fn(), - removeAssets: jest.fn(), + removeAssetIds: jest.fn(), getAssetIds: jest.fn(), hasAsset: jest.fn(), create: jest.fn(),