Replace HTTPErrors.ForbiddenError with calls to forbidden() handler (#2972)

GitOrigin-RevId: 2a0c8fdaef9ba62b97cebad84603e6f076d770c0

services/web/app/src/Features/Authorization/AuthorizationMiddleware.js

@@ -4,7 +4,7 @@ const async = require('async')
 const logger = require('logger-sharelatex')
 const { ObjectId } = require('mongojs')
 const Errors = require('../Errors/Errors')
-const HttpErrors = require('@overleaf/o-error/http')
+const HttpErrorHandler = require('../Errors/HttpErrorHandler')
 const AuthenticationController = require('../Authentication/AuthenticationController')
 const TokenAccessHandler = require('../TokenAccess/TokenAccessHandler')
 
@@ -208,7 +208,7 @@ module.exports = AuthorizationMiddleware = {
             { userId, projectId },
             'denying user admin access to project'
           )
-          next(new HttpErrors.ForbiddenError({}))
+          HttpErrorHandler.forbidden(req, res)
         }
       )
     })

services/web/app/src/Features/Collaborators/CollaboratorsController.js

@@ -1,5 +1,6 @@
 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')
 const CollaboratorsGetter = require('./CollaboratorsGetter')
@@ -100,13 +101,11 @@ async function transferOwnership(req, res, next) {
         info: { public: { message: `user not found: ${toUserId}` } }
       })
     } else if (err instanceof Errors.UserNotCollaboratorError) {
-      throw new HttpErrors.ForbiddenError({
-        info: {
-          public: {
-            message: `user ${toUserId} should be a collaborator in project ${projectId} prior to ownership transfer`
-          }
-        }
-      })
+      HttpErrorHandler.forbidden(
+        req,
+        res,
+        `user ${toUserId} should be a collaborator in project ${projectId} prior to ownership transfer`
+      )
     } else {
       throw new HttpErrors.InternalServerError({}).withCause(err)
     }

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

@@ -0,0 +1,22 @@
+const logger = require('logger-sharelatex')
+
+module.exports = {
+  forbidden(req, res, message = 'restricted', info = {}) {
+    res.status(403)
+    switch (req.accepts(['html', 'json'])) {
+      case 'html':
+        return res.render('user/restricted', { title: 'restricted' })
+      case 'json':
+        const fullInfo = { message, ...info }
+        if (info.message) {
+          logger.warn(
+            info,
+            `http error info shouldn't contain a 'message' field, will be overridden`
+          )
+        }
+        return res.json(fullInfo)
+      default:
+        return res.send('restricted')
+    }
+  }
+}

services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js

@@ -6,6 +6,7 @@ const UserMembershipHandler = require('./UserMembershipHandler')
 const EntityConfigs = require('./UserMembershipEntityConfigs')
 const Errors = require('../Errors/Errors')
 const HttpErrors = require('@overleaf/o-error/http')
+const HttpErrorHandler = require('../Errors/HttpErrorHandler')
 const TemplatesManager = require('../Templates/TemplatesManager')
 
 // set of middleware arrays or functions that checks user access to an entity
@@ -284,6 +285,6 @@ function allowAccessIfAny(accessFunctions) {
         return next()
       }
     }
-    next(new HttpErrors.ForbiddenError({}))
+    HttpErrorHandler.forbidden(req, res)
   }
 }

services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js

@@ -1,7 +1,6 @@
 const sinon = require('sinon')
 const { expect } = require('chai')
 const SandboxedModule = require('sandboxed-module')
-const HttpErrors = require('@overleaf/o-error/http')
 const Errors = require('../../../../app/src/Features/Errors/Errors.js')
 
 const MODULE_PATH =
@@ -17,6 +16,9 @@ describe('AuthorizationMiddleware', function() {
       isUserLoggedIn: sinon.stub().returns(true)
     }
     this.AuthorizationManager = {}
+    this.HttpErrorHandler = {
+      forbidden: sinon.stub()
+    }
     this.TokenAccessHandler = {
       getRequestToken: sinon.stub().returns(this.token)
     }
@@ -37,7 +39,7 @@ describe('AuthorizationMiddleware', function() {
         mongojs: {
           ObjectId: this.ObjectId
         },
-        '@overleaf/o-error/http': HttpErrors,
+        '../Errors/HttpErrorHandler': this.HttpErrorHandler,
         '../Errors/Errors': Errors,
         '../Authentication/AuthenticationController': this
           .AuthenticationController,
@@ -277,14 +279,11 @@ describe('AuthorizationMiddleware', function() {
             .yields(null, false)
         })
 
-        it('should raise a 403', function(done) {
+        it('should invoke HTTP forbidden error handler', function(done) {
+          this.HttpErrorHandler.forbidden = sinon.spy(() => done())
           this.AuthorizationMiddleware.ensureUserCanAdminProject(
             this.req,
-            this.res,
-            err => {
-              expect(err).to.be.an.instanceof(HttpErrors.ForbiddenError)
-              done()
-            }
+            this.res
           )
         })
       })
@@ -317,14 +316,11 @@ describe('AuthorizationMiddleware', function() {
             .yields(null, false)
         })
 
-        it('should raise a 403', function(done) {
+        it('should invoke HTTP forbidden error handler', function(done) {
+          this.HttpErrorHandler.forbidden = sinon.spy(() => done())
           this.AuthorizationMiddleware.ensureUserCanAdminProject(
             this.req,
-            this.res,
-            err => {
-              expect(err).to.be.an.instanceof(HttpErrors.ForbiddenError)
-              done()
-            }
+            this.res
           )
         })
       })

services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js

@@ -33,6 +33,9 @@ describe('CollaboratorsController', function() {
     this.EditorRealTimeController = {
       emitToRoom: sinon.stub()
     }
+    this.HttpErrorHandler = {
+      forbidden: sinon.stub()
+    }
     this.TagsHandler = {
       promises: {
         removeProjectFromAllTags: sinon.stub().resolves()
@@ -62,6 +65,7 @@ describe('CollaboratorsController', function() {
         './CollaboratorsGetter': this.CollaboratorsGetter,
         './OwnershipTransferHandler': this.OwnershipTransferHandler,
         '../Editor/EditorRealTimeController': this.EditorRealTimeController,
+        '../../Features/Errors/HttpErrorHandler': this.HttpErrorHandler,
         '../Tags/TagsHandler': this.TagsHandler,
         '../Authentication/AuthenticationController': this
           .AuthenticationController,
@@ -276,5 +280,13 @@ describe('CollaboratorsController', function() {
         }
       )
     })
+
+    it('invokes HTTP forbidden error handler if the user is not a collaborator', function(done) {
+      this.HttpErrorHandler.forbidden = sinon.spy(() => done())
+      this.OwnershipTransferHandler.promises.transferOwnership.rejects(
+        new Errors.UserNotCollaboratorError()
+      )
+      this.CollaboratorsController.transferOwnership(this.req, this.res)
+    })
   })
 })

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

@@ -0,0 +1,48 @@
+const { expect } = require('chai')
+const MockResponse = require('../helpers/MockResponse')
+const MockRequest = require('../helpers/MockRequest')
+const SandboxedModule = require('sandboxed-module')
+const modulePath = '../../../../app/src/Features/Errors/HttpErrorHandler.js'
+
+describe('HttpErrorHandler', function() {
+  beforeEach(function() {
+    this.req = new MockRequest()
+    this.res = new MockResponse()
+
+    this.HttpErrorHandler = SandboxedModule.require(modulePath, {
+      globals: { console }
+    })
+  })
+
+  describe('forbidden', function() {
+    it('returns 403', function() {
+      this.HttpErrorHandler.forbidden(this.req, this.res)
+      expect(this.res.statusCode).to.equal(403)
+    })
+
+    it('should print a message when no content-type is included', function() {
+      this.HttpErrorHandler.forbidden(this.req, this.res)
+      expect(this.res.body).to.equal('restricted')
+    })
+
+    it("should render a template when content-type is 'html'", function() {
+      this.req.accepts = () => 'html'
+      this.HttpErrorHandler.forbidden(this.req, this.res)
+      expect(this.res.renderedTemplate).to.equal('user/restricted')
+      expect(this.res.renderedVariables).to.deep.equal({
+        title: 'restricted'
+      })
+    })
+
+    it("should return a json object when content-type is 'json'", function() {
+      this.req.accepts = () => 'json'
+      this.HttpErrorHandler.forbidden(this.req, this.res, 'an error', {
+        foo: 'bar'
+      })
+      expect(JSON.parse(this.res.body)).to.deep.equal({
+        message: 'an error',
+        foo: 'bar'
+      })
+    })
+  })
+})