From 7d4b9b0bf1de08e9f09dcb9f20c1ad578b7ffcc0 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Tue, 5 Dec 2023 13:51:21 +0100 Subject: [PATCH 01/29] Add initial authorisation --- .../dto/board/content-element.url.params.ts | 10 ++++++++++ .../board/controller/element.controller.ts | 16 ++++++++++++++++ apps/server/src/modules/board/uc/element.uc.ts | 5 +++++ .../src/modules/tldraw/controller/tldraw.ws.ts | 17 +++++++++++++++-- .../modules/tldraw/service/tldraw.ws.service.ts | 17 ++++++++++++++++- .../src/modules/tldraw/tldraw-ws.module.ts | 3 ++- .../modules/tldraw/types/ws-close-code-enum.ts | 1 + 7 files changed, 65 insertions(+), 4 deletions(-) diff --git a/apps/server/src/modules/board/controller/dto/board/content-element.url.params.ts b/apps/server/src/modules/board/controller/dto/board/content-element.url.params.ts index d6ec2b26217..0908472861b 100644 --- a/apps/server/src/modules/board/controller/dto/board/content-element.url.params.ts +++ b/apps/server/src/modules/board/controller/dto/board/content-element.url.params.ts @@ -10,3 +10,13 @@ export class ContentElementUrlParams { }) contentElementId!: string; } + +export class DrawingPermissionUrlParams { + @IsMongoId() + @ApiProperty({ + description: 'The name of a drawing.', + required: true, + nullable: false, + }) + drawingName!: string; +} diff --git a/apps/server/src/modules/board/controller/element.controller.ts b/apps/server/src/modules/board/controller/element.controller.ts index 73eb9848774..484d13ae368 100644 --- a/apps/server/src/modules/board/controller/element.controller.ts +++ b/apps/server/src/modules/board/controller/element.controller.ts @@ -4,6 +4,7 @@ import { Controller, Delete, ForbiddenException, + Get, HttpCode, NotFoundException, Param, @@ -21,6 +22,7 @@ import { CreateSubmissionItemBodyParams, DrawingElementContentBody, DrawingElementResponse, + DrawingPermissionUrlParams, ExternalToolElementContentBody, ExternalToolElementResponse, FileElementContentBody, @@ -141,4 +143,18 @@ export class ElementController { return response; } + + @ApiOperation({ summary: 'Check permission for a drawing element.' }) + @ApiResponse({ status: 204 }) + @ApiResponse({ status: 400, type: ApiValidationError }) + @ApiResponse({ status: 403, type: ForbiddenException }) + @ApiResponse({ status: 404, type: NotFoundException }) + @HttpCode(204) + @Get(':drawingName/permission') + async checkDrawingPermission( + @Param() urlParams: DrawingPermissionUrlParams, + @CurrentUser() currentUser: ICurrentUser + ): Promise { + await this.elementUc.checkElementReadPermission(currentUser.userId, urlParams.drawingName); + } } diff --git a/apps/server/src/modules/board/uc/element.uc.ts b/apps/server/src/modules/board/uc/element.uc.ts index b9043cd71f0..a7f978a1fb3 100644 --- a/apps/server/src/modules/board/uc/element.uc.ts +++ b/apps/server/src/modules/board/uc/element.uc.ts @@ -60,6 +60,11 @@ export class ElementUc extends BaseUc { return element; } + async checkElementReadPermission(userId: EntityId, elementId: EntityId): Promise { + const element = await this.elementService.findById(elementId); + await this.checkPermission(userId, element, Action.read); + } + async createSubmissionItem( userId: EntityId, contentElementId: EntityId, diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index 343997b2aba..9658828b64a 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -1,6 +1,8 @@ import { WebSocketGateway, WebSocketServer, OnGatewayInit, OnGatewayConnection } from '@nestjs/websockets'; import { Server, WebSocket } from 'ws'; +import { Request } from 'express'; import { ConfigService } from '@nestjs/config'; +import cookie from 'cookie'; import { TldrawConfig, SOCKET_PORT } from '../config'; import { WsCloseCodeEnum } from '../types'; import { TldrawWsService } from '../service'; @@ -15,9 +17,20 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { private readonly tldrawWsService: TldrawWsService ) {} - public handleConnection(client: WebSocket, request: Request): void { + async handleConnection(client: WebSocket, request: Request): Promise { const docName = this.getDocNameFromRequest(request); - + const cookies = cookie.parse(request.headers.cookie || ''); + if (!cookies?.jwt) { + client.close(WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, 'Unauthorised connection.'); + } + try { + await this.tldrawWsService.authorizeConnection(docName, cookies.jwt); + } catch (e) { + client.close( + WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, + "Unauthorised connection - you don't have permission to this drawing." + ); + } if (docName.length > 0 && this.configService.get('FEATURE_TLDRAW_ENABLED')) { this.tldrawWsService.setupWSConnection(client, docName); } else { diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts index 660f5258fa8..36880e88cf8 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts @@ -4,6 +4,9 @@ import WebSocket from 'ws'; import { applyAwarenessUpdate, encodeAwarenessUpdate, removeAwarenessStates } from 'y-protocols/awareness'; import { encoding, decoding, map } from 'lib0'; import { readSyncMessage, writeSyncStep1, writeUpdate } from 'y-protocols/sync'; +import { firstValueFrom } from 'rxjs'; +import { HttpService } from '@nestjs/axios'; +import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { Persitence, WSConnectionState, WSMessageType } from '../types'; import { TldrawConfig } from '../config'; import { WsSharedDocDo } from '../domain/ws-shared-doc.do'; @@ -19,7 +22,8 @@ export class TldrawWsService { constructor( private readonly configService: ConfigService, - private readonly tldrawBoardRepo: TldrawBoardRepo + private readonly tldrawBoardRepo: TldrawBoardRepo, + private readonly httpService: HttpService ) { this.pingTimeout = this.configService.get('TLDRAW_PING_TIMEOUT'); } @@ -206,4 +210,15 @@ export class TldrawWsService { public async flushDocument(docName: string): Promise { await this.tldrawBoardRepo.flushDocument(docName); } + + async authorizeConnection(drawingName: string, token: string) { + await firstValueFrom( + this.httpService.get(`${Configuration.get('HOST') as string}/api/v3/elements/${drawingName}/permission`, { + headers: { + Accept: 'Application/json', + Authorization: `Bearer ${token}`, + }, + }) + ); + } } diff --git a/apps/server/src/modules/tldraw/tldraw-ws.module.ts b/apps/server/src/modules/tldraw/tldraw-ws.module.ts index 98e91b5b3e6..1a2b294f7c3 100644 --- a/apps/server/src/modules/tldraw/tldraw-ws.module.ts +++ b/apps/server/src/modules/tldraw/tldraw-ws.module.ts @@ -3,13 +3,14 @@ import { ConfigModule } from '@nestjs/config'; import { createConfigModuleOptions } from '@src/config'; import { CoreModule } from '@src/core'; import { Logger } from '@src/core/logger'; +import { HttpModule } from '@nestjs/axios'; import { TldrawBoardRepo } from './repo'; import { TldrawWsService } from './service'; import { TldrawWs } from './controller'; import { config } from './config'; @Module({ - imports: [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config))], + imports: [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config)), HttpModule], providers: [Logger, TldrawWs, TldrawWsService, TldrawBoardRepo], }) export class TldrawWsModule {} diff --git a/apps/server/src/modules/tldraw/types/ws-close-code-enum.ts b/apps/server/src/modules/tldraw/types/ws-close-code-enum.ts index 274fa99a6ae..bab15d59023 100644 --- a/apps/server/src/modules/tldraw/types/ws-close-code-enum.ts +++ b/apps/server/src/modules/tldraw/types/ws-close-code-enum.ts @@ -1,3 +1,4 @@ export enum WsCloseCodeEnum { WS_CLIENT_BAD_REQUEST_CODE = 4400, + WS_CLIENT_UNAUTHORISED_CONNECTION_CODE = 4401, } From 848142f9551390798ae85533da8cd83e5582dad2 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Tue, 5 Dec 2023 14:05:59 +0100 Subject: [PATCH 02/29] Add HttpModule to spec file --- .../src/modules/tldraw/service/tldraw.ws.service.spec.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts index ddd186fed0a..330675ba247 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts @@ -10,6 +10,7 @@ import * as SyncProtocols from 'y-protocols/sync'; import * as AwarenessProtocol from 'y-protocols/awareness'; import { encoding } from 'lib0'; import { TldrawWsFactory } from '@shared/testing/factory/tldraw.ws.factory'; +import { HttpModule } from '@nestjs/axios'; import { WsSharedDocDo } from '../domain/ws-shared-doc.do'; import { config } from '../config'; import { TldrawBoardRepo } from '../repo'; @@ -48,7 +49,7 @@ describe('TldrawWSService', () => { jest.useFakeTimers(); beforeAll(async () => { - const imports = [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config))]; + const imports = [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config)), HttpModule]; const testingModule = await Test.createTestingModule({ imports, providers: [TldrawWs, TldrawBoardRepo, TldrawWsService], From 62585e34fbf8bbde4ff05853408120313bfffc09 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Thu, 7 Dec 2023 08:59:05 +0100 Subject: [PATCH 03/29] Add HttpModule to test module --- apps/server/src/modules/tldraw/tldraw-ws-test.module.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts b/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts index 6e3c5a58479..66ffd485690 100644 --- a/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts +++ b/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts @@ -3,12 +3,13 @@ import { MongoMemoryDatabaseModule, MongoDatabaseModuleOptions } from '@infra/da import { CoreModule } from '@src/core'; import { ConfigModule } from '@nestjs/config'; import { createConfigModuleOptions } from '@src/config'; +import { HttpModule } from '@nestjs/axios'; import { TldrawBoardRepo } from './repo'; import { TldrawWsService } from './service'; import { config } from './config'; import { TldrawWs } from './controller'; -const imports = [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config))]; +const imports = [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config)), HttpModule]; const providers = [TldrawWs, TldrawBoardRepo, TldrawWsService]; @Module({ imports, From 43aa723833e4685131fe6e90cdeb4ddab6df6190 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Thu, 7 Dec 2023 12:06:53 +0100 Subject: [PATCH 04/29] Add HttpService to test module --- apps/server/src/modules/tldraw/tldraw-ws-test.module.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts b/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts index 66ffd485690..2e8ba0b997b 100644 --- a/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts +++ b/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts @@ -3,14 +3,14 @@ import { MongoMemoryDatabaseModule, MongoDatabaseModuleOptions } from '@infra/da import { CoreModule } from '@src/core'; import { ConfigModule } from '@nestjs/config'; import { createConfigModuleOptions } from '@src/config'; -import { HttpModule } from '@nestjs/axios'; +import { HttpModule, HttpService } from '@nestjs/axios'; import { TldrawBoardRepo } from './repo'; import { TldrawWsService } from './service'; import { config } from './config'; import { TldrawWs } from './controller'; const imports = [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config)), HttpModule]; -const providers = [TldrawWs, TldrawBoardRepo, TldrawWsService]; +const providers = [TldrawWs, TldrawBoardRepo, TldrawWsService, HttpService]; @Module({ imports, providers, From 3d0b014b6fd654af0dd12946fc5f6345f260bad4 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 8 Dec 2023 10:49:56 +0100 Subject: [PATCH 05/29] Remove HttpService from test module --- apps/server/src/modules/tldraw/tldraw-ws-test.module.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts b/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts index 2e8ba0b997b..66ffd485690 100644 --- a/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts +++ b/apps/server/src/modules/tldraw/tldraw-ws-test.module.ts @@ -3,14 +3,14 @@ import { MongoMemoryDatabaseModule, MongoDatabaseModuleOptions } from '@infra/da import { CoreModule } from '@src/core'; import { ConfigModule } from '@nestjs/config'; import { createConfigModuleOptions } from '@src/config'; -import { HttpModule, HttpService } from '@nestjs/axios'; +import { HttpModule } from '@nestjs/axios'; import { TldrawBoardRepo } from './repo'; import { TldrawWsService } from './service'; import { config } from './config'; import { TldrawWs } from './controller'; const imports = [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config)), HttpModule]; -const providers = [TldrawWs, TldrawBoardRepo, TldrawWsService, HttpService]; +const providers = [TldrawWs, TldrawBoardRepo, TldrawWsService]; @Module({ imports, providers, From 9049c24bb8800c7154d70c6d21a508309a6a899d Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 8 Dec 2023 11:48:07 +0100 Subject: [PATCH 06/29] Add test for added code --- .../src/modules/board/uc/element.uc.spec.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/apps/server/src/modules/board/uc/element.uc.spec.ts b/apps/server/src/modules/board/uc/element.uc.spec.ts index cb2e7846118..a57dbb5e051 100644 --- a/apps/server/src/modules/board/uc/element.uc.spec.ts +++ b/apps/server/src/modules/board/uc/element.uc.spec.ts @@ -281,4 +281,28 @@ describe(ElementUc.name, () => { }); }); }); + + describe('checkDrawingPermission', () => { + describe('', () => { + const setup = () => { + const user = userFactory.build(); + const drawingElement = drawingElementFactory.build(); + + return { drawingElement, user }; + }; + + it('should execute properly', async () => { + const { drawingElement, user } = setup(); + elementService.findById.mockResolvedValue(drawingElement); + await uc.checkElementReadPermission(user.id, drawingElement.id); + expect(elementService.findById).toHaveBeenCalledWith(drawingElement.id); + }); + + it('should throw', async () => { + const { drawingElement, user } = setup(); + elementService.findById.mockRejectedValue(new Error()); + await expect(uc.checkElementReadPermission(user.id, drawingElement.id)).rejects.toThrow(); + }); + }); + }); }); From 094be113a7dd54153d6cc2d0a908073e348e4102 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Tue, 12 Dec 2023 16:50:46 +0100 Subject: [PATCH 07/29] Add test for permission endpoint --- .../drawing-item-check-permission.api.spec.ts | 118 ++++++++++++++++++ .../board/controller/element.controller.ts | 4 +- 2 files changed, 120 insertions(+), 2 deletions(-) create mode 100644 apps/server/src/modules/board/controller/api-test/drawing-item-check-permission.api.spec.ts diff --git a/apps/server/src/modules/board/controller/api-test/drawing-item-check-permission.api.spec.ts b/apps/server/src/modules/board/controller/api-test/drawing-item-check-permission.api.spec.ts new file mode 100644 index 00000000000..e81f4164d3d --- /dev/null +++ b/apps/server/src/modules/board/controller/api-test/drawing-item-check-permission.api.spec.ts @@ -0,0 +1,118 @@ +import { EntityManager } from '@mikro-orm/mongodb'; +import { ServerTestModule } from '@modules/server'; +import { INestApplication } from '@nestjs/common'; +import { Test, TestingModule } from '@nestjs/testing'; +import { BoardExternalReferenceType } from '@shared/domain/domainobject'; +import { + TestApiClient, + UserAndAccountTestFactory, + cardNodeFactory, + cleanupCollections, + columnBoardNodeFactory, + columnNodeFactory, + courseFactory, +} from '@shared/testing'; +import { drawingElementNodeFactory } from '@shared/testing/factory/boardnode/drawing-element-node.factory'; + +const baseRouteName = '/elements'; +describe('drawing permission check (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); + }); + + afterAll(async () => { + await app.close(); + }); + + describe('when user is a valid teacher who is part of course', () => { + const setup = async () => { + await cleanupCollections(em); + + const { teacherAccount, teacherUser } = UserAndAccountTestFactory.buildTeacher(); + const course = courseFactory.build({ teachers: [teacherUser] }); + await em.persistAndFlush([teacherAccount, teacherUser, course]); + + const columnBoardNode = columnBoardNodeFactory.buildWithId({ + context: { id: course.id, type: BoardExternalReferenceType.Course }, + }); + + const columnNode = columnNodeFactory.buildWithId({ parent: columnBoardNode }); + + const cardNode = cardNodeFactory.buildWithId({ parent: columnNode }); + + const drawingItemNode = drawingElementNodeFactory.buildWithId({ parent: cardNode }); + + await em.persistAndFlush([columnBoardNode, columnNode, cardNode, drawingItemNode]); + em.clear(); + + const loggedInClient = await testApiClient.login(teacherAccount); + + return { loggedInClient, teacherUser, columnBoardNode, columnNode, cardNode, drawingItemNode }; + }; + + it('should return status 201', async () => { + const { loggedInClient, drawingItemNode } = await setup(); + + const response = await loggedInClient.get(`${drawingItemNode.id}/permission`); + + expect(response.status).toEqual(201); + }); + }); + + describe('when only teacher is part of course', () => { + const setup = async () => { + await cleanupCollections(em); + + const { teacherAccount, teacherUser } = UserAndAccountTestFactory.buildTeacher(); + const { studentAccount, studentUser } = UserAndAccountTestFactory.buildStudent(); + + const course = courseFactory.build({ students: [teacherUser] }); + await em.persistAndFlush([teacherAccount, teacherUser, course, studentAccount, studentUser]); + + const columnBoardNode = columnBoardNodeFactory.buildWithId({ + context: { id: course.id, type: BoardExternalReferenceType.Course }, + }); + + const columnNode = columnNodeFactory.buildWithId({ parent: columnBoardNode }); + + const cardNode = cardNodeFactory.buildWithId({ parent: columnNode }); + + const drawingItemNode = drawingElementNodeFactory.buildWithId({ parent: cardNode }); + + await em.persistAndFlush([columnBoardNode, columnNode, cardNode, drawingItemNode]); + em.clear(); + + const loggedInClient = await testApiClient.login(studentAccount); + + return { loggedInClient, studentUser, columnBoardNode, columnNode, cardNode, drawingItemNode }; + }; + + it('should return status 401 for student not assigned to course', async () => { + const { loggedInClient, drawingItemNode } = await setup(); + + const response = await loggedInClient.get(`${drawingItemNode.id}/permission`); + + expect(response.status).toEqual(403); + }); + + it('should return status 404 for wrong id', async () => { + const { loggedInClient } = await setup(); + const wrongRandomId = '655b048616056135293d1e63'; + + const response = await loggedInClient.get(`${wrongRandomId}/permission`); + + expect(response.status).toEqual(404); + }); + }); +}); diff --git a/apps/server/src/modules/board/controller/element.controller.ts b/apps/server/src/modules/board/controller/element.controller.ts index 484d13ae368..fdcb70a6e72 100644 --- a/apps/server/src/modules/board/controller/element.controller.ts +++ b/apps/server/src/modules/board/controller/element.controller.ts @@ -145,11 +145,11 @@ export class ElementController { } @ApiOperation({ summary: 'Check permission for a drawing element.' }) - @ApiResponse({ status: 204 }) + @ApiResponse({ status: 201 }) @ApiResponse({ status: 400, type: ApiValidationError }) @ApiResponse({ status: 403, type: ForbiddenException }) @ApiResponse({ status: 404, type: NotFoundException }) - @HttpCode(204) + @HttpCode(201) @Get(':drawingName/permission') async checkDrawingPermission( @Param() urlParams: DrawingPermissionUrlParams, From 193a5d076d783d32e1b9381212cfe7fb7202784b Mon Sep 17 00:00:00 2001 From: blazejpass Date: Thu, 14 Dec 2023 12:57:18 +0100 Subject: [PATCH 08/29] Input suggested changes --- .../drawing-item-check-permission.api.spec.ts | 21 +++++++-- .../dto/board/content-element.url.params.ts | 10 ----- .../board/controller/element.controller.ts | 12 +++-- .../src/modules/board/uc/element.uc.spec.ts | 44 +++++++++++-------- .../tldraw/service/tldraw.ws.service.spec.ts | 40 +++++++++++++++-- 5 files changed, 86 insertions(+), 41 deletions(-) diff --git a/apps/server/src/modules/board/controller/api-test/drawing-item-check-permission.api.spec.ts b/apps/server/src/modules/board/controller/api-test/drawing-item-check-permission.api.spec.ts index e81f4164d3d..25d05911069 100644 --- a/apps/server/src/modules/board/controller/api-test/drawing-item-check-permission.api.spec.ts +++ b/apps/server/src/modules/board/controller/api-test/drawing-item-check-permission.api.spec.ts @@ -61,12 +61,12 @@ describe('drawing permission check (api)', () => { return { loggedInClient, teacherUser, columnBoardNode, columnNode, cardNode, drawingItemNode }; }; - it('should return status 201', async () => { + it('should return status 200', async () => { const { loggedInClient, drawingItemNode } = await setup(); const response = await loggedInClient.get(`${drawingItemNode.id}/permission`); - expect(response.status).toEqual(201); + expect(response.status).toEqual(200); }); }); @@ -98,13 +98,28 @@ describe('drawing permission check (api)', () => { return { loggedInClient, studentUser, columnBoardNode, columnNode, cardNode, drawingItemNode }; }; - it('should return status 401 for student not assigned to course', async () => { + it('should return status 403 for student not assigned to course', async () => { const { loggedInClient, drawingItemNode } = await setup(); const response = await loggedInClient.get(`${drawingItemNode.id}/permission`); expect(response.status).toEqual(403); }); + }); + + describe('when asking for non-existing resource', () => { + const setup = async () => { + await cleanupCollections(em); + + const { teacherAccount, teacherUser } = UserAndAccountTestFactory.buildTeacher(); + await em.persistAndFlush([teacherAccount, teacherUser]); + + em.clear(); + + const loggedInClient = await testApiClient.login(teacherAccount); + + return { loggedInClient }; + }; it('should return status 404 for wrong id', async () => { const { loggedInClient } = await setup(); diff --git a/apps/server/src/modules/board/controller/dto/board/content-element.url.params.ts b/apps/server/src/modules/board/controller/dto/board/content-element.url.params.ts index 0908472861b..d6ec2b26217 100644 --- a/apps/server/src/modules/board/controller/dto/board/content-element.url.params.ts +++ b/apps/server/src/modules/board/controller/dto/board/content-element.url.params.ts @@ -10,13 +10,3 @@ export class ContentElementUrlParams { }) contentElementId!: string; } - -export class DrawingPermissionUrlParams { - @IsMongoId() - @ApiProperty({ - description: 'The name of a drawing.', - required: true, - nullable: false, - }) - drawingName!: string; -} diff --git a/apps/server/src/modules/board/controller/element.controller.ts b/apps/server/src/modules/board/controller/element.controller.ts index fdcb70a6e72..2032d91b1b1 100644 --- a/apps/server/src/modules/board/controller/element.controller.ts +++ b/apps/server/src/modules/board/controller/element.controller.ts @@ -22,7 +22,6 @@ import { CreateSubmissionItemBodyParams, DrawingElementContentBody, DrawingElementResponse, - DrawingPermissionUrlParams, ExternalToolElementContentBody, ExternalToolElementResponse, FileElementContentBody, @@ -145,16 +144,15 @@ export class ElementController { } @ApiOperation({ summary: 'Check permission for a drawing element.' }) - @ApiResponse({ status: 201 }) + @ApiResponse({ status: 200 }) @ApiResponse({ status: 400, type: ApiValidationError }) @ApiResponse({ status: 403, type: ForbiddenException }) @ApiResponse({ status: 404, type: NotFoundException }) - @HttpCode(201) - @Get(':drawingName/permission') - async checkDrawingPermission( - @Param() urlParams: DrawingPermissionUrlParams, + @Get(':contentElementId/permission') + async checkPermission( + @Param() urlParams: ContentElementUrlParams, @CurrentUser() currentUser: ICurrentUser ): Promise { - await this.elementUc.checkElementReadPermission(currentUser.userId, urlParams.drawingName); + await this.elementUc.checkElementReadPermission(currentUser.userId, urlParams.contentElementId); } } diff --git a/apps/server/src/modules/board/uc/element.uc.spec.ts b/apps/server/src/modules/board/uc/element.uc.spec.ts index a57dbb5e051..41e30972fd5 100644 --- a/apps/server/src/modules/board/uc/element.uc.spec.ts +++ b/apps/server/src/modules/board/uc/element.uc.spec.ts @@ -282,27 +282,35 @@ describe(ElementUc.name, () => { }); }); - describe('checkDrawingPermission', () => { - describe('', () => { - const setup = () => { - const user = userFactory.build(); - const drawingElement = drawingElementFactory.build(); + describe('checkElementReadPermission', () => { + const setup = () => { + const user = userFactory.build(); + const drawingElement = drawingElementFactory.build(); - return { drawingElement, user }; - }; + return { drawingElement, user }; + }; - it('should execute properly', async () => { - const { drawingElement, user } = setup(); - elementService.findById.mockResolvedValue(drawingElement); - await uc.checkElementReadPermission(user.id, drawingElement.id); - expect(elementService.findById).toHaveBeenCalledWith(drawingElement.id); - }); + it('should execute properly', async () => { + const { drawingElement, user } = setup(); + elementService.findById.mockResolvedValue(drawingElement); - it('should throw', async () => { - const { drawingElement, user } = setup(); - elementService.findById.mockRejectedValue(new Error()); - await expect(uc.checkElementReadPermission(user.id, drawingElement.id)).rejects.toThrow(); - }); + await uc.checkElementReadPermission(user.id, drawingElement.id); + + expect(elementService.findById).toHaveBeenCalledWith(drawingElement.id); + }); + + it('should throw at find element by Id', async () => { + const { drawingElement, user } = setup(); + elementService.findById.mockRejectedValue(new Error()); + + await expect(uc.checkElementReadPermission(user.id, drawingElement.id)).rejects.toThrow(); + }); + + it('should throw at check permission', async () => { + const { drawingElement, user } = setup(); + authorizationService.hasPermission.mockReturnValueOnce(false); + + await expect(uc.checkElementReadPermission(user.id, drawingElement.id)).rejects.toThrow(); }); }); }); diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts index 330675ba247..9081453c27d 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts @@ -10,7 +10,12 @@ import * as SyncProtocols from 'y-protocols/sync'; import * as AwarenessProtocol from 'y-protocols/awareness'; import { encoding } from 'lib0'; import { TldrawWsFactory } from '@shared/testing/factory/tldraw.ws.factory'; -import { HttpModule } from '@nestjs/axios'; +import { HttpService } from '@nestjs/axios'; +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { of, throwError } from 'rxjs'; +import { AxiosResponse } from 'axios'; +import { DeletionRequestOutput } from '@modules/deletion'; +import { axiosResponseFactory } from '@shared/testing'; import { WsSharedDocDo } from '../domain/ws-shared-doc.do'; import { config } from '../config'; import { TldrawBoardRepo } from '../repo'; @@ -37,6 +42,7 @@ describe('TldrawWSService', () => { let app: INestApplication; let ws: WebSocket; let service: TldrawWsService; + let httpService: DeepMocked; const gatewayPort = 3346; const wsUrl = TestConnection.getWsUrl(gatewayPort); @@ -49,13 +55,22 @@ describe('TldrawWSService', () => { jest.useFakeTimers(); beforeAll(async () => { - const imports = [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config)), HttpModule]; + const imports = [CoreModule, ConfigModule.forRoot(createConfigModuleOptions(config))]; const testingModule = await Test.createTestingModule({ imports, - providers: [TldrawWs, TldrawBoardRepo, TldrawWsService], + providers: [ + TldrawWs, + TldrawBoardRepo, + TldrawWsService, + { + provide: HttpService, + useValue: createMock(), + }, + ], }).compile(); service = testingModule.get(TldrawWsService); + httpService = testingModule.get(HttpService); app = testingModule.createNestApplication(); app.useWebSocketAdapter(new WsAdapter(app)); jest.useFakeTimers({ advanceTimers: true, doNotFake: ['setInterval', 'clearInterval', 'setTimeout'] }); @@ -447,4 +462,23 @@ describe('TldrawWSService', () => { flushDocumentSpy.mockRestore(); }); }); + + describe('authorizeConnection', () => { + it('should call properly method', async () => { + const response: AxiosResponse = axiosResponseFactory.build({ + status: 200, + }); + + httpService.get.mockReturnValueOnce(of(response)); + + await expect(service.authorizeConnection('drawingName', 'token')).resolves.not.toThrow(); + }); + + it('should throw error', async () => { + const error = new Error('unknown error'); + httpService.get.mockReturnValueOnce(throwError(() => error)); + + await expect(service.authorizeConnection('drawingName', 'token')).rejects.toThrow(); + }); + }); }); From 2148b0e8387242bf10caed4e8348bd55061a5cce Mon Sep 17 00:00:00 2001 From: blazejpass Date: Thu, 14 Dec 2023 13:22:56 +0100 Subject: [PATCH 09/29] Change params usage in test file --- .../src/modules/tldraw/service/tldraw.ws.service.spec.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts index 9081453c27d..0d99d10c1ce 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts @@ -465,20 +465,22 @@ describe('TldrawWSService', () => { describe('authorizeConnection', () => { it('should call properly method', async () => { + const params = { drawingName: 'drawingName', token: 'token' }; const response: AxiosResponse = axiosResponseFactory.build({ status: 200, }); httpService.get.mockReturnValueOnce(of(response)); - await expect(service.authorizeConnection('drawingName', 'token')).resolves.not.toThrow(); + await expect(service.authorizeConnection(params.drawingName, params.token)).resolves.not.toThrow(); }); it('should throw error', async () => { + const params = { drawingName: 'drawingName', token: 'token' }; const error = new Error('unknown error'); httpService.get.mockReturnValueOnce(throwError(() => error)); - await expect(service.authorizeConnection('drawingName', 'token')).rejects.toThrow(); + await expect(service.authorizeConnection(params.drawingName, params.token)).rejects.toThrow(); }); }); }); From b97d6862cca8c50238aec5dcd4c4f77137806eb4 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Thu, 14 Dec 2023 14:52:43 +0100 Subject: [PATCH 10/29] Adjust and add tests for jwt coockie code --- .../controller/api-test/tldraw.ws.api.spec.ts | 87 +++++++++++++++++-- .../modules/tldraw/controller/tldraw.ws.ts | 48 ++++++---- apps/server/src/modules/tldraw/types/index.ts | 2 +- .../tldraw/types/ws-close-code-enum.ts | 4 - .../src/modules/tldraw/types/ws-close-enum.ts | 9 ++ 5 files changed, 124 insertions(+), 26 deletions(-) delete mode 100644 apps/server/src/modules/tldraw/types/ws-close-code-enum.ts create mode 100644 apps/server/src/modules/tldraw/types/ws-close-enum.ts diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts index ade447b127c..009cd3e5ac0 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts @@ -3,14 +3,21 @@ import { Test } from '@nestjs/testing'; import WebSocket from 'ws'; import { TextEncoder } from 'util'; import { INestApplication } from '@nestjs/common'; -import { TldrawWsTestModule } from '@src/modules/tldraw/tldraw-ws-test.module'; -import { TldrawWs } from '../tldraw.ws'; +import { throwError } from 'rxjs'; +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { HttpService } from '@nestjs/axios'; +import { WsCloseCodeEnum, WsCloseMessageEnum } from '../../types'; +import { TldrawWsTestModule } from '../../tldraw-ws-test.module'; +import { TldrawWsService } from '../../service'; import { TestConnection } from '../../testing/test-connection'; +import { TldrawWs } from '../tldraw.ws'; describe('WebSocketController (WsAdapter)', () => { let app: INestApplication; let gateway: TldrawWs; let ws: WebSocket; + let wsService: TldrawWsService; + let httpService: DeepMocked; const gatewayPort = 3346; const wsUrl = TestConnection.getWsUrl(gatewayPort); @@ -21,8 +28,16 @@ describe('WebSocketController (WsAdapter)', () => { beforeAll(async () => { const testingModule = await Test.createTestingModule({ imports: [TldrawWsTestModule], + providers: [ + { + provide: HttpService, + useValue: createMock(), + }, + ], }).compile(); gateway = testingModule.get(TldrawWs); + wsService = testingModule.get(TldrawWsService); + httpService = testingModule.get(HttpService); app = testingModule.createNestApplication(); app.useWebSocketAdapter(new WsAdapter(app)); await app.init(); @@ -57,11 +72,12 @@ describe('WebSocketController (WsAdapter)', () => { ws.send(buffer, () => {}); expect(handleConnectionSpy).toHaveBeenCalledTimes(1); + handleConnectionSpy.mockRestore(); ws.close(); }); it(`check if client will receive message`, async () => { - const { buffer } = await setup(); + const { handleConnectionSpy, buffer } = await setup(); ws.send(buffer, () => {}); gateway.server.on('connection', (client) => { @@ -70,6 +86,7 @@ describe('WebSocketController (WsAdapter)', () => { }); }); + handleConnectionSpy.mockRestore(); ws.close(); }); }); @@ -97,6 +114,7 @@ describe('WebSocketController (WsAdapter)', () => { expect(handleConnectionSpy).toHaveBeenCalled(); expect(handleConnectionSpy).toHaveBeenCalledTimes(2); + handleConnectionSpy.mockRestore(); ws.close(); ws2.close(); }); @@ -105,24 +123,83 @@ describe('WebSocketController (WsAdapter)', () => { describe('when tldraw is not correctly setup', () => { const setup = async () => { const handleConnectionSpy = jest.spyOn(gateway, 'handleConnection'); + const setupConnectionSpy = jest.spyOn(wsService, 'setupWSConnection'); ws = await TestConnection.setupWs(wsUrl); return { handleConnectionSpy, + setupConnectionSpy, }; }; it(`should refuse connection if there is no docName`, async () => { - const { handleConnectionSpy } = await setup(); + const { handleConnectionSpy, setupConnectionSpy } = await setup(); const { buffer } = getMessage(); ws.send(buffer); expect(gateway.server).toBeDefined(); - expect(handleConnectionSpy).toHaveBeenCalled(); + expect(setupConnectionSpy).not.toHaveBeenCalled(); expect(handleConnectionSpy).toHaveBeenCalledTimes(1); + handleConnectionSpy.mockRestore(); + ws.close(); + }); + }); + + describe('when checking cookie', () => { + const setup = async () => { + const setupConnectionSpy = jest.spyOn(wsService, 'setupWSConnection'); + const closeClientSpy = jest.spyOn(gateway, 'closeClient'); + const httpGetCallSpy = jest.spyOn(httpService, 'get'); + + ws = await TestConnection.setupWs(wsUrl); + + return { + setupConnectionSpy, + closeClientSpy, + httpGetCallSpy, + }; + }; + + it(`should refuse connection if there is no jwt in cookie`, async () => { + const { setupConnectionSpy, closeClientSpy, httpGetCallSpy } = await setup(); + + const { buffer } = getMessage(); + jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); + ws.send(buffer); + + expect(closeClientSpy).toHaveBeenCalledWith( + expect.anything(), + WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE + ); + + setupConnectionSpy.mockRestore(); + closeClientSpy.mockRestore(); + httpGetCallSpy.mockRestore(); + ws.close(); + }); + + it(`should refuse connection if jwt is wrong`, async () => { + const { setupConnectionSpy, closeClientSpy, httpGetCallSpy } = await setup(); + const { buffer } = getMessage(); + const error = new Error('unknown error'); + httpGetCallSpy.mockReturnValueOnce(throwError(() => error)); + + jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); + ws.send(buffer); + + expect(closeClientSpy).toHaveBeenCalledWith( + expect.anything(), + WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE + ); + + httpGetCallSpy.mockRestore(); + setupConnectionSpy.mockRestore(); + closeClientSpy.mockRestore(); ws.close(); }); }); diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index 9658828b64a..22f46ba3a46 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -4,7 +4,7 @@ import { Request } from 'express'; import { ConfigService } from '@nestjs/config'; import cookie from 'cookie'; import { TldrawConfig, SOCKET_PORT } from '../config'; -import { WsCloseCodeEnum } from '../types'; +import { WsCloseCodeEnum, WsCloseMessageEnum } from '../types'; import { TldrawWsService } from '../service'; @WebSocketGateway(SOCKET_PORT) @@ -19,25 +19,32 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { async handleConnection(client: WebSocket, request: Request): Promise { const docName = this.getDocNameFromRequest(request); - const cookies = cookie.parse(request.headers.cookie || ''); + const cookies = this.parseCookiesFromHeader(request); if (!cookies?.jwt) { - client.close(WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, 'Unauthorised connection.'); - } - try { - await this.tldrawWsService.authorizeConnection(docName, cookies.jwt); - } catch (e) { - client.close( + this.closeClient( + client, WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - "Unauthorised connection - you don't have permission to this drawing." + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE ); - } - if (docName.length > 0 && this.configService.get('FEATURE_TLDRAW_ENABLED')) { - this.tldrawWsService.setupWSConnection(client, docName); } else { - client.close( - WsCloseCodeEnum.WS_CLIENT_BAD_REQUEST_CODE, - 'Document name is mandatory in url or Tldraw Tool is turned off.' - ); + try { + await this.tldrawWsService.authorizeConnection(docName, cookies.jwt); + if (docName.length > 0 && this.configService.get('FEATURE_TLDRAW_ENABLED')) { + this.tldrawWsService.setupWSConnection(client, docName); + } else { + this.closeClient( + client, + WsCloseCodeEnum.WS_CLIENT_BAD_REQUEST_CODE, + WsCloseMessageEnum.WS_CLIENT_BAD_REQUEST_MESSAGE + ); + } + } catch (e) { + this.closeClient( + client, + WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE + ); + } } } @@ -58,4 +65,13 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { const urlStripped = request.url.replace(/(\/)|(tldraw-server)/g, ''); return urlStripped; } + + public parseCookiesFromHeader(request: Request): { [p: string]: string } { + const parsedCookies: { [p: string]: string } = cookie.parse(request.headers.cookie || ''); + return parsedCookies; + } + + public closeClient(client: WebSocket, code: WsCloseCodeEnum, data: string): void { + client.close(code, data); + } } diff --git a/apps/server/src/modules/tldraw/types/index.ts b/apps/server/src/modules/tldraw/types/index.ts index 0579e4b8c79..957e55aab3f 100644 --- a/apps/server/src/modules/tldraw/types/index.ts +++ b/apps/server/src/modules/tldraw/types/index.ts @@ -1,3 +1,3 @@ export * from './connection-enum'; -export * from './ws-close-code-enum'; +export * from './ws-close-enum'; export * from './persistence-type'; diff --git a/apps/server/src/modules/tldraw/types/ws-close-code-enum.ts b/apps/server/src/modules/tldraw/types/ws-close-code-enum.ts deleted file mode 100644 index bab15d59023..00000000000 --- a/apps/server/src/modules/tldraw/types/ws-close-code-enum.ts +++ /dev/null @@ -1,4 +0,0 @@ -export enum WsCloseCodeEnum { - WS_CLIENT_BAD_REQUEST_CODE = 4400, - WS_CLIENT_UNAUTHORISED_CONNECTION_CODE = 4401, -} diff --git a/apps/server/src/modules/tldraw/types/ws-close-enum.ts b/apps/server/src/modules/tldraw/types/ws-close-enum.ts new file mode 100644 index 00000000000..b20a6aafbbf --- /dev/null +++ b/apps/server/src/modules/tldraw/types/ws-close-enum.ts @@ -0,0 +1,9 @@ +export enum WsCloseCodeEnum { + WS_CLIENT_BAD_REQUEST_CODE = 4400, + WS_CLIENT_UNAUTHORISED_CONNECTION_CODE = 4401, +} +export enum WsCloseMessageEnum { + WS_CLIENT_BAD_REQUEST_MESSAGE = 'Document name is mandatory in url or Tldraw Tool is turned off.', + WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE = "Unauthorised connection - you don't have permission to this drawing.", + WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE = "Unauthorised connection - you don't have permission to this drawing.", +} From a8b06c1b5fe8d8a3811122424f1fd711b199db41 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 15 Dec 2023 13:26:34 +0100 Subject: [PATCH 11/29] Improve code --- .../controller/api-test/tldraw.ws.api.spec.ts | 91 +++++++++++-------- .../modules/tldraw/controller/tldraw.ws.ts | 2 +- .../tldraw/service/tldraw.ws.service.ts | 9 +- 3 files changed, 57 insertions(+), 45 deletions(-) diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts index 009cd3e5ac0..f04677e911a 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts @@ -55,7 +55,7 @@ describe('WebSocketController (WsAdapter)', () => { jest.clearAllMocks(); }); - describe('when tldraw is correctly setup', () => { + describe('when tldraw connection is established', () => { const setup = async () => { const handleConnectionSpy = jest.spyOn(gateway, 'handleConnection'); jest.spyOn(Uint8Array.prototype, 'reduce').mockReturnValueOnce(1); @@ -67,7 +67,7 @@ describe('WebSocketController (WsAdapter)', () => { return { handleConnectionSpy, buffer }; }; - it(`should handle connection and data transfer`, async () => { + it(`should handle connection`, async () => { const { handleConnectionSpy, buffer } = await setup(); ws.send(buffer, () => {}); @@ -94,8 +94,8 @@ describe('WebSocketController (WsAdapter)', () => { describe('when tldraw doc has multiple clients', () => { const setup = async () => { const handleConnectionSpy = jest.spyOn(gateway, 'handleConnection'); - ws = await TestConnection.setupWs(wsUrl, 'TEST2'); - const ws2 = await TestConnection.setupWs(wsUrl, 'TEST2'); + ws = await TestConnection.setupWs(wsUrl, 'TEST'); + const ws2 = await TestConnection.setupWs(wsUrl, 'TEST'); const { buffer } = getMessage(); @@ -120,54 +120,27 @@ describe('WebSocketController (WsAdapter)', () => { }); }); - describe('when tldraw is not correctly setup', () => { - const setup = async () => { - const handleConnectionSpy = jest.spyOn(gateway, 'handleConnection'); - const setupConnectionSpy = jest.spyOn(wsService, 'setupWSConnection'); - - ws = await TestConnection.setupWs(wsUrl); - - return { - handleConnectionSpy, - setupConnectionSpy, - }; - }; - - it(`should refuse connection if there is no docName`, async () => { - const { handleConnectionSpy, setupConnectionSpy } = await setup(); - - const { buffer } = getMessage(); - ws.send(buffer); - - expect(gateway.server).toBeDefined(); - expect(setupConnectionSpy).not.toHaveBeenCalled(); - expect(handleConnectionSpy).toHaveBeenCalledTimes(1); - - handleConnectionSpy.mockRestore(); - ws.close(); - }); - }); - describe('when checking cookie', () => { - const setup = async () => { + const setup = () => { const setupConnectionSpy = jest.spyOn(wsService, 'setupWSConnection'); const closeClientSpy = jest.spyOn(gateway, 'closeClient'); const httpGetCallSpy = jest.spyOn(httpService, 'get'); - - ws = await TestConnection.setupWs(wsUrl); + const parseCookieSpy = jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); return { setupConnectionSpy, closeClientSpy, httpGetCallSpy, + parseCookieSpy, }; }; it(`should refuse connection if there is no jwt in cookie`, async () => { - const { setupConnectionSpy, closeClientSpy, httpGetCallSpy } = await setup(); - + const { setupConnectionSpy, closeClientSpy, httpGetCallSpy, parseCookieSpy } = setup(); const { buffer } = getMessage(); + jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); + ws = await TestConnection.setupWs(wsUrl, 'TEST'); ws.send(buffer); expect(closeClientSpy).toHaveBeenCalledWith( @@ -179,16 +152,18 @@ describe('WebSocketController (WsAdapter)', () => { setupConnectionSpy.mockRestore(); closeClientSpy.mockRestore(); httpGetCallSpy.mockRestore(); + parseCookieSpy.mockRestore(); ws.close(); }); it(`should refuse connection if jwt is wrong`, async () => { - const { setupConnectionSpy, closeClientSpy, httpGetCallSpy } = await setup(); + const { setupConnectionSpy, closeClientSpy, httpGetCallSpy, parseCookieSpy } = setup(); const { buffer } = getMessage(); const error = new Error('unknown error'); - httpGetCallSpy.mockReturnValueOnce(throwError(() => error)); - jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); + httpGetCallSpy.mockReturnValueOnce(throwError(() => error)); + jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({ jwt: 'mock-wrong-jwt' }); + ws = await TestConnection.setupWs(wsUrl, 'TEST'); ws.send(buffer); expect(closeClientSpy).toHaveBeenCalledWith( @@ -200,6 +175,42 @@ describe('WebSocketController (WsAdapter)', () => { httpGetCallSpy.mockRestore(); setupConnectionSpy.mockRestore(); closeClientSpy.mockRestore(); + parseCookieSpy.mockRestore(); + ws.close(); + }); + }); + + describe('when checking docName', () => { + const setup = () => { + const parseCookieSpy = jest.spyOn(gateway, 'parseCookiesFromHeader'); + const closeClientSpy = jest.spyOn(gateway, 'closeClient'); + const setupConnectionSpy = jest.spyOn(wsService, 'setupWSConnection'); + + return { + parseCookieSpy, + setupConnectionSpy, + closeClientSpy, + }; + }; + + it(`should close for existing cookie and not existing docName`, async () => { + const { parseCookieSpy, setupConnectionSpy, closeClientSpy } = setup(); + const { buffer } = getMessage(); + + parseCookieSpy.mockReturnValueOnce({ jwt: 'jwt-mocked' }); + + ws = await TestConnection.setupWs(wsUrl); + ws.send(buffer); + + expect(closeClientSpy).toHaveBeenCalledWith( + expect.anything(), + WsCloseCodeEnum.WS_CLIENT_BAD_REQUEST_CODE, + WsCloseMessageEnum.WS_CLIENT_BAD_REQUEST_MESSAGE + ); + + closeClientSpy.mockRestore(); + setupConnectionSpy.mockRestore(); + parseCookieSpy.mockRestore(); ws.close(); }); }); diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index 22f46ba3a46..1aebbcfffa4 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -28,8 +28,8 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { ); } else { try { - await this.tldrawWsService.authorizeConnection(docName, cookies.jwt); if (docName.length > 0 && this.configService.get('FEATURE_TLDRAW_ENABLED')) { + await this.tldrawWsService.authorizeConnection(docName, cookies.jwt); this.tldrawWsService.setupWSConnection(client, docName); } else { this.closeClient( diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts index 36880e88cf8..f423509be82 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts @@ -212,12 +212,13 @@ export class TldrawWsService { } async authorizeConnection(drawingName: string, token: string) { + const headers = { + Accept: 'Application/json', + Authorization: `Bearer ${token}`, + }; await firstValueFrom( this.httpService.get(`${Configuration.get('HOST') as string}/api/v3/elements/${drawingName}/permission`, { - headers: { - Accept: 'Application/json', - Authorization: `Bearer ${token}`, - }, + headers, }) ); } From 4454250ff4da2daac69ad0ebea69c9344c843d1c Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 15 Dec 2023 14:15:48 +0100 Subject: [PATCH 12/29] delete unnecessary import --- .../src/modules/tldraw/service/tldraw.ws.service.spec.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts index 0d99d10c1ce..94c6c0a011a 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts @@ -14,7 +14,6 @@ import { HttpService } from '@nestjs/axios'; import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { of, throwError } from 'rxjs'; import { AxiosResponse } from 'axios'; -import { DeletionRequestOutput } from '@modules/deletion'; import { axiosResponseFactory } from '@shared/testing'; import { WsSharedDocDo } from '../domain/ws-shared-doc.do'; import { config } from '../config'; @@ -466,7 +465,7 @@ describe('TldrawWSService', () => { describe('authorizeConnection', () => { it('should call properly method', async () => { const params = { drawingName: 'drawingName', token: 'token' }; - const response: AxiosResponse = axiosResponseFactory.build({ + const response: AxiosResponse = axiosResponseFactory.build({ status: 200, }); From 3effbf5cc2dd9034d57dabbde96e73980644cae2 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Mon, 18 Dec 2023 12:56:45 +0100 Subject: [PATCH 13/29] Adjust and add tests --- .../controller/api-test/tldraw.ws.api.spec.ts | 56 ++++++++++++++++--- 1 file changed, 49 insertions(+), 7 deletions(-) diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts index f04677e911a..e17ef2234f5 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts @@ -122,13 +122,11 @@ describe('WebSocketController (WsAdapter)', () => { describe('when checking cookie', () => { const setup = () => { - const setupConnectionSpy = jest.spyOn(wsService, 'setupWSConnection'); const closeClientSpy = jest.spyOn(gateway, 'closeClient'); const httpGetCallSpy = jest.spyOn(httpService, 'get'); const parseCookieSpy = jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); return { - setupConnectionSpy, closeClientSpy, httpGetCallSpy, parseCookieSpy, @@ -136,7 +134,7 @@ describe('WebSocketController (WsAdapter)', () => { }; it(`should refuse connection if there is no jwt in cookie`, async () => { - const { setupConnectionSpy, closeClientSpy, httpGetCallSpy, parseCookieSpy } = setup(); + const { closeClientSpy, httpGetCallSpy, parseCookieSpy } = setup(); const { buffer } = getMessage(); jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); @@ -149,7 +147,6 @@ describe('WebSocketController (WsAdapter)', () => { WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE ); - setupConnectionSpy.mockRestore(); closeClientSpy.mockRestore(); httpGetCallSpy.mockRestore(); parseCookieSpy.mockRestore(); @@ -157,7 +154,7 @@ describe('WebSocketController (WsAdapter)', () => { }); it(`should refuse connection if jwt is wrong`, async () => { - const { setupConnectionSpy, closeClientSpy, httpGetCallSpy, parseCookieSpy } = setup(); + const { closeClientSpy, httpGetCallSpy, parseCookieSpy } = setup(); const { buffer } = getMessage(); const error = new Error('unknown error'); @@ -173,14 +170,13 @@ describe('WebSocketController (WsAdapter)', () => { ); httpGetCallSpy.mockRestore(); - setupConnectionSpy.mockRestore(); closeClientSpy.mockRestore(); parseCookieSpy.mockRestore(); ws.close(); }); }); - describe('when checking docName', () => { + describe('when checking docName and cookie', () => { const setup = () => { const parseCookieSpy = jest.spyOn(gateway, 'parseCookiesFromHeader'); const closeClientSpy = jest.spyOn(gateway, 'closeClient'); @@ -213,5 +209,51 @@ describe('WebSocketController (WsAdapter)', () => { parseCookieSpy.mockRestore(); ws.close(); }); + + it(`should close for not authorizing connection`, async () => { + const { parseCookieSpy, setupConnectionSpy, closeClientSpy } = setup(); + const { buffer } = getMessage(); + + const httpGetCallSpy = jest.spyOn(httpService, 'get'); + const error = new Error('unknown error'); + httpGetCallSpy.mockReturnValueOnce(throwError(() => error)); + parseCookieSpy.mockReturnValueOnce({ jwt: 'jwt-mocked' }); + + ws = await TestConnection.setupWs(wsUrl, 'TEST'); + ws.send(buffer); + + expect(closeClientSpy).toHaveBeenCalledWith( + expect.anything(), + WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE + ); + + closeClientSpy.mockRestore(); + setupConnectionSpy.mockRestore(); + parseCookieSpy.mockRestore(); + httpGetCallSpy.mockRestore(); + ws.close(); + }); + + it(`should setup connection for proper data`, async () => { + const { parseCookieSpy, setupConnectionSpy, closeClientSpy } = setup(); + const { buffer } = getMessage(); + + parseCookieSpy.mockReturnValueOnce({ jwt: 'jwt-mocked' }); + const httpGetCallSpy = jest + .spyOn(wsService, 'authorizeConnection') + .mockImplementationOnce(() => Promise.resolve()); + + ws = await TestConnection.setupWs(wsUrl, 'TEST'); + ws.send(buffer); + + expect(setupConnectionSpy).toHaveBeenCalledWith(expect.anything(), 'TEST'); + + closeClientSpy.mockRestore(); + setupConnectionSpy.mockRestore(); + parseCookieSpy.mockRestore(); + httpGetCallSpy.mockRestore(); + ws.close(); + }); }); }); From ce90e287baff813f57b957149808c36b7658c797 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Thu, 21 Dec 2023 17:21:36 +0100 Subject: [PATCH 14/29] Fix code according to review suggestions --- .../board/controller/element.controller.ts | 4 +- apps/server/src/modules/tldraw/config.ts | 2 + .../api-test/tldraw.controller.api.spec.ts | 72 +++++++++++++++++++ .../controller/tldraw.controller.spec.ts | 53 -------------- .../modules/tldraw/controller/tldraw.ws.ts | 60 +++++++++++----- .../tldraw/domain/ws-shared-doc.do.spec.ts | 2 +- .../websocket-close-error.loggable.spec.ts | 22 ++++++ .../websocket-close-error.loggable.ts | 13 ++++ .../modules/tldraw/repo/tldraw-board.repo.ts | 6 +- .../tldraw/service/tldraw.ws.service.spec.ts | 4 +- .../tldraw/service/tldraw.ws.service.ts | 11 +-- .../src/modules/tldraw/tldraw-test.module.ts | 35 +++++---- .../src/modules/tldraw/types/ws-close-enum.ts | 4 +- config/default.schema.json | 1 + package-lock.json | 2 +- 15 files changed, 187 insertions(+), 104 deletions(-) create mode 100644 apps/server/src/modules/tldraw/controller/api-test/tldraw.controller.api.spec.ts delete mode 100644 apps/server/src/modules/tldraw/controller/tldraw.controller.spec.ts create mode 100644 apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.spec.ts create mode 100644 apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.ts diff --git a/apps/server/src/modules/board/controller/element.controller.ts b/apps/server/src/modules/board/controller/element.controller.ts index 2032d91b1b1..82cb8d63c58 100644 --- a/apps/server/src/modules/board/controller/element.controller.ts +++ b/apps/server/src/modules/board/controller/element.controller.ts @@ -143,13 +143,13 @@ export class ElementController { return response; } - @ApiOperation({ summary: 'Check permission for a drawing element.' }) + @ApiOperation({ summary: 'Check if user has permission for any course element.' }) @ApiResponse({ status: 200 }) @ApiResponse({ status: 400, type: ApiValidationError }) @ApiResponse({ status: 403, type: ForbiddenException }) @ApiResponse({ status: 404, type: NotFoundException }) @Get(':contentElementId/permission') - async checkPermission( + async readPermission( @Param() urlParams: ContentElementUrlParams, @CurrentUser() currentUser: ICurrentUser ): Promise { diff --git a/apps/server/src/modules/tldraw/config.ts b/apps/server/src/modules/tldraw/config.ts index a892ee6c843..50219da6516 100644 --- a/apps/server/src/modules/tldraw/config.ts +++ b/apps/server/src/modules/tldraw/config.ts @@ -10,6 +10,7 @@ export interface TldrawConfig { FEATURE_TLDRAW_ENABLED: boolean; TLDRAW_PING_TIMEOUT: number; TLDRAW_GC_ENABLED: number; + API_HOST: number; } const tldrawConnectionString: string = Configuration.get('TLDRAW_DB_URL') as string; @@ -24,6 +25,7 @@ const tldrawConfig = { CONNECTION_STRING: tldrawConnectionString, TLDRAW_PING_TIMEOUT: Configuration.get('TLDRAW__PING_TIMEOUT') as number, TLDRAW_GC_ENABLED: Configuration.get('TLDRAW__GC_ENABLED') as boolean, + API_HOST: Configuration.get('API_HOST') as string, }; export const SOCKET_PORT = Configuration.get('TLDRAW__SOCKET_PORT') as number; diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.controller.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.controller.api.spec.ts new file mode 100644 index 00000000000..2428ff4ad70 --- /dev/null +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.controller.api.spec.ts @@ -0,0 +1,72 @@ +import { INestApplication } from '@nestjs/common'; +import { EntityManager } from '@mikro-orm/mongodb'; +import { + cardNodeFactory, + cleanupCollections, + columnBoardNodeFactory, + columnNodeFactory, + courseFactory, + TestApiClient, + UserAndAccountTestFactory, +} from '@shared/testing'; +import { Test, TestingModule } from '@nestjs/testing'; +import { BoardExternalReferenceType } from '@shared/domain/domainobject'; +import { drawingElementNodeFactory } from '@shared/testing/factory/boardnode/drawing-element-node.factory'; +import { TldrawTestModule } from '@modules/tldraw'; + +const baseRouteName = '/tldraw-document'; +describe('tldraw controller (api)', () => { + let app: INestApplication; + let em: EntityManager; + let testApiClient: TestApiClient; + + beforeAll(async () => { + const module: TestingModule = await Test.createTestingModule({ + imports: [TldrawTestModule], + }).compile(); + + app = module.createNestApplication(); + await app.init(); + em = module.get(EntityManager); + testApiClient = new TestApiClient(app, baseRouteName); + }); + + afterAll(async () => { + await app.close(); + }); + + describe('with valid user', () => { + const setup = async () => { + await cleanupCollections(em); + + const { teacherAccount, teacherUser } = UserAndAccountTestFactory.buildTeacher(); + const course = courseFactory.build({ teachers: [teacherUser] }); + await em.persistAndFlush([teacherAccount, teacherUser, course]); + + const columnBoardNode = columnBoardNodeFactory.buildWithId({ + context: { id: course.id, type: BoardExternalReferenceType.Course }, + }); + + const columnNode = columnNodeFactory.buildWithId({ parent: columnBoardNode }); + + const cardNode = cardNodeFactory.buildWithId({ parent: columnNode }); + + const drawingItemNode = drawingElementNodeFactory.buildWithId({ parent: cardNode }); + + await em.persistAndFlush([columnBoardNode, columnNode, cardNode, drawingItemNode]); + em.clear(); + + const loggedInClient = await testApiClient.login(teacherAccount); + + return { loggedInClient, teacherUser, columnBoardNode, columnNode, cardNode, drawingItemNode }; + }; + + it('should return status 200 for delete', async () => { + const { loggedInClient, drawingItemNode } = await setup(); + + const response = await loggedInClient.delete(`${drawingItemNode.id}`); + + expect(response.status).toEqual(200); + }); + }); +}); diff --git a/apps/server/src/modules/tldraw/controller/tldraw.controller.spec.ts b/apps/server/src/modules/tldraw/controller/tldraw.controller.spec.ts deleted file mode 100644 index 2528fd8c4d7..00000000000 --- a/apps/server/src/modules/tldraw/controller/tldraw.controller.spec.ts +++ /dev/null @@ -1,53 +0,0 @@ -import { createMock } from '@golevelup/ts-jest'; -import { Test, TestingModule } from '@nestjs/testing'; -import { TldrawController } from './tldraw.controller'; -import { TldrawService } from '../service/tldraw.service'; -import { TldrawDeleteParams } from './tldraw.params'; - -describe('TldrawController', () => { - let module: TestingModule; - let controller: TldrawController; - let service: TldrawService; - - beforeAll(async () => { - module = await Test.createTestingModule({ - providers: [ - { - provide: TldrawService, - useValue: createMock(), - }, - ], - controllers: [TldrawController], - }).compile(); - - controller = module.get(TldrawController); - service = module.get(TldrawService); - }); - - afterAll(async () => { - await module.close(); - }); - - it('should be defined', () => { - expect(controller).toBeDefined(); - }); - - describe('delete', () => { - describe('when task should be copied via API call', () => { - const setup = () => { - const params: TldrawDeleteParams = { - docName: 'test-name', - }; - - const ucSpy = jest.spyOn(service, 'deleteByDocName').mockImplementation(() => Promise.resolve()); - return { params, ucSpy }; - }; - - it('should call service with parentIds', async () => { - const { params, ucSpy } = setup(); - await controller.deleteByDocName(params); - expect(ucSpy).toHaveBeenCalledWith('test-name'); - }); - }); - }); -}); diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index 1aebbcfffa4..83155fca11d 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -3,6 +3,9 @@ import { Server, WebSocket } from 'ws'; import { Request } from 'express'; import { ConfigService } from '@nestjs/config'; import cookie from 'cookie'; +import { BadRequestException } from '@nestjs/common'; +import { Logger } from '@src/core/logger'; +import { WebsocketCloseErrorLoggable } from '../loggable/websocket-close-error.loggable'; import { TldrawConfig, SOCKET_PORT } from '../config'; import { WsCloseCodeEnum, WsCloseMessageEnum } from '../types'; import { TldrawWsService } from '../service'; @@ -14,37 +17,56 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { constructor( private readonly configService: ConfigService, - private readonly tldrawWsService: TldrawWsService + private readonly tldrawWsService: TldrawWsService, + private readonly logger: Logger ) {} async handleConnection(client: WebSocket, request: Request): Promise { const docName = this.getDocNameFromRequest(request); const cookies = this.parseCookiesFromHeader(request); - if (!cookies?.jwt) { - this.closeClient( - client, - WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE - ); - } else { + if (docName.length > 0 && this.configService.get('FEATURE_TLDRAW_ENABLED')) { try { - if (docName.length > 0 && this.configService.get('FEATURE_TLDRAW_ENABLED')) { - await this.tldrawWsService.authorizeConnection(docName, cookies.jwt); - this.tldrawWsService.setupWSConnection(client, docName); - } else { - this.closeClient( - client, - WsCloseCodeEnum.WS_CLIENT_BAD_REQUEST_CODE, - WsCloseMessageEnum.WS_CLIENT_BAD_REQUEST_MESSAGE - ); - } - } catch (e) { + await this.tldrawWsService.authorizeConnection(docName, cookies?.jwt); + } catch (err) { this.closeClient( client, WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE ); + this.logger.warning( + new WebsocketCloseErrorLoggable( + err as Error, + `(${WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE}) ${WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE}` + ) + ); + } + try { + this.tldrawWsService.setupWSConnection(client, docName); + } catch (err) { + this.closeClient( + client, + WsCloseCodeEnum.WS_CLIENT_ESTABLISHING_CONNECTION_CODE, + WsCloseMessageEnum.WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE + ); + this.logger.warning( + new WebsocketCloseErrorLoggable( + err as Error, + `(${WsCloseCodeEnum.WS_CLIENT_ESTABLISHING_CONNECTION_CODE}) ${WsCloseMessageEnum.WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE}` + ) + ); } + } else { + this.closeClient( + client, + WsCloseCodeEnum.WS_CLIENT_BAD_REQUEST_CODE, + WsCloseMessageEnum.WS_CLIENT_BAD_REQUEST_MESSAGE + ); + this.logger.warning( + new WebsocketCloseErrorLoggable( + new BadRequestException(), + `(${WsCloseCodeEnum.WS_CLIENT_BAD_REQUEST_CODE}) ${WsCloseMessageEnum.WS_CLIENT_BAD_REQUEST_MESSAGE}` + ) + ); } } diff --git a/apps/server/src/modules/tldraw/domain/ws-shared-doc.do.spec.ts b/apps/server/src/modules/tldraw/domain/ws-shared-doc.do.spec.ts index 78cf9ea9428..7b0b0d8c60c 100644 --- a/apps/server/src/modules/tldraw/domain/ws-shared-doc.do.spec.ts +++ b/apps/server/src/modules/tldraw/domain/ws-shared-doc.do.spec.ts @@ -51,7 +51,7 @@ describe('WsSharedDocDo', () => { describe('ydoc client awareness change handler', () => { const setup = async () => { - ws = await TestConnection.setupWs(wsUrl); + ws = await TestConnection.setupWs(wsUrl, 'TEST'); class MockAwareness { on = jest.fn(); diff --git a/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.spec.ts b/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.spec.ts new file mode 100644 index 00000000000..3c70f428eb5 --- /dev/null +++ b/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.spec.ts @@ -0,0 +1,22 @@ +import { WebsocketCloseErrorLoggable } from './websocket-close-error.loggable'; + +describe('WebsocketCloseErrorLoggable', () => { + describe('getLogMessage', () => { + const setup = () => { + const error = new Error('test'); + const errorMessage = 'message'; + + const loggable = new WebsocketCloseErrorLoggable(error, errorMessage); + + return { loggable, error, errorMessage }; + }; + + it('should return a loggable message', () => { + const { loggable, error, errorMessage } = setup(); + + const message = loggable.getLogMessage(); + + expect(message).toEqual({ message: errorMessage, error }); + }); + }); +}); diff --git a/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.ts b/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.ts new file mode 100644 index 00000000000..6da84c3699f --- /dev/null +++ b/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.ts @@ -0,0 +1,13 @@ +import { ErrorLogMessage, Loggable, LogMessage, ValidationErrorLogMessage } from '@src/core/logger'; + +export class WebsocketCloseErrorLoggable implements Loggable { + constructor(private readonly error: Error, private readonly message: string) {} + + getLogMessage(): LogMessage | ErrorLogMessage | ValidationErrorLogMessage { + return { + message: this.message, + type: 'WEBSOCKET_CLOSE_ERROR', + error: this.error, + }; + } +} diff --git a/apps/server/src/modules/tldraw/repo/tldraw-board.repo.ts b/apps/server/src/modules/tldraw/repo/tldraw-board.repo.ts index ce3a124f7f0..8f3b3187158 100644 --- a/apps/server/src/modules/tldraw/repo/tldraw-board.repo.ts +++ b/apps/server/src/modules/tldraw/repo/tldraw-board.repo.ts @@ -26,9 +26,9 @@ export class TldrawBoardRepo { // eslint-disable-next-line @typescript-eslint/no-unsafe-call,@typescript-eslint/no-unsafe-assignment this.mdb = new MongodbPersistence(this.connectionString, { - collectionName: this.collectionName, - flushSize: this.flushSize, - multipleCollections: this.multipleCollections, + collectionName: 'drawings', + flushSize: 400, + multipleCollections: false, }); } diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts index 94c6c0a011a..51306923d6a 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts @@ -102,7 +102,7 @@ describe('TldrawWSService', () => { describe('send', () => { describe('when client is not connected to WS', () => { const setup = async () => { - ws = await TestConnection.setupWs(wsUrl); + ws = await TestConnection.setupWs(wsUrl, 'TEST'); const clientMessageMock = 'test-message'; const closeConSpy = jest.spyOn(service, 'closeConn').mockImplementationOnce(() => {}); @@ -166,7 +166,7 @@ describe('TldrawWSService', () => { describe('when websocket has ready state 0', () => { const setup = async () => { - ws = await TestConnection.setupWs(wsUrl); + ws = await TestConnection.setupWs(wsUrl, 'TEST'); const clientMessageMock = 'test-message'; const sendSpy = jest.spyOn(service, 'send'); diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts index f423509be82..d1aa18f6cc8 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts @@ -1,4 +1,4 @@ -import { Injectable } from '@nestjs/common'; +import { Injectable, UnauthorizedException } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import WebSocket from 'ws'; import { applyAwarenessUpdate, encodeAwarenessUpdate, removeAwarenessStates } from 'y-protocols/awareness'; @@ -6,7 +6,6 @@ import { encoding, decoding, map } from 'lib0'; import { readSyncMessage, writeSyncStep1, writeUpdate } from 'y-protocols/sync'; import { firstValueFrom } from 'rxjs'; import { HttpService } from '@nestjs/axios'; -import { Configuration } from '@hpi-schul-cloud/commons/lib'; import { Persitence, WSConnectionState, WSMessageType } from '../types'; import { TldrawConfig } from '../config'; import { WsSharedDocDo } from '../domain/ws-shared-doc.do'; @@ -211,13 +210,17 @@ export class TldrawWsService { await this.tldrawBoardRepo.flushDocument(docName); } - async authorizeConnection(drawingName: string, token: string) { + public async authorizeConnection(drawingName: string, token: string) { + if (token.length === 0) { + throw new UnauthorizedException('Token was not given'); + } const headers = { Accept: 'Application/json', Authorization: `Bearer ${token}`, }; + await firstValueFrom( - this.httpService.get(`${Configuration.get('HOST') as string}/api/v3/elements/${drawingName}/permission`, { + this.httpService.get(`${this.configService.get('API_HOST')}/api/v3/elements/${drawingName}/permission`, { headers, }) ); diff --git a/apps/server/src/modules/tldraw/tldraw-test.module.ts b/apps/server/src/modules/tldraw/tldraw-test.module.ts index 49f8bd5c820..5523e4bf69a 100644 --- a/apps/server/src/modules/tldraw/tldraw-test.module.ts +++ b/apps/server/src/modules/tldraw/tldraw-test.module.ts @@ -1,26 +1,24 @@ import { DynamicModule, Module } from '@nestjs/common'; import { MongoMemoryDatabaseModule, MongoDatabaseModuleOptions } from '@infra/database'; -import { CoreModule } from '@src/core'; -import { LoggerModule } from '@src/core/logger'; -import { AuthenticationModule } from '@modules/authentication/authentication.module'; -import { AuthorizationModule } from '@modules/authorization'; -import { Course, User } from '@shared/domain/entity'; -import { AuthenticationApiModule } from '../authentication/authentication-api.module'; -import { TldrawWsModule } from './tldraw-ws.module'; -import { TldrawWs } from './controller'; -import { TldrawBoardRepo } from './repo'; -import { TldrawWsService } from './service'; +import { Logger, LoggerModule } from '@src/core/logger'; +import { ConfigModule } from '@nestjs/config'; +import { createConfigModuleOptions } from '@src/config'; +import { RedisModule } from '@infra/redis'; +import { defaultMikroOrmOptions } from '@modules/server'; +import { HttpModule } from '@nestjs/axios'; +import { config } from './config'; +import { TldrawController } from './controller/tldraw.controller'; +import { TldrawService } from './service/tldraw.service'; +import { TldrawRepo } from './repo/tldraw.repo'; const imports = [ - TldrawWsModule, - MongoMemoryDatabaseModule.forRoot({ entities: [User, Course] }), - AuthenticationApiModule, - AuthorizationModule, - AuthenticationModule, - CoreModule, + MongoMemoryDatabaseModule.forRoot({ ...defaultMikroOrmOptions }), LoggerModule, + ConfigModule.forRoot(createConfigModuleOptions(config)), + RedisModule, + HttpModule, ]; -const providers = [TldrawWs, TldrawBoardRepo, TldrawWsService]; +const providers = [Logger, TldrawService, TldrawRepo]; @Module({ imports, providers, @@ -29,7 +27,8 @@ export class TldrawTestModule { static forRoot(options?: MongoDatabaseModuleOptions): DynamicModule { return { module: TldrawTestModule, - imports: [...imports, MongoMemoryDatabaseModule.forRoot({ ...options })], + imports: [...imports, MongoMemoryDatabaseModule.forRoot({ ...defaultMikroOrmOptions, ...options })], + controllers: [TldrawController], providers, }; } diff --git a/apps/server/src/modules/tldraw/types/ws-close-enum.ts b/apps/server/src/modules/tldraw/types/ws-close-enum.ts index b20a6aafbbf..6a3b394931f 100644 --- a/apps/server/src/modules/tldraw/types/ws-close-enum.ts +++ b/apps/server/src/modules/tldraw/types/ws-close-enum.ts @@ -1,9 +1,11 @@ export enum WsCloseCodeEnum { WS_CLIENT_BAD_REQUEST_CODE = 4400, WS_CLIENT_UNAUTHORISED_CONNECTION_CODE = 4401, + WS_CLIENT_ESTABLISHING_CONNECTION_CODE = 4500, } export enum WsCloseMessageEnum { WS_CLIENT_BAD_REQUEST_MESSAGE = 'Document name is mandatory in url or Tldraw Tool is turned off.', WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE = "Unauthorised connection - you don't have permission to this drawing.", - WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE = "Unauthorised connection - you don't have permission to this drawing.", + WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE = 'Unauthorised connection - authorization token was not provided.', + WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE = 'Unable to establish websocket connection.', } diff --git a/config/default.schema.json b/config/default.schema.json index c3ef3115a95..6d5b6b5ed52 100644 --- a/config/default.schema.json +++ b/config/default.schema.json @@ -529,6 +529,7 @@ "API_HOST": { "type": "string", "format": "uri", + "default": "http://localhost:3030", "pattern": ".*(?=9" } }, "node_modules/@ampproject/remapping": { From 56d9feeee5f67346977d525e65139f4c003e3850 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Thu, 28 Dec 2023 15:00:53 +0100 Subject: [PATCH 15/29] Fix code according to review suggestions --- .../src/modules/board/uc/element.uc.spec.ts | 46 ++++++++++++++----- .../controller/api-test/tldraw.ws.api.spec.ts | 28 +++++++---- .../tldraw/service/tldraw.ws.service.spec.ts | 22 +++++++++ 3 files changed, 76 insertions(+), 20 deletions(-) diff --git a/apps/server/src/modules/board/uc/element.uc.spec.ts b/apps/server/src/modules/board/uc/element.uc.spec.ts index 41e30972fd5..f520fc6444d 100644 --- a/apps/server/src/modules/board/uc/element.uc.spec.ts +++ b/apps/server/src/modules/board/uc/element.uc.spec.ts @@ -3,9 +3,11 @@ import { Action, AuthorizationService } from '@modules/authorization'; import { HttpService } from '@nestjs/axios'; import { ForbiddenException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; -import { BoardDoAuthorizable } from '@shared/domain/domainobject'; +import { BoardDoAuthorizable, BoardRoles, UserRoleEnum } from '@shared/domain/domainobject'; import { InputFormat } from '@shared/domain/types'; import { + cardFactory, + columnBoardFactory, drawingElementFactory, fileElementFactory, richTextElementFactory, @@ -79,7 +81,7 @@ describe(ElementUc.name, () => { const richTextElement = richTextElementFactory.build(); const content = { text: 'this has been updated', inputFormat: InputFormat.RICH_TEXT_CK5 }; - const elementSpy = elementService.findById.mockResolvedValue(richTextElement); + const elementSpy = elementService.findById.mockResolvedValueOnce(richTextElement); return { richTextElement, user, content, elementSpy }; }; @@ -107,7 +109,7 @@ describe(ElementUc.name, () => { const fileElement = fileElementFactory.build(); const content = { caption: 'this has been updated', alternativeText: 'this altText has been updated' }; - const elementSpy = elementService.findById.mockResolvedValue(fileElement); + const elementSpy = elementService.findById.mockResolvedValueOnce(fileElement); return { fileElement, user, content, elementSpy }; }; @@ -225,7 +227,7 @@ describe(ElementUc.name, () => { const user = userFactory.build(); const fileElement = fileElementFactory.build(); - elementService.findById.mockResolvedValue(fileElement); + elementService.findById.mockResolvedValueOnce(fileElement); return { fileElement, user }; }; @@ -246,7 +248,7 @@ describe(ElementUc.name, () => { const submissionContainer = submissionContainerElementFactory.build({ children: [fileElement] }); - elementService.findById.mockResolvedValue(submissionContainer); + elementService.findById.mockResolvedValueOnce(submissionContainer); return { submissionContainer, fileElement, user }; }; @@ -267,7 +269,7 @@ describe(ElementUc.name, () => { const submissionItem = submissionItemFactory.build({ userId: user.id }); const submissionContainer = submissionContainerElementFactory.build({ children: [submissionItem] }); - elementService.findById.mockResolvedValue(submissionContainer); + elementService.findById.mockResolvedValueOnce(submissionContainer); return { submissionContainer, submissionItem, user }; }; @@ -286,31 +288,51 @@ describe(ElementUc.name, () => { const setup = () => { const user = userFactory.build(); const drawingElement = drawingElementFactory.build(); + const card = cardFactory.build({ children: [drawingElement] }); + const columnBoard = columnBoardFactory.build({ children: [card] }); + authorizationService.getUserWithPermissions.mockResolvedValueOnce(user); + + const authorizableMock: BoardDoAuthorizable = new BoardDoAuthorizable({ + users: [{ userId: user.id, roles: [BoardRoles.EDITOR], userRoleEnum: UserRoleEnum.TEACHER }], + id: columnBoard.id, + }); + + boardDoAuthorizableService.findById.mockResolvedValueOnce(authorizableMock); return { drawingElement, user }; }; - it('should execute properly', async () => { + it('should properly find the element', async () => { const { drawingElement, user } = setup(); - elementService.findById.mockResolvedValue(drawingElement); + elementService.findById.mockResolvedValueOnce(drawingElement); await uc.checkElementReadPermission(user.id, drawingElement.id); expect(elementService.findById).toHaveBeenCalledWith(drawingElement.id); }); + it('should properly check element permission and not throw', async () => { + const { drawingElement, user } = setup(); + elementService.findById.mockResolvedValueOnce(drawingElement); + + await expect(uc.checkElementReadPermission(user.id, drawingElement.id)).resolves.not.toThrow(); + }); + it('should throw at find element by Id', async () => { const { drawingElement, user } = setup(); - elementService.findById.mockRejectedValue(new Error()); + elementService.findById.mockRejectedValueOnce(new Error()); await expect(uc.checkElementReadPermission(user.id, drawingElement.id)).rejects.toThrow(); }); it('should throw at check permission', async () => { - const { drawingElement, user } = setup(); - authorizationService.hasPermission.mockReturnValueOnce(false); + const { user } = setup(); + const testElementId = 'wrongTestId123'; + authorizationService.checkPermission.mockImplementationOnce(() => { + throw new Error(); + }); - await expect(uc.checkElementReadPermission(user.id, drawingElement.id)).rejects.toThrow(); + await expect(uc.checkElementReadPermission(user.id, testElementId)).rejects.toThrow(); }); }); }); diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts index e17ef2234f5..9e9d416e4c8 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts @@ -122,39 +122,47 @@ describe('WebSocketController (WsAdapter)', () => { describe('when checking cookie', () => { const setup = () => { - const closeClientSpy = jest.spyOn(gateway, 'closeClient'); + const closeClientGatewaySpy = jest.spyOn(gateway, 'closeClient'); + const closeClientWSSpy = jest.spyOn(ws, 'close'); const httpGetCallSpy = jest.spyOn(httpService, 'get'); const parseCookieSpy = jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); return { - closeClientSpy, + closeClientGatewaySpy, + closeClientWSSpy, httpGetCallSpy, parseCookieSpy, }; }; it(`should refuse connection if there is no jwt in cookie`, async () => { - const { closeClientSpy, httpGetCallSpy, parseCookieSpy } = setup(); + const { closeClientGatewaySpy, httpGetCallSpy, parseCookieSpy, closeClientWSSpy } = setup(); const { buffer } = getMessage(); jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); ws = await TestConnection.setupWs(wsUrl, 'TEST'); ws.send(buffer); - expect(closeClientSpy).toHaveBeenCalledWith( + expect(closeClientGatewaySpy).toHaveBeenCalledWith( expect.anything(), WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE ); + expect(closeClientWSSpy).toHaveBeenCalledWith( + WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE + ); - closeClientSpy.mockRestore(); + closeClientGatewaySpy.mockRestore(); + closeClientWSSpy.mockRestore(); httpGetCallSpy.mockRestore(); parseCookieSpy.mockRestore(); + closeClientWSSpy.mockRestore(); ws.close(); }); it(`should refuse connection if jwt is wrong`, async () => { - const { closeClientSpy, httpGetCallSpy, parseCookieSpy } = setup(); + const { closeClientGatewaySpy, httpGetCallSpy, parseCookieSpy, closeClientWSSpy } = setup(); const { buffer } = getMessage(); const error = new Error('unknown error'); @@ -163,14 +171,18 @@ describe('WebSocketController (WsAdapter)', () => { ws = await TestConnection.setupWs(wsUrl, 'TEST'); ws.send(buffer); - expect(closeClientSpy).toHaveBeenCalledWith( + expect(closeClientGatewaySpy).toHaveBeenCalledWith( expect.anything(), WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE ); + expect(closeClientWSSpy).toHaveBeenCalledWith( + WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE + ); httpGetCallSpy.mockRestore(); - closeClientSpy.mockRestore(); + closeClientGatewaySpy.mockRestore(); parseCookieSpy.mockRestore(); ws.close(); }); diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts index 51306923d6a..67c08795722 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts @@ -472,6 +472,27 @@ describe('TldrawWSService', () => { httpService.get.mockReturnValueOnce(of(response)); await expect(service.authorizeConnection(params.drawingName, params.token)).resolves.not.toThrow(); + httpService.get.mockRestore(); + }); + + it('should properly setup REST GET call params', async () => { + const params = { drawingName: 'drawingName', token: 'token' }; + const response: AxiosResponse = axiosResponseFactory.build({ + status: 200, + }); + const expectedUrl = 'http://localhost:3030/api/v3/elements/drawingName/permission'; + const expectedHeaders = { + headers: { + Accept: 'Application/json', + Authorization: `Bearer ${params.token}`, + }, + }; + httpService.get.mockReturnValueOnce(of(response)); + + await service.authorizeConnection(params.drawingName, params.token); + + expect(httpService.get).toHaveBeenCalledWith(expectedUrl, expectedHeaders); + httpService.get.mockRestore(); }); it('should throw error', async () => { @@ -480,6 +501,7 @@ describe('TldrawWSService', () => { httpService.get.mockReturnValueOnce(throwError(() => error)); await expect(service.authorizeConnection(params.drawingName, params.token)).rejects.toThrow(); + httpService.get.mockRestore(); }); }); }); From 621ca4ff8c4859079416c14f667f6767bca27cb6 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 29 Dec 2023 11:43:10 +0100 Subject: [PATCH 16/29] Fix ws close error loggable --- .../tldraw/loggable/websocket-close-error.loggable.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.spec.ts b/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.spec.ts index 3c70f428eb5..ba0b21c9714 100644 --- a/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.spec.ts +++ b/apps/server/src/modules/tldraw/loggable/websocket-close-error.loggable.spec.ts @@ -16,7 +16,7 @@ describe('WebsocketCloseErrorLoggable', () => { const message = loggable.getLogMessage(); - expect(message).toEqual({ message: errorMessage, error }); + expect(message).toEqual({ message: errorMessage, error, type: 'WEBSOCKET_CLOSE_ERROR' }); }); }); }); From f14dce6dc4a3b32456c5f5d26b13e0cbb07a1c1a Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 29 Dec 2023 13:43:26 +0100 Subject: [PATCH 17/29] Fix api test --- .../api-test/tldraw.controller.api.spec.ts | 51 +++++++++---------- .../src/modules/tldraw/controller/index.ts | 1 + .../modules/tldraw/factory/tldraw.factory.ts | 1 + apps/server/src/modules/tldraw/repo/index.ts | 1 + .../src/modules/tldraw/repo/tldraw.repo.ts | 8 ++- .../src/modules/tldraw/service/index.ts | 1 + .../src/shared/domain/entity/all-entities.ts | 2 + 7 files changed, 36 insertions(+), 29 deletions(-) diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.controller.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.controller.api.spec.ts index 2428ff4ad70..6d04d1d5871 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.controller.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.controller.api.spec.ts @@ -1,18 +1,13 @@ import { INestApplication } from '@nestjs/common'; import { EntityManager } from '@mikro-orm/mongodb'; -import { - cardNodeFactory, - cleanupCollections, - columnBoardNodeFactory, - columnNodeFactory, - courseFactory, - TestApiClient, - UserAndAccountTestFactory, -} from '@shared/testing'; +import { cleanupCollections, courseFactory, TestApiClient, UserAndAccountTestFactory } from '@shared/testing'; import { Test, TestingModule } from '@nestjs/testing'; -import { BoardExternalReferenceType } from '@shared/domain/domainobject'; -import { drawingElementNodeFactory } from '@shared/testing/factory/boardnode/drawing-element-node.factory'; -import { TldrawTestModule } from '@modules/tldraw'; +import { ServerTestModule } from '@modules/server'; +import { Logger } from '@src/core/logger'; +import { TldrawService } from '../../service'; +import { TldrawController } from '..'; +import { TldrawRepo } from '../../repo'; +import { tldrawEntityFactory } from '../../factory'; const baseRouteName = '/tldraw-document'; describe('tldraw controller (api)', () => { @@ -22,7 +17,9 @@ describe('tldraw controller (api)', () => { beforeAll(async () => { const module: TestingModule = await Test.createTestingModule({ - imports: [TldrawTestModule], + imports: [ServerTestModule], + controllers: [TldrawController], + providers: [Logger, TldrawService, TldrawRepo], }).compile(); app = module.createNestApplication(); @@ -43,30 +40,30 @@ describe('tldraw controller (api)', () => { const course = courseFactory.build({ teachers: [teacherUser] }); await em.persistAndFlush([teacherAccount, teacherUser, course]); - const columnBoardNode = columnBoardNodeFactory.buildWithId({ - context: { id: course.id, type: BoardExternalReferenceType.Course }, - }); + const drawingItemData = tldrawEntityFactory.build(); - const columnNode = columnNodeFactory.buildWithId({ parent: columnBoardNode }); - - const cardNode = cardNodeFactory.buildWithId({ parent: columnNode }); - - const drawingItemNode = drawingElementNodeFactory.buildWithId({ parent: cardNode }); - - await em.persistAndFlush([columnBoardNode, columnNode, cardNode, drawingItemNode]); + await em.persistAndFlush([drawingItemData]); em.clear(); const loggedInClient = await testApiClient.login(teacherAccount); - return { loggedInClient, teacherUser, columnBoardNode, columnNode, cardNode, drawingItemNode }; + return { loggedInClient, teacherUser, drawingItemData }; }; it('should return status 200 for delete', async () => { - const { loggedInClient, drawingItemNode } = await setup(); + const { loggedInClient, drawingItemData } = await setup(); + + const response = await loggedInClient.delete(`${drawingItemData.docName}`); + + expect(response.status).toEqual(204); + }); + + it('should return status 404 for delete with wrong id', async () => { + const { loggedInClient } = await setup(); - const response = await loggedInClient.delete(`${drawingItemNode.id}`); + const response = await loggedInClient.delete(`testID123`); - expect(response.status).toEqual(200); + expect(response.status).toEqual(404); }); }); }); diff --git a/apps/server/src/modules/tldraw/controller/index.ts b/apps/server/src/modules/tldraw/controller/index.ts index 0b0cf7d103b..38a96a42a75 100644 --- a/apps/server/src/modules/tldraw/controller/index.ts +++ b/apps/server/src/modules/tldraw/controller/index.ts @@ -1 +1,2 @@ export * from './tldraw.ws'; +export * from './tldraw.controller'; diff --git a/apps/server/src/modules/tldraw/factory/tldraw.factory.ts b/apps/server/src/modules/tldraw/factory/tldraw.factory.ts index 3cb63e9418b..c6e80ec2329 100644 --- a/apps/server/src/modules/tldraw/factory/tldraw.factory.ts +++ b/apps/server/src/modules/tldraw/factory/tldraw.factory.ts @@ -6,6 +6,7 @@ export const tldrawEntityFactory = BaseFactory.define { return { _id: 'test-id', + id: 'test-id', docName: 'test-name', value: 'test-value', version: `test-version-${sequence}`, diff --git a/apps/server/src/modules/tldraw/repo/index.ts b/apps/server/src/modules/tldraw/repo/index.ts index 0c1ae29e62f..3cc9ad02bf7 100644 --- a/apps/server/src/modules/tldraw/repo/index.ts +++ b/apps/server/src/modules/tldraw/repo/index.ts @@ -1 +1,2 @@ export * from './tldraw-board.repo'; +export * from './tldraw.repo'; diff --git a/apps/server/src/modules/tldraw/repo/tldraw.repo.ts b/apps/server/src/modules/tldraw/repo/tldraw.repo.ts index d826b2876ff..d8eb4330bd2 100644 --- a/apps/server/src/modules/tldraw/repo/tldraw.repo.ts +++ b/apps/server/src/modules/tldraw/repo/tldraw.repo.ts @@ -1,5 +1,5 @@ import { EntityManager } from '@mikro-orm/mongodb'; -import { Injectable } from '@nestjs/common'; +import { Injectable, NotFoundException } from '@nestjs/common'; import { TldrawDrawing } from '../entities'; @Injectable() @@ -11,7 +11,11 @@ export class TldrawRepo { } async findByDocName(docName: string): Promise { - return this._em.find(TldrawDrawing, { docName }); + const domainObject = await this._em.find(TldrawDrawing, { docName }); + if (domainObject.length === 0) { + throw new NotFoundException(`There is no '${docName}' for this docName`); + } + return domainObject; } async delete(entity: TldrawDrawing | TldrawDrawing[]): Promise { diff --git a/apps/server/src/modules/tldraw/service/index.ts b/apps/server/src/modules/tldraw/service/index.ts index a056b2ece10..2bc9f981432 100644 --- a/apps/server/src/modules/tldraw/service/index.ts +++ b/apps/server/src/modules/tldraw/service/index.ts @@ -1 +1,2 @@ export * from './tldraw.ws.service'; +export * from './tldraw.service'; diff --git a/apps/server/src/shared/domain/entity/all-entities.ts b/apps/server/src/shared/domain/entity/all-entities.ts index 94d38d08346..7163ffb9f12 100644 --- a/apps/server/src/shared/domain/entity/all-entities.ts +++ b/apps/server/src/shared/domain/entity/all-entities.ts @@ -9,6 +9,7 @@ import { ExternalToolEntity } from '@modules/tool/external-tool/entity'; import { SchoolExternalToolEntity } from '@modules/tool/school-external-tool/entity'; import { DeletionLogEntity, DeletionRequestEntity } from '@src/modules/deletion/entity'; import { RocketChatUserEntity } from '@src/modules/rocketchat-user/entity'; +import { TldrawDrawing } from '@modules/tldraw/entities'; import { Account } from './account.entity'; import { BoardNode, @@ -112,4 +113,5 @@ export const ALL_ENTITIES = [ VideoConference, GroupEntity, RegistrationPinEntity, + TldrawDrawing, ]; From 96d7e619ff9ddf16833f8fdd93dd1f4be4d87cb2 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 29 Dec 2023 14:14:46 +0100 Subject: [PATCH 18/29] Fix repo spec ts after api test changes --- .../src/modules/tldraw/repo/tldraw.repo.spec.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/apps/server/src/modules/tldraw/repo/tldraw.repo.spec.ts b/apps/server/src/modules/tldraw/repo/tldraw.repo.spec.ts index 9e6f5eabb14..5c075669431 100644 --- a/apps/server/src/modules/tldraw/repo/tldraw.repo.spec.ts +++ b/apps/server/src/modules/tldraw/repo/tldraw.repo.spec.ts @@ -1,9 +1,10 @@ import { EntityManager } from '@mikro-orm/mongodb'; import { Test, TestingModule } from '@nestjs/testing'; import { cleanupCollections } from '@shared/testing'; -import { tldrawEntityFactory } from '@src/modules/tldraw/factory'; -import { TldrawDrawing } from '@src/modules/tldraw/entities'; import { MongoMemoryDatabaseModule } from '@infra/database'; +import { NotFoundException } from '@nestjs/common'; +import { tldrawEntityFactory } from '../factory'; +import { TldrawDrawing } from '../entities'; import { TldrawRepo } from './tldraw.repo'; describe(TldrawRepo.name, () => { @@ -68,9 +69,8 @@ describe(TldrawRepo.name, () => { expect(result[0]._id).toEqual(drawing._id); }); - it('should not find any record giving wrong docName', async () => { - const result = await repo.findByDocName('invalid-name'); - expect(result.length).toEqual(0); + it('should throw NotFoundException for wrong docName', async () => { + await expect(repo.findByDocName('invalid-name')).rejects.toThrow(NotFoundException); }); }); }); @@ -84,8 +84,8 @@ describe(TldrawRepo.name, () => { const results = await repo.findByDocName(drawing.docName); await repo.delete(results); - const emptyResults = await repo.findByDocName(drawing.docName); - expect(emptyResults.length).toEqual(0); + expect(results.length).not.toEqual(0); + await expect(repo.findByDocName(drawing.docName)).rejects.toThrow(NotFoundException); }); }); }); From 3bc32db86894434d094263d837c35c2a6fcad9eb Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 29 Dec 2023 15:49:00 +0100 Subject: [PATCH 19/29] Fix tldraw.ws.api.spec.ts tests --- .../controller/api-test/tldraw.ws.api.spec.ts | 26 ++++--------------- .../modules/tldraw/controller/tldraw.ws.ts | 4 +-- .../tldraw/service/tldraw.ws.service.ts | 2 +- .../src/modules/tldraw/types/ws-close-enum.ts | 3 +-- 4 files changed, 9 insertions(+), 26 deletions(-) diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts index 9e9d416e4c8..2ac5b652a84 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts @@ -123,62 +123,46 @@ describe('WebSocketController (WsAdapter)', () => { describe('when checking cookie', () => { const setup = () => { const closeClientGatewaySpy = jest.spyOn(gateway, 'closeClient'); - const closeClientWSSpy = jest.spyOn(ws, 'close'); const httpGetCallSpy = jest.spyOn(httpService, 'get'); const parseCookieSpy = jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); return { closeClientGatewaySpy, - closeClientWSSpy, httpGetCallSpy, parseCookieSpy, }; }; it(`should refuse connection if there is no jwt in cookie`, async () => { - const { closeClientGatewaySpy, httpGetCallSpy, parseCookieSpy, closeClientWSSpy } = setup(); - const { buffer } = getMessage(); + const { closeClientGatewaySpy, httpGetCallSpy, parseCookieSpy } = setup(); jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); ws = await TestConnection.setupWs(wsUrl, 'TEST'); - ws.send(buffer); expect(closeClientGatewaySpy).toHaveBeenCalledWith( expect.anything(), WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE - ); - expect(closeClientWSSpy).toHaveBeenCalledWith( - WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE ); closeClientGatewaySpy.mockRestore(); - closeClientWSSpy.mockRestore(); httpGetCallSpy.mockRestore(); parseCookieSpy.mockRestore(); - closeClientWSSpy.mockRestore(); ws.close(); }); it(`should refuse connection if jwt is wrong`, async () => { - const { closeClientGatewaySpy, httpGetCallSpy, parseCookieSpy, closeClientWSSpy } = setup(); - const { buffer } = getMessage(); + const { closeClientGatewaySpy, httpGetCallSpy, parseCookieSpy } = setup(); const error = new Error('unknown error'); httpGetCallSpy.mockReturnValueOnce(throwError(() => error)); jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({ jwt: 'mock-wrong-jwt' }); ws = await TestConnection.setupWs(wsUrl, 'TEST'); - ws.send(buffer); expect(closeClientGatewaySpy).toHaveBeenCalledWith( expect.anything(), WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE - ); - expect(closeClientWSSpy).toHaveBeenCalledWith( - WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE ); httpGetCallSpy.mockRestore(); @@ -237,7 +221,7 @@ describe('WebSocketController (WsAdapter)', () => { expect(closeClientSpy).toHaveBeenCalledWith( expect.anything(), WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE ); closeClientSpy.mockRestore(); diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index 83155fca11d..1238aa4697d 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -31,12 +31,12 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { this.closeClient( client, WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE ); this.logger.warning( new WebsocketCloseErrorLoggable( err as Error, - `(${WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE}) ${WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE}` + `(${WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE}) ${WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE}` ) ); } diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts index d1aa18f6cc8..df24002c79f 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts @@ -211,7 +211,7 @@ export class TldrawWsService { } public async authorizeConnection(drawingName: string, token: string) { - if (token.length === 0) { + if (!token) { throw new UnauthorizedException('Token was not given'); } const headers = { diff --git a/apps/server/src/modules/tldraw/types/ws-close-enum.ts b/apps/server/src/modules/tldraw/types/ws-close-enum.ts index 6a3b394931f..90bef90c998 100644 --- a/apps/server/src/modules/tldraw/types/ws-close-enum.ts +++ b/apps/server/src/modules/tldraw/types/ws-close-enum.ts @@ -5,7 +5,6 @@ export enum WsCloseCodeEnum { } export enum WsCloseMessageEnum { WS_CLIENT_BAD_REQUEST_MESSAGE = 'Document name is mandatory in url or Tldraw Tool is turned off.', - WS_CLIENT_UNAUTHORISED_CONNECTION_LACK_PERMISSION_MESSAGE = "Unauthorised connection - you don't have permission to this drawing.", - WS_CLIENT_UNAUTHORISED_CONNECTION_JWT_NOT_PROVIDED_MESSAGE = 'Unauthorised connection - authorization token was not provided.', + WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE = "Unauthorised connection - you don't have permission to this drawing.", WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE = 'Unable to establish websocket connection.', } From df474cea9abedd33c94196a29fe9ecfef77e8c53 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 29 Dec 2023 16:13:41 +0100 Subject: [PATCH 20/29] Fix tldraw.service.spec.ts tests --- .../src/modules/tldraw/service/tldraw.service.spec.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/server/src/modules/tldraw/service/tldraw.service.spec.ts b/apps/server/src/modules/tldraw/service/tldraw.service.spec.ts index cc3a317ec3c..546ab739bb0 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.service.spec.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.service.spec.ts @@ -2,6 +2,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { EntityManager } from '@mikro-orm/mongodb'; import { MongoMemoryDatabaseModule } from '@infra/database'; import { cleanupCollections } from '@shared/testing'; +import { NotFoundException } from '@nestjs/common'; import { TldrawDrawing } from '../entities'; import { tldrawEntityFactory } from '../factory'; import { TldrawRepo } from '../repo/tldraw.repo'; @@ -44,9 +45,12 @@ describe(TldrawService.name, () => { expect(result.length).toEqual(1); await service.deleteByDocName(drawing.docName); - const emptyResult = await repo.findByDocName(drawing.docName); - expect(emptyResult.length).toEqual(0); + await expect(repo.findByDocName(drawing.docName)).rejects.toThrow(NotFoundException); + }); + + it('should throw when cannot find drawing', async () => { + await expect(service.deleteByDocName('nonExistingName')).rejects.toThrow(NotFoundException); }); }); }); From 40e1bd0ba03eea2161324c9cbba5fd2bf09bad55 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 29 Dec 2023 16:49:45 +0100 Subject: [PATCH 21/29] Fix test for failed established connection --- .../controller/api-test/tldraw.ws.api.spec.ts | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts index 2ac5b652a84..b75187beb9f 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts @@ -251,5 +251,34 @@ describe('WebSocketController (WsAdapter)', () => { httpGetCallSpy.mockRestore(); ws.close(); }); + + it(`should close after throw at setup connection`, async () => { + const { parseCookieSpy, setupConnectionSpy, closeClientSpy } = setup(); + const { buffer } = getMessage(); + + parseCookieSpy.mockReturnValueOnce({ jwt: 'jwt-mocked' }); + const httpGetCallSpy = jest + .spyOn(wsService, 'authorizeConnection') + .mockImplementationOnce(() => Promise.resolve()); + setupConnectionSpy.mockImplementationOnce(() => { + throw new Error('unknown error'); + }); + + ws = await TestConnection.setupWs(wsUrl, 'TEST'); + ws.send(buffer); + + expect(setupConnectionSpy).toHaveBeenCalledWith(expect.anything(), 'TEST'); + expect(closeClientSpy).toHaveBeenCalledWith( + expect.anything(), + WsCloseCodeEnum.WS_CLIENT_ESTABLISHING_CONNECTION_CODE, + WsCloseMessageEnum.WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE + ); + + closeClientSpy.mockRestore(); + setupConnectionSpy.mockRestore(); + parseCookieSpy.mockRestore(); + httpGetCallSpy.mockRestore(); + ws.close(); + }); }); }); From b05a2983dac350732347499c8987083cf2b92c40 Mon Sep 17 00:00:00 2001 From: blazejpass <118356546+blazejpass@users.noreply.github.com> Date: Wed, 3 Jan 2024 08:26:00 +0100 Subject: [PATCH 22/29] Update element.controller.ts Co-authored-by: virgilchiriac <17074330+virgilchiriac@users.noreply.github.com> --- apps/server/src/modules/board/controller/element.controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/board/controller/element.controller.ts b/apps/server/src/modules/board/controller/element.controller.ts index 82cb8d63c58..2f766ea1d5b 100644 --- a/apps/server/src/modules/board/controller/element.controller.ts +++ b/apps/server/src/modules/board/controller/element.controller.ts @@ -143,7 +143,7 @@ export class ElementController { return response; } - @ApiOperation({ summary: 'Check if user has permission for any course element.' }) + @ApiOperation({ summary: 'Check if user has read permission for any board element.' }) @ApiResponse({ status: 200 }) @ApiResponse({ status: 400, type: ApiValidationError }) @ApiResponse({ status: 403, type: ForbiddenException }) From 5a8400d61cf7dd3fad3e86b5ce028b5b84fad69b Mon Sep 17 00:00:00 2001 From: blazejpass Date: Wed, 3 Jan 2024 12:22:57 +0100 Subject: [PATCH 23/29] Improve tests according to comments --- .../controller/api-test/tldraw.ws.api.spec.ts | 76 +++++++------------ .../modules/tldraw/controller/tldraw.ws.ts | 38 +++------- .../tldraw/service/tldraw.ws.service.spec.ts | 9 ++- .../modules/tldraw/testing/test-connection.ts | 6 +- 4 files changed, 50 insertions(+), 79 deletions(-) diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts index b75187beb9f..3dca552a65b 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts @@ -122,141 +122,121 @@ describe('WebSocketController (WsAdapter)', () => { describe('when checking cookie', () => { const setup = () => { - const closeClientGatewaySpy = jest.spyOn(gateway, 'closeClient'); const httpGetCallSpy = jest.spyOn(httpService, 'get'); - const parseCookieSpy = jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); + const wsCloseSpy = jest.spyOn(WebSocket.prototype, 'close'); return { - closeClientGatewaySpy, httpGetCallSpy, - parseCookieSpy, + wsCloseSpy, }; }; it(`should refuse connection if there is no jwt in cookie`, async () => { - const { closeClientGatewaySpy, httpGetCallSpy, parseCookieSpy } = setup(); + const { httpGetCallSpy, wsCloseSpy } = setup(); - jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({}); - ws = await TestConnection.setupWs(wsUrl, 'TEST'); + ws = await TestConnection.setupWs(wsUrl, 'TEST', {}); - expect(closeClientGatewaySpy).toHaveBeenCalledWith( - expect.anything(), + expect(wsCloseSpy).toHaveBeenCalledWith( WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE ); - closeClientGatewaySpy.mockRestore(); httpGetCallSpy.mockRestore(); - parseCookieSpy.mockRestore(); + wsCloseSpy.mockRestore(); ws.close(); }); it(`should refuse connection if jwt is wrong`, async () => { - const { closeClientGatewaySpy, httpGetCallSpy, parseCookieSpy } = setup(); + const { wsCloseSpy, httpGetCallSpy } = setup(); const error = new Error('unknown error'); httpGetCallSpy.mockReturnValueOnce(throwError(() => error)); - jest.spyOn(gateway, 'parseCookiesFromHeader').mockReturnValueOnce({ jwt: 'mock-wrong-jwt' }); - ws = await TestConnection.setupWs(wsUrl, 'TEST'); + ws = await TestConnection.setupWs(wsUrl, 'TEST', { jwt: 'mock-wrong-jwt' }); - expect(closeClientGatewaySpy).toHaveBeenCalledWith( - expect.anything(), + expect(wsCloseSpy).toHaveBeenCalledWith( WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE ); httpGetCallSpy.mockRestore(); - closeClientGatewaySpy.mockRestore(); - parseCookieSpy.mockRestore(); + wsCloseSpy.mockRestore(); ws.close(); }); }); describe('when checking docName and cookie', () => { const setup = () => { - const parseCookieSpy = jest.spyOn(gateway, 'parseCookiesFromHeader'); - const closeClientSpy = jest.spyOn(gateway, 'closeClient'); const setupConnectionSpy = jest.spyOn(wsService, 'setupWSConnection'); + const wsCloseSpy = jest.spyOn(WebSocket.prototype, 'close'); return { - parseCookieSpy, setupConnectionSpy, - closeClientSpy, + wsCloseSpy, }; }; it(`should close for existing cookie and not existing docName`, async () => { - const { parseCookieSpy, setupConnectionSpy, closeClientSpy } = setup(); + const { setupConnectionSpy, wsCloseSpy } = setup(); const { buffer } = getMessage(); - parseCookieSpy.mockReturnValueOnce({ jwt: 'jwt-mocked' }); - - ws = await TestConnection.setupWs(wsUrl); + ws = await TestConnection.setupWs(wsUrl, '', { jwt: 'jwt-mocked' }); ws.send(buffer); - expect(closeClientSpy).toHaveBeenCalledWith( - expect.anything(), + expect(wsCloseSpy).toHaveBeenCalledWith( WsCloseCodeEnum.WS_CLIENT_BAD_REQUEST_CODE, WsCloseMessageEnum.WS_CLIENT_BAD_REQUEST_MESSAGE ); - closeClientSpy.mockRestore(); + wsCloseSpy.mockRestore(); setupConnectionSpy.mockRestore(); - parseCookieSpy.mockRestore(); ws.close(); }); it(`should close for not authorizing connection`, async () => { - const { parseCookieSpy, setupConnectionSpy, closeClientSpy } = setup(); + const { setupConnectionSpy, wsCloseSpy } = setup(); const { buffer } = getMessage(); const httpGetCallSpy = jest.spyOn(httpService, 'get'); const error = new Error('unknown error'); httpGetCallSpy.mockReturnValueOnce(throwError(() => error)); - parseCookieSpy.mockReturnValueOnce({ jwt: 'jwt-mocked' }); - ws = await TestConnection.setupWs(wsUrl, 'TEST'); + ws = await TestConnection.setupWs(wsUrl, 'TEST', { jwt: 'jwt-mocked' }); ws.send(buffer); - expect(closeClientSpy).toHaveBeenCalledWith( - expect.anything(), + expect(wsCloseSpy).toHaveBeenCalledWith( WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE ); - closeClientSpy.mockRestore(); + wsCloseSpy.mockRestore(); setupConnectionSpy.mockRestore(); - parseCookieSpy.mockRestore(); httpGetCallSpy.mockRestore(); ws.close(); }); it(`should setup connection for proper data`, async () => { - const { parseCookieSpy, setupConnectionSpy, closeClientSpy } = setup(); + const { setupConnectionSpy, wsCloseSpy } = setup(); const { buffer } = getMessage(); - parseCookieSpy.mockReturnValueOnce({ jwt: 'jwt-mocked' }); const httpGetCallSpy = jest .spyOn(wsService, 'authorizeConnection') .mockImplementationOnce(() => Promise.resolve()); - ws = await TestConnection.setupWs(wsUrl, 'TEST'); + ws = await TestConnection.setupWs(wsUrl, 'TEST', { jwt: 'jwt-mocked' }); ws.send(buffer); expect(setupConnectionSpy).toHaveBeenCalledWith(expect.anything(), 'TEST'); - closeClientSpy.mockRestore(); + wsCloseSpy.mockRestore(); setupConnectionSpy.mockRestore(); - parseCookieSpy.mockRestore(); httpGetCallSpy.mockRestore(); ws.close(); }); it(`should close after throw at setup connection`, async () => { - const { parseCookieSpy, setupConnectionSpy, closeClientSpy } = setup(); + const { setupConnectionSpy, wsCloseSpy } = setup(); const { buffer } = getMessage(); - parseCookieSpy.mockReturnValueOnce({ jwt: 'jwt-mocked' }); const httpGetCallSpy = jest .spyOn(wsService, 'authorizeConnection') .mockImplementationOnce(() => Promise.resolve()); @@ -264,19 +244,17 @@ describe('WebSocketController (WsAdapter)', () => { throw new Error('unknown error'); }); - ws = await TestConnection.setupWs(wsUrl, 'TEST'); + ws = await TestConnection.setupWs(wsUrl, 'TEST', { jwt: 'jwt-mocked' }); ws.send(buffer); expect(setupConnectionSpy).toHaveBeenCalledWith(expect.anything(), 'TEST'); - expect(closeClientSpy).toHaveBeenCalledWith( - expect.anything(), + expect(wsCloseSpy).toHaveBeenCalledWith( WsCloseCodeEnum.WS_CLIENT_ESTABLISHING_CONNECTION_CODE, WsCloseMessageEnum.WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE ); - closeClientSpy.mockRestore(); + wsCloseSpy.mockRestore(); setupConnectionSpy.mockRestore(); - parseCookieSpy.mockRestore(); httpGetCallSpy.mockRestore(); ws.close(); }); diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index 1238aa4697d..36f18216e49 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -28,44 +28,29 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { try { await this.tldrawWsService.authorizeConnection(docName, cookies?.jwt); } catch (err) { - this.closeClient( + this.closeClientAndLogError( client, WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE - ); - this.logger.warning( - new WebsocketCloseErrorLoggable( - err as Error, - `(${WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE}) ${WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE}` - ) + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE, + err as Error ); } try { this.tldrawWsService.setupWSConnection(client, docName); } catch (err) { - this.closeClient( + this.closeClientAndLogError( client, WsCloseCodeEnum.WS_CLIENT_ESTABLISHING_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE - ); - this.logger.warning( - new WebsocketCloseErrorLoggable( - err as Error, - `(${WsCloseCodeEnum.WS_CLIENT_ESTABLISHING_CONNECTION_CODE}) ${WsCloseMessageEnum.WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE}` - ) + WsCloseMessageEnum.WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE, + err as Error ); } } else { - this.closeClient( + this.closeClientAndLogError( client, WsCloseCodeEnum.WS_CLIENT_BAD_REQUEST_CODE, - WsCloseMessageEnum.WS_CLIENT_BAD_REQUEST_MESSAGE - ); - this.logger.warning( - new WebsocketCloseErrorLoggable( - new BadRequestException(), - `(${WsCloseCodeEnum.WS_CLIENT_BAD_REQUEST_CODE}) ${WsCloseMessageEnum.WS_CLIENT_BAD_REQUEST_MESSAGE}` - ) + WsCloseMessageEnum.WS_CLIENT_BAD_REQUEST_MESSAGE, + new BadRequestException() ); } } @@ -88,12 +73,13 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { return urlStripped; } - public parseCookiesFromHeader(request: Request): { [p: string]: string } { + private parseCookiesFromHeader(request: Request): { [p: string]: string } { const parsedCookies: { [p: string]: string } = cookie.parse(request.headers.cookie || ''); return parsedCookies; } - public closeClient(client: WebSocket, code: WsCloseCodeEnum, data: string): void { + private closeClientAndLogError(client: WebSocket, code: WsCloseCodeEnum, data: string, err: Error): void { client.close(code, data); + this.logger.warning(new WebsocketCloseErrorLoggable(err, `(${code}) ${data}`)); } } diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts index 67c08795722..4ce35b3bc3c 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.spec.ts @@ -495,7 +495,7 @@ describe('TldrawWSService', () => { httpService.get.mockRestore(); }); - it('should throw error', async () => { + it('should throw error for http response', async () => { const params = { drawingName: 'drawingName', token: 'token' }; const error = new Error('unknown error'); httpService.get.mockReturnValueOnce(throwError(() => error)); @@ -503,5 +503,12 @@ describe('TldrawWSService', () => { await expect(service.authorizeConnection(params.drawingName, params.token)).rejects.toThrow(); httpService.get.mockRestore(); }); + + it('should throw error for lack of token', async () => { + const params = { drawingName: 'drawingName', token: 'token' }; + + await expect(service.authorizeConnection(params.drawingName, '')).rejects.toThrow(); + httpService.get.mockRestore(); + }); }); }); diff --git a/apps/server/src/modules/tldraw/testing/test-connection.ts b/apps/server/src/modules/tldraw/testing/test-connection.ts index 638c219ea18..4231acbb286 100644 --- a/apps/server/src/modules/tldraw/testing/test-connection.ts +++ b/apps/server/src/modules/tldraw/testing/test-connection.ts @@ -6,12 +6,12 @@ export class TestConnection { return wsUrl; }; - public static setupWs = async (wsUrl: string, docName?: string): Promise => { + public static setupWs = async (wsUrl: string, docName?: string, headers?: object): Promise => { let ws: WebSocket; if (docName) { - ws = new WebSocket(`${wsUrl}/${docName}`); + ws = new WebSocket(`${wsUrl}/${docName}`, headers); } else { - ws = new WebSocket(`${wsUrl}`); + ws = new WebSocket(`${wsUrl}`, headers); } await new Promise((resolve) => { ws.on('open', resolve); From 6e951ea8fd18e6808dafbcfbd94daa2781367096 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Wed, 3 Jan 2024 12:33:37 +0100 Subject: [PATCH 24/29] Move extraction of cookies after docName and feature flag check --- apps/server/src/modules/tldraw/controller/tldraw.ws.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index 36f18216e49..e84d6761541 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -23,8 +23,8 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { async handleConnection(client: WebSocket, request: Request): Promise { const docName = this.getDocNameFromRequest(request); - const cookies = this.parseCookiesFromHeader(request); if (docName.length > 0 && this.configService.get('FEATURE_TLDRAW_ENABLED')) { + const cookies = this.parseCookiesFromHeader(request); try { await this.tldrawWsService.authorizeConnection(docName, cookies?.jwt); } catch (err) { From 669554f855412fce6c79776326349abdd7e62653 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Wed, 3 Jan 2024 12:50:04 +0100 Subject: [PATCH 25/29] Add sentence to 4500 ws close connection --- apps/server/src/modules/tldraw/types/ws-close-enum.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/tldraw/types/ws-close-enum.ts b/apps/server/src/modules/tldraw/types/ws-close-enum.ts index 90bef90c998..e905eea20be 100644 --- a/apps/server/src/modules/tldraw/types/ws-close-enum.ts +++ b/apps/server/src/modules/tldraw/types/ws-close-enum.ts @@ -6,5 +6,5 @@ export enum WsCloseCodeEnum { export enum WsCloseMessageEnum { WS_CLIENT_BAD_REQUEST_MESSAGE = 'Document name is mandatory in url or Tldraw Tool is turned off.', WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE = "Unauthorised connection - you don't have permission to this drawing.", - WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE = 'Unable to establish websocket connection.', + WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE = 'Unable to establish websocket connection. Try again later.', } From b7b74dc32fce580d59929d0b656adee1898fed15 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Thu, 4 Jan 2024 12:45:34 +0100 Subject: [PATCH 26/29] Change api-host, add return to try catch --- apps/server/src/modules/tldraw/controller/tldraw.ws.ts | 1 + apps/server/src/modules/tldraw/service/tldraw.ws.service.ts | 2 +- config/default.schema.json | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index e84d6761541..b32fb603f3e 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -34,6 +34,7 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE, err as Error ); + return; } try { this.tldrawWsService.setupWSConnection(client, docName); diff --git a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts index df24002c79f..79943a8f1b1 100644 --- a/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts +++ b/apps/server/src/modules/tldraw/service/tldraw.ws.service.ts @@ -220,7 +220,7 @@ export class TldrawWsService { }; await firstValueFrom( - this.httpService.get(`${this.configService.get('API_HOST')}/api/v3/elements/${drawingName}/permission`, { + this.httpService.get(`${this.configService.get('API_HOST')}/v3/elements/${drawingName}/permission`, { headers, }) ); diff --git a/config/default.schema.json b/config/default.schema.json index c67e6f73aef..6d0ee628412 100644 --- a/config/default.schema.json +++ b/config/default.schema.json @@ -534,7 +534,7 @@ "API_HOST": { "type": "string", "format": "uri", - "default": "http://localhost:3030", + "default": "http://localhost:3030/api", "pattern": ".*(? Date: Thu, 4 Jan 2024 18:21:50 +0100 Subject: [PATCH 27/29] Add 404 for drawing authorization proccess --- .../controller/api-test/tldraw.ws.api.spec.ts | 31 +++++++++++++++---- .../modules/tldraw/controller/tldraw.ws.ts | 23 +++++++++----- .../src/modules/tldraw/types/ws-close-enum.ts | 2 ++ 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts index 3dca552a65b..4016263dcc5 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts @@ -2,7 +2,7 @@ import { WsAdapter } from '@nestjs/platform-ws'; import { Test } from '@nestjs/testing'; import WebSocket from 'ws'; import { TextEncoder } from 'util'; -import { INestApplication } from '@nestjs/common'; +import { INestApplication, NotFoundException } from '@nestjs/common'; import { throwError } from 'rxjs'; import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { HttpService } from '@nestjs/axios'; @@ -151,7 +151,7 @@ describe('WebSocketController (WsAdapter)', () => { const error = new Error('unknown error'); httpGetCallSpy.mockReturnValueOnce(throwError(() => error)); - ws = await TestConnection.setupWs(wsUrl, 'TEST', { jwt: 'mock-wrong-jwt' }); + ws = await TestConnection.setupWs(wsUrl, 'TEST', { headers: { cookie: { jwt: 'jwt-mocked' } } }); expect(wsCloseSpy).toHaveBeenCalledWith( WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, @@ -179,7 +179,7 @@ describe('WebSocketController (WsAdapter)', () => { const { setupConnectionSpy, wsCloseSpy } = setup(); const { buffer } = getMessage(); - ws = await TestConnection.setupWs(wsUrl, '', { jwt: 'jwt-mocked' }); + ws = await TestConnection.setupWs(wsUrl, '', { headers: { cookie: { jwt: 'jwt-mocked' } } }); ws.send(buffer); expect(wsCloseSpy).toHaveBeenCalledWith( @@ -192,6 +192,25 @@ describe('WebSocketController (WsAdapter)', () => { ws.close(); }); + it(`should close for not existing docName resource`, async () => { + const { setupConnectionSpy, wsCloseSpy } = setup(); + const authorizeConnectionSpy = jest.spyOn(wsService, 'authorizeConnection'); + authorizeConnectionSpy.mockImplementationOnce(() => { + throw new NotFoundException('Resource not found'); + }); + ws = await TestConnection.setupWs(wsUrl, 'GLOBAL', { headers: { cookie: { jwt: 'jwt-mocked' } } }); + + expect(wsCloseSpy).toHaveBeenCalledWith( + WsCloseCodeEnum.WS_CLIENT_NOT_FOUND_CODE, + WsCloseMessageEnum.WS_CLIENT_NOT_FOUND_MESSAGE + ); + + authorizeConnectionSpy.mockRestore(); + wsCloseSpy.mockRestore(); + setupConnectionSpy.mockRestore(); + ws.close(); + }); + it(`should close for not authorizing connection`, async () => { const { setupConnectionSpy, wsCloseSpy } = setup(); const { buffer } = getMessage(); @@ -200,7 +219,7 @@ describe('WebSocketController (WsAdapter)', () => { const error = new Error('unknown error'); httpGetCallSpy.mockReturnValueOnce(throwError(() => error)); - ws = await TestConnection.setupWs(wsUrl, 'TEST', { jwt: 'jwt-mocked' }); + ws = await TestConnection.setupWs(wsUrl, 'TEST', { headers: { cookie: { jwt: 'jwt-mocked' } } }); ws.send(buffer); expect(wsCloseSpy).toHaveBeenCalledWith( @@ -222,7 +241,7 @@ describe('WebSocketController (WsAdapter)', () => { .spyOn(wsService, 'authorizeConnection') .mockImplementationOnce(() => Promise.resolve()); - ws = await TestConnection.setupWs(wsUrl, 'TEST', { jwt: 'jwt-mocked' }); + ws = await TestConnection.setupWs(wsUrl, 'TEST', { headers: { cookie: { jwt: 'jwt-mocked' } } }); ws.send(buffer); expect(setupConnectionSpy).toHaveBeenCalledWith(expect.anything(), 'TEST'); @@ -244,7 +263,7 @@ describe('WebSocketController (WsAdapter)', () => { throw new Error('unknown error'); }); - ws = await TestConnection.setupWs(wsUrl, 'TEST', { jwt: 'jwt-mocked' }); + ws = await TestConnection.setupWs(wsUrl, 'TEST', { headers: { cookie: { jwt: 'jwt-mocked' } } }); ws.send(buffer); expect(setupConnectionSpy).toHaveBeenCalledWith(expect.anything(), 'TEST'); diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index b32fb603f3e..414948268ac 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -3,7 +3,7 @@ import { Server, WebSocket } from 'ws'; import { Request } from 'express'; import { ConfigService } from '@nestjs/config'; import cookie from 'cookie'; -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; import { Logger } from '@src/core/logger'; import { WebsocketCloseErrorLoggable } from '../loggable/websocket-close-error.loggable'; import { TldrawConfig, SOCKET_PORT } from '../config'; @@ -28,12 +28,21 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { try { await this.tldrawWsService.authorizeConnection(docName, cookies?.jwt); } catch (err) { - this.closeClientAndLogError( - client, - WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, - WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE, - err as Error - ); + if (err instanceof NotFoundException) { + this.closeClientAndLogError( + client, + WsCloseCodeEnum.WS_CLIENT_NOT_FOUND_CODE, + WsCloseMessageEnum.WS_CLIENT_NOT_FOUND_MESSAGE, + err as Error + ); + } else { + this.closeClientAndLogError( + client, + WsCloseCodeEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_CODE, + WsCloseMessageEnum.WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE, + err as Error + ); + } return; } try { diff --git a/apps/server/src/modules/tldraw/types/ws-close-enum.ts b/apps/server/src/modules/tldraw/types/ws-close-enum.ts index e905eea20be..0cbf8021e84 100644 --- a/apps/server/src/modules/tldraw/types/ws-close-enum.ts +++ b/apps/server/src/modules/tldraw/types/ws-close-enum.ts @@ -1,10 +1,12 @@ export enum WsCloseCodeEnum { WS_CLIENT_BAD_REQUEST_CODE = 4400, WS_CLIENT_UNAUTHORISED_CONNECTION_CODE = 4401, + WS_CLIENT_NOT_FOUND_CODE = 4404, WS_CLIENT_ESTABLISHING_CONNECTION_CODE = 4500, } export enum WsCloseMessageEnum { WS_CLIENT_BAD_REQUEST_MESSAGE = 'Document name is mandatory in url or Tldraw Tool is turned off.', WS_CLIENT_UNAUTHORISED_CONNECTION_MESSAGE = "Unauthorised connection - you don't have permission to this drawing.", + WS_CLIENT_NOT_FOUND_MESSAGE = 'Drawing not found.', WS_CLIENT_ESTABLISHING_CONNECTION_MESSAGE = 'Unable to establish websocket connection. Try again later.', } From b6965770396ccf5e98fbe50fb8ced3a3856041ad Mon Sep 17 00:00:00 2001 From: blazejpass Date: Fri, 5 Jan 2024 10:21:39 +0100 Subject: [PATCH 28/29] Fix 404 for drawing authorization proccess --- .../tldraw/controller/api-test/tldraw.ws.api.spec.ts | 12 ++++++++++-- .../src/modules/tldraw/controller/tldraw.ws.ts | 5 +++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts index 4016263dcc5..6a0a3cc4fc3 100644 --- a/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts +++ b/apps/server/src/modules/tldraw/controller/api-test/tldraw.ws.api.spec.ts @@ -2,10 +2,11 @@ import { WsAdapter } from '@nestjs/platform-ws'; import { Test } from '@nestjs/testing'; import WebSocket from 'ws'; import { TextEncoder } from 'util'; -import { INestApplication, NotFoundException } from '@nestjs/common'; +import { INestApplication } from '@nestjs/common'; import { throwError } from 'rxjs'; import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { HttpService } from '@nestjs/axios'; +import { AxiosError, AxiosRequestHeaders } from 'axios'; import { WsCloseCodeEnum, WsCloseMessageEnum } from '../../types'; import { TldrawWsTestModule } from '../../tldraw-ws-test.module'; import { TldrawWsService } from '../../service'; @@ -196,7 +197,14 @@ describe('WebSocketController (WsAdapter)', () => { const { setupConnectionSpy, wsCloseSpy } = setup(); const authorizeConnectionSpy = jest.spyOn(wsService, 'authorizeConnection'); authorizeConnectionSpy.mockImplementationOnce(() => { - throw new NotFoundException('Resource not found'); + throw new AxiosError('Resource not found', '404', undefined, undefined, { + config: { headers: {} as AxiosRequestHeaders }, + data: undefined, + request: undefined, + statusText: '', + status: 404, + headers: {}, + }); }); ws = await TestConnection.setupWs(wsUrl, 'GLOBAL', { headers: { cookie: { jwt: 'jwt-mocked' } } }); diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index 414948268ac..0ee30de2c63 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -3,8 +3,9 @@ import { Server, WebSocket } from 'ws'; import { Request } from 'express'; import { ConfigService } from '@nestjs/config'; import cookie from 'cookie'; -import { BadRequestException, NotFoundException } from '@nestjs/common'; +import { BadRequestException } from '@nestjs/common'; import { Logger } from '@src/core/logger'; +import { AxiosError } from 'axios'; import { WebsocketCloseErrorLoggable } from '../loggable/websocket-close-error.loggable'; import { TldrawConfig, SOCKET_PORT } from '../config'; import { WsCloseCodeEnum, WsCloseMessageEnum } from '../types'; @@ -28,7 +29,7 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { try { await this.tldrawWsService.authorizeConnection(docName, cookies?.jwt); } catch (err) { - if (err instanceof NotFoundException) { + if ((err as AxiosError).response?.status === 404) { this.closeClientAndLogError( client, WsCloseCodeEnum.WS_CLIENT_NOT_FOUND_CODE, From f9763e455866dda0e0df554852e799d9a4929005 Mon Sep 17 00:00:00 2001 From: blazejpass Date: Mon, 8 Jan 2024 14:52:26 +0100 Subject: [PATCH 29/29] Add case of wrong mongoId passed from client --- apps/server/src/modules/tldraw/controller/tldraw.ws.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts index 0ee30de2c63..4aad2c404b3 100644 --- a/apps/server/src/modules/tldraw/controller/tldraw.ws.ts +++ b/apps/server/src/modules/tldraw/controller/tldraw.ws.ts @@ -29,7 +29,7 @@ export class TldrawWs implements OnGatewayInit, OnGatewayConnection { try { await this.tldrawWsService.authorizeConnection(docName, cookies?.jwt); } catch (err) { - if ((err as AxiosError).response?.status === 404) { + if ((err as AxiosError).response?.status === 404 || (err as AxiosError).response?.status === 400) { this.closeClientAndLogError( client, WsCloseCodeEnum.WS_CLIENT_NOT_FOUND_CODE,