From b0dc73a61c962e120133e8984a31dd4e67a62458 Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Wed, 22 Jul 2020 16:08:06 +0200 Subject: [PATCH] Remove HttpErrors.InternalServerError (#3027) * Added legacyInternal() 500 to HttpErrorHandler * replaced HttpErrors.InternalServerError being thrown with calls to HttpHandler.legacyInternal() GitOrigin-RevId: 0b7086a9693b57cdf93976d4221b90315960e8bb --- .../Collaborators/CollaboratorsController.js | 5 +- .../src/Features/Errors/HttpErrorHandler.js | 13 +++++ .../Subscription/SubscriptionController.js | 3 +- .../app/src/Features/User/UserController.js | 18 +++--- .../src/Features/User/UserEmailsController.js | 3 +- .../CollaboratorsControllerTests.js | 16 ++++++ .../unit/src/Errors/HttpErrorHandlerTests.js | 57 ++++++++++++++++++- .../SubscriptionControllerTests.js | 29 ++++++---- .../test/unit/src/User/UserControllerTests.js | 23 ++++---- .../src/User/UserEmailsControllerTests.js | 12 +++- 10 files changed, 137 insertions(+), 42 deletions(-) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsController.js b/services/web/app/src/Features/Collaborators/CollaboratorsController.js index ee82de8ea5..b2f1267b2a 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsController.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsController.js @@ -1,5 +1,4 @@ const OError = require('@overleaf/o-error') -const HttpErrors = require('@overleaf/o-error/http') const HttpErrorHandler = require('../../Features/Errors/HttpErrorHandler') const { ObjectId } = require('mongodb') const CollaboratorsHandler = require('./CollaboratorsHandler') @@ -72,7 +71,7 @@ async function setCollaboratorInfo(req, res, next) { if (err instanceof Errors.NotFoundError) { HttpErrorHandler.notFound(req, res) } else { - throw new HttpErrors.InternalServerError({}).withCause(err) + next(err) } } } @@ -103,7 +102,7 @@ async function transferOwnership(req, res, next) { `user ${toUserId} should be a collaborator in project ${projectId} prior to ownership transfer` ) } else { - throw new HttpErrors.InternalServerError({}).withCause(err) + next(err) } } } diff --git a/services/web/app/src/Features/Errors/HttpErrorHandler.js b/services/web/app/src/Features/Errors/HttpErrorHandler.js index c6857873ea..43e940b003 100644 --- a/services/web/app/src/Features/Errors/HttpErrorHandler.js +++ b/services/web/app/src/Features/Errors/HttpErrorHandler.js @@ -79,5 +79,18 @@ module.exports = { default: return res.send('unprocessable entity') } + }, + + legacyInternal(req, res, message, error) { + logger.error(error) + res.status(500) + switch (req.accepts(['html', 'json'])) { + case 'html': + return res.render('general/500', { title: 'Server Error' }) + case 'json': + return renderJSONError(res, message, {}) + default: + return res.send('internal server error') + } } } diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.js b/services/web/app/src/Features/Subscription/SubscriptionController.js index 9ede57951d..eb47c0833d 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.js +++ b/services/web/app/src/Features/Subscription/SubscriptionController.js @@ -33,7 +33,6 @@ const Errors = require('../Errors/Errors') const HttpErrorHandler = require('../Errors/HttpErrorHandler') const SubscriptionErrors = require('./Errors') const OError = require('@overleaf/o-error') -const HttpErrors = require('@overleaf/o-error/http') module.exports = SubscriptionController = { plansPage(req, res, next) { @@ -340,7 +339,7 @@ module.exports = SubscriptionController = { error ) { if (error) { - return next(new HttpErrors.InternalServerError({}).withCause(error)) + return next(error) } res.sendStatus(200) }) diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index 3ab2264403..db771d57ae 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -15,7 +15,6 @@ const SudoModeHandler = require('../SudoMode/SudoModeHandler') const Errors = require('../Errors/Errors') const HttpErrorHandler = require('../Errors/HttpErrorHandler') const OError = require('@overleaf/o-error') -const HttpErrors = require('@overleaf/o-error/http') const EmailHandler = require('../Email/EmailHandler') const UrlHelper = require('../Helpers/UrlHelper') const { promisify } = require('util') @@ -236,21 +235,20 @@ const UserController = { // update the user email UserUpdater.changeEmailAddress(userId, newEmail, err => { if (err) { - let errorData = { - message: 'problem updating users email address', - info: { userId, newEmail, public: {} } - } if (err instanceof Errors.EmailExistsError) { const translation = req.i18n.translate( 'email_already_registered' ) return HttpErrorHandler.conflict(req, res, translation) } else { - errorData.info.public.message = req.i18n.translate( - 'problem_changing_email_address' - ) - return next( - new HttpErrors.InternalServerError(errorData).withCause(err) + return HttpErrorHandler.legacyInternal( + req, + res, + req.i18n.translate('problem_changing_email_address'), + new OError({ + message: 'problem_changing_email_address', + info: { userId, newEmail } + }).withCause(err) ) } } diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index 5ab532e25a..b278985247 100644 --- a/services/web/app/src/Features/User/UserEmailsController.js +++ b/services/web/app/src/Features/User/UserEmailsController.js @@ -7,7 +7,6 @@ const UserEmailsConfirmationHandler = require('./UserEmailsConfirmationHandler') const { endorseAffiliation } = require('../Institutions/InstitutionsAPI') const Errors = require('../Errors/Errors') const HttpErrorHandler = require('../Errors/HttpErrorHandler') -const HttpErrors = require('@overleaf/o-error/http') function add(req, res, next) { const userId = AuthenticationController.getLoggedInUserId(req) @@ -167,6 +166,6 @@ module.exports = UserEmailsController = { const message = req.i18n.translate('email_does_not_belong_to_university') return HttpErrorHandler.conflict(req, res, message) } - next(new HttpErrors.InternalServerError().withCause(error)) + next(error) } } diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js index 7dc275d6f9..3ca9131ec3 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js @@ -236,6 +236,22 @@ describe('CollaboratorsController', function() { ) this.CollaboratorsController.setCollaboratorInfo(this.req, this.res) }) + + it('should pass the error to the next handler when setting the privilege level fails', function(done) { + this.next = sinon.spy(err => { + expect(err).instanceOf(Error) + done() + }) + + this.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel.rejects( + new Error() + ) + this.CollaboratorsController.setCollaboratorInfo( + this.req, + this.res, + this.next + ) + }) }) describe('transferOwnership', function() { diff --git a/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js b/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js index 93d763b8f9..7a5cdf144d 100644 --- a/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js +++ b/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js @@ -1,4 +1,5 @@ const { expect } = require('chai') +const sinon = require('sinon') const MockResponse = require('../helpers/MockResponse') const MockRequest = require('../helpers/MockRequest') const SandboxedModule = require('sandboxed-module') @@ -9,8 +10,16 @@ describe('HttpErrorHandler', function() { this.req = new MockRequest() this.res = new MockResponse() + this.logger = { + warn() {}, + error() {} + } + this.HttpErrorHandler = SandboxedModule.require(modulePath, { - globals: { console } + globals: { console }, + requires: { + 'logger-sharelatex': this.logger + } }) }) @@ -201,5 +210,51 @@ describe('HttpErrorHandler', function() { foo: 'bar' }) }) + + describe('legacyInternal', function() { + it('returns 500', function() { + this.HttpErrorHandler.legacyInternal(this.req, this.res, new Error()) + expect(this.res.statusCode).to.equal(500) + }) + + it('should send the error to the logger', function() { + const error = new Error('message') + this.logger.error = sinon.stub() + this.HttpErrorHandler.legacyInternal( + this.req, + this.res, + 'message', + error + ) + expect(this.logger.error).to.have.been.calledWith(error) + }) + + it('should print a message when no content-type is included', function() { + this.HttpErrorHandler.legacyInternal(this.req, this.res, new Error()) + expect(this.res.body).to.equal('internal server error') + }) + + it("should render a template when content-type is 'html'", function() { + this.req.accepts = () => 'html' + this.HttpErrorHandler.legacyInternal(this.req, this.res, new Error()) + expect(this.res.renderedTemplate).to.equal('general/500') + expect(this.res.renderedVariables).to.deep.equal({ + title: 'Server Error' + }) + }) + + it("should return a json object with a static message when content-type is 'json'", function() { + this.req.accepts = () => 'json' + this.HttpErrorHandler.legacyInternal( + this.req, + this.res, + 'a message', + new Error() + ) + expect(JSON.parse(this.res.body)).to.deep.equal({ + message: 'a message' + }) + }) + }) }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js index 4eb28a1e6e..f185c8b365 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js +++ b/services/web/test/unit/src/Subscription/SubscriptionControllerTests.js @@ -538,25 +538,32 @@ describe('SubscriptionController', function() { }) describe('updateAccountEmailAddress via put', function() { - beforeEach(function(done) { - this.res = { - sendStatus() { - return done() - } - } - sinon.spy(this.res, 'sendStatus') - this.SubscriptionController.updateAccountEmailAddress(this.req, this.res) - }) - it('should send the user and subscriptionId to RecurlyWrapper', function() { + this.res.sendStatus = sinon.spy() + this.SubscriptionController.updateAccountEmailAddress(this.req, this.res) this.RecurlyWrapper.updateAccountEmailAddress .calledWith(this.user._id, this.user.email) .should.equal(true) }) - it('shouldrespond with 200', function() { + it('should respond with 200', function() { + this.res.sendStatus = sinon.spy() + this.SubscriptionController.updateAccountEmailAddress(this.req, this.res) this.res.sendStatus.calledWith(200).should.equal(true) }) + + it('should send the error to the next handler when updating recurly account email fails', function(done) { + this.RecurlyWrapper.updateAccountEmailAddress.yields(new Error()) + this.next = sinon.spy(error => { + expect(error).instanceOf(Error) + done() + }) + this.SubscriptionController.updateAccountEmailAddress( + this.req, + this.res, + this.next + ) + }) }) describe('reactivateSubscription', function() { diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 2a3969e304..52d3983381 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -72,7 +72,8 @@ describe('UserController', function() { this.SudoModeHandler = { clearSudoMode: sinon.stub() } this.HttpErrorHandler = { conflict: sinon.stub(), - unprocessableEntity: sinon.stub() + unprocessableEntity: sinon.stub(), + legacyInternal: sinon.stub() } this.UserController = SandboxedModule.require(modulePath, { globals: { @@ -416,18 +417,16 @@ describe('UserController', function() { it('should pass on an error and not send a success status', function(done) { this.req.body.email = this.newEmail.toUpperCase() this.UserUpdater.changeEmailAddress.callsArgWith(2, new Error()) - const next = err => { - expect(err).to.exist - process.nextTick(() => { - // logic in User.findById - expect(this.res.send.called).to.equal(false) - expect(this.res.sendStatus.called).to.equal(false) - // logic after error handling - expect(this.User.findById.callCount).to.equal(1) + this.HttpErrorHandler.legacyInternal = sinon.spy( + (req, res, message, error) => { + expect(req).to.exist + expect(req).to.exist + message.should.equal('problem_changing_email_address') + expect(error).to.be.instanceof(OError) done() - }) - } - this.UserController.updateUserSettings(this.req, this.res, next) + } + ) + this.UserController.updateUserSettings(this.req, this.res, this.next) }) it('should call the HTTP conflict error handler when the email already exists', function(done) { diff --git a/services/web/test/unit/src/User/UserEmailsControllerTests.js b/services/web/test/unit/src/User/UserEmailsControllerTests.js index 5176b09164..75affe0e19 100644 --- a/services/web/test/unit/src/User/UserEmailsControllerTests.js +++ b/services/web/test/unit/src/User/UserEmailsControllerTests.js @@ -2,7 +2,7 @@ const sinon = require('sinon') const assertCalledWith = sinon.assert.calledWith const assertNotCalled = sinon.assert.notCalled const chai = require('chai') -const { assert } = chai +const { assert, expect } = chai const modulePath = '../../../../app/src/Features/User/UserEmailsController.js' const SandboxedModule = require('sandboxed-module') const MockRequest = require('../helpers/MockRequest') @@ -146,6 +146,16 @@ describe('UserEmailsController', function() { } }) }) + + it('should pass the error to the next handler when adding the email fails', function(done) { + this.UserUpdater.addEmailAddress.callsArgWith(3, new Error()) + this.next = sinon.spy(error => { + expect(error).instanceOf(Error) + done() + }) + this.UserEmailsController.add(this.req, this.res, this.next) + }) + it('should call the HTTP conflict handler when the email already exists', function(done) { this.UserUpdater.addEmailAddress.callsArgWith( 3,