Skip to content
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

Feature/permanent delete person #579

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
321d13d
FEAT: created permanent delete methods in account.service.ts & auth.s…
PetarDimitrov91 Nov 14, 2023
3027d0c
CHORE: DB entities
PetarDimitrov91 Nov 14, 2023
1c3ae8c
Merge branch 'master' into feature/permanent-delete-person
PetarDimitrov91 Nov 14, 2023
2c48164
FEAT: refactor permanentDeleteUser. Add exception handling.
PetarDimitrov91 Nov 14, 2023
4649ce3
FEAT: change permanentDeleteUser in auth.service.ts logic so when the…
PetarDimitrov91 Nov 16, 2023
2a94f5d
FEAT: improved the permanentDeleteUser promise chain and write some t…
PetarDimitrov91 Nov 16, 2023
9b63bef
FEAT: added safeguard to ensure that the deleted person is not corpor…
PetarDimitrov91 Nov 20, 2023
c9a866c
FEAT: fix if to check the array length and adjust tests.
PetarDimitrov91 Nov 20, 2023
7ca574d
FIX: wrong .env commit
PetarDimitrov91 Nov 21, 2023
3e5848c
FIX: wrong .env commit #2
PetarDimitrov91 Nov 21, 2023
68fb84b
CHORE: reformat.
PetarDimitrov91 Nov 21, 2023
150104c
Run yarn format
sashko9807 Nov 21, 2023
101c401
Merge pull request #1 from sashko9807/feature/permanent-delete-person
PetarDimitrov91 Nov 21, 2023
3b443fb
FIX: check for corporate profile to use companyId.
PetarDimitrov91 Nov 21, 2023
3d9782b
FIX: return organizer in the if statement & adjust tests
PetarDimitrov91 Nov 21, 2023
53b89df
CHORE: update comment
PetarDimitrov91 Nov 21, 2023
4f9b604
Merge remote-tracking branch 'upstream/master' into feature/permanent…
PetarDimitrov91 Nov 21, 2023
562ed30
FIX: made if in deleteUser() more readable
PetarDimitrov91 Nov 22, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions apps/api/src/account/account.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ export class AccountController {
}

@Delete('me')
async disableUser(@AuthenticatedUser() user: KeycloakTokenParsed) {
async deleteUser(@AuthenticatedUser() user: KeycloakTokenParsed) {
try {
return await this.accountService.disableUser(user)
return await this.accountService.deleteUser(user)
} catch (err) {
Logger.error(`Failed to disable user with keycloakId ${user.sub}. Error is: ${err}`)
Logger.error(`Failed to delete user with keycloakId ${user.sub}. Error is: ${err}`)
throw err
}
}
Expand Down
4 changes: 4 additions & 0 deletions apps/api/src/account/account.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ export class AccountService {
return await this.authService.changeEnabledStatus(user.sub, false)
}

async deleteUser(user: KeycloakTokenParsed) {
return await this.authService.deleteUser(user.sub)
}

async changeProfileActivationStatus(keycloakId: string, newStatus: boolean) {
return await this.authService.changeEnabledStatus(keycloakId, newStatus)
}
Expand Down
158 changes: 156 additions & 2 deletions apps/api/src/auth/auth.service.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { Person } from '@prisma/client'
import { Beneficiary, Person } from '@prisma/client'
import { mockDeep } from 'jest-mock-extended'
import { ConfigService } from '@nestjs/config'
import { HttpService } from '@nestjs/axios'
import { plainToClass } from 'class-transformer'
import { Test, TestingModule } from '@nestjs/testing'
import { Logger, UnauthorizedException } from '@nestjs/common'
import { InternalServerErrorException, Logger, UnauthorizedException } from '@nestjs/common'
import KeycloakConnect, { Grant } from 'keycloak-connect'
import { KEYCLOAK_INSTANCE } from 'nest-keycloak-connect'
import KeycloakAdminClient from '@keycloak/keycloak-admin-client'
Expand All @@ -23,6 +23,7 @@
import { SendGridNotificationsProvider } from '../notifications/providers/notifications.sendgrid.provider'
import { NotificationsProviderInterface } from '../notifications/providers/notifications.interface.providers'
import { MarketingNotificationsModule } from '../notifications/notifications.module'
import { PersonService } from '../person/person.service'

jest.mock('@keycloak/keycloak-admin-client')

Expand All @@ -32,6 +33,7 @@
let admin: KeycloakAdminClient
let keycloak: KeycloakConnect.Keycloak
let marketing: NotificationsProviderInterface<unknown>
let personService: PersonService

const person: Person = {
id: 'e43348aa-be33-4c12-80bf-2adfbf8736cd',
Expand Down Expand Up @@ -116,6 +118,7 @@
admin = module.get<KeycloakAdminClient>(KeycloakAdminClient)
marketing = module.get<NotificationsProviderInterface<never>>(NotificationsProviderInterface)
keycloak = module.get<KeycloakConnect.Keycloak>(KEYCLOAK_INSTANCE)
personService = module.get<PersonService>(PersonService)
})

it('should be defined', () => {
Expand Down Expand Up @@ -501,4 +504,155 @@
expect(marketingSpy).not.toHaveBeenCalled()
})
})

describe('deleteUser', () => {
const corporatePerson: any = {

Check warning on line 509 in apps/api/src/auth/auth.service.spec.ts

View workflow job for this annotation

GitHub Actions / Run API tests

Unexpected any. Specify a different type
id: 'e43348aa-be33-4c12-80bf-2adfbf8736cd',
firstName: 'Admin',
lastName: 'Dev',
companyId: null,
keycloakId: '123',
email: '[email protected]',
emailConfirmed: false,
phone: null,
picture: null,
createdAt: new Date('2021-10-07T13:38:11.097Z'),
updatedAt: new Date('2021-10-07T13:38:11.097Z'),
newsletter: false,
address: null,
birthday: null,
personalNumber: null,
stripeCustomerId: null,
profileEnabled: false,
beneficiaries: [],
organizer: null,
}

it('should delete user successfully', async () => {
const keycloakId = '123'

const personSpy = jest
.spyOn(personService, 'findOneByKeycloakId')
.mockResolvedValue(corporatePerson)

const authenticateAdminSpy = jest
.spyOn(service as any, 'authenticateAdmin')

Check warning on line 539 in apps/api/src/auth/auth.service.spec.ts

View workflow job for this annotation

GitHub Actions / Run API tests

Unexpected any. Specify a different type
.mockResolvedValueOnce('')

const adminDeleteSpy = jest.spyOn(admin.users, 'del').mockResolvedValueOnce()
const prismaDeleteSpy = jest.spyOn(prismaMock.person, 'delete').mockResolvedValueOnce(person)
const loggerLogSpy = jest.spyOn(Logger, 'log')

await expect(service.deleteUser(keycloakId)).resolves.not.toThrow()

expect(personSpy).toHaveBeenCalledOnce()
expect(authenticateAdminSpy).toHaveBeenCalledTimes(1)
expect(adminDeleteSpy).toHaveBeenCalledWith({ id: keycloakId })
expect(prismaDeleteSpy).toHaveBeenCalledWith({ where: { keycloakId } })
expect(loggerLogSpy).toHaveBeenCalledWith(
`User with keycloak id ${keycloakId} was successfully deleted!`,
)
})

it('should handle admin client rejection', async () => {
const keycloakId = '123'

const personSpy = jest
.spyOn(personService, 'findOneByKeycloakId')
.mockResolvedValue(corporatePerson)

const authenticateAdminSpy = jest
.spyOn(service as any, 'authenticateAdmin')

Check warning on line 565 in apps/api/src/auth/auth.service.spec.ts

View workflow job for this annotation

GitHub Actions / Run API tests

Unexpected any. Specify a different type
.mockResolvedValueOnce('')

const adminDeleteSpy = jest
.spyOn(admin.users, 'del')
.mockRejectedValueOnce(new Error('Admin Client Rejection!'))

const loggerLogSpy = jest.spyOn(Logger, 'error')

await expect(service.deleteUser(keycloakId)).rejects.toThrow(InternalServerErrorException)

expect(personSpy).toHaveBeenCalledOnce()
expect(authenticateAdminSpy).toHaveBeenCalledTimes(1)
expect(adminDeleteSpy).toHaveBeenCalledWith({ id: keycloakId })
expect(loggerLogSpy).toHaveBeenCalledWith(
`Deleting user fails with reason: Admin Client Rejection!`,
)
})

it('should handle Prisma rejection', async () => {
const keycloakId = '123'
const personSpy = jest
.spyOn(personService, 'findOneByKeycloakId')
.mockResolvedValue(corporatePerson)

const authenticateAdminSpy = jest
.spyOn(service as any, 'authenticateAdmin')

Check warning on line 591 in apps/api/src/auth/auth.service.spec.ts

View workflow job for this annotation

GitHub Actions / Run API tests

Unexpected any. Specify a different type
.mockResolvedValueOnce('')

const adminDeleteSpy = jest.spyOn(admin.users, 'del').mockResolvedValueOnce()

const prismaDeleteSpy = jest
.spyOn(prismaMock.person, 'delete')
.mockRejectedValueOnce(new Error('Prisma Rejection!'))

const loggerLogSpy = jest.spyOn(Logger, 'error')

await expect(service.deleteUser(keycloakId)).rejects.toThrow(InternalServerErrorException)

expect(personSpy).toHaveBeenCalledOnce()
expect(authenticateAdminSpy).toHaveBeenCalledTimes(1)
expect(adminDeleteSpy).toHaveBeenCalledWith({ id: keycloakId })
expect(prismaDeleteSpy).toHaveBeenCalledWith({ where: { keycloakId } })
expect(loggerLogSpy).toHaveBeenCalledWith(
`Deleting user fails with reason: Prisma Rejection!`,
)
})

it('should throw when corporate user has beneficiaries', async () => {
corporatePerson.beneficiaries = [{ id: '123' } as Beneficiary]

const personSpy = jest
.spyOn(personService, 'findOneByKeycloakId')
.mockResolvedValue(corporatePerson)

await expect(service.deleteUser('123')).rejects.toThrow(InternalServerErrorException)
expect(personSpy).toHaveBeenCalledOnce()
})

it('should throw when user has company id', async () => {
corporatePerson.companyId = '123'

const personSpy = jest
.spyOn(personService, 'findOneByKeycloakId')
.mockResolvedValue(corporatePerson)

await expect(service.deleteUser('123')).rejects.toThrow(InternalServerErrorException)
expect(personSpy).toHaveBeenCalledOnce()
})

it('should throw when user is organizer', async () => {
corporatePerson.organizer = { id: '123' }
const personSpy = jest
.spyOn(personService, 'findOneByKeycloakId')
.mockResolvedValue(corporatePerson)

await expect(service.deleteUser('123')).rejects.toThrow(InternalServerErrorException)
expect(personSpy).toHaveBeenCalledOnce()
})

it('should throw when corporate user has companyId & beneficiaries & is organizer', async () => {
corporatePerson.companyId = '123'
corporatePerson.beneficiaries = [{ id: '123' } as Beneficiary]
corporatePerson.organizer = { id: '123' }

const personSpy = jest
.spyOn(personService, 'findOneByKeycloakId')
.mockResolvedValue(corporatePerson)

await expect(service.deleteUser('123')).rejects.toThrow(InternalServerErrorException)
expect(personSpy).toHaveBeenCalledOnce()
})
})
})
23 changes: 23 additions & 0 deletions apps/api/src/auth/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { EmailService } from '../email/email.service'
import { ForgottenPasswordMailDto } from '../email/template.interface'
import { NewPasswordDto } from './dto/recovery-password.dto'
import { MarketingNotificationsService } from '../notifications/notifications.service'
import { PersonService } from '../person/person.service'

type ErrorResponse = { error: string; data: unknown }
type KeycloakErrorResponse = { error: string; error_description: string }
Expand Down Expand Up @@ -63,6 +64,7 @@ export class AuthService {
private sendEmail: EmailService,
@Inject(KEYCLOAK_INSTANCE) private keycloak: KeycloakConnect.Keycloak,
private readonly marketingNotificationsService: MarketingNotificationsService,
private readonly personService: PersonService,
) {}

async issueGrant(email: string, password: string): Promise<KeycloakConnect.Grant> {
Expand Down Expand Up @@ -426,4 +428,25 @@ export class AuthService {
)
}
}

async deleteUser(keycloakId: string) {
const user = await this.personService.findOneByKeycloakId(keycloakId)

//Check and throw if user is a beneficiary, organizer or corporate profile
if (!!user && user.beneficiaries.length > 0 || user?.organizer || user?.companyId) {
throw new InternalServerErrorException(
'Cannot delete a beneficiary, organizer or corporate profile',
)
}

return this.authenticateAdmin()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I am fine with this .then format.
But I think so far the common pattern is to use await.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this decision because if, for some reason, the promise from Keycloak does not resolve successfully, I want to interrupt the chain and refrain from removing the user in the API database. I am unsure if this is possible when I use await for every promise.

.then(() => this.admin.users.del({ id: keycloakId }))
.then(() => this.prismaService.person.delete({ where: { keycloakId } }))
.then(() => Logger.log(`User with keycloak id ${keycloakId} was successfully deleted!`))
.catch((err) => {
const errorMessage = `Deleting user fails with reason: ${err.message ?? 'server error!'}`
Logger.error(errorMessage)
throw new InternalServerErrorException(errorMessage)
})
}
}
9 changes: 8 additions & 1 deletion apps/api/src/person/person.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,14 @@ export class PersonService {
}

async findOneByKeycloakId(keycloakId: string) {
return await this.prisma.person.findFirst({ where: { keycloakId }, include: { company: true } })
return await this.prisma.person.findFirst({
where: { keycloakId },
include: {
company: true,
beneficiaries: { select: { id: true } },
organizer: { select: { id: true } },
},
})
}

async update(id: string, updatePersonDto: UpdatePersonDto) {
Expand Down
Loading