From c958d85996cda4c281c4b0b47f109d09616b62dd Mon Sep 17 00:00:00 2001 From: Alexander Twrdik <6052859+DiCanio@users.noreply.github.com> Date: Mon, 25 Mar 2024 19:00:05 +0100 Subject: [PATCH] feat: revise message subscription endpoint Adjusts the endpoint used for message subscriptions. Bases it off of an analysis which moves the analysis identifier from the request body to the actual URL. This has been done to put an emphasis on the fact that subscriptions for messages are grouped by an analysis identifier. Also adds existence checks before adding a message subscription. When an analysis does not exist on the central side (hub) then the client must not be able to add a message subscription. This is done because there won't be any incoming messages if for an analysis if it does not exist in the first place. --- .../subscription/subscription.controller.ts | 66 +++++++++++----- .../subscription/subscription.module.ts | 29 ++++++- .../subscription/subscription.e2e.spec.ts | 75 +++++++++++++++---- 3 files changed, 136 insertions(+), 34 deletions(-) diff --git a/src/server/message/subscription/subscription.controller.ts b/src/server/message/subscription/subscription.controller.ts index c760967..7a65fda 100644 --- a/src/server/message/subscription/subscription.controller.ts +++ b/src/server/message/subscription/subscription.controller.ts @@ -1,10 +1,12 @@ /* eslint-disable max-classes-per-file */ // Disabled to allow keeping validation classes close to where they are used! import { - Body, Controller, Get, HttpCode, Param, Post, Req, Res, UseGuards, + BadGatewayException, + Body, Controller, Get, HttpCode, HttpException, InternalServerErrorException, NotFoundException, Param, Post, Req, Res, UseGuards, } from '@nestjs/common'; -import { IsNotEmpty, IsString, IsUrl } from 'class-validator'; +import { IsUrl } from 'class-validator'; import { Request, Response } from 'express'; +import { APIClient } from '@privateaim/core'; import type { SubscriptionDto } from './subscription.service'; import { MessageSubscriptionService } from './subscription.service'; import { AuthGuard } from '../../auth/auth.guard'; @@ -14,39 +16,67 @@ import { AuthGuard } from '../../auth/auth.guard'; * Makes use of auto-validation using `class-validator`. */ class AddSubscriptionRequestBody { - @IsString() - @IsNotEmpty() - analysisId: string; - @IsUrl({ require_tld: false }) webhookUrl: URL; // TODO: might need some auth information as well (left out for brevity at the moment) } +// TODO: this somehow needs to be handled by the API Client -> maybe introduce a wrapper later on +function handleHubApiError(err: any, analysisId: string) { + if (err.statusCode !== undefined) { + if ((err.statusCode as number) === 404) { + throw new NotFoundException(`analysis '${analysisId}' does not exist`); + } + + if ((err.statusCode as number) >= 500) { + throw new BadGatewayException(`cannot check existence of analysis '${analysisId}'`, { + cause: err, + description: 'unrecoverable error when requesting central side (hub)', + }); + } + } else { + throw new InternalServerErrorException(`cannot check existence of analysis '${analysisId}'`, { cause: err }); + } +} + /** * Bundles API endpoints related to message subscriptions. */ -@Controller('messages') +@Controller('analyses/:id/messages') @UseGuards(AuthGuard) export class MessageSubscriptionController { private readonly subscriptionService: MessageSubscriptionService; - constructor(subscriptionService: MessageSubscriptionService) { + private readonly hubApiClient: APIClient; + + constructor(subscriptionService: MessageSubscriptionService, hubApiClient: APIClient) { this.subscriptionService = subscriptionService; + this.hubApiClient = hubApiClient; } @Post('subscriptions') @HttpCode(201) - async subscribe(@Body() data: AddSubscriptionRequestBody, @Req() req: Request, @Res() res: Response) { - const { id } = await this.subscriptionService.addSubscription({ - analysisId: data.analysisId, - webhookUrl: data.webhookUrl, - }); - const subscriptionResourceLocation = `${req.protocol}://${req.get('Host')}${req.originalUrl}/${id}`; - res.header('Location', subscriptionResourceLocation); - res.json({ - subscriptionId: id, - }); + async subscribe(@Param('id') analysisId: string, @Body() data: AddSubscriptionRequestBody, @Req() req: Request, @Res() res: Response) { + return this.hubApiClient.analysis.getOne(analysisId) + .catch((err) => handleHubApiError(err, analysisId)) + .then(() => this.subscriptionService.addSubscription({ + analysisId, + webhookUrl: data.webhookUrl, + })) + .catch((err) => { + if (err instanceof HttpException) { + throw err; + } else { + throw new InternalServerErrorException(`cannot save subscription for analysis '${analysisId}'`, { cause: err }); + } + }) + .then(({ id }) => { + const subscriptionResourceLocation = `${req.protocol}://${req.get('Host')}${req.originalUrl}/${id}`; + res.header('Location', subscriptionResourceLocation); + res.json({ + subscriptionId: id, + }); + }); } @Get('subscriptions/:id') diff --git a/src/server/message/subscription/subscription.module.ts b/src/server/message/subscription/subscription.module.ts index 1150b19..aba7bcb 100644 --- a/src/server/message/subscription/subscription.module.ts +++ b/src/server/message/subscription/subscription.module.ts @@ -1,5 +1,9 @@ import { Module } from '@nestjs/common'; import { MongooseModule } from '@nestjs/mongoose'; +import { APIClient } from '@privateaim/core'; +import { ConfigService } from '@nestjs/config'; +import type { ClientResponseErrorTokenHookOptions } from '@authup/core'; +import { mountClientResponseErrorTokenHook } from '@authup/core'; import { SubscriptionSchema } from './persistence/subscription.schema'; import { MessageSubscriptionService } from './subscription.service'; import { MessageSubscriptionController } from './subscription.controller'; @@ -10,7 +14,30 @@ import { MessageSubscriptionController } from './subscription.controller'; @Module({ imports: [MongooseModule.forFeature([{ name: 'subscription', schema: SubscriptionSchema }])], controllers: [MessageSubscriptionController], - providers: [MessageSubscriptionService], + providers: [ + { + provide: APIClient, + useFactory: async (configService: ConfigService) => { + const hookOptions: ClientResponseErrorTokenHookOptions = { + baseURL: configService.getOrThrow('hub.auth.baseUrl'), + tokenCreator: { + type: 'robot', + id: configService.getOrThrow('hub.auth.robotId'), + secret: configService.getOrThrow('hub.auth.robotSecret'), + }, + }; + + const hubClient = new APIClient({ + baseURL: configService.get('hub.baseUrl'), + }); + mountClientResponseErrorTokenHook(hubClient, hookOptions); + + return hubClient; + }, + inject: [ConfigService], + }, + MessageSubscriptionService, + ], exports: [MessageSubscriptionService], }) export class MessageSubscriptionModule { } diff --git a/test/unit/server/message/subscription/subscription.e2e.spec.ts b/test/unit/server/message/subscription/subscription.e2e.spec.ts index 50bf3de..7525155 100644 --- a/test/unit/server/message/subscription/subscription.e2e.spec.ts +++ b/test/unit/server/message/subscription/subscription.e2e.spec.ts @@ -1,11 +1,14 @@ +/* eslint-disable prefer-promise-reject-errors */ import request from 'supertest'; import type { INestApplication } from '@nestjs/common'; -import { ValidationPipe } from '@nestjs/common'; +import { HttpStatus, ValidationPipe } from '@nestjs/common'; import { Test } from '@nestjs/testing'; import { MongooseModule } from '@nestjs/mongoose'; import type { StartedTestContainer } from 'testcontainers'; import { GenericContainer, Wait } from 'testcontainers'; import { APP_PIPE } from '@nestjs/core'; +import type { Analysis } from '@privateaim/core'; +import { APIClient } from '@privateaim/core'; import { MessageSubscriptionModule } from '../../../../../src/server/message/subscription/subscription.module'; import { AuthGuard } from '../../../../../src/server/auth/auth.guard'; @@ -14,6 +17,7 @@ const MONGO_DB_TEST_DB_NAME: string = 'message-broker-subscriptions-test'; describe('Message Subscription Module', () => { let mongodbEnv: StartedTestContainer; let app: INestApplication; + let hubApiClient: APIClient; beforeAll(async () => { mongodbEnv = await new GenericContainer('mongo:7.0.5@sha256:fcde2d71bf00b592c9cabab1d7d01defde37d69b3d788c53c3bc7431b6b15de8') @@ -26,6 +30,8 @@ describe('Message Subscription Module', () => { const mongoDbConnString = `mongodb://${mongodbEnv.getHost()}:${mongodbEnv.getFirstMappedPort()}/`; + hubApiClient = new APIClient({}); + const moduleRef = await Test.createTestingModule({ imports: [ MongooseModule.forRootAsync({ @@ -45,25 +51,33 @@ describe('Message Subscription Module', () => { }) .overrideGuard(AuthGuard) .useValue({ canActivate: () => true }) + .overrideProvider(APIClient) + .useValue(hubApiClient) .compile(); app = moduleRef.createNestApplication(); await app.init(); }, 300000); // timeout takes into account that this image might have to be pulled first + beforeEach(() => { + jest.clearAllMocks(); + }); + afterAll(async () => { await app.close(); await mongodbEnv.stop(); }); it('/POST subscriptions should persist a new subscription', async () => { - const testAnalysisId = 'foo'; + const testAnalysisId = 'd985ddb4-e0af-407f-afd0-6d002813d29c'; const testWebhookUrl = 'http://localhost/bar'; + jest.spyOn(hubApiClient.analysis, 'getOne').mockImplementation(() => Promise.resolve(jest.fn() as unknown as Analysis)); + const subscriptionId = await request(app.getHttpServer()) - .post('/messages/subscriptions') - .send({ analysisId: testAnalysisId, webhookUrl: testWebhookUrl }) - .expect(201) + .post(`/analyses/${testAnalysisId}/messages/subscriptions`) + .send({ webhookUrl: testWebhookUrl }) + .expect(HttpStatus.CREATED) .then((res) => { const { location } = res.header; const { subscriptionId } = res.body; @@ -73,8 +87,8 @@ describe('Message Subscription Module', () => { }); await request(app.getHttpServer()) - .get(`/messages/subscriptions/${subscriptionId}`) - .expect(200) + .get(`/analyses/${testAnalysisId}/messages/subscriptions/${subscriptionId}`) + .expect(HttpStatus.OK) .then((res) => { expect(res.body.id).toBe(subscriptionId); expect(res.body.analysisId).toBe(testAnalysisId); @@ -83,14 +97,45 @@ describe('Message Subscription Module', () => { }); it.each([ - ['', 'http://localhost/foo'], - [1, 'http://localhost/foo'], - ['analysis-1', ''], - ['analysis-1', 'not:a-domain'], - ])('/POST subscriptions should return an error on malformed body', async (analysisId, webhookUrl) => { + [''], + ['not:a-domain'], + ])('/POST subscriptions should return an error on malformed body', async (webhookUrl) => { + await request(app.getHttpServer()) + .post('/analyses/foo/messages/subscriptions') + .send({ webhookUrl }) + .expect(HttpStatus.BAD_REQUEST); + }); + + it('/POST subscriptions returns 404 if analysis does not exist', async () => { + const testAnalysisId = 'b2b1a935-3f9f-452d-83c2-d5a21a5cd616'; + const testWebhookUrl = 'http://localhost/bar'; + + jest.spyOn(hubApiClient.analysis, 'getOne').mockImplementation(() => Promise.reject({ + statusCode: 404, + })); + + await request(app.getHttpServer()) + .post(`/analyses/${testAnalysisId}/messages/subscriptions`) + .send({ webhookUrl: testWebhookUrl }) + .expect(HttpStatus.NOT_FOUND); + }); + + it.each([ + [500], + [501], + [502], + [503], + ])('/POST subscriptions returns 502 if central side ', async (httpStatusCode) => { + const testAnalysisId = 'b2b1a935-3f9f-452d-83c2-d5a21a5cd616'; + const testWebhookUrl = 'http://localhost/bar'; + + jest.spyOn(hubApiClient.analysis, 'getOne').mockImplementation(() => Promise.reject({ + statusCode: httpStatusCode, + })); + await request(app.getHttpServer()) - .post('/messages/subscriptions') - .send({ analysisId, webhookUrl }) - .expect(400); + .post(`/analyses/${testAnalysisId}/messages/subscriptions`) + .send({ webhookUrl: testWebhookUrl }) + .expect(HttpStatus.BAD_GATEWAY); }); });