fix(server): Error when loading album with deleted owner (#4086)

* soft delete albums when user gets soft deleted

* fix wrong intl openapi version

* fix tests

* ability to restore albums, automatically restore when user restored

* (e2e) tests for shared albums via link and with user

* (e2e) test deletion of users and linked albums

* (e2e) fix share album with owner test

* fix: deletedAt

* chore: fix restore order

* fix: use timezone date column

* chore: cleanup e2e tests

* (e2e) fix user delete test

---------

Co-authored-by: Jason Rasmussen <jrasm91@gmail.com>

server/src/domain/album/album.repository.ts

@@ -23,6 +23,8 @@ export interface IAlbumRepository {
   getOwned(ownerId: string): Promise<AlbumEntity[]>;
   getShared(ownerId: string): Promise<AlbumEntity[]>;
   getNotShared(ownerId: string): Promise<AlbumEntity[]>;
+  restoreAll(userId: string): Promise<void>;
+  softDeleteAll(userId: string): Promise<void>;
   deleteAll(userId: string): Promise<void>;
   getAll(): Promise<AlbumEntity[]>;
   create(album: Partial<AlbumEntity>): Promise<AlbumEntity>;

server/src/domain/user/user.service.ts

@@ -91,6 +91,7 @@ export class UserService {
     if (!user) {
       throw new BadRequestException('User not found');
     }
+    await this.albumRepository.softDeleteAll(userId);
     const deletedUser = await this.userCore.deleteUser(authUser, user);
     return mapUser(deletedUser);
   }
@@ -101,6 +102,7 @@ export class UserService {
       throw new BadRequestException('User not found');
     }
     const updatedUser = await this.userCore.restoreUser(authUser, user);
+    await this.albumRepository.restoreAll(userId);
     return mapUser(updatedUser);
   }
 

server/src/infra/entities/album.entity.ts

@@ -1,6 +1,7 @@
 import {
   Column,
   CreateDateColumn,
+  DeleteDateColumn,
   Entity,
   JoinTable,
   ManyToMany,
@@ -36,6 +37,9 @@ export class AlbumEntity {
   @UpdateDateColumn({ type: 'timestamptz' })
   updatedAt!: Date;
 
+  @DeleteDateColumn({ type: 'timestamptz' })
+  deletedAt!: Date | null;
+
   @ManyToOne(() => AssetEntity, { nullable: true, onDelete: 'SET NULL', onUpdate: 'CASCADE' })
   albumThumbnailAsset!: AssetEntity | null;
 

server/src/infra/migrations/1694638413248-AddDeletedAtToAlbums.ts

@@ -0,0 +1,13 @@
+import { MigrationInterface, QueryRunner } from 'typeorm';
+
+export class AddDeletedAtToAlbums1694638413248 implements MigrationInterface {
+  name = 'AddDeletedAtToAlbums1694638413248';
+
+  public async up(queryRunner: QueryRunner): Promise<void> {
+    await queryRunner.query(`ALTER TABLE "albums" ADD "deletedAt" TIMESTAMP WITH TIME ZONE`);
+  }
+
+  public async down(queryRunner: QueryRunner): Promise<void> {
+    await queryRunner.query(`ALTER TABLE "albums" DROP COLUMN "deletedAt"`);
+  }
+}

server/src/infra/repositories/album.repository.ts

@@ -142,6 +142,14 @@ export class AlbumRepository implements IAlbumRepository {
     });
   }
 
+  async restoreAll(userId: string): Promise<void> {
+    await this.repository.restore({ ownerId: userId });
+  }
+
+  async softDeleteAll(userId: string): Promise<void> {
+    await this.repository.softDelete({ ownerId: userId });
+  }
+
   async deleteAll(userId: string): Promise<void> {
     await this.repository.delete({ ownerId: userId });
   }

server/test/e2e/album.e2e-spec.ts

@@ -19,6 +19,7 @@ const user2NotShared = 'user2NotShared';
 describe(`${AlbumController.name} (e2e)`, () => {
   let app: INestApplication;
   let server: any;
+  let admin: LoginResponseDto;
   let user1: LoginResponseDto;
   let user1Asset: AssetFileUploadResponseDto;
   let user1Albums: AlbumResponseDto[];
@@ -37,7 +38,7 @@ describe(`${AlbumController.name} (e2e)`, () => {
   beforeEach(async () => {
     await db.reset();
     await api.authApi.adminSignUp(server);
-    const admin = await api.authApi.adminLogin(server);
+    admin = await api.authApi.adminLogin(server);
 
     await api.userApi.create(server, admin.accessToken, {
       email: 'user1@immich.app',
@@ -105,7 +106,7 @@ describe(`${AlbumController.name} (e2e)`, () => {
       const { status, body } = await request(server)
         .get('/album?shared=invalid')
         .set('Authorization', `Bearer ${user1.accessToken}`);
-      expect(status).toEqual(400);
+      expect(status).toBe(400);
       expect(body).toEqual(errorStub.badRequest);
     });
 
@@ -113,13 +114,28 @@ describe(`${AlbumController.name} (e2e)`, () => {
       const { status, body } = await request(server)
         .get('/album?assetId=invalid')
         .set('Authorization', `Bearer ${user1.accessToken}`);
-      expect(status).toEqual(400);
+      expect(status).toBe(400);
       expect(body).toEqual(errorStub.badRequest);
     });
 
+    it('should not return shared albums with a deleted owner', async () => {
+      await api.userApi.delete(server, admin.accessToken, user1.userId);
+      const { status, body } = await request(server)
+        .get('/album?shared=true')
+        .set('Authorization', `Bearer ${user2.accessToken}`);
+
+      expect(status).toBe(200);
+      expect(body).toHaveLength(1);
+      expect(body).toEqual(
+        expect.arrayContaining([
+          expect.objectContaining({ ownerId: user2.userId, albumName: user2SharedLink, shared: true }),
+        ]),
+      );
+    });
+
     it('should return the album collection including owned and shared', async () => {
       const { status, body } = await request(server).get('/album').set('Authorization', `Bearer ${user1.accessToken}`);
-      expect(status).toEqual(200);
+      expect(status).toBe(200);
       expect(body).toHaveLength(3);
       expect(body).toEqual(
         expect.arrayContaining([
@@ -134,7 +150,7 @@ describe(`${AlbumController.name} (e2e)`, () => {
       const { status, body } = await request(server)
         .get('/album?shared=true')
         .set('Authorization', `Bearer ${user1.accessToken}`);
-      expect(status).toEqual(200);
+      expect(status).toBe(200);
       expect(body).toHaveLength(3);
       expect(body).toEqual(
         expect.arrayContaining([
@@ -149,7 +165,7 @@ describe(`${AlbumController.name} (e2e)`, () => {
       const { status, body } = await request(server)
         .get('/album?shared=false')
         .set('Authorization', `Bearer ${user1.accessToken}`);
-      expect(status).toEqual(200);
+      expect(status).toBe(200);
       expect(body).toHaveLength(1);
       expect(body).toEqual(
         expect.arrayContaining([
@@ -164,7 +180,7 @@ describe(`${AlbumController.name} (e2e)`, () => {
       const { status, body } = await request(server)
         .get(`/album?assetId=${asset.id}`)
         .set('Authorization', `Bearer ${user1.accessToken}`);
-      expect(status).toEqual(200);
+      expect(status).toBe(200);
       expect(body).toHaveLength(1);
     });
 
@@ -172,7 +188,7 @@ describe(`${AlbumController.name} (e2e)`, () => {
       const { status, body } = await request(server)
         .get(`/album?shared=true&assetId=${user1Asset.id}`)
         .set('Authorization', `Bearer ${user1.accessToken}`);
-      expect(status).toEqual(200);
+      expect(status).toBe(200);
       expect(body).toHaveLength(4);
     });
 
@@ -180,7 +196,7 @@ describe(`${AlbumController.name} (e2e)`, () => {
       const { status, body } = await request(server)
         .get(`/album?shared=false&assetId=${user1Asset.id}`)
         .set('Authorization', `Bearer ${user1.accessToken}`);
-      expect(status).toEqual(200);
+      expect(status).toBe(200);
       expect(body).toHaveLength(4);
     });
   });
@@ -390,15 +406,15 @@ describe(`${AlbumController.name} (e2e)`, () => {
       expect(body).toEqual(expect.objectContaining({ sharedUsers: [expect.objectContaining({ id: user2.userId })] }));
     });
 
-    // it('should not be able to share album with owner', async () => {
-    //   const { status, body } = await request(server)
-    //     .put(`/album/${album.id}/users`)
-    //     .set('Authorization', `Bearer ${user1.accessToken}`)
-    //     .send({ sharedUserIds: [user2.userId] });
+    it('should not be able to share album with owner', async () => {
+      const { status, body } = await request(server)
+        .put(`/album/${album.id}/users`)
+        .set('Authorization', `Bearer ${user1.accessToken}`)
+        .send({ sharedUserIds: [user1.userId] });
 
-    //   expect(status).toBe(400);
-    //   expect(body).toEqual(errorStub.badRequest);
-    // });
+      expect(status).toBe(400);
+      expect(body).toEqual({ ...errorStub.badRequest, message: 'Cannot be shared with owner' });
+    });
 
     it('should not be able to add existing user to shared album', async () => {
       await request(server)

server/test/e2e/shared-link.e2e-spec.ts

@@ -99,7 +99,21 @@ describe(`${PartnerController.name} (e2e)`, () => {
         .query({ key: sharedLink.key + 'foo' });
 
       expect(status).toBe(401);
-      expect(body).toEqual(expect.objectContaining({ message: 'Invalid share key' }));
+      expect(body).toEqual(errorStub.invalidShareKey);
+    });
+
+    it('should return unauthorized if target has been soft deleted', async () => {
+      const softDeletedAlbum = await api.albumApi.create(server, user1.accessToken, { albumName: 'shared with link' });
+      const softDeletedAlbumLink = await api.sharedLinkApi.create(server, user1.accessToken, {
+        type: SharedLinkType.ALBUM,
+        albumId: softDeletedAlbum.id,
+      });
+      await api.userApi.delete(server, accessToken, user1.userId);
+
+      const { status, body } = await request(server).get('/shared-link/me').query({ key: softDeletedAlbumLink.key });
+
+      expect(status).toBe(401);
+      expect(body).toEqual(errorStub.invalidShareKey);
     });
   });
 

server/test/e2e/user.e2e-spec.ts

@@ -1,17 +1,21 @@
-import { LoginResponseDto } from '@app/domain';
+import { LoginResponseDto, UserResponseDto, UserService } from '@app/domain';
 import { AppModule, UserController } from '@app/immich';
+import { UserEntity } from '@app/infra/entities';
 import { INestApplication } from '@nestjs/common';
 import { Test, TestingModule } from '@nestjs/testing';
 import { api } from '@test/api';
 import { db } from '@test/db';
 import { errorStub, userSignupStub, userStub } from '@test/fixtures';
 import request from 'supertest';
+import { Repository } from 'typeorm';
 
 describe(`${UserController.name}`, () => {
   let app: INestApplication;
   let server: any;
   let loginResponse: LoginResponseDto;
   let accessToken: string;
+  let userService: UserService;
+  let userRepository: Repository<UserEntity>;
 
   beforeAll(async () => {
     const moduleFixture: TestingModule = await Test.createTestingModule({
@@ -19,6 +23,7 @@ describe(`${UserController.name}`, () => {
     }).compile();
 
     app = await moduleFixture.createNestApplication().init();
+    userRepository = moduleFixture.get('UserEntityRepository');
     server = app.getHttpServer();
   });
 
@@ -27,6 +32,8 @@ describe(`${UserController.name}`, () => {
     await api.authApi.adminSignUp(server);
     loginResponse = await api.authApi.adminLogin(server);
     accessToken = loginResponse.accessToken;
+
+    userService = app.get<UserService>(UserService);
   });
 
   afterAll(async () => {
@@ -173,6 +180,50 @@ describe(`${UserController.name}`, () => {
     });
   });
 
+  describe('DELETE /user/:id', () => {
+    let userToDelete: UserResponseDto;
+
+    beforeEach(async () => {
+      userToDelete = await api.userApi.create(server, accessToken, {
+        email: userStub.user1.email,
+        firstName: userStub.user1.firstName,
+        lastName: userStub.user1.lastName,
+        password: 'superSecurePassword',
+      });
+    });
+
+    it('should require authentication', async () => {
+      const { status, body } = await request(server).delete(`/user/${userToDelete.id}`);
+      expect(status).toBe(401);
+      expect(body).toEqual(errorStub.unauthorized);
+    });
+
+    it('should delete user', async () => {
+      const deleteRequest = await request(server)
+        .delete(`/user/${userToDelete.id}`)
+        .set('Authorization', `Bearer ${accessToken}`);
+
+      expect(deleteRequest.status).toBe(200);
+      expect(deleteRequest.body).toEqual({
+        ...userToDelete,
+        updatedAt: expect.any(String),
+        deletedAt: expect.any(String),
+      });
+
+      await userRepository.save({ id: deleteRequest.body.id, deletedAt: new Date('1970-01-01').toISOString() });
+
+      await userService.handleUserDelete({ id: userToDelete.id });
+
+      const { status, body } = await request(server)
+        .get('/user')
+        .query({ isAll: false })
+        .set('Authorization', `Bearer ${accessToken}`);
+
+      expect(status).toBe(200);
+      expect(body).toHaveLength(1);
+    });
+  });
+
   describe('PUT /user', () => {
     it('should require authentication', async () => {
       const { status, body } = await request(server).put(`/user`);
@@ -237,9 +288,9 @@ describe(`${UserController.name}`, () => {
         lastName: 'Last Name',
       });
 
-      expect(after).toMatchObject({
+      expect(after).toEqual({
         ...before,
-        updatedAt: expect.anything(),
+        updatedAt: expect.any(String),
         firstName: 'First Name',
         lastName: 'Last Name',
       });

server/test/fixtures/album.stub.ts

@@ -15,6 +15,7 @@ export const albumStub = {
     albumThumbnailAssetId: null,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [],
   }),
@@ -29,6 +30,7 @@ export const albumStub = {
     albumThumbnailAssetId: null,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [userStub.user1],
   }),
@@ -43,6 +45,7 @@ export const albumStub = {
     albumThumbnailAssetId: null,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [userStub.user1, userStub.user2],
   }),
@@ -57,6 +60,7 @@ export const albumStub = {
     albumThumbnailAssetId: null,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [userStub.admin],
   }),
@@ -71,6 +75,7 @@ export const albumStub = {
     albumThumbnailAssetId: null,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [],
   }),
@@ -85,6 +90,7 @@ export const albumStub = {
     albumThumbnailAssetId: assetStub.image.id,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [],
   }),
@@ -99,6 +105,7 @@ export const albumStub = {
     albumThumbnailAssetId: assetStub.image.id,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [],
   }),
@@ -113,6 +120,7 @@ export const albumStub = {
     albumThumbnailAssetId: null,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [],
   }),
@@ -127,6 +135,7 @@ export const albumStub = {
     albumThumbnailAssetId: assetStub.livePhotoMotionAsset.id,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [],
   }),
@@ -141,6 +150,7 @@ export const albumStub = {
     albumThumbnailAssetId: assetStub.image.id,
     createdAt: new Date(),
     updatedAt: new Date(),
+    deletedAt: null,
     sharedLinks: [],
     sharedUsers: [],
   }),

server/test/fixtures/error.stub.ts

@@ -19,6 +19,11 @@ export const errorStub = {
     statusCode: 401,
     message: 'Invalid user token',
   },
+  invalidShareKey: {
+    error: 'Unauthorized',
+    statusCode: 401,
+    message: 'Invalid share key',
+  },
   badRequest: {
     error: 'Bad Request',
     statusCode: 400,

server/test/fixtures/shared-link.stub.ts

@@ -151,6 +151,7 @@ export const sharedLinkStub = {
       description: '',
       createdAt: today,
       updatedAt: today,
+      deletedAt: null,
       albumThumbnailAsset: null,
       albumThumbnailAssetId: null,
       sharedUsers: [],

server/test/repositories/album.repository.mock.ts

@@ -10,6 +10,8 @@ export const newAlbumRepositoryMock = (): jest.Mocked<IAlbumRepository> => {
     getOwned: jest.fn(),
     getShared: jest.fn(),
     getNotShared: jest.fn(),
+    restoreAll: jest.fn(),
+    softDeleteAll: jest.fn(),
     deleteAll: jest.fn(),
     getAll: jest.fn(),
     removeAsset: jest.fn(),