From e030bfb59dd5ee65f20a64686ec9b22ca39f70ae Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Wed, 2 Jun 2021 18:15:41 +0200 Subject: Refactor server errors handler --- server/controllers/api/videos/index.ts | 3 + server/controllers/api/videos/update.ts | 2 + server/controllers/api/videos/upload.ts | 4 ++ server/helpers/express-utils.ts | 32 +---------- server/middlewares/doc.ts | 13 +++++ server/middlewares/error.ts | 39 +++++++++++++ server/middlewares/index.ts | 2 + server/middlewares/validators/videos/videos.ts | 6 -- server/tests/api/check-params/videos.ts | 78 +++++++++++++++++++++++++- server/tests/api/server/follow-constraints.ts | 16 +++++- server/typings/express/index.d.ts | 19 +++++-- 11 files changed, 169 insertions(+), 45 deletions(-) create mode 100644 server/middlewares/doc.ts create mode 100644 server/middlewares/error.ts (limited to 'server') diff --git a/server/controllers/api/videos/index.ts b/server/controllers/api/videos/index.ts index db23e5630..7671f099e 100644 --- a/server/controllers/api/videos/index.ts +++ b/server/controllers/api/videos/index.ts @@ -2,6 +2,7 @@ import * as express from 'express' import toInt from 'validator/lib/toInt' import { doJSONRequest } from '@server/helpers/requests' import { LiveManager } from '@server/lib/live-manager' +import { docMiddleware } from '@server/middlewares/doc' import { getServerActor } from '@server/models/application/application' import { MVideoAccountLight } from '@server/types/models' import { VideosCommonQuery } from '../../../../shared' @@ -83,6 +84,7 @@ videosRouter.get('/:id/metadata/:videoFileId', asyncMiddleware(getVideoFileMetadata) ) videosRouter.get('/:id', + docMiddleware('https://docs.joinpeertube.org/api-rest-reference.html#operation/getVideo'), optionalAuthenticate, asyncMiddleware(videosCustomGetValidator('only-video-with-rights')), asyncMiddleware(checkVideoFollowConstraints), @@ -94,6 +96,7 @@ videosRouter.post('/:id/views', ) videosRouter.delete('/:id', + docMiddleware('https://docs.joinpeertube.org/api-rest-reference.html#operation/delVideo'), authenticate, asyncMiddleware(videosRemoveValidator), asyncRetryTransactionMiddleware(removeVideo) diff --git a/server/controllers/api/videos/update.ts b/server/controllers/api/videos/update.ts index 2450abd0e..09e584d30 100644 --- a/server/controllers/api/videos/update.ts +++ b/server/controllers/api/videos/update.ts @@ -20,6 +20,7 @@ import { autoBlacklistVideoIfNeeded } from '../../../lib/video-blacklist' import { asyncMiddleware, asyncRetryTransactionMiddleware, authenticate, videosUpdateValidator } from '../../../middlewares' import { ScheduleVideoUpdateModel } from '../../../models/video/schedule-video-update' import { VideoModel } from '../../../models/video/video' +import { docMiddleware } from '@server/middlewares/doc' const lTags = loggerTagsFactory('api', 'video') const auditLogger = auditLoggerFactory('videos') @@ -35,6 +36,7 @@ const reqVideoFileUpdate = createReqFiles( ) updateRouter.put('/:id', + docMiddleware('https://docs.joinpeertube.org/api-rest-reference.html#operation/putVideo'), authenticate, reqVideoFileUpdate, asyncMiddleware(videosUpdateValidator), diff --git a/server/controllers/api/videos/upload.ts b/server/controllers/api/videos/upload.ts index c33d7fcb9..93a68f759 100644 --- a/server/controllers/api/videos/upload.ts +++ b/server/controllers/api/videos/upload.ts @@ -6,6 +6,7 @@ import { createTorrentAndSetInfoHash } from '@server/helpers/webtorrent' import { getLocalVideoActivityPubUrl } from '@server/lib/activitypub/url' import { addOptimizeOrMergeAudioJob, buildLocalVideoFromReq, buildVideoThumbnailsFromReq, setVideoTags } from '@server/lib/video' import { generateVideoFilename, getVideoFilePath } from '@server/lib/video-paths' +import { docMiddleware } from '@server/middlewares/doc' import { MVideo, MVideoFile, MVideoFullLight } from '@server/types/models' import { uploadx } from '@uploadx/core' import { VideoCreate, VideoState } from '../../../../shared' @@ -60,6 +61,7 @@ const reqVideoFileAddResumable = createReqFiles( ) uploadRouter.post('/upload', + docMiddleware('https://docs.joinpeertube.org/api-rest-reference.html#operation/uploadLegacy'), authenticate, reqVideoFileAdd, asyncMiddleware(videosAddLegacyValidator), @@ -67,6 +69,7 @@ uploadRouter.post('/upload', ) uploadRouter.post('/upload-resumable', + docMiddleware('https://docs.joinpeertube.org/api-rest-reference.html#operation/uploadResumableInit'), authenticate, reqVideoFileAddResumable, asyncMiddleware(videosAddResumableInitValidator), @@ -79,6 +82,7 @@ uploadRouter.delete('/upload-resumable', ) uploadRouter.put('/upload-resumable', + docMiddleware('https://docs.joinpeertube.org/api-rest-reference.html#operation/uploadResumable'), authenticate, uploadxMiddleware, // uploadx doesn't use call next() before the file upload completes asyncMiddleware(videosAddResumableValidator), diff --git a/server/helpers/express-utils.ts b/server/helpers/express-utils.ts index 10a860787..010c6961a 100644 --- a/server/helpers/express-utils.ts +++ b/server/helpers/express-utils.ts @@ -8,7 +8,6 @@ import { isArray } from './custom-validators/misc' import { logger } from './logger' import { deleteFileAndCatch, generateRandomString } from './utils' import { getExtFromMimetype } from './video' -import { ProblemDocument, ProblemDocumentExtension } from 'http-problem-details' function buildNSFWFilter (res?: express.Response, paramNSFW?: string) { if (paramNSFW === 'true') return true @@ -126,34 +125,6 @@ function getCountVideos (req: express.Request) { return req.query.skipCount !== true } -// helpers added in server.ts and used in subsequent controllers used -const apiResponseHelpers = (req, res: express.Response, next = null) => { - res.fail = (options) => { - const { data, status = HttpStatusCode.BAD_REQUEST_400, message, title, type, docs = res.docs, instance } = options - - const extension = new ProblemDocumentExtension({ - ...data, - docs, - // fields for <= 3.2 compatibility, deprecated - error: message, - code: type - }) - - res.status(status) - res.setHeader('Content-Type', 'application/problem+json') - res.json(new ProblemDocument({ - status, - title, - instance, - // fields intended to replace 'error' and 'code' respectively - detail: message, - type: type && 'https://docs.joinpeertube.org/api-rest-reference.html#section/Errors/' + type - }, extension)) - } - - if (next) next() -} - // --------------------------------------------------------------------------- export { @@ -163,6 +134,5 @@ export { badRequest, createReqFiles, cleanUpReqFiles, - getCountVideos, - apiResponseHelpers + getCountVideos } diff --git a/server/middlewares/doc.ts b/server/middlewares/doc.ts new file mode 100644 index 000000000..aa852cd77 --- /dev/null +++ b/server/middlewares/doc.ts @@ -0,0 +1,13 @@ +import * as express from 'express' + +function docMiddleware (docUrl: string) { + return (req: express.Request, res: express.Response, next: express.NextFunction) => { + res.locals.docUrl = docUrl + + if (next) return next() + } +} + +export { + docMiddleware +} diff --git a/server/middlewares/error.ts b/server/middlewares/error.ts new file mode 100644 index 000000000..e3eb1c8f5 --- /dev/null +++ b/server/middlewares/error.ts @@ -0,0 +1,39 @@ +import * as express from 'express' +import { ProblemDocument, ProblemDocumentExtension } from 'http-problem-details' +import { HttpStatusCode } from '@shared/core-utils' + +function apiFailMiddleware (req: express.Request, res: express.Response, next: express.NextFunction) { + res.fail = options => { + const { status = HttpStatusCode.BAD_REQUEST_400, message, title, type, data, instance } = options + + const extension = new ProblemDocumentExtension({ + ...data, + + docs: res.locals.docUrl, + code: type, + + // For <= 3.2 compatibility + error: message + }) + + res.status(status) + res.setHeader('Content-Type', 'application/problem+json') + res.json(new ProblemDocument({ + status, + title, + instance, + + detail: message, + + type: type + ? `https://docs.joinpeertube.org/api-rest-reference.html#section/Errors/${type}` + : undefined + }, extension)) + } + + if (next) next() +} + +export { + apiFailMiddleware +} diff --git a/server/middlewares/index.ts b/server/middlewares/index.ts index 3e280e16f..413653dac 100644 --- a/server/middlewares/index.ts +++ b/server/middlewares/index.ts @@ -7,4 +7,6 @@ export * from './servers' export * from './sort' export * from './user-right' export * from './dnt' +export * from './error' +export * from './doc' export * from './csp' diff --git a/server/middlewares/validators/videos/videos.ts b/server/middlewares/validators/videos/videos.ts index 64e09234e..b7a9bcbe3 100644 --- a/server/middlewares/validators/videos/videos.ts +++ b/server/middlewares/validators/videos/videos.ts @@ -73,7 +73,6 @@ const videosAddLegacyValidator = getCommonVideoEditAttributes().concat([ .custom(isIdValid).withMessage('Should have correct video channel id'), async (req: express.Request, res: express.Response, next: express.NextFunction) => { - res.docs = "https://docs.joinpeertube.org/api-rest-reference.html#operation/uploadLegacy" logger.debug('Checking videosAdd parameters', { parameters: req.body, files: req.files }) if (areValidationErrors(req, res)) return cleanUpReqFiles(req) @@ -108,7 +107,6 @@ const videosAddLegacyValidator = getCommonVideoEditAttributes().concat([ */ const videosAddResumableValidator = [ async (req: express.Request, res: express.Response, next: express.NextFunction) => { - res.docs = "https://docs.joinpeertube.org/api-rest-reference.html#operation/uploadResumable" const user = res.locals.oauth.token.User const body: express.CustomUploadXFile = req.body @@ -170,7 +168,6 @@ const videosAddResumableInitValidator = getCommonVideoEditAttributes().concat([ .withMessage('Should specify the file mimetype'), async (req: express.Request, res: express.Response, next: express.NextFunction) => { - res.docs = "https://docs.joinpeertube.org/api-rest-reference.html#operation/uploadResumableInit" const videoFileMetadata = { mimetype: req.headers['x-upload-content-type'] as string, size: +req.headers['x-upload-content-length'], @@ -214,7 +211,6 @@ const videosUpdateValidator = getCommonVideoEditAttributes().concat([ .custom(isIdValid).withMessage('Should have correct video channel id'), async (req: express.Request, res: express.Response, next: express.NextFunction) => { - res.docs = 'https://docs.joinpeertube.org/api-rest-reference.html#operation/putVideo' logger.debug('Checking videosUpdate parameters', { parameters: req.body }) if (areValidationErrors(req, res)) return cleanUpReqFiles(req) @@ -268,7 +264,6 @@ const videosCustomGetValidator = ( param('id').custom(isIdOrUUIDValid).not().isEmpty().withMessage('Should have a valid id'), async (req: express.Request, res: express.Response, next: express.NextFunction) => { - res.docs = 'https://docs.joinpeertube.org/api-rest-reference.html#operation/getVideo' logger.debug('Checking videosGet parameters', { parameters: req.params }) if (areValidationErrors(req, res)) return @@ -334,7 +329,6 @@ const videosRemoveValidator = [ param('id').custom(isIdOrUUIDValid).not().isEmpty().withMessage('Should have a valid id'), async (req: express.Request, res: express.Response, next: express.NextFunction) => { - res.docs = "https://docs.joinpeertube.org/api-rest-reference.html#operation/delVideo" logger.debug('Checking videosRemove parameters', { parameters: req.params }) if (areValidationErrors(req, res)) return diff --git a/server/tests/api/check-params/videos.ts b/server/tests/api/check-params/videos.ts index c970c4a15..a6eecb13a 100644 --- a/server/tests/api/check-params/videos.ts +++ b/server/tests/api/check-params/videos.ts @@ -4,6 +4,8 @@ import 'mocha' import * as chai from 'chai' import { omit } from 'lodash' import { join } from 'path' +import { randomInt } from '@shared/core-utils' +import { PeerTubeProblemDocument } from '@shared/models' import { HttpStatusCode } from '../../../../shared/core-utils/miscs/http-error-codes' import { checkUploadVideoParam, @@ -30,7 +32,6 @@ import { checkBadStartPagination } from '../../../../shared/extra-utils/requests/check-api-params' import { VideoPrivacy } from '../../../../shared/models/videos/video-privacy.enum' -import { randomInt } from '@shared/core-utils' const expect = chai.expect @@ -411,6 +412,31 @@ describe('Test videos API validator', function () { await checkUploadVideoParam(server.url, server.accessToken, { ...fields, ...attaches }, HttpStatusCode.BAD_REQUEST_400, mode) }) + it('Should report the appropriate error', async function () { + const fields = immutableAssign(baseCorrectParams, { language: 'a'.repeat(15) }) + const attaches = baseCorrectAttaches + + const attributes = { ...fields, ...attaches } + const res = await checkUploadVideoParam(server.url, server.accessToken, attributes, HttpStatusCode.BAD_REQUEST_400, mode) + + const error = res.body as PeerTubeProblemDocument + + if (mode === 'legacy') { + expect(error.docs).to.equal('https://docs.joinpeertube.org/api-rest-reference.html#operation/uploadLegacy') + } else { + expect(error.docs).to.equal('https://docs.joinpeertube.org/api-rest-reference.html#operation/uploadResumableInit') + } + + expect(error.type).to.equal('about:blank') + expect(error.title).to.equal('Bad Request') + + expect(error.detail).to.equal('Incorrect request parameters: language') + expect(error.error).to.equal('Incorrect request parameters: language') + + expect(error.status).to.equal(HttpStatusCode.BAD_REQUEST_400) + expect(error['invalid-params'].language).to.exist + }) + it('Should succeed with the correct parameters', async function () { this.timeout(10000) @@ -645,6 +671,24 @@ describe('Test videos API validator', function () { it('Should fail with a video of another server') + it('Shoud report the appropriate error', async function () { + const fields = immutableAssign(baseCorrectParams, { licence: 125 }) + + const res = await makePutBodyRequest({ url: server.url, path: path + videoId, token: server.accessToken, fields }) + const error = res.body as PeerTubeProblemDocument + + expect(error.docs).to.equal('https://docs.joinpeertube.org/api-rest-reference.html#operation/putVideo') + + expect(error.type).to.equal('about:blank') + expect(error.title).to.equal('Bad Request') + + expect(error.detail).to.equal('Incorrect request parameters: licence') + expect(error.error).to.equal('Incorrect request parameters: licence') + + expect(error.status).to.equal(HttpStatusCode.BAD_REQUEST_400) + expect(error['invalid-params'].licence).to.exist + }) + it('Should succeed with the correct parameters', async function () { const fields = baseCorrectParams @@ -678,6 +722,22 @@ describe('Test videos API validator', function () { await getVideo(server.url, '4da6fde3-88f7-4d16-b119-108df5630b06', HttpStatusCode.NOT_FOUND_404) }) + it('Shoud report the appropriate error', async function () { + const res = await getVideo(server.url, 'hi', HttpStatusCode.BAD_REQUEST_400) + const error = res.body as PeerTubeProblemDocument + + expect(error.docs).to.equal('https://docs.joinpeertube.org/api-rest-reference.html#operation/getVideo') + + expect(error.type).to.equal('about:blank') + expect(error.title).to.equal('Bad Request') + + expect(error.detail).to.equal('Incorrect request parameters: id') + expect(error.error).to.equal('Incorrect request parameters: id') + + expect(error.status).to.equal(HttpStatusCode.BAD_REQUEST_400) + expect(error['invalid-params'].id).to.exist + }) + it('Should succeed with the correct parameters', async function () { await getVideo(server.url, videoId) }) @@ -755,6 +815,22 @@ describe('Test videos API validator', function () { it('Should fail with a video of another server') + it('Shoud report the appropriate error', async function () { + const res = await removeVideo(server.url, server.accessToken, 'hello', HttpStatusCode.BAD_REQUEST_400) + const error = res.body as PeerTubeProblemDocument + + expect(error.docs).to.equal('https://docs.joinpeertube.org/api-rest-reference.html#operation/delVideo') + + expect(error.type).to.equal('about:blank') + expect(error.title).to.equal('Bad Request') + + expect(error.detail).to.equal('Incorrect request parameters: id') + expect(error.error).to.equal('Incorrect request parameters: id') + + expect(error.status).to.equal(HttpStatusCode.BAD_REQUEST_400) + expect(error['invalid-params'].id).to.exist + }) + it('Should succeed with the correct parameters', async function () { await removeVideo(server.url, server.accessToken, videoId) }) diff --git a/server/tests/api/server/follow-constraints.ts b/server/tests/api/server/follow-constraints.ts index 8a91fbba3..3f2f71f46 100644 --- a/server/tests/api/server/follow-constraints.ts +++ b/server/tests/api/server/follow-constraints.ts @@ -18,6 +18,7 @@ import { unfollow } from '../../../../shared/extra-utils/server/follows' import { userLogin } from '../../../../shared/extra-utils/users/login' import { createUser } from '../../../../shared/extra-utils/users/users' import { HttpStatusCode } from '../../../../shared/core-utils/miscs/http-error-codes' +import { PeerTubeProblemDocument, ServerErrorCode } from '@shared/models' const expect = chai.expect @@ -153,7 +154,20 @@ describe('Test follow constraints', function () { }) it('Should not get the remote video', async function () { - await getVideo(servers[0].url, video2UUID, HttpStatusCode.FORBIDDEN_403) + const res = await getVideo(servers[0].url, video2UUID, HttpStatusCode.FORBIDDEN_403) + + const error = res.body as PeerTubeProblemDocument + + const doc = 'https://docs.joinpeertube.org/api-rest-reference.html#section/Errors/does_not_respect_follow_constraints' + expect(error.type).to.equal(doc) + expect(error.code).to.equal(ServerErrorCode.DOES_NOT_RESPECT_FOLLOW_CONSTRAINTS) + + expect(error.detail).to.equal('Cannot get this video regarding follow constraints') + expect(error.error).to.equal(error.detail) + + expect(error.status).to.equal(HttpStatusCode.FORBIDDEN_403) + + expect(error.originUrl).to.contains(servers[1].url) }) it('Should list local account videos', async function () { diff --git a/server/typings/express/index.d.ts b/server/typings/express/index.d.ts index f58436ce1..cbbf40a78 100644 --- a/server/typings/express/index.d.ts +++ b/server/typings/express/index.d.ts @@ -1,3 +1,4 @@ + import { RegisterServerAuthExternalOptions } from '@server/types' import { MAbuseMessage, @@ -20,9 +21,8 @@ import { MVideoImportDefault } from '@server/types/models/video/video-import' import { MVideoPlaylistElement, MVideoPlaylistElementVideoUrlPlaylistPrivacy } from '@server/types/models/video/video-playlist-element' import { MAccountVideoRateAccountVideo } from '@server/types/models/video/video-rate' import { HttpMethod } from '@shared/core-utils/miscs/http-methods' -import { VideoCreate } from '@shared/models' +import { PeerTubeProblemDocumentData, ServerErrorCode, VideoCreate } from '@shared/models' import { File as UploadXFile, Metadata } from '@uploadx/core' -import { ProblemDocumentOptions } from 'http-problem-details/dist/ProblemDocument' import { RegisteredPlugin } from '../../lib/plugins/plugin-manager' import { MAccountDefault, @@ -41,6 +41,7 @@ import { MVideoThumbnail, MVideoWithRights } from '../../types/models' + declare module 'express' { export interface Request { query: any @@ -86,14 +87,20 @@ declare module 'express' { // Extends Response with added functions and potential variables passed by middlewares interface Response { - docs?: string fail: (options: { - data?: Record - docs?: string message: string - } & ProblemDocumentOptions) => void + + title?: string + status?: number + type?: ServerErrorCode + instance?: string + + data?: PeerTubeProblemDocumentData + }) => void locals: { + docUrl?: string + videoAll?: MVideoFullLight onlyImmutableVideo?: MVideoImmutable onlyVideo?: MVideoThumbnail -- cgit v1.2.3