Merge pull request #3021 from overleaf/msm-oerror-remove-conflict-error

Replace ConflictError thrown with calls to HttpErrorHandler.conflict()

GitOrigin-RevId: 3b4d98af1b31e49ceab4b1b55b94b8f0323c8a9b

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

@@ -27,6 +27,21 @@ module.exports = {
     }
   },
 
+  conflict(req, res, message, info) {
+    res.status(409)
+    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('conflict')
+    }
+  },
+
   forbidden(req, res, message = 'restricted', info = {}) {
     res.status(403)
     switch (req.accepts(['html', 'json'])) {

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

@@ -237,16 +237,14 @@ const UserController = {
           UserUpdater.changeEmailAddress(userId, newEmail, err => {
             if (err) {
               let errorData = {
-                message: 'problem updaing users email address',
+                message: 'problem updating users email address',
                 info: { userId, newEmail, public: {} }
               }
               if (err instanceof Errors.EmailExistsError) {
-                errorData.info.public.message = req.i18n.translate(
+                const translation = req.i18n.translate(
                   'email_already_registered'
                 )
-                return next(
+                return HttpErrorHandler.conflict(req, res, translation)
-                  new HttpErrors.ConflictError(errorData).withCause(err)
-                )
               } else {
                 errorData.info.public.message = req.i18n.translate(
                   'problem_changing_email_address'

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

@@ -6,6 +6,7 @@ const EmailHelper = require('../Helpers/EmailHelper')
 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) {
@@ -158,31 +159,13 @@ module.exports = UserEmailsController = {
 
   _handleEmailError(error, req, res, next) {
     if (error instanceof Errors.UnconfirmedEmailError) {
-      return next(
+      return HttpErrorHandler.conflict(req, res, 'email must be confirmed')
-        new HttpErrors.ConflictError({
-          info: {
-            public: { message: 'email must be confirmed' }
-          }
-        }).withCause(error)
-      )
     } else if (error instanceof Errors.EmailExistsError) {
-      return next(
+      const message = req.i18n.translate('email_already_registered')
-        new HttpErrors.ConflictError({
+      return HttpErrorHandler.conflict(req, res, message)
-          info: {
-            public: { message: req.i18n.translate('email_already_registered') }
-          }
-        }).withCause(error)
-      )
     } else if (error.message === '422: Email does not belong to university') {
-      return next(
+      const message = req.i18n.translate('email_does_not_belong_to_university')
-        new HttpErrors.ConflictError({
+      return HttpErrorHandler.conflict(req, res, message)
-          info: {
-            public: {
-              message: req.i18n.translate('email_does_not_belong_to_university')
-            }
-          }
-        }).withCause(error)
-      )
     }
     next(new HttpErrors.InternalServerError().withCause(error))
   }

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

@@ -63,6 +63,44 @@ describe('HttpErrorHandler', function() {
     })
   })
 
+  describe('conflict', function() {
+    it('returns 409', function() {
+      this.HttpErrorHandler.conflict(this.req, this.res)
+      expect(this.res.statusCode).to.equal(409)
+    })
+
+    it('should print a message when no content-type is included', function() {
+      this.HttpErrorHandler.conflict(this.req, this.res)
+      expect(this.res.body).to.equal('conflict')
+    })
+
+    it("should render a template including the error message when content-type is 'html'", function() {
+      this.req.accepts = () => 'html'
+      this.HttpErrorHandler.unprocessableEntity(this.req, this.res, 'an error')
+      expect(this.res.renderedTemplate).to.equal('general/400')
+      expect(this.res.renderedVariables).to.deep.equal({
+        title: 'Client Error',
+        message: 'an error'
+      })
+    })
+
+    it("should return a json object when content-type is 'json'", function() {
+      this.req.accepts = () => 'json'
+      this.HttpErrorHandler.unprocessableEntity(
+        this.req,
+        this.res,
+        'an error',
+        {
+          foo: 'bar'
+        }
+      )
+      expect(JSON.parse(this.res.body)).to.deep.equal({
+        message: 'an error',
+        foo: 'bar'
+      })
+    })
+  })
+
   describe('forbidden', function() {
     it('returns 403', function() {
       this.HttpErrorHandler.forbidden(this.req, this.res)

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

@@ -71,6 +71,7 @@ describe('UserController', function() {
     }
     this.SudoModeHandler = { clearSudoMode: sinon.stub() }
     this.HttpErrorHandler = {
+      conflict: sinon.stub(),
       unprocessableEntity: sinon.stub()
     }
     this.UserController = SandboxedModule.require(modulePath, {
@@ -428,6 +429,21 @@ describe('UserController', function() {
         }
         this.UserController.updateUserSettings(this.req, this.res, next)
       })
+
+      it('should call the HTTP conflict error handler when the email already exists', function(done) {
+        this.HttpErrorHandler.conflict = sinon.spy((req, res, message) => {
+          expect(req).to.exist
+          expect(req).to.exist
+          message.should.equal('email_already_registered')
+          done()
+        })
+        this.req.body.email = this.newEmail.toUpperCase()
+        this.UserUpdater.changeEmailAddress.callsArgWith(
+          2,
+          new Errors.EmailExistsError()
+        )
+        this.UserController.updateUserSettings(this.req, this.res)
+      })
     })
 
     describe('when using an external auth source', function() {

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

@@ -6,11 +6,13 @@ const { assert } = chai
 const modulePath = '../../../../app/src/Features/User/UserEmailsController.js'
 const SandboxedModule = require('sandboxed-module')
 const MockRequest = require('../helpers/MockRequest')
+const MockResponse = require('../helpers/MockResponse')
 const Errors = require('../../../../app/src/Features/Errors/Errors')
 
 describe('UserEmailsController', function() {
   beforeEach(function() {
     this.req = new MockRequest()
+    this.res = new MockResponse()
     this.user = { _id: 'mock-user-id' }
 
     this.UserGetter = { getUserFullEmails: sinon.stub() }
@@ -38,6 +40,7 @@ describe('UserEmailsController', function() {
         .withArgs('example.com')
         .resolves({ sso_enabled: false })
     }
+    this.HttpErrorHandler = { conflict: sinon.stub() }
     this.UserEmailsController = SandboxedModule.require(modulePath, {
       globals: {
         console: console
@@ -51,6 +54,7 @@ describe('UserEmailsController', function() {
         '../Helpers/EmailHelper': this.EmailHelper,
         './UserEmailsConfirmationHandler': (this.UserEmailsConfirmationHandler = {}),
         '../Institutions/InstitutionsAPI': this.InstitutionsAPI,
+        '../Errors/HttpErrorHandler': this.HttpErrorHandler,
         '../Errors/Errors': Errors,
         'logger-sharelatex': {
           log() {
@@ -142,6 +146,33 @@ describe('UserEmailsController', function() {
         }
       })
     })
+    it('should call the HTTP conflict handler when the email already exists', function(done) {
+      this.UserUpdater.addEmailAddress.callsArgWith(
+        3,
+        new Errors.EmailExistsError()
+      )
+      this.HttpErrorHandler.conflict = sinon.spy((req, res, message) => {
+        req.should.exist
+        res.should.exist
+        message.should.equal('email_already_registered')
+        done()
+      })
+      this.UserEmailsController.add(this.req, this.res)
+    })
+
+    it("should call the HTTP conflict handler when there's a domain matching error", function(done) {
+      this.UserUpdater.addEmailAddress.callsArgWith(
+        3,
+        new Error('422: Email does not belong to university')
+      )
+      this.HttpErrorHandler.conflict = sinon.spy((req, res, message) => {
+        req.should.exist
+        res.should.exist
+        message.should.equal('email_does_not_belong_to_university')
+        done()
+      })
+      this.UserEmailsController.add(this.req, this.res)
+    })
   })
 
   describe('remove', function() {