From 5ce09a2d4a848bf89bf4eeb892aa063a835e9adc Mon Sep 17 00:00:00 2001 From: "Juan Picado @jotadeveloper" Date: Fri, 22 Jun 2018 19:12:56 +0200 Subject: [PATCH] refactor: better usage of http error functions --- src/api/endpoint/api/package.js | 2 +- src/api/endpoint/api/publish.js | 6 +++--- src/api/endpoint/api/user.js | 2 +- src/api/middleware.js | 8 +++---- src/lib/auth.js | 10 ++++----- src/lib/constants.js | 1 + src/lib/local-storage.js | 38 ++++++++++++++++----------------- src/lib/storage-utils.js | 6 +++--- src/lib/storage.js | 4 ++-- src/lib/up-storage.js | 16 +++++++------- src/lib/utils.js | 35 ++++++++++++++++-------------- 11 files changed, 66 insertions(+), 62 deletions(-) diff --git a/src/api/endpoint/api/package.js b/src/api/endpoint/api/package.js index ce1de1765..f2b8b9765 100644 --- a/src/api/endpoint/api/package.js +++ b/src/api/endpoint/api/package.js @@ -37,7 +37,7 @@ export default function(route: Router, auth: IAuth, storage: IStorageHandler, co } } } - return next(ErrorCode.get404(`version not found: ${req.params.version}`)); + return next(ErrorCode.getNotFound(`version not found: ${req.params.version}`)); }; storage.getPackage({ diff --git a/src/api/endpoint/api/publish.js b/src/api/endpoint/api/publish.js index 8cfc7b874..e1dbd52a2 100644 --- a/src/api/endpoint/api/publish.js +++ b/src/api/endpoint/api/publish.js @@ -67,7 +67,7 @@ export default function(router: Router, auth: IAuth, storage: IStorageHandler, c || Object.keys(metadata.versions).length !== 1) { // npm is doing something strange again // if this happens in normal circumstances, report it as a bug - return next(ErrorCode.get400('unsupported registry call')); + return next(ErrorCode.getBadRequest('unsupported registry call')); } if (err && err.status != 409) { @@ -108,13 +108,13 @@ export default function(router: Router, auth: IAuth, storage: IStorageHandler, c if (Object.keys(req.body).length === 1 && isObject(req.body.users)) { // 501 status is more meaningful, but npm doesn't show error message for 5xx - return next(ErrorCode.get404('npm star|unstar calls are not implemented')); + return next(ErrorCode.getNotFound('npm star|unstar calls are not implemented')); } try { metadata = validate_metadata(req.body, name); } catch (err) { - return next(ErrorCode.get422('bad incoming package data')); + return next(ErrorCode.getBadData('bad incoming package data')); } if (req.params._rev) { diff --git a/src/api/endpoint/api/user.js b/src/api/endpoint/api/user.js index 908aa3161..d68f67b23 100644 --- a/src/api/endpoint/api/user.js +++ b/src/api/endpoint/api/user.js @@ -32,7 +32,7 @@ export default function(route: Router, auth: IAuth) { // With npm registering is the same as logging in, // and npm accepts only an 409 error. // So, changing status code here. - return next( ErrorCode.getCode(err.status, err.message) || ErrorCode.get409(err.message)); + return next( ErrorCode.getCode(err.status, err.message) || ErrorCode.getConflict(err.message)); } return next(err); } diff --git a/src/api/middleware.js b/src/api/middleware.js index 627c707c9..28e67ba3e 100644 --- a/src/api/middleware.js +++ b/src/api/middleware.js @@ -38,7 +38,7 @@ export function validateName(req: $RequestExtend, res: $ResponseExtend, next: $N } else if (utilValidateName(value)) { next(); } else { - next( ErrorCode.get403('invalid ' + name)); + next( ErrorCode.getForbidden('invalid ' + name)); } } @@ -51,7 +51,7 @@ export function validatePackage(req: $RequestExtend, res: $ResponseExtend, next: } else if (utilValidatePackage(value)) { next(); } else { - next( ErrorCode.get403('invalid ' + name) ); + next( ErrorCode.getForbidden('invalid ' + name) ); } } @@ -76,7 +76,7 @@ export function encodeScopePackage(req: $RequestExtend, res: $ResponseExtend, ne export function expectJson(req: $RequestExtend, res: $ResponseExtend, next: $NextFunctionVer) { if (!isObject(req.body)) { - return next( ErrorCode.get400('can\'t parse incoming json') ); + return next( ErrorCode.getBadRequest('can\'t parse incoming json') ); } next(); } @@ -115,7 +115,7 @@ export function allow(auth: IAuth) { } else { // last plugin (that's our built-in one) returns either // cb(err) or cb(null, true), so this should never happen - throw ErrorCode.get500('bug in the auth plugin system'); + throw ErrorCode.getInternalError('bug in the auth plugin system'); } }); }; diff --git a/src/lib/auth.js b/src/lib/auth.js index 111ef7924..20ad2f005 100644 --- a/src/lib/auth.js +++ b/src/lib/auth.js @@ -53,20 +53,20 @@ class Auth { } if (user.name) { - cb(ErrorCode.get403('user ' + user.name + ' is not allowed to ' + action + ' package ' + pkg.name)); + cb(ErrorCode.getForbidden('user ' + user.name + ' is not allowed to ' + action + ' package ' + pkg.name)); } else { - cb(ErrorCode.get403('unregistered users are not allowed to ' + action + ' package ' + pkg.name)); + cb(ErrorCode.getForbidden('unregistered users are not allowed to ' + action + ' package ' + pkg.name)); } }; }; this.plugins.push({ authenticate: function(user, password, cb) { - cb(ErrorCode.get403('bad username/password, access denied')); + cb(ErrorCode.getForbidden('bad username/password, access denied')); }, add_user: function(user, password, cb) { - return cb(ErrorCode.get409('bad username/password, access denied')); + return cb(ErrorCode.getConflict('bad username/password, access denied')); }, allow_access: allow_action('access'), @@ -224,7 +224,7 @@ class Auth { const parts = authorization.split(' '); if (parts.length !== 2) { - return next( ErrorCode.get400('bad authorization header') ); + return next( ErrorCode.getBadRequest('bad authorization header') ); } const credentials = this._parseCredentials(parts); diff --git a/src/lib/constants.js b/src/lib/constants.js index c3407c00f..297a4fe2f 100644 --- a/src/lib/constants.js +++ b/src/lib/constants.js @@ -31,6 +31,7 @@ export const HTTP_STATUS = { FORBIDDEN: 403, NOT_FOUND: 404, CONFLICT: 409, + BAD_DATA: 422, }; export const PORT_SERVER_1 = '55551'; diff --git a/src/lib/local-storage.js b/src/lib/local-storage.js index 1f1a22bec..64a336c9a 100644 --- a/src/lib/local-storage.js +++ b/src/lib/local-storage.js @@ -53,12 +53,12 @@ class LocalStorage implements IStorage { const storage: any = this._getLocalStorage(name); if (_.isNil(storage)) { - return callback( ErrorCode.get404('this package cannot be added')); + return callback( ErrorCode.getNotFound('this package cannot be added')); } storage.createPackage(name, generatePackageTemplate(name), (err) => { if (_.isNull(err) === false && err.code === fileExist) { - return callback( ErrorCode.get409()); + return callback( ErrorCode.getConflict()); } const latest = getLatestVersion(pkg); @@ -80,13 +80,13 @@ class LocalStorage implements IStorage { let storage: any = this._getLocalStorage(name); if (_.isNil(storage)) { - return callback( ErrorCode.get404()); + return callback( ErrorCode.getNotFound()); } storage.readPackage(name, (err, data) => { if (_.isNil(err) === false) { if (err.code === noSuchFile) { - return callback( ErrorCode.get404()); + return callback( ErrorCode.getNotFound()); } else { return callback(err); } @@ -97,7 +97,7 @@ class LocalStorage implements IStorage { this.localData.remove(name, (removeFailed) => { if (removeFailed) { // This will happen when database is locked - return callback(ErrorCode.get422(removeFailed.message)); + return callback(ErrorCode.getBadData(removeFailed.message)); } storage.deletePackage(pkgFileName, (err) => { @@ -220,7 +220,7 @@ class LocalStorage implements IStorage { metadata = cleanUpReadme(metadata); if (data.versions[version] != null) { - return cb( ErrorCode.get409() ); + return cb( ErrorCode.getConflict() ); } // if uploaded tarball has a different shasum, it's very likely that we have some kind of error @@ -231,7 +231,7 @@ class LocalStorage implements IStorage { if (_.isNil(data._attachments[tarball].shasum) === false && _.isNil(metadata.dist.shasum) === false) { if (data._attachments[tarball].shasum != metadata.dist.shasum) { const errorMessage = `shasum error, ${data._attachments[tarball].shasum} != ${metadata.dist.shasum}`; - return cb( ErrorCode.get400(errorMessage) ); + return cb( ErrorCode.getBadRequest(errorMessage) ); } } @@ -258,7 +258,7 @@ class LocalStorage implements IStorage { this.localData.add(name, (addFailed) => { if (addFailed) { - return cb(ErrorCode.get422(addFailed.message)); + return cb(ErrorCode.getBadData(addFailed.message)); } cb(); @@ -298,7 +298,7 @@ class LocalStorage implements IStorage { * @private */ _getVersionNotFound() { - return ErrorCode.get404('this version doesn\'t exist'); + return ErrorCode.getNotFound('this version doesn\'t exist'); } /** @@ -307,7 +307,7 @@ class LocalStorage implements IStorage { * @private */ _getFileNotAvailable() { - return ErrorCode.get404('no such file available'); + return ErrorCode.getNotFound('no such file available'); } /** @@ -323,7 +323,7 @@ class LocalStorage implements IStorage { pkg: Package, revision?: string, callback: Callback) { if (!isObject(pkg.versions) || !isObject(pkg[DIST_TAGS])) { - return callback( ErrorCode.get422()); + return callback( ErrorCode.getBadData()); } this._updatePackage(name, (jsonData, cb) => { @@ -407,7 +407,7 @@ class LocalStorage implements IStorage { if (name === pkgFileName || name === '__proto__') { process.nextTick(() => { - uploadStream.emit('error', ErrorCode.get403()); + uploadStream.emit('error', ErrorCode.getForbidden()); }); return uploadStream; } @@ -423,7 +423,7 @@ class LocalStorage implements IStorage { writeStream.on('error', (err) => { if (err.code === fileExist) { - uploadStream.emit('error', ErrorCode.get409()); + uploadStream.emit('error', ErrorCode.getConflict()); } else if (err.code === noSuchFile) { // check if package exists to throw an appropriate message this.getPackageMetadata(name, function(_err, res) { @@ -464,7 +464,7 @@ class LocalStorage implements IStorage { uploadStream.done = function() { if (!length) { - uploadStream.emit('error', ErrorCode.get422('refusing to accept zero-length file')); + uploadStream.emit('error', ErrorCode.getBadData('refusing to accept zero-length file')); writeStream.abort(); } else { writeStream.done(); @@ -518,7 +518,7 @@ class LocalStorage implements IStorage { _streamSuccessReadTarBall(storage: any, filename: string): IReadTarball { const stream: IReadTarball = new ReadTarball(); const readTarballStream = storage.readTarball(filename); - const e404 = ErrorCode.get404; + const e404 = ErrorCode.getNotFound; stream.abort = function() { if (_.isNil(readTarballStream) === false) { @@ -556,7 +556,7 @@ class LocalStorage implements IStorage { getPackageMetadata(name: string, callback?: Callback = () => {}): void { const storage: IPackageStorage = this._getLocalStorage(name); if (_.isNil(storage)) { - return callback( ErrorCode.get404() ); + return callback( ErrorCode.getNotFound() ); } this._readPackage(name, storage, callback); @@ -615,7 +615,7 @@ class LocalStorage implements IStorage { storage.readPackage(name, (err, result) => { if (err) { if (err.code === noSuchFile) { - return callback( ErrorCode.get404() ); + return callback( ErrorCode.getNotFound() ); } else { return callback(this._internalError(err, pkgFileName, 'error reading')); } @@ -703,7 +703,7 @@ class LocalStorage implements IStorage { _internalError(err: string, file: string, message: string) { this.logger.error( {err: err, file: file}, `${message} @{file}: @{!err.message}` ); - return ErrorCode.get500(); + return ErrorCode.getInternalError(); } /** @@ -716,7 +716,7 @@ class LocalStorage implements IStorage { const storage: IPackageStorage = this._getLocalStorage(name); if (!storage) { - return callback( ErrorCode.get404() ); + return callback( ErrorCode.getNotFound() ); } storage.updatePackage(name, updateHandler, this._writePackage.bind(this), normalizePackage, diff --git a/src/lib/storage-utils.js b/src/lib/storage-utils.js index 5733eb696..f4790ebc6 100644 --- a/src/lib/storage-utils.js +++ b/src/lib/storage-utils.js @@ -127,7 +127,7 @@ export function checkPackageLocal(name: string, localStorage: IStorage): Promise return reject(err); } if (results) { - return reject(ErrorCode.get409('this package is already present')); + return reject(ErrorCode.getConflict('this package is already present')); } return resolve(); }); @@ -158,7 +158,7 @@ export function checkPackageRemote(name: string, isAllowPublishOffline: boolean, // checking package exist already if (_.isNil(packageJsonLocal) === false) { - return reject(ErrorCode.get409('this package is already present')); + return reject(ErrorCode.getConflict('this package is already present')); } for (let errorItem = 0; errorItem < upLinksErrors.length; errorItem++) { @@ -170,7 +170,7 @@ export function checkPackageRemote(name: string, isAllowPublishOffline: boolean, return resolve(); } - return reject(ErrorCode.get503('one of the uplinks is down, refuse to publish')); + return reject(ErrorCode.getServiceUnavailable('one of the uplinks is down, refuse to publish')); } } } diff --git a/src/lib/storage.js b/src/lib/storage.js index 67135a95a..76b34dc44 100644 --- a/src/lib/storage.js +++ b/src/lib/storage.js @@ -438,7 +438,7 @@ class Storage implements IStorageHandler { if (err || !upLinkResponse) { // $FlowFixMe - return cb(null, [err || ErrorCode.get500('no data')]); + return cb(null, [err || ErrorCode.getInternalError('no data')]); } try { @@ -478,7 +478,7 @@ class Storage implements IStorageHandler { }, (err: Error, upLinksErrors: any) => { assert(!err && Array.isArray(upLinksErrors)); if (!exists) { - return callback( ErrorCode.get404('no such package available') + return callback( ErrorCode.getNotFound('no such package available') , null , upLinksErrors ); } diff --git a/src/lib/up-storage.js b/src/lib/up-storage.js index da6faaf57..f389d68ef 100644 --- a/src/lib/up-storage.js +++ b/src/lib/up-storage.js @@ -105,10 +105,10 @@ class ProxyStorage implements IProxy { process.nextTick(function() { if (cb) { - cb(ErrorCode.get500('uplink is offline')); + cb(ErrorCode.getInternalError('uplink is offline')); } // $FlowFixMe - streamRead.emit('error', ErrorCode.get500('uplink is offline')); + streamRead.emit('error', ErrorCode.getInternalError('uplink is offline')); }); // $FlowFixMe streamRead._read = function() {}; @@ -408,10 +408,10 @@ class ProxyStorage implements IProxy { return callback(err); } if (res.statusCode === 404) { - return callback( ErrorCode.get404('package doesn\'t exist on uplink')); + return callback( ErrorCode.getNotFound('package doesn\'t exist on uplink')); } if (!(res.statusCode >= 200 && res.statusCode < 300)) { - const error = ErrorCode.get500(`bad status code: ${res.statusCode}`); + const error = ErrorCode.getInternalError(`bad status code: ${res.statusCode}`); // $FlowFixMe error.remoteStatus = res.statusCode; return callback(error); @@ -441,10 +441,10 @@ class ProxyStorage implements IProxy { readStream.on('response', function(res: any) { if (res.statusCode === 404) { - return stream.emit('error', ErrorCode.get404('file doesn\'t exist on uplink')); + return stream.emit('error', ErrorCode.getNotFound('file doesn\'t exist on uplink')); } if (!(res.statusCode >= 200 && res.statusCode < 300)) { - return stream.emit('error', ErrorCode.get500(`bad uplink status code: ${res.statusCode}`)); + return stream.emit('error', ErrorCode.getInternalError(`bad uplink status code: ${res.statusCode}`)); } if (res.headers['content-length']) { expected_length = res.headers['content-length']; @@ -465,7 +465,7 @@ class ProxyStorage implements IProxy { current_length += data.length; } if (expected_length && current_length != expected_length) { - stream.emit('error', ErrorCode.get500('content length mismatch')); + stream.emit('error', ErrorCode.getInternalError('content length mismatch')); } }); return stream; @@ -494,7 +494,7 @@ class ProxyStorage implements IProxy { requestStream.on('response', (res) => { if (!String(res.statusCode).match(/^2\d\d$/)) { - return transformStream.emit('error', ErrorCode.get500(`bad status code ${res.statusCode} from uplink`)); + return transformStream.emit('error', ErrorCode.getInternalError(`bad status code ${res.statusCode} from uplink`)); } // See https://github.com/request/request#requestoptions-callback diff --git a/src/lib/utils.js b/src/lib/utils.js index bce346e1b..477e3348a 100644 --- a/src/lib/utils.js +++ b/src/lib/utils.js @@ -7,11 +7,13 @@ import YAML from 'js-yaml'; import URL from 'url'; import fs from 'fs'; import _ from 'lodash'; +import asciidoctor from 'asciidoctor.js'; import createError from 'http-errors'; +import marked from 'marked'; +import {HTTP_STATUS} from './constants'; + import type {Package} from '@verdaccio/types'; import type {$Request} from 'express'; -import marked from 'marked'; -import asciidoctor from 'asciidoctor.js'; import type {StringValue} from '../../types'; const Logger = require('./logger'); @@ -337,26 +339,27 @@ const getLatestVersion = function(pkgInfo: Package) { }; const ErrorCode = { - get409: (message: string = 'this package is already present') => { - return createError(409, message); + getConflict: (message: string = 'this package is already present') => { + return createError(HTTP_STATUS.CONFLICT, message); }, - get422: (customMessage?: string) => { - return createError(422, customMessage || 'bad data'); + getBadData: (customMessage?: string) => { + return createError(HTTP_STATUS.BAD_DATA, customMessage || 'bad data'); }, - get400: (customMessage?: string) => { - return createError(400, customMessage); + getBadRequest: (customMessage?: string) => { + return createError(HTTP_STATUS.BAD_REQUEST, customMessage); }, - get500: (customMessage?: string) => { - return customMessage ? createError(500, customMessage) : createError(500); + getInternalError: (customMessage?: string) => { + return customMessage ? createError(HTTP_STATUS.INTERNAL_ERROR, customMessage) + : createError(HTTP_STATUS.INTERNAL_ERROR); }, - get403: (message: string = 'can\'t use this filename') => { - return createError(403, message); + getForbidden: (message: string = 'can\'t use this filename') => { + return createError(HTTP_STATUS.FORBIDDEN, message); }, - get503: (message: string = 'resource temporarily unavailable') => { - return createError(503, message); + getServiceUnavailable: (message: string = 'resource temporarily unavailable') => { + return createError(HTTP_STATUS.SERVICE_UNAVAILABLE, message); }, - get404: (customMessage?: string) => { - return createError(404, customMessage || 'no such package available'); + getNotFound: (customMessage?: string) => { + return createError(HTTP_STATUS.NOT_FOUND, customMessage || 'no such package available'); }, getCode: (statusCode: number, customMessage: string) => { return createError(statusCode, customMessage);