From 21883b4e6bfa6761c38f85194471cd20bda62183 Mon Sep 17 00:00:00 2001
From: Laurent Cozic <laurent@cozic.net>
Date: Fri, 4 Nov 2022 16:18:41 +0000
Subject: [PATCH] Server: Fail-safe when trying to delete a non-disabled
 account

---
 packages/server/src/routes/admin/users.ts     |  1 +
 .../src/services/UserDeletionService.test.ts  | 33 ++++++++++++++++++-
 .../src/services/UserDeletionService.ts       | 14 ++++++++
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/packages/server/src/routes/admin/users.ts b/packages/server/src/routes/admin/users.ts
index 900c21c58..7e199c32e 100644
--- a/packages/server/src/routes/admin/users.ts
+++ b/packages/server/src/routes/admin/users.ts
@@ -339,6 +339,7 @@ router.post('admin/users', async (path: SubPath, ctx: AppContext) => {
 			await stopImpersonating(ctx);
 			return redirect(ctx, config().baseUrl);
 		} else if (fields.disable_button || fields.restore_button) {
+			const user = await models.user().load(userId);
 			await models.user().checkIfAllowed(owner, AclAction.Delete, user);
 			await models.user().setEnabled(path.id, !!fields.restore_button);
 		} else if (fields.send_account_confirmation_email) {
diff --git a/packages/server/src/services/UserDeletionService.test.ts b/packages/server/src/services/UserDeletionService.test.ts
index dcd29ffaa..656bc2f99 100644
--- a/packages/server/src/services/UserDeletionService.test.ts
+++ b/packages/server/src/services/UserDeletionService.test.ts
@@ -32,6 +32,8 @@ describe('UserDeletionService', function() {
 		const t0 = new Date('2021-12-14').getTime();
 		const t1 = t0 + 1000;
 
+		await models().user().setEnabled(user1.id, false);
+
 		const job = await models().userDeletion().add(user1.id, t1, {
 			processData: true,
 			processAccount: false,
@@ -63,6 +65,8 @@ describe('UserDeletionService', function() {
 		const t0 = new Date('2021-12-14').getTime();
 		const t1 = t0 + 1000;
 
+		await models().user().setEnabled(user1.id, false);
+
 		const job = await models().userDeletion().add(user1.id, t1, {
 			processData: false,
 			processAccount: true,
@@ -92,7 +96,7 @@ describe('UserDeletionService', function() {
 		const content = JSON.parse(backupItem.content.toString());
 		expect(content.user.id).toBe(user1.id);
 		expect(content.user.email).toBe(user1.email);
-		expect(content.flags.length).toBe(0);
+		expect(content.flags.length).toBe(1);
 	});
 
 	test('should not delete notebooks that are not owned', async function() {
@@ -113,6 +117,8 @@ describe('UserDeletionService', function() {
 		expect(await models().share().count()).toBe(1);
 		expect(await models().shareUser().count()).toBe(1);
 
+		await models().user().setEnabled(user2.id, false);
+
 		const job = await models().userDeletion().add(user2.id, Date.now());
 		const service = newService();
 		await service.processDeletionJob(job, { sleepBetweenOperations: 0 });
@@ -140,6 +146,8 @@ describe('UserDeletionService', function() {
 		expect(await models().share().count()).toBe(1);
 		expect(await models().shareUser().count()).toBe(1);
 
+		await models().user().setEnabled(user1.id, false);
+
 		const job = await models().userDeletion().add(user1.id, Date.now());
 		const service = newService();
 		await service.processDeletionJob(job, { sleepBetweenOperations: 0 });
@@ -149,4 +157,27 @@ describe('UserDeletionService', function() {
 		expect(await models().item().count()).toBe(0);
 	});
 
+	test('should not do anything if the user is still enabled', async function() {
+		const { user: user1 } = await createUserAndSession(1);
+
+		const t0 = new Date('2021-12-14').getTime();
+		const t1 = t0 + 1000;
+
+		const job = await models().userDeletion().add(user1.id, t1, {
+			processData: false,
+			processAccount: true,
+		});
+
+		expect(await models().userDeletion().count()).toBe(1);
+
+		const service = newService();
+		await service.processDeletionJob(job, { sleepBetweenOperations: 0 });
+
+		// Nothing has been done because the user is still enabled
+		expect(await models().user().count()).toBe(1);
+
+		// And the job should have been removed from the queue
+		expect(await models().userDeletion().count()).toBe(0);
+	});
+
 });
diff --git a/packages/server/src/services/UserDeletionService.ts b/packages/server/src/services/UserDeletionService.ts
index 3167b2603..9385041b8 100644
--- a/packages/server/src/services/UserDeletionService.ts
+++ b/packages/server/src/services/UserDeletionService.ts
@@ -90,6 +90,20 @@ export default class UserDeletionService extends BaseService {
 
 		logger.info('Starting user deletion: ', deletion);
 
+		// Normally, a user that is still enabled should not be processed here,
+		// because it should not have been queued to begin with (or if it was
+		// queued, then enabled, it should have been removed from the queue).
+		// But as a fail safe we have this extra check.
+		//
+		// We also remove the job from the queue so that the service doesn't try
+		// to process it again.
+		const user = await this.models.user().load(deletion.user_id);
+		if (user.enabled) {
+			logger.error(`Trying to delete a user that is still enabled - aborting and removing the user from the queue. Deletion job: ${JSON.stringify(deletion)}`);
+			await this.models.userDeletion().removeFromQueueByUserId(user.id);
+			return;
+		}
+
 		let error: any = null;
 		let success: boolean = true;