From d8d3ac82e98e8b4f06be42f5ff53128a8344f5f9 Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Thu, 9 Jul 2020 15:47:43 +0200 Subject: [PATCH] Replace HTTPErrors.ForbiddenError with calls to forbidden() handler (#2972) GitOrigin-RevId: 2a0c8fdaef9ba62b97cebad84603e6f076d770c0 --- .../Authorization/AuthorizationMiddleware.js | 4 +- .../Collaborators/CollaboratorsController.js | 13 +++-- .../src/Features/Errors/HttpErrorHandler.js | 22 +++++++++ .../UserMembershipMiddleware.js | 3 +- .../AuthorizationMiddlewareTests.js | 24 ++++------ .../CollaboratorsControllerTests.js | 12 +++++ .../unit/src/Errors/HttpErrorHandlerTests.js | 48 +++++++++++++++++++ 7 files changed, 102 insertions(+), 24 deletions(-) create mode 100644 services/web/app/src/Features/Errors/HttpErrorHandler.js create mode 100644 services/web/test/unit/src/Errors/HttpErrorHandlerTests.js diff --git a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js index b4e80a2958..dde580b13a 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js +++ b/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) } ) }) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsController.js b/services/web/app/src/Features/Collaborators/CollaboratorsController.js index 350a20863c..abc53e216f 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsController.js +++ b/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) } diff --git a/services/web/app/src/Features/Errors/HttpErrorHandler.js b/services/web/app/src/Features/Errors/HttpErrorHandler.js new file mode 100644 index 0000000000..92b407ffa5 --- /dev/null +++ b/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') + } + } +} diff --git a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js b/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js index a078fc31e5..84b6995fc1 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js +++ b/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) } } diff --git a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js b/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js index b083ed1662..98ab23e45d 100644 --- a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js +++ b/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 ) }) }) diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js index d4e654fed7..99cde048ce 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js +++ b/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) + }) }) }) diff --git a/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js b/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js new file mode 100644 index 0000000000..8337e5870e --- /dev/null +++ b/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' + }) + }) + }) +})