diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index 5c9c35f659..873293f9c5 100644 --- a/src/lib/db/access-store.ts +++ b/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 { + return this.db(T.GROUP_USER) + .where({ + user_id: userId, + }) + .delete(); + } + + async clearUserPersonalAccessTokens(userId: number): Promise { + return this.db(T.PERSONAL_ACCESS_TOKENS) + .where({ + user_id: userId, + }) + .delete(); + } + + async clearPublicSignupUserTokens(userId: number): Promise { + return this.db(T.PUBLIC_SIGNUP_TOKENS_USER) + .where({ + user_id: userId, + }) + .delete(); + } + async getProjectUsersForRole( roleId: number, projectId?: string, diff --git a/src/lib/db/user-store.ts b/src/lib/db/user-store.ts index 517e0db1fb..ae3182f417 100644 --- a/src/lib/db/user-store.ts +++ b/src/lib/db/user-store.ts @@ -77,7 +77,9 @@ class UserStore implements IUserStore { } async update(id: number, fields: IUserUpdateFields): Promise { - 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 { const query = this.buildSelectUser(idQuery); const item = await query.first('id'); @@ -118,14 +124,13 @@ class UserStore implements IUserStore { } async getAll(): Promise { - 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 { - 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 { - 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 { - 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 { - 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 { - 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 { - await this.db(TABLE).del(); + await this.activeUsers().del(); } async count(): Promise { - return this.db - .from(TABLE) + return this.activeUsers() .count('*') .then((res) => Number(res[0].count)); } @@ -193,7 +203,7 @@ class UserStore implements IUserStore { async exists(id: number): Promise { 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 { - 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 { - 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', diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index 594f8505d1..4fac9f5370 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -351,8 +351,13 @@ export class AccessService { return this.store.getRolesForUserId(userId); } - async unlinkUserRoles(userId: number): Promise { - return this.store.unlinkUserRoles(userId); + async wipeUserPermissions(userId: number): Promise { + return Promise.all([ + this.store.unlinkUserRoles(userId), + this.store.unlinkUserGroups(userId), + this.store.clearUserPersonalAccessTokens(userId), + this.store.clearPublicSignupUserTokens(userId), + ]); } async getUsersForRole(roleId: number): Promise { diff --git a/src/lib/services/user-service.ts b/src/lib/services/user-service.ts index 937e37fdaa..a10cd0b892 100644 --- a/src/lib/services/user-service.ts +++ b/src/lib/services/user-service.ts @@ -264,7 +264,7 @@ class UserService { async deleteUser(userId: number, updatedBy?: User): Promise { 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); diff --git a/src/lib/types/stores/access-store.ts b/src/lib/types/stores/access-store.ts index a4a50596cc..42aa71a961 100644 --- a/src/lib/types/stores/access-store.ts +++ b/src/lib/types/stores/access-store.ts @@ -53,6 +53,12 @@ export interface IAccessStore extends Store { unlinkUserRoles(userId: number): Promise; + unlinkUserGroups(userId: number): Promise; + + clearUserPersonalAccessTokens(userId: number): Promise; + + clearPublicSignupUserTokens(userId: number): Promise; + getRolesForUserId(userId: number): Promise; getProjectUsersForRole( diff --git a/src/migrations/20221121133546-soft-delete-user.js b/src/migrations/20221121133546-soft-delete-user.js new file mode 100644 index 0000000000..f721a56c08 --- /dev/null +++ b/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, + ); +}; diff --git a/src/test/e2e/api/admin/user-admin.e2e.test.ts b/src/test/e2e/api/admin/user-admin.e2e.test.ts index 224555f552..6cefc11236 100644 --- a/src/test/e2e/api/admin/user-admin.e2e.test.ts +++ b/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); diff --git a/src/test/e2e/services/user-service.e2e.test.ts b/src/test/e2e/services/user-service.e2e.test.ts index 62ae96b4e9..81a9e35805 100644 --- a/src/test/e2e/services/user-service.e2e.test.ts +++ b/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 userService.loginUserSSO({ + await expect( + userService.loginUserSSO({ email: 'some@test.com', rootRole: RoleName.MEMBER, name: 'some', autoCreate: true, - }); - } catch (e) { - expect(e.message).toBe('Could not find rootRole=Member'); - } + }), + ).rejects.toThrow(new BadDataError(`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 userService.loginUserSSO({ + await expect( + userService.loginUserSSO({ email: 'some@test.com', rootRole: RoleName.MEMBER, name: 'some', autoCreate: false, - }); - } catch (e) { - expect(e.message).toBe('No user found'); - } + }), + ).rejects.toThrow(new NotFoundError(`No user found`)); }); diff --git a/src/test/e2e/stores/user-store.e2e.test.ts b/src/test/e2e/stores/user-store.e2e.test.ts index 15aa0e65b8..5984d5d3e9 100644 --- a/src/test/e2e/stores/user-store.e2e.test.ts +++ b/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'), + ); +}); diff --git a/src/test/fixtures/fake-access-store.ts b/src/test/fixtures/fake-access-store.ts index 0fbd45848a..191051493a 100644 --- a/src/test/fixtures/fake-access-store.ts +++ b/src/test/fixtures/fake-access-store.ts @@ -199,6 +199,18 @@ class AccessStoreMock implements IAccessStore { ): Promise { return Promise.resolve(undefined); } + + clearUserPersonalAccessTokens(userId: number): Promise { + return Promise.resolve(undefined); + } + + unlinkUserGroups(userId: number): Promise { + return Promise.resolve(undefined); + } + + clearPublicSignupUserTokens(userId: number): Promise { + return Promise.resolve(undefined); + } } module.exports = AccessStoreMock;