Add possibility to soft delete users (#2497)

Previously we hard deleted the users, but due to change requests and
possibly other features in future, we really want to hard-link user
table and have meaningful relationships.

But this means, when user is deleted, all linked data is also deleted.
**Workaround is to soft delete users and just clear users data and keep
the relationships alive for audit logs.**

This PR implements this feature.

src/lib/db/access-store.ts

@@ -28,6 +28,8 @@ const T = {
     PERMISSIONS: 'permissions',
     PERMISSION_TYPES: 'permission_types',
     CHANGE_REQUEST_SETTINGS: 'change_request_settings',
+    PERSONAL_ACCESS_TOKENS: 'personal_access_tokens',
+    PUBLIC_SIGNUP_TOKENS_USER: 'public_signup_tokens_user',
 };
 
 interface IPermissionRow {
@@ -215,6 +217,30 @@ export class AccessStore implements IAccessStore {
             .delete();
     }
 
+    async unlinkUserGroups(userId: number): Promise<void> {
+        return this.db(T.GROUP_USER)
+            .where({
+                user_id: userId,
+            })
+            .delete();
+    }
+
+    async clearUserPersonalAccessTokens(userId: number): Promise<void> {
+        return this.db(T.PERSONAL_ACCESS_TOKENS)
+            .where({
+                user_id: userId,
+            })
+            .delete();
+    }
+
+    async clearPublicSignupUserTokens(userId: number): Promise<void> {
+        return this.db(T.PUBLIC_SIGNUP_TOKENS_USER)
+            .where({
+                user_id: userId,
+            })
+            .delete();
+    }
+
     async getProjectUsersForRole(
         roleId: number,
         projectId?: string,

src/lib/db/user-store.ts

@@ -77,7 +77,9 @@ class UserStore implements IUserStore {
     }
 
     async update(id: number, fields: IUserUpdateFields): Promise<User> {
-        await this.db(TABLE).where('id', id).update(mapUserToColumns(fields));
+        await this.activeUsers()
+            .where('id', id)
+            .update(mapUserToColumns(fields));
         return this.get(id);
     }
 
@@ -98,7 +100,7 @@ class UserStore implements IUserStore {
     }
 
     buildSelectUser(q: IUserLookup): any {
-        const query = this.db(TABLE);
+        const query = this.activeUsers();
         if (q.id) {
             return query.where('id', q.id);
         }
@@ -111,6 +113,10 @@ class UserStore implements IUserStore {
         throw new Error('Can only find users with id, username or email.');
     }
 
+    activeUsers(): any {
+        return this.db(TABLE).where('deleted_at', null);
+    }
+
     async hasUser(idQuery: IUserLookup): Promise<number | undefined> {
         const query = this.buildSelectUser(idQuery);
         const item = await query.first('id');
@@ -118,14 +124,13 @@ class UserStore implements IUserStore {
     }
 
     async getAll(): Promise<User[]> {
-        const users = await this.db.select(USER_COLUMNS).from(TABLE);
+        const users = await this.activeUsers().select(USER_COLUMNS);
         return users.map(rowToUser);
     }
 
     async search(query: string): Promise<User[]> {
-        const users = await this.db
+        const users = await this.activeUsers()
             .select(USER_COLUMNS_PUBLIC)
-            .from(TABLE)
             .where('name', 'ILIKE', `%${query}%`)
             .orWhere('username', 'ILIKE', `${query}%`)
             .orWhere('email', 'ILIKE', `${query}%`);
@@ -133,9 +138,8 @@ class UserStore implements IUserStore {
     }
 
     async getAllWithId(userIdList: number[]): Promise<User[]> {
-        const users = await this.db
+        const users = await this.activeUsers()
             .select(USER_COLUMNS_PUBLIC)
-            .from(TABLE)
             .whereIn('id', userIdList);
         return users.map(rowToUser);
     }
@@ -146,11 +150,18 @@ class UserStore implements IUserStore {
     }
 
     async delete(id: number): Promise<void> {
-        return this.db(TABLE).where({ id }).del();
+        return this.activeUsers()
+            .where({ id })
+            .update({
+                deleted_at: new Date(),
+                email: null,
+                username: null,
+                name: this.db.raw('name || ?', '(Deleted)'),
+            });
     }
 
     async getPasswordHash(userId: number): Promise<string> {
-        const item = await this.db(TABLE)
+        const item = await this.activeUsers()
             .where('id', userId)
             .first('password_hash');
 
@@ -162,7 +173,7 @@ class UserStore implements IUserStore {
     }
 
     async setPasswordHash(userId: number, passwordHash: string): Promise<void> {
-        return this.db(TABLE).where('id', userId).update({
+        return this.activeUsers().where('id', userId).update({
             password_hash: passwordHash,
         });
     }
@@ -179,12 +190,11 @@ class UserStore implements IUserStore {
     }
 
     async deleteAll(): Promise<void> {
-        await this.db(TABLE).del();
+        await this.activeUsers().del();
     }
 
     async count(): Promise<number> {
-        return this.db
+        return this.activeUsers()
-            .from(TABLE)
             .count('*')
             .then((res) => Number(res[0].count));
     }
@@ -193,7 +203,7 @@ class UserStore implements IUserStore {
 
     async exists(id: number): Promise<boolean> {
         const result = await this.db.raw(
-            `SELECT EXISTS (SELECT 1 FROM ${TABLE} WHERE id = ?) AS present`,
+            `SELECT EXISTS (SELECT 1 FROM ${TABLE} WHERE id = ? and deleted_at = null) AS present`,
             [id],
         );
         const { present } = result.rows[0];
@@ -201,14 +211,13 @@ class UserStore implements IUserStore {
     }
 
     async get(id: number): Promise<User> {
-        const row = await this.db(TABLE).where({ id }).first();
+        const row = await this.activeUsers().where({ id }).first();
         return rowToUser(row);
     }
 
     async getUserByPersonalAccessToken(secret: string): Promise<User> {
-        const row = await this.db
+        const row = await this.activeUsers()
             .select(USER_COLUMNS.map((column) => `${TABLE}.${column}`))
-            .from(TABLE)
             .leftJoin(
                 'personal_access_tokens',
                 'personal_access_tokens.user_id',

src/lib/services/access-service.ts

@@ -351,8 +351,13 @@ export class AccessService {
         return this.store.getRolesForUserId(userId);
     }
 
-    async unlinkUserRoles(userId: number): Promise<void> {
+    async wipeUserPermissions(userId: number): Promise<void[]> {
-        return this.store.unlinkUserRoles(userId);
+        return Promise.all([
+            this.store.unlinkUserRoles(userId),
+            this.store.unlinkUserGroups(userId),
+            this.store.clearUserPersonalAccessTokens(userId),
+            this.store.clearPublicSignupUserTokens(userId),
+        ]);
     }
 
     async getUsersForRole(roleId: number): Promise<IUser[]> {

src/lib/services/user-service.ts

@@ -264,7 +264,7 @@ class UserService {
 
     async deleteUser(userId: number, updatedBy?: User): Promise<void> {
         const user = await this.store.get(userId);
-        await this.accessService.unlinkUserRoles(userId);
+        await this.accessService.wipeUserPermissions(userId);
         await this.sessionService.deleteSessionsForUser(userId);
 
         await this.store.delete(userId);

src/lib/types/stores/access-store.ts

@@ -53,6 +53,12 @@ export interface IAccessStore extends Store<IRole, number> {
 
     unlinkUserRoles(userId: number): Promise<void>;
 
+    unlinkUserGroups(userId: number): Promise<void>;
+
+    clearUserPersonalAccessTokens(userId: number): Promise<void>;
+
+    clearPublicSignupUserTokens(userId: number): Promise<void>;
+
     getRolesForUserId(userId: number): Promise<IRoleWithProject[]>;
 
     getProjectUsersForRole(

src/migrations/20221121133546-soft-delete-user.js

@@ -0,0 +1,19 @@
+'use strict';
+
+exports.up = function (db, callback) {
+    db.runSql(
+        `
+            ALTER TABLE users ADD COLUMN IF NOT EXISTS deleted_at TIMESTAMP WITH TIME ZONE;
+        `,
+        callback,
+    );
+};
+
+exports.down = function (db, callback) {
+    db.runSql(
+        `
+            ALTER TABLE users DROP COLUMN IF EXISTS deleted_at;
+        `,
+        callback,
+    );
+};

src/test/e2e/api/admin/user-admin.e2e.test.ts

@@ -215,7 +215,8 @@ test('get a single user', async () => {
 test('should delete user', async () => {
     const user = await userStore.insert({ email: 'some@mail.com' });
 
-    return app.request.delete(`/api/admin/user-admin/${user.id}`).expect(200);
+    await app.request.delete(`/api/admin/user-admin/${user.id}`).expect(200);
+    await app.request.get(`/api/admin/user-admin/${user.id}`).expect(404);
 });
 
 test('validator should require strong password', async () => {
@@ -341,7 +342,7 @@ test('generates USER_CREATED event', async () => {
 
 test('generates USER_DELETED event', async () => {
     const user = await userStore.insert({ email: 'some@mail.com' });
-    await app.request.delete(`/api/admin/user-admin/${user.id}`);
+    await app.request.delete(`/api/admin/user-admin/${user.id}`).expect(200);
 
     const events = await eventStore.getEvents();
     expect(events[0].type).toBe(USER_DELETED);

src/test/e2e/services/user-service.e2e.test.ts

@@ -15,6 +15,7 @@ import { simpleAuthSettingsKey } from '../../../lib/types/settings/simple-auth-s
 import { addDays, minutesToMilliseconds } from 'date-fns';
 import { GroupService } from '../../../lib/services/group-service';
 import { randomId } from '../../../lib/util/random-id';
+import { BadDataError } from '../../../lib/error';
 
 let db;
 let stores;
@@ -102,6 +103,20 @@ test('should create user with password', async () => {
     expect(user.username).toBe('test');
 });
 
+test('should not be able to login with deleted user', async () => {
+    const user = await userService.createUser({
+        username: 'deleted_user',
+        password: 'unleash4all',
+        rootRole: adminRole.id,
+    });
+
+    await userService.deleteUser(user.id);
+
+    await expect(
+        userService.loginUser('deleted_user', 'unleash4all'),
+    ).rejects.toThrow(new NotFoundError(`No user found`));
+});
+
 test('should not login user if simple auth is disabled', async () => {
     await settingService.insert(
         simpleAuthSettingsKey,
@@ -225,16 +240,14 @@ test('should login and create user via SSO', async () => {
 test('should throw if rootRole is wrong via SSO', async () => {
     expect.assertions(1);
 
-    try {
+    await expect(
-        await userService.loginUserSSO({
+        userService.loginUserSSO({
             email: 'some@test.com',
             rootRole: RoleName.MEMBER,
             name: 'some',
             autoCreate: true,
-        });
+        }),
-    } catch (e) {
+    ).rejects.toThrow(new BadDataError(`Could not find rootRole=Member`));
-        expect(e.message).toBe('Could not find rootRole=Member');
-    }
 });
 
 test('should update user name when signing in via SSO', async () => {
@@ -284,14 +297,12 @@ test('should update name if it is different via SSO', async () => {
 test('should throw if autoCreate is false via SSO', async () => {
     expect.assertions(1);
 
-    try {
+    await expect(
-        await userService.loginUserSSO({
+        userService.loginUserSSO({
             email: 'some@test.com',
             rootRole: RoleName.MEMBER,
             name: 'some',
             autoCreate: false,
-        });
+        }),
-    } catch (e) {
+    ).rejects.toThrow(new NotFoundError(`No user found`));
-        expect(e.message).toBe('No user found');
-    }
 });

src/test/e2e/stores/user-store.e2e.test.ts

@@ -160,3 +160,15 @@ test('should always lowercase emails on updates', async () => {
     storedUser = await store.get(storedUser.id);
     expect(storedUser.email).toBe(updatedUser.email.toLowerCase());
 });
+
+test('should delete user', async () => {
+    const user = await stores.userStore.upsert({
+        email: 'deleteuser@mail.com',
+    });
+
+    await stores.userStore.delete(user.id);
+
+    await expect(() => stores.userStore.get(user.id)).rejects.toThrow(
+        new NotFoundError('No user found'),
+    );
+});

src/test/fixtures/fake-access-store.ts

@@ -199,6 +199,18 @@ class AccessStoreMock implements IAccessStore {
     ): Promise<void> {
         return Promise.resolve(undefined);
     }
+
+    clearUserPersonalAccessTokens(userId: number): Promise<void> {
+        return Promise.resolve(undefined);
+    }
+
+    unlinkUserGroups(userId: number): Promise<void> {
+        return Promise.resolve(undefined);
+    }
+
+    clearPublicSignupUserTokens(userId: number): Promise<void> {
+        return Promise.resolve(undefined);
+    }
 }
 
 module.exports = AccessStoreMock;