Skip to content

Commit

Permalink
Fix a bug with the access to the expenses list of a campaign. (#545)
Browse files Browse the repository at this point in the history
We used to honour only the coordinator, but the organizer is as important.
  • Loading branch information
slavcho authored Sep 5, 2023
1 parent 91b86ba commit 00dfdc0
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 23 deletions.
11 changes: 4 additions & 7 deletions apps/api/src/campaign-file/campaign-file.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ describe('CampaignFileController', () => {
ConfigService,
{
provide: CampaignService,
useValue: { getCampaignByIdAndCoordinatorId: jest.fn(() => null) },
useValue: { verifyCampaignOwner: jest.fn(() => null) },
},
VaultService,
CampaignNewsService,
Expand Down Expand Up @@ -92,7 +92,7 @@ describe('CampaignFileController', () => {
).toEqual([fileId, fileId])

expect(personService.findOneByKeycloakId).toHaveBeenCalledWith(userMock.sub)
expect(campaignService.getCampaignByIdAndCoordinatorId).not.toHaveBeenCalled()
expect(campaignService.verifyCampaignOwner).not.toHaveBeenCalled()
expect(campaignFileService.create).toHaveBeenCalledTimes(2)
})

Expand All @@ -102,16 +102,13 @@ describe('CampaignFileController', () => {
await expect(controller.create(campaignId, { roles: [] }, [], userMock)).rejects.toThrowError()

expect(personService.findOneByKeycloakId).toHaveBeenCalledWith(userMock.sub)
expect(campaignService.getCampaignByIdAndCoordinatorId).not.toHaveBeenCalled()
expect(campaignService.verifyCampaignOwner).not.toHaveBeenCalled()
})

it('should throw an error for user not owning updated campaign', async () => {
await expect(controller.create(campaignId, { roles: [] }, [], userMock)).rejects.toThrowError()

expect(personService.findOneByKeycloakId).toHaveBeenCalledWith(userMock.sub)
expect(campaignService.getCampaignByIdAndCoordinatorId).toHaveBeenCalledWith(
campaignId,
personIdMock,
)
expect(campaignService.verifyCampaignOwner).toHaveBeenCalledWith(campaignId, personIdMock)
})
})
11 changes: 4 additions & 7 deletions apps/api/src/campaign-file/campaign-file.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { FilesRoleDto } from './dto/files-role.dto'
import { CampaignFileService } from './campaign-file.service'
import { CampaignService } from '../campaign/campaign.service'
import { KeycloakTokenParsed, isAdmin } from '../auth/keycloak'
import { ApiTags } from '@nestjs/swagger';
import { ApiTags } from '@nestjs/swagger'
import { CampaignFileRole } from '@prisma/client'

Check warning on line 27 in apps/api/src/campaign-file/campaign-file.controller.ts

View workflow job for this annotation

GitHub Actions / Run API tests

'CampaignFileRole' is defined but never used

Check warning on line 27 in apps/api/src/campaign-file/campaign-file.controller.ts

View workflow job for this annotation

GitHub Actions / Run API tests

'CampaignFileRole' is defined but never used

@ApiTags('campaign-file')
Expand Down Expand Up @@ -51,10 +51,7 @@ export class CampaignFileController {
}

if (!isAdmin(user)) {
const campaign = await this.campaignService.getCampaignByIdAndCoordinatorId(
campaignId,
person.id,
)
const campaign = await this.campaignService.verifyCampaignOwner(campaignId, person.id)
if (!campaign) {
throw new NotFoundException(
'User ' + user.name + 'is not admin or coordinator of campaign with id: ' + campaignId,
Expand Down Expand Up @@ -88,8 +85,8 @@ export class CampaignFileController {
'Content-Type': file.mimetype,
'Content-Disposition': 'attachment; filename="' + file.filename + '"',
'Cache-Control': file.mimetype.startsWith('image/')
? 'public, s-maxage=15552000, stale-while-revalidate=15552000, immutable'
: 'no-store'
? 'public, s-maxage=15552000, stale-while-revalidate=15552000, immutable'
: 'no-store',
})

return new StreamableFile(file.stream)
Expand Down
26 changes: 17 additions & 9 deletions apps/api/src/campaign/campaign.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,15 +235,23 @@ export class CampaignService {
return campaigns
}

async getCampaignByIdAndCoordinatorId(
campaignId: string,
coordinatorId: string,
): Promise<Campaign | null> {
const campaign = await this.prisma.campaign.findFirst({
where: { id: campaignId, coordinator: { personId: coordinatorId } },
include: { coordinator: true },
// Check if the campaign exists by coordinator or organizer
async verifyCampaignOwner(campaignId: string, personId: string): Promise<Campaign | null> {
const campaignByCoordinator = await this.prisma.campaign.findFirst({
where: { id: campaignId, coordinator: { personId } },
include: { coordinator: true, organizer: true },
})
return campaign

if (campaignByCoordinator !== null) {
return campaignByCoordinator
}

const campaignByOrganizer = await this.prisma.campaign.findFirst({
where: { id: campaignId, organizer: { personId } },
include: { coordinator: true, organizer: true },
})

return campaignByOrganizer
}

async getCampaignByIdWithPersonIds(id: string) {
Expand Down Expand Up @@ -1070,7 +1078,7 @@ export class CampaignService {
throw new UnauthorizedException()
}

const campaign = await this.getCampaignByIdAndCoordinatorId(campaignId, person.id)
const campaign = await this.verifyCampaignOwner(campaignId, person.id)
if (!campaign) {
throw new UnauthorizedException()
}
Expand Down

0 comments on commit 00dfdc0

Please sign in to comment.