From 6a7e1fcc5b6c268dd9cb39fb3046999be2006b85 Mon Sep 17 00:00:00 2001 From: Sergej Hoffmann <97111299+SevenWaysDP@users.noreply.github.com> Date: Mon, 9 Sep 2024 14:08:26 +0200 Subject: [PATCH] BC-7902 - change logic of authorization (#15) --- jest.config.cjs | 21 ++ package-lock.json | 42 ++-- package.json | 23 +-- .../authorization/authorization.module.ts | 2 + .../authorization.service.spec.ts | 189 ++++++++++++++++++ .../authorization/authorization.service.ts | 74 +++++-- .../interfaces/response.payload.ts | 6 + src/infra/authorization/response.builder.ts | 16 ++ 8 files changed, 320 insertions(+), 53 deletions(-) create mode 100644 jest.config.cjs create mode 100644 src/infra/authorization/authorization.service.spec.ts create mode 100644 src/infra/authorization/interfaces/response.payload.ts create mode 100644 src/infra/authorization/response.builder.ts diff --git a/jest.config.cjs b/jest.config.cjs new file mode 100644 index 00000000..7ee9428a --- /dev/null +++ b/jest.config.cjs @@ -0,0 +1,21 @@ +/** @type {import('ts-jest').JestConfigWithTsJest} */ +module.exports = { + extensionsToTreatAsEsm: ['.ts'], + moduleNameMapper: { + '^(\\.{1,2}/.*)\\.js$': '$1', + }, + transform: { + '^.+\\.tsx?$': [ + 'ts-jest', + { + useESM: true, + }, + ], + }, + moduleFileExtensions: ['js', 'json', 'ts'], + rootDir: 'src', + testRegex: '.*\\.spec\\.ts$', + collectCoverageFrom: ['**/*.(t|j)s'], + coverageDirectory: '../coverage', + testEnvironment: 'node', +}; diff --git a/package-lock.json b/package-lock.json index 8b801e4f..1ee3b49c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,7 +13,7 @@ "@nestjs/config": "^3.2.3", "@nestjs/core": "^10.4.1", "@nestjs/platform-express": "^10.4.1", - "@y/redis": "github:hpi-schul-cloud/y-redis#9933ff3abd948bff33c540f07b0671fe657ecd41", + "@y/redis": "github:hpi-schul-cloud/y-redis#a5e141466a759a1d4b2876d5d5af52bc9ec4930d", "ioredis": "^5.4.1", "prom-client": "^15.1.3", "reflect-metadata": "^0.2.2", @@ -22,6 +22,7 @@ "y-protocols": "^1.0.6" }, "devDependencies": { + "@golevelup/ts-jest": "^0.5.5", "@nestjs/cli": "^10.4.4", "@nestjs/schematics": "^10.1.3", "@nestjs/testing": "^10.4.1", @@ -859,6 +860,12 @@ "node": "^12.22.0 || ^14.17.0 || >=16.0.0" } }, + "node_modules/@golevelup/ts-jest": { + "version": "0.5.5", + "resolved": "https://registry.npmjs.org/@golevelup/ts-jest/-/ts-jest-0.5.5.tgz", + "integrity": "sha512-x1kAFZ6ADPpwl6rauyJY6uP3OdTXA+BHb2S6nK/dpQcoAyC1gAPlb7kBTFfzvxm8yaoIieaR/638kVZSK6GxtQ==", + "dev": true + }, "node_modules/@humanwhocodes/config-array": { "version": "0.11.14", "resolved": "https://registry.npmjs.org/@humanwhocodes/config-array/-/config-array-0.11.14.tgz", @@ -1525,9 +1532,9 @@ } }, "node_modules/@nestjs/cli": { - "version": "10.4.4", - "resolved": "https://registry.npmjs.org/@nestjs/cli/-/cli-10.4.4.tgz", - "integrity": "sha512-WKERbSZJGof0+9XeeMmWnb/9FpNxogcB5eTJTHjc9no0ymdTw3jTzT+KZL9iC/hGqBpuomDLaNFCYbAOt29nBw==", + "version": "10.4.5", + "resolved": "https://registry.npmjs.org/@nestjs/cli/-/cli-10.4.5.tgz", + "integrity": "sha512-FP7Rh13u8aJbHe+zZ7hM0CC4785g9Pw4lz4r2TTgRtf0zTxSWMkJaPEwyjX8SK9oWK2GsYxl+fKpwVZNbmnj9A==", "dev": true, "dependencies": { "@angular-devkit/core": "17.3.8", @@ -1547,7 +1554,7 @@ "tsconfig-paths": "4.2.0", "tsconfig-paths-webpack-plugin": "4.1.0", "typescript": "5.3.3", - "webpack": "5.93.0", + "webpack": "5.94.0", "webpack-node-externals": "3.0.0" }, "bin": { @@ -2013,21 +2020,13 @@ "resolved": "https://registry.npmjs.org/@types/eslint/-/eslint-9.6.0.tgz", "integrity": "sha512-gi6WQJ7cHRgZxtkQEoyHMppPjq9Kxo5Tjn2prSKDSmZrCz8TZ3jSRCeTJm+WoM+oB0WG37bRqLzaaU3q7JypGg==", "dev": true, + "optional": true, + "peer": true, "dependencies": { "@types/estree": "*", "@types/json-schema": "*" } }, - "node_modules/@types/eslint-scope": { - "version": "3.7.7", - "resolved": "https://registry.npmjs.org/@types/eslint-scope/-/eslint-scope-3.7.7.tgz", - "integrity": "sha512-MzMFlSLBqNF2gcHWO0G1vP/YQyfvrxZ0bF+u7mzUdZ1/xK4A4sru+nraZz5i3iEIk1l1uyicaDVTB4QbbEkAYg==", - "dev": true, - "dependencies": { - "@types/eslint": "*", - "@types/estree": "*" - } - }, "node_modules/@types/estree": { "version": "1.0.5", "resolved": "https://registry.npmjs.org/@types/estree/-/estree-1.0.5.tgz", @@ -2561,8 +2560,8 @@ }, "node_modules/@y/redis": { "version": "0.1.6", - "resolved": "git+ssh://git@github.com/hpi-schul-cloud/y-redis.git#9933ff3abd948bff33c540f07b0671fe657ecd41", - "integrity": "sha512-ptHv/KB3RtGqw2laezTr3vWm2+XNboIf3Y3NN67fzUIDPQoxn38Xgm2gr4uitJd4nX+6QwmoL5sGFef6050D6Q==", + "resolved": "git+ssh://git@github.com/hpi-schul-cloud/y-redis.git#a5e141466a759a1d4b2876d5d5af52bc9ec4930d", + "integrity": "sha512-wtrbVIf5ifCkzdX3EV5G5ZCwYf3Xl+0Td2gfvNh0qgGrIXxjRaqQFCTzniPRrYy0gzOR2s/OHe70dlNZSUbNYg==", "dependencies": { "ioredis": "^5.4.1", "lib0": "^0.2.95", @@ -8935,12 +8934,11 @@ "integrity": "sha512-2JAn3z8AR6rjK8Sm8orRC0h/bcl/DqL7tRPdGZ4I1CjdF+EaMLmYxBHyXuKL849eucPFhvBoxMsflfOb8kxaeQ==" }, "node_modules/webpack": { - "version": "5.93.0", - "resolved": "https://registry.npmjs.org/webpack/-/webpack-5.93.0.tgz", - "integrity": "sha512-Y0m5oEY1LRuwly578VqluorkXbvXKh7U3rLoQCEO04M97ScRr44afGVkI0FQFsXzysk5OgFAxjZAb9rsGQVihA==", + "version": "5.94.0", + "resolved": "https://registry.npmjs.org/webpack/-/webpack-5.94.0.tgz", + "integrity": "sha512-KcsGn50VT+06JH/iunZJedYGUJS5FGjow8wb9c0v5n1Om8O1g4L6LjtfxwlXIATopoQu+vOXXa7gYisWxCoPyg==", "dev": true, "dependencies": { - "@types/eslint-scope": "^3.7.3", "@types/estree": "^1.0.5", "@webassemblyjs/ast": "^1.12.1", "@webassemblyjs/wasm-edit": "^1.12.1", @@ -8949,7 +8947,7 @@ "acorn-import-attributes": "^1.9.5", "browserslist": "^4.21.10", "chrome-trace-event": "^1.0.2", - "enhanced-resolve": "^5.17.0", + "enhanced-resolve": "^5.17.1", "es-module-lexer": "^1.2.1", "eslint-scope": "5.1.1", "events": "^3.2.0", diff --git a/package.json b/package.json index 19c4e460..07e8176b 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,8 @@ "start:worker:dev": "nest start worker --watch", "start:worker:debug": "nest start worker --debug --watch", "start:worker:prod": "node dist/apps/tldraw-worker.js", - "lint": "eslint \"src/**/*.ts\" --fix", + "lint": "eslint \"src/**/*.ts\" ", + "lint:fix": "eslint \"src/**/*.ts\" --fix", "test": "jest", "test:watch": "jest --watch", "test:cov": "jest --coverage", @@ -31,7 +32,7 @@ "@nestjs/config": "^3.2.3", "@nestjs/core": "^10.4.1", "@nestjs/platform-express": "^10.4.1", - "@y/redis": "github:hpi-schul-cloud/y-redis#9933ff3abd948bff33c540f07b0671fe657ecd41", + "@y/redis": "github:hpi-schul-cloud/y-redis#a5e141466a759a1d4b2876d5d5af52bc9ec4930d", "ioredis": "^5.4.1", "prom-client": "^15.1.3", "reflect-metadata": "^0.2.2", @@ -40,6 +41,7 @@ "y-protocols": "^1.0.6" }, "devDependencies": { + "@golevelup/ts-jest": "^0.5.5", "@nestjs/cli": "^10.4.4", "@nestjs/schematics": "^10.1.3", "@nestjs/testing": "^10.4.1", @@ -61,22 +63,5 @@ "ts-node": "^10.9.2", "tsconfig-paths": "^4.2.0", "typescript": "^5.5.4" - }, - "jest": { - "moduleFileExtensions": [ - "js", - "json", - "ts" - ], - "rootDir": "src", - "testRegex": ".*\\.spec\\.ts$", - "transform": { - "^.+\\.(t|j)s$": "ts-jest" - }, - "collectCoverageFrom": [ - "**/*.(t|j)s" - ], - "coverageDirectory": "../coverage", - "testEnvironment": "node" } } diff --git a/src/infra/authorization/authorization.module.ts b/src/infra/authorization/authorization.module.ts index 15b2d82e..b30dcdd1 100644 --- a/src/infra/authorization/authorization.module.ts +++ b/src/infra/authorization/authorization.module.ts @@ -1,7 +1,9 @@ import { Module } from '@nestjs/common'; +import { LoggerModule } from '../logging/logger.module.js'; import { AuthorizationService } from './authorization.service.js'; @Module({ + imports: [LoggerModule], providers: [AuthorizationService], exports: [AuthorizationService], }) diff --git a/src/infra/authorization/authorization.service.spec.ts b/src/infra/authorization/authorization.service.spec.ts new file mode 100644 index 00000000..6147caca --- /dev/null +++ b/src/infra/authorization/authorization.service.spec.ts @@ -0,0 +1,189 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { ConfigService } from '@nestjs/config'; +import { Test, TestingModule } from '@nestjs/testing'; +import { HttpRequest } from 'uws'; +import { Logger } from '../logging/logger.js'; +import { AuthorizationService } from './authorization.service.js'; + +describe(AuthorizationService.name, () => { + let module: TestingModule; + let service: AuthorizationService; + let configService: DeepMocked; + + beforeAll(async () => { + module = await Test.createTestingModule({ + providers: [ + AuthorizationService, + { + provide: ConfigService, + useValue: createMock(), + }, + { + provide: Logger, + useValue: createMock(), + }, + ], + }).compile(); + + service = module.get(AuthorizationService); + configService = module.get(ConfigService); + }); + + afterAll(async () => { + await module.close(); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + const setupRequest = (roomId = 'roomId', cookies = 'other=ABC;jwt=eyJhbGciOiJIU') => { + const req: DeepMocked = createMock(); + jest.spyOn(req, 'getParameter').mockReturnValue(roomId); + jest.spyOn(req, 'getHeader').mockReturnValue(cookies); + configService.getOrThrow.mockReturnValue('API_HOST'); + const fetchSpy = jest.spyOn(global, 'fetch'); + + return { req, fetchSpy }; + }; + + describe('hasPermission', () => { + describe('when the user request has permission', () => { + const setup = () => { + const { req, fetchSpy } = setupRequest(); + + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ isAuthorized: true, userId: '123' }), + } as any); + + const expectedResult = { error: null, hasWriteAccess: true, room: 'roomId', userid: '123' }; + + return { req, expectedResult }; + }; + + it('should return an expectedResult response payload', async () => { + const { req, expectedResult } = setup(); + + const response = await service.hasPermission(req); + + expect(response).toEqual(expectedResult); + }); + }); + + describe('when the user has no permission', () => { + const setup = () => { + const { req, fetchSpy } = setupRequest(); + + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ isAuthorized: false, userId: '123' }), + } as any); + + const expectedResult = { + error: { + code: 4401, + reason: 'Unauthorized', + }, + hasWriteAccess: false, + room: null, + userid: null, + }; + + return { req, expectedResult }; + }; + + it('should return an expectedResult response payload', async () => { + const { req, expectedResult } = setup(); + + const response = await service.hasPermission(req); + + expect(response).toEqual(expectedResult); + }); + }); + + describe('when the roomId is not in request params', () => { + const setup = () => { + const { req } = setupRequest(''); + + const expectedResult = { + error: { + code: 4500, + reason: 'RoomId not found', + }, + hasWriteAccess: false, + room: null, + userid: null, + }; + + return { req, expectedResult }; + }; + + it('should return an expectedResult response payload', async () => { + const { req, expectedResult } = setup(); + + const response = await service.hasPermission(req); + + expect(response).toEqual(expectedResult); + }); + }); + + describe('when the jwtToken is not in request cookies', () => { + const setup = () => { + const { req } = setupRequest('roomId', 'other=ABC'); + const expectedResult = { + error: { + code: 4500, + reason: 'JWT token not found', + }, + hasWriteAccess: false, + room: null, + userid: null, + }; + + return { req, expectedResult }; + }; + + it('should return an expectedResult response payload', async () => { + const { req, expectedResult } = setup(); + + const response = await service.hasPermission(req); + + expect(response).toEqual(expectedResult); + }); + }); + + describe('when the roomId not found on server', () => { + const setup = () => { + const { req, fetchSpy } = setupRequest(); + + fetchSpy.mockResolvedValue({ + ok: false, + status: 404, + statusText: 'Not Found', + json: () => Promise.resolve({}), + } as any); + + const expectedResult = { + error: { + code: 4404, + reason: 'Not Found', + }, + hasWriteAccess: false, + room: null, + userid: null, + }; + + return { req, expectedResult }; + }; + + it('should return an expectedResult response payload', async () => { + const { req, expectedResult } = setup(); + + const response = await service.hasPermission(req); + + expect(response).toEqual(expectedResult); + }); + }); + }); +}); diff --git a/src/infra/authorization/authorization.service.ts b/src/infra/authorization/authorization.service.ts index f761ad3a..21e04bd7 100644 --- a/src/infra/authorization/authorization.service.ts +++ b/src/infra/authorization/authorization.service.ts @@ -1,32 +1,69 @@ import { Injectable } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { HttpRequest } from 'uws'; +import { Logger } from '../logging/logger.js'; +import { ResponsePayload } from './interfaces/response.payload.js'; +import { ResponsePayloadBuilder } from './response.builder.js'; @Injectable() export class AuthorizationService { - constructor(private configService: ConfigService) {} + constructor( + private configService: ConfigService, + private logger: Logger, + ) { + logger.setContext(AuthorizationService.name); + } + + async hasPermission(req: HttpRequest): Promise { + let response: ResponsePayload; + try { + const room = this.getRoom(req); + const token = this.getToken(req); + + response = await this.fetchAuthorization(room, token); + } catch (error) { + response = this.createErrorResponsePayload(4500, error.message); + } + + return response; + } - async hasPermission(req: HttpRequest) { - const apiHost = this.configService.get('API_HOST'); + private getRoom(req: HttpRequest): string { const room = req.getParameter(0); - const token = this.getCookie(req, 'jwt'); - if (!token) { - throw new Error('Missing Token'); + if (!room) { + throw new Error('RoomId not found'); + } + + return room; + } + + private getToken(req: HttpRequest): string { + const jwtToken = this.getCookie(req, 'jwt'); + + if (!jwtToken) { + throw new Error('JWT token not found'); } + return jwtToken; + } + + private async fetchAuthorization(room: string, token: string): Promise { + const apiHost = this.configService.getOrThrow('API_HOST'); const requestOptions = this.createAuthzRequestOptions(room, token); - const response = await fetch(`${apiHost}/api/v3/authorization/by-reference`, requestOptions); - const { userId } = await response.json(); + const response = await fetch(`${apiHost}/api/v3/authorization/by-reference`, requestOptions); if (!response.ok) { - throw new Error('Authorization failed'); + return this.createErrorResponsePayload(4000 + response.status, response.statusText); } - const result = { hasWriteAccess: true, room, userid: userId }; + const { isAuthorized, userId } = await response.json(); + if (!isAuthorized) { + return this.createErrorResponsePayload(4401, 'Unauthorized'); + } - return result; + return this.createResponsePayload(room, userId); } private createAuthzRequestOptions(room: string, token: string) { @@ -39,7 +76,7 @@ export class AuthorizationService { body: JSON.stringify({ context: { action: 'read', - requiredPermissions: ['COURSE_EDIT'], + requiredPermissions: ['COURSE_VIEW'], }, referenceType: 'boardnodes', referenceId: room, @@ -49,6 +86,19 @@ export class AuthorizationService { return requestOptions; } + private createResponsePayload(room: string, userId: string): ResponsePayload { + const response = ResponsePayloadBuilder.build(room, userId, null, true); + + return response; + } + + private createErrorResponsePayload(code: number, reason: string): ResponsePayload { + const response = ResponsePayloadBuilder.buildWithError(code, reason); + this.logger.error(`Error: ${code} - ${reason}`); + + return response; + } + private getCookie(request: HttpRequest, cookieName: string) { const cookie = request.getHeader('cookie'); if (!cookie) { diff --git a/src/infra/authorization/interfaces/response.payload.ts b/src/infra/authorization/interfaces/response.payload.ts new file mode 100644 index 00000000..81c5719a --- /dev/null +++ b/src/infra/authorization/interfaces/response.payload.ts @@ -0,0 +1,6 @@ +export interface ResponsePayload { + hasWriteAccess: boolean; + room: string | null; + userid: string | null; + error: Partial | null; +} diff --git a/src/infra/authorization/response.builder.ts b/src/infra/authorization/response.builder.ts new file mode 100644 index 00000000..b6950d1f --- /dev/null +++ b/src/infra/authorization/response.builder.ts @@ -0,0 +1,16 @@ +import { ResponsePayload } from './interfaces/response.payload.js'; + +export const ResponsePayloadBuilder = { + build: ( + room: string | null = null, + userid: string | null = null, + error: Partial | null = null, + hasWriteAccess: boolean = false, + ): ResponsePayload => { + return { hasWriteAccess, room, userid, error }; + }, + + buildWithError: (code: number, reason: string): ResponsePayload => { + return ResponsePayloadBuilder.build(null, null, { code, reason }); + }, +};