From ccf406708e25f25dec54128e6f4bfad0ef479e8a Mon Sep 17 00:00:00 2001 From: Viljami Kuosmanen Date: Wed, 1 Apr 2020 18:18:02 +0200 Subject: [PATCH] openapi: support standard http error codes API errors are now handled at the end of the request heap by throwing exceptions from the handler --- src/node/handler/APIHandler.js | 13 ++--- src/node/hooks/express/openapi.js | 90 +++++++++++++++++++++---------- src/package-lock.json | 53 ++++++++++++++---- src/package.json | 1 + tests/backend/specs/api/pad.js | 2 +- 5 files changed, 111 insertions(+), 48 deletions(-) diff --git a/src/node/handler/APIHandler.js b/src/node/handler/APIHandler.js index aecc92e13..3685b28ad 100644 --- a/src/node/handler/APIHandler.js +++ b/src/node/handler/APIHandler.js @@ -25,6 +25,7 @@ var log4js = require('log4js'); var padManager = require("../db/PadManager"); var randomString = require("../utils/randomstring"); var argv = require('../utils/Cli').argv; +var createHTTPError = require('http-errors'); var apiHandlerLogger = log4js.getLogger('APIHandler'); @@ -153,25 +154,19 @@ exports.handle = async function(apiVersion, functionName, fields, req, res) { // say goodbye if this is an unknown API version if (!(apiVersion in version)) { - res.statusCode = 404; - res.send({code: 3, message: "no such api version", data: null}); - return; + throw new createHTTPError.NotFound('no such api version'); } // say goodbye if this is an unknown function if (!(functionName in version[apiVersion])) { - res.statusCode = 404; - res.send({code: 3, message: "no such function", data: null}); - return; + throw new createHTTPError.NotFound('no such function'); } // check the api key! fields["apikey"] = fields["apikey"] || fields["api_key"]; if (fields["apikey"] !== apikey.trim()) { - res.statusCode = 401; - res.send({code: 4, message: "no or wrong API Key", data: null}); - return; + throw new createHTTPError.Unauthorized('no or wrong API Key'); } // sanitize any padIDs before continuing diff --git a/src/node/hooks/express/openapi.js b/src/node/hooks/express/openapi.js index 816477929..26f898b96 100644 --- a/src/node/hooks/express/openapi.js +++ b/src/node/hooks/express/openapi.js @@ -16,6 +16,7 @@ const OpenAPIBackend = require('openapi-backend').default; const formidable = require('formidable'); const { promisify } = require('util'); const cloneDeep = require('lodash.clonedeep'); +const createHTTPError = require('http-errors'); const apiHandler = require('../../handler/APIHandler'); const settings = require('../../utils/Settings'); @@ -582,17 +583,15 @@ exports.expressCreateServer = async (_, args) => { // register default handlers api.register({ - notFound: (c, req, res) => { - res.statusCode = 404; - res.send({ code: 3, message: 'no such function', data: null }); + notFound: () => { + throw new createHTTPError.notFound('no such function'); }, - notImplemented: (c, req, res) => { - res.statusCode = 501; - res.send({ code: 3, message: 'not implemented', data: null }); + notImplemented: () => { + throw new createHTTPError.notImplemented('function not implemented'); }, }); - // register operation handlers (calls apiHandler.handle) + // register operation handlers for (const funcName in apiHandler.version[version]) { const handler = async (c, req, res) => { // parse fields from request @@ -612,24 +611,31 @@ exports.expressCreateServer = async (_, args) => { apiLogger.info(`REQUEST, v${version}:${funcName}, ${JSON.stringify(fields)}`); // pass to api handler - let data = await apiHandler.handle(version, funcName, fields, req, res); - if (!data) { - data = null; - } + let data = await apiHandler.handle(version, funcName, fields, req, res).catch((err) => { + // convert all errors to http errors + if (err instanceof createHTTPError.HttpError) { + // pass http errors thrown by handler forward + throw err; + } else if (err.name == 'apierror') { + // parameters were wrong and the api stopped execution, pass the error + // convert to http error + throw new createHTTPError.BadRequest(err.message); + } else { + // an unknown error happened + // log it and throw internal error + apiLogger.error(err); + throw new createHTTPError.InternalError('internal error'); + } + }); // return in common format - let response = { code: 0, message: 'ok', data }; + let response = { code: 0, message: 'ok', data: data || null }; // log response apiLogger.info(`RESPONSE, ${funcName}, ${JSON.stringify(response)}`); - // is this a jsonp call, add the function call - if (query.jsonp && isValidJSONPName.check(query.jsonp)) { - res.header('Content-Type', 'application/javascript'); - response = `${req.query.jsonp}(${JSON.stringify(response)}`; - } - - res.send(response); + // return the response data + return response; }; // each operation can be called with either GET or POST @@ -640,23 +646,53 @@ exports.expressCreateServer = async (_, args) => { // start and bind to express api.init(); app.use(apiRoot, async (req, res) => { + let response = null; try { if (style === APIPathStyle.REST) { // @TODO: Don't allow CORS from everywhere // This is purely to maintain compatibility with old swagger-node-express res.header('Access-Control-Allow-Origin', '*'); } - await api.handleRequest(req, req, res); + // pass to openapi-backend handler + response = await api.handleRequest(req, req, res); } catch (err) { - if (err.name == 'apierror') { - // parameters were wrong and the api stopped execution, pass the error - res.send({ code: 1, message: err.message, data: null }); - } else { - // an unknown error happened - res.send({ code: 2, message: 'internal error', data: null }); - throw err; + // handle http errors + res.statusCode = err.statusCode || 500; + + // convert to our json response format + // https://github.com/ether/etherpad-lite/tree/master/doc/api/http_api.md#response-format + switch (res.statusCode) { + case 403: // forbidden + response = { code: 4, message: err.message, data: null }; + break; + case 401: // unauthorized (no or wrong api key) + response = { code: 4, message: err.message, data: null }; + break; + case 404: // not found (no such function) + response = { code: 3, message: err.message, data: null }; + break; + case 500: // server error (internal error) + response = { code: 2, message: err.message, data: null }; + break; + case 400: // bad request (wrong parameters) + // respond with 200 OK to keep old behavior and pass tests + res.statusCode = 200; // @TODO: this is bad api design + response = { code: 1, message: err.message, data: null }; + break; + default: + response = { code: 1, message: err.message, data: null }; + break; } } + + // support jsonp response format + if (req.query.jsonp && isValidJSONPName.check(req.query.jsonp)) { + res.header('Content-Type', 'application/javascript'); + response = `${req.query.jsonp}(${JSON.stringify(response)}`; + } + + // send response + return res.send(response); }); } } diff --git a/src/package-lock.json b/src/package-lock.json index d1ccfa770..19e24bef3 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -821,6 +821,25 @@ "qs": "6.7.0", "raw-body": "2.4.0", "type-is": "~1.6.17" + }, + "dependencies": { + "http-errors": { + "version": "1.7.2", + "resolved": "https://registry.npmjs.org/http-errors/-/http-errors-1.7.2.tgz", + "integrity": "sha512-uUQBt3H/cSIVfch6i1EuPNy/YsRSOUBXTVfZ+yR7Zjez3qjBz6i9+i4zjNaoqcoFVI4lQJ5plg63TvGfRSDCRg==", + "requires": { + "depd": "~1.1.2", + "inherits": "2.0.3", + "setprototypeof": "1.1.1", + "statuses": ">= 1.5.0 < 2", + "toidentifier": "1.0.0" + } + }, + "inherits": { + "version": "2.0.3", + "resolved": "https://registry.npmjs.org/inherits/-/inherits-2.0.3.tgz", + "integrity": "sha1-Yzwsg+PaQqUC9SRmAiSA9CCCYd4=" + } } }, "boolbase": { @@ -1945,22 +1964,15 @@ } }, "http-errors": { - "version": "1.7.2", - "resolved": "https://registry.npmjs.org/http-errors/-/http-errors-1.7.2.tgz", - "integrity": "sha512-uUQBt3H/cSIVfch6i1EuPNy/YsRSOUBXTVfZ+yR7Zjez3qjBz6i9+i4zjNaoqcoFVI4lQJ5plg63TvGfRSDCRg==", + "version": "1.7.3", + "resolved": "https://registry.npmjs.org/http-errors/-/http-errors-1.7.3.tgz", + "integrity": "sha512-ZTTX0MWrsQ2ZAhA1cejAwDLycFsd7I7nVtnkT3Ol0aqodaKW+0CTZDQ1uBv5whptCnc8e8HeRRJxRs0kmm/Qfw==", "requires": { "depd": "~1.1.2", - "inherits": "2.0.3", + "inherits": "2.0.4", "setprototypeof": "1.1.1", "statuses": ">= 1.5.0 < 2", "toidentifier": "1.0.0" - }, - "dependencies": { - "inherits": { - "version": "2.0.3", - "resolved": "https://registry.npmjs.org/inherits/-/inherits-2.0.3.tgz", - "integrity": "sha1-Yzwsg+PaQqUC9SRmAiSA9CCCYd4=" - } } }, "http-signature": { @@ -6671,6 +6683,25 @@ "http-errors": "1.7.2", "iconv-lite": "0.4.24", "unpipe": "1.0.0" + }, + "dependencies": { + "http-errors": { + "version": "1.7.2", + "resolved": "https://registry.npmjs.org/http-errors/-/http-errors-1.7.2.tgz", + "integrity": "sha512-uUQBt3H/cSIVfch6i1EuPNy/YsRSOUBXTVfZ+yR7Zjez3qjBz6i9+i4zjNaoqcoFVI4lQJ5plg63TvGfRSDCRg==", + "requires": { + "depd": "~1.1.2", + "inherits": "2.0.3", + "setprototypeof": "1.1.1", + "statuses": ">= 1.5.0 < 2", + "toidentifier": "1.0.0" + } + }, + "inherits": { + "version": "2.0.3", + "resolved": "https://registry.npmjs.org/inherits/-/inherits-2.0.3.tgz", + "integrity": "sha1-Yzwsg+PaQqUC9SRmAiSA9CCCYd4=" + } } }, "readable-stream": { diff --git a/src/package.json b/src/package.json index 2c0130555..fed9c9a22 100644 --- a/src/package.json +++ b/src/package.json @@ -44,6 +44,7 @@ "find-root": "1.1.0", "formidable": "1.2.1", "graceful-fs": "4.2.2", + "http-errors": "^1.7.3", "jsonminify": "0.4.1", "languages4translatewiki": "0.1.3", "lodash.clonedeep": "^4.5.0", diff --git a/tests/backend/specs/api/pad.js b/tests/backend/specs/api/pad.js index 2c3ad2fd2..67650beb9 100644 --- a/tests/backend/specs/api/pad.js +++ b/tests/backend/specs/api/pad.js @@ -111,7 +111,7 @@ describe('deletePad', function(){ it('deletes a Pad', function(done) { api.get(endPoint('deletePad')+"&padID="+testPadId) .expect('Content-Type', /json/) - .expect(200, done) + .expect(200, done) // @TODO: we shouldn't expect 200 here since the pad may not exist }); })