Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(server): Check asset permissions in bulk #5329

Merged
merged 1 commit into from
Dec 2, 2023
Merged

chore(server): Check asset permissions in bulk #5329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 72 additions & 79 deletions server/src/domain/access/access.core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { BadRequestException, UnauthorizedException } from '@nestjs/common';
import { AuthUserDto } from '../auth';
import { setDifference, setUnion } from '../domain.util';
import { setDifference, setIsEqual, setUnion } from '../domain.util';
import { IAccessRepository } from '../repositories';

export enum Permission {
Expand Down Expand Up @@ -76,7 +76,7 @@ export class AccessCore {
async requirePermission(authUser: AuthUserDto, permission: Permission, ids: string[] | string) {
ids = Array.isArray(ids) ? ids : [ids];
const allowedIds = await this.checkAccess(authUser, permission, ids);
if (new Set(ids).size !== allowedIds.size) {
if (!setIsEqual(new Set(ids), allowedIds)) {
throw new BadRequestException(`Not found or no ${permission} access`);
}
}
Expand Down Expand Up @@ -106,70 +106,98 @@ export class AccessCore {
}

switch (permission) {
case Permission.ASSET_READ:
return await this.repository.asset.checkSharedLinkAccess(sharedLinkId, ids);

case Permission.ASSET_VIEW:
return await this.repository.asset.checkSharedLinkAccess(sharedLinkId, ids);

case Permission.ASSET_DOWNLOAD:
return !!authUser.isAllowDownload
? await this.repository.asset.checkSharedLinkAccess(sharedLinkId, ids)
: new Set();

case Permission.ASSET_UPLOAD:
return authUser.isAllowUpload ? ids : new Set();

case Permission.ASSET_SHARE:
// TODO: fix this to not use authUser.id for shared link access control
return await this.repository.asset.checkOwnerAccess(authUser.id, ids);

case Permission.ALBUM_READ:
return await this.repository.album.checkSharedLinkAccess(sharedLinkId, ids);

case Permission.ALBUM_DOWNLOAD:
return !!authUser.isAllowDownload
? await this.repository.album.checkSharedLinkAccess(sharedLinkId, ids)
: new Set();
}

const allowedIds = new Set();
for (const id of ids) {
const hasAccess = await this.hasSharedLinkAccess(authUser, permission, id);
if (hasAccess) {
allowedIds.add(id);
}
default:
return new Set();
}
return allowedIds;
}

// TODO: Migrate logic to checkAccessSharedLink to evaluate permissions in bulk.
private async hasSharedLinkAccess(authUser: AuthUserDto, permission: Permission, id: string) {
const sharedLinkId = authUser.sharedLinkId;
if (!sharedLinkId) {
return false;
}

private async checkAccessOther(authUser: AuthUserDto, permission: Permission, ids: Set<string>) {
switch (permission) {
case Permission.ASSET_READ:
return this.repository.asset.hasSharedLinkAccess(sharedLinkId, id);
case Permission.ASSET_READ: {
const isOwner = await this.repository.asset.checkOwnerAccess(authUser.id, ids);
const isAlbum = await this.repository.asset.checkAlbumAccess(authUser.id, setDifference(ids, isOwner));
const isPartner = await this.repository.asset.checkPartnerAccess(
authUser.id,
setDifference(ids, isOwner, isAlbum),
);
return setUnion(isOwner, isAlbum, isPartner);
}

case Permission.ASSET_VIEW:
return await this.repository.asset.hasSharedLinkAccess(sharedLinkId, id);
case Permission.ASSET_SHARE: {
const isOwner = await this.repository.asset.checkOwnerAccess(authUser.id, ids);
const isPartner = await this.repository.asset.checkPartnerAccess(authUser.id, setDifference(ids, isOwner));
return setUnion(isOwner, isPartner);
}

case Permission.ASSET_DOWNLOAD:
return !!authUser.isAllowDownload && (await this.repository.asset.hasSharedLinkAccess(sharedLinkId, id));
case Permission.ASSET_VIEW: {
const isOwner = await this.repository.asset.checkOwnerAccess(authUser.id, ids);
const isAlbum = await this.repository.asset.checkAlbumAccess(authUser.id, setDifference(ids, isOwner));
const isPartner = await this.repository.asset.checkPartnerAccess(
authUser.id,
setDifference(ids, isOwner, isAlbum),
);
return setUnion(isOwner, isAlbum, isPartner);
}

case Permission.ASSET_SHARE:
// TODO: fix this to not use authUser.id for shared link access control
return this.repository.asset.hasOwnerAccess(authUser.id, id);
case Permission.ASSET_DOWNLOAD: {
const isOwner = await this.repository.asset.checkOwnerAccess(authUser.id, ids);
const isAlbum = await this.repository.asset.checkAlbumAccess(authUser.id, setDifference(ids, isOwner));
const isPartner = await this.repository.asset.checkPartnerAccess(
authUser.id,
setDifference(ids, isOwner, isAlbum),
);
return setUnion(isOwner, isAlbum, isPartner);
}

default:
return false;
}
}
case Permission.ASSET_UPDATE:
return await this.repository.asset.checkOwnerAccess(authUser.id, ids);

case Permission.ASSET_DELETE:
return await this.repository.asset.checkOwnerAccess(authUser.id, ids);

case Permission.ASSET_RESTORE:
return await this.repository.asset.checkOwnerAccess(authUser.id, ids);

private async checkAccessOther(authUser: AuthUserDto, permission: Permission, ids: Set<string>) {
switch (permission) {
case Permission.ALBUM_READ: {
const isOwner = await this.repository.album.checkOwnerAccess(authUser.id, ids);
const isShared = await this.repository.album.checkSharedAlbumAccess(authUser.id, setDifference(ids, isOwner));
return setUnion(isOwner, isShared);
}

case Permission.ALBUM_UPDATE:
return this.repository.album.checkOwnerAccess(authUser.id, ids);
return await this.repository.album.checkOwnerAccess(authUser.id, ids);

case Permission.ALBUM_DELETE:
return this.repository.album.checkOwnerAccess(authUser.id, ids);
return await this.repository.album.checkOwnerAccess(authUser.id, ids);

case Permission.ALBUM_SHARE:
return this.repository.album.checkOwnerAccess(authUser.id, ids);
return await this.repository.album.checkOwnerAccess(authUser.id, ids);

case Permission.ALBUM_DOWNLOAD: {
const isOwner = await this.repository.album.checkOwnerAccess(authUser.id, ids);
Expand All @@ -178,16 +206,16 @@ export class AccessCore {
}

case Permission.ALBUM_REMOVE_ASSET:
return this.repository.album.checkOwnerAccess(authUser.id, ids);
return await this.repository.album.checkOwnerAccess(authUser.id, ids);

case Permission.ASSET_UPLOAD:
return this.repository.library.checkOwnerAccess(authUser.id, ids);
return await this.repository.library.checkOwnerAccess(authUser.id, ids);

case Permission.ARCHIVE_READ:
return ids.has(authUser.id) ? new Set([authUser.id]) : new Set();

case Permission.AUTH_DEVICE_DELETE:
return this.repository.authDevice.checkOwnerAccess(authUser.id, ids);
return await this.repository.authDevice.checkOwnerAccess(authUser.id, ids);

case Permission.TIMELINE_READ: {
const isOwner = ids.has(authUser.id) ? new Set([authUser.id]) : new Set<string>();
Expand All @@ -205,22 +233,22 @@ export class AccessCore {
}

case Permission.LIBRARY_UPDATE:
return this.repository.library.checkOwnerAccess(authUser.id, ids);
return await this.repository.library.checkOwnerAccess(authUser.id, ids);

case Permission.LIBRARY_DELETE:
return this.repository.library.checkOwnerAccess(authUser.id, ids);
return await this.repository.library.checkOwnerAccess(authUser.id, ids);

case Permission.PERSON_READ:
return this.repository.person.checkOwnerAccess(authUser.id, ids);
return await this.repository.person.checkOwnerAccess(authUser.id, ids);

case Permission.PERSON_WRITE:
return this.repository.person.checkOwnerAccess(authUser.id, ids);
return await this.repository.person.checkOwnerAccess(authUser.id, ids);

case Permission.PERSON_MERGE:
return this.repository.person.checkOwnerAccess(authUser.id, ids);
return await this.repository.person.checkOwnerAccess(authUser.id, ids);

case Permission.PARTNER_UPDATE:
return this.repository.partner.checkUpdateAccess(authUser.id, ids);
return await this.repository.partner.checkUpdateAccess(authUser.id, ids);
}

const allowedIds = new Set();
Expand All @@ -247,41 +275,6 @@ export class AccessCore {
(await this.repository.activity.hasAlbumOwnerAccess(authUser.id, id))
);

case Permission.ASSET_READ:
return (
(await this.repository.asset.hasOwnerAccess(authUser.id, id)) ||
(await this.repository.asset.hasAlbumAccess(authUser.id, id)) ||
(await this.repository.asset.hasPartnerAccess(authUser.id, id))
);
case Permission.ASSET_UPDATE:
return this.repository.asset.hasOwnerAccess(authUser.id, id);

case Permission.ASSET_DELETE:
return this.repository.asset.hasOwnerAccess(authUser.id, id);

case Permission.ASSET_RESTORE:
return this.repository.asset.hasOwnerAccess(authUser.id, id);

case Permission.ASSET_SHARE:
return (
(await this.repository.asset.hasOwnerAccess(authUser.id, id)) ||
(await this.repository.asset.hasPartnerAccess(authUser.id, id))
);

case Permission.ASSET_VIEW:
return (
(await this.repository.asset.hasOwnerAccess(authUser.id, id)) ||
(await this.repository.asset.hasAlbumAccess(authUser.id, id)) ||
(await this.repository.asset.hasPartnerAccess(authUser.id, id))
);

case Permission.ASSET_DOWNLOAD:
return (
(await this.repository.asset.hasOwnerAccess(authUser.id, id)) ||
(await this.repository.asset.hasAlbumAccess(authUser.id, id)) ||
(await this.repository.asset.hasPartnerAccess(authUser.id, id))
);

default:
return false;
}
Expand Down
21 changes: 9 additions & 12 deletions server/src/domain/album/album.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ describe(AlbumService.name, () => {
describe('addAssets', () => {
it('should allow the owner to add assets', async () => {
accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123']));
accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
albumMock.getAssetIds.mockResolvedValueOnce(new Set());

Expand All @@ -534,7 +534,7 @@ describe(AlbumService.name, () => {

it('should not set the thumbnail if the album has one already', async () => {
accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123']));
accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1']));
albumMock.getById.mockResolvedValue(_.cloneDeep({ ...albumStub.empty, albumThumbnailAssetId: 'asset-id' }));
albumMock.getAssetIds.mockResolvedValueOnce(new Set());

Expand All @@ -552,7 +552,7 @@ describe(AlbumService.name, () => {

it('should allow a shared user to add assets', async () => {
accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123']));
accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.sharedWithUser));
albumMock.getAssetIds.mockResolvedValueOnce(new Set());

Expand All @@ -577,7 +577,7 @@ describe(AlbumService.name, () => {

it('should allow a shared link user to add assets', async () => {
accessMock.album.checkSharedLinkAccess.mockResolvedValue(new Set(['album-123']));
accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3']));
albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
albumMock.getAssetIds.mockResolvedValueOnce(new Set());

Expand Down Expand Up @@ -607,8 +607,7 @@ describe(AlbumService.name, () => {

it('should allow adding assets shared via partner sharing', async () => {
accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123']));
accessMock.asset.hasOwnerAccess.mockResolvedValue(false);
accessMock.asset.hasPartnerAccess.mockResolvedValue(true);
accessMock.asset.checkPartnerAccess.mockResolvedValue(new Set(['asset-1']));
albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
albumMock.getAssetIds.mockResolvedValueOnce(new Set());

Expand All @@ -621,12 +620,12 @@ describe(AlbumService.name, () => {
updatedAt: expect.any(Date),
albumThumbnailAssetId: 'asset-1',
});
expect(accessMock.asset.hasPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1');
expect(accessMock.asset.checkPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['asset-1']));
});

it('should skip duplicate assets', async () => {
accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123']));
accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-id']));
albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset));
albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id']));

Expand All @@ -639,17 +638,15 @@ describe(AlbumService.name, () => {

it('should skip assets not shared with user', async () => {
accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123']));
accessMock.asset.hasOwnerAccess.mockResolvedValue(false);
accessMock.asset.hasPartnerAccess.mockResolvedValue(false);
albumMock.getById.mockResolvedValue(albumStub.oneAsset);
albumMock.getAssetIds.mockResolvedValueOnce(new Set());

await expect(sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-1'] })).resolves.toEqual([
{ success: false, id: 'asset-1', error: BulkIdErrorReason.NO_PERMISSION },
]);

expect(accessMock.asset.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1');
expect(accessMock.asset.hasPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1');
expect(accessMock.asset.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['asset-1']));
expect(accessMock.asset.checkPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['asset-1']));
});

it('should not allow unauthorized access to the album', async () => {
Expand Down
Loading
Loading