From eb1225a0a51d705091d9cd18d2e2030a1506ef06 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Wed, 7 Jun 2023 10:37:25 -0400 Subject: [PATCH] refactor(server,web): add/remove album users (#2681) * refactor(server,web): add/remove album users * fix(web): bug fixes for multiple users * fix: linting --- .../src/api-v1/album/album-repository.ts | 28 +-- .../src/api-v1/album/album.controller.ts | 17 -- .../src/api-v1/album/album.service.spec.ts | 71 +----- .../immich/src/api-v1/album/album.service.ts | 19 -- .../src/api-v1/album/dto/add-users.dto.ts | 2 +- .../src/controllers/album.controller.ts | 19 +- server/immich-openapi-specs.json | 220 +++++++++--------- .../domain/src/album/album.service.spec.ts | 154 +++++++++++- server/libs/domain/src/album/album.service.ts | 94 +++++++- .../src/album/dto/album-add-users.dto.ts | 8 + server/libs/domain/src/album/dto/index.ts | 1 + server/libs/domain/test/fixtures.ts | 38 +++ .../src/repositories/album.repository.ts | 9 +- .../components/album-page/album-viewer.svelte | 10 +- .../album-page/share-info-modal.svelte | 160 +++++++------ 15 files changed, 521 insertions(+), 329 deletions(-) create mode 100644 server/libs/domain/src/album/dto/album-add-users.dto.ts diff --git a/server/apps/immich/src/api-v1/album/album-repository.ts b/server/apps/immich/src/api-v1/album/album-repository.ts index e52f5178c0..b32f83a96e 100644 --- a/server/apps/immich/src/api-v1/album/album-repository.ts +++ b/server/apps/immich/src/api-v1/album/album-repository.ts @@ -1,18 +1,15 @@ -import { AlbumEntity, AssetEntity, UserEntity } from '@app/infra/entities'; +import { AlbumEntity, AssetEntity } from '@app/infra/entities'; import { dataSource } from '@app/infra/database.config'; import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { AddAssetsDto } from './dto/add-assets.dto'; -import { AddUsersDto } from './dto/add-users.dto'; import { RemoveAssetsDto } from './dto/remove-assets.dto'; import { AlbumCountResponseDto } from './response-dto/album-count-response.dto'; import { AddAssetsResponseDto } from './response-dto/add-assets-response.dto'; export interface IAlbumRepository { get(albumId: string): Promise; - addSharedUsers(album: AlbumEntity, addUsersDto: AddUsersDto): Promise; - removeUser(album: AlbumEntity, userId: string): Promise; removeAssets(album: AlbumEntity, removeAssets: RemoveAssetsDto): Promise; addAssets(album: AlbumEntity, addAssetsDto: AddAssetsDto): Promise; updateThumbnails(): Promise; @@ -25,11 +22,8 @@ export const IAlbumRepository = 'IAlbumRepository'; @Injectable() export class AlbumRepository implements IAlbumRepository { constructor( - @InjectRepository(AlbumEntity) - private albumRepository: Repository, - - @InjectRepository(AssetEntity) - private assetRepository: Repository, + @InjectRepository(AlbumEntity) private albumRepository: Repository, + @InjectRepository(AssetEntity) private assetRepository: Repository, ) {} async getCountByUserId(userId: string): Promise { @@ -59,22 +53,6 @@ export class AlbumRepository implements IAlbumRepository { }); } - async addSharedUsers(album: AlbumEntity, addUsersDto: AddUsersDto): Promise { - album.sharedUsers.push(...addUsersDto.sharedUserIds.map((id) => ({ id } as UserEntity))); - album.updatedAt = new Date(); - - await this.albumRepository.save(album); - - // need to re-load the shared user relation - return this.get(album.id) as Promise; - } - - async removeUser(album: AlbumEntity, userId: string): Promise { - album.sharedUsers = album.sharedUsers.filter((user) => user.id !== userId); - album.updatedAt = new Date(); - await this.albumRepository.save(album); - } - async removeAssets(album: AlbumEntity, removeAssetsDto: RemoveAssetsDto): Promise { const assetCount = album.assets.length; diff --git a/server/apps/immich/src/api-v1/album/album.controller.ts b/server/apps/immich/src/api-v1/album/album.controller.ts index 49f8ec7d2b..02d06280a6 100644 --- a/server/apps/immich/src/api-v1/album/album.controller.ts +++ b/server/apps/immich/src/api-v1/album/album.controller.ts @@ -1,10 +1,8 @@ import { Controller, Get, Post, Body, Param, Delete, Put, Query, Response } from '@nestjs/common'; -import { ParseMeUUIDPipe } from '../validation/parse-me-uuid-pipe'; import { AlbumService } from './album.service'; import { Authenticated, SharedLinkRoute } from '../../decorators/authenticated.decorator'; import { AuthUserDto, GetAuthUser } from '../../decorators/auth-user.decorator'; import { AddAssetsDto } from './dto/add-assets.dto'; -import { AddUsersDto } from './dto/add-users.dto'; import { RemoveAssetsDto } from './dto/remove-assets.dto'; import { ApiOkResponse, ApiTags } from '@nestjs/swagger'; import { AlbumResponseDto } from '@app/domain'; @@ -29,12 +27,6 @@ export class AlbumController { return this.service.getCountByUserId(authUser); } - @Put(':id/users') - addUsersToAlbum(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto, @Body() dto: AddUsersDto) { - // TODO: Handle nonexistent sharedUserIds. - return this.service.addUsers(authUser, id, dto); - } - @SharedLinkRoute() @Put(':id/assets') addAssetsToAlbum( @@ -62,15 +54,6 @@ export class AlbumController { return this.service.removeAssets(authUser, id, dto); } - @Delete(':id/user/:userId') - removeUserFromAlbum( - @GetAuthUser() authUser: AuthUserDto, - @Param() { id }: UUIDParamDto, - @Param('userId', new ParseMeUUIDPipe({ version: '4' })) userId: string, - ) { - return this.service.removeUser(authUser, id, userId); - } - @SharedLinkRoute() @Get(':id/download') @ApiOkResponse({ content: { 'application/zip': { schema: { type: 'string', format: 'binary' } } } }) diff --git a/server/apps/immich/src/api-v1/album/album.service.spec.ts b/server/apps/immich/src/api-v1/album/album.service.spec.ts index 84b9fad13b..d621e60356 100644 --- a/server/apps/immich/src/api-v1/album/album.service.spec.ts +++ b/server/apps/immich/src/api-v1/album/album.service.spec.ts @@ -1,6 +1,6 @@ import { AlbumService } from './album.service'; import { AuthUserDto } from '../../decorators/auth-user.decorator'; -import { BadRequestException, NotFoundException, ForbiddenException } from '@nestjs/common'; +import { NotFoundException, ForbiddenException } from '@nestjs/common'; import { AlbumEntity, UserEntity } from '@app/infra/entities'; import { AlbumResponseDto, ICryptoRepository, mapUser } from '@app/domain'; import { AddAssetsResponseDto } from './response-dto/add-assets-response.dto'; @@ -39,7 +39,6 @@ describe('Album service', () => { const albumId = 'f19ab956-4761-41ea-a5d6-bae948308d58'; const sharedAlbumOwnerId = '2222'; const sharedAlbumSharedAlsoWithId = '3333'; - const ownedAlbumSharedWithId = '4444'; const _getOwnedAlbum = () => { const albumEntity = new AlbumEntity(); @@ -56,25 +55,6 @@ describe('Album service', () => { return albumEntity; }; - const _getOwnedSharedAlbum = () => { - const albumEntity = new AlbumEntity(); - albumEntity.ownerId = albumOwner.id; - albumEntity.owner = albumOwner; - albumEntity.id = albumId; - albumEntity.albumName = 'name'; - albumEntity.createdAt = new Date('2022-06-19T23:41:36.910Z'); - albumEntity.assets = []; - albumEntity.albumThumbnailAssetId = null; - albumEntity.sharedUsers = [ - { - ...userEntityStub.user1, - id: ownedAlbumSharedWithId, - }, - ]; - - return albumEntity; - }; - const _getSharedWithAuthUserAlbum = () => { const albumEntity = new AlbumEntity(); albumEntity.ownerId = sharedAlbumOwnerId; @@ -115,10 +95,8 @@ describe('Album service', () => { beforeAll(() => { albumRepositoryMock = { addAssets: jest.fn(), - addSharedUsers: jest.fn(), get: jest.fn(), removeAssets: jest.fn(), - removeUser: jest.fn(), updateThumbnails: jest.fn(), getCountByUserId: jest.fn(), getSharedWithUserAlbumCount: jest.fn(), @@ -188,53 +166,6 @@ describe('Album service', () => { await expect(sut.get(authUser, '0002')).rejects.toBeInstanceOf(NotFoundException); }); - it('removes a shared user from an owned album', async () => { - const albumEntity = _getOwnedSharedAlbum(); - albumRepositoryMock.get.mockImplementation(() => Promise.resolve(albumEntity)); - albumRepositoryMock.removeUser.mockImplementation(() => Promise.resolve()); - await expect(sut.removeUser(authUser, albumEntity.id, ownedAlbumSharedWithId)).resolves.toBeUndefined(); - expect(albumRepositoryMock.removeUser).toHaveBeenCalledTimes(1); - expect(albumRepositoryMock.removeUser).toHaveBeenCalledWith(albumEntity, ownedAlbumSharedWithId); - }); - - it('prevents removing a shared user from a not owned album (shared with auth user)', async () => { - const albumEntity = _getSharedWithAuthUserAlbum(); - const albumId = albumEntity.id; - const userIdToRemove = sharedAlbumSharedAlsoWithId; - - albumRepositoryMock.get.mockImplementation(() => Promise.resolve(albumEntity)); - - await expect(sut.removeUser(authUser, albumId, userIdToRemove)).rejects.toBeInstanceOf(ForbiddenException); - expect(albumRepositoryMock.removeUser).not.toHaveBeenCalled(); - }); - - it('removes itself from a shared album', async () => { - const albumEntity = _getSharedWithAuthUserAlbum(); - albumRepositoryMock.get.mockImplementation(() => Promise.resolve(albumEntity)); - albumRepositoryMock.removeUser.mockImplementation(() => Promise.resolve()); - - await sut.removeUser(authUser, albumEntity.id, authUser.id); - expect(albumRepositoryMock.removeUser).toHaveReturnedTimes(1); - expect(albumRepositoryMock.removeUser).toHaveBeenCalledWith(albumEntity, authUser.id); - }); - - it('removes itself from a shared album using "me" as id', async () => { - const albumEntity = _getSharedWithAuthUserAlbum(); - albumRepositoryMock.get.mockImplementation(() => Promise.resolve(albumEntity)); - albumRepositoryMock.removeUser.mockImplementation(() => Promise.resolve()); - - await sut.removeUser(authUser, albumEntity.id, 'me'); - expect(albumRepositoryMock.removeUser).toHaveReturnedTimes(1); - expect(albumRepositoryMock.removeUser).toHaveBeenCalledWith(albumEntity, authUser.id); - }); - - it('prevents removing itself from a owned album', async () => { - const albumEntity = _getOwnedAlbum(); - albumRepositoryMock.get.mockImplementation(() => Promise.resolve(albumEntity)); - - await expect(sut.removeUser(authUser, albumEntity.id, authUser.id)).rejects.toBeInstanceOf(BadRequestException); - }); - it('adds assets to owned album', async () => { const albumEntity = _getOwnedAlbum(); diff --git a/server/apps/immich/src/api-v1/album/album.service.ts b/server/apps/immich/src/api-v1/album/album.service.ts index f8e09eb271..8db380a5ae 100644 --- a/server/apps/immich/src/api-v1/album/album.service.ts +++ b/server/apps/immich/src/api-v1/album/album.service.ts @@ -1,7 +1,6 @@ import { BadRequestException, Inject, Injectable, NotFoundException, ForbiddenException, Logger } from '@nestjs/common'; import { AuthUserDto } from '../../decorators/auth-user.decorator'; import { AlbumEntity, SharedLinkType } from '@app/infra/entities'; -import { AddUsersDto } from './dto/add-users.dto'; import { RemoveAssetsDto } from './dto/remove-assets.dto'; import { AlbumResponseDto, mapAlbum } from '@app/domain'; import { IAlbumRepository } from './album-repository'; @@ -63,24 +62,6 @@ export class AlbumService { return mapAlbum(album); } - async addUsers(authUser: AuthUserDto, albumId: string, dto: AddUsersDto): Promise { - const album = await this._getAlbum({ authUser, albumId }); - const updatedAlbum = await this.albumRepository.addSharedUsers(album, dto); - return mapAlbum(updatedAlbum); - } - - async removeUser(authUser: AuthUserDto, albumId: string, userId: string | 'me'): Promise { - const sharedUserId = userId == 'me' ? authUser.id : userId; - const album = await this._getAlbum({ authUser, albumId, validateIsOwner: false }); - if (album.ownerId != authUser.id && authUser.id != sharedUserId) { - throw new ForbiddenException('Cannot remove a user from a album that is not owned'); - } - if (album.ownerId == sharedUserId) { - throw new BadRequestException('The owner of the album cannot be removed'); - } - await this.albumRepository.removeUser(album, sharedUserId); - } - async removeAssets(authUser: AuthUserDto, albumId: string, dto: RemoveAssetsDto): Promise { const album = await this._getAlbum({ authUser, albumId }); const deletedCount = await this.albumRepository.removeAssets(album, dto); diff --git a/server/apps/immich/src/api-v1/album/dto/add-users.dto.ts b/server/apps/immich/src/api-v1/album/dto/add-users.dto.ts index c4f3c677fc..0f73e43eae 100644 --- a/server/apps/immich/src/api-v1/album/dto/add-users.dto.ts +++ b/server/apps/immich/src/api-v1/album/dto/add-users.dto.ts @@ -1,4 +1,4 @@ -import { ValidateUUID } from 'apps/immich/src/decorators/validate-uuid.decorator'; +import { ValidateUUID } from '../../../../../../apps/immich/src/decorators/validate-uuid.decorator'; export class AddUsersDto { @ValidateUUID({ each: true }) diff --git a/server/apps/immich/src/controllers/album.controller.ts b/server/apps/immich/src/controllers/album.controller.ts index abb9447f11..b00858a302 100644 --- a/server/apps/immich/src/controllers/album.controller.ts +++ b/server/apps/immich/src/controllers/album.controller.ts @@ -1,7 +1,8 @@ -/* */ import { AlbumService, AuthUserDto, CreateAlbumDto, UpdateAlbumDto } from '@app/domain'; +import { AddUsersDto, AlbumService, AuthUserDto, CreateAlbumDto, UpdateAlbumDto } from '@app/domain'; import { GetAlbumsDto } from '@app/domain/album/dto/get-albums.dto'; -import { Body, Controller, Delete, Get, Param, Patch, Post, Query } from '@nestjs/common'; +import { Body, Controller, Delete, Get, Param, Patch, Post, Put, Query } from '@nestjs/common'; import { ApiTags } from '@nestjs/swagger'; +import { ParseMeUUIDPipe } from '../api-v1/validation/parse-me-uuid-pipe'; import { GetAuthUser } from '../decorators/auth-user.decorator'; import { Authenticated } from '../decorators/authenticated.decorator'; import { UseValidation } from '../decorators/use-validation.decorator'; @@ -33,4 +34,18 @@ export class AlbumController { deleteAlbum(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto) { return this.service.delete(authUser, id); } + + @Put(':id/users') + addUsersToAlbum(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto, @Body() dto: AddUsersDto) { + return this.service.addUsers(authUser, id, dto); + } + + @Delete(':id/user/:userId') + removeUserFromAlbum( + @GetAuthUser() authUser: AuthUserDto, + @Param() { id }: UUIDParamDto, + @Param('userId', new ParseMeUUIDPipe({ version: '4' })) userId: string, + ) { + return this.service.removeUser(authUser, id, userId); + } } diff --git a/server/immich-openapi-specs.json b/server/immich-openapi-specs.json index e18f370892..e3bd376d0b 100644 --- a/server/immich-openapi-specs.json +++ b/server/immich-openapi-specs.json @@ -228,6 +228,101 @@ ] } }, + "/album/{id}/users": { + "put": { + "operationId": "addUsersToAlbum", + "parameters": [ + { + "name": "id", + "required": true, + "in": "path", + "schema": { + "format": "uuid", + "type": "string" + } + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/AddUsersDto" + } + } + } + }, + "responses": { + "200": { + "description": "", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/AlbumResponseDto" + } + } + } + } + }, + "tags": [ + "Album" + ], + "security": [ + { + "bearer": [] + }, + { + "cookie": [] + }, + { + "api_key": [] + } + ] + } + }, + "/album/{id}/user/{userId}": { + "delete": { + "operationId": "removeUserFromAlbum", + "parameters": [ + { + "name": "id", + "required": true, + "in": "path", + "schema": { + "format": "uuid", + "type": "string" + } + }, + { + "name": "userId", + "required": true, + "in": "path", + "schema": { + "type": "string" + } + } + ], + "responses": { + "200": { + "description": "" + } + }, + "tags": [ + "Album" + ], + "security": [ + { + "bearer": [] + }, + { + "cookie": [] + }, + { + "api_key": [] + } + ] + } + }, "/api-key": { "post": { "operationId": "createKey", @@ -3990,58 +4085,6 @@ ] } }, - "/album/{id}/users": { - "put": { - "operationId": "addUsersToAlbum", - "parameters": [ - { - "name": "id", - "required": true, - "in": "path", - "schema": { - "format": "uuid", - "type": "string" - } - } - ], - "requestBody": { - "required": true, - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/AddUsersDto" - } - } - } - }, - "responses": { - "200": { - "description": "", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/AlbumResponseDto" - } - } - } - } - }, - "tags": [ - "Album" - ], - "security": [ - { - "bearer": [] - }, - { - "cookie": [] - }, - { - "api_key": [] - } - ] - } - }, "/album/{id}/assets": { "put": { "operationId": "addAssetsToAlbum", @@ -4152,49 +4195,6 @@ ] } }, - "/album/{id}/user/{userId}": { - "delete": { - "operationId": "removeUserFromAlbum", - "parameters": [ - { - "name": "id", - "required": true, - "in": "path", - "schema": { - "format": "uuid", - "type": "string" - } - }, - { - "name": "userId", - "required": true, - "in": "path", - "schema": { - "type": "string" - } - } - ], - "responses": { - "200": { - "description": "" - } - }, - "tags": [ - "Album" - ], - "security": [ - { - "bearer": [] - }, - { - "cookie": [] - }, - { - "api_key": [] - } - ] - } - }, "/album/{id}/download": { "get": { "operationId": "downloadArchive", @@ -4778,6 +4778,21 @@ } } }, + "AddUsersDto": { + "type": "object", + "properties": { + "sharedUserIds": { + "type": "array", + "items": { + "type": "string", + "format": "uuid" + } + } + }, + "required": [ + "sharedUserIds" + ] + }, "APIKeyCreateDto": { "type": "object", "properties": { @@ -6620,21 +6635,6 @@ "sharing" ] }, - "AddUsersDto": { - "type": "object", - "properties": { - "sharedUserIds": { - "type": "array", - "items": { - "type": "string", - "format": "uuid" - } - } - }, - "required": [ - "sharedUserIds" - ] - }, "AddAssetsResponseDto": { "type": "object", "properties": { diff --git a/server/libs/domain/src/album/album.service.spec.ts b/server/libs/domain/src/album/album.service.spec.ts index 005b8e0b10..e15ad31b3b 100644 --- a/server/libs/domain/src/album/album.service.spec.ts +++ b/server/libs/domain/src/album/album.service.spec.ts @@ -1,7 +1,17 @@ import { BadRequestException, ForbiddenException } from '@nestjs/common'; -import { albumStub, authStub, newAlbumRepositoryMock, newAssetRepositoryMock, newJobRepositoryMock } from '../../test'; +import _ from 'lodash'; +import { + albumStub, + authStub, + newAlbumRepositoryMock, + newAssetRepositoryMock, + newJobRepositoryMock, + newUserRepositoryMock, + userEntityStub, +} from '../../test'; import { IAssetRepository } from '../asset'; import { IJobRepository, JobName } from '../job'; +import { IUserRepository } from '../user'; import { IAlbumRepository } from './album.repository'; import { AlbumService } from './album.service'; @@ -10,13 +20,15 @@ describe(AlbumService.name, () => { let albumMock: jest.Mocked; let assetMock: jest.Mocked; let jobMock: jest.Mocked; + let userMock: jest.Mocked; beforeEach(async () => { albumMock = newAlbumRepositoryMock(); assetMock = newAssetRepositoryMock(); jobMock = newJobRepositoryMock(); + userMock = newUserRepositoryMock(); - sut = new AlbumService(albumMock, assetMock, jobMock); + sut = new AlbumService(albumMock, assetMock, jobMock, userMock); }); it('should work', () => { @@ -152,6 +164,18 @@ describe(AlbumService.name, () => { data: { ids: [albumStub.empty.id] }, }); }); + + it('should require valid userIds', async () => { + userMock.get.mockResolvedValue(null); + await expect( + sut.create(authStub.admin, { + albumName: 'Empty album', + sharedWithUserIds: ['user-3'], + }), + ).rejects.toBeInstanceOf(BadRequestException); + expect(userMock.get).toHaveBeenCalledWith('user-3'); + expect(albumMock.create).not.toHaveBeenCalled(); + }); }); describe('update', () => { @@ -240,4 +264,130 @@ describe(AlbumService.name, () => { expect(albumMock.delete).toHaveBeenCalledWith(albumStub.empty); }); }); + + describe('addUsers', () => { + it('should require a valid album id', async () => { + albumMock.getByIds.mockResolvedValue([]); + await expect(sut.addUsers(authStub.admin, 'album-1', { sharedUserIds: ['user-1'] })).rejects.toBeInstanceOf( + BadRequestException, + ); + expect(albumMock.update).not.toHaveBeenCalled(); + }); + + it('should require the user to be the owner', async () => { + albumMock.getByIds.mockResolvedValue([albumStub.sharedWithAdmin]); + await expect( + sut.addUsers(authStub.admin, albumStub.sharedWithAdmin.id, { sharedUserIds: ['user-1'] }), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(albumMock.update).not.toHaveBeenCalled(); + }); + + it('should throw an error if the userId is already added', async () => { + albumMock.getByIds.mockResolvedValue([albumStub.sharedWithAdmin]); + await expect( + sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, { sharedUserIds: [authStub.admin.id] }), + ).rejects.toBeInstanceOf(BadRequestException); + expect(albumMock.update).not.toHaveBeenCalled(); + }); + + it('should throw an error if the userId does not exist', async () => { + albumMock.getByIds.mockResolvedValue([albumStub.sharedWithAdmin]); + userMock.get.mockResolvedValue(null); + await expect( + sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, { sharedUserIds: ['user-3'] }), + ).rejects.toBeInstanceOf(BadRequestException); + expect(albumMock.update).not.toHaveBeenCalled(); + }); + + it('should add valid shared users', async () => { + albumMock.getByIds.mockResolvedValue([_.cloneDeep(albumStub.sharedWithAdmin)]); + albumMock.update.mockResolvedValue(albumStub.sharedWithAdmin); + userMock.get.mockResolvedValue(userEntityStub.user2); + await sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, { sharedUserIds: [authStub.user2.id] }); + expect(albumMock.update).toHaveBeenCalledWith({ + id: albumStub.sharedWithAdmin.id, + updatedAt: expect.any(Date), + sharedUsers: [userEntityStub.admin, { id: authStub.user2.id }], + }); + }); + }); + + describe('removeUser', () => { + it('should require a valid album id', async () => { + albumMock.getByIds.mockResolvedValue([]); + await expect(sut.removeUser(authStub.admin, 'album-1', 'user-1')).rejects.toBeInstanceOf(BadRequestException); + expect(albumMock.update).not.toHaveBeenCalled(); + }); + + it('should remove a shared user from an owned album', async () => { + albumMock.getByIds.mockResolvedValue([albumStub.sharedWithUser]); + + await expect( + sut.removeUser(authStub.admin, albumStub.sharedWithUser.id, userEntityStub.user1.id), + ).resolves.toBeUndefined(); + + expect(albumMock.update).toHaveBeenCalledTimes(1); + expect(albumMock.update).toHaveBeenCalledWith({ + id: albumStub.sharedWithUser.id, + updatedAt: expect.any(Date), + sharedUsers: [], + }); + }); + + it('should prevent removing a shared user from a not-owned album (shared with auth user)', async () => { + albumMock.getByIds.mockResolvedValue([albumStub.sharedWithMultiple]); + + await expect( + sut.removeUser(authStub.user1, albumStub.sharedWithMultiple.id, authStub.user2.id), + ).rejects.toBeInstanceOf(ForbiddenException); + + expect(albumMock.update).not.toHaveBeenCalled(); + }); + + it('should allow a shared user to remove themselves', async () => { + albumMock.getByIds.mockResolvedValue([albumStub.sharedWithUser]); + + await sut.removeUser(authStub.user1, albumStub.sharedWithUser.id, authStub.user1.id); + + expect(albumMock.update).toHaveBeenCalledTimes(1); + expect(albumMock.update).toHaveBeenCalledWith({ + id: albumStub.sharedWithUser.id, + updatedAt: expect.any(Date), + sharedUsers: [], + }); + }); + + it('should allow a shared user to remove themselves using "me"', async () => { + albumMock.getByIds.mockResolvedValue([albumStub.sharedWithUser]); + + await sut.removeUser(authStub.user1, albumStub.sharedWithUser.id, 'me'); + + expect(albumMock.update).toHaveBeenCalledTimes(1); + expect(albumMock.update).toHaveBeenCalledWith({ + id: albumStub.sharedWithUser.id, + updatedAt: expect.any(Date), + sharedUsers: [], + }); + }); + + it('should not allow the owner to be removed', async () => { + albumMock.getByIds.mockResolvedValue([albumStub.empty]); + + await expect(sut.removeUser(authStub.admin, albumStub.empty.id, authStub.admin.id)).rejects.toBeInstanceOf( + BadRequestException, + ); + + expect(albumMock.update).not.toHaveBeenCalled(); + }); + + it('should throw an error for a user not in the album', async () => { + albumMock.getByIds.mockResolvedValue([albumStub.empty]); + + await expect(sut.removeUser(authStub.admin, albumStub.empty.id, 'user-3')).rejects.toBeInstanceOf( + BadRequestException, + ); + + expect(albumMock.update).not.toHaveBeenCalled(); + }); + }); }); diff --git a/server/libs/domain/src/album/album.service.ts b/server/libs/domain/src/album/album.service.ts index 12217f9c8f..6bad729531 100644 --- a/server/libs/domain/src/album/album.service.ts +++ b/server/libs/domain/src/album/album.service.ts @@ -3,8 +3,9 @@ import { BadRequestException, ForbiddenException, Inject, Injectable } from '@ne import { IAssetRepository, mapAsset } from '../asset'; import { AuthUserDto } from '../auth'; import { IJobRepository, JobName } from '../job'; +import { IUserRepository } from '../user'; import { IAlbumRepository } from './album.repository'; -import { CreateAlbumDto, GetAlbumsDto, UpdateAlbumDto } from './dto'; +import { AddUsersDto, CreateAlbumDto, GetAlbumsDto, UpdateAlbumDto } from './dto'; import { AlbumResponseDto, mapAlbum } from './response-dto'; @Injectable() @@ -13,6 +14,7 @@ export class AlbumService { @Inject(IAlbumRepository) private albumRepository: IAlbumRepository, @Inject(IAssetRepository) private assetRepository: IAssetRepository, @Inject(IJobRepository) private jobRepository: IJobRepository, + @Inject(IUserRepository) private userRepository: IUserRepository, ) {} async getAll({ id: ownerId }: AuthUserDto, { assetId, shared }: GetAlbumsDto): Promise { @@ -48,7 +50,7 @@ export class AlbumService { }); } - async updateInvalidThumbnails(): Promise { + private async updateInvalidThumbnails(): Promise { const invalidAlbumIds = await this.albumRepository.getInvalidThumbnail(); for (const albumId of invalidAlbumIds) { @@ -60,7 +62,13 @@ export class AlbumService { } async create(authUser: AuthUserDto, dto: CreateAlbumDto): Promise { - // TODO: Handle nonexistent sharedWithUserIds and assetIds. + for (const userId of dto.sharedWithUserIds || []) { + const exists = await this.userRepository.get(userId); + if (!exists) { + throw new BadRequestException('User not found'); + } + } + const album = await this.albumRepository.create({ ownerId: authUser.id, albumName: dto.albumName, @@ -68,19 +76,14 @@ export class AlbumService { assets: (dto.assetIds || []).map((id) => ({ id } as AssetEntity)), albumThumbnailAssetId: dto.assetIds?.[0] || null, }); + await this.jobRepository.queue({ name: JobName.SEARCH_INDEX_ALBUM, data: { ids: [album.id] } }); return mapAlbum(album); } async update(authUser: AuthUserDto, id: string, dto: UpdateAlbumDto): Promise { - const [album] = await this.albumRepository.getByIds([id]); - if (!album) { - throw new BadRequestException('Album not found'); - } - - if (album.ownerId !== authUser.id) { - throw new ForbiddenException('Album not owned by user'); - } + const album = await this.get(id); + this.assertOwner(authUser, album); if (dto.albumThumbnailAssetId) { const valid = await this.albumRepository.hasAsset(id, dto.albumThumbnailAssetId); @@ -113,4 +116,73 @@ export class AlbumService { await this.albumRepository.delete(album); await this.jobRepository.queue({ name: JobName.SEARCH_REMOVE_ALBUM, data: { ids: [id] } }); } + + async addUsers(authUser: AuthUserDto, id: string, dto: AddUsersDto) { + const album = await this.get(id); + this.assertOwner(authUser, album); + + for (const userId of dto.sharedUserIds) { + const exists = album.sharedUsers.find((user) => user.id === userId); + if (exists) { + throw new BadRequestException('User already added'); + } + + const user = await this.userRepository.get(userId); + if (!user) { + throw new BadRequestException('User not found'); + } + + album.sharedUsers.push({ id: userId } as UserEntity); + } + + return this.albumRepository + .update({ + id: album.id, + updatedAt: new Date(), + sharedUsers: album.sharedUsers, + }) + .then(mapAlbum); + } + + async removeUser(authUser: AuthUserDto, id: string, userId: string | 'me'): Promise { + if (userId === 'me') { + userId = authUser.id; + } + + const album = await this.get(id); + + if (album.ownerId === userId) { + throw new BadRequestException('Cannot remove album owner'); + } + + const exists = album.sharedUsers.find((user) => user.id === userId); + if (!exists) { + throw new BadRequestException('Album not shared with user'); + } + + // non-admin can remove themselves + if (authUser.id !== userId) { + this.assertOwner(authUser, album); + } + + await this.albumRepository.update({ + id: album.id, + updatedAt: new Date(), + sharedUsers: album.sharedUsers.filter((user) => user.id !== userId), + }); + } + + private async get(id: string) { + const [album] = await this.albumRepository.getByIds([id]); + if (!album) { + throw new BadRequestException('Album not found'); + } + return album; + } + + private assertOwner(authUser: AuthUserDto, album: AlbumEntity) { + if (album.ownerId !== authUser.id) { + throw new ForbiddenException('Album not owned by user'); + } + } } diff --git a/server/libs/domain/src/album/dto/album-add-users.dto.ts b/server/libs/domain/src/album/dto/album-add-users.dto.ts new file mode 100644 index 0000000000..32b8770aaa --- /dev/null +++ b/server/libs/domain/src/album/dto/album-add-users.dto.ts @@ -0,0 +1,8 @@ +import { ArrayNotEmpty } from 'class-validator'; +import { ValidateUUID } from '../../../../../apps/immich/src/decorators/validate-uuid.decorator'; + +export class AddUsersDto { + @ValidateUUID({ each: true }) + @ArrayNotEmpty() + sharedUserIds!: string[]; +} diff --git a/server/libs/domain/src/album/dto/index.ts b/server/libs/domain/src/album/dto/index.ts index 5481afb175..4234895f69 100644 --- a/server/libs/domain/src/album/dto/index.ts +++ b/server/libs/domain/src/album/dto/index.ts @@ -1,3 +1,4 @@ +export * from './album-add-users.dto'; export * from './album-create.dto'; export * from './album-update.dto'; export * from './get-albums.dto'; diff --git a/server/libs/domain/test/fixtures.ts b/server/libs/domain/test/fixtures.ts index baca3cd6d4..9bf05c0246 100644 --- a/server/libs/domain/test/fixtures.ts +++ b/server/libs/domain/test/fixtures.ts @@ -61,6 +61,16 @@ export const authStub = { isShowExif: true, accessTokenId: 'token-id', }), + user2: Object.freeze({ + id: 'user-2', + email: 'user2@immich.app', + isAdmin: false, + isPublicUser: false, + isAllowUpload: true, + isAllowDownload: true, + isShowExif: true, + accessTokenId: 'token-id', + }), adminSharedLink: Object.freeze({ id: 'admin_id', email: 'admin@test.com', @@ -125,6 +135,21 @@ export const userEntityStub = { tags: [], assets: [], }), + user2: Object.freeze({ + ...authStub.user2, + password: 'immich_password', + firstName: 'immich_first_name', + lastName: 'immich_last_name', + storageLabel: null, + oauthId: '', + shouldChangePassword: false, + profileImagePath: '', + createdAt: new Date('2021-01-01'), + deletedAt: null, + updatedAt: new Date('2021-01-01'), + tags: [], + assets: [], + }), storageLabel: Object.freeze({ ...authStub.user1, password: 'immich_password', @@ -357,6 +382,19 @@ export const albumStub = { sharedLinks: [], sharedUsers: [userEntityStub.user1], }), + sharedWithMultiple: Object.freeze({ + id: 'album-3', + albumName: 'Empty album shared with users', + ownerId: authStub.admin.id, + owner: userEntityStub.admin, + assets: [], + albumThumbnailAsset: null, + albumThumbnailAssetId: null, + createdAt: new Date(), + updatedAt: new Date(), + sharedLinks: [], + sharedUsers: [userEntityStub.user1, userEntityStub.user2], + }), sharedWithAdmin: Object.freeze({ id: 'album-3', albumName: 'Empty album shared with admin', diff --git a/server/libs/infra/src/repositories/album.repository.ts b/server/libs/infra/src/repositories/album.repository.ts index 6d0ba76478..3dd091667f 100644 --- a/server/libs/infra/src/repositories/album.repository.ts +++ b/server/libs/infra/src/repositories/album.repository.ts @@ -16,6 +16,7 @@ export class AlbumRepository implements IAlbumRepository { }, relations: { owner: true, + sharedUsers: true, }, }); } @@ -153,6 +154,12 @@ export class AlbumRepository implements IAlbumRepository { private async save(album: Partial) { const { id } = await this.repository.save(album); - return this.repository.findOneOrFail({ where: { id }, relations: { owner: true } }); + return this.repository.findOneOrFail({ + where: { id }, + relations: { + owner: true, + sharedUsers: true, + }, + }); } } diff --git a/web/src/lib/components/album-page/album-viewer.svelte b/web/src/lib/components/album-page/album-viewer.svelte index d0a14c5552..1eae441ca8 100644 --- a/web/src/lib/components/album-page/album-viewer.svelte +++ b/web/src/lib/components/album-page/album-viewer.svelte @@ -42,6 +42,7 @@ import ShareInfoModal from './share-info-modal.svelte'; import ThumbnailSelection from './thumbnail-selection.svelte'; import UserSelectionModal from './user-selection-modal.svelte'; + import { handleError } from '../../utils/handle-error'; export let album: AlbumResponseDto; export let sharedLink: SharedLinkResponseDto | undefined = undefined; @@ -195,19 +196,16 @@ if (userId == 'me') { isShowShareInfoModal = false; goto(backUrl); + return; } try { const { data } = await api.albumApi.getAlbumInfo({ id: album.id }); album = data; - isShowShareInfoModal = false; + isShowShareInfoModal = data.sharedUsers.length >= 1; } catch (e) { - console.error('Error [sharedUserDeletedHandler] ', e); - notificationController.show({ - type: NotificationType.Error, - message: 'Error deleting share users, check console for more details' - }); + handleError(e, 'Error deleting share users'); } }; diff --git a/web/src/lib/components/album-page/share-info-modal.svelte b/web/src/lib/components/album-page/share-info-modal.svelte index 352347635c..593e93b345 100644 --- a/web/src/lib/components/album-page/share-info-modal.svelte +++ b/web/src/lib/components/album-page/share-info-modal.svelte @@ -1,7 +1,6 @@ - dispatch('close')}> - - -

Options

- - +{#if !selectedRemoveUser} + dispatch('close')}> + + +

Options

+ + -
- {#each album.sharedUsers as user} -
-
- -

{user.firstName} {user.lastName}

-
+
+ {#each album.sharedUsers as user} +
+
+ +

{user.firstName} {user.lastName}

+
-
- {#if isOwned} -
(isShowMenu = false)}> - showContextMenu(user.id)} - logo={DotsVertical} - backgroundColor={'transparent'} - hoverColor={'#e2e7e9'} - size={'20'} - > - {#if isShowMenu} - - removeUser(targetUserId)} text="Remove" /> +
+ {#if isOwned} +
+ showContextMenu(user)} + logo={DotsVertical} + backgroundColor="transparent" + hoverColor="#e2e7e9" + size="20" + /> + + {#if selectedMenuUser === user} + (selectedMenuUser = null)}> + {/if} - -
- {:else if user.id == currentUser?.id} - - {/if} +
+ {:else if user.id == currentUser?.id} + + {/if} +
-
- {/each} -
- + {/each} +
+ +{/if} + +{#if selectedRemoveUser && selectedRemoveUser?.id === currentUser?.id} + (selectedRemoveUser = null)} + /> +{/if} + +{#if selectedRemoveUser && selectedRemoveUser?.id !== currentUser?.id} + (selectedRemoveUser = null)} + /> +{/if}