Merge pull request #3046 from overleaf/msm-cleanup-oerror-http

Finish o-error/http cleanup

GitOrigin-RevId: 1f8cf7f1e0503d1071c51b41ac136f7fb7f38872

services/web/app/src/Features/Errors/HttpErrorController.js

@@ -1,64 +0,0 @@
-const logger = require('logger-sharelatex')
-const OError = require('@overleaf/o-error')
-const HttpErrors = require('@overleaf/o-error/http')
-const AuthenticationController = require('../Authentication/AuthenticationController')
-
-function renderHTMLError(statusCode, publicInfo, res) {
-  res.status(statusCode)
-  if (statusCode === 404) {
-    res.render('general/404', { title: 'page_not_found' })
-  } else if (statusCode === 403) {
-    res.render('user/restricted', { title: 'restricted' })
-  } else if (statusCode >= 400 && statusCode < 500) {
-    res.render('general/400', {
-      title: 'Client Error',
-      message: publicInfo.message
-    })
-  } else {
-    res.render('general/500', { title: 'Server Error' })
-  }
-}
-
-function renderJSONError(statusCode, publicInfo, res) {
-  res.status(statusCode).json(publicInfo)
-}
-
-function renderError(error, req, res) {
-  const publicInfo = OError.getFullInfo(error).public || {}
-
-  switch (req.accepts(['html', 'json'])) {
-    case 'html':
-      renderHTMLError(error.statusCode, publicInfo, res)
-      break
-    case 'json':
-      renderJSONError(error.statusCode, publicInfo, res)
-      break
-    default:
-      res.sendStatus(error.statusCode)
-  }
-}
-
-function logError(error, req) {
-  const userId = AuthenticationController.getLoggedInUserId(req)
-
-  let logLevel
-  if (error.statusCode >= 400 && error.statusCode < 500) {
-    logLevel = 'warn'
-  } else {
-    logLevel = 'error'
-  }
-
-  logger[logLevel]({ error, url: req.url, method: req.method, userId })
-}
-
-module.exports = {
-  handleError(error, req, res, next) {
-    // Only handles HttpErrors
-    if (!(error instanceof HttpErrors.HttpError)) {
-      return next(error)
-    }
-
-    logError(error, req)
-    renderError(error, req, res)
-  }
-}

services/web/app/src/Features/Errors/HttpErrorHandler.js

@@ -1,34 +1,85 @@
 const logger = require('logger-sharelatex')
 const Settings = require('settings-sharelatex')
 
-function renderJSONError(res, message, info) {
-  const fullInfo = { ...info, message }
+function renderJSONError(res, message, info = {}) {
   if (info.message) {
     logger.warn(
       info,
       `http error info shouldn't contain a 'message' field, will be overridden`
     )
   }
-  res.json(fullInfo)
+  if (message != null) {
+    res.json({ ...info, message })
+  } else {
+    res.json(info)
+  }
 }
 
-module.exports = {
-  badRequest(req, res, message, info) {
-    res.status(400)
-    switch (req.accepts(['html', 'json'])) {
-      case 'html':
-        return res.render('general/400', {
-          title: 'Client Error',
-          message: message
-        })
-      case 'json':
-        return renderJSONError(res, message, info || {})
-      default:
-        return res.send('client error')
+function handleGeneric500Error(req, res, statusCode, message) {
+  res.status(statusCode)
+  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')
+  }
+}
+
+function handleGeneric400Error(req, res, statusCode, message, info = {}) {
+  res.status(statusCode)
+  switch (req.accepts(['html', 'json'])) {
+    case 'html':
+      return res.render('general/400', {
+        title: 'Client Error',
+        message: message
+      })
+    case 'json':
+      return renderJSONError(res, message, info)
+    default:
+      return res.send('client error')
+  }
+}
+
+let HttpErrorHandler
+module.exports = HttpErrorHandler = {
+  handleErrorByStatusCode(req, res, error, statusCode) {
+    const is400Error = statusCode >= 400 && statusCode < 500
+    const is500Error = statusCode >= 500 && statusCode < 600
+
+    if (is400Error) {
+      logger.warn(error)
+    } else if (is500Error) {
+      logger.error(error)
+    }
+
+    if (statusCode === 403) {
+      HttpErrorHandler.forbidden(req, res)
+    } else if (statusCode === 404) {
+      HttpErrorHandler.notFound(req, res)
+    } else if (statusCode === 409) {
+      HttpErrorHandler.conflict(req, res, '')
+    } else if (statusCode === 422) {
+      HttpErrorHandler.unprocessableEntity(req, res)
+    } else if (is400Error) {
+      handleGeneric400Error(req, res, statusCode)
+    } else if (is500Error) {
+      handleGeneric500Error(req, res, statusCode)
+    } else {
+      logger.error(
+        { err: error, statusCode },
+        `unable to handle error with status code ${statusCode}`
+      )
+      res.sendStatus(500)
     }
   },
 
-  conflict(req, res, message, info) {
+  badRequest(req, res, message, info = {}) {
+    handleGeneric400Error(req, res, 400, message, info)
+  },
+
+  conflict(req, res, message, info = {}) {
     res.status(409)
     switch (req.accepts(['html', 'json'])) {
       case 'html':
@@ -37,7 +88,7 @@ module.exports = {
           message: message
         })
       case 'json':
-        return renderJSONError(res, message, info || {})
+        return renderJSONError(res, message, info)
       default:
         return res.send('conflict')
     }
@@ -84,15 +135,7 @@ module.exports = {
 
   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')
-    }
+    handleGeneric500Error(req, res, 500, message)
   },
 
   maintenance(req, res) {

services/web/app/src/infrastructure/BodyParserWrapper.js

@@ -1,26 +1,28 @@
-const HttpErrors = require('@overleaf/o-error/http')
 const bodyParser = require('body-parser')
+const HttpErrorHandler = require('../Features/Errors/HttpErrorHandler')
 
-const convertToHTTPError = error => {
-  if (!error.statusCode || error.statusCode < 400 || error.statusCode >= 600) {
-    // cannot be converted to a HttpError
-    return error
+function isBodyParserError(nextArg) {
+  if (nextArg instanceof Error) {
+    return (
+      nextArg.statusCode &&
+      nextArg.statusCode >= 400 &&
+      nextArg.statusCode < 600
+    )
   }
-
-  return new HttpErrors.HttpError({
-    message: error.message,
-    statusCode: error.statusCode
-  }).withCause(error)
+  return false
 }
 
-// Wraps a parser and attempt to wrap its error (if any) into a HTTPError so the
-// response code is forwarded to the client
 const wrapBodyParser = method => opts => {
   const middleware = bodyParser[method](opts)
   return (req, res, next) => {
     middleware(req, res, nextArg => {
-      if (nextArg instanceof Error) {
-        return next(convertToHTTPError(nextArg))
+      if (isBodyParserError(nextArg)) {
+        return HttpErrorHandler.handleErrorByStatusCode(
+          req,
+          res,
+          nextArg,
+          nextArg.statusCode
+        )
       }
       next(nextArg)
     })

services/web/app/src/infrastructure/Server.js

@@ -33,7 +33,6 @@ const Modules = require('./Modules')
 const Views = require('./Views')
 
 const ErrorController = require('../Features/Errors/ErrorController')
-const HttpErrorController = require('../Features/Errors/HttpErrorController')
 const HttpErrorHandler = require('../Features/Errors/HttpErrorHandler')
 const UserSessionsManager = require('../Features/User/UserSessionsManager')
 const AuthenticationController = require('../Features/Authentication/AuthenticationController')
@@ -233,7 +232,6 @@ if (enableApiRouter || notDefined(enableApiRouter)) {
   logger.info('providing api router')
   app.use(privateApiRouter)
   app.use(Validation.errorMiddleware)
-  app.use(HttpErrorController.handleError)
   app.use(ErrorController.handleApiError)
 }
 
@@ -253,12 +251,10 @@ if (enableWebRouter || notDefined(enableWebRouter)) {
 
   app.use(publicApiRouter) // public API goes with web router for public access
   app.use(Validation.errorMiddleware)
-  app.use(HttpErrorController.handleError)
   app.use(ErrorController.handleApiError)
 
   app.use(webRouter)
   app.use(Validation.errorMiddleware)
-  app.use(HttpErrorController.handleError)
   app.use(ErrorController.handleError)
 }
 

services/web/test/unit/src/Editor/EditorHttpControllerTests.js

@@ -3,7 +3,6 @@ const sinon = require('sinon')
 const { expect } = require('chai')
 const { ObjectId } = require('mongodb')
 const Errors = require('../../../../app/src/Features/Errors/Errors')
-const HttpErrors = require('@overleaf/o-error/http')
 
 const MODULE_PATH = '../../../../app/src/Features/Editor/EditorHttpController'
 
@@ -149,8 +148,7 @@ describe('EditorHttpController', function() {
         '../Project/ProjectEntityUpdateHandler': this
           .ProjectEntityUpdateHandler,
         '../Errors/HttpErrorHandler': this.HttpErrorHandler,
-        '../Errors/Errors': Errors,
-        '@overleaf/o-error/http': HttpErrors
+        '../Errors/Errors': Errors
       }
     })
   })

services/web/test/unit/src/Errors/HttpErrorControllerTests.js

@@ -1,111 +0,0 @@
-const sinon = require('sinon')
-const { expect } = require('chai')
-const modulePath = '../../../../app/src/Features/Errors/HttpErrorController.js'
-const SandboxedModule = require('sandboxed-module')
-const MockResponse = require('../helpers/MockResponse')
-const MockRequest = require('../helpers/MockRequest')
-const Errors = require('../../../../app/src/Features/Errors/Errors')
-const HttpErrors = require('@overleaf/o-error/http')
-
-describe('HttpErrorController', function() {
-  beforeEach(function() {
-    this.req = new MockRequest()
-    this.res = new MockResponse()
-
-    this.AuthenticationController = {
-      getLoggedInUserId: sinon.stub().returns(null)
-    }
-
-    this.logger = {
-      warn: sinon.stub(),
-      error: sinon.stub()
-    }
-
-    this.ErrorController = SandboxedModule.require(modulePath, {
-      globals: { console },
-      requires: {
-        '../Authentication/AuthenticationController': this
-          .AuthenticationController,
-        'logger-sharelatex': this.logger,
-        './Errors': Errors,
-        '@overleaf/o-error/http': HttpErrors
-      }
-    })
-  })
-
-  describe('handleError', function() {
-    beforeEach(function() {})
-
-    it('logs and return status code', function() {
-      let error = new HttpErrors.UnprocessableEntityError()
-
-      this.ErrorController.handleError(error, this.req, this.res)
-      expect(this.res.statusCode).to.equal(422)
-      sinon.assert.calledOnce(this.logger.warn)
-
-      const { url, method, userId } = this.logger.warn.lastCall.args[0]
-      expect(userId).not.to.exist
-      expect(method).not.to.exist
-      expect(url).not.to.exist
-    })
-
-    it('logs url method and userId', function() {
-      let error = new HttpErrors.UnprocessableEntityError()
-      this.AuthenticationController.getLoggedInUserId.returns('123abc')
-      this.req.url = 'overleaf.url'
-      this.req.method = 'GET'
-
-      this.ErrorController.handleError(error, this.req, this.res)
-
-      const { url, method, userId } = this.logger.warn.lastCall.args[0]
-      expect(userId).to.equal('123abc')
-      expect(method).to.equal('GET')
-      expect(url).to.equal('overleaf.url')
-    })
-
-    it('logs and return status code when wrapped', function() {
-      let cause = new Errors.SubscriptionAdminDeletionError()
-      let error = new HttpErrors.UnprocessableEntityError({}).withCause(cause)
-
-      this.ErrorController.handleError(error, this.req, this.res)
-      expect(this.res.statusCode).to.equal(422)
-      sinon.assert.calledOnce(this.logger.warn)
-    })
-
-    it('renders JSON with info', function() {
-      let cause = new Errors.SubscriptionAdminDeletionError({
-        info: {
-          public: { some: 'data' }
-        }
-      })
-      let error = new HttpErrors.UnprocessableEntityError({
-        info: { public: { overwrite: 'data' } }
-      }).withCause(cause)
-      this.req.accepts = () => 'json'
-
-      this.ErrorController.handleError(error, this.req, this.res)
-      expect(this.res.statusCode).to.equal(422)
-      expect(this.res.body).to.equal(
-        JSON.stringify({
-          overwrite: 'data'
-        })
-      )
-    })
-
-    it('renders HTML with info', function() {
-      let cause = new Errors.SubscriptionAdminDeletionError()
-      let error = new HttpErrors.UnprocessableEntityError({
-        info: { public: { message: 'some public message' } }
-      }).withCause(cause)
-      this.req.accepts = () => 'html'
-
-      this.ErrorController.handleError(error, this.req, this.res)
-      expect(this.res.statusCode).to.equal(422)
-      expect(this.res.renderedTemplate).to.equal('general/400')
-      expect(this.res.renderedVariables).to.deep.equal({
-        title: 'Client Error',
-        message: 'some public message'
-      })
-    })
-  })
-})

services/web/test/unit/src/Errors/HttpErrorHandlerTests.js

@@ -23,6 +23,125 @@ describe('HttpErrorHandler', function() {
     })
   })
 
+  describe('handleErrorByStatusCode', function() {
+    it('returns the http status code of 400 errors', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        400
+      )
+      expect(this.res.statusCode).to.equal(400)
+    })
+
+    it('returns the http status code of 500 errors', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        500
+      )
+      expect(this.res.statusCode).to.equal(500)
+    })
+
+    it('returns the http status code of any 5xx error', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        588
+      )
+      expect(this.res.statusCode).to.equal(588)
+    })
+
+    it('returns the http status code of any 4xx error', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        488
+      )
+      expect(this.res.statusCode).to.equal(488)
+    })
+
+    it('returns 500 for http status codes smaller than 400', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        302
+      )
+      expect(this.res.statusCode).to.equal(500)
+    })
+
+    it('returns 500 for http status codes larger than 600', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        302
+      )
+      expect(this.res.statusCode).to.equal(500)
+    })
+
+    it('returns 500 when the error has no http status code', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(this.req, this.res, err)
+      expect(this.res.statusCode).to.equal(500)
+    })
+
+    it('uses the conflict() error handler', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        409
+      )
+      expect(this.res.body).to.equal('conflict')
+    })
+
+    it('uses the forbidden() error handler', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        403
+      )
+      expect(this.res.body).to.equal('restricted')
+    })
+
+    it('uses the notFound() error handler', function() {
+      const err = new Error()
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        404
+      )
+      expect(this.res.body).to.equal('not found')
+    })
+
+    it('uses the unprocessableEntity() error handler', function() {
+      const err = new Error()
+      err.httpStatusCode = 422
+      this.HttpErrorHandler.handleErrorByStatusCode(
+        this.req,
+        this.res,
+        err,
+        422
+      )
+      expect(this.res.body).to.equal('unprocessable entity')
+    })
+  })
+
   describe('badRequest', function() {
     it('returns 400', function() {
       this.HttpErrorHandler.badRequest(this.req, this.res)

services/web/test/unit/src/Project/ProjectControllerTests.js

@@ -2,7 +2,6 @@ const SandboxedModule = require('sandboxed-module')
 const path = require('path')
 const sinon = require('sinon')
 const { expect } = require('chai')
-const HttpErrors = require('@overleaf/o-error/http')
 const { ObjectId } = require('mongodb')
 const Errors = require('../../../../app/src/Features/Errors/Errors')
 
@@ -144,7 +143,6 @@ describe('ProjectController', function() {
           err() {}
         },
         'metrics-sharelatex': this.Metrics,
-        '@overleaf/o-error/http': HttpErrors,
         './ProjectDeleter': this.ProjectDeleter,
         './ProjectDuplicator': this.ProjectDuplicator,
         './ProjectCreationHandler': this.ProjectCreationHandler,

services/web/test/unit/src/Subscription/SubscriptionControllerTests.js

@@ -21,8 +21,6 @@ const modulePath =
   '../../../../app/src/Features/Subscription/SubscriptionController'
 const Errors = require('../../../../app/src/Features/Errors/Errors')
 const SubscriptionErrors = require('../../../../app/src/Features/Subscription/Errors')
-const OError = require('@overleaf/o-error')
-const HttpErrors = require('@overleaf/o-error/http')
 
 const mockSubscriptions = {
   'subscription-123-active': {
@@ -125,8 +123,7 @@ describe('SubscriptionController', function() {
           unprocessableEntity: sinon.stub()
         }),
         '../Errors/Errors': Errors,
-        './Errors': SubscriptionErrors,
-        '@overleaf/o-error/http': HttpErrors
+        './Errors': SubscriptionErrors
       }
     })
 
@@ -450,31 +447,32 @@ describe('SubscriptionController', function() {
       this.SubscriptionHandler.createSubscription.yields(
         new SubscriptionErrors.RecurlyTransactionError({})
       )
-      this.SubscriptionController.createSubscription(this.req, null, error => {
-        expect(error).to.exist
-        expect(error).to.be.instanceof(HttpErrors.UnprocessableEntityError)
-        expect(
-          OError.hasCauseInstanceOf(
-            error,
-            SubscriptionErrors.RecurlyTransactionError
-          )
-        ).to.be.true
-      })
-      return done()
+      this.HttpErrorHandler.unprocessableEntity = sinon.spy(
+        (req, res, message) => {
+          expect(req).to.exist
+          expect(res).to.exist
+          expect(message).to.deep.equal('Unknown transaction error')
+          done()
+        }
+      )
+      this.SubscriptionController.createSubscription(this.req, this.res)
     })
 
     it('should handle validation errors', function(done) {
       this.next = sinon.stub()
       this.LimitationsManager.userHasV1OrV2Subscription.yields(null, false)
       this.SubscriptionHandler.createSubscription.yields(
-        new Errors.InvalidError({})
+        new Errors.InvalidError('invalid error test')
       )
-      this.SubscriptionController.createSubscription(this.req, null, error => {
-        expect(error).to.exist
-        expect(error).to.be.instanceof(HttpErrors.UnprocessableEntityError)
-        expect(OError.hasCauseInstanceOf(error, Errors.InvalidError)).to.be.true
-      })
-      return done()
+      this.HttpErrorHandler.unprocessableEntity = sinon.spy(
+        (req, res, message) => {
+          expect(req).to.exist
+          expect(res).to.exist
+          expect(message).to.deep.equal('invalid error test')
+          done()
+        }
+      )
+      this.SubscriptionController.createSubscription(this.req, this.res)
     })
 
     it('should handle recurly errors', function(done) {

services/web/test/unit/src/User/UserControllerTests.js

@@ -5,7 +5,6 @@ const modulePath = '../../../../app/src/Features/User/UserController.js'
 const SandboxedModule = require('sandboxed-module')
 const OError = require('@overleaf/o-error')
 const Errors = require('../../../../app/src/Features/Errors/Errors')
-const HttpErrors = require('@overleaf/o-error/http')
 
 describe('UserController', function() {
   beforeEach(function() {
@@ -128,7 +127,6 @@ describe('UserController', function() {
         },
         '../Errors/Errors': Errors,
         '@overleaf/o-error': OError,
-        '@overleaf/o-error/http': HttpErrors,
         '../Email/EmailHandler': (this.EmailHandler = {
           sendEmail: sinon.stub(),
           promises: { sendEmail: sinon.stub().resolves() }