From f38b8fef4d3d3c8bbc76d64884ff1211dc838d6b Mon Sep 17 00:00:00 2001 From: forehalo Date: Fri, 23 May 2025 03:57:29 +0000 Subject: [PATCH] feat(server): handle account deleting properly (#12399) ## Summary by CodeRabbit - **New Features** - Users are now prevented from deleting their account if they own one or more team workspaces. A clear error message instructs users to transfer ownership or delete those workspaces first. - Disabled (banned) users are explicitly prevented from signing in or re-registering. - Added new error messages and translations to improve clarity around account deletion restrictions. - **Bug Fixes** - Disabled users are now explicitly handled to prevent sign-in attempts. - **Tests** - Introduced comprehensive end-to-end tests covering account deletion, banning, and re-registration scenarios. - **Chores** - Improved event handling for user deletion and subscription cancellation. - Updated localization resources with new error messages. - Renamed payment event handler class for clarity. --- .../server/src/__tests__/e2e/create-app.ts | 10 +- .../src/__tests__/e2e/user/account.spec.ts | 150 ++++++++++++++++++ .../server/src/__tests__/mocks/user.mock.ts | 20 ++- packages/backend/server/src/base/error/def.ts | 7 + .../server/src/base/error/errors.gen.ts | 7 + .../server/src/core/auth/controller.ts | 8 +- packages/backend/server/src/core/doc/event.ts | 1 + packages/backend/server/src/models/user.ts | 28 +++- .../plugins/payment/{quota.ts => event.ts} | 2 +- .../server/src/plugins/payment/index.ts | 4 +- .../src/plugins/payment/manager/user.ts | 14 ++ packages/backend/server/src/schema.gql | 1 + packages/common/graphql/src/schema.ts | 1 + packages/frontend/i18n/src/i18n.gen.ts | 4 + packages/frontend/i18n/src/resources/en.json | 1 + 15 files changed, 235 insertions(+), 23 deletions(-) create mode 100644 packages/backend/server/src/__tests__/e2e/user/account.spec.ts rename packages/backend/server/src/plugins/payment/{quota.ts => event.ts} (98%) diff --git a/packages/backend/server/src/__tests__/e2e/create-app.ts b/packages/backend/server/src/__tests__/e2e/create-app.ts index 61c4ba2c85..782ea79989 100644 --- a/packages/backend/server/src/__tests__/e2e/create-app.ts +++ b/packages/backend/server/src/__tests__/e2e/create-app.ts @@ -154,12 +154,10 @@ export class TestingApp extends NestApplication { } async login(user: MockedUser) { - await this.POST('/api/auth/sign-in') - .send({ - email: user.email, - password: user.password, - }) - .expect(200); + return await this.POST('/api/auth/sign-in').send({ + email: user.email, + password: user.password, + }); } async switchUser(userOrId: string | { id: string }) { diff --git a/packages/backend/server/src/__tests__/e2e/user/account.spec.ts b/packages/backend/server/src/__tests__/e2e/user/account.spec.ts new file mode 100644 index 0000000000..03d25f3db4 --- /dev/null +++ b/packages/backend/server/src/__tests__/e2e/user/account.spec.ts @@ -0,0 +1,150 @@ +import { + deleteAccountMutation, + disableUserMutation, + getCurrentUserQuery, + getWorkspaceQuery, +} from '@affine/graphql'; + +import { app, e2e, Mockers } from '../test'; + +const admin = await app.create(Mockers.User, { + feature: 'administrator', +}); + +e2e('should be able to delete account', async t => { + const user = await app.signup(); + const user2 = await app.create(Mockers.User); + const ws = await app.create(Mockers.Workspace, { + owner: user, + }); + + await app.create(Mockers.WorkspaceUser, { + workspaceId: ws.id, + userId: user2.id, + }); + + await app.gql({ + query: deleteAccountMutation, + }); + + // assert session removed + const { currentUser } = await app.gql({ + query: getCurrentUserQuery, + }); + + t.is(currentUser, null); + + // assert login failed + const res = await app.login(user); + t.is(res.status, 400); + t.like(res.body, { + message: `Wrong user email or password: ${user.email}`, + }); + + // assert workspace access deleted + await app.login(user2); + await t.throwsAsync( + app.gql({ + query: getWorkspaceQuery, + variables: { + id: ws.id, + }, + }), + { + message: `You do not have permission to access Space ${ws.id}.`, + } + ); +}); + +e2e('should not delete account if is owner of team workspace', async t => { + const user = await app.signup(); + const ws = await app.create(Mockers.Workspace, { + owner: user, + }); + + await app.create(Mockers.TeamWorkspace, { + id: ws.id, + }); + + await t.throwsAsync( + app.gql({ + query: deleteAccountMutation, + }), + { + message: + 'Cannot delete account. You are the owner of one or more team workspaces. Please transfer ownership or delete them first.', + } + ); +}); + +e2e('should register deleted account again', async t => { + const user = await app.signup(); + await app.gql({ + query: deleteAccountMutation, + }); + + const res = await app.POST('/api/auth/sign-in').send({ + email: user.email, + }); + t.is(res.status, 200); + t.like(await app.mails.waitFor('SignUp'), { + to: user.email, + }); +}); + +e2e('should ban account', async t => { + const user = await app.create(Mockers.User); + + await app.login(admin); + + const { banUser } = await app.gql({ + query: disableUserMutation, + variables: { + id: user.id, + }, + }); + + t.is(banUser.disabled, true); +}); + +e2e('should not login banned account', async t => { + const user = await app.create(Mockers.User); + + await app.login(admin); + + await app.gql({ + query: disableUserMutation, + variables: { + id: user.id, + }, + }); + await app.logout(); + + const res = await app.login(user); + t.is(res.status, 400); + t.like(res.body, { + message: `Wrong user email or password: ${user.email}`, + }); +}); + +e2e('should not signup banned account', async t => { + const user = await app.create(Mockers.User); + + await app.login(admin); + + await app.gql({ + query: disableUserMutation, + variables: { + id: user.id, + }, + }); + + const res = await app.POST('/api/auth/sign-in').send({ + email: user.email, + }); + + t.is(res.status, 400); + t.like(res.body, { + message: `Wrong user email or password: ${user.email}`, + }); +}); diff --git a/packages/backend/server/src/__tests__/mocks/user.mock.ts b/packages/backend/server/src/__tests__/mocks/user.mock.ts index 5f760756a6..c75af8f77b 100644 --- a/packages/backend/server/src/__tests__/mocks/user.mock.ts +++ b/packages/backend/server/src/__tests__/mocks/user.mock.ts @@ -2,7 +2,7 @@ import { faker } from '@faker-js/faker'; import { hashSync } from '@node-rs/argon2'; import type { Prisma, User } from '@prisma/client'; -import type { UserFeatureName } from '../../models'; +import { FeatureConfigs, type UserFeatureName } from '../../models'; import { Mocker } from './factory'; export type MockUserInput = Prisma.UserCreateInput & { @@ -15,33 +15,37 @@ export type MockedUser = Omit & { export class MockUser extends Mocker { override async create(input?: Partial) { + const { feature, ...userInput } = input ?? {}; const password = input?.password ?? faker.internet.password(); const user = await this.db.user.create({ data: { email: faker.internet.email(), name: faker.person.fullName(), password: password ? hashSync(password) : undefined, - ...input, + ...userInput, }, }); - if (input?.feature) { - const feature = await this.db.feature.findFirst({ + if (feature) { + const featureRecord = await this.db.feature.findFirst({ where: { - name: input.feature, + name: feature, }, }); - if (!feature) { + if (!featureRecord) { throw new Error( - `Feature ${input.feature} does not exist in DB. You might forgot to run data-migration first.` + `Feature ${feature} does not exist in DB. You might forgot to run data-migration first.` ); } + const config = FeatureConfigs[feature]; await this.db.userFeature.create({ data: { userId: user.id, - featureId: feature.id, + featureId: featureRecord.id, + name: feature, + type: config.type, reason: 'test', activated: true, }, diff --git a/packages/backend/server/src/base/error/def.ts b/packages/backend/server/src/base/error/def.ts index 701669bde7..7f972ecdf6 100644 --- a/packages/backend/server/src/base/error/def.ts +++ b/packages/backend/server/src/base/error/def.ts @@ -792,10 +792,17 @@ export const USER_FRIENDLY_ERRORS = { type: 'action_forbidden', message: 'Cannot delete all admin accounts.', }, + + // Account errors cannot_delete_own_account: { type: 'action_forbidden', message: 'Cannot delete own account.', }, + cannot_delete_account_with_owned_team_workspace: { + type: 'action_forbidden', + message: + 'Cannot delete account. You are the owner of one or more team workspaces. Please transfer ownership or delete them first.', + }, // captcha errors captcha_verification_failed: { diff --git a/packages/backend/server/src/base/error/errors.gen.ts b/packages/backend/server/src/base/error/errors.gen.ts index ac5f95c3ab..8c54196615 100644 --- a/packages/backend/server/src/base/error/errors.gen.ts +++ b/packages/backend/server/src/base/error/errors.gen.ts @@ -908,6 +908,12 @@ export class CannotDeleteOwnAccount extends UserFriendlyError { } } +export class CannotDeleteAccountWithOwnedTeamWorkspace extends UserFriendlyError { + constructor(message?: string) { + super('action_forbidden', 'cannot_delete_account_with_owned_team_workspace', message); + } +} + export class CaptchaVerificationFailed extends UserFriendlyError { constructor(message?: string) { super('bad_request', 'captcha_verification_failed', message); @@ -1145,6 +1151,7 @@ export enum ErrorNames { MAILER_SERVICE_IS_NOT_CONFIGURED, CANNOT_DELETE_ALL_ADMIN_ACCOUNT, CANNOT_DELETE_OWN_ACCOUNT, + CANNOT_DELETE_ACCOUNT_WITH_OWNED_TEAM_WORKSPACE, CAPTCHA_VERIFICATION_FAILED, INVALID_LICENSE_SESSION_ID, LICENSE_REVEALED, diff --git a/packages/backend/server/src/core/auth/controller.ts b/packages/backend/server/src/core/auth/controller.ts index d77c615884..809d0b3dc2 100644 --- a/packages/backend/server/src/core/auth/controller.ts +++ b/packages/backend/server/src/core/auth/controller.ts @@ -27,6 +27,7 @@ import { Throttle, URLHelper, UseNamedGuard, + WrongSignInCredentials, } from '../../base'; import { Models, TokenType } from '../../models'; import { validators } from '../utils/validators'; @@ -162,7 +163,10 @@ export class AuthController { clientNonce?: string ) { // send email magic link - const user = await this.models.user.getUserByEmail(email); + const user = await this.models.user.getUserByEmail(email, { + withDisabled: true, + }); + if (!user) { if (!this.config.auth.allowSignup) { throw new SignUpForbidden(); @@ -191,6 +195,8 @@ export class AuthController { throw new InvalidEmail({ email }); } } + } else if (user.disabled) { + throw new WrongSignInCredentials({ email }); } const ttlInSec = 30 * 60; diff --git a/packages/backend/server/src/core/doc/event.ts b/packages/backend/server/src/core/doc/event.ts index 19ba3827b5..a9982fa954 100644 --- a/packages/backend/server/src/core/doc/event.ts +++ b/packages/backend/server/src/core/doc/event.ts @@ -41,6 +41,7 @@ export class DocEventsListener { @OnEvent('user.deleted') async clearUserWorkspaces(payload: Events['user.deleted']) { for (const workspace of payload.ownedWorkspaces) { + await this.models.workspace.delete(workspace); await this.workspace.deleteSpace(workspace); } } diff --git a/packages/backend/server/src/models/user.ts b/packages/backend/server/src/models/user.ts index d97518ab2d..f86a49e450 100644 --- a/packages/backend/server/src/models/user.ts +++ b/packages/backend/server/src/models/user.ts @@ -4,9 +4,11 @@ import { type ConnectedAccount, Prisma, type User } from '@prisma/client'; import { omit } from 'lodash-es'; import { + CannotDeleteAccountWithOwnedTeamWorkspace, CryptoHelper, EmailAlreadyUsed, EventBus, + UserNotFound, WrongSignInCredentials, WrongSignInMethod, } from '../base'; @@ -217,6 +219,10 @@ export class UserModel extends BaseModel { ...data, }); } else { + if (user.disabled) { + throw new UserNotFound(); + } + if (user.registered) { delete data.registered; } else { @@ -237,13 +243,25 @@ export class UserModel extends BaseModel { return user; } + async ownedWorkspaces(id: string) { + return await this.models.workspaceUser.getUserActiveRoles(id, { + role: WorkspaceRole.Owner, + }); + } + async delete(id: string) { - const ownedWorkspaces = await this.models.workspaceUser.getUserActiveRoles( - id, - { - role: WorkspaceRole.Owner, + const ownedWorkspaces = await this.ownedWorkspaces(id); + + for (const ws of ownedWorkspaces) { + const isTeamWorkspace = await this.models.workspace.isTeamWorkspace( + ws.workspaceId + ); + + if (isTeamWorkspace) { + throw new CannotDeleteAccountWithOwnedTeamWorkspace(); } - ); + } + const user = await this.db.user.delete({ where: { id } }); this.event.emit('user.deleted', { diff --git a/packages/backend/server/src/plugins/payment/quota.ts b/packages/backend/server/src/plugins/payment/event.ts similarity index 98% rename from packages/backend/server/src/plugins/payment/quota.ts rename to packages/backend/server/src/plugins/payment/event.ts index 125798cbd3..76d962e5b9 100644 --- a/packages/backend/server/src/plugins/payment/quota.ts +++ b/packages/backend/server/src/plugins/payment/event.ts @@ -6,7 +6,7 @@ import { Models } from '../../models'; import { SubscriptionPlan } from './types'; @Injectable() -export class QuotaOverride { +export class PaymentEventHandlers { constructor( private readonly workspace: WorkspaceService, private readonly models: Models, diff --git a/packages/backend/server/src/plugins/payment/index.ts b/packages/backend/server/src/plugins/payment/index.ts index 8aaa8b1a50..f0b98a30dc 100644 --- a/packages/backend/server/src/plugins/payment/index.ts +++ b/packages/backend/server/src/plugins/payment/index.ts @@ -11,13 +11,13 @@ import { UserModule } from '../../core/user'; import { WorkspaceModule } from '../../core/workspaces'; import { StripeWebhookController } from './controller'; import { SubscriptionCronJobs } from './cron'; +import { PaymentEventHandlers } from './event'; import { LicenseController } from './license/controller'; import { SelfhostTeamSubscriptionManager, UserSubscriptionManager, WorkspaceSubscriptionManager, } from './manager'; -import { QuotaOverride } from './quota'; import { SubscriptionResolver, UserSubscriptionResolver, @@ -49,7 +49,7 @@ import { StripeWebhook } from './webhook'; SelfhostTeamSubscriptionManager, SubscriptionCronJobs, WorkspaceSubscriptionResolver, - QuotaOverride, + PaymentEventHandlers, ], controllers: [StripeWebhookController, LicenseController], }) diff --git a/packages/backend/server/src/plugins/payment/manager/user.ts b/packages/backend/server/src/plugins/payment/manager/user.ts index 19b37611b6..4f6cb0481b 100644 --- a/packages/backend/server/src/plugins/payment/manager/user.ts +++ b/packages/backend/server/src/plugins/payment/manager/user.ts @@ -10,6 +10,7 @@ import { InternalServerError, InvalidCheckoutParameters, Mutex, + OnEvent, SubscriptionAlreadyExists, SubscriptionPlanNotFound, TooManyRequest, @@ -683,4 +684,17 @@ export class UserSubscriptionManager extends SubscriptionManager { throw new Error('user should exists for stripe subscription or invoice.'); } } + + @OnEvent('user.deleted') + async onUserDeleted({ id }: Events['user.deleted']) { + const subscription = await this.db.subscription.findFirst({ + where: { + targetId: id, + }, + }); + + if (subscription?.stripeSubscriptionId) { + await this.stripe.subscriptions.cancel(subscription.stripeSubscriptionId); + } + } } diff --git a/packages/backend/server/src/schema.gql b/packages/backend/server/src/schema.gql index 9376cd2d35..6f85380930 100644 --- a/packages/backend/server/src/schema.gql +++ b/packages/backend/server/src/schema.gql @@ -534,6 +534,7 @@ enum ErrorNames { BAD_REQUEST BLOB_NOT_FOUND BLOB_QUOTA_EXCEEDED + CANNOT_DELETE_ACCOUNT_WITH_OWNED_TEAM_WORKSPACE CANNOT_DELETE_ALL_ADMIN_ACCOUNT CANNOT_DELETE_OWN_ACCOUNT CANT_UPDATE_ONETIME_PAYMENT_SUBSCRIPTION diff --git a/packages/common/graphql/src/schema.ts b/packages/common/graphql/src/schema.ts index aafdbfec94..37b53da664 100644 --- a/packages/common/graphql/src/schema.ts +++ b/packages/common/graphql/src/schema.ts @@ -706,6 +706,7 @@ export enum ErrorNames { BAD_REQUEST = 'BAD_REQUEST', BLOB_NOT_FOUND = 'BLOB_NOT_FOUND', BLOB_QUOTA_EXCEEDED = 'BLOB_QUOTA_EXCEEDED', + CANNOT_DELETE_ACCOUNT_WITH_OWNED_TEAM_WORKSPACE = 'CANNOT_DELETE_ACCOUNT_WITH_OWNED_TEAM_WORKSPACE', CANNOT_DELETE_ALL_ADMIN_ACCOUNT = 'CANNOT_DELETE_ALL_ADMIN_ACCOUNT', CANNOT_DELETE_OWN_ACCOUNT = 'CANNOT_DELETE_OWN_ACCOUNT', CANT_UPDATE_ONETIME_PAYMENT_SUBSCRIPTION = 'CANT_UPDATE_ONETIME_PAYMENT_SUBSCRIPTION', diff --git a/packages/frontend/i18n/src/i18n.gen.ts b/packages/frontend/i18n/src/i18n.gen.ts index ffc8c9393f..3439acf75f 100644 --- a/packages/frontend/i18n/src/i18n.gen.ts +++ b/packages/frontend/i18n/src/i18n.gen.ts @@ -8614,6 +8614,10 @@ export function useAFFiNEI18N(): { * `Cannot delete own account.` */ ["error.CANNOT_DELETE_OWN_ACCOUNT"](): string; + /** + * `Cannot delete account. You are the owner of one or more team workspaces. Please transfer ownership or delete them first.` + */ + ["error.CANNOT_DELETE_ACCOUNT_WITH_OWNED_TEAM_WORKSPACE"](): string; /** * `Captcha verification failed.` */ diff --git a/packages/frontend/i18n/src/resources/en.json b/packages/frontend/i18n/src/resources/en.json index 23c4ced4ee..705a66eacf 100644 --- a/packages/frontend/i18n/src/resources/en.json +++ b/packages/frontend/i18n/src/resources/en.json @@ -2128,6 +2128,7 @@ "error.MAILER_SERVICE_IS_NOT_CONFIGURED": "Mailer service is not configured.", "error.CANNOT_DELETE_ALL_ADMIN_ACCOUNT": "Cannot delete all admin accounts.", "error.CANNOT_DELETE_OWN_ACCOUNT": "Cannot delete own account.", + "error.CANNOT_DELETE_ACCOUNT_WITH_OWNED_TEAM_WORKSPACE": "Cannot delete account. You are the owner of one or more team workspaces. Please transfer ownership or delete them first.", "error.CAPTCHA_VERIFICATION_FAILED": "Captcha verification failed.", "error.INVALID_LICENSE_SESSION_ID": "Invalid session id to generate license key.", "error.LICENSE_REVEALED": "License key has been revealed. Please check your mail box of the one provided during checkout.",