diff --git a/apps/backend/src/auth/auth.service.spec.ts b/apps/backend/src/auth/auth.service.spec.ts index 248f642d7..352c70d27 100644 --- a/apps/backend/src/auth/auth.service.spec.ts +++ b/apps/backend/src/auth/auth.service.spec.ts @@ -1,8 +1,15 @@ import { Test, TestingModule } from '@nestjs/testing'; +import { InternalServerErrorException } from '@nestjs/common'; +import { + AdminDisableUserCommand, + AdminEnableUserCommand, +} from '@aws-sdk/client-cognito-identity-provider'; import { AuthService } from './auth.service'; +import CognitoAuthConfig from './aws-exports'; describe('AuthService', () => { let service: AuthService; + let sendSpy: jest.SpyInstance; beforeEach(async () => { process.env.AWS_ACCESS_KEY_ID = 'test'; @@ -15,9 +22,69 @@ describe('AuthService', () => { }).compile(); service = module.get(AuthService); + + sendSpy = jest + .spyOn( + (service as unknown as { providerClient: { send: jest.Mock } }) + .providerClient, + 'send', + ) + .mockResolvedValue({} as never); + }); + + afterEach(() => { + jest.restoreAllMocks(); }); it('should be defined', () => { expect(service).toBeDefined(); }); + + describe('adminDisableUser', () => { + it('sends AdminDisableUserCommand with the email as Username', async () => { + await service.adminDisableUser('volunteer@example.com'); + + expect(sendSpy).toHaveBeenCalledTimes(1); + const command = sendSpy.mock.calls[0][0]; + expect(command).toBeInstanceOf(AdminDisableUserCommand); + expect(command.input).toEqual({ + UserPoolId: CognitoAuthConfig.userPoolId, + Username: 'volunteer@example.com', + }); + }); + + it('throws InternalServerErrorException when Cognito fails', async () => { + sendSpy.mockRejectedValueOnce(new Error('Cognito down')); + + await expect( + service.adminDisableUser('volunteer@example.com'), + ).rejects.toThrow( + new InternalServerErrorException('Failed to disable user'), + ); + }); + }); + + describe('adminEnableUser', () => { + it('sends AdminEnableUserCommand with the email as Username', async () => { + await service.adminEnableUser('volunteer@example.com'); + + expect(sendSpy).toHaveBeenCalledTimes(1); + const command = sendSpy.mock.calls[0][0]; + expect(command).toBeInstanceOf(AdminEnableUserCommand); + expect(command.input).toEqual({ + UserPoolId: CognitoAuthConfig.userPoolId, + Username: 'volunteer@example.com', + }); + }); + + it('throws InternalServerErrorException when Cognito fails', async () => { + sendSpy.mockRejectedValueOnce(new Error('Cognito down')); + + await expect( + service.adminEnableUser('volunteer@example.com'), + ).rejects.toThrow( + new InternalServerErrorException('Failed to enable user'), + ); + }); + }); }); diff --git a/apps/backend/src/auth/auth.service.ts b/apps/backend/src/auth/auth.service.ts index d03fcfde7..e19163cdc 100644 --- a/apps/backend/src/auth/auth.service.ts +++ b/apps/backend/src/auth/auth.service.ts @@ -6,6 +6,8 @@ import { import { CognitoIdentityProviderClient, AdminCreateUserCommand, + AdminDisableUserCommand, + AdminEnableUserCommand, } from '@aws-sdk/client-cognito-identity-provider'; import CognitoAuthConfig from './aws-exports'; @@ -70,4 +72,30 @@ export class AuthService { } } } + + async adminDisableUser(email: string): Promise { + const disableUserCommand = new AdminDisableUserCommand({ + UserPoolId: CognitoAuthConfig.userPoolId, + Username: email, + }); + + try { + await this.providerClient.send(disableUserCommand); + } catch { + throw new InternalServerErrorException('Failed to disable user'); + } + } + + async adminEnableUser(email: string): Promise { + const enableUserCommand = new AdminEnableUserCommand({ + UserPoolId: CognitoAuthConfig.userPoolId, + Username: email, + }); + + try { + await this.providerClient.send(enableUserCommand); + } catch { + throw new InternalServerErrorException('Failed to enable user'); + } + } } diff --git a/apps/backend/src/auth/jwt.strategy.ts b/apps/backend/src/auth/jwt.strategy.ts index 89225f859..9f699bafe 100644 --- a/apps/backend/src/auth/jwt.strategy.ts +++ b/apps/backend/src/auth/jwt.strategy.ts @@ -32,6 +32,9 @@ export class JwtStrategy extends PassportStrategy(Strategy) { async validate(payload: CognitoJwtPayload): Promise { try { const user = await this.usersService.findUserByCognitoId(payload.sub); + if (!user.active) { + return null; + } return user; } catch { return null; // Passport treats null as unauthenticated → clean 401 diff --git a/apps/backend/src/config/migrations.ts b/apps/backend/src/config/migrations.ts index 65a88e352..9e6387203 100644 --- a/apps/backend/src/config/migrations.ts +++ b/apps/backend/src/config/migrations.ts @@ -41,6 +41,7 @@ import { MakeFoodRescueRequired1773889925002 } from '../migrations/1773889925002 import { AddDonationItemConfirmation1774140453305 } from '../migrations/1774140453305-AddDonationItemConfirmation'; import { DonationItemsOnDeleteCascade1774214910101 } from '../migrations/1774214910101-DonationItemsOnDeleteCascade'; import { OrdersVolunteerActions1774883880543 } from '../migrations/1774883880543-OrdersVolunteerActions'; +import { AddUserActiveField1780531200000 } from '../migrations/1780531200000-AddUserActiveField'; const schemaMigrations = [ User1725726359198, @@ -86,6 +87,7 @@ const schemaMigrations = [ AddDonationItemConfirmation1774140453305, DonationItemsOnDeleteCascade1774214910101, OrdersVolunteerActions1774883880543, + AddUserActiveField1780531200000, ]; export default schemaMigrations; diff --git a/apps/backend/src/migrations/1780531200000-AddUserActiveField.ts b/apps/backend/src/migrations/1780531200000-AddUserActiveField.ts new file mode 100644 index 000000000..b124b71b3 --- /dev/null +++ b/apps/backend/src/migrations/1780531200000-AddUserActiveField.ts @@ -0,0 +1,17 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class AddUserActiveField1780531200000 implements MigrationInterface { + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(` + ALTER TABLE users + ADD COLUMN active boolean NOT NULL DEFAULT true + `); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(` + ALTER TABLE users + DROP COLUMN active + `); + } +} diff --git a/apps/backend/src/users/users.controller.spec.ts b/apps/backend/src/users/users.controller.spec.ts index fd923d516..466e8e8a6 100644 --- a/apps/backend/src/users/users.controller.spec.ts +++ b/apps/backend/src/users/users.controller.spec.ts @@ -26,7 +26,8 @@ describe('UsersController', () => { beforeEach(async () => { mockUserService.findUsersByRoles.mockReset(); mockUserService.findOne.mockReset(); - mockUserService.remove.mockReset(); + mockUserService.deactivate.mockReset(); + mockUserService.reactivate.mockReset(); mockUserService.update.mockReset(); mockUserService.create.mockReset(); mockUserService.getUserDashboardStats.mockReset(); @@ -64,14 +65,23 @@ describe('UsersController', () => { }); }); - describe('DELETE /:id', () => { - it('should remove a user by id', async () => { - mockUserService.remove.mockResolvedValue(mockUser1 as User); + describe('PATCH /:id/deactivate', () => { + it('should deactivate a user by id', async () => { + mockUserService.deactivate.mockResolvedValue(undefined); - const result = await controller.removeUser(1); + await controller.deactivateUser(1); - expect(result).toEqual(mockUser1); - expect(mockUserService.remove).toHaveBeenCalledWith(1); + expect(mockUserService.deactivate).toHaveBeenCalledWith(1); + }); + }); + + describe('PATCH /:id/reactivate', () => { + it('should reactivate a user by id', async () => { + mockUserService.reactivate.mockResolvedValue(undefined); + + await controller.reactivateUser(1); + + expect(mockUserService.reactivate).toHaveBeenCalledWith(1); }); }); diff --git a/apps/backend/src/users/users.controller.ts b/apps/backend/src/users/users.controller.ts index f43701343..3c2bc1222 100644 --- a/apps/backend/src/users/users.controller.ts +++ b/apps/backend/src/users/users.controller.ts @@ -1,6 +1,5 @@ import { Controller, - Delete, Get, Param, ParseIntPipe, @@ -51,12 +50,6 @@ export class UsersController { return this.usersService.getRecentPendingApplications(); } - @Roles(Role.ADMIN) - @Delete('/:id') - async removeUser(@Param('id', ParseIntPipe) userId: number): Promise { - return this.usersService.remove(userId); - } - @CheckOwnership({ idParam: 'id', resolver: resolveUserAuthorizedUserIds, @@ -70,6 +63,17 @@ export class UsersController { } @Roles(Role.ADMIN) + @Patch('/:id/deactivate') + async deactivateUser(@Param('id', ParseIntPipe) id: number): Promise { + return this.usersService.deactivate(id); + } + + @Roles(Role.ADMIN) + @Patch('/:id/reactivate') + async reactivateUser(@Param('id', ParseIntPipe) id: number): Promise { + return this.usersService.reactivate(id); + } + @Post('/') async createUser(@Body() createUserDto: userSchemaDto): Promise { return this.usersService.create(createUserDto); diff --git a/apps/backend/src/users/users.entity.ts b/apps/backend/src/users/users.entity.ts index c6c02107e..e8fa05e08 100644 --- a/apps/backend/src/users/users.entity.ts +++ b/apps/backend/src/users/users.entity.ts @@ -58,6 +58,12 @@ export class User { }) userCognitoSub!: string; + @Column({ + type: 'boolean', + default: true, + }) + active!: boolean; + @ManyToMany(() => Pantry, (pantry) => pantry.volunteers) @JoinTable({ name: 'volunteer_assignments', diff --git a/apps/backend/src/users/users.service.spec.ts b/apps/backend/src/users/users.service.spec.ts index f504fe40d..03c76ca57 100644 --- a/apps/backend/src/users/users.service.spec.ts +++ b/apps/backend/src/users/users.service.spec.ts @@ -37,6 +37,8 @@ jest.setTimeout(60000); const mockAuthService = { adminCreateUser: jest.fn().mockResolvedValue('mock-sub'), + adminDisableUser: jest.fn().mockResolvedValue(undefined), + adminEnableUser: jest.fn().mockResolvedValue(undefined), }; const mockEmailsService = mock(); @@ -125,6 +127,8 @@ describe('UsersService', () => { beforeEach(async () => { mockAuthService.adminCreateUser.mockClear(); + mockAuthService.adminDisableUser.mockClear(); + mockAuthService.adminEnableUser.mockClear(); mockEmailsService.sendEmails.mockClear(); await testDataSource.runMigrations(); }); @@ -293,32 +297,110 @@ describe('UsersService', () => { }); }); - describe('remove', () => { - it('should remove a user by id', async () => { + describe('deactivate', () => { + it('deactivates an active volunteer: sets active=false and disables Cognito', async () => { await testDataSource.query( - `DELETE FROM allocations WHERE order_id IN (SELECT order_id FROM orders WHERE assignee_id = 6)`, + `UPDATE users SET user_cognito_sub = 'cognito-sub-6' WHERE user_id = 6`, ); - await testDataSource.query(`DELETE FROM orders WHERE assignee_id = 6`); - const result = await service.remove(6); + const before = (await testDataSource + .getRepository(User) + .findOneBy({ id: 6 })) as User; + + expect(before.active).toBe(true); + + await service.deactivate(6); - expect(result.email).toBe('james.t@volunteer.org'); + expect(mockAuthService.adminDisableUser).toHaveBeenCalledWith( + 'james.t@volunteer.org', + ); const fromDb = await testDataSource .getRepository(User) .findOneBy({ id: 6 }); - expect(fromDb).toBeNull(); + expect(fromDb?.active).toBe(false); }); - it('should throw NotFoundException when user is not found', async () => { - await expect(service.remove(9999)).rejects.toThrow( + it('skips the Cognito call when the user has no Cognito account', async () => { + const before = (await testDataSource + .getRepository(User) + .findOneBy({ id: 6 })) as User; + + expect(before.userCognitoSub).toBe(''); + + await service.deactivate(6); + + expect(mockAuthService.adminDisableUser).not.toHaveBeenCalled(); + + const fromDb = await testDataSource + .getRepository(User) + .findOneBy({ id: 6 }); + expect(fromDb?.active).toBe(false); + }); + + it('throws BadRequestException when deactivating an already-inactive user', async () => { + const before = (await testDataSource + .getRepository(User) + .findOneBy({ id: 6 })) as User; + + expect(before.active).toBe(true); + + await service.deactivate(6); + + await expect(service.deactivate(6)).rejects.toThrow( + new BadRequestException('User 6 is already inactive'), + ); + }); + + it('throws NotFoundException when user is not found', async () => { + await expect(service.deactivate(9999)).rejects.toThrow( new NotFoundException('User 9999 not found'), ); }); + }); - it('should throw error for invalid id', async () => { - await expect(service.remove(-1)).rejects.toThrow( - new BadRequestException('Invalid User ID'), + describe('reactivate', () => { + it('reactivates a deactivated volunteer: sets active=true and enables Cognito', async () => { + await testDataSource.query( + `UPDATE users SET user_cognito_sub = 'cognito-sub-6' WHERE user_id = 6`, + ); + + const before = (await testDataSource + .getRepository(User) + .findOneBy({ id: 6 })) as User; + + expect(before.active).toBe(true); + + await service.deactivate(6); + mockAuthService.adminEnableUser.mockClear(); + + await service.reactivate(6); + + expect(mockAuthService.adminEnableUser).toHaveBeenCalledWith( + 'james.t@volunteer.org', + ); + + const fromDb = await testDataSource + .getRepository(User) + .findOneBy({ id: 6 }); + expect(fromDb?.active).toBe(true); + }); + + it('throws BadRequestException when reactivating an already-active user', async () => { + const before = (await testDataSource + .getRepository(User) + .findOneBy({ id: 6 })) as User; + + expect(before.active).toBe(true); + + await expect(service.reactivate(6)).rejects.toThrow( + new BadRequestException('User 6 is already active'), + ); + }); + + it('throws NotFoundException when user is not found', async () => { + await expect(service.reactivate(9999)).rejects.toThrow( + new NotFoundException('User 9999 not found'), ); }); }); diff --git a/apps/backend/src/users/users.service.ts b/apps/backend/src/users/users.service.ts index a5ea7db2d..78db4888e 100644 --- a/apps/backend/src/users/users.service.ts +++ b/apps/backend/src/users/users.service.ts @@ -205,16 +205,39 @@ export class UsersService { return this.repo.save(user); } - async remove(id: number) { - validateId(id, 'User'); + async deactivate(id: number): Promise { + const user = await this.findOne(id); + + if (!user.active) { + throw new BadRequestException(`User ${id} is already inactive`); + } + + // Disable the Cognito account first so a failure leaves active field unchanged. + // Users without a Cognito account (empty sub) have nothing to disable. + // The only users without a Cognito account are unapproved FMs and pantries + if (user.userCognitoSub) { + await this.authService.adminDisableUser(user.email); + } + + user.active = false; + await this.repo.save(user); + } + async reactivate(id: number): Promise { const user = await this.findOne(id); - if (!user) { - throw new NotFoundException(`User ${id} not found`); + if (user.active) { + throw new BadRequestException(`User ${id} is already active`); } - return this.repo.remove(user); + // Re-enable the Cognito account first so a failure leaves active field unchanged. + // Users without a Cognito account (empty sub) have nothing to enable. + if (user.userCognitoSub) { + await this.authService.adminEnableUser(user.email); + } + + user.active = true; + await this.repo.save(user); } async findUsersByRoles(roles: Role[]): Promise { diff --git a/apps/backend/src/volunteers/volunteers.service.spec.ts b/apps/backend/src/volunteers/volunteers.service.spec.ts index d8a3089d5..ab4b95679 100644 --- a/apps/backend/src/volunteers/volunteers.service.spec.ts +++ b/apps/backend/src/volunteers/volunteers.service.spec.ts @@ -170,6 +170,7 @@ describe('VolunteersService', () => { phone: '555-040-0401', role: 'volunteer', userCognitoSub: '', + active: true, pantryIds: [1], }, { @@ -180,6 +181,7 @@ describe('VolunteersService', () => { phone: '555-040-0402', role: 'volunteer', userCognitoSub: '', + active: true, pantryIds: [2, 3], }, { @@ -190,6 +192,7 @@ describe('VolunteersService', () => { phone: '555-040-0403', role: 'volunteer', userCognitoSub: '', + active: true, pantryIds: [3], }, { @@ -200,6 +203,7 @@ describe('VolunteersService', () => { phone: '555-040-0404', role: 'volunteer', userCognitoSub: '', + active: true, pantryIds: [1], }, ]); diff --git a/apps/frontend/src/types/types.ts b/apps/frontend/src/types/types.ts index fe4f111db..adf226ceb 100644 --- a/apps/frontend/src/types/types.ts +++ b/apps/frontend/src/types/types.ts @@ -267,6 +267,7 @@ export interface User { email: string; phone: string; pantries?: Pantry[]; + active: boolean; } export type UpdateProfileFields = Partial<