diff --git a/migrations/20260821202931-create-mail-deleted-addresses.js b/migrations/20260821202931-create-mail-deleted-addresses.js new file mode 100644 index 0000000..8043af7 --- /dev/null +++ b/migrations/20260821202931-create-mail-deleted-addresses.js @@ -0,0 +1,40 @@ +'use strict'; + +const TABLE_NAME = 'mail_deleted_addresses'; + +/** @type {import('sequelize-cli').Migration} */ +module.exports = { + async up(queryInterface, Sequelize) { + await queryInterface.createTable(TABLE_NAME, { + id: { + type: Sequelize.UUID, + defaultValue: Sequelize.UUIDV4, + primaryKey: true, + allowNull: false, + }, + address: { + type: Sequelize.STRING(255), + allowNull: false, + unique: true, + }, + user_id: { + type: Sequelize.UUID, + allowNull: false, + }, + created_at: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.fn('now'), + }, + updated_at: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.fn('now'), + }, + }); + }, + + async down(queryInterface) { + await queryInterface.dropTable(TABLE_NAME); + }, +}; diff --git a/src/modules/account/account.module.ts b/src/modules/account/account.module.ts index 03192cd..e545429 100644 --- a/src/modules/account/account.module.ts +++ b/src/modules/account/account.module.ts @@ -12,11 +12,13 @@ import { MailAddressKeysModel, MailAccountModel, MailAddressModel, + MailDeletedAddressModel, MailDomainModel, MailProviderAccountModel, } from './models/index.js'; import { AccountRepository } from './repositories/account.repository.js'; import { AddressRepository } from './repositories/address.repository.js'; +import { DeletedAddressRepository } from './repositories/deleted-address.repository.js'; import { DomainRepository } from './repositories/domain.repository.js'; import { MailAddressKeysRepository } from './repositories/mail-address-keys.repository.js'; @@ -26,6 +28,7 @@ import { MailAddressKeysRepository } from './repositories/mail-address-keys.repo MailAccountModel, MailAddressKeysModel, MailAddressModel, + MailDeletedAddressModel, MailDomainModel, MailProviderAccountModel, ]), @@ -37,6 +40,7 @@ import { MailAddressKeysRepository } from './repositories/mail-address-keys.repo providers: [ AccountRepository, AddressRepository, + DeletedAddressRepository, DomainRepository, MailAddressKeysRepository, AccountService, diff --git a/src/modules/account/account.service.spec.ts b/src/modules/account/account.service.spec.ts index 78529d8..c37663d 100644 --- a/src/modules/account/account.service.spec.ts +++ b/src/modules/account/account.service.spec.ts @@ -15,6 +15,7 @@ import { MailDomain } from './domain/mail-domain.domain.js'; import { MailAddress } from './domain/mail-address.domain.js'; import { AccountRepository } from './repositories/account.repository.js'; import { AddressRepository } from './repositories/address.repository.js'; +import { DeletedAddressRepository } from './repositories/deleted-address.repository.js'; import { DomainRepository } from './repositories/domain.repository.js'; import { MailAddressKeysRepository } from './repositories/mail-address-keys.repository.js'; import { @@ -50,6 +51,7 @@ describe('AccountService', () => { let provider: DeepMocked; let accounts: DeepMocked; let addresses: DeepMocked; + let deletedAddresses: DeepMocked; let domains: DeepMocked; let keys: DeepMocked; let bridge: DeepMocked; @@ -67,6 +69,7 @@ describe('AccountService', () => { provider = module.get(AccountProvider); accounts = module.get(AccountRepository); addresses = module.get(AddressRepository); + deletedAddresses = module.get(DeletedAddressRepository); domains = module.get(DomainRepository); keys = module.get(MailAddressKeysRepository); bridge = module.get(BridgeClient); @@ -77,6 +80,7 @@ describe('AccountService', () => { maxSpaceBytes: 1000, totalUsedSpaceBytes: 0, }); + deletedAddresses.findClaimedByOthers.mockResolvedValue(new Set()); }); describe('getAccount', () => { @@ -661,6 +665,20 @@ describe('AccountService', () => { expect(provider.createAccount).not.toHaveBeenCalled(); }); + it('when the address was given up by another user, then throws a conflict', async () => { + domains.findByDomain.mockResolvedValue(domain); + addresses.findByAddress.mockResolvedValue(null); + accounts.findByUserId.mockResolvedValue(null); + deletedAddresses.findClaimedByOthers.mockResolvedValue( + new Set([params.address]), + ); + + await expect(service.provisionAccount(params)).rejects.toThrow( + ConflictException, + ); + expect(accounts.create).not.toHaveBeenCalled(); + }); + it('when unique collision occurs but no account is visible, then throws a conflict', async () => { const uniqueError = new Error('Unique constraint violated'); uniqueError.name = 'SequelizeUniqueConstraintError'; @@ -816,6 +834,33 @@ describe('AccountService', () => { expect(accounts.delete).not.toHaveBeenCalled(); }); + it('when the account is destroyed, then every address is tombstoned first', async () => { + const addr1 = newMailAddressAttributes({ isDefault: true }); + const addr2 = newMailAddressAttributes({ isDefault: false }); + const account = MailAccount.build( + newMailAccountAttributes({ addresses: [addr1, addr2] }), + ); + accounts.findByUserId.mockResolvedValue(account); + + await service.deleteAccount(account.userId); + + expect(deletedAddresses.record).toHaveBeenCalledWith([ + { address: addr1.address, userId: account.userId }, + { address: addr2.address, userId: account.userId }, + ]); + }); + + it('when tombstoning fails, then the rows are kept so the address stays taken', async () => { + const account = MailAccount.build(newMailAccountAttributes()); + accounts.findByUserId.mockResolvedValue(account); + deletedAddresses.record.mockRejectedValue(new Error('DB down')); + + await expect(service.deleteAccount(account.userId)).rejects.toThrow( + 'DB down', + ); + expect(accounts.delete).not.toHaveBeenCalled(); + }); + it('when account does not exist, then throws NotFoundException', async () => { accounts.findByUserId.mockResolvedValue(null); @@ -1191,6 +1236,25 @@ describe('AccountService', () => { ); }); + it('when an address is removed, then it is tombstoned so nobody else gets it', async () => { + const nonDefaultAddr = newMailAddressAttributes({ isDefault: false }); + const account = MailAccount.build( + newMailAccountAttributes({ + addresses: [ + newMailAddressAttributes({ isDefault: true }), + nonDefaultAddr, + ], + }), + ); + accounts.findByUserId.mockResolvedValue(account); + + await service.removeAddress(account.userId, nonDefaultAddr.address); + + expect(deletedAddresses.record).toHaveBeenCalledWith([ + { address: nonDefaultAddr.address, userId: account.userId }, + ]); + }); + it('when address is default, then throws UnprocessableEntityException', async () => { const defaultAddr = newMailAddressAttributes({ isDefault: true }); const account = MailAccount.build( @@ -1239,7 +1303,11 @@ describe('AccountService', () => { }); it('when domain is available and address is not taken, return is available', async () => { - const res = await service.checkAddressAvailability('username', 'domain'); + const res = await service.checkAddressAvailability( + 'username', + 'domain', + 'user-1', + ); expect(res).toStrictEqual({ available: true, suggestion: null }); @@ -1252,7 +1320,11 @@ describe('AccountService', () => { newMailDomainAttributes({ domain: 'domain2' }), ]); - const res = await service.checkAddressAvailability('username', 'domain'); + const res = await service.checkAddressAvailability( + 'username', + 'domain', + 'user-1', + ); expect(res).toStrictEqual({ available: false, @@ -1267,7 +1339,11 @@ describe('AccountService', () => { it('when address is taken, return is not available and suggestion', async () => { addresses.findByAddresses.mockResolvedValue(new Set(['username@domain'])); - const res = await service.checkAddressAvailability('username', 'domain'); + const res = await service.checkAddressAvailability( + 'username', + 'domain', + 'user-1', + ); expect(res).toStrictEqual({ available: false, @@ -1276,10 +1352,47 @@ describe('AccountService', () => { expect(addresses.findByAddresses).toHaveBeenCalledExactlyOnceWith(taken); }); + it('when an address was given up by another user, then it is not offered', async () => { + deletedAddresses.findClaimedByOthers.mockResolvedValue( + new Set(['username@domain']), + ); + + const res = await service.checkAddressAvailability( + 'username', + 'domain', + 'user-1', + ); + + expect(deletedAddresses.findClaimedByOthers).toHaveBeenCalledWith( + taken, + 'user-1', + ); + expect(res).toStrictEqual({ + available: false, + suggestion: 'username@domain1', + }); + }); + + it('when the caller gave the address up themselves, then they may take it back', async () => { + deletedAddresses.findClaimedByOthers.mockResolvedValue(new Set()); + + const res = await service.checkAddressAvailability( + 'username', + 'domain', + 'user-1', + ); + + expect(res).toStrictEqual({ available: true, suggestion: null }); + }); + it('when all suggestions are taken, return is not available and no suggestion', async () => { addresses.findByAddresses.mockResolvedValue(new Set(taken)); - const res = await service.checkAddressAvailability('username', 'domain'); + const res = await service.checkAddressAvailability( + 'username', + 'domain', + 'user-1', + ); expect(res).toStrictEqual({ available: false, suggestion: null }); expect(addresses.findByAddresses).toHaveBeenCalledExactlyOnceWith(taken); diff --git a/src/modules/account/account.service.ts b/src/modules/account/account.service.ts index fa27cb3..81d33f6 100644 --- a/src/modules/account/account.service.ts +++ b/src/modules/account/account.service.ts @@ -25,6 +25,7 @@ import { AddressRepository, type ProviderAccountBucketContext, } from './repositories/address.repository.js'; +import { DeletedAddressRepository } from './repositories/deleted-address.repository.js'; import { DomainRepository } from './repositories/domain.repository.js'; import { MailAddressKeysRepository } from './repositories/mail-address-keys.repository.js'; @@ -50,6 +51,7 @@ export class AccountService { private readonly provider: AccountProvider, private readonly accounts: AccountRepository, private readonly addresses: AddressRepository, + private readonly deletedAddresses: DeletedAddressRepository, private readonly domains: DomainRepository, private readonly keys: MailAddressKeysRepository, private readonly bridge: BridgeClient, @@ -173,14 +175,24 @@ export class AccountService { displayName: string; keys: MailAddressKeyBundle; }): Promise { - const [tier, usage, domainRecord, existingAddress, existingAccount] = - await Promise.all([ - this.payments.getUserTier(params.userId), - this.bridge.getUserUsage(params.userId), - this.domains.findByDomain(params.domain), - this.addresses.findByAddress(params.address), - this.accounts.findByUserId(params.userId), - ]); + const [ + tier, + usage, + domainRecord, + existingAddress, + existingAccount, + givenUp, + ] = await Promise.all([ + this.payments.getUserTier(params.userId), + this.bridge.getUserUsage(params.userId), + this.domains.findByDomain(params.domain), + this.addresses.findByAddress(params.address), + this.accounts.findByUserId(params.userId), + this.deletedAddresses.findClaimedByOthers( + [params.address], + params.userId, + ), + ]); if (!tier.featuresPerService.mail?.enabled) { throw new ForbiddenException( @@ -193,7 +205,7 @@ export class AccountService { if (existingAccount) { throw new ConflictException('User already has a mail account'); } - if (existingAddress) { + if (existingAddress || givenUp.has(params.address)) { throw new ConflictException( `Address '${params.address}' is already in use`, ); @@ -288,6 +300,14 @@ export class AccountService { this.releaseNetworkBucket(driveUserUuid, a.networkBucketId), ), ); + + await this.deletedAddresses.record( + account.addresses.map((a) => ({ + address: a.address, + userId: driveUserUuid, + })), + ); + await this.accounts.delete(account.id, { force: true }); this.logger.log(`Deleted account for user '${driveUserUuid}'`); @@ -300,11 +320,12 @@ export class AccountService { password: string, displayName?: string, ): Promise { - const [usage, account, domain, existing] = await Promise.all([ + const [usage, account, domain, existing, givenUp] = await Promise.all([ this.bridge.getUserUsage(userId), this.accounts.findByUserId(userId), this.domains.findByDomain(domainName), this.addresses.findByAddress(address), + this.deletedAddresses.findClaimedByOthers([address], userId), ]); if (!account) { @@ -313,7 +334,7 @@ export class AccountService { if (!domain) { throw new NotFoundException(`Domain '${domainName}' not found`); } - if (existing) { + if (existing || givenUp.has(address)) { throw new ConflictException(`Address '${address}' already exists`); } @@ -378,6 +399,7 @@ export class AccountService { } await this.provider.deleteAccount(addressRecord.providerExternalId); + await this.deletedAddresses.record([{ address, userId }]); await Promise.all([ this.addresses.deleteProviderLink(addressRecord.id), this.addresses.delete(addressRecord.id), @@ -411,6 +433,7 @@ export class AccountService { async checkAddressAvailability( username: string, domain: string, + userId: string, ): Promise<{ available: boolean; suggestion: string | null }> { const activeMailDomains = await this.domains.findAllActive(); const activeDomains = activeMailDomains.map((m) => m.domain); @@ -438,8 +461,13 @@ export class AccountService { ); } - const taken = await this.addresses.findByAddresses(possibleAddresses); - const suggestion = possibleAddresses.find((a) => !taken.has(a)); + const [taken, givenUp] = await Promise.all([ + this.addresses.findByAddresses(possibleAddresses), + this.deletedAddresses.findClaimedByOthers(possibleAddresses, userId), + ]); + const suggestion = possibleAddresses.find( + (a) => !taken.has(a) && !givenUp.has(a), + ); if (suggestion === requestedAddress) { return { available: true, suggestion: null }; diff --git a/src/modules/account/models/index.ts b/src/modules/account/models/index.ts index 4e1d913..65d8d17 100644 --- a/src/modules/account/models/index.ts +++ b/src/modules/account/models/index.ts @@ -1,5 +1,6 @@ export { MailAccountModel } from './mail-account.model.js'; export { MailAddressKeysModel } from './mail-address-keys.model.js'; export { MailAddressModel } from './mail-address.model.js'; +export { MailDeletedAddressModel } from './mail-deleted-address.model.js'; export { MailDomainModel } from './mail-domain.model.js'; export { MailProviderAccountModel } from './mail-provider-account.model.js'; diff --git a/src/modules/account/models/mail-deleted-address.model.ts b/src/modules/account/models/mail-deleted-address.model.ts new file mode 100644 index 0000000..5f75a0e --- /dev/null +++ b/src/modules/account/models/mail-deleted-address.model.ts @@ -0,0 +1,31 @@ +import { + AllowNull, + Column, + DataType, + Default, + Model, + PrimaryKey, + Table, + Unique, +} from 'sequelize-typescript'; + +@Table({ + underscored: true, + timestamps: true, + tableName: 'mail_deleted_addresses', +}) +export class MailDeletedAddressModel extends Model { + @PrimaryKey + @Default(DataType.UUIDV4) + @Column(DataType.UUID) + declare id: string; + + @AllowNull(false) + @Unique + @Column(DataType.STRING(255)) + declare address: string; + + @AllowNull(false) + @Column(DataType.UUID) + declare userId: string; +} diff --git a/src/modules/account/repositories/deleted-address.repository.spec.ts b/src/modules/account/repositories/deleted-address.repository.spec.ts new file mode 100644 index 0000000..7c5e7a9 --- /dev/null +++ b/src/modules/account/repositories/deleted-address.repository.spec.ts @@ -0,0 +1,89 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { Test, type TestingModule } from '@nestjs/testing'; +import { getModelToken } from '@nestjs/sequelize'; +import { createMock, type DeepMocked } from '@golevelup/ts-vitest'; +import { Op } from 'sequelize'; +import { DeletedAddressRepository } from './deleted-address.repository.js'; +import { MailDeletedAddressModel } from '../models/mail-deleted-address.model.js'; + +describe('DeletedAddressRepository', () => { + let repository: DeletedAddressRepository; + let model: DeepMocked; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [DeletedAddressRepository], + }) + .useMocker((token) => { + if (token === getModelToken(MailDeletedAddressModel)) { + return createMock(); + } + return createMock(); + }) + .compile(); + + repository = module.get(DeletedAddressRepository); + model = module.get(getModelToken(MailDeletedAddressModel)); + }); + + describe('record', () => { + it('when given addresses, then inserts them ignoring ones already recorded', async () => { + const entries = [ + { address: 'alice@inxt.com', userId: 'user-1' }, + { address: 'alice2@inxt.com', userId: 'user-1' }, + ]; + + await repository.record(entries); + + expect(model.bulkCreate).toHaveBeenCalledWith(entries, { + ignoreDuplicates: true, + }); + }); + + it('when there is nothing to record, then does not touch the database', async () => { + await repository.record([]); + + expect(model.bulkCreate).not.toHaveBeenCalled(); + }); + }); + + describe('findClaimedByOthers', () => { + it('when addresses were given up by other users, then returns those addresses', async () => { + model.findAll.mockResolvedValue([ + { address: 'alice@inxt.com' }, + ] as unknown as MailDeletedAddressModel[]); + + const result = await repository.findClaimedByOthers( + ['alice@inxt.com', 'bob@inxt.com'], + 'user-1', + ); + + expect(model.findAll).toHaveBeenCalledWith({ + where: { + address: { [Op.in]: ['alice@inxt.com', 'bob@inxt.com'] }, + userId: { [Op.ne]: 'user-1' }, + }, + attributes: ['address'], + }); + expect(result).toEqual(new Set(['alice@inxt.com'])); + }); + + it('when nothing was given up, then returns an empty set', async () => { + model.findAll.mockResolvedValue([]); + + const result = await repository.findClaimedByOthers( + ['alice@inxt.com'], + 'user-1', + ); + + expect(result).toEqual(new Set()); + }); + + it('when asked about no addresses, then does not touch the database', async () => { + const result = await repository.findClaimedByOthers([], 'user-1'); + + expect(model.findAll).not.toHaveBeenCalled(); + expect(result).toEqual(new Set()); + }); + }); +}); diff --git a/src/modules/account/repositories/deleted-address.repository.ts b/src/modules/account/repositories/deleted-address.repository.ts new file mode 100644 index 0000000..3a73441 --- /dev/null +++ b/src/modules/account/repositories/deleted-address.repository.ts @@ -0,0 +1,44 @@ +import { Injectable } from '@nestjs/common'; +import { InjectModel } from '@nestjs/sequelize'; +import { Op } from 'sequelize'; +import { MailDeletedAddressModel } from '../models/mail-deleted-address.model.js'; + +const MAX_BATCH_LOOKUP = 50; + +export type RecordDeletedAddressParams = { + address: string; + userId: string; +}; + +@Injectable() +export class DeletedAddressRepository { + constructor( + @InjectModel(MailDeletedAddressModel) + private readonly deletedAddressModel: typeof MailDeletedAddressModel, + ) {} + + async record(entries: RecordDeletedAddressParams[]): Promise { + if (entries.length === 0) return; + + await this.deletedAddressModel.bulkCreate(entries, { + ignoreDuplicates: true, + }); + } + + async findClaimedByOthers( + addresses: string[], + userId: string, + ): Promise> { + if (addresses.length === 0) return new Set(); + + const models = await this.deletedAddressModel.findAll({ + where: { + address: { [Op.in]: addresses.slice(0, MAX_BATCH_LOOKUP) }, + userId: { [Op.ne]: userId }, + }, + attributes: ['address'], + }); + + return new Set(models.map((m) => m.address)); + } +} diff --git a/src/modules/addresses/addresses.controller.spec.ts b/src/modules/addresses/addresses.controller.spec.ts index 47c8ead..175c7ea 100644 --- a/src/modules/addresses/addresses.controller.spec.ts +++ b/src/modules/addresses/addresses.controller.spec.ts @@ -3,10 +3,12 @@ import { Test, type TestingModule } from '@nestjs/testing'; import { createMock, type DeepMocked } from '@golevelup/ts-vitest'; import { AddressesController } from './addresses.controller.js'; import { AccountService } from '../account/account.service.js'; +import { newUserPayload } from '../../../test/fixtures.js'; describe('AddressesController', () => { let controller: AddressesController; let accountService: DeepMocked; + const user = newUserPayload(); beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ @@ -26,7 +28,7 @@ describe('AddressesController', () => { suggestion: null, }); - const result = await controller.checkAvailability({ + const result = await controller.checkAvailability(user, { username: 'alice', domain: 'inxt.me', }); @@ -34,6 +36,7 @@ describe('AddressesController', () => { expect(accountService.checkAddressAvailability).toHaveBeenCalledWith( 'alice', 'inxt.me', + user.uuid, ); expect(result).toStrictEqual({ available: true, @@ -48,7 +51,7 @@ describe('AddressesController', () => { suggestion: 'alice1@inxt.me', }); - const result = await controller.checkAvailability({ + const result = await controller.checkAvailability(user, { username: 'alice', domain: 'inxt.me', }); @@ -56,6 +59,7 @@ describe('AddressesController', () => { expect(accountService.checkAddressAvailability).toHaveBeenCalledWith( 'alice', 'inxt.me', + user.uuid, ); expect(result).toStrictEqual({ available: false, diff --git a/src/modules/addresses/addresses.controller.ts b/src/modules/addresses/addresses.controller.ts index 04665f8..c6c343c 100644 --- a/src/modules/addresses/addresses.controller.ts +++ b/src/modules/addresses/addresses.controller.ts @@ -1,6 +1,8 @@ import { Controller, Get, Query } from '@nestjs/common'; import { ApiBearerAuth, ApiOperation, ApiTags } from '@nestjs/swagger'; import { AccountService } from '../account/account.service.js'; +import { User } from '../auth/decorators/user.decorator.js'; +import type { UserPayload } from '../auth/jwt-payload.dto.js'; import { CheckAvailabilityQueryDto, CheckAvailabilityResponseDto, @@ -17,11 +19,13 @@ export class AddressesController { summary: 'Check address availability (called by the auth service)', }) async checkAvailability( + @User() user: UserPayload, @Query() query: CheckAvailabilityQueryDto, ): Promise { return this.accountService.checkAddressAvailability( query.username, query.domain, + user.uuid, ); } }