From 50a792a81a41235dcad8f01c949cc5fe672fc3fb Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Tue, 23 May 2023 16:40:04 -0400 Subject: [PATCH] refactor(server): use cascades for keys and tokens (#2544) --- .../api-key-response.dto.ts | 5 + .../api-key-create.dto.ts => api-key.dto.ts} | 6 + .../domain/src/api-key/api-key.repository.ts | 1 - .../domain/src/api-key/api-key.service.ts | 5 +- .../src/api-key/dto/api-key-update.dto.ts | 7 - server/libs/domain/src/api-key/dto/index.ts | 2 - server/libs/domain/src/api-key/index.ts | 4 +- .../api-key-create-response.dto.ts | 6 - .../domain/src/api-key/response-dto/index.ts | 2 - .../src/user-token/user-token.repository.ts | 1 - .../libs/domain/src/user/user.service.spec.ts | 147 ++++++++---------- server/libs/domain/src/user/user.service.ts | 6 - .../domain/test/api-key.repository.mock.ts | 1 - .../domain/test/user-token.repository.mock.ts | 1 - .../libs/infra/src/entities/api-key.entity.ts | 2 +- .../infra/src/entities/user-token.entity.ts | 2 +- ...867360825-AddUserTokenAndAPIKeyCascades.ts | 20 +++ .../src/repositories/api-key.repository.ts | 4 - .../src/repositories/user-token.repository.ts | 4 - 19 files changed, 100 insertions(+), 126 deletions(-) rename server/libs/domain/src/api-key/{response-dto => }/api-key-response.dto.ts (79%) rename server/libs/domain/src/api-key/{dto/api-key-create.dto.ts => api-key.dto.ts} (67%) delete mode 100644 server/libs/domain/src/api-key/dto/api-key-update.dto.ts delete mode 100644 server/libs/domain/src/api-key/dto/index.ts delete mode 100644 server/libs/domain/src/api-key/response-dto/api-key-create-response.dto.ts delete mode 100644 server/libs/domain/src/api-key/response-dto/index.ts create mode 100644 server/libs/infra/src/migrations/1684867360825-AddUserTokenAndAPIKeyCascades.ts diff --git a/server/libs/domain/src/api-key/response-dto/api-key-response.dto.ts b/server/libs/domain/src/api-key/api-key-response.dto.ts similarity index 79% rename from server/libs/domain/src/api-key/response-dto/api-key-response.dto.ts rename to server/libs/domain/src/api-key/api-key-response.dto.ts index aee5fcd10b..f7b5b5e9f9 100644 --- a/server/libs/domain/src/api-key/response-dto/api-key-response.dto.ts +++ b/server/libs/domain/src/api-key/api-key-response.dto.ts @@ -1,5 +1,10 @@ import { APIKeyEntity } from '@app/infra/entities'; +export class APIKeyCreateResponseDto { + secret!: string; + apiKey!: APIKeyResponseDto; +} + export class APIKeyResponseDto { id!: string; name!: string; diff --git a/server/libs/domain/src/api-key/dto/api-key-create.dto.ts b/server/libs/domain/src/api-key/api-key.dto.ts similarity index 67% rename from server/libs/domain/src/api-key/dto/api-key-create.dto.ts rename to server/libs/domain/src/api-key/api-key.dto.ts index 30f5327363..5482ba31bb 100644 --- a/server/libs/domain/src/api-key/dto/api-key-create.dto.ts +++ b/server/libs/domain/src/api-key/api-key.dto.ts @@ -6,3 +6,9 @@ export class APIKeyCreateDto { @IsOptional() name?: string; } + +export class APIKeyUpdateDto { + @IsString() + @IsNotEmpty() + name!: string; +} diff --git a/server/libs/domain/src/api-key/api-key.repository.ts b/server/libs/domain/src/api-key/api-key.repository.ts index 68b9c7d1e7..60f26f2358 100644 --- a/server/libs/domain/src/api-key/api-key.repository.ts +++ b/server/libs/domain/src/api-key/api-key.repository.ts @@ -6,7 +6,6 @@ export interface IKeyRepository { create(dto: Partial): Promise; update(userId: string, id: string, dto: Partial): Promise; delete(userId: string, id: string): Promise; - deleteAll(userId: string): Promise; /** * Includes the hashed `key` for verification * @param id diff --git a/server/libs/domain/src/api-key/api-key.service.ts b/server/libs/domain/src/api-key/api-key.service.ts index a503f92809..bba778a78d 100644 --- a/server/libs/domain/src/api-key/api-key.service.ts +++ b/server/libs/domain/src/api-key/api-key.service.ts @@ -1,10 +1,9 @@ import { BadRequestException, Inject, Injectable } from '@nestjs/common'; import { AuthUserDto } from '../auth'; import { ICryptoRepository } from '../crypto'; +import { APIKeyCreateResponseDto, APIKeyResponseDto, mapKey } from './api-key-response.dto'; +import { APIKeyCreateDto } from './api-key.dto'; import { IKeyRepository } from './api-key.repository'; -import { APIKeyCreateDto } from './dto/api-key-create.dto'; -import { APIKeyCreateResponseDto } from './response-dto/api-key-create-response.dto'; -import { APIKeyResponseDto, mapKey } from './response-dto/api-key-response.dto'; @Injectable() export class APIKeyService { diff --git a/server/libs/domain/src/api-key/dto/api-key-update.dto.ts b/server/libs/domain/src/api-key/dto/api-key-update.dto.ts deleted file mode 100644 index 9db4f1ee5c..0000000000 --- a/server/libs/domain/src/api-key/dto/api-key-update.dto.ts +++ /dev/null @@ -1,7 +0,0 @@ -import { IsNotEmpty, IsString } from 'class-validator'; - -export class APIKeyUpdateDto { - @IsString() - @IsNotEmpty() - name!: string; -} diff --git a/server/libs/domain/src/api-key/dto/index.ts b/server/libs/domain/src/api-key/dto/index.ts deleted file mode 100644 index c0deb8fbe6..0000000000 --- a/server/libs/domain/src/api-key/dto/index.ts +++ /dev/null @@ -1,2 +0,0 @@ -export * from './api-key-create.dto'; -export * from './api-key-update.dto'; diff --git a/server/libs/domain/src/api-key/index.ts b/server/libs/domain/src/api-key/index.ts index 87f3632f0e..181110c5d9 100644 --- a/server/libs/domain/src/api-key/index.ts +++ b/server/libs/domain/src/api-key/index.ts @@ -1,4 +1,4 @@ +export * from './api-key-response.dto'; +export * from './api-key.dto'; export * from './api-key.repository'; export * from './api-key.service'; -export * from './dto'; -export * from './response-dto'; diff --git a/server/libs/domain/src/api-key/response-dto/api-key-create-response.dto.ts b/server/libs/domain/src/api-key/response-dto/api-key-create-response.dto.ts deleted file mode 100644 index f9f2825cce..0000000000 --- a/server/libs/domain/src/api-key/response-dto/api-key-create-response.dto.ts +++ /dev/null @@ -1,6 +0,0 @@ -import { APIKeyResponseDto } from './api-key-response.dto'; - -export class APIKeyCreateResponseDto { - secret!: string; - apiKey!: APIKeyResponseDto; -} diff --git a/server/libs/domain/src/api-key/response-dto/index.ts b/server/libs/domain/src/api-key/response-dto/index.ts deleted file mode 100644 index 82cf6a4d9a..0000000000 --- a/server/libs/domain/src/api-key/response-dto/index.ts +++ /dev/null @@ -1,2 +0,0 @@ -export * from './api-key-create-response.dto'; -export * from './api-key-response.dto'; diff --git a/server/libs/domain/src/user-token/user-token.repository.ts b/server/libs/domain/src/user-token/user-token.repository.ts index 0b2f86400c..51234eddcf 100644 --- a/server/libs/domain/src/user-token/user-token.repository.ts +++ b/server/libs/domain/src/user-token/user-token.repository.ts @@ -6,7 +6,6 @@ export interface IUserTokenRepository { create(dto: Partial): Promise; save(dto: Partial): Promise; delete(userId: string, id: string): Promise; - deleteAll(userId: string): Promise; getByToken(token: string): Promise; getAll(userId: string): Promise; } diff --git a/server/libs/domain/src/user/user.service.spec.ts b/server/libs/domain/src/user/user.service.spec.ts index 808c6ebe50..a1429b1d2f 100644 --- a/server/libs/domain/src/user/user.service.spec.ts +++ b/server/libs/domain/src/user/user.service.spec.ts @@ -6,19 +6,15 @@ import { newAssetRepositoryMock, newCryptoRepositoryMock, newJobRepositoryMock, - newKeyRepositoryMock, newStorageRepositoryMock, newUserRepositoryMock, - newUserTokenRepositoryMock, } from '../../test'; import { IAlbumRepository } from '../album'; -import { IKeyRepository } from '../api-key'; import { IAssetRepository } from '../asset'; import { AuthUserDto } from '../auth'; import { ICryptoRepository } from '../crypto'; import { IJobRepository, JobName } from '../job'; import { IStorageRepository } from '../storage'; -import { IUserTokenRepository } from '../user-token'; import { UpdateUserDto } from './dto/update-user.dto'; import { IUserRepository } from './user.repository'; import { UserService } from './user.service'; @@ -109,52 +105,37 @@ const adminUserResponse = Object.freeze({ describe(UserService.name, () => { let sut: UserService; - let userRepositoryMock: jest.Mocked; + let userMock: jest.Mocked; let cryptoRepositoryMock: jest.Mocked; let albumMock: jest.Mocked; let assetMock: jest.Mocked; let jobMock: jest.Mocked; - let keyMock: jest.Mocked; let storageMock: jest.Mocked; - let tokenMock: jest.Mocked; beforeEach(async () => { - userRepositoryMock = newUserRepositoryMock(); cryptoRepositoryMock = newCryptoRepositoryMock(); - albumMock = newAlbumRepositoryMock(); assetMock = newAssetRepositoryMock(); jobMock = newJobRepositoryMock(); - keyMock = newKeyRepositoryMock(); storageMock = newStorageRepositoryMock(); - tokenMock = newUserTokenRepositoryMock(); - userRepositoryMock = newUserRepositoryMock(); + userMock = newUserRepositoryMock(); - sut = new UserService( - userRepositoryMock, - cryptoRepositoryMock, - albumMock, - assetMock, - jobMock, - keyMock, - storageMock, - tokenMock, - ); + sut = new UserService(userMock, cryptoRepositoryMock, albumMock, assetMock, jobMock, storageMock); - when(userRepositoryMock.get).calledWith(adminUser.id).mockResolvedValue(adminUser); - when(userRepositoryMock.get).calledWith(adminUser.id, undefined).mockResolvedValue(adminUser); - when(userRepositoryMock.get).calledWith(immichUser.id).mockResolvedValue(immichUser); - when(userRepositoryMock.get).calledWith(immichUser.id, undefined).mockResolvedValue(immichUser); + when(userMock.get).calledWith(adminUser.id).mockResolvedValue(adminUser); + when(userMock.get).calledWith(adminUser.id, undefined).mockResolvedValue(adminUser); + when(userMock.get).calledWith(immichUser.id).mockResolvedValue(immichUser); + when(userMock.get).calledWith(immichUser.id, undefined).mockResolvedValue(immichUser); }); describe('getAllUsers', () => { it('should get all users', async () => { - userRepositoryMock.getList.mockResolvedValue([adminUser]); + userMock.getList.mockResolvedValue([adminUser]); const response = await sut.getAllUsers(adminUserAuth, false); - expect(userRepositoryMock.getList).toHaveBeenCalledWith({ withDeleted: true }); + expect(userMock.getList).toHaveBeenCalledWith({ withDeleted: true }); expect(response).toEqual([ { id: adminUserAuth.id, @@ -176,49 +157,49 @@ describe(UserService.name, () => { describe('getUserById', () => { it('should get a user by id', async () => { - userRepositoryMock.get.mockResolvedValue(adminUser); + userMock.get.mockResolvedValue(adminUser); const response = await sut.getUserById(adminUser.id); - expect(userRepositoryMock.get).toHaveBeenCalledWith(adminUser.id, false); + expect(userMock.get).toHaveBeenCalledWith(adminUser.id, false); expect(response).toEqual(adminUserResponse); }); it('should throw an error if a user is not found', async () => { - userRepositoryMock.get.mockResolvedValue(null); + userMock.get.mockResolvedValue(null); await expect(sut.getUserById(adminUser.id)).rejects.toBeInstanceOf(NotFoundException); - expect(userRepositoryMock.get).toHaveBeenCalledWith(adminUser.id, false); + expect(userMock.get).toHaveBeenCalledWith(adminUser.id, false); }); }); describe('getUserInfo', () => { it("should get the auth user's info", async () => { - userRepositoryMock.get.mockResolvedValue(adminUser); + userMock.get.mockResolvedValue(adminUser); const response = await sut.getUserInfo(adminUser); - expect(userRepositoryMock.get).toHaveBeenCalledWith(adminUser.id, undefined); + expect(userMock.get).toHaveBeenCalledWith(adminUser.id, undefined); expect(response).toEqual(adminUserResponse); }); it('should throw an error if a user is not found', async () => { - userRepositoryMock.get.mockResolvedValue(null); + userMock.get.mockResolvedValue(null); await expect(sut.getUserInfo(adminUser)).rejects.toBeInstanceOf(BadRequestException); - expect(userRepositoryMock.get).toHaveBeenCalledWith(adminUser.id, undefined); + expect(userMock.get).toHaveBeenCalledWith(adminUser.id, undefined); }); }); describe('getUserCount', () => { it('should get the user count', async () => { - userRepositoryMock.getList.mockResolvedValue([adminUser]); + userMock.getList.mockResolvedValue([adminUser]); const response = await sut.getUserCount({}); - expect(userRepositoryMock.getList).toHaveBeenCalled(); + expect(userMock.getList).toHaveBeenCalled(); expect(response).toEqual({ userCount: 1 }); }); }); @@ -230,30 +211,30 @@ describe(UserService.name, () => { shouldChangePassword: true, }; - when(userRepositoryMock.update).calledWith(update.id, update).mockResolvedValueOnce(updatedImmichUser); + when(userMock.update).calledWith(update.id, update).mockResolvedValueOnce(updatedImmichUser); const updatedUser = await sut.updateUser(immichUserAuth, update); expect(updatedUser.shouldChangePassword).toEqual(true); }); it('should not set an empty string for storage label', async () => { - userRepositoryMock.update.mockResolvedValue(updatedImmichUser); + userMock.update.mockResolvedValue(updatedImmichUser); await sut.updateUser(adminUserAuth, { id: immichUser.id, storageLabel: '' }); - expect(userRepositoryMock.update).toHaveBeenCalledWith(immichUser.id, { id: immichUser.id, storageLabel: null }); + expect(userMock.update).toHaveBeenCalledWith(immichUser.id, { id: immichUser.id, storageLabel: null }); }); it('should omit a storage label set by non-admin users', async () => { - userRepositoryMock.update.mockResolvedValue(updatedImmichUser); + userMock.update.mockResolvedValue(updatedImmichUser); await sut.updateUser(immichUserAuth, { id: immichUser.id, storageLabel: 'admin' }); - expect(userRepositoryMock.update).toHaveBeenCalledWith(immichUser.id, { id: immichUser.id }); + expect(userMock.update).toHaveBeenCalledWith(immichUser.id, { id: immichUser.id }); }); it('user can only update its information', async () => { - when(userRepositoryMock.get) + when(userMock.get) .calledWith('not_immich_auth_user_id', undefined) .mockResolvedValueOnce({ ...immichUser, @@ -270,12 +251,12 @@ describe(UserService.name, () => { it('should let a user change their email', async () => { const dto = { id: immichUser.id, email: 'updated@test.com' }; - userRepositoryMock.get.mockResolvedValue(immichUser); - userRepositoryMock.update.mockResolvedValue(immichUser); + userMock.get.mockResolvedValue(immichUser); + userMock.update.mockResolvedValue(immichUser); await sut.updateUser(immichUser, dto); - expect(userRepositoryMock.update).toHaveBeenCalledWith(immichUser.id, { + expect(userMock.update).toHaveBeenCalledWith(immichUser.id, { id: 'user-id', email: 'updated@test.com', }); @@ -284,23 +265,23 @@ describe(UserService.name, () => { it('should not let a user change their email to one already in use', async () => { const dto = { id: immichUser.id, email: 'updated@test.com' }; - userRepositoryMock.get.mockResolvedValue(immichUser); - userRepositoryMock.getByEmail.mockResolvedValue(adminUser); + userMock.get.mockResolvedValue(immichUser); + userMock.getByEmail.mockResolvedValue(adminUser); await expect(sut.updateUser(immichUser, dto)).rejects.toBeInstanceOf(BadRequestException); - expect(userRepositoryMock.update).not.toHaveBeenCalled(); + expect(userMock.update).not.toHaveBeenCalled(); }); it('should not let the admin change the storage label to one already in use', async () => { const dto = { id: immichUser.id, storageLabel: 'admin' }; - userRepositoryMock.get.mockResolvedValue(immichUser); - userRepositoryMock.getByStorageLabel.mockResolvedValue(adminUser); + userMock.get.mockResolvedValue(immichUser); + userMock.getByStorageLabel.mockResolvedValue(adminUser); await expect(sut.updateUser(adminUser, dto)).rejects.toBeInstanceOf(BadRequestException); - expect(userRepositoryMock.update).not.toHaveBeenCalled(); + expect(userMock.update).not.toHaveBeenCalled(); }); it('admin can update any user information', async () => { @@ -309,7 +290,7 @@ describe(UserService.name, () => { shouldChangePassword: true, }; - when(userRepositoryMock.update).calledWith(immichUser.id, update).mockResolvedValueOnce(updatedImmichUser); + when(userMock.update).calledWith(immichUser.id, update).mockResolvedValueOnce(updatedImmichUser); const result = await sut.updateUser(adminUserAuth, update); @@ -319,7 +300,7 @@ describe(UserService.name, () => { }); it('update user information should throw error if user not found', async () => { - when(userRepositoryMock.get).calledWith(immichUser.id, undefined).mockResolvedValueOnce(null); + when(userMock.get).calledWith(immichUser.id, undefined).mockResolvedValueOnce(null); const result = sut.updateUser(adminUser, { id: immichUser.id, @@ -332,18 +313,18 @@ describe(UserService.name, () => { it('should let the admin update himself', async () => { const dto = { id: adminUser.id, shouldChangePassword: true, isAdmin: true }; - when(userRepositoryMock.get).calledWith(adminUser.id).mockResolvedValueOnce(null); - when(userRepositoryMock.update).calledWith(adminUser.id, dto).mockResolvedValueOnce(adminUser); + when(userMock.get).calledWith(adminUser.id).mockResolvedValueOnce(null); + when(userMock.update).calledWith(adminUser.id, dto).mockResolvedValueOnce(adminUser); await sut.updateUser(adminUser, dto); - expect(userRepositoryMock.update).toHaveBeenCalledWith(adminUser.id, dto); + expect(userMock.update).toHaveBeenCalledWith(adminUser.id, dto); }); it('should not let the another user become an admin', async () => { const dto = { id: immichUser.id, shouldChangePassword: true, isAdmin: true }; - when(userRepositoryMock.get).calledWith(immichUser.id).mockResolvedValueOnce(immichUser); + when(userMock.get).calledWith(immichUser.id).mockResolvedValueOnce(immichUser); await expect(sut.updateUser(adminUser, dto)).rejects.toBeInstanceOf(BadRequestException); }); @@ -351,15 +332,15 @@ describe(UserService.name, () => { describe('restoreUser', () => { it('should require an admin', async () => { - when(userRepositoryMock.get).calledWith(adminUser.id, true).mockResolvedValue(adminUser); + when(userMock.get).calledWith(adminUser.id, true).mockResolvedValue(adminUser); await expect(sut.restoreUser(immichUserAuth, adminUser.id)).rejects.toBeInstanceOf(ForbiddenException); - expect(userRepositoryMock.get).toHaveBeenCalledWith(adminUser.id, true); + expect(userMock.get).toHaveBeenCalledWith(adminUser.id, true); }); it('should require the auth user be an admin', async () => { await expect(sut.deleteUser(immichUserAuth, adminUserAuth.id)).rejects.toBeInstanceOf(ForbiddenException); - expect(userRepositoryMock.delete).not.toHaveBeenCalled(); + expect(userMock.delete).not.toHaveBeenCalled(); }); }); @@ -371,13 +352,13 @@ describe(UserService.name, () => { it('should require the auth user be an admin', async () => { await expect(sut.deleteUser(immichUserAuth, adminUserAuth.id)).rejects.toBeInstanceOf(ForbiddenException); - expect(userRepositoryMock.delete).not.toHaveBeenCalled(); + expect(userMock.delete).not.toHaveBeenCalled(); }); }); describe('update', () => { it('should not create a user if there is no local admin account', async () => { - when(userRepositoryMock.getAdmin).calledWith().mockResolvedValueOnce(null); + when(userMock.getAdmin).calledWith().mockResolvedValueOnce(null); await expect( sut.createUser({ @@ -393,35 +374,35 @@ describe(UserService.name, () => { describe('createProfileImage', () => { it('should throw an error if the user does not exist', async () => { const file = { path: '/profile/path' } as Express.Multer.File; - userRepositoryMock.update.mockResolvedValue({ ...adminUser, profileImagePath: file.path }); + userMock.update.mockResolvedValue({ ...adminUser, profileImagePath: file.path }); await sut.createProfileImage(adminUserAuth, file); - expect(userRepositoryMock.update).toHaveBeenCalledWith(adminUserAuth.id, { profileImagePath: file.path }); + expect(userMock.update).toHaveBeenCalledWith(adminUserAuth.id, { profileImagePath: file.path }); }); }); describe('getUserProfileImage', () => { it('should throw an error if the user does not exist', async () => { - userRepositoryMock.get.mockResolvedValue(null); + userMock.get.mockResolvedValue(null); await expect(sut.getUserProfileImage(adminUserAuth.id)).rejects.toBeInstanceOf(NotFoundException); - expect(userRepositoryMock.get).toHaveBeenCalledWith(adminUserAuth.id, undefined); + expect(userMock.get).toHaveBeenCalledWith(adminUserAuth.id, undefined); }); it('should throw an error if the user does not have a picture', async () => { - userRepositoryMock.get.mockResolvedValue(adminUser); + userMock.get.mockResolvedValue(adminUser); await expect(sut.getUserProfileImage(adminUserAuth.id)).rejects.toBeInstanceOf(NotFoundException); - expect(userRepositoryMock.get).toHaveBeenCalledWith(adminUserAuth.id, undefined); + expect(userMock.get).toHaveBeenCalledWith(adminUserAuth.id, undefined); }); }); describe('resetAdminPassword', () => { it('should only work when there is an admin account', async () => { - userRepositoryMock.getAdmin.mockResolvedValue(null); + userMock.getAdmin.mockResolvedValue(null); const ask = jest.fn().mockResolvedValue('new-password'); await expect(sut.resetAdminPassword(ask)).rejects.toBeInstanceOf(BadRequestException); @@ -430,12 +411,12 @@ describe(UserService.name, () => { }); it('should default to a random password', async () => { - userRepositoryMock.getAdmin.mockResolvedValue(adminUser); + userMock.getAdmin.mockResolvedValue(adminUser); const ask = jest.fn().mockResolvedValue(undefined); const response = await sut.resetAdminPassword(ask); - const [id, update] = userRepositoryMock.update.mock.calls[0]; + const [id, update] = userMock.update.mock.calls[0]; expect(response.provided).toBe(false); expect(ask).toHaveBeenCalled(); @@ -444,12 +425,12 @@ describe(UserService.name, () => { }); it('should use the supplied password', async () => { - userRepositoryMock.getAdmin.mockResolvedValue(adminUser); + userMock.getAdmin.mockResolvedValue(adminUser); const ask = jest.fn().mockResolvedValue('new-password'); const response = await sut.resetAdminPassword(ask); - const [id, update] = userRepositoryMock.update.mock.calls[0]; + const [id, update] = userMock.update.mock.calls[0]; expect(response.provided).toBe(true); expect(ask).toHaveBeenCalled(); @@ -460,7 +441,7 @@ describe(UserService.name, () => { describe('handleQueueUserDelete', () => { it('should skip users not ready for deletion', async () => { - userRepositoryMock.getDeletedUsers.mockResolvedValue([ + userMock.getDeletedUsers.mockResolvedValue([ {}, { deletedAt: undefined }, { deletedAt: null }, @@ -469,17 +450,17 @@ describe(UserService.name, () => { await sut.handleUserDeleteCheck(); - expect(userRepositoryMock.getDeletedUsers).toHaveBeenCalled(); + expect(userMock.getDeletedUsers).toHaveBeenCalled(); expect(jobMock.queue).not.toHaveBeenCalled(); }); it('should queue user ready for deletion', async () => { const user = { deletedAt: makeDeletedAt(10) }; - userRepositoryMock.getDeletedUsers.mockResolvedValue([user] as UserEntity[]); + userMock.getDeletedUsers.mockResolvedValue([user] as UserEntity[]); await sut.handleUserDeleteCheck(); - expect(userRepositoryMock.getDeletedUsers).toHaveBeenCalled(); + expect(userMock.getDeletedUsers).toHaveBeenCalled(); expect(jobMock.queue).toHaveBeenCalledWith({ name: JobName.USER_DELETION, data: { user } }); }); }); @@ -491,7 +472,7 @@ describe(UserService.name, () => { await sut.handleUserDelete({ user }); expect(storageMock.unlinkDir).not.toHaveBeenCalled(); - expect(userRepositoryMock.delete).not.toHaveBeenCalled(); + expect(userMock.delete).not.toHaveBeenCalled(); }); it('should delete the user and associated assets', async () => { @@ -506,11 +487,9 @@ describe(UserService.name, () => { expect(storageMock.unlinkDir).toHaveBeenCalledWith('upload/profile/deleted-user', options); expect(storageMock.unlinkDir).toHaveBeenCalledWith('upload/thumbs/deleted-user', options); expect(storageMock.unlinkDir).toHaveBeenCalledWith('upload/encoded-video/deleted-user', options); - expect(tokenMock.deleteAll).toHaveBeenCalledWith(user.id); - expect(keyMock.deleteAll).toHaveBeenCalledWith(user.id); expect(albumMock.deleteAll).toHaveBeenCalledWith(user.id); expect(assetMock.deleteAll).toHaveBeenCalledWith(user.id); - expect(userRepositoryMock.delete).toHaveBeenCalledWith(user, true); + expect(userMock.delete).toHaveBeenCalledWith(user, true); }); it('should delete the library path for a storage label', async () => { @@ -530,7 +509,7 @@ describe(UserService.name, () => { await sut.handleUserDelete({ user }); - expect(userRepositoryMock.delete).not.toHaveBeenCalled(); + expect(userMock.delete).not.toHaveBeenCalled(); }); }); }); diff --git a/server/libs/domain/src/user/user.service.ts b/server/libs/domain/src/user/user.service.ts index ee61b2e582..74374085a4 100644 --- a/server/libs/domain/src/user/user.service.ts +++ b/server/libs/domain/src/user/user.service.ts @@ -3,14 +3,12 @@ import { BadRequestException, Inject, Injectable, Logger, NotFoundException } fr import { randomBytes } from 'crypto'; import { ReadStream } from 'fs'; import { IAlbumRepository } from '../album/album.repository'; -import { IKeyRepository } from '../api-key/api-key.repository'; import { IAssetRepository } from '../asset/asset.repository'; import { AuthUserDto } from '../auth'; import { ICryptoRepository } from '../crypto/crypto.repository'; import { IJobRepository, IUserDeletionJob, JobName } from '../job'; import { StorageCore, StorageFolder } from '../storage'; import { IStorageRepository } from '../storage/storage.repository'; -import { IUserTokenRepository } from '../user-token/user-token.repository'; import { IUserRepository } from '../user/user.repository'; import { CreateUserDto, UpdateUserDto, UserCountDto } from './dto'; import { @@ -36,9 +34,7 @@ export class UserService { @Inject(IAlbumRepository) private albumRepository: IAlbumRepository, @Inject(IAssetRepository) private assetRepository: IAssetRepository, @Inject(IJobRepository) private jobRepository: IJobRepository, - @Inject(IKeyRepository) private keyRepository: IKeyRepository, @Inject(IStorageRepository) private storageRepository: IStorageRepository, - @Inject(IUserTokenRepository) private tokenRepository: IUserTokenRepository, ) { this.userCore = new UserCore(userRepository, cryptoRepository); } @@ -174,8 +170,6 @@ export class UserService { this.logger.warn(`Removing user from database: ${user.id}`); - await this.tokenRepository.deleteAll(user.id); - await this.keyRepository.deleteAll(user.id); await this.albumRepository.deleteAll(user.id); await this.assetRepository.deleteAll(user.id); await this.userRepository.delete(user, true); diff --git a/server/libs/domain/test/api-key.repository.mock.ts b/server/libs/domain/test/api-key.repository.mock.ts index 8bf5b690fd..dbf8405fc1 100644 --- a/server/libs/domain/test/api-key.repository.mock.ts +++ b/server/libs/domain/test/api-key.repository.mock.ts @@ -5,7 +5,6 @@ export const newKeyRepositoryMock = (): jest.Mocked => { create: jest.fn(), update: jest.fn(), delete: jest.fn(), - deleteAll: jest.fn(), getKey: jest.fn(), getById: jest.fn(), getByUserId: jest.fn(), diff --git a/server/libs/domain/test/user-token.repository.mock.ts b/server/libs/domain/test/user-token.repository.mock.ts index b3b9bdb685..bebebb09f9 100644 --- a/server/libs/domain/test/user-token.repository.mock.ts +++ b/server/libs/domain/test/user-token.repository.mock.ts @@ -5,7 +5,6 @@ export const newUserTokenRepositoryMock = (): jest.Mocked create: jest.fn(), save: jest.fn(), delete: jest.fn(), - deleteAll: jest.fn(), getByToken: jest.fn(), getAll: jest.fn(), }; diff --git a/server/libs/infra/src/entities/api-key.entity.ts b/server/libs/infra/src/entities/api-key.entity.ts index 77c8a570c0..54108832d8 100644 --- a/server/libs/infra/src/entities/api-key.entity.ts +++ b/server/libs/infra/src/entities/api-key.entity.ts @@ -12,7 +12,7 @@ export class APIKeyEntity { @Column({ select: false }) key?: string; - @ManyToOne(() => UserEntity) + @ManyToOne(() => UserEntity, { onUpdate: 'CASCADE', onDelete: 'CASCADE' }) user?: UserEntity; @Column() diff --git a/server/libs/infra/src/entities/user-token.entity.ts b/server/libs/infra/src/entities/user-token.entity.ts index c5abad55ac..a39e93a33e 100644 --- a/server/libs/infra/src/entities/user-token.entity.ts +++ b/server/libs/infra/src/entities/user-token.entity.ts @@ -12,7 +12,7 @@ export class UserTokenEntity { @Column() userId!: string; - @ManyToOne(() => UserEntity) + @ManyToOne(() => UserEntity, { onUpdate: 'CASCADE', onDelete: 'CASCADE' }) user!: UserEntity; @CreateDateColumn({ type: 'timestamptz' }) diff --git a/server/libs/infra/src/migrations/1684867360825-AddUserTokenAndAPIKeyCascades.ts b/server/libs/infra/src/migrations/1684867360825-AddUserTokenAndAPIKeyCascades.ts new file mode 100644 index 0000000000..273c6b830f --- /dev/null +++ b/server/libs/infra/src/migrations/1684867360825-AddUserTokenAndAPIKeyCascades.ts @@ -0,0 +1,20 @@ +import { MigrationInterface, QueryRunner } from "typeorm"; + +export class AddUserTokenAndAPIKeyCascades1684867360825 implements MigrationInterface { + name = 'AddUserTokenAndAPIKeyCascades1684867360825' + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(`ALTER TABLE "api_keys" DROP CONSTRAINT "FK_6c2e267ae764a9413b863a29342"`); + await queryRunner.query(`ALTER TABLE "user_token" DROP CONSTRAINT "FK_d37db50eecdf9b8ce4eedd2f918"`); + await queryRunner.query(`ALTER TABLE "api_keys" ADD CONSTRAINT "FK_6c2e267ae764a9413b863a29342" FOREIGN KEY ("userId") REFERENCES "users"("id") ON DELETE CASCADE ON UPDATE CASCADE`); + await queryRunner.query(`ALTER TABLE "user_token" ADD CONSTRAINT "FK_d37db50eecdf9b8ce4eedd2f918" FOREIGN KEY ("userId") REFERENCES "users"("id") ON DELETE CASCADE ON UPDATE CASCADE`); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(`ALTER TABLE "user_token" DROP CONSTRAINT "FK_d37db50eecdf9b8ce4eedd2f918"`); + await queryRunner.query(`ALTER TABLE "api_keys" DROP CONSTRAINT "FK_6c2e267ae764a9413b863a29342"`); + await queryRunner.query(`ALTER TABLE "user_token" ADD CONSTRAINT "FK_d37db50eecdf9b8ce4eedd2f918" FOREIGN KEY ("userId") REFERENCES "users"("id") ON DELETE NO ACTION ON UPDATE NO ACTION`); + await queryRunner.query(`ALTER TABLE "api_keys" ADD CONSTRAINT "FK_6c2e267ae764a9413b863a29342" FOREIGN KEY ("userId") REFERENCES "users"("id") ON DELETE NO ACTION ON UPDATE NO ACTION`); + } + +} diff --git a/server/libs/infra/src/repositories/api-key.repository.ts b/server/libs/infra/src/repositories/api-key.repository.ts index 095c7ae632..2484b0d561 100644 --- a/server/libs/infra/src/repositories/api-key.repository.ts +++ b/server/libs/infra/src/repositories/api-key.repository.ts @@ -21,10 +21,6 @@ export class APIKeyRepository implements IKeyRepository { await this.repository.delete({ userId, id }); } - async deleteAll(userId: string): Promise { - await this.repository.delete({ userId }); - } - getKey(hashedToken: string): Promise { return this.repository.findOne({ select: { diff --git a/server/libs/infra/src/repositories/user-token.repository.ts b/server/libs/infra/src/repositories/user-token.repository.ts index e76c5df13b..9469aa1bb1 100644 --- a/server/libs/infra/src/repositories/user-token.repository.ts +++ b/server/libs/infra/src/repositories/user-token.repository.ts @@ -38,8 +38,4 @@ export class UserTokenRepository implements IUserTokenRepository { async delete(userId: string, id: string): Promise { await this.repository.delete({ userId, id }); } - - async deleteAll(userId: string): Promise { - await this.repository.delete({ userId }); - } }