From c032ef643ae079bcbc2c060a838897204ca29433 Mon Sep 17 00:00:00 2001 From: Aleksandar Petkov Date: Mon, 22 Jan 2024 08:14:02 +0200 Subject: [PATCH] src/campaign-news-file: Allow campaign organizer to upload files (#598) * src/campaign-news-file: Allow campaign organizer to upload files - Changed logic to allow campaign organizer to upload files - Added testcases to cover more cases of file uploading * Clean leftover --- .../campaign-news-file.controller.spec.ts | 189 ++++++++++++++++-- .../campaign-news-file.controller.ts | 30 ++- .../campaign-news-file.module.ts | 14 +- .../campaign-news-file.service.ts | 9 +- .../campaign-news/campaign-news.controller.ts | 11 +- .../campaign-news/campaign-news.service.ts | 15 +- 6 files changed, 211 insertions(+), 57 deletions(-) diff --git a/apps/api/src/campaign-news-file/campaign-news-file.controller.spec.ts b/apps/api/src/campaign-news-file/campaign-news-file.controller.spec.ts index 4542e2dcb..7fe0c99e6 100644 --- a/apps/api/src/campaign-news-file/campaign-news-file.controller.spec.ts +++ b/apps/api/src/campaign-news-file/campaign-news-file.controller.spec.ts @@ -4,47 +4,103 @@ import { CampaignNewsFileController } from './campaign-news-file.controller' import { CampaignNewsFileService } from './campaign-news-file.service' import { S3Service } from '../s3/s3.service' import { PersonService } from '../person/person.service' -import { MockPrismaService } from '../prisma/prisma-client.mock' +import { MockPrismaService, prismaMock } from '../prisma/prisma-client.mock' import { CampaignService } from '../campaign/campaign.service' import { VaultService } from '../vault/vault.service' import { KeycloakTokenParsed } from '../auth/keycloak' import { CampaignNewsService } from '../campaign-news/campaign-news.service' import { NotificationsProviderInterface } from '../notifications/providers/notifications.interface.providers' import { SendGridNotificationsProvider } from '../notifications/providers/notifications.sendgrid.provider' -import { MarketingNotificationsModule } from '../notifications/notifications.module' import { MarketingNotificationsService } from '../notifications/notifications.service' import { EmailService } from '../email/email.service' import { TemplateService } from '../email/template.service' +import { CampaignNewsFile, Prisma } from '@prisma/client' -describe('CampaignFileController', () => { +type PersonWithCompany = Prisma.PersonGetPayload<{ + include: { + company: true + beneficiaries: { select: { id: true } } + organizer: { select: { id: true } } + } +}> + +describe('CampaignNewsFileController', () => { let controller: CampaignNewsFileController let campaignNewsFileService: CampaignNewsFileService + let campaignNewsService: CampaignNewsService let personService: PersonService - const personIdMock = 'testPersonId' + const fileMock: CampaignNewsFile = { + id: 'fileId', + filename: 'filename', + newsId: '123', + role: 'background', + personId: '12', + mimetype: 'image/jpeg', + } + + const personMock: PersonWithCompany | null = { + id: 'e43348aa-be33-4c12-80bf-2adfbf8736cd', + firstName: 'John', + lastName: 'Doe', + keycloakId: 'some-id', + email: 'user@email.com', + emailConfirmed: false, + companyId: null, + phone: null, + picture: null, + createdAt: new Date('2021-10-07T13:38:11.097Z'), + updatedAt: new Date('2021-10-07T13:38:11.097Z'), + newsletter: true, + address: null, + birthday: null, + personalNumber: null, + stripeCustomerId: null, + profileEnabled: true, + company: { + id: '1', + legalPersonName: 'Admin Dev', + companyName: 'company-test', + createdAt: new Date(), + companyNumber: '123', + countryCode: '123', + personId: '12', + cityId: '1', + updatedAt: new Date(), + }, + organizer: { + id: '1', + }, + beneficiaries: [], + } + const fileId = 'fileId' - const articleId = 'testArticleId' + const articleId = 'e43348aa-be33-4c12-80bf-2adfbf8736cd' const userMock = { - sub: 'testKeycloackId', + sub: 'e43348aa-be33-4c12-80bf-2adfbf8736cd', resource_access: { account: { roles: [] } }, 'allowed-origins': [], } as KeycloakTokenParsed + const adminMock = { + ...userMock, + resource_access: { account: { roles: ['account-view-supporters'] } }, + } beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ - controllers: [CampaignNewsFileController, MarketingNotificationsModule], + controllers: [CampaignNewsFileController], providers: [ - { - provide: CampaignNewsFileService, - useValue: { create: jest.fn(() => fileId) }, - }, + CampaignNewsFileService, MockPrismaService, EmailService, TemplateService, - S3Service, + { + provide: S3Service, + useValue: { uploadObject: jest.fn() }, + }, { provide: PersonService, - useValue: { findOneByKeycloakId: jest.fn(() => ({ id: personIdMock })) }, + useValue: { findOneByKeycloakId: jest.fn() }, }, ConfigService, { @@ -65,6 +121,7 @@ describe('CampaignFileController', () => { controller = module.get(CampaignNewsFileController) campaignNewsFileService = module.get(CampaignNewsFileService) + campaignNewsService = module.get(CampaignNewsService) personService = module.get(PersonService) }) @@ -76,34 +133,124 @@ describe('CampaignFileController', () => { expect(controller).toBeDefined() }) - it('should call service for create campaign file for admin user', async () => { + it('should allow admins to upload files', async () => { + const files = [ + { mimetype: 'jpg', originalname: 'testName1', buffer: Buffer.from('') }, + { mimetype: 'jpg', originalname: 'testName2', buffer: Buffer.from('') }, + ] as Express.Multer.File[] + + prismaMock.campaignNews.count.mockResolvedValue(0) + + const personSpy = jest.spyOn(personService, 'findOneByKeycloakId').mockResolvedValue(personMock) + const createSpy = jest.spyOn(campaignNewsFileService, 'create') + const canEditSpy = jest.spyOn(campaignNewsService, 'canEditArticle') + prismaMock.campaignNewsFile.create.mockResolvedValue(fileMock) + expect( + await controller.create(articleId, { roles: ['background'] }, files, { + ...adminMock, + }), + ).toEqual([fileId, fileId]) + + expect(personSpy).toHaveBeenCalledWith(userMock.sub) + expect(canEditSpy).toHaveBeenCalledWith(articleId, adminMock) + expect(await campaignNewsService.canEditArticle(articleId, adminMock)).toEqual(true) + expect(prismaMock.campaignNews.count).toHaveBeenCalledWith({ + where: { + id: articleId, + campaign: { organizer: { person: { keycloakId: userMock.sub } } }, + }, + }) + expect(createSpy).toHaveBeenCalledTimes(2) + }) + + it('should allow campaign organizers to upload file ', async () => { const files = [ { mimetype: 'jpg', originalname: 'testName1', buffer: Buffer.from('') }, { mimetype: 'jpg', originalname: 'testName2', buffer: Buffer.from('') }, ] as Express.Multer.File[] + prismaMock.campaignNewsFile.create.mockResolvedValue(fileMock) + prismaMock.campaignNews.count.mockResolvedValue(1) + + const canEditSpy = jest.spyOn(campaignNewsService, 'canEditArticle') + const personSpy = jest.spyOn(personService, 'findOneByKeycloakId').mockResolvedValue(personMock) + const createSpy = jest.spyOn(campaignNewsFileService, 'create') expect( await controller.create(articleId, { roles: ['background'] }, files, { ...userMock, - ...{ resource_access: { account: { roles: ['account-view-supporters'] } } }, }), ).toEqual([fileId, fileId]) - expect(personService.findOneByKeycloakId).toHaveBeenCalledWith(userMock.sub) - expect(campaignNewsFileService.create).toHaveBeenCalledTimes(2) + expect(personSpy).toHaveBeenCalledWith(userMock.sub) + expect(canEditSpy).toHaveBeenCalledWith(articleId, userMock) + expect(await campaignNewsService.canEditArticle(articleId, adminMock)).toEqual(true) + expect(prismaMock.campaignNews.count).toHaveBeenCalledWith({ + where: { + id: articleId, + campaign: { organizer: { person: { keycloakId: userMock.sub } } }, + }, + }) + expect(createSpy).toHaveBeenCalledTimes(2) + }) + + it('should throw an error if neither admin or organizer tries to upload a file', async () => { + const files = [ + { mimetype: 'jpg', originalname: 'testName1', buffer: Buffer.from('') }, + { mimetype: 'jpg', originalname: 'testName2', buffer: Buffer.from('') }, + ] as Express.Multer.File[] + + prismaMock.campaignNewsFile.create.mockResolvedValue(fileMock) + prismaMock.campaignNews.count.mockResolvedValue(0) + + const canEditSpy = jest.spyOn(campaignNewsService, 'canEditArticle') + const personSpy = jest.spyOn(personService, 'findOneByKeycloakId').mockResolvedValue(personMock) + const createSpy = jest.spyOn(campaignNewsFileService, 'create') + await expect( + controller.create(articleId, { roles: ['background'] }, files, { + ...userMock, + }), + ).rejects.toThrowError() + + expect(personSpy).toHaveBeenCalledWith(userMock.sub) + expect(canEditSpy).toHaveBeenCalledWith(articleId, userMock) + expect(await campaignNewsService.canEditArticle(articleId, userMock)).toEqual(false) + expect(prismaMock.campaignNews.count).toHaveBeenCalledWith({ + where: { + id: articleId, + campaign: { organizer: { person: { keycloakId: userMock.sub } } }, + }, + }) + expect(createSpy).not.toHaveBeenCalled() + }) + + it('should allow campaign organizer to delete files', async () => { + prismaMock.campaignNewsFile.count.mockResolvedValue(1) + const removeSpy = jest.spyOn(campaignNewsFileService, 'remove').mockResolvedValue(fileMock) + expect(await controller.remove(fileId, userMock)).toEqual(fileMock) + expect(removeSpy).toHaveBeenCalledWith(fileId) + }) + + it('should allow admin to delete files', async () => { + prismaMock.campaignNewsFile.count.mockResolvedValue(0) + const removeSpy = jest.spyOn(campaignNewsFileService, 'remove').mockResolvedValue(fileMock) + expect(await controller.remove(fileId, adminMock)).toEqual(fileMock) + expect(removeSpy).toHaveBeenCalledWith(fileId) + }) + + it('should throw an error if delete request is not coming from organizer or admin', async () => { + prismaMock.campaignNewsFile.count.mockResolvedValue(0) + jest.spyOn(campaignNewsFileService, 'remove') + await expect(controller.remove(fileId, userMock)).rejects.toThrowError() + expect(campaignNewsFileService.remove).not.toHaveBeenCalled() }) it('should throw an error for missing person', async () => { jest.spyOn(personService, 'findOneByKeycloakId').mockResolvedValue(null) await expect(controller.create(articleId, { roles: [] }, [], userMock)).rejects.toThrowError() - - expect(personService.findOneByKeycloakId).toHaveBeenCalledWith(userMock.sub) }) it('should throw an error for user not having access', async () => { await expect(controller.create(articleId, { roles: [] }, [], userMock)).rejects.toThrowError() - - expect(personService.findOneByKeycloakId).toHaveBeenCalledWith(userMock.sub) }) }) diff --git a/apps/api/src/campaign-news-file/campaign-news-file.controller.ts b/apps/api/src/campaign-news-file/campaign-news-file.controller.ts index fdbadef58..916fedaba 100644 --- a/apps/api/src/campaign-news-file/campaign-news-file.controller.ts +++ b/apps/api/src/campaign-news-file/campaign-news-file.controller.ts @@ -10,14 +10,10 @@ import { NotFoundException, Logger, Body, - Inject, - forwardRef, ForbiddenException, } from '@nestjs/common' import { FilesInterceptor } from '@nestjs/platform-express' import { UseInterceptors, UploadedFiles } from '@nestjs/common' -import { RoleMatchingMode, Roles } from 'nest-keycloak-connect' -import { RealmViewSupporters, ViewSupporters } from '@podkrepi-bg/podkrepi-types' import { Public, AuthenticatedUser } from 'nest-keycloak-connect' import { PersonService } from '../person/person.service' import { FilesRoleDto } from './dto/files-role.dto' @@ -25,13 +21,15 @@ import { CampaignNewsFileService } from './campaign-news-file.service' import { KeycloakTokenParsed, isAdmin } from '../auth/keycloak' import { ApiTags } from '@nestjs/swagger' import { validateFileType } from '../common/files' +import { CampaignNewsService } from '../campaign-news/campaign-news.service' @ApiTags('campaign-news-file') @Controller('campaign-news-file') export class CampaignNewsFileController { constructor( - private readonly campaignFileService: CampaignNewsFileService, - @Inject(forwardRef(() => PersonService)) private readonly personService: PersonService, + private readonly campaignNewsFileService: CampaignNewsFileService, + private readonly personService: PersonService, + private readonly campaignNewsService: CampaignNewsService, ) {} @Post(':article_id') @@ -51,19 +49,22 @@ export class CampaignNewsFileController { ) { const keycloakId = user.sub const person = await this.personService.findOneByKeycloakId(keycloakId) + if (!person) { Logger.warn('No person record with keycloak ID: ' + keycloakId) throw new NotFoundException('No person record with keycloak ID: ' + keycloakId) } - if (!isAdmin(user)) { + const canEditArticle = await this.campaignNewsService.canEditArticle(articleId, user) + + if (!canEditArticle) { throw new ForbiddenException('User has no access to this operation.') } const filesRole = body.roles return await Promise.all( files.map((file, key) => { - return this.campaignFileService.create( + return this.campaignNewsFileService.create( Array.isArray(filesRole) ? filesRole[key] : filesRole, articleId, file.mimetype, @@ -81,7 +82,7 @@ export class CampaignNewsFileController { @Param('id') id: string, @Response({ passthrough: true }) res, ): Promise { - const file = await this.campaignFileService.findOne(id) + const file = await this.campaignNewsFileService.findOne(id) res.set({ 'Content-Type': file.mimetype, 'Content-Disposition': 'inline; filename="' + file.filename + '"', @@ -91,12 +92,9 @@ export class CampaignNewsFileController { } @Delete(':id') - @Roles({ - roles: [RealmViewSupporters.role, ViewSupporters.role], - mode: RoleMatchingMode.ANY, - }) - remove(@Param('id') id: string) { - console.log(` called`) - return this.campaignFileService.remove(id) + async remove(@Param('id') id: string, @AuthenticatedUser() user: KeycloakTokenParsed) { + const canDelete = await this.campaignNewsFileService.canDeleteNewsFile(id, user) + if (!canDelete) throw new ForbiddenException('User has no access for this operation') + return this.campaignNewsFileService.remove(id) } } diff --git a/apps/api/src/campaign-news-file/campaign-news-file.module.ts b/apps/api/src/campaign-news-file/campaign-news-file.module.ts index 017ac42a8..669cabbab 100644 --- a/apps/api/src/campaign-news-file/campaign-news-file.module.ts +++ b/apps/api/src/campaign-news-file/campaign-news-file.module.ts @@ -1,17 +1,15 @@ import { Module } from '@nestjs/common' import { CampaignNewsFileService } from './campaign-news-file.service' import { CampaignNewsFileController } from './campaign-news-file.controller' -import { PrismaService } from '../prisma/prisma.service' + import { S3Service } from '../s3/s3.service' -import { PersonService } from '../person/person.service' +import { PersonModule } from '../person/person.module' +import { CampaignNewsModule } from '../campaign-news/campaign-news.module' +import { PrismaService } from '../prisma/prisma.service' @Module({ controllers: [CampaignNewsFileController], - providers: [ - CampaignNewsFileService, - PrismaService, - S3Service, - PersonService, - ], + imports: [CampaignNewsModule, PersonModule], + providers: [CampaignNewsFileService, PrismaService, S3Service], }) export class CampaignNewsFileModule {} diff --git a/apps/api/src/campaign-news-file/campaign-news-file.service.ts b/apps/api/src/campaign-news-file/campaign-news-file.service.ts index 9911c3d65..614390a5a 100644 --- a/apps/api/src/campaign-news-file/campaign-news-file.service.ts +++ b/apps/api/src/campaign-news-file/campaign-news-file.service.ts @@ -5,6 +5,7 @@ import { CampaignFile, CampaignFileRole, Person } from '@prisma/client' import { S3Service } from '../s3/s3.service' import { PrismaService } from '../prisma/prisma.service' import { CreateCampaignNewsFileDto } from './dto/create-campaign-news-file.dto' +import { KeycloakTokenParsed, isAdmin } from '../auth/keycloak' @Injectable() export class CampaignNewsFileService { @@ -27,7 +28,6 @@ export class CampaignNewsFileService { personId: person.id, } const dbFile = await this.prisma.campaignNewsFile.create({ data: file }) - // Use the DB primary key as the S3 key. This will make sure it is always unique. await this.s3.uploadObject( this.bucketName, @@ -60,6 +60,13 @@ export class CampaignNewsFileService { } } + async canDeleteNewsFile(id: string, user: KeycloakTokenParsed): Promise { + const isFileOwner = await this.prisma.campaignNewsFile.count({ + where: { id, person: { keycloakId: user.sub } }, + }) + return !!isFileOwner || isAdmin(user) + } + async remove(id: string) { await this.s3.deleteObject(this.bucketName, id) return await this.prisma.campaignNewsFile.delete({ where: { id } }) diff --git a/apps/api/src/campaign-news/campaign-news.controller.ts b/apps/api/src/campaign-news/campaign-news.controller.ts index b377d3287..914b91db1 100644 --- a/apps/api/src/campaign-news/campaign-news.controller.ts +++ b/apps/api/src/campaign-news/campaign-news.controller.ts @@ -94,16 +94,9 @@ export class CampaignNewsController { throw new NotFoundException('Article not found') } - const publisher = await this.personService.findOneByKeycloakId(user.sub) - if (!publisher) { - throw new NotFoundException('Author was not found') - } + const canEditArticle = await this.campaignNewsService.canEditArticle(articleId, user) - if ( - article.state === CampaignNewsState.draft && - publisher.id !== article.publisherId && - !isAdmin(user) - ) { + if (article.state === CampaignNewsState.draft && !canEditArticle) { throw new ForbiddenException('The user has no access to delete this article') } diff --git a/apps/api/src/campaign-news/campaign-news.service.ts b/apps/api/src/campaign-news/campaign-news.service.ts index 1561d86f9..5b8479683 100644 --- a/apps/api/src/campaign-news/campaign-news.service.ts +++ b/apps/api/src/campaign-news/campaign-news.service.ts @@ -10,6 +10,7 @@ import { ConfigService } from '@nestjs/config' import { MarketingNotificationsService } from '../notifications/notifications.service' import { CampaignNewsDraftEmailDto } from '../email/template.interface' import { EmailService } from '../email/email.service' +import { KeycloakTokenParsed, isAdmin } from '../auth/keycloak' @Injectable() export class CampaignNewsService { @@ -130,10 +131,20 @@ export class CampaignNewsService { } async canCreateArticle(campaignId: string, keycloakId: string) { - const canEdit = await this.prisma.campaign.findFirst({ + const canCreate = await this.prisma.campaign.findFirst({ where: { id: campaignId, organizer: { person: { keycloakId } } }, }) - return !!canEdit + return !!canCreate + } + + async canEditArticle(articleId: string, user: KeycloakTokenParsed) { + const canEdit = await this.prisma.campaignNews.count({ + where: { + id: articleId, + campaign: { organizer: { person: { keycloakId: user.sub } } }, + }, + }) + return !!canEdit || isAdmin(user) } async listPublishedNewsWithPagination(currentPage: number) {