Skip to content

Commit

Permalink
refactor(server): shared link asset access check (immich-app#2680)
Browse files Browse the repository at this point in the history
  • Loading branch information
jrasm91 authored Jun 7, 2023
1 parent d1b0b64 commit 284edd9
Show file tree
Hide file tree
Showing 9 changed files with 39 additions and 38 deletions.
6 changes: 3 additions & 3 deletions server/apps/immich/src/api-v1/asset/asset.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ describe('AssetService', () => {

assetRepositoryMock.getById.mockResolvedValue(asset1);
sharedLinkRepositoryMock.get.mockResolvedValue(null);
sharedLinkRepositoryMock.hasAssetAccess.mockResolvedValue(true);
accessMock.hasSharedLinkAssetAccess.mockResolvedValue(true);

await expect(sut.addAssetsToSharedLink(authDto, dto)).rejects.toBeInstanceOf(BadRequestException);

Expand All @@ -242,7 +242,7 @@ describe('AssetService', () => {

assetRepositoryMock.getById.mockResolvedValue(asset1);
sharedLinkRepositoryMock.get.mockResolvedValue(sharedLinkStub.valid);
sharedLinkRepositoryMock.hasAssetAccess.mockResolvedValue(true);
accessMock.hasSharedLinkAssetAccess.mockResolvedValue(true);
sharedLinkRepositoryMock.update.mockResolvedValue(sharedLinkStub.valid);

await expect(sut.addAssetsToSharedLink(authDto, dto)).resolves.toEqual(sharedLinkResponseStub.valid);
Expand All @@ -260,7 +260,7 @@ describe('AssetService', () => {

assetRepositoryMock.getById.mockResolvedValue(asset1);
sharedLinkRepositoryMock.get.mockResolvedValue(sharedLinkStub.valid);
sharedLinkRepositoryMock.hasAssetAccess.mockResolvedValue(true);
accessMock.hasSharedLinkAssetAccess.mockResolvedValue(true);
sharedLinkRepositoryMock.update.mockResolvedValue(sharedLinkStub.valid);

await expect(sut.removeAssetsFromSharedLink(authDto, dto)).resolves.toEqual(sharedLinkResponseStub.valid);
Expand Down
6 changes: 4 additions & 2 deletions server/apps/immich/src/api-v1/asset/asset.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -564,10 +564,12 @@ export class AssetService {
}

private async checkAssetsAccess(authUser: AuthUserDto, assetIds: string[], mustBeOwner = false) {
const sharedLinkId = authUser.sharedLinkId;

for (const assetId of assetIds) {
// Step 1: Check if asset is part of a public shared
if (authUser.sharedLinkId) {
const canAccess = await this.shareCore.hasAssetAccess(authUser.sharedLinkId, assetId);
if (sharedLinkId) {
const canAccess = await this.accessRepository.hasSharedLinkAssetAccess(sharedLinkId, assetId);
if (canAccess) {
continue;
}
Expand Down
1 change: 1 addition & 0 deletions server/libs/domain/src/access/access.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ export const IAccessRepository = 'IAccessRepository';
export interface IAccessRepository {
hasPartnerAccess(userId: string, partnerId: string): Promise<boolean>;
hasPartnerAssetAccess(userId: string, assetId: string): Promise<boolean>;
hasSharedLinkAssetAccess(userId: string, assetId: string): Promise<boolean>;
}
4 changes: 0 additions & 4 deletions server/libs/domain/src/shared-link/shared-link.core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ export class SharedLinkCore {
return this.repository.update({ ...link, assets: newAssets });
}

async hasAssetAccess(id: string, assetId: string): Promise<boolean> {
return this.repository.hasAssetAccess(id, assetId);
}

checkDownloadAccess(user: AuthUserDto) {
if (user.isPublicUser && !user.isAllowDownload) {
throw new ForbiddenException();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,4 @@ export interface ISharedLinkRepository {
create(entity: Omit<SharedLinkEntity, 'id' | 'user'>): Promise<SharedLinkEntity>;
update(entity: Partial<SharedLinkEntity>): Promise<SharedLinkEntity>;
remove(entity: SharedLinkEntity): Promise<void>;
hasAssetAccess(id: string, assetId: string): Promise<boolean>;
}
1 change: 1 addition & 0 deletions server/libs/domain/test/access.repository.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ export const newAccessRepositoryMock = (): jest.Mocked<IAccessRepository> => {
return {
hasPartnerAccess: jest.fn(),
hasPartnerAssetAccess: jest.fn(),
hasSharedLinkAssetAccess: jest.fn(),
};
};
1 change: 0 additions & 1 deletion server/libs/domain/test/shared-link.repository.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,5 @@ export const newSharedLinkRepositoryMock = (): jest.Mocked<ISharedLinkRepository
create: jest.fn(),
remove: jest.fn(),
update: jest.fn(),
hasAssetAccess: jest.fn(),
};
};
32 changes: 30 additions & 2 deletions server/libs/infra/src/repositories/access.repository.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import { IAccessRepository } from '@app/domain';
import { InjectRepository } from '@nestjs/typeorm';
import { Repository } from 'typeorm';
import { PartnerEntity } from '../entities';
import { PartnerEntity, SharedLinkEntity } from '../entities';

export class AccessRepository implements IAccessRepository {
constructor(@InjectRepository(PartnerEntity) private partnerRepository: Repository<PartnerEntity>) {}
constructor(
@InjectRepository(PartnerEntity) private partnerRepository: Repository<PartnerEntity>,
@InjectRepository(SharedLinkEntity) private sharedLinkRepository: Repository<SharedLinkEntity>,
) {}

hasPartnerAccess(userId: string, partnerId: string): Promise<boolean> {
return this.partnerRepository.exist({
Expand Down Expand Up @@ -35,4 +38,29 @@ export class AccessRepository implements IAccessRepository {
},
});
}

async hasSharedLinkAssetAccess(sharedLinkId: string, assetId: string): Promise<boolean> {
return (
// album asset
(await this.sharedLinkRepository.exist({
where: {
id: sharedLinkId,
album: {
assets: {
id: assetId,
},
},
},
})) ||
// individual asset
(await this.sharedLinkRepository.exist({
where: {
id: sharedLinkId,
assets: {
id: assetId,
},
},
}))
);
}
}
25 changes: 0 additions & 25 deletions server/libs/infra/src/repositories/shared-link.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,31 +86,6 @@ export class SharedLinkRepository implements ISharedLinkRepository {
await this.repository.remove(entity);
}

async hasAssetAccess(id: string, assetId: string): Promise<boolean> {
return (
// album asset
(await this.repository.exist({
where: {
id,
album: {
assets: {
id: assetId,
},
},
},
})) ||
// individual asset
(await this.repository.exist({
where: {
id,
assets: {
id: assetId,
},
},
}))
);
}

private async save(entity: Partial<SharedLinkEntity>): Promise<SharedLinkEntity> {
await this.repository.save(entity);
return this.repository.findOneOrFail({ where: { id: entity.id } });
Expand Down

0 comments on commit 284edd9

Please sign in to comment.