-
Notifications
You must be signed in to change notification settings - Fork 17
BC-10977 - public endpoint for admin to create user deletion request #6083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
virgilchiriac
wants to merge
17
commits into
main
Choose a base branch
from
BC-10977-user-delete-via-knl
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+394
−4
Open
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
6e968a1
admin-api add public endpoint for admin to create user deletion request
virgilchiriac 105cdf0
cleanup
virgilchiriac dcea824
add Valkey envs for admin-api
virgilchiriac 8befc17
add public api deletion module
virgilchiriac 624a88a
Merge branch 'main' into BC-10977-user-delete-via-knl
virgilchiriac 51c7e45
undo change
virgilchiriac d6dea21
set default to imediat deletion
virgilchiriac f36e76e
refactor and add api tests
virgilchiriac b9e2cf7
fix tests and linter
virgilchiriac 6c10a3b
restrict deletion for students and teachers only
virgilchiriac 235956c
fix coverage
virgilchiriac a632175
Merge branch 'main' into BC-10977-user-delete-via-knl
virgilchiriac e27aa3c
linter fix
virgilchiriac a2383bf
Merge branch 'BC-10977-user-delete-via-knl' of https://github.com/hpi…
virgilchiriac 950fd74
max number of users adjustment according to UI
virgilchiriac a3201f9
prevent error for users without accounts
virgilchiriac edf58b5
Merge branch 'main' into BC-10977-user-delete-via-knl
virgilchiriac File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
213 changes: 213 additions & 0 deletions
213
...r/src/modules/deletion/api/controller/api-test/deletion-request-public-create.api.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,213 @@ | ||
| import { EntityManager, ObjectId } from '@mikro-orm/mongodb'; | ||
| import { ServerTestModule } from '@modules/server'; | ||
| import { INestApplication } from '@nestjs/common'; | ||
| import { Test, TestingModule } from '@nestjs/testing'; | ||
| import { cleanupCollections } from '@testing/cleanup-collections'; | ||
| import { TestApiClient } from '@testing/test-api-client'; | ||
| import { AccountEntity } from '@modules/account/repo'; | ||
| import { UserAndAccountTestFactory } from '@testing/factory/user-and-account.test.factory'; | ||
| import { DeletionRequestParams } from '../dto'; | ||
|
|
||
| const baseRouteName = '/deletionRequestsPublic'; | ||
|
|
||
| describe(`deletionRequest public create (api)`, () => { | ||
| let app: INestApplication; | ||
| let em: EntityManager; | ||
| let testApiClient: TestApiClient; | ||
|
|
||
| beforeAll(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| imports: [ServerTestModule], | ||
| }).compile(); | ||
|
|
||
| app = module.createNestApplication(); | ||
| await app.init(); | ||
| em = module.get(EntityManager); | ||
| testApiClient = new TestApiClient(app, baseRouteName); | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| await cleanupCollections(em); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await app.close(); | ||
| }); | ||
|
|
||
| describe('createDeletionRequestPublic', () => { | ||
| const setup = async () => { | ||
| const { studentUser, studentAccount } = UserAndAccountTestFactory.buildStudent(); | ||
|
|
||
| const { adminAccount, adminUser } = UserAndAccountTestFactory.buildAdmin({ school: studentUser.school }); | ||
|
|
||
| await em.persist([studentUser, studentAccount, adminAccount, adminUser]).flush(); | ||
| em.clear(); | ||
|
|
||
| const deletionRequestToCreate: DeletionRequestParams = { | ||
| ids: [studentUser.id], | ||
| }; | ||
| const queryString = deletionRequestToCreate.ids.map((id) => `ids[]=${id}`).join('&'); | ||
| const nonexistentId = new ObjectId().toString(); | ||
| const loggedInClient = await testApiClient.login(adminAccount); | ||
|
|
||
| return { | ||
| nonexistentId, | ||
| studentAccount, | ||
| studentUser, | ||
| queryString, | ||
| loggedInClient, | ||
| }; | ||
| }; | ||
|
|
||
| it('should return status 204', async () => { | ||
| const { queryString, loggedInClient } = await setup(); | ||
|
|
||
| const response = await loggedInClient.delete(`?${queryString}`); | ||
|
|
||
| expect(response.status).toEqual(204); | ||
| }); | ||
|
|
||
| it('should actually create a deletion request', async () => { | ||
| const { studentUser, queryString, loggedInClient } = await setup(); | ||
|
|
||
| await loggedInClient.delete(`?${queryString}`); | ||
|
|
||
| const deletionRequest = await em.findOne('DeletionRequestEntity', { targetRefId: new ObjectId(studentUser.id) }); | ||
| expect(deletionRequest).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should deactivate the user account', async () => { | ||
| const { studentUser, queryString, loggedInClient } = await setup(); | ||
|
|
||
| await loggedInClient.delete(`?${queryString}`); | ||
|
|
||
| const account = await em.findOne(AccountEntity, { userId: new ObjectId(studentUser.id) }); | ||
| expect(account?.deactivatedAt).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should not fail if the target user has no account', async () => { | ||
| const { studentUser, loggedInClient } = await setup(); | ||
| const { studentUser: studentUser2 } = UserAndAccountTestFactory.buildStudent({ school: studentUser.school }); | ||
|
|
||
| await em.persist([studentUser2]).flush(); | ||
| em.clear(); | ||
|
|
||
| const response = await loggedInClient.delete(`?ids[]=${studentUser2.id}`); | ||
|
|
||
| expect(response.status).toEqual(204); | ||
| const deletionRequest = await em.findOne('DeletionRequestEntity', { targetRefId: new ObjectId(studentUser2.id) }); | ||
| expect(deletionRequest).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should return status 400 when all deletion requests fail', async () => { | ||
| const { loggedInClient, nonexistentId } = await setup(); | ||
|
|
||
| const response = await loggedInClient.delete(`?ids[]=${nonexistentId}`); | ||
|
|
||
| expect(response.status).toEqual(400); | ||
| }); | ||
|
|
||
| it('should return status 207 when some deletion requests fail', async () => { | ||
| const { nonexistentId, queryString, loggedInClient } = await setup(); | ||
|
|
||
| const response = await loggedInClient.delete(`?${queryString}&ids[]=${nonexistentId}`); | ||
|
|
||
| expect(response.status).toEqual(207); | ||
| }); | ||
|
|
||
| it('should not allow more than 100 deletion requests to be created', async () => { | ||
| const { studentUser, loggedInClient, queryString } = await setup(); | ||
|
|
||
| let additionalQueryString = ''; | ||
| for (let i = 0; i <= 100; i++) { | ||
| additionalQueryString += `&ids[]=${new ObjectId().toString()}`; | ||
| } | ||
|
|
||
| const response = await loggedInClient.delete(`?${queryString}${additionalQueryString}`); | ||
|
|
||
| expect(response.status).toEqual(400); | ||
| const deletionRequest = await em.findOne('DeletionRequestEntity', { targetRefId: new ObjectId(studentUser.id) }); | ||
| expect(deletionRequest).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('when user is from a different school', () => { | ||
| const setup = async () => { | ||
| const { studentUser, studentAccount } = UserAndAccountTestFactory.buildStudent(); | ||
|
|
||
| const { adminAccount, adminUser } = UserAndAccountTestFactory.buildAdmin(); | ||
|
|
||
| await em.persist([studentUser, studentAccount, adminAccount, adminUser]).flush(); | ||
| em.clear(); | ||
|
|
||
| const deletionRequestToCreate: DeletionRequestParams = { | ||
| ids: [studentUser.id], | ||
| }; | ||
| const queryString = deletionRequestToCreate.ids.map((id) => `ids[]=${id}`).join('&'); | ||
| const nonexistentId = new ObjectId().toString(); | ||
| const loggedInClient = await testApiClient.login(adminAccount); | ||
|
|
||
| return { | ||
| nonexistentId, | ||
| studentUser, | ||
| queryString, | ||
| loggedInClient, | ||
| }; | ||
| }; | ||
|
|
||
| it('should return status 400', async () => { | ||
| const { queryString, loggedInClient } = await setup(); | ||
|
|
||
| const response = await loggedInClient.delete(`?${queryString}`); | ||
| expect(response.status).toEqual(400); | ||
| }); | ||
|
|
||
| it('should not create a deletion request', async () => { | ||
| const { studentUser, queryString, loggedInClient } = await setup(); | ||
|
|
||
| await loggedInClient.delete(`?${queryString}`); | ||
|
|
||
| const deletionRequest = await em.findOne('DeletionRequestEntity', { targetRefId: new ObjectId(studentUser.id) }); | ||
| expect(deletionRequest).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('when user has invalid role', () => { | ||
| const setup = async () => { | ||
| const { adminUser, adminAccount } = UserAndAccountTestFactory.buildAdmin(); | ||
| const { adminUser: targetUser, adminAccount: targetAccount } = UserAndAccountTestFactory.buildAdmin(); | ||
|
|
||
| await em.persist([adminUser, adminAccount, targetUser, targetAccount]).flush(); | ||
| em.clear(); | ||
|
|
||
| const deletionRequestToCreate: DeletionRequestParams = { | ||
| ids: [targetUser.id], | ||
| }; | ||
| const queryString = deletionRequestToCreate.ids.map((id) => `ids[]=${id}`).join('&'); | ||
| const loggedInClient = await testApiClient.login(adminAccount); | ||
|
|
||
| return { | ||
| targetUser, | ||
| queryString, | ||
| loggedInClient, | ||
| }; | ||
| }; | ||
|
|
||
| it('should return status 400', async () => { | ||
| const { queryString, loggedInClient } = await setup(); | ||
|
|
||
| const response = await loggedInClient.delete(`?${queryString}`); | ||
|
|
||
| expect(response.status).toEqual(400); | ||
| }); | ||
|
|
||
| it('should not create a deletion request', async () => { | ||
| const { targetUser, queryString, loggedInClient } = await setup(); | ||
|
|
||
| await loggedInClient.delete(`?${queryString}`); | ||
|
|
||
| const deletionRequest = await em.findOne('DeletionRequestEntity', { targetRefId: new ObjectId(targetUser.id) }); | ||
| expect(deletionRequest).toBeNull(); | ||
| }); | ||
| }); | ||
| }); |
48 changes: 48 additions & 0 deletions
48
apps/server/src/modules/deletion/api/controller/deletion-request-public.controller.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { CurrentUser, ICurrentUser, JwtAuthentication } from '@infra/auth-guard'; | ||
| import { BadRequestException, Controller, Delete, HttpCode, HttpException, Query } from '@nestjs/common'; | ||
| import { ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger'; | ||
| import { DeletionRequestPublicUc } from '../uc'; | ||
| import { DeletionRequestParams } from './dto'; | ||
|
|
||
| @ApiTags('DeletionRequest') | ||
| @JwtAuthentication() | ||
| @Controller('deletionRequestsPublic') | ||
| export class DeletionRequestPublicController { | ||
| constructor(private readonly deletionRequestPublicUc: DeletionRequestPublicUc) {} | ||
|
|
||
| @Delete('') | ||
| @HttpCode(204) | ||
| @ApiOperation({ | ||
| summary: '"Queueing" a deletion request', | ||
| }) | ||
| @ApiResponse({ | ||
| status: 201, | ||
| description: 'Users deletion requests created successfully', | ||
| }) | ||
| @ApiResponse({ | ||
| status: 207, | ||
| description: 'Partial success - Some deletion requests could not be processed', | ||
| }) | ||
| @ApiResponse({ | ||
| status: 400, | ||
| description: 'Bad Request - All deletion requests failed', | ||
| }) | ||
| public async createDeletionRequestPublic( | ||
| @CurrentUser() currentUser: ICurrentUser, | ||
| @Query() params: DeletionRequestParams | ||
| ): Promise<void> { | ||
| const errors = await this.deletionRequestPublicUc.createUserListDeletionRequest(currentUser, params.ids); | ||
|
|
||
| if (errors.length === params.ids.length) { | ||
| throw new BadRequestException(); | ||
| } else if (errors.length > 0) { | ||
| throw new HttpException( | ||
| { | ||
| status: 207, | ||
| message: 'Partial success: Some deletion requests could not be processed.', | ||
| }, | ||
| 207 | ||
| ); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
14 changes: 14 additions & 0 deletions
14
apps/server/src/modules/deletion/api/controller/dto/request/deletion-request.params.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { ApiProperty } from '@nestjs/swagger'; | ||
| import { IsArray, ArrayMaxSize, ArrayMinSize, IsMongoId } from 'class-validator'; | ||
|
|
||
| export class DeletionRequestParams { | ||
| @IsArray() | ||
| @ArrayMinSize(1) | ||
| @ArrayMaxSize(100) | ||
| @IsMongoId({ each: true }) | ||
| @ApiProperty({ | ||
| description: 'The IDs of the users to be deleted', | ||
| required: true, | ||
| }) | ||
| public ids!: string[]; | ||
| } |
88 changes: 88 additions & 0 deletions
88
apps/server/src/modules/deletion/api/uc/deletion-request-public.uc.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| import { LegacyLogger } from '@core/logger'; | ||
| import { ForbiddenException, Injectable } from '@nestjs/common'; | ||
| import { ICurrentUser } from '@infra/auth-guard'; | ||
| import { AccountService } from '@modules/account'; | ||
| import { AuthorizationService } from '@modules/authorization'; | ||
| import { UserService } from '@modules/user'; | ||
| import { Permission } from '@shared/domain/interface'; | ||
| import { EntityId } from '@shared/domain/types'; | ||
| import { DeletionRequestService } from '../../domain/service'; | ||
| import { DeletionRequestResponse } from '../controller/dto'; | ||
| import { DomainName } from '../../domain/types'; | ||
| import { RoleName } from '@modules/role'; | ||
|
|
||
| @Injectable() | ||
| export class DeletionRequestPublicUc { | ||
| constructor( | ||
| private readonly deletionRequestService: DeletionRequestService, | ||
| private readonly logger: LegacyLogger, | ||
| private readonly accountService: AccountService, | ||
| private readonly userService: UserService, | ||
| private readonly authService: AuthorizationService | ||
| ) { | ||
| this.logger.setContext(DeletionRequestPublicUc.name); | ||
| } | ||
|
|
||
| public async createUserListDeletionRequest(currentUser: ICurrentUser, ids: EntityId[]): Promise<Error[]> { | ||
| this.logger.debug({ action: 'createDeletionRequestAsUser', ids, userId: currentUser.userId }); | ||
|
|
||
| const user = await this.authService.getUserWithPermissions(currentUser.userId); | ||
| this.authService.checkAllPermissions(user, [Permission.STUDENT_DELETE, Permission.TEACHER_DELETE]); | ||
|
|
||
| const deleteAfter = new Date(); | ||
|
|
||
| const results = await Promise.allSettled( | ||
| ids.map((targetUserId) => this.createSingleUserDeletionRequest(targetUserId, deleteAfter, currentUser.schoolId)) | ||
| ); | ||
|
|
||
| const errors: Error[] = results | ||
| .filter((result): result is PromiseRejectedResult => result.status === 'rejected') | ||
| .map((result, index) => { | ||
| const error = result.reason as Error; | ||
| this.logger.error({ | ||
| action: 'createSingleUserDeletionRequest', | ||
| userId: currentUser.userId, | ||
| targetUserId: ids[index], | ||
| error: error.message, | ||
| }); | ||
| return error; | ||
| }); | ||
|
|
||
| return errors; | ||
| } | ||
|
|
||
| private async createSingleUserDeletionRequest( | ||
| targetRefId: EntityId, | ||
| deleteAfter: Date, | ||
| schoolId: EntityId | ||
| ): Promise<DeletionRequestResponse> { | ||
| const targetUser = await this.userService.findById(targetRefId); | ||
|
|
||
| if (targetUser.roles.every((role) => role.name !== RoleName.STUDENT && role.name !== RoleName.TEACHER)) { | ||
| throw new ForbiddenException('Cannot request deletion for user with invalid role'); | ||
| } | ||
|
|
||
| if (targetUser.schoolId !== schoolId) { | ||
| throw new ForbiddenException('Cannot request deletion for user from different school'); | ||
| } | ||
|
|
||
| const result: DeletionRequestResponse = await this.deletionRequestService.createDeletionRequest( | ||
| targetRefId, | ||
| DomainName.USER, | ||
| deleteAfter | ||
| ); | ||
|
|
||
| try { | ||
| await this.accountService.deactivateAccount(targetRefId, new Date()); | ||
| } catch (error) { | ||
| // it can be the user has no account (either deleted or never finished the registration process) | ||
| this.logger.error({ | ||
| action: 'deactivateAccount', | ||
| userId: targetRefId, | ||
| error: (error as Error).message, | ||
| }); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| export * from './deletion-request.uc'; | ||
| export * from './deletion-request-public.uc'; |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why implement passing of ids over query string? Wouldn't it be better to use a JSON request body? And is
DELETEthe right HTTP method? Effectively we're creating a couple of deletion requests (that eventually lead to user deletion). I would preferPOST+ JSON body here. The function namecreateDeletionRequestPublicwould also suggestPOST.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initially I had it like this. But then I changed it, to match the existing implementation from FE.
the FE does not want to create deletion requests, but wants to simply delete users. It does not know nor care about how it's done.
next feature should be to prevent users to show up in the table, so essentially like before