Merge pull request #1934 from overleaf/ta-error-type-handler

Implement ErrorType Handler

GitOrigin-RevId: 7cd735bb248c74815182e10fd54d687dd35914b8

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

@@ -66,12 +66,12 @@ class ThirdPartyUserNotFoundError extends BackwardCompatibleError {
   }
 }
 
-class SubscriptionAdminDeletionError extends BackwardCompatibleError {
-  constructor(arg) {
-    super(arg)
-    if (!this.message) {
-      this.message = 'subscription admins cannot be deleted'
-    }
+class SubscriptionAdminDeletionError extends OError {
+  constructor(options) {
+    super({
+      message: 'subscription admins cannot be deleted',
+      ...options
+    })
   }
 }
 

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

@@ -0,0 +1,61 @@
+const logger = require('logger-sharelatex')
+const OError = require('@overleaf/o-error')
+const AuthenticationController = require('../Authentication/AuthenticationController')
+const HttpErrors = require('./HttpErrors')
+
+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')
+  } else if (statusCode >= 400 && statusCode < 500) {
+    res.render('general/500', { title: 'Client Error' })
+  } 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/HttpErrors.js

@@ -0,0 +1,23 @@
+const OError = require('@overleaf/o-error')
+
+class HttpError extends OError {
+  constructor(options) {
+    super(options)
+    this.statusCode = options.statusCode || 500
+  }
+}
+
+class UnprocessableEntityError extends HttpError {
+  constructor(options) {
+    super({
+      message: 'Unprocessable Entity',
+      statusCode: 422,
+      ...options
+    })
+  }
+}
+
+module.exports = {
+  HttpError,
+  UnprocessableEntityError
+}

services/web/app/src/Features/User/UserController.js

@@ -29,6 +29,8 @@ const UserUpdater = require('./UserUpdater')
 const SudoModeHandler = require('../SudoMode/SudoModeHandler')
 const settings = require('settings-sharelatex')
 const Errors = require('../Errors/Errors')
+const OError = require('@overleaf/o-error')
+const HttpErrors = require('../Errors/HttpErrors')
 const EmailHandler = require('../Email/EmailHandler')
 
 module.exports = UserController = {
@@ -62,12 +64,24 @@ module.exports = UserController = {
         user_id,
         { deleterUser: user, ipAddress: req.ip },
         function(err) {
-          if (err != null) {
+          if (err) {
+            let errorData = {
+              message: 'error while deleting user account',
+              info: { user_id }
+            }
             if (err instanceof Errors.SubscriptionAdminDeletionError) {
-              return res.status(422).json({ error: err.name })
+              // set info.public.error for JSON response so frontend can display
+              // a specific message
+              errorData.info.public = {
+                error: 'SubscriptionAdminDeletionError'
+              }
+              return next(
+                new HttpErrors.UnprocessableEntityError(errorData).withCause(
+                  err
+                )
+              )
             } else {
-              logger.warn({ user_id }, 'error while deleting user account')
-              return next(err)
+              return next(new OError(errorData).withCause(err))
             }
           }
           const sessionId = req.sessionID

services/web/app/src/Features/User/UserDeleter.js

@@ -90,7 +90,7 @@ async function ensureCanDeleteUser(user) {
       }
 
       if (subscription) {
-        return reject(new Errors.SubscriptionAdminDeletionError())
+        return reject(new Errors.SubscriptionAdminDeletionError({}))
       }
 
       resolve()

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

@@ -50,6 +50,7 @@ const translations = require('translations-sharelatex').setup(Settings.i18n)
 const Modules = require('./Modules')
 
 const ErrorController = require('../Features/Errors/ErrorController')
+const HttpErrorController = require('../Features/Errors/HttpErrorController')
 const UserSessionsManager = require('../Features/User/UserSessionsManager')
 const AuthenticationController = require('../Features/Authentication/AuthenticationController')
 
@@ -232,6 +233,7 @@ const enableApiRouter =
 if (enableApiRouter || notDefined(enableApiRouter)) {
   logger.info('providing api router')
   app.use(privateApiRouter)
+  app.use(HttpErrorController.handleError)
   app.use(ErrorController.handleApiError)
 }
 
@@ -240,8 +242,10 @@ const enableWebRouter =
 if (enableWebRouter || notDefined(enableWebRouter)) {
   logger.info('providing web router')
   app.use(publicApiRouter) // public API goes with web router for public access
+  app.use(HttpErrorController.handleError)
   app.use(ErrorController.handleApiError)
   app.use(webRouter)
+  app.use(HttpErrorController.handleError)
   app.use(ErrorController.handleError)
 }
 

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

@@ -0,0 +1,108 @@
+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('../../../../app/src/Features/Errors/HttpErrors')
+
+describe('HttpErrorController', () => {
+  beforeEach(() => {
+    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,
+        './HttpErrors': HttpErrors
+      }
+    })
+  })
+
+  describe('handleError', () => {
+    beforeEach(() => {})
+
+    it('logs and return status code', () => {
+      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).to.not.be.defined
+      expect(method).to.not.be.defined
+      expect(url).to.not.be.defined
+    })
+
+    it('logs url method and userId', () => {
+      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', () => {
+      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', () => {
+      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', () => {
+      let cause = new Errors.SubscriptionAdminDeletionError()
+      let error = new HttpErrors.UnprocessableEntityError({}).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/500')
+      expect(this.res.renderedVariables).to.deep.equal({
+        title: 'Client Error'
+      })
+    })
+  })
+})

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

@@ -21,7 +21,9 @@ const MockResponse = require('../helpers/MockResponse')
 const MockRequest = require('../helpers/MockRequest')
 const { ObjectId } = require('mongojs')
 const assert = require('assert')
+const OError = require('@overleaf/o-error')
 const Errors = require('../../../../app/src/Features/Errors/Errors')
+const HttpErrors = require('../../../../app/src/Features/Errors/HttpErrors')
 
 describe('UserController', function() {
   beforeEach(function() {
@@ -106,7 +108,8 @@ describe('UserController', function() {
         'metrics-sharelatex': {
           inc() {}
         },
-        '../Errors/Errors': Errors
+        '../Errors/Errors': Errors,
+        '../Errors/HttpErrors': HttpErrors
       }
     })
 
@@ -234,18 +237,15 @@ describe('UserController', function() {
       })
 
       it('should return a json error', function(done) {
-        return this.UserController.tryDeleteUser(this.req, {
-          status(status) {
-            expect(status).to.equal(422)
-            return {
-              json(json) {
-                expect(json.error).to.equal(
-                  Errors.SubscriptionAdminDeletionError.name
-                )
-                return done()
-              }
-            }
-          }
+        this.UserController.tryDeleteUser(this.req, null, error => {
+          expect(error).to.be.instanceof(HttpErrors.UnprocessableEntityError)
+          expect(
+            OError.hasCauseInstanceOf(
+              error,
+              Errors.SubscriptionAdminDeletionError
+            )
+          ).to.be.true
+          done()
         })
       })
     })

services/web/test/unit/src/helpers/MockRequest.js

@@ -19,6 +19,7 @@ class MockRequest {
       }
     }
     this.prototype.route = { path: '' }
+    this.prototype.accepts = () => {}
   }
   param(param) {
     return this.params[param]